fix(combo): force defaults over declared reasoning sentinels - #5990
RHODIZSECURITY wants to merge 3 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughForce mode now recognizes all declared caller reasoning efforts, including ChangesReasoning effort override
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Force mode overrides declared caller sentinels with a ranked configured effort when target capabilities permit it. When capabilities are unknown, the request follows the existing conservative behavior; no material merge risk is established. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
리뷰 · 우선순위 66 / 80콤보는 여러 모델을 하나의 이름으로 묶어 두는 길입니다. Meta Muse는 라인 - 메인테이너의 판단이 필요한 지점 강도 목록을 모를 때도 너의 추천
이 댓글은 grok-bot이 작성했습니다 |
fcde467 to
6a5ecca
Compare
|
@coderabbitai review |
|
8a09f1e to
6e92953
Compare
Ingwannu
added
the
maintainer-sponsored
|
Maintainer sponsorship added after exact-head source review found the combo reasoning-sentinel override narrow and internally consistent. This starts executable CI; it is not merge approval. I will make the final review decision from the resulting exact-head checks. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Approved on exact head 6e929536 after source review. The change is confined to valid declared none/minimal sentinels, while the concrete target ladder, malformed input, fallback mode, and unknown capability remain conservative. React Doctor is green; merge remains conditional on queued exact-head Cross-platform CI.
|
Exact approved head |
…ch 9E) (#5998) Carry the owner's Remote Link, restart, desktop supervision and Codex routing fixes onto dev in their original order, then carry RHODIZSECURITY's combo reasoning fix as one attributed commit. | PR | Change | Author | | --- | --- | --- | | #5970 | Turn a standalone computer into a Child from its dashboard; keep Codex on its local loopback URL and protect the linked data plane. | lidge-jun | | #5973 | Reconnect the Child's SSH tunnel after sleep, outages and crashes. | lidge-jun | | #5972 | Heal opencodex-owned Codex routing left on a dead loopback endpoint, with ownership and race gates. | lidge-jun | | #5971 | Keep a proxy on the configured port through a Child restart. | lidge-jun | | #5974 | Let the desktop app supervise runtime restarts and unexpected exits. | lidge-jun | | #5990 | Apply forced combo defaults over declared none/minimal reasoning sentinels. | RHODIZSECURITY | The carry keeps the 9D one-use sibling handoff and passes link status, cached key and tunnel gate through the Child listener. Separate integration commits bound the port-conflict regression test and keep carried files below the file-size guard, including newer dev's layout entries. The five owner commits retain JUN's authorship; the #5990 squash retains RHODIZSECURITY's commit identity and noreply co-author trailer. An independent review found four integration defects. Each repair is a separate Codex-authored commit: | Finding | Commit | Repair | | --- | --- | --- | | Linked requests could fetch without a connected tunnel. | 6a29f7b | Require a positive supervisor connected verdict before every fetch; missing, failed and stopped supervision return 503 without forwarding key or body. | | IPv4 and IPv6 destinations on one port shared one probe/streak key. | 8903998 | Probe and track each hostname and port separately; a live endpoint blocks healing and an address change starts a fresh dead-probe streak. | | A same-port route change could pass the locked write guard. | f62e43b | Abort when admitted config bytes or the complete destination set changes under the lock, then require fresh probes. | | Orphan reaping could KILL a reused PID. | e0ef426 | Record the process start identity and revalidate argv, start time and orphan status before TERM and before KILL; legacy records lacking start identity never authorize a signal. | A second review confirmed those four repairs and found two remaining blockers: | Finding | Commit | Repair | | --- | --- | --- | | A competing local listener received the readiness key and private relay traffic before SSH bound the tunnel port. | 39f246a | Require an exclusive local LISTEN socket owner PID matching the SSH child before every keyed probe and relay admission; adopted processes also need matching pidfile argv and start time. Unknown scans fail closed. | | Newer dev mappings made the merge result exceed the layout file-size guard. | 3bb2ab6, 46ee24f | Merge origin/dev at a91568e, then compact formatting while retaining every explicit mapping. | A third review confirmed the competing-listener and layout repairs, then found three lookup defects: | Finding | Commit | Repair | | --- | --- | --- | | Minimal Linux lacks lsof/netstat and never proves the SSH listener. | 9c66251 | Use tool-independent async /proc/net/tcp{,6} inode lookup, checking the expected SSH PID's fd symlinks first. | | A foreign ::1 listener shares the numeric port with the owned IPv4 forward. | 9c66251 | Match only the exact 127.0.0.1 address and port on Linux, macOS and Windows. | | Synchronous owner scans block Bun on every relayed fetch. | b5e565d | Use bounded async lookups and a one-second positive proof keyed by port, SSH PID, start identity and tunnel generation; re-prove after restart. | The branch also merged current dev at 35f267d in 51747c9. The merged test registries retain both lanes' mappings and the management contract retains Kiro's account projection and Child join. Current dev through `2a3cfa5abe` was merged again in `5856179cd1` without conflicts. It brings #6012's macOS plugin ACL fix and dev's Kiro projection test clock correction; `scripts/test-layout/layout.json` stays at 1,997 lines. The merge changes no link/relay or server-management-auth files. A fourth review found that a one-second proof cache could survive a local port takeover, and that an adopted PID's start identity was only checked at adoption: | Finding | Commit | Repair | | --- | --- | --- | | Cached ownership authorized the next keyed probe or relay after a port takeover. | b8142ad | Every keyed probe and every relayed fetch now obtains a fresh bounded asynchronous socket-owner proof; concurrent admissions do not share a cached success. | | A reused adopted PID retained its old trusted start identity. | b8142ad | Re-read current argv and start time on every adopted admission, invalidate trust and mark the link failed on mismatch. | | The TCP listener can change after the check and before connect. | bee1613 | Record this pre-existing residual race and a private Unix-domain SSH forward as future hardening in the link structure contract. | A fifth review found that transient unreadable adopted identity was treated like a confirmed replacement: | Finding | Commit | Repair | | --- | --- | --- | | One null or timed-out identity read permanently disabled a live adopted link. | 0eefebf | Return an explicit unknown verdict; deny only the current keyed admission and retry on the next check without discarding the adopted record. | | A confirmed changed identity left the old adopted PID blocking recovery. | 0eefebf | Release the stale adoption and pidfile without signalling that PID; the next supervisor tick starts its own SSH tunnel. | Windows CI follow-up: `88fbb539af` samples the relay hold clock once per attempt. The initial reconnect wait now receives the full 15-second budget even when the wall clock ticks during admission; later retries still subtract elapsed time. Windows teardown follow-up: `0171b4856c` makes `server.stop(true)` await any timed-out `icacls.exe` child still reaping after config-directory hardening settles. The stop promise now marks the actual handle-release boundary before a caller removes the home. Security review: Child join still refuses Tailscale identity, a non-standalone role and a mismatched live port before SSH. Linked data routes retain the Host/Origin gate, committed-key fingerprint, caller-credential stripping and inbound byte cap. The relay now sends no key or request without positive tunnel supervision, and the supervisor obtains a fresh bounded asynchronous exact-IPv4 owner proof before every keyed probe and relay fetch; adopted processes also have their current argv and start time checked each time. An unknown read refuses only that admission; a confirmed mismatch releases the adopted PID without signalling it. Desktop supervision stays bound to its live parent, and dashboard Stop is refused before teardown while CLI/tray Stop remains available. Routing self-heal writes only owned loopback routing after all distinct endpoints were proven dead and the locked bytes were rechecked. The existing home-bound stop proof and sibling Desktop-write gate remain intact.   Co-authored-by: RHODIZSECURITY <180237049+RHODIZSECURITY@users.noreply.github.com>
3 participants
Addresses #5989; the issue was auto-closed before the reproduction was expanded, and the full reproduction is now documented there.
defaultEffortMode: forcecurrently overrides only rankedlow..ultracaller efforts becauseconcreteComboRequestBody()gates the override withisCodexReasoningEffort().noneandminimalare valid declared request sentinels, so they must also be overridden when the combo explicitly forces a default. Otherwise a forced-high combo can leaknoneto a downstream model that rejects it.This patch uses
isDeclaredReasoningEffort()for the caller-side force check while continuing to require the configured combo default itself to be a rankedisCodexReasoningEffort()value.Validation on current
dev:bun x tsc --noEmit: PASSreasoning.effort=nonechanged from HTTP 400 to HTTP 200; concrete Muse child wire effort ishighReview readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Required local validation passed; commands, results, and any full-suite exception are documented.
I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
noneorminimal. When the target’s reasoning capability is unknown, a caller’snonesetting is preserved. This makes reasoning selection consistent across declared caller settings while retaining conservative behavior when target capabilities cannot be determined.