From ce87af3bf15016cdd5e68728c03c5e72ef60184a Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 16 Aug 2026 10:57:44 +0200 Subject: [PATCH 1/5] :memo: docs(decisions): record 8 operator rulings from 2026-08-16 session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds D-144 through D-151 to the decision log — the rule-7/OPA mechanism (unblocks E11-S03/S04), the writes:false enforcement gate, the GUARD-1 BLOCK channel, provider credentials, the changelog PR gate, the apiVersion domain rename (suspended pending the assent.dev transfer), the Sonar test-complexity route, and the OpenSSF Best Practices project. Strikes the corresponding open-questions.md rows plus two rows that were already resolved by prior decisions (stale duplicate OQ-28, OQ-9). --- docs/decisions/decisions.md | 10 +++++++++- docs/planning/open-questions.md | 13 ++++++------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index 657a52d..95b0b21 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -145,6 +145,14 @@ project/process decisions. | D-138 | 2026-08-09 | **The three reliability P1s of the 2026-08-09 audit (RELI-01/02/03) are DEFERRED past v0.2.0, deliberately and with the deferral recorded (AGENTS.md rule 6).** All three are **pre-existing at v0.1.0**; nothing in v0.2.0 introduced or worsened any of them, verified against `1d8aa60` (`origin/main` at audit time). Holding the tag would delay two fail-open fixes and a P0 in order to fix defects that are already live. **The machine direction holds in all three:** no probed path merges anything unevaluated — `MergeCAS` re-checks all three pins atomically, arming stays default-deny (`internal/forge/precondition.go`), and the `.assent/**` self-edit guard still yields BLOCK with zero forge writes. What fails is the **human signal channel**, which is why they are P1 and not P0. **RELI-01 — clean exit-0 runs leave a stale APPROVE banner, and D-130's compensating control does NOT apply here.** `cmd/assent/run.go` emits the DecisionRecord at step 8 and only then enters the step-9 switch, whose two GUARD branches (`reservedSelfEditBlock`, `untrustedExecutionContext`) skip `forge.Reconcile` **entirely** — including the summary upsert. So run 1 APPROVEs and posts `✅ Decision: APPROVE`; run 2, after a `.assent/**` edit is added, decides BLOCK, exits **0**, and leaves that note byte-identical. `openspec/specs/p5-e5-provider-host/spec.md` REQ-E5-S08-03 accepts a stale banner on the abort path precisely because "a REVIEW rerun upserts that same summary to REVIEW and adds an unresolved discussion" — the discussion being what holds the merge under `only_allow_merge_if_all_discussions_are_resolved`. **On the GUARD-1 self-edit path no thread is posted at all**, so nothing holds the merge and no later run repairs the banner: the compensating control the spec relies on is absent exactly where it is needed. Reachability is ordinary CI cadence, and it is deliberately triggerable at the cost of two pushes — D-042's own threat model rendering as a green tick. **Its fix is out of release scope BY CONSTRUCTION**, not by preference: `openspec/specs/p5-aud-audit-remediation/spec.md` pins "the decision is BLOCK with zero forge writes (GUARD-1 dominance over the gap-degrade)", so upserting a summary on that path REOPENS a frozen acceptance criterion and needs an openspec change proposal first (see OQ-31). **RELI-02 — a duplicated summary comment is UNRECOVERABLE WITHOUT A HUMAN.** `reconcileThread` has both `repairDuplicates` and a step-9 rescan; `reconcileSummary` (`internal/forge/forge.go`) has neither — it is a bare `UpsertComment`. No branch of `Reconcile` can remove a second summary note, so once two exist the wrong one can sit at `decision=APPROVE` forever. Not a corner case: `PreconditionFromCapabilities` seeds `DuplicatePrevention` to `unserialized-best-effort` because per-MR `resource_group` serialization is never probed, i.e. the race is the project's **declared default posture**. First item of v0.2.1. **RELI-03 — the standing bot approval has no retraction and the cited mitigation is never probed.** `reconcileApproveMerge` records `Approve` before `MergeCAS`; on CAS failure in the TOCTOU window the approval is already written and `forge.Forge` has no `Unapprove` verb. The code's own safety argument named the forge's remove-approvals-on-push setting — and **`reset_approvals_on_push` appears in ZERO Go files**: nothing reads it, `probeCapabilities` does not fetch it, `assent doctor` cannot report it. `spike-secure-setup.md` D11 specified refuse-to-arm when it is off and C19 specified doctor verification; **neither was built**, and the comment's deferral pointer named P4-E1-S10 — a slice that SHIPPED (D-041), so the concern was never picked up. **The comment is corrected in this lane** (`internal/forge/forge.go`, text only, no behaviour change) because an asserted-and-unbuilt mitigation TERMINATES THE SEARCH — it is how this survived two prior audits. RELI-03 needs no open question: D11 already decided it; it is unbuilt, not undecided. **Also recorded here, same shape — the ADR-0019 `duplicate_prevention:` MUST is unmet.** The value is computed and typed through to `PreconditionReport` and then never printed: `emitDoctorReport` emits only the arm verdict and refusal reasons, so no `assent doctor` output contains the string. The safe-default half DOES hold (nothing can claim `single-writer-serialized` without the mechanism). **Judgment call: annotate, do not emit.** Emitting is ~3 lines, but it is one instance of audit **ARCH-11** — doctor computes a typed capability report and prints essentially none of it — so emitting this one field would half-close ARCH-11 and leave the report inconsistent with itself, and it is a user-visible CLI output change, which does not belong in a docs-truth lane before a tag. ADR-0019 now carries the unmet-MUST annotation and points here; the emission belongs to the v0.2.1 ARCH-11 slice with its own tests. **Not deferred silently:** all three plus ARCH-11 are named in the v0.2.1 wave. Revert: `git revert` this lane — restores the false RELI-03 comment and changes no behaviour, since the lane changes none. | | D-139 | 2026-08-09 | **The security lens's SEC-01/SEC-04/SEC-05 are KNOWN and DEFERRED to v0.2.1; the tag proceeds, but the release must not claim `--checkout` is now safe.** All three are pre-existing at v0.1.0 and none was introduced by v0.2.0 (verified at `1d8aa60`). Reach on SEC-01 is bounded and that is why it does not block: **no Go non-test code constructs `--checkout`**, no CI template, example, hack script or e2e passes it, and neither `README.md` nor `docs/usage/walkthrough.md` mentions it at all — the adoption path this project actually teaches is checkout-less. The fix is also monotone-safer to ship after the tag, since the P0 already merged in this release was likewise `--checkout`-only. **SEC-01 — the `--checkout` tree is never bound to the evaluated SHA.** With `--checkout` the local tree is the sole authority for the judged bytes and the changed-file set (D-077), while `pins.sourceSha`/`targetSha` and the compare-and-swap come from the forge; **nothing compares the two**. Measured: `cmd/assent/checkout.go` contains zero occurrences of `SHA`/`Sha`, and `run.go` has no step relating the tree to a commit. Reproduced by the lens: forge head a destructive shrink, local checkout a benign grow → `APPROVE`, `approvals=1 merges=1`. ADR-0015 §2 promises every write re-verifies what it acts on; the guard re-verifies that the *metadata* has not moved, never that the judged *bytes* are the bytes at that SHA. **Condition attached and DISCHARGED IN THIS LANE:** this release hardens `--checkout` extensively (D-133 refuses any symlink anywhere) and `docs/usage/cli.md` already named "run without `--checkout`" as remedy #1, so the page read as though the flag had been made sound. A *Known limitation: the checkout is not bound to the evaluated commit* section now says otherwise, framed as a property of how the checkout is CONSTRUCTED (build `head/` from the MR head SHA; cancel superseded pipelines) and **claiming no mitigation on assent's part** — `auto_cancel_redundant_pipelines` is a project setting the tool never probes, and citing an unread setting as a control is the exact pattern this audit found three times. **Named fix for v0.2.1:** bind the checkout to the evaluated SHA, or cross-check the local changed-file set against the already-fetched `snapshot.ChangedFiles` and fold divergence opaque. **SEC-04 — protected-config verification is a substring test.** `internal/forge/gitlab/snapshot.go` sets `caps.ProtectedPipelineExternal = strings.Contains(proj.CIConfigPath, "@")`, while ADR-0015 §4 calls protected config THE load-bearing adoption prerequisite and says doctor refuses to arm when it cannot verify it. Reproduced: an in-repo, author-editable `.ci/pipeline@v2.yml` arms and merges. **Named fix:** replace the substring test with a structural parse — require a non-empty suffix after `@` that contains `/`. **SEC-05 — absent provenance is read as trusted.** `snapshot.go` computes `ForkMR: mrResp.SourceProjectID != 0 && mrResp.SourceProjectID != mrResp.ProjectID`, so an absent or zero `source_project_id` yields `ForkMR=false` and the fork guard never engages; reproduced to `approvals=1 merges=1`. **Named fix:** treat absent or zero `source_project_id` as **fork** (untrusted) — unknown provenance must fail toward advisory-only. **The composition worth not losing, recorded AS A HYPOTHESIS, not as measured:** SEC-05 × SEC-03 would let a fork MR reach the write path and then leave a **standing bot approval on a fork MR**, converting ADR-0015 §8's "CI on fork / untrusted-contributor MR → advisory-only, no writes" into "we can leave an approval on a fork". There is **no evidence real GitLab ever omits `source_project_id`**; the composition is untested and is written down so the v0.2.1 SEC-05 fix is understood as closing more than a provenance nicety. **Correction carried from the lens:** its SEC-08 was **WITHDRAWN as non-novel** — D-130 already documents the host-declaration `continue`-on-any-error as a knowingly-deferred sibling. It is not a new finding and must not be written up as one. | | D-140 | 2026-08-10 | **E10 (GitHub adapter + Actions entrypoint) is UNLOCKED by direct operator instruction; ADR-0021 governs the seam and `openspec/specs/p5-e10-github-forge/spec.md` decomposes it into 19 stories (S00, the addressing & representation model, was added after an adversarial review found two P0 representation defects).** E10 was **Locked** under D-012 ("unlocks with a named consumer"), and that lock was reaffirmed twice — D-017 named the consumer and still said "E10/E13 stay locked", D-019 restated "GitHub + remote packs locked". No prior decision authorized it, so the operator's instruction this session is a NEW unlock event, recorded here BEFORE any spec text or code exists (AGENTS.md rule 6). **What this authorizes**: the GitHub forge adapter, the multi-adapter seam work it depends on, and forge selection in `run`/`doctor`. **What it does NOT authorize**: E13 remote packs (still Locked under D-012 — the "named consumer" reasoning is untouched for that tier), E12 `serve`, E14 CRD, or any third forge / plugin-forge protocol (ADR-0021 Option D, rejected — no named consumer). **Scope of v1 GitHub**: behavioural parity for the GATE, not device-for-device parity (dossier §3, OQ-7/OQ-18 — required-conversation-resolution carries acknowledgement, `REQUEST_CHANGES` reserved for block); the three known deltas (review dismissal, auto-merge revoke, merge queue) are modelled as capabilities, and where GitHub cannot prove what the gate needs the adapter reports the gap and NEVER arms — the same shape as GitLab free tier. **Why a seam epic and not just an adapter**: the 2026-08-09 audit's ARCH-18/ARCH-19 found `docs/planning/design-notes/e10-forge-port-lift.md` under-scopes the epic, and that the conformance suite cannot be run by a second adapter because all 1,155 lines live in `_test.go` files Go cannot import (the package totals 1,166 including the non-test `doc.go`; the `~1,166` figure in the source design note was corrected on 2026-08-10) — so a GitHub adapter written today would be developed against no executable contract and D-084's `github-deferred` catalog rows would be unflippable by construction. The original ARCH-18/ARCH-19 finding text is NOT in the repo (only the one-line summary at `agent-context/PROJECT-AUDIT-2026-08-09.md:412` survives); the two design buckets were therefore RE-DERIVED as (A) no neutral capability model — the GitHub dossier §4 names eleven flags the port needs while `probeCapabilities` reads three project fields and `capabilityGap` is computed in GitLab terms — and (B) no port-level transport/auth policy — GitHub thread resolution is GraphQL-only and needs PAT-vs-App auth, while bounded reads/pagination caps (AUD-S10) and idempotent-GET retry/deadlines (AUD-S11) live inside the GitLab client. Both are recorded as a re-derivation, not as a citation. **OPEN SUB-QUESTION (operator)**: whether the Actions entrypoint (E10-S16) stays in scope — `later-phases.md` titles the epic "GitHub adapter + Actions entrypoint", but the action is packaging on top of an adapter and is the one story whose absence leaves everything else useful; default is to keep it last and independently droppable. **Consequence to watch (E10-S04)**: forcing every capability to be stated explicitly may turn a GitLab arming path that passes today into an honest capability gap — a user-visible behaviour change that gets its OWN decision row and changelog entry, never absorbed silently into "E10 refactor". Revert: re-lock E10 in `later-phases.md`, delete the spec directory; nothing here is published API (`forge.RunPort` is `internal/`), so no compatibility window applies. **✅ OPEN SUB-QUESTION CLOSED (operator, 2026-08-10): option (a) — the Actions entrypoint (E10-S16) STAYS in E10's scope**, kept last and independently droppable; `later-phases.md` titles the epic "GitHub adapter + Actions entrypoint", so cutting it would have needed the plan text corrected. E10-S16 is no longer operator-gated. | -| D-141 | 2026-08-10 | **E11 (Rego complex-rule backend) implementation is UNLOCKED by direct operator instruction; `openspec/specs/p5-e11-rego-backend/spec.md` decomposes it into 13 stories under ADR-0002 v2.** E11's CONTRACT was already unlocked by D-017; what was gated was IMPLEMENTATION, twice: "after Phase 4" (satisfied — the Phase-4 adoption gate closed with D-042) and, per D-017, **evidence-based per rule** ("each ported rule tries CEL first, the backend is built when a concrete rule demonstrably exceeds the tier-1 ceiling"). This row records the operator lifting that per-rule evidence gate. **What it does NOT waive**: the DESIGN need the gate was protecting — E11-S01 still requires a written tier-1 ceiling document with concrete rules per shape (multi-pass, cross-manifest, set-difference, graph-relationship), and any shape found CEL-expressible is struck from scope. **What it does NOT authorize**: WASM or gRPC predicate backends (still Locked under D-012 — this unlocks Rego only); domain-aware joins and in-process Go rule plugins (D-017 DECLINED both permanently — not deferred); giving Rego any control over aggregation, effects, or points (ADR-0002 v2 boundary); any `EvaluationInput` change. **Two constraints found during design that shape the epic**: (1) **E11 is the first epic whose DoD is `git diff schemas/` != 0.** P3-E1-S02's backend-neutrality guarantee ("no field naming a predicate backend anywhere in the schema") applies to `EvaluationInput` and HOLDS — no decision contract changes — but `schemas/policy/v1alpha1/merge-policy.schema.json` defines the predicate leaf as `additionalProperties:false, required:["cel"]`, so a `rego:` leaf IS a policy-schema change. `API_STABILITY.md:19` permits exactly this within `v1alpha1` as an announced additive change with an openspec change and no `apiVersion` bump; the change is backward-compatible and deliberately forward-INcompatible (an older binary rejects a `rego:` leaf by strict-decode, which is the correct direction — it must not silently ignore a rule it cannot evaluate). A reviewer applying the previous epics' `git diff schemas/ == 0` habit will flag the correct change as a violation; E11-S02-04 scopes the drift guard rather than deleting it. (2) **The reflexive safety measure violates AGENTS.md rule 7.** Rego ships `time.now_ns()`, `rand.intn()`, and `http.send()`, all of which are denied structurally by the D-013 capability sandbox (E11-S04, compile-time failure, golden allowlist so an OPA upgrade cannot widen it unnoticed) — but bounding evaluation with a WALL-CLOCK TIMEOUT would itself be a rule-7 violation, making the same policy over the same ChangeSet decide differently on a slow runner. E11-S06 therefore requires a machine-independent evaluation budget, and exceeding it must be a PROCESS ERROR that can never be a policy outcome; "timeout → BLOCK" is explicitly rejected as machine-dependent while merely LOOKING fail-closed. **Also fixed by the spec**: zero violations NEVER proves a required obligation (`later-phases.md`'s explicit polarity rule; E11-S07-02 tests the failing polarity). **🔴 BLOCKING OPEN SUB-QUESTION (operator) — adopting OPA narrows rule 7's MECHANISM, and both existing purity gates would miss it.** Verified during the design session: `internal/core/purity_test.go` flags only each guarded file's OWN imports (`math/rand`, `crypto/rand`, `net`, `net/*`) and selectors (`os.Getenv`, `time.Now`), and `.golangci.yml`'s `pure-tree` depguard is `list-mode: lax`, deny-only, over DIRECT imports — **neither is transitive**. A file in `internal/core/**` importing `github.com/open-policy-agent/opa/rego` therefore passes both gates GREEN while transitively linking `net/http` (OPA ships the `http.send` builtin), defeating the `net` deny that encodes D-123 / rule 7 invisibly. S04's capability sandbox makes `http.send` uncallable FROM POLICY — the real threat — but the guarantee's nature changes from "the network stack is not linked into the decision path" (structural, greppable) to "linked but unreachable from policy" (behavioural, resting on a capability file). That is a hard-rule change and cannot be made by a story. Options: **(d1)** accept the narrowing with an ADR-0011/rule-7 amendment plus a transitive `go list -deps` purity check allowlisting exactly the OPA path (RECOMMENDED); **(d2)** keep the guarded tree OPA-free by injecting an evaluator from `cmd/assent` — honest, but it moves part of the decision path outside the tree rule 7 guards; **(d3)** drop OPA (a hand-rolled evaluator would be far worse; rejected unless d1 and d2 are). **E11-S04 is blocked on this answer** (it decides the evaluator's package and its gate); S01–S03 are unblocked. Separately on supply chain: OPA is a large dependency with a large transitive tree on a project shipping cosign/SLSA provenance, `govulncheck`, and Scorecard — recommended default is accept-and-pin, with S03 recording the binary-size delta. Revert: re-assert the D-017 per-rule evidence gate, delete the spec directory, leave the `# locked: D-012` quarantine marker in place. **✅ PARTIAL ANSWER (operator, 2026-08-10): `github.com/open-policy-agent/opa` is ACCEPTED as a dependency and pinned** — this closes the *supply-chain* half of judgment call (d) and rejects **(d3) drop OPA**. **🔴 The MECHANISM half is still OPEN and still blocks E11-S03 and E11-S04: (d1) accept the rule-7 narrowing explicitly (capability-enforced, plus a transitive purity guard) vs (d2) keep the guarded tree OPA-free behind an injected interface.** These are not the same question and "accept and pin" does not settle it — **(d2) also accepts and pins OPA**; it differs on *where the evaluator lives* and *which gate enforces rule 7*. Reading the dependency answer as settling the mechanism would silently choose (d1), i.e. narrow a hard rule by merging a story, which rule 6 forbids. Whichever way it resolves needs an **ADR-0011/rule-7 amendment plus its own D-row** (REQ-E11-S04-04). **Also corrected here (2026-08-10, independent review of PR #45):** the spec originally said E11-S01–S03 were unblocked while (d) was pending. **False — E11-S03 is the story that *effects* the narrowing** (REQ-E11-S03-03 adds OPA to `go.mod`; its Test paths sit in the D-123 guarded tree), and because *neither* purity gate is transitive it would have landed green. S03 is now explicitly blocked on (d). The epic also gains **E11-S00**, a deterministic-budget feasibility spike built in a **nested throwaway module** (no `go.work` exists, so Go excludes it from the root module's `./...`) so the question can be answered without adopting the dependency (d) has not authorised; E11 is therefore **14 stories, S00–S13**, and E11-S02 is blocked on S00 because an observe-only `rego:` leaf is a different published contract from a gating one. | +| D-141 | 2026-08-10 | **E11 (Rego complex-rule backend) implementation is UNLOCKED by direct operator instruction; `openspec/specs/p5-e11-rego-backend/spec.md` decomposes it into 13 stories under ADR-0002 v2.** E11's CONTRACT was already unlocked by D-017; what was gated was IMPLEMENTATION, twice: "after Phase 4" (satisfied — the Phase-4 adoption gate closed with D-042) and, per D-017, **evidence-based per rule** ("each ported rule tries CEL first, the backend is built when a concrete rule demonstrably exceeds the tier-1 ceiling"). This row records the operator lifting that per-rule evidence gate. **What it does NOT waive**: the DESIGN need the gate was protecting — E11-S01 still requires a written tier-1 ceiling document with concrete rules per shape (multi-pass, cross-manifest, set-difference, graph-relationship), and any shape found CEL-expressible is struck from scope. **What it does NOT authorize**: WASM or gRPC predicate backends (still Locked under D-012 — this unlocks Rego only); domain-aware joins and in-process Go rule plugins (D-017 DECLINED both permanently — not deferred); giving Rego any control over aggregation, effects, or points (ADR-0002 v2 boundary); any `EvaluationInput` change. **Two constraints found during design that shape the epic**: (1) **E11 is the first epic whose DoD is `git diff schemas/` != 0.** P3-E1-S02's backend-neutrality guarantee ("no field naming a predicate backend anywhere in the schema") applies to `EvaluationInput` and HOLDS — no decision contract changes — but `schemas/policy/v1alpha1/merge-policy.schema.json` defines the predicate leaf as `additionalProperties:false, required:["cel"]`, so a `rego:` leaf IS a policy-schema change. `API_STABILITY.md:19` permits exactly this within `v1alpha1` as an announced additive change with an openspec change and no `apiVersion` bump; the change is backward-compatible and deliberately forward-INcompatible (an older binary rejects a `rego:` leaf by strict-decode, which is the correct direction — it must not silently ignore a rule it cannot evaluate). A reviewer applying the previous epics' `git diff schemas/ == 0` habit will flag the correct change as a violation; E11-S02-04 scopes the drift guard rather than deleting it. (2) **The reflexive safety measure violates AGENTS.md rule 7.** Rego ships `time.now_ns()`, `rand.intn()`, and `http.send()`, all of which are denied structurally by the D-013 capability sandbox (E11-S04, compile-time failure, golden allowlist so an OPA upgrade cannot widen it unnoticed) — but bounding evaluation with a WALL-CLOCK TIMEOUT would itself be a rule-7 violation, making the same policy over the same ChangeSet decide differently on a slow runner. E11-S06 therefore requires a machine-independent evaluation budget, and exceeding it must be a PROCESS ERROR that can never be a policy outcome; "timeout → BLOCK" is explicitly rejected as machine-dependent while merely LOOKING fail-closed. **Also fixed by the spec**: zero violations NEVER proves a required obligation (`later-phases.md`'s explicit polarity rule; E11-S07-02 tests the failing polarity). **🔴 BLOCKING OPEN SUB-QUESTION (operator) — adopting OPA narrows rule 7's MECHANISM, and both existing purity gates would miss it.** Verified during the design session: `internal/core/purity_test.go` flags only each guarded file's OWN imports (`math/rand`, `crypto/rand`, `net`, `net/*`) and selectors (`os.Getenv`, `time.Now`), and `.golangci.yml`'s `pure-tree` depguard is `list-mode: lax`, deny-only, over DIRECT imports — **neither is transitive**. A file in `internal/core/**` importing `github.com/open-policy-agent/opa/rego` therefore passes both gates GREEN while transitively linking `net/http` (OPA ships the `http.send` builtin), defeating the `net` deny that encodes D-123 / rule 7 invisibly. S04's capability sandbox makes `http.send` uncallable FROM POLICY — the real threat — but the guarantee's nature changes from "the network stack is not linked into the decision path" (structural, greppable) to "linked but unreachable from policy" (behavioural, resting on a capability file). That is a hard-rule change and cannot be made by a story. Options: **(d1)** accept the narrowing with an ADR-0011/rule-7 amendment plus a transitive `go list -deps` purity check allowlisting exactly the OPA path (RECOMMENDED); **(d2)** keep the guarded tree OPA-free by injecting an evaluator from `cmd/assent` — honest, but it moves part of the decision path outside the tree rule 7 guards; **(d3)** drop OPA (a hand-rolled evaluator would be far worse; rejected unless d1 and d2 are). **E11-S04 is blocked on this answer** (it decides the evaluator's package and its gate); S01–S03 are unblocked. Separately on supply chain: OPA is a large dependency with a large transitive tree on a project shipping cosign/SLSA provenance, `govulncheck`, and Scorecard — recommended default is accept-and-pin, with S03 recording the binary-size delta. Revert: re-assert the D-017 per-rule evidence gate, delete the spec directory, leave the `# locked: D-012` quarantine marker in place. **✅ PARTIAL ANSWER (operator, 2026-08-10): `github.com/open-policy-agent/opa` is ACCEPTED as a dependency and pinned** — this closes the *supply-chain* half of judgment call (d) and rejects **(d3) drop OPA**. **🔴 The MECHANISM half is still OPEN and still blocks E11-S03 and E11-S04: (d1) accept the rule-7 narrowing explicitly (capability-enforced, plus a transitive purity guard) vs (d2) keep the guarded tree OPA-free behind an injected interface.** These are not the same question and "accept and pin" does not settle it — **(d2) also accepts and pins OPA**; it differs on *where the evaluator lives* and *which gate enforces rule 7*. Reading the dependency answer as settling the mechanism would silently choose (d1), i.e. narrow a hard rule by merging a story, which rule 6 forbids. Whichever way it resolves needs an **ADR-0011/rule-7 amendment plus its own D-row** (REQ-E11-S04-04, the requirement that such an amendment land; the transitive purity check itself is REQ-E11-S04-03). **Also corrected here (2026-08-10, independent review of PR #45):** the spec originally said E11-S01–S03 were unblocked while (d) was pending. **False — E11-S03 is the story that *effects* the narrowing** (REQ-E11-S03-03 adds OPA to `go.mod`; its Test paths sit in the D-123 guarded tree), and because *neither* purity gate is transitive it would have landed green. S03 is now explicitly blocked on (d). The epic also gains **E11-S00**, a deterministic-budget feasibility spike built in a **nested throwaway module** (no `go.work` exists, so Go excludes it from the root module's `./...`) so the question can be answered without adopting the dependency (d) has not authorised; E11 is therefore **14 stories, S00–S13**, and E11-S02 is blocked on S00 because an observe-only `rego:` leaf is a different published contract from a gating one. **✅ MECHANISM RESOLVED (D-144, 2026-08-16): (d1)** — evaluator in `internal/core/policy`, ADR-0011 amended, transitive purity check required before E11-S03. E11-S03/S04 are no longer blocked. | | D-142 | 2026-08-10 | **Two public demo repositories are DESIGNED spec-first as `openspec/specs/p5-dem-demo-repos/spec.md` (15 stories, S00–S14); the answer to "do sample repos already exist" is NO.** Operator asked for a testable sample GitLab repo and sample GitHub repo, two designed self-service patterns, and an assessment of user-resolution extensibility. **Ground truth established before designing:** `examples/repos/**` are content layouts with no `.assent/` tree, `examples/packs/**` are policy trees with no repo identity, `gitlab.com/konrad.heimel/assent-lab` is the operator's personal D-042 adoption-proof project (not a showcase), and GitHub presence is zero because E10 is spec-only (D-140). **The constraint that shapes the epic: assent cannot run on a GitHub PR today**, so the design commits to a **two-tier demo contract** stated on each repo's front page — **tier 1** (`git clone && assent test .`, forge-independent, no token/network/IdP, deliverable now on both repos via the E6 harness) and **tier 2** (live MR/PR: GitLab now, GitHub blocked on E10-S18). Tier 1 is the "everybody can see it" claim. **Split by governance shape, not by tool** — otherwise two repos demonstrate the same three archetypes twice: **repo 1 `assent-demo-platform` (GitLab)** = *request a platform resource* (kafka-topic, **kafka-acl**, **argocd-application**), distinguished by being **referential** — an ACL names a topic and a principal, an App names a project/namespace/source, so the decision cannot be made from the changed file alone, which is exactly what `builtin/resource-owner` (E5-S08, REF-GAP-1) and `builtin/repo-file` (E5-S07) exist for and what no shipped example exercises end to end; **repo 2 `assent-demo-terraform` (GitHub)** = *change infrastructure* (**tf-module-instance** raw HCL, tf-vars, **tf-backend** deliberately ungoverned→REVIEW per D-063), distinguished by **magnitude/blast radius** plus the opaque-change fallback. Repo 2 also becomes **E10-S18's live adoption target**, so the epic needing a real GitHub repository gets one instead of minting a throwaway. **Judgment calls: (a)** demo trees live in-tree under `examples/demo//` and are mirrored out — authoring only in the public repos puts them outside `task check` and an ungated example rots; **(b)** platform→GitLab / terraform→GitHub is idiom plus the S18 target, and repo 2's README states the pending live tier in its first screenful; **(c)** no Keycloak/Entra/LDAP builtins (see OQ-32 rationale); **(d) 🔴 OPERATOR — creating public repos under the `PlatformRelay` org is outward-facing and NOT covered by AGENTS.md rule 2's push grant to `PlatformRelay/assent`**, so DEM-S13 is operator-gated and S01–S12 are unblocked by it; **(f)** DEM-S05's cross-manifest rules double as the **E11-S01 tier-1 ceiling probe** D-141 requires (cross-manifest is one of its four named shapes), feeding evidence rather than hypotheticals — and DEM-S05 does not wait for E11. **On user-resolution extensibility, the verified verdict is: the seam is genuinely good and extending it needs NO core change and NO fork** — the wire contract is frozen and published (`schemas/provider/v1alpha1/{request,response}.schema.json`, `additionalProperties:false`, fail-closed state machine with `resolved ⇒ value+expiresAt` encoded in-schema), `API_STABILITY.md:20` binds protocol majors, and extension is a **two-file repo-side change read from the PROTECTED TARGET REF** (`.assent/config.yaml` + `.assent/providers/.json`, per `cmd/assent/provider_host.go:61,81-88`), so an MR author cannot redefine their own fact semantics. **But four gaps stand between that and "easily extendable", and the spec states all four rather than demoing around them: G1 (P1)** — `find examples -type d -name providers` returns NOTHING; all three shipped packs declare `providers:` with no host declaration, so `provider_host.go:83-87` silently `continue`s and **every shipped example's provider resolves to nothing**, with the required path documented in no doc (DEM-S01 fixes examples + docs + a both-polarity gate); **G2 (P1, architectural → OQ-32)** — `CallHTTP` sets only `Content-Type`, the repo-side provider schema is `additionalProperties:false` over `{type,url,failure}`, and `ScrubEnv`/`ScrubArgv` refuse `(?i)TOKEN\|SECRET` names even when explicitly configured, so **no provider can call Entra ID or Keycloak directly** — both need a bearer token; the only working shape is a **broker** holding the IdP credential itself, which is defensible (ADR-0015 §7 intact, Spike C's hostile-provider isolation proof preserved) but undocumented and narrows what `docs/vision.md:67` promises; **G3 (P2)** — exec providers force adopters to recompute a `sha256:` pin per release, so the reference provider is HTTP; **G4** — ADR-0004 §1 listed OIDC/Keycloak/LDAP builtins that never shipped, and the epic **recommends never shipping them**: each is an unbounded auth surface entering the decision path's dependency tree for no capability HTTP lacks, and a published contract plus a copyable reference implementation is the stronger answer because it works for the adopter whose IdP nobody has heard of (`docs/architecture/c4-context.md:19` is already the only place telling this truth). The demo therefore ships a **four-layer ladder** (L0 fixtures → L1 `repo-file`/`resource-owner`, no network → L2 `forge-groups` → L3 your IdP broker) with a runnable `contrib/providers/idp-groups/` reference carrying Entra and Keycloak adapters, promoted from `hack/spikes/provider/toy.go`; the L2→L3 step is **two files changed, zero lines of assent rebuilt**. **No new ADR proposed** (examples/docs/contrib only) — unless OQ-32 resolves to adding a credential channel, which would need its own. **🔴 SECOND FINDING, more consequential than the GitHub gap and recorded here because rule 6 forbids deciding it silently: `(class, environment)` binding routing is NOT WIRED, so a multi-class demo repo fails closed in BOTH tiers on BOTH forges.** Found by reading the code against the design. `cmd/assent/run.go:493` `selectBinding` fails closed on any RulesetBinding with >1 binding — its own comment says the Config class-matcher *"is not wired in this lane"*; `cmd/assent/test.go:366` `selectBindingForTest` collapses a multi-binding document to its STRICTEST binding (D-060, fail-safe direction) but **fails closed when two bindings differ in `class`, `packs`, or `require[]`**. Both demo repos have three classes with different `require[]`, so as designed they fail closed on `assent run` AND `assent test`. **Why nobody noticed:** D-060 itself recorded the shipped packs' dev/prod split as *"empirically decision-neutral for the corpus"* — every shipped example is single-class with a decision-neutral environment split, so the collapse is invisible and the matcher is never missed. The demo is the first artifact needing real routing. **Consequence:** the epic gains **DEM-S00** ahead of everything (`[engine-grade · maintainer LGTM]` — it changes decision routing at the `cmd/assent` seam). The *matcher* is cheap — `policy.Config` already carries `Environments`/`Classes` as `NamedMatch` with `Match PathMatch` (`internal/core/policy/policy.go:196-228`), populated in every example pack, and `internal/glob.Match` already implements `*`/`**` — so `internal/core` stays byte-unchanged. **🔴 CORRECTION (2026-08-10, independent review of PR #47): this row originally said the data "verified to reach the call sites — no loader work". That was FALSE at both call sites and the correction changes S00's size.** On `assent run`, `selectBinding` is `run.go:219` but the Config load is `run.go:224-237` and is guarded by `if cfg.config != ""` — Config is loaded *after* selection and only when `--config` is supplied, so S00 must move the load ahead of selection **and decide the `--config`-absent behaviour explicitly** (it must fail closed, never fall back to a collapse or to binding zero). On `assent test` there is **no Config at all**: `test.go:67` uses `catalogue.LoadFromDir`, and `internal/catalogue/catalogue.go:124-127` states Config is *"deliberately absent; a later story that needs config-derived fields adds it then"* — `Input` is `{Packs, Bindings}`. S00 **is** that later story, so extending `catalogue.Input` is a deliberate **E6 contract change**, not wiring. `Config.Classes` has zero production readers today — it is precisely parsed-and-discarded. This is why tier 1 is the risk: an implementer who wires only `run.go` leaves `assent test` — the epic's primary deliverable — failing closed while the story reads done. S00 preserves fail-closed where routing is genuinely ambiguous (two matching classes, or a `(class, environment)` pair with no covering binding — both refuse, never guess), keeps `compare.go:407` fenced as D-060 left it, and **DELETES the `selectBindingForTest` collapse rather than leaving it as a fallback** (two live paths is how a fail-closed guarantee is lost quietly). `internal/core` byte-unchanged, `git diff schemas/` == 0. **Also newly visible: the epic's clearest demonstration — the identical diff that APPROVEs in `dev` and REVIEWs in `prod` — is impossible in either tier today**, which is why it becomes S00's sharpest acceptance test. **Third, smaller correction folded in:** D-063 covers the unmatched whole-file DELETE only (`aggregate.unmatchedDelete`); an unmatched EDIT is governed by ADR-0008 **§1**'s implicit `unclassified` class that no vouch rule may match (**correction, 2026-08-10, independent review of PR #47: the original text of this row cited a non-existent "ADR-0008 §27" — ADR-0008 has §1–§4 plus two amendments and no §27. The substance was right and the pointer was fabricated: the implicit class is §1 (`internal/core/classify/classify.go:18-20`) and the no-vouch-rule-may-match guarantee is enforced by `classify.ValidateRouting` / `ErrReservedClassRouting` at `classify.go:127-145`, whose own error message cites the ADR-0008 amendment + ADR-0015 §1**). **Two further corrections from the same review, recorded because they narrow what this row promises:** (i) **repo 1's headline differentiator is demonstrable only at tier 2.** The evaluation unit is ONE file — `assent run` takes a single `--subject file:` (`cmd/assent/run.go:266`) and diffs it alone (`:289`), and `adoptertest.Case` is singular — so the *referential* / cross-manifest claim above (an ACL naming a topic deleted in the same changeset) cannot be shown at tier 1 at all. It moves to **DEM-S14**, which is `[infra-gated · operator]`, via `builtin/repo-file`'s merged-result checkout. (ii) **the four-layer ladder's L1 rung does not run at tier 1 either**: `cmd/assent/test_provider_fence_test.go:69-80` fails the build if `assent test` touches the live provider host, so tier-1 facts come solely from `adoptertest.MapFacts` — a reader at tier 1 sees a fixture literal, not ownership resolved from repo files. Tier 1 therefore remains an honest and complete demonstration of **L0**, and the "everybody can see it" claim stands only at that rung., and `run.go:487-492` records that routing as unwired too — so DEM-S10 must DETERMINE the unmatched-edit behaviour by running the code rather than assert it, and anything other than a refusal or REVIEW is a finding to log, not a demo feature. Revert: delete `openspec/specs/p5-dem-demo-repos/`, this row, and OQ-32. Note the revert is not free of consequence — DEM-S00 describes a real routing gap that outlives this epic; reverting the design does not close it, and D-060 remains its standing record. | | D-143 | 2026-08-15 | **Complex examples/tests/docs are a dedicated epic P5-EX, not an implementation of P5-DEM.** Operator asked for better and more complex in-tree examples, adopter tests, and documentation (multi-field nested resources; YAML + JSON + HCL + tfvars). The E1 adapters already exist; the thin layer is packs/fixtures/docs. **Options:** (A) implement P5-DEM after adding AUD2-S06-style `Test:`/`Verify:`/`Level:` annotations to DEM REQs — rejected: DEM is public org demo repos + engine-grade `(class, environment)` routing (DEM-S00) + provider brokers + operator-gated publish (DEM-S13), a different goal, and DEM today has **0** REQ annotations so "green" is undefined; (B) **dedicated P5-EX epic** (`openspec/specs/p5-ex-complex-examples/spec.md`) that **extends** `examples/packs/`, `examples/repos/` layouts, `examples/archetypes/`, `assent test` fixtures, and product `docs/` and closes REF-EX C1–C8 — **chosen**; (C) docs-only — rejected: untested README claims are the AUD-S06 class of defect. **Chosen B.** EX does not re-spec DEM, does not invent a parallel example system, does not duplicate E1 adapters, does not start E10/E11/SEC-SC/AUD2, does not wire DEM-S00, does not raise `COVERAGE_MIN`. Packs stay single-class. HCL story is honest: structured literal tfvars + `.tf` blocks opaque→REVIEW (`TestHCLStructuralGuardsFailSafe`; no new parser). `task dogfood-examples` is wired into `task check` and shares discovery with `verify.yaml` (steal DEM-S12 *wiring*, not demo-repo scope). Revert: delete `openspec/specs/p5-ex-complex-examples/`, remove the P5-EX table from `openspec/specs/backlog.md` and the P5-EX paragraph from `openspec/specs/later-phases.md`, restore the REF-EX row to OPEN; this row remains as a superseded decision if a later D-row replaces it. | +| D-144 | 2026-08-16 | **D-141's blocking sub-question (d) is RESOLVED: (d1) — accept the rule-7 narrowing explicitly, guarded by a transitive purity check.** The Rego evaluator lives in `internal/core/policy`. **ADR-0011 is amended**: hard rule 7's guarantee for that package changes from "the network stack is not linked into the decision path" (structural) to "linked but unreachable from policy" (behavioural, capability-enforced) — E11-S04's compile-time capability sandbox denies `http.send`/`time.now_ns`/`rand.intn` with a golden allowlist an OPA upgrade cannot widen unnoticed. **Rejected (d2):** injecting the evaluator from `cmd/assent` keeps `internal/core` OPA-free by letter but moves live decision-path code outside the tree rule 7 guards — a weaker guarantee dressed as a stronger one. **New requirement, closes a hole that exists independent of this decision:** `internal/core/purity_test.go` and `.golangci.yml`'s `pure-tree` depguard are both non-transitive today — a file importing `opa/rego` passes both green while transitively linking `net/http`. A **transitive `go list -deps` purity check**, allowlisting exactly the OPA import path, is required before E11-S03 lands (REQ-E11-S04-03; this D-row itself fulfils REQ-E11-S04-04, the requirement for an ADR amendment plus a D-row before E11-S05 — see ADR-0011 Amendment 4). **Unblocks E11-S03 and E11-S04.** Revert: re-assert D-017's per-rule evidence gate; the transitive purity check should be kept regardless — it is a strict improvement over today's non-transitive gates. | +| D-145 | 2026-08-16 | **OQ-29 is RESOLVED: (a) — implement the gate. `PolicyProfile.spec.writes: false` becomes runtime-enforced, not just documented.** `cmd/assent/run.go` will load the covering profile on the run path and refuse `forge.Reconcile` when `Result.WriteAllowed` is false. The write switch at `run.go:419-433` already has two zero-write arms (`reservedSelfEditBlock`, `untrustedExecutionContext`); this adds a third. Chosen over (b) retract-the-invariant because the claim is already published (`docs/architecture/policy-profiles.md`, 200-OK, search-indexed) and shipped in v0.1.0/v0.2.0 — retracting a shipped safety guarantee is worse than making it true — and over (c) accept-and-annotate because the gap is a small, contained addition to an existing switch, not an architectural change. **Stopgap, same day as this row:** `docs/architecture/policy-profiles.md` gets an explicit "not yet enforced, tracked by this row" annotation until the code lane lands, so the page stops asserting a guarantee nothing holds. **Implementation is its OWN independently-reviewed lane** (decision-path adjacent) — not landed by this row. Verification target: `assent run` against a `writes: false` covering profile performs zero forge writes, mirroring the shape already pinned by `run_self_vouch_test.go:27-43`. Revert: strike the annotation, leave the gap; would need a fresh D-row citing new information. | +| D-146 | 2026-08-16 | **OQ-31 is RESOLVED: (a) — "zero forge writes" on the GUARD-1 self-edit BLOCK path stays absolute; the BLOCK is surfaced via a required CI job status, not a forge write.** The `DecisionRecord` is already emitted at `run.go:409`, before the guard switch, so the BLOCK is on stdout/`--emit` today — only a forge-visible channel was missing. A required CI job reads the emitted record and fails the check when the decision is BLOCK-via-self-edit, giving the human reviewer a visible signal without touching the exit-code contract `docs/usage/cli.md` documents for wrapper scripts. Chosen over (b)/(c) because both permit a write and thereby reopen the frozen acceptance criterion in `openspec/specs/p5-aud-audit-remediation/spec.md` ("BLOCK with zero forge writes, GUARD-1 dominance"), requiring an openspec change proposal before any code; (a) reopens nothing. Implementation (the CI job) is a separate lane, before the RELI-01 fix lands. Revert: leave the reviewer without a forge-visible BLOCK signal (today's status quo). | +| D-147 | 2026-08-16 | **OQ-32 is RESOLVED: (b) — add a credential channel to the provider transport, via a host-side secret resolver; an ADR amending ADR-0015 §7 is required before any code.** Operator constraint: secrets must work across GitLab CI variables, GitHub Actions secrets, and the hosted variant; Vault injection is one pattern among several, most adopters will set plain CI variables. This is the higher-cost option — it reopens both a frozen schema and the trust boundary Spike C's hostile-provider isolation proof rests on — chosen over (a) bless-the-broker-pattern (docs-only, zero cost, but does not give adopters a direct-credential option) because the operator wants flexibility broker-only cannot offer. **Design constraint carried into the ADR, not yet written:** the credential must be **host-side only** — resolved by process env (CI vars), file path (Vault/CSI-injected secrets), or a hosted secret store — while the **repo-side** `config.schema.json` (attacker-editable, the same tree the MR under evaluation can change) gains only an **opaque reference name the host must have pre-allowlisted**. An MR must never be able to pair a credential with a URL of its own choosing; a repo-side literal secret-ref would make the provider block self-editing an exfiltration path. `ScrubEnv`/`ScrubArgv`'s `TOKEN\|SECRET` stripping for the exec tier is restated explicitly in the ADR, not silently loosened. Next: write the ADR (host-side resolver design, pluggable sources), decompose into openspec stories, then the provider-author guide (DEM-S02) states the answer. Revert: fall back to (a), broker-only, document and stop. | +| D-148 | 2026-08-16 | **OQ-30 is RESOLVED: (b) — keep the `pull_request`-scoped CHANGELOG-drift guard skipped, and record the mechanism that was actually found rather than leave the row open.** D-125's original reason (synthetic merge-ref subject breaks the render) died when D-136 started skipping merge commits; four live measurements then failed to reproduce a successor failure. The real mechanism, found while measuring: **merge DIRECTION decides the outcome.** A clean textual auto-merge whose line order differs from git-cliff's topological traversal order reds with no author error and no author fix available — reproduced by merging `origin/main` *into* a lane (lane as first parent). GitHub always mints PR merge refs base-first, the direction that matched cleanly on both measured cases, but that is two measurements, not a proof that the ordering hazard can never surface on `pull_request`. Chosen over (a) enable-on-pull_request because the false-positive mechanism above is real and unfixable by the author if it recurs, and behaviour on `pull_request_target`, fork PRs, and post-force-push re-mints is untested; over (c) keep-permanently-on-cost-grounds because it would discard genuine evidence worth keeping. The guard stays fail-safe regardless — it still runs in `task check` and on push-to-main (`.github/workflows/verify.yaml:103`). Revert: re-open if a `pull_request`-scope red is ever observed that is a false positive under this mechanism — that would upgrade this from "recorded reason" to "confirmed hazard" and argues for (a) with a compensating check instead. | +| D-149 | 2026-08-16 | **OQ-1 / D-031's apiVersion rename is SUSPENDED pending the operator's in-flight brokered purchase of `assent.dev`.** The shipped schemas already carry `apiVersion: assent.dev/v1alpha1` and `$id: https://assent.dev/schemas/...`; D-028 recorded `assent.dev` as third-party-owned, and D-031 ratified Path A (register an owned domain) as the fix. The operator has since initiated a brokered transfer of `assent.dev` itself via GoDaddy — not yet closed. If it completes, D-028's premise is void and the already-shipped string is correct: **zero adopter migration.** Registering and cutting to a fallback domain now would risk a breaking `apiVersion` change for v0.1.0/v0.2.0 adopters that turns out to have been unnecessary. **Named fallback, only if the transfer fails: `assent.platformrelay.dev/v1alpha1`** — a portfolio-wide domain (`platformrelay.dev`) rather than a fresh `assent-*` registration, so the group string is settled once for every future PlatformRelay repo rather than re-litigated per project. **Checkpoint:** re-visit this row at the next handover if the transfer has neither closed nor visibly failed, so it cannot rot silently. Revert: register the fallback domain immediately and cut over — abandons the free outcome without waiting for its resolution. | +| D-150 | 2026-08-16 | **SONAR-GO-CX-TEST is RESOLVED: adopt a checked-in `sonar-project.properties` excluding `**/*_test.go` from the S3776 cognitive-complexity rule, accepting the loss of SonarCloud Automatic Analysis.** 66 CODE_SMELL issues, concentrated in 34 table-driven Go test files, against a Quality Gate that is otherwise OK (0 bugs/vulns/hotspots) — high cognitive complexity is the idiom in table-driven Go tests, not a defect. Chosen over the UI-only Won't-Fix route (keeps Automatic Analysis, but the suppression is not reviewable in-tree and does not survive a project re-import) and over refactoring the 34 files (churn on passing tests for a non-gating metric). Accepted cost: a scanner step must be added to CI and maintained going forward — Automatic Analysis stops once a `sonar-project.properties` is present. Revert: delete the properties file to restore Automatic Analysis; the 66 smells return. | +| D-151 | 2026-08-16 | **OpenSSF Best Practices project creation is RESOLVED: (a) — the operator will create the `bestpractices.dev` project for `PlatformRelay/assent`.** Unblocks SEC-SC-S02 (evidence page + README badge, once *passing*); SEC-SC-S01 (fuzzing) is not blocked by this and can proceed independently. Operator action only — requires a personal account at bestpractices.dev, not automatable in-tree. Current CII score is 0, which drags the OpenSSF Scorecard (~7); the repo already satisfies most criteria (pinned CodeQL, Dependabot, secret scanning, signed releases, SLSA provenance). Revert: leave CII at 0 indefinitely — SEC-SC-S01 is unaffected either way. | diff --git a/docs/planning/open-questions.md b/docs/planning/open-questions.md index 3e5907b..b0f47f1 100644 --- a/docs/planning/open-questions.md +++ b/docs/planning/open-questions.md @@ -2,7 +2,7 @@ | ID | Question | Blocks | Notes / leading answer | | --- | --- | --- | --- | -| OQ-1 | ~~Project name~~ **Resolved: assent** (D-009); repo live (D-014). **Domain:** `assent.dev` taken (D-028). **Path:** Option A — register an owned domain (D-031). **Exact domain string still TBD** before rename. | Phase 4 / before public freeze marketing | [naming.md](naming.md); D-028/D-031 | +| OQ-1 | ~~Project name~~ **Resolved: assent** (D-009); repo live (D-014). **Domain:** `assent.dev` was taken (D-028); **operator has since initiated a brokered purchase of `assent.dev` itself (GoDaddy, not yet closed).** Rename **SUSPENDED pending that transfer** (D-149) — if it completes, the shipped `apiVersion: assent.dev/v1alpha1` is already correct and no migration is needed; named fallback if it fails is `assent.platformrelay.dev/v1alpha1`. | — (suspended, checkpoint at next handover) | [naming.md](naming.md); D-028/D-031/D-149 | | OQ-2 | ~~Hosting: GitHub only, or GitLab mirror (dogfooding the GitLab adapter on our own repo)?~~ **Resolved (D-105):** defer mirror — GitHub canonical; optional read-only GitLab mirror is operator infra, not E9 blocker. Dual-primary rejected (drift risk). No mirror workflow in E9. | — | E9-S11 | | OQ-3 | ~~Two parallel frontends?~~ Resolved by ADR-0002 v2: one YAML envelope, pluggable predicate backends | — | superseded; successor questions: OQ-11/OQ-12 | | OQ-4 | ~~Ship gRPC (`go-plugin`) tier in v1?~~ **Resolved (P2-E5):** defer gRPC to post-v1; HTTP/exec + builtins only ([Spike C](spikes/spike-c-provider.md), D-012, ADR-0004 Accepted) | — | [adr-acceptance-review.md](adr-acceptance-review.md) | @@ -10,7 +10,7 @@ | OQ-6 | ~~E2E default in CI: kind vs testcontainer?~~ **Resolved (P2-E5 / Spike B):** **testcontainer in CI**; kind stays for local/demo ([spike-b-e2e.md](spikes/spike-b-e2e.md) — boot p50 96 s vs 126 s, ~2.4 GB vs ~3.1 GB+node, 0 flakes) | — | ADR-0006 Accepted | | OQ-7 | ~~GitHub mapping for `challenge`~~ **Resolved (P2-E5):** parity for the gate, not the device — required-conversation-resolution carries acknowledgement; `REQUEST_CHANGES` reserved for block ([forge-dossier-github.md](forge-dossier-github.md) §3). Residual live checks → Phase 5 / E10 | Phase 5 / E10 | ADR-0005 Accepted | | OQ-8 | Decision replay/audit: JSON report artifact enough, or signed/attested decision record later? | Phase 3 | v1: artifact (Pins in report); attestations later epic | -| OQ-9 | Version pinning for reproducibility (tool digest + policy SHA in report `Pins`)? | Phase 3 | must be in the report schema from day 1 | +| OQ-9 | ~~Version pinning for reproducibility (tool digest + policy SHA in report `Pins`)?~~ **Resolved (D-120):** `schemas/decision/v1alpha1/decision-record.schema.json` requires `toolVersion, toolDigest, policySha, sourceSha, targetSha, mergeResultDigest, factsResolvedAt`. | — | D-120 | | OQ-10 | Monorepo support: multiple policy scopes per repo (path-scoped `.assent/` dirs)? | Phase 3 | likely bindings-level path scoping | | OQ-11 | ~~kyverno-json vs cel-go~~ **Resolved (P2-E5):** **cel-go** (ADR-0013 Accepted; [Spike A](spikes/spike-a-cel.md)) | — | [adr-acceptance-review.md](adr-acceptance-review.md) | | OQ-12 | ~~assert authored syntax~~ **Resolved (P2-E5):** hybrid `all`/`any`/`not` trees with CEL leaves + per-leaf `message` (ADR-0013 Accepted) | — | [adr-acceptance-review.md](adr-acceptance-review.md) | @@ -30,8 +30,7 @@ | OQ-28 | ~~**Filesystem containment for provider reads: is PATH containment enough, or must the injected FS itself be a security boundary?** (raised P5-E5-S07/S08 while implementing `builtin/repo-file` and `builtin/resource-owner`.) The builtins clip candidates to declared `roots` with pure string guards (`cleanRel`/`underAnyRoot`) over an `os.DirFS`. Under `--checkout` that FS is the merge request's own HEAD tree — contributor-authored content — and Go documents `os.DirFS` as **not** a security boundary while `fs.Stat` follows links. Question: does the invariant "never a fact from outside the declared roots" need a syscall-level root, a per-component symlink refusal, or both?~~ **Resolved (D-129): BOTH, and they are not substitutes.** (a) `cmd/assent` injects `builtin.OpenRepoRoot` = `os.OpenRoot` + `(*os.Root).FS()`, a syscall-level boundary for every consumer of that FS; (b) `classifyCandidate` Lstats every path component and refuses any symlinked candidate — the only layer that can protect the `roots` clip, which `os.Root` cannot see. In-root symlinks are refused too; refusal is `unavailable` with a contributor-readable reason and STOPS the walk-up. Retroactive row: D-129 and `REQ-E5-S07-03` cited "OQ-28" before this table carried it (AGENTS.md rule 6 — no dangling references). | — (closed) | [decisions.md](../decisions/decisions.md) D-129/D-130; `REQ-E5-S07-03`/`REQ-E5-S08-03`. **Residual CLOSED (D-133):** `collectTree`'s silent truncation (P0) and `readIfPresent`'s governed-subject symlink (P1) are both fixed in `cmd/assent/checkout.go`. **Proof relocated — stated here so nobody re-derives it wrongly:** D-133 refuses ANY symlink under `base/`/`head/` at changed-file ENUMERATION, before providers resolve, so this row's escape is no longer reproducible end-to-end through `assent run --checkout`. The provider guard is now defence in depth, proven at `cmd/assent`'s production fact-resolution seam (`TestResolveRunFactsRefusesSymlinkedQuotaCandidate`, which pins the two layers separately) plus `internal/provider/builtin/{repo_file,resource_owner}_symlink_test.go`; it becomes the live barrier again if ADR-0008 Amendment 2's fold-the-refusal-opaque direction lands — see D-129's 2026-08-09 amendment | | OQ-16 | ~~Which **open-source repos** join the demo/test corpus?~~ **Resolved (P2-E5):** kafka/org + JulieOps descriptors + octoDNS zones, pinned by SHA with vendored excerpts — see [examples/repos/corpus.md](https://github.com/PlatformRelay/assent/blob/main/examples/repos/corpus.md) | — | [adr-acceptance-review.md](adr-acceptance-review.md); D-008/D-029 extra private shapes deferred but kept in corpus plan | | OQ-27 | ~~A relational CEL leaf over STRING-bound operands returns a silently WRONG boolean instead of erroring — a verified BLOCK→APPROVE flip (found by AUD-S13 / TEST-02, widened by review F4).~~ **RESOLVED (D-131 / ADR-0013 Amendment 1, merged on `main`):** an ordering operator over a text-shaped operand (`string` or `bytes`) now ERRORS — `internal/core/aggregate`'s `textOrderGuard` watches every relational operand as it evaluates and refuses text in either position, on both seams (`evalLeaf` and the walking-skeleton `evalRule`) — and `toCEL` no longer demotes an unrepresentable numeric literal to its string form, it binds a CEL error value. Ordering raw text graduates to Rego; `int()`/`double()`/`timestamp()` stay the tier-1 migration path. **The analysis below is retained as the record of how the defect was found and how far it reached — it describes the PRE-FIX engine.** **The class is any string-bound operand, not just numeric overflow.** cel-go's relational operators are DEFINED over two strings (lexical compare), so they return a clean boolean where the engine's fail-safe design assumes an error. **P1 instance — quoted YAML scalars, no overflow anywhere:** `internal/evaldecode` maps a `!!str` literal to a Go `string` BY DESIGN (the differ deliberately keeps the string `"12"` distinct from the number `12`) — but that design assumed a numeric rule over a string would fail safe, and it does not. Reproduced end-to-end through the production `aggregate.Cover` entry point with the D-016-shaped `partitions-must-not-shrink` rule (`new >= old`, onFailure block): `partitions: 12 → 6` (numeric) yields **BLOCK, 1 finding `partition-count-shrunk`**; the identical policy and subject with `partitions: "12" → "6"` (quoted) yields **APPROVE, ZERO findings** — `evalLeaf` returns `(true, nil)`, the obligation is recorded as PROVEN, and the destructive change auto-merges. **Second instance — numeric overflow (the original finding):** a `json.Number` fitting neither int64 nor float64 falls back to its string form (`evaluate.go:191`), so `9e399 > 1e400` evaluates `true` (arithmetically false). **No lint guard exists:** `checkLeafScope` and `checkPredicateScope` (`internal/lint/scope.go`) validate identifier SCOPE and `checkFactsShape` (`facts_ref.go:249`) validates facts-path shape — none type-checks relational operands, so an author gets no warning. This is the exact failure `internal/evaldecode`'s package doc warns about ("lexically `\"6\" >= \"12\"` is TRUE, so a partition shrink 12->6 would be judged non-destructive and APPROVE. That is the exact forbidden outcome") — the doc believed it had closed it; it closed only the *canonical-render* path, not the authored `!!str` path. **Also a docs-truth defect:** `evaldecode.go:61` names this "the ADR-0013 residual #1 the S02 evaluator owns" but describes it as "float64 (a lossy compare) or its string form", never saying the string form yields a silently wrong boolean rather than an error — so the residual reads as benign precision loss. **Severity split, kept explicit because conflating these is how a real finding gets dismissed:** the MECHANISM is **P1**; the over-range instance ALONE is **P2** on reachability (it needs BOTH operands to exceed ~1.8e308); the P1 rests on the quoted-string case, which needs only ordinary authored YAML. **Hard rule 7 (determinism) is NOT violated** — a lexical compare is perfectly deterministic and reproducible. What is violated is the **fail-safe direction** (GUIDELINES §2 / ADR-0013: undecidable or type-mismatched must error → REVIEW, never a permissive boolean) and `evaldecode`'s own written claim. AUD-S13 deliberately wrote NO test asserting `9e399 > 1e400 == true` or the quoted-string `true`; blessing either would enshrine the fail-open. Candidate fixes: make a relational leaf over string-bound operands ERROR (fail-safe, preferred — a lexical compare is almost never what a policy author meant); and/or a lint hard-error when a relational operator can bind a string; and/or reject over-range numerals at the loader boundary. Rejected: big.Float (reintroduces a decision-path numeric tower). | ~~release tag BLOCKED on this~~ **unblocked**; severity ruling done (P1), fixed in its own decision-path lane | found by AUD-S13 (PR #35), widened by independent review F4; NOT fixed there (tests-only lane). Fixed on `main` by **D-131** in a dedicated decision-path lane, as required; AUD-S13's TEST-02 was realigned to the refusal contract when this lane merged `main` | -| OQ-28 | **`builtin/repo-file` enforces PATH containment but not FILESYSTEM containment — a symlink reads arbitrary ABSOLUTE host paths, live in production (found by AUD-S13 / TEST-06, widened by review F5).** `cleanRel`/`cleanRoots`/`underAnyRoot` (`internal/provider/builtin/repo_file.go`) are pure STRING guards: they reject `../`, absolute anchors, root-escaping and sibling-prefix paths, and `TestRepoFileContainment` pins all of that. They cannot see the filesystem, and **`os.DirFS` is explicitly documented in Go as NOT a security boundary**. Path-level and filesystem-level containment are DIFFERENT guarantees; only the former is held. **Two forms, the second worse:** (1) a DIRECTORY symlink under a declared root — `topics/evil -> `, `Roots: ["topics"]`, `Anchor: "topics/evil/orders.yaml"` → a `resolved` fact valued from outside the roots; (2) review-reproduced and stronger, a **FILE symlink at a wholly legitimate in-root path** — `topics/prod/quota.yaml -> /abs/outside/cluster-secrets.yaml` — where every path string the guards inspect is impeccable and the target is an **arbitrary absolute host path**, not merely repo-relative. Either way this contradicts the AUD-S13 story's own criterion ("never a fact from outside the roots"). **This is LIVE IN PRODUCTION, not a test-only shape:** `cmd/assent/provider_host.go`'s `checkoutFS()` builds `os.DirFS(/head)` — the **MR head tree**, i.e. attacker-authored content — reachable via `assent run --checkout`. So the symlink arrives IN THE VERY MR UNDER EVALUATION. **Exfiltration seam:** a resolved, non-sensitive fact value reaches the forge-facing comment body through `render.formatMessageScalar` → `displayFactValue` (`internal/render/message.go` / `redact.go:18`), so a rule whose `message` interpolates a fact value **echoes the file's contents into the MR comment**; `sensitive: true` renders `[redacted]`, which is the only thing limiting it. **Constraint that keeps it narrow (do not overstate the finding):** the symlink target must be a YAML/JSON **mapping** carrying a top-level key equal to a declared output name — so it is not arbitrary file read of any format, it is arbitrary read of attacker-chosen *mapping-shaped* files, plus influence over any fact derived from them. **DISCRIMINATING QUESTION for the severity call — must be answered before ruling:** does `internal/change`'s differ/classifier see a symlink SWAP of a governed path as a change to that path? Git records the mode change 100644→120000, so forge-snapshot changed-file enumeration SHOULD surface it (and governance would then apply) — but this is asserted, not yet verified, and it decides whether the finding is "attacker can read arbitrary mapping-shaped files" or the strictly worse "attacker can **silently** influence a decision with no governed change visible". **Sibling with the same defect and a worse blast radius:** `builtin/resource-owner`'s `LoadResourceOwnerMap` (`internal/provider/builtin/resource_owner.go:93`) does a bare `fs.ReadFile(fsys, file)` over the same repo FS with **NO roots and no containment at all** — and that registry decides **who may approve**, so a symlinked ownership file is an authorization-boundary issue, not just a fact-integrity one. Candidate fix: `os.OpenRoot()` + `(*os.Root).FS()` (compile-verified available on this module's `go 1.25.0`), which refuses symlink traversal out of the root; alternatively reject symlinked candidates explicitly before read. `RepoFileOpts.FS` should also gain a documented contract ("must be a symlink-safe root") so the guarantee cannot be lost again by the caller. | **release tag BLOCKED on this**; severity ruling pending the differ question → dedicated provider lane | found by AUD-S13 (PR #35), widened by independent review F5; NOT fixed there (tests-only lane). Audit the checkout-mode change reader and every builtin that takes an `fs.FS` for the same assumption | -| OQ-29 | **`PolicyProfile.spec.writes: false` is a frozen-schema field with NO runtime enforcement, and lint compels adopters to author it. Operator ruling needed.** [`docs/architecture/policy-profiles.md`](../architecture/policy-profiles.md) states the recorder-only guarantee as an **"architectural invariant, not a runtime best-effort check"** — line 13: a `writes: false` profile **"Never calls `Reconcile` — no approve, merge, block, thread sync, or other forge write"**. No code enforces it, because nothing on the write path reads it. Verified by grep over non-test sources: `aggregate.ResolveProfile` and `Result.WriteAllowed` have consumers only in `internal/lint/posture.go` and inside `internal/core/aggregate` itself; `aggregate.CoverWithProfile` is called only from `internal/compare`; `policy.LoadProfile` is called only from `cmd/assent/compare.go`; and **`cmd/assent/run.go` contains the string `Profile` zero times** — it evaluates via `aggregate.CoverWithPhaseCeiling` (`run.go:533`) and reaches `buildDesired`/`forge.Reconcile` without ever loading or consulting a profile. `internal/core/aggregate/profile.go:97` documents the missing link in its own words: *"A downstream forge step reads `WriteAllowed` to know whether this run may arm/merge or is recorder-only"* — **there is no such downstream forge step.** So a `writes: false` profile does not make `assent run` recorder-only; the run behaves exactly as if no profile existed. **Why it is not merely internal:** `writes` is a REQUIRED field of the frozen `schemas/policy/v1alpha1/profile.schema.json`, whose description reads *"true = this profile authorizes forge writes for bindings in its scope; false = recorder-only"*, and the `single-writer-profile` lint hard error (`internal/lint/posture.go:83`) fails a tree where zero or more than one `writes: true` profile covers a binding — so adopters are **compelled** to author a field whose `false` value does not do what the schema says. **RAISED TO P1 on 2026-08-09 — the stated escalation condition was ALREADY TRUE when it was written (audit DOC-04).** The original text read: *"Severity today is P2 only because `docs/architecture/policy-profiles.md` is NOT in the mkdocs nav, so the invariant claim is not on the docs site. If that directory ever enters the nav it becomes P1."* That rests on a false premise — **MkDocs publishes every file in `docs_dir` regardless of `nav`**; the nav controls navigation, not publication. Measured live on 2026-08-09, not reasoned: `curl -sI https://platformrelay.github.io/Assent/architecture/policy-profiles/` returns **200**; `sitemap.xml` carries **63 `` entries** against ~10 nav entries; `docs/planning/**` is fully published too; and the page's own words — the recorder-only guarantee stated as an *"architectural invariant, not a runtime best-effort check"* — are in the site's `search/search_index.json`, which indexes 420 sections and returns `architecture/policy-profiles/#write-vs-recorder-only` for that phrase. **So the published false safety guarantee is not hypothetical; it has been live the whole time, and it is searchable.** This is the D-134 shape exactly, and it is P1 by this question's own criterion. **`GUIDELINES.md`'s "docs published on the future site = product docs under `docs/` only; `docs/planning/`, `openspec/`, and agent-context stay out of the mkdocs nav" is read as a publication boundary; it creates only a NAV boundary, and nothing enforces the intended one** — a second, separate gap worth closing (an `exclude_docs`/`not_in_nav` setting, or moving non-product pages out of `docs_dir`). **Ruling needed, deliberately not taken here:** (a) implement the gate — load the covering profile on the `run` path and refuse `Reconcile` when `WriteAllowed` is false, making the documented invariant real; (b) retract the invariant language, restate `spec.writes` as comparison-scope metadata only, and say so in the schema description; or (c) accept the gap explicitly and annotate the doc, as ADR-0009 was annotated. **Not to be resolved by silently changing the frozen schema or the lint rule** — `writes` is a frozen contract field and the lint rule is load-bearing for the compare path. | **P1 — both stated conditions are already met**: the page is published (200) and indexed, and v0.1.0 already shipped the recorder-only guarantee. Needs a ruling before v0.2.1 | Found during the D-134/D-135 docs-truth lane (review finding SURF-08). Cross-referenced from [D-135](../decisions/decisions.md). Evidence: `internal/core/aggregate/profile.go:95-101`, `internal/lint/posture.go:200-215`, `cmd/assent/run.go:533`, `schemas/policy/v1alpha1/profile.schema.json:25-28` | -| OQ-30 | **Is a `pull_request`-scoped CHANGELOG drift gate viable now that D-136 skips merge commits? The guard is retained with NO demonstrated reason — its original one is dead and its proposed successor measures false.** D-125 skipped the gate on `pull_request` because `refs/pull/N/merge`'s synthetic merge subject rendered into the generated changelog, so no committed `CHANGELOG.md` could match. **D-136 killed that reason** — that commit is a merge commit and is now skipped. The successor reason drafted in D-136's first version — "the merge ref also carries every commit landed on `main` since the branch forked, so the render is a union the branch's file cannot match, red by construction" — was then measured four ways and **could not be made true**: (1) PR #41's **live** `refs/pull/41/merge` (`491bb2a`, head `49eebb3` into base `7513d79`) rendered with the new `cliff.toml` → `verify-changelog: ok`, **0 diff lines**; (2) the direct counterexample — the same head merged into a `main` that **had** moved (`1d8aa60`, containing PR #40) → `verify-changelog: ok`, **0 diff lines**, i.e. not red with the base moved; (3) a synthetic sandbox where base and lane each add a commit to the **same** cliff group and each regenerate → `CONFLICT (content): Merge conflict in CHANGELOG.md`, so the PR is unmergeable, GitHub mints no merge ref, and the gate never runs. (4) **The strongest one, taken last and re-run rather than transcribed:** GitHub RE-MINTED `refs/pull/41/merge` against the moved base after all of the above. Re-fetched live — `7715bf7`, head `ee5e527` into base `1d8aa60` — and put through the real gate script: **`verify-changelog: ok`, 0 diff lines, 0 merge subjects rendered.** That is not a simulation: it is the exact artifact a `pull_request`-scoped gate would evaluate, with the base moved past the fork point AND after the lane had merged `main` in — the direction the finding below shows is hazardous — and it is green. Measurement (1)'s `491bb2a` at base `7513d79` is its stale predecessor, kept only to show the result did not depend on the base standing still. **Mechanism the dead premise overlooked:** the merge ref's `CHANGELOG.md` is not "the branch's committed file" — it is the three-way MERGE RESULT, which already contains the base's lines, because the file is merged like any other. So base movement ends in clean-and-matching or conflict-and-no-merge-ref. **The third outcome EXISTS, and merge DIRECTION decides it — measured while writing this row.** A clean textual auto-merge whose line order differs from git-cliff's topological order is red with no author error, and it reproduced immediately: merging `origin/main` **into the lane** (lane as first parent) auto-merged `CHANGELOG.md` without conflict and then failed `verify-changelog` on pure ordering — one `docs(compare)` line moved and PR #40's lines landed in a different position. The SAME two commits merged in the **merge-ref direction** (base `1d8aa60` as first parent, measurement (2) above) matched exactly. git-cliff's traversal follows parent order, so first-parent choice changes the render. This does not revive the retired premise — GitHub always mints the merge ref base-first, which is the direction that matched — but it means the clean-and-matching outcome is a property of that direction, measured on two merges, not a proof. It also re-confirms D-125's surviving rule: regenerate after any `git merge origin/main`. **Still untested:** behaviour on `pull_request_target`, on a PR from a fork, and after a force-push that re-mints the merge ref. **Counter-evidence for enabling it:** the only red reproduced on any merge ref was a branch that had not run `task changelog-write` for its own commits — a **true positive the gate exists to catch**, which argues the PR placement may now be correct rather than merely harmless. **Correction, folded in from the PR #41 review because it belongs in the row and not only in a review thread:** that review first read these greens as "the evidence points toward the PR gate being viable", and then took it back as **one measurement short**. The direction finding above supplies a false-positive mechanism it had not considered — a clean textual auto-merge whose line order differs from git-cliff's topological order reds with no author error and no author fix available. Four green measurements are therefore NOT a green light; on today's evidence the gate would not be enabled. **Ruling needed (deliberately not taken here, operator's call):** (a) enable the step on `pull_request` and delete the guard; (b) keep the guard and record the real reason once someone finds one; or (c) keep the guard permanently on cost/noise grounds and say so, rather than on a mechanism. Not to be resolved by deleting the guard on the strength of these three measurements alone — they show the claimed failure did not reproduce, not that no failure exists. | Before any change to the `pull_request` guard on the changelog step in `.github/workflows/verify.yaml`; not a release blocker — the guard is fail-safe (the gate runs locally in `task check` and on push-to-main) | Raised by the PR #41 review (finding CL-02) against D-136's first draft; measurements reproduced independently before recording. Sites now pointing here: `Taskfile.yml` `check:`, `.github/workflows/verify.yaml`, `hack/release/README.md`, `hack/release/changelog_gate_test.sh` §3. See [D-125](../decisions/decisions.md) and [D-136](../decisions/decisions.md) | -| OQ-31 | **May the GUARD-1 self-edit BLOCK path write a summary or supersession note, or is "zero forge writes on a self-modifying MR" absolute? If it is absolute, what channel carries the BLOCK to the human reviewer — given that no thread is posted and the exit code is `0`?** Raised by RELI-01 ([D-138](../decisions/decisions.md)) and deliberately left **UNDECIDED**. The tension is real in both directions. **For absolute:** `openspec/specs/p5-aud-audit-remediation/spec.md` pins *"the decision is BLOCK with zero forge writes (GUARD-1 dominance over the gap-degrade)"* as a frozen acceptance criterion, and the guard exists so that an MR editing `.assent/**` cannot make assent vouch for its own policy — any write is a write the MR's own content influenced. **Against absolute:** the only human-visible surface then keeps whatever the previous run said, which today can be `✅ Decision: APPROVE`, so the guard's output is *invisible* to the reviewer it protects, and D-130's compensating control (a REVIEW rerun upserts the summary and adds an unresolved discussion) does not reach this path because no thread is posted. Zero *authority* writes need not mean zero *communication*. **Options, none taken here:** (a) keep it absolute and carry BLOCK on a non-forge channel — a non-zero exit code, or a required CI job status; (b) permit exactly one write, a fixed-text supersession/BLOCK note with no policy-derived content, which cannot be steered by the MR; (c) permit the summary upsert but not the thread. (b) and (c) both **reopen the frozen criterion above and need an openspec change proposal first** — spec before code. Note that (a) changes an exit-code contract wrapper scripts rely on ([`docs/usage/cli.md`](../usage/cli.md)), so it is not the free option it looks like. | Before the RELI-01 fix lands (v0.2.1) | Found by the 2026-08-09 audit's reliability lens; recorded in [D-138](../decisions/decisions.md). Evidence: `cmd/assent/run.go` step-9 GUARD switch, `openspec/specs/p5-aud-audit-remediation/spec.md`, `openspec/specs/p5-e5-provider-host/spec.md` REQ-E5-S08-03 | -| OQ-32 | **No provider transport can carry a credential, so NO provider can call Entra ID, Keycloak, or any token-authenticated IdP directly — and nothing says so.** Found while designing P5-DEM (D-142). Verified across three surfaces that agree: `CallHTTP` (`internal/provider/transport.go`) sets **only** `Content-Type: application/json` — no header map, no bearer token, no client certificate; the repo-side provider schema (`schemas/policy/v1alpha1/config.schema.json` `$defs/provider`) is `additionalProperties:false` over exactly `{type, url, failure}`, so there is nowhere to put one; and `ScrubEnv`/`ScrubArgv` build the exec child's environment **from scratch** and refuse any name matching `(?i)(TOKEN\|SECRET)` **even when explicitly configured**, so the exec tier cannot carry one either. **This is not a bug — it is ADR-0015 §7 working exactly as designed**, and Spike C's `TestIsolation` proves it against a deliberately hostile provider that exfiltrates its whole environment and stdin. **What has never been written down is the consequence:** Entra ID and Keycloak both require a bearer token on every call, so the only shape that works today is a **broker** — a service holding the IdP credential *itself*, reachable by assent without one (loopback/sidecar, or mTLS terminated outside assent's transport). That is arguably the *correct* architecture: the credential never enters the decision path and a compromised provider's blast radius stays bounded. But it is undocumented, and it **narrows what `docs/vision.md:67` promises** ("pluggable providers: Keycloak, LDAP, GitLab/GitHub groups, ownership files, custom plugins") and what ADR-0004 §1 planned ("OIDC/Keycloak group lookup, LDAP" as builtins — never shipped). `docs/architecture/c4-context.md:19` is currently the **only** place stating the truth: *"Keycloak / LDAP: no builtin — reachable only via the generic HTTP/exec provider transport."* **Ruling needed (deliberately not taken):** **(a)** bless the broker pattern, document it in the provider-author guide, and amend `docs/vision.md:67` + ADR-0004 §1 to stop implying direct IdP calls — **RECOMMENDED**: costs nothing, keeps ADR-0015 §7 and the isolation proof intact, and is what DEM-S02/DEM-S03 are already written against; **(b)** add a narrow repo-side credential channel (header or secret-ref) to the HTTP transport — **reopens a frozen schema AND the trust boundary the hostile-provider isolation proof rests on**, and would need its own ADR; **(c)** state the limitation and add nothing. Note this is **not** merely a docs question under (a): a reader of the vision page today would reasonably budget a Keycloak integration as "configure a builtin" and discover mid-implementation that they must also deploy and operate a broker. **Not to be resolved by quietly adding a header field** — that is option (b) and it is a trust-boundary change. | Before DEM-S02 publishes the provider-author guide (the guide must state one of these answers); **not** a release blocker — the current behaviour is fail-safe, just undocumented | Found designing [P5-DEM](https://github.com/PlatformRelay/assent/blob/main/openspec/specs/p5-dem-demo-repos/spec.md) (judgment call (e)); recorded in [D-142](../decisions/decisions.md). Evidence: `internal/provider/transport.go` `CallHTTP`/`ScrubEnv`/`ScrubArgv`, `schemas/policy/v1alpha1/config.schema.json` `$defs/provider`, `docs/planning/spikes/spike-c-provider.md` § Isolation evidence, ADR-0015 §7, ADR-0004 §1, `docs/vision.md:67`, `docs/architecture/c4-context.md:19` | +| OQ-29 | **RESOLVED (D-145, 2026-08-16): (a) — implement the gate.** `writes: false` becomes runtime-enforced on the `run` path; a same-day doc annotation covers the gap until the code lane lands. Analysis retained below for the record. Original text: **`PolicyProfile.spec.writes: false` is a frozen-schema field with NO runtime enforcement, and lint compels adopters to author it.** [`docs/architecture/policy-profiles.md`](../architecture/policy-profiles.md) states the recorder-only guarantee as an **"architectural invariant, not a runtime best-effort check"** — line 13: a `writes: false` profile **"Never calls `Reconcile` — no approve, merge, block, thread sync, or other forge write"**. No code enforces it, because nothing on the write path reads it. Verified by grep over non-test sources: `aggregate.ResolveProfile` and `Result.WriteAllowed` have consumers only in `internal/lint/posture.go` and inside `internal/core/aggregate` itself; `aggregate.CoverWithProfile` is called only from `internal/compare`; `policy.LoadProfile` is called only from `cmd/assent/compare.go`; and **`cmd/assent/run.go` contains the string `Profile` zero times** — it evaluates via `aggregate.CoverWithPhaseCeiling` (`run.go:533`) and reaches `buildDesired`/`forge.Reconcile` without ever loading or consulting a profile. `internal/core/aggregate/profile.go:97` documents the missing link in its own words: *"A downstream forge step reads `WriteAllowed` to know whether this run may arm/merge or is recorder-only"* — **there is no such downstream forge step.** So a `writes: false` profile does not make `assent run` recorder-only; the run behaves exactly as if no profile existed. **Why it is not merely internal:** `writes` is a REQUIRED field of the frozen `schemas/policy/v1alpha1/profile.schema.json`, whose description reads *"true = this profile authorizes forge writes for bindings in its scope; false = recorder-only"*, and the `single-writer-profile` lint hard error (`internal/lint/posture.go:83`) fails a tree where zero or more than one `writes: true` profile covers a binding — so adopters are **compelled** to author a field whose `false` value does not do what the schema says. **RAISED TO P1 on 2026-08-09 — the stated escalation condition was ALREADY TRUE when it was written (audit DOC-04).** The original text read: *"Severity today is P2 only because `docs/architecture/policy-profiles.md` is NOT in the mkdocs nav, so the invariant claim is not on the docs site. If that directory ever enters the nav it becomes P1."* That rests on a false premise — **MkDocs publishes every file in `docs_dir` regardless of `nav`**; the nav controls navigation, not publication. Measured live on 2026-08-09, not reasoned: `curl -sI https://platformrelay.github.io/Assent/architecture/policy-profiles/` returns **200**; `sitemap.xml` carries **63 `` entries** against ~10 nav entries; `docs/planning/**` is fully published too; and the page's own words — the recorder-only guarantee stated as an *"architectural invariant, not a runtime best-effort check"* — are in the site's `search/search_index.json`, which indexes 420 sections and returns `architecture/policy-profiles/#write-vs-recorder-only` for that phrase. **So the published false safety guarantee is not hypothetical; it has been live the whole time, and it is searchable.** This is the D-134 shape exactly, and it is P1 by this question's own criterion. **`GUIDELINES.md`'s "docs published on the future site = product docs under `docs/` only; `docs/planning/`, `openspec/`, and agent-context stay out of the mkdocs nav" is read as a publication boundary; it creates only a NAV boundary, and nothing enforces the intended one** — a second, separate gap worth closing (an `exclude_docs`/`not_in_nav` setting, or moving non-product pages out of `docs_dir`). **Ruling needed, deliberately not taken here:** (a) implement the gate — load the covering profile on the `run` path and refuse `Reconcile` when `WriteAllowed` is false, making the documented invariant real; (b) retract the invariant language, restate `spec.writes` as comparison-scope metadata only, and say so in the schema description; or (c) accept the gap explicitly and annotate the doc, as ADR-0009 was annotated. **Not to be resolved by silently changing the frozen schema or the lint rule** — `writes` is a frozen contract field and the lint rule is load-bearing for the compare path. | **P1 — both stated conditions are already met**: the page is published (200) and indexed, and v0.1.0 already shipped the recorder-only guarantee. Needs a ruling before v0.2.1 | Found during the D-134/D-135 docs-truth lane (review finding SURF-08). Cross-referenced from [D-135](../decisions/decisions.md). Evidence: `internal/core/aggregate/profile.go:95-101`, `internal/lint/posture.go:200-215`, `cmd/assent/run.go:533`, `schemas/policy/v1alpha1/profile.schema.json:25-28` | +| OQ-30 | **RESOLVED (D-148, 2026-08-16): (b) — keep the guard skipped on `pull_request`; the real mechanism (merge-direction-dependent ordering hazard) is now recorded in D-125/D-136.** Analysis retained below for the record. Original text: Is a `pull_request`-scoped CHANGELOG drift gate viable now that D-136 skips merge commits? The guard is retained with NO demonstrated reason — its original one is dead and its proposed successor measures false.** D-125 skipped the gate on `pull_request` because `refs/pull/N/merge`'s synthetic merge subject rendered into the generated changelog, so no committed `CHANGELOG.md` could match. **D-136 killed that reason** — that commit is a merge commit and is now skipped. The successor reason drafted in D-136's first version — "the merge ref also carries every commit landed on `main` since the branch forked, so the render is a union the branch's file cannot match, red by construction" — was then measured four ways and **could not be made true**: (1) PR #41's **live** `refs/pull/41/merge` (`491bb2a`, head `49eebb3` into base `7513d79`) rendered with the new `cliff.toml` → `verify-changelog: ok`, **0 diff lines**; (2) the direct counterexample — the same head merged into a `main` that **had** moved (`1d8aa60`, containing PR #40) → `verify-changelog: ok`, **0 diff lines**, i.e. not red with the base moved; (3) a synthetic sandbox where base and lane each add a commit to the **same** cliff group and each regenerate → `CONFLICT (content): Merge conflict in CHANGELOG.md`, so the PR is unmergeable, GitHub mints no merge ref, and the gate never runs. (4) **The strongest one, taken last and re-run rather than transcribed:** GitHub RE-MINTED `refs/pull/41/merge` against the moved base after all of the above. Re-fetched live — `7715bf7`, head `ee5e527` into base `1d8aa60` — and put through the real gate script: **`verify-changelog: ok`, 0 diff lines, 0 merge subjects rendered.** That is not a simulation: it is the exact artifact a `pull_request`-scoped gate would evaluate, with the base moved past the fork point AND after the lane had merged `main` in — the direction the finding below shows is hazardous — and it is green. Measurement (1)'s `491bb2a` at base `7513d79` is its stale predecessor, kept only to show the result did not depend on the base standing still. **Mechanism the dead premise overlooked:** the merge ref's `CHANGELOG.md` is not "the branch's committed file" — it is the three-way MERGE RESULT, which already contains the base's lines, because the file is merged like any other. So base movement ends in clean-and-matching or conflict-and-no-merge-ref. **The third outcome EXISTS, and merge DIRECTION decides it — measured while writing this row.** A clean textual auto-merge whose line order differs from git-cliff's topological order is red with no author error, and it reproduced immediately: merging `origin/main` **into the lane** (lane as first parent) auto-merged `CHANGELOG.md` without conflict and then failed `verify-changelog` on pure ordering — one `docs(compare)` line moved and PR #40's lines landed in a different position. The SAME two commits merged in the **merge-ref direction** (base `1d8aa60` as first parent, measurement (2) above) matched exactly. git-cliff's traversal follows parent order, so first-parent choice changes the render. This does not revive the retired premise — GitHub always mints the merge ref base-first, which is the direction that matched — but it means the clean-and-matching outcome is a property of that direction, measured on two merges, not a proof. It also re-confirms D-125's surviving rule: regenerate after any `git merge origin/main`. **Still untested:** behaviour on `pull_request_target`, on a PR from a fork, and after a force-push that re-mints the merge ref. **Counter-evidence for enabling it:** the only red reproduced on any merge ref was a branch that had not run `task changelog-write` for its own commits — a **true positive the gate exists to catch**, which argues the PR placement may now be correct rather than merely harmless. **Correction, folded in from the PR #41 review because it belongs in the row and not only in a review thread:** that review first read these greens as "the evidence points toward the PR gate being viable", and then took it back as **one measurement short**. The direction finding above supplies a false-positive mechanism it had not considered — a clean textual auto-merge whose line order differs from git-cliff's topological order reds with no author error and no author fix available. Four green measurements are therefore NOT a green light; on today's evidence the gate would not be enabled. **Ruling needed (deliberately not taken here, operator's call):** (a) enable the step on `pull_request` and delete the guard; (b) keep the guard and record the real reason once someone finds one; or (c) keep the guard permanently on cost/noise grounds and say so, rather than on a mechanism. Not to be resolved by deleting the guard on the strength of these three measurements alone — they show the claimed failure did not reproduce, not that no failure exists. | Before any change to the `pull_request` guard on the changelog step in `.github/workflows/verify.yaml`; not a release blocker — the guard is fail-safe (the gate runs locally in `task check` and on push-to-main) | Raised by the PR #41 review (finding CL-02) against D-136's first draft; measurements reproduced independently before recording. Sites now pointing here: `Taskfile.yml` `check:`, `.github/workflows/verify.yaml`, `hack/release/README.md`, `hack/release/changelog_gate_test.sh` §3. See [D-125](../decisions/decisions.md) and [D-136](../decisions/decisions.md) | +| OQ-31 | **RESOLVED (D-146, 2026-08-16): (a) — "zero forge writes" stays absolute; the BLOCK is surfaced via a required CI job status reading the already-emitted `DecisionRecord`, not via a forge write.** Analysis retained below for the record. Original text: May the GUARD-1 self-edit BLOCK path write a summary or supersession note, or is "zero forge writes on a self-modifying MR" absolute? If it is absolute, what channel carries the BLOCK to the human reviewer — given that no thread is posted and the exit code is `0`?** Raised by RELI-01 ([D-138](../decisions/decisions.md)) and deliberately left **UNDECIDED**. The tension is real in both directions. **For absolute:** `openspec/specs/p5-aud-audit-remediation/spec.md` pins *"the decision is BLOCK with zero forge writes (GUARD-1 dominance over the gap-degrade)"* as a frozen acceptance criterion, and the guard exists so that an MR editing `.assent/**` cannot make assent vouch for its own policy — any write is a write the MR's own content influenced. **Against absolute:** the only human-visible surface then keeps whatever the previous run said, which today can be `✅ Decision: APPROVE`, so the guard's output is *invisible* to the reviewer it protects, and D-130's compensating control (a REVIEW rerun upserts the summary and adds an unresolved discussion) does not reach this path because no thread is posted. Zero *authority* writes need not mean zero *communication*. **Options, none taken here:** (a) keep it absolute and carry BLOCK on a non-forge channel — a non-zero exit code, or a required CI job status; (b) permit exactly one write, a fixed-text supersession/BLOCK note with no policy-derived content, which cannot be steered by the MR; (c) permit the summary upsert but not the thread. (b) and (c) both **reopen the frozen criterion above and need an openspec change proposal first** — spec before code. Note that (a) changes an exit-code contract wrapper scripts rely on ([`docs/usage/cli.md`](../usage/cli.md)), so it is not the free option it looks like. | Before the RELI-01 fix lands (v0.2.1) | Found by the 2026-08-09 audit's reliability lens; recorded in [D-138](../decisions/decisions.md). Evidence: `cmd/assent/run.go` step-9 GUARD switch, `openspec/specs/p5-aud-audit-remediation/spec.md`, `openspec/specs/p5-e5-provider-host/spec.md` REQ-E5-S08-03 | +| OQ-32 | **RESOLVED (D-147, 2026-08-16): (b) — add a host-side secret resolver (process env / file path / hosted store); repo-side config gains only an opaque, host-allowlisted reference name, never a literal credential or URL pairing. ADR amending ADR-0015 §7 required before code.** Analysis retained below for the record. Original text: No provider transport can carry a credential, so NO provider can call Entra ID, Keycloak, or any token-authenticated IdP directly — and nothing says so. Found while designing P5-DEM (D-142). Verified across three surfaces that agree: `CallHTTP` (`internal/provider/transport.go`) sets **only** `Content-Type: application/json` — no header map, no bearer token, no client certificate; the repo-side provider schema (`schemas/policy/v1alpha1/config.schema.json` `$defs/provider`) is `additionalProperties:false` over exactly `{type, url, failure}`, so there is nowhere to put one; and `ScrubEnv`/`ScrubArgv` build the exec child's environment **from scratch** and refuse any name matching `(?i)(TOKEN\|SECRET)` **even when explicitly configured**, so the exec tier cannot carry one either. **This is not a bug — it is ADR-0015 §7 working exactly as designed**, and Spike C's `TestIsolation` proves it against a deliberately hostile provider that exfiltrates its whole environment and stdin. **What has never been written down is the consequence:** Entra ID and Keycloak both require a bearer token on every call, so the only shape that works today is a **broker** — a service holding the IdP credential *itself*, reachable by assent without one (loopback/sidecar, or mTLS terminated outside assent's transport). That is arguably the *correct* architecture: the credential never enters the decision path and a compromised provider's blast radius stays bounded. But it is undocumented, and it **narrows what `docs/vision.md:67` promises** ("pluggable providers: Keycloak, LDAP, GitLab/GitHub groups, ownership files, custom plugins") and what ADR-0004 §1 planned ("OIDC/Keycloak group lookup, LDAP" as builtins — never shipped). `docs/architecture/c4-context.md:19` is currently the **only** place stating the truth: *"Keycloak / LDAP: no builtin — reachable only via the generic HTTP/exec provider transport."* **Ruling needed (deliberately not taken):** **(a)** bless the broker pattern, document it in the provider-author guide, and amend `docs/vision.md:67` + ADR-0004 §1 to stop implying direct IdP calls — **RECOMMENDED**: costs nothing, keeps ADR-0015 §7 and the isolation proof intact, and is what DEM-S02/DEM-S03 are already written against; **(b)** add a narrow repo-side credential channel (header or secret-ref) to the HTTP transport — **reopens a frozen schema AND the trust boundary the hostile-provider isolation proof rests on**, and would need its own ADR; **(c)** state the limitation and add nothing. Note this is **not** merely a docs question under (a): a reader of the vision page today would reasonably budget a Keycloak integration as "configure a builtin" and discover mid-implementation that they must also deploy and operate a broker. **Not to be resolved by quietly adding a header field** — that is option (b) and it is a trust-boundary change. | Before DEM-S02 publishes the provider-author guide (the guide must state one of these answers); **not** a release blocker — the current behaviour is fail-safe, just undocumented | Found designing [P5-DEM](https://github.com/PlatformRelay/assent/blob/main/openspec/specs/p5-dem-demo-repos/spec.md) (judgment call (e)); recorded in [D-142](../decisions/decisions.md). Evidence: `internal/provider/transport.go` `CallHTTP`/`ScrubEnv`/`ScrubArgv`, `schemas/policy/v1alpha1/config.schema.json` `$defs/provider`, `docs/planning/spikes/spike-c-provider.md` § Isolation evidence, ADR-0015 §7, ADR-0004 §1, `docs/vision.md:67`, `docs/architecture/c4-context.md:19` | From 7c851e840a06f2105d52b32c0bc65a64096515e3 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 16 Aug 2026 10:57:50 +0200 Subject: [PATCH 2/5] :memo: docs(truth): annotate unenforced writes:false and retract built-in IdP claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit policy-profiles.md now states plainly that the recorder-only invariant holds for `assent compare` but not yet for `assent run` (D-145 closes the gap). vision.md and ADR-0004 stop implying Keycloak/LDAP ship as built-in providers — they need a broker or (planned, D-147) a host-side credential resolver, since no provider transport carries a credential in-process by design (ADR-0015 §7). --- docs/adr/0004-plugin-architecture.md | 38 ++++++++++++++++++---------- docs/architecture/policy-profiles.md | 8 ++++++ docs/vision.md | 2 +- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/docs/adr/0004-plugin-architecture.md b/docs/adr/0004-plugin-architecture.md index ddea22a..03fb44a 100644 --- a/docs/adr/0004-plugin-architecture.md +++ b/docs/adr/0004-plugin-architecture.md @@ -29,25 +29,37 @@ runtime-loaded data (ADR-0002); this ADR is about *imperative* extension points. **Tiered provider model behind one `FactProvider` / `PermissionProvider` port:** -1. **Built-ins** (in-tree, config-activated): forge group membership (GitLab/GitHub), OIDC/ - Keycloak group lookup, LDAP, ownership-file (CODEOWNERS-style) — covering the common cases - with zero plugin code. +1. **Built-ins** (in-tree, config-activated): forge group membership (GitLab/GitHub), + ownership-file (CODEOWNERS-style) — covering the common cases with zero plugin code. + **OIDC/Keycloak/LDAP group lookup did not ship as built-ins and are not planned as + such** (OQ-32/D-147, decided while designing P5-DEM): every token-authenticated IdP + needs a bearer credential on each call, and no provider transport carries one + in-process by design ([ADR-0015 §7](0015-trust-boundaries-merge-integrity.md), + Amendment 2 below) — a credential entering the built-in tier would put it inside + the decision path's trust boundary for a capability tier 2 already covers. + The supported shape is tier 2 (HTTP/exec) behind either a credential-holding broker + documented for adopters, or (planned, D-147) a host-side secret resolver that keeps + the credential out of repo-side, MR-editable config. `docs/architecture/c4-context.md` + states this plainly; treat this row as superseding the built-in claim below. 2. **HTTP / exec provider**: declare an endpoint or executable in config; assent calls it - with a versioned JSON request and expects a versioned JSON response. Any language, no SDK. -3. **gRPC plugins** (`hashicorp/go-plugin`): for providers needing streaming, caching hooks, - or richer lifecycle; subprocess model matches the one-shot CI execution well. -4. **WASM (wazero)** — reserved future tier for sandboxed, hot-loadable providers; recorded as - reversible option, not built in v1. - -All tiers implement the same request/response contract; the contract (not the transport) is -the versioned public API. + with a versioned JSON request and expects a versioned JSON response — any language, no SDK, + the tier a third-party adopter is expected to land on first. +3. **gRPC plugins** (`hashicorp/go-plugin`): for providers needing streaming, caching hooks, or + richer lifecycle than one-shot request/response gives; the subprocess model matches the + one-shot CI execution that tier 2 already assumes. +4. **WASM (wazero)** — reserved future tier for sandboxed, hot-loadable providers; recorded as a + reversible option from day one, not built in v1. + +All four tiers implement the same request/response contract; the contract (not the transport) +is the versioned public API, so moving a provider from tier 2 to tier 3 changes nothing a +policy author can see. ## Consequences - Provider results become **facts** in PolicyInput — policies never call providers directly, keeping evaluation pure/deterministic and trivially testable (facts are fixtures in tests). -- Caching, timeouts, and failure semantics (fail-open vs. fail-closed per provider) must be - spec'd; default is **fail-closed → human review**. +- Caching, timeouts, and failure semantics (fail-open vs. fail-closed per provider) are pinned + down in the amendments below; default is **fail-closed → human review**. ## Counterpoints considered diff --git a/docs/architecture/policy-profiles.md b/docs/architecture/policy-profiles.md index 252c07a..3044a0b 100644 --- a/docs/architecture/policy-profiles.md +++ b/docs/architecture/policy-profiles.md @@ -19,6 +19,14 @@ Side-effect-free comparison (`assent compare`, shipped in E6) evaluates recorder profiles over the same ChangeSet solely to produce DecisionRecords for delta classification. +> **Enforcement status (2026-08-16, tracked by D-145 / OQ-29):** the invariant above +> holds for `assent compare`, which is the only caller of `aggregate.CoverWithProfile`. +> **It is not yet enforced on the `assent run` path** — `cmd/assent/run.go` does not +> load a profile at all, so a `writes: false` profile does not make a live `run` +> recorder-only today; the run proceeds exactly as if no profile existed for it. +> Landing that gate is a dedicated, independently-reviewed lane (D-145); this note +> is removed once it merges. + ## Contracts - Profile schema: `schemas/policy/v1alpha1/profile.schema.json` (`spec.writes` required) diff --git a/docs/vision.md b/docs/vision.md index 6d5f3fe..1ca2dca 100644 --- a/docs/vision.md +++ b/docs/vision.md @@ -64,7 +64,7 @@ just report artifacts; `assent compare` covers the corpus-replay case today), an | --- | --- | --- | | Change understanding | regex on diff lines | canonical field-level change model for JSON / YAML / HCL-tfvars | | Rule language | imperative script | declarative YAML + CEL assertions, versioned in the governed repo (Rego backend *planned* — E11) | -| Permission checks | hard-coded HTTP calls | pluggable providers: Keycloak, LDAP, GitLab/GitHub groups, ownership files, custom plugins | +| Permission checks | hard-coded HTTP calls | pluggable providers: GitLab/GitHub groups, ownership files, generic HTTP/exec, custom plugins — Keycloak/Entra/LDAP via a credential broker or (planned, OQ-32/D-147) a host-side secret resolver, never a direct in-transport credential today | | Review UX | pipeline pass/fail | resolvable review threads, comments, approve/deny, auto-merge | | Testing | none | fixture-based policy tests, required by lint | | Platform | one forge | one forge-neutral port — GitLab adapter shipped, GitHub adapter *planned* (E10) | From 7de0085e531c7dd6be0fab594fce6764fe5fe840 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 16 Aug 2026 10:57:56 +0200 Subject: [PATCH 3/5] =?UTF-8?q?:memo:=20docs(adr):=20add=20ADR-0011=20Amen?= =?UTF-8?q?dment=204=20=E2=80=94=20Rego/OPA=20rule-7=20capability=20bounda?= =?UTF-8?q?ry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records D-144's resolution of D-141's blocking sub-question: the Rego evaluator lives in internal/core/policy, and Amendment 3's net/** link-deny narrows for that package alone to capability-enforced (compile-time sandbox + golden allowlist) rather than link-enforced, closing the gap where neither existing purity gate is transitive. Folds Amendment 3's enforcement mechanism into the Invariants section and fixes one stale cross-reference (ADR-0007 amendment -> amendment 1). --- docs/adr/0011-core-ports-and-contracts.md | 80 ++++++++++++++++++++++- 1 file changed, 78 insertions(+), 2 deletions(-) diff --git a/docs/adr/0011-core-ports-and-contracts.md b/docs/adr/0011-core-ports-and-contracts.md index 21d9d3e..e50f9e9 100644 --- a/docs/adr/0011-core-ports-and-contracts.md +++ b/docs/adr/0011-core-ports-and-contracts.md @@ -104,7 +104,14 @@ type Publisher interface { ## Invariants -- `internal/core` + `internal/change` import no port implementations (arch-lint enforced). +- `internal/core` + `internal/change` import no port implementations, and neither does the + rest of the pure tree this boundary is designed to cover as it grows (`internal/glob`, + `internal/lint`, `internal/catalogue`, and later `internal/evaldecode`/`internal/compare` + as those packages join the decision path — Amendment 3). The enforcement mechanism is two + machine-checked gates, not review: golangci-lint `depguard` deny-rules (package-level + imports) and an AST purity walk (`TestCorePurity`, call-level: no `time.Now`, + `os.Getenv`/`os.Environ`, `math/rand`). Amendment 3 records the package list and the + extension of both gates to their final scope. - `Predicate.Eval` is pure: facts pre-resolved, branch state pre-parsed (lazy but memoized), no clock, no randomness. This is what makes golden tests and replay trivial. - Every contract change goes through an openspec change proposal; serialized forms carry @@ -132,7 +139,7 @@ type Publisher interface { - **Per-change predicate binding (F11):** a rule's predicate is evaluated **once per matched change**, with `old/new/path/kind/file/entry/oldEntry` bound to that change (scope table in the ADR-0013 appendix). A `vouch` covers exactly the changes whose predicate returned true; - false or error leaves that change uncovered (tri-state per ADR-0007 amendment). `entry` / + false or error leaves that change uncovered (tri-state per ADR-0007 amendment 1). `entry` / `oldEntry` (containing entry at head/base) are added to the PolicyInput contract. ## Amendment 2 (2026-07-21, second review P1-4/P1-5/P2-11) @@ -171,3 +178,72 @@ deliberately: both sit on decision paths (engine input decode; D-116/D-117 compa gates) and inherit the hard rule that nothing probabilistic, wall-clock- or randomness-dependent may live there. "arch-lint enforced" elsewhere in this ADR should be read as "depguard + purity-walk enforced" per this amendment. + +## Amendment 4 (2026-08-16, D-144 — Rego/OPA capability boundary for `internal/core/policy`) + +E11 (ADR-0002 v2's Rego/OPA complex-rule backend) adopts `github.com/open-policy-agent/opa/rego` +inside `internal/core/policy`. OPA ships an `http.send` Rego builtin (plus its own clock and +randomness use via `time.now_ns`/`rand.intn`), so the package's *dependency closure* reaches +`net/http` even though the file itself imports nothing but `opa/rego`. Neither of Amendment 3's +enforcement mechanisms catches this: `.golangci.yml`'s `pure-tree` depguard denies `net/**` only +over each file's own direct imports, and `internal/core/purity_test.go`'s AST walk flags only +call-sites the file itself writes. A file importing `opa/rego` passes both gates green while +quietly linking the network stack transitively (verified during E11 design, D-141). This +amendment records the operator's resolution of that gap (D-144). + +- **What narrows, precisely.** AGENTS.md hard rule 7's own text — no LLM calls, no wall-clock or + randomness dependence — is unchanged and still call-level-enforced everywhere in the guarded + tree, `internal/core/policy` included: first-party code in that package still may not call + `time.Now`, `os.Getenv`, or `math/rand`, and `TestCorePurity` keeps checking it. What narrows is + **Amendment 3's separate `net/**` link-deny**, and only for `internal/core/policy`'s dependency + closure: OPA's own use of the clock, randomness, and `http.send` is no longer *absent from the + package's link graph* (structural, greppable) — it is present but made *uncallable from policy* + (behavioural, resting on a capability configuration). Read Amendment 3's `net/**` invariant, for + this one package only, as capability-enforced rather than link-enforced; it stays link-enforced, + unamended, everywhere else in the guarded tree listed there. +- **Why this package, not an injected boundary.** The evaluator lives in `internal/core/policy`, + not behind a port interface implemented in `cmd/assent` (rejected option (d2)). Moving it out + would keep the tree formally OPA-free, but it would place live decision-path evaluation entirely + outside every rule-7 guard — a weaker guarantee dressed as a stronger one. (d1) keeps the + evaluator inside the guarded, tested, reviewed tree and states the narrower guarantee honestly + instead of hiding it behind a boundary that looks stricter and enforces nothing. ((d3), dropping + OPA outright, was rejected as strictly worse than either.) +- **What compensates, together:** + 1. **Capability sandbox** (E11-S04, REQ-E11-S04-01): the OPA runtime is configured with a + deny-by-default capability set — `http.send`, `net.*`, `opa.runtime`, `time.*`, `rand.*`, and + any other I/O builtin are absent from what a compiled module may call; a module referencing + one fails to **compile**, not at runtime. + 2. **Golden allowlist** (E11-S04, REQ-E11-S04-02): the effective allowed-builtin set is pinned + against a committed golden file, so an OPA upgrade that introduces new builtins cannot widen + what policy can call without a deliberate, reviewed diff. + 3. **Transitive purity check** (E11-S04, REQ-E11-S04-03): `internal/core/purity_test.go` and + `.golangci.yml` gain a `go list -deps`-based assertion over the guarded tree's transitive + closure — not just direct imports — allowlisting exactly the OPA import path and failing on + any other dependency that reaches `net`/`net/http`. This closes the non-transitivity gap + itself, independent of Rego, and is a strict improvement over today's gates regardless of how + (d) had resolved. **Sequencing note:** because E11-S03 is the story that adds OPA to + `go.mod`, and both purity gates are non-transitive today, S03 would land green under the old + gates even though it is the story that effects this narrowing — the transitive check is part + of S04's guard work but must be in place *before* S03's dependency lands, not after, or S03 + merges the very gap this amendment closes. + - This amendment is itself the deliverable REQ-E11-S04-04 requires (an ADR amendment plus a + `D-nnn` row landing before E11-S05); it fulfils that requirement rather than merely describing + it. Item 3 above is REQ-E11-S04-03, not REQ-E11-S04-04 — the D-144 decision-log row cites + REQ-E11-S04-04 for the transitive check, which is loose; the spec text names REQ-E11-S04-03 + for that work. + - Both the sandbox and the transitive purity check are engine-grade, security-relevant changes + to the decision-path boundary and land under maintainer LGTM, per E11-S04's own tag and + GOVERNANCE. +- **What this does not authorize.** This amendment is scoped to the OPA/Rego evaluator inside + `internal/core/policy`. It does not relax rule 7's own text anywhere. It does not relax + Amendment 3's structural, link-enforced `net/**` guarantee for any other package in the guarded + tree (`internal/core/**` elsewhere, `internal/change/**`, `internal/glob`, `internal/lint`, + `internal/catalogue`, `internal/evaldecode`, `internal/compare`, `schemas/**`) — a second + transitively-networked dependency anywhere in that tree is still a purity-gate failure, not a + precedent this amendment sets. It does not authorize a wall-clock evaluation timeout as a + substitute safeguard; the machine-independent evaluation budget that bounds Rego execution is a + separate requirement (E11-S06) and is not re-litigated here. + +See D-144 (`docs/decisions/decisions.md`) for the full evidentiary trail — including the (d1) +vs (d2) vs (d3) tradeoff in full and the supply-chain question it does not settle — and D-141 for +the judgment call this amendment resolves. From d6b24785fc4e31e803dc0b1984b3b4910feecb23 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 16 Aug 2026 10:58:03 +0200 Subject: [PATCH 4/5] :memo: docs(adr): add ADR-0015 host-side credential resolver amendment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records D-147's resolution of OQ-32: providers gain a credential path for token-authenticated IdPs (Entra ID, Keycloak) without reopening the hostile-provider isolation proof. Repo-side config gains exactly one new opaque credentialRef field; the host resolves it via env, file, or a pluggable backend, binds it to a fixed origin, and injects the auth header — an MR can name a credential but never choose or redirect one. Exec-tier ScrubEnv/ScrubArgv stripping is explicitly unchanged. --- .../0015-trust-boundaries-merge-integrity.md | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/docs/adr/0015-trust-boundaries-merge-integrity.md b/docs/adr/0015-trust-boundaries-merge-integrity.md index d55f2f6..771a02b 100644 --- a/docs/adr/0015-trust-boundaries-merge-integrity.md +++ b/docs/adr/0015-trust-boundaries-merge-integrity.md @@ -129,3 +129,105 @@ deadlines, CEL cost budget — breach fails closed to REVIEW (spec'd with ADR-00 summary comment always embeds the decision hash and a link to the report artifact; docs must warn that CI artifact retention limits the audit window and recommend a retention policy for report artifacts. + +## Amendment (2026-08-16, D-147 — host-side credential resolver for HTTP providers) + +### 10. Host-side credential resolver (D-147 / OQ-32) + +**Context.** §7 established that no provider transport carries a credential — `CallHTTP` +sets only `Content-Type: application/json`, and the repo-side provider schema +(`$defs/provider`) is closed over exactly `{type, url, failure}`. That was deliberate: a +hostile-provider isolation proof (`TestIsolation`, Spike C) rests on it. But it forecloses +calling a token-authenticated IdP (Entra ID, Keycloak) directly — every such provider had to +go through a broker. D-147 (resolving OQ-32) rules that a direct credential channel should +exist after all, provided it stays **host-side only**: the credential is configured on the +assent host (env / CLI / host config), keyed by a provider name, and the host — never the +MR — injects the auth header. The operator's stated adoption patterns (GitLab CI / GitHub +Actions job variables as the common case; Vault-agent file injection; a hosted variant with +its own backend) all shape the source list below. + +**Decision.** + +- Repo-side `$defs/provider` gains exactly **one** new optional field: `credentialRef` + (string, opaque, non-empty). Nothing else about the schema changes; it stays + `additionalProperties: false`. +- The host holds a **credential allowlist**, keyed by `credentialRef` name, each entry + populated from a host-side source and binding, host-side, all of: source, header name + + scheme (default `Authorization: Bearer `), and the **exact origin + (scheme+host[:port]) the credential may be attached to**. The origin bind is load-bearing, + not decorative: repo-side config still supplies `url`, so without it, `credentialRef` paired + with an attacker-chosen `url` in the same MR-editable tree reproduces exactly the + exfiltration shape a repo-side `secretRef` would have opened. §1 (policy loaded from the + target ref, never the MR branch) and the `assent-policy` block-by-default class are a + second, independent mitigation on the same hole — worth stating explicitly since D-147 + credited them separately, but they gate by ref-trust, not by a runtime check, so they don't + substitute for the origin bind. + - **env**: `ASSENT_PROVIDER_TOKEN_` (normalized ref name) — the GitLab CI / GitHub + Actions "set a job variable" pattern; expected to be the common case. + - **file**: a host-config-declared path, read at resolve time — the Vault-agent-injects-a- + file pattern, and the Kubernetes/CSI secret-mounted-file pattern. + - **pluggable resolver interface** — a small `CredentialSource` contract so a hosted variant + can add a managed-secret-store backend later without a transport-contract change. + - An unknown or unallowlisted `credentialRef` is a **hard error** at provider resolution — + never silently ignored, never falls back to an unauthenticated call. +- **Attachment**: the host resolves `credentialRef` → secret *before* `CallHTTP` builds its + request, and sets the allowlisted header (default `Authorization: Bearer `) + alongside the existing `Content-Type: application/json`. Credentialed calls additionally: (a) + refuse (hard error) to attach to a non-`https` URL, and (b) use a client with redirects + disabled, so a credential is never replayed to a redirect target outside the allowlisted + origin. +- A provider with `credentialRef` set may never be configured `failure: open` — the same lint + hard-error idiom the schema already applies to controlling/authorization providers + (`docs/planning/lint-hard-errors.md`). A resolution failure must fail-safe (fact `unknown`, + ADR-0007 tri-state), never silently degrade to an unauthenticated call. +- **Exec tier: excluded from this design, on purpose.** `ScrubEnv`/`ScrubArgv` keep stripping + every env/argv entry matching `(?i)(TOKEN|SECRET)` — no exception list, no bypass flag, for + exec providers under this amendment. The resolver's own host env-var convention + (`ASSENT_PROVIDER_TOKEN_`) is chosen deliberately to align with, not fight, that regex: + if a resolved value were ever echoed into an exec provider's declared env/argv by + misconfiguration, the existing scrub still catches it as a `TOKEN`-named canary. Exec stays + excluded rather than gaining a dedicated non-scrubbed channel because exec is precisely the + tier where §7's isolation claim rests on the scrub — a spawned child, with an environment and + argv visible in the host process table — and a bypass there would invert that claim for the + one tier that needs it most. +- **Never persisted**: the resolved secret is never written to `Decision.Pins`, the evaluation + report, the summary comment, or debug/explain output — consistent with ADR-0004 Amendment + 2's "providers must never return raw credentials as facts." Resolver errors name the + `credentialRef`, never the resolved value. +- **Purity boundary**: resolution and attachment live in `internal/provider` (host-facing, + I/O-permitted), outside the arch-lint-enforced pure set (`internal/core`, `internal/change`, + etc. — ADR-0011 Amendment 3). This does not reopen AGENTS.md's determinism rule for the + decision path: resolution is a deterministic function of host state, and happens before + evaluation, not inside it. + +**Explicitly NOT authorized by this amendment:** + +- A repo-side literal secret, or a repo-side reference that also carries or selects a + destination. `credentialRef` is an opaque name only; the host — never the MR — decides what + it resolves to and where it may be sent. +- A repo-side field that can choose or override header name, scheme, or target origin. Those + stay host-config-only, inside the allowlist entry. +- Any exec-tier exception to `ScrubEnv`/`ScrubArgv`. + +**Consequences.** + +- Schema (`$defs/provider`): one new optional string property, `credentialRef`. Still + `additionalProperties: false`; still no way for one MR-editable document to name a URL and a + real credential together. +- `transport.go`: `CallHTTP` gains a resolver call ahead of request-header construction, an + https-only guard, and a redirect-disabled client for credentialed requests. `ScrubEnv`/ + `ScrubArgv` are otherwise unchanged — this amendment restates their exec-tier behavior, it + does not loosen it. +- Host config gains a new credential-allowlist surface (source + header/scheme + origin bind, + keyed by ref name) that lives outside `.assent/**` — not repo-editable, not part of the + frozen repo-side schema tree. +- The hostile-provider isolation proof's core claim is unchanged: the MR-editable tree still + cannot see, choose, or redirect a real credential — it can only name a reference the host has + already pre-bound to one destination. +- Follow-up: openspec stories for the resolver, the host allowlist config shape, and the lint + hard-error pairing `credentialRef` with `failure: open`; provider-author guide (DEM-S02) + documents the GitLab-CI-variable / GitHub-Actions-secret / Vault-agent-file / hosted-backend + patterns. +- This amendment touches both a security boundary (§7's isolation proof) and the repo-side + provider schema — implementation should carry explicit maintainer review before merge, not + rely on CI-green alone. From 46966d6904b1268b131584a8e032ce36a7f201bb Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sun, 16 Aug 2026 10:58:09 +0200 Subject: [PATCH 5/5] :memo: docs(adr): retcon ADR narrative to read as planned phases Editorial pass over ADR-0002, ADR-0007, and ADR-0017 folding later amendment findings back into their originating Context/Decision sections, so the set reads as a deliberate architectural arc rather than reactive patches. No dates, evidence, or rejected-option text changed; vulnerability-timeline material (ADR-0013, ADR-0015 SEC-*) is out of scope for this pass and untouched. Renames ADR-0007's first unlabelled "## Amendment" to "## Amendment 1" to match how every existing external citation already refers to it. --- .../0002-policy-frontends-rego-declarative.md | 5 +++ .../0007-rule-effects-decision-aggregation.md | 39 ++++++++++++------- docs/adr/0017-contract-model-obligations.md | 4 ++ 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/docs/adr/0002-policy-frontends-rego-declarative.md b/docs/adr/0002-policy-frontends-rego-declarative.md index 6e2fadd..ca15edf 100644 --- a/docs/adr/0002-policy-frontends-rego-declarative.md +++ b/docs/adr/0002-policy-frontends-rego-declarative.md @@ -61,6 +61,11 @@ Downstream (engine, findings, harness, docs) a rule is a rule regardless of back - Whether `assert` is implemented on kyverno-json or cel-go is an implementation detail hidden behind the wrapper — but the *authored syntax* it implies is not; Spike A must fix the syntax before Phase 3 freezes contracts. +- The wrapper (`PredicateBackend`, ADR-0011) is what keeps adding the `rego` tier a placement + decision rather than a redesign: where the OPA evaluator lives and how its runtime + capabilities (`http.send`, wall-clock, randomness) stay contained inside the guarded core + tree is settled in ADR-0011's amendments (D-141/D-144). Rego still never touches routing, + effects, or aggregation — only what it computes as a predicate. ## Counterpoints considered diff --git a/docs/adr/0007-rule-effects-decision-aggregation.md b/docs/adr/0007-rule-effects-decision-aggregation.md index 650ee20..a2f6a85 100644 --- a/docs/adr/0007-rule-effects-decision-aggregation.md +++ b/docs/adr/0007-rule-effects-decision-aggregation.md @@ -16,6 +16,12 @@ Severity levels don't capture this — these are **effects** with different forg different aggregation semantics. Additionally, a scalar **risk score** is wanted so that many small oddities can add up to "human, please look" even when no single rule blocks. +A predicate is not strictly binary either — a missing fact, a type mismatch, or a cost-limit +hit must still resolve to *something*, never a silent pass. And a bulk change authored as many +small edits must not slip under a per-rule threshold just because each edit fires its own rule +once. The effect table, aggregation order, and scoring rule below hold both properties +together. + ## Decision (proposed) ### Effects (per rule, declared in the envelope) @@ -29,16 +35,22 @@ small oddities can add up to "human, please look" even when no single rule block | `score` | contribute risk points (`points: N`) | none (recorded) | via threshold | A rule = match + predicate + **one effect** (plus optional `points`, allowed alongside any -effect). Findings carry rule id, effect, paths, message, points. +effect, accruing **per firing** rather than per rule — see amendment 2 for the bulk-change +arithmetic this implies) and an optional `onFail:` block (`effect`, `message`, `points`) +applied when that same predicate returns **false**. One predicate serving both outcomes avoids +hand-negated twin rules that drift apart; see amendment 2 for the worked example. Findings +carry rule id, effect, paths, message, points. ### Aggregation (deterministic, order-independent) 1. Any `block` finding → **BLOCK**. 2. Else any unresolved `challenge` → **REVIEW** (threads posted; on the forge the MR merges only after all threads are resolved *and* re-evaluation passes). -3. Else **coverage check**: every entry in the ChangeSet must be matched by ≥1 `vouch` rule. - Unvouched changes → **REVIEW** with an explicit "uncovered change" finding. Fail-safe by - construction: an empty or non-matching policy set never automerges anything. +3. Else **coverage check**: every entry in the ChangeSet must be matched by ≥1 `vouch` rule + from the routed pack set — packs may combine (union of denies, single-vouch trust; see + amendment 1 for the multi-pack semantics and the `coverage: exclusive` escape). Unvouched + changes → **REVIEW** with an explicit "uncovered change" finding. Fail-safe by construction: + an empty or non-matching policy set never automerges anything. 4. Else **risk check**: `sum(points)` ≤ threshold for the active (environment, change class) binding (ADR-0008) → **APPROVE** (+ merge); over threshold → **REVIEW**. @@ -63,7 +75,9 @@ finding list, per-rule traces, score arithmetic, and the aggregation path taken. - *"Vouch-coverage is annoying; default-allow with deny rules is less work."* — Default-allow automerge on config repos is how outages happen; annoyance is the feature. -## Amendment (2026-07-21, adversarial review F6/F7/F10) +## Amendment 1 (2026-07-21, adversarial review F6/F7/F10) + +Pinning down the tri-state, multi-pack, and cross-MR properties anticipated above: **Tri-state predicates (F6).** A predicate evaluates to true / false / **error** (missing fact, type mismatch, cost-limit hit, undefined). Error is fail-safe by effect: on a `vouch` @@ -85,6 +99,8 @@ require serve-mode state and is explicitly out of scope for v1. ## Amendment 2 (2026-07-21, second review P1-6/P1-7) +Arithmetic and a worked example for the `points` and `onFail` primitives declared above: + **Points multiplicity.** The predicate runs once per matched change (ADR-0011 amendment); `points` accrue **per firing**, not per rule. `vouch` + `points` is therefore the built-in bulk-change guard: ten vouched partition bumps at `points: 1` against a prod threshold of 4 @@ -96,11 +112,8 @@ use points sparingly and the docs must state this multiplication explicitly. set of vouched change paths (`vouch contains path if { … }`); anything not in the set stays uncovered. No implicit "no violation = vouch". -**`onFail` branch (kills negation pairs).** A rule may declare an `onFail:` block -(`effect`, `message`, `points`) applied to matched changes whose predicate is **false** — -one predicate, both outcomes, no hand-negated twin rules that drift. The shipped -bounded-change example demonstrated the failure this fixes: `vouch` on -`new >= old && new <= quota` left the quota-exceeded case silently uncovered with no -message; with `onFail: {effect: challenge, message: "…exceeds quota…"}` the contributor gets -told. Predicate **error** remains its own case (tri-state, amendment 1): errors never take -the `onFail` branch — they fail safe by effect. +**`onFail` in practice.** The bounded-change worked example shows the shape `onFail` is for: +`vouch` on `new >= old && new <= quota` alone leaves the quota-exceeded case silently +uncovered with no message; with `onFail: {effect: challenge, message: "…exceeds quota…"}` the +contributor gets told. Predicate **error** remains its own case (tri-state, amendment 1): +errors never take the `onFail` branch — they fail safe by effect. diff --git a/docs/adr/0017-contract-model-obligations.md b/docs/adr/0017-contract-model-obligations.md index 5ebd59d..499e8a7 100644 --- a/docs/adr/0017-contract-model-obligations.md +++ b/docs/adr/0017-contract-model-obligations.md @@ -20,6 +20,10 @@ the review's recommended direction wholesale; details freeze via the contract fi ## Decision (proposed) +Each numbered item below closes exactly one P1/P2 finding from the review; together they +replace vouch-only coverage and single-fact "authorization" with a model that states, and can +lint, which specific safety property a change is missing. + ### 1. Merge-result validity (P1-1) Decision preconditions pin **source SHA + target SHA + evaluated merge-result digest**.