fix(brev): reject invalid sandbox names before deploy

[ericksoa]

fixes #2869

Summary

• Route Brev deploy sandbox-name validation through a plain early error instead of letting the canonical validator escape as terse UX.
• Share the sandbox-name allowed-format wording with onboard so prompts and non-interactive errors say: lowercase, starts with a letter, letters/numbers/internal hyphens only, ends with letter/number.
• Add regression coverage that invalid NEMOCLAW_SANDBOX_NAME fails before Brev provisioning, SSH, or installer handoff.

What was late/missing before

Brev deploy did call validateName, but invalid env input surfaced as a raw validation failure without Brev-specific guidance or an explicit non-interactive correction path. The onboard prompt also used shorter wording that did not state the complete rule before entry.

Where Brev now validates early

src/lib/deploy.ts validates NEMOCLAW_SANDBOX_NAME immediately after resolving deploy flags and before credentials, brev, SSH, rsync, or installer work. Invalid names with spaces now print that spaces are not allowed and tell the user to set a valid NEMOCLAW_SANDBOX_NAME.

Tests run

• npm run build:cli
• npx vitest run src/lib/deploy.test.ts
• npx vitest run test/e2e/brev-e2e.test.ts
• npx vitest run test/onboard.test.ts -t "sandbox name|invalid sandbox|re-prompts"
• npx vitest run test/smoke-macos-install.test.ts -t "invalid sandbox"
• git diff --check

Summary by CodeRabbit

• New Features

  • Standardized sandbox name validation guidance across deploy and onboarding commands
  • Help and interactive prompts now display the allowed sandbox name format

• Bug Fixes

  • Clearer, consistent error messages when sandbox names are invalid
  • Deploy shows explicit non-interactive guidance and prevents provisioning on invalid names

• Tests

  • Added deploy validation e2e test and extended sandbox name validation tests

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c54e26e3-a023-4ed9-b4d1-ece675d170e6

📥 Commits

Reviewing files that changed from the base of the PR and between 7834342 and bffab59.

📒 Files selected for processing (2)

• src/lib/deploy.test.ts
• src/lib/runner.ts

✅ Files skipped from review due to trivial changes (2)

• src/lib/runner.ts
• src/lib/deploy.test.ts

---

📝 Walkthrough

Walkthrough

A new centralized name-validation module (name-validation.ts) defines consistent guidance messages and allowed-format text. runner.ts, deploy.ts, and onboard.ts consume it to standardize sandbox-name validation output for interactive and non-interactive flows. Tests were added/updated to assert the guidance appears and invalid names are rejected early.

Changes

Centralized Name Validation Guidance

Layer / File(s)	Summary
Core Validation Data src/lib/name-validation.ts	New module exports NAME_ALLOWED_FORMAT and getNameValidationGuidance(label, value, opts?) to produce consistent guidance lines (includes whitespace check and optional "Allowed format" line).
Validation Message Update src/lib/runner.ts	validateName() error messages now append Allowed format: ${NAME_ALLOWED_FORMAT} for required/too-long/invalid-format cases.
Deploy Flow Integration src/lib/deploy.ts	Adds validateDeploySandboxName(...) that wraps validateName and on failure prints the validation error plus guidance (without allowed-format) and a non-interactive remediation note; executeDeploy uses this helper. Usage/help text now includes NAME_ALLOWED_FORMAT.
Onboard Flow Integration src/lib/onboard.ts	promptValidatedSandboxName() and early non-prompt validation now use getNameValidationGuidance("sandbox name", value, { includeAllowedFormat: false }) instead of bespoke messages.
Tests & E2E src/lib/deploy.test.ts, test/onboard.test.ts, test/e2e/brev-e2e.test.ts	Updated and new tests import deployed validation, assert guidance usage and that prompts/display include NAME_ALLOWED_FORMAT, and assert non-interactive deploy exits early without starting provisioning when name is invalid.

Sequence Diagram

sequenceDiagram
    actor User
    participant Onboard as onboard()
    participant Deploy as deploy.ts
    participant Validate as validateName()
    participant Guidance as getNameValidationGuidance()

    User->>Onboard: Enter sandbox name
    Onboard->>Validate: validateName(input, "sandbox name")
    alt Invalid Name
        Validate-->>Onboard: Throw ValidationError
        Onboard->>Guidance: getNameValidationGuidance("sandbox name", input, includeAllowedFormat: false)
        Guidance-->>Onboard: Return guidance lines
        Onboard-->>User: Show guidance & retry or abort
    else Valid Name
        Validate-->>Onboard: ✓
        Onboard->>User: Proceed
    end

    User->>Deploy: Run deploy with NEMOCLAW_SANDBOX_NAME
    Deploy->>Validate: validateDeploySandboxName(envValue)
    alt Invalid Name
        Validate-->>Deploy: Throw ValidationError
        Deploy->>Guidance: getNameValidationGuidance("sandbox name", envValue, includeAllowedFormat: false)
        Guidance-->>Deploy: Return guidance lines
        Deploy-->>User: Print error & exit(1)
    else Valid Name
        Validate-->>Deploy: ✓
        Deploy->>User: Proceed with provisioning
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 A hop, a check, a gentle chime,

No spaces now, one steady rhyme,
From prompt to deploy the message clear,
One format shown for all to hear,
A tidy name, a smoother year.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: adding validation to reject invalid sandbox names before Brev deployment.
Linked Issues check	✅ Passed	Changes comprehensively address issue #2869: validation is performed early before provisioning, guidance text is explicit and consistent, and Brev behavior aligns with Spark path requirements.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to sandbox-name validation for deploy and onboard flows; no extraneous modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/brev-sandbox-name-validation

---

_{Review rate limit: 9/10 reviews remaining, refill in 6 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/deploy.test.ts (1)

240-248: ⚡ Quick win

Assert the env-var remediation hint explicitly.

This test validates format guidance well, but it does not lock the promised “set a valid NEMOCLAW_SANDBOX_NAME” remediation text. Adding one assertion will catch regressions in the user-facing guidance path.

Proposed assertion addition

     expect(errorText).toContain(
       "Brev deploy is non-interactive and cannot prompt for a corrected sandbox name.",
     );
+    expect(errorText).toMatch(/set\s+.*NEMOCLAW_SANDBOX_NAME/i);
     expect(fixture.calls).toEqual([]);
     expect(fixture.interactive).toEqual([]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/deploy.test.ts` around lines 240 - 248, Add an assertion to the test
that verifies the remediation hint about setting the NEMOCLAW_SANDBOX_NAME env
var is present in the aggregated error text: locate the test that builds
errorText from fixture.errors (variable errorText) in deploy.test.ts and add an
expect(errorText).toContain(...) checking for the exact remediation message that
tells users to set a valid NEMOCLAW_SANDBOX_NAME.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/lib/deploy.test.ts`:
- Around line 240-248: Add an assertion to the test that verifies the
remediation hint about setting the NEMOCLAW_SANDBOX_NAME env var is present in
the aggregated error text: locate the test that builds errorText from
fixture.errors (variable errorText) in deploy.test.ts and add an
expect(errorText).toContain(...) checking for the exact remediation message that
tells users to set a valid NEMOCLAW_SANDBOX_NAME.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fb092237-52a4-425d-8e02-99f18ef0bd82

📥 Commits

Reviewing files that changed from the base of the PR and between 4f7f7e5 and 7834342.

📒 Files selected for processing (7)

• src/lib/deploy.test.ts
• src/lib/deploy.ts
• src/lib/name-validation.ts
• src/lib/onboard.ts
• src/lib/runner.ts
• test/e2e/brev-e2e.test.ts
• test/onboard.test.ts

[github-actions]

❌ Brev E2E (full): FAILED on branch fix/brev-sandbox-name-validation — See logs