Skip to content

fix: apply PR #49 (tilde expansion + natural language commands)#52

Merged
memorysaver merged 8 commits intomainfrom
feature/pr-49-merge
Feb 23, 2026
Merged

fix: apply PR #49 (tilde expansion + natural language commands)#52
memorysaver merged 8 commits intomainfrom
feature/pr-49-merge

Conversation

@memorysaver
Copy link
Copy Markdown
Owner

Summary

Applies and fixes PR #49 from @jimmyliao (fork) with two improvements:

  • Tilde expansion: LOOPLIA_DEV_ROOT env var now expands ~ for dev plugin path resolution
  • Natural language commands: Replaces /looplia:build and /looplia:run slash commands with natural language prompts (Create a looplia workflow from the following description: / Execute the workflow:), since the SDK doesn't recognize slash commands

Also fixes all test assertions that expected the old slash command format:

Test plan

🤖 Generated with Claude Code

…e commands

- Replace /looplia:build and /looplia:run slash commands with natural language
  prompts (SDK does not recognize slash commands)
- Expand tilde (~) in LOOPLIA_DEV_ROOT env var for dev plugin path resolution
- Update all test assertions in build.test.ts (both commands/ and integration/)
  to expect the new natural language prompt format

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 23, 2026 15:09
@claude
Copy link
Copy Markdown

claude bot commented Feb 23, 2026

Code Review

Overall this is a clean, well-scoped change. The natural-language command rename is clearly necessary and the test updates are accurate. A few things worth addressing:


Bug: use homedir() instead of process.env.HOME

packages/provider/src/bootstrap/index.ts

// current
devRoot = devRoot.replace("~", process.env.HOME || "");

homedir() from node:os is already imported in this file (used at line 92 for the same kind of resolution). It's cross-platform and always returns a valid path, whereas process.env.HOME is Unix-only and can be unset.

If HOME is unset, the current code falls back to "", producing a path like /test/workspace — silently wrong rather than failing loudly.

Suggested fix:

if (devRoot.startsWith("~/")) {
  devRoot = join(homedir(), devRoot.slice(2));
}

Using join(homedir(), devRoot.slice(2)) is also cleaner than a string replace because path.join handles any edge cases in path separators.


Test: env cleanup can leave state dirty

packages/provider/test/bootstrap/index.test.ts (new test)

Two issues with the cleanup pattern in the new tilde-expansion test:

  1. process.env.X = undefined sets the string "undefined", not removing the key. If originalHome was undefined before the test, restoring it via assignment leaves HOME="undefined" in the environment, which can affect subsequent tests.

  2. No try/finally — if an assertion throws before cleanup, HOME is left mutated for the rest of the test suite. Mutating HOME is higher-risk than mutating a custom env var like LOOPLIA_DEV_ROOT.

The existing test at line 196–197 has the same pattern for LOOPLIA_DEV/LOOPLIA_DEV_ROOT, but those are controlled vars. Touching HOME raises the stakes.

Suggested approach:

it("should expand tilde in LOOPLIA_DEV_ROOT", async () => {
  const saved = {
    dev: process.env.LOOPLIA_DEV,
    root: process.env.LOOPLIA_DEV_ROOT,
    home: process.env.HOME,
  };
  try {
    process.env.LOOPLIA_DEV = "true";
    process.env.LOOPLIA_DEV_ROOT = "~/test/workspace";
    process.env.HOME = "/Users/testuser";

    const paths = await getPluginPaths();

    expect(paths[0].path).toContain("/Users/testuser/test/workspace");
    expect(paths[0].path).not.toContain("~");
  } finally {
    if (saved.dev === undefined) delete process.env.LOOPLIA_DEV;
    else process.env.LOOPLIA_DEV = saved.dev;
    if (saved.root === undefined) delete process.env.LOOPLIA_DEV_ROOT;
    else process.env.LOOPLIA_DEV_ROOT = saved.root;
    if (saved.home === undefined) delete process.env.HOME;
    else process.env.HOME = saved.home;
  }
});

Minor: bare ~ not handled

devRoot.startsWith("~/") won't expand a path that is exactly "~" with no trailing slash. This is an edge case, but worth a comment noting the limitation or expanding the condition to devRoot === "~" || devRoot.startsWith("~/").


Constants / test updates

The command rename and all 17 test assertion updates look correct. The length-calculation test is also correctly updated to compute dynamically from the prefix string rather than hardcoding the old byte count. No issues here.


Summary: The homedir() fix is the only real bug — the rest are test hygiene and edge-case notes. Happy to see this land once that's addressed.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Looplia’s dev-mode plugin path resolution and CLI agent prompts: it expands ~ in LOOPLIA_DEV_ROOT for provider bootstrap, and switches CLI “slash command” prompts to natural-language prefixes to align with SDK behavior.

Changes:

  • Expand ~/... in LOOPLIA_DEV_ROOT before computing dev plugin paths.
  • Replace /looplia:build prompt prefix with a natural-language prompt prefix (and update related build tests).
  • Update integration/unit test assertions that depended on the old /looplia:* prompt format.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/provider/src/bootstrap/index.ts Adds tilde expansion when resolving dev plugin root.
packages/provider/test/bootstrap/index.test.ts Adds coverage for ~ expansion in LOOPLIA_DEV_ROOT.
apps/cli/src/constants.ts Switches command prefixes from /looplia:* to natural-language prompt strings.
apps/cli/test/commands/build.test.ts Updates unit tests to expect the new build prompt prefix.
apps/cli/test/integration/build.test.ts Updates integration assertions to expect the new build prompt prefix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 136 to 147
expect(result).toBe("Create a looplia workflow from the following description: summarize articles");
});

it("should return just /build when no description", () => {
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These test names still refer to “/build” prompts, but buildPrompt() now emits a natural-language prefix. Renaming the test descriptions (including the one below for the empty description case) will keep the tests self-explanatory.

Copilot uses AI. Check for mistakes.
Comment on lines +461 to +462
if (devRoot.startsWith("~/")) {
devRoot = devRoot.replace("~", process.env.HOME || "");
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tilde expansion uses process.env.HOME and replaces ~ with an empty string when HOME is unset. This can yield an invalid absolute path (e.g. ~/x -> /x) and is inconsistent with the rest of this module (which uses os.homedir()). Consider expanding ~/~/ via homedir() (and ideally normalize()/join()), and handle the devRoot === "~" case as well.

Suggested change
if (devRoot.startsWith("~/")) {
devRoot = devRoot.replace("~", process.env.HOME || "");
if (devRoot === "~") {
devRoot = homedir();
} else if (devRoot.startsWith("~/")) {
devRoot = join(homedir(), devRoot.slice(2));

Copilot uses AI. Check for mistakes.
Comment on lines +209 to +217
process.env.LOOPLIA_DEV = "true";
process.env.LOOPLIA_DEV_ROOT = "~/test/workspace";
process.env.HOME = "/Users/testuser";

const paths = await getPluginPaths();

process.env.LOOPLIA_DEV = originalDev;
process.env.LOOPLIA_DEV_ROOT = originalRoot;
process.env.HOME = originalHome;
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test restores env vars by assigning the saved value back. If the original value was undefined, process.env.X = originalX may set the literal string "undefined" (and if getPluginPaths() throws, the restore code won’t run). Prefer a try/finally and delete process.env.X when the original value was unset.

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +10
* Command prefixes for looplia slash commands
* v0.6.5: Use looplia: prefix to avoid conflict with built-in commands
* v0.8.1: Changed to natural language prompts - SDK doesn't recognize /looplia:* commands
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc comment still describes these as “slash commands”, but the constants are now natural-language prompt prefixes. Updating the comment will avoid confusion for future changes (and reduces the chance someone reintroduces /looplia:* based on the comment).

Suggested change
* Command prefixes for looplia slash commands
* v0.6.5: Use looplia: prefix to avoid conflict with built-in commands
* v0.8.1: Changed to natural language prompts - SDK doesn't recognize /looplia:* commands
* Prompt prefixes for looplia CLI commands (natural-language prompts)
* v0.6.5: Used a looplia: prefix on slash commands to avoid conflict with built-in commands
* v0.8.1: Switched from /looplia:* slash commands to natural-language prompts (SDK does not recognize /looplia:* commands)

Copilot uses AI. Check for mistakes.
- Use homedir() from node:os instead of process.env.HOME for tilde
  expansion (cross-platform, handles bare ~ and ~/path)
- Add try/finally with proper delete for env cleanup in tilde test
- Update tilde test to assert against homedir() rather than a
  hardcoded path (homedir() is OS-level, not env-driven)
- Wrap long string assertions in build tests to satisfy Biome's 80-char
  print width (affects build.test.ts and integration/build.test.ts)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Feb 23, 2026

Code Review

The previous automated review comment was written against an earlier PR version — this PR has already addressed all the concerns raised there (uses homedir() correctly, has try/finally in tests, handles bare ~). Here's a fresh review of the current diff.


Overall

Clean, well-scoped change. The tilde-expansion fix is implemented correctly and the natural-language command rename is a necessary fix for SDK compatibility. All 17 test assertion updates look accurate.


bootstrap/index.ts — looks good

if (devRoot.startsWith("~/")) {
  devRoot = join(homedir(), devRoot.slice(2));
} else if (devRoot === "~") {
  devRoot = homedir();
}
  • homedir() is already imported; using it here is correct and cross-platform.
  • join() instead of string concat handles path separators properly.
  • Bare ~ is covered.

No issues.


Minor: JSDoc comment still says "slash commands"

apps/cli/src/constants.ts, line 8

/**
 * Command prefixes for looplia slash commands
 * v0.6.5: Use looplia: prefix to avoid conflict with built-in commands
 * v0.8.1: Changed to natural language prompts - SDK doesn't recognize /looplia:* commands
 */

The block heading still says "looplia slash commands" — this will mislead future readers. Consider updating to "Command prefixes for looplia agent prompts" or similar.


Minor: test doesn't cover the bare ~ case

packages/provider/test/bootstrap/index.test.ts

The new test covers ~/test/workspace but not devRoot === "~". Since the else if branch exists in the implementation, it's worth a short second test case to prevent future regressions:

it("should expand bare tilde in LOOPLIA_DEV_ROOT", async () => {
  // ...
  process.env.LOOPLIA_DEV_ROOT = "~";
  const paths = await getPluginPaths();
  expect(paths[0].path).not.toContain("~");
  expect(paths[0].path).toContain(homedir());
  // ...
});

Test coverage looks good

  • The paths.length === 2 assertion in the new test is consistent with how the existing dev-mode tests work, so no concern there.
  • The length-calculation test for COMMANDS.BUILD was updated to compute dynamically (prefix.length + 1 + 500) rather than hardcoding — this is the right approach.
  • XSS/injection test still exercises the sanitization path correctly with the new prefix.

Summary: One nit on the JSDoc comment, one missing test case for bare ~. Both are low-priority. The implementation itself is correct — good to merge after addressing the docstring if you agree it's worth the cleanup.

- Replace delete process.env.X with direct assignment in finally block
  to comply with Biome lint/performance/noDelete rule
- Rename test descriptions in build.test.ts to remove /build references
- Update JSDoc in constants.ts from slash commands to natural-language prompts

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Feb 23, 2026

Code Review

The implementation is clean and the natural-language command rename is well-scoped. A few observations:


Correctness: tilde expansion implementation is good

packages/provider/src/bootstrap/index.ts correctly uses homedir() from node:os (already imported at the top of the file) rather than process.env.HOME. This handles Windows and cases where HOME is unset. The bare ~ edge case (devRoot === "~") is also covered. No issues here.


Bug: env cleanup sets "undefined" string instead of deleting the key

packages/provider/test/bootstrap/index.test.ts (new test, lines 219–220)

} finally {
  process.env.LOOPLIA_DEV = originalDev;       // ← bug if originalDev was undefined
  process.env.LOOPLIA_DEV_ROOT = originalRoot; // ← same
}

In Node.js, process.env.X = undefined coerces undefined to the string "undefined", leaving the env var set rather than removing it. If either variable wasn't set before the test, the next test sees LOOPLIA_DEV="undefined" (which is not "true", so the branch won't trigger — but it is a leak). Correct cleanup:

} finally {
  if (originalDev === undefined) delete process.env.LOOPLIA_DEV;
  else process.env.LOOPLIA_DEV = originalDev;

  if (originalRoot === undefined) delete process.env.LOOPLIA_DEV_ROOT;
  else process.env.LOOPLIA_DEV_ROOT = originalRoot;
}

Note: the pre-existing test just above this one (around line 196) has the same pattern — not introduced by this PR, but worth fixing at the same time.


Missing test: bare ~ case

The implementation handles devRoot === "~" with no trailing slash, but there's no test for it. Given this is a distinct code path, a minimal test would be useful:

it("should expand bare tilde in LOOPLIA_DEV_ROOT", async () => {
  // ...
  process.env.LOOPLIA_DEV_ROOT = "~";
  const paths = await getPluginPaths();
  expect(paths[0].path).toContain(homedir());
  expect(paths[0].path).not.toContain("~");
  // ...
});

Minor: --name flag inside natural language prompt

The buildPrompt tests now expect values like:

"Create a looplia workflow from the following description: --name article-summary summarize articles"

The --name flag syntax embedded in a natural language prompt is potentially ambiguous to the underlying model. This isn't introduced by this PR (the flag handling pre-dates it), but it's worth revisiting whether --name should be conveyed differently in a future change (e.g., appended as a structured instruction after the description).


Test assertions look correct

All 17 updated assertions accurately reflect the new COMMANDS.BUILD/COMMANDS.RUN values. The length-calculation test now derives the expected length dynamically from the prefix string rather than hardcoding the byte count — this is the right approach.


Summary: The env cleanup bug on undefined is worth fixing before merge; the missing bare-tilde test is low-stakes but easy to add. Everything else looks good.

- Rebuild root provider before CLI bundle so new presets (M2.5) are
  picked up when node_modules symlinks resolve to the root repo
- Force-rebuild CLI via tsup after turbo build to bypass stale cache
  in git worktrees where the root node_modules are shared
- Run copy-plugins after build so apps/cli/plugins/ exists for init
- Unset CLAUDECODE in setup_test_env() to allow nested claude sessions
- Default preset updated: ZENMUX_MINIMAX_M21 → ZENMUX_MINIMAX_M25
- SKILL.md: document env -u CLAUDECODE for direct invocation inside
  Claude Code sessions; update preset reference M2.1 → M2.5

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Feb 23, 2026

Code Review

The implementation is correct and well-scoped. Two issues remain worth addressing before merge.


Bug: env cleanup sets "undefined" string instead of deleting the key

packages/provider/test/bootstrap/index.test.ts (new test)

} finally {
  process.env.LOOPLIA_DEV = originalDev;       // bug if originalDev was undefined
  process.env.LOOPLIA_DEV_ROOT = originalRoot; // same
}

In Node.js, process.env.X = undefined coerces undefined to the string "undefined" — the variable stays in the environment rather than being removed. If these vars weren't set before the test, subsequent tests see LOOPLIA_DEV="undefined" (not "true", so the branch won't trigger — but it is state leakage).

Correct cleanup:

} finally {
  if (originalDev === undefined) delete process.env.LOOPLIA_DEV;
  else process.env.LOOPLIA_DEV = originalDev;

  if (originalRoot === undefined) delete process.env.LOOPLIA_DEV_ROOT;
  else process.env.LOOPLIA_DEV_ROOT = originalRoot;
}

Note: the pre-existing test immediately above this one (the should use LOOPLIA_DEV_ROOT test) has the same pattern — worth fixing at the same time since we're touching the file.


Missing test: bare ~ case

The implementation covers devRoot === "~" (the else if branch) but there's no test for it. Since it's a distinct code path, a small test prevents silent regression:

it("should expand bare tilde in LOOPLIA_DEV_ROOT", async () => {
  const originalDev = process.env.LOOPLIA_DEV;
  const originalRoot = process.env.LOOPLIA_DEV_ROOT;
  try {
    process.env.LOOPLIA_DEV = "true";
    process.env.LOOPLIA_DEV_ROOT = "~";
    const paths = await getPluginPaths();
    expect(paths[0].path).not.toContain("~");
    expect(paths[0].path).toContain(homedir());
  } finally {
    if (originalDev === undefined) delete process.env.LOOPLIA_DEV;
    else process.env.LOOPLIA_DEV = originalDev;
    if (originalRoot === undefined) delete process.env.LOOPLIA_DEV_ROOT;
    else process.env.LOOPLIA_DEV_ROOT = originalRoot;
  }
});

Everything else looks good

  • bootstrap/index.tshomedir() (already imported) + join() is the right approach; bare ~ is handled; no issues.
  • constants.ts — JSDoc updated correctly to reflect the shift from slash commands to natural-language prompts.
  • All 17 test assertion updates — accurately reflect the new COMMANDS.BUILD/COMMANDS.RUN values. The length-calculation test derives the expected length dynamically from the prefix string rather than hardcoding — correct approach.
  • e2e-setup.sh — The worktree-aware rebuild logic and unset CLAUDECODE are sensible operational fixes.
  • Preset bump M21→M25 — straightforward config update.

Summary: The undefined cleanup bug is the only real issue; the missing bare-~ test is low-stakes but easy. Both are worth fixing before merge.

memorysaver and others added 2 commits February 24, 2026 00:57
Extend the env cleanup pattern to the pre-existing 'should return dev
paths when LOOPLIA_DEV=true' test. Previously, direct assignment of
potentially-undefined values back to process.env could leak env state
into subsequent tests if getPluginPaths() threw. Now uses try/finally
with assertions inside the try block, matching the established pattern.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Feb 23, 2026

Code Review

The core changes here are correct and well-targeted. The two outstanding issues from previous reviews are still present — confirming them below plus a few additional observations on the e2e script.


Bug (confirmed): env cleanup assigns string "undefined" instead of deleting keys

packages/provider/test/bootstrap/index.test.ts

Both the new tilde test and the refactored should use LOOPLIA_DEV_ROOT test have this pattern:

} finally {
  process.env.LOOPLIA_DEV = originalDev;       // if undefined → sets "undefined"
  process.env.LOOPLIA_DEV_ROOT = originalRoot; // same
}

In Node.js, assigning undefined to a process.env key coerces it to the string "undefined". Subsequent tests then see LOOPLIA_DEV="undefined" (not a valid truthy value, so the branch won't trigger — but it is silent state leakage). Fix:

} finally {
  if (originalDev === undefined) delete process.env.LOOPLIA_DEV;
  else process.env.LOOPLIA_DEV = originalDev;

  if (originalRoot === undefined) delete process.env.LOOPLIA_DEV_ROOT;
  else process.env.LOOPLIA_DEV_ROOT = originalRoot;
}

This applies to both the pre-existing test block and the new tilde test.


Missing test: bare ~ code path

The else if (devRoot === "~") branch in bootstrap/index.ts has no test. It's a separate branch and easy to cover alongside the "~/path" test.


e2e-setup.sh: worktree rebuild assumptions

The new build_cli() logic derives root_repo via cd "/../.." and assumes the root monorepo is exactly two levels up. This is correct for the current apps/cli nesting, but the assumption is implicit. A comment noting the expected layout (<root>/apps/cli) would help future maintainers.

Also, the rebuild logic runs twice (once for root provider, once to force-rebuild CLI), which means a worktree build always does at minimum three tsup invocations. That's fine for correctness, but a one-line comment explaining why the turbo cache bypass is needed after the provider rebuild would make the intent clear at a glance (the comment is there for the force-rebuild step but not for why the sequence matters).


What looks good

  • bootstrap/index.tshomedir() (already imported) + path.join is the right approach for tilde expansion. Bare ~ case is handled.
  • constants.ts — JSDoc updated accurately; the version note is useful for future archaeology.
  • All 17 test assertion updates — correct. The length-calculation test now derives expected length dynamically from the prefix string rather than hardcoding the byte count — this is the right approach.
  • unset CLAUDECODE in setup_test_env() — clear and well-placed; the comment explains the why.
  • Preset bump M21→M25 — straightforward.

Summary: The undefined cleanup bug affects both test blocks and is worth fixing before merge. The missing bare-~ test is low-stakes but easy to add at the same time. Everything else is solid.

@claude
Copy link
Copy Markdown

claude bot commented Feb 23, 2026

PR Review

Overall this is a clean, focused PR that correctly ports and fixes PR #49's changes. The test isolation improvements are a good defensive practice. A few observations:


Positive

  • Tilde expansion (packages/provider/src/bootstrap/index.ts:459-465) is implemented correctly using os.homedir() and path.join(), following standard Node.js patterns.
  • try/finally isolation for the getPluginPaths dev-mode test is a real improvement — env var mutations were previously left behind on test failure.
  • unset CLAUDECODE in setup_test_env() is a practical fix; without it, spawning a nested claude process from inside a Claude Code session would fail silently.
  • Test descriptions are updated accurately (removing leftover /build references).

Issues

1. Missing test coverage for the bare ~ case (packages/provider/src/bootstrap/index.ts:463)

The implementation handles both ~/path and ~ alone, but the new test only exercises ~/test/workspace. The devRoot === "~" branch has no test. Given how small it is, a one-liner test case would close the gap:

process.env.LOOPLIA_DEV_ROOT = "~";
// expect paths[0].path to start with homedir()

2. Inconsistent env-var isolation in same test file (packages/provider/test/bootstrap/index.test.ts:226-241)

The pre-existing "should return prod paths when LOOPLIA_DEV is not set" test (line 226) still sets/restores LOOPLIA_DEV without try/finally. The PR wraps the two tests above it with try/finally specifically for isolation, so leaving this test unprotected is inconsistent and was an easy fix to include.

3. --name flag embedded inside a natural-language prompt

The updated BUILD command produces strings like:

Create a looplia workflow from the following description: --name my-workflow summarize articles

The structured --name flag=value syntax is now embedded inside what is otherwise presented as a natural-language sentence. Whether the underlying model reliably extracts --name from this mixed-format prompt depends entirely on its instruction following. This is a design-level concern rather than a bug in the PR itself, but it's worth an explicit integration test or comment documenting the expected parsing behavior.

4. Brittle worktree detection in e2e-setup.sh (line 53)

root_repo="$(cd "$PROJECT_ROOT/../.." 2>/dev/null && pwd)" || true

This navigates exactly two levels up, which implicitly assumes a specific directory depth for git worktrees. If the worktree is nested at a different depth the check silently produces a wrong root_repo. A more reliable detection would be git worktree list or checking for a .git file (worktrees have a .git file rather than a directory). The current approach works for the known layout but is fragile to reorganization.

5. CHANGELOG appears incomplete for this PR's scope

The diff only adds an entry for the test isolation fix. The two behaviorally significant changes — tilde expansion in LOOPLIA_DEV_ROOT and the slash-command → natural-language prompt migration — have no CHANGELOG entries in this diff. If those were added in prior commits on this branch, ignore this; otherwise they should be recorded (at minimum under [Unreleased]).


Minor / Nits

  • In e2e-setup.sh the root-provider rebuild condition is evaluated twice (lines 54 and 71) with the same logic. Extracting a is_worktree boolean at the top of build_cli() would reduce duplication and make the intent clearer.
  • bun x tsup --silent 2>/dev/null || bun x tsup (line 56) — the fallback from silent to verbose is unusual. If the silent build fails for a real reason (compile error), the verbose retry will also fail, so this is not harmful, but the comment explaining why would help future readers.

Summary: The two primary fixes are correct. The blocking concern before merge is item 5 (CHANGELOG completeness). Items 1–2 are small enough to fix inline. Items 3–4 are design/maintainability notes that can be tracked as follow-up issues if not addressed now.

…hs test

- Add test for bare `~` LOOPLIA_DEV_ROOT (exercises the else-if branch in bootstrap/index.ts:463)
- Wrap `should return prod paths` test in try/finally to prevent env leak if getPluginPaths() throws

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Feb 23, 2026

Code Review

Overall this is a clean, well-motivated PR. The two core changes (tilde expansion and natural-language prompt migration) are sound, and the test-isolation improvements are a genuine step forward. A few things worth addressing before merge:


Bug: process.env.X = undefined sets the string "undefined", not removes the var

In the refactored test block (should return prod paths when LOOPLIA_DEV is not set) and finally blocks that restore originalDev/originalRoot when those were originally unset:

// This sets the env var to the string "undefined", not removing it
process.env.LOOPLIA_DEV = undefined;
process.env.LOOPLIA_DEV_ROOT = originalRoot; // same problem if originalRoot was undefined

The functional impact is low because the guard is === "true", so "undefined" won't match. But it leaves a dirty env between tests. The correct pattern:

} finally {
  if (originalDev === undefined) {
    delete process.env.LOOPLIA_DEV;
  } else {
    process.env.LOOPLIA_DEV = originalDev;
  }
  if (originalRoot === undefined) {
    delete process.env.LOOPLIA_DEV_ROOT;
  } else {
    process.env.LOOPLIA_DEV_ROOT = originalRoot;
  }
}

This bug exists in the pre-existing code too (line 278). The PR is a good opportunity to fix both.


Naming: COMMANDS constant is now a misnomer

// apps/cli/src/constants.ts
export const COMMANDS = {
  BUILD: "Create a looplia workflow from the following description:",
  RUN: "Execute the workflow:",
} as const;

These are prompt prefixes, not commands. COMMANDS made sense for slash commands but is now misleading. Consider PROMPT_PREFIXES or PROMPTS. Low priority — can be addressed in a follow-up.


Tilde expansion: ~username form not handled

The implementation correctly handles ~/path and bare ~ but not the POSIX ~username form. This is an acceptable scope limitation for a dev CLI reading from env vars, but a short comment documenting the known gap would help avoid a future bug report.


E2E script: || true silently swallows errors

root_repo="$(cd "$PROJECT_ROOT/../.." 2>/dev/null && pwd)" || true

The downstream [[ -n "${root_repo:-}" ]] guard handles the empty-string case correctly, so there is no incorrect behavior. But silent suppression makes unexpected failures harder to diagnose. A fallback log line when root_repo ends up empty would help.


What's done well

  • Imports are correct: homedir and join were already imported in packages/provider/src/bootstrap/index.ts — no missing imports.
  • try/finally test isolation: All three new tilde-expansion tests use it correctly, and the pre-existing test was refactored to match.
  • Tilde expansion logic: Handles ~/path and bare ~ with a simple conditional — no regex overkill.
  • unset CLAUDECODE in setup_test_env: The explanation is clear and the fix is correct for the nested-session problem.
  • Test assertions fully updated: All 14 unit test assertions and 3 integration test assertions updated, including the length calculation test which correctly recomputes expected length from the new prefix string rather than hardcoding 515.
  • CHANGELOG entry accurately reflects the test-isolation fix.

- Add SDK parenthetical to v0.8.1 history in constants.ts
- Replace env var assignment with delete-when-undefined in 4 getPluginPaths finally blocks to avoid setting literal "undefined" string in non-Bun runtimes; suppress noDelete lint rule with biome-ignore comments

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Feb 23, 2026

Code Review

This is a clean, well-scoped PR that correctly implements two fixes from PR #49. The previously raised concerns (env-var cleanup bug, missing bare-tilde test) have all been addressed in the current diff. Here's a final pass:


Previously raised bugs — confirmed fixed

  • try/finally with proper delete pattern: All four test blocks in packages/provider/test/bootstrap/index.test.ts now use the correct conditional delete / reassign pattern with biome-ignore justification comments. ✓
  • Bare ~ test: "should expand bare tilde in LOOPLIA_DEV_ROOT" test is present and exercises the else if (devRoot === "~") branch. ✓
  • homedir() + path.join(): Tilde expansion uses the already-imported homedir() — cross-platform, never produces an empty string. ✓

Remaining minor items

1. CHANGELOG may be incomplete for this PR's scope

The diff adds only one [Unreleased] entry (test isolation). The tilde expansion for LOOPLIA_DEV_ROOT and the slash-command → natural-language prompt migration are both user-visible behaviour changes. If these are not already recorded in a prior commit on this branch, they should be added. Suggested entries:

- **Tilde expansion for LOOPLIA_DEV_ROOT** - `~` and `~/path` in `LOOPLIA_DEV_ROOT` are now expanded to the OS home directory, making dev-mode plugin path configuration portable across environments.
- **Natural language commands** - Replaced `/looplia:build` and `/looplia:run` slash commands with natural-language prompt prefixes to fix SDK compatibility.

2. COMMANDS is a misnomer (nit)

apps/cli/src/constants.ts:12 — These are prompt prefixes now, not commands. PROMPT_PREFIXES would be more descriptive. Low priority, trackable as follow-up.

3. E2E worktree detection fragility (informational)

.claude/skills/looplia-e2e/scripts/e2e-setup.sh:53 — navigating two levels up (../.. ) is an implicit assumption about worktree nesting depth. Works for the current layout (<root>/apps/cli), but a comment documenting the expected structure would help future maintainers.


Everything else looks good

  • All 17 test assertion updates accurately reflect the new COMMANDS.BUILD/COMMANDS.RUN values.
  • Length-calculation test now derives expected length dynamically from the prefix string rather than hardcoding the old byte count.
  • unset CLAUDECODE in setup_test_env() is well-placed and clearly documented.
  • Preset bump M21→M25 is straightforward.
  • The ~username form is intentionally out of scope for a dev env-var; acceptable.

Summary: Item 1 (CHANGELOG completeness) is the only thing worth verifying before merge. Items 2–3 are low-priority follow-ups. The implementation is correct and the previous bugs are all resolved.

@memorysaver memorysaver merged commit 2238eb6 into main Feb 23, 2026
2 checks passed
@memorysaver
Copy link
Copy Markdown
Owner Author

Merged via squash. The changes from this PR (addressing Copilot review comments on the tilde expansion and natural language commands work) have landed on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants