feat: add Phase 5 deployment commands

[menor]

Summary

• Add 6 deployment commands for environment lifecycle management
• Add --wait flag for activity polling until completion
• Fix API response parsing for _embedded.activities wrapper
• Update smoke tests (23 tests passing against real API)

New Commands

Command	Description
environment:branch	Create branch environments
environment:activate	Activate inactive environments
environment:deactivate	Deactivate active environments
environment:delete	Delete environments
redeploy	Trigger redeployment (post_deploy hook only)
push	Push code to Upsun git remote

Smoke Test Results

=== Deployment Commands ===
PASS: redeploy (activity: u5tteru7l3n6q)
PASS: environment:branch (created smoke-test)
PASS: environment:deactivate (smoke-test)
PASS: environment:activate (smoke-test)

Test plan

• Unit tests pass (108 tests)
• Smoke tests pass (23 tests against real Upsun project)
• redeploy --wait polls until activity completes
• environment:branch --wait creates and waits for branch

🤖 Generated with Claude Code

[menor]

Code Review: PR #8 - Phase 5 Deployment Commands

Summary

Adds 6 deployment-related commands:

• environment:branch, environment:activate, environment:deactivate, environment:delete
• redeploy, push

Plus --wait flag for activity polling.

Stats: +1054/-7 lines, 9 files changed

---

Verdict: ⚠️ Merge with Suggestions

Functional and well-tested, but has naming/organization concerns worth addressing before the codebase grows.

---

Architecture Concerns

1. File Naming: deploy.go is misleading

The file contains two distinct concerns:

• Environment lifecycle: branch, activate, deactivate, delete (operations on environments)
• Deployment triggers: redeploy, push (triggering builds)

Suggestion: Split into:

• cmd/environment.go - Add lifecycle commands to existing file (or create environment_lifecycle.go)
• cmd/deploy.go - Keep only redeploy
• cmd/git.go - Move push (it's really a git operation)

Or at minimum, rename to cmd/environment_actions.go to match what it actually contains.

2. Repeated Boilerplate - DRY violation

The same validation pattern appears in 6+ commands:

projectID := ctx.ProjectID()
if projectID == "" {
    return errors.NewValidationError("no project specified").
        WithHint("Use --project or run from within a project directory")
}

And environment ID resolution appears 5 times:

envID := c.EnvironmentID
if envID == "" {
    envID = ctx.EnvironmentID()
    if envID == "" {
        return errors.NewValidationError("no environment specified")...
    }
}

Suggestion: Add helpers to Context:

func (c *Context) RequireProjectID() (string, error)
func (c *Context) ResolveEnvironmentID(explicit string) (string, error)

3. waitForActivity placement

This function is defined in deploy.go but will be useful for any async operation (future: backup:create, integration:add). Should live in:

• cmd/helpers.go, or
• As a Context method: ctx.WaitForActivity(client, projectID, activityID)

4. Missing activity failure state

switch activity.State {
case "complete":
    return activity, nil
case "cancelled":
    return activity, errors.NewValidationError("activity was cancelled")
// Missing: "failure" state - will poll until 30min timeout
}

5. Unused --yes flag on environment:delete

Flag is defined but never checked. Since Sol is agent-first with no interactive prompts, either remove it or document it's for compatibility.

---

What's Good

• Security: Proper url.PathEscape() on all paths
• Context-aware execution: exec.CommandContext for git
• Testing: Comprehensive unit tests (383 lines)
• Smoke tests: Real API tests for the full lifecycle
• API design: Clean embeddedActivitiesResponse for handling API wrapper

---

Files Changed

File	Purpose
cmd/deploy.go	6 commands + waitForActivity
cmd/deploy_test.go	Unit tests
cmd/cli.go	Register commands
internal/api/environments.go	API methods
internal/api/interfaces.go	Interface updates
internal/api/mock.go	Mock implementations
scripts/smoke-test.sh	Extended tests
README.md	Documentation

---

Recommended Action

This works as-is, but consider addressing architecture points before adding more commands. The repeated patterns will compound as the codebase grows.

Minimal fix: Rename deploy.go → environment_actions.go

Better fix: Extract helpers + split file by concern

[menor]

Code Review: PR #8 - Phase 5 Deployment Commands (Updated)

Summary

Adds 6 deployment commands with proper architecture:

• Environment lifecycle: environment:branch, environment:activate, environment:deactivate, environment:delete
• Deployment triggers: redeploy, push
• --wait flag for activity polling

Stats: +1054 lines, 11 files changed

---

Verdict: ⚠️ Needs Minor Fixes

One bug found (--wait not implemented in push). Otherwise well-architected.

---

Architecture Review: ✅ Good

File Organization (Fixed)

File	Purpose
cmd/environment.go	All environment commands (list, info, branch, activate, deactivate, delete)
cmd/deploy.go	Just RedeployCmd (44 lines)
cmd/git.go	Just PushCmd (68 lines)
cmd/context.go	Shared helpers

This is a clean separation by resource/concern.

Helpers Extracted to Context: ✅

// cmd/context.go
func (c *Context) RequireProjectID() (string, error)
func (c *Context) ResolveEnvironmentID(explicit string) (string, error)
func (c *Context) WaitForActivity(client, projectID, activityID) (*Activity, error)

These eliminate the repeated boilerplate. Commands are now concise:

func (c *RedeployCmd) Run(ctx *Context) error {
    projectID, err := ctx.RequireProjectID()  // One line instead of 5
    envID, err := ctx.ResolveEnvironmentID(c.EnvironmentID)  // One line instead of 7
    ...
}

WaitForActivity: ✅ Handles Edge Cases

switch activity.State {
case "complete":
    if activity.Result == "failure" {  // Complete but failed
        return activity, errors.NewInternalError("activity failed")
    }
    return activity, nil
case "cancelled":
    return activity, errors.NewValidationError("activity was cancelled")
case "failure":  // Direct failure state
    return activity, errors.NewInternalError("activity failed")
}

Handles: complete+success, complete+failure, cancelled, failure state, timeout, context cancellation.

---

Issues Found

🔴 Bug: PushCmd.Wait flag defined but never used (cmd/git.go:14,56)

type PushCmd struct {
    Target string `help:"Target branch" short:"t"`
    Force  bool   `help:"Force push" short:"f"`
    Wait   bool   `help:"Wait for the activity to complete" short:"w"`  // DEFINED
}

func (c *PushCmd) Run(ctx *Context) error {
    // ...
    if err := execGit(ctx, args...); err != nil { ... }
    return ctx.Output(...)  // c.Wait NEVER CHECKED
}

Fix: Either implement waiting (requires API call to find triggered activity) or remove the flag.

🟡 Minor: execGit doesn't verify git is installed (cmd/git.go:101-106)

func execGit(ctx *Context, args ...string) error {
    cmd := exec.CommandContext(ctx, "git", args...)  // Will fail cryptically if git not in PATH
    ...
}

Suggestion: Add exec.LookPath("git") check with clear error:

gitPath, err := exec.LookPath("git")
if err != nil {
    return errors.NewValidationError("git not found").WithHint("Install git to use push command")
}

🟡 Minor: API errors use fmt.Errorf instead of structured errors

In internal/api/environments.go (lines 941, 959, 975, 1001):

if len(resp.Embedded.Activities) == 0 {
    return nil, fmt.Errorf("no activity returned")  // Generic error
}

Should use structured error for consistency:

return nil, &APIError{Message: "no activity returned from API", StatusCode: 0}

🟡 Minor: Missing tests for --wait behavior and failure states

Tests cover success paths but not:

• --wait flag actually waiting
• Activity failure during wait
• Context cancellation during wait

---

Edge Cases Handled: ✅

Edge Case	Handled
No project specified	✅ RequireProjectID() returns validation error
No environment specified	✅ ResolveEnvironmentID() returns validation error
Activity completes with failure	✅ Checks activity.Result == "failure"
Activity cancelled	✅ Returns error
Activity state "failure"	✅ Returns error
Wait timeout (30 min)	✅ Returns timeout error
Context cancelled during wait	✅ Returns ctx.Err()
Project has no repository URL	✅ Returns validation error
Parent environment doesn't exist	✅ API returns 404, handled by handleAPIError
Delete active environment	✅ API returns 400, handled by handleAPIError

---

Security: ✅

• url.PathEscape() on all path parameters
• exec.CommandContext respects context cancellation
• No shell injection (args passed directly, not through shell)

---

Checklist

• Proper file organization (environment.go, deploy.go, git.go)
• Helpers extracted to Context
• WaitForActivity handles all activity states
• Security (path escaping, no shell injection)
• Unit tests for happy paths
• --wait flag implemented in PushCmd
• exec.LookPath check for git
• Tests for wait/failure edge cases

---

Recommended Actions

Must fix before merge:

1. Either implement --wait in PushCmd or remove the flag (misleading API)

Nice to have:
2. Add exec.LookPath("git") check
3. Use structured errors in API layer

[menor]

Code Review: PR #8 - Phase 5 Deployment Commands (Re-review)

Summary

Adds 6 deployment commands with --wait support for activity polling.

Stats: +1399/-24 lines, 13 files changed, 4 commits

---

Verdict: ✅ Ready to Merge

All previous review issues addressed. Architecture is sound.

---

Issues Fixed Since Last Review

Issue	Status
--wait flag on PushCmd unused	✅ Removed
exec.LookPath check for git	✅ Added with clear error message
fmt.Errorf → structured APIError	✅ Fixed in all API methods
Tests for WaitForActivity edge cases	✅ Added 339 lines of tests
Unused --yes flag on delete	✅ Removed

---

Architecture Review: ✅ Excellent

File Organization

cmd/
  environment.go    # list, info, branch, activate, deactivate, delete
  deploy.go         # redeploy (44 lines)
  git.go            # push (73 lines)
  context.go        # RequireProjectID, ResolveEnvironmentID, WaitForActivity
  context_test.go   # 339 lines of helper tests
  deploy_test.go    # 383 lines of command tests

Clean separation by resource/concern.

Context Helpers

Commands are concise thanks to extracted helpers:

func (c *RedeployCmd) Run(ctx *Context) error {
    projectID, err := ctx.RequireProjectID()      // Validates project
    envID, err := ctx.ResolveEnvironmentID(...)   // Resolves from arg/flag/env
    activity, err := ctx.WaitForActivity(...)     // Polls until complete
}

WaitForActivity - All States Handled

switch activity.State {
case "complete":
    if activity.Result == "failure" { return error }  // Complete but failed
    return activity, nil                               // Success
case "cancelled": return error
case "failure": return error
}
// + timeout handling
// + context cancellation

---

Error Handling: ✅ Comprehensive

Scenario	Handling
No project specified	RequireProjectID() returns validation error with hint
No environment specified	ResolveEnvironmentID() returns validation error with hint
Git not installed	exec.LookPath check with "Install git" hint
No repository URL	Validation error with "enable Git integration" hint
Activity fails	Error with "Check activity:log" hint
Activity cancelled	Validation error with activity_id
API returns no activity	Structured APIError{Message: "no activity returned"}
Context cancelled	Returns ctx.Err()
30 min timeout	Validation error with activity_id and state

---

Test Coverage: ✅ Good

cmd/context_test.go (339 lines)

• TestWaitForActivity_Success
• TestWaitForActivity_CompleteWithFailure
• TestWaitForActivity_FailureState
• TestWaitForActivity_Cancelled
• TestWaitForActivity_ContextCancellation
• TestWaitForActivity_APIError
• TestWaitForActivity_PollsUntilComplete
• TestRequireProjectID_Success/FromEnv/Missing
• TestResolveEnvironmentID_Explicit/FromFlag/FromEnv/Missing

cmd/deploy_test.go (383 lines)

• All command happy paths
• No project/environment specified
• API errors

---

Security: ✅

• url.PathEscape() on all API path parameters
• exec.CommandContext respects context cancellation
• exec.LookPath validates git binary exists
• No shell injection (args passed directly to exec)

---

Edge Cases Verified

Edge Case	Handled
Activity state "complete" + result "failure"	✅ Returns error
Activity state "failure"	✅ Returns error
Activity state "cancelled"	✅ Returns error
Polling timeout (30 min)	✅ Returns error with state
Context cancelled mid-poll	✅ Returns ctx.Err()
API error during poll	✅ Propagates error
Git not in PATH	✅ Clear error message
Empty repository URL	✅ Validation error
Delete active environment	✅ API returns 400, handled

---

Checklist

• Clean file organization (environment.go, deploy.go, git.go)
• Helpers extracted to Context (DRY)
• WaitForActivity handles all activity states
• Structured errors with hints throughout
• Security (path escaping, exec.LookPath, no shell injection)
• Comprehensive unit tests (722 new lines)
• Smoke tests updated
• Documentation updated

---

Ship it! 🚀