Skip to content

ci: checkout PR head on pull_request_target runs#620

Merged
davidnewhall merged 3 commits intoUnpackerr:mainfrom
SergioChan:fix/issue-619-pr-target-checkout
Mar 12, 2026
Merged

ci: checkout PR head on pull_request_target runs#620
davidnewhall merged 3 commits intoUnpackerr:mainfrom
SergioChan:fix/issue-619-pr-target-checkout

Conversation

@SergioChan
Copy link
Contributor

Summary

  • set explicit checkout repository to PR head repo (fallback to current repo) in codetests.yml
  • set explicit checkout ref to PR head SHA (fallback to current SHA)
  • apply this to all three test-and-lint jobs so pull_request_target runs validate the actual PR branch

Testing

  • python -c "import yaml; yaml.safe_load(open('.github/workflows/codetests.yml', 'r', encoding='utf-8'))"

Related

@SergioChan
Copy link
Contributor Author

Quick follow-up on the current red checks on this PR:

The failing run is pull_request_target for this PR itself, which executes the workflow definition from the base branch (main). That means this run still uses the pre-fix checkout behavior and can report unrelated lint findings from base-branch code.

So the current red status here is expected for this transition PR and does not invalidate the checkout-ref patch itself.

Validation performed for this change:

  • workflow syntax parse for .github/workflows/codetests.yml (yaml.safe_load) succeeded.

Once this lands, subsequent pull_request_target runs will use the updated checkout logic and should evaluate PR head commits as intended.

@SergioChan
Copy link
Contributor Author

Pushed a follow-up commit to make the checkout behavior explicit per event type.

What changed:

  • on pull_request_target, checkout now always uses github.event.pull_request.head.repo.full_name + github.event.pull_request.head.sha
  • on push, workflow uses a separate default checkout step

This avoids expression fallback ambiguity and prevents lint/test jobs from silently checking out origin/main during PR runs.

Validation:

  • python -c "import yaml, pathlib; yaml.safe_load(pathlib.Path('.github/workflows/codetests.yml').read_text(encoding='utf-8'))"
  • git diff --check

@SergioChan
Copy link
Contributor Author

Pushed a follow-up commit to address the failing golangci-lint checks on this branch:

  • Wrapped zenity-returned errors in pkg/ui/dlgs.go to satisfy wrapcheck
  • Renamed variadic v args to args in dialog helpers to satisfy varnamelen

Validation run locally on this branch:

  • go test ./...
  • go vet ./...

@SergioChan SergioChan force-pushed the fix/issue-619-pr-target-checkout branch from 4c9e577 to 7bdf719 Compare March 12, 2026 17:58
@SergioChan
Copy link
Contributor Author

Thanks — I force-pushed an update to keep this PR strictly scoped to #620.

What changed in this update:

  • Removed an unrelated UI/lint commit from this branch.
  • Kept only the workflow fix in .github/workflows/codetests.yml for pull_request_target vs push checkout behavior.

Validation run:

  • go test ./...

@SergioChan
Copy link
Contributor Author

Pushed follow-up commit 40ff3b0 to address the cross-platform lint failures reported on this PR.

What changed

  • pkg/ui/dlgs.go
    • renamed variadic parameters (v -> args) for clearer naming in Error, Info, and Question
    • wrapped non-cancel zenity.Entry errors with context (fmt.Errorf("zenity entry: %w", err)) to satisfy wrapcheck
    • returned explicit success tuple (entry, true, nil) on the happy path

Validation

  • git diff --check
  • GOOS=windows go test ./pkg/ui -run TestDoesNotExist -count=1
  • GOOS=darwin go test ./pkg/ui -run TestDoesNotExist -count=1 ❌ blocked by upstream github.com/getlantern/systray darwin symbol build errors in this environment (nativeLoop/registerSystray/quit unresolved)

Please re-run CI; this commit should clear the varnamelen and wrapcheck lint failures shown in the previous run.

@davidnewhall davidnewhall merged commit 15dba7b into Unpackerr:main Mar 12, 2026
12 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

CI: pull_request_target lint/test checks out base branch instead of PR head

2 participants