Render pre-agent workspace audit as a step summary table - #50103
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot the title should be in the summary. See other templates |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The changes are shell script updates (audit_pre_agent_workspace.sh and audit_pre_agent_workspace_test.sh). Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a collapsible pre-agent workspace audit table to GitHub Actions step summaries.
Changes:
- Renders audited paths, sections, and sizes as a Markdown table.
- Adds summary rendering and unset-variable tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/audit_pre_agent_workspace.sh |
Generates and appends the audit summary table. |
actions/setup/sh/audit_pre_agent_workspace_test.sh |
Tests summary output and optional summary handling. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
| rows+="| ${section} | \`${line}\` | ${size} |"$'\n' | ||
| count=$((count + 1)) | ||
| done < "${AUDIT_FILE}" | ||
| echo "<details>" |
| elif [ -d "${line}" ]; then | ||
| size="dir" | ||
| fi | ||
| rows+="| ${section} | \`${line}\` | ${size} |"$'\n' |
| # HOME - agent user home directory | ||
| # RUNNER_TEMP - runner temporary directory | ||
| # GITHUB_OUTPUT - path to the GitHub Actions output file | ||
| # GITHUB_STEP_SUMMARY- path to the GitHub Actions step summary file (optional) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Render pre-agent workspace audit as a step summary table
The implementation is clean, well-structured, and the tests are thorough. One minor test-isolation issue flagged inline — otherwise no blocking concerns.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.2 AIC · ⌖ 18.1 AIC · ⊞ 5.4K
| if GITHUB_WORKSPACE="${TEST_ROOT}/workspace-8" HOME="${TEST_ROOT}/home9" RUNNER_TEMP="${TMPDIR}" GITHUB_OUTPUT="${GH_OUT9}" bash "${SCRIPT_PATH}" >/dev/null 2>&1; then | ||
| assert "Script succeeds without GITHUB_STEP_SUMMARY" "true" | ||
| else | ||
| assert "Script succeeds without GITHUB_STEP_SUMMARY" "false" |
There was a problem hiding this comment.
Test 9 reuses workspace-8 created by Test 8, creating an implicit ordering dependency. If Test 8 were removed or failed before creating that directory, Test 9 would fail unexpectedly. Consider creating a dedicated workspace-9 directory in Test 9 for full isolation.
@copilot please address this.
There was a problem hiding this comment.
Verdict: Request changes
The table-rendering feature is a reasonable addition, but render_summary_table has an unguarded wc -c call inside a set -euo pipefail context that will abort the script (and skip the required GITHUB_OUTPUT writes) if it encounters an unreadable file during the walk.
Themes
- Blocking:
size="$(wc -c < "${line}" 2>/dev/null | tr -d ' ')"can propagate a non-zero exit status underset -eeven with stderr suppressed, verified locally with achmod 000file — this crashes beforepre-agent-audit-file/pre-agent-audit-line-countoutputs are written, silently breaking downstream steps that read them. - No test covers the permission-denied / race-condition path, so this regression wouldn't be caught by the new test suite.
- Everything else (details/summary wrapping, section parsing,
GITHUB_STEP_SUMMARYfallback to/dev/null) is implemented cleanly and matches the stated style.
🔎 Code quality review by PR Code Quality Reviewer · auto · 36.3 AIC · ⌖ 4.51 AIC · ⊞ 7.9K
Comment /review to run again
| esac | ||
| local size="-" | ||
| if [ -f "${line}" ]; then | ||
| size="$(wc -c < "${line}" 2>/dev/null | tr -d ' ')" |
There was a problem hiding this comment.
Under set -euo pipefail, a permission-denied or race-condition file read here will abort the whole script mid-loop, silently skipping the required GITHUB_OUTPUT writes on lines 143-144 that downstream steps depend on.
💡 Details and fix
find (line ~65) can list files owned by another user, mode 000, or deleted between the find scan and this read (race). Because size="$(wc -c < "${line}" 2>/dev/null | tr -d ' ')" is a command substitution assigned to a variable, bash's set -e treats a failure in the substitution's pipeline as fatal even though stderr is redirected to /dev/null — the 2>/dev/null only suppresses the error message, not the non-zero exit status of wc. I verified this locally: a chmod 000 file inside a scanned directory causes the script to exit with status 1 partway through render_summary_table, before pre-agent-audit-file / pre-agent-audit-line-count are ever written to GITHUB_OUTPUT. Since the workflow step uses continue-on-error, the job won't fail loudly, but any downstream step reading those outputs will silently get empty/undefined values.
Fix by not letting the substitution's exit status propagate:
if [ -f "${line}" ]; then
size="$(wc -c < "${line}" 2>/dev/null | tr -d ' ')" || size="?"
elif [ -d "${line}" ]; then
size="dir"
fior wrap the whole render_summary_table call with || true so a rendering failure never blocks the critical GITHUB_OUTPUT writes that follow it.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on a missing heading and undertested assertions.
📋 Key Themes & Highlights
Issues
- Missing
###heading: The PR description example shows a### Pre-agent workspace auditheading above<details>, but the implementation emits<details>as its very first line. Without it the table is invisible in a busy step summary (matching the pattern used bylog_runtime_features_summary.sh). - Undertested assertions: Test 8 checks that
agent.mdappears in the summary but doesn't verify the Section column value, the numeric size, or the row count in the<summary>tag — easy regressions to miss. - Undocumented symlink behaviour: Paths that are neither files nor directories (symlinks, disappeared paths) produce a silent
| section | /path | - |row; a comment or test would document the intentional behaviour.
Positive Highlights
- ✅ Clean progressive-disclosure structure (
<details>/<summary>) matching the project's existing style - ✅ Graceful
/dev/nullfallback for non-Actions environments - ✅ Good test structure — two focused tests covering the happy path and the unset-env-var edge case
- ✅ Section-marker parsing is correct and the
%%:*extraction idiom is idiomatic Bash
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 52 AIC · ⌖ 9.85 AIC · ⊞ 7.1K
Comment /matt to run again
| rows+="| ${section} | \`${line}\` | ${size} |"$'\n' | ||
| count=$((count + 1)) | ||
| done < "${AUDIT_FILE}" | ||
| echo "<details>" |
There was a problem hiding this comment.
[/grill-with-docs] The function emits <details> as its first output line, but the PR description example shows a ### Pre-agent workspace audit heading above it — matching the pattern used by log_runtime_features_summary.sh. Without the heading the table is unfindable in a busy step summary.
💡 Suggested fix
render_summary_table() {
echo "### Pre-agent workspace audit"
echo
echo "<details>"
...The test assertion on <summary>Pre-agent workspace audit is then also underconstrained — add a separate assertion for the heading.
@copilot please address this.
| section="${section%%:*}" | ||
| continue | ||
| ;; | ||
| "--- "*" ---") continue ;; |
There was a problem hiding this comment.
[/tdd] The size field falls back to "-" for paths that are neither files nor directories (e.g., symlinks, sockets, or paths that disappear between write and read). This produces a silent, misleading | section | /path | - | row. A test covering a symlink entry — or at minimum a comment noting the limitation — would make the intent explicit.
@copilot please address this.
| assert "Summary closes details tag" "grep -q '</details>' '${SUMMARY8}'" | ||
| assert "Summary has table header" "grep -q '| Section | Path | Size |' '${SUMMARY8}'" | ||
| assert "Summary lists agent.md row" "grep -q 'agent.md' '${SUMMARY8}'" | ||
| assert "Summary omits section marker lines" "! grep -q -- '--- Workspace agents' '${SUMMARY8}'" |
There was a problem hiding this comment.
[/tdd] Test 8 only asserts that agent.md appears in the summary, not that it has the correct Section column value (Workspace agents) or a numeric size. Without those assertions a future change that corrupts section parsing or the size column would pass this test undetected.
💡 Stronger assertions
assert "Summary row has correct section" "grep -q '| Workspace agents |' '${SUMMARY8}'"
assert "Summary row has numeric size" "grep -qE 'agent\.md.*\| [0-9]+' '${SUMMARY8}'"@copilot please address this.
| printf '%s' "${rows}" | ||
| echo | ||
| echo "</details>" | ||
| } |
There was a problem hiding this comment.
[/tdd] The row-count label in <summary> (e.g., "Pre-agent workspace audit (3 entries)") is never tested — Test 8 only checks for the prefix. If the count is zero or wrong no test will catch it.
💡 Add count assertion
assert "Summary entry count is correct" "grep -q 'Pre-agent workspace audit (2 entries)' '${SUMMARY8}'"(adjust the expected count to match the test fixture)
@copilot please address this.
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
The pre-agent audit script wrote its file listing to
/tmp/gh-aw/pre-agent-audit.txtand aGITHUB_OUTPUTvalue only, so the audited files were invisible in the run's step summary. This adds a collapsible table rendering, matching the progressive disclosure style used by other summary sections (e.g.log_runtime_features_summary.sh).Changes
actions/setup/sh/audit_pre_agent_workspace.sh: parses the audit file into a| Section | Path | Size |table — section markers (--- label: dir ---) become the Section column,(not found)and blank lines are skipped, and sizes are reported for regular files (dirfor directories).<details>with a row-count label, appended to$GITHUB_STEP_SUMMARY; falls back to/dev/nullwhen unset, so local/non-Actions invocations are unaffected.audit_pre_agent_workspace_test.sh: covers heading,<details>wrapping, table header, row content, suppression of raw section markers, and the unset-GITHUB_STEP_SUMMARYpath.Example output:
No compiler or workflow changes were needed — the step already delegates entirely to this script.