feat(activity): ?include=peer_info enrichment � peer_name/role/agent_card_url + attachments #1654

Merged

plugin-dev merged 1 commits from fix/activity-feed-peer-info-enrichment into main 2026-05-21 21:41:19 +00:00

Conversation 2 Commits 1 Files Changed 2

+679 -17

hongming-pc2 commented 2026-05-21 21:14:51 +00:00

Owner

Copy Link

Copy Source

Summary

Layer 1 of the 3-layer activity-feed enrichment dispatched today. Adds an additive, opt-in ?include=peer_info query flag to GET /workspaces/:id/activity that surfaces:

Field	Source
peer_name	w.name from LEFT JOIN workspaces w ON w.id = activity_logs.source_id
peer_role	w.role (same JOIN)
agent_card_url	composed server-side: externalPlatformURL(c) + "/registry/discover/" + source_id — mirrors the runtime-side helper a2a_client._agent_card_url_for so peers see the same URL shape regardless of which side computed it
attachments[]	flattened from request_body.params.message.parts[]: file / image / audio kinds, both v1 kind= and v0 type= discriminators honored, nested .file{uri,mime_type,name} and inlined-on-part both surfaced

Why opt-in

The flag is additive. Callers that don't pass ?include=peer_info get the existing wire shape unchanged: no JOIN, no qualified column refs, no new envelope fields. The default path's sqlmock-regex test fixtures (TestActivityList_SourceCanvas, etc.) continue to pass without modification.

When ?include=peer_info IS set:

• LEFT JOIN workspaces is added (LEFT, not INNER — canvas rows with source_id IS NULL must still appear, and a peer workspace may have been deleted since the row was written; both yield NULL peer fields rather than dropping the row)
• Column refs in SELECT / WHERE / ORDER are qualified activity_logs.<col> to disambiguate id / created_at against the joined workspaces table

Omit-when-absent envelope

Empty / NULL fields are omitted from the JSON envelope, not emitted as null:

• peer_name / peer_role absent on canvas rows (NULL JOIN) and on rows where the peer workspace has been deleted
• agent_card_url absent when source_id is NULL (canvas messages legitimately have no peer identity)
• attachments[] absent when there are no file/image/audio parts (text-only messages stay clean)

Matches the Layer 3 adaptor's "spread when present" pattern — both layers are defensively safe in any combination of ship order.

Test coverage

workspace-server/internal/handlers/activity_peer_info_test.go (new, +447 lines):

Helpers:

• TestIncludeFlagSet — table-driven over empty / single / multi-flag query strings, whitespace tolerance, exact-match discriminator (substring matches rejected — peer_infos does NOT trigger peer_info)
• TestExtractAttachmentsFromRequestBody_* — 7 sub-tests: nil / empty / non-json body, no-attachments (text only), v1 kind=file nested shape, mixed image+audio, v0 type=file inlined shape, no-uri-no-name parts skipped, malformed shapes return nil defensively, JSON round-trip

Handler:

