Merge registry-envelope-integration into main (resolve conflicts with call-envelope-integration)
This commit is contained in:
@@ -1,54 +0,0 @@
|
||||
---
|
||||
id: bug-pendingrequestmap-type-conflict
|
||||
name: Fix PendingRequestMap type name conflict between env.ts interface and call.ts class
|
||||
status: completed
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
There is a naming conflict documented in call-protocol.md § Bugs:
|
||||
|
||||
- `src/env.ts` exports a `PendingRequestMap` **interface** (reduced signature: missing `deadline`, `identity` typed as `unknown`)
|
||||
- `src/call.ts` exports the `PendingRequestMap` **class** (full signature with all options)
|
||||
- `src/index.ts` re-exports the interface as `PendingRequestMap` and the class as `PendingRequestMapClass`
|
||||
|
||||
The documented `PendingRequestMap` in the architecture refers to the **class**, but importing the type gives the reduced interface. This creates confusion for consumers.
|
||||
|
||||
The fix should:
|
||||
1. Remove the `PendingRequestMap` interface from `env.ts` entirely
|
||||
2. Rename the class export in `index.ts` from `PendingRequestMapClass` to just `PendingRequestMap` (both the class and type export)
|
||||
3. Update the `EnvOptions.callMap` type to reference the actual class or a proper minimal interface
|
||||
|
||||
This is independent of ADR-005/006 and can be fixed immediately. Note: ADR-005 will later change the `call()` return type from `Promise<unknown>` to `Promise<ResponseEnvelope>`, but fixing the naming conflict now is clean and doesn't block on that.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [x] `PendingRequestMap` interface removed from `env.ts`
|
||||
- [x] `index.ts` exports the class as `PendingRequestMap` (no `PendingRequestMapClass` alias)
|
||||
- [x] `EnvOptions.callMap` type uses the actual `PendingRequestMap` class type or a proper minimal interface
|
||||
- [x] Existing tests pass
|
||||
- [x] No consumer-visible breaking change (the class has all methods the interface had, plus more)
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/call-protocol.md § Bugs (PendingRequestMap type name conflict)
|
||||
- src/env.ts:8-9 (interface definition)
|
||||
- src/index.ts:6,14 (re-exports)
|
||||
- src/call.ts:68 (class definition)
|
||||
|
||||
## Notes
|
||||
|
||||
Renamed the interface to `CallMap` instead of completely removing it, since `EnvOptions.callMap` needs a type for dependency injection (not every consumer needs the full `PendingRequestMap` class — they may provide a minimal call-map-like object). The `CallMap` interface now matches the full `call()` signature of the class (including `deadline` and properly typed `identity`).
|
||||
|
||||
## Summary
|
||||
|
||||
Resolved the PendingRequestMap type name conflict. Changes:
|
||||
- Removed `PendingRequestMap` interface from `env.ts` and replaced it with `CallMap` interface that matches the full `call()` signature (including `deadline` and properly typed `Identity` parameters)
|
||||
- Updated `EnvOptions.callMap` to use `CallMap` type
|
||||
- Updated `index.ts` to export `CallMap` type from `env.js` (instead of `PendingRequestMap`) and export the `PendingRequestMap` class directly (instead of as `PendingRequestMapClass`)
|
||||
- Build, lint, and all 75 tests pass
|
||||
@@ -1,41 +0,0 @@
|
||||
---
|
||||
id: docs-cleanup-bugs
|
||||
name: Remove resolved bug documentation from call-protocol.md
|
||||
status: completed
|
||||
depends_on: [bug-checkaccess-resource-bypass, bug-pendingrequestmap-type-conflict]
|
||||
scope: single
|
||||
risk: trivial
|
||||
impact: isolated
|
||||
level: review
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
After the two independent bugs are fixed in code, the "Bugs" section in `call-protocol.md` § Source vs. Spec Drift should be removed. These bugs will no longer exist as drift — they'll be resolved.
|
||||
|
||||
Specifically, remove:
|
||||
|
||||
1. The `checkAccess()` resource check bypass bug entry
|
||||
2. The `PendingRequestMap` type name conflict bug entry
|
||||
|
||||
These are standalone fixes that don't depend on ADR-005 or ADR-006, so they can be cleaned up immediately after implementation.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] "Bugs" subsection removed from `call-protocol.md` § Source vs. Spec Drift
|
||||
- [ ] No other doc sections reference these bugs as unresolved
|
||||
- [ ] `call-protocol.md` still has the ADR-005 and ADR-006 drift tables (those are not yet resolved)
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/call-protocol.md § Source vs. Spec Drift → Bugs
|
||||
|
||||
## Notes
|
||||
|
||||
Both bugs (checkAccess resource bypass and PendingRequestMap type name conflict) have been resolved in prior tasks. The Bugs subsection was the only remaining reference.
|
||||
|
||||
## Summary
|
||||
|
||||
Removed the "Bugs" subsection from `call-protocol.md` § Source vs. Spec Drift and updated the introductory sentence to remove the "Bug" mention. ADR-005 and ADR-006 drift tables remain intact.
|
||||
- Modified: `docs/architecture/call-protocol.md`
|
||||
- All acceptance criteria verified: Bugs section removed, no other unresolved references, ADR drift tables preserved
|
||||
@@ -1,92 +0,0 @@
|
||||
---
|
||||
id: mcp-envelope-integration
|
||||
name: Update from_mcp adapter to use mcpEnvelope, structuredContent, and outputSchema extraction
|
||||
status: completed
|
||||
depends_on: [response-envelope-types]
|
||||
scope: moderate
|
||||
risk: medium
|
||||
impact: component
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Update `src/from_mcp.ts` to align with the response envelope specification documented in `response-envelopes.md` § MCP Adapter, `adapters.md` § from_mcp, and the drift tables in `call-protocol.md` and `api-surface.md`.
|
||||
|
||||
Changes needed (current source → target):
|
||||
|
||||
| What | Current source | Target |
|
||||
|------|---------------|--------|
|
||||
| `outputSchema` at discovery time | `Type.Unknown()` for all tools | `tool.outputSchema ? FromSchema(tool.outputSchema) : Type.Unknown()` |
|
||||
| Handler return value | Returns `result.content` (line 79) | Returns `mcpEnvelope(data, meta)` |
|
||||
| `structuredContent` | Not used | Prefer as `data` when present; fall back to `mapMCPContentBlocks(result.content)` |
|
||||
| `isError` handling | Throws `Error` (line 76) | Wraps in envelope with `meta.isError: true`, does NOT throw |
|
||||
| `Value.Cast()` | Not used | If `structuredContent && outputSchema !== Unknown`, cast `Value.Cast(outputSchema, structuredContent)` |
|
||||
|
||||
Implementation details:
|
||||
|
||||
1. **`outputSchema` extraction**: When `tool.outputSchema` is present (MCP spec 2025-06-18+), convert it to TypeBox via `FromSchema(tool.outputSchema)`. Fall back to `Type.Unknown()`.
|
||||
|
||||
2. **Handler behavior**: The handler should use `mcpEnvelope()` to wrap results:
|
||||
```ts
|
||||
handler: async (input, context) => {
|
||||
const result = await client.callTool({ name: tool.name, arguments: input })
|
||||
const data = result.structuredContent
|
||||
? (spec.outputSchema !== Type.Unknown()
|
||||
? Value.Cast(spec.outputSchema, result.structuredContent)
|
||||
: result.structuredContent)
|
||||
: mapMCPContentBlocks(result.content)
|
||||
return mcpEnvelope(data, {
|
||||
isError: result.isError ?? false,
|
||||
content: mapMCPContentBlocks(result.content),
|
||||
structuredContent: result.structuredContent,
|
||||
_meta: result._meta,
|
||||
})
|
||||
}
|
||||
```
|
||||
|
||||
3. **`mapMCPContentBlocks()`**: A helper function that maps SDK `ContentBlock[]` to our `MCPContentBlock[]` types. Unknown block types are mapped to `{ type: "text", text: JSON.stringify(block) }` as fallback.
|
||||
|
||||
4. **`isError` no longer throws**: MCP errors are represented as data, not thrown exceptions. Only transport-level errors (connection failure, tool not found) throw.
|
||||
|
||||
5. **The tool iteration** needs to capture `outputSchema` per tool since the handler closure needs it for `Value.Cast`.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `tool.outputSchema` is converted via `FromSchema()` when present; falls back to `Type.Unknown()`
|
||||
- [ ] Handler returns `mcpEnvelope()` — never raw content and never throws on `isError`
|
||||
- [ ] `structuredContent` is preferred as data when present
|
||||
- [ ] `mapMCPContentBlocks()` helper maps SDK content blocks to our `MCPContentBlock[]` types
|
||||
- [ ] Unknown MCP content block types fall back to `{ type: "text", text: JSON.stringify(block) }`
|
||||
- [ ] `Value.Cast()` normalization applied when `outputSchema !== Type.Unknown()` and `structuredContent` is present
|
||||
- [ ] `isError: true` results are wrapped in envelope, not thrown
|
||||
- [ ] Transport-level errors (connection, etc.) still throw `CallError`
|
||||
- [ ] `MCPClientWrapper.tools` type still returns `Array<OperationSpec & { handler: OperationHandler }>`
|
||||
- [ ] Existing tests updated; new tests for envelope wrapping, structuredContent, isError handling
|
||||
- [ ] `npm run build` passes, `npm run lint` passes, `npm test` passes
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/adapters.md § from_mcp (Implementation changes needed table)
|
||||
- docs/architecture/response-envelopes.md § MCP Adapter, § Handler behavior change
|
||||
- docs/architecture/api-surface.md § Source vs. Spec Drift (from_mcp row)
|
||||
- src/from_mcp.ts
|
||||
|
||||
## Notes
|
||||
|
||||
Merged `feat/response-envelope-types` branch first to get `ResponseEnvelope` types. The `Kind` symbol from `@alkdev/typebox` is used to detect `Type.Unknown()` schema at runtime, with a fallback to empty-object detection.
|
||||
|
||||
## Summary
|
||||
|
||||
Implemented MCP adapter envelope integration in `src/from_mcp.ts`:
|
||||
- Created: `test/from_mcp.test.ts` (20 tests)
|
||||
- Modified: `src/from_mcp.ts`
|
||||
- `outputSchema` extraction from `tool.outputSchema` via `FromSchema()`, falls back to `Type.Unknown()`
|
||||
- Handler returns `mcpEnvelope()` with structured/legacy data path
|
||||
- `structuredContent` preferred as `data` when present, `Value.Cast()` applied when `outputSchema` is not Unknown
|
||||
- `isError: true` wrapped in envelope meta, NOT thrown
|
||||
- Transport-level config errors throw `CallError`
|
||||
- Added `mapMCPContentBlocks()` exported helper mapping SDK `ContentBlock[]` to our `MCPContentBlock[]`
|
||||
- Unknown block types fall back to `{ type: "text", text: JSON.stringify(block) }`
|
||||
- Removed unused `OperationContext` import
|
||||
- Build, lint, and all 95 tests pass
|
||||
@@ -1,86 +0,0 @@
|
||||
---
|
||||
id: call-envelope-integration
|
||||
name: Update PendingRequestMap and CallHandler for ResponseEnvelope
|
||||
status: completed
|
||||
depends_on: [response-envelope-tests]
|
||||
scope: broad
|
||||
risk: medium
|
||||
impact: project
|
||||
level: implementation
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Update `src/call.ts` to integrate `ResponseEnvelope` throughout the call protocol. This covers all changes documented in call-protocol.md and api-surface.md's "Source vs. Spec Drift" sections for ADR-005.
|
||||
|
||||
Changes needed:
|
||||
|
||||
### `CallEventSchema["call.responded"]`
|
||||
- Change `output` from `Type.Unknown()` to `ResponseEnvelopeSchema`
|
||||
|
||||
### `PendingRequestMap.respond()`
|
||||
- Add `isResponseEnvelope()` guard — throws if `output` is not a valid envelope
|
||||
- This enforces the invariant that all call protocol responses carry source metadata
|
||||
|
||||
### `PendingRequestMap.call()`
|
||||
- Return type changes from `Promise<unknown>` to `Promise<ResponseEnvelope>`
|
||||
- Resolution: the `call.responded` subscription handler resolves with the `ResponseEnvelope` from `output` field (which is already validated by `respond()`)
|
||||
|
||||
### `CallHandler`
|
||||
- **Handler model change**: Handler returns a value; `CallHandler` wraps and publishes. Handler does NOT publish `call.responded` itself.
|
||||
- Capture handler return value
|
||||
- Apply shared result pipeline:
|
||||
1. Detect: `isResponseEnvelope(result)` → pass through
|
||||
2. Wrap: `localEnvelope(result, operationId)`
|
||||
3. Normalize: `Value.Cast(spec.outputSchema, envelope.data)` when `outputSchema !== Type.Unknown()`
|
||||
4. Validate: `collectErrors` on `envelope.data` — warning-only
|
||||
- Publish `call.responded` via `callMap.respond(requestId, envelope)`
|
||||
- On handler exception: `mapError()` converts to `CallError`, publish `call.error`
|
||||
|
||||
**Note**: Access control in `CallHandler` stays as-is (checking `identity` and `checkAccess`). ADR-006's change to make `CallHandler` call `execute()` is a separate task.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [x] `CallEventSchema["call.responded"].output` is `ResponseEnvelopeSchema`
|
||||
- [x] `PendingRequestMap.respond()` validates `output` with `isResponseEnvelope()`, throws on raw values
|
||||
- [x] `PendingRequestMap.call()` return type is `Promise<ResponseEnvelope>`
|
||||
- [x] `CallHandler` captures handler return value instead of discarding it
|
||||
- [x] `CallHandler` applies result pipeline: detect → wrap → normalize → validate
|
||||
- [x] `CallHandler` publishes `call.responded` via `callMap.respond()` with the envelope
|
||||
- [x] `CallHandler` on handler exception publishes `call.error` (not re-throws)
|
||||
- [x] Adapter handlers (pre-built envelopes via `mcpEnvelope`/`httpEnvelope`) pass through via `isResponseEnvelope()`
|
||||
- [x] Existing `call.test.ts` tests updated for new return types and behavior
|
||||
- [x] New tests for: envelope validation in `respond()`, envelope wrapping in `CallHandler`, envelope passthrough, Value.Cast normalization
|
||||
- [x] `npm run build` passes
|
||||
- [x] `npm run lint` passes
|
||||
- [x] `npm test` passes
|
||||
|
||||
## References
|
||||
|
||||
- docs/architecture/call-protocol.md § Source vs. Spec Drift (ADR-005 items)
|
||||
- docs/architecture/api-surface.md § Source vs. Spec Drift (ADR-005 items)
|
||||
- docs/architecture/response-envelopes.md § CallHandler, § PendingRequestMap
|
||||
- src/call.ts
|
||||
|
||||
## Notes
|
||||
|
||||
`CallHandlerConfig` changed from `{ registry, eventTarget? }` to `{ registry, callMap? }` to support publishing `call.responded` via `callMap.respond()`. When `callMap` is not provided, errors are thrown directly (backward-compatible for direct use without the call protocol).
|
||||
|
||||
`Value.Cast` in TypeBox does not strip excess properties from objects — it only fills defaults and upcasts values. The test was updated to verify default-filling behavior rather than property stripping.
|
||||
|
||||
## Summary
|
||||
|
||||
Integrated ResponseEnvelope throughout the call protocol in `src/call.ts`.
|
||||
|
||||
- Modified: `src/call.ts` (52 lines changed)
|
||||
- `CallEventSchema["call.responded"].output` → `ResponseEnvelopeSchema`
|
||||
- `PendingRequest.call()` → `Promise<ResponseEnvelope>`
|
||||
- `PendingRequestMap.respond()` → validates with `isResponseEnvelope()`, throws on raw values
|
||||
- `CallHandler` → captures return value, applies detect→wrap→normalize→validate pipeline, publishes via `callMap`
|
||||
- `CallHandlerConfig` → `{ registry, callMap? }` replacing `eventTarget?`
|
||||
- Added imports: `KindGuard`, `Value`, `collectErrors`, `formatValueErrors`, `ResponseEnvelopeSchema`, `isResponseEnvelope`, `localEnvelope`
|
||||
- Modified: `test/call.test.ts` (547 lines added)
|
||||
- Updated existing tests to pass `ResponseEnvelope` to `respond()`
|
||||
- Added 23 new tests: envelope validation in `respond()`, envelope wrapping in `CallHandler`, envelope passthrough (mcp/http), `Value.Cast` normalization, error publishing, and no-callMap fallback behavior
|
||||
- All 189 tests passing
|
||||
- Build and lint passing
|
||||
Reference in New Issue
Block a user