opencode: scope a manifest credential id to its provider segment

[iceteaSA]

parseHandleFile accepted any non-empty string as a credential_id, so a binding carrying oauth:openai parsed cleanly inside an anthropic provider block. Every tenant reading the shared manifest would have honoured it.

Two peer tenants (anthropic-auth, openai-auth) found the same hole in their own parsers independently and proposed the same fix. That is the argument for landing it in the shared parser rather than three times in three repos.

The rule

Segment 2 of the credential id must be the provider block it sits in.

provider "anthropic"  credential_id "oauth:anthropic"             ok   (main, 2-segment)
provider "anthropic"  credential_id "oauth:anthropic:work-alt"    ok   (fallback, 3-segment)
provider "openai"     credential_id "chatgpt:openai"              ok   (different kind)
provider "deepseek"   credential_id "apikey:deepseek:main"        ok   (static key)
provider "anthropic"  credential_id "oauth:openai"                REFUSED

Why only segment 2

Both exclusions are deliberate rather than unfinished, and both were checked against the live vault before choosing:

• Segment 1 (kind) is an open set. oauth:, chatgpt:, antigravity:, apikey: are all in use in this deployment today. A kind allowlist would refuse real ids and would need editing every time a provider family is added.
• Segment 3+ (the label) is operator-chosen and optional. Main is the 2-segment oauth:anthropic; fallbacks are 3-segment. Requiring a label rejects main, and constraining its contents rejects operator naming.

An earlier draft of this rule proposed oauth:P or oauth:P: — that kind-prefixed form would have refused four of the five credentials this vault serves.

Where it landed

The predicate moved while this was in the queue: packages/opencode/src/handles.ts is now a re-export shim over @cortexkit/claustrum-client, so the fix belongs in the shared parser. Both tenants inherit it from one place.

Verification

Mutation-proved rather than asserted. Restoring the weak predicate (exact-occurrence-count checked before substituting, so the mutation cannot land on a different line):

(fail) custody wire contracts > rejects a credential id scoped to another provider
(fail) custody wire contracts > rejects a credential id without a provider segment

Restored byte-identically (cmp), green again. Full scripts/gate.sh exit 0 on the rebuilt tree; hermetic suite 197 pass.

Two-way revert sweep against 6817148: of the 1770 distinct lines master added since this branch's original base, exactly one is deleted here — the weak predicate being replaced.

Rebuilt on master rather than rebased: #33's squash reformatted the file this branch patched, and the implementation had since moved packages, so a rebase would have resolved a conflict between a style migration and a relocation. Applying the logical change to its current home is the smaller, checkable operation.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Scopes a manifest credential_id to the provider block it sits in. Previously any non-empty ID was accepted, so a binding like oauth:openai inside an anthropic block parsed cleanly and every tenant reading the shared manifest honored it; now the second colon-separated segment must match the provider.

• Enforces the same predicate in the Rust writer and the TypeScript reader, so ck auth can no longer originate a row the reader refuses.
• Keeps the kind segment open and the label segment optional and operator-chosen.
• Rejects empty segments and case-mismatched provider segments.
• Update manifests with mismatched provider segments before deploying this change.

^{Written for commit 06badee. Summary will update on new commits.}

[cubic-dev-ai]

1 issue found across 4 files

_{You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

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="docs/opencode-custody-design.md">

<violation number="1" location="docs/opencode-custody-design.md:49">
P2: These rows promise resolution even though this parser only validates shape and never checks vault existence. Change the expectation to `MUST PARSE` (or `MUST ACCEPT`) so fixtures and consumers do not treat a syntactically valid, operator-chosen ID as a resolvable credential.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: These rows promise resolution even though this parser only validates shape and never checks vault existence. Change the expectation to MUST PARSE (or MUST ACCEPT) so fixtures and consumers do not treat a syntactically valid, operator-chosen ID as a resolvable credential.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At docs/opencode-custody-design.md, line 49:

<comment>These rows promise resolution even though this parser only validates shape and never checks vault existence. Change the expectation to `MUST PARSE` (or `MUST ACCEPT`) so fixtures and consumers do not treat a syntactically valid, operator-chosen ID as a resolvable credential.</comment>

<file context>
@@ -34,6 +34,48 @@ provider with in-request failover for providers the generic plugin serves.
+
+| expectation | provider | account label | `credential_id` | reason |
+|---|---|---|---|---|
+| MUST RESOLVE | `openai` | `main` | `chatgpt:openai` | A kind-prefix rule would reject this live OpenAI shape. |
+| MUST RESOLVE | `google` | `main` | `antigravity:google` | A kind-prefix rule would reject this live Google shape. |
+| MUST RESOLVE | `anthropic` | `work-alt` | `oauth:anthropic:something-else` | The label must not be derived or consulted. |
</file context>

Suggested change

	| MUST RESOLVE | `openai` | `main` | `chatgpt:openai` | A kind-prefix rule would reject this live OpenAI shape. |
	| MUST RESOLVE | `google` | `main` | `antigravity:google` | A kind-prefix rule would reject this live Google shape. |
	| MUST RESOLVE | `anthropic` | `work-alt` | `oauth:anthropic:something-else` | The label must not be derived or consulted. |
	| MUST PARSE | `openai` | `main` | `chatgpt:openai` | A kind-prefix rule would reject this live OpenAI shape. |
	| MUST PARSE | `google` | `main` | `antigravity:google` | A kind-prefix rule would reject this live Google shape. |
	| MUST PARSE | `anthropic` | `work-alt` | `oauth:anthropic:something-else` | The label must not be derived or consulted. |

[iceteaSA]

A peer tenant re-derived their vendored copy of this predicate against the diff and asked whether an absent credential_id reaches .split(':') and throws, since the line this replaces (!account.credential_id) looks like it was the absent-field guard.

It does not. isAccount (handles.ts:38) requires typeof account.credential_id === 'string', and item.accounts.every(isAccount) at :70 runs before the loop reaches :91. Exercised rather than read:

credential_id ABSENT      -> HandleFileValidationError: provider 0 has invalid accounts
credential_id null        -> HandleFileValidationError: provider 0 has invalid accounts
credential_id number      -> HandleFileValidationError: provider 0 has invalid accounts
credential_id empty str   -> HandleFileValidationError: ... has invalid credential id
cross-provider            -> HandleFileValidationError: ... has invalid credential id
valid 2-segment           -> ACCEPTED

The empty-string row is worth noting since it changes route: "" passes isAccount as a string, reaches the split, and "".split(':')[1] is undefined !== "anthropic". Same rejection as before, now via the new predicate rather than the emptiness check — so removing the old !account.credential_id does not open a gap.

Their question also surfaced something this PR does not change but should be recorded: a malformed row in any provider block refuses the entire document, so one tenant's bad entry denies the others. Filed separately as #45 — it is the file-wide invalid() convention rather than anything introduced here, and it wants its own decision with both tenants party to it rather than riding a predicate fix.

[iceteaSA]

Tightened: no segment of a credential_id may be empty.

The provider-segment rule alone accepted two ids that name credentials which cannot exist, because a position-1 check ignores the rest of the string:

":anthropic:x"      -> ["", "anthropic", "x"]     seg[1] == "anthropic"  -> was ACCEPTED
"oauth:anthropic:"  -> ["oauth", "anthropic", ""] seg[1] == "anthropic"  -> was ACCEPTED
"oauth::anthropic"  -> ["oauth", "", "anthropic"] seg[1] == ""           -> rejected

Neither of the accepted two is a cross-provider smuggle — segment 2 still fences the provider — but both defer a guaranteed resolve-time failure past the door. Under custody that matters more than it used to: a resolve-time failure on a tombstoned account is a dark route, not a refused row.

The third line is the other half of the argument. Empty-at-position-1 rejected while empty-at-0 and empty-at-2 passed, purely because of where the check indexed. Nobody designed that, and the next reader to meet it either "fixes" it unilaterally or works around it.

This was surfaced by the anthropic-auth tenant re-deriving their vendored copy of this predicate against the diff, and running the same inputs through both parsers. Their table matched the prediction exactly — no verdict divergence on any of the eight edge cases, including these two — so the rule as both sides had written it admitted them identically. The tightening is agreed on both sides and mirrored there, which makes it chosen rather than defaulted; the point of #45 is that the previous state was two independent defaults nobody had compared.

Checked before tightening, since a rule that breaks production is worse than the gap it closes:

• all seven live vault credential ids pass (2- and 3-segment, four distinct kinds)
• the live opencode-handles.json on this host still parses — 4 providers, 5 accounts

Mutation-proved, unlike the emptiness guard noted earlier: removing the clause reddens both new arms (rejects a credential id with an empty kind segment, rejects a credential id with an empty label segment). Restored byte-identically, gate exit 0, revert sweep clean.

Explicitly not tightened, so these are chosen too: unknown keys inside an account, and extra top-level keys, both still parse. A strict-key parser makes every future field addition a flag-day for the oldest tenant in a shared manifest. An empty segment can never become meaningful; an unknown key can.

[iceteaSA]

Extended to the Rust validator, because leaving it out made this PR a divergence rather than a fix.

A peer tenant checked their writer against this tightening rather than only their reader, on the grounds that I parse what they write. That sent me to mine, and crates/credentials-module/src/bin/cli_support/opencode_files.rs:872 still read:

if account.credential_id.is_empty() {

The Rust side never got the provider-segment rule at all — not this PR's emptiness tightening, and not the original scoping. And validate_handle_file is called from write_handle_file_for_tenant and verify_handle_written, so it is the write path: ck auth would happily write oauth:openai into an anthropic block, and the TypeScript reader would then refuse the whole document. Two implementations of one predicate in one repo, diverging because the fix landed on the reader.

Now matched: segment 2 must be the provider block, no segment may be empty, same rule and same rejection message.

Arms

Three new Rust tests in cli_opencode.rs, mutation-proved by removing the clauses:

a_handle_file_with_a_cross_provider_credential_id_is_refused   ... FAILED
a_handle_file_with_an_empty_credential_id_segment_is_refused   ... FAILED

The third is a positive control against the real deployment — all five live credential-id shapes (apikey:deepseek:main, oauth:anthropic, oauth:anthropic:work-alt, chatgpt:openai, antigravity:google) must still parse. Proved non-vacuous by over-tightening the rule to require four segments, which reddens it. A tightening that refuses a live id is worse than the gap it closes, and an acceptance test that passes because the validator accepts everything proves nothing.

Workspace floor measured on this tree and raised 610 → 613, red-checked both ways (614 fails naming the shortfall, 613 passes).

A regression this PR introduces, stated plainly

Before this change, oauth:anthropic: was a useless row — accepted, resolving to nothing, that one account dark and every other tenant fine. After it, the row is a whole-file poison pill, because a rejection refuses the entire document (#45).

So this strictly increases the blast radius of that input in exchange for catching it earlier. That trade is only correct once the radius is bounded, which means #45 is not a follow-up to this PR — it is this PR's precondition for being safe, and I had them in the wrong order. Recording it here rather than leaving it as a judgement call: the argument for per-row refusal is now "strict validation requires a bounded blast radius", not "nicer error messages".

Not holding this PR for it. No writer on either side emits an empty segment today, both live manifests are clean, and the peer is mirroring the rule at their writer first for exactly this reason.

[cubic-dev-ai]

2 issues found across 2 reviewed files. 1 file intentionally excluded from review.

_{You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

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="crates/credentials-module/tests/cli_opencode.rs">

<violation number="1" location="crates/credentials-module/tests/cli_opencode.rs:362">
P2: These tests do not exercise the write path described in their comment: every case only parses a raw file through `read_handle_file`. Add an invalid in-memory `HandleFile` write assertion so removing `validate_handle_file` from `write_handle_file_for_tenant` cannot go undetected.</violation>
</file>

<file name="scripts/gate.sh">

<violation number="1" location="scripts/gate.sh:257">
P2: The floor bump to 613 leaves the measurement note immediately above it stale: it still claims "The current measured total is 608 (debug profile, the same command this arm runs)." Taken literally, a floor above the documented measured total means this arm fails every run (ran 608, expected at least 613). Per this file's own discipline the floor is the freshly measured total on the final tree, so raising it should update that sentence to the new measured number. Update the note to the actual measurement that produced 613.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: These tests do not exercise the write path described in their comment: every case only parses a raw file through read_handle_file. Add an invalid in-memory HandleFile write assertion so removing validate_handle_file from write_handle_file_for_tenant cannot go undetected.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/tests/cli_opencode.rs, line 362:

<comment>These tests do not exercise the write path described in their comment: every case only parses a raw file through `read_handle_file`. Add an invalid in-memory `HandleFile` write assertion so removing `validate_handle_file` from `write_handle_file_for_tenant` cannot go undetected.</comment>

<file context>
@@ -349,6 +349,71 @@ fn a_handle_file_with_an_empty_credential_id_is_refused() {
+/// than left to look like an independent local rule.
+#[test]
+fn a_handle_file_with_a_cross_provider_credential_id_is_refused() {
+    let err = read_raw_handle_fixture(
+        "cross-provider-credential-id",
+        r#"{"version":1,"providers":[{"provider":"deepseek","shape":"api","serve":"opencode-claustrum","accounts":[{"label":"main","handle":"ckh_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","credential_id":"oauth:anthropic"}]}]}"#,
</file context>

[cubic-dev-ai]

P2: The floor bump to 613 leaves the measurement note immediately above it stale: it still claims "The current measured total is 608 (debug profile, the same command this arm runs)." Taken literally, a floor above the documented measured total means this arm fails every run (ran 608, expected at least 613). Per this file's own discipline the floor is the freshly measured total on the final tree, so raising it should update that sentence to the new measured number. Update the note to the actual measurement that produced 613.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At scripts/gate.sh, line 257:

<comment>The floor bump to 613 leaves the measurement note immediately above it stale: it still claims "The current measured total is 608 (debug profile, the same command this arm runs)." Taken literally, a floor above the documented measured total means this arm fails every run (ran 608, expected at least 613). Per this file's own discipline the floor is the freshly measured total on the final tree, so raising it should update that sentence to the new measured number. Update the note to the actual measurement that produced 613.</comment>

<file context>
@@ -254,7 +254,7 @@ stream and pass the arm without ever seeing it skip."
 # Raise this when tests are added. A failure here is normally that, not a defect --
 # but it should be a deliberate edit rather than a number nobody revisits.
-run_expect 610 "workspace unit + integration" \
+run_expect 613 "workspace unit + integration" \
   cargo test --locked --workspace --features credentials-core/test-support
 
</file context>

[iceteaSA]

Both review findings were valid. Fixed in 06badee.

P2 (cli_opencode.rs): the comment claimed write-path coverage the arms did not have

Correct, and it is the exact failure the comment itself was warning about. The doc block said "this is a WRITER" while every arm went through read_handle_file. Deleting validate_handle_file from write_handle_file_for_tenant would have left all of them green.

Added an_invalid_handle_file_is_refused_at_the_write_path, which constructs an invalid HandleFile in memory and calls write_handle_file — no deserialization step between the value and the disk. It asserts the refusal, that no file is left behind (a writer that validates after creating the file leaves a partial artifact, and "it returned an error" does not distinguish those), and carries a positive control so the refusal is the predicate acting rather than the writer rejecting everything.

The first version of this arm was itself decorative, and the mutation is what caught it. Removing validate_handle_file(desired) from the writer left it passing. The writer validates twice — once on the caller's value at entry, and again on the merged result after the tenant block is folded in — so removing either call alone is still caught by the other. The arm reddens only when both go:

remove entry check only    -> 71 passed, 0 failed   (arm green, gap not closed)
remove both                -> an_invalid_handle_file_is_refused_at_the_write_path ... FAILED

So the arm holds the write path as a whole and does not pin either call site individually. That limitation is now written at the arm, because the alternative is a reader inferring coverage from the name — which is precisely how the gap being fixed here was created.

P2 (gate.sh): stale measurement note

Correct. The note said 608 while the floor read 613; taken literally that arm fails every run.

Re-measured on the final tree after adding the write-path arm: 614. Floor and note both updated, red-checked in both directions (615 fails, 614 passes).

Worth recording how the measurement nearly went wrong: the first count returned 358, which is not what adding one test does. The number came from a truncated stream rather than a completed run — exit 0, 13 suite results, no failures. Re-counting from the finished log gave 614. A measurement whose result does not match the change that produced it is a measurement to repeat, not to write down.