Fix action summary for structured output#485
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the GitHub Action summary output for structured formats (stream-json / json) by extracting the assistant’s terminal content (final/error) instead of emitting trailing lifecycle metadata (run_end / done), addressing issue #483.
Changes:
- Add
scripts/action-summary.mjsto compute a best-effort, single-line, 280-char summary from captured output (structured-aware parsing). - Add a Node test suite validating summary extraction behavior across formats and CLI invocation.
- Update
action.ymlto call the new summarizer script rather thantailing the output file.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
scripts/action-summary.test.mjs |
Adds unit tests and a CLI integration test for summary extraction across output formats. |
scripts/action-summary.mjs |
Implements structured-output parsing to return final/error text instead of lifecycle JSON. |
action.yml |
Switches action output summarization to the new Node summarizer script and wires in github.action_path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Node-based summary extractor for ZERO output, updates the GitHub Action to call it, and expands tests for structured output ordering, fallback behavior, truncation, and CLI invocation. ChangesAction summary extraction fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RunZeroStep as action.yml Run ZERO step
participant Node
participant ActionSummary as action-summary.mjs
RunZeroStep->>Node: node scripts/action-summary.mjs output_file
Node->>ActionSummary: read output_file, call summarizeOutput(format, contents)
ActionSummary->>ActionSummary: parse NDJSON events for final/error event
ActionSummary-->>Node: return truncated one-line summary
Node-->>RunZeroStep: write summary to stdout
Related issues: Suggested labels: bug, github-actions Suggested reviewers: none identified 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
action.yml (1)
246-260: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the summary command from failing the step
action.yml:253—set -eis active here, so a non-zeronodeexit will abort beforeGITHUB_OUTPUTis written and beforeexit "${code}"runs, replacing the real ZERO status with the summary helper’s failure. Wrap this call inset +e/set -e(or equivalent) and fall back to an empty summary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@action.yml` around lines 246 - 260, The summary generation in the `action.yml` step can terminate the whole script because `set -e` is enabled, which prevents writing to `GITHUB_OUTPUT` and exiting with the intended `code`. Update the `summary="$(node ...)"` section in the step that writes `exit-code`, `output-file`, and `summary` so the `action-summary.mjs` call is run with error handling disabled temporarily (or equivalent), then restore `set -e` and use an empty summary fallback if the helper fails.
🧹 Nitpick comments (1)
action.yml (1)
253-253: 🧹 Nitpick | 🔵 TrivialNew hard dependency on
nodebeing present on the runner.Previously the summary extraction used only bash/grep/tail. This now requires a
nodebinary on PATH. GitHub-hosted runners ship Node, but self-hosted runners (which this composite action doesn't otherwise preclude) may not have it installed — combined with theset -eissue above, a missingnodewould fail the whole step. Consider documenting this new requirement (e.g., README/action description) or guarding with acommand -v nodecheck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@action.yml` at line 253, The summary extraction step in action.yml now hard-depends on the node binary via action-summary.mjs, so update the composite action to handle missing Node on self-hosted runners by adding a command -v node guard or an equivalent fallback before invoking node in the summary assignment, and also document this requirement in the action description or README so users know node must be available on PATH.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@action.yml`:
- Around line 246-260: The summary generation in the `action.yml` step can
terminate the whole script because `set -e` is enabled, which prevents writing
to `GITHUB_OUTPUT` and exiting with the intended `code`. Update the
`summary="$(node ...)"` section in the step that writes `exit-code`,
`output-file`, and `summary` so the `action-summary.mjs` call is run with error
handling disabled temporarily (or equivalent), then restore `set -e` and use an
empty summary fallback if the helper fails.
---
Nitpick comments:
In `@action.yml`:
- Line 253: The summary extraction step in action.yml now hard-depends on the
node binary via action-summary.mjs, so update the composite action to handle
missing Node on self-hosted runners by adding a command -v node guard or an
equivalent fallback before invoking node in the summary assignment, and also
document this requirement in the action description or README so users know node
must be available on PATH.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f35b5206-2f0c-45a2-a9ee-1a562801b4fe
📒 Files selected for processing (3)
action.ymlscripts/action-summary.mjsscripts/action-summary.test.mjs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/action-summary.test.mjs (1)
101-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate CLI spawn/assert scaffolding across absolute- and relative-path tests.
The relative-path test (124-148) repeats the mkdtemp/write/spawnSync/assert/cleanup boilerplate from the absolute-path test (101-123), differing only in the script path argument and
cwd. Extracting a small helper (e.g.,runCli(scriptArg, { cwd })) would reduce duplication without losing clarity.♻️ Suggested helper extraction
+function runCli(scriptArg, expectedSummary, { cwd } = {}) { + const dir = mkdtempSync(join(tmpdir(), 'zero-action-summary-')); + try { + const outputFile = join(dir, 'zero-output.jsonl'); + writeFileSync( + outputFile, + jsonl([ + { type: 'final', text: expectedSummary }, + { type: 'run_end', status: 'success', exitCode: 0 }, + ]), + ); + const result = spawnSync(process.execPath, [scriptArg, 'stream-json', outputFile], { + cwd, + encoding: 'utf8', + }); + assert.equal(result.status, 0); + assert.equal(result.stderr, ''); + assert.equal(result.stdout, `${expectedSummary}\n`); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/action-summary.test.mjs` around lines 101 - 148, The two CLI tests duplicate the same mkdtempSync/writeFileSync/spawnSync/assert/cleanup flow in action-summary.test.mjs; extract that repeated setup into a small helper (for example, around scriptPath, spawnSync, and the shared assertions) and call it from both the absolute-path and relative-path tests. Keep the helper flexible for the only differences: the script argument passed to process.execPath and the optional cwd for the relative-path case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/action-summary.test.mjs`:
- Around line 101-148: The two CLI tests duplicate the same
mkdtempSync/writeFileSync/spawnSync/assert/cleanup flow in
action-summary.test.mjs; extract that repeated setup into a small helper (for
example, around scriptPath, spawnSync, and the shared assertions) and call it
from both the absolute-path and relative-path tests. Keep the helper flexible
for the only differences: the script argument passed to process.execPath and the
optional cwd for the relative-path case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd01c752-558e-4074-8b01-92acf5328d4b
📒 Files selected for processing (2)
scripts/action-summary.mjsscripts/action-summary.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/action-summary.mjs
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Core fix is right — reading the final/error events instead of tailing metadata matches the event schema, and the tests cover the #483 case well.
One thing to fix first: the new summary line in action.yml runs under set -e without the || true the old grep/tail line had. If node exits non-zero — module not found, or no node on a self-hosted runner — the summary="$(node ...)" assignment aborts the step before it writes the output and before exit "${code}", so the step reports the helper's failure instead of Zero's real exit code. It won't fire on GitHub-hosted runners where node's always there, but it's a real drop in defensiveness. Append || true (or wrap the call in set +e/set -e). CodeRabbit flagged the same one.
Fix that and it's good to go.
|
done @Vasanthdev2004 |
|
please have a look again @Vasanthdev2004 |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewing — my one blocker is fixed. The summary line has the || true now, so a node failure (missing module, no node on a self-hosted runner) degrades to an empty summary instead of aborting the step under set -euo pipefail, and ACTION_PATH is wired so the script actually resolves.
The core fix is right and I checked it holds: summarizeStructured walks the stream-json events and takes the last final event's text (or an error event's message) rather than tailing the last line — which was the run_end lifecycle metadata #483 was showing. Non-structured output keeps the old last-line behavior, both paths stay bounded at 280, and it falls back to the last non-empty line if no final/error event is present. The dedicated action-summary.test.mjs covers it, and CI is green including the action.yml validation.
Marked #483 issue-approved — real bug, clean fix.
Good to merge — kevin's already signed off too.
Fixes #483. The action now reads final/error events for structured output instead of tailing run metadata.
Summary by CodeRabbit
New Features
ACTION_PATHenvironment variable during runtime.Bug Fixes
Tests