fix: await runLogParser in async test callbacks#43316
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43316 does not have the 'implementation' label and has only 2 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR fixes flaky/failing JS unit tests by properly awaiting the async runLogParser() helper, and unblocks Go validation by addressing a pre-existing modernize lint issue in ARC/DinD artifact path rewriting.
Changes:
- Updated 12
it()test callbacks inlog_parser_bootstrap.test.cjsto beasyncand toawait runLogParser(...), ensuring assertions run after post-summary side effects (e.g.,core.setFailed(),core.warning()). - Replaced
strings.HasPrefix+strings.TrimPrefixwithstrings.CutPrefixinrewriteTmpGhAwPathsForArcDindto satisfy modernization linting.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/arc_dind_artifacts.go | Modernizes prefix handling via strings.CutPrefix while preserving the existing path rewrite behavior. |
| actions/setup/js/log_parser_bootstrap.test.cjs | Fixes async test timing by awaiting runLogParser() before asserting side effects. |
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: Low
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (12 tests)
Verdict
Note: This PR is a correctness fix — 12 async test callbacks were calling Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — no blocking issues, but three tests have a cleanup-safety problem worth addressing.
📋 Key Themes & Highlights
Key Themes
- Correct async fix: Adding
async/awaitto the 12 test callbacks is exactly the right fix for the race condition. The root cause (synchronous callbacks for an async function) is fully addressed. - Inconsistent test structure: Three tests (lines 113, 130, 310) still use the comma-chain pattern — the same tests that were already tricky to read without
async. After the async fix, if anexpectassertion in those chains throws, cleanup code (file unlinks, env var deletion) is silently skipped. The surrounding tests in this file already use the correcttry/finallypattern — these three should match. - No regression test: The PR description explains the bug clearly, but there is no test that would fail if someone accidentally writes a synchronous
it()callback callingrunLogParserin the future. A JSDoc@returns {Promise<void>}onrunLogParserin the source file would help IDEs surface this immediately. - Go change is clean: The
strings.CutPrefixmodernisation inarc_dind_artifacts.gois a nice, idiomatic improvement.
Positive Highlights
- ✅ Root cause fully addressed across all 12 affected tests
- ✅ PR description is thorough and accurate
- ✅ Consistent adoption of
async/awaitwithout introducing any new patterns - ✅ The Go modernisation reduces double-traversal of the string prefix
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 54.6 AIC · ⌖ 5.63 AIC · ⊞ 6.6K
Comment /matt to run again
| (fs.writeFileSync(logFile, "content"), (process.env.GH_AW_AGENT_OUTPUT = logFile)); | ||
| const mockParseLog = vi.fn().mockReturnValue({ markdown: "## Result\n", mcpFailures: ["server1", "server2"], maxTurnsHit: !1 }); | ||
| (runLogParser({ parseLog: mockParseLog, parserName: "TestParser" }), expect(mockCore.setFailed).toHaveBeenCalledWith(`${ERR_API}: MCP server(s) failed to launch: server1, server2`), fs.unlinkSync(logFile), fs.rmdirSync(tmpDir)); | ||
| (await runLogParser({ parseLog: mockParseLog, parserName: "TestParser" }), |
There was a problem hiding this comment.
[/tdd] Comma-chain async test skips cleanup on failure — if the expect assertion throws, fs.unlinkSync and fs.rmdirSync never run, leaving temp dirs behind.
💡 Suggested refactor (try/finally)
Other tests in this same file (e.g. lines 137–162) already use the safe pattern:
it('should handle MCP failures', async () => {
const tmpDir = fs.mkdtempSync(path.join(__dirname, 'test-'));
const logFile = path.join(tmpDir, 'test.log');
try {
fs.writeFileSync(logFile, 'content');
process.env.GH_AW_AGENT_OUTPUT = logFile;
const mockParseLog = vi.fn().mockReturnValue({ markdown: '## Result\n', mcpFailures: ['server1', 'server2'], maxTurnsHit: false });
await runLogParser({ parseLog: mockParseLog, parserName: 'TestParser' });
expect(mockCore.setFailed).toHaveBeenCalledWith(`${ERR_API}: MCP server(s) failed to launch: server1, server2`);
} finally {
fs.unlinkSync(logFile);
fs.rmdirSync(tmpDir);
}
});The same pattern applies to the tests at lines 130–136 and 305–314.
@copilot please address this.
| const mockParseLog = vi.fn().mockReturnValue({ markdown: "## Result\n", mcpFailures: ["server1"], maxTurnsHit: !1 }); | ||
|
|
||
| (runLogParser({ parseLog: mockParseLog, parserName: "TestParser" }), | ||
| (await runLogParser({ parseLog: mockParseLog, parserName: "TestParser" }), |
There was a problem hiding this comment.
[/tdd] Same comma-chain cleanup hazard as line 113 — env vars and temp files aren't cleaned up if expect throws mid-chain.
💡 Suggested refactor (try/finally)
Use try/finally as the surrounding tests already do (e.g. lines 118–136 already exist cleanly — but the comma-chain equivalent here leaves GH_AW_SAFE_OUTPUTS set on failure, polluting later tests):
try {
// ... setup ...
await runLogParser({ parseLog: mockParseLog, parserName: 'TestParser' });
expect(mockCore.warning).toHaveBeenCalledWith(...);
expect(mockCore.setFailed).not.toHaveBeenCalled();
} finally {
fs.unlinkSync(logFile);
fs.unlinkSync(safeOutputsFile);
delete process.env.GH_AW_SAFE_OUTPUTS;
fs.rmdirSync(tmpDir);
}@copilot please address this.
| (fs.writeFileSync(logFile, "content"), (process.env.GH_AW_AGENT_OUTPUT = logFile)); | ||
| const mockParseLog = vi.fn().mockReturnValue({ markdown: "## Result\n", mcpFailures: [], maxTurnsHit: !0 }); | ||
| (runLogParser({ parseLog: mockParseLog, parserName: "TestParser" }), | ||
| (await runLogParser({ parseLog: mockParseLog, parserName: "TestParser" }), |
There was a problem hiding this comment.
[/tdd] Comma-chain async test: cleanup won't execute if expect throws — same structural issue as lines 113 and 130. The GH_AW_AGENT_OUTPUT env var is also never cleared, which may bleed state into subsequent tests.
💡 Suggested refactor (try/finally)
it('should handle max-turns limit reached', async () => {
const tmpDir = fs.mkdtempSync(path.join(__dirname, 'test-'));
const logFile = path.join(tmpDir, 'test.log');
try {
fs.writeFileSync(logFile, 'content');
process.env.GH_AW_AGENT_OUTPUT = logFile;
const mockParseLog = vi.fn().mockReturnValue({ markdown: '## Result\n', mcpFailures: [], maxTurnsHit: true });
await runLogParser({ parseLog: mockParseLog, parserName: 'TestParser' });
expect(mockCore.setFailed).toHaveBeenCalledWith(`${ERR_VALIDATION}: Agent execution stopped: max-turns limit reached. The agent did not complete its task successfully.`);
} finally {
fs.unlinkSync(logFile);
fs.rmdirSync(tmpDir);
}
});@copilot please address this.
| fs.rmdirSync(tmpDir)); | ||
| }), | ||
| it("should fail Claude runs when no structured log entries are parsed", () => { | ||
| it("should fail Claude runs when no structured log entries are parsed", async () => { |
There was a problem hiding this comment.
[/diagnosing-bugs] The root cause of all 12 failing tests was that runLogParser is async but the test callbacks were synchronous. This PR correctly fixes that, but a comment or a regression-guarding lint rule would prevent future callers from making the same mistake.
💡 Consider a JSDoc comment on runLogParser
Adding a JSDoc @returns {Promise<void>} annotation to runLogParser in log_parser_bootstrap.cjs makes it immediately obvious to the IDE and future test authors that the function must be awaited:
/**
* `@param` {{ parseLog: Function, parserName: string, supportsDirectories?: boolean }} opts
* `@returns` {Promise<void>}
*/
async function runLogParser(opts) { ... }This is a low-cost, permanent documentation of the async contract — exactly the kind of instrumentation /diagnosing-bugs recommends to prevent regression.
@copilot please address this.
There was a problem hiding this comment.
Review: fix: await runLogParser in async test callbacks
The fix is correct and complete. Both changed files are clean.
log_parser_bootstrap.test.cjs
runLogParser is declared async in the source (it awaits core.summary.addRaw().write()), and setFailed/warning calls for MCP failures, max-turns, and Claude guardrails are all issued after that await. Without await in the test callbacks, assertions ran against zero-call mocks. Adding async/await to the 12 affected tests is precisely the right fix.
Tests that remain synchronous (early-return paths and assertions on state set before the await) are correctly left unchanged.
pkg/workflow/arc_dind_artifacts.go
strings.CutPrefix is the idiomatic modern replacement for HasPrefix + TrimPrefix. The rewrite is semantically equivalent and the modernize lint fix is legitimate.
Minor observation (non-blocking)
Tests at lines 54 and 80 (should handle parser returning object with markdown, should generate plain text summary) still call runLogParser without await. They only assert on state set before the inner await write() call, so they pass today — but if logic ever moves after the await they will silently become flaky. Consider adding async/await as a consistency measure in a follow-up.
✅ No blocking issues. Approving.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 75.7 AIC · ⌖ 6.32 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Non-blocking: 3 tests missing try/finally cleanup
The async/await fix is correct and the Go strings.CutPrefix modernization is clean. One low-severity pattern to fix:
Details
Three tests updated in this PR use a comma-expression cleanup pattern without try/finally:
should handle MCP failures(line ~108)should warn instead of failing MCP failures when safe outputs exist(line ~118)should handle max-turns limit reached(line ~305)
If the await runLogParser() call rejects, or if any expect() assertion throws, the cleanup (fs.unlinkSync, fs.rmdirSync) in the comma expression is skipped — leaving temp directories behind. Every other async test fixed in this PR correctly uses try/finally for cleanup; these three are outliers.
This won't cause test failures in the happy path, hence non-blocking, but it will produce mysterious leftover test- directories if a test fails mid-run.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 141 AIC · ⌖ 6.36 AIC · ⊞ 5.4K
Comment /review to run again
| (await runLogParser({ parseLog: mockParseLog, parserName: "TestParser" }), | ||
| expect(mockCore.setFailed).toHaveBeenCalledWith(`${ERR_API}: MCP server(s) failed to launch: server1, server2`), | ||
| fs.unlinkSync(logFile), | ||
| fs.rmdirSync(tmpDir)); |
There was a problem hiding this comment.
Temp-dir leak if assertion throws: cleanup runs in a comma expression with no try/finally, so if expect(mockCore.setFailed) throws, fs.unlinkSync and fs.rmdirSync are never reached.
💡 Suggested fix
Refactor to use try/finally for cleanup, consistent with the majority of tests in this file:
it("should handle MCP failures", async () => {
const tmpDir = fs.mkdtempSync(path.join(__dirname, "test-"));
const logFile = path.join(tmpDir, "test.log");
try {
fs.writeFileSync(logFile, "content");
process.env.GH_AW_AGENT_OUTPUT = logFile;
const mockParseLog = vi.fn().mockReturnValue({ markdown: "## Result\n", mcpFailures: ["server1", "server2"], maxTurnsHit: false });
await runLogParser({ parseLog: mockParseLog, parserName: "TestParser" });
expect(mockCore.setFailed).toHaveBeenCalledWith(`${ERR_API}: MCP server(s) failed to launch: server1, server2`);
} finally {
fs.unlinkSync(logFile);
fs.rmdirSync(tmpDir);
}
});The same issue exists in the "should warn instead of failing MCP failures when safe outputs exist" test (~line 128) and "should handle max-turns limit reached" test (~line 308). All three were touched by this PR and are outliers — every other updated test already uses try/finally.
12 tests in
log_parser_bootstrap.test.cjswere failing because theit()callbacks were synchronous butrunLogParseris async.core.setFailed()andcore.warning()are invoked afterawait core.summary.addRaw(...).write()insiderunLogParser, so they hadn't been called by the time assertions ran — yielding "Number of calls: 0" for all 12.Changes
log_parser_bootstrap.test.cjs— made the 12 affectedit()callbacksasyncand addedawaitbefore eachrunLogParser()call:pkg/workflow/arc_dind_artifacts.go— fixed a pre-existingmodernizelint failure (HasPrefix+TrimPrefix→strings.CutPrefix) that blocked validation.