fix(devin-cli): harden the imported CLI session against path, read, and cache defects

devlog/_plan/260912_devin_hardening/000_plan.md

@@ -0,0 +1,63 @@
+# 260912 — Devin hardening and cached-token display
+
+## Why this unit exists
+
+`devin-cli` landed as a working provider in `devlog/_fin/260912_devin_cli_account_login/`:
+a signed-in local Devin CLI credentials.toml is imported as an OAuth account, and inference
+goes to the Cognition cloud endpoint through the cloud-direct adapter rather than through an
+ACP stdio loop. That unit proved the path works. It did not harden it.
+
+Two things are outstanding.
+
+The first is the auth and transport path itself. The import reads one file with two regexes,
+the session token has no modelled expiry, and the cloud-direct client's failure classification
+is thin enough that an operator cannot tell a revoked credential from a rate limit from a
+protocol drift. The adapter decodes a reverse-engineered protobuf frame, and a truncated or
+reshaped frame is a class of failure the current code does not name.
+
+The second is unrelated to Devin and was raised alongside it: a cached request's token total
+is displayed without its cached companion on several surfaces. The logs table already renders
+a total with a stacked cached line, and the surfaces that do not do this look like they are
+reporting a different number rather than the same number without its breakdown.
+
+## Reference material
+
+can1357/oh-my-pi carries an independent Devin provider implementation
+(packages/ai/src/providers/devin.ts, packages/ai/src/usage/devin.ts,
+packages/catalog/src/discovery/devin.ts, packages/catalog/src/wire/devin.ts) plus generated
+proto descriptors for the same Cognition surface. It is cloned read-only into .tmp/ref/oh-my-pi
+and is never vendored, imported, or copied: it is a second observation of the same wire
+protocol, used to decide which of our assumptions are load-bearing and which are guesses that
+happened to hold. Its open pull requests are read the same way.
+
+## Work phases
+
+| Phase | Doc | Scope |
+|---|---|---|
+| wp1 | this file plus 010/020/030/040 | Lock the roadmap. Docs only. |
+| wp2 | 010_cli_token_transition.md | CLI credential import and token transition hardening. |
+| wp3 | 020_cloud_direct_hardening.md | Cloud-direct transport, usage, and catalog hardening. |
+| wp4 | 030_cached_token_display.md | Cached companion on every total-bearing surface. |
+| wp5 | 040_stacked_delivery.md | Stacked PR chain, exact-head CI, merge into dev. |
+
+wp2 and wp3 are sequential because they share src/oauth/devin/api-base.ts and the account
+record shape. wp4 is independent of both and touches only gui/src and src/cli, so it is a
+sibling branch in the stack rather than a child.
+
+## Out of scope
+
+- The Devin session product (cog_ keys, agent VMs). credentials.toml carries devin_webapp_host
+  and devin_api_url for it; neither is inference and neither is read.
+- Any change to src/adapters/devin-cli/acp.ts stdio behaviour beyond failure classification.
+  The cloud-direct route is the one that serves traffic.
+- Vendoring anything from the reference clone.
+
+## Constraints carried into every later phase
+
+- Bun-native TypeScript. No Node-only API that Bun does not implement.
+- bun run privacy:scan stays green. A devin session token is not recognised by
+  redactSecretString, so no error path may echo a request body or a parsed credential.
+- Behaviour changes in src/ get a focused regression test next to the existing
+  tests/providers/devin-*.test.ts files.
+- Every new test file needs an entry in scripts/test-layout/layout.json and
+  tests/fixtures/test-layout-expected.json.

devlog/_plan/260912_devin_hardening/010_cli_token_transition.md

@@ -0,0 +1,53 @@
+# wp2 — Devin CLI token transition hardening
+
+Branch: codex/260912-devin-cli-token-transition (base dev)
+
+## What the path does today
+
+ocx login devin-cli reads credentials.toml from the CLI data dir, pulls windsurf_api_key and
+api_server_url with two line regexes, validates the host, and stores an OAuth account whose
+expiry is Number.MAX_SAFE_INTEGER and whose refresh throws invalid_grant. Inference then runs
+through the cloud-direct Connect client, not through core's OAuth replay path.
+
+## Defects to fix
+
+1. The session token prefix is never normalized. Every Cognition RPC expects
+   devin-session-token$<JWT>. A credential arriving without it (OPENCODEX_DEVIN_TEST_TOKEN, a
+   pasted bare JWT, a provider apiKey typed by hand) is sent verbatim and returns an opaque
+   permission_denied, which reads as a revoked account rather than a malformed credential.
+   oh-my-pi normalizes at the metadata boundary (packages/catalog/src/wire/devin.ts). We do
+   not. Fix: one normalizer applied where Metadata.apiKey is built, plus a unit test.
+
+2. An empty APPDATA or XDG_DATA_HOME resolves to a cwd-relative path.
+   src/oauth/devin-cli.ts uses env.APPDATA ?? join(homedir(), ...), and "" is a set value, so
+   join("", "devin", "credentials.toml") yields devin/credentials.toml relative to whatever
+   directory the proxy runs in. A file planted there imports as the operator's CLI session.
+   Fix: treat an empty or whitespace-only value as unset.
+
+3. The credential file is read whole with no bound and every I/O failure collapses to
+   "not signed in". EACCES, EISDIR, and a missing file are indistinguishable, so the one error
+   message the caller owns cannot name the actual recovery step. Fix: cap the read, and
+   separate missing from unreadable without putting file bytes into any thrown value.
+
+4. Logout clears the shared user-JWT and catalog cache only for provider "devin".
+   src/server/management/oauth-account-routes.ts gates the clear on that exact id, so logging
+   out of devin-cli leaves a cached api_key-bearing JWT in process memory for its whole TTL,
+   and account deletion never clears it at all. devin and devin-cli share the same cache.
+   Fix: cover both provider ids on both paths.
+
+5. A Connect EOS trailer message is echoed verbatim into the client error and /api/logs.
+   The HTTP-status paths deliberately refuse to echo bodies because a Connect error can quote
+   the request that carries the key; the trailer path then does the opposite. redactSecretString
+   recognises neither devin-session-token$... nor a bare JWT. Fix: add both patterns to the
+   redactor so anything that does reach a log is masked.
+
+## Non-goals
+
+The app.devin.ai PKCE CLI OAuth flow. The import path is the intended substitute and a second
+login protocol is its own unit. Also excluded: probing the key at import time, which changes
+login latency and deserves its own decision.
+
+## Verification
+
+bun test tests/providers/devin-cli-login.test.ts tests/providers/devin-cli-authmode-migration.test.ts tests/providers/devin-hardening.test.ts
+plus bun run privacy:scan.

devlog/_plan/260912_devin_hardening/020_cloud_direct_hardening.md

