Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
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. |
![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: 0ea81a06dc
ℹ️ 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".
| with _DIRECT.open(f"{_public_url(host, port)}/v1/local-session", timeout=1) as response: | ||
| proof = json.loads(response.read(256)) | ||
| if response.status == 200 and isinstance(proof, dict) and proof.get("identity") == identity: | ||
| return session | ||
| except (OSError, ValueError): |
There was a problem hiding this comment.
When the recorded port is occupied by an HTTP listener that closes early or advertises a longer body, response.read(256) can raise http.client.IncompleteRead (an HTTPException, not an OSError or ValueError). That exception escapes _live_session, so a second launcher crashes instead of treating the stale/unrelated session as inactive and starting normally; catch the HTTP read/protocol exceptions along with the existing connection errors.
Useful? React with 👍 / 👎.
What changed
The local capture API no longer treats any live pid plus any accepting loopback port as the recorded server. It proves ownership through a launcher-only
/v1/local-sessionresponse containing the SHA-256 hash of the session's reviewer token. The probe sends no credential and compares the returned hash with the value derived from the session file.The fix is in
backend/scripts/local_capture_api.py:159-193and lines 317-360. The launcher also takes anflockonsession.lockbefore readingsession.jsonand holds it through the server lifetime. A second launch exits before it can bind or overwrite the session.This PR is stacked on
gpt-burst/1022z-bs-281-rest.Why
A stale session file could block startup after pid reuse and an unrelated listener on the recorded port. Two launches could both pass the check, bind different ports, and race while writing credentials.
Verification
With the final regression tests and the original launcher from
832314ca,env -u NODE_ENV OMP_NUM_THREADS=1 uv run pytest -q tests/test_local_capture_api.py::test_reused_pid_and_unrelated_listener_allow_startup tests/test_local_capture_api.py::test_live_session_requires_the_running_servers_identity tests/test_local_capture_api.py::test_concurrent_launches_allow_exactly_one tests/test_local_capture_api.py::test_second_launch_refuses_a_live_sessionreturned 3 failed, 1 passed. The stale listener and wrong token were accepted, and both launches remained alive. The existing real-running-server refusal test was the one that already passed.After the fix, from
backend/,env -u NODE_ENV OMP_NUM_THREADS=1 uv run pytest -q tests/test_local_capture_api.py server/api/tests/test_auth_none.pypassed: 29 tests. Ruff check, format check, andgit diff --checkpassed.Risk
The identity endpoint is added only to the local launcher app. It is not registered in the shared API factory. The lock file remains beside
session.json;flockreleases it when the process exits, including a crash.Checklist
Generated with Claude Code
Zephyrus full-suite run (Oct 23)
Command:
cd backend && uv sync --locked --extra api && env -u NODE_ENV OMP_NUM_THREADS=1 uv run pytest -qResult: 1,532 passed, 16 skipped, 14 errors, 4 failed in 18m56s.
The 14 errors are the known pre-existing
tests/test_capture_cold_restart.pyload_receiptimport errors. The 4 failures are the known expiredKNOWN_FAILpin inserver/eval/tests/test_gates.py. No other failures occurred, so no load-sensitivity rerun was needed.