|
| 1 | +--- |
| 2 | +id: ARCH-026 |
| 3 | +title: Strict Mode and Check Severity Tiers |
| 4 | +domain: architecture |
| 5 | +rules: false |
| 6 | +files: |
| 7 | + - "src/engine/reporter.ts" |
| 8 | + - "src/commands/check.ts" |
| 9 | + - "src/commands/review-context.ts" |
| 10 | + - "src/engine/context.ts" |
| 11 | + - "src/commands/adr/sync.ts" |
| 12 | + - "src/helpers/project-config.ts" |
| 13 | + - "src/formats/project-config.ts" |
| 14 | +--- |
| 15 | + |
| 16 | +## Context |
| 17 | + |
| 18 | +`archgate check` reports findings at three distinct severities (`error`, `warning`, `info`) plus a set of diagnostics that are structurally similar to warnings but were never counted toward `pass`: `briefingWarnings` (an ADR's "Decision" or "Do's and Don'ts" section exceeds the `review-context` briefing budget — see [ARCH-003 §5's briefing truncation](./ARCH-003-output-formatting.md)), `suppressionWarnings` (missing-reason or unused `archgate-ignore` comments), and `unparsedAdrs` (an ADR file that failed to parse and was silently excluded from every other check). `--max-warnings <n>` already existed to gate on the first category (rule-severity `warning` violations) but had no way to reach the other three — they were hard-coded as advisory in `reporter.ts`'s `buildSummary()`, with a comment stating they "never affect `pass`." |
| 19 | + |
| 20 | +This split was deliberate, not an oversight: an ADR whose "Do's and Don'ts" cannot shrink below the briefing cap without losing a normative clause is _expected_ to exceed it, so failing a local `check` run on that basis by default would punish authors for writing complete governance text. The same reasoning applies to a single unused suppression comment during active development. But the same reasoning does not hold in CI, where the project may want zero tolerance: an ADR corpus that silently grows briefing overruns, stale suppressions, or unparseable files is a governance quality regression that nothing currently catches, because `check`'s own exit code stays 0 regardless. |
| 21 | + |
| 22 | +This is the same class of problem linters solve with a "warnings as errors" mode: loose during local iteration, strict at the CI gate. ESLint's `--max-warnings 0` and Rust's `-D warnings` are both single-flag escalations of an existing severity model, not a redesign of it. Two categories of "warning" exist in this codebase, and only one fits that model: |
| 23 | + |
| 24 | +1. **Structured governance diagnostics** — the four categories above, shared between `check` and `review-context` (which reuses `buildSummary()` under `--run-checks`, and independently tracks `truncatedBriefings` when briefings are requested via `--verbose`). These are typed, counted, and already flow through `ReportSummary`. |
| 25 | +2. **Ad-hoc operational messages** — bare `logWarn()` calls in `src/commands/init.ts`, `src/commands/plugin/install.ts`, and `src/helpers/credential-store.ts` (e.g. "Copilot CLI not found, install manually", git-credential-helper quirks). These have no counting or severity model and are not governance findings — they describe the state of the local machine during an install/login flow, not a defect in the ADR corpus. |
| 26 | + |
| 27 | +`archgate adr sync` carries a related but distinct concept: `result.errors`, incremented when an imported ADR's source can't be resolved, its upstream repo can't be cloned, or its local/upstream file is missing. These are genuine ADR-corpus integrity problems — the same spirit as the advisory categories above — but they are a separate counter on a separate command, not one of the four `ReportSummary` fields, and `sync` already has an unrelated `--check` flag (exit 1 when upstream has drifted) that must not be conflated with error-tolerance. |
| 28 | + |
| 29 | +**Alternatives considered:** |
| 30 | + |
| 31 | +- **Redefine `--max-warnings` to sum all four categories into one count.** Rejected: it would silently change what `--max-warnings 0` means for any CI pipeline already relying on it, and collapsing unrelated diagnostics (a rule warning vs. an unparseable ADR file) into one number loses the ability to say _why_ a run failed. |
| 32 | +- **One flag per advisory category** (`--fail-on-briefing-warnings`, `--fail-on-suppression-warnings`, `--fail-on-unparsed-adrs`). Rejected as disproportionate: it multiplies flags and documentation for a "loose in dev, strict in CI" toggle that is conceptually one on/off switch in every linter that offers it. |
| 33 | +- **Scope the new flag to every `logWarn()` call site in the CLI**, including `init`/`plugin install`/`credential-store`. Rejected: those sites have no counting or severity model to hook into, and are not quality findings about the governed project — see Decision's Scope subsection. |
| 34 | + |
| 35 | +## Decision |
| 36 | + |
| 37 | +`archgate check`, `archgate review-context`, and `archgate adr sync` MUST support a blanket `--strict` boolean flag that escalates otherwise-advisory diagnostics into command failures (exit 1), on top of each command's existing severity/threshold model. `--strict` MUST resolve CLI flag first, else a `strict: boolean` key in `.archgate/config.json` (via `getConfiguredStrict()`, mirroring `baseBranch`/`getConfiguredBaseBranch()`), else `false`. |
| 38 | + |
| 39 | +**Scope:** the four `ReportSummary` categories (rule-severity `warning`, `briefingWarnings`, `suppressionWarnings`, `unparsedAdrs`) and `adr sync`'s `result.errors`. NOT the ad-hoc `logWarn()` sites in `init.ts`, `plugin/install.ts`, `credential-store.ts` — those describe local-machine state, not ADR-corpus quality. |
| 40 | + |
| 41 | +**Per-command semantics:** |
| 42 | + |
| 43 | +- **`check`** (`buildSummary()`): acts as `maxWarnings: 0` when `--max-warnings` is unset — an explicit value always wins. Sets `strictAdvisoryExceeded` when `briefingWarnings`, `suppressionWarnings`, or `unparsedAdrs` is non-empty; either flips `pass` to `false`. |
| 44 | +- **`review-context`**: fails when `truncatedBriefings` is non-empty, or, with `--run-checks`, when `checkSummary.warningsExceeded`/`strictAdvisoryExceeded` is true. Does NOT gate on `checkSummary.failed`/`ruleErrors` — stays a context generator, not a second gate; `check` alone is authoritative for rule violations. |
| 45 | +- **`adr sync`**: fails when `result.errors > 0`, composable with `--check`'s own exit-1 condition (`withChanges > 0`). |
| 46 | + |
| 47 | +**Capabilities:** one flag and precedence across all three commands; composes with existing per-command controls rather than replacing them; config-persistable so CI need not pass the flag every invocation. |
| 48 | + |
| 49 | +## Do's and Don'ts |
| 50 | + |
| 51 | +### Do |
| 52 | + |
| 53 | +- **DO** resolve `strict` as `opts.strict ?? getConfiguredStrict(projectRoot) ?? false` in every command that supports it. |
| 54 | +- **DO** let an explicit `--max-warnings` value win over `strict`'s implicit zero-tolerance for rule-severity warnings. |
| 55 | +- **DO** compute `strict`-driven pass/fail logic once, inside `reporter.ts`'s `buildSummary()`/`getExitCode()`, and have every caller consume `strictAdvisoryExceeded` rather than re-deriving the condition. |
| 56 | +- **DO** print the full result payload before exiting non-zero under `--strict` — a piped consumer must still see what was found, not just the failure. |
| 57 | +- **DO** extend `--strict` to a new advisory-style diagnostic via the `strictAdvisoryExceeded` computation in `buildSummary()`, not a parallel condition elsewhere. |
| 58 | +- **DO** log a clear, `--strict`-attributed message (via `logWarn()`) naming which finding(s) caused the failure, before calling `exitWith(1)`. |
| 59 | +- **DO** treat `adr sync`'s `result.errors` as the sync-specific analogue of the advisory categories, kept as its own counter rather than folded into `ReportSummary`. |
| 60 | + |
| 61 | +### Don't |
| 62 | + |
| 63 | +- **DON'T** add `--strict` to `init`, `plugin install`, or `credential-store` output — those `logWarn()` calls are operational, not governance findings. |
| 64 | +- **DON'T** let `--strict` change what `--max-warnings 0` means on its own — an explicit `--max-warnings` always wins. |
| 65 | +- **DON'T** gate `review-context --strict` on `checkSummary.failed`/`ruleErrors` — that turns a context generator into a second compliance gate; `check` alone is responsible for rule violations. |
| 66 | +- **DON'T** introduce a `--no-strict` negation flag without a matching need — no boolean flag here ships a negation, and `strict` already has three resolution states without one. |
| 67 | +- **DON'T** drop an advisory finding from the payload when `--strict` is off — advisory categories MUST still surface; only pass/fail and exit code change with `--strict`. |
| 68 | + |
| 69 | +## Consequences |
| 70 | + |
| 71 | +### Positive |
| 72 | + |
| 73 | +- **CI can enforce zero-tolerance without changing local dev ergonomics**: a project sets `strict: true` in `.archgate/config.json` (or passes `--strict` in its CI job) while local `archgate check` runs stay lenient by default. |
| 74 | +- **One mental model across three commands**: contributors learn `--strict` once; its precedence and general "escalate advisories" meaning transfers between `check`, `review-context`, and `adr sync` without per-command relearning. |
| 75 | +- **No silent behavior change for existing `--max-warnings` users**: `--strict` is additive — a pipeline already pinning `--max-warnings 0` sees no change, and `strict`'s implicit zero only applies when `--max-warnings` is absent. |
| 76 | +- **Advisory categories stay genuinely advisory by default**: an ADR author hitting the briefing-budget cap locally is not blocked unless the project has opted into `--strict`, preserving the original rationale for treating these as non-blocking. |
| 77 | + |
| 78 | +### Negative |
| 79 | + |
| 80 | +- **Two severity axes to reason about**: reviewers and contributors now must track both "does this violation have `severity: error`" and "is `--strict` active" to predict whether a given run passes — more surface area than a single severity field. |
| 81 | +- **`review-context`'s scope boundary (not gating on rule violations) is a subtle, easy-to-miss distinction** from `check`'s full-gate behavior; a contributor extending `--strict` there without reading this ADR could accidentally widen it into a second compliance gate. |
| 82 | +- **`adr sync`'s `result.errors` uses the same `--strict` flag name but a different underlying condition** than `check`/`review-context`'s advisory categories — consistent naming, inconsistent mechanism, which this ADR mitigates only by documentation, not by shared code. |
| 83 | + |
| 84 | +### Risks |
| 85 | + |
| 86 | +- **New advisory-style diagnostics added to `check` later may be forgotten in the `strictAdvisoryExceeded` computation**, silently staying non-blocking under `--strict` when the author intended otherwise. |
| 87 | + - **Mitigation:** the Do's and Don'ts above prescribe extending `strictAdvisoryExceeded` in `buildSummary()` as the single point of change; code review MUST check any new `ReportSummary` array field against this ADR. |
| 88 | +- **A project enabling `strict: true` in `.archgate/config.json` may be surprised when a previously-passing `check` run starts failing in CI**, since the config default applies with no flag on the command line. |
| 89 | + - **Mitigation:** `getConfiguredStrict()` only takes effect when the CLI flag is omitted entirely, matching the pre-existing `baseBranch` precedent; the change is opt-in (the key must be explicitly added) and each command's `--strict`-driven failure logs which finding(s) caused it. |
| 90 | +- **No automated rule currently checks that a command claiming to read `ReportSummary`'s advisory fields also reads `strictAdvisoryExceeded`** — this ADR documents a convention, not a mechanically-enforced invariant. |
| 91 | + - **Mitigation:** tracked as future work below; until an `archgate` rule exists, code review is the enforcement layer for this specific risk. |
| 92 | + |
| 93 | +## Compliance and Enforcement |
| 94 | + |
| 95 | +### Automated Enforcement |
| 96 | + |
| 97 | +- **Not yet implemented.** No `archgate` rule currently verifies that a command consuming `ReportSummary`'s advisory arrays also branches on `strictAdvisoryExceeded`, or that `--strict`/config-precedence resolution matches the `opts.strict ?? getConfiguredStrict(projectRoot) ?? false` pattern. A companion `.rules.ts` could be added later if this drifts in practice — this ADR's `rules: false` reflects that no such rule exists yet. |
| 98 | +- `bun test` covers the behavior directly: `tests/engine/reporter-strict.test.ts` (severity-tier composition), `tests/integration/check-strict.test.ts`, `tests/integration/review-context-strict.test.ts`, and `tests/commands/adr/sync-strict.test.ts` (per-command gating, config precedence, non-regression of the advisory-never-blocks-by-default baseline). |
| 99 | + |
| 100 | +### Manual Enforcement |
| 101 | + |
| 102 | +Code reviewers MUST verify: |
| 103 | + |
| 104 | +1. Any new `ReportSummary` field intended as an advisory (non-blocking-by-default) diagnostic is added to the `strictAdvisoryExceeded` computation in `buildSummary()`, not left out. |
| 105 | +2. A new command supporting `--strict` resolves it via the `opts.strict ?? getConfiguredStrict(projectRoot) ?? false` precedence, not a bespoke resolution. |
| 106 | +3. `review-context --strict` changes do not begin gating on ordinary rule violations (`checkSummary.failed`/`ruleErrors`) without a new ADR decision reversing that scope boundary. |
| 107 | +4. A `--strict`-driven failure logs an explanatory message before exiting non-zero. |
| 108 | + |
| 109 | +### Exceptions |
| 110 | + |
| 111 | +None currently approved. A command with a genuine reason to interpret `--strict` differently from the precedence/scope rules above requires a new ADR (or an amendment to this one), approved the same way as any other governance change. |
| 112 | + |
| 113 | +## References |
| 114 | + |
| 115 | +- [ARCH-002 — Error Handling](./ARCH-002-error-handling.md) — defines the exit-code model (`0`/`1`/`2`/`130`) this ADR's `--strict`-driven `exitWith(1)` calls follow. |
| 116 | +- [ARCH-003 — Output Formatting](./ARCH-003-output-formatting.md) — defines the `--json`/agent-context output conventions `strictAdvisoryExceeded` flows through, and is the source of the briefing-truncation behavior `briefingWarnings` reports on. |
| 117 | +- [ESLint `--max-warnings`](https://eslint.org/docs/latest/use/command-line-interface#--max-warnings) — precedent for a count-based warning threshold, which this ADR's `strict` composes with rather than replaces. |
0 commit comments