Fix full local Go and JavaScript test suites - #55559
Conversation
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality 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. No ADR enforcement needed: PR #55559 does not have the implementation label and has only 9 new lines of code in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
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.
Verdict
COMMENT — I did not find a changed-line defect here that clearly warrants blocking the PR.
Highlights
I checked the Makefile/test wiring, the AWF reflect skip path, the harness test env changes, the glob compatibility fix, the safe-outputs temp-path handling, the installer test sudo stub, and the change-scoped custom Go lint filtering.
The only suspicious item was the custom-lint filter in scripts/agent-report-progress.sh, but the new logic still preserves non-file analyzer failures and still fails when any diagnostic hits a changed Go file, so I don't have a concrete correctness break to pin on this diff.
The background grumpy-coder helper failed to produce usable output, so this review is based on my own pass only.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 7.37 AIC · ⌖ 6.9 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
This pass focused on over-engineering only. The main simplification I’d cut is the new diagnostic parser in the custom Go lint gate; it adds a lot of shell-state handling for a narrow use case. net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #55559 · codex · mai10 · 7.76 AIC · ⌖ 2.06 AIC · ⊞ 16.7K
Comment /ponytail to run again
There was a problem hiding this comment.
Review: Fix full local Go and JavaScript test suites
All changes are targeted, correct, and improve test reliability. Key fixes:
generate_usage_activity_summary.cjs:globSyncwas incorrectly imported fromnode:fs(which has no such export). The fix to use theglobpackage is correct.awf_reflect.cjs: Making the timeout configurable viaGH_AW_REFLECT_TIMEOUT_MSis a clean testability improvement.- Test isolation (
add_comment.test.cjs,notify_comment_error.test.cjs): ProperbeforeAll/afterAllsave-and-restore ofGH_AW_PROMPTS_DIRprevents cross-test contamination. GH_AW_SKIP_REFLECT: "true"in harness test child envs prevents unintended network calls during unit tests.- Makefile: Wiring
eslint-factoryintotest-jsanddepsensures the new test suite runs in CI.
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 32.5 AIC · ⌖ 9.1 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on three correctness issues and one test hermetic concern.
📋 Key Themes & Highlights
Key Issues
- Path normalisation gap (
scripts/agent-report-progress.shline 194):golangci-lintsometimes emits./pkg/...paths;go_files[]holds barepkg/...paths. The string equality check silently suppresses changed-file diagnostics when they don't match — the main risk in this PR. - Implicit
return 0(scripts/agent-report-progress.shline 938): control flow falls through implicitly to success; the intent should be explicit to survive future edits. NaNtimeout (actions/setup/js/awf_reflect.cjsline 35):Number.parseInt("")isNaN; an empty env var makes every reflect call time out immediately with no visible error.- Hermetic gap (
notify_comment_error.test.cjsline 65): setsGH_AW_PROMPTS_DIRto a relative../mdpath instead of usingsyncRuntimePromptTemplates()like the sibling test does; will break ifcwd()differs.
Positive Highlights
- ✅
GH_AW_SKIP_REFLECTbypass is clean, well-tested, and makes subprocess tests genuinely offline. - ✅
harnessChildEnvconstant cleanly DRYs up the env setup across all harness test call-sites. - ✅
safe_outputs_mcp_server_defaults.test.cjstemp-dir creation/cleanup pattern is correct and complete. - ✅ Switching
globSyncto theglobnpm package (away fromnode:fs) is the right fix for Node 20 compatibility. - ✅
make test-jsnow coverseslint-factory— good gap to close.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 60.5 AIC · ⌖ 10.5 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
scripts/agent-report-progress.sh:194
[/diagnosing-bugs] Path comparison between linter output and go_files[] is fragile — if the linter emits ./pkg/foo/bar.go but go_files holds pkg/foo/bar.go (or vice versa), the match silently fails and all diagnostics for changed files are suppressed, making the change-scoped gate useless.
<details>
<summary>💡 Suggested fix</summary>
Strip a leading ./ from both sides before comparing:
local normalised_diag="${diagnostic_file#./}"
for changed_file in "${go_files[@]…
</details>
<details><summary>scripts/agent-report-progress.sh:938</summary>
**[/diagnosing-bugs]** When `diagnostic_found=1` but no diagnostic matches a changed file, the function prints the exit message but returns `0` (implicit). The linter failure is silently swallowed — callers have no way to know whether the gate was a no-op skip or a genuine pass.
<details>
<summary>💡 Suggested fix</summary>
The final condition should still return the original status when no diagnostics matched but the linter exited non-zero for other reasons (e.g. config errors, analyzer pani…
</details>
<details><summary>actions/setup/js/notify_comment_error.test.cjs:65</summary>
**[/tdd]** `notify_comment_error.test.cjs` sets `GH_AW_PROMPTS_DIR` to a hard-coded relative path (`path.join(process.cwd(), "../md")`), while the sibling `add_comment.test.cjs` uses the proper `syncRuntimePromptTemplates()` helper that creates a stable, isolated copy. The inconsistency means this test still depends on the layout of the source tree and will break if `cwd()` differs between environments.
<details>
<summary>💡 Suggested fix</summary>
Apply the same hermetic pattern used in `add…
</details>
<details><summary>actions/setup/js/awf_reflect.cjs:35</summary>
**[/diagnosing-bugs]** `GH_AW_REFLECT_TIMEOUT_MS` falls back to `"60000"` but `Number.parseInt` returns `NaN` for an empty string (`process.env.GH_AW_REFLECT_TIMEOUT_MS = ""`). `NaN` ms silently makes every `AbortController` timeout fire immediately, causing all reflection calls to fail without an obvious error message.
<details>
<summary>💡 Suggested fix</summary>
Guard against NaN:
```js
const _rawTimeout = Number.parseInt(process.env.GH_AW_REFLECT_TIMEOUT_MS || "60000", 10);
const AWF_REF…
</details>
<details><summary>actions/setup/js/awf_reflect.test.cjs:116</summary>
**[/tdd]** The new `fetchAWFReflect` disable test (line 668) asserts `fetchMock` was not called — good. But there is no test that verifies the **log message** is emitted when `GH_AW_SKIP_REFLECT` is absent (i.e. the normal path still logs its `fetching ...` prefix). Without that, a future refactor that accidentally gates the normal-path log behind the same env check would go undetected.
<details>
<summary>💡 Why it matters</summary>
The `"disabled"` reason code is a new signal for callers (e.…
</details>There was a problem hiding this comment.
Pull request overview
Improves reliability and completeness of local Go and JavaScript test suites.
Changes:
- Makes subprocess and prompt-dependent tests faster and hermetic.
- Runs dependency installation and tests for both JavaScript projects.
- Adds change-scoped custom Go lint filtering and Node 20 glob support.
Show a summary per file
| File | Description |
|---|---|
scripts/agent-report-progress.sh |
Filters custom linter diagnostics by changed files. |
pkg/cli/install_copilot_cli_test.go |
Stubs sudo in installer testing. |
Makefile |
Includes eslint-factory dependencies and tests. |
actions/setup/js/safe_outputs_mcp_server_defaults.test.cjs |
Provides a temporary runner directory. |
actions/setup/js/package.json |
Adds the glob dependency. |
actions/setup/js/package-lock.json |
Locks updated JavaScript dependencies. |
actions/setup/js/notify_comment_error.test.cjs |
Configures and restores the prompts directory. |
actions/setup/js/generate_usage_activity_summary.cjs |
Replaces the Node filesystem glob implementation. |
actions/setup/js/copilot_harness.test.cjs |
Accelerates and isolates harness subprocesses. |
actions/setup/js/claude_harness.test.cjs |
Accelerates and isolates harness subprocesses. |
actions/setup/js/awf_reflect.test.cjs |
Tests disabled AWF reflection. |
actions/setup/js/awf_reflect.cjs |
Adds reflection disabling and timeout configuration. |
actions/setup/js/add_comment.test.cjs |
Uses hermetic runtime prompt templates. |
Review details
Files not reviewed (1)
- actions/setup/js/package-lock.json: Generated file
- Files reviewed: 12/13 changed files
- Comments generated: 2
- Review effort level: Balanced
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32795790586
|
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>
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>
Addressed in
Validated with targeted JS/Go tests, |
Summary
sudoin the Copilot installer testundefined/directory in the checkoutValidation
make depsmake buildmake build-jsmake test-unit-allmake test-jsmake lint-cjsmake agent-report-progress