Repository navigation
DSV41-6 enroll sshpass as a host_cli_dependency for password-session runners (srv3/srv4 lack it) - #11096
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7e455ec36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -214,7 +216,7 @@ fn fleet_converge_spark_target_description() -> String { | |||
| ], "") | |||
There was a problem hiding this comment.
Adding this twentieth mode makes both enrolled assertions in dag/test/claim/workflow_dispatch_input_witness_test.dag fail: lines 362–363 still require 19 vocabulary entries and line 371 still requires 19 dispatch options. The required witness lane will therefore be red for every build of this commit until those independent count oracles are updated to 20.
Useful? React with 👍 / 👎.
| run: | | ||
| ROOT=$(git rev-parse --show-toplevel 2>/dev/null || pwd) | ||
| "$ROOT/target/release/gunbc" run --source-root "$ROOT/dag" --source-root "$ROOT/src/v2" --entry dag/gunbc/runner/runner_password_session_tool_converge.dag --function runner_password_session_tool_converge_ci_wet | ||
| cat "$ROOT/target/runner-password-session-tool-converge-receipt.txt" |
There was a problem hiding this comment.
When runner_password_session_tool_converge runs, this is the only exposure of target/runner-password-session-tool-converge-receipt.txt; the following upload step points at the runner-host-file receipt and is gated exclusively to the two runner_host_file_* modes. Consequently this mode never publishes its receipt as an artifact, including on the failure path for which the new entry explicitly writes the receipt before returning, so a dedicated always()-gated upload is needed.
Useful? React with 👍 / 👎.
| match outcome.verdict { | ||
| Srv3PredicateHolds => "present" | ||
| Srv3PredicateFails => "absent" | ||
| Srv3PredicateUnobserved { reason: _ } => "unobserved" |
There was a problem hiding this comment.
When a Fleet SSH probe is refused or an enrolled tool has no apt source, converge_one_password_session_tool stores an actionable reason in Srv3PredicateUnobserved, but this renderer discards it. Because runner_password_session_tool_converge_ci_wet returns only the rendered body on a failed outcome, operators receive merely sshpass=unobserved and cannot distinguish a transport/authentication failure from an acquisition-model gap.
Useful? React with 👍 / 👎.
…y String. Review 64038: the failing diagnostic must name why a tool is unobserved, and apt_package_of_witness_test's witness_note is an annotation, not program data. The dispatch-mode count follows the twentieth mode so the witness lane is not vacuously red, and the receipt uploads on failure because FailFast never cats it. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed review 64038 on
Also raised the fleet-converge mode-count oracle 19→20 (twentieth mode would have reddened the required witness lane) and added an |
They are uncommitted session probes, not consumers of the convergence model. Leave them on disk; do not hand-edit the generated gitignore. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Dropped the session-scaffold probes from the branch ( |
A hand-updated count against the live roster is a change detector: bumping N when a mode is added collapses the assertion to measure() == measure(). The new mode must be on the roster, wires must be unique, and options join the roster both ways. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addendum from the manager, on
|
posix_command_v_check_argv is not a PortableRemoteWord vector, so the password-session ensure refused before contacting srv4. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
version_ok would otherwise stay Unobserved after a present probe. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Converge
sshpasson password-session runners (srv3/srv4) through the existingsrv3_ensure_apt_toolinstaller over FleetSsh. Package identity comes fromextdeps.tools apt_package_of(Optional, never a default). Dispatch is a step onfleet_converge_workflow, not a new job and not a new HostEffect.Review 64038
Unobserved receipts render
unobserved:<cause>. Theapt_package_ofwitness commentary is a//annotation, not aStringrow.Scaffold probes
src/v2/test/claim/manual/dsv41_6_probe.daganddsv41_6b_probe.dagare not on this branch. They are session probes, not consumers.Mode witness (not a census literal)
fleet_converge_workflow_modesstill re-enumerates the coproduct (roster_re_enumerates_its_own_rows_stall). A hand-updatedcount == Nis a change detector: bumping N when a mode lands ismeasure() == measure(). This PR does not keep that convention. The witness joins identities:RunnerPasswordSessionToolConvergeis on the roster, wires are unique, and every dispatch option is a rostered wire (and the reverse).FleetSsh presence is path-presence, not PATH presence
posix_command_v_check_argvis not portable (sh -cpayload). FleetSsh therefore usesshell.Test.IsExecutableoverextdeps.apt apt_installed_bin_path(apt_bin_dir+ binary): executable at FHS/usr/bin/<binary>, not present on PATH. LocalShell and SshShell still usecommand -v. A tool installed outsideapt_bin_dir(diverted path,/usr/sbin, a local build) now reads absent and the ensure takes the install arm; apt no-ops if the package is already recorded, so the host is not harmed, but the receipt can report an install for a host that already had the binary elsewhere.Path-presence is the right question for an apt-acquired tool: the installer places the binary at that FHS path, and the password-session exec is that argv0. PATH search would accept a shadow apt does not own.
The FleetSsh arm of
srv3_tool_bin_pathanswers the same path-presence question (it returnsapt_installed_bin_path, not PATH stdout). It is in this change becausesrv3_apt_tool_version_okresolves the binary through that function; leaving FleetSsh onposix_command_v_check_argvwould makeversion_okUnobserved after a successful presence probe.srv3_toolchain_rowsproduction callers are unaffected:host_toolchain_ensureuseshost_identity_ssh_access(SshShell); seeded-install-media toolchain ensure usesci_deploy_srv1_access(LocalShell). Neither uses FleetSsh. Both still rely on PATH presence on those transports.Test plan
Hermetic rows in
dag/test/claim/runner/runner_password_session_tool_converge_witness_test.daganddag/test/claim/extdeps/apt_package_of_witness_test.dag.Wet
fleet-converge.yml--ref session/merry-lark-687-dsv41-6bmode=runner_password_session_tool_convergehost=srv4:test -x /usr/bin/sshpassexit=1 (absent) → install (~9s) → reprobe → receiptsshpass=present.sshpass=present.