fix: address review findings from PR #27 modularization#28
Conversation
- Fix parallel map_sub_query storing errors at outputs[0] instead of correct index - Restore sys.executable -m pytest invocation and default flags in run_tests - Fix _format_payload output="object" mode truncating raw payloads - Fix TS fallback stripper matching = in => arrow tokens as type terminator - Fix TS param stripper dropping default values alongside type annotations - Add PermissionError handling in workspace file walker - Add defensive int cast in binding_status for corrupted metadata 563 tests pass, ruff clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a set of post-modularization regressions from PR #27 by fixing runtime correctness issues (async sub-query orchestration, Node type-stripping), restoring expected tool behavior (test runner invocation, payload formatting contracts), and hardening workspace binding/file handling.
Changes:
- Restore correct behavior for
run_testsinvocation and_format_payload(..., output="object")raw-data contract (and add coverage). - Fix Node worker type-stripping edge cases (
=>handling and default parameter preservation). - Improve robustness/correctness in recipe parallel
map_sub_queryerror indexing and workspace file/binding metadata handling.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/test_mcp_formatting.py |
Adds coverage ensuring output="object" returns the raw payload and that sanitized modes redact/truncate. |
aleph/mcp/formatting.py |
Restores “object mode” to return unsanitized payloads; keeps sanitization for json/markdown. |
aleph/mcp/action_tools.py |
Fixes run_tests to use sys.executable -m and restores default pytest flags. |
aleph/repl/node_worker.cjs |
Fixes parsing/stripping logic to avoid treating => as = terminator and to preserve default param values. |
aleph/mcp/workspace_contexts.py |
Adds defensive handling for PermissionError during file iteration and invalid persisted binding metadata. |
aleph/mcp/recipe_runtime.py |
Fixes parallel map_sub_query to record errors at the correct output index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stat = path.stat() | ||
| stale = ( | ||
| stat.st_size != int(binding.get("size_bytes") or 0) | ||
| or stat.st_mtime_ns != int(binding.get("mtime_ns") or 0) | ||
| stat.st_size != expected_size | ||
| or stat.st_mtime_ns != expected_mtime_ns | ||
| ) |
There was a problem hiding this comment.
binding_status() calls path.stat() without handling OSError/PermissionError. If a bound file exists but becomes unreadable (permissions, transient FS errors), get_status will crash instead of returning a stale/refreshable status. Consider wrapping path.stat() in try/except OSError and returning a status similar to the other error cases (e.g., stale=True, reason indicating stat failure).
Addresses Copilot review comment — if a bound file exists but becomes unreadable, return stale=True with a clear reason instead of crashing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements and bug fixes across the codebase. Key changes include updating the test runner to use sys.executable -m for better virtual environment support, fixing a bug in recipe_runtime.py where error messages were incorrectly indexed, and adding robust error handling for file system operations and metadata parsing in workspace_contexts.py. Additionally, the TypeScript type-stripping logic in node_worker.cjs was refined to correctly handle arrow functions within default parameters, and the payload formatting logic now supports returning raw objects without sanitization. I have no feedback to provide as no review comments were submitted.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
--maxfail=20 silently caps test failures, hiding the true count. Let users pass their own flags via args instead of baking in defaults. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Fixes 7 bugs identified by Devin, Copilot, and Gemini reviewers on PR #27:
recipe_runtime.py— Parallelmap_sub_queryerrors stored atoutputs[0]instead of the failing task's actual index (Devin + Copilot)action_tools.py—run_testslostsys.executable -minvocation and default flags (-vv --tb=short --maxfail=20) during extraction (Devin + Copilot)formatting.py—_format_payload(output="object")was truncating/sanitizing payloads, breaking the raw-data contract (Devin)node_worker.cjs—findTypeTerminatormatched=in=>arrow tokens as type terminator;stripTypeAnnotationsFromParamsstripped default parameter values alongside type annotations (Devin)workspace_contexts.py—_iter_workspace_filescould crash onPermissionError;binding_status()int cast on bad metadata could crash (Gemini + Copilot)Test plan
pytest tests/ -q— 563 passedruff check aleph/ tests/— cleantest_local_server_format_payload_object_returns_rawvalidates raw object mode🤖 Generated with Claude Code