Autoresume timeout persisted on resume#1969
Conversation
|
@codex review |
|
@claude review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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". |
matthewlouisbrockman
changed the title
matthewlouisbrockman
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
matthewlouisbrockman
force-pushed
the
autoresume-timeout-persisted-on-resume
branch
from
7a84b9f to
fc25db9
Compare
matthewlouisbrockman
left a comment
matthewlouisbrockman
left a comment
There was a problem hiding this comment.
things i want to fix
PR SummaryMedium Risk Overview Written by Cursor Bugbot for commit ff90629. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| return timeout | ||
| } |
There was a problem hiding this comment.
Min floor overrides team cap in clamp logic
Low Severity
clampAutoResumeTimeout applies the minimum floor after the team-plan cap. If minAutoResumeTimeout exceeds teamPlanLimit, the returned timeout will exceed the team's plan limit, contradicting the PR's stated guarantee of "maximum of user's team's maximum." The min feature flag (default 60s) and team limits (in hours) make this unlikely today, but a future flag adjustment could silently violate the team cap.
There was a problem hiding this comment.
it won't violate it
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| return clampAutoResumeTimeout(timeout, getTeamPlanLimit(team), minAutoResumeTimeout) | ||
| } |
There was a problem hiding this comment.
This whole file is effectively one function from the outside world, especially if you account for autoResume being nil (as it does on line 40). Can we merge this to a single function? It'd be much more readable.
There was a problem hiding this comment.
And if you pass in the feature flag client, it's even simpler.
There was a problem hiding this comment.
good comment, adding to the improvements
3 participants
Persists the timeout between resumes for autoresume requests. Tries to respect the user's preference, but does a minimum of 1 minute and maximum of user's team's maximum for the timeout.