The unit decision is the whole feature and this PR gets it right and writes down why: flooring milliseconds to whole hours at the single point where the two units meet is what keeps Feature.Compare, Feature.Capable, the coder features list columns and the warning thresholds from silently comparing hours against milliseconds. The at-most-one-warning switch is pinned in both directions (AtAllocation asserts the soft message is absent, not just that the allocation one is present). AgentRuntimeHours/NoClaimNoFeature fails from inside the AgentRuntimeMsFn closure, which proves the query is never reached rather than only that nothing warned. And TestGetTotalUsageHBAgentRuntimeV1 gives the day-truncation and timezone behavior query-level coverage that GetTotalUsageDCManagedAgentsV1 never had. The PR description also names two decisions a reviewer could not have seen from the code, including drift in a downstream ticket. That is the standard.

Twenty-three reviewers. 6 P2, 12 P3, 1 P4, 6 nits, 5 notes. No P0 or P1: the code builds, the tests pass here (including the Postgres-backed one), the SQL matches the rollup trigger key for key, and the authz wrapper matches its sibling.

The P2s cluster, and the cluster is worse than its parts. This PR turns a sum into a customer-facing number and attaches "Additional usage may be billable" to it. That number can be permanently and unboundedly low, because the generator forfeits buckets older than a 7-day window and nothing backfills (CRF-8). It can also be too high at period boundaries, because day-truncated inclusive bounds charge the renewal day against both terms (CRF-9). The warning can fire when nothing has happened, because agent_runtime_hours_limit_soft: 0 is a valid claim and 0 >= 0 (CRF-10). When it does fire, both messages collide with the AI Governance near-limit prefix and render in the softest banner variant in the system, indistinguishable from each other (CRF-11). The number itself carries no unit at the public field, and the ticket for the UI that will render it already reads it as milliseconds (CRF-7). And it is all served to unauthenticated callers by an endpoint whose swagger annotation claims otherwise (CRF-12). Individually each is arguable. Together they describe a meter that can be wrong in both directions, can alarm about nothing, renders quietly when it is right, and publishes the result to the internet.

Three of these need a human decision rather than a code change, and none of them should be accepted as permanent by an agent: whether the 7-day forfeit window is disclosed or the generator backfills to nbf (CRF-8, a billing-dedup question, not a code one); whether entitlements are deliberately public, in which case the RBAC check in writeEntitlementWarningsHeader on the same strings is only buying the illusion of a gate (CRF-12); and who owns the "may be billable" copy, which no docs/ page defines (CRF-23, CRF-31). File tickets or state the acceptance explicitly.

On the first-pass findings: Netero's CRF-1 claimed the runtime warnings render prominently. That is wrong, and CRF-11 is why. LicenseAIGovernance90PercentWarningText.split("%d%%")[0] is "You have used ", which both new texts start with. I evaluated it rather than reasoning about it. CRF-1 is dropped and CRF-4 is folded into CRF-11. Netero's other findings stand.

Two things I want to protect from the fix pass. Keep the query name: GetTotalUsageHBAgentRuntimeV1 matches GetTotalUsageDCManagedAgentsV1/dc_managed_agents_v1 and the event type is what someone greps when the number looks wrong; the issue's literal name would break the pattern. Keep the raw hourMs literal in license_internal_test.go and the raw "hb_agent_runtime_v1" literals in querier_test.go: deriving them from the expressions under test would hide exactly the bug they exist to catch.

Luffy, on the negative case: "That's the check I look for and it's usually missing."

site/src/modules/dashboard/entitlements.ts:19

Note [CRF-34] Populating Actual makes agent_runtime_hours fail getFeatureVisibility's limitCompliant check once usage passes the allocation. (Razor)

limitCompliant = limit && actual ? limit >= actual : true, so with limit=100, actual=150 the feature's visibility flips to false. Nothing keys off agent_runtime_hours visibility today (the only site/src occurrences are in typesGenerated.ts), so this is inert in this PR.

It matters for F1: a UI wired through useFeatureVisibility would hide the runtime-hours panel at exactly the moment the customer needs to see it. Razor notes the zero-allocation case is safe by accident, since limit=0 is falsy in JS so limitCompliant stays true. Worth knowing before the UI lands, not worth changing here.

🤖

🤖 This review was automatically generated with Coder Agents.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cb1b9a3a1f

Review is blocked this round, on one unaddressed item out of 32. I want to be clear up front that this is a process gate and not a judgment on the work: 26 of the 32 round-1 findings were addressed in e42b139f8b and cb1b9a3a1f, including all six P2s, and several of the fixes are better than what the panel asked for. The query rewrite in particular went further than the finding did: reading usage_events with exact start-inclusive, end-exclusive bounds instead of the day-truncated rollup, plus a doc recording the retention dependency the change creates and a generated-SQL test pinning the event-type literal against usagetypes.UsageEventTypeHBAgentRuntimeV1. That is the right shape.

Unaddressed, and the only thing standing between this and a panel round:

CRF-34 (Note, site/src/modules/dashboard/entitlements.ts:19). Populating Actual makes agent_runtime_hours fail getFeatureVisibility's limitCompliant check (limit && actual ? limit >= actual : true) once usage passes the allocation, so a UI wired through useFeatureVisibility would hide the runtime-hours panel at exactly the moment the customer needs to see it. Inert in this PR, since nothing under site/src reads that visibility flag yet. No code change and no response: git diff b5c9e8e4..cb1b9a3a1f -- site/src touches only typesGenerated.ts, and no comment on the PR engages it.

The honest part of that: CRF-34's line fell outside the diff, so the tooling folded it into the round-1 review body instead of opening a thread. There was nothing to reply to, which makes it the easiest item on the list to miss. Either reply here accepting it ("inert today, F1 owns it"), or carry it into the F1 ticket alongside the actual-is-hours correction already noted there. Either closes it. A one-line acknowledgement is enough; the finding does not ask for code.

On the rest, for the record rather than for action now:

Deferred with tickets, both accepted as deferrals: CRF-12 (entitlements endpoint auth model) to CODAGT-890, filed with both options and the swagger-versus-enforcement mismatch recorded, and CRF-23 (docs for runtime hours, allocation, limits) to CODAGT-891, due before C1. CRF-31 is closed the right way too: you checked Linear, found CODAGT-852 specifies threshold behavior rather than copy, and put that in the PR body naming the sentence that needs license-terms sign-off. That is the answer, not a deflection.

CRF-24 acknowledged with reasoning I am not going to argue with (a third threshold collides with the at-most-one-warning rule and prefigures F4; C1 owns HardLimit; if C1 dies the claim should be revisited rather than left inert).

CRF-30 and CRF-33 are contested, and I am deliberately not resolving them myself. Both defenses are substantive: on CRF-30, that >= means the message also fires at exactly 100 of 100 where "(0 over the limit)" reads wrong; on CRF-33, that Actual should measure locally recorded usage because gating on publish success would let a Tallyman outage reset the banner. Those are arguments about the design, and the panel should evaluate them rather than the orchestrator waving them through. They carry into the next round as contested, not as closed.

CI is green (23 passed, 9 skipped), which it was not at round 1.

Nothing else was reviewed this round. No panel ran, so no fix was verified: every "addressed" above is your claim plus the churn guard reading the diff, not the panel checking the result. Reply to CRF-34 or push a fix and the next round runs the panel against the whole change, including verification of the round-1 fixes.

🤖 This review was automatically generated with Coder Agents.

Twenty-six reviewers on the fix round. The round-1 fixes are real, and two of them are better than what the panel asked for: the query rewrite moved off the rollup to exact half-open bounds and documented the retention dependency the move creates, and queries_internal_test.go pins a cross-language literal whose breakage would otherwise be silent. Knuckle and Hisoka independently measured the new plan on loaded local Postgres (2.4 ms and 4.0 ms, index scan, ~8,760 rows against 500k and 2M unrelated rows), so the "no rollup needed" argument is now measured rather than argued. Komugi ran the new tests at -race -count=60 across three GOMAXPROCS settings, green. TestGetTotalUsageHBAgentRuntimeV1 is now a real mutation test: the foreign-event-type row carries a runtime_ms key, so dropping the event_type predicate fails it.

4 P2, 17 P3, 2 P4, 5 nits, 4 notes, plus 2 re-raises. No P0 or P1.

Two of the P2s are fix-chain regressions, which is the part I want to lead with because both trace to advice this panel gave.

CRF-41: the CRF-10 fix rejects the entire signed license for agent_runtime_hours_limit_soft: 0. Pariston proved the blast radius with a throwaway test rather than arguing it: HasLicense=false, audit logs, RBAC, HA and every other paid feature drop to not_entitled because one cosmetic threshold claim is zero. Round 1 told you to put that check in the validator. Knov and Ryosuke both said so, and Ryosuke explicitly argued against treating 0 as absent because it would "hide the license defect." That advice was wrong, and three reviewers independently say so this round. Everywhere else in the same claim loop a nonsensical feature value is skipped rather than fatal. The one-line alternative is in decodeAgentRuntimeHours: treat a zero soft limit as absent.

CRF-39 and CRF-40: the CRF-11 fix removed the real prefix collision and documented a constraint that does not exist, then introduced a new collision on the mechanism that does. Thirteen reviewers landed on the doc and the test. The claim "the dashboard's LicenseBanner dispatches on both" is false: the banner imports neither constant, and its only template-level dispatch is === against two placeholder-free strings, which is prefix-insensitive by definition. Meanwhile Chopper checked the mechanism that is real, Text.split("%d")[0], and both new constants now yield exactly "Your deployment has used ". I re-ran it: identical, and Leorio measured an 88-character shared prefix. So the constraint the comment invents is unenforceable, and the constraint every future classifier will actually derive is already violated between the two new texts. That also gives CRF-1 a mechanical fix instead of a frontend-only one.

CRF-1 is reopened and raised to P2. It was dropped in round 1 because both texts collided with the AI Governance prefix and rendered muted, which inverted the finding. The CRF-11 fix removed that collision, so both now take warningProminent. Six reviewers traced the render path independently. An operator at 80 of 100 hours, inside their allocation with nothing owed, gets the identical full-orange banner and the identical "Contact sales@coder.com." as an operator at 100 of 100 whose usage "may be billable". New evidence beyond round 1: the banner has no dismiss control and the usage period is the license term, so crossing a soft limit early in a year-long term means eleven months of a prominent banner nobody can clear, and with any second message the heading reads "Your license limits have been exceeded" above a message saying the deployment is still within its allocation.

CRF-35 is Netero's, and it arrived at the same defect as the unresolved @chatgpt-codex-connector thread. Netero's framing is the sharper one: because the two half-open ranges partition on bucket start, each bucket is counted exactly once, so this is misattribution rather than the double-charge CRF-9 was. One correction to Netero's own writeup, found in cross-check: the query doc does state the bucket-start rule at line 129. What it does not state is the consequence, that a bucket straddling a bound is attributed entirely to one side.

And then the thread nobody was pulling. Hisoka, Komugi and Pariston independently found that a bucket does not contain the runtime that elapsed in that hour (CRF-48). GetTotalChatMessageRuntimeMsInRange filters chat_messages.created_at, which this codebase documents as the transaction start time, and sums each row's full runtime_ms. So a turn running 10:50 to 11:20 puts all thirty minutes into whichever bucket its row landed in. I verified the chain. The consequence is that the doc's "Exact bounds" sentence is unreachable for three independent reasons, and that the remedy the codex thread proposes, prorating boundary buckets, prorates a quantity that was never proportional to the bucket. Komugi went further and showed with a live Postgres probe that now() does not advance inside a transaction, so a chat transaction outliving the eligibility lag lands rows in a sealed bucket and ON CONFLICT (id) DO NOTHING drops that runtime permanently (CRF-49). Pariston's read is the one to take seriously before C1: the event encodes a duration as a point event with only a bucket-start timestamp, so no reader can attribute a boundary correctly and every future reader will re-derive the same approximation. That is an upstream encoding decision, not this query's bug, but this PR is what turns the number into a customer-facing figure.

Two findings the churn guard had recorded as addressed came back, and both are on their original threads rather than as new IDs. CRF-14: the fix was comment-only by its own admission, and Mafu-san found the comment it added is false against the code in the same commit. CRF-32: the pin covers event_type and stops one literal short of runtime_ms, which four reviewers reached separately.

Both contested findings are closed, and neither by me. CRF-30 closed 12 of 13: the decisive check is that the AI Governance sibling can state an overage only because its trigger is strictly actual > limit, while this one fires on >= and AtAllocation pins it at exactly 100 of 100, so the sibling's phrasing is not transferable without the sibling's trigger. Mafuuu dissented on sentence shape and was outvoted. CRF-33 closed 12 of 12, including Knuckle who raised it: over-reporting locally recorded runtime is the safe direction, because gating on publish success would let a Tallyman outage silently clear an over-allocation banner. Residual disclosure filed as CRF-70.

Things worth protecting from the next fix pass. Do not gate the runtime block on Enabled to satisfy CRF-46; the finding is that the comment is wrong, not that the behavior is. Do not delete the hourMs literal or the hand-written literals in querier_test.go for the wrong reason: CRF-32's residual asks for the payload to be built from Fields(), not for the constants to be inlined. And keep appendAgentRuntimeHoursWarning; four reviewers verified it has exactly four reachable outcomes and a case for each.

Hisoka, on the one that moved: "the bucket is not the hour."

site/src/modules/dashboard/LicenseBanner/LicenseBannerView.stories.tsx:269

P3 [CRF-52] Two new user-facing warning classes ship with no story, so the rendering they were engineered around is pinned only from Go. (Nami P3, Kite P3)

AIGovernanceNearLimit, AIGovernanceOverLimitFromFeature and TelemetryRequiredError establish the project pattern: a backend warning class gets a story that renders LicenseBanner against a mock DashboardContext and asserts the text and the link. This PR adds two warning classes and no story. Neither new constant appears anywhere under site/src except typesGenerated.ts; both Nami and Kite grepped.

Nami's point about the asymmetry is the one that matters: "The Go test can only observe Go constants. Anyone editing isAIGovernanceNearLimitWarning, messageLink, or the variant dispatch changes what these two warnings look like, and every Go test still passes."

