ci: validate PR 4062 exact head 07d8aadc

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,93 @@
+# 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
+
+## 5. A Connect trailer carries no status — closed
+
+Landed in `connectTrailerHttpStatus`. The three EOS trailer throw sites now pass a status,
+so a cap delivered as `permission_denied` with "your limit will reset" reads as 429 rather
+than 403, an `unauthenticated` trailer reaches the auth path, and an unrecognised code still
+falls back to message inference. `unimplemented` maps to 501 and is explicitly non-retryable,
+because the blanket 5xx rule was telling clients to retry a call the service does not
+implement.
+
+Accepted residuals: `internal`, `unknown` and `data_loss` map to 502 rather than Connect's
+500 — both are transient here and 502 is what this adapter already reported — and a genuine
+ACL denial whose text happens to contain the words "rate limit" would be read as a cap. The
+regex reads the raw trailer message, never the enriched text, so the tool-description
+blocklist wrapper cannot trip it.

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.
+

gui/src/components/codex-account-pool-cards.tsx

@@ -83,6 +83,20 @@ export function CodexAccountPoolCards({
         const validationPending = a.health?.reason === "validation_pending";
         const healthLabel = formatOAuthHealthLabel(t, a.health);
         const healthSummary = formatOAuthHealthSummary(t, "codex", a.id, a.health);
+        const hasCustomPriority = normalizeAccountPriority(a.priority) !== DEFAULT_ACCOUNT_PRIORITY;
+        const priorityControl = (
+          <AccountPriorityControl
+            value={a.priority}
+            selectId={`codex-account-priority-${a.id}`}
+            // Every row, not just the one being written: the controller serializes order
+            // writes behind one mutation ref, so a second row's pick would come back "busy"
+            // and be dropped with no toast. Same global lock the pause button uses.
+            // A pending switch counts too — it writes the same pin this clears, so the
+            // controller refuses to overlap them, and that refusal is equally silent.
+            disabled={priorityUpdatingId !== null || switchingId !== null}
+            onChange={(priority) => onPriorityChange(a, priority)}
+          />
+        );
         return (
         <div key={a.id} className={`card ${isNext(a) ? "card-active" : ""}`} style={{ marginBottom: 8 }}>
           <div className="card-head">
@@ -153,6 +167,7 @@ export function CodexAccountPoolCards({
             >
               <summary className="btn btn-ghost btn-sm" aria-label={`${t("codexAuth.moreActions")} — ${a.email}`} title={t("codexAuth.moreActions")}>⋯</summary>
               <div className="codex-account-more-body">
+                {!hasCustomPriority && moreOpen.has(a.id) && priorityControl}
                 <span className="mono text-caption muted">{t("prov.accountId")}: {displayAccountId(a.id)}</span>
                 <button type="button" className="btn btn-ghost btn-sm" onClick={() => idCopy.copy(a.id, a.id)}>
                   {idCopy.outcomeFor(a.id) === "copied" ? t("startup.copied") : t("codexAuth.copyId")}
@@ -174,19 +189,7 @@ export function CodexAccountPoolCards({
           </div>
           <div className="codex-account-identity">
             <div className="codex-account-identity-copy">{a.email}{a.plan ? ` · ${a.plan}` : ""}</div>
-            {(normalizeAccountPriority(a.priority) !== DEFAULT_ACCOUNT_PRIORITY || moreOpen.has(a.id)) && (
-            <AccountPriorityControl
-              value={a.priority}
-              selectId={`codex-account-priority-${a.id}`}
-              // Every row, not just the one being written: the controller serializes order
-              // writes behind one mutation ref, so a second row's pick would come back "busy"
-              // and be dropped with no toast. Same global lock the pause button uses.
-              // A pending switch counts too — it writes the same pin this clears, so the
-              // controller refuses to overlap them, and that refusal is equally silent.
-              disabled={priorityUpdatingId !== null || switchingId !== null}
-              onChange={(priority) => onPriorityChange(a, priority)}
-            />
-            )}
+            {hasCustomPriority && priorityControl}
           </div>
           {healthSummary && (
             <div className="card-sub faint">{healthSummary}</div>