runtime: broker activation-ping handshake fix + cache-stable bundle-root predicate#12
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the agent-collab package to version 3.3.0, transitioning from a single-file launch path to a closed standalone provider bundle model (manifest schema 2, native contract 3, broker transport 2). It introduces a new runtime_bundle.py module for secure bundle verification and adds a distinct broker-only gemini/governance contract with artifact-bound proof validation. Additionally, it updates various skills, build scripts, and tests to align with these architectural changes. During the review, a high-severity Time-of-Check to Time-of-Use (TOCTOU) directory replacement vulnerability was identified in verify_bundle_tree within runtime_bundle.py, where a malicious actor could swap the verified bundle directory during verification; a fix was suggested to perform a lexical root.lstat() check at the end of the verification process.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try: | ||
| root_after = os.fstat(root_descriptor) | ||
| final_names = os.listdir(root_descriptor) | ||
| except OSError: | ||
| raise BundleContractError("runtime bundle root changed") from None | ||
| if ( | ||
| _stat_identity(root_after) != _stat_identity(root_before) | ||
| or tuple(sorted(final_names, key=lambda name: name.encode("utf-8"))) | ||
| != expected_names | ||
| ): | ||
| _raise("runtime bundle root changed during verification") |
There was a problem hiding this comment.
TOCTOU Directory Replacement Vulnerability
During the bundle verification process, verify_bundle_tree performs an initial check on the directory's identity using root.lstat() and compares it to root_before (obtained via os.fstat(root_descriptor)). However, at the end of the verification, it only checks root_after = os.fstat(root_descriptor).
Because root_descriptor is an open file descriptor, any fstat on it will continue to refer to the original directory even if the directory at the path root was renamed or replaced with a malicious directory during the verification (e.g., during the external inspector call). Since the subsequent execution in resolve_runtime is path-based (using bundle_path), this allows a race condition where an attacker can replace the verified bundle directory with a malicious one, bypassing the signature and hash checks.
To prevent this, perform a lexical root.lstat() at the end of the verification and ensure its identity still matches root_before.
| try: | |
| root_after = os.fstat(root_descriptor) | |
| final_names = os.listdir(root_descriptor) | |
| except OSError: | |
| raise BundleContractError("runtime bundle root changed") from None | |
| if ( | |
| _stat_identity(root_after) != _stat_identity(root_before) | |
| or tuple(sorted(final_names, key=lambda name: name.encode("utf-8"))) | |
| != expected_names | |
| ): | |
| _raise("runtime bundle root changed during verification") | |
| try: | |
| root_after = os.fstat(root_descriptor) | |
| lexical_root_after = root.lstat() | |
| final_names = os.listdir(root_descriptor) | |
| except OSError: | |
| raise BundleContractError("runtime bundle root changed") from None | |
| if ( | |
| _stat_identity(root_after) != _stat_identity(root_before) | |
| or _stat_identity(lexical_root_after) != _stat_identity(root_before) | |
| or tuple(sorted(final_names, key=lambda name: name.encode("utf-8"))) | |
| != expected_names | |
| ): | |
| _raise("runtime bundle root changed during verification") |
![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: f5a11a2831
ℹ️ 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".
| "reviewer_model": GEMINI_GOVERNANCE_MODEL, | ||
| "reviewer_family": "google", | ||
| "selected_display": GEMINI_GOVERNANCE_DISPLAY, | ||
| "effective_effort": "high", |
There was a problem hiding this comment.
When callers use the documented/accepted gemini/governance row with effort: "xhigh" (accepted in host_policy.py and the README), this validation still hard-codes the broker proof to "high". That means a broker that actually honors the sealed xhigh request and reports effective_effort: "xhigh" is rejected, while a downgraded high-effort run can pass for an xhigh request; the proof should be compared with the sealed row effort or the row should be normalized before sealing.
Useful? React with 👍 / 👎.
…state parse) The activation ping never succeeded on a live launchd socket-activated broker: - _broker_ping did a bare connect-and-close. The peer was gone before the broker read LOCAL_PEERPID, so peer-identity observation raised ENOTCONN -> peer_error, and the broker blocked in accept() for its full timeout. Hold the connection open and half-close the write side (SHUT_WR) so the broker observes a live peer and its frame read hits end-of-stream promptly. - _broker_process_idle counted the per-endpoint 'state = active' socket lines as job-lifecycle states, so the terminal-state check never held and the ping timed out. Exclude socket 'active' states from the job-state set.
…workspace crux #1) Byte-identical parity sync of scripts/_agent_collab/runtime_bundle.py from the workspace canonical (agent-collab-workspace crux #1). The INSTALLED plugin's runtime_client loads this loose PLUGIN_ROOT/runtime_bundle.py at resolve time, so without this sync the loaded host coordinator would still reject the host-normalized 0755 bundle root (integrity_error / zero contracts) even after the workspace fix. verify_bundle_tree's bundle-ROOT directory predicate now requires owner read+execute, no group/other write, and no setuid/setgid/sticky (accepting 0o500 build-store AND 0o755 host-normalized, rejecting 0o775/0o757/0o777/special-bit). Exact per-member modes, closed membership, digest, signature, and identity checks unchanged. Test updated to assert a group-writable (0o775) root still fails closed. runtime_bundle 12/12, runtime_client 66/66. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sumitake
force-pushed
the
dev/claude/broker-ping-activation-fix
branch
from
f5a11a2 to
1376da0
Compare
The activation liveness knock budgeted 5.0s in two places: the ping socket's hold-open recv timeout and _wait_for_broker_exit's terminal-state poll. A freshly published bundle (new digest directory, non-page-cached files) cold-starts in 6-12s on first exec (codesign validation + interpreter init for the ~20MB standalone bundle; unified-log evidence: first-activation runs of 6302ms-12111ms, warm ~1.4s). The short hold recreated the ENOTCONN peer-gone race that a906df9 fixed -- the client's recv gave up and closed before the broker observed the peer -- so the FIRST install-broker after every fresh publish spuriously failed ("activation ping failed") and fell through to the restore path, which skips the ping/readback and silently masked the miscalibration as "update failed; previous version restored". Introduce BROKER_COLD_START_TIMEOUT_SECONDS = 30.0 and use it for both the ping hold-open and the exit-wait deadline. Success-path latency is unchanged: recv unblocks the moment the broker closes the connection and the exit poll returns on the first terminal-state observation; the budget only bounds the failure case. Tests: +2 (ping hold-open uses the cold-start constant and is >= 15s; simulated-clock exit-wait outlasts a 12s cold start). Focused suite 68/68; full discover 295/295. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Branch corrected + 3rd commit reviewed (2026-07-15): #10 squash-merged, so this branch was rebased onto the new main ( f5a11a2 distinct-family review (xAI, managed Grok grok-4.5): VERDICT: PROCEED / CONFIDENCE: M — no unbounded-wait/leak/busy-loop; 6 non-blocking notes (sequential ~30s+30s budgets not one shared envelope; longer-timeout-not-a-cold-start-signal but acceptable given 30s≫12s observed; soft test floor assertGreaterEqual 15.0). All three commits now converged. |
1 participant
The two commits the currently ACTIVE dev-channel runtime build (
b2ce1ba27e4a-d59ae466c950) consumes — the fixes that made activation succeed. Merging this (after #10, on which it is stacked) makes pluginmainable to rebuild the shipped runtime.a906df9— broker activation ping handshake fix (peer-id race + socket-state parse). Workspace twin reviewed in the #1886 arc (Antigravity APPROVE/HIGH, msg b8f94ddf, γ workspace #1928).d59ae46— cache-stable bundle-root predicate (parity with workspace crux release: publish clean agent-collab distribution #1: host plugin-cache normalizes the bundle root to 0755, so exact-0500 root checks fail on the LOADED copy; members stay exact 0500 + digest-pinned).Merge convergence record (operator-directed takeover, Codex idle):
🤖 Generated with Claude Code