Add workspaces_enabled toggle to gate the control-room workspaces step

[stevenolen]

Description

The control-room workspaces step (AWS WorkSpaces virtual desktops) was unconditionally registered in ControlRoomSteps and always provisioned a WorkSpaces environment for every control room, with no way to opt out. This adds a workspaces_enabled config toggle so a control room can turn the step off.

Code Flow

• AWSControlRoomConfig gains WorkspacesEnabled *bool (workspaces_enabled). It is a pointer so an absent field is distinguishable from an explicit false.
• WorkspacesIsEnabled() resolves it, defaulting to on when unset (nil -> true), mirroring EKSAccessEntriesConfig.IsEnabled(). This preserves the historical always-on behavior; only an explicit workspaces_enabled: false disables the step.
• WorkspacesStep.Run loads the control-room config (via helpers.ConfigForTarget, the same way runAWSInlineGo does) right after the ControlRoom() validation and before any credential fetch or Pulumi stack construction. When disabled it logs and returns nil, so both the apply and --destroy paths short-circuit identically.

Category of change

• New feature (non-breaking change which adds functionality)

Checklist

• I have reviewed my own diff

[claude]

Claude finished @stevenolen's task in 2m 35s —— View job

---

Review

• Load review guidelines
• Read changed files
• Analyze diff
• Post review

---

The implementation is clean and follows existing patterns well. One correctness concern worth addressing before merge, plus two minor observations posted as inline comments.

Correctness concern (inline on workspaces.go:63): The --destroy path silently no-ops when workspaces_enabled: false, which will orphan real AWS resources if the toggle is set on a control room that still has an active WorkSpaces stack. The PR comment says "once the stack has been destroyed it stays gone" — true, but that assumes the operator destroyed before toggling. The flag doesn't enforce that ordering. At minimum, a slog.Warn when s.Options.Destroy is true and the gate fires would make the silent skip visible in logs.

Minor observations (non-blocking):

• workspaces.go:51: Config is parsed twice on the apply path (Run + runAWSInlineGo). Not a performance issue, but it's a dual-maintenance point if error formats ever diverge.
• controlroom.go:94: Value receiver is correct here, but the doc comment's comparison to EKSAccessEntriesConfig.IsEnabled is slightly misleading — that method has a pointer receiver and guards on c == nil, which doesn't apply to AWSControlRoomConfig.

[claude]

The destroy path has a silent-orphan risk: if a control room already has an active WorkSpaces Pulumi stack (real AWS resources) and someone then sets workspaces_enabled: false, any subsequent ptd ensure --destroy will hit this early return and leave those resources in AWS forever — the stack is never touched again.

The comment says "once the stack has been destroyed it stays gone", which assumes the operator already tore down before toggling, but that ordering isn't enforced. Consider at minimum a warning log when Destroy is set in Options and the gate fires:

if !cfg.WorkspacesIsEnabled() {
    if s.Options.Destroy {
        slog.Warn("workspaces step disabled but --destroy was requested; existing WorkSpaces stack will NOT be destroyed",
            "target", s.DstTarget.Name())
    } else {
        slog.Info("skipping workspaces step: workspaces_enabled is false for this control room",
            "target", s.DstTarget.Name())
    }
    return nil
}

[claude]

runAWSInlineGo (in workspaces_aws.go:49-56) does the exact same ConfigForTarget + type-assert, so the config is parsed twice on the apply path. The struct is cheap to parse, so this isn't a performance issue, but it's a consistency hazard: if the type-assert error message or the config-load error format ever diverges between the two call sites, debugging gets confusing.

Not blocking, but worth noting if this pattern gets copied further.

[claude]

Minor inconsistency with EKSAccessEntriesConfig.IsEnabled() (line 21), which has a pointer receiver (c *EKSAccessEntriesConfig) that also handles c == nil. WorkspacesIsEnabled uses a value receiver (c AWSControlRoomConfig), which is fine since AWSControlRoomConfig is never used via pointer here, but the parallel drawn in the doc comment is slightly misleading — the nil-receiver guard in IsEnabled doesn't apply here.

[Lytol]

Nice.