Skip to content

feat(cli): per-verb help, notice policy, SQL input, and plugin install - #89

Merged
jamiesun merged 4 commits into
mainfrom
fix/issues-78-88
Sep 23, 2026
Merged

jamiesun merged 4 commits into
mainfrom
fix/issues-78-88

Conversation

@jamiesun

Copy link
Copy Markdown
Contributor

Fixes #78, #79, #82, #83, #84, #85, #86, #87, #88.

Ten issues, all on the surface Agents actually use: option handling, help
discovery, notice discipline, credential resolution, and plugin provisioning.
Each one was reproduced first, then fixed with a regression test, and the CI
lane it belongs to is green locally (make check, make test-e2e,
go test -race ./internal/... ./pkg/..., golangci-lint run → 0 issues).

What changed

#82 — CI Lint gate pinned (was failing on main since 25a45f3)
.golangci.yml kept the v1-era issues.exclude-rules key while declaring
version: "2", and golangci-lint-action@v7 floated on version: latest, so
the job aborted inside config verify before analysing any Go file. Migrated to
the v2 linters.exclusions.rules schema and pinned the linter to the released
v2.13.2.

#84 — --help is uniform, and answered before anything else happens
Every subcommand answers sshx <verb> --help (previously only sshx text did);
--help --json emits the same blocks as an sshx.help.v1 document, and
sshx text --help --json keeps its structured sshx.text.help.v1. The help
text now lives once, in internal/app/usage_sections.go, and is composed for
both the global and the per-verb surface, so they cannot drift. --help is
recognized in option position in any order and is answered before host
resolution, connection, or any trust-store write.

#85 — compatibility mode stops silently ignoring what it does not know
An unrecognized option is rejected with the offending token, a near-miss
suggestion, and the -- rule for commands that start with -. The guessed
--local/--remote names resolve to the real transfer surface
(--upload=<local> --to=<remote>), a missing upload/download destination names
--to, and the documented --ssh-password-key= is now accepted rather than
forwarded as part of the remote command.

#86 — a defined notice policy for merged streams
--quiet (--no-notices) suppresses human notices on stderr — deprecation
warnings, the policy-block mirror added by #81, sudo-boundary hints, sshx text
scan progress, logger narration — so a caller that merges streams (2>&1)
under --json reads exactly one parseable document in both the success and the
failure path. stdout, exit codes, and JSON documents are untouched. The policy
is written down in docs/contract.md.

#87 — SQL statements no longer have to be shell strings
sshx sql takes the statement positionally, after --, from
--statement-file=PATH, or from piped stdin, and a statement that opens with a
SQL comment is statement text instead of an "unknown sql option". Option-shaped
typos are still rejected with a suggestion, and the guard stays fail-closed: a
statement from a file or stdin goes through the identical classifier (verified:
a DROP TABLE from stdin is blocked exactly like the positional form).

#78 — run --target= resolves the sudo key per host
sshx run used the CLI default master for every target while apply used the
host's configured sudo_password_key. The rule is now expressed once
(execution.SudoKeyForTarget: explicit caller key → host key → default) and
honoured by the plan, the SSH client config, the keyring lookup, and the audit
evidence. The run summary records only a caller-level choice; each target's
audit event names the credential it used.

#79 / #83 — apply cleanup and its fixture
sshx apply cleaned its staging payload, publication temp, and unverified
backup with a bare rm -f, so a host whose rm is a trash-move wrapper
collected a byte-identical copy of every applied payload. Cleanup now tries an
absolute remover, then POSIX unlink, then PATH rm, and success means the
path is gone; cleanup_pending / exit 4 / remote_io evidence semantics are
unchanged. The new fixture (TestApplyCleanupDoesNotDependOnPathRm) fails
against the old helper on all three leak subtests. Separately, the privileged
apply fixture no longer pipes the script through stdin or captures output
through os/exec copier goroutines, and its evidence assertions are nil-safe,
which removes the intermittent failures and nil-pointer panics reported for
TestApplySudoScriptEvidenceAndCleanup.

#88 — plugins can be provisioned through the CLI
sshx plugin install <dir> [--replace] [--trust] stages a plugin directory with
sshx's own modes, validates it through the executor's loader before touching
the published plugin, refuses symlinks and non-regular entries, bounds the copy
(8MiB, 128 files), and can record the published digest in the same step. A
publication that cannot be renamed restores the previous plugin or reports where
the recovery copy was kept. plugin list groups built-in capabilities and local
plugins, always names the local plugin root (so "none installed" is visible),
skips staging directories left by an interrupted install, and a missing plugin
names the directory that was searched. Matrix row and E2E updated in
docs/roadmap.md / tests/e2e/inspect_plugin_e2e_test.go.

Behavior changes worth knowing

  • Compatibility mode rejects unknown options instead of forwarding them as part
    of the remote command. Callers that relied on a typo being executed (or on a
    token after the options being silently absorbed) must now use --.
  • sshx run --target=<name> uses the host's sudo_password_key; pass -pk to
    override per invocation, exactly as before.
  • sshx sql with no statement and a piped stdin reads it to EOF (like psql);
    use --statement-file when another process holds the pipe open.

Verification

  • make check (fmt, vet, full suite including the compiled-binary E2E) → pass
  • make test-e2e → pass; go test -race ./internal/... ./pkg/... → pass
  • golangci-lint run --timeout=5m → 0 issues; golangci-lint config verify → 0
  • Behavioural sweep against the built binary for each issue's original symptom
    (header ordering of --help, --quiet in first position, merged-stream
    parsing with and without --quiet, comment-leading SQL, stdin/file SQL,
    plugin install/show/list/missing-directory, apply cleanup under a fake
    trash-move rm, run sudo-key resolution in the plan and in the audit trail)
  • Docs updated in lock-step: usage text, README.md, README_CN.md,
    CHANGELOG.md [Unreleased], docs/contract.md, docs/roadmap.md, and the
    bundled Agent skill.

This branch was rebased onto main after #81 landed; the help refactor keeps
#81's new documentation blocks (policy-block mirror, non-leading sudo guidance,
text-scan monitoring) inside the new section constants, and --quiet covers the
notices #81 added.

jamiesun and others added 4 commits September 23, 2026 15:18
The Lint gate aborted inside `golangci-lint config verify` before analysing any
Go file: `.golangci.yml` kept the v1-era `issues.exclude-rules` key while
declaring `version: "2"`, and `golangci-lint-action@v7` floated on
`version: latest`, so the schema that rejected the file was whatever the runner
downloaded that day. The job has been failing since 25a45f3 (2026-09-16).

- `.golangci.yml`: move the exclusion to the v2 location
  (`linters.exclusions.rules`), keeping the same `text`/`linters` fields and the
  enabled linter set unchanged.
- `.github/workflows/ci.yml`: pin the linter to the released v2.13.2 so an
  upstream release cannot flip this gate on its own.

Verified with the pinned binary and the local one (2.12.2): `config verify`
exits 0 and `golangci-lint run --timeout=5m` reports 0 issues.

Fixes #82

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`TestApplySudoScriptEvidenceAndCleanup` failed at different assertion sites on
different runs in some environments (15/16 package runs reported): nil-pointer
panics on `*outcome.Executed`, digest mismatches, and wrong error kinds, with
`sh: line 25: printf: write error: Broken pipe` in the fixture output.

The fixture piped the generated script through the child's stdin and captured
stdout/stderr into `bytes.Buffer` values, which makes `os/exec` add pipes and
copier goroutines; a child that finishes while a copier unwinds can see
EPIPE/SIGPIPE and report a truncated privileged result instead of the behavior
under test.

- Run the script from a file (`sh <script>`) with stdout/stderr going to real
  files in the owned fixture directory, then read them back. No stdin pipe, no
  copier goroutines.
- Add `requireApplyOutcome` / `requireApplyExecuted`, which print the captured
  evidence and the parse error, and route every `*outcome.Executed` dereference
  through them, so a truncated report fails readably instead of panicking.
- Keep all nine subtests and the truncated-report manipulation; the "rename"
  subtest still asserts unknown (`nil`) publication evidence explicitly.

Verified: `go test ./internal/sshclient/ -count=20`,
`-count=5 -short -race`, and the isolated `after` subtest repeated.

Fixes #83

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`sshx apply` cleaned its staging payload, publication temp, and unverified
backup with a bare `rm -f`. On a host where `rm` resolves to a wrapper earlier
in PATH that moves files to the Trash (routinely done on macOS), every apply
leaked a byte-identical copy of the payload while the privileged report still
claimed a clean run.

- Try an absolute remover (`/bin/rm`, `/usr/bin/rm`, `/usr/local/bin/rm`), then
  POSIX `unlink`, and only then PATH `rm`, so a shim cannot absorb the artifact
  when an absolute remover exists.
- Success now means the path is gone (`[ ! -e ] && [ ! -L ]`), not that some
  remover returned 0.
- Unchanged evidence semantics: a removal that cannot complete still reports
  `cleanup_pending`, exit 4, and `status=remote_io` / "artifact cleanup failed",
  and only sshx-created paths are ever removed.

New `TestApplyCleanupDoesNotDependOnPathRm` shadows `rm` with a fake trash-mover
and covers a published apply, a failed publication temp, an unverified backup,
and an unremovable artifact. It fails against the previous helper on all three
leak subtests.

Fixes #79

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A batch of CLI-contract fixes and one provisioning feature, all on the
execution surface Agents use.

- #84 Per-verb help: every subcommand answers `sshx <verb> --help` instead of
  reporting `--help` as an unknown option, and `--help --json` emits the same
  blocks as an `sshx.help.v1` document (`sshx text --help --json` keeps its
  structured `sshx.text.help.v1` document). The help text now lives once in
  `internal/app/usage_sections.go` and is composed for both the global and the
  per-verb surface, so the two cannot drift. `--help` is answered in option
  position in any order, before any host resolution, connection, or trust-store
  write (a usage request previously connected and could record a host key).
- #85 Compatibility mode rejects an unrecognized option instead of forwarding it
  as part of the remote command, names the offending token, suggests the
  intended option when one is close, and explains the `--` separator. The
  guessed `--local`/`--remote` transfer options name the real surface
  (`--upload=<local> --to=<remote>`), a missing upload/download destination
  names `--to`, and the documented `--ssh-password-key=` is now accepted.
- #86 `--quiet` (`--no-notices`) suppresses human notices on stderr, so a caller
  that merges streams (`2>&1`) under `--json` still reads exactly one parseable
  document in both the success and the failure path. stdout, the exit code, and
  the JSON result are unchanged. Documented in docs/contract.md.
- #87 `sshx sql` takes the statement from a positional argument, after `--`,
  from `--statement-file=PATH`, or from piped stdin, and a statement that opens
  with a SQL comment is statement text rather than an "unknown option".
  Option-shaped typos are still rejected, and the SQL guard stays fail-closed:
  a statement from a file or stdin goes through the identical classifier.
- #78 `sshx run --target=` resolves the sudo keyring reference per host, exactly
  as single-target verbs do: an explicit `-pk` still wins, but the built-in
  `master` default no longer shadows a host's `sudo_password_key`. The rule is
  expressed once (`execution.SudoKeyForTarget`) and used by the plan, the SSH
  client, the keyring lookup, and the audit trail; each target's audit event
  names the credential it used, and the run summary records only a caller-level
  choice instead of misreporting the default.
- #88 `sshx plugin install <dir> [--replace] [--trust]` provisions an existing
  local plugin directory through the audited CLI: it stages the source with
  sshx's own modes, validates it through the executor's loader before touching
  the published plugin, refuses symlinks and non-regular entries, bounds the
  copy (8MiB, 128 files), and can record the published digest in the same step.
  `plugin list` groups built-in capabilities and local plugins, always names the
  local plugin root (so "none installed" is visible), reports provenance/trust/
  validity/digest, skips staging directories left by an interrupted install, and
  a missing plugin names the directory that was searched. A publication that
  cannot be renamed swaps the previous plugin back in or reports where the
  recovery copy was kept.

Docs stay in lock-step: usage text, README.md, README_CN.md,
CHANGELOG [Unreleased], docs/contract.md, docs/roadmap.md, and the bundled Agent
skill.

Verification: make check (fmt, vet, full suite including the compiled-binary
E2E), go test -race ./internal/... ./pkg/..., make test-e2e,
golangci-lint run --timeout=5m (0 issues), plus a behavioural sweep that
reproduces each issue's original symptom.

Fixes #78
Fixes #84
Fixes #85
Fixes #86
Fixes #87
Fixes #88

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jamiesun
jamiesun merged commit ade5dc0 into main Sep 23, 2026
11 checks passed
@jamiesun
jamiesun deleted the fix/issues-78-88 branch September 23, 2026 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[sudo] run --target= resolves sudo key to 'master' instead of the host's sudo_password_key; only the first sudo in a chain gets the injected password

1 participant