test(catalog): pin the routed reasoning joint contract (#1100) + bug-grade triage unit - #1119
test(catalog): pin the routed reasoning joint contract (#1100) + bug-grade triage unit#1119lidge-jun wants to merge 11 commits into
Conversation
…tems Phase-0 docs-only cycle for the loop that turns the 2026-08-06 bug triage into landed remediation. Twelve issues and thirteen PRs carry the `bug` label; none is a false report, so the roadmap is organized by whether the proposed FIX is sound rather than by whether the defect is real. The load-bearing finding is in 010. A routed catalog row advertises a full effort ladder while `supports_reasoning_summaries` is stripped (catalog/parsing.ts:353) and defaulted false (:266), which is #1100. But the per-model opt-in the strip's own comment says "can be added" already exists and already survives, because applyCatalogModelMetadata runs at catalog/sync.ts:269, after the strip at :267. So the defect is narrower and more honest than "we strip the flag": the ordering works and nothing asserts it stays that way. wp1 buys that joint contract and deliberately does not flip the routed default to true — advertising OpenAI-only summary delivery for arbitrary providers is the same overclaim that blocks #1092 and #978. Two adversarial audit rounds preceded this commit. Round 1 caught the planned regression test as vacuous: it called buildCatalogEntries(null, ...), which takes the fallback branch (sync.ts:291-310) and never runs the strip, so the ablation could not have failed. The amended plan drives the template path via nativeTemplate() and keeps a third test pinning the fallback asymmetry. The ablation was then proven empirically rather than argued: reordering the two calls flipped the template row true->false while the fallback row stayed true, and restore left an empty diff. Round 1 also caught the objective promising an outcome for every labelled item while only one issue had scheduled work; wp6 (060) now gives each remaining G1 issue a disposition and an unblocking condition, and the objective says plainly that this loop authors code for exactly one defect. Round 1's third finding corrected a misdiagnosis of our own making: #1068's failing gate is not a verdict on its diff. The workflow requires parseGateState (enforce-pr-target.yml:63), dev exports it (pr-quality-state.cjs:84,380), and an in-tree regression test names this exact crash as the head-YAML/base-script mismatch (enforce-pr-target.test.cjs:51-60) — the same root cause as #978's stale checkout. Blaming a contributor for a gate artifact would have been wrong. No src/ or tests/ changes in this commit; implementation starts at wp1.
A routed catalog row advertises a full effort ladder while `supports_reasoning_summaries` is stripped (catalog/parsing.ts:353) and defaulted false (:266). Codex gates construction of the entire Responses `reasoning` object on that flag, so Desktop renders an effort picker whose selection never reaches the wire. That is #1100. The escape hatch already exists: `modelSupportsReasoningSummaries` resolves at catalog/provider-fetch.ts:545 and survives the strip, but only because `applyCatalogModelMetadata` runs at catalog/sync.ts:269, two lines after `normalizeRoutedCatalogEntry` at :267. Nothing asserted that ordering, so a future reorder would silently disable effort propagation for every opted-in routed provider with a green suite. Three tests, and the choice of construction path is the whole point. The two existing summary tests (:2350, :2387) call `buildCatalogEntries(null, ...)`, which takes the fallback branch (sync.ts:291-310) and never runs the strip — so they keep passing under the broken order. A plan audit caught the first draft of this commit making exactly that mistake. The opt-in and conservative tests now pass `nativeTemplate()` to reach the template branch, and a third test keeps the `null` path deliberately, pinning the asymmetry between the two so a future unification is a visible change rather than a silent one. Red ablation, run rather than asserted. Moving the strip after the metadata restore produces: Expected: true / Received: false (fail) routed strip does not defeat an explicit reasoning-summary opt-in (pass) routed rows without an opt-in stay conservative about summaries (pass) the no-template fallback never applies the routed summary strip Only the template test goes red; the fallback staying green is what proves the template test carries the contract. Restore leaves an empty src/ diff. No src/ change: this does not fix #1100 for a reporter whose provider has no opt-in configured, and it deliberately does not flip the routed default to true — claiming OpenAI-only summary delivery for arbitrary providers is the same overclaim that blocks #1092 and #978. It pins what we have and makes the supported workaround load-bearing. tests/codex-catalog.test.ts: 119 -> 122 pass, 0 fail. typecheck clean.
Two reviews posted. They ended up in different places than the roadmap predicted, and the correction is the point of this commit. #1092 confirmed as planned. `src/combos/request.ts:43` fails closed on dev for both an unknown and an unsupported ladder, and the debug record carries `capability: unknown | unsupported` — someone deliberately made those two states separable. The PR skips the guard when the ladder is `undefined` and hardcodes the field to `"unsupported"`, so we start sending an effort to targets we know nothing about and lose the one signal that would tell us how often that happens. Review asks for the picker fix to land as written while `undefined` stays fail-closed. #978 graded wrong in the roadmap, corrected before anything was posted. It was filed as a capability-gate reversal on the strength of its title. The diff is narrower: `thinkingEligible` excludes cloud-code-assist, excludes image models so `responseModalities` still fires, keeps the Vertex freeze absent an explicit ladder, and retains the hardcoded flash slice. Its ladder source, `configuredReasoningEfforts` (src/reasoning-effort.ts:58-64), returns a value only from a registry preset or explicit user config — never a default. Probing a custom google-adapter provider on both trees: dev sends nothing in all three configs; the PR sends `thinkingLevel` only where the user asserted a ladder. That is opt-in, which is the same shape wp1 called the legitimate remedy for #1100, so blocking it would contradict our own position. Review is non-blocking with one docs ask: configuring `modelReasoningEfforts` now changes wire behavior, and that is not guessable from the field name. Both PRs remain open and unmerged. Comments 5202651774 and 5202655589.
… guards Both hazard claims re-derived from the real diffs rather than carried on the roadmap grading, after wp2 showed a grading could be wrong. Both held. #1036: `translateStructuredEditCall` returns early only on `isCursorStructuredEditToolName(toolName)`, and both call sites pass a wire-supplied name — `responsesName` in `mapSyntheticMcpExecToToolEvents`, `open.name` in the completion path. No request-scoped check asks whether the synthetic tools were advertised at all. The diff even adds a comment describing exactly that flag, so the intent exists and only the enforcement is missing. A user running an MCP server that exposes `edit_file` either has their call silently re-emitted as `apply_patch`, or gets a drop error naming a conversion they never requested. Every new test drives our own synthetic caller, so the uncovered case is the one that can actually go wrong. #1068: the `noVisionModels` line is replaced, not extended, and set arithmetic on the real constants gives the cost precisely — big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, north-mini-code-free and laguna-s-2.1-free stop being marked text-only, reopening #1043. `deepseek-v4-flash-free` sits in both sets, which is almost certainly why the replacement looked safe. The fix is a union, so the review says that rather than asking for a rewrite. Also flagged: the branch predates the per-model DeepSeek ladders that shipped for #1057 in v2.10.2. The CI-framing correction from wp2 was applied to both comments: the `enforce-target` failure is a repository-side gate artifact, and the #1068 comment retracts the earlier "substantive" framing to the author directly rather than quietly dropping it. Comments 5202680370 and 5202684051; both PRs remain open and unmerged.
…proof Three dispositions, and the harshest one is on my own PR. #557 says the full suite passes. It does not. Run 30748759350 job 91498912876 ends `2 fail / Ran 7008 tests across 482 files`, and both failures resolve to the same line: 175 | spawnStart: () => { throw new Error("must not spawn"); }, error: must not spawn at spawnStart (tests\update-job.test.ts:175:37) The preflight's restart-flow change reaches `spawnStart` on a path whose test asserts nothing may restart the proxy. That is a real question about which side is correct, not re-runnable noise. This doc also corrects itself: an earlier draft claimed the ubuntu failure was a Bun segfault and macOS was cancelled — all three legs are red, so the comment asks for separate triage rather than attributing them. That is the second roadmap claim overturned by direct verification in this loop, after #978, which is why every claim is now re-derived before it reaches a contributor. The PR also skips the preflight entirely on Windows, with a test pinning the skip, so the platform where npm cache permission failures are most common is the one platform uncovered — while the same PR breaks two Windows tests. Being the author is not an exemption from the second-maintainer security review `MAINTAINERS.md` requires for dependency-installation changes. #1093: `INGRESS_SPAN_RE` is a bare format check applied to a raw client header, so the description's "guard-issued" framing is unsupported — a regex is not provenance. Asked for three things: who issues the span and why a client value may be persisted, what happens for requests that fail before adapter resolution, and a corrected contract claim, since the diff adds fields to persisted usage and `/api/logs` while saying it changes no public contract. #997: the isolation is complete for the file, including the delete-when- previously-unset branch. But all nine assertions test metrics, so the safety property the PR exists to establish is the one thing the suite would not notice losing. Asked for a single assertion pinning the real home. Comments 5202707676, 5202714727, 5202711453. All three remain open.
The roadmap called #1095 merge-ready. Live state says it is a draft, 341 commits behind `dev`. That is the third roadmap claim overturned by direct verification in this loop, after #978 and the #557 CI attribution, and the pattern is consistent: the volatile facts — draft state, commits behind, CI outcome — decay fastest, so each is now re-derived at the P of its own work-phase before it can reach a contributor. Measured per PR (`gh pr view` + `git rev-list --count <head>..origin/dev`): #1095 draft 341 behind #1085 ready 85 behind #1111 draft 0 behind #1056 draft 44 behind #1047 draft 341 behind Four of five are drafts, and three would fail the readiness gate on the at-most-10-behind check alone. So the deliverable changed: instead of escalating a merge decision, each comment names the one blocker that PR owns. There is no merge queue waiting on a maintainer, and reporting one would have been worse than useless. The technical verdicts stand on re-reading the diffs. #1095's terminal repair synthesizes completion only after a structurally complete item lifecycle and leaves real upstream terminals authoritative. #1085 removes an unset secret reference for a loopback-only client without widening the remote-export refusal. #1111's Copilot normalization is sound but carries an unrelated 64 MiB history bound that should be its own PR. #1056 is a bounded opt-in workaround for an upstream Desktop filter, with an unrelated drain test edit to drop. #1047 syncs vision replacements into the `_rawBody` that passthrough actually serializes. Comments 5202740738, 5202742778, 5202744803, 5202747758, 5202750228. All five remain open; zero PRs merged by this loop.
…nces Aggregation barrier for the twelve bug-labelled issues. Four got a comment; seven did not, and the restraint is the substance — each of those already carries an accurate current comment naming the exact missing evidence, so re-stating it would have added noise to a reporter's inbox and bought nothing. #1100 gets the actionable half this issue never had: setting `modelSupportsReasoningSummaries: true` restores the flag Codex gates the whole `reasoning` object on. Verified against the reporter's own provider shape rather than asserted — generating the catalog for a custom openai-chat provider on their BigModel.cn base URL yields `summaries=true` with the key and `false` without it, on identical ladders. The comment also states what the user asserts by setting it, and why we did not simply flip the routed default, which would trade their problem for failures at providers that reject the shape for users who configured nothing. #1024 had a detailed reporter follow-up unanswered since 08-04, and the per-model answer is not uniformly good news. One probe is fixed, one is still blocked on their `TR` config, and one is a disagreement worth having: `mimo-v2.5-free` is deliberately absent from the text-only list because the 2026-08-05 probe found it *accepts* images, with a negative assertion in `tests/provider-registry-parity.test.ts` pinning that. "200 but blind" is a different defect from "rejects images", and adding it would silently replace a real image with a caption. The comment says so and names the capture that would change our mind, rather than quietly agreeing or quietly ignoring it. #1017 and #994 get cross-links to the PRs that block them (#1036, #1068) plus the specific hazard holding each one, so neither reads as abandoned. #994 also gets the question that separates it from a lookalike: the report says "OpenCode models" with no model id, and that 400 text has other causes. No issue closed. #1024 stays open on its `TR` half even though the Zen half shipped — partly fixed is not fixed. Comments 5202766906, 5202780631, 5202769373, 5202771720.
✅ READY
|
github-actions
Bot
added
the
chore
|
✅ Deterministic PR hygiene checks passed. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds remediation and bug-absorption plans, a grade matrix, routed-catalog regression tests, review dispositions, and contributor-response records. The only production behavior covered by code tests is routed reasoning-summary metadata handling. ChangesBug grade remediation
Bug stack absorption
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md (1)
99-115: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the audit evidence match the URL acceptance criteria.
The phase records promise captured comment URLs, but they either omit execution evidence or record only numeric IDs. This leaves the stated acceptance criteria unproven.
devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md#L99-L115: replace comment IDs with the two full URLs, or change the criterion to require IDs.devlog/_plan/260806_bug_grade_remediation/020_wp2_capability_gate_reviews.md#L89-L94: add the two posted comment URLs or mark the criteria as pending.devlog/_plan/260806_bug_grade_remediation/040_wp4_own_pr_and_provenance.md#L85-L94: add the three posting URLs and execution evidence.devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md#L40-L50: add the five posting URLs and execution evidence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md` around lines 99 - 115, Make the audit evidence satisfy the URL-based acceptance criteria: in devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md lines 99-115, replace numeric comment IDs with both full URLs; in 020_wp2_capability_gate_reviews.md lines 89-94, add the two posted comment URLs or mark the criteria pending; in 040_wp4_own_pr_and_provenance.md lines 85-94, add all three posting URLs and execution evidence; and in 050_wp5_grade2_dispositions.md lines 40-50, add all five posting URLs and execution evidence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260806_bug_grade_remediation/000_plan.md`:
- Around line 90-93: Add language identifiers to all four fenced Markdown
blocks: use text in devlog/_plan/260806_bug_grade_remediation/000_plan.md lines
90-93 and
devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md
lines 12-15 and 169-178; use diff in 010_wp1_catalog_joint_contract.md lines
148-155.
- Around line 20-31: Reconcile the claimed 25-item total with the records. In
devlog/_plan/260806_bug_grade_remediation/000_plan.md lines 20-31, either add
the missing bug-labelled PR with its evidence and disposition or correct the
plan totals; make the corresponding addition or total correction in
devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md lines 25-52,
including the missing matrix row and disposition when applicable.
- Around line 95-105: Correct the ordered sync.ts call sequence references in
both documents: devlog/_plan/260806_bug_grade_remediation/000_plan.md lines
95-105 and
devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md
lines 19-33 must identify applyCatalogModelMetadata(e, model) at sync.ts:269,
and state that the template path has no additional metadata call before
restoration. Preserve the exact-source-anchor guidance for reproducing the
normalization-order ablation.
In `@devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md`:
- Around line 22-23: Update the issue references in the paragraphs at the noted
locations, including both “#1100 and `#1102`” and the references around lines
56–59, by wrapping each issue ID in Markdown code spans or escaping each leading
#. Preserve the surrounding text and links so markdownlint no longer interprets
them as headings.
- Around line 37-45: Repair the G3 evidence table by replacing the pipe in the
`#1092` proof with plain text such as “unknown or unsupported” so the row remains
three columns, and add the exact failing test or log anchor to the `#557` Dev-side
proof cell. Verify the table in the G3 section has consistent columns and passes
markdownlint-cli2.
In
`@devlog/_plan/260806_bug_grade_remediation/020_wp2_capability_gate_reviews.md`:
- Around line 3-12: Rewrite the opening shared-principle section to distinguish
advertised or default-derived effort ladders, which are presentation metadata
and must not drive wire fields, from an explicit per-model configured capability
assertion, which may legitimately enable wire behavior. Align the framing with
the later `#978` discussion and avoid describing both PRs as the same
unknown-to-supported reversal.
In
`@devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md`:
- Around line 55-61: Add the text language tag to the fenced code block
containing the dev list, PR list, and lost entries, changing its opening fence
to specify text while preserving the block contents.
In `@devlog/_plan/260806_bug_grade_remediation/040_wp4_own_pr_and_provenance.md`:
- Around line 21-33: Tag the fenced test-output block in the remediation plan
with the text language by changing its opening fence to ```text, while leaving
the captured output unchanged.
In `@devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md`:
- Around line 24-27: Update the freshness-gate statement in the readiness-gate
discussion to say that four PRs fail the “at most 10 commits behind dev” check,
correcting the current count of three while preserving the listed PR details.
In `@devlog/_plan/260806_bug_grade_remediation/060_wp6_g1_issue_dispositions.md`:
- Around line 1-5: Reconcile the document’s counts with its contents: update the
title and acceptance criterion to state 12 total issues, and update the
execution summary to state 4 comments plus 8 deliberate non-comments. Adjust the
relevant headings or summary text near the issue-disposition table and detailed
execution rows without changing the listed issue IDs or dispositions.
---
Outside diff comments:
In
`@devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md`:
- Around line 99-115: Make the audit evidence satisfy the URL-based acceptance
criteria: in
devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md
lines 99-115, replace numeric comment IDs with both full URLs; in
020_wp2_capability_gate_reviews.md lines 89-94, add the two posted comment URLs
or mark the criteria pending; in 040_wp4_own_pr_and_provenance.md lines 85-94,
add all three posting URLs and execution evidence; and in
050_wp5_grade2_dispositions.md lines 40-50, add all five posting URLs and
execution evidence.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 403be10a-7844-4eae-8b14-948c9e2334eb
📒 Files selected for processing (9)
devlog/_plan/260806_bug_grade_remediation/000_plan.mddevlog/_plan/260806_bug_grade_remediation/001_grade_matrix.mddevlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.mddevlog/_plan/260806_bug_grade_remediation/020_wp2_capability_gate_reviews.mddevlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.mddevlog/_plan/260806_bug_grade_remediation/040_wp4_own_pr_and_provenance.mddevlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.mddevlog/_plan/260806_bug_grade_remediation/060_wp6_g1_issue_dispositions.mdtests/codex-catalog.test.ts
| ## Where the triage landed (25 items) | ||
|
|
||
| Twelve issues and thirteen PRs carry the `bug` label. **No item was fake** — every | ||
| reported defect reproduces in code. What separates them is whether the *fix* is | ||
| sound. | ||
|
|
||
| | Grade | Meaning | Items | | ||
| |---|---|---| | ||
| | G1 | Real defect, nobody is fixing it yet | issues #1102 #1100 #1024 #1017 #994 #904 #796 #1059 #418 #417 #241 #92 | | ||
| | G2 | Fix is real and correct | PRs #1095 #1085 (merge-ready) · #1111 #1056 #1047 (small change) | | ||
| | G3 | Bug real, fix dangerous | PRs #1036 #1092 #978 #1068 #557 | | ||
| | G4 | Premise or contract missing | PRs #1093 · #997 (borderline: fix right, proof absent) | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reconcile the 25-item total.
The supplied records enumerate 12 issue items and 12 PR rows, not 25 total items. One bug-labelled item therefore has no grade, evidence, or disposition.
- devlog/_plan/260806_bug_grade_remediation/000_plan.md#L20-L31: add the missing PR and its evidence, or correct the plan totals.
- devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md#L25-L52: add the missing matrix row and disposition, or correct the matrix totals.
📍 Affects 2 files
devlog/_plan/260806_bug_grade_remediation/000_plan.md#L20-L31(this comment)devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md#L25-L52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_grade_remediation/000_plan.md` around lines 20 - 31,
Reconcile the claimed 25-item total with the records. In
devlog/_plan/260806_bug_grade_remediation/000_plan.md lines 20-31, either add
the missing bug-labelled PR with its evidence and disposition or correct the
plan totals; make the corresponding addition or total correction in
devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md lines 25-52,
including the missing matrix row and disposition when applicable.
| ``` | ||
| plain/ladder-model levels=[low,high,max,ultra] default=high summaries=false | ||
| optin/ladder-model levels=[low,high,max,ultra] default=high summaries=true | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to every fenced block.
The supplied Markdown lint results report MD040 for these fences.
- devlog/_plan/260806_bug_grade_remediation/000_plan.md#L90-L93: add
text. - devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md#L12-L15: add
text. - devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md#L148-L155: add
diff. - devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md#L169-L178: add
text.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 2 files
devlog/_plan/260806_bug_grade_remediation/000_plan.md#L90-L93(this comment)devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md#L12-L15devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md#L148-L155devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md#L169-L178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_grade_remediation/000_plan.md` around lines 90 - 93,
Add language identifiers to all four fenced Markdown blocks: use text in
devlog/_plan/260806_bug_grade_remediation/000_plan.md lines 90-93 and
devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md
lines 12-15 and 169-178; use diff in 010_wp1_catalog_joint_contract.md lines
148-155.
Source: Linters/SAST tools
| The per-model opt-in the `:352` comment says should exist **already exists** | ||
| (`modelSupportsReasoningSummaries`, resolved at | ||
| `src/codex/catalog/provider-fetch.ts:545`) and survives the delete, because | ||
| `applyCatalogModelMetadata` runs at `src/codex/catalog/sync.ts:268`, one line | ||
| *after* `normalizeRoutedCatalogEntry`. | ||
|
|
||
| So the defect is narrower and more honest than "we strip the flag": the ordering | ||
| works, the escape hatch works, and **nothing asserts the two stay consistent**. | ||
| A future reordering of those two calls would silently disable effort propagation | ||
| for every routed provider that opted in, and no test would notice. That absent | ||
| joint contract is what wp1 buys, and it is squarely ours regardless of how the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,200p' | rg 'devlog/_plan/260806_bug_grade_remediation/000_plan.md|devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md|src/codex/catalog'
echo "== plan snippets =="
for f in devlog/_plan/260806_bug_grade_remediation/000_plan.md devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md; do
echo "--- $f lines 85-110 / 14-35"
sed -n '85,110p;14,35p' "$f" | cat -n
done
echo "== sync outline =="
ast-grep outline src/codex/catalog/sync.ts --view expanded || true
echo "== sync relevant lines =="
sed -n '240,320p;1,80p' src/codex/catalog/sync.ts | cat -n
echo "== provider-fetch metadata restore locations =="
rg -n "modelSupportsReasoningSummaries|applyCatalogModelMetadata|normalizeRoutedCatalogEntry|applyJawcodeCatalogMetadata|applyReasoningLevels" src/codex/catalog -SRepository: lidge-jun/opencodex
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo root files =="
git ls-files | sed -n '1,220p'
echo "== target file availability =="
for f in \
"devlog/_plan/260806_bug_grade_remediation/000_plan.md" \
"devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md" \
"src/codex/catalog/sync.ts"
do
printf '%s: ' "$f"
test -f "$f" && echo exists || echo missing
done
if [ -f "devlog/_plan/260806_bug_grade_remediation/000_plan.md" ]; then
echo "== 000_plan relevant excerpts =="
sed -n '85,110p;14,35p' devlog/_plan/260806_bug_grade_remediation/000_plan.md | cat -n
fi
if [ -f "devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md" ]; then
echo "== 010_wp1 relevant excerpts =="
sed -n '85,110p;14,35p' devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md | cat -n
fi
if [ -f "src/codex/catalog/sync.ts" ]; then
echo "== sync.ts call sites around 240-320 =="
sed -n '240,320p' src/codex/catalog/sync.ts | cat -n
echo "== sync.ts call sites around template/fallback =="
sed -n '1,80p' src/codex/catalog/sync.ts | cat -n
fi
echo "== search symbols =="
rg -n "modelSupportsReasoningSummaries|applyCatalogModelMetadata|normalizeRoutedCatalogEntry|applyJawcodeCatalogMetadata|applyReasoningLevels" . -S || trueRepository: lidge-jun/opencodex
Length of output: 50377
Correct the ordered sync.ts call sequence in both documents.
src/codex/catalog/sync.ts:269 calls applyCatalogModelMetadata(e, model), not :268, and the template path does not include another metadata call before that restore. Update devlog/_plan/260806_bug_grade_remediation/000_plan.md#L96 and devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md#L19-L33 so readers can reproduce the normalization-order ablation from exact source anchors.
📍 Affects 2 files
devlog/_plan/260806_bug_grade_remediation/000_plan.md#L95-L105(this comment)devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md#L19-L33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_grade_remediation/000_plan.md` around lines 95 - 105,
Correct the ordered sync.ts call sequence references in both documents:
devlog/_plan/260806_bug_grade_remediation/000_plan.md lines 95-105 and
devlog/_plan/260806_bug_grade_remediation/010_wp1_catalog_joint_contract.md
lines 19-33 must identify applyCatalogModelMetadata(e, model) at sync.ts:269,
and state that the template path has no additional metadata call before
restoration. Preserve the exact-source-anchor guidance for reproducing the
normalization-order ablation.
| #1100 and #1102 received maintainer RCA comments on 2026-08-06 | ||
| (`5201923105`, `5201927225`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape issue references that start with #.
markdownlint-cli2 reports MD018 at Lines 22 and 56. Wrap the issue IDs in code spans or escape the # character so these lines remain paragraphs instead of malformed ATX headings.
Also applies to: 56-59
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 22-22: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md` around lines
22 - 23, Update the issue references in the paragraphs at the noted locations,
including both “#1100 and `#1102`” and the references around lines 56–59, by
wrapping each issue ID in Markdown code spans or escaping each leading #.
Preserve the surrounding text and links so markdownlint no longer interprets
them as headings.
Source: Linters/SAST tools
| ### G3 — bug real, fix dangerous | ||
|
|
||
| | PR | Hazard | Dev-side proof it is deliberate | | ||
| |---|---|---| | ||
| | #1036 | converts every call named `edit_file`/`multi_edit`, so a genuine client/MCP tool with that name is silently reinterpreted as `apply_patch` | the PR's own tool builder avoids shadowing those names, then the translator ignores that distinction | | ||
| | #1092 | injects a combo default effort for targets whose ladder is UNKNOWN | dev fails closed and logs `capability: unknown|unsupported` (`src/combos/request.ts:43-55`) | | ||
| | #978 | sends `thinkingLevel` to any model with a configured ladder | dev restricts it to two direct Flash ids (`src/adapters/google.ts:313-326`); a configured ladder is a UI contract, not a wire capability | | ||
| | #1068 | drops `OPENCODE_ZEN_TEXT_ONLY_MODELS`, which protects six models (`src/providers/registry.ts:372-379`), and uses obsolete generic DeepSeek constants superseded by per-model ladders (`:380-434`) | | ||
| | #557 | skips the preflight entirely on Windows; the PR claims "7000 pass / 0 fail" while the recorded Windows run has two failing update tests | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Repair the G3 evidence table.
Line 42 contains unknown|unsupported, so Markdown parses four cells instead of three. Line 45 has no Dev-side proof cell, so the #557 evidence is not represented in the table. Replace the pipe with text such as unknown or unsupported, and add the exact failing test or log anchor for #557.
markdownlint-cli2 reports the table-column defects in this range.
Proposed table correction
-| `#1092` | injects a combo default effort for targets whose ladder is UNKNOWN | dev fails closed and logs `capability: unknown|unsupported` (`src/combos/request.ts:43-55`) |
+| `#1092` | injects a combo default effort for targets whose ladder is UNKNOWN | dev fails closed and logs `capability: unknown or unsupported` (`src/combos/request.ts:43-55`) |
-| `#557` | skips the preflight entirely on Windows; the PR claims "7000 pass / 0 fail" while the recorded Windows run has two failing update tests |
+| `#557` | skips the preflight entirely on Windows | the PR claims "7000 pass / 0 fail", but the recorded Windows run has two failing update tests; add the exact test names or log anchor |🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 42-42: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 42-42: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 44-44: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
[warning] 45-45: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_grade_remediation/001_grade_matrix.md` around lines
37 - 45, Repair the G3 evidence table by replacing the pipe in the `#1092` proof
with plain text such as “unknown or unsupported” so the row remains three
columns, and add the exact failing test or log anchor to the `#557` Dev-side proof
cell. Verify the table in the G3 section has consistent columns and passes
markdownlint-cli2.
Source: Linters/SAST tools
| Both PRs fix a real annoyance by deleting the distinction between *unknown* and | ||
| *supported*. The reviews must make that shared principle explicit, because each | ||
| author will otherwise read the objection as a nitpick about their one file. | ||
|
|
||
| ## Shared principle to state in both | ||
|
|
||
| A configured or advertised effort ladder is a **presentation** contract: it says | ||
| what OpenCodex offers a user. It is not evidence that the upstream endpoint | ||
| accepts that wire field. Turning presentation into wire behavior is how a 400 | ||
| reaches a user who changed nothing. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the shared framing before using this document to drive review.
Lines 3-5 describe #1092 and #978 as the same unknown-to-supported reversal. Lines 9-12 state that a configured ladder is not evidence for wire behavior. Lines 41-66 later correct this for #978: an explicit configured ladder is a user capability assertion and may change wire behavior.
This contradiction can cause the valid #978 change to be blocked. Rewrite the opening to distinguish advertised or default-derived metadata from an explicit per-model capability assertion.
Proposed wording change
-Both PRs fix a real annoyance by deleting the distinction between *unknown* and
-*supported*.
+PR `#1092` reverses the distinction between *unknown* and *supported*.
+PR `#978` uses an explicit configured ladder as a user capability assertion.
-A configured or advertised effort ladder is a **presentation** contract.
+An advertised or default-derived effort ladder is a **presentation** contract.
+An explicit per-model configuration is a user capability assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_grade_remediation/020_wp2_capability_gate_reviews.md`
around lines 3 - 12, Rewrite the opening shared-principle section to distinguish
advertised or default-derived effort ladders, which are presentation metadata
and must not drive wire fields, from an explicit per-model configured capability
assertion, which may legitimately enable wire behavior. Align the framing with
the later `#978` discussion and avoid describing both PRs as the same
unknown-to-supported reversal.
| ``` | ||
| dev list: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, | ||
| north-mini-code-free, laguna-s-2.1-free, deepseek-v4-flash-free | ||
| PR list: deepseek-v4-pro, deepseek-v4-flash, deepseek-v4-flash-free | ||
| lost: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, | ||
| north-mini-code-free, laguna-s-2.1-free | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a language tag to the fenced block.
markdownlint-cli2 reports MD040 at Line 55. Mark this set-arithmetic block as text.
Proposed Markdown fix
-```
+```text
dev list: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free,
...
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| dev list: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, | |
| north-mini-code-free, laguna-s-2.1-free, deepseek-v4-flash-free | |
| PR list: deepseek-v4-pro, deepseek-v4-flash, deepseek-v4-flash-free | |
| lost: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, | |
| north-mini-code-free, laguna-s-2.1-free | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@devlog/_plan/260806_bug_grade_remediation/030_wp3_correctness_hazard_reviews.md`
around lines 55 - 61, Add the text language tag to the fenced code block
containing the dev list, PR list, and lost entries, changing its opening fence
to specify text while preserving the block contents.
Source: Linters/SAST tools
| ``` | ||
| 2 fail | ||
| Ran 7008 tests across 482 files. [853.00s] | ||
|
|
||
| (fail) GUI update execution decisions > persists installer-derived job fields | ||
| without raw cache paths or uid values | ||
| (fail) GUI update execution decisions > sanitizer redacts space-containing | ||
| profile paths (Wibias reproduction) | ||
|
|
||
| 175 | spawnStart: () => { throw new Error("must not spawn"); }, | ||
| error: must not spawn | ||
| at spawnStart (tests\update-job.test.ts:175:37) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a language tag to the fenced test-output block.
markdownlint-cli2 reports MD040 at Line 21. Mark this output block as text.
Proposed Markdown fix
-```
+```text
2 fail
Ran 7008 tests across 482 files. [853.00s]
...
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| 2 fail | |
| Ran 7008 tests across 482 files. [853.00s] | |
| (fail) GUI update execution decisions > persists installer-derived job fields | |
| without raw cache paths or uid values | |
| (fail) GUI update execution decisions > sanitizer redacts space-containing | |
| profile paths (Wibias reproduction) | |
| 175 | spawnStart: () => { throw new Error("must not spawn"); }, | |
| error: must not spawn | |
| at spawnStart (tests\update-job.test.ts:175:37) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_grade_remediation/040_wp4_own_pr_and_provenance.md`
around lines 21 - 33, Tag the fenced test-output block in the remediation plan
with the text language by changing its opening fence to ```text, while leaving
the captured output unchanged.
Source: Linters/SAST tools
| Four of the five are drafts. The repository's readiness gate keeps a draft in | ||
| draft until its author ticks the four-box checklist, and the gate verifies two of | ||
| those claims itself — green `ci`, and the branch at most 10 commits behind dev. | ||
| At 341, 85 and 44 behind, three of these would fail that check today. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the freshness-gate count.
Four PRs are more than 10 commits behind dev: #1095 and #1047 are 341 commits behind, #1085 is 85 behind, and #1056 is 44 behind. Only #1111 is at 0.
Line 27 must say “four”, not “three”.
Proposed wording change
-At 341, 85 and 44 behind, three of these would fail that check today.
+At 341, 85 and 44 behind, four of these would fail that check today.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Four of the five are drafts. The repository's readiness gate keeps a draft in | |
| draft until its author ticks the four-box checklist, and the gate verifies two of | |
| those claims itself — green `ci`, and the branch at most 10 commits behind dev. | |
| At 341, 85 and 44 behind, three of these would fail that check today. | |
| Four of the five are drafts. The repository's readiness gate keeps a draft in | |
| draft until its author ticks the four-box checklist, and the gate verifies two | |
| of those claims itself — green `ci`, and the branch at most 10 commits behind dev. | |
| At 341, 85 and 44 behind, four of these would fail that check today. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_grade_remediation/050_wp5_grade2_dispositions.md`
around lines 24 - 27, Update the freshness-gate statement in the readiness-gate
discussion to say that four PRs fail the “at most 10 commits behind dev” check,
correcting the current count of three while preserving the listed PR details.
| # 060 — wp6: G1 issue dispositions (the 11 wp1 does not fix) | ||
|
|
||
| Added after the plan audit found the objective promising an outcome for every | ||
| bug-labelled item while the phase map scheduled work for only one issue. This | ||
| phase closes that gap honestly: a disposition is not a fix, and saying so is the |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile the issue count with the table and execution record.
The document contains 12 issue IDs, not 11. The final table row contains four IDs: #241, #417, #92, and #418. The detailed execution rows also contain eight deliberate non-comments, not seven.
Update the title, acceptance criterion, and execution summary to use 12 total issues and 4 comments plus 8 deliberate non-comments.
Proposed wording changes
-# 060 — wp6: G1 issue dispositions (the 11 wp1 does not fix)
+# 060 — wp6: G1 issue dispositions (the 12 wp1 does not fix)
-Four comments posted, seven deliberately not.
+Four comments posted, eight deliberately not.
-- Every one of the 11 issues has a disposition and a stated unblocking condition.
+- Every one of the 12 issues has a disposition and a stated unblocking condition.Also applies to: 20-30, 50-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_grade_remediation/060_wp6_g1_issue_dispositions.md`
around lines 1 - 5, Reconcile the document’s counts with its contents: update
the title and acceptance criterion to state 12 total issues, and update the
execution summary to state 4 comments plus 8 deliberate non-comments. Adjust the
relevant headings or summary text near the issue-disposition table and detailed
execution rows without changing the listed issue IDs or dispositions.
![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: 2fc5261ef7
ℹ️ 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".
| | G2 | Fix is real and correct | PRs #1095 #1085 (merge-ready) · #1111 #1056 #1047 (small change) | | ||
| | G3 | Bug real, fix dangerous | PRs #1036 #1092 #978 #1068 #557 | |
There was a problem hiding this comment.
These rows are presented as the final triage result but conflict with the phase ledgers added by the same commit: 020_wp2_capability_gate_reviews.md says the dangerous-fix grading for #978 was wrong and its disposition is not blocked, while 050_wp5_grade2_dispositions.md says #1095 is a draft 341 commits behind and explicitly not merge-ready. Leaving the overview and 001_grade_matrix.md unchanged can cause maintainers to block a sound change or treat an unready PR as mergeable; update both summaries to reflect the corrected verdicts.
Useful? React with 👍 / 👎.
| modelReasoningEfforts: { "effort-model": ["low", "high", "max"] }, | ||
| modelSupportsReasoningSummaries: { "effort-model": true }, |
There was a problem hiding this comment.
This test makes modelSupportsReasoningSummaries: true a supported user workaround for configured providers affected by #1100, but docs-site/src/content/docs/reference/configuration/providers.md only documents the false behavior and never tells users that true enables the catalog flag Codex uses to emit the entire reasoning object. Users following the configuration reference therefore cannot discover the workaround or assess what capability they are asserting; document the positive semantics here and in the translated provider references.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
| // Pins the asymmetry itself: the fallback path skips | ||
| // normalizeRoutedCatalogEntry entirely, so this row is opt-in-true for a | ||
| // different reason than the template row above. Kept explicit so a future | ||
| // unification of the two construction paths is a visible change. | ||
| const routed = buildCatalogEntries(null, [], models).find(e => e.slug === "ladder/effort-model"); | ||
| expect(routed?.supports_reasoning_summaries).toBe(true); |
There was a problem hiding this comment.
This assertion does not pin the claimed no-template asymmetry: if the fallback path is unified by calling normalizeRoutedCatalogEntry before applyCatalogModelMetadata, the metadata application restores the explicit opt-in and this expectation remains true. The exact future refactor the comment says should become visible can therefore land with this test green; exercise an observable effect unique to normalization (or narrow the test and its claim to the final opt-in value) rather than claiming that the strip was never invoked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fc5261ef7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| that the header is accepted from any client if it matches a regex, while the | ||
| description calls the values "guard-issued". Nothing issues them. |
There was a problem hiding this comment.
This wp4 entry records the security review rationale for an unmerged change: the client-supplied ingress span is accepted into persisted logs and the follow-up checklist asks who may issue/trust it. Because devlog/_plan is a tracked public directory and #1093 has not shipped, keeping this pre-release security triage here violates the repository boundary for security working notes; move the analysis to scratch space or reduce the tracked ledger to a non-sensitive public outcome after the fix/review is complete.
AGENTS.md reference: AGENTS.md:L61-L68
Useful? React with 👍 / 👎.
| - Both comment URLs captured. | ||
| - The #1092 comment quotes the dev-side source it asks the author to preserve. | ||
| - The #978 comment records the corrected verdict with the measured evidence, and | ||
| does not ask for a rewrite the diff does not need. |
There was a problem hiding this comment.
This phase requires both PR comment URLs to be captured, but the file never records an Executed section or any comment IDs; wp4 and wp5 have the same gap while wp3/wp6 do capture IDs. Since the commit summary says those reviews were posted, leaving these ledgers without stable GitHub references makes the devlog unable to prove which comments are authoritative or confirm the phase actually met its own acceptance criteria without re-querying live PR state.
Useful? React with 👍 / 👎.
Second loop of the bug campaign, scoped to author corrected replacements for
four contributor PRs and close theirs as absorbed. Three adversarial audit
rounds refuted the premise, and the plan now records that instead of the
outcome it was written to produce.
The decisive finding: I judged "has the author responded to our review" by
`updatedAt`, which moves when WE comment and therefore can never show author
activity. Comparing last-commit time against review time gives the real
picture:
#1092 commits 09:38:19Z review 09:09:51Z -> acted, in under 30 minutes
#1068 commits 08:52:23Z review 09:13:15Z -> predates the review
#1036 commits 08-05 review 09:12:51Z -> no response yet
#997 commits 02:51Z review 09:16:02Z -> no response yet
Under the wrong reading I was about to close #1092 as absorbed — taking
credit for work its author did in direct response to my own request — and to
credit #1068 with agreeing to a review it had not yet read. Neither is a
process nit; both would have been visible to the contributor.
So the plan changes shape. #1092 and #1068 become re-reviews. #1036 and #997
get a stated 72-hour response window with a mandatory head re-check before
anything is authored or closed; "has not replied within an hour" is not
abandonment, and #997's author was active at 02:51Z. This loop therefore
authors no absorbed layer at all, and says so.
#1068's re-review carries a finding neither side has: its new test asserts
all three DeepSeek ids are in `noVisionModels`, but routing merges the
registry list, which holds only the `-free` one. Reproduced with the test's
own routeModel config — Pro=false, Flash=false, Flash-free=true — so two of
three cases fail. Latent because no check currently runs that suite.
Also corrected: the layers were called dependency-ordered when they share no
files, so s3/s4 are independent heads off `origin/dev` and the
`--update-refs` cascade is reserved for genuine chains; and #978's exclusion
no longer claims it is "already correct" when it needs an author-side docs
change.
Terminal outcome is deferred, not DONE. Shrinking the criteria to match what
finished would have hidden exactly the thing worth recording.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
I cannot approve this revision yet. The runtime test addition is directionally useful, but the current head has unresolved blockers that affect the integrity and publication safety of the unit:
- The tracked devlog includes pre-merge security analysis for an unshipped PR. That conflicts with the repository rule requiring unreleased security working notes to stay in scratch space. Remove that analysis from the public unit or reduce it to a non-sensitive outcome after the underlying change ships.
- The no-template fallback test does not detect whether normalization ran; applying metadata after normalization still leaves the asserted opt-in true. Either assert a normalization-only observable or narrow the test and documentation claim to the final opt-in contract.
- The overview/grade matrix conflicts with the later phase ledgers for at least #978 and #1095, and the item/count tables are internally inconsistent. Those summaries drive maintainer disposition, so they need to be reconciled before this is treated as an authoritative triage record.
Please also resolve or explicitly disposition the remaining factual review threads. CI is green, but green CI does not resolve these content and policy blockers.
Records what the stack-and-absorb campaign actually produced once live data replaced its assumptions. #1068 merged at 10:45:57Z while I was writing its review, and the merged code is not what I reviewed: `noVisionModels: [...OPENCODE_ZEN_TEXT_ONLY_MODELS, ...DEEPSEEK_THINKING_MODELS]` — the union the review asked for. Running the merged suite against `origin/dev` gives 9 pass / 0 fail, including a new test pinning the six probed text-only models. My 10:53Z comment claiming a failing test was true of the head I had fetched and false of what landed, so it got a public correction eight minutes later. That is the second process error in this unit, and both are now written down rather than quietly fixed. First: judging author activity by `updatedAt`, which moves when WE comment. Second: commenting against a stale fetched head. Each produced a wrong public statement to a contributor. The rules are re-fetch immediately before commenting, and compare last-commit time against review time. #1092's author restored the fail-closed guard within 30 minutes of the review, with a comment keeping `unknown` distinct in debug. Credited explicitly; asked only to split the unrelated `imageInput` scope. #1036 and #997 are deferred, not absorbed. Their last commits (08-05 07:18Z, 08-06 02:51Z) predate our reviews and neither carries the requested change, but roughly two hours have passed and #997's author was active that morning. 050 states a 72-hour response window with a mandatory head re-check before anything is authored or closed. The window exists because this unit demonstrated the alternative: two of four absorb targets were being actively fixed by their authors while we drafted replacements. #994 updated with the dev-only landing (merge 7d0c02d, ancestor of `origin/dev`) and deliberately left open pending the reporter's provider confirmation and a release. Zero contributor PRs closed by this loop.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260806_bug_stack_absorb/000_plan.md`:
- Around line 20-29: Update the audit comparison table in the measured
2026-08-06 section to record full ISO-8601 UTC timestamps, the evaluated head
commit SHA, the review reference, and the calculated 72-hour deadline for each
PR. Replace the current abbreviated Last commit and Our review fields while
preserving the Acted after assessment, and apply the same contract to the
corresponding table entries around the additional referenced section.
In `@devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md`:
- Around line 23-27: Add a regression test that exercises the production routing
and provider-derivation path for the affected Zen DeepSeek models, verifying
that preserveReasoningContentModels enables plaintext reasoning replay and
removes proxy-generated ocxr1 content during serialization. Retain the existing
default blanking assertion, and avoid passing preserveRawReasoningContent
directly so the test validates routed serializer behavior rather than only the
low-level option.
- Around line 31-45: Revise the set-arithmetic section to explicitly identify
the destructive replacement as occurring in an earlier revision, not the current
head. Reference the relevant commit or branch state using the historical context
available in the document, and replace the present-tense “The PR sets” wording
so readers do not interpret it as a current finding.
In `@devlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.md`:
- Around line 144-149: Mark the Verification section as historical for the
withdrawn absorption plan, or explicitly state that its tests, lint/typecheck
commands, and GUI screenshot are not required for the current re-review.
- Around line 12-15: Update the referenced bug description to use precise
capability terminology: do not say the user is “entitled to” an effort when an
unknown ladder provides no such capability; state that a known target may
advertise the effort while src/combos/request.ts omits the default for the
unknown target. Replace “known-but-missing ladders” with “known-but-empty
ladders,” preserving the distinction from undefined.
In `@devlog/_plan/260806_bug_stack_absorb/030_s3_cursor_edit_provenance.md`:
- Around line 66-69: Add a regression case for mapSyntheticMcpExecToToolEvents
that uses the same encoded McpArgs but omits the state argument, asserting the
original tool name is preserved and the output contains no "*** Begin Patch".
Include this stateless case in the red ablation alongside the existing stateful
regression, without changing the pass-through behavior.
In `@devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md`:
- Around line 63-65: Update the ablation instructions around the beforeEach
assignment to explicitly unset OPENCODEX_HOME when running the test, using the
provided env -u OPENCODEX_HOME command. Record that the commented-out assignment
makes expect(getConfigDir()).toBe(testDir) fail by resolving to the real home,
and restoring it makes the test pass.
- Around line 43-49: Replace the cast-only fixture passed to addRequestLog with
a runtime-valid RequestLogEntry containing the required persisted fields and a
concrete requestId. After the write, read usage.jsonl from testDir and assert
the persisted content includes that requestId, while retaining the
scratch-directory isolation assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2367cf04-75ad-4b86-88d8-07b78eecc670
📒 Files selected for processing (5)
devlog/_plan/260806_bug_stack_absorb/000_plan.mddevlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.mddevlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.mddevlog/_plan/260806_bug_stack_absorb/030_s3_cursor_edit_provenance.mddevlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md
| **Measured 2026-08-06 by comparing LAST COMMIT time against OUR REVIEW time** — | ||
| the second audit round caught the first version of this table using | ||
| `updatedAt`, which moves when *we* comment and therefore proved nothing: | ||
|
|
||
| | PR | Author | Last commit | Our review | Acted after? | | ||
| |---|---|---|---|---| | ||
| | #1092 | eachann1024 | 09:38:19Z | 09:09:51Z | **YES** — fail-closed guard restored with a comment keeping `unknown` distinct | | ||
| | #1068 | justjxke | 08:52:23Z | 09:13:15Z | **no** — commits predate the review by 21 minutes | | ||
| | #1036 | ZachDreamZ | 08-05 07:18Z | 09:12:51Z | no | | ||
| | #997 | Yuxin-Qiao | 08-06 02:51Z | 09:16:02Z | no | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record immutable audit references.
The table uses incomplete timestamps and does not record the evaluated commit SHA or review reference. A later re-review cannot prove which head was compared or which review started the 72-hour window. Record full ISO-8601 UTC timestamps, the head SHA, the review reference, and the calculated deadline.
Suggested table contract
-| PR | Author | Last commit | Our review | Acted after? |
+| PR | Author | Last commit (UTC + SHA) | Our review (UTC + reference) | Deadline (UTC) | Acted after? |Also applies to: 42-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/000_plan.md` around lines 20 - 29,
Update the audit comparison table in the measured 2026-08-06 section to record
full ISO-8601 UTC timestamps, the evaluated head commit SHA, the review
reference, and the calculated 72-hour deadline for each PR. Replace the current
abbreviated Last commit and Our review fields while preserving the Acted after
assessment, and apply the same contract to the corresponding table entries
around the additional referenced section.
| justjxke diagnosed #994 correctly. `opencode-zen` and `opencode-free` reach the | ||
| SAME gateway (`https://opencode.ai/zen/v1`), and DeepSeek thinking mode requires | ||
| the assistant's original `reasoning_content` to be replayed on tool-call | ||
| continuations or the gateway answers 400. `opencode-free` carries that metadata; | ||
| `opencode-zen` does not. That asymmetry is the bug, and finding it was the work. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add a routed serializer regression.
The core defect is loss of the original reasoning_content during Zen tool-call continuations. The three proposed assertions inspect registry membership and ladder differences, but they do not serialize a routed Zen request.
The supplied tests/deepseek-reasoning-replay.test.ts:1-55 passes preserveRawReasoningContent: true directly. It does not prove that routing and provider derivation translate preserveReasoningContentModels into that serializer behavior.
Add a test that routes the relevant Zen DeepSeek models through the production path, preserves plaintext reasoning, and removes proxy-minted ocxr1 content. Keep the default blanking assertion. Otherwise, registry metadata can pass while the #994 replay failure remains.
Also applies to: 107-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md` around
lines 23 - 27, Add a regression test that exercises the production routing and
provider-derivation path for the affected Zen DeepSeek models, verifying that
preserveReasoningContentModels enables plaintext reasoning replay and removes
proxy-generated ocxr1 content during serialization. Retain the existing default
blanking assertion, and avoid passing preserveRawReasoningContent directly so
the test validates routed serializer behavior rather than only the low-level
option.
| The PR sets `noVisionModels` to the DeepSeek list, **replacing** | ||
| `OPENCODE_ZEN_TEXT_ONLY_MODELS` rather than extending it. Set arithmetic on the | ||
| real constants: | ||
|
|
||
| ``` | ||
| dev: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, | ||
| north-mini-code-free, laguna-s-2.1-free, deepseek-v4-flash-free | ||
| PR: deepseek-v4-pro, deepseek-v4-flash, deepseek-v4-flash-free | ||
| lost: big-pickle, nemotron-3-ultra-free, ling-3.0-flash-free, | ||
| north-mini-code-free, laguna-s-2.1-free | ||
| ``` | ||
|
|
||
| Five models stop being marked text-only, so the proxy resumes forwarding image | ||
| parts and the gateway 400s the request — reopening #1043. `deepseek-v4-flash-free` | ||
| sits in both sets, which is almost certainly why the replacement looked safe. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the set arithmetic as historical.
Lines 12-16 state that the destructive replacement is absent from the current diff, but Lines 31-45 say “The PR sets” in the present tense. Mark this as an earlier revision and identify the relevant commit or branch state. This prevents readers from treating a historical defect as a current-head finding.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md` around
lines 31 - 45, Revise the set-arithmetic section to explicitly identify the
destructive replacement as occurring in an earlier revision, not the current
head. Reference the relevant commit or branch state using the historical context
available in the document, and replace the present-tense “The PR sets” wording
so readers do not interpret it as a current finding.
| eachann1024 found the UI bug: `intersectComboEfforts` | ||
| (`gui/src/combo-workspace-data.ts:11`) maps an unknown ladder to `[]`, and an | ||
| empty member wipes the intersection, so ONE undiscovered target empties the whole | ||
| picker and the user cannot select an effort they are entitled to. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use precise capability terminology.
An unknown ladder does not establish that a user is “entitled to” an effort. A known target may advertise the effort, while src/combos/request.ts omits the default for the unknown target. Also replace “known-but-missing ladders” with “known-but-empty ladders” to preserve the distinction from undefined.
Also applies to: 157-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.md` around
lines 12 - 15, Update the referenced bug description to use precise capability
terminology: do not say the user is “entitled to” an effort when an unknown
ladder provides no such capability; state that a known target may advertise the
effort while src/combos/request.ts omits the default for the unknown target.
Replace “known-but-missing ladders” with “known-but-empty ladders,” preserving
the distinction from undefined.
| ## Verification | ||
|
|
||
| - `bun test tests/combo-workspace-data.test.ts` | ||
| - `bun test tests/combos.test.ts` | ||
| - `bun run lint:gui`, `bun run typecheck` | ||
| - GUI screenshot required by `enforce-target` (title/description mentions gui). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark this verification block as historical.
The document later states that the accept criteria are superseded and that the deliverable is a re-review, not an absorption. Label this test and screenshot checklist as historical verification for the withdrawn plan, or state that it is not required for the current re-review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.md` around
lines 144 - 149, Mark the Verification section as historical for the withdrawn
absorption plan, or explicitly state that its tests, lint/typecheck commands,
and GUI screenshot are not required for the current re-review.
| The stateless fallback at `protobuf-events.ts:314` has no state by design (direct | ||
| and unit callers). It **defaults to pass-through**: no provenance, no conversion. | ||
| That is the fail-closed direction — an unconverted structured call is a visible, | ||
| recoverable failure; a wrongly converted one silently edits a file. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a regression for the stateless no-provenance path.
The plan claims that calls without provenance pass through, but the shown regression supplies { state }. It does not exercise mapSyntheticMcpExecToToolEvents without state. A name-only conversion can remain in direct or unit callers while this test passes.
Add a second case with the same encoded McpArgs, omit { state }, and assert the original tool name and the absence of *** Begin Patch. Include this case in the red ablation.
Also applies to: 98-123, 130-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/030_s3_cursor_edit_provenance.md` around
lines 66 - 69, Add a regression case for mapSyntheticMcpExecToToolEvents that
uses the same encoded McpArgs but omits the state argument, asserting the
original tool name is preserved and the output contains no "*** Begin Patch".
Include this stateless case in the red ablation alongside the existing stateful
regression, without changing the pass-through behavior.
| addRequestLog({ /* minimal fixture row */ } as RequestLogEntry); | ||
|
|
||
| expect(existsSync(join(testDir, "usage.jsonl"))).toBe(true); | ||
| // The real home must be untouched by this run. If it already exists on the | ||
| // developer's machine we cannot assert absence, so assert the write landed in | ||
| // the scratch dir and that the resolver never pointed at the real path. | ||
| expect(getConfigDir().startsWith(tmpdir())).toBe(true); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching relevant names:"
git ls-files | rg '(^|/)(040_s4_usage_log_isolation\.md|request-log\.ts|config\.ts)$' || true
echo
echo "Plan excerpt:"
if [ -f devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md ]; then
nl -ba devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md | sed -n '1,90p'
fi
echo
echo "request-log relevant outline:"
if [ -f src/server/request-log.ts ]; then
ast-grep outline src/server/request-log.ts --match addRequestLog --view expanded || true
nl -ba src/server/request-log.ts | sed -n '280,365p'
fi
echo
echo "Search request log fixtures/imports:"
rg -n "RequestLogEntry|addRequestLog|usage\.jsonl|readFileSync|getConfigDir|resolveConfigDir" src/devlog devlog -S || trueRepository: lidge-jun/opencodex
Length of output: 362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Plan excerpt:"
awk 'NR<=100{printf "%6d\t%s\n", NR, $0}' devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md
echo
echo "request-log relevant excerpt:"
awk 'NR>=280 && NR<=365{printf "%6d\t%s\n", NR, $0}' src/server/request-log.ts
echo
echo "search relevant identifiers:"
rg -n "RequestLogEntry|addRequestLog|usage\.jsonl|readFileSync|getConfigDir|resolveConfigDir" . -S || trueRepository: lidge-jun/opencodex
Length of output: 50378
Use a valid RequestLogEntry fixture and assert the persisted row.
as RequestLogEntry only suppresses TypeScript checks. It does not populate runtime fields. src/server/request-log.ts:302-356 rebuilds the usage.jsonl row from fields such as requestId, timestamp, provider, model, status, durationMs, and usageStatus, and addRequestLog swallows failures. Make the fixture valid, for example an empty persisted payload plus one concrete requestId, then read join(testDir, "usage.jsonl") and assert it contains that value. existsSync alone only proves that a file exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md` around
lines 43 - 49, Replace the cast-only fixture passed to addRequestLog with a
runtime-valid RequestLogEntry containing the required persisted fields and a
concrete requestId. After the write, read usage.jsonl from testDir and assert
the persisted content includes that requestId, while retaining the
scratch-directory isolation assertion.
| Comment out `process.env.OPENCODEX_HOME = testDir;` in `beforeEach`. The new test | ||
| must fail on `expect(getConfigDir()).toBe(testDir)` — resolving to the real home. | ||
| Restore, it passes. Record both. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the red ablation independent of the ambient environment.
The test resolves to homedir/.opencodex only when OPENCODEX_HOME is unset. If the test runner inherits another value, removing the assignment resolves to that configured directory instead. Record the ablation with OPENCODEX_HOME explicitly unset.
env -u OPENCODEX_HOME bun test tests/management-api-logs-metrics.test.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/040_s4_usage_log_isolation.md` around
lines 63 - 65, Update the ablation instructions around the beforeEach assignment
to explicitly unset OPENCODEX_HOME when running the test, using the provided env
-u OPENCODEX_HOME command. Record that the commented-out assignment makes
expect(getConfigDir()).toBe(testDir) fail by resolving to the real home, and
restoring it makes the test pass.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md`:
- Line 196: Update the heading line beginning with “#994 updated with the
dev-only landing” to use valid Markdown ATX syntax by adding a space after the
heading marker, while preserving the intended heading level and text.
In `@devlog/_plan/260806_bug_stack_absorb/050_deferral_record.md`:
- Around line 6-10: Update the PR table in 050_deferral_record.md to add the
exact per-PR response deadlines: 2026-08-09T09:12:51Z for `#1036` and
2026-08-09T09:16:02Z for `#997`. Revise the 72-hour re-check and takeover rules in
the referenced deadline sections so each PR is evaluated only after its
applicable timestamp, not merely on the calendar date.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71ca156c-6161-40c0-98e0-516195136d6e
📒 Files selected for processing (3)
devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.mddevlog/_plan/260806_bug_stack_absorb/020_s2_combo_picker_unknown.mddevlog/_plan/260806_bug_stack_absorb/050_deferral_record.md
| are now guarded on the safe assumption, which is the right default — a stripped | ||
| image degrades to a caption, an unguarded one 400s the whole request. | ||
|
|
||
| #994 updated with the dev-only landing (merge `7d0c02dbe`, ancestor of |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Markdown heading syntax.
Line 196 has no space after the # marker, so Markdown does not parse it as an ATX heading. Use # #994 updated with the dev-only landing ... or the intended heading level.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 196-196: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/010_s1_zen_deepseek_replay.md` at line
196, Update the heading line beginning with “#994 updated with the dev-only
landing” to use valid Markdown ATX syntax by adding a space after the heading
marker, while preserving the intended heading level and text.
Source: Linters/SAST tools
| | PR | Last commit | Our review | Author response | | ||
| |---|---|---|---| | ||
| | #1036 | 2026-08-05T07:18:36Z | 2026-08-06T09:12:51Z | none yet | | ||
| | #997 | 2026-08-06T02:51:22Z | 2026-08-06T09:16:02Z | none yet | | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Record the exact per-PR response deadlines.
Line [15] defines a 72-hour window from each review, but Lines [34]-[37] permit takeover on the date 2026-08-09. This can allow takeover before the window expires. The deadlines are 2026-08-09T09:12:51Z for #1036 and 2026-08-09T09:16:02Z for #997.
Add these timestamps to the table and require the re-check only after the applicable PR deadline.
Proposed correction
-| PR | Last commit | Our review | Author response |
-|---|---|---|---|
-| `#1036` | 2026-08-05T07:18:36Z | 2026-08-06T09:12:51Z | none yet |
-| `#997` | 2026-08-06T02:51:22Z | 2026-08-06T09:16:02Z | none yet |
+| PR | Last commit | Our review | Response deadline | Author response |
+|---|---|---|---|---|
+| `#1036` | 2026-08-05T07:18:36Z | 2026-08-06T09:12:51Z | 2026-08-09T09:12:51Z | none yet |
+| `#997` | 2026-08-06T02:51:22Z | 2026-08-06T09:16:02Z | 2026-08-09T09:16:02Z | none yet |
-Only if a head still lacks the fix on 2026-08-09:
+Only if a head still lacks the fix after that PR's response deadline:Also applies to: 15-17, 32-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/050_deferral_record.md` around lines 6 -
10, Update the PR table in 050_deferral_record.md to add the exact per-PR
response deadlines: 2026-08-09T09:12:51Z for `#1036` and 2026-08-09T09:16:02Z for
`#997`. Revise the 72-hour re-check and takeover rules in the referenced deadline
sections so each PR is evaluated only after its applicable timestamp, not merely
on the calendar date.
#1059 carried "~207 failures" from an aborted promotion run and had not been re-measured since 2026-08-01. Dispatched the Windows leg against `dev` (run 31095755263) and waited for all four shards: shard 1/4 TIMED OUT at the 15-minute cap — no counts recoverable shard 2/4 8 fail shard 3/4 21 fail shard 4/4 47 fail Grouping the 152 recoverable `(fail)` lines by suite shows they are not spread evenly: 32 injectCodexConfig integration (Design B) 30 Codex catalog sync hardening 16 Codex catalog restore 12 two real processes contend for one lock 12 server same-target 429 retry (end-to-end) 12 codex-journal 6 Issue #702 expired forward replay state About three quarters are one family — Codex catalog and config file writing. The failing assertions are about atomic publication and permission narrowing ("writes prepared bytes atomically with the right...", "group-readable is narrowed back to owner-only", CAS txId conflicts), which is exactly where POSIX assumptions leak on Windows: rename-over-existing, chmod bits, and advisory locking. So the recorded burn-down order ("management/server fixtures first") is superseded by evidence: fix the write substrate first and roughly 78 failures should fall together rather than shard by shard. Shard 1's timeout is split out as its own defect. A shard that cannot finish gives no signal on a quarter of the suite, so no claim is made about whether the total improved against the ~207 estimate — that figure stays labelled unverified rather than contradicted. #1102 gets no new comment: the reporter has not answered the RCA's two questions, and the only fix that helps a direct-spawned app-server is an opt-in "trust loopback peers on a non-loopback bind" — a security boundary change that is the maintainer's call, not an agent's. Recorded as NEEDS_HUMAN and carried to the user.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260806_bug_stack_absorb/060_s6_pr_less_issues.md`:
- Line 19: Update the fenced code block in the markdown document by adding the
text language tag to its opening fence, changing the untagged fence to a
text-tagged fence to satisfy MD040.
- Around line 16-38: Reconcile the failure totals in the plan before using them
to prioritize work: explain whether 152 represents raw log lines and whether the
suite rows overlap, then make the suite totals internally consistent with the
recoverable count of 76. Correct the “roughly three quarters” and “~78 failures”
statements to use the same non-overlapping failure unit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b0457c8e-9db8-4cb3-9c59-45b01cb3c02d
📒 Files selected for processing (1)
devlog/_plan/260806_bug_stack_absorb/060_s6_pr_less_issues.md
| 76 recoverable failures plus an unknown shard. Grouped by suite from 152 `(fail)` | ||
| lines: | ||
|
|
||
| ``` | ||
| 32 injectCodexConfig integration (Design B) | ||
| 30 Codex catalog sync hardening | ||
| 16 Codex catalog restore | ||
| 12 two real processes contend for one lock | ||
| 12 server same-target 429 retry (end-to-end) | ||
| 12 codex-journal | ||
| 6 Issue #702 expired forward replay state | ||
| ``` | ||
|
|
||
| **The finding that changes the plan:** roughly three quarters are one family — | ||
| Codex catalog and config *file writing*. The failing assertions are about atomic | ||
| publication and permission narrowing ("writes prepared bytes atomically with the | ||
| right...", "group-readable is narrowed back to owner-only", CAS txId conflicts), | ||
| which is where POSIX assumptions leak on Windows: `rename` over an existing file, | ||
| `chmod` bits, and advisory locking all behave differently. | ||
|
|
||
| So the recorded plan ("management/server fixtures first, then platform process | ||
| semantics") is superseded by evidence: fix the write substrate first and ~78 | ||
| failures should fall together rather than shard by shard. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reconcile the failure totals before relying on this prioritization.
The recoverable shard counts are 8 + 21 + 47 = 76. The suite rows at Lines 20-26 sum to 120, not 152. The first three rows sum to 78, which also exceeds the 76 recoverable failures. Clarify whether 152 counts raw log lines or whether the suite rows overlap. Then correct the derived “roughly three quarters” and “~78 failures” claims so they use one consistent unit.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/060_s6_pr_less_issues.md` around lines
16 - 38, Reconcile the failure totals in the plan before using them to
prioritize work: explain whether 152 represents raw log lines and whether the
suite rows overlap, then make the suite totals internally consistent with the
recoverable count of 76. Correct the “roughly three quarters” and “~78 failures”
statements to use the same non-overlapping failure unit.
| 76 recoverable failures plus an unknown shard. Grouped by suite from 152 `(fail)` | ||
| lines: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced block.
Change Line 19 from totext````` so the document passes markdownlint rule MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/060_s6_pr_less_issues.md` at line 19,
Update the fenced code block in the markdown document by adding the text
language tag to its opening fence, changing the untagged fence to a text-tagged
fence to satisfy MD040.
Source: Linters/SAST tools
Two loops circled a problem the campaign had invented. The user named the answer in one line: their PRs report maintainerCanModify=true, so push our completion commit onto their branch. Their commits stay, blame stays accurate, their PR merges, and attribution is the commit graph rather than a paragraph. No replacement PR, no close, no "absorbed with credit to" prose. #1036 landed that way. Four Agent59353 commits, then ours on top, pushed with a lease pinned to their head after re-verifying the remote. The change is bounded: live-transport records the bare names we actually advertised this request, the event state carries them, and both translate call sites convert only for names in that set. Their conversion logic is byte- unchanged. The stateless fallback passes through, since it has no state to consult and an unconverted call is recoverable while a wrongly converted one edits a file — disclosed in the PR comment as a judgment call open to disagreement. Ablation: restoring the name-only gate gives 21 pass / 1 fail, red on exactly the new collision test. #997 was not ours to push. The lease rejected it as stale: the author landed their own fix mid-flight, and theirs is stronger than what we staged — we pinned getConfigDir(), they assert the resolved usage.jsonl receives the row and the default location does not. Verified instead of overwritten, 10 pass with a 9/1 ablation on their own assertion. That is the third time in this campaign that acting on a stale head produced wasted or wrong work: updatedAt as an activity signal, a "failing test" comment posted eight minutes after #1068 merged with the fix, and a commit built against a head the author had already improved. The lease caught the third; the first two reached a contributor as a wrong public statement. The rule is now written down — re-fetch immediately before preparing a commit, not before pushing.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md`:
- Around line 18-27: Correct the commit attribution statement in the listed
commit history: reconcile the four commits attributed to Agent59353 with the
claim that only the last commit was attributed to us. Clarify whether Agent59353
is the maintainer identity or whether the statement refers only to the latest
push, and update the table or concluding statement so contributor attribution is
accurate.
- Around line 53-54: Update the push workflow guidance in “Re-fetch the
contributor head immediately before preparing a commit” to require a second
fetch immediately before pushing. Pin --force-with-lease to the OID from this
second fetch, while retaining the initial fetch before commit preparation.
- Line 17: Add the text language identifier to the commit-list fenced code block
in the document, changing its opening fence to use ```text while preserving the
existing plain-text contents and closing fence.
- Around line 39-49: Reconcile the PR status timeline between
050_deferral_record.rb and 070_push_on_top.md: add the exact timestamp for the
later `#997/`#1036 update, explicitly mark the earlier record superseded, and
ensure both plan records present one authoritative status for each PR.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1da0584e-1b6f-470d-9134-578c6ff82149
📒 Files selected for processing (1)
devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md
|
|
||
| ### #1036 — done | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the commit-list fence.
markdownlint reports MD040 at Line 17. Use text so the commit list remains plain text and the Markdown lint passes.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md` at line 17, Add the
text language identifier to the commit-list fenced code block in the document,
changing its opening fence to use ```text while preserving the existing
plain-text contents and closing fence.
Source: Linters/SAST tools
| b5e292928 bitkyc08-arch fix(cursor): gate structured-edit conversion on provenance, not tool name | ||
| 33d617601 Agent59353 fix(cursor): address CodeRabbit + Codex review feedback on #1017 | ||
| d06de0f9c Agent59353 test(cursor): cover native-exec mcpArgs structured edit translation | ||
| 500cd94f6 Agent59353 fix(cursor): never shadow an existing bare edit tool ... | ||
| f724063d3 Agent59353 fix(cursor): expose structured edit tools that convert to valid apply_patch calls (#1017) | ||
| ``` | ||
|
|
||
| Pushed with `--force-with-lease` pinned to their head, after re-verifying the | ||
| remote was unchanged. `gh pr view #1036` shows five commits with only the last | ||
| attributed to us. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the commit attribution statement.
Lines 19-22 list four commits by Agent59353, but Line 27 says only the last commit was attributed to us. Clarify whether Agent59353 is the maintainer identity or whether Line 27 refers only to the latest push. Then correct the table or the statement. This record claims that contributor attribution remains accurate.
#!/bin/bash
set -euo pipefail
gh pr view 1036 --json commits --jq '.commits[] | {oid, authors, messageHeadline}'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md` around lines 18 -
27, Correct the commit attribution statement in the listed commit history:
reconcile the four commits attributed to Agent59353 with the claim that only the
last commit was attributed to us. Clarify whether Agent59353 is the maintainer
identity or whether the statement refers only to the latest push, and update the
table or concluding statement so contributor attribution is accurate.
| ### #997 — not ours to push | ||
|
|
||
| The lease rejected our push as stale: the author had landed `3304d5c8` while we | ||
| worked, and **their version is stronger than the one we staged.** We pinned | ||
| `getConfigDir()`; they assert the resolved `usage.jsonl` receives the row *and* | ||
| that the default location lacks the request id — that survives a refactor of how | ||
| the path is resolved. | ||
|
|
||
| Verified rather than overwritten: 10 pass on their head, and ablating their | ||
| isolation gives 9 pass / 1 fail on their own assertion. Comment posted saying no | ||
| changes requested. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'devlog/_plan/260806_bug_stack_absorb/*.md' | sort
echo
echo "== filenames =="
for f in devlog/_plan/260806_bug_stack_absorb/050_deferral_record.md devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md; do
if [ -f "$f" ]; then
echo "--- lines 1-90 ---" $f
cat -n "$f" | sed -n '1,90p'
else
echo "missing $f"
fi
done
echo
echo "== status mentions in plan =="
rg -n "1036|997|complete|superseded|Author|author|Response|response|August|260806|update|updated|deferred" devlog/_plan/260806_bug_stack_absorb/*.md || trueRepository: lidge-jun/opencodex
Length of output: 1924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository state =="
pwd
git rev-parse --show-toplevel 2>/dev/null || true
git status --short 2>/dev/null || true
echo
echo "== plan files =="
git ls-files 'devlog/_plan/260806_bug_stack_absorb/*.md' | sort || true
echo
echo "== target files =="
for f in devlog/_plan/260806_bug_stack_absorb/050_deferral_record.md devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md; do
if [ -f "$f" ]; then
echo "--- lines 1-100 ---" $f
cat -n "$f" | sed -n '1,100p'
else
echo "missing $f"
fi
done
echo
echo "== status mentions in plan =="
rg -n "1036|997|complete|superseded|Author|author|Response|response|August|260806|update|updated|deferred" devlog/_plan/260806_bug_stack_absorb/*.md || true
echo
echo "== all status mentions in repo (focused) =="
rg -n "1036|997|complete|superseded|Author|author|Response|response|August|260806" devlog || trueRepository: lidge-jun/opencodex
Length of output: 1924
Reconcile the PR statuses across the plan records.
devlog/_plan/260806_bug_stack_absorb/050_deferral_record.rb says both PRs still lacked fixes and remained with their authors through 2026-08-09. devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md says #1036 is complete and #997 was updated by its author. Add the exact timestamp for the later update, mark the earlier entry superseded, or correct the statuses so operators have one authoritative timeline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md` around lines 39 -
49, Reconcile the PR status timeline between 050_deferral_record.rb and
070_push_on_top.md: add the exact timestamp for the later `#997/`#1036 update,
explicitly mark the earlier record superseded, and ensure both plan records
present one authoritative status for each PR.
| **Re-fetch the contributor head immediately before preparing a commit, not before | ||
| pushing.** Three times in this campaign, acting on a stale head produced wasted or |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require a final head check before pushing.
Line 53 contradicts Lines 71-72 by saying not to re-fetch before pushing. Re-fetch immediately before preparing the commit and again immediately before pushing. Pin --force-with-lease to the second fetched head OID. This prevents stale work from being pushed after a contributor updates the branch.
Proposed fix
-**Re-fetch the contributor head immediately before preparing a commit, not before pushing.**
+**Re-fetch the contributor head immediately before preparing a commit and again immediately before pushing.**📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Re-fetch the contributor head immediately before preparing a commit, not before | |
| pushing.** Three times in this campaign, acting on a stale head produced wasted or | |
| **Re-fetch the contributor head immediately before preparing a commit and again immediately before pushing.** Three times in this campaign, acting on a stale head produced wasted or |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260806_bug_stack_absorb/070_push_on_top.md` around lines 53 -
54, Update the push workflow guidance in “Re-fetch the contributor head
immediately before preparing a commit” to require a second fetch immediately
before pushing. Pin --force-with-lease to the OID from this second fetch, while
retaining the initial fetch before commit preparation.
2 participants
Summary
bug-labelled open items (12 issues, 13 PRs) as a devlog unit, graded by whether the proposed fix is sound rather than by whether the defect is real. No item was a false report.supports_reasoning_summariesis stripped (src/codex/catalog/parsing.ts:353) and defaulted false (:266); Codex gates construction of the entire Responsesreasoningobject on that flag, so the Desktop picker offers an effort the wire never carries.applyCatalogModelMetadataruns atsrc/codex/catalog/sync.ts:269, afternormalizeRoutedCatalogEntryat:267. Nothing asserted that ordering, so a future reorder would have silently disabled effort propagation for every opted-in routed provider with a green suite.Tests only — no
src/change. This deliberately does not flip the routed default totrue: claiming OpenAI-only summary delivery for arbitrary third-party providers is the same overclaim that blocks #1092 and #978 in the triage.Three tests rather than two, and the construction path is the point. The two existing summary tests call
buildCatalogEntries(null, ...), which takes the fallback branch (sync.ts:291-310) and never runs the strip — so they keep passing under the broken order. The opt-in and conservative tests passnativeTemplate()to reach the template branch; a third keeps thenullpath to pin the asymmetry between them.An adversarial plan audit caught the first draft of this work making exactly that mistake, which is why the ablation below was executed rather than asserted.
Verification
bun test tests/codex-catalog.test.ts— 122 pass / 0 fail (119 before).bun run typecheck— clean.bun run privacy:scan— passed.bun test tests/repo-hygiene.test.ts— 11 pass / 0 fail.bun test tests/reasoning-effort.test.ts— 39 pass / 0 fail.bun run lint:g.ui— clean.Red ablation, run on the landed tests. Moving the strip after the metadata restore in
src/codex/catalog/sync.ts:Only the template test goes red; the fallback staying green in the same run is what proves the template test carries the contract. Restore leaves an empty
src/diff and returns to 122 pass.Related
Does not close #1100 — the reporter's provider has no opt-in configured, so their symptom persists. The issue now carries the verified workaround (
modelSupportsReasoningSummaries: true, confirmed against their exact provider shape) and the reasoning for not changing the default.Reviews from this triage were posted to #1092, #978, #1036, #1068, #557, #997, #1093, #1095, #1085, #1111, #1056, #1047 and issues #1100, #1024, #1017, #994.
Checklist
260806_bug_grade_remediation/carries the roadmap, the grade matrix, and a per-phase ledger).privacy:scangreen; no credential or gate behavior touched).Summary by CodeRabbit
Documentation
Tests