Skip to content

fix(mcp): parse --json=false and --exit-code=false as real booleans#8725

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-mcp-bool-flag-parsing-8689
Jul 26, 2026
Merged

fix(mcp): parse --json=false and --exit-code=false as real booleans#8725
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-mcp-bool-flag-parsing-8689

Conversation

@rsnetworkinginc

Copy link
Copy Markdown
Contributor

Closes #8689

Problem

parseOptions' generic --key=value inline-equals handler (added for --format=table, #2231) stores the raw string for every flag. So --json=false set options.json = "false" — a non-empty, truthy string — and every options.json consumer (23 call sites) plus cli-error.ts's argsWantJson checks truthiness, so the flag turned JSON output ON, the opposite of what it says. The identical bug hit doctor --exit-code=false, which still escalated the exit code to 1.

Fix

The Requirements allow "the same === true (or equivalent explicit boolean-parsing) pattern" — this PR implements the explicit boolean parse at the single choke point every consumer flows through, so all 23 options.json consumers and doctor's options.exitCode check compare against a guaranteed real boolean (never a raw string), exactly the semantics of the --refresh/--help === true convention:

  • parseOptions (packages/loopover-mcp/bin/loopover-mcp.ts): the inline --key=value values for the boolean flags json/exitCode now parse to a real boolean — only the exact string "false" (and empty "", which was already falsy) disables; any other value keeps today's enabling behavior. Bare --json and --json=true are unchanged; only the =false form changes behavior (now correctly disabling).
  • doctor: the exit-code check is now the explicit options.exitCode === true && … comparison (the --refresh line-4033 convention), so --exit-code=false keeps the exit code at 0 even when checks need attention.
  • argsWantJson (packages/loopover-mcp/lib/cli-error.ts): --json=false no longer counts as wanting JSON on the failure path.

Tests (in-process runCli, the mcp-cli-basics.test.ts #8587 pattern)

New test/unit/mcp-cli-bool-flag-parsing.test.ts + an argsWantJson case in test/unit/mcp-cli-error.test.ts, all red on main before the fix:

Deliverable Test Before (main) After
whoami --json=false → plain text asserts output is exactly JSONbored\n, not JSON failed: emitted {"status":"authenticated","l… passes
doctor --exit-code=false vs failing auth check → exit 0 asserts in-process return 0 with status: "needs_attention" failed: expected 1 to be +0 passes
bare --json / --json=true still enable JSON parses whoami JSON payload for both forms passed passes (regression guard)
bare --exit-code / --exit-code=true still escalate asserts return 1 vs failing check passed passes (regression guard)
argsWantJson("--json=false") → false direct unit test failed: expected true to be false passes
non-boolean inline values stay raw strings init-client --print=codex renders the codex snippet passed passes

Full runs: mcp-cli-bool-flag-parsing, mcp-cli-error, mcp-cli-basics, mcp-cli-doctor — 51/51 passing; tsc --noEmit clean.

Coverage

Every changed executable line in bin/loopover-mcp.ts (the booleanFlags set, the inline boolean parse, doctor's return) and lib/cli-error.ts is exercised in-process with both branch directions taken (verified against the lcov report: all BRDA arms on the changed lines are non-zero), covering the =false and default/=true branches for both flags as required.

…hey disable

parseOptions generic inline-equals handler stored the raw string for every flag, so
--json=false became the truthy string false and turned JSON output ON across all
options.json consumers (and --exit-code=false still escalated doctor exit code).
Parse the json/exit-code inline values to real booleans at the single parse
choke point — the explicit boolean-comparison convention already used for
--refresh/--help — make doctor exit-code check === true, and teach
argsWantJson that --json=false does not want JSON. Bare --json and --json=true
are unchanged; only the =false form changes behavior (now correctly disabling).
In-process runCli tests cover whoami --json=false/--json/--json=true and doctor
--exit-code=false/bare/=true against a failing check.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.82%. Comparing base (abf88ab) to head (c671397).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8725      +/-   ##
==========================================
- Coverage   90.56%   83.82%   -6.74%     
==========================================
  Files          96       98       +2     
  Lines       22490    24757    +2267     
  Branches     3884     4744     +860     
==========================================
+ Hits        20367    20752     +385     
- Misses       1945     3727    +1782     
- Partials      178      278     +100     
Flag Coverage Δ
backend 16.98% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-mcp/bin/loopover-mcp.ts 16.72% <100.00%> (ø)
packages/loopover-mcp/lib/cli-error.ts 100.00% <100.00%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-26 01:15:26 UTC

4 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR fixes a real bug: the generic inline `--key=value` handler in `parseOptions` stored raw strings, so `--json=false` and `--exit-code=false` became truthy strings and silently enabled behavior opposite of what was requested. The fix scopes an explicit boolean parse to the `json`/`exitCode` flags at the single choke point, updates `doctor`'s exit-code check to an explicit `=== true` comparison consistent with the existing `--refresh` convention, and patches `argsWantJson` to stop treating `--json=false` as wanting JSON. The diff is narrow, well-targeted, and accompanied by tests that exercise the real CLI dispatcher in-process (not fabricated payloads), covering both the previously-broken `=false` cases and the unchanged bare/`=true` cases.

Nits — 5 non-blocking
  • packages/loopover-mcp/bin/loopover-mcp.ts: the new `booleanFlags` Set uses camelCase keys ("json", "exitCode") consistent with `camel(inlineKey)` above it, but double-check that `--exit-code=false` actually maps through `camel()` to `"exitCode"` before reaching the `booleanFlags.has` check — worth a one-line comment confirming this since it's easy to typo the camelCase key and silently miss the flag.
  • test/unit/mcp-cli-bool-flag-parsing.test.ts: this new file duplicates a fair amount of harness boilerplate (`withEnv`, `captureStdout`, fixture server setup) that likely already exists in `mcp-cli-basics.test.ts`/`mcp-cli-doctor.test.ts` per the PR description — consider extracting shared helpers if this pattern repeats a third time.
  • The external brief's 'long file' flag on `loopover-mcp.ts` (5955 lines) is pre-existing file size, not something this diff introduces — not actionable for this PR.
  • packages/loopover-mcp/bin/loopover-mcp.ts:5495: consider a short inline comment or test asserting `camel("exit-code") === "exitCode"` matches the `booleanFlags` Set entry, since a future rename of either could silently break the disable path with no type error to catch it.
  • The fix correctly scopes to `json`/`exitCode` only, per the `--print=codex` passthrough test in mcp-cli-bool-flag-parsing.test.ts:127-131 — this guards against over-broad boolean coercion of unrelated inline flags, good call.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8689
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 55 registered-repo PR(s), 22 merged, 3 issue(s).
Contributor context ✅ Confirmed Gittensor contributor rsnetworkinginc; Gittensor profile; 55 PR(s), 3 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff fixes the inline `--key=value` parser to coerce json/exitCode to real booleans (only "false"/empty disable), updates doctor's exit-code check to `=== true`, and fixes argsWantJson, matching the requested convention; it also adds tests covering --json=false, --json/--json=true, --exit-code=false, and --exit-code/--exit-code=true.

Review context
  • Author: rsnetworkinginc
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript
  • Official Gittensor activity: 55 PR(s), 3 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

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

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

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

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 60f9f8f into JSONbored:main Jul 26, 2026
8 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 26, 2026
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(mcp): --json=false and --exit-code=false are parsed as truthy, enabling the flag instead of disabling it

1 participant