Add debug-scoped /tmp/gh-aw file inventory to setup post cleanup#38780
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
/tmp/gh-aw file inventory to setup post cleanup
|
Hey A couple of things worth addressing before this lands:
If you would like to address these, here is a prompt you can assign to your coding agent:
|
There was a problem hiding this comment.
Pull request overview
This PR improves troubleshooting of setup post-cleanup by emitting a bounded /tmp/gh-aw file inventory only when GitHub Actions debug mode is enabled, while keeping the cleanup behavior unchanged. It also includes a small Go lint-driven tweak and a workflow hardening update to avoid hard-coded home paths.
Changes:
- Add debug-gated, bounded, sorted
/tmp/gh-awfile inventory logging inactions/setup/post.jsbefore cleanup. - Replace a simple
fmt.Sprintfusage with string concatenation inpkg/workflow/action_resolver.goto satisfy linting. - Update the compiled smoke workflow to use
$HOMEfor Copilot config paths and export related env vars in the run script.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/action_resolver.go |
Simplifies GitHub API path construction for resolving --gh-aw-ref. |
actions/setup/post.js |
Adds debug-only, bounded /tmp/gh-aw inventory output prior to cleanup. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Switches Copilot config paths to $HOME and adjusts related env handling. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
| return; | ||
| } | ||
|
|
||
| entries.sort((a, b) => a.name.localeCompare(b.name)); |
| const walk = (currentDir, depth) => { | ||
| if (depth >= maxDepth || files.length >= maxFiles) { | ||
| return; |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38780 does not have the 'implementation' label (has_implementation_label=false) and has only 9 new lines in business logic directories, well under the 100-line threshold (requires_adr_by_default_volume=false). |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in PR #38780. The PR changes actions/setup/post.js (production), pkg/workflow/action_resolver.go (production), and a workflow lock file. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnose, and /zoom-out — commenting with non-blocking observations.
📋 Key Themes & Highlights
Key Themes
- Missing unit tests:
listTmpGhAwFilesandisDebugModeEnabledare entirely untested. The functions are not exported, so testing them requires a minor refactor (extract to a module or add a conditionalmodule.exports). - Depth truncation is silent:
maxDepthcutoff leaves no trace in the debug output — onlymaxFilesoverflow emits a signal. Adding adepthCappedflag and a companion log line would complete the observability story. - DFS traversal may produce a misleading truncated listing: the 200-file cap can be consumed entirely inside a deeply nested subdir, omitting all shallow-level siblings. For a pre-cleanup inventory, files-before-dirs ordering is more useful.
Positive Highlights
- ✅ Debug gating (
isDebugModeEnabled) correctly normalises"1"and"true"and checks bothRUNNER_DEBUGandACTIONS_STEP_DEBUG. - ✅ Bounded limits (
maxDepth=4,maxFiles=200) and read-error reporting prevent the listing from becoming noisy or hanging on permission-denied paths. - ✅ Alphabetical sort on
entriesmakes the output deterministic — great for diffing across runs. - ✅ The
action_resolver.golint fix is clean and correct; the lock-file$HOMEportability change is a solid improvement.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 209.2 AIC · ⌖ 13.8 AIC · ⊞ 29.3K
| }; | ||
| return toBool(process.env.RUNNER_DEBUG) || toBool(process.env.ACTIONS_STEP_DEBUG); | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] listTmpGhAwFiles (and isDebugModeEnabled) have no unit tests — edge-case bugs here will surface as silent debug-output gaps rather than test failures.
💡 Key cases to cover
| Scenario | Why it matters |
|---|---|
tmpDir does not exist |
guard branch already in code — should be verified |
| permission-denied on a subdirectory | readErrors counter path |
exactly maxFiles entries |
off-by-one on the truncation flag |
maxDepth = 0 |
walk should not recurse at all |
RUNNER_DEBUG=1 vs RUNNER_DEBUG=true |
both should enable debug mode |
Because the functions are not exported they would need a light refactor (e.g. module.exports = { listTmpGhAwFiles, isDebugModeEnabled } behind a test guard, or extract into a debug-inventory.js module) before a test file can import them.
| const files = []; | ||
| let readErrors = 0; | ||
|
|
||
| const walk = (currentDir, depth) => { |
There was a problem hiding this comment.
[/diagnose] The truncation signal only fires when maxFiles is hit, not when maxDepth silently caps recursion. A reader of the debug output has no way to know that subdirectories beyond depth 4 were skipped.
💡 Suggested fix
Track whether any directory was skipped due to depth, and emit a companion signal:
const walk = (currentDir, depth) => {
if (depth >= maxDepth) {
depthCapped = true; // new flag
return;
}
if (files.length >= maxFiles) {
return;
}
// ...
};Then after the walk:
if (depthCapped) {
console.log(`[debug] some subdirectories beyond depth ${maxDepth} were not listed`);
}This keeps the two truncation reasons distinct and both observable.
| return; | ||
| } | ||
|
|
||
| const fullPath = path.join(currentDir, entry.name); |
There was a problem hiding this comment.
[/zoom-out] DFS-first traversal means the 200-file limit can be exhausted entirely inside a deep subdirectory, silently omitting all sibling files at shallower levels. For a debug inventory whose purpose is to show what was left before cleanup, breadth-first (or listing files before recursing) would give a more representative snapshot.
💡 Files-before-dirs alternative
A minimal change: process file entries in the current directory before recursing into subdirs.
// separate dirs and files
const dirs = entries.filter(e => e.isDirectory());
const fileEntries = entries.filter(e => !e.isDirectory());
for (const entry of fileEntries) {
if (files.length >= maxFiles) return;
files.push(path.relative(tmpDir, path.join(currentDir, entry.name)));
}
for (const dir of dirs) {
if (files.length >= maxFiles) return;
walk(path.join(currentDir, dir.name), depth + 1);
}This ensures the root-level (most actionable) files always appear, regardless of how many nested files exist.
|
@copilot run pr-finisher skill |
Ran a pr-finisher pass on PR #38780.
Actions taken: collected review/CI state and validated locally with no file changes. |
When Actions debug mode is enabled, setup post-cleanup lacked visibility into what was left under
/tmp/gh-awbefore deletion. This updates the post step to emit a bounded file inventory so debugging cleanup/state issues is faster without noisy default logs.Debug-only
/tmp/gh-awinventoryRUNNER_DEBUGorACTIONS_STEP_DEBUG.Bounded and stable output
maxDepth = 4,maxFiles = 200).Cleanup behavior unchanged
sudo rm -rfwithfs.rmSyncfallback).Incidental repo-required lint cleanup
fmt.Sprintf("/repos/github/gh-aw/commits/%s", ref)with string concatenation inpkg/workflow/action_resolver.goto satisfy enforced linting in required progress gates.