diff --git a/go/internal/store/agent_config.go b/go/internal/store/agent_config.go index d0e2b638..9852effb 100644 --- a/go/internal/store/agent_config.go +++ b/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 diff --git a/go/internal/store/agent_config_test.go b/go/internal/store/agent_config_test.go index 123badfb..6d2df86c 100644 --- a/go/internal/store/agent_config_test.go +++ b/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", diff --git a/go/internal/store/credential_keys_gen.go b/go/internal/store/credential_keys_gen.go index a344cfdb..835827f3 100644 --- a/go/internal/store/credential_keys_gen.go +++ b/go/internal/store/credential_keys_gen.go @@ -1,8 +1,8 @@ // Code generated by gen_credential_keys.go; DO NOT EDIT. // -// Source: forks/oh-my-pi/.../config/settings-schema.ts (isCredential markers: -// `credential: true` at the def level OR `ui.secret === true`). Refresh with -// `go generate ./...` from go/internal/store at a fork bump. +// Source: @oh-my-pi/pi-coding-agent src/config/settings-schema.ts (isCredential +// markers: `credential: true` at the def level OR `ui.secret === true`). Refresh +// with `go generate ./...` from go/internal/store at an SDK bump. package store @@ -12,6 +12,7 @@ var credentialKeys = []string{ "auth.broker.token", "dev.autoqaPush.token", "hindsight.apiToken", + "images.urls.credentials", "mnemopi.embeddingApiKey", "mnemopi.llmApiKey", "searxng.basicPassword", diff --git a/go/internal/store/gen_credential_keys.go b/go/internal/store/gen_credential_keys.go index d1625c75..f58061d6 100644 --- a/go/internal/store/gen_credential_keys.go +++ b/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,18 +125,25 @@ 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 { @@ -116,15 +151,48 @@ func extractCredentialKeys(src string) ([]string, error) { 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 diff --git a/go/moon.yml b/go/moon.yml index a35bdc80..8d67b005 100644 --- a/go/moon.yml +++ b/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