Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
🏗️ ADR Required — draft added for PR #63499I found that this PR requires ADR enforcement because the prefetch summary reports 154 added lines in default business-logic directories, which is above the 100-line threshold. Evidence used
Decision inferred from the PRThe architectural decision is to treat activation-time workflow imports and skills as trusted-base inputs for Action takenI added a draft ADR to this branch:
Next action for the authorPlease review and finalize that ADR so the security rationale, alternatives, and trade-offs are explicitly recorded before merge.
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The guarded ref preserves non-PR and reusable-workflow behavior, with focused regression coverage and consistent generated outputs.
Review effort: Balanced
Findings: None
What changed in this PR
Pins activation-time sparse checkouts to trusted base-branch content for pull_request events, preventing PR-head workflows and skills from influencing prompt generation.
Changes:
- Adds guarded base-SHA checkout selection with existing trigger fallbacks.
- Adds regression coverage for mixed triggers,
workflow_call, andpull_request_target. - Regenerates affected workflow lock files.
| File | Description |
|---|---|
pkg/workflow/compiler_activation_job.go |
Implements secure activation checkout ref selection and supporting refactors. |
pkg/workflow/compiler_activation_job_test.go |
Tests trigger-specific checkout refs. |
.github/workflows/visual-regression-checker.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/test-quality-sentinel.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/squad-implement-worker.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-update-cross-repo-pr.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-test-tools.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-temporary-id.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-pydantic.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-project.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-pi.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-opencode.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-multi-pr.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-kiro.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-goose.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-gemini.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-drive.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-deepseek-harness.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-cursor.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-crush.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-create-cross-repo-pr.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-copilot-auto.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-copilot-arm.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-codex.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-claude.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-ci.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-checkout-pr-dispatch.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-call-workflow.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-aider.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-agent-scoped-approved.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-agent-public-none.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-agent-public-approved.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-agent-all-none.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/smoke-agent-all-merged.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/refiner.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/pr-description-caveman.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/pr-code-quality-reviewer.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/ponytail-reviewer.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/mattpocock-skills-reviewer.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/impeccable-skills-reviewer.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/front-page-copy-guard.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/firewall-escape.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/design-decision-gate.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/changeset.lock.yml |
Adds base-SHA activation pin. |
.github/workflows/ai-moderator.lock.yml |
Adds base-SHA activation pin. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The activation checkout hardening regresses the existing ignore-if-missing app fallback path: the same-repo guard is removed even when the runtime credential can still collapse to GITHUB_TOKEN.
Blocking theme
For reusable workflows, resolveActivationToken() already supports ${{ steps.activation-app-token.outputs.token || secrets.GITHUB_TOKEN }}. This change only preserves the same-repo checkout guard for the literal default-token string, so cross-repo workflow_call invocations with intentionally missing app credentials now fail activation instead of safely skipping the callee checkout.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 64.5 AIC · ⌖ 7.16 AIC · ⊞ 20.3K
Comment /review to run again
| // checkout is only attempted for same-repo invocations where GITHUB_TOKEN works. | ||
| // For cross-repo scenarios, users can enable the checkout by configuring | ||
| // activation-github-token or activation-github-app in the workflow frontmatter. | ||
| if activationToken == "${{ secrets.GITHUB_TOKEN }}" { |
There was a problem hiding this comment.
Covered by the activation checkout regression updates in 5e30c00; focused and full repository tests pass.
| // checkout is only attempted for same-repo invocations where GITHUB_TOKEN works. | ||
| // For cross-repo scenarios, users can enable the checkout by configuring | ||
| // activation-github-token or activation-github-app in the workflow frontmatter. | ||
| if activationToken == "${{ secrets.GITHUB_TOKEN }}" { |
There was a problem hiding this comment.
This guard only handles the literal ${{ secrets.GITHUB_TOKEN }} case, so on.github-app.ignore-if-missing: true still drops the same-repo protection even though resolveActivationToken() can fall back to secrets.GITHUB_TOKEN at runtime. In a cross-repo workflow_call with missing app credentials, activation will now try to sparse-checkout the callee repo with a repo-scoped token and fail before prompt generation.
💡 Detect fallback-to-GITHUB_TOKEN here instead of testing exact string equality.
${{ steps.activation-app-token.outputs.token || secrets.GITHUB_TOKEN }} is only cross-repo-capable when the app key is actually present. Please keep the same-repo guard whenever the runtime token may degrade to GITHUB_TOKEN, and add a regression test for the workflow_call + ignore-if-missing path.
There was a problem hiding this comment.
Fixed in 32207bd and extended in 5e30c00. The same-repo guard now applies whenever activation auth can resolve to GITHUB_TOKEN, including ignore-if-missing app fallback and an explicit ${{ secrets.GITHUB_TOKEN }}; regression coverage covers both paths.
|
@copilot Please address the blocking review feedback and continue with the
|
There was a problem hiding this comment.
Reviewed with the impeccable audit/harden lens applied to a non-UI compiler change (PR modifies pkg/workflow/compiler_activation_job.go to pin pull_request activation checkouts to base SHA). No UI surface exists here, so the review focused on correctness, edge-case handling, and test coverage of the refactored trigger-detection logic.
Findings: none blocking.
activationCheckoutRefcorrectly uses structured YAML parsing (onSectionHasTrigger→frontmatterHasTrigger) rather than substring matching, sopull_request_targetdoes not falsely matchpull_request— confirmed by the new"pull_request_target does not match pull_request base checkout pin"test case, and verified independently thatpull_request_targetparses to a distinct map key.nil/emptydata.Oninputs are handled safely (onSectionHasTriggerreturnsfalseon unmarshal of""), matching existing code paths that resetOnto""(e.g. command-trigger handling intrigger_parser.go).- The refactor of
generateCheckoutGitHubFolderForActivation(extractingactivationCheckoutDisabledByActionTag,activationSparseCheckoutExtraPaths) is behavior-preserving; workflow_call path correctly overrides the cross-repo ref only whenpull_requestis also present viaactivationCheckoutRef(data, "steps.resolve-host-repo.outputs.target_checkout_ref"). - Loop-safety fixes (
nameLineIdx >= len(lines),lines[nameLineIdx+1:]) are minor defensive hardening, no regressions found. - Build (
go build ./...),go vet ./pkg/workflow/..., and the fullTestGenerateCheckoutGitHubFolderForActivation_*suite pass, including all new sub-tests forpull_request, mixed triggers,workflow_call + pull_request, andpull_request_target. - The regenerated
.lock.ymlfiles consistently add the expectedref: ${{ github.event_name == 'pull_request' && ... }}line to the activation checkout step; spot-checked several.
No actionable Impeccable-mode findings apply since this PR contains no UI/frontend code. Approving.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 106.5 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs to the security fix in pkg/workflow/compiler_activation_job.go. This is a well-scoped, well-tested change: the root cause (activation checkout using PR-head content before base-branch restoration for pull_request-triggered workflows) is addressed directly, not just its symptom, and regression tests cover pull_request, mixed triggers, workflow_call + pull_request, and pull_request_target (verified pull_request_target correctly does not match via manual repro — frontmatterHasTrigger does exact map-key matching, avoiding substring false-positives).
📋 Key Themes & Highlights
Verification performed
- Ran
go test ./pkg/workflow/... -run TestGenerateCheckoutGitHubFolderForActivation— all pass, including the newpull_request/mixed-trigger/pull_request_targetcases. - Manually reproduced
reaction: eyes+pull_requestand confirmedactivationCheckoutRefcorrectly emits the base-SHA-pinned expression. - Confirmed
slash_command/pull_requestcannot coexist (pre-existing mutual-exclusivity check), so no gap there. gofmt -lclean on both touched Go files.
Minor findings (non-blocking)
firstString()helper is functionally justparts[0]sincestrings.Splitnever returns an empty slice — the indirection obscures trivial behavior (see inline comment).onSectionHasTrigger(the function gating this security fix) uses structured YAML parsing while the neighboringhasWorkflowCallTriggeruses a substring check — inconsistent detection strategies for sibling concerns in the same code path; worth a comment or consolidation given how security-sensitive this branch is (see inline comment).
Positive Highlights
- ✅ Root cause fixed at the source (activation checkout ref), not patched downstream.
- ✅ Correctly preserves
workflow_callcross-repo ref behavior and only overrides it whenpull_requestis the active event (... || fallback). - ✅ Extracted helpers (
activationCheckoutDisabledByActionTag,activationSparseCheckoutExtraPaths,activationCheckoutRef) improve readability over the prior monolithic function. - ✅ Lock-file regeneration is comprehensive (43 files) — no repo workflow was missed.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 173 AIC · ⌖ 15.8 AIC · ⊞ 10.1K
Comment /matt to run again
| return result | ||
| } | ||
|
|
||
| func firstString(values []string) string { |
There was a problem hiding this comment.
[/codebase-design] firstString(parts) always returns parts[0] (since strings.Split never returns an empty slice) — the loop obscures a trivial index access and reads as a general-purpose "find first non-empty string" helper it isn't.
💡 Suggested simplification
Since strings.Split on a non-empty normalized string always yields at least one element, this can just be parts[0] at the call site, dropping the helper entirely (the len(parts) == 0 check above it becomes dead code too, since strings.Split never returns an empty slice for a non-empty input).
topLevel := parts[0]
if topLevel == "" {
continue
}Low-impact, but the added indirection makes the reader hunt for behavior that doesn't exist.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 32207bd: removed firstString and derive the top-level segment inline while retaining bounds-safe validation required by the repository linter.
| compilerActivationJobLog.Print("Adding .github, .agents, and engine-specific dirs to sparse checkout for activation job") | ||
| return cm.GenerateGitHubFolderCheckoutStep("", "", activationToken, c.getActionPin, extraPaths...) | ||
| // onSectionHasTrigger reports whether a rendered on: YAML section contains trigger. | ||
| func onSectionHasTrigger(onSection, trigger string) bool { |
There was a problem hiding this comment.
[/codebase-design] onSectionHasTrigger re-parses the rendered on: YAML with yaml.Unmarshal on every call, duplicating what hasWorkflowCallTrigger does cheaply via strings.Contains a few lines above. Two different detection strategies for sibling concerns (pull_request vs workflow_call) in the same function makes the security-critical branch harder to audit at a glance.
💡 Suggested consolidation
Consider giving hasWorkflowCallTrigger-style detection and onSectionHasTrigger a shared underlying helper (or at least a comment explaining why one uses substring matching and the other does structured YAML parsing) so a future reader trusts both equally when reasoning about the base-SHA pinning security fix.
Functionally this is fine today — frontmatterHasTrigger correctly does exact map-key matching, avoiding e.g. pull_request_target false-matching pull_request — but the inconsistency is worth a note since this is the exact function gating the security fix in this PR.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 32207bd: documented that structured parsing is intentional for security-sensitive exact-key detection, unlike the legacy workflow_call substring check.
|
@copilot run pr-finisher skill |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
|
…n-job-checkout-issue Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran |
|
🎉 Great work on this security hardening! This PR correctly pins the activation job's checkout to the base SHA for The implementation looks solid:
This is ready for review!
|
|
|
Ran |
|
@copilot do a security review of changes, update compiler security spec and docs |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Security review found no confirmed vulnerabilities in the activation checkout changes. In |
|
@copilot review fork combination with this feature |
|
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed the fork interaction: forks are excluded by default; explicitly allowed forks still checkout the target repository at the PR base SHA, never the fork head during activation. Added fork-filter integration coverage in |
The fork follow-up is covered by |
For
pull_request-triggered workflows, the activation job could build runtime imports and install skills from PR-head content before base-branch restoration ran in the agent job. This allowed same-repo PR authors with activation rights to influence the instructions used to review their own PR.pull_requesttrigger.ref:for the activation sparse checkout that uses the PR base SHA on pull request events.workflow_call.Workflow call compatibility
workflow_callactivation checkouts pinned tosteps.resolve-host-repo.outputs.target_checkout_ref.pull_request.Regression coverage
pull_request, mixed triggers,workflow_call + pull_request, andpull_request_targetbehavior.Generated workflow updates
pr-sous-chefbranch refresh requested from run https://github.com/github/gh-aw/actions/runs/36221620757