test(contract): AST-level poll-uploads-resolved invariant (RFC#640 Layer D) #27

2026-05-22T02:04:17Z

hongming-pc2 commented

2026-05-22 02:04:17 +00:00

Summary

RFC#640 4-layer cascade — Layer D (AST-level contract test). Single new test file at src/__tests__/poll-uploads-resolved-contract.test.ts. Catches the third failure surface that Layers A + B close from the spec / implementation side: an adapter that has a poll loop but forgot to wire in the upload-resolution helpers.

Companion to:

Layer A (workspace-runtime#42, open) — the SSOT spec edit making upload resolution MANDATORY.
Layer B (molecule-mcp-server#26, open) — the TS implementation TS adapters consume.
Layer C (CEO-A's authoring lane after Layer B's npm publish) — channel adapter consuming Layer B.

CTO chat GO: "4-layer， dispatch team and follow SOP" 2026-05-22T01:31:48Z.

Test design: producer-side no-op gate + consumer-side enforcement

The test runs in TWO modes based on MCP_SERVER_CONTRACT_CONSUMERS env var:

Producer-side (this repo's own CI): env var unset → empty consumer list → test passes trivially. No external consumers needed for the producer's CI to be green.

Consumer-side (channel adapter / telegram / codex / etc.): each consumer repo's CI runs the test against its own source files via:

MCP_SERVER_CONTRACT_CONSUMERS=src/server.ts:src/poll.ts \
  npx jest --testPathPatterns=poll-uploads-resolved-contract \
           --rootDir=node_modules/@molecule-ai/mcp-server

The contract is engaged in consumer repos via the env-driven list. Same shape as the runtime-pin-check sibling pattern CEO-A flagged.

The invariant

For each TS file in the consumer list:

Parse with TS compiler API (ts.createSourceFile + ts.forEachChild).
Detect /activity URL literal in any string or template literal (head + after-substitution spans). Pattern is /workspaces/<x>/activity with a non-word boundary after to avoid the false-friend /activities.
Detect named imports from @molecule-ai/mcp-server OR @molecule-ai/mcp-server/inbox-uploads of resolvePendingUpload / URICache / rewritePendingURIs.
Assert: (NOT pollsActivity) OR importsResolutionHelper OR hasOptOut.

The three valid states:

(a) Does not poll /activity → invariant trivially holds.
(b) Polls AND imports resolution helpers → invariant holds.
(c) Polls AND has opt-out comment → escape hatch (see below).

Anything else → fail-CI with a structured message showing the path + each detected signal + the missing piece, so a reviewer immediately sees what's wrong without re-running locally.

Magic-comment opt-out

Intentional bypass (e.g. a logging-only inspector that polls /activity but never surfaces files to an agent) opts out by adding the comment anywhere in the file:

// @no-resolve-uploads-justification: this is a logging-only inspector

The <reason> part is informational only — the test asserts the presence of the magic-comment prefix but doesn't parse the reason. A reviewer sees the comment + reason in code review. This keeps the bypass auditable without overengineering the parser.

Test envelope (10 cases)

Producer-side mode (1):

no consumers declared (producer-side CI no-op) — passes when env var unset.

Checker self-tests via tmpdir fixtures (9):

polls + imports → passes
polls + no-imports → caught (this is the failure case the contract guards against)
polls + magic-comment opt-out → not caught
no-poll at all → trivially holds (e.g. a tool wrapper file)
subpath import @molecule-ai/mcp-server/inbox-uploads counts (Layer B exports both via root and via subpath)
false-friend /workspaces/x/activities NOT matched (boundary check)
template literal with /activity in head detected
template literal with /activity AFTER substitution span detected (catches `/workspaces/${ws}/activity?since_id=${cursor}`)
default ImportClause NOT counted as named import (sanity — import mcpserver from "@molecule-ai/mcp-server" doesn't pull resolvePendingUpload into scope)

Verified locally on operator

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

=== jest run with no env var (producer-side no-op) ===
PASS src/__tests__/poll-uploads-resolved-contract.test.ts
Test Suites: 1 passed, 1 total
Tests:       10 passed, 10 total

=== consumer gate with PASSING file ===
Tests:       10 passed, 10 total

=== consumer gate with FAILING file ===
Tests:       1 failed, 9 passed, 10 total
(failure message shows path + polls=true + imports=no + opt-out=no — clear actionable diagnostic)

The failure message format:

path: /tmp/.../failing.ts
polls /activity: true
imports resolution helper(s): no
has @no-resolve-uploads-justification: no

Reviewer reading the CI failure immediately sees what's missing without re-running.

Why AST, not regex

A regex on the file text would catch the URL string easily but FAIL on import-detection edge cases:

import { ..., resolvePendingUpload, ... } with line wrapping
import * as mcp from "..." (namespace import — does NOT pull individual names into scope, so a mcp.resolvePendingUpload reference inside the file body would still not "import" the name in the spec sense)
const { resolvePendingUpload } = require("@molecule-ai/mcp-server") (CJS destructuring — TS files can use it via ts-node etc.)

The AST-level check handles named-import statements correctly + ignores destructuring / namespace forms (intentional: only the explicit named-import is the contract signal).

Sibling reference

The env-var-driven consumer-paths design comes from CEO-A's dispatch brief citing template-claude-code/.gitea/workflows/runtime-pin-check.yml. That repo isn't available on Gitea today (cloning returns 404), so the design was reconstructed from her brief + adapted to the AST-parsing surface. If the runtime-pin-check pattern is the canonical sibling, a follow-up could harmonize the two implementations (env var name + invocation path).

Cascade context

Layer	Repo	Status
A	molecule-ai-workspace-runtime#42	open (CI running on a9323dd after fix)
B	molecule-mcp-server#26	open (CI running, 162/163 jest tests passing locally)
D (this PR)	molecule-mcp-server	now
C	molecule-mcp-claude-channel	CEO-A authors after B's npm publish

A + B + D can land in any order — they're additive at the source-code level. C waits on B's npm publish.

Test plan

tsc --noEmit clean (verified on operator).
Producer-side no-op: npx jest poll-uploads-resolved-contract → 10/10 passed without env var.
Consumer gate green: env var pointing at a passing fixture → 10/10 passed.
Consumer gate red: env var pointing at a failing fixture → 1 expected failure with clear diagnostic.
CI all-green on this branch.
2 substantive non-author reviewer approves. Per CEO-A's dispatch: core-devops + core-qa (contract-test surface — core-devops owns the CI gate semantics across producer/consumer repos; core-qa owns the test-envelope correctness across the 9 self-tests + the producer-side no-op).

🤖 Generated with Claude Code

## Summary RFC#640 4-layer cascade — **Layer D** (AST-level contract test). Single new test file at `src/__tests__/poll-uploads-resolved-contract.test.ts`. Catches the third failure surface that Layers A + B close from the spec / implementation side: an adapter that has a poll loop but forgot to wire in the upload-resolution helpers. Companion to: - Layer A (workspace-runtime#42, open) — the SSOT spec edit making upload resolution MANDATORY. - Layer B (molecule-mcp-server#26, open) — the TS implementation TS adapters consume. - Layer C (CEO-A's authoring lane after Layer B's npm publish) — channel adapter consuming Layer B. CTO chat GO: "4-layer， dispatch team and follow SOP" 2026-05-22T01:31:48Z. ## Test design: producer-side no-op gate + consumer-side enforcement The test runs in TWO modes based on `MCP_SERVER_CONTRACT_CONSUMERS` env var: **Producer-side (this repo's own CI)**: env var unset → empty consumer list → test passes trivially. No external consumers needed for the producer's CI to be green. **Consumer-side (channel adapter / telegram / codex / etc.)**: each consumer repo's CI runs the test against its own source files via: ``` MCP_SERVER_CONTRACT_CONSUMERS=src/server.ts:src/poll.ts \ npx jest --testPathPatterns=poll-uploads-resolved-contract \ --rootDir=node_modules/@molecule-ai/mcp-server ``` The contract is engaged in consumer repos via the env-driven list. Same shape as the runtime-pin-check sibling pattern CEO-A flagged. ## The invariant For each TS file in the consumer list: 1. **Parse with TS compiler API** (`ts.createSourceFile` + `ts.forEachChild`). 2. **Detect `/activity` URL literal** in any string or template literal (head + after-substitution spans). Pattern is `/workspaces/<x>/activity` with a non-word boundary after to avoid the false-friend `/activities`. 3. **Detect named imports** from `@molecule-ai/mcp-server` OR `@molecule-ai/mcp-server/inbox-uploads` of `resolvePendingUpload` / `URICache` / `rewritePendingURIs`. 4. **Assert**: `(NOT pollsActivity) OR importsResolutionHelper OR hasOptOut`. The three valid states: - (a) **Does not poll `/activity`** → invariant trivially holds. - (b) **Polls AND imports resolution helpers** → invariant holds. - (c) **Polls AND has opt-out comment** → escape hatch (see below). Anything else → fail-CI with a structured message showing the path + each detected signal + the missing piece, so a reviewer immediately sees what's wrong without re-running locally. ## Magic-comment opt-out Intentional bypass (e.g. a logging-only inspector that polls /activity but never surfaces files to an agent) opts out by adding the comment **anywhere in the file**: ```ts // @no-resolve-uploads-justification: this is a logging-only inspector ``` The `<reason>` part is informational only — the test asserts the presence of the magic-comment prefix but doesn't parse the reason. A reviewer sees the comment + reason in code review. This keeps the bypass auditable without overengineering the parser. ## Test envelope (10 cases) **Producer-side mode (1)**: - `no consumers declared (producer-side CI no-op)` — passes when env var unset. **Checker self-tests via tmpdir fixtures (9)**: - polls + imports → passes - polls + no-imports → caught (this is the failure case the contract guards against) - polls + magic-comment opt-out → not caught - no-poll at all → trivially holds (e.g. a tool wrapper file) - subpath import `@molecule-ai/mcp-server/inbox-uploads` counts (Layer B exports both via root and via subpath) - false-friend `/workspaces/x/activities` NOT matched (boundary check) - template literal with `/activity` in head detected - template literal with `/activity` AFTER substitution span detected (catches `` `/workspaces/${ws}/activity?since_id=${cursor}` ``) - default `ImportClause` NOT counted as named import (sanity — `import mcpserver from "@molecule-ai/mcp-server"` doesn't pull resolvePendingUpload into scope) ## Verified locally on operator ``` === tsc type check === (clean) === jest run with no env var (producer-side no-op) === PASS src/__tests__/poll-uploads-resolved-contract.test.ts Test Suites: 1 passed, 1 total Tests: 10 passed, 10 total === consumer gate with PASSING file === Tests: 10 passed, 10 total === consumer gate with FAILING file === Tests: 1 failed, 9 passed, 10 total (failure message shows path + polls=true + imports=no + opt-out=no — clear actionable diagnostic) ``` The failure message format: ``` path: /tmp/.../failing.ts polls /activity: true imports resolution helper(s): no has @no-resolve-uploads-justification: no ``` Reviewer reading the CI failure immediately sees what's missing without re-running. ## Why AST, not regex A regex on the file text would catch the URL string easily but FAIL on import-detection edge cases: - `import { ..., resolvePendingUpload, ... }` with line wrapping - `import * as mcp from "..."` (namespace import — does NOT pull individual names into scope, so a `mcp.resolvePendingUpload` reference inside the file body would still not "import" the name in the spec sense) - `const { resolvePendingUpload } = require("@molecule-ai/mcp-server")` (CJS destructuring — TS files can use it via ts-node etc.) The AST-level check handles named-import statements correctly + ignores destructuring / namespace forms (intentional: only the explicit named-import is the contract signal). ## Sibling reference The env-var-driven consumer-paths design comes from CEO-A's dispatch brief citing `template-claude-code/.gitea/workflows/runtime-pin-check.yml`. That repo isn't available on Gitea today (cloning returns 404), so the design was reconstructed from her brief + adapted to the AST-parsing surface. If the runtime-pin-check pattern is the canonical sibling, a follow-up could harmonize the two implementations (env var name + invocation path). ## Cascade context | Layer | Repo | Status | |---|---|---| | A | molecule-ai-workspace-runtime#42 | open (CI running on a9323dd after fix) | | B | molecule-mcp-server#26 | open (CI running, 162/163 jest tests passing locally) | | D (this PR) | molecule-mcp-server | now | | C | molecule-mcp-claude-channel | CEO-A authors after B's npm publish | A + B + D can land in any order — they're additive at the source-code level. C waits on B's npm publish. ## Test plan - [x] `tsc --noEmit` clean (verified on operator). - [x] Producer-side no-op: `npx jest poll-uploads-resolved-contract` → 10/10 passed without env var. - [x] Consumer gate green: env var pointing at a passing fixture → 10/10 passed. - [x] Consumer gate red: env var pointing at a failing fixture → 1 expected failure with clear diagnostic. - [ ] CI all-green on this branch. - [ ] 2 substantive non-author reviewer approves. Per CEO-A's dispatch: **core-devops + core-qa** (contract-test surface — core-devops owns the CI gate semantics across producer/consumer repos; core-qa owns the test-envelope correctness across the 9 self-tests + the producer-side no-op). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hongming-pc2 added 1 commit 2026-05-22 02:04:17 +00:00

test(contract): AST-level poll-uploads-resolved invariant (RFC#640 Layer D)

CI / test (pull_request) Successful in 38s

Details

8441900db3

Layer D of the 4-layer cascade — fail-CI on any TS adapter that polls
/workspaces/<ws>/activity but does NOT also import the upload-resolution
helpers from @molecule-ai/mcp-server. Catches the third failure surface
(adapter forgot to wire in the resolution flow) that Layer A's MANDATORY
spec + Layer B's TS implementation close from the spec/implementation
side.

Test design: producer-side CI no-op gate + consumer-side enforcement.

  # In a consumer repo's CI (channel adapter / telegram / codex / etc.):
  MCP_SERVER_CONTRACT_CONSUMERS=src/server.ts:src/poll.ts \
    npx jest --testPathPatterns=poll-uploads-resolved-contract \
             --rootDir=node_modules/@molecule-ai/mcp-server

When MCP_SERVER_CONTRACT_CONSUMERS is unset (this repo's own CI), the
test runs against an empty list and passes trivially. The contract is
engaged in consumer repos via the env-driven list.

Invariant per file:
  1. Parse with TS compiler API (ts.createSourceFile + ts.forEachChild).
  2. Detect /activity URL literal in any string or template literal
     (head + after-substitution spans). Pattern is /workspaces/<x>/activity
     with a non-word boundary after to avoid the false-friend /activities.
  3. Detect named imports from @molecule-ai/mcp-server OR
     @molecule-ai/mcp-server/inbox-uploads of resolvePendingUpload /
     URICache / rewritePendingURIs.
  4. Assert: (NOT pollsActivity) OR importsResolutionHelper OR hasOptOut.

Magic-comment opt-out for intentional bypass:
  // @no-resolve-uploads-justification: <reason>

Reason text is informational only (asserted in code review). The test
just looks for the prefix anywhere in the file.

Test envelope (10 cases):
  - producer-side no-op when MCP_SERVER_CONTRACT_CONSUMERS unset (1)
  - self-tests via tmpdir fixtures (9):
    - polls + imports → passes
    - polls + no-imports → caught
    - polls + opt-out → not caught
    - no-poll → trivially holds
    - subpath import @molecule-ai/mcp-server/inbox-uploads counts
    - false-friend /workspaces/x/activities NOT matched
    - template literal /activity in head detected
    - template literal /activity after substitution span detected
    - default ImportClause NOT counted as named import

Verified on operator:
  tsc --noEmit         : clean
  producer-side jest   : 10/10 passed (no-op + 9 self-tests)
  consumer gate green  : passing fixture → 10/10 passed
  consumer gate red    : failing fixture → 1 expected failure with clear msg

Sibling design reference: the runtime-pin-check pattern (CEO-A flagged
template-claude-code's runtime-pin-check.yml; that repo wasn't available
on Gitea so the env-var-driven consumer-paths shape was reconstructed
from her dispatch brief).

Origin: RFC#640 4-layer cascade Layer D. 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>

core-devops approved these changes 2026-05-22 02:15:31 +00:00

core-devops left a comment

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

Verified against head 8441900. mergeable=True. Read full +384/-0 diff (single new test file). Focus on the producer/consumer CI semantics + the env-var-driven design + the failure diagnostic shape.

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

Producer-side vs consumer-side CI semantics — clean two-mode design.

The design splits cleanly:

Producer-side (this repo's own CI): MCP_SERVER_CONTRACT_CONSUMERS is unset → the test consumers list is empty → the test runs it("no consumers declared (producer-side CI no-op)", ...) which trivially passes. No external consumers needed for the producer's CI to be green.

Consumer-side (channel adapter / telegram / codex / etc.): the consumer repo's CI runs MCP_SERVER_CONTRACT_CONSUMERS=src/server.ts:src/poll.ts npx jest --testPathPatterns=poll-uploads-resolved-contract --rootDir=node_modules/@molecule-ai/mcp-server. The contract is engaged at the consumer-side CI step.

This is the right shape for a CONTRACT test — the producer can't enumerate all consumer files (they live in different repos), but the producer ships the test code that consumer repos can opt into. Same model as the runtime-pin-check sibling pattern CEO-A flagged (which I couldn't independently inspect because template-claude-code 404s on Gitea — author noted this in the PR body).

AST-level via TS compiler API — right choice, not regex.

The implementation uses ts.createSourceFile + ts.forEachChild for parsing. Three edge cases this catches that regex would miss:

Wrapped named imports:

import {
  resolvePendingUpload,
  ...other stuff...
} from "@molecule-ai/mcp-server";

Regex would need careful multi-line handling; AST naturally parses this.

Default vs named imports:
```
import mcpserver from "@molecule-ai/mcp-server";  // does NOT pull resolvePendingUpload into scope
```
The test correctly counts only NAMED imports (ts.NamedImports), not default. A regex grep for resolvePendingUpload would silently false-positive on a default-import + later mcpserver.resolvePendingUpload(...) reference (which IS valid TS but doesn't satisfy the "explicit import" contract intent — the contract wants the name in the module's import list so a static analysis tool sees the dependency).
Template literal substitution spans:
```
const u = `/workspaces/${ws}/activity?since_id=${cursor}`;
```
The /activity literal sits in the SECOND template fragment AFTER the ${ws} substitution. The test's ts.TemplateExpression handler concatenates head + each post-substitution fragment and re-runs the pattern. Regex on the raw source might catch this via greedy matching, but AST gives semantic certainty.

URL pattern boundary check — false-friend protection.

The pattern \/workspaces\/[^/]*\/activity(?:\?|$|[^a-zA-Z0-9_/-]) requires a non-word boundary after activity. This catches:

/workspaces/x/activity ✓
/workspaces/x/activity?since_id=1 ✓
/workspaces/x/activity/foo ✓ (the / boundary)
/workspaces/x/activities ✗ (the s is a word char — correctly rejected)

The boundary test URL pattern: rejects /workspaces/X/activities (false-friend) pins this explicitly. A future refactor that simplified the boundary to just /activity would catch the false-friend and fail this test.

Magic-comment opt-out — auditable escape hatch.

The // @no-resolve-uploads-justification: <reason> pattern is:

Discoverable (any reviewer reading the file sees it)
Searchable (grep @no-resolve-uploads-justification across consumer repos)
Auditable (the reason text gets read in code review)
Not parsed (the test only checks for the prefix — reason text is free-form documentation, not machine-validated)

The OPT_OUT_COMMENT regex /\/\/\s*@no-resolve-uploads-justification:/ is permissive on whitespace; allows both // @no-resolve-... and //@no-resolve-... styles. Reasonable.

Failure-diagnostic shape — actionable.

path: /tmp/.../failing.ts
polls /activity: true
imports resolution helper(s): no
has @no-resolve-uploads-justification: no

A CI reviewer sees this and immediately knows: (a) which file failed, (b) which signal triggered the contract requirement, (c) what's missing (the import or the opt-out). No need to re-run locally to diagnose.

The Jest assertion shape expect({ok: status, info: reasonLines.join("\n ")}).toEqual({ok: true, info: ...}) is unusual but intentional — by including the info string in BOTH sides of the comparison, Jest's diff output shows the structured info verbatim. A more standard expect(status).toBe(true) would show just expected true, got false with no context.

CI invocation shape for consumer repos — documented in the file header.

MCP_SERVER_CONTRACT_CONSUMERS=src/server.ts:src/poll.ts \
  npx jest --testPathPatterns=poll-uploads-resolved-contract \
           --rootDir=node_modules/@molecule-ai/mcp-server

This is the exact invocation a consumer repo adds to its CI step. The --rootDir flag points Jest at the installed package, so the test file is found at node_modules/@molecule-ai/mcp-server/dist/__tests__/... (after Layer B's npm publish). One CI line per consumer repo; minimal integration burden.

Self-tests use tmpdir fixtures — clean test isolation.

Each self-test creates a unique tmpdir via fs.mkdtempSync, writes a fixture file, runs checkConsumerFile against it, asserts result. afterEach cleans up. No fixtures committed to the repo — tests are self-contained.

Test execution confirmed locally:

Producer-side no-op: 10/10 passed.
Consumer gate green (passing fixture): 10/10 passed.
Consumer gate red (failing fixture): 1 expected failure with clear diagnostic.

Three modes pinned end-to-end.

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

The test ships in the producer repo's own test suite (src/__tests__/). When Layer B's npm publish happens, the test file lands at dist/__tests__/poll-uploads-resolved-contract.test.ts in the published package. The README / publish-step doesn't currently exclude __tests__/ from the publish — confirmed by checking package.json files field is absent (defaults to including everything in dist/). This is INTENTIONAL for the contract-test pattern (consumers need to be able to invoke the test from their CI), but worth flagging so future package-size optimizations don't accidentally exclude __tests__/. Cheap follow-up; could be a package.json files field that explicitly includes dist/__tests__/poll-uploads-resolved-contract.test.ts. Not blocking.

No regression risks. No REQUEST_CHANGES. LGTM, approving.

**Independent non-author review (reviewer = core-devops of engineers team, author = hongming-pc2).** Verified against head `8441900`. mergeable=True. Read full +384/-0 diff (single new test file). Focus on the producer/consumer CI semantics + the env-var-driven design + the failure diagnostic shape. **Pre-flight CI gate** — `CI / test` success on `8441900`. Hygiene gate cleared. **Producer-side vs consumer-side CI semantics — clean two-mode design.** The design splits cleanly: **Producer-side (this repo's own CI)**: `MCP_SERVER_CONTRACT_CONSUMERS` is unset → the test consumers list is empty → the test runs `it("no consumers declared (producer-side CI no-op)", ...)` which trivially passes. No external consumers needed for the producer's CI to be green. **Consumer-side (channel adapter / telegram / codex / etc.)**: the consumer repo's CI runs `MCP_SERVER_CONTRACT_CONSUMERS=src/server.ts:src/poll.ts npx jest --testPathPatterns=poll-uploads-resolved-contract --rootDir=node_modules/@molecule-ai/mcp-server`. The contract is engaged at the consumer-side CI step. This is the right shape for a CONTRACT test — the producer can't enumerate all consumer files (they live in different repos), but the producer ships the test code that consumer repos can opt into. Same model as the runtime-pin-check sibling pattern CEO-A flagged (which I couldn't independently inspect because `template-claude-code` 404s on Gitea — author noted this in the PR body). **AST-level via TS compiler API — right choice, not regex.** The implementation uses `ts.createSourceFile` + `ts.forEachChild` for parsing. Three edge cases this catches that regex would miss: 1. **Wrapped named imports**: ```ts import { resolvePendingUpload, ...other stuff... } from "@molecule-ai/mcp-server"; ``` Regex would need careful multi-line handling; AST naturally parses this. 2. **Default vs named imports**: ```ts import mcpserver from "@molecule-ai/mcp-server"; // does NOT pull resolvePendingUpload into scope ``` The test correctly counts only NAMED imports (`ts.NamedImports`), not default. A regex grep for `resolvePendingUpload` would silently false-positive on a default-import + later `mcpserver.resolvePendingUpload(...)` reference (which IS valid TS but doesn't satisfy the "explicit import" contract intent — the contract wants the name in the module's import list so a static analysis tool sees the dependency). 3. **Template literal substitution spans**: ```ts const u = `/workspaces/${ws}/activity?since_id=${cursor}`; ``` The `/activity` literal sits in the SECOND template fragment AFTER the `${ws}` substitution. The test's `ts.TemplateExpression` handler concatenates head + each post-substitution fragment and re-runs the pattern. Regex on the raw source might catch this via greedy matching, but AST gives semantic certainty. **URL pattern boundary check — false-friend protection.** The pattern `\/workspaces\/[^/]*\/activity(?:\?|$|[^a-zA-Z0-9_/-])` requires a non-word boundary after `activity`. This catches: - `/workspaces/x/activity` ✓ - `/workspaces/x/activity?since_id=1` ✓ - `/workspaces/x/activity/foo` ✓ (the `/` boundary) - `/workspaces/x/activities` ✗ (the `s` is a word char — correctly rejected) The boundary test `URL pattern: rejects /workspaces/X/activities (false-friend)` pins this explicitly. A future refactor that simplified the boundary to just `/activity` would catch the false-friend and fail this test. **Magic-comment opt-out — auditable escape hatch.** The `// @no-resolve-uploads-justification: <reason>` pattern is: - Discoverable (any reviewer reading the file sees it) - Searchable (`grep @no-resolve-uploads-justification` across consumer repos) - Auditable (the reason text gets read in code review) - Not parsed (the test only checks for the prefix — reason text is free-form documentation, not machine-validated) The OPT_OUT_COMMENT regex `/\/\/\s*@no-resolve-uploads-justification:/` is permissive on whitespace; allows both `// @no-resolve-...` and `//@no-resolve-...` styles. Reasonable. **Failure-diagnostic shape — actionable.** ``` path: /tmp/.../failing.ts polls /activity: true imports resolution helper(s): no has @no-resolve-uploads-justification: no ``` A CI reviewer sees this and immediately knows: (a) which file failed, (b) which signal triggered the contract requirement, (c) what's missing (the import or the opt-out). No need to re-run locally to diagnose. The Jest assertion shape `expect({ok: status, info: reasonLines.join("\n ")}).toEqual({ok: true, info: ...})` is unusual but intentional — by including the info string in BOTH sides of the comparison, Jest's diff output shows the structured info verbatim. A more standard `expect(status).toBe(true)` would show just `expected true, got false` with no context. **CI invocation shape for consumer repos — documented in the file header.** ``` MCP_SERVER_CONTRACT_CONSUMERS=src/server.ts:src/poll.ts \ npx jest --testPathPatterns=poll-uploads-resolved-contract \ --rootDir=node_modules/@molecule-ai/mcp-server ``` This is the exact invocation a consumer repo adds to its CI step. The `--rootDir` flag points Jest at the installed package, so the test file is found at `node_modules/@molecule-ai/mcp-server/dist/__tests__/...` (after Layer B's npm publish). One CI line per consumer repo; minimal integration burden. **Self-tests use tmpdir fixtures — clean test isolation.** Each self-test creates a unique tmpdir via `fs.mkdtempSync`, writes a fixture file, runs `checkConsumerFile` against it, asserts result. `afterEach` cleans up. No fixtures committed to the repo — tests are self-contained. **Test execution confirmed locally**: - Producer-side no-op: 10/10 passed. - Consumer gate green (passing fixture): 10/10 passed. - Consumer gate red (failing fixture): 1 expected failure with clear diagnostic. Three modes pinned end-to-end. **One non-blocking observation** (logged-but-NOT-blocking-merge): The test ships in the producer repo's own test suite (`src/__tests__/`). When Layer B's npm publish happens, the test file lands at `dist/__tests__/poll-uploads-resolved-contract.test.ts` in the published package. The README / publish-step doesn't currently exclude `__tests__/` from the publish — confirmed by checking `package.json` `files` field is absent (defaults to including everything in `dist/`). This is INTENTIONAL for the contract-test pattern (consumers need to be able to invoke the test from their CI), but worth flagging so future package-size optimizations don't accidentally exclude `__tests__/`. Cheap follow-up; could be a `package.json` `files` field that explicitly includes `dist/__tests__/poll-uploads-resolved-contract.test.ts`. Not blocking. **No regression risks. No REQUEST_CHANGES. LGTM, approving.**

core-qa approved these changes 2026-05-22 02:16:12 +00:00

core-qa left a comment

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

Verified against head 8441900. mergeable=True. Read full +384/-0 diff. Focus on test-envelope completeness for the contract checker logic + the per-mode coverage.

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

Test envelope structure — 10 cases across 2 surfaces (producer-side / self-tests).

The test file has TWO describe blocks:

"RFC#640 Layer D — poll-uploads-resolved contract" — the actual contract gate. Producer-side mode (when env var unset) runs ONE test (the no-op). Consumer-side mode (env var set) runs ONE test per consumer file.
"RFC#640 Layer D — checker self-tests" — 9 self-tests against tmpdir fixtures exercising the checkConsumerFile analysis function directly.

The split is correct: outer is the GATE (engaged via env var), inner is the CHECKER (exercised regardless of env). Two surfaces with different jobs in the same file.

Self-tests cover the checker's full decision-space.

I traced each self-test's catch-target:

polls + imports → passes: happy path. Catches: a refactor that broke the import-detection logic (would now fail this when it shouldn't).
polls + no-imports → caught: load-bearing failure case. The whole point of Layer D. Without this test, a refactor that silently passed all files would slip through.
polls + opt-out → not caught: escape-hatch sanity. Without this, a refactor that ignored the opt-out comment would block intentional opt-outs.
no-poll → trivially holds: a tool wrapper file (e.g. tools/workspaces.ts that doesn't poll /activity at all) doesn't need to import resolution helpers. Without this test, the checker might over-zealously fail tool files.
subpath import counts: import { URICache } from "@molecule-ai/mcp-server/inbox-uploads" (the subpath form Layer B exports). Both root + subpath are valid signals.
false-friend /workspaces/x/activities NOT matched: boundary check. Without this, apiCall("GET", "/workspaces/x/activities") would falsely trigger the contract requirement on a hypothetical future endpoint named activities.
template literal /activity in head detected: covers the `/workspaces/${ws}/activity` case.
template literal /activity AFTER substitution span detected: covers `/workspaces/${ws}/activity?since_id=${cursor}` — the multi-segment template literal case. The /activity literal sits in the second fragment after ${ws}; without the multi-fragment walk, this would slip through.
default ImportClause NOT counted: import mcpserver from "@molecule-ai/mcp-server" (default import) does NOT pull individual names into scope. Even though mcpserver.resolvePendingUpload is a valid runtime reference, the contract intent is "explicit named import in the file's import list" — so default forms should NOT satisfy the contract. Without this test, the checker might silently accept default imports.

Producer-side no-op test — meaningful assertion.

it("no consumers declared (producer-side CI no-op)", () => {
  expect(consumers.length).toBe(0);
});

This asserts that the producer-side run has an EMPTY consumer list. If someone accidentally hard-coded a consumer path into the test file (e.g. for local development), the producer's own CI would catch it via this test failure. Defensive assertion, not just a passes-trivially placeholder.

Test isolation via tmpdir — clean.

beforeEach creates a unique tmpdir via fs.mkdtempSync(path.join(os.tmpdir(), "layer-d-self-")). afterEach removes it with fs.rmSync(..., {recursive:true, force:true}). Each test writes its fixture into the tmpdir + runs the checker. No fixture files committed to the repo + no cross-test interference.

Failure-message format — actionable for CI reviewer.

The assertion shape:

expect({ ok: status, info: reasonLines.join("\n  ") }).toEqual({
  ok: true,
  info: reasonLines.join("\n  "),
});

Including the info string in BOTH sides of the comparison is a clever trick: when Jest renders the diff, the info text appears in the expected/received frames, so the reviewer sees the structured diagnostic (path / polls / imports / opt-out) verbatim. A simple expect(status).toBe(true) would render as Expected true, Received false with no context.

Coverage gap check — what's NOT tested but matters?

The test covers the checker's import + URL detection but doesn't test consumer-side mode with REAL multi-file iteration (e.g. MCP_SERVER_CONTRACT_CONSUMERS=a:b:c). The describe-block construction creates one nested describe per consumer; the test logic is identical per-file. Adding a multi-file iteration test would be a small +20 LOC addition. Not blocking — the per-file logic IS the per-file invariant; testing it once with one file covers the core path.
The test doesn't pin behavior on a SYNTACTICALLY INVALID TS file (e.g. import { with an unclosed brace). ts.createSourceFile handles this gracefully (returns a partial AST with diagnostics); the visit walk continues on what it can parse. A defensive assertion that the test PASSES (doesn't throw) on a broken consumer file would harden the failure-mode docs. Cheap follow-up; not blocking.

Test execution confirmed locally:

producer-side mode:  10/10 passed (0.496s)
consumer green:      10/10 passed (passing fixture)
consumer red:        1 failed, 9 passed (failing fixture; structured diagnostic emitted)

All three modes work end-to-end.

Consistency with Layer A's structural tests.

This test pins the IMPLEMENTATION side; Layer A's test_upload_resolution_contract.py pins the SPEC side; together they catch drift from either direction. Plus this PR's checker IS the CI gate that fires on consumer-side drift — three-tier enforcement (spec text + implementation surface + consumer adoption).

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

The 9 self-tests are nested inside a single describe("RFC#640 Layer D — checker self-tests", ...). With the producer-side mode test as a sibling, the test runner reports 1 + 9 = 10 cases. If Jest's --testNamePattern ever needed to skip just the self-tests (e.g. during a fast CI tier), the pattern would need to match the full nested describe path. Not an issue today; could be addressed by splitting into two test files if test-filtering granularity becomes important. 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 `8441900`. mergeable=True. Read full +384/-0 diff. Focus on test-envelope completeness for the contract checker logic + the per-mode coverage. **Pre-flight CI gate** — `CI / test` success on `8441900`. Hygiene gate cleared. **Test envelope structure — 10 cases across 2 surfaces (producer-side / self-tests).** The test file has TWO `describe` blocks: 1. `"RFC#640 Layer D — poll-uploads-resolved contract"` — the actual contract gate. Producer-side mode (when env var unset) runs ONE test (the no-op). Consumer-side mode (env var set) runs ONE test per consumer file. 2. `"RFC#640 Layer D — checker self-tests"` — 9 self-tests against tmpdir fixtures exercising the `checkConsumerFile` analysis function directly. The split is correct: outer is the GATE (engaged via env var), inner is the CHECKER (exercised regardless of env). Two surfaces with different jobs in the same file. **Self-tests cover the checker's full decision-space.** I traced each self-test's catch-target: - **polls + imports → passes**: happy path. Catches: a refactor that broke the import-detection logic (would now fail this when it shouldn't). - **polls + no-imports → caught**: load-bearing failure case. The whole point of Layer D. Without this test, a refactor that silently passed all files would slip through. - **polls + opt-out → not caught**: escape-hatch sanity. Without this, a refactor that ignored the opt-out comment would block intentional opt-outs. - **no-poll → trivially holds**: a tool wrapper file (e.g. `tools/workspaces.ts` that doesn't poll /activity at all) doesn't need to import resolution helpers. Without this test, the checker might over-zealously fail tool files. - **subpath import counts**: `import { URICache } from "@molecule-ai/mcp-server/inbox-uploads"` (the subpath form Layer B exports). Both root + subpath are valid signals. - **false-friend /workspaces/x/activities NOT matched**: boundary check. Without this, `apiCall("GET", "/workspaces/x/activities")` would falsely trigger the contract requirement on a hypothetical future endpoint named `activities`. - **template literal /activity in head detected**: covers the `` `/workspaces/${ws}/activity` `` case. - **template literal /activity AFTER substitution span detected**: covers `` `/workspaces/${ws}/activity?since_id=${cursor}` `` — the multi-segment template literal case. The `/activity` literal sits in the second fragment after `${ws}`; without the multi-fragment walk, this would slip through. - **default ImportClause NOT counted**: `import mcpserver from "@molecule-ai/mcp-server"` (default import) does NOT pull individual names into scope. Even though `mcpserver.resolvePendingUpload` is a valid runtime reference, the contract intent is "explicit named import in the file's import list" — so default forms should NOT satisfy the contract. Without this test, the checker might silently accept default imports. **Producer-side no-op test — meaningful assertion.** ```ts it("no consumers declared (producer-side CI no-op)", () => { expect(consumers.length).toBe(0); }); ``` This asserts that the producer-side run has an EMPTY consumer list. If someone accidentally hard-coded a consumer path into the test file (e.g. for local development), the producer's own CI would catch it via this test failure. Defensive assertion, not just a passes-trivially placeholder. **Test isolation via tmpdir — clean.** `beforeEach` creates a unique tmpdir via `fs.mkdtempSync(path.join(os.tmpdir(), "layer-d-self-"))`. `afterEach` removes it with `fs.rmSync(..., {recursive:true, force:true})`. Each test writes its fixture into the tmpdir + runs the checker. No fixture files committed to the repo + no cross-test interference. **Failure-message format — actionable for CI reviewer.** The assertion shape: ```ts expect({ ok: status, info: reasonLines.join("\n ") }).toEqual({ ok: true, info: reasonLines.join("\n "), }); ``` Including the info string in BOTH sides of the comparison is a clever trick: when Jest renders the diff, the info text appears in the expected/received frames, so the reviewer sees the structured diagnostic (path / polls / imports / opt-out) verbatim. A simple `expect(status).toBe(true)` would render as `Expected true, Received false` with no context. **Coverage gap check** — what's NOT tested but matters? - The test covers the checker's import + URL detection but doesn't test consumer-side mode with REAL multi-file iteration (e.g. `MCP_SERVER_CONTRACT_CONSUMERS=a:b:c`). The describe-block construction creates one nested describe per consumer; the test logic is identical per-file. Adding a multi-file iteration test would be a small +20 LOC addition. Not blocking — the per-file logic IS the per-file invariant; testing it once with one file covers the core path. - The test doesn't pin behavior on a SYNTACTICALLY INVALID TS file (e.g. `import {` with an unclosed brace). `ts.createSourceFile` handles this gracefully (returns a partial AST with diagnostics); the visit walk continues on what it can parse. A defensive assertion that the test PASSES (doesn't throw) on a broken consumer file would harden the failure-mode docs. Cheap follow-up; not blocking. **Test execution confirmed locally**: ``` producer-side mode: 10/10 passed (0.496s) consumer green: 10/10 passed (passing fixture) consumer red: 1 failed, 9 passed (failing fixture; structured diagnostic emitted) ``` All three modes work end-to-end. **Consistency with Layer A's structural tests.** This test pins the IMPLEMENTATION side; Layer A's `test_upload_resolution_contract.py` pins the SPEC side; together they catch drift from either direction. Plus this PR's checker IS the CI gate that fires on consumer-side drift — three-tier enforcement (spec text + implementation surface + consumer adoption). **One non-blocking observation** (logged-but-NOT-blocking-merge): The 9 self-tests are nested inside a single `describe("RFC#640 Layer D — checker self-tests", ...)`. With the producer-side mode test as a sibling, the test runner reports 1 + 9 = 10 cases. If Jest's `--testNamePattern` ever needed to skip just the self-tests (e.g. during a fast CI tier), the pattern would need to match the full nested describe path. Not an issue today; could be addressed by splitting into two test files if test-filtering granularity becomes important. Not blocking. **No regression risks. No REQUEST_CHANGES. LGTM, approving.**

plugin-dev merged commit 7dc978f60c into main

2026-05-22 02:34:55 +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#27