Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0326537a92
ℹ️ 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".
| ) | ||
| .await? | ||
| { | ||
| ReceiptMatch::Replay(response) => return Ok((*response).replayed()), |
There was a problem hiding this comment.
When an authority.status response is committed while active but lost in transit, the broker contract directs callers to retry the identical request bytes. If the operator fences the generation before that retry, this branch still returns the stored Active result, allowing the retrying caller to admit a new spawn or prompt after the terminal fence. Treat status as a current lifecycle read rather than replaying its old receipt, or invalidate such receipts during fencing.
AGENTS.md reference: AGENTS.md:L11-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f80b51c. Fencing and prior status-receipt invalidation now occur in the same BEGIN IMMEDIATE transaction. The regression verifies that an identical lost pre-fence request returns a fresh fenced verdict before any later replay.
| sqlx::query( | ||
| "INSERT INTO broker_receipts | ||
| (credential_hash, request_id, request_digest, response_json) | ||
| VALUES (?, ?, ?, ?)", | ||
| ) |
There was a problem hiding this comment.
Every fresh authenticated request, including authority.status and unsupported actions, permanently inserts a receipt with no retention or quota. Status polling with unique request IDs therefore grows SQLite continuously, and even a fenced bearer remains authenticated and can rapidly fill the host disk with new IDs. Skip durable receipts for operations without side effects, or add a bounded retention/rate-limit strategy that preserves deduplication only where it is needed.
AGENTS.md reference: AGENTS.md:L20-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f80b51c. Read-only/unsupported receipts are capped at the latest 64 per credential, with a regression proving the bound and recent replay. The README explicitly reserves a separate durable effect ledger for the later effect-host slice.
| #[cfg(unix)] | ||
| fn require_private_directory(metadata: &std::fs::Metadata) -> Result<(), StoreError> { | ||
| use std::os::unix::fs::PermissionsExt as _; | ||
| if metadata.permissions().mode() & 0o077 != 0 { |
There was a problem hiding this comment.
When a privileged broker is pointed at a 0700 directory owned by another UID, this check accepts it because it examines only mode bits. That directory owner can precreate or replace the SQLite database with a valid attacker-controlled authority and known credential digest, defeating the claimed host-owned boundary. Check the directory and existing database UID against the broker's effective UID in addition to their modes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f80b51c. Unix state directories and existing database files now require both owner-only mode bits and metadata UID equality with geteuid(); the Windows-only dead variant is cfg-gated.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f80b51cdce
ℹ️ 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".
| fn is_owned_by_effective_user(metadata: &std::fs::Metadata) -> bool { | ||
| use std::os::unix::fs::MetadataExt as _; | ||
| // SAFETY: geteuid(2) has no preconditions and only reads process identity. | ||
| metadata.uid() == unsafe { libc::geteuid() } |
There was a problem hiding this comment.
On every Unix build, this introduces an unsafe block into production code even though the repository explicitly prohibits unsafe code. Use a safe OS abstraction for the effective UID (and remove the direct libc call) so the new broker host conforms to the repository's safety boundary.
AGENTS.md reference: AGENTS.md:L158-L160
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 37d6474. The direct libc dependency and unsafe block are removed; Unix ownership now uses the safe nix::unistd::geteuid().as_raw() API. Local host tests and workspace clippy pass with no unsafe in the new crate.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37d64743bd
ℹ️ 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".
| fn require_private_directory(_metadata: &std::fs::Metadata) -> Result<(), StoreError> { | ||
| Ok(()) |
There was a problem hiding this comment.
On Windows, this unconditionally accepts the state directory, while the corresponding file check only verifies is_file(). If the configured directory grants another local user write access, that user can precreate or replace the SQLite database with a canonical authority and a credential digest they know, defeating the host-owned authority boundary. Validate Windows ownership/ACLs for both paths, or fail startup on Windows until that protection is implemented.
Useful? React with 👍 / 👎.
Summary
buzz-broker-hostfor durable managed-ACPauthority.statusRelated issue
None found. This is the first durable host slice following the managed-ACP client/runtime authority contract.
Testing
cargo test -p buzz-broker-host— 11 passedcargo clippy -p buzz-broker-host --all-targets -- -D warningscargo test -p buzz-sdk broker::— 44 passedcargo test -p buzz-broker-client— 13 passedcargo clippy --workspace --all-targets -- -D warningsjust cicompleted Rust/workspace, Desktop/Web, Mobile analysis, and 1,831 Mobile tests; two unrelated Mobile timing tests failed in the parallel suite, then both passed when rerun individually by exact test nameReview corrections
activereplay