types(mcp): finish the option-plumbing sweep, and cover both arms of every fallback (#9824) - #9826
Conversation
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-29 12:51:45 UTC
Review summary Nits — 5 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed 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.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9826 +/- ##
==========================================
+ Coverage 91.59% 91.64% +0.04%
==========================================
Files 915 915
Lines 112700 112710 +10
Branches 27077 27067 -10
==========================================
+ Hits 103229 103288 +59
+ Misses 8144 8129 -15
+ Partials 1327 1293 -34
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…every fallback (#9824) The review found `explainReviewRiskCli` reading `options.login` raw. That one was the visible end of a family -- seven more sites read an option without narrowing it, each reachable by an ordinary typo: - `analyze-branch --login` and `review-pr --login` sent boolean `true` as the contributor login. - `login --github-token` with no value took the token branch with `true` as the token, instead of falling through to the device flow. - `Number(options.limit)` on a bare `--limit` is 1, so the flag silently capped an audit at one row rather than leaving the server's default in place. Same for `--window-days`. - `String(options.since)` sent `?since=true`, which the route reads as a cursor. - `validate-config --file` passed a `!options.file` guard (a bare flag is `true`, which is truthy) and then read the manifest from the path "". Rather than add an eighth copy of each chain, the duplicated ones are now single helpers -- `resolveLogin` (flag, profile session, then two env vars, at 14 call sites), `readOptionalTextFile` and `issueNumbersOption` -- so there is one place to get it right and one place to test. `optionalInteger` and `optionalNumber` lose their `any` parameters, which is this issue's subject. `branchEligibilityFromOptions` restated the route's two enums by hand and dropped anything it did not recognise. It parses against `branchEligibilityFields` now, exported from the contract for the purpose: the schema was a pipe, so its members were unreachable through it. Coverage: the diff is at 100% of lines AND branches in the CLI. Every fallback is exercised from both sides in `test/unit/mcp-cli-option-resolution.test.ts`, in-process -- the subprocess harness the rest of the CLI suite uses runs the compiled `dist/`, which v8 cannot instrument, so a path covered only there reads as uncovered and a regression in one of these arms would land unnoticed.
4ed62fa to
31bdcbb
Compare
1 participant
Closes #9824. Follow-up to #9785 — the review of that PR caught one site the sweep missed, and it turned out to be the visible end of a family.
The seven that were left
analyze-branch --logintruesent to/v1/local/branch-analysisas the contributor loginreview-pr --loginlogin --github-tokentrueis truthy, so the token branch ran withtrueas the token instead of falling through to the device flowmaintain selftune-audit --limitNumber(true)is 1 — the audit silently capped at one row instead of the server defaultmaintain precision --window-daysmaintain audit-feed --sinceString(true)sent?since=true, which the route reads as a cursorvalidate-config --file!options.filepassed (a bare flag istrue, which is truthy) and the manifest was read from the path""Fewer copies, not more narrowing calls
Rather than add an eighth copy of each chain, the duplicated ones became single helpers:
resolveLogin— flag → profile session →LOOPOVER_LOGIN→GITHUB_LOGIN, previously written out at 14 call sites. That duplication is precisely how two of them ended up different from the other twelve.readOptionalTextFile— replaces three copies ofreadCliTextFile(optionText(x) ?? "", …), which read the path""for a valueless flag.issueNumbersOption— replaces three copies of the array/scalar/absent dance, two of which still carried(value: any)annotations.optionalIntegerandoptionalNumberlose theiranyparameters — the annotation this issue exists to remove — and a namedCliOptionValuestates the union once instead of four times.An enum the CLI was restating
branchEligibilityFromOptionshand-copied the route's two vocabularies and silently dropped anything it did not recognise, so a status the route accepts could be discarded by the CLI with no error. It parses against the route's own schema now.branchEligibilitySchemais a zod pipe (it transforms), so its members were unreachable through it — the contract exports the pre-transform objectbranchEligibilityFieldsfor the purpose, which is also the honest fix for anyone else who needs those fields.Coverage: 100% of the diff, lines and branches
Measured on the changed file:
Every fallback is exercised from both sides in
test/unit/mcp-cli-option-resolution.test.ts(24 cases), driven in-process. That is not a style preference: the rest of the CLI suite spawns the compileddist/through a subprocess harness, which v8 cannot instrument, so a path covered only there reads as uncovered — which is how a regression in one of these arms would land unnoticed, and whycodecov/patchon #9785 saw branch gaps that the passing subprocess tests did nothing to close.One dead branch is removed rather than covered:
ensureProfile'soptions: CliOptions = {}default had a single call site that always passes options.Validation
npx vitest run --changed=origin/main: 2207 passed, 154 files.tsc --noEmitclean.