Skip to content

test(subagent): widen three short approval bounds to 30 s - #6966

Open
Lstarsky0 wants to merge 2 commits into
codewhale-hq:mainfrom
Lstarsky0:test/child-approval-bounds-share-ceiling
Open

Lstarsky0 wants to merge 2 commits into
codewhale-hq:mainfrom
Lstarsky0:test/child-approval-bounds-share-ceiling

Conversation

@Lstarsky0

@Lstarsky0 Lstarsky0 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What and why

child_permission_gate::ask_without_a_durable_receipt_store_fails_closed_before_prompting gives registry.execute 250 ms to refuse the call. On windows-latest it has overrun that four times since 10-11, three of them on main:

a missing receipt store must fail closed without waiting for a decision: Elapsed(())

#6698 moved this module's approval waits to one 30 s CHILD_APPROVAL_EVENT_READINESS after a loaded run overran the 2 s and 5 s ceilings. Three bounds in the module stayed on private values:

  • the 250 ms one above,
  • the 2 s wait for the prompt in child_prompt_carries_agent_scoped_keys,
  • the 1 s wait for the wait-end event once the host drains.

The two event waits now use the shared ceiling. The 250 ms one waits for registry.execute to refuse the call and asserts that no event is sent, which is not what the constant's comment describes, so it keeps a bound of its own and that becomes 30 s. (The first commit put all three on the constant; the second takes this one back off.)

Each still ends as soon as the call returns or the event arrives, and still fails the test when it never does. Only the 250 ms bound has been seen to fail. The other two are the same shape, so I moved them in the same change.

Issue

Refs #6698

How I tested it

  • To reproduce it locally I ran the test in a loop at the lowest priority next to 60 busy loops at the same priority. With the 250 ms bound, 48 of 80 runs failed with the Elapsed(()) above. With the 30 s bound, none of 138 runs failed under the same load (80 on the first commit, 58 on the second). On an idle machine both pass.
  • The three touched tests still finish in about 0.1 s each in a normal run; the ceiling only matters when something is stuck.
  • cargo fmt --all -- --check, cargo clippy -p codewhale-tui --all-targets --all-features with CI's flags, and cargo check -p codewhale-tui --all-targets: clean. Clippy also on 1.99, plus cargo +1.89.0 check -p codewhale-tui.
  • cargo nextest run -p codewhale-tui --lib: 15242 passed, 44 failed. The same 44 fail on main (35b1128) on my machine; most need an extension-host Node version I don't have.

No new tests; this changes three bounds in existing ones.

Checklist

  • One focused change, rebased on current main
  • cargo fmt --all passes, and tests cover new or changed behavior
  • If this adds or changes a user-facing feature, I updated its docs and its row in docs/features.toml (cargo test -p codewhale-tui --test feature_registry checks it) — n/a

…s ceiling

ask_without_a_durable_receipt_store_fails_closed_before_prompting gives
registry.execute 250 ms to refuse the call. On windows-latest it has
overrun that four times since 10-11 (runs 38123604545, 38146297151,
38213790167 and 38234571127; three of them on main), each time with

    a missing receipt store must fail closed without waiting for a decision: Elapsed(())

codewhale-hq#6698 moved this module's approval waits to one 30 s
CHILD_APPROVAL_EVENT_READINESS after a loaded run overran the 2 s and 5 s
ceilings. Three bounds in the module stayed on private values: this
250 ms one, the 2 s wait for the prompt in
child_prompt_carries_agent_scoped_keys, and the 1 s wait for the wait-end
event once the host drains. All three now use the shared ceiling.

Each still ends as soon as the call returns or the event arrives, and
still fails the test when it never does. Only the 250 ms bound has been
seen to fail.

Signed-off-by: Lstarsky0 <59827030+Lstarsky0@users.noreply.github.com>
@Lstarsky0
Lstarsky0 requested a review from Hmbown as a code owner October 12, 2026 15:37

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions github-actions Bot added the contribution-gate Author not yet in .github/APPROVED_CONTRIBUTORS; a maintainer grants access with /lgtm label Oct 12, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 12, 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-10-12T15:41:19.542653Z 9f975da PR opened
ℹ️ 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: 9f975da412

ℹ️ 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".

Comment thread crates/tui/src/tools/subagent/tests.rs Outdated

let result = tokio::time::timeout(
std::time::Duration::from_millis(250),
CHILD_APPROVAL_EVENT_READINESS,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Update the shared timeout's documented scope

This use has no approval prompt or wait-end event to deliver—the next assertion requires rx to stay empty—so it makes the constant's declaration at lines 23998-24001 (which describes only host-event delivery and says the wait ends on a matching event) false. Future changes to the shared 30-second value can no longer tell whether they are tuning event readiness or a fail-closed execution watchdog; either keep a separate execution bound or update the constant's name and documentation to cover this third use.

AGENTS.md reference: AGENTS.md:L196-L197

Useful? React with 👍 / 👎.

CHILD_APPROVAL_EVENT_READINESS is documented as the ceiling for a prompt
or wait-end event reaching hosts, and as ending on the first matching
event. ask_without_a_durable_receipt_store_fails_closed_before_prompting
waits for registry.execute to refuse the call and asserts that no event
was sent, so putting it on that constant made the comment wrong.

The test goes back to a bound of its own, 30 s instead of the original
250 ms. The other two waits are for a prompt and for a wait-end event and
stay on the shared ceiling.

Signed-off-by: Lstarsky0 <59827030+Lstarsky0@users.noreply.github.com>
@Lstarsky0 Lstarsky0 changed the title test(subagent): three private approval bounds use the shared readiness ceiling test(subagent): widen three short approval bounds to 30 s Oct 12, 2026

@Hmbown Hmbown 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.

thank you so much - this is great and helpful!!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution-gate Author not yet in .github/APPROVED_CONTRIBUTORS; a maintainer grants access with /lgtm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants