Enforce the destructive-operation confirmation flow and fix error reporting#47
Merged
Conversation
Reject malformed numeric and ID inputs at startup instead of silently truncating, classify HTTP failures by status code rather than message sniffing, and rebuild the abilities cache around per-signature slots with in-flight de-duplication so concurrent requests for different dashboards or namespaces can never serve each other's data. Claude-Session: https://claude.ai/code/session_01FCAjEih9Rry2hdTrJP9eou
Share one config fixture and mock-logger factory across eval and integration suites, fold the standalone security unit tests into the colocated suite, and replace not-toThrow smoke assertions with checks on interpolated output. Claude-Session: https://claude.ai/code/session_01FCAjEih9Rry2hdTrJP9eou
Run tsc --noEmit in CI, include scripts/ in the lint and typecheck project, and ignore runtime log output. Claude-Session: https://claude.ai/code/session_01FCAjEih9Rry2hdTrJP9eou
Confirmation flow: previews only send dry_run when the ability schema declares it (confirm-only abilities get a token-issuing confirmation step with no upstream call); bare destructive calls on confirm-capable tools now return PREVIEW_REQUIRED instead of proceeding. Tool permissions: shared allow/block predicate enforced at all three execution entry points (tool calls, site resource, completions). Caching: tool conversion clones ability schemas before enrichment so the abilities cache is never mutated, eliminating spurious tool_list_changed notifications on every TTL refresh; change detection fingerprints schemas and annotations, not just names. Transport: request timeout stays armed through body reads and maps to retryable ETIMEDOUT while external aborts remain cancellations; query serialization rejects unsupported nested input instead of silently sending "[object Object]"; pagination no longer warns falsely when the page count exactly hits the cap. Config and docs: malformed boolean env vars fail startup; bearer-token auth is consistently documented as expected to fail against the Abilities API, with a startup warning when no complete Application Password pair is configured; prompt arguments get transport-shape validation; structured HTTP statuses map to MCP error codes before message sniffing. Adds src/index.test.ts covering server handlers in-memory. Claude-Session: https://claude.ai/code/session_01Bc1uZtuMhp4GEDLnu1amBh
… tests Follow-ups from the second review round and a pre-PR audit of the branch. Behavior changes: - Passing dry_run: true to an ability that does not declare a dry_run parameter is rejected with an invalid-parameter error. It used to be forwarded upstream, where a handler ignoring unknown input would have run the destructive operation without confirmation. - The ability cache signature now includes a hash of the auth identity, so a server constructed with different credentials cannot read another identity's cached catalog. - Unknown tool names return TOOL_NOT_FOUND (-32003) via the dedicated factory instead of falling through message sniffing to RESOURCE_NOT_FOUND (-32002). The test pins the numeric code. Test suite: - tests/integration/ folded into the unit suites: of its 32 cases, 23 duplicated existing unit tests through the same mocked fetch and 9 unique ones were ported into src/*.test.ts. The parallel fixture catalog it required is gone with it. - New src/session.test.ts covers the session data cap, which had no coverage on its RESOURCE_EXHAUSTED throw (session.ts 67% -> 96%). - Removed an eval test that could not fail (expect(true) guarding a staleness warning that read an untracked file, so CI checked nothing). - Repeated Config literals and copy-pasted cases replaced with a shared makeBaseConfig helper and it.each tables. Manual test harness: destructive tests are excluded unless requested and the plugin lifecycle tests only touch a safelist of throwaway plugins, after a run auto-picked and deleted a production plugin. Docs and config: README, security, troubleshooting, and configuration docs aligned with the strict confirmation gating and bearer-token behavior; resources table now lists the site and tool-help templates; CHANGELOG entries for everything above, with the gating change flagged as breaking. Claude-Session: https://claude.ai/code/session_01NnJiXybDffmpEsdwE1cnYT
Iterations 3-5 of the CR review loop. The two guards in index.ts that gate the mainwp://site resource and site-id completions checked hardcoded tool names, so allow/block lists keyed on namespaced names (when mainwp is not the primary namespace) never matched. Both now derive the exposed name via abilityNameToToolName. The completion failure log moved from debug to info so auth problems show up in production, where the comment already claimed they would. manual-test.ts no longer auto-picks Akismet for lifecycle tests (its uninstall drops the configured API key, so deleting it is not free), rejects a --plugin-slug value that is missing or looks like a flag, and only selects inactive plugins so a default run cannot leave hello-dolly deactivated. Explicit --plugin-slug still accepts anything. The bare destructive call rejection now carries a reason that matches the actual case (no confirmation parameters) instead of the user_confirmed-specific text. README parameter lists for the five destructive tools gain the confirmation_token field they were missing. Four findings were rejected and logged in .mwpdev REVIEW_DECISIONS.md instead: PREVIEW_REQUIRED doc wording, session counter reset in createServer, AbortError-only timeout classification, and host-based skipSslVerify restriction. Claude-Session: https://claude.ai/code/session_01NnJiXybDffmpEsdwE1cnYT
A PHP dashboard serializing an empty properties map as [] (seen live on
two experiment-branch abilities) invalidated the entire tools/list
response for spec-compliant MCP clients: the official SDK rejects the
payload, leaving the server connected with zero usable tools. Coerce
non-object properties to {} and drop malformed required entries, per
the treat-remote-schemas-as-hostile rule.
Claude-Session: https://claude.ai/code/session_01NnJiXybDffmpEsdwE1cnYT
…nt layers tests/acceptance runs the local tree as an installed npm package: npm pack, clean consumer install, real stdio MCP client, scenarios verified against independent Abilities API reads. Fixture dashboard enables the credential-free CI job; --writes gates state-changing scenarios behind a host allowlist; agent-run.ts drives claude -p headless with env- placeholder MCP config so credentials never touch disk. Artifacts (manifest, JSONL events, results, summary, server stderr) land in test-results/acceptance/ with secret redaction. Claude-Session: https://claude.ai/code/session_01NnJiXybDffmpEsdwE1cnYT
…tion flow deterministically A live Dashboard returns HTTP 403 with code mainwp_site_not_found for a nonexistent site; the fallback classifier trusted the status before the structured code, so MCP clients received PERMISSION_DENIED (-32008) instead of RESOURCE_NOT_FOUND (-32002). Structured not-found codes (*_not_found, rest_no_route) now classify first. The not-found and invalid-args acceptance scenarios pin their exact error codes so a fixture/live divergence can no longer pass silently. Add fixture-confirmation-flow: a fixture-targeted write scenario that exercises the full destructive confirmation path (dry-run preview, forged-token rejection, token-bound execution, replay rejection) with independent state verification, running in the default fixture suite so CI covers the confirmation gate on every run. The fixture dashboard executes delete_site_v1 in memory when confirmed, and fixture-target writes bypass the live host guard while live gating stays unchanged. Claude-Session: https://claude.ai/code/session_01NnJiXybDffmpEsdwE1cnYT
…overage The agent layer can now target the fixture dashboard: the new agent-confirm-delete-site scenario gives a headless model a natural-language deletion task and grades the transcript deterministically (preview returning CONFIRMATION_REQUIRED with a token, confirmed call with that token, independent verifier proving exactly one site removed). prompt-completions verifies update-workflow completion values against the independently fetched ability schema and proves policy enforcement at the completion surface (blocked list_sites_v1 denies site-id completions). Fixture-only transport scenarios exercise the oversized response cap and request timeout via opt-in fault modes, asserting structured errors and same-session recovery. test:acceptance:human chains fixture, writes, and agent layers as a single entry point. Claude-Session: https://claude.ai/code/session_01NnJiXybDffmpEsdwE1cnYT
check-site cross-checks the MCP result against an independent direct execution and enforces a 20-second latency ceiling, so the Dashboard's dns_get_record(DNS_ANY) stall class (mainwp/mainwp-6#180, not yet on main) fails the suite instead of hiding in a passing run. It runs on live and fixture targets. site-themes mirrors the plugin inventory cross-check. list-updates brackets the MCP snapshot between two direct reads so live churn is tolerated without weakening the assertion. clients-count-consistency and list-tags-cross-check paginate both sides fully and compare complete inventories, which stays meaningful when the testbed has zero of either. Claude-Session: https://claude.ai/code/session_01NnJiXybDffmpEsdwE1cnYT
Confirmed execution fell back to matching the pending preview by tool name and arguments when no confirmation_token was supplied, so user_confirmed: true with the same arguments executed the destructive operation without the token, letting a caller confirm a preview it never read. The agent acceptance scenario caught this live: a model that omitted the token still deleted the site. Case 3 now rejects tokenless confirmation with PREVIEW_REQUIRED and a reason naming the missing parameter, before any upstream call. The issued token stays valid after a rejected attempt. Unit regression covers the pending-preview bypass and the expiry path now carries the token; the fixture-confirmation-flow scenario asserts the tokenless rejection with unchanged state. Claude-Session: https://claude.ai/code/session_01NnJiXybDffmpEsdwE1cnYT
Add five scenarios: a nonexistent-site hallucination trap, a tags subsystem question, a two-tool theme chain, a safe-mode refusal that requires a correlated SAFE_MODE_BLOCKED result, and a full-coverage site-status check. New serverEnv plumbing lets a scenario launch the packed server with extra env (used for MAINWP_SAFE_MODE). Negation-aware answer matchers live in the exported lib/agent-matchers.ts module with counterexample unit suites, including two regressions from live transcripts (a remedy suggestion mentioning safe mode off, and a long probe hostname overflowing a matcher gap). Verified 9/9 PASSED against the testbed.
Two live agent runs produced correct absence answers the matcher missed: "isn't in your MainWP Dashboard" (membership phrasing, no absence verb) and "has no site named X" (verdict 140 chars away inside an affirmative clause). Accept a relayed mainwp_site_not_found error code as phrasing-independent absence evidence (the exists-claim guard still runs first), add membership and no-site-named patterns, and pin both live transcripts plus the error-code accept/reject pair as unit regressions. Claude-Session: https://claude.ai/code/session_01Cge9BoBa6AHp9Pqo1fbMZf
A public package installing a global command named mcp collides with other MCP tooling; nothing documented or used it. npx @mainwp/mcp still resolves since a single bin entry is used unconditionally. The packed acceptance assertions now check only mainwp-mcp. Also remove two unreferenced logo files and replace a testbed-flavored hostname example in an agent-run comment with a generic one. Claude-Session: https://claude.ai/code/session_01Cge9BoBa6AHp9Pqo1fbMZf
The box-drawing warning becomes three plain stderr lines, and the section banners in the prompt definitions array duplicated the name field directly below each one. Claude-Session: https://claude.ai/code/session_01Cge9BoBa6AHp9Pqo1fbMZf
"Once safe mode is off, re-run" contains a copula and tripped the disabled-state guard even though the block was reported faithfully. The guard now skips copulas preceded by a conditional marker; the live transcript phrasing is pinned as a regression. Claude-Session: https://claude.ai/code/session_01Cge9BoBa6AHp9Pqo1fbMZf
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds packed acceptance testing and CI coverage while tightening configuration validation, ability caching, confirmation tokens, authorization checks, transport timeout handling, structured errors, tool-schema sanitization, and related documentation and tests. ChangesRuntime behavior and contracts
Acceptance harness
Delivery and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
npm 10 runs a dependency's prepare script during npm pack despite --ignore-scripts (fixed in npm 11), so the registry step died in CI on eventsource's prepare build while passing locally under npm 11. Stage a copy of each dependency, strip prepare/prepack/postpack, and pack the copies so the result does not depend on the npm version. Verified with the fixture layer under both npm 10.9.8 and 11.16.0. Claude-Session: https://claude.ai/code/session_01Cge9BoBa6AHp9Pqo1fbMZf
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tool-schema.ts (1)
38-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSanitize each property schema before mutating it.
Lines 38–53 validate only the
propertiescontainer. A payload such as{ properties: { site_id: "bogus" } }reachesprop.description = ...and throws, causing the whole tool list to fail instead of isolating the malformed property.Proposed fix
- const properties = ( + const rawPropertyMap = ( rawProperties && typeof rawProperties === 'object' && !Array.isArray(rawProperties) ? rawProperties : {} - ) as { [key: string]: Record<string, unknown> }; + ) as Record<string, unknown>; + const properties: Record<string, Record<string, unknown>> = {}; + + for (const [name, prop] of Object.entries(rawPropertyMap)) { + properties[name] = + prop !== null && typeof prop === 'object' && !Array.isArray(prop) + ? (prop as Record<string, unknown>) + : {}; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tool-schema.ts` around lines 38 - 53, Sanitize each entry in the properties loop before accessing or mutating it: only process non-null object schemas and skip malformed values such as strings, preserving the valid property entries and preventing one invalid schema from aborting tool-list processing. Update the loop around properties and the prop.description assignment; keep the existing description backfill behavior for valid property objects.
🟠 Major comments (23)
.github/workflows/ci.yml-62-63 (1)
62-63: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable checkout credential persistence in the acceptance job.
This job executes repository-controlled packed and fixture code, while
actions/checkoutcurrently leavesGITHUB_TOKENin.git/configfor child processes. Addpersist-credentials: falseto prevent acceptance code from reading the token.Proposed fix
- name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 62 - 63, Update the actions/checkout step in the acceptance job to set persist-credentials to false, ensuring child processes cannot read the GITHUB_TOKEN from .git/config.Source: Linters/SAST tools
tests/acceptance/agent-run.ts-844-879 (1)
844-879: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a deadline for the spawned agent process.
--max-turnsdoes not cover startup, transport, or shutdown hangs. A stuck CLI can block the acceptance job indefinitely; terminate it after a bounded timeout and awaitclose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/agent-run.ts` around lines 844 - 879, Update runClaude to enforce a bounded deadline for the spawned child process, covering startup, transport, and shutdown hangs. On timeout, terminate the child, then await its close event before resolving with the appropriate failure exit code; ensure the timer is cleared when the process closes normally or errors.tests/acceptance/agent-run.ts-234-241 (1)
234-241: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire the tool results to prove that the site is absent.
Any correlated lookup result currently passes, so an incomplete inventory plus a canned “not found” answer can satisfy this scenario. Require either
mainwp_site_not_foundor a complete inventory demonstrably excluding the probe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/agent-run.ts` around lines 234 - 241, Update the correctMcpResult pass condition to require proof that the site is absent: accept mainwp_site_not_found, or require a complete inventory whose results demonstrably exclude the probe site. Do not allow merely having lookupUses and lookupResults to pass; preserve the existing evidence fields and use the available truth symbols to validate completeness and exclusion.tests/acceptance/agent-run.ts-1032-1039 (1)
1032-1039: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not disable TLS verification for every live target.
This exposes live Basic credentials and destructive requests to certificate spoofing. Default verification to enabled and require an explicit opt-in for self-signed testbeds.
Proposed fix
- MAINWP_SKIP_SSL_VERIFY: scenario.target === 'live' ? 'true' : 'false', + MAINWP_SKIP_SSL_VERIFY: + scenario.target === 'live' + ? (process.env.MAINWP_SKIP_SSL_VERIFY ?? 'false') + : 'false',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/agent-run.ts` around lines 1032 - 1039, Update the environment construction in the agent-run acceptance setup so MAINWP_SKIP_SSL_VERIFY defaults to 'false' for live targets, and is enabled only through an explicit self-signed-testbed opt-in configuration. Preserve the existing fixture HTTP behavior and ensure live credentials and requests use TLS verification unless that opt-in is provided.tests/acceptance/lib/guards.ts-8-15 (1)
8-15: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not implicitly allow every
.localdashboard.A
.localhostname can identify a real LAN dashboard, so--writesmay authorize destructive acceptance scenarios without explicit host approval. Keep loopback defaults and require all other hosts inMAINWP_MCP_ACCEPTANCE_WRITE_HOSTS.Proposed fix
host === 'localhost' || host === '127.0.0.1' || - host.endsWith('.local') || additionalHosts.some(candidate => candidate.toLowerCase() === host)Also applies to: 27-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/guards.ts` around lines 8 - 15, Update isWriteHostAllowed to remove the host.endsWith('.local') implicit allowlist entry. Preserve localhost and 127.0.0.1 loopback access, and require every other hostname—including .local hosts—to match an entry in additionalHosts case-insensitively.tests/acceptance/lib/agent-matchers.ts-165-168 (1)
165-168: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire the safe-mode result to be marked as an error.
A successful payload merely containing
SAFE_MODE_BLOCKEDcurrently passescorrectMcpResult, so the scenario does not verify structured error classification.Proposed fix
- return result ? containsSafeModeBlocked(result.content) : false; + return result?.isError === true && containsSafeModeBlocked(result.content);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/agent-matchers.ts` around lines 165 - 168, Update the blockedCall matching logic in correctMcpResult to require that the associated result is marked as an error in addition to containing SAFE_MODE_BLOCKED. Preserve the existing tool-use lookup and content check, but reject successful payloads lacking the structured error classification.tests/acceptance/run.ts-119-125 (1)
119-125: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not disable TLS verification by default for live runs.
Live mode always sets
MAINWP_SKIP_SSL_VERIFY=truewhile transmitting dashboard credentials. This permits interception by an untrusted endpoint. Keep certificate verification enabled and require an explicit opt-in environment flag for self-signed test dashboards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/run.ts` around lines 119 - 125, Update the environment object returned by the acceptance test configuration to stop setting MAINWP_SKIP_SSL_VERIFY automatically for live targets. Keep TLS verification enabled by default, and only add that variable when an explicit environment opt-in flag requests self-signed dashboard support; preserve the fixture-specific MAINWP_ALLOW_HTTP behavior.tests/acceptance/lib/commands.ts-38-58 (1)
38-58: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a timeout and terminate stalled commands.
run()can wait indefinitely forclose, so a hungnpm install, registry operation, or lifecycle script stalls the entire acceptance job. Accept a timeout orAbortSignal, kill the child on expiry, and still record the timed-out result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/commands.ts` around lines 38 - 58, The run method must stop waiting indefinitely for stalled child processes. Add timeout or AbortSignal support to the options, terminate the spawned child when triggered, and resolve with a recorded timed-out CommandResult while preserving normal stdout, stderr, exit-code, and error handling.tests/acceptance/lib/server.ts-106-119 (1)
106-119: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake server cleanup exception-safe.
A failed connection never explicitly closes
transport; likewise, ifrawClient.close()rejects, command recording andrmSyncare skipped whileclosedprevents retry. Close the transport/client and removeisolationRootfromfinallyblocks so failed scenarios cannot orphan processes or directories.Also applies to: 126-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/server.ts` around lines 106 - 119, Make the connection and shutdown cleanup in the server acceptance flow exception-safe: ensure transport and rawClient are closed regardless of whether connect or close rejects, and move command recording plus isolationRoot removal into finally blocks. Update the logic around rawClient.connect and rawClient.close so cleanup is retried or still performed even when an earlier cleanup operation fails, without letting the closed guard suppress required cleanup.tests/acceptance/lib/artifacts.ts-95-98 (1)
95-98: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRedact stderr as a stream, not per chunk.
tests/acceptance/lib/server.tsforwards arbitrary stream chunks here. A credential split between two chunks will not match either redaction pass and can leak into live acceptance artifacts. Buffer an overlap at least as long as the largest replacement, then flush it when the server closes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/artifacts.ts` around lines 95 - 98, Update appendServerStderr and its server-close flow so stderr redaction operates across arbitrary chunks rather than independently per value. Maintain a per-server overlap buffer at least as long as the longest redaction replacement, redact only safe completed content on each append, and flush the remaining buffered content when the server closes before writing the final artifact.tests/acceptance/run.ts-399-399 (1)
399-399: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail acceptance when scenarios are unverified.
Precondition and independent-verification errors become
unverified, but the runner exits successfully when no assertion is marked failed. This allows unavailable or broken verification infrastructure to produce a green CI job.Proposed fix
- return runResults.totals.failed > 0 ? 1 : 0; + return runResults.totals.failed > 0 || runResults.totals.unverified > 0 ? 1 : 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/run.ts` at line 399, Update the acceptance runner’s exit-status logic in the return expression using runResults.totals so unverified scenarios produce a failing exit code, alongside failed assertions. Preserve the successful exit code only when both failed and unverified totals are zero.tests/acceptance/scenarios/writes.ts-26-44 (1)
26-44: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not silently accept a tokenless confirmation response during cleanup.
When
statusisCONFIRMATION_REQUIREDbut the token is absent, this branch returns successfully. Cleanup can then finish while leaving the live plugin disabled.Proposed fix
- if (data && data.status === 'CONFIRMATION_REQUIRED' && data.confirmation_token) { + if (data?.status === 'CONFIRMATION_REQUIRED') { + if (!data.confirmation_token) { + throw new Error(`${tool} required confirmation but issued no token`); + } const confirmed = await ctx.client.callTool(tool, {Also applies to: 146-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/scenarios/writes.ts` around lines 26 - 44, Update the non-confirmation path in the write flow around ctx.client.callTool to treat any CONFIRMATION_REQUIRED response without a confirmation_token as an error. Throw a descriptive error instead of returning successfully, while preserving the existing confirmed retry when a token is present and the current handling for other statuses.tests/acceptance/scenarios/ability-reads.ts-116-187 (1)
116-187: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply one bounded-pagination policy across the acceptance harness.
Every implementation trusts mutable response metadata and can loop indefinitely on malformed or non-progressing pages.
tests/acceptance/scenarios/ability-reads.ts#L116-L187: add a page cap and repeated/non-progressing page detection to all four helpers.tests/acceptance/lib/verify.ts#L151-L163: apply the same guard to direct site pagination.tests/acceptance/scenarios/types.ts#L129-L140: apply the same guard to MCP site pagination.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/scenarios/ability-reads.ts` around lines 116 - 187, Introduce one shared bounded-pagination policy for verifierListAll, mcpListAll, verifierListUpdates, and mcpListUpdates in tests/acceptance/scenarios/ability-reads.ts:116-187, including a maximum page limit and detection of repeated or non-progressing pages before continuing. Apply the identical guard to direct site pagination in tests/acceptance/lib/verify.ts:151-163 and MCP site pagination in tests/acceptance/scenarios/types.ts:129-140; preserve normal completion and MCP error handling while preventing reliance on mutable response totals.tests/acceptance/scenarios/configuration.ts-43-46 (1)
43-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert the required packaging checks explicitly.
Iterating the producer-supplied object is vacuously successful when a required check is omitted. Compare its keys against an independently defined set covering tarball contents, CLI bin, entry point, and installed version before asserting values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/scenarios/configuration.ts` around lines 43 - 46, Update the packaging-check assertions in the scenario around ctx.packedPackage.checks to validate an independently defined required-key set covering tarball contents, CLI bin, entry point, and installed version, rather than iterating only producer-supplied keys. Assert that all required keys are present and true while preserving the existing unavailable-metadata error.tests/acceptance/scenarios/types.ts-142-171 (1)
142-171: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestrict live scenario candidates to connected, reachable sites.
The current helpers can fail on an unusable site before reaching a valid candidate.
tests/acceptance/scenarios/types.ts#L142-L171: filter candidates by connectivity and continue past individual inventory-read failures.tests/acceptance/scenarios/writes.ts#L72-L80: choose a connected site instead of the first listed site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/scenarios/types.ts` around lines 142 - 171, Update tests/acceptance/scenarios/types.ts lines 142-171 so findSiteWithPlugins and findHelloDolly consider only connected, reachable sites, continue when an individual getSitePlugins inventory read fails, and return a valid candidate when one exists. Update tests/acceptance/scenarios/writes.ts lines 72-80 to select a connected site rather than blindly using the first listed site.tests/acceptance/lib/verify.ts-190-203 (1)
190-203: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a hard deadline to verifier requests
tests/acceptance/lib/verify.ts:195-202—request()here can wait indefinitely if the dashboard stalls. Pass anAbortSignaland a bounded timeout so the acceptance verifier cannot outlive its scenario budget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/verify.ts` around lines 190 - 203, Update requestJsonResponse to create a bounded timeout AbortSignal and pass it to request alongside the existing method, headers, body, and dispatcher options. Ensure the timeout is cleaned up appropriately so verifier requests cannot wait indefinitely or outlive the scenario budget.src/tool-schema.ts-296-303 (1)
296-303: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the advertised confirmation flow aligned with token enforcement.
The standard flow omits
confirmation_token, while both modes promise a preview even whenhasDryRunis false. Additionally, the schema injects onlyuser_confirmed; schemas withadditionalProperties: falsecannot submit the required token. This makes confirmed destructive calls fail despite following the advertised workflow.Add
confirmation_tokento the injected properties, require callers to reuse the preview response token, and describe the no-dry_runpath accurately.Also applies to: 325-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tool-schema.ts` around lines 296 - 303, Update the destructive-tool confirmation workflow in the isDestructive && hasConfirm block to include confirmation_token in the injected schema properties. Require the execution call to reuse the token returned by the preview response, and ensure schemas with additionalProperties disabled accept it. Revise the advertised steps so the preview behavior accurately reflects whether hasDryRun is enabled, including the no-dry_run path.src/abilities.ts-344-352 (1)
344-352: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate each fetched entry before reading
name.
paginateApi<Ability>is only a compile-time cast over untrusted JSON. If the dashboard returnsnullor an object with a non-stringname,isAllowedNamespace(a.name, ...)throws before the malformed-name guard, poisoning the entire catalog refresh.Proposed fix
const newAbilities = allAbilities.filter(a => { + if (!a || typeof a !== 'object' || typeof a.name !== 'string') { + logger?.warning('Skipping malformed ability entry'); + return false; + } if (!isAllowedNamespace(a.name, namespaces)) return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/abilities.ts` around lines 344 - 352, Validate each fetched ability entry in the process callback before accessing its name: ensure the entry is non-null and has a string name, skip invalid entries safely, then apply isAllowedNamespace and ABILITY_NAME_RE to valid names. Keep the existing warning behavior for malformed names and prevent invalid dashboard data from aborting catalog refresh.src/abilities.ts-125-141 (1)
125-141: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInclude transport-security limits in the cache signature.
This module-level cache can be shared across server configurations, but the signature omits
skipSslVerifyandmaxResponseSize. A strict instance can therefore reuse data fetched by another instance with certificate validation disabled or a larger response-size allowance.Proposed fix
- return JSON.stringify([config.dashboardUrl, config.abilityNamespaces, authIdentity]); + return JSON.stringify([ + config.dashboardUrl, + config.abilityNamespaces, + config.skipSslVerify, + config.maxResponseSize, + authIdentity, + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/abilities.ts` around lines 125 - 141, Update cacheSignature to include config.skipSslVerify and config.maxResponseSize in the returned cache signature, ensuring cache entries and in-flight fetches remain isolated when transport-security limits differ. Preserve the existing dashboard, namespace, and hashed authentication identity components.src/index.ts-268-276 (1)
268-276: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply policy filtering to every tool-derived resource.
This gate protects only
mainwp://site/{id}.mainwp://abilities,mainwp://help, andmainwp://help/tool/{tool_name}still expose blocked or non-allowlisted ability schemas and documentation. Filter those resource paths throughisToolAllowedas well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 268 - 276, Extend the resource access checks in the handler containing the existing getSiteToolName gate so every tool-derived resource—mainwp://abilities, mainwp://help, and mainwp://help/tool/{tool_name}—is filtered through isToolAllowed. Derive each path’s corresponding tool name with the existing abilityNameToToolName convention, and deny access using the same McpErrorFactory.permissionDenied behavior when blocked.tests/acceptance/lib/client.ts-52-62 (1)
52-62: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not include raw tool output in parse failures.
Line 61 copies the entire response into the exception, potentially exposing credentials, PII, or other dashboard data in test logs and artifacts. Report only safe metadata such as content length.
Proposed fix
- throw new Error(`Tool result was not JSON: ${text.text}`, { cause: error }); + throw new Error(`Tool result text was not valid JSON (${text.text.length} characters)`, { + cause: error, + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/client.ts` around lines 52 - 62, Update parseToolJson so JSON parse failures do not include the raw text from the tool response in the thrown error. In the catch block, retain the original error as the cause and report only safe metadata, such as text.length, while preserving the existing missing-text error behavior.src/confirmation.ts-184-204 (1)
184-204: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStrip or reject
confirmwhen honoring explicitdry_run.Lines 189-204 forward
{ confirm: true, dry_run: true }unchanged for declared dry-run tools, before token validation. Removeconfirmor reject this ambiguous combination so upstream confirmation semantics cannot trigger execution.Proposed fix
logger.debug('Explicit dry_run bypasses confirmation flow', { toolName }); - return { action: 'skip' }; + const { confirm: _confirm, ...dryRunArgs } = effectiveArgs; + return { action: 'execute', effectiveArgs: dryRunArgs };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/confirmation.ts` around lines 184 - 204, Update the explicit dry-run branch in the confirmation flow to handle args containing both confirm: true and dry_run: true before returning the skip action. Reject this ambiguous combination or remove confirm from the forwarded arguments, while preserving the existing undeclared-dry_run validation and dry-run bypass behavior.src/index.ts-591-594 (1)
591-594: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve the entrypoint check for the installed bin symlink.
src/index.ts:592comparesimport.meta.urltopathToFileURL(process.argv[1]).href, which can skipmain()whenmainwp-mcpis invoked fromnode_modules/.bin. Compare real paths instead, or useimport.meta.mainif that stays within the supported Node range.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 591 - 594, Update the entrypoint guard in src/index.ts around the top-level main invocation to resolve symlinks before comparing the module URL with process.argv[1], or use import.meta.main if supported by the project’s Node range. Ensure invoking the installed mainwp-mcp bin still executes main(), while preserving the behavior that imports used by tests do not start the server.
🟡 Minor comments (6)
docs/troubleshooting.md-141-155 (1)
141-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign stale-token guidance with
PREVIEW_EXPIRED.Line 141 says a stale token yields
PREVIEW_REQUIRED, but the followingPREVIEW_EXPIREDsection says expiry occurs after five minutes. Document stale tokens under the actual expiry error and reserve this section for missing or mismatched preview state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/troubleshooting.md` around lines 141 - 155, Update the destructive-operation troubleshooting guidance to remove stale or expired confirmation tokens from the `CONFIRMATION_REQUIRED` cases and document them under the `PREVIEW_EXPIRED` section, including the five-minute expiry behavior. Keep `CONFIRMATION_REQUIRED` focused on missing or mismatched preview state, such as confirming without a valid preceding preview.docs/security.md-120-120 (1)
120-120: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSpecify a language for this fenced example.
The unlabeled fence triggers markdownlint MD040.
Proposed fix
-``` +```text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/security.md` at line 120, Update the unlabeled fenced example in docs/security.md to specify the text language by adding the text fence annotation, preserving the example content and closing fence.Source: Linters/SAST tools
tests/acceptance/agent-run.ts-779-783 (1)
779-783: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not depend on JSON property order.
This regex only matches when
slugprecedesactive. A semantically identical response with reversed fields fails acceptance; parse and inspect matching objects instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/agent-run.ts` around lines 779 - 783, Update the plugin validation branch using truth.pluginSlug and truth.pluginActive so it parses the response and inspects matching objects by property values rather than relying on JSON field order. Preserve the existing slug escaping and active-state comparison, and return true when any matching plugin object satisfies both conditions.tests/acceptance/lib/pack.ts-47-51 (1)
47-51: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove
tempRootwhen setup fails.If packing, registry startup, or installation throws, no
PackedPackagereachesrunAcceptance, so its cleanup cannot run. The current pipeline failure follows this path and leaves the temporary tree behind. Wrap setup in a catch that removestempRootbefore rethrowing unless preservation was requested.Also applies to: 83-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/pack.ts` around lines 47 - 51, Update the setup flow around tempRoot, packResult, registry startup, and installation to catch setup failures and remove tempRoot before rethrowing when preservation is not requested. Keep the temporary tree intact when the existing preservation option is enabled, and leave successful runAcceptance cleanup unchanged.Source: Pipeline failures
tests/acceptance/lib/harness.test.ts-50-63 (1)
50-63: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winTest the stated no-expansion and no-persistence guarantees.
This passes even if
parseAcceptanceEnvmutatesprocess.env, and the input contains no variable reference whose literal preservation is checked. Snapshot the relevant environment keys and include a value such as$HOME.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/harness.test.ts` around lines 50 - 63, Strengthen the parseAcceptanceEnv test by snapshotting the relevant process.env keys before parsing and asserting they are unchanged afterward. Add a variable value containing a literal reference such as $HOME, then assert the parsed result preserves that text without expansion while retaining the existing field expectations.tests/acceptance/scenarios/read.ts-171-183 (1)
171-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a deterministic nonexistent identifier.
99999999can be a valid live site ID. Use the reserved probe hostname already used by this harness, such asnonexistent-acceptance-probe.invalid, so this reliably tests not-found classification.Proposed fix
- const result = await ctx.client.callTool('get_site_v1', { site_id_or_domain: 99999999 }); + const result = await ctx.client.callTool('get_site_v1', { + site_id_or_domain: 'nonexistent-acceptance-probe.invalid', + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/scenarios/read.ts` around lines 171 - 183, Update the notFoundInput scenario’s get_site_v1 call to use the harness’s reserved nonexistent probe hostname, such as nonexistent-acceptance-probe.invalid, instead of the potentially valid numeric site_id_or_domain value. Keep the existing structured error assertions and assertSessionRecovery flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/confirmation.ts`:
- Around line 327-330: Update getPreviewKey and its serialization helper to
recursively canonicalize nested argument objects before generating the preview
key, ensuring differing nested values such as settings.role produce distinct
keys and cannot bypass confirmation binding. Preserve the existing top-level key
ordering and add a test covering a nested argument swap through the confirmation
flow.
---
Outside diff comments:
In `@src/tool-schema.ts`:
- Around line 38-53: Sanitize each entry in the properties loop before accessing
or mutating it: only process non-null object schemas and skip malformed values
such as strings, preserving the valid property entries and preventing one
invalid schema from aborting tool-list processing. Update the loop around
properties and the prop.description assignment; keep the existing description
backfill behavior for valid property objects.
---
Major comments:
In @.github/workflows/ci.yml:
- Around line 62-63: Update the actions/checkout step in the acceptance job to
set persist-credentials to false, ensuring child processes cannot read the
GITHUB_TOKEN from .git/config.
In `@src/abilities.ts`:
- Around line 344-352: Validate each fetched ability entry in the process
callback before accessing its name: ensure the entry is non-null and has a
string name, skip invalid entries safely, then apply isAllowedNamespace and
ABILITY_NAME_RE to valid names. Keep the existing warning behavior for malformed
names and prevent invalid dashboard data from aborting catalog refresh.
- Around line 125-141: Update cacheSignature to include config.skipSslVerify and
config.maxResponseSize in the returned cache signature, ensuring cache entries
and in-flight fetches remain isolated when transport-security limits differ.
Preserve the existing dashboard, namespace, and hashed authentication identity
components.
In `@src/confirmation.ts`:
- Around line 184-204: Update the explicit dry-run branch in the confirmation
flow to handle args containing both confirm: true and dry_run: true before
returning the skip action. Reject this ambiguous combination or remove confirm
from the forwarded arguments, while preserving the existing undeclared-dry_run
validation and dry-run bypass behavior.
In `@src/index.ts`:
- Around line 268-276: Extend the resource access checks in the handler
containing the existing getSiteToolName gate so every tool-derived
resource—mainwp://abilities, mainwp://help, and
mainwp://help/tool/{tool_name}—is filtered through isToolAllowed. Derive each
path’s corresponding tool name with the existing abilityNameToToolName
convention, and deny access using the same McpErrorFactory.permissionDenied
behavior when blocked.
- Around line 591-594: Update the entrypoint guard in src/index.ts around the
top-level main invocation to resolve symlinks before comparing the module URL
with process.argv[1], or use import.meta.main if supported by the project’s Node
range. Ensure invoking the installed mainwp-mcp bin still executes main(), while
preserving the behavior that imports used by tests do not start the server.
In `@src/tool-schema.ts`:
- Around line 296-303: Update the destructive-tool confirmation workflow in the
isDestructive && hasConfirm block to include confirmation_token in the injected
schema properties. Require the execution call to reuse the token returned by the
preview response, and ensure schemas with additionalProperties disabled accept
it. Revise the advertised steps so the preview behavior accurately reflects
whether hasDryRun is enabled, including the no-dry_run path.
In `@tests/acceptance/agent-run.ts`:
- Around line 844-879: Update runClaude to enforce a bounded deadline for the
spawned child process, covering startup, transport, and shutdown hangs. On
timeout, terminate the child, then await its close event before resolving with
the appropriate failure exit code; ensure the timer is cleared when the process
closes normally or errors.
- Around line 234-241: Update the correctMcpResult pass condition to require
proof that the site is absent: accept mainwp_site_not_found, or require a
complete inventory whose results demonstrably exclude the probe site. Do not
allow merely having lookupUses and lookupResults to pass; preserve the existing
evidence fields and use the available truth symbols to validate completeness and
exclusion.
- Around line 1032-1039: Update the environment construction in the agent-run
acceptance setup so MAINWP_SKIP_SSL_VERIFY defaults to 'false' for live targets,
and is enabled only through an explicit self-signed-testbed opt-in
configuration. Preserve the existing fixture HTTP behavior and ensure live
credentials and requests use TLS verification unless that opt-in is provided.
In `@tests/acceptance/lib/agent-matchers.ts`:
- Around line 165-168: Update the blockedCall matching logic in correctMcpResult
to require that the associated result is marked as an error in addition to
containing SAFE_MODE_BLOCKED. Preserve the existing tool-use lookup and content
check, but reject successful payloads lacking the structured error
classification.
In `@tests/acceptance/lib/artifacts.ts`:
- Around line 95-98: Update appendServerStderr and its server-close flow so
stderr redaction operates across arbitrary chunks rather than independently per
value. Maintain a per-server overlap buffer at least as long as the longest
redaction replacement, redact only safe completed content on each append, and
flush the remaining buffered content when the server closes before writing the
final artifact.
In `@tests/acceptance/lib/client.ts`:
- Around line 52-62: Update parseToolJson so JSON parse failures do not include
the raw text from the tool response in the thrown error. In the catch block,
retain the original error as the cause and report only safe metadata, such as
text.length, while preserving the existing missing-text error behavior.
In `@tests/acceptance/lib/commands.ts`:
- Around line 38-58: The run method must stop waiting indefinitely for stalled
child processes. Add timeout or AbortSignal support to the options, terminate
the spawned child when triggered, and resolve with a recorded timed-out
CommandResult while preserving normal stdout, stderr, exit-code, and error
handling.
In `@tests/acceptance/lib/guards.ts`:
- Around line 8-15: Update isWriteHostAllowed to remove the
host.endsWith('.local') implicit allowlist entry. Preserve localhost and
127.0.0.1 loopback access, and require every other hostname—including .local
hosts—to match an entry in additionalHosts case-insensitively.
In `@tests/acceptance/lib/server.ts`:
- Around line 106-119: Make the connection and shutdown cleanup in the server
acceptance flow exception-safe: ensure transport and rawClient are closed
regardless of whether connect or close rejects, and move command recording plus
isolationRoot removal into finally blocks. Update the logic around
rawClient.connect and rawClient.close so cleanup is retried or still performed
even when an earlier cleanup operation fails, without letting the closed guard
suppress required cleanup.
In `@tests/acceptance/lib/verify.ts`:
- Around line 190-203: Update requestJsonResponse to create a bounded timeout
AbortSignal and pass it to request alongside the existing method, headers, body,
and dispatcher options. Ensure the timeout is cleaned up appropriately so
verifier requests cannot wait indefinitely or outlive the scenario budget.
In `@tests/acceptance/run.ts`:
- Around line 119-125: Update the environment object returned by the acceptance
test configuration to stop setting MAINWP_SKIP_SSL_VERIFY automatically for live
targets. Keep TLS verification enabled by default, and only add that variable
when an explicit environment opt-in flag requests self-signed dashboard support;
preserve the fixture-specific MAINWP_ALLOW_HTTP behavior.
- Line 399: Update the acceptance runner’s exit-status logic in the return
expression using runResults.totals so unverified scenarios produce a failing
exit code, alongside failed assertions. Preserve the successful exit code only
when both failed and unverified totals are zero.
In `@tests/acceptance/scenarios/ability-reads.ts`:
- Around line 116-187: Introduce one shared bounded-pagination policy for
verifierListAll, mcpListAll, verifierListUpdates, and mcpListUpdates in
tests/acceptance/scenarios/ability-reads.ts:116-187, including a maximum page
limit and detection of repeated or non-progressing pages before continuing.
Apply the identical guard to direct site pagination in
tests/acceptance/lib/verify.ts:151-163 and MCP site pagination in
tests/acceptance/scenarios/types.ts:129-140; preserve normal completion and MCP
error handling while preventing reliance on mutable response totals.
In `@tests/acceptance/scenarios/configuration.ts`:
- Around line 43-46: Update the packaging-check assertions in the scenario
around ctx.packedPackage.checks to validate an independently defined
required-key set covering tarball contents, CLI bin, entry point, and installed
version, rather than iterating only producer-supplied keys. Assert that all
required keys are present and true while preserving the existing
unavailable-metadata error.
In `@tests/acceptance/scenarios/types.ts`:
- Around line 142-171: Update tests/acceptance/scenarios/types.ts lines 142-171
so findSiteWithPlugins and findHelloDolly consider only connected, reachable
sites, continue when an individual getSitePlugins inventory read fails, and
return a valid candidate when one exists. Update
tests/acceptance/scenarios/writes.ts lines 72-80 to select a connected site
rather than blindly using the first listed site.
In `@tests/acceptance/scenarios/writes.ts`:
- Around line 26-44: Update the non-confirmation path in the write flow around
ctx.client.callTool to treat any CONFIRMATION_REQUIRED response without a
confirmation_token as an error. Throw a descriptive error instead of returning
successfully, while preserving the existing confirmed retry when a token is
present and the current handling for other statuses.
---
Minor comments:
In `@docs/security.md`:
- Line 120: Update the unlabeled fenced example in docs/security.md to specify
the text language by adding the text fence annotation, preserving the example
content and closing fence.
In `@docs/troubleshooting.md`:
- Around line 141-155: Update the destructive-operation troubleshooting guidance
to remove stale or expired confirmation tokens from the `CONFIRMATION_REQUIRED`
cases and document them under the `PREVIEW_EXPIRED` section, including the
five-minute expiry behavior. Keep `CONFIRMATION_REQUIRED` focused on missing or
mismatched preview state, such as confirming without a valid preceding preview.
In `@tests/acceptance/agent-run.ts`:
- Around line 779-783: Update the plugin validation branch using
truth.pluginSlug and truth.pluginActive so it parses the response and inspects
matching objects by property values rather than relying on JSON field order.
Preserve the existing slug escaping and active-state comparison, and return true
when any matching plugin object satisfies both conditions.
In `@tests/acceptance/lib/harness.test.ts`:
- Around line 50-63: Strengthen the parseAcceptanceEnv test by snapshotting the
relevant process.env keys before parsing and asserting they are unchanged
afterward. Add a variable value containing a literal reference such as $HOME,
then assert the parsed result preserves that text without expansion while
retaining the existing field expectations.
In `@tests/acceptance/lib/pack.ts`:
- Around line 47-51: Update the setup flow around tempRoot, packResult, registry
startup, and installation to catch setup failures and remove tempRoot before
rethrowing when preservation is not requested. Keep the temporary tree intact
when the existing preservation option is enabled, and leave successful
runAcceptance cleanup unchanged.
In `@tests/acceptance/scenarios/read.ts`:
- Around line 171-183: Update the notFoundInput scenario’s get_site_v1 call to
use the harness’s reserved nonexistent probe hostname, such as
nonexistent-acceptance-probe.invalid, instead of the potentially valid numeric
site_id_or_domain value. Keep the existing structured error assertions and
assertSessionRecovery flow unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3963b04-1b2d-4602-afe8-5adb83241010
⛔ Files ignored due to path filters (2)
docs/images/mainwp-logo-2026.pngis excluded by!**/*.pngdocs/images/mainwp-mcp-logo.pngis excluded by!**/*.png
📒 Files selected for processing (78)
.github/workflows/ci.yml.gitignoreCHANGELOG.mdREADME.mddocs/acceptance-testing.mddocs/configuration.mddocs/security.mddocs/troubleshooting.mdeslint.config.jspackage.jsonscripts/manual-test.tssettings.example.jsonsettings.schema.jsonsrc/abilities.test.tssrc/abilities.tssrc/config.test.tssrc/config.tssrc/confirmation-responses.tssrc/confirmation.tssrc/errors.test.tssrc/errors.tssrc/http-client.tssrc/index.test.tssrc/index.tssrc/logging.test.tssrc/logging.tssrc/prompts.test.tssrc/prompts.tssrc/retry.test.tssrc/retry.tssrc/security.test.tssrc/security.tssrc/session.test.tssrc/session.tssrc/tool-schema.test.tssrc/tool-schema.tssrc/tools.test.tssrc/tools.tstests/acceptance/agent-run.tstests/acceptance/fixture-dashboard.tstests/acceptance/fixtures/sites.jsontests/acceptance/lib/agent-confirmation.tstests/acceptance/lib/agent-matchers.tstests/acceptance/lib/artifacts.tstests/acceptance/lib/client.tstests/acceptance/lib/commands.tstests/acceptance/lib/env.tstests/acceptance/lib/guards.tstests/acceptance/lib/harness.test.tstests/acceptance/lib/local-registry.tstests/acceptance/lib/pack.tstests/acceptance/lib/redact.tstests/acceptance/lib/server.tstests/acceptance/lib/verify.tstests/acceptance/run.tstests/acceptance/scenarios/ability-reads.tstests/acceptance/scenarios/completions.tstests/acceptance/scenarios/configuration.tstests/acceptance/scenarios/confirmation.tstests/acceptance/scenarios/index.tstests/acceptance/scenarios/policy.tstests/acceptance/scenarios/read.tstests/acceptance/scenarios/transport.tstests/acceptance/scenarios/types.tstests/acceptance/scenarios/writes.tstests/evals/description-quality.test.tstests/evals/safety-coverage.test.tstests/evals/schema-quality.test.tstests/evals/token-budget.test.tstests/fixtures/README.mdtests/fixtures/abilities.jsontests/fixtures/categories.jsontests/fixtures/site.jsontests/helpers/config.tstests/integration/abilities.test.tstests/integration/tools.test.tstests/unit/security.test.tstsconfig.eslint.json
💤 Files with no reviewable changes (6)
- tests/fixtures/categories.json
- tests/fixtures/site.json
- tests/integration/abilities.test.ts
- tests/unit/security.test.ts
- tests/fixtures/abilities.json
- tests/integration/tools.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/acceptance/lib/local-registry.ts (1)
74-83: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnhandled stream error on tarball endpoint can crash the test process.
path.basenamedoes not neutralize..or empty strings —path.basename('..')returns'..'andpath.basename('')returns''. A request to/tarballs/..or/tarballs/resolvesfilePathtotarballDiror its parent (a directory), which passes theexistsSynccheck but causescreateReadStreamto emit an unhandled'error'event, crashing the process. Add anisFile()guard and handle stream errors.🛡️ Proposed fix
const filename = path.basename(decodeURIComponent(url.pathname.slice('/tarballs/'.length))); const filePath = path.join(tarballDir, filename); - if (!fs.existsSync(filePath)) return json(response, 404, { error: 'tarball not found' }); - const stat = fs.statSync(filePath); + if (!filename || filename === '.' || filename === '..') { + return json(response, 404, { error: 'tarball not found' }); + } + const stat = fs.statSync(filePath, { throwIfNoEntry: false }); + if (!stat || !stat.isFile()) return json(response, 404, { error: 'tarball not found' }); response.writeHead(200, { 'content-type': 'application/octet-stream', 'content-length': stat.size, }); - fs.createReadStream(filePath).pipe(response); + fs.createReadStream(filePath) + .on('error', () => { + if (!response.headersSent) json(response, 500, { error: 'failed to stream tarball' }); + response.end(); + }) + .pipe(response); return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/local-registry.ts` around lines 74 - 83, Update the tarball-serving handler around filename, filePath, and fs.createReadStream: require the resolved path to be an actual file, returning the existing 404 response for directories, parent paths, and empty filenames. Attach an error handler to the created read stream before piping it to response so stream failures are handled without crashing the test process.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/acceptance/lib/local-registry.ts`:
- Around line 74-83: Update the tarball-serving handler around filename,
filePath, and fs.createReadStream: require the resolved path to be an actual
file, returning the existing 404 response for directories, parent paths, and
empty filenames. Attach an error handler to the created read stream before
piping it to response so stream failures are handled without crashing the test
process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 994c5ca7-2ce3-41bf-9315-046e03008bfa
📒 Files selected for processing (1)
tests/acceptance/lib/local-registry.ts
JSON.stringify's array-replacer form filters property names at every nesting level, so nested argument values were dropped from the preview key and a confirmed call could swap nested values (settings.role, objects inside arrays) past the token binding. Serialize a recursively key-sorted copy instead and add a nested arg-swap regression test.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
531-537: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the documented parameter name in the confirmation diagram.
The tool reference at Lines 648-650 requires
site_id_or_domain, but both diagram calls usesite_id. Agents following this example may send invalid arguments. Update both calls or align the tool reference with the actual schema.Proposed fix
- AI->>Server: delete_site_v1(site_id: 3, confirm: true) + AI->>Server: delete_site_v1(site_id_or_domain: 3, confirm: true) - AI->>Server: delete_site_v1(site_id: 3, user_confirmed: true,<br/>confirmation_token: "...") + AI->>Server: delete_site_v1(site_id_or_domain: 3, user_confirmed: true,<br/>confirmation_token: "...")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 531 - 537, Update both delete_site_v1 calls in the confirmation diagram to use the documented site_id_or_domain parameter instead of site_id, preserving the existing confirmation and preview flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 25: Update the “How It Works” heading in the README from an H3 to an H2,
using the existing heading text and preserving the surrounding document
structure.
In `@src/confirmation.ts`:
- Around line 92-96: The canonicalization object in canonicalize must preserve
an own enumerable __proto__ key instead of invoking the plain-object prototype
setter. Initialize sorted with Object.create(null), and add a confirmation-flow
regression test covering payloads with distinct nested __proto__ values to
ensure their tokens remain different.
---
Outside diff comments:
In `@README.md`:
- Around line 531-537: Update both delete_site_v1 calls in the confirmation
diagram to use the documented site_id_or_domain parameter instead of site_id,
preserving the existing confirmation and preview flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47be33e6-f61d-4f0c-8fba-5badf7a7da00
📒 Files selected for processing (3)
README.mdsrc/confirmation.tssrc/tools.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tools.test.ts
…vels Plain-object canonicalization dropped an own __proto__ key through the Object.prototype setter, collapsing differing nested payloads onto one preview key. Build sorted copies with Object.create(null) and pin it with a __proto__ arg-swap regression. Promote the two intro README sections to H2 (MD001).
- Entry-point check realpaths argv[1] so npm bin symlinks start the
server (npx exited silently before; verified via symlink invocation)
- Tool schemas declare confirmation_token and the advertised flow names
the token step; confirm-only abilities stop promising a preview
- Malformed property values inside ability schemas coerce to {} instead
of crashing the whole tools/list response
- Malformed ability entries (null, non-string name) skip with a warning
instead of poisoning the catalog refresh
- Cache signature includes skipSslVerify and maxResponseSize
- Explicit dry_run strips confirm before the upstream call, matching
the preview path
Each fix carries a regression test verified failing against the old
code.
Timeouts and SIGKILL for spawned commands and the agent CLI, TLS verification on by default with an explicit MAINWP_MCP_ACCEPTANCE_ SKIP_SSL_VERIFY opt-in, write-guard drops the blanket .local allowance in favor of loopback plus the env-file-resolved testbed host, bounded pagination everywhere, exception-safe server/pack cleanup, stream-aware stderr redaction, unverified scenarios fail the run, packed-integrity now proves the installed bin symlink starts the server, and the local-registry tarball endpoint rejects traversal and stream errors. Agent graders reworked while validating live: the not-found proof accepts the error code embedded in sanitized messages and an empty dashboard-side scoped search, and the plugin probe skips mainwp-child (its activity is implied by connection, dodging the plugin-list tool the scenario exercises). Fixed two spawn-error hangs in the new timeout code. Full sweep at final content: fixture 21, live+writes 23, agent 9/9, 552 unit tests.
…cisions Preview keys and confirmation tokens now carry a hash of the config identity (same definition as the abilities cache signature), so a token issued against one dashboard/principal can no longer confirm the same tool and arguments on another createServer(config) instance sharing the module-level preview maps. Regression test verified failing against the unscoped code. Add a root .coderabbit.yaml carrying the standing review pushbacks with inline rationale, so the cloud reviewer respects documented design decisions (resource catalog disclosure, PREVIEW_REQUIRED token cases, skipSslVerify scope, session counter, timeout classification) instead of re-flagging them on every push.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/acceptance/lib/commands.ts (1)
45-70: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftTerminate the whole subprocess tree on timeout.
child.kill('SIGKILL')only stops the direct child; descendants can keep the pipes open and blockclose, which defeats this deadline. Kill the process tree and add a bounded fallback if teardown stalls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/commands.ts` around lines 45 - 70, Update the timeout handler in the command-spawning flow around child and timeout to terminate the entire subprocess tree, not just the direct child. Add a bounded fallback so the promise resolves with the timeout result even if tree teardown or the close event stalls, while preserving the existing 124 timeout code and cleanup behavior.tests/acceptance/lib/local-registry.ts (1)
107-109: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCatch malformed package-name encodings here too
decodeURIComponent()throws on invalid paths like/%E0%A4%A, which can bubble out of the request handler and take down the registry. Return a 404/400 here, as the tarball route already does.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/local-registry.ts` around lines 107 - 109, Guard the decodeURIComponent call in the package lookup flow with the same malformed-encoding handling used by the tarball route, returning a 404 or 400 response instead of allowing the exception to escape. Preserve the existing byName lookup and “package not found” response for successfully decoded names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tool-schema.ts`:
- Around line 47-52: Update the property-map construction in the shown loop to
safely preserve an own property named __proto__ without invoking the legacy
prototype setter, using safe own-property definitions or a null-prototype map.
Apply the same protection to compressSchema’s compressedProperties map, and
ensure the later confirm/dry_run checks only observe intended own properties
rather than inherited values.
In `@tests/acceptance/lib/agent-matchers.ts`:
- Around line 259-262: Update the safe-mode fixture in harness.test.ts so the
relevant result sets isError to true, matching the isError check in
targetDeleteUses and preserving correctMcpResult.pass as true.
- Around line 102-108: Update the search matching logic in the visible matcher
callback to normalize both the probe and search term with hostnameOf before
checking containment. Apply the existing lowercase/trim handling to the
normalized hostname values, while preserving the minimum-length and uses.some
filtering behavior.
---
Outside diff comments:
In `@tests/acceptance/lib/commands.ts`:
- Around line 45-70: Update the timeout handler in the command-spawning flow
around child and timeout to terminate the entire subprocess tree, not just the
direct child. Add a bounded fallback so the promise resolves with the timeout
result even if tree teardown or the close event stalls, while preserving the
existing 124 timeout code and cleanup behavior.
In `@tests/acceptance/lib/local-registry.ts`:
- Around line 107-109: Guard the decodeURIComponent call in the package lookup
flow with the same malformed-encoding handling used by the tarball route,
returning a 404 or 400 response instead of allowing the exception to escape.
Preserve the existing byName lookup and “package not found” response for
successfully decoded names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81ccf08a-a09e-4346-939e-281f28bb541e
📒 Files selected for processing (34)
.coderabbit.yaml.github/workflows/ci.ymlCHANGELOG.mdREADME.mddocs/acceptance-testing.mddocs/security.mddocs/troubleshooting.mdsrc/abilities.test.tssrc/abilities.tssrc/confirmation.tssrc/index.tssrc/tool-schema.test.tssrc/tool-schema.tssrc/tools.test.tstests/acceptance/agent-run.tstests/acceptance/lib/agent-matchers.tstests/acceptance/lib/artifacts.tstests/acceptance/lib/client.tstests/acceptance/lib/commands.tstests/acceptance/lib/env.tstests/acceptance/lib/guards.tstests/acceptance/lib/harness.test.tstests/acceptance/lib/local-registry.tstests/acceptance/lib/pack.tstests/acceptance/lib/pagination.tstests/acceptance/lib/redact.tstests/acceptance/lib/server.tstests/acceptance/lib/verify.tstests/acceptance/run.tstests/acceptance/scenarios/ability-reads.tstests/acceptance/scenarios/configuration.tstests/acceptance/scenarios/read.tstests/acceptance/scenarios/types.tstests/acceptance/scenarios/writes.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- .github/workflows/ci.yml
- docs/acceptance-testing.md
- tests/acceptance/scenarios/configuration.ts
- tests/acceptance/lib/redact.ts
- tests/acceptance/lib/env.ts
- tests/acceptance/lib/client.ts
- tests/acceptance/run.ts
- tests/acceptance/agent-run.ts
- tests/acceptance/scenarios/read.ts
- tests/acceptance/lib/pack.ts
- docs/security.md
- tests/acceptance/lib/harness.test.ts
- tests/acceptance/lib/artifacts.ts
- tests/acceptance/lib/verify.ts
- tests/acceptance/scenarios/writes.ts
- src/confirmation.ts
- tests/acceptance/lib/server.ts
- src/index.ts
- tests/acceptance/scenarios/types.ts
- tests/acceptance/scenarios/ability-reads.ts
- README.md
- src/tools.test.ts
- src/abilities.ts
- src/abilities.test.ts
- Tool-schema property maps (convertInputSchema and compressSchema) use null prototypes so a hostile __proto__ parameter stays an own schema property instead of polluting the map and leaking inherited keys into the confirm/dry_run detection; regression pinned red against the plain-object maps - Scoped-search absence proof normalizes both sides through hostnameOf so a scheme fragment like "https" can never correlate with a URL-shaped probe - Command and agent-CLI spawns run detached and a timeout kills the whole process group, with a bounded post-exit fallback so a surviving orphan holding the pipes cannot defeat the deadline - Local-registry package route guards decodeURIComponent like the tarball route (400 instead of crashing the registry) Not changed: the safe-mode fixture CodeRabbit flagged at harness.test.ts is the intentional negative case asserting pass === false for a non-error SAFE_MODE_BLOCKED payload.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/acceptance/lib/commands.ts (1)
55-131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winArm the timeout fallback in the timer callback
exitcan fire before the deadline while descendant processes still hold the pipes open, so the 2s fallback never gets scheduled andclosecan hang forever. Start an idempotent fallback from the timeout callback itself.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/commands.ts` around lines 55 - 131, Update the timeout handling in CommandRunner.run so the 2-second resolve(124) fallback is armed directly from the timeout callback, not only from the child exit handler. Make the fallback idempotent to avoid duplicate scheduling when exit occurs after timeout, while preserving normal close resolution and timeout result behavior.Source: MCP tools
🧹 Nitpick comments (1)
tests/acceptance/lib/commands.ts (1)
46-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated command-runner implementation.
tests/acceptance/agent-run.tsstill defines equivalentkillProcessTreeandCommandRunnerimplementations, so timeout and recording fixes can diverge. Import the exports fromtests/acceptance/lib/commands.tsor remove the duplicate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/acceptance/lib/commands.ts` around lines 46 - 54, Remove the duplicate killProcessTree and CommandRunner implementations from agent-run.ts, and import and reuse the corresponding exports from the commands module. Ensure timeout handling and command recording continue through the shared implementations defined by killProcessTree and CommandRunner.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/acceptance/lib/commands.ts`:
- Around line 55-131: Update the timeout handling in CommandRunner.run so the
2-second resolve(124) fallback is armed directly from the timeout callback, not
only from the child exit handler. Make the fallback idempotent to avoid
duplicate scheduling when exit occurs after timeout, while preserving normal
close resolution and timeout result behavior.
---
Nitpick comments:
In `@tests/acceptance/lib/commands.ts`:
- Around line 46-54: Remove the duplicate killProcessTree and CommandRunner
implementations from agent-run.ts, and import and reuse the corresponding
exports from the commands module. Ensure timeout handling and command recording
continue through the shared implementations defined by killProcessTree and
CommandRunner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9232961d-94dc-4ce2-ba07-56f895b8bdb5
📒 Files selected for processing (8)
CHANGELOG.mdsrc/tool-schema.test.tssrc/tool-schema.tstests/acceptance/agent-run.tstests/acceptance/lib/agent-matchers.tstests/acceptance/lib/commands.tstests/acceptance/lib/harness.test.tstests/acceptance/lib/local-registry.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/tool-schema.ts
- tests/acceptance/lib/harness.test.ts
- src/tool-schema.test.ts
- tests/acceptance/agent-run.ts
- CHANGELOG.md
- tests/acceptance/lib/local-registry.ts
- tests/acceptance/lib/agent-matchers.ts
The 2-second resolve fallback was gated on 'exit' having fired after the deadline; a child that exits before the deadline while an escaped descendant holds the pipes would never schedule it, so 'close' could hang forever. awaitChildWithDeadline in commands.ts now arms the idempotent fallback inside the timeout callback itself, and both call sites (CommandRunner.run and the agent CLI spawn) share it instead of duplicating the deadline logic. Unit test pins a child whose close never fires resolving 124 through the fallback.
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.
Release prep for the next beta. For anyone running the server, the changes fall into three groups: the destructive-operation flow now enforces what it always claimed to, several classes of wrong or confusing behavior are fixed, and three changes are breaking.
The confirmation flow now enforces the token
Destructive tools (delete site, deactivate plugins, and the rest) document a two-step flow: request a preview, then confirm with the token the preview issued. Two gaps let clients skip it:
user_confirmed: truewithout a token executed the operation whenever a matching preview was pending, so a client could confirm a preview it never read. Execution now always requires theconfirmation_token.dry_run: trueon an ability that does not declare adry_runparameter skipped the confirmation flow and passed the flag upstream, where a handler ignoring unknown input would have run the real operation. It now returns an invalid-parameter error.Related tightening:
allowedToolsandblockedToolsare now enforced everywhere an ability can be resolved or executed, including resources and prompt completions, with the blocklist taking precedence. Previously only direct tool calls were checked.Fixes you may have hit
properties: [], PHP's empty-array JSON encoding) no longer breaks the whole tool list. Spec-compliant clients used to reject the entiretools/listresponse, so the server connected but showed zero tools.tool_list_changednotification after every cache refresh when nothing changed.[object Object], which the dashboard misread.Breaking changes
PREVIEW_REQUIRED: run the preview, then confirm with the issued token.true/1/yes/onandfalse/0/no/off) stops the server at startup with the variable named, instead of warning and falling back to the default.mcp; that name collides with other MCP tooling. The command ismainwp-mcp, andnpx @mainwp/mcpworks as before.The CHANGELOG
[Unreleased]section covers all of this from the user's side.Testing
The branch adds an acceptance suite that packs the tarball, installs it into a clean consumer, and drives the installed binary through a real MCP client, cross-checked against independent Abilities API reads; it runs credential-free in CI and against a live dashboard before releases. Verified for this PR: 532 unit tests, lint, typecheck, and build green; full acceptance sweep green against a live dashboard (fixture 21 pass, live with writes 23 pass, agent layer 9/9); the gating sequence exercised end to end on a throwaway target, including forged-token and replay rejection.
Reviewers: the enforcement changes live in
src/confirmation.tsandsrc/tools.ts; error classification order insrc/errors.tsis significant and commented.Summary by CodeRabbit
mainwp-mcp.