Budget the editor-handoff pause sleep, and note why main looked green without it

[Hmbown]

One line of JSON, plus something about the gate that is worth knowing.

No-Issue: ratchet bookkeeping for a site #6165 landed deliberately; the underlying convention is #6149.

What

#6239 (which closed #6165) added a bounded thread::sleep to
tui/ui/terminal_input.rs::pause_for_child_terminal. The blocking-calls ratchet
has no entry for that file, so it now reports:

crates/tui/src/tui/ui/terminal_input.rs: thread_sleep sites 1 > budget 0

This adds the one entry. Nothing else in the file changes.

Why budgeted and not rewritten

This is the case check-blocking-calls-budget.py's own message names — "raise
the budget with --update if the site can only run on synchronous code" — and
its author had already written the justification at the call site:

a bounded retry, capped by TERMINAL_INPUT_CHILD_PAUSE_TIMEOUT, in a
synchronous API whose caller is about to block this very thread on a
foreground editor for as long as the user keeps it open. tokio::time is not
reachable from here and would not change what the thread does.

That reads correctly to me. The caller is about to hand the terminal to
$EDITOR and block until the user quits it; a few polled milliseconds before
that is not what parks the thread. The sibling sleep at :192 is already inside
a thread::Builder::spawn, which is why the scanner never counted it.

The part worth reading

Main is currently green on this, and that is not evidence.

The budget steps are gated on if: needs.changes.outputs.heavy == 'true'. On
main's run for e8a226552 all four budget steps report skipped:

skipped  Check dead-code budget
skipped  Check blocking-calls budget
skipped  Check runtime-contract budget
skipped  Check persistence-backlog budget

Combined with continue-on-error on pull requests, a violation can cross a PR
advisory-only and then land on a main push that never runs the check. That is
exactly the path this took: green PR, green main, real violation, fires on
whatever heavy push comes next — at which point it looks like that unrelated
push's fault.

I am not changing the gating here; narrowing heavy or making these steps
unconditional is a CI-cost tradeoff someone should make deliberately. But the
current shape means a green main does not tell you the ratchets passed, and
that seemed worth stating where someone will read it.

Evidence

check-blocking-calls-budget.py   625 sites across 181 files, within budget

Diff is one added object:

+  "crates/tui/src/tui/ui/terminal_input.rs": {
+    "thread_sleep": 1
+  },

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 1m 52s —— View job

---

Review complete

• Read PR context and diff
• Inspect terminal_input.rs::pause_for_child_terminal and the sibling sleep at :192
• Verify check-blocking-calls-budget.py scanner behavior (why one site is counted, the other isn't)
• Verify the CI gating claim (heavy condition + continue-on-error)
• Post review findings

Summary

This is a correctly-scoped, data-only ratchet update: one JSON entry, no code changes. I traced the scanner logic by hand against the file and the numbers check out.

Verified correct:

• crates/tui/src/tui/ui/terminal_input.rs:132 (thread::sleep(TERMINAL_INPUT_CHILD_PAUSE_POLL_INTERVAL)) sits in a plain fn scope (pause_terminal_input_for_child), not inside spawn_blocking/thread::Builder/test — so check-blocking-calls-budget.py's scope-stack scanner counts it once. That matches the added "thread_sleep": 1 entry.
• The sibling sleep at :192 is inside the closure passed to thread::Builder::new().spawn(...) (line 185), which pushes a "blocking" scope onto the scanner's stack — correctly excluded, as the PR description says.
• Alphabetical placement is correct (provider_setup.rs < terminal_input.rs < tests.rs), matching the script's sort_keys=True output.
• The CI-gating claim checks out against .github/workflows/ci.yml: the blocking-calls-budget step is gated on needs.changes.outputs.heavy == 'true' (line 435) and uses continue-on-error: ${{ github.event_name == 'pull_request' }} (line 436). A push whose changed paths don't hit the heavy classifier skips the step entirely; a PR run makes it advisory-only. Also confirmed scripts/check-blocking-calls-budget.json itself isn't in the light-path allowlist in the changes job, so this PR's own run is classified heavy and will actually exercise the check (as the pasted "within budget" evidence shows) rather than skip it.
• No provider/model/route logic, no reuse/dead-code concern, no security surface — appropriately no new tests for a pure ratchet-bookkeeping change, consistent with the repo's "don't add tests by default" guidance.

One nit (description only, not the diff): the PR body attributes the budgeted sleep to pause_for_child_terminal (terminal_input.rs:302), but that function is async and already awaits tokio::time::sleep (line 320) — it has no thread::sleep and needs no budget entry. The quoted justification comment and the actual thread::sleep call are in the similarly-named but distinct synchronous function pause_terminal_input_for_child (terminal_input.rs:108-135). Doesn't affect correctness of the shipped JSON change, but worth fixing in the PR description so a future reader matching function name to call site doesn't get pointed at the wrong (and already-correct) async function.

[codewhale-agent]

Codewhale review

Complete review coverage: 1/1 passes, 1 file patches, sha256:e3aac133bcff3abde64e4ff3d74310f4763537031748db482abc0ecc2b2d3fd8.

Pass 1: The PR adds a single budget entry to scripts/check-blocking-calls-budget.json for crates/tui/src/tui/ui/terminal_input.rs with thread_sleep = 1. The hunk is syntactically valid JSON, correctly indented (2 spaces for the key, 4 for the inner count), and inserted in the file's existing alphabetical position between provider_setup.rs and tests.rs. No other content changes. The change is minimal and matches the scanner's reported violation.

Findings

• [INFO] Verify the key is not already present elsewhere in the budget file (scripts/check-blocking-calls-budget.json:454)
  The diff shows only the insertion context around line 451, so a pre-existing entry for "crates/tui/src/tui/ui/terminal_input.rs" elsewhere in the JSON cannot be ruled out from the patch alone. JSON parsers silently keep the last duplicate key while the generator (--update) may emit only one, so a duplicate would produce confusing future diffs and a budget that does not match what the author intended. Since the file is sorted alphabetically, a duplicate should be adjacent (or would break the ordering assumption), which makes this cheap to confirm with a quick grep before merge.
• [INFO] This file is not validated by CI on main, so the entry cannot be machine-verified here (scripts/check-blocking-calls-budget.json)
  As the PR description states, the four budget steps are gated on needs.changes.outputs.heavy == 'true' and were skipped on main's run for e8a2265; combined with continue-on-error on pull requests, a malformed or wrong-count entry can land without ever being exercised by check-blocking-calls-budget.py. The author explicitly declares this gating change out of scope, which is reasonable, but it means the only verification for this diff is manual: the key must byte-match the path printed by the scanner and the count must equal the site count the scanner reports for that file. Worth confirming locally with the script rather than relying on the green main signal.
• [INFO] Rationale for the budgeted exception lives only outside the repository (scripts/check-blocking-calls-budget.json:455)
  The justification quoted in the PR (bounded retry capped by TERMINAL_INPUT_CHILD_PAUSE_TIMEOUT, synchronous API whose caller blocks the thread on a foreground editor) is the reason the budget is raised instead of the call being rewritten, but it will not be discoverable from the budget file, which is plain JSON with no comment support. A reader investigating later why this file carries a non-zero thread_sleep budget will have to dig through git history to find it. Nothing to fix in this diff, but noting it so the tradeoff is at least acknowledged.

Assessment

Pass 1: Correct and minimal. The added object is valid JSON, uses the file's existing 2/4-space indentation, and is placed in the correct alphabetical slot (provider_setup.rs < terminal_input.rs < tests.rs), so it will not churn under a future --update. The budgeted value of 1 matches the single site the scanner reports, and the sibling sleep at terminal_input.rs:192 is inside a thread::Builder::spawn so it is legitimately uncounted. The main risk is not in the patch but in its verification path: because the budget steps are gated behind the heavy output and are advisory-only on PRs, this entry is not actually exercised by CI, so the path/key match and absence of a duplicate key should be confirmed manually. Approve once that is checked.

---

Advisory review by Codewhale (codewhale review --pr 6243 --post, head fbf715889e0763ac7f5ad126e5c20e66ea6f71f2). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

[codewhale-agent]

[INFO] Verify the key is not already present elsewhere in the budget file

The diff shows only the insertion context around line 451, so a pre-existing entry for "crates/tui/src/tui/ui/terminal_input.rs" elsewhere in the JSON cannot be ruled out from the patch alone. JSON parsers silently keep the last duplicate key while the generator (--update) may emit only one, so a duplicate would produce confusing future diffs and a budget that does not match what the author intended. Since the file is sorted alphabetically, a duplicate should be adjacent (or would break the ordering assumption), which makes this cheap to confirm with a quick grep before merge.

[codewhale-agent]

[INFO] Rationale for the budgeted exception lives only outside the repository

The justification quoted in the PR (bounded retry capped by TERMINAL_INPUT_CHILD_PAUSE_TIMEOUT, synchronous API whose caller blocks the thread on a foreground editor) is the reason the budget is raised instead of the call being rewritten, but it will not be discoverable from the budget file, which is plain JSON with no comment support. A reader investigating later why this file carries a non-zero thread_sleep budget will have to dig through git history to find it. Nothing to fix in this diff, but noting it so the tradeoff is at least acknowledged.

[Hmbown]

A second, larger blind spot in this same gate — partly confirmed, partly not

(Reposted: the first version of this comment lost several filenames to shell backtick expansion.)

A multi-agent review of the guard surface flagged scripts/check-blocking-calls-budget.py for a defect beyond the one this PR patches. I verified the mechanism and could not reproduce the headline number, so both are below.

Confirmed: the pattern anchors on the literal std::fs::

"std_fs": re.compile(
    r"\bstd::fs::(?:read|read_to_string|write|create_dir|…)\b"
),

A file that does use std::fs; and then calls fs::read_to_string(...) — the more idiomatic of the two spellings — is invisible to this gate. Same call, same blocking syscall, not counted.

Measured on origin/main (raw occurrences, no scope filtering):

std::fs:: sites the gate can see : 3253
bare  fs:: sites it cannot see   :  476  across 390 files

Largest uncounted: crates/tui/src/runtime_api/tests.rs 298, crates/tui/src/config/tests.rs 296, crates/tui/src/project_context.rs 184, crates/tui/src/session_manager.rs 154, crates/tui/src/plugins/tests.rs 121, crates/config/src/tests.rs 120.

Not confirmed: "the gate sees 45% of sites"

The review reported 624 recorded sites against 1403 under a widened pattern. I could not reproduce that ratio. The two figures measure different things — the budget's 624 is after scope filtering (tests, spawn_blocking and dedicated-thread scopes are excluded), while a raw pattern count is not. My raw numbers put the blind spot nearer 13% of occurrences, and a large share of those sit in tests.rs files the gate deliberately ignores anyway.

So: the blind spot is real, its size is unconfirmed, and the honest production-only number is the one nobody has measured yet. Widening the regex and re-running --update would produce it in one command; I did not do that here because it would bury a one-line fix under a re-baseline.

Why it belongs in its own issue, not this PR

Widening the pattern moves the budget number for reasons unrelated to any code change. That deserves its own commit whose message says the number rose because the measurement got honest, not because the tree got worse. Folding it in here would make this PR's one-line intent unreviewable.

It is also another instance of the class in #6242 — a gate that asserts a spelling rather than a property. That issue is the archetype; this is the third confirmed instance, after the dead-code ratchet (#6241) and the skipped-steps problem described in this PR's own description.

One fact for whoever picks it up: there are zero use tokio::fs sites in the tree, so a widened pattern needs no import-gating to avoid false positives.