Tranche G: conversation attribution — report, gates, read-switch, write-deny - #1432
Draft
ptone wants to merge 114 commits into
Draft
Tranche G: conversation attribution — report, gates, read-switch, write-deny#1432ptone wants to merge 114 commits into
ptone wants to merge 114 commits into
Conversation
Read-only command that scans all messages and reports conversation attribution completeness with three distinct unattributed buckets: - backfillable: both principals are valid UUIDs, key derivation succeeds - non-UUID principal: at least one principal fails uuid.Parse (federated identities, slugs); permanently unattributable without DEF-32 - unresolvable: both principals are UUIDs but derivation still fails Non-zero non-UUID principal or unresolvable counts are declared flip-blocking in the command output itself. Non-UUID examples enumerate the offending principal IDs for diagnosis. Uses production DeriveConversationKey — no reimplementation. Does not import or reference DivergenceMetrics. Tests: - Mutation guard: seeds messages, runs report, asserts no row changed - DivergenceMetrics dependency guard: parses source, asserts no reference - Production derivation guard: asserts source calls messaging.DeriveConversationKey - Bucket classification, flip-blocking output, unresolvable examples, multi-project aggregation, empty database AC-G-1, AC-G-2, AC-G-3, AC-G-10.
G1-a: Add broadcast bucket. Broadcasts with NULL conversation_id are
now a separate flip-blocking bucket ('unattributed — broadcast'). The
backfill service skips broadcasts (backfill.go:127), so they are never
attributed. All three read-switch sites (handlers_messages.go:70, :259,
handlers_chat_v2.go:1782) scope by ConversationID with no broadcast-
specific alternative path — broadcasts become invisible at the flip.
G1-b: Reconciliation check. On all-projects runs, compare the report's
unattributed total against CountUnbackfilledMessages(ctx, ""). Print a
loud RECONCILIATION MISMATCH line if they disagree, and treat the
mismatch as flip-blocking.
G1-c: Behavioral production derivation test. Table of KeyInputs with
known-pass and known-fail cases run through the classifier; asserts
exact bucket placement. Mutate/fail/revert/pass proven.
…aveat G4-a (DEF-58): Add negative compile-time gate asserting brokerIdentityImpl satisfies neither UserIdentity nor AgentIdentity. Document the intentional empty-SenderID comparison in the broker delivery path. G4-b (DEF-79): Add PathRecorder context-keyed trace mechanism and instrument handleAgentMessage with 12 ordered checkpoints. TestDEF79_ProductionPathTrace sends a user→agent message through the full HTTP handler and asserts the step sequence; removing any step from the production code fails the test. G4-c (DEF-80): Add unbackfilled_blind_spot caveat to the divergence board explicitly stating that messages with empty ConversationID (pre-dual-write) are invisible to the consistency check and a clean board is not evidence of clean data.
…veat G4-d: List every untraced path (agent-scoped route, agent→agent, broker-inbound, group fan-out, managed-agent, outbound) in a comment next to expectedPathSteps so a green result is not mistaken for full path coverage. G4-e: Add sampling_window caveat to the divergence board disclosing the 50/25-row lookup limits in CheckConversationConsistency — the mismatch count is a lower bound on a sample, not a census.
Remove the channel+thread fallback at all three read sites when the ConversationReadSwitch is ON. Unresolved conversations now return HTTP 409 with machine-readable code "conversation_not_resolved" instead of silently falling back to the legacy filter. Fix the DM key parse bug: a key with a part count other than 5 now returns 409 with code "invalid_dm_key" (defense-in-depth; validDMKey's regex already rejects non-5-part keys at the HTTP layer with 400). Sites modified: - S1: handleConversationHistory (handlers_chat_v2.go) - S2: handleMessages (handlers_messages.go) - S3: handleAgentMessages (handlers_messages.go) Switch-OFF behaviour is unchanged. Agent lookup failures at S2 still skip the conversation path per R-9 discipline. AC-G3-1 through AC-G3-5 covered by tests in handlers_read_switch_test.go.
…r (G3-d/e) G3-d: Carry Channel:"web" into the switch-on filter at handleConversationHistory. Without this, flipping the switch silently drops the channel constraint and widens visibility to messages from non-web surfaces (discord, telegram) sharing the same conversation_id. Widening is the direction we cannot take back. Test proves discord messages in the same conversation are excluded. G3-e: Add SwitchBypassCounter (messaging.SwitchBypassMetrics) measuring switch coverage separately from migration readiness (DivergenceMetrics). Five reason labels, each with a test: - slug_param: S2, agent param is a slug (uuid.Parse fails) - agent_not_found: S2, agent UUID not in store - non_dm_key: S2, no agent param → no DM key to derive - wcs_nil: S1, webChatStore nil for non-DM key (early return) - non_web_channel: S3, channel not web/"" and no thread_id
When threadID is present but agent.ProjectID is empty (or nil UUID), the handler previously fell through to the DM branch — silently serving DM-scoped results for a thread query. This is a wrong-answer-with-200, the exact failure class Tranche G exists to remove. Make threadID the primary discriminator in S3's switch-on block: - threadID present + no project → 409 with thread_project_required - threadID present + has project → thread resolution (existing path) - no threadID + web/empty channel → DM resolution (existing path) - no threadID + non-web channel → bypass counter (existing path) The check covers both empty string and uuid.Nil.String() since the ent store always returns a UUID string for ProjectID.
The bug was threadID!="" with empty ProjectID falling through to the DM branch. The comment described the opposite, making it impossible for the next reader to reconstruct why the guard exists.
Reverse the B10 contract on the write path: conversation key derivation and resolution failures now deny the write (message not persisted, not published) instead of logging and continuing. Producer changes: - ResolveOrCreateDMConversation: returns (*ConversationResult, error) - ResolveOrCreateThreadConversation: returns (*ConversationResult, error) - ResolveOrCreateConversationByKey: returns (*ConversationResult, error) All nil-return points now return (nil, error) with descriptive messages. Consumer changes (21 swallow points flipped to denials): - handlers_agent_messaging.go: derive/resolve errors → 400/500 - handlers_broker_inbound.go: resolve errors → 500 - handlers_chat_v2.go: resolve errors → 500, return "" - messagebroker.go: resolve errors → log + return (no HTTP) - notifications.go: resolve errors → log + return Two deliberate exceptions preserved: - EnsureParticipant failure stays non-fatal (listing concern, not access) - Federated subscriber UUID parse failure still skips (not denied) ValidateAttributed (previously unreachable behind nil guards) now runs unconditionally on all three handler paths. Test fixture updates: - Non-UUID sender/recipient IDs replaced with tid() UUIDs - Non-canonical DM keys replaced with DMConversationKey() derivation - Topics given conversation_ids via setTopicConversationID helper Acceptance tests added: AC-G2-1 through AC-G2-5.
…al counters G2-f: All 25 write-path denial sites are now gated behind ConversationWriteDenySwitch (defaults OFF). When OFF, behavior is byte-for-byte identical to the base scion/tranche-g branch (B10 contract: log-and-continue). When ON, derivation/resolution failures deny the write (G2 contract). G2-g: Each denial site increments WriteDenialMetrics with a site label (e.g. "chat_v2.human.thread", "mb.user.dm", "notif.inbox"). Counter is in divergence.go, separate from DivergenceMetrics / SwitchBypassMetrics. AC-G2-6: Integration test in handlers_chat_v2_test.go verifies switch-OFF yields 201 and switch-ON yields 500 for the same topic without conversation_id.
All HTTP-facing write-deny sites now return 409 Conflict with machine-readable code "conversation_not_resolved" instead of 500 Internal Server Error. This matches G3's read-path shape: same failure, same status, same code — so an operator who turns the switch ON can distinguish "switch is correctly refusing unbackfilled topics" from "something else broke". 409 is the semantically correct status: the request conflicts with the current state of the resource (no conversation row), and retrying will not help until backfill runs. Sites changed: - handlers_agent_messaging.go: outbound.derive, outbound.resolve, outbound.validate, agent_msg.phase11, agent_msg.derive, agent_msg.resolve, agent_msg.validate - handlers_broker_inbound.go: broker.phase11, broker.thread, broker.dm, broker.validate - handlers_chat_v2.go: chat_v2.agent_routed.thread, chat_v2.agent_routed.dm, chat_v2.agent_routed.validate, chat_v2.human.thread, chat_v2.human.dm AC-G2-6 integration test updated to assert 409 + code "conversation_not_resolved".
…re-existing DBs The CREATE UNIQUE INDEX on webchat_topic(conversation_id) in the DDL block fails on databases where the table was created before #1380 added the column — CREATE TABLE IF NOT EXISTS is a no-op, so the column is absent and the index creation errors. This prevents Init from reaching the migration that would have added the column. Remove the redundant index from the DDL in both SQLite and Postgres store implementations. The addTopicConversationID migration already creates both the column and the index, gated by migrationCompleted(). Fixes the "failed to initialize webchat store" warning that silently disables the entire web chat surface on pre-existing deployments.
Add GET/PUT /api/v1/admin/messaging endpoints for the two Tranche G operational switches (conversation_read_switch, conversation_write_deny_switch). Follows the maintenance/project_defaults pattern: DB-only section with KoanfPaths nil (not seeded at startup), presence-aware partial update (PUT one switch without clearing the other), audit fields (updated_by, origin:"managed", revision increment), and admin-gated via route guard. Absent/empty/malformed DB rows continue to yield OFF on both switches (fail-safe default preserved). No reader logic changed.
ptone
marked this pull request as draft
August 31, 2026 01:33
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a read-only attribution-report command to analyze conversation attribution completeness and implements the G2 write-deny migration, which rejects message writes when conversation resolution fails. It also adds a message path trace test (DEF-79) and removes the idx_webchat_topic_conversation index from the webchat topic schema. The review feedback is highly accurate and identifies critical issues: a potential nil pointer dereference in handleBrokerInbound when write-deny is disabled, potential memory exhaustion (OOM) in the attribution report due to unbounded appending of non-UUID examples, and several test assertions in webchannel_store_c4fix_test.go that will fail because they check for the removed index.
Comment on lines
+258
to
275
| if convErr != nil { | ||
| if s.writeDenyEnabled() { | ||
| messaging.WriteDenialMetrics.Inc("broker.phase11") | ||
| log.Error("conversation resolution failed", "error", convErr) | ||
| writeError(w, http.StatusConflict, ErrCodeConversationNotResolved, "conversation resolution failed", nil) | ||
| return | ||
| } | ||
| log.Warn("conversation resolution failed (write-deny OFF, continuing)", "error", convErr) | ||
| } else { | ||
| if req.Message.Metadata == nil { | ||
| req.Message.Metadata = make(map[string]string) | ||
| } | ||
| req.Message.Metadata["conversation_id"] = convResult.ConversationID | ||
| log.Info("Resolved conversation for broker inbound", | ||
| "conversation_id", convResult.ConversationID, | ||
| "surface", req.Surface, "external_ref", req.ExternalRef) | ||
| } | ||
| log.Info("Resolved conversation for broker inbound", | ||
| "conversation_id", convResult.ConversationID, | ||
| "surface", req.Surface, "external_ref", req.ExternalRef) | ||
| } |
Contributor
There was a problem hiding this comment.
A nil pointer dereference will occur here if convErr != nil and s.writeDenyEnabled() is false. In that case, the handler logs a warning and continues, but convResult remains nil. Accessing convResult.ConversationID on line 273 will then cause a panic. Moving the log.Info call inside the else block (where convResult is guaranteed to be non-nil) resolves this issue.
Suggested change
| if convErr != nil { | |
| if s.writeDenyEnabled() { | |
| messaging.WriteDenialMetrics.Inc("broker.phase11") | |
| log.Error("conversation resolution failed", "error", convErr) | |
| writeError(w, http.StatusConflict, ErrCodeConversationNotResolved, "conversation resolution failed", nil) | |
| return | |
| } | |
| log.Warn("conversation resolution failed (write-deny OFF, continuing)", "error", convErr) | |
| } else { | |
| if req.Message.Metadata == nil { | |
| req.Message.Metadata = make(map[string]string) | |
| } | |
| req.Message.Metadata["conversation_id"] = convResult.ConversationID | |
| log.Info("Resolved conversation for broker inbound", | |
| "conversation_id", convResult.ConversationID, | |
| "surface", req.Surface, "external_ref", req.ExternalRef) | |
| } | |
| log.Info("Resolved conversation for broker inbound", | |
| "conversation_id", convResult.ConversationID, | |
| "surface", req.Surface, "external_ref", req.ExternalRef) | |
| } | |
| if convErr != nil { | |
| if s.writeDenyEnabled() { | |
| messaging.WriteDenialMetrics.Inc("broker.phase11") | |
| log.Error("conversation resolution failed", "error", convErr) | |
| writeError(w, http.StatusConflict, ErrCodeConversationNotResolved, "conversation resolution failed", nil) | |
| return | |
| } | |
| log.Warn("conversation resolution failed (write-deny OFF, continuing)", "error", convErr) | |
| } else { | |
| if req.Message.Metadata == nil { | |
| req.Message.Metadata = make(map[string]string) | |
| } | |
| req.Message.Metadata["conversation_id"] = convResult.ConversationID | |
| log.Info("Resolved conversation for broker inbound", | |
| "conversation_id", convResult.ConversationID, | |
| "surface", req.Surface, "external_ref", req.ExternalRef) | |
| } | |
| } |
Comment on lines
+281
to
+289
| if !senderIsUUID || !recipientIsUUID { | ||
| report.NonUUIDPrincipal++ | ||
| report.NonUUIDExamples = append(report.NonUUIDExamples, NonUUIDExample{ | ||
| MessageID: msg.ID, | ||
| SenderID: senderID, | ||
| RecipientID: recipientID, | ||
| }) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Unbounded appending to report.NonUUIDExamples can cause memory exhaustion (OOM) and crash the CLI when running the report on a large database with many legacy or federated messages. Limiting NonUUIDExamples to a small sample size (e.g., 10), similar to UnresolvableExamples, prevents this issue and keeps the output clean.
Suggested change
| if !senderIsUUID || !recipientIsUUID { | |
| report.NonUUIDPrincipal++ | |
| report.NonUUIDExamples = append(report.NonUUIDExamples, NonUUIDExample{ | |
| MessageID: msg.ID, | |
| SenderID: senderID, | |
| RecipientID: recipientID, | |
| }) | |
| return | |
| } | |
| if !senderIsUUID || !recipientIsUUID { | |
| report.NonUUIDPrincipal++ | |
| if len(report.NonUUIDExamples) < 10 { | |
| report.NonUUIDExamples = append(report.NonUUIDExamples, NonUUIDExample{ | |
| MessageID: msg.ID, | |
| SenderID: senderID, | |
| RecipientID: recipientID, | |
| }) | |
| } | |
| return | |
| } |
Comment on lines
+360
to
+372
| func mergeAttributionReport(dst, src *AttributionReport) { | ||
| dst.Total += src.Total | ||
| dst.Attributed += src.Attributed | ||
| dst.Backfillable += src.Backfillable | ||
| dst.BroadcastNotBackfillable += src.BroadcastNotBackfillable | ||
| dst.NonUUIDPrincipal += src.NonUUIDPrincipal | ||
| dst.Unresolvable += src.Unresolvable | ||
| dst.NonUUIDExamples = append(dst.NonUUIDExamples, src.NonUUIDExamples...) | ||
| dst.UnresolvableExamples = append(dst.UnresolvableExamples, src.UnresolvableExamples...) | ||
| if len(dst.UnresolvableExamples) > 10 { | ||
| dst.UnresolvableExamples = dst.UnresolvableExamples[:10] | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
When merging reports, NonUUIDExamples should also be capped to prevent unbounded memory growth during aggregation across multiple projects.
func mergeAttributionReport(dst, src *AttributionReport) {
dst.Total += src.Total
dst.Attributed += src.Attributed
dst.Backfillable += src.Backfillable
dst.BroadcastNotBackfillable += src.BroadcastNotBackfillable
dst.NonUUIDPrincipal += src.NonUUIDPrincipal
dst.Unresolvable += src.Unresolvable
dst.NonUUIDExamples = append(dst.NonUUIDExamples, src.NonUUIDExamples...)
if len(dst.NonUUIDExamples) > 10 {
dst.NonUUIDExamples = dst.NonUUIDExamples[:10]
}
dst.UnresolvableExamples = append(dst.UnresolvableExamples, src.UnresolvableExamples...)
if len(dst.UnresolvableExamples) > 10 {
dst.UnresolvableExamples = dst.UnresolvableExamples[:10]
}
}
Comment on lines
+208
to
+213
| require.True(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | ||
| "conversation_id column must exist after fresh Init") | ||
| require.True(t, sqliteIndexExists(db, "idx_webchat_topic_conversation"), | ||
| "idx_webchat_topic_conversation must exist after fresh Init") | ||
| require.True(t, sqliteMigrationRecorded(db, "topic_conversation_id"), | ||
| "topic_conversation_id migration must be recorded after fresh Init") |
Contributor
There was a problem hiding this comment.
Since idx_webchat_topic_conversation was removed from the schema in webchannel_store.go and webchannel_store_postgres.go, asserting its existence will cause the tests to fail. This assertion should be removed.
Suggested change
| require.True(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must exist after fresh Init") | |
| require.True(t, sqliteIndexExists(db, "idx_webchat_topic_conversation"), | |
| "idx_webchat_topic_conversation must exist after fresh Init") | |
| require.True(t, sqliteMigrationRecorded(db, "topic_conversation_id"), | |
| "topic_conversation_id migration must be recorded after fresh Init") | |
| require.True(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must exist after fresh Init") | |
| require.True(t, sqliteMigrationRecorded(db, "topic_conversation_id"), | |
| "topic_conversation_id migration must be recorded after fresh Init") |
Comment on lines
+230
to
+234
| // Verify the column does NOT exist before Init. | ||
| require.False(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | ||
| "precondition: conversation_id must not exist before Init") | ||
| require.False(t, sqliteIndexExists(db, "idx_webchat_topic_conversation"), | ||
| "precondition: idx_webchat_topic_conversation must not exist before Init") |
Contributor
There was a problem hiding this comment.
Remove the precondition assertion for idx_webchat_topic_conversation since the index has been removed from the schema.
Suggested change
| // Verify the column does NOT exist before Init. | |
| require.False(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | |
| "precondition: conversation_id must not exist before Init") | |
| require.False(t, sqliteIndexExists(db, "idx_webchat_topic_conversation"), | |
| "precondition: idx_webchat_topic_conversation must not exist before Init") | |
| // Verify the column does NOT exist before Init. | |
| require.False(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | |
| "precondition: conversation_id must not exist before Init") |
Comment on lines
+239
to
+244
| require.True(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | ||
| "conversation_id column must exist after Init on pre-existing DB") | ||
| require.True(t, sqliteIndexExists(db, "idx_webchat_topic_conversation"), | ||
| "idx_webchat_topic_conversation must exist after Init on pre-existing DB") | ||
| require.True(t, sqliteMigrationRecorded(db, "topic_conversation_id"), | ||
| "topic_conversation_id migration must be recorded after Init on pre-existing DB") |
Contributor
There was a problem hiding this comment.
Remove the post-init assertion for idx_webchat_topic_conversation since the index has been removed from the schema.
Suggested change
| require.True(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must exist after Init on pre-existing DB") | |
| require.True(t, sqliteIndexExists(db, "idx_webchat_topic_conversation"), | |
| "idx_webchat_topic_conversation must exist after Init on pre-existing DB") | |
| require.True(t, sqliteMigrationRecorded(db, "topic_conversation_id"), | |
| "topic_conversation_id migration must be recorded after Init on pre-existing DB") | |
| require.True(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must exist after Init on pre-existing DB") | |
| require.True(t, sqliteMigrationRecorded(db, "topic_conversation_id"), | |
| "topic_conversation_id migration must be recorded after Init on pre-existing DB") |
Comment on lines
+257
to
+260
| require.True(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | ||
| "conversation_id column must still exist after second Init") | ||
| require.True(t, sqliteIndexExists(db, "idx_webchat_topic_conversation"), | ||
| "idx_webchat_topic_conversation must still exist after second Init") |
Contributor
There was a problem hiding this comment.
Remove the idempotent assertion for idx_webchat_topic_conversation since the index has been removed from the schema.
Suggested change
| require.True(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must still exist after second Init") | |
| require.True(t, sqliteIndexExists(db, "idx_webchat_topic_conversation"), | |
| "idx_webchat_topic_conversation must still exist after second Init") | |
| require.True(t, sqliteColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must still exist after second Init") |
Comment on lines
+471
to
+476
| require.True(t, pgColumnExists(db, "webchat_topic", "conversation_id"), | ||
| "conversation_id column must exist after fresh Init") | ||
| require.True(t, pgIndexExists(db, "idx_webchat_topic_conversation"), | ||
| "idx_webchat_topic_conversation must exist after fresh Init") | ||
| require.True(t, pgMigrationRecorded(db, "topic_conversation_id"), | ||
| "topic_conversation_id migration must be recorded after fresh Init") |
Contributor
There was a problem hiding this comment.
Remove the Postgres fresh DB assertion for idx_webchat_topic_conversation since the index has been removed from the schema.
Suggested change
| require.True(t, pgColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must exist after fresh Init") | |
| require.True(t, pgIndexExists(db, "idx_webchat_topic_conversation"), | |
| "idx_webchat_topic_conversation must exist after fresh Init") | |
| require.True(t, pgMigrationRecorded(db, "topic_conversation_id"), | |
| "topic_conversation_id migration must be recorded after fresh Init") | |
| require.True(t, pgColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must exist after fresh Init") | |
| require.True(t, pgMigrationRecorded(db, "topic_conversation_id"), | |
| "topic_conversation_id migration must be recorded after fresh Init") |
Comment on lines
+497
to
+502
| require.True(t, pgColumnExists(db, "webchat_topic", "conversation_id"), | ||
| "conversation_id column must exist after Init on pre-existing DB") | ||
| require.True(t, pgIndexExists(db, "idx_webchat_topic_conversation"), | ||
| "idx_webchat_topic_conversation must exist after Init on pre-existing DB") | ||
| require.True(t, pgMigrationRecorded(db, "topic_conversation_id"), | ||
| "topic_conversation_id migration must be recorded after Init on pre-existing DB") |
Contributor
There was a problem hiding this comment.
Remove the Postgres pre-existing DB assertion for idx_webchat_topic_conversation since the index has been removed from the schema.
Suggested change
| require.True(t, pgColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must exist after Init on pre-existing DB") | |
| require.True(t, pgIndexExists(db, "idx_webchat_topic_conversation"), | |
| "idx_webchat_topic_conversation must exist after Init on pre-existing DB") | |
| require.True(t, pgMigrationRecorded(db, "topic_conversation_id"), | |
| "topic_conversation_id migration must be recorded after Init on pre-existing DB") | |
| require.True(t, pgColumnExists(db, "webchat_topic", "conversation_id"), | |
| "conversation_id column must exist after Init on pre-existing DB") | |
| require.True(t, pgMigrationRecorded(db, "topic_conversation_id"), | |
| "topic_conversation_id migration must be recorded after Init on pre-existing DB") |
added 12 commits
August 31, 2026 02:09
…t fixes
P0: Move the log.Info("Resolved conversation …") inside the else branch
so it only runs when convResult is non-nil. Before this fix, when
ResolveOrCreateConversationByKey returned (nil, error) and write-deny
was OFF (the default), the trailing log dereference panicked.
Add TestHandleBrokerInbound_ConvResolutionFailure_WriteDenyOff which
triggers the exact scenario and asserts no panic + fail-open dispatch.
P1 (errcheck): Wrap rows.Close / db.Close in webchannel_store_c4fix_test.go
with `defer func() { _ = x.Close() }()` to satisfy golangci-lint errcheck,
matching the existing package convention.
P1 (unbounded slice): Cap NonUUIDExamples at 10 entries in both the
collection path and mergeAttributionReport, matching UnresolvableExamples.
P2: Pass allowed methods to MethodNotAllowed in admin_messaging.go and
admin_messaging_divergence.go so the 405 response includes an Allow header.
…-soft on error
The initOperationalSettings caller was gated on
strings.EqualFold(cfg.Database.Driver, "postgres"), but every callee is
already driver-agnostic (SQLite no-ops the advisory lock via the
AdvisoryLocker branch). The gate made GetOperationalSettings() nil on
SQLite, so the messaging admin API returned 501 and both switches were
unreachable.
Changes:
1. Remove the postgres-only guard — initOperationalSettings now runs on
all drivers.
2. On init failure, log at ERROR with the wrapped error and continue
booting (fail-soft) rather than aborting the hub. The messaging
switches remain fail-closed (OFF) when OperationalSettings is nil.
3. Update doc comments on OperationalSettings and
startSettingsPropagation to reflect driver-agnostic usage.
4. Add explicit fail-closed unit tests for both messaging switches
covering all four degenerate inputs: absent row, empty {}, malformed
JSON, and nil OperationalSettings pointer.
…ts (DEF-89) handlers_chat_v2.go never populates ConversationID, leaving CreateTopic's tested dual-write branch unreachable. New webchat topics were created with no conversation row, causing the backfill to be a decaying artifact. When ConversationID is empty and hasConversationsTable() returns true, CreateTopic now generates a ConversationID before entering the existing dual-write branch. The external_ref derivation (empty string) matches backfillTopicConversations. hasConversationsTable() is called before BeginTx, preserving INVARIANT U-TX-1 (same pattern as EnsureGeneralTopic). Tests updated: backfill tests use raw SQL inserts to simulate pre-existing data without conversation_id; new AutoGen tests cover the happy path and MaxOpenConns=1 deadlock safety; all dual-write tests use context timeouts.
The previous commit fixed sqliteWebChatStore.CreateTopic; this extends the same fix to pgWebChatStore.CreateTopic, which had the identical bug: empty ConversationID took the legacy no-linkage INSERT path. Unlike the SQLite store, generation is unconditional — Postgres Init() migrations guarantee the conversations table exists, so there is no hasConversationsTable() gate. The legacy path is removed (dead code); the SQLite store retains it because hasConversationsTable() can return false in pre-migration environments. Behavior changes on Postgres (both tightenings): - Empty ConversationID + empty ProjectID: previously succeeded silently via the legacy path (inserting a topic with no project_id). Now fails with "project_id is required" because the auto-generated ConversationID triggers the existing ProjectID guard. The handler always provides ProjectID. - Partially-migrated DB without conversations table: previously succeeded via the legacy path (topic with no conversation). Now fails because the generated ConversationID triggers the dual-write tx, and the INSERT INTO conversations fails. Silent success becomes loud failure. This is intentional — a topic without a conversation is the defect being fixed. NOTE: the Postgres path is unverified by test. No pg test infrastructure exists in this repo (no testcontainers, dockertest, sqlmock, or postgres build tag; zero test files reference pgWebChatStore). The SQLite store's identical logic IS tested. Filed as DEF-99.
…-100) ResolveThreadConversationForRead now accepts an optional WithReadTopicLookup option that mirrors the write-path intercept in ResolveOrCreateConversationByKey. For native web topics whose conversations row has external_ref='', the resolver finds the conversation_id via the webchat_topic row instead of the impossible external_ref match that produced 409 on every read. Both handler call sites updated: - handlers_chat_v2.go S1: passes wcs (already holding the topic) - handlers_messages.go S3: passes s.webChatStore (had no topic lookup) Resolution order: topic lookup first; if store.ErrNotFound (not a native topic), fall through to external_ref. Infrastructure errors do NOT fall through. Non-native surfaces with well-formed external_ref are preserved. DM keys are unaffected (kind="direct" bypasses the intercept).
Add TestFormatForDelivery_ByteIdentity asserting that FormatForDelivery output is byte-identical to pinned golden strings for a representative spread of inputs: plain, raw, urgent, broadcast, threaded, with attachments, with metadata, group-set with recipients, system message, metadata filtering, and a fully-featured combined message. This is the Phase 9a safety net. Phase 9b introduces a new delivery envelope — the byte-identity guard ensures the switch-off path remains unchanged. Later phases must not weaken this test. Pure test addition; no production code changes.
…t Refresh (DEF-92) Today three getters — ConversationReadSwitch, ConversationWriteDenySwitch, and ProjectDefaultScratchpad — silently swallow JSON parse failures, returning the compiled default without logging. This makes a corrupted hub_settings row invisible to operators. Add a Malformed bool to sectionState, set it at parse time in Refresh and Update via json.Valid, and log the failure at error level ONCE per ingest. Getters can now distinguish "validated document" from "unreadable document" — which is the prerequisite for making default-ON and fail-closed simultaneously expressible (Phase 9a §4.6.2). This commit changes no default values and no getter behaviour. The Malformed field is generic to all sections, not just messaging, so every section benefits from the parse-time detection.
…ON switch (Phase 9a)
Replace conversation_read_switch and conversation_write_deny_switch with
a single conversation_envelope_switch that:
- defaults ON when the section is absent (compiled default)
- defaults ON when the key is omitted from the document
- returns OFF when the document is malformed (fail-closed, DEF-92)
- returns the explicit value when the key is present
Key changes:
opsettings/sections.go: add ConversationEnvelopeSwitch field; keep
stale fields for backward-compatible deserialization only.
opsettings/registry.go: update messaging schema to accept only
conversation_envelope_switch. Stale keys are rejected on write
(additionalProperties: false) and self-clean on first PUT.
operational_settings.go: new ConversationEnvelopeSwitch() getter using
the three-way sectionState split (absent → ON, malformed → OFF,
omitted → ON). Old getters removed.
admin_messaging.go: collapse from two fields to one. Explicit-null
reset uses DeleteSection so the absent→default (ON) path runs,
rather than writing a literal false (the f:=false trap in §4.6.3).
server.go, handlers_messages.go, handlers_chat_v2.go: all callers
updated to use ConversationEnvelopeSwitch.
No data migration. The new key is absent on every existing hub, so every
hub takes the compiled default (ON). Stale keys self-clean because
handlePutMessaging rebuilds the document from the Go struct.
Tests cover AC-9-7 (five stored states), AC-9-7a (stale-keys cutover),
AC-9-7b (malformed logged once per refresh), AC-9-7c (PUT cleans stale
keys), and AC-9-7d (null reset returns ON, not false).
…ence
Three fixes from review:
(1) DEF-92 type-mismatch observability. json.Valid catches syntactic
malformation but not semantic — a document like
{"conversation_envelope_switch":"yes"} is valid JSON but fails to
unmarshal into *bool. Without detection, this falls through to the
compiled default (ON), silently enabling the switch on a hub where
an operator made a typo.
Fix: at Refresh/Update time, after json.Valid passes, also
unmarshal into the section's typed struct via the registry's New
function. If that fails, set Malformed=true and log at ERROR with
the unmarshal error. Generic to all sections.
(2) nil-ops GET divergence. handleGetMessaging returned ON when
OperationalSettings was nil, but enforcement sites read
`ops != nil && ops.ConversationEnvelopeSwitch()` → OFF. The admin
endpoint was reporting a switch state that was not in effect.
Fix: default to false (what enforcement does) when ops is nil.
(3) Count accuracy noted — no code change needed.
4 files changed: operational_settings.go (+24), operational_settings_test.go (+31),
admin_messaging.go (+4 -3), admin_messaging_test.go (+8 -10).
Add two tests proving the consolidated envelope switch goes live on upgrade without data migration: - TestPhase9a_UpgradeCutover_WriteDenyLiveByDefault: no messaging row present → absent-row compiled default (ON) → write-deny fires 409. - TestPhase9a_UpgradeCutover_StaleKeysOnly_WriteDenyLive: messaging row with only the two stale keys (both false) → new key absent → compiled default (ON) → write-deny fires 409. Also updates TestG2_AC6 comments to clarify the first half passes because ops is nil (not because the switch is OFF).
…Phase 9b-i) Add DeliveryText to StructuredMessage and MessageRequest so the hub can pre-render the agent-facing envelope and the broker delivers it verbatim. Broker preference order: 1. req.DeliveryText (top-level wire field) 2. req.StructuredMessage.DeliveryText (carrier on StructuredMessage) 3. FormatForDelivery(req.StructuredMessage) (legacy fallback) 4. req.Message (plain text fallback) Co-located adapter (resolveDeliveryText) uses the same DeliveryText-first preference. HTTP and control-channel transports promote DeliveryText to the top-level wire field for the broker.
…elper (Phase 9b-ii) Enrich ConversationResult with Kind, Surface, DisplayName from the already-loaded store.Conversation at all resolution paths (zero extra queries). Add RenderDeliveryText and RenderDeliveryTextWithLookup as the single shared rendering entry point for all hub send paths. Invariants enforced by the helper: - Never fabricates an identifier. msg.ID is set from the persisted row. - reply_to is always omitted (genuine reply targets arrive in 9c-iii). - When ConvResult is nil, conversation is absent (honest absence).
The conversation divergence detector could not report a mismatch. Both branches of ComputeDivergenceMatch compared a routing key against a key derived from the same input fields in the same request, so its 'match' verdicts carried no information; and CheckConversationConsistency -- the one genuinely independent check, which queries prior persisted messages -- had its return value discarded at all seven call sites. Consequence, now fixed: the admin board reported 'matches: N' where N counted tautologies, and DEF-138 (agent replies persisting into a DM instead of the thread they came from) was invisible to every counter on that board while being trivially visible to a single round-trip test. - ComputeDivergenceMatch keeps its pure signature and is left tautological by deliberate choice; its docstring now says so accurately and preserves the one residual mismatch path (WithTopicLookup thread remap). Threading a store through to make it independent would duplicate a check that already exists. - CheckConversationConsistency gains its own counters and all seven call sites consume the return with a descriptive WARN. Instrumentation only -- no site fails a request. - Side effect: Inc() is now reached only from LogDivergence, so the mismatches counter no longer conflates the two signals (DEF-124/125 de-conflation). - Admin board gains consistency_checks/consistency_mismatches and a caveat stating that a match count of N means N tautological comparisons. - AC-7 mutation test, verified by the architect against a real production-code mutation rather than the compile error originally offered as evidence. - AC-8 structural guard fails if any call site re-discards the return.
… agent message path P-1: Add ConversationID to OutboundMessageRequest and propagate onto structuredMsg so it survives through the broker path to deliverToUser. P-2: Authorize caller-supplied ConversationID on the outbound handler. Port the DEF-49 authorization block with direction-aware docstrings: the agent is the SENDER (not recipient), so group-case asserts "the conversation belongs to the sending agent's project." Parse failure denies, always. Never normalise a DM key — a differing round-trip is an error, not a rewrite. P-3: deliverToUser honours a pre-resolved ConversationID from the upstream handler instead of re-deriving. This is the edit that makes explicit routing actually take effect — without it the handler's resolution was discarded when the broker was present, producing the inbound/outbound conversation split. P-4: Open the CLI conv:<uuid> and #<thread> gates. Update help text and rewrite SKILL.md so replying into the addressed conversation is the described default behaviour, not an optional new capability. AC-1 through AC-12 tests covering: round-trip per surface, unauthorised assertion denied (403 + no row), no memory, proactive send unchanged, absent field unchanged, metadata bypass blocked, SKILL.md content guard, and missing-conversation mismatch signal.
…e double divergence log BLOCKER 1: The pre-resolved ConversationID path in deliverToUser built a minimal ConversationResult with empty ExternalRef, causing ComputeDivergenceMatch to report a false routing-type-mismatch on every correctly-routed explicit message. Fixed by adding a distinct explicit-routing outcome (LogExplicitRouting) with its own counter that bypasses ComputeDivergenceMatch entirely. BLOCKER 2: AC-6 not met — LogDivergence fired twice per outbound reply (once in the handler, once in deliverToUser). Removed the handler's divergence block and consistency check; the broker's deliverToUser handles both at persistence time. Guard expected count updated 7→6. New test: TestDEF138_ExplicitRouting_NeverCountedAsMismatch exercises deliverToUser with a pre-resolved ConversationID and asserts DivergenceMetrics.Mismatches() does not increase. Mutation-verified: replacing LogExplicitRouting body with DivergenceMetrics.Inc(false) keeps the build green but fails the test.
Add explicit_routes counter to the divergence board response alongside consistency_checks/consistency_mismatches. Add explicit_routing_adoption caveat explaining that explicit routing is not a comparison — the caller stated the conversation identity — and that the counter measures adoption of the new routing path, not correctness. explicit_routes / (comparisons + explicit_routes) approximates the fraction of outbound traffic using explicit conversation routing.
An agent's reply was delivered to the right channel but persisted into a DM instead of the thread it came from. All four surfaces set ThreadID inbound, so the message resolved a group conversation; an agent's reply carries no ThreadID, so the outbound path always took the DM branch. The root cause was not the branch. An agent replies to a principal, and a principal does not identify a conversation, so the system was answering a question the caller never answered. The fix completes the address rather than improving the guess. Rule: explicit conversation wins; else explicit thread; else derive from the caller's own supplied address; else error. No component consults a side-table memory of where an agent was last active. The envelope already carries the conversation, so a reply carries it back in-band and its absence genuinely means 'not a reply' -- for which a derived DM is correct. - OutboundMessageRequest gains conversation_id; the CLI conv:<uuid> and #<thread> gates are opened. - Caller-asserted conversations are authorized before use, mirroring the DEF-49 block but with docstrings rewritten for the sender direction -- in the sibling handler 'agent' is the recipient, so identical-looking code asserts a different claim. - The handler's resolution now reaches the writer, collapsing a double resolution that previously discarded one result and logged divergence twice. - Explicit routes bypass ComputeDivergenceMatch entirely via a separate LogExplicitRouting outcome and counter. Comparing is meaningless when the caller stated the answer; an earlier revision fed an empty ExternalRef into the comparison and reported a false routing-type-mismatch on every correctly routed message. - explicit_routes is surfaced on the admin divergence board with a caveat stating it measures adoption, not correctness. - SKILL.md makes replying into the addressed conversation the described default; it previously said conversation_id was 'not yet required'. Reviewed over three rounds. Both blockers found in review were mutation-verified by the architect against production-code mutations with the build green.
…ng channel surface Conversations created via the thread path were always stamped surface="native" regardless of the originating channel (discord, slack, teams, etc). The channel was available in the request but not plumbed through to conversation creation. Add WithThreadSurface option to ResolveOrCreateThreadConversation, mirroring the existing WithSurface/WithTopicLookup pattern. Forward it to the shared ResolveOrCreateConversationByKey sink. Update all three callers that handle external-channel traffic: - resolvePhase5Conversation (handlers_broker_inbound.go) - deliverToUser thread branch (messagebroker.go) - deliverToAgent thread branch (messagebroker.go) handlers_chat_v2.go call sites are web-native (Channel:"web") and correctly keep the "native" default — no change needed. Empty channel is guarded: only non-empty values are forwarded, preserving the "native" default per R3. First-writer-wins is inherent in the upsert design: the (surface, external_ref) unique index means a new surface on the same external_ref creates a separate conversation row, not an update.
…n test double BLOCKER 1: All three call sites passed raw msg.Channel into WithThreadSurface without validation. Channel "web" (used by handlers_chat_v2.go) is not a valid surface enum value — passing it through would cause SurfaceValidator to reject the conversation write, denying the message. This violates R3. Add ChannelToSurface() as the single mapping function: - Valid surface names (discord, slack, telegram, gchat, teams, native) pass through directly. - "web" maps explicitly to "native" (web-chat is the native surface). - Unknown/empty channels fall back to "native" with a WARN log. - All three call sites now use ChannelToSurface() instead of raw channel. BLOCKER 2: surfaceTrackingUpserter accepted any surface string, hiding defects that production SurfaceValidator would reject. Now validates against the real enum — rejects values the real store would reject. New tests: - ChannelToSurface: valid channels, "web"→"native", empty→"native", unknown→"native" with warning - End-to-end: unknown channel resolves conversation successfully on "native", and raw unknown channel is rejected by the hardened test double
… nil-guard logger Eliminate the three-copy drift hazard: validSurfaces (production whitelist), validSurfaceEnum (test double), and the ent SurfaceValidator enum were linked only by comments. - Delete validSurfaceEnum entirely; the test double now uses validSurfaces. - Add TestValidSurfaces_MatchesEntEnum: imports pkg/ent/conversation and asserts validSurfaces matches the SurfaceValidator enum in both directions. An addition or removal to either side fails the test. - Guard ChannelToSurface against nil logger to prevent panic on exported API.
…ng cross-check The prior cross-check hand-listed the six ent Surface constants. Adding a seventh to the schema would not fail the test — the guard could not catch the exact drift it existed to prevent. Replace with a source-scanning test that reads the ent schema file (pkg/ent/schema/conversation.go), extracts the Values(...) arguments for the surface enum field via regex, and compares to validSurfaces in both directions. Fail-closed: if the file cannot be read, the pattern cannot be matched, or zero values are extracted, the test fails with a clear diagnostic. Remove the pkg/ent/conversation import — no longer needed since the test reads the schema source directly instead of importing generated constants.
…ing channel Every conversation created through the thread path was stamped surface 'native', whatever channel produced it. Found by ptone on a live gteam test: a Discord message returned an envelope reading surface: native. The row was written wrong at creation, not rendered wrong. WithSurface was reachable from two call sites, both gated on req.Surface and req.ExternalRef, and no plugin sets either — so every inbound message took the thread branch, which had no surface parameter at all. The hub already knew the channel; it used it twenty lines away for the affinity record. This matters more than a mislabelled field. conversation.go:89-92 declares a unique index on the PAIR (surface, external_ref), so conversation identity is per-channel by design — ptone's model, already in the schema. With surface pinned to a constant the index contributed nothing and silently degraded to uniqueness on external_ref alone. The fix reactivates a dormant constraint. Consequence, deliberate and tested: an existing (native, thread:P:T) row and a new (discord, thread:P:T) resolve to DIFFERENT conversations. Live histories fork rather than merge. TestDEF140_DiscordThreadCreatesSeparateConversation asserts the split happens and the old row is untouched. - ChannelToSurface is the single mapping point: valid surfaces pass through, web maps to native explicitly, unknown falls back to native with a WARN. Review blocker: the first revision forwarded the raw channel, and 'web' is not in the enum, so it would have converted a working path into a denied write. - The test double now rejects invalid enum values. Previously it accepted any string, which is why a change that could deny writes produced a green gate. - TestValidSurfaces_MatchesSchemaEnum scans the ent schema declaration rather than a transcription of it, fail-closed on unreadable file, unmatched pattern or zero values. Two earlier attempts compared hand-written copies against each other and could not fail in the direction that mattered. Architect verified independently: numstat, both mutation directions, and the full gate.
…ation in routing provenance
Add ConversationAsserted to StructuredMessage and a three-way classification
switch in the broker so that explicit_routes counts caller assertions only,
derived_routes counts hub derivations, and the adoption ratio can go down.
P1: ConversationAsserted field on StructuredMessage; IncDerivedRouting,
DerivedRoutes, LogDerivedRouting in pkg/messaging/divergence.go.
P2: asserted bool set true only after authorization in the handler's
explicit branch; propagated at the convResult site.
P3: Broker classification switches on ConversationAsserted, not
ConversationID != "". Honouring block unchanged (P-3 preserved).
P4: Admin board exposes derived_routes; caveat updated to state
explicit_routes counts caller assertions only.
P5: Tests for AC-1 through AC-7. AC-4 mutations verified (3/3 caught,
build green, semantic failure, clean restore).
Refs: DEF-141, DEFECTS.md [^81]
…ed binding from any JSON source AC-5 required fix: the original json:"conversation_asserted,omitempty" tag allowed three structs embedding StructuredMessage (inboundMessageRequest, MessageRequest, BroadcastMessageRequest) to bind the field from request JSON. Changed to json:"-" so the property holds by construction, not by reachability. Added route-independent unmarshal test (TestDEF141_AC5_ConversationAsserted_UnmarshalIgnored) that tests the tag directly — this test cannot rot regardless of future route changes. Mutation verified: reverting json:"-" to json:"conversation_asserted,omitempty" causes the unmarshal test to fail (build green, semantic failure).
…display names
resolveThread now collects ALL matches across all pages before resolving.
When >=2 group conversations in a project share a display_name, the send
is refused with reason:"ambiguous" and both candidates listed as
conv:<uuid> (surface=<surface>).
This addresses the DEF-140 fork path: (native, thread:P:T) and
(discord, thread:P:T) carry the same display_name. Pre-P1, the first
pagination hit was silently returned, delivering to a different audience
depending on sort order.
Tests (5 new):
- TestResolve_Thread_Ambiguous_DEF140ForkPath: DEF-140 fixture
(native + discord rows, same display_name) → ambiguous
- TestResolve_Thread_Ambiguous_ErrorMessage: error format validation
- TestResolve_Thread_SingleMatch_StillResolves: positive control
- TestResolve_Thread_ThreeMatches_StillAmbiguous: 3-way ambiguity
- TestResolve_Thread_Ambiguous_AcrossPages: cross-page detection (110 rows)
Mutation verified: reverting to first-match (return on first DisplayName hit)
causes 4 ambiguity tests to fail (build green, semantic failures).
SingleMatch positive control still passes under mutation.
…guity test
TestResolve_Thread_Ambiguous_AcrossPages used uuid.NewString() for fixture
IDs. mockStore sorts by ID for pagination, so random UUIDs gave random
sort order — both "target" rows landed on page 1 ~85% of the time, making
the test pass under a pagination-break mutation 17/20 runs.
Fix: use fmt.Sprintf("00000000-0000-0000-0000-%012d", i) so sorted order
equals insertion order and targets at i=5 and i=105 always straddle the
Limit:100 page boundary.
Mutation verified: breaking the pagination loop after page 1 now causes
the test to fail 20/20 (build green, semantic failure).
Known bounded gap: mockStore paginates on ID alone; production uses
keyset pagination on (created_at, id) via decodeCursor in
conversation_store.go. This test proves the loop iterates across pages;
it does not prove production pagination semantics.
…re through handler
P2: Added ConversationRef field to OutboundMessageRequest (hub) and its
hubclient mirror. Accepted forms: conv:<uuid>, @<agent-slug>, @<email>,
#<thread-name>. Mutually exclusive with ConversationID.
P3: Handler wiring in handleAgentOutboundMessage, in load-bearing order:
1. Both ConversationRef and ConversationID set → 400 (before anything)
2. Message validation BEFORE Resolve (Resolve can create a row)
3. Resolve with ResolveContext built ONLY from the authenticated caller
(agent.ProjectID, never from the request body)
4. Resolved ID flows through the EXISTING DEF-138 authorization block
5. asserted = true → explicit routing path
ELEVATED CONSTRAINT (G-1): ResolveContext.ProjectID is the containment
boundary for P1's ambiguity error disclosure. Every rctx field comes from
the authenticated caller. TestDEF142_G1_ResolveContext_ProjectFromAuth_NotBody
proves a foreign project's threads are invisible via conversation_ref.
Tests (8 new):
- MutualExclusion_BothRefAndID: both fields → 400
- ConversationRef_ConvUUID: conv:<uuid> resolves
- ConversationRef_ThreadRef: #thread resolves
- ConversationRef_NotFound: unknown thread → 400
- ConversationRef_Ambiguous: DEF-140 fork path → 400 with candidates
- ConversationRef_InvalidFormat: bad ref → 400
- ConversationRef_SetsAsserted: explicit_routes increments (broker path)
- G1_ResolveContext_ProjectFromAuth_NotBody: foreign project isolation
Mutation verified (AC-9): changing agent.ProjectID to req.ConversationID
in ResolveContext causes 6/8 tests to fail (build green, semantic failures).
G1 test specifically catches the project isolation breach.
…t resolve-or-create auth
AC-3: "not-found", "not-a-participant", and "boundary-violation" now
produce BYTE-IDENTICAL response bodies ("conversation_ref could not be
resolved"). The real reason is logged server-side only. Without this,
conv:<uuid> probes could distinguish "does not exist" from "exists but
sender is not a participant" — an enumeration oracle for conversation IDs.
"ambiguous" and "no-shared-project" remain distinct: ambiguity candidates
are group conversations in the caller's own project (already authorized),
and no-shared-project is a caller-side configuration error.
AC-3 mutation verified: un-collapsing (passing resErr.Error() through)
causes TestDEF142_AC3_NotFound_vs_NotParticipant_ByteIdentical to fail
with a diff showing "not found" vs "sender is not a participant" in the
response bodies. Build green, semantic failure.
AC-6: TestDEF142_AC6_ResolveOrCreate_FlowsThroughDEF138Auth verifies
that @agent-slug resolve-or-create (Created==true, Resolve skips its own
post-resolution auth) still flows through the DEF-138 authorization block
with asserted=true, causing explicit_routes to increment.
AC-6 mutation verified: removing the promotion (req.ConversationID =
resolveResult.ConversationID → _ = resolveResult) causes the test to fail
because explicit_routes stays at 0 — the resolved ID never enters the
DEF-138 block. Build green, semantic failure.
The AC-3 switch was a denylist: named reasons collapsed, default disclosed.
A future ResolutionError reason would land in default and disclose in full,
silently, with no test failing. That is fail-open on a disclosure control.
Inverted to allowlist: only "ambiguous" and "no-shared-project" disclose.
Default collapses. Same behaviour today, byte for byte. Different behaviour
on the day someone extends the enum: a new reason is collapsed until someone
deliberately adds it to the allowlist.
TestDEF142_AC3_FutureReason_CollapsedByDefault makes the allowlist
load-bearing via two mechanisms:
1. Source scan: asserts the handler contains the allowlist case and NOT
the denylist case (same pattern as consistency_check_guard_test.go)
2. End-to-end: a known-collapsed reason produces the generic message
without reason-specific text
Mutation verified: reverting to denylist shape causes the source scan to
fail. Build green, semantic failure.
…versation_ref
The CLI's two-step resolve-then-send pattern (call /conversations/resolve,
then send with the resolved ID) is replaced by passing conversation_ref
directly in the outbound message request. The server resolves it inline
(P3) and routes through the existing DEF-138 auth block.
Changes:
- Delete ConversationResolveRequest, ConversationResolveResponse types
and ResolveConversation from pkg/hubclient/messages.go (interface +
implementation)
- Delete /conversations/resolve mock from cmd/message_convref_test.go
- Rewrite sendMessageViaConversation in cmd/message.go:
- Agent context (SCION_AGENT_NAME set): all ref kinds use outbound
endpoint with conversation_ref
- Human CLI context: @agent uses SendStructuredMessage; server derives
the conversation from sender/recipient principals (DEF-138 Rule 3)
- Add TestSendMessageViaConversation_AgentRef_AgentContext test
- AC-8: grep finds zero occurrences of "conversations/resolve"
…ason Extract the AC-3 disclosure decision into a pure function disclosableResolutionReason(reason string) bool so the allowlist is directly testable. The handler delegates to it; the switch moves inside. Replace the source-grep test (Part 1) with a table-test that exercises every known reason plus "some-future-reason" and "" — binding to the actual artefact every reason passes through. Rename the end-to-end test (Part 2) to TestDEF142_AC3_KnownReason_CollapsedEndToEnd since it tests collapse of a known reason, not a future one. Table-test cases: "ambiguous" → true, "no-shared-project" → true, "not-found" → false, "not-a-participant" → false, "boundary-violation" → false, "some-future-reason" → false, "" → false
…on_ref Add TestDEF142_AC7_ConversationRef_ThroughRealMux that drives the CLI reference path through srv.Handler().ServeHTTP() with a real agent JWT token. The request traverses the full mux chain: guarded → agent route → agent action → handleAgentOutboundMessage → Resolve(conversation_ref) → DEF-138 auth → message dispatch. The test verifies the stored message lands in the resolved group conversation, not in a DM from the derivation fallback. Mandatory mutation evidence: removing the Resolve block from the handler causes the test to go red — the message lands in a DM (derivation) with a different conversation_id than the expected group conversation. Mutation output: expected: group conv "15020c94-..." (resolved via #d142-ac7-thread) actual: DM conv "8da84c8f-..." (derivation fallback) Clean restore: git diff handlers_agent_messaging.go → empty
…ject_id Send raw JSON body containing a foreign project_id in the request, creating two candidate sources for ProjectID (body vs authenticated agent token). The test proves the authenticated source always wins: under a "prefer-when-present" mutation the foreign thread resolves and DEF-138 auth rejects the project mismatch (400→403), catching the provenance violation.
Brings tranche-g up to date with main following the merge of the small-fix PRs (GoogleCloudPlatform#1467-GoogleCloudPlatform#1472). 41 main-only commits, 105 tranche-g-only. Merge base 9f66cd5. Two conflicts, both in const blocks: 1. pkg/hub/errors.go - purely additive. Ours adds 6 codes (conversation resolution + DEF-126 addressee), main adds 11 (B5 governance). No name collisions and no string-value collisions; kept both. 27 codes total, verified unique on both name and value. 2. pkg/store/concurrency.go - NOT additive. Both sides independently allocated 0x5C100014: ours to LockDataMigrations, main's to LockWebchatMigration. Keeping both verbatim would have shipped two distinct advisory locks sharing one value, silently serialising against each other on Postgres multi-replica. Main shipped first, so ours was renumbered to 0x5C100015 (next free; allocation is contiguous 0x5C100001-0x5C100014, plus 0x5C100020 and 0x5C1010xx). Advisory locks are ephemeral session locks - never persisted, no-op on SQLite - so renumbering is safe. Also registers main's two new keys (LockWebchatMigration, LockRecoveryAuthz) in the TestAdvisoryLockKeys_AllUnique registry. Main added both without registering either; main's own TestAdvisoryLockKeys_NonOverlapping samples 9 of 24 keys and covers none of the three keys in play, so it passed throughout the collision. The tranche-g registry test caught it: Check 2 (source-drift regex) named both unregistered keys, and with the collision reintroduced as a mutation, Check 1 reported "duplicate advisory lock key value 0x5C100014: LockWebchatMigration and LockDataMigrations".
Pre-existing on tranche-g before the main merge - the identical 14 files fail `gofmt -l` at 8795a70, and main and tranche-g carry the same `go 1.26.1` directive, so this is not toolchain skew and not merge fallout. The drift accumulated silently because `fmt-check` is a target of `make ci` but is NOT a job in .github/workflows/ci.yml. Nothing in the blocking gate ever ran it, so local `make ci` has been red on this branch while CI stayed green. Changes are comment and whitespace only. Thirteen files are pure whitespace (const-block comment alignment). cmd/boot_m9_test.go also gains two `//` lines from gofmt's doc-comment reflow around an indented code block - comment text only, no code tokens. `git diff -w --ignore-blank-lines` is empty for all other thirteen.
…merge The main merge (b4d78c0) left pkg/hub test code uncompilable. Main's authorization refactor (a61cddd) removed the policy concept and renamed createProjectMembersGroupAndPolicy -> createProjectMembersGroup, updating every call site it could see. Tranche-g had three it could not: two in handlers_broker_inbound_test.go and def135_test.go (files both branches edited) and one in a tranche-g-only test file. Git merged both sides cleanly. handlers_broker_inbound_test.go ended up with four call sites on the new name from main's rename and one on the old name from tranche-g, in the same file, with no conflict raised. Pure rename: identical signature (ctx, project *store.Project, callerUserID ...string), same defining file, and all three sites pass only (ctx, project). Why this got through: `go build ./...` passes because only TEST code referenced the old name, and the blocking CI gate is `make test-fast` (-tags no_sqlite), which excludes these files. Verified now with `go vet ./...` over the whole tree, which compiles test files in every package and reports zero errors.
…nt (DEF-152) The guard at line 210 in handleAgentOutboundMessage rejected any request with an empty recipient BEFORE the conversation_ref resolver at line 334 could run. Since conv:<uuid>, #<thread>, and @<agent> references carry no explicit recipient by design, they were always rejected with "recipient is required." Changes: - Relax the guard: requests with a conversation_ref but no recipient now pass through to the resolver. Requests with NEITHER are still 400 with the original error message. - After conversation resolution + DEF-138 authorization, derive the addressee from the resolved conversation's DM key for direct conversations. For group conversations (no single addressee), refuse explicitly rather than guessing. - Security: the addressee is derived from the conversation's participant set (the DM key), never from request input. Non-user addressees (agent-to-agent DMs) are refused on this user-delivery endpoint. Tests added: 1. conv:<uuid> with no recipient → 2xx, correct recipient derived from DM key (the missing test that would have caught this). 2. #<thread> with no recipient → clear refusal for group conversations. 3. Neither recipient nor conversation_ref → still 400, message unchanged. 4. Conversation the sender is not a participant of → refused, no disclosure of project IDs. 5. Backwards compatibility: ref + explicit recipient still works.
Two mutations survived the initial test suite: 1. Replacing the sender-matching if/else with `addrKind, addrID = kindB, idB` (always take side B) passed all tests because the fixture always had the agent on side A of the DM key. 2. Replacing `if addrKind != "user"` with `if false` passed all tests because no test exercised an agent-to-agent DM through this endpoint. Added: - TestDEF152_SenderOnSideB_DerivedAddresseeStillCorrect: uses a hand-crafted DM key with the user on side A and the agent on side B. Under mutation 1, the test fails (400 instead of 200: "non-user addressee"). - TestDEF152_NonUserAddressee_AgentToAgentDM_Refused: sends conv:<uuid> pointing to an agent-to-agent DM. Under mutation 2, the test fails (500 instead of 400: user lookup fails for agent ID).
Both flags are now refused early in RunE with an actionable error naming the replacement command (scion broadcast / scion broadcast --all). The flags remain registered (hidden, using BoolP) so cobra parses them without an "unknown flag" error, but they are no longer bound to package-level variables. Changes: - Remove msgBroadcast/msgAll variables and all conditional branches - Clean up sendMessageViaHub signature: remove dead broadcast/all params - Remove local-mode broadcast dispatch (fan-out, WaitGroup, etc.) - Remove Broadcasted field from buildStructuredMessage (only cmd/broadcast.go and server-side handleProjectBroadcast set it now) - Remove unused imports (state, config) - Reword conv:/# ref error messages to be unmistakably by-design: state the positive case (works inside an agent container where SCION_AGENT_NAME is set) - Update tests: remove broadcast/all test functions, update call signatures, fix Changed state cleanup across test isolation - Update docs (cli.md, messaging.md, SKILL.md) Security: server-side Broadcasted=true forcing in handleProjectBroadcast is untouched (verified by check-security-marker-gates).
scion broadcast is not in agentAllowed, so the refusal error must not recommend it when running in agent mode. In agent mode the error now says broadcasting is not available and tells agents to address recipients explicitly. Human mode keeps the existing scion broadcast pointer. Also fixes SKILL.md (agent-facing doc) to stop recommending scion broadcast — states plainly that broadcasting is not available in agent mode. Human-facing docs-site pages are unchanged. Adds tests for both agent-mode refusals asserting the error does NOT contain "scion broadcast".
broadcast is absent from agentAllowed by construction (it is an allowlist), but nothing in the test suite caught accidental grants. Injecting "broadcast": true into the map left all tests green. Add broadcast to: - buildTestTree: so applyModeRestrictions sees and removes it - TestApplyModeRestrictions_Agent absent list: pins end-to-end removal - TestAgentAllowedList notAllowed list: pins the map entry directly Mutation-tested: injecting "broadcast": true into agentAllowed causes both TestAgentAllowedList and TestApplyModeRestrictions_Agent to fail; reverting restores green.
Removes the --broadcast and --all flags from 'scion message'. Flags remain registered but hidden as tombstones so they refuse with an actionable error rather than cobra's bare 'unknown flag'. Refusal is mode-aware: agents are told to address recipients explicitly, since 'scion broadcast' is not in the agentAllowed allowlist and they cannot run it. Also pins broadcast's absence from agent mode (DEF-155), which was previously true by construction but asserted by no test. Reviewed by cr-msg-bcast: APPROVE, no Critical or Required findings.
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
1 participant
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.
Tranche G: conversation attribution made observable, then enforceable.
G1
scion server attribution-report— read-only; answers what disappears if the read switch flips. Blocking buckets + reconciliation against a global count.G4 DEF-58 negative gate, DEF-79 path trace, DEF-80 divergence caveats.
G3 read-switch fallback removed; typed 409 instead of silent legacy filter;
Channel:"web"preserved so the flip cannot widen visibility; 5 labelled bypass counters.G2 write path denies on derivation failure, behind
ConversationWriteDenySwitch(default OFF); 25 labelled denial counters; EnsureParticipant and federated-subscriber exceptions preserved.Deploy is inert. Sequence: deploy -> write-deny ON -> backfill -> G1 clean -> read switch ON. Each step reversible by ops toggle, no redeploy.