Skip to content

fix(platform): feedback reach, OAuth renewals, unique slots, one 429 - #3197

Draft
larryro wants to merge 7 commits into
mainfrom
fix/followup-authz-consistency
Draft

fix(platform): feedback reach, OAuth renewals, unique slots, one 429#3197
larryro wants to merge 7 commits into
mainfrom
fix/followup-authz-consistency

Conversation

@larryro

@larryro larryro commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Five verified follow-up defects from the backend review, each fixed at its root with a colocated unit test and a real-Postgres probe. Base: origin/main @ 1ea4cc9e9.

Per-finding outcome

# Finding Outcome Evidence
1 POST /feedback never verifies messageId/threadId belong to the caller's org fixed Service now requires the message to exist in the caller's org + thread and the thread to be the caller's own or project-shared (loadOwnedThread / loadProjectSharedThread, the chat surface's own read grants); anything else is one opaque 404 MESSAGE_NOT_FOUND. Red on base: itest probe answered 200 and stored the rival's row (items=2).
2 A second Slack workspace connect (and Reconnect) always fails with storage_failed fixed (still live after open #3189, which keeps name: displayName) storeOauth2Grant: a workspace the team route already maps to this org RENEWS its credential (tokens rotated, active, detail cleared); a no-workspace connector renews its one/default oauth2 credential; a new workspace gets a unique label — Slack (Second Workspace) from the vendor's team.name (now parsed), or Slack (2). Red on base: itest reconnecting … → error/storage_failed, second workspace → error/storage_failed.
3 app.messages lacks UNIQUE (thread_id, order, step_order) fixed Migration 0074 (0071–0073 belong to open #3191): dedup first, then CREATE UNIQUE INDEX messages_thread_slot, drop the redundant plain index. Appenders use INSERT … ON CONFLICT DO NOTHING RETURNING + bounded re-claim. Red on base: 12 concurrent appends → 5 distinct orders (ties). Branch: 12/12 distinct, contiguous.
4 Twelve doors hand-roll divergent RateLimitExceededError → 429 shapes fixed (#3186's rateLimitedResponse is NOT on base — its file is added here verbatim as a superset) lib/rate-limit-response.ts: rateLimitedResponse (Hono), rateLimitedPlainResponse (Slack webhook, SSE auth), rateLimitExceededCause (documents/TTS wrappers carry the refusal as cause). Every refusal is 429 {error:'RATE_LIMITED', data:{retryAfterMs}} + whole-second Retry-After ≥ 1. Red on base: 5/6 doors had no Retry-After, the 6th the wrong body.
5a .env.example describes the 0.5 backend as an opt-in parallel build with Convex present fixed Rewrote the backend block (always on, BACKEND_UPSTREAM = proxy upstream override only, TALE_CONTROL_TOKEN semantics), removed POSTGRES_URL/INSTANCE_NAME (read nowhere), corrected INSTANCE_SECRET's real role (WebDAV HMAC key derivation), /metrics/backend, the TALE_PROVIDER_KEY_ name rule (1–64 chars, no 40-char env-sync cap), "convex image" → platform image.
5b generateDevCompose emits depends_on: object-store with no such service fixed tale dev needs blobs (the backend seeds the default blob connection at boot and refuses uploads without it; DEV_VOLUME_NAMES already carried object-store-data) → the dev stack now defines object-store via the shared createObjectStorageService. Red on base: new generator test fails with backend-api depends on object-store, which the dev compose does not define.

Intentionally untouched RateLimitExceededError sites (protocol adapters, not app 429 shapes): auth/auth.ts (Better Auth APIError TOO_MANY_REQUESTS), webdav/handlers.ts (WebDAV AppError consumed by the DAV lib), documents/service.ts#retryRagIndexingForDocument (an RPC {success:false} soft result).

Messages unique key + dedup

Key: (thread_id, "order", step_order) — exactly the ordering readers sort by; every appender writes step_order = 0 at max(order)+1, branch/arena copies preserve both columns into a fresh thread, and nothing legitimately shares a slot. Dedup (same migration, before the index): within every (thread_id, "order") group holding a tie, rows are renumbered 0..n-1 by (step_order, created_at_ms, id) — the order readers already observe, deterministic, no row deleted, untied groups untouched (replayed on a scratch table in the itest). Rolling-safe: the old image's appenders carry no ON CONFLICT, so a lost race during the roll answers an error for one send instead of writing a tie. Under SERIALIZABLE, ON CONFLICT raises 40001 and transactSerializable reruns the transaction; under READ COMMITTED the bounded loop re-claims the next slot.

Tests

  • vitest (server project), all new, red on base (19 failing tests + 1 unresolvable module): feedback/access.test.ts (4), feedback/routes.access.test.ts (1), connectors/oauth.test.ts (8), threads/store.test.ts (3), chat/append-message.test.ts (3), lib/rate-limit-response.test.ts (7); token_exchange.test.ts expectation extended for teamName.
  • bun test (CLI): generate-dev-compose.test.ts +2 (red on base: 2 fail).
  • integration-check: +16 probes (feedback ×2 incl. the rewritten upsert probe, slots ×2, 429 ×6, OAuth ×3 + 1 expectation), all green on branch; red on base as listed above.

Verification (observed)

  • services/platform: tsc --noEmit clean; oxlint --type-aware clean; vitest server project 5580 pass.
  • tools/cli: tsc clean, oxlint clean, bun test 313 pass / 18 skip.
  • backend:integration, throwaway tale-db + MinIO per run: branch 187 PASS / 7 FAIL, base 171 PASS / 7 FAIL — the 7 are identical base-environment failures (see discoveries), and both runs abort at the same point. The batch's own 16 probes: 16/16 on branch; feedback/slots/429/OAuth probes red on base code.
  • Compose: read-only validated through the generator test (parsed YAML: every depends_on target defined; object-store mounts the declared object-store-data volume).

Cross-class discoveries (not fixed here)

  1. Tip-of-main itest defectcheckTwoFactor's third probe (fix(platform): gate conversation writes and audit 2FA successes #3187) runs two-factor/enable→disable on the suite's MAIN user; Better Auth drops that session on disable (auth.api.getSession → null → requireSession 401), so every later probe fails (chat memories/composer/deferred ERR/401, automations trigger mint → "404 Not Found" parsed as JSON → fatal abort). Reproduced standalone: GET /api/app/chat/memories 200 → enable → 200 → disable → 401. Fix belongs to that probe (re-sign-in, refresh ctx.cookie) — it currently hides ~200 later checks, including checkConnectorOauth (verified here by temporarily reordering, not committed).
  2. fix(platform): gate conversation writes and audit 2FA successes #3187 × fix(platform): blob refs grant nothing; upload and import lanes gated #3160 conflict on main — the editor-or-above write gate on POST /api/app/conversations/compose now fires before fix(platform): blob refs grant nothing; upload and import lanes gated #3160's probe expects 403 attachment_not_owned (compose → 403 FORBIDDEN).
  3. Vite fs allow-list in worktrees — three app/routes/*.test.tsx fail to load (Denied ID …/tale/node_modules/@fontsource/...woff2) whenever node_modules is a symlink to another clone; identical on base.
  4. Docs: docs/en/self-hosted/configuration/environment-reference.md:45 still says INSTANCE_SECRET derives "the deploy admin key" (Convex-era); needs the same correction as .env.example in all three locales.

Merge notes (open siblings)

POST /api/app/feedback recorded a vote against any messageId/threadId the
client named, so a member could vote on (and probe) another organization's
message, or another member's private thread. The service now requires the
message to exist in the caller's organization and thread, and the thread to
be the caller's own or shared with a project they can read (the two grants
the chat surface reads through); anything else is one opaque 404.
Completing a connector consent a second time always created a credential
named after the connector, hit CREDENTIAL_NAME_TAKEN and surfaced as
storage_failed — so the settings card's Reconnect never reconnected, and a
second Slack workspace could not be connected at all.

A workspace the team route already maps to this organization now RENEWS its
credential (fresh tokens, active again); a connector without a workspace
notion renews its one oauth2 credential the same way; a new workspace gets
its own credential under a label no sibling holds — "Slack (Workspace)" from
the vendor's team name, or a counter. The exchange now surfaces team.name.
Every appender claimed max("order") + 1 with a read followed by a write, so
two concurrent appends to one thread landed on the same slot and the rows
tied — readers sorted them arbitrarily and branch forks copied both.

Migration 0074 first repairs existing ties deterministically (renumbering
step_order within each tied (thread, order) group by step, arrival, id —
nothing deleted, untied rows untouched), then makes the slot UNIQUE. Both
appenders insert with ON CONFLICT DO NOTHING and re-claim the next slot on a
lost race; under SERIALIZABLE the conflict is a serialization failure and
transactSerializable reruns the transaction.
Twelve doors each built their own RateLimitExceededError → 429: some with
no Retry-After, some with no retryAfterMs, the REST door with a prose body.
lib/rate-limit-response.ts is now the one shape — RATE_LIMITED plus
retryAfterMs, and a whole-second Retry-After that is never zero — with a
plain-Response twin for the Slack webhook and the SSE auth pre-checks. The
documents and TTS services keep their coded wrappers for the REST helpers
and job consumers but carry the refusal as `cause`, so their app doors
answer the same 429.
generateDevCompose emitted the backend tier with depends_on object-store but
defined no such service, a compose file docker refuses outright. The backend
seeds the deployment-default blob connection at boot and refuses every
upload without the store, so dev needs it: the same service as the stateful
stack, on the object-store-data dev volume DEV_VOLUME_NAMES already
pre-creates. The generator test now asserts every depends_on target exists.
The example still described the backend as an opt-in compose profile with
Convex behind a reversible BACKEND_UPSTREAM switch, named Convex-era
variables (POSTGRES_URL, INSTANCE_NAME, /metrics/convex, the env-sync name
cap) and the "convex image". The backend is always part of the stack and
Convex is retired: say what INSTANCE_SECRET actually derives, where
BACKEND_UPSTREAM and TALE_CONTROL_TOKEN really apply, and the current
metrics lanes and name rules.
Real-Postgres probes for the batch: a rival organization's vote is refused
opaquely and records nothing (the existing feedback probe now votes on a real
owned message); a reconnect renews the same Slack credential and a second
workspace gets its own label; twelve concurrent appends take twelve distinct
slots and the 0074 renumbering replays deterministically on a scratch table;
six doors refuse a spent budget in the one 429 shape with Retry-After.
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