fix: make an empty workspaceId fail like an omitted one (#214) - #345
Open
ProfSynapse wants to merge 3 commits into
Open
fix: make an empty workspaceId fail like an omitted one (#214)#345ProfSynapse wants to merge 3 commits into
ProfSynapse wants to merge 3 commits into
Conversation
Both envelope schemas list `workspaceId` in `required`, and on the MCP path that array is enforced at runtime (ValidationService -> validateParams), so omitting the field hard-fails. But `required` only asks whether the key is PRESENT: `workspaceId: ""` passed that check and then hit `params.workspaceId || 'default'` in ToolCliNormalizer.normalizeContext, which silently filed the call under the global workspace. A caller whose template rendered to empty behaved completely differently from one that omitted the field, and nothing told it so. The same schemas described the field as 'Workspace ID. Optional. Defaults to "default".' while listing it as required. Requiredness is the half that is actually enforced, and with no session stickiness (the other half of #214, deliberately not implemented) a silent default is a guess dressed up as a decision — so the descriptions were corrected to match the contract rather than the contract relaxed to match the descriptions. - normalizeContext now refuses absent, empty and whitespace-only workspaceId with one shared recoverable steering message that names the coercion that used to happen. Surrounding whitespace on a real value is trimmed, so " default " cannot reach storage as its own path segment. - The guard lives in the normalizer because tool parameter schemas are documentation plus CLI-normalizer hints, not runtime validation. - The placeholder-steering message for workspaceId no longer offers "or omit it to default to default", which is no longer true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
Envelope validation (#311, #317) only covers callers that come through useTools. Below it, six repositories built a JSONL path straight from a caller-supplied workspaceId — `workspaces/ws_<id>.jsonl` for session, state, trace and workspace events, `tasks/tasks_<workspaceId>.jsonl` for task and project events — so any string that reached a repository by another door became a directory on disk, silently and permanently. The reporter's census of one real vault found 41 of 56 workspace directories were phantoms, including `ws_--workspaceId` (a flag name that leaked in as a value) and one named `ws_` (the empty id). `workspaceStreamPath` / `taskStreamPath` now stand in front of every mint and refuse anything that can be neither a workspace id NOR a workspace name: empty or blank, padded, leading-dash flag names, path separators, `..` traversal (normalizePath does not strip it), control characters, non-strings and absurd lengths. Rejection is a thrown error naming the value, the reason and the way out, before any event is written. Deliberately structural, not a lookup: asking whether the workspace EXISTS needs the live list, belongs at the envelope, and cannot move down here without every repository depending on the service that owns one of them. So a well-formed id for a workspace that no longer exists still passes, and so do real names with spaces and accents — rejecting those would turn a working call into a hard error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the validation half of #214. Stickiness is deliberately not implemented here — see below.
The inconsistency
Omitting
workspaceIdcorrectly hard-failed.workspaceId: ''did not.requiredis genuinely enforced at runtime —ValidationService→validateParamswalks the array, which is why omission failed. But that check ishasProperty, so an empty string satisfied it and flowed intoparams.workspaceId || 'default'inToolCliNormalizer.normalizeContext.That is the shape most likely to bite a caller building the envelope programmatically: a template that renders to empty behaves completely differently from one that omits the field.
The schema also contradicted itself — describing
workspaceIdas "Optional. Defaults to default" while listing it asrequired. Therequiredlist was the truth; the descriptions inuseTools.ts,getTools.tsandtypes.tswere the lie, and are corrected. Relaxing to optional instead would have re-armed the misfiling this issue is about, sincegetToolsalready returns the live workspace list.The empty string was not harmless
Grepped before changing it. Nothing depends on
'' === default:'default'for bothundefinedand''before reaching the normalizerTaskBoardView'sworkspaceId: ''means "all workspaces" for in-memory filtering and never becomes a pathTaskManagerToolsalready rejected'', so the envelope now agrees with itOne live path did depend on it, and only to produce garbage:
getToolswith''flowed intocreateAutoSession('')and mintedworkspaces/ws_.jsonl— the mysteriousws_directory from the census in #214. The repository guard now throws there, andToolExecutionStrategyalready catches that and falls back to an in-memory session id.Second commit: a repository-layer guard
Validation previously depended entirely on the envelope being the only door. The guard is structural, not a lookup — it refuses what can be neither an id nor a name (empty, padded,
--flag, separators,.., control characters, absurd length) across all six workspace-keyed repositories.It deliberately still accepts real names with spaces and accents, which legitimately reach
SessionRepositorytoday, and zero-filled or truncated UUIDs — rejecting those needs the live workspace list, which would make every repository depend on the service that owns one of them.Not in scope
Stickiness is genuinely unimplemented — after ~130 envelope calls,
getAllSessionContexts()returned size 0. That is a design question about where session state should live, not a validation bug, and it deserves its own discussion.Verification
git stash push -- src, re-run, pop)appendEventnever called) carry that proof insteadnpx jest tests/unit— 328 suites, 4341 testsnpm run buildclean;check_documented_commands.py README.mdclean🤖 Generated with Claude Code
https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
Generated by Claude Code