Skip to content

Pre-bundle Process Safe Outputs stdout/stderr in safe-output failure artifacts - #47855

Merged
pelikhan merged 6 commits into
mainfrom
copilot/deep-report-pre-bundle-stdout-stderr
Jul 25, 2026
Merged

Pre-bundle Process Safe Outputs stdout/stderr in safe-output failure artifacts#47855
pelikhan merged 6 commits into
mainfrom
copilot/deep-report-pre-bundle-stdout-stderr

Conversation

Copilot AI commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

safe_outputs failures in the Process Safe Outputs step were not carrying actionable step-level logs in pre-bundled artifacts, forcing timeout-prone post-hoc log retrieval and blocking root-cause analysis. This change ensures the step’s stdout/stderr are captured and shipped with the existing safe-output artifact bundle.

  • Safe-output handler step logging

    • Updated safe-output step generation to tee runtime output streams to stable files under /tmp/gh-aw during Process Safe Outputs.
    • Restores original stream writers after execution to avoid side effects on subsequent logic.
  • Artifact pre-bundle coverage

    • Extended the always-uploaded safe-output items artifact path set to include:
      • /tmp/gh-aw/process-safe-outputs.stdout.log
      • /tmp/gh-aw/process-safe-outputs.stderr.log
  • Compiler assertions for regression protection

    • Added focused compiler expectations that generated workflows include both log capture wiring and the two artifact paths.
- name: Upload Safe Outputs Items
  if: always()
  with:
    path: |
      /tmp/gh-aw/safe-output-items.jsonl
      /tmp/gh-aw/temporary-id-map.json
      /tmp/gh-aw/process-safe-outputs.stdout.log
      /tmp/gh-aw/process-safe-outputs.stderr.log

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Pre-bundle stdout/stderr on safe-output job failure Pre-bundle Process Safe Outputs stdout/stderr in safe-output failure artifacts Jul 24, 2026
Copilot AI requested a review from pelikhan July 24, 2026 20:57
c.addSafeOutputGitHubTokenForConfig(&steps, data, configToken)

steps = append(steps, " script: |\n")
steps = append(steps, " const fs = require('fs');\n")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot refactor JavaScript to actions/setup/js

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2b0a4f8. I moved the inline Process Safe Outputs JavaScript into actions/setup/js/process_safe_outputs.cjs and updated the compiler step to load that module.

Copilot AI and others added 2 commits July 24, 2026 21:08
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

Triage | Category: chore | Risk: low | Score: 31/100 (impact:10, urgency:10, quality:11)

Recommended action: batch_review (Batch E: lock file bundles) — Pre-bundles safe-outputs artifacts in lock files; 266 files changed, draft, no CI. Mass lock file update.

Score breakdown: impact=10 (infra artifact bundling), urgency=10 (draft), quality=11 (no CI)

Generated by 🔧 PR Triage Agent · sonnet46 · 31.3 AIC · ⌖ 5.51 AIC · ⊞ 5.7K ·

@pelikhan
pelikhan marked this pull request as ready for review July 25, 2026 04:24
Copilot AI review requested due to automatic review settings July 25, 2026 04:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds pre-bundled stdout/stderr diagnostics for Process Safe Outputs.

Changes:

  • Captures handler output in /tmp/gh-aw.
  • Uploads captured logs with safe-output artifacts.
  • Updates compiler assertions and generated workflows.
Show a summary per file
File Description
actions/setup/js/process_safe_outputs.cjs Implements output capture.
pkg/workflow/compiler_safe_outputs_steps.go Invokes the capture wrapper.
pkg/workflow/compiler_safe_outputs_steps_test.go Updates step expectations.
pkg/workflow/compiler_safe_outputs_job.go Adds artifact log paths.
pkg/workflow/compiler_jobs_test.go Verifies artifact paths.
.github/workflows/workflow-skill-extractor.lock.yml Regenerates workflow.
.github/workflows/workflow-normalizer.lock.yml Regenerates workflow.
.github/workflows/workflow-health-manager.lock.yml Regenerates workflow.
.github/workflows/workflow-generator.lock.yml Regenerates workflow.
.github/workflows/visual-regression-checker.lock.yml Regenerates workflow.
.github/workflows/video-analyzer.lock.yml Regenerates workflow.
.github/workflows/uk-ai-operational-resilience.lock.yml Regenerates workflow.
.github/workflows/typist.lock.yml Regenerates workflow.
.github/workflows/tidy.lock.yml Regenerates workflow.
.github/workflows/test-quality-sentinel.lock.yml Regenerates workflow.
.github/workflows/terminal-stylist.lock.yml Regenerates workflow.
.github/workflows/super-linter.lock.yml Regenerates workflow.
.github/workflows/sub-issue-closer.lock.yml Regenerates workflow.
.github/workflows/static-analysis-report.lock.yml Regenerates workflow.
.github/workflows/stale-repo-identifier.lock.yml Regenerates workflow.
.github/workflows/stale-pr-cleanup.lock.yml Regenerates workflow.
.github/workflows/spec-librarian.lock.yml Regenerates workflow.
.github/workflows/smoke-workflow-call.lock.yml Regenerates workflow.
.github/workflows/smoke-test-tools.lock.yml Regenerates workflow.
.github/workflows/smoke-temporary-id.lock.yml Regenerates workflow.
.github/workflows/smoke-service-ports.lock.yml Regenerates workflow.
.github/workflows/smoke-otel-backends.lock.yml Regenerates workflow.
.github/workflows/smoke-opencode.lock.yml Regenerates workflow.
.github/workflows/smoke-copilot-small.lock.yml Regenerates workflow.
.github/workflows/smoke-copilot-sdk.lock.yml Regenerates workflow.
.github/workflows/smoke-codex.lock.yml Regenerates workflow.
.github/workflows/smoke-ci.lock.yml Regenerates workflow.
.github/workflows/smoke-call-workflow.lock.yml Regenerates workflow.
.github/workflows/smoke-agent-scoped-approved.lock.yml Regenerates workflow.
.github/workflows/smoke-agent-public-none.lock.yml Regenerates workflow.
.github/workflows/smoke-agent-public-approved.lock.yml Regenerates workflow.
.github/workflows/smoke-agent-all-none.lock.yml Regenerates workflow.
.github/workflows/skillet.lock.yml Regenerates workflow.
.github/workflows/sergo.lock.yml Regenerates workflow.
.github/workflows/semantic-function-refactor.lock.yml Regenerates workflow.
.github/workflows/security-review.lock.yml Regenerates workflow.
.github/workflows/security-compliance.lock.yml Regenerates workflow.
.github/workflows/scout.lock.yml Regenerates workflow.
.github/workflows/research.lock.yml Regenerates workflow.
.github/workflows/repo-tree-map.lock.yml Regenerates workflow.
.github/workflows/release.lock.yml Regenerates workflow.
.github/workflows/refiner.lock.yml Regenerates workflow.
.github/workflows/q.lock.yml Regenerates workflow.
.github/workflows/pr-triage-agent.lock.yml Regenerates workflow.
.github/workflows/pr-nitpick-reviewer.lock.yml Regenerates workflow.
.github/workflows/pr-description-caveman.lock.yml Regenerates workflow.
.github/workflows/poem-bot.lock.yml Regenerates workflow.
.github/workflows/plan.lock.yml Regenerates workflow.
.github/workflows/pdf-summary.lock.yml Regenerates workflow.
.github/workflows/org-health-report.lock.yml Regenerates workflow.
.github/workflows/objective-impact-report.lock.yml Regenerates workflow.
.github/workflows/notion-issue-summary.lock.yml Regenerates workflow.
.github/workflows/metrics-collector.lock.yml Regenerates workflow.
.github/workflows/lint-monster.lock.yml Regenerates workflow.
.github/workflows/jsweep.lock.yml Regenerates workflow.
.github/workflows/issue-triage-agent.lock.yml Regenerates workflow.
.github/workflows/issue-monster.lock.yml Regenerates workflow.
.github/workflows/issue-arborist.lock.yml Regenerates workflow.
.github/workflows/hippo-embed.lock.yml Regenerates workflow.
.github/workflows/grumpy-reviewer.lock.yml Regenerates workflow.
.github/workflows/gpclean.lock.yml Regenerates workflow.
.github/workflows/go-pattern-detector.lock.yml Regenerates workflow.
.github/workflows/go-fan.lock.yml Regenerates workflow.
.github/workflows/firewall.lock.yml Regenerates workflow.
.github/workflows/example-workflow-analyzer.lock.yml Regenerates workflow.
.github/workflows/example-permissions-warning.lock.yml Regenerates workflow.
.github/workflows/example-failure-category-filter.lock.yml Regenerates workflow.
.github/workflows/eslint-refiner.lock.yml Regenerates workflow.
.github/workflows/eslint-monster.lock.yml Regenerates workflow.
.github/workflows/duplicate-code-detector.lock.yml Regenerates workflow.
.github/workflows/draft-pr-cleanup.lock.yml Regenerates workflow.
.github/workflows/discussion-task-miner.lock.yml Regenerates workflow.
.github/workflows/dev.lock.yml Regenerates workflow.
.github/workflows/dev-hawk.lock.yml Regenerates workflow.
.github/workflows/designer-drift-audit.lock.yml Regenerates workflow.
.github/workflows/deployment-incident-monitor.lock.yml Regenerates workflow.
.github/workflows/dependabot-go-checker.lock.yml Regenerates workflow.
.github/workflows/delight.lock.yml Regenerates workflow.
.github/workflows/deepsec-security-scan.lock.yml Regenerates workflow.
.github/workflows/deep-report.lock.yml Regenerates workflow.
.github/workflows/daily-token-consumption-report.lock.yml Regenerates workflow.
.github/workflows/daily-team-status.lock.yml Regenerates workflow.
.github/workflows/daily-syntax-error-quality.lock.yml Regenerates workflow.
.github/workflows/daily-sentrux-report.lock.yml Regenerates workflow.
.github/workflows/daily-secrets-analysis.lock.yml Regenerates workflow.
.github/workflows/daily-reliability-review.lock.yml Regenerates workflow.
.github/workflows/daily-regulatory.lock.yml Regenerates workflow.
.github/workflows/daily-observability-report.lock.yml Regenerates workflow.
.github/workflows/daily-news.lock.yml Regenerates workflow.
.github/workflows/daily-model-resolution.lock.yml Regenerates workflow.
.github/workflows/daily-model-inventory.lock.yml Regenerates workflow.
.github/workflows/daily-geo-optimizer.lock.yml Regenerates workflow.
.github/workflows/daily-file-diet.lock.yml Regenerates workflow.
.github/workflows/daily-evals-report.lock.yml Regenerates workflow.
.github/workflows/daily-credit-limit-test.lock.yml Regenerates workflow.
.github/workflows/daily-cli-tools-tester.lock.yml Regenerates workflow.
.github/workflows/daily-cli-performance.lock.yml Regenerates workflow.
.github/workflows/daily-choice-test.lock.yml Regenerates workflow.
.github/workflows/craft.lock.yml Regenerates workflow.
.github/workflows/copilot-opt.lock.yml Regenerates workflow.
.github/workflows/copilot-centralization-optimizer.lock.yml Regenerates workflow.
.github/workflows/contribution-check.lock.yml Regenerates workflow.
.github/workflows/commit-changes-analyzer.lock.yml Regenerates workflow.
.github/workflows/cli-consistency-checker.lock.yml Regenerates workflow.
.github/workflows/breaking-change-checker.lock.yml Regenerates workflow.
.github/workflows/brave.lock.yml Regenerates workflow.
.github/workflows/bot-detection.lock.yml Regenerates workflow.
.github/workflows/blog-auditor.lock.yml Regenerates workflow.
.github/workflows/aw-failure-investigator.lock.yml Regenerates workflow.
.github/workflows/artifacts-summary.lock.yml Regenerates workflow.
.github/workflows/architecture-guardian.lock.yml Regenerates workflow.
.github/workflows/archie.lock.yml Regenerates workflow.
.github/workflows/approach-validator.lock.yml Regenerates workflow.
.github/workflows/agentic-token-optimizer.lock.yml Regenerates workflow.
.github/workflows/agent-performance-analyzer.lock.yml Regenerates workflow.
.github/workflows/ace-editor.lock.yml Regenerates workflow.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

pkg/workflow/compiler_safe_outputs_job.go:909

  • The linked issue expects logs for every safe-output failure, but this uploader is still omitted when safe_outputs.staged is enabled (appendFinalSafeOutputSteps, lines 655-659). The regenerated stale-repo-identifier, poem-bot, and daily-choice-test locks demonstrate the gap: they create these log files and then leave the job without any upload step. Always emit an artifact uploader for the logs in staged mode as well; if-no-files-found: ignore can continue to handle the absent staged manifest.
  • Files reviewed: 266/266 changed files
  • Comments generated: 3
  • Review effort level: Medium

" path: |\n",
" /tmp/gh-aw/safe-output-items.jsonl\n",
fmt.Sprintf(" /tmp/gh-aw/%s\n", constants.TemporaryIdMapFilename),
" /tmp/gh-aw/process-safe-outputs.stdout.log\n",
steps = append(steps, " setupGlobals(core, github, context, exec, io, getOctokit);\n")
steps = append(steps, " const { main } = require('"+SetupActionDestination+"/safe_output_handler_manager.cjs');\n")
steps = append(steps, " await main();\n")
steps = append(steps, generateGitHubScriptWithRequire("process_safe_outputs.cjs"))
const originalStdoutWrite = process.stdout.write.bind(process.stdout);
const originalStderrWrite = process.stderr.write.bind(process.stderr);

