docs(devlog): close out the priority-65 unit - #3485
Closed
lidge-jun wants to merge 17 commits into
Closed
Conversation
Eight work-phases, each one PABCD cycle consuming one decade doc. Two adversarial audit rounds; the first found that the planned schema guard would have broken the passthrough fail-closed contract, and re-running all three candidate designs against 63 files proved it (schema guard: +1 regression; translation-path guard: none).
…ifier table The plan claimed 55 of the 63 candidate files reach the guard. Measured: 18. The other 45 are adapter/parser unit tests that never enter handleResponses, so V5 proves exhaustive candidate collection rather than broad guard execution.
privacy:scan reads devlog/, so a Co-authored-by trailer spelled out in a plan document fails CI. The trailer still ships in the commit; the plan now names the gh query that resolves the address at implementation time.
PR #3461 is a fork PR whose head has only the four gate checks; Cross-platform CI never ran and fork workflows wait on approval. Merging on gate-green alone would violate the unit's own exact-head-CI criterion.
A provider can report its own target hard cap with a non-semantic vendor code (5059 + invalid_request_prompt_too_long). classifyError remaps any "maximum context" text to context_length_exceeded, which the stop list catches, so the chain ended at the first target even when a larger-context target was still queued behind it. The matcher is deliberately narrow: status 400 AND (the type string OR code 5059 together with the "Prompt N > M maximum context length" shape). A bare 5059 still stops, as do a generic 400 context refusal and a generic 413. Carried from #3461 because that fork PR's head only ever ran the four gate checks -- Cross-platform CI never ran on it, and fork workflows wait on approval, so gate-green was not merge evidence. Co-authored-by: RHODIZ IT <info.rhodiz@gmail.com>
A tool result is paired by call_id, but inputItemSchema's permissive catch-all (schema.ts:106) accepts a tool item whose strict alternative failed only for a missing call_id, and parser.ts:738/752 then assigns it unchecked. A translating adapter consumes `toolCallId: string` holding undefined: kiro-wire.ts:32 TypeErrors, ollama-native.ts:334 throws, and anthropic.ts:775 sends "[tool_result without adjacent tool_use: undefined]" upstream. Guard in handleResponsesInner after the passthrough branch, keyed on the adapter rather than on position. The check cannot live in the schema: parseRequest runs before the passthrough branch, so a parse-time rejection would also kill forward/key passthrough and routed compaction - paths that build from _rawBody, never read context.messages, and already degrade an unpaired output to "[tool output for unknown call]" on their own. routedCompaction skips the passthrough branch yet is still _rawBody-based, which is why the condition tests the adapter instead of the code position. Closes #3259
…budget Reimplementation of #3332, which could not be cherry-picked: the PR is CONFLICTING/DIRTY because dev added modelReasoningEfforts to both Anthropic registry entries after it was written. Thin Claude discovery rows carry only id + context window, so the combo intersection collapsed to text-only with no effort ladder and the Codex app hid image attachments and the effort picker for every Claude combo. Fall back to the generated vendor table when the caller supplies no fallback, tolerating point releases by trimming to the family row (claude-fable-5-1 -> claude-fable-5). Codex never sends max_output_tokens, so the Anthropic adapter's omitted-limit default of 8192 truncated long answers with stop_reason=max_tokens. Honor the provider's configured budget and register 64k for both Anthropic entries. One line is deliberately changed from the original PR. It mapped the vendor metadata.maxTokens OUTPUT ceiling onto maxInputTokens; that value is read by the combo intersection's Math.min over member input ceilings, collapsing a 1M Claude combo window to 128k and its autoCompactTokenLimit from 900k to 128k (measured). It fills maxOutputTokens here instead. The original test used toMatchObject on contextWindow only and could not see the defect, so a dedicated regression asserts the input window and autoCompact budget survive. Verification: bun run typecheck, bun test tests/codex-catalog.test.ts (268 pass), bun test tests/anthropic-reasoning.test.ts (67 pass) - all exit 0. Co-authored-by: full999 <daiki.furutani@walker-s.co.jp>
…ntity The durable ledger for manual reset-credit operations was complete and had no production caller: openManualResetCreditOperation, settleManualResetCreditOperation, and markManualResetCreditOperationAmbiguous were referenced only by their own test file. Meanwhile the consume endpoint minted a fresh crypto.randomUUID() per call and sent it as redeem_request_id, so a retry of the same logical redeem looked like a new one to upstream. Spending a reset credit is irreversible, which is the case where idempotency has to be the caller's to assert. An optional operationId in the request body now opens a ledger row keyed by the physical ChatGPT account, and the canonical id becomes the redeem_request_id. Opening fails closed: capacity and unavailable return 503 rather than falling back to a random id, because that fallback is exactly the double-spend the identity exists to prevent. A row that is already terminal replays its recorded code instead of trusting upstream idempotency, and an id owned by another account returns 409. Settling fails open. By then the credit is already spent, so reporting a ledger failure to the user would invite a manual retry -- the double-spend again, from the other direction. Dispatch errors and non-2xx responses mark the row ambiguous so a later replay is never mistaken for a new operation. Omitting operationId keeps today's behavior exactly, including the random id, so no existing caller changes. The CLI gains --operation-id; the GUI is unchanged, since guessing a reuse window there could swallow a genuinely intended second redeem.
…comment Carried from #3327. Two gaps the original #3198 tests left open: the uncalibrated-plan notice was never pinned independently of the incomplete-coverage gate, so folding it under that branch would have passed every existing fixture while silently hiding it; and the malformed-plan path was described as if it reached aggregation the same way an unlisted plan name does, when poolAccountDto strips it earlier via codexPlanValue and the aggregate sees an absent plan instead. One assertion is narrowed from the original. over the whole envelope also matches any unrelated field whose name contains that substring -- serviceTier, tierOutcome -- so it would fail on changes with nothing to do with plan leakage. Scoped to the report rows and to the quoted key, which is where the malformed value could actually surface. Carried rather than merged in place: #3327 is a fork PR, and Cross-platform CI never ran on its head. enforce-target was also red there because touching gui/tests/ trips the UI-screenshot gate on a test-only change. Co-authored-by: olddonkey <olddonkeyblog@gmail.com>
Carried from #3251 (both commits, in order). The backend already computed `tierOutcome` and shipped it to the GUI on every log entry via requestLogEntryFromPersistedUsage, and the GUI consumed it nowhere -- `rg tierOutcome gui/src/` returned zero hits before this change. So a bare `responseTier=default` read as a denial even when the turn had in fact been scheduled as priority. The tooltip now qualifies the echoed tier with its confirmation: responseTier=default (assumed) responseTier=default (downgraded: response-declined) responseTier=priority (confirmed) Deliberately not turning `assumed` into `confirmed` for the ChatGPT-internal Codex backend. That backend answers `service_tier: "default"` on turns it scheduled as priority, and reading the echo as authoritative is what #2558 was. The point is to show the uncertainty rather than to paper over it. Carried rather than merged in place: #3251 is a fork PR whose head never ran Cross-platform CI, and its enforce-target failure is the UI-screenshot gate. Co-authored-by: Abhishek Sharma <abhicse24@gmail.com>
An expired rollback row was a dead entry: its snapshot bytes were gone, so restore was refused, and nothing could remove it. The journal route was GET-only and the list offered no delete affordance anywhere. Deletion is expressed as an APPEND. Rewriting journal.jsonl without the row would break all three things the file header promises -- appendOperation commits and nothing else, so a read-modify-write races concurrent appends; a torn rewrite truncates the whole log rather than the one trailing line listOperations tolerates; and no lock covers this file, because append-only never needed one. A tombstone record retires an opId instead, and listOperations collects tombstones in the same pass before any client filtering, then filters after the whole file is read. Both orderings matter: a tombstone carries an opId and no clientId, so filtering by client first would resurrect the row on the per-client route while the global route hid it, and an in-loop check would miss every tombstone because it is always appended after the row it retires. The newest row per client is refused with 409 by the SERVER, re-read immediately before the write rather than trusted from the rendered list. It is the undo entry point, and an admin-token caller has no GUI to hide a button in. The GUI mirrors that with a server-computed deletable field so the rule does not exist in two places. Snapshot bytes are pruned AFTER the tombstone commits, so a failed cleanup leaves the row retired and the leftover disclosed through retentionDegraded, rather than destroying a backup for a deletion that then failed to record. Documented fallout: pruneSnapshots builds its keep set from listOperations, so a retired row no longer occupies a retention slot. Snapshots survive longer, never disappear early, but "ten backups per client" now counts live rows. The DELETE route carries a deferred-verb exemption; the CLI verb is owed by a later work-phase. Confirmation reuses the existing ConsequenceDialog, and the 409/404 codes are registered in refusal-copy so the server English never reaches a non-English locale. Design of record: devlog/_plan/260904_priority65_closeout/060_wp7_rollback_journal_crud.md Refs #3379
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
documentation
Contributor
⏳ DRAFT
What to do
This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (64)
📝 WalkthroughWalkthroughThe pull request combines several closeout work packages. It adds tool-result boundary validation, combo metadata and failover handling, reset-credit idempotency, rollback journal deletion, model-tier display changes, localized documentation, tests, and development outcome records. ChangesResponses boundary
Combo metadata and failover
Reset-credit operation identity
Rollback journal deletion
Tier display and closeout records
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)Reset-credit operation flowsequenceDiagram
participant CLI
participant ResetCreditAPI
participant ResetCreditLedger
participant UpstreamCreditService
CLI->>ResetCreditAPI: consume with optional operationId
ResetCreditAPI->>ResetCreditLedger: open operation for account identity
ResetCreditLedger-->>ResetCreditAPI: execution state
ResetCreditAPI->>UpstreamCreditService: redeem canonical operationId
UpstreamCreditService-->>ResetCreditAPI: consume result
ResetCreditAPI->>ResetCreditLedger: settle or mark ambiguous
ResetCreditAPI-->>CLI: replay, success, or mapped error
Rollback journal deletion flowsequenceDiagram
participant RollbackHistory
participant IntegrationRoutes
participant IntegrationStateStore
participant Journal
RollbackHistory->>IntegrationRoutes: DELETE journal entry
IntegrationRoutes->>IntegrationStateStore: verify operation and newest-row rule
IntegrationRoutes->>IntegrationStateStore: retire operation
IntegrationStateStore->>Journal: append tombstone
IntegrationRoutes->>IntegrationStateStore: prune snapshot
IntegrationRoutes-->>RollbackHistory: deletion result or refusal
Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Ingwannu
requested changes
Sep 4, 2026
Ingwannu
left a comment
Ingwannu
left a comment
Owner
There was a problem hiding this comment.
The closeout record is useful, but the current exact head 3656bb74ff173a4c2e450bfd001c86c481f61bb2 is not documentation-only relative to the current dev base.
GitHub currently reports 64 changed files, 5,941 additions, and runtime/GUI/test changes. The branch forked at 2421e44ce; it is 17 commits ahead and 13 commits behind current dev (066146980). Its history carries unsquashed implementations of work that already landed on dev through different squash commits, so merging this head would replay or conflict with those implementations despite the PR description stating that no src/, gui/, or tests/ files change.
Please rebuild the closeout as a docs-only branch from the latest dev (for example, cherry-pick only the intended closeout documentation commits or create one clean documentation commit), then confirm that the GitHub changed-file list contains only the intended devlog/ records. After the head changes, exact-head CI can establish the final result.
Owner
Author
|
Superseded — the branch still carried commits already squashed onto |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3656bb74ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1
to
+3
| # 080 — 결과 기록 | ||
|
|
||
| base `2421e44ce` → 최종 `origin/dev` `413227888`. |
There was a problem hiding this comment.
_fin
This outcome records the landed SHAs and final regression result, so the unit has reached a terminal state. Leaving the entire unit under devlog/_plan makes the repository's open-work inventory falsely report completed work as still active; move devlog/_plan/260904_priority65_closeout to the corresponding _fin path.
AGENTS.md reference: AGENTS.md:L83-L86
Useful? React with 👍 / 👎.
Comment on lines
1393
to
+1396
| liveModels: true, | ||
| modelContextWindows: { ...ANTHROPIC_MODEL_CONTEXT_WINDOWS }, | ||
| modelReasoningEfforts: { ...ANTHROPIC_MODEL_REASONING_EFFORTS }, | ||
| defaultMaxOutputTokens: ANTHROPIC_DEFAULT_MAX_OUTPUT_TOKENS, |
There was a problem hiding this comment.
For the API-key provider, liveModels: true permits models outside ANTHROPIC_MODELS, but this provider-wide default is then sent by the adapter for every discovered model lacking a configured override. The repository metadata still supports models with lower output ceilings, such as claude-opus-4-0 at 32k and claude-3-haiku-20240307 at 4096, so an omitted caller limit becomes max_tokens: 64000 and those otherwise valid requests are rejected upstream. Populate canonical per-model limits or consult the derived model metadata before using the 64k fallback.
AGENTS.md reference: src/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
Comment on lines
+672
to
+675
| try { | ||
| await deleteJournalEntry(apiBase, deleting.opId); | ||
| } catch (error) { | ||
| /* |
There was a problem hiding this comment.
When another tab has already deleted the row, this request receives the documented 404; similarly, a concurrently changed history can return integration_journal_newest_protected. The catch rethrows before historyResource.refresh() runs, so the dialog remains open over a stale row and every retry repeats the same refusal, even though the localized 404 message says the list will refresh. Refresh the journal, and close or reconcile the dialog, for these stale-state responses.
AGENTS.md reference: gui/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes out the priority-65 unit by recording what each work-phase actually produced. Documentation only — no
src/,gui/, ortests/changes.Three PRs landed from this unit:
4968d0f2600834d710413227888What these documents record
The outcome files keep the parts that would otherwise be lost, which are mostly the places where a plan turned out to be wrong:
deletablefield; the key list was extended rather than the assertion loosened.Regression proof
2421e44cetests/codex-auth-context.test.ts:1461413227888devwas already red when this work started, which is why the baseline was captured first — otherwise "did I break it" is unanswerable. All three squash SHAs verified as ancestors of the finalorigin/dev.Also recorded: macOS failed twice during this unit and passed on rerun both times, on
tests/codex-shim.test.tsandtests/lab-fabric-task.test.ts. Neither file is in any of these diffs, and both pass locally (77/77 and 49/49). Two timing-sensitive tests are intermittently unstable on the macOS runner; that is worth its own look.Verification
bun run typecheckexit 0,bun run privacy:scanpassed. The full local suite was not run at any point in this unit.Checklist
bun x tsc --noEmitcleanbun run privacy:scancleanSummary by CodeRabbit