From 4eb80b5b0220ca0a50e582565322362cf6d33af8 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Wed, 19 Aug 2026 00:05:57 +0200 Subject: [PATCH 1/2] :lock: fix(cmd): discriminate absent provider declaration from forge failure (REL-03) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveRunFacts treated ANY FileAtRef error on providers/.json as "declaration absent" and continued, so a retry-exhausted 5xx, a throttle or a token scoped away from the governance repo was indistinguishable from "this provider declares nothing". The fail-safe DIRECTION held — no fact binds, CEL sees an absent attribute, the run degrades to REVIEW — but the PATH was invisible: the operator got a missing-attribute predicate.error naming nothing, and a has()-tolerant policy silently took its fallback branch. A wrong decision reached without a diagnostic. continue now happens only for errors.Is(err, forge.ErrNotFound), the neutral port sentinel every adapter wraps with %w. Every other error ends the run with an error naming the provider, the declaration path and the ref. No new discriminator idiom: this is D-130's ruling applied to its sibling call site. loadResourceOwnerRegistry (same file) fixed exactly this conflation for the who-may-approve registry; fileAtRefOrAbsent (run.go) is the same shape. Hard-failing is also the consistent choice locally — an UNREADABLE declaration now ends the run exactly as a MALFORMED one already did six lines down (GUIDELINES Safety 2). REQ-AUD2-S02-01 absence still skips (through the real gitlab 404 wrap chain) REQ-AUD2-S02-02 5xx returns an error naming provider, path and ref REQ-AUD2-S02-03 401/403 returns an error; a scope misconfiguration is never absence REQ-AUD2-S02-04 non-vacuity: reverting the guard to a bare continue reddens -02 and -03 while -01 stays green REQ-AUD2-S02-05 no shipped decision outcome changes; task check (which runs dogfood-examples over all three packs) exits 0 --- cmd/assent/provider_host.go | 20 +++- cmd/assent/provider_host_test.go | 159 +++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 2 deletions(-) create mode 100644 cmd/assent/provider_host_test.go diff --git a/cmd/assent/provider_host.go b/cmd/assent/provider_host.go index 4beb57d5..00d2f5ee 100644 --- a/cmd/assent/provider_host.go +++ b/cmd/assent/provider_host.go @@ -81,8 +81,24 @@ func resolveRunFacts( declPath := path.Join(declDir, name+".json") raw, err := client.FileAtRef(project, declPath, targetRef) if err != nil { - // Missing host declaration → skip (cannot know outputs; inventing - // unavailable keys would change CEL from "absent" to "false"). + if !errors.Is(err, forge.ErrNotFound) { + // REL-03 / D-130, GUIDELINES §Safety 2: a broken forge is not an + // absent file. This used to `continue` on ANY error, so a 503, a + // throttle or a token scoped away from the governance repo was + // indistinguishable from "this provider declares nothing". The + // fail-safe DIRECTION held (no fact binds → CEL sees an absent + // attribute → REVIEW), but the PATH was invisible: the operator + // got a missing-attribute predicate error naming nothing, and a + // `has()`-tolerant policy silently took its fallback branch — a + // wrong decision reached without a diagnostic. Failing here is + // also the consistent choice: an UNREADABLE declaration now ends + // the run exactly as a MALFORMED one already does, six lines down. + return nil, nil, fmt.Errorf("provider %q declaration %q at ref %q: %w", name, declPath, targetRef, err) + } + // Genuinely absent host declaration → skip (cannot know outputs; + // inventing unavailable keys would change CEL from "absent" to + // "false"). `forge.ErrNotFound` is the neutral port sentinel for + // absence, which every adapter wraps with %w. continue } hostCfg, err := provider.LoadProviderConfig(raw) diff --git a/cmd/assent/provider_host_test.go b/cmd/assent/provider_host_test.go new file mode 100644 index 00000000..70def8f8 --- /dev/null +++ b/cmd/assent/provider_host_test.go @@ -0,0 +1,159 @@ +package main + +import ( + "context" + "strconv" + "strings" + "testing" + "time" + + "github.com/PlatformRelay/assent/internal/core/policy" +) + +// AUD2-S02 / REL-03 — `providers/.json` absence vs. an unanswering forge. +// +// resolveRunFacts used to `continue` on ANY FileAtRef error for the host +// declaration, so a 503, a throttle or a mis-scoped token was indistinguishable +// from "this provider declares nothing". The fail-safe DIRECTION held (the fact +// never binds, so CEL sees an absent attribute and the run degrades to REVIEW), +// but the path was invisible: the operator read a `predicate.error` about a +// missing attribute, and a `has()`-tolerant policy quietly took its fallback +// branch. D-130 already ruled this exact conflation for the ownership registry +// at loadResourceOwnerRegistry; these tests pin the sibling call site. +// +// The declaration path under test is `/providers/.json` at the +// TARGET ref (D-065) — `.assent/providers/quota.json` for the fixtures below. +const quotaDeclPath = ".assent/providers/quota.json" + +// declErrPort is the whole forge with ONE read broken: the host declaration for +// `quota` fails with the supplied error, everything else answers normally. Same +// shape as registry503Port (provider_host_registry_test.go) — a stub covering +// only the declaration read could not express a forge that is otherwise healthy, +// which is precisely the live shape of a blip or a scope-limited token. +type declErrPort struct { + forgePort + declPath string + err error +} + +func (c declErrPort) FileAtRef(project, p, ref string) ([]byte, error) { + if p == c.declPath { + return nil, c.err + } + return c.forgePort.FileAtRef(project, p, ref) +} + +// TestProviderDeclarationAbsentSkipsProvider — REQ-AUD2-S02-01. +// +// Absence stays absence: today's behaviour, preserved byte for byte. The fake +// serves no declaration for `quota`, so the request 404s and the REAL gitlab +// adapter wraps it onto forge.ErrNotFound through its own chain — this proves +// the wrap survives, which a hand-rolled sentinel stub could not. +func TestProviderDeclarationAbsentSkipsProvider(t *testing.T) { + f := newFakeGitLab(t) + f.config = configQuotaRepoFile() + f.providerDecls = nil // nothing declared → the fake answers 404 + client := f.factory()("", "tok", "assent-bot") + + conf, err := policy.LoadConfig([]byte(configQuotaRepoFile())) + if err != nil { + t.Fatal(err) + } + + facts, resolvedAt, err := resolveRunFacts( + context.Background(), conf, ".assent/config.yaml", client, + "42", "main", t.TempDir(), "file:topics/prod/orders.yaml", time.Now().UTC(), + ) + if err != nil { + t.Fatalf("an absent declaration must stay a skip, not an error: %v", err) + } + if len(facts) != 0 || len(resolvedAt) != 0 { + t.Fatalf("a skipped provider must bind no facts; got facts=%v resolvedAt=%v", facts, resolvedAt) + } +} + +// TestProviderDeclarationForgeErrorAbortsResolveRunFacts — REQ-AUD2-S02-02. +// +// A 503 on the declaration read is NOT an absent file. Before this story the run +// continued with an empty fact map and never mentioned the forge failure; the +// operator saw only a missing-attribute predicate error, and a `has()`-tolerant +// policy took its fallback branch on a forge blip. Consistency argument: the very +// next statement in resolveRunFacts already hard-fails on a MALFORMED declaration, +// so failing on an UNREADABLE one is the same policy applied to the same input. +func TestProviderDeclarationForgeErrorAbortsResolveRunFacts(t *testing.T) { + f := newFakeGitLab(t) + f.config = configQuotaRepoFile() + f.providerDecls = map[string]string{"quota": quotaDeclarationJSON} + client := declErrPort{ + forgePort: f.factory()("", "tok", "assent-bot"), + declPath: quotaDeclPath, + err: brokenForge(quotaDeclPath, "main", 503), + } + + conf, err := policy.LoadConfig([]byte(configQuotaRepoFile())) + if err != nil { + t.Fatal(err) + } + + facts, resolvedAt, err := resolveRunFacts( + context.Background(), conf, ".assent/config.yaml", client, + "42", "main", t.TempDir(), "file:topics/prod/orders.yaml", time.Now().UTC(), + ) + if err == nil { + t.Fatalf("a 503 on the host declaration must abort fact resolution; "+ + "got facts=%v resolvedAt=%v", facts, resolvedAt) + } + // The message has to be actionable: WHICH provider, WHICH path, WHICH ref. + // Without the ref an operator cannot tell a target-ref read from a head read. + for _, want := range []string{`provider "quota"`, quotaDeclPath, `"main"`, "503"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error %q must contain %q (provider, declaration path, ref, forge failure)", err, want) + } + } + // Fail-closed shape: no partial fact map escapes alongside the error, so no + // caller can evaluate a provider-configured policy on an empty Facts map. + if facts != nil || resolvedAt != nil { + t.Fatalf("error path must return no facts; got %v / %v", facts, resolvedAt) + } +} + +// TestProviderDeclarationUnauthorizedAbortsResolveRunFacts — REQ-AUD2-S02-03. +// +// A 401/403 is deterministic, not transient: a token scoped away from the +// governance repo would have made EVERY provider silently vanish on EVERY run, +// converting approvable MRs to REVIEW forever with nothing in the logs to say +// why. This is the misconfiguration the old `continue` hid best. +func TestProviderDeclarationUnauthorizedAbortsResolveRunFacts(t *testing.T) { + for _, status := range []int{401, 403} { + t.Run(strconv.Itoa(status), func(t *testing.T) { + f := newFakeGitLab(t) + f.config = configQuotaRepoFile() + f.providerDecls = map[string]string{"quota": quotaDeclarationJSON} + client := declErrPort{ + forgePort: f.factory()("", "tok", "assent-bot"), + declPath: quotaDeclPath, + err: brokenForge(quotaDeclPath, "main", status), + } + + conf, err := policy.LoadConfig([]byte(configQuotaRepoFile())) + if err != nil { + t.Fatal(err) + } + + facts, resolvedAt, err := resolveRunFacts( + context.Background(), conf, ".assent/config.yaml", client, + "42", "main", t.TempDir(), "file:topics/prod/orders.yaml", time.Now().UTC(), + ) + if err == nil { + t.Fatalf("a %d on the host declaration must abort fact resolution; "+ + "got facts=%v resolvedAt=%v", status, facts, resolvedAt) + } + if !strings.Contains(err.Error(), `provider "quota"`) { + t.Fatalf("error %q must name the provider whose declaration could not be read", err) + } + if facts != nil || resolvedAt != nil { + t.Fatalf("error path must return no facts; got %v / %v", facts, resolvedAt) + } + }) + } +} From dafeb867a334fac4d0352c0aca7241765e8eccc4 Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Wed, 19 Aug 2026 00:09:20 +0200 Subject: [PATCH 2/2] :white_check_mark: test(cmd): assert the REL-03 error wrap as one contiguous substring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REQ-AUD2-S02-02 asks the error to name the provider, the declaration path AND the ref. The first version checked those as three separate strings.Contains calls — but brokenForge (the fake's adapter-shaped error) already renders the path and the ref inside the wrapped cause, so two of the three assertions could not fail: a bare `provider %q: %w` wrap would have passed them. The test was measuring the fake, not the code. Asserted now as ONE contiguous substring that only the outer wrap can produce, plus a separate check that the forge's own cause survives the %w. Non-vacuity re-proved: mutating the implementation to `provider %q: %w` reddens TestProviderDeclarationForgeErrorAbortsResolveRunFacts and nothing else. The first version of the test stayed GREEN under that same mutation. Also corrects the code comment: the malformed-declaration hard-fail this change is made consistent with is in the very next statement (LoadProviderConfig), not "six lines down" as the previous commit body and comment both said. --- CHANGELOG.md | 6 ++++++ cmd/assent/provider_host.go | 3 ++- cmd/assent/provider_host_test.go | 18 ++++++++++++++---- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8150734..6bcaf4dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,12 @@ repository still gets a decision, never by following the link; no release carrie ### Fixes - :bug: docs(decisions): reference the AUD2 spec as a path, not an mkdocs link + +### Security +- :lock: fix(cmd): discriminate absent provider declaration from forge failure (REL-03) + +### Testing +- :white_check_mark: test(cmd): assert the REL-03 error wrap as one contiguous substring ## [0.3.0] - 2026-08-18 ### Chores diff --git a/cmd/assent/provider_host.go b/cmd/assent/provider_host.go index 00d2f5ee..767ab9a1 100644 --- a/cmd/assent/provider_host.go +++ b/cmd/assent/provider_host.go @@ -92,7 +92,8 @@ func resolveRunFacts( // `has()`-tolerant policy silently took its fallback branch — a // wrong decision reached without a diagnostic. Failing here is // also the consistent choice: an UNREADABLE declaration now ends - // the run exactly as a MALFORMED one already does, six lines down. + // the run exactly as a MALFORMED one already does, in the very + // next statement (LoadProviderConfig). return nil, nil, fmt.Errorf("provider %q declaration %q at ref %q: %w", name, declPath, targetRef, err) } // Genuinely absent host declaration → skip (cannot know outputs; diff --git a/cmd/assent/provider_host_test.go b/cmd/assent/provider_host_test.go index 70def8f8..41b01250 100644 --- a/cmd/assent/provider_host_test.go +++ b/cmd/assent/provider_host_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "fmt" "strconv" "strings" "testing" @@ -105,10 +106,19 @@ func TestProviderDeclarationForgeErrorAbortsResolveRunFacts(t *testing.T) { } // The message has to be actionable: WHICH provider, WHICH path, WHICH ref. // Without the ref an operator cannot tell a target-ref read from a head read. - for _, want := range []string{`provider "quota"`, quotaDeclPath, `"main"`, "503"} { - if !strings.Contains(err.Error(), want) { - t.Fatalf("error %q must contain %q (provider, declaration path, ref, forge failure)", err, want) - } + // + // Asserted as ONE CONTIGUOUS substring on purpose. brokenForge already renders + // the path and the ref inside the wrapped cause, so three separate Contains + // checks would pass against a bare `provider %q: %w` wrap — two of the three + // could not fail, and the test would measure the fake instead of the code. + // Only the outer wrap can produce this exact run of bytes. + wantWrap := fmt.Sprintf("provider %q declaration %q at ref %q", "quota", quotaDeclPath, "main") + if !strings.Contains(err.Error(), wantWrap) { + t.Fatalf("error %q must contain %q — the provider, the declaration path and the ref", err, wantWrap) + } + // ...and the forge's own cause must survive the wrap (%w, not %v-and-drop). + if !strings.Contains(err.Error(), "503") { + t.Fatalf("error %q must carry the forge's own failure", err) } // Fail-closed shape: no partial fact map escapes alongside the error, so no // caller can evaluate a provider-configured policy on an empty Facts map.