diff --git a/.github/workflows/dev-version-bump.yml b/.github/workflows/dev-version-bump.yml new file mode 100644 index 0000000000..45ecd4ae3f --- /dev/null +++ b/.github/workflows/dev-version-bump.yml @@ -0,0 +1,170 @@ +name: Dev version bump + +# When a release publishes, open a pull request that moves `dev` past the published +# version. Without this, `dev` keeps carrying a version that is at or behind a released +# one, and `tests/release-version-line.test.ts` fails on `dev` and on every pull request +# opened against it - inherited red a contributor cannot fix from their own diff. +# +# That has been repaired by hand four times: 32529c2b2, e4a85d134, 076ad3036, befcac3e1. +# The second of those ADDED the detector and two more repairs followed it, so more +# visibility was never the missing piece; a prepared change was. +# +# WHAT THIS DOES NOT DO. It does not push to `dev`. It opens a pull request and a human +# merges it, because ruleset `Protect dev` requires an approving review and code-owner +# sign-off that a bot cannot supply. Until that merge the red persists. This converts a +# forgotten chore into a queued, reviewable change - not into an automatic repair. +# +# A `release` event resolves this workflow file from the repository DEFAULT branch +# (`main`), not from `dev` - the same trap documented in cleanup-closed-pr-branches.yml. +# So merging this file to `dev` installs it but arms nothing; it first fires after an +# ordinary dev -> main promotion carries it there. +# +# There is deliberately no `workflow_dispatch`: a branch-selected manual run executes +# THAT branch body with `contents: write`. Re-drive a missed run by running +# `bun scripts/bump-dev-version.ts package.json` locally and opening the pull +# request normally. +on: + release: + types: [published] + +permissions: {} + +concurrency: + group: dev-version-bump + cancel-in-progress: false + +jobs: + open-bump-pr: + runs-on: ubuntu-latest + permissions: + # Push the new codex/dev-version-* branch. Ruleset `Protect dev` covers only + # refs/heads/dev, so the bump branch is unprotected and this token cannot + # bypass dev review. It is the ruleset that keeps this job off dev, not the + # permission name. + contents: write + # Open the pull request. + pull-requests: write + steps: + - name: Checkout dev + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + ref: dev + # Tags are load-bearing, not decoration: the freeness gate below is a bun + # test that reads the local tag set, and release-version-line.test.ts + # returns EARLY on an empty set. A shallow checkout would make that gate + # silently vacuous instead of failing loudly. + fetch-depth: 0 + # Do NOT set persist-credentials: false here as the read-only workflows do. + # This job has to push its bump branch. + + # The repository-owned composite action, not a hand-pinned setup-bun SHA: it + # resolves the Bun version from package.json so the runtime SOT stays in one + # place. An independently pinned action here would drift from every other job. + - name: Setup project Bun + uses: ./.github/actions/setup-project-bun + + - name: Install dependencies + run: bun install --frozen-lockfile + + - name: Decide the version dev should carry + id: decide + env: + RELEASED_VERSION: ${{ github.event.release.tag_name }} + run: | + set -euo pipefail + bun scripts/bump-dev-version.ts "${RELEASED_VERSION}" package.json + + - name: Prove the chosen version is unused + if: ${{ steps.decide.outputs.changed == 'true' }} + # The script decides the candidate from the released version SHAPE, which is all + # a pure function can see. Whether that candidate is actually FREE is a property + # of the tag set, so it is settled here by the detector that already owns the + # question. If this fails, no pull request is opened and the job goes red asking + # for a human decision - which is the correct outcome, not a fallback. + run: bun test tests/release-version-line.test.ts + + - name: Open the bump pull request + if: ${{ steps.decide.outputs.changed == 'true' }} + env: + GH_TOKEN: ${{ github.token }} + NEXT_VERSION: ${{ steps.decide.outputs.version }} + RELEASED_VERSION: ${{ github.event.release.tag_name }} + run: | + set -euo pipefail + + branch="codex/dev-version-${NEXT_VERSION}" + + # Idempotent: a second publish, a re-run, or a manual repair must not turn a + # successful release into a red job. + # + # Check the PULL REQUEST as well as the branch, not just the branch. A security + # review caught that: an open bump pull request whose head branch was deleted + # leaves the branch check passing, so the job would recreate the branch and then + # fail on `gh pr create` with "already exists" — turning a successful release red + # for a repair that was already queued. + open_prs="$(gh pr list --base dev --head "${branch}" --state open --json number --jq 'length')" + if [ "${open_prs}" != "0" ]; then + echo "::notice::a bump pull request for ${branch} is already open; nothing to do" + exit 0 + fi + + # An existing branch is NOT terminal. If a previous run pushed the branch and then + # failed at `gh pr create`, exiting here would leave the repair permanently unqueued + # while every rerun reports success - the exact failure mode a reviewer caught. So + # reuse the branch and fall through to pull-request creation instead. + if git ls-remote --exit-code --heads origin "${branch}" >/dev/null 2>&1; then + echo "::notice::${branch} exists without an open pull request; validating it" + git fetch origin "${branch}" + + # Fail closed on unexpected content. The branch carries the bot's own one-line + # bump, so anything else on it means a human or another job is using that name and + # this job must not push to it or open a pull request from it. + changed_files="$(git diff --name-only "origin/dev...origin/${branch}")" + if [ "${changed_files}" != "package.json" ]; then + echo "::error::${branch} touches unexpected files: ${changed_files:-}" + exit 1 + fi + branch_version="$(git show "origin/${branch}:package.json" | node -p "JSON.parse(require('fs').readFileSync(0,'utf8')).version")" + if [ "${branch_version}" != "${NEXT_VERSION}" ]; then + echo "::error::${branch} carries ${branch_version}, expected ${NEXT_VERSION}" + exit 1 + fi + git checkout -B "${branch}" "origin/${branch}" + else + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git checkout -b "${branch}" + git add package.json + git commit -m "fix(release): move dev to ${NEXT_VERSION} after ${RELEASED_VERSION}" + git push origin "${branch}" + fi + + gh pr create \ + --base dev \ + --head "${branch}" \ + --title "fix(release): move dev to ${NEXT_VERSION} after ${RELEASED_VERSION}" \ + --body "$(cat < package.json`, + then open the pull request normally. ## The retired `dev2-go` line diff --git a/Start-OpenCodex.cmd b/Start-OpenCodex.cmd new file mode 100644 index 0000000000..ea8f81fa73 --- /dev/null +++ b/Start-OpenCodex.cmd @@ -0,0 +1,13 @@ +@echo off +setlocal + +powershell.exe -NoLogo -NoProfile -ExecutionPolicy Bypass -File "%~dp0Start-OpenCodex.ps1" %* +set "launcher_exit=%ERRORLEVEL%" + +if not "%launcher_exit%"=="0" ( + echo. + echo OpenCodex could not be started. Review the error above. + pause +) + +exit /b %launcher_exit% diff --git a/Start-OpenCodex.ps1 b/Start-OpenCodex.ps1 new file mode 100644 index 0000000000..b28e836573 --- /dev/null +++ b/Start-OpenCodex.ps1 @@ -0,0 +1,140 @@ +[CmdletBinding()] +param( + [ValidateRange(1, 65535)] + [int]$Port = 10100, + + [ValidateRange(1, 120)] + [int]$StartupTimeoutSeconds = 30, + + [switch]$NoBrowser +) + +$ErrorActionPreference = "Stop" +$repoRoot = $PSScriptRoot +$dashboardUrl = "http://127.0.0.1:$Port/" +$healthUrl = "${dashboardUrl}healthz" +$logDirectory = Join-Path $repoRoot ".tmp" +$stdoutLog = Join-Path $logDirectory "launcher.out.log" +$stderrLog = Join-Path $logDirectory "launcher.err.log" + +function Get-OpenCodexHealth { + try { + $response = Invoke-RestMethod -Uri $healthUrl -Method Get -TimeoutSec 2 + if ($response.service -eq "opencodex" -and $response.status -eq "ok") { + return $response + } + } + catch { + return $null + } + + return $null +} + +function Open-Dashboard { + if (-not $NoBrowser) { + Start-Process $dashboardUrl + } +} + +function Test-IsLocalCheckoutProcess { + param([Parameter(Mandatory = $true)][int]$ProcessId) + + try { + $runningProcess = Get-CimInstance Win32_Process -Filter "ProcessId=$ProcessId" + if ($null -eq $runningProcess) { + return $false + } + + if (-not [string]::IsNullOrWhiteSpace($runningProcess.ExecutablePath) -and + $runningProcess.ExecutablePath.StartsWith($repoRoot, [StringComparison]::OrdinalIgnoreCase)) { + return $true + } + + if ([string]::IsNullOrWhiteSpace($runningProcess.CommandLine)) { + return $false + } + + $expectedEntryPoint = Join-Path $repoRoot "src\cli\index.ts" + return $runningProcess.CommandLine.IndexOf($expectedEntryPoint, [StringComparison]::OrdinalIgnoreCase) -ge 0 + } + catch { + return $false + } +} + +$localBunExecutable = Join-Path $repoRoot "node_modules\bun\bin\bun.exe" +$bunApplication = Get-Command bun.exe -CommandType Application -ErrorAction SilentlyContinue +if (Test-Path -LiteralPath $localBunExecutable) { + $bunExecutable = $localBunExecutable +} +elseif ($null -ne $bunApplication) { + $bunExecutable = $bunApplication.Source +} +else { + throw "Bun was not found. Install Bun from https://bun.sh, then run this launcher again." +} + +if (-not (Test-Path -LiteralPath (Join-Path $repoRoot "node_modules"))) { + throw "Dependencies are missing. Open PowerShell in '$repoRoot', run 'bun install', then try again." +} + +$existingHealth = Get-OpenCodexHealth +if ($null -ne $existingHealth) { + if (Test-IsLocalCheckoutProcess -ProcessId $existingHealth.pid) { + Write-Host "This OpenCodex checkout is already running on port $Port (PID $($existingHealth.pid))." + Open-Dashboard + exit 0 + } + + Write-Host "A different OpenCodex installation is using port $Port (PID $($existingHealth.pid))." + Write-Host "Stopping it before starting this checkout..." + & $bunExecutable run src/cli/index.ts stop + if ($LASTEXITCODE -ne 0) { + throw "The existing OpenCodex instance could not be stopped safely." + } + + $stopDeadline = (Get-Date).AddSeconds(15) + do { + Start-Sleep -Milliseconds 250 + $existingHealth = Get-OpenCodexHealth + } while ($null -ne $existingHealth -and (Get-Date) -lt $stopDeadline) + + if ($null -ne $existingHealth) { + throw "The previous OpenCodex instance is still using port $Port." + } +} + +New-Item -ItemType Directory -Path $logDirectory -Force | Out-Null + +Write-Host "Starting OpenCodex on port $Port..." +$process = Start-Process ` + -FilePath $bunExecutable ` + -ArgumentList @("run", "src/cli/index.ts", "start", "--port", "$Port") ` + -WorkingDirectory $repoRoot ` + -WindowStyle Hidden ` + -RedirectStandardOutput $stdoutLog ` + -RedirectStandardError $stderrLog ` + -PassThru + +$deadline = (Get-Date).AddSeconds($StartupTimeoutSeconds) +do { + Start-Sleep -Milliseconds 250 + $process.Refresh() + + $health = Get-OpenCodexHealth + if ($null -ne $health) { + if (-not (Test-IsLocalCheckoutProcess -ProcessId $health.pid)) { + throw "Port $Port became healthy, but it belongs to a different OpenCodex installation." + } + Write-Host "OpenCodex is ready at $dashboardUrl (PID $($health.pid))." + Open-Dashboard + exit 0 + } + + if ($process.HasExited) { + throw "OpenCodex stopped during startup (exit code $($process.ExitCode)). See '$stderrLog'." + } +} while ((Get-Date) -lt $deadline) + +throw "OpenCodex did not become ready within $StartupTimeoutSeconds seconds. See '$stderrLog'." diff --git a/assets/pr2950-capacity-expiry.png b/assets/pr2950-capacity-expiry.png new file mode 100644 index 0000000000..10e3c995b9 Binary files /dev/null and b/assets/pr2950-capacity-expiry.png differ diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/000_rca.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/000_rca.md similarity index 100% rename from devlog/_plan/260829_cursor_tool_continuation_pairing/000_rca.md rename to devlog/_fin/260829_cursor_tool_continuation_pairing/000_rca.md diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/001_audit_round1.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/001_audit_round1.md similarity index 100% rename from devlog/_plan/260829_cursor_tool_continuation_pairing/001_audit_round1.md rename to devlog/_fin/260829_cursor_tool_continuation_pairing/001_audit_round1.md diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/002_audit_round2_redesign.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/002_audit_round2_redesign.md similarity index 100% rename from devlog/_plan/260829_cursor_tool_continuation_pairing/002_audit_round2_redesign.md rename to devlog/_fin/260829_cursor_tool_continuation_pairing/002_audit_round2_redesign.md diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/010_phase1_call_result_pairing.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/010_phase1_call_result_pairing.md similarity index 100% rename from devlog/_plan/260829_cursor_tool_continuation_pairing/010_phase1_call_result_pairing.md rename to devlog/_fin/260829_cursor_tool_continuation_pairing/010_phase1_call_result_pairing.md diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/020_phase2_tests_and_delivery.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/020_phase2_tests_and_delivery.md similarity index 100% rename from devlog/_plan/260829_cursor_tool_continuation_pairing/020_phase2_tests_and_delivery.md rename to devlog/_fin/260829_cursor_tool_continuation_pairing/020_phase2_tests_and_delivery.md diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/030_phase4_final_gate.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/030_phase4_final_gate.md similarity index 100% rename from devlog/_plan/260829_cursor_tool_continuation_pairing/030_phase4_final_gate.md rename to devlog/_fin/260829_cursor_tool_continuation_pairing/030_phase4_final_gate.md diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/040_phase5_checkpoint_suffix_gap.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/040_phase5_checkpoint_suffix_gap.md similarity index 100% rename from devlog/_plan/260829_cursor_tool_continuation_pairing/040_phase5_checkpoint_suffix_gap.md rename to devlog/_fin/260829_cursor_tool_continuation_pairing/040_phase5_checkpoint_suffix_gap.md diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/050_phase6_native_turn_orphan.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/050_phase6_native_turn_orphan.md similarity index 100% rename from devlog/_plan/260829_cursor_tool_continuation_pairing/050_phase6_native_turn_orphan.md rename to devlog/_fin/260829_cursor_tool_continuation_pairing/050_phase6_native_turn_orphan.md diff --git a/devlog/_plan/260829_cursor_tool_continuation_pairing/060_phase7_positional_bound.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/060_phase7_positional_bound.md similarity index 100% rename from devlog/_plan/260829_cursor_tool_continuation_pairing/060_phase7_positional_bound.md rename to devlog/_fin/260829_cursor_tool_continuation_pairing/060_phase7_positional_bound.md diff --git a/devlog/_fin/260829_cursor_tool_continuation_pairing/070_phase8_checkpoint_suffix_orphan_strip.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/070_phase8_checkpoint_suffix_orphan_strip.md new file mode 100644 index 0000000000..58abb93c18 --- /dev/null +++ b/devlog/_fin/260829_cursor_tool_continuation_pairing/070_phase8_checkpoint_suffix_orphan_strip.md @@ -0,0 +1,714 @@ +# wp6 — the orphan-strip loop eats the whole checkpoint suffix + +Status: plan. Work-phase wp6, criterion `c-2`. Predecessors: `050` (superseded), `060` (merged as +#2936 / `d882caed5`). + +## Symptom the user reported + +Cursor models "무한 출력" and "툴 출력을 못받고" — the turn never terminates and the model behaves as if it +never saw its tool output. + +Reproduced on merged `dev` `d882caed5`, isolated proxy, `cursor/grok-4.6`, three sequential `echo` +commands requested one at a time. Counts are from the COMPLETED artifacts, recounted after audit r8 +found the first table had been read from a file that was still being written: + +| observed | `live3b.jsonl` | `live3.jsonl` | +|---|---|---| +| distinct commands requested | 3 | 3 | +| `command_execution` items emitted | 21 | 133 | +| STEP1 runs | 10 | 64 | +| STEP2 runs | 10 | 67 | +| STEP3 runs | 1 | 2 | +| "interrupted" mentions | 8 | 134 | +| terminal answer | reached, after 21 executions | reached, after 133 | + +The turn does eventually terminate. The defect is that it burns 21 to 133 tool executions to run three +commands, repeatedly re-running work that already succeeded. The earlier claim that it never terminates +was an artifact of counting a file mid-run and is withdrawn. + +The narration alternates verbatim: "STEP1 already ran. Next is STEP2." then "STEP1 was interrupted last +time, so I'll run it now." The model contradicts itself every other turn, which is the signature of a +prompt whose history changes shape between turns rather than of a confused model. + +## Root cause + +`rootPromptMessages` ends its external-model pruning with an orphan guard: + +```ts +const historyEntries = [...keptPrior, ...active]; +// Guard against orphan assistant / toolResult at the start of the retained suffix. +while (historyEntries[0]?.role === "assistant" || historyEntries[0]?.role === "toolResult") { + if (historyEntries.length <= active.length) break; + historyEntries.shift(); +} +``` + +On a **full replay** the premise holds: history starts at the real conversation start, so a leading +assistant or result entry means the user turn was pruned and the entry is genuinely orphaned. + +On the **checkpoint path** the premise is false. `buildPreparedCursorRunRequest` replays only +`rawMessages.slice(suffixStart)`, and `suffixStart` is `coveredMessageCount` — the count of messages the +checkpoint already carries. A suffix therefore legitimately **begins** with the assistant message +whose initiating user turn sits inside the checkpoint. The loop reads that as an orphan and shifts it +off, then reads the next entry the same way, and keeps going until `historyEntries.length <= active.length` +stops it — that is, until nothing but the trailing active result block is left. + +The `break` is what makes this total rather than partial: it fires only when the survivors are exactly +the active block, so every earlier pair is discarded no matter how many there are. + +### Measured, with a checkpoint covering message 0 and N completed pairs in the suffix + +| pairs in suffix | `rawMessages` | roots emitted | what the model sees | +|---|---|---|---| +| 1 | 3 | 2 | seed + result 1 | +| 2 | 5 | 2 | seed + result **2** only | +| 3 | 7 | 2 | seed + result **3** only | +| 4 | 9 | 2 | seed + result **4** only | + +This table needs one qualifier audit r8 supplied: it holds for the shape a real agent produces, where +the assistant NARRATES before calling a tool. With a bare tool call and no assistant text there is no +strippable entry at the head of the suffix, `activeStart` walks back over the whole block, and the counts +grow normally (2, 3, 4, 5). The narration root is what arms the loop — which is why the defect looked +intermittent rather than universal. + +The suffix grows and the payload does not. Live diagnostics agree: one checkpoint series measured +`rawMessages` 8, 10, 12, 14, 16, 18 across consecutive tool-continuation turns with `rootBlobs` pinned at +8 and `continuationMode: checkpoint` every time. (An earlier draft cited 9..19 against a pinned 5 and a +proxy port that no artifact contains; the property is real, those specific figures were not, and they are +corrected here rather than restated.) + +That explains both halves of the report. The model cannot see the output of the command it just ran two +turns ago ("툴 출력을 못받고"), so it re-runs it; and because every turn presents the same collapsed shape, +it never accumulates enough state to finish ("무한 출력"). + +### Causation, not correlation + +Gating the loop off behind a scratch environment variable, changing nothing else, turns the roots +column from 2, 2, 2, 2 into 3, 5, 7, 9. The scratch mutation was reverted; `git diff` is empty. + +## Why the guard cannot simply be deleted + +It is load-bearing on the full-replay path. `tests/cursor-blob.test.ts` covers the case it was written +for: byte pressure consumes the budget with one large active result, the user turn that asked for it is +pruned, and `conversationTurns()` then discards the result too for lack of a current turn — the wire +request degenerates to system roots plus a bare result marker. #1527. + +The fix must keep that behaviour for full replay and stop applying it to a suffix whose initiating turn +is covered by the checkpoint. + +## Change + +`src/adapters/cursor/protobuf-request.ts`, `rootPromptMessages`: + +1. The function already receives `knownCallsOffset` (added by #2936), which is `suffixStart` on the + checkpoint path and `0` on full replay. A non-zero offset is exactly the "my history starts + mid-conversation" signal the guard is missing. Introduce a named boolean from it — + `suffixContinuesCoveredTurn` — rather than testing the arithmetic inline, because the two meanings + (positional re-basing vs. provenance) must not silently merge again. +2. Skip the orphan-strip loop when that flag is set. A covered-turn suffix has no orphan to strip: its + initiating turn exists, upstream, inside the checkpoint. +3. Leave the `#1527` initiator-recovery block below it unchanged. Its own comment already argues it + needs no mode distinction, and `activeStart > 0` confines it to this call's own slice — so it stays + correct for both paths and is not part of this defect. + +Not in scope: the `suffixStart === 0` edge, where a checkpoint reports zero covered messages and the +suffix is the full history. The flag is false there, which is the correct answer — that request *is* a +full replay in every respect that matters to the guard. + +## Verification + +- Red first: the growth table above becomes a test that asserts roots grow with pairs. It must fail on + `d882caed5` and pass after. +- The `#1527` full-replay assertions in `tests/cursor-blob.test.ts` must stay green untouched; they are + the guard's reason to exist and the only proof this change is narrow. +- `a checkpoint suffix may legitimately begin with a tool result` must stay green — it is the existing + expectation that most nearly overlaps this change. +- Live re-measurement of the exact repro above on an isolated proxy: three commands, one run each, + zero interrupt narrations, terminal `ALLDONE`. +- `bun x tsc --noEmit` and `bun run privacy:scan`; full suite on `ssh lidge`, never locally. + +## Audit r8 reopened the change: one mechanism was not enough + +The first implementation fixed only the orphan-strip loop. An independent audit measured two further +paths to the same user-visible symptom, both confirmed here before anything was changed. + +### The orphan fix is inert under byte pressure + +Eight pairs of 64 KiB results still produced 2 roots, with and without the orphan fix. The `keptPrior` +loop above the guard admits **complete turns**, and a turn starts at a `user` root — which a checkpoint +suffix does not have, by definition. `turnStart` walks to 0, the whole prior block becomes one +all-or-nothing pseudo-turn, and the first budget overrun drops every entry. The orphan guard then has +nothing left to strip, so it never runs and the fix cannot help. + +The remedy is to admit entries individually when the suffix continues a covered turn: without a turn +boundary to respect there is nothing for turn-granularity to protect, and keeping the most recent +history that fits beats keeping none. Measured 2 → 15 roots on that fixture. + +This matters more than a partial loss would, because root replay is the **only** channel carrying suffix +history. `conversationTurns` walks from `historyMessageStart` and never meets a `user` message in a +suffix, so `current` is never created and every entry hits `if (!current) continue` — the suffix +contributes 0 turns both before and after this change. Verified directly rather than assumed. + +### Restored growth collided with the cumulative envelope + +Suffix pruning measured only its own slice, so it produced suffixes that were individually legal and +cumulatively fatal. Once replay actually grew, the downstream envelope guard began throwing +`CursorRootEnvelopeLimitError` — a non-retryable 400 — on conversations that previously degraded +silently: 50 pairs behind 100 checkpoint roots, 10 behind 180, 4 behind 190. Growth was also +non-monotonic, with 95 pairs giving 191 roots and 96 collapsing back to 2. + +Two things were wrong and both are fixed. Pruning now subtracts the checkpoint's own roots and bytes, so +the suffix is measured against the room that actually remains. And when a checkpoint leaves no room at +all, the checkpoint is **abandoned** for a full replay under a new `envelope_exhausted` invalidation +reason rather than pruned to fit. Pruning to fit would emit the covered prefix and silently drop every +uncovered message — this unit's own defect, reintroduced at the top of the range — and throwing would +hand the caller a 400 it cannot retry. A full replay rebuilds a self-contained prompt and prunes it +coherently. After the change all three fixtures stay at 191 roots with no throw and no cliff. + +### The abandon decision reads pruning's result, not a byte threshold + +Two threshold attempts both left a live gap, which is why the predicate ended up where it is. Comparing +carried bytes against the raw limit left a few-hundred-byte band below it where the checkpoint was kept, +the suffix budget collapsed, and the newest tool result vanished — silently, where the old code at least +threw. Adding `systemBytes` moved the band instead of closing it, and the surviving positions were the +instructive ones: pruning kept the assistant narration and dropped the result, then kept the result +truncated so hard that only the truncation marker remained. Both leave the model looking at a call with no +answer, which is worse than keeping nothing. + +So the condition is not predictive. Pruning runs first, and the checkpoint is abandoned when the message +the turn continues from did not survive it. Two earlier attempts at that predicate are worth recording +because each failed differently. Matching the result's own output text against the serialized root broke +on JSON escaping the moment real output contained a newline, which made every live continuation abandon +its checkpoint — correct output, checkpointing silently dead. Checking the surviving roots' roles could not +distinguish the result from the narration beside it. The predicate is now positional: `rootPromptMessages` +returns the source message index of every root that survived, plus the indexes whose output was elided +entirely by truncation, and the caller asks whether the last replayed message is in the first set and out +of the second. + +That second set exists because "the result root survived" is not the same as "the result survived". +Truncation has two ways to leave a root that answers nothing: reduce it to the marker alone, or cut +mid-envelope before the `output:` line. Both were live in the band, and both now set `outputElided` at the +single place that produces them, so no threshold has to guess. + +Swept across 15 positions from 100 KiB below the byte limit to 100 bytes above it, the newest result is +present at every one; before, five positions dropped it. Live turns still resume from their checkpoint +(`mode=checkpoint`, no invalidation reason) — the predicate costs nothing on ordinary conversations. + +Scoped out explicitly rather than silently: the abandon branch sits inside the `suffixStart`-valid block, +so a plain resume turn with an oversized checkpoint still throws as it did before this unit. That path has +no suffix to lose and no measurement here, so widening it belongs to its own phase. + +Two pre-existing tests asserted the throw. They now assert the bound instead: the assembled request stays +inside the envelope and the uncovered history is still present. + +An earlier draft claimed those two rewrites were mutation-checked against the `carriedRoots` subtraction. +The re-audit measured otherwise and it was wrong: both exit through the abandon branch — the count case +uses unmeasurable checkpoint roots, the byte case a checkpoint large enough to trip abandonment — so +neither touched the subtraction. Deleting it reintroduced all three throws with the suite still 97/0 +green. The subtraction now has its own case built to reach it: measurable checkpoint roots, a count three +below the limit so abandonment does not fire, and a suffix that only fits if pruning knows what the +checkpoint spends. Removing the subtraction now reddens three tests. + +## Verification (as performed) + +- Focused suite: `bun test tests/cursor-blob.test.ts tests/cursor-tool-result-invocation.test.ts + tests/cursor-tool-continuation.test.ts` — 138 pass / 0 fail at the head of this unit (133 when this line + was first written, before the later rounds added assertions). +- Every assertion driven red against the implementation it exists to catch, each mutation applied alone: + restoring the unconditional orphan guard reddens the two suffix-growth rows; restoring turn-granular + admission reddens the byte-pressure row; removing the `carriedRoots` subtraction reddens three rows; + neutering the result-survival predicate reddens the byte-band row; skipping the orphan guard + unconditionally reddens the full-replay orphan row. +- Live re-measurement on an isolated proxy built from the final tree, counted after the run exited + (`/tmp/ocxv2.ojEUBe/v2.jsonl`): 3 commands, one execution each, 0 interrupt mentions, terminal + `ALLDONE`. The run-request diagnostics from that same proxy's debug buffer report `rawMessages`/`rootBlobs` + of 3/4, 5/6, 7/8, 9/10 across the four turns, with the last three in `checkpoint` mode and no + invalidation reason — roots tracking history instead of pinned to a constant, and checkpointing intact. + An earlier draft cited a series read from a snapshot log copied out of the operator's home, which could + not be traced to the run it described. +- The operator's own proxy (port 10100, pid 62773, 2.35.0) was never touched; every probe ran against a + scratch `OPENCODEX_HOME` on a scratch port. + +## Audit round 3: the predicate had to learn which path it applies to + +The positional predicate was correct for the path it was written against and wrong for two others. Both +were measured before being changed. + +### Native models were losing their checkpoint on every continuation + +`suffixKeptItsResult` asked whether the replayed result root survived pruning. A native resume model has +no such root: its result travels in server-side turn state, so `echoToolResultInRoot` is false and +`rootPromptMessages` skips it. The question answered "no" unconditionally, which meant the checkpoint was +discarded on **every** native tool continuation — including `cursor/auto`, the default id — regardless of +size or byte pressure. + +That is not a cosmetic loss. `pendingToolCalls`, `readPaths` and `previousWorkspaceUris` exist only inside +the checkpoint, and a full replay does not rebuild them, so this unit's own defect had been relocated to +the native path. Measured through the real builder: `readPaths` went 2 → 0 for `auto`, +`composer-2.5-fast` and `composer-3`, while `composer-2.5` and `grok-4.6` were unaffected — exactly the +split `cursorNeedsExternalToolContinuation` draws. The predicate is now gated on it. + +Worth stating plainly: this was introduced by the fix for the previous round's finding, not by the original +defect. Three rounds of audit each found one, which is the argument for the rounds rather than against +them. + +### Parallel results were protected one at a time + +The check read the last replayed index only. Parallel tool calls arrive as a run of results, and under byte +pressure the older ones were the ones being emptied — a prompt with three calls and one answer, which the +code's own comment calls worse than keeping nothing. `historyOutputElided` already recorded them; nothing +read them. The whole trailing run of results is checked now. Swept 628 (carried-bytes, payload-size) +positions: 10 partial-answer positions before, 0 after. + +### The invalidation reason still reaches nothing, and that is now a recorded decision + +`envelope_exhausted` is assigned to a local, so it lands in the debug diagnostic and stops there. +`src/adapters/cursor.ts` drops a dead checkpoint by reading `request.checkpointInvalidationReason`, so an +exhausted checkpoint is re-decoded and re-abandoned every turn until its TTL. + +Round 3 asked for it to be propagated and the obvious fix — writing the field back onto the argument, which +is what `request-builder.ts` does — was implemented and then measured inert. `live-transport.ts` prepares a +**spread copy** of the request, so the write lands on the copy: the outer object the adapter reads stayed +`undefined`. A test asserting on the argument would have passed while proving nothing about the real path, +which is the same vacuous-coverage trap round 2 caught. + +Reaching the store means threading the reason back through `PreparedCursorRunRequest`, a signature change +on the shared prepare path. That belongs to its own phase. The cost of leaving it is bounded and worth +stating: wasted work each turn, not wrong output — the request assembled is correct either way. + +### Verification of this round + +- `bun test` across `cursor-blob`, `cursor-tool-result-invocation`, `cursor-tool-continuation` and + `cursor-request-builder`: 188 pass / 0 fail, and 102 / 0 in `cursor-blob` alone. An earlier draft said 187, + which matched no commit in the stack — recounted after audit round 4 flagged it. +- Each new assertion driven red against the implementation it catches: removing the native gate reddens the + native-checkpoint row; reading only the last index reddens the parallel row. The parallel fixture's + 375-byte offset was derived from the sweep rather than guessed — it is the one position where a + last-index-only check leaves exactly one answer standing. +- Sweeps re-run clean after the change: 15/15 band positions deliver the newest result, 222 edge positions + (multi-byte UTF-8, empty, whitespace-only, error, self-referential `output:` payload) with no loss, 628 + parallel positions with no partial answers and no throws. + +## Audit round 4: the gate covered one disjunct out of three + +The abandon condition is a three-way disjunction, and round 3 gated only the last term. The middle one — +"the suffix produced no history roots at all" — is about the same thing, a replayed root going missing, so +it was equally meaningless for a model whose results never become roots. + +It fired whenever a native assistant turn was a **bare tool call with no narration**: no text root, no +result root, zero history roots, condition true, checkpoint discarded. Measured on the silent shape, +`readPaths` went 2 → 0 for `auto`, `composer-1`, `composer-2.5-fast` and `composer-3` while +`composer-2.5` and `grok-4.6` were unaffected — the same split, the same loss, one disjunct over. Both +survival terms are gated now; the count-full term stays ungated because it is a real envelope fact +independent of who echoes results. + +### Why four rounds each found something + +Every fix in this unit was correct for the path it was written against and silent about a sibling path in +the same condition. The fixture that let round 4's blocker through was round 3's own test: it asserted the +native path with narration, so the narration-free shape of the same path stayed invisible. The test is now +a cross product — four model ids by four assistant shapes (narrated, silent, empty text, whitespace text) — +because that is the axis the bugs kept hiding along, not because sixteen cases are inherently better than +four. + +Two counts in this document were also wrong and are corrected: the four-suite total is 188, not 187, and +the three-suite figure is 138 at head rather than the 133 true when it was written. + +## Audit round 5: the count budget was computed and never applied to the trailing run + +`historyLimit` subtracts `carriedRoots.count`, and every prior round reasoned about that subtraction as if +it bounded the assembled payload. It did not. It was read by the prior-history `while` loop alone. The +trailing tool-result block was assembled before that loop under **byte** pressure only, and +`historyEntries` was then built as `[...keptPrior, ...active]` with no count check anywhere. When +`keptPrior` is empty — the ordinary checkpoint-continuation shape — `historyEntries.length` equals +`active.length`, bounded by nothing at all. + +`truncateToolResultBlob` cannot save it: shrinking a result frees bytes, never a root slot. + +The abandon condition was supposed to catch the overflow, and it tested +`carriedRoots.count + suffixSystemCount` — carried plus system, asking whether there is room for **one** +more root. A parallel tool-call batch needs `active.length` of them. With 190 carried roots and a +3-result batch the test computes `190 + 1 >= 192` → false, keeps the checkpoint, appends 3 to 190, and +throws `CursorRootEnvelopeLimitError`: status 400, `retryable: false`, and `src/adapters/cursor.ts` fails +closed on the invalid-argument retry path when the last raw message is a tool result, which is exactly +this shape. + +Measured at `bde5b19dd`, before the fix: + +``` +carried=190 parallel=2 -> OK roots=192 +carried=190 parallel=3 -> THROW 193 roots +carried=189 parallel=4 -> THROW 193 roots +carried=188 parallel=8 -> THROW 196 roots +carried=170 parallel=25 -> THROW 195 roots +``` + +Reachable by ordinary growth, not a crafted fixture. Feeding each turn's assembled state back as the next +checkpoint — what `commitCursorCheckpoint` does — a plain conversation of 3-parallel-call turns died at +turn 48, and 5 calls per turn at turn 32. Both survive 200 turns after the fix, as do 1, 2 and 8 calls +per turn. + +The fix bounds `active` by count where it is assembled, rather than adding a fourth disjunct that has to +predict the suffix width. Oldest results drop first, matching the direction byte pressure already prunes, +and at least one always survives; the existing abandon check then reads `historyMessageIndexes`, sees the +dropped result, and falls back to a coherent full replay. That is why the grid shows the newest result +delivered at all 78 positions rather than merely "no throw". + +### Why the existing 188 could not see it + +The three pressure fixtures this document already claims — 50 pairs behind 100 roots, 10 behind 180, 4 +behind 190 — are all **sequential** pairs, and a sequential suffix has a trailing run of exactly 1, the +single width at which `+ 1` predicts the suffix correctly. The 628-position parallel sweep applied +**byte** pressure, where the abandon branch fires before the count cliff is reachable. Both axes existed +in the suite; neither case crossed them. All 188 tests passed identically with and without the production +fix, which is the sharpest available proof that no assertion covered this path. + +`tests/cursor-blob.test.ts` now crosses them: three `test.each` rows (carried 190 × 3 results, 188 × 8, +170 × 25) assert both halves — inside `CURSOR_EXTERNAL_ROOT_BLOB_LIMIT` **and** the newest output still +present, because staying inside the envelope by sending nothing useful is the other half of this defect. +Disabling the new bound reddens exactly those three and nothing else. Four-suite total is 191 pass / 0 +fail, `cursor-blob` alone 105. + +The pattern named after round 4 held for a fifth time, one level up: rounds 2 through 4 all reasoned about +the count budget as a settled fact and argued about the disjuncts consuming it, while the budget itself was +never applied to the wider of the two things it was supposed to bound. + +## Audit round 6: the r5 fix dropped in root space, and the check that guards it read raw space + +The count bound from round 5 acts on `active`, a list of ROOT entries. The abandon check derived its +trailing run by scanning `suffixMessages`, which is RAW messages. The two spaces are not the same, and they +diverge on the most ordinary assistant shape there is: a bare tool call with no narration emits no root at +all, so two sequentially-executed results sit ADJACENT as roots while raw space still separates them with an +assistant message. + +Consequence: both results entered the root-space trailing run, the count bound dropped the older one, and +the raw-space scan — seeing a run of length one, the newest result, which survived — reported "kept". The +checkpoint was retained and the request went out with a tool call answered by nothing. Measured at 190 +carried roots with bare-call pairs: the first answer was absent from every root and from `turns[]`. No +throw, no diagnostic, and the model's only sensible response is to re-issue the call — the exact loop this +unit exists to end, reintroduced by the fix for the previous round's blocker. + +`tests/cursor-blob.test.ts` uses that bare-call shape in nine fixtures, so this was not an exotic input. + +Two separable defects sat in the same place. The drop was also unnecessary: `historyLimit` subtracted +`systemEntryCount` on the checkpoint path, where the caller appends only `ids.slice(suffixSystemCount)` and +the checkpoint's own system roots are already inside `carriedRoots.count`. One free slot was charged twice, +so at 190 carried roots the limit came out 1 where 2 results fit. + +Both are fixed at the origin of the mismatch rather than at the call site. `rootPromptMessages` now returns +`activeMessageIndexes` — the trailing run as pruning saw it, recorded before pruning can shrink it — and the +abandon check reads that instead of re-deriving a run it cannot see correctly. It falls back to the +raw-space scan when the field is empty, which is how the full-replay and native shapes keep their previous +behaviour. `chargeableSystemCount` is zero on the covered-turn path, closing the double charge. + +Measured after the fix: 24 bare-call configurations across carried 170-190 and 2-8 pairs lose no answer at +all, and the reclaimed slot is visible — 192 roots where the defect emitted 191. + +### Mutation evidence, including one gap this caught in its own first attempt + +- abandon check re-derives from raw space → 2 red +- system count charged twice → 1 red +- the round 5 count bound removed → 5 red + +The middle row is worth keeping. The first version of the silent-loss test passed with the double charge +still in place, because that defect abandons the checkpoint and a full replay carries every answer — correct +output, reached wastefully, which no assertion about answer presence can distinguish. It took a second case +asserting the exact root count at exact fit to pin the arithmetic. A test that cannot fail against the +defect it was written for is the thing five of these six rounds actually kept finding. + +Round 6 also found that `outputElided` on the marker-only truncation return had no coverage: removing the +flag left all 191 tests green, and `tests/` is outside `tsconfig`'s `include`, so nothing else would have +noticed either. Covered now by asserting the abandonment it is supposed to trigger. + +Four-suite total is 197 pass / 0 fail; `cursor-blob` alone 111. + +## Audit round 7: the repetition note stopped the walk that protects the results + +The trailing-result walk tested one thing — `role === "toolResult"` — and walked backwards from the very end +of `history`. The repetition breaker appends a synthetic `[context note]` **user** root after the transcript +when the same output repeats three times or more. That note stands for no message, so it carries no +`messageIndex`, and the walk hit it immediately and stopped: `activeStart === history.length`, the trailing +run came out empty, `activeMessageIndexes` came out `[]`. + +Two failures at once, both worse than the defect round 6 fixed: + +The results lost trailing-run status altogether. They fell through into `prior` and were pruned as ordinary +history, so the "keep at least one result" floor never applied to them. + +And the empty `activeMessageIndexes` sent the abandon check into its raw-space fallback — the exact scan +round 6 exists to avoid. Measured: at 186 carried roots the note-armed shape was RETAINED where the +identical shape without the note correctly abandoned to a coherent full replay. + +The trigger is the worst possible one. The note arms on three consecutive identical assistant narrations, +which is the runaway-repetition shape this entire unit exists to end — so the input most likely to hit the +defect is the input the fix was written for. + +Instrumented state at the moment of the break: + +``` +PRUNE {historyLen:10, activeStart:10, active:0, activeIdx:[], historyLimit:6, lastRole:"user"} +ABANDON {activeIdx:[], usedFallback:true, trailingIndexes:[19], keptEnough:true} +``` + +`activeStart` equal to `historyLen` is the whole bug in one number. + +The walk now skips trailing roots that carry no `messageIndex` before looking for the result run, and the +excluded roots are re-appended afterwards so the note itself still reaches the model. That re-append is the +part that needed care: a root added after pruning has to be paid for DURING pruning, or the envelope is +overrun by exactly its number. Left uncharged, note-armed continuations at 188-190 carried roots threw the +non-retryable 400 for both sequential and parallel suffixes. `syntheticCount` and `syntheticBytes` are +therefore charged in the count bound, in the prior-history admission loop, and in the byte accounting, and +the orphan-strip floor counts them too so the strip cannot eat into the trailing run. + +### The byte relaxation was dropped rather than covered + +Round 7 also found that `chargeableSystemBytes = 0` had no coverage: reverting it alone left all four suites +green. The double-charge argument applies to bytes in principle, but no configuration could be found where +relaxing it changes the assembled payload — six crossings of carried bytes against system size against +result size in the deciding band produced byte-identical output either way. So it is gone. Charging the +system bytes twice only ever errs conservative, and untested new code on the envelope path is a liability, +not a saving. The count relaxation stays: it is covered, and its own case reddens without it. + +### Mutation evidence + +- the `messageIndex` walk removed (r11 defect restored) → 1 red +- `syntheticCount` uncharged in the count bound → 1 red +- the note dropped from the payload instead of re-appended → 1 red +- `syntheticCount` uncharged in the prior-history loop → 2 red + +The middle two are why this round's first attempt was not finished: both charges initially had no failing +test, exactly the condition round 6 had already been caught on once. A 224-configuration count sweep across +carried 185-191 by note-armed sequential and parallel suffixes showed the uncharged version throwing and the +charged version clean, which is what the new boundary case now asserts. + +Four-suite total is 198 pass / 0 fail; `cursor-blob` alone 115. Sweeps re-run clean at this head: 1440 +configurations across narrated, bare-call, whitespace-text and parallel shapes with zero envelope overruns, +zero orphaned calls and zero lost newest results; 78-position count-by-parallel grid clean; all five +multi-turn growth shapes survive 200 turns. + +## Audit round 8: the note was inside the array every pruning block reasons about + +Round 7 re-appended the note into `historyEntries` before the pruning blocks ran, and from that point every +one of them had to recognise a tail it could only identify by position. The initiator-recovery block could +not. Its floor is "stop when one entry is left", so with `[toolResult, note]` it counted the note as the +survivor and shifted off the **result**. + +What reached the model, one 600 KB result, three identical narrations instead of two the only difference: + +``` +PLAIN roots=3 lens=[16, 24, 524067] <- the answer +ARMED roots=3 lens=[16, 24, 193] <- the note, and nothing else +``` + +193 bytes of "take a DIFFERENT action" in place of the output the model was waiting for. The result had +already been truncated to fit; the recovery block deleted it anyway. This is the reported symptom exactly — +no tool output, so the model runs the command again — re-entered through the fix for it. + +A second mechanism compounded it. `activeBytes` included `syntheticBytes` while the equal-share divisor did +not, so shares summed to the entire budget and adding the note back always exceeded it. The +shrink-toward-equal-share pass — whose whole purpose is "a missing result is worse than a truncated one" — +became structurally unfittable, and control fell through to the loop that deletes a whole result. 246 bytes +of note cost a 200 KB answer. Reviewer measured 166 of 432 byte-pressure configurations losing an answer. + +### The fix is structural, not another floor + +Adding `+ trailingSynthetic.length` to each floor would have worked and would have left the next block to +discover the same trap. Instead the tail is held **out** of `historyEntries` entirely until assembly, and +every budget below is expressed net of it: `historyLimitForReal` and `historyBudgetForReal` are computed +once, before the first result is measured. The pruning blocks then reason only about real history and cannot +mistake one kind of root for the other, and the reservation is what keeps the tail from overrunning the +envelope when it returns. + +That the reservation is load-bearing was proved twice over: with it removed the same shapes 400 on the byte +limit, and an intermediate version that held the tail out without reserving its bytes committed 51 bytes +over. + +### Coverage, which was the round's second finding + +The entire `syntheticBytes` charge family had no test: neutralizing it in one edit left the suite green +while a sweep against that mutation threw 148 envelope errors. That is the third uncovered hunk in this +unit, and it landed in the same commit whose message drops `chargeableSystemBytes` for being uncovered — +the argument was made and then not applied to the new code beside it. + +Mutation evidence at this head: + +- byte reservation removed → 2 red +- count reservation removed → 3 red +- note dropped from the payload → 4 red +- `messageIndex` walk removed (r11 defect) → 3 red +- note re-appended into `historyEntries` **and** the gross budget spent (r12 defect in full) → 2 red + +The last row is worth stating precisely: re-appending alone is now harmless, because the reservation +prevents the loss on its own. The defect needed both halves, and the test catches the pair. + +Four-suite total is 201 pass / 0 fail; `cursor-blob` alone 118. Sweeps at this head: 896 note-armed +configurations across four assistant shapes crossed with count and byte pressure, 1440-case +call-answer-invariant sweep, 224-case count sweep, 78-position grid — zero overruns, zero orphaned calls, +zero lost answers, zero notes lost. Five multi-turn growth shapes survive 200 turns. + +### What eight rounds actually found + +One defect, re-entering through each of its own fixes. Every round's patch was correct for the path it was +written against and silent about a sibling path in the same condition — and three times the sibling was +created by the previous fix. The through-line is not carelessness about the condition; it is that each fix +added a fact to the pruning code (`carriedRoots`, a count bound, a root-space run, a synthetic tail) without +asking which existing block already assumed that fact absent. The last fix is the first that removes a +distinction rather than adding one. + +## Audit round 9: a subtraction clamped at zero cannot say "unaffordable" + +The reservation was `Math.max(0, historyBudget - syntheticBytes)`, and the tail was appended +unconditionally. Those two facts are compatible only while the difference is non-negative. Below that the +clamp reports "the note costs nothing", every pruning block correctly reasons about a budget of zero and +emits nothing, and the note is appended anyway — so the payload lands over the limit by exactly the deficit +the clamp erased. With 26 bytes free and a 246-byte note, 220 bytes over and a non-retryable 400. + +Holding the tail out of `historyEntries` is what made it unrecoverable. No block below could see it, so +none could charge it. + +Ninth iteration of the same pattern, and this time the new fact was *the tail is always appended*; the +construct that assumed otherwise was the clamp introduced beside it. + +### Why every fixture missed it + +The exposed shape is a turn that does **not** end in a tool result — an ordinary user interjection after a +repetitive stretch. With a trailing result the abandon check's survival disjuncts fire and rescue the turn; +on a plain follow-up they structurally cannot, and nothing else bounded the tail. Every fixture in +`cursor-blob` is a tool continuation. Measured across 42 carried-byte positions: 13 throws with the note +armed, 0 without, all on the interjection tail. + +The note is now dropped when it cannot be paid for. That is this unit's own priority order, stated in the +round 8 record and applied here: a missing instruction is recoverable, a missing tool result restarts the +loop. + +### One inert condition removed rather than shipped + +The first version of the affordability test also required a free root slot. It could not be made to matter: +60 boundary positions at and past the root limit behaved identically with and without it, because the count +bound already stops at one surviving result. It is gone. Byte affordability alone decides. + +That is the second time in this unit an inert guard was written and then dropped, and the reason is worth +recording: an envelope condition that cannot fail is indistinguishable from one that is wrong, so keeping it +costs the next reader the same audit it cost this one. + +Also corrected: one `activeBytes > historyBudget` gate still read the gross budget while its body wrote the +net one. Provably no behavioural difference — the entry has already been truncated to net by then — but it +is the exact drift that seeded rounds 5 and 6. + +Mutation evidence: affordability removed → 3 red; tail appended regardless of affordability → 3 red. + +Four-suite total is 208 pass / 0 fail; `cursor-blob` alone 122. Every sweep re-run clean at this head: 42 +deficit positions, 60 count-boundary positions, 150 zero-budget boundary cases, 896 note-armed +configurations, 1440-case call-answer invariant, 224-case count sweep, 78-position grid, 24 bare-call cases, +and five multi-turn growth shapes surviving 200 turns. + +## Audit round 10: the guard removed as inert was load-bearing at exactly one value + +Round 9 dropped the count half of the affordability test, arguing that the count bound below always leaves a +slot free because it keeps one result. That is true for every value of `historyLimit` except 1 — where the +one free slot is precisely the one the surviving result takes. The note was then judged affordable on bytes +alone, the reservation clamped to zero, and the append pushed full replay to 193 roots. + +Four armed-only `CursorRootEnvelopeLimitError` throws at 191 system prompts, across both tails and both +suffix widths, where the same request without the note assembled 192 and succeeded. Full replay has no +abandon branch, so nothing rescued it. + +The reasoning error is worth naming precisely, because the sweep that supported it was real. It varied +**carried roots on the checkpoint path**, where the count-full disjunct abandons the checkpoint long before +`historyLimit` can reach 1. The reachable route is full replay with many system prompts — a different axis +entirely, and one no earlier round had needed. "Inert across 60 positions" was a true statement about the +wrong sixty. + +Both conjuncts are restored. The lesson is not that removing inert guards was wrong; it is that "inert" +needs the axis that can make it fire, and a sweep along one axis does not establish it along another. + +### The reservation was uncovered, distinctly from the append + +Round 9's own mutation table claimed the affordability check was covered. It was covered at the **append** +site only: neutering `syntheticCount`/`syntheticBytes` while leaving `trailingSynthetic` gated left the +suite green, because asserting on the assembled payload cannot separate "the deficit was charged" from "the +tail simply was not appended". Asserting the exact root count at the boundary does separate them, and that +case is now present. + +Mutation evidence at this head, each applied alone: + +- count conjunct removed (the r14 defect) → 4 red +- byte conjunct removed (the r13 defect) → 3 red +- reservation neutered, append still gated → 6 red +- append ungated → 7 red + +Four-suite total is 212 pass / 0 fail; `cursor-blob` alone 126. + +### Ten rounds, one shape + +Every round found the same class of defect: a fact added to the pruning code beside a construct that assumed +it absent. Rounds 5 through 10 were each triggered by the previous round's own fix. Two of those were +arguments about whether a guard could fire — one dropped correctly, one dropped wrongly and restored here — +which suggests the code's real difficulty is that its budget arithmetic has several axes and any single sweep +silently fixes all but one of them. + +## Audit round 11: PASS, and the two notes it left + +Round 11 found no blocker. It confirmed `syntheticCountRaw` can only be 0 or 1 — one push site, once per +request — so the conjunct reduces to `historyLimit >= 2` when the note exists, and checked that threshold in +both directions: at 1 the single free slot belongs to the result, at 2 both fit exactly at 192 roots. It +audited all 25 budget references and found gross values only in the affordability test itself, which is +where they belong. Across 5040 checkpoint configurations and 200-turn feedback growth at five call widths: +no throw, no overrun, no lost newest result, no orphaned call. + +Its attribution rig is the more useful artifact. Driving HEAD, the parent, and base `dev` through identical +576-position grids: HEAD is never worse than its parent anywhere, and the 8 positions where HEAD throws and +`dev` did not are all 192 system prompts, where the prompts alone exceed the envelope and HEAD throws with +or without the note. On those same positions `dev` emitted 192 roots carrying **zero** tool results — the +re-run loop this unit exists to end. Totals: HEAD 104 throws / 232 newest-lost, parent 112 / 232, `dev` +96 / 372. + +### The threshold is now pinned from the tight side too + +Round 11's one actionable note: tightening `>= 1` to `>= 2` left all 212 tests green. Over-conservative is +safer than over-eager, but a suite that cannot tell a correct bound from an unnecessarily strict one is +exactly the gap that cost round 14. A case at two free slots now asserts that the note and the answer both +arrive at exactly 192 roots: relaxing the bound reddens 4, tightening it reddens 1. + +### A claim in the round 10 record was wrong + +That record said the reservation had been pinned at the append site only, and that neutering +`syntheticCount`/`syntheticBytes` left the suite green. On the parent commit that mutation already reddens +6, all of them pre-existing round 8 and 9 cases. The count-conjunct finding stands on its own evidence; this +secondary claim did not, and the root-count case is not what closed it. + +### Remaining known gap, scoped out deliberately + +On the extreme byte axis — a single system prompt near 523 KB — the note can be kept while the result +truncates to a marker, which inverts this unit's stated priority order. That band is identical on the parent +(24 positions) and far worse on `dev` (180), so it is pre-existing and improved here rather than introduced. +Full replay has no abandon branch to rescue it, which makes it a genuine follow-up rather than a +non-problem, and it belongs to its own phase. + +Four-suite total is 213 pass / 0 fail; `cursor-blob` alone 127. + +## Terminal outcome + +PR #2940 landed on `dev` as squash commit `62df78d8dd2451accdc0ddd615b9fad080d64a60`, from head +`0340d17599b65dda8b739a30107f59297e0d145b`, with CI green at that exact head. The remote gate on +`ssh lidge` was re-run against the merge commit itself and reported exit 0 with 16359 pass / 0 fail / +16 skip, so the landed tree is verified rather than only the pre-merge head. + +Round 11 is the closing verdict: PASS, with two minor notes, both addressed in `0340d1759` before the +merge — a threshold case pinned from the tight side, and the correction of a wrong secondary claim in +the round 10 record. Rounds 1 through 10 each found a genuine blocker, and rounds 5 through 10 were +each triggered by the previous round's own fix. That is the finding worth carrying forward: every one +of those fixes added a fact to the pruning code without asking which existing block had assumed that +fact absent. + +Three items were scoped out on purpose and are not defects of this unit. The `envelope_exhausted` +reason still does not reach the checkpoint store, and the spread copy in `live-transport.ts` makes it +provably inert rather than merely unobserved; propagating it needs a signature change on a shared +prepare path. On the extreme byte axis near a 523 KB system prompt the repetition note can survive +while the result truncates to a marker, which is pre-existing and measurably better here than on the +parent. And `composer-2.5` assembles 194 roots because it is a hybrid — `echoToolResultInRoot` true +with `externalModel` false — which places it outside the envelope guard; that behaviour is identical +on `dev` and predates this work. + +This unit moves to `_fin` under the rule in `AGENTS.md`: the work it records is now visible in public +git history. diff --git a/devlog/_fin/260829_cursor_tool_continuation_pairing/080_final_gate.md b/devlog/_fin/260829_cursor_tool_continuation_pairing/080_final_gate.md new file mode 100644 index 0000000000..732fcb4a21 --- /dev/null +++ b/devlog/_fin/260829_cursor_tool_continuation_pairing/080_final_gate.md @@ -0,0 +1,78 @@ +# 080 — Final gate: independent audit of the landed fix + +Reviewed tree: `7747bf74f`, checked out detached and clean, which at the time was `origin/dev`. +The fix path and this unit are byte-identical at later heads, so the audit still describes them. + +## Why a separate gate, after eleven rounds + +Rounds 1 through 11 in `070` audited the change while it was being built, against a plan the same +session wrote. This gate asks a narrower question that those rounds structurally could not: does the +landed code on `dev` hold up to someone who did not build it, and is every claim in the written record +true against git rather than against memory. + +The reviewer was given the three deliberately scoped-out items up front — the inert +`envelope_exhausted` propagation, the extreme-byte-axis note ordering, and `composer-2.5`'s hybrid +root count — precisely so it could not bill known accepted tradeoffs as new findings, and was told a +PASS was an acceptable outcome so it had no incentive to manufacture one. + +## Verdict: pass, no findings + +### The invariant is not the one the plan named + +The most useful thing the gate produced is a correction to how this fix should be described. It does +not emit a separate assistant `[Tool Call]` root before the result. It names the invocation *inside* +the result envelope, as an `invoked: with ` line +(`src/adapters/cursor/protobuf-request.ts`). That is deliberate: a standalone `[Tool Call]` marker +gets few-shot-mimicked by the model and breaks multi-tool continuations, which is what the remote +suite's 363-B guard caught when this unit first tried that shape. + +So the invariant worth testing is "no replayed result root lacks its invocation line", not "a call +root precedes the result". The goalplan's own criterion wording carries the older framing. + +### Coverage, measured rather than asserted + +Six mutations, each reddening on-point tests against a 166 pass / 0 fail baseline on four cursor +suites: + +| Mutation | Red | +|---|---| +| Orphan-strip skip reverted | 3 — suffix-growth and byte-pressure rows | +| Guard skipped unconditionally | 1 — the full-replay orphan row | +| `callBefore` positional bound dropped | 2 — both history-position rows | +| `knownCallsOffset` dropped from the root bound | 3 — including the pre-cut call naming row | +| `carriedRoots.count` removed from `historyLimit` | 10 | +| Turn-granular admission restored for suffixes | 1 — incremental pruning | + +The round 11 note threshold reproduces in both directions: relaxing `>= 1` to `>= 0` reddens 4, +tightening to `>= 2` reddens 1. A suite that can tell a correct bound from an unnecessarily strict one +is the strongest single piece of evidence in this unit, and it is what rounds 5 through 10 lacked. + +### Sweeps + +96 shapes across pair counts, full replay and two checkpoint cuts, bare-call and narrated histories, +result sizes to 64 KiB: 503 result roots, none missing an invocation line. A wider 576-configuration +sweep over parallel batches, system-prompt counts, carried roots, tail kinds and note arming reported +no throws, no count overrun, no orphan cases and no lost newest result. 55 invocation pairings across +checkpoint cuts produced no mislabel, so no result was ever named with a later command. + +### The checkpoint skip does not rest on trusting the checkpoint + +Three hostile shapes attacked the `knownCallsOffset > 0` premise: a covered prefix with no user turn, +`suffixStart = 1` where message 0 is an assistant, and a cut falling between a call and its result. +All three kept the invocation line and produced no orphan, because the line is keyed by call id over +full history. The positional bound and the orphan-strip skip are independent mechanisms, which is why +the skip cannot resurrect the original looping symptom. + +### Claims checked against git + +`62df78d8d` is #2940's squash commit and an ancestor of the reviewed head; `0340d1759` is that PR's +recorded head with all-success CI; `git diff` between them on the fix path is empty, which is what +makes "the landed tree is verified, not only the pre-merge head" a fair statement rather than a +flourish. The 213 / 127 test counts reproduce exactly. `bun run typecheck` is clean. + +## One behaviour named, not counted as a defect + +When two calls share a decoded call id, `toolCallsByCallId` drops the id as ambiguous and both results +replay with no invocation line — the pre-fix orphan shape, for that narrow case. The code argues the +tradeoff explicitly: a wrong invocation is undetectable by the model, a missing one is honest. Upstream +Codex call ids are unique. The gate agreed with the choice and recorded it rather than filing it. diff --git a/devlog/_plan/260829_green_pr_merge_train/000_plan.md b/devlog/_plan/260829_green_pr_merge_train/000_plan.md new file mode 100644 index 0000000000..f7957d9503 --- /dev/null +++ b/devlog/_plan/260829_green_pr_merge_train/000_plan.md @@ -0,0 +1,99 @@ +# 260829 — Green-PR merge train + +Eight rebased pull requests reached a fully green test matrix on `dev@e546c160b` and are +candidates to land. This unit records why each one is safe to merge, the order the merges +must happen in, and the two integration designs that have to be built before their PRs can +land at all. + +## Why this needs a written analysis rather than eight merge clicks + +The eight diffs are not independent. Five pairs touch the same file, and three of those +pairs touch `src/config.ts` — the shared config parser every provider path reads. Merging +in arrival order would produce conflicts that a later merge resolves blindly, which is the +failure mode that produced the #2850 → #2851 follow-up: a merge that looked clean and +needed a security repair one hour later. + +A second reason is drift. `dev` moved from `e546c160b` to `8d1dc1f5d` while this set was +being prepared (#2861, #2862, #2865, #2868, #2869). Every green result recorded earlier +belongs to the head that produced it, not to the head the merge will land on. + +## Regression-impact inventory + +Source files each PR touches, ignoring docs and tests: + +| PR | Subject | `src/` surface | +|---|---|---| +| #2365 | usage cache metrics | `usage/summary.ts` | +| #2429 | `test:changed` local check | `AGENTS.md` only | +| #1756 | Grok per-model reasoning effort | `grok/{catalog,effort,inject,models}.ts`, `server/index.ts` | +| #2050 | combo routing strategies | `combos/*`, `cli/*`, `providers/quota*.ts`, `router.ts`, `types/config.ts` | +| #2827 | trusted Responses request id | `server/index.ts`, `server/request-log.ts` | +| #2364 | Vercel AI Gateway routing | `adapters/openai-chat.ts`, `config.ts`, `providers/vercel-gateway-routing.ts`, `server/auth-cors.ts`, `types.ts`, `types/provider.ts` | +| #2712 | xAI `x_search` opt-in | `adapters/{openai-responses,xai-web-search}.ts`, `config.ts`, `server/auth-cors.ts`, `server/responses/core.ts`, `types/provider.ts` | +| #2854 | blocked-model redirection | `config.ts`, `lib/shadow-call.ts`, `router.ts`, `types/config.ts` | + +## Overlap matrix + +Computed by intersecting the `src/` file sets, not by reading titles: + +``` +#1756 x #2827 src/server/index.ts +#2050 x #2854 src/router.ts, src/types/config.ts +#2364 x #2712 src/config.ts, src/server/auth-cors.ts, src/types/provider.ts +#2364 x #2854 src/config.ts +#2712 x #2854 src/config.ts +``` + +Collision degree per PR: `#2854=3`, `#2364=2`, `#2712=2`, `#1756=1`, `#2050=1`, +`#2827=1`, `#2365=0`, `#2429=0`. + +## Derived merge order + +Ascending collision degree, so each merge lands against the largest possible amount of +already-settled `dev`, and the diff most likely to conflict resolves last against a tree +that already contains everything it must coexist with: + +``` +#2365 -> #2429 -> #1756 -> #2050 -> #2827 -> #2364 -> #2712 -> #2854 +``` + +`#2854` merging last is the load-bearing part of this order. It touches `config.ts` +alongside both #2364 and #2712, and `router.ts` alongside #2050 — it is the only PR that +collides with more than one other cluster, so it is the only one whose conflicts are +cheaper to resolve once rather than three times. + +Waves, because `dev` CI is the gate and a wave is the smallest useful unit to verify: + +- **Wave A** — `#2365`, `#2429`, `#1756`: zero or single collisions, no shared config surface. +- **Wave B** — `#2050`, `#2827`: single collisions each. +- **Wave C** — `#2364`, `#2712`, `#2854`: the `config.ts` / `auth-cors.ts` cluster. + +**Corrected after audit.** An earlier draft claimed wave B's collisions were "already +settled by wave A". They are not: `#2050` collides with `#2854`, which is in wave C, and +`#2827` collides with `#1756` in wave A. Only `#2827`'s is settled by A. `#2050` is placed +in B because its one collision partner merges later, so `#2854` absorbs the resolution — +which is the same reason `#2854` is last. + +## Per-merge mechanics (added after audit) + +Merge order alone does not make a later PR land against settled `dev`; it only decides who +resolves the conflict. All eight heads currently share merge base `e546c160b`, and `dev` is +already five commits past it, so each merge must carry its own freshness step: + +1. Rebase the PR onto the then-current `dev`. +2. Push and let CI run on that exact head. +3. Merge only on a green technical matrix. +4. Re-read `dev` CI before starting the next merge. + +Skipping step 1 would also drift heads past the repository's ten-commit readiness +allowance as the train advances, so the freshness step is a gate requirement and not only +a correctness preference. + +#2429 and #2827 cannot enter their wave until the two designs below are built. + +## What this unit does not cover + +Five rebased PRs are excluded because CI found real defects in them, not stale-base +artifacts: #2716 (display name leaks into the opencode selector), #2351 (management route +not declared in the registry), #2213 (xAI wire defaults), #2496 (residual failures), and +#1829 (macOS launcher flake, unrelated to its own diff). They stay open. diff --git a/devlog/_plan/260829_green_pr_merge_train/010_wp1_2429_privacy_scan.md b/devlog/_plan/260829_green_pr_merge_train/010_wp1_2429_privacy_scan.md new file mode 100644 index 0000000000..aba75a3d7b --- /dev/null +++ b/devlog/_plan/260829_green_pr_merge_train/010_wp1_2429_privacy_scan.md @@ -0,0 +1,79 @@ +# wp1 — #2429: the privacy scanner rejects its own test fixture + +## Symptom + +`gates` fails on #2429's head. The failing step is `Privacy scan`, not a test: + +``` +Privacy scan failed: +tests/test-runner.test.ts:42 email: testopencodex.invalid +error: script "privacy:scan" exited with code 1 +``` + +Every test shard, `macos`, and all three `npm-global` matrices pass. The only red checks +are `gates` and the two draft-checklist gates (`hygiene`, `enforce-target`), which are +process gates rather than code failures. + +## Cause + +The PR's test helper commits a fixture repository and needs a git identity to do it: + +```ts +runGit( + cwd, + "-c", "user.name=OpenCodex Test", + "-c", "user.email=testopencodex.invalid", + "commit", "-m", message, +); +``` + +`scripts/privacy-scan.ts` matches `/[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}/gi` across the +tree. The fixture address satisfies that pattern, and `.invalid` is not on the +allow-list, so the scanner is behaving correctly — the literal really is an email-shaped +string in a tracked file. + +(This document writes the address as `testopencodex.invalid` for exactly the same +reason the fix exists: quoting the literal verbatim would make this file trip the scanner +too. It did, on the first commit of this unit.) + +This is not a false positive worth loosening the scanner for. The scanner's value comes +from having almost no exceptions; every added exception is a hole someone's real address +can later fall through. + +## Design + +Use the idiom the repository already uses for exactly this problem. `privacy-scan.ts` and +its own fixtures avoid self-matching by never writing an email as one literal: + +```ts +["1", "gmail.com"].join("@") +["stranger", "third-party.example.org"].join("@") +``` + +So the fix is a join at the call site: + +```ts +const TEST_COMMIT_EMAIL = ["test", "opencodex.invalid"].join("@"); +``` + +The value handed to git is byte-identical, so the fixture commits exactly as before and no +test expectation changes. The scanner no longer sees an email literal because there is no +longer one in the source. + +### Rejected alternatives + +- **Add `tests/test-runner.test.ts` to the scanner's allow-list.** The allow-list currently + holds two narrowly-argued entries (`a@b.com` in tests, a URL-userinfo fixture that reads + as `pw@host`). Adding a whole file would exempt every future email added to it. +- **Allow the `.invalid` TLD globally.** `.invalid` is reserved and safe in principle, but + the exemption would apply repository-wide and the scanner's job is to be boring, not + clever. +- **Drop the git identity and rely on ambient config.** CI runners have no global + `user.email`, so the fixture commit would fail. The identity is load-bearing. + +## Verification + +- `bun run privacy:scan` exits 0 locally. +- `bun x tsc --noEmit` clean. +- `gates` returns success on the pushed head. +- No local full-suite run: the user has forbidden it, and CI covers the shards. diff --git a/devlog/_plan/260829_green_pr_merge_train/020_wp2_2827_expose_header.md b/devlog/_plan/260829_green_pr_merge_train/020_wp2_2827_expose_header.md new file mode 100644 index 0000000000..62e0e059db --- /dev/null +++ b/devlog/_plan/260829_green_pr_merge_train/020_wp2_2827_expose_header.md @@ -0,0 +1,114 @@ +# wp2 — #2827: the request id a browser cannot read + +## Symptom + +#2827 is green across every check. The defect is not a failing test — it is a feature that +silently does nothing for its stated consumer, found by review rather than by CI. + +The PR adds a response header carrying the request-log id: + +```ts +const REQUEST_LOG_ID_RESPONSE_HEADER = "x-opencodex-request-id"; + +function withRequestLogId(response: Response, requestId: string): Response { + const headers = new Headers(response.headers); + headers.set(REQUEST_LOG_ID_RESPONSE_HEADER, requestId); + return new Response(response.body, { status: response.status, statusText: response.statusText, headers }); +} +``` + +## Cause + +`corsHeaders()` in `src/server/auth-cors.ts` emits `Access-Control-Allow-Origin`, +`-Allow-Methods`, `-Allow-Headers`, and `Vary` — but no `Access-Control-Expose-Headers`. + +The CORS default is that cross-origin JavaScript may read only the seven CORS-safelisted +response headers. A custom `x-` header is not one of them, so `response.headers.get( +"x-opencodex-request-id")` returns `null` in a browser even though the header is on the +wire and visible in devtools. + +The tests pass because they call the handler directly. Server-side fetches see every header; +the restriction is enforced by the browser, and nothing in the suite is a browser. This is +the same shape of gap as a feature guarded by a flag no test sets — the code is right and +unreachable. + +`-Allow-Headers` does not help: it governs what the **request** may send, not what the +**response** may reveal. + +## Design + +Add the response-header allow-list next to the request one, naming exactly the header this +proxy adds: + +```ts +"Access-Control-Expose-Headers": REQUEST_LOG_ID_RESPONSE_HEADER, +``` + +Two constraints on how: + +1. **The constant moves to `auth-cors.ts` and `server/index.ts` imports it.** Two string + literals that must agree will eventually disagree; the header name has one owner. +2. **`Vary` does not change.** `Expose-Headers` here is a constant, not a function of the + request, so it introduces no new cache dimension. Adding it to `Vary` would fragment the + cache for no reason. + +### Scope note + +**Corrected after audit.** The first draft said `managementCorsHeaders()` was "separate and +not touched". That was wrong, and the audit caught it: + +```ts +export function managementCorsHeaders(req?: Request, config?: OcxConfig): Record { + const headers = corsHeaders(); // <- inherits everything, including a new Expose-Headers + ... +} +``` + +Adding the key inside `corsHeaders()` would therefore have propagated it to every +management response, which is the opposite of the scope the design claimed. The two +options are to set it only in the data-plane wrapper, or to add it in `corsHeaders()` and +strip it in `managementCorsHeaders()`. + +Take the first. `withCors()` is the data-plane wrapper and the only path that serves +`/v1/responses`, so exposing the header there grants exactly the reach the feature needs. +Stripping a key the shared helper just added would leave two places that must stay in +agreement about a header neither of them owns. + +Concretely: `corsHeaders()` is left alone, and `withCors()` sets +`Access-Control-Expose-Headers: x-opencodex-request-id` after copying the shared keys. + +## Regression test + +The existing suite cannot catch this class of defect, so the test asserts the header +contract directly rather than the behavior of a browser we do not have: + +- `withCors(new Response(...), req, policy)` output contains `Access-Control-Expose-Headers` + naming `x-opencodex-request-id`. The assertion targets the wrapper, not `corsHeaders()`, + because the amended design deliberately leaves the shared helper untouched. +- The exposed name matches the header `withRequestLogId` actually sets — one assertion + comparing the two, so a future rename of either side fails here instead of shipping a + header nobody can read. +- `managementCorsHeaders()` output does NOT contain the key. This assertion is the one that + would have failed under the original design, so it is the reason the test exists. + +## Wrapper order (verified) + +The route composes the two wrappers as: + +```ts +return withRequestLogId( + withCors(responseWithDeferredRequestLog(response, requestId, start, logCtx), req, policy), + requestId, +); +``` + +`withCors()` runs first and `withRequestLogId()` wraps its result, copying headers through +`new Headers(response.headers)`. So an expose header set inside `withCors()` survives onto +the final response. Checked on #2827's head at `src/server/index.ts:1397` and `:1441`; no +success path carrying the request-id header bypasses `withCors()`. + +## Verification + +- `bun x tsc --noEmit` clean. +- Focused run of the CORS and request-log tests only. +- CI green on the pushed head, including `gates`. diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/000_cause_and_roadmap.md b/devlog/_plan/260830_dev_version_line_bump_pr/000_cause_and_roadmap.md new file mode 100644 index 0000000000..32b1a7efd6 --- /dev/null +++ b/devlog/_plan/260830_dev_version_line_bump_pr/000_cause_and_roadmap.md @@ -0,0 +1,134 @@ +# dev version line: stop repairing it by hand + +Unit: `devlog/_plan/260830_dev_version_line_bump_pr/` + +Named for what it ships: a version-bump PULL REQUEST opened when a release publishes. +The unit was briefly called `..._autobump`, which an audit correctly rejected — the +workflow prepares the change and a human merges it, so nothing is automatic end to +end. +Goalplan: `repair-the-dev-version-line-and-add-a-post-relea` + +## The symptom, today + +`dev` head `df8b3882f` carries `package.json` version `2.36.0`. Tag `v2.36.0` +names `c7d8407d2`, which is `origin/main`. So the tree claims a version that is +already published from a different commit, and +`tests/release-version-line.test.ts` reports exactly that: + +``` +(fail) release version line > the in-tree version is never behind a released one +error: package.json version 2.36.0 equals release tag v2.36.0, but this commit is +not the one that tag names. The tree claims an already-published version: +publishing is refused as a duplicate. Bump package.json. +``` + +This fails CI jobs `test 2/4` and `macos` on `dev` itself (run 33312566315, cut at +`c2778ca3a` — `dev` has since advanced to `df8b3882f` and the failure still +reproduces there) and therefore on every PR opened against it. PR #3007 inherited +the same two red jobs for a two-file GUI change, and branch protection refused the +merge until it was overridden. + +## Why a one-line bump is not the fix + +The same defect has been repaired by hand FOUR times: + +| commit | what it did | +|---|---| +| `32529c2b2` | `2.24.2` -> `2.27.0`, after dev trailed the published channel by two releases | +| `e4a85d134` | `2.32.1-preview.20260825` -> `2.34.0`; also ADDED `release-version-line.test.ts` | +| `076ad3036` | `2.34.0` -> `2.35.0`, right after v2.34.0 shipped | +| `befcac3e1` | `2.35.0` -> `2.36.0`, after v2.36.0-preview.20260829 shipped | + +Note the second row: the detector was added DURING this sequence, and two more +hand-repairs followed it. Visibility was never the missing piece — that is the +finding that decided the design in `020`. + +Four repairs of one cause is a missing actor, not four accidents. The cause is +structural and visible in `scripts/release.ts`: the release runs only on `main` or +`preview` (`allowedBranches = ["main", "preview"]`, line 496), bumps +`package.json` there, commits `release: v`, pushes THAT branch, and +dispatches `release.yml`. The workflow ends at "Create GitHub release" — tag plus +GitHub release, nothing more. No step in either file ever advances `dev`. The +workflow declares `permissions: {}` at the top (line 32) and grants each job only +`contents: read` or `contents: write`, which is what makes an added `dev` write +there a security-review problem rather than a convenience. + +So the version line on `dev` goes stale the moment a release publishes, and stays +stale until a human notices red CI on an unrelated PR. The cost lands on +contributors: inherited red they did not cause and cannot fix from their own diff. + +What this unit can and cannot promise: it moves the repair from "someone eventually +remembers" to "a reviewable PR is waiting." It does not make the red impossible, +because the bump still needs a human merge — `Protect dev` requires an approving +review and code-owner sign-off, which a bot cannot supply. Claiming more than that +was the defect an audit caught in the first two drafts of `020`. + +## Constraint that shapes the design + +`dev` carries the NEXT STABLE version; the preview train adds its own suffix at +release time. That is the precedent `befcac3e1` states explicitly and the three +earlier repairs followed. A mechanism must preserve it — bumping dev to a preview +string would contradict every prior repair. + +The existing test is already the right detector. It reads the local tag set, needs +no network, and distinguishes "equal on the release commit" (legal) from "equal +anywhere else" (duplicate). Nothing about the detector needs changing. What is +missing is anything that PREPARES the repair: today the detector reports the problem +to whoever happens to open the next PR, and the fix is left to memory. + +## Phase map + +Each decade doc below is one full PABCD cycle. Dependency-ordered: the version +repair lands first because it unblocks CI for everything else, then the actor that +prepares the next repair as a reviewable PR, then the ship. + +- `010_version_repair.md` — move `dev` off the consumed `2.36.0` (wp2). +- `020_post_release_bump.md` — open the dev bump as a PR when a release publishes (wp3). + Note: that workflow only runs once it reaches `main`, the default branch. Merging it + to `dev` does not activate it. +- `030_ship.md` — PR against `dev`, CI evidence, merge (wp4). + +## Audit record + +TWO drafts of this roadmap were FAILED by an independent reviewer, and both verdicts +changed the design rather than the wording. + +Round 1: `020` chose a printed notice inside the release script and called it an +autobump. The reviewer showed the existing test is already louder than any printout, +and that two hand-repairs happened AFTER it landed. It also caught a wrong +"highest tag" claim in `010` and a test plan citing a `--dry-run` flag and reusable +shim helpers that do not exist. + +Round 2: the replacement PR-workflow design could not have worked. A `release` event +runs the workflow from the DEFAULT branch (`main`), which the scope forbade touching; +the named comparator `compareReleaseVersions` sits behind a module-scope +`process.exit` in `scripts/release.ts` and cannot be imported; and the "+minor" bump +rule contradicted `befcac3e1`, which moved `dev` to `2.36.0` on a +`v2.36.0-preview.*` publish. All three are fixed in the third draft, which imports +`compareReleaseTags` from `scripts/release-notes.ts` instead, records the `main` +promotion as a named follow-up in `030`, and replaces "+minor" with the two-branch +rule in `020`. The unit was also renamed. + +Round 3 caught the sequel to that last fix: "lowest unused stable" is not a pure +function of the script's two inputs, because "unused" is a property of the tag set and +the registry. The rule is now split — shape arithmetic in the script, freeness in the +tag-aware detector that already exists. It also caught that the out-of-scope list +below forbade the very promotion `030` depends on. + +Every rejected option and its reason stay in `020` so the decision is auditable. + +## Out of scope + +No publish, tag, or Release dispatch. No `main`/`preview` change IN THIS UNIT. No +merge of `main` back into `dev` to "sync" the version: `010_wp2_version_line.md` +names that as the trap that lands the consumed string on top of newer commits. + +That `main` exclusion is a scope boundary, not a claim that `main` is irrelevant. The +workflow in `020` cannot run until an ordinary maintainer-controlled promotion carries +it to the default branch; `030` records that as the named follow-up. Two consequences +worth stating plainly: + +- Merging this unit into `dev` fixes the red CI immediately (that is `010`) but arms + nothing (that is `020`, dormant until promotion). +- The next release cut from the CURRENT `main` will still strand `dev` one last time. + The loop closes on the release AFTER the workflow reaches `main`. diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/010_version_repair.md b/devlog/_plan/260830_dev_version_line_bump_pr/010_version_repair.md new file mode 100644 index 0000000000..f74141e12c --- /dev/null +++ b/devlog/_plan/260830_dev_version_line_bump_pr/010_version_repair.md @@ -0,0 +1,55 @@ +# 010 — move dev off the consumed 2.36.0 (wp2) + +One line. `package.json` `version`: `2.36.0` -> `2.37.0`. + +## Why 2.37.0 + +Verified against the real state, not read off a pattern: + +| candidate | verdict | +|---|---| +| `2.36.0` (current) | tag `v2.36.0` names `c7d8407d2`, not dev's head; npm `latest` = 2.36.0. Consumed. | +| `2.36.1` | mechanically legal but labels the range a patch, against the `befcac3e1` precedent | +| `2.36.1-preview.*` | contradicts "dev carries the next STABLE version" | +| `2.37.0` | `npm view @bitkyc08/opencodex@2.37.0` -> E404; no `v2.37.0` in the tag set; forward of every tag | + +Highest existing tag by the repository's own ordering is `v2.36.0` — NOT the +later-dated `v2.36.0-preview.20260830`. Sorting all 218 `v*` tags with +`compareReleaseTags` puts the stable release above its own prerelease, which is +correct SemVer precedence and the reason the failing message names `v2.36.0`: + +``` +top 5: v2.34.0 v2.35.0 v2.36.0-preview.20260829 v2.36.0-preview.20260830 v2.36.0 +HIGHEST = v2.36.0 +compareReleaseTags("v2.37.0", "v2.36.0") -> 1 +``` + +The first draft of this doc asserted the preview was highest while claiming to have +run the comparator. It had not. Run it. + +npm dist-tags at the time of writing: `latest` = 2.36.0, `preview` = +2.36.0-preview.20260830. + +## The diff + +```json +- "version": "2.36.0", ++ "version": "2.37.0", +``` + +No other file carries the product version. `gui/package.json` is `0.0.0`, +`docs-site/package.json` is `0.0.1`, and `src/generated/*` hold catalog hashes. +Re-verify with a repo-wide search excluding `node_modules`, `.tmp`, `devlog`, +`gui/dist` before claiming the line is unique. + +## Verification + +- `bun test tests/release-version-line.test.ts` — all three tests pass, including + "the in-tree version is never behind a released one" which currently fails. +- Re-run the freeness checks (`npm view`, `git tag --list`) immediately before + committing: another release landing mid-cycle would consume the candidate. + +## What this does not do + +It does not publish, tag, or promote, and it does not stop the next release from +stranding dev again. That is `020`. diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md b/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md new file mode 100644 index 0000000000..b3b51de7d3 --- /dev/null +++ b/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md @@ -0,0 +1,202 @@ +# 020 — open the dev bump as a PR when a release publishes (wp3) + +Third draft. Two independent audit rounds failed the first two; both verdicts and the +reasons are recorded below, because each one changed the design rather than the prose. + +## Round 1 rejected a printed notice (option C) + +> The detector already exists and is louder than a notice. +> `tests/release-version-line.test.ts` fails CI on every unrelated PR, and TWO +> hand-repairs happened after it landed — `e4a85d134` added it. A printout is a +> reminder, not a mechanism. + +Also verified: `dryRun = !args.includes("--publish")` (`scripts/release.ts:492`) makes +the default invocation a rehearsal, so a notice fires on every dry run until it is +trained away; and `release.yml` is `workflow_dispatch`, so an Actions-tab release +never runs `scripts/release.ts` at all. + +## Round 2 rejected the first PR-workflow design + +Three blockers, each confirmed against the repository: + +1. **It would never fire.** A `release` event runs the workflow file from the + DEFAULT branch. `gh repo view` reports `main`. This repository already documents + the identical trap in `cleanup-closed-pr-branches.yml:8-10` for scheduled + workflows. Landing the file on `dev` alone starts nothing. +2. **It could not import its comparator.** `compareReleaseVersions` is exported from + `scripts/release.ts:303`, but that file parses `process.argv` and calls + `process.exit(1)` at module scope (lines 487-491) with no `import.meta.main` + guard. `tests/release-version-line.test.ts:27-29` already records that importing + it kills the runner. +3. **The bump rule was wrong for preview-first releases.** `befcac3e1` moved `dev` + from `2.35.0` to `2.36.0` when the published tag was + `v2.36.0-preview.20260829`. "Increment the released core's minor" would have said + `2.37.0` and skipped a stable version that had not shipped. + +## Chosen design + +A separate workflow that opens a PULL REQUEST against `dev`, plus a pure script that +decides the version. + +**Honest scope.** This does not silently repair `dev`; it converts a forgotten chore +into a review-queue item that a human merges. Until that merge, +`release-version-line` stays red on `dev`. That is a real improvement over today — +a PR is durable where a printout is not, and it lands in the same place +`MAINTAINERS.md` already requires every `dev` change to land — but it is not an +autobump, and this unit should not be described as one. + +## The version rule + +Not "+minor" — that contradicts `befcac3e1`. But not "lowest unused stable" either, +phrased as if the script could evaluate it: "unused" is a property of the TAG SET and +the npm registry, and a pure function cannot see either. Stating the rule that way +would have made the doc unimplementable in exactly the manner the previous two drafts +were. + +Split the rule by who can answer it: + +**The script decides the CANDIDATE from the published version's SHAPE alone.** + +| published | candidate | precedent | +|---|---|---| +| `X.Y.Z-preview.*` (a prerelease of an unreleased core) | `X.Y.Z` | `befcac3e1`: 2.35.0 -> 2.36.0 on v2.36.0-preview.20260829 | +| `X.Y.Z` (stable) | `X.(Y+1).0` | `e4a85d134` 2.33.0 -> 2.34.0; `076ad3036` 2.34.0 -> 2.35.0; `32529c2b2` tip 2.26.0 -> 2.27.0 | + +Both rows are pure string arithmetic on the published version, and both are pinned by +tests. The prerelease row is the one that matters: the stable core of a +preview-first release has NOT shipped, so `dev` should carry it rather than skip it. + +**Freeness is verified where the tag set is visible.** The candidate is passed to the +existing detector, not re-derived: after the bump the workflow runs +`bun test tests/release-version-line.test.ts` in the `dev` checkout, which sorts the +real local tags with `compareReleaseTags` and fails if the candidate is at or behind +any published version. If that test fails, the workflow opens NO PR and the job goes +red — a visible request for a human decision, not a wrong PR. + +This is the honest division: shape arithmetic in the pure function, set membership in +the tag-aware gate that already exists. The script additionally refuses to emit a +candidate that `compareReleaseTags` does not rank strictly ahead of both `dev`'s +current version and the published one, which is the part it CAN check without I/O. + +## Files + +**`scripts/bump-dev-version.ts`** — pure decision logic, no git and no network. + +- Imports `compareReleaseTags` from `scripts/release-notes.ts`, NOT + `compareReleaseVersions` from `scripts/release.ts`. `release-notes.ts` guards its + CLI behind `import.meta.main` (line 1231) and already exports the comparator at + line 66, which is exactly why `release-version-line.test.ts` imports from there. + This avoids editing `scripts/release.ts` at all, keeping the release authority and + its security review surface untouched. +- Takes the released version and an explicit `package.json` path, so a test can + operate on a temp copy and the script is genuinely pure with respect to the + checkout. +- Emits a MACHINE CONTRACT, not prose: writes `changed=true|false` and `version=` + to `$GITHUB_OUTPUT` when set, and prints the same as JSON otherwise. Round 2 was + right that "print the chosen version" mixed with "print that nothing is needed" is + not an interface. +- `dev` already ahead -> `changed=false`, file untouched, exit 0. +- Malformed released version -> non-zero exit, file untouched. + +**`.github/workflows/dev-version-bump.yml`** — the actor. + +- Trigger: `release: [published]` only. No `workflow_dispatch`: round 2 correctly + noted that a branch-selected manual run executes THAT branch's body with + `contents: write`, which is the pattern this repository's own workflow comments + refuse. A missed run is re-driven by running the script by hand and opening the PR + normally. +- `permissions: {}` at the top; the single job takes `contents: write` (to push a new + `codex/dev-version-` branch — ruleset `Protect dev` covers only + `refs/heads/dev`, so the new branch is unprotected) and `pull-requests: write` (to + open the PR). Not `issues: write`, not `id-token: write`. +- `actions/checkout` with `ref: dev` AND `fetch-depth: 0` (or `fetch-tags: true`). A + `release` checkout defaults to the tag on `main`/`preview`, which is the wrong tree + to bump — and the tags are not optional decoration: `release-version-line.test.ts` + returns early on an empty tag set (line 93), so a shallow checkout would make the + freeness gate below silently vacuous rather than failing loudly. +- Do NOT copy `persist-credentials: false` from the repository's read-only workflows. + This job has to push its bump branch. +- Set up Bun and run `bun install` before the freeness gate: that gate is a + `bun test` invocation, not a shell comparison. +- Idempotent: if `codex/dev-version-` or its PR already exists, log and exit 0 + rather than failing the push. A second publish must not error. +- The workflow file must reach `main` to ever run. That is a promotion, not a + `dev`-only change, and `030` records it as an explicit follow-up rather than + pretending the merge to `dev` activates it. + +**Known limitation, stated not hidden:** a PR opened with `GITHUB_TOKEN` does not +start `pull_request` workflows, so the bump PR arrives without CI. `Protect dev` +additionally requires an approving review and code-owner review, and +`.github/CODEOWNERS` assigns `/.github/` and `/package.json` to human owners. A bot +cannot satisfy those. The PR is therefore a prepared, reviewable change — which is +the honest ceiling for automation here, and the reason the "autobump" framing is +dropped. + +## Test + +`tests/bump-dev-version.test.ts`, against temp copies of `package.json`. No shim +harness: the script is pure and takes a path. + +- dev `2.36.0`, released stable `2.36.0` -> `2.37.0`, `changed=true`. +- dev `2.35.0`, released stable `2.36.0` -> `2.37.0` (behind, not merely equal). +- dev `2.35.0`, released `2.36.0-preview.20260829` -> `2.36.0`. Pins `befcac3e1`, and + fails under a naive "+minor" rule, which is what makes it the load-bearing case. +- dev `2.36.0`, released `2.36.0-preview.20260830` -> `changed=false`: dev already + carries the prerelease's stable core, so there is nothing to do. +- dev `2.37.0`, released `2.36.0` -> `changed=false`, file BYTE-IDENTICAL, exit 0. +- dev `2.37.0-preview.1`, released `2.36.0` -> `changed=false`; a preview of a future + core is ahead, which `release-version-line.test.ts` already pins. +- released version malformed -> non-zero exit, file untouched. + +Red-first for a CLI is a behavioral red, not an import error: assert the chosen +version and the untouched-file invariant, and confirm each assertion fails against a +deliberately wrong rule (e.g. always `+minor`, which breaks the preview case) before +committing. + +## Also + +`MAINTAINERS.md`: after a release publishes, a `dev` version-bump PR is opened +automatically; merging it is part of closing out the release. Note that the workflow +only runs once it is on `main`. + +## As implemented + +Shipped in `075a33be8`. Three deviations from the sketch above, recorded because each +was forced by the tree rather than chosen: + +1. **Bun setup uses the repository's composite action**, `./.github/actions/setup-project-bun`, + not a hand-pinned `oven-sh/setup-bun` SHA. That action resolves the version from + `package.json` so the runtime source of truth stays in one place; an independently + pinned SHA here would have drifted from every other job. The first draft of the + workflow pinned its own and disagreed with the one already in the tree. +2. **`parseReleaseTag` is not exported** from `release-notes.ts`, so the script does its + own shape parse rather than widening that module's surface for one caller. Only + `compareReleaseTags` is imported. +3. **A `v`-prefix normaliser was required.** The workflow passes + `github.event.release.tag_name` (`v2.36.0`) while `package.json` holds a bare version, + so prefixing blindly built `vv2.36.0` and every comparison against it misordered. It + surfaced as the script rejecting a correct candidate: "candidate 2.37.0 does not rank + ahead of released v2.36.0". Now pinned by a test. + +The tests also caught a defect the plan did not anticipate. The ahead-check originally +compared `dev` against the CANDIDATE, which is the wrong question: a `dev` at +`2.37.0-preview.1` with `2.36.0` published is genuinely ahead of the release but behind +the candidate `2.37.0`, so the script would have "repaired" a healthy tree and +downgraded a legitimate prerelease line. It now compares against the released version, +which is the same question `release-version-line.test.ts` asks. + +A security review of the shipped workflow also found one gap worth recording. The +idempotency guard originally checked only whether the bump BRANCH existed. An open bump +pull request whose head branch had been deleted leaves that check passing, so the job +would recreate the branch and then fail on `gh pr create` with "already exists" - turning +a successful release red for a repair that was already queued. It now checks for an open +pull request first, then the branch. + +Two residual gaps are accepted rather than fixed, and named so a later reader does not +mistake them for oversights: + +- `Bun.write` to `$GITHUB_OUTPUT` truncates rather than appends. That is equivalent to a + first write today because the step emits nothing else, but it is not append-safe if a + later edit adds a second output in the same step. +- There is no test that exercises the `$GITHUB_OUTPUT` path itself; the tests cover the + decision and the file rewrite. diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/030_ship.md b/devlog/_plan/260830_dev_version_line_bump_pr/030_ship.md new file mode 100644 index 0000000000..5d1bb35545 --- /dev/null +++ b/devlog/_plan/260830_dev_version_line_bump_pr/030_ship.md @@ -0,0 +1,73 @@ +# 030 — ship it (wp4) + +## Branch and commits + +Branch `codex/dev-version-line-bump-pr` off `origin/dev`. Two commits, matching +the two implementation phases: + +1. `fix(release): move dev's version line past the published 2.36.0` +2. `feat(release): open the dev version bump as a PR when a release publishes` + +Plus the devlog unit. Push with `--no-verify` as the user directed. + +## PR + +Against `dev`, filling every `.github/PULL_REQUEST_TEMPLATE.md` section: Summary, +Verification, Checklist. No screenshot section is required — this touches no GUI. + +The description must state the four prior hand-repairs, because that history is the +argument for the mechanism. Reviewers who see only the version bump will read it as +routine maintenance. + +Release-tooling changes require explicit security review per `scripts/AGENTS.md` and +`MAINTAINERS.md`. Call that out in the description rather than leaving a reviewer to +discover it, and be precise about what the new workflow can do: it takes +`contents: write` to push a NEW unprotected bump branch and `pull-requests: write` to +open the PR. It does not use the release deploy key, does not write to protected +`dev` directly, and is a separate file from `release.yml` so the publish job's +permissions are unchanged. + +## Evidence required before the merge claim + +Local: +- `bun test tests/release-version-line.test.ts` — pass. +- `bun test tests/bump-dev-version.test.ts` — pass, including the NOOP case that must + leave `package.json` byte-identical. +- `bun test tests/release-helper.test.ts` — pass, proving the existing release + contract is unbroken. `scripts/release.ts` is deliberately NOT modified by this + unit, so this suite is a regression check rather than coverage of new behavior. +- `actionlint` on the new workflow if available; otherwise state that the YAML was + not machine-validated. +- `bun x tsc --noEmit` — clean. +- `bun run privacy:scan` — clean. +- `bun run prepush` — required by `scripts/AGENTS.md` for release-tooling changes. +- Red-then-green transcript for each new assertion. + +Remote: +- `gh pr checks` for the PR head showing `test 2/4` and `macos` GREEN. This is the + specific flip that proves the fix: those two jobs are red on `dev` today for this + exact test. + +The full local root suite is prohibited by the user. State that boundary in the PR +and rely on CI for whole-suite coverage. + +## Merge + +Merge into `dev` once the two previously-red jobs are green. If some unrelated job +is red, check whether it is also red on `dev` at `c2778ca3a` before deciding — the +point of this unit is to stop inheriting someone else's red, not to add to it. + +## Required follow-up, not part of this PR + +`.github/workflows/dev-version-bump.yml` DOES NOT RUN until it reaches `main`. A +`release` event resolves the workflow file from the repository default branch, which +`gh repo view` reports as `main`; this repository documents the same trap for +scheduled workflows in `cleanup-closed-pr-branches.yml:8-10`. + +So merging this PR into `dev` installs the file but arms nothing. The workflow first +fires after the next ordinary `dev` -> `main` promotion carries it there. That +promotion is maintainer-controlled (`MAINTAINERS.md`) and explicitly out of scope +here: this unit must not touch `main`. + +State this in the PR description. A reviewer who assumes the merge activates the +automation will believe the loop is closed a release earlier than it is. diff --git a/devlog/_plan/260830_kiro_post_answer_tool_calls/000_research.md b/devlog/_plan/260830_kiro_post_answer_tool_calls/000_research.md new file mode 100644 index 0000000000..09d299304d --- /dev/null +++ b/devlog/_plan/260830_kiro_post_answer_tool_calls/000_research.md @@ -0,0 +1,139 @@ +# Kiro post-final-answer tool calls — measurement and root cause + +Reported symptom, twice: routed through Kiro, the agent keeps issuing tool calls +after its final response has already been delivered. + +## Hosts measured + +| Host | Proxy | Version | Checkout | Kiro attempt rows | +| --- | --- | --- | --- | --- | +| local (this machine) | PID 99470, port 10100 | 2.36.0 | primary source checkout | 4080 | +| `macmini-cf` | PID 96671, port 10100 | 2.35.0 | `~/opencodex` | 0 | + +`macmini-cf` carries no Kiro attempt diagnostics at all, so every behavioral +row below comes from the local 2.36.0 proxy. The remote host is one release +behind and is not the reporting surface. + +## What the attempt rows say + +`ocx:kiro:attempt_complete` over the local log, bucketed: + +| Count | mode | sawText | sawRealTool | completionCalls | stopReason | +| --- | --- | --- | --- | --- | --- | +| 2643 | required | true | true | 0 | TOOL_USE | +| 1400 | required | false | true | 0 | TOOL_USE | +| 23 | required | true | false | 1 | TOOL_USE | +| 10 | disabled | true | false | 0 | END_TURN | +| 2 | required | false | false | 1 | TOOL_USE | +| 1 | required | true | false | 0 | END_TURN | +| 1 | text_fallback | false | false | 1 | TOOL_USE | + +4069 of 4080 attempts ran in `required` mode and every one of them ended with +upstream `stopReason: TOOL_USE`. Only 25 attempts ever called the private +completion tool. The model overwhelmingly prefers another tool call to the +completion channel. + +## What is NOT the cause + +Two candidate mechanisms were ruled out with evidence rather than reading. + +Replayed history is not the cause. 532 client rollouts under +`~/.codex/sessions/2026/08/{29,30}` were scanned for a `final_answer` message +followed by a tool call with no intervening user turn. What actually follows a +recorded `final_answer`: END 478, user message 131, developer message 5, tool +call 0. The client never replays a post-answer tool call. + +The delivered-answer local terminal is not broken. Two live probes against the +running proxy replayed a closed turn — once with `phase: "final_answer"` +echoed, once without it, matching real Codex traffic — and both returned +`output: []` with `end_turn: true` and added zero upstream Kiro requests. +The guard added in `b557a8140`/`68eaf45d8` works. + +It has simply never been needed: `~/.opencodex/usage.jsonl` holds 25042 Kiro +rows with zero `localTerminalReason` and zero `locallyAnswered`. Real turns +never arrive already closed, because the client ends the turn itself. So the +defect lives inside a live turn, not across turns. + +## Rejected first hypothesis + +The first diagnosis was that the model calls the completion tool, waits for a +tool result that never arrives, and then calls another tool. An independent +read-only audit refuted it with the parser: `flushOpen` consumes a valid +completion call and records `completionAnswer` without emitting any tool-call +event, the stream end yields the answer as `final_answer` followed by +`done(endTurn: true)`, and `parseKiroStream` returns without another request. +A completion call therefore terminates locally inside one inference; there is +no later inference in which the model could "keep going". Mixed +completion-plus-real-tool output in one inference also fails closed before any +answer is delivered. + +That refutation is correct, and it narrows the defect rather than dissolving it: +the problem is not what happens AFTER a completion call, it is that the model +mostly never makes one. + +## Root cause + +The private completion tool is advertised to the model as an ordinary tool. + +A source probe (`buildKiroPayload` with an `exec`/`wait` catalog) renders the +wire tool names as `["exec","wait","codex_kiro_final_answer"]` and injects: + +> Valid tool names for this turn are exactly \`exec\`, \`wait\`, +> \`codex_kiro_final_answer\`. These listed names are the complete top-level +> tool-call surface for this turn. + +That sentence comes from the shared, provider-agnostic nudge in +`src/adapters/tool-catalog-nudge.ts`, which knows nothing about completion +semantics. It cannot distinguish the proxy's private terminal channel from +`exec`, and the same nudge closes with: + +> Count a tool call only after its tool result returns. + +`KIRO_COMPLETION_INSTRUCTIONS` is the only text that describes the completion +tool, and it never contradicts that: + +> When tools are available, ordinary assistant text is mid-task commentary and +> does not end the turn. Continue using tools after progress updates. When the +> task is fully complete and no more tool calls are needed, call +> `codex_kiro_final_answer` exactly once with the complete user-facing final +> answer in `answer`. Do not provide the final answer as ordinary assistant +> text. + +Every sentence there is about WHEN to call it. Nothing marks it as different in +kind from `exec`, and nothing states what happens after. So the model holds a +contract in which the terminal channel is one more ordinary tool it may defer +while it keeps working — and the generic nudge's "count a tool call only after +its tool result returns" applies to it as uniformly as to everything else. + +The failure that follows is one of SELECTION, not sequencing. Across 4069 +required-mode attempts the completion tool was chosen 25 times: 0.6%. The model +keeps emitting finished prose as commentary and calling ordinary tools instead +of completing through the channel built for it. + +That is what the user sees. Measured over 1116 Kiro turns in the same two days +of client rollouts: 626 turns ended through the completion channel, 462 ended +on a tool call, and 28 ended with answer-shaped commentary prose and no +completion call at all. Those 28 are answers the model had already finished +writing — they open with "Done.", "완료", "머지까지 끝났습니다", "All ten items are +done" — delivered as mid-task commentary, which by the proxy's own contract +"does not end the turn". Three of them are followed by 4, 10, and 12 further +tool calls after the closing summary was already on screen. + +The missing terminal distinction is the leading mechanism behind that measured +selection failure: the terminal channel is advertised as an ordinary, deferrable +tool, and nothing tells the model that this is the one call that ends the turn. +It is a defect in the proxy's own injected text, not a client bug and not a +stream-parsing bug. Causality is not claimed as proven — establishing it +requires a live post-change comparison of the same selection rate, which this +unit records as the follow-up measurement rather than asserting up front. + +## Fix direction + +State terminal semantics where the model reads them: calling the completion +tool ENDS the turn, returns no tool result, and nothing may follow it. The +completion tool's own schema description is the load-bearing site — it travels +with the tool the nudge enumerates — with the prose contract kept consistent. + +Removing the tool from the enumeration is not an option: the nudge states that +names mentioned only in instructions are not callable, so an unlisted +completion tool would be a tool the model is told not to call. diff --git a/devlog/_plan/260830_kiro_post_answer_tool_calls/010_wp2_terminal_completion_contract.md b/devlog/_plan/260830_kiro_post_answer_tool_calls/010_wp2_terminal_completion_contract.md new file mode 100644 index 0000000000..313d49c110 --- /dev/null +++ b/devlog/_plan/260830_kiro_post_answer_tool_calls/010_wp2_terminal_completion_contract.md @@ -0,0 +1,71 @@ +# wp2 — make the completion tool's terminal semantics explicit + +## Change + +Two injected surfaces describe the private completion tool. Both need the same +fact, and the schema description is the one that travels with the tool the +nudge enumerates. + +`src/adapters/kiro.ts`, `kiroCompletionTool()` description: mark the tool as a +terminal channel rather than an ordinary work tool, make completing an +obligation rather than a permitted option, and state that the call ends the +turn, returns no tool result, and admits nothing after it. This sits directly on +the tool object the model is choosing between, so it is read in the same place +the model decides whether to call `exec` again. + +Exact target string: + +> Terminal completion channel, not an ordinary work tool. When the task is fully +> complete and no more work or tool calls are needed, you must call this tool +> exactly once instead of providing the final answer as ordinary assistant text. +> Put the complete user-facing final answer in \`answer\`. The call is complete +> when issued: it ends the turn, returns no tool result, and no text or tool call +> may follow it. + +`src/adapters/kiro-constants.ts`, `KIRO_COMPLETION_INSTRUCTIONS`: keep the +existing commentary-vs-completion rules verbatim and append the terminal clause, +so the prose contract cannot contradict the schema. + +Exact appended string: + +> This completion tool is not an ordinary work tool. When the task is complete, +> call it instead of emitting answer-shaped ordinary assistant text. The call is +> terminal and is the exception to generic tool-result counting: it is complete +> when issued, ends the turn, returns no tool result, and no text or tool call +> may follow it. + +## What must not change + +The commentary rule stays. "Ordinary assistant text is mid-task commentary and +does not end the turn" and "continue using tools after progress updates" are +the behavior that keeps a mid-task turn alive; the new wording constrains only +what happens after the completion call itself. A model must still be free to +call ten more tools before it completes — the fix is that after completing, it +must stop. + +The tool stays in the wire catalog and in the nudge enumeration. The nudge +states that instruction-only names are not callable, so delisting the +completion tool would advertise a tool the model is told to refuse. + +No change to `src/router.ts`, `src/server/lifecycle.ts`, or +`src/server/responses/core.ts`: the Lab core boundary is unrelated to this +defect and `tests/core-lab-boundary.test.ts` guards it. + +## Regression test + +`tests/kiro-adapter.test.ts` gets a focused case asserting the rendered wire +payload carries terminal semantics on BOTH injected surfaces: the completion +tool's schema description and the injected prose contract. Driven red before +the fix. + +What this test proves and does not prove: it proves the contract reaches the +model on both surfaces, which is the deliverable. It does not prove the model's +selection rate improves — that is a live behavioral property measured from +attempt diagnostics (`completionCalls` per required-mode attempt), recorded in +`000_research.md` at 25/4069 before the change. The change is prompt hardening +against a measured selection failure, not a parser fix. + +## Verification + +`bun run typecheck` plus the focused Kiro suites. The full local suite is +excluded by explicit user instruction for this unit; CI covers it on the PR. diff --git a/devlog/_plan/260830_kiro_post_answer_tool_calls/020_close_out.md b/devlog/_plan/260830_kiro_post_answer_tool_calls/020_close_out.md new file mode 100644 index 0000000000..503f955cef --- /dev/null +++ b/devlog/_plan/260830_kiro_post_answer_tool_calls/020_close_out.md @@ -0,0 +1,81 @@ +# Close-out — terminal completion contract shipped + +Terminal outcome: **DONE**. + +## What shipped + +PR #3012, merged to `dev` at 2026-08-30T15:31:15Z as `f5a625cf3`. Two injected +surfaces now state that the private completion tool is terminal: + +- `kiroCompletionTool()` schema description in `src/adapters/kiro.ts`, which + travels with the tool object the model chooses between. +- `KIRO_COMPLETION_INSTRUCTIONS` in `src/adapters/kiro-constants.ts`, so the + prose contract cannot contradict the schema. + +The mid-task rules are untouched: commentary still does not end the turn, and +the model must still keep using tools before completing. Only what may follow +the completion call is constrained. `tests/kiro-adapter.test.ts` pins both +surfaces and asserts the two commentary sentences survive. + +## Verification + +`bun run typecheck` clean. 197 pass / 0 fail across `kiro-adapter`, +`kiro-stream`, and `tool-catalog-nudge`. `privacy:scan` passes. The regression +test was driven red against the old description first. + +CI on the merged head: 20 checks green. The single failure, on both +`test 2/4` and `macos`, was `release version line > the in-tree version is +never behind a released one` — reproduced identically on a pristine +`origin/dev` worktree at `df8b3882f` with none of this unit's commits, and +owned by PR #3006. A release-version bump does not belong in an unrelated bug +fix. + +## Review findings and what was done with them + +Three findings, all answered on the PR. + +A P1 privacy finding was correct: the measurement table carried a remote +absolute home path into a public devlog directory, which `privacy:scan` rejects. +Fixed in `da9b4989c`; the hostname, PID, and version carry the evidence without +an account identifier. + +A truncation-ordering finding was plausible and turned out to be unreachable. +The completion contract is charged last against +`MAX_KIRO_INJECTED_INSTRUCTION_CHARS` (16384), so in principle a large enough +injected context could slice it mid-clause. Measured: the omission notice tops +out at 922 characters under a hostile 60-tool probe, the nudge ceiling is about +5540 under the 48-tool and 64-character caps, and the contract is 696 — roughly +9900 characters of headroom. The caller's system prompt is not charged to this +budget at all, so no caller input can crowd the contract out. + +A reservation guard plus a budget-exhaustion test were implemented first, then +reverted: the test passed with the guard removed, because the only available +lever for inflating the budget was the uncharged system prompt. A guard for an +unreachable path whose test cannot detect its own removal is worse than the +documented measurement, so the measurement is what stayed. If a future addition +starts charging caller-sized text to this budget, the numbers above are the +starting point. + +A duplicate-`.find` finding was a false positive: one call rendered across two +lines. Applying the proposed fix would have deleted the only lookup. + +## Follow-up + +Causality is not claimed as proven. The pre-change selection rate is recorded at +25 completion calls across 4069 required-mode attempts; the follow-up is the +same measurement on traffic served by a proxy running `f5a625cf3` or later. +Both hosts were on older builds at measurement time (local 2.36.0, `macmini-cf` +2.35.0), so a restart onto current `dev` is the precondition for that comparison. + +## Landed-state verification + +Checked against `origin/dev` after both merges (`6f75616f0`), reading the files +out of the remote ref rather than the working tree: + +- `src/adapters/kiro.ts` contains the terminal schema description. +- `src/adapters/kiro-constants.ts` contains the appended terminal clause. +- `tests/kiro-adapter.test.ts` contains the both-surfaces regression test. +- This unit's close-out record is present. + +Merge trail: `f5a625cf3` (#3012, the contract change) and `6f75616f0` +(#3014, this record). diff --git a/devlog/_plan/260830_lane_r_2941_copilot_vision/000_units.md b/devlog/_plan/260830_lane_r_2941_copilot_vision/000_units.md new file mode 100644 index 0000000000..ac50aef2e3 --- /dev/null +++ b/devlog/_plan/260830_lane_r_2941_copilot_vision/000_units.md @@ -0,0 +1,70 @@ +# Lane R / #2941 — Copilot vision models are cataloged text-only + +## The report + +Every `github-copilot` model arrives in the catalog with `inputModalities: ["text"]`, so Codex refuses image attachments with "This model does not support image inputs" on 32+ models that do accept them (Claude Opus/Sonnet, GPT-4o/4.1/5.x, Gemini, Grok). The same model reached through `openrouter` accepts images, which is what makes this clearly a metadata defect rather than an upstream limitation. + +## Why every path returns nothing + +Copilot's `/models` endpoint nests the flag one level deeper than the flat form the parser reads. No other checked-in fixture uses this shape, which is all a repository search can establish — it says nothing about what other live catalogs return: + +```json +{ + "id": "claude-opus-4.6", + "capabilities": { + "supports": { "vision": true }, + "limits": { "vision": { "max_prompt_images": 20 } } + } +} +``` + +`modelInputModalities()` in `src/codex/catalog/provider-fetch.ts` tries three signals and all three miss: + +- `item.input_modalities` / `item.modalities` — Copilot emits neither. +- `capabilityRecord?.vision` — `capabilityRecord` is the `capabilities` object itself, so its keys are `supports` and `limits`. `.vision` is `undefined`. +- the `capabilities` string array — `supports` is an object, not the literal `true` that scan looks for. + +The fallback chain then lands on `["text"]`. + +Note the shape carries a second `vision` key under `limits`. Any fix that searches loosely for "a vision key somewhere in capabilities" would find `limits.vision`, which is an object describing image count — truthy, and meaningless as a capability signal. + +## Outcome: #2943 shipped the fix, this unit adds the missing coverage + +@Ingwannu opened #2943 for the same defect 17 minutes before my #2944. Their implementation landed as `370052648`, and it is better than mine on one case that matters — see the precedence section below. My unit reduced to the tests and the explanatory comment. + +## Fix + +Read the nested boolean, positioned after the explicit-modality and architecture signals, with precedence **by specificity**: a flat `capabilities.vision` boolean is authoritative whenever present, the nested `supports.vision` boolean is consulted only otherwise, and a non-boolean at either level decides nothing so the remaining signals still apply. + +**The read is not scoped to `github-copilot`, and that is deliberate rather than overlooked.** `modelInputModalities` never receives a provider name, and the nested field is the same kind of evidence wherever it appears — a boolean statement about one model. Scoping it would mean a provider reporting the identical shape gets a worse answer for no reason. What the choice does mean is that any provider emitting `capabilities.supports.vision` as a boolean now has it honoured, so the nested field must behave **exactly** like the flat field it stands in for. That equivalence is pinned by a test comparing both shapes against the same loose capability-array claim. + +Strictness matters in both directions. A truthy test would let the string `"yes"` advertise image support, and coercing a non-record `supports` into a denial would suppress a `features: ["vision"]` signal that is still valid. + +## The precedence mistake, recorded because it nearly shipped + +I first wrote the denial as `flat === false || nested === false` — deny-wins across both sources. It reads as the safe direction and is not. + +On a provider reporting flat `vision: true` with nested `supports: { vision: false }`, deny-wins returns `["text"]` where the old code returned `["text", "image"]`. That is a silent behaviour change in a parser shared by **every** provider, shipped by a patch whose entire purpose was to stop models being wrongly marked text-only. Eleven of twelve capability shapes agree between the two resolutions; that one does not, and it is the one that would have caused a regression. + +A differential probe over both resolutions is what surfaced it — not review, and not green tests, since neither suite covered a disagreeing pair. The case is now pinned: reintroducing deny-wins turns `a flat vision boolean outranks a disagreeing nested one` red. + +## Rejected: a registry seed + +The issue offers "just add `modelInputModalities` to the `github-copilot` registry entry" as the easier route. + +An audit pushed back on my first reason for rejecting it, correctly. `modelInputModalities` is a **per-model** map, not a provider-wide boolean, and other providers do seed selectively — xAI lists specific verified vision ids. So "Copilot serves both vision and text-only models" does not by itself rule out a seed, and a selective one would even help during first start or degraded `/models` discovery, since the registry model list is explicitly a cold-start fallback. + +The real reason to omit it here is narrower: **a seed is only as good as the audited list behind it, and no verified model-by-model Copilot vision list exists.** Writing one from the 32 models named in the issue would be guesswork duplicating a remote catalog that changes without us. A selective seed remains a legitimate follow-up for whoever can audit the list. + +One qualifier on "parse what upstream reports": it is the best per-model evidence available, not an oracle. When two upstream forms disagree the output can still be internally contradictory — a model can end up with `inputModalities: ["text"]` next to `capabilities: ["vision"]`. That contradiction predates this work (flat `false` has always beaten a capability-array `"vision"` string) and is left alone here rather than fixed silently under a Copilot ticket. Resolving how a boolean denial and a loose capability list should reconcile is its own unit. + +## Tests + +`tests/provider-model-discovery-contract.test.ts`, using the reporter's exact payload including the `limits.vision` sibling. + +| Mutation | Expected | +|---|---| +| remove the nested read entirely (pre-fix state) | tri-state test red — reproduces the report | +| drop `nestedVision === false` | tri-state test red | +| `Boolean(nestedVision)` instead of `=== true` | malformed-hint test red | +| move the nested read above the explicit-modality return | precedence test red | diff --git a/devlog/_plan/260830_models_provider_header/000_baseline_and_roadmap.md b/devlog/_plan/260830_models_provider_header/000_baseline_and_roadmap.md new file mode 100644 index 0000000000..f56cca71d6 --- /dev/null +++ b/devlog/_plan/260830_models_provider_header/000_baseline_and_roadmap.md @@ -0,0 +1,168 @@ +# 000 — Models provider-row header: unreadable chip, overlapping name, meaningless controls + +Reported against the running dashboard's Models page with a screenshot: "이부분도 +존나 이상해 신규 2개 꺼짐, 펜, 스위치(이건 뭘하는지도 모르겠음), 사용자 지정창이랑 +마지막 스위치는 뭔지도 모름". + +Two distinct failures are stacked in one header, and they need different fixes: + +- **Geometry** — the "신규 N개, 꺼짐" chip collapses into a rounded blob and the + provider name paints on top of the active count. +- **Meaning** — three controls are operable but unlabeled: a sighted user cannot + tell what they do. This half is not a layout bug and cannot be fixed by + layout. + +As with the sidecar unit, every defect below carries a measured baseline from a +CDP harness (`Emulation.setDeviceMetricsOverride`, dpr 2, live +`getBoundingClientRect`), so each claim is re-checkable. + +## Baseline (ko, provider rows on `#models`) + +Measured with `.tmp/uiux2/head.ts`, which settles on `innerWidth === target` and +on rendered provider rows before reading geometry. The proxy at 127.0.0.1:10100 +supplies the live provider list through the Vite `OPENCODEX_PROXY_TARGET` proxy, +so these are real rows, not fixtures. + +| width | provider | header h | name box w | chip lines | chip w | +|-------|----------|----------|------------|-----------|--------| +| 1440 | opencode-free | 44.8 | 96.5 | 1 | 92.1 | +| 1280 | opencode-free | 55.7 | 67.2 | **2** | 69.6 | +| 1280 | openai | 55.7 | **9.9** | **2** | 57.7 | +| 1100 | opencode-free | **115.1** | **0.0** | **6** | 34.1 | +| 1100 | cursor | **115.1** | **0.0** | **6** | 34.1 | +| 1024 | opencode-free | 75.8 | 96.5 | 1 | — | + +The 1100 row is the screenshot state: a six-line chip 34.1px wide, a name box +measuring **zero**, and a header 2.6x its correct height. 1024 recovers because +the container query at `styles-models-workspace.css:517` moves the actions onto +their own row, which returns the toggle's width. The defect therefore lives in a +**band** (roughly 1040-1380 in this layout), which is why it is easy to miss at +either extreme. + +## Defect 1 — the chip is a shrinkable flex item with no single-line floor + +`.models-chip` (`styles-models-workspace.css:315`) declares +`display: inline-block` plus padding, border and `border-radius`, and nothing +else. Because it sits inside `.row models-provider-toggle` +(`Models.tsx:1226`) and `.row` is `display: flex` (`styles.css:1205`), the chip +is a **flex item**: its `inline-block` outer display is blockified and its +initial `flex-shrink: 1` applies. Measured computed values confirm it — +`white-space: normal`, `flex-shrink: 1`. + +`inline-block` does not imply `white-space: nowrap`. The chip's only floor is +`min-width: auto`, which resolves to the text's **min-content** width — and for +Korean that is nearly one syllable, because CJK line-breaking permits a break +between Hangul syllable blocks. So `신규 2개, 꺼짐` legally becomes +`신규 / 2 / 개, / 꺼 / 짐`, and the fixed padding wrapped around that narrow +column is exactly the observed blob. + +~~Fix: give the chip a single-line floor.~~ **Superseded.** Measurement showed the +chip is not independently broken: it is starved of width by a collapsed ancestor, +and it returns to one line as soon as that ancestor claims its intrinsic width. An +audit also found a chip-level floor unsafe across the eight other `.models-chip` +call sites. The shipped fix leaves the shared `.models-chip` primitive untouched; +it adds an ellipsis only to the toggle-scoped descendant — see `010` and `011`. + +## Defect 2 — the name overflows a zero-width box instead of reflowing + +The name span carries inline `whiteSpace: "nowrap"` (`Models.tsx:1232`) while +`styles-models-workspace.css:267` gives it `min-width: 0` and +`overflow-wrap: anywhere`. Those two fight: `nowrap` suppresses the wrapping +that `overflow-wrap: anywhere` was added to provide, `min-width: 0` lets the box +shrink to nothing, and the default `overflow: visible` means the glyphs keep +painting outside the box — straight across the sibling count. + +Nothing positions these elements on top of each other: there is no `position`, +transform, or negative margin anywhere in the applicable rules. The count is +laid out normally *after* a box that measures 0px, so the collision is pure +overflow. + +Why the header's own `flex-wrap: wrap` does not save it: the header's direct +children are only the toggle button and the actions container. Wrapping does not +propagate into descendants, and the toggle's inner `.row` has no `flex-wrap`, +so the chevron, name, chips and count are locked on one line and shrink against +each other. + +The upstream enabler is `flex: 1` on the toggle (`Models.tsx:1229`), which +resolves to `flex: 1 1 0%` — zero basis, shrink allowed — combined with +`min-width: 0`. The toggle then accepts whatever the wide actions cluster leaves +it rather than forcing a wrap. + +~~Fix: let the toggle's own row wrap.~~ **Superseded.** Inner wrapping is inert: +line construction inside the button runs after its used width has been assigned, so +wrapping redistributes 31px rather than asking for more. Measured: the candidate +left the name box at 0.0px, byte-identical to baseline. The shipped fix gives the +toggle a real flex **basis** so its content enters the header's wrap decision, and +removes every child's min-content floor so the row can always shrink back inside the +card — see `010`. + +## Defect 3 — three controls carry no visible meaning (two switches and the `+`) + +`Switch` (`ui.tsx:8`) accepts a `label` prop and spends it **only** on +`aria-label` (`ui.tsx:11`); its sole child is ``. So +every `Switch` in this codebase is, to a sighted user, an unlabeled toggle. The +user's "이건 뭘하는지도 모르겠음" is a correct reading of the UI. + +Audit of the header controls in visual order: + +| control | visible | aria-label | title | verdict | +|---------|---------|-----------|-------|---------| +| collapse button | chevron + name + count | (children) | — | OK | +| pencil | icon only | 공급자 별칭 편집 | yes | OK | +| default-aliases Switch | knob only | 기본 별칭 사용 | — | **OPAQUE** | +| 사용자 지정 창 | text | — | — | OK | +| `+` | `+` only | 커스텀 모델 추가 | — | **OPAQUE** | +| preset segmented | 프리셋 / 전체 | group only | — | OK | +| 모두 켜기 / 모두 끄기 | text | — | — | OK | +| cap Switch | knob only | 기본 {value} | — | **OPAQUE** | +| cap Select | number only | 기본 {value} | — | **OPAQUE** | + +The pencil is fine precisely because it pairs an icon with `title` — that is the +pattern the opaque controls are missing. + +Two aggravating details: + +1. The cap Switch's accessible name is `기본 128k` — a *value*, not a function. + Even a screen-reader user is not told this governs the context-window cap. +2. For routed providers with the cap off, `(capOn || nativeProviderGroup)` + (`Models.tsx:1360`) hides the Select, so the only thing left is a bare + toggle with no adjacent number to hint at its purpose. The worst state is the + default state. + +### Design constraint + +This is a dense expert control surface: `DESIGN_VARIANCE 2`, `MOTION 1`, density +D6+. The domain gate is strict — no decorative kit, no motion, no new color. The +fix is *labels and reflow*, and the correct instrument is the existing +`title`-plus-icon pattern already proven by the pencil, plus a visible text +label where the header has room for one. + +UX-LAZY-01 was applied to each control before relabeling it rather than after: +every one of them is a real per-provider setting with no correct global default, +so none can be deleted or absorbed. They need meaning, not removal. + +## Roadmap + +- `010` — let the toggle's content be seen, and make every child yield (geometry). + Five designs; the first four were rejected by audit or stress measurement and + `011` records why. +- `020` — control affordances: visible labels for the opaque controls, and a + `Switch` that can render one. + +Each is one PABCD work-phase and one stacked PR. `010` lands first because `020` +adds visible text to the same header and would otherwise be measured against a +layout that is still collapsing. + +## Verification contract + +- Re-measure the sweep at 1440/1280/1100/1024 in ko + ru + fr + en and require: + chip `lines === 1` everywhere, name box width > 0, zero name/count overlap, and + header height within one line-height of the 1440 baseline. +- A focused `gui/tests` regression per phase, driven red against current CSS + first. +- Remote gates only (`ssh lidge` + `ocx-run`); the local full suite is forbidden + by the user. Push `--no-verify`. +- Before/after screenshots at the failing width, per `AGENTS.md` enforce-target. + + + diff --git a/devlog/_plan/260830_models_provider_header/010_toggle_basis_and_shrink.md b/devlog/_plan/260830_models_provider_header/010_toggle_basis_and_shrink.md new file mode 100644 index 0000000000..dd86c0b9d3 --- /dev/null +++ b/devlog/_plan/260830_models_provider_header/010_toggle_basis_and_shrink.md @@ -0,0 +1,286 @@ +# 010 — Let the toggle's content be seen, and make every child yield + +Fixes the geometry half. Meaning is phase `020`. + +**Sixth design.** The five before it were each rejected by an adversarial reviewer or +by a stress measurement, and the rejections are the useful part — they map the shape +of the problem: + +| draft | approach | killed by | +|-------|----------|-----------| +| 1 | shared-chip `nowrap`/`flex-shrink: 0` + inner `flex-wrap` + name ellipsis | inner wrap is inert; shared-chip change unsafe elsewhere | +| 2 | `min-width: max-content` floor | unbounded: 64-char name overflowed the card by 216px | +| 3 | floor + 16rem name cap + 12rem chip cap | 64-char name **and** alias together still overflowed 64px | +| 4 | `flex-basis: auto` + ellipsis on name and chip | the count and badge children kept min-content floors | +| 5 | `flex-basis: auto` + one rule for every child | let the fixed-size chevron shrink to 2.5px | +| **6** | **draft 5 + a `flex: none` exemption for the icon** | — | + +Drafts 2-4 were three versions of one mistake: bound the row by naming the children +that could overflow it, then discover the next child. Draft 5 stops naming children — +and then over-applied, shrinking an icon that has no text to truncate. Draft 6 keeps +the universal rule and exempts the one child whose size is intrinsic rather than +textual. `011` records each failure. + +## The mechanism, measured + +At 1100px the collapsed row measures: + +| element | width | +|---------|-------| +| `.models-provider-head` | 488.0 | +| `.models-provider-actions` | **422.9** (scrollWidth 423) | +| `.models-provider-toggle` | **31.1** (scrollWidth 93) | +| name span inside it | **0.0** (scrollWidth 44) | + +The toggle carries inline `flex: 1` (`Models.tsx:1229`), which resolves to +`flex: 1 1 0%`. That zero **basis** is the defect. A flex item with a zero base size +never reports a content requirement, so the header — which already has +`flex-wrap: wrap` — never learns the toggle needs room and never wraps the actions +cluster to its own line. It keeps one line and hands the toggle the 31px remainder. + +Inside that remainder the name absorbs the whole deficit, measures 0.0px, and — +carrying inline `white-space: nowrap` with default `overflow: visible` — paints its +glyphs across the count. The chip blob is the same starvation, finished by CJK +line-breaking between Hangul syllables. Even the chevron collapses: measured 0px wide +on a starved row, against 14px on a healthy one. + +Two independent properties are required: + +- **Visibility** — the toggle's content must enter the header's wrap decision, so it + receives a share rather than a remainder. That is `flex-basis: auto`. +- **Boundedness** — whatever the content, the row must not force itself wider than the + card. Shrinkability alone does not give this: a flex child stops at its own + `min-width: auto` floor, which is its min-content width, and the *sum* of those + floors can exceed the container. + +The bound has one precondition worth stating plainly, because the round-5 audit caught +the document overstating it: `> *` selects **element** children. A bare string +interpolated directly into the button becomes an anonymous flex item, which no selector +can reach, and it would keep its own min-content floor. Every child today is an +`` or a ``, so the rule covers all of them — but the guarantee is +"every element child, and the markup keeps children element-wrapped", not "anything +anyone adds later". The regression test asserts that second half. + +Draft 2 bought visibility with a raised *minimum*, which is the direct enemy of +boundedness. Draft 4 bought boundedness for the two children it named and left the +count and the discovery badge with their automatic floors intact. + +## The change + +`gui/src/pages/Models.tsx` (—1229), the inline style on the toggle button: + +```diff +- style={{ flex: 1, border: 0, ... }} ++ style={{ flex: "1 1 auto", border: 0, ... }} +``` + +It has to be the TSX: an inline style beats any stylesheet rule short of +`!important`, and reaching for `!important` against markup we own is the wrong +trade. + +`gui/src/styles-models-workspace.css`: + +```css + .models-provider-toggle { + min-width: 0; + } + ++/* Every child, not an enumerated list. Four earlier designs bounded the row by ++ naming the children that could overflow it (name, then alias chip, then the ++ count and badge), and each revision found another one; a child added later ++ would have reintroduced the defect silently. Quantifying over the children ++ instead: min-width:0 removes the automatic min-content floor that stops a flex ++ child shrinking, and the ellipsis makes that shrink legible instead of clipped. ++ Covers every ELEMENT child; a bare interpolated string would become an ++ anonymous flex item no selector can reach, so keep children element-wrapped. */ ++.models-provider-toggle > * { ++ min-width: 0; ++ overflow: hidden; ++ text-overflow: ellipsis; ++ white-space: nowrap; ++} + ++/* The one exemption, and why it is not a return to enumerating children: every ++ other child is TEXT, whose overflow the ellipsis makes legible. The chevron is ++ an icon at a fixed 14px with nothing to truncate, so shrinking it destroys the ++ collapse affordance instead of abbreviating it. Selected by element TYPE, not ++ by identity — any future icon child inherits it without being named. Measured: ++ without this, the adversarial stress case shrinks the chevron to 2.5px while ++ the containment gate still reports success. */ ++.models-provider-toggle > svg { ++ flex: none; ++} +``` + +`min-width: 0` on the toggle is **kept**, not replaced. That also means the existing +assertion at `gui/tests/models-provider-head.test.ts:29` stays green — draft 2 would +have broken it. + +**No `max-width` anywhere, and no child named by identity.** The bound comes from +removing every child's floor, so there is nothing to forget and nothing to re-tune when +a chip is added to this header later. The single exemption selects on element type +(`svg`), which is the distinction that matters: text children abbreviate, icons do not. + +## Measured result + +Gate: in every cell `chipLines === 1`, name width > 0, name text overflow +(`scrollWidth - width`) <= 0, no page overflow. + +| | ko | ru | fr | en | de | +|-|----|----|----|----|----| +| 1440 | pass | pass | pass | pass | pass | +| 1280 | pass | pass | pass | pass | pass | +| 1100 | pass | pass | pass | pass | pass | +| 1024 | pass | pass | pass | pass | pass | + +20/20, worst bad-cell count 0, re-measured after the chevron exemption was added +(draft 6). The chevron also returns to 14px on the rows where it had collapsed to 0. + +Containment, reading `cardScrollOver` = card `scrollWidth` minus its width, where +**positive means the card is silently clipping** (`.models-provider-card` sets +`overflow: hidden`, `styles-models-workspace.css:296`): + +| stress case | baseline | draft 3 | draft 4 | draft 5 | **draft 6** | +|-------------|---------:|--------:|--------:|--------:|------------:| +| 64-char name @1100 | 39 | -2 | -2 | -2 | **-2** | +| 64-char alias @1100 | 16 | -2 | -2 | -2 | **-2** | +| name + alias together @1100 | 229 | **64** | -2 | -2 | **-2** | +| realistic worst row @1100 (64-char name + alias + longest `de` badge) | 229 | — | -2 | -2 | **-2** | +| adversarial: every child forced to 64 chars @1100 | 484 | — | **484** | -2 | **-2** | +| chevron width in that adversarial case | 14 | — | — | **2.5** | **14** | + +The last row is what draft 4 could not survive and what forced the universal rule. It +is deliberately beyond reachable input — the count and badge are localized strings +with small interpolated numbers, not free text — but it is the only case that proves +the bound does not depend on knowing what the children are. + +The final row is the round-5 audit finding, and it is the reason containment alone is +not a sufficient gate: draft 5 reported `cardScrollOver: -2` on the adversarial case +**while** silently shrinking the 14px collapse chevron to 2.5px. A gate that measures +only "does the row fit" certifies a fix that bought the fit by destroying an +affordance. `flex: none` on the icon restores 14px with containment unchanged at -2. + +The gate is not vacuous: against the unpatched stylesheet it reports +`ko/1100 bad=3` (0px name, **6-line** chip) and `ko/1280 bad=4` (name 9.9px, chip 2 +lines). `ru/1100` is green even unpatched — Russian wraps to a wider min-content — +which is why a single-locale check would have missed this defect entirely. + +## Removal test + +| dropped | normal bad cells @ko/1100 | adversarial stress | realistic stress | chevron @adversarial | +|---------|--------------------------:|-------------------:|-----------------:|---------------------:| +| nothing | 0 | -2 | -2 | 14 | +| `flex: 1 1 auto` | **3** | -2 | -2 | 14 | +| the child rule | 0 | **908** | **229** | 14 | +| the `svg` exemption | 0 | -2 | -2 | **2.5** | + +All three are load-bearing and none substitutes for another: the basis fixes the +everyday defect, the child rule bounds the pathological ones, and the exemption keeps +the child rule from paying for that bound with the collapse affordance. Each row was +driven by actually removing the declaration and re-measuring. Contrast drafts 1 and 3, +where four of five and two of three declarations measured inert. + +## Cost of the universal rule + +`white-space: nowrap` on every child means no child of this header can wrap. That is +correct here — it is a single-line identity row of a slug, chips and a count, none of +which should ever wrap — but it is a real constraint on future content. Anything +genuinely multi-line belongs in `.models-provider-body`, not the header. The +alternative was another enumerated exception list, which is what drafts 2-4 already +disproved. + +## What is deliberately NOT changed + +- **The shared `.models-chip` rule.** Only the toggle's own children are touched. The + model-row chips at `Models.tsx:1447-1455` sit in a non-wrapping `.row` with long + translations (de "Benutzerdefiniert", ru "Пользовательская"); a primitive-level + change there was rejected in draft 1. +- `overflow-wrap: anywhere` stays on the existing name rule although the inline + `white-space: nowrap` makes it dead. Removing it is unrelated cleanup; it is noted + so the next reader knows it is inert rather than load-bearing. + +## Diff scope + +- `gui/src/pages/Models.tsx` — one inline style value. +- `gui/src/styles-models-workspace.css` — two rules added (the universal child rule + and the `svg` exemption); the existing `min-width: 0` on the toggle is kept. +- `gui/tests/models-provider-head.test.ts` — extended; the existing line-29 + `min-width: 0` assertion stays valid and must not be removed. +- `gui/tests/helpers/css-declarations.ts` — NEW. The shared source-text CSS readers, + lifted out of `viewport-scroll-caps.test.ts` so two tests can use one copy. +- `gui/tests/viewport-scroll-caps.test.ts` — its four file-local helpers are deleted and + replaced by an import from that module; its assertions are unchanged. + +## Regression test (red first) + +Use the effective-declaration reader so a commented-out or custom-property occurrence +cannot satisfy an assertion. + +**It lives in `gui/tests/helpers/css-declarations.ts`**, which exports +`effectiveDeclaration`, `ruleBodies`, `allRuleBodies` and `withoutComments`. + +That module is part of this change. The reader originated in +`viewport-scroll-caps.test.ts` (PR #2915) as four **file-local, unexported** functions, +so it could not be imported as first planned. B resolved that by moving all four into the +shared module and rewriting the original test to import them — one copy, not the third +copy that copying them here would have produced. + +**What this gate can and cannot see.** The reader's own comment (:53) records that it +does not model competing specificity, `!important`, or at-rule nesting. So it proves +the four declarations exist on the exact selector, and nothing about computed layout: +the ellipsis, the containment numbers, and the 14px chevron are **measurements** +recorded above, not unit assertions. That split is deliberate and is why the tables in +this document are the primary evidence for the fix. + +1. The provider-toggle button in `Models.tsx` carries `flex: "1 1 auto"`. The + negative half must be **scoped to that style object**, not a file-wide search for + `flex: 1` — a legitimate bare `flex: 1` exists at `Models.tsx:2162`, so a global + assertion would be wrong. This is the declaration whose absence reproduces the + user's screenshot. +2. `.models-provider-toggle > *` declares `min-width: 0`, `overflow: hidden`, + `text-overflow: ellipsis` and `white-space: nowrap`, with a comment naming the + defect so the rule is not narrowed back to specific children later. +3. `.models-provider-toggle > svg` declares `flex: none`. Assert this **separately** + from rule 2: it is the declaration whose removal reintroduces the 2.5px chevron, and + a reader who sees only the universal rule is likely to delete it as redundant. +4. Every direct child the toggle renders is an **element**, never a bare string. The + universal selector cannot reach an anonymous flex item, so this is the invariant the + `> *` bound actually rests on. Assert that the JSX between the toggle's opening and + closing tag contains no bare interpolation — all seven children today are `` or + ``. + +A declaration test cannot observe clipping, so the containment table above stays a +recorded measurement rather than a unit assertion. + +## Render grounding + +Screenshots at the failing width, captured from the running dashboard and then **read +back** rather than merely produced: `evidence/010-before-ko-1100.png` and +`evidence/010-after-ko-1100.png` (ko, 1100px, dpr 2, the second chip-bearing provider +row). The before shot is the shipped build with the fix reverted **in the browser** by an +injected override, so both images come from the same code and differ only by the two +declarations. + +| | before | after | +|-|-------:|------:| +| capture height (dpr 2) | 696px | **416px** | +| rows containing ink | 365 | **101** | +| name box | 8.6px, chip on 6 lines | **43.6px, chip on 1 line** | + +Pixel readback is the observation step: ink was counted per row against the sampled +background luminance, which is what confirms the vertical sprawl actually collapsed +rather than the clip rectangle merely shrinking. + +The chevron was verified the same way, since a rendered width is exactly what the round-5 +audit found the numbers hiding. Under the adversarial stress row at 1100: + +| | `getBoundingClientRect` | drawn glyph span | +|-|------------------------:|-----------------:| +| shipped (exemption present) | **14.0px**, `flex-shrink: 0` | 9.5px | +| exemption overridden away | 4.9px, `flex-shrink: 1` | 36.8px of smeared ink | + +Two notes on reproducing this. `Page.captureScreenshot` hangs indefinitely over CDP +unless `Page.bringToFront` is called first. And an injected `