fix(cli): fail closed on GitHub verification skip in issues register#713
Conversation
anderdc
left a comment
There was a problem hiding this comment.
Functionality looks correct and the fix is proportionate. A few things to clean up before merge.
1. Docstring style doesn't match the repo convention
Existing docstrings in helpers.py (and the rest of the CLI modules) are terse, plain-text, no backticks, typically 1-2 sentences. Examples from the same file:
"""Apply Click decorators in the declared display order.""""""Format raw token amount (9-decimal) as human-readable ALPHA string.""""""Wrap status text with the appropriate Rich color tag."""
The new docstrings in this PR use RST-style double-backticks (``validate_repository``, ``click.BadParameter``) and multi-paragraph design-justification prose. The CLI modules have zero prior uses of double-backticks, so this PR would be introducing a new style into files that have never had it.
Please rewrite the new docstrings to match. For _raise_github_verification_required, something like:
def _raise_github_verification_required(...) -> None:
\"\"\"Raise BadParameter when a GitHub existence probe could not complete.
Used by the strict variants of validate_repository and validate_github_issue
so mutation commands never fall through to on-chain writes on transient failures.
\"\"\"Same for the updated docstrings on validate_repository and validate_github_issue — keep them terse, drop the backticks, drop the multi-paragraph rationale.
2. Error messages leak Python exception class names to end users
The current format produces:
Could not verify issue #42 in owner/repo on GitHub (status 503). ...(readable)Could not verify issue #42 in owner/repo on GitHub (ConnectionError). ...(leaks Python class name)Could not verify issue #42 in owner/repo on GitHub (Timeout). ...(leaks Python class name)
ConnectionError / Timeout are Python class names, not meaningful to CLI users. Map exception paths to human-readable terms:
# Currently:
_raise_github_verification_required(..., type(exc).__name__, param_hint='--issue')
# Better:
_raise_github_verification_required(..., 'network error', param_hint='--issue')If you want to distinguish timeout from connection error, map them explicitly:
detail = 'timeout' if isinstance(exc, requests.Timeout) else 'network error'3. Optional — latent parameter combo
validate_repository accepts both verify_exists=True (default) and require_verified_exists=False (default). If a future caller passes verify_exists=False, require_verified_exists=True, the verification block is skipped entirely and require_verified_exists silently has no effect. Not an issue today since issue_register uses the right combination, but worth a guard:
if require_verified_exists and not verify_exists:
raise ValueError('require_verified_exists requires verify_exists=True')Optional, take it or leave it depending on your defensive-coding taste.
Otherwise:
The bug is real and well-documented (bittoby's issue #712 was excellent), the fix is narrow (opt-in flag, default preserves all existing callers), the tests are unusually good (back-compat lock-in, 404 still wins, sentinel tests that contract_instance.exec is never reached). Once the docstring style and the exception-name leak are fixed, this is ready to merge.
1191bd6 to
b1df6b1
Compare
… of warn-and-skip Made-with: Cursor
b1df6b1 to
4d12ae5
Compare
|
@anderdc Thanks for your feedback. I updated all based on your feedback. Please review again. |
|
can you provide screenshots of the commands working correctly/failing properly |
Summary
gitt issues registeris owner-only and spends real ALPHA. Before writing to the contract, it callsvalidate_repository()andvalidate_github_issue()to check the target on GitHub — but both helpers silently warn-and-continue on network errors, 5xx, or 403 rate-limits.issue_registerdrops the return value, so a GitHub blip is enough for the CLI to fall through tocontract_instance.exec('register_issue', ...)with nothing verified. With-ythere isn't even a confirm prompt in between.The validator side doesn't clean these up either — unresolvable entries just get logged each cycle until the owner cancels them manually.
Fix is narrow and opt-in: add a
require_verified_exists=Falseflag to both helpers. Default keeps every existing caller (view, list, vote, admin, harvest) exactly as it was. Onlyissue_registerflips it toTrue, and the register flow now aborts with a clear error before the on-chain call if GitHub verification couldn't complete. A 404 still raises its existing "not found" message; the flag only tightens the skip branches.Changes
gittensor/cli/issue_commands/helpers.py— new private_raise_github_verification_required()helper, and arequire_verified_existskwarg onvalidate_repository/validate_github_issuethat turns the warn-and-skip branches intoclick.BadParameter.gittensor/cli/issue_commands/mutations.py—issue_registerpassesrequire_verified_exists=Trueto both validators. Nothing else changes; the existingexcept click.BadParameteralready surfaces it as a non-zeroClickException.tests/cli/test_cli_helpers.py— unit tests for 503 / 403 /ConnectionError/Timeout, default-off back-compat, 404 still winning. Plus two end-to-end tests that rungitt issues register ... -y, mock GitHub into a 503 /ConnectionError, and use a sentinel to assertcontract_instance.execis never reached.Test plan
uv run --with pytest pytest tests/cli/test_cli_helpers.py -q— 84 passeduv run --with ruff ruff check gittensor/cli/issue_commands/ tests/cli/uv run --with ruff ruff format --check gittensor/cli/issue_commands/ tests/cli/gitt issues list,gitt vote ...,gitt admin ...,gitt harvest) still warn-and-skip on network errors — they don't pass the flag.Closes #712