Declare what every operation destroys, sends and reads from strangers - #230
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Retry validation and generated idempotency metadata are inconsistent, and one destructive journal path is misclassified.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Adds explicit operation metadata for destructive effects, external delivery, draft conditions, and untrusted content, enabling safer MCP policy decisions.
Changes:
- Defines and applies behavior/provenance traits across all operations.
- Generates corresponding OpenAPI and behavior-model metadata.
- Adds validation tripwire tests and CI enforcement.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
spec/hey.smithy |
Classifies operation behavior and provenance. |
spec/hey-traits.smithy |
Defines traits and validation rules. |
scripts/test-behavior-traits |
Tests validator tripwires. |
scripts/generate-behavior-model |
Emits new behavior fields. |
openapi.json |
Adds generated trait extensions. |
behavior-model.json |
Adds generated policy metadata. |
Makefile |
Integrates tripwire tests into checks. |
.github/workflows/smithy-verify.yml |
Runs tripwire tests in CI. |
AGENTS.md |
Documents trait conventions and workflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38d9298891
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review status at a2ad894
|
a2ad894 to
7eb30b7
Compare
7eb30b7 to
c323084
Compare
|
Review status at c323084 (2026-10-02)
|
c323084 to
d4d5bd6
Compare
Three explicit declarations on every operation, emitted into openapi.json as x-hey-* extensions and into behavior-model.json for consumers such as the MCP toolkit: - @heyDestructive (writes) -> destructive: a path that destroys data, or the caller's access to it, with no way back for the caller. - @heyOpenWorld (writes) -> open_world: the call can deliver mail, publish to HEY World, or send calendar cancellations. @heyDraftWhen -> draft_when names the request-body conditions under which a send saves a draft instead. - @heyUntrustedContent (all) -> untrusted_content: the response can carry text someone other than the caller wrote. EmitEachSelector validators make each declaration mandatory and forbid resending an open-world POST/PUT; scripts/test-behavior-traits breaks the model on purpose to prove they fire, and runs in make check and the Smithy CI job.
…asure destructive - behavior-model.json counted any @heyIdempotent as idempotent, so UpdateMessage, which opts out with natural: false, was advertised as safe to repeat. Only natural decides now; UpdateMessage is the one operation that changes. - HeyOpenWorldRetried now mirrors how every generator decides a resend: an open-world operation that does not say natural: false is refused when @idempotent, natural: true, or a GET/HEAD/PUT verb would resend it. - @heyDraftWhen needs at least one condition; an empty list holds vacuously. - UpdateJournalEntry with empty content destroys the entry: destructive. The tripwire test gains the dropped-opt-out and empty-conditions cases.
Main gained ListAddressableContacts (#231) after this branch's HeyUntrustedContentUndeclared validator was written, so the rebased spec failed smithy-validate. Its labels are the display names correspondents declared for themselves, the same content ListContacts and GetContact already mark untrusted.
d4d5bd6 to
8486588
Compare




Today no operation in
behavior-model.jsoncarries a destructive trait, and nothing tells a consumer which operations send mail or return text written by someone else. So the MCP toolkit (basecamp/mcp) guesses from action names (catalog.BridgeDestructive), hey-mcp-server hand-curatesempty_spamandempty_trash, and every HEY tool is annotatedopenWorldHint: false. That last one is wrong for sends, and it matters for the App Security review: private mail, plus sender-authored content reaching the agent, plus an outbound send, is the "lethal trifecta". The toolkit can only gate the send if it can tell which calls send.This PR has every operation declare three things, each classified from haystack's controllers rather than from the verb or the name.
The traits (
spec/hey-traits.smithy)behavior-model.json@heyDestructive(bool)x-hey-destructivedestructive(reads:false)@heyOpenWorld(bool)x-hey-open-worldopen_world(reads:false)@heyDraftWhen([...])x-hey-draft-whendraft_when: [{pointer, equals | not_equals}]@heyUntrustedContent(bool)x-hey-untrusted-contentuntrusted_contentNaming. basecamp-sdk has no equivalent to reuse. Its
scripts/gen-catalog/main.gorecords the destructive trait as a known gap ("That trait does not exist in the Smithy model yet"), and it has nothing for open-world or provenance.basecampSensitiveis a member-level redaction trait and a different concept. So these follow basecamp-sdk's conventions instead:<product>Xtrait →x-<product>-xextension → a flat behavior-model key. Thedestructivekey is exactly whatmcp/catalogand basecamp-sdk'sgen-catalogalready read as a tri-state. If basecamp-sdk adoptsbasecampDestructive,basecampOpenWorldandbasecampUntrustedContent, both SDKs emit the same keys.Tripwire.
EmitEachSelectorvalidators at severity DANGER make each declaration mandatory. An operation added without one failssmithy validate, and with itmake checkand the Smithy CI job. One more validator forbids resending an open-world operation, because a retry after an ambiguous first attempt could deliver twice. It follows the rule every generator uses to decide a resend:x-hey-idempotent.naturalwhen set, otherwise@readonlyor@idempotent, otherwise the verb. Withoutnatural: false, an open-world operation is refused if@idempotent,natural: true, or a GET/HEAD/PUT verb would resend it. DELETE is exempt because the cancellations HEY sends on a delete are keyed to the record it destroys, so a resend gets a 404.A validator whose selector stops matching passes silently. To catch that,
scripts/test-behavior-traits(make behavior-traits-test, incheck-mvp/check-fulland CI) breaks the model one declaration at a time and requires a refusal under the expected validator id and shape. It also checks that the unbroken model validates.generate-behavior-modelindependently refuses to emit an undeclared operation, so if a validator is edited away, a missing declaration still can't turn intofalse.@heyDraftWhenrequires at least one condition, because an empty list would hold vacuously and pass every send as a draft.One existing value changes.
generate-behavior-modelused to count any@heyIdempotentas idempotent, so UpdateMessage (natural: false, it delivers) was advertised asidempotent: true. It now readsnatural, and UpdateMessage is the only operation that flips. No generated SDK code changes: the generators readx-hey-idempotent.naturalfirst and ignore the new keys and extensions.Classification: 131 operations (76 writes, 55 reads)
Evidence is pinned to haystack
49cbde18.Destructive (13):
EmptySpam,EmptyTrash:Topic#deleted!on every thread (spam, trash).DeleteCalendarEvent,DeleteCalendarEventOccurrence:destroy/destroy!(event, occurrence). A shared event is removed for every member.DeleteHabit(L49),DeleteTimeTrack(L48),DeleteCalendarTodo(L31),DeleteSticky(L19): harddestroy.UpdateJournalEntry: empty content destroys the entry (Calendar::Days::JournalEntriesController#update).DeleteContactNote:note: nil, and the text is gone (L22).DeleteExtenzion:destroy_contactable_and_erect_tombstoneplus access teardown (L85).TrashPostings: judgment call, see below.PuntClearances: judgment call, see below.Open world (6):
CreateMessage,UpdateMessage,CreateReply: these deliver unless drafted (messages, replies).CreateMessagecan also publish to HEY World (L163). All three carrydraft_when.CreateBulkReply: always delivers (L18).DeleteCalendarEvent,DeleteCalendarEventOccurrence: when the caller organizes the event, these email a cancellation to every attendee who hasn't declined (notifier).Untrusted content (45): mailbox and box reads, topics, entries, messages, drafts, search, folders, collections-with-postings, contacts and clearances (and the clearance/contact writes that echo them), calendar periods and recordings, calendars, clips, the reply/forward/bulk-reply compose prefills, and the workflow stage page.
Not destructive, noted: these are reversible.
TrashTopicandDeleteDraft:trashed!, restorable for 30 days (restore, window).MarkPostingsSpam,MarkEntrySpam:MarkTopicHamundoes them. They do train rspamd.HideContact(DELETE) ↔RevealContact.DeleteBoxGroup: moves the group's mail back to the Imbox (L16).DeleteBoxDesignation: a rule you can recreate.UpdateMessage,UpdateContact, ...).Judgment calls worth a look
TrashPostingsis destructive. For JSON the server treats the removal decision as made. On a shared thread, that revokes the caller's own access (accesses.destroy_by) instead of trashing it (L15, L38), and the caller can't undo that. Non-shared threads go to the restorable trash. MCP's hint means "may", sotrue.PuntClearancesis destructive for the same reason. It trashes every pending sender's threads, but revokes access on shared ones (L12).gen-catalogcallsTrashRecording"genuinely destructive", and the toolkit's bridge treats atrashprefix as destructive. Once basecamp-sdk declares its own trait, the two SDKs should agree on this definition.draft_whenrequires/entry/scheduled_delivery≠"true"as well as/entry/status="drafted". A drafted entry with a schedule is still delivered at the scheduled hour with no further call (L19), so a gate that trustedstatusalone would wave through a timed send.untrusted_contentcalls are conservative:ListDraftsandGetMessageEdit: a reply draft carries the thread's subject and quoted text.ListCalendars: subscribed feeds and shared calendars are named by others.CreateContact,UpdateContact,RevealContact): they return a contact whose name and address its owner may have declared.false. The sharedRecordingschema's third-party fields (organizer,attendances,attached_entry) belong to theCalendar::Eventvariant only.How the toolkit should consume this (not changed here)
In
basecamp/mcp:Read the new keys in
catalog.behaviorTraits, tri-state likedestructive:OpenWorld *bool(open_world)UntrustedContent *bool(untrusted_content)DraftWhen []struct{ Pointer, Equals, NotEquals string }(draft_when, withnot_equalson the wire)Absent means undeclared. That covers basecamp-sdk today, which should keep its current behavior.
destructive: nothing new to read. Every HEY operation now declares it, soBridgeDestructivenever fires for HEY. hey-mcp-server must delete itsDestructiveActionsoverride forempty_spam/empty_trashwhen it vendors this model. The catalog loader refuses an override of a declared trait, by design. Some actions flip from bridged-trueto declared-false:delete_draft,delete_box_group,delete_box_designation,trash_topic,remove_postings_from_box_group.openWorldHint: set it per tool asAnyOpenWorld()over the served actions, instead of the hard-codedfalseingateway.BuildMCPServer. This puts it on messages, entries, bulk reply and calendar events.Delivery gate: in
dispatch, treat anopen_worldaction as a send unless everydraft_whencondition holds on the call's body.equalsholds when the value is present and equal;not_equalsholds when the value is absent or different. Hold sends to an explicit policy: off unless the operator enables sending (the way read-only filtering already trims writes), or behind a per-call confirmation or elicitation. Drafts pass, which keeps "the agent drafts, the human sends" frictionless.Provenance: mark
untrusted_contentresults as untrusted input before the model reads them, for example by delimiting the text and adding a_metaflag, and taint the session. Once a session is tainted, an open-world call needs the gate even where policy would otherwise allow it. That's the trifecta rule: untrusted content may be read, but it can't carry data out on its own.Verification
make -k checkon Linux: everything passes exceptcargo deny, which isn't installed on that host. CI's Rust job runscargo deny.scripts/test-behavior-traits: all eight cases, including the unbroken control.smithy-build, url-routes, fingerprint, coverage, Go, Rust, TypeScript, Kotlin, Swift): onlyopenapi.jsonandbehavior-model.jsonchange.