Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
No ADR enforcement needed: PR #51174 does not have the 'implementation' label and has only 42 new lines of code in business logic directories (threshold: 100). |
|
|
|
|
There was a problem hiding this comment.
Pull request overview
Fixes cached Copilot CLI activation so absolute-path harness invocations work reliably.
Changes:
- Always installs a wrapper at the configured Copilot path.
- Adds regression coverage for toolcache hits with
GITHUB_PATH.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/install_copilot_cli.sh |
Installs the cached CLI wrapper unconditionally. |
pkg/cli/install_copilot_cli_test.go |
Verifies path export and wrapper installation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-tested. Removing the early return 0 ensures the wrapper is always installed for absolute-path invocations in the containerized harness, while still exporting GITHUB_PATH for later steps. Tests are appropriately extended to assert wrapper installation in the toolcache path.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.8 AIC · ⊞ 5.5K
Test Quality Analysis — PR #51174SummaryThis PR modifies existing tests in Modified Tests
Quality Analysis✅ Strengths
Test Verification
ScoringClassification: Test Enhancement (0 new test functions; existing tests strengthened)
Score: N/A — Infrastructure Enhancement (no new tests added) Recommendation✅ APPROVE — Targeted assertion additions that strengthen existing tests. Changes demonstrate good testing discipline with no violations and minimal test inflation.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving.
📋 Key Themes & Highlights
Root cause & fix
The early return 0 in activate_cached_copilot_bin() prevented wrapper installation whenever GITHUB_PATH was set. The containerized AWF harness invokes Copilot by absolute path (/usr/local/bin/copilot), so cached toolcache hits silently left that path absent. The fix is minimal and correctly addresses the root cause rather than patching the symptom.
Positive Highlights
- ✅ Root cause addressed directly — one-line removal with a clear explanatory comment
- ✅ Regression test extended to assert both
GITHUB_PATHexport and wrapper installation on a cache hit - ✅ Test verifies wrapper content delegates to the cached binary via
exec - ✅ Updated comment explains why the wrapper is needed even when
GITHUB_PATHis set
Minor Observation (inline comment)
The sudo install stub is repeated in three test functions — extracting a helper would make future changes easier.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.6 AIC · ⊞ 7.1K
Comment /matt to run again
| echo "install:${mode}:${dest}" >> "`+installLog+`" | ||
| cp "$src" "`+installedWrapper+`" | ||
| chmod "$mode" "`+installedWrapper+`" | ||
| exit 0 |
There was a problem hiding this comment.
[/tdd] The sudo install stub is duplicated across three test functions. Not a blocker, but a shared helper would reduce future maintenance burden if the install invocation signature changes.
💡 Suggestion
Extract a fakeSudoScript(installLog, installedWrapper string) string helper and reference it from all three tests.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 3a7203a by extracting the shared fakeSudoScript(...) helper and reusing it across the cached-install test setups.
|
$ Please refresh the branch, address any remaining review feedback, run the
Run: https://github.com/github/gh-aw/actions/runs/31215510357
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 3a7203a. I extracted the duplicated sudo-install test helper and fixed the unrelated |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
|
✅ Smoke OTEL completed successfully!
|
|
✅ Smoke Copilot MAI completed successfully!
|
|
Cached path forgets Inspired by the fix ensuring the Copilot CLI wrapper installs at an absolute path even on cached toolcache hits.
|
Agent Container Tool Check
Result: 12/12 tools available ✅ — PASS
|
Smoke Test: Gemini - 31231217590
Overall Status: FAIL
|
|
Smoke test: PR ✅ Web ❌ File ✅ Bash ✅ Build ❌ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
Smoke test summary: workflow and engine improvements, docs/tests, and dependency updates dominated the last 24h.
|
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. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
Commit pushed:
|
🤖 Smoke Test Results — Run 31231217508Core Tests: PR Review Tests: Overall: PARTIAL Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! Run 31231217508
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · sonnet46 · 74 AIC · ⌖ 18.8 AIC · ⊞ 768
Comment /smoke-claude to run again
Comments that could not be inline-anchored
pkg/cli/install_copilot_cli_test.go:55
🤖 Smoke Test Review Comment #1 — The test coverage here looks solid. Consider adding edge cases for concurrent toolcache access. (Run 31231217508)
actions/setup/sh/install_copilot_cli.sh:10
🤖 Smoke Test Review Comment #2 — Good defensive coding here. The ENOENT fix ensures absolute-path invocations work after a toolcache hit. (Run 31231217508)
Smoke Test SummaryPR: Fix cached Copilot CLI wrapper activation 1 gh-proxy OK | 2 mcpscripts-gh OK | 3 serena FAIL(timeout) | 4 playwright OK | 5 web-fetch OK | 6 file+bash OK | 7 discussion OK | 8 build OK | 9 artifact OK | 10 discussion-create OK | 11 dispatch OK | 12 PR review OK | 13 comment-memory OK | 14 file-summarizer FAIL | 15 check-run OK | 16 LSP OK Overall: FAIL (tests 3, 14 failed) Author: app/copilot-swe-agent - Assignees: pelikhan, Copilot Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
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. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke test: no diff hunks available for inline comments in this PR context.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · auto · 43.6 AIC · ⌖ 3 AIC · ⊞ 8.7K
Comment /smoke-copilot to run again
Add label smoke to run again
A Copilot CLI toolcache hit only updated
PATH/GITHUB_PATH, leaving/usr/local/bin/copilotabsent. The compiled AWF harness invokes Copilot by absolute path, so cached installs could fail withENOENTinside the container.${INSTALL_DIR}/copilotwrapper for cached Copilot CLI activations, including GitHub Actions runs whereGITHUB_PATHis set.GITHUB_PATHand installs/usr/local/bin/copilot.Run: https://github.com/github/gh-aw/actions/runs/31215510357> Generated by 👨🍳 PR Sous Chef · gpt54 · 5.84 AIC · ⊞ 6.1K · ◷
✨ PR Review Safe Output Test - Run 31231217508> [!WARNING]