fix(agent-os): stop escalate() crashing at max escalation depth - #3569
fix(agent-os): stop escalate() crashing at max escalation depth#3569PratikDhanave (PratikDhanave) wants to merge 1 commit into
Conversation
SupervisorHierarchy.escalate() built its depth-limit decision as TrustDecision(..., policy_name="escalation_limit"), but TrustDecision has no policy_name field (and authority is required with no default). So the moment an escalation chain exceeded max_escalation_depth, the constructor raised TypeError instead of returning the limiting deny — a crash at exactly the point the limiter is meant to engage, and a potential fail-open if a caller swallows the exception. Construct the decision with the fields the dataclass defines: authority "trust_root" (this is the trust root's escalation cap being enforced) and the existing reason; deterministic defaults to True. Add a regression test driving an escalation chain past several max-depth settings. Fixes microsoft#3539
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
There was a problem hiding this comment.
Pull request overview
Fixes a crash in agent_os supervisor escalation depth limiting by constructing the max-depth TrustDecision with the dataclass’s actual fields, and adds a regression test for issue #3539.
TL;DR: 0 blockers, 0 warnings. No issues found. Clean change.
Changes:
- Replace invalid
TrustDecision(..., policy_name=...)construction withauthority="trust_root"when max escalation depth is exceeded. - Add a regression test that escalates past
max_escalation_depthand asserts a well-formed deny decision is returned (not aTypeError).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
agent-governance-python/agent-os/src/agent_os/supervisor.py |
Fixes the depth-limit branch to return a valid TrustDecision instead of raising due to an unexpected constructor argument. |
agent-governance-python/agent-os/tests/test_supervisor_escalation.py |
Adds regression coverage ensuring escalation past configured max depth returns a deterministic deny decision with expected fields. |
|
This fixes the crash and the parametrized test over max_depth is stronger than the one in #3557. The commit has no Signed-off-by, so DCO fails. authority is set to trust_root, but the branch returns before calling trust_root.validate_action, so the decision is attributed to an authority that never ran; #3557 uses supervisor and also updates the comment above the branch, which here still says the final decision comes from the trust root. The test is worth carrying over to #3557. |
Imran Siddique (imran-siddique)
left a comment
Imran Siddique (imran-siddique)
left a comment
There was a problem hiding this comment.
Correct diagnosis and the right value. Endorsing this one.
The bug. escalate() built its depth-cap decision with policy_name="escalation_limit", which TrustDecision never defined, and omitted authority, which is required and has no default. So the branch fails twice over and raises TypeError rather than denying. The reason it matters more than a crash: this is the runaway-escalation limiter, so if any caller above it swallows the exception, the guard becomes a fail-open at precisely the depth it exists to stop. A guard that throws is not a guard.
On authority="trust_root", which is the only thing separating this from #3557. AlgoVoi (Christopher Hopley) (@chopmob-cloud) found the same bug a day earlier and fixed it with authority="supervisor". Both are defensible from the code, so worth saying why this one is right rather than leaving it to taste.
The cap is self.trust_root.max_escalation_depth. SupervisorHierarchy performs the comparison but does not own the bound, and escalate() documents its return as "TrustDecision from the trust root (always deterministic)". A consumer reading authority is asking who decided, and the answer at the cap is the same authority that set the cap. Attributing it to the supervisor would make the field say a supervisor overrode an escalation, which is the one thing the hierarchy is built to prevent.
Worth flagging beyond this PR: there is no authority vocabulary to be consistent with. The only value anywhere in the codebase is "native-runtime", emitted twice by TrustRoot, and get_authority_chain() returns supervisor names rather than roles. So this PR and #3557 were each inventing a term with nothing to check against, and the style does not match "native-runtime" either. That is not this PR's problem to solve and I would not hold it, but a downstream consumer keying on authority currently has no defined set.
AlgoVoi (Christopher Hopley) (@chopmob-cloud) sorry to send you the other way on this one, your diagnosis in #3557 was identical and equally well written.
No approve bit on this repo, so this is a comment, but I would merge it.
Summary
SupervisorHierarchy.escalate()builds its depth-limit decision as:but
TrustDecision(trust_root.py) defines onlyallowed,reason,authority,deterministic— there is nopolicy_namefield, andauthorityis required with no default. So the moment an escalation chain exceedsmax_escalation_depth, the constructor raises:The path meant to cap runaway escalations instead crashes the caller — and if some layer above swallows the exception, it becomes a potential fail-open at exactly the point the limiter should engage.
Fixes #3539
Change
Construct the limiting decision with the fields the dataclass actually defines:
authority="trust_root"(this is the trust root's escalation cap being enforced — consistent with the method docstring, "TrustDecision from the trust root") and the existingreason;deterministicdefaults toTrue.Testing
Added
tests/test_supervisor_escalation.py, which drives an escalation chain past severalmax_escalation_depthsettings (0, 1, 2) and asserts a well-formed limitingTrustDecision(allowed=False, reason mentions max depth,authority="trust_root",deterministic=True).Verified the test fails on the pre-fix code (all cases raise
TypeError: ... unexpected keyword argument 'policy_name') and passes with the fix. Ruff lint/format clean on the changed files.