-
Notifications
You must be signed in to change notification settings - Fork 0
feat(sharecli): land fr007 health-pool-status-csv + agent-call admission kernel #731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,211 @@ | ||
| //! Pure admission policy for agent-issued commands. | ||
|
|
||
| use std::cell::Cell; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::time::Duration; | ||
|
|
||
| const DEFAULT_DEADLINE: Duration = Duration::from_secs(30); | ||
|
|
||
| /// The reason an agent call must wait before it can run. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum PauseCode { | ||
| /// The command would search a host-level or otherwise unsafe root. | ||
| HazardousRoot, | ||
| /// The configured per-project call limit has been reached. | ||
| ProjectLimit, | ||
| /// The host has no thermal headroom for a new call. | ||
| Thermal, | ||
| /// The configured build-command slot limit has been reached. | ||
| BuildSlot, | ||
| } | ||
|
|
||
| /// An admitted command or a structured pause instruction. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct AgentCallDecision { | ||
| command: String, | ||
| pause_code: Option<PauseCode>, | ||
| resume_condition: Option<String>, | ||
| deadline: Duration, | ||
| } | ||
|
|
||
| impl AgentCallDecision { | ||
| /// The command after policy normalization. | ||
| pub fn command(&self) -> &str { | ||
| &self.command | ||
| } | ||
|
|
||
| /// The reason the command is paused, when admission was refused. | ||
| pub fn pause_code(&self) -> Option<PauseCode> { | ||
| self.pause_code | ||
| } | ||
|
|
||
| /// A human-readable condition that permits retrying a paused command. | ||
| pub fn resume_condition(&self) -> Option<&str> { | ||
| self.resume_condition.as_deref() | ||
| } | ||
|
|
||
| /// The bounded execution deadline for this decision. | ||
| pub fn deadline(&self) -> Duration { | ||
| self.deadline | ||
| } | ||
| } | ||
|
|
||
| /// Deterministic, local-only admission policy for agent calls. | ||
| #[derive(Debug)] | ||
| pub struct AgentCallPolicy { | ||
| project_root: PathBuf, | ||
| project_limit: usize, | ||
| admitted_calls: Cell<usize>, | ||
| thermal_headroom: bool, | ||
| build_slots: usize, | ||
| admitted_builds: Cell<usize>, | ||
| } | ||
|
|
||
| impl AgentCallPolicy { | ||
| /// Create a policy scoped to `project_root` with unrestricted local limits. | ||
| pub fn new(project_root: PathBuf) -> Self { | ||
| Self { | ||
| project_root, | ||
| project_limit: usize::MAX, | ||
| admitted_calls: Cell::new(0), | ||
| thermal_headroom: true, | ||
| build_slots: usize::MAX, | ||
| admitted_builds: Cell::new(0), | ||
| } | ||
| } | ||
|
|
||
| /// Set the maximum number of admitted calls for this project. | ||
| pub fn with_project_limit(mut self, limit: usize) -> Self { | ||
| self.project_limit = limit; | ||
| self | ||
| } | ||
|
|
||
| /// Set whether the host has headroom for another call. | ||
| pub fn with_thermal_headroom(mut self, available: bool) -> Self { | ||
| self.thermal_headroom = available; | ||
| self | ||
| } | ||
|
|
||
| /// Set the number of available build-command slots. | ||
| pub fn with_build_slots(mut self, slots: usize) -> Self { | ||
| self.build_slots = slots; | ||
| self | ||
| } | ||
|
|
||
| /// Normalize and admit a command, or return a pause decision. | ||
| pub fn admit(&self, command: &str) -> AgentCallDecision { | ||
| let normalized = self.normalize(command); | ||
|
|
||
| if targets_hazardous_root(&normalized) { | ||
| return self.paused( | ||
| normalized, | ||
| PauseCode::HazardousRoot, | ||
| "use a path inside the project root", | ||
| ); | ||
| } | ||
| if !self.thermal_headroom { | ||
| return self.paused(normalized, PauseCode::Thermal, "wait for thermal headroom"); | ||
| } | ||
| if self.admitted_calls.get() >= self.project_limit { | ||
| return self.paused( | ||
| normalized, | ||
| PauseCode::ProjectLimit, | ||
| "wait for an active project call to finish", | ||
| ); | ||
| } | ||
|
|
||
| let build = is_build_command(&normalized); | ||
| if build && self.admitted_builds.get() >= self.build_slots { | ||
| return self.paused( | ||
| normalized, | ||
| PauseCode::BuildSlot, | ||
| "wait for an available build slot", | ||
| ); | ||
| } | ||
|
|
||
| self.admitted_calls.set(self.admitted_calls.get().saturating_add(1)); | ||
| if build { | ||
| self.admitted_builds.set(self.admitted_builds.get().saturating_add(1)); | ||
| } | ||
|
Comment on lines
+126
to
+129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 |
||
|
|
||
| AgentCallDecision { | ||
| command: normalized, | ||
| pause_code: None, | ||
| resume_condition: None, | ||
| deadline: DEFAULT_DEADLINE, | ||
| } | ||
| } | ||
|
|
||
| fn normalize(&self, command: &str) -> String { | ||
| let words: Vec<_> = command.split_whitespace().collect(); | ||
| let Some(program) = words.first() else { | ||
| return command.to_owned(); | ||
| }; | ||
|
|
||
| if !matches!(*program, "grep" | "egrep") || !has_recursive_flag(&words[1..]) { | ||
| return command.to_owned(); | ||
| } | ||
|
|
||
| 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() | ||
| ) | ||
|
Comment on lines
+149
to
+158
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The recursive-grep rewrite parses shell input with Severity Level: Major
|
||
| } | ||
|
|
||
| fn paused( | ||
| &self, | ||
| command: String, | ||
| pause_code: PauseCode, | ||
| resume_condition: &str, | ||
| ) -> AgentCallDecision { | ||
| AgentCallDecision { | ||
| command, | ||
| pause_code: Some(pause_code), | ||
| resume_condition: Some(resume_condition.to_owned()), | ||
| deadline: DEFAULT_DEADLINE, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn has_recursive_flag(words: &[&str]) -> bool { | ||
| words.iter().any(|word| { | ||
| *word == "--recursive" | ||
| || word.starts_with('-') && word[1..].chars().any(|flag| matches!(flag, 'r' | 'R')) | ||
| }) | ||
| } | ||
|
|
||
| fn is_build_command(command: &str) -> bool { | ||
| matches!(command.split_whitespace().next(), Some("cargo" | "make" | "just")) | ||
| } | ||
|
|
||
| fn targets_hazardous_root(command: &str) -> bool { | ||
| command.split_whitespace().any(|word| is_hazardous_root(Path::new(word))) | ||
|
Comment on lines
+187
to
+188
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an agent uses normal shell quoting around a hazardous root, such as Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| 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)) | ||
|
Comment on lines
+187
to
+210
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The hazardous-root check only accepts exact path matches, so recursive searches of descendants such as 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 |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| use std::path::PathBuf; | ||
| use std::time::Duration; | ||
|
|
||
| use sharecli::agent_call_policy::{AgentCallPolicy, PauseCode}; | ||
|
|
||
| fn policy() -> AgentCallPolicy { | ||
| AgentCallPolicy::new(PathBuf::from("/workspace/project")) | ||
| } | ||
|
|
||
| #[test] | ||
| fn rewrites_recursive_grep_to_a_bounded_ripgrep_command() { | ||
| let decision = policy().admit("grep -R TODO ."); | ||
|
|
||
| assert_eq!( | ||
| decision.command(), | ||
| "rg --hidden --glob '!target' --glob '!node_modules' TODO /workspace/project" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn leaves_nonrecursive_grep_unchanged() { | ||
| let decision = policy().admit("grep TODO README.md"); | ||
|
|
||
| assert_eq!(decision.command(), "grep TODO README.md"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pauses_searches_targeting_a_hazardous_root() { | ||
| let decision = policy().admit("rg TODO /System"); | ||
|
|
||
| assert_eq!(decision.pause_code(), Some(PauseCode::HazardousRoot)); | ||
| assert!(decision.resume_condition().is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pauses_recursive_grep_targeting_filesystem_root() { | ||
| let decision = policy().admit("grep -R TODO /"); | ||
|
|
||
| assert_eq!(decision.pause_code(), Some(PauseCode::HazardousRoot)); | ||
| assert!(decision.resume_condition().is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pauses_when_the_project_concurrency_limit_is_reached() { | ||
| let policy = policy().with_project_limit(1); | ||
| let _first = policy.admit("rg TODO src"); | ||
| let decision = policy.admit("rg FIXME src"); | ||
|
|
||
| assert_eq!(decision.pause_code(), Some(PauseCode::ProjectLimit)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pauses_when_thermal_headroom_is_unavailable() { | ||
| let decision = policy().with_thermal_headroom(false).admit("rg TODO src"); | ||
|
|
||
| assert_eq!(decision.pause_code(), Some(PauseCode::Thermal)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pauses_builds_when_no_build_slot_is_available() { | ||
| let policy = policy().with_build_slots(1); | ||
| let _first = policy.admit("cargo test"); | ||
| let decision = policy.admit("cargo build"); | ||
|
|
||
| assert_eq!(decision.pause_code(), Some(PauseCode::BuildSlot)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn attaches_a_nonzero_deadline_to_admitted_calls() { | ||
| let decision = policy().admit("rg TODO src"); | ||
|
|
||
| assert!(decision.deadline() > Duration::ZERO); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
limitsuccessful calls, the policy will returnProjectLimit/BuildSlotforever even though no calls are active, making the limits lifetime quotas instead of active concurrency controls.Useful? React with 👍 / 👎.