ams(mcp): the AMS management tool family — governor-gated mutations, doctor, tenant health (#9523) - #9607
Conversation
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 04:47:24 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed 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.
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
loopover-ui | e4ea2a7 | Jul 29 2026, 04:19 AM |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9607 +/- ##
==========================================
+ Coverage 90.24% 90.27% +0.02%
==========================================
Files 900 904 +4
Lines 112914 113122 +208
Branches 26776 26821 +45
==========================================
+ Hits 101896 102116 +220
+ Misses 9687 9676 -11
+ Partials 1331 1330 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Bundle ReportBundle size has no change ✅ |
852105d to
0d5f79e
Compare
0d5f79e to
808d5e7
Compare
The miner MCP exposed 11 read-only tools while its entire mutating ops surface was CLI-only. This adds 10 tools: a dedicated doctor (split out of status so status stays cheap), a structured metrics snapshot, and eight mutations. Every mutation dispatches through the miner's existing governor-gated chat-action chokepoint -- the same boundary the dashboard's own actions use. The MCP layer never touches a store: it dispatches an action NAME, and the registry structurally refuses any handler not produced by governorGatedHandler(), whose brand is a private symbol a raw function cannot forge. That is what makes "an MCP caller cannot reach a write path the dashboard could not" a property of the code rather than of review discipline, and the structural test asserts it from both ends. Three claims I had to correct against the real code rather than ship as written. The migrate CLI has no dry-run, because applying a migration IS opening the store -- so the tool has no apply flag either, instead of advertising a safety mode that does not exist. The deny-hook store keys proposals by (repo, id), so the decide tool takes the repo rather than scanning every repo's proposals to resolve an id. And purgeRepoAcrossStores is extracted from runPurge so the tool runs the CLI's own purge over the CLI's own target list, rather than a second implementation free to miss a store. collectMinerPredictionMetrics is likewise extracted in the engine: the Prometheus text renderer now formats those families, so the scrape and the JSON snapshot share one aggregation and cannot disagree about what a counter means. validate:mcp earned its keep twice here. It caught the two AMS tenant tools declared but never registered, and it caught loopover_miner_run_migrations/_purge_repo returning fields their output schemas did not declare -- the .shape re-wrap that drops looseObject's catchall, the same -32602 class this epic already fixed once. Both outputs now declare every field their handlers return. AMS tenant create/list/destroy are deliberately absent: #9522's loopover_tenant_* tools are product-parameterized and already serve product "ams", because the control plane's routes are. Only health and wake -- the pair with no ORB counterpart -- are added. The catalog's recorded exclusions (calibration floors, raw run-state set, the one-way kill switch) stay CLI-only, pinned by a test so reversing that decision has to be deliberate.
The store operations and the registered handlers were only reachable through the MCP tools,
so nothing measured them directly. These drive each action end to end -- action name
through the governor gate to the store call -- plus the validator guard clauses for null,
primitive, and array params, and the fail-closed paths (flag disabled, unknown action,
malformed params never reaching a store).
registerMinerOpsChatActions gains the same evaluateGate seam the dashboard's own
registerPortfolioQueueChatActions already exposes, so a test can drive the dispatch path
without the real chokepoint; production passes none and gets the real one.
paramsOf loses its `?? {}`: every action that calls it has a validator requiring an object,
and dispatchChatAction runs that validator first, so the fallback had no reachable case.
The hand-edited list this branch added is replaced by the generated one now that #9590 has landed -- which is what the issue asked for: the tool tables pick new tools up with no hand-edits. 163 registry entries across the three servers.
codecov/patch was short because the new tool REGISTRATIONS were never driven in-process --
the miner bin's 169 changed lines had 54 uncovered statements, and the AMS tenant handlers
had none of their own tests at all.
These drive every new tool over the in-memory transport against injected seams: the doctor
mapping from status.js's {name, ok, detail} onto the contract's pass/warn/fail, the metrics
snapshot's shared aggregation, and each mutating tool's dispatch and result shaping --
including that a governor refusal comes back as a structured blocked result rather than a
thrown error, and that a purge with confirm absent or false is rejected by the schema
BEFORE any dispatch happens.
The AMS tenant pair gets its not-configured, healthy, and throttled paths, the last of
which is the one that must not be audited as a cycle: the schedule guard refusing a
too-soon wake is the guard working, not work that ran.
Writing the metrics test surfaced a fixture trap worth naming: toPredictionRecords reads
the LEDGER row shape (conclusion/targetId/ts), so a fixture shaped like the downstream
record yields conclusion: undefined and fails output validation. The test now uses the
ledger's own shape.
808d5e7 to
e4ea2a7
Compare
|
Both blockers cite files that do not exist in this repo, and I could not reproduce either against the real code. Details, since I'd rather show the evidence than just assert it: 1. On the substance: it is correct that there is no
2. Both claims about it are wrong:
The two findings also appear to have been swapped: the one told it lacks I have pushed real fixes for the two genuine problems in the meantime — the merge conflict (resolved against #9522, now merged) and patch coverage, which is now at zero uncovered changed lines across every file in the diff. |
…suite codecov reported this file at 35.84% under the `engine` flag while the root vitest suite showed it fully covered. Both were right: @loopover/engine uploads coverage from its own node:test suite (packages/loopover-engine/test/**), and that suite had no test for this file at all — the vitest one lands under `backend` and does not lift the engine flag. So the aggregation and the renderer get tests where the engine actually measures itself, including the property the split exists for: every sample `collectMinerPredictionMetrics` reports appears verbatim in what `renderMinerPredictionMetrics` emits, so the JSON snapshot and the Prometheus scrape cannot report different numbers. Also pins the escaping, which is a correctness rule rather than cosmetics: a conclusion is DATA, so a quote, backslash, or newline inside one must not forge a second series.
|
Confirming the
const hadError = perStoreResults.some((entry) => "error" in entry);
return { outcome: hadError ? "partial" : "purged", ... };And The 2-on-partial contract stays pinned by the existing suite ( |
1 participant
Closes #9523. Stacked on #9604 (
feat/orb-management-tools-9522) — review that first; this branch contains its commits.What changed
The miner MCP exposed 11 read-only tools while its entire mutating ops surface was CLI-only. This adds 10: a dedicated
doctor(split out ofstatusso status stays cheap and doctor can grow checks), a structuredmetrics_snapshot, and eight mutations — governor pause/resume, queue release/requeue, claim release, deny-hook decide, store migrations, and a confirmed right-to-be-forgotten purge.Every mutation dispatches through the miner's existing governor-gated chat-action chokepoint — the same boundary the dashboard's own actions use. The MCP layer never touches a store: it dispatches an action name, and the registry structurally refuses any handler not produced by
governorGatedHandler(), whose brand is a private symbol a raw function cannot forge. That is what makes "an MCP caller cannot reach a write path the dashboard could not" a property of the code rather than of review discipline, and the structural test asserts it from both ends — including that a blocked gate never reaches the store operation.Three claims I had to correct against the real code
Each of these I'd written into the contract from the issue's description, then found the code disagreed:
applyflag either, rather than advertising a safety mode that does not exist and could only be built as a second implementation of the migration walk.(repo, id). The decide tool takes the repo rather than scanning every repo's proposals to resolve a bare id.purgeRepoAcrossStoresis extracted fromrunPurge, so the tool runs the CLI's own purge over the CLI's own target list — not a second implementation free to miss a store as targets are added.collectMinerPredictionMetricsis likewise extracted in the engine: the Prometheus text renderer now formats those families, so the scrape and the JSON snapshot share one aggregation and cannot disagree about what a counter means. All 762 engine tests pass unchanged.validate:mcp earned its keep twice
It caught the two AMS tenant tools declared but never registered, and it caught
loopover_miner_run_migrations/_purge_reporeturning fields their output schemas did not declare — the.shapere-wrap that dropslooseObject's catchall, the same -32602 class this epic already fixed once. Both outputs now declare every field their handlers return.Catalog decisions, recorded rather than improvised
AMS tenant create/list/destroy are deliberately absent: #9522's
loopover_tenant_*tools are product-parameterized and already serveproduct: "ams", because the control plane's own routes are. A second AMS-only spelling would be two names for one capability — the rot that keptloopover_fleet_get_analyticsout of #9522. Onlyhealthandwake, the pair with no ORB counterpart, are added.The issue's recorded exclusions — calibration floors, raw run-state
set, the one-way kill switch — stay CLI-only, pinned by a test so reversing that decision has to be deliberate rather than accidental.Validation
Typecheck clean;
validate:mcppasses across all three servers.chat-miner-ops-actions.tsandminer-ops-actions.tsare at 100% statement and branch, covering the full path (action name → gate → store call), the validator guard clauses, and the fail-closed cases.paramsOflost its?? {}— every caller has a validator requiring an object that runs first, so the fallback had no reachable case.Docs: the miner README's tool list is hand-edited here because #9521's generator is not merged yet; once #9590 lands it owns that table and the entries regenerate.