Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 39 additions & 41 deletions docs/decisions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,48 +83,46 @@ not access logs, so it is not proof of non-access.
**Decision.** The PostgREST Data API is not part of Helpthread's architecture. The
application reaches Postgres directly over the pooler (`DATABASE_URL`) and uses Supabase
Storage with the `service_role` key; no anon-key client exists anywhere in the codebase.
Migration 027 therefore enables RLS on every table and revokes the `anon`/`authenticated`
grants, including via `ALTER DEFAULT PRIVILEGES` so future tables created by the migrating
role do not arrive pre-granted. Neither half hardcodes `public`, because `PostgresDb`
supports a `schema` option; the revokes derive the schema from `'conversations'::regclass`
β€” deliberately *not* `current_schema()`, which resolves to search_path's first entry rather
Migration 027 enables RLS on every table and revokes the `anon`/`authenticated` grants,
including via `ALTER DEFAULT PRIVILEGES` so future tables created by the migrating role do
not arrive pre-granted. Neither half hardcodes `public`, because `PostgresDb` supports a
`schema` option; the revokes derive the schema from `'conversations'::regclass` β€”
deliberately *not* `current_schema()`, which resolves to search_path's first entry rather
than to the schema actually holding the tables, and so can diverge from the unqualified
`ALTER TABLE`s. (An earlier revision of this change used `current_schema()` and was caught
in review; the divergence fails silently, leaving the grants in place while the migration
reports success.) The same lockdown was applied directly to the production project ahead of
the migration landing, to close live exposure rather than wait on review.

Be precise about the limits of the `ALTER DEFAULT PRIVILEGES` layer, which is easy to
overrate: it *deletes a default-ACL entry* rather than installing a standing deny, and
without `FOR ROLE` it binds only to the role that ran the migration. It does not survive
Supabase re-running its stock bootstrap, and it does not touch defaults defined for other
roles such as `supabase_admin`. The durable protection is the standing rule below.

**Alternatives considered.** Enabling RLS alone was rejected: the grants would remain, so
a single future permissive policy would reopen everything. Revoking grants alone was
rejected for the mirror-image reason β€” anything that re-grants, including Supabase
re-running its stock bootstrap, restores them, and as noted above the default-privileges
layer does not prevent that. Writing real per-tenant RLS policies was rejected as solving a problem Helpthread
does not have: policies exist to make anon access safe, and there is no anon access to
make safe. Deny-by-default with no policies is the honest expression of that.

**Consequences.** The Supabase security advisor now reports `rls_enabled_no_policy` at
INFO level for every table. That is the intended end state, not an outstanding item.
Tables are owned by `postgres`, which bypasses RLS unless `FORCE ROW LEVEL SECURITY` is
set β€” deliberately not set β€” so the application is unaffected. A standing rule follows:
any migration adding a table must also enable RLS on it.

**This promotes an implicit deployment detail into an invariant: `DATABASE_URL` must
connect as the role that owns the tables.** It does today β€” the runbook has the operator
use the Supabase pooler string (role `postgres`) for both the app and `scripts/migrate.ts`,
so connecting role and owning role coincide by construction. But pointing `DATABASE_URL` at
a dedicated least-privilege role β€” a supported Supabase pattern β€” now breaks the engine, and
the two halves break differently: reads go quiet (RLS with no policies returns zero rows
instead of raising, so the symptom is an empty inbox rather than a database error) while
every write hard-errors with `new row violates row-level security policy`. The loud signal
arrives first, via inbound ingest. No test can catch the misconfiguration, because PGlite
also runs as the owner. Separately,
`splitStatements` in `src/db/migrate.ts` had to learn about dollar quoting, since
`ALTER TABLE`s, silently leaving the grants in place while the migration reports success.
The same lockdown was applied directly to the production project ahead of the migration
landing, to close live exposure rather than wait on review.

The `ALTER DEFAULT PRIVILEGES` layer is easy to overrate: it *deletes a default-ACL entry*
rather than installing a standing deny, and without `FOR ROLE` it binds only to the role
that ran the migration. It does not survive Supabase re-running its stock bootstrap, and it
does not touch defaults defined for other roles such as `supabase_admin`. The durable
protection is the standing rule below.

**Alternatives considered.** Enabling RLS alone was rejected: the grants would remain, so a
single future permissive policy would reopen everything. Revoking grants alone was rejected
for the mirror-image reason β€” anything that re-grants, including Supabase's stock bootstrap,
restores them. Writing real per-tenant RLS policies was rejected as solving a problem
Helpthread does not have: policies exist to make anon access safe, and there is no anon
access to make safe. Deny-by-default with no policies is the honest expression of that.

**Consequences.** The Supabase security advisor now reports `rls_enabled_no_policy` at INFO
level for every table β€” the intended end state, not an outstanding item. Tables are owned by
`postgres`, which bypasses RLS unless `FORCE ROW LEVEL SECURITY` is set (deliberately not
set), so the application is unaffected. **Standing rule: any migration adding a table must
also enable RLS on it.**

**This promotes an implicit deployment detail into an invariant: `DATABASE_URL` must connect
as the role that owns the tables.** It does today β€” the runbook has the operator use the
Supabase pooler string (role `postgres`) for both the app and `scripts/migrate.ts`. But
pointing `DATABASE_URL` at a dedicated least-privilege role β€” a supported Supabase pattern β€”
now breaks the engine, and the two halves break differently: reads go quiet (RLS with no
policies returns zero rows rather than raising, so the symptom is an empty inbox, not a
database error) while every write hard-errors with `new row violates row-level security
policy`. The loud signal arrives first, via inbound ingest. No test can catch the
misconfiguration, because PGlite also runs as the owner.

Separately, `splitStatements` in `src/db/migrate.ts` had to learn about dollar quoting, since
migration 027's role guard is a `DO $$ ... $$` block whose body contains semicolons.

**Supersedes.** Nothing.
Expand Down
34 changes: 15 additions & 19 deletions docs/modules/assistants-and-drafts.md
Original file line number Diff line number Diff line change
Expand Up @@ -156,12 +156,11 @@ looked at anything.

## The Agent approval flow

Everything past this point is an **Agent** action β€” the core Helpthread
inbox UI does this for a human clicking "approve" or "discard," and it
consumes exactly the same API, so these are also the calls a module author
would use to build their own review tooling or to understand what the UI is
doing. All three require `Authorization: Bearer $HELPTHREAD_API_TOKEN` +
`X-Helpthread-Agent-Id: <acting Agent's uuid>` β€” missing either is `401`.
Everything past this point is an **Agent** action. The core inbox UI does this for a
human clicking "approve" or "discard" and consumes exactly the same API, so these are
also the calls to build your own review tooling against. All three require
`Authorization: Bearer $HELPTHREAD_API_TOKEN` + `X-Helpthread-Agent-Id: <acting Agent's
uuid>` β€” missing either is `401`.
Comment on lines +159 to +163

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

πŸ—„οΈ Data Integrity & Integration | 🟠 Major | ⚑ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate files =="
git ls-files | rg '(^|/)(assistants-and-drafts\.md|agents-and-auth\.md|agent-inbox-v1\.md)$' || true

echo
echo "== target excerpts =="
sed -n '145,170p' docs/modules/assistants-and-drafts.md 2>/dev/null || true
echo "---"
sed -n '375,402p' specs/auth/agents-and-auth.md 2>/dev/null || true
echo "---"
rg -n "draft|approve|discard|X-Helpthread-Agent-Id|Agent-Id|agents-and-auth|agents/" docs/modules/assistants-and-drafts.md specs/auth/agents-and-auth.md specs -S || true

Repository: Helpthread/helpthread

Length of output: 26163


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== route/header occurrences =="
rg -n "approve|discard|draft" docs/modules/assistants-and-drafts.md specs auth 2>/dev/null || true

echo
echo "== focused spec/header context =="
for f in docs/modules/assistants-and-drafts.md specs/auth/agents-and-auth.md specs/api/agent-inbox-v1.md; do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    rg -C 5 "approve|discard|draft|X-Helpthread-Agent-Id|Authorization|Bearer" "$f" || true
  fi
done

Repository: Helpthread/helpthread

Length of output: 37305


Align acting-Agent header requirements for draft-review API.

docs/modules/assistants-and-drafts.md documents X-Helpthread-Agent-Id for GET /api/v1/drafts, POST .../approve, and POST .../discard, but specs/auth/agents-and-auth.md Β§8 only lists /agents/*, /auth/me, and PUT /conversations/{id}/assignee. Make these spec sites agree: add the draft-review endpoints to the per-endpoint header-required set, or remove the header requirement from the guide and update the spec accordingly.

🧰 Tools
πŸͺ› LanguageTool

[style] ~160-~160: β€˜exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...ing "approve" or "discard" and consumes exactly the same API, so these are also the calls to bui...

(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)

πŸ“ Affects 2 files
  • docs/modules/assistants-and-drafts.md#L159-L163 (this comment)
  • specs/auth/agents-and-auth.md#L387-L396
πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/modules/assistants-and-drafts.md` around lines 159 - 163, The
acting-Agent header requirements are inconsistent between the draft-review guide
and authentication spec. Update specs/auth/agents-and-auth.md at lines 387-396
to include GET /api/v1/drafts, POST .../approve, and POST .../discard in the
per-endpoint header-required set, preserving the existing Authorization and
X-Helpthread-Agent-Id requirements documented in
docs/modules/assistants-and-drafts.md at lines 159-163; no direct change is
needed in the guide.


**List the review queue** (every conversation's drafts, across the whole
deployment, newest first):
Expand Down Expand Up @@ -193,19 +192,16 @@ curl -X POST "$BASE_URL/api/v1/drafts/$THREAD_ID/approve" \
-d '{"bodyText": "Edited reply text..."}'
```

Approval is a state transition, not a resend: it mints the reply's
threading token and Message-ID, derives the envelope (recipient, subject,
`In-Reply-To`/`References`) exactly the way a normal Agent reply does, and
hands off to the same delivery worker β€” the mail that goes out is
equivalent to what a human typing the same body and hitting reply would
send. Fires `draft.resolved` immediately (`{ threadId, resolution:
'approved', edited }`), and `conversation.reply_sent` once delivery actually
confirms `sent` (not at accept-for-send time β€” modules reacting to "we
replied" get truth, not intent).

Refused `404` (indistinguishable-from-nonexistent) if the conversation is
missing/soft-deleted or `$THREAD_ID` doesn't name a draft currently
`awaiting_review`; refused `409 conflict` if the conversation is `spam`.
Approval is a state transition, not a resend: it mints the reply's threading token and
Message-ID, derives the envelope (recipient, subject, `In-Reply-To`/`References`) exactly
as a normal Agent reply does, and hands off to the same delivery worker. Fires
`draft.resolved` immediately (`{ threadId, resolution: 'approved', edited }`), and
`conversation.reply_sent` only once delivery confirms `sent` β€” not at accept-for-send time,
so modules reacting to "we replied" get truth, not intent.

Refused `404` (indistinguishable-from-nonexistent) if the conversation is missing or
soft-deleted, or `$THREAD_ID` doesn't name a draft currently `awaiting_review`; refused
`409 conflict` if the conversation is `spam`.

**Discard** (no send, row kept for audit):

Expand Down
18 changes: 2 additions & 16 deletions docs/modules/webhooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -245,22 +245,8 @@ if (!result.valid) {
const event = JSON.parse(rawBody)
```

Reject a stale `t` (the 5-minute default above matches the spec's
recommendation) to close a replay window β€” an attacker who captures one
valid delivery cannot resend it indefinitely.

> **Verified, not just written.** This exact function was checked before
> landing in this doc, and re-checked after the hex-validation fix above:
> (1) signed with the engine's own `signWebhookPayload`
> (`src/webhooks/delivery.ts`) and verified successfully by this function,
> byte-for-byte, with a real HMAC computed both ways; (2) cross-checked
> against the independent verifier in `module-draft-assistant/src/verify.ts`
> (the reference module referenced throughout this guide) β€” both verifiers
> agree on the same signed payload; (3) correctly rejects a wrong secret, a
> tampered body, a stale timestamp, and a signature with trailing non-hex
> garbage appended after a valid-length prefix (`Buffer.from(str, 'hex')`
> otherwise silently truncates instead of rejecting it). The throwaway
> script that ran these checks exited `0`.
Reject a stale `t` β€” the 5-minute default above β€” to close a replay window: an
attacker who captures one valid delivery cannot resend it indefinitely.

## Delivery guarantees

Expand Down
Loading