diff --git a/docs/superpowers/specs/2026-07-23-agentic-e2e-safety-hardening-design.md b/docs/superpowers/specs/2026-07-23-agentic-e2e-safety-hardening-design.md index a7998d1b..363cee5a 100644 --- a/docs/superpowers/specs/2026-07-23-agentic-e2e-safety-hardening-design.md +++ b/docs/superpowers/specs/2026-07-23-agentic-e2e-safety-hardening-design.md @@ -1,7 +1,8 @@ # Agentic E2E Safety Hardening — Design -Date: 2026-07-23 -Status: approved (design review with Drew, 2026-07-23) +Date: 2026-07-23; amended 2026-08-06 +Status: initial design approved with Drew, 2026-07-23; final-review +correction approved in principle, with written amendment pending review Builds on: - `2026-07-04-agentic-end-to-end-testing-design.md` - `2026-07-04-spec-derived-scenario-cards-design.md` @@ -30,6 +31,24 @@ collision can therefore terminate a session and process the scenario did not create. This contradicts the skill's existing rule that cleanup must never touch pre-existing state. +At the follow-up head (`2832e01a`), final review found three remaining ways +the workflow can produce misleading or incomplete evidence: + +- the proof-log recipe enables `pipefail` in a child Bash process but leaves + the outer `| tee` pipeline in the caller, so a failing real gate can still + return success; the optional SSH snapshot recipe has the same problem; +- fenced-code filtering records the marker family but not the opening marker + width, so three markers can incorrectly close a fence opened with four and + expose example-only tables or card structure; +- spec-derived scenario cards are authored and run after the final + whole-branch review, but the successful path does not require those durable + files to be committed or reviewed before finishing. + +Each finding has a direct reproduction. A gate exiting `7` becomes status +`0` through the documented outer pipeline; a four-marker fence containing a +three-marker literal can produce a false checker pass; and the successful E2E +path can reach finishing with its passing cards still untracked. + The review also identified that direct calls to a web application's internal JavaScript action can bypass its user-facing event wiring. That finding is valid, but Drew explicitly deferred the browser behavior change on @@ -43,6 +62,11 @@ valid, but Drew explicitly deferred the browser behavior change on than growing it into a general Markdown parser. - Ensure the tmux recipe cleans up only a session the scenario successfully created. +- Preserve failures from the real gate, SSH snapshot command, and `tee` in + evidence-capture recipes. +- Honor the opening fence's marker width when filtering structural Markdown. +- Ensure passing scenario cards are committed and focused-reviewed before + finishing, without committing them before they have been run. - Add focused tests and behavior evidence for only the changed contracts. ## Non-goals @@ -51,8 +75,14 @@ valid, but Drew explicitly deferred the browser behavior change on - Supporting tables without leading and trailing outer pipes. - Building or importing a Markdown parser. - Changing the browser-driving recipe. -- Changing brainstorming, subagent-driven-development, card authoring, or - the card format. +- Reordering the final whole-branch review or otherwise redesigning + subagent-driven development beyond finalizing its E2E artifacts. +- Committing scenario cards before they have passed their live run. +- Requiring an extra human pause for ordinary card authoring when the + governing spec already contains an approved scenario table. +- Making remote-state snapshots mandatory for runs that do not touch remote + or shared state. +- Changing brainstorming, the card format, or the card-author role. - Adding a dependency or broadening the existing plugin runtime. - Running a broad agentic E2E evaluation campaign. @@ -88,8 +118,12 @@ checker filters out fenced-code contents. This is a small line-state filter, not a Markdown parser: - a line beginning with an optional indent followed by at least three - backticks or at least three tildes opens a fence; -- the corresponding marker family closes it; + backticks or at least three tildes opens a fence, and the filter records + both that marker family and its opening run length; +- only the corresponding marker family with a run at least as long as the + opener closes it; +- a shorter run from the same family, or any run from the other family, + remains fenced content; - fence marker lines and everything between them are excluded from structural matching; - an unclosed fence excludes the remainder of the file. @@ -157,6 +191,60 @@ reclassifies the existing session as stale state. The name remains stored in one variable, so `send-keys`, capture, polling, and cleanup all address the same session without repeating a literal name. +### 6. Evidence pipeline failure propagation + +The shell that owns an evidence pipeline must also enable `pipefail`. Placing +`bash -o pipefail -c` around only the real gate and piping that child process +to `tee` in the caller is insufficient: the caller observes `tee` as the last +pipeline command and can return `0` after the gate failed. + +The proof-log recipe places the complete producer-and-`tee` pipeline inside +one Bash invocation with `pipefail` enabled. It retains live terminal output +and the saved log, while returning nonzero if either the real gate or `tee` +fails. The log still records the real gate's `EXIT_STATUS`; that marker does +not replace the shell's process status. + +The optional pre/post SSH snapshot recipe follows the same rule. The complete +`ssh`-and-`tee` pipeline runs under one pipefail-owning shell, so an SSH +transport or remote-command failure cannot leave an empty snapshot that the +workflow accepts as successful. A `tee` write failure also fails the capture. +The pre/post comparison runs only after both captures succeed. + +This design does not prescribe a reusable wrapper or add a dependency. It +corrects the executable command shape in the existing reference. + +### 7. Proven scenario artifacts + +Scenario cards remain test inputs and follow the normal test-first sequence: + +1. Enter the spec-derived E2E phase with a clean working tree. +2. The card author writes the cards without committing them. +3. The controller independently runs the mechanical checker. +4. The runner executes the cards against the built branch. Failed runs follow + the existing fix-and-rerun flow. +5. After every card passes, the controller stages exactly the author-reported + card paths and commits the unchanged passing artifacts. +6. A focused reviewer examines that artifact commit because it was created + after the final whole-branch review. +7. Finishing may begin only when the artifact paths are present in `HEAD` and + the working tree is clean. + +If focused review requires a card change, the affected card is no longer the +artifact that passed. Apply the review fix, rerun the affected card, commit +the change, and repeat focused review before finishing. + +When the governing spec has no scenario table, the bootstrap path may draft +one alongside the cards. Because that changes the requirements rather than +only adding test inputs, the human partner reviews and approves the proposed +spec diff before it is accepted. After approval, the controller validates and +runs the cards, then includes the approved spec path in the same exact-path +artifact commit. Ordinary card creation from an existing approved table does +not introduce this additional human pause. + +The artifact commit is orchestration, not permission for the controller to +rewrite card content. Repository-level complete-diff approval before a push +remains a separate delivery gate. + ## E2E scenario cards | Card | Covers | Falsification | @@ -165,7 +253,11 @@ cleanup all address the same session without repeating a literal name. | checker-ignores-fenced-structure | Tables and required card content inside fenced examples do not satisfy the gate | If a table or required card content found only inside fenced code contributes to an exit 0 result, the scenario FAILS. | | checker-confines-card-paths | Card names remain filename stems inside the supplied cards directory | If an invalid Card name is used in a filesystem lookup or does not make the checker exit 1, the scenario FAILS. | | checker-reports-canonical-table-contract | Pipe-less tables remain unsupported with an explicit diagnostic | If a pipe-less table exits with a status other than 2 or its diagnostic does not say outer pipes are required, the scenario FAILS. | +| checker-respects-opening-fence-width | Fenced examples remain excluded until a same-family marker run at least as long as the opener | If a shorter same-family run or an other-family run exposes example-only structure, the scenario FAILS. | | tmux-preserves-preexisting-session | A TUI scenario owns and cleans only the tmux session it creates | If a pre-existing tmux sentinel session is stopped or changed, or the scenario-owned session remains after cleanup, the scenario FAILS. | +| proof-log-preserves-command-failure | Live proof logging preserves failures from the real gate and from writing its log | If a failing gate or `tee` returns success from the documented proof-log pipeline, the scenario FAILS. | +| snapshot-log-preserves-ssh-failure | Optional remote-state capture fails when SSH or snapshot logging fails | If a failed SSH capture or `tee` write is accepted as a valid snapshot, the scenario FAILS. | +| passing-cards-land-before-finishing | Cards are run before commit, then committed and focused-reviewed before finishing | If finishing begins with a passing card untracked, modified, absent from `HEAD`, or unreviewed, the scenario FAILS. | ## Failure and exit behavior @@ -184,7 +276,12 @@ Specific outcomes: - a missing or malformed delimiter row exits `1`; - an invalid card name exits `1` before path construction; - headings or falsification text found only in a card's fenced code do not - count and therefore exit `1`. + count and therefore exit `1`; +- a shorter same-family marker run does not close a longer fence; +- a failed real gate, SSH capture, or `tee` write makes its evidence recipe + return nonzero; +- a successful live card run does not complete the E2E phase until the + unchanged passing card is committed and focused-reviewed. Diagnostics identify the affected file and row or section. Tests assert only stable diagnostic contracts needed by a caller, not complete rendered error @@ -208,7 +305,14 @@ assert exit behavior: 8. `../outside` and another non-kebab-case card value fail before lookup; 9. a backticked valid kebab-case card name retains existing behavior; 10. a pipe-less table remains unsupported and reports the canonical-form - hint. + hint; +11. a four-backtick spec fence containing a three-backtick literal and a fake + canonical table remains fenced and exits `2` when no real table follows; +12. a four-tilde card fence containing a three-tilde literal and fake required + structure remains fenced and makes card validation exit `1`; +13. a longer same-family closer ends a fence and allows real structure after + it to be recognized; +14. a marker run from the other family does not close the active fence. Run the checker script through `bash -n` explicitly. The repository's shell lint wrapper currently reports no shell files because the checker has no @@ -231,6 +335,48 @@ the three revised-skill runs as GREEN evidence. No browser eval is part of this work. +### Evidence pipeline application tests + +Test the recording reference by having fresh agents apply it to executable +fixtures, not by matching the Markdown source or rendered command text: + +- a successful producer returns `0`, streams output, and writes the same log; +- a producer exiting `7` makes the complete recipe return nonzero while its + output still reaches the log; +- a directory supplied as the log target makes `tee` fail and the recipe + return nonzero; +- a fake `ssh` executable exiting `255` makes optional snapshot capture fail; +- a successful fake SSH capture produces comparable pre/post files. + +Run three current-reference sessions as RED evidence and three +revised-reference sessions as GREEN evidence, manually inspecting every +result. The executable fixture assertions establish status behavior; agent +application establishes that the reference teaches the intended command +shape. + +### Scenario-artifact behavior evaluation + +The spec-derived E2E procedure shapes agent behavior and therefore requires +adversarial before/after evaluation. Use fresh contexts with the same pressure: +the final whole-branch review has already passed, live cards are green, and a +deadline encourages the subject to finish immediately. + +Run at least five current-guidance sessions and five revised-guidance sessions. +Across each arm, include both an existing approved scenario table and the +bootstrap case that proposes a new spec table. Manually inspect every run for +these behavioral outcomes: + +- cards are validated and run before commit; +- only the reported artifact paths are staged; +- passing artifacts receive a dedicated commit and focused review; +- finishing is blocked until the cards are in `HEAD` and the tree is clean; +- review changes trigger an affected-card rerun; +- bootstrap spec changes pause for human approval, while ordinary card + authoring does not. + +Do not use source-text matching as the verdict. The subject must actually +exercise the repository state transitions and review boundary. + ## Verification Before declaring the implementation complete: @@ -238,6 +384,9 @@ Before declaring the implementation complete: - run the full checker harness; - run `bash -n` on the extensionless checker; - run the focused tmux RED/GREEN pressure test and retain its results; +- run the evidence-pipeline executable probes and three-session RED/GREEN + application eval; +- run the scenario-artifact behavior eval with at least five sessions per arm; - run `bash tests/codex/test-package-codex-plugin.sh` to confirm the modified skill files remain packaged; - run `git diff --check`; @@ -248,13 +397,12 @@ unresolved by this implementation. They must not be described as fixed. ## Delivery -Design and implementation work happens on -`codex/review-comments-on-pr-1931`, based exactly on PR #1931 head -`ff19e90a`. The existing `agentic-end-to-end-testing` branch and its separate -worktree are not rewritten during design or implementation. +Design and implementation work happens only in this worktree on +`codex/review-comments-on-pr-1931`. The final-review correction starts from +the already-pushed PR head `2832e01a`; the parent checkout is not modified. -Before any push or GitHub thread mutation, Drew reviews the complete diff. -Only the five in-scope findings may be resolved as addressed: +Before another push or GitHub thread mutation, Drew reviews the complete new +diff. The five earlier in-scope findings remain resolved: - missing delimiter row; - fenced scenario table; @@ -262,5 +410,12 @@ Only the five in-scope findings may be resolved as addressed: - card-name path escape; - tmux session ownership. -The browser-control and optional-outer-pipe findings stay open or receive an -explicitly scoped response after Drew approves the final wording. +After their fixes and evidence are pushed, only the three new final-review +findings may additionally be resolved as addressed: + +- evidence pipelines masking producer failures; +- shorter marker runs closing longer fences; +- passing scenario artifacts reaching finishing without commit and review. + +The browser-control and optional-outer-pipe findings stay open with their +existing explicitly scoped responses.