CRF-11 was exactly that class of break, and it was caught by a reviewer reading TSX, not by a test. CRF-1 is the same class again. Two stories driving LicenseBanner with the formatted soft-limit text and the formatted allocation text, asserting the variant class and the link, would close the gap and would fail today on CRF-1. site/AGENTS.md also makes this the required test form for this behavior rather than a nice-to-have.

🤖

enterprise/coderd/license/license.go:765

P3 [CRF-68] The CRF-15 fix replaced AddFeature with a direct map write in the new block and left the identical field-patch three lines above it. (Melody P3, Meruem P4)

The fix landed with a comment explaining why: routing through AddFeature "would silently drop the write if Feature.Compare ever stopped preferring a non-nil Actual". The managed-agent block, in the same function, still does the thing that comment argues against:

agentLimit.Actual = &managedAgentCount
entitlements.AddFeature(codersdk.FeatureManagedAgentLimit, agentLimit)

Both reviewers traced Compare for the managed-agent call and got the same answer: existing and add differ in exactly one field, every earlier step ties, and the write lands only at step 7, if f.Actual != nil && b.Actual == nil { return 1 }.

Melody's consequence: if that tiebreaker changes, Actual for FeatureManagedAgentLimit is silently dropped while LicenseManagedAgentLimitExceededWarningText still fires, because the warning reads the local managedAgentCount and not the feature. The banner then shows a limit-exceeded warning with no count behind it.

Meruem rated this P4 because nothing is broken today and the line is outside the diff; Melody rated it P3 because the two adjacent blocks now disagree about which mechanism is safe with the correct analysis written next to only one of them. Higher wins. One line, the same shape the PR already applied:

entitlements.Features[codersdk.FeatureManagedAgentLimit] = agentLimit

Raising it rather than merely reporting it because the author already did the analysis and applied it to one of the two instances.

🤖

enterprise/coderd/license/license.go:437

Note [CRF-61] CRF-13's rule holds at the two call sites the fix touched, and the same unauthenticated payload still carries a raw error from the line above. (Kurapika)

fmt.Sprintf("Invalid license (%s) parsing claims: %s", ..., err.Error()) puts a JWT parse error verbatim into entitlements.Errors, which GET /api/v2/entitlements serves without authentication (CRF-12, deferred to CODAGT-890). The fix generalized correctly from the runtime closure to the managed-agent closure; this is the third instance of the same class in the same function, and it predates the PR.

Recording rather than raising: the leak here is license-parse detail rather than schema detail, the line is outside the diff, and CODAGT-890 already owns the question of what that endpoint may expose. Worth having in the ticket, since "no raw errors on the entitlements payload" is a rule that is now true in two of three places and will read as true in all three to the next person who checks two of them.

🤖

🤖 This review was automatically generated with Coder Agents.

Two blockers and a split, then a panel round that found six more P2s in exactly the code Law says has not been reviewed.

The merged tree cannot start coderd. Migration version 000562 is already taken on main by 000562_oauth2_public_client_tokens. Law found it; I verified it before posting. git merge-tree --write-tree origin/main HEAD produces a tree containing all four files, and the collision is silent because the filenames differ. golang-migrate v4.19.0, which go.mod pins, returns source.ErrDuplicateMigration from iofs.go:69 when a second migration lands on an existing version, so the embedded source fails to open and every path that loads migrations fails before running any of them. main is at 000564; ./coderd/database/migrations/fix_migration_numbers.sh renumbers to 000565. Check against origin/main afterwards, not the merge base, which is four migrations stale.

That also reconciles a disagreement between two of my own reviewers about the red CI. Netero ran all four failing checks locally at head and every one passed, including the full make lint with the pinned golangci-lint and a complete offlinedocs export, and concluded the diff was not at fault. He was testing head, which has one 000562. CI checks out the merge ref, which has two. Razor reproduced the merge-ref failure and reports it accounts for all four checks; Netero's local runs bound how much of that is the diff itself. Either way, renumber first and re-read CI before spending a round on the code.

Law returned a mandatory split, and the panel then ran anyway because the tooling requires it once a panel has reviewed a PR. That turned out to matter. Law's argument was never about size: three independent concerns entered in 41bda6d145, the head commit, and no panel had seen them. The panel found six P2s this round and every one of them is in those three concerns, not in the feature. That is the split argument proving itself inside a single round.

Law's slices, in dependency order: (1) the write-path invariants, migration 000562 plus the InsertUsageEvent arbiter change; (2) usage-measurement diagnostics; (3) stop rejecting licenses over unusable runtime-hour claims; (4) the feature. He rejects cutting slice 4 further, by name. The counter-argument that splitting at round 4 discards continuity is real, and checkable: git log -S puts all three carved-out concerns in the head commit, so the continuity that exists covers the feature and the slices have none to lose.

The theme of this round is that both round-3 P2 fixes over-corrected toward silence, and the diagnostics rework traded one wrong signal for another.

Silence. CRF-41 said rejecting a whole signed license over agent_runtime_hours_limit_soft: 0 was disproportionate; the fix deleted validateAgentRuntimeHours and all five sentinel errors, so a license with a soft limit above its allocation now yields a healthy-looking deployment that never fires the warning the customer was sold, and TestAgentRuntimeHoursClaimTolerance pins that silence rather than catching it (CRF-73). CRF-51 asked that a duplicate bucket surface rather than be counted, which is the rationale the panel accepted it on; the fix made it silent and widened silent suppression to the insert every usage event type shares (CRF-71). In both cases the destination is defensible and the mechanism removed a signal nobody asked to remove.

Signals. CRF-43 asked that a failed usage query stop rendering as a license error. It now renders as a license limit warning: the four diagnostics are excluded from isMutedWarning, so a database blip paints the prominent orange banner under the heading "Your license limits have been exceeded" (CRF-83, ten reviewers). And demoting them from Errors to Warnings moved them out of coderd_license_errors, into the warnings gauge, and into the entitlement warning HTTP header, silently changing two documented Prometheus gauges that nothing in the PR body mentions (CRF-82).

Takumi's P2 is the one I would fix first after the P0, because it is load-bearing for three separate things this PR added. The cancellation bail is xerrors.Is(err, context.Canceled), and a real Postgres cancel does not return a context sentinel. Measured against this workspace's Postgres with coder's own lib/pq fork, 200 staggered cancels: 195 came back as pq.Error 57014, 5 as sentinels, and those 5 landed before dispatch. So a dropped browser request during POST /api/v2/licenses publishes a false deployment-wide "usage unavailable" warning that persists until the next refresh, and every shutdown that interrupts a refresh writes the error log the suppressor added in round 3 exists to prevent.

Two contradictions that a split would have caught. The migration says a duplicate bucket "surfaces as an insert error and gets logged"; the query doc in the same commit says it is silently suppressed. Netero settled it against a live database: INSERT 0 0. Three reviewers found the false claim appears in the migration twice, and four found a fourth stale ON CONFLICT (id) reference that Netero missed, in querier_test.go:10766, where it is a test's stated reason for passing. Migration files are the one artifact nobody edits again, and you are already renumbering this one.

One test that cannot fail. Zoro checked the CRF-32 fix rather than accepting it: the loop asserts each HBAgentRuntime.Fields() key appears in the generated SQL, and the SQL contains AS total_runtime_ms, so require.Contains(sql, "runtime_ms") passes even if the event_data->> key is renamed. I reproduced it. The pin added to close a silent-zero class is vacuous for the literal it was added for (CRF-89). Netero separately proved the new stories vacuous the same way: all fourteen pass with isMutedWarning stubbed to false (CRF-75).

What verified clean, so it is not relitigated: the half-open bounds and the @start_time/@end_time rename; the stable operator-facing strings with causes logged and no driver text on the payload; the two warning-text prefix relations the banner actually reads; make -B gen/db reproducing every generated file byte for byte; the CHECK constraint's timezone correctness, which Knuckle measured rather than assumed, along with his finding that it is not a lock problem, so do not reach for NOT VALID. Kurapika confirmed the new authorization check is correctly scoped and that no untrusted input reaches either path.

CRF-63 had no panel vote available when I first drafted this round, and I dropped it as orchestrator; Mafu-san and Pariston have since evaluated the defense and agree it is acceptable, so it is closed either way.

Law, on why this is a split and not a nag: "A contradiction that survives inside a single commit is what an unreviewed concern looks like."

enterprise/coderd/usage/generator.go:52

P3 [CRF-78] Three comments still name ON CONFLICT (id) DO NOTHING as the mechanism that makes concurrent replicas safe; the arbiter no longer exists. (Netero)

The Generator doc comment reads "Deterministic event IDs plus the database's ON CONFLICT (id) DO NOTHING make concurrent replicas safe without locking." Same dead reference at generator.go:160 and at querier_test.go:10766.

The last one matters most: it sits eight lines above TestGetTotalUsageHBAgentRuntimeV1 and states that the re-insert idempotency the test above it relies on comes from the (id) arbiter. So the next person to touch that test reads a false explanation of why it passes.

None of these three files is in the diff, and all three became wrong because of it. That is the signature of a change reaching past its own boundary, and it is a second piece of evidence for treating the write-path change as its own slice: the concern has consumers this PR never looked at.

🤖

enterprise/coderd/license/license.go:1018

P2 [CRF-84] The claim constants still document parse-time validation that this diff deleted, including a soft-limit range that now contradicts the decoder. (Mafuuu P2, Razor P2, Gon P2, Meruem P3, Leorio P3)

Deleting validateAgentRuntimeHours was the CRF-41 fix. The documentation that described it stayed. Five reviewers found the surviving claims, and they are on the constants a license issuer reads to learn what a valid license looks like.

Razor: the block still specifies parse-time validation and states a soft-limit range that the tolerant decoder no longer enforces, so the two now disagree about what a soft limit may be. Meruem counted four comments asserting a validation that no longer exists. Leorio found the one that matters most, at license.go:550: it is the comment a reader lands on when asking what happens to a bad claim, and it still promises rejection.

Gon's is the sharpest and is arguably its own defect: decodeAgentRuntimeHours documents a "concurrency-limited mode" in the present tense, and that mode exists nowhere in the repository. It is C1's future behaviour described as current fact, on the function a future implementer will read as the specification.

Consequence: the deployment now silently drops a claim the docs say will be rejected, and the only written description of the contract is the wrong one. This is the same shape as CRF-72, and it is why Law wants the validation change reviewed as its own slice: the code change was one function, the contract it invalidated is spread across four comments and two files.

🤖

codersdk/deployment.go:412

P3 [CRF-91] CRF-59's fix rewrote two bullet lists in this struct and left the third, so the regenerated table still ships a malformed cell one row below the row it fixed. (Kurapika P3, Leorio P3, Gon P3, Ryosuke Nit, Knov P4)

Five reviewers. docs/reference/api/schemas.md:7799 still renders the usage_period description as a run-on cell with inline hyphens, exactly the defect CRF-59 described, in the same generated table, one row below the row this PR fixed and regenerated.

Knov's framing is the one to act on: the fix rewrote two doc comments instead of the generator that flattens them, and the surviving broken cell is the one the fixed row's own text refers the reader to. So the PR fixed the sentence that points at a cell it left broken.

Two ways to close it. Rewrite the third bullet list as prose the way the other two were, which is the cheap, in-scope fix and makes the whole table consistent. Or teach the doc-comment-to-table generator to render lists, which is out of scope here but is the reason this keeps recurring. Kurapika notes this PR regenerates the table either way, so the marginal cost of the first option is one comment and a make gen you are already running.

🤖

enterprise/cli/server.go:148

P4 [CRF-92] The only usage.Cron job registration omits EventType, so Register rejects it, the error is discarded, and no hb_ai_seats_v1 event is ever produced. (Kite P4, Melody P4)

Two reviewers found this independently while tracing the created_at alignment question for the new CHECK constraint. Register validates that a CronJob carries an EventType and returns an error when it does not; the single production call site omits the field and the returned error is discarded, so the job is never registered and the event type is never generated.

Outside this diff, and reported rather than requested. It is here for two reasons. It is the second writer to usage_events that the new CHECK constraint would apply to, which is how both reviewers found it, so anyone reasoning about that constraint's blast radius needs to know this writer is currently inert. And Kite notes the discarded error is what makes it invisible: the deployment looks configured.

If hb_ai_seats_v1 is expected to be flowing in production today, this is worth checking before it becomes someone's billing discrepancy. A ticket is the right home.

🤖

🤖 This review was automatically generated with Coder Agents.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d0e5c875a

The mandatory split is discharged. Sixteen reviewers addressed it independently and all sixteen concluded the three concerns are reviewable in place. Netero supplied the mechanical evidence that each is independently tested at head; Bisky ran those tests rather than reading them; Knuckle, Takumi, Knov, Razor, Meruem, Ryosuke, Zoro, Leorio, Pariston, Hisoka, Luffy, Killua and Pen Botter each checked the seams from their own domain and reached the same answer. "I don't have standing to restructure the PR into four" is an honest statement about authority rather than a dodge, and the reshaping is genuine partial compliance.

One residual, recorded rather than resolved, because it is a human call about merge strategy. Mafu-san split Law's two legs and only one is discharged. Leg (a), three unreviewed concerns positioned to merge, is gone: the round-4 panel reviewed exactly those concerns and found six P2s in them. Leg (b) is about revert granularity and no amount of in-place seam hygiene answers it. A squash merge makes the migration, the diagnostics rework and the claim tolerance one revert unit, and the piece most likely to need an independent revert is the migration, because it validates existing rows and deliberately aborts the upgrade on a violator. Reverting it post-merge drags the feature with it. Mafuuu reached the same place from the contract seat: revertibility is one-directional.

Round 4's CRF-71 was wrong and your correction is right. I asked for verification rather than acceptance and got it twice. Netero removed the race-resolution branch, leaving the arbiter alone, and TestGeneratorConcurrentReplicas failed 4 of 4 runs with pq: duplicate key value violates unique constraint "idx_usage_events_agent_runtime"; with the branch restored, 8 of 8 pass. Takumi confirmed the interleaving under the race detector. The mechanism is exactly as you described: ON CONFLICT (id) DO NOTHING absorbs concurrent same-id inserts only on the arbiter index, so the finding's claim was true before this PR added a second unique index and false after. The branch is load-bearing, not defensive decoration. Correcting a panel with a reproducible test is the right way to handle a review finding you think is wrong, and it worked.

The P0 is fixed and I verified it against the merge tree rather than the branch: git ls-tree over git merge-tree --write-tree origin/main HEAD now shows only 000562_oauth2_public_client_tokens at that version and the runtime-hours migration at 000565.

Four P2s, five P3s, four nits. The one to fix first is Hisoka's, and it is uncomfortable because this panel caused it.

CRF-102: the CRF-87 fix matches SQLSTATE 57014, and Postgres uses 57014 for statement_timeout kills as well as client cancels. I verified the chain: IsQueryCanceledError matches any pq.Error with that code, measureUsage treats it as "the caller went away" and fails the whole LicensesEntitlements call, and enterprise/coderd/coderd.go:791 runs updateEntitlements inside New() and returns the error. So on a deployment with an aggressive statement_timeout and a slow usage query, every refresh aborts with only a Warn log, entitlements freeze at the last good snapshot, none of the usage-unavailable diagnostics this PR built for exactly this case ever render, and coderd crash-loops on restart. On main the same 57014 landed in Errors as a diagnostic and the refresh completed. Round 4 asked you to catch 57014 from a client cancel, which was right; nobody said 57014 is only a client cancel. Hisoka's bound is the cheap fix: treat 57014 as an abort only when ctx.Err() != nil, or distinguish on pq.Error.Message. Not reproduced against a real timeout, and the 57014-for-statement_timeout mapping is standard Postgres, so this is full severity anyway.

That is the fourth consecutive round in which a fix this panel asked for introduced a new defect, and Mafu-san flagged the narrower version of the same pattern: CRF-101 is the third consecutive round in which a review fix shipped a new error branch with zero coverage. I am not asking you to fix the pattern. I am naming it because at this point the shape of our findings is part of the cause: a review that says "catch X" without saying "and here is what X also means" keeps producing this.

Nine reviewers landed on the claims-ignored warning (CRF-103) from nine directions, which is the widest convergence of the round. It names no license and no claim, nothing is logged server-side, and it fires once per parsed license, so a customer who does what it says still sees it. Pariston's version is the one to read: a superseded license keeps the banner up after the replacement lands.

What verified clean, so it is not relitigated: the arbiter restore and all four stale comment sites; the existsErr sites' behaviour under a real race; the migration's two constraints, each exercised at the SQL layer; the CRF-89 pin now asserting event_data->>'field' rather than a bare key; CRF-99 verified by two reviewers reading the head tree; and CRF-75 verified by mutation, with the storybook suite now failing 5 tests when isMutedWarning is stubbed, which is the exact mutation Netero used to prove the gap in round 3. CRF-81 and CRF-88 were accepted with reasoning that four reviewers independently judged sound, and the migration now states the deliberate abort in the artifact.

No comments folded into the review body this round. I checked all twenty-two candidate lines against the diff hunks before drafting, which is the fix for a failure mode that has cost this review two rounds.

Luffy, on the banner: "OI THIS PART IS GREAT. The warnings finally act like what they are."

🤖 This review was automatically generated with Coder Agents.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c29d9cd60f

Blocked on three unanswered nits. Eleven of the fourteen open findings were addressed, including both P2s, and the two that mattered most were addressed well. This is a process gate, and clearing it costs three replies.

Unaddressed, with no code change and no response on any of their threads:

CRF-112 (coderd/database/querier_test.go:10806). The CHECK-constraint assertion hand-copies the constraint name into ErrorContains where its sibling sixteen lines up uses the generated database.Check* constant.

CRF-113 (codersdk/licenses_test.go:53). One mutual-prefix assertion still guards a classifier that does not exist, and its failure message describes that classifier as real.

CRF-114 (enterprise/coderd/license/license_test.go:3298, now :3425). Hand-rolled map copy loop where maps.Copy applies.

I want to be precise about why these block, because the last time I blocked this review it was on a finding you had no way to reply to. That is not the case here. All three are inline threads, all three are still unresolved, and none has a reply. A one-line "not doing this, and here is why" closes any of them; the gate is a response, not a change. If you disagree with all three, say so and they close as contested.

On the round-5 fixes, recorded rather than reviewed, because no panel ran: the churn guard classifies CRF-101 through CRF-111 as addressed, including CRF-102, where the 57014 match is now bounded, and CRF-101, where the unique-violation fallback distinguishes the exists-check failure. Those are the two I would most want verified, and they have not been. Every "addressed" here is a diff reading, not a panel checking the result, and four rounds running this review has found a fix that was incomplete or that introduced a new defect.

Separately, and independent of the gate: CI is red on Flake Check (run 92803291237). That job runs the changed tests at -count=35 -shuffle=on -parallel=16 against real Postgres, which is exactly the shape of run that finds a test whose coverage depends on a schedule landing. I tried to narrow it and could not reproduce it locally at those settings for enterprise/coderd/usage (TestGenerator), the full enterprise/coderd/license package, or coderd/database (TestGetTotalUsageHBAgentRuntimeV1, TestUsageEventsTrigger), all with DB=ci. So this is an inability to reproduce, not a diagnosis, and the failing test name is in the job log rather than anything I can see from here.

Worth connecting to a finding you already have: Bisky's CRF-109 said in round 5 that TestGeneratorConcurrentReplicas's only positive coverage is a scheduling accident, and asked for two deterministic cases that cannot pass by luck. The churn guard reads that as addressed. If Flake Check is failing on that test, the deterministic cases are both the fix and the proof.

Reply to the three nits, or push a change, and the next round runs the panel against the whole diff including verification of the round-5 fixes.

🤖 This review was automatically generated with Coder Agents.

This is the first round where nothing the panel found is about how the code behaves. Two P2s, five P3s, three nits and a note, and every one of them is documentation, test coverage, or a comment. Six reviewers returned nothing at all, which has not happened in seven rounds.

The round-5 P2s are both verified fixed, by execution rather than reading, and by reviewers who did not coordinate. CRF-102: seven reviewers independently confirmed the abort now keys on the computation's own ctx.Err() rather than on SQLSTATE 57014, so a statement_timeout can no longer wedge entitlements or stop coderd booting. Meruem's framing is the one worth keeping, that the fix addresses the root cause rather than the symptom, because keying on the caller's context is what the branch always meant. CRF-101: the exists-check failure is carried with errors.Join(err, existsErr) instead of discarded, and the branch that had zero coverage now has three deterministic cases.

CRF-109 deserves its own paragraph because it is the finding that predicted the round-6 CI failure class. Bisky asked for coverage that could not pass by luck, and TestGenerateBucketUniqueViolation pins all three unique-violation outcomes without racing for any of them. Five reviewers verified it by running it.

On the round-6 red: Netero settled it by mutation. With the TestEntitlements/FullLicense special-case guard disabled, the test fails 1 of 1 at head against real Postgres via the claims-ignored warning; with it, it passes. So that was a deterministic failure surfacing under -count=35, not a schedule-dependent flake, and there is nothing left to chase there. Worth noting the failure and CRF-107 shared a root cause: agent_runtime_hours is not a valid claim name, which is exactly the silent-drop the finding described, and the production half was fixed separately by adding the feature name to ignoredClaims.

Law re-ran, because effective additions grew 669 since his last analysis, and returned don't split, advisory. His reasoning is worth quoting because it closes the thread properly rather than by fatigue: the round-4 mandatory split targeted this same concern set and was discharged 16/16, new evidence would have to be a new concern or a new risk domain entering the diff, and he verified the opposite, that every line added since is remediation of panel findings inside the already-enumerated concerns. "The scrutiny a split would have bought has been delivered by process." One residual stands unchanged and is a merge-strategy call for a human, not a code finding: under a squash merge the slices are one revert unit, and the migration is the piece most likely to need an independent revert because it deliberately aborts the upgrade on a violator. Law notes slice A is self-contained and could be cherry-picked out before merge if independent revertibility is wanted.

The two P2s are both recurrences, and I want to be straight about one of them.

CRF-115: the PR body still says a canceled query "(context sentinel or SQLSTATE 57014) aborts the refresh", which is precisely the design the CRF-102 fix removed. Five reviewers caught it. This is the same class as CRF-86, which you fixed in round 4 by rewriting the body against the head commit; the body has gone stale again because the fix changed the behaviour it described. Under squash merge the body is the permanent commit message, so a future bisect lands on a message describing a classifier the commit deleted.

CRF-116: fifth consecutive round of comment duplication, and the commit that claimed to deduplicate introduced two verbatim copies twenty lines apart. I am keeping it at P2 for consistency with how I rated the same finding in round 5, but the severity is clearly not the lever, so the ask changes. The last four passes deleted instances. That is why it keeps coming back: deleting a copy does not decide where the fact lives. Pick the owning location for each fact once, put the rationale there, leave a one-line pointer everywhere else, and stop rewriting rationale during fix passes, which is where every new copy has come from.

Everything else is small and specific: three test gaps (CRF-117, CRF-118, CRF-121), one layering residue from CRF-105 that the measureUsage extraction half-closed (CRF-119), one migration comment that mislabels what the alignment CHECK buys (CRF-120), and three nits.

Kite's note is the one I would not lose: a zero-value FeatureArguments.Logger silently discards the ignored-claims detail that the published warning tells the operator to go look for. Nothing in this PR constructs one, so it is not a defect today.

Bisky, on the stories, which is a sentence I did not expect to be writing about this file three rounds ago: "real tests, not costume jewelry."

🤖 This review was automatically generated with Coder Agents.