Skip to content

chore: Keep CLI minimum-version reminders accurate#1224

Merged
hatayama merged 2 commits into
v3-betafrom
feature/hatayama/move-cli-warning-to-go
May 27, 2026
Merged

chore: Keep CLI minimum-version reminders accurate#1224
hatayama merged 2 commits into
v3-betafrom
feature/hatayama/move-cli-warning-to-go

Conversation

@hatayama

Copy link
Copy Markdown
Owner

Summary

  • Pull request authors now get the CLI minimum-version reminder from the same Go toolchain that owns native CLI validation.
  • The reminder ignores automation-only Go files and keeps successful GitHub CLI notices out of parsed command output.

User Impact

  • PRs that change shipped native CLI behavior still get a clear reminder to confirm MINIMUM_REQUIRED_CLI_VERSION.
  • PRs that only update the reminder automation no longer get misleading compatibility warnings.

Changes

  • Added a small Go command for CLI minimum-version comment detection and GitHub comment upsert/resolve.
  • Reduced the shell script to a GitHub Actions entrypoint and set up Go in the workflow.
  • Added focused tests for diff classification, stdout parsing, and workflow behavior.

Verification

  • go test ./internal/automation ./internal/architecture -run TestMinimumVersion -count=1
  • scripts/test-comment-cli-minimum-version-warning.sh
  • scripts/test-cli-minimum-version-warning-workflow.sh
  • git diff --check
  • scripts/check-go-cli.sh
  • ~/.codex/skills/codex-review/scripts/codex-review --parallel-tests "scripts/check-go-cli.sh" v3-beta

Closes #1222

Keep the shell script as a thin GitHub Actions entrypoint while the Go command owns diff inspection and reminder comment upsert or resolution. This makes the warning behavior testable in the Go CLI codebase instead of leaving it in a large shell script.
@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 494ca598-a623-4153-af0a-d08fe80eaf09

📥 Commits

Reviewing files that changed from the base of the PR and between dd2b47e and 621e229.

📒 Files selected for processing (2)
  • Packages/src/Cli~/internal/automation/minimum_version_warning.go
  • Packages/src/Cli~/internal/automation/minimum_version_warning_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • Packages/src/Cli~/internal/automation/minimum_version_warning.go

📝 Walkthrough

Walkthrough

This PR migrates the CLI minimum version warning logic from a shell script into a new Go automation module. It adds RunMinimumVersionWarning with env-to-config resolution, changed-file detection, and GitHub comment upsert/resolve behavior; provides unit tests; adds a small Go main; updates the shell wrapper to invoke the Go command; and adds a Go setup step to the workflow.

Changes

Move CLI minimum version warning logic to Go

Layer / File(s) Summary
Core automation module
Packages/src/Cli~/internal/automation/minimum_version_warning.go
Constants define the GitHub comment marker, warning and resolved bodies, and CLI package root. Configuration struct holds repository context. Main entrypoint loads config from environment with fallbacks, resolves the repository name, computes changed files via git diff base...head, and orchestrates the comment lifecycle: post a warning if Go CLI changed but version file didn't, update existing warning, or resolve completed warnings.
Comment ops: upsert/resolve/find
Packages/src/Cli~/internal/automation/minimum_version_warning.go
Locates existing marker comments by paginating PR comments, writes temp JSON bodies for gh api --input, PATCHes existing comments or POSTs new ones for warnings, and PATCHes to a resolved body when no warning is required.
Helpers and classification
Packages/src/Cli~/internal/automation/minimum_version_warning.go
Exec helper runs external commands capturing stdout/stderr, file-classification rules identify qualifying Go CLI changes (cmd/, internal/, go.mod/sum, layout/contract JSON), and output helpers format messages to stdout/stderr.
Automation test coverage
Packages/src/Cli~/internal/automation/minimum_version_warning_test.go
Five tests: env-to-config mapping with fallbacks; early-skip when base ref missing; table-driven file-classification cases; suppression when MINIMUM_REQUIRED_CLI_VERSION changes; and ensuring stdout-only output parsing excludes stderr.
Go entrypoint and workflow setup
Packages/src/Cli~/cmd/comment-cli-minimum-version-warning/main.go, .github/workflows/cli-minimum-version-warning.yml
Adds a small main that calls RunMinimumVersionWarning and exits with its code. Workflow gains a "Setup Go" step using actions/setup-go with go-version-file: Packages/src/Cli/.go-version and cache: false.
Shell script refactor
scripts/comment-cli-minimum-version-warning.sh
Shell wrapper now computes CLI directory, changes into it, sets ULOOP_REPOSITORY_ROOT, and delegates logic to go run ./cmd/comment-cli-minimum-version-warning, removing the previous in-script Git/GH logic.
Architecture and integration test updates
Packages/src/Cli~/internal/architecture/architecture_test.go, Packages/src/Cli~/internal/architecture/comment_cli_minimum_version_warning_test.go, scripts/test-cli-minimum-version-warning-workflow.sh
Allows /internal/automation imports in architecture tests. Integration tests now assert the Go implementation prints "Posted", "Updated", and "Resolved" status messages. Workflow test asserts presence of the pinned actions/setup-go step with cache: false.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: moving CLI minimum-version reminder logic to Go to maintain accuracy by filtering out irrelevant automation files.
Description check ✅ Passed The description is well-related to the changeset, explaining the summary, user impact, and detailed changes made across Go files and shell scripts.
Linked Issues check ✅ Passed The PR fulfills all acceptance criteria from issue #1222: delegates shell script to Go implementation, covers GitHub comment behavior with tests, preserves external behavior, and updates workflow with Go setup.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective of moving CLI minimum-version warning logic from shell to Go, with no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/hatayama/move-cli-warning-to-go

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Packages/src/Cli`~/internal/automation/minimum_version_warning.go:
- Around line 48-58: The code is resolving the repository with
resolveMinimumVersionWarningRepository before checking config.baseRef, which
causes failures (e.g., missing GITHUB_REPOSITORY or gh) even when we should
early-skip; change the logic so you first check if config.baseRef == "" and call
writeMinimumVersionWarningLine(stdout, "Skipping CLI minimum version comment
because no base ref was provided.") and return 0 immediately, and only if a
baseRef exists call resolveMinimumVersionWarningRepository(ctx, config), assign
to config.repository, and handle its error as before; update references to
repository/config.repository accordingly so resolution happens after the
early-skip.
- Around line 231-250: minimumVersionWarningIsGoCliFile incorrectly treats
test-only files under the CLI internal/ tree as shipped changes; update
minimumVersionWarningIsGoCliFile to early-return false for files with suffix
"_test.go" (check strings.HasSuffix(changedFile, "_test.go") or inspect the
relativePath after trimming goCliPackageRoot) before the block that treats
internal/ changes as true, and add a unit test regression case asserting that an
internal *_test.go file is considered non-CLI-change (false). Ensure references
to goCliPackageRoot and the internal/ handling logic in
minimumVersionWarningIsGoCliFile are used so the fix targets the right branch.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bf6b2d42-37ab-4a8e-8868-4fb2962abc82

📥 Commits

Reviewing files that changed from the base of the PR and between 7c365ee and dd2b47e.

📒 Files selected for processing (8)
  • .github/workflows/cli-minimum-version-warning.yml
  • Packages/src/Cli~/cmd/comment-cli-minimum-version-warning/main.go
  • Packages/src/Cli~/internal/architecture/architecture_test.go
  • Packages/src/Cli~/internal/architecture/comment_cli_minimum_version_warning_test.go
  • Packages/src/Cli~/internal/automation/minimum_version_warning.go
  • Packages/src/Cli~/internal/automation/minimum_version_warning_test.go
  • scripts/comment-cli-minimum-version-warning.sh
  • scripts/test-cli-minimum-version-warning-workflow.sh

Comment thread Packages/src/Cli~/internal/automation/minimum_version_warning.go Outdated
Comment thread Packages/src/Cli~/internal/automation/minimum_version_warning.go

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

2 issues found across 8 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread Packages/src/Cli~/internal/automation/minimum_version_warning.go Outdated
Comment thread Packages/src/Cli~/internal/automation/minimum_version_warning.go
Skip missing-base runs before repository resolution and ignore Go test files when deciding whether a PR changes shipped native CLI behavior.
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