feat(harness): report categorized post-script failures on issue/PR#38
feat(harness): report categorized post-script failures on issue/PR#38ifireball wants to merge 15 commits into
Conversation
Port fullsend-ai/fullsend#2921: add shared post-failure-report.sh with categorized failure comments, output sanitization, and workflow run links. post-code.sh posts detailed failures on the issue; post-fix.sh posts equivalent comments on the PR. Extend post-code-test.sh and post-fix-test.sh. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 7:39 AM UTC · Completed 7:54 AM UTC |
PR Summary by QodoReport categorized post-script failures on issues/PRs (shared failure reporter)
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewRe-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
New changes verified correct
FindingsLow
Previous runReviewVerdict: 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 ( Prior review findings — resolvedBoth prior findings have been addressed:
New changes verified correct
Findings1. Bundler source-line regex does not handle inline comments — LowFile: Severity anchored from prior review (04f52c7). The regex
Previous run (2)ReviewVerdict: 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 ( Prior review findings — resolvedAll four prior findings have been addressed:
Findings1.
|
fullsend-ai-review
Bot
added
the
requires-manual-review
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
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?
Bring in script-test CI, post-scribe-test, and other mainline changes before adding the script bundling build stage. Signed-off-by: Barak Korren <bkorren@redhat.com>
|
Addressed @ralphbean's review with a script bundling build stage: |
|
🤖 Finished Review · ✅ Success · Started 12:45 PM UTC · Completed 12:59 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Review · |
|
🤖 Review · ❌ Terminated · Started 1:10 PM UTC · Ended 1:27 PM UTC |
|
CI is green on
@ralphbean — the runtime lib sourcing concern should be addressed by the bundling build stage ( |
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 1:10 PM UTC · Completed 1:27 PM UTC |
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
@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.
…cripts Introduce bundle-sh.sh to inline scripts/lib/*.lib.sh into .src.sh sources, producing committed .sh artifacts for harness deployment. Add script-build and check-bundle Makefile targets, split lib tests into post-failure-report-test, and run script-test in source and bundled modes in CI. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Sanitize captured command output before echoing to GHA logs, restore org-mode URL routing tests, fix bundle-sh SC2094, and document POST_FAILURE_DETAIL_MAX_LINES in post-script headers. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…test Add gha_echo() for sanitised ::warning::/::error::/::notice:: output, cover cross-library bundle dedup in bundle-sh-test, and extend review follow-ups from fullsend-ai-review. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad pass (Claude x2, Gemini) — 9 unique Medium+ findings posted inline below. Previously-raised items (protected scripts/ paths, markdown fence injection, runtime lib-sourcing, unsanitized issue_number in workflow commands) were verified as already resolved/acknowledged and are not re-flagged.
Regenerate bundles unconditionally in check-bundle to avoid Make mtime false positives, restrict bundle-sh lib inlining to scripts/lib only, split comment vs log GHA sanitization, add push-failed category and PUSH_TOKEN literal redaction, document issues:write scope, and expand bundle and failure-report test coverage. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
Add scripts/lib/*.lib.sh as Makefile prerequisites for script-build, avoid static PEM literals in tests, and fix shellcheck SC2034 in the failure-report test helper. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
…ilure-reporting Signed-off-by: Barak Korren <bkorren@redhat.com>
Merge upstream/main (eval agent selection, GITHUB_WORKSPACE guard from fullsend-ai#67) into post-code/post-fix sources and regenerate bundled artifacts so check-bundle passes on the PR merge ref. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
ifireball
force-pushed
the
feat/post-script-failure-reporting
branch
from
487d4b8 to
dd38d47
Compare
|
🤖 Review · |
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ifireball
force-pushed
the
feat/post-script-failure-reporting
branch
from
dd38d47 to
679e1ca
Compare
|
🤖 Review · ❌ Terminated · Started 10:32 PM UTC · Ended 10:46 PM UTC |
|
@waynesun09 — all 9 inline findings addressed in
@ralphbean — bundling ( |
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 10:32 PM UTC · Completed 10:45 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass (3 agents: Claude, Claude, Gemini) on the current head, after independently confirming all 9 of my prior findings (3 HIGH + 6 MEDIUM) are genuinely fixed with real engineering work. This round found new issues in the areas those fixes touched — most notably a CRITICAL supply-chain gap: the lib-containment check was tightened at the glob-pattern level but never added symlink/identity resolution, so a symlinked .lib.sh file still bypasses it and gets its target content inlined into the committed script artifact (verified with a working PoC). Also a HIGH secret-redaction bypass via PEM header casing (also PoC-verified). Recommend fixing both before merge; the 3 MEDIUM findings are good hardening but not blocking.
Canonicalize bundled lib paths with readlink -f to reject symlink bypasses, make PEM redaction case-insensitive, and redact literal tokens without glob matching. Persist a runner-temp marker per workflow run to avoid duplicate failure comments across re-source and retries. Run check-bundle drift test against a temp repo copy instead of mutating committed scripts. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
POST_FAILURE_REPORTED now only prevents duplicate posts within a single script invocation. Workflow re-runs intentionally post a fresh comment so users can see each retry produced new results. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 6:11 AM UTC · Completed 6:26 AM UTC |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| fi | ||
| first=false | ||
|
|
||
| if [[ "${line}" =~ ^[[:space:]]*source[[:space:]]+(.+)[[:space:]]*$ ]]; then |
There was a problem hiding this comment.
[low] edge-case
Bundler source-line regex does not handle inline comments. The regex ^[[:space:]]source[[:space:]]+(.+)[[:space:]]$ captures everything after source, including inline comments. Currently no files use inline comments on source lines, so not exploitable today. Severity anchored from prior review.
| if command -v realpath >/dev/null 2>&1; then | ||
| realpath "${path}" 2>/dev/null && return 0 | ||
| fi | ||
| printf '%s' "${path}" |
There was a problem hiding this comment.
[low] edge-case
bundle_canonical_path() silently falls back to the unresolved path when neither readlink -f nor realpath is available. The symlink protection would not detect symlinks pointing outside scripts/lib/ on such systems. CI (ubuntu-24.04) is not affected.
Suggested fix: Consider emitting a warning when the fallback is reached and the path is a symlink.
| return 0 | ||
| fi | ||
|
|
||
| awk -v token="${token}" -v repl='[REDACTED]' ' |
There was a problem hiding this comment.
[low] secrets-handling
_redact_literal_token() passes the token to awk via -v, which interprets C-style backslash escape sequences. A token containing a literal backslash followed by an awk escape character would not match its occurrences. GitHub tokens use base-62 + underscore and never contain backslashes, so the risk is theoretical for current callers.
Suggested fix: Consider using awk ENVIRON instead of -v for fully literal matching.
fullsend-ai-review
Bot
added
requires-manual-review
3 participants
Summary
Ports fullsend-ai/fullsend#2921: categorized post-script failure reporting for
post-code.shandpost-fix.sh.Failure reporting
scripts/lib/post-failure-report.lib.shwith categorized failure comments, output sanitization (tokens/PEM redaction, GHA workflow-command stripping), and workflow run linkspost-code.shposts detailed failure comments on the issue (push rejected, pre-commit blocked, secret scan, PR creation failed, etc.)post-fix.shposts equivalent failure comments on the PR (previously silent on many post-script failures)post-code-test.sh/post-fix-test.shand addpost-failure-report-test.shfor comment building, sanitization, org-mode URL routing, and push categorizationScript bundling (added during review)
The harness fetches each runner script as an isolated blob, so post-scripts cannot
sourcefiles fromscripts/lib/at runtime (same constraint that motivated the CSMA lib fix forpost-prioritize.sh). To keep shared failure-reporting logic DRY without breaking runtime, this PR adds a small build stage:scripts/lib/*.lib.shscripts/*.src.shsourcecallsscripts/*.sh(from.src.sh)Build tooling:
scripts/bundle-sh.sh— inlinesscripts/lib/*.lib.shinto self-contained.shartifacts (nested sources, deduped)make script-build/make check-bundle— regenerate and verify committed bundlesscript-test.yml) runscheck-bundle, thenscript-testin bothsourceandbundledmodesScripts converted to the source→bundle workflow:
post-code.src.sh→post-code.shpost-fix.src.sh→post-fix.shpost-prioritize.src.sh→post-prioritize.sh(re-extractsgithub-api-csma.lib.shfrom the prior inline copy)After editing a
.src.shor.lib.shfile, runmake script-buildand commit source + bundled files together. See README “Script bundling” section.Test plan
make script-build && make check-bundlemake script-test(source mode)make script-test SCRIPT_TEST_TARGET=bundledbash scripts/post-failure-report-test.shf9542d5