feat: inherit CIRISAgent 2.9.41's client work — superset restored - #18
Conversation
CIRISAgent cut v2.9.41-stable and kept working on the client code after our 0.5.191 tag. CIRISClient#11 asked for seven fixes; they are all here, plus one neither tree had fixed. Verified against the tree rather than the list: five files differed from theirs and three test files were absent entirely. WHAT WE ALREADY HAD. The log-buffer race (#1) was fixed here in PR #12 from the Codex review — compare-and-set with the id derived inside the update block. Their implementation is semantically identical; only the seed differs (their first id is 0, ours 1). No merge needed, but their concurrency test comes across, because the fix without the test is a claim. WHAT WE DID NOT HAVE: 2 BUNDLE REDACTION. The bundle exists to be SENT — pasted into an issue, mailed to support — and nothing scrubbed it. Applied at `render()`, the single choke point all three screens pass through, so the environment rows, the caller's `extra` map and the log buffer are covered by construction rather than by three callers remembering. Patterns match credential SHAPES, never entropy, and `token: expired` is excluded as a state: a redactor that eats ordinary log text destroys the artifact's reason for existing. 3 ANDROID EXPORT LEFT EMPTY FILES IN DOWNLOADS. The row is inserted before `openOutputStream`; a failed write returned failure and left the row. An empty debug bundle is worse than none — someone sends it believing they sent their logs. IS_PENDING until the bytes land, delete if not. 4 THE BUNDLE REGISTERED ITS CONTENT HEIGHT, NOT ITS VIEWPORT. `.testable()` sat after `.verticalScroll()`, and `onGloballyPositioned` reports the node's size at its own point in the chain. A full bundle registered as 17952px, so `/tree` advertised an element taller than any screen and automation aiming at its centre targeted roughly y=10000. Ours had this defect, and our own walk-test harness reads `/tree`. 5 RESET WIPED LOCAL STATE WHILE DRIVING A REMOTE NODE. `CIRIS_API_URL` is a supported way to point the client at another host and `PythonRuntime` honours it; the wipe never consulted it. Reset reported success having reset nothing the user was using, while deleting the database, identity and keys of whatever local node shared the disk. Their fourth misresolution in that function and the first above the filesystem layer: the previous three picked the wrong directory, this one picked the wrong machine. COMPOSED with our declared-home resolution rather than replacing it — asking the node where it lives is right when the node is ours, and this decides whether it is. Loopback matched as an ADDRESS, because `127.0.0.1.evil.com` is an ordinary hostname. 7 TESTS FOR THE WIPE, and `wipeGeneratedState` extracted so they can drive it against a real populated tree. Every bug in that function was "deleted more than it should have", which is only observable by asserting what SURVIVES — a thesis file, a src/, a .git. Asserting the targets are gone would have passed for all four. AND ONE NEITHER TREE HAD FIXED (#6, which their issue flagged and left with us): `getAppVersion()` prefers the JAR manifest, and the Compose uber-jar writes only `Main-Class` — so `DESKTOP_VERSION_FALLBACK` is the ONLY value desktop ever reports. As a hand-maintained literal it said "2.3.2" while this repo shipped 0.5.191, in the one artifact whose entire job is to say what was running. Their advice was "if you keep a hand-maintained constant, put a check on it". We do not keep it: the value is now the GENERATED CLIENT_VERSION, so there is no second number to drift and no check needed for one that cannot exist. MUTATION-TESTED, because inherited tests are someone else's evidence until they have been watched fail here: redactSecrets -> identity 2 redaction tests fail ownsLocalBackend -> always true the loopback test fails Both revert clean. `:shared:desktopTest` green (3 + 5 + 5 inherited, plus the new desktop-version test), wasmJs compiles, localization and vendoring pass. Their closing note does not apply to us: CIRISAgent's CI ran zero Kotlin tests. Ours has run `:shared:desktopTest` since the 0.5.188 matrix, which is how the 390 in commonTest stay honest — and as of PR #10 it also walks the app. Closes #11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e268f0f20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return h == "localhost" || h == "::1" || h == "0.0.0.0" || LOOPBACK_IPV4.matches(h) | ||
| } | ||
|
|
||
| private fun ownsLocalBackend(): Boolean = ownsLocalBackend(System.getenv("CIRIS_API_URL")) |
There was a problem hiding this comment.
When the client starts locally and the user switches to a remote profile, NodeSwitcherViewModel.switchTo() repoints the shared API client at profile.baseUrl (NodeSwitcherViewModel.kt:293-295) without changing CIRIS_API_URL, so this overload still returns true. Reset then uses the remote node's declared home as a local filesystem path; if that path exists locally it deletes unrelated local state, and if it does not exist the function reports a successful reset while leaving the remote node untouched. The same mismatch occurs when desktop is configured through CIRIS_NODE_URL, which Main.kt:185-188 prefers but this check ignores. Pass and validate the actual active node URL instead of rereading only CIRIS_API_URL.
Useful? React with 👍 / 👎.
| // costs the reader the one fact the line carried. | ||
| + """(?!(?:expired|missing|present|invalid|unknown|revoked|pending|""" | ||
| + """refreshed|required|rejected|absent)\b)""" | ||
| + """([^\s"',;)}\]]{6,})""" |
There was a problem hiding this comment.
When a quoted credential contains whitespace, such as password="correct horse battery staple", this value group stops at the first space, so the output becomes password="<redacted> horse battery staple" and most of the credential is still copied or exported in the debug bundle. Quoted passwords and secrets may legitimately contain spaces; match through the closing quote (while retaining the current delimiter behavior for unquoted assignments) so the entire value is removed.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // Publish only now that the file is complete. | ||
| ctx.contentResolver.update( |
There was a problem hiding this comment.
When the byte write succeeds but this final update throws, the outer runCatching returns null without executing the deletion used for write failures, leaving the full bundle stored as an orphaned pending MediaStore item. The returned row count is also ignored, so an update that returns zero reports Downloads/$fileName even though the item remains pending and invisible. Treat both an exception and a zero-row update as publication failure and delete the inserted URI.
Useful? React with 👍 / 👎.
Codex reviewed the inheritance and found the fixes I brought across from
CIRISAgent 2.9.41 do not fully close what they claim to. All three are real,
verified against the code rather than the description, and two are the same
class as the defect being fixed.
1 THE WIPE GUARD READ AN ENVIRONMENT VARIABLE, NOT THE ACTIVE NODE. P1.
`ownsLocalBackend()` re-read `CIRIS_API_URL`. Two live paths bypass that:
- `NodeSwitcherViewModel.switchTo` calls `apiClient.updateBaseUrl(...)`
and never touches the environment. After switching to a saved remote
profile the env still says loopback and the guard still says "ours".
- `Main.kt` PREFERS `CIRIS_NODE_URL`; a desktop configured that way was
checked against a variable nobody set.
Reset would then take a REMOTE node's declared home as a local path:
deleting unrelated local state if that path exists here, and otherwise
reporting a successful reset while the node the user is looking at is
untouched. That is precisely the "wrong machine" error the inherited guard
was added to prevent, reached by a different road — the fix was written
against the configuration that motivated it rather than against the
question it asks.
`wipeLocalData` now takes the ACTIVE node URL and CIRISApp passes the live
`nodeBaseUrl`. The environment is a fallback, and reads BOTH names in the
order `Main.kt` resolves them.
2 THE REDACTOR LEAKED MOST OF A QUOTED SECRET. P1.
The value group stopped at whitespace, and quoted secrets legitimately
contain spaces. Reproduced before changing anything:
password="correct horse battery staple"
password="<redacted> horse battery staple"
Three words of a four-word passphrase, in the artifact whose entire purpose
is to be mailed to a stranger. A quoted arm now matches through the CLOSING
QUOTE and is tried first; the unquoted arm is unchanged. The state
exclusion is carried into both, so `token="expired"` still reads as a state
rather than being redacted to nothing.
3 A FAILED PUBLISH ORPHANED THE ROW. P2.
The bytes landing and the row becoming visible are different events. If the
IS_PENDING=0 update threw, the outer runCatching returned null without
reaching the delete, leaving the complete bundle as an orphaned pending item
— invisible to every file manager and never cleaned up. A zero-row update
neither threw nor published, and the function reported `Downloads/<name>`
for a file the user cannot find. Both now delete the row and report failure,
which is what this function already did for an empty write.
Two new redaction cases; `:shared:desktopTest` green; desktop and wasmJs
compile.
Worth sending back to CIRISAgent: 1 and 3 are in their tree as shipped, and 2
is their pattern verbatim.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7603769431
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A node switch repoints the API client | ||
| // without touching the environment, so the | ||
| // environment is not the answer (Codex, PR #18). | ||
| activeNodeUrl = nodeBaseUrl, |
There was a problem hiding this comment.
When the user switches from the local profile to a remote profile, this argument remains the immutable CIRISApp parameter; NodeSwitcherViewModel.switchTo() mutates apiClient.baseUrl instead. Fresh evidence in this revision is that the new line explicitly claims to pass the current URL while still passing the stale nodeBaseUrl. The desktop guard therefore accepts the original loopback URL and can interpret the remote node's declared home as a local path, deleting unrelated local state or falsely reporting success. Pass apiClient.baseUrl or the active profile URL instead.
AGENTS.md reference: AGENTS.md:L62-L62
Useful? React with 👍 / 👎.
| + """(\s*[:=]\s*)(["'])""" | ||
| + """(?!(?:expired|missing|present|invalid|unknown|revoked|pending|""" | ||
| + """refreshed|required|rejected|absent)["'])""" | ||
| + """([^"']{6,})(\3)""" |
There was a problem hiding this comment.
When a double-quoted secret contains an apostrophe, such as password="correct horse's battery staple", this quoted arm fails because its value class excludes both quote types rather than only the captured delimiter; the unquoted fallback then redacts at most the prefix and leaves the remainder of the credential in the exported bundle. Fresh evidence in this revision is the new [^"'] value group added to address quoted secrets. Use delimiter-specific alternatives so the opposite quote character can occur inside the value.
Useful? React with 👍 / 👎.
…uestion
Codex reviewed my fixes for its own findings and both were wrong, in the same
way, and it is the same way the inherited fixes were wrong: written against the
case that motivated them rather than against the question being asked.
1 I PASSED THE STALE URL WHILE SAYING I PASSED THE CURRENT ONE. P1.
The fix threaded an `activeNodeUrl` through `wipeLocalData` and CIRISApp
passed `nodeBaseUrl` — an immutable CIRISApp PARAMETER, fixed for the
composition. `switchTo` mutates `apiClient.baseUrl` through `updateBaseUrl`
and never touches the parameter. So after a switch to a remote profile the
guard still read loopback and still said "ours": the whole fix did nothing
for the exact case it was written for.
Worse than not fixing it, because the line carried a comment asserting it
passed the URL the client was talking to right now. A reader checking this
would have read the claim and moved on. Now `apiClient.baseUrl`.
2 ONE QUOTED ARM CANNOT COVER TWO DELIMITERS. P1.
The arm excluded BOTH quote characters from the value, so a double-quoted
secret containing an apostrophe never matched:
password="correct horse's battery staple" unchanged, no match
and fell through to the unquoted arm, which stops at whitespace — leaking
everything after the first word again. That is the defect I had just fixed,
reappearing through the character I chose to exclude.
One arm per delimiter, each excluding only its own, so the opposite quote
can appear inside the value — which it may, and does in ordinary English.
`SECRET_NAMES` and `NOT_A_STATE` are now single definitions shared by all
three arms, because three copies of an alternation is a drift waiting to
happen and this file now has three arms.
Four redaction cases in total for the quoting rules, and both delimiters are
tested carrying the other. `:shared:desktopTest` green, desktop compiles.
The pattern across both rounds is worth naming: a fix aimed at the reproduction
passes the reproduction. The question was never "does this handle the example"
but "what does this guard actually read" and "what does this pattern actually
delimit".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2d1103d8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // | ||
| // `["]` rather than a bare quote: a raw string cannot end with one. | ||
| Regex( | ||
| """(?i)\b($SECRET_NAMES)(\s*[:=]\s*)["](?!(?:$NOT_A_STATE)["])([^"]{6,})["]""" |
There was a problem hiding this comment.
When a credential is serialized with an escaped double quote, such as {"password":"correct \"horse\" battery staple"}, [^”] treats the escaped quote as the closing delimiter and produces password="<redacted> \"horse\" battery staple". The remaining passphrase is therefore retained in the debug bundle; match escaped characters as part of the quoted value so the actual unescaped closing quote terminates redaction.
Useful? React with 👍 / 👎.
| private const val SECRET_NAMES = | ||
| """api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|""" + | ||
| """auth[_-]?token|token|secret|password|passwd|client[_-]?secret""" |
There was a problem hiding this comment.
When a bundle contains assignments using credential names already present in the client models, such as llm_api_key, system_admin_password, or owner_password, none of these alternatives match: the leading word boundary cannot start at api_key or password after an underscore, and the pattern requires a delimiter immediately after the matched name. The complete credential is therefore exported unchanged; include the compound field spellings while avoiding status-only names such as llm_api_key_set.
Useful? React with 👍 / 👎.
| val host = runCatching { java.net.URI(url).host }.getOrNull() | ||
| ?: return false // unparseable: assume not ours | ||
| val h = host.trim().removePrefix("[").removeSuffix("]").lowercase() | ||
| return h == "localhost" || h == "::1" || h == "0.0.0.0" || LOOPBACK_IPV4.matches(h) |
There was a problem hiding this comment.
When desktop is configured with a valid expanded loopback URL such as http://[0:0:0:0:0:0:0:1]:8080, URI.host retains that expanded spelling, so this comparison classifies the locally owned backend as remote. The reset flow has already shut down the local runtime before this predicate runs and then handles the false return as a wipe failure, leaving the user logged out with no data erased; normalize IPv6 addresses or explicitly recognize the expanded loopback representation.
Useful? React with 👍 / 👎.
Round three, and one finding changes what the redaction is FOR.
COMPOUND NAMES. P1, and the important one. `\b(api[_-]?key|password|...)`
cannot fire inside `llm_api_key`: `_` is a word character, so there is no
boundary before `api`. The spellings this codebase uses for credentials are all
compound —
llm_api_key 7 sites
system_admin_password 4
backup_llm_api_key 3
owner_password 1
— so the pattern was blind to exactly the names a leak here would wear. I told
this repo's owner earlier that the redactor guards a hypothetical future log
line. That was true, and it guarded it INCORRECTLY: the 2am mistake would be
written `platformLog("llm_api_key=$key")`, because that is what this code calls
the thing, and none of it would have matched. A shape matcher with a blind spot
aligned to the local naming convention is worse than none, because it reports
having looked.
The name may now carry a prefix, with the delimiter still required IMMEDIATELY
after it — which is what keeps `llm_api_key_set=true` readable: no split of that
string ends in a credential name followed by `=`. Tested in both directions.
ESCAPED DELIMITERS. P1. `[^"]` reads the `\"` in
`{"password":"correct \"horse\" battery staple"}` as the closing quote and stops
there. The value now consumes escapes as part of itself. Fixing that surfaced a
second miss in the same input: in JSON the NAME is quoted too, so the delimiter
run is `":"` and the pattern had no room for the name's own closing quote. Both
are in, and the JSON form is the case in the test.
EXPANDED IPv6 LOOPBACK. P2, with the worst consequence in the flow. `::1` and
`0:0:0:0:0:0:0:1` are one address and `URI.host` returns whichever was typed, so
a desktop configured with the expanded form was classified REMOTE and refused —
after reset had already shut the local runtime down. The operator is left logged
out, node stopped, nothing erased. Now the STACK decides what loopback means
(`InetAddress.isLoopbackAddress || isAnyLocalAddress`), gated behind a
hex-colons-dots test so a hostname is never resolved: a DNS lookup here would be
slow and would also let a remote name answer "loopback".
12 redaction cases and 7 wipe cases, both directions each — what must be
redacted, and what must survive.
THREE ROUNDS, EIGHT DEFECTS, ALL IN ONE REGEX. That is the argument against
shape matching making an artifact safe, and it should be recorded rather than
smoothed over: every round found a credential form the previous round's pattern
missed, and there is no reason to believe this round is the last. The control
that would actually prevent the leak is on the logging side, where it is
decidable — this remains defense in depth and should not be described as more.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c94ab8411b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // closing delimiter and stops there, leaving the rest of the passphrase | ||
| // in the bundle — the same leak again, through the escape this time. | ||
| Regex( | ||
| """(?i)\b($NAME_PREFIX(?:$SECRET_NAMES))(["']?\s*[:=]\s*)["](?!(?:$NOT_A_STATE)["])((?:[^"\\]|\\.){6,})["]""" |
There was a problem hiding this comment.
When an explicitly named credential has fewer than six characters, such as client_secret="abc" or password=12345, every assignment arm fails because its value quantifier is {6,}, so the credential is exported unchanged. Credential names already provide the intended shape check, and the state-word exclusions handle false positives; short values must be redacted as well.
Useful? React with 👍 / 👎.
| if (!ownsLocalBackend(activeNodeUrlOrEnv(activeNodeUrl))) { | ||
| println("[LocalDataWipe] refusing: CIRIS_API_URL points at a node we do not own") | ||
| return false |
There was a problem hiding this comment.
When the active profile is remote, this guard correctly returns false, but CIRISApp has already called pythonRuntime.shutdown() before invoking wipeLocalData. Thus, after switching from a locally started node to a remote profile and pressing Reset, the app stops the unrelated local node, logs the user out, and leaves the runtime stopped even though it intentionally performs no wipe; perform the ownership check before shutdown.
Useful? React with 👍 / 👎.
| // JWTs — the shape is unmistakable and never appears in prose. | ||
| Regex("""eyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}(\.[A-Za-z0-9_-]+)?""") to "<redacted:jwt>", | ||
| // Authorization headers, including the `service:TOKEN` form. | ||
| Regex("""(?i)\b(bearer\s+)(service:)?[A-Za-z0-9._~+/=-]{12,}""") to "$1<redacted>", |
There was a problem hiding this comment.
On the wasmJs target, Kotlin Regex is backed by ECMAScript regular expressions, where the leading (?i) construct is invalid. Clicking “Open a GitHub issue” on a failure panel calls DebugBundle.environment(), which initializes this object and all SECRET_PATTERNS, so construction throws before the report URL can open; pass RegexOption.IGNORE_CASE instead of embedding the JVM-style flag.
Useful? React with 👍 / 👎.
…opped nodes it would not wipe
Round four. Three findings, and one of them is a live crash rather than a leak.
`(?i)` IS INVALID IN ECMAScript. P1. On wasmJs Kotlin's Regex is backed by JS
regular expressions, which reject the inline flag — so building SECRET_PATTERNS
threw at object initialization. `DebugBundle.environment()` touches the object,
and that is what "Open a GitHub issue" calls on a failure panel: the button on
the screen you reach WHEN SOMETHING IS ALREADY WRONG crashed before it could
open the report. Inherited verbatim from CIRISAgent, where nothing builds wasm.
Now `RegexOption.IGNORE_CASE`, which every target honours.
SHORT VALUES WERE NOT REDACTED. P1. The quantifier was `{6,}`, so
`client_secret="abc"` and `password=12345` were exported whole. The NAME is the
shape check — that is the entire premise of matching on names — and length was
never doing work the name was not already doing. Relaxed to `+`, which makes the
state list load-bearing for the first time: `token: ok` and `password: none` are
now long enough to match, so the exclusions grew to cover them and both
directions are tested.
RESET STOPPED THE NODE IT THEN REFUSED TO WIPE. P2. `wipeLocalData` correctly
declines when the client is driving a remote node, and the reset flow called
`pythonRuntime.shutdown()` BEFORE it. So on a remote profile the app killed an
unrelated local node, logged the operator out, and declined to wipe — reporting
failure for having done the right thing, having already done a wrong one. The
refusal was right and its position was not.
`ownsLocalNode(activeNodeUrl)` is now an expect/actual so the question can be
asked from common code before anything is stopped: desktop delegates to the real
loopback check, Android and iOS answer true (the node is in-process, there is no
other machine to confuse it with), and web answers false (no local runtime, so
no local state to erase).
15 redaction cases. Both directions on every rule.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43ac4ae24b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| /** Android runs its node in-process; there is no other machine to confuse it with. */ | ||
| actual fun ownsLocalNode(activeNodeUrl: String?): Boolean = true |
There was a problem hiding this comment.
When an Android user switches to a remote profile through NodeSwitcherViewModel.switchTo() and then reaches Login through logout or session expiry, apiClient.baseUrl remains remote, but this hardcoded result lets the reset proceed and recursively deletes the unrelated on-device node while leaving the active remote node untouched. The identical iOS actual has the same problem; both mobile implementations should classify the supplied activeNodeUrl rather than assuming every active node is local.
Useful? React with 👍 / 👎.
| Regex("""eyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}(\.[A-Za-z0-9_-]+)?""") to "<redacted:jwt>", | ||
| // Authorization headers, including the `service:TOKEN` form. | ||
| Regex( | ||
| """\b(bearer\s+)(service:)?[A-Za-z0-9._~+/=-]{12,}""", |
There was a problem hiding this comment.
When a logged header contains a valid short credential such as Authorization: Bearer abc123 or Bearer service:abc123, the {12,} minimum prevents this arm from matching, so DebugBundle.render() exports the entire token. Although this revision removed the analogous minimum from named-assignment patterns, the remaining bearer arm still has it; an explicit Bearer marker is sufficient to redact regardless of value length.
Useful? React with 👍 / 👎.
| // UNQUOTED name = value. Whitespace really is the delimiter here. | ||
| Regex( | ||
| """\b($NAME_PREFIX(?:$SECRET_NAMES))(\s*["']?\s*[:=]\s*["']?)""" + | ||
| """(?!(?:$NOT_A_STATE)\b)([^\s"',;)}\]]+)""", |
There was a problem hiding this comment.
When an unquoted credential starts with a state word followed by punctuation, such as password=valid-secret-123, password=no-way-this-leaks, or token=expired-value, this negative lookahead sees the word boundary after valid, no, or expired and suppresses the entire match. The actual credential is therefore exported verbatim; exempt these words only when they comprise the complete delimited value, as the quoted-value arms already do.
Useful? React with 👍 / 👎.
| // aiming at its centre would target y~10000. | ||
| modifier = Modifier | ||
| .heightIn(max = 220.dp) | ||
| .testable("txt_debug_bundle") |
There was a problem hiding this comment.
When the bundle is taller than 220 dp, moving testable() within this modifier chain does not give it a distinct viewport layout node: the desktop implementation uses onGloballyPositioned, and reordering that callback around verticalScroll does not change the finalized LayoutCoordinates it receives. The automation registry can therefore continue reporting the full content height and targeting an off-screen center; attach the tag to the surrounding bounded Surface or another separate viewport container instead.
Useful? React with 👍 / 👎.
…ery that compensated for not doing so
Reset device -> Reset local device
Reset device? -> Reset this device?
and the body now says a node you are connected to on another machine is not
affected.
THAT IS THE ENTIRE FIX, AND IT REPLACES FOUR ROUNDS OF MINE. Codex found that
Reset wiped local state while the client was driving a remote node; I inherited
CIRISAgent's guard for it, and then spent four review rounds making the guard
correct — an env-var read that missed `CIRIS_NODE_URL`, a stale `nodeBaseUrl`
passed while a comment claimed otherwise, an IPv6 spelling that classified a
local node as remote, an ordering that stopped the runtime before refusing, and
mobile actuals I hardcoded to `true` that would have deleted the on-device node.
Every one of those was a real defect in a mechanism that should not exist.
The button was not picking the wrong machine. It was failing to say which
machine it meant. Naming it dissolves all of it:
removed ownsLocalNode expect + four actuals
removed isLoopbackUrl, LOOPBACK_IPV4, activeNodeUrlOrEnv, ownsLocalBackend
removed the activeNodeUrl parameter threaded through wipeLocalData
removed the pre-shutdown refusal in CIRISApp
removed four tests that existed only to check the above
AND THE FEATURE IS A CONVENIENCE, NOT A BOUNDARY. Anyone who can press this
button can uninstall and reinstall, or delete the directory by hand. It exists
so someone who forgot a password or wants to change the main login does not have
to. A guard that refuses is protecting nothing — the same person achieves the
same result with a file manager, slower.
We do NOT inherit CIRISAgent's fix #5 for this. Deliberate, and recorded here
rather than left as a silent gap: their guard answers a question the label now
answers better.
One redaction fix rides along, because the test that caught it was already
failing: the state exemption's terminator set omitted the quote characters that
the unquoted arm absorbs into its delimiter group, so `token="expired"` was
redacted as though `expired` were a credential.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc1920f6a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "login_reset_device_cancel": "Cancel", | ||
| "login_reset_device_confirm_body": "This will erase all local data and return to the setup wizard. This cannot be undone.", | ||
| "login_reset_device_confirm_title": "Reset device?", | ||
| "login_reset_device_confirm_body": "This erases the node data stored on THIS device and returns to the setup wizard. A node you are connected to on another machine is not affected. This cannot be undone.", |
There was a problem hiding this comment.
For every non-English locale, the confirmation still ships the old promise that reset erases “all local data” (for example, es.json:1863 and ja.json:1863), omitting the new clarification that only this device’s node data is erased and a remote node is unaffected. Users of 28 of the 29 locales can therefore authorize this destructive action with materially incorrect scope information; run these three changed keys through the repository’s required translation pipeline and update all shipped bundles together.
AGENTS.md reference: AGENTS.md:L14-L17
Useful? React with 👍 / 👎.
|
|
||
| private val SECRET_PATTERNS: List<Pair<Regex, String>> = listOf( | ||
| // JWTs — the shape is unmistakable and never appears in prose. | ||
| Regex("""eyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}(\.[A-Za-z0-9_-]+)?""") to "<redacted:jwt>", |
There was a problem hiding this comment.
When a raw valid JWT has a short claims segment, such as the base64url encoding e30 for an empty claims object, the {8,} requirement on the second segment prevents this pattern from matching at all. If that token appears in a log without an accompanying Bearer or credential-field marker, DebugBundle.render() exports the complete signed credential; recognize JWT segments without imposing this payload-length floor.
Useful? React with 👍 / 👎.
`eyJ` IS the shape check — that is a base64url `{"` and nothing else begins that
way by accident. Requiring 8+ characters in each segment meant a JWT with a
small claims set was exported whole (Codex, PR #18).
Third length floor in this file, after `{6,}` on named assignments and `{12,}`
on bearer headers. Same mistake each time: treating value length as evidence
when the marker beside it was already the evidence.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…9.41 # Conflicts: # client/VENDORING.md
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Two evaluate runs, different languages failing each time, one cause:
run 1 ha dropped "local"
my added a "wipe/clear" step not in the source
run 2 bn rendered it "local DATA" — narrowing the scope
ha dropped "local"
th dropped "local"
"Reset local device" is ambiguous in the ENGLISH. Does "local" modify the device
— this one, as opposed to the node on another machine — or the data on it?
Translators picked both readings and were marked wrong for each. `ha` and `th`
were rejected for rendering it "this device", which is exactly what it means.
So say that. The confirmation title has said "Reset this device?" since the
rename; the button now matches it, and the body already carries the contrast the
adjective was straining to imply: a node you are connected to on another machine
is not affected.
Third time in this session the translation blocker was the English, and the
third time the fix made the string simpler. A word that needs a footnote in one
language needs one in twenty-eight.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Machine translation, independently reviewed against MQM, and repaired where the review found a critical, major or terminology error. Every value here is status=draft / review_status=needs_native_review: this pipeline guarantees terminology, structure and meaning, and does not guarantee native fluency. Validated by check_localization_sync.py --strict in this same run. The MQM findings are attached to the run as i18n-report.json. Review like any other diff.
…9.41 # Conflicts: # client/VENDORING.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d370ef13bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) to "\$1\$2'<redacted>'", | ||
| // UNQUOTED name = value. Whitespace really is the delimiter here. | ||
| Regex( | ||
| """\b($NAME_PREFIX(?:$SECRET_NAMES))(\s*["']?\s*[:=]\s*["']?)""" + |
There was a problem hiding this comment.
When a quoted credential is truncated or logged without its closing delimiter, such as password="correct horse battery staple, both quoted arms fail and this fallback consumes the opening quote but stops the value at the first space. The bundle therefore contains password="<redacted> horse battery staple, exposing most of the credential. Handle an unmatched opening quote by redacting through the end of the line rather than treating it as an unquoted assignment.
Useful? React with 👍 / 👎.
| fun render(extra: Map<String, String> = emptyMap()): String = | ||
| redactSecrets(renderRaw(extra)) |
There was a problem hiding this comment.
When a FailurePanel receives credential-shaped text in its raw server failure detail, clicking “Open a GitHub issue” still passes that detail directly through reportUrl() into a potentially public issue body. This change scrubs only DebugBundle.render(), while FailurePanel.kt:325-330 independently builds its report from DebugBundle.environment() and the unredacted detail; apply the same redaction at that export path or expose a single scrubbed report builder.
Useful? React with 👍 / 👎.
| """refreshed|required|rejected|absent|ok|set|unset|none|null|""" + | ||
| """true|false|yes|no|enabled|disabled|empty|blank|valid""" | ||
|
|
||
| private val SECRET_PATTERNS: List<Pair<Regex, String>> = listOf( |
There was a problem hiding this comment.
On a fresh unclaimed node, when the ownership banner is among the callback lines retained by StartupViewModel.kt:184-190, the runtime forwards and logs the full line shaped like CLAIM PIN: 7F3K-Q9MZ (PythonRuntime.desktop.kt:488-490). None of these patterns recognizes either the CLAIM PIN label or its strict token shape, so downloading the startup debug bundle can expose the still-live one-time credential needed to claim the node; add a dedicated claim-PIN redaction pattern.
Useful? React with 👍 / 👎.
…th was never redacted
I told this repo's owner that nothing in this client logs a credential — that I
had checked rather than assumed. I had checked the wrong thing. My grep looked
for a credential-named variable interpolated into a log call; it cannot see a
RAW LINE forwarded verbatim that happens to contain one.
StartupViewModel:189 PlatformLogger.d(TAG, "... Line #$n: ${line.take(100)}")
Logger.desktop:16 DebugLogBuffer.add("DEBUG", tag, message)
node banner ║ CLAIM PIN: 7F3K-Q9MZ (one-time; console-only)
Every node stdout line goes through PlatformLogger — first 50, then every tenth
— PlatformLogger feeds DebugLogBuffer, and the ownership banner is emitted in
the node's first lines on a fresh unclaimed node. So the live one-time secret
that grants ownership sits in a bundle designed to be mailed to strangers, and
no pattern matched it, because it is a LABELLED BANNER and every arm was written
for `name=value`.
The redaction is not guarding a hypothetical. It never was.
Matched on label AND shape — 8 Crockford base32 characters as XXXX-XXXX, the
alphabet `PythonRuntime.desktop.CLAIM_PIN_REGEX` parses — so an ordinary dashed
token in prose survives. The NodeCode on the neighbouring line is deliberately
left alone: it is a public bootstrap handle and the reader needs it.
AND THE OTHER EXPORT WAS NEVER SCRUBBED AT ALL. `FailurePanel.reportUrl()`
builds its body from `DebugBundle.environment()` plus the RAW server detail and
opens a GitHub issue — a public destination, worse than the bundle, which is at
least a file the user chooses to send. `render()` scrubbed its own output while
this path passed the same text through untouched, because the scrubbing was
attached to a FUNCTION rather than to leaving the device. Now both go through
`redactSecrets`.
Third: an opening quote with no closing one — a truncated log line — fell to the
unquoted arm and stopped at the first space. An unmatched quote means the value
runs to end of line, so it is redacted that far.
19 redaction cases. The claim PIN is tested in both directions, and so is the
NodeCode beside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50e21c1c3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """\b($NAME_PREFIX(?:$SECRET_NAMES))(\s*[:=]\s*)(["'])(?!(?:$NOT_A_STATE)["']?${'$'})([^"'\n]+)${'$'}""", | ||
| setOf(RegexOption.IGNORE_CASE, RegexOption.MULTILINE), |
There was a problem hiding this comment.
When a truncated quoted credential contains an escaped delimiter or the opposite quote, such as password="correct \"horse\" battery staple or password="correct horse's battery staple, this arm fails because its value class excludes both quote types and does not consume escape sequences. The unquoted fallback then redacts only the first whitespace-delimited word, leaving most of the credential in the exported bundle; make this delimiter-specific and consume escaped characters through the line end.
Useful? React with 👍 / 👎.
| "login_reset_device_cancel": "Cancel", | ||
| "login_reset_device_confirm_body": "This will erase all local data and return to the setup wizard. This cannot be undone.", | ||
| "login_reset_device_confirm_title": "Reset device?", | ||
| "login_reset_device_confirm_body": "This erases the node data stored on THIS device and returns to the setup wizard. A node you are connected to on another machine is not affected. This cannot be undone.", |
There was a problem hiding this comment.
When desktop is connected to a remote profile, this new promise is false: CIRISApp.kt:2093-2105 still obtains getNodeHomePath() from the active remote apiClient and passes that remote machine's path to the local filesystem wipe. If that path exists locally, reset deletes allow-listed directories from an unrelated local path; otherwise it can report success without clearing this device's node. Fresh evidence in this revision is the explicit device-local wording and removal of the remote ownership machinery, so resolve the local runtime's home instead of using the active remote node's declaration.
Useful? React with 👍 / 👎.
Renaming the button made the promise explicit and left the implementation pointing somewhere else. `getNodeHomePath()` queried `$baseUrl` — the ACTIVE node. Connected to a remote profile, "Reset this device" asked THAT MACHINE where its home is and handed the answer to a LOCAL filesystem wipe: deleting allow-listed directories from an unrelated local path if it happened to exist here, and otherwise reporting success having cleared nothing on the device the button names (Codex, PR #18). It now takes the node URL and defaults to LOCAL_NODE_URL, and the reset passes it explicitly. That is also the reason no ownership guard is needed: the scope is fixed by WHAT WE QUERY rather than policed after the fact. Removing the guard was right; leaving the home resolution pointed at the active node was not, and the guard had been hiding it. Also: the unterminated-quote arm I added in the previous commit used the `[^"']` value class the terminated arms had already been fixed away from, so a truncated secret containing an apostrophe or an escaped quote fell through to the first-space truncation. One arm per delimiter, consuming escapes, like the terminated pair. Same bug, one arm later. 23 redaction cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c5262057f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // not policed after the fact. | ||
| val h = runCatching { | ||
| apiClient.getNodeHomePath( | ||
| ai.ciris.mobile.shared.api.CIRISApiClient.LOCAL_NODE_URL |
There was a problem hiding this comment.
When desktop uses the supported CIRIS_API_URL override, both Main.kt:185-188 and PythonRuntime.desktop.kt:45-52 point the device-local node at that URL, but this query still goes to the hardcoded 127.0.0.1:4243. If the actual node was started externally with a different home, the query returns null and resolveNodeHome() guesses from the client's environment, potentially deleting generated state from an unrelated local installation while leaving the intended node untouched. Fresh evidence in this revision is the explicit LOCAL_NODE_URL argument added while claiming to query the device-local node; pass the configured immutable nodeBaseUrl instead.
Useful? React with 👍 / 👎.
| // secret containing an apostrophe or an escaped quote fell through to | ||
| // the same first-space truncation. Same bug, one arm later. | ||
| Regex( | ||
| """\b($NAME_PREFIX(?:$SECRET_NAMES))(\s*[:=]\s*)["](?!(?:$NOT_A_STATE)["]?${'$'})((?:[^"\\\n]|\\.)+)${'$'}""", |
There was a problem hiding this comment.
When a truncated quoted credential ends with a lone backslash, such as password="correct horse\, the \\. alternative cannot consume that final character because it requires another character after the backslash, while the other alternative excludes backslashes. This unterminated arm therefore fails and the unquoted fallback redacts only correct, exporting horse\; the single-quoted arm has the same problem. Fresh evidence in this revision is the new escape-consuming unterminated pattern, which should also accept a terminal backslash through line end.
Useful? React with 👍 / 👎.
| file under `client/` except this one: | ||
|
|
||
| **state digest:** `be1f4ba8990a3b289cc30f414e91ea7524fd1444160340308ffb8dc5a25dba68` | ||
| **state digest:** `0ff13365d8b1b367cd9bc382f0b89e2bc51200dc0629ed93523b049020e147a8` |
There was a problem hiding this comment.
This commit says it inherited CIRISAgent 2.9.41's client tree and re-seals that result with this digest, but the provenance table at VENDORING.md:25 still records 2.9.36 as the agent's last merged state. Section 8 starts the next upstream pull from exactly that recorded commit, so the next pull will replay the already-integrated 2.9.37–2.9.41 changes and can produce avoidable conflicts or incorrect resolutions instead of using the actual branch point; update the agent state, input commit, and pull date alongside the digest.
Useful? React with 👍 / 👎.
| // closing delimiter and stops there, leaving the rest of the passphrase | ||
| // in the bundle — the same leak again, through the escape this time. | ||
| Regex( | ||
| """\b($NAME_PREFIX(?:$SECRET_NAMES))(["']?\s*[:=]\s*)["](?!(?:$NOT_A_STATE)["])((?:[^"\\]|\\.)+)["]""", |
There was a problem hiding this comment.
When one log entry contains an unterminated credential and a later entry contains the same quote character, this value class consumes newlines, so the terminated arm matches across entries before either multiline unterminated arm can run. For example, password="secret\n[INFO] user said "hello" redacts the intervening diagnostic line along with the secret, and a distant closing quote can remove a large portion of the bundle. Exclude line terminators in the terminated quoted arms so a clipped credential is handled by the dedicated end-of-line patterns instead.
Useful? React with 👍 / 👎.
1 participant
Closes #11. CIRISAgent cut v2.9.41-stable and kept working on the client after our 0.5.191 tag.
Verified against the tree, not the list: five files differed from theirs, three test files were absent entirely.
Already had
1. Log-buffer race — fixed here in PR #12 from the Codex review. Their implementation is semantically identical (only the id seed differs: theirs starts at 0, ours at 1). Their concurrency test still comes across, because a fix without the test is a claim.
Did not have
render()— the one choke point all three screens pass throughIS_PENDING.testable()before.verticalScroll()/treeadvertised an element taller than any screen — and our own walk-test harness reads/treewipeGeneratedStateextractedFix 5 is composed with our declared-home resolution rather than replacing it — asking the node where it lives is right when the node is ours, and this decides whether it is.
And one neither tree had fixed
#6, which the issue flagged and left with us.
getAppVersion()prefers the JAR manifest; the Compose uber-jar writes onlyMain-Class, so the fallback is the only value desktop ever reports. As a hand-maintained literal it said2.3.2while this repo shipped0.5.191— in the one artifact whose entire job is to say what was running.Their advice was "if you keep a hand-maintained constant, put a check on it." We don't keep it: the value is now the generated
CLIENT_VERSION, so there's no second number to drift and no check needed for one that can't exist.Mutation-tested
Inherited tests are someone else's evidence until they've been watched fail here.
redactSecrets→ identityownsLocalBackend→ always trueBoth revert clean.
:shared:desktopTestgreen (3 + 5 + 5 inherited, plus a new desktop-version test), wasmJs compiles, localization and vendoring pass.One note back
Your closing point doesn't apply to us — CIRISAgent's CI ran zero Kotlin tests, but ours has run
:shared:desktopTestsince the 0.5.188 matrix, which is how the 390 incommonTeststay honest. As of #10 it also walks the running app against four node configurations.🤖 Generated with Claude Code
https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1