feat: configurable on_error for ranking stages - #206
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is fully reviewed and the only remaining comment is a minor documentation nit.
Review effort: Lite
Findings: None
What changed in this PR
Adds configurable on_error: skip | fail handling for ranking stages, with failure classification, exit codes 6/7, JSON reporting, tests, and documentation updates.
Changes:
- Validates per-stage error policies.
- Records and reports stage failures.
- Updates CLI output, tests, and architecture documentation.
| File | Summary |
|---|---|
test/e2e_pipeline_test.go |
Adds end-to-end coverage. |
README.md |
Documents configuration and behavior. |
internal/config/stage.go |
Parses and validates on_error. |
internal/config/pipeline_test.go |
Tests configuration handling. |
internal/cli/cli.go |
Adds exit-code and JSON failure handling. |
internal/cli/cli_test.go |
Tests CLI behavior. |
internal/analysis/stages.go |
Applies stage failure policies. |
internal/analysis/stages_test.go |
Tests stage policies. |
internal/analysis/stage/stage.go |
Defines stage error kinds. |
internal/analysis/stage/cosine.go |
Classifies cosine-stage failures. |
internal/analysis/stage/cosine_test.go |
Tests cosine failure classification. |
internal/analysis/pipeline_test.go |
Tests pipeline failure behavior. |
internal/analysis/engine.go |
Records and reports stage failures. |
docs/arch/0022-candidate-scoring-pipeline.md |
Documents pipeline error handling. |
docs/arch/0014-json-check-output.md |
Documents JSON failure output. |
CLAUDE.md |
Updates project guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
7 tasks
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
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.
Summary
Adds a per-stage
on_error: skip | failto therankandrerankranking stages, so users can make a check fail when a ranking stage can't complete, and CI can tell an unavailable dependency (exit 6) from an unmet precondition (exit 7). The default is unchanged: a failed stage skips its file and the run exits 0.Related issue
Closes #200
In scope
on_error: skip | failonrankandrerank; any other value fails at config load (ExitConfig) naming the stage and key.stage.Kindonstage.Error(unavailable, the zero value, orprecondition_not_met). Cosine's embedding failure isunavailable; cosine with no embedding provider isprecondition_not_met.fail, the engine records aStageFailure(stage, file, kind, error), stops that file's remaining stages, and lets other files run.6(unavailable) and7(precondition not met, wins when both occur).check --format jsongains afailuresarray (omitted when empty, so default output is unchanged); error text goes to stderr.CLAUDE.md, and ADRs 0014 and 0022 updated.Out of scope
on_errordefaults (separate tickets under epic: configurable ranking and screening pipeline #196).on_error, as ADR 0022 and the CI policy already document.Architectural notes
Deviations from "identical behavior" and decisions made along the way:
count.--update-baselinewith afailfailure returns 6 or 7 and does not write the baseline, so a partial snapshot can't replace a good one.fail, the error line readsError: stage <name> failed for <file> (<kind>): <err>and doesn't repeat the action (generating embedding); the underlying error text is included. The skip path's message is unchanged.precondition_not_metcase for cosine ("no embedding provider configured") is unreachable through the real binary, which always builds an embedder. It is covered at unit/engine level; exit 7 has no e2e coverage.docs/arch/0022-candidate-scoring-pipeline.md(amended) rather than a new ADR, since it extends that pipeline's error handling.Follow-ups
None filed.
QA / testing
Verified locally:
go test ./...passes (including the e2e suite that builds and runs the mock binary).golangci-lintv2.12.2 in a Linux container: 0 issues (the local Windows binary panics on a go1.27 dependency, so it isn't used).\rstripped.go test -racein a Linux container forinternal/analysis/...,internal/cli/...,internal/config/....5a6eb82) vs this branch on identical fixtures (no pipeline, pipeline withouton_error, drift, embed failure, text and JSON): stdout, stderr and exit codes identical; JSON never gains afailureskey.on_error: skipbehaves the same as unset.fail: embed failure exits 6, drift plus failure exits 6,--update-baselineplus failure exits 6 with no baseline written.Not verified here, needs CI:
go test -race -cover ./...including the Docker-backedinternal/indextests (only the packages above were run under-race).Checklist
feat:,fix:,docs:,refactor:,build(deps):, etc.)go test -race -cover ./...passes (partial locally; full run left to CI)golangci-lint run --timeout=5mis cleanCLAUDE.mdupdated if this changes build/test commands, adds or renames a top-level package, changes a cross-package interface, or adds a footgundocs/arch/if this embodies an architecturally-significant decision (existing ADR 0022 amended instead)🤖 Generated with Claude Code