Skip to content

fix: violation record on exception - #148

Merged
araujof merged 6 commits into
0.1.xfrom
fix/violation-record-on-exception
Aug 4, 2026
Merged

fix: violation record on exception#148
araujof merged 6 commits into
0.1.xfrom
fix/violation-record-on-exception

Conversation

@prakhar-singh1928

Copy link
Copy Markdown
Contributor

Summary

Fixes the silent gap in PluginResult.executions when violations_as_exceptions=True — the denying plugin's ControlExecutionRecord was never written, making it impossible to identify which control blocked an invocation from telemetry alone.

Closes: #147


Changes

  • cpex/framework/manager.py_run_serial_phase: The except PluginViolationError block now appends a ControlExecutionRecord for the denying plugin (status=COMPLETED, effective_allow=False, matched=True, applied=True, error_code and reason from the violation) before re-raising. Previously this path was a bare raise with the comment "propagate immediately, no record needed".

  • cpex/framework/manager.pyexecute: The entire pipeline dispatch is now wrapped in a try/except PluginViolationError that attaches list(executions) to pve.executions before propagating. Callers catching PluginViolationError can now inspect pve.executions to see the full execution chain including the blocking plugin.

  • cpex/framework/errors.pyPluginViolationError: Added executions: list[ControlExecutionRecord] | None = None attribute. Defaults to None (backward compatible, no signature change). Set to the accumulated records list by execute() before the exception reaches the caller.

  • tests/unit/cpex/framework/test_execution_records.py: Two new tests directly assert pve.executions on the raised exception:

    • test_executions_on_violation_exception_single_plugin — one plugin denies; asserts 1 record with correct fields.
    • test_executions_on_violation_exception_two_plugin_chain — allow + deny chain; asserts 2 records in order, the second having effective_allow=False and the correct error_code.

Checks

  • make lint passes
  • make test passes
  • CHANGELOG updated (if user-facing)

Notes

No changes required on the ContextForge side. The existing except PluginViolationError workaround in tool_service.py (ctl_acc.mark_denied(hook="pre/post")) continues to work unchanged — pve.executions is purely additive.

@prakhar-singh1928
prakhar-singh1928 changed the base branch from main to 0.1.x July 31, 2026 12:49
@araujof araujof changed the title Fix/violation record on exception fix: violation record on exception Jul 31, 2026
@araujof araujof added bug Something isn't working Python 0.1.x labels Jul 31, 2026
@araujof araujof added this to CPEX Jul 31, 2026
@github-project-automation github-project-automation Bot moved this to Backlog in CPEX Jul 31, 2026
@araujof araujof added this to the 0.1.3 milestone Jul 31, 2026
@araujof araujof moved this from Backlog to In review in CPEX Jul 31, 2026

@terylt terylt 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.

Hi @prakhar-singh1928 , Thanks for the PR. Overall it's good. Here are some suggested changes:

[Medium — incomplete fix] The same gap remains for CONCURRENT-mode plugins

execute_plugin raises PluginViolationError for both SEQUENTIAL and CONCURRENT modes
when violations_as_exceptions=True (manager.py:1177, :1194-1204). But the deny record
is only appended in _run_serial_phase. In the concurrent path:

  • execute_plugin raises inside the task;
  • _tagged only catches PluginTimeoutError (manager.py:1028), so the PVE escapes the
    as_completed loop before the record-append at manager.py:451 and before the
    task-cancellation at :474;
  • the new outer handler then attaches an executions list that is missing the denying
    concurrent plugin
    — and the pending sibling tasks are never cancelled (pre-existing
    leak).

So a caller using violations_as_exceptions=True with any concurrent plugin still hits the
exact bug #147 describes. Recommend either (a) handling PVE in _tagged/the concurrent loop
symmetrically, or (b) explicitly scoping this PR to serial and noting the concurrent gap as
a follow-up.

Why it's serial-only: the record-writing code lives solely in
_run_serial_phase's exception handler. Concurrent plugins are dispatched in a separate
asyncio-task block that writes its record by inspecting a returned result
(manager.py:451). Because execute_plugin raises rather than returns for concurrent
denials under violations_as_exceptions=True, the exception unwinds past that append — that
concurrent-halt branch is effectively dead code in this mode.

[Low — traceability] Issue-number mismatch

The PR says "Closes #147", but every code comment, the errors.py docstring, and the test
section header reference cpex#130. #130 is the closed umbrella feature ("structured
control execution records"); #147 is the actual bug. The inline references should point at
#147.

[Nit] errors.py import is heavier than needed

errors.py already imports from cpex.framework.models at runtime
(PluginErrorModel, PluginViolation), so there's no circular-import risk in adding
ControlExecutionRecord to that existing line. The added from __future__ import annotations + TYPE_CHECKING guard works but is more machinery than required. Harmless
either way.

[Note — pre-existing] FAF records absent on the exception path

On a normal (non-exception) sequential halt, _build_halt_result schedules fire-and-forget
plugins and their records land in executions; on the violations_as_exceptions=True raise
path, FAF plugins don't fire, so pve.executions won't include them. Not introduced here,
but since the PR makes executions authoritative on the exception path, a one-line note
would keep consumers from assuming parity between the two halt shapes.


