Agent-friendly constraints plan #877

Clustering summary

Recurring agent mistakes this period are not random review nits. They are a few ways a fixed clock, a platform probe, or a teardown race reports the wrong failure. The highest-leverage fixes are APIs and fixtures that make that wrong report unreachable. Lint and CI cover reintroduction. Prose is only the residue a check cannot see.

Scan, and the denominators used below:

• 80 most recently merged PRs (gh pr list --state merged --limit 80), 2026-10-04 through 2026-10-16. 40 recently closed PRs overlap that set; test: fix #829 flakes at the source and cut fixed test waits #855, fix(web): show image tool results instead of empty success #874, and fix(web): keep provider failures visible after the run goes idle #875 were still open when scanned.
• Requested items Flaky CI tests: session-history E2E anchor, Windows git-info refresh, Windows setup-config lock timeout #829, test: fix root causes of two flaky tests (#829) #831, test: fix #829 flakes at the source and cut fixed test waits #855, test: close the five highest-value gaps from the test audit #857, Add contributor-only test-value skill #860, fix: stop three CI flakes from treating clocks as readiness #861, fix(setup): use a heartbeat lease for the config lock #863, fix(terminal): lay out sustained output incrementally #869, fix(web): keep streaming updates independent of history length #870, fix(web): bound session catalog and history read cost #871, fix(web): re-render only the live transcript row while streaming #873, fix(web): show image tool results instead of empty success #874, fix(web): keep provider failures visible after the run goes idle #875: PR or issue body, commits, and GraphQL reviewThreads(first: 100).
• 163 inline review comments from GET /repos/openpi-dev/openpi/pulls/comments (updated descending, paginated). Newest is 2026-10-12, oldest 2026-08-24.
• 100 newest main workflow runs: 68 success, 24 cancelled, 8 failure. ci.yml sets concurrency.cancel-in-progress: true, so the cancellations are superseded runs, not flakes.
• git log --since=2026-09-15 is 563 commits, plus git log -S for cscript, //B, and unrouteAll.

The Oct 14–16 PRs in the requested set have zero review threads and do not appear in the 163 inline comments. This week's evidence is commit messages, PR bodies, and CI logs.

Of the 8 red main runs, two are Pages (unpinned actions on 37187211113; environment protection on 37187193606). On 38736671833 and 37883190349, the Node and Background terminals jobs are gate steps after UI_RESULT=failure (job logs read). They are not extra product failures.

#869, #870, #871, #873, #874, and #875 are product or cost fixes and do not add another pitfall class. #860 lands the test-value skill, which already says retries, wider timeouts, and refreshed snapshots must not conceal a regression.

Three main failures in the 100 runs did not repeat often enough to cluster: a spinner frame mismatch (⠹ vs ⠸) on 37185879432, git-info process output -1 !== 7 on 37172625519, and a Windows EBUSY rmdir of a temp fixture on 36023380925 (same run as the git-info cold start).

A local prototype on unpushed scratch/agent-constraints-prototypes (fd8b808, not a PR) checked the mechanisms below. It is not part of this repo's main.

Plan

1. Drain Playwright routes in one fixture

• Category: teardown racing an in-flight route handler (Response has been disposed).
• Evidence: 947e36e on #863. The PR body also cites run 38774705615 (log not re-downloaded here). Open #855 added the same unrouteAll({ behavior: "wait" }) drain. tests/web/openpi-web.e2e.ts now drains in afterEach. The prototype listed 23 other route files with no unrouteAll at all, including session-history.e2e.ts, settings-parity.e2e.ts, and workbench-polish.e2e.ts. tests/web/playwright.config.ts has no global drain.
• Constraint: One Playwright fixture owns afterEach and calls page.unrouteAll({ behavior: "wait" }) unless the page is already closed. A CI grep fails any tests/web/*.e2e.ts that calls page.route( or context.route( and does not contain unrouteAll. The prototype's grep matched those 23 files and ignored a comment-only //B style false pattern for the sibling check.
• Pyramid: codebase (fixture), with a CI grep so a new spec cannot skip it.
• Effort: M.
• Risk: A handler that never finishes hangs the teardown. The existing comment in openpi-web.e2e.ts is the right bound: wait for in-flight handlers, and still fail the test if the handler throws for any other reason. Do not raise the test timeout to hide a stuck route.

2. Keep process liveness on the heartbeat, and ban //B

• Category: per-platform process queries. cscript //B swallowed a JScript syntax error, so a broken WMI query looked like empty output and the lock waited out 5s. The same PR then chased UTF-16 and a PowerShell cold start.
• Evidence: five commits on #857: 297e581 (drop PowerShell for cscript), 0d693e0 (UTF-16), b03b1df (PowerShell fallback), ea256c6 (WMI only), fe57ee4 (//B hid the syntax error; args were ["//Nologo", "//B", script, pid]). #863 deletes the probe and uses a heartbeat lease (#862). #829 records the Windows lock timeout on run 38558753554. 2 of 80 merged PRs.
• Constraint: Liveness stays the lock-file heartbeat (futimes on the holder's fd). A CI grep fails a string literal "//B" or '//B' anywhere under extensions/ and scripts/. The prototype matched line 1285 of fe57ee4^:extensions/shared/setup-config.ts, matched nothing in current setup-config.ts, and did not match a comment or a URL that merely contains the characters //B.
• Pyramid: codebase (the heartbeat is already the API). CI grep is the reintroduction stop. A custom lint is not a better layer than that grep.
• Effort: S.
• Risk: Low. The literal form is the footgun. A comment that says "//B" inside extensions/ would fail the grep; that is an acceptable false positive.

3. Read UI that publishes on a timer from a check-phase helper

• Category: findByRole / waitFor and the product timer share a phase, so a stalled Windows runner reports a missing README or burns the test budget. Sync getByRole then fails before React commits. The rejected fix was a larger timeout.
• Evidence: 2f6bd28 on #863 adds paintedRole in tests/web/workbar-session-position.spec.ts. #861 cites composer-context-navigation timing out on run 38850034958. Main UI log 37883190349: Unable to find role="button" and name "README.md" and Test timed out in 20000ms in workbar-session-position.spec.ts. 36881409881: session-sidebar-actions.spec.ts:376 timed out in 5000ms. Open #855 proposed asyncUtilTimeout: 5_000 and testTimeout: 15_000. That bump is not on main (git log -S asyncUtilTimeout is empty). 2 of 80 merged PRs, plus test: fix #829 flakes at the source and cut fixed test waits #855.
• Constraint: One helper in the UI test setup: flush a check-phase turn (act plus a macrotask), then queryByRole, and call getByRole only after the existing 1s budget so absence still fails. Specs that follow a timer-published list or review snapshot use that helper instead of findByRole / waitFor. Do not raise testTimeout or Testing Library's async timeout for this class.
• Pyramid: test helper. A lint that bans getByRole or findByRole will false-positive on nodes that are already committed. That choice stays prose, and the test-value skill already says a wider timeout does not supply a missing phase.
• Effort: M.
• Risk: Medium. A helper that always waits 1s slows the suite if callers use it for synchronous nodes. Keep the fast path (queryByRole on the first turn) and do not import it into tests that are not waiting on a timer publish.

4. Arm a deadline only after its precondition

• Category: a fixed budget that includes process start. The sandbox armed its 1s non-yielding-script watchdog before the child was listening, so a slow node --permission spawn was reported as Script execution timed out. The git-info fixture copied node.exe to git.exe and timed the first, cold exec against the 3s product budget.
• Evidence: 20a7b98 on #861. The PR body cites run 38848560632. Current extensions/workflows/sandbox.ts arms the watchdog inside beginExecution, which runs on { kind: "listening" }. Git cold start: 8ed7d566 on #831, and the same assertion on main run 36023380925 (Timed out waiting for local git info, 6436ms, tests/extensions/git-info/index.test.ts). #829 cites the later Windows git-info run 38550635790. 2 of 80 merged PRs.
• Constraint: The sandbox shape is the invariant: spawn, wait until the child says it is listening, then arm the script budget. A copied-binary fixture launches once untimed before any assertion clock. There is no shared type that expresses "this number starts later" for arbitrary setTimeout calls.
• Pyramid: codebase for the sandbox (landed). Test helper for cold executables (landed in that one file). A new watchdog that does not use this order cannot be banned by a lint without a false-positive flood.
• Effort: S for a regression that the listening message precedes armWatchdog (the sandbox test from fix: stop three CI flakes from treating clocks as readiness #861 is the start). No new abstraction.
• Risk: Low if the work stops at the regression. Extracting a generic armAfterReady helper would be unused ceremony.
• Prose residue: "A deadline starts when the thing it measures has started." The test-value skill should keep that sentence only if a later change shows agents still arm timers early. It is not enforceable, and this plan does not edit the skill.

5. Separate a canonical path from display text

• Category: a directory-name substring, a Windows 8.3 path, or sanitized display text used as the file identity.
• Evidence: 924d651 and e3df017 on #857. scripts/provenance.mjs now compares realpathSync.native and the package name, not a folder named openpi. Review on #705 (2026-10-09, the only October inline review in the 163 that is this class): sanitizeTerminalText was applied to path / resolvedPath, so the preview opened the cleaned string. 2 distinct PRs. The other 22 "path" review comments in the 163 are different defects and are not counted here.
• Constraint: evaluateOpenPiProvenance stays on normalizePath. For tool evidence, keep the original path bytes for open/read and put the sanitized string in a display field only (the fix: sanitize hidden terminal strings in web evidence #705 follow-up already states that split). A branded CanonicalPath is worth it only at the open/read boundary, where a string display field cannot be passed without an explicit conversion.
• Pyramid: type at the file-open boundary. Provenance is already an API. A repo-wide "no includes(basename)" lint will flag legitimate substring checks.
• Effort: M.
• Risk: Medium. Branding every path in the protocol is a wide type change. Limit it to the function that opens a user-visible path. Do not brand internal joins.

6. Reject empty retrigger commits; do not treat a wider timeout as a fix

• Category: a required check goes green because the job was rerun, an empty commit retriggered CI, or a timeout was raised.
• Evidence: empty non-merge commits since 2026-10-01 whose subject matches retry/rerun/retrigger: 99c8621 on #707 (ci: retry flaky validation matrix), 3be3d0b on #718 (chore: rerun CI for web rerun confirmation fix), 5bf446c on #857 (UI timeout; the message says rerunning failed jobs was not permitted). #829 says the three flakes passed when only the failed jobs were rerun. #855 is the timeout-widening attempt and is unmerged. Current tree has no test.retry, vitest retry:, or --retry outside product "retry" copy. 3 of 80 merged PRs, plus Flaky CI tests: session-history E2E anchor, Windows git-info refresh, Windows setup-config lock timeout #829 and test: fix #829 flakes at the source and cut fixed test waits #855.
• Constraint: CI fails a push whose commit has an empty git diff-tree and a subject matching retry|rerun|retrigger. The prototype flagged those three SHAs and did not flag 708a44b (#871). It does not ban a commit that touches a comment in order to get a new SHA. There is no vitest retry to delete.
• Pyramid: CI check for the empty-commit shape. The rest is prose: the test-value skill already forbids hiding a regression behind retries, wider tolerances, or a rerun. A two-number timeout type does not help (item 7).
• Effort: S.
• Risk: Low for the empty-commit check. It will not catch a one-line comment retrigger. Do not fail every empty merge commit; those are naturally empty and were the majority of empty diff-tree results since October.

7. Drag-then-click and shared SSE budgets: do not add a type

• Category: drag suppression used a 250ms wall clock, so a slow post-drop render let the trailing click through. Separately, one timeoutMs is both the first-SSE deadline and the lifetime deadline, both measured from attempt start, so a 5ms test budget loses to a 12ms runner delay and reports first SSE event.
• Evidence: Drag: 20a7b98 and current SessionSidebar.tsx (suppressDragClick is a flag cleared by the trailing click or a later pointerdown, not a timer). The regression in session-sidebar.spec.ts waits 300ms and still expects no toggleWorkspace. Main run 38736671833 failed session-sidebar.spec.ts:265 because toggleWorkspace was called once. #861 cites the same flake on run 38850838416. SSE: e0d6bcd on #857. provider.ts still sets both deadlines from attemptStartedAt. Production first-event defaults are 60s and 300s, so this is a test-clock bug, not the Windows lock bug. The prototype showed { firstEventMs: 5, idleMs: 5 } still passes a positive-number check, and arming the idle clock at the first event still returns first-event when the first read is already 12ms late. A frozen clock, then a jump of one budget, returns lifetime. That is what withSseClock already does.
• Constraint: Leave the drag flag and its 300ms regression. Leave withSseClock as the way the lifetime test moves time. Do not add a two-field timeout type. Do not widen the harness timeout.
• Pyramid: already landed (flag, regression, test clock). A type is the wrong layer; the prototype is the evidence.
• Effort: S, and the work is "do not undo this," not a new subsystem.
• Risk: Low. A future test can pass timeoutMs: 5 on a real clock and flake again.
• Prose residue: that future test. The skill already tells an agent not to paper over it with a longer timeout. A lint for the literal timeoutMs: 5 is too narrow to keep.

What stays prose

Enforceable work is items 1–6. These are not enforceable, and the test-value skill already covers the last two:

• Whether a committed node should be read with getByRole or the paint helper.
• A retrigger commit that is non-empty.
• A new watchdog written as a raw setTimeout before a spawn.
• A new test that uses a tiny real timeoutMs as if it were a phase selector.

No AGENTS.md addition is proposed. The skill sentence is enough for that residue, and this issue does not change the skill.