Repository navigation
approve: the CLI decides before it shows — a reviewer approves a plan they have not seen #46
Description
Activity
Fixed on
mainin #99, following the acceptance criteria as written.--showprints the parked plan and decides nothing. It is the only mode that writes no decision file, and it prints the fingerprint precisely so the follow-up can be bound to it:$ grapharc approve runs --show plan : 0666d74d8128 fingerprint : 2ef3fc6dd5270c3b nodes : gather, analyse, report edges : __start__ -> gather gather -> analyse analyse -> report report -> __end__ approve : grapharc approve runs --fingerprint 2ef3fc6dd5270c3b refuse : grapharc approve runs --fingerprint 2ef3fc6dd5270c3b --deny--fingerprint FPrefuses a decision bound to a stale plan, naming both, with exit 2:$ grapharc approve runs --fingerprint deadbeef error: that is not the plan now waiting: you reviewed deadbeef, the parked plan is 2ef3fc6dd5270c3b. Re-read it with `grapharc approve runs --show`. $ echo $? 2The one-shot form is unchanged — automation and the Slack button path both depend on it — but it now prints the plan before writing the decision rather than after, which was the specific complaint in the report.
test_the_plan_is_printed_before_the_decision_linepins the ordering.Both flags are also admitted through the Slack gate, where they make the path narrower rather than wider: from a phone, "let me look first" and "only if it is still the plan I read" are exactly the two things a reviewer needs.
13 tests in
tests/test_approval_review.py; the existing approve/deny tests pass unchanged.Left deliberately undone: making
--showthe default. The criteria call for the existing tests to pass unchanged, and flipping the default would break both them and the documented Slack flow. If you want the human default inverted, that is a deliberate breaking change worth its own issue —--showexisting is what makes it safe to consider.
What happens
grapharc approve <trace>reads the parked request and writes an approve decision immediately; the plan's contents —approved: plan 337ed97f9a2c (triage, patch, verify)and the fingerprint — are printed after the decision is already written. There is no way to look first: no flag that prints the pending request without deciding, and no argument that binds the decision to a fingerprint the reviewer previously read.The file protocol underneath is already careful —
write_decisionquotes the request's fingerprint and the loop discards a decision whose fingerprint does not match the parked request — so a stale decision cannot land on a newer plan. What is missing is the human half: the CLI never shows the reviewer what they are saying yes to.Why it matters
The approval gate is the product's central trust claim ("there is no already-approved path"). An approve command that works sight-unseen reduces that gate to a button — in a Slack-driven workflow especially,
approvebecomes a reflex, and the audit trail then records informed consent that never happened.What to consider
grapharc approve <trace> --show(orapprovewith no decision flag): print the parked plan — nodes, kinds, edges, fingerprint, estimated cost — and exit without deciding.grapharc approve <trace> --fingerprint <fp>: write the decision bound to the fingerprint the reviewer actually reviewed; refuse with exit 2 if the parked request differs. The plumbing already exists — the CLI just never exposes it.--yes?) for automation; the default should lean toward review-then-decide for humans.Out of scope
The decision file format and the loop's fingerprint matching — both are correct today.
Acceptance criteria
A reviewer can print a parked plan without deciding; a decision bound to a stale fingerprint is refused with a message naming both fingerprints; the existing approve/deny tests pass unchanged.