Test coverage

Good for the serial case. Gaps:

  • No concurrent-mode test (which would surface the Medium issue above).
  • No regression guard that the non-exception halt path still attaches executions
    correctly.
  • Consider asserting pve.executions is a distinct list from the manager's internal
    accumulator (the list(...) copy), so a later mutation can't leak back.

…onError

When violations_as_exceptions=True and a plugin denies an invocation,
the except PluginViolationError handler in _run_serial_phase previously
re-raised immediately without appending a ControlExecutionRecord for
the denying plugin to the executions accumulator. The same gap existed
for CONCURRENT-mode plugins: _tagged only caught PluginTimeoutError,
so PluginViolationError escaped the as_completed loop before the
record-append and before sibling task cancellation.

This meant PluginResult.executions only contained records for plugins
that ran *before* the denial, making it impossible for consumers to
identify which specific control blocked the invocation from telemetry
alone.

Changes:
- cpex/framework/manager.py (_run_serial_phase): Append the denying
  plugin's ControlExecutionRecord (status=COMPLETED, effective_allow=False,
  matched=True, applied=True, error_code/reason from violation) before
  re-raising PluginViolationError. Covers SEQUENTIAL, TRANSFORM, AUDIT.
- cpex/framework/manager.py (_tagged): Catch PluginViolationError and
  return it as a sentinel alongside the tag (mirrors PluginTimeoutError),
  so it never escapes the concurrent as_completed loop unpaired.
- cpex/framework/manager.py (concurrent as_completed loop): Handle the
  new violation_err sentinel — write the denial record, cancel all sibling
  tasks, and re-raise. Fixes the pre-existing sibling-task leak on this
  path.
- cpex/framework/manager.py (execute): Outer try/except PluginViolationError
  attaches list(executions) to pve.executions before propagating. FAF
  plugins are not scheduled on this raise path (noted in comment).
- cpex/framework/errors.py (PluginViolationError): Add executions attribute
  (list[ControlExecutionRecord] | None, default None). Simplified import —
  ControlExecutionRecord added to existing runtime import line, dropping
  the unnecessary __future__/TYPE_CHECKING guard.
- tests/unit/cpex/framework/test_execution_records.py: Four new tests:
  - test_executions_on_violation_exception_single_plugin
  - test_executions_on_violation_exception_two_plugin_chain
  - test_executions_on_violation_exception_concurrent_plugin (new — covers
    the concurrent-mode gap)
  - test_executions_on_violation_exception_list_is_independent_copy (new —
    asserts pve.executions is a distinct list from the internal accumulator)

Fixes: #147
@prakhar-singh1928
prakhar-singh1928 force-pushed the fix/violation-record-on-exception branch from 88ce985 to fc51c0d Compare August 4, 2026 08:32
araujof added 5 commits August 4, 2026 14:58
Attach executions at the two PluginViolationError raise sites instead of
wrapping the whole pipeline in a try, which tripped ruff PLW0717. Retarget
stale #130 references to #147 and note that FAF records are absent on the
exception path.

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
PRs targeting 0.1.x triggered no checks — every workflow filtered on main
only. Add dev and 0.1.x, the long-lived maintenance base branch for the 0.1
series.

Drop the docs job from the CI pipeline on this branch: docs are built and
published from main only. It has to go from ci.yml rather than docs-build.yaml,
because workflow_call bypasses the called workflow's own branch filter.

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
ci.yml calls lint.yml and tests.yaml via workflow_call, but both also had their
own push/pull_request triggers, so every job ran twice per PR. Make them
reusable-only; ci.yml owns the branch filters.

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
mcp 2.0.0 renamed McpError -> MCPError, breaking the import in
cpex/framework/external/mcp/client.py and erroring out test collection.
Verified McpError is present in 1.26.0 through 1.29.0 and absent in 2.0.0,
so the cap is <2 rather than a tighter pin.

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Clears the two E303 errors that failed the ruff job and the outstanding
ruff format diff. Formatting only — ASTs are unchanged.

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
@araujof

araujof commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review feedback addressed:

  • Concurrent-mode gap closed_tagged now catches PluginViolationError, so the loop writes the deny record, cancels sibling tasks, and re-raises. New concurrent-denial test.
  • #130 references retargeted to #147; errors.py import simplified; FAF-absence on the exception path documented.
  • executions is now attached at the two raise sites instead of wrapping the whole pipeline in a try — fixes ruff PLW0717 and drops ~400 lines of reindentation from the diff. Added a test that pve.executions is an independent copy.

Separately: CI had never run on this branch — every workflow filtered on main only. Enabled dev/0.1.x, deduped jobs that were running twice, capped mcp<2 (2.0 renamed McpError, which broke test collection), and applied ruff format. All 10 checks now green.

@araujof
araujof merged commit 154c8d2 into 0.1.x Aug 4, 2026
10 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in CPEX Aug 4, 2026
@araujof
araujof deleted the fix/violation-record-on-exception branch August 4, 2026 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.1.x bug Something isn't working Python

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[BUG]: PluginResult.executions missing ControlExecutionRecord for denying plugin when violations_as_exceptions=True

3 participants