OpenCode custody: static-key slots served from the vault through an in-process auth proxy - #28
OpenCode custody: static-key slots served from the vault through an in-process auth proxy#28iceteaSA wants to merge 1 commit into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
f1f7f5b to
abe93be
Compare
Bytes unchanged (verified IDENTICAL x2); the previous pin named a commit off cortexkit/claustrum#28's history after its squash. Also: biome check in the pre-commit hook errored when every staged path was ignored, so any golden-only bump commit failed the hook. Pass --no-errors-on-unmatched.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 56 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/plugin.ts">
<violation number="1" location="packages/opencode/src/plugin.ts:117">
P2: When a malformed auth source contains many sentinel-like strings, the recovery scan processes every hit and creates one refusal provider per hit without a cap. Bound the number of scan hits and use a fail-closed fallback for the capped case so corrupted input cannot exhaust OpenCode during configuration.</violation>
</file>
<file name="scripts/spikes/opencode-config-fetch.sh">
<violation number="1" location="scripts/spikes/opencode-config-fetch.sh:236">
P2: The wire assertion can pass without the sentinel in `Authorization`. Match records by their actual `headers.authorization` value, not by sentinel text anywhere in the serialized request.</violation>
</file>
<file name="packages/opencode/src/freshness.ts">
<violation number="1" location="packages/opencode/src/freshness.ts:221">
P2: When `credential.get` returns `context_overflow`, this branch retries the unchanged OAuth `minTtlMs` after a 60-second backoff. Handle this class separately by reducing the request or making the slot unusable instead of retrying the same failing demand.</violation>
</file>
<file name="crates/credentials-module/tests/cli_admin.rs">
<violation number="1" location="crates/credentials-module/tests/cli_admin.rs:20">
P3: After moving tmp_root into this module, unique_temp_dir's doc comment still points at `cli_admin::tmp_root`, which no longer exists (it now lives in `common`). Update the reference to `tmp_root` so the cross-reference stays accurate.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Bytes unchanged (verified IDENTICAL x2); the previous pin named a commit off cortexkit/claustrum#28's history after its squash. Also: biome check in the pre-commit hook errored when every staged path was ignored, so any golden-only bump commit failed the hook. Pass --no-errors-on-unmatched.
|
Reviewed the admin/CLI surface, which is what you asked for. Verified the two structural claims at source rather than reading them, and found one gap in the handle lifecycle. The two claims hold"No new admin op, nothing secret-returning added to the admin surface." The 163 lines out of Reading material back through the ordinary consumer The gap: a minted handle can outlive the operation that minted itBoth directions share a shape — mint, then persist, then use: // restore
handle = mint_handle(global, &account.credential_id)?;
update_specific_handle(&mut handles, provider, &account.label, &handle)?;
write_and_verify_handles(&args.handle_file, &handles)?;
get_material(global, &handle)?
// migrate
None => mint_handle(global, &id)?,
if old_handle.is_none() {
update_handle(&mut handles, ...)?;
write_and_verify_handles(&args.handle_file, &handles)?;If The window is narrow — the mint has to succeed and a local file write has to fail — and it is not unrecoverable: the mint is in the audit chain, so What makes it worth fixing rather than documenting is that your The cheapest shape that closes it is the journal's own: write the intent before the effect. Record the id in the handle file (or the superseded list) before calling Two smaller notes
Your redacted What I have not reviewedThe TypeScript half, the plugin's fetch proxy, and the 40 automated findings. Two of those findings look like real defects to me on their face — the |
abe93be to
f28e419
Compare
There was a problem hiding this comment.
10 issues found across 58 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/client/src/errors.ts">
<violation number="1" location="packages/client/src/errors.ts:1">
P2: Because `ERROR_CLASS_WIRE_SET` is a mutable exported array, a JavaScript consumer can alter error decoding and action selection at runtime. Freeze the array (or use a private immutable membership set) before using it for wire validation.</violation>
</file>
<file name="packages/opencode/src/plugin.ts">
<violation number="1" location="packages/opencode/src/plugin.ts:367">
P1: When native LLM mode is enabled, this guard does not cover real-credential and other refusal paths, which only install a rejecting generic fetch. The native runtime bypasses that hook and can send the existing `options.apiKey`; apply the native-mode fail-closed handling before the tombstone split branch and ensure every refusal path blocks the native credential path.</violation>
</file>
<file name="scripts/spikes/opencode-config-fetch.sh">
<violation number="1" location="scripts/spikes/opencode-config-fetch.sh:9">
P2: When `OPENCODE_AUTH_CONTENT` is inherited, OpenCode ignores this fixture's `auth.json`, potentially consuming a real credential and making the assertions nondeterministic. Unset the auth-content override before launching OpenCode.</violation>
<violation number="2" location="scripts/spikes/opencode-config-fetch.sh:16">
P2: Deleting the directory returned by `mktemp` forfeits its ownership guarantee and creates a `/tmp` pathname race that can redirect the fixture's writes outside the temporary directory. Keep the directory created by `mktemp` and remove this second `rm -rf`.</violation>
</file>
<file name="packages/client/src/wire.ts">
<violation number="1" location="packages/client/src/wire.ts:74">
P2: When the handle is revoked or unknown, the daemon intentionally omits `record_version` from `credential.status`; this decoder rejects that valid response as `invalid_status`. Make `recordVersion` optional and validate it only when present.</violation>
</file>
<file name="scripts/accept-opencode-custody.sh">
<violation number="1" location="scripts/accept-opencode-custody.sh:53">
P2: When a post-migration check fails, cleanup preserves `$ROOT` containing plaintext API-key files for diagnostics. Scrub `$AUTH_FILE`, `$PRIVATE_ENTRY`, and other restored secret copies before retaining logs, or remove the scratch directory on failure.</violation>
<violation number="2" location="scripts/accept-opencode-custody.sh:56">
P2: When SIGINT or SIGTERM arrives between commands, `cleanup` can exit with the preceding zero status and report an interrupted acceptance as successful. Install signal traps that exit with 130/143 and let the EXIT trap perform cleanup.</violation>
</file>
<file name="scripts/gate.sh">
<violation number="1" location="scripts/gate.sh:125">
P2: The new `opencode-test-seam` arms in gate.sh have no counterparts in `.github/workflows/ci.yml`, violating this file's own invariant ("THE SET MUST MATCH CI... Every arm here corresponds to a step in .github/workflows/ci.yml; when a step is added there, add it here."). CI's "Clippy (conformance seams)" step still passes `--features kill9-test-seam,rotate-test-seam,login-test-seam,migration-tools` without `opencode-test-seam`, and no CI step runs the two new `run_expect 1` cli_opencode crash-cut tests — the `#[cfg(feature = "opencode-test-seam")]` tests at `crates/credentials-module/tests/cli_opencode.rs:1142` and `:1430` are compiled out of every CI cargo invocation. The seam-gated code in `opencode_migration.rs`/`opencode_accounts.rs` is therefore never compiled or linted in CI, and the tombstone-reread and handle-write crash-cut tests never run there; a regression in either would pass CI and only fail the local gate — the exact silent-divergence failure mode the gate header describes. Add `opencode-test-seam` to the CI clippy features and add CI steps mirroring the two `run_expect 1` arms (e.g. alongside the existing "Security-conformance suite" step).</violation>
</file>
<file name="packages/opencode/src/handles.ts">
<violation number="1" location="packages/opencode/src/handles.ts:161">
P1: A group-writable, non-sticky handle parent passes this check, so another group member can replace the capability file. Reject group or world-writable parents unless sticky-bit protection applies.</violation>
</file>
<file name="crates/credentials-core/src/oauth.rs">
<violation number="1" location="crates/credentials-core/src/oauth.rs:369">
P3: The second suggested command, `migrate-opencode --restore`, is not a valid invocation — the verb is `ck auth migrate-opencode --restore <provider>` (as written in this PR's operator-runbook change and design doc §5). An operator following the message would run a nonexistent command. Drop the bare form or write the full `ck auth migrate-opencode --restore <provider>`.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Re-trigger cubic
|
|
||
| // The native runtime reads `provider.options.apiKey` directly instead of this fetch | ||
| // seam. Its case-sensitive flag parser therefore gets an allowlist, not a best guess. | ||
| if (nativeLlmEnabled(process.env.OPENCODE_EXPERIMENTAL_NATIVE_LLM)) { |
There was a problem hiding this comment.
P1: When native LLM mode is enabled, this guard does not cover real-credential and other refusal paths, which only install a rejecting generic fetch. The native runtime bypasses that hook and can send the existing options.apiKey; apply the native-mode fail-closed handling before the tombstone split branch and ensure every refusal path blocks the native credential path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/plugin.ts, line 367:
<comment>When native LLM mode is enabled, this guard does not cover real-credential and other refusal paths, which only install a rejecting generic fetch. The native runtime bypasses that hook and can send the existing `options.apiKey`; apply the native-mode fail-closed handling before the tombstone split branch and ensure every refusal path blocks the native credential path.</comment>
<file context>
@@ -0,0 +1,434 @@
+
+ // The native runtime reads `provider.options.apiKey` directly instead of this fetch
+ // seam. Its case-sensitive flag parser therefore gets an allowlist, not a best guess.
+ if (nativeLlmEnabled(process.env.OPENCODE_EXPERIMENTAL_NATIVE_LLM)) {
+ const observed = process.env.OPENCODE_EXPERIMENTAL_NATIVE_LLM;
+ const refusal = new CustodyNativeRuntimeError(
</file context>
| if ((parent.mode & 0o002) !== 0 && (parent.mode & 0o1000) === 0) { | ||
| invalid("handle file parent is world-writable without sticky bit"); |
There was a problem hiding this comment.
P1: A group-writable, non-sticky handle parent passes this check, so another group member can replace the capability file. Reject group or world-writable parents unless sticky-bit protection applies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/handles.ts, line 161:
<comment>A group-writable, non-sticky handle parent passes this check, so another group member can replace the capability file. Reject group or world-writable parents unless sticky-bit protection applies.</comment>
<file context>
@@ -0,0 +1,196 @@
+ if (expectedUid !== undefined && parent.uid !== undefined && parent.uid !== expectedUid) {
+ invalid("handle file parent is not owned by the current uid");
+ }
+ if ((parent.mode & 0o002) !== 0 && (parent.mode & 0o1000) === 0) {
+ invalid("handle file parent is world-writable without sticky bit");
+ }
</file context>
| if ((parent.mode & 0o002) !== 0 && (parent.mode & 0o1000) === 0) { | |
| invalid("handle file parent is world-writable without sticky bit"); | |
| if ((parent.mode & 0o022) !== 0 && (parent.mode & 0o1000) === 0) { | |
| invalid("handle file parent is group/world-writable without sticky bit"); |
| @@ -0,0 +1,75 @@ | |||
| export const ERROR_CLASS_WIRE_SET = [ | |||
There was a problem hiding this comment.
P2: Because ERROR_CLASS_WIRE_SET is a mutable exported array, a JavaScript consumer can alter error decoding and action selection at runtime. Freeze the array (or use a private immutable membership set) before using it for wire validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/errors.ts, line 1:
<comment>Because `ERROR_CLASS_WIRE_SET` is a mutable exported array, a JavaScript consumer can alter error decoding and action selection at runtime. Freeze the array (or use a private immutable membership set) before using it for wire validation.</comment>
<file context>
@@ -0,0 +1,75 @@
+export const ERROR_CLASS_WIRE_SET = [
+ 'transient',
+ 'permanent',
</file context>
| } | ||
| ImportError::CustodyTombstone => write!( | ||
| f, | ||
| "refusing Claustrum tombstone material; run ck auth migrate-opencode or migrate-opencode --restore" |
There was a problem hiding this comment.
P3: The second suggested command, migrate-opencode --restore, is not a valid invocation — the verb is ck auth migrate-opencode --restore <provider> (as written in this PR's operator-runbook change and design doc §5). An operator following the message would run a nonexistent command. Drop the bare form or write the full ck auth migrate-opencode --restore <provider>.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-core/src/oauth.rs, line 369:
<comment>The second suggested command, `migrate-opencode --restore`, is not a valid invocation — the verb is `ck auth migrate-opencode --restore <provider>` (as written in this PR's operator-runbook change and design doc §5). An operator following the message would run a nonexistent command. Drop the bare form or write the full `ck auth migrate-opencode --restore <provider>`.</comment>
<file context>
@@ -350,6 +364,10 @@ impl std::fmt::Display for ImportError {
}
+ ImportError::CustodyTombstone => write!(
+ f,
+ "refusing Claustrum tombstone material; run ck auth migrate-opencode or migrate-opencode --restore"
+ ),
}
</file context>
| "refusing Claustrum tombstone material; run ck auth migrate-opencode or migrate-opencode --restore" | |
| "refusing Claustrum tombstone material; run ck auth migrate-opencode or ck auth migrate-opencode --restore <provider>" |
Bytes unchanged (verified IDENTICAL x2); the previous pin named a commit off cortexkit/claustrum#28's history after its squash. Also: biome check in the pre-commit hook errored when every staged path was ignored, so any golden-only bump commit failed the hook. Pass --no-errors-on-unmatched.
f28e419 to
898b131
Compare
|
Thanks for reading the admin surface at that level — the −163 in Handle lifecycle — fixed,
Tests under the Rebased onto The two bot findings you flagged ( Zeroize — agreed it should land as its own change against #29 rather than here; Branch is one commit on top of master; the review-round history is on |
There was a problem hiding this comment.
2 existing issues remain and 2 new issues found across 58 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/tombstone.ts">
<violation number="1" location="packages/opencode/src/tombstone.ts:66">
P2: When a legitimate API key begins with `claustrum-tombstone:v1:`, `carriesSentinel` classifies it as custody material and the plugin refuses the provider even without a handle. Reserve and reject this prefix during migration, or use an unambiguous sentinel encoding.</violation>
</file>
<file name="packages/opencode/package.json">
<violation number="1" location="packages/opencode/package.json:23">
P2: OpenCode typecheck fails on a fresh checkout because it resolves `@cortexkit/claustrum-client` types from the client package's `dist/index.d.ts`, but CI runs typecheck before build and `dist` is gitignored and never committed. Build the client before typechecking opencode (e.g. reorder CI to build before typecheck, or have the root typecheck build the client's dist first), otherwise every clean CI run errors on TS2307 for the import in `plugin.ts`.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 11 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
|
|
||
| function hasSentinel(value: unknown): boolean { | ||
| return typeof value === "string" && value.startsWith(TOMBSTONE_PREFIX); |
There was a problem hiding this comment.
P2: When a legitimate API key begins with claustrum-tombstone:v1:, carriesSentinel classifies it as custody material and the plugin refuses the provider even without a handle. Reserve and reject this prefix during migration, or use an unambiguous sentinel encoding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tombstone.ts, line 66:
<comment>When a legitimate API key begins with `claustrum-tombstone:v1:`, `carriesSentinel` classifies it as custody material and the plugin refuses the provider even without a handle. Reserve and reject this prefix during migration, or use an unambiguous sentinel encoding.</comment>
<file context>
@@ -0,0 +1,98 @@
+}
+
+function hasSentinel(value: unknown): boolean {
+ return typeof value === "string" && value.startsWith(TOMBSTONE_PREFIX);
+}
+
</file context>
Bytes unchanged (verified IDENTICAL x2); the previous pin named a commit off cortexkit/claustrum#28's history after its squash. Also: biome check in the pre-commit hook errored when every staged path was ignored, so any golden-only bump commit failed the hook. Pass --no-errors-on-unmatched.
898b131 to
f2e65bb
Compare
There was a problem hiding this comment.
5 existing issues remain and 5 new issues found across 58 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/client/src/secret-json.ts">
<violation number="1" location="packages/client/src/secret-json.ts:1">
P3: This new file is dead code: nothing imports `parseSecretJson` or `ConnectionJsonParseError`, and `index.ts` never re-exports them, so package consumers cannot reach it either. The client's connection-file parsing lives in `detect.ts` via `readConnectionFile`, not here. It also duplicates `packages/opencode/src/secret-json.ts`, the copy actually used by `handles.ts`/`plugin.ts`. Remove the file or wire it into the client and re-export it.</violation>
</file>
<file name="packages/client/src/identity.ts">
<violation number="1" location="packages/client/src/identity.ts:7">
P2: When `storagePath` contains `..` after a symlink, `resolve` collapses the path before `realpathSync`, so `storageFingerprint` hashes a different location than the storage path the filesystem opens. Canonicalize the original path with symlink-aware resolution, including an equivalent fallback for missing paths, rather than normalizing dot segments first.</violation>
</file>
<file name="packages/client/package.json">
<violation number="1" location="packages/client/package.json:6">
P2: This package is published as `"type": "module"` with an ESM-only `exports.import` target, but the tsc build (tsconfig.base.json uses `moduleResolution: "Bundler"`, `module: "ESNext"`) emits extensionless relative imports in dist (e.g. `export { ... } from './detect'` in dist/index.js). Node's ESM loader requires explicit `.js` extensions in relative imports, so any Node ESM consumer of the published `@cortexkit/claustrum-client` will fail with ERR_MODULE_NOT_FOUND. The in-repo consumer is bun (opencode bundles via `bun build`), which resolves extensionless imports, so the current flow works — but the published artifact is not Node-compatible. Either add `.js` extensions to the source relative imports or bundle the client before publishing.</violation>
</file>
<file name="packages/opencode/package.json">
<violation number="1" location="packages/opencode/package.json:22">
P2: The plugin bundle that OpenCode actually loads (`dist/opencode-plugin.js`, produced by `bun build` overwriting the tsc output) is never imported by any test; all tests import from `src/`. The gate only proves the bundle compiles, not that it runs, so a bundling/runtime failure (e.g. how `@cortexkit/claustrum-client` or its `@cortexkit/subc-client` dependency gets inlined) ships green. The same source is also emitted two ways — the bundled plugin versus the plain tsc `./server` and `"."` entries — and only the non-bundled path is tested. Add a test that dynamically imports the built `dist/opencode-plugin.js` (as `lifecycle.test.ts` does for `../opencode-plugin`) so the shipped artifact is exercised.</violation>
</file>
<file name="packages/opencode/README.md">
<violation number="1" location="packages/opencode/README.md:62">
P3: The maintenance census is already stale at this commit. Running the documented `grep -cE '^\s*(catch|} catch)|^\s*return[; ]|^\s*continue;|^\s*if \(' src/plugin.ts` returns 68, but the table's latest row records 64. The README states "A changed count without a matching sweep row is a review failure, not harmless churn," so this drift should be recorded as a new sweep row (68/27) rather than left mismatched.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 5 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| import type { BindIdentity } from '@cortexkit/subc-client' | ||
|
|
||
| export function storageFingerprint(storagePath: string): string { | ||
| const absolutePath = resolve(storagePath) |
There was a problem hiding this comment.
P2: When storagePath contains .. after a symlink, resolve collapses the path before realpathSync, so storageFingerprint hashes a different location than the storage path the filesystem opens. Canonicalize the original path with symlink-aware resolution, including an equivalent fallback for missing paths, rather than normalizing dot segments first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/client/src/identity.ts, line 7:
<comment>When `storagePath` contains `..` after a symlink, `resolve` collapses the path before `realpathSync`, so `storageFingerprint` hashes a different location than the storage path the filesystem opens. Canonicalize the original path with symlink-aware resolution, including an equivalent fallback for missing paths, rather than normalizing dot segments first.</comment>
<file context>
@@ -0,0 +1,30 @@
+import type { BindIdentity } from '@cortexkit/subc-client'
+
+export function storageFingerprint(storagePath: string): string {
+ const absolutePath = resolve(storagePath)
+ let canonicalPath: string
+ try {
</file context>
| | `eb034af` | 47/22 | 26/17 | | ||
| | `57ce561` | 58/22 | 26/17 | | ||
| | closing-wave worktree | 65/27 | 26/17 | | ||
| | custody review triage | 64 | 27 | |
There was a problem hiding this comment.
P3: The maintenance census is already stale at this commit. Running the documented grep -cE '^\s*(catch|} catch)|^\s*return[; ]|^\s*continue;|^\s*if \(' src/plugin.ts returns 68, but the table's latest row records 64. The README states "A changed count without a matching sweep row is a review failure, not harmless churn," so this drift should be recorded as a new sweep row (68/27) rather than left mismatched.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/README.md, line 62:
<comment>The maintenance census is already stale at this commit. Running the documented `grep -cE '^\s*(catch|} catch)|^\s*return[; ]|^\s*continue;|^\s*if \(' src/plugin.ts` returns 68, but the table's latest row records 64. The README states "A changed count without a matching sweep row is a review failure, not harmless churn," so this drift should be recorded as a new sweep row (68/27) rather than left mismatched.</comment>
<file context>
@@ -0,0 +1,67 @@
+| `eb034af` | 47/22 | 26/17 |
+| `57ce561` | 58/22 | 26/17 |
+| closing-wave worktree | 65/27 | 26/17 |
+| custody review triage | 64 | 27 |
+
+A changed count without a matching sweep row is a review failure, not harmless churn.
</file context>
f2e65bb to
98b46d4
Compare
There was a problem hiding this comment.
2 existing issues remain and 5 new issues found across 59 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/spikes/opencode-config-fetch.sh">
<violation number="1" location="scripts/spikes/opencode-config-fetch.sh:258">
P2: This spike can pass without the custom fetch owning the inference request: it only proves that the wrapper handled some request and that another request carried the sentinel. Record the request path or an ID and require the wrapper observation to correspond to `/chat/completions` or `/responses` before reporting success.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:134">
P2: The Windows branch is not platform-agnostic: `bun test packages/client` runs a test that creates a symbolic link without handling Windows link privileges, so this CI leg can fail before completing. Exclude that test on Windows or make the fixture use a Windows-supported link strategy.</violation>
</file>
<file name="packages/opencode/src/serve.ts">
<violation number="1" location="packages/opencode/src/serve.ts:141">
P2: When a handle file is replaced during an in-flight request, this closure can send material from the old handle after ownership has changed. `verifyOwnership` runs before the asynchronous freshness lookup, and revision changes do not abort the old account; revalidate ownership immediately before every upstream forward or make a revision change invalidate the in-flight attempt.</violation>
</file>
<file name="scripts/gate.sh">
<violation number="1" location="scripts/gate.sh:119">
P2: On Windows, this arm runs the Unix-only OpenCode tests that CI intentionally excludes, so the gate cannot pass on a supported platform. Select the same platform-agnostic test subset on Windows and keep `test:hermetic` for Unix hosts.</violation>
</file>
<file name="crates/credentials-module/tests/cli_opencode.rs">
<violation number="1" location="crates/credentials-module/tests/cli_opencode.rs:44">
P2: The fake-daemon threads only poll the shutdown channel inside the `listener.accept()` select, so a daemon blocked in `read_frame` on an accepted connection cannot be interrupted by `TestDaemon::drop`'s `join()`. If a CLI invocation ever keeps a connection open without sending the expected frame, the test binary hangs instead of failing. Wrap the per-frame reads in a `select!` against the shutdown channel (or add a timeout) so teardown can always terminate the thread.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 3 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
98b46d4 to
86ce2a0
Compare
There was a problem hiding this comment.
2 existing issues remain and 6 new issues found across 60 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/handles.ts">
<violation number="1" location="packages/opencode/src/handles.ts:211">
P3: This throw is unreachable because `readHandleSnapshot` returns or throws on every path through the preceding `try`; remove the dead code.</violation>
</file>
<file name="tsconfig.base.json">
<violation number="1" location="tsconfig.base.json:12">
P3: The shared base restricts auto-included global types to "bun" only, which excludes @types/node. The client package is a published, plain-Node-compatible library that imports node: built-ins and relies on Node globals, and opencode uses the NodeJS namespace; tying both to Bun's global types through the shared base bakes a bun-only type environment into a general-purpose library and hides any node-global the bun types don't cover. Since @types/bun is the only @types package in scope, dropping the types array from the shared base keeps bun globals auto-included without blocking node types.</violation>
</file>
<file name="packages/opencode/src/serve.ts">
<violation number="1" location="packages/opencode/src/serve.ts:273">
P2: A legitimate same-origin redirect chain longer than six hops is rejected, even though no origin boundary was crossed. Use a documented standard redirect limit and a distinct max-redirect error so valid chains and diagnostics are not conflated.</violation>
</file>
<file name="scripts/gate.sh">
<violation number="1" location="scripts/gate.sh:306">
P2: The release build is never used by the subsequent `validation_bypass_is_absent` test, which therefore checks the debug test binary instead of the shipped artifact. Export `CRED_CLI_BIN` to the release binary before running that assertion.</violation>
</file>
<file name="packages/opencode/package.json">
<violation number="1" location="packages/opencode/package.json:7">
P3: The "exports" map omits a "./package.json" subpath, so the manifest can no longer be resolved by subpath once exports is present. Add an entry like "./package.json": "./package.json" so tooling that reads the package manifest keeps working.</violation>
</file>
<file name="packages/client/src/tests/client.test.ts">
<violation number="1" location="packages/client/src/tests/client.test.ts:190">
P2: This discovery test is not isolated and can fail or give false coverage on any machine that matches the project's own runtime state. getDefaultClaustrumConnectionPath() resolves the production-home branch through userInfo().homedir (getpwuid), so setting process.env.HOME = homeDir has no effect there: if the real home contains ~/.local/share/cortexkit/run/subc-connection.json, or if any other `subc-*.connection.json` exists in the shared OS tmpdir (e.g. a live daemon, exactly what this daemon writes), matches.length > 1 makes the glob return undefined and the test fails. The HOME override also means the production-home fallback the test claims to mirror is never actually exercised. Isolate the test by pointing XDG_RUNTIME_DIR and the home lookup at controlled dirs and cleaning the temp-glob scope, or scope the assertion to the controlled inputs rather than the shared tmpdir.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| options.log?.error({ provider: options.provider, errorClass: refusal.name, errorMessage: refusal.message }); | ||
| throw refusal; | ||
| } | ||
| if (hop === 5) { |
There was a problem hiding this comment.
P2: A legitimate same-origin redirect chain longer than six hops is rejected, even though no origin boundary was crossed. Use a documented standard redirect limit and a distinct max-redirect error so valid chains and diagnostics are not conflated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/serve.ts, line 273:
<comment>A legitimate same-origin redirect chain longer than six hops is rejected, even though no origin boundary was crossed. Use a documented standard redirect limit and a distinct max-redirect error so valid chains and diagnostics are not conflated.</comment>
<file context>
@@ -0,0 +1,294 @@
+ options.log?.error({ provider: options.provider, errorClass: refusal.name, errorMessage: refusal.message });
+ throw refusal;
+ }
+ if (hop === 5) {
+ await discard(response);
+ const refusal = new CustodyRedirectRefusedError(options.provider, fromOrigin, next.origin);
</file context>
| "type": "module", | ||
| "main": "./dist/index.js", | ||
| "types": "./dist/index.d.ts", | ||
| "exports": { |
There was a problem hiding this comment.
P3: The "exports" map omits a "./package.json" subpath, so the manifest can no longer be resolved by subpath once exports is present. Add an entry like "./package.json": "./package.json" so tooling that reads the package manifest keeps working.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/package.json, line 7:
<comment>The "exports" map omits a "./package.json" subpath, so the manifest can no longer be resolved by subpath once exports is present. Add an entry like "./package.json": "./package.json" so tooling that reads the package manifest keeps working.</comment>
<file context>
@@ -0,0 +1,31 @@
+ "type": "module",
+ "main": "./dist/index.js",
+ "types": "./dist/index.d.ts",
+ "exports": {
+ ".": {
+ "types": "./dist/index.d.ts",
</file context>
| "skipLibCheck": true, | ||
| "resolveJsonModule": true, | ||
| "declaration": true, | ||
| "types": ["bun"] |
There was a problem hiding this comment.
P3: The shared base restricts auto-included global types to "bun" only, which excludes @types/node. The client package is a published, plain-Node-compatible library that imports node: built-ins and relies on Node globals, and opencode uses the NodeJS namespace; tying both to Bun's global types through the shared base bakes a bun-only type environment into a general-purpose library and hides any node-global the bun types don't cover. Since @types/bun is the only @types package in scope, dropping the types array from the shared base keeps bun globals auto-included without blocking node types.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tsconfig.base.json, line 12:
<comment>The shared base restricts auto-included global types to "bun" only, which excludes @types/node. The client package is a published, plain-Node-compatible library that imports node: built-ins and relies on Node globals, and opencode uses the NodeJS namespace; tying both to Bun's global types through the shared base bakes a bun-only type environment into a general-purpose library and hides any node-global the bun types don't cover. Since @types/bun is the only @types package in scope, dropping the types array from the shared base keeps bun globals auto-included without blocking node types.</comment>
<file context>
@@ -0,0 +1,14 @@
+ "skipLibCheck": true,
+ "resolveJsonModule": true,
+ "declaration": true,
+ "types": ["bun"]
+ }
+}
</file context>
86ce2a0 to
0e9dee7
Compare
|
Verified the fix at The two-helper split is the part I would have got wrong if I had written it myself: one keeps the handle on success and one never does, and collapsing them into a single "always revoke" helper would have broken the migrate path silently. One gap, and it is in a guard of mine rather than in your fixThe seam is a cargo feature, and this repo's standing rule for test escape hatches is Your Four hatches, one asserted. The scan is honest about its own subject and blind to everything added after it was written, which is the failure mode I have been chasing across this repo all week: a guard whose population is hardcoded stops being a guard for whatever arrives next, and reads as covering it. Adding the four strings to that test closes it and is the smallest correct change. What I would rather have — and would take as a follow-up rather than a condition — is the population derived: scan the source for Not blocking on the derived version. Blocking on the four strings, because the alternative is that the next hatch inherits a green check that never looked at it. Closed since my first passThe (Re-checked at |
Live on the deploy box — 2026-09-02 18:45–18:59ZRan the operator procedure from this branch ( Migration (
After: the three Proof — plugin registered from with Two things learned that are not in the branch:
Sessions started before the migration keep working on their in-memory key until restarted; |
|
Correction to the comment above — two of the three migrations were reversed at 19:10Z. The migration premise "model traffic is unaffected; only tok's balance/quota readers see the tombstone" was wrong on this box: the
Doc consequence for the operator procedure: the precondition step must enumerate every reader of |
…h proxy
OpenCode keeps provider API keys in auth.json and reads them per request.
This moves custody of those keys into the vault: the key is replaced on
disk by a non-secret tombstone (`claustrum-tombstone:v1:<provider>`), a
capability handle lives in a mode-600 handle file, and a plugin on
OpenCode's `config` hook injects `{apiKey: <tombstone>, fetch}` so the
request-time closure substitutes the served credential wherever the SDK
placed the sentinel. The plugin holds zero provider knowledge; ownership
is the conjunction of a tombstone in auth.json and a `serve` claim in the
handle file.
Rust (ck-auth):
- `migrate-opencode` (dry-run, `--provider`, `--restore`, `--replace`,
`--force-shape`) and `opencode-account add|remove|list`, online against
the daemon or offline on the lease; every write is temp+fsync+rename at
mode 0600 in a checked parent; a `superseded` journal on the handle
file makes a crash between tombstone write and revoke converge without
rotating handles.
- Shared `route_client.rs` transport and a capability-only
`credential_client.rs`; no new admin op and no secret-returning surface.
- Rust handle-file validation mirrors the TS parser rule for rule
(provider/label charset, `ckh_` base64url handles, `superseded`
entries); provider ids never `__proto__`/`constructor`/`prototype`.
- `opencode-provider-shapes.json`: providers whose key leaves OpenCode's
fetch seam (env copy, discovery, metadata) are refused at migrate time
with the shape, the reason, the source citation, and the consequence of
forcing; `ck auth usable` warns on an existing tombstone whose shape
moved. Data with provenance (anomalyco/opencode@dc4449df0d, method and
its edge stated), maintained by delta per OpenCode base update.
- Vault import refuses a tombstone as credential material.
- `opencode-test-seam` feature (two env seams) for crash-cut tests;
compiles nothing into release, and gate.sh ends on a default build.
TypeScript:
- `@cortexkit/claustrum-client`: detect, wire, identity, reconnect and
error classes extracted from anthropic-auth; policy-free.
- `@cortexkit/claustrum-opencode`: seven-cell ownership table (split
custody installs a REFUSING fetch; orphan injects nothing), ordered
per-account failover with 401 reporting fenced on the served
record_version, 429/402 cooldowns, manual same-origin-only redirects,
bounded warm and a 60 s oauth tick for idle-account custody, redacted
logging with canary tests, and a lifecycle suite driven through the
exported plugin (`dist/opencode-plugin.js`, v1 `{id, server}` shape).
- Fail-closed on every path that sees a tombstone: unreadable or oversized
files, unrecognised native-runtime flag values, absent handle file.
auth.json past the parse cap is scanned for sentinels with each hit
becoming a refusal directly. `readAuth` mirrors `Auth.all` precedence
including its error behaviour.
- A containment property test asserts the plugin's refusal set is a
superset of every provider OpenCode would load with a sentinel in it,
over auth-source × handle-state rows, against a reference model of the
host derived from its source (not from the plugin) and proven so by a
two-sided mutation.
Gates: bash scripts/gate.sh with bun arms (install, typecheck, build,
test) ahead of the cargo arms; workspace floor measured in the profile
the gate runs. Live acceptance in scripts/accept-opencode-custody.sh:
migrate, serve a real model call through the vault, refuse a hand-restored
key as split custody, restore — against the running daemon in a scratch
XDG home.
Review response folded in: provider ids are validated where they are
MATERIALIZED (one function at every cfg.provider write; auth.json keys
were a third, unvalidated site); parse errors on secret-bearing files
(auth.json, the handle file, the daemon connection file) never echo
parser text — Bun's SyntaxError quotes the input token, which put a
handle verbatim into a thrown message; catch sites log a fixed code and
the error name, never the message; the handle file is read once through
an O_NOFOLLOW descriptor, fstat-validated, revision from the same bytes;
query substitution percent-encodes the material and leaves untouched
parameters byte-identical; 303-to-GET drops the RFC 9110 representation
headers; connection-file discovery mirrors the daemon's order; the
acceptance script arms rollback before migrating.
Handle lifecycle (maintainer finding): a minted handle never outlives
the operation that minted it. `mint_then_persist` revokes the handle if
its file write fails; `with_scoped_handle` revokes a comparison-only
handle on every exit; both name the credential id and the closing
commands if the revoke itself fails. No bare mint remains outside the
two helpers. Rebased onto master's redacted-Debug change (0679dea).
Design: docs/opencode-custody-design.md. Follow-ups: cortexkit#29.
The TypeScript suite runs hermetically (no daemon, no HOME) and that run is the gate
in scripts/gate.sh and both CI jobs; migrate-opencode and opencode-account add refuse
keys carrying the reserved tombstone prefix; the auth.json read is single-descriptor
bounded.
A rejection from a stale handle revision cannot poison the replacement slot, a stalled
get expires instead of pinning the slot, descriptor reads are bounded to the cap on the
bytes actually read, percent-encoded sentinels match case-insensitively, the client
ships Node-loadable ESM, and the hermetic suite runs on every CI leg including Windows.
Connection discovery mirrors the daemon tier-for-tier and refuses an ambiguous match;
the serve path renders error names only, with a structured code for callers and a canary
covering the substitution-failure arm; a stalled tick warm expires like a request warm.
0e9dee7 to
2c39e3d
Compare
|
Closed at Blocking half: the release scan now asserts all five seam strings absent. Preferred half, delivered: the population is derived — the test scans Your standing rule, adopted: the four seams moved from the Red-checks: (a) release build with Also since your first pass: three static-key providers were migrated on the deploy box and one request answered through each with only the tombstone in |
1 participant
Implements the plugin design from #17, static-key slice: OpenCode
type:"api"providers (deepseek, synthetic, minimax-coding-plan, …) get their keys moved into the vault, theauth.jsonentry becomes a non-secret tombstone, and a plugin serves the real key per request from a capability handle.How it works (source-verified on stock OpenCode 1.18.25; the spike in
scripts/spikes/is the executed proof):confighook by injecting{ apiKey: sentinel, fetch }intocfg.provider[id].options. Config provider options are re-applied after everyauth.loader(provider.ts:1643-1650), so the injectedfetchalso replaces a shipped plugin's — no fork, no per-provider knowledge.fetchis an in-process auth proxy: it rewrites every header value and URL query value equal to the sentinel with the vault-served material, forwards, and observes the response. 401 →report_auth_failurewith the record_version that was actually served, then the next account; 429/402 → cooldown, next; 403/5xx → returned as-is (not a credential verdict).auth.json(absence of a local credential) ANDserve: "opencode-claustrum"in the handle file (who owns the slot). The seven cells are explicit; a real key sitting behind an owned slot gets a refusing fetch, never either copy.apikey:<provider>:<label>, handle-file order is failover order.x-api-keynever leaves the configured origin.CLI (
ck auth, admin gate, key never on argv):migrate-opencode [--dry-run|--replace|--restore <provider>] [--provider …]... [--serve-by …]andopencode-account add|remove|list. Idempotent nine-step transaction; the handle-filesupersededjournal makes a crash between tombstone and revoke converge on rerun. Compare/restore read material through the ordinary consumercredential.getwith a handle the CLI minted — no new admin op, nothing secret-returning added to the admin surface.Packages (bun workspace beside the cargo one; CI gets
setup-bun):@cortexkit/claustrum-client— detect / identity / wire / errors extracted from anthropic-auth's soak-proven client, policy-free;@cortexkit/opencode-claustrum— the plugin.packages/opencode/golden/{tombstone,handles}.jsonare the single cross-language source; Rust pins them withinclude_str!, TS imports them, anthropic-auth vendors them by SHA (cortexkit/anthropic-auth#182).Verification:
scripts/gate.shgreen (workspace floor 501 + 57 bun tests + two crash-seam arms; release binary is seam-free).scripts/accept-opencode-custody.shran against the live daemon on a scratch XDG home: migrate → real routed request served through the vault → hand-restored key refused as split custody (sentinel never sent) →--restoreround-trips the key and revokes the handle;oauth:anthropic*and legacyapikey:*byte-identical throughout. Two defects only the live arm found (OpenCode treats every export of a plugin module as a plugin; "inject nothing" on the split cell let stock OpenCode serve the local key) are fixed with tests.Out of scope, seams left: OAuth main slots (xai is proven to take the same
fetch; anthropic stays with anthropic-auth byserve), Claude Code / Codex.Design doc: docs/opencode-custody-design.md. Draft until the maintainer has had a look at the admin/CLI surface.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Moves supported OpenCode
type:"api"credentials out ofauth.jsonand into the vault. The auth entry becomes a non-secret tombstone, and@cortexkit/opencode-claustrumserves live credentials through an in-processfetchproxy; existing installations need migration, while unsupported provider shapes and split custody fail closed.CLI and safety
ck auth migrate-opencodeandck auth opencode-account add|remove|listwith dry runs, restore/replace, ordered failover, and crash-safe handle cleanup.OPENCODE_AUTH_CONTENT, and rejects tombstones as importable credentials.ck auth usablewarnings when a tombstoned shape changes.Verification
@cortexkit/claustrum-client, Bun builds and tests, Rust CLI and crash-seam coverage, hermetic CI coverage on Windows, and live migration, serving, split-custody, and restore acceptance tests.anthropic-auth; useck auth migrate-opencode --restore <provider>to return a credential toauth.jsonand revoke its handle.Written for commit 2c39e3d. Summary will update on new commits.