Repository navigation
Skip private MCP cache during artifact redaction - #65368
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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. PR review findings were produced locally, but safeoutputs review-comment writes were blocked by the environment with: Permission denied and could not request permission from user. No GitHub write was applied.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. 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. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Custom artifact declarations can still upload files from the skipped cache without secret redaction.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This PR prevents artifact redaction from failing when gh-aw’s private MCP cache is inaccessible.
Changes:
- Skip
/tmp/gh-aw/aw-mcpduring redaction traversal. - Add regression coverage confirming artifact logs are still redacted.
| File | Description |
|---|---|
| actions/setup/js/redact_secrets.test.cjs | Tests inaccessible-cache handling and artifact redaction. |
| actions/setup/js/redact_secrets.cjs | Excludes the private MCP cache from traversal. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| const fullPath = path.join(dir, entry.name); | ||
| // MCP tool downloads are a private runtime cache, not an artifact source. | ||
| // The sandbox may own this directory with permissions the runner cannot traverse. | ||
| if (entry.isDirectory() && fullPath === "/tmp/gh-aw/aw-mcp") continue; |
There was a problem hiding this comment.
Updated in 4c52bd1: removed the subtree skip so readable MCP logs are redacted. Added coverage for both direct and broad safe-artifact declarations; they remain included because their files are still scanned.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This fix sidesteps the EACCES failure by excluding the entire /tmp/gh-aw/aw-mcp subtree from redaction. That subtree also contains /tmp/gh-aw/aw-mcp/logs/, which multiple workflows populate and many generated workflows upload from /tmp/gh-aw, so this change can let secrets in downloaded logs bypass sanitization entirely.
Blocking theme
The redactor needs to keep covering artifact-bound log files while avoiding the inaccessible private cache. Skip only the truly non-artifact cache leaf or handle EACCES during traversal; do not blackhole the whole aw-mcp tree.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 39.8 AIC · ⌖ 7.01 AIC · ⊞ 20.2K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (via pr-triage). Nice, focused fix with a solid regression test, but the fix is incomplete relative to its own stated root cause.
📋 Key Themes & Highlights
Key Themes
- Incomplete fix scope:
findFilesis invoked against both/tmp/gh-awand${RUNNER_TEMP}/gh-aw(same file, line 356), but the new skip only guards the literal/tmp/gh-aw/aw-mcppath. If the sandbox-owned cache can also land underRUNNER_TEMP, the exact sameEACCEScrash this PR fixes could recur unaddressed. - Magic-string duplication: the hardcoded path duplicates knowledge already encoded in
pkg/constants/constants.go(TmpAwMcpLogsDir) and this file's ownconstants.cjs(TMP_GH_AW_PATH), both of which this project explicitly tries to keep in sync per the file header comment. - Root cause vs. symptom: the actual bug is "sandbox may own a directory with permissions the runner can't traverse" — a path-specific exact-match skip addresses today's known offender, but a more robust fix would catch
EACCES/EPERMduring traversal generically (or skip any directory the runner doesn't own), which would prevent recurrence for other sandbox-owned paths.
Positive Highlights
- ✅ Good regression test: simulates the exact
EACCESfailure mode via areaddirSyncspy and asserts both that the cache is never traversed and that real secrets are still redacted. - ✅ Clear, well-commented rationale directly in the source explaining why the skip exists.
- ✅ Minimal, surgical diff that doesn't touch unrelated redaction logic.
See inline comments for specifics on the RUNNER_TEMP gap and the suggested shared constant.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 74.3 AIC · ⌖ 14.6 AIC · ⊞ 10K
Comment /matt to run again
| const fullPath = path.join(dir, entry.name); | ||
| // MCP tool downloads are a private runtime cache, not an artifact source. | ||
| // The sandbox may own this directory with permissions the runner cannot traverse. | ||
| if (entry.isDirectory() && fullPath === "/tmp/gh-aw/aw-mcp") continue; |
There was a problem hiding this comment.
[/diagnosing-bugs] This fix only skips the hardcoded /tmp/gh-aw/aw-mcp path, but findFiles is also called with ${RUNNER_TEMP}/gh-aw as a root (line 356). If the sandbox mounts an equivalent cache under RUNNER_TEMP, the same EACCES would recur there, unaddressed by this patch.
💡 Suggested fix
Compute both roots explicitly, e.g.:
const SANDBOX_MCP_CACHE_DIRS = new Set([
"/tmp/gh-aw/aw-mcp",
path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/aw-mcp"),
]);
...
if (entry.isDirectory() && SANDBOX_MCP_CACHE_DIRS.has(fullPath)) continue;Also worth considering: rather than a single exact-path check, catching EACCES/EPERM specifically when recursing into subdirectories would guard against any sandbox-owned path (not just this one), which is the actual root cause described in the PR body (a permission error during traversal), not just this one directory.
@copilot please address this.
There was a problem hiding this comment.
Updated in 4c52bd1: EACCES and EPERM from directory reads are handled at any scanned root, including RUNNER_TEMP; readable directories continue to be scanned.
| const fullPath = path.join(dir, entry.name); | ||
| // MCP tool downloads are a private runtime cache, not an artifact source. | ||
| // The sandbox may own this directory with permissions the runner cannot traverse. | ||
| if (entry.isDirectory() && fullPath === "/tmp/gh-aw/aw-mcp") continue; |
There was a problem hiding this comment.
[/codebase-design] The "/tmp/gh-aw/aw-mcp" path is a magic string duplicated from pkg/constants/constants.go's TmpAwMcpLogsDir (/tmp/gh-aw/aw-mcp/logs) and this file's own TMP_GH_AW_PATH constant in constants.cjs. A future rename of the cache dir only updates one side, silently breaking this skip check again.
💡 Suggested fix
This file already imports from ./error_helpers.cjs and ./error_codes.cjs; consider adding an AW_MCP_CACHE_DIR constant to constants.cjs (alongside TMP_GH_AW_PATH) and importing it here, so the Go and JS sides have a single documented source of truth (per the file header comment: "Where a constant has a counterpart in pkg/constants/constants.go, their values should be kept in sync").
@copilot please address this.
There was a problem hiding this comment.
Updated in 4c52bd1: removed the hard-coded cache path check entirely, so there is no duplicated path constant; readable MCP logs continue through normal redaction.
There was a problem hiding this comment.
Review Summary
Small, focused fix: skip the sandbox-owned /tmp/gh-aw/aw-mcp directory during artifact redaction to avoid EACCES failures, with a regression test that simulates the inaccessible cache. Logic is correct and well-tested for the reported failure.
Non-blocking suggestion: the skip only matches the literal path /tmp/gh-aw/aw-mcp, while findFiles is also called against ${RUNNER_TEMP}/gh-aw. Matching by directory name instead of a single absolute literal would make the guard resilient to other base directories passed to findFiles in the future.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 168 AIC · ⌖ 12.1 AIC · ⊞ 8.1K
| const fullPath = path.join(dir, entry.name); | ||
| // MCP tool downloads are a private runtime cache, not an artifact source. | ||
| // The sandbox may own this directory with permissions the runner cannot traverse. | ||
| if (entry.isDirectory() && fullPath === "/tmp/gh-aw/aw-mcp") continue; |
There was a problem hiding this comment.
findFiles is invoked for both /tmp/gh-aw (line 355) and ${RUNNER_TEMP}/gh-aw (line 356), but this skip only matches the literal /tmp/gh-aw/aw-mcp path. The MCP logs guardrail writes to constants.TmpAwMcpLogsDir (/tmp/gh-aw/aw-mcp/logs) unconditionally regardless of topology, so this specific match is safe for that call site — but if any future caller passes a different base dir (e.g. a ${RUNNER_TEMP}/gh-aw equivalent cache), the same EACCES regression would recur unguarded here. Consider matching on path.basename(fullPath) === "aw-mcp" (scoped to direct children of a gh-aw root) instead of a single absolute literal, so the fix generalizes to any base directory findFiles is called with.
@copilot please address this.
There was a problem hiding this comment.
Updated in 4c52bd1: the permission-error handling is root-independent and applies to any inaccessible directory, rather than matching one aw-mcp path.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed the pr-finisher pass and pushed |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The Daily Agent of the Day agent job failed when secret redaction hit
EACCESwhile traversing the sandbox-owned/tmp/gh-aw/aw-mcpdirectory.The run also reported a separate pull-request creation failure; this change addresses the redaction failure only.