Record shared behavior audit findings

This commit is contained in:
2026-11-02 01:45:54 +00:00
parent 5e6db4de76
commit 083bc83331

View File

@@ -0,0 +1,84 @@
# 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](../../policy/architecture.md), 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](../../../internal/candidate/check_test.go)
exercise clean worktree/index and stable-HEAD requirements for candidate
checking. [TestBuildSourceEligibilitySurvivesLaterCheckoutCleanup](../../../build_test.go)
checks the same kinds of changes before and during a build, while
[TestDeliverRejectsDirtyTaggedSourceBeforeBuildOrHostedWrites](../../../delivery_test.go)
and [TestInspectTaggedSourceReportsDirtyCheckoutAndReadCommitFile](../../../internal/git/source_test.go)
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](../../../internal/candidate/check.go)
runs branch, HEAD and porcelain-status commands. [build.gitCommit and cleanSource](../../../internal/build/build.go)
independently run HEAD and status commands through direct subprocesses.
[git.CaptureHead and InspectTaggedSource](../../../internal/git/source.go)
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](03-complexity.md#2-git-source-inspection-is-repeated-across-package-boundaries)
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](01-correctness.md).
## 2. Build-artifact declarations are checked independently by receipts and delivery
**Test evidence:**
[TestBuildReceiptRoundTripBindsRootAndNestedModuleIdentity](../../../cmd/releasekit/internal/cli/receipt_test.go)
constructs and reloads a receipt binding a selected target to its expected
filename. [TestDeclaredBuildAssetsGenerateManifestFromBuildResult and TestDeclaredBuildAssetsRejectsArtifactDeclarationMismatch](../../../internal/delivery/assets_test.go)
exercise the corresponding build-result declarations and reject a mismatched
filename. [TestBuildAndPrebuiltDeliveryUseSameDirectoryBase](../../../cmd/releasekit/assets_test.go)
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](../../../cmd/releasekit/internal/cli/receipt.go) and
[declaredBuildAssets](../../../internal/delivery/assets.go) 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](../../../internal/artifact/artifact.go), 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.