feat(agentex): run Slack turns as the invoking user, and the flow to link them - #410
Merged
Merged
Conversation
…link them Part of the event-driven agents work. Builds on the identity-link storage from #409, which nothing consumed until now. Before this, every Slack-originated turn ran as a single shared bot service account, so the agent acted with one fixed identity no matter who asked. Its tools could only ever reach whatever that bot could reach. Now, when the person who sent the message has linked their account, the turn runs as them and their own connected integrations (Notion, Linear, ...) resolve. Two pieces: 1. Gateway wiring. `_turn_identity()` resolves the Slack (team, user) to a link and returns that user's principal plus acting headers; absent a link it falls back to the existing bot behavior, so unlinked users are unaffected. Task naming becomes per-user (`slack:{thread_ts}:{sgp_user_id}`) for linked users so two people in one thread don't share a task; unlinked users keep the legacy key. `slack_user_id` / `sgp_user_id` land in task_metadata for attribution. 2. The link flow. The mapping cannot be derived, since nothing in a webhook says anything about SGP, so it is established in the one moment where both identities are authenticated at once: - Slack's HMAC proves the Slack side; that verified identity is parked in a single-use, short-TTL nonce (`link_nonce_service`). - The user clicks through to `/integrations/slack/link`, which is deliberately NOT auth-whitelisted (unlike `/slack`), so the auth middleware turns their own browser session into the SGP side. - Both halves now present in one request: a confirmation page names each identity, then POST mints a key as that user, encrypts and stores it, and burns the nonce. The nonce matters: with ids in the query string an attacker could bind someone else's chat identity to their own SGP account by editing the URL, after which the victim's messages would run as the attacker. An opaque token means nothing in the URL is forgeable. Notes on the pieces that are easy to get wrong: - Keys must come from identity-service, not egp-api-backend. Both mint API keys and only the former's are accepted by sgp-secrets; the latter authenticate fine against their own issuer and then fail at the vault with an opaque 401. A stored key of the wrong kind yields a link that looks healthy and silently reads nothing, so the client refuses anything not `ssk_`-shaped. - Minting requires the user's *cookie*. `POST /api-keys` is guarded by a JWT cookie guard that rejects `x-api-key`, which is why the browser leg is load-bearing and cannot be scripted away. - The identity-service URL is read from the environment with no default. It is deployment-specific, and a wrong default would POST a user's forwarded session credentials at whatever answers. - `acting_headers()` is never cached, while resolution is (with a shorter TTL for negative results). Caching a credential is how you serve a revoked one. - Every "cannot act as them" case returns None rather than a partial identity, so the gateway falls back explicitly instead of half-assuming a user. Not wired up yet: nothing sends the user a link. On an unlinked mention the gateway still runs as the bot rather than offering to connect, so the flow is only reachable via scripts/dev_seed_link_nonce.py. The DM trigger (plus its rate-limiting and replay of the stashed turn) is the next change. The live mint is also unverified: it needs a real browser session against a deployed host, which no test can stand in for. Everything up to the mint, and everything after it, is covered. Testing: 69 new unit tests -- 58 across four new files (link service, nonce service, identity-service client, routes) and 11 added to the gateway suite. 139 pass across the five touched test files; the full unit suite passes. Local integration tests error at fixture setup for want of a working Docker socket, which reproduces identically on a clean checkout of main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✱ Stainless preview buildsThis PR will update the openapi python typescript
|
Repeated mentions previously minted a fresh nonce each time, leaving a user with
several separately-redeemable links. That is worse than untidy: a nonce is a
bearer token, so whoever holds one gets linked to that provider identity by
signing in as themselves. Several live tokens means several chances for one to be
redeemed by the wrong person, and consuming one did not invalidate its siblings —
so a user who linked successfully still had working tokens pointing at their
Slack identity until each expired on its own.
Now there is at most one live nonce per identity:
- `create()` invalidates whatever nonce that identity already held, via a
`link_nonce_user:{provider}:{team}:{user}` pointer.
- `create_or_reuse()` returns the live token when there is one, so a second
mention re-sends the same link instead of minting a parallel one.
- `claim_send()` caps DMs about a given link at 2 (IDENTITY_LINK_MAX_DMS). Past
the cap the caller should fall back to an ephemeral in-channel notice rather
than going silent. The counter is cleared whenever a fresh nonce is minted, so
a genuinely new link is never withheld.
Two deliberate choices:
- Reuse does NOT extend the TTL. Otherwise mentioning the agent every few
minutes keeps a single token alive indefinitely, and the bounded lifetime is
the point of the nonce.
- The pending turn IS refreshed within the remaining window, so linking answers
what the user most recently asked rather than their first attempt. Best-effort:
without KEEPTTL the earlier question stands, which is worse UX than the newest
but better than dropping the nonce and forcing a re-link.
The pointer is verified against the request's identity rather than trusted, so a
stale or corrupted pointer cannot hand one user a token that links someone
else's identity.
Testing: 18 new unit tests (30 in the file) covering supersede-on-recreate,
per-identity isolation including the same user in two workspaces, TTL
non-extension, pending-turn refresh with and without KEEPTTL, the stale and
cross-identity pointer cases, the send cap and its reset, and repair of a
counter left without an expiry by a crash between INCR and EXPIRE.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The unit tests for the nonce run against a hand-written _FakeRedis, so they assert our *model* of Redis rather than Redis. Where the model is wrong the unit tests pass and production breaks, so these cover the places it could be wrong: - The app configures decode_responses=False, so real Redis returns bytes where the fake returns str. If the pointer read isn't decoded, create_or_reuse silently misses and mints a parallel token -- exactly the accumulation the pointer exists to prevent, and the fake cannot catch it. - GETDEL really removing the key, and doing so atomically: five concurrent consumes of one token must produce exactly one winner. A read-then-delete would let two through and mint two credentials for one link. - KEEPTTL really preserving an expiry while rewriting the value. This is the load-bearing one and the fake cannot prove it at all: if the payload rewrite dropped the TTL, someone mentioning the agent every few minutes would keep a token alive indefinitely, and the bounded lifetime is the whole point of a nonce. - INCR/EXPIRE/TTL behaving as the send cap assumes, including real Redis reporting -1 for a key with no expiry (the crash-between-INCR-and-EXPIRE repair path) and ten concurrent claim_send calls yielding exactly _MAX_SENDS. Depends only on the redis_url fixture rather than isolated_repositories, which also starts Postgres and MongoDB. Nothing here needs either, and dropping them keeps the file fast (~3s) and runnable where the Mongo image won't boot. The Redis container is session-scoped and shared, so each test namespaces its keys by test name instead of flushing the database out from under its neighbours. 14 tests, verified locally against a real Redis and stable across three runs. Note for anyone else running integration tests on Rancher Desktop: scripts/ run_tests.py sets TESTCONTAINERS_HOST_OVERRIDE to the VM address, which the host cannot reach, so Redis connections time out. Overriding it to 127.0.0.1 and disabling the testcontainers reaper works: TESTCONTAINERS_RYUK_DISABLED=true \ TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 \ DOCKER_HOST=unix://$HOME/.rd/docker.sock \ uv run python -m pytest tests/integration/test_link_nonce_service_redis.py Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mint could never have worked, and a manual test against the live service is
what found it. identity-service permits ONE API key per user; every active user
already has one (auto-named "<email> - API Key"), so POST /api-keys answers
409 "API key already exists for this user" -- with a unique name, so it is not a
name collision. 409 is not even a documented response on that endpoint. The
existing key's secret cannot be read back, and rotating theirs would silently
break whatever else uses it.
All 86 unit tests passed against this design. Only a real request found it.
Instead, store the credential the caller already presents: their own session
cookie. Verified against the live services:
agentex-auth, no auth -> 401, and the body enumerates the
accepted forms, including jwt_cookies
agentex-auth, cookie only -> 400 "Account ID is required"
agentex-auth, cookie + account -> 200, full principal
sgp-secrets, cookie only -> 400 "Authentication failed"
sgp-secrets, cookie + account -> 404 NOT_FOUND (route-level: auth PASSED)
The 400 -> 404 transition is the proof: with account context the vault stops
rejecting the credential and starts looking for a route.
This is not a workaround. delegation_headers.py already forwards session cookies
as x-acting-user-cookie, with _identityJwt allowlisted by default, so
acting_headers() only had to return a cookie instead of an api-key. Delegation
itself is untouched.
Why the session cookie is the better credential anyway:
- It carries its own expiry, so credential_expires_at comes from the JWT's exp
rather than a TTL we invent. The user's existing API key has expiresOn: null.
- It is already in the request, so there is no outbound call left to fail --
which removes the mint's whole error surface, IDENTITY_SERVICE_URL, and the
cookie-guard constraint that made the mint untestable in the first place.
- Taking it changes nothing about the user's existing credentials.
The cost, stated plainly: the link now lives and dies with the session. Sign-out
or revocation ends it, which surfaces downstream as a rejected credential and
should prompt a re-link rather than an error.
Account id is now required to link. The secrets service refuses a session
credential without one, and an account the user is not a member of is a 403
("Identity <id> (user) is not authorized to access"), so it must be *their*
account, captured from their own principal. Storing a link without one would look
connected and resolve nothing.
Removed the identity-service adapter and its 15 tests: unused, and the approach
it encodes is now known not to work.
No migration. #409's credential_ciphertext column is credential-agnostic, so what
changes is only what we put in it.
Testing: 57 tests across the reworked routes, service, and the new session_jwt
util. The util's tests concentrate on malformed input, since anything unreadable
must return "unknown" so the caller substitutes a bounded fallback -- including
exp=True, which a naive isinstance check accepts as an int and turns into
1970-01-01. Full unit suite: 666 passed.
Still unverified: an actual secret read. Auth against the vault is proven; I could
not find its route path to fetch a secret end to end.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`task/create` is get-or-create on the name, so two turns that produce the same
name become ONE task -- merging their prompts, metadata, agent configuration and
account context into a single conversation. The per-user key was
`slack:{thread_ts}:{sgp_user_id}`, which pins the user but not where they were
talking.
`thread_ts` is a microsecond Slack timestamp and is unique within a workspace, so
a same-workspace collision is not realistic. Nothing makes it unique ACROSS
workspaces, though, and this gateway is explicitly multi-workspace -- `team_id` is
part of the identity everywhere else in the flow. The odds are tiny, the failure is
silent and cross-tenant, and the two extra segments cost nothing:
slack:{team_id}:{channel}:{thread_ts}:{sgp_user_id}
They also bound a latent hazard: `normalize()` falls back to `thread_ts=""` when an
event carries neither `thread_ts` nor `ts`. On the old key every such turn by one
user collapsed into a single global task. With workspace and channel present it
degrades to one task per (workspace, channel, user), which is a coherent
conversation anyway. Both current construction paths do populate it -- `event.ts`
is always present on app_mention/message, and the modal path returns early if its
breadcrumb post fails -- so this is defence, not a live bug.
No back-compat concern: no identity links exist in any environment yet, so there
are no live per-user tasks to re-key.
The legacy unlinked key (`slack:{thread_ts}`) is left alone. It has the same
cross-workspace weakness and predates this work, but widening it would re-key
threads already in flight. Flagged rather than changed.
Also fixes a regression from the session-credential rework in the same area:
`_resolve_config_id` required `x-api-key`, which the shared bot has and a linked
user does not -- their acting headers carry a session cookie. So config-name
resolution silently returned None for exactly the users this feature is for, and
they would land on the default config while a bot-run turn resolved the same name
correctly, with nothing in the logs pointing at the cause. The credential check is
now form-agnostic and forwards whatever we hold.
Whether that endpoint accepts cookie auth is unverified: SLACK_GATEWAY_SGP_BASE_URL
is unset in dev, so the path is inert there and could not be exercised. If it does
not accept cookies the request fails and we fall back to the default id, which is
the same outcome as before -- so this is safe either way, just no longer silently
wrong for one credential form only.
Testing: 4 new unit tests -- cross-workspace and cross-channel collisions, the
empty-thread_ts degradation, and a cookie-credential config resolution that asserts
the credential is forwarded as-is. 80 pass in the gateway suite; full unit suite
670 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…al change
Both comments still said the Slack gateway forwards an x-api-key. It forwards a
session cookie when the turn runs as a linked user, which build_delegation_headers
converts to x-acting-user-cookie. Comments only -- no behavior change.
Verified the conversion rather than assuming it: the gateway's acting headers reach
get_delegation_headers via _ScheduledRunRequest.headers, and
build_delegation_headers turns {"cookie": "_identityJwt=..."} into
{"x-acting-user-cookie": "_identityJwt=..."}, filtering a realistic browser Cookie
header down to just the allowlisted morsel. The bot path still yields
x-acting-user-api-key.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
michael-chou359
added a commit
that referenced
this pull request
Aug 28, 2026
#412) ## What The last piece of the identity-link flow. #409 added the storage, #410 the gateway wiring and the link routes — but nothing ever handed a user a link, so the only way in was a dev script. Now an unlinked mention offers one. When a turn is **not** running as a person — unlinked, or linked with a credential we can't use (expired session, undecryptable ciphertext) — the gateway: ``` 1. mint (or reuse) a nonce carrying the Slack identity Slack's HMAC just verified, plus the message that triggered it 2. conversations.open -> chat.postMessage DM that user the link 3. chat.postEphemeral tell them in-channel a DM is waiting ``` A dead credential gets the same offer as no credential, since re-linking is the fix for both. ## The link is DMed and never posted in a channel This is the part worth reviewing closely. The nonce is a **bearer token**: whoever opens it gets linked to that Slack identity by signing in as themselves. Posted in a channel, the first person to read it could bind someone else's Slack identity to their own SGP account. So when `conversations.open` or the DM fails, we log and stop — there is no fallback to anywhere visible. A test asserts the token never appears in any payload addressed to the origin channel, and another that a failed DM leaks it nowhere. ## Best-effort by construction The turn is already proceeding (as the shared bot, or being refused immediately after), and nothing in the offer path may change that. A Redis outage, a missing scope, a closed DM — all end in "no offer" and an unaffected turn. Offers also require `SLACK_GATEWAY_PUBLIC_BASE_URL`. Unset means no offers at all: the host must be browser-reachable *and* a sibling subdomain of the SGP host or the session cookie never arrives at the callback. A link that cannot work is worse than no link. ## Rate limiting, two layers for two problems - **`claim_send`** (from #410) caps DMs about one live link at 2, so a re-mention re-sends the same link rather than going quiet. - **A cooldown key** (`SLACK_LINK_OFFER_COOLDOWN_S`, default 1h) stops a fresh nonce from re-arming that budget every time the old one expires. Without it, a persistent mentioner collects ~12 DMs an hour instead of ~2. It fails **open** on a Redis error — never offering is the worse failure, and `claim_send` still bounds it. ## Email match — shipped OFF The nonce stops an attacker forging someone else's Slack identity. It does **not** stop them forwarding their *own* link: if a victim clicks it while signed in, the attacker's Slack identity binds to the victim's SGP account, and from then on the attacker's Slack messages run as the victim, with the victim's integrations. The confirmation page naming both identities catches a mis-click but reduces to user vigilance against a deliberate attempt. Comparing the Slack account's email against the signed-in SGP account's closes it. It ships disabled because it needs the **`users:read.email` Slack scope, which is not granted** — verified, `users.lookupByEmail` returns `missing_scope`. Enable `IDENTITY_LINK_REQUIRE_EMAIL_MATCH` and the scope together.⚠️ **The check treats an unreadable email as a MISMATCH, not as "skip".** So turning the flag on without the scope refuses every link. That direction is deliberate: failing open would silently disable the only defence the moment the scope lapsed. Tests pin both directions. ## Not included **Replaying the stashed turn.** `pending_turn` is recorded for it, but the confirmation page still says "ask me again in Slack". Wiring the link route back into the gateway is a coupling worth doing on its own, with care that a replay failure can't undo a successful link. ## Testing **15 new unit tests**, aimed at the failure modes rather than the happy path: - link offer: the token never reaching the origin channel; a failed DM leaking it nowhere; no offer without a public base URL; the send cap acknowledging in-channel instead of going silent; the cooldown suppressing before a nonce is even minted; a Redis error swallowed; the DM text warning against forwarding; `pending_turn` stashed for a later replay. - email match: disabled-by-default doesn't call Slack at all; matching (and case-insensitive) emails link; a mismatch refuses **and leaves the nonce intact**, so a legitimate owner can still use their own link; unreadable Slack email and missing SGP email both fail closed. Full unit suite: **685 passed**. `ruff` clean. The 193 local integration errors are the pre-existing testcontainers/Mongo issue in files this PR doesn't touch — they reproduce identically on a clean checkout of main. ## Deploy notes Nothing here activates on its own: | Variable | Effect if unset | |---|---| | `SLACK_GATEWAY_PUBLIC_BASE_URL` | **no offers are sent at all** | | `SLACK_LINK_OFFER_COOLDOWN_S` | defaults to 1h | | `IDENTITY_LINK_REQUIRE_EMAIL_MATCH` | email match skipped (current state) | Scopes: `im:write` and `chat:write` are already granted, so DMs and ephemerals work today. `users:read.email` is the only addition needed, and only for the email match. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- greptile_comment --> <h3>Greptile Summary</h3> Adds Slack account-link offers for users whose turns cannot run under a usable personal identity, with bearer links delivered only through DMs and an ephemeral channel acknowledgment. - Adds Slack profile lookup and optional Slack/SGP email matching during confirmation. - Adds nonce reuse, delivery limits, and a per-user offer cooldown. - Adds unit coverage for delivery confidentiality, failure handling, rate limiting, pending turns, and email matching. <details><summary><h3>Confidence Score: 4/5</h3></summary> The failed-delivery cooldown behavior should be fixed before merging because a transient Slack failure can prevent the user from receiving a link for an hour. The gateway records the offer cooldown before attempting DM delivery and does not undo it on either Slack failure path, causing later mentions to be suppressed despite no successful offer. **Files Needing Attention:** agentex/src/domain/use_cases/slack_gateway_use_case.py </details> <details><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | agentex/src/domain/use_cases/slack_gateway_use_case.py | Adds the Slack DM offer flow, profile lookup, cooldown, and ephemeral acknowledgment; failed DM delivery incorrectly leaves the cooldown active. | | agentex/src/api/routes/integrations.py | Adds an explicitly opt-in, fail-closed email correspondence check before persisting a Slack identity link. | | agentex/tests/unit/use_cases/test_slack_gateway_use_case.py | Covers link confidentiality and major offer outcomes but does not verify that a failed delivery remains retryable. | | agentex/tests/unit/api/test_integrations_routes.py | Covers enabled, disabled, matching, mismatching, and unreadable-email confirmation behavior. | </details> <details><summary><h3>Sequence Diagram</h3></summary> ```mermaid sequenceDiagram participant U as Slack user participant G as Slack gateway participant R as Redis/nonce service participant S as Slack API participant I as Link confirmation route G->>G: Resolve invoking identity alt No usable personal identity G->>R: Claim offer cooldown G->>R: Create or reuse nonce and claim send G->>S: conversations.open G->>S: chat.postMessage with bearer link G->>S: chat.postEphemeral acknowledgment U->>I: Open link and confirm while signed in I->>I: Optionally compare Slack and SGP email I->>R: Consume nonce and persist identity link end ``` </details> <a href="https://app.greptile.com/api/ide/cursor?prompt=Greploop%20scaleapi%2Fscale-agentex%20PR%20%23412%3A%20work%20through%20Greptile's%20open%20review%20comments%2C%20then%20keep%20reviewing%20and%20fixing%20until%20it%20comes%20back%20clean%20at%205%2F5%20with%20zero%20unresolved%20comments.%0AStart%20by%20reading%20the%20comments%20off%20the%20PR%20itself.%20On%20GitHub%2C%20use%20paginated%20%60gh%20api%20graphql%60%20to%20query%20%60pullRequest.reviewThreads%60%20with%20each%20thread's%20%60isResolved%60%20value%20and%20inline%20%60comments%60.%20%60gh%20pr%20view%20--comments%60%20only%20includes%20conversation%20comments%2C%20so%20do%20not%20use%20it%20as%20the%20findings%20source.%20They%20are%20not%20listed%20in%20this%20prompt%20on%20purpose%3A%20the%20PR%20is%20current%2C%20a%20pasted%20copy%20would%20not%20be.%20Skip%20anything%20already%20resolved%2C%20and%20if%20you%20judge%20a%20comment%20wrong%2C%20say%20so%20rather%20than%20changing%20code%20to%20satisfy%20it.%0A%0APrefer%20the%20Greptile%20CLI%2C%20which%20reviews%20the%20working%20tree%20with%20no%20push%20and%20no%20CI%20run.%20Fall%20back%20to%20PUSH%20LOOP%20only%20where%20a%20step%20below%20says%20to.%0A1.%20Run%20%60command%20-v%20greptile%60.%20Missing%3A%20go%20to%20PUSH%20LOOP%2C%20and%20pitch%20the%20CLI%20at%20the%20end.%0A2.%20Run%20%60greptile%20whoami%60.%20It%20exits%200%20even%20when%20signed%20out%2C%20so%20read%20the%20output%3A%20if%20it%20says%20%22Not%20signed%20in%22%2C%20ask%20the%20user%20to%20run%20%60greptile%20login%60%20%28it%20opens%20a%20browser%2C%20do%20not%20run%20it%20yourself%29%20and%20go%20to%20PUSH%20LOOP%20with%20no%20pitch.%0A3.%20Look%20for%20%60greploop%2FSKILL.md%60%20under%20%60.agents%2Fskills%2F%60%20or%20%60.claude%2Fskills%2F%60%2C%20both%20in%20the%20repo%20root%20and%20in%20the%20user's%20home%20directory.%20Found%3A%20follow%20that%20skill%2C%20never%20pushing%20between%20rounds%2C%20and%20you%20are%20done.%0A4.%20Missing%3A%20run%20%60greptile%20skills%20list%60.%20If%20it%20fails%20as%20an%20unknown%20command%2C%20this%20CLI%20predates%20the%20skill%2C%20so%20tell%20the%20user%20to%20run%20%60greptile%20update%60%20and%20go%20to%20PUSH%20LOOP%20with%20no%20pitch.%20Otherwise%20ask%20the%20user%20before%20running%20%60greptile%20skills%20install%20greploop%60%2C%20then%20follow%20the%20skill%20it%20writes.%0A%0APUSH%20LOOP%20%28only%20when%20a%20step%20above%20sends%20you%20here%29%3A%0A%0AGREPLOOP%20ENABLED%3A%20After%20fixing%20all%20issues%2C%20commit%20and%20push%20your%20changes.%20Greptile%20will%20automatically%20re-review%20the%20PR%20via%20webhook.%20Wait%20for%20the%20review%20to%20complete%2C%20then%20read%20the%20new%20review%20comments%20and%20confidence%20score.%20If%20the%20confidence%20score%20is%20below%205%2F5%20or%20there%20are%20unresolved%20comments%2C%20fix%20those%20issues%20and%20push%20again.%20Repeat%20until%20the%20confidence%20score%20is%205%2F5%20with%20zero%20unresolved%20comments.%20Maximum%205%20iterations.%20Do%20NOT%20open%20a%20new%20PR%20%E2%80%94%20keep%20pushing%20to%20the%20same%20branch.%0A%0AIF%20YOU%20ENDED%20UP%20ON%20PUSH%20LOOP%20BECAUSE%20THE%20CLI%20WAS%20MISSING%3A%20once%20the%20loop%20is%20done%2C%20print%20the%20block%20below%20and%20nothing%20else%20about%20the%20CLI.%20Reproduce%20it%20character%20for%20character%20inside%20a%20fenced%20code%20block%2C%20so%20the%20borders%20line%20up.%20Do%20not%20reword%20it%2C%20translate%20it%2C%20summarize%20it%2C%20add%20to%20it%2C%20or%20drop%20the%20box.%20Print%20it%20once%2C%20and%20do%20not%20bring%20the%20CLI%20up%20again%20later%20in%20the%20session.%0A%0A%2B----------------------------------------------------------------------------%2B%0A%7C%20%20You%20can%20run%20greploops%20faster%20locally%20with%20our%20CLI.%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%7C%0A%7C%20%20Install%20it%20at%20https%3A%2F%2Fwww.greptile.com%2Fcli%2C%20or%20I%20can%20install%20it%20for%20you.%20%20%7C%0A%2B----------------------------------------------------------------------------%2B%0A%0AIf%20they%20take%20you%20up%20on%20it%2C%20install%20with%20%60npm%20install%20-g%20greptile%60%20%28or%20%60brew%20install%20greptileai%2Ftap%2Fgreptile%60%29%2C%20then%20%60greptile%20skills%20install%20greploop%60.%20Leave%20%60greptile%20login%60%20to%20them%2C%20it%20opens%20a%20browser.&pr=412&platform=github"><img alt="Fix all with Greploop" src="https://greptile-static-assets.s3.us-east-1.amazonaws.com/badges/FixAllInGrepLoop.svg?v=2"></a> <a href="https://app.greptile.com/api/ide/cursor?prompt=%23%23%23%20Issue%201%0Aagentex%2Fsrc%2Fdomain%2Fuse_cases%2Fslack_gateway_use_case.py%3A1270%0A**Failed%20DMs%20consume%20the%20cooldown**%0A%0AWhen%20%60conversations.open%60%20or%20%60chat.postMessage%60%20fails%2C%20the%20cooldown%20has%20already%20been%20claimed%20and%20is%20not%20released%2C%20causing%20every%20subsequent%20mention%20to%20suppress%20another%20offer%20for%20up%20to%20one%20hour%20even%20though%20the%20user%20received%20no%20link.%0A%0A---%0A%0AFor%20each%20issue%20above%2C%20determine%20whether%20it%20is%20valid%20and%20should%20be%20fixed.%20If%20so%2C%20fix%20it%20directly.&pr=412&platform=github"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixAllInCursorDark.svg?v=6"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixAllInCursor.svg?v=6"><img alt="Fix All in Cursor" src="https://greptile-static-assets.s3.amazonaws.com/badges/FixAllInCursor.svg?v=6"></picture></a> <a href="https://app.greptile.com/ide/claude-code?prompt=%23%23%23%20Issue%201%0Aagentex%2Fsrc%2Fdomain%2Fuse_cases%2Fslack_gateway_use_case.py%3A1270%0A**Failed%20DMs%20consume%20the%20cooldown**%0A%0AWhen%20%60conversations.open%60%20or%20%60chat.postMessage%60%20fails%2C%20the%20cooldown%20has%20already%20been%20claimed%20and%20is%20not%20released%2C%20causing%20every%20subsequent%20mention%20to%20suppress%20another%20offer%20for%20up%20to%20one%20hour%20even%20though%20the%20user%20received%20no%20link.%0A%0A---%0A%0AFor%20each%20issue%20above%2C%20determine%20whether%20it%20is%20valid%20and%20should%20be%20fixed.%20If%20so%2C%20fix%20it%20directly.&repo=scaleapi%2Fscale-agentex&pr=412&platform=github"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixAllInClaudeDark.svg?v=6"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixAllInClaude.svg?v=6"><img alt="Fix All in Claude Code" src="https://greptile-static-assets.s3.amazonaws.com/badges/FixAllInClaude.svg?v=6"></picture></a> <a href="https://app.greptile.com/api/ide/codex?prompt=IMPORTANT%3A%20Work%20in%20the%20repository%20%22scaleapi%2Fscale-agentex%22%20on%20the%20existing%20branch%20%22mc%2Fslack-link-dm-trigger%22.%20Checkout%20that%20branch%20%E2%80%94%20do%20NOT%20create%20a%20new%20branch%20or%20open%20a%20new%20PR.%20Push%20your%20changes%20to%20%22mc%2Fslack-link-dm-trigger%22.%0A%0A%23%23%23%20Issue%201%0Aagentex%2Fsrc%2Fdomain%2Fuse_cases%2Fslack_gateway_use_case.py%3A1270%0A**Failed%20DMs%20consume%20the%20cooldown**%0A%0AWhen%20%60conversations.open%60%20or%20%60chat.postMessage%60%20fails%2C%20the%20cooldown%20has%20already%20been%20claimed%20and%20is%20not%20released%2C%20causing%20every%20subsequent%20mention%20to%20suppress%20another%20offer%20for%20up%20to%20one%20hour%20even%20though%20the%20user%20received%20no%20link.%0A%0A---%0A%0AFor%20each%20issue%20above%2C%20determine%20whether%20it%20is%20valid%20and%20should%20be%20fixed.%20If%20so%2C%20fix%20it%20directly.&repo=scaleapi%2Fscale-agentex&pr=412&platform=github"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixAllInCodexDark.svg?v=6"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixAllInCodex.svg?v=6"><img alt="Fix All in Codex" src="https://greptile-static-assets.s3.amazonaws.com/badges/FixAllInCodex.svg?v=6"></picture></a> <details><summary>Prompt To Fix All With AI</summary> `````markdown ### Issue 1 agentex/src/domain/use_cases/slack_gateway_use_case.py:1270 **Failed DMs consume the cooldown** When `conversations.open` or `chat.postMessage` fails, the cooldown has already been claimed and is not released, causing every subsequent mention to suppress another offer for up to one hour even though the user received no link. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. ````` </details> <sub>Reviews (1): Last reviewed commit: ["feat(agentex): DM unlinked Slack users a..."](ee8104a) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=57819517)</sub> > Greptile also left **1 inline comment** on this PR. <!-- /greptile_comment --> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
What
Part of the event-driven agents work, and the half of it that changes behavior.
#409 added storage for linking an external chat identity to an SGP user; nothing
consumed it. This wires it up and adds the flow that creates the links.
Before this, every Slack-originated turn ran as a single shared bot service account.
The agent acted with one fixed identity regardless of who asked, and its tools could
only ever reach what that bot could reach. After this, when the person who sent the
message has linked their account, the turn runs as them — so their own connected
integrations (Notion, Linear, …) resolve.
Unlinked users are unaffected. No link means the existing bot path, unchanged.
Verified end to end
The credential path is proven against the live dev services, not just mocked:
So a stored session credential really does read that user's own decrypted secret,
with
owner_user_idderived from the caller, and the shared bot cannot read it.One caveat on that control: the bot is refused at authentication (its key comes from
a different issuer than the vault accepts), not at authorization. It establishes that
the bot can't reach the user's secrets, but not that per-owner scoping is what stops
it. The
owner=<the user>in the list response is the evidence for scoping.The linking flow
The mapping can't be derived — nothing in a webhook says anything about SGP. So it's
established in the one moment where both identities are authenticated at once, with a
server-side nonce tying two requests together.
Why a nonce rather than ids in the URL. A query string is user-editable. Given
?slack_user=<someone else>, an attacker could click their own link while signed in asthemselves and bind that person's chat identity to their own SGP account — after
which the victim's messages would run as the attacker, using the attacker's
integrations, with the victim's prompts landing in the attacker's account. An opaque
token means nothing in the URL is meaningful, so nothing in it is forgeable.
Why the session cookie, and not a minted API key
Minting was the original design. It cannot work, and only a live request found
that out — all 86 unit tests passed against it first.
identity-service permits one API key per user, and every active user already has
one (auto-named
"<email> - API Key"). SoPOST /api-keysanswers409 "API key already exists for this user"— reproduced with a unique name, so it'snot a name collision, and 409 isn't even a documented response on that endpoint. The
existing key's secret can't be read back, and rotating theirs would silently break
whatever else uses it.
Storing the caller's session cookie is better on its own merits, not just as a
fallback:
credential_expires_atcomes from the JWT'sexpinstead of a TTL we invent. (The API key we'd have minted alongside theirs would
have sat next to one with
expiresOn: null.)mint's entire error surface, the
IDENTITY_SERVICE_URLconfig, and the cookie-guardconstraint that made the mint impossible to test in the first place.
And it isn't a workaround: delegation_headers.py
already forwards session cookies as
x-acting-user-cookiewith_identityJwtallowlisted by default.
acting_headers()just had to return a cookie instead of anapi-key; delegation itself is untouched.
The cost, stated plainly: the link now lives and dies with the session. Sign-out or
revocation ends it, surfacing downstream as a rejected credential. The gateway should
treat that as "re-link needed" rather than an error — that belongs with the DM trigger.
Change
Gateway (
slack_gateway_use_case.py)_turn_identity()resolves(team, user)→ link → that user's principal and actingheaders. Falls back to the bot when there's no usable link.
slack:{thread_ts}:{sgp_user_id}), sotwo people in one thread don't write into a shared task. Unlinked users keep the
legacy key, so existing threads keep their history.
slack_user_id/sgp_user_idrecorded intask_metadatafor attribution.Link flow (new)
link_nonce_service.py— create / peek / consume.peekis non-consuming so arefresh or a link-prefetching browser doesn't burn the nonce;
consumeisGETDEL,so single-use is enforced by Redis rather than a read-then-delete race. Requires
Redis and raises rather than degrading.
At most one live nonce per identity. A nonce is a bearer token, so several live
ones means several chances for a link to be redeemed by the wrong person — and
consuming one doesn't invalidate its siblings, so a user who linked successfully
would still have working tokens pointing at their Slack identity until each expired.
create()invalidates whatever nonce the identity already held;create_or_reuse()re-sends the live link rather than minting a parallel one;
claim_send()caps DMsabout a given link at 2 (
IDENTITY_LINK_MAX_DMS), resetting when a genuinely newnonce is minted so a new link is never withheld.
Reuse deliberately does not extend the TTL — otherwise mentioning the agent every
few minutes keeps one token alive indefinitely. The pending turn is refreshed within
the remaining window, so linking answers what the user most recently asked.
integrations.py— the two routes and the confirmation page. Deliberately under/integrations, not/slack: the latter is auth-whitelisted because Slack'ssignature is its auth, and a callback there would run unauthenticated, defeating the
whole mechanism.
session_jwt.py— reads theexpclaim. Does not verify the signature, and mustnever be used as an auth check: the token arrives on an already-authenticated request,
and verifying would mean holding the signing key, which agentex shouldn't have.
identity_link_service.py— cached resolution; uncached credential access.Account id is now required to link
The vault refuses a session credential with no account context (
400 "Account ID is required"), and an account the user isn't a member of is a403 "Identity <id> (user) is not authorized to access". So it has to be their account, captured from theirown principal — not the gateway's configured one, which only works for members of it
and would anyway look in the wrong account for their connections.
Linking without one returns a 400 asking them to pick an account, and
acting_headers()returnsNonerather than partial headers, because a link storedwithout an account would look connected and resolve nothing.
Other details that are easy to get wrong
acting_headers()is never cached, though resolution is (shorter TTL for negativeresults). Caching a credential is how you end up serving a revoked one.
None, not a partial identity, so thegateway falls back explicitly instead of half-assuming a user.
the link clickable instead of sending the user back to Slack.
unbounded credential is how you end up holding one indefinitely.
What this does NOT do
Nothing sends anyone a link. On an unlinked mention the gateway still runs as the
bot; it doesn't offer to connect. Step 1 has no trigger, so today the flow is only
reachable through
scripts/dev_seed_link_nonce.py. The DM trigger —conversations.open+chat.postMessage, an ephemeral in-channel notice, and replayingthe stashed
pending_turn— is the next change.claim_send()is the primitive it willcall; nothing calls it yet.
Also open, and better settled in that PR than here:
stopping someone forwarding their own link to another person, who could click
through and bind the sender's chat identity to their own SGP account. The page names
both identities, which covers a mis-click but reduces to user vigilance against a
deliberate attempt. Comparing the Slack account's email against the SGP session's
email would close it — the storage already has
IdentityLinkMethod.EMAIL_MATCH.So this PR is safe to merge but not yet user-visible.
Testing
~100 new unit tests across the routes, link service, nonce service and the
session-JWT util, plus 11 added to the gateway suite. Full CI green.
Coverage aims at failure modes rather than the happy path: unauthenticated caller
stores nothing, expired/stale nonce refused, session cookie absent, session already
expired, principal with no account, SGP account already linked to a different Slack
user, unconfigured encryption key reported as 503 rather than 500, and the nonce token
leaking nothing identifying.
The JWT util's tests concentrate on malformed input, since anything unreadable must
return "unknown" so the caller substitutes a bounded fallback — including
exp=True,which a naive
isinstancecheck accepts as an int and turns into 1970-01-01, i.e. acredential that looks permanently expired.
14 integration tests against a real Redis. The nonce's unit tests use a
hand-written fake, so they assert our model of Redis rather than Redis. These cover
the divergences that matter: the app runs
decode_responses=Falseso real Redisreturns bytes where the fake returns
str(an undecoded pointer read wouldsilently mint parallel tokens);
GETDELatomicity under five concurrent consumes;KEEPTTLgenuinely preserving an expiry while rewriting the payload — which the fakecan't prove at all, and which the "reuse must not extend the lifetime" guarantee rests
on; and
INCR/EXPIRE/TTLreturning what the send cap assumes.These depend only on
redis_url, notisolated_repositories, so they need no Postgresor MongoDB — keeping them at ~3s and runnable where the Mongo image won't boot.
Verified locally against real Redis, stable across three runs.
Still unverified: the browser click-through itself, which needs this deployed —
there's no route to click until then. Every hop it depends on is individually proven
(cookie → principal → vault → decrypted secret).
Deploy notes
AGENTEX_CREDENTIAL_ENCRYPTION_KEYmust be set, or linking returns 503 (by design —it will not store a credential in plaintext). Already present in dev.
never arrives and step 3 can't resolve anyone.
credential_ciphertextcolumn is credential-agnostic, so only its contents change.
🤖 Generated with Claude Code
Greptile Summary
This PR links Slack identities to authenticated SGP users and delegates linked Slack turns through the user’s stored session credential.
Confidence Score: 3/5
The PR is not yet safe to merge because the previously reported nonce-redemption race still permits multiple authenticated confirmations to persist before a unique claim is established.
The confirmation route still peeks at the nonce, persists the identity link, and only afterward consumes the nonce without checking whether that request won the claim; overlapping submissions can therefore both mutate the mapping and report success.
Files Needing Attention: agentex/src/api/routes/integrations.py, agentex/src/domain/services/link_nonce_service.py
Important Files Changed
Sequence Diagram
Reviews (6): Last reviewed commit: "docs(agentex): correct delegation commen..." | Re-trigger Greptile
Context used: