diff --git a/.github/workflows/dev-version-bump.yml b/.github/workflows/dev-version-bump.yml new file mode 100644 index 00000000000..45ecd4ae3fb --- /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/devlog/_fin/260830_kiro_post_answer_tool_calls/000_research.md b/devlog/_fin/260830_kiro_post_answer_tool_calls/000_research.md new file mode 100644 index 00000000000..09d299304d0 --- /dev/null +++ b/devlog/_fin/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/_fin/260830_kiro_post_answer_tool_calls/010_wp2_terminal_completion_contract.md b/devlog/_fin/260830_kiro_post_answer_tool_calls/010_wp2_terminal_completion_contract.md new file mode 100644 index 00000000000..313d49c110b --- /dev/null +++ b/devlog/_fin/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/_fin/260830_kiro_post_answer_tool_calls/020_close_out.md b/devlog/_fin/260830_kiro_post_answer_tool_calls/020_close_out.md new file mode 100644 index 00000000000..503f955cefa --- /dev/null +++ b/devlog/_fin/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/_fin/260831_kiro_pause_path_and_answer_shape/000_research.md b/devlog/_fin/260831_kiro_pause_path_and_answer_shape/000_research.md new file mode 100644 index 00000000000..69e41fa26cd --- /dev/null +++ b/devlog/_fin/260831_kiro_pause_path_and_answer_shape/000_research.md @@ -0,0 +1,130 @@ +# Kiro post-answer tool calls, round two: why the prompt fix failed + +The user reported the same defect a second time, with a hard pair of +requirements: never recur, and do not regress anything. + +## The first fix is live and did not work + +PR #3012 (`f5a625cf3`) appended terminal semantics to both injected surfaces. +That code was serving when the defect recurred: + +| Fact | Value | +| --- | --- | +| Serving proxy | PID 55727, port 10100, version 2.37.0 | +| Proxy start | 09:23:37 | +| Adapter file mtime | 01:28 (same day, before the start) | +| `f5a625cf3` ancestry | ancestor of the serving HEAD | +| Failure | 09:27 | + +So prompt wording is not a sufficient mechanism for this defect. That is the +finding that governs this unit. + +## What actually happened + +A 1454-char answer-shaped message and the next `exec` call came out of ONE +inference, 4ms apart (message `00:27:23.114`, call `00:27:23.118`), with +`sendCount: 1` and no second upstream request. This was never "the model keeps +working after the turn ended" — there was no second turn to keep working in. + +The message text is the proof. It ends: + +> ...이 호출 경계 동작은 문서에 아직 정확히 안 들어가 있습니다. 넣어둘까요.레퍼런스에 +> 있는 "scope persists" 서술이 실측과 어긋나니 그 부분부터 고치겠습니다. + +A permission question, glued with no separator to a sentence that overrides the +question and proceeds. Two emissions merged into one message: the model asked +the user something and then answered itself, inside one inference. + +The adapter behaved exactly as designed. `defer()` holds staged text, a real +tool call proves the turn continues, and the staged prose is released as +`commentary`. + +## Root cause: there is no sanctioned way to pause + +In `required` mode the model has three expressible moves and none of them is +"I have a question, hold for the user": + +- Ordinary prose does not end the turn, by explicit contract. +- The completion tool means "the task is fully complete and no more work or tool + calls are needed" — which a pending question is not. +- A real tool call continues the turn. + +`KIRO_COMPLETION_RETRY_MESSAGE` closes the last door explicitly: *"Do not ask +the user for another task or emit another progress-only message."* + +So a model that wants to ask has no explicitly endorsed blocking-question move. +Pausing is mechanically expressible — the ask tool is advertised and works — but +nothing in the injected contract endorses it, and the one instruction the model +sees at the moment it fails to complete reads as a ban on asking. Continuing to +work is the only move the contract actively endorses, which is what it did. + +## The regression boundary, measured + +Across 644 Kiro rollouts, same-inference prose (>=600 chars) followed by a real +tool call occurs 26 times. Only 4 have a tail that asks the user something. The +other 22 are ordinary progress narration and must keep working unchanged. + +Prose length cannot separate them: + +- 4 question-tailed: 1329, 1454, 1697, 1938 chars. +- 22 legitimate: 608 ... 3141 chars. + +The ranges overlap completely. Any length or volume threshold that catches the +four also catches legitimate narration, and any threshold that spares the 22 +also spares the defect. There is no structural discriminator for the general +prose-plus-tool shape, which an independent read-only design audit confirmed +from the code: at `flushOpen` the adapter knows only that a non-completion tool +was emitted, its restored identity, and its arguments. `stopReason` cannot help +either — Kiro emits `END_TURN` for progress prose, and the adapter deliberately +ignores it when a real tool exists. + +## Rejected candidates + +**Gate the answer-shaped prose.** Rejected: it requires a text heuristic to tell +the 4 from the 22, and the measurement above shows no non-heuristic signal +exists. A regex would decide whether the user sees their agent's work. + +**Discard prose when a real tool follows** (what `consumeSupersededByCompletion` +does on the completion path). Rejected: on the real-tool path no replacement +answer ever arrives, so all 22 legitimate progress messages would silently +vanish rather than arrive later. + +**Isolate the ask tool** (relay `request_user_input`, fail closed on any later +real tool from the same inference). Rejected as the primary mechanism: measured +8 `request_user_input` calls across 644 rollouts, and the +ask-then-another-tool shape occurs **0** times. It guards a case that does not +happen. Shipping a guard for an unreachable path with a test that cannot fail is +the exact trap this unit's predecessor already fell into once. + +## What the evidence does support + +`request_user_input` already terminates a Kiro turn cleanly. Measured three +times in the failing session itself: staged prose, then the ask call, then the +turn yields and the human answers. The pause path exists and works — the model +simply did not take it, because the contract never told it to and the retry +message reads as a ban on asking. + +The completion channel is also content-agnostic: a completion answer yields +`final_answer` and `done(endTurn: true)` regardless of what the answer says. A +question delivered there ends the turn correctly today. + +So the gap is not enforcement, it is expressibility. The contract describes two +states (still working, fully done) for a model that has three (still working, +done, blocked on the user). The fix is to make the third state expressible and +to stop forbidding it. + +## Honest statement of limits + +This unit cannot make the defect impossible without model compliance, and it +will not claim otherwise. Deterministic prevention would need a structural +upstream signal that does not exist: Kiro accepts only automatic or no tool +choice, so the proxy cannot force a typed progress/pause/complete protocol. The +bad and good event streams are observationally equivalent at the adapter. + +What this unit can do is remove the contradiction that made the defect the +model's *only* endorsed move: make the blocked-on-user state expressible, and +stop forbidding it. That is one change and it is testable. It introduces no +adapter-side regression for the 22 — the non-regression test guards their +structural output byte for byte — but it is a prompt change, so it can still +shift what the model chooses to emit. Adapter behavior is pinned; model +behavior is influenced, not fixed. diff --git a/devlog/_fin/260831_kiro_pause_path_and_answer_shape/010_wp2_pause_path.md b/devlog/_fin/260831_kiro_pause_path_and_answer_shape/010_wp2_pause_path.md new file mode 100644 index 00000000000..50229b92444 --- /dev/null +++ b/devlog/_fin/260831_kiro_pause_path_and_answer_shape/010_wp2_pause_path.md @@ -0,0 +1,144 @@ +# wp2 — make the blocked-on-user state expressible + +One change, in the injected contract, carried on every surface that describes +when to complete. It removes the contradiction that made continuing to work the +model's only endorsed move. No adapter enforcement is added, because no +non-heuristic signal exists to enforce on. + +## A blocking question is a valid final answer + +The pause state rides the channel that already terminates correctly. A +completion answer yields `final_answer` and `done(endTurn: true)` +**regardless of what the answer says** — verified in `kiro.ts` at the +completion terminal. So a question delivered there ends the turn today, with no +new event type, no new phase, and no change to `endTurn` semantics for real +tools. + +`KIRO_COMPLETION_INSTRUCTIONS` therefore gains one clause: when the model needs +a decision from the user before it can continue, that question IS its final +answer — deliver it through the completion tool and stop, instead of writing the +question as prose and then answering itself. + +The completion tool's own schema carries the same clause. That description is the +surface the model reads while CHOOSING a tool, and it admitted only "fully +complete"; a model holding a blocking question reads the tool as unavailable and +keeps working, which is the defect. Round one changed only the terminality half of +this description and left the eligibility half narrow, which is part of why prose +alone did not move the outcome. The `answer` property description is widened the +same way, since it is what tells the model what may go in the field. + +This is deliberately NOT conditioned on an ask tool being present. An earlier +draft keyed it on `request_user_input` appearing in the emitted catalog, which +would have named a tool that is absent from most turns and contradicted the +nudge's rule that instruction-only names are not callable (the failure mode +`0325a5afd` fixed for the catalog nudge). The completion tool is always present +when this instruction is emitted — that is the condition under which the +instruction exists at all — so the clause can never name something uncallable. + +`KIRO_COMPLETION_RETRY_MESSAGE` currently ends with *"Do not ask the user for +another task or emit another progress-only message."* In context that forbids +soliciting NEW work, but it reads as a blanket ban on asking, and it is the one +instruction the model sees at exactly the moment it failed to complete. It is +narrowed to keep forbidding a request for another task while making a blocking +question an explicitly acceptable final answer. + +Both sites are compared by constant, never by inline literal +(`currentUim.content !== KIRO_COMPLETION_RETRY_MESSAGE`), and both pinning +tests assert against the constant, so editing the text cannot desynchronize the +equality checks that keep the bounded retry from stacking. + +This depends on model compliance and is labeled as such. It is the reason this +unit does not promise impossibility: the tests prove the contract is delivered, +not that the model obeys it. + +## Ask-tool isolation: considered and dropped + +An earlier draft added a `flushOpen` invariant: once a turn emitted the ask +tool, a later real tool call from the same inference would fail closed. It is not +being implemented, for three independent reasons. + +It is unreachable. Measured across 644 Kiro rollouts: 8 `request_user_input` +calls, and the ask-then-another-real-tool shape occurs **0** times. + +It cannot be keyed safely. `nameMap` aliases every REQUESTED tool before +admission, and admission can omit tools, so `nameMap` membership does not prove +a tool was emitted. Doing it correctly means threading an explicit emitted-ask +identity through `buildRequest`, the fallback state, and `parseKiroStream`. + +It can false-positive. `OcxTool` carries no "asks the user" marker, and the +parser reduces ordinary function tools to the same generic shape, so an unrelated +bare function named `request_user_input` is indistinguishable from Codex's ask +tool. Claiming it "cannot affect any ordinary tool" was an overclaim. + +Three costs and a measured zero benefit. The previous unit already shipped and +then reverted a guard for an unreachable path; this one declines it before the +commit instead of after. + +## What is NOT changed, and why + +No prose-shape gate. No length threshold. No discard of staged commentary. The +measurement in `000_research.md` shows the 4 defect cases (1329-1938 chars) and +the 22 legitimate ones (608-3141) overlap completely, so every such rule is a +coin flip on whether the user sees their agent's work. + +No change to `src/router.ts`, `src/server/lifecycle.ts`, or +`src/server/responses/core.ts`: the Lab core boundary is unrelated and +`tests/core-lab-boundary.test.ts` guards it. No change to the shared +`tool-catalog-nudge` sentence or its pinned test. + +## Tests + +**Red-first, contract shape:** every emitted completion contract carries the +pause clause, and the retry message permits a blocking question as a final +answer. Driven red against the current strings before the edit. + +**Non-regression, the 22:** staged progress prose followed by `exec` still +yields the prose as `commentary` and `done(endTurn: false)` with the tool call +relayed, byte-identical to today, parameterized across the measured lengths +608 / 1329 / 1454 / 1938 / 3141. This is the test that protects the user's +"회귀없도록" requirement, and it must pass unchanged both before and after the +diff. + +## Budget truncation: measured unreachable, pinned by test + +The completion contract is charged last against +`MAX_KIRO_INJECTED_INSTRUCTION_CHARS` (16384), so in principle a large enough +injected context could slice the pause clause mid-sentence. Measured against a +hostile catalog (80 requested tools, 64-character names, 6000-character +descriptions): + +| Charged item | Chars | +| --- | --- | +| Omission notice | 922 | +| Catalog nudge | 2432 | +| Total before the contract | 3354 | +| Headroom | 13030 | +| Contract | 696 | + +Both charged inputs are structurally capped — the notice names at most 12 tools, +the nudge is bounded by the 48-tool and 64-character limits — and the caller's +system prompt is not charged to this budget at all. So no reachable input +approaches the slice boundary. + +A reservation guard is therefore dead code, and the previous unit already proved +its test would be vacuous: it passed with the guard removed. Instead this unit +pins the property that makes truncation unreachable, at BOTH hostile extremes, +asserting the contract arrives COMPLETE including its closing pause clause: + +- **Largest nudge:** `MAX_KIRO_TOOL_COUNT` admitted tools with unique 64-character + names and one-character descriptions. Every admitted tool is named in the nudge, + so this maximizes the charged nudge. Nothing is omitted, which the test asserts + so it cannot pass while silently charging less than it claims. +- **Notice plus nudge:** oversized descriptions on twice the tool limit, which + forces admission to omit and charges the omission notice on top of the nudge. + The test asserts the notice is actually present for the same reason. + +The 6000-character-description shape alone would not have been enough: it reduces +the admitted count and therefore shrinks the nudge, so it never probes the +largest charged input. Both tests fail if a future change lets the notice or the +nudge grow without bound. A reservation's test could not detect its own removal. + +## Verification + +`bun run typecheck`, the focused Kiro suites, and `bun run privacy:scan`. The +full local suite is excluded by explicit user instruction; CI covers it. diff --git a/devlog/_fin/260831_kiro_pause_path_and_answer_shape/020_close_out.md b/devlog/_fin/260831_kiro_pause_path_and_answer_shape/020_close_out.md new file mode 100644 index 00000000000..4036612ea59 --- /dev/null +++ b/devlog/_fin/260831_kiro_pause_path_and_answer_shape/020_close_out.md @@ -0,0 +1,111 @@ +# Close-out: the blocked-on-user state + +Merged. `afc1cc59c` (#3031) carries the contract change, the tests, and the +public documentation. + +## What shipped + +The injected Kiro contract now describes three states instead of two. Alongside +"still working" and "fully complete" there is "cannot continue until the user +supplies a decision, information, or a clarification that only they can give" — +and that question is delivered as the final answer, through the channel that +already terminates the turn correctly. + +Four surfaces carry it, because a model that reads one of them and not the +others gets a contradiction: + +- `KIRO_COMPLETION_INSTRUCTIONS` — the prose contract. +- `KIRO_COMPLETION_RETRY_MESSAGE` — the one instruction the model sees at the + exact moment it failed to complete. Its old tail, *"Do not ask the user for + another task"*, was scoped to soliciting new work but read as a ban on asking + anything. +- The completion tool's schema description — what the model reads while + *choosing* a tool. +- The `answer` property description — what may go in the field. + +Round one changed the terminality half of that schema description and left the +eligibility half saying only "fully complete". That asymmetry is part of why +prose alone did not move the outcome. + +## Review changed the shape twice + +The review record is worth keeping, because both corrections were substantive. + +An independent design audit ran six rounds before the commit. It killed +ask-tool isolation as unreachable (measured: 8 `request_user_input` calls +across 644 rollouts, ask-then-another-tool **0** times), caught a test +precondition that could not fail — `not.toContain("omitted")` against a notice +that says capitalized `Omitted` — and caught the schema description still +admitting only a completed answer. + +PR review then found the trigger was too narrow. "Blocked on a decision" does +not cover "what is the account id" or "which of these paths did you mean", and a +model stuck on a missing value is stuck exactly as hard as one stuck on a +choice. The wording now names a decision, information, and clarification, and +the doc comments say why, so a future reader does not narrow it back. + +Review also asked for ordering assertions on the non-regression test. Counts and +payloads alone would have passed on a reordered stream, or on an early `done` +followed by a second one. + +## What was deliberately not built + +No adapter gate. Across 644 rollouts the same-inference prose-plus-tool shape +occurs 26 times: 4 question-tailed (1329-1938 chars) and 22 ordinary progress +narration (608-3141). The ranges overlap completely, and at `flushOpen` the +adapter knows only that a non-completion tool was emitted, its restored +identity, and its arguments. `stopReason` cannot help, because Kiro sends +`END_TURN` for progress prose too. Every gate is a coin flip on whether a user +sees their agent's work. + +No reservation guard for the injection budget. Both charged inputs are +structurally capped and the predecessor unit already proved that guard's test +passes with the guard removed. Two hostile-catalog tests pin the property +instead, each asserting its own precondition so neither can pass while charging +less than it claims. + +## The limit, stated plainly + +This is influence, not enforcement, and it cannot promise non-recurrence. Kiro +accepts only automatic or no tool choice, so no typed progress/pause/complete +protocol can be forced upstream, and the good and bad event streams are +observationally identical at the adapter. The tests prove the contract is +delivered, not that the model obeys it. + +The goalplan criterion demanding a code-level mechanism was amended by recorded +steering (`260831-c1-amend-prose-only-mechanism`) rather than reinterpreted to +fit the diff. + +## Landed-state verification + +Read out of `origin/dev` after the merge, not the working tree: + +- `src/adapters/kiro-constants.ts` carries the widened trigger on both + constants. +- `src/adapters/kiro.ts` carries it on the schema description and the `answer` + property. +- `tests/kiro-adapter.test.ts` carries the two contract tests and the two + hostile-catalog tests. +- `tests/kiro-stream.test.ts` carries the parameterized non-regression test. +- `docs-site/src/content/docs/reference/adapters.md` documents the pause + semantics; seven translated locales carry the same statement. + +CI on the merged head: 23 checks green, the only non-pass being the +intentionally skipped Windows matrix placeholder. One `macos` failure was +investigated and is unrelated — +`tests/shutdown-launcher.test.ts` spawns a real launcher and waits on POSIX +signal delivery under a 20s bound, contains no Kiro reference, and passed on +rerun of the identical commit. + +Focused evidence at the merged tree: `bun run typecheck` exit 0; 206 pass / 0 +fail / 797 expect() calls across `kiro-adapter`, `kiro-stream`, and +`tool-catalog-nudge`; `bun run privacy:scan` passed. The local full suite was +excluded by instruction; CI covered it. + +## Merge trail + +- `f5a625cf3` (#3012) — round one, terminal completion contract. +- `6f75616f0` (#3014) — round one close-out. +- `1031b6fff` (#3016) — round one landed-state record. +- `afc1cc59c` (#3031) — this unit. + 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 00000000000..32b1a7efd62 --- /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 00000000000..f74141e12c0 --- /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 00000000000..b3b51de7d33 --- /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 00000000000..5d1bb35545b --- /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_sidecar_control_band/010_shared_control_band.md b/devlog/_plan/260830_sidecar_control_band/010_shared_control_band.md new file mode 100644 index 00000000000..8d585899b85 --- /dev/null +++ b/devlog/_plan/260830_sidecar_control_band/010_shared_control_band.md @@ -0,0 +1,86 @@ +# Sidecar control band: one shared start position + +Status: shipped on `codex/sidecar-shared-control-band`. + +## Reported symptom + +The two dashboard sidecar cards looked centre-aligned rather than +left-copy / right-controls, and the two model selects did not start at the +same x. + +## Two independent causes + +Both were measured over CDP in a real browser across the eight shipped +locales, not inferred from the stylesheet. + +### 1. Every two-up card was born stacked + +`.dash-sidecar-grid` used a `21rem` track floor. The stacking container query +fires at `36rem` of card. A two-up card therefore had 309-517px of content -- +always under the threshold -- so it entered the stacked regime immediately: +copy on a full-width flex line, controls on a second full-width line +inheriting `justify-content: flex-end` from the shared +`.dash-delegation-controls` rule. The model select floated in the middle of +the card with the switch pinned to the right edge, which is what reads as +centred. + +The track floor is now `39rem`: 624px of track, 586px of content after the +panel's 2x19px padding, clear of the 576px stacking threshold. A card the grid +places two-up can now hold a real row, and below that the grid drops to one +column where a card is wide enough for the same row. + +The general lesson: the track floor is the width at which a card can render +its intended LAYOUT, not the width at which its widest control stops +overflowing. The old comment stated the second and the number satisfied only +that. + +### 2. The two control groups sized intrinsically + +The groups do not hold the same controls. Web search is one select plus a +label and a switch; vision is two selects. Measured natural widths at 1600px: + +| locale | web search | vision | select start delta | +|--------|-----------:|-------:|-------------------:| +| ja | 268px | 569px | 301px | +| ko | 275px | 569px | 294px | +| zh | 272px | 569px | 297px | +| de | 309px | 569px | 260px | +| ru | 326px | 569px | 244px | +| fr | 344px | 569px | 225px | + +Both groups packed to the card's right edge. Equal right edges with unequal +widths gives unequal left edges, so the start position was a function of +translated label width. + +`flex: 0 0 min(100%, 26rem)` makes the band definite and identical in both +cards, with `justify-content: space-between` packing from the band's left +edge while keeping the trailing switch on the card's right. The vision card no +longer overrides the band width or the copy basis: copy absorbs leftover +width, so a per-card copy basis moved the band's left edge by the difference, +which is why equal band widths alone were not sufficient. + +## Rendered verification + +At 1920/1600/1440/1200/1024/900/760/600/430 across ko/en/ru/fr/ja/de/tr/zh: +band start delta 0px at every cell, no horizontal or vertical overflow, no +truncated select label, no clipped hint. The streaming label also stops +wrapping to three lines, because the band gives it room to stay on one. + +Evidence images in `evidence/`. "Before" is the same build with the shipped +declarations reverted by an injected in-browser override, so the pair differs +only by the fix: 571px of select divergence at ko/1440 and 256px at ko/1024, +both 0px after. + +## Regression coverage + +`gui/tests/sidecar-layout.test.ts` gains two source-oracle assertions -- the +definite unshrinkable band with no per-card override, and a track floor that +must exceed the stacking threshold plus padding. Both were driven red against +the pre-fix values before being committed. + +One trap worth recording: `allRuleBodies` matches a selector everywhere it +appears, including inside `@container` blocks, where the stacked regime +legitimately sets `flex: 0 1 auto`. A row-regime assertion read those +overrides and failed against a correct stylesheet. The new `baseCascade` +helper strips container queries so the base-rule invariants are tested against +the base rules only. diff --git a/devlog/_plan/260830_sidecar_control_band/evidence/010-after-ko-1024.png b/devlog/_plan/260830_sidecar_control_band/evidence/010-after-ko-1024.png new file mode 100644 index 00000000000..341dfa6f352 Binary files /dev/null and b/devlog/_plan/260830_sidecar_control_band/evidence/010-after-ko-1024.png differ diff --git a/devlog/_plan/260830_sidecar_control_band/evidence/010-after-ko-1440.png b/devlog/_plan/260830_sidecar_control_band/evidence/010-after-ko-1440.png new file mode 100644 index 00000000000..4a30133aebb Binary files /dev/null and b/devlog/_plan/260830_sidecar_control_band/evidence/010-after-ko-1440.png differ diff --git a/devlog/_plan/260830_sidecar_control_band/evidence/010-before-ko-1024.png b/devlog/_plan/260830_sidecar_control_band/evidence/010-before-ko-1024.png new file mode 100644 index 00000000000..2e15689f3b0 Binary files /dev/null and b/devlog/_plan/260830_sidecar_control_band/evidence/010-before-ko-1024.png differ diff --git a/devlog/_plan/260830_sidecar_control_band/evidence/010-before-ko-1440.png b/devlog/_plan/260830_sidecar_control_band/evidence/010-before-ko-1440.png new file mode 100644 index 00000000000..51a58c2a71a Binary files /dev/null and b/devlog/_plan/260830_sidecar_control_band/evidence/010-before-ko-1440.png differ diff --git a/devlog/_plan/260830_sidecar_control_band/evidence/020-after-en-1600.png b/devlog/_plan/260830_sidecar_control_band/evidence/020-after-en-1600.png new file mode 100644 index 00000000000..098f9ac140d Binary files /dev/null and b/devlog/_plan/260830_sidecar_control_band/evidence/020-after-en-1600.png differ diff --git a/devlog/_plan/260830_sidecar_control_band/evidence/020-after-ko-1024.png b/devlog/_plan/260830_sidecar_control_band/evidence/020-after-ko-1024.png new file mode 100644 index 00000000000..d7301da9009 Binary files /dev/null and b/devlog/_plan/260830_sidecar_control_band/evidence/020-after-ko-1024.png differ diff --git a/devlog/_plan/260830_sidecar_control_band/evidence/020-after-ko-1440.png b/devlog/_plan/260830_sidecar_control_band/evidence/020-after-ko-1440.png new file mode 100644 index 00000000000..0b73847ecba Binary files /dev/null and b/devlog/_plan/260830_sidecar_control_band/evidence/020-after-ko-1440.png differ diff --git a/devlog/_plan/260830_sidecar_control_band/evidence/020-after-ru-1440.png b/devlog/_plan/260830_sidecar_control_band/evidence/020-after-ru-1440.png new file mode 100644 index 00000000000..8a846b0513a Binary files /dev/null and b/devlog/_plan/260830_sidecar_control_band/evidence/020-after-ru-1440.png differ diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/000_plan.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/000_plan.md new file mode 100644 index 00000000000..e9a52b9c7f5 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/000_plan.md @@ -0,0 +1,44 @@ +# 260831 — priority-70+ train: entitlement floor, roster TTL, Windows spill drain + +Frozen scope taken at 2026-08-31T01:1x KST from the open bug backlog. Three +targets, dependency-ordered. Each implementation phase consumes exactly one +decade doc as one full PABCD cycle. + +| wp | target | doc | priority | +| --- | --- | --- | --- | +| wp0 | this roadmap | `000`-`009` | — | +| wp1 | #3022 entitlement client_version floor + empty-vs-negative roster | `010` | 78/80 | +| wp2 | #3023 roster TTL expiry drops entitled rows | `020` | 71/80 | +| wp3 | #3011 Windows ACL spill stall (PR #3018 audit) | `030` | 71/80 | + +## Why this order + +wp1 is the stack base: it changes what `resolveCodexModelEntitlements` records +for an account. wp2 changes *when* the management surfaces re-read that record. +Landing wp2 first would leave the shared entry point refreshing a value that is +still wrong, so the fix would look effective on a warm cache and fail on a cold +one. + +wp3 is independent of both — it touches `src/responses/spill-store.ts` and +`src/responses/state.ts`, no catalog code — so it does not stack on wp1/wp2 and +can land in parallel. + +## Evidence provenance + +Three read-only `gpt-5.6-sol` high-effort research lanes were dispatched at wp0. +Their file:line findings are recorded in `001`-`003`. Every claim below that is +load-bearing was re-verified directly in the tree by the main session before +being written here. + +## Verification constraint (user-imposed) + +Local full test suites are forbidden for this train. Focused +`bun test ` runs locally; every suite, typecheck, and privacy scan runs on +`ssh lidge` (Linux x86_64, bun + git + gh present). Every completion claim +carries a receipt: command, exit code, pass/fail counts. Each regression test is +driven red against pre-fix code and that red result recorded. + +## Delivery + +Stacked PRs per `DEV-STACK-01`, pushed `--no-verify` (the pre-push hook runs the +forbidden local suite). Merge into `dev` is authorized for this goal. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/001_research_3022_entitlement_floor.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/001_research_3022_entitlement_floor.md new file mode 100644 index 00000000000..e1aa27627fb --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/001_research_3022_entitlement_floor.md @@ -0,0 +1,124 @@ +# 001 — #3022: the entitlement client_version floor is below what upstream honours + +Verified against `origin/dev` `870a2adb6` (package 2.37.0). + +## The two defects + +This issue is two defects wearing one symptom. Fixing either alone leaves the +other able to reproduce it. + +### Defect A — the derived floor is wrong + +`resolveCodexEntitlementClientVersion` has three tiers +(`src/codex/model-entitlements.ts:122`): inbound request version, persisted +runtime `selectedVersion`, then `GATED_MODEL_CLIENT_VERSION_FLOOR`. + +Tier 3 is derived from the bundled snapshot, not hardcoded +(`src/codex/model-entitlements.ts:59`, `:81`, `:83`): + +``` +GATED_MODEL_CLIENT_VERSION_FLOOR = deriveGatedClientVersionFloor(snapshot) ?? FALLBACK +``` + +Measured in-tree: + +``` +GATED_MODEL_CLIENT_VERSION_FLOOR -> 0.142.2 + +src/codex/data/upstream-models.json + gpt-5.6-sol minimal_client_version = 0.142.2 context_window = 372000 + gpt-5.6-terra minimal_client_version = 0.142.2 context_window = 372000 + gpt-5.6-luna minimal_client_version = 0.142.2 context_window = 372000 +``` + +**The repository already contains a live measurement that contradicts its own +snapshot.** `devlog/_fin/260817_native_gpt56_1m_context/001_measurement_evidence.md` +records `GET /backend-api/codex/models?client_version=` against a real Codex +login: + +| client_version | rows returned | +| --- | --- | +| 0.60.0 | 0 | +| 0.142.2 | 5 — **no gpt-5.6** | +| >= 0.144.0 | 8 — includes sol/terra/luna | + +and records the live rows as `minimal_client_version = 0.144.0`, +`context_window = 272000`. The reporter measurement on #2886 (0.142.2 -> 200 +without gpt-5.6; 0.144.0 / 0.146.0 -> 200 with) independently reproduces this on +a different account and machine. + +So the snapshot's `0.142.2` and `372000` are both stale — they are PR #31684-era +values. Deriving the floor from that file faithfully produces a version upstream +does not honour, and tier 3 recreates the very defect #2891 set out to fix. + +**Raising `GATED_MODEL_CLIENT_VERSION_FLOOR_FALLBACK` fixes nothing.** The +expression is `derived ?? fallback`; derivation succeeds, so the fallback is +unreachable (`src/codex/model-entitlements.ts:83`). Confirmed by reading the +code, not assumed. + +### Defect B — an empty roster is recorded as a confirmed negative + +`parseAccountModels` returns a `Set` for any payload whose `models` is an array +(`src/codex/model-entitlements.ts:374`). `{"models":[]}` therefore yields an +**empty but non-null** `Set`. Rows filtered for `visibility === "hide"` or +`supported_in_api !== true` can empty it the same way. + +`fetchAccountModels` then converts non-null into confirmation +(`src/codex/model-entitlements.ts:414`, `:420`): + +```ts +expiresAt: now + (models ? MODEL_ROSTER_TTL_MS : MODEL_ROSTER_FAILURE_TTL_MS), +models: models ?? new Set(), +confirmed: models !== null, +``` + +An empty `Set` is truthy, so the account is marked `confirmed` **and** gets the +five-minute success TTL instead of the fifteen-second failure TTL. Downstream, +the account enters `confirmedAccountIds` with a set that lacks the gated slugs, +and every projection reads that omission as a decided denial +(`:547`, `:573`). Catalog sync then strips the gated bare and selector rows +(`src/codex/catalog/sync.ts:1579`, `:1616`) and runtime auth excludes the account +(`src/codex/auth-context.ts:458`). + +**`models.size > 0` is not a sufficient guard.** The reported short roster +contains `gpt-5.5`, so the set is non-empty while every gated row is absent. The +distinction that matters is whether the roster was obtained under a version +capable of returning the gated rows at all. + +## Why it reaches users + +Tier 1 and tier 2 mask the defect. It surfaces on the path with neither: a +background catalog sync or convergence pass with no inbound request +(`src/codex/catalog/sync.ts:1834`, `src/codex/convergence.ts:409`) on a host +where no Codex runtime was ever resolved, so `codex-runtime.json` carries no +`selectedVersion`. That matches the #2886 reporter's clean-reinstall reproduction +on a machine with no `codex` CLI. + +## Fix surface + +`src/codex/model-entitlements.ts`, and the snapshot only if its stale metadata is +corrected as a separate concern. + +1. Tier 3 must not be a bare snapshot derivation. Take + `max(derived, independently-measured minimum)` so a stale snapshot can lower + documentation but never lower the question we ask upstream. The numeric + comparator at `:88` already supports this. +2. An empty usable roster must be unconfirmed and take the failure TTL. +3. A roster fetched under a version below the trustworthy minimum must not make + omission authoritative for gated slugs. + +## Must not change + +- The fail-closed posture itself (#2550). Unknown stays ineligible; this unit + makes *unknown* distinguishable from *denied*, it does not admit unknown. +- Per-account and per-version cache keys (`:269`) — collapsing them reintroduces + cross-version evidence leakage. +- Inbound/runtime precedence. Hardcoding every request to one version would + advertise models to genuinely older clients (`:100`). + +## Open question carried forward + +The roster contract has no completeness marker and no per-model denial field, so +omission is the only signal available pre-dispatch. Whether `0.144.0` is stable +across all accounts is unproven: both measurements used one credential each, +though they were different credentials on different machines and agreed. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/002_research_3023_roster_ttl.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/002_research_3023_roster_ttl.md new file mode 100644 index 00000000000..d48b5cf796e --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/002_research_3023_roster_ttl.md @@ -0,0 +1,92 @@ +# 002 — #3023: expired roster silently shortens every management model surface + +Verified against `origin/dev` `870a2adb6`. + +## Mechanism + +`listManagementModelRows` builds native rows through `nativeModelRows(config)` +(`src/server/management/model-rows.ts:50`). A gated slug is included only when +`cachedAvailableAccountGatedNativeModels(Date.now(), ...)` returns it +(`src/codex/catalog/metadata.ts:414`). That projection is **synchronous** and +requires a confirmed entry with `expiresAt > now` +(`src/codex/model-entitlements.ts:596`). + +So a never-fetched roster and an *expired* roster project identically: empty. + +`listManagementModelRows` calls only `fetchAllModels` +(`src/server/management/shared.ts:173`), which gathers routed providers and never +resolves Codex entitlements. `/v1/models` differs — it runs `fetchAllModels` and +`resolveCodexModelEntitlements` together (`src/server/index.ts:1143`) — which is +exactly why one `GET /v1/models` repairs all three surfaces. + +## The three surfaces are not identical + +The reporter treated them as one path; they are three paths over one entry point: + +- `/api/models` calls `listManagementModelRows` directly + (`src/server/management/model-routes.ts:352`). +- `/api/client-config` goes through `loadExportModels` + (`:393`, `src/server/management/model-rows.ts:164`). +- `ocx export` does **not** use `loadExportModels`. It requests `/api/models` + over HTTP and serializes the rows itself + (`src/cli/export-command.ts:169`). + +That third detail matters for the test plan: a fixture that stubs rows instead of +going through the real management handler cannot see this defect. + +## Why an unconditional refresh is not acceptable + +The dashboard polls `/api/sidecar-settings` every 5 seconds +(`gui/src/pages/use-dashboard-data.ts:237`), and that route computes vision and +web-search candidates independently +(`src/server/management/config-routes.ts:589`), so each tick reaches the shared +list **twice**. The Models page adds `/api/models` every 10 seconds +(`gui/src/pages/Models.tsx:452`). + +That is ~24 shared-list calls/minute with the dashboard open, ~30 with the Models +catalog active. Polls pause while the document is hidden +(`gui/src/client-resource.ts:538`). An unconditional `resolveCodexModelEntitlements` +at the shared entry point would put credential enumeration on that cadence. + +## Fix surface + +Add a cheap ensure/freshness operation in `src/codex/model-entitlements.ts` and +await it from `listManagementModelRows` before `nativeModelRows`, in parallel with +`fetchAllModels` — the shape `/v1/models` already uses +(`src/server/index.ts:1155`). It must: + +- treat confirmed-empty and the 15-second unconfirmed entry as *cached answers*, + not as cache misses; +- preserve per-account/version keys and in-flight deduplication + (`:223`, `:455`) so concurrent pollers collapse into one fetch; +- never refresh from inside the synchronous `nativeModelRows`. + +## Must not change + +- The expiry check itself. Serving expired grants while refreshing would stop the + visual disappearance but break fail-closed revocation (`:509`). +- `/v1/models` authorization or version behaviour. +- Failure must stay a bounded fail-closed roster result, not an exception: a throw + from the shared entry would degrade sidecar candidates and could turn + client-config into a 503 (`src/sidecar/candidates.ts:29`). + +## The dishonest status is a separate, additive change + +`discovery: {"status":"ok"}` comes from routed-provider discovery +(`src/codex/catalog/provider-fetch.ts:1510`, `src/codex/model-cache.ts:94`), not +from entitlements, which never write it. So "ok" is *true* for what it describes. +Overloading it would erase a simultaneously-correct routed result. The honest fix +is an additive entitlement diagnostic; GUI types currently admit only provider +discovery states (`gui/src/models-groups.ts:2`). + +## Test plan + +- `tests/codex-model-entitlements.test.ts` — repeated fresh ensure calls do zero + refetches; at TTL+1 concurrent callers produce exactly one; failed refresh stays + unconfirmed with no retry for 15s. +- `tests/native-model-toggle.test.ts` — expired confirmed roster, then + `/api/models` still lists sol/terra/luna. Red today. +- `tests/management-client-config-route.test.ts` — same fixture, OpenCode map + contains the GPT-5.6 entries, entitlement fetch count is 1. +- `tests/cli-export-command.test.ts` — point its fake proxy at the real + `/api/models` handler; its current stubbed rows bypass the defective boundary. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/003_audit_3018_spill_drain.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/003_audit_3018_spill_drain.md new file mode 100644 index 00000000000..58c3a136357 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/003_audit_3018_spill_drain.md @@ -0,0 +1,107 @@ +# 003 — #3011 / PR #3018 audit: correct fix, one shutdown blocker + +PR #3018, branch `ingw/fix-3011-async-acl-spill`, exact head `aec717722`, +rebased onto `870a2adb6` by this train. Repository CI green, 0 failures. + +## Verdict: the fix is right, and it is not yet mergeable + +CI-green is not correctness evidence here, because the gap is in a path no test +exercises. + +### What the PR gets right + +It genuinely removes the ACL subprocess wait from the response event loop. On +`dev`, `writeResponseSpillDurably` calls synchronous `harden()` +(`src/responses/spill-store.ts:180`, `:324`) which runs `Bun.spawnSync()` +(`src/lib/windows-secret-acl.ts:307`). `/healthz` shares that Bun fetch handler +(`src/server/index.ts:884`), so it cannot run during the wait — that is the 47s +stall. + +The PR queues Windows publications on a serialized promise tail +(`aec717722:src/responses/state.ts:280-308`) and awaits +`hardenSecretDirAsync`/`hardenSecretPathAsync`, which use `Bun.spawn()` plus +`await proc.exited` (`src/lib/windows-secret-acl.ts:329`). + +Linux and macOS are genuinely untouched: all three async routing points are gated +on `windowsSecretAclApplies()`, and false continues to the existing synchronous +`writeResponseSpillDurably` (`aec717722:src/responses/state.ts:547`, `:575`, +`:1250`). + +No Lab import is introduced, and no Node-only API — `Bun.spawn` is Bun-native. + +### Blocker — graceful shutdown does not drain pending publications + +`responseSpillPublicationTail` is awaited in exactly one place, and it is marked +test-only: + +``` +state.ts:187 let responseSpillPublicationTail: Promise = Promise.resolve(); +state.ts:306 responseSpillPublicationTail = responseSpillPublicationTail.then(...) +state.ts:328 await responseSpillPublicationTail; <- flushPendingResponseSpillsForTests +``` + +`flushResponseState()` — the function shutdown actually calls +(`src/server/lifecycle.ts:492`) — awaits only `persistGate` and the snapshot +write. It never observes the publication tail. Verified by reading the function +body at the PR head. + +The loss is concrete because oversized residents are deliberately excluded from +the snapshot (`aec717722:src/responses/state.ts:1015`): + +```ts +if (state.kind === "resident" && size > SNAPSHOT_ENTRY_MAX_BYTES) continue; +``` + +with `SNAPSHOT_ENTRY_MAX_BYTES = 2 * 1024 * 1024` (`:35`). + +So: a request queues a Windows spill for a payload over 2 MiB and returns while +`icacls` is still in flight. Shutdown flushes the snapshot, which *skips* that +resident because it is oversized, and the spill stub that would have replaced it +is not installed yet. The stop paths then call `process.exit()` +(`src/server/lifecycle.ts:489`, `src/server/management-api.ts:278`, +`src/cli/index.ts:365`), which also bypasses the writer's temp cleanup +(`aec717722:src/responses/spill-store.ts:467-505`). The continuation is lost and a +temp file can be orphaned. + +Before the PR this race did not exist: publication was synchronous, so by the time +the request returned the stub was already installed. + +### Required remediation + +Drain the publication tail to a **stable fixed point** before writing the shutdown +snapshot: repeatedly capture and await the tail until the captured promise still +equals the current one. A single `await` is insufficient because a settling job can +append another (`:306`). + +Surface: `src/responses/state.ts` (production drain + call it from +`flushResponseState` before snapshot serialization), `tests/responses-state.test.ts`, +and a note in `structure/02_config-and-codex-home.md`, whose current text describes +queueing but not shutdown ordering. `src/server/lifecycle.ts` needs no change — it +already calls `flushResponseState()` at the right boundary. + +## Coverage gaps in the PR's own tests + +The three added tests are real regressions — two wait on an injected async runner +the old synchronous path never enters (`aec717722:tests/responses-state.test.ts:720-818`). +They cover yielding, timeout recovery, and same-id supersession. They do not cover: +ordinary (non-timeout) ACL failure, cross-id serialization, shutdown, or +copy-fallback cleanup. + +## Security check + +Required-mode ACL failure appears to fail closed: the required helpers throw +(`src/lib/windows-secret-acl.ts:872`) and the state catch replaces the entry with a +failure marker (`aec717722:src/responses/state.ts:261`). Not covered by a test, +which is why the plan adds one. + +Residual, unresolved without a real Windows host: if ACL hardening fails *and* +unlink also fails, cleanup is best-effort +(`aec717722:src/responses/spill-store.ts:310`, `:498`) and a full payload can remain +on disk. Whether another local user can read it depends on the resulting NTFS ACL. +Recorded as a follow-up, not a blocker for this unit — it predates the PR. + +## Disposition + +wp3 is not "merge #3018". It is: land the drain fix on top of the PR head, prove it +with the shutdown regression, then merge. The author's work is correct as far as it +goes; the missing piece is the boundary his change created. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/004_audit_round1_synthesis.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/004_audit_round1_synthesis.md new file mode 100644 index 00000000000..58c60ba6ceb --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/004_audit_round1_synthesis.md @@ -0,0 +1,136 @@ +# 004 — audit round 1: FAIL, and what it changed + +An adversarial `gpt-5.6-sol` high-effort plan auditor returned **FAIL** on the +first draft of `010`/`020`/`030`. Four blockers, all re-verified in-tree by the +main session before acceptance. This document records the synthesis +(REVIEW-SYNTHESIS-01) and the plan amendments it forced. + +## Blocker 1 (accepted) — wp1 applied model-scoped doubt as account-wide denial + +`CachedAccountModels.confirmed` is **one bit for the whole roster** +(`src/codex/model-entitlements.ts:223-234`), and every projection ignores the +account entirely when it is false: `entitledCodexAccountIdsForModel`, +`availableAccountGatedNativeModels`, and `isDirectCallerEntitledToCodexModel` all +require `confirmedAccountIds.has(accountId)` before consulting the model set +(`:547-550`, `:573-593`, `:527`). + +So the draft's `usable` flag would have thrown away *affirmative* rows too. A +roster fetched under `0.142.2` still legitimately confirms `gpt-5.5`, `gpt-5.4`, +and `codex-auto-review`. Marking the whole account unconfirmed would hide models +the account demonstrably owns, and would re-fetch every 15 seconds forever for a +genuinely un-entitled account. + +`gpt-daybreak-blue-latest` sharpens it: it is in the gated set +(`src/codex/catalog/native-models.ts`) but has **no row in the snapshot at all**, +so no measured minimum exists for it. A blanket version rule would deny it on +every path. + +**Amendment.** Separate the two questions the code currently conflates: + +- *Was this roster a usable answer at all?* -> stays account-scoped + (`confirmed`). Only an unparseable response or an empty parsed roster makes it + false. +- *Is this roster authoritative about a PARTICULAR gated model's absence?* -> + becomes model-scoped, answered by whether the roster's own + `clientVersion` is at or above that model's known minimum. + +Positive evidence needs no version test: a returned row is a grant regardless of +which version asked. Only *absence* needs the version to be trustworthy. A model +with no known minimum (Daybreak) keeps today's behaviour — omission is denial — +because inventing a floor for it would be a guess. + +## Blocker 2 (accepted) — wp2 would enumerate credentials forever when logged out + +`MAIN_CODEX_ACCOUNT_ID` is always a candidate +(`src/codex/model-entitlements.ts:500-506`), but with no credential +`accountCredentialSnapshot` returns null, so the account is filtered out before any +cache entry exists (`:539-550`). A "refresh entries that are missing" rule +therefore **misses forever** on a logged-out host and runs the full resolver on +every poll — the exact cost the plan forbade, at ~24 calls/minute. + +**Amendment.** The ensure needs a bounded negative memo for "no usable credential +for this account" with its own short TTL, checked before enumeration. Regression: +repeated logged-out ensures perform zero credential enumerations after the first. + +## Blocker 3 (accepted) — wp3's drain had no real bound + +The async `icacls` runner's timer calls `proc.kill()` and then **still awaits +`proc.exited`** (`src/lib/windows-secret-acl.ts:329-347`). Killing is not +settling: if the child ignores the kill, the await does not return, so a drain that +only awaits the tail inherits an unbounded wait at shutdown. + +The draft's escape hatch was also incoherent: it said an expired cap would "leave +the resident durably recorded", but the whole reason this matters is that residents +over 2 MiB are *excluded* from the snapshot (`:1002-1018`). There is nowhere for +it to be recorded. + +**Amendment.** wp3 must define settlement, not just waiting: + +1. Drain with a wall-clock cap. +2. On cap expiry the fallback must be a real durable write, not the snapshot — + either force a synchronous publication for the outstanding job, or persist the + oversized resident to its own spill path directly. +3. `Promise.race` alone is forbidden: a late writer publishing after snapshot + serialization is the same lost-continuation bug wearing a timeout. + +If a bounded settlement cannot be established in wp3's P, the honest move is to +split it: land the drain for the common case and file the pathological +never-exiting-`icacls` case separately, rather than shipping a shutdown hang. + +## Blocker 4 (accepted) — wp2's diagnostic had no transport + +`/api/models` returns a bare **array** (`src/server/management/model-routes.ts:352-354`), +and both the GUI and `ocx export` depend on that shape +(`gui/src/pages/Models.tsx:402-417`, `src/cli/export-command.ts:169-185`). A +top-level field breaks them; a per-row field duplicates global state. + +**Amendment.** Drop the diagnostic from wp2 entirely and make it its own +work-phase (wp4) that picks an owning endpoint — `/api/providers` already carries +`discovery`, so an additive sibling there is the natural home. wp2 stays the +refresh fix. + +## Corrections to the research docs (accepted, non-blocking) + +The auditor caught five citation errors and one substantive over-claim. The +over-claim matters: + +- **`372000` does not feed `NATIVE_GPT56_CONTEXT_WINDOW`.** That constant is + independently `272_000` and *overrides* the snapshot for runtime projections + (`src/codex/catalog/metadata.ts:130`, `:155-157`). Verified. So leaving the + snapshot stale is safe for behaviour — raw pinned-entry consumers still expose + `372000` (`tests/codex-catalog.test.ts:2901-2909`), which is a documentation + wart, not a defect. `010`'s decision to not edit the JSON stands, but for a + better reason than it gave. +- Line corrections: hidden-tab polling is `gui/src/client-resource.ts:197-215,270-294` + (not 538); `/v1/models` parallel resolution is `src/server/index.ts:1158-1164` + (not 1155); `src/server/lifecycle.ts:489` does **not** call `process.exit` — the + real exits are `src/server/management-api.ts:280` and `src/cli/index.ts:360,370`; + required-ACL throws are `src/lib/windows-secret-acl.ts:877,881` (not 872). + +## Confirmed sound (no change needed) + +- Every load-bearing factual claim in `001`-`003` held: the floor really is + `0.142.2`, the snapshot really records it, an empty `Set` really earns + `confirmed: true` with the 5-minute TTL (the auditor probed it directly: no + refetch at 15,001 ms, refetch only after 300,001 ms), `flushResponseState` really + ignores the publication tail, and the 2 MiB exclusion really applies to residents. +- wp1 stays fail-closed: the auditor confirmed no path admits an unentitled account. + The draft's flaw was over-denial, not under-denial. +- PR #3018 does not publish an unhardened reference: the temp is hardened before + linking, the copy fallback hardens its destination before returning, and the state + swap happens only after writer success. +- wp1 regressions are genuinely red today, and both masking tests are real: + `tests/codex-model-entitlements.test.ts:226-259` gates its mock at minor version + 142, and `:79-92` asserts *confirmed* for an all-filtered roster. That file + currently passes 20/20. +- Sequencing holds: wp1 and wp2 share `model-entitlements.ts`, so wp2 stacks; wp3 + shares no file with either and is genuinely parallel. + +## wp3 test-plan correction + +The auditor found drafts 3 and 4 would **pass against the PR head already** — +ordinary ACL failure tombstones and cleans up (`:261-266`, +`spill-store.ts:498-505`), and copy-fallback hardening failure already removes the +destination (`:297-315`). They are worth keeping as coverage but must not be +presented as red-first regressions. Only tests 1 and 2 (the shutdown drain) are +genuine red-first proof. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/005_audit_round2_synthesis.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/005_audit_round2_synthesis.md new file mode 100644 index 00000000000..8d6d8d8559f --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/005_audit_round2_synthesis.md @@ -0,0 +1,139 @@ +# 005 — audit round 2: FAIL again, and the collapse it forced + +Same reviewer, re-verification round on the amended plan +(`22e5eebed` -> `9020c0723`). Two of four blockers closed; two stayed open and one +new defect appeared. All re-verified in-tree before acceptance. + +The honest reading: round 1 fixed the *descriptions* but wp1 2b and wp3's fallback +were still unimplementable as written. This round collapses both to something that +can actually be built. + +## CLOSED — blocker 2 (logged-out enumeration) + +The bounded negative memo closes the loop. One correction accepted: credential +commits do not invalidate entitlement state today +(`src/codex/account-store.ts:131`, `src/codex/auth-api.ts:2015`, +`src/codex/model-entitlements.ts:630`), so a fresh login stays invisible for up to +the memo TTL. Amendment: pin the TTL explicitly and clear the memo on known +credential writes. Added to `020`. + +## CLOSED — blocker 4 (diagnostic transport) + +`/api/providers` returns provider objects already carrying `discovery` +(`src/server/management/provider-routes.ts:455`), so an additive sibling is a real +compatible transport. + +## STILL OPEN — blocker 1: "unknown" has nowhere to live + +This is the finding that matters, and it kills 2b as drafted. + +Two facts, both verified: + +1. `CachedAccountModels` records `clientVersion` (`:230`) but + `resolveCodexModelEntitlements` **discards it** when building + `CodexModelEntitlementSnapshot` (`:236`, `:547-550`). The projections literally + cannot see which version answered. +2. The projections are **positive-only**. `entitledCodexAccountIdsForModel` and + `availableAccountGatedNativeModels` compute "which accounts/models are + granted" (`:570`, `:573`). Adding a third boolean term to a positive-only + filter can only do one of two things: narrow it further (redundant — absence + already yields nothing), or *widen* it to include a model upstream never + granted. The second is exactly the fail-closed violation `010` promised not to + commit. + +So "treat absence as unknown rather than denied" has no representation in the +current contract. There is no third state to move a model into; there is only +*granted* and *not present in the output*. + +Third fact, and it makes the draft's own test wrong: **`gpt-5.5` is not +account-gated.** `ACCOUNT_GATED_NATIVE_OPENAI_MODELS` is exactly +`{gpt-5.6-sol, gpt-5.6-terra, gpt-5.6-luna, gpt-daybreak-blue-latest}` +(`src/codex/catalog/native-models.ts:5-10`); `gpt-5.5` is in the ungated native +list at `:70`. Test 4 asserted that `gpt-5.5` survives — but `gpt-5.5` was never +at risk, because `confirmed` only gates the gated set. The test would pass +vacuously and prove nothing about over-denial. + +### Collapse: drop 2b from wp1 + +2b was a safety net for a *future* upstream floor bump, not the repair for #3022. +Change 1 already fixes the reported defect by asking under `0.144.0`. A tri-state +entitlement contract — snapshot field for the answering version, an explicit +per-model minimum map, and projections that admit only `granted` while carrying +`unknown` separately — is a real subsystem change with its own blast radius across +three exported functions and every caller. + +**Decision: wp1 ships Change 1 + Change 2a only.** 2a is well-formed and +account-scoped: an empty parsed roster stops being a confirmation. The tri-state +work becomes `wp5` (`050`), sequenced after the train's user-visible fixes, where +it can be designed rather than smuggled in. + +This is a scope reduction, not a scope *retreat*: #3022's reported symptom is fully +addressed by Change 1, and 2a removes the five-minute lockout that made recovery +slow. What is deferred is hardening against a hypothetical future bump. + +## STILL OPEN — blocker 3: the fallback outruns its own cap + +Round 1 added a wall-clock cap to the drain, then specified a synchronous +publication as the cap-expiry fallback. The reviewer caught that the fallback is +not inside the cap: + +- the synchronous writer hardens the directory and the temp as **separate** calls + (`src/responses/spill-store.ts:324`); +- each hardening call resolves its **own** 30s budget + (`src/lib/windows-secret-acl.ts:799`, `HARDEN_DEADLINE_DEFAULT_MS = 30_000` at + `:255`), with the documented timeout-path worst case ~90s at load and ~60.25s on + the owner path. + +So entering the fallback *after* the cap expired can block for another minute or +more. A 5-second cap followed by a 60-second fallback is not a bound. + +### Amendment: the fallback must share one deadline, and the async writer must be disowned + +wp3 now requires: + +1. A single end-to-end shutdown budget covering drain **and** fallback. +2. The fallback passes its remaining budget down rather than letting each harden + call open a fresh 30s window. `OPENCODEX_ACL_TIMEOUT_MS` shows the deadline is + already parameterizable; the plumbing is the work. +3. Explicit ownership transfer: when the drain gives up on a job, the async writer + must be marked superseded so a late completion cannot publish over the + fallback's result. A late writer winning the race is the same lost-continuation + bug wearing a different hat. +4. New regressions the round-1 list omitted entirely: cap expiry enters the + fallback; the fallback respects the *remaining* budget; a late async completion + after cap expiry does not overwrite. + +**Accepted from the reviewer:** a synchronous stall is acceptable *at shutdown +specifically*, because that is the only path that reaches `flushResponseState()` +and no request is being served. #3011 is about the request path. But it is only +acceptable with an enforceable end-to-end deadline — otherwise wp3 trades a +startup stall for a shutdown hang. + +If that plumbing turns out to be larger than the drain itself, split it: land the +drain with a cap that simply *abandons* (accepting the documented loss for the +>2 MiB case, unchanged from today's behaviour) and file the bounded-fallback work +separately. Abandoning is not worse than the status quo; hanging is. + +## NEW DEFECT — wp4 specified an unreachable state + +`040` listed "confirmed roster that is genuinely empty" as a diagnostic state, +while `010` Change 2a makes every empty parsed roster **unconfirmed**. The state +cannot occur. + +The deeper point: without a completeness marker in the roster contract, the system +genuinely cannot distinguish "this account owns nothing" from "upstream returned an +unusable empty answer". Inventing a diagnostic label for a distinction the data +does not support would be a lie in a status field — the exact failure `002` blamed +`discovery: ok` for. + +Amendment: `040` drops that state and replaces it with `unconfirmed-empty`, +described honestly as "upstream returned no usable rows; we cannot tell whether +that means no entitlement". Recorded in `001`'s open questions as the underlying +contract gap. + +## Non-blocking correction carried + +`010` still repeated the disproven `372000` -> `NATIVE_GPT56_CONTEXT_WINDOW` claim +that `004` had already corrected. Fixed in this round: the constant is +independently `272_000` and overrides the snapshot +(`src/codex/catalog/metadata.ts:130`, `:155-157`). diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/006_audit_round3_synthesis.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/006_audit_round3_synthesis.md new file mode 100644 index 00000000000..c9b65f1cfbd --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/006_audit_round3_synthesis.md @@ -0,0 +1,98 @@ +# 006 — audit round 3: wp1 cleared, wp3 and wp4 corrected + +Round 3 on `cef1ea527`. Blocker 1 **CLOSED**, and the question the whole train +exists to answer came back **Yes**. Two findings remain, both accepted. + +## CLEARED — wp1 as reduced is sound, and it does fix #3022 + +The reviewer traced the reported path end to end and confirmed it: + +> No inbound version and no `codex-runtime.json` makes tier 3 select +> `GATED_MODEL_CLIENT_VERSION_FLOOR`. Change 1 raises that effective floor to +> `0.144.0`; background sync directly invokes the resolver. The valid Plus +> credential is fetched, upstream is queried under `0.144.0`, and the returned +> sol/terra/luna rows enter a confirmed snapshot. Catalog projection then retains +> those gated rows. + +with `src/codex/model-entitlements.ts:122`, `:539`, `:547`, +`src/codex/catalog/sync.ts:1834`, `:1579`. + +Also cleared: + +- **Change 1's `max()` is sound.** It touches tier 3 only; a genuinely older + inbound or runtime version still wins. A later snapshot that lowers its recorded + minimum cannot lower the measured floor — which is the intended safety property, + not a side effect. +- **2a does not cause a retry storm.** The 15s TTL plus flight dedup bounds it to + ~4 fetches/minute per account/version. +- **No over-denial.** Projections still expose only roster-present gated models + (`:573`), so the reduced change cannot widen grants either. + +Residual blockers on #3022 are honest ones: no usable credential, an upstream +failure, or an account that genuinely does not get the rows at `0.144.0`. +Management surfaces that never trigger background discovery are wp2's problem, not +#3022's. + +## STILL OPEN — wp3, on two counts + +### 1. "Enter the fallback at cap expiry" and "fallback uses the remaining budget" contradict each other + +If the drain consumes the whole end-to-end deadline, the fallback inherits zero +time. Round 2 wrote both requirements without noticing they cancel. + +**Amendment.** Split the budget explicitly: the drain gets a sub-deadline, and a +reserved slice belongs to the fallback. Concretely — end-to-end budget `B`, drain +cap `B - R`, fallback reserve `R`, and the fallback receives `R` rather than +whatever happens to be left. `R` must be large enough for one directory harden plus +one file harden at a reduced per-call deadline, since those are two separate calls +(`src/responses/spill-store.ts:324`). + +### 2. Abandon-and-file is a real regression, not the status quo + +This correction matters and the round-2 text was wrong. + +On `origin/dev`, oversized candidates are published **synchronously before the +request returns** (`src/responses/state.ts:382`, `:393` — `admitOversizedCandidate` +calls `writeResponseSpillDurably` inline). So today there is no shutdown-loss +window at all for that case. `030` even says so itself, then contradicted it by +calling abandonment "unchanged behaviour". + +Abandonment is only equivalent to **PR #3018's head**, which is precisely the state +that introduced the loss window. Measuring against the unmerged PR instead of +`dev` is how a regression gets waved through. + +**Amendment.** The split condition is withdrawn. wp3 lands the bounded fallback, or +wp3 does not land. If the budget plumbing proves too large, the correct fallback is +**not** to abandon — it is to keep #3018 unmerged until the drain is complete, +because `dev` is currently *correct* on durability and merely slow on Windows. A +47-second stall is worse UX; a lost continuation is worse behaviour. We do not +trade the second for the first. + +## NEW — wp4 cannot tell empty from failed + +Verified: parsed-empty and network/timeout failure both produce +`{models: new Set(), confirmed: false}` — the success path when `parseAccountModels` +returns an empty set (`src/codex/model-entitlements.ts:414`) and the catch path +(`:424`) are indistinguishable downstream. + +So wp4's `unconfirmed-empty` and "refresh failed" are the same state in the data. +Reporting them as different would be the invented-status-field lie `002` objected +to. + +**Amendment.** wp4 gains an explicit prerequisite: record failure **provenance** on +the cache entry (parsed-empty / http-error / timeout / unparseable) before any +diagnostic claims to distinguish them, plus a regression asserting the two states +are actually distinct. If provenance is not added, wp4 reports one merged +`unconfirmed` state and says so plainly. + +## Standing after three rounds + +- wp1 (#3022, the 78/80 shipped regression): **cleared to implement.** +- wp2 (#3023): no blocker raised in rounds 2 or 3; the memo TTL and invalidation + hook amendments stand. +- wp3 (#3011): blocked pending the budget split and the withdrawal of + abandon-as-acceptable. +- wp4, wp5: sequenced after, with wp4 now dependent on failure provenance. + +Implementation begins with wp1, which is the user-visible regression and the one +the reviewer has now positively traced to a fix. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/007_audit_round4_wp1_plan.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/007_audit_round4_wp1_plan.md new file mode 100644 index 00000000000..76a9dcf92f9 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/007_audit_round4_wp1_plan.md @@ -0,0 +1,97 @@ +# 007 — audit round 4: wp1 plan re-audit against the rebased tree + +Auditor: sol-high subagent, read-only, run after `codex/prio70-train-260831` was +rebased onto `origin/dev` = `7666f7d2a`. Verdict **FAIL** — the approach holds, +the plan's claims about its own tests did not. + +## Findings + +1. `src/codex/model-entitlements.ts:83-86` is exactly `derived ?? fallback`. The + gated set (`src/codex/catalog/native-models.ts:5-10`) is sol/terra/luna plus + Daybreak; the snapshot carries only the three gpt-5.6 rows, each recording + `0.142.2` (`src/codex/data/upstream-models.json:4-66`, `:122-184`, `:238-296`). + Daybreak has no row. Derived value confirmed as `0.142.2`. + +2. **Planned regression 2 would not have gone red.** The existing mock at + `tests/codex-model-entitlements.test.ts:245-246` admits every minor `>= 142`, + and `144 >= 142`. Raising the floor alone leaves it green. The mock threshold + must move to `>= 144` and the surrounding comment at `:238-240` with it. + +3. **Planned regression 4 is not red-first.** "A non-empty roster stays confirmed" + passes before and after. It is a characterization guard against over-reach, and + the plan must stop claiming otherwise. Only 1, 2 (after the mock correction), 3 + and 5 are genuinely red. + +4. The proposed `tests/claude-models-discovery.test.ts` addition needs a + version-sensitive backend. The existing no-inbound mock at `:404-411` answers + Daybreak for every version, so as written the new case is green on both sides. + +5. Only one existing assertion changes outcome anywhere in the suite: + `tests/codex-model-entitlements.test.ts:90`, `confirmedAccountIds.has("main")` + from `true` to `false`. Nothing in `tests/e2e-style/` asserts it. + `tests/codex-catalog-sync-hardening.test.ts:385-442` sends a non-empty usable + roster and `tests/claude-models-discovery.test.ts:518-559` supplies inbound + `0.151.7`, so neither is affected. + +6. No consumer needs an all-filtered response to stay confirmed. Every projection + requires confirmation **and** membership (`:570`, `:578-580`, `:587-593`, + `:611-619`), so an empty set denies identically either way. Collapsing + "zero rows returned" and "rows parsed to empty" is correct under the current + usable-roster contract. + +7. **The 15s TTL is demand-driven, not timer-driven**, so 2a opens no background + churn. Refetch happens only through `/v1/models` + (`src/server/index.ts:1158-1164`), Direct gated authorization + (`src/codex/auth-context.ts:382-385`), catalog sync + (`src/codex/catalog/sync.ts:1834-1840`) and convergence + (`src/codex/convergence.ts:409-416`). Same account and version coalesce onto one + flight (`:461-470`); distinct versions are capped at four concurrent per account + (`:472-483`). Worst case for a legitimately empty account under continuous + polling is roughly four fetches per minute per active version. Sequential + high-cardinality version cycling is concurrency-bounded but not rate-limited — + recorded as pre-existing, out of scope here. + +## Required plan changes (all applied to `010`) + +- Correct the `:245` mock threshold to `>= 144` as part of regression 2. +- Relabel regression 4 as a green characterization guard. +- Give the Claude discovery case a version-sensitive backend. +- **New regression 6:** exercise the composition with a synthetic derived floor + *above* `0.144.0`. Without it, replacing the exported floor with the bare + literal `0.144.0` passes every other test while destroying the stated + future-snapshot property (`010:28-36`). +- **New regression 7:** the all-filtered case must also prove refetch after + 15,001 ms. Flipping `confirmed` while leaving the five-minute TTL in place + would otherwise pass regression 5. + +## Round 5 (same reviewer, amended plan) + +**VERDICT: FAIL** — three further corrections, all now applied to `010`: + +1. Regressions 3 and 7 would not have exercised a real cache entry. + `boundedCacheSet` runs only when `currentCredentialIdentity(accountId)` matches + the snapshot identity (`src/codex/model-entitlements.ts:486-490`, `:330-340`), + and the suite's `credential()` helper mints `test:` + (`tests/codex-model-entitlements.test.ts:28-34`), which matches nothing — so the + TTL assertions would have measured an uncached path. The plan now prescribes the + Direct-caller path (identity is a SHA-256 token fingerprint, `:437-451`) or a + genuinely persisted record, and requires both halves: no refetch before 15s and + exactly one after 15,001 ms. +2. The churn bound wrongly credited continuous dashboard polling. `/api/models` + reaches only `listManagementModelRows` + (`src/server/management/model-routes.ts:352-354`), which never resolves + entitlements (`src/server/management/model-rows.ts:50-55`). The bound belongs to + `/v1/models` and Direct gated demand; dashboard polling becomes a caller only + when wp2 lands, so wp2 inherits the cost. +3. The plan still claimed an unconfirmed account loses `gpt-5.5`/`gpt-5.4`. It does + not: both projections apply the flag only to gated slugs + (`!ACCOUNT_GATED.has(slug) || (confirmed && entitled.has(slug))` at + `src/codex/catalog/sync.ts:1617-1620` and + `src/codex/convergence.ts:280-284`). `confirmed` suppresses account-gated models + only. Retracted. + +## Round 6 + +**VERDICT: PASS.** Two non-blocking notes (a duplicated sentence and +`direct:` vs the actual SHA-256 fingerprint spelling) fixed on the spot. +Cleared to implement. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/008_audit_round5_wp2_plan.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/008_audit_round5_wp2_plan.md new file mode 100644 index 00000000000..908ff7f540b --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/008_audit_round5_wp2_plan.md @@ -0,0 +1,86 @@ +# 008 — audit round 5: wp2 plan re-audit after wp1 landed + +Auditor: sol-high subagent, read-only, run against `codex/prio70-train-260831` at +`1b6b36b96` (wp1 merged into the branch). Verdict **FAIL** — five substantive +corrections plus a stale-citation sweep. All are folded into `020`. + +## Findings + +1. **Citations shifted by wp1** (+45 lines or so in `model-entitlements.ts`). + `:223` → `:323-325`; `:455` → `:524-526`, `:542-552`; `:500-506` → `:556-563`; + `:539-550` → `:391-419`, `:592-602`; `:630` → `:686-693`; expiry `:509` → `:673`. + Also `auth-api.ts:2015` → `:2016`, `server/index.ts:1155` → `:1158-1164`, + `export-command.ts:169` → `:181`, `sidecar/candidates.ts:29` → `:35-40`. + Still valid: `account-store.ts:131`, `model-rows.ts:50`, + `model-routes.ts:352-354`, `Models.tsx:402-417`. + +2. **"Zero credential validation in the steady state" is unachievable as written.** + The cache key is `(accountId, clientVersion)` (`:323-325`), but freshness also + compares `credentialIdentity` (`:517-522`), and obtaining the current identity + reads `auth.json` or the account store (`:378-389`). A pure cache read would need + a cross-process credential-generation signal that does not exist. Redefined + target: **zero token refresh and zero network**, via an identity-only read, with + the full `accountCredentialSnapshot` (`:391-419`) reserved for entries that are + missing, stale, or identity-mismatched. + +3. **The invalidation hook list was incomplete**, and one entry was wrong. + `auth-api.ts:2015` is the branch, not the save (`:2016`), and hooking it misses + new-account login at `:546`. Also required: pool CAS refresh + (`account-store.ts:195-218`, called at `:705`, `:790`), owner/alias refresh + (`:252-309`, called at `:858`), deletion/tombstone (`:312-320` via + `account-lifecycle.ts:158`), and main-token refresh + (`main-account.ts:133-163`, `:219`). External replacement of + `~/.codex/auth.json` cannot be hooked at all, so a bounded identity probe has to + remain. Active-account switch (`auth-api.ts:1630-1641`) is config-only and needs + no clear. One cycle-free mutation epoch at every successful commit and tombstone, + rather than a hook per call site. + +4. **Awaiting in `listManagementModelRows` is structurally safe; rejecting is not.** + The function is already async, every production caller awaits it, and + `nativeModelRows` stays synchronous (`src/codex/catalog/metadata.ts:414`). But a + rejection degrades sidecar to auth slots (`src/sidecar/candidates.ts:35-61`) and + makes client-config answer 503 (`model-routes.ts:393-409`). The ensure must be + non-throwing at its boundary, with a regression for each of those two outcomes. + +5. **The 8-second figure is not a total bound.** `MODEL_ROSTER_TIMEOUT_MS` starts + inside the roster fetch (`:438-445`); credential refresh can spend 30 seconds + first (`main-account.ts:188-190`, `account-store.ts:743-746`). And flight dedup + begins only *after* credential enumeration (`:524-552`), so abandoning a wait + avoids duplicate roster fetches but not repeated credential work. The whole + ensure operation has to be deduplicated, with the deadline anchored to the + flight's start. + +6. **Three of the four regression claims were false reds.** Fresh-zero-refetch, + TTL+1-single-fetch, and unconfirmed-failure-caching are already green through + `:517-552`, and the empty/all-filtered 15s behaviour is already covered by wp1's + own tests (`tests/codex-model-entitlements.test.ts:672-749`). The genuine wp2 + reds are the **credential-read count** and the **logged-out negative memo**. The + `/api/models` and client-config assertions are red only if the fixture supplies a + usable credential and upstream roster — and client-config's expected fetch count + of 1 is red because the current count is **0**, which is the defect itself. The + CLI case is red only after its stub (`tests/cli-export-command.test.ts:51-57`) is + replaced with the real handler. Baseline across the four files: 93 pass, 0 fail. + +7. wp1 removed the concept the plan still named: there is no "confirmed-empty + entry" any more (`:465-478`). The correct noun is "an empty or all-filtered + **unconfirmed** entry within `MODEL_ROSTER_FAILURE_TTL_MS`". + +## Open question — settled + +The plan's default (wait out the refresh) is wrong, and my proposed flat 3s +per-waiter deadline was also wrong: sequential sidecar calls +(`src/server/management/config-routes.ts:589-593`) would accumulate up to 6s. + +**Decision.** One whole-ensure flight carrying `{ startedAt, promise }`, caught +fail-closed, and **never aborted by a management timeout** — the upstream work +always runs to completion so the cache is populated for the next caller. + +- Sidecar candidate paths join with a **0 ms** wait. They never stall; they read + whatever is already cached. +- `/api/models`, `/api/client-config`, integrations and `ocx export` wait up to + **3000 ms**, measured from the flight's original `startedAt` rather than from each + waiter's own arrival. + +That keeps the useful one-shot window for `ocx export` — which is the surface +#3023 actually reported — while a dashboard poll never stalls and converges within +one 5s cycle. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/009_audit_rounds6to8_wp2_and_wp3_review.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/009_audit_rounds6to8_wp2_and_wp3_review.md new file mode 100644 index 00000000000..80586908b38 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/009_audit_rounds6to8_wp2_and_wp3_review.md @@ -0,0 +1,84 @@ +# 009 — audit rounds 6-8: wp2 plan, and the wp3 implementation review + +Two independent sol-high reviewers, both read-only. Recorded together because they +ran concurrently. + +## wp2 plan — rounds 6, 7 and 8 (all FAIL, all folded into `020`) + +**Round 6.** + +1. The whole-ensure flight was underspecified: a bare `{ startedAt, promise }` can + cross-answer different candidate sets, client versions or credential epochs, while + the per-entry dedup it wraps is keyed by account, identity and version + (`:524-552`). Fixed by giving the flight a real key. +2. The mutation-hook set was still incomplete: `native-profile-manager.ts:1307`, + `:1335`, `:1449` write canonical `auth.json` during switch, rollback and recovery + (transitions at `:1318`, `:1453`). These are OpenCodex's own writes, so they belong + in the epoch. +3. The two wait policies could not be expressed by `ensureCodexEntitlementFreshness(config)` + as written, because sidecar reaches the same `listManagementModelRows` + (`src/sidecar/candidates.ts:40`). Needs an options path. +4. The credential-read regression was **false-green**: "zero token refreshes and zero + fetches" is already satisfied today while the full credential snapshot still runs + (`:592-601`). Must assert zero `accountCredentialSnapshot` calls. +5. The negative-memo TTL was never pinned, and "converges within one 5s cycle" + contradicted the acknowledged 30s credential refresh. + +**Round 7.** + +1. The epoch alone cannot fence external `auth.json` writers — the document says so + itself — so a caller holding a new identity could still join an old-identity + flight. The flight key now carries the identity vector + `(accountId, credentialIdentity | null)`. +2. The negative memo could still be published against a login that landed mid-flight. + Publication is now fenced on the captured identity vector, and expiry is measured + from the **absence observation**, not from settlement — a flight that spent 30s in + a credential refresh must not hand out evidence that is treated as 5s fresh. + +**Round 8 (final).** + +The key still omitted the **workset**. Candidate set, version, epoch and identities +can be unchanged while an entry expires mid-flight: a flight that started when B was +fresh refreshes only A, B expires, a second caller computes the same key and joins, +and the resolver's `now` is fixed from flight start (`:586`) so B stays a cache hit +(`:517-522`). `ocx export` — the surface #3023 actually reported — then returns short +rows having refreshed nothing. The key now includes normalized +`needsRefreshAccountIds`, or joining requires a subset relationship. + +wp2 is planned but **not implemented**. The document is now implementable; the work +itself is the next cycle. + +## wp3 implementation review — FAIL, one high finding + +Reviewed commit `135f22932` (on top of Ingwannu's `aec717722`). + +**Confirmed correct, no change needed:** drain ordering before the 2 MiB snapshot +exclusion (`state.ts:1236-1247` vs `:1133`); the stable-fixed-point loop with no bare +`Promise.race` (`:443-450`); the budget split `B=5000`/`R=4000` with the fallback +receiving `R`; the shared ACL deadline reaching both the directory and temp hardeners +(`spill-store.ts:197-227`, `:440-452`); test 2c's six `icacls` calls and its +max-timeout assertion, which would genuinely detect a fresh 30s window. No +MUST-NOT-CHANGE violation, no payload or token logging. + +**HIGH — the abandoned writer can still publish, and can orphan a temp.** The async +writer creates and fills its temp *before* awaiting file ACL hardening +(`spill-store.ts:505-511`). At cap expiry the fallback marks the job cancelled and +drops its tracking (`state.ts:417-422`) but takes no ownership of that temp; the drain +returns (`:446-448`) and shutdown exits (`management-api.ts:278-280`, +`cli/index.ts:365-370`). A resuming writer publishes *first* (`spill-store.ts:515-529`) +and only then hits the cancellation check (`state.ts:257-260`). State overwrite +prevention is airtight; filesystem publication and cleanup are merely narrow. The +commit's own test encoded the violation as expected, asserting **two** publications +(`tests/responses-state.test.ts:999-1000`). + +**MEDIUM — the fixed point has no regression behind it.** The shutdown tests queue a +single job (`:823-891`), so a one-shot tail await would pass them. The loop is right; +nothing proves it stays right. + +Both sent back for repair before wp3 lands. + +## Residual risk carried + +A real Windows host is still needed to prove NTFS inheritance, `icacls` timeout +behaviour, and unlink semantics while `icacls` holds a path. Everything above was +exercised through the repository's injected Windows/ACL runners. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/010_wp1_entitlement_floor_and_empty_roster.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/010_wp1_entitlement_floor_and_empty_roster.md new file mode 100644 index 00000000000..43d3fc703f1 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/010_wp1_entitlement_floor_and_empty_roster.md @@ -0,0 +1,206 @@ +# 010 — wp1: entitlement floor + empty-vs-negative roster (#3022) + +Stack base. Consumes `001`. One PABCD cycle. + +Branch: `codex/3022-entitlement-floor-empty-roster` off `origin/dev`. + +## Change 1 — tier 3 stops trusting the snapshot alone + +`src/codex/model-entitlements.ts` + +Add an independently measured minimum next to the derivation, and take the higher +of the two. The snapshot may raise the floor; it may never lower it below what we +have measured upstream to honour. + +```ts +/** + * Lowest client_version measured to actually return the gated rows. + * + * devlog/_fin/260817_native_gpt56_1m_context/001_measurement_evidence.md and the + * #2886/#3022 reporter captures agree: 0.142.2 returns 200 with five rows and no + * gpt-5.6; 0.144.0 and above return the gated rows. The bundled snapshot records + * 0.142.2, so a derivation that trusts it asks a question upstream answers with + * an empty gated set. + */ +const MEASURED_GATED_CLIENT_VERSION_MINIMUM = "0.144.0"; +``` + +and the exported floor becomes the max of `deriveGatedClientVersionFloor(...)`, +the measured minimum, and the existing fallback only when derivation yields +nothing. `compareClientVersions` (`:88`) already does the ordering. + +Keep `deriveGatedClientVersionFloor` exported and unchanged in behaviour — it is +separately tested and its job (read the snapshot faithfully) is still correct. The +correction belongs at the composition site, so a future snapshot refresh that +records `0.144.0` or higher takes over naturally and the constant becomes inert +rather than conflicting. + +Tiers 1 and 2 are untouched. An inbound or runtime version still wins, because +those describe a real client and this constant does not. + +## Change 2 — separate "usable answer" from "authoritative about this model" + +> Amended after audit round 1 (`004`, blocker 1). The first draft used one +> account-wide `usable` flag, which would have discarded affirmative rows too. + +Same file, `fetchAccountModels` (`:414`). + +Today: + +```ts +expiresAt: now + (models ? MODEL_ROSTER_TTL_MS : MODEL_ROSTER_FAILURE_TTL_MS), +models: models ?? new Set(), +confirmed: models !== null, +``` + +An empty `Set` is truthy, so `{"models":[]}` earns `confirmed: true` and the +five-minute success TTL. + +`confirmed` is a single bit for the whole account (`:223-234`) and every gated +projection requires it (`:547-550`, `:573-593`, `:527`). It cannot carry a +per-model judgement, which is why 2b had to move to wp5. + +Correction from audit round 5: an unconfirmed account does **not** lose +`gpt-5.5`/`gpt-5.4`. Both projections skip the flag entirely for any slug outside +`ACCOUNT_GATED_NATIVE_OPENAI_MODELS` — the filter is +`!ACCOUNT_GATED.has(slug) || (confirmed && entitled.has(slug))` +(`src/codex/catalog/sync.ts:1617-1620`, `src/codex/convergence.ts:280-284`). So +`confirmed` suppresses account-gated models only, and the earlier draft's worry +about ungated native rows was unfounded. What it *does* mean is that a wrong +`confirmed: true` on an empty roster is a confirmed denial of sol/terra/luna — +which is the defect. + +Two distinct changes: + +**2a. Account-scoped: an empty parsed roster is not a confirmation.** +`{"models":[]}`, and an all-filtered roster, mean no usable evidence. Set +`confirmed: false` and take `MODEL_ROSTER_FAILURE_TTL_MS` (15s) rather than +locking in a wrong answer for five minutes. A non-empty roster stays confirmed. + +**2b — deferred to wp5.** See `005` (audit round 2, blocker 1). The draft wanted +model-scoped absence authority: "absence of gated model *M* is denial only when the +answering version could have returned *M*". It is not implementable inside this +cycle, for two verified reasons. + +First, the answering version is not visible where the decision happens. +`CachedAccountModels` records `clientVersion` (`:230`), but +`resolveCodexModelEntitlements` discards it when building the snapshot (`:236`, +`:547-550`). + +Second, the projections are **positive-only**: they compute which accounts/models +are *granted* (`:570`, `:573`). A third boolean term in a positive-only filter can +only narrow it (redundant — absence already yields nothing) or widen it to include +a model upstream never granted. There is no third slot for "unknown" to occupy, so +expressing it means changing the snapshot contract and all three exported +projections. That is a subsystem change, not a line. + +Change 1 already fixes the reported defect by asking under `0.144.0`. 2b was only +ever a safety net against a *future* upstream bump, so it goes to `050` where it +can be designed with a real tri-state contract. + +## Change 3 (bounded) — correct the stale snapshot metadata + +`src/codex/data/upstream-models.json`: the three gated rows record +`minimal_client_version: 0.142.2` and `context_window: 372000`, both contradicted +by the in-repo live measurement (`0.144.0`, `272000`). + +Treat this as **optional for this cycle and out of scope if it moves anything +else.** The file is a pinned catalog snapshot consumed as exact model metadata; the +context-window value has its own pinned-entry tests +(`tests/codex-catalog.test.ts:2901-2909`). + +Correction from audit round 1: `372000` does **not** feed +`NATIVE_GPT56_CONTEXT_WINDOW`. That constant is independently `272_000` and +overrides the snapshot for runtime projections +(`src/codex/catalog/metadata.ts:130`, `:155-157`). So the stale value is a +documentation wart with no behavioural reach, which is *why* leaving it is safe. + +Decision for this cycle: **do not edit the JSON.** Change 1 makes the stale value +harmless, and the max-composition means correcting it later is safe. Record the +staleness as a follow-up so it is not lost. + +## Regressions + +> Amended after audit round 4 (`007`). Two of the original claims about these +> tests were wrong, and two regressions were missing. Red-first status is now +> marked per case rather than asserted for the set. + +`tests/codex-model-entitlements.test.ts` + +1. **Red-first.** Effective floor is `0.144.0`. Red now: returns `0.142.2`. +2. **Red-first only after the mock is corrected.** No inbound and no runtime + version, upstream mock returning gated rows only at `>= 0.144.0` -> the request + uses `0.144.0` and sol/terra/luna are available. The existing test at `:226` + mocks the gate at minor `>= 142`, and `144 >= 142`, so raising the floor alone + leaves it green — that mock threshold moves to `>= 144` as part of this + regression, along with the comment at `:238-240` that explains it. +3. **Red-first.** `{"models":[]}` -> account not confirmed, failure TTL. Red now: + confirmed with the 5-minute TTL (probe: no refetch at 15,001 ms, refetch only + after 300,001 ms). + + Audit round 5, finding 1: the TTL half of this only proves anything against a + **real cache entry**. `boundedCacheSet` runs only when + `currentCredentialIdentity(accountId)` equals the snapshot's identity + (`src/codex/model-entitlements.ts:486-490`, `:330-340`), and the suite's + `credential()` helper mints `test:` + (`tests/codex-model-entitlements.test.ts:28-34`), which matches nothing. Use the + Direct-caller path — `isDirectCallerEntitledToCodexModel` with + `directHeaders(...)`, whose identity is a stable SHA-256 token fingerprint + (`direct:`, `src/codex/model-entitlements.ts:437-451`) — or a genuinely + persisted pool/main record. Then assert both halves: **no** refetch before 15s, and **exactly one** after 15,001 ms. +4. **Green on both sides — characterization guard, not a red-first regression.** A + non-empty roster stays confirmed. It bounds 2a against over-reach: only the + *empty* case may change, so an ordinary short roster must still confirm the + account and expose whatever it grants. +5. **Red-first.** The existing "all rows filtered as hidden/api-disabled" case at + `:79` currently asserts *confirmed* at `:90`; it must assert unconfirmed. This + is the single existing assertion anywhere in the suite whose outcome changes, + and it is an intentional flip called out for review rather than a quiet edit. +6. **Red-first. New in round 4.** The floor is a *composition*, so test it as one: + with a synthetic derived floor above `0.144.0`, the higher value must win. + Without this, replacing the export with the bare literal `0.144.0` passes every + other case while destroying the future-snapshot property stated at `:28-36`. +7. **Red-first. New in round 4.** The all-filtered case must also prove refetch + after 15,001 ms, under the same real-cache-entry requirement as regression 3. + Flipping `confirmed` while leaving the five-minute TTL in place would otherwise + pass regression 5 with the bug still present. + +`tests/claude-models-discovery.test.ts` beside the client-version forwarding test +at `:518`: with no inbound or runtime evidence, `/v1/models` still exposes the +gated rows. The backend for this case must be **version-sensitive** — the existing +no-inbound mock at `:404-411` answers Daybreak for every version, so a +version-blind copy of it is green on both sides and proves nothing. + +## Churn bound for 2a (audit round 4, finding 7) + +The 15s failure TTL is demand-driven, not timer-driven, so shortening it opens no +background traffic. Refetch happens only through `/v1/models` +(`src/server/index.ts:1158-1164`), Direct gated authorization +(`src/codex/auth-context.ts:382-385`), catalog sync +(`src/codex/catalog/sync.ts:1834-1840`) and convergence +(`src/codex/convergence.ts:409-416`). Identical account and version coalesce onto +one in-flight request (`:461-470`), and distinct versions are capped at four +concurrent flights per account (`:472-483`). + +Worst case for a legitimately empty account is about four fetches per minute per +active version under continuous **`/v1/models` or Direct gated** demand. +Correction from audit round 5: this is **not** dashboard polling today. +`/api/models` reaches only `listManagementModelRows` +(`src/server/management/model-routes.ts:352-354`), which never resolves +entitlements (`src/server/management/model-rows.ts:50-55`). Dashboard polling +becomes a caller of this path only once wp2 lands, so wp2 inherits the cost bound +rather than wp1 paying it. + +Sequential high-cardinality version cycling is concurrency-bounded but not +rate-limited; that is pre-existing and out of scope here. + +## Verification + +Focused locally during iteration; full suite + typecheck + privacy scan on +`ssh lidge`. Receipt recorded in `070_outcome.md`. + +## Out of scope + +Routing, dispatch, the `372000` context window, the roster contract's lack of a +completeness marker (recorded in `001` as an open question), sequential +version-cycling rate limits, and anything under `src/lab/`. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/020_wp2_roster_ttl_refresh.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/020_wp2_roster_ttl_refresh.md new file mode 100644 index 00000000000..12f1ad07e51 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/020_wp2_roster_ttl_refresh.md @@ -0,0 +1,301 @@ +# 020 — wp2: refresh the expired roster at the shared entry point (#3023) + +Stacks on wp1. Consumes `002`. One PABCD cycle. + +Branch: `codex/3023-roster-ttl-refresh`, based on the wp1 head (stacked child). + +## Why it stacks rather than lands independently + +wp2 makes the management surfaces re-read the entitlement record. wp1 fixes *what* +that record contains. Landing wp2 alone would refresh a still-wrong answer: the +bug would appear fixed on a warm cache and persist on a cold one. + +## Change 1 — a conditional ensure, not an unconditional resolve + +New export in `src/codex/model-entitlements.ts`: an ensure/freshness operation that +enters the real resolver **only** for credential/version entries that are missing +or past their own deadline. It must treat as cached answers: + +- a confirmed roster inside `MODEL_ROSTER_TTL_MS`; +- an empty or all-filtered UNCONFIRMED entry within `MODEL_ROSTER_FAILURE_TTL_MS` (wp1 + removed the confirmed-empty case entirely, `:465-478`); +- an unconfirmed failure entry inside `MODEL_ROSTER_FAILURE_TTL_MS`. + +It must reuse the existing per-account/version keys and in-flight deduplication +(`:323-325`, `:524-526`, `:542-552`) so concurrent pollers collapse into one upstream fetch. + +### Amendment after audit round 1 (`004`, blocker 2): the logged-out hole + +"Refresh entries that are missing" **misses forever** when there is no credential. +`MAIN_CODEX_ACCOUNT_ID` is always a candidate (`:556-563`), but +`accountCredentialSnapshot` returns null without one, so the account is filtered +out before any cache entry is created (`:391-419`, `:592-602`). Nothing is ever cached, so +every poll re-enters the full resolver — ~24 times/minute, which is precisely the +cost this cycle forbids. + +So the ensure needs a **bounded negative memo**: "no usable credential for account +X as of T", with its own short TTL, checked before credential enumeration. Absence +of a credential is a cacheable answer, not a cache miss. + +Regression: repeated logged-out ensures perform zero credential enumerations after +the first. This is the assertion that proves the steady state, and it is red +against a naive implementation. + +### The memo needs an invalidation hook (audit round 2 — `005`) + +Credential commits do not invalidate entitlement state today +(`src/codex/account-store.ts:131`, `src/codex/auth-api.ts:2016`, +`src/codex/model-entitlements.ts:686-693`). So a negative memo means a **fresh login +stays invisible until the memo expires** — the user logs in and the dashboard still +shows nothing. + +Two requirements, both testable: + +1. **TTL pinned at 5000 ms.** It bounds how stale a successful login can look, so it + is a UX number rather than an implementation detail, and it is written here so a + later change has to argue with a number instead of a vibe. +2. **Clear the memo on every credential write.** Regression: log in, then the very + next ensure sees the credential without waiting out the TTL. + +### The full hook set (audit round 5 — `008`, finding 3) + +Round 2's two hooks were not enough, and one was misidentified. +`src/codex/auth-api.ts:2015` is the *branch*; the save is `:2016`, and hooking that +line still misses new-account login at `:546`. The complete set of local credential +mutations: + +- `src/codex/account-store.ts:131` — the commit primitive (correctly identified). +- `src/codex/auth-api.ts:2016` and `:546` — existing-account save and new-account + login. +- `src/codex/account-store.ts:195-218` — pool CAS refresh, called at `:705`, `:790`. +- `src/codex/account-store.ts:252-309` — owner/alias refresh, called at `:858`. +- `src/codex/account-store.ts:312-320` — deletion and tombstone, via + `src/codex/account-lifecycle.ts:158`. +- `src/codex/main-account.ts:133-163` and `:219` — main-token refresh. +- `src/codex/native-profile-manager.ts:1307`, `:1335`, `:1449` — canonical + `auth.json` writes during native-profile switch, rollback and recovery, with the + successful transitions at `:1318` and `:1453`. Added after audit round 6 + (finding 2): these are OpenCodex's own writes, not external ones, so they belong in + the epoch rather than being left to the identity probe. Regression: a credential + write that lands **during** an in-flight ensure must not be masked by that flight's + result. + +`src/codex/auth-api.ts:1630-1641` (active-account switch) is config-only and needs +**no** memo clear. + +Two consequences for the design: + +1. **One cycle-free mutation epoch**, incremented at every successful commit and + tombstone, rather than a clear-call bolted onto each of the eight call sites. + Eight hooks is eight chances to miss one, and the miss is silent. +2. **A bounded identity probe has to remain.** External replacement of + `~/.codex/auth.json` — another Codex process, a manual edit, `codex login` outside + this proxy — cannot be hooked at all. The memo can be cheap, but it cannot be + authoritative. + +Without this, the cycle fixes a missing-rows bug by shipping a different +missing-rows bug. + +## Change 2 — await it from the shared entry point + +`src/server/management/model-rows.ts:50`, `listManagementModelRows`: await the +ensure in parallel with `fetchAllModels`, before `nativeModelRows` — the shape +`/v1/models` already uses (`src/server/index.ts:1158-1164`). + +The two wait policies cannot be expressed by a bare call (audit round 6, finding 3): +sidecar reaches this very function (`src/sidecar/candidates.ts:40`), so the waiting +policy has to be a parameter of the entry point rather than a property of the +ensure. + +```ts +export async function listManagementModelRows( + config: OcxConfig, + options: { entitlementWaitMs?: number } = {}, +): Promise { + const [routed] = await Promise.all([ + fetchAllModels(config), + ensureCodexEntitlementFreshness(config, { waitMs: options.entitlementWaitMs ?? 3_000 }), + ]); +``` + +Sidecar passes `{ entitlementWaitMs: 0 }`; `/api/models`, `/api/client-config`, +integrations and `ocx export` take the 3000 ms default. + +This repairs all three reported surfaces at once because they funnel here: +`/api/models` directly, `/api/client-config` via `loadExportModels`, and +`ocx export` by requesting `/api/models` over HTTP +(`src/cli/export-command.ts:181`). + +**Failure must not throw.** A rejection here would degrade sidecar candidates +(`src/sidecar/candidates.ts:35-40`) and could turn client-config into a 503. The +ensure resolves with a bounded fail-closed result; the rows stay short, which is +the honest outcome, and Change 3 makes that visible. + +## Cost bound (the constraint that shapes this) + +The dashboard reaches this entry point ~24 times/minute (`/api/sidecar-settings` +every 5s, computing vision and web-search candidates independently), ~30 with the +Models page open. Polls pause on a hidden document. + +So the ensure must be a **cache read** in the steady state. Corrected after audit +round 5 (`008`, finding 2): "no credential validation" is not achievable as stated. +The cache key is `(accountId, clientVersion)` (`:323-325`) but freshness also +compares `credentialIdentity` (`:517-522`), and reading the current identity means +touching `auth.json` or the account store (`:378-389`). A pure cache read would +need a cross-process credential-generation signal that does not exist. + +Measured target, restated honestly: **zero token refresh and zero network** with a +fresh roster. An identity-only read is permitted; the full +`accountCredentialSnapshot` (`:391-419`) is reserved for entries that are missing, +stale, or identity-mismatched. That assertion is a test, not a hope. + +## Change 3 — moved out of this cycle + +> Removed after audit round 1 (`004`, blocker 4). The draft named no transport. + +`/api/models` returns a bare **array** (`src/server/management/model-routes.ts:352-354`) +and both the GUI and `ocx export` depend on that shape +(`gui/src/pages/Models.tsx:402-417`, `src/cli/export-command.ts:169-185`). A +top-level field breaks them; a per-row field duplicates global state on every row. + +The honest diagnostic therefore needs its own endpoint decision, which is a design +question, not a line of code. It is now **wp4** (`040`). wp2 is the refresh fix and +nothing else. + +## Regressions + +> Corrected after audit round 5 (`008`, finding 6). Three of the four original +> claims were false reds — already green through `:517-552` and wp1's own tests at +> `tests/codex-model-entitlements.test.ts:672-749`. Baseline across the four files +> is 93 pass / 0 fail, so a claim of "red today" has to be earned per case. + +**Genuine reds (the wp2 defect itself):** + +- `tests/codex-model-entitlements.test.ts` — **credential-read count**: after priming, + repeated ensures perform **zero `accountCredentialSnapshot` calls**. Corrected after + audit round 6 (finding 4): asserting only "zero token refreshes and zero fetches" + is false-green, because the current resolver already satisfies it while still + invoking the full credential snapshot (`:592-601`). Identity-only reads are + permitted and counted separately. +- `tests/codex-model-entitlements.test.ts` — **logged-out negative memo**: repeated + ensures with no credential perform exactly one credential enumeration, not one per + call. Red against a naive "refresh what is missing" implementation, which never + caches the absence and so re-enters the resolver ~24 times a minute. +- `tests/codex-model-entitlements.test.ts` — **memo invalidation**: after a + credential write, the very next ensure sees it without waiting out the memo TTL. +- `tests/management-client-config-route.test.ts` — entitlement fetch count is **1**. + Red because the current count is **0**: the shared entry point never resolves + entitlements at all, which is the defect. +- `tests/native-model-toggle.test.ts` — expired confirmed roster, `/api/models` + still lists sol/terra/luna. Red **only if** the fixture supplies a usable + credential and an upstream roster; without both, it is vacuous. +- `tests/cli-export-command.test.ts` — red **only after** its stub proxy + (`tests/cli-export-command.test.ts:51-57`) is repointed at the real `/api/models` + handler and the assertion names the gated ids. The stubbed rows bypass the + defective boundary entirely, so today's green proves nothing about this defect. + +**Already green — do not claim as proof:** fresh repeated ensure yielding zero +refetches, TTL+1 collapsing to exactly one fetch, and an unconfirmed failure entry +surviving 15s. All three hold today through `:517-552`. + +**Rejection regressions (audit round 5, finding 4):** the ensure must not throw at +its boundary. One case for sidecar degrading to auth slots +(`src/sidecar/candidates.ts:35-61`) and one for client-config answering 503 +(`src/server/management/model-routes.ts:393-409`) — both must be unreachable via a +failed ensure. +## Must not change + +The expiry check (`:673`) — serving expired grants breaks fail-closed revocation. +`/v1/models` authorization or version behaviour. Per-account/version keys. +Synchronous `nativeModelRows` must stay synchronous. + +## Open question — SETTLED (audit rounds 5 and 6, `008`) + +The draft default (wait out the refresh) is wrong, and a flat per-waiter 3s +deadline is also wrong: the sequential sidecar calls at +`src/server/management/config-routes.ts:589-593` would accumulate up to 6s. + +**One whole-ensure flight**, caught fail-closed, and **never aborted by a management +timeout** — the upstream work always runs to completion so the cache is warm for the +next caller. Deduplicating the *whole* ensure matters because roster-fetch dedup +begins only after credential enumeration (`:524-552`), so abandoning a wait would +otherwise repeat the credential work. + +### The flight needs an identity, not just a timestamp (audit rounds 6 and 7) + +A bare `{ startedAt, promise }` can cross-answer: two callers with different +candidate sets, different client versions, or a credential change in between would +join a flight that is not answering their question. The existing per-entry dedup is +keyed by account, credential identity and version (`:524-526`), and the whole-ensure +flight must not be weaker than the thing it wraps. + +The mutation epoch alone is **not** sufficient (audit round 7, finding 1). External +`auth.json` writers cannot advance it — that is stated a few sections up and it is +exactly the gap: a caller that has already observed a new identity would otherwise +join an old-identity flight, which the per-entry dedup would never have allowed. + +So the flight key is: + +**(normalized candidate set, client version, mutation epoch, identity vector)** + +where the identity vector is the normalized list of +`(accountId, credentialIdentity | null)` observed by the joining caller. The +identity-only read that the cost bound already permits is what produces it, so this +costs nothing extra: the caller has the vector in hand before it decides to join. + +### The key must also carry the workset (audit round 8, final blocker) + +Candidate set, version, epoch and identities can all be unchanged while an **entry +expires mid-flight**. Concretely: a flight starts when account B is still fresh and +so refreshes only A; B then expires; a second caller computes an identical key, +joins, and the resolver's `now` is still fixed from the flight's start +(`src/codex/model-entitlements.ts:586`), so B remains a cache hit (`:517-522`). The +one-shot caller — `ocx export`, the surface #3023 reported — returns short rows +having refreshed nothing. + +So the key includes the normalized **`needsRefreshAccountIds`** workset, or joining +is permitted only when the new caller's workset is a **subset** of the running +flight's. Overlap is already deduplicated one level down by the per-account flights +(`:524-552`), so the stricter rule costs at most one extra credential pass and never +an extra roster fetch. + +Regression: B expires while an A-only ensure flight is running; the next caller must +refresh B rather than joining and reporting stale-short rows. +### Negative-memo publication is fenced on identity, not just epoch (round 7, finding 2) + +An epoch fence alone still lets an external login during a flight receive a freshly +published 5000 ms negative memo once that flight settles. Two requirements: + +1. **Fence publication on the captured identity vector.** A settling flight may + publish "no credential for X" only if X's identity is still what the flight + observed when it started. Otherwise it discards the memo and lets the next caller + re-ask. +2. **Measure expiry from the absence observation, not from settlement.** A flight + that spent 30s in a credential refresh must not then hand out a memo that is + treated as 5000 ms fresh; its evidence is already 30s old. + +Regressions, both genuinely red: a **local epoch write during an in-flight ensure**, +and an **external `auth.json` replacement during an in-flight ensure**. The second +is the one an epoch-only design silently fails. + +### Wait policy + +- Sidecar candidate paths join with a **0 ms** wait: they never stall, they read + what is already cached. +- `/api/models`, `/api/client-config`, integrations and `ocx export` wait up to + **3000 ms**, measured from the flight's original start rather than from each + waiter's arrival. + +`MODEL_ROSTER_TIMEOUT_MS` (8s) is **not** a total bound and must not be treated as +one: it starts inside the roster fetch (`:438-445`), and credential refresh can +spend 30s before that (`src/codex/main-account.ts:188-190`, +`src/codex/account-store.ts:743-746`). The 3000 ms ceiling is what the management +surfaces actually observe; the work behind it may outlive that ceiling. + +### Convergence, stated accurately (audit round 6, finding 5) + +Not "within one 5s poll cycle" — that contradicts the 30s credential refresh and 8s +roster bound above. The honest statement: a management surface converges on **the +first poll after the background flight settles**. With a warm credential that is +usually the next poll; with a cold credential refresh it can be several. The +guarantee this cycle makes is that **no poll stalls**, not that every poll is fresh. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/030_wp3_spill_publication_drain.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/030_wp3_spill_publication_drain.md new file mode 100644 index 00000000000..00b81b7898e --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/030_wp3_spill_publication_drain.md @@ -0,0 +1,189 @@ +# 030 — wp3: drain Windows spill publications at shutdown (#3011 / PR #3018) + +Independent of wp1/wp2 — different files, no catalog code. Consumes `003`. +One PABCD cycle. + +Branch: `codex/3018-shutdown-drain`, based on PR #3018's head `aec717722`. + +## Disposition + +Not "merge #3018". The PR's async publication is the right fix for the 47s +`/healthz` stall, but it creates a shutdown boundary it does not close. wp3 lands +the drain on top of the author's head, proves it, then merges the combined result. +Ingwannu's commit stays intact and credited. + +## The blocker, restated + +`responseSpillPublicationTail` is awaited only by +`flushPendingResponseSpillsForTests` (`aec717722:src/responses/state.ts:328`). +`flushResponseState()` — what shutdown actually calls +(`src/server/lifecycle.ts:492`) — awaits `persistGate` and the snapshot write, and +never the tail. + +Oversized residents are excluded from snapshots +(`aec717722:src/responses/state.ts:1015`, cap 2 MiB at `:35`), so for a payload +over 2 MiB there is a window where the snapshot skips the resident *and* the spill +stub is not installed yet. `process.exit()` on the stop paths +(`src/server/lifecycle.ts:489`, `src/server/management-api.ts:278`, +`src/cli/index.ts:365`) also skips the writer's temp cleanup +(`aec717722:src/responses/spill-store.ts:467-505`). + +Result: a lost continuation plus a possible orphaned temp. Before the PR the race +did not exist, because publication completed before the request returned. + +## Change — drain to a stable fixed point + +`src/responses/state.ts`: add a production drain and call it from +`flushResponseState()` **before** snapshot serialization. + +```ts +async function drainResponseSpillPublications(): Promise { + // A settling job can append another to the tail, so one await is not a + // fixed point: keep observing until the promise we awaited is still current. + for (;;) { + const observed = responseSpillPublicationTail; + await observed.catch(() => {}); + if (observed === responseSpillPublicationTail) return; + } +} +``` + +Ordering matters: draining *after* the snapshot keeps the bug, because the +oversized resident is skipped at serialization time. + +`flushPendingResponseSpillsForTests` should delegate to the same drain so the test +helper and production cannot diverge. + +### Bounding it (amended after audit round 1 — `004`, blocker 3) + +The draft assumed the ACL helper's timeout made the drain finite. It does not. +`defaultAsyncIcaclsRunner` sets a timer that calls `proc.kill()` and then **still +awaits `proc.exited`** (`src/lib/windows-secret-acl.ts:329-347`). Killing is not +settling: a child that ignores the kill leaves that await outstanding, so a drain +that only awaits the tail inherits an unbounded shutdown wait. + +The draft's fallback was also incoherent — it said cap expiry would "leave the +resident durably recorded", but the reason this bug exists is that residents over +2 MiB are *excluded* from the snapshot (`:1002-1018`). There is nowhere for it to +go. + +So wp3 must define **settlement**, not just waiting: + +1. Drain with an explicit wall-clock cap. +2. On expiry, take a real durable action. Falling back to the snapshot is not an + option, for exactly the 2 MiB reason. +3. `Promise.race` alone is **forbidden**: a writer that publishes after snapshot + serialization is the same lost-continuation bug wearing a timeout. + +### The fallback must share the cap (audit round 2 — `005`, blocker 3) + +Round 1 named a synchronous publication as the cap-expiry fallback without checking +its cost. Verified: the synchronous writer hardens the directory and the temp as +**separate** calls (`src/responses/spill-store.ts:324`), and each call resolves its +**own** budget — `HARDEN_DEADLINE_DEFAULT_MS = 30_000` +(`src/lib/windows-secret-acl.ts:255`, resolved per call at `:799`), with the +file's own comment documenting a ~90s timeout-path worst case at load and ~60.25s +on the owner path. + +A 5-second drain cap followed by a 60-second fallback is not a bound. So: + +1. **A budget split, not just a total** (audit round 3 — `006`). "Enter the fallback + at cap expiry" and "the fallback uses the remaining budget" cancel each other: a + drain that consumes the whole deadline leaves the fallback zero time. So reserve + the slice up front — end-to-end budget `B`, drain cap `B - R`, fallback reserve + `R`, and the fallback receives `R` rather than the remainder. `R` must cover one + directory harden plus one file harden at a reduced per-call deadline, because + those are two separate calls (`src/responses/spill-store.ts:324`). +2. The fallback **passes its reserved budget down** instead of letting each harden + call open a fresh 30s window. `OPENCODEX_ACL_TIMEOUT_MS` proves the deadline is + already parameterizable (`:799`); the plumbing is the work. +3. **Explicit ownership transfer.** When the drain abandons a job, mark the async + writer superseded so a late completion cannot publish over the fallback's + result. A late writer winning that race is the same lost continuation. + +**A synchronous stall is acceptable here specifically.** Only the graceful-shutdown +path reaches `flushResponseState()`, and no request is being served — #3011 is +about the request path. But it is acceptable *only* with an enforceable end-to-end +deadline. + +### The split condition is withdrawn (audit round 3 — `006`) + +Round 2 claimed abandonment "leaves exactly today's behaviour". That was wrong, and +the error was measuring against the wrong baseline. + +On `origin/dev`, oversized candidates are published **synchronously before the +request returns**: `admitOversizedCandidate` calls `writeResponseSpillDurably` +inline (`src/responses/state.ts:382`, `:393`). There is no shutdown-loss window on +`dev` for that case at all. This doc says as much a few paragraphs up — "before the +PR this race did not exist" — and then contradicted itself. + +Abandonment is equivalent to **PR #3018's head**, which is exactly the state that +introduced the loss. Measuring a proposed regression against an unmerged PR instead +of `dev` is how a regression gets waved through. + +So: wp3 lands the bounded fallback, or wp3 does not land. If the budget plumbing +proves larger than the drain, the correct move is to keep #3018 **unmerged** until +the drain is complete. `dev` is currently correct on durability and merely slow on +Windows. A 47-second stall is worse UX; a lost continuation is worse behaviour, and +we do not trade the second for the first. + +`src/server/lifecycle.ts` needs no change — it already calls `flushResponseState()` +at the right boundary. + +## Regressions (each driven red first) + +`tests/responses-state.test.ts` + +1. Gate ACL calls under `responses-state-spill`, queue a candidate, call + `flushResponseState()`; assert it stays pending until released. Red at the PR + head: the flush ignores the tail. +2. After release, the flush returns only once the stub is installed; then clear + memory and verify restart replay. Payload **over 2 MiB** so an early snapshot + cannot mask it. +2b. **Cap expiry enters the fallback.** Hold the ACL gate past the cap; assert the + fallback path runs rather than the flush returning with the job outstanding. +2c. **The fallback respects the remaining budget.** With the cap nearly spent, the + fallback must not open a fresh 30s harden window. Assert total elapsed stays + inside the end-to-end budget. +2d. **A late async completion after cap expiry does not overwrite.** Release the + gate after the fallback has published; assert the fallback's result stands and + the superseded writer publishes nothing. + + (Round 1's list omitted all three of these, which is what let the unbounded + fallback look acceptable.) +3. (**coverage, not red-first**) Inject an ordinary non-timeout `icacls` failure: + no stub installed, exactly one write-failure/tombstone, no owned spill or temp + left. Required-mode throws are at + `src/lib/windows-secret-acl.ts:877,881` and the state catch is at + `aec717722:src/responses/state.ts:261-266`. +4. (**coverage, not red-first**) Force link failure into the exclusive-copy + fallback, then fail destination hardening: destination removed, never returned + as a ref (`aec717722:src/responses/spill-store.ts:286-315`). + +Audit round 1 established that 3 and 4 **already pass at the PR head** — the code +is fail-closed there and cleans up. Keep them as coverage for a path no test +touches, but do not present them as red-first proof. Only 1 and 2 are that. + +## Docs + +`structure/02_config-and-codex-home.md` describes the queueing but not shutdown +ordering. Record that graceful shutdown drains publication before the snapshot +flush, and why (the 2 MiB exclusion is what makes it load-bearing). + +## Must not change + +Non-Windows synchronous branches. Required-ACL policy — failure must keep +throwing; swallowing it publishes secrets fail-open. Global serialization of the +tail (parallelizing invites generation/cleanup races). The pending-byte cap and +compare-before-swap identity check. + +## Carried follow-ups (not blockers) + +`src/server/lifecycle.ts:489` does not itself call `process.exit` — the real exits +are `src/server/management-api.ts:280` and `src/cli/index.ts:360,370`. The bypassed +temp cleanup still follows, but attribute it correctly. + +If ACL hardening fails *and* unlink fails, cleanup is best-effort and a full +payload can remain on disk; whether another local user can read it depends on the +resulting NTFS ACL. Predates this PR, needs a real Windows host to settle. File +separately rather than expanding wp3. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/040_wp4_entitlement_diagnostic.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/040_wp4_entitlement_diagnostic.md new file mode 100644 index 00000000000..9e7e4b4223a --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/040_wp4_entitlement_diagnostic.md @@ -0,0 +1,92 @@ +# 040 — wp4: an honest entitlement diagnostic + +Split out of wp2 by audit round 1 (`004`, blocker 4). Stacks on wp2. +Lowest priority in this train — it makes an existing silence visible; it does not +fix a broken behaviour. + +## The problem + +While entitled rows were missing, `GET /api/providers` still reported +`discovery: {"status":"ok"}`. The reporter read that as the proxy lying. + +It is not lying — it is answering a different question. That field is written by +routed-provider discovery (`src/codex/catalog/provider-fetch.ts:1510`, +`src/codex/model-cache.ts:94`) and entitlement resolution never touches it. Routed +discovery genuinely was ok. Nothing anywhere reports on entitlement freshness, so +the operator has no way to tell "this account owns nothing" from "we could not ask". + +## Why it is not wp2's job + +`/api/models` returns a bare array (`src/server/management/model-routes.ts:352-354`) +and both the GUI and `ocx export` depend on that shape +(`gui/src/pages/Models.tsx:402-417`, `src/cli/export-command.ts:169-185`). A +top-level field breaks both consumers; a per-row field stamps one global fact onto +every row. Neither is acceptable, so the transport is a design decision rather than +an implementation detail. + +## Direction + +`/api/providers` already carries `discovery` per provider, so an additive sibling +there is the natural home for canonical OpenAI: same endpoint, same mental model, +no shape change for array consumers. + +States to distinguish (from `002` and the wp1/wp2 work): + +- no logged-in Codex credential +- fresh confirmed roster +- `unconfirmed-empty` — upstream returned no usable rows +- refresh failed (upstream error, timeout) +- expired, refresh in flight + +> Audit round 2 (`005`) removed a state this doc originally listed: "confirmed +> roster that is genuinely empty". wp1 Change 2a makes every empty parsed roster +> **unconfirmed**, so that state is unreachable. +> +> The deeper reason it must not come back: the roster contract has no completeness +> marker, so the system genuinely cannot distinguish "this account owns nothing" +> from "upstream returned an unusable empty answer". Labelling one as the other +> would be a lie in a status field — precisely what `002` criticised +> `discovery: ok` for. `unconfirmed-empty` says only what is known. + +Do **not** overload `discovery`. It would erase a simultaneously-true routed +result and cannot express partial per-account success. GUI types admit only +provider discovery states today (`gui/src/models-groups.ts:2`), so both sides are +additive. + +## Regression + +`tests/management-provider-validation.test.ts` (~`:655`): provider discovery stays +`ok` while the entitlement status independently reports fresh / failed / +unavailable. The point of the test is that the two fields are *independent* — that +is the whole reason this phase exists. + +## Scope guard + +If this grows past an additive field, its GUI type, and one regression, stop and +re-plan. It is a diagnostic, not a subsystem. + +## Dependency + +Needs wp1 and wp2 landed first: the states above only become distinguishable once +wp1 separates unknown from denied and wp2 knows whether a refresh was attempted. + +## Prerequisite: failure provenance (audit round 3 — `006`) + +Verified blocker: parsed-empty and network/timeout failure produce the **identical** +cache entry today. The success path when `parseAccountModels` returns an empty set +(`src/codex/model-entitlements.ts:414`) and the catch path (`:424`) both yield +`{models: new Set(), confirmed: false}`. Nothing downstream can tell them apart. + +So `unconfirmed-empty` and "refresh failed" are one state in the data. Reporting +them as two would be exactly the invented-status-field lie `002` objected to in +`discovery: ok`. + +wp4 therefore requires, in order: + +1. Record provenance on the cache entry — `parsed-empty` / `http-error` / + `timeout` / `unparseable` — as a discriminated field, not a boolean. +2. A regression asserting the two states are genuinely distinct end to end. +3. Only then may the diagnostic name them separately. + +If provenance is not added, wp4 reports a single merged `unconfirmed` state and +says so plainly. An honest coarse answer beats a fabricated precise one. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/050_wp5_tristate_entitlement_authority.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/050_wp5_tristate_entitlement_authority.md new file mode 100644 index 00000000000..b89b245fe99 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/050_wp5_tristate_entitlement_authority.md @@ -0,0 +1,72 @@ +# 050 — wp5: tri-state entitlement authority + +Split out of wp1 by audit round 2 (`005`, blocker 1). Lowest priority in the train: +it hardens against a future upstream change rather than fixing a reported symptom. + +## What it is for + +#3022 happened because a roster fetched under a too-low `client_version` came back +without the gated rows, and their absence was recorded as a decided denial. wp1 +Change 1 fixes the *current* instance by asking under `0.144.0`. + +It does not make the system robust to the next bump. When upstream raises the +gated minimum to `0.148.0`, the same shape recurs: a version that was correct +yesterday silently produces confirmed negatives. wp5 removes the class. + +## Why it could not ride along in wp1 + +Two structural facts, verified in round 2: + +1. **The answering version is discarded.** `CachedAccountModels` records + `clientVersion` (`src/codex/model-entitlements.ts:230`), but + `resolveCodexModelEntitlements` drops it when building + `CodexModelEntitlementSnapshot` (`:236`, `:547-550`). The projections cannot see + it. +2. **The projections are positive-only.** `entitledCodexAccountIdsForModel` and + `availableAccountGatedNativeModels` answer "which are granted" (`:570`, `:573`). + A third boolean term either narrows redundantly or widens into granting a model + upstream never gave. "Unknown" has no slot. + +So the change is: a new snapshot field, an explicit per-model minimum source, and +three exported projections that admit only `granted` while carrying `unknown` +separately. Every caller of those three is in scope. That is a subsystem, and +smuggling it into a symptom fix is how a fail-closed gate gets accidentally opened. + +## Shape + +1. **Carry the answering version into the snapshot** — a `clientVersionByAccount` + map beside `modelsByAccount`, so a projection can ask "under what question was + this answered?". +2. **An explicit per-model minimum source.** Not the stale snapshot: sol/terra/luna + have a measured `0.144.0`, and `gpt-daybreak-blue-latest` has **no row at all** + (`src/codex/data/upstream-models.json`), so it has no minimum and must keep + omission-as-denial rather than being handed a guess. +3. **Tri-state at the boundary.** `granted` / `denied` / `unknown`. Projections keep + returning only `granted` — that is what preserves fail-closed. `unknown` exists + so it can be *reported* (wp4) and so it takes the 15s failure TTL instead of the + 5-minute success TTL, making recovery prompt. +4. **Positive evidence is never version-tested.** A returned row is a grant no + matter which version asked. Only absence needs a trustworthy question. + +## Regressions + +- A gated slug omitted from a roster fetched **below** its minimum is `unknown`: + not exposed, and not cached as a 5-minute denial. +- The same slug omitted from a roster fetched **at or above** its minimum is + `denied`: the question was capable, so absence is real evidence. +- A gated slug **present** in a roster fetched below the minimum is `granted`. + Guards requirement 4. +- `gpt-daybreak-blue-latest` keeps omission-as-denial at every version. Guards + against the predicate silently un-gating a model with no known minimum. +- No projection ever returns a slug that was not in a roster. The fail-closed + invariant, asserted directly rather than assumed. + +Use a gated slug for every one of these. Round 2 caught the earlier draft asserting +on `gpt-5.5`, which is **not** in `ACCOUNT_GATED_NATIVE_OPENAI_MODELS` +(`src/codex/catalog/native-models.ts:5-10`, ungated list at `:70`) and therefore +never at risk — a vacuous test. + +## Dependency + +After wp1 (shares `model-entitlements.ts`) and ideally after wp4, whose diagnostic +is the natural consumer of `unknown`. diff --git a/devlog/_plan/260831_prio70_entitlement_and_spill_train/070_outcome.md b/devlog/_plan/260831_prio70_entitlement_and_spill_train/070_outcome.md new file mode 100644 index 00000000000..b9e29f74e44 --- /dev/null +++ b/devlog/_plan/260831_prio70_entitlement_and_spill_train/070_outcome.md @@ -0,0 +1,109 @@ +# 070 — outcome and receipts + +Filled in as each work-phase closes. Every receipt records the command, the host it +ran on, the exit code, and pass/fail counts. Local full suites are forbidden for +this train, so suite receipts name `lidge`. + +## wp0 — roadmap (docs-only) + +- Status: closing. +- Deliverable: 12 docs — `000` plan, `001`-`003` research, `004`-`006` audit + syntheses, `010`/`020`/`030`/`040`/`050` decade docs, `070` receipts. +- Branch: `codex/prio70-train-260831` at `903243d04`. +- Research: three read-only `gpt-5.6-sol` high-effort lanes. Every load-bearing + claim was re-verified in-tree by the main session before it entered a doc. +- Audit: three adversarial `gpt-5.6-sol` rounds, all FAIL, each one amended rather + than argued with. Round 3 closed the wp1 blocker and positively traced the + reduced wp1 to a fix for #3022. + +### Receipt — wp0 (host `lidge`, Linux x86_64, bun 1.3.14) + +``` +cd ~/ocx-ci/opencodex && git checkout -B verify-prio70 origin/codex/prio70-train-260831 + -> 903243d04, dirty=0 +bun install --frozen-lockfile -> 106 installs / 145 packages, no changes +bun run privacy:scan -> exit 0, "Privacy scan passed" +bun run typecheck -> exit 0 +bun test tests/repo-hygiene.test.ts -> exit 0, 12 pass / 0 fail +``` + +No full suite was run locally, per the standing constraint. `repo-hygiene` is the +focused file that actually covers a `devlog/` change (tracked-devlog and +no-gitlink assertions), so it is the right narrow check for a docs-only phase. + +### What the audits changed + +Recording this because the diff between the first draft and the landed roadmap is +the real output of wp0: + +- **wp1 shrank.** The draft would have applied model-scoped doubt as an + account-wide denial, hiding models the account owns. Now Change 1 (measured + `0.144.0` floor) plus Change 2a (empty roster is not a confirmation) only. +- **wp3 inverted.** It began as "review and merge #3018". The audit found the PR + leaves a shutdown-loss window, so wp3 is now "land a bounded drain, then merge", + and the option to abandon an outstanding job was withdrawn once round 3 showed + `dev` publishes those candidates synchronously today. +- **Two phases were born from blockers.** wp4 (diagnostic transport) split out of + wp2; wp5 (tri-state authority) split out of wp1. +- **Three vacuous or wrong test plans were caught before implementation:** a + `gpt-5.5` assertion on a model that is not account-gated, two wp3 cases that + already pass at the PR head, and a wp4 state that cannot occur. + +## wp1 — #3022 entitlement floor + empty roster + +- Status: pending. +- Receipt: _pending_ + +## wp2 — #3023 roster TTL refresh + +- Status: pending. +- Receipt: _pending_ + +## wp3 — #3011 spill publication drain + +- Status: pending. +- Receipt: _pending_ + +## wp1 — CLOSED (shipped) + +`#3022` is fixed on `dev`. PR #3035 squash-merged as `4bdc0f6fb`. + +Two defects, one file (`src/codex/model-entitlements.ts`): the gated client-version +floor derived `0.142.2` from the bundled snapshot when upstream only returns the +gpt-5.6 rows at `0.144.0` and above, and an empty roster was recorded as a +confirmation because an empty `Set` is truthy. The floor is now composed as +`max(derived, measured, fallback)`, and a roster with no usable rows is unconfirmed +on the 15s failure TTL. + +Eight regressions, each driven red against the unfixed source. Reverting both changes +produces exactly six failures in `tests/codex-model-entitlements.test.ts` and one in +`tests/claude-models-discovery.test.ts`; restoring returns 37/37. One existing +assertion was intentionally flipped (an all-filtered roster is no longer "confirmed"), +and one existing mock was corrected — it gated at minor `>= 142`, which is precisely +why the suite never caught the regression. + +Verified on `ssh lidge` at the exact pushed head `1b6b36b96`: privacy scan passed, +typecheck clean, full suite **16510 pass / 0 fail / 16 skip**, `EXIT=0`. Repo CI green +across all four test shards, Windows, macOS, keyring, npm-global and the gates. + +## wp2 — CLOSED as a planning cycle; implementation is wp6 + +Four audit rounds, four correctness holes, all in the same place: what a deduplicated +ensure is allowed to answer for. The flight key grew from a bare timestamp to +`(candidate set, client version, mutation epoch, identity vector, workset)`, one term +per round, each added because a reviewer produced a concrete cross-answering sequence. + +Round 8's is the one worth remembering: every other term can be unchanged while an +entry expires mid-flight, so a second caller joins a flight that will never refresh +the account it came to refresh, and `ocx export` — the surface #3023 actually +reported — returns short rows having refreshed nothing. + +wp2 does not claim an implementation, because there is none. It is registered as wp6. + +## wp3 — repair in flight + +The drain itself is right: correct ordering before the 2 MiB snapshot exclusion, a +genuine stable-fixed-point loop, `B=5000`/`R=4000` with the fallback receiving its +reserved slice, and the shared ACL deadline reaching both hardeners. The review found +one high defect: supersession reaches the state tracking but not the writer, so an +abandoned writer can still publish to the filesystem and orphan a temp. Sent back. diff --git a/docs-site/src/content/docs/fr/reference/adapters.md b/docs-site/src/content/docs/fr/reference/adapters.md index b98394708ff..2b33a26e2e5 100644 --- a/docs-site/src/content/docs/fr/reference/adapters.md +++ b/docs-site/src/content/docs/fr/reference/adapters.md @@ -125,7 +125,7 @@ Le texte de l’assistant Kiro ne comporte pas, à lui seul, de phase fiable ind `END_TURN`, `STOP_SEQUENCE` ou l’absence de motif d’arrêt peuvent emprunter le chemin de compatibilité. Les autres motifs explicites ont déjà interrompu l’inférence en amont ; l’adaptateur les signale donc au lieu de consommer une nouvelle requête au modèle. Une limite de jetons de sortie est présentée comme une sortie incomplète que le client peut poursuivre, tandis qu’un épuisement de la fenêtre de contexte devient une erreur de longueur de contexte non renouvelable plutôt qu’une sortie tronquée. Les arrêts dus au filtrage et aux garde-fous sont présentés comme des sorties filtrées incomplètes. Un arrêt `TOOL_USE` sans appel d’outil réel est signalé comme une contradiction, et non comme une progression. -Lorsqu’un outil client ordinaire est disponible, opencodex ajoute un outil privé `codex_kiro_final_answer` à la requête en amont. Le texte de progression est diffusé comme commentaire et ne peut pas clore le tour. L’adaptateur consomme l’appel privé, émet sa réponse comme texte final et n’expose jamais cet outil privé à Codex ou Claude Code. Le motif d’arrêt n’arrivant qu’à la fin du flux, le texte de l’assistant est retenu pendant un tour avec outils jusqu’au début d’un véritable appel d’outil ou jusqu’à la fin du flux. Il est alors libéré comme commentaire, sauf si l’outil privé a fourni la réponse finale. Lorsque le service auxiliaire de recherche Web est actif, les commentaires libérés continuent d’être diffusés avant l’événement terminal ; seuls les événements nécessaires pour déterminer si le modèle a demandé une recherche synthétique restent en mémoire tampon. +Lorsqu’un outil client ordinaire est disponible, opencodex ajoute un outil privé `codex_kiro_final_answer` à la requête en amont. Le texte de progression est diffusé comme commentaire et ne peut pas clore le tour. L’adaptateur consomme l’appel privé, émet sa réponse comme texte final et n’expose jamais cet outil privé à Codex ou Claude Code. Le motif d’arrêt n’arrivant qu’à la fin du flux, le texte de l’assistant est retenu pendant un tour avec outils jusqu’au début d’un véritable appel d’outil ou jusqu’à la fin du flux. Il est alors libéré comme commentaire, sauf si l’outil privé a fourni la réponse finale. Lorsque le service auxiliaire de recherche Web est actif, les commentaires libérés continuent d’être diffusés avant l’événement terminal ; seuls les événements nécessaires pour déterminer si le modèle a demandé une recherche synthétique restent en mémoire tampon. Lorsque le modèle ne peut pas continuer sans une décision, une information ou une clarification que seul l’utilisateur peut fournir, le contrat lui demande de transmettre cette question via l’outil d’achèvement et de s’arrêter ; un tel tour arrive lui aussi comme `final_answer` avec le tour clos, et non comme un commentaire. Si Kiro s’arrête sans appeler l’outil d’achèvement, l’adaptateur effectue une continuation. Les nouvelles tentatives ne contenant que du raisonnement conservent le tour utilisateur/résultat d’outil valide d’origine au lieu de fabriquer un message assistant vide ; la progression visible est rejouée avec une instruction non vide appartenant à l’adaptateur. Avant le transport, la conversation générée est contrôlée afin de vérifier l’alternance des rôles, l’absence de tours structurels vides et la correspondance des identifiants d’utilisation et de résultat d’outil. Une sortie d’outil vide reçoit un texte de remplacement neutre et non vide. La nouvelle tentative ne peut pas se répéter récursivement : si elle est vide ou ne contient que du raisonnement, elle est renvoyée comme incomplète mais renouvelable ; un véritable appel d’outil client maintient le tour ouvert. Une réponse de l’outil d’achèvement est toujours émise comme `final_answer`, même si elle répète exactement un commentaire antérieur, car l’exactitude de la phase prime sur la déduplication esthétique. Les requêtes sans outil conservent le comportement normal d’achèvement textuel. diff --git a/docs-site/src/content/docs/ja/reference/adapters.md b/docs-site/src/content/docs/ja/reference/adapters.md index 5e9427de599..0fea01bc2bf 100644 --- a/docs-site/src/content/docs/ja/reference/adapters.md +++ b/docs-site/src/content/docs/ja/reference/adapters.md @@ -150,6 +150,7 @@ filtered incomplete になります。実際のツール呼び出しを伴わな ツール有効ターンでは非公開の `codex_kiro_final_answer` を追加します。再試行は空の assistant/user ターンを 生成せず、元の user/tool-result を保持し、送信前にロール交互性、空の構造メッセージ、tool use/result の対応を検証します。 完了ツールの回答は以前の commentary と同じでも `final_answer` として送出します。 +ユーザーしか出せない判断・情報・確認が得られず先に進めない場合も、その質問を完了ツールで送って停止するよう契約が指示します。これも commentary ではなくターンを終えた `final_answer` として届きます。 ### Reasoning effort diff --git a/docs-site/src/content/docs/ko/reference/adapters.md b/docs-site/src/content/docs/ko/reference/adapters.md index dd81eddb41d..c086eb4c110 100644 --- a/docs-site/src/content/docs/ko/reference/adapters.md +++ b/docs-site/src/content/docs/ko/reference/adapters.md @@ -163,6 +163,7 @@ commentary로 유지하고 비공개 완료 툴을 한 번 검증합니다. 툴이 있는 턴에는 비공개 `codex_kiro_final_answer`를 추가합니다. 완료 재시도는 빈 assistant/user 턴을 만들지 않고 원래 user/tool-result를 보존하며, 전송 전에 역할 교대·빈 구조 메시지·tool use/result 짝을 검증합니다. 완료 툴 답변은 이전 commentary와 같더라도 `final_answer`로 내보냅니다. +사용자만 줄 수 있는 결정·정보·설명이 없어 더 진행할 수 없을 때도 그 질문을 완료 툴로 보내고 멈추도록 계약이 지시합니다. 이때도 commentary가 아니라 턴이 끝난 `final_answer`로 도착합니다. ### Reasoning effort diff --git a/docs-site/src/content/docs/reference/adapters.md b/docs-site/src/content/docs/reference/adapters.md index dd566be5be0..3709bcf4519 100644 --- a/docs-site/src/content/docs/reference/adapters.md +++ b/docs-site/src/content/docs/reference/adapters.md @@ -255,6 +255,15 @@ answer. When the web-search sidecar is active, released commentary still streams ahead of the terminal event; only the events needed to decide whether the model requested a synthetic search remain buffered. +A question the model cannot proceed without is also a final answer. The injected contract tells a +routed model that when it needs a decision, a piece of information, or a clarification only the user +can give, it should deliver that question through `codex_kiro_final_answer` and stop, rather than +writing the question as ordinary text and continuing. Such a turn arrives like any other completed +answer: final text with the turn ended, not commentary and not a client tool call. Without this, +the contract described only "still working" and "fully complete", and a model holding a blocking +question had no way to say so — the observed result was a question and a self-override emitted as one +message, followed by another tool call from the same inference. + If Kiro stops without calling the completion tool, the adapter makes one continuation. Reasoning- only retries preserve the original valid user/tool-result turn rather than manufacturing an empty assistant message; visible progress is replayed with a non-empty adapter-owned instruction. Before diff --git a/docs-site/src/content/docs/ru/reference/adapters.md b/docs-site/src/content/docs/ru/reference/adapters.md index ecff865ffc1..548192f3573 100644 --- a/docs-site/src/content/docs/ru/reference/adapters.md +++ b/docs-site/src/content/docs/ru/reference/adapters.md @@ -175,6 +175,7 @@ incomplete. `TOOL_USE` без фактического вызова инстру создаёт пустые assistant/user-сообщения, сохраняет исходный user/tool-result и перед отправкой проверяет чередование ролей, непустые структурные сообщения и пары tool use/result. Ответ инструмента завершения всегда выдаётся как `final_answer`, даже если он совпадает с предыдущим commentary. +Если продолжить нельзя без решения, сведений или уточнения, которые может дать только пользователь, контракт предписывает отправить этот вопрос через инструмент завершения и остановиться. Такой ход тоже приходит как `final_answer` с завершённым ходом, а не как commentary. ### Reasoning effort diff --git a/docs-site/src/content/docs/tr/reference/adapters.md b/docs-site/src/content/docs/tr/reference/adapters.md index b872a455d28..8d4e65e696f 100644 --- a/docs-site/src/content/docs/tr/reference/adapters.md +++ b/docs-site/src/content/docs/tr/reference/adapters.md @@ -248,6 +248,11 @@ sidecar'ı etkinken serbest bırakılan yorum terminal olayından önce yine de yalnızca modelin sentetik bir arama talep edip etmediğine karar vermek için gereken olaylar arabelleğe alınmış olarak kalır. +Yalnızca kullanıcının verebileceği bir karar, bilgi ya da açıklama olmadan devam +edilemiyorsa, sözleşme bu soruyu tamamlama aracıyla gönderip durmayı söyler. Böyle +bir tur da yorum ya da istemci araç çağrısı değil, turu bitiren `final_answer` +olarak ulaşır. + Kiro tamamlama aracını çağırmadan durursa adaptör bir devam işlemi yapar. Yalnızca akıl yürütme yeniden denemeleri boş bir asistan mesajı üretmek yerine orijinal geçerli kullanıcı/araç sonucu turunu korur; görünür ilerleme boş @@ -324,4 +329,3 @@ Vizyon duyarlı adaptörler tarafından kullanılan paylaşılan yardımcılar: parçalarını metne düzleştirir (açıklanmayan bir görsel kısa bir `[image]` işaretçisi haline gelir, asla belirteç patlatan bir base64 bloğu olmaz). - diff --git a/docs-site/src/content/docs/zh-cn/reference/adapters.md b/docs-site/src/content/docs/zh-cn/reference/adapters.md index 046488f9e39..61fab20c1fc 100644 --- a/docs-site/src/content/docs/zh-cn/reference/adapters.md +++ b/docs-site/src/content/docs/zh-cn/reference/adapters.md @@ -150,6 +150,7 @@ Kiro 的 assistant 文本本身没有可靠的回合结束标记，但终止的 启用工具时，opencodex 会添加私有 `codex_kiro_final_answer`。重试不会制造空的 assistant/user 回合， 而会保留原始 user/tool-result，并在发送前校验角色交替、非空结构消息以及 tool use/result 配对。 完成工具的回答即使与先前 commentary 完全相同，也会作为 `final_answer` 发出。 +当缺少只有用户能提供的决定、信息或澄清而无法继续时，契约同样要求把该问题通过完成工具发出并停止；它也作为结束回合的 `final_answer` 到达，而不是 commentary。 ### Reasoning effort diff --git a/docs-site/src/content/docs/zh-tw/reference/adapters.md b/docs-site/src/content/docs/zh-tw/reference/adapters.md index c92be2b73d1..c74a22304a7 100644 --- a/docs-site/src/content/docs/zh-tw/reference/adapters.md +++ b/docs-site/src/content/docs/zh-tw/reference/adapters.md @@ -140,6 +140,9 @@ Kiro 的 assistant 文字本身沒有可靠的回合結束標記，但終止的 重複抑制嚴格限定為空白歸一化後的完全一致：改寫過的狀態更新可能改變本回合的結果（從"仍在進行"變成"已完成"）， 丟掉那句話比顯示一次表面重複更糟糕。 +當缺少只有使用者能提供的決定、資訊或說明而無法繼續時，契約要求把該問題透過完成工具送出並停止； +這樣的回合同樣以結束回合的 `final_answer` 抵達，而不是 commentary 或用戶端工具呼叫。 + ### Reasoning effort `gpt-5.6-sol` 和 `claude-opus-5` 支援原生 effort，且請求欄位名不同。`low` / `medium` / `high` / diff --git a/gui/src/pages/dashboard-overview-sections.tsx b/gui/src/pages/dashboard-overview-sections.tsx index 971cc3a64ff..7bdf2829315 100644 --- a/gui/src/pages/dashboard-overview-sections.tsx +++ b/gui/src/pages/dashboard-overview-sections.tsx @@ -511,19 +511,25 @@ export function DashboardSidecarPanels({ d }: { d: Dash }) {
{t("dash.webSearchSidecar")}
{t("dash.webSearchSidecarHint")}
+ {/* Same two-row shape as the vision card: the model select owns the first row, + and the secondary control sits right-aligned on its own row below. Sharing the + structure is what keeps the two cards' first rows on one line — the streaming + label used to sit beside the select and wrap to three lines in ko/ja/tr. */}
- { + void saveSidecar({ webSearch: webSearchSidecarSelectionForModel(models, sidecarModels, model) }); + }} + disabled={!sidecar || sidecarSaving} + label={t("dash.sidecarModel")} + align="right" + /> +
+
+ {t("dash.webSearchStream")}
-
+