Skip to content

Conversation

@chicks-net
Copy link
Member

Done

  • ⏬ [just] broken GHAs should not prevent displaying the reviews in the terminal

Meta

(Automated in .just/gh-process.just.)

Copilot AI review requested due to automatic review settings October 18, 2025 21:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Allows displaying PR review info in the terminal even when some GitHub Actions checks fail by relaxing strict bash error handling.

  • Updated internal comment from version 3.3 to 3.4.
  • Removed bash strict mode (set -euo pipefail) from pr_checks to prevent premature exit on failing checks.
Comments suppressed due to low confidence (1)

.just/gh-process.just:1

  • Removing bash strict mode may allow silent failures (e.g., unset variables, ignored non-zero exit codes in pipelines) and make diagnosing real issues harder. If the goal is to tolerate specific failing commands (e.g., gh pr checks), consider reinstating set -euo pipefail and selectively relaxing with constructs like command || true or set +e around the intended tolerant block, then restoring with set -e.
# git/gh process justfile

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@claude
Copy link

claude bot commented Oct 18, 2025

Code Review - PR #26

Summary

This PR removes set -euo pipefail from the pr_checks recipe to prevent broken GitHub Actions from blocking the display of review comments in the terminal. The change is focused and addresses a specific UX issue.

✅ Positive Aspects

  1. Clear Intent: The change directly addresses the stated goal - allowing the script to continue and display reviews even when gh pr checks fails
  2. Minimal Scope: Only removes the strict error handling where needed, leaving it intact in the pr recipe above (line 21)
  3. Version Bump: Properly incremented the comment version from 3.3 to 3.4

🔍 Code Quality Observations

Good:

  • The change is surgical and doesn't affect other parts of the workflow
  • The rationale is clear from the commit message and PR description

Consider:

  1. Partial Error Handling: After removing set -euo pipefail, there's no error handling at all in pr_checks. Consider using selective error handling:

    #!/usr/bin/env bash
    set -uo pipefail  # Keep undefined variable and pipe failure checks
    
    gh pr checks --watch -i 5 || true  # Allow this specific command to fail

    This approach would catch other potential errors (undefined variables, pipe failures) while allowing gh pr checks to fail gracefully.

  2. Exit Code Clarity: When gh pr checks fails, the rest of the script continues, but the overall exit code may be non-zero depending on the last command. Consider explicitly handling this if needed.

🐛 Potential Issues

Minor:

  • If there are any issues with the jq parsing, GraphQL query, or subsequent commands (lines 132-167), they will now silently fail rather than stopping execution
  • Undefined variables in the complex bash script (e.g., if GitHub API changes response structure) won't be caught

🔒 Security Concerns

No security issues identified. The change removes error checking but doesn't introduce new attack vectors.

⚡ Performance Considerations

No performance impact. The change is purely about error handling behavior.

🧪 Test Coverage

Manual Testing Recommended:

  • Test with a PR that has passing GitHub Actions
  • Test with a PR that has failing GitHub Actions (the main use case)
  • Test with a repo that has no GitHub Actions workflows
  • Verify Copilot and Claude comments still display correctly in all cases

📝 Recommendation

APPROVE with suggestions

The change accomplishes its stated goal. Consider the more selective error handling approach suggested above to maintain some safety while allowing the specific command to fail gracefully. The current implementation will work but sacrifices all error detection in the pr_checks recipe.


Review generated by Claude Code

@chicks-net chicks-net merged commit 959ed35 into main Oct 18, 2025
6 checks passed
@chicks-net chicks-net deleted the chicks/2025-10-18-just-tweak branch October 18, 2025 21:44
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.

2 participants