feat(execution): add public platform batch - #3
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe structured execution API expands from seven to fourteen closed operations. It adds YouTube search and subtitles, four V2EX operations, and Exa Web search with new contracts, backends, validation, tests, workflow checks, and documentation. ChangesClosed execution expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant registry
participant execute_v2ex
participant V2exTransport
participant execute_exa
participant mcporter
Caller->>registry: submit validated operation
registry->>execute_v2ex: dispatch V2EX request
execute_v2ex->>V2exTransport: fetch and project API data
V2exTransport-->>execute_v2ex: normalized execution result
registry->>execute_exa: dispatch Exa request
execute_exa->>mcporter: validate artifacts and invoke fixed process
mcporter-->>execute_exa: parse and project search results
execute_exa-->>Caller: ExecutionResultV1
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
agent_reach/execution/v1/v2ex.py (2)
319-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the loop variable in
_project_replies.Line 328 rebinds the
valueparameter as the loop variable. The behavior is correct becausevaluesis extracted first, but the shadowing makes the function harder to read and fragile under future edits. Useraw_replyfor the loop variable.♻️ Proposed change
- for value in values: + for raw_reply in values: _checkpoint(context) - raw = _mapping(value) + raw = _mapping(raw_reply)🤖 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 `@agent_reach/execution/v1/v2ex.py` around lines 319 - 355, Rename the `_project_replies` loop variable from `value` to `raw_reply`, and update the immediately following `_mapping` call to use `raw_reply`; leave the function’s behavior and other parameter references unchanged.
83-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuppress the
_CheckpointRaisedwrapper when re-raising the host exception. Both backends wrap a host checkpoint failure in an internal_CheckpointRaisedexception and then re-raise the original inside theexceptblock. Withoutfrom None, Python chains the internal wrapper onto the host's cancellation or deadline exception as__context__, so an implementation detail appears in the host traceback.
agent_reach/execution/v1/v2ex.py#L83-L84: changeraise raised.originaltoraise raised.original from None.agent_reach/execution/v1/exa.py#L114-L115: changeraise raised.originaltoraise raised.original from None.🤖 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 `@agent_reach/execution/v1/v2ex.py` around lines 83 - 84, Suppress the internal _CheckpointRaised context when re-raising the original host exception: update the raise in agent_reach/execution/v1/v2ex.py at lines 83-84 and the corresponding raise in agent_reach/execution/v1/exa.py at lines 114-115 to use explicit context suppression.Source: Linters/SAST tools
tests/test_execution_exa.py (1)
114-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winShare the artifact closure across tests.
The
artifact_fixturefixture is function-scoped. For each test it copies the whole Python interpreter tonode, and everycapability()call hashes that copy through_file_sha256and walks the tree through_mcporter_tree_digest. Several tests callcapability()more than once. With roughly 40 test cases in this module the suite repeats hundreds of megabytes of copying and hashing.Move the immutable parts (the interpreter copy and
pyvenv.cfg) to a session-scoped fixture and keep only the mutablecli.js,package.json, and config files per test. Tests that mutate artifacts, such astest_artifact_drift_fails_before_spawnand_write_cli, must keep writing into per-test copies.Also applies to: 214-217
🤖 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_execution_exa.py` around lines 114 - 152, Refactor artifact_fixture into shared session-scoped setup for the immutable interpreter copy and pyvenv.cfg, while retaining per-test creation of cli.js, package.json, and sterile-config.json. Update the _ArtifactFixture paths and dependent fixtures as needed so each test still receives isolated mutable artifacts; preserve mutation behavior for test_artifact_drift_fails_before_spawn and _write_cli.agent_reach/execution/v1/contracts.py (2)
961-1004: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared public-URL host checks.
_valid_public_result_urlrepeats almost all of_valid_public_location(lines 1014-1055): scheme set, credential rejection, default-port rule, local-host rejection, IP scope check, and DNS label rules. The two differ only in the length limit, the extra whitespace and backslash checks, and the query/fragment rule.Extract one helper that validates the parsed host and port, then let both functions add their own pre-checks. This keeps the two security rules aligned when either is changed.
🤖 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 `@agent_reach/execution/v1/contracts.py` around lines 961 - 1004, Extract the common host and port validation logic from _valid_public_result_url into a new helper function (such as _valid_public_host_and_port) that validates the scheme, credentials, port against expected default, local-host rejection, IP address scope, and DNS label format. Update _valid_public_result_url to apply its unique pre-checks (type, ascii, length, trimming, control characters, whitespace, and backslashes) before calling the new helper with the parsed URL. Reference _valid_public_location which contains similar logic to identify what should move into the shared helper versus what remains function-specific, ensuring the two security rules stay aligned as either evolves.
897-935: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrecompile the reply-URL pattern.
Line 921 builds and compiles a regex for every reply item.
native_idalready matched_POSITIVE_DECIMAL, so the value is safe, but the compile cost repeats per item. Compare the URL against an f-string instead, as_valid_result_schema_sequencealready does at line 890.♻️ Proposed change
if item.schema_id == "v2ex.reply.v1": url = item.fields.get("url") author = item.fields.get("author") + topic_id = url.split("/t/", 1)[1].split("#", 1)[0] if type(url) is str and "/t/" in url else "" return bool( type(url) is str - and re.fullmatch( - rf"https://www[.]v2ex[.]com/t/[1-9][0-9]{{0,31}}`#reply`{re.escape(native_id)}", - url, - ) + and _POSITIVE_DECIMAL.fullmatch(topic_id) is not None + and url == f"https://www.v2ex.com/t/{topic_id}`#reply`{native_id}" and type(author) is str and _V2EX_IDENTIFIER.fullmatch(author) )🤖 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 `@agent_reach/execution/v1/contracts.py` around lines 897 - 935, In the _valid_v2ex_item function's v2ex.reply.v1 schema branch, replace the re.fullmatch call that builds and compiles a regex pattern with every item with a simple f-string equality check. Since native_id has already been validated to match _POSITIVE_DECIMAL, construct the expected URL directly as an f-string using the native_id value and compare it directly against the url field, following the pattern used in the v2ex.topic.v1 branch above it.Source: Linters/SAST tools
agent_reach/execution/v1/registry.py (2)
377-407: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the argument limits from the capability instead of literals.
Lines 379, 393, and 407 hardcode
50. The V2EXbrowse.hotandbrowse.node_topicscapabilities already declaremaximum_items=50, and the Exasearch.webcapability declaresmaximum_items=20. The other branches in this function usecapability.maximum_items. A future change to a capability entry will silently desynchronize these literals.Use
capability.maximum_itemsfor both V2EX branches. For Exa, the accepted request limit intentionally exceeds the item cap, so bind it to a named constant that states that intent.♻️ Proposed change
if key == ("v2ex", "browse.hot") and set(arguments) == {"limit"}: limit = arguments["limit"] - return type(limit) is int and 1 <= limit <= 50 + return type(limit) is int and 1 <= limit <= capability.maximum_items @@ return bool( _valid_v2ex_identifier(node) and type(page) is int and 1 <= page <= 100 and type(limit) is int - and 1 <= limit <= 50 + and 1 <= limit <= capability.maximum_items ) @@ if key == ("exa", "search.web") and set(arguments) == {"query", "limit"}: - return _valid_query_and_limit(arguments, maximum_limit=50) + return _valid_query_and_limit(arguments, maximum_limit=_MAX_EXA_REQUEST_LIMIT)🤖 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 `@agent_reach/execution/v1/registry.py` around lines 377 - 407, The validation logic contains hardcoded limit values (50 for both V2EX branches and for Exa) that should instead derive from the capability definitions already available in the codebase. For the two V2EX validations under the browse.hot and browse.node_topics key checks, replace the hardcoded 50 limit with capability.maximum_items to keep the validation synchronized with the capability definitions. For the Exa search.web validation, replace the hardcoded 50 with a named constant that expresses the intent that the accepted request limit intentionally exceeds the item cap, ensuring future capability changes won't silently cause desynchronization.
357-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the contract patterns for the language and identifier checks.
Lines 364-369 re-implement
_YOUTUBE_LANGUAGEfromagent_reach/execution/v1/contracts.py, and_valid_v2ex_identifierre-implements_V2EX_IDENTIFIER.agent_reach/execution/v1/youtube.pyrevalidates the language with_YOUTUBE_LANGUAGE, andagent_reach/execution/v1/v2ex.pyrevalidates identifiers with its own copy of the same pattern. Three copies of one rule can drift.Import the compiled patterns from
contractsand match against them here._valid_query_and_limitalso uses_MAX_BILIBILI_QUERY_CHARACTERSfor YouTube and Exa queries; rename that constant to a source-neutral name so the shared use is explicit.Also applies to: 411-434
🤖 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 `@agent_reach/execution/v1/registry.py` around lines 357 - 371, Replace the inline validation logic for the YouTube language parameter (lines 364-369 in the first validation block) with a pattern match against the imported _YOUTUBE_LANGUAGE pattern from contracts.py. Apply the same approach to the identifier validation patterns in the second block (also applies to lines 411-434) by importing and matching against the appropriate compiled patterns from contracts instead of re-implementing the validation rules inline. Additionally, rename _MAX_BILIBILI_QUERY_CHARACTERS to a more generic name that reflects its shared use across multiple services (YouTube, Exa, and Bilibili), and update all references to use the renamed constant throughout the registry validation logic.
🤖 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 `@agent_reach/execution/v1/_v2ex_transport.py`:
- Around line 42-47: Promote httpcore==1.0.9 from the dev-only dependency
section into the main pinned optional dependency/lock path so runtime
environments resolve that version. Add an explicit runtime contract test for
pool_factory(...) and url_factory(...) that invokes each with the supported
runtime keyword arguments and verifies the resulting behavior, rather than only
checking symbol availability.
In `@agent_reach/execution/v1/contracts.py`:
- Around line 853-869: Update the subtitle validation and projection flow around
_valid_youtube_subtitle_item and _project_subtitle_success so truncating text
according to ExecutionLimitsV1.maximum_text_characters cannot invalidate the
required WEBVTT prefix. Preserve WEBVTT validation against the original subtitle
text, or enforce a minimum limit that retains the prefix before execution.
In `@agent_reach/execution/v1/exa.py`:
- Around line 170-177: The _validate_artifacts function performs expensive
full-tree validation via _mcporter_tree_digest on every request, which dominates
the process budget. Introduce process-level caching that stores the result of
full tree validation (the call to _mcporter_tree_digest) keyed by the declared
digests (artifacts.node_sha256 and artifacts.mcporter_tree_sha256) plus file
metadata (st_dev, st_ino, st_size, st_mtime_ns) of the node_executable,
mcporter_cli, config_path, and mcporter_root. Check the cache before calling
_mcporter_tree_digest and only perform the expensive walk when the cache key is
not present or has changed, while keeping the existing per-request comparison
logic as a cheap metadata fallback check.
- Around line 687-700: Update _kill_and_reap to guard the
os.killpg/process-group signaling path behind a non-Windows platform check,
avoiding references to unavailable Windows APIs. On Windows, skip directly to
process.kill(), while preserving the existing process-group behavior and
fallback handling on other platforms.
In `@agent_reach/execution/v1/youtube.py`:
- Around line 386-395: The YouTube subtitle flow currently uses the process CWD
as its workspace, allowing cleanup to remove unrelated files. Replace the
_private_workspace_root-based flow with a per-call tempfile.TemporaryDirectory
created under Path.cwd(), route the paths and outtmpl from _fixed_options into
that directory, and remove the entire temporary directory after the call;
eliminate the _cleanup_new_workspace_files snapshot cleanup and preserve
existing workspace validation/error behavior where applicable.
---
Nitpick comments:
In `@agent_reach/execution/v1/contracts.py`:
- Around line 961-1004: Extract the common host and port validation logic from
_valid_public_result_url into a new helper function (such as
_valid_public_host_and_port) that validates the scheme, credentials, port
against expected default, local-host rejection, IP address scope, and DNS label
format. Update _valid_public_result_url to apply its unique pre-checks (type,
ascii, length, trimming, control characters, whitespace, and backslashes) before
calling the new helper with the parsed URL. Reference _valid_public_location
which contains similar logic to identify what should move into the shared helper
versus what remains function-specific, ensuring the two security rules stay
aligned as either evolves.
- Around line 897-935: In the _valid_v2ex_item function's v2ex.reply.v1 schema
branch, replace the re.fullmatch call that builds and compiles a regex pattern
with every item with a simple f-string equality check. Since native_id has
already been validated to match _POSITIVE_DECIMAL, construct the expected URL
directly as an f-string using the native_id value and compare it directly
against the url field, following the pattern used in the v2ex.topic.v1 branch
above it.
In `@agent_reach/execution/v1/registry.py`:
- Around line 377-407: The validation logic contains hardcoded limit values (50
for both V2EX branches and for Exa) that should instead derive from the
capability definitions already available in the codebase. For the two V2EX
validations under the browse.hot and browse.node_topics key checks, replace the
hardcoded 50 limit with capability.maximum_items to keep the validation
synchronized with the capability definitions. For the Exa search.web validation,
replace the hardcoded 50 with a named constant that expresses the intent that
the accepted request limit intentionally exceeds the item cap, ensuring future
capability changes won't silently cause desynchronization.
- Around line 357-371: Replace the inline validation logic for the YouTube
language parameter (lines 364-369 in the first validation block) with a pattern
match against the imported _YOUTUBE_LANGUAGE pattern from contracts.py. Apply
the same approach to the identifier validation patterns in the second block
(also applies to lines 411-434) by importing and matching against the
appropriate compiled patterns from contracts instead of re-implementing the
validation rules inline. Additionally, rename _MAX_BILIBILI_QUERY_CHARACTERS to
a more generic name that reflects its shared use across multiple services
(YouTube, Exa, and Bilibili), and update all references to use the renamed
constant throughout the registry validation logic.
In `@agent_reach/execution/v1/v2ex.py`:
- Around line 319-355: Rename the `_project_replies` loop variable from `value`
to `raw_reply`, and update the immediately following `_mapping` call to use
`raw_reply`; leave the function’s behavior and other parameter references
unchanged.
- Around line 83-84: Suppress the internal _CheckpointRaised context when
re-raising the original host exception: update the raise in
agent_reach/execution/v1/v2ex.py at lines 83-84 and the corresponding raise in
agent_reach/execution/v1/exa.py at lines 114-115 to use explicit context
suppression.
In `@tests/test_execution_exa.py`:
- Around line 114-152: Refactor artifact_fixture into shared session-scoped
setup for the immutable interpreter copy and pyvenv.cfg, while retaining
per-test creation of cli.js, package.json, and sterile-config.json. Update the
_ArtifactFixture paths and dependent fixtures as needed so each test still
receives isolated mutable artifacts; preserve mutation behavior for
test_artifact_drift_fails_before_spawn and _write_cli.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 855aecaf-e8f2-4497-94ad-12479bb7e2d1
📒 Files selected for processing (19)
.github/workflows/pytest.ymlREADME.mdagent_reach/execution/v1/__init__.pyagent_reach/execution/v1/_v2ex_transport.pyagent_reach/execution/v1/contracts.pyagent_reach/execution/v1/exa.pyagent_reach/execution/v1/registry.pyagent_reach/execution/v1/v2ex.pyagent_reach/execution/v1/youtube.pydocs/README_en.mddocs/README_ja.mddocs/README_ko.mddocs/execution-v1.mdpyproject.tomltests/test_execution_bilibili.pytests/test_execution_contract.pytests/test_execution_exa.pytests/test_execution_v2ex.pytests/test_execution_youtube.py
1 participant
Trellis task:
.trellis/tasks/07-31-agent-reach-public-platform-batchAdds seven independently closed
execution.v1descriptors in one reviewed public-read batch:search.videosandread.subtitlesbrowse.hot,browse.node_topics,read.topic, andread.usersearch.webEach operation keeps a static argument/result contract and exact host capabilities. This adds no generic command, argv, backend, endpoint, MCP method, credential, browser, or fallback selector. Exa Web uses the fixed public provider route; query text is provider-visible and may be retained, while local error/provenance paths remain redacted.
Validation:
env -u EXA_API_KEY uv run pytest -q(847 passed)agent_reach/executiongit diff --checkRisk: expands the fork-owned execution surface from 7 to 14 operation-scoped descriptors. Hermes still performs independent integrity, worker, result, authorization, receipt, and audit validation.
Rollback: restore exact integration commit
2a5829cf3b50bc435c647bfae4c050b1837d0235. No protocol, database, grant, receipt, or audit migration is required.This PR must remain unmerged and untagged until the paired Hermes pin and full cross-repository gates pass.
Summary by CodeRabbit