refactor(ssh): Phase 2 — route dx/flatcar/kde-smoke/software/vanilla-gnome onto ssh_config.ssh_argv() - #790
mrbobbytables wants to merge 2 commits into
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![kubestellar-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved by @castrojo for Hive auto-merge on green CI.
hanthor
left a comment
hanthor
left a comment
There was a problem hiding this comment.
Needs a rebase before it can be assessed properly
mergeable_state: dirty. Reproduced with a local test merge onto main at db329ee:
PR790 CONFLICT
14 files across tests/dx, tests/flatcar, tests/kde-smoke, tests/software, tests/vanilla-gnome, tests/shared and 4 unit-test modules, on a base from 2026-09-06. The green checks on this PR are from that date and no longer reflect what merging would produce.
Worth re-validating after the rebase
The three behaviour changes called out in the description are real fixes, not cosmetics, and each deserves a re-check once the branch is current:
tests/dx/tests/flatcargaining an explicit-p— these genuinely had no-pand noenvironment.pysettingssh_port, so a port-forwarded lane silently hit 22.kde-smoke's_run_hostreadingcontext.kde["ssh"], a dictbefore_allnever populates.vanilla-gnomepassing a 900s command timeout as ssh'sConnectTimeout.
Two things to watch
tests/vanilla-gnome/features/environment.pyandtests/shared/results_dir.pyoverlap with #791. #791 currently introduces fourF405errors in that file by deleting animport oswithout replacing it. Whichever lands second will need to reconcile. Worth coordinating the order.- Re-run the lint gate after rebasing. This PR widens
MIGRATED_MODULESintest_ssh_transport_contract.py, and several of the touched suites usefrom ... import *.mainis currently clean (ruff check tests/ scripts/ --select E,F,W --ignore E501→All checks passed!), so any newF405/F821would be attributable to this branch.
Separately: the base branch is currently failing Coverage snapshot fresh and one unit test independently of this PR (see #807). Don't read a red re-run as this PR's fault until that lands.
Generated by Claude Code
7df9b10 to
13d1fc5
Compare
|
Rebased cleanly onto latest
🐝 Hive Agent: |
… image_cache, screenshot, and run_ssh onto ssh_config.ssh_argv() Phase 2 of projectbluefin#772: extract the remaining raw ssh argv builders onto the single transport policy in tests/shared/ssh_config.py. - Add ssh_argv(quiet=...) so callers that need LogLevel=ERROR (previously hand-rolled at each site) opt in explicitly instead of restating it. - Fold ssh_steps.run_ssh onto ssh_argv(quiet=True); it now always resolves the port through the shared precedence chain (context > userdata > env > default), so unset ssh_port emits an explicit '-p 22' instead of silently relying on ssh's own default. - Migrate tests/dx and tests/flatcar's raw ssh argv (both gain the -p flag, fixing silent port loss under port-forwarded lanes). - Fix tests/kde-smoke/features/steps/steps.py:_run_host, which read a context.kde['ssh'] dict that before_all never populated — it always fell through to raw os.environ reads and silently ignored userdata-driven runs. It now resolves through ssh_argv(context), honouring the context attributes before_all does set. - Migrate tests/software's _flatpak/_run_in_session/_has_bazaar, which already used resolve_ssh_details, onto ssh_argv() for a single argv builder instead of two. - Migrate tests/shared/image_cache.py and tests/shared/screenshot.py. screenshot._ssh_run previously read only os.environ; it now resolves via the behave context bound by configure_screenshot_context. - Fix tests/vanilla-gnome/features/steps/steps.py, which passed the command's own timeout as ssh's ConnectTimeout, conflating the connect deadline with the command deadline (a 900s long-command timeout became a 900s connect timeout). It now uses ssh_argv()'s fixed 10s connect timeout and threads context through so future context-based resolution works. - Widen tests/unit/test_ssh_transport_contract.py's MIGRATED_MODULES to cover all of the above, so any of them re-growing a private ssh argv copy fails CI. tests/smoke/features/steps/offline_boot_steps.py remains unmigrated per the issue's Phase 1 exclusion (PR projectbluefin#768 covers its unit tests). Fixes projectbluefin#772 Signed-off-by: mrbobbytables <mrbobbytables@users.noreply.github.com>
13d1fc5 to
4e12b08
Compare
castrojo
left a comment
castrojo
left a comment
There was a problem hiding this comment.
Backlog triage pass. I have not approved and have not merged or enqueued anything: .github/copilot-instructions.md says agents never approve, merge, or enqueue, and AGENTS.md:136,163 + docs/skills/meta/human-gates/SKILL.md make merge a human gate. This is a review comment only.
The two red checks were not this PR's fault. pytest and Coverage snapshot fresh were both already failing on base commit db329ee from a stale committed coverage snapshot (tests/unit/test_update_coverage_snapshot.py::TestRepositoryInvariant::test_committed_snapshot_is_current). This PR touches no .feature file, so it cannot move the snapshot. Rebasing onto current main cleared both with no change to your diff; all six checks are green.
Reviewed the substance, and the quiet mapping is per-call-site correct rather than blanket: the sites whose old argv carried LogLevel=ERROR (run_ssh, image_cache, _has_bazaar, dx/flatcar/vanilla-gnome) get quiet=True, and the ones that never had it (kde-smoke _run_host, software _flatpak/_run_in_session, screenshot._ssh_run) keep the default. screenshot._ssh_run's new ssh_argv(_CURRENT_CONTEXT) is valid — _CURRENT_CONTEXT is a module global at tests/shared/screenshot.py:23, bound by configure_screenshot_context. Adapting test_no_port_flag_when_ssh_port_is_none into test_default_port_used_when_ssh_port_is_none rather than deleting it is the honest move given -p is now always emitted.
What a second reviewer should weigh. This carries three deliberate behaviour changes — dx/flatcar gain -p, kde-smoke stops reading the never-populated context.kde["ssh"], vanilla-gnome stops passing a command timeout as ConnectTimeout — and none of them has had a real-VM run, because ghost-lab has been red repo-wide since 2026-09-15. GHA boots no VM. Per docs/skills/ci-ops/contributing/references/reviewing-and-merging.md:98, that should be stated explicitly in the PR description.
AGENTS.md:146 also bites harder here than on most PRs: this changes an in-repo transport contract (-p always emitted, quiet opt-in) across five suites with no docs/skills/** note. A short paragraph in the ssh/test-authoring skill would be the substantive ask. See #861 on whether that rule is mandatory or advisory.
Sequencing: #816 adds unit coverage for tests/shared/screenshot.py's capture path, including argv assertions that this PR's _ssh_run rewrite will break. Whichever lands second needs those mocks re-verified.
Outstanding: two approving reviews, plus the repo-wide ghost-lab outage.
Gate status for this PR
- Ruleset
main — merge queuerequires 2 approving reviews (dismiss_stale_reviews_on_push: true,require_last_push_approval: true, emptybypass_actors). Required checks:Lint & syntax,Behave dry-run,pytest. - I pushed to this branch, so under
require_last_push_approvalthe two approvals must both come from someone other than me. ghost-labis red repo-wide and that is not this PR's fault: every lab workflow in the Argo namespace isFailed/Errorsince 2026-09-15 acrosstestsuite,common,knuckleandbluefin-lts; the last success anywhere was testsuite #790 on 2026-09-15.docs/skills/meta/human-gates/SKILL.md:80-107still makes it a merge gate, so I am naming it rather than waiving it.- On
AGENTS.md:146(matchingdocs/skills/**update):AGENTS.mdcalls it a mandatory gate,docs/skills/ci-ops/contributing/SKILL.md:78calls it a review expectation that no CI job enforces. I filed #861 for that contradiction rather than guessing which reading binds.
The behavioural half of this suite is good - fallback ordering, the
stale-PNG deletion, the every-method-fails path, and the fastfetch
terminal recovery all fail on a plausible bug. Six assertions did not:
- exact argv option list (`StrictHostKeyChecking=no`, the literal port
string) - ssh_config owns those flags and may reorder them
- the hardcoded default key path `/home/bluefin-test/.ssh/id_ed25519` -
a default, not an observable behaviour
- `calls[0].startswith("source /tmp/session.env && ")` - pins the exact
prefix rather than the requirement that the session env is sourced
- `len(calls) == 1` / `== 2` - pins the call count rather than the
ordering it stands for
- `argv[-1] == '"/tmp/out.png"'` - pins exact quoting rather than that
the path is quoted
Rewritten to assert the same contracts through what a consumer sees: the
SSH target and command, that grim runs first and short-circuits the rest,
that gnome-screenshot picks up the capture when grim is missing, that the
session env is sourced, and that a path with spaces stays quoted.
Mutation-checked: removing the `source /tmp/session.env && ` prefix from
tests/shared/screenshot.py still fails the suite; restoring it passes.
This also stops the suite from breaking on #790, which rewrites
_ssh_run to build its argv through ssh_config.ssh_argv().
Assisted-by: Claude Opus 4.5 via pi
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The behavioural half of this suite is good - fallback ordering, the
stale-PNG deletion, the every-method-fails path, and the fastfetch
terminal recovery all fail on a plausible bug. Several assertions did not,
and would have broken on a refactor that changes nothing observable:
- exact ssh option strings (`StrictHostKeyChecking=no`) - ssh_config owns
those flags and may reorder them
- the hardcoded default key path `/home/bluefin-test/.ssh/id_ed25519` -
a default value, not a behaviour
- `calls[0].startswith("source /tmp/session.env && ")` - pins the exact
prefix rather than the requirement that the session env is sourced
- `len(calls) == 1` / `== 2` - pins the call count rather than the
ordering it stands for
- `argv[-1] == '"/tmp/out.png"'` - pins exact quoting rather than that
the path is quoted
Rewritten to assert those contracts through what a consumer observes: the
SSH target and command, that SSH_KEY/SSH_PORT overrides reach the
connection and the default port is used without them, that grim runs first
and short-circuits the rest, that gnome-screenshot picks the capture up
when grim is missing, that the session env is sourced, and that a path
with spaces stays quoted.
Mutation-checked twice: dropping the `source /tmp/session.env && ` prefix
fails the suite, and ignoring the SSH_PORT override fails it; reverting
each passes.
Also documents the capture fallback chain and the stale-file deletion in
docs/skills/test-authoring/gnome/SKILL.md, which AGENTS.md:146 asks for on
a tests/** change and which no page covered before.
This also stops the suite from breaking on #790, which rewrites _ssh_run
to build its argv through ssh_config.ssh_argv().
Assisted-by: Claude Opus 4.5 via pi
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This PR does not just dedup a helper - it adds an AST guard in tests/unit/test_suite_environment_contract.py that makes "no suite may redefine _first_value" an enforced contract. AGENTS.md:146 wants the matching docs/skills/** note when a change introduces a contract. Placed in the unit-test reference rather than shared-ssh.md so this PR stays disjoint from projectbluefin#790, which documents the ssh_argv contract there. Assisted-by: Claude Opus 4.5 via pi Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This PR adds an AST guard asserting no after_all carries a /tmp/results literal, which makes the results-dir routing an enforced contract rather than a convention. AGENTS.md:146 wants the matching docs/skills/** note; no page documented resolve_results_dir before this. Filed under e2e-workflow troubleshooting because the observable symptom is a missing artefact, and to keep this PR disjoint from projectbluefin#790 (shared-ssh.md) and projectbluefin#787 (unit-test-module-stubs.md). Assisted-by: Claude Opus 4.5 via pi Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
castrojo
left a comment
castrojo
left a comment
There was a problem hiding this comment.
Backlog triage pass. I have not approved and have not merged or enqueued anything — .github/copilot-instructions.md says agents never approve, merge, or enqueue, and AGENTS.md:136,163 make merge a human gate. Review comment only.
Addendum — added the docs/skills/** note this PR was missing.
I flagged in my earlier review that AGENTS.md:146 bites harder here than on most PRs, because this alters an in-repo transport contract across five suites. Rather than leave that for you, I wrote it: docs/skills/test-authoring/behave/references/shared-ssh.md now records the two properties that are easy to get wrong —
-pis always emitted, including whenSSH_PORTis unset (falls back to22), so no test should assert the port flag is absent;quiet=Trueadds-o LogLevel=ERRORand is opt-in per call site, because passing it everywhere would hide diagnostics that kde-smoke and the screenshot helper depend on;
plus a pointer to the MIGRATED_MODULES guard so a new suite gets added there rather than hand-rolling argv.
I deliberately put this in shared-ssh.md and moved the notes for #787 and #825 elsewhere (unit-test-module-stubs.md and e2e-workflow/references/troubleshooting.md). My first attempt had all three writing to shared-ssh.md, which would have created a new three-way collision — exactly the disjointness problem AGENTS.md:122 exists to prevent, and exactly what I spent this pass clearing. The three file sets are now disjoint; I recomputed the overlap graph across all open PRs to confirm.
Re-verified on 395425d1: all six checks pass, 1611 unit tests green locally.
Everything else from my earlier review stands — in particular that the three deliberate behaviour changes (dx/flatcar gain -p, kde-smoke stops reading the never-populated context.kde["ssh"], vanilla-gnome stops passing a command timeout as ConnectTimeout) have had no real-VM run, and per reviewing-and-merging.md:98 that should be stated in the PR description.
Ruleset main — merge queue requires 2 approving reviews (dismiss_stale_reviews_on_push, require_last_push_approval, empty bypass_actors). I pushed to this branch, so both must come from someone other than me. ghost-lab has not posted for this head; it is red or unposted across the entire backlog and every other repo in the factory, and the pods are garbage-collected so I could not establish a root cause — that needs the lab owner.
|
Addressed the review feedback:
🐝 Hive Agent:
|
There was a problem hiding this comment.
Reviewed (correctness) — verified at head 395425d. One minor finding:
docs/skills/test-authoring/behave/references/shared-ssh.md:114says theMIGRATED_MODULEScontract lives intests/unit/test_suite_environment_contract.py, but it is actually intests/unit/test_ssh_transport_contract.py(line 20). A file namedtest_suite_environment_contract.pydoes exist, which makes the wrong pointer stick — contributors sent there will not find the allowlist.
Verified: quiet=True matches the prior LogLevel=ERROR usage at every migrated call site (dx, flatcar, image_cache, run_ssh, _has_bazaar, vanilla-gnome) and is correctly omitted where it was absent (kde-smoke, screenshot, software _flatpak/_run_in_session); dx/flatcar environment.py set key/user/ip but never ssh_port, so the new explicit -p genuinely fixes the silent port loss; touched unit tests (68) pass locally at head.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.78
AGENTS.md:146 wants a matching docs/skills/** update when a change alters a contract. This PR changes one across five suites - `-p` is now always emitted and `quiet` is opt-in per call site - with no note. Records both properties and the MIGRATED_MODULES guard in the shared-ssh reference. Assisted-by: Claude Opus 4.5 via pi Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
395425d to
47dfa7c
Compare
|
Addressed the review feedback:
🐝 Hive Agent:
|
4 participants
Summary
Phase 2 of #772: migrates the remaining raw
sshargv builders onto the single transport policy intests/shared/ssh_config.py, following on from #773 (Phase 1, smoke suite).Changes
ssh_config.ssh_argv()gains aquietkwarg — callers that previously hand-rolledLogLevel=ERRORnow opt in explicitly (ssh_argv(context, quiet=True)) instead of restating it.ssh_steps.run_sshnow callsssh_argv()instead of building its own argv. It always resolves the port through the shared precedence chain (context → userdata → env → default), so an unsetssh_portnow emits an explicit-p 22rather than silently relying on ssh's own default — behaviourally equivalent, but no longer a special case.tests/dxandtests/flatcar— both previously built argv with no-pflag and noenvironment.pysettingssh_port; under a port-forwarded lane they silently connected to port 22 regardless ofSSH_PORT. Both now go throughssh_argv()and gain the flag.tests/kde-smoke/features/steps/steps.py:_run_host— read connection details fromcontext.kde["ssh"], a dictbefore_allnever populates, so it always silently fell through to rawos.environreads and ignored userdata-driven runs (-D vm_ip=...). It now resolves viassh_argv(context), honouring the context attributesbefore_alldoes set.tests/software(_flatpak,_run_in_session,_has_bazaar) — already usedresolve_ssh_detailsbut rebuilt the argv inline; now callssh_argv()directly.tests/shared/image_cache.py,tests/shared/screenshot.py—screenshot._ssh_runpreviously read onlyos.environ; it now resolves via the behave context bound byconfigure_screenshot_context, honouring context/userdata too.tests/vanilla-gnome/features/steps/steps.py— passed the command's own timeout as ssh'sConnectTimeout, conflating the connect deadline with the command deadline (a 900s long-command timeout became a 900s connect timeout). Now usesssh_argv()'s fixed 10s connect timeout, withcontextthreaded through the helper chain for future context-based resolution.tests/unit/test_ssh_transport_contract.py—MIGRATED_MODULESwidened to cover all of the above, so any of them re-growing a privatesshargv copy fails CI.test_ssh_config.py,test_ssh_steps.py,test_vanilla_gnome_steps.py) whose mocks/assertions encoded the old per-site behaviour.tests/smoke/features/steps/offline_boot_steps.pyremains unmigrated, per the issue's Phase 1 exclusion (PR #768 already added its unit coverage).After this PR,
StrictHostKeyCheckingappears only intests/shared/ssh_config.py(source of truth) andtests/smoke/features/steps/offline_boot_steps.py(deliberately deferred) — everything else callsssh_argv().Behaviour changes (as flagged in the issue)
tests/dx,tests/flatcargain an explicit-pflag (fixes silent port loss under non-default SSH ports).tests/kde-smokesteps now actually honour userdata/context-driven connection settings instead of only environment variables.tests/vanilla-gnomeno longer conflates connect timeout with command timeout.Testing
Real-VM testing notice: Per
docs/skills/ci-ops/contributing/references/reviewing-and-merging.md:98, note that this change has had no real-VM run (ghost-laboutage repo-wide); the three deliberate behaviour changes (dx/flatcargaining-p,kde-smokehonouring userdata/context connection settings, andvanilla-gnomeusing fixed connect timeout) are verified via unit tests and behave dry-run only.python3 -m pytest tests/unit/ -q— 1529 passed.PYTHONPATH=. python3 -m behave --dry-run --no-summary tests/flatcar/features/andtests/kde-smoke/features/— clean, no step/import errors.tests/software,tests/vanilla-gnome,tests/dxdry-runs are blocked in this sandbox byqecore/dogtailnot being installable (pre-existing environment limitation, unrelated to this change); their step modules import cleanly under the existing unit-test stubs.Closes #772 (Phase 2; Phase 3 — repo-wide contract allowlist — left for a follow-up once
offline_boot_steps.pyis migrated).— hive: backend=copilot model=claude-sonnet-5
🐝 Hive Agent:
contributor| SHA:7df9b10b