opencode: persist custody telemetry to a bounded file, and say so when serving - #35
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 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/log.ts">
<violation number="1" location="packages/opencode/src/log.ts:97">
P2: When the file is near 5 MiB, this append can push it over the limit and leave it oversized until another event occurs. Rotate based on existing size plus the next line's byte length before appending.</violation>
</file>
<file name="packages/opencode/src/plugin.ts">
<violation number="1" location="packages/opencode/src/plugin.ts:396">
P2: A split provider (custody handle present with a real local credential) is logged with both `state: "split"` and `state: "unmanaged"`. The split branch never continues, so control falls through to the `unmanaged` line after setting the throwing fetch. These are contradictory states and this also breaks the PR's stated "one info line per provider" contract. Only log `unmanaged` for providers that are not split, e.g. move `log.info({ provider, state: "unmanaged" })` into an `else` of the `owner === OUR_PLUGIN_ID` branch (or `continue` at the end of that branch).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| initialized = true; | ||
| } | ||
| rotateIfNeeded(); | ||
| appendFileSync(path, `${JSON.stringify(fileEntry(entry))}\n`, { mode: 0o600 }); |
There was a problem hiding this comment.
P2: When the file is near 5 MiB, this append can push it over the limit and leave it oversized until another event occurs. Rotate based on existing size plus the next line's byte length before appending.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/log.ts, line 97:
<comment>When the file is near 5 MiB, this append can push it over the limit and leave it oversized until another event occurs. Rotate based on existing size plus the next line's byte length before appending.</comment>
<file context>
@@ -32,12 +41,78 @@ function defaultSink(entry: CustodyLogEntry): void {
+ initialized = true;
+ }
+ rotateIfNeeded();
+ appendFileSync(path, `${JSON.stringify(fileEntry(entry))}\n`, { mode: 0o600 });
+ chmodSync(path, 0o600);
+ } catch {
</file context>
| fetch: async () => { throw error; }, | ||
| }; | ||
| } | ||
| log.info({ provider, state: "unmanaged" }); |
There was a problem hiding this comment.
P2: A split provider (custody handle present with a real local credential) is logged with both state: "split" and state: "unmanaged". The split branch never continues, so control falls through to the unmanaged line after setting the throwing fetch. These are contradictory states and this also breaks the PR's stated "one info line per provider" contract. Only log unmanaged for providers that are not split, e.g. move log.info({ provider, state: "unmanaged" }) into an else of the owner === OUR_PLUGIN_ID branch (or continue at the end of that branch).
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 396:
<comment>A split provider (custody handle present with a real local credential) is logged with both `state: "split"` and `state: "unmanaged"`. The split branch never continues, so control falls through to the `unmanaged` line after setting the throwing fetch. These are contradictory states and this also breaks the PR's stated "one info line per provider" contract. Only log `unmanaged` for providers that are not split, e.g. move `log.info({ provider, state: "unmanaged" })` into an `else` of the `owner === OUR_PLUGIN_ID` branch (or `continue` at the end of that branch).</comment>
<file context>
@@ -369,36 +371,42 @@ export function createOpencodeClaustrumPlugin(dependencies: ConfigHookDependenci
fetch: async () => { throw error; },
};
}
+ log.info({ provider, state: "unmanaged" });
continue;
}
</file context>
|
Pushed Now: console carries Pinned by inverting the test that had documented the old routing ( Verified on the built bundle: exercising the config hook with stdout and stderr captured separately gives 0 stdout lines, 0 stderr lines, 3 file lines ( |
There was a problem hiding this comment.
1 issue found across 2 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/tests/log.test.ts">
<violation number="1" location="packages/opencode/src/tests/log.test.ts:40">
P3: This test calls `createLogger()` with no sink, so it constructs a real `createFileLogSink()` that writes every logged record - including the `state:"serving"` line - to the process's actual default path (`$XDG_STATE_HOME` or `~/.local/state/cortexkit/opencode-plugin/custody.jsonl`). The test is not hermetic: it pollutes the developer's real custody log and silently depends on that path being writable. In an environment where the default path is unwritable (e.g. a read-only HOME), the file sink's fail-open path emits a console.error warning on the first write, changing `errorLines` to length 3 and making `expect(errorLines).toHaveLength(2)` plus `errorLines[0]`/`errorLines[1]` assertions fail. Since this test only exercises console routing, route the records explicitly instead of the default logger so the real filesystem is not touched.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| test("the console sink carries only faults: info and debug never reach stdout or stderr", () => { | ||
| // The console is the OpenCode TUI's screen. Happy-path telemetry surfacing | ||
| // there is the defect this pins (2026-09-05: three "serving" lines per boot in the TUI). | ||
| const real = createLogger(); |
There was a problem hiding this comment.
P3: This test calls createLogger() with no sink, so it constructs a real createFileLogSink() that writes every logged record - including the state:"serving" line - to the process's actual default path ($XDG_STATE_HOME or ~/.local/state/cortexkit/opencode-plugin/custody.jsonl). The test is not hermetic: it pollutes the developer's real custody log and silently depends on that path being writable. In an environment where the default path is unwritable (e.g. a read-only HOME), the file sink's fail-open path emits a console.error warning on the first write, changing errorLines to length 3 and making expect(errorLines).toHaveLength(2) plus errorLines[0]/errorLines[1] assertions fail. Since this test only exercises console routing, route the records explicitly instead of the default logger so the real filesystem is not touched.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/log.test.ts, line 40:
<comment>This test calls `createLogger()` with no sink, so it constructs a real `createFileLogSink()` that writes every logged record - including the `state:"serving"` line - to the process's actual default path (`$XDG_STATE_HOME` or `~/.local/state/cortexkit/opencode-plugin/custody.jsonl`). The test is not hermetic: it pollutes the developer's real custody log and silently depends on that path being writable. In an environment where the default path is unwritable (e.g. a read-only HOME), the file sink's fail-open path emits a console.error warning on the first write, changing `errorLines` to length 3 and making `expect(errorLines).toHaveLength(2)` plus `errorLines[0]`/`errorLines[1]` assertions fail. Since this test only exercises console routing, route the records explicitly instead of the default logger so the real filesystem is not touched.</comment>
<file context>
@@ -34,20 +34,23 @@ describe("custody logger", () => {
+ test("the console sink carries only faults: info and debug never reach stdout or stderr", () => {
+ // The console is the OpenCode TUI's screen. Happy-path telemetry surfacing
+ // there is the defect this pins (2026-09-05: three "serving" lines per boot in the TUI).
+ const real = createLogger();
+ real.debug({ provider: "deepseek", state: "available" });
+ real.info({ provider: "deepseek", state: "serving" });
</file context>
|
Gated green at The diagnosis is right and worth stating plainly, because it is the part that makes the rest necessary: telemetry going to a pty that nothing persists is telemetry that does not exist, and a plugin silent on the happy path cannot be distinguished from a plugin that is not running. I reviewed the security claim rather than the feature, since that is my half. One thing
|
|
Both landed, two commits: The canary as you specified it cannot go RED on this branch — and that is a finding, not a dodge. The implementer drove a real malformed handle file ( So the protection is now a mechanism at the sink, which is what you asked for.
Your other three, as asked: Hermetic 158/158, |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
All five, in Field rules now match what the producers emit, not a character class:
P3s: the integration arm removes its Hermetic 160/160, |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Both, in Rules now: The population is now mechanical rather than typed: the source-scan test collects every Secret table (all rejected by both rules, checked outside the suite as well): The integration file now has the arm you asked for: through the real logger and file sink, Hermetic 161/161, |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Correct observation, and the answer is a ruling rather than a fix:
Hermetic 162/162, |
|
Re-reviewed at The caller gap is closed, and the second test is what closes itMy earlier point was that the protection was a convention held at four call sites: the canary hand-built its entry, so it proved the sink drops That one hand-builds too, and that is correct now. Before, the sink dropped one field and the caller decided what went in the others, so only a real fault path could tell you what a caller does. Now every field passes a rule, so the mechanism is the rule and a hand-built input is the right way to exercise it. The distinction is worth stating because "hand-built" was my criticism and it stopped being one when the thing under test changed.
My finding survives, moved fields, and my example expiredI said "every secret shape fails both" overshoots because But Reaching it still needs a caller to put a hex string in one of those fields, and no producer does — the wire classes are If you want it closed for free: reject an all-hex body. Every real value on both sides has a non-hex letter or an underscore, so the rule costs nothing and removes the class rather than narrowing it. Your call. And the general shape is worth more than the instance: a length bound is not a shape bound. Tightening 32 to 24 makes the window smaller and leaves it open, while widening a sibling rule to accept a new legitimate form can re-open it on another field with nothing to notice — which is what happened between two commits here. The gate fails on a stale lockfile again, not on your changeThree waves since your branch point, on a PR that touches no Rust. Same as last time — a rebase clears it. Worth knowing that One thing I would still change before merge
|
9efc44d to
c117b21
Compare
|
Rebased onto Hex class: closed, your way. Both rules now reject an all-hex (or all-digit) body outright — Your general shape is the thing I am keeping: a length bound is not a shape bound. The hex window was moved by one commit and re-opened on a sibling field by the next, and nothing in the suite could see it because the tests were checking lengths. A shape predicate closes the class; a cap only makes it smaller. Pre-filter spread: one line at the site — the additions before the filter are process-generated ( Rebase: the branch was three lock waves stale, as you said. Rebased with siblings pinned to what master's lock declares ( Hermetic 162/162, |
|
Gated at The hex hole is properly closedI reported that tightening Producers still pass, the hex shapes do not, and the rule now sits on both fields rather than on one of them. One false-positive class, not a blockThe control found it: English words that happen to be all hex characters are rejected. Bounded and in the safe direction — such a value renders Your red gate is mine, not yoursDo not go looking for a defect in your own diff. I nearly misdiagnosed this the way I misdiagnosed #33 an hour earlier — assumed a stale lockfile, swapped in master's, and it stayed red:
So both failure modes — Sequencing hazard I created, stated plainly because it is going to hit anything else that is open: the moment 0.19 landed, every PR touching Notes
|
c117b21 to
f95125e
Compare
|
Rebased onto Your diagnosis was right and I did not go looking in my own diff. The branch was on The rebase moved the base and nothing else. Worth stating how that is checked rather than asserted, since a squash on a stale base silently reverted four upstream commits on #28 a week ago and the gate stayed green through it: The false-positive class is now written where it will be read. Four lines above Gate green on the rebased tree; bun suite 162 pass. On the sequencing hazard: announcing the next wire migration before landing it would have saved this round, but the cost fell on me for a reason I own too — both PRs sat open across it. Small open branches rebased eagerly are cheaper than correctly diagnosed ones. |
f95125e to
96377c4
Compare
|
Rebased past Worth saying why this one needed it at all: the diff here is eight TypeScript files and touches no Rust and no Cargo file. Merging it could never have moved your lockfile backwards. But you build branch heads locally, and this head carried All three of my open PRs had the same stale lock; #33 and #38 are rebased and pushed too. |
Lock only. subc-protocol resolves to 0.19.0 and subc-transport to 0.6.0 in this set; no source change is required, which is checkable rather than assumed -- the gate passes on the unchanged tree. CAUGHT BY THE CONTROL, NOT BY SUSPICION, and this instance is the one that shows why the control is worth running every time. PR cortexkit#35's gate failed on a lockfile refusal. Their merge-base is 3c509b3, which ALREADY CARRIES yesterday's wave-16 lock -- so "their branch is stale" was not merely the obvious reading, it was the reading their own merge-base seemed to rule out. It was still wrong: their branch cannot update the lock file ... --locked was passed MY master cannot update the lock file ... --locked was passed Third time today the same control has reversed my answer, and the first time it did so against a branch whose base looked current. A lockfile refusal is a statement about the sibling checkouts ON THIS DISK at THIS MOMENT, not about the branch under test, and the branch's own recency is not evidence either way.
96377c4 to
a77e2f7
Compare
|
Rebased onto Gate green, bun hermetic green. Lock byte-identical to master. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Flagging a merge-order hazard on this branch before it is reviewed, not after. This branch's
No marker catches it either, because "the gate passes" is true on both sides of the defect. Rebasing onto current master and re-measuring on the merged tree, rather than carrying the number forward. Noting it here because the hazard is invisible in the diff: the file that causes it is one this PR never touches. Found by checking merge-bases across my open PRs rather than recalling that I had rebuilt them — a peer tenant had just been bitten by assuming one branch was stacked on another when it had forked earlier and was missing a production fix, and every release marker they had specified still passed because those markers came from the other branch. Same shape: verification that cannot see the thing it exists to see. |
A floor is a lower bound, and lower bounds do not complain about being lowered. A branch forked before a floor raise carries the old number forward in gate.sh and merges green, silently reverting the raise. Nothing goes red -- the count still clears the now-smaller minimum, so the gate passes on both sides of the defect. The branch need not touch gate.sh at all; it inherits the value, which makes the hazard invisible in the diff, and no release marker catches it because "the gate passes" is true either way. Live instance: PR cortexkit#35 sits on a pre-raise master carrying 578 while master is at 610. Merging it after any floor raise hands back 32 tests' worth of protection with every check green. Replayed that exact file against this ratchet and it fails. The comparison is against the TARGET's value rather than gate.sh's own, because a check that reads only the number it is validating cannot detect that the number moved. Unresolvable is not passing. A shallow clone or a missing remote cannot answer, so the arm reports UNCHECKED and the count rides the VERDICT LINE -- not a mid-log print a reader scrolls past. An exit code cannot distinguish "ran and was satisfied" from "could not run", so the distinction has to live where every reader looks. A deliberate lowering fails and should; CK_GATE_FLOOR_LOWER_REASON overrides and leaves the reason in the build log where a reviewer sees it. Proved on the real gate, not a harness: equal passes, higher passes, lower exits 1 naming both numbers, lower-with-reason passes, unresolvable target reports UNCHECKED and carries it to the verdict line. Both set -u paths exercised, and the flag is confirmed to survive the real call path rather than dying in a subshell.
A floor is a lower bound, and lower bounds do not complain about being lowered. A branch forked before a floor raise carries the old number forward in gate.sh and merges green, silently reverting the raise. Nothing goes red -- the count still clears the now-smaller minimum, so the gate passes on both sides of the defect. The branch need not touch gate.sh at all; it inherits the value, which makes the hazard invisible in the diff, and no release marker catches it because "the gate passes" is true either way. Live instance: PR #35 sits on a pre-raise master carrying 578 while master is at 610. Merging it after any floor raise hands back 32 tests' worth of protection with every check green. Replayed that exact file against this ratchet and it fails. The comparison is against the TARGET's value rather than gate.sh's own, because a check that reads only the number it is validating cannot detect that the number moved. Unresolvable is not passing. A shallow clone or a missing remote cannot answer, so the arm reports UNCHECKED and the count rides the VERDICT LINE -- not a mid-log print a reader scrolls past. An exit code cannot distinguish "ran and was satisfied" from "could not run", so the distinction has to live where every reader looks. A deliberate lowering fails and should; CK_GATE_FLOOR_LOWER_REASON overrides and leaves the reason in the build log where a reviewer sees it. Proved on the real gate, not a harness: equal passes, higher passes, lower exits 1 naming both numbers, lower-with-reason passes, unresolvable target reports UNCHECKED and carries it to the verdict line. Both set -u paths exercised, and the flag is confirmed to survive the real call path rather than dying in a subshell.
da337ac to
334e5f7
Compare
|
Rebuilt on master Rebased rather than rebuilt was not an option: master turned The floor hazard I flagged earlier is goneThis branch carried What changed versus the old stackThe branch's Reaching for the public piece would have been wrong in a way worth naming: So: export the predicate, not the set. Exporting
The armsThe first version of this change was correct and unprotected. Replacing both Two arms added, and mutation-proved against the realistic regression rather than a strawman. Swapping the predicate for Each arm asserts forbidden → Gate exit 0, hermetic 218/0, revert sweep clean, One note for the record: |
|
Gate is green on the merged tree and the ratchet is satisfied (
|
|
Finding confirmed and the fix is right. Two corrections to the severity, both of which change how the test must be written — and the second is the reason I am posting before pushing rather than after. The example in the finding would produce a vacuous testA real handle is
|
…n it cannot Persist allowlisted custody telemetry as JSONL at $XDG_STATE_HOME/cortexkit/opencode-plugin/custody.jsonl, creating the directory at 0700 and the file at 0600. Rotate at 5 MiB to .1 and support CLAUSTRUM_CUSTODY_LOG as an override or off switch. Telemetry is fail-open: a sink failure drops subsequent entries and emits one unavailable notice. Normal console output is removed so custody telemetry cannot reach the OpenCode TUI; the single notice exists only when the file sink itself is unavailable. Sink-side shape validation and a strict field allowlist omit errorMessage, preventing a Bun JSON.parse SyntaxError from carrying a bearer handle into the log. This rebuilds PR cortexkit#35 on master after handle-file implementation moved to packages/client. The branch export from opencode handles became obsolete; client now additively exports the parser-owned identifierIsValid predicate so the sink uses the exact parser rule without duplicating or drifting from it.
|
Fixed in Fixture is One thing I withdrew rather than let the implementer produceI had asked for a second mutation — swap the fixture to 45 chars, confirm the arm passes with the fix removed. The implementer stopped and pointed out it cannot: with a 45-char value and no exclusion the arm still asserts I refused that. Mutating implementation and expectation together proves nothing — any test can be made green by changing both sides, and a suite that agrees with whatever the code does is a mirror rather than an oracle. Replaced with a direct predicate measurement, which is what I actually wanted and needs no mutation at all. Residual surface, since the claim is structuralRunning the new predicate across shapes: So the claim after this commit is narrower than "the file cannot carry a secret": it is that the file cannot carry a capability handle. An API-key-shaped string passes, because I checked reachability rather than leaving it hypothetical. Both writers gate Not expanding scope for that on this PR. Flagging it so the merged claim is the accurate one, and because "identifier-shaped" is a weak guarantee against secret material generally — if you want the stronger property, the fix is an allowlist of known labels rather than a shape test, and that is a different change. Gate green on the rebased tree, |
334e5f7 to
012bfa4
Compare
2 participants
The custody plugin's telemetry was going nowhere. It logs structured JSON via
console.log/console.error; under the OpenCode TUI that is the pty, which nothing persists —opencode.loghad 0 such lines, the daemon journal had 0. And the plugin was silent on the happy path (1 debug / 0 info / 2 warn / 2 error call sites), so "no news" was indistinguishable from "not running". The only witness that it was serving at all was the vault's audit chain, which only works when one operator owns both ends.What this adds
A bounded JSONL file sink, on by default, alongside the console sink.
$CLAUSTRUM_CUSTODY_LOGif set, else${XDG_STATE_HOME:-~/.local/state}/cortexkit/opencode-plugin/custody.jsonl. Dir 0700, file 0600.<path>.1past 5 MiB; one generation kept.CLAUSTRUM_CUSTODY_LOG=off|0|false|nodisables it.Two happy-path lines, both bounded.
confighook, oneinfoper provider with the cell decision in the plugin's existing vocabulary (serving/ refusal states).infowith{provider, label, credentialId, recordVersion, state:"served"}. Never per request.The property that matters: the file cannot carry a secret
Entries written to the file pass through an explicit allowlist (
FILE_FIELDS: level, provider, label, credentialId, recordVersion, state, httpStatus, cooldownUntil, errorClass, errorCode).errorMessageis not in it — Bun'sJSON.parsequotes adjacent tokens into its error message, which is how a hand-edited handle file leaks a bearer handle (fixed once already in #28). No free-text field reaches disk.Pinned by a canary that drives a fault path whose error text contains a fake 47-char
ckh_handle and a fake key, then asserts neither appears in the file. Mutation-proved independently of the implementer: addingerrorMessageto the written record (exactly one site) turns it red —— byte-identical restore, 7 pass.
Verified on the installed bundle
Exercised the built bundle's config hook in-process against a scratch
XDG_STATE_HOMEon the live box: wrote{"level":"info","provider":"minimax-coding-plan","state":"serving","ts":…,"pid":…}(the one provider under custody here), file 600 / dir 700, secret-shape scan clean. Bundle shape unchanged: single{id, server}default export, Node builtins only.Hermetic 152/152, gate green, exit census updated (plugin 33/41, serve 13/15).
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes custody telemetry that printed into the OpenCode pty and was lost, while the plugin stayed silent on the happy path. Telemetry now persists to a bounded JSONL file (on by default), and no plugin log level reaches the TUI anymore — warn and error included.
File sink
$CLAUSTRUM_CUSTODY_LOGif set, else~/.local/state/cortexkit/opencode-plugin/custody.jsonl(dir 0700, file 0600); rotates to.1past 5 MiB, andoff|0|false|nodisables it.tsandpid, and pass a field allowlist plus shape rules that droperrorMessageand secret-shaped values.Log lines and safety
servedline per provider per process — never per request.identifierIsValid, so the sink accepts only what the parser would.Written for commit 012bfa4. Summary will update on new commits.