@@ -0,0 +1,78 @@
+# wp3 — Devin cloud-direct hardening
+
+Branch: codex/260912-devin-cloud-direct-hardening (base codex/260912-devin-cli-token-transition)
+
+## 1. Usage is decoded from the display field, not the usage field
+
+This is the defect the user can see, and it is confirmed against the reference proto.
+
+decodeUsageBlock in src/adapters/devin/cloud-direct/chat.ts treats GetChatMessageResponse
+field 28 as a usage block keyed by metric-id strings. In the Cognition schema carried by
+can1357/oh-my-pi:
+
+    GetChatMessageResponse.usage                      = 7   (ModelUsageStats)
+    GetChatMessageResponse.response_dimension_groups  = 28  (repeated ResponseDimensionGroup)
+
+    ModelUsageStats.input_tokens       = 2   uint64 varint
+    ModelUsageStats.output_tokens      = 3   uint64 varint
+    ModelUsageStats.cache_write_tokens = 4   uint64 varint
+    ModelUsageStats.cache_read_tokens  = 5   uint64 varint
+
+Field 28 is not an older usage shape. It is the current display message:
+ResponseDimensionGroup is {title, dimensions}, and ResponseDimension.uid is field 5 — which is
+exactly the sub-field today's decoder reads as metric_id. So the existing decoder works by
+reading presentation rows whose uid happens to spell the metric, and it yields cache numbers
+only when the server chose to render cache rows. Field 7 carries them unconditionally.
+
+Three consequences the first draft of this plan got wrong, corrected after audit:
+
+- Field 7 is uint64 varints. The existing entry walker only descends length-delimited
+  sub-messages and reads a fixed32 float, so it cannot read field 7 at all. Field 7 needs its
+  own decoder.
+- "Decode both, field 7 wins" is not what decoding both produces. Both fields arrive in the
+  same response and src/adapters/devin.ts replaces usage on every usage event, so a naive
+  addition lets field 28 land last and win. Within one message, field 7 must suppress
+  field 28 outright; field 28 stays only as the fallback for a message that carries no field 7.
+- The adapter must merge usage fields across events rather than replacing the object, so a
+  later partial frame cannot zero an earlier input count.
+
+## 2. Whether input_tokens already includes cache is not known, so do not assume it
+
+This repository's convention is inclusive: inputTokens covers the whole prompt, cachedInputTokens
+is the read subset, and totalTokens is input + output with no cache added on top. Adapters split
+on what the wire gives them — anthropic.ts and kiro-events.ts fold cache into input because their
+wire format is exclusive, while openai-responses.ts passes input_tokens through because it is
+already inclusive.
+
+oh-my-pi summing input + output + cacheRead + cacheWrite is evidence that Devin might be
+exclusive. It is not proof, and guessing wrong in the inclusive direction silently inflates
+input and bills cache at the uncached rate, because normalizeCostTokens only rejects
+read + write > input.
+
+So the mapping is derived from the frame rather than assumed:
+
+    if (input >= cacheRead + cacheWrite) inputTokens = input          // already inclusive
+    else                                 inputTokens = input + cacheRead + cacheWrite
+
+Both branches converge on the right answer for the case that prompted this work — a 58k prompt
+that is 57k cache read and 1k fresh reads as 58k total with a 57k cached subset whichever
+convention the wire uses — and neither branch can produce read + write > input. The heuristic
+is written down in the code with that reasoning, and replaced with a fixed mapping the moment a
+live ModelUsageStats frame settles the question.
+
+## 3. An HTTP status never reaches the classifier
+
+CloudChatError is thrown as "GetChatMessage failed (HTTP <status>)" with no status field, so a
+401 on a revoked import is a generic adapter failure rather than an authentication error, and
+inferHttpStatusFromAdapterMessage turns an HTTP 429 into a 502 — which means core's failover
+never rotates or backs off. Fix: carry status on the error and map 401, 403, 429 and 5xx.
+
+## 4. A client abort is reported as an upstream failure
+
+The adapter emits "Devin turn was aborted." with no status, and isClientClosedMessage does not
+recognise that wording, so a cancelled turn infers 502. Fix: emit the phrase the classifier
+already knows, with status 499.
+
+## Verification
+
+bun test tests/providers/devin-adapter.test.ts tests/providers/devin-hardening.test.ts

devlog/_plan/260912_devin_hardening/030_cached_token_display.md

