DAH-2593: stop the CLI Action layer from swallowing errors - #104
Conversation
| _emit_json_error("lium_error", str(e), EXIT_API_ERROR) | ||
| console.error(f"Error: {e}") | ||
| raise SystemExit(EXIT_GENERAL_ERROR) | ||
| raise SystemExit(EXIT_API_ERROR) |
There was a problem hiding this comment.
Critical: the published exit table has no 3 or 6 and gives both to lium provider, where 6 means config missing. Needs a lium-docs PR alongside.
There was a problem hiding this comment.
Right — the published table stops at 5 and hands 3/6/7 to lium provider. Moved the source of truth into the constants block in lium/cli/utils.py: every code now carries its meaning inline, plus a note that lium provider keeps its own map where 3 and 6 mean portal error and missing config. The lium-docs side (adding rows 3 and 6 to docs/developers/cli/reference/index.md and rewording the "Two exceptions" note) is a separate PR I have not opened yet.
| else: | ||
| console.error(f"Error: {e}") | ||
| raise SystemExit(EXIT_CONFIGURATION_ERROR) | ||
| except LiumPermissionError as e: |
There was a problem hiding this comment.
Heads up: PR #103 rewrites this same block, adding escape() to every branch. Whoever lands second merges by hand.
There was a problem hiding this comment.
Acknowledged — I will take the manual merge if #103 lands first; this PR leaves the escape() branches alone.
| ui.error("Bittensor library not installed") | ||
| ui.dim("Install with: pip install bittensor") | ||
| return | ||
| raise LiumError("Bittensor library not installed. Install with: pip install bittensor") |
There was a problem hiding this comment.
NIT: a missing library is a config error (2), not an API error (3). PR #103 also rewrites these lines with CliFailure.
There was a problem hiding this comment.
Agreed, it is a local environment problem — now CliFailure("bittensor_not_installed", ..., EXIT_CONFIGURATION_ERROR). Applied the same to the identical ImportError in the --alpha path so both exit 2.
| if result.error: | ||
| ui.warning(result.error) | ||
| if not result.ok: | ||
| raise CliFailure("key_not_found", result.error, EXIT_GENERAL_ERROR) |
There was a problem hiding this comment.
NIT: unsetting an absent key is the same idempotent case as rm --all, which you keep at exit 0.
There was a problem hiding this comment.
Agreed — reverted to the previous behaviour: a warning and exit 0, since the key is already in the asked-for state.
| ui.error(error) | ||
| return | ||
| # "not found" is a miss; a pod that exists but cannot take a session is ssh's own failure. | ||
| if "not found" in error and "SSH connection" not in error: |
There was a problem hiding this comment.
NIT: matching on error text breaks as soon as the wording changes. Better to return a code from parsing.parse.
There was a problem hiding this comment.
Done — parsing.parse now returns tuple[dict | None, CliFailure | None] and builds the failure with its own code, so the command just re-raises it. validation.validate still returns text; say the word and I will move it over too.
… is a config error
ba8b089 to
a3e2b3b
Compare
taiberium
left a comment
There was a problem hiding this comment.
Approved.
All five comments are addressed. The one thing left is outside this repo: the exit-code table in lium-docs still stops at 5 and says 3/6/7 belong to lium provider only — that needs its own docs PR before this reads correctly.
Continuation of DAH-2556. Every
Action.execute()wrapped its work in a blanketexcept Exceptionand returnedActionResult(ok=False, error=str(e)); thecommand then did
if not result.ok: ui.error(result.error); return, and a barereturnfrom a Click callback exits 0.handle_errors— the one place thatmaps a failure to an exit code — never ran, because nothing was ever raised.
What changed
Actions stop swallowing. The exception reaches
handle_errors, whichalready renders it once and maps it to a code. Commands raise
CliFailureonlyfor decisions they make themselves — the
rm/command.py:30-65pattern.Three classes of
exceptblock are treated differently:except Exceptionaround an SDK call with no decisioninside it.
failed_huids(
rm×2,reboot,rsync,scp,volumes rm,schedules rm). Removingtheir
exceptwould stop the loop at the first bad item. Their command raisesonce at the end, mirroring
rm/command.py:122-134.EditConfigAction'sCalledProcessError,SetupSshKeyAction's localssh-keygen,
fund's bittensor/subtensor wrappers, a best-effortexcept OSError: passaround the ssh-key cache write.Exit codes 3 and 6 became reachable.
lium/sdkgrows a typedLiumPermissionErrorraised on 403 at bothclient.pystatus sites (_requestand the streaming/logs path).
handle_errorsgains anexcept LiumPermissionErrorclause before the generalexcept LiumError— ordermatters, or the general clause absorbs it — mapping 403 to
EXIT_PERMISSION_DENIED(6). The generalLiumErrorbranch moves fromEXIT_GENERAL_ERROR(1) toEXIT_API_ERROR(3), soLiumAuthError,LiumNotFoundError,LiumRateLimitErrorandLiumServerErrorstop collapsingto exit 1. The JSON envelope's code token stays
"lium_error"— that token is aseparate contract from the exit number and
test/test_topup_cli.py:96pins it.The
rmasymmetry is preserved where it applies.--allagainst an emptyaccount stays an idempotent no-op (exit 0); a named target that matches nothing
is a failure (exit 5).
rebootgains that asymmetry;rmalready had it.Sites the ticket did not name
lswas never fixed by DAH-2556.ls/command.py:100-102still swallowed,so an API error while listing the market exited 0. DAH-2556 only fixed ls's
--sort/Pareto-star bug.ensure_config()(lium/cli/utils.py:783) discarded both Action resultswithout even checking
.ok, and gates ~15 commands.fundwithout--alphais the default path, not dead code despite the_legacy_tao_fundname. It held five silent exit-0 sites.update,theme,templates,ssh-keys list,ssh-keys sync,volumes new,volumes list,ps,logs.psis the command an agent reaches for right afterls.port-forward, found by the closing grep sweep: four named-target missesthat printed an error and exited 0.
Behaviour changes worth reviewing
lium ps <named-target>that matches nothing now exits 5 in both outputformats.
--format jsonused to print[]and exit 0 even when the callernamed one specific pod — the
rmasymmetry read backwards. An unfilteredempty list still exits 0.
lium sshon a dropped connection (ssh's own 255) now exits 4, matching whatup's SSH path already did. A non-zero remote shell exit code still exits0 — that is the remote's business.
rsync,scp,volumes rmandschedules rmlose the ability to no-opagainst an empty account: they have no boolean
--allto key the asymmetryoff, only a
targetsstring that may literally read"all". Onlyrmandrebootkeep the no-op.raise LiumError(...)site (_alpha_fund,topup) moves fromexit 1 to exit 3. No test outside the out-of-scope trees asserted the old
number.
lium config reseton an already-empty config stays a warning at exit 0 — therequested end state is already true, same reasoning as
rm --all.Known limits, deliberately not fixed here
lium ls --format jsonstill renders failures as Rich text on stderr ratherthan the JSON error envelope:
handle_errorskeys the envelope off a kwargnamed
json_output, andlsnames its flagoutput_format. The exitcode — the thing an agent acts on — is fixed either way.
get_template, the GPU-shortname resolver,exec_all'sper-pod wrapper) still turn any exception into
None/an untyped dict, and_ensure_ssh_keys_registeredstill downgradesLiumErrorto a warning bydesign (ssh-key registration must not block a rental). A 403 arriving through
those paths will not become exit 6.
lium provider …has its own exit-code taxonomy that collides with the maintable.
ProviderError+_EXIT_CODESinprovider/_render.py:59-84uses6 = config missing, 2 = auth, 5 = ssh, 7 = token-cache contention, while
the main table reads 2 = configuration, 5 = pod-not-found, 6 =
permission-denied. The two agree only on 3. Nothing in that tree swallows
errors, so it needed no fix here — but the collision is real and worth its own
ticket.
Tests
19 new behavioural tests, one per command family, in
test/test_agent_cli_contract.pyplus two SDK-level 403 tests intest/test_sdk_client.py. Each was verified RED against the unfixed code andGREEN after.
test_up_fails_when_ssh_is_unavailablewas rewritten: it assertedresult.ok is FalsefromPrepareSSHAction, i.e. it encoded the very swallowthis PR removes. It now asserts
pytest.raises(CliFailure)withexit_code == EXIT_SSH_ERROR.Full suite: 388 passed, 11 failed — the same 11 pre-existing failures as on
main(test/provider/×5,test_gpu_splitting_cli.py×3,test_release_binary_targets.py×3), zero new. Note that no CI workflow runsthe general suite:
ci.ymlandrelease.ymlonly runtest/test_release_binary_targets.py.