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/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/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); + }); +}); 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); + }); +});