新增 Codex 适配入口，并补充项目级输出路由兼容 - #11
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds Codex launchers, interactive management flows, a project-scoped MCP adapter, backup and binding support, tests, and documentation. It also separates ReverseLabToolsMCP lab discovery from data output roots and updates CTF path and wrapper resolution. ChangesCodex Launcher and MCP Adapter
ReverseLabToolsMCP Data Root Refactor
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Launcher as CODEX.bat / LAUNCHER.bat
participant Entry as codex_entry.ps1
participant Manager as open_reverselab_codex_manager.py
participant Support as open_reverselab_codex_support.py
participant MCP as open_reverselab_codex_mcp.py
User->>Launcher: Start Codex workflow
Launcher->>Entry: Invoke PowerShell entrypoint
Entry->>Manager: Run install, bind, status, or restore command
Manager->>Support: Execute adapter operation
Support-->>Manager: Return JSON status and confirmation data
Manager-->>Entry: Return payload and exit code
Entry-->>User: Display result or request confirmation
MCP->>Support: Resolve project root and invoke project-mode operation
Support-->>MCP: Return routed operation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
scripts/codex/python/open_reverselab_codex_mcp.py (1)
33-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReaching into FastMCP's private
_tool_manager._toolsinstead of the publiclist_tools()API.
upstream.mcp._tool_manager._toolsaccesses an internal, underscore-prefixed attribute of the upstream FastMCP server. The SDK exposes a public accessor for this exact purpose — return self._tools.get(name) def list_tools(self) -> list[Tool]: """List all registered tools.""" returns the list of registeredToolobjects. Relying on the private dict makes this wrapper fragile to internal refactors infastmcp/mcpreleases.🛠️ Suggested fix
def _load_upstream_tools() -> dict[str, Any]: upstream = _load_upstream_module() - return upstream.mcp._tool_manager._tools + return {tool.name: tool for tool in upstream.mcp._tool_manager.list_tools()}🤖 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 `@scripts/codex/python/open_reverselab_codex_mcp.py` around lines 33 - 36, The _load_upstream_tools helper is reaching into FastMCP internals via upstream.mcp._tool_manager._tools, which should be replaced with the public list_tools() API. Update _load_upstream_tools to use the upstream mcp object's supported tool accessor, and adjust any callers that expect a raw dict so they work with the returned Tool objects or convert them explicitly if needed. Keep the change localized around _load_upstream_tools and the code that consumes its result.tests/codex/test_codex_support.py (1)
47-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for the single-line
developer_instructionsedge case.Given the pattern already established here (
test_upsert_and_remove_global_instructions_and_server), it'd be cheap to add a case whereoriginalcontainsdeveloper_instructions = "short text"(single-line, not triple-quoted) to lock in whatever fix is applied for the duplicate-key risk flagged inopen_reverselab_codex_support.py.🤖 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/codex/test_codex_support.py` around lines 47 - 69, Add a regression case in test_upsert_and_remove_global_instructions_and_server for the single-line developer_instructions form, since ensure_global_server_and_instructions/remove_global_instructions can hit a duplicate-key path when the input already has developer_instructions = "short text". Update the test to start from an original config with that exact single-line key, then verify ensure_global_server_and_instructions still inserts the global block and server cleanly and remove_global_server_and_instructions restores the original developer_instructions entry without duplication or corruption.scripts/codex/codex_entry.ps1 (2)
39-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate command-execution logic.
Invoke-ManagerJsonandInvoke-PythonJsonare nearly identical (build args, invoke python, parse JSON with the same fallback). Consider havingInvoke-ManagerJsondelegate toInvoke-PythonJsonwith$Context.ManagerScriptto remove the duplication.🤖 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 `@scripts/codex/codex_entry.ps1` around lines 39 - 108, `Invoke-ManagerJson` and `Invoke-PythonJson` duplicate the same Python invocation and JSON parsing flow; refactor `Invoke-ManagerJson` to call `Invoke-PythonJson` using `$Context.ManagerScript` and `$ManagerArgs`, keeping the shared execution/parse fallback logic in one place. Preserve the existing return shape (`ExitCode`, `Output`, `Payload`) and the Python resolution/error handling in `Resolve-PythonCommand` so both paths behave identically.
55-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMerging stderr into stdout before JSON parsing risks corrupting the payload.
2>&1 | Out-Stringcombines stdout and stderr beforeConvertFrom-Json. If the invoked Python script writes anything to stderr (warnings, tracebacks) alongside its JSON stdout, parsing will fail and fall back toraw_output/parse_error— degrading gracefully but losing the structured status the UI relies on (e.g.,Show-StartupSummaryreading.global_install_ready). Capturing stderr separately would keep JSON parsing reliable.Also applies to: 91-92
🤖 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 `@scripts/codex/codex_entry.ps1` around lines 55 - 56, The JSON parsing path in codex_entry.ps1 is currently mixing stderr into stdout, which can corrupt the payload before ConvertFrom-Json runs. Update the command execution around the $output/$exitCode capture so stdout is preserved for JSON parsing and stderr is captured separately or handled independently, then only feed the clean stdout into the existing parsing/fallback logic. Apply the same fix anywhere else the same pattern appears, including the later duplicated block referenced by the comment, so downstream consumers like Show-StartupSummary can keep reading structured fields reliably.
🤖 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 `@scripts/codex/codex_entry.ps1`:
- Around line 10-22: Resolve-PythonCommand currently returns any available
python or py executable without verifying it meets the required Python 3.10+
version. Update the version-checking logic in Resolve-PythonCommand so it
inspects the selected interpreter’s version before returning Exe/Args, and only
accepts 3.10 or newer; if you keep the current behavior, adjust the later Python
3.10+ error message to match. Use the Resolve-PythonCommand function and the
subsequent version check/error path as the main touchpoints.
In `@scripts/codex/python/open_reverselab_codex_manager.py`:
- Around line 140-150: The restore subcommands currently invoke destructive
helpers without any confirmation gate. Update the argument setup in
open_reverselab_codex_manager.py for backup_restore and upgrade_batch_restore so
they require the same safeguard used by bind_project, unbind_project, and
repair_project, such as a confirmation text or --yes flag, before
command_backup_restore or command_upgrade_batch_restore can run.
In `@scripts/codex/python/open_reverselab_codex_mcp.py`:
- Around line 60-68: The async wrapper in _make_upstream_wrapper is calling
support.invoke_upstream_tool inline even though that helper blocks on
subprocess.run, which can stall the event loop. Update wrapper to offload the
invoke_upstream_tool call via asyncio.to_thread, keeping the existing
ctx/project_root/target_project flow and preserving the project_root_used field
handling on dict results.
In `@scripts/codex/python/open_reverselab_codex_support.py`:
- Around line 360-368: `find_assignment_span` only detects triple-quoted values,
so `upsert_developer_instructions` can miss an existing single-line
`developer_instructions` entry and insert a duplicate key. Update the matching
logic in `find_assignment_span` (and any callers such as
`upsert_developer_instructions`) to recognize both single-line quoted strings
and triple-quoted strings, then replace the existing assignment instead of
inserting a new one when `developer_instructions` is already present.
- Around line 960-1001: The managed .gitignore block in ensure_gitignore_block
and remove_gitignore_block is missing a closing marker, so the regex can
overmatch and remove user-added entries. Add an explicit end/footer marker to
the managed ignore block, then update both functions to replace/remove content
only between the header and footer, similar to the
PROJECT_BLOCK_BEGIN/PROJECT_BLOCK_END handling used elsewhere in
open_reverselab_codex_support.py.
- Around line 761-785: The confirmation flow does not surface the privileged
Codex runtime settings emitted by project_codex_ctf_config_text() and
project_codex_managed_block(), so update confirm_text() to show the
approval_policy and sandbox_mode values before proceeding, or change the flow so
dangerous mode must be explicitly opted into. Use the existing PROJECT_CODEX_*
helpers and the confirm_text() prompt generation to keep the preview consistent
with the runtime configuration.
In `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/config.py`:
- Around line 22-28: Validate OUTPUT_NAMESPACE before it is used in _data_dir as
a path segment, since the current os.environ.get("REVERSELAB_OUTPUT_NAMESPACE",
"").strip() value can contain traversal like ../outside and escape the intended
DATA_ROOT scope. Add a strict check in config.py, using the OUTPUT_NAMESPACE
constant and _data_dir(name), to allow only a safe namespace format (for
example, a single relative segment with no separators or dot segments) and
reject or ignore invalid values before constructing the Path.
In `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/web_ctf.py`:
- Line 249: Sanitize the case name before building the directory path in
web_ctf.py: the current case_dir assignment in the case-creation flow can be
abused with path traversal values. Reuse the existing _safe_case_name(...)
helper, as already done in ctf_save_request, so the name used by the case
directory logic is normalized and confined under CASES_DIR; update the relevant
case directory construction in the same flow to use the sanitized name instead
of the raw input.
---
Nitpick comments:
In `@scripts/codex/codex_entry.ps1`:
- Around line 39-108: `Invoke-ManagerJson` and `Invoke-PythonJson` duplicate the
same Python invocation and JSON parsing flow; refactor `Invoke-ManagerJson` to
call `Invoke-PythonJson` using `$Context.ManagerScript` and `$ManagerArgs`,
keeping the shared execution/parse fallback logic in one place. Preserve the
existing return shape (`ExitCode`, `Output`, `Payload`) and the Python
resolution/error handling in `Resolve-PythonCommand` so both paths behave
identically.
- Around line 55-56: The JSON parsing path in codex_entry.ps1 is currently
mixing stderr into stdout, which can corrupt the payload before ConvertFrom-Json
runs. Update the command execution around the $output/$exitCode capture so
stdout is preserved for JSON parsing and stderr is captured separately or
handled independently, then only feed the clean stdout into the existing
parsing/fallback logic. Apply the same fix anywhere else the same pattern
appears, including the later duplicated block referenced by the comment, so
downstream consumers like Show-StartupSummary can keep reading structured fields
reliably.
In `@scripts/codex/python/open_reverselab_codex_mcp.py`:
- Around line 33-36: The _load_upstream_tools helper is reaching into FastMCP
internals via upstream.mcp._tool_manager._tools, which should be replaced with
the public list_tools() API. Update _load_upstream_tools to use the upstream mcp
object's supported tool accessor, and adjust any callers that expect a raw dict
so they work with the returned Tool objects or convert them explicitly if
needed. Keep the change localized around _load_upstream_tools and the code that
consumes its result.
In `@tests/codex/test_codex_support.py`:
- Around line 47-69: Add a regression case in
test_upsert_and_remove_global_instructions_and_server for the single-line
developer_instructions form, since
ensure_global_server_and_instructions/remove_global_instructions can hit a
duplicate-key path when the input already has developer_instructions = "short
text". Update the test to start from an original config with that exact
single-line key, then verify ensure_global_server_and_instructions still inserts
the global block and server cleanly and remove_global_server_and_instructions
restores the original developer_instructions entry without duplication or
corruption.
🪄 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: 86c96a46-4e05-4154-b1be-f20e0d7bd5ab
📒 Files selected for processing (15)
.gitignoreCODEX.batLAUNCHER.batREADME.mdREADME.zh.mddocs/codex/CODEX_ONLY.mddocs/codex/CODEX_USAGE.zh-CN.mdscripts/codex/codex_entry.ps1scripts/codex/launcher_entry.ps1scripts/codex/python/open_reverselab_codex_manager.pyscripts/codex/python/open_reverselab_codex_mcp.pyscripts/codex/python/open_reverselab_codex_support.pytests/codex/test_codex_support.pytools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/config.pytools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/web_ctf.py
| function Resolve-PythonCommand { | ||
| $python = Get-Command python -ErrorAction SilentlyContinue | ||
| if ($python) { | ||
| return @{ Exe = $python.Source; Args = @() } | ||
| } | ||
|
|
||
| $py = Get-Command py -ErrorAction SilentlyContinue | ||
| if ($py) { | ||
| return @{ Exe = $py.Source; Args = @("-3") } | ||
| } | ||
|
|
||
| return $null | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does PEP 604 union syntax (X | Y) require from __future__ import annotations to run on Python 3.8/3.9?
💡 Result:
Whether PEP 604 union syntax (X | Y) requires from future import annotations to run on Python 3.8/3.9 depends entirely on how the syntax is used. The short answer is: you cannot use PEP 604 syntax as an actual operator in Python 3.8 or 3.9 because those versions lack the necessary runtime implementation [1][2]. However, if the syntax is used strictly within type annotations, you can use from future import annotations to avoid runtime errors [1]. Here is the technical breakdown: 1. Runtime Evaluation: PEP 604 is a feature of Python 3.10 and later [1][2]. If you attempt to use the | operator between types (e.g., int | str) anywhere that the Python interpreter evaluates the expression—such as in isinstance calls, variable assignments, or function bodies—it will raise a TypeError in Python 3.8 and 3.9 [1]. 2. Type Annotations with from future import annotations: If you only use this syntax in type annotations (e.g., def func(x: int | str): pass), you can use from future import annotations [1]. This import enables "postponed evaluation" (PEP 563), which tells Python to treat annotations as strings rather than evaluating them at definition time [3][1]. Because the interpreter does not evaluate the expression, the | operator is never executed, and the code will run without error [1]. 3. Runtime Dependency: Note that even if your code runs, any external tool or library that inspects your annotations at runtime (like Pydantic, FastAPI, or custom reflection tools) will likely fail if it attempts to resolve those annotations, because the underlying Python 3.8/3.9 interpreter does not know how to handle the | operator [1]. In summary, from future import annotations is not a "magic" way to backport PEP 604; it is a way to hide the syntax from the interpreter during evaluation [1]. For any use case involving runtime evaluation or reflection, PEP 604 is not supported on Python 3.8 or 3.9 [1].
Citations:
- 1: https://pydevtools.com/handbook/explanation/what-is-pep-604/
- 2: https://bugs.python.org/issue41428
- 3: https://docs.python.org/3.9/library/__future__.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== codex_entry.ps1 relevant lines =="
nl -ba scripts/codex/codex_entry.ps1 | sed -n '1,180p'
echo
echo "== search for python version checks and annotations =="
rg -n --hidden --glob '!**/.git/**' -e 'from __future__ import annotations|Path \| None|sys\.version_info|Python 3\.10\+' scripts . 2>/dev/null | sed -n '1,220p'Repository: LING71671/open-reverselab
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scripts/codex/codex_entry.ps1 (lines 1-180) =="
sed -n '1,180p' scripts/codex/codex_entry.ps1 | cat -n
echo
echo "== search for python version checks and annotations =="
rg -n --hidden --glob '!**/.git/**' \
-e 'from __future__ import annotations' \
-e 'Path \| None' \
-e 'Python 3\.10\+' \
-e 'sys\.version_info' \
scripts . 2>/dev/null | sed -n '1,240p'Repository: LING71671/open-reverselab
Length of output: 19807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== python version requirements / markers =="
rg -n --hidden --glob '!**/.git/**' \
-e 'python_requires' \
-e 'requires-python' \
-e '3\.10\+' \
-e '>= *3\.10' \
-e '>= *3\.11' \
-e '>= *3\.12' \
-e 'match [A-Za-z_]' \
scripts/codex pyproject.toml setup.cfg setup.py 2>/dev/null | sed -n '1,240p'
echo
echo "== codex python files outline =="
for f in scripts/codex/python/open_reverselab_codex_manager.py scripts/codex/python/open_reverselab_codex_mcp.py scripts/codex/python/open_reverselab_codex_support.py; do
if [ -f "$f" ]; then
echo "-- $f --"
sed -n '1,40p' "$f" | cat -n
fi
doneRepository: LING71671/open-reverselab
Length of output: 1026
Check the Python version before returning a command
scripts/codex/codex_entry.ps1:10 only checks that python/py exists, so it can hand back an older interpreter while the script still says Python 3.10+ 未找到. If 3.10+ is required, reject versions below that here; otherwise relax the error text.
🤖 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 `@scripts/codex/codex_entry.ps1` around lines 10 - 22, Resolve-PythonCommand
currently returns any available python or py executable without verifying it
meets the required Python 3.10+ version. Update the version-checking logic in
Resolve-PythonCommand so it inspects the selected interpreter’s version before
returning Exe/Args, and only accepts 3.10 or newer; if you keep the current
behavior, adjust the later Python 3.10+ error message to match. Use the
Resolve-PythonCommand function and the subsequent version check/error path as
the main touchpoints.
| backup_restore = sub.add_parser("backup-restore", help="Restore a local adapter backup") | ||
| backup_restore.add_argument("backup_id") | ||
| backup_restore.set_defaults(func=command_backup_restore) | ||
|
|
||
| upgrade_batch_list = sub.add_parser("upgrade-batch-list", help="List recorded project upgrade batches") | ||
| upgrade_batch_list.set_defaults(func=command_upgrade_batch_list) | ||
|
|
||
| upgrade_batch_restore = sub.add_parser("upgrade-batch-restore", help="Restore one project from an upgrade batch") | ||
| upgrade_batch_restore.add_argument("batch_id") | ||
| upgrade_batch_restore.add_argument("project_id") | ||
| upgrade_batch_restore.set_defaults(func=command_upgrade_batch_restore) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files | rg '^(scripts/codex/python/open_reverselab_codex_manager\.py|scripts/codex/python/open_reverselab_codex_support\.py|codex_entry\.ps1)$'
printf '\n== manager outline ==\n'
ast-grep outline scripts/codex/python/open_reverselab_codex_manager.py --view expanded || true
printf '\n== support outline ==\n'
ast-grep outline scripts/codex/python/open_reverselab_codex_support.py --view expanded || true
printf '\n== entry ps1 search ==\n'
rg -n "Show-PreviewAndConfirm|RESTORE|backup-restore|upgrade-batch-restore|--confirm-text|confirm" codex_entry.ps1 scripts/codex/python/open_reverselab_codex_manager.py scripts/codex/python/open_reverselab_codex_support.pyRepository: LING71671/open-reverselab
Length of output: 15531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in scripts/codex/python/open_reverselab_codex_manager.py scripts/codex/python/open_reverselab_codex_support.py codex_entry.ps1; do
if [ -f "$f" ]; then
echo "== $f =="
wc -l "$f"
fi
done
printf '\n== manager relevant lines ==\n'
sed -n '120,190p' scripts/codex/python/open_reverselab_codex_manager.py
printf '\n== support relevant lines ==\n'
sed -n '1,220p' scripts/codex/python/open_reverselab_codex_support.py
printf '\n== entry relevant lines ==\n'
sed -n '350,450p' codex_entry.ps1Repository: LING71671/open-reverselab
Length of output: 9422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== manager command implementations ==\n'
sed -n '1,120p' scripts/codex/python/open_reverselab_codex_manager.py
printf '\n== support restore functions ==\n'
sed -n '560,690p' scripts/codex/python/open_reverselab_codex_support.py
printf '\n== support confirmation helpers around project mode ==\n'
sed -n '1288,1410p' scripts/codex/python/open_reverselab_codex_support.pyRepository: LING71671/open-reverselab
Length of output: 14362
Add a confirmation gate to the restore subcommands. backup-restore and upgrade-batch-restore at scripts/codex/python/open_reverselab_codex_manager.py:140-150 call destructive restore helpers directly, with no --confirm-text/--yes guard like bind-project, unbind-project, and repair-project. Add the same safeguard here so direct CLI calls can’t overwrite or delete files by accident.
🤖 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 `@scripts/codex/python/open_reverselab_codex_manager.py` around lines 140 -
150, The restore subcommands currently invoke destructive helpers without any
confirmation gate. Update the argument setup in open_reverselab_codex_manager.py
for backup_restore and upgrade_batch_restore so they require the same safeguard
used by bind_project, unbind_project, and repair_project, such as a confirmation
text or --yes flag, before command_backup_restore or
command_upgrade_batch_restore can run.
| def _make_upstream_wrapper(tool_name: str, tool_obj: Any): | ||
| async def wrapper(*args, ctx: Context, project_root: str = "", **kwargs): | ||
| roots = await _context_roots(ctx) | ||
| target_project = _selected_project_root(project_root, roots) | ||
| result = support.invoke_upstream_tool(tool_name, kwargs, target_project) | ||
| if isinstance(result, dict): | ||
| result.setdefault("project_root_used", str(target_project)) | ||
| return result | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== open_reverselab_codex_mcp.py ==\n'
sed -n '1,140p' scripts/codex/python/open_reverselab_codex_mcp.py | cat -n
printf '\n== open_reverselab_codex_support.py ==\n'
sed -n '1,260p' scripts/codex/python/open_reverselab_codex_support.py | cat -n
printf '\n== Search for invoke_upstream_tool usage and async/thread handling ==\n'
rg -n "invoke_upstream_tool|to_thread|run_in_executor|subprocess\.run|async def wrapper|FastMCP" scripts/codex/python -SRepository: LING71671/open-reverselab
Length of output: 17110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== invoke_upstream_tool and nearby helpers ==\n'
sed -n '1700,1815p' scripts/codex/python/open_reverselab_codex_support.py | cat -n
printf '\n== direct subprocess.run sites in support ==\n'
sed -n '1538,1568p' scripts/codex/python/open_reverselab_codex_support.py | cat -n
sed -n '1778,1805p' scripts/codex/python/open_reverselab_codex_support.py | cat -nRepository: LING71671/open-reverselab
Length of output: 7586
Offload upstream tool execution from the async wrapper scripts/codex/python/open_reverselab_codex_mcp.py:61-67 — wrapper is async, but it calls support.invoke_upstream_tool(...) inline, and that helper runs a blocking subprocess.run(...) with no timeout. Move the call to asyncio.to_thread(...) so one long upstream scan doesn’t stall other MCP requests on the same event loop.
🤖 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 `@scripts/codex/python/open_reverselab_codex_mcp.py` around lines 60 - 68, The
async wrapper in _make_upstream_wrapper is calling support.invoke_upstream_tool
inline even though that helper blocks on subprocess.run, which can stall the
event loop. Update wrapper to offload the invoke_upstream_tool call via
asyncio.to_thread, keeping the existing ctx/project_root/target_project flow and
preserving the project_root_used field handling on dict results.
| def find_assignment_span(text: str, key: str) -> tuple[int, int, str] | None: | ||
| pattern = re.compile( | ||
| rf'(?ms)^(?P<prefix>\s*{re.escape(key)}\s*=\s*)(?P<quote>"""|\'\'\')(?P<body>.*?)(?P=quote)\s*$' | ||
| ) | ||
| match = pattern.search(text) | ||
| if not match: | ||
| return None | ||
| return match.start(), match.end(), match.group("body") | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
developer_instructions upsert only recognizes triple-quoted strings — risk of duplicate-key TOML corruption.
find_assignment_span matches only """..."""/'''...''' values. If a user's existing ~/.codex/config.toml already has developer_instructions = "short text" (a common single-line form), find_assignment_span returns None, and upsert_developer_instructions falls through to inserting a brand-new developer_instructions = """...""" assignment before the first [ header — producing two developer_instructions keys in the same document. TOML forbids duplicate keys, so this can render the user's global Codex config unparsable after running install/repair.
🛠️ Suggested direction
def find_assignment_span(text: str, key: str) -> tuple[int, int, str] | None:
pattern = re.compile(
rf'(?ms)^(?P<prefix>\s*{re.escape(key)}\s*=\s*)(?P<quote>"""|\'\'\')(?P<body>.*?)(?P=quote)\s*$'
)
match = pattern.search(text)
if not match:
- return None
+ # Fall back to detecting single-line quoted assignments so we don't
+ # blindly insert a second `developer_instructions = ...` key.
+ single_line = re.compile(
+ rf'(?m)^\s*{re.escape(key)}\s*=\s*(?:"(?:[^"\\]|\\.)*"|\'(?:[^\'\\]|\\.)*\')\s*$'
+ )
+ single_match = single_line.search(text)
+ if single_match:
+ raise ValueError(
+ f"{key} exists as a single-line string; refusing to auto-migrate to a managed block"
+ )
+ return None
return match.start(), match.end(), match.group("body")Longer term, this file hand-rolls TOML editing with regexes across ~10 functions (global block, gitignore block, project codex block). Consider adopting a round-trip TOML library (e.g. tomlkit) to eliminate this whole class of quoting/escaping edge cases.
Also applies to: 405-425
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 360-362: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.compile(
rf'(?ms)^(?P\s*{re.escape(key)}\s*=\s*)(?P"""|''')(?P.?)(?P=quote)\s$'
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🤖 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 `@scripts/codex/python/open_reverselab_codex_support.py` around lines 360 -
368, `find_assignment_span` only detects triple-quoted values, so
`upsert_developer_instructions` can miss an existing single-line
`developer_instructions` entry and insert a duplicate key. Update the matching
logic in `find_assignment_span` (and any callers such as
`upsert_developer_instructions`) to recognize both single-line quoted strings
and triple-quoted strings, then replace the existing assignment instead of
inserting a new one when `developer_instructions` is already present.
| def project_codex_ctf_config_text() -> str: | ||
| return "\n".join( | ||
| [ | ||
| "# open-reverselab Codex CTF runtime template", | ||
| f'model_instructions_file = {toml_basic_string(PROJECT_CODEX_PROMPT)}', | ||
| 'approval_policy = "never"', | ||
| 'sandbox_mode = "danger-full-access"', | ||
| "", | ||
| ] | ||
| ) | ||
|
|
||
|
|
||
| def project_codex_managed_block() -> str: | ||
| return "\n".join( | ||
| [ | ||
| PROJECT_CODEX_BLOCK_BEGIN, | ||
| "# Managed by open-reverselab CODEX.bat / LAUNCHER.bat", | ||
| "# Source reference: .codex/open-reverselab.ctf.config.toml", | ||
| f'model_instructions_file = {toml_basic_string(PROJECT_CODEX_PROMPT)}', | ||
| 'approval_policy = "never"', | ||
| 'sandbox_mode = "danger-full-access"', | ||
| PROJECT_CODEX_BLOCK_END, | ||
| ] | ||
| ).strip() | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '700,820p' scripts/codex/python/open_reverselab_codex_support.py
rg -n "enable|repair|danger-full-access|approval_policy|sandbox_mode|confirm" scripts/codex/python/open_reverselab_codex_support.pyRepository: LING71671/open-reverselab
Length of output: 7643
🏁 Script executed:
sed -n '840,915p' scripts/codex/python/open_reverselab_codex_support.py
sed -n '1110,1410p' scripts/codex/python/open_reverselab_codex_support.pyRepository: LING71671/open-reverselab
Length of output: 15527
Surface the runtime privilege change in the confirmation flow
project_codex_ctf_config_text() and project_codex_managed_block() always write approval_policy = "never" and sandbox_mode = "danger-full-access" for enable/repair, but confirm_text() only asks for <project> open-reverselab Codex 模式. Expose those flags in the preview/confirmation, or make the dangerous mode opt-in.
🤖 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 `@scripts/codex/python/open_reverselab_codex_support.py` around lines 761 -
785, The confirmation flow does not surface the privileged Codex runtime
settings emitted by project_codex_ctf_config_text() and
project_codex_managed_block(), so update confirm_text() to show the
approval_policy and sandbox_mode values before proceeding, or change the flow so
dangerous mode must be explicitly opted into. Use the existing PROJECT_CODEX_*
helpers and the confirm_text() prompt generation to keep the preview consistent
with the runtime configuration.
| def ensure_gitignore_block(project_root: Path) -> dict[str, Any]: | ||
| if not (project_root / ".git").exists(): | ||
| return {"updated": False, "reason": "not-a-git-project", "path": str(project_root / ".gitignore")} | ||
|
|
||
| gitignore = project_root / ".gitignore" | ||
| existing = read_text(gitignore) | ||
| block = MANAGED_IGNORE_HEADER + "\n" + "\n".join(MANAGED_IGNORE_ENTRIES) | ||
| if MANAGED_IGNORE_HEADER in existing: | ||
| updated = re.sub( | ||
| rf"{re.escape(MANAGED_IGNORE_HEADER)}.*?(?=\n\n|\Z)", | ||
| block, | ||
| existing, | ||
| flags=re.S, | ||
| ).rstrip() + "\n" | ||
| else: | ||
| updated = existing.rstrip() | ||
| if updated: | ||
| updated += "\n\n" | ||
| updated += block + "\n" | ||
| write_text(gitignore, updated) | ||
| return {"updated": True, "path": str(gitignore)} | ||
|
|
||
|
|
||
| def remove_gitignore_block(project_root: Path) -> dict[str, Any]: | ||
| gitignore = project_root / ".gitignore" | ||
| if not gitignore.exists(): | ||
| return {"updated": False, "path": str(gitignore)} | ||
| existing = read_text(gitignore) | ||
| if MANAGED_IGNORE_HEADER not in existing: | ||
| return {"updated": False, "path": str(gitignore)} | ||
| updated = re.sub( | ||
| rf"\n?{re.escape(MANAGED_IGNORE_HEADER)}.*?(?=\n\n|\Z)", | ||
| "", | ||
| existing, | ||
| flags=re.S, | ||
| ).strip() | ||
| if updated: | ||
| write_text(gitignore, updated + "\n") | ||
| else: | ||
| gitignore.unlink() | ||
| return {"updated": True, "path": str(gitignore)} | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Gitignore managed block has no closing marker — repeated ensure_gitignore_block runs can delete unrelated user entries.
Unlike the AGENTS.md/.codex/config.toml managed blocks (which use explicit BEGIN/END markers), the ignore block only has MANAGED_IGNORE_HEADER with no footer. The update regex HEADER.*?(?=\n\n|\Z) relies on finding a blank line or end-of-file to bound the block. If a user (or another tool) appends lines directly below the managed entries without a blank-line separator — a very natural thing to do in a .gitignore — the next repair/upgrade call (apply_project_binding → ensure_gitignore_block) will silently consume and delete that trailing content along with the managed block.
🛠️ Suggested direction
MANAGED_IGNORE_HEADER = "# open-reverselab Codex managed ignore entries"
+MANAGED_IGNORE_FOOTER = "# open-reverselab Codex managed ignore entries end"and bound both ensure_gitignore_block/remove_gitignore_block regexes between header and footer, mirroring the PROJECT_BLOCK_BEGIN/PROJECT_BLOCK_END pattern used elsewhere in this file.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 967-972: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.sub(
rf"{re.escape(MANAGED_IGNORE_HEADER)}.*?(?=\n\n|\Z)",
block,
existing,
flags=re.S,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
[warning] 989-994: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.sub(
rf"\n?{re.escape(MANAGED_IGNORE_HEADER)}.*?(?=\n\n|\Z)",
"",
existing,
flags=re.S,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🤖 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 `@scripts/codex/python/open_reverselab_codex_support.py` around lines 960 -
1001, The managed .gitignore block in ensure_gitignore_block and
remove_gitignore_block is missing a closing marker, so the regex can overmatch
and remove user-added entries. Add an explicit end/footer marker to the managed
ignore block, then update both functions to replace/remove content only between
the header and footer, similar to the PROJECT_BLOCK_BEGIN/PROJECT_BLOCK_END
handling used elsewhere in open_reverselab_codex_support.py.
| OUTPUT_NAMESPACE = os.environ.get("REVERSELAB_OUTPUT_NAMESPACE", "").strip() | ||
|
|
||
|
|
||
| def _data_dir(name: str) -> Path: | ||
| if OUTPUT_NAMESPACE: | ||
| return DATA_ROOT / name / OUTPUT_NAMESPACE | ||
| return DATA_ROOT / name |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate REVERSELAB_OUTPUT_NAMESPACE before using it as a path segment.
A namespace like ../outside makes _data_dir("exports") resolve outside the intended DATA_ROOT / exports tree, which breaks the project-scoped routing boundary.
Proposed fix
OUTPUT_NAMESPACE = os.environ.get("REVERSELAB_OUTPUT_NAMESPACE", "").strip()
+if OUTPUT_NAMESPACE:
+ namespace_path = Path(OUTPUT_NAMESPACE)
+ if (
+ namespace_path.is_absolute()
+ or namespace_path.name != OUTPUT_NAMESPACE
+ or OUTPUT_NAMESPACE in {".", ".."}
+ ):
+ raise ValueError("REVERSELAB_OUTPUT_NAMESPACE must be a single safe path segment")
def _data_dir(name: str) -> Path:📝 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.
| OUTPUT_NAMESPACE = os.environ.get("REVERSELAB_OUTPUT_NAMESPACE", "").strip() | |
| def _data_dir(name: str) -> Path: | |
| if OUTPUT_NAMESPACE: | |
| return DATA_ROOT / name / OUTPUT_NAMESPACE | |
| return DATA_ROOT / name | |
| OUTPUT_NAMESPACE = os.environ.get("REVERSELAB_OUTPUT_NAMESPACE", "").strip() | |
| if OUTPUT_NAMESPACE: | |
| namespace_path = Path(OUTPUT_NAMESPACE) | |
| if ( | |
| namespace_path.is_absolute() | |
| or namespace_path.name != OUTPUT_NAMESPACE | |
| or OUTPUT_NAMESPACE in {".", ".."} | |
| ): | |
| raise ValueError("REVERSELAB_OUTPUT_NAMESPACE must be a single safe path segment") | |
| def _data_dir(name: str) -> Path: | |
| if OUTPUT_NAMESPACE: | |
| return DATA_ROOT / name / OUTPUT_NAMESPACE | |
| return DATA_ROOT / name |
🤖 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 `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/config.py` around lines 22
- 28, Validate OUTPUT_NAMESPACE before it is used in _data_dir as a path
segment, since the current os.environ.get("REVERSELAB_OUTPUT_NAMESPACE",
"").strip() value can contain traversal like ../outside and escape the intended
DATA_ROOT scope. Add a strict check in config.py, using the OUTPUT_NAMESPACE
constant and _data_dir(name), to allow only a safe namespace format (for
example, a single relative segment with no separators or dot segments) and
reject or ignore invalid values before constructing the Path.
| def ctf_new_challenge(name: str, url: str = "") -> dict: | ||
| """Create a new CTF challenge case directory.""" | ||
| case_dir = REVERSE_ROOT / "cases" / name | ||
| case_dir = CASES_DIR / name |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sanitize name before creating the case directory.
case_dir = CASES_DIR / name lets an MCP caller pass values like ../other-case and create/write outside CASES_DIR. Reuse _safe_case_name(...) here like ctf_save_request already does for request paths.
Proposed fix
def ctf_new_challenge(name: str, url: str = "") -> dict:
"""Create a new CTF challenge case directory."""
- case_dir = CASES_DIR / name
+ safe_name = _safe_case_name(name)
+ case_dir = (CASES_DIR / safe_name).resolve()
+ ensure_under(case_dir, [CASES_DIR], "case directory")📝 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.
| case_dir = CASES_DIR / name | |
| def ctf_new_challenge(name: str, url: str = "") -> dict: | |
| """Create a new CTF challenge case directory.""" | |
| safe_name = _safe_case_name(name) | |
| case_dir = (CASES_DIR / safe_name).resolve() | |
| ensure_under(case_dir, [CASES_DIR], "case directory") |
🤖 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 `@tools/skills/mcp/ReverseLabToolsMCP/reverselab_mcp/tools/web_ctf.py` at line
249, Sanitize the case name before building the directory path in web_ctf.py:
the current case_dir assignment in the case-creation flow can be abused with
path traversal values. Reuse the existing _safe_case_name(...) helper, as
already done in ctf_save_request, so the name used by the case directory logic
is normalized and confined under CASES_DIR; update the relevant case directory
construction in the same flow to use the sanitized name instead of the raw
input.
1 participant
变更概述
这个 PR 为
open-reverselab增加了一套可选的 Codex 专属适配层，但不替换原有的START_HERE工作流。本次改动包括：
CODEX.bat，作为 Codex 用户的首选入口LAUNCHER.bat，作为可选总入口scripts/codex/适配层，包括open_reverselab_codexMCP 桥接服务docs/codex/文档tests/codex/test_codex_support.pyopen_reverselab_codex_support.py中上游工具调用可能卡住超时的问题ReverseLabToolsMCP增加项目级输出目录路由兼容MCP 桥接说明
这次新增的
open_reverselab_codex不是单独复制一套 MCP 工具，而是在 Codex 侧增加一层桥接：codex_enable_project_modecodex_disable_project_modecodex_project_mode_statuscodex_repair_project_mode同时，这层桥接会自动包装上游
ReverseLabToolsMCP的现有工具，并补充可选project_root参数，用于把产物路由到当前项目，而不是默认写回open-reverselab仓库。项目级输出路由主要通过以下环境变量完成：
REVERSELAB_LAB_ROOTREVERSELAB_DATA_ROOTREVERSELAB_OUTPUT_NAMESPACE这样可以让上游工具继续复用原有实现，同时把
notes / reports / exports / patches / projects / samples / cases等产物写入当前项目的 namespaced 目录。当前范围与行为
CODEX.bat是 Codex 用户的首选入口LAUNCHER.bat是可选总入口LAUNCHER.bat的Codex菜单会进入新的 Codex 适配层Claude和其他 CLI菜单目前仍然只是回到原版START_HERE流程（未测试使用）START_HERE工作流README只补充最小入口说明，详细使用方法放在docs/codex/验证
已在干净
worktree中完成以下检查：python scripts/misc/public_release_check.py --stagedpython -m pytest tests/codex/test_codex_support.py说明
START_HERE透传方式Summary by CodeRabbit