chore(guards): #9860 items 1, 4 and 5 — derive one list, pin one registry, write down the coverage rule - #9949
Merged
Conversation
…verage 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).
…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.
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-30 20:29:22 UTC
Review summary Nits — 5 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9949 +/- ##
==========================================
- Coverage 91.88% 91.87% -0.01%
==========================================
Files 928 928
Lines 113671 113673 +2
Branches 27412 27412
==========================================
- Hits 104441 104439 -2
Misses 7931 7931
- Partials 1299 1303 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #9860. Three of its five items, bundled — each is small and independently reviewable.
Item 1 — a hand-typed list that had already drifted
loopover-mcp --help'sLOOPOVER_LOGINline named its commands as prose, and it was wrong: it omittedcontributor-profile,explain-review-riskandwatch, all three of which accept--loginand resolve it through the sameresolveLoginfallback. Anyone reading--helpwas told those commands would not pick up their configured login. They do.Derived from
CLI_COMMAND_SPECnow.resolveLoginis the single helper and always falls back toLOOPOVER_LOGIN ?? GITHUB_LOGIN, and its 13 call sites correspond exactly to the 13 commands declaring--loginin usage — so the usage table is the fact to read, not a second list to keep in step.Item 1's other three surfaces, and item 2, were already closed by the MCP contract epic — I verified rather than assumed.
STDIO_TOOL_DESCRIPTORSderives fromSTDIO_TOOL_NAMES,printHelp's command list fromCLI_COMMAND_SPEC, the README block is generated bymcp:tool-reference, and the stdio bin declares zero zod shapes of its own (all 119z.uses arez.inferover contract-imported schemas). This line was the one hand-maintained list left in that surface.Item 4 — the focus-manifest registry
FOCUS_MANIFEST_TOP_LEVEL_FIELDSis 28 hand-kept keys whose doc comment claims it is "every top-level keyparseFocusManifestactually reads". Nothing checked that, and it is load-bearing both ways:Now computed from the parser's own source. Currently in sync at 28/28 — this pins today's state rather than fixing a live bug, which is the point, since #9813 and #9821 each shipped from missing one of this registry's touchpoints.
Source-scanned deliberately: the failure mode is a key the parser reads and the list forgot, which by construction produces no behavioural difference to assert on. Includes an anti-vacuity check — 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*ConfigToJsonof their own because they are nested under the review config and serialized through it. A naive 1:1 check fires four false positives, and a checker that cries wolf gets muted — so I left it, rather than shipping a guard that trains people to ignore it.Item 5 — the coverage asymmetry, written down
packages/loopover-engine/src/**is credited by two uploads whose hits Codecov unions, and the two runs disagree about which lines exist (c8--allinstruments files no test imports; v8 does not). A genuinely 100%-tested engine change can therefore land at ~65% oncodecov/patch(hit on #9821, and again in this session). The rule is now in the contributor skill: change engine source ⇒ add the test topackages/loopover-engine/test/**as well.This also corrects
AGENTS.md, which stated that onlysrc/**is measured. Untrue since the engine flag, and misleading in exactly the direction that costs someone a PR.And it records the issue's own closing note:
ui:typecheckis intest:cibut not in the roottypecheck, so a locally-clean type change can still breakapps/**(#9815 shipped exactly that).Validation
test/unit+test/integration: 25,761 passed, 0 failures.check-*scripts: none failing.ui:lint/ui:typecheckclean (warnings only, all pre-existing);ui:test1,092 passing.--loginlist fails both help tests — including the negative direction, sinceloginis itself a command whose name is a substring of the flag and would pass a naive check.