feat(harness): report categorized post-script failures on issue/PR

[ifireball]

Summary

Ports fullsend-ai/fullsend#2921: categorized post-script failure reporting for post-code.sh and post-fix.sh.

Failure reporting

• Add shared scripts/lib/post-failure-report.lib.sh with categorized failure comments, output sanitization (tokens/PEM redaction, GHA workflow-command stripping), and workflow run links
• post-code.sh posts detailed failure comments on the issue (push rejected, pre-commit blocked, secret scan, PR creation failed, etc.)
• post-fix.sh posts equivalent failure comments on the PR (previously silent on many post-script failures)
• Workflow-permission push rejections are called out explicitly as environmental limitations
• Extend post-code-test.sh / post-fix-test.sh and add post-failure-report-test.sh for comment building, sanitization, org-mode URL routing, and push categorization

Script bundling (added during review)

The harness fetches each runner script as an isolated blob, so post-scripts cannot source files from scripts/lib/ at runtime (same constraint that motivated the CSMA lib fix for post-prioritize.sh). To keep shared failure-reporting logic DRY without breaking runtime, this PR adds a small build stage:

Kind	Path	Edit?
Library	scripts/lib/*.lib.sh	Yes — functions only
Source	scripts/*.src.sh	Yes — editable script with source calls
Bundled	scripts/*.sh (from .src.sh)	No — generated; referenced by harness

Build tooling:

• scripts/bundle-sh.sh — inlines scripts/lib/*.lib.sh into self-contained .sh artifacts (nested sources, deduped)
• make script-build / make check-bundle — regenerate and verify committed bundles
• CI (script-test.yml) runs check-bundle, then script-test in both source and bundled modes

Scripts converted to the source→bundle workflow:

• post-code.src.sh → post-code.sh
• post-fix.src.sh → post-fix.sh
• post-prioritize.src.sh → post-prioritize.sh (re-extracts github-api-csma.lib.sh from the prior inline copy)

After editing a .src.sh or .lib.sh file, run make script-build and commit source + bundled files together. See README “Script bundling” section.

Test plan

• make script-build && make check-bundle
• make script-test (source mode)
• make script-test SCRIPT_TEST_TARGET=bundled
• bash scripts/post-failure-report-test.sh
• CI green on f9542d5

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:39 AM UTC · Completed 7:54 AM UTC
Commit: c1a58d8 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Report categorized post-script failures on issues/PRs (shared failure reporter)

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Add shared post-script failure reporter with categorization, sanitization, and run links.
• Post detailed failure comments to the issue/PR for push, scan, and hook failures.
• Expand harness tests to cover categories, redaction, and push-rejection classification.

Diagram

graph TD
A["scripts/post-code.sh"] --> G{{"Push/scan/hooks"}} --> L[["scripts/lib/post-failure-report.sh"]] --> I(("Issue comment"))
B["scripts/post-fix.sh"] --> G --> L --> P(("PR comment"))
L --> R{{"Workflow run link"}}
subgraph Legend
direction LR
_s[Script] ~~~ _lib[[Library]] ~~~ _ext{{External/Tool}} ~~~ _tgt((Comment target))
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep per-script bespoke error comments

• ➕ No new shared library surface area
• ➕ Each script can tailor messaging without shared constraints
• ➖ Duplication and drift between post-code and post-fix
• ➖ Harder to keep sanitization/redaction consistent everywhere
• ➖ More test duplication

2. Implement as a composite GitHub Action / reusable workflow

• ➕ Centralizes behavior at the workflow layer (less bash plumbing)
• ➕ Easier to standardize permissions, environment, and outputs
• ➕ Can be reused across repos with versioned releases
• ➖ Higher migration cost and requires workflow refactors
• ➖ Harder to unit test locally compared to pure bash scripts
• ➖ May reduce flexibility for repo-specific logic

3. Use GitHub API directly (curl) instead of `gh`

• ➕ Avoids dependency on GitHub CLI availability/behavior
• ➕ More explicit control over auth and error handling
• ➖ More code and more auth/header boilerplate
• ➖ Higher likelihood of subtle API/JSON handling bugs in bash

Recommendation: The shared post-failure-report.sh approach is the best fit here: it removes duplicated comment construction, enforces consistent sanitization, and makes failure categories (notably workflow-permission limitations) visible to users. Consider a composite action only if multiple workflows/repos need the same reporting logic beyond these harness scripts.

Files changed (5) +507 / -173

Enhancement (3) +342 / -118

post-failure-report.shAdd shared categorized failure reporting + sanitization utilities +240/-0

Add shared categorized failure reporting + sanitization utilities

• Introduces a reusable library that categorizes post-script failures, redacts sensitive output (tokens/PEM keys), truncates noisy logs, and builds consistent issue/PR comment bodies including workflow run links. Provides helpers to post best-effort comments to issues or PRs and convenience functions to set category/detail and exit.

scripts/lib/post-failure-report.sh

post-code.shPost categorized failure comments to the issue on post-code errors +52/-79

Post categorized failure comments to the issue on post-code errors

• Replaces bespoke ERR-trap issue commenting with the shared failure reporter and sets a global ERR trap to report failures. Converts several hard exits into categorized 'post_fail_to_issue' calls (branch validation, gitleaks/secret scan, signed-off-by, pre-commit failures, push failures, PR creation failures) and captures tool output for inclusion in sanitized comments.

scripts/post-code.sh

post-fix.shPost categorized failure comments to the PR on post-fix errors +50/-39

Post categorized failure comments to the PR on post-fix errors

• Adds shared failure reporter sourcing and sets an ERR trap to comment on the PR when post-fix fails. Replaces silent/uncategorized exits across secret scan, signed-off-by, pre-commit, push, result scanning, and structured output processing with categorized 'post_fail_to_pr' calls and captured output for sanitized reporting.

scripts/post-fix.sh

Tests (2) +165 / -55

post-code-test.shRefactor failure comment tests to use shared library + add redaction/categorization coverage +106/-55

Refactor failure comment tests to use shared library + add redaction/categorization coverage

• Switches tests from an inline comment builder to 'build_post_failure_comment' from the shared library. Adds new cases for category labeling (push rejected, workflow permission, secret scan, pre-commit) plus sanitization and push-failure categorization tests.

scripts/post-code-test.sh

post-fix-test.shAdd PR failure comment tests for post-fix using shared library +59/-0

Add PR failure comment tests for post-fix using shared library

• Adds test coverage ensuring post-fix failure comments include correct category labeling, workflow-permission environmental notes, generic secret-scan messaging, retry hint ('/fs-fix'), and workflow run link presence, while avoiding leaking secret scan findings.

scripts/post-fix-test.sh

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

1. Protected scripts/ paths changed ✗ Dismissed 📜 Skill insight § Compliance

Description

This PR modifies protected governance/infrastructure paths under scripts/, which must always be
flagged for required human review and must not be auto-approved. These changes affect post-script
behavior and failure reporting in automation.

Code

scripts/post-code.sh[R40-46]

+SCRIPT_DIR_POST="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+# shellcheck source=lib/post-failure-report.sh
+source "${SCRIPT_DIR_POST}/lib/post-failure-report.sh"
+
# ---------------------------------------------------------------------------
# Configuration
# ---------------------------------------------------------------------------

Evidence

PR Compliance ID 1538392 mandates raising a finding whenever protected paths (including scripts/)
are modified. The diff shows new/modified automation scripts under scripts/ (e.g.,
scripts/post-code.sh) including new sourcing/trap behavior.

scripts/post-code.sh[40-60]
Skill: pr-review

---

2. ISSUE_NUMBER in ::warning unsanitized ✓ Resolved 📜 Skill insight ⛨ Security

Description

The new failure-reporting library emits GitHub Actions workflow commands (::warning::...) with
interpolated values that are not sanitized. Unsanitized interpolation can allow workflow-command
injection via ::, encoded newlines (%0A/%0D), or control characters.

Code

scripts/lib/post-failure-report.sh[R193-223]

+  echo "::warning::Posting failure comment to issue #${ISSUE_NUMBER}..."
+  gh issue comment "${ISSUE_NUMBER}" \
+    --repo "${REPO_FULL_NAME}" \
+    --body "${body}" 2>/dev/null || \
+    echo "::warning::Failed to post error comment to issue #${ISSUE_NUMBER}"
+}
+
+report_post_failure_to_pr() {
+  local exit_code="${1:-$?}"
+
+  if [ "${POST_FAILURE_REPORTED}" = "true" ]; then
+    return 0
+  fi
+  POST_FAILURE_REPORTED=true
+
+  _post_failure_ensure_token
+
+  local category="${POST_FAILURE_CATEGORY:-post-script-error}"
+  local detail="${POST_FAILURE_DETAIL:-Post-fix script failed before push or PR update completed.}"
+  local body
+  # PR_NUMBER and REPO_FULL_NAME are required by post-fix.sh before sourcing.
+  # shellcheck disable=SC2153
+  body="$(build_post_failure_comment \
+    "fix" "${exit_code}" "${category}" "${detail}" \
+    "${REPO_FULL_NAME}" "/fs-fix")"
+
+  echo "::warning::Posting failure comment to PR #${PR_NUMBER}..."
+  gh pr comment "${PR_NUMBER}" \
+    --repo "${REPO_FULL_NAME}" \
+    --body "${body}" 2>/dev/null || \
+    echo "::warning::Failed to post error comment to PR #${PR_NUMBER}"

Evidence

PR Compliance ID 1538382 requires individually sanitizing every interpolated variable used in GitHub
Actions workflow commands. The new code echoes ::warning::... messages containing interpolated
ISSUE_NUMBER / PR_NUMBER (and other dynamic content) without any GHA-command sanitization.

scripts/lib/post-failure-report.sh[193-223]
Skill: pr-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`scripts/lib/post-failure-report.sh` prints GitHub Actions workflow commands (e.g., `::warning::...`) that include interpolated variables (e.g., `ISSUE_NUMBER`, `PR_NUMBER`) without sanitizing each variable for workflow-command injection vectors (`::`, `%0A/%0D`, ANSI/control chars).

## Issue Context
Compliance requires that **every interpolated value** in a GitHub Actions workflow command be individually sanitized, not just some values or only "likely safe" values.

## Fix Focus Areas
- scripts/lib/post-failure-report.sh[193-223]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Markdown fence injection ✓ Resolved 🐞 Bug ≡ Correctness

Description

build_post_failure_comment embeds sanitized failure output inside a triple-backtick fenced block
without escaping embedded ``` sequences, so certain command outputs can prematurely terminate the
fence and corrupt the rendered issue/PR comment. This is reachable because post-code.sh/post-fix.sh
pass raw command output (push/pre-commit/etc.) into the failure detail that becomes the comment
body.

Code

scripts/lib/post-failure-report.sh[R138-145]

+  if [ -n "${sanitized_detail}" ]; then
+    detail_block="$(cat <<EOF
+
+**Details:**
+\`\`\`
+${sanitized_detail}
+\`\`\`
+EOF

Evidence

The comment builder inserts ${sanitized_detail} directly between literal ``` fences, but the
sanitizer only redacts token patterns and PEM blocks and does not escape backticks, so the Markdown
structure can be broken by outputs containing ```.

scripts/lib/post-failure-report.sh[38-56]
scripts/lib/post-failure-report.sh[138-146]
scripts/post-code.sh[382-401]
scripts/post-fix.sh[300-319]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`build_post_failure_comment()` builds a Markdown comment that wraps `${sanitized_detail}` in a triple-backtick fenced code block. If `${sanitized_detail}` contains ``` (or an unmatched fence-like sequence), the fence can be closed early and the rest of the comment renders incorrectly.

### Issue Context
Although the content is token-redacted, it is still untrusted command output (e.g., git push / pre-commit output) and may contain arbitrary text.

### Fix Focus Areas
- scripts/lib/post-failure-report.sh[118-166]

### Suggested fix
Update `build_post_failure_comment()` to avoid fenced blocks entirely by rendering details as an indented code block:
- Build `detail_block` by prefixing every line of `${sanitized_detail}` (including empty lines) with 4 spaces, e.g. via `sed 's/^/    /'`.
- Keep the existing token/PEM redaction and line truncation.

This removes the possibility of fence-termination by embedded backticks and preserves readability in GitHub-flavored Markdown.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

Re-review of 3552d6b → 975dd1b. Prior review provenance: app-verified.

Prior low finding (bundler sed pipeline ordering for quoted paths with inline comments) addressed — comment-stripping sed expression now runs first, confirmed by comment.src.sh test fixture.

Findings

High

• [protected-path] — This PR modifies 25 files under protected paths (scripts/, .github/). No linked issue in this repository authorizes these changes (the PR references fullsend-ai/fullsend#2921 as the source port, which provides traceability but not local authorization). Human approval is always required for protected-path changes.

Low

• [logic-error] scripts/lib/post-failure-report.lib.sh:41 — The include guard [[ -n "${POST_FAILURE_REPORT_SH_LOADED:-}" ]] && return 0 uses return, which is only valid inside a function or sourced script. In bundled output (e.g., post-code.sh), this code is inlined at the top level. The bundler's deduplication ensures the guard body appears only once and the variable is unset on first run, so the guard never fires in normal operation. However, if the environment variable were pre-set (e.g., inherited from a parent process), return 0 would produce a bash error at the top level. Same pattern exists in github-api-csma.lib.sh.

• [edge-case] scripts/lib/post-failure-report.lib.sh:99 — gha_echo sanitizes message bodies through _sanitize_workflow_value, which strips all :: sequences. Diagnostic messages containing legitimate :: patterns (e.g., std::string in compiler output) will have those stripped from GHA log annotations. The comment-body sanitizer sanitize_comment_workflow_commands correctly handles this by only stripping :: at line start, but the log sanitizer is more aggressive. Cosmetic only — log annotations are for human consumption.

• [injection] scripts/lib/post-failure-report.lib.sh:68 — sanitize_comment_workflow_commands() strips warning|error|notice|debug|group|endgroup but not set-output|set-env|add-mask|stop-commands. This is defense-in-depth only — comment bodies go through the GitHub API and are never interpreted as GHA commands. The primary stdout path (_sanitize_workflow_value) correctly strips all :: sequences.

Previous run

Review

Re-review of 72e1a96 → 3552d6b (fix(scripts): address fullsend-ai-review low findings). Prior review provenance: app-verified.

All three prior low-severity findings addressed:

• Bundler source-line regex inline comments (was Low): ✅ Addressed — comment-stripping sed expression added. Works correctly for unquoted paths; see finding below for an ordering subtlety with quoted paths.
• bundle_canonical_path() silent fallback (was Low): ✅ Resolved — now emits a warning when a symlink cannot be canonicalized.
• _redact_literal_token() awk -v backslash escapes (was Low): ✅ Resolved — now uses ENVIRON for fully literal matching. Test sanitize-redacts-literal-push-token-backslash confirms.

Findings

Low

• [logic-error] scripts/bundle-sh.sh:142 — The inline-comment stripping sed expression (-e 's/[[:space:]]+#.*$//') is applied after quote-stripping in the pipeline, causing a trailing double-quote to remain for quoted paths with inline comments (e.g., source "${SCRIPT_DIR}/lib/foo.lib.sh" # explanation → lib/foo.lib.sh"). The bundler fails closed — the glob */scripts/lib/*.lib.sh rejects the quoted path with "source outside scripts/lib/" error. No existing source lines use inline comments, so this is purely latent. Fix: move the comment-stripping expression to be the first -e in the sed pipeline, before quote stripping.

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• .github/workflows/script-test.yml
• scripts/bundle-sh-test.sh
• scripts/bundle-sh.sh
• scripts/lib/github-api-csma.lib.sh
• scripts/lib/post-failure-report.lib.sh
• scripts/lib/test-fixtures/leaf.lib.sh
• scripts/lib/test-fixtures/nested.lib.sh
• scripts/lib/test-fixtures/parent.lib.sh
• scripts/post-code-test.sh
• scripts/post-code.sh
• scripts/post-code.src.sh
• scripts/post-failure-report-test.sh
• scripts/post-fix-test.sh
• scripts/post-fix.sh
• scripts/post-fix.src.sh
• scripts/post-prioritize-test.sh
• scripts/post-prioritize.sh
• scripts/post-prioritize.src.sh
• scripts/test-fixtures/bundle/cross-dedup.src.sh
• scripts/test-fixtures/bundle/dedup.src.sh
• scripts/test-fixtures/bundle/nested.src.sh
• scripts/test-fixtures/bundle/outside-lib.src.sh
• scripts/test-fixtures/bundle/simple.src.sh
• scripts/test-lib.sh

Previous run (2)

Review

Re-review of 679e1ca → 72e1a96 (10372ec review feedback, 9ade211 shellcheck, 72e1a96 cross-retry dedup). Prior review provenance: app-verified.

All prior findings either resolved or unchanged. Three new commits address round-2 reviewer feedback: awk-based literal token redaction (replacing glob-unsafe bash substitution), case-insensitive PEM detection, symlink resolution in the bundler, check-bundle drift test refactored to avoid modifying repo files, and new tests for each change.

Prior review findings — status

• Bundler source-line regex does not handle inline comments (was Low): ⬜ Unchanged — see finding 1 below.

New changes verified correct

• _redact_literal_token() design: Replaces ${detail//${PUSH_TOKEN}/[REDACTED]} (bash glob substitution — *, ?, [ in tokens would break matching) with awk index() (literal string search). Test sanitize-redacts-literal-push-token-glob-chars confirms tok*en?value is correctly redacted.
• Case-insensitive PEM detection: _redact_multiline_pem() now uses tolower() + regex instead of [A-Z ]* character class. Catches lowercase PEM headers. Test sanitize-redacts-lowercase-pem-block confirms.
• Symlink protection in bundler: bundle_canonical_path() resolves symlinks via readlink -f or realpath. After resolution, bundle_is_lib() validates the canonical path matches */scripts/lib/*.lib.sh. A symlink from scripts/lib/evil.lib.sh → ../../outside/evil.lib.sh resolves outside the allowed directory and is rejected. Test symlink-lib-should-fail confirms.
• Check-bundle drift test: Refactored from modifying (and restoring) the repo's post-code.sh to copying the repo into a temp directory. No longer risks leaving the repo dirty on test failure.
• Dedup guard test: post-failure-dedups-within-single-invocation verifies that calling report_post_failure_to_issue twice only posts once. Guards against trap + explicit call posting duplicate comments.
• POST_FAILURE_REPORTED comment: Documents that the dedup guard is intentionally per-invocation, not cross-retry.
• Bundled output consistency: post-code.sh and post-fix.sh reflect all library changes identically.

Findings

Low

• [edge-case] scripts/bundle-sh.sh — Severity anchored from prior review (679e1ca). Bundler source-line regex ^[[:space:]]*source[[:space:]]+(.+)[[:space:]]*$ captures everything after source, including inline comments (e.g., source "lib/foo.lib.sh" # explanation). Currently no .src.sh or .lib.sh files use inline comments on source lines. Consider documenting the restriction or tightening the regex.

• [edge-case] scripts/bundle-sh.sh — bundle_canonical_path() silently falls back to the unresolved path when neither readlink -f nor realpath is available (e.g., older macOS without coreutils). The symlink protection would not detect symlinks pointing outside scripts/lib/ on such systems. CI (ubuntu-24.04) has readlink -f and is not affected. Consider emitting a warning when the fallback is reached and the path is a symlink.

• [secrets-handling] scripts/lib/post-failure-report.lib.sh — _redact_literal_token() passes the token to awk via -v, which interprets C-style backslash escape sequences (\n → newline, \t → tab). A token containing a literal backslash followed by an awk escape character would not match its occurrences in the detail string. GitHub tokens (ghp_*, ghs_*, github_pat_*) use base-62 + underscore and never contain backslashes, so the risk is theoretical for current callers. Consider using awk ENVIRON instead of -v for fully literal matching.

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• .github/workflows/script-test.yml
• scripts/bundle-sh-test.sh
• scripts/bundle-sh.sh
• scripts/lib/github-api-csma.lib.sh
• scripts/lib/post-failure-report.lib.sh
• scripts/lib/test-fixtures/leaf.lib.sh
• scripts/lib/test-fixtures/nested.lib.sh
• scripts/lib/test-fixtures/parent.lib.sh
• scripts/post-code-test.sh
• scripts/post-code.sh
• scripts/post-code.src.sh
• scripts/post-failure-report-test.sh
• scripts/post-fix-test.sh
• scripts/post-fix.sh
• scripts/post-fix.src.sh
• scripts/post-prioritize-test.sh
• scripts/post-prioritize.sh
• scripts/post-prioritize.src.sh
• scripts/test-fixtures/bundle/cross-dedup.src.sh
• scripts/test-fixtures/bundle/dedup.src.sh
• scripts/test-fixtures/bundle/nested.src.sh
• scripts/test-fixtures/bundle/outside-lib.src.sh
• scripts/test-fixtures/bundle/simple.src.sh
• scripts/test-lib.sh

Previous run (3)

Review

Verdict: comment — all prior findings resolved or unchanged. One low-severity follow-up remains. Protected paths require human approval.

Re-review of 04f52c7 → 679e1ca (1e45ee8 review feedback, 267a3d9 lib-deps + CI hooks, 5a4d522 merge upstream, 1790d27 rebuild post-merge, 679e1ca gitlint cleanup). Prior review provenance: app-verified.

This PR extracts failure reporting into a shared library (scripts/lib/post-failure-report.lib.sh) with categorized failure types, output sanitization (token/PEM redaction, GHA workflow-command stripping, line truncation), and workflow run links. It replaces inline echo "::error::..." && exit 1 patterns in both post-code.sh and post-fix.sh with structured post_fail_to_issue/post_fail_to_pr calls that post detailed failure comments to the issue/PR. A script bundling system (bundle-sh.sh, Makefile targets) inlines scripts/lib/*.lib.sh sources into self-contained .sh artifacts since the harness fetches scripts as isolated blobs. A CSMA/CD rate-limiting library (github-api-csma.lib.sh) is extracted from the previously inlined copy in post-prioritize.sh.

Prior review findings — resolved

Both prior findings have been addressed:

• categorize_push_failure default fallthrough (was Low): ✅ Resolved. Default changed from push-rejected to push-failed. New push-failed case added to post_failure_category_label() with label "Push failed". Test added for the new category (categorize-unexpected-push-failed).
• Bundler source-line regex does not handle inline comments (was Low): ⬜ Unchanged — see finding 1 below.

New changes verified correct

• sanitize_comment_workflow_commands() design: New function correctly targets comment body context — strips only line-start GHA commands (^::(warning|error|notice|debug|group|endgroup)::) while preserving mid-string :: (e.g. std::string in compiler output). This is appropriate because gh issue comment/gh pr comment body text is NOT interpreted by the GHA runner for workflow commands. The older _sanitize_workflow_value() (which strips ALL ::) continues to be used for runner log output via gha_echo and print_sanitized_gha_log. Test sanitize-preserves-scoped-names verifies std::string survives.
• Literal PUSH_TOKEN/GH_TOKEN redaction: sanitize_failure_detail() now does ${detail//${PUSH_TOKEN}/[REDACTED]} after regex redaction. Empty-token guard ([ -n "${PUSH_TOKEN:-}" ]) prevents false matches. Dedup guard ([ "${GH_TOKEN}" != "${PUSH_TOKEN:-}" ]) avoids redundant work. Test sanitize-redacts-literal-push-token confirms.
• Sanitization pipeline ordering: Regex token redaction (sed + awk) → literal PUSH_TOKEN/GH_TOKEN substitution → comment workflow command stripping → line truncation. Correct — tokens are removed before any content transformation.
• push-failed consumer completeness: categorize_push_failure() returns 3 values: push-workflow-permission, push-rejected, push-failed. All 3 handled by post_failure_category_label(). post_failure_environmental_note() special-cases only push-workflow-permission with wildcard default — push-failed falls through correctly to empty note.
• gha_echo usage in report functions: report_post_failure_to_issue/report_post_failure_to_pr now use gha_echo warning "..." instead of raw echo "::warning::...". The gha_echo function sanitizes via sanitize_gha_log_output. Error messages now include actionable hint ("check issues:write on PUSH_TOKEN").
• Bundle test fixture relocation: Fixtures moved from scripts/test-fixtures/bundle/lib/ to scripts/lib/test-fixtures/ so they live under the scripts/lib/ path tree. bundle_is_lib() glob */scripts/lib/*.lib.sh matches scripts/lib/test-fixtures/leaf.lib.sh because * in bash [[ ]] matches /. bundle_is_lib() simplified to single condition. parent.lib.sh updated to use relative source "nested.lib.sh" for intra-lib references.
• Makefile check-bundle rewrite: Now uses temp dir + diff -u instead of git diff --exit-code. More robust — doesn't depend on git working directory state. Test check-bundle-detects-equal-mtime-drift verifies drift detection.
• Makefile LIB_DEPS wildcard: $(wildcard scripts/lib/*.lib.sh) added as dependency for scripts/%.sh pattern rule. Lib changes now trigger bundle rebuilds. Note: Make's wildcard doesn't match subdirectories, so scripts/lib/test-fixtures/ fixtures correctly don't trigger production rebuilds.
• GITHUB_WORKSPACE guard: Fallback candidate loops now wrapped in if [ -n "${GITHUB_WORKSPACE:-}" ] to prevent expanding empty GITHUB_WORKSPACE into root filesystem paths ("/scripts"). Applied consistently across post-code.src.sh, post-fix.src.sh, pre-code.sh, pre-fix.sh.
• New test coverage: PEM redaction, bearer token redaction, PUSH_TOKEN literal redaction, report_post_failure_to_issue integration with mock gh, outside-lib bundle failure, check-bundle drift detection, cross-dedup assertion. Comprehensive.
• PUSH_TOKEN documentation: Script headers updated to list contents:write + issues:write + pull-requests:write. Accurate.
• Cross-repo backward compatibility: Exit codes unchanged (0/1). All existing required env vars preserved. New push-failed category is additive — no existing consumer expects push-rejected for network errors. Script filenames unchanged. Fully backward compatible.
• Commit message conventions: fix(scripts) correct for review feedback. chore correct for gitlint cleanup. Merge commits don't require conventional format. PR title feat(harness) correct for user-visible failure comments.

Findings

1. Bundler source-line regex does not handle inline comments — Low

File: scripts/bundle-sh.sh

Severity anchored from prior review (04f52c7).

The regex ^[[:space:]]*source[[:space:]]+(.+)[[:space:]]*$ captures everything after source, including inline comments (e.g., source "lib/foo.lib.sh" # explanation). The captured group would include # explanation, which flows through bundle_expand_source's sed pipeline and would fail with "missing library" or "unsupported source expression." Currently no .src.sh or .lib.sh files in the repo use inline comments on source lines, so this is not exploitable today — a future contributor adding source "..." # why would get a confusing bundler error. Consider documenting the restriction or tightening the regex.

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• .github/workflows/script-test.yml
• scripts/bundle-sh-test.sh
• scripts/bundle-sh.sh
• scripts/lib/github-api-csma.lib.sh
• scripts/lib/post-failure-report.lib.sh
• scripts/lib/test-fixtures/leaf.lib.sh
• scripts/lib/test-fixtures/nested.lib.sh
• scripts/lib/test-fixtures/parent.lib.sh
• scripts/post-code-test.sh
• scripts/post-code.sh
• scripts/post-code.src.sh
• scripts/post-failure-report-test.sh
• scripts/post-fix-test.sh
• scripts/post-fix.sh
• scripts/post-fix.src.sh
• scripts/post-prioritize-test.sh
• scripts/post-prioritize.sh
• scripts/post-prioritize.src.sh
• scripts/test-fixtures/bundle/cross-dedup.src.sh
• scripts/test-fixtures/bundle/dedup.src.sh
• scripts/test-fixtures/bundle/nested.src.sh
• scripts/test-fixtures/bundle/outside-lib.src.sh
• scripts/test-fixtures/bundle/simple.src.sh
• scripts/test-lib.sh

Previous run (4)

Review

Verdict: approve — all prior medium-severity findings resolved. Two low-severity follow-ups remain. No blocking issues.

Re-review of f9542d5 → 04f52c7 (603b401 GHA sanitization + cross-dedup test, 04f52c7 end-of-file fix, plus rebase correcting commit type). Prior review provenance: app-verified.

This PR extracts failure reporting into a shared library (scripts/lib/post-failure-report.lib.sh) with categorized failure types, output sanitization (token/PEM redaction, GHA workflow-command stripping, line truncation), and workflow run links. It replaces inline echo "::error::..." && exit 1 patterns in both post-code.sh and post-fix.sh with structured post_fail_to_issue/post_fail_to_pr calls that post detailed failure comments to the issue/PR. A script bundling system (bundle-sh.sh, Makefile targets) inlines scripts/lib/*.lib.sh sources into self-contained .sh artifacts since the harness fetches scripts as isolated blobs. A CSMA/CD rate-limiting library (github-api-csma.lib.sh) is extracted from the previously inlined copy in post-prioritize.sh.

Prior review findings — resolved

All four prior findings have been addressed:

• Commit message type violation on bundling commit (was Medium): ✅ Resolved.
• Remaining unsanitized variables in GHA workflow commands (was Low): ✅ Resolved.
• categorize_push_failure default fallthrough (was Low): ⬜ Unchanged.
• Bundler dedup test gap for nested cross-library case (was Low): ✅ Resolved.

Findings

1. categorize_push_failure default fallthrough — Low

File: scripts/lib/post-failure-report.lib.sh

Severity anchored from prior review (c1a58d8).

The function returns push-rejected for ALL unmatched push errors. Consider a distinct fallthrough like push-failed.

2. Bundler source-line regex does not handle inline comments — Low

File: scripts/bundle-sh.sh

The regex captures inline comments. Not exploitable today.

Previous run (5)

Review

Verdict: approve — all prior medium-severity findings resolved. Two low-severity follow-ups remain. No blocking issues.

Re-review of f9542d5 → 04f52c7 (603b401 GHA sanitization + cross-dedup test, 04f52c7 end-of-file fix, plus rebase correcting commit type). Prior review provenance: app-verified.

This PR extracts failure reporting into a shared library (scripts/lib/post-failure-report.lib.sh) with categorized failure types, output sanitization (token/PEM redaction, GHA workflow-command stripping, line truncation), and workflow run links. It replaces inline echo "::error::..." && exit 1 patterns in both post-code.sh and post-fix.sh with structured post_fail_to_issue/post_fail_to_pr calls that post detailed failure comments to the issue/PR. A script bundling system (bundle-sh.sh, Makefile targets) inlines scripts/lib/*.lib.sh sources into self-contained .sh artifacts since the harness fetches scripts as isolated blobs. A CSMA/CD rate-limiting library (github-api-csma.lib.sh) is extracted from the previously inlined copy in post-prioritize.sh.

Prior review findings — resolved

All four prior findings have been addressed:

• Commit message type violation on bundling commit (was Medium): ✅ Resolved. Commit e55867c now uses build(scripts): add script bundling build stage for lib-backed post-scripts instead of the prior feat(scripts). Correct per COMMITS.md: bundling infrastructure is internal build tooling.
• Remaining unsanitized variables in GHA workflow commands (was Low): ✅ Resolved. All echo "::warning::..." / echo "::error::..." / echo "::notice::..." replaced with gha_echo which sanitizes the message body via sanitize_gha_log_output. All captured command output (PUSH_OUTPUT, PRECOMMIT_OUTPUT, FORCE_PUSH_OUTPUT, GITLEAKS_OUTPUT) now echoed via print_sanitized_gha_log. The remaining raw echo "::warning::..." in report_post_failure_to_issue/pr use safe_issue_number/safe_pr_number (sanitized via _sanitize_workflow_value). Comprehensive.
• categorize_push_failure default fallthrough (was Low): ⬜ Unchanged — see finding 1 below.
• Bundler dedup test gap for nested cross-library case (was Low): ✅ Resolved. cross-dedup.src.sh fixture added: sources parent.lib.sh (which itself sources nested.lib.sh) AND directly sources nested.lib.sh. Test asserts nested_fn() appears exactly once in bundled output.

Verified correct

• ERR trap re-entry guard: POST_FAILURE_REPORTED=true prevents double-posting when exit 1 inside post_fail_to_issue re-triggers the trap. Verified correct.
• Dispatch table completeness: All 9 failure categories used in callers (secret-scan, pre-commit-blocked, signed-off-by, push-workflow-permission, push-rejected, pr-creation-failed, branch-validation, setup-error, process-output-failed) are handled by post_failure_category_label(). The ERR trap fallback uses the wildcard case. Complete.
• Token redaction coverage: gh[pousr]_ regex covers all single-letter GitHub token prefixes (ghp_, gho_, ghu_, ghs_, ghr_). github_pat_, x-access-token:..., and Bearer/token prefixes are also covered.
• PEM redaction safety: _redact_multiline_pem fails closed — missing END marker suppresses all subsequent lines rather than leaking.
• Secret scan detail suppression: All secret-scan paths use POST_FAILURE_SECRET_SCAN_MESSAGE (static string). build_post_failure_comment double-checks: when category == "secret-scan", detail is replaced regardless of what was passed.
• Sanitization pipeline ordering: Token redaction (sed + awk) → GHA command stripping (sanitize_gha_log_output) → line truncation (tail). Correct — sanitizing before truncation ensures all content is cleaned.
• gha_echo safety: Level parameter is always hardcoded (error, warning, notice) — never user-controlled. Message body goes through sanitize_gha_log_output. No injection risk.
• Bundled output guard safety: [[ -n "${..._LOADED:-}" ]] && return 0 guard lines in bundled output are never reached for duplicate inclusions because the bundler replaces second inclusions with a # (already bundled: ...) comment.
• CSMA RETURN trap: trap "rm -f ..." RETURN in github_csma_run/_run_pipe/_run_cmd correctly cleans up temp files on function return.
• Removal staleness: report_failure_to_issue (old function) and SAFE_PUSH (old variable) exist only in the base branch's post-code.sh and are fully replaced by the PR. No stale references remain in any .src.sh, .lib.sh, or test file.
• Cross-repo backward compatibility: Exit codes unchanged (0/1). All existing required env vars preserved. New optional POST_FAILURE_DETAIL_MAX_LINES has a default value. Script filenames unchanged. Fully backward compatible.
• Commit message conventions: feat(harness) correct for user-visible failure comments. build(scripts) correct for bundling. fix(scripts) correct for review feedback. chore(scripts) correct for whitespace. PR title follows conventional commit format.

Findings

1. categorize_push_failure default fallthrough — Low

File: scripts/lib/post-failure-report.lib.sh

Severity anchored from prior review (c1a58d8).

The function returns push-rejected for ALL unmatched push errors, including network timeouts (fatal: unable to access), DNS failures, and TLS errors. The label "Push rejected" implies the remote actively refused the push, which isn't the case for connectivity issues. Functionally harmless since the failure comment still links to workflow logs. Consider a distinct fallthrough like push-failed with label "Push failed" for unrecognized errors.

2. Bundler source-line regex does not handle inline comments — Low

File: scripts/bundle-sh.sh

The regex ^[[:space:]]*source[[:space:]]+(.+)[[:space:]]*$ captures everything after source, including inline comments (e.g., source "lib/foo.lib.sh" # explanation). The captured group would include # explanation, which flows through bundle_expand_source's sed pipeline and would fail with "missing library" or "unsupported source expression." Currently no .src.sh or .lib.sh files in the repo use inline comments on source lines, so this is not exploitable today — a future contributor adding source "..." # why would get a confusing bundler error. Consider documenting the restriction or tightening the regex.

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• .github/workflows/script-test.yml
• scripts/bundle-sh-test.sh
• scripts/bundle-sh.sh
• scripts/lib/github-api-csma.lib.sh
• scripts/lib/post-failure-report.lib.sh
• scripts/post-code-test.sh
• scripts/post-code.sh
• scripts/post-code.src.sh
• scripts/post-failure-report-test.sh
• scripts/post-fix.sh
• scripts/post-fix.src.sh
• scripts/post-prioritize-test.sh
• scripts/post-prioritize.sh
• scripts/post-prioritize.src.sh
• scripts/test-fixtures/bundle/cross-dedup.src.sh
• scripts/test-fixtures/bundle/dedup.src.sh
• scripts/test-fixtures/bundle/lib/leaf.lib.sh
• scripts/test-fixtures/bundle/lib/nested.lib.sh
• scripts/test-fixtures/bundle/lib/parent.lib.sh
• scripts/test-fixtures/bundle/nested.src.sh
• scripts/test-fixtures/bundle/simple.src.sh
• scripts/test-lib.sh

Previous run (6)

Review

Verdict: comment — one medium-severity commit-convention finding and three low-severity follow-ups. No blocking issues. Significant improvement since prior review.

Re-review of 7529160 + 2 new commits (38a95d7 review feedback, f9542d5 shellcheck fixes). Prior review provenance: app-verified.

This PR extracts failure reporting into a shared library (scripts/lib/post-failure-report.lib.sh) with categorized failure types, output sanitization (token/PEM redaction, GHA workflow-command stripping), and workflow run links. It replaces inline echo "::error::..." && exit 1 patterns in both post-code.sh and post-fix.sh with structured post_fail_to_issue/post_fail_to_pr calls that post detailed failure comments to the issue/PR. A script bundling system (bundle-sh.sh, Makefile targets) inlines scripts/lib/*.lib.sh sources into self-contained .sh artifacts since the harness fetches scripts as isolated blobs. A CSMA/CD rate-limiting library (github-api-csma.lib.sh) is extracted from the previously inlined copy in post-prioritize.sh.

Prior review findings — resolved

The two new commits address the primary findings from the prior review:

• GHA workflow command injection in runner logs (was Medium): ✅ Resolved. New sanitize_gha_log_output() and print_sanitized_gha_log() functions added. All echo "${OUTPUT}" statements replaced with print_sanitized_gha_log "${OUTPUT}". sanitize_failure_detail() now calls sanitize_gha_log_output before truncation, so comment bodies are also protected. Pipeline ordering is correct: token redaction → GHA command stripping → line truncation.
• Missing org-mode URL routing negative test (was Low): ✅ Resolved. failure-comment-org-mode-not-source-repo and failure-comment-non-org-mode-fallback tests restored.
• POST_FAILURE_DETAIL_MAX_LINES undocumented (was Low): ✅ Resolved. Now documented in both post-code.src.sh and post-fix.src.sh script headers.
• Bundler while read correctness (not previously flagged): ✅ Improved. mapfile -t replaced while IFS= read -r — correctly handles files without trailing newlines.

Verified correct

• ERR trap re-entry guard: POST_FAILURE_REPORTED=true prevents double-posting when exit 1 inside post_fail_to_issue re-triggers the trap. Verified correct.
• Dispatch table completeness: All 9 failure categories used in callers (secret-scan, pre-commit-blocked, signed-off-by, push-workflow-permission, push-rejected, pr-creation-failed, branch-validation, setup-error, process-output-failed) are handled by post_failure_category_label(). The ERR trap fallback uses the wildcard case. Complete.
• Token redaction coverage: Verified that gh[pousr]_ regex covers all single-letter GitHub token prefixes (ghp_, gho_, ghu_, ghs_, ghr_). github_pat_, x-access-token:..., and Bearer/token prefixes are also covered.
• PEM redaction safety: _redact_multiline_pem fails closed — missing END marker suppresses all subsequent lines rather than leaking.
• Secret scan detail suppression: All secret-scan paths use POST_FAILURE_SECRET_SCAN_MESSAGE (static string). build_post_failure_comment double-checks: when category == "secret-scan", detail is replaced regardless of what was passed.
• Sanitization pipeline ordering: Token redaction (sed + awk) → GHA command stripping (sanitize_gha_log_output) → line truncation (tail). Correct — sanitizing before truncation ensures all content is cleaned.
• Bundled output guard safety: [[ -n "${..._LOADED:-}" ]] && return 0 guard lines in bundled output are never reached for duplicate inclusions because the bundler replaces second inclusions with a # (already bundled: ...) comment.
• CSMA RETURN trap: trap "rm -f ..." RETURN in github_csma_run/_run_pipe/_run_cmd correctly cleans up temp files on function return.

Findings

1. Commit message type violation on bundling commit — Medium

File: N/A (commit 7529160)

Severity anchored from prior review (7529160).

Commit 7529160 uses feat(scripts): add script bundling build stage for lib-backed post-scripts. Per COMMITS.md: "Adding internal helpers or abstractions that don't change user-visible behavior → refactor" and the build type is for "Build system or dependency changes." The bundling infrastructure is internal build tooling — users don't interact with the bundler or .src.sh files. The correct type would be build(scripts) or chore(scripts).

The first commit (feat(harness): report categorized post-script failures on issue/PR) correctly uses feat since failure comments on issues/PRs are user-visible. The fix commits (38a95d7, f9542d5) correctly use fix(scripts).

2. Remaining unsanitized variables in GHA workflow commands — Low

Files: scripts/post-code.src.sh, scripts/post-fix.src.sh

Several pre-existing ::warning:: / ::notice:: / ::error:: commands interpolate variables not passed through _sanitize_workflow_value(): BRANCH (from git branch --show-current), file (from git diff --name-only), TARGET_BRANCH (from API response), and PR_NUMBER_FROM_URL (from PR create output). These are not directly user-controlled, and git branch name restrictions limit the practical attack surface. The PR significantly improves the overall sanitization posture by establishing the print_sanitized_gha_log pattern for all captured command output — this finding is about extending that same pattern to the remaining interpolated variables for defense-in-depth consistency.

3. categorize_push_failure default fallthrough — Low

File: scripts/lib/post-failure-report.lib.sh

Severity anchored from prior review (c1a58d8).

The function returns push-rejected for ALL unmatched push errors, including network timeouts (fatal: unable to access), DNS failures, and TLS errors. The label "Push rejected" implies the remote actively refused the push, which isn't the case for connectivity issues. Functionally harmless since the failure comment still links to workflow logs. Consider a distinct fallthrough like push-failed with label "Push failed" for unrecognized errors.

4. Bundler dedup test gap for nested cross-library case — Low

File: scripts/bundle-sh-test.sh

Severity anchored from prior review (7529160).

The dedup.src.sh fixture tests that sourcing the same library twice produces only one copy of leaf_fn(). The nested.src.sh fixture tests that nested library sourcing executes correctly. However, there is no test for the cross-reference dedup case: a .src.sh that sources parent.lib.sh (which itself sources nested.lib.sh) AND directly sources nested.lib.sh. Consider adding an assertion that nested_fn() appears exactly once in the nested bundle output, similar to the dedup-leaf-fn-count assertion.

Previous run (7)

Review

Verdict: comment — one medium-severity commit-convention finding and three low-severity follow-ups. No blocking issues. Significant improvement since prior review.

Re-review of 7529160 + 2 new commits (38a95d7 review feedback, f9542d5 shellcheck fixes). Prior review provenance: app-verified.

This PR extracts failure reporting into a shared library (scripts/lib/post-failure-report.lib.sh) with categorized failure types, output sanitization (token/PEM redaction, GHA command stripping, line truncation), and workflow run links. It replaces inline echo "::error::..." && exit 1 patterns in both post-code.sh and post-fix.sh with structured post_fail_to_issue/post_fail_to_pr calls that post detailed failure comments to the issue/PR. A script bundling system (bundle-sh.sh, Makefile targets) inlines scripts/lib/*.lib.sh sources into self-contained .sh artifacts since the harness fetches scripts as isolated blobs. A CSMA/CD rate-limiting library (github-api-csma.lib.sh) is extracted from the previously inlined copy in post-prioritize.sh.

Prior review findings — resolved

The two new commits address the primary findings from the prior review:

• GHA workflow command injection in runner logs (was Medium): ✅ Resolved. New sanitize_gha_log_output() and print_sanitized_gha_log() functions added. All echo "${OUTPUT}" statements replaced with print_sanitized_gha_log "${OUTPUT}". sanitize_failure_detail() now calls sanitize_gha_log_output before truncation, so comment bodies are also protected. Pipeline ordering is correct: token redaction → GHA command stripping → line truncation.
• Missing org-mode URL routing negative test (was Low): ✅ Resolved. failure-comment-org-mode-not-source-repo and failure-comment-non-org-mode-fallback tests restored.
• POST_FAILURE_DETAIL_MAX_LINES undocumented (was Low): ✅ Resolved. Now documented in both post-code.src.sh and post-fix.src.sh script headers.
• Bundler while read correctness (not previously flagged): ✅ Improved. mapfile -t replaced while IFS= read -r — correctly handles files without trailing newlines.

Verified correct

• ERR trap re-entry guard: POST_FAILURE_REPORTED=true prevents double-posting when exit 1 inside post_fail_to_issue re-triggers the trap. Verified correct.
• Dispatch table completeness: All 9 failure categories used in callers (secret-scan, pre-commit-blocked, signed-off-by, push-workflow-permission, push-rejected, pr-creation-failed, branch-validation, setup-error, process-output-failed) are handled by post_failure_category_label(). The ERR trap fallback uses the wildcard case. Complete.
• Token redaction coverage: Verified that gh[pousr]_ regex covers all single-letter GitHub token prefixes (ghp_, gho_, ghu_, ghs_, ghr_). github_pat_, x-access-token:..., and Bearer/token prefixes are also covered.
• PEM redaction safety: _redact_multiline_pem fails closed — missing END marker suppresses all subsequent lines rather than leaking.
• Secret scan detail suppression: All secret-scan paths use POST_FAILURE_SECRET_SCAN_MESSAGE (static string). build_post_failure_comment double-checks: when category == "secret-scan", detail is replaced regardless of what was passed.
• Sanitization pipeline ordering: Token redaction (sed + awk) → GHA command stripping (sanitize_gha_log_output) → line truncation (tail). Correct — sanitizing before truncation ensures all content is cleaned.
• Bundled output guard safety: [[ -n "${..._LOADED:-}" ]] && return 0 guard lines in bundled output are never reached for duplicate inclusions because the bundler replaces second inclusions with a # (already bundled: ...) comment.
• CSMA RETURN trap: trap "rm -f ..." RETURN in github_csma_run/_run_pipe/_run_cmd correctly cleans up temp files on function return.

Findings

1. Commit message type violation on bundling commit — Medium

File: N/A (commit 7529160)

Severity anchored from prior review (7529160).

Commit 7529160 uses feat(scripts): add script bundling build stage for lib-backed post-scripts. Per COMMITS.md: "Adding internal helpers or abstractions that don't change user-visible behavior → refactor" and the build type is for "Build system or dependency changes." The bundling infrastructure is internal build tooling — users don't interact with the bundler or .src.sh files. The correct type would be build(scripts) or chore(scripts).

The first commit (feat(harness): report categorized post-script failures on issue/PR) correctly uses feat since failure comments on issues/PRs are user-visible. The fix commits (38a95d7, f9542d5) correctly use fix(scripts).

2. Remaining unsanitized variables in GHA workflow commands — Low

Files: scripts/post-code.src.sh, scripts/post-fix.src.sh

Several pre-existing ::warning:: / ::notice:: / ::error:: commands interpolate variables not passed through _sanitize_workflow_value(): BRANCH (from git branch --show-current), file (from git diff --name-only), TARGET_BRANCH (from API response), and PR_NUMBER_FROM_URL (from PR create output). These are not directly user-controlled, and git branch name restrictions limit the practical attack surface. The PR significantly improves the overall sanitization posture by establishing the print_sanitized_gha_log pattern for all captured command output — this finding is about extending that same pattern to the remaining interpolated variables for defense-in-depth consistency.

3. categorize_push_failure default fallthrough — Low

File: scripts/lib/post-failure-report.lib.sh

Severity anchored from prior review (c1a58d8).

The function returns push-rejected for ALL unmatched push errors, including network timeouts (fatal: unable to access), DNS failures, and TLS errors. The label "Push rejected" implies the remote actively refused the push, which isn't the case for connectivity issues. Functionally harmless since the failure comment still links to workflow logs. Consider a distinct fallthrough like push-failed with label "Push failed" for unrecognized errors.

4. Bundler dedup test gap for nested cross-library case — Low

File: scripts/bundle-sh-test.sh

Severity anchored from prior review (7529160).

The dedup.src.sh fixture tests that sourcing the same library twice produces only one copy of leaf_fn(). The nested.src.sh fixture tests that nested library sourcing executes correctly. However, there is no test for the cross-reference dedup case: a .src.sh that sources parent.lib.sh (which itself sources nested.lib.sh) AND directly sources nested.lib.sh. Consider adding an assertion that nested_fn() appears exactly once in the nested bundle output, similar to the dedup-leaf-fn-count assertion.

Previous run (8)

Review

Verdict: comment — two medium-severity observations and several low-severity follow-ups. No blocking issues.

Re-review of c1a58d8 + 2 new commits (dc355c7 merge upstream/main, 7529160 bundling infrastructure). Prior review provenance: app-verified.

This PR extracts failure reporting into a shared library (scripts/lib/post-failure-report.lib.sh) with categorized failure types, output sanitization (token/PEM redaction, line truncation), and workflow run links. It replaces inline echo "::error::..." && exit 1 patterns in both post-code.sh and post-fix.sh with structured post_fail_to_issue/post_fail_to_pr calls that post detailed failure comments to the issue/PR. A script bundling system (bundle-sh.sh, Makefile targets) inlines scripts/lib/*.lib.sh sources into self-contained .sh artifacts since the harness fetches scripts as isolated blobs. The design is sound — the shared library follows the existing guard-load pattern, the ERR trap + re-entry guard is correct, and the architecture properly separates GHA command sanitization (_sanitize_workflow_value) from comment body sanitization (sanitize_failure_detail).

Verified correct

• ERR trap re-entry guard: POST_FAILURE_REPORTED=true prevents double-posting when exit 1 inside post_fail_to_issue re-triggers the trap. Verified correct.
• Dispatch table completeness: All 9 failure categories used in post_fail_to_issue/post_fail_to_pr calls are handled by post_failure_category_label(). The ERR trap fallback uses the wildcard case. Complete.
• Variable capture: PRECOMMIT_OUTPUT="$(pre-commit run ... 2>&1)" correctly captures stdout+stderr regardless of exit code. The captured output flows through sanitize_failure_detail before posting.
• Cross-repo compatibility: Required env vars, exit codes, and external interface contracts are preserved. The bundled .sh files are functionally identical to their predecessors.
• Secret scan detail suppression: All secret-scan failure paths consistently use POST_FAILURE_SECRET_SCAN_MESSAGE (static string) instead of actual gitleaks findings.

Findings

1. sanitize_failure_detail() does not strip GHA workflow command sequences — Medium

File: scripts/lib/post-failure-report.lib.sh

Severity anchored from prior review (c1a58d8).

The function redacts tokens and PEM keys but does not strip :: sequences or %0A/%0D URL-encoded newlines. These sequences are relevant for GHA workflow command injection.

Updated assessment: The function's output flows to gh issue comment / gh pr comment body text, which is NOT subject to GHA command injection. The separate _sanitize_workflow_value() function correctly strips :: and %0A/%0D for the values interpolated into ::warning:: commands (verified: ISSUE_NUMBER → safe_issue_number, PR_NUMBER → safe_pr_number). The remaining GHA injection surface is the echo "${PUSH_OUTPUT}" / echo "${PRECOMMIT_OUTPUT}" statements that echo raw command output to the runner log — but this is pre-existing behavior (the old code also echoed PUSH_OUTPUT unconditionally). The old SAFE_PUSH sanitization only protected the ::error:: interpolation in one branch, which this PR eliminates entirely.

Recommendation: For defense-in-depth, consider adding a sanitize_for_gha_log() function that strips :: and %0A/%0D from values before echoing to the runner log, applied to all echo "${PUSH_OUTPUT}", echo "${PRECOMMIT_OUTPUT}", and echo "${FORCE_PUSH_OUTPUT}" statements.

2. Commit message type violation on bundling commit — Medium

File: N/A (commit 7529160)

Commit 7529160 uses feat(scripts): add script bundling build stage for lib-backed post-scripts. Per COMMITS.md: "Adding internal helpers or abstractions that don't change user-visible behavior → refactor" and the build type is for "Build system or dependency changes." The bundling infrastructure is not user-facing — users don't interact with the bundler or .src.sh files. The correct type would be build(scripts) or chore(scripts).

The first commit (feat(harness): report categorized post-script failures on issue/PR) correctly uses feat since failure comments on issues/PRs are user-visible.

3. Missing negative test for org-mode URL routing — Low

File: scripts/post-failure-report-test.sh

Severity anchored from prior review (c1a58d8).

The old post-code-test.sh had error-comment-org-mode-not-source-repo verifying the workflow URL does NOT contain the source repo name when GITHUB_REPOSITORY differs. The new post-failure-report-test.sh has the positive test (failure-comment-org-mode-uses-dispatch-repo) but not the negative assertion. The positive test alone is weaker — a bug that includes both repos in the URL would pass the positive test.

Consider adding:

run_failure_comment_test "failure-comment-org-mode-not-source-repo" \
  "push-rejected" "push failed" "test-org/my-app" "12345" \
  "https://github.com/test-org/my-app/actions/runs/12345" "no" \
  "test-org/.fullsend"

4. categorize_push_failure default fallthrough — Low

File: scripts/lib/post-failure-report.lib.sh

The function returns push-rejected for ALL unmatched push errors, including network timeouts (fatal: unable to access), DNS failures, and TLS errors. This is semantically imprecise — the label "Push rejected" implies the remote actively refused the push, which isn't the case for connectivity issues. Functionally harmless since the failure comment still links to workflow logs.

Consider a distinct fallthrough like push-failed with label "Push failed" for unrecognized errors.

5. POST_FAILURE_DETAIL_MAX_LINES undocumented in script headers — Low

Files: scripts/post-code.src.sh, scripts/post-fix.src.sh

Severity anchored from prior review (c1a58d8).

The POST_FAILURE_DETAIL_MAX_LINES environment variable (defaulting to 30) is configurable but not listed in the "Optional environment variables" sections of either script header. The library header documents it, but users reading the script headers won't discover it.

6. Bundler dedup test gap for nested cross-library case — Low

File: scripts/bundle-sh-test.sh

The dedup.src.sh fixture tests that sourcing the same library twice produces only one copy of leaf_fn(). The nested.src.sh fixture tests that nested library sourcing executes correctly. However, there is no test for the cross-reference dedup case: a .src.sh that sources parent.lib.sh (which itself sources nested.lib.sh) AND directly sources nested.lib.sh. Consider adding an assertion that nested_fn() appears exactly once in the nested bundle output, similar to the dedup-leaf-fn-count assertion.

Note on PR body accuracy

The PR body states "Extend post-code-test.sh / post-fix-test.sh coverage for failure reporting." In practice, post-fix-test.sh is not modified in this PR. The failure reporting tests were moved to a new shared test file (post-failure-report-test.sh) which covers both code and fix paths. This is functionally adequate but the PR description is slightly misleading.

Previous run (9)

Review

Verdict: comment — two medium-severity observations and several low-severity follow-ups. No blocking issues.

Re-review of c1a58d8 + 2 new commits (dc355c7 merge upstream/main, 7529160 bundling infrastructure). Prior review provenance: app-verified.

This PR extracts failure reporting into a shared library (scripts/lib/post-failure-report.lib.sh) with categorized failure types, output sanitization (token/PEM redaction, line truncation), and workflow run links. It replaces inline echo "::error::..." && exit 1 patterns in both post-code.sh and post-fix.sh with structured post_fail_to_issue/post_fail_to_pr calls that post detailed failure comments to the issue/PR. A script bundling system (bundle-sh.sh, Makefile targets) inlines scripts/lib/*.lib.sh sources into self-contained .sh artifacts since the harness fetches scripts as isolated blobs. The design is sound — the shared library follows the existing guard-load pattern, the ERR trap + re-entry guard is correct, and the architecture properly separates GHA command sanitization (_sanitize_workflow_value) from comment body sanitization (sanitize_failure_detail).

Verified correct

• ERR trap re-entry guard: POST_FAILURE_REPORTED=true prevents double-posting when exit 1 inside post_fail_to_issue re-triggers the trap. Verified correct.
• Dispatch table completeness: All 9 failure categories used in post_fail_to_issue/post_fail_to_pr calls are handled by post_failure_category_label(). The ERR trap fallback uses the wildcard case. Complete.
• Variable capture: PRECOMMIT_OUTPUT="$(pre-commit run ... 2>&1)" correctly captures stdout+stderr regardless of exit code. The captured output flows through sanitize_failure_detail before posting.
• Cross-repo compatibility: Required env vars, exit codes, and external interface contracts are preserved. The bundled .sh files are functionally identical to their predecessors.
• Secret scan detail suppression: All secret-scan failure paths consistently use POST_FAILURE_SECRET_SCAN_MESSAGE (static string) instead of actual gitleaks findings.

Findings

1. sanitize_failure_detail() does not strip GHA workflow command sequences — Medium

File: scripts/lib/post-failure-report.lib.sh

Severity anchored from prior review (c1a58d8).

The function redacts tokens and PEM keys but does not strip :: sequences or %0A/%0D URL-encoded newlines. These sequences are relevant for GHA workflow command injection.

Updated assessment: The function's output flows to gh issue comment / gh pr comment body text, which is NOT subject to GHA command injection. The separate _sanitize_workflow_value() function correctly strips :: and %0A/%0D for the values interpolated into ::warning:: commands (verified: ISSUE_NUMBER → safe_issue_number, PR_NUMBER → safe_pr_number). The remaining GHA injection surface is the echo "${PUSH_OUTPUT}" / echo "${PRECOMMIT_OUTPUT}" statements that echo raw command output to the runner log — but this is pre-existing behavior (the old code also echoed PUSH_OUTPUT unconditionally). The old SAFE_PUSH sanitization only protected the ::error:: interpolation in one branch, which this PR eliminates entirely.

Recommendation: For defense-in-depth, consider adding a sanitize_for_gha_log() function that strips :: and %0A/%0D from values before echoing to the runner log, applied to all echo "${PUSH_OUTPUT}", echo "${PRECOMMIT_OUTPUT}", and echo "${FORCE_PUSH_OUTPUT}" statements.

2. Commit message type violation on bundling commit — Medium

File: N/A (commit 7529160)

Commit 7529160 uses feat(scripts): add script bundling build stage for lib-backed post-scripts. Per COMMITS.md: "Adding internal helpers or abstractions that don't change user-visible behavior → refactor" and the build type is for "Build system or dependency changes." The bundling infrastructure is not user-facing — users don't interact with the bundler or .src.sh files. The correct type would be build(scripts) or chore(scripts).

The first commit (feat(harness): report categorized post-script failures on issue/PR) correctly uses feat since failure comments on issues/PRs are user-visible.

3. Missing negative test for org-mode URL routing — Low

File: scripts/post-failure-report-test.sh

Severity anchored from prior review (c1a58d8).

The old post-code-test.sh had error-comment-org-mode-not-source-repo verifying the workflow URL does NOT contain the source repo name when GITHUB_REPOSITORY differs. The new post-failure-report-test.sh has the positive test (failure-comment-org-mode-uses-dispatch-repo) but not the negative assertion. The positive test alone is weaker — a bug that includes both repos in the URL would pass the positive test.

Consider adding:

run_failure_comment_test "failure-comment-org-mode-not-source-repo" \
  "push-rejected" "push failed" "test-org/my-app" "12345" \
  "https://github.com/test-org/my-app/actions/runs/12345" "no" \
  "test-org/.fullsend"

4. categorize_push_failure default fallthrough — Low

File: scripts/lib/post-failure-report.lib.sh

The function returns push-rejected for ALL unmatched push errors, including network timeouts (fatal: unable to access), DNS failures, and TLS errors. This is semantically imprecise — the label "Push rejected" implies the remote actively refused the push, which isn't the case for connectivity issues. Functionally harmless since the failure comment still links to workflow logs.

Consider a distinct fallthrough like push-failed with label "Push failed" for unrecognized errors.

5. POST_FAILURE_DETAIL_MAX_LINES undocumented in script headers — Low

Files: scripts/post-code.src.sh, scripts/post-fix.src.sh

Severity anchored from prior review (c1a58d8).

The POST_FAILURE_DETAIL_MAX_LINES environment variable (defaulting to 30) is configurable but not listed in the "Optional environment variables" sections of either script header. The library header documents it, but users reading the script headers won't discover it.

6. Bundler dedup test gap for nested cross-library case — Low

File: scripts/bundle-sh-test.sh

The dedup.src.sh fixture tests that sourcing the same library twice produces only one copy of leaf_fn(). The nested.src.sh fixture tests that nested library sourcing executes correctly. However, there is no test for the cross-reference dedup case: a .src.sh that sources parent.lib.sh (which itself sources nested.lib.sh) AND directly sources nested.lib.sh. Consider adding an assertion that nested_fn() appears exactly once in the nested bundle output, similar to the dedup-leaf-fn-count assertion.

Note on PR body accuracy

The PR body states "Extend post-code-test.sh / post-fix-test.sh coverage for failure reporting." In practice, post-fix-test.sh is not modified in this PR. The failure reporting tests were moved to a new shared test file (post-failure-report-test.sh) which covers both code and fix paths. This is functionally adequate but the PR description is slightly misleading.

Previous run (10)

Review

Verdict: comment — one medium-severity defense-in-depth improvement and several low-severity observations. No blocking issues.

This PR extracts failure reporting into a well-structured shared library (scripts/lib/post-failure-report.sh), adds categorized failure types and output sanitization, and extends both post-code.sh and post-fix.sh to post detailed failure comments. The design is sound — the shared library follows the existing scripts/lib/ pattern, the ERR trap + post_fail_to_{issue,pr} pattern is clean, and the token redaction is a strict improvement over the base (which had zero redaction in error comments).

Findings

1. sanitize_failure_detail() does not strip GHA workflow command sequences — Medium

File: scripts/lib/post-failure-report.sh

The sanitize_failure_detail() function redacts tokens and PEM keys but does not strip :: sequences or %0A/%0D URL-encoded newlines. The PR removes the SAFE_PUSH sanitization block from post-code.sh (lines 419–425 in the base) that previously stripped these sequences from push output before ::error:: interpolation.

In the new code path, push output and pre-commit output are echoed to the GHA runner log (echo "${PUSH_OUTPUT}", echo "${PRECOMMIT_OUTPUT}") before being passed to post_fail_to_issue/post_fail_to_pr. If the output contains GHA workflow command sequences (e.g., ::add-mask::, ::warning::), the runner will interpret them.

Mitigating factors: (1) The base code's echo "${PUSH_OUTPUT}" on line 409 was also unsanitized, so this is partially pre-existing; (2) The PR removes the specific ::error:: interpolation that was the main vector; (3) The comment body posted via gh issue comment is not subject to GHA command injection.

Recommendation: Add :: and %0A/%0D stripping to sanitize_failure_detail(), or create a dedicated sanitize_for_gha_log() function applied to all echo statements that output captured command results.

2. Org-mode URL routing test coverage — Low

File: scripts/post-code-test.sh

The base branch has 3 org-mode URL routing tests (error-comment-org-mode-uses-dispatch-repo, error-comment-org-mode-not-source-repo, error-comment-non-org-mode-fallback) that verify the GITHUB_REPOSITORY vs REPO_FULL_NAME distinction. The PR replaces these with the new run_failure_comment_test helper. The replacement test failure-comment-org-mode-uses-dispatch-repo covers the dispatch-repo case, but the negative test (URL must NOT contain the source repo) and the non-org-mode fallback test appear to be dropped.

Consider adding equivalent coverage for these edge cases.

3. ERR trap handler robustness — Low

File: scripts/lib/post-failure-report.sh

The report_post_failure_to_issue() / _to_pr() functions set POST_FAILURE_REPORTED=true before calling build_post_failure_comment(). Under set -e, if build_post_failure_comment fails (e.g., awk/sed error on malformed input), the handler aborts before reaching gh issue comment, and the re-entry guard prevents retry. The CI run still exits non-zero, but the error comment is silently skipped.

This is a theoretical concern (sed/awk are extremely unlikely to fail), and the base code has the same structural pattern.

4. Markdown code fence escape — Low

File: scripts/lib/post-failure-report.sh

build_post_failure_comment() wraps sanitized_detail in a markdown code fence (triple backticks). sanitize_failure_detail() does not strip triple-backtick sequences from the detail. Pre-commit output containing ``` could close the fence and inject arbitrary markdown into the comment. Impact is cosmetic (GitHub sanitizes HTML/scripts), not a security vulnerability.

5. POST_FAILURE_DETAIL_MAX_LINES undocumented — Low

Files: scripts/post-code.sh, scripts/post-fix.sh

The POST_FAILURE_DETAIL_MAX_LINES environment variable is configurable but not listed in the script headers' "Optional environment variables" sections. Since the library is sourced by both scripts, users setting this variable might not discover it without reading the library source.

[ralphbean]

I think we learned in the fix for the prioritize agent's CSMA lib fix that the post scripts of agents cannot reach a lib script. Right?

[ifireball]

Addressed @ralphbean's review with a script bundling build stage: .src.sh + .lib.sh sources are bundled into committed .sh artifacts via make script-build (harness still references scripts/post-*.sh).

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:45 PM UTC · Completed 12:59 PM UTC
Commit: 7529160 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:06 PM UTC · Ended 1:09 PM UTC
Commit: 2941769 · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 1:10 PM UTC · Ended 1:27 PM UTC
Commit: 2941769 · View workflow run →

[ifireball]

CI is green on f9542d5. Follow-up commits address the review feedback from 7529160:

• GHA log sanitization: print_sanitized_gha_log() strips :: / %0A / %0D from captured push/pre-commit/gitleaks output before printing to runner logs; same stripping added to sanitize_failure_detail() for comment bodies.
• Org-mode URL tests: restored failure-comment-org-mode-not-source-repo and failure-comment-non-org-mode-fallback.
• Shellcheck CI: fixed bundle-sh.sh SC2094 (mapfile), bundled post-prioritize.sh SC2034, and SC2005 via the print helper.
• Docs: POST_FAILURE_DETAIL_MAX_LINES documented in post-code/post-fix headers.

@ralphbean — the runtime lib sourcing concern should be addressed by the bundling build stage (make script-build inlines scripts/lib/*.lib.sh into committed .sh artifacts). Happy to adjust if you see a remaining gap.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:10 PM UTC · Completed 1:27 PM UTC
Commit: f9542d5 · View workflow run →

[ifireball]

@ralphbean Re: runtime lib sourcing — agreed, and addressed in 7529160+.

Post-scripts are now maintained as *.src.sh and bundled via make script-build (scripts/bundle-sh.sh inlines scripts/lib/*.lib.sh into the committed *.sh artifacts the harness fetches). No runtime source of sibling lib files.

post-prioritize follows the same pattern (CSMA lib re-extracted to github-api-csma.lib.sh and bundled). CI runs make check-bundle plus script-test in both source and bundled modes.

Happy to walk through the layout if helpful — otherwise a re-review/approval when you have a moment would unblock merge.

[waynesun09]

Merge queue ejection root cause — check-bundle fails on the merge-queue commit.

The merge queue creates a temporary commit merging PR head into main. Git textually merges the old post-code.sh (hand-written, from main) with the new post-code.sh (generated by the bundler, from this PR). The merged result doesn't match what bundle-sh.sh produces from post-code.src.sh, so make check-bundle detects the stale bundle and fails with exit 2.

This is a one-time bootstrap problem: the first PR that introduces the bundler will always hit this because main has the pre-bundler version of the scripts.

Possible fixes:

1. Add make script-build before check-bundle in the script-test.yml workflow (just for this PR, or permanently as a safety net)
2. Skip check-bundle when the merge-queue commit produces merge conflicts in bundled files
3. Squash-merge directly (bypassing the merge queue) since CI on the PR head already passes

Job log: https://github.com/fullsend-ai/agents/actions/runs/29539949315/job/87759861026

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:30 AM UTC · Completed 6:37 AM UTC
Commit: 6712e5a · View workflow run →

[fullsend-ai-review]

Review skipped — this PR is already merged.

The /fs-review command only reviews open pull requests.

_{Posted by fullsend post-review check}

[fullsend-ai-retro]

Retro: PR #38 — Categorized post-script failure reporting

PR: fullsend-ai/agents#38 by ifireball
Lifecycle: 12 days (Jul 7–19), 20 commits, 27 changed files (+3835/−309 lines)
Review runs: 18 dispatched → 9 successful, 9 cancelled (50% waste)

Timeline

1. Jul 7 — PR opened porting failure reporting from fullsend#2921. Review agent first pass: 1 MEDIUM + 4 LOW findings (GHA sanitization, commit type, edge cases). Qodo bot found 3 findings (unsanitized issue_number, protected paths, markdown fence injection).
2. Jul 7–12 — ralphbean identified runtime lib-sourcing constraint; author added script bundling system (*.src.sh → bundled *.sh via make script-build).
3. Jul 12–13 — Review agent ran 4 more passes, found only LOW findings. Approved at Round 4 (commit 04f52c7) with 2 LOW remaining.
4. Jul 13 — Human reviewer (waynesun09) ran independent multi-agent review squad (Claude×2 + Gemini). Found 3 HIGH + 6 MEDIUM findings — all missed by the platform review agent. Includes: Make mtime staleness bypass, bundle_is_lib containment bypass, permission scope gap, overly aggressive sanitizer, dead-code branch, missing secret redaction backstop, unverified architecture assumptions, and test coverage gaps.
5. Jul 13 — Author fixed all 9 findings in commit 1e45ee8.
6. Jul 14 — Human review squad Round 2: 1 CRITICAL + 1 HIGH + 3 MEDIUM new findings on fix commits. CRITICAL: symlink bypass of bundle_is_lib containment. HIGH: case-sensitive PEM redaction bypass. MEDIUM: glob-based token redaction, unsafe test file mutation, no cross-retry dedup.
7. Jul 14 — Author fixed all Round 2 findings. Review agent continued finding only LOW issues.
8. Jul 15–16 — maruiz93 docs feedback, rh-hemartin and maruiz93 approved. waynesun09 approved with 4 hardening suggestions (1 HIGH, 3 MEDIUM).
9. Jul 19 — Merged after merge-queue ejection fix.

Key findings

Review quality gap: The review agent's security sub-agent ran 9 successful passes using the opus model but found zero HIGH+ issues. The human reviewer's independent squad found 14 unique HIGH+ findings (1 CRITICAL, 4 HIGH, 9 MEDIUM) — all shell-script-specific security control bypass vectors that the platform agent entirely missed. The agent's approve verdict at Round 4 came on the exact same commit where the human found 3 HIGH + 6 MEDIUM.

Safety net worked: The post-review.sh protected-path gate correctly downgraded the approve verdict to comment-only for this scripts/-touching PR, preventing auto-merge. The multi-layer defense (agent verdict → protected-path gate → human review) functioned as designed.

Cancellation waste: 9/18 review runs cancelled, with 6 in a 20-minute window during fix iterations on Jul 13. This is consistent with existing debounce proposals (fullsend#4960, #5139, #5210) and provides additional quantitative evidence.

No false positives: Both the review agent and human reviewer had zero false positives. The agent's findings were valid but surface-level; the human's were deeper and security-critical.

Proposals

Two proposals filed targeting the security and correctness sub-agent prompts in fullsend-ai/agents.

Proposals filed

• Security sub-agent should verify shell script security controls against bypass vectors (in fullsend-ai/agents)
• Correctness sub-agent should flag security-critical functions lacking adversarial test coverage (in fullsend-ai/agents)

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:36 AM UTC · Completed 7:01 AM UTC
Commit: 6712e5a · View workflow run →