fix: populate safe_outputs in activity/summary.json even when manifest is empty#49650
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey 👋 — thanks for initiating this fix for the missing
This looks like a solid fix for issue #49367. Once the checklist is complete and tests pass, this should be ready for review.
|
…st is present
- In generate_usage_activity_summary.cjs main(): when parseSafeOutputsManifest()
returns null (empty manifest), check if the file exists and if so write
safe_outputs: {total_items: 0, items_by_type: {}} to the summary. This lets
downstream consumers distinguish "manifest not downloaded" (safe_outputs absent)
from "manifest present but 0 items created" (total_items == 0).
- Add core.info logging for all three manifest states for diagnostic visibility.
- Add /tmp/gh-aw/safe-output-items.jsonl to the diagnostic file status list in
notify_comment.go so download failures become visible in the conclusion job log.
- Recompile all 268 lock files after notify_comment.go change.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49650 does not have the 'implementation' label and has only 1 new line of code in default business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Updates safe-output usage reporting to distinguish empty manifests from missing artifacts and improves diagnostics.
Changes:
- Emits zero-valued
safe_outputsfor present, empty manifests. - Logs manifest status during summary generation and workflow conclusion.
- Recompiles generated workflow lock files.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/generate_usage_activity_summary.cjs |
Adds manifest-state handling and diagnostics. |
pkg/workflow/notify_comment.go |
Checks safe-output manifest availability. |
268 .github/workflows/*.lock.yml files |
Propagates the diagnostic check. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (2)
actions/setup/js/generate_usage_activity_summary.cjs:472
- This zero-valued branch does not restore the Safe-Output Writes metric described by the PR and linked issue.
applyUsageActivitySummaryToResultonly backfills whenTotalItems > 0(pkg/cli/logs_usage_activity.go:171), while non-empty manifests were already included before this change. Therefore an unexpectedly empty manifest still producesrun.safe_items_count == 0, which the API Consumption Report sums as zero. The fix needs to address why runs with actuated writes produce an empty manifest (or persist an authoritative non-zero count), rather than recording that empty manifest as a genuine zero.
actions/setup/js/generate_usage_activity_summary.cjs:475 - The new present-empty versus missing-manifest behavior is not exercised by
generate_usage_activity_summary.test.cjs; its tests only call the parser, which still returnsnullfor both cases. Add coverage that runs or extracts the summary-building logic and assertssafe_outputsis present with zero for an empty file and absent for a missing file, including the read-error state.
- Files reviewed: 270/270 changed files
- Comments generated: 1
- Review effort level: Balanced
| } else if (fs.existsSync(MANIFEST_FILE_PATH)) { | ||
| // Manifest was downloaded but contained no loggable items. | ||
| summary.safe_outputs = { total_items: 0, items_by_type: {} }; | ||
| core.info(`safe-output-items manifest: 0 item(s) logged (file present but empty)`); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 55 AIC · ⌖ 7.39 AIC · ⊞ 5.4K
| if (safeOutputs) { | ||
| summary.safe_outputs = safeOutputs; | ||
| core.info(`safe-output-items manifest: ${safeOutputs.total_items} item(s) logged (types: ${Object.keys(safeOutputs.items_by_type).join(", ") || "none"})`); | ||
| } else if (fs.existsSync(MANIFEST_FILE_PATH)) { |
There was a problem hiding this comment.
Potential bug: else if (fs.existsSync(...)) also fires on read errors.
parseSafeOutputsManifest() returns null in three cases:
- File does not exist → correctly handled by the
elsebranch - File exists but
readFileSyncthrows (permissions, I/O error, etc.) → silently producestotal_items: 0 - File exists but has 0 valid JSONL lines → correctly handled by this new branch
The new else if is intended for case 3 but it also silently swallows case 2 — an I/O failure — and emits { total_items: 0, items_by_type: {} } as if the manifest was genuinely empty. This makes read failures indistinguishable from a valid empty run.
Consider distinguishing the two null-return paths in parseSafeOutputsManifest (e.g. throw on read error, or return a { error: true } sentinel), so main() can log a warning instead of writing misleading zero counts when a read failure occurs.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — requesting changes on test coverage and a design nit.
📋 Key Themes & Highlights
Key Issues
-
Missing test for new branch (
/tdd): Theelse if (fs.existsSync(MANIFEST_FILE_PATH))path inmain()— the core of this fix — is not covered by any test. If it regresses,safe_outputswill silently go missing again with no failing test to catch it. -
Dual-null ambiguity (
/diagnosing-bugs):parseSafeOutputsManifest()returnsnullfor two structurally different reasons: file absent vs. file present but zero items. The fix works around this inmain()by re-callingfs.existsSync, but a cleaner approach is to have the function return a value (e.g.{ total_items: 0, items_by_type: {} }) whenever the file is found, reservingnullstrictly for "file not found". This would make the zero-item case testable at the unit level and remove the second filesystem probe.
Positive Highlights
- ✅ Correct semantic: distinguishing "absent" from "zero" is exactly the right model for downstream consumers.
- ✅ Good diagnostic
core.infologging for all three manifest states. - ✅
notify_comment.gochange is minimal and appropriate — adding the JSONL path to the diagnostic loop surfaces download failures without any logic change.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42.1 AIC · ⌖ 8.07 AIC · ⊞ 7.1K
Comment /matt to run again
| if (safeOutputs) { | ||
| summary.safe_outputs = safeOutputs; | ||
| core.info(`safe-output-items manifest: ${safeOutputs.total_items} item(s) logged (types: ${Object.keys(safeOutputs.items_by_type).join(", ") || "none"})`); | ||
| } else if (fs.existsSync(MANIFEST_FILE_PATH)) { |
There was a problem hiding this comment.
[/tdd] The new else if (fs.existsSync(MANIFEST_FILE_PATH)) branch has no test coverage — a regression here would silently zero-out safe-output counts again.
💡 Suggested test case
Add a test to generate_usage_activity_summary.test.cjs that exercises the main() path (or at minimum an integration helper) where the manifest file exists but is empty or contains only blank lines, and asserts that the resulting summary JSON includes safe_outputs: { total_items: 0, items_by_type: {} }.
The parseSafeOutputsManifest unit tests already verify the null-return path, but the new else if branch in main() is not covered.
@copilot please address this.
| if (safeOutputs) { | ||
| summary.safe_outputs = safeOutputs; | ||
| core.info(`safe-output-items manifest: ${safeOutputs.total_items} item(s) logged (types: ${Object.keys(safeOutputs.items_by_type).join(", ") || "none"})`); | ||
| } else if (fs.existsSync(MANIFEST_FILE_PATH)) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The fix re-checks fs.existsSync(MANIFEST_FILE_PATH) in main() after parseSafeOutputsManifest() already called it — a second I/O call on the same path — because the function conflates two different "nothing to return" states into a single null.
💡 Cleaner design
Consider returning a sentinel that distinguishes the two cases, for example:
// In parseSafeOutputsManifest:
if (!fs.existsSync(manifestPath)) {
return { found: false, data: null };
}
// ... parse ...
return { found: true, data: totalItems > 0 ? { total_items, items_by_type } : null };Or simpler: always return { total_items, items_by_type } when the file is found (including the zero case), and reserve null strictly for file-not-found. That removes the need for main() to re-probe the filesystem, keeps the logic in one place, and the test for the zero-item case naturally lives inside parseSafeOutputsManifests
There was a problem hiding this comment.
REQUEST_CHANGES — untested three-state branching + error/empty conflation in the manifest fix
The core fix (distinguishing "manifest absent" vs "manifest present but empty" vs "manifest present with items") is sound in intent, but two issues should be addressed before merge.
💡 Themes
- Error/empty conflation:
parseSafeOutputsManifest()returnsnullfor three different reasons (file missing, read failure, zero valid items). The newelse if (fs.existsSync(MANIFEST_FILE_PATH))branch inmain()cannot tell a genuine read failure apart from a legitimately empty manifest — both get reported astotal_items: 0, silently masking infra problems. - Zero test coverage on
main(): the new branching logic that fixes this bug is untested —mainisn't even exported, and there's no test verifyingsummary.safe_outputs = { total_items: 0, items_by_type: {} }for the "present but empty" case. Without a regression test, this exact bug could resurface silently.
The notify_comment.go diagnostic-loop addition and the resulting 268 .lock.yml regenerations are mechanical and correct — no concerns there.
🔎 Code quality review by PR Code Quality Reviewer · auto · 148 AIC · ⌖ 13.7 AIC · ⊞ 7.8K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/generate_usage_activity_summary.cjs:470
Redundant fs.existsSync call re-checks the same path parseSafeOutputsManifest() already tested internally, creating a TOCTOU gap and conflating a genuine read failure with an empty-manifest state.
<details><summary>💡 Details</summary>
parseSafeOutputsManifest() returns null for three distinct reasons: (a) file missing, (b) readFileSync throws (permission error / file deleted mid-race), (c) file present but 0 valid JSON lines. The new else if (fs.existsSync(MANIFEST_FILE_PATH)) br…
actions/setup/js/generate_usage_activity_summary.cjs:439
main() is not exported and has zero test coverage, so the exact three-state branching this PR adds (and the bug it fixes) has no regression guard.
<details><summary>💡 Details</summary>
The PR description states the bug was safe_items_count silently zeroed because parseSafeOutputsManifest() couldn't distinguish "file not found" from "file exists with 0 items". The new fix lives entirely inside main(), but main is never added to module.exports (only parseFirewallLogs, `parseSessi…
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30721766175
|
…nifest
- parseSafeOutputsManifest() now has three distinct outcomes:
null → file not found (unchanged)
{ total_items: 0, ... } → file present, no loggable items (was null)
throws → file exists but cannot be read (was silently null)
- main() wraps parseSafeOutputsManifest() in try/catch; the existsSync
workaround is removed since the function now returns a zero-item
object for successful empty reads
- Updated tests: empty/blank-line manifest cases now expect zero-item
object instead of null
- Added tests: unreadable-file throws; all-invalid-lines returns zero
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit
|
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30726226343
|
All three items were addressed in commit
|
🔍 Triage Summary
Populates safe_outputs in activity/summary.json when manifest empty. Large diff (271 files touched), CI green but CHANGES_REQUESTED review outstanding — resolve before merge. Automated triage — see the triage report for full context.
|
|
@copilot quick triage for this PR:
|
…ix-safe-items-count Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Merged origin/main to bring in aw-failure-investigator changes: 05bc7f1 dropped "cancelled" from FAILURE_CONCLUSIONS 1d9b0f8 updated the test to match - Fixed copyloopvar lint: removed redundant `tt := tt` in format_list_test.go (obsolete since Go 1.22 loop-variable semantics, flagged after merge) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The root cause was a stale branch: main had landed |
The unit test failure from https://github.com/github/gh-aw/actions/runs/30722553785/job/91481380575 was already fixed in The three-state manifest logic is fully in place:
The recent |
safe_items_countwas silently zeroed inrun_summary.jsonbecauseparseSafeOutputsManifest()returnednullfor both "file not found" and "file exists but 0 items" — making it impossible for the Go CLI backfill to distinguish the two cases, and causing the API Consumption Report to report 0 Safe-Output Writes for days with non-zero activity.Changes
generate_usage_activity_summary.cjs—main()now distinguishes three manifest states instead of two:parseSafeOutputsManifest()returns non-null → writesafe_outputswith actual counts (unchanged)safe_outputs: { total_items: 0, items_by_type: {} }(new)safe_outputsentirely (unchanged behavior, newcore.infowarning)Consumers can now interpret
safe_outputsabsent as "artifact never downloaded" vstotal_items == 0as "manifest present, genuinely no items." The Go backfill condition (TotalItems > 0) already correctly handles the zero case — no Go changes needed.notify_comment.go— Added/tmp/gh-aw/safe-output-items.jsonlto the diagnostic file status loop in the "Collect usage artifact files" step, making manifest download failures visible in the conclusion job log.268
.lock.ymlfiles recompiled after thenotify_comment.gochange.branch refresh requested from
Run details: https://github.com/github/gh-aw/actions/runs/30726226343