fix: correct error-retry classification and resume checkpoint safety - #401
Open
prithvee07 wants to merge 8 commits into
Open
fix: correct error-retry classification and resume checkpoint safety#401prithvee07 wants to merge 8 commits into
prithvee07 wants to merge 8 commits into
Conversation
prithvee07
commented
Jul 29, 2026
prithvee07
commented
- classifyErrorForTemporal now honors a PentestError's explicit retryable flag when no ErrorCode is set, instead of letting message substring matching override author intent (was flipping intentionally retryable queue-validation failures to non-retryable, and vice versa for unrecoverable preflight config errors).
- runPiPrompt's billing/session-limit text classification now only applies to turns the harness itself flags as errors, not every successful turn's assistant text — legitimate pentest report prose (e.g. "no usage limit enforced") could otherwise discard a successful, paid-for agent run.
- runPiPrompt's catch block now preserves a caught PentestError's code and retryable fields instead of rebuilding them from generic string heuristics, restoring the billing/spending-cap retry path.
- restoreGitCheckpoint now only treats a checkpoint-existence check failure as "checkpoint missing" for git's actual missing-object errors; any other failure (lock exhaustion, FS error) propagates instead of silently skipping the reset/clean/deliverable-cleanup steps during resume.
- classifyErrorForTemporal now honors a PentestError's explicit retryable flag when no ErrorCode is set, instead of letting message substring matching override author intent (was flipping intentionally retryable queue-validation failures to non-retryable, and vice versa for unrecoverable preflight config errors). - runPiPrompt's billing/session-limit text classification now only applies to turns the harness itself flags as errors, not every successful turn's assistant text — legitimate pentest report prose (e.g. "no usage limit enforced") could otherwise discard a successful, paid-for agent run. - runPiPrompt's catch block now preserves a caught PentestError's code and retryable fields instead of rebuilding them from generic string heuristics, restoring the billing/spending-cap retry path. - restoreGitCheckpoint now only treats a checkpoint-existence check failure as "checkpoint missing" for git's actual missing-object errors; any other failure (lock exhaustion, FS error) propagates instead of silently skipping the reset/clean/deliverable-cleanup steps during resume.
- forwardEtcHostsFlags no longer brackets IPv6 literals for --add-host; Docker's flag parser splits on the first colon and validates the remainder as a bare IP, so a bracketed literal was rejected outright, breaking scan startup for anyone with a non-loopback IPv6 /etc/hosts entry. - validateAuthentication now wraps its state-file cleanup and prompt loading in try/catch and returns Err instead of letting a thrown error escape as a raw promise rejection, honoring its Promise<Result<void, PentestError>> contract. - findings-renderer skips and logs queue entries missing ID or vulnerability_type instead of rendering literal "undefined: undefined" into the customer-facing report. - restoreGitCheckpoint's incompleteAgents is now derived from the run's actual expectedAgents scope instead of ALL_AGENTS, so a class-scoped resume doesn't target out-of-scope agents for git-clean/deletion. - WorkflowLogger is now cached and reused per session (getOrCreateWorkflowLogger/ closeWorkflowLogger) instead of every AuditSession opening its own WriteStream onto the same workflow.log; closed once at the true end-of-workflow point in logWorkflowComplete. - Credential config writes now chmod 0o600 explicitly after write, since Node ignores the writeFileSync mode option when overwriting an existing file. - migrateLegacyWorkspaceLayout now stages renames in a temp directory and renames it into place as one atomic step, so a failure partway through can no longer leave the internalPath existence check permanently (and silently) skipping migration on every future run.
Improve error classification and retry logic for transient failures
- task-tool.ts: replace Model<any> with Model<Api>, matching the upstream pi-ai Api union instead of suppressing the any-check. - exploit-renderer.ts: use String.charAt(0) instead of value[0]! — charAt always returns a string, so it drops the non-null assertion without introducing the "undefined" + rest bug that optional chaining (value[0]?.) would have on an empty/out-of-range index.
chore: clean up remaining biome lint warnings
…iew pass - resolveRepo now rejects bare repo names that resolve outside ./repos/ (e.g. containing ../), which could otherwise bind-mount an arbitrary host directory into the ephemeral worker container. - assembleReportActivity now always renders per-class findings from the exploitation queue before assembling the report, not only when exploit=false. Previously, a partial run with exploit=true whose exploit agent failed for one class would have neither an evidence nor a findings file for that class, so assembleFinalReport silently omitted it and the report claimed no vulnerabilities were found despite confirmed queue findings on disk. renderFindingsFromQueues now skips a class whenever its evidence file already exists, so this is a no-op for classes whose exploit agent succeeded. - checkDeprecatedFields now rejects a top-level 'login' config block. The schema accepted it silently (deprecated:true is documentation only, not enforced) and distributeConfig never read it, so a legacy config using the old block resolved authentication to null and ran the scan unauthenticated with no indication why. - rollbackGitWorkspace's git reset --hard is now scoped to the provided per-agent paths (via targeted reset + checkout, since reset --hard has no path-scoping form), matching the git clean call beside it. Previously only the untracked-file cleanup was scoped; the tracked-file reset always reverted the entire shared deliverables repo, so a failing agent's retry could silently discard a concurrent sibling agent's uncommitted work during the parallel vuln/exploit phase.
- MetricsTracker.recalculateAggregations now includes every agent's cost (and an effective duration when final_duration_ms was never set) regardless of status, instead of only 'success' agents. A permanently-failed agent's real, already-billed spend was being dropped from the cumulative cost surfaced to the user. - login-instructions.txt now defines API and BASIC sections (the template previously only covered FORM/SSO). login_type: api/basic are two of the four documented login types but got no login instructions at all, since the fallback-to-full-template guard never fires while COMMON/VERIFICATION are present. - config-schema.json's top-level anyOf now includes pipeline, so a config containing only pipeline settings (e.g. for subscription rate-limit retry behavior) no longer fails validation. Mirrored in config-parser.ts's hasAnySteering check so it doesn't warn about "no steering fields" for a pipeline-only config either. - checkForDuplicates/checkForConflicts now compare rules using the same normalization sanitizeRule applies later (trimmed value, trimmed+lowercased type), so a duplicate/conflict hiding behind incidental whitespace no longer bypasses the check. - performSecurityValidation now checks authentication.success_condition.value against DANGEROUS_PATTERNS, matching the other authentication fields rendered into the same login prompt templates. - formatDuration now rounds before branching on the 60s boundary, so a duration like 59.95s no longer displays as "60.0s" — it correctly crosses into the "1m 0s" branch instead. - AuditSession.initialize's session.json creation path is now protected by the same sessionMutex every other session.json mutator uses, closing a gap that was dormant only because of the current phase-ordering assumption (pre-recon is the sole non-parallel first agent).
- executeGitCommandWithRetry's retry-exhaustion PentestError was
unreachable dead code — a lock error on the final attempt still hit
the raw `throw error` branch. Restructured so exhausting retries on
a genuine lock conflict now surfaces the classified, retryable
error instead of the raw git/zx failure.
- DANGEROUS_PATTERNS in config-parser.ts was over-broad: /data:/ and
/file:/ matched inside ordinary words ("metadata:", "profile:"),
and /[<>]/ rejected any bare '<' or '>' (e.g. "count > 5"). Patterns
now require a word boundary before URL schemes and require '<' to
look like an actual tag/comment open, eliminating the false
positives while still catching data:/javascript:/file: URLs and
HTML/XML injection.
- processIncludes now resolves @include() directives recursively (with
a depth guard against circular includes), so a shared prompt partial
can itself use @include() without silently leaving the literal,
unresolved directive text in the assembled prompt.
- save-deliverable.ts now writes via the shared atomicWrite (temp file
+ rename) instead of a direct writeFileSync, matching the
crash-safety pattern used for every other deliverable/state write
in the codebase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.