Repository navigation
feat(dsh): add structured iteration form and SSH execution - #361
zengchuang-hw wants to merge 6 commits into
Conversation
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>
|
Review of base 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:
Minor/optional: field label is "硬件后端" in Not verified: real Linux target execution of the bootstrap, and the vendored ssh2 bundle contents vs. the stated upstream version. |
|
Small correction to the line references in my comment above (same head
The |
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
left a comment
There was a problem hiding this comment.
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.
| inFlight.current = true; | ||
| setSending(true); | ||
| setError(''); | ||
| submittedConnection.current = form.execution === 'remote'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Addressed in 010c9bcb, including Amy's matching follow-up.
- The form now tracks
owned / pending / handedOff / ambiguousrather than declaring handoff before sending. Wrapper-local failures before invokingconversation.sendare definite rejection: abandoning that form closes its unused receipt, including rejection arriving after unmount. - Success confirms admission and retains the connection. Errors after entering
sendare conservatively ambiguous: the actual rc.5 send contract returnsPromise<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.
|
Re-review of incremental changes: Thanks for the follow-up — my previous six points look addressed on this head:
Still blocking for my approve (also raised by @Dong1017 on
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. |
|
Thanks for the review and corrected references. Addressed in 1792257c:
Final-tree local verification: Windows / CPython 3.12.1 / Node 24.13.1, 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 |
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
left a comment
There was a problem hiding this comment.
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:
connectionHandoffstates (owned/pending/handedOff/ambiguous) replace the earlysubmittedConnectionflag.submitPromptclassifies preflight failures asrejectedvs post-conversation.sendfailures asambiguous.- 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.mdmatch that contract. tests/check_dsh_form.cjscovers 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
left a comment
There was a problem hiding this comment.
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.
| 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(() => {}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
|
Re-review of incremental changes: Thanks — @Dong1017's pre-submission edit-teardown finding (
One thing I'm not sure about, so I'm not approving yet:
Optional (not blocking): Not verified here: tests were not executed by me, no native DSH Web UI or real Linux target run, and |
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
left a comment
There was a problem hiding this comment.
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.cjsclose()returns distinctunknown_receiptwhen the ID is neither live nor inclosedReceipts; cross-session live/tombstone cases stayconnection_unavailable.client.cjsenables 放弃此本地连接记录 only after a typedunknown_receipton a queued ID, with a second confirmation step, request-token freshness, and no automatic close call; docs indocs/dsh-plugin.mdmatch 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.pyfor 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.
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.
Knowledge authorship declaration
Not applicable: no paths protected by
.github/knowledge-model-policy.jsonare changed. The commit retains DCO sign-off and material agent attribution.Changed artifacts and evidence
plugins/inferrsi-dsh/,tools/build_dsh_plugin.pyclient.cjs,form.cjsremote-host.cjs,remote.cjsdocs/dsh-plugin.mdInitial 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.src,tests, and the two plugin builders: passed.git diff --check: passed.>=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.Review-feedback fixes
Commit
1792257caddresses Amy's six review points, including the corrected references in her follow-up.maxMinutesbudget; 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.exec.agent.id, with cross-session receipts rejected.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 tree7056876e661704cfec26aa5b40c453a8cff3b86f; this is implementation verification, not a formal independent approval.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
010c9bcbaddresses 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 tree402368012c2ad99964d7cbfbf78b6b556915d7ed. 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
3254433faddresses the field-edit teardown finding. Current follow-upec99fe71also addresses Amy's Host-lifetime follow-up. The branch includes upstream main through195bfc34, 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_closedhistory.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_receipterror. 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:
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.b5bf57500a770d31362a760ce591e6788f890daf, both DSH package/SSH tests passed withCI=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.Remaining work and Issue closure