Skip to content

perf(planning): cut the journey suite's shell and page waits - #11847

Merged
kyle-sexton merged 5 commits into
mainfrom
perf/8757-journey-apply
Nov 1, 2026
Merged

kyle-sexton merged 5 commits into
mainfrom
perf/8757-journey-apply

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Refs: #8757

Summary

Problem: surface-journey.test.sh runs over the 120 s goal on main: the four whole-suite runs after 312a9f2 took 129, 111, 99 and 135 s (median 120.0 s). This PR cuts time the shell and the page spent waiting, and fixes a race in phase 13 that showed up locally as an intermittent failure.

Fix

  • The journey's 24 applies and 5 handles run python -m round from the surface directory instead of round.sh, so Python reuses cached bytecode instead of recompiling the 3,521-line round.py on every call (about 40 ms each). round.sh stays covered by the suite's other calls.
  • The sleep 1 before note n3 waits only until the next whole second, which still gives n3 a later time than n2 (notes carry whole-second times).
  • Phase 2 dispatches visibilitychange so the second tab reads /api/state at once instead of waiting for its 3 s poll; the poll path stays covered by the read before it.
  • Phase 13 closed the Activity panel with a click that timed out at 2 s on every run (the l press had already closed it); it now clicks only when the panel is open.
  • Phase 13's queued-toast check could read the toast after a state push had emptied it; it now records every text the toast shows and passes on the queued text. The product behavior is filed as fix(planning): keep the Notes send toast across a state push #11843.

Verification

  • surface-journey.test.sh locally: four runs after the change all PASS=254 FAIL=0; per-phase totals went from 67/71/66 s to 58/61/62 s, and the shell steps from about 4.9 s to about 3.2 s per run. Local timing is not CI timing.
  • Left out: CI's whole-suite median. perf(test): cut the local affected-test gate's shell suites to a budget #8757 closes only once main's median measures under 120 s after this merges, so this PR refers to it rather than closing it.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 4 commits November 1, 2026 01:14
Each journey apply went through round.sh, which probes Python and then
runs round.py as a script: Python never caches a script's bytecode, so
every call recompiled the 3521-line file (about 40 ms here) on top of
the bash and probe launches. japply and jhandle now run `python -m round`
from the surface directory, which reuses round's cached bytecode; the
command, its arguments and the data dir are unchanged, so every phase
sees the same writes in the same batches. The other round.sh calls in
the suite still cover the wrapper.

The pause before the n3 Notes reply now waits for the next whole second
instead of a full second. Notes carry whole-second times, so n3 still
gets a later time than n2.

Measured on this host (WSL2):
- Replay of the 24 journey applies, server running, no browser:
  round.sh 2951/2780/2761 ms, python -m round 1628/1985/1662 ms.
- Full suite before: apply 2507ms/24, handle 527ms/5, sleep 1002ms/1.
- Full suite after (two runs): apply 2459ms/24 and 1891ms/24,
  handle 533ms/5 and 405ms/5, sleep 338ms/1 and 570ms/1.

Refs: #8757

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The change only touches the surface test, so it releases nothing, as
the earlier #8757 test fragment did.

Refs: #8757

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s toast race

Timing every page call per phase showed settle already returns as soon
as the page has caught up (1 of about 160 calls ran to its cap, while
the server was stopped). Two waits ran long on the happy path:

- Phase 13 pressed l, which closes the Activity panel phase 10 left
  open, then clicked #flyClose with a 2 s timeout that expired every
  run. It now clicks #flyClose only when the press opened the panel;
  the panel ends closed either way, as before.
- Phase 2's third second-tab read waited for the next 3 s /api/state
  poll. A visibilitychange on the visible tab now reads /api/state at
  once (catchUp in index.html). The second read still comes from the
  poll, so the polling path stays covered.

Phase 13's "queued, Claude is busy" check could race: the note toast
is set after the send, and the next state frame re-renders the Notes
panel (focus is on Send note, not the textarea), emptying the toast.
A MutationObserver now records every text the toast shows, and the
check passes only when one of them says Queued, Claude is busy.

Per-phase seconds summed over phases 1-20, this host:
- before: 67, 71, 66 (phase 2: 10, 11, 10; phase 13: 5, 9, 5)
- after:  58, 61, 62 (phase 2: 7, 7, 7; phase 13: 3, 3, 3)
All three after runs: PASS=254 FAIL=0 SKIP=0.

Refs: #8757

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s check

The phase-13 toast recorder observed the whole page body and was never
disconnected, so it ran on every DOM change through phase 20 of the same
page. It now disconnects right after the queued-toast check.

Verified: surface-journey.test.sh PASS=254 FAIL=0.

Refs: #8757

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 1, 2026 05:16
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-01T05:17:41.312152Z 9612dd7 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9612dd7cb0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed c3dcae13a4de09668a5911f7706250130dad36f4: read 1 of 1 changed file(s); 0 finding(s), 0 as inline comments.

Security review: incremental (9612dd7 → c3dcae1)

Scope: I read the whole incremental change. It touches one file, plugins/planning/surface/surface-checks.sh, in one hunk. Every other file in this pull request is unchanged since the last review.

What changed: One wait in the journey test now reads EPOCHREALTIME with a ${EPOCHREALTIME:-} default. Bash 4 doesn't define that variable, so under set -u the old read would have failed. The microsecond fraction is now taken from that local value. If the variable is missing or the fraction isn't a valid six-digit value, the existing regex guard keeps the wait at 1 second, which is the fallback the comment describes. The arithmetic 10#$us runs only after the value matches ^[0-9]{6}$, so nothing outside that pattern reaches it.

Security assessment: No security issues found. The change is limited to a test helper's timing and touches no security-relevant code:

  • It handles no tokens, secrets or credentials.
  • No untrusted input reaches it. The value comes from a shell built-in, and the regex checks it before it's used in arithmetic or passed to sleep.
  • It changes no trust control, workflow, merge hold, plugin manifest or standing instruction. None of the trusted-path globs match this file.

critical-findings: 0

@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed c3dcae13a4de09668a5911f7706250130dad36f4: read 1 of 1 changed file(s); 0 finding(s), 0 as inline comments.

Code review (incremental: 9612dd7 → c3dcae1)

Scope: I reviewed the one hunk in plugins/planning/surface/surface-checks.sh that changed since the last review of this lane. The other files in this PR haven't changed since 9612dd7 and were reviewed then.

What changed: the wait before note n3 used to read ${EPOCHREALTIME#*.} directly. The file runs under set -u (line 17), so on bash 4, which has no EPOCHREALTIME, that line stops the script with an "unbound variable" error. The new code reads ${EPOCHREALTIME:-} into now first, then takes the fraction from it. The read at lines 347 and 350 already uses this pattern.

Checks (by reading the code, not by running it):

  • No EPOCHREALTIME: now is empty, so us is empty. The ^[0-9]{6}$ test fails and wait stays at 1, which keeps the old full-second sleep as intended.
  • Locale that writes a decimal comma (bash 5 can output 1730000000,123456): ${now#*.} finds no . and returns the whole value. That doesn't match six digits, so the wait falls back to 1 s safely.
  • Normal case: the sleep runs to the next whole-second boundary, so n3 gets a later whole-second time than n2. That matches the PR description's claim. The 000000 guard prevents a 1.000000 sleep being written as 0.1000000, and 10#$us stops leading zeros from being read as octal.
  • local ... us declares us without a value and assigns it on the next line before any read, so set -u doesn't trip.

Findings: none.

Closing issue: the PR says Refs: #8757 and closes no issue, so there are no acceptance criteria to check.

Holds: none. The change only affects test timing and spends nothing.

… EPOCHREALTIME

The next-second wait before note n3 read ${EPOCHREALTIME#*.} under set -u,
so on bash 4, which has no EPOCHREALTIME, the suite stopped with an unbound
variable before phase 16. It now reads ${EPOCHREALTIME:-} first and waits
the full second when the variable is missing, as jtime already does.

Refs: #8757

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton

Copy link
Copy Markdown
Member Author

Lane 5 arm request for the hub. Hub sessions exit before a lane can reply over the socket, so I'm posting it here. Please arm at c3dcae13a4de09668a5911f7706250130dad36f4 with arm-when-verified.sh 11847 c3dcae13a4de09668a5911f7706250130dad36f4.

Gate state:

  • ready for review, no labels, merge state clean
  • ci-status green
  • both claude lanes report 0 findings on this head
  • the Codex thread is fixed (c3dcae1) and resolved
  • pr-verify is pending; the script waits for it

Also, in answer to your 05:3xZ question: PRs #11820 and #11821 are not Lane 5's.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit 6b9b267 Nov 1, 2026
48 checks passed
@kyle-sexton
kyle-sexton deleted the perf/8757-journey-apply branch November 1, 2026 06:49
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