Skip to content

Commit 76cfb33

Browse files
tercelclaude
andcommitted
fix(executor): address two code-forge:review warnings
Two warnings from the code-forge:review pass on the cleanup-2026-04 round, neither blocker but both worth resolving before push: 1. _translate_abort approval branch is structurally unreachable for the built-in BuiltinApprovalGate (which raises typed Approval{Denied,Timeout,Pending}Error subclasses directly, never aborts via StepResult). The string-matching synthetic-result branch only fired for hypothetical custom approval steps that aborted with explanation strings — undocumented and fragile. Delete the branch and document the contract: custom approval steps MUST raise the typed Approval*Error subclass with a real ApprovalResult, not abort with explanation strings. Drops three now-unused error imports from executor.py. 2. stream() Phase 3 was swallowing post-stream output_validation / middleware_after errors with bare `except: pass`. The "non-fatal for already-yielded chunks" comment is correct (chunks are already out the door), but silent failure makes bad streams undebuggable. Replace with _logger.debug(... exc_info=True) so the failure is still discoverable. Verification: ruff ✓ black ✓ pyright 0 errors ✓ pytest 2252/2252 ✓. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 8818bb5 commit 76cfb33

1 file changed

Lines changed: 16 additions & 14 deletions

File tree

src/apcore/executor.py

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,6 @@
2020
from apcore.context import Context
2121
from apcore.errors import (
2222
ACLDeniedError,
23-
ApprovalDeniedError,
24-
ApprovalPendingError,
25-
ApprovalTimeoutError,
2623
InvalidInputError,
2724
ModuleError,
2825
ModuleExecuteError,
@@ -491,16 +488,13 @@ def _translate_abort(self, abort: PipelineAbortError) -> ModuleError:
491488
if len(pair) == 2:
492489
caller_id, target_id = pair[0].strip(), pair[1].strip()
493490
return ACLDeniedError(caller_id=caller_id, target_id=target_id)
494-
if step == "approval_gate":
495-
from apcore.approval import ApprovalResult
496-
497-
lowered = explanation.lower()
498-
if "rejected" in lowered or "denied" in lowered:
499-
return ApprovalDeniedError(result=ApprovalResult(status="rejected", reason=explanation))
500-
if "timeout" in lowered:
501-
return ApprovalTimeoutError(result=ApprovalResult(status="timeout", reason=explanation))
502-
if "pending" in lowered:
503-
return ApprovalPendingError(result=ApprovalResult(status="pending", reason=explanation))
491+
# Note: there is no `approval_gate` branch here. BuiltinApprovalGate
492+
# raises typed Approval{Denied,Timeout,Pending}Error subclasses
493+
# *directly* (see builtin_steps.BuiltinApprovalGate.execute), so the
494+
# error never reaches this translation path. Custom user-supplied
495+
# approval steps MUST follow the same contract: raise the typed
496+
# Approval*Error subclass with a real ApprovalResult — do NOT abort
497+
# via StepResult(action="abort", explanation="...rejected...").
504498
if step == "input_validation" and "validation failed" in explanation.lower():
505499
return SchemaValidationError(message=explanation)
506500
if step == "output_validation" and "validation failed" in explanation.lower():
@@ -682,7 +676,15 @@ async def stream(
682676
try:
683677
await self._pipeline_engine.run(post_strategy, pipe_ctx)
684678
except Exception:
685-
pass # Post-stream validation errors are non-fatal for already-yielded chunks
679+
# Non-fatal: chunks have already been yielded to the caller,
680+
# so a failed post-stream validation/middleware run cannot
681+
# change the observable output. Log at DEBUG so the failure
682+
# is still discoverable when investigating bad streams.
683+
_logger.debug(
684+
"Post-stream validation/middleware failed for module %s",
685+
module_id,
686+
exc_info=True,
687+
)
686688

687689
# _execute_async removed in v0.17 (replaced by BuiltinExecute pipeline step)
688690

0 commit comments

Comments
 (0)