Skip to content

Add PATCH/DELETE transport and coding-agent-config CRUD clients - #270

Open
tt-le wants to merge 3 commits into
tien/managed-setup-wizardfrom
tien/managed-apply-clients
Open

Add PATCH/DELETE transport and coding-agent-config CRUD clients#270
tt-le wants to merge 3 commits into
tien/managed-setup-wizardfrom
tien/managed-apply-clients

Conversation

@tt-le

@tt-le tt-le commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Changes

Stacked on #268 (which is stacked on #267) — review those first. This PR's diff includes them until they merge.

The write-side API plumbing for ucode apply (#PR3b). No CLI wiring and no interactive flow: transport helpers plus three clients, scoped the same way the read client landed in #263.

Transport. databricks.py had _http_get_json/_http_post_json but no PATCH or DELETE. Rather than a third and fourth near-copy of the same 40 lines of error handling, the body-sending path is factored into _http_send_json(method, ...) and the verbs become thin wrappers. _http_post_json's behavior is unchanged.

DELETE needed one real difference: its success response is google.protobuf.Empty, which arrives as {} or an empty body depending on the gateway. An empty body would otherwise be reported as "response was not valid JSON", so allow_empty_body treats it as success.

Clients for the three admin RPCs, all workspace-admin gated server-side:

Client Verb Note
create_coding_agent_config POST v0 allows one config per workspace, so returns ALREADY_EXISTS when one exists
update_coding_agent_config PATCH Preferred over delete-then-create — see below
delete_coding_agent_config DELETE Returns only a reason; there's no payload worth handing back

Why PATCH rather than delete-then-create. The original plan was delete-then-create, since Create returns ALREADY_EXISTS. But UpdateCodingAgentConfig exists and every field ucode authors is in the handler's MUTABLE_UPDATE_MASK_PATHS. Delete-then-create has a window where the workspace has no managed config: if the create failed there, every developer would silently fall back to their own settings until someone re-ran the command. The server applies the mask inside a single entityStore.update, so a failed PATCH leaves the current config intact.

MANAGED_CONFIG_UPDATE_MASK_PATHS is every field ucode's manifest can set. The server requires a non-empty mask and rejects paths outside its mutable set; this is that set minus what ucode doesn't author — budget_id (deprecated for budget_policy.budget_id, and rejected on write) and default_options/tiers (the legacy model-only shape). Sending every path ucode owns, not just the populated ones, is what lets a re-run clear a field the admin removed: the server merges per path, so an omitted path leaves the old value in place.

Validation gaps found against the server

Three things surfaced from reading the validation this manifest is written for (universe #2365441). All are reachable through --from-file even though the wizard can't produce them.

budget_policy.budget_id must parse as a UUID. The handler requires it; ucode only checked non-empty, so a hand-written "budget_id": "eng-budget" passed local validation and failed at the API. Local pre-flight exists precisely to spend the round trip on real problems.

Tier positions are now 0-based. The server indexes with zipWithIndex, so ucode's tiers[1] and the API's tiers[0] described the same tier — an admin reconciling the two messages would look at the wrong one.

The deprecated top-level budget_id is never emitted. The serializer already only writes budget_policy.budget_id; this pins behavior that was correct by construction rather than by intent.

Test fixtures used short placeholders ("b", "budget-1") where a real budget_configuration_id would be, so those are now UUIDs — 20 occurrences. list_workspace_budgets only ever returns real ones, so the fixtures described input the wizard can't produce.

Testing

uv run pytest — 1354 passed, 6 skipped. 21 new cases.

The mask is checked against serialize_managed_config's actual output rather than a restated list, so adding a manifest field fails the test instead of shipping a mask that can't clear it.

Mutation-verified six ways — dropping allow_empty_body, dropping the update_mask, dropping one mask path, dropping the UUID check, reverting to 1-based indices, and emitting the top-level budget_id each fail a specific test.

This pull request and its description were written by Isaac.

@tt-le
tt-le force-pushed the tien/managed-setup-wizard branch from 48d9066 to b078138 Compare August 5, 2026 17:53
@tt-le
tt-le force-pushed the tien/managed-apply-clients branch 2 times, most recently from b0ba733 to 7e2ec09 Compare August 5, 2026 19:58
@tt-le
tt-le force-pushed the tien/managed-setup-wizard branch from 7fff00b to b8e191a Compare August 5, 2026 20:49
@tt-le
tt-le force-pushed the tien/managed-apply-clients branch from 7e2ec09 to 88f43af Compare August 5, 2026 20:49
@tt-le
tt-le force-pushed the tien/managed-setup-wizard branch from b8e191a to 57cb08d Compare August 5, 2026 21:03
@tt-le
tt-le force-pushed the tien/managed-apply-clients branch from 88f43af to b8dff9e Compare August 5, 2026 21:03
@tt-le
tt-le force-pushed the tien/managed-setup-wizard branch from 57cb08d to c536a37 Compare August 5, 2026 21:12
@tt-le
tt-le force-pushed the tien/managed-apply-clients branch from b8dff9e to 03a3f13 Compare August 5, 2026 21:12
@tt-le
tt-le force-pushed the tien/managed-setup-wizard branch from c536a37 to 0d02f98 Compare August 5, 2026 21:21
@tt-le
tt-le force-pushed the tien/managed-apply-clients branch from 03a3f13 to 32205ad Compare August 5, 2026 21:21
@tt-le
tt-le force-pushed the tien/managed-setup-wizard branch from 0d02f98 to 7f8ac63 Compare August 5, 2026 22:13
@tt-le
tt-le force-pushed the tien/managed-apply-clients branch from 32205ad to d386d18 Compare August 5, 2026 22:13
tt-le added 3 commits August 5, 2026 23:06
The write-side API plumbing for `ucode apply` (next change). No CLI wiring and no
interactive flow: transport helpers plus three clients, mirroring how the read
client landed.

`databricks.py` had `_http_get_json`/`_http_post_json` but no PATCH or DELETE.
Rather than a third and fourth near-copy of the same 40 lines of error handling,
the body-sending path is factored into `_http_send_json(method, ...)` and the
three verbs become thin wrappers. `_http_post_json`'s behavior is unchanged.

DELETE needed one real difference: its success response is
`google.protobuf.Empty`, which arrives as `{}` or an empty body depending on the
gateway. An empty body would otherwise be reported as "response was not valid
JSON", so `allow_empty_body` treats it as success. `delete_coding_agent_config`
returns only a reason — there is no payload worth handing back.

Clients for the three admin RPCs, all workspace-admin gated server-side:
- `create_coding_agent_config` — POST to the collection. v0 allows one config per
  workspace, so this returns ALREADY_EXISTS when one exists.
- `update_coding_agent_config` — PATCH the resource. Preferred over
  delete-then-create, which has a window where the workspace has *no* managed
  config: if the create failed, every developer would lose their config until
  someone re-ran the command. The server applies the mask inside a single
  entity-store update, so a failed write leaves the old config intact.
- `delete_coding_agent_config` — DELETE by resource name.

`MANAGED_CONFIG_UPDATE_MASK_PATHS` is every field ucode's manifest can set. The
server requires a non-empty mask and rejects paths outside its mutable set; this
is that set minus what ucode doesn't author — `budget_id` (deprecated for
`budget_policy.budget_id`, and rejected on write) and `default_options`/`tiers`
(the legacy model-only shape). Sending every path ucode owns, not just the
populated ones, is what lets a re-run *clear* a field the admin removed: the
server merges per path, so an omitted path leaves the old value in place.

`_coding_agent_config_url` joins on the API root rather than the collection URL,
since the resource name already carries the `coding-agent-configs/` segment and
would otherwise be duplicated.

Tests: 15 cases. The mask is checked against `serialize_managed_config`'s actual
output rather than a restated list, so adding a manifest field fails the test
instead of shipping a mask that cannot clear it. Mutation-verified three ways:
dropping `allow_empty_body`, dropping the `update_mask`, and dropping one mask
path each fail a specific test.

Co-authored-by: Isaac
Three gaps found by reading the server-side validation this manifest is written
for (universe #2365441), all reachable through `--from-file` even though the
wizard can't produce them.

`budget_policy.budget_id` must parse as a UUID. The handler requires it, and
until now ucode only checked non-empty — so a hand-written manifest carrying
`"budget_id": "eng-budget"` passed local validation and failed at the API with an
INVALID_PARAMETER_VALUE. Local pre-flight exists precisely to spend the round
trip on real problems. The message names `budget_configuration_id` so an admin
knows where to get a valid one.

Tier positions are now reported 0-based. The server indexes with `zipWithIndex`,
so ucode's `tiers[1]` and the API's `tiers[0]` described the same tier — an admin
reconciling the two messages would be looking at the wrong one.

Added a test that the deprecated top-level `CodingAgentConfig.budget_id` (field 3)
is never emitted, even when a hand-written manifest sets it. The serializer
already only writes `budget_policy.budget_id`; the handler rejects the top-level
field, so this pins behavior that is currently correct by construction rather
than by intent.

Test fixtures used short placeholders (`"b"`, `"budget-1"`) where a real
`budget_configuration_id` would be, so those are now UUIDs — 20 occurrences
across the two files. `list_workspace_budgets` only ever returns real ones, so
the fixtures were describing input the wizard can't produce.

Tests: +6. Mutation-verified: dropping the UUID check fails four cases, reverting
to 1-based indices fails `test_tier_positions_are_reported_zero_based`, and
emitting the top-level `budget_id` fails
`test_a_manifest_carrying_a_top_level_budget_id_still_omits_it`.

Co-authored-by: Isaac
Review feedback on #267: `_AGENT_MODEL_CONFIG_VARIANT` mapped each agent to its
`AgentModelConfig` oneof key, but the proto's field names are ucode's tool names
verbatim — claude, codex, opencode, pi, gemini, copilot — so every entry mapped a
name to itself. One use site, so the tool now serves as the key directly.

The dict's only other effect was a KeyError on an unknown agent, which was already
unreachable: `serialize_managed_config` filters to `tool in AGENT_TOOL_TO_ENUM`
before calling this, and the `AGENT_TOOL_TO_ENUM[tool]` lookup two lines down would
raise first anyway.

No new test. The variant keys are already covered — hard-coding the wrong one fails
`test_codex_model_config_has_no_model_list` and
`test_flat_list_agents_use_repeated_models`, and the round-trip through
`normalize_managed_config` asserts the alignment for every agent.

The other half of that review comment — validating a model against its agent's
dialect, so a manifest can't pin a GPT id for Claude Code — is deliberately not
here. It turned out to need a decision rather than a patch: the agent -> families
mapping already exists twice (`agents._TOOL_DISCOVERY_SOURCES` and
`managed_setup._AGENT_MODEL_FAMILIES`) and the two disagree for codex, copilot,
opencode, and pi. Picking one requires checking each agent's own writer, and the
likely outcome is that this module's opencode entry is wrong — it lists `codex`
while `build_opencode_base_urls` serves no OpenAI route — which would be a picker
bug in the wizard, not a refactor. Landing that separately.

Co-authored-by: Isaac
@tt-le
tt-le force-pushed the tien/managed-setup-wizard branch from 7f8ac63 to 30154f4 Compare August 5, 2026 23:06
@tt-le
tt-le force-pushed the tien/managed-apply-clients branch from d386d18 to 64d8258 Compare August 5, 2026 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants