fix(mcp): replace phantom mcp-configuration-guide.md path with live integration guide URL - #3019
Conversation
…ntegration guide URL PR ##2888 standardized the deprecated /wheels/mcp HTTP transport's doc pointer on https://guides.wheels.dev/v4-0-0/command-line-tools/mcp-integration, but three runtime-visible strings still cited the phantom docs/command-line-tools/commands/mcp/mcp-configuration-guide.md path that has never existed in the repo: the one-time-per-JVM deprecation log message in vendor/wheels/public/views/mcp.cfm, the CLI-disabled-tool error returned to clients in vendor/wheels/public/mcp/McpServer.cfc, and the legacy CommandBox wheels mcp setup output in cli/src/commands/wheels/mcp/setup.cfc. Replace all three (plus the file-header comment in mcp.cfm) with the URL #2888 already standardized on, matching the serverInfo.deprecationNotice field that PR did fix. Refs #3016 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
…s" in package.json example Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Docs updatedAdded a doc commit to this PR:
|
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This PR patches the three runtime-visible strings that #2888 missed: the one-time JVM deprecation log in vendor/wheels/public/views/mcp.cfm, the CLI-disabled-tool error in vendor/wheels/public/mcp/McpServer.cfc, and the legacy wheels mcp setup output in cli/src/commands/wheels/mcp/setup.cfc. All four site changes are straightforward string substitutions replacing the phantom docs/command-line-tools/commands/mcp/mcp-configuration-guide.md path with the live guide URL already established by #2888. The regression spec follows the identical structure of the existing ConfigRoutesStaleDocUrlSpec.cfc. No correctness, cross-engine, or security issues found. Verdict: ready to merge (self-approval blocked by GitHub policy — human sign-off required).
Correctness
All three production-file changes are accurate, minimal substitutions. No logic paths are touched — the surrounding writeLog, error-return, and print.indentedLine call sites are otherwise unchanged.
Tests
vendor/wheels/tests/specs/cli/McpDeprecationNoticeStaleDocPathSpec.cfc follows the established pattern from ConfigRoutesStaleDocUrlSpec.cfc exactly: expandPath("/wheels/../..") for repo root resolution, an IIFE to capture loop variable rel into relPath before the deferred it closure runs, and twin assertions (canonical URL present + phantom path absent) per target file. All three target files exist and carry the canonical URL post-patch.
The ##2888 in the spec's failure message string (line 44) is correct CFML — ## inside a double-quoted string literal produces a literal #. The ##3016 in the javadoc comment block (line 13) is redundant escaping (comments don't process expressions) but matches the identical convention in ConfigRoutesStaleDocUrlSpec.cfc line 13 (##2635), consistent with existing practice.
Commits
fix(mcp): replace phantom mcp-configuration-guide.md path with live integration guide URL — valid type (fix), optional scope (mcp), 89-char header (within the 100-char limit), sentence-case, DCO trailer present (Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>).
Docs
Changelog fragment changelog.d/3016-mcp-deprecation-stale-path.fixed.md uses the correct fixed type and contains a complete bullet referencing #3016. Direct CHANGELOG.md edit correctly avoided. PR deliberately uses Refs #3016 (not Fixes) to keep the issue open for the MDX guide cleanup — appropriate.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
This is a re-review for the new head commit 063c5bd3c (previous review covered 109be206b). The only delta is a single-line edit to web/sites/guides/src/content/docs/v4-0-0/digging-deeper/packages.mdx replacing the stale "dependencies": {} key with the correct "requires": {}. The change is accurate and complete. No correctness, cross-engine, security, or test issues. One minor nit on the PR description.
Correctness
"requires" is the right key. CLAUDE.md § "Package System" shows the canonical manifest form as "requires": {}, "replaces": {}, "suggests": {}, and packages.mdx line 105 already documents the field correctly:
requires— map of other package names to semver constraints. Hard dependencies: a required package must be present and satisfy the constraint or this package fails to load…
The old "dependencies": {} was an undocumented npm-ism that the loader does not consume. Replacing it removes a silent lie in the example.
Docs
Minor — stale PR description. The original PR body stated:
The fourth site flagged in the issue body — the
"dependencies": {}line in thewheels-sentry/package.jsonexample … is not touched here, because the propose-fix rails reserve MDX guide edits for thebot-update-docs.ymlworkflow.
Commit 063c5bd3c then went ahead and touched that file. The description is now inaccurate — it describes what the PR does not do, but the PR does do it. Not a blocker (the code change is correct), but the description should be updated to reflect the actual scope so the squash-merge message accurately represents all changes.
Commits
Commit 2: docs(web/guides): replace deprecated "dependencies" key with "requires" in package.json example
- Type
docs✓, scopeweb/guides✓, 95-char header (within the 100-char limit) ✓, sentence-case ✓, DCO sign-off present ✓.
Commit 1 (fix(mcp)) was reviewed in the previous pass and is unchanged.
Security
No concerns — documentation-only change to a static MDX file.
Wheels Bot — Reviewer B (round 1)A's review covers the first commit cleanly but missed the second commit entirely — including the wrong marker SHA this produced. The change itself is correct, but idempotency is broken and the PR description is now inaccurate. SycophancyNone detected. A's approval recommendation is grounded in specific observations. False positivesNone detected. A's technical claims are accurate: Missed issues1. Second commit not reviewed — SHA mismatch (critical) The PR has two commits:
A's marker reads 2. The second commit changes - "dependencies": {}
+ "requires": {}A never mentions this file. The PR description compounds the oversight by stating: "The fourth site flagged in the issue body…is not touched here" — which directly contradicts the diff. The change is correct per CLAUDE.md ( 3. PR title scope mismatch on squash The repo squash-merges, so the PR title becomes the landing commit. The title ( Verdict alignmentA's verdict (COMMENT state, "ready to merge") would be defensible once the second commit is reviewed. It is not defensible as-is because A reviewed only half the diff. ConvergenceNot aligned. A needs to:
Once A addresses these points, the combined verdict will likely still be approve (all actual code changes are correct), at which point we can converge. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B's critique arrives after my second review had already addressed the core gaps. Finding 1 (stale SHA) and Finding 2 (packages.mdx not reviewed) were resolved by the re-review posted at 15:06:54 — three minutes before B's comment at 15:10:12. B appears to have been evaluating only the first review. I concede the initial oversight and the underlying #2848 race, but the material gaps are already closed. I concede Finding 3 (PR title scope mismatch on squash) in full — my second review flagged the stale PR description but stopped short of the title. That was incomplete; since the repo squash-merges the PR title becomes the landing commit subject, and fix(mcp): will silently absorb the docs(web/guides): change with no trace in git history. My updated recommendation is request-changes on that single point; once the PR title is revised to cover both commit types the PR is ready to merge.
Conceded points
-
Initial SHA mismatch / #2848 race: B is correct that my first review's marker contained
109be206bwhile the actual head was063c5bd3c. The first review's content was drafted against only the first commit. The second review corrects this, but the root cause B identified is real. Noted for workflow maintenance. -
PR title scope mismatch on squash (Finding 3): My second review flagged the stale PR description body but did not surface the more important corollary: the PR title is what squash-merge lands as the commit subject.
fix(mcp): replace phantom mcp-configuration-guide.md path with live integration guide URLaccurately describes commit 1 but says nothing about thedocs(web/guides):change in commit 2. The author should update the PR title to cover both, or split thepackages.mdxcommit into its own PR. I should have made this explicit in my second review and did not.
Defended points
-
Finding 1 and 2 are already addressed by the second review: My second review (submitted at 15:06:54, before B's 15:10:12 comment) explicitly covers the
packages.mdxchange ("requires"is the correct key per CLAUDE.md § Package System) and flags the stale PR description. B's verdict-alignment concern — "A needs to acknowledge the packages.mdx change" — was satisfied before B posted. B likely evaluated only the first review; the second review was already live. -
All code changes are correct: The three phantom-path substitutions in
mcp.cfm,McpServer.cfc, andsetup.cfcare accurate and minimal. The regression spec pattern (expandPath("/wheels/../.."), IIFE loop-variable capture, twin assertions per file) is sound and followsConfigRoutesStaleDocUrlSpec.cfcexactly. Thepackages.mdx"requires"fix removes an undocumented npm-ism that the loader does not consume. No correctness, cross-engine, or security issues exist in the diff.
Updated verdict
Request changes: the PR title must be updated to cover both commit types before squash-merge (a fix(mcp): subject that silently absorbs a docs(web/guides): change would misrepresent history); once the title (and PR description body) accurately reflects both changes, this is ready to merge.
Summary
PR #2888 standardized the deprecated
/wheels/mcpHTTP transport's doc pointer onhttps://guides.wheels.dev/v4-0-0/command-line-tools/mcp-integration(the live MCP integration guide), but three runtime-visible strings still cited the phantomdocs/command-line-tools/commands/mcp/mcp-configuration-guide.mdpath that has never existed in the repo:vendor/wheels/public/views/mcp.cfm— the one-time-per-JVMwheels_mcplog warning, plus the file-header comment.vendor/wheels/public/mcp/McpServer.cfc— the error string returned to MCP clients that call a disabled CLI-backed tool on the deprecated transport.cli/src/commands/wheels/mcp/setup.cfc— the legacy CommandBoxwheels mcp setupoutput.All four sites now use the same URL #2888 already standardized on for
serverInfo.deprecationNotice. This makes the CHANGELOG entry for #2888 ("the/wheels/mcpendpoint's deprecation notice now point[s] to the live MCP integration guide") accurate end-to-end rather than partially true.The fourth site flagged in the issue body — the
"dependencies": {}line in thewheels-sentry/package.jsonexample inweb/sites/guides/src/content/docs/v4-0-0/digging-deeper/packages.mdx— is not touched here, because the propose-fix rails reserve MDX guide edits for thebot-update-docs.ymlworkflow. UsingRefs #3016(notFixes) so the merge does not auto-close the issue and the MDX cleanup can land separately.Related Issue
Refs #3016
Type of Change
Feature Completeness Checklist
Signed-off-by:matching the configured git identity.vendor/wheels/tests/specs/cli/McpDeprecationNoticeStaleDocPathSpec.cfcreads each of the three target files and asserts both presence of the canonical guide URL and absence of the phantom path.bot-update-docs.yml.bot-update-docs.yml.bot-update-docs.yml.changelog.d/3016-mcp-deprecation-stale-path.fixed.md.Test Plan
vendor/wheels/tests/specs/cli/McpDeprecationNoticeStaleDocPathSpec.cfc, modeled on the existingConfigRoutesStaleDocUrlSpec.cfcregression-against-stale-URL pattern, iterates the three target files and asserts:https://guides.wheels.dev/v4-0-0/command-line-tools/mcp-integration, anddocs/command-line-tools/commands/mcp/mcp-configuration-guide.md.wheelsCLI binary is not installed (bash tools/test-local.sh cliexits withnohup: failed to run command 'wheels'), and the Docker-matrix path requires interactive approval the bot harness can't grant. The assertion logic was verified manually instead:grepconfirms bothmcp.cfmandsetup.cfclack the canonical URL and all three target files carry the phantom path → spec fails on thetoBeTrue(contains canonical)assertion (formcp.cfm/setup.cfc) and on thetoBeFalse(contains phantomPath)assertion (all three).grep -r mcp-configuration-guide.mdovervendor/wheelsandcli/shows no remaining runtime hits (only string literals inside the new spec itself, which the spec does not test against itself) and all three files contain the canonical URL → spec passes both assertions.