fix(codex): log unattended Codex write service-home refusals - #6100
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughWhen Codex write admission is refused because service-home ownership cannot be established, sync logs a fixed explanation without exposing the private path. The refusal result and return behavior remain unchanged. A regression test and documentation cover the refusal. ChangesService-home refusal logging
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The refusal remains available through POST /api/sync, while the new log avoids exposing private paths. No merge-blocking issue is identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new refusal log does not include private paths, and the service-home check still blocks catalog and configuration writes. The API continues to return the detailed reason; its access controls were not independently verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 44 / 80이 PR은 사람이 지켜보지 않을 때 Codex 쓰기가 거절되면, 그 사실을 로그에 한 줄 남겨요. 베이스는 프록시가 설치해 둔 서비스 홈의 주인이라고 증명하지 못하면, 이제는 거절 권한이 structure/config.md:273 - "경로 없는 로그 한 줄"이 항상 남는 것처럼 읽혀요. 로그를 넘긴 호출만 찍어요. src/cli/dispatch.ts:490 - 메인테이너의 판단이 필요한 지점 로그를 비우는 주기 갱신이, 시작 때 한 번 찍은 뒤에 같은 거부를 다시 만나도 조용한 채로 둘지예요. 시작 로그를 못 본 프로세스는 다시 조용해져요.
너의 추천 거절을 유지하고 경로는 로그에서 빼는 방향은 맞아요. 머지해도 돼요. 그 전에 #5782는 닫지 마세요. 진단 한 조각만 여기 있어요. Windows 종료와 ChatGPT 브리지는 그 이슈에 남겨 두세요. types.ts와 config.ts를 나누는 중복 PR은 이 변경과 겹치지 않아요. 베이스는 이 댓글은 grok-bot이 작성했습니다 |
9147a29 to
8f52270
Compare
A service-home admission refusal in syncModelsToCodex returned without any log line. Startup then marked /readyz failed with no evidence and `ocx sync` only said the sync did not complete. Log one line naming the authority and pointing to POST /api/sync for the specific reason. The admission message can contain private home and service-definition paths, so it stays out of the log. Reimplements the diagnostic slice of #5782 without the raw message. Co-authored-by: tcflying <77509083+tcflying@users.noreply.github.com>
8f52270 to
75f863c
Compare
Summary
When an unattended proxy cannot prove it owns the installed service homes,
syncModelsToCodexrefuses Codex writes on theservice-homeauthority. Until now that refusal left nothing behind. Startup marked/readyzfailed with no log line, andocx syncprinted only "Codex sync did not complete". #5782 reports a nine-day catalog-sync stall that went unnoticed for this reason.The sync now logs one line naming the authority, saying that catalog sync and Codex config injection were skipped, and pointing to
POST /api/sync, which already returns the concrete reason in its 409 body. The admission message stays out of the log because it can name private home and service-definition paths. Callers that passlog = null, such as the periodic catalog auto-refresh, remain silent, so drift healing does not repeat the line. Behavior for other refusal authorities is unchanged.structure/config.mdnow describes this exception to the "concrete messages on stderr" rule.This reimplements the diagnostic slice of #5782 by @tcflying without the raw message. The rest of #5782 (Windows durable stop and ChatGPT bridge) stays under review there.
Co-authored-by: tcflying 77509083+tcflying@users.noreply.github.com
Verification
tests/codex-integration/codex-sync-api.test.ts: failed before the change (no log line), passes after. It also asserts that nothing is refreshed or injected and that the logged line does not contain the refusal's path.bun test tests/codex-integration/codex-sync-api.test.ts tests/codex-integration/codex-admission.test.ts: 30 pass / 0 fail.bun test tests/codex-integration/codex-composed-acceptance.test.ts: 8 pass / 0 fail.bun run typecheck,bun run privacy:scan,bun run structure:check,git diff --check origin/dev...HEAD: exit 0.structure/config.md, which is fixed in this PR.dev6d64ea26(two Swift tray files), then onto468b954c(fix(images): use managed Pool with proxy admission bearer, scope first #6097, image Pool admission). Neither touches this diff. After the rebase,codex-sync-api.test.ts(18 pass) andstructure:checkwere rerun. A localtest:changedattempt was stopped externally (SIGTERM) while it waited for another lane's test lock, so it produced no result.test:changednot run for this PR: seven release lanes share this machine, and broad coverage is left to CI.Checklist