Repository navigation
refactor(server): dispatch authored construction lineage through per-operation handlers - #206
Merged
matthewpeterkort merged 1 commit intoNov 13, 2026
Conversation
…operation handlers Extract one private handler per construction operation kind from authoredConstructionOutputsWithCombineInputs (733 lines) into named apply<Operation>ConstructionStep functions that receive an explicit constructionStepContext (document, step, stepIndex, prior schema, authored metadata, source-only projection IDs, compiled output, resolved Combine inputs, capability snapshot, seeded next metadata, and the shared addOutput validator) and return the next metadata map or an error. The owner keeps the shared per-step schema transition: source-projection exclusion set, prior/priorIDsByName seeding with compiled source-stage merge, nextAuthored seeding, addOutput validation and error wrapping, step output-schema validation, clear(authored)/copy-back per-step atomicity, first-failure precedence, and the unsupported-kind default error. Behavior is preserved by construction, not by assertion tuning: 569 case-body lines moved line-exact under the only two sanctioned rewrites (two-tab dedent; single-value error returns gain a nil first value), all owner-retained regions line-exact, and the function-region string-literal multiset identical, verified against the base blob by a rerunnable transformation plus a correspondence checker. Verification: go test ./internal/server -count=1 gives the identical test-level result as the base (695 pass, 33 service-gated skips, 0 fail); focused runs TestAuthoredConstructionOutputs|TestDeriveSourceProjection| TestExplorerReceipt (9 pass) and the operation-contract regex (28 pass) match base; TestExplorer.*(Receipt|Reconcile|Construction) matches zero tests at base and after; gofmt, go vet, scripts/check_no_code_comments.py and git diff --check clean. Issue: #193
Collaborator
Author
CI recovery noteFirst run failed
|
matthewpeterkort
left a comment
Collaborator
Author
There was a problem hiding this comment.
Review: PR #206 @ 8a325a2 — per-operation construction lineage handlers
Verdict: APPROVED — no changes requested; ready. (Posted as COMMENT because GitHub rejects APPROVE/REQUEST_CHANGES on own-account reviews; this sentence carries the verdict.)
Scope reviewed. Full diff against base 80e8877 on feature/postgres-backend: one file, internal/server/explorer_output_reconcile.go (+710/-556), two hunks. I compared the base function body (old lines 644-1376) against the new driver and all 13 apply*ConstructionStep handlers line by line.
What I verified:
- Faithful extraction: every handler body matches the corresponding base case — error strings and %w wrapping (derive/pivot/coded-pivot/unpivot quality errors), addOutput call order, break/continue flow, group key/aggregate/row-value loops, source-projection claim gates for derive/unpivot/filter, combine projection resolution, and related-* metadata writes are all behaviorally identical.
- Owner keeps the shared per-step transition unchanged: prior seeding with the compiled source-stage merge, per-step nextAuthored seeding, addOutput validation, step output-schema validation, clear(authored)/copy-back atomicity, first-failure precedence, and the unsupported-kind default.
- Error semantics preserved: handlers return (nil, err) and the driver returns stepErr before clear(authored)/prior advance, so caller-visible authored/prior maps see no partial mutation on failure, same as the old direct returns.
- Shared-state aliasing is consistent: stepContext.nextAuthored, handler-local copies, and the addOutput closure all reference the single per-step map, and every handler returns that same map.
- Wiring: 13 handler definitions, 13 switch dispatch sites, one-to-one; default branch identical to base.
- CI on 8a325a2: all 15 checks green including Go unit tests; existing suites (unpivot/derive source projection, source-child, receipt-contract, proposal) drive these paths through the entry points and assert extracted error strings such as the snapshot-claim message.
Optional suggestions (no action required):
- nextAuthored now travels through two channels: the constructionStepContext field and the handler return value. All handlers mutate-and-return the same map today, so behavior is identical; but if a future handler returned a fresh map, addOutput writes and its duplicate check (via the captured loop variable) would target the seed map and be silently dropped from the copy-back — a latent validation-bypass footgun. Consider single-channel state (context field only) or keeping the return but documenting/enforcing the same-map invariant.
- Direct table-driven tests on each apply* handler would pin the per-operation failure paths the PR advertises (grep-by-name debuggability) at handler granularity, complementing the entry-point coverage.
matthewpeterkort
marked this pull request as ready for review
November 13, 2026 21:17
matthewpeterkort
merged commit Nov 13, 2026
8943086
into
feature/postgres-backend
28 of 29 checks passed
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.
Closes #193. Base:
80e8877onfeature/postgres-backend.What
authoredConstructionOutputsWithCombineInputs(733 lines) walks authored construction steps, carries prior schema + authored lineage, maps each operation to outputs, mutates source metadata, and rejects compiler/authored identity mismatches. This PR extracts one private handler per operation kind (13 handlers) that receives an explicitconstructionStepContext(document, step, stepIndex, prior schema, authored metadata, source-only projection IDs, compiled output, resolved Combine inputs, capability snapshot, seeded next metadata, sharedaddOutputvalidator) and returns the next metadata map or an error. The owner keeps the shared per-step schema transition: prior seeding with compiled source-stage merge,nextAuthoredseeding,addOutputvalidation and error wrapping, step output-schema validation,clear(authored)/copy-back per-step atomicity, first-failure precedence, and the unsupported-kind default. Ownership stays ininternal/server; no new package, no registry, no framework — one explicit switch, one static call per kind.Why this reduces reasoning burden (not line count)
grep applyUnpivotConstructionSteplocates the melt/claim error paths; before, they were found by reading 400 lines of neighbours.addOutputthrough the context; there is no operation-local re-implementation of declared-ID/name/label/duplicate/lineage checks, so the invariant is structural.Behavior-preservation evidence
Extraction is mechanical and rerunnable (
>500lines changed rule). Only two sanctioned rewrites: two-tab dedent, and single-value error returns gain anilfirst value. Correspondence checker output (before/after moved-body, from the base blob):The checker also proves: owner-retained regions (shared schema prelude,
nextAuthoredseeding,addOutput, output-schema validation + clear/copy-back, default kind error) are line-exact versus the base, and the function-region string-literal multiset is identical — exact public error texts and refusal messages preserved, nothing added or dropped. File prefix (lines 1-643) and suffix (combineProjectionInputNameto EOF) are byte-identical versus base.Checks (ticket
checks[]+ complete focused suites)git diff --checkgo test -json ./internal/server -run 'TestAuthoredConstructionOutputs|TestDeriveSourceProjection|TestExplorerReceipt' -count=1Test(AuthoredConstructionOutputs|DeriveSourceProjection|AuthoredConstructionOutputsResolveCompilerSourceChildren|.*UnpivotSource|ExplorerReceipt)TestExplorer.*(Receipt|Reconcile|Construction)no tests to run, exit 0 at base and head — no test name matches this pattern)go test ./internal/server -count=1(full)go vet ./internal/server/go build ./...scripts/check_no_code_comments.pyon changed fileFull-suite equivalence is test-level: base
695 pass / 33 skip / 0 failvs head identical,test-level diffs: NONE. The 33 skips are service-gated integration tests (Arango / ClickHouse / PostgreSQL fixtures), identical set at base and head, so those paths stay CI-gated rather than locally exercised. No test was modified; no new cases were added because per-test parity shows the extraction exposed no observable gap.Scope honored
internal/server/explorer_output_reconcile.gotouched (owned path). No frontend, no generated files, zero handwritten code comments (file had none; checker confirms).authoredConstructionOutputswrapper stays (it names the test-facing intent; removing it would add boilerplate to tests for no comprehension gain).verification/*/reviewed-sources.jsoncontain no gatedfilesrows for this path (prose round-records only), and no test reads this file's source.Rerunning the transformation
transform.pyreads the pristine base blob viagit show, aborts on any anchor drift, and refuses on comment-count delta; the checker exits non-zero on any non-matching line.transform.py
verify_correspondence.py