feat(sharecli): land fr007 health-pool-status-csv + agent-call admission kernel - #731
Conversation
… admission kernel Sharded from feat/ac-007-82-health-pool-status-csv + feat/agent-call-policy-20260812 onto current main. Drops the sharecli-dirty-20260801 preservation noise (those state files are not for merging — kept in branch for historical reference). Includes: - fr007 health/pool/status --csv export (AC-007.82) - agent-call admission kernel + tests
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:L
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughSummaryThis PR adds:
The changes appear focused and suitable for merge. Must FixNo blocking issues identified from the provided change summary. Should FixRun and confirm:
Consider
Approve / Request ChangesApprove, subject to successful formatting, clippy, and workspace test checks. WalkthroughChangesAgent call admission policy
Specification and CSV compatibility updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| self.admitted_calls.set(self.admitted_calls.get().saturating_add(1)); | ||
| if build { | ||
| self.admitted_builds.set(self.admitted_builds.get().saturating_add(1)); | ||
| } |
There was a problem hiding this comment.
Suggestion: The admission counters are incremented for every admitted call, but there is no release method or RAII guard to decrement them when the command finishes. After project_limit calls, or after build_slots builds, every subsequent admission remains paused permanently even though the resume condition says to wait for an active call or build to finish. Return a permit whose Drop implementation releases the corresponding counters, matching the existing build-slot lifecycle. [missing cleanup]
Severity Level: Critical 🚨
- ❌ Limited project policies eventually reject every subsequent call.
- ❌ Limited build pools permanently reject later build commands.
- ⚠️ Pause messages promise recovery that cannot occur.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/agent_call_policy.rs
**Line:** 126:129
**Comment:**
*Missing Cleanup: The admission counters are incremented for every admitted call, but there is no release method or RAII guard to decrement them when the command finishes. After `project_limit` calls, or after `build_slots` builds, every subsequent admission remains paused permanently even though the resume condition says to wait for an active call or build to finish. Return a permit whose `Drop` implementation releases the corresponding counters, matching the existing build-slot lifecycle.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let mut positional = words[1..].iter().copied().filter(|word| !word.starts_with('-')); | ||
| let pattern = positional.next().unwrap_or(""); | ||
| let target = match positional.next() { | ||
| Some(".") | None => self.project_root.as_path(), | ||
| Some(target) => Path::new(target), | ||
| }; | ||
| format!( | ||
| "rg --hidden --glob '!target' --glob '!node_modules' {pattern} {}", | ||
| target.display() | ||
| ) |
There was a problem hiding this comment.
Suggestion: The recursive-grep rewrite parses shell input with split_whitespace, then reconstructs the pattern and target without quoting and discards every dash-prefixed argument. A command such as grep -R "foo bar" . is rewritten with only "foo as the pattern and bar" as the target, while options such as --include or --exclude silently disappear. This changes the search semantics and can produce an invalid or unintended command; preserve shell argument boundaries and translate supported grep options explicitly with proper escaping. [api mismatch]
Severity Level: Major ⚠️
- ❌ Agent searches with spaces query incorrect patterns.
- ❌ Include and exclude filters disappear during normalization.
- ⚠️ Search results can include unintended files.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/agent_call_policy.rs
**Line:** 149:158
**Comment:**
*Api Mismatch: The recursive-grep rewrite parses shell input with `split_whitespace`, then reconstructs the pattern and target without quoting and discards every dash-prefixed argument. A command such as `grep -R "foo bar" .` is rewritten with only `"foo` as the pattern and `bar"` as the target, while options such as `--include` or `--exclude` silently disappear. This changes the search semantics and can produce an invalid or unintended command; preserve shell argument boundaries and translate supported grep options explicitly with proper escaping.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| fn targets_hazardous_root(command: &str) -> bool { | ||
| command.split_whitespace().any(|word| is_hazardous_root(Path::new(word))) | ||
| } | ||
|
|
||
| fn is_hazardous_root(path: &Path) -> bool { | ||
| const ROOTS: &[&str] = &[ | ||
| "/", | ||
| "/Applications", | ||
| "/Library", | ||
| "/System", | ||
| "/Users", | ||
| "/bin", | ||
| "/dev", | ||
| "/etc", | ||
| "/opt", | ||
| "/private", | ||
| "/proc", | ||
| "/sys", | ||
| "/tmp", | ||
| "/usr", | ||
| "/var", | ||
| "/Volumes", | ||
| ]; | ||
| ROOTS.iter().any(|root| path == Path::new(root)) |
There was a problem hiding this comment.
Suggestion: The hazardous-root check only accepts exact path matches, so recursive searches of descendants such as /System/Secrets, /etc/shadow, or /proc/1 bypass the policy even though they remain host-level roots. Equivalent paths containing .. or redundant components also bypass it. Normalize or canonicalize the candidate path and reject paths that are the hazardous root or descendants of one. [security]
Severity Level: Critical 🚨
- ❌ Host-level descendant searches bypass hazardous-root admission.
- ⚠️ Sensitive system files may be searched by agent commands.
- ⚠️ Path aliases weaken the intended project boundary.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/agent_call_policy.rs
**Line:** 187:210
**Comment:**
*Security: The hazardous-root check only accepts exact path matches, so recursive searches of descendants such as `/System/Secrets`, `/etc/shadow`, or `/proc/1` bypass the policy even though they remain host-level roots. Equivalent paths containing `..` or redundant components also bypass it. Normalize or canonicalize the candidate path and reject paths that are the hazardous root or descendants of one.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| /// FR-007 / AC-007.82 — health --csv rejects --watch (one-shot export only). | ||
| #[test] | ||
| #[serial_test::serial] | ||
| fn fr007_health_csv_rejects_watch() { | ||
| let out = bin() | ||
| .args(["health", "--csv", "--watch", "1"]) | ||
| .output() | ||
| .expect("spawn sharecli health --csv --watch 1"); | ||
| assert!( | ||
| !out.status.success(), | ||
| "health --csv --watch MUST fail loudly (AC-007.82)" | ||
| ); | ||
| let stderr = String::from_utf8_lossy(&out.stderr); | ||
| assert!( | ||
| stderr.contains("csv") && stderr.contains("watch"), | ||
| "error MUST mention csv/watch incompatibility (AC-007.82); stderr: {stderr}" | ||
| ); |
There was a problem hiding this comment.
Suggestion: This test introduces a contradictory contract: the existing fr007_operator_csv_watch integration test requires health --csv --watch 1 to emit multiple CSV frames, and the current command implementation enters that watch loop, but this newly added test requires the same invocation to fail. The test suite cannot satisfy both requirements; either remove this rejection test or update the command and the existing watch contract together according to the intended API. [api mismatch]
Severity Level: Major ⚠️
- ❌ CI cannot satisfy both health CSV contracts.
- ❌ Existing watch behavior conflicts with new one-shot requirements.
- ⚠️ Operators receive inconsistent documented CLI semantics.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/fr007_health_pool_status_csv.rs
**Line:** 230:246
**Comment:**
*Api Mismatch: This test introduces a contradictory contract: the existing `fr007_operator_csv_watch` integration test requires `health --csv --watch 1` to emit multiple CSV frames, and the current command implementation enters that watch loop, but this newly added test requires the same invocation to fail. The test suite cannot satisfy both requirements; either remove this rejection test or update the command and the existing watch contract together according to the intended API.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![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: e8fb436b63
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| assert!( | ||
| !out.status.success(), | ||
| "health --csv --watch MUST fail loudly (AC-007.82)" |
There was a problem hiding this comment.
This new test now requires health --csv --watch 1 to fail, but the command implementation still accepts that combination by entering the CSV watch branch in src/commands/mod.rs::health, and the existing tests/fr007_operator_csv_watch.rs::fr007_health_csv_watch_stderr_silent_and_envelope still asserts the same command streams CSV frames successfully. As written, the added assertion conflicts with existing behavior and will fail the required test gate until the CLI implementation and old watch contract/tests are updated together.
AGENTS.md reference: AGENTS.md:L134-L140
Useful? React with 👍 / 👎.
| fn targets_hazardous_root(command: &str) -> bool { | ||
| command.split_whitespace().any(|word| is_hazardous_root(Path::new(word))) |
There was a problem hiding this comment.
When an agent uses normal shell quoting around a hazardous root, such as rg TODO "/" or grep -R TODO "/", this whitespace split leaves the quotes in the token, so Path::new("\"/\"") does not match / and the decision is admitted; the returned command would then be interpreted by the shell as a search of the filesystem root. Parse the command as shell argv, or otherwise unquote/lexically normalize paths, before applying the hazardous-root check.
Useful? React with 👍 / 👎.
| self.admitted_calls.set(self.admitted_calls.get().saturating_add(1)); | ||
| if build { | ||
| self.admitted_builds.set(self.admitted_builds.get().saturating_add(1)); |
There was a problem hiding this comment.
When a finite project or build limit is configured and the same policy instance is reused, every admitted call permanently increments these counters, but there is no handle or release method that decrements them after the command finishes. After limit successful calls, the policy will return ProjectLimit/BuildSlot forever even though no calls are active, making the limits lifetime quotas instead of active concurrency controls.
Useful? React with 👍 / 👎.
Quality Gate Report✅ Unit Tests: PASSED |
…733) * fix(test): repair fr007 health-csv test for StatusJson::log_location e2ceacd (#731) added log_location to StatusJson and updated seven of the eight fr007 test literals but missed tests/fr007_health_pool_status_csv.rs, which omits the required field (E0063) and was never rustfmt-formatted. Main's own ci-gate run on e2ceacd fails; every PR against it inherits the break. - Add `log_location: None` to the StatusJson literal. - cargo fmt --all reflow of the file. Verified: cargo fmt --all -- --check (0 diffs) and cargo check --workspace --all-targets both pass. * ci: re-trigger checks after stuck coverage runner * fix(test): remove stale health --csv --watch rejection test fr007_health_csv_rejects_watch asserted that `sharecli health --csv --watch 1` must fail fast (a misreading of AC-007.82), but AC-007.89 defines health --csv --watch as a supported multi-frame watch mode and both tests/fr007_operator_csv_watch.rs and the envelope-parity suite exercise it as working behavior; the CLI implements the watch loop. The stale test spawned the infinite watch process and blocked on output() forever, hanging the fr007 test binary and stalling the Coverage lane's cargo llvm-cov run (reproduced locally: the suite hung on this test with and without instrumentation; CI coverage ran 2h45m without completing). Remove the test; the AC-007.89 watch envelope behavior remains covered by fr007_operator_csv_watch.rs / fr007_operator_envelope_parity_suite.rs.
* fix(test): repair fr007 health-csv test for StatusJson::log_location e2ceacd (#731) added log_location to StatusJson and updated seven of the eight fr007 test literals but missed tests/fr007_health_pool_status_csv.rs, which omits the required field (E0063) and was never rustfmt-formatted. Main's own ci-gate run on e2ceacd fails; every PR against it inherits the break. - Add `log_location: None` to the StatusJson literal. - cargo fmt --all reflow of the file. Verified: cargo fmt --all -- --check (0 diffs) and cargo check --workspace --all-targets both pass. * ci: re-trigger checks after stuck coverage runner * fix(test): remove stale health --csv --watch rejection test fr007_health_csv_rejects_watch asserted that `sharecli health --csv --watch 1` must fail fast (a misreading of AC-007.82), but AC-007.89 defines health --csv --watch as a supported multi-frame watch mode and both tests/fr007_operator_csv_watch.rs and the envelope-parity suite exercise it as working behavior; the CLI implements the watch loop. The stale test spawned the infinite watch process and blocked on output() forever, hanging the fr007 test binary and stalling the Coverage lane's cargo llvm-cov run (reproduced locally: the suite hung on this test with and without instrumentation; CI coverage ran 2h45m without completing). Remove the test; the AC-007.89 watch envelope behavior remains covered by fr007_operator_csv_watch.rs / fr007_operator_envelope_parity_suite.rs.
User description
Sharded content from
feat/ac-007-82-health-pool-status-csv+feat/agent-call-policy-20260812onto current main. Therecovery/sharecli-dirty-20260801preservation state is excluded (those files were for historical snapshot only, not for merge).Includes:
CodeAnt-AI Description
Add guarded agent command admission and enforce one-shot CSV exports
What Changed
grepsearches are redirected to bounded project-scopedrgcommands that skip common dependency and build directories.--watchand report the CSV/watch conflict clearly; CSV output remains one-shot with gate, host, pool, and status companion records.Impact
✅ Safer agent-issued searches✅ Fewer resource-overloaded agent calls✅ Clearer CSV/watch command errors💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.