Repository navigation
fix(config): interpolate plugin and package names in load-trace log messages - #2641
Conversation
…essages The "Loading plugin..." / "Loading package..." INFO lines in wheels_security.log were emitting literal placeholder tokens (`#arguments.dirName#`, `#local.pluginKey#`, etc.) instead of the resolved values. The `WriteLog(text="...")` strings escaped each `#` as `##` — CFML's way to write a literal `#` — so the placeholders never ran through expression evaluation and the log was useless for diagnosing which plugin or package was being loaded. Drop the escaping so single-pound interpolation kicks in. Add a spec under `vendor/wheels/tests/specs/packages/` that inspects both call sites and fails fast if the escape pattern reappears. Fixes #2630 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — No doc updatesReviewed this PR's diff and found no docs that need updating (purely internal logging bug fix: two |
![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 fixes a real, reproducible bug -- double-escaping in two WriteLog call sites that caused literal placeholder tokens to land in wheels_security.log instead of resolved values. The two-line production fix is correct and minimal. The accompanying spec is well-placed and follows established patterns. Three minor nits below, none blocking.
Correctness
Fix is accurate. In CFScript, double-quoted string literals evaluate pound-sign expressions at runtime. The original ##var## tells the runtime to emit the literal characters #var# (each ## becomes one #); stripping one pound from each side restores interpolation. The change to both call sites is correct.
No other bugged call sites. Every other WriteLog invocation in PackageLoader.cfc (lines 198, 218, 271, 304, 360, 433, 465) and Plugins.cfc (lines 120, 176, 238, 246, 562, 781) already uses single-pound interpolation. The PR scope is exactly right -- it does not over-fix or under-fix.
Conventions
Commit scope is mildly misleading.
The commit is fix(config): interpolate plugin and package names... but the touched files are vendor/wheels/PackageLoader.cfc and vendor/wheels/Plugins.cfc -- the package/plugin loader layer, not the config layer. Scope is unrestricted by commitlint.config.js, so this is not a hard violation, but a future maintainer running git log --grep config to audit config changes will pick this up unexpectedly. A scope like packages or plugin would map more cleanly to the files touched.
Tests
Spec location and style are correct. vendor/wheels/tests/specs/packages/PackageLoaderLogInterpolationSpec.cfc extends wheels.WheelsTest, uses BDD describe/it syntax, and follows the source-inspection pattern already in use in CliTemplateInstallVerbSpec.cfc (lines 16-31). ExpandPath("/wheels/PackageLoader.cfc") is the correct mapping under a standard Wheels install. The ####var#### -> runtime ##var## escape logic in the assertions is correctly reasoned.
Test only negatively asserts -- consider adding a positive guard.
The spec proves the bad pattern is absent but does not prove the correct pattern is present. If a future refactor removes the log line entirely (e.g. restructures the lazy-vs-eager path), the test stays green even though the diagnostic log message is gone. A companion positive assertion would lock both sides:
// suggested addition to the PackageLoader it() block
expect(FindNoCase("'##arguments.dirName##'", source) GT 0).toBeTrue(
"PackageLoader.cfc must retain single-pound interpolation around "
& "arguments.dirName in WriteLog text -- see issue ##2630."
);(and symmetrically for arguments.pkgDir, local.pluginKey, and local.pluginValue.folderPath in the Plugins block)
Verbose block comment in the spec. CLAUDE.md convention is to omit comments unless the WHY is non-obvious. A 13-line block comment at the top of the spec re-states what the PR description already captures. A single-line reference would be sufficient.
Docs
CHANGELOG [Unreleased] -> Fixed entry is present and well-worded. No user-facing API changed, so MDX guide updates are correctly omitted.
Security
No concerns. If anything, making wheels_security.log emit real values rather than placeholder tokens makes the log more useful for operators diagnosing load failures.
Wheels Bot — Reviewer B (round 1)A's review is technically accurate and well-scoped. The fix correctness check is thorough — A verified every other WriteLog call site in both files, confirmed no over-fixing or under-fixing, and correctly reasoned through the CFML escape semantics. The two actionable nits (verbose block comment, missing positive assertions) are real and both rooted in CLAUDE.md conventions or test robustness. No sycophancy or false positives found. Aligning with A; emitting SycophancyNone detected. A backed each claim with specific line references or escape-sequence reasoning and did not approve despite flagging actionable issues. False positivesNone detected. The CFML escape analysis is accurate ( Missed issuesNone detected. The diff is a two-line production change plus a 60-line spec; A's coverage is proportionate. Verdict alignmentA chose ConvergenceAligned. Joint recommendation: (1) collapse the 13-line block comment at the top of |
- Collapse the 13-line block comment at the top of vendor/wheels/tests/specs/packages/PackageLoaderLogInterpolationSpec.cfc to a single line per CLAUDE.md's "one short line max" rule. - Add positive assertions in both it() blocks verifying the correct single-pound pattern is present in PackageLoader.cfc and Plugins.cfc so the spec stays meaningful if the WriteLog line is later removed. Refs PR #2641, issue #2630. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):
Skipped:
Verification:
The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR correctly fixes double-escaped pound signs in two WriteLog call sites so plugin and package names actually appear in wheels_security.log instead of literal #var# placeholders. The round-1 feedback from the previous review cycle has been addressed: the 13-line top-of-file block comment is now a single line, and both it() blocks carry positive assertions that lock the correct pattern in place. Two minor convention nits remain, neither blocking.
Conventions
Multi-line comment blocks in the first it() block violate CLAUDE.md's one-short-line-max rule.
CLAUDE.md says "Never write multi-line docstrings or multi-line comment blocks -- one short line max." Two places in the PackageLoader it() block exceed that:
1. The 4-line escaping-logic comment (introduced in the original commit, not condensed by the round-1 fix):
// "####" in a CFML string literal evaluates to the two-char
// sequence "##", so this search looks for the literal four
// characters "####" in the source. That four-char sequence is
// CFML's escape for "##" in a runtime string -- the bug.The WHY is non-obvious and worth a comment, but one line is sufficient. Suggested:
// "####" in CFML string literals evaluates to "##" -- searches for the 4-char escape form that is the bug.2. The 2-line positive-assertion comment (introduced by the round-1 fix commit):
// Positive assertions: lock the single-pound form in place so a
// future refactor that silently removes the log line fails the
// test instead of staying green.Condense to one line:
// Positive guard: fails if the WriteLog line is silently removed.The equivalent comment in the Plugins it() block (// Positive assertions: see PackageLoader block above for rationale.) is already a single line and is fine.
First commit scope remains config.
The first commit (d1d9f863) still carries fix(config): interpolate plugin and package names.... The files touched are vendor/wheels/PackageLoader.cfc and vendor/wheels/Plugins.cfc -- the package/plugin loader layer, not config. The round-1 fix commit correctly used fix(packages). Scope is unrestricted by commitlint.config.js so this is not a hard violation, but a reader running git log --grep=config to audit config-layer changes will pick this up unexpectedly. Fixing requires rebasing the first commit; acceptable to leave if the team prefers linear history.
Tests
Both it() blocks now carry negative assertions (the double-escape form must be absent) and positive assertions (the single-pound form must be present). This makes the spec meaningful even if a future refactor removes or restructures the WriteLog line. Round-1 feedback on test coverage is fully addressed.
The source-inspection approach (FileRead(ExpandPath("/wheels/PackageLoader.cfc"))) uses only standard CFML built-ins and is portable across all engines. The #### / ## escape arithmetic in the spec is correct: "####arguments.dirName####" evaluates at runtime to "##arguments.dirName##", which is the literal bug-form to search for; "'##arguments.dirName##'" evaluates to "'#arguments.dirName#'", which is the correct single-pound form the fix introduced.
Docs
CHANGELOG [Unreleased] -> Fixed entry is present and accurate. No user-facing API changed; MDX guide updates are correctly omitted.
Commits
| SHA | Subject | DCO |
|---|---|---|
d1d9f863 |
fix(config): interpolate plugin and package names... |
Signed-off present |
0988de83 |
fix(packages): address Reviewer A/B consensus findings (round 1) |
Signed-off present |
Both commits conform to commitlint (type from allowlist, subject <= 100 chars, not ALL-CAPS). DCO Signed-off-by trailers are present on both.
Security
No concerns. The values interpolated into the log (arguments.dirName, arguments.pkgDir, local.pluginKey, local.pluginValue.folderPath) originate from filesystem directory scanning, not user-controlled input. The data was already being passed to WriteLog before this fix -- the change only makes the placeholders resolve correctly instead of emitting literal token text.
Wheels Bot — Reviewer B (round 1)A's review on the updated SHA is accurate and well-grounded. Both remaining findings are real CLAUDE.md violations confirmed by direct reading of the diff — the round-1 address-review commit condensed the 13-line top-of-file comment but left two shorter multi-line blocks inside the first SycophancyNone detected. A verified each finding by citing the actual lines and explaining the applicable rule before calling them non-blocking, and did not approve despite leaving them open. False positivesNone detected.
Missed issuesNone detected. The production fix is a two-line change in Verdict alignmentA chose ConvergenceAligned. Joint recommendation: (1) condense the 4-line escaping-logic comment in the PackageLoader |
- Collapse the 4-line escaping-logic comment in the PackageLoader it() block of vendor/wheels/tests/specs/packages/PackageLoaderLogInterpolationSpec.cfc to a single line per CLAUDE.md's "one short line max" rule. - Collapse the 3-line positive-assertion comment in the same it() block to a single line: "Positive guard: fails if the WriteLog line is silently removed." The single-line comment in the Plugins it() block was already compliant. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Address Review (round 2)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 2):
Already compliant (no change needed):
Verification:
The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR fixes the double-escaping bug in two WriteLog call sites so plugin and package names actually resolve in wheels_security.log. The production fix is correct and minimal. All feedback from both prior review cycles has been addressed — multi-line comment blocks have been condensed to single lines, and both it() blocks carry positive assertions that lock the correct single-pound form in place. One cosmetic nit on the first commit's scope has been noted twice already and is not blocking. This is ready for a human approver.
Correctness
Production fix verified. Both call sites now use single-pound interpolation.
vendor/wheels/PackageLoader.cfc:369:
text = "[Wheels] Loading package '#arguments.dirName#' from #arguments.pkgDir#",vendor/wheels/Plugins.cfc:144:
text = "[Wheels] Loading plugin '#local.pluginKey#' from #local.pluginValue.folderPath#",No other call sites in either file use the double-escape form.
Conventions
The round-2 commit correctly condenses the remaining multi-line comment blocks:
- 4-line escaping-logic comment condensed to:
// "####" in a CFML string literal evaluates to "##" at runtime — the bug form. - 3-line positive-assertion comment condensed to:
// Positive guard: fails if the WriteLog line is silently removed.
Both now comply with CLAUDE.md's one-short-line-max rule.
Residual nit (third mention, non-blocking): The first commit (d1d9f863) still carries scope config even though the files touched are vendor/wheels/PackageLoader.cfc and vendor/wheels/Plugins.cfc. Not a commitlint violation — scope is unrestricted — but a reader grepping history for config-layer changes will pick it up. The two subsequent commits correctly use scope packages. Acceptable to leave for linear history.
Tests
vendor/wheels/tests/specs/packages/PackageLoaderLogInterpolationSpec.cfc:
- Extends
wheels.WheelsTest, BDDdescribe/itsyntax — correct. - Each
it()carries negative assertions (double-escape form must be absent) and positive assertions (single-pound form must be present). Both sides locked. - Escape arithmetic correct:
"####arguments.dirName####"evaluates at runtime to"##arguments.dirName##"(the literal bug string);"'##arguments.dirName##'"evaluates to"'#arguments.dirName#'"(the correct fixed form). Verified against actual source text. - Uses only standard CFML built-ins (
FileRead,ExpandPath,FindNoCase) — cross-engine safe.
Commits
| SHA | Subject | DCO |
|---|---|---|
d1d9f863 |
fix(config): interpolate plugin and package names in load-trace log messages |
Signed-off present |
0988de83 |
fix(packages): address Reviewer A/B consensus findings (round 1) |
Signed-off present |
6ee27fef |
fix(packages): address Reviewer A/B consensus findings (round 2) |
Signed-off present |
All commits conform to commitlint (type from allowlist, subject ≤ 100 chars, not ALL-CAPS).
Docs
CHANGELOG [Unreleased] → Fixed entry is present and accurate. No user-facing API changed; MDX guide and .ai/ reference updates are correctly omitted.
Security
No concerns. The interpolated values (arguments.dirName, arguments.pkgDir, local.pluginKey, local.pluginValue.folderPath) originate from filesystem directory scanning, not user-controlled input.
Wheels Bot — Reviewer B (round 1)A's review on the current SHA is accurate, well-grounded, and reaches the correct conclusion. The production fix is a minimal two-line change with correct escape semantics. Both SycophancyNone detected. A did not approve despite the outstanding non-blocking nit (commit scope on False positivesNone detected.
Missed issuesNone detected. The scope of this PR is a two-line production fix plus a 68-line spec. A checked correctness, convention compliance, test coverage, commit hygiene, docs, and security. Coverage is proportionate to the change size. Verdict alignmentA's ConvergenceAligned. The PR is in a merge-ready state. The first commit's |
…ssages-emit-literal-va
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR (already merged) correctly fixes the double-escaped pound signs in two WriteLog call sites so plugin and package names resolve in wheels_security.log instead of emitting literal #var# placeholders. The production fix is minimal and accurate, the spec covers both negative and positive assertions, and all CLAUDE.md feedback from the prior two review rounds has been addressed. Two cosmetic nits noted below, neither blocking.
Correctness
Production fix verified in the final diff:
vendor/wheels/PackageLoader.cfc:369:
text = "[Wheels] Loading package '#arguments.dirName#' from #arguments.pkgDir#",vendor/wheels/Plugins.cfc:144:
text = "[Wheels] Loading plugin '#local.pluginKey#' from #local.pluginValue.folderPath#",Both call sites use single-pound interpolation. No other WriteLog call in either file uses the double-escape form.
Conventions
Verbose assertion error message in the first toBeFalse() call of each it() block.
The first negative assertion in each block uses a 5-line concatenated string argument:
// vendor/wheels/tests/specs/packages/PackageLoaderLogInterpolationSpec.cfc, lines 12–18
expect(FindNoCase("####arguments.dirName####", source) GT 0).toBeFalse(
"PackageLoader.cfc must not emit escaped pounds around "
& "arguments.dirName in WriteLog text — CFML reads each "
& "doubled pound as a literal pound, so the placeholder "
& "never interpolates. Use a single pound on each side. "
& "See issue ##2630."
);Subsequent assertions in the same block use a concise 1–2 line form. Assertion messages are not comment blocks, so this is not a strict CLAUDE.md violation, but the inconsistency is a style nit. Could be condensed:
expect(FindNoCase("####arguments.dirName####", source) GT 0).toBeFalse(
"PackageLoader.cfc must not double-escape arguments.dirName in WriteLog — see issue ##2630."
);Not blocking.
First commit scope config (noted in rounds 1 and 2) — d1d9f863 still carries fix(config): interpolate plugin and package names... but touches vendor/wheels/PackageLoader.cfc and vendor/wheels/Plugins.cfc. Not a commitlint violation; scope is unrestricted. Third and final mention — no action needed.
Tests
vendor/wheels/tests/specs/packages/PackageLoaderLogInterpolationSpec.cfc:
- Extends
wheels.WheelsTest, BDDdescribe/itsyntax — correct; not RocketUnit. - Each
it()carries negative assertions (double-escape form must be absent) and positive assertions (single-pound form must be present). Round-1 feedback on missing positive guards fully addressed. - Comment blocks condensed to single lines per CLAUDE.md. Round-2 feedback addressed.
- Uses only standard CFML built-ins (
FileRead,ExpandPath,FindNoCase) — cross-engine safe across Lucee, Adobe CF, and BoxLang. - Escape arithmetic correct:
"####arguments.dirName####"evaluates at runtime to"##arguments.dirName##"(the literal bug string);"'##arguments.dirName##'"evaluates to"'#arguments.dirName#'"(the correct fixed form). Both verified against source text.
Commits
| SHA | Subject | DCO |
|---|---|---|
d1d9f863 |
fix(config): interpolate plugin and package names in load-trace log messages |
Signed-off present |
0988de83 |
fix(packages): address Reviewer A/B consensus findings (round 1) |
Signed-off present |
6ee27fef |
fix(packages): address Reviewer A/B consensus findings (round 2) |
Signed-off present |
All conform to commitlint (type from allowlist, subject ≤ 100 chars, not ALL-CAPS). DCO trailers present on all three.
Docs
CHANGELOG [Unreleased] → Fixed entry is present and accurately describes the fix. No user-facing API changed; MDX guide and .ai/ reference updates correctly omitted.
Security
No concerns. The interpolated values (arguments.dirName, arguments.pkgDir, local.pluginKey, local.pluginValue.folderPath) originate from filesystem directory scanning, not user-controlled input. Making the log emit real values rather than placeholder tokens improves operator diagnostics.
Wheels Bot — Reviewer B (round 1)A's review on the merge SHA is accurate and consistent with the prior converged-approve issued on SycophancyNone detected. A did not issue a blanket LGTM — the two nits are genuine observations with specific line references and clear rationale, and A withheld a formal APPROVE because of them. False positivesNone detected.
Missed issuesNone detected. The diff is a two-line production change plus a 68-line spec. The merge commit ( Verdict alignmentA's COMMENTED verdict is appropriate. The PR is merged, both nits are cosmetic and non-blocking, and A's narrative conclusion (fix is correct, spec is thorough, all prior feedback addressed) is consistent with the COMMENTED state and the absence of a request-changes verdict. ConvergenceAligned. B converged-approve on the pre-merge SHA |
Resolves the literal-placeholder bug in the plugin and package loader trace logs. Both call sites —
vendor/wheels/PackageLoader.cfc:369andvendor/wheels/Plugins.cfc:144— were writingWriteLog(text="...##var##..."). CFML reads each##as the escape sequence for a literal#, so the runtime string passed toWriteLogalready contained#var#and never made it through expression evaluation. The log line ended up asLoading package '#arguments.dirName#' from #arguments.pkgDir#— useless for diagnosing which plugin or package was being loaded when a failure occurred. Dropping the escape so each##var##becomes#var#makes single-pound interpolation kick in and the rendered values land in the log.A new spec at
vendor/wheels/tests/specs/packages/PackageLoaderLogInterpolationSpec.cfcinspects both call sites and asserts the malformed escape pattern is gone, so a future regression fails the test suite immediately instead of silently showing up inwheels_security.log.Fixes #2630
Type of Change
Verification Checklist
vendor/wheels/tests/specs/packages/PackageLoaderLogInterpolationSpec.cfc(failing → passing).ai/wheels/)[Unreleased]wheels.tests.specs.packagesdirectory: 83 pass / 0 fail / 0 error (Lucee 7 + SQLite, local). Full suite re-run: 3482 pass / 0 fail / 0 error.