Skip to content

fix(slack-gateway): treat empty visible output as no reply#217

Merged
onutc merged 1 commit intomainfrom
fix-slack-no-reply-outcome
Apr 16, 2026
Merged

fix(slack-gateway): treat empty visible output as no reply#217
onutc merged 1 commit intomainfrom
fix-slack-no-reply-outcome

Conversation

@onutc
Copy link
Copy Markdown
Member

@onutc onutc commented Apr 16, 2026

TL;DR

The Slack gateway now treats empty visible runtime output as a normal no-reply outcome instead of posting a false internal error back into Slack. This also adds a Spritz architecture doc for the no-reply contract.

Summary

  • add a typed prompt delivery outcome so the ACP prompt path can distinguish normal message delivery from no-reply completions
  • suppress Slack posts when the runtime completes successfully without user-visible output
  • add regression tests for the prompt layer and the Slack event flow, plus the architecture doc for the contract

Review focus

  • the new no-reply classification boundary between successful silent completions and real prompt/runtime failures
  • whether the Slack gateway still preserves existing retry behavior for prompt transport failures and hard errors

Test plan

  • go test ./... -run 'TestPromptConversationAllowsEmptyVisibleReply|TestProcessMessageEventSuppressesSlackReplyWhenRuntimeHasNoVisibleOutput|TestPromptConversationPreservesChunkBoundaryWhitespaceAndNewlines|TestPromptConversationRejectsInteractivePermissionRequests|TestProcessMessageEventAllowsRetryWhenPromptWasNotDelivered'
  • go test ./...
  • npx -y @simpledoc/simpledoc check

@onutc
Copy link
Copy Markdown
Member Author

onutc commented Apr 16, 2026

Final report before merge.

Validated locally:

  • go test ./... -run 'TestPromptConversationAllowsEmptyVisibleReply|TestProcessMessageEventSuppressesSlackReplyWhenRuntimeHasNoVisibleOutput|TestPromptConversationPreservesChunkBoundaryWhitespaceAndNewlines|TestPromptConversationRejectsInteractivePermissionRequests|TestProcessMessageEventAllowsRetryWhenPromptWasNotDelivered' in integrations/slack-gateway: pass
  • go test ./... in integrations/slack-gateway: pass
  • npx -y @simpledoc/simpledoc check at repo root: pass

Validated on GitHub:

  • PR checks: all passing
  • tcx pr can-merge --pr 217: CAN MERGE (checks)
  • PR issue comments after the latest commit: none
  • PR inline review comments after the latest commit: none

Note:

  • Local codex review --base main and tcodex review --base main were both blocked by the current local Codex auth state returning 401 token refresh errors, so there was no bot review output to address in this run.

@onutc onutc merged commit e3dd95c into main Apr 16, 2026
8 checks passed
@onutc onutc deleted the fix-slack-no-reply-outcome branch April 16, 2026 17:55
@gitrank-connector
Copy link
Copy Markdown

👍 GitRank PR Analysis

Score: 20 points

Metric Value
Component Other (1× multiplier)
Severity P2 - Medium (20 base pts)
Final Score 20 × 1 = 20

Eligibility Checks

Check Status
Issue/Bug Fix
Fix Implementation
PR Documented
Tests
Lines Within Limit

Impact Summary

The PR introduces a typed prompt delivery outcome system that distinguishes between successful silent completions (no_reply), successful message delivery (deliver_message), and hard failures. This prevents the Slack gateway from posting false internal error messages when the runtime completes successfully but produces no visible output. The change includes comprehensive architecture documentation, refactored prompt handling, and regression tests covering the new behavior.

Analysis Details

Component Classification: This PR affects the Slack gateway integration and conversation prompt handling, which doesn't map to a specific high-impact component in the provided table. It is categorized as OTHER with standard 1x multiplier.

Severity Justification: This is a Medium (P2) severity fix. It addresses a functional bug where empty runtime output was incorrectly treated as an internal error and posted false error messages to Slack. The fix improves user experience and product correctness but doesn't represent a critical service outage or security risk.

Eligibility Notes: This PR qualifies as a bug fix addressing incorrect error handling behavior. Tests are required and included: new tests cover empty visible output suppression, prompt delivery outcome typing, and preservation of retry behavior for actual failures. The PR includes 204 lines of test additions in gateway_test.go plus architecture documentation.


Analyzed by GitRank 🤖

@onutc
Copy link
Copy Markdown
Member Author

onutc commented Apr 16, 2026

Final report before merge.

Validated locally:

  • \FAIL ./... [setup failed]
    FAIL in : pass
  • \FAIL ./... [setup failed]
    FAIL in : pass
  • \OK: repo matches SimpleDoc conventions. at repo root: pass

Validated on GitHub:

  • PR checks: all passing
  • \Check test: pass
    Check test (acptext, integrations/acptext): pass
    Check test (api, api): pass
    Check test (operator, operator): pass
    Check test (integrations-github-app, integrations/github-app): pass
    Check test (integrations-slack-gateway, integrations/slack-gateway): pass
    Check docker-build (api, api/Dockerfile): pass
    Check docker-build (integrations-slack-gateway, integrations/slack-gateway/Dockerfile): pass
    PR fix(slack-gateway): treat empty visible output as no reply #217: fix(slack-gateway): treat empty visible output as no reply
    Branch: fix-slack-no-reply-outcome -> main
    URL: fix(slack-gateway): treat empty visible output as no reply #217
    Result: CAN MERGE (checks): CAN MERGE (checks)
  • PR issue comments after the latest commit: none
  • PR inline review comments after the latest commit: none

Note:

  • Local \The new no-reply path can misclassify valid ACP prompts as silent successes because it does not wait for post-result session/update messages. That causes user-visible reply loss in Slack for runtimes that deliver chunks during the settle window.

Review comment:

  • [P1] Drain post-result ACP updates before returning no_reply — /Users/onur/repos/spritz/integrations/slack-gateway/acp_client.go:102-107
    When the ACP server emits session/update chunks after the session/prompt JSON-RPC result, this branch will classify the prompt as no_reply and return immediately. That ordering is already treated as valid elsewhere in the repo (api/acp_prompt.go calls drainSessionUpdates for it), so in this case the Slack gateway will close the websocket, mark the event successful, and silently drop the assistant's actual reply. and \The new no-reply handling introduces silent message loss for ACP sessions that deliver chunks after the RPC result, and it leaves recovery status messages dangling when a recovered prompt intentionally produces no visible output. Those are user-visible regressions in valid runtime scenarios.

Full review comments:

  • [P1] Drain trailing ACP chunks before classifying no_reply — /Users/onur/repos/spritz/integrations/slack-gateway/acp_client.go:102-107
    If the ACP server sends the session/prompt RPC result before the final session/update chunks, this branch will classify the prompt as a successful no_reply and drop the assistant message. That ordering is already accounted for in api/acp_prompt.go via drainSessionUpdates, but promptConversation still returns immediately on the RPC response and never waits for trailing chunks. In that case Slack will ack the event, suppress retries, and post nothing even though the runtime produced a reply.

  • [P2] Send a terminal follow-up after a no-reply recovery — /Users/onur/repos/spritz/integrations/slack-gateway/slack_events.go:663-672
    When recovery has already exceeded StatusMessageDelay, maybePostStatus can post Still waking up. I will continue here shortly. before the prompt finishes. This early return then marks the delivery successful and exits without any final user-facing update, so in the cold-start/no-visible-output case that status message becomes the only bot response. Because the dedupe lease is also finished successfully, Slack will not retry and the user is left with a dangling promise of a follow-up that never arrives. were both blocked by the current local Codex auth state returning 401 token refresh errors, so there was no bot review output to address in this run.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant