Skip to content

feat(#505): post comment on issue when agent takes no-op exit path - #508

Merged
ggallen merged 3 commits into
mainfrom
agent/505-noop-issue-comment
Jul 28, 2026
Merged

feat(#505): post comment on issue when agent takes no-op exit path#508
ggallen merged 3 commits into
mainfrom
agent/505-noop-issue-comment

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

When post-code.sh takes a no-op exit path (agent determined no changes needed), the source issue now receives an informational comment explaining why no PR was created. Previously, the issue got zero feedback — users had to dig into workflow logs.

Adds a post_noop_comment() helper called at each of the three no-op exits:

  • No feature branch — agent stayed on main/master or detached HEAD
  • No changed files — agent committed nothing
  • All agent artifacts — only .agentready/ / .fullsend-workspace/ files were present

The comment includes the specific reason, a workflow run link, and sanitized agent context from the result file when available. Posting is best-effort — failure does not change the exit code.

Testing

  • 11 new unit tests for the comment body construction (build_noop_comment helper)
  • Tests verify: reason text, issue number, workflow run URL, agent context presence/absence, no token leaks, retry instruction, artifacts reason
  • Existing detect_noop tests continue to pass (no behavioral change to detection logic)
  • Bundle check (make check-bundle) passes
  • script-has-noop-comment check verifies the function is present in the bundled script

Closes #505

Post-script verification

  • Branch is not main/master (agent/505-noop-issue-comment)
  • Secret scan passed (gitleaks — 8b770c4c19ceed8548eb75281e40eb59709a1b34..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

When the code agent determines no changes are needed and post-code.sh
takes a no-op exit path, the source issue previously received no
feedback. Users had to dig into workflow logs to understand why no PR
was created, leading to confusion and unnecessary /fs-code re-triggers.

Add a post_noop_comment() helper that posts an informational comment
on the source issue at each of the three no-op exit paths:

1. No feature branch — agent stayed on main/master or detached HEAD
2. No changed files — agent committed nothing
3. All agent artifacts — only .agentready/.fullsend-workspace/ files

The comment includes the specific reason, a workflow run link, and
agent context from the result file (pr_body) when available. The
agent context is sanitized via sanitize_failure_detail to strip
tokens and workflow commands. Comment posting is best-effort — a
failure does not change the exit code.

Closes #505
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 28, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:35 PM UTC · Completed 4:49 PM UTC
Commit: a896335 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] scripts/ — All three changed files (scripts/post-code.sh, scripts/post-code.src.sh, scripts/post-code-test.sh) are under the scripts/ protected path. The PR links to issue post-code.sh: comment on issue when agent determines no changes needed #505 and explains the rationale clearly. Human approval is always required for protected-path changes regardless of context.

Low

  • [scope-expansion] scripts/post-code.src.sh:271 — The PR adds post_noop_comment at the "all agent artifacts" exit path, which issue post-code.sh: comment on issue when agent determines no changes needed #505 does not explicitly mention (it references "the two no-op paths"). The expansion is natural — this is another no-op exit producing the same user-facing silence — but worth noting for scope awareness.
Previous run

Review

Findings

Medium

  • [protected-path] scripts/ — All three changed files (scripts/post-code.sh, scripts/post-code.src.sh, scripts/post-code-test.sh) are under the scripts/ protected path. The PR links to issue post-code.sh: comment on issue when agent determines no changes needed #505 and explains the rationale clearly. Human approval is always required for protected-path changes regardless of context.

Low

  • [error-handling] scripts/post-code.src.sh:148post_noop_comment reads .pr_body from RESULT_FILE for agent context, but is called from exit paths where the agent may not have produced a result file. The guards prevent crashes and sanitization prevents data leaks, but stale agent context from a prior iteration could produce a misleading (though harmless) comment.

  • [injection] scripts/post-code.src.sh:162 — Raw ISSUE_NUMBER is interpolated into the comment body (issue #${ISSUE_NUMBER}) without using the already-computed safe_issue_number. The existing codebase uses raw ISSUE_NUMBER in comment bodies elsewhere, so this is consistent but not ideal for defense-in-depth.

  • [test-fidelity] scripts/post-code-test.sh:1035 — The test helper build_noop_comment reimplements the comment body construction rather than testing the production function directly. This follows the established test architecture in this file but carries inherent drift risk if the production template changes.

  • [scope-expansion] scripts/post-code.src.sh:267 — The PR adds post_noop_comment at the "all agent artifacts" exit path, which issue post-code.sh: comment on issue when agent determines no changes needed #505 does not explicitly mention (it references "the two no-op paths"). The expansion is natural — this is another no-op exit producing the same user-facing silence — but worth noting for scope awareness.

Previous run (2)

Review

Findings

Medium

  • [protected-path] scripts/ — All three changed files (scripts/post-code.sh, scripts/post-code.src.sh, scripts/post-code-test.sh) are under the scripts/ protected path. The PR links to issue post-code.sh: comment on issue when agent determines no changes needed #505 and explains the rationale clearly. Human approval is always required for protected-path changes regardless of context.

Low

  • [error-handling-idiom] scripts/post-code.shpost_noop_comment does not pre-sanitize ISSUE_NUMBER via _sanitize_workflow_value before interpolating it in the gha_echo warning message. The existing report_post_failure_to_issue pre-sanitizes into safe_issue_number as defense-in-depth. gha_echo itself already sanitizes via sanitize_gha_log_output, so no actual injection is possible — this is a consistency concern only.

  • [test-fidelity] scripts/post-code-test.sh:1035 — The test helper build_noop_comment reimplements the comment body construction rather than testing the production function directly. This follows the established test architecture in this file (all sections use reimplementations), but carries inherent drift risk if the production template changes.

  • [scope-expansion] scripts/post-code.sh:895 — The PR adds post_noop_comment at the "all agent artifacts" exit path, which issue post-code.sh: comment on issue when agent determines no changes needed #505 does not explicitly mention. The expansion is natural — this is another no-op exit producing the same user-facing silence — but worth noting for scope awareness.


Labels: PR modifies the code agent post-script infrastructure (scripts/post-code.sh, scripts/post-code.src.sh, scripts/post-code-test.sh)

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment code-agent labels Jul 28, 2026
@ggallen

ggallen commented Jul 28, 2026

Copy link
Copy Markdown
Member

/fs-fix - Rebase and fix all of the review issues, both inline and in the stick comment. Also resolve all conversations that you have fixed.

@ggallen ggallen closed this Jul 28, 2026
@ggallen ggallen reopened this Jul 28, 2026
@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Fix · ❌ Terminated · Started 4:55 PM UTC · Ended 5:01 PM UTC
Commit: a896335 · View workflow run →

@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 28, 2026

Copy link
Copy Markdown

🤖 Retro · ⚠️ Cancelled · Started 4:56 PM UTC · Ended 4:56 PM UTC
Commit: a896335 · View workflow run →

Pre-sanitize ISSUE_NUMBER via _sanitize_workflow_value before
interpolating it in the gha_echo warning message, consistent with
report_post_failure_to_issue.

Addresses review feedback on #508
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 4:55 PM UTC · Completed 5:01 PM UTC
Commit: a896335 · View workflow run →

@ggallen

ggallen commented Jul 28, 2026

Copy link
Copy Markdown
Member

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 28, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:08 PM UTC · Completed 5:25 PM UTC
Commit: 33e6bfb · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@ggallen

ggallen commented Jul 28, 2026

Copy link
Copy Markdown
Member

/fs-fix - Rebase and fix all of the review issues, both inline and in the stick comment. Also resolve all conversations that you have fixed.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 5:28 PM UTC · Completed 5:35 PM UTC
Commit: 33e6bfb · View workflow run →

…ale context

- Use safe_issue_number in the no-op comment body text for defense-in-depth
  consistency with gha_echo messages (addresses [injection] review finding)
- Add comment documenting that RESULT_FILE may point to a prior iteration's
  output at no-op exit paths; context is sanitized and best-effort so this
  is acceptable (addresses [error-handling] review finding)

Addresses review feedback on #508
@ggallen

ggallen commented Jul 28, 2026

Copy link
Copy Markdown
Member

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 28, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:53 PM UTC · Completed 6:12 PM UTC
Commit: 803fd67 · View workflow run →

@ggallen
ggallen added this pull request to the merge queue Jul 28, 2026
Merged via the queue into main with commit 8b9e25e Jul 28, 2026
12 checks passed
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 28, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 6:21 PM UTC · Completed 6:33 PM UTC
Commit: 803fd67 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #508 — post comment on issue when agent takes no-op exit path

Timeline

Issue #505 (opened May 8 by ascerra) requested that post-code.sh post an explanatory comment on the source issue when the code agent determines no changes are needed. On Jul 28, ggallen added a fresh real-world example and triggered the pipeline:

  1. Triage (15:43–15:47 UTC, run 30374615691) — correctly identified 3 no-op exit paths (vs. 2 in the issue), proposed a clear implementation plan with test suggestions.
  2. Code (15:54–16:01 UTC, run 30375541913) — produced a solid 256-line implementation across 3 files with 11 test cases. Covered all 3 exit paths with sanitization, best-effort posting, and agent context extraction.
  3. Review 1 (16:35–16:49 UTC, run 30378863639) on commit a896335 — found 4 findings: protected-path (medium, structural), error-handling-idiom (low, challenger downgraded from medium), test-fidelity (low), scope-expansion (low).
  4. Fix 1 (16:55–17:01 UTC, run 30380435242) — addressed the error-handling-idiom finding (added safe_issue_number sanitization in gha_echo). Correctly assessed remaining findings as non-actionable. Pushed 33e6bfb.
  5. Review 2 (17:08–17:25 UTC, run 30381434832) on commit 33e6bfb — found 2 new medium-severity findings that Review 1 missed: (a) raw ISSUE_NUMBER interpolated in the comment body without sanitization, (b) stale RESULT_FILE edge case at no-op exit paths.
  6. Fix 2 (17:28–17:35 UTC, run 30382926532) — addressed both new findings: switched to safe_issue_number in comment body, added inline documentation about stale RESULT_FILE. Pushed 803fd67.
  7. Review 3 (17:53–18:12 UTC, run 30384841747) on commit 803fd67 — confirmed prior findings resolved. Only protected-path (medium, structural) and scope-expansion (low) remained.
  8. Human approval (18:16 UTC) by ggallen → merged (18:18 UTC).

Total time issue-to-merge: ~2h 37m. The extra review+fix cycle (steps 5–6) added ~1 hour.

What went well

  • Triage quality was excellent — it correctly identified a third no-op exit path beyond the two mentioned in the issue body, proposed concrete test suggestions, and noted related issue Code agent 'successful' but didn't create a PR #437.
  • Code agent quality was strong — the initial implementation was well-structured, comprehensive (covering all 3 exit paths), included tests, and used existing sanitization infrastructure. The scope expansion to the third exit path was the right engineering call.
  • Review agent finding quality was good across all rounds — all findings were legitimate, well-reasoned, and provided actionable suggested fixes. The challenger pass was effective (correctly downgrading or removing speculative findings).
  • Fix agent was effective — correctly distinguished actionable from informational findings, made targeted changes, and ran verification (tests, secret scan, bundle check) before pushing.
  • Human reviewer approved efficiently after confirming agent-identified issues were resolved.

What could go better

The primary rework driver was Review 1 missing two medium-severity findings that Review 2 subsequently caught on essentially the same code. The injection finding (raw ISSUE_NUMBER in comment body) and the stale RESULT_FILE concern both existed in the original commit a896335 that Review 1 examined. Review 2 found them on commit 33e6bfb, which only differed by a one-line sanitization change in gha_echo. This caused an unnecessary review→fix→review cycle costing ~1 hour of wall time and ~$4 in agent compute.

Evidence for existing issues

  • #380 (Correctness sub-agent should verify symmetric application of defensive patterns): The injection finding is a textbook case — safe_issue_number was used in the gha_echo call but raw ISSUE_NUMBER in the comment body, within the same function. A symmetric-application check would have caught this in Review 1.
  • #478 / fullsend#956 (thread reply/resolution): Both fix runs attempted to resolve PR review threads per the human's instruction but were blocked by sandbox GraphQL restrictions. The fix agent's inability to call resolveReviewThread is a gap between Post-fix script should reply to review inline threads it addressed #478 (scoped to replies only) and fullsend#956 (scoped to the review agent).

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

post-code.sh: comment on issue when agent determines no changes needed

1 participant