fix(test): route browser-test artifact dir creation through File.mkdirs() for Adobe CF#2616
Conversation
…rs() for Adobe CF `BrowserTest.$captureFailureArtifacts` and `McpServer`'s test-file generator both called `directoryCreate(path, true)`. Adobe ColdFusion rejects the Lucee-only `createPath` flag with "Parameter validation error for the DIRECTORYCREATE function. The function takes 1 parameter.", which crashed the core test suite on ACF any time a browser spec failed and the artifact directory did not yet exist. Route the recursive directory creation through `java.io.File.mkdirs()`, which works uniformly on Lucee, Adobe CF, and BoxLang — same canonical workaround already used by `services/packages/ManifestCache.cfc::$ensureDir` (#2567). Adds `BrowserTestArtifactDirSpec` covering the nested-parent and already-exists cases against the extracted `$ensureArtifactDir` helper. Fixes #2614 Signed-off-by: Peter Amiri <petera@pai.com>
![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 correctly replaces the two Lucee-only directoryCreate(path, true) calls with java.io.File.mkdirs(), matches the canonical pattern established by ManifestCache in #2567, ships a properly-structured BDD spec that tests the multi-parent and no-op cases, and includes a CHANGELOG entry. The fix is sound; the nits below are minor convention items that do not block merging.
Correctness
McpServer.cfc — mkdirs() return value not checked (line 1121)
// vendor/wheels/public/mcp/McpServer.cfc:1121
createObject("java", "java.io.File").init(local.fullTargetDir).mkdirs();The canonical pattern in ManifestCache.$ensureDir (lines 132–135) captures the return value and re-verifies with DirectoryExists before throwing:
local.created = CreateObject("java", "java.io.File").init(arguments.path).mkdirs();
if (!local.created && !DirectoryExists(arguments.path)) {
Throw(type="Wheels.Packages.CacheDir", message="Could not create cache directory ...");
}For McpServer this is partially mitigated by the outer catch at line 1142 which converts any exception to an error string — the user would see a file-write error rather than a directory-creation error, which is survivable. Still, for consistency and diagnosability, consider the canonical form. Not a blocker.
Conventions
McpServer.cfc — stale meta-comment (line 1108)
// Debug logging removed - path resolution working correctlyThis is a comment about removed code, not about current behavior — exactly what CLAUDE.md flags: "Don't reference the current task, fix, or callers." It should be deleted.
BrowserTest.cfc — multi-line Javadoc block (lines 280–289)
/**
* Ensures the artifact directory exists, creating it (and any missing
* parents) when necessary. Routes through java.io.File.mkdirs() rather
* than directoryCreate(path, true) because Adobe ColdFusion rejects the
* ...
*/CLAUDE.md says "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The WHY is worth keeping, but a single inline reference is enough:
// Adobe CF rejects directoryCreate(path, true) — see issue #2614. Use mkdirs() instead.
public void function $ensureArtifactDir(required string path) {The same applies to the large block comment inside BrowserTestArtifactDirSpec.cfc (lines 6–15). The issue number in the it() label is sufficient documentation.
Tests
The spec extends wheels.WheelsTest (BDD syntax, not RocketUnit), covers the two meaningful cases (deeply-nested parents do not exist; directory already exists), cleans up via finally, and uses CreateUUID() to avoid test-run collisions. The ListFirst(Replace(...), "/") sweep logic in the first finally block correctly targets the root UUID directory for recursive delete.
One note: both it() closures instantiate new wheels.wheelstest.BrowserTest() inside the closure body rather than in a shared beforeEach. Constructing it twice is harmless here (no Playwright involved), but a shared setup would be idiomatic for this test framework.
Commits
fix(test): route browser-test artifact dir creation through File.mkdirs() for Adobe CF — type fix, scope test, subject 76 chars, not ALL-CAPS. Passes commitlint.
…back Reviewer A flagged four items on PR #2616: 1. McpServer.$ensureDir now matches ManifestCache's canonical mkdirs + re-check + throw pattern, so a silent mkdirs() failure surfaces as "Could not create test directory" instead of a downstream FileWrite error. 2. Drop the pre-existing "Debug logging removed - path resolution working correctly" meta-comment that violated CLAUDE.md's "no references to past changes" rule. 3. Shrink the $ensureArtifactDir Javadoc block to a single line per CLAUDE.md's "one short line max" guidance. 4. Compress the regression-context comment in BrowserTestArtifactDirSpec from 10 lines to 2, while preserving the issue reference in both the comment and the it() label. Note: the it() label now uses `##2614` because unescaped `#` in CFML string literals triggers the "Invalid Syntax Closing [##] not found" trap and crashes the entire spec bundle — verified locally. The shared-beforeEach suggestion was skipped (reviewer marked it "harmless here" and YAGNI applies — `new BrowserTest()` has no side-effects without Playwright). Signed-off-by: Peter Amiri <petera@pai.com>
|
Addressed in
Gotcha for archaeologists: moving |
Wheels Bot — Reviewer B (round 1)A's core findings are valid and A correctly avoided sycophancy. One genuine missed issue: the same multi-line-comment convention A flagged in BrowserTest.cfc and BrowserTestArtifactDirSpec.cfc was also violated by the new inline block in McpServer.cfc -- which this PR introduced -- but A skipped it. That inconsistency needs acknowledgment before we converge. One minor note on the stale-comment finding is also worth clarifying. SycophancyNone detected. A's TL;DR identifies real issues before saying the fix is sound, and the COMMENTED (non-approving) state is consistent with having outstanding findings. False positivesNone detected. I verified every specific claim:
Missed issuesMcpServer.cfc - new 6-line inline comment block (same violation A flagged elsewhere) The diff adds this to McpServer.cfc: // Adobe CF's DirectoryCreate rejects the Lucee-only createPath
// flag with "Parameter validation error for the DIRECTORYCREATE
// function. The function takes 1 parameter." (#2614). Route
// through java.io.File.mkdirs() so the recursive-create path
// works on every engine. Same pattern as ManifestCache (#2567)
// and BrowserTest.$ensureArtifactDir.This is six consecutive McpServer.cfc stale comment at line 1108 - pre-existing, not introduced by this PR A flagged Verdict alignmentA used COMMENTED with "fix is sound; nits do not block merging." That reads as a permissive non-blocking review, consistent with the nature of the issues (all convention nits, no functional defects). The COMMENTED state is appropriate given outstanding findings. ConvergenceNot aligned yet. A needs to acknowledge the McpServer.cfc 6-line comment block - either flag it as the same non-blocking nit as the others, or explain why it is treated differently. Once A addresses that inconsistency, the joint recommendation is clear: the functional fix is correct, all findings are convention nits in the same tier, and the verdict should be a permissive approve. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The refactor commit addresses all four items Reviewer A raised on the first round (meta-comment dropped, McpServer now uses the canonical mkdirs-check-throw pattern, docstring shrunk to one line, spec comment compressed). The fix itself is correct and the cross-engine approach is sound. One finding requires action before merge: both commits carry a DCO sign-off email that differs from the commit author email, which risks failing the required DCO status check.
Correctness
DCO email mismatch — both commits (required status check risk)
Both commits show:
Author: Peter Amiri <peter@alurium.com>
Signed-off-by: Peter Amiri <petera@pai.com>
petera@pai.com ≠ peter@alurium.com. CONTRIBUTING.md (§ DCO) and the shared-rails guidance both require the sign-off to match the configured git author identity. The DCO GitHub App is a required status check on every PR — if petera@pai.com is not a GitHub-verified email for this account, the check will fail and block the merge.
Fix: add the correct email to GitHub's verified addresses, or amend both commits to use Signed-off-by: Peter Amiri <peter@alurium.com>.
Conventions
BrowserTest.$ensureArtifactDir omits the mkdirs() return-value check (minor nit, not a blocker)
vendor/wheels/wheelstest/BrowserTest.cfc lines 281–287:
// Adobe CF rejects directoryCreate(path, true) — see #2614. Use mkdirs() instead.
public void function $ensureArtifactDir(required string path) {
if (directoryExists(arguments.path)) {
return;
}
createObject("java", "java.io.File").init(arguments.path).mkdirs();
}The canonical pattern from ManifestCache (lines 132–138, the reference implementation for this workaround) captures the return value and re-verifies:
local.created = CreateObject("java", "java.io.File").init(arguments.path).mkdirs();
if (!local.created && !DirectoryExists(arguments.path)) {
Throw(type="Wheels.Packages.CacheDir", message="Could not create cache directory...");
}The McpServer path (also changed in this PR) already follows the canonical form. $ensureArtifactDir is partially mitigated by the try/catch wrapping all of $captureFailureArtifacts (which deliberately swallows errors to avoid masking test failures), so the practical impact is: on a permission-denied failure, no screenshot is saved and no diagnostic surfaces. Aligning with the canonical pattern would produce a clearer log entry before the swallow. Not a blocker on its own.
Tests
The BrowserTestArtifactDirSpec covers the two meaningful paths against BrowserTest.$ensureArtifactDir (deeply-nested parents, already-exists no-op), uses CreateUUID() for collision-safety, and cleans up in finally. The McpServer path has no dedicated spec, but that path requires complex path-resolution setup; integration coverage via compat-matrix.yml is reasonable for it.
Commits
Both commit subjects pass commitlint (types fix and refactor, scope test, headers 82 and 73 chars respectively, not ALL-CAPS). The DCO sign-off concern is noted above under Correctness.
Cross-engine
No new cross-engine concerns introduced. java.io.File.mkdirs() is JVM-level and works identically on Lucee, Adobe CF, and BoxLang. The single-argument DirectoryCreate calls in the spec are safe on all engines.
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
Both of B's points are correct. I concede the McpServer 6-line comment-block miss (genuine inconsistency: I flagged the same CLAUDE.md violation in BrowserTest.cfc and BrowserTestArtifactDirSpec.cfc but skipped the identical block in McpServer.cfc, all three introduced by the same commit) and the pre-existing stale-comment attribution (line 1108 was above the @@ -1112 hunk start, so A should have noted it as a boy-scout observation, not a change-introduced issue). Both issues were subsequently fixed in the refactor commit (ed4ce69); no new action required on these points.
Conceded points
- McpServer.cfc 6-line comment block: Confirmed by reading
git show 71816cc85— the first commit introduced exactly the 6-line block B quoted (// Adobe CF's DirectoryCreate rejects the Lucee-only createPath...) without capturingmkdirs()'s return value. I flagged the equivalent multi-line blocks in BrowserTest.cfc and BrowserTestArtifactDirSpec.cfc but missed this one. The refactor commit collapsed it to one line and added the canonical return-value check, so it is resolved on the current SHA. - Stale comment at line 1108 is pre-existing: Confirmed — the first commit's hunk opens at line 1112 (
@@ -1112,7 +1112,13), placing line 1108 outside the changed region. A should have flagged it explicitly as a pre-existing boy-scout item rather than presenting it as PR-introduced. The refactor commit deleted it, resolving it on the current SHA.
Defended points
None. B's findings on the old SHA are accurate on both counts.
Updated verdict
On the old SHA the three comment-block findings are in the same nit tier and the functional fix was correct — B's proposed permissive-approve direction is right for that code. On the current SHA (ed4ce69) my separate review identified a DCO sign-off email mismatch (peter@alurium.com author vs Signed-off-by: petera@pai.com), which is a required status check; that concern stands and warrants request-changes until resolved. Once the DCO is cleared, the PR is approvable.
Wheels Bot — Reviewer B (round 1)A's review on the current SHA is accurate and well-reasoned. The DCO email mismatch finding is legitimate and A's CHANGES_REQUESTED verdict is consistent with it. One minor presentation gap: A does not explicitly confirm that B's round-1 concern (the McpServer 6-line comment block) was resolved by the refactor — but the code confirms it was. No false positives; no sycophancy; no significant missed issues. SycophancyNone detected. A identifies a real blocking issue (DCO) before acknowledging the fix is sound, and CHANGES_REQUESTED is not a permissive rubber-stamp. False positivesNone detected. Both claims verified against the diff:
Missed issuesA does not acknowledge B's round-1 finding was resolved (presentation gap, not a code defect) B's round-1 critique flagged that the McpServer.cfc 6-line comment block violated CLAUDE.md's "one short line max" rule — the same violation A had flagged in BrowserTest.cfc and BrowserTestArtifactDirSpec.cfc. The refactor commit compressed that block to a single line: // Adobe CF rejects directoryCreate(path, true) — see #2614. Use mkdirs() instead.A's current review does not mention this. It is not a miss in the traditional sense (the code is correct), but A should confirm the prior cross-review finding was addressed rather than only listing the four items from A's own original review. This does not change the verdict. No other missed issues found. Cross-engine, commitlint, and test coverage are all correctly assessed. Verdict alignmentA used CHANGES_REQUESTED driven by the DCO email mismatch. That is consistent: the DCO check is a required status that blocks merge, and the fix is not a code edit — it requires the author to either add ConvergenceAligned. A and B agree: the functional fix is correct, the cross-engine approach is sound, and the only outstanding item is the DCO sign-off email. The |
… pattern Capture mkdirs() return value and re-verify with directoryExists before throwing, matching the reference implementation in ManifestCache.$ensureDir. The surrounding try/catch in $captureFailureArtifacts still swallows the throw to preserve best-effort artifact capture semantics, but the throw now produces a diagnosable log entry before the swallow when a permission- denied or filesystem failure occurs. Signed-off-by: Peter Amiri <peter@alurium.com>
Wheels Bot — Address Review (round 1)Consensus from Reviewer A and Reviewer B (converged at
No new commits pushed in this round. The convergence loop will resume once the DCO is resolved; if Option A is taken, a fresh Reviewer A run on the same SHA should suffice. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The merge commit (44902d3b) brings no functional code change -- it only syncs the PR branch with develop. The substantive fix (routing directoryCreate(path, true) through java.io.File.mkdirs() in both BrowserTest and McpServer) is correct, cross-engine clean, and well-tested. One required-status-check blocker remains from the previous review cycle: two of the three code commits still carry a Signed-off-by email that does not match the git author email, meaning the DCO GitHub App check will fail and prevent merging.
Correctness
DCO sign-off mismatch -- commits 71816cc8 and ed4ce696 (required status check blocks merge)
# 71816cc854 -- fix(test): route browser-test artifact dir creation through File.mkdirs()
Author: Peter Amiri <peter@alurium.com>
Signed-off-by: Peter Amiri <petera@pai.com> <- mismatch
# ed4ce696d1 -- refactor(test): tighten browser-test artifact dir fix per review feedback
Author: Peter Amiri <peter@alurium.com>
Signed-off-by: Peter Amiri <petera@pai.com> <- mismatch
Commit f2890e9c5 (the most-recent code commit) already corrected this -- its sign-off reads peter@alurium.com -- but the two earlier commits were not amended to match. The DCO GitHub App is a required status check on every PR; it validates every commit, not just the tip. With two mismatched commits in the history the check will fail and block the merge.
Fix (one of):
- Rebase and amend
71816cc8anded4ce696to useSigned-off-by: Peter Amiri <peter@alurium.com>, or - Add
petera@pai.comas a verified email on the GitHub account so the DCO App accepts it.
Conventions
BrowserTestArtifactDirSpec.cfc lines 6-7: two-line comment block (minor nit)
// Regression for #2614 -- Adobe CF rejects directoryCreate(path, true);
// $ensureArtifactDir routes through java.io.File.mkdirs() instead.CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks -- one short line max." The content is a single thought and compresses cleanly:
// Regression for #2614: Adobe CF rejects directoryCreate(path, true); routes through File.mkdirs() instead.Not a blocker, but the prior review cycle already trimmed a 10-line block to reach this 2-line remnant -- finishing the trim is the obvious follow-up.
Cross-engine
No new cross-engine concerns. java.io.File.mkdirs() is JDK-level and works identically on Lucee, Adobe CF, and BoxLang -- confirmed by .ai/wheels/cross-engine-compatibility.md section "DirectoryCreate() Second Argument Is Lucee-Only" (lines 151-165), which prescribes this exact workaround. The single-argument DirectoryCreate(existingDir) used in the spec's setup is also Adobe-safe.
Tests
BrowserTestArtifactDirSpec extends wheels.WheelsTest (BDD syntax, not RocketUnit), covers the two meaningful paths (deeply-nested parents, already-exists no-op), uses CreateUUID() to avoid cross-run collisions, and cleans up via finally. The ##2614 escape in the it() label correctly avoids the "Invalid Syntax Closing [#] not found" crash documented in CLAUDE.md.
Commits
Three code commits. All subjects pass commitlint (types fix / refactor, scope test, headers 76/73/82 chars, not ALL-CAPS). DCO sign-off concern on two commits is noted above under Correctness.
…DE.md Reviewer A flagged the remaining 2-line block as one thought; folding it into a single line completes the trim from round 1. Signed-off-by: Peter Amiri <peter@alurium.com>
Wheels Bot — Reviewer B (round 1)A's review on SycophancyNone detected. A leads with the real blocking issue (DCO) before acknowledging the fix is correct. CHANGES_REQUESTED is consistent with the findings. False positivesNone detected. Both claims verified against the commit API:
Missed issuesTiming note only: the nit was resolved by the contributor before B ran Commit Verdict alignmentA's CHANGES_REQUESTED is consistent with the DCO finding. The DCO GitHub App validates every commit in the PR, not just the tip; two mismatched commits block the merge regardless of how clean the rest of the history is. ConvergenceAligned. A and B agree: the functional fix is correct, the comment nit has been resolved independently by the contributor, and the only outstanding item is the DCO email mismatch on |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new d4b11e3fa commit correctly resolves the last nit from round 3 — the 2-line spec comment is now a single line, matching CLAUDE.md's "one short line max" rule. The functional fix (routing directoryCreate(path, true) through java.io.File.mkdirs() in both BrowserTest and McpServer) remains correct, cross-engine clean, and well-tested. One blocker from rounds 2 and 3 is still unresolved: two of the four code commits carry a Signed-off-by email that does not match the git author email. CONTRIBUTING.md § DCO states the DCO GitHub App is a required status check that validates every commit; until those two commits are corrected, the merge is at risk.
Correctness
DCO sign-off mismatch — commits 71816cc8 and ed4ce696 (third consecutive round flagging this)
Both commits carry petera@pai.com in the trailer but peter@alurium.com as the author, unchanged from rounds 2 and 3:
# 71816cc854 — fix(test): route browser-test artifact dir creation through File.mkdirs()
Author: Peter Amiri <peter@alurium.com>
Signed-off-by: Peter Amiri <petera@pai.com> ← mismatch
# ed4ce696d1 — refactor(test): tighten browser-test artifact dir fix per review feedback
Author: Peter Amiri <peter@alurium.com>
Signed-off-by: Peter Amiri <petera@pai.com> ← mismatch
Commits f2890e9c5 and d4b11e3fa both use peter@alurium.com correctly — the fix is known. Per CONTRIBUTING.md § DCO:
The DCO GitHub App runs as a required status check on every PR. It verifies that every commit in the PR carries a valid Signed-off-by trailer and blocks merge if any are missing.
Fix (one of):
- Rebase and amend
71816cc8anded4ce696to useSigned-off-by: Peter Amiri <peter@alurium.com>, or - Add
petera@pai.comas a verified email on the GitHub account so the DCO App accepts it.
Commits
New commit d4b11e3fa (docs(test): collapse two-line regression comment to one line per CLAUDE.md): type docs, scope test, subject 58 chars, not ALL-CAPS — passes commitlint. DCO sign-off on this commit matches (peter@alurium.com). The mismatch concerns on 71816cc8 and ed4ce696 are noted under Correctness.
Cross-engine
No new cross-engine concerns. java.io.File.mkdirs() is JDK-level and works identically on Lucee, Adobe CF, and BoxLang. The single-argument DirectoryCreate(existingDir) used in the spec setup is also Adobe-safe.
Wheels Bot — Address Review (round 2)Reviewer A and Reviewer B converged on No commit pushed in this round:
Handing back to humans. The convergence loop will resume once a new SHA appears (Option B) or once the DCO check re-runs and passes (Option A); a fresh Reviewer A run will pick it up automatically. |
1 participant
Summary
vendor/wheels/wheelstest/BrowserTest.cfc:265andvendor/wheels/public/mcp/McpServer.cfc:1115both calleddirectoryCreate(path, true). Adobe ColdFusion rejects the Lucee-onlycreatePathflag withParameter validation error for the DIRECTORYCREATE function. The function takes 1 parameter., which crashed the core test suite on ACF whenever a browser spec failed and the artifact directory did not yet exist.Route the recursive directory creation through
java.io.File.mkdirs()— the same canonical engine-agnostic workaround already used byvendor/wheels/services/packages/ManifestCache.cfc::$ensureDirfrom #2567. The newBrowserTest.$ensureArtifactDir(path)helper centralizes the pattern for the browser-test base class so future call sites pick up the cross-engine-safe behavior automatically.Failing-then-passing spec
Added
vendor/wheels/tests/specs/wheelstest/BrowserTestArtifactDirSpec.cfc:The method $ensureArtifactDir does not exist.createPath=truewas meant to handle) and the already-exists no-op.Local run (Lucee 7 + SQLite):
wheels.tests.specs.wheelstest: 109 pass / 0 fail / 12 err (the 12 errors are pre-existingBrowserLoginSpec/BrowserRouteSpecconnection failures, unrelated to this change)Test plan
method $ensureArtifactDir does not existcompat-matrix.yml)Fixes #2614
🤖 Generated with Claude Code