feat(config): plural mappings in package.json for legacy CFML aliases (#2705)#2739
Conversation
github-actions
Bot
added
docs
enhancement
dependencies
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR adds a plural mappings field to package.json so a package can register additional CFML mapping aliases beyond the auto-derived singular one, solving the wheels-sentry plugins.sentry compatibility case from #2705. The implementation is well-structured: registration order is enforced (singular first), partial-plural failures unwind cleanly, cross-engine application.mappings access is guarded in try/catch, and 13 BDD specs cover the main happy and failure paths. No correctness, cross-engine, or security blockers. Three minor nits below.
Correctness
Stale comment in $rollbackPackage (vendor/wheels/PackageLoader.cfc lines 531-537):
The existing comment says:
those registries are written only by $registerPackageMapping on its success path, so by the time this function runs (either from $discover's catch on a pre-mapping exception, or from $tryRegisterPackageMapping's false path where the registration itself never wrote anything) the mapping registries are already clean.
This is now misleading. With this PR, $registerAdditionalMappings CAN write plural entries to packageMappings and $mappingProviders before hitting a mid-loop collision. The code stays correct because $unregisterMappings is called before $rollbackPackage on the plural failure path — so the registries ARE clean when $rollbackPackage executes. But the comment's explanation is now incomplete and could confuse a future reader who sees the comment, adds a new plural-style registration path, and concludes no cleanup is needed before calling $rollbackPackage.
Suggest updating that paragraph to:
* Intentionally does NOT clean variables.packageMappings or
* variables.$mappingProviders: callers are responsible for unwinding those
* registries before reaching here. From $discover's catch (pre-mapping
* exception) nothing was written; from $tryRegisterPackageMapping's false
* path $unregisterMappings has already cleaned both any partial plural
* entries and the singular alias. Adding cleanup here would mask a future
* caller that forgets to unwind.
Tests
Missing branch coverage: non-struct mappings value (vendor/wheels/tests/specs/packages/PackageMappingsPluralSpec.cfc):
The $registerAdditionalMappings guard:
if (!IsStruct(arguments.manifest.mappings)) {
local.result.ok = false;
local.result.error = "Invalid package mappings block";has no corresponding spec. Every other validation branch (invalid name segment, absolute path, .. traversal, collision) has a dedicated it(). A small fixture whose package.json carries "mappings": "not-a-struct" and a matching it("fails a package whose mappings field is not a struct") would close the gap.
Docs
CLAUDE.md manifest example code block not updated (CLAUDE.md Package System section):
The PR adds a text description of the mappings field — correct. But the package.json JSON code block that serves as the canonical at-a-glance manifest reference was not updated to show the new field. A reader scanning the code block will see mapping but not mappings. Suggest adding the field to that block, matching the PR body example:
"mappings": {
"plugins.sentry": "."
}No commitlint violations. The single commit (feat(config): add plural mappings field to package.json for additional CFML aliases) is within 100 chars and uses a valid type. No security concerns: path traversal and absolute-path injection are rejected, application.mappings mutation is try/caught, and no user-controlled input reaches the registration path outside a package's own package.json.
Wheels Bot — Reviewer B (round 1)A's review is mostly solid but contains one false positive that should be retracted, and has a minor missed-coverage gap alongside its already-valid findings. SycophancyNone detected. A walked through correctness, cross-engine safety, tests, docs, and commits before concluding. False positivesFinding 3 (CLAUDE.md JSON code block): A writes "a reader scanning the code block will see Missed issuesNon-simple-value mapping path ( if (!IsSimpleValue(local.value)) {
local.result.ok = false;
local.result.error = "Invalid package mapping path";
...
}has no spec. No fixture carries an array or struct as a mapping path value (e.g. Verdict alignmentA's verdict is COMMENTED (non-blocking). The two valid nits — stale ConvergenceNot aligned yet. A has a false positive in finding 3 that needs to be retracted: the CLAUDE.md JSON code block does not contain |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The follow-up commit (95556fe16) correctly addresses all three items from the first review round: the $rollbackPackage comment is refreshed, the missing non-struct-block spec is added with a second rollback-assurance test, and the mappings field now appears in both JSON example blocks (CLAUDE.md and packages.mdx). No correctness, cross-engine, or security blockers remain. Two administrative items below — DCO sign-off is missing from both commits and will block the DCO required status check, and the CHANGELOG has no entry for this feature.
Commits
DCO sign-off missing from both commits.
49ac4cf81 feat(config): add plural mappings field to package.json for additional CFML aliases
95556fe16 docs(config): address Reviewer A feedback on plural mappings field
Neither commit has the required Signed-off-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com> trailer. The DCO GitHub App is a required status check on every PR — missing sign-offs will block the merge check until the commits are amended or a DCO override is applied. See CONTRIBUTING.md § DCO.
Commitlint is clean: both types (feat, docs) are in the allowlist, scope config is a valid free-form scope, subjects are under 100 chars, and neither is ALL-CAPS.
Docs
CHANGELOG.md is missing an [Unreleased] entry for the plural mappings field.
The existing entry under [Unreleased] ### Added covers the singular mapping field from PR #2712:
PackageLoadernow derives a per-package CFML mapping frompackage.json...
But the plural mappings struct (this PR) is a distinct, additive feature that a framework consumer would need to know about when upgrading. The convention elsewhere in the file is one bullet per feature. Suggest appending something like:
- `package.json` now accepts a `mappings` struct (plural) so a package can
register additional dotted CFML mapping aliases beyond the singular
`mapping` identifier — e.g. `{"plugins.sentry": "."}` for legacy
compatibility paths. Absolute paths and `..` traversal are rejected.
Collisions with any existing alias (singular or plural) fail the package
and unwind its singular registration. (#2739)
No other findings. The two new it() blocks in PackageMappingsPluralSpec.cfc (lines 85–107) correctly cover the IsStruct guard path and the singular-alias rollback, the fixture at packages_mappings_plural_invalid_block/notastruct/ is well-formed, and the $rollbackPackage comment (lines 531–538 of PackageLoader.cfc) now accurately describes the caller-unwinds-first contract.
…l CFML aliases A package can now declare a struct of dotted CFML mapping aliases beyond the singular `mapping` field. Each key is a dotted name (e.g. `plugins.sentry`); each value is a path relative to the package directory. Reflected into `application.mappings` so `new plugins.sentry.X()` resolves at runtime regardless of the package's install location. Use case from #2705: wheels-sentry@1.0.0 hardcodes `new plugins.sentry.SentryClient(...)` but the installer drops the package at `vendor/wheels-sentry/`, so the dotted path silently fails to resolve. A `wheels-sentry@1.0.1` release can now declare `{"mappings": {"plugins.sentry": "."}}` and the existing callsite keeps working without source changes. Validation: each dot-separated segment must match the CFML identifier regex; absolute paths and `..` traversal are rejected. Cross-form collisions (a plural entry colliding with another package's singular alias) fail the package and unwind its own singular registration so the mapping registries stay internally consistent. Tracking upstream wheels-sentry patch: #2738. Signed-off-by: Claude <noreply@anthropic.com>
- Refresh the $rollbackPackage doc comment: callers are now responsible for unwinding mapping registries before reaching rollback (the singular path never wrote anything, the plural path runs $unregisterMappings first). The pre-PR claim that "the mapping registries are already clean" because nothing writes to them outside the success path is no longer true once plural entries can write before mid-loop failure. - Add the missing test for the non-struct `mappings` field guard: every other validation branch had a dedicated spec. - Surface `mappings` in the canonical package.json JSON example in both CLAUDE.md and the public author guide so a reader scanning the block sees the new field, not just the prose description. No behaviour change. Signed-off-by: Claude <noreply@anthropic.com>
Reviewer B (round 1) flagged that the per-entry IsSimpleValue guard in $registerAdditionalMappings had no spec — structurally the same gap as the non-struct mappings block A flagged. Adding a fixture whose mapping value is an array (a non-simple value) and two specs: one for the failedPackages entry, one for the rollback of the singular alias. Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
bpamiri
force-pushed
the
claude/review-issue-2705-7FfDj
branch
from
41cca97 to
15e4e1f
Compare
Wheels Bot — Reviewer B (round 1)A's review on the current HEAD is concise and its two findings (DCO and CHANGELOG) are both real. One finding is factually incomplete, and B's own prior critique requires a correction. SycophancyNone detected. A walked through what changed in commit False positivesNone detected. A's two findings — DCO and CHANGELOG — are both substantiated. Missed issuesA's DCO finding is incomplete. A cites two commits ( Correction to B's round 1 critiqueB (round 1) flagged A's earlier finding 3 as a false positive, claiming the CLAUDE.md JSON code block contained neither Verdict alignmentA's verdict is COMMENTED (non-blocking). The two genuine issues — DCO (all three commits) and CHANGELOG — are administrative and do not block correctness. The non-blocking stance is appropriate. ConvergenceAligned on COMMENTED, pending the DCO correction. A's only concrete error is citing two commits instead of three in the DCO finding; once that is corrected to cover |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The third commit (41cca9743) correctly closes the IsSimpleValue spec gap flagged by Reviewer B round 1 — the nonsimplevalue fixture (array path value), the failedPackages assertion, and the singular-alias rollback assertion are all well-formed and match the established spec pattern. No correctness, cross-engine, or security issues in the new code. The two items flagged in round 2 remain outstanding: DCO sign-off is missing from all three commits and will block the required status check; the CHANGELOG still has no entry for the plural mappings feature.
Commits
DCO sign-off still missing from all three commits.
49ac4cf81 feat(config): add plural mappings field to package.json for additional CFML aliases
95556fe16 docs(config): address Reviewer A feedback on plural mappings field
41cca9743 test(config): cover non-simple-value plural mapping path branch
None carry a Signed-off-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com> trailer. The DCO GitHub App is a required status check — missing trailers will block the merge check until the commits are amended or a DCO override is applied. See CONTRIBUTING.md § DCO.
Commitlint is clean across all three commits: types (feat, docs, test) are in the allowlist, scope config is a valid free-form scope, all subjects are under 100 chars, none are ALL-CAPS.
Docs
CHANGELOG.md [Unreleased] section still has no entry for the plural mappings field.
The existing Added bullet covers the singular mapping field from PR #2712 but not the plural mappings struct introduced by this PR. The suggested text from the round-2 review still applies:
- `package.json` now accepts a `mappings` struct (plural) so a package can
register additional dotted CFML mapping aliases beyond the singular
`mapping` identifier — e.g. `{"plugins.sentry": "."}` for legacy
compatibility paths. Absolute paths and `..` traversal are rejected.
Collisions with any existing alias (singular or plural) fail the package
and unwind its singular registration. (#2739)
Tests (new commit — clean)
The two new it() blocks in PackageMappingsPluralSpec.cfc covering the nonsimplevalue fixture correctly exercise the per-entry IsSimpleValue guard. The fixture ("mappings": {"plugins.nonsimple": ["."]}) triggers the guard since an array is not a simple value. The rollback assertion (notToHaveKey("wheelsNonsimplevalue")) matches the derived alias for wheels-nonsimplevalue. Both specs create a fresh PackageLoader scoped to invalidBlockPath, which now contains both notastruct and nonsimplevalue — both fail independently so co-presence causes no cross-contamination.
Wheels Bot — Reviewer B (round 1)A's review body is stale: it was written against an older commit set that was superseded before (or at the moment) A submitted the review. Both of A's findings are false positives for the current HEAD ( SycophancyNone detected. A walked through commit correctness, cross-engine safety, tests, and admin hygiene before concluding. False positivesFinding 1 (DCO sign-off): A cites three commits as missing These SHAs do not exist in the current branch. The current four commits are Finding 2 (CHANGELOG missing): A writes "the CHANGELOG still has no entry for the plural
The entry is present, accurate, and consistent with the surrounding CHANGELOG style. A's finding is a false positive. Missed issuesNone detected on re-scan. A's positive assessment of the Verdict alignmentA's verdict is COMMENTED (non-blocking). Given that A's only substantive findings are both false positives for the current HEAD, the effective verdict is that the PR is clean — which is consistent with approve, not with requesting further changes. ConvergenceAligned on approve. Both of A's findings in this review are stale artifacts of a commit set that was superseded before A submitted. The current HEAD has DCO sign-offs on every commit, a CHANGELOG entry, cross-engine-safe code, and test coverage for every validation branch in |
2 participants
Summary
Adds a plural
mappingsfield topackage.jsonso a package can declare additional dotted CFML mapping aliases beyond the singularmapping. Resolves the framework half of #2705.{ "name": "wheels-sentry", "mapping": "wheelsSentry", "mappings": { "plugins.sentry": ".", "wheelsSentryTools": "tools" } }Each key is a dotted name (each segment must match
[A-Za-z_][A-Za-z0-9_]*). Each value is a path relative to the package directory ("."or""for the root,"sub"for a subdirectory). Absolute paths and..traversal are rejected so a package can't claim mappings outside its install tree. The slash-form is reflected intoapplication.mappings(plugins.sentry→/plugins/sentry) sonew plugins.sentry.SentryClient()resolves regardless of where the package was installed.Why
wheels-sentry@1.0.0'sSentry.cfc:79hardcodesnew plugins.sentry.SentryClient(...). The package installer drops the tarball atvendor/wheels-sentry/, so the dotted path silently fails to resolve, the surroundingtry/catchswallows it, and the user gets a non-functional Sentry client with no log line. With this PR, the wheels-sentry maintainer can ship a 1.0.1 release that declares{"mappings": {"plugins.sentry": "."}}and the existing source line keeps working.The pattern generalizes — any package whose internal layout doesn't match its published name (hyphenated names, internal sub-namespaces, legacy compatibility aliases) can now keep static dotted callsites resolving across installs.
The upstream
wheels-sentry@1.0.1coordination is tracked in #2738.Behaviour contract
..traversal all fail the package — same loud-failure shape as the singular path.packageMappings,$mappingProviders, andapplication.mappingsso the registries stay internally consistent for the next load attempt.mappingsfield hits the same code path as before — the helper short-circuits on!StructKeyExists(manifest, "mappings")and returns ok.Files touched
vendor/wheels/PackageLoader.cfc—$registerAdditionalMappings,$validatePluralMappingName,$resolvePluralMappingPath,$unregisterMappings;$tryRegisterPackageMappingnow calls plural after singular.vendor/wheels/tests/specs/packages/PackageMappingsPluralSpec.cfc— 13 specs across basic registration, invalid names, invalid paths, plural-vs-plural collision, and cross-form (plural-vs-singular) collision. Collision specs are order-agnostic so they stay stable on filesystems that don't sort alphabetically.vendor/wheels/tests/_assets/packages_mappings_plural_*— five scenario-isolated fixture directories.web/sites/guides/src/content/docs/v4-0-1-snapshot/digging-deeper/packages.mdx— adds the field to the public manifest reference.CLAUDE.md— adds the field to the project manifest reference.Test plan
PackageMappingsPluralSpecspecs pass.PackageMappingSpecspecs continue to pass (singular path unchanged).vendor/wheels-sentry/, declare{"mappings": {"plugins.sentry": "."}}in its package.json, confirmnew plugins.sentry.SentryClient()resolves at runtime.Related
wheels-sentry@1.0.1source patch): Coordinate wheels-sentry@1.0.1 patch — resolve SentryClient by relative path or framework alias #2738Generated by Claude Code