refactor(store): read the credential denylist from the installed SDK, not the vendored subtree (RIG-2407) - #928
Open
rigel-mintaka wants to merge 2 commits into
Conversation
… not the vendored subtree (RIG-2407)
PR2 of 3. Repoints the credential-denylist generator off `forks/oh-my-pi/` onto the installed `@oh-my-pi/pi-coding-agent`, removing the last **build-time** dependency on the vendored subtree so PR3 can delete it.
### What moves
`schemaRelPath` (`gen_credential_keys.go`) now resolves the settings schema from the installed package. Note bun's isolated linker keeps packages under `node_modules/.bun/node_modules/`, not at the plain top level, so that prefix is load-bearing — and `bun install` must have run before `go generate`.
The parser itself is unchanged, and deliberately so: `isCredential` in 18.0.11 (`src/config/settings-schema.ts:6064-6071`) still marks a path credential on `credential: true` at the def level **or** `ui.secret === true`, which is exactly the rule `extractCredentialKeys` reproduces structurally. This stays a path change, not a semantics change.
Also fixed the **hardcoded** provenance literal the generator emits into `credential_keys_gen.go`. Regenerating alone would not have corrected it — it would have re-emitted `forks/oh-my-pi/...` into a file whose source no longer exists after PR3.
### The denylist grows by one, which is the fail-safe direction
```
+ images.urls.credentials
```
7 → 8 paths, **nothing removed**. Verified the new path is genuinely credential-marked upstream (`settings-schema.ts:1054`, `credential: true`), so this is the generator correctly tracking the pinned dependency rather than drift: one more credential path the store door rejects.
`TestCredentialKeysMatchSchema`'s `want` set is updated in the same commit — a denylist diff is the expected outcome of an SDK bump, not a failure. I also sharpened that test's docstring to say which direction of diff is safe: a path appearing is adopted, but a path **disappearing** means the door stopped rejecting something it used to, and that is a policy regression to investigate before accepting. The old wording ("regenerate and re-review") did not distinguish them.
### Verification
- `go generate ./...` from `go/internal/store` — clean, writes 8 paths
- `go test ./internal/store/` — ok (full package, `-count=1`)
- `go test ./internal/store/ -run 'TestCredentialKeys|TestValidateConfigBundleRejectsCredentialKeys'` — ok
- `gofmt -l internal/store/` — clean; `go vet ./internal/store/` — clean
- `grep -rn 'forks/oh-my-pi' go/internal/store/` — no matches
`forks/oh-my-pi/` is still present after this lands; PR3 deletes the tree, its `.moon/workspace.yml` registration, and the remaining prose references.
Spec-impact: none. Refs RIG-2407
Co-authored-by: Matt Wilkinson <matt@rigel.build>
|
Compass engineering docs preview: https://compass-repo-rig-2407-genera.compass-eng-docs.pages.dev Deployed from |
…ma through the consuming package (RIG-2407) Review fixes on the generator repoint. One high, two medium, one low. ### The change-detector could no longer detect the thing it exists for (high) Moving the schema into gitignored `node_modules` broke the property that made "regenerate at an SDK bump" enforceable. `TestCredentialKeysMatchSchema` compares `credentialKeys` against a hand-maintained `want` list — both in-repo, so an SDK bump changes the source of truth while touching **neither side**, and the test stays green. Under the vendored subtree the schema was a tracked file, so a bump landed a reviewable diff; that visibility is what the repoint silently removed. Renovate bumps the caret pin unattended, so this is the expected path, not a hypothetical. Added `compass-go:credential-keys-drift` to `go/moon.yml`, modeled on the `sqlc-drift` precedent already there and in `ci` for the same reasons (fully local, no DB, no cross-project delegation): snapshot the generated file, regenerate, fail on any byte diff. Since the schema is gitignored and cannot be a task input, it keys on `bun.lock`, where the pinned version is visible, so a bump reschedules it. Verified red-green rather than assumed: injecting one `credential: true` path into the installed schema makes the gate write 9 paths and exit 1 with the diff naming `zz.injected.probeToken` — while `TestCredentialKeysMatchSchema` stays **green**, which is exactly the blind spot. Restored, the gate passes. `TestCredentialKeysMatchSchema` keeps its real remaining value (catching a hand-edit or bad merge of the DO-NOT-EDIT file); its docstring now says plainly that it does *not* detect a bump and names the gate that does, instead of gating advice on an "if this reds" that cannot happen. ### The schema path resolved through bun's hoist alias (medium) `schemaRelPath` pointed into `node_modules/.bun/node_modules/`, a flat namespace with one entry per package **name**. With two dependents on different versions it can resolve to a version the agent does not run, and it fails silently — the wrong schema parses fine. Repointed to the consuming package's own dependency edge, `packages/compass-agent/node_modules/@oh-my-pi/pi-coding-agent/...`, which is what the frozen design record specifies (`design.md:451-455`, `oq-resolutions.md:29-33`) and which I had deviated from without saying so. The symlink confirms it resolves version-correctly by construction (`-> …/@oh-my-pi+pi-coding-agent@18.0.11+09a228d332b3c507/…`). Regenerating through the new path is **byte-identical**. ### The parser under-collected on ordinary authoring (medium) `extractCredentialKeys` compared marker text with a bare `== "true"`, so shapes the SDK could adopt at any time dropped a credential path *silently*: `credential: true // yes`, `credential: true /* yes */`, and `credential: true as const` all yielded no match. Replaced with an `isLiteralTrue` helper that strips trailing line/block comments and an `as const` assertion before comparing, while still refusing to guess at anything it cannot evaluate (a helper call, a variable). Unit-checked all seven shapes. The `len(keys) == 0` guard only caught a *total* parse failure, which by construction cannot catch a partial miss. Added a plausibility floor on the total top-level key count (`minSchemaKeys = 400`; the pinned schema has ~480), so a parser that has lost its footing on a new shape is a loud error rather than a short denylist. ### Behavioral coverage for the new entry (low) `images.urls.credentials` is the first denylist entry whose leaf is a **record** rather than a scalar string, so it exercises `yamlPathIsSet`'s non-nil-leaf check differently from every existing case, and nothing covered it. Added a door case. Confirmed red-green: with the path removed from the denylist the door **accepts** the credential bundle (`want ErrInvalidArgument, got <nil>`) — the exact silent regression it now catches. ### Verification - `go test ./internal/store/... -count=1` — ok - `go generate ./internal/store/...` — byte-identical through the new path, 8 paths - `moon run compass-go:credential-keys-drift` — green on the clean tree, red on an injected schema path - `gofmt -l internal/store/` clean; `go vet ./internal/store/` clean - `grep -rn 'forks/oh-my-pi' go/internal/store/` — no matches Refs RIG-2407 Co-authored-by: Matt Wilkinson <matt@rigel.build>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a stack containing 2 PRs:
mainPR2 of 3. Repoints the credential-denylist generator off
forks/oh-my-pi/onto the installed@oh-my-pi/pi-coding-agent, removing the last build-time dependency on the vendored subtree so PR3 can delete it.What moves
schemaRelPath(gen_credential_keys.go) now resolves the settings schema from the installed package. Note bun's isolated linker keeps packages undernode_modules/.bun/node_modules/, not at the plain top level, so that prefix is load-bearing — andbun installmust have run beforego generate.The parser itself is unchanged, and deliberately so:
isCredentialin 18.0.11 (src/config/settings-schema.ts:6064-6071) still marks a path credential oncredential: trueat the def level orui.secret === true, which is exactly the ruleextractCredentialKeysreproduces structurally. This stays a path change, not a semantics change.Also fixed the hardcoded provenance literal the generator emits into
credential_keys_gen.go. Regenerating alone would not have corrected it — it would have re-emittedforks/oh-my-pi/...into a file whose source no longer exists after PR3.The denylist grows by one, which is the fail-safe direction
7 → 8 paths, nothing removed. Verified the new path is genuinely credential-marked upstream (
settings-schema.ts:1054,credential: true), so this is the generator correctly tracking the pinned dependency rather than drift: one more credential path the store door rejects.TestCredentialKeysMatchSchema'swantset is updated in the same commit — a denylist diff is the expected outcome of an SDK bump, not a failure. I also sharpened that test's docstring to say which direction of diff is safe: a path appearing is adopted, but a path disappearing means the door stopped rejecting something it used to, and that is a policy regression to investigate before accepting. The old wording ("regenerate and re-review") did not distinguish them.Verification
go generate ./...fromgo/internal/store— clean, writes 8 pathsgo test ./internal/store/— ok (full package,-count=1)go test ./internal/store/ -run 'TestCredentialKeys|TestValidateConfigBundleRejectsCredentialKeys'— okgofmt -l internal/store/— clean;go vet ./internal/store/— cleangrep -rn 'forks/oh-my-pi' go/internal/store/— no matchesforks/oh-my-pi/is still present after this lands; PR3 deletes the tree, its.moon/workspace.ymlregistration, and the remaining prose references.Spec-impact: none. Refs RIG-2407
Co-authored-by: Matt Wilkinson matt@rigel.build