fix(agentex): tell a user when SGP won't let their account hold a Slack config - #422
Merged
Merged
Conversation
michael-chou359
force-pushed
the
mc/slack-config-forbidden-notice
branch
from
September 2, 2026 23:28
c01c714 to
e52b0c0
Compare
…ck config
SGP can refuse an account two ways, and both leave the person with no config of
their own: unable to CREATE one (POST /v5/agent_configs -> 403 "action=create,
legacy_roles=['admin','manager','editor']"), or unable to LIST the directory at all.
The consequence is identical -- every turn silently falls back to the shared bot --
so both are now treated the same.
Before this, such a user kept getting bot-run turns forever with nothing said, having
just completed the link flow, which told them their turns would run as them. They had
no way to find out why their own integrations never work.
Note this is NOT the same permission as running the agent, and lacking it does not
imply lacking access. Agentex authorization (agent.execute) is checked separately and
earlier: the users whose turns hung had tasks created and dispatched, so they could
run golden-agent fine and only the config read failed. A viewer-role account is
exactly the case with execute but not create.
A 403 is an authorization decision, so retrying changes nothing. _list_configs and the
create path both raise _SgpAccessForbidden for it, and _run_turn posts a one-per-day
ephemeral naming the role to ask for before degrading to the bot as before.
Everything else still degrades quietly, because the remedies differ:
- 401 means the credential reached SGP and was rejected, which a re-link fixes, not
a role grant -- and the re-link prompt is driven elsewhere, so this only needs to
be diagnosable rather than messaged twice. Logged apart from the 4xx/5xx bucket.
- other 4xx/5xx and transport errors may work on the next turn and are not worth
telling anyone about.
The raise is contained at the two call sites where propagating would be wrong:
- selector resolution is best-effort and its caller falls back to the default
config, so raising there would error a turn that can still be answered;
- the post-create confirmation already has a working config, so a refused re-read
keeps what was made rather than discarding it.
Both broad `except Exception` handlers re-raise the signal explicitly, since either
would otherwise swallow it and restore the silent behaviour.
The cooldown is only kept if the notice was actually DELIVERED. Claiming it first is
deliberate -- it stops two concurrent turns both posting -- but committing it before
attempting delivery meant a rejected ephemeral suppressed the message for the whole
window while the user was told nothing, which is the failure this is supposed to
prevent.
Slack refusing an ephemeral is not rare: it rejects them outside a channel context,
i.e. an assistant pane (a DM with this app), and that rejection is PERMANENT for the
conversation, so simply retrying would deliver nothing forever. So the notice falls
back to an in-thread message -- in a pane the audience is identical, and in a channel a
visible message beats silence for something the person has to act on. Only if that
fails too is the cooldown released, letting the next turn try again.
_post_ephemeral now returns whether it landed. Existing callers ignore it, so the
change is additive.
Once a day rather than once ever because role grants change, and a stale "ask an
admin" is better repeated occasionally than never retracted. Same SET NX cooldown
shape as the link offer, failing open for the same reason: saying it twice beats never
saying it.
Adds 9 tests: 403 on create raises, 403 on list raises (and does not then attempt a
create), 503 stays quiet, 401 stays quiet, the selector path survives a forbidden
directory, and the notice is suppressed on a second call. Verified non-vacuous by
removing each guard in turn -- the raise tests fail with DID NOT RAISE, and the
selector test fails by propagating _SgpAccessForbidden out of a turn that could have
been answered.
154 passed. ruff check + format clean.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
michael-chou359
force-pushed
the
mc/slack-config-forbidden-notice
branch
from
September 2, 2026 23:33
e52b0c0 to
c5c762f
Compare
Comment on lines
+2078
to
+2079
| await self._deliver(inbound, _SGP_FORBIDDEN_MESSAGE) | ||
| return |
Contributor
There was a problem hiding this comment.
Silent fallback retains cooldown
When the ephemeral notice is rejected and _deliver has no bot token or Slack returns ok: false, _deliver returns normally without posting anything. This unconditional return then skips _release_notice_cooldown, so the user receives no explanation and further attempts remain suppressed for 24 hours.
Prompt To Fix With AI
This is a comment left during a code review.
Path: agentex/src/domain/use_cases/slack_gateway_use_case.py
Line: 2078-2079
Comment:
**Silent fallback retains cooldown**
When the ephemeral notice is rejected and `_deliver` has no bot token or Slack returns `ok: false`, `_deliver` returns normally without posting anything. This unconditional return then skips `_release_notice_cooldown`, so the user receives no explanation and further attempts remain suppressed for 24 hours.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap
SGP can refuse an account two ways, and both leave the person with no config of their own:
Either way every turn silently falls back to the shared bot — so both are now treated the same. Previously only the create half was recognised as permanent; a read refusal looked like a transient blip forever.
Such a user kept getting bot-run turns with nothing said, having just completed the link flow that told them their turns would run as them, and no way to learn why their own integrations never work.
This is not the same permission as running the agent
Worth stating, because it's a natural assumption that lacking config access means lacking access altogether. It doesn't:
The users whose turns hung had tasks created and dispatched (
status_reason = "Task created, forwarding to ACP server", named with their ownsgp_user_id)._authorizeruns before dispatch and posts "You're not authorized to run …" on failure — so authorization passed and only the config read failed. A viewer-role account is exactly the case with execute but not create.What changed
A 403 is an authorization decision, so retrying changes nothing. Both paths raise
_SgpAccessForbidden, and_run_turnposts a one-per-day ephemeral naming the role to ask for, then degrades to the bot as before.Everything else still degrades quietly, because the remedies differ:
403read or create401The raise is contained where propagating would be wrong
Both broad
except Exceptionhandlers re-raise the signal explicitly — either would otherwise swallow it and restore the silent behaviour.The cooldown only counts if the notice was delivered
Claiming the cooldown first is deliberate — it stops two concurrent turns both posting. But committing it before attempting delivery meant a rejected ephemeral suppressed the message for the whole 24h window while the user was told nothing, which is the exact failure this PR exists to prevent.
Slack refusing an ephemeral isn't rare: it rejects them outside a channel context — an assistant pane, i.e. a DM with this app — and that refusal is permanent for the conversation, so retrying would deliver nothing forever.
_post_ephemeralnow returns whether it landed. Existing callers ignore it, so the change is additive.Testing
9 new tests: 403 on create raises, 403 on list raises and does not then attempt a create, 503 stays quiet, 401 stays quiet, the selector path survives a forbidden directory, and the notice is suppressed on a second call.
Verified non-vacuous by removing each guard in turn — the raise tests fail with
DID NOT RAISE, and the selector test fails by propagating_SgpAccessForbiddenout of a turn that could have been answered.Note
Whether the linked users actually lack this permission is still unverified — the 403 I measured came from the shared bot's credential, not theirs. This change means that if they do, they'll be told rather than left guessing. Confirming directly needs one of them to try creating a config in SGP.
🤖 Generated with Claude Code
Greptile Summary
The PR distinguishes permanent SGP configuration-access failures from transient failures and adds a rate-limited Slack notice before falling back to the shared bot.
Confidence Score: 4/5
The PR is not yet safe to merge because the notice cooldown can still be retained when both delivery attempts fail.
The fallback assumes a normal return from
_delivermeans delivery succeeded, although_deliverreturns normally when credentials are absent or Slack rejects the post, leaving the user uninformed and suppressing retries for 24 hours.Files Needing Attention: agentex/src/domain/use_cases/slack_gateway_use_case.py
Important Files Changed
_deliveras either successful or raising and misses its normal-return failure modes.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Linked user's Slack turn] --> B[Read or create SGP config] B -->|Success| C[Run with user config] B -->|403| D[Claim Redis cooldown] D -->|Already claimed| E[Use shared bot] D -->|Claimed| F[Post ephemeral notice] F -->|Delivered| E F -->|Rejected| G[Post notice in thread] G -->|Raises| H[Release cooldown] G -->|Returns normally| EPrompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(agentex): tell a user when SGP won't..." | Re-trigger Greptile
Context used: