Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c56c95ebec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if base != nil && current.RegistrationEpoch != base.RegistrationEpoch { | ||
| return merged | ||
| } | ||
| merged.Quota = mergeQuotaObservation(current.Quota, updated.Quota) |
There was a problem hiding this comment.
When the current credential has an active credential_quota cooldown, the normal UpdateRefreshedAuth path subsequently executes auth.Quota = existing.Quota in Manager.updateInternal, overwriting the observation merged here. Consequently, a successful Devin refresh during that cooldown still discards its new plan and remaining-quota signals before persistence or the management UI can see them; the added test only calls this helper directly and misses the overwrite. Preserve only the existing cooldown fields at that later merge, or reapply the refreshed observation afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5ae3e63f. The regression now exercises Register → a concurrent credential-quota cooldown → UpdateRefreshedAuth, so it covers the later Manager.updateInternal merge. Refresh mode now applies only the existing cooldown fields to the newly merged quota observation; replace and prepare behavior remains unchanged. The affected package tests, go vet, and server build pass.
c56c95e to
5ae3e63
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ae3e63fe2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if base != nil && current.RegistrationEpoch != base.RegistrationEpoch { | ||
| return merged | ||
| } | ||
| merged.Quota = mergeQuotaObservation(current.Quota, updated.Quota) |
There was a problem hiding this comment.
When a subsequent Devin status response omits an optional value such as plan, a reset timestamp, or a plan date, DevinExecutor.Refresh clones the base quota and conditionally overwrites only values present in the response (internal/runtime/executor/devin_executor.go:203-223). This merge now copies that inherited map after its ObservedAt has been advanced, so omitted values from an older response are incorrectly presented to the management UI as part of the latest snapshot. Clear/rebuild updated.Quota.Signals from the status response before merging it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6cd99935. DevinExecutor.Refresh now rebuilds Quota.Signals from an empty map for every successful status response, so optional values omitted by the latest response cannot survive from the cloned base. The existing Refresh test now seeds stale plan-period and unknown signals and verifies that they are removed, while the manager-level test still verifies that active cooldown fields survive persistence.
5ae3e63 to
6cd9993
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
1 participant
Fixes #5812.
DevinExecutor.Refreshrecords a fresh quota snapshot on the updated auth, butMergeRefreshedAuthstarts from the current auth and never carries that snapshot across. As a result, refreshed Devin plan and remaining-quota signals disappear before the management UI can read them.This change merges the refreshed observation after the registration-epoch guard using the existing
mergeQuotaObservationhelper. The newest observation reaches the merged auth while cooldown and scheduler fields from a concurrent current auth remain intact. Stale refreshes from an earlier registration epoch are still rejected.Validation:
go test -count=1 ./sdk/cliproxy/... ./internal/api/handlers/managementgo vet ./sdk/cliproxy/auth ./internal/runtime/executor ./internal/api/handlers/managementgo build -o test-output ./cmd/serverAssistance disclosure: implementation and verification were AI-assisted and reviewed against the repository contribution rules.