feat(trust): pgserve trust add/list/remove (singleton G3 verb 2)#87
Conversation
Second of four CLI verbs from `.genie/wishes/pgserve-singleton-no-proxy/`
Group 3. Manages the user-extensible cosign trust store at
`~/.pgserve/trust/identities.json`, layered on top of the hardcoded
`TRUSTED_IDENTITIES` constant from `src/cosign/trust-list.js`.
src/cosign/trust-store.js (new):
- Atomic writer (`<file>.tmp.<pid>` + rename), 0700 dir / 0600 file.
- Schema v1: `{ schemaVersion, entries: [...] }`. Throws ETRUSTSTORE on
parse failure, missing `entries` array, or unsupported schemaVersion.
- validateEntry: requires non-empty {id, issuer, identityRegexp};
id must match /^[a-z0-9][a-z0-9._-]{0,63}$/i; identityRegexp must
parse as a JS RegExp (catches obvious garbage).
- addUserTrust: refuses ETRUSTSHADOW when an id collides with a
hardcoded entry. Idempotent on the same id (replace semantics).
- removeUserTrust: refuses ETRUSTHARDCODED for compiled-in entries;
returns false (no-op) for unknown user ids.
- listAllTrust: hardcoded entries first (source: 'hardcoded',
removable: false), then user entries (source: 'user', removable: true).
src/commands/trust.js (new):
- Subverbs: list / add <id> / remove <id> (rm alias). --json mode.
- add flags: --issuer <url>, --identity-regexp <regex>, optional
--publisher, --description.
- Exit codes: 0 success / 1 user error / 2 corrupted store on disk.
bin/pgserve-wrapper.cjs + src/cli-install.cjs:
- Add 'trust' to __installSubcommands allowlist (bypass bun probe; pure
node) and dispatch via the dynamic-import pattern used by `verify`
and `doctor`.
Tests (28 pass, 0 fail):
- tests/cosign/trust-store.test.js: read/write round-trip, invalid JSON
rejection, schemaVersion enforcement, mode 0600/0700 enforcement,
validateEntry happy + sad paths, hardcoded-shadow refusal,
hardcoded-removal refusal, listAllTrust ordering invariant.
- tests/cli/trust.test.js: parseFlags shape contract, dispatch error
paths (missing args, missing --issuer, unknown subverb).
Smoke test (HOME=$(mktemp -d) node bin/pgserve-wrapper.cjs trust list --json):
surfaces all 3 hardcoded entries, confirming the wrapper allowlist +
dispatch wiring.
Cohort: Singleton G3 verb 2 of 4. Verb 1 (doctor) shipped in PR #86;
gc + provision come next.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e47fa263c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| emit(false, null, 'pgserve trust: no entries'); | ||
| return 0; | ||
| } | ||
| const widthId = Math.max(2, ...entries.map((e) => e.id.length)); |
There was a problem hiding this comment.
Validate trust entries before formatting list output
This code assumes every entry has a string id, but readTrustStore() only checks that entries is an array, not that each element has the required fields. If ~/.pgserve/trust/identities.json contains malformed items (for example {"entries":[{}]} from manual edits or stale data), pgserve trust list throws a TypeError here and exits 1 instead of reporting a malformed store with exit 2 as documented; the command should reject bad entry shapes (or sanitize them) before reaching the formatter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request implements the 'pgserve trust' command, enabling users to manage a custom cosign trust store located at ~/.pgserve/trust/identities.json. The changes include CLI command dispatching, entry validation, and storage management. Review feedback identified several improvements: removing redundant process.exit calls in the CLI dispatcher, normalizing trust entry IDs to lowercase to ensure consistent case-insensitive behavior, implementing file locking to prevent race conditions during store updates, and ensuring consistent ID normalization in the removal logic.
| // pgserve singleton (v2.4) — wish Group 3, second read-only verb. | ||
| // `pgserve trust add/list/remove` manages the user-extensible cosign | ||
| // trust store at ~/.pgserve/trust/identities.json. Pure node. | ||
| return import('./commands/trust.js').then((mod) => mod.runTrust(args).then((code) => process.exit(code))); |
There was a problem hiding this comment.
The process.exit(code) call here is redundant and inconsistent with other subcommands like verify (line 1120). The pgserve-wrapper.cjs already handles the exit code from the returned promise (see bin/pgserve-wrapper.cjs line 65). Removing it allows the wrapper to manage the process lifecycle and any necessary cleanup or error reporting consistently.
return import('./commands/trust.js').then((mod) => mod.runTrust(args));
| if (!/^[a-z0-9][a-z0-9._-]{0,63}$/i.test(candidate.id)) { | ||
| throw new Error( | ||
| `trust entry id "${candidate.id}" must match /^[a-z0-9][a-z0-9._-]{0,63}$/i`, | ||
| ); | ||
| } | ||
| // Validate the identityRegexp parses as JS regex (cosign uses a similar | ||
| // RE2-ish dialect; this catches the obvious garbage while letting valid | ||
| // sigstore patterns through). | ||
| try { | ||
| new RegExp(candidate.identityRegexp); | ||
| } catch (err) { | ||
| throw new Error(`trust entry identityRegexp is not a valid regex: ${err.message}`); | ||
| } | ||
| return { | ||
| id: candidate.id, |
There was a problem hiding this comment.
The ID validation regex uses the /i flag, suggesting that IDs are intended to be case-insensitive. However, the IDs are currently stored with their original casing and compared using strict equality (===) in isHardcodedId and findIndex. This can lead to duplicate entries with different casing or accidental shadowing of hardcoded IDs if the casing doesn't match exactly. Normalizing IDs to lowercase during validation ensures consistent behavior.
| if (!/^[a-z0-9][a-z0-9._-]{0,63}$/i.test(candidate.id)) { | |
| throw new Error( | |
| `trust entry id "${candidate.id}" must match /^[a-z0-9][a-z0-9._-]{0,63}$/i`, | |
| ); | |
| } | |
| // Validate the identityRegexp parses as JS regex (cosign uses a similar | |
| // RE2-ish dialect; this catches the obvious garbage while letting valid | |
| // sigstore patterns through). | |
| try { | |
| new RegExp(candidate.identityRegexp); | |
| } catch (err) { | |
| throw new Error(`trust entry identityRegexp is not a valid regex: ${err.message}`); | |
| } | |
| return { | |
| id: candidate.id, | |
| if (!/^[a-z0-9][a-z0-9._-]{0,63}$/i.test(candidate.id)) { | |
| throw new Error( | |
| `trust entry id "${candidate.id}" must match /^[a-z0-9][a-z0-9._-]{0,63}$/i`, | |
| ); | |
| } | |
| // Validate the identityRegexp parses as JS regex (cosign uses a similar | |
| // RE2-ish dialect; this catches the obvious garbage while letting valid | |
| // sigstore patterns through). | |
| try { | |
| new RegExp(candidate.identityRegexp); | |
| } catch (err) { | |
| throw new Error(`trust entry identityRegexp is not a valid regex: ${err.message}`); | |
| } | |
| return { | |
| id: candidate.id.toLowerCase(), |
| export function addUserTrust(candidate, opts = {}) { | ||
| const entry = validateEntry(candidate); | ||
| if (isHardcodedId(entry.id)) { | ||
| const e = new Error( | ||
| `cannot add "${entry.id}" — id collides with a hardcoded trust root and would shadow it`, | ||
| ); | ||
| e.code = 'ETRUSTSHADOW'; | ||
| throw e; | ||
| } | ||
| const store = readTrustStore(opts); | ||
| const existing = store.entries.findIndex((x) => x.id === entry.id); | ||
| if (existing >= 0) { | ||
| store.entries[existing] = entry; | ||
| } else { | ||
| store.entries.push(entry); | ||
| } | ||
| writeTrustStore(store, opts); | ||
| return entry; | ||
| } |
There was a problem hiding this comment.
The addUserTrust function (and similarly removeUserTrust) performs a read-modify-write operation on the trust store file without any locking mechanism. This creates a race condition where concurrent executions of the trust command could lead to lost updates. Since this file is managed via a CLI and intended for user extensions, consider using a simple lockfile or fs.openSync with O_EXCL to ensure exclusive access during the update process.
| export function removeUserTrust(id, opts = {}) { | ||
| if (typeof id !== 'string' || id.length === 0) { | ||
| throw new Error('removeUserTrust: id must be a non-empty string'); | ||
| } | ||
| if (isHardcodedId(id)) { | ||
| const e = new Error(`cannot remove "${id}" — hardcoded trust roots are not removable`); | ||
| e.code = 'ETRUSTHARDCODED'; | ||
| throw e; | ||
| } | ||
| const store = readTrustStore(opts); | ||
| const before = store.entries.length; | ||
| store.entries = store.entries.filter((x) => x.id !== id); |
There was a problem hiding this comment.
To maintain consistency with the suggested ID normalization in validateEntry, the id argument in removeUserTrust should also be normalized to lowercase before performing checks and filtering. This ensures that a user can remove an entry regardless of the casing used in the command line argument.
| export function removeUserTrust(id, opts = {}) { | |
| if (typeof id !== 'string' || id.length === 0) { | |
| throw new Error('removeUserTrust: id must be a non-empty string'); | |
| } | |
| if (isHardcodedId(id)) { | |
| const e = new Error(`cannot remove "${id}" — hardcoded trust roots are not removable`); | |
| e.code = 'ETRUSTHARDCODED'; | |
| throw e; | |
| } | |
| const store = readTrustStore(opts); | |
| const before = store.entries.length; | |
| store.entries = store.entries.filter((x) => x.id !== id); | |
| export function removeUserTrust(id, opts = {}) { | |
| if (typeof id !== 'string' || id.length === 0) { | |
| throw new Error('removeUserTrust: id must be a non-empty string'); | |
| } | |
| const normalizedId = id.toLowerCase(); | |
| if (isHardcodedId(normalizedId)) { | |
| const e = new Error(`cannot remove "${normalizedId}" — hardcoded trust roots are not removable`); | |
| e.code = 'ETRUSTHARDCODED'; | |
| throw e; | |
| } | |
| const store = readTrustStore(opts); | |
| const before = store.entries.length; | |
| store.entries = store.entries.filter((x) => x.id !== normalizedId); |
PR #87 bot review feedback. Filtered low-value suggestions; fixed correctness gaps and one consistency nit: src/cosign/trust-store.js: - readTrustStore() now validates each entry's required fields (id, issuer, identityRegexp). Without this, a manually-edited identities.json containing `{"entries":[{}]}` slipped past the store-level guard and crashed the formatter in `pgserve trust list` with a generic TypeError exit 1 instead of the documented exit 2 "malformed store" path. Same ETRUSTSTORE error code so downstream callers can branch uniformly. - validateEntry() lowercases the id. The regex accepts upper-case (/i flag) but storage was case-sensitive — an entry typed "Foo" could shadow a hardcoded "foo" silently. Normalizing once on write keeps the hardcoded-shadow check simple and makes `trust remove FOO` idempotent with `trust add foo`. - isHardcodedId() lowercases both sides for symmetry. - removeUserTrust() lowercases the id arg so the operator's mixed- case input matches the lowercase entries on disk. src/cli-install.cjs: - Drop the redundant `process.exit(code)` from the trust dispatch. The wrapper at bin/pgserve-wrapper.cjs:60-69 already handles the numeric-exit-code case; matches the verify dispatch style so the wrapper, not the verb, owns process.exit. Eliminates the chance that an awaited cleanup task is killed before it runs. Tests (+6, total 34 pass / 0 fail): tests/cosign/trust-store.test.js - readTrustStore throws ETRUSTSTORE on entries[0]={} and entries[0] with empty id; accepts a populated entry. - validateEntry lowercases the id. - addUserTrust refuses ETRUSTSHADOW when the id collides with a hardcoded one in mixed case (AUTOMAGIK-genie-RELEASE collides with 'automagik-genie-release'). - removeUserTrust honors mixed-case input ('FORK' removes 'fork'). Filtered out (low-value): - "Add lockfile / O_EXCL for concurrent add/remove" — CLI is not typically called concurrently; adding lockfile state is more failure surface than it prevents in practice. Defer until we see real concurrent-edit corruption. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
# Conflicts: # bin/pgserve-wrapper.cjs
…5 more PR #93 bot reviews caught 7 findings (1 HIGH + 5 MEDIUM + 1 P2). All addressed: [HIGH — Gemini] Decision 7 contradicted reality. README + CHANGELOG + package.json all show `autopg` and `pgserve` as INTERCHANGEABLE CLI bins — both ship in package.json bin, both route to the same dispatcher. Earlier draft incorrectly claimed `autopg` was "never as a CLI verb." Flipped Decision 7 to match reality: both bins coexist; new verbs land for both; examples use `pgserve <verb>` for consistency with PRs #86–#92 but `autopg <verb>` is equivalent at runtime. No rename, no bin removal in this wish. [MEDIUM — Gemini] `~/.pgserve/` vs `~/.autopg/` inconsistency. Reality on main: both exist. autopg houses admin.json + settings.json (newer cohort schema); pgserve houses trust + audit (authored in PRs #87 + #90 before the split was rationalized). Added Decision 9 documenting the split + explicitly OUTING migration as scope of a future wish. G3's per-consumer state goes under `~/.autopg/<sanitized-slug>/...` to match autopg posture; trust/audit references stay at `~/.pgserve/` to match shipped code. [MEDIUM — Gemini] install.sh 404 doesn't help operators with bookmarked URLs. Replaced "let it 404" plan with a tiny ≤15-line shim at `install.sh` that prints a deprecation note + new URL on stderr and exits 0. Acceptance criterion added. [MEDIUM — Gemini] `<scope>` directory naming was ambiguous — `@demo/app` could be read as nested. Pinned the sanitization rule to `src/provision/db-naming.js#sanitizeSlug` (lowercase + non-alnum → '_' + collapse + trim), so `@demo/app` becomes `demo_app` (one flat dir). [MEDIUM — Gemini] State across 3 locations (autopg_meta + admin.json + manifest) without a stated source-of-truth invited desync. Pinned `autopg_meta` as authoritative; admin.json + manifest are derived caches; `pgserve doctor` reports divergences and `--fix` regenerates the caches from the table. Documented in the deliverable. [MEDIUM — Gemini] G5 smoke test used `npx pgserve@latest install` — that tests the PUBLISHED version, not the changes about to be released. Switched to local-build install (npm pack + npx local tarball, or worktree wrapper directly). [P2 — Codex] G4 validation `grep -E … docs/` passes a directory without `-r`, returning exit 2 ("Is a directory"). Added `-r` flag. `genie wish lint autopg-distribution-cutover-finalize`: clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Second of four CLI verbs from the
pgserve-singleton-no-proxywish (Group 3). Manages the user-extensible cosign trust store at~/.pgserve/trust/identities.json, layered on top of the hardcodedTRUSTED_IDENTITIESconstant.src/cosign/trust-store.js— atomic writer/reader, schema v1, mode 0700/0600, hardcoded-shadow refusal.src/commands/trust.js—list/add <id>/remove <id>subverbs with--jsonmode.doctorandverify).Files
Test plan
HOME=\$(mktemp -d) node bin/pgserve-wrapper.cjs trust list --jsonreturns the 3 hardcoded entries withsource: 'hardcoded', removable: false.addUserTrust({id: 'automagik-genie-release'})throwsETRUSTSHADOW.removeUserTrust('automagik-genie-release')throwsETRUSTHARDCODED.Cohort
Singleton G3 verb 2 of 4. Verb 1 (
doctor) is in PR #86. Verbs 3/4 (gc,provision) come next; each ~150-300 LOC, separately shippable.🤖 Generated with Claude Code