process.stdout.write = (chunk, encoding, callback) => {
Comment thread .github/workflows/ace-editor.lock.yml Outdated
with:
github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}
script: |
const fs = require('fs');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot refactor to file in actions/setup/js

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The inline script in all lock files now delegates to actions/process_safe_outputs.cjs. All 261 lock files were recompiled and are in sync — verified with make recompile (0 errors, lock drift check clean).

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (10 additions detected).

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Pre-bundle Process Safe Outputs stdout/stderr

The approach is correct and well-structured — capturing stdout/stderr to files during Process Safe Outputs and including them in the artifact bundle is a solid improvement for post-hoc debugging.

One blocking issue found in the new process_safe_outputs.cjs module (see inline comment): the process.stdout.write / process.stderr.write overrides only handle the 3-argument form. Node's Writable.write is also callable with 2 arguments (chunk, callback), in which case the callback arrives as the second parameter and the third is undefined. The current override forwards this incorrectly, silently dropping the caller's completion callback. This should be fixed before merging.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 40.1 AIC · ⌖ 7.42 AIC · ⊞ 5K

const originalStderrWrite = process.stderr.write.bind(process.stderr);

process.stdout.write = (chunk, encoding, callback) => {
stdoutStream.write(chunk);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The process.stdout.write and process.stderr.write overrides only handle the 3-argument form (chunk, encoding, callback). Node.js stream.Writable.write also accepts a 2-argument form (chunk, callback) where the callback is the second argument. In that case the override receives the callback as encoding and undefined as callback, so originalStdoutWrite(chunk, callback_fn, undefined) is called — the real callback never fires.

Fix by forwarding all received args unchanged:

process.stdout.write = (chunk, encodingOrCb, callback) => {
  stdoutStream.write(chunk);
  return originalStdoutWrite(chunk, encodingOrCb, callback);
};

Same applies to the process.stderr.write override on line 33.

@copilot please address this.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

⚠️ Test Quality Score: 70/100 — Acceptable

Analyzed 3 test change(s): 3 design, 0 implementation, 0 violation(s).

📊 Metrics (3 test changes)
Metric Value
Analyzed 3 (Go: 3, JS: 0)
✅ Design 3 (100%)
⚠️ Implementation 0 (0%)
Edge/error coverage 0 (0%)
Duplicate clusters 0
Inflation No
🚨 Violations 0
Test File Classification Issues
TestCompileSafeOutputsJob (2 new path assertions) compiler_jobs_test.go:1577 design_test Happy-path only
TestBuildHandlerManagerStep (filename update) compiler_safe_outputs_steps_test.go:372 design_test None

Verdict

Passed. 0% implementation tests (threshold: 30%). No guideline violations.

The changes are surgical: 2 new assertions verify that pre-bundled stdout/stderr log paths appear in the compiled artifact upload YAML, and 1 expectation updates the bundled script filename from safe_output_handler_manager.cjs to process_safe_outputs.cjs. Both verify user-visible behavioral contracts in the generated YAML output. Build tags are present on both files. Minor note: no error-path assertions were added alongside the new path checks, but the scope of the change is small enough that this is acceptable.

🧪 Test quality analysis by Test Quality Sentinel · sonnet46 · 31.1 AIC · ⌖ 7.5 AIC · ⊞ 7K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 70/100. 0% implementation tests (threshold: 30%). No violations.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes Required

The PR has one critical blocking issue plus two medium issues in process_safe_outputs.cjs.

Critical — compiler/lock-file divergence (blocks merge): The 2b0a4f8 refactor correctly moved the tee logic into process_safe_outputs.cjs and updated the Go compiler, but make recompile was never run. All 261+ .lock.yml files still contain the old inline JavaScript and call safe_output_handler_manager.cjs directly. The new module is dead code in production until a recompile is committed.

Medium issues in process_safe_outputs.cjs
  • stream.end() errors swallowed: new Promise(resolve => stream.end(resolve)) never rejects — close errors are silently lost. Use a resolve/reject wrapper with an "error" listener.
  • stream.write() errors unhandled: No "error" listener on the file streams; a disk-full error emits an unhandled event that can crash the step.
  • Append mode (flags: "a") poisons artifacts: Stale log files from previous failed runs are appended to, producing multi-run mixed output in the artifact — undermining the whole diagnostic goal. Use flags: "w".

🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 65.8 AIC · ⌖ 5 AIC · ⊞ 5.7K
Comment /review to run again

} finally {
process.stdout.write = originalStdoutWrite;
process.stderr.write = originalStderrWrite;
await Promise.all([new Promise(resolve => stdoutStream.end(resolve)), new Promise(resolve => stderrStream.end(resolve))]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stream.end() errors are silently swallowed, masking close failures: Using new Promise(resolve => stdoutStream.end(resolve)) only resolves — if the stream emits error during close (e.g., disk full on flush), that error is never observed and the artifact is silently incomplete.

💡 Suggested fix
const endStream = (s) => new Promise((resolve, reject) => {
  s.once("error", reject);
  s.end(resolve);
});
await Promise.all([endStream(stdoutStream), endStream(stderrStream)]);

Without this, a write-failure-at-close produces no diagnostic and the artifact is silently truncated.

const stderrStream = fs.createWriteStream(path.join(LOGS_DIR, STDERR_LOG), { flags: "a" });
const originalStdoutWrite = process.stdout.write.bind(process.stdout);
const originalStderrWrite = process.stderr.write.bind(process.stderr);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stdoutStream.write() errors are never caught, risking unhandled rejections or silently dropped output: The tee patch calls stdoutStream.write(chunk) but ignores the returned boolean and never attaches an error listener. A disk-full or EBADF error from the file stream will emit an unhandled error event, crashing the process or silently losing data.

💡 Suggested fix

Attach an error listener once after creating each stream:

stdoutStream.on("error", (err) => core.warning(`stdout log write error: ${err.message}`));
stderrStream.on("error", (err) => core.warning(`stderr log write error: ${err.message}`));

This degrades gracefully (warns but continues) rather than crashing the step or silently corrupting logs.

throw new Error(`Failed to create logs directory: ${getErrorMessage(err)}`, { cause: err });
}

const stdoutStream = fs.createWriteStream(path.join(LOGS_DIR, STDOUT_LOG), { flags: "a" });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append mode (flags: "a") causes stale log data from previous runs to persist in the artifact: If a prior run left log files in /tmp/gh-aw/ (e.g., if the artifact upload step failed), the current run appends to those files. The artifact then contains mixed output from multiple runs, making root-cause analysis of failures unreliable — the exact scenario this feature is meant to fix.

💡 Suggested fix

Use flags: "w" (write/truncate) instead of flags: "a":

const stdoutStream = fs.createWriteStream(path.join(LOGS_DIR, STDOUT_LOG), { flags: "w" });
const stderrStream = fs.createWriteStream(path.join(LOGS_DIR, STDERR_LOG), { flags: "w" });

Each run gets a clean file. The artifact upload with if-no-files-found: ignore already handles the case where logs are absent.

steps = append(steps, " setupGlobals(core, github, context, exec, io, getOctokit);\n")
steps = append(steps, " const { main } = require('"+SetupActionDestination+"/safe_output_handler_manager.cjs');\n")
steps = append(steps, " await main();\n")
steps = append(steps, generateGitHubScriptWithRequire("process_safe_outputs.cjs"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lock files are not regenerated after the process_safe_outputs.cjs refactor — compiler and generated artifacts are out of sync: Commit 2b0a4f8 moved the tee logic into process_safe_outputs.cjs and updated the compiler to call generateGitHubScriptWithRequire("process_safe_outputs.cjs"), but make recompile was never run. All 261+ .lock.yml files still contain the old inline tee code and call safe_output_handler_manager.cjs directly. The compiler source says one thing; every generated workflow does another.

💡 Impact
  • The new process_safe_outputs.cjs module is never actually executed in CI — deployed workflows still call the old path.
  • Any bug fixes or future changes to process_safe_outputs.cjs will silently have no effect until a recompile happens.
  • Run make recompile and commit the result before merging.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues and one test coverage gap.

📋 Key Themes & Highlights

Issues

  • Encoding dropped in tee (actions/setup/js/process_safe_outputs.cjs line 29/34): stdoutStream.write(chunk) ignores the encoding argument passed by the Node.js stream API, which can silently corrupt multi-byte log output.
  • Silent stream-close errors (line 43): new Promise(resolve => stream.end(resolve)) never rejects — a write error on stream close will be swallowed, leaving a truncated or missing log file with no signal.
  • Weakened regression tests (pkg/workflow/compiler_safe_outputs_steps_test.go): the refactor removed the assertions for process-safe-outputs.stdout.log and process-safe-outputs.stderr.log from TestBuildHandlerManagerStep, so the two artifact paths can now be silently dropped without failing CI.

Positive Highlights

  • ✅ Good extraction of the inline Go string-concat script into a dedicated .cjs module — easier to read and test.
  • finally block correctly restores the original write methods before awaiting stream close.
  • ✅ Artifact upload path additions are minimal and well-placed in buildSafeOutputItemsManifestUploadStep.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 58.9 AIC · ⌖ 4.88 AIC · ⊞ 6.7K
Comment /matt to run again

const originalStderrWrite = process.stderr.write.bind(process.stderr);

process.stdout.write = (chunk, encoding, callback) => {
stdoutStream.write(chunk);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] encoding is dropped when writing to the file stream — stdoutStream.write(chunk) ignores it, which can corrupt multi-byte sequences in the log file.

💡 Suggested fix
process.stdout.write = (chunk, encoding, callback) => {
  stdoutStream.write(chunk, encoding);
  return originalStdoutWrite(chunk, encoding, callback);
};

Same fix needed for stderrStream on line 34.

@copilot please address this.

} finally {
process.stdout.write = originalStdoutWrite;
process.stderr.write = originalStderrWrite;
await Promise.all([new Promise(resolve => stdoutStream.end(resolve)), new Promise(resolve => stderrStream.end(resolve))]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] Stream errors in finally are silently swallowed — if stdoutStream.end() rejects, the Promise.all resolves anyway because the resolve-only callbacks never reject.

💡 Suggested fix
const endStream = (s) =>
  new Promise((resolve, reject) => s.end((err) => (err ? reject(err) : resolve())));

await Promise.all([endStream(stdoutStream), endStream(stderrStream)]);

A failed stream close would silently lose the log file without any signal to the caller.

@copilot please address this.

"setupGlobals",
"safe_output_handler_manager.cjs",
"process_safe_outputs.cjs",
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] After the refactor to process_safe_outputs.cjs, the test no longer asserts that the generated YAML includes the two log paths (process-safe-outputs.stdout.log, process-safe-outputs.stderr.log). This weakens regression protection — the artifact upload step could silently drop the log paths without any test failure.

💡 Suggested fix

Restore the removed assertions in TestBuildHandlerManagerStep:

checkContains: []string{
    ...
    "process_safe_outputs.cjs",
    "process-safe-outputs.stdout.log",
    "process-safe-outputs.stderr.log",
},

Or add them to the integration test in compiler_jobs_test.go if that is the canonical coverage point.

@copilot please address this.

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher

…s_safe_outputs.cjs

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher

Done in the latest commit. Here's a summary of what was addressed:

CI failures fixed (js-typecheck, impacted-js-tests)

  • Fixed TypeScript overload assignment errors on process.stdout.write / process.stderr.write (used /** @type {any} */ named consts, compatible with prettier's formatting)

Stream hardening

  • Encoding forwarded correctly to the file stream (handles both 2-arg write(chunk, cb) and 3-arg write(chunk, encoding, cb) forms)
  • Changed flags: "a"flags: "w" — each run gets a clean log file, no cross-run contamination
  • Added error listeners on streams — degrades gracefully via core.warning instead of crashing
  • endStream helper now rejects on close errors instead of silently swallowing them

Test coverage

  • Added process_safe_outputs.test.cjs (11 tests): success path, rejection path, stdout/stderr tee capture, post-call stream restoration, 2-arg write forwarding, truncate-mode verification
  • Added TestBuildSafeOutputItemsManifestUploadStep in Go to assert the artifact includes process-safe-outputs.stdout.log and process-safe-outputs.stderr.log

Local validation: 0 TypeScript errors, 261 workflows compiled (0 errors), all lock files in sync, all tests pass. CI re-trigger needed for final green check.

@pelikhan

Copy link
Copy Markdown
Collaborator

/smoke-copilot

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing...

@github-actions

Copy link
Copy Markdown
Contributor

Smoke 30145115095

📰 BREAKING: Report filed by Smoke Copilot · gpt54 · 88.8 AIC · ⌖ 7.37 AIC · ⊞ 20.9K ·
Comment /smoke-copilot to run again
Add label smoke to run again

@github-actions

Copy link
Copy Markdown
Contributor

Comment Memory

Automation hums low
Browsers balk, but builds still speak
Signals mark the run

Note

This comment is managed by comment memory.

It stores persistent context for this thread in the code block at the top of this comment.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

📰 BREAKING: Report filed by Smoke Copilot · gpt54 · 88.8 AIC · ⌖ 7.37 AIC · ⊞ 20.9K ·
Comment /smoke-copilot to run again
Add label smoke to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smoke review tools exercised for run 30145115095.

📰 BREAKING: Report filed by Smoke Copilot · gpt54 · 88.8 AIC · ⌖ 7.37 AIC · ⊞ 20.9K
Comment /smoke-copilot to run again
Add label smoke to run again

@pelikhan
pelikhan merged commit f9be509 into main Jul 25, 2026
44 checks passed
@pelikhan
pelikhan deleted the copilot/deep-report-pre-bundle-stdout-stderr branch July 25, 2026 05:16
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.83.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[deep-report] Pre-bundle the Process Safe Outputs step stdout/stderr on safe-output job failure

3 participants