feat: plumb time_til_autostop_notify template field#26439
Conversation
Docs preview📖 View docs preview for |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 5 | Last posted: Round 5, 13 findings (1 P2, 4 P3, 2 P4, 4 Nit, 2 Note), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel (15 reviewers). 1 P2, 2 P3, 3 Nit, 2 Note posted. 1 Nit dropped. Reviewed against 3e68dd3..a77fdbc. Round 2BLOCKED. 3 addressed (CRF-3, CRF-7, CRF-10). 6 silent. Round 3Orchestrator override (code moved to stack). Panel (9 reviewers). 1 P3, 2 P4, 1 Nit new. All R1 findings resolved. Reviewed against d21121f..0aa4537. Round 4Panel (4 reviewers). CRF-12, CRF-13, CRF-14 verified fixed. CRF-11 re-raised. No new findings. Reviewed against 1c5a2f2..e28f8d6. Round 5CRF-11 verified fixed. Enterprise test About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
Documentation CheckUpdates Needed
Already Addressed in This PR
No Longer in This PR
Automated review via Coder Agents |
a77fdbc to
2c369cc
Compare
There was a problem hiding this comment.
Clean plumbing PR. The field threads correctly through all layers: DB migration, SQLC, AGPL and enterprise schedule stores (Get + Set + no-op guard), API create/patch, SDK, CLI, audit table, notification template, and frontend types. Conversion chain (nanoseconds in DB, milliseconds in API, time.Duration in Go) is consistent at every hop. Test density is strong at 47%, and the TimeTilAutostopNotifyPreservedWhenOmitted test is a good addition that catches the nil-vs-zero class of bug.
Severity summary: 1 P2, 2 P3, 3 Nit, 2 Note.
The P2 is about the notification body text living in a migration and attributing the stop to "its autostop schedule" when the deadline may come from other sources. The P3s are a missing validation-rejection test, a design note about upper-bound behavior for the future dispatcher, and the empty PR description.
"I tried to build a case against this PR and couldn't. The problem is correctly understood. The solution is proportional." (Pariston)
🤖 This review was automatically generated with Coder Agents.
2c369cc to
eb60af0
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 is blocked. The author addressed 3 findings (CRF-3, CRF-7, CRF-10), but 6 remain without substantive response. Further review is on hold until the author responds or pushes fixes for the open findings.
Findings needing response (respond, fix, or explain why no change is needed):
CRF-5 (P2): Notification body says "due to its autostop schedule" but the deadline may come from default_ttl or admin-enforced requirements. Author replied "decline" with no reasoning. The text lives in a migration, so fixing later costs another migration. What is the reasoning for keeping the current wording?
CRF-4 (P3): No upper bound on time_til_autostop_notify. The original finding explicitly said no fix is needed today, but the dispatcher PR should handle notify_time < now as a no-op. Acknowledge or note a ticket.
CRF-9 (Nit): notified_autostop_deadline has no SQL COMMENT. Author replied "decline" with no reasoning. The column name alone does not communicate that it stores the last-notified deadline value for idempotence.
CRF-1 (Note), CRF-2 (Note): These are observations about plumbing scope (no UPDATE query, notification never enqueued). Author replied "decline." If the intent is "acknowledged, no action needed because the dispatcher PR will add these," please say so.
CRF-6 (P3): PR description is empty boilerplate. Not blocking, but context on design choices would help future readers.
The panel has not yet reviewed the new diff (migration renumbered, CLI flag renamed, validation tests added, validation error message updated). The panel will review after this block is resolved.
🤖 This review was automatically generated with Coder Agents.
5885637 to
d21121f
Compare
eb60af0 to
0aa4537
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All R1 findings resolved. The PR was restructured into a 5-branch stack; migration, notification, and audit code moved to earlier branches. CRF-3 (validation tests), CRF-7 (CLI flag rename), CRF-10 (error message), CRF-6 (PR description) all fixed. The new PR description is excellent.
R3 panel (9 reviewers) found 1 P3, 2 P4, 1 Nit on the narrowed diff. The P3 is an enterprise test coverage gap. The P4s are defensive test additions. 5 of 9 reviewers reported no findings.
Commit scope coderd/schedule does not contain all changed files (changes span cli/, codersdk/, site/, enterprise/coderd/schedule/). Either omit the scope or use a broader one. (Mafu-san)
"Conversion chain: API (milliseconds) to time.Duration to int64 nanoseconds to DB bigint. Read path reverses cleanly. No-op checks in both AGPL and enterprise Set correctly include the new field. The chain is the same as round 1, but now without the migration and notification code in the way, it is easier to verify." (Melody)
🤖 This review was automatically generated with Coder Agents.
|
|
||
| if int64(opts.DefaultTTL) == tpl.DefaultTTL && | ||
| int64(opts.ActivityBump) == tpl.ActivityBump && | ||
| int64(opts.TimeTilAutostopNotify) == tpl.TimeTilAutostopNotify && |
There was a problem hiding this comment.
P3 [CRF-11] Enterprise schedule store's TimeTilAutostopNotify plumbing has zero test coverage.
Three code paths changed in this file: Get (line 91), the no-op short-circuit (line 120), and Set params (line 158). No enterprise test exercises any of them. The no-op comparison is the risky one: if this line had a copy-paste error (e.g., compared against tpl.ActivityBump instead of tpl.TimeTilAutostopNotify), updates to TimeTilAutostopNotify on enterprise deployments would be silently swallowed. The AGPL store has the same pattern and is exercised through integration tests, but the enterprise store has its own Set implementation with different conditional logic.
(Hisoka)
🤖
There was a problem hiding this comment.
P3 [CRF-11] Still open, no author response across two rounds.
enterprise/coderd/schedule/template_test.go has zero references to TimeTilAutostopNotify. Three production lines in the enterprise store are untested: Get return (line 91), no-change guard (line 120), Set params (line 158). The AGPL integration tests exercise the AGPL store, not this one.
Mitigating: the enterprise code is character-for-character identical to the AGPL store's handling, and ActivityBump has the same pre-existing gap. But a copy-paste error in the no-change comparison (e.g., comparing against tpl.ActivityBump instead of tpl.TimeTilAutostopNotify) would silently swallow updates on enterprise deployments.
What response is needed: add a test row to TestTemplateTTL or another enterprise schedule test that verifies TimeTilAutostopNotify round-trips through the enterprise store, or explain why enterprise test coverage is intentionally deferred.
(Bisky, Mafuuu, Hisoka)
🤖
There was a problem hiding this comment.
Verified fixed in 2537503fb48c. TestTemplateScheduleTimeTilAutostopNotify covers:
- Set/Get round-trip (1h persists through enterprise store)
- No-op guard: changes only
TimeTilAutostopNotifyfrom 1h to 2h with all other options identical, confirming the comparison at line 120 references the correct field
The test comment explicitly documents the CRF-11 scenario. Well done.
🤖
| assert.Equal(t, template.AllowUserAutostop, updated.AllowUserAutostop) | ||
| }) | ||
|
|
||
| t.Run("TimeTilAutostopNotifyPreservedWhenOmitted", func(t *testing.T) { |
There was a problem hiding this comment.
P4 [CRF-12] No integration test for disabling autostop notify after enabling it (patch to ptr.Ref(int64(0)) after creating with a non-zero value).
This is a real user flow: "I enabled reminders, now I want to disable them." The resolver test (TimeTilAutostopNotifyMillisZeroExplicit) covers the logic, but no API-level test round-trips the disable path through the handler. If the handler had a bug in zero-value plumbing (e.g., treating ptr.Ref(int64(0)) the same as nil), this flow would break silently.
(Bisky)
🤖
| require.Contains(t, err.Error(), "default_ttl_ms: Must be a positive integer") | ||
| }) | ||
|
|
||
| t.Run("TimeTilAutostopNotifyTooLow", func(t *testing.T) { |
There was a problem hiding this comment.
P4 [CRF-13] No boundary-value test at exactly 1 minute (60,000ms). Validation accepts 0 and >= 1 minute, rejects 0 < x < 1 minute. Tests exercise 30s (rejected) and 5m (accepted) but never the exact boundary. If someone changed < time.Minute to <= time.Minute, no existing test would catch it.
(Bisky)
🤖
| VersionID: version.ID, | ||
| TimeTilAutostopNotifyMillis: ptr.Ref(int64(30_000)), | ||
| }) | ||
| var apiErr *codersdk.Error |
There was a problem hiding this comment.
Nit [CRF-14] TimeTilAutostopNotifyTooLow and TimeTilAutostopNotifyNegative assert apiErr.Validations[0].Field but not apiErr.Validations[0].Detail. The two tests send different invalid values (30s vs -1) to exercise distinct validation branches that produce different error messages, but both only check the field name. If someone accidentally swapped the messages, both tests would still pass.
(Chopper)
🤖
0aa4537 to
987590b
Compare
d21121f to
1c5a2f2
Compare
987590b to
e28f8d6
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Thanks for adding the boundary test, the disable-after-enable test, and the Detail assertions. CRF-12, CRF-13, CRF-14 all verified fixed by the panel.
One finding remains from R3: 1 P3 across 1 inline comment. CRF-11 (enterprise test coverage) has had no response across two rounds.
🤖 This review was automatically generated with Coder Agents.
e28f8d6 to
f98783e
Compare
Add the per-template time_til_autostop_notify duration (ns; 0=disabled) through the schedule store (AGPL + enterprise), codersdk Template/CreateTemplateRequest/UpdateTemplateMeta, the template create/patch handlers, and the CLI --autostop-reminder flag. Validation rejects negatives and sub-minute values. Not entitlement-gated, mirrors activity_bump.
1c5a2f2 to
f9b398f
Compare
f98783e to
2537503
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
CRF-11 verified fixed. TestTemplateScheduleTimeTilAutostopNotify exercises the enterprise store's Set/Get round-trip and, critically, the no-op short-circuit guard: it changes only TimeTilAutostopNotify while keeping all other options identical, confirming the comparison uses the correct field. This is the exact scenario the finding described.
All 14 findings across 5 rounds are resolved. Test density is now 74%. The plumbing is clean and thoroughly tested across both AGPL and enterprise stores.
🤖 This review was automatically generated with Coder Agents.
Summary
Branch 3 of the workspace autostop reminder stack. Plumbs the per-template
time_til_autostop_notifyscheduling field end to end: schedule store (AGPL + enterprise), codersdk types, template create/patch handlers, and the CLI. This branch wires the configuration; it does not yet send anything (the lifecycle-executor dispatcher lands in the next branch).What this enables
Template admins can set how long before a workspace's autostop deadline its owner is reminded (e.g. 1h).
0(the default) disables the reminder for that template's workspaces.Design choices
default_ttl/ user TTL on AGPL), and the AGPL lifecycle executor already enqueues other workspace notifications (TemplateWorkspaceDormant,TemplateWorkspaceAutoUpdated). A reminder about an AGPL event should also be AGPL, so this field mirrorsactivity_bump: read/written by both the AGPL and enterprise schedule stores, with noadvanced_template_schedulinggate.templates.time_til_autostop_notifycolumn isbigintnanoseconds to match every other duration column (default_ttl,activity_bump,time_til_dormant), which map directly to Gotime.Duration. The API exposestime_til_autostop_notify_msin milliseconds to match the existing*_msconvention used byactivity_bump_ms,default_ttl_ms, etc., so the frontend and SDK stay consistent.0or>= 1 minute. The future dispatcher runs on the lifecycle executor's one-minute tick, so a sub-minute reminder window could be missed entirely. Negative values are rejected separately with "Must be a positive integer." (matching the sibling fields' message).UpdateTemplateMetauses a pointer (*int64). Omittingtime_til_autostop_notify_msfrom a PATCH preserves the existing value (viaptr.NilToDefault), consistent with the other optional meta fields.Context for the dispatcher (next branch)
The schema branch (already merged-below) also added
workspace_builds.notified_autostop_deadline, a sentinel storing the deadline value a reminder was last sent for. The dispatcher will send a reminder when the build deadline is withintime_til_autostop_notifyof now andnotified_autostop_deadline != deadline, then stamp the sentinel to the current deadline. This gives once-per-deadline idempotence that re-arms automatically when the deadline moves (activity bump, manual extend, schedule change) and is safe across HA replicas. The dispatcher will also treat a notify-time already in the past (when the configured lead exceeds a workspace's remaining lifetime) as at-most-one reminder, not a per-tick flood.Changes
coderd/database/queries/templates.sql:time_til_autostop_notifyparam onUpdateTemplateScheduleByID.coderd/schedule/template.go+enterprise/coderd/schedule/template.go:TemplateScheduleOptions.TimeTilAutostopNotifyin both stores'Get/Set.codersdk/templates.go,codersdk/organizations.go:Template/UpdateTemplateMeta/CreateTemplateRequestfields (time_til_autostop_notify_ms).coderd/templates.go,coderd/templates_meta_update.go: create/patch validation,convertTemplatemapping,Setwiring.cli/templateedit.go:--autostop-reminderflag (preserve-current-value pattern).Stack
01-db— schema + audit (columns + notification template seed)02-template— notification template Go wiring + goldens + docs03-schedule-sdk(this PR) —time_til_autostop_notifyconfig plumbing04-executor— lifecycle-executor dispatcher + eligibility/marker queries (turns the feature on)05-ui— template schedule-settings form field