Repository navigation
SecurityError from Workspace#add_artifact bypasses every rescue between the LLM and the plan record #31
Description
Summary
Workspace#add_artifact raises SecurityError for a bad artifact name (traversal today, symlink escape after #30). SecurityError inherits from Exception, not StandardError, so neither rescue on the path from LLM output to the plan record catches it:
FileGenerationCapability.executewraps each artifact inrescue => ewith the comment "Continue with other artifacts" (lib/agentic/capabilities/file_generation_capability.rb:138). AStandardErrorfrom one artifact skips it and continues; aSecurityErroraborts the whole task.PlanOrchestrator#execute_taskhas the same shape atlib/agentic/plan_orchestrator.rb:630(rescue => e→TaskFailure.from_exception, "unexpected_error"). ASecurityErrorbypasses it, so the task gets no failure record, and because the task runs underAsync::Barrier, the exception re-raises out ofbarrier.waitinexecute_plan.
Net: one LLM-chosen name like ../notes.md in a five-file generation doesn't cost one file, it takes down the plan run with no execution record, which is the opposite of what Direction asks for ("better failure recovery, better execution history"). Verified the class hierarchy (SecurityError.ancestors.include?(StandardError) → false); the rescue shapes are by inspection.
Why this is a contract call, not a drive-by fix
The capability's own docs say @raise [SecurityError] If an artifact fails workspace security validation, and the inline "continue" comment says the opposite. One of them is the contract. Options:
- Swallow at the artifact level.
rescue SecurityError, StandardError => ein the capability loop: the bad artifact is logged and skipped, the rest of the generation proceeds. Update the@raisedoc. The orchestrator'srescue => eshould also becomerescue SecurityError, StandardErrorso any futureSecurityError(e.g. fromSecurity::Sanitizercontent checks, which raise the same class) becomes a recordedTaskFailureinstead of a crash. - Keep propagating, but as a
StandardError. DefineAgentic::Errors::ArtifactSecurityError < StandardError(or reuse an existing error), raise that fromWorkspace, keepSecurityErroronly for the sanitizer's content checks. Bigger surface:add_artifact's documented raise changes class, so consumers rescuingSecurityErrorneed a deprecation window.
Either way the orchestrator-level rescue is the safety net the plan record depends on.
Class: public-api-change (L0)
Minerva's lean: Option 1 — widen both rescues to SecurityError, StandardError, keep the raise class as is, and fix the doc; a bad file name from the model is a per-artifact failure, not a reason to lose the plan.
Origin: loop:security session 2026-09-14, surfaced by the adversarial panel on #30 (gem-consumer and Obie seats). Checked closed/deferred issues: nothing on error-class propagation; #10 (stranded dependents) is adjacent but about :pending state after a recorded failure, not an unrecorded one.
Activity
minerva-sky commentedon Sep 24, 2026
Option 1 is up as #50: both rescues now name
SecurityErroralongsideStandardError,Workspace#add_artifactstill raisesSecurityError, and the capability doc now says skip-and-continue instead of@raise. Specs cover a traversal-named artifact being skipped with the rest written, and the orchestrator recording aSecurityErroras a failed task instead of escapingexecute_plan.Fail closed on SecurityError in FileGenerationCapability