Repository navigation
Conversation
Synchronize model-aware HOPE MCP logic and integration tests from HOPE.jl 8126f0f. Register reconnect-safe holistic status and optional solver handling while retaining PowerMCP ownership of its MCP 2 transport and packaging.
qian-harvard
left a comment
There was a problem hiding this comment.
Thanks, the holistic workflow is a good addition, and the #130 hardening survives the sync. In a harness run:
- every Julia call still passes values through
ARGS; - injection payloads in
solverare rejected; - invalid
request_idvalues (../x,a/b,.., newlines, over 128 chars) are rejected; - eight concurrent launches with the same
request_idproduce exactly one job.
HOPE/tests + tests/ pass (475 passed), as does the PR's own unittest run.
One thing blocks the merge:
1. The sync removes PowerMCP's config hook. Main's core.py has _hope_setting() (from f994a15), which falls back to powermcp config for hope.repo_root, hope.julia_bin and hope.julia_depot_path. These settings are documented in README.md, powermcp/README.md and powermcp/registry.py. The synced core.py drops the hook, so those settings are silently ignored:
repo_rootfalls back toparents[4], which in an installed wheel issite-packages/powermcp;- Julia comes from
PATH; - no depot is set.
powermcp doctor and the setup wizard still report HOPE as ready. Since the upstream sync treats core.py as a byte-for-byte copy, the next sync would drop it again. Could you either add the hook upstream, or move it into a file PowerMCP owns (e.g. server.py)? In the second case, set HOPE_REPO_ROOT / HOPE_JULIA_BIN / JULIA_DEPOT_PATH from powermcp.config only when they're unset.
Worth fixing in this PR or a follow-up:
- A run can stay "running" forever. The run manifest records no PID, and
hope_holistic_statusnever checks whether the process is alive. If the Julia child dies without writing the manifest (server killed, OOM), the status staysrunning. Re-launching with the samerequest_idreturnsreused=True, status=running, so that id can't be recovered. A crash between creating the workspace and launching leaves the run stuck at CREATED/VALIDATED the same way. Recording the PID, and treating a dead PID as failed, would fix both. - Status responses change after a reconnect. Before a reconnect a finished run reports
status="done"and a failed oneok=False, error_type=job_failed. After a reconnect the same runs report"published"andok=True, status="failed". - No check for the HOPE.jl version.
core.pycallstools/repo_utils/run_holistic_agent_job.jl, which only exists in recent HOPE.jl (around 8126f0f). With an older checkout, the whole workspace is copied before Julia fails. Please check that the script exists up front, and state the minimum HOPE.jl in the README. - Two paths raise instead of returning a structured error: invalid YAML in a case's settings (
ParserError), and a corrupt manifest (ValueErrorfromhope_job_status). - Small items:
ModelCases/XandXhash differently under the samerequest_id, which returnsrequest_id_conflict.- Every launch reads and hashes every input file before checking for an existing manifest.
artifacts/agent_runsgrows without any cleanup.- There are no unit tests for the
hope_run_holistic/hope_holistic_statusstate machine (the integration tests need Julia and are opt-in). A fake-Julia test like the harness above would cover the idempotency and status paths in CI.
Restore PowerMCP path configuration through a downstream-owned adapter. Sync process-aware persistent status handling, compatibility and parsing preflights, canonical case IDs, and fast state-machine regression tests from HOPE.jl 6c69565.
|
Thanks for the detailed review. I addressed the blocking config regression and the agent-state issues in commit 15cc275.
Validation: 46 HOPE connector tests pass locally (2 opt-in real-solver tests skipped), the Windows PID probe succeeds, the connector wheel includes the adapter, and the upstream HOPE MCP suite passes 68 tests (2 opt-in skips). I kept full content hashing because request IDs currently promise source-fingerprint conflict detection. Workspaces remain intentionally retained as audit records; the README now states that operators should apply their own archival/removal policy rather than PowerMCP deleting model evidence automatically. Please take another look when convenient. |
Summary
Validation