feat: L-FRESH-BOOT fresh-user smoke + isolated dev-server helper#1506
Open
ystknsh wants to merge 11 commits into
Open
feat: L-FRESH-BOOT fresh-user smoke + isolated dev-server helper#1506ystknsh wants to merge 11 commits into
ystknsh wants to merge 11 commits into
Conversation
…ion static path Default static path (`<server>/../client/`) resolves correctly only in the prepared package layout that `bin/prepare-dist.js` copies `dist/client/` into. When running the server straight from a source checkout (fresh-user smoke tests spawn `tsx server/index.ts` without that copy step), the path resolves to a non-existent `<repo-root>/client/`. The new `MULMOCLAUDE_CLIENT_DIR` env lets callers point the production static path at any directory — fresh-boot tests use it to target `<repo-root>/dist/client/`. Empty string is treated as "not set" so the default keeps working without ceremony.
…-server helper
`spawnIsolatedDevServer` (e2e-live/fixtures/isolated-dev-server.ts)
boots a fully isolated `tsx server/index.ts` subprocess with four
overrides:
- HOME=<temp-home> -> claude CLI auth lookup, ~/.claude/skills/
- MULMOCLAUDE_WORKSPACE_PATH=<temp-ws> -> workspace dir creation
- PORT=<random-free> -> no collision with `yarn dev`
- MULMOCLAUDE_AUTH_TOKEN=<pinned hex> -> bearer token known up-front
`~/.claude/.credentials.json` is copied into the test HOME so the
spawned claude CLI subprocess has auth. `DISABLE_SANDBOX=1` and
`NODE_ENV=production` are set: the spec is about boot path / first-run
UX, not sandbox semantics (L-FRESH-SANDBOX-BUILD covers the Docker
path separately) and production mode makes express serve index.html
itself (with the bearer token substituted into the meta tag).
`fresh-boot.spec.ts` (L-FRESH-BOOT) drives the spawn helper and asserts
5 layers of first-run integrity:
(a) /api/health responds 200 (re-issued with the pinned bearer so the
trace shows the green light verbatim)
(b) workspace auto-init created `conversations/chat`, `config/helps`,
and `.session-token`
(c) GET / serves index.html with the `<meta name="mulmoclaude-auth">`
token substituted (placeholder must be gone)
(d) one chat turn (`Reply with the single word: okfresh-<nonce>`)
completes — assistant body contains the marker, thinking
indicator clears
(e) AFTER server stop, host `~/mulmoclaude/` and `~/.claude/skills/`
mtimes match the pre-spawn baseline — the strongest isolation
contract; any stray `homedir()` call that bypassed the env
override surfaces here
`yarn test:e2e:live:fresh-boot` runs it standalone. Wall time ~15-19s
on warm cache (server boot ~5s + 1 LLM turn ~5s + Vite client build
amortised across runs). CI no-LLM matrix is intentionally not updated
because the spec self-spawns its server (same pattern as docker.spec.ts).
…lated-server helper Update the implementation status table, the 2026-05-23 re-eval matrix, the implementation order roadmap, and the open TODO list to reflect L-FRESH-BOOT landing along with the reusable `spawnIsolatedDevServer` helper that L-10 / L-13 / L-FRESH-* can extend rather than reinventing. `docs/e2e-live-testing.md` gets a one-paragraph callout describing the fresh-user smoke exception to the "shared E2E_LIVE_BASE_URL server" rule, so future spec authors know when self-spawning is the acceptable pattern versus when they should stick with the shared boot.
…e-live Capture the design follow-up surfaced in PR review: `spawnIsolatedDevServer` could be promoted to a suite-level Playwright `webServer` / `globalSetup` spawn so the entire e2e-live suite stops depending on the developer's `yarn dev` and stops touching the host `~/mulmoclaude/`. Records the trade-offs (one-time boot cost, loss of host workspace coverage, dual configs for dev / pre-release modes, Docker matrix switch shape) and explicitly notes L-FRESH-BOOT must stay per-test spawn because its "empty workspace on every run" assertion would be weakened by a suite-shared isolated server.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an isolated dev-server test fixture, integrates client-dir override into server startup, introduces Playwright fresh-boot smoke test and drift/unit tests for host snapshot/assertion, updates docs/plans, and adds a package script to run the fresh-boot e2e spec. ChangesFresh Boot E2E Test Implementation
🎯 4 (Complex) | ⏱️ ~40 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant Fixture as spawnIsolatedDevServer
participant Builder as ensureClientBuilt
participant Env as buildServerEnv
participant Child as tsx_server_index
participant Health as /api/health
Test->>Fixture: spawnIsolatedDevServer(testId, opts)
Fixture->>Builder: ensureClientBuilt()
Fixture->>Fixture: probeFreePort(), snapshotHostFs(), makeTestDirs()
Fixture->>Env: buildServerEnv(...)
Fixture->>Child: spawn tsx server/index.ts (env, pinned token)
Fixture->>Child: forwardChildLogs()
Fixture->>Health: waitForHealth(baseUrl, authToken)
Health-->>Fixture: HTTP 200 OK
Fixture-->>Test: return IsolatedServerHandle (baseUrl, token, baselines, _process)
Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * isolation contract is broken (probably a stray `homedir()` call | ||
| * that bypassed the env override). | ||
| */ | ||
| export async function assertHostUntouched(baselines: readonly HostMtimeBaseline[]): Promise<void> { |
There was a problem hiding this comment.
assertHostUntouched currently snapshots only the directory mtimes of ~/mulmoclaude and ~/.claude/skills. That misses an important contamination class: writes to existing files under those directories (for example ~/mulmoclaude/config/settings.json), because file-content updates do not change the parent directory mtime. So this can report "host untouched" even when host files were modified. Can we make the baseline recursive (or at least include critical file mtimes/hashes) so the isolation guarantee matches the comment?
|
CODEX VERDICT: CHANGES REQUESTED
|
…branches The MULMOCLAUDE_CLIENT_DIR env override branch was reachable end-to-end via L-FRESH-BOOT (the spec helper sets the env), but the DEFAULT branch (env unset / empty string — what every `npx mulmoclaude` user hits) had no automated coverage. A future refactor that accidentally changed the default path would silently break every production install while every test (which sets the env) still passes. Extract the resolver into a pure function `resolveClientDir(baseDir, envValue)` in server/utils/clientDir.ts and unit-test five branches: - env non-empty -> verbatim (override) - env undefined -> <baseDir>/../client (default, what npx users hit) - env empty string -> falls back to default - env relative path -> verbatim (no auto-absolutize) - env whitespace-only -> verbatim (intentional, length > 0 check) server/index.ts now delegates to the resolver; behavior unchanged.
| export async function assertHostUntouched(baselines: readonly HostMtimeBaseline[]): Promise<void> { | ||
| const drift: string[] = []; | ||
| for (const baseline of baselines) { | ||
| const current = await snapshotMtime(baseline.path); |
There was a problem hiding this comment.
assertHostUntouched still compares only the top-level directory mtimes. That can false-pass when a file under those trees is modified in place (for example ~/mulmoclaude/config/settings.json), because file-content writes do not update the parent directory mtime. Since this helper is the main proof of host isolation, can we make this check recursive (or snapshot a critical-file set) so contamination is reliably detected?
|
CODEX VERDICT: CHANGES REQUESTED
|
ystknsh
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
e2e-live/fixtures/isolated-dev-server.ts (1)
182-212: ⚡ Quick winRefactor oversized helpers to satisfy the 20-line function limit.
buildServerEnv(starts at Line 182) andspawnIsolatedDevServer(starts at Line 224) exceed the configured cap. Please split setup steps (env inheritance, token/bootstrap prep, child spawn/wait lifecycle) into smaller functions.As per coding guidelines,
Keep functions under 20 lines; split into smaller functions if needed.Also applies to: 224-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-live/fixtures/isolated-dev-server.ts` around lines 182 - 212, buildServerEnv and spawnIsolatedDevServer are too large; split them into smaller helpers so each function stays under 20 lines. Extract the environment construction into two functions (e.g., buildInheritedEnv(process.env) that returns Record<string,string> and buildOverrides(env: ServerEnv) that returns the fixed keys like HOME, PORT, MULMOCLAUDE_AUTH_TOKEN, DISABLE_SANDBOX), move token/bootstrap preparation into a prepareAuthAndBootstrap(env: ServerEnv) helper, and move child process creation and lifecycle/wait logic from spawnIsolatedDevServer into spawnAndAwaitChild(command, args, options). Replace the original code by composing these helpers (buildInheritedEnv + buildOverrides => buildServerEnv, prepareAuthAndBootstrap before spawnAndAwaitChild) so each named function (buildServerEnv, spawnIsolatedDevServer, buildInheritedEnv, buildOverrides, prepareAuthAndBootstrap, spawnAndAwaitChild) is small and focused.test/server/utils/test_clientDir.ts (1)
25-66: ⚡ Quick winSplit the
describecallback into smaller helpers to meet the function-length cap.On Line 25, the suite callback is currently long; moving repeated assertions into tiny helper functions keeps the body under the 20-line threshold and makes failures easier to scan.
As per coding guidelines,
Keep functions under 20 lines; split into smaller functions if needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/server/utils/test_clientDir.ts` around lines 25 - 66, The describe("resolveClientDir", ...) callback is too long; refactor the test by extracting repeated assertions into small helper functions (e.g., assertEnvVerbatim, assertFallbackDefault, assertWhitespaceHandled) and call those helpers from the describe block so the suite callback body stays under 20 lines; keep references to resolveClientDir and SAMPLE_BASE_DIR/EXPECTED_DEFAULT in the helpers and preserve the same assertions and messages so behavior and test semantics remain unchanged.e2e-live/tests/fresh-boot.spec.ts (1)
36-127: ⚡ Quick winBreak the main test body into small exported step helpers.
On Line 36, this callback now packs multiple independent phases (health, workspace init, auth meta validation, one-turn chat, teardown checks). Splitting them into exported helpers will satisfy the 20-line cap and improve test reuse/readability.
As per coding guidelines,
Keep functions under 20 lines; split into smaller functions if neededandExtract pure logic into exported helpers for testability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-live/tests/fresh-boot.spec.ts` around lines 36 - 127, The test callback in the "L-FRESH-BOOT" test is too large and should be split into small exported step helpers: extract the health check, workspace-init assertions, auth-meta validation, SPA one-turn chat (new-session, send, assert response using getCurrentSessionId and marker), and teardown/host-checks (stopIsolatedDevServer + assertHostUntouched) into separate exported async functions (e.g., checkHealth, verifyWorkspaceInit, validateAuthMeta, runOneTurnChat, finalizeAndAssertHostUntouched) and call them from the test body; keep each exported helper under 20 lines, accept the server/page/testInfo or specific params they need, and reuse existing helpers like spawnIsolatedDevServer/stopIsolatedDevServer/getCurrentSessionId to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e-live/fixtures/isolated-dev-server.ts`:
- Around line 182-212: buildServerEnv and spawnIsolatedDevServer are too large;
split them into smaller helpers so each function stays under 20 lines. Extract
the environment construction into two functions (e.g.,
buildInheritedEnv(process.env) that returns Record<string,string> and
buildOverrides(env: ServerEnv) that returns the fixed keys like HOME, PORT,
MULMOCLAUDE_AUTH_TOKEN, DISABLE_SANDBOX), move token/bootstrap preparation into
a prepareAuthAndBootstrap(env: ServerEnv) helper, and move child process
creation and lifecycle/wait logic from spawnIsolatedDevServer into
spawnAndAwaitChild(command, args, options). Replace the original code by
composing these helpers (buildInheritedEnv + buildOverrides => buildServerEnv,
prepareAuthAndBootstrap before spawnAndAwaitChild) so each named function
(buildServerEnv, spawnIsolatedDevServer, buildInheritedEnv, buildOverrides,
prepareAuthAndBootstrap, spawnAndAwaitChild) is small and focused.
In `@e2e-live/tests/fresh-boot.spec.ts`:
- Around line 36-127: The test callback in the "L-FRESH-BOOT" test is too large
and should be split into small exported step helpers: extract the health check,
workspace-init assertions, auth-meta validation, SPA one-turn chat (new-session,
send, assert response using getCurrentSessionId and marker), and
teardown/host-checks (stopIsolatedDevServer + assertHostUntouched) into separate
exported async functions (e.g., checkHealth, verifyWorkspaceInit,
validateAuthMeta, runOneTurnChat, finalizeAndAssertHostUntouched) and call them
from the test body; keep each exported helper under 20 lines, accept the
server/page/testInfo or specific params they need, and reuse existing helpers
like spawnIsolatedDevServer/stopIsolatedDevServer/getCurrentSessionId to
preserve behavior.
In `@test/server/utils/test_clientDir.ts`:
- Around line 25-66: The describe("resolveClientDir", ...) callback is too long;
refactor the test by extracting repeated assertions into small helper functions
(e.g., assertEnvVerbatim, assertFallbackDefault, assertWhitespaceHandled) and
call those helpers from the describe block so the suite callback body stays
under 20 lines; keep references to resolveClientDir and
SAMPLE_BASE_DIR/EXPECTED_DEFAULT in the helpers and preserve the same assertions
and messages so behavior and test semantics remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0999de2-a7cb-4ca9-9cd9-175274acb307
📒 Files selected for processing (8)
docs/e2e-live-testing.mde2e-live/fixtures/isolated-dev-server.tse2e-live/tests/fresh-boot.spec.tspackage.jsonplans/feat-e2e-live.mdserver/index.tsserver/utils/clientDir.tstest/server/utils/test_clientDir.ts
Codex GHA (PR #1506 iter-1 + iter-2 both repeated the same finding): `assertHostUntouched` only checked top-level directory mtimes (`~/mulmoclaude`, `~/.claude/skills`), so an in-place rewrite of an existing file under either tree (e.g. `config/settings.json`) would leave the parent dir mtime unchanged and the assertion would false- pass — the very isolation guarantee it exists to prove. Switch `snapshotMtime` / `snapshotHostBaselines` to a recursive walk that captures every descendant entry's mtime into a flat `Map<absolutePath, mtimeMs>`, and rewrite `assertHostUntouched` to diff baseline vs current three ways (modified mtime, created entry, removed entry). Also surface absent→present existence flips as a distinct failure mode. Drift output is capped at 20 entries (with a "...and N more" tail) so a runaway leak does not produce an unreadable trace. Add `test/e2e-live/test_isolated_dev_server_drift.ts` covering the four detection shapes against a real tmpdir fixture — including the specific in-place rewrite case Codex flagged. The regression net will fail immediately if a future refactor re-introduces shallow mtime checking. Known false-positive risk documented inline: an external process mutating the host roots concurrently (developer's own `yarn dev`, editor autosave) will surface as drift. Acceptable because L-FRESH- BOOT's target environments are CI and dev-server-off local runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex flagged two follow-up correctness gaps in the recursive walk introduced by 9477c9b, then approved after the fix: iter 2 — [must-fix] symlink-to-dir recursion gap: - walkMtimeTree gated recursion on entry.isDirectory() (Dirent type, returns false for symlink-to-dir), so in-place rewrites under a symlinked subtree of ~/.claude/skills/ or ~/mulmoclaude/ were still missed. Switched to stat(full).isDirectory() which follows the symlink target. iter 2 — [must-fix] global dev:ino dedupe caused false drift: - The cycle guard added in iter 1 (dedup by dev:ino) was too aggressive: two sibling symlinks pointing at the same real dir would have only ONE alias traversed, in a readdir-order-dependent way. Replaced with a per-branch ancestor-realpath chain: a child is skipped only when its realpath matches an ancestor in the current recursion stack (true cycle), not when any other branch has previously visited the same inode. iter 2 — [question] EACCES/EPERM tolerance: - Extended per-entry error tolerance from ENOENT-only to {ENOENT, EACCES, EPERM} so symlinks into restricted trees (/etc, foreign home, unreadable mount) skip the subtree rather than aborting the walk. Documented the blind spot explicitly in iter 3 (POSIX write permission is independent of read permission, so the assertion is "untouched where readable" not "untouched everywhere"). Defensive: MAX_WALK_DEPTH = 64 belt-and-braces guard against pathological chains the realpath check might miss. Helpers split into readEntries / visitEntry / safeRealpath to stay under the project's cognitive-complexity threshold. Unit tests added: - follows symlinked directories so in-place rewrites under them are detected - records both aliases when two symlinks point at the same real dir (no global inode dedupe) - does not loop on a symlink cycle (ancestor-realpath guard) Codex iter 3 final verdict: APPROVED (no blocking findings). Local checks: yarn format / lint / typecheck:e2e-live / 13 unit tests / L-FRESH-BOOT e2e all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CODEX VERDICT: LGTM Re-checked the current diff against prior thread history. The earlier Codex concerns about top-level-only mtime checks are resolved in the latest patch via recursive host snapshots ( I did not find additional correctness, security, accessibility, i18n-lockstep, or test-coverage issues that remain outstanding in this PR. |
…ad of (baseDir, envValue) Review feedback on PR #1506: the previous signature took a `baseDir` argument that the resolver silently discarded under the override branch — a parameter that one of two execution paths ignores is a smell that hides the conditional dependency at the call site. Move the default-path construction to the caller and have the resolver pick between two ready values (env override vs caller- provided default). Both arguments now flow into the output symmetrically; reading the signature tells you exactly what each argument is for. // Before const clientDir = resolveClientDir(__dirname, process.env.MULMOCLAUDE_CLIENT_DIR); // After const clientDir = resolveClientDir( process.env.MULMOCLAUDE_CLIENT_DIR, path.join(__dirname, "../client"), ); Unit tests updated to the new signature; added one extra case (`does not touch defaultDir when env is set`) that pins the intended symmetry with a sentinel `defaultDir` value so a future regression to the old "argument-ignored" shape fails immediately. Local checks: yarn format / lint / typecheck:server+test / 6 unit tests / L-FRESH-BOOT e2e all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CODEX VERDICT: LGTM Re-checked the current diff against prior review history.
Note: I could not execute the new tests locally in this environment because |
|
CODEX VERDICT: LGTM Re-reviewed the latest patch set and cross-checked prior thread history.
Note: I could not execute the new tests locally in this environment because |
Contributor
Author
|
これで、実装はシンプルなままにしつつ、 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/server/utils/test_clientDir.ts (1)
6-20: ⚡ Quick winAdd regression coverage for whitespace-only and relative env values.
This suite currently misses two valuable branches: whitespace-only env input and relative path passthrough.
Proposed test additions
describe("resolveClientDir", () => { const DEFAULT_DIR = "/abs/repo/client"; + it("uses env value when relative", () => { + assert.equal(resolveClientDir("dist/client", DEFAULT_DIR), "dist/client"); + }); + it("uses env value when non-empty", () => { assert.equal(resolveClientDir("/custom/client", DEFAULT_DIR), "/custom/client"); }); @@ it("falls back to default when env is empty string", () => { assert.equal(resolveClientDir("", DEFAULT_DIR), DEFAULT_DIR); }); + + it("falls back to default when env is whitespace-only", () => { + assert.equal(resolveClientDir(" ", DEFAULT_DIR), DEFAULT_DIR); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/server/utils/test_clientDir.ts` around lines 6 - 20, Add two test cases for resolveClientDir: one verifying that a whitespace-only env string (e.g., " ") is treated as empty and falls back to DEFAULT_DIR, and another verifying that a relative path env value (e.g., "./client" or "client") is passed through unchanged. Locate the test suite for resolveClientDir in test_clientDir.ts and add two it blocks that call resolveClientDir with the whitespace-only string and with a relative path, asserting the expected fallback to DEFAULT_DIR for the former and exact passthrough for the latter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/utils/clientDir.ts`:
- Line 2: The current fallback uses envValue || defaultDir which treats
whitespace-only strings as valid; change the logic to trim envValue before
deciding to fall back by computing a trimmed value (e.g., const trimmed =
envValue?.trim()) and return trimmed if non-empty, otherwise return defaultDir;
reference the existing envValue and defaultDir variables to locate and replace
the return expression.
---
Nitpick comments:
In `@test/server/utils/test_clientDir.ts`:
- Around line 6-20: Add two test cases for resolveClientDir: one verifying that
a whitespace-only env string (e.g., " ") is treated as empty and falls back to
DEFAULT_DIR, and another verifying that a relative path env value (e.g.,
"./client" or "client") is passed through unchanged. Locate the test suite for
resolveClientDir in test_clientDir.ts and add two it blocks that call
resolveClientDir with the whitespace-only string and with a relative path,
asserting the expected fallback to DEFAULT_DIR for the former and exact
passthrough for the latter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5370c2c-09dd-4e4a-b69c-fdb704aa8c2b
📒 Files selected for processing (3)
server/index.tsserver/utils/clientDir.tstest/server/utils/test_clientDir.ts
| @@ -0,0 +1,4 @@ | |||
| export function resolveClientDir(envValue: string | undefined, defaultDir: string): string { | |||
| if (typeof envValue === "string" && envValue.trim().length > 0) return envValue; | |||
There was a problem hiding this comment.
resolveClientDir validates envValue.trim().length > 0 but returns the original untrimmed string. If someone sets MULMOCLAUDE_CLIENT_DIR with accidental surrounding whitespace (e.g. " /repo/dist/client "), this passes the guard but express.static() gets a path with spaces and fails to resolve the client bundle. Returning the trimmed value here would make this branch robust to that misconfiguration.
| try { | ||
| await waitForHealth(baseUrl, authToken); | ||
| } catch (err) { | ||
| await killChild(child); |
There was a problem hiding this comment.
spawnIsolatedDevServer allocates a temp root (mkdtemp) before waiting for health, but on boot failure this catch only kills the child and rethrows. Because no handle is returned, callers cannot invoke stopIsolatedDevServer, so failed boots leak temp directories under os.tmpdir(). Consider cleaning up the temp root in this failure path as well (similar to stopIsolatedDevServer) so repeated failures do not accumulate orphaned dirs.
|
CODEX VERDICT: CHANGES REQUESTED
Prior |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/utils/clientDir.ts`:
- Line 2: The validation currently checks typeof envValue === "string" and
envValue.trim().length > 0 but returns the untrimmed envValue; change the return
to the trimmed string (e.g., return envValue.trim()) so callers (functions using
envValue with express.static/path.join) receive a normalized path; locate the
check in the function that validates environment client directory (the envValue
branch) and replace the returned value accordingly.
In `@test/server/utils/test_clientDir.ts`:
- Around line 20-23: Add a test to assert behavior when env contains a non-empty
value with leading/trailing whitespace (e.g., " /custom/client ") to document
whether resolveClientDir should trim or preserve whitespace; update
test_clientDir.ts to call resolveClientDir with that string and assert against
the expected outcome (using DEFAULT_DIR and resolveClientDir function names to
locate the code), and if the intended behavior is to trim, adjust
server/utils/clientDir.ts (resolveClientDir) to trim env values before returning
so the test passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48734884-1562-4f95-b4e6-0dc210bebddf
📒 Files selected for processing (2)
server/utils/clientDir.tstest/server/utils/test_clientDir.ts
|
CODEX VERDICT: LGTM Re-checked the current diff against prior thread history.
I did not find additional outstanding issues in correctness, security (XSS/SSRF/path traversal/prompt injection), accessibility, i18n lockstep impact, or test coverage for the new behavior. Note: I could not run the added tests locally here because |
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
1 participant
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.
Summary
e2e-live/fixtures/isolated-dev-server.tsのspawnIsolatedDevServer)~/mulmoclaude/を一切汚さず e2e-live を走らせる (= e2e-live suite 全体の self-contained 化、 plan で TODO 化済)検証する 5 段 (L-FRESH-BOOT spec assertion):
/api/healthが 200 応答conversations/chat,config/helps,.session-token)<meta name="mulmoclaude-auth">token が注入Reply with the single word: okfresh-<nonce>) を assistant body が echo~/mulmoclaude//~/.claude/skills/の mtime が test 前と変わらない (recursive walk で in-place 上書きまで検出)Items to Confirm / Review
reviewer に重点的に見てほしい設計判断:
server/index.tsにMULMOCLAUDE_CLIENT_DIRenv を読む分岐を追加。 用途は「test spawner がdist/client/を指すため」 のみで、 production user (npx mulmoclaude tarball) は env 未設定 → default の<__dirname>/../client/を踏むので動作変化なし。 ただし「test のために production code を触っている」 構造であることは事実。 代替案 (worktree への symlink、 prepare-dist 起動) も検討したが、 副作用最小の env seam を採用。 →resolveClientDirを pure function に切り出し、 env set / undefined / 空文字列 / 相対 path / whitespace-only の 5 ブランチを unit test (test/server/utils/test_clientDir.ts) で被覆。 default 経路 (npx mulmoclaude ユーザーが実際に踏む) の退行は unit test で防御buildServerEnvが parentprocess.envを全 inherit — spawned isolated server は親 shell のGEMINI_API_KEY/ANTHROPIC_API_KEY/X_BEARER_TOKEN等を継承する。 厳密な「fresh user」 simulation ではない (= 開発者 shell の export 状態に左右される)。 ただしPATH不在でtsxが起動しない等を避けるため一律 strip はしていないDISABLE_SANDBOX=1を helper が常に set — fresh-boot は boot path 連動性のみを検証、 sandbox 経路は L-FRESH-SANDBOX-BUILD (別 PR、 未着手) で扱う前提assertHostUntouchedは top-level dir mtime のみ/codex-cross-review3 ラウンドで以下を実装 (commits9477c9b3+b6a90637): (a) 再帰 walk で全 descendant の mtime をMap<absolutePath, mtimeMs>に snapshot、 (b) symlinked dir も follow (stat().isDirectory()判定)、 (c) ancestor-realpath chain で cycle 検出 (aliased symlink は両方 walk される)、 (d)MAX_WALK_DEPTH=64safety net、 (e)EACCES/EPERM/ENOENTを tolerate、 (f) drift は modified / created / removed / existence-flip の 4 種を双方向 diff。test/e2e-live/test_isolated_dev_server_drift.tsで 8 ケース被覆 (in-place rewrite / aliased symlink / cycle / removed / created / existence-flip 等)fresh-boot.spec.tsは自前 server を spawn する構造のため、MULMOCLAUDE_FAKE_AGENT=1で起動する shared dev server を前提とした既存 matrix に乗らない。 docker.spec.ts と同じ位置付けyarn build:clientを helper が自動起動 —dist/client/index.html不在時のみ。 cold cache で +10-20s、 warm では no-opUser Prompt
L-FRESH-BOOT (新規ユーザー smoke) を実装。重要度 A (first-run UX = ユーザー獲得関門)。test 専用 dev server spawn infra (HOME + MULMOCLAUDE_WORKSPACE_PATH + MULMOCLAUDE_PORT で隔離 + 認証 copy) を含む。launcher 抽象化を最小限で済ませて 1 PR に収める方針、 後の L-10 / L-13 / L-FRESH-* で同 infra を再利用する。
実装の詳細
コミット構成
525b43edserver/index.tsにMULMOCLAUDE_CLIENT_DIRenv を読む分岐を追加946c6133spawnIsolatedDevServerhelper +fresh-boot.spec.ts+ package.json script96f4488377f4beccaabbbe75resolveClientDirを pure function に切り出し + unit test 5 ブランチHelper の env 設定 (
buildServerEnv)HOME=<temp>~/.claude/skills/を temp dir に振るMULMOCLAUDE_WORKSPACE_PATH=<temp>PORT=<random>yarn dev(3001) との衝突回避MULMOCLAUDE_AUTH_TOKEN=<random hex>NODE_ENV=productionMULMOCLAUDE_CLIENT_DIR=<repo>/dist/clientDISABLE_SANDBOX=1設計指針 (plan の階層モデル)
plans/feat-e2e-live.md の「環境を壊さず再現する設計指針」 より:
MULMOCLAUDE_CLIENT_DIRの追加が該当 (最小限)後続シナリオでの再利用設計
env引数で extend (Gemini key 削除 / server restart trigger)MULMOCLAUDE_SANDBOX_IMAGEenv 新設 (別 PR)globalSetup/webServerで 1 回 spawn する形に格上げすれば全 e2e-live が host 無汚染になる構想 (plans に TODO 化、 別 PR)動作確認
ローカル (macOS, host dev = port 3001 起動中) で実行:
```bash
yarn test:e2e:live:fresh-boot
```
yarn devと非衝突~/mulmoclaude//~/.claude/skills/mtime 不変unit test:
```bash
./node_modules/.bin/tsx --test test/server/utils/test_clientDir.ts
```
CI チェック:
yarn format/yarn lint/yarn typecheck/yarn build全通過。関連
Summary by CodeRabbit
Documentation
Tests
Chores
Improvements