Skip to content

fix(policies): make the evaluate fast path see root session policies - #7319

Closed
omni-resolve-agent[bot] wants to merge 1 commit into
mainfrom
fix/34772159134
Closed

omni-resolve-agent[bot] wants to merge 1 commit into
mainfrom
fix/34772159134

Conversation

@omni-resolve-agent

@omni-resolve-agent omni-resolve-agent Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Related issue

Closes #7316
Resolves OMNI-7734

Summary

A child session with no policies of its own could bypass approval rules inherited from the root session.

The policy fast path now checks the root session's policies before allowing an early return, so descendants reach the normal inheritance-aware evaluator.

Test Plan

Regression coverage in this PR targets the reported behavior. The original description did not record a test-run result; the linked resolve update contains the historical verification context.

Changed regression coverage:

  • tests/e2e_ui/approvals/test_subagent_policy_inheritance.py
  • tests/runtime/policies/test_builder_session_policies.py

Suggested check from the PR checkout, with the repository test dependencies installed (not rerun for this edit):

python -m pytest tests/e2e_ui/approvals/test_subagent_policy_inheritance.py tests/runtime/policies/test_builder_session_policies.py -q
Source reports and verification runs

Original resolve workflow

Latest resolve-agent update

Demo

  • Visual demo attached below
  • Non-visual evidence provided below or in Test Plan
  • Not applicable — no behavioral change

Recordings from the resolve-agent update; Linear sign-in required. Captions describe the recorded environment, not a new test run.

  • Before fix (bug reproduced) — attach the 'Require Approval for File & Shell Operations' policy to a session -> its own shell command parks an approval card -> approve it -> open the sub-agent session spawned under it -> the sub-agent's shell command fires -> no approval card ever appears: the command is allowed ungated
    before-subagent-ungated.mp4
  • After fix (bug resolved) — attach the 'Require Approval for File & Shell Operations' policy to a session -> its own shell command parks an approval card -> approve it -> open the policy-free sub-agent session spawned under it -> the sub-agent's shell command fires -> the inherited approval card parks on the child session before the tool runs -> approve it and the command is allowed
    after-subagent-gated.mp4

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Coverage notes

Test results above are attributed to the original resolve runs. No product tests were rerun for this description-only update.

any_policies_apply checked the agent spec, the sub-agent's own spec,
server/store defaults, and the session's OWN policies, but never the
ROOT conversation's session policies, which build_policy_engine
inherits into every descendant's pipeline. A sub-agent carrying no
policies of its own therefore short-circuited POST /policies/evaluate
to POLICY_ACTION_ALLOW and ran shell/file tools ungated past the
root session's ask-on-shell approval gate.

Check the root's session policies through the conversation row's
root_conversation_id before fast-pathing; the read hits the same
per-session LRU cache, so the guard stays O(1) on warm calls.

Covers the guard with a builder-layer unit test and an end-to-end
browser test proving a policy-free sub-agent's Bash call parks the
inherited approval card before the tool runs.
@github-actions github-actions Bot added size/L Pull request size: L P1-high Priority: major feature broken, no workaround labels Sep 13, 2026
@github-actions

github-actions Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

UI Preview for this PR has been removed.

@omnigent-ci

omnigent-ci Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

None. The fix is correct for normal operation: it reads the root's session policies via the same LRU-cached _load_session_policy_specs the engine uses, guards with the same root_conversation_id != conversation_id condition, and returns True (build the engine) rather than short-circuiting. Because any_policies_apply returning True only builds the engine (which re-verifies the tree), the sole risky direction is a false False, and that cannot occur for a stable session: for any non-deleted row the engine's verified root equals conversation.root_conversation_id, so the fast path and the engine agree.

Security vulnerabilities

None introduced. This PR closes a real gate-bypass: previously every policy-free sub-agent short-circuited to ALLOW, running shell/file tools past the root's inherited ask-on-shell gate. The change strictly tightens that boundary.

Non-blocking notes

  • Suggested-root vs. verified-root divergence. build_policy_engine deliberately does not trust the row's root_conversation_id; it loads a verified root via load_verified_session_tree because the raw suggested root can be wrong after a delete/recreate-under-a-different-root race (there's an explicit comment to that effect at ~line 425). The new fast-path guard uses the raw conversation.root_conversation_id. In the specific race where a session id is recreated under a new root that has an ASK/DENY policy while the old root (still named by the row) has none, the fast path could return False and skip the gate. In practice this is a narrow in-request TOCTOU: the evaluate route reads conv fresh at request start (routes_hooks.py, reusing the ACL row), so the row is current for the request rather than a stale cached snapshot — hence non-blocking. Still, aligning with the engine's own reasoning (verify the root, or conservatively build the engine when the root can't be cheaply verified) would remove the divergence entirely. Worth a one-line code comment noting the intentional cheaper-read tradeoff.
  • Test coverage gap. Both new tests exercise only stable rows; neither covers the delete/recreate-with-changed-root path, so the divergence above is untested. Optional given the low reachability.

Approach

Sound and consistent with existing patterns — it reuses the exact helper and inheritance guard the engine relies on, keeping the fast path O(1) on warm cache. The only more-consistent alternative is to route the fast-path root resolution through the same verification the engine uses, but that reintroduces the tree-load cost the fast path exists to avoid; the current cheaper read is a reasonable tradeoff for a guard that only ever errs toward building the engine (except the narrow race noted above).

Summary

A well-scoped, well-documented fix for a genuine authorization-boundary bug: policy-free sub-agents were skipping their root session's inherited gate. The core logic is correct and mirrors the engine's inheritance, and the unit + e2e tests capture the intended behavior and its isolation. The one substantive observation is that the fast path trusts the row's suggested root while the engine trusts a verified root — a deliberate perf tradeoff whose only failure mode is a very narrow, freshly-read in-request race. Not blocking; consider a clarifying comment (and optionally a race-path test). No lockfile or dependency-extra changes to flag.


Automated review by Polly · workflow run

@omni-resolve-agent

Copy link
Copy Markdown
Contributor Author

Closing this PR because the work it addresses is already resolved: #7316 closed as completed; OMNI-7734 is Done. The fix appears to have landed elsewhere.

If that's not right and this PR still has unique value, comment /reopen (while the source branch still exists) and link a live issue or ticket. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1-high Priority: major feature broken, no workaround size/L Pull request size: L ui-preview

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] any_policies_apply skips root-session policy inheritance, so sub-agents run ungated

0 participants