5.2 KiB
Behavioral equivalence and shared abstraction audit
Reviewed baseline: 5e6db4d on main. This review started with tests of
corresponding behavior and traced the implementation behind each candidate.
It considered the existing facade and package boundaries, including the
architecture policy, and retained deliberate
differences between operations. No production code or tests were modified.
The offline tests for internal/candidate, internal/git, internal/delivery,
and cmd/releasekit/internal/cli passed using the Go test cache. Focused public
tests of build source eligibility, inspection errors, and dirty tagged-source
delivery also passed.
1. Source-state observations are independently implemented
Test evidence: TestCheckRejectsDirtyTrackedAndUntrackedState and TestCheckRechecksHeadAndWorktreeAfterCallback exercise clean worktree/index and stable-HEAD requirements for candidate checking. TestBuildSourceEligibilitySurvivesLaterCheckoutCleanup checks the same kinds of changes before and during a build, while TestDeliverRejectsDirtyTaggedSourceBeforeBuildOrHostedWrites and TestInspectTaggedSourceReportsDirtyCheckoutAndReadCommitFile exercise dirty-state observations for tagged delivery. These tests imply a common need for accurate Git HEAD and repository-wide status observations; they do not imply identical decisions about dirty source.
Implementation: candidate.inspectLocalState runs branch, HEAD and porcelain-status commands. build.gitCommit and cleanSource independently run HEAD and status commands through direct subprocesses. git.CaptureHead and InspectTaggedSource perform overlapping observations through the Git package's command boundary.
Opportunity: The existing Git package could own small shared commit and status observation primitives, leaving each operation to choose when to read them and how to interpret them. This would centralize a concrete Git responsibility and reduce the number of paths needing parallel fixes for inspection, error propagation, and cancellation. The complexity audit also identifies the duplication; the tests above establish the common behavioral evidence for considering it here.
Tradeoffs: Candidate checking rejects dirty state and checks the branch; builds can succeed with dirty state but record source eligibility before and after compilation; delivery additionally checks a specific tag. Do not merge those policies or alter observation timing. Retain separate caller-level tests, so a shared helper cannot make every test pass while all consumers interpret its results incorrectly. Coordinate with the existing replacement-object and cancellation findings.
2. Build-artifact declarations are checked independently by receipts and delivery
Test evidence: TestBuildReceiptRoundTripBindsRootAndNestedModuleIdentity constructs and reloads a receipt binding a selected target to its expected filename. TestDeclaredBuildAssetsGenerateManifestFromBuildResult and TestDeclaredBuildAssetsRejectsArtifactDeclarationMismatch exercise the corresponding build-result declarations and reject a mismatched filename. TestBuildAndPrebuiltDeliveryUseSameDirectoryBase connects the two paths through a real build, receipt, and prebuilt delivery. Together they support a shared requirement that a target's recorded artifact identity agree with the selected build, without requiring receipt loading to read artifact contents.
Implementation: Both
validateBuildReceipt and
declaredBuildAssets independently
check target uniqueness, artifact counts, one artifact per target, and names
derived from targets. They already share artifact.ExpectedBuild and
artifact.ValidComponent, but each builds and checks its own target-to-name
mapping.
Opportunity: Consider a narrow, pure declaration-matching helper in the existing artifact package, accepting target/filename metadata and reporting a mismatch without opening files. Both paths could reuse it after converting their own types. Centralizing the shared identity rule would reduce drift when target or filename conventions evolve.
Tradeoffs: The receipt additionally requires a schema, specific output
path and digest syntax, and intentionally loads without checking the files;
delivery accepts source-only results and separately stats and verifies local
content after preflight. Keep those boundary-specific rules and ordering where
they are. Avoid making internal/artifact depend on CLI receipt types,
internal/build, or forge policy. Preserve independent public and CLI tests
with concrete expected filenames and bytes rather than using the new helper to
compute every test expectation.