feat(approval-gate): config rules inline + infinite holds - #283
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 33 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe approval-gate worker replaces its external YAML policy check and cron-based ChangesInline Permissions Rules and Sweep Removal
Sequence Diagram(s)sequenceDiagram
participant Caller
participant gate as approval::gate
participant perms as cfg.permissions()
participant pending as pending state
Caller->>gate: pre_trigger(function_id, args)
gate->>gate: is_human_only()?
alt human-only approval::* functions
gate-->>Caller: HookOutput::Deny (fail-closed)
else non-human functions
gate->>perms: check(function_id, args, mode)
alt Decision::Allow
perms-->>gate: Allow { rule_id }
gate-->>Caller: HookOutput::Continue
else Decision::Deny
perms-->>gate: Deny { rule_id, matched_constraint }
gate-->>Caller: HookOutput::Deny (permissions envelope)
else Decision::NeedsApproval
perms-->>gate: NeedsApproval
gate->>pending: get/put PendingApprovalRecord
gate-->>Caller: HookOutput::Hold (unit variant)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
approval-gate/src/decision.rs (1)
15-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring mentions
configuration::*but implementation excludes it.The docstring at lines 15-18 states that both
approval::*andconfiguration::*are operator surfaces that should be human-only. However, the implementation at line 20 only checks for theapproval::prefix, and the test at line 108 explicitly asserts thatconfiguration::setis not human-only.If the narrowing is intentional (configuration functions no longer need human-only protection), update the docstring to match:
Proposed docstring fix
-/// `approval::*` and `configuration::*` are operator surfaces — an agent -/// that could call them would approve its own calls (spec § Human-only -/// defense). Prefix match deliberately broadens the prior art's -/// six-function list. +/// `approval::*` functions are operator surfaces — an agent that could +/// call them would approve its own calls (spec § Human-only defense). +/// Prefix match deliberately broadens the prior art's explicit list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@approval-gate/src/decision.rs` around lines 15 - 21, The docstring for the is_human_only function mentions that both `approval::*` and `configuration::*` are operator surfaces requiring human-only protection, but the actual implementation only checks for the `approval::` prefix. Since the test confirms that `configuration::set` should not be treated as human-only, update the docstring to remove the reference to `configuration::*` and clarify that only functions with the `approval::` prefix are considered human-only. This will align the documentation with the actual implementation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@approval-gate/architecture/integration.md`:
- Line 24: The function IDs in the documentation on line 24 use underscores
(on_session_deleted, on_turn_completed) but the registered wire IDs use hyphens
(approval::on-session-deleted, approval::on-turn-completed). Update the table
entry to replace the underscore versions with the hyphenated versions to match
the actual registered IDs shown in the first column, ensuring consistency and
preventing confusion for integrators.
In `@approval-gate/src/gate_config.rs`:
- Around line 66-74: The parse_permissions function currently treats an explicit
empty rules array the same as a missing rules key, both falling back to
default_permissions(). To fix this, remove the second condition that checks if
specs.is_empty() and returns built_in, since this prevents operators from
intentionally disabling built-in permissions with an explicit empty rules array.
Instead, only return default_permissions() when the rules key is not present,
and allow the specs from parse_rules_from_config to be used directly even when
empty, so that an explicit "rules": [] configuration actually results in empty
permissions rather than silently reverting to defaults.
In `@approval-gate/src/permissions/compile.rs`:
- Around line 39-42: The CompileError::Rule instances are hardcoding the index
field to 0 instead of using the actual rule index value, which causes
inconsistent error diagnostics with duplicated rule prefixes in the message.
Replace the hardcoded index value of 0 with the actual rule index from the
function parameters or loop iteration in all three locations where
CompileError::Rule is used (around lines 39-42, 52-58, and 78-81), ensuring each
error reports the correct index of the rule that failed to compile.
In `@approval-gate/src/permissions/mod.rs`:
- Around line 94-102: The current implementation uses filter_map to parse
constraints from the args object, which silently discards any constraints that
fail to parse. This is a security issue because invalid constraints in allow
rules can be dropped, potentially allowing unauthorized calls. Instead of using
filter_map, collect the results from parse_constraint into a collection and
check for parse errors. If any constraint fails to parse, the entire rule
parsing should fail and return an error rather than silently dropping the
invalid constraint. This strict parsing approach should be applied consistently
to all constraint parsing locations where args are processed (including the
section mentioned at lines 122-127).
In `@approval-gate/src/testkit/engine.rs`:
- Around line 124-126: In the timeout path where Instant::now() exceeds the
deadline and child.kill() is called, you need to fully clean up resources before
returning None. After calling child.kill(), add a call to wait() on the child
process to ensure it's properly reaped and not left as a zombie. Additionally,
before returning None, ensure the temporary directory (likely self.work_dir or
similar) is explicitly cleaned up to remove test artifacts. This ensures no
zombie processes or leftover files are left behind when a test engine boot times
out.
- Around line 134-141: The issue is that the require_engine function uses
OnceCell<Option<Engine>> which permanently caches a None result when
spawn_engine() fails, preventing retries on transient failures. Modify the
require_engine function to only cache successful Engine instances in the
OnceCell. Change the logic so that failed initializations from spawn_engine()
are not stored as None but instead allow the initialization to be retried on
subsequent calls. Consider changing the OnceCell to store only the Engine
directly rather than Option<Engine>, or modify the initialization logic to only
call get_or_init when spawn_engine() succeeds, allowing transient boot failures
to be retried in future calls.
---
Outside diff comments:
In `@approval-gate/src/decision.rs`:
- Around line 15-21: The docstring for the is_human_only function mentions that
both `approval::*` and `configuration::*` are operator surfaces requiring
human-only protection, but the actual implementation only checks for the
`approval::` prefix. Since the test confirms that `configuration::set` should
not be treated as human-only, update the docstring to remove the reference to
`configuration::*` and clarify that only functions with the `approval::` prefix
are considered human-only. This will align the documentation with the actual
implementation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13a322f7-02ea-442a-abbb-4bec3a30354b
⛔ Files ignored due to path filters (1)
approval-gate/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
approval-gate/Cargo.tomlapproval-gate/README.mdapproval-gate/architecture/README.mdapproval-gate/architecture/integration.mdapproval-gate/architecture/internals.mdapproval-gate/config.yamlapproval-gate/src/config.rsapproval-gate/src/decision.rsapproval-gate/src/error.rsapproval-gate/src/events.rsapproval-gate/src/functions/gate.rsapproval-gate/src/functions/get_pending.rsapproval-gate/src/functions/list_pending.rsapproval-gate/src/functions/mod.rsapproval-gate/src/functions/on_config_change.rsapproval-gate/src/functions/on_session_deleted.rsapproval-gate/src/functions/on_turn_completed.rsapproval-gate/src/functions/purge.rsapproval-gate/src/functions/remove_always_allow.rsapproval-gate/src/functions/resolve.rsapproval-gate/src/functions/sweep.rsapproval-gate/src/gate_config.rsapproval-gate/src/lib.rsapproval-gate/src/main.rsapproval-gate/src/manifest.rsapproval-gate/src/pending.rsapproval-gate/src/permissions/compile.rsapproval-gate/src/permissions/default_rules.rsapproval-gate/src/permissions/mod.rsapproval-gate/src/permissions/types.rsapproval-gate/src/policy.rsapproval-gate/src/settings.rsapproval-gate/src/testkit/engine.rsapproval-gate/src/testkit/mod.rsapproval-gate/src/types.rsapproval-gate/tests/golden/schemas/approval.get-pending.jsonapproval-gate/tests/golden/schemas/approval.list-pending.jsonapproval-gate/tests/golden/schemas/approval.pending-created.jsonapproval-gate/tests/golden/schemas/approval.pending-resolved.jsonapproval-gate/tests/golden/schemas/approval.sweep.jsonapproval-gate/tests/integration.rsapproval-gate/tests/schemas.rs
💤 Files with no reviewable changes (16)
- approval-gate/src/functions/on_session_deleted.rs
- approval-gate/src/functions/on_turn_completed.rs
- approval-gate/tests/golden/schemas/approval.pending-resolved.json
- approval-gate/tests/golden/schemas/approval.sweep.json
- approval-gate/src/policy.rs
- approval-gate/tests/golden/schemas/approval.pending-created.json
- approval-gate/src/functions/get_pending.rs
- approval-gate/src/functions/sweep.rs
- approval-gate/tests/golden/schemas/approval.list-pending.json
- approval-gate/src/functions/list_pending.rs
- approval-gate/src/functions/mod.rs
- approval-gate/src/config.rs
- approval-gate/src/manifest.rs
- approval-gate/src/events.rs
- approval-gate/tests/integration.rs
- approval-gate/tests/golden/schemas/approval.get-pending.json
Comment on lines
+66
to
+74
| fn parse_permissions(value: &Value) -> Permissions { | ||
| let built_in = default_permissions(); | ||
| let Some(rules_val) = value.get("rules") else { | ||
| return built_in; | ||
| }; | ||
| let specs = parse_rules_from_config(rules_val); | ||
| if specs.is_empty() { | ||
| return built_in; | ||
| } |
There was a problem hiding this comment.
Explicit empty rules cannot disable built-in permissions.
Line 72 treats an explicit empty array ("rules": []) the same as “no rules provided” and falls back to default_permissions(). That prevents operators from enforcing a strict “no pre-allow rules” posture and can silently widen access versus intended config.
🔧 Suggested fix
fn parse_permissions(value: &Value) -> Permissions {
let built_in = default_permissions();
let Some(rules_val) = value.get("rules") else {
return built_in;
};
+ let Some(raw_rules) = rules_val.as_array() else {
+ return built_in;
+ };
+ if raw_rules.is_empty() {
+ return Permissions::empty();
+ }
let specs = parse_rules_from_config(rules_val);
if specs.is_empty() {
return built_in;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn parse_permissions(value: &Value) -> Permissions { | |
| let built_in = default_permissions(); | |
| let Some(rules_val) = value.get("rules") else { | |
| return built_in; | |
| }; | |
| let specs = parse_rules_from_config(rules_val); | |
| if specs.is_empty() { | |
| return built_in; | |
| } | |
| fn parse_permissions(value: &Value) -> Permissions { | |
| let built_in = default_permissions(); | |
| let Some(rules_val) = value.get("rules") else { | |
| return built_in; | |
| }; | |
| let Some(raw_rules) = rules_val.as_array() else { | |
| return built_in; | |
| }; | |
| if raw_rules.is_empty() { | |
| return Permissions::empty(); | |
| } | |
| let specs = parse_rules_from_config(rules_val); | |
| if specs.is_empty() { | |
| return built_in; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@approval-gate/src/gate_config.rs` around lines 66 - 74, The parse_permissions
function currently treats an explicit empty rules array the same as a missing
rules key, both falling back to default_permissions(). To fix this, remove the
second condition that checks if specs.is_empty() and returns built_in, since
this prevents operators from intentionally disabling built-in permissions with
an explicit empty rules array. Instead, only return default_permissions() when
the rules key is not present, and allow the specs from parse_rules_from_config
to be used directly even when empty, so that an explicit "rules": []
configuration actually results in empty permissions rather than silently
reverting to defaults.
Comment on lines
+39
to
+42
| Regex::new(&re).map_err(|e| CompileError::Rule { | ||
| index: 0, | ||
| message: format!("invalid glob {pattern:?}: {e}"), | ||
| }) |
There was a problem hiding this comment.
Fix incorrect rule index propagation in compile errors.
CompileError::Rule.index is hardcoded to 0 and then the real index is spliced into the message string, producing inconsistent diagnostics (e.g., duplicated rule prefixes). Use the real rule index in the structured field instead of message rewriting.
Suggested fix
-fn compile_function_glob(pattern: &str) -> Result<Regex, CompileError> {
+fn compile_function_glob(pattern: &str, index: usize) -> Result<Regex, CompileError> {
...
- Regex::new(&re).map_err(|e| CompileError::Rule {
- index: 0,
+ Regex::new(&re).map_err(|e| CompileError::Rule {
+ index,
message: format!("invalid glob {pattern:?}: {e}"),
})
}
...
- let glob = compile_function_glob(pattern).map_err(|mut e| {
- let CompileError::Rule {
- index: _,
- ref mut message,
- } = &mut e;
- *message = format!("rule {index}: {message}");
- e
- })?;
+ let glob = compile_function_glob(pattern, index)?;Also applies to: 52-58, 78-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@approval-gate/src/permissions/compile.rs` around lines 39 - 42, The
CompileError::Rule instances are hardcoding the index field to 0 instead of
using the actual rule index value, which causes inconsistent error diagnostics
with duplicated rule prefixes in the message. Replace the hardcoded index value
of 0 with the actual rule index from the function parameters or loop iteration
in all three locations where CompileError::Rule is used (around lines 39-42,
52-58, and 78-81), ensuring each error reports the correct index of the rule
that failed to compile.
Comment on lines
+94
to
+102
| let args = obj | ||
| .get("args") | ||
| .and_then(Value::as_object) | ||
| .map(|map| { | ||
| map.iter() | ||
| .filter_map(|(field, c)| parse_constraint(c).map(|spec| (field.clone(), spec))) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Do not silently drop invalid constraints/rules during config parsing.
Current parsing broadens policy on malformed input (filter_map drops bad constraints/rules). For structured allow rules, this can unintentionally allow calls that should require approval. Parse structured args strictly (fail the whole rule when any constraint is invalid) and surface a warning/error path.
Suggested strict-parse adjustment
- let args = obj
- .get("args")
- .and_then(Value::as_object)
- .map(|map| {
- map.iter()
- .filter_map(|(field, c)| parse_constraint(c).map(|spec| (field.clone(), spec)))
- .collect()
- })
- .unwrap_or_default();
+ let args = match obj.get("args") {
+ None => Vec::new(),
+ Some(v) => {
+ let map = v.as_object()?;
+ map.iter()
+ .map(|(field, c)| parse_constraint(c).map(|spec| (field.clone(), spec)))
+ .collect::<Option<Vec<_>>>()?
+ }
+ };Also applies to: 122-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@approval-gate/src/permissions/mod.rs` around lines 94 - 102, The current
implementation uses filter_map to parse constraints from the args object, which
silently discards any constraints that fail to parse. This is a security issue
because invalid constraints in allow rules can be dropped, potentially allowing
unauthorized calls. Instead of using filter_map, collect the results from
parse_constraint into a collection and check for parse errors. If any constraint
fails to parse, the entire rule parsing should fail and return an error rather
than silently dropping the invalid constraint. This strict parsing approach
should be applied consistently to all constraint parsing locations where args
are processed (including the section mentioned at lines 122-127).
Move permission evaluation into the approval-gate configuration entry so harness turns no longer depend on policy::check_permissions, and drop sweep/timeout so holds persist until human resolve or session/turn purge.
Scope built-in defaults to denying approval::* only, persist rules via initial_value and backfill on boot so the console Configuration editor is pre-filled, and hold all other calls until operators extend rules.
ytallo
force-pushed
the
feat/approval-gate-config-rules-infinite-holds
branch
from
June 17, 2026 17:18
7399f72 to
cbefa00
Compare
Contributor
skill-check — worker0 verified, 22 skipped (no docs/).
Four for four. Nicely done. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
approval-gate/src/permissions/compile.rs (1)
208-212: 💤 Low valueUnreachable
elsebranch — dead code.When
constraintsis non-empty, the loop always setsmatched = Some(...)on the first iteration (line 205). If all constraints pass,matchedis guaranteed to beSomeafter the loop, making theelsebranch at lines 210-211 unreachable.Simplify by removing the unreachable branch
- if matched.is_some() { - (ConstraintMatch::With, matched) - } else { - (ConstraintMatch::NoConstraint, None) - } + (ConstraintMatch::With, matched)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@approval-gate/src/permissions/compile.rs` around lines 208 - 212, The else branch in the conditional block checking if matched.is_some() is unreachable dead code because the prior loop iteration guarantees that matched will be Some when constraints is non-empty. Remove the else branch that returns ConstraintMatch::NoConstraint and simplify the code to only return the ConstraintMatch::With case with the matched value, since that is the only path that will ever execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@approval-gate/src/config.rs`:
- Around line 387-394: The test function from_yaml_expands_env_var has
formatting that does not conform to Rust's standard formatting conventions as
enforced by cargo fmt. Run cargo fmt --all locally to automatically reformat the
entire codebase including this test function to match the project's formatting
standards, then commit the changes.
In `@approval-gate/src/configuration.rs`:
- Around line 108-112: The set_config_value function has formatting that does
not comply with Rust's standard formatting rules as enforced by cargo fmt. To
fix this, run cargo fmt --all from the root of the project to automatically
apply the required formatting changes to the function and any other files that
need formatting adjustments.
---
Nitpick comments:
In `@approval-gate/src/permissions/compile.rs`:
- Around line 208-212: The else branch in the conditional block checking if
matched.is_some() is unreachable dead code because the prior loop iteration
guarantees that matched will be Some when constraints is non-empty. Remove the
else branch that returns ConstraintMatch::NoConstraint and simplify the code to
only return the ConstraintMatch::With case with the matched value, since that is
the only path that will ever execute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 735edb99-44ff-4b55-9588-b9f7abd13b44
⛔ Files ignored due to path filters (1)
approval-gate/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
approval-gate/Cargo.tomlapproval-gate/README.mdapproval-gate/architecture/README.mdapproval-gate/architecture/integration.mdapproval-gate/architecture/internals.mdapproval-gate/src/config.rsapproval-gate/src/configuration.rsapproval-gate/src/decision.rsapproval-gate/src/error.rsapproval-gate/src/events.rsapproval-gate/src/functions/gate.rsapproval-gate/src/functions/get_pending.rsapproval-gate/src/functions/list_pending.rsapproval-gate/src/functions/mod.rsapproval-gate/src/functions/on_session_deleted.rsapproval-gate/src/functions/on_turn_completed.rsapproval-gate/src/functions/purge.rsapproval-gate/src/functions/resolve.rsapproval-gate/src/functions/sweep.rsapproval-gate/src/lib.rsapproval-gate/src/main.rsapproval-gate/src/manifest.rsapproval-gate/src/pending.rsapproval-gate/src/permissions/compile.rsapproval-gate/src/permissions/default_rules.rsapproval-gate/src/permissions/mod.rsapproval-gate/src/permissions/types.rsapproval-gate/src/policy.rsapproval-gate/src/testkit/engine.rsapproval-gate/src/testkit/mod.rsapproval-gate/src/types.rsapproval-gate/tests/golden/schemas/approval.get-pending.jsonapproval-gate/tests/golden/schemas/approval.list-pending.jsonapproval-gate/tests/golden/schemas/approval.pending-created.jsonapproval-gate/tests/golden/schemas/approval.pending-resolved.jsonapproval-gate/tests/golden/schemas/approval.sweep.jsonapproval-gate/tests/integration.rsapproval-gate/tests/schemas.rs
💤 Files with no reviewable changes (9)
- approval-gate/tests/golden/schemas/approval.sweep.json
- approval-gate/src/functions/sweep.rs
- approval-gate/src/events.rs
- approval-gate/tests/golden/schemas/approval.pending-resolved.json
- approval-gate/tests/golden/schemas/approval.get-pending.json
- approval-gate/src/functions/on_turn_completed.rs
- approval-gate/tests/golden/schemas/approval.pending-created.json
- approval-gate/tests/golden/schemas/approval.list-pending.json
- approval-gate/src/policy.rs
✅ Files skipped from review due to trivial changes (6)
- approval-gate/Cargo.toml
- approval-gate/src/functions/list_pending.rs
- approval-gate/architecture/README.md
- approval-gate/src/functions/purge.rs
- approval-gate/src/error.rs
- approval-gate/src/pending.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- approval-gate/src/permissions/default_rules.rs
- approval-gate/src/testkit/mod.rs
- approval-gate/src/functions/get_pending.rs
- approval-gate/src/permissions/types.rs
- approval-gate/src/functions/on_session_deleted.rs
- approval-gate/src/lib.rs
- approval-gate/architecture/integration.md
- approval-gate/src/functions/mod.rs
- approval-gate/src/permissions/mod.rs
- approval-gate/src/functions/resolve.rs
- approval-gate/src/types.rs
…holds never expire Consolidate the four overlapping permission sources (deployment rules, per-session deltas, console localStorage defaults, harness structural floor) into one canonical source: the `approval-gate` configuration `rules` (architecture/permissions-source.md). Auto-mode trust formerly in `always_allow_seed` is now expressed as allow rules with `modes: ["auto"]`; session `always_allow` / `approved_always` stay as per-session human deltas. - permissions: inline first-match rule evaluation in `approval::gate` (allow / deny / no-match holds); mode-scoped allow rules seed the auto allowlist - holds never expire: the harness sweep skips any pending with `held_by` (approval/hook holds); only sub-agent child pendings time out - slim config.rs / configuration.rs to the consolidated model; drop the dead per-call timeout_ms plumbing and the STATE_/CONFIG_TIMEOUT_MS constants (state/config trigger calls use the SDK default) - harness v1 wiring: deferred resolve, hook runner, turn loop - tests: contract_parity + harness_integration lock the cross-worker wire shapes - drop the retired !policy::check_permissions floor from iii-permissions.yaml
Replace the console's localStorage permission defaults with the canonical `approval-gate` configuration entry, matching the worker-side single-source consolidation. - approval-gate-config.ts (new): load/save default_mode + auto allowlist via configuration::get/set on the `approval-gate` entry; derive the harness FunctionPolicy (deny floor) from the deployment rules - real.ts: send the policy derived from the config on each turn, with a fail-closed fallback (deny approval::*/configuration::*) when the read fails - use-approval-settings.ts: seed a new conversation's mode + allowlist from the config instead of localStorage; degrade to manual on read failure - ConsoleSettingsTab.tsx: edit the deployment config entry (load on mount, persist on change) instead of localStorage - DefaultPermissionModePicker.tsx: skip localStorage writes when the parent controls the value via onChange
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
approval-gate/architecture/integration.md (1)
79-79:⚠️ Potential issue | 🟡 MinorUpdate gate hold response in diagram to reflect removal of
pending_timeout_msThe diagram at line 79 shows
{ decision: "hold", pending_timeout_ms: 0 }, but the code has completely removedpending_timeout_msfrom the response. The test at types.rs:413 and the function comment at functions/gate.rs:97-99 both confirm the response is now simply{ decision: "hold" }.Update line 79 to:
AG-->>H: { decision: "hold" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@approval-gate/architecture/integration.md` at line 79, Update the sequence diagram response at line 79 in the integration.md file to remove the pending_timeout_ms field from the gate hold response. The response from AG to H should be changed to only include the decision field set to "hold", removing the pending_timeout_ms: 0 part entirely to match the actual implementation confirmed in the gate function and test cases.approval-gate/src/types.rs (2)
177-183:⚠️ Potential issue | 🔴 CriticalWire contract change:
Timeout→Abortedrequires downstream consumer updates.The
ResolvedOutcomeenum now produces"aborted"instead of"timeout"in the wire format. However, the TypeScript consumer inconsole/web/src/types/iii-agent-event.ts(line 56) still defines the type with'timeout', and the tech spec attech-specs/2026-06-agentic/approval-gate.md(line 427) still documents"timeout"as valid. These must be updated to remove'timeout'to match the Rust changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@approval-gate/src/types.rs` around lines 177 - 183, The ResolvedOutcome enum in Rust now serializes "aborted" instead of "timeout" for the wire format. Update the TypeScript type definition in console/web/src/types/iii-agent-event.ts (around line 56) to replace the 'timeout' variant with 'aborted' to match the Rust enum. Additionally, update the tech spec documentation in tech-specs/2026-06-agentic/approval-gate.md (around line 427) to replace all references to "timeout" with "aborted" to keep the specification aligned with the actual wire contract.
144-175:⚠️ Potential issue | 🔴 CriticalUpdate console TypeScript type to match Rust removal of
expires_at.The Rust
PendingApprovalRecordstruct (lines 144–175) no longer includesexpires_at, butconsole/web/src/types/iii-agent-event.tsstill defines it as a required field (expires_at: number). When the approval-gate sends records without this field, deserialization will fail or create type mismatches. Either removeexpires_atfrom the TypeScript interface or mark it optional (expires_at?: number).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@approval-gate/src/types.rs` around lines 144 - 175, The TypeScript interface definition for PendingApprovalRecord still includes expires_at as a required field, but the Rust PendingApprovalRecord struct no longer contains this field, causing deserialization failures. Update the TypeScript interface to either remove the expires_at field entirely or make it optional by changing it to expires_at?: number to match the Rust struct definition.
🧹 Nitpick comments (2)
approval-gate/src/permissions/mod.rs (1)
123-136: 💤 Low valueSilent dropping of invalid mode strings may widen policy unintentionally.
If an operator specifies
modes: ["auto", "typo"], the typo is silently ignored and the rule applies only toautomode. This may not match operator intent — consider logging a warning when unknown mode strings are encountered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@approval-gate/src/permissions/mod.rs` around lines 123 - 136, The modes parsing logic silently drops invalid mode strings using filter_map without notifying the operator of the issue. Modify the filter_map call that matches mode strings against the PermissionMode variants so that when a string doesn't match any known mode (manual, auto, or full), log a warning message indicating the unrecognized mode value before filtering it out. This ensures operators are aware when their configuration contains typos or invalid mode values instead of silently narrowing the policy scope.console/web/src/pages/Configuration/tabs/ConsoleSettingsTab.tsx (1)
95-117: ⚡ Quick winAvoid calling async functions inside setState callbacks.
The
addAllowandremoveAllowcallbacks invokepersistDefaults(an async operation) inside thesetAllowlistupdater function. While the current code happens to work becausepersistDefaultsuses its own argument rather than reading from state, this pattern mixes side effects with state updates and can lead to subtle bugs if the logic evolves.♻️ Suggested refactor to separate concerns
const addAllow = useCallback( (functionId: string) => { - setAllowlist((prev) => { - if (prev.includes(functionId)) return prev - const next = [...prev, functionId] - void persistDefaults(defaultMode, next) - return next - }) + setAllowlist((prev) => { + if (prev.includes(functionId)) return prev + return [...prev, functionId] + }) + // Fire persistence after state update (uses updated closure value next render) + const next = allowlist.includes(functionId) ? allowlist : [...allowlist, functionId] + void persistDefaults(defaultMode, next) }, [defaultMode, persistDefaults], + [allowlist, defaultMode, persistDefaults], ) const removeAllow = useCallback( (functionId: string) => { - setAllowlist((prev) => { - if (!prev.includes(functionId)) return prev - const next = prev.filter((id) => id !== functionId) - void persistDefaults(defaultMode, next) - return next - }) + setAllowlist((prev) => { + if (!prev.includes(functionId)) return prev + return prev.filter((id) => id !== functionId) + }) + const next = allowlist.filter((id) => id !== functionId) + void persistDefaults(defaultMode, next) }, - [defaultMode, persistDefaults], + [allowlist, defaultMode, persistDefaults], )Alternatively, keep the current approach but add a comment explaining that
persistDefaultsintentionally uses its argument, not closure state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/web/src/pages/Configuration/tabs/ConsoleSettingsTab.tsx` around lines 95 - 117, Move the persistDefaults async function call outside the setState updater callback in both addAllow and removeAllow functions. Instead of invoking persistDefaults inside the setAllowlist updater, first update the state with the new array value, then call persistDefaults with the updated array after the state setter. This separates the side effect (async persistence) from the state update logic, following the principle of separating concerns and avoiding side effects within state updaters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@approval-gate/src/config.rs`:
- Around line 276-281: The from_yaml_expands_env_var_in_rules test function uses
std::env::set_var and std::env::remove_var which are not thread-safe and will
cause race conditions when tests run in parallel. Fix this by either adding the
#[serial] attribute from the serial_test crate above the test function to ensure
it runs serially, or refactor the expand_env function to accept an environment
variable lookup closure parameter that can be mocked in tests, allowing tests to
run without relying on global environment state.
In `@approval-gate/src/testkit/engine.rs`:
- Around line 446-452: The harness subprocess spawned by spawn_harness_worker
can be orphaned if the wait_for_harness readiness check fails, since the panic
occurs before the child process is stored in TestStack where Drop cleanup would
normally occur. Modify the code to explicitly kill and wait for the child
process before the assertion panics when wait_for_harness fails, ensuring the
subprocess is properly cleaned up regardless of the readiness check outcome.
In `@approval-gate/tests/contract_parity.rs`:
- Around line 5-37: The tests in gate_hold_matches_harness_parse_output,
resolve_allow_payload_matches_harness_function_resolve_request, and
resolve_deny_payload_matches_harness_function_resolve_request are
self-referential because they hardcode the expected JSON structure and only
validate against that same hardcoded structure. To properly verify contract
parity between approval-gate and harness, replace the hardcoded json! macro
payloads with actual calls to the approval-gate serialization functions that
produce these payloads in the real code, then validate that the actual
serialized output matches the expected harness contract. This ensures the tests
catch real contract drift when the producer changes.
In `@approval-gate/tests/harness_integration.rs`:
- Around line 12-31: The with_harness_stack function calls spawn_engine()
directly which can cause race conditions in parallel tests. After the harness
binary environment variable check, replace the current engine spawning logic
with a call to testkit::with_stack instead. This will reuse the testkit boot
lock mechanism to serialize engine startup and prevent concurrent boot races.
Refactor the function to use testkit::with_stack which handles the engine
spawning with proper serialization, ensuring all tests inherit the same
thread-safe boot guarantees.
In `@harness/src/deferred.rs`:
- Around line 249-259: The pending_call_expired function has a bug on the line
computing elapsed time where now.saturating_sub(pending_at) as u64 can produce
incorrect results when pending_at is greater than now due to clock skew. When
the subtraction yields a negative i64 value and is cast to u64, it wraps around
to a huge positive value, causing premature timeout resolution. Fix this by
clamping the elapsed time calculation to ensure it does not go negative before
the u64 cast, such as by using max(0, elapsed_time) or by checking if the
elapsed time is negative and returning false in that case to indicate the call
is not expired when clock skew causes inverted timestamps.
---
Outside diff comments:
In `@approval-gate/architecture/integration.md`:
- Line 79: Update the sequence diagram response at line 79 in the integration.md
file to remove the pending_timeout_ms field from the gate hold response. The
response from AG to H should be changed to only include the decision field set
to "hold", removing the pending_timeout_ms: 0 part entirely to match the actual
implementation confirmed in the gate function and test cases.
In `@approval-gate/src/types.rs`:
- Around line 177-183: The ResolvedOutcome enum in Rust now serializes "aborted"
instead of "timeout" for the wire format. Update the TypeScript type definition
in console/web/src/types/iii-agent-event.ts (around line 56) to replace the
'timeout' variant with 'aborted' to match the Rust enum. Additionally, update
the tech spec documentation in tech-specs/2026-06-agentic/approval-gate.md
(around line 427) to replace all references to "timeout" with "aborted" to keep
the specification aligned with the actual wire contract.
- Around line 144-175: The TypeScript interface definition for
PendingApprovalRecord still includes expires_at as a required field, but the
Rust PendingApprovalRecord struct no longer contains this field, causing
deserialization failures. Update the TypeScript interface to either remove the
expires_at field entirely or make it optional by changing it to expires_at?:
number to match the Rust struct definition.
---
Nitpick comments:
In `@approval-gate/src/permissions/mod.rs`:
- Around line 123-136: The modes parsing logic silently drops invalid mode
strings using filter_map without notifying the operator of the issue. Modify the
filter_map call that matches mode strings against the PermissionMode variants so
that when a string doesn't match any known mode (manual, auto, or full), log a
warning message indicating the unrecognized mode value before filtering it out.
This ensures operators are aware when their configuration contains typos or
invalid mode values instead of silently narrowing the policy scope.
In `@console/web/src/pages/Configuration/tabs/ConsoleSettingsTab.tsx`:
- Around line 95-117: Move the persistDefaults async function call outside the
setState updater callback in both addAllow and removeAllow functions. Instead of
invoking persistDefaults inside the setAllowlist updater, first update the state
with the new array value, then call persistDefaults with the updated array after
the state setter. This separates the side effect (async persistence) from the
state update logic, following the principle of separating concerns and avoiding
side effects within state updaters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c88e17a6-a532-48f5-9ead-e0ec5eb96f00
⛔ Files ignored due to path filters (1)
approval-gate/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
approval-gate/Cargo.tomlapproval-gate/README.mdapproval-gate/architecture/README.mdapproval-gate/architecture/integration.mdapproval-gate/architecture/internals.mdapproval-gate/architecture/permissions-source.mdapproval-gate/src/config.rsapproval-gate/src/configuration.rsapproval-gate/src/functions/add_always_allow.rsapproval-gate/src/functions/approve_always.rsapproval-gate/src/functions/clear_settings.rsapproval-gate/src/functions/gate.rsapproval-gate/src/functions/get_pending.rsapproval-gate/src/functions/get_settings.rsapproval-gate/src/functions/list_pending.rsapproval-gate/src/functions/on_session_deleted.rsapproval-gate/src/functions/purge.rsapproval-gate/src/functions/remove_always_allow.rsapproval-gate/src/functions/resolve.rsapproval-gate/src/functions/set_mode.rsapproval-gate/src/harness.rsapproval-gate/src/main.rsapproval-gate/src/pending.rsapproval-gate/src/permissions/compile.rsapproval-gate/src/permissions/default_rules.rsapproval-gate/src/permissions/mod.rsapproval-gate/src/permissions/types.rsapproval-gate/src/session.rsapproval-gate/src/settings.rsapproval-gate/src/state.rsapproval-gate/src/testkit/engine.rsapproval-gate/src/types.rsapproval-gate/tests/contract_parity.rsapproval-gate/tests/golden/schemas/approval.add-always-allow.jsonapproval-gate/tests/golden/schemas/approval.approve-always.jsonapproval-gate/tests/golden/schemas/approval.gate.jsonapproval-gate/tests/golden/schemas/approval.get-settings.jsonapproval-gate/tests/golden/schemas/approval.remove-always-allow.jsonapproval-gate/tests/golden/schemas/approval.set-mode.jsonapproval-gate/tests/harness_integration.rsapproval-gate/tests/integration.rsconsole/web/src/components/permissions/DefaultPermissionModePicker.tsxconsole/web/src/hooks/use-approval-settings.tsconsole/web/src/lib/backend/approval-gate-config.test.tsconsole/web/src/lib/backend/approval-gate-config.tsconsole/web/src/lib/backend/real.tsconsole/web/src/pages/Configuration/tabs/ConsoleSettingsTab.tsxharness/src/deferred.rsharness/src/functions/function_trigger.rsharness/src/hooks/runner.rsharness/src/turn_loop.rsiii-permissions.yaml
💤 Files with no reviewable changes (1)
- iii-permissions.yaml
✅ Files skipped from review due to trivial changes (9)
- approval-gate/architecture/permissions-source.md
- approval-gate/tests/golden/schemas/approval.add-always-allow.json
- approval-gate/tests/golden/schemas/approval.set-mode.json
- approval-gate/tests/golden/schemas/approval.remove-always-allow.json
- approval-gate/tests/golden/schemas/approval.approve-always.json
- approval-gate/tests/golden/schemas/approval.get-settings.json
- approval-gate/src/functions/purge.rs
- approval-gate/architecture/README.md
- approval-gate/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- approval-gate/src/permissions/types.rs
- approval-gate/tests/integration.rs
- approval-gate/architecture/internals.md
- approval-gate/src/permissions/compile.rs
Comment on lines
+276
to
281
| fn from_yaml_expands_env_var_in_rules() { | ||
| std::env::set_var("APPROVAL_GATE_TEST_RULE", "state::get"); | ||
| let cfg = WorkerConfig::from_yaml("rules:\n - \"${APPROVAL_GATE_TEST_RULE}\"\n").unwrap(); | ||
| assert_eq!(cfg.rules, vec![json!("state::get")]); | ||
| std::env::remove_var("APPROVAL_GATE_TEST_RULE"); | ||
| } |
There was a problem hiding this comment.
std::env::set_var is not thread-safe and can cause flaky tests.
set_var/remove_var are inherently racy when tests run in parallel. Consider using a serial test attribute or extracting expand_env testing to use a mock environment lookup.
Suggested approach
Use #[serial] from the serial_test crate, or refactor expand_env to accept an environment lookup closure for testability:
// Option 1: Add serial_test dependency and use #[serial]
#[test]
#[serial]
fn from_yaml_expands_env_var_in_rules() { ... }
// Option 2: Parameterize expand_env for testing
fn expand_env_with<F>(input: &str, lookup: F) -> String
where F: Fn(&str) -> Option<String> { ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@approval-gate/src/config.rs` around lines 276 - 281, The
from_yaml_expands_env_var_in_rules test function uses std::env::set_var and
std::env::remove_var which are not thread-safe and will cause race conditions
when tests run in parallel. Fix this by either adding the #[serial] attribute
from the serial_test crate above the test function to ensure it runs serially,
or refactor the expand_env function to accept an environment variable lookup
closure parameter that can be mocked in tests, allowing tests to run without
relying on global environment state.
Comment on lines
+446
to
+452
| let harness_child = if opts.real_harness { | ||
| let child = spawn_harness_worker(&engine.url).expect("spawn harness worker"); | ||
| assert!( | ||
| wait_for_harness(&iii).await, | ||
| "harness worker did not become ready" | ||
| ); | ||
| Some(child) |
There was a problem hiding this comment.
Avoid orphaning the harness subprocess when readiness fails.
At Line 448, a failed readiness check panics before child is stored in TestStack, so the TestStack::Drop cleanup path never runs for that process. Add explicit kill/wait before panicking.
Suggested fix
- let harness_child = if opts.real_harness {
- let child = spawn_harness_worker(&engine.url).expect("spawn harness worker");
- assert!(
- wait_for_harness(&iii).await,
- "harness worker did not become ready"
- );
- Some(child)
+ let harness_child = if opts.real_harness {
+ let mut child = spawn_harness_worker(&engine.url).expect("spawn harness worker");
+ if !wait_for_harness(&iii).await {
+ let _ = child.kill();
+ let _ = child.wait();
+ panic!("harness worker did not become ready");
+ }
+ Some(child)
} else {
None
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@approval-gate/src/testkit/engine.rs` around lines 446 - 452, The harness
subprocess spawned by spawn_harness_worker can be orphaned if the
wait_for_harness readiness check fails, since the panic occurs before the child
process is stored in TestStack where Drop cleanup would normally occur. Modify
the code to explicitly kill and wait for the child process before the assertion
panics when wait_for_harness fails, ensuring the subprocess is properly cleaned
up regardless of the readiness check outcome.
Comment on lines
+5
to
+37
| #[test] | ||
| fn gate_hold_matches_harness_parse_output() { | ||
| let hold = json!({ "decision": "hold" }); | ||
| assert!(hold.get("pending_timeout_ms").is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_allow_payload_matches_harness_function_resolve_request() { | ||
| let payload = json!({ | ||
| "session_id": "s_1", | ||
| "turn_id": "t_1", | ||
| "function_call_id": "c_1", | ||
| "action": "execute", | ||
| }); | ||
| assert_eq!(payload["action"], json!("execute")); | ||
| assert!(payload.get("content").is_none()); | ||
| assert!(payload.get("is_error").is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_deny_payload_matches_harness_function_resolve_request() { | ||
| let payload = json!({ | ||
| "session_id": "s_1", | ||
| "turn_id": "t_1", | ||
| "function_call_id": "c_1", | ||
| "action": "deliver", | ||
| "is_error": true, | ||
| "content": [{ "type": "text", "text": "denied" }], | ||
| "details": { "status": "denied", "denied_by": "user" }, | ||
| }); | ||
| assert_eq!(payload["action"], json!("deliver")); | ||
| assert_eq!(payload["is_error"], json!(true)); | ||
| } |
There was a problem hiding this comment.
These parity tests are self-referential and won’t catch real contract drift.
At Lines 7-37, each test constructs inline JSON and then validates that same literal shape. This does not verify the actual approval-gate/harness serialization path, so producer/consumer contract regressions can still pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@approval-gate/tests/contract_parity.rs` around lines 5 - 37, The tests in
gate_hold_matches_harness_parse_output,
resolve_allow_payload_matches_harness_function_resolve_request, and
resolve_deny_payload_matches_harness_function_resolve_request are
self-referential because they hardcode the expected JSON structure and only
validate against that same hardcoded structure. To properly verify contract
parity between approval-gate and harness, replace the hardcoded json! macro
payloads with actual calls to the approval-gate serialization functions that
produce these payloads in the real code, then validate that the actual
serialized output matches the expected harness contract. This ensures the tests
catch real contract drift when the producer changes.
Comment on lines
+12
to
+31
| async fn with_harness_stack<F, Fut>(opts: BootOpts, f: F) | ||
| where | ||
| F: FnOnce(approval_gate::testkit::TestStack) -> Fut, | ||
| Fut: std::future::Future<Output = ()>, | ||
| { | ||
| if engine_bin().is_none() { | ||
| eprintln!("skipping: no iii engine"); | ||
| return; | ||
| } | ||
| let Some(engine) = spawn_engine().await else { | ||
| eprintln!("skipping: failed to spawn engine"); | ||
| return; | ||
| }; | ||
| if std::env::var("CARGO_BIN_EXE_harness").is_err() { | ||
| eprintln!("skipping: harness binary not built for integration tests"); | ||
| return; | ||
| } | ||
| let stack = boot(&engine, opts).await; | ||
| f(stack).await; | ||
| } |
There was a problem hiding this comment.
Serialize harness integration engine boot to prevent parallel startup races.
At Line 21, spawn_engine() is called directly in multi-thread tests, bypassing the testkit boot lock that was added to avoid concurrent boot races/flakes. Reuse testkit::with_stack (after the harness-binary env check) so these tests inherit the same serialization guarantees.
Suggested fix
-use approval_gate::testkit::{
- boot, call, engine_bin, hook_input, spawn_engine, state_get, state_set, BootOpts,
-};
+use approval_gate::testkit::{call, hook_input, state_get, state_set, with_stack, BootOpts};
async fn with_harness_stack<F, Fut>(opts: BootOpts, f: F)
where
F: FnOnce(approval_gate::testkit::TestStack) -> Fut,
Fut: std::future::Future<Output = ()>,
{
- if engine_bin().is_none() {
- eprintln!("skipping: no iii engine");
- return;
- }
- let Some(engine) = spawn_engine().await else {
- eprintln!("skipping: failed to spawn engine");
- return;
- };
if std::env::var("CARGO_BIN_EXE_harness").is_err() {
eprintln!("skipping: harness binary not built for integration tests");
return;
}
- let stack = boot(&engine, opts).await;
- f(stack).await;
+ with_stack(opts, f).await;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@approval-gate/tests/harness_integration.rs` around lines 12 - 31, The
with_harness_stack function calls spawn_engine() directly which can cause race
conditions in parallel tests. After the harness binary environment variable
check, replace the current engine spawning logic with a call to
testkit::with_stack instead. This will reuse the testkit boot lock mechanism to
serialize engine startup and prevent concurrent boot races. Refactor the
function to use testkit::with_stack which handles the engine spawning with
proper serialization, ensuring all tests inherit the same thread-safe boot
guarantees.
Comment on lines
+249
to
+259
| fn pending_call_expired(cp: &crate::types::turn::CallCheckpoint, default_timeout_ms: u64, now: i64) -> bool { | ||
| if cp.state != CallState::Pending { | ||
| return false; | ||
| } | ||
| if cp.held_by.is_some() { | ||
| return false; | ||
| } | ||
| let timeout = cp.pending_timeout_ms.unwrap_or(default_timeout_ms); | ||
| let pending_at = cp.pending_at.unwrap_or(now); | ||
| now.saturating_sub(pending_at) as u64 >= timeout | ||
| } |
There was a problem hiding this comment.
Clamp negative elapsed time before converting to u64.
On Line 258, now.saturating_sub(pending_at) as u64 can turn negative deltas into huge positive values, causing premature timeout resolution when pending_at > now (e.g., clock skew).
Suggested fix
fn pending_call_expired(cp: &crate::types::turn::CallCheckpoint, default_timeout_ms: u64, now: i64) -> bool {
if cp.state != CallState::Pending {
return false;
}
if cp.held_by.is_some() {
return false;
}
let timeout = cp.pending_timeout_ms.unwrap_or(default_timeout_ms);
let pending_at = cp.pending_at.unwrap_or(now);
- now.saturating_sub(pending_at) as u64 >= timeout
+ let elapsed_ms = now.saturating_sub(pending_at).max(0) as u64;
+ elapsed_ms >= timeout
} #[test]
fn sub_agent_pending_uses_default_timeout_when_unset() {
@@
}
+
+#[test]
+fn future_pending_at_does_not_expire_immediately() {
+ let now = 1_000_000;
+ let checkpoint = cp(CallState::Pending, None, Some(60_000), now + 5_000);
+ assert!(!pending_call_expired(&checkpoint, 1_800_000, now));
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn pending_call_expired(cp: &crate::types::turn::CallCheckpoint, default_timeout_ms: u64, now: i64) -> bool { | |
| if cp.state != CallState::Pending { | |
| return false; | |
| } | |
| if cp.held_by.is_some() { | |
| return false; | |
| } | |
| let timeout = cp.pending_timeout_ms.unwrap_or(default_timeout_ms); | |
| let pending_at = cp.pending_at.unwrap_or(now); | |
| now.saturating_sub(pending_at) as u64 >= timeout | |
| } | |
| fn pending_call_expired(cp: &crate::types::turn::CallCheckpoint, default_timeout_ms: u64, now: i64) -> bool { | |
| if cp.state != CallState::Pending { | |
| return false; | |
| } | |
| if cp.held_by.is_some() { | |
| return false; | |
| } | |
| let timeout = cp.pending_timeout_ms.unwrap_or(default_timeout_ms); | |
| let pending_at = cp.pending_at.unwrap_or(now); | |
| let elapsed_ms = now.saturating_sub(pending_at).max(0) as u64; | |
| elapsed_ms >= timeout | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@harness/src/deferred.rs` around lines 249 - 259, The pending_call_expired
function has a bug on the line computing elapsed time where
now.saturating_sub(pending_at) as u64 can produce incorrect results when
pending_at is greater than now due to clock skew. When the subtraction yields a
negative i64 value and is cast to u64, it wraps around to a huge positive value,
causing premature timeout resolution. Fix this by clamping the elapsed time
calculation to ensure it does not go negative before the u64 cast, such as by
using max(0, elapsed_time) or by checking if the elapsed time is negative and
returning false in that case to indicate the call is not expired when clock skew
causes inverted timestamps.
The consolidation commit left two latent failures that CI hit once the
formatting check ran:
- rustfmt: format files the change touched but didn't run fmt on
(config.rs, permissions/mod.rs, tests/*, harness/deferred.rs)
- clippy::needless_update: drop the now-redundant `..WorkerConfig::default()`
in two tests — WorkerConfig is just { default_mode, rules }, so every field
was already specified
No behavior change.
sergiofilhowz
approved these changes
Jun 18, 2026
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
approval-gateconfiguration entry via a new inlinepermissions/module, replacing thepolicy::check_permissionsRPC in the gate.pending_timeout_ms,expires_at,approval::sweep, and thetimeoutresolved outcome — holds wait until human resolve or turn/session purge.iii-permissions.yaml, harness policy worker, and claude-code are unchanged for now.Test plan
cd approval-gate && cargo testcargo test --test integrationcargo test --test schemascargo clippy --all-targets --all-features -- -D warningsSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores