Skip to content

feat(verdict): add executed-without-online-safety contract - #108

Merged
Kiran01bm merged 4 commits into
mainfrom
kiran01bm/pt4-accepted-blocking-verdict
Sep 11, 2026
Merged

feat(verdict): add executed-without-online-safety contract#108
Kiran01bm merged 4 commits into
mainfrom
kiran01bm/pt4-accepted-blocking-verdict

Conversation

@Kiran01bm

@Kiran01bm Kiran01bm commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Adds the verdict and plan-report contracts for operator-accepted blocking execution.

Why

Operators and automation need to distinguish bounded execution of an accepted refusal from online-safe success without parsing prose, and a dry run needs to say which refusals could later be accepted.

What

  • Add the executed-without-online-safety outcome with retained refusal identity (reason, class, cause, safer_idiom), explicit lock_timeout / statement_timeout, the blocking_passthrough audit field, and exit code 3; the constructor rejects a missing identity, an ineligible refusal, or a non-positive budget, and stamps the in-process refusal proof (RF-8) the same way WithRefusal does.
  • Move the closed accepted-blocking eligibility registry into pkg/verdict so plan reports can consult it without importing the front door; the front door's completeness, fail-closed, class-keyed, and pinned-rows tests still cover every registered key.
  • Add blocking_passthrough_eligible to every refused dry-run statement (absent on non-refused statements) and bump the plan report format version to 5 per its documented policy; demo/tour.sh asserts the new version.
  • Stamping a refusal onto a verdict carries its typed cause into the JSON, so a partitioned-parent refusal reports parent-blocking-index-build and the identity round-trips through Refusal(). Cause is documented as the refusal-wide discriminator (budget fired, or parent shape refused), not a budget-only vocabulary.
  • Pin the text and JSON renderers for the new outcome and the eligibility field in docs/cli-output-examples.md; the design doc's text example follows the renderers' field order.

Nothing produces the new outcome yet; the executor and the flag land in later PRs.

Before / after

One statement, DROP INDEX app.orders_created_at_idx, first as a dry run and then executed by an operator who has accepted the blocking refusal.

before                                      after
──────────────────────────────────────────  ──────────────────────────────────────────
plan (dry run), format_version 4            plan (dry run), format_version 5
  disposition: refuse                         disposition: refuse
  reason: index-statement                     reason: index-statement
  class:  by-design                           class:  by-design
  (no way to tell whether the operator        blocking_passthrough_eligible: true
   could ever accept this refusal)            (DROP INDEX a, b or REINDEX SCHEMA
                                               would say false)

execute, refusal accepted                   execute, refusal accepted
  no outcome exists for this path;            outcome: executed-without-online-safety
  the only choices are                        refusal:   by-design / index-statement
    executed-natively  exit 0  (a lie:        statement: DROP INDEX app.orders_created_at_idx
      the drop blocked writers), or           safer:     DROP INDEX CONCURRENTLY
    refused            exit 2  (nothing       budgets:   lock 3s, statement 10m
      ran)                                    blocking_passthrough: true
                                              exit 3
                                              JSON keeps reason, class, cause, budgets;
                                              Refusal() rebuilds the same typed refusal

Automation that treats exit 0 as "online-safe success" keeps that meaning; the accepted-blocking path is a distinct outcome and exit code it must opt into.

🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.

Keep future blocking execution fail-closed by admitting only typed refusal
shapes whose lock risk can be meaningfully bounded. Distinguish one-relation
index maintenance at the parse-only gate so broader forms remain ineligible.
Preserve typed refusal identity when an operator accepts bounded blocking work.
Expose plan eligibility and a distinct outcome and exit code so automation
never mistakes this path for online-safe success.
Base automatically changed from kiran01bm/pt2-eligibility-registry to main September 11, 2026 04:24
@Kiran01bm

Copy link
Copy Markdown
Collaborator Author

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/108, follow-up commit

All three findings are fixed in the follow-up commit, which also merges origin/main (the squashed registry PR landed with the IndexTarget.String() and RF-8 refusal-proof changes) and removes the pkg/migrate copy of the eligibility registry that the merge would otherwise have reintroduced beside the pkg/verdict move.

# Finding Status Explanation
1 demo/tour.sh asserts plan and diff format_version 4 while pkg/plan.FormatVersion is now 5, so make demo-check fails on this branch Fixed All four plan and diff assertions in demo/tour.sh now expect 5; the lint (1) and suggest (2) assertions are separate contracts and are unchanged. make demo-check against the compose Postgres passes end to end.
2 The Cause type and Verdict.Cause field comments, and the not-native-safe-budget-exceeded row in docs/cli-output-examples.md, still describe cause as a budget-only vocabulary although the registry keys partitioned-parent refusals on it Fixed The type comment now says Cause narrows a refusal reason to its typed discriminator — the budget that fired under budget-exceeded, or the parent shape refused under unsupported-partitioned-parent — and that the accepted-blocking registry keys on it, so it is refusal identity rather than rendering detail. The const block comment and CauseNone comment match. The field comment adds that the cause is preserved on an accepted-blocking verdict. The doc row now states the same field carries the partitioned-parent shape and keeps the note that both are unrelated to the plan statement's create-shape cause.
3 The docs/lock-budgeted-passthrough.md text-renderer example lists statement: before refusal:, but both Verdict.String() and the CLI renderer print refusal: first Fixed The example now reads table:, refusal:, statement:, safer:, budgets:, matching the order both renderers emit; docs/cli-output-examples.md already had the correct order and is unchanged.

Decisions to veto

  • Merge resolution keeps the pkg/verdict registry and deletes the pkg/migrate copy that main's squashed registry PR added; main's new registry tests (TestAcceptedBlockingUnknownKeysFailClosed, TestAcceptedBlockingRowsRequireTheirClass, TestGateVerdictAcceptedBlockingEligibility, the pinned-rows walk) are kept in pkg/migrate and call verdict.AcceptedBlockingDecision / verdict.AcceptedBlockingEligible. Main did not change the registry logic, so no behaviour was dropped.
  • WithAcceptedBlocking now also stamps main's RF-8 in-process proof field, so Refusal() on an executed-without-online-safety verdict returns the full refusal (including the site) in-process, the same as a refused verdict. Without it the accepted verdict would reconstruct from JSON fields only and lose site-keyed eligibility in-process. Main's Refusal() (proof first, then field reconstruction with cause) is taken as-is; the JSON round-trip assertion in TestGateVerdictAcceptedBlockingEligibility still holds because the site is not serialised.
  • pkg/statement/statement.go takes main's side wholesale (IndexTargets() and IndexTarget.String()); the registry test key format follows it (target=single-relation rather than target=1).

Source: #108, scratch review scratch/code-reviews/pg-sprite-pr108-review.md at head e8b7d43a; fixes in 7aba874

@Kiran01bm
Kiran01bm marked this pull request as ready for review September 11, 2026 05:10
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@aparajon

Copy link
Copy Markdown
Collaborator

🤖 1/2 — the contract and its coverage. The invariant registry, the docs, and the stack are in 2/2.

Reviewed 231ab81f..7aba874 — 16 files, +383/−112 (the branch merges main rather than rebasing, so git log still shows the pre-squash a109572; see 2/2). This adds a terminal outcome, a new exit code, and a new field in a versioned wire contract, so I mutated each new guard rather than reading it, then probed the audit record against a real round trip. Fifteen mutations over ./pkg/{verdict,plan,migrate}/... ./internal/cli/... ./cmd/...; base green (full SKIP_INTEGRATION=1 go test ./... green, lint 0 issues, CI 15/15).

Mutation Result
eligibility gate dropped from WithAcceptedBlocking caught — TestAcceptedBlockingVerdictRejectsIllegalStates/ineligible_refusal
lock budget accepts zero caught — .../zero_lock_budget
statement budget accepts zero caught — .../zero_statement_budget
BlockingPassthrough flag not set caught — TestAcceptedBlockingVerdictContract, TestCLIOutputExamplesMatchPipelineOutput
by-design class check flipped in the registry caught — TestAcceptedBlockingRowsRequireTheirClass/index_site_outside_by-design
capability-boundary class check flipped caught — .../parent_cause_outside_capability-boundary
plan eligibility hardcoded false caught — TestRefusedStatementsReportAcceptedBlockingEligibility/blocking_parent_index
formatDuration minute branch dropped caught — TestAcceptedBlockingVerdictContract, TestCLIOutputExamplesMatchPipelineOutput
zero-refusal guard dropped survived — equivalent
NewRefusal re-validation dropped survived — equivalent
v.proof = r dropped survived
Refusal() accepts any outcome survived
exit 3 becomes exit 2 survived
headline severity warninghelp survived
budgets line dropped from writeVerdictText survived

Two things here are better than the PR describes them. provenRefusal (verdict.go:402) re-validates the proof through NewRefusal and checks it against the exported fields before handing it out — that closes the door WithSite/WithCause had opened onto Refusal(), and it does it in the place that makes RF-7's enforcement sentence true again rather than by constraining the builders. And moving the eligibility registry from pkg/migrate to pkg/verdict is the change that makes a consumer other than migrate able to ask the question at all — pkg/executor is the one that needs to, and its inability to import pkg/migrate is precisely why ExecuteAcceptedBlocking currently invents its own admission rule. Worth saying in the summary; it reads as a file move and it is a dependency-direction fix.

TestRefusalRegistryIsComplete's walk is also holding up well under extension — require.True(t, decided, ...) per key, with the anti-vacuity floor and the named sentinels, means a new cause or site cannot enter the vocabulary without an explicit eligibility decision. That is the structural guarantee the rest of this diff leans on, and it survived the move to another package intact.

The two equivalent survivors are worth naming as such rather than listing as gaps. Dropping the r.IsZero() check and dropping the NewRefusal re-validation are each masked by the eligibility gate below them: a zero refusal has reason "", which falls to AcceptedBlockingDecision's default: return false, false. And the one case that would distinguish the NewRefusal check — an eligible class/reason pair carrying an owner — is unconstructible, since ByDesign/CapabilityBoundary never set an owner and NoOnlineSafetyProblem sets a class that is never eligible. Three guards for one reachable failure is defense in depth, not missing tests.


1. The audit record cannot be checked against the registry for the site-keyed row

WithAcceptedBlocking records Reason, Class, Owner, Cause, and the budgets, and keeps the full refusal in v.proof (:345-349). The INV: RF-8 comment above it says the point is "so a consumer that re-derives eligibility reads the same identity the gate did" — true in process. The verdict's JSON is the part that leaves the process, and Site has no JSON field.

I ran both eligible rows through the round trip that an audit consumer performs:

Eligible row In process After json.MarshalUnmarshalRefusal()
by-design / index-statement / site index-single-relation eligible ineligiblesite="", and err == nil
capability-boundary / unsupported-partitioned-parent / cause parent-blocking-index-build eligible still eligible — cause is on the wire

The emitted record for the first row is exactly the one this PR publishes at cli-output-examples.md:226-236:

{"outcome":"executed-without-online-safety","reason":"index-statement","class":"by-design",
 "blocking_passthrough":true,"lock_timeout":"3s","statement_timeout":"10m"}

Nothing in it distinguishes the eligible index-single-relation site from the ineligible index-other site, so the record of a legitimate accepted execution is byte-identical to the record a buggy or hand-written one would produce for DROP INDEX a, b. Refusal() returns no error on the decoded verdict — the reconstruction silently answers a narrower question than the one asked.

RF-8 anticipates half of this and forbids the wrong fix: "Accepted-blocking eligibility is therefore consumed only from the proof of the verdict the same front-door invocation produced, never from a decoded verdict." That rule is right, and it is why a decoded verdict must never authorize anything. But it was written about eligibility as a decision input, and this PR introduces the verdict as an output — the audit record of an override that already happened. Those are different obligations: the input must not be forgeable, the output must be complete. Keeping the site off the wire satisfies the first by sacrificing the second, and the tension dissolves once they are separated — recording site as a descriptive field creates no authorization path, because RF-8 already forbids consuming eligibility from a decoded verdict regardless of which fields are present.

So the choice is between two defensible states, and the PR should pick one:

  • Put the site on the wire for this outcome (it is already a closed vocabulary with a String()), and amend RF-8's "cannot recover the unexported refusal site" clause, which this change would make false for accepted-blocking verdicts.
  • Say the record is not self-verifying, and name what an auditor reads instead — the plan report's blocking_passthrough_eligible, or a server-side log line — so the gap is a decision rather than an omission.

The current state is the third thing: a record that looks self-describing, with a comment citing an invariant that covers the in-process half only.

2. The renderer parity lock reports coverage it does not have

fullVerdict (color_test.go:164) exists to make coverage structural — its comment says the reflection check "fails when a field is added to Verdict without setting it here, so a field rendered by only one of the two renderers cannot slip past the parity test in a shape no realistic fixture sets." This PR adds BlockingPassthrough, LockTimeout, and StatementTimeout to the fixture (:182-184) and the reflection check passes.

But the fixture's outcome is OutcomeFailed, and every renderer branch that reads those three fields is gated on Outcome == OutcomeExecutedWithoutOnlineSafety. So the fields are set, the non-zero assertion is satisfied, and neither renderer emits them — the lock's guarantee is about field presence in the fixture, and the new fields' rendering is gated on a field the fixture also sets, to a different value.

Three mutations demonstrate it. Deleting the budgets line from writeVerdictText:42-44 leaves the suite green. Changing outcomeHeadline:76 from severity("warning", …) to severity("help", …) — the same severity executed natively uses, i.e. rendering an accepted blocking execution as an ordinary success — leaves the suite green. And no test anywhere constructs a verdict with the new outcome and renders it: OutcomeExecutedWithoutOnlineSafety appears in exactly one test line, verdict_test.go:51, which asserts the field, not the rendering. The four verdicts in TestVerdictTextColorWrapsLabelsOnly are executed, refused, failed, and mystery.

That means the entire new text rendering — headline, severity, the refusal: line, the budgets line — is unpinned in both renderers, and the docs/lock-budgeted-passthrough.md:251 text block that this PR hand-reordered to match the code has nothing verifying it. TestCLIOutputExamplesMatchPipelineOutput covers only the ```console … --json blocks.

The fix is one entry, not a new harness: add a fifth verdict with Outcome: OutcomeExecutedWithoutOnlineSafety and the three fields to TestVerdictTextColorWrapsLabelsOnly's list, which pins writeVerdictText against verdict.String() byte for byte and immediately covers all four branches in both. Worth also reconsidering fullVerdict's outcome: as long as one fixture must set every field at once, the outcome it picks silently decides which branches the parity lock can reach, and the comment claiming otherwise is the part that makes this easy to miss again.

3. Exit 3 is the headline of the contract and nothing tests it

Changing main.go:31 to os.Exit(verdict.ExitCodeRefused) leaves the suite green. verdict_test.go:110 pins that the two sentinels are distinct (errors.Is(ErrRefused, ErrAcceptedBlocking) is false), which is the right check and not the whole of it — nothing asserts the constants' values or that the mapping in main sends each sentinel to its own code.

That matters more here than the usual "main is untestable" case, because the values are the contract. verdict.go:23-25 states it — "it cannot be confused with online-safe exit 0" — and cli-output-examples.md:211 publishes "exit 3" to operators who will branch on it in CI. Two assertions cover the part that does not need a process: ExitCodeRefused == 2, ExitCodeAcceptedBlocking == 3, and neither equal to 0 or 1. The main mapping itself is three lines and could be a small exitCode(err) int helper with a table test, which is what would have caught the mutation above.


Smaller notes

  • Refusal()'s outcome guard has no test. Replacing :379's condition with if false leaves the suite green. It is mostly self-healing — a non-refused verdict usually has no refusal fields, so NewRefusal errors anyway — but not always: Verdict is exported and populatable, and fullVerdict itself is an OutcomeFailed verdict carrying Class, Reason, and Owner that validate together, so under the mutation it becomes a refusal. One line asserting Verdict{Outcome: OutcomeExecuted}.Refusal() errors closes it.
  • TestReportJSONShape shows a refused statement with no blocking_passthrough_eligible. Its third statement sets Disposition: DispositionRefuse directly, so the field stays nil and the "exact keys, exact omissions" contract test asserts its absence on a refusal — while plan-report.md:75 lists it as "refusals only." The same is already true of class there, so this is consistent with the existing fixture rather than new, and absence reads as false in every consumer, which is the ineligible and therefore safe direction. Worth a note in the test that the fixture is hand-built and does not exercise refuseStatements, since the field's presence is coupled to going through that function rather than to being a refusal.
  • Refusal text output is reordered for every refusal, and nothing pins it. table: now precedes class:/owner: (verdict_text.go:20-30, mirrored in verdict.String()). This is an improvement — refusals were the one outcome that put the table second — but it is a user-visible change to existing output that the PR summary does not mention, and because both renderers moved together the parity test cannot see it. No doc shows a refused text rendering, so nothing went stale; a golden for the refused case would be worth having before the next reorder.
  • formatDuration is round-trippable, which is the property that matters for a value published in JSON: 3s, 10m, and 1h all parse back through time.ParseDuration. Good.
  • new(verdict.AcceptedBlockingEligible(r)) (plan.go:255, :351) is Go 1.26's new(expr), and go.mod declares go 1.26 with CI resolving the toolchain from it, so this is fine — worth knowing it is the first use in the repo and pins the floor.

This review was generated by Claude Code (claude-opus-5).

@aparajon

Copy link
Copy Markdown
Collaborator

🤖 2/2 — the invariant registry, the wire contract, and the stack. The code and its coverage are in 1/2.

This change adds a terminal outcome, a process exit code, a versioned wire field, and a third site that consumes accepted-blocking eligibility — and it touches docs/invariants.md not at all. Three of the four items below are about that.

1. RF-8 gains a third enforcement site and RF-7's registry pointer now resolves past the code

WithAcceptedBlocking carries an INV: RF-8 comment at verdict.go:345-346 and is the function that decides, for the first time in shipped code, that a refusal may be executed. RF-8's *Enforced:* line names Verdict.WithRefusal, Verdict.Refusal, and three tests. It does not name WithAcceptedBlocking, which is now the only place the rule "eligibility is consumed only from the proof" is applied to an actual authorization rather than asserted about a reconstruction.

RF-7 has the sharper version of the same problem. The registry's own preamble is what makes this matter: it says this doc states "where each rule is enforced," so an *Enforced:* line is the thing a reader follows instead of re-deriving the rule. Its line says the classification registry is "(pkg/plan/refusal.go, pkg/migrate/refusal_registry.go)". This PR deletes acceptedBlockingDecision from the second of those and recreates it as pkg/verdict/accepted_blocking.go, so a reader following the pointer to pkg/migrate/refusal_registry.go to find the eligibility half of the registry finds it gone. The pointer still resolves to a real file — that is the failure mode worth naming, because a dangling path gets noticed and a path that resolves to the wrong file does not. The completeness harness the same line names, TestRefusalRegistryIsComplete, stays in pkg/migrate and still calls through (refusal_registry_test.go:118), so the guarantee is intact and only the citation is wrong. Both are one-line edits in this PR.

The move itself is right and I said so in 1/2 — pkg/verdict is where an executor can reach it. The registry should follow it.

2. A new terminal outcome and a new exit code arrive with no entry

executed-without-online-safety is the first outcome that means committed, and the engine does not vouch for it, and exit 3 exists so that state cannot collapse into exit 0. That is a safety property in the registry's own sense: it is what stops automation from reading a blocking execution as an online-safe one. Nothing in docs/invariants.md says it.

The rule writes itself from verdict.go:20-26 and is true of shipped code today:

Committed execution without an online-safety guarantee is reported as its own outcome and its own exit code; exit 0 is reserved for online-safe success, and exit 2 for refusal. Enforced: verdict.WithAcceptedBlocking, the ExitCode* constants, and the sentinel mapping in cmd/pg-sprite.

Written that way it also names what 1/2's finding 3 shows is untested, which is the usual value of adding the entry in the same PR. The --accept-blocking front door is still step 4 and its invariants belong there, but this outcome and this exit code ship here and are reachable by any in-tree caller of WithAcceptedBlocking — the same reachability argument the AB-1/AB-2 marker makes about the executor in #107.

If the intent is that the whole family lands together at step 4, the rollout section should say so explicitly rather than leaving the outcome undocumented in the registry between now and then.

3. The published console block runs flags that do not exist, and its JSON did not come from the pipeline

cli-output-examples.md:225 publishes:

$ pg-sprite migrate --alter 'DROP INDEX public.users_email_idx' --accept-blocking public.users --lock-timeout 3s --statement-timeout 10m --json

The prose two lines above is honest that "nothing produces this outcome until --accept-blocking lands in a later change," so this is disclosed rather than hidden. But the page's premise is captured output, and the test that guards it is TestCLIOutputExamplesMatchPipelineOutput — and for this one block the "pipeline" is a struct literal (docs_test.go:94-101) while the other nine come from alterReport(t, …) and real plan construction. So the JSON is pinned and the command line above it is not, cannot be, and is currently wrong: a reader who runs it gets an unknown-flag error.

It also quietly pins a flag spelling--accept-blocking <table> plus --lock-timeout and --statement-timeout — that no code enforces, so step 4 either matches three invented names or silently makes this page wrong again. The block-discovery regex requires a $ pg-sprite … --json line, which is what forced the invention; relaxing that to admit a JSON block without a command is the fix, not choosing better flag names. Failing that, a comment on the require.Len(t, blocks, 10) at docs_test.go:78 saying which block is synthetic would at least keep the next reader from trusting the command.

4. blocking_passthrough_eligible is the operator's decision input and sits outside the fingerprint

plan-report.md:75 classifies the new field as "explanatory and excluded from the fingerprint," and that is accurate — Fingerprint hashes only sql, route, backend, disposition, and exec_sql.

It is also the one new field whose whole purpose is to change what an operator does next: it is how they learn that --accept-blocking is available for this refusal. The fingerprint exists so that the plan someone reviewed is the plan that runs, and two reports for the same refused statement with different eligibility now have the same fingerprint — a plan reviewed while the field read false still matches after a catalog change flips it to true, with no plan-fingerprint-mismatch.

This is not a hole in the engine's gate: eligibility is decided in process from the live refusal at execution time, and the operator has to type the flag on that invocation, so nothing stale can authorize anything. It is a question about the review surface, and it is worth answering in the doc rather than leaving the classification to be inferred: either the field is explanatory because the live gate is the authority (in which case say that where the field is defined, since "explanatory" currently sits next to cause and class, which nobody acts on), or an operator's go/no-go input belongs inside the integrity envelope.

Smaller notes

  • The presence wording for the new field is weaker than its neighbours'. class and owner say "Present exactly when …"; blocking_passthrough_eligible says only "refusals only" in the presence column. Given 1/2's note that a refused statement built outside refuseStatements omits it, the weaker phrasing is arguably the accurate one — but then it should say what it means, because a consumer reading the column alongside class's row will take it as the same claim.
  • Cause is promoted to refusal identity without a closed-set function. The new doc comment at verdict.go:426-430 says "The accepted-blocking registry keys on it, so a cause is part of the refusal identity, not a rendering detail." Reason has Reasons() and Class has Classes(), each pinned by test; Cause has neither. In practice TestRefusalRegistryIsComplete derives its keys from preflight.PartitionRefusalCauses() and the default: return false, false arm fails closed, so an unclassified cause is ineligible rather than dangerous. Still, a field that now keys an authorization decision is the one place a closed vocabulary earns its keep, and the doc change is what makes the asymmetry visible.
  • The format_version 5 bump is done properlyplan-report.md:20, all nine example payloads, TestReportJSONShape, TestReportJSONOmitsUnsetOptionalFields, and the TestSourcesVocabularyPinned comment all move together, and TestExistingVerdictJSONIsUnchanged pins that no existing verdict key changed shape. That is the part of a wire-contract change most likely to be half-done and it is not.
  • The not-native-safe-budget-exceeded row rewrite (cli-output-examples.md:90) is a real clarification: cause carrying both the budget vocabulary and the partitioned-parent shape, distinct from the create-shape cause on plan statements, was genuinely confusing with three same-named fields in play.
  • The branch merges main rather than rebasing. 7aba874 is a merge of 231ab81 into the feature branch, so git log origin/main.. still lists the pre-squash a109572 and the PR reads as though feat: classify accepted-blocking refusal eligibility #105's work were part of it. The effective delta is the 16 files I reviewed. Same note as feat(executor): run accepted blocking statements under both budgets #107; worth resolving once for the stack rather than per PR, since feat: classify accepted-blocking refusal eligibility #105 landed as a squash and the duplicate commit will keep reappearing in everything based on that branch.

Approving. Items 1 and 2 are the ones I would do before merge — both are edits to docs/invariants.md in this PR rather than code changes. Item 1 is the quieter of the two and the one I would not want to leave: nothing about moving a file looks safety-relevant, so an *Enforced:* line that still resolves to a real file is the kind of wrong pointer that survives review indefinitely. Item 3 is a small doc-honesty fix; item 4 is a question for the author, not a defect.

This review was generated by Claude Code (claude-opus-5).

@aparajon aparajon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving. Reviewed in two comments above — the contract and its coverage in 1/2, the invariant registry and wire contract in 2/2. Fifteen mutations plus a round-trip probe of the audit record; full unit suite green, lint clean, CI 15/15.

provenRefusal is the right fix in the right place, and moving the eligibility registry into pkg/verdict is more than a file move — it is what lets an executor ask the question at all, which is the dependency direction #107 needed.

Four findings: the JSON audit record cannot be checked against the registry for the site-keyed row (site is not on the wire, and the decoded verdict re-derives as ineligible with no error — the published example is exactly that row); the renderer parity lock sets the three new fields on an OutcomeFailed fixture, so neither renderer emits them and the whole new text rendering is unpinned in both; exit 3 has no test though "cannot be confused with exit 0" is the contract's headline; and no docs/invariants.md change, which leaves RF-7's registry pointer resolving to a file the accepted-blocking half just left.

This stamp was left by Claude Code (claude-opus-5).

@Kiran01bm

Kiran01bm commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/108, follow-up commit

All three findings are fixed in the follow-up commit, which also merges origin/main (the squashed registry PR landed with the IndexTarget.String() and RF-8 refusal-proof changes) and removes the pkg/migrate copy of the eligibility registry that the merge would otherwise have reintroduced beside the pkg/verdict move.

# Finding Status Explanation
1 demo/tour.sh asserts plan and diff format_version 4 while pkg/plan.FormatVersion is now 5, so make demo-check fails on this branch Fixed All four plan and diff assertions in demo/tour.sh now expect 5; the lint (1) and suggest (2) assertions are separate contracts and are unchanged. make demo-check against the compose Postgres passes end to end.
2 The Cause type and Verdict.Cause field comments, and the not-native-safe-budget-exceeded row in docs/cli-output-examples.md, still describe cause as a budget-only vocabulary although the registry keys partitioned-parent refusals on it Fixed The type comment now says Cause narrows a refusal reason to its typed discriminator — the budget that fired under budget-exceeded, or the parent shape refused under unsupported-partitioned-parent — and that the accepted-blocking registry keys on it, so it is refusal identity rather than rendering detail. The const block comment and CauseNone comment match. The field comment adds that the cause is preserved on an accepted-blocking verdict. The doc row now states the same field carries the partitioned-parent shape and keeps the note that both are unrelated to the plan statement's create-shape cause.
3 The docs/lock-budgeted-passthrough.md text-renderer example lists statement: before refusal:, but both Verdict.String() and the CLI renderer print refusal: first Fixed The example now reads table:, refusal:, statement:, safer:, budgets:, matching the order both renderers emit; docs/cli-output-examples.md already had the correct order and is unchanged.

Source: #108, scratch review scratch/code-reviews/pg-sprite-pr108-review.md at head e8b7d43a; fixes in 7aba874

@Kiran01bm
Kiran01bm enabled auto-merge (squash) September 11, 2026 07:16
@Kiran01bm
Kiran01bm merged commit 4372f2c into main Sep 11, 2026
15 checks passed
@Kiran01bm
Kiran01bm deleted the kiran01bm/pt4-accepted-blocking-verdict branch September 11, 2026 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants