Skip to content

fix(policies): detect and wrap legacy (content, phase) callables in resolve_function_policy - #49

Merged
github-actions[bot] merged 5 commits into
omnigent-ai:mainfrom
aarushisingh04:fix/gap-7-legacy-policy-callable-shim
Jun 17, 2026
Merged

fix(policies): detect and wrap legacy (content, phase) callables in resolve_function_policy#49
github-actions[bot] merged 5 commits into
omnigent-ai:mainfrom
aarushisingh04:fix/gap-7-legacy-policy-callable-shim

Conversation

@aarushisingh04

Copy link
Copy Markdown
Contributor

Summary

closes Gap 7 from tests/e2e/omnigent/TODO_omnigent_coverage.md : legacy omnigent (content, phase) policy callables silently no-oping under resolve_function_policy.

  • resolve_function_policy now detects legacy omnigent-style (content, phase) callables by inspecting the first two positional parameter names, and wraps them via the existing _omnigent_legacy_shim._wrap_legacy before storing them in FunctionPolicy. without this, any callable with that signature would silently ALLOW every event, the phase == "tool_call" check always fails when phase receives an event dict instead of a string.
  • the import is deferred inside the function (not module-level) to break a circular init chain: function.py → _omnigent_legacy_shim → policies.types → policies.__init__ → function.py.
  • modern single-arg (event) callables are unaffected; the adapter path (handler:_translate_function_policy_yaml → shim factory) is also unaffected and does not double-wrap.

Type of change

  • Bug fix
  • Feature
  • 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 rationale

three unit tests appended to tests/runtime/policies/test_function_policy.py under the Gap 7 section:

  • test_resolve_function_policy_wraps_legacy_callable : asserts that resolve_function_policy with a (content, phase) callable produces a FunctionPolicy (detection and wrapping succeed at build time).
  • test_resolve_function_policy_legacy_callable_evaluates_correctly : asserts the wrapped callable returns the correct DENY action and reason at evaluation time, verifying the full shim path including _coerce_legacy_return.
  • test_resolve_function_policy_modern_callable_not_wrapped : asserts a modern (event) callable passes through unchanged, confirmed by checking policy._callable.__name__ == "modern_allow" (shim inners are named _sync_shim/_async_shim).

no E2E coverage added: the omnigent example callables (block_long_sleep, etc.) were already rewritten to the modern single-arg format before this PR, so no existing e2e fixture exercises a legacy callable via resolve_function_policy. the unit tests above are the only coverage of this path. E2E verification would require introducing a legacy-signature callable into an example YAML, which is out of scope for this fix.

@aarushisingh04

Copy link
Copy Markdown
Contributor Author

note for reviewers:

if this fix looks correct, tests/e2e/omnigent/TODO_omnigent_coverage.md should be updated to move Gap 7 from "Outstanding gaps" to "Fixed". the fixed-section entry would note that resolve_function_policy now detects and wraps legacy (content, phase) callables via _omnigent_legacy_shim, and that the motivating example callables were already rewritten to modern format before this landed.

i can add the above modification to this PR if approved.

PattaraS
PattaraS previously approved these changes Jun 17, 2026
@github-actions github-actions Bot added the size/M Pull request size: M label Jun 17, 2026
@PattaraS

Copy link
Copy Markdown
Contributor

LGTM thanks for the PR!

@PattaraS PattaraS added the e2e-approved Maintainer approval to run secret-bearing e2e on a fork PR label Jun 17, 2026
@aarushisingh04

Copy link
Copy Markdown
Contributor Author

note for reviewers:

if this fix looks correct, tests/e2e/omnigent/TODO_omnigent_coverage.md should be updated to move Gap 7 from "Outstanding gaps" to "Fixed". the fixed-section entry would note that resolve_function_policy now detects and wraps legacy (content, phase) callables via _omnigent_legacy_shim, and that the motivating example callables were already rewritten to modern format before this landed.

i can add the above modification to this PR if approved.

@PattaraS thanks for the review! i would like to mention this again. what would you suggest?

@PattaraS

Copy link
Copy Markdown
Contributor

@aarushisingh04 That makes sense to me! Thanks for the note, we can mark the gap as fixed :)

@aarushisingh04

Copy link
Copy Markdown
Contributor Author

@PattaraS great! i have added the updated status in the docs for gap 7, would appreciate a final review!

@aarushisingh04
aarushisingh04 requested a review from PattaraS June 17, 2026 07:00

@PattaraS PattaraS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks again for the contribution!

@PattaraS

Copy link
Copy Markdown
Contributor

/merge

@github-actions
github-actions Bot merged commit 97b7c33 into omnigent-ai:main Jun 17, 2026
47 of 48 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🤖 /merge from @PattaraS, auto-merge enabled (squash, delete branch). ✅ gate is green, merging now.

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

Labels

e2e-approved Maintainer approval to run secret-bearing e2e on a fork PR size/M Pull request size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants