Repository navigation
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
This comment has been minimized.
This comment has been minimized.
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (101 new lines in 📄 Draft ADR committed:
📋 What to do next
Once the ADR is linked, this gate will re-run and check that the implementation matches the decision. 📋 Michael Nygard ADR Format ReferenceAn ADR needs four sections to be complete:
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found one blocking correctness issue: the new identity check fails no-checkout/no-origin workflows that previously skipped authentication. See the inline comment for a fail-closed gating fix and regression coverage.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd to the identity check and the compiled wiring. Requesting changes on four items: an empty-target gap, a token left in origin on failure, a fail-closed change for non-git workspaces, and stale lock files.
📋 Key Themes & Highlights
Key Themes
- Error-path credential exposure: a failed post-auth check leaves the token in
.git/config - Boundary values: an explicitly empty target passes the identity check
- Behavior change: a no-
.gitworkspace with an explicit target now fails instead of skipping - Generated output:
GH_AW_GIT_REPOSITORYis not in any committed lock file
Positive Highlights
- ✅
${GH_AW_GIT_REPOSITORY-...}(single dash) honors an explicit empty value rather than falling back to the reservedGITHUB_REPOSITORY - ✅ The identity check covers both fetch and push URLs, and the post-auth check does fail (rc=1) on a
pushInsteadOfrewrite, as the test expects - ✅
cd -- ...in a subshell handles hyphen-prefixed paths without leaking the working directory
Scope
Findings come from direct probes of configure_git_credentials.sh run with env -i in temp directories. I did not run the JS test suite, so test status is unverified.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~12 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
Credential checks permit token exposure over plaintext HTTP and incorrectly reject valid trial checkouts, blocking jobs that persist state.
6 open issues
-before changing directories · New
What changed in this PR
This PR hardens gh-aw’s credential setup to preserve cross-repository checkout identity, addressing the failure scenario in #73023 without claiming a confirmed cause for the intermittent reports.
Changes:
- Pass the intended repository through
GH_AW_GIT_REPOSITORYinstead of overriding a runner-reserved variable. - Validate effective fetch and push identities before and after authentication, including safe-output sub-checkouts.
- Add regression coverage, refresh golden fixtures, and document the central-workflow pattern and recompilation requirement.
| File | Description |
|---|---|
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden |
Refresh credential environment in the imports fixture. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Refresh credential environment in the smoke fixture. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden |
Refresh credential environment in the Playwright fixture. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/custom-safe-output-max-length.golden |
Refresh credential environment in the safe-output fixture. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden |
Refresh credential environment in the basic fixture. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden |
Update Pi engine credential expectations. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden |
Update Gemini engine credential expectations. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden |
Update Copilot engine credential expectations. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden |
Update Codex engine credential expectations. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden |
Update Claude engine credential expectations. |
pkg/workflow/git_configuration_steps.go |
Emit explicit current and root checkout identities. |
pkg/workflow/git_config_test.go |
Cover external root and GitHub App checkout identities. |
pkg/workflow/compiler_safe_outputs_steps.go |
Pass root and trial identities to credential generation. |
pkg/workflow/compiler_safe_outputs_steps_test.go |
Update safe-output credential assertions. |
pkg/workflow/checkout_step_generator.go |
Support an explicit root identity for credential steps. |
pkg/workflow/checkout_manager_test.go |
Verify helper-based sub-checkout authentication rendering. |
pkg/workflow/checkout_credentials_render.go |
Route sub-checkout authentication through the shared helper. |
docs/src/content/docs/reference/checkout.md |
Explain central workflows, identity validation, and recompilation. |
actions/setup/sh/configure_git_credentials.sh |
Validate effective remote identities with credential-safe diagnostics. |
actions/setup/js/configure_git_credentials.test.cjs |
Add identity, URL mismatch, rewrite, and legacy-caller coverage. |
Review effort: Max (auto)
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 0441942
|
Preserve no-checkout behavior, validate secure remote identities without Node, restore origin on failed authentication checks, and use each checkout's effective repository identity. Regenerate compiled workflows and add regressions. Validation: focused Go and 41 credential-helper tests passed, standard Go/JS/shell lint passed, and workflow drift passed. The repository gate reports 28 existing custom-linter findings on unchanged lines. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fail explicitly for untrusted checkout configuration instead of treating it as a missing origin. Match trial-mode checkout identity when no logical repository is specified. Add reproducing regressions and update checkout documentation. Validation: 42 credential-helper tests and focused Go identity/WASM tests passed; standard lint and full workflow drift passed. The aggregate gate remains blocked by pre-existing custom-linter findings on unchanged lines. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Matt Pocock Skills ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
Keep the untrusted-checkout regression deterministic when runner system or environment Git configuration trusts all directories. Preserve the refusal, credential-redaction, and unchanged-config assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
External
current: truecheckouts intermittently report the workflow repository asorigindespite retaining the target commit. Credential setup relied on overriding runner-reservedGITHUB_REPOSITORY; this is a plausible failure path, not a confirmed explanation of the intermittent reports.GH_AW_GIT_REPOSITORY, including workspace-root and trial checkouts.