feat(006): add T075 pure authority evaluator - #76
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughAdds a crate-visible delegation authority model and evaluator. The evaluator checks authority planes, worker identity, topology, enforcement evidence, visibility, and untrusted text. New tests cover precedence, fail-closed behavior, determinism, and contract immutability. ChangesDelegation authority
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This PR adds a test-only, pure authority evaluator without changing runtime behavior, dependencies, persistence, or operation execution. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant AuthorityRequest
participant evaluate_delegation
participant DelegationContract
participant AuthorityEvaluation
AuthorityRequest->>evaluate_delegation: submit request
evaluate_delegation->>DelegationContract: inspect grants and policy ceilings
evaluate_delegation->>AuthorityEvaluation: record decision, visibility, and human action
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
TheHalfMoon
marked this pull request as ready for review
August 21, 2026 17:40
Owner
Author
|
@coderabbitai review Please review the current exact candidate head |
|
|
PR Summary by QodoAdd T075 pure authority/delegation evaluator (fixture-only) with proofs
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Owner
Author
T075 exact-head author qualificationAuthor correctness / safety / evidence-integrity verdict — PASS
Ponytail / YAGNI verdict — PASSThe implementation stays inside the T075 slice: one pure module, one focused test module, and test-only crate registration. It does not pre-build T076 approval persistence/digests, T077/T078 protocol clients, runtime execution, recursive fleets, generic policy engines, or persistence abstractions. Independent exact-head review remains required before merge. Any head movement invalidates this qualification and requires exact-head gates/review to be rerun. |
Owner
Author
T075 external-review reconciliation — exact headReconciliationQodo reviewed the T075 slice and recommended retaining the current small, pure evaluator approach as the best fit for the fixture-only deterministic proof goal. CodeRabbit reviewed the same exact base-to-head range and generated no actionable comments, rating merge risk Minimal. CodeRabbit also reported a docstring-coverage warning (60% vs its generic 80% threshold). This is classified NON_MATERIAL / NO_CHANGE because:
No review thread is unresolved. No code/head mutation is required from external review. Any subsequent head movement invalidates this reconciliation and requires fresh exact-head qualification. |
Code Review by Qodo
1. Missing rule can allow
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+24
to
+27
| self.rules | ||
| .get(target) | ||
| .copied() | ||
| .unwrap_or(self.default_decision) |
There was a problem hiding this comment.
1. Missing rule can allow 🐞 Bug ⛨ Security
AuthorityPlane::decision_for falls back to default_decision, so if any plane is constructed with default_decision=Allow, an unspecified (missing-scope) target can be authorized, violating fail-closed expectations. This creates a silent privilege escalation path driven by configuration rather than explicit rules.
Agent Prompt
## Issue description
`AuthorityPlane::decision_for()` uses `unwrap_or(self.default_decision)`. That means a caller can accidentally (or maliciously, if contracts become derived from partially untrusted config later) set `default_decision=Allow` and get an implicit allow for targets not explicitly present in the rules map.
This conflicts with the spec’s “fail closed” posture for authority evaluation and undermines the test `missing_scope_fails_closed_instead_of_inheriting_visibility_or_prose`, which currently only passes because the fixtures set `default_decision=Deny`.
## Issue Context
The evaluator is meant to be a pure authority evaluator; it should be robustly fail-closed and not depend on “callers remembered to set defaults to deny” to maintain safety.
## Fix Focus Areas
- src/agentic_authority.rs[22-28]
- src/agentic_authority.rs[178-195]
## Suggested fix
Choose one of these approaches (prefer A):
A) Remove `default_decision` from authorization semantics for this evaluator and always treat missing rules as `Deny` (fail-closed). For example, change `decision_for` to `self.rules.get(target).copied().unwrap_or(AuthorityDecision::Deny)`.
B) Keep `default_decision` but add explicit validation in `evaluate_delegation()` that all planes involved in delegation have `default_decision == Deny`. If any plane has a non-deny default, return `Deny` (or `Ask`, depending on desired UX) with a reason like `ApprovalRequired`/a new `UnsafePolicyDefault` and a clear `HumanAction::ChangeProtectedPolicy`.
Also add a test that constructs a plane with `default_decision=Allow` and a request for an unmapped target, and assert the decision remains fail-closed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+160
to
+165
| if worker.parent_planner_id != contract.planner_id || worker.worker_id == contract.planner_id { | ||
| return make_evaluation( | ||
| AuthorityDecision::Deny, | ||
| AuthorityReason::InvalidTopology, | ||
| HumanAction::ReduceToSingleWorker, | ||
| Vec::new(), |
There was a problem hiding this comment.
2. Wrong action for self-loop 🐞 Bug ≡ Correctness
When a worker is invalid because worker.worker_id == contract.planner_id, the evaluator returns HumanAction::ReduceToSingleWorker, which cannot remediate a self-loop (it’s already a single worker). This will mislead operators/tests about the corrective action needed for that invalid topology.
Agent Prompt
## Issue description
The invalid-topology branch conflates multiple topology failures under `HumanAction::ReduceToSingleWorker`. In particular, `worker.worker_id == contract.planner_id` indicates a self-loop (planner acting as worker), which is not fixable by reducing to a single worker.
## Issue Context
This evaluator returns a human action meant to guide remediation. For correctness and safety workflows, that action should be specific enough that following it can actually fix the problem.
## Fix Focus Areas
- src/agentic_authority.rs[159-167]
## Suggested fix
Split the combined condition:
- If `contract.workers.len() != 1`: keep `ReduceToSingleWorker`.
- If `worker.parent_planner_id != contract.planner_id`: return an action that indicates fixing parent/relationship (either introduce a new `HumanAction::FixPlannerWorkerRelationship` or reuse `ChangeProtectedPolicy` if that’s the intended remediation bucket).
- If `worker.worker_id == contract.planner_id`: return `SelectAuthorizedWorker` (or a new more specific action like `SeparatePlannerAndWorkerIdentity`).
Add/adjust a focused test case for the self-loop topology to assert the updated `human_action`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
What changed
T075 only: add a pure, fixture-only authority/delegation evaluator for one Planner -> one Worker relationship.
Changed exactly:
src/agentic_authority.rssrc/t075_agentic_authority_tests.rssrc/main.rstest-only registrationNo migration. No dependency change. No Store mutation. No Agent process or prompt. No ACP/MCP/daemon IPC/remote execution. No operation execution or automatic landing.
Canonical base:
ad9d5b223f55e78d6db73c69bb5ca076a1da3ea1.Exact qualified head:
311c92fdf0e35bb58b00ca224a2dab934054a8d5.Spec Kit traceability
specs/006-agentic-terminal-local-delegation-control-plane/spec.mdDeterministic exact-head evidence
quality#702 / run32508355226— SUCCESSwindows-terminal#408 / run32508355223— SUCCESSrelease-candidate#469 / run32508355215— SUCCESScargo fmt --all -- --checkcargo clippy --locked --all-targets --all-features -- -D warningscargo test --locked --all-targets --all-featuresReview stack
External-review reconciliation
CodeRabbit's only warning was generic docstring coverage (60% vs 80%). Classified NON_MATERIAL / NO_CHANGE: T075 is a private/test-only fixture proof surface, canonical acceptance does not require docstring coverage, all Rust/CI gates pass, and adding documentation-only churn solely for a generic bot metric would violate the accepted YAGNI posture.
Winds safety invariants
WINDS_ENFORCEDcannot survive without complete Winds mediation evidenceAny candidate-head movement invalidates this qualification and requires fresh exact-head gates and review before merge.