Repository navigation
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
❌ PR Specification Reviewer was cancelled. Please review the logs for details. PR Specification ReviewerOutcome: The agent emitted a no-op output, but successful task completion is unconfirmed. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Warning Threat detection engine error Threat detection could not complete its analysis due to a tooling error. This is not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs to troubleshoot the error. 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Threat detection engine error Threat detection could not complete its analysis due to a tooling error. This is not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs to troubleshoot the error. Design Decision Gate 🏗️Outcome: 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.
|
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 test-quality issue: the new executable-path regression test passes even without the production fix because its fake uv is not on PATH. The inline comment explains the required correction. Other candidate concerns were either pre-existing behavior or covered by existing tests.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| } | ||
| t.Setenv("GH_AW_TEST_UV_ARGS_FILE", argsFile) | ||
|
|
||
| err := NewCompiler().validateUvPackagesWithExecutable([]string{"requests"}, "uv") |
There was a problem hiding this comment.
This regression test does not actually exercise the unpatched behavior: the fake executable is written under tempDir, but tempDir is never added to PATH, while the call passes the relative name "uv". Before this change, exec.Command("uv", ...) would simply fail to start and leave argsFile absent, so the os.IsNotExist assertion would still pass. Add t.Setenv("PATH", tempDir+string(os.PathListSeparator)+os.Getenv("PATH")) (or assert the specific validation error) so the test fails without the fix.
There was a problem hiding this comment.
Updated the regression test to change into the fixture directory and invoke ./uv, so the fake executable runs if the path guard is removed. Commit: daa618b.
There was a problem hiding this comment.
🟡 Changes recommended
The regression test can falsely pass without the path-validation guard because the fake uv executable is not on PATH, allowing command lookup failure to mask the regression.
1 open issue
What changed in this PR
This PR closes the UV executable-path validation gap in gh-aw’s compiler package lookup while preserving existing package-name checks and argument protections.
Changes:
- Validate the UV executable path before execution and use the validated result.
- Add a regression test for rejecting relative executable paths before execution.
| File | Description |
|---|---|
pkg/workflow/pip_validation.go |
Validates the UV executable path while retaining fixed arguments and the option terminator. |
pkg/workflow/argument_injection_test.go |
Adds a fake-UV test intended to verify rejection before execution. |
Review effort: Max (auto)
| if err := os.WriteFile(uvPath, []byte(script), 0o755); err != nil { | ||
| t.Fatalf("failed to write fake uv executable: %v", err) | ||
| } | ||
| t.Setenv("GH_AW_TEST_UV_ARGS_FILE", argsFile) |
There was a problem hiding this comment.
Updated the regression test to change into the fixture directory and invoke ./uv, so the fake executable runs if the path guard is removed. Commit: daa618b.
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs. The production change is sound and I verified it. The new regression test is vacuous, so requesting changes.
📋 Key Themes & Highlights
Key Themes
- Regression test does not guard the fix:
TestValidateUvPackagesWithExecutable_RejectsUnvalidatedExecutablePathpasses the bare nameuv, not the fake executable, so it passes on the pre-fix code too.
Verification
go test ./pkg/workflow -run TestValidateUvPackagespasses.- A probe that runs a relative
./uvfails on the pre-fix code and is rejected with the fix. This confirms the production change works. The probe was temporary and has been removed.
Positive Highlights
- ✅ Validating at the invocation boundary, and passing the canonical path returned by
ValidateExecutablePathtoexec.Command, is the right defence-in-depth. - ✅ Fixed arguments and the
--option terminator are preserved.
@copilot please address the review comment above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~3 AIC · ⌖ ~2 AIC · ⊞ 6.3K
Comment /matt to run again
| } | ||
| t.Setenv("GH_AW_TEST_UV_ARGS_FILE", argsFile) | ||
|
|
||
| err := NewCompiler().validateUvPackagesWithExecutable([]string{"requests"}, "uv") |
There was a problem hiding this comment.
[/tdd] [/diagnosing-bugs] This regression test does not exercise the fix: it passes the bare name "uv" instead of the fake executable uvPath. The fake script is never run, so the test also passes against the pre-fix pip_validation.go. I confirmed this by reverting the pip_validation.go hunk and re-running it.
💡 Suggested change
Chdir into the temp dir and pass a relative path, so the new check is what rejects it:
t.Setenv("GH_AW_TEST_UV_ARGS_FILE", argsFile)
t.Chdir(tempDir)
err := NewCompiler().validateUvPackagesWithExecutable([]string{"requests"}, "./uv")With this change the test fails on the pre-fix code (the relative ./uv runs and writes the args file) and passes with the fix.
@copilot please address this.
There was a problem hiding this comment.
Updated the regression test to change into the fixture directory and invoke ./uv; the unpatched implementation now runs the fake binary and makes the test fail. Commit: daa618b.
|
@copilot run pr-finisher skill, fix ci failures |
|
🍳 A little prep to help this PR along! Copilot assignment requested for the following outstanding work:
Keeping the changes focused; human review and CI get the final taste test. No merging or CI triggering. Structured data{
"completed_task": 0,
"head": "0e4bb063494678e1e630b6ea430022d828b10f1d",
"kind": "requested",
"schema_version": "1",
"state": "60092bb45253b9138cc34d7c4de4d17de243a234c7e742e0c8979be14be5f165",
"work": [
"1027833686a7050a64d8bfe136387980a34b94be7e9b120f51fd825652824672",
"1d1e0bc12378a092e2b8f34e7e1aa1da733b42f3805a79c1e024be01117338c1",
"20c5fb9e2e37672daf5b7271dae9c95ff59f03cf41d0cde5ac7e2acc9456d2d1",
"b3128910de978c3d2b1a79eae53581873ac33f4899c1fe9917d2fc310d7654ab"
],
"workflow": "pr-sous-chef"
}
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The setup failed while downloading
The regression test now executes its fake |
…curity-findings-again Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Merged the latest |
|
Warning Threat detection engine error Threat detection could not complete its analysis due to a tooling error. This is not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs to troubleshoot the error. 🍳 A little prep to help this PR along! Copilot assignment requested for the following outstanding work:
Keeping the changes focused; human review and CI get the final taste test. No merging or CI triggering. Structured data{
"completed_task": 0,
"head": "5af9005241ceaffac282967d268861298f4a536e",
"kind": "requested",
"schema_version": "1",
"state": "ba2923f5de66d50bf3e61030d891f8b6f05e94642d405686219b4647b6ccbeed",
"work": [
"20c5fb9e2e37672daf5b7271dae9c95ff59f03cf41d0cde5ac7e2acc9456d2d1",
"42e5d572d9d75b7168fe7d0aeda0717cc07e65ed240f6d299e6fec661214f1ac",
"960a6fe297781e0f1e1c8c03fa3740110735e1332e99a040b5aa7b1b6d14a3f2"
],
"workflow": "pr-sous-chef"
}
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed the impacted Go test by updating its expected step name to match the workflow source;
Merged latest |
|
🍳 The prep is done, but CI needs another taste test. Copilot's assigned task has finished; these checks are still failing: Completed task
Flagging the remaining blockers for maintainer investigation, not starting another copy of the same task. Structured data{
"completed_task": 42584642554,
"head": "fcedaeb4adbff9a7ac7ec97e61b40e3f44ea7b73",
"kind": "ci-follow-up",
"schema_version": "1",
"state": "e878ea6f5825173f878cdc50c034fae541ab21afcda61e90b2e02b064519321f",
"work": [
"42e5d572d9d75b7168fe7d0aeda0717cc07e65ed240f6d299e6fec661214f1ac"
],
"workflow": "pr-sous-chef"
}
|
Reported findings covered pip/uv package lookups and Pi sub-agent execution. This change closes the UV helper’s executable-path validation gap while preserving the existing argument and input protections.
shell: falseand sends task text through stdin.