Repository navigation
feat(dsh): Refactor PowerContext integration to utilize MCP tools and implement full ability - #1784
Conversation
| const client = await load<DshMcpClient>('@deepseek-ai/dsh-mcp-client') | ||
| await client.apply(ctx, mcpConfig(config)) |
There was a problem hiding this comment.
Mutations, including candidate approval, now execute without an approval channel: the native MCP client does not enforce approval from annotations. Is dropping the previous per-call approval requirement intentional?
| For MCP operations that require scope_id, call mcp__powercontext__resolve_scope_binding with allow_default: true | ||
| (and the available binding keys) first, then pass only its exact returned scope_id. Do not derive a Scope from a | ||
| directory, branch, repository, prompt, or process working directory. Automatic lifecycle hooks resolve their own | ||
| Scope for recall and Source capture; they do not rewrite MCP arguments. |
There was a problem hiding this comment.
With Scope A configured or workspace-bound, automatic capture uses A, but following the new guidance with resolve_scope_binding(allow_default=true) selects default Scope B. The model receives neither the configured Scope nor its binding key. Please expose the host-resolved Scope so ordinary MCP operations use the same project context.
| if (!operation) return next() | ||
|
|
||
| const mode = usesScope(operation) | ||
| if (mode !== 'none') { |
There was a problem hiding this comment.
[P1] Scope enforcement is skipped for some scoped native MCP tools. usesScope() reads OPERATIONS[operation].scopeMode, and this branch only validates when it is not none; however the generated metadata marks exposed /v1/scopes/{scope_id} MCP operations such as get_scope, list_dream_runs, get_dream_run, and create_dream_run as scopeMode: none. A model can therefore call mcp__powercontext__get_scope or mcp__powercontext__list_dream_runs with a different scope_id and this hook will fall through to next(), contradicting the new docs that DSH refuses calls for another Scope. Please treat path scope_id arguments for exposed MCP tools as host-scope-bound as well, and add a regression covering a scopeMode: none operation with path scope_id.
AlexStocks
left a comment
There was a problem hiding this comment.
Thanks for this — the architecture is sound and the capability surface is genuinely complete (I verified the 46 mcp__powercontext__* ids in capabilities.toml are set-equal to the Server's _MCP_OPERATION_IDS). Requesting changes because the PR removes three guards that would catch the model-facing surface breaking.
Blocking
1. integrations/dsh/README.md:24 still documents the deleted pc_* HTTP path
Not in the 36-file change list, so it was missed:
- named `pc_*` tools call the public `/v1/...` HTTP API;
This is the top-level README of integrations/dsh/ — the first thing a reader sees when browsing the integration. docs/en|zh/docs/integrations/dsh.md, capabilities.md and the plugin README were all updated; this one was not.
Fix: rewrite the bullet to the three-path model (native MCP for model-facing operations, bounded HTTP for lifecycle hooks and /pc), matching docs/en/docs/integrations/dsh.md:30-41.
2. tests/guidance.spec.ts no longer checks that guidance tool names resolve
The old test built the real registered catalog and asserted every name mentioned in the guidance and Skill text exists in it:
registerTools({ tools: { register: tool => tools.push(tool) }, ... }, ...)
const names = new Set(tools.map(tool => tool.name))
for (const name of (...text...).match(/\bpc_[a-z_]+\b/g) ?? [])
expect(names.has(name), `unavailable tool referenced in DSH guidance: ${name}`).toBe(true)The replacement only checks the prefix:
expect([...names].every(name => name.startsWith('mcp__powercontext__'))).toBe(true)
for (const tool of skill!.content.match(/\bmcp__powercontext__[a-z_]+\b/g) ?? [])
expect(tool.startsWith('mcp__powercontext__'), tool).toBe(true)startsWith cannot fail on a name that does not exist. This PR hardcodes 19 MCP names into the Skill text and the guidance string, and nothing verifies them anymore — a typo such as mcp__powercontext__serch_memory passes CI.
Fix: assert the extracted names against the real catalog — Object.keys(OPERATIONS) from src/operations.generated.ts, or the _MCP_OPERATION_IDS-derived set that scripts/integration_manifest.py already uses on the Python side.
3. tests/e2e/unscoped-session.spec.ts deletes three live-Server assertions with no replacement
The -37/+0 diff removes the helper plus three real assertions on the model-facing path:
pc_searchin a session withheader.cwd === undefined→expect(tool).toMatchObject({ ok: true })rememberthenpc_search→expect(found.data?.hits?.some(hit => hit.text === TEXT)).toBe(true)pc_rememberagainst a nonexistent Scope →expect(remembered).toMatchObject({ ok: false, code: 'not_found', error_code: 'scope_not_found', status: 404 })
The third one is the live-Server end-to-end proof of the contract the new docs promise (docs/en/docs/integrations/dsh.md:64-67: "Scope-dependent native MCP tools ... return a controlled failure before dispatch if host Scope resolution fails ..."). Nothing replaces it — tests/mcp.spec.ts covers the equivalent case with mocks only, so the HTTP status and error codes are no longer asserted anywhere against a running Server.
Fix: keep at least the Scope-failure case by driving the MCP policy hook (registerMcpPolicy + resolveScope returning undefined) against the e2e server and asserting the deny reason; or move that assertion into tests/mcp.spec.ts against a live server.
Should fix
4. scripts/integration_manifest.py: the dsh tool-surface probe no longer reads the host's real registrations
Before, DSH_TOOLS parsed src/tools.ts for actual pcTool(...) registrations. Now:
if probe is ToolSurfaceProbe.DSH_MCP:
source = _read(root, "integrations/dsh/plugins/powercontext/src/mcp.ts")
if "@deepseek-ai/dsh-mcp-client" not in source or "mcpEndpoint" not in source:
raise ValueError("DSH MCP bridge is incomplete")
from powercontext.server.mcp import _MCP_OPERATION_IDS
return {f"mcp__powercontext__{operation}" for operation in _MCP_OPERATION_IDS}The plugin side is reduced to two substring checks; the returned set is synthesized from the Server catalog. tests/test_integration_manifest.py also drops the parametrized negative case (missing_operation=False,True), leaving only assert tool_surface_errors(isolated) == ().
I verified the remaining guard still has teeth (removing a declared tool yields unexpected [...]; adding an unknown id or a legacy pc_search yields missing [...]), but what it can no longer detect is "capabilities.toml declares a tool the plugin does not actually expose". It also imports the private _MCP_OPERATION_IDS, which AGENTS.md discourages.
5. The dsh native guidance mapping exercises an operation the host can no longer call
scripts/integration_guidance_native.mjs maps pc_handoff_prepare → prepare_handoff, and prepare_handoff is HTTP-only: present in src/operations.generated.ts:50 but absent from _MCP_OPERATION_IDS (grep of src/powercontext/server/mcp.py → 0 hits). The callMcpTool guard validates against OPERATIONS (the HTTP table), so the check passes while driving a tool that is not in dsh's MCP surface. Separately, tests/test_integration_guidance_evaluation.py:160 still declares dsh's catalog as {"host": "dsh", "tools": [{"name": "pc_search"}]}.
Fix: validate the mapping against the MCP operation catalog, not the HTTP one.
6. Rebase will strand the hand-copied lists
The base's Server catalog is 46 operations; upstream master is at 47 (added prepare_handoff_hint, verified with git show ob-ssh/master:src/powercontext/server/mcp.py). capabilities.toml (46) and the hand-copied MUTATING_MCP_OPERATIONS (25) will both need updating after rebase. The manifest check will catch the toml drift; nothing protects the mutating list.
Suggest adding a small Python test that asserts _MCP_OPERATION_IDS - _MCP_READ_ONLY_OPERATION_IDS ⊆ MUTATING_MCP_OPERATIONS (parsed from src/mcp.ts), so the hand-copied list can never silently fall behind.
Verified as fine — do not re-check
- 26/26 CI green.
@deepseek-ai/dsh-mcp-clientis properly declared. OptionalpeerDependencyat"*"inintegrations/dsh/plugins/powercontext/package.json:47, andtests/runtime/pnpm-lock.yamlactually resolves0.1.2-rc.1. This is not the zero-declaration, zero-pin situation that blocksdsh-app-bootin #1807.- "Full ability" holds. No missing, no extra, no tool lacking either
capabilitiesornon_profile_reason. - The mutating set is conservative, never permissive. The plugin's 25-item
MUTATING_MCP_OPERATIONSis a strict superset of the Server's 12 non-read-only operations (13 extra approvals, 0 missed). lib/index.jsis in sync.Makefile:148-151runsgit diff --exit-codeoverlibafterpnpm build; green CI is the proof.- Docs are updated in both locales, and
docs/en/docs/integrations/dsh.md:22states that the DSH profile must provide the MCP client.
Items 1–3 are all the same class of issue: a guard that would catch the model-facing surface breaking was removed. Fixing them together restores the safety net the rename depends on.
|
@AlexStocks @Teingi all complete, PTAl |
|
@Teingi @AlexStocks PTAL |
|
please update & resolve conflicts |
done @hidb4ai |
|
@hidb4ai @AlexStocks PTAL |
|
please update & resolve conflicts |
hireview001
left a comment
There was a problem hiding this comment.
- Resolve merge conflicts with the target branch.
| export function hasSecretContent(operation: string, payload: unknown): boolean { | ||
| if (PAYLOAD_WRITES.has(operation)) return containsSecretValue(payload) | ||
| const fields = CONTENT_FIELDS.get(operation) | ||
| if (!fields || !payload || typeof payload !== 'object') return false |
There was a problem hiding this comment.
[P2] Extend secret rejection to the newly exposed content writes
Newly exposed Work Contract, task outcome, and candidate proposal/revision operations are absent from both secret-check tables: invoking the policy with create_work_contract and contract.objective: "api_key=FAKE_REVIEW_CANARY" returns ask, allowing transmission and persistence after approval. Include their caller-authored content fields in secret validation before approval or dispatch.
@hidb4ai fixed PTAL |
Which issue or RFC does this PR close?
Relates to #1450
Rationale for this change
The DSH integration previously exposed PowerContext operations through direct HTTP-backed tools. This caused tool naming and endpoint failures, did not expose the complete PowerContext capability surface, and made routing inconsistent with the DSH MCP model.
This change moves all model-facing PowerContext operations to the native DSH MCP client while retaining HTTP only for host lifecycle control such as automatic prepare, Source capture, flush, and
/pcdiagnostics.What changes are included in this PR?
@deepseek-ai/dsh-mcp-clientintegration using the PowerContext/mcpendpoint.mcp__powercontext__*tools.pc_*tool wrappers./pccommands as host lifecycle/control-plane operations.Are there any user-facing changes?
Yes.
pc_*wrappers to native names such asmcp__powercontext__search_memory.@deepseek-ai/dsh-mcp-client./mcpendpoint./pccommands.How was this change tested?
pnpm --dir integrations/dsh/plugins/powercontext buildpnpm --dir integrations/dsh/plugins/powercontext testpnpm --dir integrations/dsh/plugins/powercontext test:e2epnpm --dir integrations/dsh/plugins/powercontext test:allpnpm --dir integrations/dsh/plugins/powercontext/tests/runtime testuv run pytest tests/test_integration_manifest.py -qmake integration-manifest-checkuv run python scripts/generate_integration_manifest_docs.py --checkuv run ruff check scripts/integration_manifest.py tests/test_integration_manifest.pygit diff --checkThe deterministic pinned DSH runtime acceptance passed, including native MCP tool calls, Scope failures, automatic recall/capture, snapshot persistence, and recovery scenarios.
uv run ty checkremains affected by existing Windows/platform diagnostics unrelated to this change.AI usage statement
OpenAI Codex, based on GPT-5, was used through the Codex desktop application for repository inspection, implementation, test execution, and preparation of this pull request description.