Skip to content

I3 [CRIT/CLI] Exit-code consistency across every parent subcommand (I4 + I23 + I27 + I36 + I44 + I46)#35

Open
espadonne wants to merge 17 commits into
trunkfrom
i3/exit-code-consistency
Open

I3 [CRIT/CLI] Exit-code consistency across every parent subcommand (I4 + I23 + I27 + I36 + I44 + I46)#35
espadonne wants to merge 17 commits into
trunkfrom
i3/exit-code-consistency

Conversation

@espadonne
Copy link
Copy Markdown
Contributor

Summary

CI scripts grep on exit code; pre-fix they couldn't. Pre-existing E16 + C18 patched a handful of parents (cache, repo, issue, pr, org). The I-audit found 17 more silently exiting 0 on unknown subcommands. This PR closes the gap symmetrically.

Closed:

  • I4: every parent (auth, alias, extension, config, variable, release, workflow, gpg-key, search, label, run, project, secret, attestation, gist, ssh-key) now applies Args: cobra.ArbitraryArgs + RunE: cmdutil.ParentRunE(). Unknown subcommand → exit 1 with unknown command "X" for "shithub Y"; run 'shithub Y --help' for usage.
  • I23: the audit's contrast finding (completion xxxxxxxx exits 1 but auth zzzz exits 0) — closes automatically once I4 closes.
  • I27: secret get parent has no get subcommand. New Long doc explicitly notes secrets are write-only (matches gh). shithub secret get MY now surfaces unknown command "get" for "shithub secret". Partial caveat: shithub secret get MY -R foo/bar still shows unknown shorthand flag: 'R' in -R because cobra parses flags before resolving the subcommand. Documented as known limitation; tracked separately.
  • I36: api -X "" (explicit empty value) used to silently default to GET. New MethodSet field on Options, populated from c.Flags().Changed("method") — distinguishes "user didn't pass -X" (default-GET, unchanged) from "user passed -X with empty value" (typo, now errors).
  • I44: re-verified live; already exits 1 on repo create "../sneaky-name". Audit-doc reproduction was stale. The error message is still misleading (org not found) — that's I24 territory, lives in I12 bundle.
  • I46: cross-ref of I27; closed by the I27 fix above.

Test plan

  • TestRunRejectsEmptyMethodFlagapi -X "" with MethodSet=true returns error mentioning -X requires a value
  • TestRunAcceptsUnsetMethod — default-GET path unchanged when MethodSet=false
  • make ci clean across the whole tree (16 parent files touched, _dev excluded because it's behind //go:build dev)
  • Manual dogfood of every patched parent — every <parent> totallyunknown now exits 1 with a useful message
  • Reviewer: confirm shithub <whatever> badverb; echo $? reports 1 across the board

Diff shape

17 commits, one per file (one extra for the api -X "" work). Each parent gets a 3-line addition — the helper already existed (internal/cmdutil/parent.go::ParentRunE); these PRs apply it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant