[test] Catch agent turns that end with no answer and no error - #6113
Open
mmabrouk wants to merge 3 commits into
Open
[test] Catch agent turns that end with no answer and no error#6113mmabrouk wants to merge 3 commits into
mmabrouk wants to merge 3 commits into
Conversation
…nd the release gate A turn whose model call fails comes back as a clean empty end_turn: the runner returns ok:true with no output, records it as a completed turn, and the user sees a blank bubble with no error anywhere (ASD-EST100). Nothing in the suites caught that shape, so these tests pin it at every layer it passes through. Runner: silent-turn-contract.test.ts drives runSandboxAgent through a fake harness wired to the REAL otel run, so emptiness is decided by the code under test rather than by a canned output string. It asserts the envelope (ok/error), the emitted event order (an error must precede the terminal done), and the continuity store (a failed turn must not be recorded as the session's last good turn). daytona-transcript-recovery.test.ts stands a Pi transcript up inside a fake remote sandbox served over the daemon file API. SDK: test_vercel_stream_silent_failure.py pins the SSE half — a terminal-only ok:false must carry the caller's own reason, never the generic no-output text. Release gate: check_no_silent_turn makes a bare turn an automatic FAIL there. Seven tests use it.fails to pin contracts that are not implemented yet (five await 'fail-loud empty turn', two await 'Daytona swallowed-error reader'); they turn red when those fixes land, which is the signal to drop the marker. Because it.fails is satisfied by any failure, including a fixture that dies before running a turn, each is backed by a guard test proving its setup reaches a real turn — and every fail-loud assertion is paired with a parked/answered turn that must keep succeeding, so neither side can be satisfied by suppressing the other.
…d close four review gaps Review found the invariant was defined, tested, and called by nobody: the gate still PASSed a completely bare turn, which is worse than no check because the coverage docs implied it was covered. Wired `check_no_silent_turn` into the ten cells that hold turns and whose PASS depends on something NOT appearing — a turn that produced nothing satisfies those by doing nothing at all. Each folds it in as `and not silent["violations"]` and reports the violations in its `why`. matrix_w5 passes only its post-interrupt turns, since its first turn is interrupted on purpose and legitimately ends bare. Documented in SKILL.md and in a new cross-cutting invariants section in coverage.md, both telling cell authors to add the conjunct. Aligned the content definition with the product's own `content_parts_emitted` (the Vercel egress): the check now reads the frame stream, so a turn whose only output is a file, an attachment delivery, or any data payload is no longer a false violation. Reasoning stays excluded, matching the adapter — a turn that only thought renders as a blank bubble and is exactly what this catches. The frame list is a deny-list on purpose: data-<name> payloads are open-ended, so an allow-list would fail healthy turns, while a deny-list can only under-report. Two test-integrity fixes. The tool-call expectation is `it.fails`, so its in-body check that a tool call arrived would have been satisfied by a fixture that stopped emitting one, leaving it 'expected fail' forever; that check now also lives in an external guard test. And a run without an explicit cwd now gets its own mkdtemp directory instead of a fixed shared /tmp path, where a stray Pi transcript could have flipped the empty-turn expectations and looked exactly like the fix landing.
…they stand in for Three review follow-ups, all about fixtures that agreed with themselves instead of with the code they imitate. The fake sandbox's runProcess returned a bare exit code, but the real contract answers on stdout (sandboxRelayHost.list in src/tools/relay.ts reads String(ls?.stdout ?? '')). A Daytona reader that finds the transcript by LISTING the directory would have got an empty listing, so both Daytona expectations would have stayed green straight through the fix that is supposed to close them. The fake now answers an ls with the transcript filename, and the test header says the expectations are neutral about how the reader locates the file. Pi's transcript format was hand-rolled in two places — this util and sandbox-agent-pi-error.test.ts. Two copies of a format drift together and both stop matching what Pi writes, with nothing to notice. Both now encode through one shared builder. The SSE tests asserted the generic failure message was absent by matching its prose, which goes vacuously true the moment anyone rewords it. They assert the structural failure code instead, which is what the file already did on the positive side. Not deduplicated: the SandboxAgentDeps fake that mirrors sandbox-agent-orchestration.test.ts. Its fake stubs createOtel with a recording run object so its tests can assert on recorded calls, while this one wires the REAL otel because the event-order and banner assertions depend on it. Sharing a base would mean parameterizing the otel seam of the package's largest test file for no behavioral gain.
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds ChangesSilent-turn QA validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to This test-only change improves silent-failure coverage, but the release-gate validation can still pass without checking the new invariant on one path, and a transcript-recovery regression test may not exercise the intended ownership behavior because of a cwd mismatch. These bounded correctness gaps should be fixed or explicitly accepted before merging. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 92d44c7c-bc5f-4a44-9fd2-31b59d7ffc17
📒 Files selected for processing (19)
.agents/skills/agent-release-gate/SKILL.md.agents/skills/agent-release-gate/resources/coverage.md.agents/skills/agent-release-gate/resources/matrix_b1_builtin_find.py.agents/skills/agent-release-gate/resources/matrix_invariant_commit_auth_refusal.py.agents/skills/agent-release-gate/resources/matrix_l3_abandoned_approval.py.agents/skills/agent-release-gate/resources/matrix_t8_saved_files.py.agents/skills/agent-release-gate/resources/matrix_w3.py.agents/skills/agent-release-gate/resources/matrix_w4.py.agents/skills/agent-release-gate/resources/matrix_w5.py.agents/skills/agent-release-gate/resources/matrix_w7.py.agents/skills/agent-release-gate/resources/matrix_w7_daytona.py.agents/skills/agent-release-gate/resources/matrix_w7_per_harness.py.agents/skills/agent-release-gate/resources/qa_matrix_lib.py.agents/skills/agent-release-gate/resources/test_qa_matrix_lib_silent_turns.pysdks/python/oss/tests/pytest/unit/agents/adapters/test_vercel_stream_silent_failure.pyservices/runner/tests/unit/daytona-transcript-recovery.test.tsservices/runner/tests/unit/sandbox-agent-pi-error.test.tsservices/runner/tests/unit/silent-turn-contract.test.tsservices/runner/tests/utils/silent-turn.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Comment on lines
+191
to
+200
| # `settled2` is satisfied by a turn with no approval and no error — which a turn | ||
| # that produced nothing also satisfies (ASD-EST100). | ||
| silent = check_no_silent_turn(turns_a + turns_b + nudge_turns) | ||
| ok = settled2 and a_present2 and b_present2 and not silent["violations"] | ||
| return { | ||
| "status": "PASS" if ok else "FAIL", | ||
| "why": ( | ||
| f"STAGE 2 (diagnose-and-ask, then a bare 'yes, retry' with zero mechanics " | ||
| f"coached): settled2={settled2}, a_present={a_present2}, b_present={b_present2}" | ||
| f"coached): settled2={settled2}, a_present={a_present2}, b_present={b_present2}, " | ||
| f"silent_turns={silent['violations']}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce and verify the invariant on every matrix_w3.py PASS path.
Stage 1 returns before the current silent-turn check. The wiring test does not detect that bypass.
.agents/skills/agent-release-gate/resources/matrix_w3.py#L191-L200: evaluateturns_a + turns_bbefore the Stage 1 return and fail when violations exist..agents/skills/agent-release-gate/resources/test_qa_matrix_lib_silent_turns.py#L175-L185: add a Stage 1-specific assertion or mocked execution path.
📍 Affects 2 files
.agents/skills/agent-release-gate/resources/matrix_w3.py#L191-L200(this comment).agents/skills/agent-release-gate/resources/test_qa_matrix_lib_silent_turns.py#L175-L185
Comment on lines
+175
to
+185
| @pytest.mark.parametrize("cell", WIRED_CELLS) | ||
| def test_the_invariant_is_wired_into_the_cell(cell): | ||
| """The check was dead code when it first landed: defined, tested, and called by nobody, so | ||
| the gate still PASSed a completely bare turn. This fails if a cell drops the wiring.""" | ||
| source = (RESOURCES / cell).read_text() | ||
|
|
||
| assert "check_no_silent_turn" in source, f"{cell} does not call the invariant" | ||
| # It must feed the verdict, not just be computed and discarded. | ||
| assert re.search(r'not silent\["violations"\]', source), ( | ||
| f"{cell} computes the invariant but does not let it decide the verdict" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the Stage 1 PASS path in matrix_w3.py.
This source-level check passes when a cell contains not silent["violations"] in any branch. matrix_w3.py contains that condition in Stage 2, but its Stage 1 return can pass before the invariant runs.
Add an assertion that the Stage 1 branch checks turns_a + turns_b, or execute that branch with mocked dependencies.
Comment on lines
+113
to
+115
| parts = [part async for part in agent_run_to_vercel_parts(run)] | ||
|
|
||
| assert _error_texts(parts), "an empty turn produced no error frame at all" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the error frame order.
The test accepts an error frame after finish. A browser can treat finish as terminal before it receives the error. Assert that finish is last and that error occurs before it.
Proposed test change
assert _error_texts(parts), "an empty turn produced no error frame at all"
+ types = [part["type"] for part in parts]
+ assert types[-1] == "finish"
+ assert types.index("error") < types.index("finish")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| parts = [part async for part in agent_run_to_vercel_parts(run)] | |
| assert _error_texts(parts), "an empty turn produced no error frame at all" | |
| parts = [part async for part in agent_run_to_vercel_parts(run)] | |
| assert _error_texts(parts), "an empty turn produced no error frame at all" | |
| types = [part["type"] for part in parts] | |
| assert types[-1] == "finish" | |
| assert types.index("error") < types.index("finish") |
Comment on lines
+85
to
+88
| sandboxTranscript: piTranscriptWithError( | ||
| "/home/sandbox", | ||
| RATE_LIMIT_ERROR, | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the transcript cwd to the simulated Daytona cwd.
runSilentTurn uses its generated temporary directory as createDaytonaCwd when cwd is omitted. These transcripts instead record /home/sandbox. The existing reader accepts a transcript only when its session-record cwd matches the workspace cwd.
Set cwd: "/home/sandbox" for both runs, or derive both values from one remoteCwd constant. This keeps the regression test from accepting a recovery path that ignores transcript ownership.
Also applies to: 109-112
Contributor
Railway Preview Environment
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Users keep reporting that the agent "stops answering and says nothing" (ASD-EST100). When a provider rejects a model call (no credit, quota, rate limit, bad key), pi-acp maps the failure to a clean
{stopReason: "end_turn"}with no content. The runner believes it, returnsok: truewith an empty output, records the turn as a completed one, and the playground renders a blank bubble with no error anywhere. The only copy of the real message sits in Pi's transcript inside the sandbox, which is then destroyed.No suite caught that shape. This PR adds the regression tests at each layer the failure passes through. It changes no product code.
Changes
A swallowed provider failure on Daytona comes back today as:
with an event stream of exactly
["done"]. That is the incident's signature verbatim, and the tests reproduce it through the real engine rather than a fixture.The contract they assert is:
with an
errorevent ahead of the terminaldonethe API reconciles on, and with no continuity recorded, because recording an empty turn as the session's last good turn is what let one silent failure poison every later turn.Three layers are covered.
silent-turn-contract.test.tsdrivesrunSandboxAgentthrough a fake harness wired to the real otel run, so whether a turn was empty is decided by the code under test and not by a canned output string.daytona-transcript-recovery.test.tsstands a Pi transcript up inside a fake remote sandbox served over the daemon file API, since the transcript reader is switched off on Daytona and cloud runs on Daytona.test_vercel_stream_silent_failure.pypins the SSE half: a terminal-onlyok: falsemust carry the caller's own reason, never the generic "produced no output" frame.The QA release gate gets the same invariant.
check_no_silent_turnis wired into the ten cells that hold turns and whose PASS depends on something not appearing, because a turn that produced nothing satisfies those cells by doing nothing at all. Its definition of content mirrorscontent_parts_emittedin the product's own Vercel egress, so a turn whose only output is a file or a data payload is not reported. Reasoning stays excluded, matching the adapter: a turn that only thought renders as a blank bubble.Seven tests use
it.fails. They pin contracts that are not implemented yet, so they pass today because the assertion fails, and they turn red the moment the named fix lands, which is the signal to drop the marker. Five await the fail-loud empty-turn guardrail (empty turn on Daytona, empty turn on a non-Pi harness, continuity recorded for an empty turn, a banner-only turn, a turn that called a tool and then died). Two await the Daytona swallowed-error reader.Two properties keep the suite honest.
it.failsis satisfied by any failure, including a fixture that dies before running a turn, so every one is backed by a separate guard test proving its setup reaches a real turn. And every fail-loud assertion is paired with a turn that must keep succeeding (a parked turn, a turn that answered), so the pending fix cannot be satisfied by failing everything, and the empty-turn check cannot be satisfied by suppressing it.Tests / notes
tsc --noEmitclean.conciseErrorrewrites the provider's raw text into a curated classification, so "the real provider message" reaches the user as a classification of it. And the SDK's zero-content backstop keys only on content and error state, never onstopReason, so a paused turn with zero content parts would emit the generic no-output frame. Gates always emit content today, so it does not fire in practice.check_no_silent_turn's content definition. It is a deny-list on purpose:data-<name>payloads are open-ended, so an allow-list would fail healthy turns, while a deny-list can only under-report.