Skip to content

fix(miner): honor --json on governor pause/resume/status error paths (#5914) - #6034

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
cleanjunc:fix/governor-pause-cli-json-contract
Jul 15, 2026
Merged

fix(miner): honor --json on governor pause/resume/status error paths (#5914)#6034
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
cleanjunc:fix/governor-pause-cli-json-contract

Conversation

@cleanjunc

Copy link
Copy Markdown
Contributor

Summary

  • governor-pause-cli.js was the last CLI module in packages/loopover-miner/lib/ that never adopted the shared cli-error.js failure contract: runGovernorPause, runGovernorResume, and runGovernorStatus each handled their parse-error and catch-error paths with a raw console.error(...); return 2;. So loopover-miner governor pause|resume|status --json printed plain text to stderr on any error path instead of the documented {ok:false,error} envelope on stdout, breaking scripted/MCP consumers that always pass --json and expect JSON regardless of outcome.
  • Routes all 6 of those error paths through reportCliFailure(...), matching the reference implementation in claim-ledger-cli.js:222,253 and the same shape already used by plan-store-cli.js, run-state-cli.js, event-ledger-cli.js, portfolio-queue-cli.js, and 10 other sibling modules.
  • Parse-error paths pass argsWantJson(args) (raw argv) and catch paths pass parsed.json, exactly as the siblings do. This distinction is load-bearing, not an inconsistency: a parse failure returns {error} with no .json field, and a token can abort the parse before --json is ever read — loopover-miner governor resume extra --json is a real case, and it has a dedicated test.
  • Behavior on non---json invocations is unchanged: plain text to stderr, exit code 2. reportCliFailure's default exitCode is already 2, so every path returns the same code it did before.

Closes #5914

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • Unchecked boxes are checks I did not run locally, listed honestly rather than assumed. The change is confined to one miner lib/*.js module and its unit test; the unrun checks cover surfaces this diff does not touch — no workflow files (actionlint), no worker/MCP/OpenAPI/UI files (test:workers, build:mcp, test:mcp-pack, ui:*), no migrations, schema, wrangler.jsonc bindings, or env.* reads (so no cf-typegen / env-reference regeneration is owed). CI runs all of them regardless.
  • test:coverage: I ran the changed file's coverage scoped rather than the full unsharded suite. Result on packages/loopover-miner/lib/governor-pause-cli.js: 100% statements (93/93), 100% branches (53/53), 100% lines — the whole file, so every changed line and branch is covered by construction. The 4 changed statements themselves introduce no new conditionals.
  • typecheck: tsc --noEmit exits 0 on the full workspace.
  • The regression tests fail on the unfixed code — verified, not assumed. Reverting only governor-pause-cli.js to main and re-running the new #5914 block fails 6 of 7 tests with SyntaxError: "undefined" is not valid JSON (nothing reaches stdout, because the old code wrote to stderr). The 7th (keeps non-JSON error paths on stderr as plain text) passes both before and after by design — it is the behavior-preservation test pinning the unchanged non---json contract.
  • Blast radius checked: runGovernorPause/runGovernorResume/runGovernorStatus have exactly one non-test consumer — governor-ledger-cli.js:150-152, which dispatches governor pause|resume|status. Its suite (test/unit/miner-governor-ledger-cli.test.ts) passes unchanged. apps/loopover-miner-ui/vite-governor-api.ts only references the module in a comment and reads governor-state directly; nothing shells out to this CLI to parse its output. node --check passes (what build:miner does).

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

The four unchecked Safety boxes are not applicable to this diff, so they are left unchecked rather than checked as a formality: it touches no auth/cookie/CORS/App/Cloudflare/session surface, no API/OpenAPI/MCP behavior, and no UI — so there is no UI Evidence to attach. The --json error envelope is an existing documented contract this module was not honoring; no public doc changes, and no changelog edit (not a release-prep PR).

Notes

  • The error-path messages are unchanged — only their destination and framing on --json runs. describeCliError reproduces the previous error instanceof Error ? error.message : String(error) normalization exactly, so the non-Error throw path keeps stringifying identically (still covered by the pre-existing tests at miner-governor-pause-cli.test.ts).
  • I deliberately did not extract the repeated if ("error" in parsed) return reportCliFailure(argsWantJson(args), parsed.error); into a local helper: all 15 sibling CLI modules inline this exact pattern, and matching the established convention keeps governor-pause-cli.js diffable against its siblings. For the same reason I did not add an inline comment explaining argsWantJson(args) vs parsed.json — that rationale is documented once at the definition site (cli-error.js:19) and no sibling repeats it; this PR's Summary records the reasoning instead.

@cleanjunc
cleanjunc requested a review from JSONbored as a code owner July 15, 2026 07:11
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.16%. Comparing base (0c8c4c4) to head (5e241c6).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6034       +/-   ##
===========================================
+ Coverage   78.84%   95.16%   +16.31%     
===========================================
  Files         592      592               
  Lines       46949    46945        -4     
  Branches    15003    15003               
===========================================
+ Hits        37019    44675     +7656     
+ Misses       7754     1512     -6242     
+ Partials     2176      758     -1418     
Flag Coverage Δ
shard-1 43.93% <0.00%> (?)
shard-2 35.59% <0.00%> (-0.94%) ⬇️
shard-3 32.74% <0.00%> (?)
shard-4 31.96% <0.00%> (+0.01%) ⬆️
shard-5 32.26% <0.00%> (-0.27%) ⬇️
shard-6 44.93% <100.00%> (+0.31%) ⬆️

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

Files with missing lines Coverage Δ
packages/loopover-miner/lib/governor-pause-cli.js 100.00% <100.00%> (+1.07%) ⬆️

... and 209 files with indirect coverage changes

@loopover-orb loopover-orb Bot added gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. labels Jul 15, 2026
@loopover-orb

loopover-orb Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-15 07:23:08 UTC

2 files · 1 AI reviewer · no blockers · readiness 80/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This mechanically ports governor-pause-cli.js's 6 error paths (2 per subcommand across pause/resume/status) to the shared reportCliFailure/cli-error.js contract, matching the established pattern already used in claim-ledger-cli.js and loop-cli.js. The parse-error vs catch-error json-source distinction (argsWantJson(args) vs parsed.json) is correctly applied and matches the reference implementation, with a dedicated test covering the case where a stray positional arg aborts parsing before --json is read. Tests cover all 6 paths plus the non-JSON stderr-preserving behavior, and the diff is narrow and fully consistent with its stated intent.

Nits — 4 non-blocking
  • The new test file duplicates a lot of near-identical assertion boilerplate across the six JSON-envelope tests (test/unit/miner-governor-pause-cli.test.ts) — a small helper (e.g. `expectJsonError(fn, args, options, message)`) would cut the repetition.
  • The comment above the runGovernorResume parse-error test is helpful but slightly redundant with the PR description; consider trimming it to just the load-bearing fact (extra arg aborts parse before --json is read).
  • Consider adding a similarly-worded top-of-file comment note in governor-pause-cli.js itself (like loop-cli.js's header) so future readers of that file alone see the argsWantJson vs parsed.json rationale without needing the PR description.
  • If there are other CLI modules still on the raw console.error pattern, a follow-up sweep could reference this PR as the template.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #5914
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High 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: 49 registered-repo PR(s), 18 merged, 30 issue(s).
Contributor context ✅ Confirmed Gittensor contributor cleanjunc; Gittensor profile; 49 PR(s), 30 issue(s).
Gate result ✅ Passing No configured blocker found.
Improvement ✅ Minor risk: clean · value: minor · LLM: minor
Linked issue satisfaction

Addressed
The diff imports argsWantJson/reportCliFailure/describeCliError and replaces all 6 raw console.error/return 2 paths in runGovernorPause, runGovernorResume, and runGovernorStatus with the shared contract, matching the sibling CLI convention. New tests directly cover both --json and non-json arms of each error path, including the parse-error-before-json-parsed edge case called out in the issue.

Review context
Contributor next steps
  • Start here: Review top overlaps.
  • Then work through the remaining 2 steps in the Signals table above.
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://gittensory.aethereal.dev/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 5db3b5e into JSONbored:main Jul 15, 2026
16 checks passed
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. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AMS: governor pause/resume/status CLI error paths bypass the --json contract

1 participant