Close fully redacted tool proposals - #1149
Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
GitHub Actions
Failure
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![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: 576d1dd0f8
ℹ️ 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".
| #[tokio::test] | ||
| async fn fully_suppressed_tool_arguments_are_non_executable() { |
There was a problem hiding this comment.
This scenario directly enforces the INV-035 credential boundary—whole-object credential suppression must not emit executable tool material—but neither its name nor its doc comment identifies that invariant, unlike the surrounding credential-boundary tests. Add the INV-035 tag so invariant-focused discovery can find this new enforcement.
AGENTS.md reference: AGENTS.md:L231-L235
Useful? React with 👍 / 👎.
| /// A CLI-redacted argument object is not an executable tool request. The | ||
| /// completed provider call closes as the existing unrepresentable-material | ||
| /// failure instead of entering the tool loop with sentinel JSON. | ||
| #[test] | ||
| fn fully_suppressed_tool_arguments_close_as_known_failure() { |
There was a problem hiding this comment.
This test establishes the INV-014 terminal-closeout behavior for a newly introduced class of unrepresentable tool material, yet neither the name nor the attached comment carries the invariant tag. Add INV-014 so the test is discoverable alongside the adjacent malformed-proposal closeout coverage.
AGENTS.md reference: AGENTS.md:L231-L235
Useful? React with 👍 / 👎.
| /// A tool call whose argument object the CLI credential boundary suppressed | ||
| /// as a whole. No executable proposal exists beyond the adapter boundary. | ||
| SuppressedToolCall, |
There was a problem hiding this comment.
When a structured completion contains one valid contract proposal plus an additional suppressed proposal, decode_structured ignores this new variant and sees exactly one matching value. RuntimeApprovalJudgeModel calls that decoder directly rather than going through classify_terminal, so it can accept an approval decision from a response that also contained an extra hidden tool call; before this change, the sentinel remained a ToolCall and was counted as a multiplicity failure. Treat any suppressed part as an explicit structured-decode failure.
Useful? React with 👍 / 👎.
| a tool argument object suppressed as a whole crosses the adapter as typed | ||
| non-executable material and closes the model call as a known failure, never as | ||
| sentinel JSON that could enter the tool loop; a private-key PEM block is |
There was a problem hiding this comment.
The implementation now emits SuppressedToolCall from both CLI adapters, but this added contract remains inside the Codex-only credential paragraph and the verification note explicitly names only Codex. Consequently the owning runtime specification does not describe the new Claude behavior even though model-runtime-claude-cli/src/event.rs implements it; extend this section or the Claude credential section to cover that adapter as well.
AGENTS.md reference: AGENTS.md:L46-L48
Useful? React with 👍 / 👎.
Rust coverage (report only)Report only. This measurement has no threshold, gates no merge, and
What this number does not measure
Per crate, least-covered first
25 files with the most uncovered lines
Measured at |
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
unrepresentable_tool_materialMeaningfully changed lines: 314 (excluding
Cargo.lock).Numeric-bound ceilings added: none.
Validation
Focused Codex, Claude, and provider-runtime tests pass. Workspace check, warning-denied Clippy, doctests, warning-denied rustdoc, metadata, and the required PostgreSQL integration suite pass.