Skip to content

Commit 9783944

Browse files
authored
chore(guards): #9860 items 1, 4 and 5 — derive one list, pin one registry, write down the coverage rule (#9949)
* 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). * 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.
1 parent afa08af commit 9783944

5 files changed

Lines changed: 122 additions & 5 deletions

File tree

.claude/skills/contributing-to-loopover/reference.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,23 @@ checks go green) is the only way to know you didn't break it.
150150
patch coverage; a backend `src/**` change owes coverage on **every changed line + branch**.
151151
- **Measure unsharded locally:** `npm run test:coverage`. CI shards into 3 and Codecov merges them,
152152
so a single local shard under-reports — never trust it.
153+
- **`npm run typecheck` does not cover `apps/**`.** `ui:typecheck` is a separate step, present in
154+
`test:ci` but NOT in the root `typecheck` script — so a type change you validated locally with
155+
`npm run typecheck` can still break the UI build in CI (#9815 shipped exactly this). Run
156+
`npm run ui:typecheck` too, or just run the whole `npm run test:ci`.
153157
- **Flaky tests are already tracked.** Every shard uploads a JUnit report (`report_type: test_results`),
154158
which auto-enables Codecov Test Analytics with no extra config — check a PR's "Tests" tab or its
155159
Codecov bot comment if a test needed a retry, rather than assuming it's pure infra noise.
160+
- **`packages/loopover-engine/src/**` is credited by TWO uploads, and this is the biggest gotcha in the
161+
repo (#9860 item 5).** The unflagged `backend` report (root vitest, v8) and the `engine` flag (the
162+
package's own `node:test` suite, c8 `--all`) both cover those lines, and Codecov *unions* their hits.
163+
The two runs disagree about which lines even exist — c8 `--all` instruments every file including ones
164+
no test imports, v8 does not — so **an engine change that is genuinely 100% tested by root-suite tests
165+
alone can still land as ~65% on `codecov/patch`** (seen on #9821).
166+
**If you change `packages/loopover-engine/src/**`, add or extend a test in
167+
`packages/loopover-engine/test/**` (the `node:test` suite), not only in root `test/**`.** Run it with
168+
`npm run test --workspace @loopover/engine`. A root-suite test alone is not a reliable way to clear the
169+
patch gate on engine source, no matter what your local `npm run test:coverage` says.
156170

157171
---
158172

AGENTS.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@ process evolves — edits to those files improve both Claude Code and Codex.
2020
a valid linked issue is **auto-MERGED**. So make it perfect before you push.
2121
2. **99% patch coverage, branch-counted.** Aim for **100% of every changed line *and branch*** — test
2222
both sides of every `??` / ternary / `&&` (a `SUM()` can return `NULL`, so the nullish arm is real),
23-
plus invariant tests and a regression test for every fix. Only `src/**` is measured by Codecov.
23+
plus invariant tests and a regression test for every fix. Codecov measures `src/**` **and**
24+
`packages/loopover-engine/src/**` — and the engine's lines are credited by two uploads whose hits are
25+
unioned, so an engine change tested only from root `test/**` can still fail the patch gate. Add the test
26+
to `packages/loopover-engine/test/**` too; see the Codecov section of `reference.md`.
2427
3. **The whole local gate must be green:** `npm run test:ci` (+ `npm audit --audit-level=moderate`).
2528
Measure coverage **unsharded** with `npm run test:coverage` (CI shards + merges, so a single shard
2629
under-reports).

packages/loopover-mcp/bin/loopover-mcp.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4343,9 +4343,16 @@ function printHelp() {
43434343
// construction, which the old hand-written string provably did not guarantee -- it had already
43444344
// dropped commands. `--stdio` is the one non-command entry, listed first because it is the mode
43454345
// MCP clients launch.
4346-
const usageLines = ["--stdio", ...Object.values(CLI_COMMAND_SPEC).flatMap((entry) => entry.usage)]
4347-
.map((line) => ` loopover-mcp ${line}`.replace("loopover-mcp --stdio", "loopover-mcp --stdio"))
4348-
.join("\n");
4346+
const usageLines = ["--stdio", ...Object.values(CLI_COMMAND_SPEC).flatMap((entry) => entry.usage)].map((line) => ` loopover-mcp ${line}`).join("\n");
4347+
// Also DERIVED (#9860): the commands that default `--login` from the environment. This was a hand-typed
4348+
// prose list and had already drifted -- it omitted contributor-profile, explain-review-risk and watch, all
4349+
// of which take `--login` and resolve it through the same `resolveLogin` fallback. Every command declaring
4350+
// `--login` in its usage resolves it that way, so the usage table is the fact to read rather than a second
4351+
// list to keep in step with it.
4352+
const loginDefaulting = Object.entries(CLI_COMMAND_SPEC)
4353+
.filter(([, entry]) => entry.usage.some((usage) => usage.includes("--login")))
4354+
.map(([name]) => name)
4355+
.join(", ");
43494356
process.stdout.write(`Usage:
43504357
${usageLines}
43514358

@@ -4354,7 +4361,7 @@ ${usageLines}
43544361
LOOPOVER_PROFILE
43554362
LOOPOVER_CONFIG_PATH or LOOPOVER_CONFIG_DIR
43564363
LOOPOVER_API_TOKEN, LOOPOVER_MCP_TOKEN, LOOPOVER_TOKEN, or a session from loopover-mcp login
4357-
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)
4364+
LOOPOVER_LOGIN or GITHUB_LOGIN (default --login for ${loginDefaulting})
43584365
GITHUB_TOKEN for non-interactive login bootstrap
43594366
GITTENSOR_SCORE_PREVIEW_CMD
43604367
GITTENSOR_ROOT
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
import { readFileSync } from "node:fs";
2+
import { describe, expect, it } from "vitest";
3+
4+
import { FOCUS_MANIFEST_TOP_LEVEL_FIELDS } from "../../packages/loopover-engine/src/focus-manifest";
5+
6+
// #9860 (item 4): FOCUS_MANIFEST_TOP_LEVEL_FIELDS is a hand-kept list of 28 keys whose doc comment claims it
7+
// is "every top-level key `parseFocusManifest` below actually reads". Nothing checked that claim, and it is
8+
// load-bearing in both directions:
9+
//
10+
// • A key the parser reads but the list omits is silently reported to operators as an UNKNOWN FIELD --
11+
// the config-lint/validator path warns about a setting that in fact works.
12+
// • A key in the list the parser never reads is worse: the validator blesses it, the runtime ignores it,
13+
// and an operator's setting does nothing while every surface says it is fine.
14+
//
15+
// #9813 and #9821 each shipped a bug from missing one of this registry's touchpoints. This computes the
16+
// relation instead of trusting the comment.
17+
//
18+
// SOURCE-SCANNED rather than exercised through the parser, because the failure is a key the parser reads and
19+
// the list forgot -- which by construction produces no observable behaviour difference to assert on. The
20+
// scan is narrow: `record` is the single local `parseFocusManifest` binds the raw object to, and the function
21+
// body is delimited by the two exported functions around it.
22+
const SOURCE = "packages/loopover-engine/src/focus-manifest.ts";
23+
24+
/** Every `record.<key>` / `record["<key>"]` read inside parseFocusManifest's body. */
25+
function topLevelKeysReadByParser(source: string): Set<string> {
26+
const start = source.indexOf("export function parseFocusManifest(raw: unknown");
27+
const end = source.indexOf("export function parseFocusManifestContent");
28+
expect(start).toBeGreaterThan(-1);
29+
expect(end).toBeGreaterThan(start);
30+
const body = source.slice(start, end);
31+
const keys = new Set<string>();
32+
for (const match of body.matchAll(/\brecord\.([A-Za-z_][A-Za-z0-9_]*)/g)) keys.add(match[1]!);
33+
for (const match of body.matchAll(/\brecord\["([^"]+)"\]/g)) keys.add(match[1]!);
34+
return keys;
35+
}
36+
37+
describe("FOCUS_MANIFEST_TOP_LEVEL_FIELDS is the set the parser actually reads (#9860)", () => {
38+
const source = readFileSync(SOURCE, "utf8");
39+
40+
it("declares every key parseFocusManifest reads", () => {
41+
// Missing here => the validator calls a working setting an unknown field.
42+
const read = topLevelKeysReadByParser(source);
43+
const declared = new Set<string>(FOCUS_MANIFEST_TOP_LEVEL_FIELDS);
44+
expect([...read].filter((key) => !declared.has(key)).sort()).toEqual([]);
45+
});
46+
47+
it("declares nothing the parser ignores", () => {
48+
// Present here but unread => the validator blesses a setting that silently does nothing at runtime,
49+
// which is the more dangerous direction: every surface reports success.
50+
const read = topLevelKeysReadByParser(source);
51+
expect(FOCUS_MANIFEST_TOP_LEVEL_FIELDS.filter((key) => !read.has(key)).sort()).toEqual([]);
52+
});
53+
54+
it("guards against a vacuous scan: the parser really does read a known key", () => {
55+
// Without this, a refactor that renamed `record` would empty both sets and make the two assertions above
56+
// pass while checking nothing at all.
57+
const read = topLevelKeysReadByParser(source);
58+
expect(read.size).toBeGreaterThan(20);
59+
expect(read.has("wantedPaths")).toBe(true);
60+
expect(read.has("gate")).toBe(true);
61+
});
62+
});

test/unit/mcp-cli-help.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,34 @@ describe("loopover-mcp --help lists every real top-level command (#6991)", () =>
2323
expect(output).toMatch(/loopover-mcp contributor-profile/);
2424
});
2525
});
26+
27+
// #9860: the help banner's LOOPOVER_LOGIN line used to be a hand-typed prose list of commands, and it had
28+
// already drifted -- omitting contributor-profile, explain-review-risk and watch, all of which take `--login`
29+
// and resolve it through the same fallback. It is derived from CLI_COMMAND_SPEC now; these pin that the
30+
// derivation stays truthful rather than merely stable.
31+
describe("the LOOPOVER_LOGIN command list is derived, not remembered (#9860)", () => {
32+
const loginLine = () => run(["--help"]).split("\n").find((line) => line.includes("LOOPOVER_LOGIN")) ?? "";
33+
34+
it("names the commands the old hand-written list had dropped", () => {
35+
const line = loginLine();
36+
for (const command of ["contributor-profile", "explain-review-risk", "watch"]) expect(line).toContain(command);
37+
});
38+
39+
it("names EVERY command whose usage declares --login, and no others", async () => {
40+
// Read from the same table printHelp derives from, so a command added there shows up here by
41+
// construction. Comparing against a literal list would just move the hand-maintained list into a test.
42+
const { CLI_COMMAND_SPEC } = await import("../../packages/loopover-mcp/bin/loopover-mcp");
43+
const expected = Object.entries(CLI_COMMAND_SPEC)
44+
.filter(([, entry]) => entry.usage.some((usage: string) => usage.includes("--login")))
45+
.map(([name]) => name);
46+
expect(expected.length).toBeGreaterThan(5);
47+
48+
const line = loginLine();
49+
for (const command of expected) expect(line).toContain(command);
50+
// And nothing that does NOT take --login is claimed. `login` itself is the trap: it is a real command
51+
// whose name is a substring of the flag, so a naive check would pass while the line was wrong.
52+
const notLoginDefaulting = Object.keys(CLI_COMMAND_SPEC).filter((name) => !expected.includes(name));
53+
const named = line.slice(line.indexOf("default --login for")).split(/[,()]/).map((part) => part.trim());
54+
for (const command of notLoginDefaulting) expect(named).not.toContain(command);
55+
});
56+
});

0 commit comments

Comments
 (0)