Repository navigation
Codex/feature/wework source header - #1424
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesProject-mode request header injection
Sequence Diagram(s)sequenceDiagram
participant ClaudeCodeAgent
participant capabilities
participant LocalModeStrategy
participant api_headers as api_headers module
ClaudeCodeAgent->>capabilities: get_project_id(task_data)
capabilities-->>ClaudeCodeAgent: "42" or empty string
ClaudeCodeAgent->>LocalModeStrategy: use_global_capabilities(True, project_id="42")
ClaudeCodeAgent->>LocalModeStrategy: configure_client_options(...)
LocalModeStrategy->>api_headers: merge_anthropic_custom_headers(process_env + runtime_env)
LocalModeStrategy->>api_headers: extract_default_headers(merged_env)
alt global_capabilities enabled and project_id set
LocalModeStrategy->>api_headers: merge_project_header(default_headers, "42")
api_headers-->>LocalModeStrategy: {wecode-project: "42", ...}
end
LocalModeStrategy->>api_headers: merge_header_map(custom_headers, final_defaults)
LocalModeStrategy->>LocalModeStrategy: serialize and inject DEFAULT_HEADERS_ENV_KEYS
LocalModeStrategy-->>ClaudeCodeAgent: SDK env with merged ANTHROPIC_CUSTOM_HEADERS + DEFAULT_HEADERS
sequenceDiagram
participant CodeXAgent
participant build_codex_config
participant helpers as _resolve and _build helpers
participant api_headers
CodeXAgent->>build_codex_config: model_config, project_id="42"
build_codex_config->>helpers: _resolve_explicit_model_provider(model_config)
helpers-->>build_codex_config: provider resolved or None
build_codex_config->>helpers: _build_header_overrides(default_headers, "42", provider)
helpers->>api_headers: merge_project_header(default_headers, "42")
api_headers-->>helpers: merged headers with wecode-project
helpers-->>build_codex_config: TOML override strings for model_providers.*.http_headers
build_codex_config->>build_codex_config: append to config_overrides
build_codex_config-->>CodeXAgent: CodeXConfig with wecode-project and wecode-executor overrides
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@executor/agents/api_headers.py`:
- Around line 134-140: The _normalize_project_id function in
executor/agents/api_headers.py currently accepts negative IDs and zero, while
the capability resolver in executor/modes/local/capabilities.py rejects numeric
IDs <= 0. Update _normalize_project_id to convert the string value to a numeric
check and return an empty string for any numeric ID that is less than or equal
to zero, ensuring consistency between the two normalization paths and preventing
mismatched wecode-project header injection.
In `@shared/models/openai_converter.py`:
- Around line 390-395: The `metadata.get("standalone_chat_workspace", False)`
call at line 391 does not properly normalize null values to boolean—when the key
exists with a null value in metadata, dict.get() returns None instead of the
default False, which violates the bool type contract expected by
ExecutionRequest. Modify the standalone_chat_workspace parameter assignment to
explicitly normalize the value to a real boolean, ensuring that both missing
keys and null values result in False being passed to the ExecutionRequest
constructor.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97f9f96e-b44c-42b2-a073-e4037c8aa9f4
📒 Files selected for processing (14)
docs/en/developer-guide/local-device-architecture.mddocs/zh/developer-guide/local-device-architecture.mdexecutor/agents/api_headers.pyexecutor/agents/claude_code/claude_code_agent.pyexecutor/agents/claude_code/local_mode_strategy.pyexecutor/agents/codex/codex_agent.pyexecutor/agents/codex/config_builder.pyexecutor/modes/local/capabilities.pyexecutor/tests/agents/test_codex_config_builder.pyexecutor/tests/agents/test_mode_strategy.pyexecutor/tests/agents/test_project_workspace_paths.pyexecutor/tests/modes/local/test_global_capabilities.pyshared/models/openai_converter.pyshared/tests/test_openai_converter.py
| def _normalize_project_id(project_id: Any) -> str: | ||
| if project_id is None: | ||
| return "" | ||
| value = str(project_id).strip() | ||
| if not value or value == "0": | ||
| return "" | ||
| return value |
There was a problem hiding this comment.
Unify project-id normalization semantics with capability resolution.
This helper accepts -1/00 as valid IDs, while the resolver in executor/modes/local/capabilities.py rejects numeric IDs <= 0. That mismatch can produce inconsistent wecode-project header injection when callers pass raw IDs.
Suggested fix
def _normalize_project_id(project_id: Any) -> str:
if project_id is None:
return ""
value = str(project_id).strip()
- if not value or value == "0":
+ if not value:
return ""
+ try:
+ if int(value) <= 0:
+ return ""
+ except ValueError:
+ pass
return value📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _normalize_project_id(project_id: Any) -> str: | |
| if project_id is None: | |
| return "" | |
| value = str(project_id).strip() | |
| if not value or value == "0": | |
| return "" | |
| return value | |
| def _normalize_project_id(project_id: Any) -> str: | |
| if project_id is None: | |
| return "" | |
| value = str(project_id).strip() | |
| if not value: | |
| return "" | |
| try: | |
| if int(value) <= 0: | |
| return "" | |
| except ValueError: | |
| pass | |
| return value |
🤖 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 `@executor/agents/api_headers.py` around lines 134 - 140, The
_normalize_project_id function in executor/agents/api_headers.py currently
accepts negative IDs and zero, while the capability resolver in
executor/modes/local/capabilities.py rejects numeric IDs <= 0. Update
_normalize_project_id to convert the string value to a numeric check and return
an empty string for any numeric ID that is less than or equal to zero, ensuring
consistency between the two normalization paths and preventing mismatched
wecode-project header injection.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executor/agents/api_headers.py (1)
157-163:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid fallback line-parsing after successful JSON parsing.
If input is valid JSON but normalizes to an empty map (for example
{"x": null}), the current fallback at Line 163 re-parses it as header lines and creates malformed keys. This can emit invalid custom headers.Suggested fix
-def _parse_anthropic_custom_headers(headers: str) -> dict[str, str]: - if not headers: - return {} - parsed_json = _parse_header_map(headers) - if parsed_json: - return parsed_json - return _parse_header_lines(headers) +def _parse_anthropic_custom_headers(headers: str | None) -> dict[str, str]: + if not headers: + return {} + stripped = headers.strip() + if not stripped: + return {} + try: + parsed = json.loads(stripped) + except json.JSONDecodeError: + return _parse_header_lines(stripped) + if isinstance(parsed, Mapping): + return _parse_header_map(parsed) + return {}🤖 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 `@executor/agents/api_headers.py` around lines 157 - 163, The _parse_anthropic_custom_headers function cannot distinguish between failed JSON parsing and valid JSON that normalizes to an empty map (like {"x": null}). When valid JSON produces an empty dict, the function incorrectly falls back to _parse_header_lines which creates malformed keys. Instead of checking if parsed_json is truthy, check if it is not None to determine whether JSON parsing actually succeeded, and only fall back to _parse_header_lines when JSON parsing truly fails, not when it returns an empty but valid dict.
🤖 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.
Outside diff comments:
In `@executor/agents/api_headers.py`:
- Around line 157-163: The _parse_anthropic_custom_headers function cannot
distinguish between failed JSON parsing and valid JSON that normalizes to an
empty map (like {"x": null}). When valid JSON produces an empty dict, the
function incorrectly falls back to _parse_header_lines which creates malformed
keys. Instead of checking if parsed_json is truthy, check if it is not None to
determine whether JSON parsing actually succeeded, and only fall back to
_parse_header_lines when JSON parsing truly fails, not when it returns an empty
but valid dict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39970634-0535-4922-9f01-4437c9f874db
📒 Files selected for processing (8)
docs/en/developer-guide/local-device-architecture.mddocs/zh/developer-guide/local-device-architecture.mdexecutor/agents/api_headers.pyexecutor/agents/claude_code/local_mode_strategy.pyexecutor/agents/codex/config_builder.pyexecutor/tests/agents/test_codex_config_builder.pyexecutor/tests/agents/test_mode_strategy.pyexecutor/tests/agents/test_project_workspace_paths.py
✅ Files skipped from review due to trivial changes (1)
- docs/en/developer-guide/local-device-architecture.md
🚧 Files skipped from review as they are similar to previous changes (5)
- executor/tests/agents/test_project_workspace_paths.py
- executor/tests/agents/test_codex_config_builder.py
- executor/tests/agents/test_mode_strategy.py
- executor/agents/claude_code/local_mode_strategy.py
- executor/agents/codex/config_builder.py
…k-source-header # Conflicts: # executor/agents/claude_code/local_mode_strategy.py
Summary by CodeRabbit
0).