feat(agents): enforce trusted heartbeat source preflight - #5602
feat(agents): enforce trusted heartbeat source preflight#5602Greg-Moskalenko wants to merge 6 commits into
Conversation
Co-authored-by: Greg Moskalenko <greg1mosk@gmail.com> Signed-off-by: Greg Moskalenko <greg1mosk@gmail.com>
![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: 939b524723
ℹ️ 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".
| use std::os::unix::ffi::OsStrExt; | ||
|
|
||
| type Acl = *mut c_void; | ||
| unsafe extern "C" { |
There was a problem hiding this comment.
This introduces an unsafe extern block and several unsafe calls for ACL inspection, despite the repository’s explicit prohibition on any unsafe code. Replace this implementation with a safe wrapper or command-based ACL inspection so the macOS heartbeat verification path complies with the project’s required safety invariant.
AGENTS.md reference: AGENTS.md:L114-L116
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8589050c9. The macOS verifier no longer uses FFI or any unsafe code. It now performs fail-closed ACL inspection through /bin/ls -lde with a cleared environment, bounded output, strict status/stderr/UTF-8 checks, and an ACL regression test. The full just ci gate passes on this exact head.
| }; | ||
|
|
||
| // ── Pre-Phase 2: validate provider config BEFORE any side effects ──────── | ||
| input.validate_heartbeat_preflight(&pubkey)?; |
There was a problem hiding this comment.
When heartbeatPreflight is supplied during creation, the target pubkey is generated immediately above this call and is never exposed to the caller, but policy validation requires target_agent_pubkey to equal that newly generated value. A caller therefore cannot prepare a valid policy in advance, and retrying after failure generates another key, making the new create-time designation path effectively unusable; allocate or expose the key before accepting the policy, or require designation through the subsequent update path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8589050c9. Create requests now reject a heartbeat designation before any side effect, the TypeScript create surface no longer exposes that field, and designation is required through update_managed_agent after creation returns the agent pubkey. The new boundary test and the full just ci gate pass on this exact head.
Require heartbeat designation through the post-create update path once the agent pubkey exists. Replace the macOS ACL FFI with fail-closed system ACL inspection so the trusted harness path contains no unsafe Rust. Co-authored-by: Greg Moskalenko <greg1mosk@gmail.com> Signed-off-by: Greg Moskalenko <greg1mosk@gmail.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Current head 8589050 addresses both automated review findings and passes the full local |
Co-authored-by: Greg Moskalenko <greg1mosk@gmail.com> Signed-off-by: Greg Moskalenko <greg1mosk@gmail.com>
Bind local macOS trust to the exact broker identifier and certificate leaf fingerprint while preserving Developer ID TeamIdentifier verification. Co-authored-by: Greg Moskalenko <greg1mosk@gmail.com> Signed-off-by: Greg Moskalenko <greg1mosk@gmail.com>
Resolve the managed-agent creation and model-update overlap while retaining the owner-bound preflight gate. Co-authored-by: Greg Moskalenko <greg1mosk@gmail.com> Signed-off-by: Greg Moskalenko <greg1mosk@gmail.com>
Co-authored-by: Greg Moskalenko <greg1mosk@gmail.com> Signed-off-by: Greg Moskalenko <greg1mosk@gmail.com>
|
@codex review Current head is rebased through upstream |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b1ceed9c1
ℹ️ 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".
| .and_then(|value| nostr::Keys::parse(value).ok()) | ||
| .map(|keys| keys.public_key()); | ||
| let resolved_owner = agent_public_key.as_ref().and_then(|agent_pubkey| { |
There was a problem hiding this comment.
When a helper subcommand runs with matching BUZZ_ACP_REQUIRED_AGENT_OWNER and BUZZ_ACP_AGENT_OWNER but no BUZZ_PRIVATE_KEY, agent_public_key is None, so this and_then skips the configured-owner fallback entirely and models, auth-methods, and authenticate all fail the latch. Those discovery/auth helpers do not otherwise require an agent key, and this also breaks Desktop helper probes inheriting an operator-configured latch; consult configured_owner independently and require the private key only when verifying an auth tag.
Useful? React with 👍 / 👎.
1 participant
Summary
The bug is structural: prompt text can ask an agent to call every source, but it cannot prove those calls happened before the agent reports a no-op. This moves that decision to a pre-agent runtime gate and makes incomplete source coverage suppress the model turn.
Key safety properties
Testing
env -u BUZZ_ACP_LAZY_POOL ./bin/just cipassed on exact head2b1ceed9c1e449a75ba4ed5c9148b38a20ae4285git diff --checkpassedmainatdf9e773a13f17a270fd6531fc74948b8059d58c3; GitHub reports it mergeableManual testing / screenshots
No new end-user UI is introduced, so screenshots are not applicable. A production acceptance run still requires the separately deployed gateway, source credentials/helpers, and fresh signed source receipts; the feature deliberately fails closed until those are present.
Deferred follow-up