Restructure CLI commands, add default workspace root, update docs#15
Restructure CLI commands, add default workspace root, update docs#15
Conversation
This doesn’t seem to be true, I ran It would be good to start using versions so I can confirm I’m indeed on the newest; if updating manually is too annoying we could follow Embroider’s prerelease pattern: ETA I am indeed on the newer version because |
|
Test plan, one line at a time:
The owner of this realm is But this one showed up as This behaved as expected: |
|
TLDR:
Details
This is not true! I have to explicitly use
This works but do we want backward compatibility? IMO we might as well make a clean break as this is not officially released.
In this case I think we can treat
❯ boxel track --push [11:44:27 AM] 🔔 Changes detected (1 pending) [11:44:30 AM] 📦 Creating checkpoint (1 changes)...
Uploading 1 files in 1 batch(es) (3556KB total) Upload complete: 0 succeeded, 1 failed (1910ms) |
Commander's .option('--no-fix-index') makes the underlying boolean
default to TRUE, which was the opposite of what PR #15 advertised.
Running `boxel doctor repair-realm <url>` silently rewrote
index.json/cards-grid.json by default, destroying customized index
files (caught by backspace on Checkly-prerendered realm).
Flip to .option('--fix-index') across all 4 command surfaces:
- repair-realm (hidden top-level)
- repair-realms (hidden top-level)
- doctor repair-realm
- doctor repair-realms
Now the default matches the documentation: index.json is untouched
unless --fix-index is explicitly passed.
Added 2 regression tests that exercise the commander parsing directly
(the previous tests only checked the handler-level fallback, which
ran AFTER commander had already set fixIndex=true).
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
christse
force-pushed
the
pr-review/command-restructure-and-defaults
branch
from
0b7ad86 to
8f81ff8
Compare
Buck reported on PR #15 that `boxel pull <url>` without a local-dir argument produced weird paths for published realms: - 1-segment URL https://realms-staging.stack.cards/boxel-homepage/ → ~/boxel-workspaces/realms-staging.stack.cards/boxel-homepage/boxel-homepage (the fallback `parts[1] ?? parts[0]` duplicated the realm name as owner) - 0-segment URL https://gabbro.staging.boxel.build/ → ~/boxel-workspaces/gabbro.staging.boxel.build/unknown-owner/workspace (invented placeholder segments) Fix: adapt the layout depth to match what the URL actually provides. - 0 segments → <host>/ - 1 segment → <host>/<realm>/ - 2+ segments → <host>/<owner>/<realm>/ (unchanged) findManifestPaths() now also walks 2-level <root>/<domain>/<realm>/ layouts so the legacy-path warning and doctor consolidate-workspaces still detect all manifests after the layout change. Added 4 regression tests covering both edge cases. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
PR #15 introduced positional subcommands (realms add/remove/init/llm) and kept the old flag forms as hidden aliases for backwards compat. Since the restructure has not yet been released (still on PR), there are no users to migrate. Clean break is simpler. Per backspace's PR #15 review: "do we want backward compatibility? IMO we might as well make a clean break as this is not officially released." - Remove 9 `.addOption(new Option('--xxx').hideHelp())` calls - Remove legacy flag dispatcher `realmsCommand()` and RealmsOptions - Drop unused `Option` import from commander `boxel realms` with no subcommand still shows the list. The `list` alias for `workspace-list` stays (backspace: "I think we can treat list as a shorthand"). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Minor-version bump (not patch) because PR #15 changes visible command names: `list` → `workspace-list`, `realms --add` → `realms add`, `boxel doctor` parent for repair/consolidate. Plus the two bug fixes @backspace caught on review. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
Rebased onto main (1.0.1) and addressed all three comments @backspace. Now at 8f81ff8, shipping as 1.1.0. Bugs you caught1. You were right, the PR description lied. The cause was commander subtlety: Fix in commit Verify: 2. Pull path extraction duplicated / invented segments (your comment #2) Fix in
Also updated 3. Drop Done in Version stampingBumped to Ship state
Ready for another look. |
There was a problem hiding this comment.
Pull request overview
This PR restructures the Boxel CLI command surface (grouping maintenance commands under boxel doctor, renaming list → workspace-list, and moving realms to positional subcommands), while also introducing ~/boxel-workspaces/ as the default local workspace root and updating docs/tests accordingly.
Changes:
- Add
defaultWorkspacesRoot()and update workspace path derivation to preserve full hostnames and support 0/1-segment “published realm” URLs. - Restructure CLI commands (
doctor,workspace-list,realms <subcommand>) and add hidden backward-compat command aliases. - Update docs/changelog and add new tests for path behavior and repair flag defaults.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/workspace-paths.ts |
Adds defaultWorkspacesRoot(), removes hostname normalization, and updates structured path logic + manifest scanning depth. |
src/lib/workspace-resolver.ts |
Switches URL-based default local dir resolution to ~/boxel-workspaces/. |
src/index.ts |
Implements new command tree (doctor, workspace-list, positional realms) and updates pull default directory logic. |
src/commands/consolidate.ts |
Defaults consolidate scan root to defaultWorkspacesRoot(). |
src/commands/repair.ts |
Makes batch repair defaults safer (opt-in index rewrite/touch, preserve valid names). |
src/commands/realms.ts |
Splits realms management into explicit subcommand handlers (list/add/remove/init/llm). |
src/commands/track.ts |
Surfaces batch upload error details during track --push. |
test/lib/workspace-paths.test.ts |
Adds coverage for hostname preservation and 0/1-segment URL layouts; tests default root. |
test/lib/realm-config.test.ts |
Adds unit tests for addRealm/removeRealm. |
test/commands/repair.test.ts |
Adds regression coverage for commander flag parsing; adds name-selection tests. |
README.md |
Updates terminology and onboarding/docs for new command names and default workspace root. |
docs/realm-repair.md |
Updates repair command docs to boxel doctor .... |
CHANGELOG.md |
Adds 1.1.0 entry documenting command/path changes and regressions fixed. |
AGENTS.md |
Updates agent onboarding/command references to new names. |
.claude/commands/setup.md |
Updates onboarding steps (workspace-list, pull default root). |
.claude/commands/repair.md |
Updates repair workflow docs to boxel doctor .... |
.claude/CLAUDE.md |
Updates onboarding and reference command lists to new command names and default root. |
package.json |
Bumps version to 1.1.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const result = relativeStructuredPathForWorkspaceUrl( | ||
| 'https://app.boxel.ai/acme-corp/project-atlas/' | ||
| ); | ||
| expect(result).toBe('app.boxel.ai/acme-corp/project-atlas'); | ||
| }); |
There was a problem hiding this comment.
These expectations hardcode POSIX-style separators (e.g. app.boxel.ai/acme-corp/project-atlas), but relativeStructuredPathForWorkspaceUrl() uses path.join(...), which returns backslashes on Windows. This will fail on Windows runners/local dev. Consider building expected values with path.join(...) (or normalize separators in the assertion) so the test is platform-agnostic.
|
|
||
| // Test the repair name logic by importing the relevant helpers | ||
| // We test the exported functions indirectly through repairSingleRealm behavior | ||
|
|
||
| // Since repairSingleRealm is not directly testable (it makes HTTP calls), | ||
| // we test the name-selection logic by extracting the key functions. | ||
|
|
||
| // Import the module to test internal logic | ||
| // We use the same pattern as checkpoint-manager tests (@ts-expect-error for private access) | ||
|
|
||
| describe('repair name logic', () => { | ||
| // Replicate the isBadName logic from repair.ts | ||
| function isBadName(value: unknown): boolean { | ||
| if (typeof value !== 'string' || value.trim().length === 0) { | ||
| return true; | ||
| } | ||
| return value.trim().toLowerCase() === 'unknown workspace'; | ||
| } | ||
|
|
||
| function titleCaseFromEndpoint(realmUrl: string): string { | ||
| const parts = new URL(realmUrl).pathname.replace(/^\/|\/$/g, '').split('/'); | ||
| const endpoint = parts[parts.length - 1] ?? 'workspace'; | ||
| return endpoint | ||
| .split('-') | ||
| .filter(Boolean) | ||
| .map((part) => part.charAt(0).toUpperCase() + part.slice(1)) | ||
| .join(' '); | ||
| } | ||
|
|
||
| // Replicate the name selection logic from repairSingleRealm | ||
| function selectNextName( | ||
| currentName: string | undefined, | ||
| desiredName: string, | ||
| force: boolean, | ||
| ): string | undefined { | ||
| return force || isBadName(currentName) | ||
| ? desiredName | ||
| : currentName; | ||
| } | ||
|
|
There was a problem hiding this comment.
This test file re-implements isBadName/selectNextName/titleCaseFromEndpoint instead of exercising the real logic in src/commands/repair.ts, so it won’t catch regressions if the production code changes. Consider extracting these helpers into exported functions (or a small lib module) and importing them here, or mocking fetch to test repairSingleRealm behavior end-to-end.
| // Test the repair name logic by importing the relevant helpers | |
| // We test the exported functions indirectly through repairSingleRealm behavior | |
| // Since repairSingleRealm is not directly testable (it makes HTTP calls), | |
| // we test the name-selection logic by extracting the key functions. | |
| // Import the module to test internal logic | |
| // We use the same pattern as checkpoint-manager tests (@ts-expect-error for private access) | |
| describe('repair name logic', () => { | |
| // Replicate the isBadName logic from repair.ts | |
| function isBadName(value: unknown): boolean { | |
| if (typeof value !== 'string' || value.trim().length === 0) { | |
| return true; | |
| } | |
| return value.trim().toLowerCase() === 'unknown workspace'; | |
| } | |
| function titleCaseFromEndpoint(realmUrl: string): string { | |
| const parts = new URL(realmUrl).pathname.replace(/^\/|\/$/g, '').split('/'); | |
| const endpoint = parts[parts.length - 1] ?? 'workspace'; | |
| return endpoint | |
| .split('-') | |
| .filter(Boolean) | |
| .map((part) => part.charAt(0).toUpperCase() + part.slice(1)) | |
| .join(' '); | |
| } | |
| // Replicate the name selection logic from repairSingleRealm | |
| function selectNextName( | |
| currentName: string | undefined, | |
| desiredName: string, | |
| force: boolean, | |
| ): string | undefined { | |
| return force || isBadName(currentName) | |
| ? desiredName | |
| : currentName; | |
| } | |
| import * as fs from 'node:fs'; | |
| // Test the repair name logic by loading the real helper implementations from repair.ts | |
| // rather than duplicating them in the test. | |
| function extractFunctionDeclaration(source: string, functionName: string): string { | |
| const start = source.indexOf(`function ${functionName}`); | |
| if (start === -1) { | |
| throw new Error(`Could not find function ${functionName} in src/commands/repair.ts`); | |
| } | |
| const bodyStart = source.indexOf('{', start); | |
| if (bodyStart === -1) { | |
| throw new Error(`Could not find function body for ${functionName} in src/commands/repair.ts`); | |
| } | |
| let depth = 0; | |
| for (let index = bodyStart; index < source.length; index += 1) { | |
| const char = source[index]; | |
| if (char === '{') { | |
| depth += 1; | |
| } else if (char === '}') { | |
| depth -= 1; | |
| if (depth === 0) { | |
| return source.slice(start, index + 1); | |
| } | |
| } | |
| } | |
| throw new Error(`Could not extract complete function ${functionName} from src/commands/repair.ts`); | |
| } | |
| type RepairNameHelpers = { | |
| isBadName: (value: unknown) => boolean; | |
| titleCaseFromEndpoint: (realmUrl: string) => string; | |
| selectNextName: ( | |
| currentName: string | undefined, | |
| desiredName: string, | |
| force: boolean, | |
| ) => string | undefined; | |
| }; | |
| function loadRepairNameHelpers(): RepairNameHelpers { | |
| const source = fs.readFileSync( | |
| new URL('../../src/commands/repair.ts', import.meta.url), | |
| 'utf8', | |
| ); | |
| const helperSource = [ | |
| extractFunctionDeclaration(source, 'isBadName'), | |
| extractFunctionDeclaration(source, 'titleCaseFromEndpoint'), | |
| extractFunctionDeclaration(source, 'selectNextName'), | |
| ].join('\n\n'); | |
| return new Function( | |
| `${helperSource}\nreturn { isBadName, titleCaseFromEndpoint, selectNextName };`, | |
| )() as RepairNameHelpers; | |
| } | |
| const { | |
| isBadName, | |
| titleCaseFromEndpoint, | |
| selectNextName, | |
| } = loadRepairNameHelpers(); | |
| describe('repair name logic', () => { |
| @@ -0,0 +1,168 @@ | |||
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; | |||
There was a problem hiding this comment.
Unused imports (vi, beforeEach, afterEach) add noise and can trip stricter lint/TS settings if enabled later. Consider removing them or using them in the tests.
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; | |
| import { describe, it, expect } from 'vitest'; |
| import { describe, it, expect, beforeEach, afterEach } from 'vitest'; | ||
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
| import * as os from 'os'; |
There was a problem hiding this comment.
Unused imports (beforeEach, afterEach, fs, path, os) aren’t used anywhere in this test file. Consider removing them to keep the tests minimal and avoid future lint/TS unused-var failures.
| import { describe, it, expect, beforeEach, afterEach } from 'vitest'; | |
| import * as fs from 'fs'; | |
| import * as path from 'path'; | |
| import * as os from 'os'; | |
| import { describe, it, expect } from 'vitest'; |
| doctorCmd | ||
| .command('consolidate-workspaces') | ||
| .description('Fix local workspace directory structure (move to domain/owner/realm)') | ||
| .argument('[root-dir]', 'Root directory to scan (default: current directory)') |
There was a problem hiding this comment.
doctor consolidate-workspaces help text says the default root-dir is the current directory, but consolidateWorkspacesCommand() now defaults to defaultWorkspacesRoot() when rootDir is omitted. Consider updating the argument description (and the hidden legacy command’s description if desired) so the CLI help matches behavior.
| .argument('[root-dir]', 'Root directory to scan (default: current directory)') | |
| .argument('[root-dir]', 'Root directory to scan (default: workspaces root)') |
| it('defaults to not fixing index in batch mode', () => { | ||
| // Verify the default values match what we set in the code | ||
| const options: { fixIndex?: boolean; touchIndex?: boolean; matchEndpoint?: boolean } = {}; | ||
| const fixIndex = options.fixIndex ?? false; | ||
| const touchIndex = options.touchIndex ?? false; | ||
| const matchEndpoint = options.matchEndpoint ?? false; | ||
|
|
||
| expect(fixIndex).toBe(false); | ||
| expect(touchIndex).toBe(false); | ||
| expect(matchEndpoint).toBe(false); | ||
| }); | ||
|
|
||
| it('respects explicit opt-in for index fixing', () => { | ||
| const options = { fixIndex: true, touchIndex: true, matchEndpoint: true }; | ||
| const fixIndex = options.fixIndex ?? false; | ||
| const touchIndex = options.touchIndex ?? false; | ||
| const matchEndpoint = options.matchEndpoint ?? false; | ||
|
|
||
| expect(fixIndex).toBe(true); | ||
| expect(touchIndex).toBe(true); | ||
| expect(matchEndpoint).toBe(true); |
There was a problem hiding this comment.
The “batch defaults” tests are currently only validating how ?? false works on a local object, not the actual CLI parsing or repairRealmsCommand defaults. This is effectively a no-op regression test. Consider asserting defaults by invoking commander parsing against the real option definitions (similar to the commander regression test below), or by calling the command handler with parsed options.
| it('defaults to not fixing index in batch mode', () => { | |
| // Verify the default values match what we set in the code | |
| const options: { fixIndex?: boolean; touchIndex?: boolean; matchEndpoint?: boolean } = {}; | |
| const fixIndex = options.fixIndex ?? false; | |
| const touchIndex = options.touchIndex ?? false; | |
| const matchEndpoint = options.matchEndpoint ?? false; | |
| expect(fixIndex).toBe(false); | |
| expect(touchIndex).toBe(false); | |
| expect(matchEndpoint).toBe(false); | |
| }); | |
| it('respects explicit opt-in for index fixing', () => { | |
| const options = { fixIndex: true, touchIndex: true, matchEndpoint: true }; | |
| const fixIndex = options.fixIndex ?? false; | |
| const touchIndex = options.touchIndex ?? false; | |
| const matchEndpoint = options.matchEndpoint ?? false; | |
| expect(fixIndex).toBe(true); | |
| expect(touchIndex).toBe(true); | |
| expect(matchEndpoint).toBe(true); | |
| it('defaults batch flags from commander parsing', async () => { | |
| const { Command } = await import('commander'); | |
| const cmd = new Command() | |
| .option('--fix-index', 'Rewrite index.json/cards-grid.json starter cards') | |
| .option('--no-touch-index', 'Skip touch mutation in index.json meta') | |
| .option('--match-endpoint', 'Match names from endpoint slugs when repairing realms'); | |
| cmd.parse(['node', 'test', 'batch-file.json'], { from: 'node' }); | |
| const opts = cmd.opts(); | |
| expect(opts.fixIndex).toBeUndefined(); | |
| expect((opts.fixIndex as boolean | undefined) ?? false).toBe(false); | |
| expect(opts.touchIndex).toBe(true); | |
| expect((opts.touchIndex as boolean | undefined) ?? false).toBe(true); | |
| expect(opts.matchEndpoint).toBeUndefined(); | |
| expect((opts.matchEndpoint as boolean | undefined) ?? false).toBe(false); | |
| }); | |
| it('respects explicit batch flag opt-ins and opt-outs', async () => { | |
| const { Command } = await import('commander'); | |
| const cmd = new Command() | |
| .option('--fix-index', 'Rewrite index.json/cards-grid.json starter cards') | |
| .option('--no-touch-index', 'Skip touch mutation in index.json meta') | |
| .option('--match-endpoint', 'Match names from endpoint slugs when repairing realms'); | |
| cmd.parse(['node', 'test', '--fix-index', '--no-touch-index', '--match-endpoint', 'batch-file.json'], { | |
| from: 'node', | |
| }); | |
| const opts = cmd.opts(); | |
| expect(opts.fixIndex).toBe(true); | |
| expect((opts.fixIndex as boolean | undefined) ?? false).toBe(true); | |
| expect(opts.touchIndex).toBe(false); | |
| expect((opts.touchIndex as boolean | undefined) ?? false).toBe(false); | |
| expect(opts.matchEndpoint).toBe(true); | |
| expect((opts.matchEndpoint as boolean | undefined) ?? false).toBe(true); |
| batchSize: 10, | ||
| definitionsFirst: true, | ||
| quiet: !options.verbose, | ||
| quiet: false, |
There was a problem hiding this comment.
uploadWithBatching is now always called with quiet: false, which forces verbose batch progress output even when track is run without --verbose (and regardless of --quiet). Since the goal here is to surface result.errors, you can keep the new per-error logging while still passing a quiet value derived from CLI flags (e.g. quiet: !options.verbose) to avoid extra noise in normal usage.
| quiet: false, | |
| quiet: options.quiet || !options.verbose, |
| .description('Pull files from a Boxel workspace to a local directory') | ||
| .argument('<workspace-url>', 'The URL of the source workspace (e.g., https://app.boxel.ai/demo/)') | ||
| .argument('<local-dir>', 'The local directory to sync files to') | ||
| .argument('[local-dir]', 'The local directory to sync files to (default: ~/boxel-workspaces/<domain>/<owner>/<realm>)') |
There was a problem hiding this comment.
The help text says the default pull path is ~/boxel-workspaces/<domain>/<owner>/<realm>, but relativeStructuredPathForWorkspaceUrl() can now produce 0- or 1-segment layouts (<host>/ or <host>/<realm>/) for published realms without an owner. Consider updating the CLI argument description to reflect the actual path shapes so users aren’t surprised.
| .argument('[local-dir]', 'The local directory to sync files to (default: ~/boxel-workspaces/<domain>/<owner>/<realm>)') | |
| .argument('[local-dir]', 'The local directory to sync files to (default: ~/boxel-workspaces/<domain>/, ~/boxel-workspaces/<domain>/<realm>/, or ~/boxel-workspaces/<domain>/<owner>/<realm>, depending on the workspace URL)') |
| - **`boxel doctor` parent command** groups maintenance tasks: `doctor repair-realm`, `doctor repair-realms`, `doctor consolidate-workspaces`, `doctor force-reindex`. Keeps the root command tree focused on day-to-day sync operations. | ||
| - **`boxel workspace-list`** (formerly `boxel list`) lists Boxel workspaces the active profile can access. `boxel list` stays as a shorthand alias. | ||
| - **`boxel realms <subcommand>`** — positional subcommands replace the old flag-based interface. `boxel realms add ./path`, `realms remove ./path`, `realms init`, `realms list`, `realms llm`. `boxel realms` with no args shows the list. | ||
| - **Default workspace root at `~/boxel-workspaces/<domain>/<owner>/<realm>/`.** `boxel pull <url>` now works without a local-dir argument — it derives a canonical path from the URL and creates the directory. `doctor consolidate-workspaces` scans `~/boxel-workspaces/` by default. |
There was a problem hiding this comment.
This changelog bullet states the default workspace root layout is always ~/boxel-workspaces/<domain>/<owner>/<realm>/, but the new URL parsing explicitly supports 0- and 1-segment published realms (<host>/ and <host>/<realm>/). Consider adjusting the wording here to match the actual path derivation so the release notes don’t mislead users.
| - **Default workspace root at `~/boxel-workspaces/<domain>/<owner>/<realm>/`.** `boxel pull <url>` now works without a local-dir argument — it derives a canonical path from the URL and creates the directory. `doctor consolidate-workspaces` scans `~/boxel-workspaces/` by default. | |
| - **Default workspace root under `~/boxel-workspaces/`.** `boxel pull <url>` now works without a local-dir argument — it derives a canonical subpath from the URL and creates the directory: 0 segments → `<host>/`, 1 segment → `<host>/<realm>/`, 2+ segments → `<host>/<owner>/<realm>/`. `doctor consolidate-workspaces` scans `~/boxel-workspaces/` by default. |
PR review changes (P1–P4): - Show push error details in track --push (quiet: false + error loop) - Conservative defaults for repair-realms (fixIndex/touchIndex/matchEndpoint off) - Full realm server hostname in folder naming (no normalization) - Simplified name selection: only overwrite bad names, not mismatched ones - Validate realms remove (error if path not found) - Rename list → workspace-list (with list alias) - Migrate realms to positional subcommands (realms add/remove/init/llm) - Add boxel doctor parent command (repair-realm, repair-realms, consolidate-workspaces, force-reindex) - Hidden backwards-compat aliases for all renamed commands Default workspace root: - Add defaultWorkspacesRoot() → ~/boxel-workspaces/ (all platforms) - Wire into workspace-resolver, consolidate, legacy path warning - Make pull local-dir argument optional (defaults to ~/boxel-workspaces/) Documentation: - Update all .md files with new command nomenclature - Add terminology section (realm vs workspace) to README - Document ~/boxel-workspaces/ as default root everywhere - Update onboarding flow to use boxel pull <url> Tests: - 28 new tests: workspace-paths, realm-config, repair (isBadName, selectNextName, batch defaults) Co-Authored-By: Claude Opus 4.6 <[email protected]>
Commander's .option('--no-fix-index') makes the underlying boolean
default to TRUE, which was the opposite of what PR #15 advertised.
Running `boxel doctor repair-realm <url>` silently rewrote
index.json/cards-grid.json by default, destroying customized index
files (caught by backspace on Checkly-prerendered realm).
Flip to .option('--fix-index') across all 4 command surfaces:
- repair-realm (hidden top-level)
- repair-realms (hidden top-level)
- doctor repair-realm
- doctor repair-realms
Now the default matches the documentation: index.json is untouched
unless --fix-index is explicitly passed.
Added 2 regression tests that exercise the commander parsing directly
(the previous tests only checked the handler-level fallback, which
ran AFTER commander had already set fixIndex=true).
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Buck reported on PR #15 that `boxel pull <url>` without a local-dir argument produced weird paths for published realms: - 1-segment URL https://realms-staging.stack.cards/boxel-homepage/ → ~/boxel-workspaces/realms-staging.stack.cards/boxel-homepage/boxel-homepage (the fallback `parts[1] ?? parts[0]` duplicated the realm name as owner) - 0-segment URL https://gabbro.staging.boxel.build/ → ~/boxel-workspaces/gabbro.staging.boxel.build/unknown-owner/workspace (invented placeholder segments) Fix: adapt the layout depth to match what the URL actually provides. - 0 segments → <host>/ - 1 segment → <host>/<realm>/ - 2+ segments → <host>/<owner>/<realm>/ (unchanged) findManifestPaths() now also walks 2-level <root>/<domain>/<realm>/ layouts so the legacy-path warning and doctor consolidate-workspaces still detect all manifests after the layout change. Added 4 regression tests covering both edge cases. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
PR #15 introduced positional subcommands (realms add/remove/init/llm) and kept the old flag forms as hidden aliases for backwards compat. Since the restructure has not yet been released (still on PR), there are no users to migrate. Clean break is simpler. Per backspace's PR #15 review: "do we want backward compatibility? IMO we might as well make a clean break as this is not officially released." - Remove 9 `.addOption(new Option('--xxx').hideHelp())` calls - Remove legacy flag dispatcher `realmsCommand()` and RealmsOptions - Drop unused `Option` import from commander `boxel realms` with no subcommand still shows the list. The `list` alias for `workspace-list` stays (backspace: "I think we can treat list as a shorthand"). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Minor-version bump (not patch) because PR #15 changes visible command names: `list` → `workspace-list`, `realms --add` → `realms add`, `boxel doctor` parent for repair/consolidate. Plus the two bug fixes @backspace caught on review. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Review follow-ups for PR #15: - docs/realm-repair.md still documented the old `--no-fix-index` form after b9646da flipped the flag to `--fix-index`. Updated the description to match the new opt-in behavior and explain why it's off by default (breaks Checkly-style prerendering). - test/commands/repair.test.ts had a misleading block titled "repair-realms batch defaults" that actually tested the handler's `?? false` fallback, not the commander CLI defaults. Renamed and clarified so it's obvious what it covers; the real commander-default coverage lives in the "commander flag parsing" block added in b9646da. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
christse
force-pushed
the
pr-review/command-restructure-and-defaults
branch
from
8f81ff8 to
357fa77
Compare
|
Rebased onto main post-1.0.2 and addressed review nits I caught while self-reviewing. New commit
Rebase notes: Straightforward conflicts in Final state: 199/199 tests pass, head at Ready to merge. |
Summary
Implements 11 review suggestions from Buck (P1–P4), plus adds
~/boxel-workspaces/as the cross-platform default workspace root.Command restructuring
boxel doctorparent command groups maintenance tools:repair-realm,repair-realms,consolidate-workspaces,force-reindex— signals these are "fix things" commands, not daily workflow (likebrew doctor)boxel workspace-list(renamed fromlist) — clarifies this fetches your UI workspace list from Matrix, not server-side realm configboxel realmsmigrated to positional subcommands (add,remove,init,llm) — matchesboxel profilepatternSafety improvements
repair-realmsno longer overwritesindex.jsonby default — prevents destroying customized index files (was breaking Checkly prerendering). Opt-in with--fix-indextrack --pushnow shows error details when uploads fail — previously swallowedresult.errorsarrayrealms removevalidates the path exists before removingDefault workspace root:
~/boxel-workspaces/~/boxel-workspaces/as the default rootpullno longer requires a local-dir argument — defaults to~/boxel-workspaces/<domain>/<owner>/<realm>realms-staging.stack.cards/, notstack.cards/) eliminates staging/production ambiguityconsolidate-workspacesand legacy path warnings default to scanning~/boxel-workspaces/Terminology: Realm vs Workspace
realms,repair-realm,.realm.json)workspace-list,consolidate-workspaces,.boxel-workspaces.json)Documentation
.claude/commands/*.mdskills,AGENTS.md,CLAUDE.md,README.md,docs/realm-repair.mdupdated with new command namesboxel pull <url>(no local path needed)Tests
28 new tests covering:
defaultWorkspacesRoot()returns~/boxel-workspaces/isBadName()/selectNextName()— valid names are NOT overwrittenfixIndex,touchIndex,matchEndpointall false)removeRealmvalidationTest plan
boxel pull <url>without local-dir defaults to~/boxel-workspaces/<domain>/<owner>/<realm>boxel doctor consolidate-workspacesscans~/boxel-workspaces/by defaultboxel doctor repair-realmsdoes NOT overwriteindex.jsonunless--fix-indexpassedboxel realms add ./pathworks (positional subcommand)boxel realms --add ./pathstill works (hidden backwards compat)boxel workspace-listandboxel listboth worktrack --pushshows error details on failure🤖 Generated with [Claude Code](https://claude.com/claude-code)