Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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.
LGTM — straightforward, additive protobuf schema change.
What was reviewed: the four new request_start_timeout optional int32 fields in controller.proto (field numbers 20/14/17/18, all previously unused, no collisions with existing fields); the regenerated controller_pb2.py/.pyi were checked for consistency with the .proto (field accessors, HasField/ClearField/WhichOneof literals, and the serialized-descriptor byte-offset shifts all line up as expected from a mechanical protoc regen); and the new parametrized tests in test_proto.py, which correctly use HasField/ClearField/FromString round-tripping and a legacy DescriptorPool to verify optional-presence semantics and backward/forward wire compatibility.
Extended reasoning...
The change adds one new optional int32 field (request_start_timeout) to four existing protobuf messages in controller.proto, plus the corresponding regenerated controller_pb2.py/.pyi and new tests; it touches no auth, crypto, or data-handling logic, only schema/wire format. I confirmed all four new field numbers are previously unused in their messages (no collisions), the .pyi diff shows exactly the expected additions in the expected locations, and the pb2.py diff is the expected mechanical serialized-descriptor blob plus byte-offset shifts consistent with a protoc regen. Tests follow existing file conventions and correctly exercise optional-field presence and wire compatibility with a legacy schema. This is small, self-contained, and mechanical enough that no human look is strictly necessary.
Adds optional
request_start_timeoutfields toMachineRequirements,UpdateApplicationRequest,ApplicationInfo, andAliasInfofor an app's default queue-wait timeout. Processing and runner-startup timeouts retain their existing fields and wire tags.Deploy/update requests use positive seconds to set the default, omission to preserve it under existing deployment/update rules, and an explicitly present
0to clear it. Responses omit the field when no app default is configured. The controller follow-up must translate explicit zero to an unset database value before positive-value validation.Bindings were regenerated with
tools/regen_grpc.pyagainst isolate v0.26.12. Tests cover optional presence, serialization of explicit zero, existing processing-timeout fields, and compatibility with older protobuf readers.Validation: all 23 isolate-proto tests and
pre-commit run --all-filespassed.This supplies the protocol for the controller/SDK follow-up after an isolate-proto release.
Note
Low Risk
Additive protobuf fields with backward-compatible wire tags; behavior depends on controller follow-up to honor and validate the new field.
Overview
Introduces
request_start_timeouton the controller API so apps can configure a default queue-wait limit separately fromrequest_timeout(processing) andstartup_timeout.The optional field is added to
MachineRequirements(deploy/register via nested requirements),UpdateApplicationRequest, and read modelsApplicationInfoandAliasInfo. Semantics: positive seconds set the default; omission preserves the current value on update/deploy (per existing inheritance rules); explicit0clears the default on write paths. Responses leave the field unset when no app default exists.Python
controller_pb2/.pyiwere regenerated. New tests cover optional presence, wire round-trip,0vs omitted, field numbers, and forward/backward protobuf compatibility with readers that only knowrequest_timeout.Reviewed by Cursor Bugbot for commit 902cbd0. Bugbot is set up for automated code reviews on this repo. Configure here.