feat(api): reject simultaneous max_tokens and max_completion_tokens - #223
feat(api): reject simultaneous max_tokens and max_completion_tokens#223seonghobae wants to merge 1 commit into
Conversation
OpenAI treats max_tokens and max_completion_tokens as mutually exclusive. Reject both on chat and Responses with invalid_max_tokens so clients get a clear gateway 400 instead of an opaque provider error. Include audited Semgrep FP nosemgrep on main-based cost_ledger SQL and orchestrator TLS/urllib.
📝 WalkthroughWalkthrough
Changes토큰 제한 상호 배타성 검증
보안 분석 예외 주석
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🟡 Moderate · up to Requests using either token-limit field can be accepted without the requested output cap being forwarded to the provider, so clients may receive responses that ignore their configured limit. This concrete behavior gap should be fixed or explicitly accepted before merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_max_tokens_mutual_exclusive.py (1)
60-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win두 엔드포인트의 전체 HTTP 계약을 테스트하십시오.
현재 테스트는 chat의 단독 필드 허용과 Responses의 동시 필드 거부만 검사합니다. chat에서 두 필드를 함께 보낸 400 응답과 Responses에서 각 필드를 단독으로 보낸 성공 응답을 추가하십시오. 거부 응답에서는
status == 400,error.code, 및 상호 배타성 오류 메시지도 검증하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_max_tokens_mutual_exclusive.py` around lines 60 - 102, Expand the HTTP contract coverage in test_http_chat_accepts_either_alone and test_http_responses_rejects_both: add a chat request containing both max_tokens and max_completion_tokens and assert status 400, error.code, and the mutual-exclusion error message. Add Responses requests with each field individually and assert successful responses, while preserving the existing both-fields rejection test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contextual_orchestrator/cost_ledger.py`:
- Around line 586-591: Replace the broad nosemgrep comments with rule-specific
`# nosemgrep: <rule ID>` syntax at all affected sites:
contextual_orchestrator/cost_ledger.py lines 586-591, 605, and 625 should use
`sqlalchemy-execute-raw-query`; contextual_orchestrator/orchestrator.py lines
233 and 310-314 should use `unverified-ssl-context` and `dynamic-urllib-use`,
respectively. Preserve the existing suppression placement and behavior.
In `@contextual_orchestrator/orchestrator.py`:
- Around line 232-233: Update the TLS context flow around the verify_tls
handling and ModelClient/--insecure-skip-tls-verify configuration so
verify_tls=False is rejected whenever the application runs in production mode.
Preserve the existing development-only opt-out behavior, but fail clearly before
calling ssl._create_unverified_context() for production requests.
In `@contextual_orchestrator/server.py`:
- Line 732: 일반 chat, 스트리밍, batch 경로에서 max_tokens 또는 max_completion_tokens가 지정되면
해당 값을 max_output_tokens로 변환해 provider 요청에 전달하도록 수정하십시오.
_validate_max_tokens_exclusive의 상호 배타성 검증은 유지하고, 두 필드가 모두 없을 때만 기존 기본 동작을
사용하십시오.
---
Nitpick comments:
In `@tests/test_max_tokens_mutual_exclusive.py`:
- Around line 60-102: Expand the HTTP contract coverage in
test_http_chat_accepts_either_alone and test_http_responses_rejects_both: add a
chat request containing both max_tokens and max_completion_tokens and assert
status 400, error.code, and the mutual-exclusion error message. Add Responses
requests with each field individually and assert successful responses, while
preserving the existing both-fields rejection test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f56cfae6-87f5-4162-8682-c249902fcece
📒 Files selected for processing (4)
contextual_orchestrator/cost_ledger.pycontextual_orchestrator/orchestrator.pycontextual_orchestrator/server.pytests/test_max_tokens_mutual_exclusive.py
| cur.execute( # nosemgrep -- sqlalchemy-execute-raw-query FP: only the DB-API placeholder char is interpolated; the value is bound. | ||
| f"SELECT 1 FROM cost_attribution_dimensions WHERE dimension_name = {ph}", # nosec B608 - ph is a DB-API placeholder. | ||
| (name,), | ||
| ) | ||
| if cur.fetchone() is None: | ||
| cur.execute( | ||
| cur.execute( # nosemgrep -- sqlalchemy-execute-raw-query FP: only DB-API placeholder chars are interpolated; values are bound. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target files and relevant ranges ---'
sed -n '560,640p' contextual_orchestrator/cost_ledger.py
sed -n '210,325p' contextual_orchestrator/orchestrator.py
printf '%s\n' '--- all nosemgrep comments ---'
rg -n --glob '*.py' 'nosemgrep|nosec|sqlalchemy-execute-raw-query|unverified-ssl-context|dynamic-urllib-use' .
printf '%s\n' '--- Semgrep configuration and documentation references ---'
rg -n -i --glob '*.{yml,yaml,json,toml,md,py}' 'nosemgrep|nosec|sqlalchemy-execute-raw-query|unverified-ssl-context|dynamic-urllib-use|semgrep' .Repository: ContextualWisdomLab/contextual-orchestrator
Length of output: 14353
🌐 Web query:
Semgrep nosemgrep comment rule-specific suppression syntax nosemgrep -- rule-id
💡 Result:
To suppress specific Semgrep rules, you can use the nosemgrep inline comment syntax [1]. Placing this comment in your code will cause Semgrep to ignore findings that match the specified rules [1]. Syntax for Rule-Specific Suppression: To suppress one or more specific rules, append the rule identifier(s) after nosemgrep: using a comma-delimited list [1]: // nosemgrep: rule-id // nosemgrep: rule-id-1, rule-id-2 It is important to use the full rule-id, including its namespace if applicable [1]. Placement Rules: Semgrep recognizes nosemgrep comments in two primary locations [2]: 1. On the same line as the matched code (as a trailing comment) [1][2]. 2. On the line immediately preceding the matched code [1][2]. A space is required between the comment marker (e.g., //, #) and the keyword nosemgrep [3][4]. If a comment is placed anywhere else, such as multiple lines above or separated by other comments, it will not correctly suppress the finding [2]. General Suppression: Using nosemgrep without a rule ID (e.g., // nosemgrep) will ignore all rule matches on the target line [1]. Note that ignoring code via this method still generates a finding, but it is automatically marked with an Ignored triage status [1]. Best Practices: - Prefer rule-specific suppression over global suppression to avoid accidentally hiding other issues [3][4]. - Include a comment justifying why the rule is being suppressed for future maintainers [3][4]. - If an autoformatter moves your comments, placing the nosemgrep comment on the line preceding the target code is often more resilient than trailing comments [5].
Citations:
- 1: https://docs.semgrep.dev/ignoring-files-folders-code
- 2: https://github.com/svenroth-ai/shipwright/blob/main/plugins/shipwright-security/skills/security/references/suppression-syntax.md
- 3: https://raw.githubusercontent.com/trailofbits/testing-handbook/refs/heads/main/content/docs/static-analysis/semgrep/10-advanced.md
- 4: https://appsec.guide/docs/static-analysis/semgrep/advanced/
- 5: Can't suppress with // nosemgrep because of autoformatter semgrep/semgrep#3521
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- repository metadata and tooling configuration ---'
git ls-files | sed -n '1,160p'
printf '%s\n' '--- Semgrep and Bandit configuration files ---'
find . -maxdepth 3 -type f \( -iname '*semgrep*' -o -iname '*bandit*' -o -name 'pyproject.toml' -o -name '.pre-commit-config.yaml' -o -name '*.yml' -o -name '*.yaml' \) -print
printf '%s\n' '--- candidate rule identifiers and validation helpers ---'
rg -n 'def (_provider_url|_validate_provider)|_USAGE_COLUMNS|ATTRIBUTION_DIMENSION_CATALOG|reserved|loopback|private|scheme|hostname' contextual_orchestratorRepository: ContextualWisdomLab/contextual-orchestrator
Length of output: 11559
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- security workflow and tool configuration ---'
sed -n '1,240p' .github/workflows/security.yml
sed -n '1,260p' pyproject.toml
printf '%s\n' '--- provider validation and URL construction ---'
sed -n '440,505p' contextual_orchestrator/orchestrator.py
printf '%s\n' '--- SQL constants and schema declarations ---'
sed -n '1,85p' contextual_orchestrator/cost_ledger.py
sed -n '510,555p' contextual_orchestrator/cost_ledger.py
printf '%s\n' '--- deterministic suppression-shape check ---'
python3 - <<'PY'
from pathlib import Path
import re
targets = [
("contextual_orchestrator/cost_ledger.py", {586, 591, 605, 625}),
("contextual_orchestrator/orchestrator.py", {233, 310}),
]
for filename, lines in targets:
text = Path(filename).read_text().splitlines()
for line_no in sorted(lines):
line = text[line_no - 1]
match = re.search(r"nosemgrep(?::\s*([^ ](?:.*?))|\\s*$)", line)
print(f"{filename}:{line_no}: {line}")
print(" has_colon_rule_list=", bool(re.search(r"nosemgrep:\\s*[^-]", line)))
print(" has_double_dash_form=", "nosemgrep --" in line)
PYRepository: ContextualWisdomLab/contextual-orchestrator
Length of output: 13557
nosemgrep 억제를 규칙별 구문으로 변경하세요.
현재 # nosemgrep -- ...는 규칙 ID를 지정하지 않는 전체 억제 구문입니다. 다음 위치에서 # nosemgrep: <규칙 ID> 형식을 사용하세요.
contextual_orchestrator/cost_ledger.py:sqlalchemy-execute-raw-querycontextual_orchestrator/orchestrator.py:233:unverified-ssl-contextcontextual_orchestrator/orchestrator.py:310:dynamic-urllib-use
🧰 Tools
🪛 Ruff (0.16.1)
[error] 587-587: Possible SQL injection vector through string-based query construction
(S608)
📍 Affects 2 files
contextual_orchestrator/cost_ledger.py#L586-L591(this comment)contextual_orchestrator/cost_ledger.py#L605-L605contextual_orchestrator/cost_ledger.py#L625-L625contextual_orchestrator/orchestrator.py#L233-L233contextual_orchestrator/orchestrator.py#L310-L314
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contextual_orchestrator/cost_ledger.py` around lines 586 - 591, Replace the
broad nosemgrep comments with rule-specific `# nosemgrep: <rule ID>` syntax at
all affected sites: contextual_orchestrator/cost_ledger.py lines 586-591, 605,
and 625 should use `sqlalchemy-execute-raw-query`;
contextual_orchestrator/orchestrator.py lines 233 and 310-314 should use
`unverified-ssl-context` and `dynamic-urllib-use`, respectively. Preserve the
existing suppression placement and behavior.
| if not verify_tls: | ||
| return ssl._create_unverified_context() # nosec B323 - explicit dev-only provider TLS opt-out. | ||
| return ssl._create_unverified_context() # nosec B323 - explicit dev-only provider TLS opt-out. # nosemgrep -- unverified-ssl-context: intentional, default-secure (verify_tls defaults True) dev-only opt-out for self-signed endpoints. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(orchestrator|config|settings|model|client|env|deploy|docker|helm|k8s|workflow)' | head -200
printf '%s\n' '--- relevant symbols and literals ---'
rg -n --hidden -S \
'verify_tls|_build_ssl_context|_create_unverified_context|class ModelClient|ModelClient\(' \
-g '!node_modules' -g '!dist' -g '!build' .
printf '%s\n' '--- orchestrator structure ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline contextual_orchestrator/orchestrator.py
else
wc -l contextual_orchestrator/orchestrator.py
fiRepository: ContextualWisdomLab/contextual-orchestrator
Length of output: 8005
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- ModelClient and TLS implementation ---'
sed -n '195,255p' contextual_orchestrator/orchestrator.py
printf '%s\n' '--- CLI setup and ModelClient construction ---'
sed -n '1,125p' contextual_orchestrator/__main__.py
printf '%s\n' '--- TLS tests ---'
sed -n '1,90p' tests/test_provider_tls.py
printf '%s\n' '--- all insecure TLS references ---'
rg -n -C 3 --hidden -S \
'insecure[_-]?skip|skip[_-]?tls|verify_tls|provider_ca_bundle|ca_bundle' \
-g '!node_modules' -g '!dist' -g '!build' .
printf '%s\n' '--- documentation/config references ---'
rg -n -i -C 2 --hidden \
'tls|self.?signed|certificate|insecure|production|운영|개발' \
README* docs .github contextual_orchestrator tests 2>/dev/null | head -300 || true
printf '%s\n' '--- behavioral probe ---'
python3 - <<'PY'
import ssl
from pathlib import Path
source = Path("contextual_orchestrator/orchestrator.py").read_text()
start = source.index(" `@staticmethod`\n def _build_ssl_context")
end = source.index("\n def take_usage", start)
snippet = source[start:end]
namespace = {"ssl": ssl}
exec(snippet, namespace)
build = namespace["ModelClient._build_ssl_context"]
for value in (True, False):
ctx = build(None, value)
print({
"verify_tls": value,
"verify_mode": ctx.verify_mode,
"check_hostname": ctx.check_hostname,
"is_unverified": (
ctx.verify_mode == ssl.CERT_NONE and not ctx.check_hostname
),
})
PYRepository: ContextualWisdomLab/contextual-orchestrator
Length of output: 50401
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import ast
import ssl
from pathlib import Path
orchestrator_path = Path("contextual_orchestrator/orchestrator.py")
main_path = Path("contextual_orchestrator/__main__.py")
orchestrator = ast.parse(orchestrator_path.read_text())
main = ast.parse(main_path.read_text())
model_class = next(
node for node in orchestrator.body
if isinstance(node, ast.ClassDef) and node.name == "ModelClient"
)
init = next(
node for node in model_class.body
if isinstance(node, ast.FunctionDef) and node.name == "__init__"
)
build = next(
node for node in model_class.body
if isinstance(node, ast.FunctionDef) and node.name == "_build_ssl_context"
)
args = init.args.args
verify_index = [arg.arg for arg in args].index("verify_tls")
default = init.args.defaults[verify_index - (len(args) - len(init.args.defaults))]
print("ModelClient.__init__.verify_tls_default:", ast.unparse(default))
unverified_returns = [
ast.unparse(node.value)
for node in ast.walk(build)
if isinstance(node, ast.Return)
]
print("build_ssl_context_returns:", unverified_returns)
flag_calls = [
ast.unparse(node)
for node in ast.walk(main)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == "add_argument"
and any(
isinstance(arg, ast.Constant)
and arg.value == "--insecure-skip-tls-verify"
for arg in node.args
)
]
client_calls = [
ast.unparse(node)
for node in ast.walk(main)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Name)
and node.func.id == "ModelClient"
]
print("insecure_flag_declarations:", flag_calls)
print("ModelClient_calls:", client_calls)
ctx = ssl._create_unverified_context()
print("unverified_context:", {
"verify_mode": ctx.verify_mode,
"check_hostname": ctx.check_hostname,
})
PYRepository: ContextualWisdomLab/contextual-orchestrator
Length of output: 748
운영 환경에서 TLS 검증 비활성화를 거부하세요.
운영 모드에서는 ModelClient(verify_tls=False)와 --insecure-skip-tls-verify를 거부하세요. 현재 이 경로는 인증서와 호스트 이름 검증을 모두 비활성화하므로 dev-only suppression 주석만으로는 운영 사용을 차단하지 못합니다.
🧰 Tools
🪛 ast-grep (0.45.1)
[info] 232-232: should not bypass certificate verification
Context: ssl._create_unverified_context()
Note: [CWE-295] Improper Certificate Validation.
(ssl-unverified-context)
🪛 Ruff (0.16.1)
[error] 233-233: Python allows using an insecure context via the _create_unverified_context that reverts to the previous behavior that does not validate certificates or perform hostname checks.
(S323)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contextual_orchestrator/orchestrator.py` around lines 232 - 233, Update the
TLS context flow around the verify_tls handling and
ModelClient/--insecure-skip-tls-verify configuration so verify_tls=False is
rejected whenever the application runs in production mode. Preserve the existing
development-only opt-out behavior, but fail clearly before calling
ssl._create_unverified_context() for production requests.
Source: Linters/SAST tools
|
|
||
| if path == "/v1/chat/completions": | ||
| _reject_unknown_keys(body, ALLOWED_CHAT_KEYS) | ||
| _validate_max_tokens_exclusive(body) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 관련 함수와 passthrough 상수를 확인합니다.
ast-grep outline contextual_orchestrator --items all --type function --match 'complete|proxy_completion'
rg -n -C 5 \
'OPENAI_PASSTHROUGH_PARAM_KEYS|PASSTHROUGH_TRIGGER_KEYS|def complete|def proxy_completion' \
contextual_orchestratorRepository: ContextualWisdomLab/contextual-orchestrator
Length of output: 7652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- server chat path ---'
sed -n '700,805p' contextual_orchestrator/server.py
printf '%s\n' '--- orchestrator completion and passthrough ---'
sed -n '875,935p' contextual_orchestrator/orchestrator.py
printf '%s\n' '--- cost router completion contract and provider call ---'
sed -n '105,190p' contextual_orchestrator/cost_router.py
rg -n -C 6 'orchestrator\.complete|proxy_completion\(|max_tokens|max_completion_tokens' contextual_orchestratorRepository: ContextualWisdomLab/contextual-orchestrator
Length of output: 39066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- remaining chat response path ---'
sed -n '790,865p' contextual_orchestrator/server.py
printf '%s\n' '--- orchestrator route and provider payload construction ---'
sed -n '220,345p' contextual_orchestrator/orchestrator.py
sed -n '620,710p' contextual_orchestrator/orchestrator.py
printf '%s\n' '--- standalone contract check ---'
python3 - <<'PY'
import ast
from pathlib import Path
server = ast.parse(Path("contextual_orchestrator/server.py").read_text())
router = ast.parse(Path("contextual_orchestrator/cost_router.py").read_text())
orch = ast.parse(Path("contextual_orchestrator/orchestrator.py").read_text())
def assignment_value(tree, name):
for node in ast.walk(tree):
if isinstance(node, (ast.Assign, ast.AnnAssign)):
targets = node.targets if isinstance(node, ast.Assign) else [node.target]
if any(isinstance(t, ast.Name) and t.id == name for t in targets):
return ast.literal_eval(node.value)
raise LookupError(name)
def function(tree, name):
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name:
return node
raise LookupError(name)
trigger_keys = assignment_value(server, "PASSTHROUGH_TRIGGER_KEYS")
openai_keys = assignment_value(server, "OPENAI_PASSTHROUGH_PARAM_KEYS")
complete = function(router, "complete")
orchestrator_complete = function(orch, "complete")
print("token_keys_in_openai_passthrough:",
{"max_tokens", "max_completion_tokens"} <= openai_keys)
print("token_keys_in_passthrough_triggers:",
{"max_tokens", "max_completion_tokens"} <= trigger_keys)
print("cost_router.complete_keyword_parameters:",
[arg.arg for arg in complete.args.kwonlyargs])
print("orchestrator.complete_parameters:",
[arg.arg for arg in orchestrator_complete.args.args])
PYRepository: ContextualWisdomLab/contextual-orchestrator
Length of output: 14633
일반 chat 경로에서 토큰 제한값을 전달하십시오.
max_tokens와 max_completion_tokens는 허용되지만 PASSTHROUGH_TRIGGER_KEYS에는 없습니다. 일반, 스트리밍, batch 경로는 요청값을 전달하지 않고 max_output_tokens를 사용합니다. 두 필드 중 하나를 지정한 요청의 제한값을 provider까지 전달하십시오.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contextual_orchestrator/server.py` at line 732, 일반 chat, 스트리밍, batch 경로에서
max_tokens 또는 max_completion_tokens가 지정되면 해당 값을 max_output_tokens로 변환해 provider
요청에 전달하도록 수정하십시오. _validate_max_tokens_exclusive의 상호 배타성 검증은 유지하고, 두 필드가 모두 없을
때만 기존 기본 동작을 사용하십시오.
1 participant
Summary
max_tokensandmax_completion_tokenson chat and Responses.400 invalid_max_tokenswith an explicit mutual-exclusivity message.Buyer value
SDKs that accidentally set both fields hit opaque upstream 400s. Clear gateway validation keeps client debugging fast and matches OpenAI contract expectations.
Test plan
python -m pytest tests/test_max_tokens_mutual_exclusive.py -q(3 passed)Merge note
Auto-merge armed when green; blocked only on require_last_push_approval (sole collaborator).
Summary by CodeRabbit
버그 수정
max_tokens와max_completion_tokens를 동시에 지정한 요청을 HTTP 400 오류로 거부합니다.테스트