refactor(store): read the credential denylist from the installed SDK, not the vendored subtree (RIG-2407)

go/internal/store/agent_config.go

@@ -1013,7 +1013,8 @@ func parseYAMLMapping(content []byte, joined string) (map[string]any, error) {
 }
 
 // credentialKeys (credential_keys_gen.go) is generated from the SDK's
-// isCredential markers; refresh it at a fork bump with `go generate ./...`.
+// isCredential markers, read from the installed @oh-my-pi/pi-coding-agent;
+// refresh it at an SDK bump with `go generate ./...`.
 //
 //go:generate go run gen_credential_keys.go
 

go/internal/store/agent_config_test.go

@@ -569,6 +569,16 @@ func TestValidateConfigBundleRejectsCredentialKeys(t *testing.T) {
 			member:  tarEntry{name: "models.yml", content: "providers:\n  0: junk\n  openai:\n    1: junk\n    apiKey: sk-secret\n"},
 			wantSub: "providers.openai.apiKey",
 		},
+		{
+			// The only denylist entry whose leaf is a RECORD rather than a scalar
+			// string, so it exercises yamlPathIsSet's non-nil-leaf check
+			// differently from every case above (all of which terminate in a
+			// string). Pins the door's behavior on the path the SDK 18.x bump
+			// added.
+			name:    "settings record-valued credential leaf",
+			member:  tarEntry{name: "settings/config.yml", content: "images:\n  urls:\n    credentials:\n      s3: {key: v}\n"},
+			wantSub: "images.urls.credentials",
+		},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -584,15 +594,27 @@ func TestValidateConfigBundleRejectsCredentialKeys(t *testing.T) {
 	}
 }
 
-// TestCredentialKeysMatchSchema is a change-detector on the generated denylist:
-// the seven SDK isCredential paths (settings-schema.ts) are the load-bearing
-// door policy, so a fork bump that adds or drops one must be caught. If this
-// reds, regenerate credential_keys_gen.go (`go generate ./...`) and re-review.
+// TestCredentialKeysMatchSchema guards the generated denylist against a
+// hand-edit or a bad merge of the DO-NOT-EDIT file: it pins credentialKeys to an
+// explicit list, so an in-repo change to either side reds.
+//
+// It deliberately does NOT detect an SDK bump, and cannot: the schema lives in
+// gitignored node_modules, so a bump changes the source of truth while touching
+// neither credentialKeys nor `want`, leaving this green. The
+// `compass-go:credential-keys-drift` moon task owns that detection by
+// regenerating against the installed schema and failing on a byte diff.
+//
+// When that gate reds, regenerate (`go generate ./...` from go/internal/store)
+// and update `want` here in the same commit — a new credential-marked path is
+// expected at a bump and must be ADOPTED, but a path DISAPPEARING means the door
+// stopped rejecting something it used to, which is a policy regression to
+// investigate before accepting.
 func TestCredentialKeysMatchSchema(t *testing.T) {
 	want := []string{
 		"auth.broker.token",
 		"dev.autoqaPush.token",
 		"hindsight.apiToken",
+		"images.urls.credentials",
 		"mnemopi.embeddingApiKey",
 		"mnemopi.llmApiKey",
 		"searxng.basicPassword",

go/internal/store/gen_credential_keys.go

@@ -4,24 +4,29 @@
 // settings schema. It is the authoritative refresh step for the store door's
 // credential denylist (RIG-1678 T1, OQ-2 (c)): the door rejects a
 // settings/config.yml that sets any SDK credential-marked path, and that path
-// set MUST track the SDK's own isCredential marker across fork bumps.
+// set MUST track the SDK's own isCredential marker across SDK bumps.
 //
-// isCredential (forks/oh-my-pi/packages/coding-agent/src/config/settings-schema.ts,
+// isCredential (@oh-my-pi/pi-coding-agent src/config/settings-schema.ts,
 // `export function isCredential`) marks a path credential when its schema def
 // carries EITHER `credential: true` at the def level OR `ui.secret === true`.
 // This generator reproduces that exact rule by structurally parsing the
 // SETTINGS_SCHEMA object literal — a self-contained scan with no Node/Bun
-// runtime and no native-module load, so it stays reproducible at a fork bump
+// runtime and no native-module load, so it stays reproducible at an SDK bump
 // (running isCredential itself would drag the SDK's native `@oh-my-pi/pi-*`
 // modules and their platform binaries into the build).
 //
-// Refresh at a fork bump: from go/internal/store, run
+// The schema is read from the INSTALLED package in node_modules rather than a
+// vendored copy, so the denylist tracks the pinned dependency. It resolves
+// through the CONSUMING package's own dependency edge (see schemaRelPath), so
+// the denylist is generated from the schema compass-agent actually runs.
+//
+// Refresh at an SDK bump: from go/internal/store, run
 //
 //	go generate ./...
 //
 // (the //go:generate directive on credentialKeys in agent_config.go invokes
 // this), then commit the regenerated credential_keys_gen.go. This is the
-// fork-bump checklist entry CP-3's prevention property names — any new
+// SDK-bump checklist entry CP-3's prevention property names — any new
 // credential-marked SDK path lands in the denylist here.
 package main
 
@@ -39,7 +44,21 @@ import (
 // schemaRelPath is the settings schema, relative to this generator's own
 // directory (go/internal/store) — resolved from runtime.Caller so `go generate`
 // finds it regardless of the caller's working directory.
-const schemaRelPath = "../../../forks/oh-my-pi/packages/coding-agent/src/config/settings-schema.ts"
+//
+// It deliberately resolves through the CONSUMING workspace package's own
+// node_modules edge rather than bun's `node_modules/.bun/node_modules/` hoist
+// alias. That alias is a flat namespace with one entry per package NAME, so
+// with two dependents on different versions it can resolve to a version the
+// agent does not run — and it would fail silently, since the wrong schema
+// still parses fine. This path expresses the real invariant: the schema
+// belonging to the package that consumes the SDK.
+//
+// The schema lives in gitignored node_modules, so an SDK bump changes this
+// generator's input without touching any tracked file. The
+// `compass-go:credential-keys-drift` CI task is what makes such a bump fail
+// loudly if the denylist was not regenerated; a `bun install` must have run
+// before `go generate`.
+const schemaRelPath = "../../../packages/compass-agent/node_modules/@oh-my-pi/pi-coding-agent/src/config/settings-schema.ts"
 
 func main() {
 	if err := run(); err != nil {
@@ -59,21 +78,30 @@ func run() error {
 		return fmt.Errorf("read settings schema %q: %w", schemaPath, err)
 	}
 
-	keys, err := extractCredentialKeys(string(src))
+	keys, total, err := extractCredentialKeys(string(src))
 	if err != nil {
 		return fmt.Errorf("parse settings schema: %w", err)
 	}
 	if len(keys) == 0 {
 		return fmt.Errorf("no credential-marked paths found — schema shape changed, refusing to emit an empty denylist")
 	}
+	// A plausibility floor on the TOTAL parsed key count. The empty-denylist
+	// guard above only catches a total parse failure; the hand-rolled parser's
+	// real risk is UNDER-collection — a def shape it mishandles yields a short
+	// list, not an error, which would silently drop a path from the door's
+	// denylist. The pinned schema carries ~480 top-level paths, so a count far
+	// below that means the parser lost its footing on a new shape.
+	if total < minSchemaKeys {
+		return fmt.Errorf("parsed only %d top-level schema paths (expected >= %d) — schema shape changed and the parser is likely under-collecting; refusing to emit a possibly-short denylist", total, minSchemaKeys)
+	}
 	sort.Strings(keys)
 
 	var b bytes.Buffer
 	fmt.Fprintln(&b, "// Code generated by gen_credential_keys.go; DO NOT EDIT.")
 	fmt.Fprintln(&b, "//")
-	fmt.Fprintln(&b, "// Source: forks/oh-my-pi/.../config/settings-schema.ts (isCredential markers:")
-	fmt.Fprintln(&b, "// `credential: true` at the def level OR `ui.secret === true`). Refresh with")
-	fmt.Fprintln(&b, "// `go generate ./...` from go/internal/store at a fork bump.")
+	fmt.Fprintln(&b, "// Source: @oh-my-pi/pi-coding-agent src/config/settings-schema.ts (isCredential")
+	fmt.Fprintln(&b, "// markers: `credential: true` at the def level OR `ui.secret === true`). Refresh")
+	fmt.Fprintln(&b, "// with `go generate ./...` from go/internal/store at an SDK bump.")
 	fmt.Fprintln(&b, "")
 	fmt.Fprintln(&b, "package store")
 	fmt.Fprintln(&b, "")
@@ -97,34 +125,74 @@ func run() error {
 	return nil
 }
 
+// minSchemaKeys is the plausibility floor for the total top-level path count
+// (see the check in run). The pinned schema has ~480; this is set well below to
+// stay quiet across ordinary schema growth or trimming while still catching a
+// parser that has stopped walking the literal properly.
+const minSchemaKeys = 400
+
 // extractCredentialKeys parses the SETTINGS_SCHEMA object literal in the schema
-// source and returns every top-level key whose def is credential-marked. It
+// source and returns every top-level key whose def is credential-marked, plus
+// the total number of top-level keys walked (for run's plausibility floor). It
 // mirrors isCredential: a def is credential-marked when it has `credential:
 // true` directly, or a `ui` object with `secret: true`.
-func extractCredentialKeys(src string) ([]string, error) {
+func extractCredentialKeys(src string) ([]string, int, error) {
 	p := &jsParser{s: src}
 	if err := p.seekSchema(); err != nil {
-		return nil, err
+		return nil, 0, err
 	}
 	schema, err := p.parseObject()
 	if err != nil {
-		return nil, err
+		return nil, 0, err
 	}
 	var keys []string
 	for key, val := range schema {
 		def, ok := val.(map[string]any)
 		if !ok {
 			continue
 		}
-		if def["credential"] == "true" {
+		if isLiteralTrue(def["credential"]) {
 			keys = append(keys, key)
 			continue
 		}
-		if ui, ok := def["ui"].(map[string]any); ok && ui["secret"] == "true" {
+		if ui, ok := def["ui"].(map[string]any); ok && isLiteralTrue(ui["secret"]) {
 			keys = append(keys, key)
 		}
 	}
-	return keys, nil
+	return keys, len(schema), nil
+}
+
+// isLiteralTrue reports whether a parsed def value is the literal `true`.
+// Non-object values arrive as their trimmed raw text, so ordinary authoring
+// that decorates the literal must not read as false: a trailing line or block
+// comment (`credential: true // yes`) and a `as const` assertion are both
+// stripped before comparing. Anything else — a helper call, a variable, a
+// computed expression — is deliberately NOT treated as true, since the
+// generator cannot evaluate it.
+func isLiteralTrue(v any) bool {
+	s, ok := v.(string)
+	if !ok {
+		return false
+	}
+	if i := strings.Index(s, "//"); i >= 0 {
+		s = s[:i]
+	}
+	for {
+		i := strings.Index(s, "/*")
+		if i < 0 {
+			break
+		}
+		j := strings.Index(s[i+2:], "*/")
+		if j < 0 {
+			s = s[:i]
+			break
+		}
+		s = s[:i] + s[i+2+j+2:]
+	}
+	s = strings.TrimSpace(s)
+	s = strings.TrimSuffix(s, "const")
+	s = strings.TrimSpace(strings.TrimSuffix(strings.TrimSpace(s), "as"))
+	return s == "true"
 }
 
 // jsParser is a minimal structural parser over a JS/TS source string. It reads

go/moon.yml

@@ -252,6 +252,37 @@ tasks:
       runFromWorkspaceRoot: false
     inputs: *sqlc_sources
 
+  credential-keys-drift:
+    # Fail if the checked-in credential denylist is stale vs the INSTALLED SDK
+    # settings schema: snapshot the committed file, regenerate, and fail on any
+    # byte diff. This exists because the generator's input
+    # (packages/compass-agent/node_modules/@oh-my-pi/pi-coding-agent) is
+    # gitignored, so an SDK bump moves the source of truth without touching any
+    # tracked file — the in-repo change-detector test compares credentialKeys
+    # against a hand-maintained want list, and BOTH sides stay unchanged at a
+    # bump, so it cannot catch it. Renovate bumps the caret pin
+    # (`^18.0.11`) unattended, and a missed regeneration means the store door
+    # silently stops rejecting a newly credential-marked path. Modeled on
+    # sqlc-drift above and in `ci` for the same reason: fully local, no DB, no
+    # cross-project delegation. `git diff --no-index` compares raw files (reads
+    # no index/worktree state, and git is always on PATH), so it behaves
+    # identically in CI and in a secondary jj workspace. The generator writes in
+    # place and is idempotent, so a no-drift run leaves the byte-identical file.
+    #
+    # Requires a completed `bun install` — the generator fails loudly, naming the
+    # absolute schema path, when the dependency is not installed.
+    script: 'tmp=$(mktemp -d); trap ''rm -rf "$tmp"'' EXIT; cp internal/store/credential_keys_gen.go "$tmp/credential_keys_gen.go"; go generate ./internal/store/...; if ! git diff --no-index --quiet "$tmp/credential_keys_gen.go" internal/store/credential_keys_gen.go; then echo "credential-keys drift: internal/store/credential_keys_gen.go is stale vs the installed SDK schema — run \`go generate ./...\` from go/internal/store and commit. A path APPEARING is an SDK bump to adopt; a path DISAPPEARING means the door stopped rejecting a credential it used to, and is a policy regression to investigate before accepting:"; git diff --no-index "$tmp/credential_keys_gen.go" internal/store/credential_keys_gen.go; exit 1; fi'
+    options:
+      runFromWorkspaceRoot: false
+    inputs:
+      - 'internal/store/gen_credential_keys.go'
+      - 'internal/store/credential_keys_gen.go'
+      - 'internal/store/agent_config.go'
+      # The schema itself is gitignored and cannot be an input, so key on the
+      # lockfile: the pinned SDK version is visible there, so a bump reschedules
+      # this task.
+      - '/bun.lock'
+
   sqlc-vet:
     # `sqlc vet` with the sqlc/db-prepare rule PREPAREs every generated query
     # against a live, schema-loaded Postgres — the strongest check sqlc offers
@@ -304,15 +335,16 @@ tasks:
 
   ci:
     # module is affected: format, vet, lint (incl. exhaustiveness), -race test,
-    # build, the supply-chain fence, and the sqlc drift gate. `drift` is NOT a
+    # build, the supply-chain fence, and the two drift gates. `drift` is NOT a
     # dep here — it belongs to the schema pipeline (compass-proto) and is
     # scheduled through the gen tree, matching compass-proto/moon.yml's own
-    # `ci` composition. `sqlc-drift` IS a dep: it is fully local (no
-    # cross-project delegation, no DB), so it belongs in ci directly. `sqlc-vet`
+    # `ci` composition. `sqlc-drift` and `credential-keys-drift` ARE deps: both
+    # are fully local (no cross-project delegation, no DB), so they belong in ci
+    # directly. `sqlc-vet`
     # is deliberately NOT a dep: it needs a live Postgres, which the moon battery
     # has no business realizing (ci.yml has no service on the moon job); it is
     # invoked instead by the env-bearing `pgtest` CI job, the same way pgtest/
     # microvm/forge/gtk4 are peeled into their own service-carrying peer jobs.
-    deps: ['fmt', 'vet', 'lint', 'nilaway', 'test', 'build', 'vuln', 'licenses', 'sqlc-drift']
+    deps: ['fmt', 'vet', 'lint', 'nilaway', 'test', 'build', 'vuln', 'licenses', 'sqlc-drift', 'credential-keys-drift']
     options:
       cache: false