feat(ratelimit): wire admission-aware rate limiting into the server - #45
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
|
Temporarily converted back to draft. This PR activates primitives from #44, which was merged with a reproduced policy-transition P1. Hotfix #47 must merge first. After that, rebase/merge dev into this branch, run the policy-transition and admission suites together, and request a fresh exact-head review before marking ready. |
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
3846dd3 to
39e1f21
Compare
Includes the policy-transition safety fix from #47 before enabling runtime admission routing.
|
Exact-head re-review requested for Please rerun, not infer:
Keep this PR draft until exact-head CI and this review are clean. |
|
[code]smith (@codesmith-bot) independently review exact head |
|
CodeRabbit (@coderabbitai) review Review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== HEAD =='
git rev-parse HEAD
git status --short
echo '== Commit =='
git show --no-patch --format=fuller 3dea136c72086ba0c9e81a27a9c2074c9aa556ec
echo '== Files changed from first parent =='
git diff-tree --no-commit-id --name-status -r 3dea136c72086ba0c9e81a27a9c2074c9aa556ec
echo '== Rate limit candidates =='
fd -i 'ratelimit|rate-limit|rate_limit' . || true
rg -n -i -C2 '\brateLimit\b|\brate.limit\b|RateLimit|rate-limit' \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' . | head -n 1000
echo '== TypeScript outlines for candidate files =='
while IFS= read -r f; do
case "$f" in
*.ts|*.tsx|*.js|*.mjs|*.cjs)
echo "--- $f"
ast-grep outline "$f" --items all 2>/dev/null || true
;;
esac
done < <(fd -i 'ratelimit|rate-limit|rate_limit' .)
echo '== Relevant test references =='
rg -n -i -C3 'websocket.*global|policy.transition|overflow|invalid.key|snapshot|retry-after|rateLimit' \
tests -g '*ratelimit*' 2>/dev/null || trueLength of output: 69612 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Merge-aware file changes =='
git diff-tree -m --no-commit-id --name-status -r 3dea136c72086ba0c9e81a27a9c2074c9aa556ec | sort -u
echo '== Server admission control =='
nl -ba src/server/rate-limit.ts | sed -n '1,310p'
echo '== Token bucket and concurrency primitives =='
nl -ba src/ratelimit/token-bucket.ts | sed -n '1,330p'
nl -ba src/ratelimit/concurrency.ts | sed -n '1,240p'
echo '== Configuration validation =='
nl -ba src/config.ts | sed -n '670,750p'
echo '== Principal resolution and management authentication =='
nl -ba src/server/auth-cors.ts | sed -n '240,305p'
nl -ba src/server/management-auth.ts | sed -n '175,220p'
echo '== All admission and reservation call sites =='
rg -n -C8 '\.gate\(|\.afterAuth\(|\.reserveConcurrency\(|rateLimitRelease|server\.upgrade\(' \
src/server/index.ts src/server -g '*.ts'
echo '== Server WebSocket handler and upgrade path =='
nl -ba src/server/index.ts | sed -n '250,440p'
nl -ba src/server/index.ts | sed -n '930,1035p'
echo '== Exact-head admission tests =='
nl -ba tests/ratelimit-admission.test.ts | sed -n '1,1100p'
echo '== Exact-head policy-transition tests =='
nl -ba tests/ratelimit-policy-transition.test.ts | sed -n '1,500p'Length of output: 2030 🏁 Script executed: #!/bin/bash
set -euo pipefail
show() {
local file="$1" start="$2" end="$3"
echo "== ${file}:${start}-${end} =="
sed -n "${start},${end}p" "$file"
}
show src/server/rate-limit.ts 1 310
show src/ratelimit/token-bucket.ts 1 330
show src/ratelimit/concurrency.ts 1 240
show src/config.ts 670 750
show src/server/auth-cors.ts 240 305
show src/server/management-auth.ts 175 220
echo '== Exact call-site lines and context =='
rg -n -C12 '\.gate\(|\.afterAuth\(|\.reserveConcurrency\(|rateLimitRelease|server\.upgrade\(' \
src/server -g '*.ts'
show src/server/index.ts 250 440
show src/server/index.ts 930 1035
echo '== Route registration lines =='
rg -n -C8 'fetch\(|pathname|/v1/responses|/v1/chat/completions|/v1/messages|count_tokens|/api/|images|live|realtime|models' \
src/server/index.ts
echo '== Admission test declarations =='
rg -n -C8 '^(describe|test|it)\(' tests/ratelimit-admission.test.ts tests/ratelimit-policy-transition.test.ts
echo '== Admission tests (part 1) =='
sed -n '1,600p' tests/ratelimit-admission.test.ts
echo '== Admission tests (part 2) =='
sed -n '601,1200p' tests/ratelimit-admission.test.ts
echo '== Policy transition tests =='
sed -n '1,520p' tests/ratelimit-policy-transition.test.tsLength of output: 146731
I found no P1 or P2 blocker in the reviewed admission-control code.
I could not run the requested runtime reproductions, test suites, or CI in this sandbox. The environment permits static inspection only and prohibits executing repository code. Exact-head CI and the dynamic WebSocket and policy-transition suites must still pass before the PR is marked ready. ✅ Action performedReview finished.
|
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Independent review of exact head Reproductions executed (controlled-clock scripts against the real primitives):
Auth/429 precedence: every gated route follows the documented contract: WebSocket release paths: reservation acquired before Snapshot redaction: drove validated key, invalid key, management token, remote-address, and anonymous principals plus WS reservations through a live admission control; the serialized Finding (P2, fixed): the Non-blocker (P3 follow-up): rate-limit aggregates are not yet projected into Validation on the fixed head: |
![View with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg)
|
Exact-head reset after fixing a newly reproduced management-plane precedence defect. Current head: CodeRabbit (@coderabbitai) review
All reviews on |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='22ee3e5f11b9b1467fe4214aa8481bbef64a409e'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\n' "$actual"
test "$actual" = "$expected"
git status --short
printf '\n== Diff against dev merge-base ==\n'
base="$(git merge-base HEAD dev)"
printf 'merge-base=%s\n' "$base"
git diff --name-status "$base"...HEAD
printf '\n== Candidate files ==\n'
fd -i 'rate-limit|ratelimit|management-auth|auth-cors|config|index' src tests || true
printf '\n== Source outlines ==\n'
for f in src/server/rate-limit.ts src/ratelimit/token-bucket.ts src/ratelimit/concurrency.ts src/server/management-auth.ts src/server/auth-cors.ts src/server/index.ts src/config.ts; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all
fi
done
printf '\n== Focused call sites ==\n'
rg -n -C 10 '\.gate\(|\.afterAuth\(|\.reserveConcurrency\(|rateLimitRelease|server\.upgrade\(|Origin|requestIP\(|snapshot\(' \
src/server src/ratelimit src/config.ts -g '*.ts'
printf '\n== Focused test declarations and assertions ==\n'
rg -n -C 6 'rejected Origin|origin|websocket|upgrade|close|policy transition|low.to.high|high.to.low|overflow|snapshot|fingerprint|credential|Retry-After' \
tests -g '*ratelimit*.test.ts'Length of output: 1669 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Available refs and exact commit =='
git branch -a
git show --no-patch --format=fuller HEAD
echo '== Parents =='
git show --no-patch --format='%P' HEAD
echo '== Files changed from first parent =='
git diff-tree --no-commit-id --name-status -r HEAD
echo '== Source outlines =='
for f in src/server/rate-limit.ts src/ratelimit/token-bucket.ts src/ratelimit/concurrency.ts src/server/management-auth.ts src/server/auth-cors.ts src/server/index.ts src/config.ts; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all
fi
done
echo '== Management and limiter call sites =='
rg -n -C 12 '\.gate\(|\.afterAuth\(|\.reserveConcurrency\(|rateLimitRelease|server\.upgrade\(|Origin|requestIP\(|snapshot\(' \
src/server src/ratelimit src/config.ts -g '*.ts'
echo '== Relevant source slices =='
nl -ba src/server/rate-limit.ts | sed -n '1,330p'
nl -ba src/ratelimit/token-bucket.ts | sed -n '1,360p'
nl -ba src/ratelimit/concurrency.ts | sed -n '1,280p'
nl -ba src/server/management-auth.ts | sed -n '150,245p'
nl -ba src/server/auth-cors.ts | sed -n '230,330p'
nl -ba src/config.ts | sed -n '650,780p'
nl -ba src/server/index.ts | sed -n '240,460p'
nl -ba src/server/index.ts | sed -n '900,1060p'
echo '== Focused regression test declarations and bodies =='
for f in tests/ratelimit-management-origin.test.ts tests/ratelimit-admission.test.ts tests/ratelimit-policy-transition.test.ts; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n -C 10 '^(describe|test|it)\(|rejected Origin|same.origin|exhaust|websocket|upgrade|close|low.to.high|high.to.low|overflow|snapshot|fingerprint|credential' "$f"
fi
doneLength of output: 140421 🏁 Script executed: #!/bin/bash
set -euo pipefail
numbered() {
local file="$1" start="$2" end="$3"
echo "== ${file}:${start}-${end} =="
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
numbered src/server/rate-limit.ts 90 270
numbered src/ratelimit/token-bucket.ts 70 280
numbered src/ratelimit/concurrency.ts 1 190
numbered src/config.ts 660 750
numbered src/server/management-auth.ts 185 245
numbered src/server/auth-cors.ts 260 305
numbered src/server/index.ts 378 460
numbered src/server/index.ts 790 845
numbered src/server/index.ts 985 1025
echo '== Every WebSocket release-handle reference =='
rg -n -C 8 '\brateLimitRelease\b|\breserveConcurrency\s*\(' src tests -g '*.ts'
echo '== Admission snapshot producers and consumers =='
rg -n -C 8 '\.snapshot\(\)|statsSnapshot\(|bucketCounts\(|RateLimitAggregateSnapshot|rateLimit.*snapshot' \
src tests -g '*.ts'
for f in tests/ratelimit-management-origin.test.ts tests/ratelimit-admission.test.ts tests/ratelimit-policy-transition.test.ts; do
echo "== $f =="
awk '{ printf "%6d %s\n", NR, $0 }' "$f"
doneLength of output: 123771
I found no P1 or P2 blocker in the reviewed admission-control paths.
I could not execute the requested runtime reproductions, Bun test suites, or CI. This sandbox permits static inspection only. Keep the PR draft until exact-head CI and the dynamic management-Origin, WebSocket, and policy-transition suites pass. ✅ Action performedReview finished.
|
Independent review of exact head Root cause of the fixed defect, confirmed in code: Scenario results (gate-level with controlled principals, plus live server):
Validation: Non-blocking notes (P3): the unmetered cross-origin management path is safe (it requires a valid admin token and does no handler work beyond the 403), and rate-limit aggregates are still not projected into PR: #45 |
Triage 2026-08-02 ADR (OCX → Sofie / Pi-Rust)Classification: Locked ADR: Sofie = autonomous OCX runtime; OpenCodex TS = donor/evidence, not automatic Rust/Pi architecture. Sofie deploy today
Why keep (not supersede)
Direction
|
|
yone_suke (@codesmith) independently audit exact head |
Independent audit of exact head A. Validated-vs-anonymous classification (src/server/auth-cors.ts, management-auth.ts): a configured data key is B. Management Origin 403 precedence: valid admin + rejected Origin before exhaustion → C. Token policy transitions (token-bucket.ts:229 D. WebSocket reservation lifecycle: cap-1 denial returns 429 with integer E. Snapshot redaction: after driving validated/invalid/management principals and a held reservation, the serialized Validation at this head: |
Combine the merged ChefVault provider-security plane with admission-aware rate limiting. Preserve the ChefVault auth/config DTO changes, isolate opaque rate-limit principal resolution, and augment the canonical config types without rewriting the shared types module.
|
Exact-head integration review required for yone_suke (@codesmith) independently execute the focused ChefVault + rate-limit suites and inspect the two conflict-isolation modules:
Report or fix every P1/P2 blocker; prior reviews on |
|
CodeRabbit (@coderabbitai) review Review only exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='f8e2b4e4bce7b90fda0a45454f31a91e565d59a5'
actual="$(git rev-parse HEAD)"
printf 'HEAD=%s\n' "$actual"
if [ "$actual" != "$expected" ]; then
echo "MISMATCH: expected $expected"
fi
git status --short
printf '\n== Diff against dev merge-base ==\n'
base="$(git merge-base HEAD dev)"
printf 'merge-base=%s\n' "$base"
git diff --name-status "$base"...HEAD
printf '\n== Candidate conflict-isolation files ==\n'
fd -i 'rate-limit-auth|config-types' src
printf '\n== Outlines ==\n'
for f in src/server/rate-limit-auth.ts src/ratelimit/config-types.ts src/config.ts; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all
fi
done
printf '\n== safeConfigDTO references ==\n'
rg -n -C 10 'safeConfigDTO|credentialRef|hasApiKey' src -g '*.ts'Length of output: 1231 🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin dev --quiet || true
git branch -a | head -n 20
base="$(git merge-base HEAD origin/dev 2>/dev/null || true)"
printf 'merge-base=%s\n' "${base:-none}"
if [ -n "${base:-}" ]; then
git diff --name-status "$base"...HEAD
fi
printf '\n== Candidate conflict-isolation files ==\n'
fd -i 'rate-limit-auth|config-types' src
printf '\n== Outlines ==\n'
for f in src/server/rate-limit-auth.ts src/ratelimit/config-types.ts src/config.ts; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all
fi
done
printf '\n== safeConfigDTO references ==\n'
rg -n -C 10 'safeConfigDTO|credentialRef|hasApiKey' src -g '*.ts'Length of output: 52496 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== rate-limit-auth.ts (full) =='
nl -ba src/server/rate-limit-auth.ts
echo '== config-types.ts (full) =='
nl -ba src/ratelimit/config-types.ts
echo '== ratelimit/index.ts relevant exports =='
rg -n 'RateLimitSurface|rateLimitFingerprinter|RateLimitPrincipal' src/ratelimit/index.ts | head -50
echo '== callers of resolveDataPlaneRateLimitPrincipal =='
rg -n -C6 'resolveDataPlaneRateLimitPrincipal' src -g '*.ts'
echo '== isDataPlaneAdmissionSecret def and usage =='
rg -n -C8 'isDataPlaneAdmissionSecret' src -g '*.ts'
echo '== OcxRateLimitConfig usages =='
rg -n -C4 'OcxRateLimitConfig' src -g '*.ts'
echo '== types.ts rateLimit field =='
rg -n -C6 'rateLimit\??:' src/types.ts
echo '== rate-limit.ts principal usage near gate/afterAuth =='
nl -ba src/server/rate-limit.ts | sed -n '1,260p'Length of output: 1215 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== rate-limit-auth.ts (full) =='
cat -n src/server/rate-limit-auth.ts
echo '== config-types.ts (full) =='
cat -n src/ratelimit/config-types.ts
echo '== ratelimit/index.ts relevant exports =='
rg -n 'RateLimitSurface|rateLimitFingerprinter|RateLimitPrincipal' src/ratelimit/index.ts | head -50
echo '== callers of resolveDataPlaneRateLimitPrincipal =='
rg -n -C6 'resolveDataPlaneRateLimitPrincipal' src -g '*.ts'
echo '== isDataPlaneAdmissionSecret def and usage =='
rg -n -C8 'isDataPlaneAdmissionSecret' src -g '*.ts'
echo '== OcxRateLimitConfig usages =='
rg -n -C4 'OcxRateLimitConfig' src -g '*.ts'
echo '== types.ts rateLimit field =='
rg -n -C6 'rateLimit\??:' src/types.ts
echo '== rate-limit.ts principal usage near gate/afterAuth =='
cat -n src/server/rate-limit.ts | sed -n '1,260p'Length of output: 18065 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== tsconfig include settings =='
cat -n tsconfig.json 2>/dev/null | sed -n '1,80p' || true
fd -e json -i tsconfig
echo '== Is config-types.ts imported anywhere? =='
rg -n "config-types" src -g '*.ts'
echo '== rateLimitConfigSchema (zod) full definition =='
cat -n src/config.ts | sed -n '660,745p'
echo '== Does OcxConfig get exported/used with rateLimit field checked elsewhere (type-level) =='
rg -n -C4 'config\.rateLimit\b' src -g '*.ts'
echo '== Check src/types.ts import graph / whether config-types augmentation is picked up via any barrel import =='
rg -n 'from "\./ratelimit/config-types"|from "\.\./ratelimit/config-types"|import .*config-types' src -g '*.ts'
echo '== ratelimit/index.ts head (to see if config-types re-exported) =='
cat -n src/ratelimit/index.ts | sed -n '1,40p'Length of output: 1767 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== rateLimitConfigSchema (zod) full definition =='
cat -n src/config.ts | sed -n '660,745p'
echo '== Does OcxConfig get exported/used with rateLimit field checked elsewhere (type-level) =='
rg -n -C4 'config\.rateLimit\b' src -g '*.ts' || true
echo '== Check for import of config-types via side-effect import anywhere =='
rg -n 'config-types' src -g '*.ts' || true
echo '== ratelimit/index.ts head =='
cat -n src/ratelimit/index.ts | sed -n '1,40p'
echo '== management-auth.ts principal resolution 175-220 =='
cat -n src/server/management-auth.ts | sed -n '175,225p'Length of output: 11166
Merge quality of the isolation split
I could not execute the Bun test suites or CI in this sandbox; this remains a static-only review, consistent with every earlier round on this PR. Keep the PR draft until exact-head CI and the dynamic ChefVault + rate-limit suites confirm the above statically-verified boundaries at runtime. ✅ Action performedReview finished.
You are interacting with an AI system. |
Integration review of exact merge head Conflict-isolation modules:
ChefVault contract: relative to Reruns at this head, all green: management and Validation: P3 notes (non-blocking): the credential-header chain now exists in two textual copies ( PR: #45 |
|
[code]smith (@codesmith-bot) Apply two focused follow-ups on existing branch
Run typecheck plus focused ratelimit tests before pushing one coherent commit. |
OnlineChef
left a comment
OnlineChef
left a comment
There was a problem hiding this comment.
Live-sideband concurrency remains the final runtime lifecycle gap.
OnlineChef
left a comment
OnlineChef
left a comment
There was a problem hiding this comment.
The remaining E2E expectation still describes the removed pre-auth limiter behavior.
OnlineChef
left a comment
OnlineChef
left a comment
There was a problem hiding this comment.
The management E2E contains the same removed pre-auth assumption.
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…ive-sideband concurrency - /api/* now rejects cross-origin requests in the router before commit() (mirrors /metrics), so a 403 can never consume the management admission budget. This supersedes the surface-specific isAllowedManagementOrigin guard inside commit() (694bbd0), which is removed again: the auth-first gate keeps HTTP policy out of the limiter, exactly as 5feafac intended. - Live-sideband WebSocket upgrades reserve a concurrency slot after the upgrade resolves and before the handshake completes, roll it back when Bun refuses the upgrade, and release it via the existing idempotent close() handle. - Unit coverage for live reservations (per-principal/global caps, rollback, idempotent release) plus an e2e sideband reservation/release test; stale pre-auth-charging comments in index.ts and remaining docs wording (websocket scope, per-surface handshake charging) updated. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
Both follow-ups are done and pushed to PR #45 (head now What happened While I was working, the CI-autofix session pushed 3 overlapping commits to the branch (
My commit adds
Validation: typecheck clean, all 6 focused suites pass (53 tests), privacy scan green. Full suite: only the 5 known pre-existing sandbox failures (cli-help/service path assertions) + GUI jsx-dev-runtime errors — unrelated. PR body updated: sideband follow-up sentence replaced with the shipped behavior, and the two merge-gate checkboxes ticked. Remaining gates are cross-platform CI green on this head and the exact-head revalidation. |
|
greptile-apps Revalidate exact head
Report any concrete reproducible defect against this exact head; otherwise mark safe to merge. |
|
CodeRabbit (@coderabbitai) review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected_short='847c2f5'
head="$(git rev-parse HEAD)"
printf 'HEAD=%s\n' "$head"
case "$head" in
"$expected_short"*) ;;
*) echo "ERROR: expected HEAD prefix ${expected_short}" >&2; exit 2 ;;
esac
printf '\n== Commit and change scope ==\n'
git show --no-patch --format=fuller HEAD
base="$(git merge-base HEAD dev 2>/dev/null || true)"
printf 'merge-base-with-dev=%s\n' "${base:-unavailable}"
if [ -n "${base:-}" ]; then
git diff --name-status "$base"...HEAD
fi
printf '\n== Candidate files ==\n'
fd -i 'rate-limit|ratelimit|management-auth|auth-cors|config|index' src tests || true
printf '\n== Source outlines ==\n'
for f in \
src/server/rate-limit.ts \
src/server/rate-limit-auth.ts \
src/server/management-auth.ts \
src/server/index.ts \
src/server/auth-cors.ts \
src/ratelimit/token-bucket.ts \
src/ratelimit/concurrency.ts \
src/ratelimit/config-types.ts \
src/config.ts; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all
fi
done
printf '\n== Admission and WebSocket call sites ==\n'
rg -n -C 10 \
'\.gate\(|\.commit\(|\.afterAuth\(|reserveConcurrency\(|rateLimitRelease|server\.upgrade\(|resolveLiveSidebandUpgrade|isAllowedManagementOrigin|snapshot\(' \
src tests -g '*.ts'
printf '\n== Focused test declarations ==\n'
rg -n -C 5 \
'^(describe|test|it)\(|invalid.*(key|credential)|Origin|origin|live.sideband|websocket|upgrade|rollback|close|effective.default|policy.transition|snapshot|redact|fingerprint|credential' \
tests -g '*ratelimit*.test.ts'Length of output: 181595 🏁 Script executed: #!/bin/bash
set -euo pipefail
show() {
local file="$1" start="$2" end="$3"
printf '\n== %s:%s-%s ==\n' "$file" "$start" "$end"
nl -ba "$file" | sed -n "${start},${end}p"
}
show src/server/rate-limit.ts 90 255
show src/server/rate-limit-auth.ts 1 80
show src/server/management-auth.ts 185 235
show src/ratelimit/token-bucket.ts 80 280
show src/ratelimit/concurrency.ts 55 165
show src/config.ts 665 745
show src/server/index.ts 800 865
show src/server/index.ts 1015 1045
show tests/server-live.test.ts 540 650
show tests/ratelimit-auth-precedence.test.ts 1 125
show tests/ratelimit-management-origin.test.ts 1 110
show tests/ratelimit-admission.test.ts 90 140
show tests/ratelimit-admission.test.ts 210 275
show tests/ratelimit-admission.test.ts 425 510
show tests/ratelimit-admission.test.ts 530 610
printf '\n== Current rate-limit documentation and stale pre-auth wording ==\n'
rg -n -i -C 3 \
'rate.?limit|pre.?auth|anonymous.*(charge|bucket|limit)|429.*401|401.*429|sideband.*(reserv|concurren)' \
README.md docs .github -g '!node_modules/**' -g '!dist/**' -g '!build/**' 2>/dev/null || true
printf '\n== Full current-HEAD diff for directly reviewed files ==\n'
git show --format= --find-renames HEAD -- \
src/server/index.ts \
src/server/rate-limit.ts \
src/server/rate-limit-auth.ts \
src/ratelimit/concurrency.ts \
src/config.ts \
tests/ratelimit-admission.test.ts \
tests/ratelimit-auth-precedence.test.ts \
tests/server-live.test.ts \
tests/ratelimit-management-origin.test.ts \
README.md docs 2>/dev/null || trueLength of output: 1940 🏁 Script executed: #!/bin/bash
set -euo pipefail
show() {
local file="$1" start="$2" end="$3"
printf '\n== %s:%s-%s ==\n' "$file" "$start" "$end"
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
show src/server/rate-limit.ts 90 255
show src/server/rate-limit-auth.ts 1 80
show src/server/management-auth.ts 185 235
show src/ratelimit/token-bucket.ts 80 280
show src/ratelimit/concurrency.ts 55 165
show src/config.ts 665 745
show src/server/index.ts 800 865
show src/server/index.ts 1015 1045
show tests/server-live.test.ts 540 650
show tests/ratelimit-auth-precedence.test.ts 1 125
show tests/ratelimit-management-origin.test.ts 1 110
show tests/ratelimit-admission.test.ts 90 140
show tests/ratelimit-admission.test.ts 210 275
show tests/ratelimit-admission.test.ts 425 510
show tests/ratelimit-admission.test.ts 530 610
printf '\n== Current rate-limit documentation and stale pre-auth wording ==\n'
rg -n -i -C 3 \
'rate.?limit|pre.?auth|anonymous.*(charge|bucket|limit)|429.*401|401.*429|sideband.*(reserv|concurren)' \
README.md docs .github -g '!node_modules/**' -g '!dist/**' -g '!build/**' 2>/dev/null || true
printf '\n== Current-HEAD diff for directly reviewed files ==\n'
git show --format= --find-renames HEAD -- \
src/server/index.ts \
src/server/rate-limit.ts \
src/server/rate-limit-auth.ts \
src/ratelimit/concurrency.ts \
src/config.ts \
tests/ratelimit-admission.test.ts \
tests/ratelimit-auth-precedence.test.ts \
tests/server-live.test.ts \
tests/ratelimit-management-origin.test.ts \
README.md docs 2>/dev/null || trueLength of output: 106076
I found one P2 blocker. P2 — Failed live-sideband upgrades are recorded as successful In This creates a false successful-upgrade record for every failed Bun upgrade. It affects request-log accuracy and any downstream metrics or diagnostics that consume these records. Move the The reviewed admission boundaries otherwise match the requested behavior:
Cross-platform CI is reported green. The failed-upgrade request-log status still requires correction before this PR is safe to merge. You are interacting with an AI system. |
|
Final merge audit for exact head
Merge basis: resolved reproduced findings + exact-head manual security/lifecycle audit + fully green exact-head CI. Merging with expected-head SHA lock. |
1 participant
Scope
Integrates bounded admission rate limiting into the OCX server. The feature is opt-in and default OFF: configs without
rateLimit, or withenabled: false, preserve existing request behavior and emit no rate-limit headers.Security and admission model
Originand forwarded headers are never identity or bypass signals.Covered surfaces
Management
/api/*and/metrics, model discovery, Responses HTTP/compact/WebSocket, Chat Completions, Claude Messages/count-tokens, images, search, live/realtime, and canonical sideband upgrades.Responses WebSocket and live-sideband reservations are acquired before upgrade, rolled back on failed upgrade, stored as idempotent release handles, and released on every close path. Cross-origin
/api/*requests are rejected in the router before the limiter commits, matching/metrics, so a 403 never consumes admission budget.Combined security base
The branch preserves the current ChefVault/provider-security behavior rather than overwriting shared files:
src/server/auth-cors.tskeeps safe DTOcredentialRefandhasApiKeyhandling.src/types.tskeepsOcxProviderConfig.credentialRef.src/server/rate-limit-auth.tsreuses the canonicalisDataPlaneAdmissionSecretboundary.src/ratelimit/config-types.tsaugmentsOcxConfigwithout rewriting provider types.Configuration and responses
The strict
rateLimitsubtree rejects unknown surfaces, invalid rates/bursts/caps, contradictory enable/bypass settings, and effective WebSocket limits where a supplied value conflicts with its default-resolved counterpart.Rate-limit settings are startup-scoped. Stop/restart OCX after changing them.
429 responses use integer
Retry-After,RateLimit-Limit,RateLimit-Remaining, andRateLimit-Resetheaders,Cache-Control: no-store, and native OpenAI, Anthropic, or management envelopes. Aggregate snapshots expose no principal, credential, fingerprint, prompt, model, account, Origin or remote address.Relationship to merged lanes
dev;Projection of rate-limit aggregates into
RuntimeMetricsremains a separate P3 observability follow-up after runtime admission merges.Regression coverage
Focused suites cover feature-off compatibility, credential isolation, auth-first invalid-key behavior, loopback trust, auth/Origin precedence, protocol-native 429 envelopes, shared management budgets, policy transitions, bounded overflow, WebSocket reservation/rollback/release/restart isolation, effective default validation, ChefVault forwarding, provider-security redaction, and safe config DTO behavior.
Merge gates
847c2f5;CodeRabbit's additional textual review was unavailable because of quota and is not represented as an approval.
Greptile Summary
This change introduces opt-in admission rate limiting for management, HTTP, and WebSocket traffic, including authenticated principal handling and WebSocket concurrency limits.
Authenticated artifact downloads are not covered: repeated requests to
/v1/opencodex/artifacts/:idcontinue to succeed after an enabled, exhausted low-capacity policy. The artifact route needs to enter the same post-authentication, post-Origin admission flow as the other data-plane routes.Confidence Score: 4/5
Not safe to merge until artifact downloads are covered by admission limiting.
Runtime verification reproduced repeated successful authenticated artifact downloads despite an enabled exhausted rate-limit policy. This leaves an externally reachable data-plane route outside the intended protection.
Files Needing Attention: src/server/index.ts, src/ratelimit/token-bucket.ts, src/config.ts
Security Review
The authenticated artifact-download endpoint bypasses the new admission controls. An authenticated caller can repeatedly retrieve retained artifact bytes without consuming a rate-limit budget, and there is no separate artifact-download policy for operators to configure. This leaves a data-transfer endpoint outside the protection introduced by this change.
What T-Rex did
Comments Outside Diff (2)
src/server/index.ts, line 604-631 (link)This authenticated data-plane route validates the API key and Origin, then returns the artifact directly without creating or committing an admission gate. With rate limiting enabled and the
imagespolicy exhausted after one request, repeated authenticated/v1/opencodex/artifacts/:iddownloads still return200without any rate-limit headers. Add post-authentication admission to this branch—preferably using a dedicated artifact-download surface, or explicitly sharing theimagessurface—and expose that policy in configuration.Artifacts
Focused artifact rate-limit runtime harness source
Artifact download behavior before rate limiting is enabled
Artifact download behavior with an exhausted images rate-limit policy
Prompt To Fix With AI
General comment
imagespolicy set to one request per minute with burst one, three authenticated requests to a freshly materialized/v1/opencodex/artifacts/:idPNG all return 200 and no rate-limit headers. The route is authenticated data-plane traffic and transfers retained artifact bytes, yet it is not admitted or charged. There is also no configurableartifactssurface, so operators cannot limit it independently.src/server/index.ts:604-631performs API authentication and origin validation, then resolves and returns the file directly. Unlike adjacent data-plane branches, it never invokesadmission.gate(surface, req, requestServer)or commits a token.RateLimitSurfaceand rate-limit config omit an artifact surface.images, useadmission.gate("images", req, requestServer)there and document that scope; a dedicated surface is preferable because downloads can be large and have distinct abuse characteristics.Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(ratelimit): enforce origin before co..." | Re-trigger Greptile