chore(guards): #9860 items 1, 4 and 5 — derive one list, pin one registry, write down the coverage rule

.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.
 
 ---
 

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).

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

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.<key>` / `record["<key>"]` read inside parseFocusManifest's body. */
+function topLevelKeysReadByParser(source: string): Set<string> {
+  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<string>();
+  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<string>(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);
+  });
+});

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