feat(claude-lane-outcome): project error_type on substring classification

[kyle-sexton]

Summary

Add an additive error_type key to the claude-lane-outcome classifier projection. On the substring path it carries the matched published error-type allowlist token; on numeric-status and unmatched paths it is null. class and api_error_status semantics are unchanged.

Retargets #253 away from the retired bash/codegen paths onto .github/actions/claude-lane-outcome/classify.cjs.

Test plan

• node --test .github/actions/claude-lane-outcome/classify.test.cjs
• CI green on this PR

Related

Fixes #253

  

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 2m 5s —— View job

---

Claude finished the review

Todo list

• Gather context (CLAUDE.md, REVIEW.md, PR diff)
• Review classify.cjs changes
• Review classify.test.cjs changes
• Post findings

Summary

The change refactors the three duplicated classFromErrorText type-string checks into a single ERROR_TYPE_ALLOWLIST array + matchErrorType() lookup, and threads the matched token through as an additive error_type field on the substring path only. The refactor preserves class/api_error_status semantics: the allowlist iterates in the same order as the old if/else chain (auth ×3 → rate-limit → overloaded ×3 → other), so first-match-wins behavior is unchanged. Centralizing the lookup is a good move — it removes the risk of class and error_type ever drifting from two separate matches against the same text, since both now come from one matchErrorType() call.

The public-log-hygiene invariant documented at the top of the file (only the class token and structured projection ever leave the module, never raw model/SDK text) is upheld: error_type is only ever one of the closed allowlist's own literal type strings, never free text from errors[].

One finding posted inline:

• 🟡 classFromErrorText (lines 82-84) is dead code left over from the refactor — classifyExecutionFile now calls matchErrorType directly, and nothing else references the wrapper.

Not independently verified

The author's test-plan checkbox for node --test .github/actions/claude-lane-outcome/classify.test.cjs is author-claimed; I did not have permission to execute Bash commands in this review session (node --test ... and file writes outside the repo were both blocked), so I reviewed the test additions and the classifier logic by reading, not by running the suite. Reading through it, the new test (lines 202-219) correctly exercises all three cases named in the PR description (substring match carries the token, numeric-status path is null, unmatched substring path is null) and looks consistent with matchErrorType's behavior.

Branch: cursor/issue-253-error-type-9b36

[claude]

Claude finished @kyle-sexton's task in 52s —— View job

---

Security Review — PR #408

• Gather context (PR diff, changed files)
• Read classify.cjs and classify.test.cjs
• Analyze for logic/authz/injection/secrets-handling/workflow-permission issues
• Post findings

Scope: .github/actions/claude-lane-outcome/classify.cjs and .github/actions/claude-lane-outcome/classify.test.cjs (the only files changed in this PR).

Findings: none.

This change adds an error_type field to the classifier's projection, sourced exclusively from the closed ERROR_TYPE_ALLOWLIST (classify.cjs#L63-L71) — a fixed set of literal strings, never raw model/SDK text. The module's stated invariant (module header, classify.cjs#L7-L11) is that only the class token and structured projection ever leave this module, since the repo is public and result/errors[] may contain model-authored free text or raw stacks. That invariant is preserved:

• matchErrorType (classify.cjs#L73-L80) only ever returns errorType values taken verbatim from the allowlist array — it does not extract or echo any substring of the attacker/model-controlled text itself, so there's no reflection/leak path for arbitrary content in errors[] to reach the output.
• The new test (classify.test.cjs#L207-L219) plus the existing CANARY guard in the test corpus (classify.test.cjs#L15-L18) exercise all three paths (numeric-status, matched substring, unmatched) and confirm error_type is null off the substring path and never carries the canary/free-text.
• error_type is derived with a plain String.includes substring check against fixed literal patterns — no regex, no dynamic construction from input, so no ReDoS or injection surface.
• No changes to permissions, triggers, secrets handling, or any workflow YAML — this PR touches only the classifier module and its unit tests.

I found no logic flaws, authorization gaps, injection surfaces, or secret-handling issues in this diff.

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/ci-workflows/actions/runs/31546845372
• Failure class: other
• Last SDK result: (no execution file was produced)

The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging.

Re-run the job to retry the review; a new push also retries it only if the caller's pull_request triggers include synchronize (the canonical security caller keeps it). An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

• rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
• a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

[github-actions]

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.