Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughAdds configurable per-model completion limits with a process-local sliding-window limiter. The limiter applies to exec and interactive turn sessions, including provider escalation. Configuration validation, error hints, tests, and user documentation are also added. ChangesPer-model RPM limits
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant CLI
participant ConfiguredTurnSessions
participant ModelRPMLimiter
participant TurnSessionProvider
CLI->>ConfiguredTurnSessions: provide profile and limiter options
ConfiguredTurnSessions->>ModelRPMLimiter: wrap the selected model session
CLI->>ModelRPMLimiter: call Stream
ModelRPMLimiter->>TurnSessionProvider: forward admitted Stream call
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change adds process-local per-model admission limits before provider streams, and the inspected configuration flow preserves the documented cap behavior. No actionable merge-blocking risk is established; it appears ready for normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The request cap can be silently lost when startup enters provider setup to repair an incomplete configuration. Later ordinary turns in that process can then run without the configured cap. This is a bounded admission-control defect; no broader privilege expansion was established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
LortuArte
marked this pull request as ready for review
October 4, 2026 07:36
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #390 — approved RPM-only slice. This does not implement the broader cost/TPM request.
Summary
Zero currently has no local per-model RPM admission gate at the turn-session boundary.
This change adds a process-local sliding 60-second request window keyed by resolved model ID and rejects request N+1 before the wrapped
TurnSession.Streammethod is entered.The limiter is wired into:
zero execwhile preserving the existing provider/transport selection.
FileConfig.modelRPMconfigures the cap. Unset or zero means unlimited.Project configuration may tighten an existing user limit, but may not relax it.
Admitted requests consume their slot even when the provider subsequently fails.
Scope / non-goals
This PR is intentionally limited to the scope discussed in #390:
zero execerrorIt does not add:
Separate processes have independent windows and restarting Zero clears the limiter state.
The limiter counts
TurnSession.Streamadmissions. It does not claim to count setup/prewarm traffic, compaction traffic, provider-internal retries, discovery calls, or requests made outside this boundary.See
docs/MODEL_RPM.md.Regression proof
The regression tests verify that request N+1 is rejected before entering the wrapped transport.
As a negative control, I temporarily bypassed the admission wrapper.
With the gate removed:
N+1 was not denied: <nil>The wrapper was then restored before final validation.
This verifies that the new tests fail when the behavior they are intended to protect is removed.
Concurrency result
A 100-request concurrent test with a configured RPM limit of 7 produced:
No additional admitted request reached the wrapped transport.
Validation environment
Original validation run: 2026-09-27.
Revalidated for delivery on 2026-10-03.
Before opening this draft, I rechecked the upstream repository and confirmed that
mainstill points to the exact validated base:99721c762f37cd43ac511007a5f51d1846df959eThe submitted branch was compared against the preserved validation files before publication.
Tested on:
99721c762f37cd43ac511007a5f51d1846df959ePassing checks
-race: PASSmake fmt-check: PASSgo vet ./...: PASSgo run ./cmd/zero-release build: PASSgo run ./cmd/zero-release smoke: PASSmake vulncheck: PASS (No vulnerabilities found)git diff HEAD --check: PASSgit apply --check: PASSFull-suite limitation
go test ./...does not pass completely in this validation environment.It reports 14 failing tests.
I repeated the same suite on an unmodified clean checkout of
mainat:99721c762f37cd43ac511007a5f51d1846df959eunder the same environment and network restrictions.
The same 14 test names fail on clean
main.No additional failed-test names were introduced by this patch.
The reproduced failures are:
TestAltScreenTranscriptScrollKeepsFooterFixedTestPlatformTransportRoundTripTestServeCleansStatusThroughBoundDirectoryAfterSwapTestServePreservesPreviousStatusWhenReplacementFailsTestServeRemovesSocketBoundAfterFinalRuntimeSwapTestServeSupportsReadOnlyCustomRuntimeDirectoryTestServerEndToEndTestServerPublishesDefaultStatusAfterCrashReportCreatesRuntimeDirectoryTestServerRejectsUnknownCommandTestServerSecondInstanceFailsTestTerminateAndReapDaemonProcessTestTerminateCommandKillsChildAfterLeaderExitsTestTerminateCommandStopsUnconfiguredCommandTestUnixTransportFallbackAndSocketModeThe failures include Unix-socket restrictions (
operation not permitted), daemon/process lifecycle tests and one transcript-title assertion.This comparison is evidence that the same failed-test names reproduce on the clean base in this runner.
It is not being presented as a full-suite PASS and it does not establish macOS or Windows validation.
No unrelated test was disabled or modified to hide these failures.
Static lint
make lint-staticis also not completely clean in this environment.It reports four findings:
internal/installtest/workflow_permissions_test.go:20:19— QF1001internal/proxydial/proxydial.go:67:15— QF1008internal/proxydial/proxydial.go:72:33— QF1008internal/tools/web_fetch.go:315:21— QF1008The same four findings reproduce on clean
main, and these files are not modified by this PR.I left the unrelated code unchanged.
Network isolation note
The new CLI fixtures explicitly inject a simulated/no-op MCP runtime.
For the broader validation run, external HTTP was rejected using a local deny-only HTTP proxy so tests did not make live MCP requests.
That proxy is not an OS-level network sandbox.
No credentials, live provider accounts, or real funds were used by the new fixtures.
The lint/vulnerability tooling was allowed to obtain its normal public tooling/data.
Windows revalidation — 2026-10-04
After the initial draft review, I fixed a recovery-path issue where
ModelRPMcould be lost whenResolvereturnedErrNoActiveProvider.Regression evidence:
ModelRPM[gpt-test] = 0./internal/configpassesgo test ./internal/config ./internal/cli -count=1: PASSgo test -race ./internal/config ./internal/cli -count=1: PASSgo vet ./...: PASSgo run ./cmd/zero-release build: PASSgo run ./cmd/zero-release smoke: PASSgovulncheck: PASS (No vulnerabilities found)git diff HEAD --check: PASSWindows environment:
The repository-wide
go test ./...is still not all-green in this Windows environment.On the PR branch, one full-suite run failed in
internal/toolsatTestExecCommandForegroundServerReturnsSessionAndServesHTTP; that same test then passed in isolation.I then created a detached clean worktree at the exact base commit:
99721c762f37cd43ac511007a5f51d1846df959eOn that clean base:
TestExecCommandForegroundServerReturnsSessionAndServesHTTPpassed 5/5 in isolationgo test ./...run still failed, but in a different unrelated baseline test:TestACPEndToEndPromptwithcontext deadline exceededI have not modified unrelated tests or runtime code to hide these environment/baseline failures.
Latest fix commit:
9de88d736ed1d1202e1093d958bcb44d7c1f2836Maintainer review status
The RPM implementation, recovery-path regression fix, focused tests, race validation, vet, build, smoke, formatting, vulnerability scan and diff hygiene all pass locally.
The repository-wide Windows suite is not all-green, but the clean base also produces unrelated full-suite failures under the same environment.
The PR is ready for project CI and maintainer review. I have intentionally left unrelated baseline/environment failures unchanged.
Disclosure
AI-assisted implementation and local verification.
This PR does not claim maintainer endorsement, Zero adoption of AEGIS, or production certification.
Summary by CodeRabbit
modelRPM. Limits apply to interactive turns andzero exec; requests over the limit are rejected locally with guidance on when to retry.