Repository navigation
Security review: surface the session's reason when it deliberately ends incomplete #607
Description
Activity
This was generated by AI during triage.
Agent Brief
Category: bug
Summary: Give the Security review an explicit incomplete final line, and surface its reason in the Run's CauseCurrent behavior:
The Security review prompt defines a single terminal line,Security review: {"unaddressed_count":…,"findings":[…],"pre_existing_count":…}, which is required only after a complete review. When the session deliberately ends incomplete (for example, no sandbox, a missing prerequisite or a blocked validation), it ends with prose.Review::from_final_messagefails, andsecurity::review::Outcome::ofmaps every error exceptSafeguardRefusalto the same fixed text: "Security review incomplete: session failed, ended early or omitted a valid final line; see Session log". So a well-explained, deliberate incomplete looks the same as a crash.Desired behavior:
- The Security review prompt defines a second terminal line:
Security review: incomplete: <one-line reason>. The prompt asks for it whenever the review is incomplete or missing a prerequisite, and keeps the JSON line for complete reviews. The audit'sSecurity audit: incompleteprotocol is the model for this. - Parsing the final message tells three cases apart:
- a valid JSON line, which is a complete review (unchanged);
- an explicit incomplete line with a nonempty, single-line reason, which becomes an incomplete outcome carrying that reason;
- anything else (no message, no recognizable line, invalid JSON, inconsistent counts), which keeps today's generic text.
- In case 2, the Run's Cause line, the PR's "Security review outcome" entry and the Hold all show the session's reason. Example:
Security review incomplete: required sandbox isolation is unavailable. - The private-report check still applies to explicit incompletes: if the report file holds entries, they are still validated and recorded, or the mismatch is reported. Pre-existing vulnerability details must never reach the public reason. The reason comes from the session, so treat it as public text under the same rule the prompt already applies to the final message, and cap its length.
- Safeguard refusals keep their existing "Security review refused: …" wording.
Key interfaces:
prompt::security_review: the final-line contract text. Regenerateprompts/withUPDATE_PROMPTS=1 cargo test prompts_page; don't editprompts/by hand.Review::from_final_message: today it returnsResult<Review>. It needs a way to return "incomplete, with reason" that is different from a parse failure, for example a dedicated error type thatOutcome::ofdowncasts as it doesSafeguardRefusal, or an enum return.security::review::Outcome::of/Outcome::Incomplete(String): carries the session's reason.
Acceptance criteria:
- A final message ending
Security review: incomplete: required sandbox isolation is unavailableproduces an incomplete outcome whose Cause and PR entry include that reason. - A final message with no recognizable final line, invalid JSON or inconsistent counts still produces the existing generic incomplete text.
- A valid JSON final line behaves exactly as it does today.
- An empty or multi-line incomplete reason is treated as an invalid final line (generic text).
- The safeguard-refusal wording is unchanged.
- The generated
prompts/matches the updated prompt module.
Out of scope:
- Making a sandbox available, or checking for one in
thirdshift setup(tracked separately). - Changing whether an incomplete Security review holds the Run.
- Changes to the Security audit's terminal protocol.
- The Security review prompt defines a second terminal line:
What happened
The Run for #596 (PR #605) failed with:
The Security review session didn't crash or end early. It finished in 2m 53s and deliberately reported itself incomplete. The
thirdshift-security-auditskill says "If every control cannot be enforced, do not execute target code". The host had no usable sandbox (bwrapwasn't installed, andunsharewas refused underkernel.apparmor_restrict_unprivileged_userns=1), so the reviewer stopped before running target code. Its final message explained this clearly:The review prompt asks for the
Security review: {json}line only "after a complete review", and otherwise says "say so and do not claim completion". So the reviewer followed the contract. thirdshift then reported its reason with the same generic text it uses for a crash or a malformed line, so the maintainer had to read the session log to learn why.The Security audit already avoids this with an explicit
Security audit: incompletefinal line. The review has no equivalent.