Repository navigation
feat(approval-gate): enhance configuration management and permissions - #276
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (46)
💤 Files with no reviewable changes (5)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (29)
📝 WalkthroughWalkthroughReplaces the approval-gate Changesapproval-gate: configuration subsystem refactor
context-manager: LeaseCell hot-swap and per-call summarizer timeout
Sequence Diagram(s)sequenceDiagram
participant CLI
participant main as approval-gate main
participant configuration as configuration.rs
participant ConfigWorker as configuration worker
participant handlers as approval::* handlers
participant ConfigCell
rect rgba(30, 100, 200, 0.5)
note over CLI,ConfigWorker: Boot sequence
CLI->>main: start (--url, optional --config seed)
main->>configuration: register_config(seed?)
configuration->>ConfigWorker: register schema + seed initial_value
main->>configuration: fetch_config()
ConfigWorker-->>main: WorkerConfig (authoritative)
main->>ConfigCell: Arc<RwLock<Arc<WorkerConfig>>>
main->>configuration: bind_hook + bind_sweep → TriggerHandles
main->>configuration: register_config_trigger(cell, handles)
end
rect rgba(20, 150, 80, 0.5)
note over ConfigWorker,ConfigCell: Live config reload
ConfigWorker->>configuration: configuration:updated trigger
configuration->>ConfigWorker: re-fetch authoritative WorkerConfig
configuration->>configuration: compare boot_signature()
alt hook or sweep_expression changed
configuration->>configuration: rebind_slot (register new, unregister old)
end
configuration->>ConfigCell: apply_config (snapshot swap)
end
rect rgba(180, 60, 20, 0.5)
note over handlers,ConfigCell: Per-call config read
handlers->>ConfigCell: deps.config().await
ConfigCell-->>handlers: Arc<WorkerConfig> snapshot
handlers->>handlers: use cfg.* timeouts and defaults
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
skill-check — worker0 verified, 22 skipped (no docs/).
Four for four. Nicely done. |
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 (1)
tech-specs/2026-06-agentic/approval-gate.md (1)
359-364:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign configuration dependency semantics with the implemented boot contract.
Line 359 still describes
configurationas a soft dependency with fallback defaults. The current runtime contract is hard dependency (register_config/fetch_configfailure aborts boot), so this section is now inconsistent with the worker behavior.🤖 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 `@tech-specs/2026-06-agentic/approval-gate.md` around lines 359 - 364, The section describing the `configuration` dependency (starting at line 359) currently documents it as a soft dependency with built-in fallback defaults, but the actual implementation treats it as a hard dependency where failures in `register_config` or `fetch_config` abort the boot process. Update this section to remove the description of soft dependency behavior and fallback defaults, and instead clearly document that `configuration` is a required hard dependency whose initialization failures will halt system boot.
🤖 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`:
- Around line 20-24: The integration.md architecture document has inconsistent
fully-qualified function IDs in the function table. Add the missing `approval::`
prefix to the function names remove-always-allow, on-session-deleted, and
on-turn-completed so they match the consistent naming pattern used for other
entries like add-always-allow, approve-always, get-settings, and clear-settings.
Ensure all function IDs in the table include the namespace prefix to prevent
breaking consumer bindings and provide accurate documentation of callable
function identifiers.
In `@approval-gate/src/config.rs`:
- Around line 86-89: The documentation comments describing the BOOT SIGNATURE
behavior for `hook` and `sweep_expression` at lines 86-89 and 98-103 are
outdated and incorrectly state that config changes to these fields are refused
on hot-reload with a restart required message. Update these documentation
comments to accurately reflect the current runtime behavior, which now re-binds
both `hook` and `sweep_expression` live during hot-reload rather than refusing
the changes. This will align the schema documentation with the actual
operational behavior and prevent operator confusion.
In `@approval-gate/src/configuration.rs`:
- Around line 274-286: The issue is that apply_config is called unconditionally
even when rebind_slot operations for hook or sweep may have failed (returned
None), which advances the boot_signature in ConfigCell while stale bindings
remain active, preventing retries on future config changes. Add guards to ensure
apply_config is only called after successful rebind operations: verify that both
rebind_slot calls for the hook (when hook config changes) and sweep (when
sweep_expression changes) complete successfully, or restructure the logic to
only call apply_config if all necessary structural rebinds have produced valid
new handles, keeping the last-good signature in ConfigCell when rebinds fail.
In `@tech-specs/2026-06-agentic/approval-gate.md`:
- Around line 439-444: The harness hook trigger type name documented in the spec
does not match the actual implementation. On line 439, replace the hook name
`harness::hook::pre_dispatch` with the correct hook name
`harness::hook::pre-trigger` to align with the current approval-gate binding
implementation. This ensures integrators reference the correct hook identifier
when implementing the approval-gate functionality.
- Around line 819-820: In the migration table on line 819, the trigger name
`pending_resolved` is missing the `approval::` prefix and does not match the
naming convention used for other triggers in the same cell. Replace
`pending_resolved` with `approval::pending-resolved` to maintain consistency
with the wire surface naming convention established by the
`approval::pending-created` trigger shown in the same line.
---
Outside diff comments:
In `@tech-specs/2026-06-agentic/approval-gate.md`:
- Around line 359-364: The section describing the `configuration` dependency
(starting at line 359) currently documents it as a soft dependency with built-in
fallback defaults, but the actual implementation treats it as a hard dependency
where failures in `register_config` or `fetch_config` abort the boot process.
Update this section to remove the description of soft dependency behavior and
fallback defaults, and instead clearly document that `configuration` is a
required hard dependency whose initialization failures will halt system boot.
🪄 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: 360e48c4-d630-47d4-81dc-9da403887c62
📒 Files selected for processing (48)
approval-gate/README.mdapproval-gate/architecture/integration.mdapproval-gate/architecture/internals.mdapproval-gate/config.yamlapproval-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/mod.rsapproval-gate/src/functions/on_config_change.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/functions/sweep.rsapproval-gate/src/gate_config.rsapproval-gate/src/lib.rsapproval-gate/src/main.rsapproval-gate/src/manifest.rsapproval-gate/src/settings.rsapproval-gate/src/testkit/engine.rsapproval-gate/tests/golden/schemas/approval.on-config-change.jsonapproval-gate/tests/integration.rsapproval-gate/tests/schemas.rscontext-manager/README.mdcontext-manager/architecture/README.mdcontext-manager/architecture/internals.mdcontext-manager/config.yamlcontext-manager/src/adapters/router.rscontext-manager/src/config.rscontext-manager/src/configuration.rscontext-manager/src/functions/assemble.rscontext-manager/src/functions/compact.rscontext-manager/src/main.rscontext-manager/src/ports.rscontext-manager/tests/common/workers.rscontext-manager/tests/common/world.rscontext-manager/tests/integration.rsdocs/sops/binary-worker.mddocs/sops/configuration.mdiii-permissions.yamltech-specs/2026-06-agentic/approval-gate.md
💤 Files with no reviewable changes (5)
- approval-gate/tests/golden/schemas/approval.on-config-change.json
- approval-gate/src/functions/on_config_change.rs
- context-manager/config.yaml
- approval-gate/config.yaml
- approval-gate/src/gate_config.rs
| | `approval::add-always-allow` / `remove-always-allow` | console (human-only) | Curate the auto-mode trust list (idempotent add / no-op remove). | | ||
| | `approval::approve-always` | console (human-only) | Per-session grant honoured in **every** mode; call it right before `resolve { decision: "allow" }` for an "Approve always" button. | | ||
| | `approval::get-settings` | console | Effective settings + `source: "stored" \| "defaults"`. Never writes. | | ||
| | `approval::clear-settings` | console | Drop the stored record; revert to deployment defaults. | | ||
| | `approval::on-config-change` / `on_session_deleted` / `on_turn_completed` / `approval::sweep` | trigger handlers | Internal — never call directly. | | ||
| | `approval::on-config-change` / `on-session-deleted` / `on-turn-completed` / `approval::sweep` | trigger handlers | Internal — never call directly. | |
There was a problem hiding this comment.
Use fully-qualified function IDs consistently in the function table.
Line 20 and Line 24 drop the approval:: prefix on remove-always-allow, on-session-deleted, and on-turn-completed. This documents non-existent IDs and can break consumer bindings/calls.
Proposed doc fix
-| `approval::add-always-allow` / `remove-always-allow` | console (human-only) | Curate the auto-mode trust list (idempotent add / no-op remove). |
+| `approval::add-always-allow` / `approval::remove-always-allow` | console (human-only) | Curate the auto-mode trust list (idempotent add / no-op remove). |
-| `approval::on-config-change` / `on-session-deleted` / `on-turn-completed` / `approval::sweep` | trigger handlers | Internal — never call directly. |
+| `approval::on-config-change` / `approval::on-session-deleted` / `approval::on-turn-completed` / `approval::sweep` | trigger handlers | Internal — never call directly. |📝 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.
| | `approval::add-always-allow` / `remove-always-allow` | console (human-only) | Curate the auto-mode trust list (idempotent add / no-op remove). | | |
| | `approval::approve-always` | console (human-only) | Per-session grant honoured in **every** mode; call it right before `resolve { decision: "allow" }` for an "Approve always" button. | | |
| | `approval::get-settings` | console | Effective settings + `source: "stored" \| "defaults"`. Never writes. | | |
| | `approval::clear-settings` | console | Drop the stored record; revert to deployment defaults. | | |
| | `approval::on-config-change` / `on_session_deleted` / `on_turn_completed` / `approval::sweep` | trigger handlers | Internal — never call directly. | | |
| | `approval::on-config-change` / `on-session-deleted` / `on-turn-completed` / `approval::sweep` | trigger handlers | Internal — never call directly. | | |
| | `approval::add-always-allow` / `approval::remove-always-allow` | console (human-only) | Curate the auto-mode trust list (idempotent add / no-op remove). | | |
| | `approval::approve-always` | console (human-only) | Per-session grant honoured in **every** mode; call it right before `resolve { decision: "allow" }` for an "Approve always" button. | | |
| | `approval::get-settings` | console | Effective settings + `source: "stored" \| "defaults"`. Never writes. | | |
| | `approval::clear-settings` | console | Drop the stored record; revert to deployment defaults. | | |
| | `approval::on-config-change` / `approval::on-session-deleted` / `approval::on-turn-completed` / `approval::sweep` | trigger handlers | Internal — never call directly. | |
🤖 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` around lines 20 - 24, The
integration.md architecture document has inconsistent fully-qualified function
IDs in the function table. Add the missing `approval::` prefix to the function
names remove-always-allow, on-session-deleted, and on-turn-completed so they
match the consistent naming pattern used for other entries like
add-always-allow, approve-always, get-settings, and clear-settings. Ensure all
function IDs in the table include the namespace prefix to prevent breaking
consumer bindings and provide accurate documentation of callable function
identifiers.
| /// - The BOOT SIGNATURE (`hook` + `sweep_expression`): consumed ONCE at | ||
| /// startup to bind the `harness::hook::pre-trigger` hook and the cron | ||
| /// sweep. A config change that alters either is REFUSED on hot-reload | ||
| /// (logged "restart required", the previous snapshot kept). |
There was a problem hiding this comment.
Align structural reload docs with actual live rebind behavior.
Line 86-89 and Line 98/101 still describe hook/sweep_expression as restart-required, but runtime now re-binds them live. This mismatch can mislead operators via schema/docs.
Suggested doc-only patch
-/// - The BOOT SIGNATURE (`hook` + `sweep_expression`): consumed ONCE at
-/// startup to bind the `harness::hook::pre-trigger` hook and the cron
-/// sweep. A config change that alters either is REFUSED on hot-reload
-/// (logged "restart required", the previous snapshot kept).
+/// - The BOOT SIGNATURE (`hook` + `sweep_expression`): structural trigger
+/// bindings. On change, runtime re-binds the affected trigger live.
@@
- /// The `harness::hook::pre-trigger` binding (restart-required).
+ /// The `harness::hook::pre-trigger` binding (live re-bound on change).
@@
- /// 6-field cron expression for the expiry sweep (restart-required).
+ /// 6-field cron expression for the expiry sweep (live re-bound on change).Also applies to: 98-103
🤖 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 86 - 89, The documentation comments
describing the BOOT SIGNATURE behavior for `hook` and `sweep_expression` at
lines 86-89 and 98-103 are outdated and incorrectly state that config changes to
these fields are refused on hot-reload with a restart required message. Update
these documentation comments to accurately reflect the current runtime behavior,
which now re-binds both `hook` and `sweep_expression` live during hot-reload
rather than refusing the changes. This will align the schema documentation with
the actual operational behavior and prevent operator confusion.
| if old.boot_signature() != cfg.boot_signature() { | ||
| if old.hook != cfg.hook { | ||
| rebind_slot(&handles.hook, bind_hook(iii, &cfg)); | ||
| tracing::info!("approval-gate hook re-bound (hook config changed)"); | ||
| } | ||
| if old.sweep_expression != cfg.sweep_expression { | ||
| rebind_slot(&handles.sweep, bind_sweep(iii, &cfg)); | ||
| tracing::info!("approval-gate sweep re-bound (sweep_expression changed)"); | ||
| } | ||
| } | ||
|
|
||
| apply_config(cell, cfg).await; | ||
| tracing::info!("approval-gate configuration reloaded"); |
There was a problem hiding this comment.
Don’t swap ConfigCell when structural rebind fails.
At Line 285, apply_config runs even when Line 276/280 produced no new handle (None). That can advance boot_signature in ConfigCell while old trigger bindings remain active, preventing retries on later config-change events and leaving stale enforcement wiring.
Suggested guard to keep last-good structural signature
async fn on_config_change(iii: &III, cell: &ConfigCell, handles: &TriggerHandles) {
@@
- if old.boot_signature() != cfg.boot_signature() {
+ let mut structural_rebind_failed = false;
+ if old.boot_signature() != cfg.boot_signature() {
if old.hook != cfg.hook {
- rebind_slot(&handles.hook, bind_hook(iii, &cfg));
- tracing::info!("approval-gate hook re-bound (hook config changed)");
+ let new = bind_hook(iii, &cfg);
+ if new.is_none() {
+ structural_rebind_failed = true;
+ tracing::error!("config-change: hook rebind failed; keeping previous config snapshot");
+ } else {
+ rebind_slot(&handles.hook, new);
+ tracing::info!("approval-gate hook re-bound (hook config changed)");
+ }
}
if old.sweep_expression != cfg.sweep_expression {
- rebind_slot(&handles.sweep, bind_sweep(iii, &cfg));
- tracing::info!("approval-gate sweep re-bound (sweep_expression changed)");
+ let new = bind_sweep(iii, &cfg);
+ if new.is_none() {
+ structural_rebind_failed = true;
+ tracing::error!("config-change: sweep rebind failed; keeping previous config snapshot");
+ } else {
+ rebind_slot(&handles.sweep, new);
+ tracing::info!("approval-gate sweep re-bound (sweep_expression changed)");
+ }
}
}
+
+ if structural_rebind_failed {
+ return;
+ }
apply_config(cell, cfg).await;📝 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.
| if old.boot_signature() != cfg.boot_signature() { | |
| if old.hook != cfg.hook { | |
| rebind_slot(&handles.hook, bind_hook(iii, &cfg)); | |
| tracing::info!("approval-gate hook re-bound (hook config changed)"); | |
| } | |
| if old.sweep_expression != cfg.sweep_expression { | |
| rebind_slot(&handles.sweep, bind_sweep(iii, &cfg)); | |
| tracing::info!("approval-gate sweep re-bound (sweep_expression changed)"); | |
| } | |
| } | |
| apply_config(cell, cfg).await; | |
| tracing::info!("approval-gate configuration reloaded"); | |
| let mut structural_rebind_failed = false; | |
| if old.boot_signature() != cfg.boot_signature() { | |
| if old.hook != cfg.hook { | |
| let new = bind_hook(iii, &cfg); | |
| if new.is_none() { | |
| structural_rebind_failed = true; | |
| tracing::error!("config-change: hook rebind failed; keeping previous config snapshot"); | |
| } else { | |
| rebind_slot(&handles.hook, new); | |
| tracing::info!("approval-gate hook re-bound (hook config changed)"); | |
| } | |
| } | |
| if old.sweep_expression != cfg.sweep_expression { | |
| let new = bind_sweep(iii, &cfg); | |
| if new.is_none() { | |
| structural_rebind_failed = true; | |
| tracing::error!("config-change: sweep rebind failed; keeping previous config snapshot"); | |
| } else { | |
| rebind_slot(&handles.sweep, new); | |
| tracing::info!("approval-gate sweep re-bound (sweep_expression changed)"); | |
| } | |
| } | |
| } | |
| if structural_rebind_failed { | |
| return; | |
| } | |
| apply_config(cell, cfg).await; | |
| tracing::info!("approval-gate configuration reloaded"); |
🤖 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/configuration.rs` around lines 274 - 286, The issue is that
apply_config is called unconditionally even when rebind_slot operations for hook
or sweep may have failed (returned None), which advances the boot_signature in
ConfigCell while stale bindings remain active, preventing retries on future
config changes. Add guards to ensure apply_config is only called after
successful rebind operations: verify that both rebind_slot calls for the hook
(when hook config changes) and sweep (when sweep_expression changes) complete
successfully, or restructure the logic to only call apply_config if all
necessary structural rebinds have produced valid new handles, keeping the
last-good signature in ConfigCell when rebinds fail.
| - **`harness::hook::pre_dispatch`** ([harness](harness.md#hooks)) → `approval::gate` — the | ||
| synchronous hook binding itself (`functions` filter, `timeout_ms`, `on_error: "fail_closed"`; | ||
| see [The `approval::gate` hook](#the-approvalgate-hook)). | ||
| - **`configuration`** on `configuration_id: "approval-gate"` → `approval::on_config_change` — | ||
| - **`configuration`** on `configuration_id: "approval-gate"` → `approval::on-config-change` — | ||
| reload deployment defaults reactively (replaces the prior deployment's | ||
| `approval::on_harness_config` binding on the harness entry). |
There was a problem hiding this comment.
Update the harness hook trigger type name in the spec.
Line 439 documents harness::hook::pre_dispatch, but current approval-gate binding uses harness::hook::pre-trigger. This mismatch can cause incorrect integrator implementations.
🤖 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 `@tech-specs/2026-06-agentic/approval-gate.md` around lines 439 - 444, The
harness hook trigger type name documented in the spec does not match the actual
implementation. On line 439, replace the hook name `harness::hook::pre_dispatch`
with the correct hook name `harness::hook::pre-trigger` to align with the
current approval-gate binding implementation. This ensures integrators reference
the correct hook identifier when implementing the approval-gate functionality.
| | No pending signal; console derives from `turn_state_changed` + `awaiting_approval[]` | `approval::pending-created` / `pending_resolved` triggers + `approval_pending` inbox | | ||
| | No global pending list | `approval::list-pending` / `approval::get-pending` | |
There was a problem hiding this comment.
Fix the remaining snake_case trigger name in the migration table.
Line 819 uses pending_resolved without worker prefix. It should be approval::pending-resolved to match the renamed wire surface.
🤖 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 `@tech-specs/2026-06-agentic/approval-gate.md` around lines 819 - 820, In the
migration table on line 819, the trigger name `pending_resolved` is missing the
`approval::` prefix and does not match the naming convention used for other
triggers in the same cell. Replace `pending_resolved` with
`approval::pending-resolved` to maintain consistency with the wire surface
naming convention established by the `approval::pending-created` trigger shown
in the same line.
- Added new permission rules `!context::on-config-change` and `!approval::on-config-change` to the `iii-permissions.yaml` file to improve security and control over configuration changes. - Removed the obsolete `approval-gate/config.yaml` file, consolidating configuration management under the `configuration` worker for better clarity and authority. - Updated the `README.md` and other documentation to reflect the new configuration structure and the removal of `config.yaml`, emphasizing the role of the `configuration` worker. - Introduced a new `configuration.rs` file to handle the integration with the `configuration` worker, ensuring hot-reload capabilities for configuration changes without requiring a restart. - Refactored the `main.rs` to streamline the boot process, ensuring the authoritative configuration is fetched from the `configuration` worker.
4d26288 to
4a8d2cb
Compare
!context::on-config-changeand!approval::on-config-changeto theiii-permissions.yamlfile to improve security and control over configuration changes.approval-gate/config.yamlfile, consolidating configuration management under theconfigurationworker for better clarity and authority.README.mdand other documentation to reflect the new configuration structure and the removal ofconfig.yaml, emphasizing the role of theconfigurationworker.configuration.rsfile to handle the integration with theconfigurationworker, ensuring hot-reload capabilities for configuration changes without requiring a restart.main.rsto streamline the boot process, ensuring the authoritative configuration is fetched from theconfigurationworker.Summary by CodeRabbit
config.yamlfiles are no longer used;--configis now only a first-registration seed.