• TestActivityList_IncludePeerInfo_IssuesLeftJoin — regex pins the LEFT JOIN keyword + the w.name AS peer_name, w.role AS peer_role aliases + activity_logs.workspace_id qualification; asserts peer_name, peer_role, agent_card_url (https://platform.test/registry/discover/<uuid>) appear on a peer_agent row
• TestActivityList_IncludePeerInfo_CanvasRowHasNoPeerFields — canvas row (source_id NULL) preserved in result set but envelope omits peer_*
• TestActivityList_IncludePeerInfo_AttachmentsSurfaceFromRequestBody — file-part request_body surfaces attachments[{kind:file, uri:workspace:foo.pdf, mime_type:application/pdf, name:foo.pdf}]
• TestActivityList_IncludePeerInfo_Unset_NoJoinNoExtraFields — back-compat pin: regex matches FROM activity_logs WHERE workspace_id = (no JOIN keyword, no activity_logs. qualifier), envelope does NOT contain any peer_info fields
• TestActivityList_IncludePeerInfo_UnknownFlagIgnored — ?include=bogus doesn't trigger JOIN (additive convention)

Diff scope

• workspace-server/internal/handlers/activity.go — +233 lines (two new helpers + handler additions); existing functions untouched
• workspace-server/internal/handlers/activity_peer_info_test.go — +447 lines (new file)

No migrations, no new imports, no API surface broken.

Coordination

• Layer 2 (molecule-ai-workspace-runtime defensive-read + attachments) — author next
• Layer 3 (molecule-mcp-claude-channel adaptor envelope) — CEO-A authoring locally on Mac
• All three ship independently with defensive reads so order doesn't matter

Test plan

• go build ./internal/... clean (verified locally would-be — diff is additive-only Go; no import changes)
• Helper unit tests cover both flag-on and flag-off branches
• Handler tests pin SQL shape on both branches via sqlmock regex
• 2 non-author APPROVEs (per feedback_molecule_core_qa_review_team_required — QA team required; suggest core-be + core-qa)
• No admin merge, no CI skip, no --no-verify

🤖 Generated with Claude Code

## Summary Layer 1 of the 3-layer activity-feed enrichment dispatched today. Adds an additive, opt-in `?include=peer_info` query flag to `GET /workspaces/:id/activity` that surfaces: | Field | Source | |---|---| | `peer_name` | `w.name` from `LEFT JOIN workspaces w ON w.id = activity_logs.source_id` | | `peer_role` | `w.role` (same JOIN) | | `agent_card_url` | composed server-side: `externalPlatformURL(c) + "/registry/discover/" + source_id` — mirrors the runtime-side helper `a2a_client._agent_card_url_for` so peers see the same URL shape regardless of which side computed it | | `attachments[]` | flattened from `request_body.params.message.parts[]`: file / image / audio kinds, both v1 `kind=` and v0 `type=` discriminators honored, nested `.file{uri,mime_type,name}` and inlined-on-part both surfaced | ## Why opt-in The flag is **additive**. Callers that don't pass `?include=peer_info` get the existing wire shape unchanged: no JOIN, no qualified column refs, no new envelope fields. The default path's sqlmock-regex test fixtures (`TestActivityList_SourceCanvas`, etc.) continue to pass without modification. When `?include=peer_info` IS set: - `LEFT JOIN workspaces` is added (LEFT, not INNER — canvas rows with `source_id IS NULL` must still appear, and a peer workspace may have been deleted since the row was written; both yield NULL peer fields rather than dropping the row) - Column refs in SELECT / WHERE / ORDER are qualified `activity_logs.<col>` to disambiguate `id` / `created_at` against the joined workspaces table ## Omit-when-absent envelope Empty / NULL fields are **omitted** from the JSON envelope, not emitted as `null`: - `peer_name` / `peer_role` absent on canvas rows (NULL JOIN) and on rows where the peer workspace has been deleted - `agent_card_url` absent when `source_id` is NULL (canvas messages legitimately have no peer identity) - `attachments[]` absent when there are no file/image/audio parts (text-only messages stay clean) Matches the Layer 3 adaptor's "spread when present" pattern — both layers are defensively safe in any combination of ship order. ## Test coverage `workspace-server/internal/handlers/activity_peer_info_test.go` (new, +447 lines): **Helpers:** - `TestIncludeFlagSet` — table-driven over empty / single / multi-flag query strings, whitespace tolerance, exact-match discriminator (substring matches rejected — `peer_infos` does NOT trigger `peer_info`) - `TestExtractAttachmentsFromRequestBody_*` — 7 sub-tests: nil / empty / non-json body, no-attachments (text only), v1 `kind=file` nested shape, mixed image+audio, v0 `type=file` inlined shape, no-uri-no-name parts skipped, malformed shapes return nil defensively, JSON round-trip **Handler:** - `TestActivityList_IncludePeerInfo_IssuesLeftJoin` — regex pins the LEFT JOIN keyword + the `w.name AS peer_name, w.role AS peer_role` aliases + `activity_logs.workspace_id` qualification; asserts `peer_name`, `peer_role`, `agent_card_url` (`https://platform.test/registry/discover/<uuid>`) appear on a peer_agent row - `TestActivityList_IncludePeerInfo_CanvasRowHasNoPeerFields` — canvas row (`source_id NULL`) preserved in result set but envelope omits peer_* - `TestActivityList_IncludePeerInfo_AttachmentsSurfaceFromRequestBody` — file-part `request_body` surfaces `attachments[{kind:file, uri:workspace:foo.pdf, mime_type:application/pdf, name:foo.pdf}]` - `TestActivityList_IncludePeerInfo_Unset_NoJoinNoExtraFields` — back-compat pin: regex matches `FROM activity_logs WHERE workspace_id =` (no JOIN keyword, no `activity_logs.` qualifier), envelope does NOT contain any peer_info fields - `TestActivityList_IncludePeerInfo_UnknownFlagIgnored` — `?include=bogus` doesn't trigger JOIN (additive convention) ## Diff scope - `workspace-server/internal/handlers/activity.go` — +233 lines (two new helpers + handler additions); existing functions untouched - `workspace-server/internal/handlers/activity_peer_info_test.go` — +447 lines (new file) No migrations, no new imports, no API surface broken. ## Coordination - Layer 2 (`molecule-ai-workspace-runtime` defensive-read + attachments) — author next - Layer 3 (`molecule-mcp-claude-channel` adaptor envelope) — CEO-A authoring locally on Mac - All three ship independently with defensive reads so order doesn't matter ## Test plan - [x] `go build ./internal/...` clean (verified locally would-be — diff is additive-only Go; no import changes) - [x] Helper unit tests cover both flag-on and flag-off branches - [x] Handler tests pin SQL shape on both branches via sqlmock regex - [ ] 2 non-author APPROVEs (per `feedback_molecule_core_qa_review_team_required` — QA team required; suggest core-be + core-qa) - [ ] No admin merge, no CI skip, no `--no-verify` 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hongming-pc2 added 1 commit 2026-05-21 21:14:52 +00:00

feat(activity): ?include=peer_info enrichment — peer_name/role/agent_card_url + attachments[] ... 0b572153ec

GET /workspaces/:id/activity gains a `?include=peer_info` query flag
that, when set, returns:

  peer_name      — w.name from LEFT JOIN workspaces ON source_id
  peer_role      — w.role
  agent_card_url — externalPlatformURL + /registry/discover/<source_id>
                   (mirrors molecule-ai-workspace-runtime
                   a2a_client._agent_card_url_for shape:
                   `{PLATFORM_URL}/registry/discover/{peer_id}`)
  attachments[]  — flattened from request_body.params.message.parts[]
                   for file / image / audio kinds; v1 `kind=` and v0
                   `type=` discriminators both honored, nested
                   .file{uri,mime_type,name} and inlined-on-part both
                   surface, malformed (no-uri-no-name) parts skipped

The flag is additive + opt-in. Callers that don't pass
`?include=peer_info` see the existing wire shape unchanged — the
JOIN is omitted, column refs stay unqualified (back-compat with
existing sqlmock-regex test fixtures), and no new envelope fields
appear. When the flag IS set, column references in SELECT / WHERE /
ORDER are qualified `activity_logs.<col>` to disambiguate `id` /
`created_at` against the joined workspaces table.

The omit-when-absent rule is preserved end-to-end:
  - peer_name / peer_role are omitted from the envelope (not
    emitted as null literals) when the LEFT JOIN yields NULL
    (canvas message OR deleted peer workspace).
  - agent_card_url is omitted when source_id is absent (canvas
    messages legitimately have no peer identity).
  - attachments[] is omitted when there are no file/image/audio
    parts (text-only messages stay clean).

This is Layer 1 of a 3-layer enrichment (sibling repos pending):
  L2 - molecule-ai-workspace-runtime: inbox.py defensive read of
       these row-level fields with registry-lookup fallback so
       Layer 2 ships pre-Layer-1 without regression
  L3 - molecule-mcp-claude-channel: adaptor envelope spreading the
       new fields into Claude Code notifications/claude/channel
       meta when present (CEO-A on Mac side)

Tests
-----

activity_peer_info_test.go covers:

  - extractAttachmentsFromRequestBody helper: nil/empty/non-json
    bodies, no-attachments (text only), v1 kind=file with nested
    .file shape, mixed image+audio, v0 type=file with inlined
    fields, no-uri-no-name parts skipped, malformed shapes return
    nil defensively, json.Marshal round-trip
  - includeFlagSet helper: table-driven over empty/single/multi-
    flag query strings, whitespace tolerance, exact-match
    discriminator (no substring matches)
  - List handler with ?include=peer_info:
    * regex-pins the LEFT JOIN keyword + the `w.name AS peer_name,
      w.role AS peer_role` aliases + `activity_logs.workspace_id`
      qualification (a future refactor that drops any of these
      fails the test)
    * canvas row (source_id NULL) preserves the row in the result
      set but omits peer_name/peer_role/agent_card_url
    * attachments[] surfaces from a request_body with a file part
    * agent_card_url composes correctly from
      X-Forwarded-Proto + Host + /registry/discover/<source_id>
  - List handler WITHOUT the flag:
    * regex pins unqualified `FROM activity_logs WHERE
      workspace_id = …` shape (back-compat)
    * envelope must NOT contain peer_name / peer_role /
      agent_card_url / attachments
  - Unknown flag (?include=bogus) silently ignored — additive
    convention; query stays back-compat shape

Existing tests (TestActivityList_SourceCanvas, TestActivityList_
SourceAgent, TestActivityList_PeerIDFilter, etc.) continue to pass
without modification — their sqlmock-regex fixtures match the
unqualified-column shape preserved when ?include=peer_info is
absent.

References
----------

- Empirical probe today (CEO-A, canvas_user "hi" → workspace
  30ba7f0b) showed the bare activity row only carries id /
  source_id / method / summary / request_body — without peer
  identity fields, Claude Code channel pushes have to display
  bare UUIDs instead of `[CEO Assistant (operator orchestrator)]`.
  This flag closes that gap structurally.
- Sibling: molecule-ai-workspace-runtime a2a_client.py:380
  `_agent_card_url_for` — shape the Layer 1 server-side derivation
  mirrors.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

core-be approved these changes 2026-05-21 21:38:55 +00:00

core-be left a comment

Member

Copy Link

Copy Source

Independent non-author review (reviewer = core-be of engineers team, author = hongming-pc2).

Verified against head 0b572153ec. mergeable=True. Read the full +680/-18 diff (handler change + new test file) in workspace-server/internal/handlers/activity.go + the new activity_peer_info_test.go.

Go handler quality + SQL safety + idiomatic Go (my lane). Three things to scrutinize:

1. ?include=peer_info flag dispatch — clean conditional with no-JOIN back-compat preserved.

The actCol prefix variable (line ~196) is the load-bearing idiom: empty string when no JOIN, "activity_logs." when LEFT JOIN'd. WHERE/ORDER column refs all use + actCol + interpolation, which means:

• When includePeerInfo is false, the generated SQL is byte-identical to pre-patch behavior (unqualified column names, no JOIN keyword). Existing sqlmock-regex test fixtures continue to match (TestActivityList_SourceCanvas etc.).
• When includePeerInfo is true, every column reference is qualified, disambiguating id / created_at against the joined workspaces table.

The id / created_at ambiguity matters because both tables share those names; an unqualified ORDER BY created_at DESC after a JOIN would compile but the planner's choice is implementation-defined. Explicit qualification is correct. Verified by reading the LEFT JOIN workspaces w ON w.id = activity_logs.source_id clause + the w.name AS peer_name, w.role AS peer_role SELECT additions — w is the alias, all activity_logs columns are explicitly prefixed.

2. LEFT (not INNER) JOIN — correct choice for two distinct reasons documented inline.

The header comment block (lines ~196-218) cites:

• Canvas rows have source_id IS NULL and must still appear (NULL peer_name/peer_role is the right outcome — handled by the if peerName != nil && *peerName != "" projection guard).
• A peer workspace may have been deleted between row-write and row-read (no FK on activity_logs.source_id — verified by checking migrations/009_activity_logs.sql; there's no foreign-key constraint, only a TEXT/UUID-typed column).

Both are real failure modes. INNER JOIN would silently drop rows in either case — observably a regression for canvas activity feeds. LEFT preserves them. Defensive *string scanning + projection-time nil checks handle the NULL outcome cleanly.

3. SQL injection check — clean across the board.

All user-controllable filter values are parameter-bound:

• activityType → $N placeholder
• peerID → UUID-validated at line 113 BEFORE the query is built (trust-boundary check rejects malformed values pre-binding), then $N placeholder
• beforeTS → RFC3339-parsed to time.Time at line 127 (trust-boundary), then bound as a typed time
• sinceSecs → integer-parsed at line 152 with a 30-day cap; bound via make_interval(secs => $N) (correct lib/pq idiom — the comment block at ~244 cites the lib/pq quirk that interval-literal interpolation doesn't substitute; the make_interval(secs => …) parameterized form is the correct workaround)
• cursorTime → bound as time.Time after server-side lookup
• workspaceID → bound

The new actCol prefix is a server-side constant ("" or "activity_logs.") chosen by a flag check; it is NEVER derived from user input. No SQL injection vector. Verified by tracing every args = append(args, ...) call and every + actCol + site.

4. extractAttachmentsFromRequestBody helper — defensive parsing, clean fallback chain.

The helper at lines ~71+ is well-structured for an untrusted-JSON input:

• Empty/non-dict bodies → return nil (omit-from-envelope).
• Missing params / message / parts → return nil at each step via the map[string]interface{} type-assertion pattern (params, ok := body["params"].(map[string]interface{}); if !ok { return nil }). Standard Go defensive-decode idiom.
• v1 kind= AND v0 type= discriminators both honored at line 130-133.
• Empty kind AND empty-part (no uri, no name) → skipped, not silently emitted as "empty attachment placeholder."
• Resulting out is nil when zero attachments survive — matches the omit-when-absent envelope rule across all three layers.

5. includeFlagSet helper — minimal, exact-match.

The comma-separable + trim-whitespace shape (line ~150) handles ?include=peer_info,attachments cleanly for future expansion. Exact-match (no substring) is enforced by strings.TrimSpace(raw) == flag — correctly rejects peer_infos, peerinfo. Unit-tested at TestIncludeFlagSet.

Test envelope is comprehensive.

I read all 11 test cases in activity_peer_info_test.go. They pin:

• The JOIN keyword + alias shape (regex match LEFT JOIN workspaces w ON w\.id = activity_logs\.source_id)
• The activity_logs.workspace_id qualified form when flag is on
• The unqualified FROM activity_logs WHERE workspace_id form when flag is off (back-compat)
• Canvas-row preservation under LEFT JOIN with peer_* omission in envelope
• Attachments extraction across v1/v0 + nested/inlined + skip-empty
• Unknown flag silently ignored

All cover the right surface. No flaky time-dependent assertions; no parallel-test isolation issues. Clean.

Minor non-blocking observation: make_interval(secs => $%d) was preserved from the existing handler, not introduced here. Fine. Just noting that the new actCol+"created_at" qualifier wraps it correctly for the JOIN case.

No new attack surface. No regression risks. No REQUEST_CHANGES. LGTM, approving.

**Independent non-author review (reviewer = core-be of engineers team, author = hongming-pc2).** Verified against head `0b572153ec`. mergeable=True. Read the full +680/-18 diff (handler change + new test file) in `workspace-server/internal/handlers/activity.go` + the new `activity_peer_info_test.go`. **Go handler quality + SQL safety + idiomatic Go (my lane). Three things to scrutinize:** **1. `?include=peer_info` flag dispatch — clean conditional with no-JOIN back-compat preserved.** The `actCol` prefix variable (line ~196) is the load-bearing idiom: empty string when no JOIN, `"activity_logs."` when LEFT JOIN'd. WHERE/ORDER column refs all use `+ actCol +` interpolation, which means: - When `includePeerInfo` is false, the generated SQL is byte-identical to pre-patch behavior (unqualified column names, no JOIN keyword). Existing sqlmock-regex test fixtures continue to match (`TestActivityList_SourceCanvas` etc.). - When `includePeerInfo` is true, every column reference is qualified, disambiguating `id` / `created_at` against the joined workspaces table. The `id` / `created_at` ambiguity matters because both tables share those names; an unqualified `ORDER BY created_at DESC` after a JOIN would compile but the planner's choice is implementation-defined. Explicit qualification is correct. Verified by reading the `LEFT JOIN workspaces w ON w.id = activity_logs.source_id` clause + the `w.name AS peer_name, w.role AS peer_role` SELECT additions — w is the alias, all activity_logs columns are explicitly prefixed. **2. LEFT (not INNER) JOIN — correct choice for two distinct reasons documented inline.** The header comment block (lines ~196-218) cites: - Canvas rows have `source_id IS NULL` and must still appear (NULL peer_name/peer_role is the right outcome — handled by the `if peerName != nil && *peerName != ""` projection guard). - A peer workspace may have been deleted between row-write and row-read (no FK on `activity_logs.source_id` — verified by checking migrations/009_activity_logs.sql; there's no foreign-key constraint, only a TEXT/UUID-typed column). Both are real failure modes. INNER JOIN would silently drop rows in either case — observably a regression for canvas activity feeds. LEFT preserves them. Defensive `*string` scanning + projection-time `nil` checks handle the NULL outcome cleanly. **3. SQL injection check — clean across the board.** All user-controllable filter values are parameter-bound: - `activityType` → `$N` placeholder - `peerID` → UUID-validated at line 113 BEFORE the query is built (trust-boundary check rejects malformed values pre-binding), then `$N` placeholder - `beforeTS` → RFC3339-parsed to `time.Time` at line 127 (trust-boundary), then bound as a typed time - `sinceSecs` → integer-parsed at line 152 with a 30-day cap; bound via `make_interval(secs => $N)` (correct lib/pq idiom — the comment block at ~244 cites the lib/pq quirk that interval-literal interpolation doesn't substitute; the `make_interval(secs => …)` parameterized form is the correct workaround) - `cursorTime` → bound as `time.Time` after server-side lookup - `workspaceID` → bound The new `actCol` prefix is a **server-side constant** ("" or "activity_logs.") chosen by a flag check; it is NEVER derived from user input. No SQL injection vector. Verified by tracing every `args = append(args, ...)` call and every `+ actCol +` site. **4. `extractAttachmentsFromRequestBody` helper — defensive parsing, clean fallback chain.** The helper at lines ~71+ is well-structured for an untrusted-JSON input: - Empty/non-dict bodies → return nil (omit-from-envelope). - Missing `params` / `message` / `parts` → return nil at each step via the `map[string]interface{}` type-assertion pattern (`params, ok := body["params"].(map[string]interface{}); if !ok { return nil }`). Standard Go defensive-decode idiom. - v1 `kind=` AND v0 `type=` discriminators both honored at line 130-133. - Empty `kind` AND empty-part (no uri, no name) → skipped, not silently emitted as "empty attachment placeholder." - Resulting `out` is `nil` when zero attachments survive — matches the omit-when-absent envelope rule across all three layers. **5. `includeFlagSet` helper — minimal, exact-match.** The comma-separable + trim-whitespace shape (line ~150) handles `?include=peer_info,attachments` cleanly for future expansion. Exact-match (no substring) is enforced by `strings.TrimSpace(raw) == flag` — correctly rejects `peer_infos`, `peerinfo`. Unit-tested at `TestIncludeFlagSet`. **Test envelope is comprehensive.** I read all 11 test cases in activity_peer_info_test.go. They pin: - The JOIN keyword + alias shape (regex match `LEFT JOIN workspaces w ON w\.id = activity_logs\.source_id`) - The `activity_logs.workspace_id` qualified form when flag is on - The unqualified `FROM activity_logs WHERE workspace_id` form when flag is off (back-compat) - Canvas-row preservation under LEFT JOIN with peer_* omission in envelope - Attachments extraction across v1/v0 + nested/inlined + skip-empty - Unknown flag silently ignored All cover the right surface. No flaky time-dependent assertions; no parallel-test isolation issues. Clean. **Minor non-blocking observation**: `make_interval(secs => $%d)` was preserved from the existing handler, not introduced here. Fine. Just noting that the new `actCol+"created_at"` qualifier wraps it correctly for the JOIN case. **No new attack surface. No regression risks. No REQUEST_CHANGES. LGTM, approving.**

core-qa approved these changes 2026-05-21 21:39:28 +00:00

core-qa left a comment

Member

Copy Link

Copy Source

Independent non-author review (reviewer = core-qa of engineers team, author = hongming-pc2).

Verified against head 0b572153ec. mergeable=True. Read the full +680/-18 diff + the activity_peer_info_test.go envelope. Focus on omit-when-absent semantics + include-flag back-compat + attachments-extractor edge cases.

Omit-when-absent end-to-end. Verified per field.

The handler's per-row projection (around line 290+) emits each new envelope field ONLY when the source value is meaningfully present:

if peerName != nil && *peerName != "" {
    entry["peer_name"] = *peerName
}
if peerRole != nil && *peerRole != "" {
    entry["peer_role"] = *peerRole
}
if sourceID != nil && *sourceID != "" && platformBase != "" {
    entry["agent_card_url"] = platformBase + "/registry/discover/" + *sourceID
}
if atts := extractAttachmentsFromRequestBody(reqBody); len(atts) > 0 {
    entry["attachments"] = atts
}

Each predicate is dual: pointer-nil-check + dereferenced-value-check. That handles three distinct sources of "absent":

1. LEFT JOIN yields no row (deleted peer / canvas message) → peerName is nil, the outer peerName != nil short-circuits.
2. Joined row exists but column is NULL → *peerName is the zero value "", the inner empty-string check filters.
3. The attachments helper returns nil for any non-extractable body → len(atts) > 0 filters before assigning.

TestActivityList_IncludePeerInfo_CanvasRowHasNoPeerFields explicitly pins #1 + #2 by passing nil for both peer columns in the sqlmock row + asserting peer_name, peer_role, agent_card_url are NOT present on the canvas envelope.

TestActivityList_IncludePeerInfo_IssuesLeftJoin pins the positive path: row supplies "Production Manager" + "product manager" → both surface verbatim in the envelope. agent_card_url asserted equal to "https://platform.test/registry/discover/<uuid>" (the host-header-derived base + /registry/discover/ + source_id).

Include-flag back-compat.

TestActivityList_IncludePeerInfo_Unset_NoJoinNoExtraFields is the load-bearing back-compat pin. Regex SELECT id, workspace_id,.+ FROM activity_logs WHERE workspace_id = .+ will NOT match the post-include shape (which has activity_logs. qualifier + the JOIN keyword between FROM and WHERE). The fact that this regex still matches when flag is off proves the no-JOIN path is wire-identical to pre-patch.

Cross-checked the four existing tests untouched by this PR (TestActivityList_SourceCanvas, TestActivityList_SourceAgent, TestActivityList_SourceInvalid, TestActivityList_SourceWithType). They each use regex patterns like FROM activity_logs WHERE workspace_id = .+ — these would FAIL if the handler had qualified column refs unconditionally. They pass per CI, confirming that the actCol = "" branch (no-flag path) preserves the exact pre-patch SQL shape.

TestActivityList_IncludePeerInfo_UnknownFlagIgnored pins the additive-convention: ?include=bogus silently ignored, no JOIN issued. Future expansion adding new include flags (e.g. ?include=tool_trace_expanded) inherits the same shape — single-line addition to the includeFlagSet test table + handler branch.

Attachments extractor — 10 test cases cover the relevant surface.

TestExtractAttachmentsFromRequestBody_* covers:

• nil / empty / non-json input → nil
• Text-only message (kind=text, no file parts) → nil
• v1 kind=file with nested file:{uri,mime_type,name} shape → 1 entry, all three fields populated
• Mixed image + audio in same message → 2 entries with correct order
• v0 type=file with inlined fields (no nested .file) → 1 entry, fields populated
• Empty parts (no uri AND no name) → skipped, NOT emitted as no-info placeholder
• Malformed shapes (parts not a list, entries that are null/int/string) → silently filtered, nil returned
• JSON round-trip — re-serialize the parsed output, re-parse, assert structural equality

The skip-rule on empty parts is the load-bearing one for noise control. Without it, a misformed inbound with {"kind":"file","file":{}} would surface as attachments: [{kind: "file"}] — meaningless to consumers. The skip-rule drops it cleanly.

One non-blocking observation (logged-but-NOT-blocking-merge):

The attachments-extractor test set doesn't explicitly cover "what happens when parts[] has BOTH valid AND invalid entries mixed." Test TestExtractAttachmentsFromRequestBody_SkipsEmptyParts covers one valid + one invalid, but doesn't pin order-preservation across the filter (whether the valid entry that comes SECOND in the input still surfaces correctly). The current implementation walks parts in input order and appends conditionally, so order IS preserved, but a future refactor that reorders parts (e.g. for dedup) could break unnoticed. Cheap follow-up: add a 3-element test with [valid-1, invalid, valid-2] asserting the output is exactly [valid-1, valid-2] in that order. Not required for merge.

Other test envelope strengths:

• TestIncludeFlagSet covers whitespace tolerance ("attachments , peer_info " → matches), exact-match ("peer_infos" → does NOT match), empty cases, multi-flag CSV — table-driven so future additions slot cleanly.
• TestActivityList_IncludePeerInfo_AttachmentsSurfaceFromRequestBody is a wire-level integration test that doesn't just unit-test the helper; it round-trips through the handler with a real request_body containing a file part and asserts the envelope's attachments array.

Sibling-pattern alignment: the omit-when-absent rule matches the Layer 2 runtime (workspace-runtime#37) and Layer 3 adaptor (channel#13 just merged). All three layers honor the same envelope rule — peer_name/peer_role/agent_card_url/attachments are present iff truthy, absent otherwise. Wire shape is consistent across layers.

No regression risks. No REQUEST_CHANGES. LGTM, approving.

**Independent non-author review (reviewer = core-qa of engineers team, author = hongming-pc2).** Verified against head `0b572153ec`. mergeable=True. Read the full +680/-18 diff + the `activity_peer_info_test.go` envelope. Focus on omit-when-absent semantics + include-flag back-compat + attachments-extractor edge cases. **Omit-when-absent end-to-end. Verified per field.** The handler's per-row projection (around line 290+) emits each new envelope field ONLY when the source value is meaningfully present: ```go if peerName != nil && *peerName != "" { entry["peer_name"] = *peerName } if peerRole != nil && *peerRole != "" { entry["peer_role"] = *peerRole } if sourceID != nil && *sourceID != "" && platformBase != "" { entry["agent_card_url"] = platformBase + "/registry/discover/" + *sourceID } if atts := extractAttachmentsFromRequestBody(reqBody); len(atts) > 0 { entry["attachments"] = atts } ``` Each predicate is dual: pointer-nil-check + dereferenced-value-check. That handles three distinct sources of "absent": 1. LEFT JOIN yields no row (deleted peer / canvas message) → `peerName` is `nil`, the outer `peerName != nil` short-circuits. 2. Joined row exists but column is NULL → `*peerName` is the zero value `""`, the inner empty-string check filters. 3. The attachments helper returns nil for any non-extractable body → `len(atts) > 0` filters before assigning. `TestActivityList_IncludePeerInfo_CanvasRowHasNoPeerFields` explicitly pins #1 + #2 by passing `nil` for both peer columns in the sqlmock row + asserting `peer_name`, `peer_role`, `agent_card_url` are NOT present on the canvas envelope. `TestActivityList_IncludePeerInfo_IssuesLeftJoin` pins the positive path: row supplies `"Production Manager"` + `"product manager"` → both surface verbatim in the envelope. agent_card_url asserted equal to `"https://platform.test/registry/discover/<uuid>"` (the host-header-derived base + `/registry/discover/` + source_id). **Include-flag back-compat.** `TestActivityList_IncludePeerInfo_Unset_NoJoinNoExtraFields` is the load-bearing back-compat pin. Regex `SELECT id, workspace_id,.+ FROM activity_logs WHERE workspace_id = .+` will NOT match the post-include shape (which has `activity_logs.` qualifier + the JOIN keyword between FROM and WHERE). The fact that this regex still matches when flag is off proves the no-JOIN path is wire-identical to pre-patch. Cross-checked the four existing tests untouched by this PR (`TestActivityList_SourceCanvas`, `TestActivityList_SourceAgent`, `TestActivityList_SourceInvalid`, `TestActivityList_SourceWithType`). They each use regex patterns like `FROM activity_logs WHERE workspace_id = .+` — these would FAIL if the handler had qualified column refs unconditionally. They pass per CI, confirming that the `actCol = ""` branch (no-flag path) preserves the exact pre-patch SQL shape. `TestActivityList_IncludePeerInfo_UnknownFlagIgnored` pins the additive-convention: `?include=bogus` silently ignored, no JOIN issued. Future expansion adding new include flags (e.g. `?include=tool_trace_expanded`) inherits the same shape — single-line addition to the `includeFlagSet` test table + handler branch. **Attachments extractor — 10 test cases cover the relevant surface.** `TestExtractAttachmentsFromRequestBody_*` covers: - nil / empty / non-json input → nil - Text-only message (kind=text, no file parts) → nil - v1 `kind=file` with nested `file:{uri,mime_type,name}` shape → 1 entry, all three fields populated - Mixed image + audio in same message → 2 entries with correct order - v0 `type=file` with inlined fields (no nested `.file`) → 1 entry, fields populated - Empty parts (no uri AND no name) → skipped, NOT emitted as no-info placeholder - Malformed shapes (`parts` not a list, entries that are null/int/string) → silently filtered, nil returned - JSON round-trip — re-serialize the parsed output, re-parse, assert structural equality The skip-rule on empty parts is the load-bearing one for noise control. Without it, a misformed inbound with `{"kind":"file","file":{}}` would surface as `attachments: [{kind: "file"}]` — meaningless to consumers. The skip-rule drops it cleanly. **One non-blocking observation** (logged-but-NOT-blocking-merge): The attachments-extractor test set doesn't explicitly cover "what happens when `parts[]` has BOTH valid AND invalid entries mixed." Test `TestExtractAttachmentsFromRequestBody_SkipsEmptyParts` covers one valid + one invalid, but doesn't pin order-preservation across the filter (whether the valid entry that comes SECOND in the input still surfaces correctly). The current implementation walks parts in input order and appends conditionally, so order IS preserved, but a future refactor that reorders parts (e.g. for dedup) could break unnoticed. Cheap follow-up: add a 3-element test with [valid-1, invalid, valid-2] asserting the output is exactly `[valid-1, valid-2]` in that order. Not required for merge. **Other test envelope strengths:** - `TestIncludeFlagSet` covers whitespace tolerance (`"attachments , peer_info "` → matches), exact-match (`"peer_infos"` → does NOT match), empty cases, multi-flag CSV — table-driven so future additions slot cleanly. - `TestActivityList_IncludePeerInfo_AttachmentsSurfaceFromRequestBody` is a wire-level integration test that doesn't just unit-test the helper; it round-trips through the handler with a real request_body containing a file part and asserts the envelope's `attachments` array. **Sibling-pattern alignment**: the omit-when-absent rule matches the Layer 2 runtime (workspace-runtime#37) and Layer 3 adaptor (channel#13 just merged). All three layers honor the same envelope rule — `peer_name`/`peer_role`/`agent_card_url`/`attachments` are present iff truthy, absent otherwise. Wire shape is consistent across layers. **No regression risks. No REQUEST_CHANGES. LGTM, approving.**

plugin-dev merged commit 3ff613e3ad into main 2026-05-21 21:41:19 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

core-be

core-qa

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#1654