Repository navigation
Conversation
Emit gateway stderr through core.info during shutdown and explicitly exclude the legacy stderr path from artifact uploads. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1554a64b-2d03-4321-819f-4d590e44e95b
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…err-artifact Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Regenerate workflow lock files with the core.info shutdown logger and stderr artifact exclusion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1554a64b-2d03-4321-819f-4d590e44e95b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1554a64b-2d03-4321-819f-4d590e44e95b
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (35 additions detected). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Updates MCP gateway shutdown and artifact handling to avoid retaining gateway stderr in artifacts.
Changes:
- Captures gateway stderr through the shutdown action instead of a file.
- Excludes the legacy stderr path, including ARC/DinD rewrites.
- Regenerates affected workflow lock files and adds tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/start_mcp_gateway.cjs |
Stops persisting gateway stderr. |
actions/setup/js/stop_mcp_gateway.cjs |
Emits stderr during shutdown. |
actions/setup/js/stop_mcp_gateway.test.cjs |
Tests stderr-only emission. |
pkg/constants/constants.go |
Defines the legacy stderr path. |
pkg/workflow/compiler_yaml_ai_execution.go |
Uses the JavaScript shutdown action. |
pkg/workflow/compiler_yaml_post_agent.go |
Excludes stderr from artifacts. |
pkg/workflow/arc_dind_artifacts.go |
Rewrites negated ARC/DinD paths. |
pkg/workflow/arc_dind_artifacts_test.go |
Tests negated-path rewriting. |
pkg/workflow/mcp_logs_upload_test.go |
Tests stderr exclusion and shutdown generation. |
.github/workflows/*.lock.yml |
Regenerates affected workflows with the new shutdown step and artifact exclusion. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 284/284 changed files
- Comments generated: 3
- Review effort level: Balanced
| } | ||
|
|
||
| async function main() { | ||
| await printGatewayStderr(exec, core); |
There was a problem hiding this comment.
Fixed in the latest commit. main() now runs stop_mcp_gateway.sh first, then calls docker logs awmg-mcpg after the container is stopped (the EXIT trap issues docker stop, leaving the container available). The container is then explicitly removed with docker rm after the stderr has been collected.
| // Container stderr is retrieved from Docker and emitted through core.info | ||
| // by stop_mcp_gateway.cjs after agent execution. Never persist it in mcp-logs, | ||
| // because it can contain credentials that must not enter the agent artifact. | ||
| stdio: ["pipe", outputFd, "ignore"], |
There was a problem hiding this comment.
Fixed. After the stop script runs, collectGatewayStderr in stop_mcp_gateway.cjs writes the Docker stderr to /tmp/gh-aw/mcp-logs/stderr.log (the path already excluded from artifacts by !constants.TmpMcpStderrLog). parse_mcp_gateway_log.cjs still reads from that path for ai_credits_rate_limit_error and unknown_model_ai_credits detection — the parser flow is fully preserved.
|
|
||
| const stderr = result.stderr.trim(); | ||
| if (stderr) { | ||
| coreApi.info(`MCP Gateway stderr:\n${stderr}`); |
There was a problem hiding this comment.
Fixed. The core.info call that printed raw stderr has been removed. The stop step now only logs the byte count (MCP Gateway stderr written to log (N bytes).), never the content. The raw stream is written directly to the excluded file path without any emission to the Actions log.
…t' into copilot/avoid-mcp-stderr-artifact Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. PR Review: Avoid storing MCP gateway stderr in artifacts
One minor observation worth addressing in a follow-up: In it("reports when stderr is unavailable", async () => {
const execApi = {
getExecOutput: vi.fn().mockResolvedValue({ exitCode: 1, stdout: "", stderr: "" }),
};
const coreApi = { info: vi.fn() };
await printGatewayStderr(execApi, coreApi);
expect(coreApi.info).toHaveBeenCalledWith("MCP Gateway stderr is unavailable.");
});...would complete the coverage of all three branches per @copilot please address the review comments above.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — well-structured security fix. Approving with a minor test coverage note.
Positive Highlights
- ✅ Stderr redirected to
"ignore"on spawn — root cause eliminated - ✅
!stderr.logexclusion protects against stale files from older runners - ✅
docker logsseparation viagetExecOutput+silent: truepreserves diagnostics without leaking credentials - ✅
printGatewayStderris injected (not global) — clean and testable - ✅ ARC/DinD path rewriting correctly handles
!prefix> 🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 58.6 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Avoid storing MCP gateway stderr in artifacts
This is a well-scoped, security-focused change. The implementation is correct and test coverage is thorough.
What the PR does
- Eliminates credentials leak vector: Gateway stderr was written to
/tmp/gh-aw/mcp-logs/stderr.logand included in job artifacts. Since gateway stdout can contain generated client configuration (API keys, ports), routing stderr to an artifact-included file is a latent credential-leakage risk. The PR stops this. - Redirects stderr to
core.infoat shutdown: The newstop_mcp_gateway.cjscallsdocker logs awmg-mcpgwithsilent: trueand emits onlyresult.stderrviacore.info, correctly ignoringresult.stdout. - Adds artifact exclusion as defense-in-depth:
!/tmp/gh-aw/mcp-logs/stderr.logis appended tocollectArtifactPathsso a stale or third-party file at the legacy path is never uploaded. - Fixes ARC/DinD path rewriting for negated globs:
rewriteTmpGhAwPathsForArcDindnow strips the leading!before prefix substitution, preserving the negation — test asserts!${{ runner.temp }}/gh-aw/mcp-logs/stderr.log.
Quality observations
- All five error-path branches in
start_mcp_gateway.cjsare updated consistently. stop_mcp_gateway.test.cjsexplicitly asserts that stdout (the API key fixture) never appears incore.infocalls — directly guarding the security property.TmpMcpStderrLogconstant is introduced so the path is defined in one place.- Lock files are regenerated as expected.
No blocking issues found.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 59.6 AIC · ⊞ 5.3K
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report🔶 Test Quality Score: 80/100 — Excellent (failed on violation)
📊 Metrics (7 tests)
|
|
@copilot Run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
REQUEST_CHANGES: gateway stderr redaction regressed
This PR removes the stderr-to-artifact leak but introduces a worse one: gateway stderr (explicitly documented as possibly containing credentials) is now piped straight to the Actions log via core.info, which is never scrubbed by the redact_secrets step. That's a regression, not a fix, for the stated goal of not exposing secrets.
Themes
- Correctness/security:
printGatewayStderrbypasses the existing secret-redaction mechanism (file-based scrubbing only), exposing potential credentials directly in the live log output — a step backward from hiding them in an excluded artifact file. - Minor: exclusion-glob ordering for the new
!/tmp/gh-aw/mcp-logs/stderr.logpattern depends on append order across several code paths; worth double-checking there's no later insertion that could shadow it. - Positive: the ARC/DinD negation-prefix handling in
rewriteTmpGhAwPathsForArcDindand the added tests are clean and correctly preserve the!prefix through rewriting.
Comments that could not be inline-anchored
actions/setup/js/stop_mcp_gateway.cjs:22
Gateway stderr is printed straight into the Actions log via core.info, before the redaction step runs and outside its file-scan mechanism — so any credential it contains reaches unmasked, unredacted output.
<details><summary>Why this matters and a fix</summary>
The redact_secrets.cjs step (which runs after Stop MCP Gateway in the generated workflow) only scans files under /tmp/gh-aw and ${RUNNER_TEMP}/gh-aw for known secret values and replaces them in-place; it never touches text already …
pkg/workflow/compiler_yaml_post_agent.go:289
The exclusion glob is appended after the directory path it needs to negate, and upload-artifact/glob ordering for negation patterns is fragile — verify the generated YAML actually places !.../stderr.log after /tmp/gh-aw/mcp-logs/ in the final include list for every code path (including ARC/DinD rewriting), not just the common case.
<details><summary>Details</summary>
actions/upload-artifact (via @actions/glob) evaluates patterns in file order, so a negation pattern only takes effect i…
Done. Fixed the three CI failures from the prior run:
All local tests pass. Note: the |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot PR still needs follow-up before a maintainer can investigate quickly. Please address these unresolved review threads, newest first:
There is also a pending commit status on the current head SHA, so please wait for checks to settle after you push fixes. After resolving the review feedback, run the pr-finisher skill.
|
…d log path, remove container Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all three review threads in the latest commit:
|
| fs.mkdirSync(path.dirname(MCP_STDERR_LOG_PATH), { recursive: true }); | ||
| fs.writeFileSync(MCP_STDERR_LOG_PATH, stderr, { mode: 0o600 }); | ||
| coreApi.info(`MCP Gateway stderr written to log (${stderr.length} bytes).`); |
There was a problem hiding this comment.
@copilot do not write log to file, instead use core.debug. DO NOT WRITE THE LOG TO A FILE.
There was a problem hiding this comment.
Done. collectGatewayStderr now calls core.debug instead of writing to a file. The fs dependency and all file-write logic have been removed.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ty_commit tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed. Added |
Summary
/tmp/gh-aw/mcp-logs/stderr.logcore.infoduring gateway shutdown without logging stdout configurationValidation
make agent-report-progresscomponents passed (build, JavaScript lint, schema freshness, impacted Go and JavaScript tests)golangci-lint run ./pkg/constants ./pkg/workflowWarning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.