Skip to content

fix: green the four pre-existing CI failures on nplez1/main - #10

Merged
nplez1 merged 4 commits into
nplez1/mainfrom
fix/fork-red-test-failures
Sep 19, 2026
Merged

nplez1 merged 4 commits into
nplez1/mainfrom
fix/fork-red-test-failures

Conversation

@nplez1

@nplez1 nplez1 commented Sep 19, 2026 •

Copy link
Copy Markdown
Owner
Files Added Deleted Net
Test 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​23 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​22
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​8 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​8

ELI5

Four areas of CI were already red on nplez1/main before this branch existed — three test shards and the packaging census. Every one of them has a distinct, reproducible cause; none is a flaky test. This branch fixes all four, so the next PR on this fork can go green.

What Changed

Before: every PR Checks run failed static analysis (from #8 onward), test shards 4/8, 5/8 and 7/8, and the aggregate verify gate — 13 tests in four files. PR #7 was merged with that red CI; the same 13 tests, in the same four files, failed there before this branch existed.

After: all four files pass, locally on macOS and (for the one platform-dependent case) in a run that forces process.platform = 'linux', which is what CI uses.

file tests cause fix
src/main/ai-vault/session-scanner-service-cancellation.test.ts 4 the fork's index-first scan path calls sessionSearch.listSessions(...), but the upstream test's hand-rolled SessionScannerServiceSearch double only has handles() and close(), so every request threw and settled before the cancel arrived the double answers null, which is its "no index here yet" answer — the scanner fallback the cases are about
src/main/rate-limits/codex-fetcher-probe-shutdown.test.ts 1 since the macOS resolver probe moved off the main thread it spawns /usr/sbin/scutil through the same node:child_process these cases mock, and that spawn lands during a later case's spawn tally the diagnostic is stubbed to its no-hint path; the hint is orthogonal to probe shutdown and home-lock ownership
src/renderer/src/components/settings/AccountsPane.copilot-credentials.test.tsx 5 the pane mounts CodexLoginLinkNotice, which subscribes via window.api.codexAccounts.onPendingLoginUrlChanged, and the hand-rolled api double only carried codexAccounts.list the double honours the subscription contract and publishes no link
config/scripts/mobile-web-bundle-packaging-workflow-contract.test.mjs 3 fork-release.yml packs mac and Windows without install-mobile-dependencies, which upstream's packaging sweep added to every packaging job two days after this fork's workflow was written both jobs install them right after the root install, as every other packaging workflow does, and the census's expected-job list names the two jobs

Nothing here changes product behaviour: three of the four are test-double or assertion drift, and the fourth is a CI workflow that had fallen behind an upstream invariant.

Why

  • Why fix the tests and not the product in the first three. Each subject is code this fork deliberately ships: the index-first scan path is local fork work (LOCAL-PATCHES.md names "the indexed list" and the service query/refresh protocol as staying), the off-main-thread resolver probe is upstream's, and the Codex sign-in link notice is upstream's. The tests were the stale side in every case — the mocks and counters had drifted from the interfaces they stand in for.
  • Why stub the diagnostic instead of only counting codex spawns. The stray /usr/sbin/scutil spawn also consumes the mock's mockReturnValueOnce queue, so a filtered counter would still hand the wrong child to the wrong probe. Stubbing the module removes the count and the queue hazard.
  • Why the workflow got the step rather than an exemption. That census exists so "a new packaging workflow has to be added here deliberately, with its bundle step, rather than slipping in"; an exemption would delete the invariant instead of meeting it.
  • Considered and rejected: skipping or deleting the failing cases (each encodes an invariant worth keeping — cancellation ownership, one probe per running home, credential-source presentation, packaging coverage); rewinding fork-release.yml to a pre-sweep shape (it would still pack without the mobile bundle step); building a fuller fake search class than these cases need (the console of it is the null return).

Linked Issue

None to link: issues are disabled on nplez1/orca, and there is no matching upstream issue. These four fixes were requested directly, as follow-up to the CI failures observed on #8.

Visual Proof

N/A — no product UI or behaviour change. Three of the four commits touch only test files and the fourth touches only a CI workflow, so there is nothing to render before or after.

Testing

Ran:

  • The four files together: 57 tests pass.
  • Widened: src/main/ai-vault, src/main/rate-limits, src/renderer/src/components/settings and the packaging contract — 403 files, 3,461 tests pass.
  • Every workflow/CI contract test in config/scripts (this change edits a workflow, and a dozen of those contracts crawl all of .github/workflows): 208 files, 1,885 tests pass.
  • The whole static analysis battery CI runs: audit:anti-slop, audit:code-quality:native, check:code-quality:changed against the base commit, check:react-doctor:changed, check:zustand-selector-fanout, check:dead-classes, check:max-lines-ratchet, check:ts-nocheck-ratchet, check:runtime-electron-ratchet, check:reliability-gates.
  • pnpm tc — full typecheck across every project.
  • Platform check for the rate-limits case, since its failure was platform-dependent: it passes on macOS (where the stray spawn lands in the previous case) and in a copy of the file that forces process.platform = 'linux' (where it landed in the failing case, as it does on CI).

Not run: the full repo-wide pnpm test (sharded across eight CI jobs), the rest of pnpm lint, pnpm build, and the Electron e2e suite. fork-release.yml has never been dispatched from this worktree — the workflow change is validated by the packaging census and by YAML parsing only, not by a release run.

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

AI Disclosure

Written with the pi coding agent (model: deepseek-flash). Each failure was reproduced locally, its cause traced to a specific commit or interface drift, and the fix re-run on both macOS and a Linux-forced copy before committing.

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Security: no product code changes. The workflow step runs the existing install-mobile-dependencies composite action with --frozen-lockfile, which pr.yml already runs in four jobs, and its drift guard compares the mobile lockfiles to the workspace files.
  • Cross-platform: the rate-limits fix is specifically about platform divergence — it removes a darwin-only spawn from a counter, so the case now behaves the same on Linux CI and macOS. The other three are platform-independent.
  • Worth knowing about the workflow fix: install-mobile-dependencies is currently a forward-looking invariant rather than a repair of a failing release. src/mobile-web is still a 2.5 KB placeholder with no mobile imports (pnpm run build:mobile-web succeeds here without mobile/node_modules), so the fork's releases were not failing at the bundle step. The step removes the census failure and aligns the fork's release path with upstream's rule before the bundle grows into it.
  • SSH / remote, mobile, persisted state, wire formats: untouched. No protocol, schema or stored value changes, so there is nothing here for an older client or a paired host to disagree with.
  • Performance: no product impact. The workflow step adds one frozen install per release job.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason — N/A, no UI change
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass — ran typecheck, the four suites, the widened suites, every CI contract test and the static-analysis battery; full repo-wide test, full lint, build and e2e were not run, see Testing

…ice calls

The fork's index-first scan patch made session-scanner-service-entry call
sessionSearch.listSessions(...) before falling back to the scanner, but the
test's hand-rolled SessionScannerServiceSearch double still only had handles()
and close(). Every request therefore threw before it could be cancelled, so
each case failed on its first cancelAndObserveSet with 'Expected an admitted
cancellation.' Null is the double's 'no index here' answer, which is the
scanner fallback these cases are about.
… count

Since the macOS resolver probe moved off the main thread it spawns
/usr/sbin/scutil through node:child_process, which these cases mock and count
as probe spawns. The spawn lands after the case that triggered it — darwin-only
on macOS, and via the case that forces darwin on Linux CI — so the tally broke
in a different case on each platform. The hint is orthogonal to probe shutdown
and home-lock ownership, so the diagnostic is stubbed to its no-hint path.
…ne mounts

AccountsPane renders CodexLoginLinkNotice, which subscribes through
window.api.codexAccounts.onPendingLoginUrlChanged on mount. The Copilot cases
install a hand-rolled api double that only carried codexAccounts.list, so the
pane threw on mount before any credential assertion ran. The double now honours
the subscription contract and publishes no link.
Upstream's packaging census fails any job that packs without
install-mobile-dependencies, and fork-release.yml predates that sweep
(2026-09-15 vs 2026-09-18), so the contract test failed on both of its jobs.
Both now install them right after the root install, as every other packaging
workflow does, and the census's expected-job list names them so a future
packaging job still has to be added deliberately.
@nplez1
nplez1 merged commit 46cba88 into nplez1/main Sep 19, 2026
54 of 56 checks passed
nplez1 pushed a commit that referenced this pull request Oct 1, 2026
stablyai#23968)

* test: retire native-chat and sidebar cases their fixtures or predicates neutralize

Semantic sweep of renderer sidebar, right-sidebar and native-chat. 10 cases
removed across 6 files; one test-only production re-export removed.

Two new shapes here, both of which make a case unable to fail:

A fixture that neutralizes its own variation. `NativeChatNoticeRow`'s
`old-reader compatibility` table had six rows, but the test's own
`oldStatusSchema.omit({tone, presentation})` strips the metadata being varied, so
all six render the identical body `{kind:'status', text:'…'}`. Its
schema-acceptance half is owned case-for-case at
`agent-session-journal-schemas.test.ts:310-318`.

A fixture that makes the distinction its name claims impossible. "Counts a new
turn from its stamp, not a row held open by background work" set `turnStartedAt`
and `mainAgent.stateStartedAt` to the same `now - 5_000`, so it could not tell the
two links apart; `native-chat-terminal-turn.test.ts:75` owns the real version.

Also removed: two cases varying `stateHistory`, which no native-chat production
file reads (`NativeChatHookTurnEntry` omits it); the `agent-session` row of a
transport table whose guard is `valueSource === 'dispatched' && transport ===
'catalog'`, so under `reported` the transport is never consulted — the `catalog`
row stays as the only test of the non-dispatched arm; `openclaude` from an
`it.each` where the only agent test on that path is `args.agent === 'codex'`, so
both agents take the identical arm.

Production change: `NativeChatMessageList.tsx` no longer re-exports
`ProviderFrameRow`. That export existed only for
`NativeChatMessageList.provider-frame.test.tsx`; both production callers
(`NativeChatNoticeRow.tsx:9`, `NativeChatMessageRow.tsx:24`) already import it
from `NativeChatTranscriptChrome` directly, so the test now does too.

Kept where the call site differs though the assertions match: a dispatched-pill
tooltip case and a transport-hedge case hit `NativeChatSessionOptionPickers.tsx:278`
(model pill) versus `:302` (options pill). Kept as live branches: the
`lifecycle === null || lifecycle === 'published'` disjuncts, the four distinct
wirings in `native-chat-reader-scroll-input.ts:33-46`, and a ring case that is the
only one reaching the module's own keydown listener, because the sibling's click
path already runs `dropPendingHover`.

* test: delete the assembler/id-merge differential, which cannot fail

Reverses my own restore of `native-chat-assembler-merge-parity.test.ts`. I kept it
believing it was a live cross-implementation differential oracle; it is not.

`native-chat-session-assembler.ts:233` documents the gate: the `turnKey` fallback
"only merges a candidate against an existing message of a DIFFERENT source (#10)".
The test passes `sources: { transcript }` — a single source — so that fallback is
gated off, the assembler reduces to id-dedup-and-append, and it equals the mobile
id-only merge by construction. The assertion is identity === identity.

Both contracts it nominally covered are owned directly, by tests asserting concrete
values rather than comparing two implementations that degenerate to the same
operation: identical same-source prompts surviving at
`native-chat-session-assembler.test.ts:328` and `:354` (both citing #10), and
id-append order at `mobile/src/session/mobile-native-chat-merge.test.ts:18`.

The lesson generalizes: a differential is only worth keeping if the two
implementations can actually diverge on the fixture used. Check that the fixture
reaches the code that differs.
nplez1 pushed a commit that referenced this pull request Oct 19, 2026
…-page readers off the deprecated owner resolver (stablyai#30478)

Thirteen read-routing callers (#4, #5, #10-#20 of the round-3 caller list) read the owner through
runtimeEnvironmentIdWithLegacyFallbackUnlessTwin. A workspace two hosts publish is no longer
searched on either host. The focus-read ratchet now counts the legacy-fallback adapter, which
reads the focused runtime inside an object literal its discovery skipped; the five F1 entries
that blind spot pruned come back. Deprecated importers 53 -> 40.
nplez1 pushed a commit that referenced this pull request Oct 21, 2026
…win projections stay on their host (review-5 #2 #9 #11) (stablyai#30890)

* fix(renderer): host writes route only through the strict owner, and twin projections stay on their host (review-5 #2 #9 #11)

A selected ssh: host that two paired servers proxy now resolves ambiguous at the one
selected-host resolver, so no caller can fall back to this app's own SSH connection.
File Explorer and SSH mutation expectations derive the full {endpoint, place} pair from
resolveOwner, so a server folder's nested SSH target stays the write's host. A local
lineage write projects onto its own host's rows only. The id-only store-write ratchet
now also covers persistence repo-row lookups by id alone (review-5 #10's shape).

* fix(ratchet): the id-only lookup scan also sees optional chaining and destructured ids

* fix(renderer): an unstamped folder keeps reading its host from its repos; editor focus fallback left for a follow-up

* docs(renderer): the File Explorer owner comment names its folder fallback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant