Skip to content

docs: pin the four-code exit ladder in the passthrough design - #103

Merged
Kiran01bm merged 4 commits into
mainfrom
kiran01bm/pt2-exit-code-ladder
Sep 11, 2026
Merged

docs: pin the four-code exit ladder in the passthrough design#103
Kiran01bm merged 4 commits into
mainfrom
kiran01bm/pt2-exit-code-ladder

Conversation

@Kiran01bm

Copy link
Copy Markdown
Collaborator

Follow-up to #102: state the full exit-code ladder the passthrough introduces, how exit 3 is produced, and every doc that must gain it when the behavior ships.

Why

The merged design names exit 3 but leaves the reader to infer the rest of the ladder: it never says which code a mismatched acknowledgement gets, and it never says how a successful verdict can leave a non-zero status when the entry point only maps ErrRefused today. Rollout step 3 also pointed at cli-output-examples.md alone, while the exit-code contract is stated in three more places that would silently keep describing a three-code ladder.

What

  • Adds an exit ladder table (0 / 1 / 2 / 3) to the verdict section, each row answering "did the statement run?" and "online-safe?"; a mismatched or unresolvable acknowledgement is pinned as a usage error, exit 1.
  • States that exit 3 lands as a constant and sentinel in pkg/verdict beside ExitCodeRefused / ErrRefused, mapped in the entry point the same way, because kong's error path would otherwise print the success as a failure and exit 1.
  • Rollout step 3 now lists every exit-code surface to update in the shipping change: cli-output-examples.md (new section and header paragraph), the exit-codes bullet in execution-model.md, the dry-run exit-code paragraph in postgres-online-ddl-reference.md, and the README's exit-code gate paragraph.

Before / after

One thing changes: the design now answers what an implementer of step 3 ships for exit codes and where the contract is written. Nothing about eligibility, budgets, or the verdict shape changes. The example is an operator running pg-sprite migrate --accept-blocking app.orders against DROP INDEX app.other_idx (an index on a different table), then a CI reader checking the docs.

Before — the design as merged in #102

  --accept-blocking app.orders + DROP INDEX app.other_idx
        │
        ▼
  index resolves to app.other, not app.orders ──▶ "usage error, nothing runs"
                                                   (no exit code stated;
                                                    implementer must guess 1 or 2)
  passthrough commits ──▶ "exit 3"
                          (no mechanism stated; main.go maps only ErrRefused → 2,
                           so a naive success verdict would exit 0)
  step 3 updates ──▶ cli-output-examples.md
                     execution-model.md            still says "1 failure, 2 refusal"
                     postgres-online-ddl-reference  still says "0 or 2"
                     README gate paragraph          still says "0 or 2"

After — this PR

  --accept-blocking app.orders + DROP INDEX app.other_idx
        │
        ▼
  index resolves to app.other, not app.orders ──▶ usage error, nothing runs, exit 1
                                                   (ladder row 1: "mismatched or
                                                    unresolvable acknowledgement")
  passthrough commits ──▶ exit 3 via a second sentinel + constant in pkg/verdict,
                          mapped in main.go beside ErrRefused → ExitCodeRefused
  step 3 updates ──▶ cli-output-examples.md        new "— exit 3" section + header
                     execution-model.md            exit-codes bullet gains 3
                     postgres-online-ddl-reference  dry-run paragraph gains 3
                     README gate paragraph          says all-non-zero gates stay fail-closed

The ladder also settles that a refusal for an ineligible statement, a missing acknowledgement, or an exhausted lock budget stays exit 2 with nothing run.

Add the exit-code ladder (0 online-safe, 1 failure or usage error,
2 refused with nothing run, 3 committed through the accepted blocking
passthrough) to the verdict section, with the question each code answers,
and state how exit 3 is produced: a constant and sentinel in pkg/verdict
beside ExitCodeRefused, mapped in the entry point, because a success
verdict that must leave a non-zero status cannot ride kong's error path.

Rollout step 3 now names every place the exit-code contract is stated
today — cli-output-examples.md, execution-model.md,
postgres-online-ddl-reference.md, and the root README's gate paragraph —
so they gain exit 3 in the change that ships it.

🤖 Generated with Amp (Claude Opus 4.6)
@Kiran01bm
Kiran01bm marked this pull request as ready for review September 10, 2026 21:25
@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 ladder itself. Rollout-step completeness and docs are in 2/2.

Reviewed ca13da71..ea5ca199 (one file, +28/−3). Mutation testing does not apply to a design doc, so the oracle I used is the same one as for #102: every code citation must resolve to what the doc says it does, and every claim about behavior must be true of the engine as shipped. The mechanism half is exactly right. One finding on the taxonomy, and it is the one worth acting on.

Verified at source:

  • "The exit code is produced the way exit 2 is today: migrate returns a typed sentinel after printing the verdict, and the entry point maps that sentinel to the code" is a faithful restatement of both halves. internal/cli/migrate.go:105-123emit prints, then returns verdict.ErrRefused — and cmd/pg-sprite/main.go:24-30 maps it before k.FatalIfErrorf. pkg/verdict/verdict.go:23-24 even describes itself in those words: the sentinel "the CLI returns after printing a refusal verdict, so the entry point can map it to ExitCodeRefused."
  • The kong claim holds for the reason given. main.go:30 is the only other terminal path, so without a second sentinel an exit-3 verdict would reach FatalIfErrorf and be printed as an operational failure at exit 1. Naming the constant and the sentinel in step 3, rather than leaving "exit code 3" as an abstraction, is the difference between a design a reader can implement and one they have to re-derive.
  • Extending exit 1 to "an operational or usage error before it … (nothing ran)" agrees with the code's own definition: verdict.go:18-20 calls 1 "an operational error (could not connect, bad flag, SQL error)." This settles the acknowledgement-rejection question from the docs: decide the lock-budgeted passthrough contract #102 review — usage errors sit at 1, and the row says so explicitly rather than leaving the reader to infer it from FatalIfErrorf.
  • #dry-run-diagnostic-codes resolves (postgres-online-ddl-reference.md:285), and all three named files exist.

1. Row 1 moves statement-budget cancellation from exit 2 to exit 1, and the doc presents that as the status quo

Row 1 puts "statement-budget cancellation after execution started (attempted, rolled back)" at exit 1. Today that same event is exit 2:

// pkg/migrate/verdicts.go:172-201  (budgetVerdict)
}.WithRefusal(budgetExceededRefusal())      // outcome: refused
switch budgetErr.Cause {
case executor.CauseStatement:
    v.Cause = verdict.CauseStatementBudget
    // "cancelled after the %s statement budget: …"

and exit 2 follows mechanically, because emit keys on the outcome alone (internal/cli/migrate.go:120-122: if v.Outcome == verdict.OutcomeRefused { return verdict.ErrRefused }). So this is not an exit-code mapping to adjust — the verdict outcome for an existing case has to change from refused to failed, and step 3's list of contract additions does not mention it.

Three shipped statements say the opposite of row 1, and all three are load-bearing:

  • docs/optimistic-attempt.md:230, case 3b — "Statement ran past statement_timeout" → reason: not-native-safe-budget-exceeded, cause: statement-budget, i.e. a refusal, with the online-safety column reading "No — rolled back cleanly."
  • docs/optimistic-attempt.md:345 — "a budget error is a refusal input, not an operational failure."
  • docs/optimistic-attempt.md:529 — "A budget refusal is not a failure. Budget and size errors are refusal inputs."

The sentence immediately above the new table (:279-280, unchanged here) reads "1 remains execution failure, and 2 remains refusal with nothing run." With row 1 as written, "remains" is doing work it cannot do: one case moves. Rollout step 4 gives away that the move is intended — it schedules a demo assertion for "statement-budget failure" — so the design has decided this; the doc just does not say it is a decision.

Two ways to close it, and they are materially different:

  • If the reassignment is only about the passthrough path, scope row 1's budget clause to it. On the passthrough there is no alternative strategy to fall back to, so a cancelled blocking statement really is a failure rather than a routing input, and that reasoning deserves to be in the doc. The optimistic path then keeps case 3b at exit 2 and nothing else changes.
  • If it applies to every path, say so at the top of the section as a behavior change, add the outcome change to step 3's contract list, and add optimistic-attempt.md to step 3's document list — it is the doc that owns the contradicted rule, and it states it in three places.

Either way this is the finding to resolve before the doc is treated as pinned, because a consumer branching on the ladder as written would expect exit 1 on a statement-budget cancellation and get exit 2 from every non-passthrough run.

2. The new "Did the statement run?" column is the right axis, and it immediately catches exit 2 overclaiming

Row 2 answers "no" for exit 2. That is true of every cause the row lists, but not of exit 2 as a whole today: cause: statement-budget is a refusal whose statement ran before being cancelled — optimistic-attempt.md:230 says as much with "rolled back cleanly," and verdict.go:301-303 describes it as "the statement ran past statement_timeout and was cancelled."

That inaccuracy is not introduced here; it is inherited by the two glosses this doc builds on — execution-model.md:174-175 ("a refusal — where nothing was ever attempted — exits 2") and refusal-classes.md:9 ("Exit code 2 means nothing ran"). But adding a column whose entire job is to answer that question makes this the doc where the imprecision becomes visible, and it is the doc claiming to pin the ladder. The honest distinction is nothing committed rather than nothing ran, which is the phrasing low-level-design.md:821-823 already uses ("distinguish nothing-committed from partial state left behind") and which stays true whichever way finding 1 is resolved.

3. The table reads as the binary's ladder, but two rows are migrate-only

Row 0 already reaches past migrate — "or a dry run found every statement executable" — so a reader takes the table as the process contract for the whole binary. Exit 2 has four producers: internal/cli/migrate.go:121, diff.go:66, dryrun.go:104, and pull.go:184. Row 2's cause list ("ineligible, no acknowledgement supplied, or the lock budget was exhausted") covers only the first, and pull.md:34 documents its own three-code ladder independently.

A clause fixes it either way: name the table as migrate's ladder and drop the dry-run clause from row 0, or keep it binary-wide and say that every command's refusals share exit 2. The second is more useful to a CI author, who is gating on a process status and does not care which subcommand produced it.


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

@aparajon

Copy link
Copy Markdown
Collaborator

🤖 2/2 — the rollout step's completeness claim. The ladder itself is in 1/2.

The most valuable thing in this diff is the sentence that turns a vague "update the docs" into an enumeration: "The exit-code contract is stated in three more places that today describe a three-code ladder and must gain exit 3 in the same change." That is a completeness claim, so it is testable — I grepped every markdown file in the tree for exit-code statements and checked each hit against the list.

The three named places are all real and all describe a three-code ladder, and the one that would be easiest to miss is already covered:

  • docs/execution-model.md:174-175 — the exit-codes bullet, 1 and 2 only.
  • docs/postgres-online-ddl-reference.md:291-295 — the dry-run paragraph, 0 and 2, and the anchor resolves.
  • README.md:138 — the exit-code gate paragraph. Requiring it to state that a gate treating every non-zero status as failure stays fail-closed is the right instruction: it is the one place a reader arrives with no context, and "we added a new non-zero code" is exactly the change that would otherwise read as a new way for CI to go green.
  • docs/cli-output-examples.md:22-28 also states the contract, and step 3 names it separately — "the exit-code contract paragraph at its head" — so it is not a miss.
  • docs/pull.md:34 states a full three-code ladder of its own and is correctly absent: pull never executes DDL, so exit 3 cannot occur there.

Two hits the list does not account for:

  • docs/low-level-design.md:821-823 frames the whole exit-code space as failure-versus-refusal, and does it for precisely the reason exit 3 breaks: "an execution failure ends in a failed verdict (exit 1, distinct from the refusal exit 2) … so automation can distinguish nothing-committed from partial state left behind." Exit 3 is a third case — committed, and not vouched for — so that sentence becomes incomplete in the same change, and it is the sentence an embedder reads before writing their branch. (It is also the one place already using nothing-committed rather than nothing ran, which is the wording I argue for in 1/2.)
  • demo/README.md:38 enumerates the codes outright: "on exit codes (0 success, 2 refusal, 1 lint gate)." Step 4 covers the demo assertions, but not this line.

Neither is a large edit. I raise them because the value of the enumeration is that it is exhaustive — a reader who trusts "three more places" will not re-grep, and a stale ladder in low-level-design.md is worse than a vague instruction would have been, since that doc is where library consumers go.

While the list is being revised, optimistic-attempt.md belongs on it too if finding 1 in 1/2 resolves as a whole-binary change; it states the contradicted rule three times.


On the rest, which is accurate.

  • Step 3's expansion is a real improvement over what it replaced. "exit code 3" as a bare item left the mechanism to the implementer; naming the constant and sentinel beside ExitCodeRefused and ErrRefused, plus the mapping site, makes it a step someone can execute — and it matches how exit 2 actually works (pkg/verdict/verdict.go:18-25, internal/cli/migrate.go:105-123, cmd/pg-sprite/main.go:24-30).
  • Naming the new cli-output-examples.md section as executed-without-online-safety — exit 3 matches that file's existing heading convention exactly (:209, :251, :287 all read … — exit 2), so the generated anchors will be consistent with the ones already in its table of contents.
  • The table's fourth column is the right thing to publish. "Online-safe?" being not applicable for 1 and 2 and no for 3 is the distinction the whole design exists to preserve, and putting it in a column makes it impossible to read exit 3 as a variant of success.
  • The four-row table plus "Exit 3 is the only code where the statement committed and the engine does not vouch for online safety, so a consumer can read it without JSON" is a better argument for the distinct code than the paragraph above it, which argues from what shell CI would have to parse. Both are true; the new one is the one a reviewer of the design needs.

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. The mechanism half checks out against the code — the sentinel-and-constant description matches how exit 2 actually works, and the kong reasoning is right — and this settles the acknowledgement exit-code question from the #102 review.

Findings are in the two comments above. The one to resolve before the ladder is treated as pinned is finding 1: row 1 puts statement-budget cancellation at exit 1, where today it is a refusal at exit 2, and optimistic-attempt.md states the opposite rule in three places.

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

@aparajon

Copy link
Copy Markdown
Collaborator

🤖 Re-reviewed the move ea5ca199..bb3048dc. It is a main merge picking up #81, #99 and #101, and this PR's own diff is byte-identical across it — same one file, same +28/−3, and docs/lock-budgeted-passthrough.md hashes the same at both heads. So the three findings in 1/2 and 2/2 stand exactly as written and nothing new is owed by the merge itself.

The one thing a main move can invalidate here without touching the diff is the completeness claim at :419 — "the exit-code contract is stated in three more places" is a statement about the tree, not about this file, so I re-ran the enumeration against the new base 2c2dfc6.

The surface set is unchanged. Twenty markdown files state an exit code at ca13da71 and the same twenty at 2c2dfc6 — no additions, no removals. Among them only docs/cli-output-examples.md changed at all (#99's capabilities section, exit 0), which step 3 already names. So the merge neither broke nor extended the list.

Two things I can add now that I could not last time.

replay/ is correctly absent, and #101 is what makes that worth saying

replay/replay.sh hard-codes the ladder in five places — status -eq 0 at :192, status -eq 2 at :201, :212, :227 and :240 — and #101 (merged inside this move) tightened the refusal branch further, so :227 now requires the reason, the engine-emitted class and the owner to match. That reads at first like a sixth surface the enumeration missed.

It is not, and for the same reason docs/pull.md is not: exit 3 is unreachable there. The replay invokes migrate --url --json --alter with no --accept-blocking (replay.sh:173), and the whole design gates the passthrough behind that flag plus a matching acknowledgement, so the harness cannot produce a 3. Worth noting that it would also fail in the safe direction if it somehow did — a 3 falls through every branch, lands as FAIL, and skips both psql_apply calls, which is right, because unlike a refusal an exit-3 statement already committed and must not be re-applied. That is a genuine property of the harness rather than luck, and it is the kind of thing the enumeration is implicitly asserting about every surface it leaves off.

docs/capabilities.md is a third unaccounted surface, and I missed it in 2/2

My 2/2 claimed to have grepped every markdown file and named two hits the list does not cover (low-level-design.md, demo/README.md). There is a third, and it is the most exposed page in the set:

  • :51-53 — "migrate and its dry-run exit with code 0 only when the change is executable through an online-safe path; a refusal exits 2 with a typed reason. CI can gate on the exit code alone." A two-code ladder, stated as the product's core promise.
  • :62 — the T2 row's What you see today column reads "A typed refusal naming the reason, exit 2 — never a silent fallback to a blocking form." Once this design ships, a T2 change under an explicit acknowledgement yields exit 3. The row stays true in spirit — the acknowledgement is exactly what makes it not silent — but a reader mapping tiers to codes gets the wrong answer, and the tier table is the first thing on that page anyone reads.

What makes this one worth adding to step 3 rather than filing separately is that capabilities.md is the page that argues the exit-code contract is the product (:291-292), so it is where an outside reader decides what a non-zero status from this binary means. Its ## Why typed refusal, not passthrough section is already correct for this design — :306-311 describes the constrained variant as planned, in terms that match the doc under review — which is precisely why the two stale code citations above it are easy to walk past.

Both edits are prose above the first generated marker (:144), so #100's make check-capabilities gate does not constrain them; no YAML change is needed. That does mean capabilities.md should be named explicitly in step 3, since "regenerate the page" would not produce either edit.

Small note on CI

The board is green and all-green passed, with lint, build, test, smoke test and aws-boundary all SKIPPED by path filter and "unit tests (no Docker; docs guards)" doing the real work. That is the correct shape for a docs-only PR and the guards did run — flagging it only because a wall of SKIPPED next to an approval is worth one sentence of confirmation rather than an assumption.


Re-approving on bb3048dc. Nothing here is new blast radius from the merge; finding 1 in 1/2 (the statement-budget row moving from exit 2 to exit 1) is still the one to resolve before the ladder is treated as pinned.

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.

🤖 Re-approving on bb3048dc. The merge is content-free for this PR — its own diff is byte-identical across the move — and I re-ran the :419 completeness enumeration against the new base: the exit-code surface set is unchanged at twenty files. Two additions in the comment: replay/ is correctly absent (exit 3 is unreachable without the flag, and the harness fails in the safe direction anyway), and docs/capabilities.md is a third unaccounted surface that I missed last pass — it states the two-code ladder at :51-53 and :62, in prose above the generated markers, so step 3 should name it.

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

@Kiran01bm

Copy link
Copy Markdown
Collaborator Author

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

Verdict: every finding from all three rounds is fixed in the follow-up commit; the exit-1 reassignment is scoped to the passthrough with its reasoning stated, the table asks what committed, and rollout step 3 now enumerates every surface the sweeps found — including capabilities.md — and records what already shipped.

# Concern Status Explanation
C1-F1 Row 1 moves statement-budget cancellation from exit 2 to exit 1 and presents it as the status quo; optimistic-attempt.md states the opposite rule three times. Restated by C3 as the one to resolve before the ladder is treated as pinned fixed Resolved as passthrough-only. Row 1 reads "On this path only, a statement-budget cancellation is also a failure", and a paragraph under the table gives the reason: on the optimistic attempt a budget overrun is a routing input the engine refuses and redirects, but the operator on this path has already accepted the blocking strategy and there is no further route, so a started-and-cancelled statement is a failed attempt. Every other path keeps exit 2; optimistic-attempt.md is linked as unchanged. Step 3 records that the passthrough produces a failed verdict with the executor's budget-statement-exceeded code, which is what the shipped executor path emits for SQLSTATE 57014.
C1-F2 "Did the statement run?" answers "no" for exit 2, but a statement-budget refusal ran and rolled back fixed Column renamed "Anything committed?", true for every exit-2 producer; the sentence above the table reads "2 remains refusal with nothing committed". Row 1's cell notes the committed prefix a failed sequence leaves in executed_sql. Step 3 schedules tightening the inherited "nothing ran" glosses in execution-model.md and refusal-classes.md to "nothing committed".
C1-F3 The table reads as the binary's ladder but two rows are migrate-only fixed Kept binary-wide. The lead-in states that refusals from migrate, diff, a dry run and pull share exit 2 and only migrate produces exit 3 because no other command executes DDL; row 2's cause list ends "or any other command's typed refusal".
C2-F1 low-level-design.md frames the exit-code space as failure-versus-refusal and is missing from step 3's enumeration fixed Listed, with the instruction that the sentence must add the committed-and-not-vouched-for case.
C2-F2 demo/README.md enumerates 0, 2, 1 and is not covered by step 4's demo-assertion item fixed Step 4 says the demo README's exit-code enumeration gains exit 3 beside the new assertions.
C2-F3 optimistic-attempt.md belongs on the list if C1-F1 resolves as a whole-binary change fixed Moot under the passthrough-only resolution; the doc says so where the difference is explained. Step 3 also records that pull.md's own ladder is deliberately left alone since pull never executes DDL.
C3-F1 docs/capabilities.md is a third unaccounted surface: the "a refusal is a feature" contract bullet states a two-code ladder, and the T2 row's "exit 2" cell becomes exit 3 under an explicit acknowledgement; both sit above the generated markers so make gen-capabilities will not produce the edits fixed Both named in step 3 as hand edits above the markers that make gen-capabilities in step 5 does not touch; the enumeration count is updated to five.

No action: C3's replay/replay.sh analysis (exit 3 unreachable without --accept-blocking; an unexpected 3 falls through to FAIL and skips the psql re-apply, the safe direction) — recorded in step 3 as a surface deliberately left alone, with that reason. C3's confirmation that the surface set is unchanged across the main move, the CI-shape note, and every "verified correct" item in C1/C2 need nothing.

Source: block/pg-sprite#103, review comments 5625849966 and 5625850144 with review 5172572759 at head ea5ca199, and review comment 5627878188 with review 5174001112 at head bb3048dc.

Kiran01bm and others added 2 commits September 11, 2026 17:44
…the surface list

The exit-code table now asks "Anything committed?" instead of "Did the statement run?",
which is the question every exit-2 producer answers the same way, and says that a
statement-budget cancellation is a failure on this path only: the operator has already
accepted the blocking strategy, so there is no route left to redirect to, unlike the
optimistic attempt where the same cancellation is a routing input. The lead-in states that
refusals from every command share exit 2 and only `migrate` produces exit 3.

Rollout step 3 lists every surface that states the ladder — execution-model.md,
postgres-online-ddl-reference.md, low-level-design.md, capabilities.md's hand-written
contract bullet and T2 row, the root README — and names the two left alone on purpose
(pull.md, replay.sh) with the reason exit 3 cannot reach them. Step 4 adds the demo
README's enumeration. Step 3 records what shipped and moves the surface edits and the
"nothing ran" → "nothing committed" tightening to step 4 with the flag.
@Kiran01bm
Kiran01bm merged commit 4951478 into main Sep 11, 2026
11 checks passed
@Kiran01bm
Kiran01bm deleted the kiran01bm/pt2-exit-code-ladder branch September 11, 2026 07:51
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