Skip to content

fix(miner): replace never-registered run with loop in fleet-mode manifests - #9524

Merged
JSONbored merged 1 commit into
mainfrom
claude/laughing-euler-8d5f5f
Jul 28, 2026
Merged

fix(miner): replace never-registered run with loop in fleet-mode manifests#9524
JSONbored merged 1 commit into
mainfrom
claude/laughing-euler-8d5f5f

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • docker-compose.miner.yml and k8s/miner-deployment.yaml both invoke the miner CLI with run — a subcommand that has never been registered in packages/loopover-miner/bin/loopover-miner.ts's dispatch table, confirmed all the way back to each manifest's first commit (feat(miner-deployment): add docker-compose.miner.yml for AMS fleet mode #5299, feat(miner-deployment): add Kubernetes StatefulSet + Secret example for AMS fleet-mode #5258) via git log -S. A fleet worker built from either manifest sets up the egress firewall, then immediately exits 1 with Unknown command: run instead of ever mining. loop is the real continuous fleet-worker daemon (see loop-cli.ts, DEPLOYMENT.md, systemd/loopover-miner.service.example).
  • Fixes both manifests to invoke loop with placeholder <owner/repo>/<login> args (the daemon requires a real target and --miner-login, with no env-var fallback), updates their comments, and fixes the two test files that pinned the broken run value (test/unit/miner-docker-compose.test.ts, test/unit/miner-k8s-manifests.test.ts), adding a regression assertion to each.
  • Extends the existing DEPLOYMENT.md docs-accuracy audit (scripts/check-miner-deployment-docs.ts, packages/loopover-miner/lib/deployment-docs-audit.ts) to also cross-check these two container manifests' command:/args: against the CLI's registered-command table — the same drift class the audit already catches for markdown prose, now covering deploy YAML too, so this exact bug fails CI going forward instead of only being caught by inspection.

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 — not applicable: this is a maintainer-authored fix for a bug found via direct code/history inspection (confirmed run was never registered, in any commit, via git log -S), not a contributor PR against the issue queue.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — not run in full (unsharded whole-repo run); instead ran targeted vitest run + --coverage on the four affected test files (miner-docker-compose.test.ts, miner-k8s-manifests.test.ts, miner-deployment-docs-audit.test.ts, check-miner-deployment-docs.test.ts, 55/55 passing) and confirmed the one Codecov-measured touched file, packages/loopover-miner/lib/deployment-docs-audit.ts, hits 100% lines/branches/functions via the resulting coverage/lcov.info. scripts/** (the other touched source file) is explicitly excluded from Codecov's patch gate (codecov.yml's ignore: list, mirroring vitest.config.ts's coverage.include).
  • npm run test:workers — not run; no Cloudflare Worker code touched.
  • npm run build:mcp — not run; no MCP package touched.
  • npm run test:mcp-pack — not run; no MCP package touched.
  • npm run ui:openapi:check — not run; no API/OpenAPI surface touched.
  • npm run ui:lint / ui:typecheck / ui:build — not run; no UI code touched.
  • npm audit --audit-level=moderate — ran; reports 11 pre-existing high-severity findings (sharp/libvips, minimatch chain) unrelated to this diff — this PR makes no package.json/package-lock.json changes (verified via git diff main -- package.json package-lock.json, empty).
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — full branch coverage on the new audit functions (extractContainerCommandClaim, auditContainerCommands, assertContainerCommandsInSync), plus a run-must-not-appear regression test in each manifest's own test file and a live-tree integration test proving the new container-manifest audit path is genuinely wired (asserts claimCounts.containerCommands === 2 against real files, not a stub).

Also directly ran the actual CI entrypoint this PR changes: npm run test:miner-deployment-docs-auditMiner deployment docs audit ok: 30 env vars, 16 paths, 5 subcommands, 2 container commands.

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. — not applicable, no such surface touched.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — not applicable, no such surface touched.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — not applicable, no UI touched.
  • Visible UI changes include a UI Evidence section — not applicable, no visible UI change (deploy manifests + a Node CI script).
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. — the manifests' own inline comments are the operator-facing docs here and are updated; DEPLOYMENT.md itself already documented loop correctly (only the two manifests had drifted), so no further doc edit was needed.

Notes

  • The placeholder args (<owner/repo>, <login>) in both manifests are deliberate: loop requires a real repository target (or --search <query>) plus --miner-login <login>, and there is no environment-variable fallback for either in loop-cli.ts's argument parser, so an un-edited manifest fails loudly on a clearly-named placeholder rather than silently misbehaving.

@loopover-orb

loopover-orb Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-28 06:38:45 UTC

8 files · 1 AI reviewer · 2 blockers · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This PR fixes a real bug: both fleet-mode manifests invoked a never-registered `run` subcommand, which would immediately exit 1 with "Unknown command: run" rather than mine, and replaces it with the actual `loop` daemon plus placeholder args it requires. It also extends the existing DEPLOYMENT.md drift-audit machinery to catch this same class of drift in container manifests going forward, and updates the two manifest test files plus adds regression tests. The regex-based extraction (`CONTAINER_COMMAND_PATTERN`) is narrow (anchors on the literal `command:`/`args:` list with a leading string), which is a reasonable tradeoff for these two known files but is fragile if manifest formatting changes.

Nits — 5 non-blocking
  • The CONTAINER_COMMAND_PATTERN regex (packages/loopover-miner/lib/deployment-docs-audit.ts:47-49) requires the command/args key to start the line and the array's first element to be a quoted lowercase-only token; if either manifest's YAML style changes (e.g. multi-line list, single quotes) extractContainerCommandClaim silently returns null and the manifest drops out of the audit instead of failing loudly — consider asserting `containerCommands === CONTAINER_MANIFESTS.length` somewhere live, not just in the two hardcoded unit-test counts.
  • The CI failures (validate, validate-tests) are unlabeled and this branch is 1 commit behind default — per the given BASE BRANCH STATUS this is more likely explained by that drift than by a defect in this diff, but it's worth confirming after a rebase.
  • scripts/check-miner-deployment-docs.ts:107 builds `containerReality` by spreading `reality` even in the non-test-mode branch, which is a harmless no-op but slightly obscures that the two audits normally share one `reality` object — could simplify to `reality` directly when testMode isn't the fixture.
  • Consider tightening CONTAINER_COMMAND_PATTERN or adding a live assertion that every manifest in CONTAINER_MANIFESTS actually yields a non-null claim, so a future manifest reformatting can't silently exit the audit instead of failing it.
  • If a rebase is easy, do it to see whether validate/validate-tests were failing for an unrelated reason already fixed on default.

Concerns raised — review before merging

  • No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue: This repo's maintainer focus manifest requires every PR to reference a tracked issue. — Link the relevant issue (for example Closes #123) before opening the PR.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.

2. Maintainer requires a linked issue: This repo's maintainer focus manifest requires every PR to reference a tracked issue. — Link the relevant issue (for example `Closes #123`) before opening the PR.

Decision drivers

  • ❌ Code review — 2 blockers (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 14 registered-repo PR(s), 13 merged, 328 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 14 PR(s), 328 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 14 PR(s), 328 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • 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://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: hold · clause: missing_linked_issue
  • config: 3ba95b5c8403320cfff6139c0137286b5fb436ad43cad70588d1566a31f0ba93 · pack: oss-anti-slop · ci: failed
  • record: f4ece1a347e5c098f2b023604c0241f01af5053198878834a70d69f14b3effe0 (schema v5, head 647f322)

🟩 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

@JSONbored JSONbored self-assigned this Jul 28, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.56%. Comparing base (6f3b613) to head (5bd0846).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9524   +/-   ##
=======================================
  Coverage   89.56%   89.56%           
=======================================
  Files         843      843           
  Lines      110201   110213   +12     
  Branches    26230    26233    +3     
=======================================
+ Hits        98705    98717   +12     
  Misses      10233    10233           
  Partials     1263     1263           
Flag Coverage Δ
backend 95.28% <100.00%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
...ckages/loopover-miner/lib/deployment-docs-audit.ts 100.00% <100.00%> (ø)

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 28, 2026
…manifests

docker-compose.miner.yml and k8s/miner-deployment.yaml both invoked `run`
since their first commits (#5299, #5258) -- a subcommand the miner CLI has
never dispatched. A fleet worker built from either manifest sets up the
egress firewall, then immediately exits 1 with "Unknown command: run"
instead of ever mining. `loop` is the real continuous fleet-worker daemon.

Also extends the DEPLOYMENT.md docs-accuracy audit
(scripts/check-miner-deployment-docs.ts) to cross-check these container
manifests' commands against the CLI's registered-command table, the same
way it already does for markdown prose, so this drift class fails CI going
forward instead of only being caught by inspection.
@JSONbored
JSONbored force-pushed the claude/laughing-euler-8d5f5f branch from 647f322 to 5bd0846 Compare July 28, 2026 06:23
@JSONbored
JSONbored merged commit 6bb8588 into main Jul 28, 2026
7 checks passed
@JSONbored
JSONbored deleted the claude/laughing-euler-8d5f5f branch July 28, 2026 06:38
@github-actions github-actions Bot mentioned this pull request Jul 28, 2026
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant