Skip to content

feat(dsh): add structured iteration form and SSH execution - #361

Draft
zengchuang-hw wants to merge 6 commits into
InferMatrixLab:mainfrom
zengchuang-hw:fix/152-dsh-input-remote
Draft

zengchuang-hw wants to merge 6 commits into
InferMatrixLab:mainfrom
zengchuang-hw:fix/152-dsh-input-remote

Conversation

@zengchuang-hw

@zengchuang-hw zengchuang-hw commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Governing Issue and delivery slice

Refs #152

The requester delegated the DSH form, remote-target configuration, existing-dashboard integration, and Draft PR delivery in the current development request. This is a bounded manual-install DSH preview within the proposed integration RFC; acceptance of the full Issue remains with its human owner.

Users can open InferRSI beside the DSH message composer, choose GPU/NPU and local/remote execution, fill in the goal and iteration budgets, and start an iteration in the current Agent conversation. Remote execution first requires a successful SSH connection test.

Solution

The implementation and usage guide is docs/dsh-plugin.md, including the architecture, installation, connection lifecycle, and dashboard workflow.

  • Native DSH bundle with the canonical InferRSI skill, Python runtime, and knowledge resources.
  • Structured form for framework, model, repository, objective, SIAS plane, maximum rounds, time budget, and optional acceptance/benchmark inputs. One submission produces one formatted prompt in the selected conversation.
  • Remote configuration supports IP, port, username, password or uploaded SSH key, optional passphrase, and host-key fingerprint confirmation. Connection testing checks authentication, Linux execution, and repository access.
  • Owner-only connection endpoint and session-scoped tool receipts keep authentication material and server identifiers outside prompts and experiment records. Repository operations enforce the selected root and designated scripts.
  • The existing dashboard provides recorded rounds, history, progress, and evidence. Remote dashboard access uses a loopback SSH tunnel; no separate monitor is introduced.
  • A vendored pure-JavaScript SSH transport avoids native installation scripts. Source versions, regeneration instructions, and complete third-party notices are included; the generated file is marked as vendored.

Knowledge authorship declaration

Not applicable: no paths protected by .github/knowledge-model-policy.json are changed. The commit retains DCO sign-off and material agent attribution.

Changed artifacts and evidence

Requirement Actual change Check / evidence and result Remaining gap
Installable native DSH integration plugins/inferrsi-dsh/, tools/build_dsh_plugin.py Native DSH 0.2.1-rc.5 Windows Web installation and skill/form discovery checked Other host/OS profiles
Formatted input and selected-session submission client.cjs, form.cjs Form validation, prompt contents, scoped send checks; actual browser double-click produced one accepted Agent prompt Model-backed optimization run
GPU/NPU and local/remote configuration Native form and remote bridge Browser field selection, pre-test start blocking, receipt invalidation, credential clearing Actual accelerator availability
SSH key/password and test connection remote-host.cjs, remote.cjs Real SSH protocol fixture exercises both auth methods, fingerprint trust/mismatch, session ownership, path containment and lifecycle cleanup; both methods checked through native DSH Web Real Linux target and remote Python bootstrap execution
Existing dashboard and history Bundled runtime, remote tunnel, skill instructions Isolated runtime/dashboard smoke; SSH tunnel and concurrent benchmark/dashboard cancellation regression checks Live model campaign and three-host acceptance
Reviewable solution and lifecycle docs/dsh-plugin.md Installation, form, SSH trust, process cleanup, architecture and limitations documented Maintainer review

Initial implementation checks and dependencies

  • python -m pytest tests/test_dsh_plugin.py tests/test_codex_plugin.py tests/test_dashboard_live.py -k 'not release and not comparison': 27 passed, 19 deselected.
  • Ruff over src, tests, and the two plugin builders: passed.
  • Node form, bridge, and SSH transport regression checks: passed.
  • git diff --check: passed.
  • Manual native DSH 0.2.1-rc.5 Web checks: install, open from blank conversation, GPU/NPU and local/remote form, password/key connection tests, host fingerprint confirmation, prompt privacy, single submission, and narrow layout.
  • Final native package built from the committed source. Test services have been stopped.
  • The full repository suite was not completed on Windows: an earlier broad run encountered existing overlong parametrized-test/environment-variable handling. The selected plugin/dashboard checks above completed.
  • SSH validation uses a synthetic local SSH/SFTP server. It exercises the actual transport but synthesizes remote CLI/dashboard responses; the transported Python bootstrap is syntax-checked, not executed on a real Linux target.
  • DSH target range: >=0.2.1-rc.2 <0.2.2. Local runtime needs Python 3.10+. Remote Linux Python, Git, engine, accelerator runtime, model assets, and benchmark scripts must be prepared separately.
  • Formal maintainer review remains pending. Current revision verification is recorded below. No performance or campaign verdict is claimed.

Review-feedback fixes

Commit 1792257c addresses Amy's six review points, including the corrected references in her follow-up.

  • Remote scripts require an explicit remaining maxMinutes budget; long CLI calls accept the same hard cap. Command timeout, cancellation or error closes only its execution channel, retaining SSH/SFTP and a separate live dashboard; command ownership remains busy until the channel actually closes.
  • A successful connection test transfers connection lifetime to the session/plugin instead of retaining the HTTP request's abort listener. Pending-test cancellation and the result-boundary race still clean up.
  • Shared skill, form prompt and DSH guide distinguish local execution from the tested remote tool path, including dashboard startup/shutdown. Tool and guide explicitly describe execution with the SSH user's permissions, rather than an execution sandbox.
  • DSH rc.5 owner-route admission and its common Agent/session identity were checked against official pinned source and an actual Host route dispatch probe. The request's session ID selects the owner's target; tool calls use exec.agent.id, with cross-session receipts rejected.
  • Final-tree Windows verification (CPython 3.12.1, Node 24.13.1, CI=true): 14 selected DSH/Codex plugin tests passed, 19 deselected; real SSH-protocol fixture, bridge checks, Ruff and whitespace checks passed. A negative probe confirms missing Node fails in CI. A separate Codex code cross-check passed tree 7056876e661704cfec26aa5b40c453a8cff3b86f; this is implementation verification, not a formal independent approval.
  • The original native Web UI receipt applies to 1379a20b. It was not rerun after these fixes. The SSH fixture still simulates remote command responses; actual Linux bootstrap/benchmark execution, full native UI journeys and the full local repository suite remain unverified in this follow-up.

Submission-lifetime follow-up

Commit 010c9bcb addresses the send/abandon finding, also raised in Amy's re-review.

The form distinguishes a local preflight rejection from pending, confirmed and ambiguous delivery. Abandoning a definitely rejected submission releases its unused SSH receipt, including a rejection received after unmount. Success or an uncertain post-send result preserves the connection; edits, retesting and duplicate sends cannot implicitly replace it. A clearly labelled disconnect action may stop the remote task and must succeed before the form resets.

Final-tree verification (Windows / CPython 3.12.1 / Node 24.13.1, CI=true): both DSH package/SSH tests passed, including actual bundled-component handlers for rejected/pending/unmounted/success/ambiguous outcomes, stale handlers and failed/successful disconnects. Ruff and whitespace checks passed. A separate implementation review passed tree 402368012c2ad99964d7cbfbf78b6b556915d7ed. Native browser and real Linux execution were not rerun; current-head hosted CI is tracked separately.

Pre-submission close confirmation and Host-reload recovery

Commit 3254433f addresses the field-edit teardown finding. Current follow-up ec99fe71 also addresses Amy's Host-lifetime follow-up. The branch includes upstream main through 195bfc34, resolving the shared-packager conflicts after #327.

Connection-field edits and switching execution location wait for explicit closure confirmation before changing the old fields or releasing the receipt. Failed or unconfirmed closes retain their cleanup IDs across form remounts. Local teardown failures remain retryable; only successful local cleanup records same-owner already_closed history.

DSH rc.5 can reuse same-revision client modules and cached exports, so this implementation does not assume that a Host restart re-evaluates the client bundle. A Host with no live entry or closure history returns a distinct unknown_receipt error. Other-session live entries and tombstones remain rejected. Only that explicit unknown result permits a two-step action to forget the selected local record; it sends no close request and does not claim that SSH or a remote task has stopped. Ordinary HTTP/permission/network failures retain the record. Global session/ID attempt tracking and confirmation tokens reject late responses and stale confirmations; other cleanup IDs remain protected.

Verification:

  • At integration tree ef993b39360a97ffa136f0f08368c157dae231c0, 35 selected DSH/Codex/WorkSwarm package, runtime, installation and integrity tests passed. Independent comparison confirmed Codex and WorkSwarm archives remain byte-identical for the same synthetic input before/after the packaging extraction.
  • At current tree b5bf57500a770d31362a760ce591e6788f890daf, both DSH package/SSH tests passed with CI=true (Windows / CPython 3.12.1 / Node 24.13.1). Ruff and whitespace checks passed. A separate implementation review independently generated the current client bundle and passed the form, HTTP/tool bridge and real loopback SSH2/SFTP/tunnel fixtures.
  • Coverage includes close rejection/retry/unmount/remount, Host-unknown recovery with explicit confirmation, foreign receipts, overlapping cleanup IDs and late requests, stale confirmation invalidation, and negative HTTP/permission/network cases.
  • Native DSH browser journeys, an actual Host-restart experiment, real Linux bootstrap/benchmark execution and a line-by-line vendored SSH audit were not rerun. Host/client lifetime conclusions come from pinned rc.5 source; SSH fixtures simulate remote command responses. Current-head hosted CI is recorded separately.

Remaining work and Issue closure

  • Validate installation and the complete optimization/dashboard workflow on real Linux GPU and NPU targets and the RFC's remaining host combinations.
  • A remotely hosted DSH Web deployment also requires forwarding the returned Host loopback dashboard port to the browser machine.
  • Release automation and marketplace distribution remain follow-up work.
  • The human Issue owner retains full-RFC acceptance and closure.
  • PR readiness: Draft for implementation review.
  • Goal ready for acceptance? Not claimed.

Add a native input form, session-scoped SSH connections and canonical dashboard access through the bundled InferRSI runtime.

Agent-assisted-by: Codex /root (implementation)

Agent-assisted-by: Codex /root/packaging (SSH transport and regression checks)
Signed-off-by: zengchuang <zengchuang3@huawei.com>

Copy link
Copy Markdown
Collaborator

Review of base 01c73521ced6ca760cdfa42c9b937fe985a20d64 and head 1379a20b5ff9572380d3c66144d99634e7986122 (first, full review; vendor/ssh2.cjs itself not audited line by line).

Thanks for the thorough write-up and the synthetic SSH fixture — the host-key-before-auth ordering, credential clearing and path containment checks look carefully done. A few things I think need to change or be clarified before this can pass:

  1. run_script / non-run CLI ops are capped at 120 s and a timeout tears down the whole connection — plugins/inferrsi-dsh/remote.cjs L539 (let timeout = 120000) with the exclusive cleanup at L552 (() => destroy(entry)). Only cli run gets a --max-minutes-derived timeout. A real benchmark or quality script (L515-523) will commonly exceed 2 minutes; when it does, the SSH session, SFTP handle and any live dashboard tunnel are destroyed, and because credentials were cleared after the test the user has to re-enter them in the form. Expected: derive the run_script timeout from the task budget (or an explicit argument), and on timeout/cancel terminate only the running channel instead of the whole connection. Please also add a fixture case for a script that outlives the default timeout.

  2. The shared skill text contradicts the new remote path — plugins/inferrsi/skills/inferrsi/SKILL.md L112-113 still says "The runner is local … This plugin does not dispatch SSH jobs", and the new dashboard section (L141-144) says to "use the existing authorized port-forwarding setup" for a remote target. The DSH prompt (form.cjs L60) instead tells the agent to use inferrsi_remote cli/dashboard. Since this SKILL.md is the same file shipped to Codex and DSH, the agent gets conflicting instructions. Expected: either a host-conditional section that documents inferrsi_remote for DSH, or keep remote guidance out of the shared skill and put it in DSH-only assets.

  3. The "no general shell commands" boundary is not actually enforced — remote-host.cjs L80 advertises that, but write (remote.cjs L486 onward) can overwrite any regular file under the repo root, including the user-designated benchmark/quality script, .git/hooks/*, or any script later passed to cli run --baseline-command, and run_script / cli run then execute it. That is effectively arbitrary code execution as the SSH user. This may be acceptable by design (the agent edits code anyway), but then the tool description and docs/dsh-plugin.md should say so plainly rather than imply a sandbox; alternatively block writes to the designated scripts and .git/. Which is the intended model?

  4. Question — test-connection lifetime is tied to the HTTP request signal — remote-host.cjs L52 builds signal from request.signal, and L69 closes the connection when it aborts, even after a successful response (the listener is also never removed). Some server adapters abort request.signal when the response/connection closes normally. Could you confirm DSH's connection.fetch does not do that (the bridge check at tests/check_dsh_bridge.cjs L1695 actually exercises abort-after-success → close)? If it can, a verified connection would vanish right after the form shows success.

  5. Question — owner check on /api/inferrsi/connection — the PR describes an "owner-only connection endpoint", but the handler (L45-77) trusts the body's sessionId, while tool calls use exec.agent.id. Is the route already authenticated/session-bound by DSH, and is it guaranteed that the client sessionId equals exec.agent.id? If not, please bind the test endpoint to the caller's authenticated session.

  6. CI coverage — the Node checks are skipped when node is absent (tests/test_dsh_plugin.py L135, L198). Please confirm CI has Node so the bridge/form/remote checks actually run, or make their absence fail in CI.

Minor/optional: field label is "硬件后端" in client.cjs but "计算后端" in form.cjs error messages; every cli call re-uploads the full base64 runtime zip over stdin, which may be worth caching remotely later.

Not verified: real Linux target execution of the bootstrap, and the vendored ssh2 bundle contents vs. the stated upstream version.

Copy link
Copy Markdown
Collaborator

Small correction to the line references in my comment above (same head 1379a20b5ff9572380d3c66144d99634e7986122, content unchanged):

  • Item 4: plugins/inferrsi-dsh/remote-host.cjs L51 (signal built from request.signal) and L68 (abort listener that closes the connection); the bridge check is tests/check_dsh_bridge.cjs L50 (controller.abort() after the successful test).
  • Item 5: the handler is remote-host.cjs L45-76.
  • Item 6: the Node skips are tests/test_dsh_plugin.py L37 and L100.

The remote.cjs, form.cjs and SKILL.md references were correct.

@Dong1017 Dong1017 added the area:workbench CLI workbench: profile, A/B loop, quality gates, evidence reports label Nov 5, 2026 — with ChatGPT Codex Connector
Bound remote scripts by the remaining task budget, retain command ownership until channel close, and separate request cancellation from established connection lifetime. Clarify host execution paths and trust boundaries, and require native Node checks in CI.

Agent-assisted-by: Codex (implementation and verification)
Signed-off-by: zengchuang <zengchuang3@huawei.com>

@Dong1017 Dong1017 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scoped incremental static review of PR #361 at 1792257c48af30d2af745dbe239da30b94f78c2d (base 01c73521ced6ca760cdfa42c9b937fe985a20d64): one additional form-submission/SSH lifetime finding in client.cjs, which is unchanged by the newest commit. This is distinct from the existing timeout, host request-signal, and owner-binding discussion; those threads should remain in place. I inspected the submit/cleanup call paths and the existing form fixture, but did not execute tests or audit the vendored SSH bundle. This COMMENT does not approve the PR or alter its scope.

Comment thread plugins/inferrsi-dsh/client.cjs Outdated
inFlight.current = true;
setSending(true);
setError('');
submittedConnection.current = form.execution === 'remote';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lifecycle / failed submission (medium). This sets submittedConnection.current = true before submitPrompt() resolves. If conversation.send rejects (e.g. the conversation service is unavailable or a definite local validation failure occurs), the catch at L122–123 never resets it. If the user then navigates away and this form unmounts while the Agent session itself remains open, the cleanup at L63–72 skips remoteRequest({action:'close'}), even though no task was handed off; the tested SSH connection/tunnel can remain allocated until separate session/plugin teardown. Please distinguish confirmed delivery from rejected/ambiguous delivery and provide a safe disconnect-on-abandon path (do not blindly close on an ambiguous send that may actually have been accepted). A form test for rejected submit → unmount, alongside the existing success path, would make this observable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 010c9bcb, including Amy's matching follow-up.

  • The form now tracks owned / pending / handedOff / ambiguous rather than declaring handoff before sending. Wrapper-local failures before invoking conversation.send are definite rejection: abandoning that form closes its unused receipt, including rejection arriving after unmount.
  • Success confirms admission and retains the connection. Errors after entering send are conservatively ambiguous: the actual rc.5 send contract returns Promise<void> and flattens rejected results to ordinary errors, so I do not infer safe rejection by parsing message text.
  • Pending, confirmed and ambiguous submissions cannot replace the receipt through edits, retesting or duplicate sends. The UI provides retain/check-conversation and explicitly labelled disconnect choices. Disconnect may stop the accepted remote task; only a successful close resets the form. Failed close retains the protected state.
  • The actual bundled-component regression covers definite rejection → unmount, unmount before each late outcome, success/ambiguous retention, stale handlers, blocked edits/retests/resubmits, and failed/successful explicit disconnect. Docs describe the same ownership boundary.

Final-tree Windows verification (CPython 3.12.1 / Node 24.13.1, CI=true): both DSH package/SSH tests passed, with the expanded form fixture included; Ruff and whitespace checks passed. A separate implementation cross-check passed tree 402368012c2ad99964d7cbfbf78b6b556915d7ed. All five applicable Ready jobs and required policy passed at this head. Amy's current-head approval also confirms this finding is addressed.

No native browser UI or real Linux bootstrap/benchmark rerun is claimed; the remote fixture still synthesizes command responses.

Copy link
Copy Markdown
Collaborator

Re-review of incremental changes: 1379a20b5ff9572380d3c66144d99634e7986122 .. 1792257c48af30d2af745dbe239da30b94f78c2d (commit 1792257c48af).

Thanks for the follow-up — my previous six points look addressed on this head:

  1. run_script / long CLI budgets — run_script now requires maxMinutes; long CLI ops accept the same hard cap; timeout/cancel closes only the execution channel and keeps SSH/SFTP + a separate dashboard; busy ownership lasts until the channel closes. Fixture coverage advances past the old 120 s wall.
  2. Shared skill vs remote path — SKILL.md, form.cjs prompt and docs/dsh-plugin.md now distinguish local launcher vs inferrsi_remote cli/dashboard.
  3. Trust boundary wording — tool description and docs state that path containment is not an execution sandbox and that write + run_script/cli run execute with the SSH user's permissions.
  4. Test-connection vs request.signal — a completed test no longer retains an HTTP abort listener; only a still-pending / already-aborted test tears the connection down. Bridge regression covers that.
  5. Owner / session binding — docs + bridge checks cover owner-route admission, session selection vs exec.agent.id, and cross-session receipt rejection. (I did not re-audit upstream DSH source myself.)
  6. CI Node — require_node_in_ci fails the suite when CI is set and Node is missing.

Still blocking for my approve (also raised by @Dong1017 on plugins/inferrsi-dsh/client.cjs ~L117):

  • submittedConnection is set before submitPrompt resolves. On a definite rejected send, the catch shows an error but never clears submittedConnection.current. If the form then unmounts while the Agent session remains open, the cleanup at L63–72 skips remoteRequest({action:'close'}), so the tested SSH connection can stay allocated until separate session/plugin teardown. Please mark the connection as handed off only after confirmed delivery (or provide a safe disconnect-on-abandon path that does not blindly close an ambiguous send that may already have been accepted), and add a form/bridge case for rejected submit → unmount alongside the success path.

Not approving yet; happy to re-review once that submit/abandon lifecycle is fixed. I did not re-run the native DSH Web UI or a real Linux target for this incremental head.

Copy link
Copy Markdown
Contributor Author

Thanks for the review and corrected references. Addressed in 1792257c:

  1. Budget and cancellation: run_script now requires an explicit maxMinutes from the remaining task budget. Long CLI operations accept the same hard cap; cli run also retains its own bound. Timeout/cancel/error requests TERM and closes only that command's channel, preserving SSH, SFTP and a separate dashboard. Command ownership remains busy until the channel closes. The SSH fixture covers exceeding the old 120 s limit, both budget-order cases, timeout/cancel reuse and delayed-close/error races.
  2. Shared instructions: the skill, form prompt and guide now distinguish local execution from DSH's verified inferrsi_remote cli/dashboard path, including startup checks, returned URL and shutdown behavior. No local launcher path is prescribed for remote execution.
  3. Trust model: the intended model permits editing task code and running designated scripts with the SSH user's permissions. Tool text and guide now state explicitly that the fixed operations/path checks are not an execution sandbox; edited code, scripts or hooks can have that user's effects. No write restriction is claimed.
  4. HTTP lifetime: a successful test transfers connection lifetime to the session/plugin, without retaining the request's abort listener. Pending cancellation and cancellation at result handoff still clean up. Regression tests also prove a later request abort leaves the successful connection usable. For precision, rc.5's normal-response guard avoids aborting an already completed response; this is not a claim that every normal rc.5 response closed its connection.
  5. Owner/session contract: verified against the actual rc.5 Host dispatcher (owner: 200; non-owner: 403) and official route source. DSH authenticates the owner before invoking the handler. Body sessionId selects that owner's target session, rather than authenticating it. DSH's Agent identity uses the same SessionId; tool calls use exec.agent.id, and cross-session read/close attempts are rejected. This distinction is documented.
  6. CI coverage: missing Node now fails the DSH module in CI instead of silently bypassing native checks; an explicit missing-Node negative probe passed. All five applicable current-head Ready jobs passed (both full-check and installed-package versions plus PR references); KV qualification was skipped for Draft. Required policy is successful.

Final-tree local verification: Windows / CPython 3.12.1 / Node 24.13.1, CI=true: 14 DSH/Codex plugin tests passed, 19 deselected, including the packaged form/bridge checks and real SSH-protocol fixture. Ruff, whitespace checks and a separate code cross-check of tree 7056876e661704cfec26aa5b40c453a8cff3b86f passed.

The fixture still synthesizes remote command responses. Real Linux bootstrap/benchmark execution and the full native UI journey were not rerun for this fix; the earlier native UI receipt remains bound to 1379a20b. The PR description now separates that earlier evidence from this fix. Draft and whole-Issue acceptance status remain as recorded.

Distinguish preflight rejection, pending and confirmed delivery, and ambiguous send failures. Clean up rejected abandoned receipts, preserve accepted tasks, and require an explicit disconnect before retrying or replacing an uncertain or handed-off connection.

Agent-assisted-by: Codex (implementation and verification)
Signed-off-by: zengchuang <zengchuang3@huawei.com>
amy-why-3459
amy-why-3459 previously approved these changes Nov 5, 2026

@amy-why-3459 amy-why-3459 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

PR review verdict — PASS
Reviewed base 01c73521ced6ca760cdfa42c9b937fe985a20d64 and head 010c9bcbd457bca998cf9d2a494689db180c4869 (incremental 1792257c48af30d2af745dbe239da30b94f78c2d .. 010c9bcbd457bca998cf9d2a494689db180c4869).

Verified the previous blocking submit/abandon finding is addressed in plugins/inferrsi-dsh/client.cjs:

  • connectionHandoff states (owned / pending / handedOff / ambiguous) replace the early submittedConnection flag.
  • submitPrompt classifies preflight failures as rejected vs post-conversation.send failures as ambiguous.
  • Rejected abandoned submissions close the unused SSH receipt; confirmed or ambiguous delivery retains it until explicit disconnect or session/plugin disposal.
  • Form fields and retest/resubmit stay blocked while protected; docs in docs/dsh-plugin.md match that contract.
  • tests/check_dsh_form.cjs covers rejected/pending/unmounted/success/ambiguous races and failed/successful disconnect.

Prior six review points on 1792257 remain addressed on this lineage. Not re-verified here: native DSH Web UI after this commit, real Linux bootstrap/benchmark execution, or a line-by-line audit of vendor/ssh2.cjs.

@Dong1017 Dong1017 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scoped incremental static review of PR #361 at head 010c9bcbd457bca998cf9d2a494689db180c4869 (base 01c73521ced6ca760cdfa42c9b937fe985a20d64). The earlier submit/abandon handoff issue is addressed in this revision and is not reopened. One distinct pre-submission form-edit teardown failure path remains: a failed connection close can lose the receipt needed for cleanup or retry; see inline on client.cjs:99. I traced the form, Host close endpoint, remote manager lifecycle, and form regression cases. This is a source-only COMMENT review; no tests, remote target, or vendored SSH implementation were executed or validated, and no approval is implied.

Comment thread plugins/inferrsi-dsh/client.cjs Outdated
if (['execution', 'repository', 'host', 'sshPort', 'sshUser', 'authType', 'privateKey',
'password', 'passphrase', 'sshFingerprint', 'benchmark', 'quality'].includes(name)) {
testAbort.current?.abort();
if (connection) remoteRequest({ action: 'close', sessionId, connectionId: connection.connectionId }).catch(() => {});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — a failed close during field edit loses the live SSH receipt. After a successful connection test but before prompt submission, changing repository/host/other connection fields (or switching execution to local) fires this asynchronous close, ignores any failure, then unconditionally clears connectionRef.current and the form's connection at L100–103. If the close never reaches the Host or is rejected before remote.cjs destroys the entry, the SSH/SFTP connection remains in the manager's connections map, but this form has discarded its connectionId; unmount cleanup also sees a null receipt and cannot retry. The new disconnectHandoff() path correctly preserves the receipt on close failure, but this pre-handoff field-change path does not. Please retain the receipt until close is confirmed; on failure or ambiguous transport outcome, keep an explicit retry/disconnect state rather than silently dropping ownership (handle an already-closed backend as a separate case). Add a form regression for connected → edit field / switch local → close rejects → retry or unmount cleanup, alongside the successful-close case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 3254433f and ec99fe71, including Amy's Host-lifetime follow-up. 195bfc34 also resolves the conflict with the merged shared host-packager changes.

  • Connection edits and switching execution location apply only after an explicit close acknowledgement. Failed/uncertain teardown retains its receipt across retry/unmount/remount.
  • @amy-why-3459 rc.5 can preserve same-revision active client entries and cached module exports, so the fix does not rely on Host/client lifetime coupling.
  • Only a genuinely missing live/history entry returns typed unknown_receipt. Other-session live entries/tombstones remain rejected. A typed unknown result permits a separately confirmed local-record-only discard; it sends no close request and does not claim SSH or remote tasks stopped. Permission/network/unconfirmed failures retain the record.
  • Global session/ID pending-attempt tracking and result-bound confirmation tokens prevent late responses, remount races or stale confirmation handlers from authorizing a discard. Other cleanup IDs remain protected.

Verification: both current-tree DSH package/SSH tests passed with CI=true on Windows/CPython 3.12.1/Node 24.13.1, plus Ruff and whitespace checks. Separate implementation verification rebuilt the current client bundle and passed the form, HTTP/tool bridge and real loopback SSH2/SFTP/tunnel fixtures at tree b5bf57500a770d31362a760ce591e6788f890daf. The upstream packaging integration also passed 35 selected DSH/Codex/WorkSwarm tests and archive byte-parity checks at tree ef993b39360a97ffa136f0f08368c157dae231c0.

Current head ec99fe71f5ff55125b83a359fc4317847210edcf: Ready run 42077946492 passed all five applicable jobs (installed-package 3.10/3.11, full-check 3.10/3.11, pr-references); kv-task-qualification was skipped for Draft. The required knowledge policy also passed. GitHub confirms the branch has no merge conflicts.

Scope limits: native DSH browser journeys, a real Host-restart experiment, real Linux bootstrap/benchmark execution and a line-by-line vendored SSH audit were not rerun. The lifetime conclusion above is source-based; SSH fixtures synthesize remote command responses. The PR remains Draft.

Confirm close before applying connection edits, preserve failed cleanup receipts across form remounts, and distinguish owner-scoped already-closed acknowledgements from unknown or other-session IDs. Keep partial teardown retryable and cover failure, concurrency and lifecycle paths.

Agent-assisted-by: Codex (implementation and verification)
Signed-off-by: zengchuang <zengchuang3@huawei.com>

Copy link
Copy Markdown
Collaborator

Re-review of incremental changes: 010c9bcbd457bca998cf9d2a494689db180c4869 .. 3254433f71fb7ea5498a8ee56cf097c546741dcc (commit 3254433f71, "retain receipts until remote closure is confirmed"). Base 01c73521ced6ca760cdfa42c9b937fe985a20d64.

Thanks — @Dong1017's pre-submission edit-teardown finding (client.cjs ~L99 on the previous head) looks addressed on this head:

  • update() (plugins/inferrsi-dsh/client.cjs L152-170) now awaits closeReceipt() and only applies the field change after a confirmed closed / already_closed; a failed or non-confirming result (L96) keeps the old inputs and receipt, and locks edits/submit/retest via closeState / lockedConnection.
  • Failed receipts survive remounts in the session-scoped pendingClosures map (L5), unmount joins an in-flight close and retries once, and late test results after abandonment go through the same path.
  • remote.cjs destroy() (L221-240) now keeps the entry when teardown throws so the owner can retry, and close() (L624-635) distinguishes owner-scoped already_closed from unknown / cross-session IDs. Host + tool close now return the manager result (remote-host.cjs L55-56, L98-99).
  • Form / bridge / remote fixtures cover edit → close rejects → retry, already_closed, unmount retry, remount resume, cross-session isolation and concurrent late receipts.

One thing I'm not sure about, so I'm not approving yet:

  1. Possible permanent lock when the Host no longer knows a queued receipt. Once an ID is in pendingClosures, the form blocks edits, submit and retest (L153, L172, L209), and the only action is 重试断开远程连接, which loops the queue and stops on the first failure (disconnectHandoff, L244-256). On the Host side, close() answers connection_unavailable for any ID that is neither live nor in closedReceipts (L635), and closedReceipts is cleared by closeSession() (L642) and dispose() (L650). So if the Host side is disposed/reloaded (plugin reload/upgrade, Host restart) while the client module — and therefore pendingClosures — survives, or if a session ID is ever reused after closeSession, every retry fails and that session's form stays locked with no way out. Could you confirm whether the client bundle is always re-evaluated together with the Host plugin lifetime in DSH rc.5? If not, please add a safe escape — e.g. scope queued receipts to a Host lifetime/instance token so a new Host lifetime is recognised as "nothing left to close", or a clearly-labelled "forget this receipt" action once the Host reports the ID as unknown to this owner — plus a form case for "queued receipt → Host reports connection_unavailable → form recovers".

Optional (not blocking): closedReceipts only shrinks on session/plugin disposal, so a long-lived session that tests many connections accumulates entries; a cap or TTL would bound it.

Not verified here: tests were not executed by me, no native DSH Web UI or real Linux target run, and vendor/ssh2.cjs not audited.

Preserve the merged WorkSwarm and Codex builders, move canonical runtime staging into their common packaging module, and use it from DSH. Keep both host entry links and verify 35 affected packaging/runtime tests.

Agent-assisted-by: Codex (implementation and verification)
Signed-off-by: zengchuang <zengchuang3@huawei.com>
Keep unknown Host receipts distinct from cross-session refusals. Permit a two-step local-only discard after a typed unknown response, guard it against overlapping close attempts and stale confirmations, and retain retry behavior for all other failures.

Agent-assisted-by: Codex (implementation and verification)
Signed-off-by: zengchuang <zengchuang3@huawei.com>

@amy-why-3459 amy-why-3459 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

PR review verdict — PASS
Reviewed base 57621339ad08bb32228bccda7d60217545316351 and head ec99fe71f5ff55125b83a359fc4317847210edcf (incremental 3254433f71fb7ea5498a8ee56cf097c546741dcc .. ec99fe71f5ff55125b83a359fc4317847210edcf, including merge 195bfc347485).

Verified the previous Host-unknown permanent-lock concern is addressed:

  • remote.cjs close() returns distinct unknown_receipt when the ID is neither live nor in closedReceipts; cross-session live/tombstone cases stay connection_unavailable.
  • client.cjs enables 放弃此本地连接记录 only after a typed unknown_receipt on a queued ID, with a second confirmation step, request-token freshness, and no automatic close call; docs in docs/dsh-plugin.md match that contract.
  • Form/bridge/remote fixtures cover two-step forget, remount resume, stale confirmations, overlapping close attempts, foreign IDs/sessions, and negative HTTP/permission/network paths that must not authorize forget.
  • Shared packaging import via tools/plugin_bundle.py for DSH looks consistent with the upstream merge.

Prior review points on earlier heads remain addressed on this lineage. Optional closedReceipts TTL remains a non-blocking follow-up.

Not verified here: tests were not executed by me; no native DSH Web UI or real Host-restart/Linux target run; vendor/ssh2.cjs not audited. Hosted CI for this head was partially green (declarations/installed-package/pr-references) with full-check still in progress at review time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:workbench CLI workbench: profile, A/B loop, quality gates, evidence reports

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants