[jnumainville]

New behavior for waiting:

1. wait=False now uses quick_operation_timeout_seconds instead of a literal 0 for it’s deadline to ensure operations aren’t just instantly failing without a chance to execute
2. wait=True waits for a terminal state such as RUNNING

Use python scripts/pq_wait_test.py --url <server> --username <username> to exercise the new behavior.

[semanticdiff-com]

Review changes with  

Changed Files

File	Status
src/deephaven_mcp/client/_timeouts.py	66% smaller
src/deephaven_mcp/cli/_commands/pq.py	24% smaller
tests/client/test__controller_client.py	12% smaller
docs/CLI.md	Unsupported file format
docs/CONFIGURATION.md	Unsupported file format
pyproject.toml	Unsupported file format
scripts/pq_wait_test.py	0% smaller
src/deephaven_mcp/client/_controller_client.py	Unsupported file format
src/deephaven_mcp/mcp_systems_server/_tools/pq.py	Unsupported file format
tests/cli/_commands/test_pq.py	0% smaller
tests/mcp_systems_server/_tools/test_pq.py	0% smaller

[Copilot]

🟡 Changes recommended

Settlement logic can return prematurely or falsely time out, and the verification script needs cleanup and configuration-path corrections.

4 open findings

• Verify individual deletions before reporting cleanup success · New
• Resolve relative config paths before launching MCP subprocess · New
• Correct settlement handling for DISCONNECTED and KILLED states · New
• Correlate restart completion with the requested attempt · New

What changed in this PR

Separates persistent-query request submission from lifecycle waiting, and extends CLI timeouts to accommodate batch operations.

Changes:

• Adds explicit settlement waits and nonzero submission deadlines.
• Updates CLI timeout budgeting, help, and configuration documentation.
• Revises unit tests and adds a live-server verification script.

File	Description
tests/​mcp_systems_server/​_tools/​test_pq.py	Tests lifecycle dispatch and no-wait behavior.
tests/​client/​test__controller_client.py	Tests submission deadlines and settlement predicates.
tests/​cli/​_commands/​test_pq.py	Tests lifecycle request-timeout calculations.
tests/​cli/​_commands/​test__wrapping.py	Tests per-call timeout overrides.
src/​deephaven_mcp/​mcp_systems_server/​_tools/​pq.py	Routes lifecycle requests to submit or wait methods.
src/​deephaven_mcp/​client/​_timeouts.py	Clarifies submission and settlement timeout roles.
src/​deephaven_mcp/​client/​_controller_client.py	Implements submission and state-based waiting.
src/​deephaven_mcp/​cli/​_commands/​pq.py	Extends lifecycle timeouts and updates help.
src/​deephaven_mcp/​cli/​_commands/​_wrapping.py	Forwards per-call timeout overrides.
scripts/​pq_wait_test.py	Adds live Enterprise lifecycle checks.
docs/​CONFIGURATION.md	Documents lifecycle timeout semantics.
docs/​CLI.md	Explains waiting and readiness outcomes.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Restart waiting can finish on the previous run’s teardown instead of the replacement run’s outcome.

2 open findings

• Exclude STOPPING from restart watcher terminal markers · New
• Correct settlement handling for DISCONNECTED and KILLED states

3 resolved since last review

• Correlate restart completion with the requested attempt
• Resolve relative config paths before launching MCP subprocess
• Verify individual deletions before reporting cleanup success

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Previously reported lifecycle-state classification issues remain unresolved and can cause premature completion or false timeouts.

0 open findings

2 resolved since last review

• Exclude STOPPING from restart watcher terminal markers
• Correct settlement handling for DISCONNECTED and KILLED states

🧠 Review effort: Balanced

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[Copilot]

🔵 Needs a closer look

The restart watcher can mistake an existing startup's progress for the replacement run and return prematurely.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Require replacement evidence before accepting restart outcome

src/​deephaven_mcp/​client/​_controller_client.py:153

If a restart is requested while the existing attempt is in ACQUIRING_WORKER (init=0), that attempt can still progress through INITIALIZING (init=2) to RUNNING (init=2) before the restart takes effect. The changed timestamp makes this watcher accept RUNNING even though _old_run_gone is still False. Consequently, wait=True can return the previous attempt's outcome before the replacement starts. Track the pre-existing startup separately and require evidence of a replacement before accepting its outcome. Add this sequence as a regression test.

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Settlement handling can misreport lifecycle outcomes, and temporary log creation permits symlink-based file truncation.

No open issues 🎉

4 previously missed issues

In code that hasn't changed since last review

Create log files atomically in the private temporary directory

scripts/​pq_wait_test.py:458

The log path is outside the private temporary directory and is predictable from that directory's visible name. Another local user can create a symlink there while the password prompt is open. On systems permitting that symlink traversal, open(..., "w") truncates the target with the invoking user's permissions. Allocate the log atomically with tempfile.mkstemp(), which also creates it with private permissions.

Treat KILLED as a settled start/restart outcome

src/​deephaven_mcp/​client/​_controller_client.py:75

A new attempt ending in KILLED never satisfies _start_settled, even with a different initialization timestamp, so start/restart reports a timeout instead of the settled outcome. client/_protobuf.py:72 already classifies KILLED as terminal. Include it in the ended statuses, add a predicate test, and update the documented start/restart outcome lists.

Treat KILLED as a settled stop outcome

src/​deephaven_mcp/​client/​_controller_client.py:93

KILLED is missing from the stopped statuses, although client/_protobuf.py:72 classifies it as terminal. If a worker ends in that state while stopping, _stop_settled stays false and reports a timeout despite termination. Include KILLED and add a stop-predicate test for that outcome.

Prevent stale runs from satisfying replacement-run settlement

src/​deephaven_mcp/​client/​_controller_client.py:130

A changed initialization timestamp does not always identify a replacement run. If the initial snapshot is ACQUIRING_WORKER with timestamp 0, the previous run can initialize before the restart is processed. A delayed RUNNING or FAILED snapshot from that run then has a nonzero timestamp and satisfies this predicate, ending the wait before the replacement settles. Use a reliable attempt identifier or a synchronized stop/new-attempt boundary, and add a regression case where the previous run initializes during submission.

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The settlement predicates omit terminal KILLED states, causing ended operations to be reported as timeouts.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Include KILLED in start and stop terminal status predicates

src/​deephaven_mcp/​client/​_controller_client.py:91

Both _START_ENDED_STATUSES and _STOPPED_STATUSES omit KILLED, which client/_protobuf.py:72 classifies as terminal. If a new start/restart attempt ends in KILLED, or a stop leaves the PQ in that state, the corresponding predicate never settles and wait=True reports a timeout despite the worker having terminated. Include KILLED in both sets, add regression cases for both predicates, and update the documented outcome lists.

_{Review effort: Balanced}

[bmingles]

Per Claude: the CLI can time out before the server does. The extra 60s doesn't cover a first call that has to authenticate (60–120s) and subscribe (30s). Fix: add auth_timeout_seconds + subscribe_timeout_seconds to the CLI timeout.

[jnumainville]

Done

[Copilot]

🟡 Changes recommended

Unsafe log creation permits symlink attacks that truncate user-writable files, while omitted KILLED statuses cause settled start, restart, and stop operations to wait until timeout.

2 open issues

• Log path permits symlink-based truncation outside the temp directory · New
• KILLED status is missing from start and stop outcomes · New

_{Review effort: Balanced}