fix(qavis-advisory): say why the advisory was skipped instead of failing open silently by norvalbv · Pull Request #178 · norvalbv/devkit

norvalbv · 2026-07-22T10:01:12Z

The gate shells qavis route --staged --gate and fails OPEN when qavis can't be reached — correct, but it did so with zero output. defaultRouteVerdict collapsed every failure into a bare null, which runQavisAdvisory then handled identically to SILENT, so three states were indistinguishable at the terminal: 'nothing to QA', 'qavis not on PATH', and 'route blew up'. A dead gate looked exactly like a healthy quiet one, and could stay that way for months.

Change

The route result now carries its reason, so there is one printer and no detail is discarded:

RouteResult replaces 'ADVISE' | 'SILENT' | null; the null arm holds skip, the human phrase for why the advisory didn't run.
ENOENT is discriminated from a qavis that ran and exited non-zero (which throws with status set), separating 'never installed' from 'installed but broken'.
The third silent null — exit 0 with unparseable stdout — is now reported as qavis route printed no verdict (…) rather than read as SILENT.
On a skip it prints one stderr line plus the existing mute, and still returns 0 on a plain commit and under a strict ship alike. An advisor's own failure must never cost an exit code, so the line is the only signal there is.

qavis-advisory: skipped — qavis not on PATH.
   (.qavis/recipe.json is present, so this repo expects it; mute with GUARD_NO_QAVIS_ADVISORY=1.)

A repo WITHOUT .qavis/recipe.json stays entirely silent — the zero-weight path for non-qavis consumers is unchanged and test-asserted.

Contract

Untouched: 0 = continue, 3 = ADVISE under a strict ship. No exit 1, no fail-closed. cli.mts and the husky fragments needed no change. The new stderr text collides with none of the grep attributions in commit-with-gate-capture.sh.

Verification

Typecheck + lint clean; suite green. Exercised end-to-end against this repo: healthy (silent, rc=0); qavis hidden from PATH (warns, rc=0); hidden + GUARD_AI_STRICT=1 (still rc=0); muted (nothing at all); a stub qavis exiting 7 (qavis route failed: …, rc=0).

Follow-up

A devkit doctor advisory line reporting recipe + binary presence is deliberately NOT in this PR: cli/commands/doctor.mts is grandfathered at exactly 894 lines in eslint/baselines/size-lines.json (shrink-only, zero headroom), so it needs an extraction to pay for itself. That lands separately.

Summary by CodeRabbit

Bug Fixes
- Advisory checks now clearly report when they cannot run, including missing tools, route failures, or invalid results.
- Fail-open behavior remains in place, while skipped checks now provide an explanatory diagnostic instead of failing silently.
- Valid SILENT and ADVISE outcomes remain quiet and preserve their existing exit behavior.
Documentation
- Updated advisory gate documentation to describe the new diagnostic output and skip reasons.

…ing open silently The gate shells `qavis route --staged --gate` and fails OPEN when qavis can't be reached — correct, but it did so with zero output. `defaultRouteVerdict` collapsed every failure into a bare `null`, which `runQavisAdvisory` then handled identically to SILENT, so three states were indistinguishable at the terminal: 'nothing to QA', 'qavis not on PATH', and 'route blew up'. A dead gate looked exactly like a healthy quiet one, and could stay that way for months. ## Change The route result now carries its reason, so there is one printer and no detail is discarded: - `RouteResult` replaces `'ADVISE' | 'SILENT' | null`; the null arm holds `skip`, the human phrase for why the advisory didn't run. - `ENOENT` is discriminated from a qavis that ran and exited non-zero (which throws with `status` set), separating 'never installed' from 'installed but broken'. - The third silent null — exit 0 with unparseable stdout — is now reported as `qavis route printed no verdict (…)` rather than read as SILENT. - On a skip it prints one stderr line plus the existing mute, and still returns 0 on a plain commit and under a strict ship alike. An advisor's own failure must never cost an exit code, so the line is the only signal there is. ``` qavis-advisory: skipped — qavis not on PATH. (.qavis/recipe.json is present, so this repo expects it; mute with GUARD_NO_QAVIS_ADVISORY=1.) ``` A repo WITHOUT `.qavis/recipe.json` stays entirely silent — the zero-weight path for non-qavis consumers is unchanged and test-asserted. ## Contract Untouched: 0 = continue, 3 = ADVISE under a strict ship. No exit 1, no fail-closed. `cli.mts` and the husky fragments needed no change. The new stderr text collides with none of the grep attributions in `commit-with-gate-capture.sh`. ## Verification Typecheck + lint clean; suite green. Exercised end-to-end against this repo: healthy (silent, rc=0); qavis hidden from PATH (warns, rc=0); hidden + GUARD_AI_STRICT=1 (still rc=0); muted (nothing at all); a stub qavis exiting 7 (`qavis route failed: …`, rc=0). ## Follow-up A `devkit doctor` advisory line reporting recipe + binary presence is deliberately NOT in this PR: `cli/commands/doctor.mts` is grandfathered at exactly 894 lines in `eslint/baselines/size-lines.json` (shrink-only, zero headroom), so it needs an extraction to pay for itself. That lands separately.

coderabbitai · 2026-07-22T10:01:43Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f5891806-01ce-4ba2-b2ca-c017f2e5c90d

📥 Commits

Reviewing files that changed from the base of the PR and between 645d05d and 8487af0.

📒 Files selected for processing (3)

docs/decisions/qavis-advisory-gate.md
gate-engine/qavis-advisory/__tests__/qavis-advisory.test.mts
gate-engine/qavis-advisory/check.mts

📝 Walkthrough

Walkthrough

Changes

Qavis advisory routing and diagnostics

Layer / File(s)	Summary
Structured routing results `gate-engine/qavis-advisory/check.mts`	Introduces `RouteResult` and updates routing dependencies to preserve explicit skip reasons for missing qavis, route failures, and invalid output.
Fail-open diagnostics and coverage `gate-engine/qavis-advisory/check.mts`, `gate-engine/qavis-advisory/__tests__/*`, `docs/decisions/qavis-advisory-gate.md`	Reports skipped advisory execution on stderr while retaining exit-0 behavior, and expands tests and decision documentation for the updated contract.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant runQavisAdvisory
  participant defaultRoute
  participant qavis
  participant stderr
  runQavisAdvisory->>defaultRoute: request advisory verdict
  defaultRoute->>qavis: execute qavis route
  qavis-->>defaultRoute: verdict or skip condition
  defaultRoute-->>runQavisAdvisory: RouteResult
  runQavisAdvisory->>stderr: report skip reason when verdict is null
  runQavisAdvisory-->>runQavisAdvisory: return exit code 0 for fail-open skip

Possibly related PRs

norvalbv/devkit#40: Modifies the same qavis advisory gate routing and exit-contract implementation and tests.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/qavis-advisory-loud-fail-open

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

fix(qavis-advisory): say why the advisory was skipped instead of failing open silently

Two pieces of housekeeping found while cleaning the working tree after PR #178. ## 1. The committed skill projections disagree with their own source `skills/using-devkit/SKILL.md` (the source) and `.devkit/skills-manifest.json` both hash to `d678d168…`, but the committed `.claude/` and `.cursor/` projections are still `65a1e814…` — an earlier revision. A source change landed without its re-synced projections, so `devkit doctor` on a fresh clone reports skills DRIFT against the manifest that ships beside them. These are the `devkit sync-skills` output, restoring the invariant that source, manifest, and both projections agree. The content they were missing is the ship argument-ordering guidance: the positional `<branch> "<title>"`-before-flags rule, the linked-worktree row, and the `--pr` row for a branch that already exists on origin. No source change here — only the projections move, and they move to what the manifest already claims. ## 2. Track `search-code.config.json` It was sitting untracked and unignored, so it surfaced as permanent noise in every `git status`. It is deliberate, documented configuration rather than machine state: it pins the dup-gate describer to `mode: remote` (through the Claude Code login — zero API spend and no local GPU dependency, after local describes were measured at ~78s/chunk and blocked a ship for 40+ minutes), and mirrors `guard.config.json`'s `scanRoots` in `indexing.sourceRoots` so the matcher scans the authored surface only. Embeddings stay on-device, so the index and its provenance stamp are unchanged. Committing it makes that the shared default instead of one machine's local tuning.

…tchet by extracting the asset checks Follow-up to #178. That PR made the qavis-advisory gate name its own failure at commit time; this one makes the same signal visible from `devkit doctor`, so a dead gate is discoverable without waiting for a commit. ## The advisory line `printQavisAdvisoryHealth` prints one line in all three doctor modes (package, overlay, self-host) when `qavis-advisory` is a selected guard: ``` ✓ qavis-advisory: qavis on PATH (.qavis/recipe.json present) · qavis-advisory: .qavis/recipe.json present but qavis is NOT on PATH — the QA advisory is skipped on every commit (install qavis, or drop the guard) · qavis-advisory: no .qavis/recipe.json — gate inert (nothing to QA) ``` Deliberately **advisory**: never a `CheckResult`, never in `results`, never a `--fix` target, so it cannot move doctor's exit code. A repo that keeps the guard selected but does not install qavis made a choice, not drift. Resolved against the **git root** — that is the cwd the husky fragment shells the gate from, so doctor reports what the hook would actually see rather than what this cwd sees. The probe (`qavisOnPath`) is a plain PATH scan. It never calls `qavis route`, so doctor stays free of model spend. ## Paying the size ratchet `cli/commands/doctor.mts` is grandfathered at **894 lines** in `eslint/baselines/size-lines.json` — shrink-only, and it was sitting at 893. There was no room for the feature, so this PR pays for it first: - `cli/lib/doctor/asset-checks.mts` — the four synced-asset drift checks (`checkSkills`, `checkAgents`, `checkAgentHookScripts`, `checkRegistrations`). They share one contract (a `.devkit/*-manifest.json` sha256 per file; drift is any disagreement between record, bundled source, and consumer copy) and are **leaf** checks that call nothing else in doctor — so moving them leaves the command's orchestration, which its own comments deliberately keep together, untouched. - `cli/lib/doctor/check-result.mts` — the `CheckResult` primitive. `doctor.mts` goes **893 → 771**, comfortably back under the cap with room for the feature. A bonus falls out: `checkCommitMsgHook` previously restated `CheckResult` structurally because importing it from `doctor.mts` would have been a cycle. With the type in its own module it just imports it — one real structural duplicate removed. `cli/lib` sits at exactly the 12-file fan-out cap, so this lands as a **registered subfolder** rather than flat files: `doctor` is added to `structure.trees[cli].libDomains.lib` in `guard.config.json`. ## Verification `vitest run` 1954 passed / 0 failed, `typecheck`, `lint`, `lint:structure` all clean. `bun cli/index.mts doctor` on this repo (self-host, qavis installed) prints the ✓ line. New test in `init-doctor.test.mts` covers both the inert arm and the NOT-on-PATH arm — the latter with a scrubbed PATH — and asserts the exit code is unmoved by either. `qavisOnPath` is unit-tested against a synthetic PATH.

Minor bump: recent changes to the review process, plus the ship-correctness fixes merged since v0.37.2: - #163 — reject flags in the <branch>/<title> positional slots; base the review judges on the ship's resolved base rather than origin/HEAD. - #167 — link gate deps (.husky/_, node_modules, coverage) from the main worktree when the ship is run from a linked worktree, instead of failing closed. - #178/#179/#180 — qavis-advisory loud-fail-open fix, using-devkit projection re-sync, doctor qavis-advisory liveness reporting. package.json only, matching prior `chore: bump version to X` commits; devkitRef and dist/ are handled by the release step.

fix(qavis-advisory): say why the advisory was skipped instead of failing open silently

Two pieces of housekeeping found while cleaning the working tree after PR #178. ## 1. The committed skill projections disagree with their own source `skills/using-devkit/SKILL.md` (the source) and `.devkit/skills-manifest.json` both hash to `d678d168…`, but the committed `.claude/` and `.cursor/` projections are still `65a1e814…` — an earlier revision. A source change landed without its re-synced projections, so `devkit doctor` on a fresh clone reports skills DRIFT against the manifest that ships beside them. These are the `devkit sync-skills` output, restoring the invariant that source, manifest, and both projections agree. The content they were missing is the ship argument-ordering guidance: the positional `<branch> "<title>"`-before-flags rule, the linked-worktree row, and the `--pr` row for a branch that already exists on origin. No source change here — only the projections move, and they move to what the manifest already claims. ## 2. Track `search-code.config.json` It was sitting untracked and unignored, so it surfaced as permanent noise in every `git status`. It is deliberate, documented configuration rather than machine state: it pins the dup-gate describer to `mode: remote` (through the Claude Code login — zero API spend and no local GPU dependency, after local describes were measured at ~78s/chunk and blocked a ship for 40+ minutes), and mirrors `guard.config.json`'s `scanRoots` in `indexing.sourceRoots` so the matcher scans the authored surface only. Embeddings stay on-device, so the index and its provenance stamp are unchanged. Committing it makes that the shared default instead of one machine's local tuning.

…tchet by extracting the asset checks Follow-up to #178. That PR made the qavis-advisory gate name its own failure at commit time; this one makes the same signal visible from `devkit doctor`, so a dead gate is discoverable without waiting for a commit. ## The advisory line `printQavisAdvisoryHealth` prints one line in all three doctor modes (package, overlay, self-host) when `qavis-advisory` is a selected guard: ``` ✓ qavis-advisory: qavis on PATH (.qavis/recipe.json present) · qavis-advisory: .qavis/recipe.json present but qavis is NOT on PATH — the QA advisory is skipped on every commit (install qavis, or drop the guard) · qavis-advisory: no .qavis/recipe.json — gate inert (nothing to QA) ``` Deliberately **advisory**: never a `CheckResult`, never in `results`, never a `--fix` target, so it cannot move doctor's exit code. A repo that keeps the guard selected but does not install qavis made a choice, not drift. Resolved against the **git root** — that is the cwd the husky fragment shells the gate from, so doctor reports what the hook would actually see rather than what this cwd sees. The probe (`qavisOnPath`) is a plain PATH scan. It never calls `qavis route`, so doctor stays free of model spend. ## Paying the size ratchet `cli/commands/doctor.mts` is grandfathered at **894 lines** in `eslint/baselines/size-lines.json` — shrink-only, and it was sitting at 893. There was no room for the feature, so this PR pays for it first: - `cli/lib/doctor/asset-checks.mts` — the four synced-asset drift checks (`checkSkills`, `checkAgents`, `checkAgentHookScripts`, `checkRegistrations`). They share one contract (a `.devkit/*-manifest.json` sha256 per file; drift is any disagreement between record, bundled source, and consumer copy) and are **leaf** checks that call nothing else in doctor — so moving them leaves the command's orchestration, which its own comments deliberately keep together, untouched. - `cli/lib/doctor/check-result.mts` — the `CheckResult` primitive. `doctor.mts` goes **893 → 771**, comfortably back under the cap with room for the feature. A bonus falls out: `checkCommitMsgHook` previously restated `CheckResult` structurally because importing it from `doctor.mts` would have been a cycle. With the type in its own module it just imports it — one real structural duplicate removed. `cli/lib` sits at exactly the 12-file fan-out cap, so this lands as a **registered subfolder** rather than flat files: `doctor` is added to `structure.trees[cli].libDomains.lib` in `guard.config.json`. ## Verification `vitest run` 1954 passed / 0 failed, `typecheck`, `lint`, `lint:structure` all clean. `bun cli/index.mts doctor` on this repo (self-host, qavis installed) prints the ✓ line. New test in `init-doctor.test.mts` covers both the inert arm and the NOT-on-PATH arm — the latter with a scrubbed PATH — and asserts the exit code is unmoved by either. `qavisOnPath` is unit-tested against a synthetic PATH.

Minor bump: recent changes to the review process, plus the ship-correctness fixes merged since v0.37.2: - #163 — reject flags in the <branch>/<title> positional slots; base the review judges on the ship's resolved base rather than origin/HEAD. - #167 — link gate deps (.husky/_, node_modules, coverage) from the main worktree when the ship is run from a linked worktree, instead of failing closed. - #178/#179/#180 — qavis-advisory loud-fail-open fix, using-devkit projection re-sync, doctor qavis-advisory liveness reporting. package.json only, matching prior `chore: bump version to X` commits; devkitRef and dist/ are handled by the release step.

norvalbv merged commit ccd06f7 into main Jul 22, 2026
1 of 2 checks passed

This was referenced Jul 22, 2026

chore: re-sync using-devkit projections and track the search-code config #179

Merged

feat(doctor): report qavis-advisory gate liveness, paying the size ratchet by extracting the asset checks #180

Merged

chore: bump version to 0.38.0 #181

Merged

norvalbv added a commit that referenced this pull request Jul 22, 2026

Merge pull request #178 from norvalbv/fix/qavis-advisory-loud-fail-open

49bdabd

fix(qavis-advisory): say why the advisory was skipped instead of failing open silently

norvalbv added a commit that referenced this pull request Jul 24, 2026

Merge pull request #178 from norvalbv/fix/qavis-advisory-loud-fail-open

18daf40

fix(qavis-advisory): say why the advisory was skipped instead of failing open silently

coderabbitai Bot mentioned this pull request Jul 28, 2026

fix(qavis): make advisory remediation executable #265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(qavis-advisory): say why the advisory was skipped instead of failing open silently - #178

fix(qavis-advisory): say why the advisory was skipped instead of failing open silently#178
norvalbv merged 1 commit into
mainfrom
fix/qavis-advisory-loud-fail-open

norvalbv commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

norvalbv commented Jul 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change

Contract

Verification

Follow-up

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

norvalbv commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading