Conversation
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You’re at about 95% 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.
Architecture diagram
sequenceDiagram
participant W as Writer (writeCustodyHandleManifestEntry)
participant V as isScopedCustodyCredentialId
participant M as Manifest (handles.json)
participant R as Reader (readCustodyHandles)
participant P as Co-tenant Parser (Claustrum)
participant C as corruptLabels
Note over W,C: Shared manifest - co-tenant file
Note over W,V: WRITE PATH
W->>V: Validate credentialId
alt All segments non-empty AND segments[1] === provider
V-->>W: Valid
W->>M: Write entry to manifest
M-->>W: { status: "written" }
else Empty segment found (e.g., "oauth:anthropic:")
V-->>W: Invalid
W-->>W: Refuse before disk
W-->>W: Return { status: "refused", reason: "invalid entry" }
end
Note over R,C: READ PATH
R->>M: Read manifest
M-->>R: Raw rows
R->>V: Validate each credentialId
alt All segments non-empty AND segments[1] === provider
V-->>R: Valid
R->>R: Add to accounts
else Empty segment (e.g., ":anthropic:x" or "oauth::x")
V-->>R: Invalid
R->>C: Add to corruptLabels set
Note over C: Rows isolated individually
end
Note over R,P: FAILURE MODE COMPARISON
R-->>R: Per-row handling (our parser)
P->>M: Read same manifest
alt Malformed row present
P-->>P: Reject ENTIRE file
else All rows valid
P-->>P: Accept manifest
end
Note over W,R: REMOVAL PATH (kept permissive)
R->>R: removeCustodyHandleManifestEntry
Note over R: Uses isValidCustodyCredentialId (lenient)
Note over R: Can remove entries written before this fix
iceteaSA
force-pushed
the
fix/custody-credential-id-segments
branch
from
September 18, 2026 16:49
92dd41f to
a871457
Compare
iceteaSA
force-pushed
the
fix/custody-credential-id-segments
branch
from
September 18, 2026 18:06
a871457 to
6a2e7da
Compare
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
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.
isScopedCustodyCredentialIdchecked that segment 1 matched the provider but never checked the segments were non-empty, so:anthropic:x,oauth::x,oauth:anthropic:andoauth:anthropic::yall passed.The handle manifest is a co-tenant file — a sibling plugin writes its own block into the same file. Claustrum has tightened their parser to reject any empty segment, and their parser refuses the whole file on a malformed row while ours isolates the bad row to
corruptLabels. So the writer was the real exposure: emittingoauth:anthropic:hands a co-tenant a row that takes their entire manifest down, and their failure mode is total rather than per-row.The rule
Every colon-separated segment must be non-empty, in addition to
segments[1] === provider.The kind segment stays an open set — deliberately not enumerated. Four kinds are live in the vault today (
oauth:,chatgpt:,antigravity:,apikey:) and enumerating them would reject a new one the moment it ships. The label (segment 3+) is still never consulted here; it is a lookup key elsewhere, not an authorization check.Remover left deliberately weak
removeCustodyHandleManifestEntrykeeps the permissiveisValidCustodyCredentialId. Tightening it would strand an already-written malformed entry — the entry could never be removed by the code that refuses to name it. Removal is a cleanup path and must stay able to address anything the writer previously let through.Verification
Independent mutation proof per site, because reader and writer call the same helper and one test can go green while the other site is untested:
writer refuses empty-segment credential ids before they reach disk{ status: "written" }rejects credential ids with any empty colon-separated segmentSet {"empty-kind"}, gotSet {}Conformance probed against the built predicate, not only via unit tests — the accept rows are live vault credential shapes and a regression there would mean the kind segment had been narrowed:
Gates:
packages/core202 pass / 0 fail (488 assertions), root typecheck clean.Context: cortexkit/claustrum#44 (the rule) and #45 (per-row refusal as its safety precondition).
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Rejects empty colon-separated segments in custody credential IDs so malformed IDs can’t reach the shared manifest, where a co-tenant parser rejects the whole file. Previously only the provider segment was checked, so IDs like
oauth:anthropic:were accepted on write and treated as valid on read.Written for commit 6a2e7da. Summary will update on new commits.