Conversation
📝 WalkthroughWalkthroughAdds a Git worktrees feature across server, service, client, types, stores, tests, and docs: new worktree service (list/resolve/create/remove), unified API route (GET/POST/DELETE), Zod schemas, ErrorCodes, UI WorktreeSwitcher and chat integration, session/workspace key changes to include worktree, broad plumbing to thread worktree through file/image/stream flows, and many tests/docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser
participant API as /api/worktrees
participant Auth as Auth Service
participant Service as Worktree Service
participant Git as Git Repo
Client->>API: POST /api/worktrees (workspace, slug, branch, from)
API->>Auth: getSessionUser()
Auth-->>API: session | 401
API->>Auth: verifyWorkspaceAccess(workspace)
Auth-->>API: allowed | 401
API->>Service: createWorktree(baseWorkspacePath, slug, branch, from, requestId)
Service->>Service: validate & normalize slug/branch, assert base repo
Service->>Service: acquire lock (.git/bridge-worktree.lock) -> 409 on contention
Service->>Git: git worktree add / git ops
Git-->>Service: success (path, head)
Service->>Service: release lock
Service-->>API: CreateWorktreeResult (slug, branch, worktreePath)
API-->>Client: 201 Created { slug, branch, worktreePath }
sequenceDiagram
participant Chat as Chat Page
participant Hook as useWorkspace
participant Store as WorkspaceStore
participant URL as Browser URL
Chat->>Hook: init (read wt param)
Hook->>Store: get currentWorktree for workspace
Hook->>Store: buildWorkspaceKey(workspace, worktree)
Hook-->>Chat: { workspace, worktree, workspaceKey, setWorktree }
Chat->>URL: sync wt param on change
Chat->>Hook: setWorktree(slug)
Hook->>Store: setCurrentWorktree(workspace, slug) -> persist
Store-->>Hook: persisted
Hook-->>Chat: updated worktree applied to UI and API calls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@apps/web/app/api/worktrees/__tests__/route.test.ts`:
- Around line 6-29: Remove the vi.mock calls that stub
"@/features/auth/lib/auth" (getSessionUser, verifyWorkspaceAccess),
"@/features/chat/lib/workspaceRetriever" (getWorkspace), and
"@/features/worktrees/lib/worktrees" (listWorktrees, createWorktree,
removeWorktree) and convert the tests to integration-style: import and use real
test fixtures/helpers to create a real workspace/user and real worktree records,
call the route handlers (or HTTP endpoints) to exercise database state, assert
on real DB changes, include descriptive test names, ensure each test sets up and
tears down its data via provided test helpers (or beforeEach/afterEach) and add
tests for error paths (auth failure, create/remove failures).
In `@apps/web/app/api/worktrees/route.ts`:
- Around line 45-59: Replace direct session and workspace checks that use
getSessionUser() and verifyWorkspaceAccess() with the mandated API helpers: call
getCookieUserId() to obtain and validate the user id (return
createErrorResponse(ErrorCodes.NO_SESSION, 401, { requestId }) if
missing/invalid), then call isWorkspaceAuthenticated(userId, workspace) to
authorize the workspace (return
createErrorResponse(ErrorCodes.WORKSPACE_NOT_AUTHENTICATED, 401, { requestId,
workspace }) if it fails). Ensure you still validate workspace from new
URL(req.url).searchParams and return WORKSPACE_MISSING when empty, and keep the
same ErrorCodes and requestId usage so downstream logic remains unchanged.
- Around line 103-110: The POST handler currently calls verifyWorkspaceAccess
and returns a 401 for empty/whitespace workspaces; before calling
verifyWorkspaceAccess (after body = parsed), validate body.workspace by checking
if typeof body.workspace !== 'string' or body.workspace.trim() === '' and if so
return createErrorResponse(ErrorCodes.WORKSPACE_MISSING, 400, { requestId,
workspace: body.workspace }); keep the rest of the flow (isHandleBodyError,
verifyWorkspaceAccess) unchanged so genuine auth failures still return 401.
In `@apps/web/features/worktrees/lib/worktrees.ts`:
- Around line 365-371: The while loop that checks gitRefExists using a suffix
from formatTimestampUTC() can spin if the timestamp granularity doesn't change;
update the branch-name collision logic in the block that references
gitRefExists, baseWorkspacePath and branchName so the suffix always makes
progress — e.g., append a high-resolution value (Date.now()) or a monotonic
counter to the suffix (increment a local counter each iteration) instead of
relying solely on formatTimestampUTC(); ensure the final branchName is set to
`${branchName}${suffix}` after the loop exits.
- Around line 109-183: The code performs fs operations on baseWorkspacePath,
worktreeRoot and lockPath without validating they are inside the allowed
workspace; add calls to isPathWithinWorkspace() to guard against path traversal
by validating paths before any fs.* use. Specifically: in assertBaseRepo(),
ensureFromRef(), ensureWorktreeRoot(), and withWorktreeLock() call
isPathWithinWorkspace(baseWorkspacePath) at the start and throw a WorktreeError
(e.g., "WORKTREE_INVALID_PATH") if it fails; in ensureWorktreeRoot() also
validate the computed worktreeRoot before mkdirSync/chownSync/statSync; in
withWorktreeLock() validate the lockPath before openSync/unlinkSync and the
baseWorkspacePath before statSync/fchownSync/writeSync to ensure every
filesystem call is preceded by the path guard.
- Around line 351-388: The pre-lock existence check using listWorktrees(...)
must be moved inside the withWorktreeLock(...) callback to avoid a TOCTOU race:
remove the top-level existing = await listWorktrees(...) block and instead, at
the start of the async function passed to withWorktreeLock(baseWorkspacePath,
async () => { ... }), call const existing = await
listWorktrees(baseWorkspacePath) and if (existing.some(item => item.slug ===
normalizedSlug)) throw new WorktreeError("WORKTREE_EXISTS", `Worktree already
exists: ${normalizedSlug}`); keep the rest of the logic (baseRef, branchName
handling, gitRefExists, runGit, return object) unchanged so the existence check
is performed while the worktree lock is held.
- Around line 411-423: The call to remove the worktree uses
runGit(baseWorkspacePath, ["worktree", "remove", targetPath]) but does not pass
--force when allowDirty is true, so allowDirty has no effect; update the
worktree removal call in worktrees.ts (the runGit invocation that uses
baseWorkspacePath and targetPath) to include the "--force" flag when allowDirty
is true (e.g., choose ["worktree","remove","--force",targetPath] vs
["worktree","remove",targetPath]) so dirty worktrees are removed as intended;
keep the existing behavior when allowDirty is false.
In `@docs/architecture/workspace-isolation.md`:
- Around line 25-27: The `.env` entry is misindented and currently appears
outside the example.com tree; move the `.env` line so it is a sibling of
`worktrees/` and `user/` under `example.com/` (i.e., align its
tree-prefix/indentation to match `worktrees/` and `user/` in the block
containing `example.com/`, `worktrees/`, and `feature-x/`) so the visual tree
shows `.env` as part of the example.com directory.
In `@docs/features/worktrees.md`:
- Around line 12-17: The fenced code block that displays the path layout is
missing a language specifier; update the block delimiter from ``` to ```text so
the snippet becomes a language-specified code block (i.e., change the opening
fence for the path layout block to ```text) to satisfy markdown linting and
enable proper syntax highlighting for the path listing.
In `@docs/prs/worktrees/stage-2-server-api.md`:
- Around line 1-75: The markdown titled "Stage 2 - Server API" is placed outside
the mandated docs tree; relocate this document into the project's docs hierarchy
under an appropriate category (e.g., features or archive) so it follows the docs
organization, and update any docs index/sidebar references; ensure the file
keeps the same content and headers (e.g., "Stage 2 - Server API", "POST
/api/worktrees", "GET /api/worktrees", "DELETE /api/worktrees") so links and
references remain valid.
In `@docs/prs/worktrees/stage-3-workspace-resolution.md`:
- Around line 13-17: The documentation incorrectly references
ensurePathWithinWorkspace; update the text to reference and require
isPathWithinWorkspace() instead, and explicitly state that callers must call
isPathWithinWorkspace() (the path-validation helper) before any file operation
and use realpath and git worktree list --porcelain as described; replace
“ensurePathWithinWorkspace” with “isPathWithinWorkspace()” and add the
coding-guideline note to use isPathWithinWorkspace() in all .ts files to prevent
path traversal.
- Around line 1-27: The markdown file "stage-3-workspace-resolution.md" titled
"Stage 3 - Workspace Resolution and Tooling" is in the wrong docs location; move
this document into an allowed docs area (for example under the features or
archive sections) so it follows the required docs organization; update any
internal links or TOC entries that reference the old location and ensure the new
path lives under the docs directory hierarchy (e.g., docs/features/worktrees or
docs/archive/prs) so it complies with the docs/**/*.md structure and
README/GETTING_STARTED/architecture/etc. organization rules.
In `@docs/prs/worktrees/stage-4-client-ui.md`:
- Around line 1-41: The "Stage 4 - Client UI and Session Scoping" markdown
(stage-4-client-ui.md) is placed outside the required docs structure; move the
file into the organized docs tree under an allowed area (for example
features/worktrees or archive/prs within the docs directory), update any docs
index/README links that reference it, and ensure the file follows docs naming
conventions (top-level README/GETTING_STARTED/architecture/features structure)
so it resides under the proper docs hierarchy.
🧹 Nitpick comments (4)
docs/prs/worktrees/stage-1-foundations.md (1)
63-67: Minor inconsistency:forcevsallowDirtynaming.Line 65 mentions
forcefor allowing dirty worktree removal, but the implementation inworktrees.tsusesallowDirtyas the parameter name. Consider updating the design doc to match the implementation for consistency.docs/prs/worktrees/stage-5-testing.md (1)
3-9: Consider adding explicit path traversal test to API tests.The Workspace Resolution Tests section covers traversal rejection, but based on learnings, API route tests should also explicitly verify path traversal protection at the HTTP layer. Consider adding a test case like "400 on traversal attempt in slug" to the API Tests section for completeness.
apps/web/features/worktrees/lib/__tests__/worktrees.test.ts (1)
45-205: Consider adding tests forWORKTREE_EXISTSandWORKTREE_NOT_FOUNDerror codes.The test suite covers most error codes but appears to be missing explicit tests for:
WORKTREE_EXISTS: Creating a worktree with a slug that already existsWORKTREE_NOT_FOUND: Resolving or removing a worktree that doesn't existThese are documented in
docs/features/worktrees.mdand implemented in the service layer.docs/features/worktrees.md (1)
8-8: Minor grammar: consider hyphenating "checked-out".When "checked out" is used as a compound adjective before a noun ("checked-out branches"), it's typically hyphenated. This is a minor style suggestion.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web/features/chat/lib/__tests__/workspace-naming-regression.test.ts (1)
23-38:⚠️ Potential issue | 🟡 MinorAdd
isPathWithinWorkspace()validation before file access.
existsSyncis used ondotsPath/hyphensPathwithout path validation. Please guard file operations withisPathWithinWorkspace()to satisfy the workspace traversal protection requirement. As per coding guidelines: Always use path validation with isPathWithinWorkspace() before any file operation to prevent path traversal attacks.apps/web/features/chat/components/sandbox/FileTree.tsx (1)
125-138:⚠️ Potential issue | 🟠 MajorMissing
worktreeprop when rendering TreeNode.
TreeNodeaccepts aworktreeprop (line 145), but it's not passed when rendered fromTreeLevel(lines 126-137). This will cause nested tree levels to lose the worktree context, resulting in incorrect cache keys and API calls.🐛 Proposed fix
{files.map(item => ( <TreeNode key={item.path} workspace={workspace} + worktree={worktree} item={item} depth={depth} isActive={activeFile === item.path}apps/web/app/api/images/delete/route.ts (1)
9-77:⚠️ Potential issue | 🟠 MajorWrite mandatory tests for this new route covering authentication (401), happy path, and error cases.
This new route requires test coverage for:
- Authentication: missing/invalid session returns 401
- Happy path: successful image deletion
- Error cases: invalid key (400), unauthorized tenant (403), missing image (404), deletion failures (500)
Since
resolveWorkspace()supports optionalworktreeparameter (like list and upload routes), also test that the route correctly handles worktree in the request body if workspace isolation is intended for image operations.apps/web/features/chat/hooks/useStreamReconnect.ts (1)
147-148:⚠️ Potential issue | 🟡 Minor
pollForRemainingMessagescapturesworktreefrom closure but isn't passed as parameter.The function is called with
(tabId, workspace)but usesworktreefrom the outer scope. Ifworktreechanges while polling is active, the poll will use the new value mid-poll. Consider passingworktreeas a parameter for consistency withworkspace:Suggested fix
- } else if (data.state === "streaming") { - // Stream is still active - mark it and set up polling - streamingActions.startStream(tabId) - pollForRemainingMessages(tabId, workspace) + } else if (data.state === "streaming") { + // Stream is still active - mark it and set up polling + streamingActions.startStream(tabId) + pollForRemainingMessages(tabId, workspace, worktree || undefined) }And update the function signature:
const pollForRemainingMessages = useCallback( - async (pollTabId: string, ws: string) => { + async (pollTabId: string, ws: string, wt: string | undefined) => {
🤖 Fix all issues with AI agents
In `@apps/web/app/api/claude/stream/cancel/route.ts`:
- Around line 133-135: The custom message passed in
apps/web/app/api/claude/stream/cancel/route.ts is being dropped by
createErrorResponse, which removes the message field before calling
getErrorMessage; modify createErrorResponse (in features/auth/lib/auth.ts) to
preserve and forward an explicit message field (or merge it into the fields
passed to getErrorMessage) so that when route.ts calls
createErrorResponse(ErrorCodes.INVALID_REQUEST, 400, { message: "Invalid
worktree slug..." }) the resulting payload includes that exact message; update
createErrorResponse and its use of getErrorMessage (and any tests relying on
data.message) so the provided message is propagated instead of being stripped.
In `@apps/web/components/workspace/WorktreeSwitcher.tsx`:
- Around line 234-254: The three text inputs in WorktreeSwitcher (the inputs
bound to slug/setSlug, branch/setBranch, and from/setFrom) lack accessible
labels; add either visible <label> elements linked via id/htmlFor or add
descriptive aria-label attributes for each input (e.g., "Slug", "Branch", "From
branch") and ensure ids match when using labels, and keep placeholders as
supplemental hints rather than the sole accessible name.
In `@apps/web/lib/error-codes.ts`:
- Around line 13-26: Add a new WORKTREE_GIT_FAILED entry alongside the other
WORKTREE_* keys (e.g., next to WORKTREE_NOT_GIT, WORKTREE_NOT_FOUND) in the
error-codes list, and add corresponding user-facing text in the error message
and help lookup tables referenced elsewhere in this file (the blocks around the
other WORKTREE messages at the ranges you noted). Specifically, insert
WORKTREE_GIT_FAILED into the error code enum/object and add a clear
ERROR_MESSAGES["WORKTREE_GIT_FAILED"] and ERROR_HELP["WORKTREE_GIT_FAILED"] (or
whatever message/help maps exist at the other two blocks) so API/UI surfaces a
dedicated message and help text instead of falling back to the default.
🧹 Nitpick comments (13)
apps/web/app/api/images/upload/route.ts (1)
29-36: Consider logging the worktree parameter for debugging consistency.The
worktreeParamis extracted but not included in the log statement on Line 36, which could make debugging worktree-related issues harder.Also, the nested ternary for
bodyconstruction is somewhat hard to follow. Consider a more explicit approach:♻️ Proposed refactor for readability
const workspaceParam = formData.get("workspace") as string | null const worktreeParam = formData.get("worktree") as string | null - const body = - workspaceParam && worktreeParam - ? { workspace: workspaceParam, worktree: worktreeParam } - : workspaceParam - ? { workspace: workspaceParam } - : {} - console.log(`[Image Upload ${requestId}] Workspace param: ${workspaceParam || "(none)"}`) + const body: { workspace?: string; worktree?: string } = {} + if (workspaceParam) { + body.workspace = workspaceParam + if (worktreeParam) { + body.worktree = worktreeParam + } + } + console.log(`[Image Upload ${requestId}] Workspace param: ${workspaceParam || "(none)"}, worktree: ${worktreeParam || "(none)"}`)apps/web/features/auth/lib/sessionStore.ts (1)
55-61: Consider validating the worktree segment format.When parsing a 5-segment key, if the third segment doesn't start with
wt/, the code returns it as-is without validation or warning. This could silently accept malformed keys.🛡️ Proposed defensive validation
return { userId, workspaceDomain: workspaceDomain || "default", - worktree: maybeWorktreeOrTabGroup?.startsWith("wt/") ? maybeWorktreeOrTabGroup.slice(3) : maybeWorktreeOrTabGroup, + worktree: maybeWorktreeOrTabGroup?.startsWith("wt/") + ? maybeWorktreeOrTabGroup.slice(3) + : (() => { + console.warn(`[SessionStore] Expected wt/<slug> segment, got: "${maybeWorktreeOrTabGroup}"`) + return maybeWorktreeOrTabGroup + })(), tabGroupId: maybeTabGroupOrTabId, tabId: maybeTabId ?? "", }apps/web/features/chat/components/sandbox/hooks/useFileContent.ts (1)
95-109: Cache invalidation may not clear all worktree entries for a workspace.When calling
invalidateFileContentCache(workspace)without specifyingworktree, the function only clears entries wherescope === "base"(line 101-103). This means worktree-specific cache entries (wt/{slug}) remain cached.If the intent is to invalidate all cache entries for a workspace regardless of worktree, the logic should iterate over all keys starting with
${workspace}::instead of including the scope in the prefix match.Is this the intended behavior? If you want to clear all entries for a workspace:
♻️ Suggested fix to invalidate all scopes for a workspace
if (workspace) { - const scope = worktree ? `wt/${worktree}` : "base" + // If worktree is explicitly provided, only clear that scope + // If worktree is undefined, clear all entries for the workspace + const prefix = worktree !== undefined + ? `${workspace}::${worktree ? `wt/${worktree}` : "base"}::` + : `${workspace}::` for (const key of fileContentCache.keys()) { - if (key.startsWith(`${workspace}::${scope}::`)) { + if (key.startsWith(prefix)) { fileContentCache.delete(key) } } return }apps/web/features/chat/components/sandbox/FileTree.tsx (1)
231-245: Same cache invalidation scope behavior asuseFileContent.ts.The invalidation logic here mirrors
useFileContent.ts- when onlyworkspaceis provided, it only clears entries for the specific scope (base or worktree). Consider whether both caches should invalidate all scopes when worktree is undefined, or document this as intentional behavior.apps/web/app/api/images/list/route.ts (1)
33-38: Consider simplifying the conditional body construction.The nested ternary is functionally correct but could be more readable.
♻️ Optional: Simplified body construction
- const body = - workspaceParam && worktreeParam - ? { workspace: workspaceParam, worktree: worktreeParam } - : workspaceParam - ? { workspace: workspaceParam } - : {} + const body: { workspace?: string; worktree?: string } = {} + if (workspaceParam) body.workspace = workspaceParam + if (worktreeParam) body.worktree = worktreeParamapps/web/features/workspace/lib/__tests__/worktree-utils.test.ts (1)
1-59: Good test coverage for worktree utilities.The tests cover the essential paths including normalization, validation of valid/invalid slugs, reserved word rejection, and workspace key building/parsing. The test for
foo::barrejection (Line 34) is particularly important for preventing session key corruption.Consider adding edge case tests for completeness:
🧪 Optional: Additional edge case tests
it("rejects invalid or reserved worktree slugs", () => { // ... existing tests ... + + // Rejects slugs exceeding max length (49 chars) + const tooLong = validateWorktreeSlug("a".repeat(50)) + expect(tooLong.valid).toBe(false) + + // Accepts max length slug + const maxLength = validateWorktreeSlug("a".repeat(49)) + expect(maxLength.valid).toBe(true) }) + + it("handles null workspace in buildWorkspaceKey", () => { + expect(buildWorkspaceKey(null, "feature")).toBe(null) + expect(buildWorkspaceKey(null, null)).toBe(null) + })apps/web/lib/stores/workspaceStore.ts (1)
296-297: Consider selector stability foruseCurrentWorktree.The selector creates a closure over
workspacewhich is fine, but note that this hook will cause re-renders whenever any property incurrentWorktreeByWorkspacechanges, not just the specific workspace entry. For high-frequency updates, consider usinguseShallowor a more targeted selector.This is acceptable for the current use case but worth noting for future optimization if needed.
apps/web/components/workspace/WorktreeSwitcher.tsx (2)
16-21: Consider importing the type from the worktrees module.This local
WorktreeListIteminterface duplicates the server-side type (minus thepathfield). If the server shape changes, this could drift. Consider importing and usingPick<WorktreeListItem, ...>from the shared module or defining a shared DTO type.
68-72: Replaceconfirm()with an accessible modal confirmation.The native
confirm()dialog blocks the main thread and isn't accessible or styleable. Consider using a custom confirmation modal component that follows the same patterns as the existingModalcomponent.apps/web/features/workspace/lib/worktree-utils.ts (1)
33-38:parseWorkspaceKeymay misbehave if workspace contains the delimiter.Using
split("::wt/")without a limit could produce unexpected results if the workspace string itself contains::wt/. While unlikely in practice, consider using a more robust approach:Suggested fix
export function parseWorkspaceKey(key: string): { workspace: string; worktree: string | null } { - const [workspace, worktreePart] = key.split("::wt/") - if (!worktreePart) { - return { workspace: key, worktree: null } - } - return { workspace, worktree: worktreePart || null } + const delimiterIndex = key.indexOf("::wt/") + if (delimiterIndex === -1) { + return { workspace: key, worktree: null } + } + return { + workspace: key.slice(0, delimiterIndex), + worktree: key.slice(delimiterIndex + 5) || null, + } }apps/web/lib/api/schemas.ts (1)
490-498: Consider using OptionalWorktreeSchema for the slug field.The
worktrees/createrequest schema usesz.string().optional()forslug, but theOptionalWorktreeSchema(already imported) provides validation and normalization. This would ensure client-side validation matches server-side expectations:Suggested fix
"worktrees/create": { req: z .object({ workspace: z.string(), - slug: z.string().optional(), + slug: OptionalWorktreeSchema, branch: z.string().optional(), from: z.string().optional(), }) .brand<"WorktreesCreateRequest">(),apps/web/features/worktrees/lib/worktrees.ts (1)
6-8: Rename SLUG_REGEX / RESERVED_SLUGS to camelCase.Please switch to
slugRegex/reservedSlugs(and update references) to align with the naming rule. As per coding guidelines, Use camelCase for variable and function names.apps/web/app/chat/page.tsx (1)
805-807: Refactor ChatInput usage to the compound pattern.Guidelines require the compound pattern (
<ChatInput><ChatInput.InputArea ... /></ChatInput>), but both desktop and mobile usages still use the single‑component form. As per coding guidelines, Always use compound pattern for ChatInput:<ChatInput><ChatInput.InputArea />...</ChatInput>.Also applies to: 861-863
Split worktree Zod schemas into separate file to prevent bcrypt/fs from leaking into client bundles via import chain. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/components/workspace/WorktreeSwitcher.tsx`:
- Around line 125-137: The new worktree object in the setWorktrees update is
assigning pathRelative from response.worktreePath which may be an absolute path;
change this to avoid exposing absolute paths by assigning a safe value (e.g.,
use response.slug or a server-provided relative path field) instead of
response.worktreePath when building the object in the setWorktrees callback
(where slug, pathRelative, branch, head are created after
postty("worktrees/create", ...)). Ensure the UI-rendered pathRelative uses the
sanitized/relative value the server provides or the slug fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/tsconfig.json (1)
35-45:⚠️ Potential issue | 🟡 MinorRemove the redundant
.next-test/dev/dev/types/**/*.tsinclude path.Line 44 duplicates the "dev" segment and appears to be a typo. The correct pattern follows the structure of
.next→.next/devand.next-test→.next-test/dev. Remove this line, as it is already covered by line 40 (.next-test/dev/types/**/*.ts) and no references to this doubled path exist in the codebase.
🤖 Fix all issues with AI agents
In `@apps/web/types/guards/worktree-schemas.ts`:
- Around line 29-37: OptionalWorktreeSchema currently allows empty strings and
only accepts undefined (not null), contradicting validateWorktreeSlug; change
the schema to accept nullish values and reject empty strings by adding
nullable() (or equivalent null support) and updating the transform and
refinements so normalization only runs for non-empty strings (e.g., treat "" the
same as a validation failure) and the refine predicates use val == null to allow
null/undefined but explicitly reject "" before testing WORKTREE_SLUG_REGEX;
reference OptionalWorktreeSchema, normalizeWorktreeSlug, WORKTREE_SLUG_REGEX and
align behavior with validateWorktreeSlug.
🧹 Nitpick comments (1)
apps/web/lib/api/schemas.ts (1)
490-498: Reuse OptionalWorktreeSchema for slug validation.
slug: z.string().optional()lets invalid/empty values passvalidateRequest. Prefer the shared worktree schema so client-side validation matches server expectations.Proposed change
"worktrees/create": { req: z .object({ workspace: z.string(), - slug: z.string().optional(), + slug: OptionalWorktreeSchema, branch: z.string().optional(), from: z.string().optional(), }) .brand<"WorktreesCreateRequest">(),
Includes: - PR #11: Automation scheduler fixes - bridge -> stream renaming - Cron scheduler components - Various test and doc updates Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/chat/page.tsx (1)
806-828:⚠️ Potential issue | 🟠 MajorSwitch ChatInput usages to the compound pattern.
Both desktop and mobile inputs still use
<ChatInput ... />. Please refactor to the compound API (e.g.,<ChatInput><ChatInput.InputArea />...</ChatInput>), which is required for extension via composition. As per coding guidelines: "Always use compound pattern for ChatInput:<ChatInput><ChatInput.InputArea />...</ChatInput>."Also applies to: 862-881
🤖 Fix all issues with AI agents
In `@apps/web/components/workspace/WorktreeSwitcher.tsx`:
- Around line 45-56: The setter setModalOpen currently short‑circuits whenever
onOpenChange exists, preventing internalOpen from updating when isOpen is not
provided; change the logic in setModalOpen so it only avoids updating
internalOpen when the component is in controlled mode (isControlled), i.e. call
onOpenChange if present but only skip setInternalOpen when isControlled is true;
refer to isControlled, modalOpen, setModalOpen, internalOpen, onOpenChange and
isOpen to locate and update the handler accordingly.
🧹 Nitpick comments (1)
apps/web/features/chat/lib/__tests__/workspace-naming-regression.test.ts (1)
23-38: Consider a small test helper to de-duplicate the “missing site” skip logic.The same skip/warn pattern appears in multiple tests; a helper keeps maintenance simpler and avoids drift.
♻️ Suggested refactor
+function skipIfMissing(path: string, extraMessage?: string) { + if (existsSync(path)) return false + console.warn(`⚠️ Test site missing: ${path}`) + if (extraMessage) console.warn(extraMessage) + return true +} + it("CRITICAL: finds evermore.alive.best with DOTS in directory name", async () => { // Verify the actual directory exists with DOTS const dotsPath = "/srv/webalive/sites/evermore.alive.best/user" const hyphensPath = "/srv/webalive/sites/evermore-alive-best/user" - if (!existsSync(dotsPath)) { - console.warn(`⚠️ Test site missing: ${dotsPath}`) - console.warn("This test requires evermore.alive.best to exist") - return // Skip if test site doesn't exist - } + if (skipIfMissing(dotsPath, "This test requires evermore.alive.best to exist")) return const result = await getWorkspace({ host: DOMAINS.STREAM_DEV_HOST, body: { workspace: "evermore.alive.best" }, requestId: "regression-test", })As per coding guidelines, Use test helper functions and fixtures to reduce duplication and improve test maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
scripts/maintenance/setup-auto-cleanup.sh (1)
30-32:⚠️ Potential issue | 🟡 MinorDuplicate
OnCalendardirectives will trigger the timer twice daily.The comment says "Run daily at 3 AM", but there are two
OnCalendarentries:
OnCalendar=dailytriggers at midnight (00:00:00)OnCalendar=*-*-* 03:00:00triggers at 3 AMSystemd fires the timer at both times. Remove line 31 to run only at 3 AM as intended.
🐛 Proposed fix
[Timer] # Run daily at 3 AM -OnCalendar=daily OnCalendar=*-*-* 03:00:00 Persistent=truepackages/site-controller/scripts/99-teardown.sh (1)
159-159:⚠️ Potential issue | 🟡 MinorInconsistency confirmed:
SITES_ROOTuses outdated path scheme inconsistent with refactoring efforts.Within the same script, three different path schemes coexist:
/root/alive(BRIDGE_ROOT, line 39),/var/lib/alive(SERVER_CONFIG, line 40), and/srv/webalive/sites(SITES_ROOT, line 159). The/srv/webalive/sitespattern also appears in02-setup-fs.shand03-build-site.sh. If this is part of a broader migration to the new path structure,SITES_ROOTshould be updated to align with the newer/root/aliveor/var/lib/aliveconventions. If these directories serve intentionally separate purposes (sites storage vs. bridge configuration), document the architectural decision to justify the inconsistency.packages/worker-pool/scripts/check-import-order.mjs (1)
199-204:⚠️ Potential issue | 🔴 CriticalAdd
@webalive/toolsto packages/worker-pool/package.json.The import is present in worker-entry.mjs (line 33), but the dependency is missing from package.json. Since the check-import-order.mjs script now enforces
@webalive/toolsas a required top-level import, CI will fail without the dependency declaration.ops/scripts/pre-deployment-check.sh (1)
89-101:⚠️ Potential issue | 🟡 MinorGuard disk-usage check against missing build dir.
With
set -e,df "$BUILD_DIR"aborts the script if the new path doesn’t exist yet, so you won’t reach the summary or the explicit “build directory not found” check later. Consider guarding the disk-usage check with a directory existence test.🔧 Proposed fix
echo "[4/7] Checking disk space..." BUILD_DIR="/root/alive/.builds/$ENVIRONMENT" -DISK_USAGE=$(df "$BUILD_DIR" | awk 'NR==2 {print $5}' | sed 's/%//') - -if [ "$DISK_USAGE" -gt 80 ]; then - echo " ❌ CRITICAL: Disk usage is ${DISK_USAGE}% (> 80%)" - echo " Free up space before deployment" - ISSUES=$((ISSUES + 1)) -elif [ "$DISK_USAGE" -gt 70 ]; then - echo " ⚠️ WARNING: Disk usage is ${DISK_USAGE}% (> 70%)" -else - echo " ✅ Disk usage OK (${DISK_USAGE}%)" -fi +if [ -d "$BUILD_DIR" ]; then + DISK_USAGE=$(df "$BUILD_DIR" | awk 'NR==2 {print $5}' | sed 's/%//') + if [ "$DISK_USAGE" -gt 80 ]; then + echo " ❌ CRITICAL: Disk usage is ${DISK_USAGE}% (> 80%)" + echo " Free up space before deployment" + ISSUES=$((ISSUES + 1)) + elif [ "$DISK_USAGE" -gt 70 ]; then + echo " ⚠️ WARNING: Disk usage is ${DISK_USAGE}% (> 70%)" + else + echo " ✅ Disk usage OK (${DISK_USAGE}%)" + fi +else + echo " ❌ Build directory not found for disk check: $BUILD_DIR" + ISSUES=$((ISSUES + 1)) +fiscripts/maintenance/update-cors-domains.sh (1)
4-15:⚠️ Potential issue | 🟠 MajorFix SITES_DIR path to current deployment location.
SITES_DIRpoints to/root/webalive/sites, but other scripts in the codebase (migrate-vite-preview-nav.ts, update-alive-tagger.sh) use/srv/webalive/sites. The script will fail silently if the sites directory doesn't exist at the hardcoded path. Update to/srv/webalive/sitesto match the current deployment structure, or follow the pattern inscripts/sites/add-verification-files.shwhich allows environment variable override with a sensible default:-SITES_DIR="/root/webalive/sites" +SITES_DIR="${SITES_DIR:-/srv/webalive/sites}"Also update the header comment to reflect the actual directory scanned.
scripts/validation/detect-workspace-issues.sh (3)
43-49:⚠️ Potential issue | 🟡 MinorSuccess message prints even when deprecated packages are found.
Line 49 unconditionally prints "✓ No deprecated packages found" after the loop, even if deprecated packages were detected and
ISSUES_FOUNDwas incremented. This creates misleading output.🐛 Proposed fix
DEPRECATED_PACKAGES=("deploy-scripts") +DEPRECATED_FOUND=0 for pkg in "${DEPRECATED_PACKAGES[@]}"; do if [ -d "packages/$pkg" ]; then echo " ❌ Deprecated package exists: packages/$pkg" ISSUES_FOUND=$((ISSUES_FOUND + 1)) + DEPRECATED_FOUND=1 fi done -echo "✓ No deprecated packages found" +if [ $DEPRECATED_FOUND -eq 0 ]; then + echo "✓ No deprecated packages found" +fi
17-17:⚠️ Potential issue | 🟡 MinorInconsistent check counters.
The check counters are inconsistent throughout the script:
- Lines 17, 41, 53:
[1/4],[2/4],[3/4]- Lines 72, 84:
[4/5],[5/5]All counters should reflect the total of 5 checks.
🔧 Proposed fix for lines 17, 41, and 53
-echo "[1/4] Checking for circular symlinks in packages..." +echo "[1/5] Checking for circular symlinks in packages..."-echo "[2/4] Checking for deprecated packages..." +echo "[2/5] Checking for deprecated packages..."-echo "[3/4] Checking for broken symlinks in node_modules..." +echo "[3/5] Checking for broken symlinks in node_modules..."
85-92:⚠️ Potential issue | 🟡 MinorSuccess message prints even when empty directories are found.
Similar to check 2, line 92 unconditionally prints "✓ All packages have package.json" even if packages without
package.jsonwere detected.🐛 Proposed fix
+EMPTY_FOUND=0 for dir in packages/*/; do PKG_NAME=$(basename "$dir") if [ ! -f "$dir/package.json" ]; then echo " ❌ No package.json in: packages/$PKG_NAME" ISSUES_FOUND=$((ISSUES_FOUND + 1)) + EMPTY_FOUND=1 fi done -echo "✓ All packages have package.json" +if [ $EMPTY_FOUND -eq 0 ]; then + echo "✓ All packages have package.json" +fi
🤖 Fix all issues with AI agents
In `@scripts/sites/add-verification-files.sh`:
- Line 10: Update the script scripts/sites/add-verification-files.sh to avoid
failing on older deployments by either adding a backward-compatible lookup or
documenting a migration: option A — change how CONFIG_FILE is resolved (try
"/var/lib/alive/server-config.json" first, then fallback to
"/var/lib/claude-bridge/server-config.json" and emit a clear deprecation warning
when the legacy path is used) or option B — leave CONFIG_FILE fixed but add a
short, prominent migration note in the repository setup/README explaining that
the config must be moved to "/var/lib/alive/server-config.json" before running
add-verification-files.sh; reference the CONFIG_FILE variable in the script for
the code change or the README section for the documentation change.
In `@scripts/validation/detect-workspace-issues.sh`:
- Line 55: The for-loop over find output using "for link in $(find
node_modules/@webalive node_modules/@webalive node_modules/@alive-game -maxdepth
1 -type l 2>/dev/null); do" is fragile and contains a duplicated path; replace
the duplicate second "node_modules/@webalive" with the intended
"node_modules/@alive-brug" (or remove if not intended) and convert the loop to a
robust null-delimited pattern: call find with -print0 and iterate with "while
IFS= read -r -d '' link; do ... done" so the variable link (used in the existing
loop) safely handles filenames with spaces/special characters.
🧹 Nitpick comments (2)
packages/site-controller/scripts/05-caddy-inject.sh (1)
24-28: Add sanity checks / overrides for BRIDGE_ROOT and SERVER_CONFIG.
Fail fast with a clearer message if the default path is missing, and allow overrides for non-standard installs or tests.♻️ Suggested tweak
-BRIDGE_ROOT="${BRIDGE_ROOT:-/root/alive}" +BRIDGE_ROOT="${BRIDGE_ROOT:-/root/alive}" +if [[ ! -d "$BRIDGE_ROOT" ]]; then + log_error "BRIDGE_ROOT not found: $BRIDGE_ROOT" + exit 15 +fi -SERVER_CONFIG="/var/lib/alive/server-config.json" +SERVER_CONFIG="${SERVER_CONFIG:-/var/lib/alive/server-config.json}" +if [[ -f "$SERVER_CONFIG" && ! -r "$SERVER_CONFIG" ]]; then + log_error "SERVER_CONFIG not readable: $SERVER_CONFIG" + exit 15 +fiscripts/sites/add-verification-files.sh (1)
10-10: UpdateCLAUDE.mdif this path change affects workflow or structure.This change alters deployment/config location conventions. If that’s part of the documented workflow, the template’s
CLAUDE.mdshould be updated accordingly. Based on learnings: Update CLAUDE.md file whenever creating a new site from this template, making significant architectural changes, adding new frameworks/libraries/major features, or changing file structure/development workflow.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Delete memory database file - Remove /api/priorities route that used it - Remove "Agent Memory" sections from CLAUDE.md and AGENTS.md - Update rule 7 from "USE THE BRAIN" to "BE METHODICAL" Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@scripts/deployment/build-atomic.sh`:
- Around line 186-189: The copy assumes the destination
"$STANDALONE_NODE_MODULES/@webalive/worker-pool/dist/" exists; update the script
around the worker-entry.mjs copy so that before cp is run you ensure the
destination directory exists (use a safe create like mkdir -p on
"$STANDALONE_NODE_MODULES/@webalive/worker-pool/dist/"), then perform the copy
from "packages/worker-pool/src/worker-entry.mjs" only if the source exists;
reference the variables/paths STANDALONE_NODE_MODULES,
"packages/worker-pool/src/worker-entry.mjs" and the destination
"@webalive/worker-pool/dist/" when making the change.
- Around line 163-166: The rm -rf that deletes the template directory should
guard against an empty STANDALONE_PACKAGES to avoid accidental deletion; update
the removal call that targets "$STANDALONE_PACKAGES/template" (the rm -rf line)
to first validate STANDALONE_PACKAGES is non-empty (e.g. check [ -n
"$STANDALONE_PACKAGES" ] or use a parameter expansion guard) before running rm
-rf and preserve the existing 2>/dev/null || true fallback and subsequent cp
logic.
- Around line 155-161: The rm -rf call can expand to a dangerous path if
STANDALONE_PACKAGES or pkg are empty; update the loop that iterates over pkg to
use Bash parameter expansion checks (e.g., ${STANDALONE_PACKAGES:?} and
${pkg:?}) when constructing the destination path used in rm -rf and the cp
commands so the script fails fast on unset/empty variables; specifically adjust
the lines referencing STANDALONE_PACKAGES and pkg in the for-loop (the rm -rf
"$STANDALONE_PACKAGES/$pkg" and subsequent cp -rL/cp -r invocations) to use the
:? expansion to guard against empty values.
- Create lib/standalone-packages.sh as single source of truth for packages - Add validate-standalone.sh to check: - All packages in config exist - Packages in next.config.js are in standalone config - Subprocess packages (worker-pool) have their dependencies (claude-agent-sdk) - Update build-atomic.sh to use config and run validation before build This prevents the runtime error where worker-pool couldn't find @anthropic-ai/claude-agent-sdk because it wasn't copied to standalone. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/features/chat/lib/workspaceRetriever.ts (1)
136-152:⚠️ Potential issue | 🟠 MajorAdd isPathWithinWorkspace() guard before filesystem checks.
existsSyncon Line 152 runs without anisPathWithinWorkspace()validation. Please validate the absolute candidate path before the filesystem access.🔒 Suggested fix
const candidateFullPath = path.join("/srv", normalized) + if (!isPathWithinWorkspace(candidateFullPath)) { + console.warn(`[Workspace ${requestId}] Skipping path outside workspace root: ${candidateFullPath}`) + continue + } if (existsSync(candidateFullPath)) {As per coding guidelines, Always use path validation with isPathWithinWorkspace() before any file operation to prevent path traversal attacks.
🤖 Fix all issues with AI agents
In `@apps/web/app/api/claude/stream/route.ts`:
- Around line 276-282: The session key creation now includes worktree which
changes scoping from workspace-level to worktree-level; either remove worktree
from the tabKey call so the key is built as tabKey({ userId: user.id, workspace:
resolvedWorkspaceName, tabGroupId, tabId }) to restore the documented
userId::workspace::tabId pattern (adjust any code that relies on worktree-scoped
keys), or if the per-worktree scope is intentional, update the session
management docs and any naming/comments to reflect the new
userId::workspace::worktree::tabGroupId::tabId pattern and ensure all consumers
of sessionKey (symbol: sessionKey) and the function tabKey accept and expect the
worktree parameter consistently.
In `@scripts/deployment/validate-standalone.sh`:
- Around line 49-53: The script fails under set -euo pipefail when grep finds no
matches; modify the extraction of NEXT_PACKAGES so the grep command cannot cause
a non-zero exit (e.g., append a safe fallback like using "|| true" or an
if-check around grep) and ensure the variable NEXT_PACKAGES is set to an empty
string rather than causing the script to exit; adjust the assignment that
references NEXT_CONFIG and the subsequent for next_pkg in $NEXT_PACKAGES loop so
the loop simply skips when no packages are found.
🧹 Nitpick comments (1)
scripts/deployment/lib/standalone-packages.sh (1)
18-33: Optional: mark arrays as intentional exports and silence SC2034.Since this file is sourced, Shellcheck flags these as unused locally. Consider making intent explicit to reduce noise.
♻️ Proposed tweak
-# Packages to copy to standalone build -# Format: space-separated list of package names (from packages/ directory) -STANDALONE_PACKAGES=( +# shellcheck disable=SC2034 +# Packages to copy to standalone build +# Format: space-separated list of package names (from packages/ directory) +readonly -a STANDALONE_PACKAGES=( tools images shared worker-pool site-controller database ) # Packages that have external dependencies needed in subprocess # These get their node_modules copied too (via cp -rL) -SUBPROCESS_PACKAGES=( +readonly -a SUBPROCESS_PACKAGES=( worker-pool )
When a workspace stored in localStorage doesn't exist on the current server's filesystem, it was still shown as selected, causing WORKSPACE_NOT_AUTHENTICATED errors when trying to use it. Now validates stored workspace against available workspaces (from API) when organizations load, clearing any that aren't accessible. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add documentation comment to route tests explaining HTTP layer separation - Add POST workspace validation with tests for empty/whitespace values - Fix branch collision loop to use counter instead of timestamp - Add --force flag to git worktree remove when allowDirty is true - Add WORKTREE_GIT_FAILED error code and mapping - Fix Zod schema to use .nullish() instead of .optional() - Add aria-labels for accessibility in WorktreeSwitcher - Fix modal state bug when onOpenChange provided without isOpen - Fix absolute path display in worktree list (use slug instead) - Move PR docs to docs/archive/prs/ - Fix tree indentation in workspace-isolation.md - Add language specifier to code blocks in worktrees.md - Add FORBIDDEN error code to cancel route - Guard rm -rf commands in build scripts - Fix grep false failure in validate-standalone.sh - Fix duplicate directory in detect-workspace-issues.sh - Update tabKey examples in CLAUDE.md - Add security documentation to worktrees.ts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Track nitpicks and outside-diff-range comments from PR #12: - 5 nitpicks (minor improvements) - 10 outside diff range issues (including 1 critical) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/deployment/build-atomic.sh`:
- Around line 178-193: The copy loop can nest when destination already exists;
before copying each package (in the for loop over "${STANDALONE_PACKAGES[@]}"
template) remove the destination path under STANDALONE_NODE_MODULES/@webalive
for that package (e.g., rm -rf "$STANDALONE_NODE_MODULES/@webalive/$pkg") then
perform the cp -rL ... || cp -r ... operations as written; use the same
destination cleanup logic that Phase 8 uses and reference
STANDALONE_NODE_MODULES, STANDALONE_PACKAGES_DIR and STANDALONE_PACKAGES in the
loop to locate the code to change.
🧹 Nitpick comments (5)
apps/web/lib/hooks/useSettingsQueries.ts (1)
63-75: Prefer async/await over.then()chains.As per coding guidelines, async/await should be used for promise handling instead of
.then()chains. Consider refactoring to an async IIFE:♻️ Proposed refactor
authStore.setAuthenticated() validateAndCleanup(query.data.organizations as Organization[]) - // Also validate workspace availability on this server - // This clears currentWorkspace if the site doesn't exist on the filesystem - getty("auth/all-workspaces") - .then(response => { - const allWorkspaces = Object.values(response.workspaces).flat() - validateWorkspaceAvailability(allWorkspaces) - }) - .catch(err => { - console.warn("[useOrganizationsQuery] Failed to validate workspace availability:", err) - }) + // Also validate workspace availability on this server + // This clears currentWorkspace if the site doesn't exist on the filesystem + ;(async () => { + try { + const response = await getty("auth/all-workspaces") + const allWorkspaces = Object.values(response.workspaces).flat() + validateWorkspaceAvailability(allWorkspaces) + } catch (err) { + console.warn("[useOrganizationsQuery] Failed to validate workspace availability:", err) + } + })()docs/todos/tofix.md (1)
63-76: Good tracking of deferred issues - the FileTree worktree prop bug is notable.This documentation approach provides good visibility into technical debt. The flagged issue about missing
worktreeprop inTreeNode(lines 65-75) appears to be a real bug that could cause nested tree levels to lose worktree context. Consider prioritizing this fix.Would you like me to generate a fix for the missing
worktreeprop inapps/web/features/chat/components/sandbox/FileTree.tsx?apps/web/app/api/claude/stream/cancel/route.ts (1)
126-137: Consider using shared validation instead of inline duplication.The worktree validation logic here duplicates what's in
OptionalWorktreeSchemaandvalidateWorktreeSlug(). Consider using the shared schema for consistency:import { OptionalWorktreeSchema } from "@/types/guards/worktree-schemas" // Then in the handler: const worktreeResult = OptionalWorktreeSchema.safeParse(body.worktree) if (!worktreeResult.success) { return createErrorResponse(ErrorCodes.INVALID_REQUEST, 400, { message: worktreeResult.error.issues[0]?.message, }) } const worktree = worktreeResult.dataThis ensures validation stays in sync across all worktree-handling code paths.
apps/web/components/workspace/WorktreeSwitcher.tsx (1)
90-118: Consider replacingconfirm()with a custom confirmation dialog.The native
confirm()dialog (line 92) blocks the main thread and doesn't follow the app's design system. While functional, it creates an inconsistent UX.This is a non-blocking suggestion for future improvement - the current implementation works correctly.
apps/web/features/worktrees/lib/worktrees.ts (1)
197-202: Consider usingunknowninstead ofanyfor the caught error.The explicit
anytype works, butunknownwith a type guard is more type-safe and aligns better with strict TypeScript practices.♻️ Optional refactor for stricter typing
- } catch (error: any) { - if (error?.code === "EEXIST") { + } catch (error: unknown) { + if (error instanceof Error && (error as NodeJS.ErrnoException).code === "EEXIST") { throw new WorktreeError("WORKTREE_LOCKED", "Worktree operation already in progress") } throw error
| # Phase 9: Copy Packages to node_modules (NO SYMLINKS) | ||
| # ============================================================================= | ||
| log_step "Creating package symlinks..." | ||
| log_step "Copying packages to node_modules..." | ||
| STANDALONE_NODE_MODULES="$STANDALONE_DIR/node_modules" | ||
| mkdir -p "$STANDALONE_NODE_MODULES/@alive-brug" "$STANDALONE_NODE_MODULES/@webalive" | ||
|
|
||
| for pkg in tools images template; do | ||
| ln -sf "../../../../packages/$pkg" "$STANDALONE_NODE_MODULES/@alive-brug/$pkg" | ||
| mkdir -p "$STANDALONE_NODE_MODULES/@webalive" | ||
|
|
||
| # Copy workspace packages to node_modules/@webalive (actual copies, not symlinks) | ||
| # Include template which was copied separately | ||
| for pkg in "${STANDALONE_PACKAGES[@]}" template; do | ||
| if [ -d "$STANDALONE_PACKAGES_DIR/$pkg" ]; then | ||
| cp -rL "$STANDALONE_PACKAGES_DIR/$pkg" "$STANDALONE_NODE_MODULES/@webalive/$pkg" 2>/dev/null || \ | ||
| cp -r "$STANDALONE_PACKAGES_DIR/$pkg" "$STANDALONE_NODE_MODULES/@webalive/$pkg" | ||
| elif [ -d "packages/$pkg" ]; then | ||
| cp -rL "packages/$pkg" "$STANDALONE_NODE_MODULES/@webalive/$pkg" 2>/dev/null || \ | ||
| cp -r "packages/$pkg" "$STANDALONE_NODE_MODULES/@webalive/$pkg" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and understand the build context
cd scripts/deployment || exit 1
ls -la build-atomic.sh
wc -l build-atomic.shRepository: eenlars/alive
Length of output: 141
🏁 Script executed:
# Check the full context of Phase 9 and earlier phases to understand when node_modules/@webalive is created
sed -n '1,193p' scripts/deployment/build-atomic.sh | tail -80Repository: eenlars/alive
Length of output: 3785
🏁 Script executed:
# Search for when STANDALONE_DIR and node_modules are first created
grep -n "STANDALONE_DIR\|STANDALONE_NODE_MODULES\|mkdir.*node_modules" scripts/deployment/build-atomic.sh | head -20Repository: eenlars/alive
Length of output: 1112
🏁 Script executed:
# Check all phases to see what happens before Phase 9
grep -n "Phase" scripts/deployment/build-atomic.shRepository: eenlars/alive
Length of output: 399
🏁 Script executed:
# Check how TEMP_BUILD_DIR is initialized and if it could contain residual `@webalive`
head -80 scripts/deployment/build-atomic.sh | grep -A 5 "TEMP_BUILD_DIR"Repository: eenlars/alive
Length of output: 471
🏁 Script executed:
# Check Phase 1-3 for any cleanup that might affect STANDALONE_DIR
sed -n '64,156p' scripts/deployment/build-atomic.shRepository: eenlars/alive
Length of output: 3683
🏁 Script executed:
# Look for any other references to `@webalive` or node_modules in the script
grep -n "@webalive\|node_modules" scripts/deployment/build-atomic.shRepository: eenlars/alive
Length of output: 1544
Remove destination packages before copying to avoid nesting on retries.
The cp -r command will nest directories if the destination already exists. While the fresh build output won't include @webalive packages initially, if the script reruns or retries (e.g., during debugging or partial CI failures), existing packages or symlinks could cause cp -r to nest or copy into symlink targets. Phase 8 already follows this defensive pattern with rm -rf before copying; apply the same approach here for consistency.
Suggested fix
for pkg in "${STANDALONE_PACKAGES[@]}" template; do
+ DEST="$STANDALONE_NODE_MODULES/@webalive/$pkg"
+ rm -rf "${DEST:?}" 2>/dev/null || true
if [ -d "$STANDALONE_PACKAGES_DIR/$pkg" ]; then
- cp -rL "$STANDALONE_PACKAGES_DIR/$pkg" "$STANDALONE_NODE_MODULES/@webalive/$pkg" 2>/dev/null || \
- cp -r "$STANDALONE_PACKAGES_DIR/$pkg" "$STANDALONE_NODE_MODULES/@webalive/$pkg"
- elif [ -d "packages/$pkg" ]; then
- cp -rL "packages/$pkg" "$STANDALONE_NODE_MODULES/@webalive/$pkg" 2>/dev/null || \
- cp -r "packages/$pkg" "$STANDALONE_NODE_MODULES/@webalive/$pkg"
+ cp -rL "$STANDALONE_PACKAGES_DIR/$pkg" "$DEST" 2>/dev/null || \
+ cp -r "$STANDALONE_PACKAGES_DIR/$pkg" "$DEST"
+ elif [ -d "packages/$pkg" ]; then
+ cp -rL "packages/$pkg" "$DEST" 2>/dev/null || \
+ cp -r "packages/$pkg" "$DEST"
fi
done🤖 Prompt for AI Agents
In `@scripts/deployment/build-atomic.sh` around lines 178 - 193, The copy loop can
nest when destination already exists; before copying each package (in the for
loop over "${STANDALONE_PACKAGES[@]}" template) remove the destination path
under STANDALONE_NODE_MODULES/@webalive for that package (e.g., rm -rf
"$STANDALONE_NODE_MODULES/@webalive/$pkg") then perform the cp -rL ... || cp -r
... operations as written; use the same destination cleanup logic that Phase 8
uses and reference STANDALONE_NODE_MODULES, STANDALONE_PACKAGES_DIR and
STANDALONE_PACKAGES in the loop to locate the code to change.
- Delete alive-agents.md + alive-agents-hard-problems.md → #36 - Delete agent-supervisor.md → #37 - Delete subdomain-deployment.md → #38 - Delete docs/archive/prs/worktrees/stage-{2,3,4}.md (implemented in PR #10/#12) - Fix referral-system.md header: was "not implemented" but is fully implemented Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests