Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/36652c5b-9bcd-47c8-82c8-9bc21ab30538 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/36652c5b-9bcd-47c8-82c8-9bc21ab30538 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/36652c5b-9bcd-47c8-82c8-9bc21ab30538 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
pelikhan
April 23, 2026 03:47
View session
pelikhan
approved these changes
Apr 23, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability of the activity_report maintenance flow by adding explicit timeouts and process cleanup for log downloads, and wiring a step-level timeout into generated maintenance workflows.
Changes:
- Added
timeout-minutes: 20to the “Generate agentic workflow activity report” step in both generated and checked-in maintenance workflows. - Updated
run_activity_report.cjsto run log downloads with a timeout, record the spawned PID, and attempt SIGTERM/SIGKILL cleanup; added a post-download settle delay. - Updated Go and JS tests to assert the new workflow timeout wiring and updated runner injection.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/side_repo_maintenance_integration_test.go | Asserts side-repo generated workflow includes the activity report step timeout. |
| pkg/workflow/side_repo_maintenance.go | Adds step-level timeout to the generated side-repo maintenance workflow. |
| pkg/workflow/maintenance_workflow_yaml.go | Adds step-level timeout to the generated main maintenance workflow. |
| pkg/workflow/maintenance_workflow_test.go | Extends workflow generation test assertions for the new step timeout. |
| actions/setup/js/run_activity_report.test.cjs | Updates tests to inject a custom command runner/sleep function and assert timeout plumbing. |
| actions/setup/js/run_activity_report.cjs | Implements timeout-controlled spawning, PID logging, termination escalation, and settle delay. |
| .github/workflows/agentics-maintenance.yml | Applies the new step timeout to the checked-in workflow. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/run_activity_report.cjs:160
- The SIGKILL escalation timer created inside the timeout handler isn't stored/cleared. If the process exits after SIGTERM, this pending 5s timer can keep the Node event loop alive unnecessarily (and delay step completion) on timeout paths. Store the timer handle and clear it when the child settles, or
unref()the timer so it doesn’t hold the process open.
setTimeout(() => {
if (settled) {
return;
}
core.warning(`gh aw PID ${childPid} still running; sending SIGKILL`);
try {
process.kill(childPid, "SIGKILL");
} catch (error) {
core.warning(`Could not SIGKILL PID ${childPid}: ${getErrorMessage(error)}`);
}
}, 5000);
}, timeoutMs);
- Files reviewed: 7/7 changed files
- Comments generated: 5
Comment on lines
+45
to
+46
| const timeoutMs = options.timeoutMs || LOG_DOWNLOAD_TIMEOUT_MS; | ||
| const settleDelayMs = options.settleDelayMs || POST_DOWNLOAD_SETTLE_DELAY_MS; |
There was a problem hiding this comment.
Option defaulting uses ||, which prevents explicitly passing 0 (e.g., settleDelayMs: 0 to disable the settle wait in tests or future callers). Prefer nullish-coalescing (??) for timeoutMs/settleDelayMs so valid falsy values aren’t overridden.
This issue also appears on line 148 of the same file.
Suggested change
| const timeoutMs = options.timeoutMs || LOG_DOWNLOAD_TIMEOUT_MS; | |
| const settleDelayMs = options.settleDelayMs || POST_DOWNLOAD_SETTLE_DELAY_MS; | |
| const timeoutMs = options.timeoutMs ?? LOG_DOWNLOAD_TIMEOUT_MS; | |
| const settleDelayMs = options.settleDelayMs ?? POST_DOWNLOAD_SETTLE_DELAY_MS; |
Comment on lines
+58
to
+77
| it("creates an activity report issue with 24h and 7d time ranges", async () => { | ||
| mockExec.getExecOutput.mockResolvedValueOnce({ stdout: "## 24h report\nok", stderr: "", exitCode: 0 }).mockResolvedValueOnce({ stdout: "## 7d report\nok", stderr: "", exitCode: 0 }); | ||
| mockCommandRunner.mockResolvedValueOnce({ stdout: "## 24h report\nok", stderr: "", exitCode: 0 }).mockResolvedValueOnce({ stdout: "## 7d report\nok", stderr: "", exitCode: 0 }); | ||
|
|
||
| const { main } = await import("./run_activity_report.cjs"); | ||
| await main(); | ||
| await main({ commandRunner: mockCommandRunner, sleepFn: mockSleepFn, settleDelayMs: 1 }); | ||
|
|
||
| expect(mockExec.getExecOutput).toHaveBeenCalledTimes(2); | ||
| expect(mockExec.getExecOutput).toHaveBeenNthCalledWith( | ||
| expect(mockCommandRunner).toHaveBeenCalledTimes(2); | ||
| expect(mockCommandRunner).toHaveBeenNthCalledWith( | ||
| 1, | ||
| "gh", | ||
| expect.arrayContaining(["aw", "logs", "--repo", "testowner/testrepo", "--start-date", "-1d", "--count", "1000", "--output", "./.cache/gh-aw/activity-report-logs", "--format", "markdown"]), | ||
| expect.objectContaining({ ignoreReturnCode: true }) | ||
| 1200000 | ||
| ); | ||
| expect(mockExec.getExecOutput).toHaveBeenNthCalledWith( | ||
| expect(mockCommandRunner).toHaveBeenNthCalledWith( | ||
| 2, | ||
| "gh", | ||
| expect.arrayContaining(["aw", "logs", "--repo", "testowner/testrepo", "--start-date", "-1w", "--count", "1000", "--output", "./.cache/gh-aw/activity-report-logs", "--format", "markdown"]), | ||
| expect.objectContaining({ ignoreReturnCode: true }) | ||
| 1200000 | ||
| ); | ||
| expect(mockSleepFn).toHaveBeenCalledTimes(2); |
There was a problem hiding this comment.
The updated tests validate that main() passes a timeout value into the injected runner, but they don’t exercise the new timeout/cleanup behavior (SIGTERM then SIGKILL after 5s) introduced in runCommandWithTimeout. Add focused unit tests around runCommandWithTimeout using fake timers and mocking child_process.spawn/process.kill so regressions in the timeout/cleanup logic get caught.
| ${{ runner.os }}-activity-report-logs- | ||
| `) | ||
| yaml.WriteString(` - name: Generate agentic workflow activity report | ||
| timeout-minutes: 20 |
There was a problem hiding this comment.
The step-level timeout-minutes: 20 conflicts with the script’s per-command timeout defaults: run_activity_report.cjs can run two sequential log downloads, each allowed up to 20 minutes, plus settle delays and issue creation. With a 20-minute step timeout, the step can be killed before the script-level timeout/cleanup logic completes (and before the second range runs). Consider making the step timeout comfortably larger than the worst-case script runtime, or reducing the per-command timeout so the overall step stays within 20 minutes.
Suggested change
| timeout-minutes: 20 | |
| timeout-minutes: 60 |
| ${{ runner.os }}-activity-report-logs- | ||
| `) | ||
| yaml.WriteString(` - name: Generate agentic workflow activity report in target repository | ||
| timeout-minutes: 20 |
There was a problem hiding this comment.
The step-level timeout-minutes: 20 may be too low given the script’s behavior: run_activity_report.cjs runs multiple log downloads sequentially with a default 20-minute timeout per download, plus settle delays. This means the workflow step can time out before the script’s own timeout/cleanup kicks in or before all ranges complete. Align the step timeout with the script’s worst-case runtime, or lower the per-download timeout accordingly.
Suggested change
| timeout-minutes: 20 | |
| timeout-minutes: 120 |
| ${{ runner.os }}-activity-report-logs-${{ github.repository }}- | ||
| ${{ runner.os }}-activity-report-logs- | ||
| - name: Generate agentic workflow activity report | ||
| timeout-minutes: 20 |
There was a problem hiding this comment.
This step timeout (20 minutes) is lower than the script’s worst-case runtime: run_activity_report.cjs runs two sequential log downloads with a default 20-minute timeout each, plus settle delays. The step can be terminated by Actions before the script’s own timeout/cleanup completes or before the second range runs. Consider increasing the step timeout above the script’s maximum expected duration, or reducing the per-download timeout so the step stays within 20 minutes end-to-end.
Suggested change
| timeout-minutes: 20 | |
| timeout-minutes: 50 |
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 62/100
Test Classification DetailsView all 5 test classifications
Suggested Improvements
|
| Test file | Lines added | Production file | Lines added | Ratio |
|---|---|---|---|---|
maintenance_workflow_test.go |
+6 | maintenance_workflow_yaml.go |
+1 | 6:1 |
side_repo_maintenance_integration_test.go |
+4 | side_repo_maintenance.go |
+1 | 4:1 |
run_activity_report.test.cjs |
+12 | run_activity_report.cjs |
+111 | 0.11:1 |
The Go ratio is a false positive: the production changes are minimal one-line bug-fixes (timeout value, env var), while the new assertions verify the full generated YAML structure — exactly the right approach. No structural concern here.
Score Breakdown
| Component | Score |
|---|---|
| Behavioral coverage — 5/5 design tests | 40/40 |
| Error/edge case coverage — 1/5 tests | 6/30 |
| Low duplication — none detected | 20/20 |
| Proportional growth — technical inflation flagged | 0/10 (see note above) |
| Total | 62/100 |
Language Support
Tests analyzed:
- 🐹 Go (
*_test.go): 2 files modified — unit (//go:build !integration) and integration (//go:build integration) ✅ build tags present - 🟨 JavaScript (
*.test.cjs): 1 file modified — vitest; mocking targets areglobal.core,global.github,global.context(external I/O — acceptable) ✅
Verdict
✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). No coding-guideline violations detected. All five modified test blocks verify observable behavioural contracts. Edge-case coverage is the main area for improvement.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §24815688269
🧪 Test quality analysis by Test Quality Sentinel · ● 577.1K · ◷
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Collaborator
|
@copilot simplify the logs download steps with a single call to gh aw logs for 7d and at most 100 runs. No javascript wrapper script. |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c20c68a3-5880-45d7-b66c-eb70d9a4eb50 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Implemented in 89313c7. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
Fix activity_report log timeout, settle delay, and gh aw process cleanup
Simplify activity_report log download to a single gh aw logs call
Apr 23, 2026
Collaborator
|
@copilot add step after saving cache to generate activity report based on the downloaded logs. See https://github.com/github/gh-aw/actions/runs/24816980590/job/72633251266#step:8:1 where issue is not created. |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8621b272-b02f-42a0-9df9-28efb8eb5043 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Implemented in 9ebb673. Added a post-cache step to generate the activity report issue from the downloaded logs markdown ( Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
Simplify activity_report log download to a single gh aw logs call
Simplify activity_report logs download and restore issue generation from downloaded logs
Apr 23, 2026
This was referenced Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Simplifies
activity_reportmaintenance log downloading by replacing the JavaScript wrapper-based flow with a single directgh aw logscommand, and restores activity report issue creation using the downloaded logs output.Changes
gh aw logs --start-date -1w --count 100--format markdown > ./.cache/gh-aw/activity-report-logs/report.md[aw] agentic status reportissue from the downloaded markdown report.pkg/workflow/maintenance_workflow_yaml.gopkg/workflow/side_repo_maintenance.go.github/workflows/agentics-maintenance.ymlrun_activity_report.cjsis no longer invoked from maintenance workflows).gh aw logsusage--start-date -1w--count 100Validation
npx vitest run run_activity_report.test.cjsgo test -v -run "TestGenerateMaintenanceWorkflow_OperationJobConditions" ./pkg/workflow/go test -tags=integration -v -run "TestSideRepoMaintenanceWorkflowGenerated_EndToEnd" ./pkg/workflow/make agent-finish) still fails due unrelated existing baseline failures inpkg/workflow:TestCopilotDetectionDefaultModelTestWasmGolden_CompileFixtures🤖 Smoke CI scheduled run: https://github.com/github/gh-aw/actions/runs/24817076783