feat(progress): report the statement each step is executing

[Kiran01bm]

Progress snapshots now include detail.statement: the canonical, qualified SQL the executor is running for the current step.

Why

A greenfield create runs several steps — the CREATE TABLE then each index — and the progress report identified them only by step/total_steps. An observer (the CLI's status view, or an orchestrator polling progress) had to map the step index back to the plan to know what was executing. The executor already holds the exact statement; reporting it removes that indirection.

What

• progress.Detail.Statement (json:"statement,omitempty"), set by Tracker.StartStep(step, operation, statement) and cleared on Finish.
• Every step producer passes its canonical SQL: the create path, runSequence, ExecuteNativeWithProgress, and BuildIndexConcurrentlyWithProgress.
• format_version stays at 1: the field is additive and optional; docs/progress-report.md now states that rule and documents the field.

Before / after

Before                                  After
"detail": {                             "detail": {
  "operation": "brief",                   "operation": "brief",
  "active": true                          "statement": "CREATE INDEX t_v_idx ON public.t (v)",
}                                         "active": true
                                        }

[chatgpt-codex-connector]

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

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 0eb57013.

Verdict: the plumbing is right — every producer reports the statement it actually runs, and a reused tracker cannot leak one. I checked all four call sites against what executes: the create path passes step.SQL() (the qualified, re-parsed statement executeBoundedAttempt receives), runSequence passes step.st.SQL(), ExecuteNativeWithProgress passes st.SQL(), and the concurrent build passes the same sql it hands the server — no display rendering anywhere, matching the field's doc. Start resets detail wholesale, so the statement is cleared on a new run as well as on Finish. Three findings, all about the contract around the field rather than the field's value.

Findings

1. The statement is cleared on the terminal snapshot — the one snapshot a poller most needs it on. Finish wipes Statement while deliberately keeping Operation and Attempt, and the doc justifies it by saying the typed *SequenceStepError carries the SQL. But that error only reaches the in-process caller, and the audience the summary names is "an observer (the CLI's status view, or an orchestrator polling progress)" — for that consumer the field is present during every step that succeeds and absent at the moment the run fails, which is when "which statement was that?" actually gets asked. The doc's own sentence concedes the shape of the problem: step, operation, and attempt remain "so a poller can locate the step in the plan" — i.e. the poller is handed a cross-reference exercise the new field exists to remove. Keeping Statement on a terminal snapshot (or at least on failed) costs nothing and is consistent with the three fields already kept.

2. "Additive optional fields do not bump format_version" contradicts this project's own precedent, and it ships in the PR that first relies on it. docs/plan-report.md says: "The current version is 2: version 2 added the statement-level guidance field" — an optional, explanatory, fingerprint-excluded field, exactly the class this PR declares non-bumping, and it bumped. So the two report contracts now carry opposite additive-field policies with nothing explaining why they differ. Worse, the progress contract's core instruction is that a consumer must reject a version it does not recognize rather than guess at field semantics; a consumer that validates strictly today (unknown-field rejection is the natural reading of that instruction) breaks on the new field with no version signal, and it had no opportunity to adopt "ignore unrecognized optional fields" first, because the rule arrives with the field it licenses. Bumping to 2 makes the rule detectable from the version it was introduced in and costs one constant.

3. statement needs the clamp-and-escape caveat that plan-report.md gives sql. The plan report is explicit: "sql is the submitter's statement and decisions[].operation interpolates catalog identifiers, so a consumer rendering either into a shared surface must clamp and escape them." detail.statement is the same content class — author-written SQL carrying catalog identifiers — and progress is the report most likely to be rendered live into a PR comment or a chat surface, since that is the point of polling it. The new row has no such caveat. One sentence, mirroring the plan report's wording.

Action items

1. (Finding 1) Keep Detail.Statement on terminal snapshots, or at minimum on failed, and pin it in TestTerminalSnapshotFreezesElapsed's table; update the statement row's presence column to match.
2. (Finding 2) Bump progress.FormatVersion to 2 and say in progress-report.md that version 2 added detail.statement and introduced the additive-field rule — or, if the rule is meant to be retroactive, reconcile it explicitly with the plan report's version-2 note so the two contracts don't read as contradicting each other.
3. (Finding 3) Add the clamp-and-escape sentence for statement in docs/progress-report.md.

Verified (tried to break, couldn't)

The four producers pass exactly the SQL that runs, so the field cannot describe a step other than the one executing: step.SQL() in executeCreate is the post-Qualify, post-ParseOne statement handed to executeBoundedAttempt, and runSequence's step.st.SQL() is the admitted step's own statement rather than the submitted form, so a safer sequence reports the sub-step actually in flight instead of the statement it was derived from. StartStep still takes the lock, still resets session/buildPID so a later step cannot poll a stale build, and still sets stepStart from the injected clock — the added parameter changed none of that. Start rebuilds detail from scratch, so a reused tracker reports no statement before its first step and cannot carry a prior run's SQL (the prior-run reset test still holds). omitempty keeps an idle tracker's JSON unchanged, so the "always-on keys only" test is untouched, and the JSON-shape test now pins the field's exact position and value. The integration tests pin the statement on all four paths rather than only the create path. Build clean; ./pkg/progress and ./pkg/executor pass locally at head; CI green on PostgreSQL 15/16/17/18.

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

[aparajon]

🤖 Second pass on the same head (0eb57013).

Lens 1 — outside adopter

Right call and the right shape: the executor already held the statement, the observer was reconstructing it from step against the plan, and a canonical string is cheaper and less error-prone than that mapping. Setting it in StartStep rather than at each call site means a new step producer gets it for free, which is the version of this that stays correct as the executor grows paths.

The gap an adopter feels is finding 1 — the field is there for every step that works and gone on the snapshot that says the run failed. An adopter writing a status view will end up keeping the last non-terminal snapshot in memory purely to recover the statement the terminal one dropped, which is the same indirection the field was added to remove.

Lens 2 — SchemaBot integration

Nothing to change on our side; today the PostgreSQL engine does not read pg-sprite's progress snapshot at all. Engine.Progress (pkg/engine/postgres/apply.go) synthesizes its own result and hardcodes "step": "1", "steps_total": "1", with the comment "Rich server progress is intentionally absent until the PostgreSQL executor exposes it." This PR is a step down that road, so worth naming what we'd want when we wire it up:

• The first thing we need is not statement — it is step / total_steps, which the snapshot already carried. Our 1/1 is wrong for exactly the case this PR's Why section describes, a greenfield create running the CREATE TABLE and then each index.
• statement is what would make our per-table progress honest: we currently report Tables[0].DDL as the whole change's SQL for every step, so a five-step create shows the same DDL five times. Per-step SQL fixes that.
• When we do adopt it, that string has to go through SchemaBot's sanitizeReasonText before it reaches a PR comment — which is the consumer-side half of finding 3, and the reason the caveat belongs in the contract doc rather than in each consumer's tribal knowledge.

The terminal-snapshot gap matters for us specifically: our failure path publishes a terminal progress result and an operator reads the PR comment afterwards, so a statement present only while the step is live is a statement we would never get to show at the moment it matters.

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

[aparajon]

🤖 Approving — the plumbing is correct (all four producers report the statement that actually runs, and a reused tracker can't leak one), and my three findings are follow-ups on the contract around the field: keep it on the terminal snapshot, reconcile the additive-field rule with the plan report's version-2 precedent, and give it the clamp-and-escape caveat sql already has. Details in the two review comments above.

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