mirror of
https://github.com/obra/superpowers
synced 2026-08-09 11:24:19 +00:00
docs(specs): define final-review corrections
Amend the PR 1931 safety-hardening design for the three findings raised by final Codex review at 2832e01a.\n\nSpecify that proof and optional SSH snapshot pipelines are wholly owned by a pipefail-enabled Bash process, and that fenced-code filtering records both marker family and opening width.\n\nKeep scenario cards on the normal test-first lifecycle: author, validate, and run them before committing; after they pass, commit only the reported artifacts, focused-review that delta, and require the cards in HEAD with a clean tree before finishing. Bootstrap changes to the governing spec still require human approval.\n\nDefine behavior-focused RED/GREEN coverage for the checker, evidence recipes, and post-review artifact lifecycle while preserving the deferred browser-control and pipe-less-table scope.
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user