jpmschweitzer/core-api
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 10 Wiki Activity

feat(scribe): accept a feedback message to tower-control with no sender (T-15)

Browse Source 
workspace D-70 ("`feedback` needs no address"): the failure channel must
not depend on the thing most likely to break. `scribe_messages.from_identity`
becomes nullable (migration 010); `POST /scribe/messages` accepts a missing
`X-Office-Identity` for exactly one shape — an office caller, `to` is
`["tower-control"]` alone, and `tags` contains `feedback` — stored with
`from_identity` null and `from_consumer` the caller's consumer. Every other
send without an identity is still 400 `identity_required`, enforced in
ScribeService.send_message rather than the caller dependency, since the
decision needs the request body. `MessageResponse.from_identity` is now
optional so mailbox/history/find/message_id answer such a message with a
null sender. Migration 010's downgrade refuses outright while any
sender-less row exists rather than inventing one.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
jpmschweitzer
2026-10-19 23:37:46 +02:00
co-authored by Claude Sonnet 5
parent 8b30e73661
commit 8ec89336df
7 changed files with 367 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
alembic/versions/20261019_1900_010_make_scribe_messages_from_identity_nullable.py
+67
View File
@@ -0,0 +1,67 @@
"""A feedback message to tower-control may have no sender (core-api T-15)
Revision ID: 010
Revises: 009
Create Date: 2026-10-19
workspace D-70 ("`feedback` needs no address"): the one shape that may ship
with no sender is an Office caller's `feedback` to `tower-control` alone.
`scribe_messages.from_identity` becomes nullable to hold it — the service
layer (core-api T-15) is what actually narrows which sends may leave it
null; this migration only makes the column able to carry that.
No data step: every existing row already has a sender.
"""
from typing import Sequence, Union
from alembic import op
import sqlalchemy as sa
# revision identifiers, used by Alembic.
revision: str = "010"
down_revision: Union[str, None] = "009"
branch_labels: Union[str, Sequence[str], None] = None
depends_on: Union[str, Sequence[str], None] = None
def upgrade() -> None:
"""Drop the NOT NULL constraint. No data step needed."""
op.alter_column(
"scribe_messages",
"from_identity",
existing_type=sa.String(length=63),
nullable=True,
)
def downgrade() -> None:
"""Refuse outright if any message was stored with no sender — there is
no real sender to invent, and fabricating one would misattribute a
retained message (migration 006: nothing deletes a message). Delete or
backfill those rows by hand first if this downgrade is genuinely
wanted; `ALTER COLUMN ... SET NOT NULL` would refuse anyway once nulls
exist, but the explicit check names the reason rather than surfacing it
as a generic constraint-violation error."""
op.execute(
"""
DO $$
DECLARE
sender_less_count integer;
BEGIN
SELECT count(*) INTO sender_less_count
FROM scribe_messages WHERE from_identity IS NULL;
IF sender_less_count > 0 THEN
RAISE EXCEPTION
'migration 010 downgrade refused: % scribe_messages row(s) have no from_identity (core-api T-15 / D-70) - delete or backfill them before downgrading',
sender_less_count;
END IF;
END $$;
"""
)
op.alter_column(
"scribe_messages",
"from_identity",
existing_type=sa.String(length=63),
nullable=False,
)
src/domains/scribe/controller.py
+13 -2
View File
@@ -6,7 +6,11 @@ The Scribe's messaging verbs (dossier core-api T-5): register, list
a mailbox entry and look one up by message number (core-api T-13/T-9).
Every route is behind an Office or user caller (T-1/T-4); every query
filters on the caller's tenant (D-73) — an address or message in another
tenant answers exactly like one that does not exist.
tenant answers exactly like one that does not exist. `send` is the one
write that does not require an identity at the dependency layer: core-api
T-15 / D-70 lets an office caller's `feedback` to `tower-control` alone go
with no sender, so ScribeService.send_message enforces identity_required
itself, after seeing the body.
"""
from typing import Optional
@@ -35,6 +39,13 @@ logger = get_logger(__name__)
require_scribe_reader = require_caller("office", "user")
require_scribe_writer = require_caller("office", "user", identity_required=True)
# `/messages` POST is not `identity_required` at this layer: core-api T-15 /
# workspace D-70 lets one shape through with no identity (an office
# caller's `feedback` to `tower-control` alone), and that decision needs
# the request body, which a caller dependency never sees. ScribeService
# .send_message is the single place that enforces identity_required for
# every other shape — see its docstring.
require_scribe_sender = require_caller("office", "user")
def _address_response(address, owned: bool) -> AddressResponse:
@@ -127,7 +138,7 @@ class ScribeController(BaseController):
)
async def send_message(
data: MessageSendRequest,
caller: Caller = Depends(require_scribe_writer),
caller: Caller = Depends(require_scribe_sender),
session: AsyncSession = Depends(get_async_session),
):
try:
src/domains/scribe/models.py
+6 -1
View File
@@ -90,7 +90,12 @@ class ScribeMessage(Base):
String(63), ForeignKey("tenants.id", ondelete="RESTRICT"), nullable=False, index=True
)
topics: Mapped[List[str]] = mapped_column(ARRAY(Text), nullable=False, default=list)
from_identity: Mapped[str] = mapped_column(String(63), nullable=False)
# Nullable since migration 010 (core-api T-15 / D-70): a `feedback`
# message to `tower-control` alone may ship with no sender — the one
# shape the failure channel must not depend on an address for. The
# service layer (ScribeService._is_exempt_feedback) is what actually
# narrows which sends may leave this null.
from_identity: Mapped[Optional[str]] = mapped_column(String(63), nullable=True)
from_consumer: Mapped[Optional[str]] = mapped_column(String(63), nullable=True)
subject: Mapped[Optional[str]] = mapped_column(String(500), nullable=True)
body: Mapped[str] = mapped_column(Text, nullable=False)
src/domains/scribe/schemas.py
+4 -1
View File
@@ -49,7 +49,10 @@ class MessageSendRequest(BaseSchema):
class MessageResponse(BaseSchema):
id: int
topics: List[str]
from_identity: str
# Optional since core-api T-15 / D-70: a sender-less `feedback` message
# to `tower-control` answers with from_identity null, everywhere this
# schema is used (mailbox, history, find, message_id lookup).
from_identity: Optional[str]
from_consumer: Optional[str]
subject: Optional[str]
body: str
src/domains/scribe/service.py
+46 -10
View File
@@ -62,6 +62,19 @@ class ScribeService:
except TenantError as error:
raise ScribeError(error.code, error.status_code) from error
def _is_exempt_feedback(self, caller: Caller, recipients: List[str], tags: List[str]) -> bool:
"""core-api T-15 / workspace D-70 ("`feedback` needs no address"):
the one shape that may ship with no sender is an office caller's
`feedback` to `tower-control` alone — nothing broader. `recipients`
and `tags` are the de-duplicated lists `send_message` already
computed, so a second or different recipient, or a message missing
the `feedback` tag, never qualifies."""
return (
caller.kind == "office"
and recipients == ["tower-control"]
and "feedback" in tags
)
def _validate_address_name(self, name: str) -> None:
"""Beyond the schema's regex (shape only): not `group-*`, not reserved.
Mirrors src/domains/auth/callers.py's `_validate_identity` rule —
@@ -142,14 +155,38 @@ class ScribeService:
async def send_message(
self, session: AsyncSession, data: MessageSendRequest, caller: Caller
) -> ScribeMessage:
"""`send` — `from` is the caller's own identity, never the body."""
"""`send` — `from` is the caller's own identity, never the body.
core-api T-15 / workspace D-70: `require_scribe_sender` (the
controller's dependency for this route) no longer enforces
`identity_required` itself, because that decision depends on the
body (`to`, `tags`), which a caller dependency cannot see. This
method is therefore the single place that enforces it: a missing
identity (`caller.identity == ""`, which only an office caller with
no `X-Office-Identity` header can produce) is allowed through
exactly for `_is_exempt_feedback`'s shape, and is still
`identity_required` for every other send — the tenant check stays
on this path too (no sender is not no tenant)."""
tenant = await self._require_tenant(session, caller)
from_address = await self._get_address(session, tenant, caller.identity)
if from_address is None or from_address.owner_consumer != caller.consumer:
raise ScribeError("identity_not_owned", 403)
recipients = list(dict.fromkeys(data.to)) # de-dup, keep order
tags = list(dict.fromkeys(data.tags))
no_identity = caller.identity == ""
exempt = no_identity and self._is_exempt_feedback(caller, recipients, tags)
if no_identity and not exempt:
raise ScribeError("identity_required", 400)
if exempt:
from_identity = None
from_consumer = caller.consumer
else:
from_address = await self._get_address(session, tenant, caller.identity)
if from_address is None or from_address.owner_consumer != caller.consumer:
raise ScribeError("identity_not_owned", 403)
from_identity = caller.identity
from_consumer = caller.consumer
found_stmt = select(ScribeAddress.name).where(
ScribeAddress.owner_tenant == tenant, ScribeAddress.name.in_(recipients)
)
@@ -163,13 +200,11 @@ class ScribeService:
if reply_target is None or reply_target.owner_tenant != tenant:
raise ScribeError("not_found", 404)
tags = list(dict.fromkeys(data.tags))
message = ScribeMessage(
owner_tenant=tenant,
topics=recipients,
from_identity=caller.identity,
from_consumer=caller.consumer,
from_identity=from_identity,
from_consumer=from_consumer,
subject=data.subject,
body=data.body,
tags=tags,
@@ -187,7 +222,8 @@ class ScribeService:
await session.refresh(message)
logger.info(
f"Scribe message sent: id={message.id} from={caller.identity} to={recipients}"
f"Scribe message sent: id={message.id} from={from_identity} "
f"from_consumer={from_consumer} to={recipients}"
)
return message
tests/test_scribe.py
+128
View File
@@ -7,6 +7,8 @@ overridden to the test database for every test, building a fresh
engine/session per call because TestClient drives the ASGI app on a
different event loop than pytest-asyncio's fixture setup.
"""
from unittest.mock import patch
import pytest
from fastapi.testclient import TestClient
@@ -68,6 +70,17 @@ def office_headers(identity: str, consumer: str, tenant: str = TENANT) -> dict:
}
def office_headers_no_identity(consumer: str, tenant: str = TENANT) -> dict:
"""No `X-Office-Identity` at all — core-api T-15's one exemption (an
office caller's `feedback` to `tower-control` alone) is reachable only
through this shape; every other write still needs the header."""
return {
"Authorization": f"Bearer {OFFICE_KEY}",
"X-Office-Consumer": consumer,
"X-Office-Tenant": tenant,
}
def register(client, identity: str, consumer: str, tenant: str = TENANT):
return client.post(
"/scribe/addresses",
@@ -403,6 +416,121 @@ class TestSendValidation:
assert reply.json()["detail"]["code"] == "not_found"
# =============================================================================
# `feedback` needs no address (core-api T-15 / workspace D-70): the one
# shape that may ship with no sender is an office caller's `feedback` to
# `tower-control` alone — nothing broader.
# =============================================================================
class TestFeedbackWithoutIdentity:
def test_feedback_to_tower_control_alone_with_no_identity_succeeds(self, client):
register(client, "tower-control", "office-gemini")
sent = client.post(
"/scribe/messages",
json={
"to": ["tower-control"],
"body": "the find verb has no limit guard",
"tags": ["feedback"],
},
headers=office_headers_no_identity("office-claude"),
)
assert sent.status_code == 201
message = sent.json()
assert message["from_identity"] is None
assert message["from_consumer"] == "office-claude"
assert message["topics"] == ["tower-control"]
assert message["tags"] == ["feedback"]
mailbox = client.get(
"/scribe/mailbox", headers=office_headers("tower-control", "office-gemini")
).json()
assert mailbox["total"] == 1
assert mailbox["entries"][0]["message"]["from_identity"] is None
assert mailbox["entries"][0]["message"]["id"] == message["id"]
history = client.get(
"/scribe/addresses/tower-control/history",
headers=office_headers("tower-control", "office-gemini"),
).json()
assert history["messages"][0]["from_identity"] is None
found = client.get(
"/scribe/messages",
params={"q": "limit"},
headers=office_headers("tower-control", "office-gemini"),
).json()
assert found["total"] == 1
assert found["entries"][0]["message"]["from_identity"] is None
def test_feedback_with_no_identity_still_needs_a_known_tenant(self, client):
"""No sender is not no tenant (the brief): `_require_tenant` keeps
running on this path, ahead of the identity decision."""
response = client.post(
"/scribe/messages",
json={"to": ["tower-control"], "body": "x", "tags": ["feedback"]},
headers=office_headers_no_identity("office-claude", tenant="no-such-tenant"),
)
assert response.status_code == 403
assert response.json()["detail"]["code"] == "tenant_unknown"
def test_feedback_with_no_identity_still_needs_a_granted_consumer(self, client):
response = client.post(
"/scribe/messages",
json={"to": ["tower-control"], "body": "x", "tags": ["feedback"]},
headers=office_headers_no_identity("office-ungranted"),
)
assert response.status_code == 403
assert response.json()["detail"]["code"] == "consumer_not_granted"
@pytest.mark.parametrize(
"to,tags",
[
(["tower-control", "jeroen"], ["feedback"]), # a second recipient
(["jeroen"], ["feedback"]), # another recipient, not tower-control
(["tower-control"], []), # no feedback tag
(["tower-control"], ["urgent"]), # tagged, but not feedback
],
ids=[
"second-recipient",
"another-recipient",
"no-feedback-tag",
"tagged-but-not-feedback",
],
)
def test_every_broader_shape_with_no_identity_is_still_400(self, client, to, tags):
response = client.post(
"/scribe/messages",
json={"to": to, "body": "x", "tags": tags},
headers=office_headers_no_identity("office-claude"),
)
assert response.status_code == 400
assert response.json()["detail"]["code"] == "identity_required"
def test_user_kind_caller_gains_nothing_from_the_exemption(self, client):
"""A user-kind caller's identity is never empty (callers.py always
falls back to a name, never ""), so it never reaches the
no-identity branch at all: sending this exact shape is unaffected
by T-15 and still needs an owned address, same as before. Not 400
identity_required — that code names an office caller resolved with
none; this is 403 identity_not_owned, because "jeroen" was never
registered under this (consumer-less) user identity."""
register(client, "tower-control", "office-gemini")
with patch("src.domains.auth.callers.get_current_user") as mock_get_current_user:
mock_get_current_user.return_value = {
"preferred_username": "jeroen",
"tatlock_user": TENANT,
}
response = client.post(
"/scribe/messages",
json={"to": ["tower-control"], "body": "x", "tags": ["feedback"]},
headers={"Authorization": "Bearer some-jwt"},
)
assert response.status_code == 403
assert response.json()["detail"]["code"] == "identity_not_owned"
# =============================================================================
# Isolation (dossier verification #3): the hard tenant boundary, both
# directions — every cross-tenant answer is indistinguishable from "does
tests/test_scribe_migration.py
+103 -1
View File
@@ -1,5 +1,5 @@
"""
Tests for Scribe migrations 006 and 009.
Tests for Scribe migrations 006, 009 and 010.
Runs against `core_api_test` only (see conftest.py — skip when
TEST_DATABASE_URL is unset, hard failure if it resolves to `core_api`).
@@ -27,6 +27,57 @@ async def _table_exists(url: str, table: str) -> bool:
await engine.dispose()
async def _is_nullable(url: str, table: str, column: str) -> bool:
asyncpg_url = url.replace("postgresql://", "postgresql+asyncpg://", 1)
engine = create_async_engine(asyncpg_url, poolclass=NullPool)
try:
async with engine.connect() as conn:
result = await conn.execute(
text(
"SELECT is_nullable FROM information_schema.columns "
"WHERE table_name = :table AND column_name = :column"
),
{"table": table, "column": column},
)
return result.scalar() == "YES"
finally:
await engine.dispose()
async def _clear_scribe_messages(url: str) -> None:
"""Truncate just the two row-bearing Scribe tables that 010's downgrade
inspects — never `scribe_addresses`/`tenant_consumers`, which other
modules' seeded grants depend on. Self-contained so this file's
row-level checks never depend on what another test module's data left
behind (there is no FK from either table to `scribe_addresses`)."""
asyncpg_url = url.replace("postgresql://", "postgresql+asyncpg://", 1)
engine = create_async_engine(asyncpg_url, poolclass=NullPool)
try:
async with engine.begin() as conn:
await conn.execute(text("TRUNCATE scribe_mailbox_entries, scribe_messages"))
finally:
await engine.dispose()
async def _insert_sender_less_message(url: str) -> None:
"""Insert one scribe_messages row with `from_identity` NULL directly —
bypassing the API, which (even under T-15's exemption) always exists
for the one shape it allows; this proves the migration's own
constraint/refusal, not the service's."""
asyncpg_url = url.replace("postgresql://", "postgresql+asyncpg://", 1)
engine = create_async_engine(asyncpg_url, poolclass=NullPool)
try:
async with engine.begin() as conn:
await conn.execute(
text(
"INSERT INTO scribe_messages (owner_tenant, topics, from_identity, body) "
"VALUES ('jpmschweitzer', ARRAY['tower-control'], NULL, 'no sender')"
)
)
finally:
await engine.dispose()
async def _columns(url: str, table: str) -> set:
asyncpg_url = url.replace("postgresql://", "postgresql+asyncpg://", 1)
engine = create_async_engine(asyncpg_url, poolclass=NullPool)
@@ -108,3 +159,54 @@ class TestMigration009RoundTrip:
assert up.returncode == 0, up.stderr
columns_at_head = await _columns(test_database_url, "scribe_mailbox_entries")
assert {"archived_at", "trashed_at"} <= columns_at_head
class TestMigration010RoundTrip:
"""Migration 010 (core-api T-15 / D-70) must upgrade and downgrade
cleanly — dossier verification #2: 009 -> 010 -> 009 -> 010 — and its
downgrade must name a decision about a sender-less row rather than
silently inventing one (the brief: refusing is acceptable, fabricating
a sender is not). `_clear_scribe_messages` scopes each test to its own
row state rather than trusting what another test module left behind."""
@pytest.mark.asyncio
async def test_upgrade_makes_from_identity_nullable(self, test_database_url):
result = run_alembic(test_database_url, "upgrade", "head")
assert result.returncode == 0, result.stderr
assert await _is_nullable(test_database_url, "scribe_messages", "from_identity")
@pytest.mark.asyncio
async def test_downgrade_then_upgrade_round_trips_cleanly(self, test_database_url):
baseline = run_alembic(test_database_url, "upgrade", "head")
assert baseline.returncode == 0, baseline.stderr
await _clear_scribe_messages(test_database_url) # no sender-less row in the way
down = run_alembic(test_database_url, "downgrade", "009")
assert down.returncode == 0, down.stderr
assert not await _is_nullable(test_database_url, "scribe_messages", "from_identity")
# Leave the schema at head (010 is head) for the rest of the suite.
up = run_alembic(test_database_url, "upgrade", "head")
assert up.returncode == 0, up.stderr
assert await _is_nullable(test_database_url, "scribe_messages", "from_identity")
@pytest.mark.asyncio
async def test_downgrade_refuses_while_a_sender_less_row_exists(self, test_database_url):
"""The chosen behaviour, named: the downgrade does not run, and the
column stays nullable — nothing silently invents a sender for the
row it found."""
baseline = run_alembic(test_database_url, "upgrade", "head")
assert baseline.returncode == 0, baseline.stderr
await _clear_scribe_messages(test_database_url)
await _insert_sender_less_message(test_database_url)
down = run_alembic(test_database_url, "downgrade", "009")
assert down.returncode != 0
assert "migration 010 downgrade refused" in (down.stdout + down.stderr)
assert await _is_nullable(test_database_url, "scribe_messages", "from_identity")
# Leave the schema at head, clean, for the rest of the suite.
await _clear_scribe_messages(test_database_url)
up = run_alembic(test_database_url, "upgrade", "head")
assert up.returncode == 0, up.stderr