diff --git a/CLAUDE.md b/CLAUDE.md index 7e50a36..4a3046c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,7 +27,7 @@ CI (`.github/workflows/ci.yml`) runs `golangci-lint`, then `go test -v -race -co Execution flow, in order: `cmd/archguard/main.go` → `internal/cli.Execute` → `internal/analysis.Engine.Run`. - **`internal/cli`**: Parses args, finds the git root and `chdir`s into it (all paths are resolved relative to repo root, not cwd), loads `archguard.yaml` via `internal/config`, constructs the configured `llm.Provider`, and dispatches to `init` / `index` / `check`. `Execute` takes a `ProviderFactories{Chat, Embed}` struct as its test injection point (zero value in production) — `cmd/archguard-e2e/main.go` is a second binary entrypoint that supplies two distinct `llm.MockProvider` instances (one per role) so `test/e2e_test.go` can build and exec that binary as a subprocess instead of hitting real LLM APIs or Ollama, including for configs where `llm.provider` and `vector_store.provider` differ. `resolveEmbedProviderInstance` reuses the chat provider for embedding when the two roles share a provider name; otherwise it calls `factories.Embed`, or fails fast at `ExitConfig` if `factories.Embed` is nil — the same fail-fast contract the real-provider path already has via `buildProvider`'s error return. `check --format json` (default `text`) prints a single JSON document to stdout with everything else (banner, debug/progress text, notices) routed to stderr instead; see `docs/arch/0014-json-check-output.md` for the `Engine.Writer`/`JSONOutput` plumbing and the `checkWantsJSON` pre-parse that suppresses the startup banner before `checkFlags.Parse` runs. -- **`internal/config`**: Defines `Config` (YAML-backed). `analysis.pipeline` (`Pipeline` in `pipeline.go`, `StageConfig` in `stage.go`) declares optional `rank` and `rerank` stages, each with `scorer` (`cosine`, the default), `threshold` (0-1) and `top_k` (positive); `Pipeline.UnmarshalYAML` rejects an unrecognized stage or stage key, and every bad value fails inside `LoadConfig` so it exits `ExitConfig` naming the stage and key. `Analysis.RelevantADRLimit()` is the one place `max_relevant_adrs` defaults to 3. `ARCHGUARD_DB_URL` env var overrides `vector_store.connection_string`; `ARCHGUARD_API_KEY` supplies the chat provider's (`llm.provider`) API key, and `ARCHGUARD_EMBEDDING_API_KEY` supplies the embedding provider's (`vector_store.provider`) key when it differs from the chat provider — both read in `cli`, not `config` (see `docs/arch/0004-decoupled-chat-and-embedding-providers.md`). +- **`internal/config`**: Defines `Config` (YAML-backed). `analysis.pipeline` (`Pipeline` in `pipeline.go`, `StageConfig` in `stage.go`) declares optional `rank` and `rerank` stages, each with `scorer` (`cosine`, the default), `threshold` (0-1), `top_k` (positive) and `on_error` (`skip`, the default, or `fail`); `Pipeline.UnmarshalYAML` rejects an unrecognized stage or stage key, and every bad value fails inside `LoadConfig` so it exits `ExitConfig` naming the stage and key. `Analysis.RelevantADRLimit()` is the one place `max_relevant_adrs` defaults to 3. `ARCHGUARD_DB_URL` env var overrides `vector_store.connection_string`; `ARCHGUARD_API_KEY` supplies the chat provider's (`llm.provider`) API key, and `ARCHGUARD_EMBEDDING_API_KEY` supplies the embedding provider's (`vector_store.provider`) key when it differs from the chat provider — both read in `cli`, not `config` (see `docs/arch/0004-decoupled-chat-and-embedding-providers.md`). - **`internal/index`**: ADR ingestion and the vector store. - `Provider` interface fetches ADRs from a source; `LocalProvider` reads `analysis.adr_path` from disk, `ConfluenceProvider` pulls from Atlassian Confluence (REST API v2). `CompositeProvider` fans out to all configured providers concurrently and merges results — a single provider failing doesn't fail the run unless *all* fail. `GetADRs` returns `([]ADR, FetchStats, error)`; `FetchStats` (discovered/parse-failed/status-rejected counts) feeds `internal/index/health.go`'s `summarizeCorpus`, which also detects duplicate ADR IDs and unscoped ADRs post-merge, for `archguard index`'s corpus-health summary. See `docs/arch/0010-index-corpus-health-reporting.md`. As of `docs/arch/0012-configurable-adr-id-extraction-pattern.md`, `LocalProvider`'s filename-based ADR ID extraction (default: split on the first hyphen) can be overridden via `analysis.adr_id_pattern`, compiled once at startup and set on the provider via `SetIDPattern`. As of `docs/arch/0021-configurable-frontmatter-field-mappings.md`, `analysis.frontmatter_mappings` (canonical field name -> the corpus's actual YAML key, for `title`/`status`/`scope`/`similarity_threshold`) is validated once at startup (`internal/cli.validateFrontmatterMappings`, `ExitConfig` on an unknown field or a source-key collision) and set on `LocalProvider`/`ConfluenceProvider` via `SetFrontmatterMappings`, mirroring `SetIDPattern`; `ParseADRContent` (shared by both providers) decodes frontmatter through a generic `map[string]yaml.Node` instead of `FrontMatter`'s fixed yaml tags whenever a mapping is configured, so a field left unmapped keeps reading its canonical key unchanged. - `VectorStore` interface (`LocalStore` = JSON file at `.archguard/index.json`; `PgStore` = Postgres + pgvector with HNSW) is chosen by `NewVectorStore(cfg, w)` based on whether `vector_store.connection_string` is set. `ScopedADRs(filePath)` returns the scope-matched ADRs unscored and needs no embedding; it is how the scoring pipeline gets its candidates. `PgStore` caches the project's ADR rows after the first call and `BuildIndex` drops the cache, so a check run reads the corpus once rather than once per file (an `archguard index` run by another process isn't seen until the next run). `Search(queryEmbedding, threshold, topK, filePath)` applies ADR `scope` as a structural filter *before* ranking/limiting to `topK`, not after -- both backends funnel through shared `filterByScope`/`rankAndLimit` helpers in `internal/index/rank.go` so they can't diverge; `PgStore` fetches up to a `MaxSearchCandidates` (1000) cap per query instead of `topK` directly, since ADR scope is a `doublestar` glob Postgres can't evaluate in SQL. See `docs/arch/0007-scope-filtered-before-topk-similarity.md`. `LocalStore`/`PgStore` route progress and warning output through an unexported `writer io.Writer` field (nil defaults to `os.Stdout`, resolved dynamically per write via the package-level `diagWriter` helper) rather than hardcoding `os.Stdout` -- `NewPgStore` takes it as a constructor parameter since it prints during construction, `NewVectorStore(cfg, w)` forwards it to whichever backend it builds. `LocalProvider`/`ConfluenceProvider`/`CompositeProvider` have the same field plus a `SetWriter` method, mirroring `SetIDPattern`. `cli.runCheck` and `cli.runIndex` (which takes a `w io.Writer` parameter) pass `human`/`os.Stdout` through so `check --format json` stays valid JSON on stdout even when it triggers an index rebuild or a provider-fetch warning. See `docs/arch/0015-index-diagnostic-writer.md`. `SearchRejected` mirrors `Search` but returns scope-matched candidates scoring *below* threshold instead of at-or-above it, for `--debug` diagnostics only -- nothing on the engine path calls it, since `CosineRanker` uses `SearchWithDebugInfo`. See `docs/arch/0009-debug-visibility-for-rejected-adr-candidates.md`. `SearchTruncated` is `SearchRejected`'s sibling: it returns scope-matched, threshold-passing candidates that `rankAndLimit` cut purely for exceeding `topK`, via the same `truncatedByTopK` complement-of-`rankAndLimit` helper in `internal/index/rank.go` -- also `--debug`-only. `SearchWithDebugInfo` issues a single query and derives `hits`/`rejected`/`truncated` from that one candidate set instead of three independent queries, since two separate approximate HNSW queries under `hnsw.iterative_scan=relaxed_order` aren't guaranteed to agree -- `CosineRanker` calls this instead of `Search`+`SearchRejected`+`SearchTruncated` whenever `--debug` is set. See `docs/arch/0019-single-query-consistency-for-debug-diagnostics.md`. As of `docs/arch/0011-per-adr-similarity-threshold-override.md`, an ADR's frontmatter may set its own `similarity_threshold`, resolved by `index.EffectiveThreshold` and applied via the shared `meetsThreshold` predicate both filters key off -- so a per-ADR override needs no separate call-site wiring in either backend. Glob matching itself (`MatchGlob`, `doublestar` under the hood) lives in `internal/index` (not `internal/analysis`, which imports `index` and would create a cycle otherwise) and is reused by `internal/analysis` for its unrelated `exclude_patterns` check. @@ -39,7 +39,7 @@ Execution flow, in order: `cmd/archguard/main.go` → `internal/cli.Execute` → - **`internal/analysis`**: The core engine (`engine.go`). - `ContentProvider` abstracts *which* files to scan and how to read them: `UncommittedProvider` (default), `StagedProvider` (`--staged`), `AllProvider` (`--all` or a `.` path arg), `MultiFileProvider` (one or more explicit path args). - `Engine.Run` fans out over files with a bounded worker pool (`analysis.max_concurrency`, default 5) via `errgroup`. Per file: fetch context → strip unified-diff patch metadata (`stripDiffMetadata`, only when the content actually came from `GetDiff` — never applied to the whole-file-content fallback, since heuristically detecting "is this a diff" on arbitrary file content risks false positives) → build candidates (`VectorStore.ScopedADRs`, minus `archguard-ignore: ` suppressions) → run the scoring stages (see below), where the default cosine stage takes its `topK` from `analysis.max_relevant_adrs` (default 3 when unset or `<= 0`) — see `docs/arch/0007-scope-filtered-before-topk-similarity.md` and `docs/arch/0020-configurable-topk-adr-limit.md` → call the LLM for each remaining ADR. Outside `--debug`, `CosineRanker` calls `Search` with an unbounded `topK` (scope and threshold already applied) and the stage applies the top-K cut. In `--debug` mode it calls `VectorStore.SearchWithDebugInfo` instead, which derives `hits` and `rejected` from one query's candidate set (not independent calls to `Search`/`SearchRejected`/`SearchTruncated`) so rejected ADRs keep real scores; the stage prints each below-threshold ADR's title/score ("Below threshold", capped at `MaxKeep`) and each top-K-cut ADR's title/score/rank/qualifying-count ("Cut by top-K limit"), and `candidateSource` prints "Skipping ADR ... (Suppressed)" for every suppressed scope-matched ADR before them -- see `docs/arch/0009-debug-visibility-for-rejected-adr-candidates.md` (and its amendment for #190) for the diagnostic format, and `docs/arch/0019-single-query-consistency-for-debug-diagnostics.md` for why the three independent-query approach was replaced. - - **Scoring pipeline** (`internal/analysis/stage`): a `stage.Scorer` returns one score per `Candidate` for a file, on the scale of its stage's `Threshold` (0-1 for screening models, raw cosine similarity for `CosineRanker`), for all candidates in one call, given a `File` and a `Debug`; a `stage.Stage` wraps a scorer with a `Threshold` (minimum score, optionally per ADR) and `MaxKeep`, dropping, ordering, and cutting by score. `Engine.Stages` defaults to one `stage.NewCosineStage` around `CosineRanker` (`cosine.go`); `cli.runCheck` overrides it with `analysis.BuildStages` (`stages.go`) when `analysis.pipeline` is set and gets nil otherwise, so no block means the default. `BuildStages` gives `rank` the legacy fallbacks (`vector_store.similarity_threshold`, `analysis.max_relevant_adrs`) and `rerank` a threshold of 0 and `top_k` of 3 with a printed warning per unset key (to `human`, so stderr under `--format json`); a `rerank` alone runs after the default `rank`. `CosineRanker` owns the embed call (through `llm.Embedder`) and the store query. `candidateSource` builds each file's candidates from `VectorStore.ScopedADRs` (scope-matched, no embedding needed) minus `archguard-ignore`-suppressed ADRs, so suppressed ADRs never reach a scorer or occupy a top-K slot. A pipeline without `CosineRanker` makes no embedding calls. A scorer failure surfaces as a `stage.Error` whose `Action` the engine prints (`generating embedding` for cosine); a failure loading candidates (for example Postgres unreachable) skips the file with `Error loading candidate ADRs for ` rather than passing it. The LLM remains the only source of violations. See `docs/arch/0022-candidate-scoring-pipeline.md`. + - **Scoring pipeline** (`internal/analysis/stage`): a `stage.Scorer` returns one score per `Candidate` for a file, on the scale of its stage's `Threshold` (0-1 for screening models, raw cosine similarity for `CosineRanker`), for all candidates in one call, given a `File` and a `Debug`; a `stage.Stage` wraps a scorer with a `Threshold` (minimum score, optionally per ADR) and `MaxKeep`, dropping, ordering, and cutting by score. `Engine.Stages` defaults to one `stage.NewCosineStage` around `CosineRanker` (`cosine.go`); `cli.runCheck` overrides it with `analysis.BuildStages` (`stages.go`) when `analysis.pipeline` is set and gets nil otherwise, so no block means the default. `BuildStages` gives `rank` the legacy fallbacks (`vector_store.similarity_threshold`, `analysis.max_relevant_adrs`) and `rerank` a threshold of 0 and `top_k` of 3 with a printed warning per unset key (to `human`, so stderr under `--format json`); a `rerank` alone runs after the default `rank`. `CosineRanker` owns the embed call (through `llm.Embedder`) and the store query. `candidateSource` builds each file's candidates from `VectorStore.ScopedADRs` (scope-matched, no embedding needed) minus `archguard-ignore`-suppressed ADRs, so suppressed ADRs never reach a scorer or occupy a top-K slot. A pipeline without `CosineRanker` makes no embedding calls. A scorer failure surfaces as a `stage.Error` whose `Action` the engine prints (`generating embedding` for cosine) and whose `Kind` is `KindUnavailable` (the default: a dependency didn't respond) or `KindPreconditionNotMet` (the stage can't run, e.g. cosine with no embedding provider); by default the file is skipped, but a stage with `on_error: fail` (`Stage.FailOnError`, named by `Stage.Name`, `rank` or `rerank`) instead appends a `StageFailure` to `Engine.StageFailures`, stops that file's remaining stages, and lets other files run; `cli.runCheck` maps the failures to `ExitStageUnavailable` (6) or `ExitStagePrecondition` (7, which wins when both occur), ahead of `ExitDriftDetected`, and `--update-baseline` returns that code without writing the baseline; `check --format json` adds a `failures` array (omitted when empty); a failure loading candidates (for example Postgres unreachable) skips the file with `Error loading candidate ADRs for ` rather than passing it. The LLM remains the only source of violations. See `docs/arch/0022-candidate-scoring-pipeline.md`. - **Context sizing** (`fetchContext`): counts the file against `llm.max_tokens` via `Provider.CountTokens` — each provider counts using its own real tokenizer/backend rather than one hardcoded encoder (see `docs/arch/0002-provider-scoped-token-counting.md`). If it fits, uses the content whole; otherwise prefers a diff (if available) over truncation (`truncateToTokenLimit`), which estimates a byte cutoff and verifies/shrinks it via further `CountTokens` calls, guaranteeing the result stays within budget. **Smart Truncation** rolls truncated content back to the nearest preceding newline so files aren't cut mid-line. - **`Engine.EmbedProvider`**: optional; when set, embedding goes through it (an `llm.Embedder`) instead of `Provider`. `internal/cli.Execute` sets this whenever `vector_store.provider` names a different provider than `llm.provider` (required when `llm.provider` is `claude`, since Claude has no embeddings API — see `docs/arch/0004-decoupled-chat-and-embedding-providers.md`). - **Caching**: LLM analysis results are cached in `.archguard/cache/.json`, keyed by model name + ADR content + file content + system prompt + prompt template (`internal/cache`). A cache hit skips the LLM call entirely. @@ -59,7 +59,7 @@ Execution flow, in order: `cmd/archguard/main.go` → `internal/cli.Execute` → - Architectural Decision Records for this codebase's own design live in `docs/arch/` — check there before making a design decision that might already be settled. - Tests are table-driven where the cases share shape, and provider tests (`internal/llm/*_test.go`) mock the backend with `net/http/httptest` rather than hitting a live API — no live network calls in unit tests. `test/e2e_test.go` builds and execs the `cmd/archguard-e2e` binary (which wires `cli.Execute` to a `MockProvider` factory) rather than calling `cli.Execute` in-process. - Conventional Commits for commit messages (`feat: ...`, `fix: ...`, `docs: ...`, `refactor: ...`, `build(deps): ...`). -- Exit codes are meaningful and tested: `0` success, `1` general error, `2` usage, `3` config, `4` drift detected, `5` index error — preserve these in `internal/cli` if you touch command dispatch. +- Exit codes are meaningful and tested: `0` success, `1` general error, `2` usage, `3` config, `4` drift detected, `5` index error, `6` a stage with `on_error: fail` hit an unavailable dependency, `7` a stage with `on_error: fail` hit an unmet precondition (the default `on_error: skip` skips the file and exits `0`) — preserve these in `internal/cli` if you touch command dispatch. - **Code comments: default to zero. When one's genuinely needed, it's 1-2 lines, never more.** Only write a comment for a non-obvious WHY (a hidden constraint, a workaround, a subtle invariant) — never to restate what the code does, narrate the task/fix that produced it, or read like a tutorial. A comment over 2 lines is bloat by default; treat that length as a hard signal to cut it down or delete it, not a reason to search for a justification. Write it plainly enough that a non-expert could follow the words even if the concept is technical. This applies everywhere, not just new code — trim a bloated comment you touch in passing. It's a deliberate rule against the tendency (especially in AI-written code) to over-explain: every extra line costs every future reader, human or AI, more than it cost to type. ## Ticket & PR Conventions diff --git a/README.md b/README.md index 2b85dab..358bd94 100644 --- a/README.md +++ b/README.md @@ -124,6 +124,7 @@ analysis: scorer: "cosine" threshold: 0.75 # Minimum score; defaults to vector_store.similarity_threshold top_k: 3 # Maximum ADRs kept; defaults to max_relevant_adrs (3 when unset) + on_error: "skip" # "skip" (default) skips the file when the stage fails; "fail" fails the check accepted_statuses: ["Accepted", "Active"] # Use ["*"] to include all statuses exclude_patterns: - "**/*_test.go" @@ -159,6 +160,7 @@ For each changed file, ArchGuard picks which of the ADRs whose `scope` matches i - `scorer`: how candidates are scored. `cosine` (embedding similarity) is the only scorer today, and it is the default when omitted. - `threshold`: the minimum score, from 0 to 1. Candidates scoring below it are dropped. - `top_k`: the maximum number of candidates kept, a positive integer. +- `on_error`: what happens when the stage fails, `skip` or `fail` (see [When a stage fails](#when-a-stage-fails)). Unset behaves as `skip`. | | `rank` | `rerank` | |---|---|---| @@ -182,7 +184,27 @@ A `rerank` configured without a `rank` runs after the default `rank`. An ADR's o Every cosine stage re-scores the same candidates, so a cosine `rerank` after a cosine `rank` can only tighten `threshold` or `top_k` and costs one extra embedding call per file. It becomes useful once other scorers can fill a stage. -An unknown scorer, an unrecognized stage or stage key, a non-numeric or out-of-range `threshold`, or a non-positive `top_k` stops `archguard` at startup with exit code 3 and a message naming the stage and key. +An unknown scorer, an unrecognized stage or stage key, a non-numeric or out-of-range `threshold`, a non-positive `top_k`, or an `on_error` other than `skip` or `fail` stops `archguard` at startup with exit code 3 and a message naming the stage and key. + +#### When a stage fails + +By default, if a stage fails for a file (for example the embedding call errors), ArchGuard skips that file, prints the error, counts it in the skipped-files summary, and the run still exits `0`. Set `on_error: fail` on a stage to fail the check instead: + +```yaml +analysis: + pipeline: + rank: + on_error: fail +``` + +Under `fail`, a failed stage stops that file's remaining stages, other files still run, and every failure is printed with its stage, file and error. The run then exits non-zero: + +| Failure kind | Meaning | Exit code | +|---|---|---| +| `unavailable` | A dependency did not respond, such as the embedding provider | `6` | +| `precondition_not_met` | The stage could not run at all, such as no embedding provider being configured | `7` | + +A run with both kinds exits `7`. `on_error: skip` behaves exactly like leaving it unset. With `--update-baseline`, a `fail` failure exits `6` or `7` without writing the baseline. Under `--format json` the failures are listed in a `failures` array (see [Machine-Readable Output](#machine-readable-output)). ### ADR Format @@ -277,6 +299,10 @@ This will automatically create the `archguard_adrs` table and safely scope all A - **3**: Config error (failed to load or validate `archguard.yaml`). - **4**: Architectural drift detected. - **5**: Index error (failed to build, load, or fetch ADRs for the vector store). +- **6**: A ranking stage with `on_error: fail` could not reach a dependency it needs, such as the embedding provider (see [Ranking Stages](#ranking-stages)). +- **7**: A ranking stage with `on_error: fail` could not run because a precondition was not met, such as no embedding provider being configured. If a run has both kinds of failure, it exits `7`. + +Codes `6` and `7` take precedence over `4`: a run that also found drift still exits `6` or `7`, because the check was incomplete. They are only returned when a stage sets `on_error: fail`; by default a failed ranking stage skips its file and the run exits `0` (see [Ranking Stages](#ranking-stages)). ### Machine-Readable Output @@ -299,6 +325,8 @@ This will automatically create the `archguard_adrs` table and safely scope all A } ``` +When a stage with `on_error: fail` fails, the document also carries a `failures` array, each entry with the `stage`, the `file`, the `kind` (`unavailable` or `precondition_not_met`) and the underlying `error`; the array is omitted when nothing failed. The error text itself goes to stderr. + `count` matches the number of new (non-baselined) violations that drives the `4` (drift detected) exit code above. `suggestion` is present only when `--suggest-fixes` was passed; it's an LLM-generated pointer, not a verified or guaranteed fix, and it is omitted from the JSON entirely (not an empty string) when `--suggest-fixes` is off or the LLM produced nothing. > **Note:** run `archguard index` before a `--format json` check. If the index needs an automatic rebuild during `check` (e.g. a stale/missing index, or an ADR provider warning), that rebuild's own progress text currently still prints to stdout ahead of the JSON document (tracked in [#163](https://github.com/Tgenz1213/ArchGuard/issues/163)). With an up-to-date index this doesn't happen. diff --git a/docs/arch/0014-json-check-output.md b/docs/arch/0014-json-check-output.md index d43abb8..0728f20 100644 --- a/docs/arch/0014-json-check-output.md +++ b/docs/arch/0014-json-check-output.md @@ -16,6 +16,7 @@ scope: "internal/**" - stdout carries exactly one JSON document: `{"violations": [...], "count": N}`, where each violation has `file`, `adr_id`, `adr_title`, `line`, `reasoning`, `quoted_code`, and (per `docs/arch/0016-llm-suggested-remediation.md`) an optional `suggestion`, and `count` matches `DriftDetectedError.Count` (the same number that drives the `ExitDriftDetected` exit code). - Everything else that would normally print to stdout (the startup banner, `--debug` logging, per-file progress, index-rebuild notices, the final "No new architectural violations found." summary) is redirected to stderr instead of being suppressed outright, so `--format json --debug` still gives visibility into what happened without breaking a pipe consuming stdout. +- When a stage with `on_error: fail` fails (see `docs/arch/0022-candidate-scoring-pipeline.md`), the document also carries a `failures` array of `{stage, file, kind, error}`; it is omitted when empty. - Exit codes are unaffected: `--format` only changes what's printed, never what's returned. - `--update-baseline` ignores `--format` entirely (a printed note explains this) -- its output is a maintenance summary about the baseline file, not the violation report `--format json` targets, and baselining suppresses the very violations this flag would otherwise report. diff --git a/docs/arch/0022-candidate-scoring-pipeline.md b/docs/arch/0022-candidate-scoring-pipeline.md index 241a8d0..5193b19 100644 --- a/docs/arch/0022-candidate-scoring-pipeline.md +++ b/docs/arch/0022-candidate-scoring-pipeline.md @@ -17,9 +17,9 @@ Candidates flow through an ordered list of `stage.Stage`s (`internal/analysis/st - `stage.Scorer` receives a `File`, a `Debug`, and all of a file's `Candidate`s in one call, and returns one score per candidate, in order, on the scale its stage's `Threshold` uses (0-1 for screening models; raw cosine similarity, -1 to 1, for `CosineRanker`). It only scores; it never drops or reorders. - `stage.Stage` wraps a `Scorer` with a `Threshold` (the minimum score, optionally per ADR) and `MaxKeep`. The stage drops candidates below the threshold, orders survivors by descending score, and cuts to `MaxKeep`. The next stage receives only the survivors. - `stage.File` exposes the path and the text to embed (`QueryText`, built lazily, diff-preferred and capped). `stage.Debug` is a real writer under `--debug` and a no-op otherwise, so scorers never nil-check. -- `stage.Error` carries the action that failed (`generating embedding`, `scoring candidates`); the engine reports `Error for ` without knowing which scorer ran. +- `stage.Error` carries the action that failed (`generating embedding`, `scoring candidates`) and a `Kind`: `KindUnavailable` (a dependency did not respond, the zero value) or `KindPreconditionNotMet` (the stage cannot run at all, such as cosine with no embedding provider). The engine reports `Error for ` without knowing which scorer ran. - `Engine.Stages` holds the pipeline. When unset it is one `stage.NewCosineStage`: `CosineRanker` scoring, an `ADRThreshold` (an ADR's own `similarity_threshold` over `vector_store.similarity_threshold`), and `analysis.max_relevant_adrs` as `MaxKeep`. -- `analysis.pipeline` configures the stages. `config.Pipeline` holds optional `rank` and `rerank` `StageConfig`s (`scorer`, `threshold`, `top_k`), validated when the config loads. `analysis.BuildStages` turns it into `Engine.Stages`, and returns nil when there is no block so the default above stays in force. An absent `rank` is the default cosine stage; an unset `rank` `threshold` or `top_k` falls back to `vector_store.similarity_threshold` and `analysis.max_relevant_adrs`; `rerank` uses 0 and 3 with a printed warning. A stage `threshold` is the default for ADRs without their own `similarity_threshold`, which wins in every cosine stage. +- `analysis.pipeline` configures the stages. `config.Pipeline` holds optional `rank` and `rerank` `StageConfig`s (`scorer`, `threshold`, `top_k`, `on_error`), validated when the config loads. `analysis.BuildStages` turns it into `Engine.Stages`, and returns nil when there is no block so the default above stays in force. An absent `rank` is the default cosine stage; an unset `rank` `threshold` or `top_k` falls back to `vector_store.similarity_threshold` and `analysis.max_relevant_adrs`; `rerank` uses 0 and 3 with a printed warning. A stage `threshold` is the default for ADRs without their own `similarity_threshold`, which wins in every cosine stage. - The LLM is the only source of reported violations. Stages choose which ADRs are judged, never whether a file violates one. The pipeline's input is built in a fixed order by `candidateSource`: @@ -35,7 +35,8 @@ Embedding is done by `CosineRanker` alone, through `llm.Embedder`, so a pipeline - Adding a scorer means implementing `stage.Scorer`; stage ordering, thresholds, caps, and reporting are unchanged. - Scorer names are validated in `internal/config`, which cannot import `stage`, so a new scorer is also added to that list. - Every cosine stage scores the same candidates, so a cosine `rerank` after a cosine `rank` only tightens `threshold` or `top_k` and embeds the file a second time. -- An embedding failure is reported as `Error generating embedding for `; any other scorer failure as `Error scoring candidates for `. Both count the file as skipped. +- An embedding failure is reported as `Error generating embedding for `; any other scorer failure as `Error scoring candidates for `. Both count the file as skipped, unless the stage sets `on_error: fail`. +- `on_error` is per stage, `skip` (the default) or `fail`. Under `fail` the engine records a `StageFailure` (stage name, file, kind, error) in `Engine.StageFailures` instead of counting a skipped file, stops that file's remaining stages, and lets other files run. `cli.runCheck` exits `6` when every failure is `KindUnavailable` and `7` when any is `KindPreconditionNotMet`; both take precedence over the drift exit code `4`, because an incomplete check is not a clean verdict, and `--update-baseline` exits with them without writing the baseline. `check --format json` lists the failures under `failures`, omitted when there are none, so a run without `fail` failures produces the same document as before. - Every `VectorStore` implementation provides `ScopedADRs`, and it returns an error rather than an empty list when the backend fails; the engine reports `Error loading candidate ADRs for ` and counts the file as skipped, so an unreachable database can't make a check pass. - Equal scores keep candidate order (the store's order), so a tie at the top-K boundary is resolved deterministically. - `--debug` prints `Skipping ADR ... (Suppressed)` for every suppressed scope-matched ADR first, then the stage's `Below threshold` (capped at `MaxKeep`) and `Cut by top-K limit` lines, whichever scorer produced the scores. diff --git a/internal/analysis/engine.go b/internal/analysis/engine.go index f1224ec..8aff91c 100644 --- a/internal/analysis/engine.go +++ b/internal/analysis/engine.go @@ -7,6 +7,7 @@ import ( "io" "os" "regexp" + "sort" "strings" "sync" "unicode/utf8" @@ -39,6 +40,7 @@ type Engine struct { JSONOutput bool Writer io.Writer CollectedViolations []Violation + StageFailures []StageFailure // Off by default: adds one LLM call per reported violation. SuggestFixes bool Stages []stage.Stage @@ -54,6 +56,13 @@ type Violation struct { Suggestion string `json:"suggestion,omitempty"` } +type StageFailure struct { + Stage string `json:"stage"` + File string `json:"file"` + Kind stage.Kind `json:"kind"` + Error string `json:"error"` +} + var ErrDriftDetected = errors.New("architectural drift detected") type DriftDetectedError struct { @@ -119,6 +128,7 @@ func (e *Engine) Run(ctx context.Context) error { skippedADRChecks int collectedEntries []baseline.Entry collectedViolations []Violation + stageFailures []StageFailure mu sync.Mutex ) @@ -198,10 +208,16 @@ func (e *Engine) Run(ctx context.Context) error { for _, st := range stages { hits, err = st.Apply(ctx, query, debug, hits) if err != nil { - sb.WriteString(scoringErrorMessage(file, err)) mu.Lock() + if st.FailOnError { + failure := newStageFailure(st.Name, file, err) + sb.WriteString(failureMessage(failure)) + stageFailures = append(stageFailures, failure) + } else { + sb.WriteString(scoringErrorMessage(file, err)) + skippedFiles++ + } _, _ = fmt.Fprint(e.writer(), sb.String()) - skippedFiles++ mu.Unlock() return nil } @@ -391,6 +407,13 @@ func (e *Engine) Run(ctx context.Context) error { e.SkippedFiles = skippedFiles e.SkippedADRChecks = skippedADRChecks + sort.Slice(stageFailures, func(i, j int) bool { + if stageFailures[i].File != stageFailures[j].File { + return stageFailures[i].File < stageFailures[j].File + } + return stageFailures[i].Stage < stageFailures[j].Stage + }) + e.StageFailures = stageFailures if e.JSONOutput { if collectedViolations == nil { collectedViolations = []Violation{} @@ -612,6 +635,19 @@ func writeViolationOutput(sb *strings.Builder, v violationOutput, verified bool) } } +func newStageFailure(name, file string, err error) StageFailure { + failure := StageFailure{Stage: name, File: file, Error: err.Error()} + var stageErr *stage.Error + if errors.As(err, &stageErr) { + failure.Kind = stageErr.Kind + } + return failure +} + +func failureMessage(f StageFailure) string { + return fmt.Sprintf("Error: stage %s failed for %s (%s): %s\n", f.Stage, f.File, f.Kind, f.Error) +} + func scoringErrorMessage(file string, err error) string { var stageErr *stage.Error if errors.As(err, &stageErr) { diff --git a/internal/analysis/pipeline_test.go b/internal/analysis/pipeline_test.go index 3e6a282..571e7c6 100644 --- a/internal/analysis/pipeline_test.go +++ b/internal/analysis/pipeline_test.go @@ -1,8 +1,11 @@ package analysis_test import ( + "bytes" "context" + "encoding/json" "errors" + "io" "strings" "sync" "testing" @@ -191,6 +194,160 @@ func TestPipeline_SuppressedADRsNeverReachScorerOrLLM(t *testing.T) { } } +func runRankWithOnError(t *testing.T, onError string, embed llm.Embedder) (*scorerHarness, string) { + t.Helper() + h := newScorerHarness(t, []index.ADR{scorerADR("0001", 1)}, "good.go", "package good") + h.engine.Content.(*MockContentProvider).Files["bad.go"] = "package BAD" + var out bytes.Buffer + h.engine.Writer = &out + cfg := &config.Config{ + VectorStore: config.VectorStore{SimilarityThreshold: 0}, + Analysis: config.Analysis{Pipeline: &config.Pipeline{Rank: &config.StageConfig{Scorer: config.ScorerCosine, OnError: onError}}}, + } + h.engine.Stages = analysis.BuildStages(cfg, h.engine.Store, embed, io.Discard) + if err := h.engine.Run(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + return h, out.String() +} + +func embedFailingOnBAD() llm.Embedder { + return &llm.MockProvider{ + EmbedFunc: func(ctx context.Context, text string, task llm.EmbeddingTaskType) ([]float32, error) { + if strings.Contains(text, "BAD") { + return nil, errors.New("embedding service down") + } + return []float32{1, 0, 0, 0}, nil + }, + } +} + +func TestPipeline_OnErrorSkipAndDefaultSkipTheFile(t *testing.T) { + for _, onError := range []string{"", config.OnErrorSkip} { + t.Run("on_error="+onError, func(t *testing.T) { + h, out := runRankWithOnError(t, onError, embedFailingOnBAD()) + if h.engine.SkippedFiles != 1 || len(h.engine.StageFailures) != 0 { + t.Fatalf("SkippedFiles = %d, StageFailures = %v; want the file skipped and no failures", h.engine.SkippedFiles, h.engine.StageFailures) + } + if !strings.Contains(out, "Error generating embedding for bad.go: embedding service down") { + t.Errorf("output %q missing the skipped-file error", out) + } + if strings.Join(h.judged, ",") != "0001" { + t.Errorf("judged %v, want the healthy file still judged", h.judged) + } + }) + } +} + +func TestPipeline_OnErrorFailUnavailable(t *testing.T) { + h, out := runRankWithOnError(t, config.OnErrorFail, embedFailingOnBAD()) + if h.engine.SkippedFiles != 0 || len(h.engine.StageFailures) != 1 { + t.Fatalf("SkippedFiles = %d, StageFailures = %v; want one failure and no skips", h.engine.SkippedFiles, h.engine.StageFailures) + } + f := h.engine.StageFailures[0] + if f.Stage != "rank" || f.File != "bad.go" || f.Kind != stage.KindUnavailable || !strings.Contains(f.Error, "embedding service down") { + t.Errorf("failure = %+v", f) + } + for _, want := range []string{"stage rank", "bad.go", "unavailable", "embedding service down"} { + if !strings.Contains(out, want) { + t.Errorf("output %q missing %q", out, want) + } + } + if strings.Join(h.judged, ",") != "0001" { + t.Errorf("judged %v, want the healthy file still judged", h.judged) + } +} + +func TestPipeline_OnErrorFailPreconditionNotMet(t *testing.T) { + h, _ := runRankWithOnError(t, config.OnErrorFail, nil) + if len(h.engine.StageFailures) != 2 || len(h.judged) != 0 { + t.Fatalf("StageFailures = %v, judged = %v; want both files failed and nothing judged", h.engine.StageFailures, h.judged) + } + for _, f := range h.engine.StageFailures { + if f.Stage != "rank" || f.Kind != stage.KindPreconditionNotMet { + t.Errorf("failure = %+v, want a rank precondition failure", f) + } + } +} + +func TestPipeline_OnErrorSkipPreconditionSkipsFiles(t *testing.T) { + h, _ := runRankWithOnError(t, config.OnErrorSkip, nil) + if h.engine.SkippedFiles != 2 || len(h.engine.StageFailures) != 0 { + t.Fatalf("SkippedFiles = %d, StageFailures = %v; want both files skipped", h.engine.SkippedFiles, h.engine.StageFailures) + } +} + +func failingScorer(kind stage.Kind) stage.Scorer { + return scorerFunc(func(ctx context.Context, file stage.File, debug stage.Debug, candidates []stage.Candidate) ([]float64, error) { + return nil, &stage.Error{Action: "scoring candidates", Kind: kind, Err: errors.New("boom")} + }) +} + +func TestPipeline_OnErrorFailStopsRemainingStagesForThatFileOnly(t *testing.T) { + h := newScorerHarness(t, []index.ADR{scorerADR("0001", 1)}, "good.go", "package good") + h.engine.Content.(*MockContentProvider).Files["bad.go"] = "package BAD" + + var mu sync.Mutex + var secondStageFiles []string + first := scorerFunc(func(ctx context.Context, file stage.File, debug stage.Debug, candidates []stage.Candidate) ([]float64, error) { + if file.Path() == "bad.go" { + return nil, errors.New("boom") + } + return make([]float64, len(candidates)), nil + }) + second := scorerFunc(func(ctx context.Context, file stage.File, debug stage.Debug, candidates []stage.Candidate) ([]float64, error) { + mu.Lock() + secondStageFiles = append(secondStageFiles, file.Path()) + mu.Unlock() + return make([]float64, len(candidates)), nil + }) + h.engine.Stages = []stage.Stage{ + {Name: "rank", Scorer: first, FailOnError: true}, + {Name: "rerank", Scorer: second}, + } + + if err := h.engine.Run(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(h.engine.StageFailures) != 1 || h.engine.StageFailures[0].File != "bad.go" { + t.Fatalf("StageFailures = %v, want one failure for bad.go", h.engine.StageFailures) + } + if strings.Join(secondStageFiles, ",") != "good.go" { + t.Errorf("second stage ran for %v, want only good.go", secondStageFiles) + } +} + +func TestPipeline_StageFailuresAreSortedByFile(t *testing.T) { + h := newScorerHarness(t, []index.ADR{scorerADR("0001", 1)}, "m.go", "package m") + files := h.engine.Content.(*MockContentProvider).Files + files["z.go"] = "package z" + files["a.go"] = "package a" + h.engine.Config.Analysis.MaxConcurrency = 3 + h.engine.Stages = []stage.Stage{{Name: "rank", Scorer: failingScorer(stage.KindUnavailable), FailOnError: true}} + + if err := h.engine.Run(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + var got []string + for _, f := range h.engine.StageFailures { + got = append(got, f.File) + } + if strings.Join(got, ",") != "a.go,m.go,z.go" { + t.Errorf("failure order = %v, want sorted by file", got) + } +} + +func TestPipeline_StageFailureJSONCarriesKind(t *testing.T) { + b, err := json.Marshal(analysis.StageFailure{Stage: "rerank", File: "a.go", Kind: stage.KindPreconditionNotMet, Error: "boom"}) + if err != nil { + t.Fatal(err) + } + want := `{"stage":"rerank","file":"a.go","kind":"precondition_not_met","error":"boom"}` + if string(b) != want { + t.Errorf("json = %s, want %s", b, want) + } +} + func TestPipeline_ScorerErrorSkipsFile(t *testing.T) { h := newScorerHarness(t, []index.ADR{scorerADR("0001", 1)}, "svc.go", "package svc") h.engine.Stages = []stage.Stage{{Scorer: scorerFunc(func(ctx context.Context, file stage.File, debug stage.Debug, candidates []stage.Candidate) ([]float64, error) { diff --git a/internal/analysis/stage/cosine.go b/internal/analysis/stage/cosine.go index 85875a1..18d4604 100644 --- a/internal/analysis/stage/cosine.go +++ b/internal/analysis/stage/cosine.go @@ -2,6 +2,7 @@ package stage import ( "context" + "errors" "math" "github.com/tgenz1213/archguard/internal/index" @@ -18,6 +19,9 @@ type CosineRanker struct { } func (c *CosineRanker) Score(ctx context.Context, file File, debug Debug, candidates []Candidate) ([]float64, error) { + if c.Embed == nil { + return nil, &Error{Action: "generating embedding", Kind: KindPreconditionNotMet, Err: errors.New("no embedding provider configured")} + } embedding, err := c.Embed.CreateEmbedding(ctx, file.QueryText(), llm.EmbeddingTaskQuery) if err != nil { return nil, &Error{Action: "generating embedding", Err: err} @@ -54,6 +58,7 @@ func adrKey(adr *index.ADR) string { func NewCosineStage(store index.VectorStore, embed llm.Embedder, threshold float64, topK int) Stage { return Stage{ + Name: "rank", Scorer: &CosineRanker{Store: store, Embed: embed, Threshold: threshold}, Min: ADRThreshold{Global: threshold}, MaxKeep: topK, diff --git a/internal/analysis/stage/cosine_test.go b/internal/analysis/stage/cosine_test.go index b1c837a..d4256d0 100644 --- a/internal/analysis/stage/cosine_test.go +++ b/internal/analysis/stage/cosine_test.go @@ -107,6 +107,30 @@ func TestCosineStage_EmbeddingFailureIsReportedAsGeneratingEmbedding(t *testing. if !errors.As(err, &stageErr) || stageErr.Action != "generating embedding" || !errors.Is(err, boom) { t.Fatalf("err = %v, want a *stage.Error with action generating embedding wrapping the cause", err) } + if stageErr.Kind != stage.KindUnavailable { + t.Errorf("Kind = %v, want %v", stageErr.Kind, stage.KindUnavailable) + } +} + +func TestCosineStage_MissingEmbedderIsPreconditionNotMet(t *testing.T) { + store := cosineStore(cosineADR("a", 1, 0)) + s := stage.NewCosineStage(store, nil, 0, 5) + + _, err := s.Apply(context.Background(), fakeFile{path: "svc.go"}, stage.NoDebug, candidatesFor(store)) + + var stageErr *stage.Error + if !errors.As(err, &stageErr) || stageErr.Kind != stage.KindPreconditionNotMet { + t.Fatalf("err = %v, want a *stage.Error of kind precondition_not_met", err) + } +} + +func TestKind_String(t *testing.T) { + if got := stage.KindUnavailable.String(); got != "unavailable" { + t.Errorf("KindUnavailable = %q", got) + } + if got := stage.KindPreconditionNotMet.String(); got != "precondition_not_met" { + t.Errorf("KindPreconditionNotMet = %q", got) + } } func TestCosineStage_OmittedADRsScoreBelowAZeroThreshold(t *testing.T) { diff --git a/internal/analysis/stage/stage.go b/internal/analysis/stage/stage.go index 7abdb29..181244b 100644 --- a/internal/analysis/stage/stage.go +++ b/internal/analysis/stage/stage.go @@ -7,9 +7,26 @@ import ( "sort" ) +type Kind int + +const ( + KindUnavailable Kind = iota + KindPreconditionNotMet +) + +func (k Kind) String() string { + if k == KindPreconditionNotMet { + return "precondition_not_met" + } + return "unavailable" +} + +func (k Kind) MarshalText() ([]byte, error) { return []byte(k.String()), nil } + // Error names the action that failed so the engine can report it without knowing which scorer ran. type Error struct { Action string + Kind Kind Err error } @@ -17,9 +34,11 @@ func (e *Error) Error() string { return e.Err.Error() } func (e *Error) Unwrap() error { return e.Err } type Stage struct { - Scorer Scorer - Min Threshold - MaxKeep int + Name string + Scorer Scorer + Min Threshold + MaxKeep int + FailOnError bool } func (s Stage) Apply(ctx context.Context, file File, debug Debug, candidates []Candidate) ([]Candidate, error) { diff --git a/internal/analysis/stages.go b/internal/analysis/stages.go index 3f341eb..ea44e5d 100644 --- a/internal/analysis/stages.go +++ b/internal/analysis/stages.go @@ -40,7 +40,11 @@ func rankStage(cfg *config.Config, sc *config.StageConfig, store index.VectorSto topK = *sc.TopK } } - return stage.NewCosineStage(store, embed, threshold, topK) + st := stage.NewCosineStage(store, embed, threshold, topK) + if sc != nil { + st.FailOnError = sc.OnError == config.OnErrorFail + } + return st } func rerankStage(sc *config.StageConfig, store index.VectorStore, embed llm.Embedder, warnings io.Writer) stage.Stage { @@ -57,5 +61,8 @@ func rerankStage(sc *config.StageConfig, store index.VectorStore, embed llm.Embe } else { _, _ = fmt.Fprintf(warnings, "Warning: analysis.pipeline.rerank.top_k not set, defaulting to %d\n", rerankDefaultTopK) } - return stage.NewCosineStage(store, embed, threshold, topK) + st := stage.NewCosineStage(store, embed, threshold, topK) + st.Name = "rerank" + st.FailOnError = sc.OnError == config.OnErrorFail + return st } diff --git a/internal/analysis/stages_test.go b/internal/analysis/stages_test.go index 9ab0427..70c8111 100644 --- a/internal/analysis/stages_test.go +++ b/internal/analysis/stages_test.go @@ -132,6 +132,36 @@ func TestBuildStages_RankThenRerankInOrder(t *testing.T) { } } +func TestBuildStages_OnErrorSetsNameAndPolicy(t *testing.T) { + skip := &config.StageConfig{OnError: config.OnErrorSkip} + fail := &config.StageConfig{OnError: config.OnErrorFail} + tests := []struct { + name string + rank *config.StageConfig + rerank *config.StageConfig + wantFail []bool + }{ + {"unset", &config.StageConfig{}, &config.StageConfig{}, []bool{false, false}}, + {"skip", skip, skip, []bool{false, false}}, + {"fail", fail, fail, []bool{true, true}}, + {"rerank alone", nil, fail, []bool{false, true}}, + } + wantNames := []string{"rank", "rerank"} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stages, _ := buildStages(t, configWith(&config.Pipeline{Rank: tt.rank, Rerank: tt.rerank})) + if len(stages) != 2 { + t.Fatalf("got %d stages, want 2", len(stages)) + } + for i, st := range stages { + if st.Name != wantNames[i] || st.FailOnError != tt.wantFail[i] { + t.Errorf("stage %d = {Name %q, FailOnError %v}, want {%q, %v}", i, st.Name, st.FailOnError, wantNames[i], tt.wantFail[i]) + } + } + }) + } +} + func TestBuildStages_ADROverrideBeatsStageThreshold(t *testing.T) { stages, _ := buildStages(t, configWith(&config.Pipeline{Rank: &config.StageConfig{Scorer: config.ScorerCosine, Threshold: ptr(0.8)}})) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index c1c7a9d..9fdcf26 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -18,6 +18,7 @@ import ( "github.com/joho/godotenv" "github.com/tgenz1213/archguard/internal/analysis" + "github.com/tgenz1213/archguard/internal/analysis/stage" "github.com/tgenz1213/archguard/internal/baseline" "github.com/tgenz1213/archguard/internal/config" "github.com/tgenz1213/archguard/internal/git" @@ -28,12 +29,14 @@ import ( type ExitCode int const ( - ExitSuccess ExitCode = 0 - ExitError ExitCode = 1 - ExitUsage ExitCode = 2 - ExitConfig ExitCode = 3 - ExitDriftDetected ExitCode = 4 - ExitIndexError ExitCode = 5 + ExitSuccess ExitCode = 0 + ExitError ExitCode = 1 + ExitUsage ExitCode = 2 + ExitConfig ExitCode = 3 + ExitDriftDetected ExitCode = 4 + ExitIndexError ExitCode = 5 + ExitStageUnavailable ExitCode = 6 + ExitStagePrecondition ExitCode = 7 ) const defaultADRPath = "./docs/arch" @@ -650,10 +653,15 @@ func runCheck(cfg *config.Config, chatProvider, embedProvider llm.Provider, inde engine.SuggestFixes = *suggestFixes runErr := engine.Run(context.Background()) + stageFailureCode, stageFailureErr := stageFailureExit(engine.StageFailures) + if *updateBaseline { if runErr != nil { return exitCodeForAnalysisError(runErr), fmt.Errorf("analysis failed: %v", runErr) } + if stageFailureErr != nil { + return stageFailureCode, fmt.Errorf("%v; baseline not written", stageFailureErr) + } if err := engine.CollectedBaseline.Save(baseline.Path); err != nil { return ExitError, fmt.Errorf("failed to write baseline file %s: %v", baseline.Path, err) } @@ -663,11 +671,15 @@ func runCheck(cfg *config.Config, chatProvider, embedProvider llm.Provider, inde } if jsonOutput { - if err := writeCheckReport(os.Stdout, engine.CollectedViolations); err != nil { + if err := writeCheckReport(os.Stdout, engine.CollectedViolations, engine.StageFailures); err != nil { return ExitError, fmt.Errorf("failed to write json report: %v", err) } } + if stageFailureErr != nil { + return stageFailureCode, stageFailureErr + } + if runErr != nil { return exitCodeForAnalysisError(runErr), fmt.Errorf("analysis failed: %v", runErr) } @@ -710,17 +722,32 @@ func newIndexFlagSet() *flag.FlagSet { } type checkReport struct { - Violations []analysis.Violation `json:"violations"` - Count int `json:"count"` + Violations []analysis.Violation `json:"violations"` + Count int `json:"count"` + Failures []analysis.StageFailure `json:"failures,omitempty"` } -func writeCheckReport(w io.Writer, violations []analysis.Violation) error { +func writeCheckReport(w io.Writer, violations []analysis.Violation, failures []analysis.StageFailure) error { if violations == nil { violations = []analysis.Violation{} } enc := json.NewEncoder(w) enc.SetIndent("", " ") - return enc.Encode(checkReport{Violations: violations, Count: len(violations)}) + return enc.Encode(checkReport{Violations: violations, Count: len(violations), Failures: failures}) +} + +// A precondition failure outranks an unavailable dependency when a run has both. +func stageFailureExit(failures []analysis.StageFailure) (ExitCode, error) { + if len(failures) == 0 { + return ExitSuccess, nil + } + code := ExitStageUnavailable + for _, f := range failures { + if f.Kind == stage.KindPreconditionNotMet { + code = ExitStagePrecondition + } + } + return code, fmt.Errorf("%d stage failure(s) with on_error: fail; compliance was not verified", len(failures)) } func resolveContentProvider(human io.Writer, files []string, staged, all, updateBaseline bool) analysis.ContentProvider { diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 7777be7..a5e2d64 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -3,6 +3,7 @@ package cli import ( "bytes" "context" + "encoding/json" "errors" "fmt" "io" @@ -15,6 +16,7 @@ import ( "testing" "github.com/tgenz1213/archguard/internal/analysis" + "github.com/tgenz1213/archguard/internal/analysis/stage" "github.com/tgenz1213/archguard/internal/baseline" "github.com/tgenz1213/archguard/internal/config" "github.com/tgenz1213/archguard/internal/llm" @@ -43,6 +45,76 @@ func TestExitCodeForAnalysisError(t *testing.T) { }) } +func TestStageFailureExit(t *testing.T) { + unavailable := analysis.StageFailure{Stage: "rank", File: "a.go", Kind: stage.KindUnavailable} + precondition := analysis.StageFailure{Stage: "rank", File: "b.go", Kind: stage.KindPreconditionNotMet} + tests := []struct { + name string + failures []analysis.StageFailure + want ExitCode + }{ + {"none", nil, ExitSuccess}, + {"unavailable only", []analysis.StageFailure{unavailable}, ExitStageUnavailable}, + {"precondition only", []analysis.StageFailure{precondition}, ExitStagePrecondition}, + {"both kinds", []analysis.StageFailure{unavailable, precondition, unavailable}, ExitStagePrecondition}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + code, err := stageFailureExit(tt.failures) + if code != tt.want || (err != nil) != (tt.want != ExitSuccess) { + t.Fatalf("stageFailureExit = (%d, %v), want code %d", code, err, tt.want) + } + }) + } +} + +func TestStageExitCodeValues(t *testing.T) { + if ExitStageUnavailable != 6 || ExitStagePrecondition != 7 { + t.Fatalf("stage exit codes = %d and %d, want 6 and 7", ExitStageUnavailable, ExitStagePrecondition) + } +} + +func TestStageExitCodesAreDistinctFromExistingCodes(t *testing.T) { + seen := map[ExitCode]string{} + for name, code := range map[string]ExitCode{ + "success": ExitSuccess, "error": ExitError, "usage": ExitUsage, "config": ExitConfig, + "drift": ExitDriftDetected, "index": ExitIndexError, + "unavailable": ExitStageUnavailable, "precondition": ExitStagePrecondition, + } { + if other, dup := seen[code]; dup { + t.Errorf("%s and %s share exit code %d", name, other, code) + } + seen[code] = name + } +} + +func TestWriteCheckReport_FailuresOmittedWhenNone(t *testing.T) { + var buf bytes.Buffer + if err := writeCheckReport(&buf, nil, nil); err != nil { + t.Fatal(err) + } + if strings.Contains(buf.String(), "failures") { + t.Errorf("report %q should not mention failures when there are none", buf.String()) + } +} + +func TestWriteCheckReport_IncludesFailureKind(t *testing.T) { + var buf bytes.Buffer + failures := []analysis.StageFailure{{Stage: "rank", File: "a.go", Kind: stage.KindUnavailable, Error: "down"}} + if err := writeCheckReport(&buf, nil, failures); err != nil { + t.Fatal(err) + } + var got struct { + Failures []map[string]string `json:"failures"` + } + if err := json.Unmarshal(buf.Bytes(), &got); err != nil { + t.Fatalf("report is not valid JSON: %v", err) + } + if len(got.Failures) != 1 || got.Failures[0]["kind"] != "unavailable" || got.Failures[0]["stage"] != "rank" || got.Failures[0]["file"] != "a.go" || got.Failures[0]["error"] != "down" { + t.Errorf("failures = %v", got.Failures) + } +} + func TestValidateProviderConfig_ClaudeRequiresEmbeddingProvider(t *testing.T) { cfg := &config.Config{ LLM: config.LLMConfig{Provider: "claude"}, diff --git a/internal/config/pipeline_test.go b/internal/config/pipeline_test.go index 9c0922b..aac2e26 100644 --- a/internal/config/pipeline_test.go +++ b/internal/config/pipeline_test.go @@ -156,6 +156,29 @@ func TestLoadConfig_PipelineMergeKeys(t *testing.T) { } } +func TestLoadConfig_PipelineOnError(t *testing.T) { + tests := []struct { + name string + yaml string + want string + }{ + {"unset", "rank:\n top_k: 2", ""}, + {"skip", "rank:\n on_error: skip", OnErrorSkip}, + {"fail", "rank:\n on_error: fail", OnErrorFail}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg, err := loadFromYAML(t, "analysis:\n pipeline:\n "+tt.yaml+"\n") + if err != nil { + t.Fatal(err) + } + if got := cfg.Analysis.Pipeline.Rank.OnError; got != tt.want { + t.Errorf("OnError = %q, want %q", got, tt.want) + } + }) + } +} + func TestLoadConfig_PipelineInvalid(t *testing.T) { tests := []struct { name string @@ -164,7 +187,10 @@ func TestLoadConfig_PipelineInvalid(t *testing.T) { }{ {"unknown scorer", "rank:\n scorer: jev", []string{"analysis.pipeline.rank.scorer", `"jev"`, "cosine"}}, {"unrecognized stage", "pre_judge:\n scorer: cosine", []string{"analysis.pipeline", `"pre_judge"`}}, - {"unrecognized stage key", "rerank:\n on_error: skip", []string{"analysis.pipeline.rerank", `"on_error"`}}, + {"unrecognized stage key", "rerank:\n retries: 2", []string{"analysis.pipeline.rerank", `"retries"`}}, + {"unknown on_error", "rank:\n on_error: warn", []string{"analysis.pipeline.rank.on_error", `"warn"`, "skip, fail"}}, + {"non-string on_error", "rerank:\n on_error: [skip]", []string{"analysis.pipeline.rerank.on_error", "skip, fail"}}, + {"null on_error", "rank:\n on_error:", []string{"analysis.pipeline.rank.on_error", "skip, fail"}}, {"non-numeric threshold", "rank:\n threshold: high", []string{"analysis.pipeline.rank.threshold", "number"}}, {"threshold above range", "rerank:\n threshold: 1.5", []string{"analysis.pipeline.rerank.threshold", "1.5", "between 0 and 1"}}, {"threshold below range", "rank:\n threshold: -0.1", []string{"analysis.pipeline.rank.threshold", "-0.1"}}, diff --git a/internal/config/stage.go b/internal/config/stage.go index 50bee21..de39b2c 100644 --- a/internal/config/stage.go +++ b/internal/config/stage.go @@ -12,12 +12,21 @@ import ( const ScorerCosine = "cosine" -var scorerNames = []string{ScorerCosine} +const ( + OnErrorSkip = "skip" + OnErrorFail = "fail" +) + +var ( + scorerNames = []string{ScorerCosine} + onErrorModes = []string{OnErrorSkip, OnErrorFail} +) type StageConfig struct { Scorer string Threshold *float64 TopK *int + OnError string } func decodeStage(name string, node *yaml.Node) (*StageConfig, error) { @@ -27,7 +36,7 @@ func decodeStage(name string, node *yaml.Node) (*StageConfig, error) { return &StageConfig{Scorer: ScorerCosine}, nil } if node.Kind != yaml.MappingNode { - return nil, fmt.Errorf("%s: must be a mapping with scorer, threshold and top_k keys", prefix) + return nil, fmt.Errorf("%s: must be a mapping with scorer, threshold, top_k and on_error keys", prefix) } fields, err := decodeFields(prefix, node) @@ -46,8 +55,10 @@ func decodeStage(name string, node *yaml.Node) (*StageConfig, error) { stage.Threshold, err = decodeThreshold(prefix, value) case "top_k": stage.TopK, err = decodeTopK(prefix, value) + case "on_error": + stage.OnError, err = decodeOnError(prefix, value) default: - err = fmt.Errorf("%s: unrecognized key %q (expected scorer, threshold or top_k)", prefix, key) + err = fmt.Errorf("%s: unrecognized key %q (expected scorer, threshold, top_k or on_error)", prefix, key) } if err != nil { return nil, err @@ -88,3 +99,14 @@ func decodeTopK(prefix string, node *yaml.Node) (*int, error) { } return &topK, nil } + +func decodeOnError(prefix string, node *yaml.Node) (string, error) { + var mode string + if err := node.Decode(&mode); err != nil || node.Tag == "!!null" { + return "", fmt.Errorf("%s.on_error: must be one of %s", prefix, strings.Join(onErrorModes, ", ")) + } + if slices.Contains(onErrorModes, mode) { + return mode, nil + } + return "", fmt.Errorf("%s.on_error: unknown value %q (available: %s)", prefix, mode, strings.Join(onErrorModes, ", ")) +} diff --git a/test/e2e_pipeline_test.go b/test/e2e_pipeline_test.go index 3a53879..e2ce686 100644 --- a/test/e2e_pipeline_test.go +++ b/test/e2e_pipeline_test.go @@ -1,14 +1,18 @@ package test import ( + "bytes" "encoding/json" "fmt" "os" + "os/exec" "path/filepath" "strings" "testing" + "github.com/tgenz1213/archguard/internal/baseline" "github.com/tgenz1213/archguard/internal/cli" + "github.com/tgenz1213/archguard/internal/testutil" ) func pipelineConfigYAML(pipeline string) string { @@ -109,6 +113,117 @@ func TestE2E_PipelineConfig(t *testing.T) { } } +func TestE2E_PipelineOnError_EmbeddingFailure(t *testing.T) { + tests := []struct { + name string + pipeline string + wantExit cli.ExitCode + wantFailures int + }{ + {"no pipeline skips the file", "", cli.ExitSuccess, 0}, + {"on_error skip skips the file", " pipeline:\n rank:\n on_error: skip\n", cli.ExitSuccess, 0}, + {"on_error fail fails the check", " pipeline:\n rank:\n on_error: fail\n", cli.ExitStageUnavailable, 1}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tempDir, binaryPath := buildE2EBinary(t) + writeE2EConfig(t, tempDir, pipelineConfigYAML(tt.pipeline)) + writePipelineADRs(t, tempDir) + fixture := fmt.Sprintf("function f() {\n console.log(%q);\n}\n", testutil.MockEmbedFailureTrigger) + if err := os.WriteFile(filepath.Join(tempDir, fixtureFilename), []byte(fixture), 0644); err != nil { + t.Fatalf("Failed to create fixture: %v", err) + } + + stdout, stderr, exitCode := runCheckJSON(t, tempDir, binaryPath, fixtureFilename) + + if exitCode != int(tt.wantExit) { + t.Fatalf("expected exit code %d, got %d. stderr: %s", tt.wantExit, exitCode, stderr) + } + var report struct { + Failures []map[string]string `json:"failures"` + } + if err := json.Unmarshal([]byte(stdout), &report); err != nil { + t.Fatalf("stdout is not valid JSON: %v\nstdout: %q", err, stdout) + } + if len(report.Failures) != tt.wantFailures { + t.Fatalf("failures = %v, want %d", report.Failures, tt.wantFailures) + } + wantStderr := "generating embedding" + if tt.wantFailures == 1 { + wantStderr = "stage rank failed for " + fixtureFilename + } + if !strings.Contains(stderr, wantStderr) || !strings.Contains(stderr, "mock embed failure") { + t.Errorf("stderr should carry %q and the embedding error, got: %s", wantStderr, stderr) + } + if tt.wantFailures == 1 { + f := report.Failures[0] + if f["stage"] != "rank" || f["kind"] != "unavailable" || f["file"] != fixtureFilename { + t.Errorf("failure = %v", f) + } + } + }) + } +} + +func TestE2E_PipelineOnErrorFail_TakesPrecedenceOverDrift(t *testing.T) { + tempDir, binaryPath := buildE2EBinary(t) + writeE2EConfig(t, tempDir, pipelineConfigYAML(" pipeline:\n rank:\n on_error: fail\n")) + writePipelineADRs(t, tempDir) + if err := os.WriteFile(filepath.Join(tempDir, fixtureFilename), []byte(violationFixtureContent()), 0644); err != nil { + t.Fatalf("Failed to create fixture: %v", err) + } + failing := "embed-fails.js" + if err := os.WriteFile(filepath.Join(tempDir, failing), []byte(fmt.Sprintf("console.log(%q);\n", testutil.MockEmbedFailureTrigger)), 0644); err != nil { + t.Fatalf("Failed to create fixture: %v", err) + } + + cmd := exec.Command(binaryPath, "check", "--format", "json", fixtureFilename, failing) + cmd.Dir = tempDir + cmd.Env = append(os.Environ(), "ARCHGUARD_API_KEY=mock_key") + var outBuf, errBuf bytes.Buffer + cmd.Stdout, cmd.Stderr = &outBuf, &errBuf + err := cmd.Run() + exitError, ok := err.(*exec.ExitError) + if !ok || exitError.ExitCode() != 6 { + t.Fatalf("expected exit code 6, got err %v. stderr: %s", err, errBuf.String()) + } + + var report struct { + Count int `json:"count"` + Failures []map[string]string `json:"failures"` + } + if err := json.Unmarshal(outBuf.Bytes(), &report); err != nil { + t.Fatalf("stdout is not valid JSON: %v\nstdout: %q", err, outBuf.String()) + } + if report.Count == 0 || len(report.Failures) != 1 { + t.Errorf("count = %d, failures = %v; want the healthy file's drift and one failure both reported", report.Count, report.Failures) + } +} + +func TestE2E_PipelineOnErrorFail_UpdateBaselineDoesNotWriteBaseline(t *testing.T) { + tempDir, binaryPath := buildE2EBinary(t) + writeE2EConfig(t, tempDir, pipelineConfigYAML(" pipeline:\n rank:\n on_error: fail\n")) + writePipelineADRs(t, tempDir) + failing := "embed-fails.js" + if err := os.WriteFile(filepath.Join(tempDir, failing), []byte(fmt.Sprintf("console.log(%q);\n", testutil.MockEmbedFailureTrigger)), 0644); err != nil { + t.Fatalf("Failed to create fixture: %v", err) + } + gitAdd(t, tempDir, failing) + + cmd := exec.Command(binaryPath, "check", "--update-baseline") + cmd.Dir = tempDir + cmd.Env = append(os.Environ(), "ARCHGUARD_API_KEY=mock_key") + out, err := cmd.CombinedOutput() + exitError, ok := err.(*exec.ExitError) + if !ok || exitError.ExitCode() != 6 { + t.Fatalf("expected exit code 6, got err %v. Output: %s", err, out) + } + if _, statErr := os.Stat(filepath.Join(tempDir, baseline.Path)); !os.IsNotExist(statErr) { + t.Errorf("baseline file must not be written when a stage failed, stat err: %v", statErr) + } +} + func TestE2E_PipelineConfig_InvalidExitsWithConfigError(t *testing.T) { tests := []struct { name string @@ -119,6 +234,7 @@ func TestE2E_PipelineConfig_InvalidExitsWithConfigError(t *testing.T) { {"unrecognized stage", " pipeline:\n pre_judge:\n scorer: cosine\n", "analysis.pipeline: unrecognized stage \"pre_judge\""}, {"out-of-range threshold", " pipeline:\n rerank:\n threshold: 2\n", "analysis.pipeline.rerank.threshold"}, {"non-positive top_k", " pipeline:\n rank:\n top_k: 0\n", "analysis.pipeline.rank.top_k"}, + {"unknown on_error", " pipeline:\n rerank:\n on_error: warn\n", "analysis.pipeline.rerank.on_error"}, } for _, tt := range tests {