Skip to content

feat(#3697): add emoji reaction status notifications - #5957

Merged
ralphbean merged 11 commits into
mainfrom
feat/3697-emoji-reaction-notifications
Aug 24, 2026
Merged

feat(#3697): add emoji reaction status notifications#5957
ralphbean merged 11 commits into
mainfrom
feat/3697-emoji-reaction-notifications

Conversation

@ralphbean

@ralphbean ralphbean commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

Stacked on #5736 — this adds the "react to the issue with an emoji" alternative from #3697, as a supplement/alternative to status comments.

Blocked on: #5994perRepoConfig has no status_notifications field, and the live pkg/behaviourtest e2e suite only supports per-repo installs, so there is currently no test harness that can enable reactions to exercise the maintainer-mandated behavior test below.

  • New AddIssueReaction/DeleteIssueReaction on forge.Client, implemented for GitHub (GitLab returns ErrNotSupported — no caller exercises that path yet)
  • New AddIssueCommentReaction/DeleteIssueCommentReaction on forge.Client so reactions can target the triggering comment for slash-command-invoked runs, per Triage agent causes unnecessary notifications - should skip initial comment #3697's explicit requirement
  • New status_notifications.reaction config block (start/completion, same enabled/on_failure/disabled values as comment), defaulting to disabled since it's an opt-in addition
  • Notifier.PostStart adds a 👀 reaction when reaction.start: enabled; PostCompletionWithDetail swaps it for 👍 (success) or 😕 (failure/cancelled/skipped/unrecognized) depending on reaction.completion
  • Reactions target the triggering comment (via --status-comment-id, wired through action.yml and all reusable workflow call sites) when the run was invoked by a slash command, and the issue/PR otherwise
  • Reactions generate no GitHub notification, so unlike comments they don't need the on_failure start-suppression workaround
  • Docs updated under "Status Notifications" in docs/guides/user/customizing-agents.md

Out of scope:

  • ReconcileOrphaned does not yet reconcile orphaned reaction state on hard-killed runs — plumbing a reaction ID across process boundaries would need more design than is justified here.
  • Reactions have no per-run identity (GitHub's reactions API is keyed by actor+subject+content), so concurrent same-role runs can collide on the same reaction. Documented as a known limitation with inline code comments; not fixable without GitHub API support.
  • GitLab silently no-ops on reaction calls (ErrNotSupported) with no config-time validation warning users their reaction settings do nothing. Documented as a known limitation; follow-up needed (also applies to the new JIRA poll input driver).
  • Live e2e behavior test for "slash command targets the comment, not the issue" — blocked on perRepoConfig has no status_notifications field — reactions/comments toggles are org-only #5994. Covered today by unit tests only (TestPostStart_ReactionTargetsTriggeringComment, TestPostCompletion_ReactionTargetsTriggeringComment in internal/statuscomment/statuscomment_test.go).

Test plan

  • go build ./...
  • go vet ./...
  • gofmt -l clean
  • go test ./... — all green except pre-existing, unrelated internal/runtime failures (verified present on base branch too)
  • New unit tests for config parsing/validation, FakeClient, GitHub REST calls, and Notifier reaction lifecycle (start/completion/on_failure/cleanup, comment-scoped targeting)
  • Vendor a binary built off this branch and try it against a real test repo
  • Live e2e behavior test for comment-scoped reactions — blocked on perRepoConfig has no status_notifications field — reactions/comments toggles are org-only #5994

Assisted-by: Claude Opus 4.6 noreply@anthropic.com

@ralphbean
ralphbean requested a review from a team as a code owner August 5, 2026 21:34
@ralphbean ralphbean added the fullsend-fix Enables automatic bot-triggered fix runs on human-authored PRs label Aug 5, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 5, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:34 PM UTC · Completed 9:53 PM UTC
Commit: 390f99e · View workflow run →

@qodo-code-review

qodo-code-review Bot commented Aug 5, 2026

Copy link
Copy Markdown

PR Summary by Qodo

Add emoji reaction status notifications (with comment-targeting for slash commands)

✨ Enhancement ⚙️ Configuration changes 📝 Documentation 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Add opt-in status notification reactions (👀 start, 👍/😕 completion) alongside comments.
• Wire slash-command runs to react on the triggering comment via status-comment-id.
• Extend orphaned status reconciliation to respect on_failure mode and CI job outcome.
Diagram

graph TD
  WF["GitHub reusable workflows"] --> ACT["Composite action (action.yml)"] --> RUN["fullsend run (CLI)"] --> NOTIF["statuscomment.Notifier"] --> FORGE["forge.Client"] --> GHAPI["GitHub Issues/Reactions API"]
  WF --> RECON["Post-job reconcile-status"] --> ORPH["ReconcileOrphaned"] --> FORGE
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use GitHub Checks/Commit Statuses instead of reactions
  • ➕ Native UI on PRs with rich status context and links
  • ➕ Avoids comment/reaction collisions and doesn’t depend on reactions API quirks
  • ➖ Different permission/model (checks/status) and more setup complexity
  • ➖ Doesn’t satisfy the explicit requirement to react on the triggering slash-command comment
2. Implement GitLab award-emoji support now
  • ➕ Consistent cross-forge behavior; avoids silent no-op on GitLab
  • ➖ More API surface/edge cases (award IDs, permissions) with limited current coverage
  • ➖ Higher risk without an existing caller/test harness for GitLab reaction flows
3. Persist reaction identity for reconciliation (e.g., write reaction info into a marker comment)
  • ➕ Could enable out-of-process cleanup of orphaned 👀 reactions after hard kills
  • ➖ GitHub reactions are keyed by actor+subject+content; IDs aren’t stable across concurrent runs
  • ➖ Adds complexity and cross-process state plumbing that may not be reliable anyway

Recommendation: The PR’s approach (opt-in reactions layered onto the existing status notifier, with comment-targeting for slash commands) best matches the stated UX goal: low-noise status signals without extra GitHub notifications. A future follow-up worth considering is implementing GitLab’s award-emoji API (with config-time warnings when unsupported) once there’s a test harness/caller to validate the behavior.

Files changed (24) +1464 / -106

Enhancement (6) +343 / -32
reconcilestatus.goPass completion mode + CI job status into orphan reconciliation +35/-9

Pass completion mode + CI job status into orphan reconciliation

• Extends reconcile-status to accept --fullsend-dir and --job-status, loads org config (when available) to detect comment.completion mode, and calls ReconcileOrphaned with completionMode/jobStatus.

internal/cli/reconcilestatus.go

run.goAllow notifier to target triggering comment for reactions +9/-4

Allow notifier to target triggering comment for reactions

• Adds --status-comment-id flag to run and wires it into the status notifier via SetTriggerCommentID when present.

internal/cli/run.go

forge.goExtend forge.Client with issue/comment reaction methods +27/-0

Extend forge.Client with issue/comment reaction methods

• Adds new interface methods for adding/deleting reactions on issues and issue comments, documenting supported GitHub reaction content and ErrNotSupported semantics.

internal/forge/forge.go

github.goImplement issue/comment reactions for GitHub client +51/-0

Implement issue/comment reactions for GitHub client

• Implements Add/DeleteIssueReaction and Add/DeleteIssueCommentReaction against GitHub’s reactions API, with local validation of allowed content values and response ID decoding.

internal/forge/github/github.go

issue.goStub reaction methods on GitLab client as unsupported +22/-0

Stub reaction methods on GitLab client as unsupported

• Adds reaction methods that return forge.ErrNotSupported, documenting that GitLab award-emoji support is not implemented yet.

internal/forge/gitlab/issue.go

statuscomment.goAdd reaction-based status notifications and shared on_failure logic +199/-19

Add reaction-based status notifications and shared on_failure logic

• Introduces reaction lifecycle management (👀 start, 👍/😕 completion) with opt-in defaults, and adds triggerCommentID support so slash-command runs react on the invoking comment. Refactors completion decision logic to support on_failure for both comments and reactions, and extends ReconcileOrphaned to synthesize interrupted comments when on_failure suppresses start markers but the job fails/cancels.

internal/statuscomment/statuscomment.go

Tests (5) +881 / -12
reconcilestatus_test.goAdd tests for config-derived completion mode in reconcile-status +143/-0

Add tests for config-derived completion mode in reconcile-status

• Adds stubbing helpers and tests verifying reconcile-status reads org config from --fullsend-dir, handles malformed/missing configs, and forwards completionMode/jobStatus to ReconcileOrphaned.

internal/cli/reconcilestatus_test.go

config_test.goTest status notification validation and parsing for on_failure and reactions +180/-0

Test status notification validation and parsing for on_failure and reactions

• Adds coverage ensuring on_failure is accepted only for completion fields, reaction settings parse/validate correctly, and marshaling includes the reaction block.

internal/config/config_test.go

fake_test.goTest FakeClient reaction recording and error injection +64/-0

Test FakeClient reaction recording and error injection

• Adds unit tests for Add/Delete issue reactions and comment reactions, verifying IDs increment and calls are recorded correctly.

internal/forge/fake_test.go

github_comment_test.goAdd HTTP-level tests for GitHub reactions endpoints +88/-0

Add HTTP-level tests for GitHub reactions endpoints

• Adds tests that assert correct GitHub API paths/methods and payloads for issue and comment reactions, plus invalid-content short-circuit behavior.

internal/forge/github/github_comment_test.go

statuscomment_test.goExpand tests for on_failure behavior, reactions, and comment-targeting +406/-12

Expand tests for on_failure behavior, reactions, and comment-targeting

• Adds unit tests covering comment on_failure suppression/posting behavior, reaction start/completion swap semantics, non-fatal reaction failures, reaction/comment consistency on comment API failure, comment-targeted reactions for slash-command runs, and new ReconcileOrphaned synthesis behavior gated by completionMode/jobStatus.

internal/statuscomment/statuscomment_test.go

Documentation (4) +64 / -14
cli-internals.mdDocument new reconcile-status flags +3/-1

Document new reconcile-status flags

• Updates the CLI internals reference to include --fullsend-dir and --job-status for reconcile-status.

docs/guides/dev/cli-internals.md

operations.mdPoint operators to user-facing Status Notifications docs +3/-12

Point operators to user-facing Status Notifications docs

• Replaces embedded status notification docs with a link to the user guide and updates the composite action input table to include status-comment-id.

docs/guides/getting-started/operations.md

customizing-agents.mdAdd full Status Notifications guide (comments + reactions) +57/-0

Add full Status Notifications guide (comments + reactions)

• Adds a dedicated Status Notifications section documenting comment completion modes (including on_failure) and the new reaction.start/reaction.completion settings, defaults, and limitations (GitHub-only, orphaned start reactions).

docs/guides/user/customizing-agents.md

running-agents-locally.mdUpdate Status Notifications doc link +1/-1

Update Status Notifications doc link

• Updates the reference from operations.md to the new Status Notifications section in customizing-agents.md.

docs/guides/user/running-agents-locally.md

Other (9) +176 / -48
reusable-code.ymlPass triggering comment id into status notification inputs +1/-0

Pass triggering comment id into status notification inputs

• Adds status-comment-id input wiring so slash-command-triggered runs can target reactions on the originating comment.

.github/workflows/reusable-code.yml

reusable-dispatch.ymlPropagate status-comment-id across dispatch workflow variants +6/-0

Propagate status-comment-id across dispatch workflow variants

• Threads the triggering comment id through multiple job paths so the composite action can scope reactions to the slash-command comment when available.

.github/workflows/reusable-dispatch.yml

reusable-fix.ymlForward slash-command comment id to action inputs +1/-0

Forward slash-command comment id to action inputs

• Adds status-comment-id forwarding for fix workflow runs invoked by comment events.

.github/workflows/reusable-fix.yml

reusable-retro.ymlForward slash-command comment id to action inputs +1/-0

Forward slash-command comment id to action inputs

• Adds status-comment-id so retro runs can attach reactions to the triggering comment when invoked via slash command.

.github/workflows/reusable-retro.yml

reusable-review.ymlForward slash-command comment id to action inputs +1/-0

Forward slash-command comment id to action inputs

• Adds status-comment-id wiring for review workflow runs so reactions can target the invoking comment.

.github/workflows/reusable-review.yml

reusable-triage.ymlForward slash-command comment id to action inputs +1/-0

Forward slash-command comment id to action inputs

• Adds status-comment-id propagation for triage runs invoked by issue comment events.

.github/workflows/reusable-triage.yml

action.ymlAdd status-comment-id input and improve post-job reconcile context +23/-7

Add status-comment-id input and improve post-job reconcile context

• Introduces a new optional status-comment-id input and passes it to the CLI as --status-comment-id. Reorders artifact upload to ensure reconcile-status runs last and extends reconciliation flags with --fullsend-dir and --job-status.

action.yml

config.goAdd status_notifications.reaction config block and validation helpers +39/-9

Add status_notifications.reaction config block and validation helpers

• Extends StatusNotificationConfig with Reaction settings, defines valid start/completion value sets (including on_failure for completion), and centralizes validation in a helper.

internal/config/config.go

fake.goAdd reaction APIs to FakeClient for tests +103/-32

Add reaction APIs to FakeClient for tests

• Adds record types and implementations for issue and comment reaction add/delete operations, including ID generation and error injection support.

internal/forge/fake.go

ralphbean added a commit to appdumpster/test-repo that referenced this pull request Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Site preview

Preview: https://5064faff-site.fullsend-ai.workers.dev

Commit: 730c0a760a21aa465b9eb553d507772018e87421

@qodo-code-review

qodo-code-review Bot commented Aug 5, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. Comment ID never forwarded ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New
Description
The new status-comment-id: ${{ fromJSON(...).comment.id }} workflow inputs cannot work for
dispatcher-generated slash-command runs because the dispatcher-built event_payload.comment object
does not include an id field, so reactions will always fall back to targeting the issue/PR instead
of the triggering comment.
Code

.github/workflows/reusable-dispatch.yml[635]

+          status-comment-id: ${{ fromJSON(needs.route.outputs.event_payload).comment.id }}
Relevance

●●● Strong

Breaks stated slash-command comment-targeting behavior; team usually fixes reusable-dispatch
workflow logic issues.

PR-#1688
PR-#2106

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The per-repo dispatcher builds event_payload.comment with only body, so .comment.id cannot be
present. The org-mode dispatcher template builds the same minimal payload shape, so per-org reusable
workflows also cannot supply a comment ID via inputs.event_payload. The newly added
status-comment-id line therefore reads a field that is never serialized into the payload.

.github/workflows/reusable-dispatch.yml[513-519]
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml[485-494]
.github/workflows/reusable-dispatch.yml[632-637]

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

## Issue description
Workflows now pass `status-comment-id: ${{ fromJSON(...).comment.id }}`, but the dispatcher-generated `event_payload` JSON omits `comment.id` (it only includes `comment.body`). As a result, comment-scoped reactions for slash-command runs can never be targeted at the triggering comment.

## Issue Context
Both the per-repo inlined dispatcher (`.github/workflows/reusable-dispatch.yml`) and the scaffolded org-mode dispatcher template (`internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml`) build a minimal `event_payload` using `jq` and currently serialize `comment` as `{body: ...}` only.

## Fix Focus Areas
- .github/workflows/reusable-dispatch.yml[513-519]
- internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml[485-494]
- .github/workflows/reusable-dispatch.yml[632-637]

## Suggested fix
1. Update the `jq` payload builders to include the comment ID when a comment exists, e.g.:
  - `comment: (.comment // null | if . then {id, body: .body[:4096]} else null end)`
2. (Optional hardening) In the workflow call sites, use a null-safe fallback so missing comment IDs don’t propagate as a non-numeric string:
  - `status-comment-id: ${{ fromJSON(...).comment.id || '' }}`

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



Remediation recommended

2. Reaction swap before comment ✓ Resolved 🐞 Bug ☼ Reliability
Description
Notifier.PostCompletionWithDetail updates/removes reactions before attempting to create/update the
completion comment, so a later comment API failure can leave reactions indicating completion while
the status comment remains in the "Started" state (or removes the start reaction without
successfully posting completion). This introduces inconsistent user-visible status signaling on
transient GitHub API failures.
Code

internal/statuscomment/statuscomment.go[R257-259]

+	n.postCompletionReaction(ctx, status, cleanupReaction, postReaction)
+
+	if !postComment {
Relevance

●●● Strong

Team often hardens statuscomment against unexpected API states; reordering avoids misleading
user-visible completion signaling.

PR-#1871

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The code invokes reaction cleanup/posting before any completion comment body is built or API calls
are made, but later returns an error if comment update/post fails; reaction operations are
logged-only and not rolled back.

internal/statuscomment/statuscomment.go[239-295]
internal/statuscomment/statuscomment.go[298-316]

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

### Issue description
`Notifier.PostCompletionWithDetail` performs reaction cleanup/posting (`postCompletionReaction`) before attempting the completion comment update/post. If the comment call fails (and `PostCompletionWithDetail` returns an error), the issue/PR can show a completion (or no) reaction while the status comment still shows "Started", which is inconsistent.

### Issue Context
- `PostCompletionWithDetail` returns errors on `UpdateIssueComment` / `CreateIssueComment` failures.
- Reaction lifecycle operations are intentionally fail-open (logged, not returned), so once the swap happens there is no rollback.

### Fix Focus Areas
- internal/statuscomment/statuscomment.go[239-293]

Suggested approach:
- If `postComment` is true, attempt the comment update/post first; only after it succeeds should you delete the start reaction / add the completion reaction.
- If `postComment` is false (completion comment suppressed), keep current best-effort reaction cleanup/posting behavior.
- Consider clearing `n.startReactionID` after a successful deletion to avoid accidental double-deletes if `PostCompletionWithDetail` is called twice.

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


Grey Divider

Context sources
✅ Compliance rules (platform): 54 rules

Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 730c0a7

Results up to commit 390f99e ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Reaction swap before comment ✓ Resolved 🐞 Bug ☼ Reliability
Description
Notifier.PostCompletionWithDetail updates/removes reactions before attempting to create/update the
completion comment, so a later comment API failure can leave reactions indicating completion while
the status comment remains in the "Started" state (or removes the start reaction without
successfully posting completion). This introduces inconsistent user-visible status signaling on
transient GitHub API failures.
Code

internal/statuscomment/statuscomment.go[R257-259]

+	n.postCompletionReaction(ctx, status, cleanupReaction, postReaction)
+
+	if !postComment {
Relevance

●●● Strong

Team often hardens statuscomment against unexpected API states; reordering avoids misleading
user-visible completion signaling.

PR-#1871

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The code invokes reaction cleanup/posting before any completion comment body is built or API calls
are made, but later returns an error if comment update/post fails; reaction operations are
logged-only and not rolled back.

internal/statuscomment/statuscomment.go[239-295]
internal/statuscomment/statuscomment.go[298-316]

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

### Issue description
`Notifier.PostCompletionWithDetail` performs reaction cleanup/posting (`postCompletionReaction`) before attempting the completion comment update/post. If the comment call fails (and `PostCompletionWithDetail` returns an error), the issue/PR can show a completion (or no) reaction while the status comment still shows "Started", which is inconsistent.

### Issue Context
- `PostCompletionWithDetail` returns errors on `UpdateIssueComment` / `CreateIssueComment` failures.
- Reaction lifecycle operations are intentionally fail-open (logged, not returned), so once the swap happens there is no rollback.

### Fix Focus Areas
- internal/statuscomment/statuscomment.go[239-293]

Suggested approach:
- If `postComment` is true, attempt the comment update/post first; only after it succeeds should you delete the start reaction / add the completion reaction.
- If `postComment` is false (completion comment suppressed), keep current best-effort reaction cleanup/posting behavior.
- Consider clearing `n.startReactionID` after a successful deletion to avoid accidental double-deletes if `PostCompletionWithDetail` is called twice.

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


Grey Divider

Qodo Logo

Comment thread internal/statuscomment/statuscomment.go Outdated
@ralphbean

Copy link
Copy Markdown
Member Author

It worked at appdumpster/test-repo#43

image

@ralphbean
ralphbean force-pushed the feat/3697-on-failure-comment-completion branch from efad69d to d6dcdc9 Compare August 5, 2026 21:44
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [consumer-completeness] .github/workflows/reusable-dispatch.yml:1399 — The prioritize stage is the only agent stage that does not pass status-comment-id to the action. All other 6 stages (triage, code, review, fix, retro, harness-run) include it. Since /fs-prioritize is a slash command triggered via issue_comment, its reaction will target the issue/PR instead of the triggering comment, inconsistent with every other slash-command-triggered stage.
    Remediation: Add status-comment-id: ${{ fromJSON(needs.route.outputs.event_payload).comment.id }} to the prioritize stage's with: block.

  • [stale-doc] docs/guides/dev/cli-internals.md:103 — The CLI command tree lists --run-url, --status-repo, --status-number, and --mint-url but omits the new --status-comment-id flag added in this PR.
    Remediation: Add --status-comment-id <int> to the command tree between --status-number and --mint-url.

  • [stale-doc] docs/guides/user/running-agents-locally.md:250 — The "Status notification flags" table omits --status-comment-id.
    Remediation: Add a row for the new flag.

  • [protected-path] .github/workflows/reusable-*.yml — 6 files under .github/ are modified. The PR links to issue Triage agent causes unnecessary notifications - should skip initial comment #3697 and explains the rationale (wiring status-comment-id through all reusable workflows). Human approval is always required for protected-path changes.

Low

  • [test-coverage] internal/scaffold/workflow_call_alignment_test.go:797TestReusableDispatchStatusCommentPassthrough validates run-url, status-repo, and status-number threading but does not check status-comment-id. This test would have caught the prioritize stage omission.
    Remediation: Add a status-comment-id: assertion alongside the existing checks.

  • [test-fidelity] internal/forge/fake.go:1523FakeClient.ListIssueReactions omits the reaction ID from returned Reaction structs (populates Content and User but not ID). The LiveClient populates ID.
    Remediation: Set ID: r.ID in the Reaction struct constructed in FakeClient.ListIssueReactions.

  • [edge-case] e2e/behaviour/features/dispatch/reaction-notifications.feature:30 — The e2e scenario asserts the issue has a "+1" reaction but does not assert the absence of "eyes" reaction. If start-reaction cleanup fails, the test still passes.
    Remediation: Add And the issue does not have a "eyes" reaction after the +1 assertion.

  • [stale-doc] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications merge semantics description enumerates only comment.start/comment.completion but not the new reaction.start/reaction.completion settings.
    Remediation: Update to include reaction sub-fields.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run

Review

Findings

Medium

  • [consumer-completeness] .github/workflows/reusable-dispatch.yml:1399 — The prioritize stage ("Run prioritize agent" step) is the only agent stage that does not pass status-comment-id to the action. All other 6 stages (triage, code, review, fix, retro, harness-run) include it. Since /fs-prioritize is a slash command, its reaction will target the issue/PR instead of the triggering comment, inconsistent with every other slash-command-triggered stage.
    Remediation: Add status-comment-id: ${{ fromJSON(needs.route.outputs.event_payload).comment.id }} to the prioritize stage's with: block.

  • [stale-doc] docs/guides/dev/cli-internals.md:103 — The CLI command tree lists --run-url, --status-repo, --status-number, and --mint-url but omits the new --status-comment-id flag added in this PR.
    Remediation: Add --status-comment-id <int> to the command tree between --status-number and --mint-url.

  • [stale-doc] docs/guides/user/running-agents-locally.md:250 — The "Status notification flags" table omits --status-comment-id.
    Remediation: Add a row for the new flag.

  • [protected-path] .github/workflows/reusable-*.yml — 6 files under .github/ are modified. The PR links to issue Triage agent causes unnecessary notifications - should skip initial comment #3697 and explains the rationale (wiring status-comment-id through all reusable workflows). Human approval is always required for protected-path changes.

Low

  • [stale-doc] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications merge semantics description enumerates only comment.start/comment.completion but not the new reaction.start/reaction.completion settings.
    Remediation: Update to include reaction sub-fields.

  • [missing-test] internal/statuscomment/statuscomment_test.go — No test covers the code path where a start reaction exists but refreshClient fails at completion time (cleanup needed but impossible). The fail-open behavior is correct but untested for this specific combination.
    Remediation: Add a test injecting a clientFactory failure at completion and verifying the start reaction survives, a warning is logged, and no panic occurs.

  • [edge-case] e2e/behaviour/features/dispatch/reaction-notifications.feature:30 — The e2e scenario asserts the issue has a "+1" reaction but does not assert the absence of "eyes" reaction. If start-reaction cleanup fails, the test still passes.
    Remediation: Add And the issue does not have a "eyes" reaction after the +1 assertion.

  • [orphaned-resource] internal/statuscomment/statuscomment.go:84startReactionID is in-memory only. A hard-killed process leaves a stray eyes reaction indefinitely. ReconcileOrphaned documents this limitation but cannot reconcile reactions (no recoverable identity).

  • [test-fidelity] internal/forge/fake.go:1523FakeClient.ListIssueReactions omits the reaction ID from returned Reaction structs (populates Content and User but not ID). The LiveClient populates ID.
    Remediation: Set ID: r.ID in the Reaction struct constructed in FakeClient.ListIssueReactions.

  • [test-coverage] internal/scaffold/workflow_call_alignment_test.go:797TestReusableDispatchStatusCommentPassthrough validates run-url, status-repo, and status-number threading but does not check status-comment-id. This test would have caught the prioritize stage omission.
    Remediation: Add a status-comment-id: assertion alongside the existing checks.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (2)

Review

Findings

Medium

  • [scope-alignment] .github/workflows/reusable-dispatch.yml — The prioritize stage job does not receive the new status-comment-id input, even though all six other agent steps (triage, code, review, fix, retro, harness-run) do. This creates incorrect reaction targeting for slash-command-triggered prioritize runs — reactions will target the issue instead of the triggering comment.
    Remediation: Add status-comment-id: ${{ fromJSON(needs.route.outputs.event_payload).comment.id }} to the prioritize agent step.

  • [stale-doc] docs/guides/dev/cli-internals.md:103 — The CLI command tree lists --run-url, --status-repo, --status-number, and --mint-url but omits the new --status-comment-id flag added in this PR.
    Remediation: Add --status-comment-id <int> to the command tree between --status-number and --mint-url.

  • [stale-doc] docs/guides/user/running-agents-locally.md:250 — The "Status notification flags" table omits --status-comment-id.
    Remediation: Add a row for the new flag.

  • [stale-doc] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications merge semantics description enumerates only comment.start/comment.completion but not the new reaction.start/reaction.completion settings.
    Remediation: Update to include reaction sub-fields.

  • [protected-path] .github/workflows/reusable-*.yml — 6 files under .github/ are modified. The PR links to issue Triage agent causes unnecessary notifications - should skip initial comment #3697 and explains the rationale (wiring status-comment-id through all reusable workflows). Human approval is always required for protected-path changes.

Low

  • [missing-test] internal/statuscomment/statuscomment_test.go — No test covers the code path where a start reaction exists but refreshClient fails at completion time (cleanup needed but impossible). The fail-open behavior is correct but untested for this specific combination.
    Remediation: Add a test injecting a clientFactory failure at completion and verifying the start reaction survives, a warning is logged, and no panic occurs.

  • [edge-case] internal/statuscomment/statuscomment.go:309 — When refreshClient fails and postComment is false, the function returns nil without attempting reaction cleanup. A start reaction is permanently orphaned on transient mint failure. This is consistent with the documented known limitation.

  • [edge-case] e2e/behaviour/features/dispatch/reaction-notifications.feature:30 — The e2e scenario asserts the issue has a "+1" reaction but does not assert the absence of "eyes" reaction. If start-reaction cleanup fails, the test still passes.
    Remediation: Add And the issue does not have a "eyes" reaction after the +1 assertion.

  • [orphaned-resource] internal/statuscomment/statuscomment.go:84startReactionID is in-memory only. A hard-killed process leaves a stray 👀 reaction indefinitely. ReconcileOrphaned documents this limitation but cannot reconcile reactions (no recoverable identity).

  • [test-coverage] internal/scaffold/workflow_call_alignment_test.goTestReusableDispatchStatusCommentPassthrough validates run-url, status-repo, and status-number threading but does not check status-comment-id. This test would have caught the prioritize stage omission.
    Remediation: Add a status-comment-id: assertion alongside the existing checks.

  • [naming-convention] internal/cli/run.go:145 — Field statusComment does not follow sibling naming (runURL, statusRepo, statusNum, mintURL). Consider statusCommentID to match the --status-comment-id flag.

  • [pattern-inconsistency] internal/forge/forge.go:603 — The new reaction methods sit within the existing issue operations section without a sub-section comment. Other method groups have // Category operations comments.

  • [breaking-api] pkg/behaviourtest/drivers/scm/driver.go:41ListIssueReactions added to the scm.Driver interface in pkg/. While technically importable externally, this is a test infrastructure package unlikely to have external implementors.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (3)

Review

Findings

High

  • [logic-error] pkg/behaviourtest/steps/reaction.go:58 — Commit 15 removes w.ReactionsEnabled = true from givenReactionsEnabled() without replacement, but CleanupScenario in cleanup.go still gates reaction cleanup on w.ReactionsEnabled. Since ReactionsEnabled is never set to true, DisableReactionNotifications is never called during scenario teardown, leaving sticky reaction config across scenarios on shared pool-repo slots.
    Remediation: Restore w.ReactionsEnabled = true after the CommitFile call, or remove the ReactionsEnabled field and revert cleanup to use reactionsEnabledInConfig.

Medium

  • [missing-test] internal/statuscomment/statuscomment.go:384 — The ErrNotFound branch in postCompletionReaction — where a 404 on start-reaction delete clears startReactionID and falls through to post the completion reaction — has no unit test. The generic error and ErrNotSupported paths both have dedicated tests.
    Remediation: Add a test injecting forge.ErrNotFound and asserting the ID is zeroed, no warning is logged, and the completion reaction is posted.

  • [dead-code] pkg/behaviourtest/steps/reaction.go:133reactionsEnabledInConfig and its five test cases are retained but never called from production code after commit 13 replaced the only call site with w.ReactionsEnabled. See also: [logic-error] finding — these two findings are coupled.
    Remediation: Delete the function and its tests, or re-use it from CleanupScenario if reverting the ReactionsEnabled flag approach.

  • [stale-doc] docs/guides/dev/cli-internals.md:103 — The CLI command tree lists --run-url, --status-repo, --status-number, and --mint-url but omits the new --status-comment-id flag added in this PR.
    Remediation: Add --status-comment-id to the command tree.

  • [stale-doc] docs/guides/user/running-agents-locally.md:250 — The "Status notification flags" table omits --status-comment-id.
    Remediation: Add a row for the new flag.

  • [stale-doc] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications merge semantics description enumerates only comment.start/comment.completion but not the new reaction.start/reaction.completion settings.
    Remediation: Update to include reaction sub-fields.

  • [protected-path] .github/workflows/reusable-*.yml — 6 files under .github/ are modified (reusable-code, reusable-dispatch, reusable-fix, reusable-retro, reusable-review, reusable-triage). The PR links to issue Triage agent causes unnecessary notifications - should skip initial comment #3697 and explains the rationale (wiring status-comment-id through all workflows). Human approval is always required for protected-path changes.

Low

  • [breaking-api] pkg/behaviourtest/drivers/scm/driver.go:41ListIssueReactions added to the scm.Driver interface in pkg/. While technically importable externally, this is a test infrastructure package unlikely to have external implementors.

  • [pattern-inconsistency] internal/forge/forge.go:600 — The new reaction methods sit within the existing issue operations section without a sub-section comment. Optional: add a // Issue/PR reactions sub-comment.

  • [naming-convention] internal/cli/run.go:132 — Field statusComment does not follow sibling naming (runURL, statusRepo, statusNum, mintURL). Consider statusCommentID to match the --status-comment-id flag.

  • [edge-case] internal/harnessdispatch/input/ghaevent.go:350 — When intField(comment, "id") returns 0, strconv.Itoa(0) produces "0" which passes the != "" check in buildEventPayload. Guard with a zero check to omit the sentinel.

  • [pattern-inconsistency] internal/forge/fake.go:280 — New DeletedReactions and DeletedCommentReactions fields lack inline comments (// reaction IDs) unlike sibling fields.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (4)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml, .github/workflows/reusable-triage.yml — Six reusable workflow files under .github/ are modified to pass the new status-comment-id input through the workflow call chain. The PR links to issue Triage agent causes unnecessary notifications - should skip initial comment #3697 and the description explains the rationale (plumbing the triggering comment ID for slash-command-scoped reactions). Human approval is required for protected-path changes regardless of context.

  • [incomplete-doc] docs/guides/dev/cli-internals.md:103 — The CLI command tree for fullsend run lists --run-url, --status-repo, --status-number, and --mint-url but does not include the new --status-comment-id flag added in this PR.
    Remediation: Add a --status-comment-id <int> entry between --status-number and --mint-url in the CLI command tree.

  • [incomplete-doc] docs/guides/user/running-agents-locally.md:254 — The "Status notification flags" table lists five flags but is missing the new --status-comment-id flag.
    Remediation: Add a row to the status notification flags table: --status-comment-id — ID of the triggering slash-command comment; when set, reactions target that comment instead of the issue/PR.

  • [breaking-api] pkg/behaviourtest/drivers/scm/driver.go:40 — The scm.Driver interface gains a new ListIssueReactions method. Because this package lives under pkg/ (not internal/), it is importable by external repositories. Any external implementation of scm.Driver will fail to compile after upgrading. The diff confirms this break — six internal fake SCM implementations all required stub additions.
    Remediation: Consider a separate optional interface (e.g., ReactionLister) with type-assertion at call sites for backward compatibility, or document the break in release notes.

Low

  • [dead-code] pkg/behaviourtest/steps/reaction.go:131reactionsEnabledInConfig is defined and tested but never called by production or framework code. The cleanup path in cleanup.go uses w.ReactionsEnabled (a boolean flag) instead.
    Remediation: Remove the function and its tests, or wire it into the cleanup path if intended as a defensive fallback.

  • [pattern-inconsistency] internal/forge/fake.go:596CommentReactionRecord omits the ID int64 field that ReactionRecord carries. Both AddIssueReaction and AddIssueCommentReaction return an ID, but only ReactionRecord records it. This asymmetry could cause test confusion when asserting on comment reaction IDs.
    Remediation: Add ID int64 to CommentReactionRecord for symmetry with ReactionRecord.

  • [incomplete-doc] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications merge-semantics note mentions comment.start/comment.completion but not the new reaction.start/reaction.completion fields added by this PR.
    Remediation: Update to mention reaction.start/reaction.completion alongside the comment fields.


Labels: PR modifies reusable dispatch workflows and adds a new feature (emoji reaction notifications)

Previous run (5)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml, .github/workflows/reusable-triage.yml — Six workflow files under the .github/ protected path are modified. The changes are mechanical (adding status-comment-id input passthrough), and the linked issue Triage agent causes unnecessary notifications - should skip initial comment #3697 authorizes the reaction feature that requires this wiring. Human approval is required for protected-path changes.

Low

  • [scope-constraint-transparency] PR body documents that live e2e behavior test for comment-scoped reactions is blocked on perRepoConfig has no status_notifications field — reactions/comments toggles are org-only #5994 (now closed). The PR has comprehensive unit test coverage (TestPostStart_ReactionTargetsTriggeringComment, TestPostCompletion_ReactionTargetsTriggeringComment) and a .feature file, but the live per-repo e2e suite does not yet exercise comment-scoped reactions.
    Remediation: Consider adding the live e2e test now that perRepoConfig has no status_notifications field — reactions/comments toggles are org-only #5994 is resolved, or file a follow-up issue.

  • [incomplete-documentation] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications field description mentions only nested comment.start/comment.completion settings, but the PR adds reaction.start and reaction.completion config keys. The merge semantics are still accurate; this is an illustrative-example gap.
    Remediation: Update to mention reaction.start/reaction.completion alongside comment.start/comment.completion.

  • [incomplete-documentation] docs/guides/user/running-agents-locally.md:253 — The "Status notification flags" table lists --run-url, --status-repo, --status-number, and --mint-url, but the PR adds --status-comment-id. This flag should be documented alongside the other status notification flags.
    Remediation: Add a row for the --status-comment-id flag.

  • [incomplete-documentation] docs/guides/dev/cli-internals.md:102 — The fullsend run flag tree lists --run-url, --status-repo, --status-number, and --mint-url but not --status-comment-id.
    Remediation: Add --status-comment-id to the flag tree.


Labels: PR adds new behaviour test infrastructure (reaction steps, feature file, cleanup logic) under pkg/behaviourtest/ and e2e/behaviour/

Previous run (6)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml, .github/workflows/reusable-triage.yml — Six workflow files under the .github/ protected path are modified. The changes are mechanical (adding status-comment-id input passthrough), and the linked issue Triage agent causes unnecessary notifications - should skip initial comment #3697 authorizes the reaction feature that requires this wiring. Human approval is required for protected-path changes.

Low

  • [edge-case] internal/statuscomment/statuscomment.go — On forges that don't support reactions (e.g. GitLab), postCompletionReaction calls deleteReaction (returns ErrNotSupported, falls through) then addReaction (also returns ErrNotSupported, silently ignored). Both calls are harmless no-ops but slightly wasteful. Covered by TestPostCompletion_ReactionErrNotSupported_Silent.

  • [incomplete-documentation] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications field description mentions only nested comment.start/comment.completion settings, but the PR adds reaction.start and reaction.completion config keys. The merge semantics are still accurate; this is an illustrative-example gap.
    Remediation: Update to mention reaction.start/reaction.completion alongside comment.start/comment.completion.

  • [incomplete-documentation] docs/guides/user/running-agents-locally.md:253 — The "Status notification flags" table lists --run-url, --status-repo, --status-number, and --mint-url, but the PR adds --status-comment-id. This flag should be documented alongside the other status notification flags.
    Remediation: Add a row for the --status-comment-id flag.

  • [incomplete-documentation] docs/guides/dev/cli-internals.md:102 — The fullsend run flag tree lists --run-url, --status-repo, --status-number, and --mint-url but not --status-comment-id.
    Remediation: Add --status-comment-id to the flag tree.

Previous run (7)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml, .github/workflows/reusable-triage.yml — Six workflow files under the .github/ protected path are modified. The changes are mechanical (adding status-comment-id input passthrough), and the linked issue Triage agent causes unnecessary notifications - should skip initial comment #3697 authorizes the reaction feature that requires this wiring. Human approval is required for protected-path changes.

  • [stale-documentation] docs/normative/normalized-event/v1/README.md:192 — The execution ref projection table states that event_payload.comment contains only {body: transition.comment.body}, but the PR adds comment.id to this structure in reusable-dispatch.yml and passes it through as status-comment-id. The normalized-event spec does not reflect the new id field.
    Remediation: Update the event_payload.comment projection in the normalized-event spec to include the id field.

Low

  • [incomplete-documentation] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications field description mentions only nested comment.start/comment.completion settings, but the PR adds reaction.start and reaction.completion config keys. The merge semantics are still accurate; this is an illustrative-example gap.
    Remediation: Update to mention reaction.start/reaction.completion alongside comment.start/comment.completion.

  • [incomplete-documentation] docs/guides/user/running-agents-locally.md:253 — The "Status notification flags" table lists --run-url, --status-repo, --status-number, and --mint-url, but the PR adds --status-comment-id. This flag should be documented alongside the other status notification flags.
    Remediation: Add a row for the --status-comment-id flag.

  • [incomplete-documentation] docs/guides/dev/cli-internals.md:102 — The fullsend run flag tree lists --run-url, --status-repo, --status-number, and --mint-url but not --status-comment-id.
    Remediation: Add --status-comment-id to the flag tree.

Previous run (8)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml, .github/workflows/reusable-triage.yml — Six workflow files under the .github/ protected path are modified. The changes are mechanical (adding status-comment-id input passthrough), and the linked issue Triage agent causes unnecessary notifications - should skip initial comment #3697 authorizes the reaction feature that requires this wiring. Human approval is required for protected-path changes.

  • [stale-documentation] docs/normative/normalized-event/v1/README.md:192 — The execution ref projection table states that event_payload.comment contains only {body: transition.comment.body}, but the PR adds comment.id to this structure in reusable-dispatch.yml and passes it through as status-comment-id. The normalized-event spec does not reflect the new id field.
    Remediation: Update the event_payload.comment projection in the normalized-event spec to include the id field.

Low

  • [test-fidelity] internal/forge/fake.go:1508FakeClient.ListIssueReactions omits the reaction ID from the returned Reaction struct. The real GitHub implementation populates Reaction.ID, but the fake always returns ID: 0. No current code path uses the ID from ListIssueReactions results, so this is not a live bug, but it could mask issues in future code that uses the list API to find and delete specific reactions.
    Remediation: Copy r.ID into the returned Reaction: Reaction{ID: r.ID, Content: r.Content, User: f.AuthenticatedUser}.

  • [incomplete-documentation] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications field description mentions only nested comment.start/comment.completion settings, but the PR adds reaction.start and reaction.completion config keys. The merge semantics are still accurate; this is an illustrative-example gap.
    Remediation: Update to mention reaction.start/reaction.completion alongside comment.start/comment.completion.

  • [incomplete-documentation] docs/guides/user/running-agents-locally.md:253 — The "Status notification flags" table lists --run-url, --status-repo, --status-number, and --mint-url, but the PR adds --status-comment-id. This flag should be documented alongside the other status notification flags.
    Remediation: Add a row for the --status-comment-id flag.

  • [incomplete-documentation] docs/guides/dev/cli-internals.md:102 — The fullsend run flag tree lists --run-url, --status-repo, --status-number, and --mint-url but not --status-comment-id.
    Remediation: Add --status-comment-id to the flag tree.

Previous run (9)

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml, .github/workflows/reusable-triage.yml — Six workflow files under the .github/ protected path are modified. The changes are mechanical (adding status-comment-id input passthrough), and the linked issue Triage agent causes unnecessary notifications - should skip initial comment #3697 authorizes the reaction feature that requires this wiring. Human approval is required for protected-path changes.

  • [stale-documentation] docs/normative/normalized-event/v1/README.md:192 — The execution ref projection table states that event_payload.comment contains only {body: transition.comment.body}, but the PR adds comment.id to this structure in reusable-dispatch.yml and passes it through as status-comment-id. The normalized-event spec does not reflect the new id field.
    Remediation: Update the event_payload.comment projection in the normalized-event spec to include the id field.

Low

  • [error-handling] internal/statuscomment/statuscomment.go:302 — In PostCompletionWithDetail, when refreshClient fails and only reaction/cleanup operations are pending (no comment), all pending work is skipped with a warning. This is consistent with the fail-open design for non-critical notification operations.

  • [edge-case] internal/statuscomment/statuscomment.go:196reactionForStatus treats all non-"success" statuses (including unrecognized or empty values) as "confused". This is a reasonable default for a supplementary signal.

  • [incomplete-documentation] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications field description mentions only nested comment.start/comment.completion settings, but the PR adds reaction.start and reaction.completion config keys. The merge semantics are still accurate; this is an illustrative-example gap.
    Remediation: Update to mention reaction.start/reaction.completion alongside comment.start/comment.completion.

  • [incomplete-documentation] docs/guides/user/running-agents-locally.md:253 — The "Status notification flags" table lists --run-url, --status-repo, --status-number, and --mint-url, but the PR adds --status-comment-id. This flag should be documented alongside the other status notification flags.
    Remediation: Add a row for the --status-comment-id flag.

  • [incomplete-documentation] docs/guides/dev/cli-internals.md:102 — The fullsend run flag tree lists --run-url, --status-repo, --status-number, and --mint-url but not --status-comment-id.
    Remediation: Add --status-comment-id to the flag tree.

Previous run (10)

Review

Findings

Medium

  • [stale-documentation] docs/normative/normalized-event/v1/README.md:192 — The execution ref projection table states that event_payload.comment contains only {body: transition.comment.body}, but the PR adds comment.id to this structure in reusable-dispatch.yml and passes it through as status-comment-id. The normalized-event spec does not reflect the new id field.
    Remediation: Update the event_payload.comment projection in the normalized-event spec to include the id field.

  • [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml, .github/workflows/reusable-triage.yml — Six workflow files under the .github/ protected path are modified. The changes are mechanical (adding status-comment-id input passthrough), and the linked issue Triage agent causes unnecessary notifications - should skip initial comment #3697 authorizes the reaction feature that requires this wiring. Human approval is required for protected-path changes.

Low

  • [error-handling] internal/statuscomment/statuscomment.go:302 — In PostCompletionWithDetail, when refreshClient fails and only reaction/cleanup operations are pending (no comment), all pending work is skipped with a warning. This is consistent with the fail-open design for non-critical notification operations.

  • [edge-case] internal/statuscomment/statuscomment.go:196reactionForStatus treats all non-"success" statuses (including unrecognized or empty values) as "confused". This is a reasonable default for a supplementary signal.

  • [incomplete-documentation] docs/guides/infrastructure/layered-config-reference.md:295 — The status_notifications field description mentions only nested comment.start/comment.completion settings, but the PR adds reaction.start and reaction.completion config keys. The merge semantics are still accurate; this is an illustrative-example gap.
    Remediation: Update to mention reaction.start/reaction.completion alongside comment.start/comment.completion.

Previous run (11)

Review

Findings

Medium

  • [stale-documentation] docs/normative/normalized-event/v1/README.md:192 — The execution ref projection table states that event_payload.comment contains only {body: transition.comment.body}, but the PR adds comment.id to this structure in reusable-dispatch.yml and passes it through as status-comment-id. The normalized-event spec does not reflect the new id field.
    Remediation: Update the event_payload.comment projection in the normalized-event spec to include the id field.

  • [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml, .github/workflows/reusable-triage.yml — Six workflow files under the .github/ protected path are modified. The changes are mechanical (adding status-comment-id input passthrough), and the linked issue Triage agent causes unnecessary notifications - should skip initial comment #3697 authorizes the reaction feature that requires this wiring. Human approval is required for protected-path changes.

Low

  • [error-handling] internal/statuscomment/statuscomment.go:302 — In PostCompletionWithDetail, when refreshClient fails and only reaction/cleanup operations are pending (no comment), all pending work is skipped with a warning. This is consistent with the fail-open design for non-critical notification operations.

  • [edge-case] internal/statuscomment/statuscomment.go:705ReconcileOrphaned synthesizes an "Interrupted" comment with empty startTimeStr when no matching marker was found, producing a comment with no "Started" timestamp. This is pre-existing behavior unchanged by this PR.

  • [edge-case] internal/statuscomment/statuscomment.go:196reactionForStatus treats all non-"success" statuses (including unrecognized or empty values) as "confused". This is a reasonable default for a supplementary signal.

  • [scope-documentation] The PR defers ReconcileOrphaned reaction cleanup as a known limitation, documented in code comments and user-facing docs, but no follow-up issue tracks the gap. An orphaned 👀 reaction will persist after SIGKILL/OOM.
    Remediation: File a follow-up issue to track orphaned reaction cleanup in ReconcileOrphaned.

Previous run (12)

Review

Findings

Medium

  • [API-contract-mismatch] internal/statuscomment/statuscomment.go — The PR adds completionMode, jobStatus string to ReconcileOrphaned, but main (after merging feat(#3697): add on_failure mode for comment.completion status notifications #5736) already has (... completionMode, jobStatus string, wasSkipped bool, agentDescription string). The PR branch is stale relative to main and will produce merge conflicts. The test stubs in reconcilestatus_test.go and the call site in reconcilestatus.go also use the shorter signature.
    Remediation: Rebase onto current main and reconcile the ReconcileOrphaned signature with the wasSkipped and agentDescription parameters already present.

  • [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml, .github/workflows/reusable-triage.yml — Six workflow files under the .github/ protected path are modified. The changes are mechanical (adding status-comment-id input passthrough), and the linked issue Triage agent causes unnecessary notifications - should skip initial comment #3697 authorizes the reaction feature that requires this wiring. Human approval is required for protected-path changes.

Low

  • [error-handling] internal/statuscomment/statuscomment.go — In PostCompletionWithDetail, when refreshClient fails and only reaction operations (not comments) are pending, all reaction work is silently skipped. The log message ("failed to mint token for completion") does not distinguish whether comment or reaction operations were intended. This is by design (fail-open for reactions) and low impact.

  • [edge-case] internal/statuscomment/statuscomment.go — The ReconcileOrphaned on_failure synthesis passes empty strings for description and startTimeStr, producing a comment with generic heading "Agent run interrupted" and no "Started" timestamp. This is the best available information when no start comment was posted, but provides less context than a normal interrupted comment.

  • [scope-documentation] The PR defers ReconcileOrphaned reaction cleanup to future work but no follow-up issue is filed to track the gap. An orphaned 👀 reaction will persist after SIGKILL/OOM.
    Remediation: File a follow-up issue to track orphaned reaction cleanup in ReconcileOrphaned.

  • [naming-consistency] internal/config/config.goReactionNotificationConfig type-level doc comment includes behavioral notes about defaults ("both fields default to disabled") that could be placed on field-level comments for consistency with CommentNotificationConfig's documentation style.

  • [parameter-naming] internal/statuscomment/statuscomment.goReconcileOrphaned now has 11 parameters in the PR. After rebasing onto main (which adds 2 more), it will have 13. The codebase convention does not use option structs here, but the growing parameter list affects readability.

Previous run (13)

Review

Findings

Medium

  • [incomplete-documentation] docs/guides/getting-started/operations.md:176 — The "Status notifications" section documents status_notifications.comment but omits the newly added status_notifications.reaction field. Readers consulting this guide will get an incomplete picture of the notification config surface.
    Remediation: Add documentation for the reaction sub-field including schema, default values (disabled), emoji mappings (👀 start, 👍/👎 completion), and the GitHub-only note.

Low

  • [scope-documentation] The PR defers ReconcileOrphaned reaction cleanup to future work but no follow-up issue is filed to track the gap. An orphaned 👀 reaction will persist after SIGKILL/OOM.
    Remediation: File a follow-up issue to track orphaned reaction cleanup in ReconcileOrphaned.

  • [error-handling] internal/statuscomment/statuscomment.go:253 — In PostCompletionWithDetail, when refreshClient fails and no comment is needed but reaction/comment cleanup is pending, both are silently skipped. The log message ("failed to mint token for completion") doesn't distinguish which operation was intended. This is by design (fail-open for reactions) and low impact.

  • [incomplete-documentation] docs/guides/infrastructure/layered-config-reference.md — The per-field merge rules table does not document status_notifications (pre-existing gap, not introduced by this PR, but now more noticeable with the expanded config surface).
    Remediation: Add status_notifications to the merge rules table.


Labels: PR modifies agent runner notification lifecycle (status reactions in statuscomment package) and includes user-facing documentation updates.

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 component/runner Agent runner behavior and lifecycle component/docs User-facing documentation labels Aug 5, 2026

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HIGH: PR head has diverged from its declared base — diff includes stale/unrelated content and reports a real merge conflict

Re-verified live: this PR's mergeable/mergeStateStatus currently report CONFLICTING/DIRTY, and a compare of feat/3697-on-failure-comment-completion...feat/3697-emoji-reaction-notifications reports status: diverged, ahead_by: 16, behind_by: 17. The base branch (#5736) has moved 17 commits ahead of where this branch was cut, while this branch still carries its own older copy of the same on_failure/reconcile-status machinery. The rendered diff for this PR therefore mixes stale duplicate content (internal/cli/reconcilestatus.go, action.yml, docs/guides/dev/cli-internals.md) in with the actual emoji-reaction work, even though the PR summary frames the emoji-reaction work as the only change. This is a real, currently-active state and will guarantee conflicts or silently reintroduce stale content when merged as-is.

Suggestion: Rebase feat/3697-emoji-reaction-notifications onto the current tip of feat/3697-on-failure-comment-completion (or onto main once #5736 merges), then re-diff to confirm only the reaction-related changes remain before requesting re-review.

Comment thread internal/forge/forge.go
Comment thread internal/statuscomment/statuscomment.go
Comment thread internal/statuscomment/statuscomment.go
Comment thread internal/config/config.go
Comment thread internal/statuscomment/statuscomment.go

@ascerra ascerra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've discussed using thumbs up and thumbs down as a way for users to tell us if they liked or disliked the agent response so we can gather metrics on user approval ratings and things like that.

I see the thumbs up is added as the bot so this might not be a problem... our metric collector one day will just need to check for human added emojis

@ralphbean

Copy link
Copy Markdown
Member Author

Marking this blocked on #5994. Turns out perRepoConfig has no status_notifications field at all, and pkg/behaviourtest only supports per-repo installs — so there's no way to turn reactions on in the live e2e suite today. That leaves the comment-targeting behavior test with unit coverage only for now (TestPostStart_ReactionTargetsTriggeringComment etc.). Once #5994 lands I'll come back and add the live scenario.

@ralphbean

Copy link
Copy Markdown
Member Author

Converting this to draft until #5994 is resolved.

@ralphbean

Copy link
Copy Markdown
Member Author

Re: #5957 (comment)

  • Docs gap (medium): fixed in 8f1c454.
  • No follow-up issue for orphaned-reaction cleanup: went with documenting it inline instead (next to startReactionID and in ReconcileOrphaned's doc comment, ad6b587) rather than a tracking issue — same call as the identical suggestion on the reaction-identity review thread.
  • Ambiguous log message on refreshClient failure: leaving as is, agreed this is low impact.
  • layered-config-reference.md merge-rules gap: that's pre-existing and unrelated to this PR, not fixing it here.

@ralphbean
ralphbean force-pushed the feat/3697-on-failure-comment-completion branch from f42097a to b4fec87 Compare August 10, 2026 19:28
Base automatically changed from feat/3697-on-failure-comment-completion to main August 11, 2026 15:31
@ralphbean
ralphbean marked this pull request as ready for review August 11, 2026 16:05
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 24, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:56 PM UTC · Completed 7:16 PM UTC

Commit: cc41a71 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.54

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 24, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:29 PM UTC · Completed 7:49 PM UTC

Commit: 849118c · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.06

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

@ralphbean

Copy link
Copy Markdown
Member Author

Unable to get past functional-tests without #6562

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 24, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 7:56 PM UTC · Ended 8:10 PM UTC

Commit: 730c0a7 · View workflow run →

@ralphbean

Copy link
Copy Markdown
Member Author

@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 24, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 8:04 PM UTC · Completed 8:13 PM UTC

Commit: 730c0a7 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.98

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:

  • .github/workflows/reusable-dispatch.yml (file-level): Line 1399 · [medium] consumer-completeness

The prioritize stage is the only agent stage that does not pass status-comment-id to the action. All other 6 stages (triage, code, review, fix, retro, harness-run) include it. Since /fs-prioritize is a slash command triggered via issue_comment, its reaction will target the issue/PR instead of the triggering comment, inconsistent with every other slash-command-triggered stage.

Suggested fix: Add status-comment-id: ${{ fromJSON(needs.route.outputs.event_payload).comment.id }} to the prioritize stage's with: block.

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread internal/forge/fake.go
Comment thread e2e/behaviour/features/dispatch/reaction-notifications.feature
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:56 PM UTC · Completed 8:10 PM UTC

Commit: 730c0a7 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.69

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: behaviour test failure on PR #5957

Root cause

The behaviour test failure was caused by GCP WIF pool quota exhaustion (HTTP 429), not a code regression. All 11 failed scenarios hit the same error during test setup:

provisioning WIF for inference: creating WIF pool: unexpected status 429 creating WIF pool: Resource has been exhausted (e.g. check quota).

The same 22 scenarios all passed on the previous commit (849118c, run 32768303356) just ~27 minutes earlier. The only diff between the two commits (849118c730c0a7) was four eval annotation YAML files — zero production or test code changes. The 11 scenarios that passed on 730c0a7 are ones that don't require WIF provisioning (Jira, branch namespace, pi runtime tests).

Code-level gap

CreateWIFPool in internal/dispatch/gcf/gcp.go (line 272) uses a bare DoRequest call with no retry logic. Meanwhile, all four WIF provider functions (CreateWIFProvider, UpdateWIFProvider, undeleteWIFProvider, enableWIFProvider) are wrapped by doWIFRequestWithRetry, which retries up to 7 times on HTTP 429 with exponential backoff. This asymmetry means pool creation fails immediately on quota pressure while provider operations would survive.

Existing issues already cover this

No new proposals are needed — existing open issues substantively address the gaps:

No workflow or agent issues

The agent workflow on this PR functioned correctly: the review agent ran 12+ iterations, the fix agent completed 5 iterations addressing review findings, and the human reviewer (waynesun09) approved after thorough multi-pass review. The /fs-retro investigation correctly identified an infrastructure failure, not an agent or code quality issue.

@ralphbean
ralphbean added this pull request to the merge queue Aug 24, 2026
Merged via the queue into main with commit de9ed2e Aug 24, 2026
42 of 45 checks passed
@ralphbean
ralphbean deleted the feat/3697-emoji-reaction-notifications branch August 24, 2026 20:34
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 24, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 8:36 PM UTC · Completed 8:47 PM UTC

Commit: 730c0a7 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.51

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #5957 — Emoji reaction status notifications

PR: #5957 by ralphbean, implementing issue #3697
Timeline: Aug 5–24 (19 days), 34 files changed, +1233/−70 lines
Review iterations: 14 agent runs, 7 human review passes (waynesun09), 5+ fix agent runs

What went well

  • The review agent correctly identified one HIGH-severity logic error (run 11): the ReactionsEnabled flag cleanup leak across shared pool-repo e2e slots. This was a real bug that was fixed.
  • The agent's protected-path flagging was consistently useful across all 14 runs, correctly requiring human approval for 6 workflow files.
  • The agent's documentation gap detection (stale CLI docs, missing config reference fields) identified real completeness issues.
  • The fix agent was effective at addressing clearly-scoped review findings, completing 4 successful fix iterations that addressed reaction lifecycle ordering, pagination, interface signatures, and comment-ID wiring.
  • Review cost decreased across iterations ($9.54 → $7.06 → $3.69), suggesting effective prior-context reuse.

Review quality gap

The human reviewer (waynesun09) substantially outperformed the agent on correctness bugs for this complex, cross-cutting feature PR. The human caught:

  • Reactions always targeting the issue/PR, silently dropping comment-targeting (the core feature requirement)
  • Concurrent same-role runs sharing reaction identity due to GitHub's (actor, subject, content) keying
  • Cross-branch type contract conflict (Comment.ID int vs string from a merged ADR)
  • Missing comment.id in the matrix event_payload builder (contradicting the commit message)
  • Interface signatures missing issue number, making GitLab implementation impossible
  • Multiple error-handling edge cases in the reaction lifecycle

The agent's 14 review runs never flagged any of these. This suggests the correctness sub-agent lacks guidance for two specific patterns: (1) verifying external API resource identity under concurrent use, and (2) tracing new input propagation across reusable workflow call chains.

Evidence for existing open issues

  • #158 (pre-fix script should skip fix agent for workflow-only findings): Two fix iterations on this PR failed because the agent attempted to modify .github/workflows/ files, which require workflows write permission the app lacks. This wasted ~$7–20 in agent cost and added hours to resolution.
  • #447 (review agent should incorporate outstanding human reviews): waynesun09 posted detailed correctness findings across 7 review passes, but the agent reviewed independently each time without referencing them. If the agent had seen the human's findings, it could have verified fixes and added value by catching regressions.
  • #6462 (triage eval case crashes non-deterministically): The merge queue functional test failure on this PR showed the same pattern — case 001-bug-url-encoding crashed with exit 1 in 6 seconds at $0.00 cost. The identical signature (single-case startup crash, other 3 cases pass) also appeared on PR fix(#6475): raise triage eval max_turns from 30 to 35 #6476 with case 002, confirming this is a systemic eval harness startup reliability issue, not case-specific.

Autonomy readiness

This PR demonstrates the agent is not ready for autonomous approval on large, cross-cutting feature PRs. The agent's review contributions were primarily documentation-oriented; the human reviewer caught all high-severity correctness bugs. However, the agent is effective at protected-path flagging and documentation completeness checks, which could support increased autonomy for narrow, well-scoped changes (e.g., doc-only PRs, config-only changes) where those are the primary review concerns.

Proposals filed

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

Labels

blocked Blocked by another issue or external dependency component/dispatch Workflow dispatch and triggers component/docs User-facing documentation component/e2e End-to-end tests component/runner Agent runner behavior and lifecycle fullsend-fix Enables automatic bot-triggered fix runs on human-authored PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants