Add PATCH/DELETE transport and coding-agent-config CRUD clients - #270
Merged
Conversation
tt-le
force-pushed
the
tien/managed-setup-wizard
branch
from
August 5, 2026 17:53
48d9066 to
b078138
Compare
tt-le
force-pushed
the
tien/managed-apply-clients
branch
2 times, most recently
from
August 5, 2026 19:58
b0ba733 to
7e2ec09
Compare
tt-le
force-pushed
the
tien/managed-setup-wizard
branch
from
August 5, 2026 20:49
7fff00b to
b8e191a
Compare
tt-le
force-pushed
the
tien/managed-apply-clients
branch
from
August 5, 2026 20:49
7e2ec09 to
88f43af
Compare
tt-le
force-pushed
the
tien/managed-setup-wizard
branch
from
August 5, 2026 21:03
b8e191a to
57cb08d
Compare
tt-le
force-pushed
the
tien/managed-apply-clients
branch
from
August 5, 2026 21:03
88f43af to
b8dff9e
Compare
tt-le
force-pushed
the
tien/managed-setup-wizard
branch
from
August 5, 2026 21:12
57cb08d to
c536a37
Compare
tt-le
force-pushed
the
tien/managed-apply-clients
branch
from
August 5, 2026 21:12
b8dff9e to
03a3f13
Compare
tt-le
force-pushed
the
tien/managed-setup-wizard
branch
from
August 5, 2026 21:21
c536a37 to
0d02f98
Compare
tt-le
force-pushed
the
tien/managed-apply-clients
branch
from
August 5, 2026 21:21
03a3f13 to
32205ad
Compare
AarushiShah-db
previously approved these changes
Aug 5, 2026
tt-le
force-pushed
the
tien/managed-setup-wizard
branch
from
August 5, 2026 22:13
0d02f98 to
7f8ac63
Compare
tt-le
force-pushed
the
tien/managed-apply-clients
branch
from
August 5, 2026 22:13
32205ad to
d386d18
Compare
tt-le
force-pushed
the
tien/managed-setup-wizard
branch
from
August 5, 2026 23:06
7f8ac63 to
30154f4
Compare
tt-le
force-pushed
the
tien/managed-apply-clients
branch
from
August 5, 2026 23:06
d386d18 to
64d8258
Compare
Base automatically changed from
tien/managed-setup-wizard
to
tien/managed-setup-serializer
August 6, 2026 19:13
tt-le
force-pushed
the
tien/managed-setup-serializer
branch
from
August 6, 2026 20:10
5d1cb00 to
868ccce
Compare
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
The publish step for the manifest `ucode setup` authors. Validates, shows what would change, confirms, then writes it to the workspace via the clients added in the parent commit. Updates in place rather than replacing. When the workspace already has a config, `apply` PATCHes it using its resource name (read back from the existing-config GET, which `normalize_managed_config` preserves). Delete-then-create was the original plan — v0's Create returns ALREADY_EXISTS — but it has a window where the workspace has *no* managed config, and 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 update mask inside a single entity-store update, so a failed PATCH leaves the current config intact. It is still a whole-manifest write: every path ucode owns is sent, so a field the admin dropped on a re-run is cleared rather than left behind. Refuses to publish when it cannot tell whether a config already exists. A failed existence check used to be the one case where "just try the create" would either duplicate or silently overwrite an admin's work, so an unreadable check is a hard error naming the reason rather than a warning. `_explain_publish_failure` maps the failures an admin will actually hit. FEATURE_DISABLED is the likely first experience — the CRUD flag is off by default — so it names `codingAgentConfigCrudEnabled` instead of printing an HTTP 400. INVALID_PARAMETER_VALUE is passed through verbatim: the server names the offending field, which beats any paraphrase. `--yes` skips the confirmation for CI; `--dry-run` validates and previews without writing. The admin gate and validation both run before anything is sent, so an invalid manifest or a non-admin costs no round trip. README documents the publish step and drops the "no partial update yet" caveat, which the PATCH path makes untrue. Verified against eng-ml-inference.staging: `apply --dry-run` authenticated, verified admin, rendered the summary, detected the existing config, and chose the update path without writing. Tests: 19 cases. Mutation-verified three ways — always-create instead of PATCH, publishing despite an unreadable existence check, and publishing an invalid manifest each fail a specific test. Also covers that `typer.Exit(0)` isn't caught by the command's own RuntimeError handler, the same trap `setup` hit. Co-authored-by: Isaac
tt-le
force-pushed
the
tien/managed-apply-clients
branch
from
August 7, 2026 13:23
f8fe6f1 to
ae99ee5
Compare
`ucode apply` rejected a model `ucode setup` had just offered:
claude: model 'system.ai.claude-opus-4-8' is not available on this workspace.
`state["claude_models"]` holds only the newest id per family, because the launch
path pins one model per family alias. The wizard deliberately offers the older
versions too — pinning `default_opus_model` to a known-good `claude-opus-4-8` is a
normal thing for an admin to want — and stashes the full listing on
`state["all_claude_models"]` so validation recognizes them.
That stash is never persisted. `setup` saves the manifest, not the state, so a
separate `apply` process starts from a fresh `load_state()` without it and
validates against the narrow per-family inventory. The failure lands at the very
end of the flow, naming a model the wizard itself had listed a moment earlier.
`apply` now re-fetches the listing instead of trusting what `setup` left behind,
which also covers a hand-edited or `--from-file` manifest authored on another
machine. Best-effort: a failed listing leaves validation on the narrower inventory
rather than blocking a publish on a transient API error. `ensure_databricks_auth`
moves above validation since the listing needs a token; nothing is written until
well after.
Aarushi predicted this failure, and its exact message, reviewing the invariant on
`_claude_candidates` in #268. The fix there documented and tested the invariant
within one process — both paths satisfied it — and so missed that `setup` and
`apply` are two processes.
Tests: +2, covering the older-version publish and that a failed fetch still
publishes. Mutation-verified: validating against bare `state` reproduces the
original error.
Co-authored-by: Isaac
The PATCH was rejected by the server it was written for:
HTTP 400 INVALID_PARAMETER_VALUE: Field 'update_mask' is required and must
contain at least one subfield with a non-default value!
Two mistakes, both visible in the RPC's HTTP binding (universe
ai-gateway-api/api/proto/service.proto:319):
patch: "/ai-gateway/v2/{coding_agent_config.name=coding-agent-configs/*}"
body: "coding_agent_config"
`body: "coding_agent_config"` means the config *is* the entire request body, so
there is nowhere in it for a sibling `update_mask` — the one we nested was parsed
as an unknown config field, leaving the mask genuinely absent. It belongs in the
query string.
And `update_mask` is a `google.protobuf.FieldMask`, whose JSON and query form is a
single comma-separated string, not the `{"paths": [...]}` object we sent.
`name` stays in the body: the path template reads it from the config.
The mask contents were right, and the 15 tests covering them all passed — they
asserted on `payload["update_mask"]["paths"]`, which is exactly the shape the
server rejects. A unit test that mirrors the client's own assumption cannot catch
a wire-format error; this was only ever going to surface against a real workspace.
Tests: the update test now asserts the mask arrives in the query string, as one
comma-separated FieldMask, and is absent from the body. Mutation-verified:
restoring the nested-object form fails it.
Co-authored-by: Isaac
AarushiShah-db
previously approved these changes
Aug 7, 2026
tt-le
enabled auto-merge (squash)
August 7, 2026 15:13
One conflict, both sides additive: #261 added `resolve_current_budget_spend` and its `TestResolveCurrentBudgetSpend` suite next to this branch's coding-agent-config CRUD client tests, and each side added an import (`Decimal` vs `parse_qs`). Kept both classes and both imports.
AarushiShah-db
approved these changes
Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
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.pyhad_http_get_json/_http_post_jsonbut 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", soallow_empty_bodytreats it as success.Clients for the three admin RPCs, all workspace-admin gated server-side:
create_coding_agent_configALREADY_EXISTSwhen one existsupdate_coding_agent_configdelete_coding_agent_configWhy PATCH rather than delete-then-create. The original plan was delete-then-create, since Create returns
ALREADY_EXISTS. ButUpdateCodingAgentConfigexists and every field ucode authors is in the handler'sMUTABLE_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 singleentityStore.update, so a failed PATCH leaves the current config intact.MANAGED_CONFIG_UPDATE_MASK_PATHSis 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 forbudget_policy.budget_id, and rejected on write) anddefault_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-fileeven though the wizard can't produce them.—
budget_policy.budget_idmust 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'stiers[1]and the API'stiers[0]described the same tier — an admin reconciling the two messages would look at the wrong one.— The deprecated top-level
budget_idis never emitted. The serializer already only writesbudget_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 realbudget_configuration_idwould be, so those are now UUIDs — 20 occurrences.list_workspace_budgetsonly 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 theupdate_mask, dropping one mask path, dropping the UUID check, reverting to 1-based indices, and emitting the top-levelbudget_ideach fail a specific test.This pull request and its description were written by Isaac.