feat: add agent runtime hour license claims and entitlement feature - #27459
Conversation
Licenses can now carry three claims for agent runtime hours: agent_runtime_hours_allocation, agent_runtime_hours_limit_soft, and agent_runtime_hours_limit_hard (unit: hours). They surface as the new usage-period feature agent_runtime_hours in GET /api/v2/entitlements, where limit carries the allocation and the new optional soft_limit and hard_limit fields carry the thresholds. Invalid claim combinations (soft/hard without allocation, negative allocation, soft outside 0 <= soft < allocation, hard < allocation) reject the entire license, following the validateClaims precedent. Soft and hard limits are not comparison inputs in Feature.Compare; they ride along with whichever license wins (newest iat, existing behavior). Old servers ignore all three claims since none of them is a feature name, protecting rollout of licenses minted with the new claims. Part of CODAGT-837. The claim name constants defined here are the canonical contract for github.com/coder/license (X1).
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 11 findings (4 P3, 1 P4, 5 Nit, 1 Note), APPROVE. Review Finding inventoryFinding inventory - PR #27459Findings
Round logRound 2Churn guard PROCEED: 9 addressed, 0 silent, in commit ccd4572 (head 3463d7e..ccd4572). Panel (14: Netero first-pass + Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Ging-go, Gon, Leorio, Ryosuke, Knov, Chopper, Kurapika, Komugi, and wildcard Zoro). Netero clean. All nine R1 findings verified fixed at root cause on encounter (CRF-5 mutation-tested load-bearing by Bisky and Mafu-san, which also resolved Netero's doubt about the assertion). New code ( Round 1Panel (18: Netero first-pass + Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Ging-go, Gon, Leorio, Kurapika, Knov, Ryosuke, Chopper, Melody, Komugi, Robin, Luffy, and wildcards Meruem, Razor). Netero clean at mechanical floor (P3 + Note only), so panel proceeded. 4 P3, 1 P4, 4 Nit. No P0/P1; event COMMENT. Reviewed against 671173b..3463d7e. Design-level Notes recorded but not posted as line findings (covered in review body): whole-license rejection blast radius on a bad claim combo (author's confirmed reject-license stance, fail-loud; Pariston/Hisoka/Mafuuu), claim-name string contract drift with the licensor X1 (Pariston/Hisoka), About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped groundwork. Three flat license claims (agent_runtime_hours_allocation, _limit_soft, _limit_hard) map onto one usage-period feature agent_runtime_hours, mirroring the managed-agent path rather than inventing a new mechanism. The panel verified the load-bearing invariants hold: validation runs on every parse path (ParseClaims and ParseClaimsIgnoreNbf both route through validateClaims), the raw feature-name claim is skipped so an unvalidated allocation cannot win Feature.Compare precedence, soft/hard ride along with the winning license via AddFeature whole-struct replacement (never merged), and the feature is correctly excluded from Premium/Enterprise defaults. Bisky on the tests: "this test suite is lovely, and this time it's real." Test density is 83.4% and the assertions check real numbers and wire keys, not mocks echoing themselves.
Severity count: 4 P3, 1 P4, 4 Nit. No P0/P1, so this is a COMMENT, not a request for changes. The strongest finding is a validation gap this PR itself introduces (allocation=0 accepting a positive hard ceiling); the rest are coverage, maintainability, naming, and doc-generation edges.
Design observations the human should weigh, none blocking and none needing a code change here:
- Whole-license rejection blast radius: a single malformed threshold rejects the entire license (400 on upload;
HasLicense=falseand every feature dropped for a stored license). This is the confirmed reject-license stance (decision 2) and fails loud rather than shipping a wrong limit; no field-minted license can carry these brand-new claim names, so the practical window is a future licensor bug. Worth confirming the blast radius is acceptable versus degrading to ignoring only the malformed feature. - Cross-repo name contract: the claim constants are declared canonical for
github.com/coder/license(X1) but matched by exact string with no mechanical link. A rename or typo on either side falls through the unknown-claim tolerance and the feature silently vanishes. Worth generating or checking the licensor's bounds against these same constants when X1 lands, rather than hand-duplicating. soft_limit/hard_limitare served onGET /api/v2/entitlements, which is registered outsideapiKeyMiddlewareand is unauthenticated. Same disclosure class as thelimit/enabled/entitlementalready exposed there, so nothing new, but the enforcement follow-ups should not treat these thresholds as secret.- soft/hard are inert data today (no reader in production; B4/C1/F1 are follow-ups). Correct scope, flagged so nobody reads three numbers off the entitlements response and assumes something enforces them.
FeatureAgentRuntimeHoursis added toUsesLimit()but that arm never fires (theUsesUsagePeriod()case precedes it). Harmless and consistent withFeatureManagedAgentLimit, which sets both; removing it would change nothing.
🤖 This review was automatically generated with Coder Agents.
| // hard limit claims can be set with Feature using | ||
| // license.ClaimAgentRuntimeHoursLimitSoft and | ||
| // license.ClaimAgentRuntimeHoursLimitHard. | ||
| func (opts *LicenseOptions) AgentRuntimeHours(allocation int64) *LicenseOptions { |
There was a problem hiding this comment.
P3 [CRF-1] The new exported AgentRuntimeHours test builder has zero callers and is itself untested. (Netero)
Its sibling
ManagedAgentLimit(same file, line 237) has three production-test callers ... Every test added in this PR bypasses the builder and constructs the map directly
Because no test exercises it, a wrong mapping in the helper (for example pointing at the soft-limit claim) would ship undetected. Either delete it or use it in the new tests so it is covered. Assuming the usage-wiring follow-ups will call it is a deferral without a caller in this diff.
🤖
There was a problem hiding this comment.
remove if its not used
There was a problem hiding this comment.
Fixed in ccd4572. Removed the AgentRuntimeHours builder. Every test constructs license.Features directly, so it had no callers and no coverage; the usage-wiring follow-ups can add it back with a caller.
Generated with Coder Agents on behalf of @jaaydenh.
There was a problem hiding this comment.
Verified fixed in ccd4572. The unused AgentRuntimeHours builder is removed; tests construct license.Features directly with the claim constants, so no dangling reference remains.
🤖
…ndings - Remove the unused AgentRuntimeHours license test builder. - Reject agent runtime hours soft and hard limit claims when the allocation is 0, since a zero allocation disables the feature and a zero hard limit would read as a ceiling that blocks all agent runtime. - Move claim-to-feature decoding into decodeAgentRuntimeHours so the loop no longer remaps and attaches limits in separate places. - Pin the rollout invariant that no agent runtime hours claim name is a feature name. - Assert the entitlement warning is suppressed for agent runtime hours. - Rename the error vars to carry Hours and name the offending claims. - Document HardLimit so the generated API schema describes it.
|
/coder-agents-review All nine R1 findings are addressed in
|
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Round 2. All nine round-1 findings are fixed at the root cause and pinned, verified on the current code rather than trusted from the replies. The zero-allocation gap (CRF-2) is closed symmetrically with an explicit allocation == 0 && (hasSoft || hasHard) guard ahead of the bound checks, and ZeroAllocationWithPositiveHard pins the exact scenario. The three-site claim knowledge (CRF-4) is collapsed into a single decodeAgentRuntimeHours plus an isAgentRuntimeHoursClaim skip, which also eliminated the load-bearing claim-consumption ordering the old inline code depended on. CRF-5 was the one that could have been faked, so Bisky and Mafu-san both deleted the suppression branch and reran: the test failed with the warning present, so the NotContains assertion is genuinely load-bearing. The rollout invariant (CRF-3) is now pinned by ClaimNamesAreNotFeatureNames, and the four doc/naming nits are all resolved. Coverage of the three new functions reads 100% of statements. Bisky on the tests: "Usually this much jewelry is costume. I went through every stone. They're real."
The fix commit is proportional: no unrequested refactor rode along beyond the decodeAgentRuntimeHours/isAgentRuntimeHoursClaim pair that CRF-4 asked for, and the commit message maps one-to-one onto the diff.
Approving. Two minor new items and one process nit, none blocking:
- CRF-10 (Nit): the three claim functions disagree on receiver style. Inline.
- CRF-11 (Note): the claim string literal values are not pinned by any test, so a value rename (not a name rename) would stay green while silently breaking minted licenses. Inline, cheap to close.
- CRF-12 (Nit, process): the fix commit subject
fix(enterprise/coderd/license): ...uses a scope that does not contain every changed file. Per AGENTS.md, a scope must be a real filesystem path containing every changed file; this commit also touchescodersdk/deployment.go,coderd/apidoc/*,docs/reference/api/schemas.md,site/src/api/typesGenerated.ts, andenterprise/coderd/coderdenttest/coderdenttest.go. Drop the scope for this cross-cutting change (fix: address agent runtime hours review findings); thefeatcommit and PR title already get this right.
Carryover design notes from round 1 still stand and still need no code change: soft/hard remain inert data until B4/C1/F1 (Leorio flags that the hard_limit doc says "enforcement ceiling" as present fact while nothing enforces it yet, borderline against the neighboring usage_period wording); GET /api/v2/entitlements remains unauthenticated but soft/hard are the same license-quota disclosure class already served there; and the ride-along-not-merge behavior (a newer allocation-only license nulls an older license's thresholds) is the single load-bearing assumption B4 must design around, now pinned by NewerIssuedAtWinsOverSoftHardLimits.
🤖 This review was automatically generated with Coder Agents.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
Just prose nits and one observation.
| // runtime hour claims. Invalid combinations reject the entire license rather | ||
| // than ignoring the feature, since these claims form a contract with the | ||
| // license issuer. | ||
| func (f Features) validateAgentRuntimeHours() error { |
There was a problem hiding this comment.
Observation: why not roll the validation into decoding? Or is the separation of decoding and validation a pre-existing convention?
There was a problem hiding this comment.
If this is in validation, then its a fail fast and loudly situation when the license is uploaded. I thought that was better for avoiding invalid combinations of the soft limit, hard limit, and runtime allocation. As far as I can tell decoding doesn't have a clear way to surface like validation does. The thinking is that this helps when there is more complexity in the licenses.
2 participants
closes CODAGT-837
Licenses can now carry three agent runtime hour claims:
agent_runtime_hours_allocation,agent_runtime_hours_limit_soft, andagent_runtime_hours_limit_hard(unit: hours). They surface as the new usage-period featureagent_runtime_hoursinGET /api/v2/entitlements, wherelimitcarries the allocation and the new optionalsoft_limit/hard_limitfields oncodersdk.Featurecarry the thresholds.Invalid combinations reject the entire license via
validateClaims(both at upload and when computing entitlements for stored licenses): soft/hard without allocation, negative allocation, soft outside0 <= soft < allocation, orhard < allocation.Soft and hard limits are not comparison inputs in
Feature.Compare; they ride along with whichever license wins (newestiat, existing behavior). None of the three claim names is a feature name, so old servers ignore them via the existing unknown-claim tolerance, protecting rollout of licenses minted with the new claims.The claim name constants defined in
enterprise/coderd/licenseare the canonical contract forgithub.com/coder/license(X1).Part of CODAGT-837. Blocks B4 (usage wiring + warnings), C1 (hard-limit admission gate), F1 (licenses page), A4 (managed-agent coexistence), X1 (licensor).
Out of scope, handled by follow-up issues:
Actualusage wiring, threshold warnings, admission gating, premium defaults, and FE surfacing beyond regenerated types.Implementation plan and decision log
Decisions (confirmed by jaayden, 2026-07-23)
agent_runtime_hours_allocation- allocation (unit: hours, int64)agent_runtime_hours_limit_soft- soft limitagent_runtime_hours_limit_hard- hard limitFeatureName; all three map to the single new usage-period featureagent_runtime_hours(FeatureAgentRuntimeHours), mirroring howmanaged_agent_limit_softmapped ontomanaged_agent_limit. Old servers therefore ignore all three claims via theFeatureNamesMapcheck.validateClaims(upload returns 400 viaParseClaimsIgnoreNbf; already-stored licenses produce anInvalid license ... parsing claimsentitlements error and contribute nothing).Design notes
codersdk.Featurehad aSoftLimitfield until 051ed34 ("feat: convert soft_limit to limit", feat: convertsoft_limittolimit#22048) collapsed managed-agent soft/hard into a singlelimit. This reintroduces soft/hard as optional fields without changing managed-agent behavior.UsagePeriodfromnbf/exp(usagePeriodStart/usagePeriodEndinLicensesEntitlements); reused unchanged, consistent with managed agents.Entitlements.AddFeaturereplaces wholeFeaturestructs (no merging), so soft/hard automatically ride along with the winning license. NoFeature.Comparelogic change; doc updates plus tests pin that soft/hard are not comparison inputs.FeatureManagedAgentLimit; usage-based warnings arrive with B4.Changes
codersdk/deployment.go: newFeatureAgentRuntimeHours(inFeatureNames,UsesLimit(),UsesUsagePeriod(), keeping it out ofFeatureSetexpansion);Feature.SoftLimit/Feature.HardLimit(soft_limit/hard_limit, omitempty); doc updates forUsagePeriodandCompare.enterprise/coderd/license/license.go: canonical claim constants; validation helper called fromvalidateClaims; al-la-carte loop maps the allocation claim to the feature and attaches soft/hard from the companion claims; skips for the companion claims and the raw feature name; generic warning loop skip.enterprise/coderd/coderdenttest:AgentRuntimeHours(allocation)builder.TestAgentRuntimeHoursLicenses: entitled/grace round-trips (including JSON field assertions), allocation-only, explicit zero,IssuedAtRankingmirror, soft/hard ride-along with a newer allocation-only license, direct feature-name claim ignored, unknown-claims compatibility (old-server simulation).TestAgentRuntimeHoursClaimValidation: table of valid/invalid claim combinations againstParseClaims, plus stored-license entitlements error.TestPostLicense: API-level 400 rejection and a happy-path POST +GET /api/v2/entitlementsround-trip.TestFeatureComparison: soft/hard ignored in comparison; newestiatwins over larger soft/hard.make gen: regeneratedsite/src/api/typesGenerated.ts,coderd/apidoc/*,docs/reference/api/*.Verification
go test ./enterprise/coderd/license/ ./codersdk/andgo test ./enterprise/coderd/ -run 'TestPostLicense|TestEntitlements'pass.golangci-lintclean on changed packages;make lint/emdashclean; FEtsc --noEmitclean.Note
Generated by Coder Agents on behalf of @jaaydenh (Linear CODAGT-837 agent session).