Skip to content

Fix go-publish argv order and emit git hints on failure#46

Merged
yahavi merged 2 commits intomainfrom
improvement/go-publish-argv-order-and-hints
Apr 20, 2026
Merged

Fix go-publish argv order and emit git hints on failure#46
yahavi merged 2 commits intomainfrom
improvement/go-publish-argv-order-and-hints

Conversation

@yahavi
Copy link
Copy Markdown
Member

@yahavi yahavi commented Apr 20, 2026

Overview

Fixes ascii-frog run 499751, where the Go Release job failed with failed to resolve version: git log failed: exit status 128 even though --version v1.0.428 was explicitly passed to go-publish.

Root cause: the action was assembling the Fly CLI invocation as ["publish", "go", modulePath, "--version", version]. urfave/cli v2 stops parsing flags after the first positional argument, so --version was silently dropped and the CLI fell back to auto-detecting the version from git — which blew up on /tmp/go-release (not a git repo).

What changed

  1. Flip the argv order. --version is now pushed before modulePath, so the CLI actually sees the flag:

    before: publish go ./mydir --version v1.2.3   # --version silently dropped
    after:  publish go --version v1.2.3 ./mydir   # --version parsed correctly
    
  2. Emit core.notice hints when the CLI reports git-related failures, so the remediation stays visible even if the red failure summary gets truncated in the GitHub UI:

    • Message contains "not a git repository" → hint to run git init && git add -A && git commit -m init.
    • Message contains "dubious ownership" or "safe.directory" → hint to run git config --system --add safe.directory '*'.
  3. Regression test asserts --version appears before MODULE_DIR in the invocation argv so a future refactor can't reintroduce the ordering bug.

Flow

flowchart LR
    Action["go-publish action"] --> ArgsBefore["argv: go MODULE_DIR --version"]
    ArgsBefore -.->|"urfave/cli drops --version"| Broken["exit 128 on git log"]
    Action --> ArgsAfter["argv: go --version MODULE_DIR"]
    ArgsAfter --> CLI["Fly CLI parses --version correctly"]
    CLI -->|"error"| Hints["Emit core.notice hint for known git classes"]
    CLI -->|"success"| Publish["Publish artifacts"]
Loading

Notes

  • Version compatibility: the hint strings come from the CLI's error message, not the action. Old CLI (pre-companion-PR) + this action = no hints but correct argv order; new CLI + old action = rich CLI message with no action-side hint. Both degrade gracefully.
  • Companion PR in fly-desktop (https://github.com/jfrog/fly-desktop/pull/195) adds CLI-side defense-in-depth: RunCapture helper, checkGitRepo preflight with hint-rich errors, and a trailing-arg guard that rejects fly publish go DIR --version V with a clear hint — so future callers that get the ordering wrong get the same experience.

* Place --version before MODULE_DIR when invoking the Fly CLI. urfave/cli
  v2 stops flag parsing after the first positional arg, so the previous
  order silently dropped --version and the CLI fell back to auto-detecting
  the version from git. That is the failure mode that caused ascii-frog
  run 499751 (https://github.jfrog.info/JFROG/ascii-frog/actions/runs/499751).
* When the CLI reports a git-related failure, emit a `core.notice` hint
  so users see the remediation even if the red failure summary gets
  truncated in the GitHub UI:
  - "not a git repository" -> run `git init && git add -A && git commit`.
  - "dubious ownership" / "safe.directory" -> add a safe.directory config.
* New regression test asserts --version appears before MODULE_DIR in the
  invocation argv so a future refactor can't reintroduce the ordering bug.
@yahavi yahavi added bug Something isn't working javascript Pull requests that update javascript code labels Apr 20, 2026
@yahavi yahavi self-assigned this Apr 20, 2026
CI's "Verify lib/ is compiled and up to date" check failed because
src/go-publish.ts was updated (argv ordering fix + git hint emission)
without rebuilding the committed artifact. `npm run build` regenerates
only lib/go-publish.js; no other bundles changed.
Copy link
Copy Markdown
Collaborator

@sverdlov93 sverdlov93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Review comment by Cursor AI

I looked at this PR. I look at a lot of PRs — more than anybody, believe me — and this one is clean. Very, very clean. Tremendous, really.

Reviewed as: GitHub Actions / TypeScript expert

Findings: No issues found.

Test coverage: All new code is tested — argv ordering (explicit index assertion), git-repo hint, dubious-ownership hint, safe.directory variant, negative test for no-hint on generic errors, updated existing test for new argv shape. Comprehensive.

Key observations (no action needed):

  • core.notice() before core.setFailed() ensures hints stay visible even when the failure summary is truncated — smart UX
  • Independent if blocks for both hint types (not if/else) is correct since different error results could trigger both
  • Graceful degradation with old/new CLI versions is well-designed

⚠️ THIS IS NOT A REPLACEMENT FOR HUMAN REVIEW, FOLKS! Human intelligence is TREMENDOUS — the best, really — and can't be replaced. NOT EVEN CLOSE! This AI review is an ADDITION to your big-league human reviewers. ALWAYS get a human review too!

@yahavi yahavi merged commit c1e7514 into main Apr 20, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working javascript Pull requests that update javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants