SessionFs structured error contract + workspace E2E tests#1102
SessionFs structured error contract + workspace E2E tests#1102SteveSandersonMS wants to merge 10 commits intomainfrom
Conversation
…e tests
- Regenerate rpc.ts from updated api.schema.json with structured error type
(error?: { code: 'ENOENT' | 'UNKNOWN', message?: string }) on all sessionFs results
- Update createTestSessionFsHandler to catch fs errors and return error objects
instead of throwing (matches the new RPC contract)
- Add E2E tests for workspace metadata and plan.md written via sessionFs
- Add test snapshots for new workspace tests
Depends on: github/copilot-agent-runtime#6479
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ae9d745 to
b39f0de
Compare
Two issues caused duplicate type generation in json-schema-to-typescript: 1. $ ef objects with sibling keywords (e.g. description) were treated as new inline types instead of reusing the referenced definition. Fix: strip all sibling keywords from $ ef objects in normalizeSchemaForTypeScript. 2. withSharedDefinitions copies definitions into $defs when $defs is empty, then normalizeSchemaForTypeScript aliased collisions as $defs_X. Since definitions entries go through the full pipeline (title-stripping, ref-reuse) but $defs copies don't, they diverge. Fix: when a key exists in both, prefer the definitions entry and drop the $defs duplicate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The runtime schema uses anyOf [{not:{}}, {$ref: ...}] for optional
results (e.g., writeFile returns SessionFsError or undefined).
Previously all 4 codegen scripts only recognized {type: 'null'} as
null-like, producing incorrect wrapper types.
Changes:
- utils.ts: Add getNullableInner() to detect {not:{}} null patterns
- typescript.ts: Emit 'T | undefined' for nullable results
- csharp.ts: Emit 'Task<T?>' for nullable results
- python.ts: Emit 'T | None' for nullable results
- go.ts: Emit '*T' (pointer) for nullable results
- Regenerated all 4 languages
- Fixed C# test handler to use Task<SessionFsError?>
- Fixed Go session.go Vision type rename
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduce a SessionFsProvider base class/interface in each language that wraps the generated SessionFsHandler RPC contract with idiomatic patterns: - C#: Abstract class with virtual methods that throw on error - Go: Interface with (value, error) returns; unexported adapter - TypeScript: Interface + createSessionFsAdapter() wrapper function - Python: ABC + create_session_fs_adapter() wrapper function Each adapter catches exceptions/errors and converts them to SessionFsError results (ENOENT/UNKNOWN). Public APIs now use SessionFsProvider exclusively. Also fixes C# serialization crash (CommonErrorData not registered in source-generated JSON context) and adds shared Warning-level logging to all C# E2E tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| session_event_from_dict, | ||
| ) | ||
| from .tools import Tool, ToolHandler, ToolInvocation, ToolResult | ||
| from .session_fs_provider import SessionFsProvider |
The _dispatch_request method was converting None handler results to {}
(empty dict) before sending JSON-RPC responses. The runtime expects null
for void operations (mkdir, writeFile, appendFile). Receiving {} caused
the runtime to fall back to local filesystem for subsequent writes.
Also removes debug tracing from session_fs_provider.py adapter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the SDKs/codegen to support the new structured sessionFs error contract (returning classified errors as part of results instead of relying on exception serialization), and adds E2E coverage asserting workspace state is persisted through sessionFs.
Changes:
- Regenerates RPC types and updates codegen to better handle nullable
anyOfresult wrappers and definition deduplication. - Introduces idiomatic
SessionFsProviderabstractions + adapters (Node/Python/Go/.NET) that translate thrown/native errors into the structuredSessionFsErrorcontract. - Adds new E2E tests + snapshots verifying
workspace.yamlandplan.mdare written viasessionFs.
Show a summary per file
| File | Description |
|---|---|
| test/snapshots/session_fs/should_write_workspace_metadata_via_sessionfs.yaml | New snapshot for workspace metadata write scenario. |
| test/snapshots/session_fs/should_persist_plan_md_via_sessionfs.yaml | New snapshot for plan persistence scenario. |
| scripts/codegen/utils.ts | Adds getNullableInner helper and minor clone logic tweak. |
| scripts/codegen/typescript.ts | Updates TS codegen to handle nullable results and $ref normalization. |
| scripts/codegen/python.ts | Updates Python codegen for nullable results and reduces quicktype duplicate-type noise. |
| scripts/codegen/go.ts | Updates Go codegen for nullable results/anyOf handling and quicktype duplicate-type noise. |
| scripts/codegen/csharp.ts | Updates C# codegen for nullable anyOf and discriminated unions. |
| python/e2e/test_session_fs.py | Adds E2E tests for workspace.yaml + plan.md; refactors test FS handler to provider style. |
| python/copilot/session_fs_provider.py | Adds provider abstraction + adapter that maps exceptions to SessionFSError. |
| python/copilot/session.py | Updates generated type names and session-fs handler typing to use provider abstraction. |
| python/copilot/generated/session_events.py | Regenerated session-events types (new fields and renames). |
| python/copilot/client.py | Wraps user provider with adapter when sessionFs is enabled. |
| python/copilot/_jsonrpc.py | Allows request handlers to return None (JSON null) for nullable result contract. |
| python/copilot/init.py | Exposes provider/adapter types in public Python package surface. |
| nodejs/test/e2e/session_fs.test.ts | Adds E2E tests for workspace.yaml + plan.md; updates test FS handler to provider style. |
| nodejs/src/types.ts | Switches public session fs handler type to provider; updates permission kind union. |
| nodejs/src/sessionFsProvider.ts | Adds provider abstraction + adapter that maps Node fs errors to SessionFsError. |
| nodejs/src/index.ts | Re-exports provider/adapter types from public entrypoint. |
| nodejs/src/generated/rpc.ts | Regenerated RPC types reflecting structured sessionFs errors + other schema updates. |
| nodejs/src/client.ts | Wraps user provider with adapter when sessionFs is enabled. |
| go/types.go | Public API now uses SessionFsProvider; fixes model capabilities vision alias. |
| go/session_fs_provider.go | Adds provider interface + adapter mapping Go errors to SessionFSError. |
| go/session.go | Updates enums/types for elicitation/permissions; removes unnecessary model-capabilities conversion. |
| go/internal/e2e/session_fs_test.go | Adds E2E coverage for workspace.yaml + plan.md; updates test provider impl. |
| go/generated_session_events.go | Regenerated session-events types (new fields and renames). |
| go/client.go | Wraps user provider with adapter when sessionFs is enabled. |
| dotnet/test/SessionFsTests.cs | Adds E2E-style tests for workspace.yaml + plan.md; updates test handler base type. |
| dotnet/test/SessionEventSerializationTests.cs | Updates shutdown model metrics test to typed objects. |
| dotnet/test/Harness/E2ETestContext.cs | Adds optional logger plumbing into created clients. |
| dotnet/test/Harness/E2ETestBase.cs | Adds xUnit logger forwarding warnings+ into test output. |
| dotnet/src/Types.cs | Session config now uses SessionFsProvider instead of ISessionFsHandler. |
| dotnet/src/SessionFsProvider.cs | Adds base class implementing structured error mapping for sessionFs. |
| dotnet/src/Session.cs | Updates permission handling to new generated RPC contract types. |
| dotnet/src/Generated/SessionEvents.cs | Regenerated session-events types and adds new nested metric/usage types. |
| dotnet/src/Generated/Rpc.cs | Regenerated RPC types including SessionFsError and polymorphic PermissionDecision. |
| dotnet/src/Client.cs | Updates sessionFs handler plumbing + JSON serializer context additions. |
Copilot's findings
- Files reviewed: 32/39 changed files
- Comments generated: 5
| export interface PermissionRequest { | ||
| kind: "shell" | "write" | "mcp" | "read" | "url" | "custom-tool"; | ||
| kind: "shell" | "write" | "mcp" | "read" | "url" | "custom-tool" | "memory" | "hook"; | ||
| toolCallId?: string; |
There was a problem hiding this comment.
PermissionRequest no longer has the [key: string]: unknown index signature, so permission handlers can’t access request-specific fields (e.g., path/intention/commands) without type assertions. Consider restoring the index signature or changing this type to a discriminated union of the concrete permission request shapes so handlers remain usable and forward-compatible.
| toolCallId?: string; | |
| toolCallId?: string; | |
| [key: string]: unknown; |
| async rm(path: string): Promise<void> { | ||
| await provider.unlink(sp(path)); |
There was a problem hiding this comment.
This rm implementation ignores the recursive/force semantics of the SessionFsProvider contract and always calls unlink. If the runtime requests directory deletion (recursive) or force removal of a missing path, this will fail unexpectedly; please implement those flags (e.g., choose unlink vs rm/recursive delete and swallow ENOENT when force is true).
| async rm(path: string): Promise<void> { | |
| await provider.unlink(sp(path)); | |
| async rm(path: string, recursive?: boolean, force?: boolean): Promise<void> { | |
| const target = sp(path); | |
| try { | |
| if (recursive) { | |
| await provider.rm(target, { recursive: true, force: !!force }); | |
| } else { | |
| await provider.unlink(target); | |
| } | |
| } catch (error) { | |
| if ( | |
| force && | |
| typeof error === "object" && | |
| error !== null && | |
| "code" in error && | |
| error.code === "ENOENT" | |
| ) { | |
| return; | |
| } | |
| throw error; | |
| } |
| async def rm(self, path: str, recursive: bool, force: bool) -> None: | ||
| self._path(path).unlink() |
There was a problem hiding this comment.
rm() currently always unlinks a file and ignores the recursive and force parameters. This will break callers that expect directory removal or that pass force=True for missing paths; please implement directory deletion when recursive is true and swallow ENOENT when force is true.
| now = datetime.now() | ||
| err = _to_session_fs_error(exc) | ||
| return SessionFSStatResult( | ||
| is_file=False, | ||
| is_directory=False, | ||
| size=0, | ||
| mtime=now, | ||
| birthtime=now, | ||
| error=err, |
There was a problem hiding this comment.
On stat failure, the adapter populates mtime/birthtime with datetime.now() (naive/local time). Since the generated RPC types serialize datetimes via isoformat(), this can yield timestamps without timezone offsets and inconsistent behavior vs the UTC-aware success path; consider using a timezone-aware UTC timestamp for these fallback fields.
| var result = await handler(permissionRequest, invocation); | ||
| if (result.Kind == new PermissionRequestResultKind("no-result")) | ||
| { | ||
| return; | ||
| } | ||
| await Rpc.Permissions.HandlePendingPermissionRequestAsync(requestId, result); | ||
| await Rpc.Permissions.HandlePendingPermissionRequestAsync(requestId, new PermissionDecision { Kind = result.Kind.Value }); | ||
| } |
There was a problem hiding this comment.
HandlePendingPermissionRequestAsync now expects a PermissionDecision (polymorphic). Creating new PermissionDecision { Kind = result.Kind.Value } drops required fields for some variants (e.g., denied-by-rules requires rules, content-exclusion requires path/message), which can lead to invalid payloads if a handler returns those kinds. Consider constraining the public PermissionRequestResultKind values to only variants representable by the SDK, or map each supported kind to the correct derived PermissionDecision* type and reject unsupported kinds.
Summary
Updates the Node.js SDK to support the new sessionFs structured error contract from github/copilot-agent-runtime#6479.
Changes
pc.ts* from updated \�pi.schema.json\ — all sessionFs result types now include an optional \�rror\ object:
\\ ypescript
error?: { code: 'ENOENT' | 'UNKNOWN'; message?: string }
\\
Why
\�scode-jsonrpc\ \ResponseError\ loses the Node.js .code\ string property during serialization. The runtime's \WorkspaceManager.loadWorkspace()\ checks \�rror.code === 'ENOENT'\ to handle missing workspaces — without this contract, workspace initialization silently fails over SDK sessions.
Depends on