📝 docs(openspec): close 5 P2/P3 findings from the PR #45 re-review - #46
Merged
Conversation
G1 -- REQ-E11-S00-01's isolation guard rewritten to '! go list ./... | grep -q spikes/rego'. NOTE the review's stated mechanism did not reproduce: in a default shell a pipeline's exit status is the LAST command's, so 'grep -c ... | grep -qx 0' actually exited 0 when isolated and 1 when leaked -- the correct polarity. It inverts to a false red only under 'set -o pipefail'. Verified both polarities under bash -o pipefail before and after. Conclusion right, reasoning wrong; fixed because a guard that flips meaning with a shell option is not a guard, and this one protects the 'no OPA in go.mod while (d) is open' invariant. G2 -- REQ-E11-S03-04's Verify used '-run TestPurity', which matches NO test (the real names are TestCorePurity*). Confirmed vacuous: it printed 'no tests to run' and exited 0. That was the verification for the single REQ enforcing the (d1) hard-rule narrowing -- exactly the tests-that-cannot-fail class this repo keeps re-finding, and it would have read as governed while checking nothing. Now -run TestCorePurity, confirmed to execute. G3 -- the retained-FileAtRef enumeration was incomplete and one line number was wrong: actual ref-addressed policy loads are run.go:203 (MergePolicy), :211 (RulesetBinding), :230 (Config) and :249 (pack); :253 is LoadPack, not a FileAtRef call. The omitted :230 is the most damaging to get wrong -- .assent/config.yaml declares the provider hosts, so migrating it to an MR-relative accessor would let a fork's head redefine its own fact semantics, the precise trust-boundary regression the F1 fix exists to prevent. G4 -- renamed the spike doc to spike-e-rego-budget.md; 'Spike D' is already the Kubernetes CRD feasibility spike that E14 is gated on. G5 -- E11-S13's story dependencies corrected to S00-S12 to match the backlog row.
…austive H1 -- the enumeration added in the previous commit covered run.go only, while newly asserting it was exhaustive. Two further ref-addressed decision-input reads live in cmd/assent/provider_host.go: the provider host declaration (:82) and the resource-owner registry (:275). Six call sites, not four. provider_host.go:275 is the most dangerous read in the repo to migrate. That registry decides WHO MAY APPROVE, and its own comment records that it once preferred the checkout -- which under --checkout is the merge request's own head tree -- letting an MR ship a registry naming its author as owner of the resource it is changing. That escalation was found and fixed as D-130. An implementer writing REQ-E10-S02-05(ii)'s guard test from a four-site list would leave it uncovered, and a later 'consistency' refactor onto FileAtHead would reintroduce D-130 with a green suite. H2 -- cmd/assent declares TWO port interfaces, not one: run.go:64 forgePort (the anonymous literal) and provider_host.go:246 refFilePort, a hand-rolled FileAtRef-only interface. E10-S02's DoD said 'anonymous port literal deleted' (singular), so replacing forgePort alone would leave refFilePort standing with the DoD reading satisfied and both go build and task lint green. The DoD now names both, and notes that refFilePort is a named interface -- which is why the original wording did not describe it and so did not cover it. Both found by the independent reviewer after the P2 pass; both verified against the tree before acting.
…iant (I1-I3) I1 -- H2 fixed the DoD by naming the two ports that exist today, but an allowlist goes stale the moment a third is added, and cmd/assent already declares three interfaces (forgePort, refFilePort, and the benign checkout.go:27 localCheckout). New REQ-E10-S02-07 states the invariant instead: no interface declared in cmd/assent may carry a forge read or write method except forge.RunPort, enforced in depguard_test.sh -- which already walks cmd/assent source and already carries the mutation-control pattern REQ-E10-S02-04 rebuilds. localCheckout must stay green, so the guard has to distinguish a local-tree seam from a forge port rather than banning interfaces outright. Same upgrade this chain applied to REQ-E11-S03-04: enforced, not merely stated. The enumeration has been wrong three times; a human re-reading the tree at S02 time is what kept failing. I2 -- S02-05(i) said 'no FileAtRef call remains on the governed-subject path', which an implementer could read as 'all governed-subject sourcing is now MR-relative'. It is not: under --checkout, run.go:283 overrides base/head from the local tree via dirCheckout.FileContents, which carries no FileAtRef call and is invisible to the source-level guard. That is intended existing behaviour (EFE-S03 / ADR-0008 s4 -- the local head tree is the presence authority) and S02 does not change it. Now stated. I3 -- the two provider_host.go reads take a targetRef parameter, not info.TargetBranch; REQ-E10-S02-05(ii) still said targetBranch after ADR item 5 had already been corrected.
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
1 participant
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.
Follow-up to #45, which merged with an independent APPROVE (zero P0, zero P1). The re-review also raised five P2/P3 findings that landed after the merge. Three of them sit on the exact guards that close #45's P1s, so they are worth fixing before an implementer picks up E10-S02 or E11-S00/S03.
The two that mattered
G2 — a verification that could not fail.
REQ-E11-S03-04's Verify wasgo test ./internal/core/... -run TestPurity.TestPuritymatches no test in the tree (the real names areTestCorePurity,TestCorePurityWalkReachesEveryGuardedDir, …), so it printedno tests to runand exited 0. That was the verification for the single requirement enforcing the (d1) rule-7 narrowing — it would have read as governed while checking nothing, which is the failure mode that requirement's own text warns about. Now-run TestCorePurity, confirmed to execute.G3 — an incomplete
FileAtRefenumeration. ADR-0021 and the E10 spec listed the retained ref-addressed policy loads asrun.go:203, :211, :253. Verified against the tree, the actual call sites are:203(MergePolicy),:211(RulesetBinding),:230(Config) and:249(pack) —:253ispolicy.LoadPack(packBytes), not aFileAtRefcall. The omitted:230is the most damaging one to get wrong:.assent/config.yamldeclares the provider hosts, so migrating it to an MR-relative accessor would let a fork's head redefine its own fact semantics — precisely the trust-boundary regression #45's F1 fix exists to prevent. The enumeration read as exhaustive, so an implementer writing the "policy still ref-addressed" test over three call sites would have left that one unguarded.One correction to the review
G1 was reported as "the Verify chain aborts, so
task checknever runs". That mechanism did not reproduce. In a default shell a pipeline's exit status is the last command's, so… | grep -c 'spikes/rego' | grep -qx 0exited 0 when the spike was correctly isolated and 1 when it leaked — the correct polarity. It inverts to a false red only underset -o pipefail. Fixed anyway, to! go list ./... | grep -q 'spikes/rego': a guard that flips meaning with a shell option is not a guard, and this one protects the "no OPA ingo.modwhile judgment call (d) is open" invariant. Both polarities verified underbash -o pipefailbefore and after.Also
spike-e-rego-budget.md; Spike D is already the Kubernetes CRD feasibility spike that E14 is gated on.E11-S13's story dependencies corrected toS00–S12, matching the backlog row.Gates
task checkgreen locally at the branch tip (exit 0) — noted explicitly becauserelease-exitgateskips on PRs in this repo, so CI never runs it.Docs/spec only: no Go, no
schemas/, no workflows.Still open, unchanged by this PR
The (d1) vs (d2) mechanism half of E11's rule-7 judgment call still blocks E11-S03 and E11-S04. The operator's "accept and pin OPA" answer settled the supply-chain half only — (d2) also accepts and pins OPA; the two differ on where the evaluator lives and which gate enforces rule 7. Recorded in D-141 and in the INBOX.