Skip to content

miner(deny-hooks): the CLI mis-parses --history's value and ignores --json on failures #9687

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

runDenyHooks in packages/loopover-miner/lib/deny-hooks-cli.ts is the only miner subcommand that hand-rolls both its argument parsing and its error reporting.

1. Flag values leak into the positional list.

const positional = args.filter((arg) => !arg.startsWith("--"));
const [subcommand, repoFullName, proposalId] = positional;

--history's value (a file path) does not start with --, so it lands in positional. With the flag placed before the repo — loopover-miner deny-hooks refresh --history hist.json acme/widgets — the destructure yields repoFullName = "hist.json", and the command then operates on a nonsense repo identifier. Every other miner CLI consumes a flag's value by index (index += 1) precisely to avoid this; closed issue #5833 fixed the identical class of bug for hooks check --tool/--input.

2. Failures bypass the shared CLI error contract. packages/loopover-miner/lib/cli-error.ts defines reportCliFailure(wantsJson, message, exitCode = 2), documented as "when --json is set, emit a parseable { ok: false, error } object on stdout (matching each command's success-path JSON stream)". Every other subcommand uses it — portfolio-queue-cli.ts, purge-cli.ts, claim-ledger-cli.ts, migrate-cli.ts, governor-pause-cli.ts, loop-cli.ts, attempt-cli.ts. deny-hooks-cli.ts does not import cli-error.js at all: it prints console.error(USAGE) / raw String(error) to stderr regardless of --json, and returns 2 for usage errors but 1 for runtime errors. A --json consumer of deny-hooks list --json gets a bare stack-ish string on stderr and an exit code no other subcommand produces.

Requirements

  • Replace the args.filter(...) positional extraction with an index-based loop that consumes --history <value> as a flag+value pair, so the value never becomes a positional. The loop must reject a missing value or a value beginning with - by returning the usage error, matching parseRepoIdentifierArgs's --api-base-url handling in portfolio-queue-cli.ts:201-209.
  • The loop must reject any unrecognized token beginning with - with Unknown option: <token>, matching portfolio-queue-cli.ts:210-212.
  • Import argsWantJson, describeCliError and reportCliFailure from ./cli-error.js and route every failure path (missing subcommand, missing repo, missing proposal id, missing --history, unknown subcommand, and the catch-all) through reportCliFailure, so --json produces { ok: false, error } on stdout.
  • Every failure path must return 2 (the shared default), not 1.
  • Success paths, their output shapes, and the store.close() in finally must be unchanged.

⚠️ Required pattern: mirror parseRepoIdentifierArgs + runQueueRelease in packages/loopover-miner/lib/portfolio-queue-cli.ts:180-235 and :396-430 — an index-based parse returning a { ... } | { error } union, then a runner that routes both the parse error and the catch through reportCliFailure. It does NOT satisfy this issue to keep the filter-based extraction and merely special-case --history's value, to add --json handling only to the catch-all, or to leave any failure path returning 1.

Deliverables

  • deny-hooks-cli.ts exports a parseDenyHooksArgs(args) returning a { subcommand, repoFullName, proposalId, historyPath, json } | { error } union, built with an index-based loop that consumes --history <value>.
  • loopover-miner deny-hooks refresh --history h.json acme/widgets resolves repoFullName to acme/widgets (today it resolves to h.json).
  • Every failure path in runDenyHooks returns reportCliFailure(json, message) (exit 2), and the module imports from ./cli-error.js.
  • New named regression tests in test/unit/miner-deny-hooks-cli.test.ts cover: --history before the repo positional resolving the repo correctly; a missing --history value returning the usage error; an unknown -x option returning Unknown option: -x; and a failing store call under --json printing { "ok": false, "error": ... } on stdout with exit 2.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example rewriting the parser without routing failures through reportCliFailure — does not resolve this issue.

Test Coverage Requirements

packages/loopover-miner/lib/**/*.ts IS inside Codecov's coverage.include in vitest.config.ts, so the 99%+ branch-counted codecov/patch gate applies exactly as for src/**. Both arms of every new parse branch need a test (flag present / absent, value present / missing, value flag-like / not), plus both arms of argsWantJson on at least one failure path. The --history-before-repo case must be a named regression test that fails against the current code.

Expected Outcome

loopover-miner deny-hooks parses its flags the same way every other miner subcommand does, and its failures are machine-readable under --json with the package's standard exit code 2.

Links & Resources

packages/loopover-miner/lib/deny-hooks-cli.ts:32-96, packages/loopover-miner/lib/cli-error.ts, packages/loopover-miner/lib/portfolio-queue-cli.ts:180-235, :396-430. Precedent: closed issue #5833.

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions