diff --git a/CHANGELOG.md b/CHANGELOG.md index c89a6d3..9a52740 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,9 @@ repository still gets a decision, never by following the link; no release carrie - :memo: docs(openspec): fix two P0 design defects found by adversarial review - :memo: docs(openspec): correct the E10 story count to 19 after adding S00 - :memo: docs(openspec): fix 3 P1 design defects found by independent review of PR #45 +- :memo: docs(openspec): close 5 P2/P3 findings from the PR #45 re-review +- :memo: docs(openspec): close H1/H2 — the FileAtRef enumeration was not exhaustive +- :memo: docs(openspec): make the cmd/assent port boundary an enforced invariant (I1-I3) ### Testing - :white_check_mark: test(release): anchor the D-120 note check on its header sentence, not the bare token diff --git a/docs/adr/0021-multi-adapter-forge-seam.md b/docs/adr/0021-multi-adapter-forge-seam.md index 1b12ea4..f95ad99 100644 --- a/docs/adr/0021-multi-adapter-forge-seam.md +++ b/docs/adr/0021-multi-adapter-forge-seam.md @@ -108,8 +108,20 @@ consisting of four committed pieces: `FileAtBase(mr, path string) ([]byte, error)` / `FileAtHead(mr, path string) ([]byte, error)`. **The two accessors are not redundant and neither replaces the other** — item 5 decides which is legal where. `FileAtRef` survives because *policy* is ref-addressed by contract: - ADR-0015 §1 requires the MergePolicy, RulesetBinding and pack to load from the **target - ref by name**, which `cmd/assent/run.go:203,211,253` does today and must keep doing. + ADR-0015 §1 requires the MergePolicy, RulesetBinding, Config and pack to load from the + **target ref by name**, which `cmd/assent/run.go:203`, `:211`, `:230` and `:249` do today + and must keep doing. **That list is exhaustive for `run.go` and NOT for + `cmd/assent`**: two further ref-addressed decision-input reads live in + `cmd/assent/provider_host.go` — the provider host declaration at `:82` and the + **resource-owner registry** at `:275`. Verify against the tree before relying on either + list. Two of the six deserve specific mention. `run.go:230` reads `.assent/config.yaml`, + which carries the provider-host declarations, so migrating it would let a fork's head + redefine its own fact semantics. `provider_host.go:275` is the most dangerous read in the + repository to move, because that registry **decides who may approve**: 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**; moving this read + onto an MR-relative accessor reintroduces it. `FileAtBase`/`FileAtHead` are the **governed subject's** only legal accessors. An adapter that implements `FileAtBase` by delegating to `FileAtRef(project, path, sourceBranch)` reintroduces the defect item 5 exists to kill. @@ -148,7 +160,8 @@ consisting of four committed pieces: **Scope of the narrowing, stated precisely because item 1 keeps both accessors:** it binds the governed subject only — `run.go:270,274`, the reads whose 404-maps-to-`nil` feeds `change.OneSidedLifecycle` and mints the fabricated whole-file DELETE. The **policy** loads - at `run.go:203,211,253` are *deliberately* still `FileAtRef(project, path, targetBranch)`: + at `run.go:203`, `:211`, `:230`, `:249` — **plus `provider_host.go:82` and `:275`** — are + *deliberately* still `FileAtRef(project, path, targetRef)`: they read the protected target ref of the target project, which is exactly the trust boundary ADR-0015 §1 draws, and a fork's head must never be able to reach them. Rewriting those onto an MR-relative accessor would be a trust-boundary regression, not a cleanup. diff --git a/openspec/specs/p5-e10-github-forge/spec.md b/openspec/specs/p5-e10-github-forge/spec.md index 0085b41..a539c12 100644 --- a/openspec/specs/p5-e10-github-forge/spec.md +++ b/openspec/specs/p5-e10-github-forge/spec.md @@ -252,10 +252,19 @@ runs it. - **Goal**: `cmd/assent` depends on one named, forge-neutral interface and on no concrete adapter package. - **Dependencies**: S00, S01 (so the port change is proven by an executable suite). -- **Definition of done**: `forge.RunPort` declared in `internal/forge`; `cmd/assent`'s - anonymous port literal deleted; **the neutral adapter factory lands in this story**; - depguard denies **both** concrete adapters from `cmd/assent`; zero behaviour change - (goldens and conformance byte-identical). +- **Definition of done**: `forge.RunPort` declared in `internal/forge`; **both** of + `cmd/assent`'s port declarations retired — `run.go:64 forgePort` (the anonymous literal at + the call site) **and** `provider_host.go:246 refFilePort`, a second, hand-rolled + `FileAtRef`-only interface. Naming only the first is how this story closes while + `cmd/assent` still depends on a private port: replacing `forgePort` alone leaves + `refFilePort` standing, the DoD reads satisfied, and `go build` + `task lint` stay green. + (`refFilePort` is a *named* interface, not an anonymous literal — the DoD's original wording + did not describe it and so did not cover it.) Plus: **the neutral adapter factory lands in + this story**; depguard denies **both** concrete adapters from `cmd/assent`; zero behaviour + change (goldens and conformance byte-identical). **The two names above are evidence, not the + contract** — an allowlist of the ports that exist today goes stale the moment a third is + added, and this enumeration has now been wrong three times. REQ-E10-S02-07 states the + invariant and enforces it mechanically. - **Corrected after adversarial review — the first draft of this story could not close.** It required depguard to deny both adapters "with no symbol allowlist", but `cmd/assent/main.go:72,83` calls `gitlab.New(endpoint, token, botAuthor)` and no story supplied a neutral factory until @@ -272,7 +281,12 @@ runs it. and `cmd/assent` references that named type only. **Both accessors are required and they are not interchangeable** — REQ-E10-S02-05 binds which is legal where. `FileAtRef` is retained **only** for the ref-addressed *policy* loads ADR-0015 §1 mandates (`cmd/assent/run.go:203`, - `:211`, `:253` — MergePolicy, RulesetBinding, pack, all from the target ref by name); + `:211`, `:230`, `:249` — MergePolicy, RulesetBinding, **Config** and pack — **plus + `cmd/assent/provider_host.go:82` (provider host declaration) and `:275` (resource-owner + registry)**, all from the target ref by name. That is **six** call sites, not four: the + `run.go` list alone is not exhaustive for `cmd/assent`. `provider_host.go:275` is the single + most dangerous one to migrate — the registry decides **who may approve**, and preferring the + checkout there was the D-130 vouching escalation); implementing this REQ by freezing `FileAtRef` as the *sole* content accessor satisfies the signature while preserving the fabricated-DELETE defect, and is a failure of this story. - Test: `internal/forge/port.go`, `cmd/assent/run.go` @@ -311,15 +325,36 @@ runs it. (i) the governed-subject reads (`cmd/assent/run.go:270`, `:274`, via `fileAtRefOrAbsent`) call `FileAtBase`/`FileAtHead` and **no** `FileAtRef` call remains on the governed-subject path — asserted by a source-level guard, because a green `TestForkMRNoFabricatedDelete` - against a fake that happens to serve the right bytes does not prove the call was rewritten; - (ii) the **policy** loads (`run.go:203`, `:211`, `:253`) still use - `FileAtRef(project, path, targetBranch)` and are **not** migrated — a test asserts policy is + against a fake that happens to serve the right bytes does not prove the call was rewritten. + **Read this as scoped to the forge-sourced path, not as "all governed-subject sourcing is now + MR-relative":** under `--checkout`, `run.go:283` overrides base/head from the local tree via + `dirCheckout.FileContents(governed)`, which carries no `FileAtRef` call and is therefore + invisible to the source-level guard. That is intended existing behaviour (EFE-S03 / + ADR-0008 §4 — the local head tree is the presence authority), and S02 does not change it; + (ii) the **policy and decision-input** loads (`run.go:203`, `:211`, `:230`, `:249` and + `provider_host.go:82`, `:275` — all six) still use + `FileAtRef(project, path, targetRef)` and are **not** migrated — a test asserts policy is read from the target ref of the target project even for a fork MR, so a well-meaning "consistency" refactor onto an MR-relative accessor (which would let a fork's head reach the policy load) fails the suite rather than silently crossing ADR-0015 §1's trust boundary. - Test: `internal/forge/port.go`, `internal/forge/conformance/`, `cmd/assent/run.go` - Verify: `go test ./... -run 'TestForkMRNoFabricatedDelete|TestPolicyLoadsFromTargetRefOnForkMR'` - Level: L1 +- **REQ-E10-S02-07** — Given an allowlist of today's ports cannot survive a port added + tomorrow, when `hack/lint/depguard_test.sh` runs, then it enforces the **invariant** rather + than the list: **no interface declared in `cmd/assent` may carry a forge read or write method + except `forge.RunPort` itself**. The scanner already walks `cmd/assent` source and already + carries the mutation-control pattern at `:356-363` that REQ-E10-S02-04 rebuilds, so this is + an added assertion, not new machinery. A mutation control proves it goes **red** on a + hand-rolled `interface{ FileAtRef(...) }` reintroduced anywhere in `cmd/assent`. + `checkout.go:27 localCheckout` must stay **green** — it is a local-tree seam carrying no + forge method, and a guard that cannot tell those apart would either block legitimate seams or + be switched off. Without this REQ, "both port declarations retired" is provable only by a + human re-reading the tree at S02 time — which is exactly what failed on this enumeration + three times over. + - Test: `hack/lint/depguard_test.sh`, `cmd/assent/run.go`, `cmd/assent/provider_host.go` + - Verify: `task lint && task lint-depguard-test` + - Level: L1 - **REQ-E10-S02-06** — Given ADR-0021 item 7, when the port is declared, then it exposes the **authenticated identity**, and a case proves markers are recognised as our own under **both** auth shapes — a PAT identity is a `User`, so an "exclude any bot" filter would make diff --git a/openspec/specs/p5-e11-rego-backend/spec.md b/openspec/specs/p5-e11-rego-backend/spec.md index 92b1228..909484d 100644 --- a/openspec/specs/p5-e11-rego-backend/spec.md +++ b/openspec/specs/p5-e11-rego-backend/spec.md @@ -236,7 +236,7 @@ human dependency. the annoying direction. This story exists so that discovery is free. - **Dependencies**: none. It is deliberately **not** blocked on judgment call (d) — see REQ-E11-S00-01, which is what makes that true. -- **Definition of done**: `docs/planning/spikes/spike-d-rego-budget.md` records the verdict, the +- **Definition of done**: `docs/planning/spikes/spike-e-rego-budget.md` records the verdict, the pinned OPA version it was established against, the exact API surface examined, and the reproduction command — and the repository's own `go.mod`/`go.sum` are byte-unchanged. @@ -248,7 +248,7 @@ human dependency. spike is unbuildable by `task check` and cannot smuggle the adoption (d) has not yet authorised. Spiking a dependency is not adopting it. - Test: `hack/spikes/rego/go.mod`, root `go.mod`, root `go.sum` - - Verify: `git diff --exit-code -- go.mod go.sum && go list ./... | grep -c 'spikes/rego' | grep -qx 0 && task check` + - Verify: `git diff --exit-code -- go.mod go.sum && ! go list ./... | grep -q 'spikes/rego' && task check` - Level: L0 - **REQ-E11-S00-02** — Given the question is empirical, when the spike runs, then it either (i) **names the public API** that bounds evaluation by a machine-independent count and @@ -256,7 +256,7 @@ human dependency. across at least two `GOMAXPROCS` settings — the property S06 will later have to gate on — or (ii) records that **no such API exists** in the pinned version, with the surface examined enumerated so the finding is falsifiable rather than an absence-of-evidence claim. - - Test: `hack/spikes/rego/`, `docs/planning/spikes/spike-d-rego-budget.md` + - Test: `hack/spikes/rego/`, `docs/planning/spikes/spike-e-rego-budget.md` - Verify: `cd hack/spikes/rego && go test ./...` - Level: L0 - **REQ-E11-S00-03** — Given the verdict routes the epic, when it is recorded, then it states @@ -267,7 +267,7 @@ human dependency. can only ever observe is a *different published contract* from one that can gate, and shipping the gating shape first would announce a capability the epic cannot deliver. Under no outcome is this resolved by "timeout → BLOCK" (judgment call (b), (iii)). - - Test: `docs/planning/spikes/spike-d-rego-budget.md`, `docs/decisions/decisions.md` + - Test: `docs/planning/spikes/spike-e-rego-budget.md`, `docs/decisions/decisions.md` - Verify: manual review - Level: L0 @@ -393,7 +393,7 @@ human dependency. as governed while checking nothing. If the operator answers **(d2)** this REQ is struck and replaced by the depguard rule denying the evaluator package from `internal/core/**`. - Test: `internal/core/purity_test.go`, `.golangci.yml`, `hack/lint/depguard_test.sh` - - Verify: `task lint && task lint-depguard-test && go test ./internal/core/... -run TestPurity` + - Verify: `task lint && task lint-depguard-test && go test ./internal/core/... -run TestCorePurity` - Level: L1 ### E11-S04 — OPA capability sandbox `[autonomous · engine-grade · maintainer LGTM]` @@ -623,7 +623,7 @@ human dependency. ### E11-S13 — Exit gate `[autonomous]` -- **Dependencies**: S01–S12. +- **Dependencies**: S00–S12. - **Definition of done**: `hack/policy/e11_exitgate_test.sh` proves in one invocation: denied builtins fail compilation (all cases); determinism over N ≥ 100 runs; the empty-violations polarity test present and failing-closed; the effect/points boundary held; the scoped schema