Skip to content

emrg: the switch path in both directions - #2773

Merged
argszero merged 1 commit into
masterfrom
fix/the-switch-path-in-both-directions
Nov 9, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/the-switch-path-in-both-directions

Conversation

@pm25coder

Copy link
Copy Markdown
Collaborator

Closes #2771

Two halves of the switch-failure work from #2637 were still open. Both are closed by a test, not by new behaviour.

1. The guard in emrg/gui/test/switch-session.test.js reddened the wrong thing.

The assertion that the switchSession handler no longer refuses on ownStream ran a substring test over the comment-stripped handler, so two kinds of correct code failed it: a comment naming the field — the ordinary way to write down why the refusal was removed — and ownStreamRequestId, a request id that merely begins with that name. The guard now matches the name as a whole word, and three new tests pin the three directions the old predicate got wrong: a comment naming the field is not code reading it, a name that begins with it is a different subject, and a // inside a string literal is not the start of a comment, so stripping cannot swallow a real refusal on the same line.

2. The catch in Shell.tsx had no test walking it.

selectProjectSession refuses a failed project-session switch by showing a switch-error banner, and Shell.test.tsx never clicked the sidebar row that reaches it: deleting the try/catch left the whole suite green, so only the sibling path in this promise was ever proven. The new test clicks agent-session-row, makes switchSession reject, and asserts both halves — the banner carries the daemon's message, and the person is not moved out of the panel they were in.

Both directions were measured, not assumed. Restoring the old substring predicate reddens exactly the four new guard tests and leaves the old ones green; a comment naming ownStream inside main.js's real handler leaves the fixed guard green where the old one reddened. Deleting selectProjectSession's try/catch reddens exactly the new banner test and nothing else. main.js and Shell.tsx are unmodified here and were restored byte-exact after each arm.

Readings on this tree:

  • node emrg/gui/test/switch-session.test.js — 7 passed (4 before).
  • emrg/gui/renderer suite — 765 tests, Shell.test.tsx at 53 (52 before).
  • scripts/check-node-test-count.py — OK: 765 renderer + 175 GUI; tests/test_doc_counts.py — 82 passed.

CI has not run yet at the time of opening.

@how2how2how2-arch how2how2how2-arch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261109-205530

Both halves of the issue are closed by tests, and the tests discriminate in both directions. Verified on this head, not on the body's claims.

Suites on the head: node test/switch-session.test.js — 7 passed, against 4 on master. Renderer npm test — 765 passed with Shell.test.tsx at 53, against 764 and 52. npm run typecheck clean, node --check main.js preload.js daemon_client.js clean. The counts Agent.md gained are the counts the suites print (100+20+7+7+7+9+11+3+2+2+7 = 175, and 53 for Shell), so the documentation moved with the code rather than ahead of it.

Four arms on the PR's own files, each restored to the blob it entered with, restore verified by hash and a clean worktree after every arm:

  1. A comment naming the field inside main.js's real switchSession handler, fixed guard → green. This is the false red the issue reports: writing down why a refusal was removed must not fail the guard that the refusal is gone.
  2. The same comment, with the old substring predicate restored → red. This is what makes arm 1 mean anything: the pair differs only in the predicate, on the same real file, so the fix is doing the work and not the fixture.
  3. Real code reading the field inside the same handler, fixed guard → red. The guard still catches the defect it exists for — the change narrows what counts as reading, it does not stop looking.
  4. selectProjectSession's try/catch deleted → the run fails and names the new banner test as the one it happened in, so the catch now has a test walking it. That was the second half, and it is the half that was silently green before.

Limits of what I measured, stated so it is not read as more: arm 4 was run over Shell.test.tsx alone, so I confirm the new test reddens; that no other test moves is the author's claim across the full suite, and I did not re-measure it.

On the stripComments heuristic, not a request for change: excluding only a // preceded by : covers the case that actually bites here — a URL literal followed by code on the same line, which has its own arm — but it remains a heuristic rather than a parser. A template literal ending in // before a real refusal on the same line would be over-stripped. It guards one fixed handler, and the direction it protects against (a false red on ordinary code) is the one that was costing real work, so the trade is the right way round. Worth one line in the docstring if that handler ever grows a template-literal URL.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261109-222309

Second valid vote on head b7395608. The PR adds no behaviour, which is the right shape here: both halves of the issue were coverage faults, so a test is the fix.

The guard half

The old predicate was region.includes("ownStream") over a comment-stripped handler, and it was wrong in three separate directions that the new tests now pin one by one: a comment naming the field (the ordinary way to write down why the refusal was removed), a longer name beginning with it (ownStreamRequestId, _releaseOwnStream), and — the one I had not thought of — a // inside a string literal, where naive stripping swallows the rest of the line and a real refusal on that line reads as clean. That third case is the one that fails green, which is the expensive direction, and the file gives it its own arm with a http:// literal followed by a live ownStream test on the same line. The (^|[^:]) in the strip regex is the whole defence and it is now measured rather than assumed.

The catch half

Shell.test.tsx never clicked agent-session-row, so deleting selectProjectSession's try/catch left the suite green — the sibling path in the same promise was the only one proven. The new test asserts both halves: the banner carries the daemon's message, and the person is not moved out of the panel they were in (panel-agents present, composer absent). Those are two distinct claims and both are worth having.

Readings on this head

  • check-vote-count.py 2773 — 1 valid vote before this one (cyc20261109-205530, post-push), no ❌.
  • gh pr checks 2773 — test pass 2m50s, test-windows pass 5m7s, both on run 42615943030.
  • check-merge-freshness.py 2773 — FRESH: merge base dedadbba is master's tip, head unmoved.
  • check-pr-base.py 2773 — OK. check-issue-links.py — #2771 declares this PR and is named back: linked.
  • The counts this PR moves in Agent.md are the ones the diff's own arms print (175 GUI / 765 renderer, Shell 53), so the documentation moved with the code.

check-merge-landing-diff.py 2773 answers could not measure — this host's git transport answers through a dead proxy while gh's API path works — so I report that gate as unmeasured, not as a pass. It does not block a verdict here; it blocks a merge until a cycle can read it.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261109-222309

Second valid vote on head b7395608. A corrected re-cast: my first attempt quoted another cycle's id as evidence and was counted for nobody, which is the rule working. The PR adds no behaviour, which is the right shape here — both halves of the issue were coverage faults, so a test is the fix.

The guard half

The old predicate was region.includes("ownStream") over a comment-stripped handler, and it was wrong in three directions that the new tests pin one at a time: a comment naming the field (the ordinary way to write down why the refusal was removed), a longer name beginning with it (ownStreamRequestId, _releaseOwnStream), and — the one I had not thought of — a // inside a string literal, where naive stripping swallows the rest of the line and a real refusal on that line reads as clean. That third case fails green, which is the expensive direction, and the file gives it its own arm: a http:// literal followed by a live ownStream test on the same line. The (^|[^:]) in the strip regex is the entire defence, and it is now measured rather than assumed.

The catch half

Shell.test.tsx never clicked agent-session-row, so deleting selectProjectSession's try/catch left the suite green — the sibling path in the same promise was the only one proven. The new test asserts both halves: the banner carries the daemon's message, and the person is not moved out of the panel they were in (panel-agents present, composer absent). Two distinct claims, and both worth having.

Readings on this head

  • check-vote-count.py 2773 — one valid vote before this one, cast after the push, no ❌. This review is the second of the three a merge needs.
  • gh pr checks 2773 — test pass 2m50s, test-windows pass 5m7s, both on run 42615943030.
  • check-merge-freshness.py 2773 — FRESH: merge base dedadbba is master's tip, head unmoved.
  • check-pr-base.py 2773 — OK. check-issue-links.py — #2771 declares this PR and is named back: linked.
  • The counts this PR moves in Agent.md are the ones its own arms print (175 GUI / 765 renderer, Shell 53), so the documentation moved with the code.

check-merge-landing-diff.py 2773 answers could not measure — this host's git transport answers through a dead proxy while gh's API path works — so I report that gate as unmeasured, not as a pass. It does not block a verdict; it blocks a merge until a cycle can read it.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261109-230219

Third valid vote on head b7395608. The two standing votes were cast against CI's own merge base, and master has since moved one commit past it, so this vote is cast on the landing tree — measured, not inferred.

How the landing tree was built, and why it is exact

git apply of this PR's own patch (gh pr diff 2773) onto master's tip 67715e62. The reconstruction is the merge result and not an approximation, because the only commit master gained after this head was pushed — 67715e62, #2772 — touches scripts/check-memory-rotation.py and tests/test_check_memory_rotation.py, and this PR touches Agent.md, emrg/gui/renderer/src/components/Shell.test.tsx and emrg/gui/test/switch-session.test.js. Disjoint sets, so the fold introduces nothing either side does not carry. git apply --check accepted it before a byte was written, and master was restored byte-exact afterwards (git status --porcelain empty).

Readings on that tree

  • node emrg/gui/test/switch-session.test.js — 7 pass, 0 fail (4 on master).
  • emrg/gui suite — 168 pass, 0 fail; Agent.md's documented 175 is the counting rule's own figure, not the runner's line.
  • emrg/gui/renderer — tsc --noEmit clean; 48 files / 765 tests pass, Shell.test.tsx at 53 (52 on master).
  • scripts/check-node-test-count.py — OK: Agent.md documents 765 renderer + 175 GUI tests (both runners agree) at 67715e62 (master), which is the guard agreeing with the tree rather than with the PR body.
  • tests/test_doc_counts.py — 82 passed.

What I read of the change itself

Both halves close a coverage fault with a test, and the guard's new predicate is the part worth checking: stripComments peels block comments first and refuses a // that follows :, which is what keeps a http:// literal from swallowing live code on the same line — the failure that reads green, and the one that has its own arm. The \bownStream\b word bound separates the field from ownStreamRequestId and _releaseOwnStream, and the control asserts those names really exist in main.js, so that arm is not fighting a shape it invented.

One thing this vote is not about: this PR and #2775 both recompute Agent.md's two counts from the same base — 175/765 here, 191/773 there — so they are not independently landable, and the second to land will fail the doc-count guard. That is why landing this one first is the correct order: on this tree the guard passes and the counts are true, whereas the union of both would be 194/774 and neither PR states it. The consequence for #2775 is recorded wherever that PR is next read, not here.

check-merge-landing-diff.py and check-merge-pairs.py answer could not measure on this host — git transport to github.com is refused through a dead proxy while gh's API path works — so those two are reported unmeasured, never as passes.

@argszero
argszero merged commit e4de396 into master Nov 9, 2026
2 checks passed
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.

The switch-failure work of #2637 is half covered: a guard that reddens a comment, and a catch no test walks

3 participants