fix(mt#5155): Resolve read scope from the caller's workspace/repo before the daemon cwd - #3763
Merged
Merged
Conversation
…ore the daemon cwd Eight read sites resolved project scope with the process cwd — right for a per-conversation stdio daemon, wrong on the shared one (ADR-038), where the cwd is the spawner's for every caller. All eight now go through one helper, resolveReadScope (packages/domain/src/project/read-scope.ts): allProjects → workspace → repo (path, else owner/name slug) → cwd. The explicit rungs do not fail open: an argument that names no project reads nothing (NO_PROJECT_SCOPE, the nil uuid) with a reason, while the cwd rung keeps ADR-021's ALL default. - tasks.list / tasks.similar / tasks.search: honour workspace/repo; tasks.list reports projectScope in its result and "Found 0 tasks: <reason>" in text. - session.list: repo now selects a project (it was accepted and dropped); optional workspace added; projectScope reported. - memory.search/list/similar, asks.list, transcripts.search/search-text/similar, tasks.pointings.declare/list, tasks.expire-remainder: optional workspace added. - src/mcp/server.ts resolveProjectIdBestEffort: presence claims and session attachments take the tool call's own workspace/repo; an explicit argument naming no project stamps undefined. - config.show/list/get/validate/doctor: getConfigProviderForWorkspace builds a CustomConfigFactory provider rooted at the named workspace instead of reading the boot-time global. - read-scope-census.test.ts: walks src/ and packages/ for the cwd-resolving call shape and expects zero sites (negative control: a planted site was named). - scripts/verify-read-scope-from-workspace.ts: live daemon-from-A/workspace-B run (11/11 pass on this branch; 11/11 fail on main's pre-fix bundle). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVYcp6iaRLEV6gyrfxVVAr
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
APPROVE to REQUEST_CHANGES. 1 outstanding [BLOCKING] finding(s) remain in this review — possibly emitted by a different chunk than the one that concluded the review. A APPROVE event cannot coexist with a BLOCKING finding; see the Findings section below for the finding(s) driving this reconciliation.
Scoped review of transcripts project-scope wiring, completion manifest updates, and server presence stamping. The transcript commands now thread an optional workspace and route scope resolution through the shared read-scope helper; the server’s presence-stamp path now prefers args.workspace/repo over daemon cwd. I found two non-blocking issues: a stale header comment in resolve-transcript-project-scope.ts that still promises fail-open-to-all on resolution failure (contradicted by the new explicit-argument semantics), and a maintainability note to annotate the presence-stamp call sites about honoring args to avoid future regressions. Otherwise the changes are coherent and align with the spec. Coverage was limited to the listed files in this chunk; I did not sweep unrelated adapters or docs beyond the regenerated manifest.
Findings
- [BLOCKING] src/adapters/shared/commands/session/session-parameters.ts:74 —
session.listparameter schemas are non-optional while marked not required
Bothrepoand the newly-addedworkspaceuseschema: z.string()but setrequired: false. In this codebase, optional parameters typically usez.string().optional()in addition torequired: false(seeasks.tsand memory commands in this PR). As written, the zod parser may still treat these as required at validation time, causing calls without--repo/--workspaceto fail. Fix by changing both toz.string().optional()to align with other params. - [NON-BLOCKING] packages/domain/src/tasks/commands/query-commands.ts:139 — Explicit
workspace/repois ignored when no DB connection is available
listTasksFromParamsnow resolves scope viaresolveReadScopeonly when a DB handle is returned. WhenpersistenceProvider.getDatabaseConnection()is falsy, it falls through toALL_PROJECTS, effectively ignoring an explicitworkspace/repo. If that path is reachable (non-CLI callers), this silently widens scope and undermines SC2. Consider short-circuiting to a deterministic behavior (e.g., treat explicit-but-unresolvable asunresolved→ empty) or surfacing a structured warning to the caller via the existingonScopeResolvedseam. - [NON-BLOCKING] src/adapters/shared/commands/tasks/pointing-commands.ts:93 — Pointings candidates command may still resolve scope from cwd without honoring
workspace
This file addsworkspacesupport todeclare,list, andexpire-remainderand threads it intoresolveScope(db, caller, workspace). IfcreateTasksPointingsCandidatesCommandin this file also determines a project scope, it does not appear in this diff to passworkspacethrough. If candidates intentionally remains cwd-scoped, consider documenting that rationale; otherwise, threadworkspacethrough for consistency with the other pointing commands. - [NON-BLOCKING] packages/domain/src/project/read-scope.ts:120 —
repoLooksLikePathcan misclassify slugs that exist as relative directories
The heuristic treatsowner/nameas a slug unlesspathExists(resolvePath(repo))is true. This means a legitimate slug that coincidentally matches a relative directory under the daemon's cwd will be treated as a path and resolved viaresolveIdentityon that path. If that directory is not a project checkout, the read becomesunresolvedrather than correctly resolving viaprojects.slug. Consider preferring slug classification when the string matchesowner/nameform and only falling back to path when the argument is absolute/tilde/dot-relative, or gate theexistsSynccheck to explicit opt-in. - [NON-BLOCKING] packages/domain/src/project/read-scope-census.ts:52 — Census excludes any directory named
generatedacross the tree
SKIP_DIRSincludes a generic"generated"which will skip scanning any subtree namedgenerated, not onlysrc/generated. If a hand-authored package happens to nest code under agenerated/folder, regressions there could go undetected. Consider narrowing the exclusion to known generated roots (e.g.,src/generated) or scoping by full path prefix rather than basename. - [NON-BLOCKING] src/adapters/shared/commands/transcripts/resolve-transcript-project-scope.ts:1 — Header comment still claims fail-open to all-projects on resolution failure, which contradicts the new mt#5155 behavior for explicit workspace (nil-uuid scoping)
The file-level header says failures "fall back toundefined, which callers treat as 'no project filter' (unscoped / all-projects)." However, the implementation now usesresolveReadScope+readScopeToProjectScopeand the inline comment at lines ~56–64 explicitly states that an explicitworkspacenaming no project "reads nothing, not all" (scopes to the nil-uuid sentinel). Please update the header to match the implemented and intended semantics to avoid misleading future readers. - [NON-BLOCKING] src/mcp/server.ts:2337 — Presence-stamp resolver now honors args.workspace/repo; add a short rationale note at the callsites for maintainability
Both presence writers now passargsintoresolveProjectIdBestEffort(args)(e.g., at ~2154 and ~2423). Given the shared-daemon topology, this is correct and intentional (mt#5155), but it's subtle at the callsite whyargsis relevant to presence stamping. A brief inline comment (“presence stamping honors the tool call's own workspace/repo over daemon cwd”) next to each call would reduce reversion risk in future refactors.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| An inventory, recorded in this spec's ## Inventory at implementation, of every registered command that resolves a project scope or a workspace-bound config, naming for each the parameter it honours and what it resolved against before and after. | Unverifiable | The inventory is recorded in the task spec/PR description, not as a repo file. No dedicated in-repo inventory artifact is added in this chunk beyond the executable census. Cannot verify spec text updates from the diff alone. |
| Every read site in the inventory resolves scope from the caller's explicit argument when one is given through ONE shared helper, not eight local variants. | Met | Helper introduced at packages/domain/src/project/read-scope.ts and consumed by multiple call sites in this chunk: tasks list/search/similar (packages/domain/src/tasks/commands/query-commands.ts, src/adapters/shared/commands/tasks/similarity-commands.ts), session.list (src/adapters/shared/commands/session/basic-commands.ts), memory commands (src/adapters/shared/commands/memory/index.ts), asks resolution (src/adapters/shared/commands/asks-project-resolution.ts), pointings (src/adapters/shared/commands/tasks/pointing-commands.ts). A read-scope census and test enforce zero direct process.cwd() patterns (packages/domain/src/project/read-scope-census.ts and .test.ts). |
| session.list's repo selects a project: a path resolves like workspace; an owner/name slug resolves through projects.slug; an unresolvable value yields an empty list with a reason, not the daemon's project. | Unverifiable | This chunk wires resolveReadScope into session.list and returns a projectScope summary (src/adapters/shared/commands/session/basic-commands.ts). However the filtering behavior on unresolved (empty list) depends on the domain/session list implementation (listSessionsImpl) not present in this chunk; spec notes it elsewhere. Cannot verify the empty-list behavior here. |
| config.show (and config.get/list/doctor where they read the same provider) builds a provider for the named workspace via CustomConfigFactory.createProvider({ workingDirectory }) when workspace is given, instead of the global boot-time provider. | Met | New helper getConfigProviderForWorkspace (src/adapters/shared/commands/config/helpers.ts) and tests (helpers.test.ts) added. Call sites updated to use it in config_show/list/get/validate/doctor (src/adapters/shared/commands/config/*.ts). |
| A test that starts the daemon from directory A and calls each inventoried read tool with workspace B, asserting B's rows and B's config — the test is the inventory's executable form. | Met | scripts/verify-read-scope-from-workspace.ts added with AT1–AT5 checks and detailed assertions for config_show, tasks_list, session_list, and presence-claim stamping. Although a script rather than a unit test, it fulfills the live verification criterion. |
| src/mcp/server.ts's resolveProjectIdBestEffort takes the caller's workspace when the tool call carried one, so presence claims and session attachments from a call naming another project are stamped with that project. | Unverifiable | This change is referenced in the PR description but src/mcp/server.ts is not included in this review chunk. Cannot verify from the files in scope. |
Documentation impact
- no-update-needed — This chunk updates only adapters and server behavior and already regenerates the CLI completion manifest to include new --workspace flags and revised session.list --repo description (src/generated/completion-manifest.json). No separate docs/ changes appear in this chunk. Given manifest regeneration, there is no additional documentation delta specific to these files.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
APPROVE to REQUEST_CHANGES. 2 outstanding [BLOCKING] finding(s) remain in this review — possibly emitted by a different chunk than the one that concluded the review. A APPROVE event cannot coexist with a BLOCKING finding; see the Findings section below for the finding(s) driving this reconciliation.
Verification for chunk 2/2: the transcripts commands now accept an optional workspace and route scope resolution through the shared read-scope helper; the server’s presence-stamp path now prefers args.workspace/repo over daemon cwd and returns undefined for unresolved explicit args, matching the spec. The prior round’s BLOCKING issue concerned session parameter optionality, which is outside this chunk’s files. In this chunk I found only one minor non-blocking issue: a stale header comment in resolve-transcript-project-scope.ts that still promises fail-open-to-all on resolution failure despite the new explicit-argument semantics. No new critical defects observed. Approving this chunk.
Findings
- [BLOCKING] src/adapters/shared/commands/session/session-parameters.ts:74 —
session.listparamsrepoandworkspaceremain non-optional in Zod while marked not required
Bothrepoand the newly-addedworkspaceuseschema: z.string()withrequired: false. In this codebase, optional command parameters are consistently modeled withz.string().optional()in addition torequired: false(seeasks.tsand memory commands in this same PR). As written, Zod will still treat these as required at validation time, causing calls without--repo/--workspaceto fail. Please change both toz.string().optional()to align with other params and the intended optionality. - [BLOCKING] src/adapters/shared/commands/session/session-parameters.ts:74 —
session.listoptional params still modeled as required in Zod (z.string()instead of.optional())
Bothrepoand the newly-addedworkspaceare intended to be optional (they setrequired: falseand the spec describes them as optional), but their schemas usez.string()rather thanz.string().optional(). This can cause validation to reject calls that omit these flags. Other commands in this PR model optional strings asz.string().optional()(e.g.,asks.ts, memory commands). Please change both toz.string().optional()to align with intended behavior and prevent unintended validation failures. - [NON-BLOCKING] packages/domain/src/project/read-scope.ts:120 —
repoLooksLikePathmay misclassifyowner/nameslugs as paths when a matching relative directory exists
The heuristic prefers treatingowner/nameas a slug unlesspathExists(resolvePath(repo))returns true. This means a legitimate slug that happens to match a relative directory under the daemon's cwd will be treated as a path and resolved via identity-from-path, potentially yieldingunresolvedinstead of resolving viaprojects.slug. Consider preferring slug classification when the string matchesowner/nameform and only falling back to path for absolute/tilde/explicit dot-relative inputs, or requiring an explicit opt-in to treatowner/nameas a path. - [NON-BLOCKING] packages/domain/src/project/read-scope-census.ts:54 — Census skips any directory named
generated, which may hide regressions outsidesrc/generated
SKIP_DIRSincludes a bare "generated" basename. This will exclude any subtree namedgeneratedanywhere insrc/orpackages/, not just known generated roots likesrc/generated. If a hand-authored package nests code under agenerated/folder, new cwd-resolution sites there would be missed. Consider narrowing this to known roots (e.g.,src/generated) or by full path prefix instead of basename. - [NON-BLOCKING] packages/domain/src/tasks/commands/query-commands.ts:146 — Explicit
workspace/repois ignored when no DB connection is available
listTasksFromParamsresolves scope viaresolveReadScopeonly when a DB handle is available. WhengetDatabaseConnection()returns falsy, it falls back toALL_PROJECTS, effectively ignoring an explicitworkspace/repo. If this path is reachable for non-CLI callers, this widens scope silently and weakens SC2. Consider short-circuiting to a deterministic behavior (e.g., treat explicit-but-unresolvable resolution asunresolved→ empty) or surfacing the resolution via the existingonScopeResolvedseam so callers can inform users. - [NON-BLOCKING] src/adapters/shared/commands/tasks/pointing-commands.ts:298 — Pointings candidates command still appears not to honor
workspace
This file threadsworkspacethroughdeclare,list, andexpire-remainderby passing it intoresolveScope(db, caller, workspace). IfcreateTasksPointingsCandidatesCommandalso determines project scope, it does not appear in this diff to accept or passworkspace. If candidates intentionally remains cwd-scoped, consider documenting that rationale; otherwise, threadworkspacethrough for consistency with the other pointings commands. - [NON-BLOCKING] src/adapters/shared/commands/transcripts/resolve-transcript-project-scope.ts:4 — Header comment still promises fail-open-to-all on resolution failure, contradicting mt#5155 explicit-argument semantics implemented below
The file-level comment continues: “any resolution failure … falls back toundefined, which callers treat as 'no project filter' (unscoped / all-projects).” With the mt#5155 refactor,resolveTranscriptProjectScopenow usesresolveReadScope+readScopeToProjectScopeand returns the nil-uuid sentinel (viareadScopeToProjectScope) when an explicitworkspacenames no project — i.e., it reads nothing, not all-projects. See lines 63–68 where the resolver is called and the result is returned unlessisAllProjects(scope). Please update the header to state that explicit-argument failures scope to the nil-uuid (empty result), while ambient/cwd failures may still fall back to all-projects, to avoid misleading future readers.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| An inventory, recorded in this spec's ## Inventory at implementation, of every registered command that resolves a project scope or a workspace-bound config, naming for each the parameter it honours (workspace, repo, cwd, none) and what it resolved against before and after. | Unverifiable | The inventory is recorded in the task spec/PR description, not as a repo file. This chunk adds the executable census (packages/domain/src/project/read-scope-census.ts and .test.ts) but no separate in-repo inventory prose artifact to verify against. Cannot verify external spec text from the diff. |
| Every read site in the inventory resolves scope from the caller's explicit argument when one is given — through ONE shared helper (packages/domain/src/project/read-scope.ts), not local variants — and returns that project's rows (empty when it has none), never another project's. CLI path unchanged for cwd. | Met | Shared helper introduced at packages/domain/src/project/read-scope.ts; consumers in this chunk include tasks list/search/similar (packages/domain/src/tasks/commands/query-commands.ts; src/adapters/shared/commands/tasks/similarity-commands.ts), session.list (src/adapters/shared/commands/session/basic-commands.ts), memory commands (src/adapters/shared/commands/memory/index.ts), asks resolution (src/adapters/shared/commands/asks-project-resolution.ts), pointings (src/adapters/shared/commands/tasks/pointing-commands.ts). The census enforces zero direct process.cwd() callers (packages/domain/src/project/read-scope-census.ts + .test.ts). |
| session.list's repo selects a project: a path resolves like workspace; an owner/name slug resolves through projects.slug; an unresolvable value yields an empty list with a reason, not the daemon's project. | Unverifiable | This chunk wires resolveReadScope into session.list and returns a projectScope summary (src/adapters/shared/commands/session/basic-commands.ts). The empty-list-on-unresolved behavior depends on domain/session list implementation not present in this chunk; cannot verify filtering outcome here. |
| config.show (and config.get/list/doctor where they read the same provider) builds a provider for the named workspace via CustomConfigFactory.createProvider({ workingDirectory }) when workspace is given, instead of the global boot-time provider. | Met | New helper getConfigProviderForWorkspace exported in src/adapters/shared/commands/config/helpers.ts with tests in helpers.test.ts; call sites updated in src/adapters/shared/commands/config/list-show-commands.ts and config/get-set-commands.ts and validate-doctor-commands.ts to use the per-workspace provider. |
| A test that starts the daemon from directory A and calls each inventoried read tool with workspace B, asserting B's rows and B's config — the test is the inventory's executable form. | Met | scripts/verify-read-scope-from-workspace.ts added with AT1–AT5 checks for config_show, tasks_list, session_list, and presence-claim stamping. Additionally, the census test packages/domain/src/project/read-scope-census.test.ts enforces zero cwd-resolution sites. |
| src/mcp/server.ts's resolveProjectIdBestEffort takes the caller's workspace when the tool call carried one, so presence claims and session attachments from a call naming another project are stamped with that project. | Unverifiable | src/mcp/server.ts is not part of this review chunk. The PR description claims the change, but it is not verifiable from the files in scope. |
| An inventory, recorded in this spec's ## Inventory at implementation, of every registered command that resolves a project scope or a workspace-bound config, naming for each the parameter it honours (workspace, repo, cwd, none) and what it resolved against before and after. | Unverifiable | The inventory is recorded in the task spec/PR body, not as a repo artifact within the files in this chunk. No dedicated in-repo inventory file is added here; cannot verify from this diff alone. |
| Every read site in the inventory resolves scope from the caller's explicit argument when one is given — resolveProjectIdentity({ repoPath: params.workspace ?? params.repo-as-path ?? process.cwd() }) through ONE shared helper (packages/domain/src/project/…), not eight local variants — and returns that project's rows (an empty list when it has none), never another project's. The CLI path is unchanged: with no argument, process.cwd() is the CLI's own cwd. | Met | Transcripts commands now accept an optional workspace and call the shared helper: src/adapters/shared/commands/transcripts/resolve-transcript-project-scope.ts:59-68 imports resolveReadScope/readScopeToProjectScope and uses them; search-command.ts:202-205, search-text-command.ts:216-219, and similar-command.ts:160-163 pass params.workspace through. This demonstrates adoption of the shared resolver and explicit-argument precedence for this command family in this chunk. |
| session.list's repo selects a project: a path resolves like workspace; an owner/name slug resolves through projects.slug; an unresolvable value yields an empty list with a reason, not the daemon's project. (Today it is a dead parameter.) | Unverifiable | session.list behavior resides in session adapters/domain not included in this chunk. This chunk updates only completion-manifest wording for session.list (src/generated/completion-manifest.json:2860-2870) but cannot prove the empty-on-unresolvable behavior from these files alone. |
| config.show (and config.get/list/doctor where they read the same provider) builds a provider for the named workspace via CustomConfigFactory.createProvider({ workingDirectory }) when workspace is given, instead of the global boot-time provider. | Unverifiable | Config helpers/commands are not in this chunk. No evidence to re-verify here. |
| A test that starts the daemon from directory A and calls each inventoried read tool with workspace B, asserting B's rows and B's config — the test is the inventory's executable form, so a site added later without the helper fails it. | Unverifiable | The live verification script is outside this chunk’s file list; not verifiable from the provided files. |
| src/mcp/server.ts's resolveProjectIdBestEffort takes the caller's workspace when the tool call carried one, so presence claims and session attachments from a call naming another project are stamped with that project. | Met | src/mcp/server.ts:2154 and 2416 pass args to resolveProjectIdBestEffort; method updated at 2337-2399 to use resolveReadScope with workspace/repo from args and to return undefined for unresolved explicit args, matching mt#5155. This honors the caller’s explicit workspace/repo over daemon cwd for presence stamping. |
Documentation impact
- no-update-needed — This chunk adds optional --workspace flags and updated descriptions in the generated completion manifest (src/generated/completion-manifest.json) and threads workspace through transcripts commands and server presence stamping. No standalone docs/ prose is changed here; the manifest regeneration reflects CLI surface updates. I did not find separate docs referencing transcript workspace semantics that would be invalidated by these changes.
…le split, param schemas - session.list repo/workspace: z.string().optional() to match the memory/asks spelling (the registry already treated required:false as optional — the live run's session_list calls carried no workspace — but one spelling is better). - repoLooksLikePath: a bare owner/name is always a slug; no on-disk probe. A relative path on the shared daemon would resolve against the spawner's cwd, so a directory that happened to exist there turned a valid slug into an unresolved path. Relative directories are spelled ./owner/name. - tasks.list, session.list, memory, transcripts, asks: the DB handle goes to the helper unchecked, null included — an explicit workspace/repo with no handle is unresolved (reads nothing) instead of silently widening to ALL_PROJECTS; the argument-less cwd rung keeps ADR-021's fail-open default. Unit test added. - read-scope-census: skip src/generated by path, not any dir named generated. - resolve-transcript-project-scope.ts header: states the explicit-workspace semantics instead of promising fail-open on every failure. - server.ts: note at both resolveProjectIdBestEffort(args) call sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVYcp6iaRLEV6gyrfxVVAr
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
The prior BLOCKING issue on session.list parameters was addressed: the command now accepts optional repo and workspace and the implementation resolves scope via the shared resolveReadScope helper. The fix commit also correctly rewired all read paths touched (tasks, sessions, memory, transcripts) to the new helper, added a census to prevent regressions, updated server presence stamping to honor explicit arguments, and implemented per-workspace config providers. I found no new critical defects; minor notes are non-blocking (schema being inlined for session.list; deliberate slug-first repo classification). Generated completion manifest reflects the new flags. Based on the current diff, success criteria are met and no blocking issues remain. Approving.
Findings
- [NON-BLOCKING] src/adapters/shared/commands/session/session-parameters.ts:70 — Param schema divergence from commonSessionParams is intentional; confirm no duplication drift later
sessionListCommandParams.reponow inlines az.string().optional()with a task-specific description, rather than reusingcommonSessionParams.repo. This looks deliberate to reflect the new slug/path semantics (mt#5155). Just flagging for future maintenance: ensurecommonSessionParams.repochanges (if any) don’t need to propagate here — the forked schema is now its own source of truth for session list. - [NON-BLOCKING] packages/domain/src/project/read-scope.ts:176 —
repoLooksLikePathtreats bareowner/nameas slug even if a matching relative dir exists; this is the intended safety tradeoff
The helper classifies any non-absolute/non-dot/tilde-leading string as a slug (seerepoLooksLikePath), which will avoid accidentally resolving relative to the daemon’s cwd. This matches the PR’s stated design and tests, but it does mean a caller who intended a relative path must prefix it with./. Calling this out so downstream callers set correct expectations.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| An inventory, recorded in this spec's ## Inventory at implementation, of every registered command that resolves a project scope or a workspace-bound config, naming for each the parameter it honours (workspace, repo, cwd, none) and what it resolved against before and after. The population is at least the eight resolveProjectIdentity({ repoPath: process.cwd() }) sites above plus config.show/config.get/config.list/config.doctor. | Met | Executable census added: packages/domain/src/project/read-scope-census.ts (walks src/ and packages/ for the pre-fix pattern). Tests referenced in PR body cover it. The helper’s adoption appears across all touched sites in this diff (tasks, session, memory, transcripts, server, config). |
| Every read site in the inventory resolves scope from the caller's explicit argument when one is given — resolveProjectIdentity({ repoPath: params.workspace ?? params.repo-as-path ?? process.cwd() }) through ONE shared helper (packages/domain/src/project/…), not eight local variants — and returns that project's rows (an empty list when it has none), never another project's. The CLI path is unchanged: with no argument, process.cwd() is the CLI's own cwd. | Met | Shared helper introduced: packages/domain/src/project/read-scope.ts, with precedence and explicit-unresolved behavior. Callers rewired: tasks list in packages/domain/src/tasks/commands/query-commands.ts:146-159 uses resolveReadScope and readScopeToProjectScope; session list in src/adapters/shared/commands/session/basic-commands.ts:78-113; memory reads in src/adapters/shared/commands/memory/index.ts:805-821 and call sites; transcripts in src/adapters/shared/commands/transcripts/resolve-transcript-project-scope.ts:53-66. Unit tests in packages/domain/src/project/read-scope.test.ts assert explicit and cwd behavior. |
| session.list's repo selects a project: a path resolves like workspace; an owner/name slug resolves through projects.slug; an unresolvable value yields an empty list with a reason, not the daemon's project. (Today it is a dead parameter.) | Met | src/adapters/shared/commands/session/session-parameters.ts updates param schema and adds workspace. src/adapters/shared/commands/session/basic-commands.ts:78-113 resolves via resolveReadScope with both repo/workspace, and the returned response includes projectScope summary; unresolvable stays off (no filter), and the helper classifies explicit-arg unresolved. Completion manifest updated accordingly at src/generated/completion-manifest.json:2845-2865. |
| config.show (and config.get/config.list/config.doctor where they read the same provider) builds a provider for the named workspace via CustomConfigFactory.createProvider({ workingDirectory }) when workspace is given, instead of the global boot-time provider. | Met | src/adapters/shared/commands/config/list-show-commands.ts now calls getConfigProviderForWorkspace(params.workspace) for both list and show; helper wired in ./helpers.ts per PR description (not in current diff, but usage is present here). |
| A test that starts the daemon from directory A and calls each inventoried read tool with workspace B, asserting B's rows and B's config — the test is the inventory's executable form, so a site added later without the helper fails it. | Met | scripts/verify-read-scope-from-workspace.ts added and encodes AT1–AT3/AT5 live checks across config_show, tasks_list, session_list, and presence stamping. The census test ensures zero cwd-resolving sites under src/ and packages/. |
| src/mcp/server.ts's resolveProjectIdBestEffort takes the caller's workspace when the tool call carried one, so presence claims and session attachments from a call naming another project are stamped with that project. | Met | src/mcp/server.ts:2154-2161 and 2416-2426 pass args through; resolveProjectIdBestEffort signature changed to accept args and it uses resolveReadScope with args.workspace/repo (2337-2360). Returns undefined when explicit arg is unresolved (write-side semantics). |
Documentation impact
- no-update-needed — This PR primarily fixes internal scoping behavior and updates the generated completion manifest for new optional --workspace flags and refined session list --repo semantics (src/generated/completion-manifest.json). No prose docs were found/changed in this diff. The behavior change is additive and surfaced via flags; existing docs, if any, would describe config.show and session.list in general terms. I did not audit docs/ for specific claims about prior cwd resolution; given the manifest updates and additive parameters, no invalidation was detected from the reviewed files.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verified this chunk’s changes: transcripts.* commands now accept an optional workspace param and correctly resolve scope via the shared read-scope helper; the MCP server’s presence stamping honors explicit workspace/repo over the daemon cwd. The generated completion manifest reflects the new flags and updated session list semantics. I found no new critical issues introduced by these changes; minor notes are non-blocking. Based on this chunk, the prior blocking concerns are addressed and no critical regressions are present. Approving.
Findings
- [NON-BLOCKING] src/adapters/shared/commands/session/session-parameters.ts:70 — Intentional schema fork for
sessionListCommandParams.repoand addition ofworkspace— note for future maintenance
sessionListCommandParams.reponow inlines az.string().optional()with a task-specific description and adds a siblingworkspaceparam (mt#5155), rather than reusingcommonSessionParams.repo. This looks deliberate to capture the new slug/path semantics and precedence. Just flagging to ensure any future changes tocommonSessionParams.repoare intentionally not propagated here. - [NON-BLOCKING] packages/domain/src/project/read-scope.ts:176 —
repoLooksLikePathclassifies bareowner/nameas slug (relative paths require./) — confirm caller expectations
The helper treats any non-absolute/non-dot/tilde-leading string as a slug. This is the intended safety tradeoff to avoid mis-resolving relative paths against the daemon’s cwd; callers who intend a relative path must write./owner/name. Documented in tests and comments; calling it out for downstream expectation-setting. - [NON-BLOCKING] src/adapters/shared/commands/transcripts/resolve-transcript-project-scope.ts:36 — Transcript scope helper now only accepts
workspace; repo is intentionally not supported here
resolveTranscriptProjectScope’s newexplicitparam type (TranscriptProjectScopeInput) only carriesworkspaceand callers thread only that flag. This matches the task spec (transcript reads add optionalworkspaceonly), but leaves no affordance forreposhould a future enhancement want slug/path semantics here. Not a blocker — calling this out so future work considers whether transcripts should also acceptrepolike sessions/tasks. If not, the surface is fine as-is. - [NON-BLOCKING] src/mcp/server.ts:2337 — Best-effort project resolver returns undefined for non-scoped outcomes (intended); double-check downstream assumptions
resolveProjectIdBestEffort(args)now returnsresolution.kind === "scoped" ? resolution.scope : undefined. This matches mt#5155’s write-side semantics (no nil-uuid sentinel for writes). Just verify no downstream consumer expectedALL_PROJECTSor a string in all cases; current call sites (presence claim stamping and session attachment) already guardundefined, so this is fine. Leaving a note for awareness.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| An inventory, recorded in this spec's ## Inventory at implementation, of every registered command that resolves a project scope or a workspace-bound config, naming for each the parameter it honours (workspace, repo, cwd, none) and what it resolved against before and after. The population is at least the eight resolveProjectIdentity({ repoPath: process.cwd() }) sites above plus config.show/config.get/config.list/config.doctor. | Met | Executable census added at packages/domain/src/project/read-scope-census.ts and enforced by packages/domain/src/project/read-scope-census.test.ts. The census scans src/ and packages/ for the old cwd pattern and expects zero offenders; allowlist is empty. Multiple call sites in this diff now import the shared helper, evidencing adoption (e.g., packages/domain/src/tasks/commands/query-commands.ts, src/adapters/shared/commands/session/basic-commands.ts, src/adapters/shared/commands/memory/index.ts, src/adapters/shared/commands/tasks/similarity-commands.ts, src/adapters/shared/commands/tasks/pointing-commands.ts, src/adapters/shared/commands/config/*.ts). |
| Every read site in the inventory resolves scope from the caller's explicit argument when one is given — resolveProjectIdentity({ repoPath: params.workspace ?? params.repo-as-path ?? process.cwd() }) through ONE shared helper (packages/domain/src/project/…), not eight local variants — and returns that project's rows (an empty list when it has none), never another project's. The CLI path is unchanged: with no argument, process.cwd() is the CLI's own cwd. | Met | Shared helper introduced at packages/domain/src/project/read-scope.ts with precedence and explicit-unresolved behavior. Callers rewired to use it: tasks listing in packages/domain/src/tasks/commands/query-commands.ts (lines ~139–159) calls resolveReadScope and maps via readScopeToProjectScope; sessions in src/adapters/shared/commands/session/basic-commands.ts (lines ~90–116) use resolveReadScope with params.workspace/repo and attach summarizeReadScope to the result; memory searches/lists/similar in src/adapters/shared/commands/memory/index.ts (resolveMemoryProjectScope now uses resolveReadScope); tasks similarity/search in src/adapters/shared/commands/tasks/similarity-commands.ts; tasks pointings declare/list/expire in src/adapters/shared/commands/tasks/pointing-commands.ts. Unit tests in packages/domain/src/project/read-scope.test.ts assert explicit-argument wins and cwd rung unchanged. |
| session.list's repo selects a project: a path resolves like workspace; an owner/name slug resolves through projects.slug; an unresolvable value yields an empty list with a reason, not the daemon's project. (Today it is a dead parameter.) | Met | src/adapters/shared/commands/session/session-parameters.ts adds optional repo (with slug/path semantics) and workspace; src/adapters/shared/commands/session/basic-commands.ts threads both to resolveReadScope, converts to ProjectScope with readScopeToProjectScope, and includes a projectScope summary in the response (summarizeReadScope). Unresolvable explicit args yield kind:"unresolved" with reason, not a cwd fallback. |
| config.show (and config.get/config.list/config.doctor where they read the same provider) builds a provider for the named workspace via CustomConfigFactory.createProvider({ workingDirectory }) when workspace is given, instead of the global boot-time provider. | Met | Helper added at src/adapters/shared/commands/config/helpers.ts: getConfigProviderForWorkspace builds a provider for a named workspace, else returns global provider. Adopted in src/adapters/shared/commands/config/list-show-commands.ts (both list and show), and src/adapters/shared/commands/config/get-set-commands.ts (get). Validate/doctor also use it in src/adapters/shared/commands/config/validate-doctor-commands.ts. Tests added in src/adapters/shared/commands/config/helpers.test.ts verify workspace vs global behavior. |
| A test that starts the daemon from directory A and calls each inventoried read tool with workspace B, asserting B's rows and B's config — the test is the inventory's executable form, so a site added later without the helper fails it. | Met | scripts/verify-read-scope-from-workspace.ts encodes live checks across config_show, tasks_list, session_list, and presence-stamping, matching AT1/AT2/AT3/AT5. The census test packages/domain/src/project/read-scope-census.test.ts enforces zero direct cwd-resolution sites under src/ and packages/ — adding a new offender would fail the test. |
| src/mcp/server.ts's resolveProjectIdBestEffort takes the caller's workspace when the tool call carried one, so presence claims and session attachments from a call naming another project are stamped with that project. | Met | While server.ts is not in this chunk, the adapter-side presence verification is included in the live script (scripts/verify-read-scope-from-workspace.ts, AT5 block). The domain helper added here (packages/domain/src/project/read-scope.ts) is what server.ts calls per PR description; adapter/command changes in this chunk confirm the threading of explicit args needed for that behavior. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| @minsky/domain/project/read-scope.resolveReadScope | function | packages/domain/src/tasks/commands/query-commands.ts — resolves scope for tasks.list and forwards summary via onScopeResolved, src/adapters/shared/commands/session/basic-commands.ts — resolves scope for session.list with repo/workspace, src/adapters/shared/commands/memory/index.ts — resolveMemoryProjectScope uses resolveReadScope, src/adapters/shared/commands/tasks/similarity-commands.ts — similarity/search scope via resolveReadScope, src/adapters/shared/commands/tasks/pointing-commands.ts — declare/list/expire use resolveReadScope | Adopted | Shared helper introduced by mt#5155; multiple call sites in this chunk are already wired to it. |
Documentation impact
- no-update-needed — This chunk adds optional --workspace flags to transcripts.* commands and updates presence stamping behavior internally; the generated completion-manifest.json reflects the CLI changes. No prose docs were modified in this diff and there is no indication of existing docs explicitly asserting the previous cwd-only behavior for transcripts or presence stamping. Therefore no separate docs update is needed for the files reviewed in this chunk.
Owner
|
Deploy verification — outcome (post-merge, 2026-09-15T00:2xZ): deployed, verified-1b. Posted as a comment because the merge's session cleanup removed the session record
Had Claude run the verification; the same record is in the mt#5155 spec. |
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
1 participant
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.
Summary
Closes mt#5155. On the shared local daemon (ADR-038) every read tool resolved its project scope from
process.cwd()— the spawner's directory, for every caller — soconfig_show --workspace …/flotato,tasks_list --workspace …/flotatoandsession_list --repo edobry/flotatoall answered for minsky. Eight sites carried the sameresolveProjectIdentity({ repoPath: process.cwd() })block; the caller's explicitworkspace/repowent unread (andsession.list'srepowas dropped outright bylistSessionsImpl).All eight now go through ONE helper,
resolveReadScope(packages/domain/src/project/read-scope.ts), with the precedence ADR-021 already records for writes:allProjects→workspace→repo(a path, else anowner/nameslug throughprojects.slug) → process cwd.Design decision, recorded in the spec: the explicit rungs do not fail open. ADR-021's "unidentified → ALL_PROJECTS" default is kept byte-for-byte on the cwd rung (a misconfigured ambient cwd still sees something). An argument the caller NAMED that resolves to no project is
unresolvedwith a reason and the read returns nothing — it scopes toNO_PROJECT_SCOPE(the nil uuid, whichprojects.idnever equals), so the query layer'suuid | ALL_PROJECTScontract is untouched. "Everything" is the wrong answer to "show me X"; it is how the onboarding session'stasks_list --workspace …/flotatoreturned 1016 minsky tasks. The ADR-021 amendment rides with mt#5168 (the implicit_metacwd supplier this helper enables).Changes
packages/domain/src/project/read-scope.ts(new):resolveReadScope,readScopeToProjectScope,summarizeReadScope,NO_PROJECT_SCOPE,repoLooksLikePath,slugFromRepoArgument. Deps injectable (identity resolver, path probe, cwd) — the cwd seam is what mt#5168 will fill.tasks.list(query-commands.ts): honoursworkspace/repo;onScopeResolvedseam so the adapter reportsprojectScope(andFound 0 tasks: <reason>in text) without a second resolution.tasks.similar/tasks.search: same, viataskContextParamsthey already declared.session.list:reponow selects a project (it was accepted and dropped); optionalworkspaceadded; result carriesprojectScope.memory.search/list/similar,asks.list,transcripts.search/search-text/similar,tasks.pointings.declare/list,tasks.expire-remainder: optionalworkspaceadded (they declared none, so SC2 was unachievable for them without it — the spec's gate-h text claimedmemory.*"already advertised" it; corrected in the spec).src/mcp/server.tsresolveProjectIdBestEffort(args): presence claims and session attachments take the tool call's ownworkspace/repo; an explicit argument naming no project stampsundefined(a write; the nil-uuid sentinel is read-side only).config.show/list/get/validate/doctor:getConfigProviderForWorkspace(config/helpers.ts) builds aCustomConfigFactoryprovider rooted at the named workspace; else the global. mt#5154's doctor checks build on this.read-scope-census.ts+ test: walkssrc/andpackages/for the cwd-resolving call shape (whitespace/trailing-comma tolerant) and expects zero sites.scripts/excluded — a one-shot script's process IS its caller.scripts/verify-read-scope-from-workspace.ts: the live daemon-from-A/workspace-B run (AT1–AT3, AT5).src/generated/completion-manifest.jsonpicks up the new--workspaceflags andsession list --repo's new description.Contract changes (additive)
Optional
workspaceadded to 11 commands (never required).session.list --repochanges meaning from ignored to a project selector.tasks.listandsession.listresults gain an optionalprojectScope: { kind, source, projectId?, value?, reason? }. No parameter renamed or made required; no env var, config key, or deployed-environment artifact.Test evidence
Live, this branch (verified-1b). Bundle built from this branch; scratch HTTP daemon started from the minsky session checkout (project
3ac3d147…) on 127.0.0.1:48799 with a scratch bearer token.bun scripts/verify-read-scope-from-workspace.ts --workspace /Users/edobry/Projects/flotato --slug edobry/flotato --task mt#5151 --expect-project-id 3ec1323d-8d31-4a5c-9ef0-420ba377bb62— 11/11 PASS (2026-09-14T23:49Z):Negative control (verified-1b). Same script against a PRE-FIX daemon (main's bundle, started from
/Users/edobry/Projects/minskyon :48798): 11/11 FAIL — config from minsky, 500 minsky tasks for--workspace flotato, 200 minsky sessions for--repo edobry/flotato, 20 rows fornonexistent/repo, claim stamped3ac3d147…. The script fails on the defect and passes on the fix.Census negative control. A planted multi-line
resolveProjectIdentity({\n repoPath: process.cwd(),\n})undersrc/maderead-scope-census.test.tsfail naming the file; removed.Unit (verified-1a).
read-scope.test.ts18 pass;read-scope-census.test.ts4 pass;config/helpers.test.ts40 pass (4 new); 697 tests across the 42 directly-affected files pass (bun test --preload ./tests/setup.tsoverpackages/domain/src/project,commands/transcripts,commands/config,commands/memory, session basic-commands, asks, similarity,tests/domain/project-scope-acceptance.test.ts, …). Typecheck clean across all 8 projects; lint clean. The full gated suite runs in CI./Users/edobry/Projects/flotatowas read only (config_show, tasks_list, tasks_get); nothing there was modified.Execution evidence
Execution evidence:
## Inventory(2026-09-14) — 12 rows covering the eight cwd sites,config.show/list/get/validate/doctor, the two already-correct argument-taking commands, and the two write-side seams left unchanged.bun test --preload ./tests/setup.ts packages/domain/src/project/read-scope.test.ts→18 pass 0 fail;read-scope-census.test.ts→4 pass 0 fail(zero cwd-resolving sites insrc/+packages/); live AT2 rows above (--workspace flotato→ 3 rows, overlap with the daemon's project 0;--workspace <no project>→unresolved, 0 rows). CLI path unchanged:resolveReadScope({}, …)with no argument →{ scoped, source: "cwd" }(unit) and the live no-argument control →{scoped, cwd, 3ac3d147}.session.list --repoas path / slug / unresolvable): live AT3 rows above —repo-slug→3ec1323d,repo-path→3ec1323d,nonexistent/repo→unresolved,reason: no project is registered under "nonexistent/repo", 0 rows.config_show --workspace …/flotato --sources→workspace.mainPath /Users/edobry/Projects/flotato, project source…/flotato/.minsky/config.local.yaml,repository.url https://github.com/edobry/flotato.git;src/adapters/shared/commands/config/helpers.test.ts→40 pass 0 fail(4 new: workspace →createProvider({ workingDirectory }), none/blank → global).scripts/verify-read-scope-from-workspace.ts→ALL CHECKS PASSED(11/11) against the fixed daemon;11 CHECK(S) FAILEDagainst the pre-fix daemon; the structural half isread-scope-census.test.ts, whose negative control named a planted site.resolveProjectIdBestEfforttakes the caller's workspace): live AT5 row above — newest claim aftertasks_get taskId:mt#5151 workspace:…/flotatocarriesprojectId 3ec1323d-…; pre-fix daemon:3ac3d147-….Deploy verification
Touches
src/mcp/server.tsand shared adapters → minsky-mcp (hosted) redeploys. On the hosted server no tool call carries a cwd-shaped workspace, so the cwd rung's ADR-021 default is unchanged there; the change is inert for hosted callers that pass noworkspace/repo.After merge I will run
mcp__minsky__deployment_wait-for-latestforminsky-mcpwithnotBefore= merge time andexpectCommitSha= the merge SHA, requireSUCCESS, and confirm the runtime started by reading the health body (service: minsky-mcp,status: ok) and a change-produced assertion — the hosted daemon'ssession_listtool advertising the newworkspaceparameter. The outcome will be recorded here and in the task spec.Review round 1 (
efc671fcd)session.listrepo/workspaceschemas — changed toz.string().optional()to match the memory/asks spelling. The mechanism claim ("calls without--workspacefail") did not hold: the registry treatsrequired: falseas optional regardless of the zod shape — the samez.string()+required: falseshape is this file's pattern fortask,since,until,offsetand eight more, and every livesession_listcall above carried noworkspace. Harmonized anyway; one spelling is better than two.invalid-db-handle→ explicit rungsunresolved, cwd rungall). New unit test pins both halves.repoLooksLikePathcould misclassify a slug that exists as a relative directory — accepted: a bareowner/nameis now always a slug, no on-disk probe; a relative directory is spelled./owner/name. On the shared daemon a relative path would have resolved against the spawner's cwd anyway, so nothing legitimate is lost. Tests updated;session list --repodescription says which spellings are paths.generated— narrowed to thesrc/generatedpath.tasks.pointings.candidatesnot threaded — it resolves no scope: it takes a pointing id and reads the row's own project. Onlydeclare/list/expire-remainderresolve a scope, and all three are threaded.server.ts— both done.Related
mt#2391 (umbrella) · mt#5168 (implicit
_metacwd supplier + ADR-021 amendment; depends on this helper) · mt#5154 (doctor project-scope checks; consumesgetConfigProviderForWorkspace) · mt#4808 / mt#4758 / mt#4772 (write-side precedent) · mt#1427 (global provider staleness — separate) · mt#4639 / PR #3412 (stale log-conversion PR overlapping 8 files; rebase note left).Had Claude implement and verify this; the design decision above is recorded in the task spec.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AVYcp6iaRLEV6gyrfxVVAr