docs(web/guides): clarify legacy adapter does not shim application.wirebox rename#2636
Conversation
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Pure documentation change — adds an <Aside type="caution"> to upgrade-guide item 10, a scope table to the Legacy Compatibility Adapter section, and a Common Issues bullet for the wirebox boot failure. Applied consistently to both v4-0-0 and v4-0-1-snapshot. No correctness, cross-engine, or security issues. Two minor clarity nits noted below; neither blocks merge.
Docs
Nit 1 — "before the upgrade" wording may mislead post-upgrade readers
File: web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx (line 323; identical in v4-0-1-snapshot)
Current:
must update those references before the upgrade — the adapter cannot soften this item.
The Legacy Compatibility Adapter section is also read by developers who have already shipped the upgrade with the adapter installed and are now working through the remaining manual items. "Before the upgrade" implies they missed their window. A wording like the following would serve both audiences:
must update those references manually — the adapter cannot defer or soften this item.
Nit 2 — "See item 10" in Common Issues is plain text, not a link
File: web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx (line 370; identical in v4-0-1-snapshot)
Current:
... Replace any `application.wirebox` references with `application.wheelsdi`. See item 10.
All other Common Issues bullets use #nnnn issue references for navigation. Since this one points to a section on the same page, an anchor link would be more useful for readers who land directly on the Common Issues section:
See [item 10](#10-applicationwirebox-renamed-to-applicationwheelsdi).
Verify the Starlight-generated anchor slug before landing — it may differ slightly depending on the MDX heading renderer.
Commits
Single commit 0d27237 — type docs, scope web/guides, subject well under 100 chars, not ALL-CAPS. DCO Signed-off-by trailer present. Clean.
Tests
No specs expected or needed — pure prose and Markdown table addition.
No blocking issues found. The core information (adapter does not shim the wirebox rename, users must fix both application.wirebox references and the new wirebox.system.ioc.Injector bootstrap call regardless of adapter installation) is accurate, prominently placed, and consistent across both versioned guides. CHANGELOG [Unreleased] entry is present and correctly scoped to ### Documentation.
Wheels Bot — Reviewer B (round 1)Critiquing review 4277718938 by wheels-bot[bot] on commit Overall assessmentReviewer A's classification (pure documentation change, no code paths touched, no specs needed) is correct. The two nits are valid and well-located. No false positives found. Sycophancy checkReview state is False positive checkNit 1 line reference (line 323): Confirmed correct in both Nit 2 line reference (line 370): Confirmed correct. "See item 10." appears at line 370 in both files. Anchor slug suggestion: Reviewer A suggested Missed issuesOne gap worth noting: the scope table added in this PR lists six items as "Covered by the adapter":
The No other missed issues. The CHANGELOG entry is under ConvergenceAligned with Reviewer A. The two nits ("before the upgrade" wording and the plain-text "See item 10") are both actionable and neither is a blocker. The unverifiable scope table is the only additional concern, and it is minor given the first-party provenance of the adapter package. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B's critique aligns with my initial read on all points. No false positives were identified in either nit, and both line references were confirmed accurate. The one gap B added — the unverifiable adapter scope table — is valid: since wheels-legacy-adapter lives in a standalone external repo and is absent from vendor/, the six "Covered by the adapter" claims in the new table rest on authorial assertion rather than source-verifiable fact. I'm conceding this as a worthwhile note to the PR author. My overall position is unchanged: docs-only PR, no blocking issues, three nits (two original plus the adapter table note).
Conceded points
- The scope table's six "Covered by the adapter" claims cannot be verified against adapter source code from this repo. B is correct that the PR author should confirm the table reflects the current release of
wheels-dev/wheels-legacy-adapterbefore merge. I accepted the table at face value without flagging the external provenance gap.
Defended points
- Nit 1 ("before the upgrade" wording, line 323): B confirmed the line reference and the concern. Wording misleads post-upgrade readers who have already shipped and are working through remaining manual items. Stands.
- Nit 2 ("See item 10" plain text, line 370): B confirmed the line reference and the concern. An anchor link would serve readers who land directly on the Common Issues section. Anchor slug caveat was appropriate given Starlight's MDX rendering. Stands.
- COMMENT state: B agreed this posture is correct for a docs-only PR with nits only — not sycophantic, not a false approval. No change.
Updated verdict
Still COMMENT — no blocking issues. The PR may merge once the author addresses (or consciously declines) the three nits: the "before the upgrade" wording, the plain-text "See item 10" cross-reference, and a confirmation that the adapter scope table reflects the actual current release of wheels-dev/wheels-legacy-adapter.
|
@claude can you fix the failing ci steps |
…rebox rename Add an Aside callout to upgrade-guide item 10 stating that wheels-legacy-adapter does not create an application.wirebox alias and does not restore the wirebox.system.ioc.Injector package path. Add a scope table to the Legacy Compatibility Adapter section listing what the adapter covers versus what requires manual remediation. Add a Common Issues entry for the wirebox package-not-found boot failure. Applied to both v4-0-0 and v4-0-1-snapshot guides. Fixes #2627 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
0d27237 to
d430ea3
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is the follow-up commit addressing both nits from the prior round (SHA 0d27237). Nit 1 ("before the upgrade" wording) and Nit 2 ("See item 10" plain text) have both been resolved cleanly. The anchor slug #10-applicationwirebox-renamed-to-applicationwheelsdi matches the actual heading at line 155. One residual informational note from the prior round remains: the scope table's "Covered by the adapter" rows cannot be verified against wheels-dev/wheels-legacy-adapter source from this repo. Not a blocker, but worth a confirmation from the PR author before merge. No correctness, cross-engine, or security issues. COMMENT verdict.
Docs
Prior Nit 1 — resolved. The wording at web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx line 323 (identical in v4-0-1-snapshot) now reads:
must update those references manually — the adapter cannot defer or soften this item.
The "before the upgrade" phrasing that misleads post-upgrade readers is gone. Clean.
Prior Nit 2 — resolved. The Common Issues bullet at line 370 (identical in v4-0-1-snapshot) is now:
See [item 10](#10-applicationwirebox-renamed-to-applicationwheelsdi).
The target heading at line 155 is:
### 10. `application.wirebox` renamed to `application.wheelsdi`
Starlight/remark-slug strips backticks, lowercases, and replaces spaces with hyphens, yielding #10-applicationwirebox-renamed-to-applicationwheelsdi. The anchor is correct.
Residual note — scope table not source-verifiable (carried from Reviewer B, prior round)
File: web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx lines 310–321 (identical in v4-0-1-snapshot)
The scope table's six "Covered by the adapter" rows:
| renderPage() → renderView() shim | ...
| renderPageToString() shim | ...
| $legacySendEmail() argument remap | ...
| $legacyAppScopeGet(key) opt-in helper | ...
| Migration scanner | ...
| Deprecation call logger | ...
wheels-legacy-adapter lives in the external wheels-dev/wheels-legacy-adapter repo and is not present in vendor/ here, so these claims can't be verified against source from this PR. The PR author should confirm the table accurately reflects the current published release of wheels-dev/wheels-legacy-adapter before merge. Not a blocker — just an authorial confirmation request.
Commits
Single commit d430ea3 — type docs, scope web/guides, subject 62 chars, not ALL-CAPS. Signed-off-by trailer present. Clean.
Tests
No specs needed or expected — pure prose, table addition, and anchor-link fix.
Both nits from the prior round are resolved. The scope table provenance note is the only remaining item, and it is informational. COMMENT verdict; safe to merge once the adapter scope table is confirmed by the PR author.
Scope-table verificationThe "Covered by the adapter" rows were verified against the published
Method names match the rows exactly; the standalone |
Wheels Bot — Reviewer B (round 1)Critiquing review 4278358305 by wheels-bot[bot] on commit A's review is accurate across all claims. Both prior nits are confirmed resolved in the diff, the anchor slug logic is sound, and the commit is clean. No sycophancy, no false positives, and no missed issues. SycophancyNone detected. A rendered a COMMENT verdict (not APPROVE) despite the PR being functionally merge-ready, citing one unverifiable external claim. That is not sycophantic — it is appropriately cautious. False positivesNone detected. Nit 1 resolution ("before the upgrade" wording): A claims the v4-0-0 and v4-0-1-snapshot files now read "must update those references manually — the adapter cannot defer or soften this item." Confirmed from the diff: the second Nit 2 resolution ("See item 10."): A claims the Common Issues bullet is now Anchor slug claim: A asserts that Starlight/remark-slug processes Commit metadata: A claims type Missed issuesNone detected. CI is all green (Deploy guides, Visual regression, Lucee 7 + SQLite, Validate Commit Messages all pass). The collaborator comment about failing CI was posted before A's review; those issues were resolved before A ran. Verdict alignmentA's COMMENT verdict is consistent with the findings. The only outstanding item — confirming the scope table against the external ConvergenceAligned with Reviewer A. No bot-driven changes are needed. The PR is technically correct: both prior nits are resolved, the anchor link is valid, and CI is clean. The remaining item (scope table confirmation) is an informational note for the PR author, not a requested change. Safe to merge once the author confirms the table reflects the current adapter release and a human reviewer approves. |
The upgrade guide's item 10 (
application.wirebox→application.wheelsdi) previously said nothing about whatwheels-legacy-adapterdoes and does not cover for this specific rename. A reader following the guide's "install the adapter, ship the upgrade, modernize incrementally" framing could reasonably expect the adapter to handle item 10 — it does not. Apps that bootstrap WireBox directly or read fromapplication.wireboxboot-fail immediately after upgrade, even with the adapter installed.This PR adds an explicit
<Aside type="caution">to item 10 in both thev4-0-0andv4-0-1-snapshotupgrade guides, adds a scope table to the Legacy Compatibility Adapter section, and adds aCommon Issuesentry for the wirebox boot failure.Changes
web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx<Aside type="caution">stating the adapter does not create anapplication.wireboxalias and does not restore thewirebox.system.ioc.Injectorpackage path<Aside type="caution">reinforcing the item 10 gapweb/sites/guides/src/content/docs/v4-0-1-snapshot/upgrading/3x-to-4x.mdx— identical changesCHANGELOG.md— added### Documentationentry under[Unreleased]Screenshots needed
None — this is a prose and table addition with no UI component.
Fixes #2627