Repository navigation
fix(zcode): export reasoning configuration for thought-capable models - #4153
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe ZCode configuration export now includes reasoning metadata for models with supported reasoning efforts. It filters the ChangesZCode reasoning export
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to ZCode exports now expose supported reasoning levels and valid defaults so the Thought Level control can be shown correctly. The relevant output, filtering, and omission behavior are covered, with no current merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
리뷰 · 우선순위 58 / 80이 PR은 고치는 방식은 이미 라인 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
ccb2f8e to
021bbed
Compare
ZCode's Thought Level picker reads on-disk reasoning.variants. The exporter only wrote name/modalities/limit, so catalog models that already support reasoning_effort looked like plain-text models. Drop the Codex none sentinel, keep catalog ultra, and set defaultVariant only when it remains in the emitted ladder.
021bbed to
abf35fa
Compare
|
Rebased onto current This change is ready to merge from the author side. I do not have write access on |
…n#4141 unblock Three more rows are settled. lidge-jun#4153 merged as 2ce5f38 and closed lidge-jun#4147, as the contributor's own commit so authorship reaches his graph. lidge-jun#4160 merged as 8a5cfd3 and closed lidge-jun#3859. And PR lidge-jun#4152 landed as 9ba04b6, which frees lidge-jun#4141 to start. Two operational facts are written down because they were easy to get wrong. A fork pull request does not start repository CI by itself, so the thin check list on lidge-jun#4153 was action_required rather than a passing PR. And the force-push that unstacked lidge-jun#4160 left an earlier run cancelled at the same SHA, whose aggregate job then reported failure; that is the third cancelled run this round that could have been read as a verdict. Also records the one real defect the Lane B audit found. The free-only filter counts the group header from the unfiltered rows, so the header claims more models than the list shows. The empty state gets it right; the header does not. Assigned to Lane B. NOT RUN: local test suite, typecheck, build, lint. Remote CI is the gate.
…xport-reasoning Maintainer integration on dev under the MAINTAINERS.md policy that lets a maintainer with maintain or admin access land a PR on dev without a second approval, recording the decision and the exact-head CI evidence. Landed as the contributor's own commit so the contribution shows on his graph. Authorship is richardfeiliu-a11y throughout; nothing was reimplemented or carried. What decided the review is that the contributor did not guess the schema. He read a live ~/.zcode/v2/config.json and the shipped parser inside ZCode.app and reported that the on-disk shape is enabled + variants + defaultVariant, with levels being the post-parse in-memory object. This repository corroborates that from inside the tree: src/integrations/ownership-policy.ts already treats models.*.reasoning as a 3.8.1 key with enabled/variants, and tests/clients/integrations-writer.test.ts carries a fixture in exactly that shape. Vocabulary is right for this client. The none rung is dropped because it is Codex's omit-sentinel rather than a picker option. The ultra rung is kept because ZCode forwards the selected variant as reasoning_effort and ultra is a real tier, so omp's no-ultra vocabulary is omp-specific. And defaultVariant is emitted only when it survives the filter. A fork pull request does not start repository CI on its own. Cross-platform CI and React Doctor sat at action_required until a maintainer approved them, which is why the earlier check list showed only hygiene, target, label and resolve-pr. Both were approved and then concluded on their own. Exact-head CI at 7267df1, verified by exit code: gh run view 34411681339 --exit-status -> 0 Cross-platform CI gh run view 34411681800 --exit-status -> 0 React Doctor gh run view 34411874833 --exit-status -> 0 Enforce PR target branch gh run view 34411874830 --exit-status -> 0 PR hygiene None cancelled at that SHA. Independent read-only review recorded at devlog/_plan/260910_post249_round2/_research/_audit_pr4153.md, verdict PASS with no blocker. It confirmed the only byte-pinned ZCode export string in the tree is the facade golden this PR updates, and that no other snapshot, fixture or docs sample now disagrees. NOT RUN locally: bun run test, bun run typecheck, bun run build, bun run lint:gui, bun run privacy:scan, bun install. The maintainer set a no-local-suite constraint for this round and remote CI is the only gate.
…n#4141 unblock Three more rows are settled. lidge-jun#4153 merged as 0c79314 and closed lidge-jun#4147, as the contributor's own commit so authorship reaches his graph. lidge-jun#4160 merged as b27aa50 and closed lidge-jun#3859. And PR lidge-jun#4152 landed as ba8e8f8, which frees lidge-jun#4141 to start. Two operational facts are written down because they were easy to get wrong. A fork pull request does not start repository CI by itself, so the thin check list on lidge-jun#4153 was action_required rather than a passing PR. And the force-push that unstacked lidge-jun#4160 left an earlier run cancelled at the same SHA, whose aggregate job then reported failure; that is the third cancelled run this round that could have been read as a verdict. Also records the one real defect the Lane B audit found. The free-only filter counts the group header from the unfiltered rows, so the header claims more models than the list shows. The empty state gets it right; the header does not. Assigned to Lane B. NOT RUN: local test suite, typecheck, build, lint. Remote CI is the gate.
2 participants
Summary
ocx zcode/ ZCode integration sync omitted the on-diskreasoningblock, so ZCode hid the Thought Level picker even for catalog models that already supportreasoning_efforton the wire.buildZcodeClientConfignow emitsreasoning.enabled,variants, anddefaultVariantwhen the model has a selectable ladder. That is the schema ZCode 3.x persists on disk and later parses into in-memorylevels/defaultLevel.noneis dropped because it is a Codex omit-sentinel, not a ZCode picker option. Catalogultrais kept.defaultVariantis set only when it survives that filter.Closes #4147
Verification
bun test tests/providers/zcode-client.test.ts tests/config/client-config-export.test.ts tests/config/client-config-export-new-clients.test.ts— 115 pass, 0 failbun run typecheck— exit 0bun run privacy:scan— Privacy scan passedbun run test— 22388 pass, 80 skip, 0 fail, exit 0Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
"none"option is excluded from selectable reasoning variants while other declared levels remain available.