@@ -0,0 +1,54 @@
+# wp4 — Cached-token companion on every total
+
+Branch: codex/260912-cached-token-companion (base dev, sibling of the Devin chain)
+
+## The complaint
+
+A cached request whose total is 58,000 tokens is about 57,000 cache-read plus 1,000 fresh.
+The Logs table row already renders that as a total with a stacked "c 5.7만". Every other
+surface prints a bare 5.8만, which reads as a different, smaller request rather than the same
+request with its breakdown hidden. The conversation-totals banner sits directly above rows
+that do show the companion, so the mismatch is visible in one screenshot.
+
+## Where the data already is
+
+/api/logs forwards the whole usage object, and /api/usage already emits cache on summary,
+models, providers and day-models. No backend change is needed. The loss is client-side, and it
+is not only the GUI row types: Usage's UsageModel and UsageProvider, the dashboard's
+UsageSummary30d, summarizeFilteredLogs in Logs.tsx, and the CLI's CostRow each drop the fields
+before they reach a renderer.
+
+## Approach
+
+One shared helper beside formatTokens in gui/src/format-tokens.ts:
+
+    formatTokensWithCache(total, cached, locale) -> "5.8만 c5.7만"
+
+It returns the bare total when cached is undefined or zero. It does not hide the companion when
+cached equals the total: an all-cache turn with no fresh input is exactly the case worth
+showing, and suppressing it would blank the most cached request on the page. The "c" marker
+matches the existing logs.tokens.cacheRead label, which already reads "cache read (c)", so no
+new i18n key is needed.
+
+Surfaces to convert, in order of how visible the mismatch is:
+
+1. Logs conversation-totals banner — summarizeFilteredLogs also sums cacheSplit(entry).read.
+2. Usage per-model and per-provider token columns — widen the row types to keep the cache
+   fields the API already sends.
+3. Dashboard 30-day total tile — widen UsageSummary30d the same way.
+4. CLI usage report provider/model/account rows, matching the summary line that already
+   prints "cached N".
+
+The log detail panel is deliberately left alone: it already has separate cache read and cache
+write cells, so stacking the companion onto its total would duplicate them.
+
+## CI gate
+
+missing_ui_screenshot in .github/scripts/pr-quality.cjs is path-based: touching gui/src trips
+it whether or not the description says "gui". This PR therefore carries a real screenshot of
+the changed surface, produced from a build of this branch served by a throwaway proxy instance
+on its own port and its own OPENCODEX_HOME, so the operator's running service is untouched.
+
+## Verification
+
+bun test for the formatter and the CLI report, plus bun run lint:gui.

devlog/_plan/260912_devin_hardening/040_stacked_delivery.md

@@ -0,0 +1,24 @@
+# wp5 — Stacked delivery
+
+Four branches, each one PR, chained so a reviewer sees one concern at a time.
+
+    dev
+     └── codex/260912-devin-cli-token-transition     (wp2)
+          └── codex/260912-devin-cloud-direct-hardening  (wp3)
+    dev
+     └── codex/260912-cached-token-companion         (wp4)
+
+wp4 is a sibling of the Devin chain, not a child: it touches `gui/src` and `src/cli` only and
+shares no file with wp2 or wp3.
+
+Rules carried from the repository:
+
+- Every PR fills `.github/PULL_REQUEST_TEMPLATE.md` in full and targets its parent branch;
+  children retarget to `dev` once the parent lands.
+- Pushes use `--no-verify`; the local product suite is not run. Remote CI on the exact final
+  head is the evidence, and any skipped local check is labelled NOT RUN.
+- Merges into `dev` are serialized, parent first, and each child is rebased onto the moved
+  parent before its own merge.
+- A PR whose title or description mentions `gui` needs a screenshot, so wp4's description
+  avoids that word unless a screenshot is attached.
+

src/adapters/devin/cloud-direct/metadata.ts

@@ -55,6 +55,33 @@ const CLOUD_CHAT_OS = 'windows';
  */
 const DEVICE_FINGERPRINT_BYTES = 366;
 
+/** Prefix every Cognition session key carries in `Metadata.api_key`. */
+const DEVIN_SESSION_TOKEN_PREFIX = 'devin-session-token$';
+
+/** A bare JWT: three base64url segments. Nothing else is reshaped. */
+const BARE_JWT_PATTERN = /^[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]*$/;
+
+/**
+ * Restore the `devin-session-token$` prefix on a bare JWT.
+ *
+ * Cognition reads `Metadata.api_key` as a prefixed session token. A key that
+ * arrives without the prefix — a JWT pasted into `apiKey` by hand, or one
+ * copied out of the CLI's file without its prefix — is sent verbatim and comes
+ * back as an opaque `permission_denied`, which reads as a revoked account
+ * rather than as a malformed credential.
+ *
+ * Only a bare JWT is reshaped. The other key formats this field has carried are
+ * not JWTs and must pass through untouched: a Codeium-classic bare UUID, an
+ * `sk-ws-01-…` Windsurf key, and a `cog_…` session key would all break if they
+ * were prefixed. Anything already containing `$` is left alone for the same
+ * reason.
+ */
+export function normalizeDevinSessionToken(apiKey: string): string {
+  const trimmed = apiKey.trim();
+  if (!trimmed || trimmed.includes('$')) return apiKey;
+  return BARE_JWT_PATTERN.test(trimmed) ? `${DEVIN_SESSION_TOKEN_PREFIX}${trimmed}` : apiKey;
+}
+
 export interface MetadataInput {
   /** Persistent api_key from OAuth (`devin-session-token$<JWT>`). */
   apiKey: string;
@@ -100,12 +127,14 @@ function osString(): string {
 export function buildMetadata(input: MetadataInput): Buffer {
   const version = input.windsurfVersion ?? WINDSURF_VERSION_STRING;
   const os = input.osName ?? osString();
+  // One boundary, so no caller has to remember the prefix rule.
+  const apiKey = normalizeDevinSessionToken(input.apiKey);
   if (input.cloudChatShape) {
     const clientVersion = input.windsurfVersion ?? CLOUD_CHAT_CLIENT_VERSION;
     return Buffer.concat([
       encodeString(1, CLOUD_CHAT_CLIENT_NAME),
       encodeString(2, clientVersion),
-      encodeString(3, input.apiKey),
+      encodeString(3, apiKey),
       encodeString(4, 'en'),
       encodeString(5, input.osName ?? CLOUD_CHAT_OS),
       encodeString(7, clientVersion),
@@ -117,7 +146,7 @@ export function buildMetadata(input: MetadataInput): Buffer {
   const parts: Buffer[] = [
     encodeString(1, 'windsurf'),                     // ide_name
     encodeString(2, version),                         // extension_version
-    encodeString(3, input.apiKey),                    // api_key
+    encodeString(3, apiKey),                          // api_key
     encodeString(4, 'en'),                            // locale
     encodeString(5, os),                              // os
     encodeString(7, version),                         // ide_version

src/lib/redact.ts

@@ -252,6 +252,13 @@ const SECRET_VALUE_PATTERNS: Array<[RegExp, string]> = [
   [/((?:"(?:api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|refreshToken|accessToken|clientSecret|apiKey)"\s*:\s*"))([^"]+)(")/gi, `$1${REDACTED_SECRET}$3`],
   // Raw JSON "token" field values (Copilot token exchange bodies echo the credential here).
   [/(("token"\s*:\s*"))([^"]+)(")/gi, `$1${REDACTED_SECRET}$4`],
+  // Cognition/Devin session keys, and the bare JWTs several providers hand out.
+  // A Connect EOS trailer can quote the request that carried the key, and the
+  // rules above only fire on a label — `Bearer`, `api_key=`, `"token":` — which
+  // a quoted proto field does not have. `eyJ` is the base64url of `{"`, so the
+  // JWT rule needs a real three-segment shape and does not match ordinary prose.
+  [/\bdevin-session-token\$[A-Za-z0-9._~+/=-]{8,}/g, REDACTED_SECRET],
+  [/\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]*/g, REDACTED_SECRET],
   [/\b(arn:aws:[A-Za-z0-9_-]+:[A-Za-z0-9-]*:\d{12}:[A-Za-z0-9_/:+=,.@-]+)\b/g, REDACTED_SECRET],
 ];