From d79bf37673f41d627cfcbf89e3f929dbb586223c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 12:43:35 -0700 Subject: [PATCH 1/2] chore(guards): pin the focus-manifest field registry, document the coverage asymmetry Refs #9860 (items 4 and 5). ITEM 4. FOCUS_MANIFEST_TOP_LEVEL_FIELDS is a hand-kept list of 28 keys whose doc comment claims it is every top-level key parseFocusManifest actually reads. Nothing checked that claim, and it is load-bearing both ways: a key the parser reads but the list omits is reported to operators as an unknown field, while a key in the list the parser never reads is worse -- the validator blesses it, the runtime ignores it, and the operator's setting silently does nothing while every surface says it is fine. #9813 and #9821 each shipped a bug from missing one of this registry's touchpoints. The relation is now computed from the parser's own source rather than trusted. Currently in sync at 28/28, so this pins today's state rather than fixing a live bug. Source-scanned deliberately: the failure is a key the parser reads and the list forgot, which produces no behavioural difference to assert on. Includes an anti-vacuity check, because a refactor renaming the local would otherwise empty both sets and make the assertions pass while checking nothing. Not guarded: the parse/serialize pairing. Four parsers (AutoReview, MaxFindings, SelfHostAiModel, Visual) have no *ConfigToJson of their own because they are nested under the review config and serialized through it, so a naive 1:1 check would fire four false positives -- and a checker that cries wolf gets muted. ITEM 5. The engine coverage asymmetry is now a contributor-facing rule rather than folklore. packages/loopover-engine/src/** is credited by two uploads whose hits Codecov unions, and the two runs disagree about which lines exist (c8 --all instruments files no test imports; v8 does not), so a genuinely 100%-tested engine change can land at ~65% on codecov/patch. The rule: change engine source, add the test to packages/loopover-engine/test/** as well. Also corrects AGENTS.md, which stated that only src/** is measured -- untrue since the engine flag, and misleading in exactly the direction that costs a PR. And records the issue's own closing note: ui:typecheck is in test:ci but not in the root typecheck script, so a locally-clean type change can still break apps/** (#9815). --- .../contributing-to-loopover/reference.md | 14 +++++ AGENTS.md | 5 +- .../focus-manifest-top-level-fields.test.ts | 62 +++++++++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 test/unit/focus-manifest-top-level-fields.test.ts diff --git a/.claude/skills/contributing-to-loopover/reference.md b/.claude/skills/contributing-to-loopover/reference.md index 287306d15f..41d3eb9c55 100644 --- a/.claude/skills/contributing-to-loopover/reference.md +++ b/.claude/skills/contributing-to-loopover/reference.md @@ -150,9 +150,23 @@ checks go green) is the only way to know you didn't break it. patch coverage; a backend `src/**` change owes coverage on **every changed line + branch**. - **Measure unsharded locally:** `npm run test:coverage`. CI shards into 3 and Codecov merges them, so a single local shard under-reports — never trust it. +- **`npm run typecheck` does not cover `apps/**`.** `ui:typecheck` is a separate step, present in + `test:ci` but NOT in the root `typecheck` script — so a type change you validated locally with + `npm run typecheck` can still break the UI build in CI (#9815 shipped exactly this). Run + `npm run ui:typecheck` too, or just run the whole `npm run test:ci`. - **Flaky tests are already tracked.** Every shard uploads a JUnit report (`report_type: test_results`), which auto-enables Codecov Test Analytics with no extra config — check a PR's "Tests" tab or its Codecov bot comment if a test needed a retry, rather than assuming it's pure infra noise. +- **`packages/loopover-engine/src/**` is credited by TWO uploads, and this is the biggest gotcha in the + repo (#9860 item 5).** The unflagged `backend` report (root vitest, v8) and the `engine` flag (the + package's own `node:test` suite, c8 `--all`) both cover those lines, and Codecov *unions* their hits. + The two runs disagree about which lines even exist — c8 `--all` instruments every file including ones + no test imports, v8 does not — so **an engine change that is genuinely 100% tested by root-suite tests + alone can still land as ~65% on `codecov/patch`** (seen on #9821). + → **If you change `packages/loopover-engine/src/**`, add or extend a test in + `packages/loopover-engine/test/**` (the `node:test` suite), not only in root `test/**`.** Run it with + `npm run test --workspace @loopover/engine`. A root-suite test alone is not a reliable way to clear the + patch gate on engine source, no matter what your local `npm run test:coverage` says. --- diff --git a/AGENTS.md b/AGENTS.md index 80d4c96a99..948b52feee 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,7 +20,10 @@ process evolves — edits to those files improve both Claude Code and Codex. a valid linked issue is **auto-MERGED**. So make it perfect before you push. 2. **99% patch coverage, branch-counted.** Aim for **100% of every changed line *and branch*** — test both sides of every `??` / ternary / `&&` (a `SUM()` can return `NULL`, so the nullish arm is real), - plus invariant tests and a regression test for every fix. Only `src/**` is measured by Codecov. + plus invariant tests and a regression test for every fix. Codecov measures `src/**` **and** + `packages/loopover-engine/src/**` — and the engine's lines are credited by two uploads whose hits are + unioned, so an engine change tested only from root `test/**` can still fail the patch gate. Add the test + to `packages/loopover-engine/test/**` too; see the Codecov section of `reference.md`. 3. **The whole local gate must be green:** `npm run test:ci` (+ `npm audit --audit-level=moderate`). Measure coverage **unsharded** with `npm run test:coverage` (CI shards + merges, so a single shard under-reports). diff --git a/test/unit/focus-manifest-top-level-fields.test.ts b/test/unit/focus-manifest-top-level-fields.test.ts new file mode 100644 index 0000000000..2e0ffa780a --- /dev/null +++ b/test/unit/focus-manifest-top-level-fields.test.ts @@ -0,0 +1,62 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; + +import { FOCUS_MANIFEST_TOP_LEVEL_FIELDS } from "../../packages/loopover-engine/src/focus-manifest"; + +// #9860 (item 4): FOCUS_MANIFEST_TOP_LEVEL_FIELDS is a hand-kept list of 28 keys whose doc comment claims it +// is "every top-level key `parseFocusManifest` below actually reads". Nothing checked that claim, and it is +// load-bearing in both directions: +// +// • A key the parser reads but the list omits is silently reported to operators as an UNKNOWN FIELD -- +// the config-lint/validator path warns about a setting that in fact works. +// • A key in the list the parser never reads is worse: the validator blesses it, the runtime ignores it, +// and an operator's setting does nothing while every surface says it is fine. +// +// #9813 and #9821 each shipped a bug from missing one of this registry's touchpoints. This computes the +// relation instead of trusting the comment. +// +// SOURCE-SCANNED rather than exercised through the parser, because the failure is a key the parser reads and +// the list forgot -- which by construction produces no observable behaviour difference to assert on. The +// scan is narrow: `record` is the single local `parseFocusManifest` binds the raw object to, and the function +// body is delimited by the two exported functions around it. +const SOURCE = "packages/loopover-engine/src/focus-manifest.ts"; + +/** Every `record.` / `record[""]` read inside parseFocusManifest's body. */ +function topLevelKeysReadByParser(source: string): Set { + const start = source.indexOf("export function parseFocusManifest(raw: unknown"); + const end = source.indexOf("export function parseFocusManifestContent"); + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + const body = source.slice(start, end); + const keys = new Set(); + for (const match of body.matchAll(/\brecord\.([A-Za-z_][A-Za-z0-9_]*)/g)) keys.add(match[1]!); + for (const match of body.matchAll(/\brecord\["([^"]+)"\]/g)) keys.add(match[1]!); + return keys; +} + +describe("FOCUS_MANIFEST_TOP_LEVEL_FIELDS is the set the parser actually reads (#9860)", () => { + const source = readFileSync(SOURCE, "utf8"); + + it("declares every key parseFocusManifest reads", () => { + // Missing here => the validator calls a working setting an unknown field. + const read = topLevelKeysReadByParser(source); + const declared = new Set(FOCUS_MANIFEST_TOP_LEVEL_FIELDS); + expect([...read].filter((key) => !declared.has(key)).sort()).toEqual([]); + }); + + it("declares nothing the parser ignores", () => { + // Present here but unread => the validator blesses a setting that silently does nothing at runtime, + // which is the more dangerous direction: every surface reports success. + const read = topLevelKeysReadByParser(source); + expect(FOCUS_MANIFEST_TOP_LEVEL_FIELDS.filter((key) => !read.has(key)).sort()).toEqual([]); + }); + + it("guards against a vacuous scan: the parser really does read a known key", () => { + // Without this, a refactor that renamed `record` would empty both sets and make the two assertions above + // pass while checking nothing at all. + const read = topLevelKeysReadByParser(source); + expect(read.size).toBeGreaterThan(20); + expect(read.has("wantedPaths")).toBe(true); + expect(read.has("gate")).toBe(true); + }); +}); From 3036ccf17bc1adc300dc239ec409f1b020f55002 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 12:39:08 -0700 Subject: [PATCH 2/2] chore(mcp): derive the help banner's --login command list instead of typing it Refs #9860 (item 1). The LOOPOVER_LOGIN line in `loopover-mcp --help` named its commands as hand-typed prose, and it had already drifted: it omitted contributor-profile, explain-review-risk and watch, all three of which accept --login and resolve it through the same resolveLogin fallback. A user reading --help was told those commands would not pick up their configured login. They do. Derived from CLI_COMMAND_SPEC now -- every command declaring --login in its usage resolves it that way, and resolveLogin is the single helper that does it, so the usage table is the fact to read rather than a second list to keep in step. Also drops a no-op .replace() on the usage line beside it. Items 1 and 2 of #9860 are otherwise already closed by the MCP contract epic: STDIO_TOOL_DESCRIPTORS derives from STDIO_TOOL_NAMES, printHelp's command list derives from CLI_COMMAND_SPEC, the README block is generated by mcp:tool-reference, and the stdio bin now declares no zod shapes of its own -- all 119 z. uses are z.infer over contract-imported schemas. This line was the one hand-maintained list left in that surface. --- packages/loopover-mcp/bin/loopover-mcp.ts | 15 ++++++++--- test/unit/mcp-cli-help.test.ts | 31 +++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/packages/loopover-mcp/bin/loopover-mcp.ts b/packages/loopover-mcp/bin/loopover-mcp.ts index 49248dedad..8bc4a1bb4d 100644 --- a/packages/loopover-mcp/bin/loopover-mcp.ts +++ b/packages/loopover-mcp/bin/loopover-mcp.ts @@ -4343,9 +4343,16 @@ function printHelp() { // construction, which the old hand-written string provably did not guarantee -- it had already // dropped commands. `--stdio` is the one non-command entry, listed first because it is the mode // MCP clients launch. - const usageLines = ["--stdio", ...Object.values(CLI_COMMAND_SPEC).flatMap((entry) => entry.usage)] - .map((line) => ` loopover-mcp ${line}`.replace("loopover-mcp --stdio", "loopover-mcp --stdio")) - .join("\n"); + const usageLines = ["--stdio", ...Object.values(CLI_COMMAND_SPEC).flatMap((entry) => entry.usage)].map((line) => ` loopover-mcp ${line}`).join("\n"); + // Also DERIVED (#9860): the commands that default `--login` from the environment. This was a hand-typed + // prose list and had already drifted -- it omitted contributor-profile, explain-review-risk and watch, all + // of which take `--login` and resolve it through the same `resolveLogin` fallback. Every command declaring + // `--login` in its usage resolves it that way, so the usage table is the fact to read rather than a second + // list to keep in step with it. + const loginDefaulting = Object.entries(CLI_COMMAND_SPEC) + .filter(([, entry]) => entry.usage.some((usage) => usage.includes("--login"))) + .map(([name]) => name) + .join(", "); process.stdout.write(`Usage: ${usageLines} @@ -4354,7 +4361,7 @@ ${usageLines} LOOPOVER_PROFILE LOOPOVER_CONFIG_PATH or LOOPOVER_CONFIG_DIR LOOPOVER_API_TOKEN, LOOPOVER_MCP_TOKEN, LOOPOVER_TOKEN, or a session from loopover-mcp login - LOOPOVER_LOGIN or GITHUB_LOGIN (default --login for analyze-branch, preflight, review-pr, decision-pack, repo-decision, monitor-open-prs, pr-outcomes, notifications, notifications-read, and agent plan/packet) + LOOPOVER_LOGIN or GITHUB_LOGIN (default --login for ${loginDefaulting}) GITHUB_TOKEN for non-interactive login bootstrap GITTENSOR_SCORE_PREVIEW_CMD GITTENSOR_ROOT diff --git a/test/unit/mcp-cli-help.test.ts b/test/unit/mcp-cli-help.test.ts index b7c7058aa6..de5c920397 100644 --- a/test/unit/mcp-cli-help.test.ts +++ b/test/unit/mcp-cli-help.test.ts @@ -23,3 +23,34 @@ describe("loopover-mcp --help lists every real top-level command (#6991)", () => expect(output).toMatch(/loopover-mcp contributor-profile/); }); }); + +// #9860: the help banner's LOOPOVER_LOGIN line used to be a hand-typed prose list of commands, and it had +// already drifted -- omitting contributor-profile, explain-review-risk and watch, all of which take `--login` +// and resolve it through the same fallback. It is derived from CLI_COMMAND_SPEC now; these pin that the +// derivation stays truthful rather than merely stable. +describe("the LOOPOVER_LOGIN command list is derived, not remembered (#9860)", () => { + const loginLine = () => run(["--help"]).split("\n").find((line) => line.includes("LOOPOVER_LOGIN")) ?? ""; + + it("names the commands the old hand-written list had dropped", () => { + const line = loginLine(); + for (const command of ["contributor-profile", "explain-review-risk", "watch"]) expect(line).toContain(command); + }); + + it("names EVERY command whose usage declares --login, and no others", async () => { + // Read from the same table printHelp derives from, so a command added there shows up here by + // construction. Comparing against a literal list would just move the hand-maintained list into a test. + const { CLI_COMMAND_SPEC } = await import("../../packages/loopover-mcp/bin/loopover-mcp"); + const expected = Object.entries(CLI_COMMAND_SPEC) + .filter(([, entry]) => entry.usage.some((usage: string) => usage.includes("--login"))) + .map(([name]) => name); + expect(expected.length).toBeGreaterThan(5); + + const line = loginLine(); + for (const command of expected) expect(line).toContain(command); + // And nothing that does NOT take --login is claimed. `login` itself is the trap: it is a real command + // whose name is a substring of the flag, so a naive check would pass while the line was wrong. + const notLoginDefaulting = Object.keys(CLI_COMMAND_SPEC).filter((name) => !expected.includes(name)); + const named = line.slice(line.indexOf("default --login for")).split(/[,()]/).map((part) => part.trim()); + for (const command of notLoginDefaulting) expect(named).not.toContain(command); + }); +});