feat(loop): drive-session row tagging so ui_review teardown drops exactly the drive-created rows#1333
Merged
Conversation
…ve-created rows The Stage-2 drive now advertises a per-run drive-session id on the X-UI-Review-Drive-Session request header and emits a session-stamped row manifest (one record per mutating step) in its result envelope. Stage-5 teardown consumes the manifest and, on --confirm, deletes exactly the tagged rows via the project's uiReview.run.rowTeardown.deleteCommand (dev DB only). An absent/untagged manifest or a missing recipe still fails closed to a drop failure or may-remain-untagged — never a guess or a wrong-scope delete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177JAWNT67FY6d48qQJeDwo
Owner
Author
Gate review:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes ui_review Stage-5 teardown capable of deleting exactly the dev-DB rows created during a Stage-2 drive run by introducing a per-drive “drive-session” identifier, emitting a session-stamped row manifest from the drive, and wiring the teardown CLI to run a project-provided delete-by-session command on confirmation.
Changes:
- Stage-2 drive now generates/accepts a drive-session id, advertises it via
X-UI-Review-Drive-Session, and emits a session-stampedrowManifestin the result envelope. - Stage-5 teardown CLI now resolves
uiReview.run.rowTeardown.deleteCommandfrom config and executes it (confirmed only) withUI_REVIEW_DRIVE_SESSIONset, reportingDROPPEDwith a count. - Adds/updates tests and docs to cover/configure the new
rowTeardown.deleteCommandcontract and validate fail-closed behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/loop/ui-review-teardown.test.mjs | Adds end-to-end and CLI-seam tests verifying manifest-driven row drop behavior and fail-closed paths. |
| test/loop/ui-review-drive.test.mjs | Adds tests asserting session-stamped manifest emission and the no-session empty-manifest behavior. |
| test/docs/ui-review-recipe-doc.test.mjs | Updates schema-leaf count to account for the new rowTeardown config key. |
| scripts/loop/ui-review-teardown.mjs | Wires a real dropRows seam: resolves rowTeardown.deleteCommand, extracts a shared session, runs delete command in worktree, and reports dropped count. |
| scripts/loop/ui-review-drive.mjs | Generates a per-run drive-session UUID, sets it as an HTTP header, and passes it into core drive orchestration. |
| packages/core/src/loop/ui-review-teardown.mjs | Updates core documentation to reflect session-tagged manifest model vs. prior “untagged rows” limitation. |
| packages/core/src/loop/ui-review-drive.mjs | Introduces DRIVE_SESSION_HEADER, mutating-step manifest emission, and driveSession/rowManifest on the result envelope. |
| packages/core/src/config/config.mjs | Adds optional uiReview.run.rowTeardown.deleteCommand config schema and resolves it into the run recipe. |
| docs/ui-review-recipe-contract.md | Documents the header + session tagging contract and the new rowTeardown.deleteCommand recipe key. |
sessionFromManifest only collected non-empty sessions, so a manifest with one tagged row and the rest untagged resolved to a single session and dropped it while the untagged rows went unaccounted-for but were reported dropped. Any untagged row now fails closed (refuse, DROP_FAILED), never a partial delete misreported as complete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177JAWNT67FY6d48qQJeDwo
…y return The fail-closed return when no uiReview.login recipe is declared omitted the documented driveSession/rowManifest keys (they were only set once a session was generated later), so the CLI output shape depended on the failure mode. Emit driveSession:null + rowManifest:[] there too, and pin it with a runCli test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177JAWNT67FY6d48qQJeDwo
…he row delete A present-but-non-string worktreePath signals a corrupted provision result; the old fallback silently used the primary checkout as the delete cwd, risking a wrong-scope destructive delete. Resolve a malformed path to null and refuse the drop (DROP_FAILED) — distinct from an absent/empty path, which still legitimately falls back to the repo root. The cwd guard runs first so the refusal is explicit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177JAWNT67FY6d48qQJeDwo
Owner
Author
Gate review:
|
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
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.
Summary
Makes the ui_review Stage-5 teardown dev-DB row-drop real instead of honest-but-inert. Stage 5 ships a side-effect ledger + confirmation gate, but the row-drop could only report
may-remain-untaggedbecause the Stage-2 drive never tagged the rows it created. This adds drive-session row tagging so teardown can drop exactly the rows a drive created — dev DB only, fail-closed.Scope and context
packages/core/src/loop/ui-review-drive.mjs+scripts/loop/ui-review-drive.mjs: the drive stamps a per-run drive-session id and emits arowManifest(one session-stamped record{ session, flow, step, action }per mutating step —click/select/upload/dispatch;goto/fillexcluded) plusdriveSessionon its result envelope. It also advertises the session on anX-UI-Review-Drive-Sessionrequest header so a cooperating app can tag the dev-DB rows it persists under those requests.scripts/loop/ui-review-teardown.mjs: the previously-inertdropRowsseam is now real. On--confirmit resolves the shared session from the--row-manifestand deletes exactly those rows via a project-declareduiReview.run.rowTeardown.deleteCommand, run in the provisioned worktree with the session passed inUI_REVIEW_DRIVE_SESSION(env, not interpolated). The ledger reportsdroppedwith the count. An absent/untagged manifest, a missing recipe, or a non-zero delete fails closed (may-remain-untagged/ drop-failed) — never a guess, never a wrong-scope delete.packages/core/src/config/config.mjs: newuiReview.run.rowTeardown.deleteCommand(optional; the operator-provided delete-by-session command).docs/ui-review-recipe-contract.md: documents the session header, thedeleteCommandrecipe a consuming repo must provide, and the by-session tagging contract.Fail-closed / trust boundary
The
deleteCommandis trusted operator config (like a build command), run only under--confirmin the dev worktree; the session id crosses into it via theUI_REVIEW_DRIVE_SESSIONenv var (not shell-interpolated), so it cannot inject, and the worktreecwdis POSIX single-quote-escaped before thecd(so a branch-derived path with shell metacharacters can't break out). Dev DB only — no production path. Absent manifest / no recipe / non-zero exit all resolve to a stated ledger reason, not a guess.Acceptance criteria
Satisfies #1287's ACs (checked off at merge): the Stage-2 drive emits a session-tagged row manifest in its result envelope; Stage-5 teardown drops exactly the manifested rows on
--confirm(dev DB only) and the ledger reportsdroppedwith the count instead ofmay-remain-untagged; the fail-closed paths are distinct — a genuinely absent/empty manifest resolves tomay-remain-untagged, while an untagged/mixed-session manifest, a missingdeleteCommandrecipe, or a non-zero delete resolves todrop-failed(never a guess, never a wrong-scope delete); a test drives a fixture that creates rows and asserts the manifest is emitted and teardown drops exactly those on confirm (no real DB);npm run verifygreen.Non-goals
Production row teardown. Migration rollback (the ledger records migrations; reverting is a separate action). Changing unrelated teardown behavior (worktree/app teardown, the artifact-hosting prune).
Validation
node --test test/loop/ui-review-drive.test.mjs test/loop/ui-review-teardown.test.mjs— 80 pass (drive emits the session-stamped manifest for mutating steps + a stable envelope shape on the no-recipe early return; teardown drops exactly the manifested rows on--confirmwith the count, via an injected run seam — no real DB; absent manifest →may-remain-untagged, and untagged/mixed-session or missing recipe or non-zero delete or malformed worktree path →drop-failed, with the runner never invoked).npm run verify— green (all suites: test:scripts, assets, extension, core, docs, pack, dev-loop);generate-claude-assets --checkclean.Closes #1287