docs(create-app): issue-resolution spec for local Docker start failure and the resulting permanent UVE 403 (#37262) - #37263
Merged
Conversation
14 tasks
fmontes
changed the title
Contributor
|
Claude finished @fmontes's task in 2m 8s —— View job Spec review — claims verified against the treeI checked every load-bearing claim in the spec against the working tree rather than re-reading the prose. The two prior reviews (zJaaal, nicobytes) are substantially incorporated already: the unawaited async moves now appear in Cause 3 / AC-008 / the P1 list; the OpenSearch "prior art" framing is withdrawn to "deliberate deviation from every non-migration example";
New Issues
Minor (non-blocking)
Verified cleanEverything else the spec cites holds against the tree:
One small factual addition on the last row: the shared Verdict: Approvable as a spec. The AC-012 over-claim is the only thing I'd want tightened before sign-off, since the spec is otherwise scrupulous about distinguishing verified-present from to-be-built and this one line breaks that discipline. It's a doc-only PR (610 additions, no code), so nothing here blocks the spec-kit gate on its own. |
3 tasks
fmontes
changed the title
Spec-only PR 1 for the create-app local-Docker failure. Covers the compose
ordering/restart defect, the transient UVE 403, and the CLI's discarding of
recoverable state.
Verified every claim in the issue against the tree before writing:
- single-node-demo-site compose: dotcms has no depends_on condition, no
restart policy, no healthcheck, and does not publish 8090; opensearch has
no healthcheck; db's healthcheck exists but nothing consumes it
- src/index.ts:370 exits before the scaffolding at :377
- src/index.ts:597 tests `if (!result)` against a truthy `{ ok: false, val }`
- checkPortsAvailability() hard-fails on the ports a successful run holds
- the package contains no spec files, so this establishes the harness
Defers two P2 items as explicit non-goals: the user.isAdmin() exception
swallowing (legacy Liferay, hot permission path, wide blast radius) and
image-tag pinning (intersects binding ADR-0019). Neither blocks the P0 fix.
Names compose reviewers from git blame, since .github/CODEOWNERS does not
cover docker/ and the runtime-fetched compose file is the highest-blast-radius
part of this change.
Refs #37262
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two strongest git-blame signals on the compose file — spbolton (dominant blame on every hunk in scope, and author of the lgtm-observability stack whose service_healthy pattern this change copies) and dcolina — are no longer collaborators on dotCMS/core, so GitHub rejects review requests for them. Replaces them with jcastro-dotcms (second-most-active docker/ contributor over the last 12 months) and records the resulting coverage gap explicitly: nobody currently assignable designed the pattern being copied, so the plan phase should read lgtm-observability/docker-compose.yml as the specification rather than rely on a reviewer to catch a faithful-copy error. Refs #37262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Planning verified the spec's claims against the tree and four did not hold. Correcting them here, before sign-off, so reviewers approve what we will actually build. - "Six siblings use condition: service_healthy" — there are three, and NONE gates dotcms on opensearch being healthy; all use service_started. Gating on both is therefore a deliberate deviation from every precedent, not the house pattern the spec implied. Kept, with the rationale stated and using os-migration's proven probe. - The stated risk that a bad healthcheck would make restart: unless-stopped flap the container cannot happen: Compose restart policies react to container exit, not health status. Replaced with the real exposure — a wrong probe blocks `docker compose up --wait` until timeout — and required an explicit --wait-timeout. - Publishing 8090 changed from "8090:8090, acceptable for a local stack" to "127.0.0.1:8090:8090". InfrastructureManagementFilter authorizes purely by arrival port: no credential check, no IP allowlist, so a wildcard binding puts /dotmgt/health and /dotmgt/metrics on the local network. Added AC-011. - The package has no spec files but the Jest harness already exists, so this fix adds specs rather than establishing a harness. Recorded pnpm install as a Red-gate prerequisite and warned that passWithNoTests makes an empty run green. Also adds AC-012 for a compose/CLI compatibility landmine found while reading: updateDockerComposeStarterUrl rewrites the file with a regex and throws on no match, so reformatting CUSTOM_STARTER_URL would break --starter for every installed CLI. Notes that .env is new behavior, not a restoration, and downgrades the /dotmgt/livez-on-latest assumption to partly-verified with a gating check. Two of the three open review questions are now settled or reframed. Refs #37262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… not a race Planning tested the spec's own hypothesis and disproved it. Correcting before sign-off so reviewers approve the real diagnosis. The spec claimed a transient startup race in which the CLI writes while roles and permissions are still settling, with user.isAdmin() swallowing an exception via Try.of(...).getOrElse(false). Two experiments (M5/64GB host, dotcms constrained to 2 CPUs / 4G) say otherwise. Clean boot has no settling window. The UVE endpoint is usable at 46s — two seconds BEFORE /dotmgt/readyz goes green — because the starter import (T+20s) and ES reindex (T+44s) both finish inside Tomcat startup and the connector accepts no traffic until after them. The hypothesised race cannot occur. The reporter's actual path does reproduce it, and permanently. Killing dotcms mid starter-import and hand-starting it (reproduction step 4) yields the reported log exactly — token 200, defaultSite 200, UVE 403 — and then 403 on 193 consecutive attempts over ~7 minutes with zero successes. The server says the admin user lacks READ permission on demo.dotcms.com: the interrupted import never wrote the site's permission rows, and the restart does not repair them. So cause 2 is a consequence of cause 1, not an independent defect, and fixing the compose file removes it. Design consequences, not just narrative: - AC-005 no longer polls until 200. A poll would never terminate; retry is restricted to 5xx and 403 explicitly does not retry. - On 403 the CLI must tell the user the instance is unrecoverable and to run `docker compose down -v` — offering manual UVE setup steps is wrong advice, since manual configuration fails identically. - The P2 backend non-goal is re-pointed: not isAdmin() exception swallowing, but the larger defect that any interrupted first boot silently bricks the instance while reporting a clean startup. Filed separately. Title and Reproducibility updated: the 403 is deterministic once the crash has happened, not timing-dependent. Refs #37262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AC-005 was corrected when the diagnosis changed, but the matching Fix Scope bullet was not — leaving the spec contradicting itself: one section said "poll GET until 200, retry on 401/403/5xx" while the other forbade exactly that. Caught by /speckit-analyze. Fix Scope now specifies a single probe, retry on 5xx only, and no retry on 403. Also splits the non-fatal UVE guidance in two, because the cases need opposite advice. On 403 the instance's permissions were never written, so manual UVE setup fails identically — pointing the user at the configuration guide would send them down a path that cannot work. That case tells them to recreate the instance and references #37268. Every other failure keeps the guide link plus host, site ID and app key. Refs #37262, #37268 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fix no longer modifies docker/docker-compose-examples/single-node-demo-site/docker-compose.yml. The CLI gets its own file, bundled in the npm package, and the shared example is left exactly as it is for README readers and installed CLIs. Reason: every hardening step this fix wanted was otherwise a behavior change shipped unversioned to consumers who never asked for it, because that file is fetched from main at runtime. Gating dotcms on opensearch health was the sharpest case — it introduces a way for dotCMS to never start if the probe later breaks (an opensearch:1 -> :2 bump invalidating admin:admin), where today it starts regardless. Owning the file makes strictness free. This removes what the spec itself called the single largest regression risk in the work. Two smaller risks replace it, both recorded: the bundled asset must be listed in package.json `files` AND project.json esbuild `assets` or it ships missing and every local-Docker run fails at step one (new AC-013); and strict gating means a future broken opensearch probe stops dotCMS starting, contained to this CLI's own stack. Accepted consequence: users on <=1.2.5 keep the old shared file and are not repaired. This starts fresh local instances rather than serving CI, no known users have it in CI, and npx resolves to the latest published version. AC-009 now requires continuous feedback for the whole wait, not just visible pull progress — ten minutes of frozen spinner is the failure this issue was reported for. AC-010 inverts to asserting docker/docker-compose-examples/* is UNCHANGED, verified by diff. Reviewer rationale updated: the blame-derived reviewers were chosen for a shared file this work no longer touches. Refs #37262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fmontes
force-pushed
the
issue-37262-create-app-docker-uve-spec
branch
from
August 28, 2026 20:38
09da5c7 to
5d4cf45
Compare
Review against the tree turned up five places where the spec and the code disagreed, and left four questions for the reviewer to decide. All are now resolved in the spec so PR 1 is approving what will actually be built. Decisions: * start_period is 180s, not the two conflicting values the spec carried (120s in Fix Scope, 180s in the review question). ~4x the measured ~46s boot and above both in-repo precedents. * The stricter opensearch: service_healthy gate is kept, using single-node-os-migration's proven probe. A credential-free variant was considered and rejected: the admin:admin coupling is contained by the major-version tag pin, and a proven probe beats an unproven one on the critical path. * The two duplicated UVE call sites collapse into one configureUVE() owner that contains no process.exit, rather than being patched in place. * AC-012 is enforced by a Jest spec against the real bundled asset. The scripts/verify-cold-start.sh reference is removed - that file does not exist and nothing tasked its creation. Corrections found while applying them: * Two wrong premises about start_period. A too-short window does not make `docker compose up --wait` block until timeout; it marks the container unhealthy and makes --wait abort early, abandoning an instance that would have been healthy, and restart: unless-stopped cannot rescue it because restart policies react to exit, not health. This is why overshooting start_period is free. * The 403 guidance cannot be shared between the two paths. On the local stack it is the bricked boot and `docker compose down -v` is the fix; on a user-supplied server there is no stack to recreate and a 403 means the token lacks permission on the site, where manual UVE setup does work. configureUVE() now takes a mode and AC-005 specifies both messages. * The compose verification method targeted the shared single-node-demo-site file that AC-010 requires to be unchanged, and which has no dotcms healthcheck for --wait to assess. Retargeted at the bundled asset. * Sibling counts corrected: four files use condition: service_healthy and three gate dotcms directly. single-node-os-migration gates dotcms on OpenSearch health transitively via provision jobs, so the stricter gate has prior art here and is not the clean break from precedent the spec claimed. One item remains open and is blocking: /dotmgt/livez is unconfirmed on the released dotcms/dotcms:latest image, and the whole compose design depends on it. Refs #37262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…scope The blame-derived reviewer list was selected against the shared single-node-demo-site/docker-compose.yml, back when this work modified that file. It no longer does: the compose file that ships lives in core-web/libs/sdk/create-app/assets/ and is reviewed as SDK code with the rest of the CLI change. With no infrastructure change to gate on its owners, the list is dropped rather than carried as courtesy CCs. Two points from that section survive on their own merits and are kept: * .github/CODEOWNERS covers neither docker/ nor the SDK tree, so PR 2 draws no automatic reviewer and one has to be requested by hand. * lgtm-observability/docker-compose.yml should be read directly as the reference for correct condition: service_healthy usage - its author is no longer a collaborator, so the file is the specification. It binds 8090 on the wildcard, which this design deliberately does not copy. Section renamed from "Required reviewers for the compose change" to "Compose design decisions". The four settled decisions and the /dotmgt/livez confirmation are unchanged. Refs #37262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both were written into the spec during review and both turned out to be wrong. Correcting them in the spec rather than leaving the implementation to silently disagree with it. AC-012 described scripts/verify-cold-start.sh as a phantom reference that nothing created, and replaced it with a Jest guard. The script exists. It ships in this package, it was already written against the bundled asset rather than the shared example, and it already carries a --static mode that runs the config-only checks with no Docker daemon - measured at 9 passed / 0 failed in 0.3s. The claim was made after checking only the spec branch, where the file genuinely is absent because it lands one PR up the stack. What WAS wrong is the path: the script is under the package's own scripts/, not the repository root. Both guards are now named, because they assert different things and neither subsumes the other: --static checks that the FILE still matches the shape installed CLIs depend on, while the Jest spec runs applyStarterUrl() and checks the FUNCTION's output. Keeping only one leaves a real gap. The image-tag deferral cited ADR-0019 as the reason it "deserves its own decision". Reading ADR-0019 in full, that is backwards: the ADR's motivating problem is precisely a client pinned to `latest` against a mismatched instance producing a cryptic runtime failure, and under date-lockstep the SDK version IS the release version - so it supplies the tag to pin rather than complicating the choice. The deferral still stands, but on scope: `latest` is what ships today, so leaving it preserves the status quo rather than introducing new drift inside a bug fix. Refs #37262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nicobytes
reviewed
Sep 1, 2026
nicobytes
left a comment
nicobytes
left a comment
Member
There was a problem hiding this comment.
Two questions from a /speckit-clarify ambiguity pass over the spec. Both are gaps that change task decomposition or test design, so they are worth settling before /speckit-plan output is treated as final — I checked #37264 and neither is resolved there. Details inline on the two ACs they affect.
Member
Review: spec is approvable, with one missed defect and two claims to softenI checked the spec's claims against the tree rather than just reading it. Everything cited verifies. Details below, in the order I'd act on them. 1. Missed defect: two unawaited async calls (
|
zJaaal
previously approved these changes
Sep 1, 2026
Four findings from @nicobytes and @zJaaal, all verified against the tree before acting. --wait was listed under "CLI (P1)" while AC-001 and AC-002 are P0 and have no other stated mechanism (@nicobytes). Shipping P0 without it would leave both unsatisfiable. Promoted to P0, and AC-009's continuous feedback travels with it for the same reason - a ten-minute silent wait is the symptom this issue was reported for. AC-009 previously carried no tier tag at all. Also stated explicitly, since the spec never did: all P1 items are in scope for this fix, not a follow-up; the tiers say what must land for the fix to be coherent versus what makes it good, not how it is split into PRs. AC-013 had no verification method (@nicobytes), which the spec itself calls the most likely way to break the release. The gate is now named, and named carefully: it asserts the BUILD OUTPUT, not the source tree. Checking the two manifests is necessary but not sufficient - a wrong `output:` in the esbuild assets entry satisfies a manifest check and still puts the file where the package does not carry it. Likewise a Jest spec or `verify-cold-start.sh --static` resolving the asset relative to src/ passes identically whether or not it ever ships, which is precisely the failure AC-013 exists to catch. The gate is a post-build assertion over dist/ plus `npm pack --dry-run` tarball contents. The OpenSearch "prior art" framing was mine and it was wrong (@zJaaal). single-node-os-migration is the tester harness for the unreleased OpenSearch 3.x migration; its gate exists so provision jobs create search users before dotCMS connects, not as boot ordering. All three non-migration examples use `opensearch: service_started`. So the stricter gate IS a deliberate deviation from every non-migration precedent, and calling it "prior art, one step removed" made a deviation sound like house practice. Withdrawn. (The reviewer also notes that file pins opensearch:1.3.20 while the demo stack floats on :1, so "proven in single-node-os-migration" was proven against a different image - the independent ~15s measurement on this stack is what actually carries the probe.) Cause 3 said moveDockerComposeOneLevelUp() "calls process.exit(1) internally" (@zJaaal). It does not - the exits are in startScaffoldingFrontEnd at :588 and :599. Conclusion unaffected, attribution corrected. The same finding surfaced a real defect the spec had missed: both moves are `async` and called WITHOUT await, so the rename may not have landed before the clone runs against a directory that must be empty. try/finally does not fix a floating promise. AC-008 now requires the awaits as well. Still open for a decision, not actioned here: @zJaaal asks whether a one-line `db: condition: service_healthy` should land in the shared single-node-demo-site example after all. AC-010 freezes it, which leaves every CLI <=1.2.5 user and every README reader on the broken file indefinitely, and that one change carries none of the risk the rescope removed. Refs #37263, #37262 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fmontes
added a commit
that referenced
this pull request
Sep 1, 2026
) Closes the gap @nicobytes raised on #37263: AC-013 is the one criterion the spec itself calls "the most likely way to break the release", and nothing actually enforced it. What existed asserted the wrong thing. packaging.spec.ts reads package.json and project.json from the SOURCE TREE, and verify-cold-start.sh --static resolves the asset relative to src/. Both pass identically whether or not the file ever ships. A wrong `output:` in the esbuild assets entry satisfies every one of them and still publishes a package with no compose file - and then every local-Docker run fails at its first step, which is precisely what AC-013 exists to prevent. The packaging had in fact been verified once, by hand, with npm pack --dry-run; there was no gate. scripts/verify-package.sh asserts the artifact instead: * the compose asset is in dist/libs/sdk/create-app at the path the CLI resolves at runtime (resolveComposeSource walks up from the bundle entry, so it must sit beside index.js exactly as it does in the source tree), and * npm pack --dry-run lists it in the tarball contents. Those are two independent failures and both are checked, because either alone ships a broken package: esbuild copies the file but `files` omits it, or `files` is right and the copy never happened. Confirmed to FAIL on both modes rather than assumed: removing dist/assets fails both checks; restoring the asset and reverting `files` to its pre-fix ["*.js", "README.md"] passes the first and fails the second, naming package.json. Wired as the nx target `verify-package` with dependsOn build, deliberately NOT folded into `test` - tests must stay fast and must not require a build. Also tags T052 with AC-011. That criterion (8090 answers on loopback, refused on the LAN address) was already enforced by verify-cold-start.sh's T007 check but referenced by no task, so the only security criterion in the list read as uncovered. Acceptance-criteria coverage is now 13/13. 144 tests, 11 suites; build, lint and both verification scripts clean. Refs #37262, #37263 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fmontes
added a commit
that referenced
this pull request
Sep 1, 2026
Neither script ran in CI. The Maven build drives nx with exactly four invocations - `nx affected -t lint`, `nx format:check`, `nx run-many -t build`, `nx affected -t test` - so `verify-package` was never reached, and verify-cold-start.sh was not an nx target at all. Both were manual, which for AC-013 in particular means the criterion the spec calls "the most likely way to break the release" had a gate nobody ran. Both now hang off this project's `test` target, so `nx affected -t test` picks them up. That keeps the change inside libs/sdk/create-app/project.json rather than editing core-web/pom.xml, which every project in the monorepo shares. New target `verify-compose-static` runs verify-cold-start.sh --static: the config-only half, no Docker, ~0.3s. It is what catches a reformatted CUSTOM_STARTER_URL line silently breaking --starter for every installed CLI. The runtime half (cold start, restart recovery, LAN exposure) needs a real daemon and a multi-minute starter import and stays manual. Both targets get `cache: true` with narrow inputs - the compose asset, the manifests and the scripts themselves - so they do not re-run on unrelated edits. Measured: 4s cold, 1s warm, against 144 Jest tests. This reverses the "keep tests fast and buildless" call made when verify-package was added; that rule earns its keep on a large Angular library, not on a CLI whose build is 1.3s. Verified the chain actually fails rather than assumed: reverting `files` in package.json to its pre-fix ["*.js", "README.md"] fails the packaging gate and aborts the run before Jest executes. Refs #37262, #37263 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zJaaal
approved these changes
Sep 1, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 1, 2026
fmontes
added a commit
that referenced
this pull request
Sep 1, 2026
) Closes the gap @nicobytes raised on #37263: AC-013 is the one criterion the spec itself calls "the most likely way to break the release", and nothing actually enforced it. What existed asserted the wrong thing. packaging.spec.ts reads package.json and project.json from the SOURCE TREE, and verify-cold-start.sh --static resolves the asset relative to src/. Both pass identically whether or not the file ever ships. A wrong `output:` in the esbuild assets entry satisfies every one of them and still publishes a package with no compose file - and then every local-Docker run fails at its first step, which is precisely what AC-013 exists to prevent. The packaging had in fact been verified once, by hand, with npm pack --dry-run; there was no gate. scripts/verify-package.sh asserts the artifact instead: * the compose asset is in dist/libs/sdk/create-app at the path the CLI resolves at runtime (resolveComposeSource walks up from the bundle entry, so it must sit beside index.js exactly as it does in the source tree), and * npm pack --dry-run lists it in the tarball contents. Those are two independent failures and both are checked, because either alone ships a broken package: esbuild copies the file but `files` omits it, or `files` is right and the copy never happened. Confirmed to FAIL on both modes rather than assumed: removing dist/assets fails both checks; restoring the asset and reverting `files` to its pre-fix ["*.js", "README.md"] passes the first and fails the second, naming package.json. Wired as the nx target `verify-package` with dependsOn build, deliberately NOT folded into `test` - tests must stay fast and must not require a build. Also tags T052 with AC-011. That criterion (8090 answers on loopback, refused on the LAN address) was already enforced by verify-cold-start.sh's T007 check but referenced by no task, so the only security criterion in the list read as uncovered. Acceptance-criteria coverage is now 13/13. 144 tests, 11 suites; build, lint and both verification scripts clean. Refs #37262, #37263 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fmontes
added a commit
that referenced
this pull request
Sep 1, 2026
Neither script ran in CI. The Maven build drives nx with exactly four invocations - `nx affected -t lint`, `nx format:check`, `nx run-many -t build`, `nx affected -t test` - so `verify-package` was never reached, and verify-cold-start.sh was not an nx target at all. Both were manual, which for AC-013 in particular means the criterion the spec calls "the most likely way to break the release" had a gate nobody ran. Both now hang off this project's `test` target, so `nx affected -t test` picks them up. That keeps the change inside libs/sdk/create-app/project.json rather than editing core-web/pom.xml, which every project in the monorepo shares. New target `verify-compose-static` runs verify-cold-start.sh --static: the config-only half, no Docker, ~0.3s. It is what catches a reformatted CUSTOM_STARTER_URL line silently breaking --starter for every installed CLI. The runtime half (cold start, restart recovery, LAN exposure) needs a real daemon and a multi-minute starter import and stays manual. Both targets get `cache: true` with narrow inputs - the compose asset, the manifests and the scripts themselves - so they do not re-run on unrelated edits. Measured: 4s cold, 1s warm, against 144 Jest tests. This reverses the "keep tests fast and buildless" call made when verify-package was added; that rule earns its keep on a large Angular library, not on a CLI whose build is 1.3s. Verified the chain actually fails rather than assumed: reverting `files` in package.json to its pre-fix ["*.js", "README.md"] fails the packaging gate and aborts the run before Jest executes. Refs #37262, #37263 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wezell
pushed a commit
that referenced
this pull request
Sep 3, 2026
) Closes the gap @nicobytes raised on #37263: AC-013 is the one criterion the spec itself calls "the most likely way to break the release", and nothing actually enforced it. What existed asserted the wrong thing. packaging.spec.ts reads package.json and project.json from the SOURCE TREE, and verify-cold-start.sh --static resolves the asset relative to src/. Both pass identically whether or not the file ever ships. A wrong `output:` in the esbuild assets entry satisfies every one of them and still publishes a package with no compose file - and then every local-Docker run fails at its first step, which is precisely what AC-013 exists to prevent. The packaging had in fact been verified once, by hand, with npm pack --dry-run; there was no gate. scripts/verify-package.sh asserts the artifact instead: * the compose asset is in dist/libs/sdk/create-app at the path the CLI resolves at runtime (resolveComposeSource walks up from the bundle entry, so it must sit beside index.js exactly as it does in the source tree), and * npm pack --dry-run lists it in the tarball contents. Those are two independent failures and both are checked, because either alone ships a broken package: esbuild copies the file but `files` omits it, or `files` is right and the copy never happened. Confirmed to FAIL on both modes rather than assumed: removing dist/assets fails both checks; restoring the asset and reverting `files` to its pre-fix ["*.js", "README.md"] passes the first and fails the second, naming package.json. Wired as the nx target `verify-package` with dependsOn build, deliberately NOT folded into `test` - tests must stay fast and must not require a build. Also tags T052 with AC-011. That criterion (8090 answers on loopback, refused on the LAN address) was already enforced by verify-cold-start.sh's T007 check but referenced by no task, so the only security criterion in the list read as uncovered. Acceptance-criteria coverage is now 13/13. 144 tests, 11 suites; build, lint and both verification scripts clean. Refs #37262, #37263 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wezell
pushed a commit
that referenced
this pull request
Sep 3, 2026
Neither script ran in CI. The Maven build drives nx with exactly four invocations - `nx affected -t lint`, `nx format:check`, `nx run-many -t build`, `nx affected -t test` - so `verify-package` was never reached, and verify-cold-start.sh was not an nx target at all. Both were manual, which for AC-013 in particular means the criterion the spec calls "the most likely way to break the release" had a gate nobody ran. Both now hang off this project's `test` target, so `nx affected -t test` picks them up. That keeps the change inside libs/sdk/create-app/project.json rather than editing core-web/pom.xml, which every project in the monorepo shares. New target `verify-compose-static` runs verify-cold-start.sh --static: the config-only half, no Docker, ~0.3s. It is what catches a reformatted CUSTOM_STARTER_URL line silently breaking --starter for every installed CLI. The runtime half (cold start, restart recovery, LAN exposure) needs a real daemon and a multi-minute starter import and stays manual. Both targets get `cache: true` with narrow inputs - the compose asset, the manifests and the scripts themselves - so they do not re-run on unrelated edits. Measured: 4s cold, 1s warm, against 144 Jest tests. This reverses the "keep tests fast and buildless" call made when verify-package was added; that rule earns its keep on a large Angular library, not on a CLI whose build is 1.3s. Verified the chain actually fails rather than assumed: reverting `files` in package.json to its pre-fix ["*.js", "README.md"] fails the packaging gate and aborts the run before Jest executes. Refs #37262, #37263 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Spec-only PR (PR 1 of 2) for #37262, per the repo's spec-kit flow. No code changes — this PR is the approval gate for the approach. Implementation lands in the stacked PR on top of this branch.
specs/37262-create-app-docker-uve/spec.md— an issue-resolution spec for the@dotcms/create-applocal-Docker failure.specify/feature.jsonat the new featureWhat's broken (three compounding defects):
single-node-demo-site/docker-compose.ymlletsdotcmsboot before Postgres accepts connections and gives it no restart policy, so it exits and stays exited. The CLI reports "containers started successfully" and burns its retry budget on a dead container. Users get past this only by pressing ▶ in Docker Desktop.POSTthen returns a transient 403 and the CLI callsprocess.exit(1). Because UVE setup runs before scaffolding, the user is left with an empty directory and the working token and site ID are discarded without ever being printed.docker-compose.ymlneeded to tear it down.Verified against the tree before writing — every claim in the issue holds:
dotcmshasdepends_onwith nocondition, norestart:, no healthcheck, and does not publish 8090;opensearchhas no healthcheck;db's healthcheck exists but nothing consumes itsrc/index.ts:370exits before the scaffolding at:377src/index.ts:597testsif (!result)against a truthy{ ok: false, val }, making the npm-install failure branch unreachablecheckPortsAvailability()(src/utils/index.ts:479) hard-fails on exactly the ports a successful run holdsDeliberately deferred as explicit non-goals, so the P0 fix stays bounded:
user.isAdmin()swallowing exceptions into a false "not admin" — real defect, but legacy Liferay code on a hot permission path with wide blast radius, and the P0 fix doesn't depend on itCUSTOM_STARTER_URL— intersects binding ADR-0019 (date-lockstep SDK versioning), so it deserves its own decision rather than riding along in a bug fixReviewers — and two questions for you
.github/CODEOWNERSdoes not coverdocker/, so the highest-blast-radius file in this fix gets no automatic reviewer. Reviewers below are derived fromgit blameon the exact hunks the implementation will change, weighting design ownership over line count (the two largest raw counts are a bulk restore, #27432, and a pgvector bump, #29915 — both mechanical).db/opensearchshape we're adding healthchecks to. Most senior still-active owner ofdocker/.dotcmsservice (#36490); closest to theCUSTOM_STARTER_URL/ starter-import behavior the readiness race depends ondocker/over the last 12 months.Blame's two strongest signals can't be assigned.
spboltonholds dominant blame on every hunk in scope and authoredlgtm-observability(#32980) — the exactcondition: service_healthypattern this change copies — anddcolinaholds part of theopensearchblock (#29915). Both are no longer collaborators on this repo (last commits 2026-03-30 and 2026-04-07), so GitHub rejects the review request. Flagging it rather than quietly dropping them: the person who designed the pattern we're copying is not available to check that we're copying it faithfully. If anyone has context on the lgtm-observability healthcheck choices, that's the gap to fill.Experiment 1 — a clean boot has no settling window. Measured on an M5/64GB host with dotCMS constrained to 2 CPUs / 4G:
POST api-tokenGET apps/dotema-config-v2/{site}/dotmgt/livez,/dotmgt/readyz/api/v1/appconfiguration(current CLI probe)The UVE endpoint is usable two seconds before
readyzgoes green. The starter import (T+20s) and ES reindex (T+44s) both complete inside Tomcat startup, and the connector accepts no traffic until after them. The hypothesised race cannot happen.Experiment 2 — the reporter's actual path reproduces it, permanently. Killing dotCMS mid starter-import and hand-starting it (reproduction step 4):
The reported log, line for line. Server-side:
The interrupted import never wrote the site's permission rows; the restart re-runs
Task00004LoadStarter, reports a clean startup, and the permissions never appear. The instance does not recover.What changed in the spec as a result:
5xx; 403 explicitly does not retry.docker compose down -v.user.isAdmin()swallowing an exception (there is nothing to swallow — the data is absent), but a larger defect: any interrupted first boot silently bricks the instance while reporting success. Filed separately as An interrupted first boot silently bricks the instance: starter import leaves site permissions unwritten and every Apps API call 403s forever #37268.This raises US1's value. Fixing the compose file doesn't just stop a crash — it removes the 403 entirely, because it removes the interrupted boot that causes it.
Evidence limits: one host, one starter, one image, kill point fixed at 25s. Which kill-points corrupt is unmapped.
Updated after planning. Verifying the spec's claims against the tree turned up four that didn't hold, and I've folded the corrections back in before sign-off so you're approving what will actually be built:
condition: service_healthy" — there are three, and none of them gatesdotcmson OpenSearch being healthy (all useservice_started). So doing both is a deliberate deviation from every precedent, not the house pattern the spec implied. Kept, with the reasoning stated, usingsingle-node-os-migration's proven probe.docker compose up --waituntil timeout.8090:8090→127.0.0.1:8090:8090.InfrastructureManagementFilterauthorizes purely by arrival port — no credential check, no IP allowlist — so a wildcard binding puts/dotmgt/healthand/dotmgt/metricson the local network. New AC-011 asserts it's refused on the LAN address.Also added AC-012 for a landmine found while reading:
updateDockerComposeStarterUrlrewrites the compose file with a regex and throws on no match, so reformattingCUSTOM_STARTER_URLwould break--starterfor every already-installed CLI, with no release able to reach them.What's left for you to settle (the 8090 question is now answered above):
dotcmsonopensearch: service_healthygoes further than all four existing examples. Right call for a stack driven by an unattended CLI, or should it match the house pattern? (My recommendation: keep the stricter gate.)start_period: 180senough for a cold demo-starter import on a slow machine? Above both precedents (lgtm 120s, metrics-monitoring 20s). Too short and--waitblocks to timeout.Note on blast radius
The compose file is fetched from
mainat runtime by every installed@dotcms/create-app. A change ships instantly and unversioned to all existing CLI users — including older versions that won't know about port 8090. That cuts both ways: it's why the fix is P0 and reaches users without a release, and it's the single largest regression risk here. Worth a careful look.Checklist
Additional Info
CLAUDE.md:/speckit-specify-fix→ PR 1 approved →/speckit-plan→/speckit-tasks→/speckit-implement→ PR 2before_planhook. ADR-0019 (accepted, binding) is recorded in the spec; ADR-0016 matched on keywords but is an unfilled template and imposes nothing. No conflicts with an accepted ADR.@dotcms/create-appCLI #35096 (E2E suite for@dotcms/create-app) — owns the fault-injection coverage, out of scope hereRefs #37262
This PR fixes: #37262
This PR fixes: #37262