feat(inbox-uploads): TS port of upload-resolution flow (RFC#640 Layer B) #26

2026-05-22T01:59:47Z

hongming-pc2 commented

2026-05-22 01:59:47 +00:00

Summary

RFC#640 4-layer cascade — Layer B (TS implementation). Adds src/inbox-uploads.ts mirroring the Python reference at molecule-ai-workspace-runtime/molecule_runtime/inbox_uploads.py. Closes the asymmetry between Python SDK (full 724 LOC module, production-deployed) and TS base MCP (previously zero inbox plumbing).

Companion to:

Layer A (workspace-runtime#42, also open) — the SSOT spec edit in _build_channel_instructions making the resolution flow MANDATORY.
Layer D (pending — molecule-mcp-server PR after this one) — AST-level contract test.
Layer C (CEO-A's authoring lane after this PR's npm publish) — channel adapter consuming this module.

CTO chat GO: "4-layer， dispatch team and follow SOP" 2026-05-22T01:31:48Z (canvas activity), relayed via CEO-Assistant.

Empirical trigger

2026-05-21 ~23:12Z agents-team: CTO pasted a PNG into the canvas. The chat_upload_receive row arrived at the channel plugin correctly, but the plugin had no upload-resolution code path. Agent saw a platform-pending:<ws>/<file_id> URI it couldn't open. Zero error surfaced. Silent file loss.

Every TS adapter polling /activity re-implements basic poll but misses upload resolution. This layer gives them a one-line import to replace per-adapter hand-rolling.

API surface

Re-exported from src/index.ts so consumers can either:

import { resolvePendingUpload, URICache, rewritePendingURIs } from "@molecule-ai/mcp-server";
// OR
import { ... } from "@molecule-ai/mcp-server/inbox-uploads";  // subpath import for tree-shake

package.json adds ./inbox-uploads to exports for the subpath form.

`URICache`

Bounded LRU mapping platform-pending:<ws>/<file_id> → file://<localPath>. JS Maps preserve insertion order; set re-inserts to promote to most-recent; eviction pops the first (oldest) entry.

Default URI_CACHE_MAX_ENTRIES = 32 — tighter than Python's 1024 because TS adapters typically run in tighter memory budgets (channel plugin / telegram-style adapters / codex bridges). Adapters with looser budgets can override via the constructor. Layer A spec section documents both values + the rationale.

`resolvePendingUpload(opts: ResolveUploadOptions): Promise<ResolveUploadResult>`

Runs the 5-step MANDATORY flow named in _build_channel_instructions:

GET /workspaces/<ws>/pending-uploads/<file_id>/content (using the caller's auth headers; the same bearer used for /activity polling — /pending-uploads/* is wsAuth-gated, no separate handshake).
Write to <cacheDir>/<32-hex-prefix>-<sanitized-filename> with mode 0o600 + wx flag (refuse-if-exists). Random prefix matches Python's pysecrets.token_hex(16) for parallel-upload-safe naming.
POST /workspaces/<ws>/pending-uploads/<file_id>/ack. Ack failure WARNS but does NOT throw — bytes are on disk and the user-visible outcome (agent can read the file) is achieved; the platform-side row eventually surfaces via Phase 3 sweep.
cache.set("platform-pending:<ws>/<file_id>", "file://<localPath>") if a cache was passed.
URI rewrite is the caller's concern — use rewritePendingURIs against any subsequent activity row.

Returns { localPath, localUri, mimeType, size, cachedPendingUri }.

`rewritePendingURIs(body: unknown, cache: URICache): unknown`

Deep walk + non-destructive rewrite. Walks arbitrary JSON shapes. The walk explicitly handles the two documented surfaces:

Top-level attachments[] (peer_info-enriched activity rows from L1 mc#1654)
Embedded params.message.parts[*].file.uri (a2a-sdk v1 message parts)

Conservative: only rewrites string values that exactly start with platform-pending: AND are present in the cache. Cache miss preserves the URI (no silent drop — better to surface an unopenable URI than to drop the reference entirely).

`isChatUploadReceiveRow(row: unknown): boolean`

Convenience matcher mirroring Python's is_chat_upload_row. Adapters fork upload rows off the regular A2A-message-handling path.

Bidirectional drift guard

The file header (15+ lines) explicitly tells future editors:

IF YOU EDIT THIS FILE:

Mirror the change in the Python reference (molecule_runtime/inbox_uploads.py).

If the contract semantics change (steps, ordering, endpoint shape), ALSO update the spec section in molecule_runtime/a2a_mcp_server.py::_build_channel_instructions ("Upload resolution (MANDATORY...)" block).

The Layer D contract test in __tests__/inbox-uploads-import-contract.test.ts will fail-CI on any TS file that imports apiCall from @molecule-ai/mcp-server to poll /activity but does NOT also import resolvePendingUpload.

Layer A's spec tests + Layer B's implementation tests + Layer D's contract test give us drift catchable from any side.

Test envelope (`src/tests/inbox-uploads.test.ts`, 27 cases)

URICache (8 tests): missing-key returns undefined / store-and-retrieve / set-replaces-without-growth / cap-exceeded-evicts-oldest / promote-on-get-survives-eviction / clear / bounds-check (rejects <1) / default-constant-is-32.

isChatUploadReceiveRow (4 tests): matches happy path / rejects other methods / rejects non-object defensively / rejects object-without-method.

resolvePendingUpload (7 tests):

Happy path: fetch+persist+ack+cache flow with mock fetch + real fs tmpdir.
Asserts BOTH endpoints called with the right method + URL shape.
Asserts file written with correct size + filename pattern matches ^[0-9a-f]{32}-pasted\.png$.
Asserts result envelope shape + cache populated.
Non-2xx GET → throws.
Size-cap breach → throws BEFORE writing (verified tmpdir stays empty).
Ack-failure → warns + returns successfully (bytes on disk wins).
Filename traversal ../../../etc/passwd → sanitized to safe passwd basename.
URL percent-encoding for workspaceId/fileId with special chars.
Required-field validation (workspaceId / fileId / cacheDir each tested).

rewritePendingURIs (8 tests):

Bare string rewrite.
Cache miss preserves URI (no silent drop).
Top-level attachments[].uri rewrite.
Embedded message.parts[*].file.uri rewrite — multiple parts in one body.
Non-URI strings pass through ("hello world", "workspace:..." etc.).
Non-mutation guarantee: input identity unchanged, output is new object.
null / undefined / number / boolean defensive.
Deep nested walk (5 levels deep).

Full jest suite: 6 suites / 162 passed / 1 skipped. No regression on the 135 pre-existing tests.

tsc + jest verified locally

=== tsc type check ===
(clean)

=== jest run ===
PASS src/__tests__/inbox-uploads.test.ts
Test Suites: 6 passed, 6 total
Tests:       1 skipped, 162 passed, 163 total
Time:        1.156 s

Run on operator with Node 20.20.2 + npm 10.8.2.

Cascade context

Layer	Repo	Status
A	molecule-ai-workspace-runtime#42	open + CI running (HEAD a9323dd)
B (this PR)	molecule-mcp-server	now
C	molecule-mcp-claude-channel	CEO-A authors after this PR's npm publish
D	molecule-mcp-server	pending — my parallel-with-B PR

When this PR merges → @molecule-ai/mcp-server@1.3.0 published (or whatever the next minor) → Layer C consumes the new version → cascade complete.

Test plan

tsc --noEmit clean (verified on operator).
jest src/__tests__/inbox-uploads.test.ts → 27/27 passed (verified on operator with Node 20.20.2).
jest full suite → 162 passed, 1 skipped (no regression).
LF line endings on commit (autocrlf=true on PC2).
CI all-green on this branch.
2 substantive non-author reviewer approves. Per CEO-A's dispatch: runtime-be + core-qa (TS port of a Python module — runtime-be owns the @molecule-ai/mcp-server shape; core-qa owns the test envelope across both the URICache LRU semantics and the wire-flow integration).

🤖 Generated with Claude Code

## Summary RFC#640 4-layer cascade — **Layer B** (TS implementation). Adds `src/inbox-uploads.ts` mirroring the Python reference at `molecule-ai-workspace-runtime/molecule_runtime/inbox_uploads.py`. Closes the asymmetry between Python SDK (full 724 LOC module, production-deployed) and TS base MCP (previously **zero** inbox plumbing). Companion to: - Layer A (workspace-runtime#42, also open) — the SSOT spec edit in `_build_channel_instructions` making the resolution flow MANDATORY. - Layer D (pending — molecule-mcp-server PR after this one) — AST-level contract test. - Layer C (CEO-A's authoring lane after this PR's npm publish) — channel adapter consuming this module. CTO chat GO: "4-layer， dispatch team and follow SOP" 2026-05-22T01:31:48Z (canvas activity), relayed via CEO-Assistant. ## Empirical trigger 2026-05-21 ~23:12Z agents-team: CTO pasted a PNG into the canvas. The `chat_upload_receive` row arrived at the channel plugin correctly, but the plugin had no upload-resolution code path. Agent saw a `platform-pending:<ws>/<file_id>` URI it couldn't open. Zero error surfaced. Silent file loss. Every TS adapter polling `/activity` re-implements basic poll but misses upload resolution. This layer gives them a one-line import to replace per-adapter hand-rolling. ## API surface Re-exported from `src/index.ts` so consumers can either: ```ts import { resolvePendingUpload, URICache, rewritePendingURIs } from "@molecule-ai/mcp-server"; // OR import { ... } from "@molecule-ai/mcp-server/inbox-uploads"; // subpath import for tree-shake ``` `package.json` adds `./inbox-uploads` to `exports` for the subpath form. ### `URICache` Bounded LRU mapping `platform-pending:<ws>/<file_id>` → `file://<localPath>`. JS Maps preserve insertion order; `set` re-inserts to promote to most-recent; eviction pops the first (oldest) entry. **Default `URI_CACHE_MAX_ENTRIES = 32`** — tighter than Python's `1024` because TS adapters typically run in tighter memory budgets (channel plugin / telegram-style adapters / codex bridges). Adapters with looser budgets can override via the constructor. Layer A spec section documents both values + the rationale. ### `resolvePendingUpload(opts: ResolveUploadOptions): Promise<ResolveUploadResult>` Runs the 5-step MANDATORY flow named in `_build_channel_instructions`: 1. `GET /workspaces/<ws>/pending-uploads/<file_id>/content` (using the caller's auth headers; the same bearer used for `/activity` polling — `/pending-uploads/*` is wsAuth-gated, no separate handshake). 2. Write to `<cacheDir>/<32-hex-prefix>-<sanitized-filename>` with `mode 0o600` + `wx` flag (refuse-if-exists). Random prefix matches Python's `pysecrets.token_hex(16)` for parallel-upload-safe naming. 3. `POST /workspaces/<ws>/pending-uploads/<file_id>/ack`. Ack failure WARNS but does NOT throw — bytes are on disk and the user-visible outcome (agent can read the file) is achieved; the platform-side row eventually surfaces via Phase 3 sweep. 4. `cache.set("platform-pending:<ws>/<file_id>", "file://<localPath>")` if a cache was passed. 5. URI rewrite is the caller's concern — use `rewritePendingURIs` against any subsequent activity row. Returns `{ localPath, localUri, mimeType, size, cachedPendingUri }`. ### `rewritePendingURIs(body: unknown, cache: URICache): unknown` Deep walk + non-destructive rewrite. Walks arbitrary JSON shapes. The walk explicitly handles the two documented surfaces: - Top-level `attachments[]` (peer_info-enriched activity rows from L1 mc#1654) - Embedded `params.message.parts[*].file.uri` (a2a-sdk v1 message parts) Conservative: only rewrites string values that exactly start with `platform-pending:` AND are present in the cache. Cache miss preserves the URI (no silent drop — better to surface an unopenable URI than to drop the reference entirely). ### `isChatUploadReceiveRow(row: unknown): boolean` Convenience matcher mirroring Python's `is_chat_upload_row`. Adapters fork upload rows off the regular A2A-message-handling path. ## Bidirectional drift guard The file header (15+ lines) explicitly tells future editors: > IF YOU EDIT THIS FILE: > - Mirror the change in the Python reference (`molecule_runtime/inbox_uploads.py`). > - If the contract semantics change (steps, ordering, endpoint shape), ALSO update the spec section in `molecule_runtime/a2a_mcp_server.py::_build_channel_instructions` ("Upload resolution (MANDATORY...)" block). > - The Layer D contract test in `__tests__/inbox-uploads-import-contract.test.ts` will fail-CI on any TS file that imports `apiCall` from `@molecule-ai/mcp-server` to poll /activity but does NOT also import `resolvePendingUpload`. Layer A's spec tests + Layer B's implementation tests + Layer D's contract test give us drift catchable from any side. ## Test envelope (`src/__tests__/inbox-uploads.test.ts`, 27 cases) **URICache (8 tests)**: missing-key returns undefined / store-and-retrieve / set-replaces-without-growth / cap-exceeded-evicts-oldest / promote-on-get-survives-eviction / clear / bounds-check (rejects `<1`) / default-constant-is-32. **isChatUploadReceiveRow (4 tests)**: matches happy path / rejects other methods / rejects non-object defensively / rejects object-without-method. **resolvePendingUpload (7 tests)**: - Happy path: fetch+persist+ack+cache flow with mock fetch + real fs tmpdir. - Asserts BOTH endpoints called with the right method + URL shape. - Asserts file written with correct size + filename pattern matches `^[0-9a-f]{32}-pasted\.png$`. - Asserts result envelope shape + cache populated. - Non-2xx GET → throws. - Size-cap breach → throws BEFORE writing (verified tmpdir stays empty). - Ack-failure → warns + returns successfully (bytes on disk wins). - Filename traversal `../../../etc/passwd` → sanitized to safe `passwd` basename. - URL percent-encoding for workspaceId/fileId with special chars. - Required-field validation (workspaceId / fileId / cacheDir each tested). **rewritePendingURIs (8 tests)**: - Bare string rewrite. - Cache miss preserves URI (no silent drop). - Top-level `attachments[].uri` rewrite. - Embedded `message.parts[*].file.uri` rewrite — multiple parts in one body. - Non-URI strings pass through (`"hello world"`, `"workspace:..."` etc.). - Non-mutation guarantee: input identity unchanged, output is new object. - null / undefined / number / boolean defensive. - Deep nested walk (5 levels deep). **Full jest suite**: 6 suites / 162 passed / 1 skipped. No regression on the 135 pre-existing tests. ## tsc + jest verified locally ``` === tsc type check === (clean) === jest run === PASS src/__tests__/inbox-uploads.test.ts Test Suites: 6 passed, 6 total Tests: 1 skipped, 162 passed, 163 total Time: 1.156 s ``` Run on operator with Node 20.20.2 + npm 10.8.2. ## Cascade context | Layer | Repo | Status | |---|---|---| | A | molecule-ai-workspace-runtime#42 | open + CI running (HEAD a9323dd) | | B (this PR) | molecule-mcp-server | now | | C | molecule-mcp-claude-channel | CEO-A authors after this PR's npm publish | | D | molecule-mcp-server | pending — my parallel-with-B PR | When this PR merges → `@molecule-ai/mcp-server@1.3.0` published (or whatever the next minor) → Layer C consumes the new version → cascade complete. ## Test plan - [x] `tsc --noEmit` clean (verified on operator). - [x] `jest src/__tests__/inbox-uploads.test.ts` → 27/27 passed (verified on operator with Node 20.20.2). - [x] `jest` full suite → 162 passed, 1 skipped (no regression). - [x] LF line endings on commit (autocrlf=true on PC2). - [ ] CI all-green on this branch. - [ ] 2 substantive non-author reviewer approves. Per CEO-A's dispatch: **runtime-be + core-qa** (TS port of a Python module — runtime-be owns the @molecule-ai/mcp-server shape; core-qa owns the test envelope across both the URICache LRU semantics and the wire-flow integration). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hongming-pc2 added 1 commit 2026-05-22 01:59:47 +00:00

feat(inbox-uploads): TS port of upload-resolution flow (RFC#640 Layer B)

CI / test (pull_request) Successful in 35s

Details

54932de63c

Mirrors molecule_runtime/inbox_uploads.py — closes the asymmetry between
Python SDK (full 724 LOC module + production-deployed) and TS base MCP
(previously zero inbox plumbing). Every TS adapter polling /activity
that consumes chat_upload_receive rows can now `import {
resolvePendingUpload, URICache, rewritePendingURIs } from
'@molecule-ai/mcp-server/inbox-uploads'` instead of hand-rolling the
fetch + persist + ack + cache + rewrite flow per-adapter.

Empirical trigger: 2026-05-21 ~23:12Z agents-team canvas paste —
channel plugin had no resolution code path and the agent saw a
platform-pending: URI it couldn't open. Layer A made the spec
MANDATORY; this layer is the implementation TS adapters consume.

API surface (re-exported from index.ts):
- `URICache`: bounded LRU mapping platform-pending: → local URI.
  Default 32 entries (URI_CACHE_MAX_ENTRIES) — tighter than Python's
  1024 because TS adapters typically have less memory budget.
- `resolvePendingUpload(opts)`: runs the 5-step flow:
    1. GET /workspaces/<ws>/pending-uploads/<file_id>/content
    2. Write to <cacheDir>/<32-hex-prefix>-<sanitized-filename> mode 0600
    3. POST /workspaces/<ws>/pending-uploads/<file_id>/ack
    4. cache.set("platform-pending:<ws>/<file_id>", "file://<localPath>")
    5. (URI rewrite is the caller's concern — use rewritePendingURIs)
  Returns {localPath, localUri, mimeType, size, cachedPendingUri}.
- `rewritePendingURIs(body, cache)`: deep walk + non-destructive
  rewrite across attachments[] + message.parts[*].file.uri surfaces.
  Cache miss preserves the URI (no silent drop).
- `isChatUploadReceiveRow(row)`: convenience matcher.

package.json adds `./inbox-uploads` to `exports` for the
`@molecule-ai/mcp-server/inbox-uploads` subpath import; the root
re-export still works for the standard `from '@molecule-ai/mcp-server'`
shape.

Test envelope (src/__tests__/inbox-uploads.test.ts, 27 cases):
- URICache: LRU eviction + promote-on-get/set + size/clear + bounds.
- isChatUploadReceiveRow: positive + 4 defensive negative cases.
- resolvePendingUpload: fetch+persist+ack+cache happy path / non-2xx
  throws / size-cap-breach pre-write / ack-failure warns but no throw
  / default-filename + traversal sanitization / URL percent-encoding
  / required-field validation.
- rewritePendingURIs: bare string / cache miss preserves / top-level
  attachments[] rewrite / nested message.parts[*].file.uri rewrite /
  non-URI strings pass through / no-mutation / null+undefined+primitive
  defenses / deep nested walk.

Full jest suite: 6 suites / 162 passed, 1 skipped. No regression.

File header carries the BIDIRECTIONAL DRIFT GUARD per CEO-A's brief:
"if you edit this file, mirror the change in the Python reference +
update the Layer A spec section if you're changing the contract".
Both sides can catch drift from the other.

Origin: RFC#640 4-layer cascade Layer B. CTO chat GO via
"4-layer， dispatch team and follow SOP" 2026-05-22T01:31:48Z.

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

infra-runtime-be approved these changes 2026-05-22 02:14:11 +00:00

infra-runtime-be left a comment

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

Verified against head 54932de. mergeable=True. Read full +837/-0 diff across src/inbox-uploads.ts + src/__tests__/inbox-uploads.test.ts + index.ts + package.json. Focus on the TS port correctness vs. Python reference + the surface adapter authors will integrate against.

Pre-flight CI gate — CI / test success on 54932de. Hygiene gate cleared.

Bidirectional drift guard in the file header — exact right shape.

The 15+ line header explicitly tells future editors three things:

Mirror changes in the Python reference (molecule_runtime/inbox_uploads.py).
If contract semantics change, ALSO update the Layer A spec section in _build_channel_instructions.
The Layer D contract test will fail-CI on adapters that import the poll-loop helper without also importing the resolution helpers.

This is the load-bearing drift catcher — both the Python file (via Layer A's structural tests) AND this TS file (via Layer D's CI gate) can catch divergence from the other side. Belt-and-braces.

URICache implementation — matches the Python reference's semantics.

I checked the Python _URICache class vs. this TS implementation side-by-side:

Operation	Python (OrderedDict)	TS (Map)
`get(k)` returns value if present	`_entries.get(k)`	`entries.get(k)`
`get(k)` promotes to most-recent	`move_to_end(k)`	`delete(k); set(k, local)`
`set(k, v)` re-inserts to most-recent	`_entries[k] = v; move_to_end(k)`	`if (has) delete; set`
Evict oldest when over cap	`popitem(last=False)`	`entries.keys().next().value` then `delete`
`size()` / `__len__`	`len(_entries)`	`entries.size`
`clear()`	`_entries.clear()`	`entries.clear()`

Functionally identical. JS Map preserves insertion order per spec (es2015+), so the "first inserted is the oldest" semantic holds. The promote-on-get pattern via delete + set is the idiomatic JS LRU approach for plain Map.

The TS default URI_CACHE_MAX_ENTRIES = 32 divergence from Python's 1024 is correctly documented in both the constant's JSDoc AND the Layer A spec text — TS adapters in tighter memory budgets vs Python's in-container generosity.

resolvePendingUpload — 5-step flow matches the spec contract.

I traced each step against Layer A's MANDATORY contract:

GET content — fetchImpl(contentUrl, { method: "GET", headers: authHeaders }). Auth headers passed through verbatim (not synthesized), so the adapter's existing /activity bearer flows through unchanged. Endpoint URL constructed with encodeURIComponent on both workspaceId AND fileId — defensive against UUIDs with reserved chars (although in practice both are RFC4122 UUIDs that don't need encoding; defensive doesn't hurt).
Persist — fs.mkdir(cacheDir, {recursive:true}) + fs.writeFile(path, bytes, {mode:0o600, flag:"wx"}). Mode 0600 matches Python's _open_safe. The wx flag (exclusive write) rejects pre-existing files at the target — matches Python's O_CREAT|O_EXCL. The 32-hex random prefix matches Python's pysecrets.token_hex(16).
Ack — fetchImpl(ackUrl, { method: "POST", headers: authHeaders }). Failure here WARNS but does NOT throw — bytes are on disk; user-visible outcome (agent can read the file) is achieved; the platform-side row eventually surfaces via Phase 3 sweep. This is the right ordering: persist-success matters more than ack-success for the immediate caller.
Cache — cache.set(pendingUri, localUri) if a cache was passed. Optional via opts.cache — gives callers the flexibility to manage their own cache lifecycle (per-session vs per-process).
Rewrite — explicitly the caller's concern via rewritePendingURIs. Separates the I/O step (1-3) from the in-memory rewrite step. Adapters can call rewrite on each subsequent activity row's body without re-running fetch.

rewritePendingURIs — deep walk + non-destructive.

The walk is recursive across object/array/primitive — handles arbitrary JSON shapes. Two important properties:

Non-destructive — returns a NEW value with substitutions applied; the input is not mutated. Tested at inbox-uploads.test.ts:296 with explicit identity comparison expect(out).not.toBe(input). Important for the inbox poller: the original activity row's body field shouldn't change semantics depending on whether rewrite ran.
Conservative rewrite — only rewrites string values that exactly start with platform-pending: AND are present in the cache. Cache miss preserves the URI (no silent drop). Text content, identity fields, JSON literals all pass through unchanged.

The walk explicitly handles the two documented surfaces: top-level attachments[] + embedded params.message.parts[*].file.uri. Tests pin both surfaces.

Size-cap guard placement — correct.

The bytes.byteLength > maxBytes check is BEFORE the file write. A platform that returns a malicious / oversized response can't trigger a disk write. Tmpdir-empty assertion in the test confirms this. Belt-and-braces above the platform's same-side 25 MB staging cap.

Filename sanitization — defends against traversal + meta chars.

sanitizeFilename strips path components (replace(/^.*[/\\]/, "")), reduces non-portable chars to _, collapses runs of underscores. Tested via ../../../etc/passwd → strips to passwd. Maximum length 240 (leaves room for the 32-hex prefix in ext4's 255 NAME_MAX).

Test envelope (27 tests) is comprehensive.

URICache (8), isChatUploadReceiveRow (4), resolvePendingUpload (7), rewritePendingURIs (8). I read each test:

URICache happy-path + edge cases (eviction-of-oldest under cap stress + promote-on-get) + bounds validation.
isChatUploadReceiveRow positive + 4 defensive negatives.
resolvePendingUpload uses mock fetch + real fs (tmpdir). Tests happy path, throw-on-non-2xx, size-cap-pre-write, ack-warns-no-throw, traversal-sanitization, URL encoding, required-field validation.
rewritePendingURIs covers: bare string, cache miss, top-level attachments[], nested message-parts, non-URI passthrough, no-mutation, primitives, deep nested walk.

Full suite still passes (162 passed / 1 skipped) — no regression on the 135 pre-existing tests.

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

The ack failure path uses console.warn. The repo's src/utils/logger.ts has a warn export that does structured logging. Using the local logger.warn(...) would be more consistent with the rest of the codebase (which uses pino through the logger wrapper). The current console.warn works but bypasses the structured logging surface. Cheap follow-up; not blocking the merge.

No regression risks. No REQUEST_CHANGES. LGTM, approving.

**Independent non-author review (reviewer = infra-runtime-be of engineers team, author = hongming-pc2).** Verified against head `54932de`. mergeable=True. Read full +837/-0 diff across `src/inbox-uploads.ts` + `src/__tests__/inbox-uploads.test.ts` + index.ts + package.json. Focus on the TS port correctness vs. Python reference + the surface adapter authors will integrate against. **Pre-flight CI gate** — `CI / test` success on `54932de`. Hygiene gate cleared. **Bidirectional drift guard in the file header — exact right shape.** The 15+ line header explicitly tells future editors three things: 1. Mirror changes in the Python reference (`molecule_runtime/inbox_uploads.py`). 2. If contract semantics change, ALSO update the Layer A spec section in `_build_channel_instructions`. 3. The Layer D contract test will fail-CI on adapters that import the poll-loop helper without also importing the resolution helpers. This is the load-bearing drift catcher — both the Python file (via Layer A's structural tests) AND this TS file (via Layer D's CI gate) can catch divergence from the other side. Belt-and-braces. **URICache implementation — matches the Python reference's semantics.** I checked the Python `_URICache` class vs. this TS implementation side-by-side: | Operation | Python (OrderedDict) | TS (Map) | |---|---|---| | `get(k)` returns value if present | `_entries.get(k)` | `entries.get(k)` | | `get(k)` promotes to most-recent | `move_to_end(k)` | `delete(k); set(k, local)` | | `set(k, v)` re-inserts to most-recent | `_entries[k] = v; move_to_end(k)` | `if (has) delete; set` | | Evict oldest when over cap | `popitem(last=False)` | `entries.keys().next().value` then `delete` | | `size()` / `__len__` | `len(_entries)` | `entries.size` | | `clear()` | `_entries.clear()` | `entries.clear()` | Functionally identical. JS Map preserves insertion order per spec (es2015+), so the "first inserted is the oldest" semantic holds. The promote-on-get pattern via `delete + set` is the idiomatic JS LRU approach for plain Map. The TS default `URI_CACHE_MAX_ENTRIES = 32` divergence from Python's `1024` is correctly documented in both the constant's JSDoc AND the Layer A spec text — TS adapters in tighter memory budgets vs Python's in-container generosity. **resolvePendingUpload — 5-step flow matches the spec contract.** I traced each step against Layer A's MANDATORY contract: 1. **GET content** — `fetchImpl(contentUrl, { method: "GET", headers: authHeaders })`. Auth headers passed through verbatim (not synthesized), so the adapter's existing /activity bearer flows through unchanged. Endpoint URL constructed with `encodeURIComponent` on both workspaceId AND fileId — defensive against UUIDs with reserved chars (although in practice both are RFC4122 UUIDs that don't need encoding; defensive doesn't hurt). 2. **Persist** — `fs.mkdir(cacheDir, {recursive:true})` + `fs.writeFile(path, bytes, {mode:0o600, flag:"wx"})`. Mode 0600 matches Python's `_open_safe`. The `wx` flag (exclusive write) rejects pre-existing files at the target — matches Python's `O_CREAT|O_EXCL`. The 32-hex random prefix matches Python's `pysecrets.token_hex(16)`. 3. **Ack** — `fetchImpl(ackUrl, { method: "POST", headers: authHeaders })`. Failure here WARNS but does NOT throw — bytes are on disk; user-visible outcome (agent can read the file) is achieved; the platform-side row eventually surfaces via Phase 3 sweep. This is the right ordering: persist-success matters more than ack-success for the immediate caller. 4. **Cache** — `cache.set(pendingUri, localUri)` if a cache was passed. Optional via opts.cache — gives callers the flexibility to manage their own cache lifecycle (per-session vs per-process). 5. **Rewrite** — explicitly the caller's concern via `rewritePendingURIs`. Separates the I/O step (1-3) from the in-memory rewrite step. Adapters can call rewrite on each subsequent activity row's body without re-running fetch. **rewritePendingURIs — deep walk + non-destructive.** The walk is recursive across object/array/primitive — handles arbitrary JSON shapes. Two important properties: 1. **Non-destructive** — returns a NEW value with substitutions applied; the input is not mutated. Tested at `inbox-uploads.test.ts:296` with explicit identity comparison `expect(out).not.toBe(input)`. Important for the inbox poller: the original activity row's `body` field shouldn't change semantics depending on whether rewrite ran. 2. **Conservative rewrite** — only rewrites string values that exactly start with `platform-pending:` AND are present in the cache. Cache miss preserves the URI (no silent drop). Text content, identity fields, JSON literals all pass through unchanged. The walk explicitly handles the two documented surfaces: top-level `attachments[]` + embedded `params.message.parts[*].file.uri`. Tests pin both surfaces. **Size-cap guard placement — correct.** The `bytes.byteLength > maxBytes` check is BEFORE the file write. A platform that returns a malicious / oversized response can't trigger a disk write. Tmpdir-empty assertion in the test confirms this. Belt-and-braces above the platform's same-side 25 MB staging cap. **Filename sanitization — defends against traversal + meta chars.** `sanitizeFilename` strips path components (`replace(/^.*[/\\]/, "")`), reduces non-portable chars to `_`, collapses runs of underscores. Tested via `../../../etc/passwd` → strips to `passwd`. Maximum length 240 (leaves room for the 32-hex prefix in ext4's 255 NAME_MAX). **Test envelope (27 tests) is comprehensive.** URICache (8), isChatUploadReceiveRow (4), resolvePendingUpload (7), rewritePendingURIs (8). I read each test: - URICache happy-path + edge cases (eviction-of-oldest under cap stress + promote-on-get) + bounds validation. - isChatUploadReceiveRow positive + 4 defensive negatives. - resolvePendingUpload uses mock fetch + real fs (tmpdir). Tests happy path, throw-on-non-2xx, size-cap-pre-write, ack-warns-no-throw, traversal-sanitization, URL encoding, required-field validation. - rewritePendingURIs covers: bare string, cache miss, top-level attachments[], nested message-parts, non-URI passthrough, no-mutation, primitives, deep nested walk. Full suite still passes (162 passed / 1 skipped) — no regression on the 135 pre-existing tests. **One non-blocking observation** (logged-but-NOT-blocking-merge): The `ack` failure path uses `console.warn`. The repo's `src/utils/logger.ts` has a `warn` export that does structured logging. Using the local `logger.warn(...)` would be more consistent with the rest of the codebase (which uses pino through the logger wrapper). The current `console.warn` works but bypasses the structured logging surface. Cheap follow-up; not blocking the merge. **No regression risks. No REQUEST_CHANGES. LGTM, approving.**

core-qa approved these changes 2026-05-22 02:14:51 +00:00

core-qa left a comment

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

Verified against head 54932de. mergeable=True. Read full diff. Focus on the test envelope's coverage of the failure modes that matter + the LRU semantics correctness via direct assertion.

Pre-flight CI gate — CI / test success on 54932de. Hygiene gate cleared.

Test envelope is comprehensive — 27 cases across 4 surfaces.

I traced what each cluster catches:

URICache (8 tests) — LRU correctness pinned

The most subtle LRU class invariant is "promote-on-get survives eviction." If implemented wrong (e.g. promote only on set, not get), a recently-accessed entry would still get evicted when the cap is hit. The test promotes on get — most-recently-accessed survives eviction is the load-bearing one:

const c = new URICache(3);
c.set("a", "1"); c.set("b", "2"); c.set("c", "3");
expect(c.get("a")).toBe("1");  // promote "a"
c.set("d", "4");  // evicts "b" (now oldest), not "a"
expect(c.get("b")).toBeUndefined();
expect(c.get("a")).toBe("1");

This is exactly the canonical LRU semantics check. If a future refactor switched promote-on-get to a no-op, this test catches it at CI.

Also covers: missing-key returns undefined, set replaces without growth, clear empties, bounds reject <1 constructor arg, default constant is 32 (catches accidental bump).

isChatUploadReceiveRow (4 tests) — defensive matchers

Positive happy-path + 4 negative cases covering different ways "not a chat_upload_receive row" can arrive (other method / null / undefined / non-object / object-without-method). Wire-defensiveness pinned — a runtime-mocked / malformed row can't accidentally trigger the upload-resolution code path.

resolvePendingUpload (7 tests) — wire-flow correctness + failure modes

Each test pins a specific call-shape pattern:

Happy path: mock fetch + real fs (tmpdir). Asserts BOTH endpoints called with the right method + URL + once each (calls.length === 2). Asserts file written with correct size + filename pattern matching ^[0-9a-f]{32}-pasted\.png$ (32-hex prefix + sanitized original name). Result envelope shape pinned (localPath, localUri, mimeType, size, cachedPendingUri). Cache populated.
Non-2xx GET → throws — pinned with regex /403 Forbidden/. A platform-side auth failure surfaces as an exception (caller decides retry behavior).
Size-cap breach BEFORE writing — asserts tmpdir.length === 0 after the throw. A platform that returns oversized response can't trigger a disk write.
Ack failure → warn + return success — uses jest.spyOn(console, "warn"), asserts the warning was logged AND the function returned successfully. This is the right semantics: bytes are on disk; user-visible outcome (agent can read) is achieved; platform-side row will surface via Phase 3 sweep.
Filename traversal sanitization — ../../../etc/passwd → ^[0-9a-f]{32}-passwd$. The traversal components stripped, leaving a safe basename.
URL percent-encoding — ws with space + file/with/slash both encoded correctly. Defensive against future workspace IDs that aren't RFC4122 UUIDs.
Required-field validation — workspaceId / fileId / cacheDir each tested with empty string, each throws.

rewritePendingURIs (8 tests) — JSON-walk correctness

Bare string — straight rewrite.
Cache miss preserves URI — load-bearing safety: a network race / restart-with-stale-cursor scenario must NOT silently drop the URI from the body. Agent will see something it can't open, which is preferable to seeing nothing.
Top-level attachments[] — peer_info-enriched activity row shape from L1 mc#1654.
Embedded message.parts[*].file.uri — a2a-sdk v1 message-part shape. Multiple parts in one body covered.
Non-URI strings pass through — text content, workspace: URIs not in cache.
No-mutation — explicit expect(out).not.toBe(input) + expect(input.x).toBe(original) pin both sides of the non-destructive contract.
null / undefined / primitives — defensive, walk doesn't crash on edge inputs.
Deep nested walk — 5-level deep object/array nesting.

The two-surface rewrite pin is the load-bearing test. A future refactor that only handles the top-level surface (or only the message-parts surface) would compile, type-check, pass partially, but break adapters consuming the other shape. Both tests must keep passing in parallel.

Test execution confirmed locally — jest src/__tests__/inbox-uploads.test.ts → 27/27 passed in 0.493s. Full suite (162/163) — no regression.

Mocking discipline.

The mockFetch pattern (async (url, init) => Response(...)) doesn't require Jest's heavyweight mocking infrastructure. Each test constructs a closure-scoped recorder + returns appropriate Responses. Clean, explicit, no vi.mock magic. The jest.spyOn(console, "warn").mockImplementation pattern correctly captures + restores around each test. Tests are independent of each other (each beforeEach creates a fresh tmpdir; each afterEach cleans up).

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

The cache-miss test asserts rewritePendingURIs("platform-pending:ws/missing", cache) returns the original URI (no rewrite). Good. But the test doesn't pin the JSON-shape variant — i.e., a body like {attachments: [{uri: "platform-pending:ws/missing"}]} with a cache miss. The current behavior IS preserve-on-miss (verified by reading the implementation), but adding an explicit JSON-shape miss-test would close that documentation gap. Cheap follow-up; not blocking.

No regression risks. No REQUEST_CHANGES. LGTM, approving.

**Independent non-author review (reviewer = core-qa of engineers team, author = hongming-pc2).** Verified against head `54932de`. mergeable=True. Read full diff. Focus on the test envelope's coverage of the failure modes that matter + the LRU semantics correctness via direct assertion. **Pre-flight CI gate** — `CI / test` success on `54932de`. Hygiene gate cleared. **Test envelope is comprehensive — 27 cases across 4 surfaces.** I traced what each cluster catches: ### URICache (8 tests) — LRU correctness pinned The most subtle LRU class invariant is "promote-on-get survives eviction." If implemented wrong (e.g. promote only on `set`, not `get`), a recently-accessed entry would still get evicted when the cap is hit. The test `promotes on get — most-recently-accessed survives eviction` is the load-bearing one: ```ts const c = new URICache(3); c.set("a", "1"); c.set("b", "2"); c.set("c", "3"); expect(c.get("a")).toBe("1"); // promote "a" c.set("d", "4"); // evicts "b" (now oldest), not "a" expect(c.get("b")).toBeUndefined(); expect(c.get("a")).toBe("1"); ``` This is exactly the canonical LRU semantics check. If a future refactor switched promote-on-get to a no-op, this test catches it at CI. Also covers: missing-key returns undefined, set replaces without growth, clear empties, bounds reject `<1` constructor arg, default constant is 32 (catches accidental bump). ### isChatUploadReceiveRow (4 tests) — defensive matchers Positive happy-path + 4 negative cases covering different ways "not a chat_upload_receive row" can arrive (other method / null / undefined / non-object / object-without-method). Wire-defensiveness pinned — a runtime-mocked / malformed row can't accidentally trigger the upload-resolution code path. ### resolvePendingUpload (7 tests) — wire-flow correctness + failure modes Each test pins a specific call-shape pattern: - **Happy path**: mock fetch + real fs (tmpdir). Asserts BOTH endpoints called with the right method + URL + once each (`calls.length === 2`). Asserts file written with correct size + filename pattern matching `^[0-9a-f]{32}-pasted\.png$` (32-hex prefix + sanitized original name). Result envelope shape pinned (localPath, localUri, mimeType, size, cachedPendingUri). Cache populated. - **Non-2xx GET → throws** — pinned with regex `/403 Forbidden/`. A platform-side auth failure surfaces as an exception (caller decides retry behavior). - **Size-cap breach BEFORE writing** — asserts `tmpdir.length === 0` after the throw. A platform that returns oversized response can't trigger a disk write. - **Ack failure → warn + return success** — uses `jest.spyOn(console, "warn")`, asserts the warning was logged AND the function returned successfully. This is the right semantics: bytes are on disk; user-visible outcome (agent can read) is achieved; platform-side row will surface via Phase 3 sweep. - **Filename traversal sanitization** — `../../../etc/passwd` → `^[0-9a-f]{32}-passwd$`. The traversal components stripped, leaving a safe basename. - **URL percent-encoding** — `ws with space` + `file/with/slash` both encoded correctly. Defensive against future workspace IDs that aren't RFC4122 UUIDs. - **Required-field validation** — workspaceId / fileId / cacheDir each tested with empty string, each throws. ### rewritePendingURIs (8 tests) — JSON-walk correctness - **Bare string** — straight rewrite. - **Cache miss preserves URI** — load-bearing safety: a network race / restart-with-stale-cursor scenario must NOT silently drop the URI from the body. Agent will see something it can't open, which is preferable to seeing nothing. - **Top-level attachments[]** — peer_info-enriched activity row shape from L1 mc#1654. - **Embedded message.parts[*].file.uri** — a2a-sdk v1 message-part shape. Multiple parts in one body covered. - **Non-URI strings pass through** — text content, workspace: URIs not in cache. - **No-mutation** — explicit `expect(out).not.toBe(input)` + `expect(input.x).toBe(original)` pin both sides of the non-destructive contract. - **null / undefined / primitives** — defensive, walk doesn't crash on edge inputs. - **Deep nested walk** — 5-level deep object/array nesting. **The two-surface rewrite pin is the load-bearing test.** A future refactor that only handles the top-level surface (or only the message-parts surface) would compile, type-check, pass partially, but break adapters consuming the other shape. Both tests must keep passing in parallel. **Test execution confirmed locally** — `jest src/__tests__/inbox-uploads.test.ts` → 27/27 passed in 0.493s. Full suite (162/163) — no regression. **Mocking discipline.** The `mockFetch` pattern (`async (url, init) => Response(...)`) doesn't require Jest's heavyweight mocking infrastructure. Each test constructs a closure-scoped recorder + returns appropriate Responses. Clean, explicit, no `vi.mock` magic. The `jest.spyOn(console, "warn").mockImplementation` pattern correctly captures + restores around each test. Tests are independent of each other (each `beforeEach` creates a fresh tmpdir; each `afterEach` cleans up). **One non-blocking observation** (logged-but-NOT-blocking-merge): The cache-miss test asserts `rewritePendingURIs("platform-pending:ws/missing", cache)` returns the original URI (no rewrite). Good. But the test doesn't pin the JSON-shape variant — i.e., a body like `{attachments: [{uri: "platform-pending:ws/missing"}]}` with a cache miss. The current behavior IS preserve-on-miss (verified by reading the implementation), but adding an explicit JSON-shape miss-test would close that documentation gap. Cheap follow-up; not blocking. **No regression risks. No REQUEST_CHANGES. LGTM, approving.**

plugin-dev merged commit 585d39b09a into main

2026-05-22 02:34:33 +00:00

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-mcp-server#26