feat: add on_success workspace build request handling - #25758
Conversation
Docs preview📖 View docs preview for |
geokat
changed the title
92bf471 to
17867b6
Compare
58700d9 to
3edff4e
Compare
dca2e69 to
2e97f53
Compare
Add the on_success request shape for workspace builds and validate the supported server-side restart flow. When a stop build includes on_success, persist the requested child start build intent in workspace_build_orchestrations for later processing. Add SDK/API generated docs and tests for validation, persistence, and child request fields. Ref: https://linear.app/codercom/issue/PLAT-143/add-workspace-restart-functionality-to-api Ref: #5800
2e97f53 to
fa1fd82
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 11 findings (5 P2, 4 P3, 1 Nit, 1 Note), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel. 5 P2, 4 P3, 1 Nit, 1 Note, 1 P4 dropped. CRF-1 subsumed by CRF-8. CRF-2 dropped (dead code for stacked PRs). CRF-10 noted in body (code from prior PR, DB constraint prevents exploit). Reviewed against 6cd5c5e..fa1fd82. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean stack decomposition: PR 1 adds storage, PR 2 (this) adds the API surface and persistence, PR 3 adds processing. The transaction atomicity is correct; the parent build and orchestration insert share a single InTx, so both succeed or both roll back. The dbauthz layer front-loads authorization at insert time so the system-context orchestrator doesn't need to re-check permissions later. Test density is solid at 57.8%.
Severity counts: 5 P2, 4 P3, 1 Nit, 1 Note.
The two highest-impact findings are input validation gaps on the child request: (1) preset-without-version passes validation and hits a raw DB CHECK constraint as a 500 with a leaked constraint name, and (2) a cross-template version ID can be stored without validating the version belongs to the workspace's template. The convergence on these was strong: five reviewers independently flagged the preset gap and two flagged the cross-template gap.
"The user loses both the stop and the restart intent, with an opaque error." (Hisoka, on preset-without-version)
coderd/database/dbauthz/dbauthz.go:6438
Note [CRF-10] Defense-in-depth: this guard checks arg.ChildTemplateVersionID.Valid but not arg.ChildTemplateVersionPresetID.Valid. A preset FK-resolves to exactly one template version, so specifying only a preset is semantically equivalent to a version pin. The DB constraint (workspace_build_orchestrations_child_preset_version_check) currently prevents preset-without-version, so this is not exploitable today. If that constraint is ever relaxed to support auto-resolution, the auth check would need to also gate on arg.ChildTemplateVersionPresetID.Valid.
(Meruem)
🤖
🤖 This review was automatically generated with Coder Agents.
Emyrk
left a comment
Emyrk
left a comment
There was a problem hiding this comment.
Reviewed the on_success request handling. Behaviour and validation look right, transaction rollback on the dbauthz rejection is correct, and the table-driven validation test maps one-to-one to validateCreateWorkspaceBuildOnSuccess. Three small nits inline, all non-blocking.
This review was drafted by Coder Agents on behalf of @Emyrk.
6b6fcd3
into
george/plat-143/1-workspace-build-orchestration-storage
2 participants
This PR is part of a stack that adds durable server-side workspace restart support
to the API.
Add an
on_successfield to workspace build requests so callers can attachdurable follow-up build intent to a parent build. In this stack, the supported
flow is a stop build with an
on_successstart build, which lets the serverpersist restart intent before the parent stop build finishes.
This adds SDK/API request types, validation, generated API documentation, and
persistence of the child build request into
workspace_build_orchestrations. Italso adds coverage for validation, persistence, and stored child request fields.
Ref: https://linear.app/codercom/issue/PLAT-143/add-workspace-restart-functionality-to-api
Ref: #5800
This PR is part of a stack that merges into
main:Created with stakk