ssh: fix sudo failing with "password required" (v6 / 1.1.0)
The connector asked for a sudo password on every privileged call and failed whenever nobody answered it, which is every unattended run. - Always probe `sudo -n` first, even for aliases set to sudo="prompt". `_sudo_prefix` used to elicit unconditionally, so a host granting this user NOPASSWD still opened an Agent Inbox prompt; with no human there it hit the client's 300s ELICITATION_DEADLINE, got back `cancel`, and surfaced as "sudo password required (user declined or timed out)". sudo refuses before running anything when it wants a password, so the probe is side-effect free. - Strip a leading `sudo` from `command` and turn it into sudo=true. Agents write `exec(command="sudo systemctl restart x")`: with sudo=false that ran a tty-less sudo, with sudo=true it nested `sudo -S ... sudo ...` whose inner prompt had no tty either. Handles -u/-n/-S/-E/-H/-i/-k/-p/--; an unknown flag leaves the command alone. sudo_user now implies sudo=true. - Run privileged commands as `sh -c '<command>'`, so `&&`, pipes and redirections are elevated too instead of only the first word. - Add SSH_MCP_SUDO_PASSWORD (optional, secret) for unattended runs. It is consulted only after `sudo -n` proved a password is needed, so on a NOPASSWD host it never lands in the command's own stdin. - Actionable errors for every sudo failure mode, and a `hint` on a nested sudo we could not peel off. General review of the same server: - Drain stdout and stderr together and make timeout_sec a real wall-clock deadline. Both streams share one SSH channel window, so reading stdout to EOF first stalled once a chatty stderr filled it. Command stdin is now closed after the optional password. - Queue messages that arrive while awaiting an elicitation reply instead of discarding them, so a concurrent tools/call is not lost. - Record the client's `elicitation` capability at initialize and fail fast when it is absent rather than blocking on a prompt nobody can answer. - Tolerate null/string integer arguments (depth, max_results, context_lines, timeout_sec). - Realign the version across both manifests: fragment.json said 5/1.0.4 while connector.json said 2/1.0.1, so the feed was permanently ahead of the installed version and offered an update forever. Also lands the pending docs work: CONNECTOR_MANIFEST_GUIDE.md as the single source of truth, docs/connector.manifest_guide.md retired to a pointer, CLAUDE.md audited against the repo, compile.py docstring fixed, and an opencode.json config.
This commit is contained in:
@@ -7,8 +7,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- **ssh: `sudo` almost always failed with "sudo password required (user declined or timed out)" (v6 / 1.1.0).** Three independent causes, all fixed:
|
||||
- **`sudo -n` is now always tried first**, even when the alias is configured `sudo: "prompt"`. Previously `prompt` went straight to `sudo -S` and *unconditionally* elicited a password — so a host where the login user has a `NOPASSWD:` rule still opened an Agent Inbox prompt, and any unattended run (scheduled agent, no human watching the Inbox) hit the client's 300 s elicitation deadline and got back `cancel`, i.e. the reported error. A password is now requested only after `sudo -n` has proved the host actually demands one; `sudo` never runs the command when it refuses for a missing password, so the probe is side-effect free.
|
||||
- **A leading `sudo` in `command` is stripped and turned into `sudo=true`.** Agents routinely write `exec(command="sudo systemctl restart x")`. With `sudo=false` that ran a bare `sudo` on a tty-less channel ("a terminal is required"); with `sudo=true` it nested `sudo -S … sudo …`, where the *inner* sudo prompts on a tty it does not have. The prefix (with `-u USER`, `-n`, `-S`, `-E`, `-H`, `-i`, `-k`, `-p PROMPT`, `--` and friends) is now peeled off and expressed as `sudo=true` + `sudo_user`; an unrecognised flag leaves the command untouched rather than mangling it. `sudo_user` alone also implies `sudo=true`.
|
||||
- **Under sudo the command now runs as `sh -c '<command>'`**, so pipes and redirections are privileged too — `sudo tee`/`sudo … > file` behaves as written instead of running the redirection as the login user.
|
||||
- Errors are now actionable: sudo-disabled, `nopasswd`-but-password-required, password-rejected (the cached password is discarded so the next call re-prompts) and no-password-available each say what to change. A command that failed on a *nested* sudo we could not peel off (e.g. `cd /x && sudo …`) comes back with a `hint` telling the agent to use `sudo=true` instead.
|
||||
|
||||
### Added
|
||||
|
||||
- **ssh: `SSH_MCP_SUDO_PASSWORD`** — optional, secret, non-interactive sudo password for unattended runs where nobody can answer the Inbox prompt (mirrors the existing `SSH_MCP_KEY_PASSPHRASE`). Consulted only after `sudo -n` has shown the host requires a password, so on a NOPASSWD host it is never fed to sudo's stdin — where it would have landed in the command's own stdin instead.
|
||||
|
||||
### Changed
|
||||
|
||||
- **ssh: version realigned to 6 / 1.1.0 in both manifests.** `fragment.json` carried `5` / `1.0.4` while `connector.json` carried `2` / `1.0.1`. Skald takes the installed version from the manifest and the feed version from the index, so the index was permanently 3 ahead: the connector advertised an update forever and re-installed to `2` every time.
|
||||
- **ssh: stdout and stderr are drained together, and `timeout_sec` is a real wall-clock deadline.** Both streams share one SSH channel window, so reading stdout to EOF first stalled as soon as a chatty stderr filled that window (a >2 MB stderr deadlocked the call until the timeout). Command stdin is now always closed after the optional sudo password, so a remote command that reads stdin sees EOF instead of hanging.
|
||||
- **ssh: messages arriving while the server waits for an elicitation reply are queued, not dropped.** A concurrent `tools/call` used to be discarded silently, leaving the client to time out on a request the server had thrown away.
|
||||
- **ssh: the client's `elicitation` capability is recorded at `initialize`.** Without it the server no longer blocks on a prompt nobody can answer — it fails immediately and says so.
|
||||
- **ssh: tool descriptions rewritten** to tell the agent explicitly not to put `sudo` in `command`, and to describe `prompt` as "tries `sudo -n` first, prompts only if the host demands it". Integer arguments (`depth`, `max_results`, `context_lines`, `timeout_sec`) now tolerate `null` and numeric strings instead of raising an internal error.
|
||||
|
||||
- **Docs: `CONNECTOR_MANIFEST_GUIDE.md` is now the single source of truth, rewritten against the client implementation.** `~/projects/skald-circle` dropped its copy and references this file instead, so the guide was re-verified line by line against `src/frontend/api/{marketplace,mcp}.rs` and `crates/skald-core/src/mcp/{mod,install,verify,oauth}.rs`. What was wrong and is now fixed:
|
||||
- **It told the author to hand-write `connectors.json` with `sha256sum`** (§1a, §8) and never mentioned `fragment.json`, `index.json` or `scripts/compile.py` — i.e. it described a workflow this repo abandoned. The build pipeline is now §1 in full, and `connectors.json` is documented as generated.
|
||||
- **`tools[]` was said to go in `fragment.json`.** The client parses no `tools` field on the index entry — a block placed only there is inert. It must be in `connector.json` (this is why `google-trends`' four display names never reached the UI).
|
||||
- **`auth` was said to resolve "the same way whether it appears in the index entry or the manifest".** The index's `auth` is never parsed; only the manifest's is, with `requires` as the sole coarse fallback.
|
||||
- **`auth.type: "password"` was undocumented** while `email` ships it — the client recognizes only `none`/`api_key`/`oauth2`/`qr`/`ssh_key` and silently normalizes everything else to `none`.
|
||||
- **Placeholder substitution was overstated.** It happens in exactly two places (`mcp_config.url` and `verify.command`) with *different* miss behaviour (literal token + api_key fallback vs empty string), and never in `mcp_config.env` — so an `env[].name` must be the real environment variable name.
|
||||
- **`verify` was described as an `api_key`/`none` feature with a 15 s default.** Any auth type may use it; the script must be a shipped file whose basename appears in the command; the runtime applies a fixed 20 s and ignores `timeout_secs`.
|
||||
- **`deliver: {as: "file"}` was labelled "legacy"** — it is rejected at activation, unimplemented.
|
||||
- Added: the "who reads what" authority table (index vs manifest), the flat-folder constraint (`compile.py` does not recurse, so a subdirectory's files are silently unshipped), the version-desync trap (manifest wins, index-lower kills updates forever), exact dependency-install commands and paths, host assets not copied into containers, and the client's hard limits (8 MiB/file, path-safety rejects, no-`files[]` refusal). Also fixed an unbalanced code fence that swallowed the end of the file.
|
||||
- **Docs: `docs/connector.manifest_guide.md` retired** to a pointer at the root guide — the duplicate copy is what allowed the drift.
|
||||
- **Docs: `CLAUDE.md` audited against the repo.** Added § The consuming project (the client is the separate Rust repo at `~/projects/skald-circle`, with a map of the files that implement each part of the format) and corrected the same inaccuracies listed above where they also appeared here: placeholder scope and miss behaviour, verify workdir (`./connectors/<id>/` on the host, not `./scripts/<id>/`), verify timeout, `tools[]` location, `deliver: file`, `auth.type: "password"`, plus new invariants for version desync and flat folders.
|
||||
- **`scripts/compile.py`: docstring corrected** — it claimed `connector.json` was auto-excluded from `files[]`, the opposite of what `EXCLUDE_FILES` does.
|
||||
|
||||
- **Docs: `SKALD.md` merged into `CLAUDE.md` and removed.** The two files had drifted apart — `CLAUDE.md` still described a hand-maintained `connectors.json` (pre-`compile.py`), an rsync deploy, 5 connectors, and a `files[]` that excluded `connector.json`. `CLAUDE.md` is now the single authoritative spec, carrying every `SKALD.md` section (full schemas for `connectors.json` / `fragment.json` / `connector.json`, reserved enums, `auth`/`deliver`/`env`/`verify` fields, placeholder syntax, icon conventions, file integrity, local workflow, deploy) corrected against the actual repo state: 18 connectors, the `compile.py` pipeline, and `connector.json` included in the hashed `files[]`. `CLAUDE.md` now also names `CONNECTOR_MANIFEST_GUIDE.md` (repo root) as the authoritative connector-authoring spec.
|
||||
|
||||
## 2026-08-24
|
||||
|
||||
Reference in New Issue
Block a user