Skip to content

fix(devbox): pin workspace_region on coder update#60483

Merged
rnegron merged 5 commits into
masterfrom
worktree-devbox-outdated-gate
May 28, 2026
Merged

fix(devbox): pin workspace_region on coder update#60483
rnegron merged 5 commits into
masterfrom
worktree-devbox-outdated-gate

Conversation

@rnegron
Copy link
Copy Markdown
Member

@rnegron rnegron commented May 28, 2026

Problem

When coder update runs against a workspace whose template has had its workspace_region option set modified since the workspace was created (e.g. the recent addition of eu-central-1), Coder surfaces an interactive picker to select the new value. Per Coder's docs: "when template authors modify parameter options (add, remove, or substitute values), workspace users will be prompted to select the new value." Neither --use-parameter-defaults nor --yes bypasses this picker — it's a parameter-collection prompt, not a confirmation. (For the same reason, coder create had to start forwarding --preset explicitly; see comment at coder.py:33.)

When the picker fires in an environment that can't deliver stdin — for example an IDE's read-only output channel — the command wedges with no way out. Reported in the wild this morning after a manual hogli devbox:update.

Note: coder update doesn't even accept --yes (confirmed in coder/cli/update.go — it only registers parameterFlags.allOptions() and bflags.cliOptions(), neither of which contains cliui.SkipPromptOption()). Earlier commits on this branch tried to add --yes to coder update; that would have broken the command outright with an unknown-flag error. Reverted.

Changes

  • New helper _pinned_region_param(workspace) in cli.py: reads the workspace's current region from coder metadata, defaulting to us-east-1 for boxes that predate the region metadata item.
  • _sync_workspace_parameters(name, workspace) now takes the workspace dict and merges the pinned region into the params dict — only when other params are being pushed, preserving the "skip when nothing to sync" fast path.
  • devbox:update always merges the pinned region into its params dict (it always invokes coder update).
  • Comment block at coder.py:48 rewritten to explain why region is forwarded on update — the previous comment's "immutable, so never forward on update" logic is what allowed the bug to land.
  • The retry shim in _run_with_param_retry already handles the template-no-longer-declares-this-parameter case, so this is forward-compatible with template changes.

(Earlier commits on this branch went through a Y/n gate on devbox:start and an incorrect --yes addition before landing on the actual fix. Cumulative branch diff against master is only the changes described above — happy to squash on merge.)

How did you test this code?

I'm an agent. No manual run on a real devbox.

  • Updated existing tests test_syncs_then_starts_when_stopped and test_devbox_update_applies_when_outdated to assert the pinned region appears in the params dict.
  • Added test_sync_pins_workspace_region_from_metadata_to_suppress_picker exercising the metadata-driven region pin.
  • Existing test_update_paths_drop_unknown_params_and_retry already covers the case where Coder rejects a forwarded parameter — confirms workspace_region is dropped cleanly if the template removes it.
  • Full test_devbox.py suite: 241 passed.
  • ruff check, ruff format --check, pre-commit (bin/hogli lint:python:fix, bin/hogli format:python, uv run ty check) all green.

Publish to changelog?

no

Docs update

n/a

🤖 Agent context

  • Tooling: Claude Code (Opus 4.7), /wt worktree skill, /code-review for the calibration pass that surfaced the wrong-flag issue.
  • Two false starts before the right fix: (1) a Y/n gate on devbox:start based on a misread of the symptom, (2) adding --yes to coder update based on a misread of which Coder prompt was wedging. Both reverted on this branch. The actual mechanism is the rich-parameter picker that fires when a template's option set changes, which only goes away when callers forward the current value explicitly — same pattern as --preset on create.
  • Verified coder update's flag set by reading coder/cli/update.go and coder/cli/parameter.go upstream; --yes is not a valid flag for update (it's a per-command option attached via cliui.SkipPromptOption() on commands like start/stop/restart/delete).
  • Considered widening the region pin to also fire when no other params need pushing (i.e. always invoke coder update on devbox:start to clear any pending parameter prompt proactively). Out of scope: that adds a Coder round-trip on every start without addressing a known wedge surface — left as a follow-up if reviewers want it.

@rnegron rnegron changed the title feat(devbox): gate template update on devbox:start fix(devbox): pass --yes to coder update May 28, 2026
@rnegron rnegron changed the title fix(devbox): pass --yes to coder update fix(devbox): pin workspace_region on coder update May 28, 2026
@rnegron rnegron marked this pull request as ready for review May 28, 2026 18:43
@rnegron rnegron added the stamphog Request AI review from stamphog label May 28, 2026
@assign-reviewers-posthog assign-reviewers-posthog Bot requested a review from a team May 28, 2026 18:43
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Clean fix to internal developer tooling: pins the current workspace region parameter on coder update to suppress an interactive region picker. Author is on the owning team, tests cover both the default-region and EU-region cases, and there are no production, security, or data-model concerns.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 28, 2026

Comments Outside Diff (1)

  1. tools/hogli-commands/hogli_commands/tests/test_devbox.py, line 2199-2240 (link)

    P2 New region-pin test not parametrised alongside existing case

    The new test_sync_pins_workspace_region_from_metadata_to_suppress_picker test and the existing test_syncs_workspace_parameters_before_starting_stopped_workspace test exercise _start_existing_workspace with the same structure — the only meaningful differences are the workspace metadata dict (no metadata → DEFAULT_REGION vs. eu-central-1 in metadata → "eu-central-1") and the resulting expected workspace_region value. Per the team's preference, these two cases would naturally belong together as a single @pytest.mark.parametrize-driven test. Similarly, test_devbox_update_applies_when_outdated only covers the DEFAULT_REGION path for devbox_update; a second parametrised case with non-default region metadata would confirm that path reads region from the workspace dict too.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: tools/hogli-commands/hogli_commands/tests/test_devbox.py
    Line: 2199-2240
    
    Comment:
    **New region-pin test not parametrised alongside existing case**
    
    The new `test_sync_pins_workspace_region_from_metadata_to_suppress_picker` test and the existing `test_syncs_workspace_parameters_before_starting_stopped_workspace` test exercise `_start_existing_workspace` with the same structure — the only meaningful differences are the workspace metadata dict (no metadata → `DEFAULT_REGION` vs. `eu-central-1` in metadata → `"eu-central-1"`) and the resulting expected `workspace_region` value. Per the team's preference, these two cases would naturally belong together as a single `@pytest.mark.parametrize`-driven test. Similarly, `test_devbox_update_applies_when_outdated` only covers the `DEFAULT_REGION` path for `devbox_update`; a second parametrised case with non-default region metadata would confirm that path reads region from the workspace dict too.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
tools/hogli-commands/hogli_commands/tests/test_devbox.py:2199-2240
**New region-pin test not parametrised alongside existing case**

The new `test_sync_pins_workspace_region_from_metadata_to_suppress_picker` test and the existing `test_syncs_workspace_parameters_before_starting_stopped_workspace` test exercise `_start_existing_workspace` with the same structure — the only meaningful differences are the workspace metadata dict (no metadata → `DEFAULT_REGION` vs. `eu-central-1` in metadata → `"eu-central-1"`) and the resulting expected `workspace_region` value. Per the team's preference, these two cases would naturally belong together as a single `@pytest.mark.parametrize`-driven test. Similarly, `test_devbox_update_applies_when_outdated` only covers the `DEFAULT_REGION` path for `devbox_update`; a second parametrised case with non-default region metadata would confirm that path reads region from the workspace dict too.

Reviews (1): Last reviewed commit: "chore(devbox): drop redundant dict() wra..." | Re-trigger Greptile

@rnegron rnegron removed the request for review from a team May 28, 2026 18:49
@rnegron rnegron merged commit 9a5c65c into master May 28, 2026
170 checks passed
@rnegron rnegron deleted the worktree-devbox-outdated-gate branch May 28, 2026 19:54
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented May 28, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-05-28 20:28 UTC Run
prod-us ✅ Deployed 2026-05-28 20:44 UTC Run
prod-eu ✅ Deployed 2026-05-28 20:44 UTC Run

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

Labels

stamphog Request AI review from stamphog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant