fix(supervisor): return a deny decision instead of crashing at max escalation depth

[AlgoVoi (Christopher Hopley) (chopmob-cloud)]

Summary

At the escalation-depth cap in SupervisorHierarchy.escalate(), the guard built its decision as:

TrustDecision(allowed=False, reason="Max escalation depth exceeded", policy_name="escalation_limit")

but TrustDecision (trust_root.py) is a dataclass defining only allowed, reason, authority, deterministic. So the call raises TypeError two ways: an unexpected policy_name keyword, and a missing required authority. The branch meant to cap runaway escalation instead crashes the caller, and if an outer layer swallows the exception it becomes a fail-open at exactly the point the limiter should engage.

Change

Construct the decision with the fields the dataclass defines, using authority="supervisor" (the depth cap is enforced by SupervisorHierarchy; the only other authority literal in the tree is "native-runtime"). The cap now returns a well-formed deny.

Tests

Adds tests/test_supervisor_escalation_depth.py: one test drives an escalation chain past max_escalation_depth and asserts a well-formed limiting TrustDecision(allowed=False, ...) with no TypeError; one asserts below-cap escalation still flows to the trust root. Both pass, and the existing test_trust_root, test_escalation, and test_escalation_and_reversibility suites still pass.

Refs #3539

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

[github-actions]

🔴 Contributor Check: HIGH

Check	Result
Profile	HIGH
Credential	LOW
Overall	HIGH

Automated check by AGT Contributor Check.

[Copilot]

Pull request overview

TL;DR: 1 blocker, 1 warning. Fix the new regression test assertions and this ships.

#	Sev	Issue	Where
1	Block	Regression test should assert the specific authority value (and avoid is True/False assertions)	test_supervisor_escalation_depth.py
2	Warn	Inline comment is misleading now that escalate() can return early at the depth cap	supervisor.py

Changes:

• Fix SupervisorHierarchy.escalate() to return a well-formed deny TrustDecision at max escalation depth (instead of raising TypeError).
• Add regression tests covering above-cap and below-cap escalation behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
agent-governance-python/agent-os/src/agent_os/supervisor.py	Fixes the depth-cap decision construction to match TrustDecision fields.
agent-governance-python/agent-os/tests/test_supervisor_escalation_depth.py	Adds regression tests for the max escalation depth guard and normal escalation flow.

Suppressed comments (2)

agent-governance-python/agent-os/tests/test_supervisor_escalation_depth.py:58

• Use a truthiness assertion (assert decision.allowed) instead of is True to avoid boolean identity comparisons and keep tests consistent with the rest of the suite’s style.

    assert decision.allowed is True

agent-governance-python/agent-os/src/agent_os/supervisor.py:149

• This comment is now misleading because escalate() can return early with a depth-cap deny decision (i.e., the final decision does not always come from the trust root). Update it to reflect the depth-cap behavior.

                    authority="supervisor",
                )

        # Final decision always comes from the deterministic trust root
        return self.trust_root.validate_action(action)

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

agent-governance-python/agent-os/src/agent_os/supervisor.py:148

• The inline comment says the final decision always comes from the trust root, but escalate() can now return early when the escalation-depth cap is exceeded. This comment is misleading and should be updated to reflect that the trust root is only reached when the depth cap is not hit.

        # Final decision always comes from the deterministic trust root

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

agent-governance-python/agent-os/src/agent_os/supervisor.py:146

• The escalate() docstring still states the return value always comes from the trust root, but this method can now return early with a supervisor-authored deny when the escalation-depth cap is exceeded. Updating the Returns: section will keep the documentation accurate for callers.

                return TrustDecision(
                    allowed=False,
                    reason="Max escalation depth exceeded",
                    authority="supervisor",
                )

[Imran Siddique (imran-siddique)]

Duplicate of #3569, which fixes the same bug (#3539) in the same place. Yours came first by a day and the diagnosis is identical and equally well written, so this is not a quality call.

The two differ in exactly one thing: you set authority="supervisor", #3569 sets authority="trust_root". I have gone with trust_root and endorsed that one, on the reasoning that the cap is self.trust_root.max_escalation_depth and escalate() documents its return as coming from the trust root. Attributing the denial to the supervisor would make the field read as a supervisor overriding an escalation, which is the thing the hierarchy exists to prevent. Reasonable people could read it the other way and you clearly did.

One thing your PR has that the other does not, and it should not be lost: you also corrected the trailing comment to say "Below the escalation-depth cap, the final decision comes from the deterministic trust root". The original read "Final decision always comes from the deterministic trust root", which stopped being true the moment the cap branch started returning its own decision. That is worth carrying over.

Also worth saying: #3659 and #3660 are both yours, both green, and both close real fail-open paths. I have reviewed and endorsed both. This one going to the other PR is a coin-flip on a naming detail, not a signal about your work.

Leaving this open rather than closing it, since a maintainer may prefer your version or want the comment fix.

[AlgoVoi (Christopher Hopley) (chopmob-cloud)]

Thanks for the careful read, and no disagreement on the call. trust_root is a reasonable place to land it: the cap reads off self.trust_root.max_escalation_depth and the docstring already frames the return as coming from the root, so the field lines up with the documented contract. Happy to defer to #3569 on that.

The one piece worth not losing is the trailing comment. Once the cap branch returns its own decision, "Final decision always comes from the deterministic trust root" is no longer strictly true, so I changed it to "Below the escalation-depth cap, the final decision comes from the deterministic trust root". That is a one line change and easy to fold into #3569 if you would like it carried over. Whichever way is least friction for you.

Fine to close this in favour of #3569 whenever suits.