diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b9ab2f59..ed7e8a52 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,8 +13,8 @@ jobs: name: Package versions runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 + - uses: actions/checkout@v7 + - uses: actions/setup-node@v7 with: node-version: 22 - name: Verify application package versions agree @@ -52,29 +52,29 @@ jobs: matrix: os: [ubuntu-latest, macos-latest] steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - - run: cargo build --locked - - run: cargo test --locked - - run: cargo clippy --locked --all-targets --all-features -- -D warnings + - run: cargo build --locked --workspace + - run: cargo test --locked --workspace + - run: cargo clippy --locked --workspace --all-targets --all-features -- -D warnings fmt: name: Rustfmt runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@stable with: components: rustfmt - - run: cargo fmt --check + - run: cargo fmt --all --check viewer-ui: name: Viewer frontend runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 + - uses: actions/checkout@v7 + - uses: actions/setup-node@v7 with: node-version: 22 cache: npm diff --git a/README.md b/README.md index c0a42849..89352c4b 100644 --- a/README.md +++ b/README.md @@ -316,10 +316,12 @@ The two halves have two owners. The session writes which repositories are open a nightcrow itself knows nothing about the CLIs you run in its panes — an agent and a person get the same PTY. Behaviour that *does* need to know a particular tool lives in a plugin: a separate executable that nightcrow launches and talks -to over a pipe. Plugins are off unless you turn one on, and one can only ever -see a pane you handed it by name. +to over a pipe. Plugins are off unless you turn one on, and one only ever sees a +pane you handed it by name — or, if you also set `watch_on_signal`, a pane that +something running inside it spoke to the plugin from. A plugin is never given a +list of your panes either way. -The bundled plugin is `nightcrow-recovery`. When an opted-in pane's CLI hits its +The bundled plugin is `nightcrow-recovery`. When a watched pane's CLI hits its usage limit, it waits for the reset time the provider reported and then re-opens that exact session. It only waits — it does not bypass, raise, or work around any provider limit, and it sends nothing while a limit is in effect. Claude @@ -354,19 +356,34 @@ allowed_resume_flags = ["--resume", "resume", "--session"] [[startup_command]] name = "Claude" command = "claude" -plugin = "recovery" # without this line, no plugin ever sees this pane +plugin = "recovery" # without this line, no plugin sees this pane unless + # watch_on_signal is set (see below) ``` +That covers the panes you configured. For the pane you did not — you opened a +shell with ` t` and typed `claude` into it yourself — add +`watch_on_signal = true` to the `[[plugin]]` block. nightcrow puts a random token +in each pane's environment and nowhere else, so the CLI's own hook can quote it +back and the plugin can ask for "the pane this token names"; a plain shell never +speaks to a plugin, so your shells stay untouched. It is off by default. Such a +pane can be waited for and typed into but never relaunched — nightcrow launched +no command in it, so there is nothing to put back. + For Claude Code, let the plugin install its hook and statusline entries so it can read the exact session id and reset time instead of guessing from what is -printed on screen. It merges into your existing `~/.claude/settings.json` and -backs it up first: +printed on screen. With a reset time it waits exactly once; without one it falls +back to retrying on a backoff, which can give up. It merges into your existing +`~/.claude/settings.json` and backs it up first: ```bash nightcrow-recovery install-hooks nightcrow-recovery uninstall-hooks # removes only what it added ``` +Claude Code's `statusLine` holds one command, so installing does replace yours — +but it is then run from the plugin's own statusline with the same input, and what +it prints is what you see. `uninstall-hooks` puts it back. + A pane that is waiting shows its state and deadline on its tab. Cancel it with `` then the recovery key (see [Leader commands](#leader-commands-press-prefix-then-the-key)), or from the web viewer; typing into the pane yourself also cancels it. @@ -609,19 +626,25 @@ live_watch = true # watch expanded dirs and refresh the tree live; set f name = "Claude" # optional tab label; falls back to the command text command = "claude" # required; must not be empty plugin = "recovery" # optional; names the [[plugin]] allowed to act on this - # pane. Omitted — the default — means no plugin ever - # sees it. + # pane. Omitted — the default — means no plugin sees + # it unless that plugin sets watch_on_signal. [[startup_command]] command = "cargo test --watch" # External plugin processes — see "Plugins" above. Up to 8 entries, names unique. -# Nothing runs unless an entry exists AND enabled = true AND a pane opted in. +# Nothing runs unless an entry exists AND enabled = true AND either a pane opted +# in or watch_on_signal is set. [[plugin]] name = "recovery" # the name panes opt in with command = "nightcrow-recovery" # found on PATH or in ~/.nightcrow/plugins args = [] # passed to the plugin verbatim enabled = false # off by default +watch_on_signal = false # off by default; when true, a pane no + # [[startup_command]] named is also handed over + # once something inside it quotes that pane's + # token to this plugin. Such a pane is never + # relaunched, only typed into while it lives. allowed_resume_flags = [] # flags the plugin may append to re-open a # session; empty refuses every relaunch diff --git a/config.example.toml b/config.example.toml index be248403..0816a443 100644 --- a/config.example.toml +++ b/config.example.toml @@ -108,7 +108,8 @@ live_watch = true # watch expanded dirs and refresh the tree live; set f # section fits any provider or tool. # # Nothing here runs unless you both add an entry AND set enabled = true, and a -# plugin can only act on panes whose [[startup_command]] named it. +# plugin can only act on panes whose [[startup_command]] named it — unless you +# also set watch_on_signal, described further down. # # The bundled "recovery" plugin watches an opted-in pane for its provider's # usage-limit message, waits for the stated reset time to pass, and then resumes @@ -126,11 +127,31 @@ live_watch = true # watch expanded dirs and refresh the tree live; set f # Leave it empty (the default) and the plugin can still wait and type into a # live pane, but never relaunch one. # +# watch_on_signal covers the pane you did not configure: you opened a shell with +# t and started a coding CLI in it by hand. nightcrow gives every pane a +# random token and puts it in that pane's environment only, so the CLI's own hook +# — a child of the CLI, a grandchild of the pane — can quote it back to the +# plugin. The plugin then asks nightcrow for "the pane this token names", and +# nightcrow checks the token really is one of its panes before handing it over. +# +# What you are turning on, precisely: a pane becomes plugin-visible once +# something running inside it has spoken to that plugin. A plain shell never +# does, so your shells stay untouched — but anything you start in a pane that +# talks to this plugin is consenting on that pane's behalf. The plugin is never +# given a list of your panes and cannot ask for one it has not been told about, +# and a pane another plugin already watches is not taken away. Leave it off if +# you want the [[startup_command]] list to be the whole of what a plugin sees. +# +# A pane taken on this way can be waited for and typed into, but never +# relaunched: nightcrow never launched a command in it, so there is nothing to +# put back. allowed_resume_flags has no effect on such a pane. +# # [[plugin]] # name = "recovery" # required; the name panes opt in with # command = "nightcrow-recovery" # required; found on PATH or in the plugin dir # args = [] # passed to the plugin verbatim # enabled = false # off by default; set true to actually run it +# watch_on_signal = false # off by default; see the paragraphs above # allowed_resume_flags = ["--resume", "resume", "--session"] # # empty by default, which refuses relaunches. # # These three are what the bundled recovery diff --git a/docs/architecture.md b/docs/architecture.md index fe8fcee6..b87e5e96 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -169,6 +169,7 @@ src/ │ ├── host_pump.rs # stdin/stdout/stderr pump threads + capped line reader │ ├── guard.rs # the trust boundary: PluginCommand -> Approved | Refused │ ├── guard_budget.rs # per-slot rate ceilings, keyed by PaneToken +│ ├── guard_watch.rs # the one rule that can widen what a plugin sees │ └── registry.rs # ~/.nightcrow/plugins: install / list / remove ├── git/ │ ├── mod.rs @@ -739,19 +740,116 @@ OpenCode를 아는 코드는 `plugins/nightcrow-recovery`에 산다. 코어 `src 버전이 어긋나는 순간 UB다. cargo feature 게이트는 재컴파일을 요구하므로 "설치·제거 가능"이 아니다. 남는 것은 프로세스 경계이고, 그 편이 신뢰 모델도 정직하다 — plugin은 우리 주소 공간에 없다. 프레이밍은 stdin/stdout의 개행 구분 JSON이고 버전(`v`)이 맞지 않는 줄은 거부한다. -- **opt-in이 곧 도달 범위**: plugin은 `[[startup_command]]`이 `plugin = "이름"`으로 지목한 pane만 - 본다. 클라이언트가 직접 연 pane에는 연결이 생기지 않는다 — 임의의 셸 pane이 자동으로 감지되고 - 조작되는 일은 구조적으로 불가능하다. `[[plugin]]`은 `enabled = false`가 기본이다. +- **도달 범위의 기본은 opt-in, 확장은 증거로만**: plugin은 `[[startup_command]]`이 + `plugin = "이름"`으로 지목한 pane을 본다. 여기에 `[[plugin]]`의 `watch_on_signal`(기본 + `false`)을 켜면 두 번째 경로가 열린다 — **pane 자신의 토큰을 제시한 요청**, 즉 + `PluginCommand::WatchPane { token }`이다. 토큰은 spawn 시각에 그 pane의 자식 환경에만 들어가고 + (`pty_spawn.rs`, 명령 없이 연 pane도 예외 없이) 자식들이 상속하므로, 토큰을 말할 수 있는 것은 + 그 pane 안에서 도는 프로세스뿐이다. 근거가 열거가 아니라 증명이라는 것이 핵심이다: plugin에게 + pane 목록을 주는 경로는 여전히 없고, 맨 셸은 어떤 provider helper도 띄우지 않으므로 영원히 + 채택되지 않는다. "임의의 셸 pane이 자동으로 조작되는 일은 없다"는 성질은 pane을 숨기는 것이 + 아니라 이 증명 요구로 유지된다. `[[plugin]]`은 `enabled = false`가 기본이다. +- **왜 그 확장이 필요했나**: 실제로 압도적으로 흔한 사용은 ` t`로 셸을 열고 `claude`를 + 손으로 치는 것이다. 그 pane은 `create_pane_with(None, None)`으로 열려 launch command가 없고, + `detect(None)`은 어떤 provider도 붙이지 못한다 — 그래서 이 경우 recovery는 **아무것도** 하지 + 않았다. `WatchPane`은 그 구멍만 메운다. `PROTOCOL_VERSION`은 그래서 2가 되었고, 이 명령은 + `generation`을 싣지 않는다: 들어본 적 없는 pane에 대해 어느 spawn인지 정직하게 주장할 수 + 없으므로, 답으로 오는 `PaneOpened`가 그것을 말한다. `Plugins::start`도 그래서 조건이 둘이다 — + enabled이고 **(opt-in됐거나 `watch_on_signal`이거나)**. 후자의 pane은 앞으로 말을 걸어올 + pane이므로 host가 그보다 먼저 떠 있어야 하고, 오지 않을 opt-in을 기다리면 스위치가 아무 뜻도 + 갖지 못한다. +- **요청은 plugin 쪽에서 먼저 줄인다**(`runloop_adopt.rs`): 거부는 응답이 없는 것과 구별되지 + 않으므로, 답을 못 받은 요청이 타이트 루프가 되거나 낯선 토큰마다 상태를 남기면 안 된다. + 미해결 요청은 `MAX_PENDING`개까지만 들고(초과분은 새 것을 버려 실패를 닫힌 방향으로 낸다), + 같은 토큰은 `REQUEST_COOLDOWN` 동안 다시 묻지 않는다 — Claude Code의 statusline은 매 렌더마다 + 돌기 때문에, 이것이 없으면 남의 pane 하나가 초당 몇 번씩 명령을 써서 host의 tick당 예산을 + 정작 필요한 요청과 함께 태운다. 그리고 요청을 정당화한 **신호는 버리지 않고 들고 있다가 + `PaneOpened` 뒤에 재생한다**: 신호가 pane보다 먼저 도착하고(그 신호가 pane이 도착한 이유다) + host는 새로 넘긴 pane에 어떤 history도 재생해 주지 않으므로, 버리면 지금 복구해야 할 그 한도가 + 사라져 provider가 다시 실패할 때까지 pane이 방치된다. 이때 provider는 명령줄이 아니라 + `detect_from_signal`이 고른다 — `SignalKind`는 정확히 한 adapter의 helper만 발행하므로 신호 + 종류 자체가 무엇이 돌고 있는지에 대한 증거이고, 그래서 두 번째 sniffing 경로가 아니라 wire + kind에 대한 lookup이다. +- **늦게 채택된 pane은 relaunch되지 않는다**: launch command가 `None`이므로 프로세스를 되돌려 + 놓으면 provider가 아니라 셸이 다시 뜬다. guard는 이것을 `Refused::NoLaunchCommand`로 — + 인자 문제와 구별되는 자기 이유로 — 거부하고, `allowed_resume_flags`를 어떻게 열어도 통과하지 + 않는다. hub도 같은 판단을 한다: watched pane이 종료했을 때 `is_relaunchable`이 거짓이면 + `PENDING_RELAUNCH_TTL` 동안 slot을 붙잡는 대신 곧바로 닫는 경로를 탄다 — 되돌릴 것이 없는 + slot을 9일 붙잡을 이유가 없다. 이런 pane이 받을 수 있는 recovery는 살아 있는 프로세스에 + 타이핑하는 것 하나뿐이고, plugin 쪽도 같은 결론을 미리 내려 `NeedsAttention`으로 간다 + (`state_resume.rs`). - **신뢰 경계는 `guard.rs` 하나다**: `protocol::decode_command`는 모양과 크기만 본다. 권한은 `Guard::judge`만 판단하고, plugin이 우회할 경로가 없다. 규칙: pane이 존재하고 opt-in했는가, `generation`이 현재와 같은가(이것이 교체된 프로세스에 대한 결정이 후임에게 닿는 것을 막는다), - 살아 있고 조용할 때만 입력을 넣는가, 죽었을 때만 relaunch하는가, 제어문자가 섞이지 않았는가, - slot당 횟수 상한 안인가. 거부는 로그로 남고 재시도되지 않는다. + 살아 있고 조용할 때만 입력을 넣는가, 죽었을 때만 relaunch하는가, 되돌릴 명령이 있는가, + 제어문자가 섞이지 않았는가, slot당 횟수 상한 안인가. 거부는 로그로 남고 재시도되지 않는다. +- **pane을 얻는 규칙만 따로 산다**(`guard_watch.rs`): 나머지 규칙이 모두 "이미 배정된 pane"에서 + 출발하는 데 반해 이것은 배정 자체를 만드는 유일한 자리라, 큰 판단 안의 분기가 아니라 조건 + 목록 하나로 읽히게 분리했다. 순서대로 — 토큰이 아는 pane인가, `watch_on_signal`이 켜졌는가, + 다른 plugin이 이미 보고 있지 않은가(pane 하나에 watcher 하나. 둘이 같은 키보드를 몰면 서로가 + 바꾸는 상태 위에서 recovery가 섞인다), 프로세스가 살아 있는가. 예산은 청구하지 않는다 — + pane을 받는 것은 pane에 하는 일이 아니고, 이어질 행위는 각각 청구되므로 여기서 세면 곧 쓸 + allowance를 미리 태우게 된다. 이미 자기 것인 pane을 다시 물으면 **거부가 아니라 승인**이다: + 명령줄로는 안에 있는 것을 알아볼 수 없었던 opt-in pane이 다시 시도할 유일한 방법이 + `PaneOpened`를 한 번 더 받는 것이기 때문이다. 알 수 없는 토큰이 압도적 다수라는 것도 이 + 설계의 전제다 — 같은 사용자의 다른 nightcrow 세션 pane들이 같은 소켓에 닿는다. - **`PaneToken`이 정체성인 이유**: `PaneId`는 backend별 카운터라 backend가 다시 만들어지면 1로 돌아간다. 프로세스 밖에서 pane을 가리키기에 부적합하고, cwd도 답이 못 된다 — 한 저장소에 여러 pane을 두는 것이 지원되는 레이아웃이다. 그래서 난수 토큰을 spawn 시각에 자식 환경 (`NIGHTCROW_PANE_TOKEN`)으로 넣는다. provider가 띄우는 hook/statusline 자식들이 이를 상속하므로, plugin은 어떤 pane에서 온 사건인지 추측 없이 안다. +- **provider의 설정 파일은 병합만 한다**(`hooks.rs` / `hooks_merge.rs`): `~/.claude/settings.json`은 + 사용자 것이고 우리가 모르는 키와 hook event를 담고 있을 수 있으므로, 모든 수정은 우리가 넣지 + 않은 것을 보존하는 병합이고, 파일을 이해할 수 없으면(JSON이 아니거나 top-level이 object가 + 아니면) 추측하는 대신 멈춘다. 쓰기는 같은 디렉터리의 temp file → rename이고 모드 `0600`은 + rename 전에 건다(대상이 잠깐이라도 world-readable이 되지 않도록), 첫 쓰기 전에 `.bak`을 + 남긴다. 등록하는 hook event는 정확히 하나다 — `HOOK_EVENT = "StopFailure"`, + `HOOK_MATCHER = "rate_limit"` 아래 + `{"type":"command","command":" hook","timeout":5}`. 최소 권한이라서 그렇다: + `authentication_failed`·`billing_error` 같은 무관한 실패의 payload는 이 프로세스에 아예 + 도달하지 않고, 그 대가로 일시적 `overloaded`/`server_error`는 pane 출력에서 알아본다. + `statusLine`도 같은 자리에서 등록한다. 우리 엔트리를 알아보는 표시는 `command` 문자열에 + `MARKER`가 들어 있는지 하나뿐이다 — provider의 스키마에서 자유 텍스트를 넣을 수 있는 필드가 + 거기뿐이고, 우리 마음대로 만든 키는 provider가 unknown으로 거부하거나 경고할 수 있다. 그래서 + install은 `current_exe()`로 해석한 절대 경로가 `MARKER`를 담지 않으면 **거부한다**: 나중에 + uninstall이 자기 엔트리를 알아볼 수 없게 되기 때문이다. 경로를 `argv[0]`이 아니라 해석해서 + 쓰는 이유는 그 파일을 읽는 것이 작업 디렉터리가 다른 다른 프로세스라는 것이다. +- **helper는 provider의 임계 경로에 있으므로 최소한만 한다**(`helper.rs`): 등록되는 명령은 이 + plugin의 바이너리를 내부 서브커맨드로 다시 부르는 것(`main.rs`의 `Mode::Hook` / + `Mode::Statusline`)이다. `hook()`은 stdin을 상한까지만 읽고 + `["session_id","error_type","hook_event_name"]`만 통과시킨다 — whitelisting이 프라이버시 + 경계다. `StopFailure` payload는 transcript 파일 경로와 provider의 에러 산문을 담으므로, + 상태 기계가 실제로 읽는 필드만 소켓을 건넌다. pane은 `NIGHTCROW_PANE_TOKEN`에서 읽고, 한 줄을 + unix socket으로 보내고 끝난다. 어느 실패도 호출자에게 보고하지 않는다 — 돌지 않는 recovery + plugin은 설치되지 않은 것과 정확히 같아 보여야 한다. +- **IPC 랑데부는 경로 규칙 하나다**(`ipc.rs`): `$XDG_RUNTIME_DIR/nightcrow/recovery.sock`, + 없으면 `~/.nightcrow/run/recovery.sock`. 디렉터리는 `0700`, 소켓은 `0600`이고 bind마다 다시 + 건다. 남아 있는 소켓 파일은 **아무도 듣고 있지 않을 때만** unlink한다(살아 있는 listener를 + 가로채지 않기 위해). `parse_line`은 줄 크기, JSON object 여부, `v` 일치, 토큰의 문자 집합과 + 길이, 아는 `kind`, object payload를 모두 검사하고 실패마다 무엇이 틀렸는지 말한다 — + 여기가 untrusted input이 상태가 되는 경계이므로 조용히 강제 변환하는 필드가 곧 버그다. + **토큰은 correlation key이고 authorisation이 아니다**: 소켓에 닿을 수 있는 것은 아무 pane이나 + 주장할 수 있고, 위조된 메시지가 할 수 있는 최대는 이 plugin이 host에게 무언가를 묻게 만드는 + 것이며 그것은 guard가 처음부터 다시 판단한다. +- **statusline은 가로채지 않고 이어붙인다**(`helper_statusline.rs` / `helper_delegate.rs`): + `statusLine`은 목록이 아니라 명령 하나라 install은 사용자 것을 반드시 밀어낸다. 예전에는 + 거기서 끝나 사용자가 자기 statusline을 잃었다. 지금은 `helper::statusline()`이 pass-through다 — + stdin 바이트를 **그대로** 보관하고, 사본만 파싱해 `rate_limits`를 IPC로 넘기고, install이 + sidecar에 기록해 둔 밀려난 명령을 그 원본 바이트를 stdin으로 주어 실행한 뒤 그 stdout을 + 출력한다. 재직렬화하지 않는 이유는 키 순서와 숫자 표기가 provider의 것이고, 우리가 생기기 + 전부터 그 입력을 읽던 명령이 재배열된 것을 보면 안 되기 때문이다. 실행은 `sh -c`로 한다 — + Claude Code가 `statusLine` 명령은 셸에서 돈다고 문서화하고 자기 예시가 `~`, `jq` 파이프, + 인라인 `$(...)`에 의존하므로 우리가 argv로 쪼개면 사용자가 쓴 뜻이 조용히 바뀐다. `$SHELL`이 + 아니라 `sh`인 것은 대화형 셸이면 refresh마다 rc 파일을 읽기 때문이다. 예산은 2초이고 넘기면 + 죽이고 우리 줄로 떨어진다 — provider는 statusline에 timeout을 문서화하지 않았지만 300ms로 + debounce하고 다음 갱신이 오면 진행 중 스크립트를 취소하므로, 이 상한은 반대 방향(끝나지 않는 + 명령이 이 프로세스를 불멸로 만들지 않게)을 위한 것이다. stderr는 버린다(로그용 경고가 + statusline으로 렌더링되면 안 된다). sidecar에 든 것이 우리 자신의 바이너리면 다시 실행하지 + 않는다(install/uninstall이 쓰는 `is_ours`를 그대로 재사용하므로, 거부되는 것이 그 둘이 자기 + 것으로 아는 것과 정확히 같다). 모든 실패 경로는 plugin 자신의 줄로 격하된다 — 에러를 띄우는 + statusline은 평범한 statusline보다 나쁘다. 비자명한 함정 하나: 밀어낼 `statusLine`이 애초에 + 없었으면 `merge_into`가 `Some(Value::Null)`을 돌려주므로 **sidecar가 `null`을 담을 수 있다**. + 없음(sidecar 없음/읽기 실패)만이 빈 경우가 아니고, `null`도 "실행할 것이 없다"로 읽어야 한다. - **횟수 상한은 slot(토큰) 기준으로 센다**: relaunch는 반드시 새 `PaneId`를 만든다. 상한을 id로 세면 relaunch마다 예산이 새로 생겨서, 즉시 끝나는 명령과 매 종료마다 relaunch하는 plugin이 만나면 상한에 영원히 닿지 않는다. 토큰은 relaunch를 건너 살아남는 유일한 값이라 상한이 @@ -772,6 +870,17 @@ OpenCode를 아는 코드는 `plugins/nightcrow-recovery`에 산다. 코어 `src 걸친 escape는 완전히 제거되지 않는다. 이것이 허용되는 이유는 출력 텍스트가 언제나 fallback 신호일 뿐이라는 것이다 — Claude는 hook과 statusline, Codex는 rollout JSONL, OpenCode는 로컬 서버의 세션 상태가 1차 신호다. +- **신호의 역할은 분리돼 있고, 이것이 하중을 받는 사실이다**(`provider/claude.rs`): 한도를 + **선언**할 수 있는 것은 `StopFailure`(`on_stop_failure`)와 출력 fallback뿐이다. statusline은 + 정확한 reset epoch만 공급하고 결코 선언하지 않는다 — `on_rate_limits`는 `resets_at`만 기억하고 + `used_percentage`는 100이어도 의도적으로 무시한다(꽉 찬 창은 한도를 뒷받침하지만 선언하지는 + 않는다). 여러 창이 보고되면 가장 이른 것이 유용한 deadline이다. 이 분리의 결과가 + `state_clock.rs`의 `arm_wait`에서 갈린다: `LimitKind::UsageLimit`이고 `resets_at`이 알려져 + 있으면 `WaitingForReset`으로 **정확히 한 번** 기다리고 resume attempt를 쓰지 않는다(아직 아무 + 것도 시도하지 않았으므로). 모르면 `arm_backoff`로 떨어지고, 그쪽은 attempt 예산에 묶인 + 재시도 루프라 `MAX_RESUME_ATTEMPTS`에 닿으면 `NeedsAttention`으로 끝난다. 그래서 hook과 + statusline을 둘 다 설치하는 것의 실질적 이득은 "감지"가 아니라 **기다림이 정확해지고 예산을 + 쓰지 않는다**는 것이다. - **OpenCode에는 개입하지 않는다**: 자체 재시도가 상한 없이 계속되므로 "재시도 소진"을 기다리는 설계가 성립하지 않는다. 프로세스가 끝났거나 상태가 `idle`로 바뀐 뒤에만 손을 댄다. - **와이어 계약이 두 벌 있다**: plugin은 독립 빌드라 `plugins/nightcrow-recovery`가 프로토콜 diff --git a/plugins/nightcrow-recovery/src/helper.rs b/plugins/nightcrow-recovery/src/helper.rs index fd66b7cd..05324ebd 100644 --- a/plugins/nightcrow-recovery/src/helper.rs +++ b/plugins/nightcrow-recovery/src/helper.rs @@ -12,6 +12,11 @@ //! carries whatever else the provider decided to include. Only the fields the //! state machine actually reads cross the socket, so nothing else can be //! accidentally logged, buffered, or written down later. +//! +//! The one thing that leaves this process whole is the statusline payload handed +//! to the command we displaced (see [`status_line`]) — and that command was being +//! given the same bytes by Claude Code before this plugin was installed. We +//! narrow what we keep; we do not narrow what someone else was already told. use crate::ipc::{IpcMessage, send, socket_path}; use crate::protocol::PANE_TOKEN_ENV; @@ -19,6 +24,10 @@ use crate::provider::SignalKind; use serde_json::{Map, Value}; use std::io::Read; use std::process::ExitCode; +use std::time::Duration; + +#[path = "helper_statusline.rs"] +mod status_line; /// Most stdin a helper will read. /// @@ -35,16 +44,11 @@ const STOP_FAILURE_FIELDS: [&str; 3] = ["session_id", "error_type", "hook_event_ /// The only statusline field this plugin wants. const RATE_LIMITS_FIELD: &str = "rate_limits"; -/// Shown when the statusline payload carries no usage numbers — which is normal: -/// `rate_limits` is absent for accounts without a subscription window and before -/// the session's first response. -const STATUSLINE_FALLBACK: &str = "nightcrow: watching"; - /// Forward a `StopFailure` payload. Always succeeds from the caller's point of /// view; `StopFailure` ignores our exit code anyway, and a hook that fails /// loudly would be worse than one that does nothing. pub fn hook() -> ExitCode { - if let Some((token, payload)) = read_stdin_object().and_then(|body| { + if let Some((token, payload)) = parse_object(&read_stdin_bytes()).and_then(|body| { let token = pane_token()?; Some((token, pick(&body, &STOP_FAILURE_FIELDS))) }) { @@ -60,16 +64,15 @@ pub fn hook() -> ExitCode { ExitCode::SUCCESS } -/// Forward the statusline's `rate_limits` and print a line, so installing this -/// plugin does not cost the user their statusline. +/// Forward the statusline's `rate_limits` and print a line — the line the user's +/// own statusline command printed, whenever installing this plugin displaced one. +/// Claude Code's `statusLine` holds a single command, so the only way not to cost +/// the user their statusline is to run it from ours; see [`status_line`]. pub fn statusline() -> ExitCode { - let body = read_stdin_object(); - let rate_limits = body - .as_ref() - .and_then(|b| b.get(RATE_LIMITS_FIELD)) - .and_then(Value::as_object) - .cloned(); - if let (Some(token), Some(limits)) = (pane_token(), rate_limits.clone()) { + let raw = read_stdin_bytes(); + let displaced = status_line::displaced(); + let refresh = refresh(&raw, displaced.as_ref(), status_line::BUDGET); + if let (Some(token), Some(limits)) = (pane_token(), refresh.rate_limits) { let _ = send( &socket_path().unwrap_or_default(), &IpcMessage { @@ -79,30 +82,30 @@ pub fn statusline() -> ExitCode { }, ); } - println!("{}", render_statusline(rate_limits.as_ref())); + println!("{}", refresh.line); ExitCode::SUCCESS } -/// A short line built only from fields whose meaning is documented: the usage -/// percentage of each window the provider reported. -fn render_statusline(rate_limits: Option<&Map>) -> String { - let Some(limits) = rate_limits else { - return STATUSLINE_FALLBACK.to_string(); - }; - let mut parts = Vec::new(); - for (label, key) in [("5h", "five_hour"), ("7d", "seven_day")] { - if let Some(used) = limits - .get(key) - .and_then(|w| w.get("used_percentage")) - .and_then(Value::as_f64) - { - parts.push(format!("{label} {}%", used.round() as i64)); - } - } - if parts.is_empty() { - return STATUSLINE_FALLBACK.to_string(); - } - parts.join(" | ") +/// What one statusline refresh comes to: the usage numbers to forward, and the +/// line to print. +struct Refresh { + rate_limits: Option>, + line: String, +} + +/// Decide both halves of a refresh without touching the socket or stdout, so +/// every way a displaced command can disappoint us stays testable — and so the +/// usage numbers are read out of the payload before anything is delegated, which +/// is what keeps a misbehaving statusline command from costing us them. +fn refresh(raw: &[u8], displaced: Option<&Value>, budget: Duration) -> Refresh { + let rate_limits = parse_object(raw).and_then(rate_limits_of); + let line = status_line::line(displaced, raw, rate_limits.as_ref(), budget); + Refresh { rate_limits, line } +} + +/// The usage windows of a statusline payload, when it reported any. +fn rate_limits_of(body: Map) -> Option> { + body.get(RATE_LIMITS_FIELD)?.as_object().cloned() } /// The pane this helper belongs to, from the environment its provider inherited. @@ -114,13 +117,24 @@ fn pane_token() -> Option { .filter(|t| !t.trim().is_empty()) } -fn read_stdin_object() -> Option> { - let mut body = String::new(); - std::io::stdin() +/// Every byte the provider wrote, kept exactly as it wrote them. The statusline +/// helper hands these on to the command it displaced, and a re-serialised copy is +/// not the same thing: key order and number formatting are the provider's to +/// choose, and a command that was reading its input before we existed should not +/// find it rearranged now. A read that fails part-way keeps what did arrive — +/// unparseable, and treated as such below. +fn read_stdin_bytes() -> Vec { + let mut raw = Vec::new(); + let _ = std::io::stdin() .take(MAX_HELPER_STDIN_BYTES) - .read_to_string(&mut body) - .ok()?; - match serde_json::from_str::(&body) { + .read_to_end(&mut raw); + raw +} + +/// The payload as an object, when that is what it is. Anything else is not an +/// error here: there is simply nothing of ours to forward out of it. +fn parse_object(raw: &[u8]) -> Option> { + match serde_json::from_slice::(raw) { Ok(Value::Object(map)) => Some(map), _ => None, } diff --git a/plugins/nightcrow-recovery/src/helper_delegate.rs b/plugins/nightcrow-recovery/src/helper_delegate.rs new file mode 100644 index 00000000..28db083b --- /dev/null +++ b/plugins/nightcrow-recovery/src/helper_delegate.rs @@ -0,0 +1,116 @@ +//! Running a statusline command that is not ours, on a budget. +//! +//! Split out of `helper_statusline.rs` so that file decides *which* line gets +//! printed and this one is the process plumbing under it. Everything here is +//! written for a caller that must not be made to wait and must not be made to +//! fail: the child is bounded, killed when it overruns, reaped on every path, and +//! any disappointment comes back as `None`. + +use std::io::{Read, Write}; +use std::process::{Child, Command, Stdio}; +use std::sync::mpsc; +use std::time::{Duration, Instant}; + +/// A POSIX shell, resolved on `PATH`. Not `$SHELL`: an interactive shell would +/// read the user's rc files on every single refresh. +const SHELL: &str = "sh"; + +/// Most stdout to take from a displaced command. A statusline is one short line; +/// this only stops a runaway script from growing this process. +const MAX_DELEGATED_STDOUT_BYTES: u64 = 64 * 1024; + +/// How often to look for a child that has closed its stdout but not yet exited. +const EXIT_POLL: Duration = Duration::from_millis(2); + +/// Run `command` with `raw` on its stdin and bring back what it printed, or +/// `None` if it could not be started, did not end well, or overran `budget`. +/// +/// Through `sh -c`, not an argv we split ourselves: the provider documents that a +/// `statusLine` command "runs in a shell", and its own examples rely on it — a `~` +/// path, a `jq` pipeline, an inline `$(...)`. Re-splitting the string the user wrote +/// would quietly change what it means. This is the user's own configuration rather +/// than input from a stranger, but it is also not ours to reinterpret. +pub(super) fn capture(command: &str, raw: &[u8], budget: Duration) -> Option { + let deadline = Instant::now() + budget; + let mut child = Command::new(SHELL) + .arg("-c") + .arg(command) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + // Claude Code reads our stdout for the statusline, and a chatty script's + // stderr shares the terminal with it. Discarded, so a warning meant for a + // log cannot end up rendered as the statusline. + .stderr(Stdio::null()) + .spawn() + .ok()?; + + // Each pipe gets its own thread. Writing the payload first would deadlock + // against a command that answers before draining its input, and reading first + // would wedge a command that is still waiting for the rest of its input. + if let Some(mut stdin) = child.stdin.take() { + let payload = raw.to_vec(); + std::thread::spawn(move || { + // A command that ignores its input closes the pipe early. That is a + // choice it is allowed to make, not a failure of ours. + let _ = stdin.write_all(&payload); + }); + } + let Some(stdout) = child.stdout.take() else { + return abandon(child); + }; + let (tx, rx) = mpsc::channel(); + std::thread::spawn(move || { + let mut captured = Vec::new(); + let _ = stdout + .take(MAX_DELEGATED_STDOUT_BYTES) + .read_to_end(&mut captured); + let _ = tx.send(captured); + }); + + let Ok(captured) = rx.recv_timeout(remaining(deadline)) else { + return abandon(child); + }; + if !exited_well(&mut child, deadline) { + return abandon(child); + } + // Output we cannot decode is output we cannot print; the caller has a line of + // its own for that. + let text = String::from_utf8(captured).ok()?; + // The child owns the content of the line; we own its framing, and the caller + // is the one that ends it with a newline. A command that printed nothing but + // whitespace did not render a statusline at all. + let printed = text.trim_end_matches(['\r', '\n']); + if printed.trim().is_empty() { + return None; + } + Some(printed.to_string()) +} + +/// Whether the child finished, and finished happily, before `deadline`. +/// +/// Polled rather than waited on: `wait` has no timeout, and a command that closes +/// its stdout and then sleeps must not get to hold a refresh open. Stdout is +/// already at EOF by the time this is called, so the first look nearly always +/// finds the child gone. +fn exited_well(child: &mut Child, deadline: Instant) -> bool { + loop { + match child.try_wait() { + Ok(Some(status)) => return status.success(), + Ok(None) if Instant::now() < deadline => std::thread::sleep(EXIT_POLL), + _ => return false, + } + } +} + +/// A child we are done waiting for. Killed so a wedged statusline command does not +/// outlive the refresh that started it, and reaped so it does not sit as a zombie +/// for whatever is left of this process's life. +fn abandon(mut child: Child) -> Option { + let _ = child.kill(); + let _ = child.wait(); + None +} + +fn remaining(deadline: Instant) -> Duration { + deadline.saturating_duration_since(Instant::now()) +} diff --git a/plugins/nightcrow-recovery/src/helper_statusline.rs b/plugins/nightcrow-recovery/src/helper_statusline.rs new file mode 100644 index 00000000..a432e015 --- /dev/null +++ b/plugins/nightcrow-recovery/src/helper_statusline.rs @@ -0,0 +1,128 @@ +//! The one line Claude Code renders, and who gets to write it. +//! +//! Installing this plugin necessarily takes the user's statusline away: +//! `statusLine` in `settings.json` holds one command, not a list, so ours replaces +//! whatever was there. Chaining is the only way to give it back — install recorded +//! the value it displaced in a sidecar, and every refresh runs that command with +//! the very bytes Claude Code sent us and prints what it printed. This plugin's own +//! two-number line ([`render_statusline`]) stands in only when there is nothing to +//! chain to. Running the command itself is [`delegate`]'s job. +//! +//! Nothing here fails upwards. A statusline that shows an error is worse than a +//! plain one, so a missing sidecar, a value we cannot execute, a spawn failure, a +//! non-zero exit, a wedged child and non-UTF-8 output all end in the same place: +//! our own line, printed as if no chaining had been attempted. + +use crate::hooks::{SettingsPaths, displaced_statusline, is_ours}; +use serde_json::{Map, Value}; +use std::time::Duration; + +#[path = "helper_delegate.rs"] +mod delegate; + +/// Shown when the statusline payload carries no usage numbers — which is normal: +/// `rate_limits` is absent for accounts without a subscription window and before +/// the session's first response. +const STATUSLINE_FALLBACK: &str = "nightcrow: watching"; + +/// How long a displaced statusline command may take before we give up on it. +/// +/// Claude Code documents no timeout for a statusline: it debounces updates at 300ms +/// and cancels an in-flight script when the next update arrives, so the provider is +/// already the one deciding we took too long. This bound is for the other direction +/// — a command that never returns must not make this process immortal, and our own +/// line has to get printed either way. Two seconds is many times that debounce and +/// generous even for the `git`-shelling scripts the provider's own guidance calls +/// slow, while keeping a wedged child's cost finite. It is also inside the five +/// seconds this plugin asks Claude Code to allow its hook, the most patience +/// anything here claims of the provider. +pub(super) const BUDGET: Duration = Duration::from_secs(2); + +const TYPE_KEY: &str = "type"; +const COMMAND_KEY: &str = "command"; +/// The only `type` of `statusLine` entry there is anything for us to run. +const COMMAND_TYPE: &str = "command"; + +/// The `statusLine` install displaced, when there is one to chain to. Every way +/// there can be nothing — no `HOME` to look under, no sidecar because we displaced +/// nothing, a sidecar we cannot read — reads the same from here. +pub(super) fn displaced() -> Option { + displaced_statusline(&SettingsPaths::discover().ok()?) +} + +/// The line to print for this refresh: the displaced command's, when there is one +/// that can produce it, and ours otherwise. +pub(super) fn line( + displaced: Option<&Value>, + raw: &[u8], + rate_limits: Option<&Map>, + budget: Duration, +) -> String { + delegated(displaced, raw, budget).unwrap_or_else(|| render_statusline(rate_limits)) +} + +fn delegated(displaced: Option<&Value>, raw: &[u8], budget: Duration) -> Option { + let command = command_of(displaced?)?; + // Our own command in the sidecar would be a chain that runs this binary from + // itself, and again from there. `is_ours` is the same substring test install and + // uninstall recognise our entries by, so what is refused here is exactly what + // those two already consider ours. + if is_ours(command) { + return None; + } + delegate::capture(command, raw, budget) +} + +/// The command string inside whatever Claude Code allowed as a `statusLine`. +/// +/// Install recorded that value verbatim, so this reads the shape the provider +/// documents and the one this plugin itself writes — an object with `type` and +/// `command` — and also accepts a bare string, which costs nothing and is the +/// obvious hand-written form. A value with some other `type` is a statusline we do +/// not know how to run, and guessing at it is worse than standing in for it; so is +/// `null`, which is what install records when it displaced nothing at all. +/// +/// The entry's other fields are Claude Code's to act on, not ours: `padding` and +/// `refreshInterval` describe how the provider treats a statusline, and the +/// provider is reading them off our entry now, not off this one. +fn command_of(value: &Value) -> Option<&str> { + let command = match value { + Value::String(command) => command.as_str(), + Value::Object(map) => { + let declared = map.get(TYPE_KEY).and_then(Value::as_str); + if declared.is_some_and(|kind| kind != COMMAND_TYPE) { + return None; + } + map.get(COMMAND_KEY)?.as_str()? + } + _ => return None, + }; + let command = command.trim(); + (!command.is_empty()).then_some(command) +} + +/// A short line built only from fields whose meaning is documented: the usage +/// percentage of each window the provider reported. +fn render_statusline(rate_limits: Option<&Map>) -> String { + let Some(limits) = rate_limits else { + return STATUSLINE_FALLBACK.to_string(); + }; + let mut parts = Vec::new(); + for (label, key) in [("5h", "five_hour"), ("7d", "seven_day")] { + if let Some(used) = limits + .get(key) + .and_then(|w| w.get("used_percentage")) + .and_then(Value::as_f64) + { + parts.push(format!("{label} {}%", used.round() as i64)); + } + } + if parts.is_empty() { + return STATUSLINE_FALLBACK.to_string(); + } + parts.join(" | ") +} + +#[cfg(test)] +#[path = "helper_statusline_tests.rs"] +mod tests; diff --git a/plugins/nightcrow-recovery/src/helper_statusline_tests.rs b/plugins/nightcrow-recovery/src/helper_statusline_tests.rs new file mode 100644 index 00000000..b1d362ab --- /dev/null +++ b/plugins/nightcrow-recovery/src/helper_statusline_tests.rs @@ -0,0 +1,192 @@ +//! Who writes the line, and what happens when the displaced command will not. +//! +//! These tests really do spawn shells, which is the point of them: how a +//! statusline command behaves towards us — printing, failing, hanging, shouting on +//! stderr — is only worth pinning if a real one gets to do it. + +use super::*; +use std::time::Instant; + +/// A budget no test in here is meant to reach. Only the wedged case waits. +const ENOUGH: Duration = Duration::from_secs(5); + +/// The budget for a command that will never finish. Short enough not to be felt, +/// long enough that expiry is a decision rather than a lost race with `sh`. +const BRIEF: Duration = Duration::from_millis(150); + +/// What `render_statusline` makes of [`limits`], so `OURS` in an assertion reads +/// as "the chain was declined or came to nothing". +const OURS: &str = "5h 40%"; + +/// The shape this plugin writes into `statusLine`, and so the shape it records +/// when it displaces one. +fn entry(command: &str) -> Value { + serde_json::json!({ "type": "command", "command": command, "padding": 2 }) +} + +fn limits() -> Map { + match serde_json::json!({"five_hour": {"used_percentage": 40.0}}) { + Value::Object(map) => map, + _ => unreachable!("the fixture is an object"), + } +} + +/// What a refresh prints when install recorded `displaced`, with usage numbers +/// always there to fall back on. +fn rendered(displaced: &Value, raw: &[u8], budget: Duration) -> String { + line(Some(displaced), raw, Some(&limits()), budget) +} + +#[test] +fn the_displaced_commands_own_line_becomes_our_line() { + let displaced = entry("echo 'hud | main | 12%'"); + + let printed = rendered(&displaced, b"{}", ENOUGH); + + assert_eq!(printed, "hud | main | 12%"); +} + +#[test] +fn the_bytes_claude_code_sent_reach_the_displaced_command_unchanged() { + // Key order, number formatting and string escapes are all the provider's, and + // a parsed and re-encoded copy would keep none of the three. + let raw = br#"{"zeta":1,"alpha":2.50,"big":1e3,"who":"a\/b"}"#; + + let printed = rendered(&entry("cat"), raw, ENOUGH); + + assert_eq!(printed.as_bytes(), raw); +} + +#[test] +fn a_displaced_statusline_recorded_as_a_bare_string_is_run_too() { + let displaced = Value::String("echo theirs".to_string()); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), "theirs"); +} + +#[test] +fn a_multi_line_displaced_statusline_keeps_its_own_line_breaks() { + let displaced = entry("printf 'top\\nbottom\\n'"); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), "top\nbottom"); +} + +#[test] +fn with_nothing_displaced_our_own_line_is_printed() { + let numbers = limits(); + + assert_eq!(line(None, b"{}", Some(&numbers), ENOUGH), OURS); + // A JSON null is what install records when it displaced no statusline at all. + assert_eq!(rendered(&Value::Null, b"{}", ENOUGH), OURS); + assert_eq!(line(None, b"{}", None, ENOUGH), STATUSLINE_FALLBACK); +} + +#[test] +fn a_displaced_command_that_fails_falls_back_however_much_it_printed() { + let displaced = entry("echo half-a-line; exit 3"); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), OURS); +} + +#[test] +fn a_displaced_command_that_cannot_be_run_falls_back() { + let displaced = entry("/nonexistent/statusline-c0ffee --now"); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), OURS); +} + +#[test] +fn a_displaced_command_that_prints_nothing_usable_falls_back() { + for silent in ["true", "printf '\\n\\n'", "printf ' '"] { + let printed = rendered(&entry(silent), b"{}", ENOUGH); + + assert_eq!(printed, OURS, "{silent}"); + } +} + +#[test] +fn a_displaced_commands_stderr_never_reaches_the_statusline() { + let displaced = entry("echo noise >&2; echo theirs"); + + assert_eq!(rendered(&displaced, b"{}", ENOUGH), "theirs"); +} + +#[test] +fn a_displaced_command_that_never_finishes_is_given_up_on_and_falls_back() { + let displaced = entry("sleep 30"); + let started = Instant::now(); + + let printed = rendered(&displaced, b"{}", BRIEF); + + assert_eq!(printed, OURS); + assert!( + started.elapsed() < Duration::from_secs(5), + "the refresh waited {:?} on a command it had given up on", + started.elapsed() + ); +} + +#[test] +fn our_own_command_is_never_run_from_our_own_statusline() { + // Both of these would print if they ran, and both carry the marker install + // and uninstall recognise us by, so neither may: a sidecar naming this binary + // would otherwise chain the statusline into itself. + for ours in [ + "echo nightcrow-recovery statusline", + "/opt/nightcrow/libexec/nightcrow-recovery statusline || echo theirs", + ] { + let printed = rendered(&entry(ours), b"{}", ENOUGH); + + assert_eq!(printed, OURS, "{ours}"); + } +} + +#[test] +fn a_displaced_value_we_cannot_execute_falls_back_rather_than_guessing() { + // The first is the case that matters: a future `type` may mean something + // entirely unlike a shell command, so its `command` is not ours to run. + for unrunnable in [ + serde_json::json!({"type": "some-future-kind", "command": "echo theirs"}), + serde_json::json!({"type": "command", "command": 42}), + serde_json::json!({"type": "command"}), + serde_json::json!({"command": " "}), + serde_json::json!([{"command": "echo theirs"}]), + serde_json::json!(7), + ] { + let printed = rendered(&unrunnable, b"{}", ENOUGH); + + assert_eq!(printed, OURS, "{unrunnable}"); + } +} + +#[test] +fn a_statusline_reports_the_usage_of_every_window_the_provider_gave() { + let limits = match serde_json::json!({ + "five_hour": {"used_percentage": 23.5, "resets_at": 1_767_225_600i64}, + "seven_day": {"used_percentage": 41.2, "resets_at": 1_767_657_600i64} + }) { + Value::Object(map) => map, + _ => unreachable!("the fixture is an object"), + }; + assert_eq!(render_statusline(Some(&limits)), "5h 24% | 7d 41%"); +} + +#[test] +fn a_statusline_with_one_window_reports_only_that_window() { + let limits = match serde_json::json!({"seven_day": {"used_percentage": 8.0}}) { + Value::Object(map) => map, + _ => unreachable!("the fixture is an object"), + }; + assert_eq!(render_statusline(Some(&limits)), "7d 8%"); +} + +#[test] +fn a_statusline_still_prints_a_line_when_the_provider_reported_no_windows() { + assert_eq!(render_statusline(None), STATUSLINE_FALLBACK); + assert_eq!(render_statusline(Some(&Map::new())), STATUSLINE_FALLBACK); + let unusable = match serde_json::json!({"five_hour": {"used_percentage": "lots"}}) { + Value::Object(map) => map, + _ => unreachable!("the fixture is an object"), + }; + assert_eq!(render_statusline(Some(&unusable)), STATUSLINE_FALLBACK); +} diff --git a/plugins/nightcrow-recovery/src/helper_tests.rs b/plugins/nightcrow-recovery/src/helper_tests.rs index ab942798..e539f630 100644 --- a/plugins/nightcrow-recovery/src/helper_tests.rs +++ b/plugins/nightcrow-recovery/src/helper_tests.rs @@ -1,6 +1,8 @@ -//! The whitelisting and the statusline text. Reading stdin and connecting to the -//! socket are covered by `ipc_tests`; what matters here is that nothing outside -//! the whitelist can be forwarded, whatever a provider puts in its payload. +//! The whitelisting, and what one refresh decides. Reading stdin and connecting +//! to the socket are covered by `ipc_tests`; what matters here is that nothing +//! outside the whitelist can be forwarded, whatever a provider puts in its +//! payload, and that nothing a displaced statusline command does can lose the +//! usage numbers on the way. Which command gets to print is `helper_statusline`'s. use super::*; @@ -61,34 +63,69 @@ fn a_payload_missing_every_whitelisted_field_forwards_nothing() { assert!(pick(&Map::new(), &STOP_FAILURE_FIELDS).is_empty()); } +/// A statusline payload with usage numbers in it, as the bytes a provider would +/// have written them. +const STATUSLINE_BODY: &[u8] = + br#"{"session_id":"s","rate_limits":{"five_hour":{"used_percentage":40.0}}}"#; + +/// Only the command that hangs spends this, and no assertion below turns on +/// whether the others managed to print in time. +const BUDGET: Duration = Duration::from_millis(200); + +/// A generous budget, for the cases that are about what a command printed. +const ENOUGH: Duration = Duration::from_secs(5); + +fn statusline_entry(command: &str) -> Value { + serde_json::json!({ "type": "command", "command": command }) +} + +fn five_hour(refresh: &Refresh) -> Option<&Value> { + refresh.rate_limits.as_ref()?.get("five_hour") +} + #[test] -fn a_statusline_reports_the_usage_of_every_window_the_provider_gave() { - let limits = match serde_json::json!({ - "five_hour": {"used_percentage": 23.5, "resets_at": 1_767_225_600i64}, - "seven_day": {"used_percentage": 41.2, "resets_at": 1_767_657_600i64} - }) { - Value::Object(map) => map, - _ => unreachable!("the fixture is an object"), - }; - assert_eq!(render_statusline(Some(&limits)), "5h 24% | 7d 41%"); +fn a_refresh_forwards_the_usage_numbers_whatever_the_displaced_command_does() { + let expected = serde_json::json!({"used_percentage": 40.0}); + for displaced in [ + Value::Null, + statusline_entry("echo theirs"), + statusline_entry("exit 1"), + statusline_entry("sleep 30"), + statusline_entry("/x/nightcrow-recovery statusline"), + serde_json::json!({"type": "some-future-kind"}), + ] { + let refresh = refresh(STATUSLINE_BODY, Some(&displaced), BUDGET); + + let forwarded = five_hour(&refresh); + + assert_eq!(forwarded, Some(&expected), "lost them for {displaced}"); + } } #[test] -fn a_statusline_with_one_window_reports_only_that_window() { - let limits = match serde_json::json!({"seven_day": {"used_percentage": 8.0}}) { - Value::Object(map) => map, - _ => unreachable!("the fixture is an object"), - }; - assert_eq!(render_statusline(Some(&limits)), "7d 8%"); +fn a_refresh_prints_the_displaced_statuslines_line_rather_than_our_own() { + let displaced = statusline_entry("echo theirs"); + + let refresh = refresh(STATUSLINE_BODY, Some(&displaced), ENOUGH); + + assert_eq!(refresh.line, "theirs"); } #[test] -fn a_statusline_still_prints_a_line_when_the_provider_reported_no_windows() { - assert_eq!(render_statusline(None), STATUSLINE_FALLBACK); - assert_eq!(render_statusline(Some(&Map::new())), STATUSLINE_FALLBACK); - let unusable = match serde_json::json!({"five_hour": {"used_percentage": "lots"}}) { - Value::Object(map) => map, - _ => unreachable!("the fixture is an object"), - }; - assert_eq!(render_statusline(Some(&unusable)), STATUSLINE_FALLBACK); +fn a_refresh_with_nothing_displaced_prints_the_numbers_it_forwarded() { + let refresh = refresh(STATUSLINE_BODY, None, BUDGET); + + assert_eq!(refresh.line, "5h 40%"); +} + +#[test] +fn a_body_we_cannot_parse_still_reaches_the_displaced_command() { + // Parsing is for the fields we forward; the bytes are the displaced command's + // business, and a payload we cannot read may well be one it can. + let displaced = statusline_entry("cat"); + + let refresh = refresh(b"not json at all", Some(&displaced), ENOUGH); + + assert!(refresh.rate_limits.is_none()); + assert_eq!(refresh.line, "not json at all"); } diff --git a/plugins/nightcrow-recovery/src/hooks.rs b/plugins/nightcrow-recovery/src/hooks.rs index 42d3766f..c59d40bf 100644 --- a/plugins/nightcrow-recovery/src/hooks.rs +++ b/plugins/nightcrow-recovery/src/hooks.rs @@ -17,6 +17,11 @@ mod merge; use merge::{MARKER, STATUSLINE_KEY}; +/// Re-exported for the statusline helper, which must recognise our own command in +/// order to refuse chaining to it — the same check install and uninstall use to +/// recognise our entries. +pub(crate) use merge::is_ours; + const CLAUDE_DIR: &str = ".claude"; const SETTINGS_FILE: &str = "settings.json"; const BACKUP_FILE: &str = "settings.json.bak"; @@ -126,6 +131,16 @@ pub fn uninstall(paths: &SettingsPaths) -> Result> { Ok(changes) } +/// The `statusLine` install displaced, for the statusline helper to chain to. +/// +/// Read on every refresh, which is why it stays this cheap: one small file, and +/// any trouble reading it means no chain rather than a failure. The value can be +/// JSON `null` — that is what install records when it found no `statusLine` to +/// displace — so a caller must decide what a null means to it. +pub fn displaced_statusline(paths: &SettingsPaths) -> Option { + read_sidecar(&paths.sidecar) +} + /// Absent or empty means "no settings yet"; anything that is not a JSON object /// is a file we do not understand, and guessing at it is worse than stopping. fn read_settings(path: &Path) -> Result { diff --git a/plugins/nightcrow-recovery/src/main.rs b/plugins/nightcrow-recovery/src/main.rs index 1c86d525..8ac9f0ca 100644 --- a/plugins/nightcrow-recovery/src/main.rs +++ b/plugins/nightcrow-recovery/src/main.rs @@ -17,6 +17,8 @@ mod ipc; mod protocol; mod provider; mod runloop; +mod runloop_adopt; +mod runloop_io; mod state; mod wait; @@ -41,7 +43,8 @@ enum Mode { /// payload on stdin and forwards a few fields to the running plugin. Hook, /// Internal: the command Claude Code runs for its statusline. Forwards the - /// usage windows to the running plugin and prints a short line. + /// usage windows to the running plugin, then prints the statusline this + /// plugin displaced at install time, or a short line of its own. Statusline, } diff --git a/plugins/nightcrow-recovery/src/protocol.rs b/plugins/nightcrow-recovery/src/protocol.rs index ffa27e00..7f70e8ed 100644 --- a/plugins/nightcrow-recovery/src/protocol.rs +++ b/plugins/nightcrow-recovery/src/protocol.rs @@ -9,7 +9,11 @@ use serde::{Deserialize, Serialize}; /// Contract version this plugin speaks. The host refuses anything else. -pub const PROTOCOL_VERSION: u32 = 1; +/// +/// 2 is the first version with [`PluginCommand::WatchPane`], which this plugin +/// needs: a pane somebody started a provider CLI in by hand is never named to +/// us, so asking for it is the only way to watch it at all. +pub const PROTOCOL_VERSION: u32 = 2; /// Longest line the host will read from us; also the cap we apply to what we /// read, so a corrupt stream cannot make this process allocate without bound. @@ -128,6 +132,10 @@ pub enum PluginCommand { deadline_epoch: Option, attempt: u32, }, + /// Ask for the pane this token names. Carries no generation: we are asking + /// about a pane the host has never described to us, so we cannot know which + /// spawn it is on — the `PaneOpened` the host answers with is what says. + WatchPane { v: u32, token: PaneToken }, Log { v: u32, level: LogLevel, @@ -187,6 +195,13 @@ pub fn encode_command(cmd: &PluginCommand) -> anyhow::Result { Ok(line) } +pub fn watch_pane(token: PaneToken) -> PluginCommand { + PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token, + } +} + pub fn log(level: LogLevel, message: impl Into) -> PluginCommand { PluginCommand::Log { v: PROTOCOL_VERSION, diff --git a/plugins/nightcrow-recovery/src/protocol_tests.rs b/plugins/nightcrow-recovery/src/protocol_tests.rs index ff912edf..f41c8bef 100644 --- a/plugins/nightcrow-recovery/src/protocol_tests.rs +++ b/plugins/nightcrow-recovery/src/protocol_tests.rs @@ -4,7 +4,7 @@ const TOKEN: &str = "0123456789abcdef0123456789abcdef"; fn opened_line() -> String { format!( - r#"{{"event":"pane_opened","v":1,"token":"{TOKEN}","generation":2,"title":null,"command":"claude","cwd":"/w/repo"}}"# + r#"{{"event":"pane_opened","v":{PROTOCOL_VERSION},"token":"{TOKEN}","generation":2,"title":null,"command":"claude","cwd":"/w/repo"}}"# ) } @@ -28,23 +28,42 @@ fn a_host_event_is_parsed_from_its_wire_shape() { #[test] fn an_event_from_another_protocol_version_is_refused_and_both_versions_named() { - let line = opened_line().replace("\"v\":1", "\"v\":2"); + let alien = PROTOCOL_VERSION + 1; + let line = opened_line().replace( + &format!("\"v\":{PROTOCOL_VERSION}"), + &format!("\"v\":{alien}"), + ); let err = decode_event(&line) .expect_err("a version this build cannot speak") .to_string(); - assert!(err.contains('2') && err.contains('1'), "{err}"); + assert!( + err.contains(&alien.to_string()) && err.contains(&PROTOCOL_VERSION.to_string()), + "{err}" + ); +} + +#[test] +fn an_event_from_the_previous_protocol_version_is_refused_too() { + // Refused in both directions on purpose. A host still speaking 1 has no + // `watch_pane` to honour, so the panes this plugin exists for would silently + // never be watched — better to fail at the first line than halfway. + let line = opened_line().replace(&format!("\"v\":{PROTOCOL_VERSION}"), "\"v\":1"); + assert!(decode_event(&line).is_err()); } #[test] fn an_unknown_event_is_refused_rather_than_guessed() { - let line = r#"{"event":"pane_teleported","v":1,"token":"abc","generation":1}"#; - assert!(decode_event(line).is_err()); + let line = format!( + r#"{{"event":"pane_teleported","v":{PROTOCOL_VERSION},"token":"abc","generation":1}}"# + ); + assert!(decode_event(&line).is_err()); } #[test] fn an_event_missing_a_required_field_is_refused() { - let line = r#"{"event":"pane_idle","v":1,"token":"abc","generation":1}"#; - assert!(decode_event(line).is_err(), "idle_ms is required"); + let line = + format!(r#"{{"event":"pane_idle","v":{PROTOCOL_VERSION},"token":"abc","generation":1}}"#); + assert!(decode_event(&line).is_err(), "idle_ms is required"); } #[test] @@ -56,7 +75,8 @@ fn a_line_over_the_length_limit_is_refused_before_it_is_parsed() { #[test] fn a_shutdown_names_no_pane() { - let event = decode_event(r#"{"event":"shutdown","v":1}"#).expect("a shutdown"); + let event = decode_event(&format!(r#"{{"event":"shutdown","v":{PROTOCOL_VERSION}}}"#)) + .expect("a shutdown"); assert_eq!(event.token(), None); assert_eq!(event.generation(), None); } @@ -72,7 +92,25 @@ fn a_command_is_encoded_as_one_line_tagged_with_its_name() { .expect("encodable"); assert!(!line.contains('\n')); assert!(line.contains("\"cmd\":\"relaunch\""), "{line}"); - assert!(line.contains("\"v\":1"), "{line}"); + assert!( + line.contains(&format!("\"v\":{PROTOCOL_VERSION}")), + "{line}" + ); +} + +#[test] +fn a_watch_pane_request_carries_only_the_token() { + // Deliberately no generation: this is asked about a pane the host has never + // described to us, so any generation we put here would be invented — and the + // host would be right to refuse it. + let line = encode_command(&watch_pane(TOKEN.to_string())).expect("encodable"); + assert!(line.contains("\"cmd\":\"watch_pane\""), "{line}"); + assert!(line.contains(&format!("\"token\":\"{TOKEN}\"")), "{line}"); + assert!(!line.contains("generation"), "{line}"); + assert!( + line.contains(&format!("\"v\":{PROTOCOL_VERSION}")), + "{line}" + ); } #[test] @@ -133,7 +171,7 @@ fn the_hosts_copy_of_the_contract_still_says_the_same_thing() { let source = std::fs::read_to_string(host) .unwrap_or_else(|e| panic!("cannot read the host's protocol at {host}: {e}")); - assert_eq!(host_const(&source, "PROTOCOL_VERSION"), "1"); + assert_eq!(host_const(&source, "PROTOCOL_VERSION"), "2"); assert_eq!(host_const(&source, "MAX_LINE_BYTES"), "64 * 1024"); assert_eq!(host_const(&source, "MAX_INPUT_BYTES"), "8 * 1024"); assert_eq!( diff --git a/plugins/nightcrow-recovery/src/provider/mod.rs b/plugins/nightcrow-recovery/src/provider/mod.rs index b7d9b8f4..30c643ad 100644 --- a/plugins/nightcrow-recovery/src/provider/mod.rs +++ b/plugins/nightcrow-recovery/src/provider/mod.rs @@ -159,6 +159,25 @@ pub fn detect(command: Option<&str>) -> Option> { } } +/// Pick an adapter from a signal that arrived over the IPC socket, for a pane +/// whose command line says nothing — the shell somebody opened and then started +/// a provider CLI inside by hand. +/// +/// Sound because a [`SignalKind`] is minted by exactly one provider's helper: +/// a `stop_failure` line can only have come from the Claude Code hook this +/// binary installed into Claude Code's own settings. The signal is therefore +/// evidence of what the pane is running, in a way terminal text never is — which +/// is why this is a lookup on the wire kind and deliberately not a second +/// sniffing path. A kind added later has to be classified here rather than +/// falling through to a guess. +pub fn detect_from_signal(kind: SignalKind) -> Option> { + match kind { + SignalKind::StopFailure | SignalKind::RateLimits => { + Some(Box::new(claude::Claude::default())) + } + } +} + /// The command's program name without its directory, so `/usr/local/bin/claude` /// and `claude --foo` both resolve to `claude`. fn first_word(command: &str) -> Option<&str> { diff --git a/plugins/nightcrow-recovery/src/provider/mod_tests.rs b/plugins/nightcrow-recovery/src/provider/mod_tests.rs index 9b52ebb6..8ee1b9c0 100644 --- a/plugins/nightcrow-recovery/src/provider/mod_tests.rs +++ b/plugins/nightcrow-recovery/src/provider/mod_tests.rs @@ -93,6 +93,26 @@ fn a_pane_running_something_else_is_not_watched_at_all() { } } +#[test] +fn every_signal_kind_names_the_adapter_whose_helper_minted_it() { + // A pane with no command line of its own — the shell somebody typed `claude` + // into — has only the signal to go on, and the signal's kind is enough: each + // one is written by exactly one provider's helper. + for kind in [SignalKind::StopFailure, SignalKind::RateLimits] { + let provider = + detect_from_signal(kind).unwrap_or_else(|| panic!("{kind:?} names an adapter")); + assert_eq!(provider.name(), "claude"); + } +} + +#[test] +fn a_signal_binds_an_adapter_where_the_command_line_cannot() { + // The pair that makes the late-adoption path work at all: `detect` gives up + // on a pane with no command, and the signal is what answers instead. + assert!(detect(None).is_none()); + assert!(detect_from_signal(SignalKind::StopFailure).is_some()); +} + #[test] fn a_signal_kind_round_trips_through_its_wire_name() { for kind in [SignalKind::StopFailure, SignalKind::RateLimits] { diff --git a/plugins/nightcrow-recovery/src/runloop.rs b/plugins/nightcrow-recovery/src/runloop.rs index 94a3ea6f..20de066d 100644 --- a/plugins/nightcrow-recovery/src/runloop.rs +++ b/plugins/nightcrow-recovery/src/runloop.rs @@ -7,17 +7,19 @@ //! wakeup, expired or not, ticks the state machines. //! //! Everything the plugin says goes out from this one thread, so the NDJSON -//! stream cannot interleave two half-written lines. +//! stream cannot interleave two half-written lines — see +//! [`runloop_io`](crate::runloop_io), which owns both ends of that stream. use crate::ipc::{Ipc, IpcMessage, socket_path}; -use crate::protocol::{LogLevel, PluginCommand, PluginEvent, decode_event, encode_command, log}; -use crate::provider::{PaneContext, Provider, detect}; +use crate::protocol::{LogLevel, PluginEvent, log}; +use crate::provider::{OutOfBand, PaneContext, Provider, detect, detect_from_signal}; +use crate::runloop_adopt::Adoptions; +use crate::runloop_io::{Message, emit, emit_all, spawn_stdin_reader}; use crate::state::{PaneRecovery, RecoveryState}; use crate::wait::now_epoch; use anyhow::Result; use std::collections::HashMap; -use std::io::{BufRead, BufReader, Write}; -use std::sync::mpsc::{Sender, channel}; +use std::sync::mpsc::channel; use std::time::{Duration, Instant}; /// How often the state machines are advanced when nothing else happens. @@ -31,20 +33,12 @@ const TICK: Duration = Duration::from_secs(1); /// /// A session holds a handful of panes. The cap exists so a host that somehow /// announced panes without ever closing them cannot grow this process without -/// bound; reaching it means dropping the *new* pane, which fails closed. +/// bound; reaching it means dropping the *new* pane, which fails closed. It +/// binds the panes we asked for as well as the ones we were given — the ask is +/// bounded separately (see [`Adoptions`]), but this is the ceiling on what any +/// number of asks can add up to. const MAX_TRACKED_PANES: usize = 64; -/// What the main thread waits on. -enum Message { - Host(PluginEvent), - /// A line from the host that could not be understood. Reported and skipped: - /// one bad line is not a reason to abandon a session's panes. - HostGarbage(String), - Signal(IpcMessage), - /// stdin ended. The host is gone, so there is nothing left to serve. - HostGone, -} - /// One tracked pane: its recovery progress and the adapter watching it. struct Watch { recovery: PaneRecovery, @@ -79,6 +73,7 @@ pub fn run() -> Result<()> { spawn_stdin_reader(tx); let mut panes: HashMap = HashMap::new(); + let mut adoptions = Adoptions::default(); emit(&log(LogLevel::Info, "nightcrow-recovery watching panes"))?; loop { let message = match rx.recv_timeout(TICK) { @@ -92,13 +87,14 @@ pub fn run() -> Result<()> { Some(Message::HostGone) | Some(Message::Host(PluginEvent::Shutdown { .. })) => { return farewell(&panes); } - Some(Message::Host(event)) => on_host_event(&mut panes, &event)?, + Some(Message::Host(event)) => on_host_event(&mut panes, &mut adoptions, &event)?, Some(Message::HostGarbage(reason)) => { emit(&log(LogLevel::Warn, reason))?; } - Some(Message::Signal(msg)) => on_signal(&mut panes, msg)?, + Some(Message::Signal(msg)) => on_signal(&mut panes, &mut adoptions, msg)?, None => {} } + adoptions.prune(Instant::now()); tick(&mut panes)?; } } @@ -123,33 +119,19 @@ fn farewell(panes: &HashMap) -> Result<()> { Ok(()) } -fn spawn_stdin_reader(tx: Sender) { - std::thread::spawn(move || { - let reader = BufReader::new(std::io::stdin()); - for line in reader.lines() { - let Ok(line) = line else { - break; - }; - if line.trim().is_empty() { - continue; - } - let message = match decode_event(&line) { - Ok(event) => Message::Host(event), - Err(e) => Message::HostGarbage(format!("ignoring a host line: {e}")), - }; - if tx.send(message).is_err() { - return; - } - } - let _ = tx.send(Message::HostGone); - }); -} - -fn on_host_event(panes: &mut HashMap, event: &PluginEvent) -> Result<()> { +fn on_host_event( + panes: &mut HashMap, + adoptions: &mut Adoptions, + event: &PluginEvent, +) -> Result<()> { let Some(token) = event.token().cloned() else { return Ok(()); }; let now = now_epoch(); + // The signal that won this pane its watcher, held back until the pane has + // been through the housekeeping below — an adapter must not be asked about a + // pane whose first event has not been applied yet. + let mut held = None; if let PluginEvent::PaneOpened { generation, command, @@ -157,7 +139,14 @@ fn on_host_event(panes: &mut HashMap, event: &PluginEvent) -> Res .. } = event { - open_pane(panes, &token, *generation, command.as_deref(), cwd)?; + held = open_pane( + panes, + adoptions, + &token, + *generation, + command.as_deref(), + cwd, + )?; } let Some(watch) = panes.get_mut(&token) else { return Ok(()); @@ -184,39 +173,57 @@ fn on_host_event(panes: &mut HashMap, event: &PluginEvent) -> Res if matches!(event, PluginEvent::PaneClosed { .. }) { panes.remove(&token); } + if let Some(signal) = held { + deliver_signal(panes, &token, &signal)?; + } Ok(()) } +/// Start watching the pane `token` names, answering with the signal that has been +/// waiting for it — see [`Adoptions`]. fn open_pane( panes: &mut HashMap, + adoptions: &mut Adoptions, token: &str, generation: u32, command: Option<&str>, cwd: &str, -) -> Result<()> { +) -> Result> { let ctx = PaneContext { token: token.to_string(), generation, cwd: cwd.to_string(), command: command.map(str::to_string), }; + // Claimed unconditionally: whether or not it decides the adapter below, a + // request that has been answered must not stay outstanding. + let claimed = adoptions.claim(token); if let Some(watch) = panes.get_mut(token) { // A relaunch reopens the same slot. The recovery state survives, so an // attempt budget cannot be reset by relaunching into the same limit. watch.ctx = ctx; - return Ok(()); + return Ok(claimed); } - let Some(provider) = detect(command) else { + // The command line first, since it is the host's own record of what the pane + // was launched as. Only when there is none does the signal decide, which is + // the pane somebody started a CLI in by hand: it says nothing about itself, + // but a provider's helper has just spoken for it. + let provider = detect(command).or_else(|| { + claimed + .as_ref() + .and_then(|signal| detect_from_signal(signal.kind)) + }); + let Some(provider) = provider else { // A pane running something this plugin knows nothing about is not // watched at all, which is the cheapest way to stay out of it. - return Ok(()); + return Ok(None); }; if panes.len() >= MAX_TRACKED_PANES { emit(&log( LogLevel::Warn, format!("already watching {MAX_TRACKED_PANES} panes; not watching another"), ))?; - return Ok(()); + return Ok(None); } emit(&log( LogLevel::Info, @@ -230,18 +237,39 @@ fn open_pane( ctx, }, ); + Ok(claimed) +} + +fn on_signal( + panes: &mut HashMap, + adoptions: &mut Adoptions, + msg: IpcMessage, +) -> Result<()> { + if panes.contains_key(&msg.token) { + let (token, signal) = msg.into_signal(); + return deliver_signal(panes, &token, &signal); + } + // Not a pane we were told about — and that is the common case rather than the + // odd one. The token is proof the sender runs inside one of the host's panes, + // so ask for it; a token from another nightcrow session simply goes + // unanswered. See [`Adoptions`] for why asking is bounded. + if let Some(command) = adoptions.request(msg, Instant::now()) { + emit(&command)?; + } Ok(()) } -fn on_signal(panes: &mut HashMap, msg: IpcMessage) -> Result<()> { - let (token, signal) = msg.into_signal(); - // A signal for a pane we do not track is ordinary: a provider helper from - // another nightcrow session, or a pane whose command we do not watch. - let Some(watch) = panes.get_mut(&token) else { +/// Hand one out-of-band signal to the adapter watching `token`. +fn deliver_signal( + panes: &mut HashMap, + token: &str, + signal: &OutOfBand, +) -> Result<()> { + let Some(watch) = panes.get_mut(token) else { return Ok(()); }; let now = now_epoch(); - if let Some(limit) = watch.provider.on_signal(&watch.ctx, &signal, now) { + if let Some(limit) = watch.provider.on_signal(&watch.ctx, signal, now) { let commands = watch.recovery.note_limit(limit, now, Instant::now()); emit_all(&commands)?; } @@ -263,27 +291,3 @@ fn tick(panes: &mut HashMap) -> Result<()> { } Ok(()) } - -fn emit_all(commands: &[PluginCommand]) -> Result<()> { - for command in commands { - emit(command)?; - } - Ok(()) -} - -/// Write one command as one NDJSON line. A command that cannot be framed is -/// dropped with a complaint rather than corrupting the stream. -fn emit(command: &PluginCommand) -> Result<()> { - let line = match encode_command(command) { - Ok(line) => line, - Err(e) => encode_command(&log( - LogLevel::Error, - format!("dropped an unencodable command: {e}"), - ))?, - }; - let mut out = std::io::stdout().lock(); - out.write_all(line.as_bytes())?; - out.write_all(b"\n")?; - out.flush()?; - Ok(()) -} diff --git a/plugins/nightcrow-recovery/src/runloop_adopt.rs b/plugins/nightcrow-recovery/src/runloop_adopt.rs new file mode 100644 index 00000000..bd0752f9 --- /dev/null +++ b/plugins/nightcrow-recovery/src/runloop_adopt.rs @@ -0,0 +1,105 @@ +//! Asking the host for a pane it never named to us. +//! +//! The dominant way a coding CLI gets started is by hand: the user opens a plain +//! shell and types `claude` into it. That pane's `[[startup_command]]` names no +//! plugin, so the host never mentions it — but the CLI's hook still reaches us +//! over the socket, carrying the token the host put in that pane's environment. +//! Presenting the token back is the whole request; the host decides whether to +//! honour it, and never tells us when it does not. +//! +//! Everything here exists because of that silence. A refusal is indistinguishable +//! from a token belonging to another nightcrow session's pane, so a request that +//! goes unanswered must not be repeated in a tight loop and must not leave state +//! behind that grows with every stranger that knocks. + +use crate::ipc::IpcMessage; +use crate::protocol::{PaneToken, PluginCommand, watch_pane}; +use crate::provider::OutOfBand; +use std::collections::HashMap; +use std::time::{Duration, Instant}; + +/// Most tokens we will have an outstanding request for at once. +/// +/// These are unsolicited: anything that can reach the socket can name a token we +/// have never seen. A session has a handful of panes and the host answers within +/// one of its ticks, so a backlog this deep already means the requests are not +/// being honoured — and dropping the newest is what keeps a stream of strangers +/// from growing this map. +const MAX_PENDING: usize = 8; + +/// How long one token's unanswered request suppresses another for that token. +/// +/// The host answers in milliseconds or never, so this is not a retry interval — +/// it is the rate at which a token that is not ours may cost us a command. +/// Claude Code's statusline runs on every render, so without it a foreign pane +/// would have us writing a request several times a second, all refused and every +/// one of them counted against the host's per-tick command budget alongside the +/// requests that matter. Half a minute is far longer than any honoured request +/// takes and short enough that a pane which only just became ours is not shut +/// out for long. +const REQUEST_COOLDOWN: Duration = Duration::from_secs(30); + +/// One outstanding request, and the signal that justified making it. +struct Pending { + /// Kept so the adapter still gets it. The signal arrives *before* the pane + /// does — it is the reason the pane arrives at all — and the host replays no + /// history to a pane it has just handed over, so dropping it would lose the + /// very limit being recovered from and leave the pane parked until the + /// provider happened to fail again. + signal: OutOfBand, + asked_at: Instant, +} + +/// The tokens we have asked about and not yet been given. +#[derive(Default)] +pub(crate) struct Adoptions(HashMap); + +impl Adoptions { + /// Turn a signal for an untracked pane into a request, or into nothing. + /// + /// Consumes the message either way: when the answer is nothing, the token and + /// its payload are dropped here rather than recorded, so a token that will + /// never be honoured costs one hash probe and no growth. + pub(crate) fn request(&mut self, msg: IpcMessage, now: Instant) -> Option { + let (token, signal) = msg.into_signal(); + // Already asked, and [`Self::prune`] has not yet given up on it. + if self.0.contains_key(&token) { + return None; + } + if self.0.len() >= MAX_PENDING { + return None; + } + self.0.insert( + token.clone(), + Pending { + signal, + asked_at: now, + }, + ); + Some(watch_pane(token)) + } + + /// Take back the signal that won `token` its request, now that the host has + /// described the pane. Answers `None` for a pane we never asked about, which + /// is every configured pane. + pub(crate) fn claim(&mut self, token: &str) -> Option { + self.0.remove(token).map(|pending| pending.signal) + } + + /// Give up on requests the host has not answered within + /// [`REQUEST_COOLDOWN`], which is also what lets that token be asked about + /// again. + /// + /// Without it a handful of foreign tokens would hold every slot for the + /// process's whole life, and a pane that later became ours could not get a + /// request in. Giving up is safe: a pane that really is ours signals again, + /// and the held signal is stale by then anyway. + pub(crate) fn prune(&mut self, now: Instant) { + self.0 + .retain(|_, p| now.saturating_duration_since(p.asked_at) < REQUEST_COOLDOWN); + } +} + +#[cfg(test)] +#[path = "runloop_adopt_tests.rs"] +mod tests; diff --git a/plugins/nightcrow-recovery/src/runloop_adopt_tests.rs b/plugins/nightcrow-recovery/src/runloop_adopt_tests.rs new file mode 100644 index 00000000..0a1efae3 --- /dev/null +++ b/plugins/nightcrow-recovery/src/runloop_adopt_tests.rs @@ -0,0 +1,128 @@ +//! Asking for a pane: what is asked once, what is not asked twice, and what a +//! stream of tokens we will never be given can cost this process. + +use super::*; +use crate::provider::SignalKind; + +fn signal(token: &str) -> IpcMessage { + IpcMessage { + token: token.to_string(), + kind: SignalKind::StopFailure, + payload: serde_json::json!({"error_type": "rate_limit"}), + } +} + +fn asked_token(command: &PluginCommand) -> String { + match command { + PluginCommand::WatchPane { token, .. } => token.clone(), + other => panic!("expected a watch_pane request, got {other:?}"), + } +} + +#[test] +fn a_signal_for_an_unknown_pane_asks_the_host_for_it_by_token() { + let mut a = Adoptions::default(); + let command = a + .request(signal("abc123"), Instant::now()) + .expect("a first signal asks"); + assert_eq!(asked_token(&command), "abc123"); +} + +#[test] +fn a_second_signal_for_the_same_token_asks_nothing_more() { + // The host answers in milliseconds or never, so a repeat inside the cooldown + // could only be noise — and Claude Code's statusline is noisy: it runs on + // every render. + let mut a = Adoptions::default(); + let now = Instant::now(); + assert!(a.request(signal("abc123"), now).is_some()); + for _ in 0..50 { + assert!(a.request(signal("abc123"), now).is_none()); + } +} + +#[test] +fn a_token_may_be_asked_about_again_once_its_request_has_been_given_up_on() { + let mut a = Adoptions::default(); + let now = Instant::now(); + assert!(a.request(signal("abc123"), now).is_some()); + + a.prune(now + REQUEST_COOLDOWN); + + assert!( + a.request(signal("abc123"), now + REQUEST_COOLDOWN) + .is_some(), + "a pane that only just became ours must not be shut out for good" + ); +} + +#[test] +fn a_request_still_inside_its_cooldown_survives_pruning() { + let mut a = Adoptions::default(); + let now = Instant::now(); + assert!(a.request(signal("abc123"), now).is_some()); + + a.prune(now + REQUEST_COOLDOWN / 2); + + assert!( + a.request(signal("abc123"), now + REQUEST_COOLDOWN / 2) + .is_none() + ); +} + +#[test] +fn a_flood_of_unknown_tokens_stops_at_the_pending_ceiling() { + // Unsolicited state: anything that can reach the socket can name a token we + // have never seen, so this must stop growing rather than stop working. + let mut a = Adoptions::default(); + let now = Instant::now(); + for i in 0..MAX_PENDING { + assert!( + a.request(signal(&format!("token{i}")), now).is_some(), + "the first {MAX_PENDING} tokens are asked about" + ); + } + assert!( + a.request(signal("onemore"), now).is_none(), + "past the ceiling a new token is dropped, not queued" + ); + // And the dropped token left nothing behind, so the ceiling is a ceiling on + // memory and not merely on requests. + a.prune(now + REQUEST_COOLDOWN); + assert!( + a.request(signal("onemore"), now + REQUEST_COOLDOWN) + .is_some() + ); +} + +#[test] +fn the_signal_that_won_a_pane_its_request_is_handed_back_when_the_pane_arrives() { + // The signal arrives before the pane does and the host replays no history, so + // losing it here would lose the very limit the recovery is about. + let mut a = Adoptions::default(); + assert!(a.request(signal("abc123"), Instant::now()).is_some()); + + let held = a.claim("abc123").expect("the signal was kept"); + + assert_eq!(held.kind, SignalKind::StopFailure); + assert_eq!(held.payload["error_type"], "rate_limit"); +} + +#[test] +fn a_claimed_request_is_not_handed_back_a_second_time() { + let mut a = Adoptions::default(); + assert!(a.request(signal("abc123"), Instant::now()).is_some()); + assert!(a.claim("abc123").is_some()); + assert!( + a.claim("abc123").is_none(), + "one signal must not be applied twice" + ); +} + +#[test] +fn a_pane_we_never_asked_about_has_no_signal_waiting_for_it() { + // Every configured pane takes this path: the host named it, so no request was + // ever made for it. + let mut a = Adoptions::default(); + assert!(a.claim("never-asked").is_none()); +} diff --git a/plugins/nightcrow-recovery/src/runloop_io.rs b/plugins/nightcrow-recovery/src/runloop_io.rs new file mode 100644 index 00000000..fa647caa --- /dev/null +++ b/plugins/nightcrow-recovery/src/runloop_io.rs @@ -0,0 +1,69 @@ +//! The plugin's two ends of the host's NDJSON stream. +//! +//! Split out of `runloop.rs` so that file is the loop's reasoning and this one is +//! its plumbing. Everything the plugin says leaves through [`emit`], called only +//! from the main thread, which is what keeps two half-written lines from +//! interleaving on stdout. + +use crate::ipc::IpcMessage; +use crate::protocol::{LogLevel, PluginCommand, PluginEvent, decode_event, encode_command, log}; +use anyhow::Result; +use std::io::{BufRead, BufReader, Write}; +use std::sync::mpsc::Sender; + +/// What the main thread waits on. +pub(crate) enum Message { + Host(PluginEvent), + /// A line from the host that could not be understood. Reported and skipped: + /// one bad line is not a reason to abandon a session's panes. + HostGarbage(String), + Signal(IpcMessage), + /// stdin ended. The host is gone, so there is nothing left to serve. + HostGone, +} + +pub(crate) fn spawn_stdin_reader(tx: Sender) { + std::thread::spawn(move || { + let reader = BufReader::new(std::io::stdin()); + for line in reader.lines() { + let Ok(line) = line else { + break; + }; + if line.trim().is_empty() { + continue; + } + let message = match decode_event(&line) { + Ok(event) => Message::Host(event), + Err(e) => Message::HostGarbage(format!("ignoring a host line: {e}")), + }; + if tx.send(message).is_err() { + return; + } + } + let _ = tx.send(Message::HostGone); + }); +} + +pub(crate) fn emit_all(commands: &[PluginCommand]) -> Result<()> { + for command in commands { + emit(command)?; + } + Ok(()) +} + +/// Write one command as one NDJSON line. A command that cannot be framed is +/// dropped with a complaint rather than corrupting the stream. +pub(crate) fn emit(command: &PluginCommand) -> Result<()> { + let line = match encode_command(command) { + Ok(line) => line, + Err(e) => encode_command(&log( + LogLevel::Error, + format!("dropped an unencodable command: {e}"), + ))?, + }; + let mut out = std::io::stdout().lock(); + out.write_all(line.as_bytes())?; + out.write_all(b"\n")?; + out.flush()?; + Ok(()) +} diff --git a/plugins/nightcrow-recovery/src/state_resume.rs b/plugins/nightcrow-recovery/src/state_resume.rs index 07131c5d..8d6697e0 100644 --- a/plugins/nightcrow-recovery/src/state_resume.rs +++ b/plugins/nightcrow-recovery/src/state_resume.rs @@ -31,6 +31,11 @@ fn is_safe_arg_char(c: char) -> bool { c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-' | ':' | '/' | '=' | '@' | '+') } +/// Said of a pane the host launched no command in, so the person reading the +/// status line learns why the wait ended without a resume. +const NO_COMMAND_TO_RELAUNCH: &str = + "this pane has no command to relaunch; start the session again by hand"; + fn args_are_safe(args: &[String]) -> bool { !args.is_empty() && args.len() <= MAX_RESUME_ARGS @@ -68,7 +73,7 @@ impl PaneRecovery { self.goto(RecoveryState::NeedsAttention) } ResumePlan::Input(data) => self.send_input(data, now_epoch, now), - ResumePlan::Relaunch(args) => self.relaunch(args, now), + ResumePlan::Relaunch(args) => self.relaunch(args, ctx, now), } } @@ -94,12 +99,25 @@ impl PaneRecovery { self.spend_attempt(command, now) } - fn relaunch(&mut self, args: Vec, now: Instant) -> Vec { + fn relaunch( + &mut self, + args: Vec, + ctx: &PaneContext, + now: Instant, + ) -> Vec { if self.alive { // The host refuses a relaunch of a live pane, so wait for the exit // rather than spending an attempt learning that again. return Vec::new(); } + if ctx.command.is_none() { + // A pane with no command of its own is a shell somebody started the + // provider inside by hand. Putting a process back would start the + // shell again, not the session, so the host refuses it outright — + // and there is nothing to wait for that would change that. + self.detail = Some(NO_COMMAND_TO_RELAUNCH.to_string()); + return self.goto(RecoveryState::NeedsAttention); + } if !args_are_safe(&args) { self.detail = Some("adapter offered resume args the host would refuse".to_string()); return self.goto(RecoveryState::NeedsAttention); diff --git a/plugins/nightcrow-recovery/src/state_tests/resume.rs b/plugins/nightcrow-recovery/src/state_tests/resume.rs index cae3d61a..ead34ce2 100644 --- a/plugins/nightcrow-recovery/src/state_tests/resume.rs +++ b/plugins/nightcrow-recovery/src/state_tests/resume.rs @@ -185,6 +185,59 @@ fn an_adapter_offering_unsafe_resume_args_is_refused_without_asking_the_host() { assert!(action(&out).is_none()); } +/// A pane the host launched no command in: the plain shell somebody started a +/// provider CLI inside by hand, which is the only kind of pane this plugin can be +/// given on a signal. +fn bare_shell() -> PaneContext { + PaneContext { + command: None, + ..ctx() + } +} + +#[test] +fn a_pane_the_host_launched_no_command_in_is_never_relaunched() { + // Putting a process back would start the shell again, not the session, so the + // host refuses it outright — and no amount of waiting changes that, which is + // why this ends in needs_attention instead of another backoff. + let mut rec = recovery(); + let provider = FakeProvider::relaunch_only(); + let mono = Instant::now(); + rec.note_limit(usage(Some(SESSION), Some(RESET)), T0, mono); + rec.on_event(&exited(1)).expect("current generation"); + let out = rec.tick( + &provider, + &bare_shell(), + T0 + AFTER_RESET, + at(mono, AFTER_RESET), + ); + assert_eq!(rec.state(), RecoveryState::NeedsAttention); + assert!(action(&out).is_none(), "the host is asked for nothing"); + assert_eq!(rec.attempt(), 0, "and no attempt is spent learning that"); +} + +#[test] +fn a_pane_with_no_command_is_still_typed_into_while_its_process_lives() { + // The other half of the same rule: typed input is the whole of the recovery + // such a pane can get, so the missing command line must not cost it that too. + let mut rec = recovery(); + let provider = FakeProvider::default(); + let mono = Instant::now(); + rec.note_limit(usage(Some(SESSION), Some(RESET)), T0, mono); + rec.on_event(&went_idle(1)).expect("current generation"); + let out = rec.tick( + &provider, + &bare_shell(), + T0 + AFTER_RESET, + at(mono, AFTER_RESET), + ); + assert_eq!(rec.state(), RecoveryState::Resuming); + match action(&out) { + Some(PluginCommand::SendInput { data, .. }) => assert_eq!(data, "continue\r"), + other => panic!("expected typed input, got {other:?}"), + } +} + #[test] fn an_adapter_with_nothing_to_say_costs_one_attempt_and_backs_off() { let mut rec = recovery(); diff --git a/src/application/session_terminals_tests.rs b/src/application/session_terminals_tests.rs index 26b9d9aa..30333eac 100644 --- a/src/application/session_terminals_tests.rs +++ b/src/application/session_terminals_tests.rs @@ -27,7 +27,7 @@ const DEADLINE: Duration = Duration::from_secs(15); fn attached(dir: &tempfile::TempDir, repos: &[String]) -> (DaemonSocket, DaemonClient) { let socket = DaemonSocket::bind(&dir.path().join("d.sock")).expect("binds"); let listener = socket.listener().try_clone().expect("clones"); - let state = crate::test_util::session_state(repos); + let state = crate::test_util::session_state(repos, dir.path()); let session = crate::daemon::serve::start(state).expect("starts the watcher"); std::thread::spawn(move || crate::daemon::serve::serve(listener, session)); let client = DaemonClient::connect(socket.path()).expect("attaches"); diff --git a/src/config/plugin.rs b/src/config/plugin.rs index 99ae0882..f603a760 100644 --- a/src/config/plugin.rs +++ b/src/config/plugin.rs @@ -4,8 +4,10 @@ use std::collections::{BTreeMap, BTreeSet}; /// An external plugin process (`[[plugin]]`). nightcrow itself knows nothing /// about what a plugin does: it launches the executable and speaks its protocol. -/// A plugin only ever sees a pane whose `[[startup_command]]` opted in by name, -/// so adding a plugin here does not hand it the whole session. +/// A plugin only ever sees a pane that opted in — by name in a +/// `[[startup_command]]`, or, with [`watch_on_signal`](Self::watch_on_signal), +/// by something inside the pane quoting the pane's own token — so adding a +/// plugin here does not hand it the whole session. #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct PluginConfig { /// Name a startup pane refers to in its `plugin =` field. @@ -30,6 +32,19 @@ pub struct PluginConfig { /// Off unless explicitly turned on. #[serde(default)] pub enabled: bool, + /// Whether this plugin may be given a pane no `[[startup_command]]` named + /// it in, when a process *inside* that pane reports to it quoting the pane's + /// own token. + /// + /// Off by default, so an existing config keeps the property that the opt-in + /// list is the whole of what a plugin can see. Turning it on trades that for + /// a narrower one: a pane is reachable once something running in it has + /// spoken to the plugin, which a plain shell never does. The token is what + /// makes the difference — it is random, per pane, and only in that pane's + /// child environment, so it cannot be guessed from outside and the plugin is + /// never told which panes exist. + #[serde(default)] + pub watch_on_signal: bool, } /// Check the `[[plugin]]` list and every startup pane's opt-in against it. diff --git a/src/config/tests/plugin.rs b/src/config/tests/plugin.rs index 99e44b1b..1268583f 100644 --- a/src/config/tests/plugin.rs +++ b/src/config/tests/plugin.rs @@ -49,6 +49,34 @@ fn plugin_args_and_env_default_to_empty_when_omitted() { assert!(cfg.plugins[0].env.is_empty()); } +#[test] +fn watch_on_signal_is_off_unless_the_config_asks_for_it() { + // The default is what every existing config gets, and it has to keep meaning + // "the opt-in list is the whole of what this plugin can see". + let cfg: Config = + toml::from_str("[[plugin]]\nname = \"recovery\"\ncommand = \"run-me\"\n").unwrap(); + assert!(!cfg.plugins[0].watch_on_signal); + + let cfg: Config = toml::from_str( + "[[plugin]]\nname = \"recovery\"\ncommand = \"run-me\"\nwatch_on_signal = true\n", + ) + .unwrap(); + assert!(cfg.plugins[0].watch_on_signal); + validate_config(&cfg).unwrap(); +} + +#[test] +fn a_plugin_that_watches_on_signal_needs_no_pane_to_name_it() { + // Its panes are the ones that will speak to it, and none of them can be known + // in advance — so requiring an opt-in here would make the switch unusable. + let cfg: Config = toml::from_str( + "[[plugin]]\nname = \"recovery\"\ncommand = \"run-me\"\n\ + enabled = true\nwatch_on_signal = true\n", + ) + .unwrap(); + validate_config(&cfg).unwrap(); +} + #[test] fn a_plugin_without_name_or_command_fails_to_deserialize() { assert!(toml::from_str::("[[plugin]]\ncommand = \"run-me\"\n").is_err()); diff --git a/src/daemon/client.rs b/src/daemon/client.rs index 72677f19..9b4b5438 100644 --- a/src/daemon/client.rs +++ b/src/daemon/client.rs @@ -24,7 +24,6 @@ use std::time::Duration; /// invented out of an idle session. const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(5); -/// A connection to the session daemon. #[derive(Debug)] pub struct DaemonClient { /// The write half. The reader thread owns the other. diff --git a/src/daemon/client_tests.rs b/src/daemon/client_tests.rs index 870a1bc7..55b95582 100644 --- a/src/daemon/client_tests.rs +++ b/src/daemon/client_tests.rs @@ -33,9 +33,10 @@ fn daemon(dir: &tempfile::TempDir, repos: &[String]) -> TestDaemon { persist: false, startup_commands: Vec::new(), hot: crate::config::AgentIndicatorConfig::default(), - prefs: PrefsStore::at(std::path::PathBuf::from( - "/nonexistent/nightcrow/viewer.json", - )), + // In the test's own directory, beside the socket: opening a repository + // records it as the active one, so this file is written, and a path + // shared with another test would make the two one session. + prefs: PrefsStore::at(dir.path().join("viewer.json")), })); let session = crate::daemon::serve::start(state).expect("starts the watcher"); std::thread::spawn(move || crate::daemon::serve::serve(listener, session)); diff --git a/src/daemon/serve_tests/accent.rs b/src/daemon/serve_tests/accent.rs index 7b70a2c9..c819da00 100644 --- a/src/daemon/serve_tests/accent.rs +++ b/src/daemon/serve_tests/accent.rs @@ -53,6 +53,16 @@ fn an_accent_past_the_end_of_the_cycle_wraps_rather_than_being_refused() { accent: crate::config::Accent::ALL.len() + 2, }); + // Read back rather than waited for, which is the claim above: the watcher + // announces a *difference*, so a session already painted in the colour the + // index wraps to answers the change with silence — and a test waiting for a + // frame nobody owes it would sit out the read timeout and blame the wrap. A + // list is owed an answer whatever the accent is. This connection's requests + // are served in order, so the accent is already stored when the list is + // recorded as owed, and either frame that can come back — the announcement + // or the answer — carries what was stored. + client.send(ClientMessage::ListRepos); + assert_eq!(client.next_accent(), 2); drop(repo); } diff --git a/src/daemon/serve_tests/harness.rs b/src/daemon/serve_tests/harness.rs index a31d3b50..4eaefd1f 100644 --- a/src/daemon/serve_tests/harness.rs +++ b/src/daemon/serve_tests/harness.rs @@ -33,7 +33,10 @@ pub(super) fn daemon(dir: &tempfile::TempDir, repos: &[String]) -> TestDaemon { let path = dir.path().join("d.sock"); let socket = DaemonSocket::bind(&path).expect("binds"); let listener = socket.listener().try_clone().expect("clones"); - let state = crate::test_util::session_state(repos); + // The same directory the socket is in, so this session's preferences — the + // accent among them — belong to this test rather than to whichever one wrote + // them last. + let state = crate::test_util::session_state(repos, dir.path()); let served = std::sync::Arc::clone(&state); let session = crate::daemon::serve::start(served).expect("starts the watcher"); std::thread::spawn(move || crate::daemon::serve::serve(listener, session)); diff --git a/src/plugin/guard.rs b/src/plugin/guard.rs index b268fe4b..9fde4e3e 100644 --- a/src/plugin/guard.rs +++ b/src/plugin/guard.rs @@ -5,10 +5,14 @@ //! [`Guard::judge`], which returns either an [`Approved`] action naming a real //! [`PaneId`] or a [`Refused`] saying why not. There is no other way through: a //! [`PluginCommand`] carries a token, and only the guard turns one into a pane. +//! +//! That includes gaining a pane in the first place: `guard_watch` beside this +//! file holds the only rule that can widen what a plugin is allowed to see. use super::guard_budget::{Budgets, RateAction, RateLimits}; use super::guard_refusal::Refused; use super::guard_text::{is_forbidden_control, truncate_message}; +use super::guard_watch::judge_watch; use super::protocol::{LogLevel, MAX_INPUT_BYTES, PluginCommand}; use crate::backend::slot::resume_command_line; use crate::backend::{PaneGeneration, PaneId, PaneToken}; @@ -22,8 +26,17 @@ use std::time::{Duration, Instant}; pub struct PaneFacts { pub pane: PaneId, pub generation: PaneGeneration, - /// The pane's `[[startup_command]]` named *this* plugin. + /// This plugin already has the pane, by opt-in or by an earlier + /// [`PluginCommand::WatchPane`]. pub opted_in: bool, + /// Some *other* plugin has the pane. Distinct from `!opted_in`, which is + /// equally true of a pane no plugin has at all — and those two answer + /// [`PluginCommand::WatchPane`] differently. + pub watched_by_another: bool, + /// The requesting plugin's `watch_on_signal`: whether the operator allowed + /// it to be given a pane it was never named by. A property of the plugin + /// rather than of the pane, carried here so the rules stay one struct wide. + pub may_watch_on_signal: bool, /// The pane's process is still running. pub alive: bool, /// Since the pane last produced output. @@ -57,6 +70,11 @@ pub enum Approved { deadline_epoch: Option, attempt: u32, }, + /// The plugin may be given this pane. Recording that, and telling the + /// plugin, is the caller's job. + WatchPane { + pane: PaneId, + }, Log { level: LogLevel, message: String, @@ -127,6 +145,10 @@ impl Guard { let facts = pane_facts(&token, generation, facts)?; self.judge_relaunch(&token, facts, resume_args, allowed_resume_flags, now) } + // Deliberately outside `pane_facts`: this is the one command whose + // whole point is a pane that has *not* opted in, and it names no + // generation to check. + PluginCommand::WatchPane { token, .. } => judge_watch(&token, facts), PluginCommand::Status { token, generation, @@ -183,7 +205,8 @@ impl Guard { code: bad as u32, }); } - self.spend(token, pane, RateAction::SendInput, now)?; + self.budgets + .spend(token, pane, RateAction::SendInput, &self.limits, now)?; Ok(Approved::SendInput { pane, data: data.into_bytes(), @@ -204,6 +227,15 @@ impl Guard { // recovery acts on live panes, relaunch only on exited ones. return Err(Refused::PaneStillRunning { pane }); } + if facts.launch_command.is_none() { + // A bare shell. Putting a process back here would start the shell + // again, not whatever the person ran inside it, and the resume + // arguments would have nothing to attach to — so the pane's only + // recovery is the one typed into it while it is still alive. Checked + // before `resume_command_line`, which also refuses this, so the log + // says the pane was never relaunchable rather than blaming the args. + return Err(Refused::NoLaunchCommand { pane }); + } let command_line = resume_command_line( facts.launch_command.as_deref(), &resume_args, @@ -213,42 +245,14 @@ impl Guard { pane, reason: e.to_string(), })?; - self.spend(token, pane, RateAction::Relaunch, now)?; + self.budgets + .spend(token, pane, RateAction::Relaunch, &self.limits, now)?; Ok(Approved::Relaunch { pane, resume_args, command_line, }) } - - /// Charge the budget, as the last check before approval. - /// - /// Only approvals spend, deliberately: the budget bounds what a plugin - /// *does* to a pane, and a refused command did nothing. Charging refusals - /// would let noise — a stale generation the plugin could not have known - /// about, a flag config forbids — eat the budget a legitimate action needs, - /// losing the pane's one real attempt to a race. Spam is bounded elsewhere - /// and more cheaply: the outbound queue drops and every refusal is logged. - fn spend( - &mut self, - token: &PaneToken, - pane: PaneId, - action: RateAction, - now: Instant, - ) -> Result<(), Refused> { - if self.budgets.try_spend(token, action, &self.limits, now) { - return Ok(()); - } - Err(Refused::RateLimited { - pane, - action, - limit: match action { - RateAction::SendInput => self.limits.max_sends_per_window, - RateAction::Relaunch => self.limits.max_relaunches_per_window, - }, - window: self.limits.window, - }) - } } /// Rules 2, 3 and 4: the pane must exist, have opted in, and be the same spawn. diff --git a/src/plugin/guard_budget.rs b/src/plugin/guard_budget.rs index c17c1b27..4d1cdafd 100644 --- a/src/plugin/guard_budget.rs +++ b/src/plugin/guard_budget.rs @@ -10,7 +10,8 @@ //! on every exit would then loop with nothing to stop it. The token is what //! survives a relaunch, so it is what the ceiling has to be attached to. -use crate::backend::PaneToken; +use super::guard_refusal::Refused; +use crate::backend::{PaneId, PaneToken}; use std::collections::HashMap; use std::time::{Duration, Instant}; @@ -76,7 +77,7 @@ impl Budgets { /// Nothing is recorded when there is not, so a refusal here costs the slot /// nothing beyond the refusal itself. Each list is capped at its limit by /// construction, so this stays O(limit). - pub(super) fn try_spend( + fn try_spend( &mut self, token: &PaneToken, action: RateAction, @@ -96,6 +97,36 @@ impl Budgets { true } + /// Charge the budget, as the last check before an approval. + /// + /// Only approvals spend, deliberately: the budget bounds what a plugin + /// *does* to a pane, and a refused command did nothing. Charging refusals + /// would let noise — a stale generation the plugin could not have known + /// about, a flag config forbids — eat the budget a legitimate action needs, + /// losing the pane's one real attempt to a race. Spam is bounded elsewhere + /// and more cheaply: the outbound queue drops and every refusal is logged. + pub(super) fn spend( + &mut self, + token: &PaneToken, + pane: PaneId, + action: RateAction, + limits: &RateLimits, + now: Instant, + ) -> Result<(), Refused> { + if self.try_spend(token, action, limits, now) { + return Ok(()); + } + Err(Refused::RateLimited { + pane, + action, + limit: match action { + RateAction::SendInput => limits.max_sends_per_window, + RateAction::Relaunch => limits.max_relaunches_per_window, + }, + window: limits.window, + }) + } + pub(super) fn clear(&mut self, token: &PaneToken) { self.0.remove(token); } diff --git a/src/plugin/guard_refusal.rs b/src/plugin/guard_refusal.rs index 5729e0f5..fc9bd041 100644 --- a/src/plugin/guard_refusal.rs +++ b/src/plugin/guard_refusal.rs @@ -38,8 +38,19 @@ pub enum Refused { ControlCharacter { pane: PaneId, code: u32 }, /// A relaunch for a pane whose process is still running. PaneStillRunning { pane: PaneId }, + /// A relaunch for a pane the host never launched a command for. + /// + /// Its own reason rather than one of `ResumeArgsRejected`'s, because nothing + /// about the arguments is wrong: the pane is a bare shell, so there is no + /// invocation to append them to and no amount of retrying changes that. + NoLaunchCommand { pane: PaneId }, /// The resume arguments did not survive the command-line rules. ResumeArgsRejected { pane: PaneId, reason: String }, + /// A plugin asked for a pane it was never named by, without the config + /// switch that allows it. + WatchNotAllowed { pane: PaneId, token: PaneToken }, + /// A plugin asked for a pane another plugin already has. + PaneWatchedByAnother { pane: PaneId }, RateLimited { pane: PaneId, action: RateAction, @@ -91,9 +102,22 @@ impl fmt::Display for Refused { "pane {pane} is still running, so it cannot be relaunched" ) } + Self::NoLaunchCommand { pane } => write!( + f, + "pane {pane} was launched with no command, so there is nothing to relaunch" + ), Self::ResumeArgsRejected { pane, reason } => { write!(f, "relaunch of pane {pane} refused: {reason}") } + Self::WatchNotAllowed { pane, token } => write!( + f, + "pane {pane} (token {}) was not opted in, and this plugin's config \ + does not let it watch a pane on a signal", + token.as_str() + ), + Self::PaneWatchedByAnother { pane } => { + write!(f, "pane {pane} is already watched by another plugin") + } Self::RateLimited { pane, action, diff --git a/src/plugin/guard_tests/mod.rs b/src/plugin/guard_tests/mod.rs index e939b633..2c8cd688 100644 --- a/src/plugin/guard_tests/mod.rs +++ b/src/plugin/guard_tests/mod.rs @@ -8,6 +8,7 @@ mod budget; mod identity; mod input; mod relaunch; +mod watch; pub(super) const PANE: PaneId = 4; pub(super) const GENERATION: PaneGeneration = 9; @@ -28,12 +29,27 @@ pub(super) fn facts() -> PaneFacts { pane: PANE, generation: GENERATION, opted_in: true, + watched_by_another: false, + may_watch_on_signal: false, alive: true, idle: MIN_IDLE, launch_command: Some(LAUNCH.to_string()), } } +/// A live pane no plugin has yet, in a session where the asking plugin is +/// allowed to ask — the one shape [`watch`] is meant to be approved for. +pub(super) fn adoptable_facts() -> PaneFacts { + PaneFacts { + opted_in: false, + may_watch_on_signal: true, + // A pane somebody started a CLI in by hand: the host launched no command + // of its own in it. + launch_command: None, + ..facts() + } +} + /// The same pane after its process ended, which is what a relaunch needs. pub(super) fn exited_facts() -> PaneFacts { PaneFacts { @@ -72,6 +88,13 @@ pub(super) fn status(token: &PaneToken) -> PluginCommand { } } +pub(super) fn watch(token: &PaneToken) -> PluginCommand { + PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token: token.clone(), + } +} + pub(super) fn flags(names: &[&str]) -> Vec { names.iter().map(|n| n.to_string()).collect() } diff --git a/src/plugin/guard_tests/relaunch.rs b/src/plugin/guard_tests/relaunch.rs index f62e461c..674850a2 100644 --- a/src/plugin/guard_tests/relaunch.rs +++ b/src/plugin/guard_tests/relaunch.rs @@ -1,4 +1,5 @@ -//! Rules 8-9: when a pane's process may be replaced, and with what. +//! Rules 8-9: when a pane's process may be replaced, and with what — including +//! the pane that has no launch command to replace it with. use super::*; @@ -84,13 +85,56 @@ fn a_relaunch_with_an_allowed_flag_carries_the_built_command_line() { #[test] fn relaunching_a_pane_with_no_configured_command_is_refused() { + // Its own reason, not one about the arguments: a bare shell is a pane whose + // process cannot be put back at all, which is what a plugin given a pane on a + // signal has to be told about the only pane it will ever have. let mut g = guard(); let facts = PaneFacts { launch_command: None, ..exited_facts() }; - assert!(matches!( - g.judge(relaunch(&token(), &[]), Some(&facts), &[], Instant::now()), - Err(Refused::ResumeArgsRejected { pane: PANE, .. }) - )); + assert_eq!( + g.judge(relaunch(&token(), &[]), Some(&facts), &[], Instant::now()) + .expect_err("refused"), + Refused::NoLaunchCommand { pane: PANE } + ); +} + +#[test] +fn a_relaunch_of_a_pane_with_no_command_is_refused_whatever_flags_are_allowed() { + // The flag list is what normally decides a relaunch, so the refusal must not + // be something a generous `allowed_resume_flags` can talk its way past. + let mut g = guard(); + let facts = PaneFacts { + launch_command: None, + ..exited_facts() + }; + assert_eq!( + g.judge( + relaunch(&token(), &["--resume", "abc123"]), + Some(&facts), + &flags(&["--resume"]), + Instant::now() + ) + .expect_err("refused"), + Refused::NoLaunchCommand { pane: PANE } + ); +} + +#[test] +fn a_refused_relaunch_of_a_bare_shell_costs_no_relaunch_budget() { + // Nothing was done to the pane, and a plugin that keeps asking must not be + // able to exhaust an allowance a legitimate pane would need. + let mut g = guard(); + let t = token(); + let now = Instant::now(); + let limits = RateLimits::default(); + let facts = PaneFacts { + launch_command: None, + ..exited_facts() + }; + for _ in 0..limits.max_relaunches_per_window + 1 { + assert!(g.judge(relaunch(&t, &[]), Some(&facts), &[], now).is_err()); + } + assert_eq!(g.budgets.spent(&t, RateAction::Relaunch, &limits, now), 0); } diff --git a/src/plugin/guard_tests/watch.rs b/src/plugin/guard_tests/watch.rs new file mode 100644 index 00000000..431dddb5 --- /dev/null +++ b/src/plugin/guard_tests/watch.rs @@ -0,0 +1,130 @@ +//! Rule 12: when a plugin may be given a pane nobody handed it. + +use super::*; + +#[test] +fn a_watch_request_for_a_live_unclaimed_pane_is_approved() { + let mut g = guard(); + assert_eq!( + g.judge( + watch(&token()), + Some(&adoptable_facts()), + &[], + Instant::now() + ) + .expect("allowed"), + Approved::WatchPane { pane: PANE } + ); +} + +#[test] +fn a_watch_request_for_a_token_with_no_live_pane_is_refused() { + // The token could not be resolved, so the sender is not inside any pane this + // host owns — another nightcrow session's helper, most likely. + let mut g = guard(); + let t = token(); + assert_eq!( + g.judge(watch(&t), None, &[], Instant::now()) + .expect_err("refused"), + Refused::UnknownPane { token: t } + ); +} + +#[test] +fn a_watch_request_from_a_plugin_the_config_did_not_allow_is_refused() { + // The switch is the whole difference between this and enumerating panes, so + // its absence has to refuse even a request that is otherwise perfect. + let mut g = guard(); + let t = token(); + let facts = PaneFacts { + may_watch_on_signal: false, + ..adoptable_facts() + }; + assert_eq!( + g.judge(watch(&t), Some(&facts), &[], Instant::now()) + .expect_err("refused"), + Refused::WatchNotAllowed { + pane: PANE, + token: t + } + ); +} + +#[test] +fn a_watch_request_for_a_pane_another_plugin_watches_is_refused() { + let mut g = guard(); + let facts = PaneFacts { + watched_by_another: true, + ..adoptable_facts() + }; + assert_eq!( + g.judge(watch(&token()), Some(&facts), &[], Instant::now()) + .expect_err("refused"), + Refused::PaneWatchedByAnother { pane: PANE } + ); +} + +#[test] +fn a_watch_request_for_a_pane_whose_process_has_exited_is_refused() { + // Nothing left to watch: the only recovery such a pane can be given is typed + // into a live process, and this one has none. + let mut g = guard(); + let facts = PaneFacts { + alive: false, + ..adoptable_facts() + }; + assert_eq!( + g.judge(watch(&token()), Some(&facts), &[], Instant::now()) + .expect_err("refused"), + Refused::PaneNotRunning { pane: PANE } + ); +} + +#[test] +fn a_watch_request_for_a_pane_this_plugin_already_has_is_approved_again() { + // How a plugin recovers from having been given an opted-in pane it could not + // recognise: asking again is what gets it another `PaneOpened`. + let mut g = guard(); + let facts = PaneFacts { + opted_in: true, + ..adoptable_facts() + }; + assert_eq!( + g.judge(watch(&token()), Some(&facts), &[], Instant::now()) + .expect("allowed"), + Approved::WatchPane { pane: PANE } + ); +} + +#[test] +fn a_watch_request_does_not_spend_the_panes_input_budget() { + // Being given a pane is not something done *to* the pane. Charging it would + // spend the allowance the recovery it unlocks is about to need. + let mut g = guard(); + let t = token(); + let now = Instant::now(); + let limits = RateLimits::default(); + for _ in 0..limits.max_sends_per_window * 2 { + g.judge(watch(&t), Some(&adoptable_facts()), &[], now) + .expect("allowed"); + } + assert_eq!(g.budgets.spent(&t, RateAction::SendInput, &limits, now), 0); + assert_eq!(g.budgets.spent(&t, RateAction::Relaunch, &limits, now), 0); +} + +#[test] +fn a_watch_request_names_no_generation_so_it_cannot_be_stale() { + // A plugin asking for a pane it has never been told about cannot know which + // spawn it is looking at; the `PaneOpened` it is answered with is what says. + // A generation rule here would therefore refuse every honest request. + assert_eq!(watch(&token()).generation(), None); + let mut g = guard(); + let facts = PaneFacts { + generation: GENERATION + 5, + ..adoptable_facts() + }; + assert!( + g.judge(watch(&token()), Some(&facts), &[], Instant::now()) + .is_ok() + ); +} diff --git a/src/plugin/guard_watch.rs b/src/plugin/guard_watch.rs new file mode 100644 index 00000000..83f3666c --- /dev/null +++ b/src/plugin/guard_watch.rs @@ -0,0 +1,62 @@ +//! Rule 12: when a plugin may be given a pane nobody handed it. +//! +//! Every other rule in this layer starts from a pane the operator already +//! assigned. This one is the single place an assignment can be *created* at +//! runtime, so it is kept apart from the rest and reads as one list of +//! conditions rather than as a branch inside a larger judgement. +//! +//! What makes it safe is where the token came from. A pane token is random, is +//! minted per slot, and is put only into that pane's child environment, so a +//! process able to quote one is a process running inside that pane. The pane's +//! own occupant asking for a watcher is a different thing from a plugin +//! enumerating the session, and only the first is allowed here — nothing in this +//! file looks at a list of panes. +//! +//! Still not authority by itself: the operator's config switch has to be on, and +//! a pane already spoken for is not taken away from the plugin that has it. + +use super::guard::{Approved, PaneFacts}; +use super::guard_refusal::Refused; +use crate::backend::PaneToken; + +/// Decide one [`PluginCommand::WatchPane`](super::protocol::PluginCommand). +/// +/// Takes no clock and charges no budget. Being given a pane is not something +/// done *to* the pane — it changes who is told about it, and every act that +/// follows is charged when it is asked for. Charging here would spend the very +/// allowance the recovery this unlocks is about to need. +pub(super) fn judge_watch( + token: &PaneToken, + facts: Option<&PaneFacts>, +) -> Result { + let Some(facts) = facts else { + // Overwhelmingly the ordinary case: a token from another nightcrow + // session's panes, reaching this plugin because both are the same user. + return Err(Refused::UnknownPane { + token: token.clone(), + }); + }; + if !facts.may_watch_on_signal { + return Err(Refused::WatchNotAllowed { + pane: facts.pane, + token: token.clone(), + }); + } + if facts.watched_by_another { + // One pane, one watcher. Two plugins driving the same keyboard would + // interleave their recoveries, and the second would be acting on a pane + // whose state the first is changing underneath it. + return Err(Refused::PaneWatchedByAnother { pane: facts.pane }); + } + if !facts.alive { + // Nothing left to watch: the slot may still exist, but the process that + // proved what the pane was running has gone, and the only recovery a + // pane taken on this way can get is typed into a live process. + return Err(Refused::PaneNotRunning { pane: facts.pane }); + } + // Deliberately allowed when this plugin already has the pane. That happens + // when an opted-in pane was handed over but its occupant could not be + // recognised from the command line, and asking again is how the plugin gets + // the `PaneOpened` it needs to try once more. + Ok(Approved::WatchPane { pane: facts.pane }) +} diff --git a/src/plugin/mod.rs b/src/plugin/mod.rs index aa087264..8f7eb3cc 100644 --- a/src/plugin/mod.rs +++ b/src/plugin/mod.rs @@ -6,6 +6,12 @@ //! pane identity it names — before anything acts on it. This module is types //! and parsing only: no IO, no process spawning, no threads. //! +//! Which panes a plugin may see is settled here too. It is normally the +//! operator's list, and a plugin can add to it only by quoting a pane's own +//! token back — something only a process inside that pane can know — and only +//! where the operator turned that on. A plugin is never handed a list of panes +//! to choose from. +//! //! The contract is deliberately provider-agnostic. It speaks of panes, output, //! idleness and relaunching, and never of any particular tool; which program a //! pane runs stays the host's knowledge, which is what keeps @@ -23,6 +29,7 @@ pub mod registry; mod guard_budget; mod guard_refusal; mod guard_text; +mod guard_watch; mod host_pump; pub use guard::{Approved, Guard, PaneFacts}; diff --git a/src/plugin/protocol.rs b/src/plugin/protocol.rs index 558a2611..3b59f1c9 100644 --- a/src/plugin/protocol.rs +++ b/src/plugin/protocol.rs @@ -17,7 +17,12 @@ use serde::{Deserialize, Serialize}; /// Bumped when a field changes meaning. A plugin built against a version the /// host does not speak is refused rather than half-understood. -pub const PROTOCOL_VERSION: u32 = 1; +/// +/// 2 added [`PluginCommand::WatchPane`]. A version-1 plugin cannot ask for a +/// pane it was never named by, so an old build talking to this host would be +/// silently less capable rather than wrong — the bump is what makes that +/// difference loud instead. +pub const PROTOCOL_VERSION: u32 = 2; /// Longest line the host will read from a plugin. /// @@ -128,6 +133,15 @@ pub enum PluginCommand { deadline_epoch: Option, attempt: u32, }, + /// Ask to be given the pane this token names, so its events start arriving. + /// + /// The token is the whole of the argument: it is minted per pane and reaches + /// only that pane's child processes, so a plugin can present one merely by + /// having been told it from inside the pane. Carries no generation — a + /// plugin asking for a pane it has never been told about cannot know which + /// spawn it is looking at, and the [`PluginEvent::PaneOpened`] the host + /// answers with is what says. + WatchPane { v: u32, token: PaneToken }, /// A line for the host's log. Log { v: u32, @@ -152,6 +166,7 @@ impl PluginCommand { Self::SendInput { v, .. } | Self::Relaunch { v, .. } | Self::Status { v, .. } + | Self::WatchPane { v, .. } | Self::Log { v, .. } => *v, } } @@ -163,18 +178,23 @@ impl PluginCommand { match self { Self::SendInput { token, .. } | Self::Relaunch { token, .. } - | Self::Status { token, .. } => Some(token), + | Self::Status { token, .. } + | Self::WatchPane { token, .. } => Some(token), Self::Log { .. } => None, } } /// Which spawn of the slot this addresses, paired with [`Self::token`]. + /// + /// `None` for [`Self::WatchPane`] as well as [`Self::Log`]: naming a slot + /// and naming a spawn within it are separate claims, and that command makes + /// only the first. pub fn generation(&self) -> Option { match self { Self::SendInput { generation, .. } | Self::Relaunch { generation, .. } | Self::Status { generation, .. } => Some(*generation), - Self::Log { .. } => None, + Self::WatchPane { .. } | Self::Log { .. } => None, } } @@ -243,5 +263,5 @@ pub fn decode_command(line: &str) -> Result { } #[cfg(test)] -#[path = "protocol_tests.rs"] +#[path = "protocol_tests/mod.rs"] mod tests; diff --git a/src/plugin/protocol_tests/bounds.rs b/src/plugin/protocol_tests/bounds.rs new file mode 100644 index 00000000..59ad9d70 --- /dev/null +++ b/src/plugin/protocol_tests/bounds.rs @@ -0,0 +1,106 @@ +//! What `decode_command` refuses, and where each limit sits. +//! +//! Split from the shape tests beside it: those pin the wire's *names*, these pin +//! the wire's *edges*. A plugin is untrusted input on a stream with no length +//! prefix, so every one of these is a bound the host would otherwise not have. + +use super::{GENERATION, TOKEN, token}; +use crate::plugin::protocol::{ + MAX_INPUT_BYTES, MAX_LINE_BYTES, PROTOCOL_VERSION, PluginCommand, decode_command, is_blank_line, +}; + +#[test] +fn a_command_from_another_protocol_version_is_refused_and_both_versions_named() { + let line = format!( + r#"{{"cmd":"log","v":{},"level":"info","message":"hi"}}"#, + PROTOCOL_VERSION + 1 + ); + let err = decode_command(&line).expect_err("refused"); + let message = err.to_string(); + assert!( + message.contains(&(PROTOCOL_VERSION + 1).to_string()) + && message.contains(&PROTOCOL_VERSION.to_string()), + "message names only one version: {message}" + ); +} + +#[test] +fn a_line_over_the_length_limit_is_refused_before_it_is_parsed() { + // Refused on length alone, so nothing here depends on the payload being + // valid JSON — an unbounded line must not become an unbounded parse. + let line = "x".repeat(MAX_LINE_BYTES + 1); + let message = decode_command(&line).expect_err("refused").to_string(); + assert!(message.contains(&MAX_LINE_BYTES.to_string()), "{message}"); +} + +#[test] +fn a_line_at_the_length_limit_is_parsed() { + let padding = "p".repeat(MAX_INPUT_BYTES); + let line = format!(r#"{{"cmd":"log","v":2,"level":"debug","message":"{padding}"}}"#); + assert!(line.len() <= MAX_LINE_BYTES); + assert!(decode_command(&line).is_ok()); +} + +#[test] +fn an_unknown_cmd_is_refused_rather_than_guessed() { + let message = decode_command(r#"{"cmd":"drop_everything","v":2}"#) + .expect_err("refused") + .to_string(); + assert!(message.contains("drop_everything"), "{message}"); + assert!(message.contains("cmd"), "{message}"); +} + +#[test] +fn a_command_missing_a_required_field_is_refused() { + assert!(decode_command(r#"{"cmd":"send_input","v":2}"#).is_err()); +} + +#[test] +fn a_watch_pane_command_from_the_previous_protocol_version_is_refused() { + // The command did not exist at version 1, so a build that speaks 1 cannot + // have sent it — accepting one would be honouring a claim about a contract + // the sender never agreed to. + let line = format!(r#"{{"cmd":"watch_pane","v":1,"token":"{TOKEN}"}}"#); + assert!(decode_command(&line).is_err()); +} + +#[test] +fn send_input_data_over_the_input_limit_is_refused() { + let command = PluginCommand::SendInput { + v: PROTOCOL_VERSION, + token: token(), + generation: GENERATION, + data: "y".repeat(MAX_INPUT_BYTES + 1), + }; + let message = command.validate().expect_err("refused").to_string(); + assert!(message.contains(&MAX_INPUT_BYTES.to_string()), "{message}"); + + let line = serde_json::to_string(&command).unwrap(); + assert!( + line.len() <= MAX_LINE_BYTES, + "the length cap would mask this" + ); + assert!(decode_command(&line).is_err()); +} + +#[test] +fn send_input_data_at_the_input_limit_is_accepted() { + let command = PluginCommand::SendInput { + v: PROTOCOL_VERSION, + token: token(), + generation: GENERATION, + data: "y".repeat(MAX_INPUT_BYTES), + }; + assert!(command.validate().is_ok()); +} + +#[test] +fn a_blank_line_is_recognised_as_blank_and_carries_no_command() { + for line in ["", " ", "\t", "\r\n"] { + assert!(is_blank_line(line), "not seen as blank: {line:?}"); + assert!(decode_command(line).is_err()); + } + assert!(!is_blank_line( + r#"{"cmd":"log","v":2,"level":"info","message":"x"}"# + )); +} diff --git a/src/plugin/protocol_tests.rs b/src/plugin/protocol_tests/mod.rs similarity index 63% rename from src/plugin/protocol_tests.rs rename to src/plugin/protocol_tests/mod.rs index f32bd984..4a974be6 100644 --- a/src/plugin/protocol_tests.rs +++ b/src/plugin/protocol_tests/mod.rs @@ -1,9 +1,11 @@ -use super::{ - LogLevel, MAX_INPUT_BYTES, MAX_LINE_BYTES, PROTOCOL_VERSION, PluginCommand, PluginEvent, - decode_command, encode_event, is_blank_line, -}; +//! The wire contract, and the fixtures both halves of it share: this file pins +//! the shapes and names, `bounds` beside it pins what is refused. + +use super::{LogLevel, PROTOCOL_VERSION, PluginCommand, PluginEvent, decode_command, encode_event}; use crate::backend::{PaneGeneration, PaneToken}; +mod bounds; + const TOKEN: &str = "0123456789abcdef0123456789abcdef"; const GENERATION: PaneGeneration = 2; @@ -77,6 +79,10 @@ fn every_command() -> Vec { deadline_epoch: Some(1_700_000_000), attempt: 1, }, + PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token: token(), + }, PluginCommand::Log { v: PROTOCOL_VERSION, level: LogLevel::Warn, @@ -140,7 +146,7 @@ fn an_events_json_shape_is_the_wire_contract() { assert_eq!( encode_event(&event).unwrap(), format!( - r#"{{"event":"pane_idle","v":1,"token":"{TOKEN}","generation":2,"idle_ms":30000}}"# + r#"{{"event":"pane_idle","v":2,"token":"{TOKEN}","generation":2,"idle_ms":30000}}"# ) ); } @@ -155,7 +161,7 @@ fn a_commands_json_shape_is_the_wire_contract() { }; assert_eq!( serde_json::to_string(&command).unwrap(), - format!(r#"{{"cmd":"send_input","v":1,"token":"{TOKEN}","generation":2,"data":"go"}}"#) + format!(r#"{{"cmd":"send_input","v":2,"token":"{TOKEN}","generation":2,"data":"go"}}"#) ); } @@ -173,93 +179,6 @@ fn a_log_levels_json_shape_is_lowercase() { ); } -#[test] -fn a_command_from_another_protocol_version_is_refused_and_both_versions_named() { - let line = format!( - r#"{{"cmd":"log","v":{},"level":"info","message":"hi"}}"#, - PROTOCOL_VERSION + 1 - ); - let err = decode_command(&line).expect_err("refused"); - let message = err.to_string(); - assert!( - message.contains(&(PROTOCOL_VERSION + 1).to_string()) - && message.contains(&PROTOCOL_VERSION.to_string()), - "message names only one version: {message}" - ); -} - -#[test] -fn a_line_over_the_length_limit_is_refused_before_it_is_parsed() { - // Refused on length alone, so nothing here depends on the payload being - // valid JSON — an unbounded line must not become an unbounded parse. - let line = "x".repeat(MAX_LINE_BYTES + 1); - let message = decode_command(&line).expect_err("refused").to_string(); - assert!(message.contains(&MAX_LINE_BYTES.to_string()), "{message}"); -} - -#[test] -fn a_line_at_the_length_limit_is_parsed() { - let padding = "p".repeat(MAX_INPUT_BYTES); - let line = format!(r#"{{"cmd":"log","v":1,"level":"debug","message":"{padding}"}}"#); - assert!(line.len() <= MAX_LINE_BYTES); - assert!(decode_command(&line).is_ok()); -} - -#[test] -fn an_unknown_cmd_is_refused_rather_than_guessed() { - let message = decode_command(r#"{"cmd":"drop_everything","v":1}"#) - .expect_err("refused") - .to_string(); - assert!(message.contains("drop_everything"), "{message}"); - assert!(message.contains("cmd"), "{message}"); -} - -#[test] -fn a_command_missing_a_required_field_is_refused() { - assert!(decode_command(r#"{"cmd":"send_input","v":1}"#).is_err()); -} - -#[test] -fn send_input_data_over_the_input_limit_is_refused() { - let command = PluginCommand::SendInput { - v: PROTOCOL_VERSION, - token: token(), - generation: GENERATION, - data: "y".repeat(MAX_INPUT_BYTES + 1), - }; - let message = command.validate().expect_err("refused").to_string(); - assert!(message.contains(&MAX_INPUT_BYTES.to_string()), "{message}"); - - let line = serde_json::to_string(&command).unwrap(); - assert!( - line.len() <= MAX_LINE_BYTES, - "the length cap would mask this" - ); - assert!(decode_command(&line).is_err()); -} - -#[test] -fn send_input_data_at_the_input_limit_is_accepted() { - let command = PluginCommand::SendInput { - v: PROTOCOL_VERSION, - token: token(), - generation: GENERATION, - data: "y".repeat(MAX_INPUT_BYTES), - }; - assert!(command.validate().is_ok()); -} - -#[test] -fn a_blank_line_is_recognised_as_blank_and_carries_no_command() { - for line in ["", " ", "\t", "\r\n"] { - assert!(is_blank_line(line), "not seen as blank: {line:?}"); - assert!(decode_command(line).is_err()); - } - assert!(!is_blank_line( - r#"{"cmd":"log","v":1,"level":"info","message":"x"}"# - )); -} - #[test] fn only_a_pane_scoped_command_reports_an_identity() { for command in &every_command() { @@ -268,6 +187,13 @@ fn only_a_pane_scoped_command_reports_an_identity() { assert_eq!(command.token(), None); assert_eq!(command.generation(), None); } + // Names a slot but not a spawn within it: the plugin is asking about + // a pane the host has never described to it, so there is no + // generation it could honestly claim. + PluginCommand::WatchPane { .. } => { + assert_eq!(command.token(), Some(&token())); + assert_eq!(command.generation(), None); + } _ => { assert_eq!(command.token(), Some(&token())); assert_eq!(command.generation(), Some(GENERATION)); @@ -275,3 +201,17 @@ fn only_a_pane_scoped_command_reports_an_identity() { } } } + +#[test] +fn a_watch_pane_commands_json_shape_is_the_wire_contract() { + // Pinned literally, like the events above: a plugin that cannot be rebuilt + // with this host writes this exact line, and there is no generation in it. + let command = PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token: token(), + }; + assert_eq!( + serde_json::to_string(&command).unwrap(), + format!(r#"{{"cmd":"watch_pane","v":2,"token":"{TOKEN}"}}"#) + ); +} diff --git a/src/plugin/registry.rs b/src/plugin/registry.rs index 12964b66..4fbab8c6 100644 --- a/src/plugin/registry.rs +++ b/src/plugin/registry.rs @@ -1,8 +1,9 @@ //! The on-disk set of installed plugin executables (`~/.nightcrow/plugins`). //! //! Installing a binary here does not switch it on: the host only ever launches -//! a plugin that `config.toml` declares in a `[[plugin]]` table *and* that a -//! `[[startup_command]]` opted a pane into by name. That edit is left to the +//! a plugin that `config.toml` declares in a `[[plugin]]` table *and* that some +//! pane can reach — either a `[[startup_command]]` opted one in by name, or +//! `watch_on_signal` lets a provider signal name one. That edit is left to the //! user on purpose — a plugin can drive a pane's terminal, so the file that //! grants it that has to be one a person read. //! diff --git a/src/runtime/snapshot/worker.rs b/src/runtime/snapshot/worker.rs index 11d34f05..918f406b 100644 --- a/src/runtime/snapshot/worker.rs +++ b/src/runtime/snapshot/worker.rs @@ -91,7 +91,7 @@ impl Worker { } if awake && state.changed && due(state.last_read) { - if !self.read_once(&mut state) { + if !self.read_once(&mut state, &watch) { // The receiver is gone: nobody is left to read this. return; } @@ -153,6 +153,8 @@ impl Worker { /// reports and what [`wait`](Self::wait) reads to choose an interval. fn follow_git_dir(&self, state: &mut ReadState, roots: &mut Roots, watch: &mut Watches) { let Some(repo) = state.repo.as_ref() else { + // No handle to ask, and nothing to correct: the read that dropped it + // has already said what is being watched. return; }; // None means it is inside the work tree, where the recursive watch @@ -177,7 +179,10 @@ impl Worker { } /// Read the tree and send what it says. `false` once the receiver is gone. - fn read_once(&self, state: &mut ReadState) -> bool { + /// + /// `watch` is read for one answer only: what is left watching a directory + /// that turns out to hold no repository — see the failure below. + fn read_once(&self, state: &mut ReadState, watch: &Watches) -> bool { if state.reads_since_open >= REOPEN_REPO_EVERY_READS { state.repo = None; } @@ -188,9 +193,23 @@ impl Worker { state.reads_since_open = 0; } Err(err) => { - // `changed` is left standing, so the next permitted slot - // retries — a directory that is not a repository yet may - // become one. + // Counted as a read taken, `changed` and all. A directory that + // is not a repository yet may become one, but leaving + // `changed` standing to retry for that held the reader at + // `MIN_READ_INTERVAL` and sent this same error every second for + // as long as the session lived — the cost watching exists to + // remove, and enough on its own to make a test counting reads + // wait for a silence that never came. + // + // The watch is what notices instead: `git init` and a checkout + // appearing both write inside the tree being watched. And that + // watch is the whole of what can be watched here — the second + // one follows the git directory of a repository, and there is + // no repository — which is what earns the idle interval rather + // than the fixed fallback for a tree nothing reports on. See + // [`wait`](Self::wait). + state.changed = false; + self.watching.store(watch.tree.is_some(), Ordering::Release); state.last_read = Some(Instant::now()); let msg = SnapshotMsg::Err(format!("not a git repository: {err}")); return self.deliver(msg); @@ -259,3 +278,7 @@ impl Worker { fn due(last_read: Option) -> bool { last_read.is_none_or(|last| last.elapsed() >= MIN_READ_INTERVAL) } + +#[cfg(test)] +#[path = "worker_tests.rs"] +mod tests; diff --git a/src/runtime/snapshot/worker_tests.rs b/src/runtime/snapshot/worker_tests.rs new file mode 100644 index 00000000..a3ee95ef --- /dev/null +++ b/src/runtime/snapshot/worker_tests.rs @@ -0,0 +1,69 @@ +//! What the reader does with a directory that holds no repository. +//! +//! Beside the worker rather than with the other reader tests, which are already +//! at the length this project splits at, and borrowing their helpers: the claim +//! is the same kind — how many reads arrive, not what they say. + +use crate::runtime::snapshot::tests::{SETTLE, next_read, quiesce, reads_during}; +use crate::runtime::snapshot::{MIN_READ_INTERVAL, SnapshotChannel, SnapshotMsg}; +use crate::test_util::run_git; + +/// An empty directory, which no `git status` can be taken of. +fn not_a_repository() -> (tempfile::TempDir, String) { + let dir = tempfile::TempDir::new().unwrap(); + let path = dir.path().to_string_lossy().to_string(); + (dir, path) +} + +#[test] +fn a_path_with_no_repository_is_reported_once_rather_than_every_second() { + // A pane opened on a directory that is not a checkout — a path typed wrong, a + // clone that has not landed — used to be told so once a second for as long as + // it stayed open, which is exactly the cost watching rather than polling + // exists to remove. + let (dir, path) = not_a_repository(); + let channel = SnapshotChannel::spawn(&path); + + assert!( + matches!(next_read(&channel), SnapshotMsg::Err(_)), + "a directory with no repository in it is reported as an error" + ); + + // Asserted rather than assumed, as the tests next door do: with no watch the + // reader falls back to the fixed interval by design, and the count below would + // then fail for a reason that has nothing to do with what it is checking. + assert!( + channel.is_watching(), + "this directory could not be watched, so this machine cannot run this test" + ); + // Starting the watch owes a read for the gap it was not up for, and that read + // would be counted below as the once-a-second cadence this denies. Wait for the + // reader to actually go quiet rather than trusting it already is. + quiesce(&channel); + + assert_eq!( + reads_during(&channel, SETTLE), + 0, + "and reported once, not once per {MIN_READ_INTERVAL:?}" + ); + drop(dir); +} + +#[test] +fn a_repository_created_where_there_was_none_is_read() { + // The other half of the claim above: the reader goes quiet because the + // directory is watched, not because it gave up on it. `git init` writes + // inside that watch, so the repository is read when it appears rather than + // whenever the safety net next comes round. + let (dir, path) = not_a_repository(); + let channel = SnapshotChannel::spawn(&path); + assert!(matches!(next_read(&channel), SnapshotMsg::Err(_))); + + run_git(&path, &["init"]); + + assert!( + matches!(next_read(&channel), SnapshotMsg::Ok(..)), + "the repository that appeared must be read" + ); + drop(dir); +} diff --git a/src/runtime/snapshot_tests.rs b/src/runtime/snapshot_tests.rs index e4b104d8..d89b90a3 100644 --- a/src/runtime/snapshot_tests.rs +++ b/src/runtime/snapshot_tests.rs @@ -4,6 +4,9 @@ //! while nothing happens, so most of these are about *absence*: how many reads //! arrive, not what they say. They use short windows against a ten-second idle //! interval, so a slow machine delays them rather than failing them. +//! +//! The helpers that count reads are `pub(super)`: the same claim is made about a +//! path with no repository in it, and those tests sit beside the worker. use super::{IDLE_READ_INTERVAL, MIN_READ_INTERVAL, SnapshotChannel, SnapshotMsg}; use crate::test_util::{make_linked_worktree, make_repo, run_git}; @@ -12,11 +15,11 @@ use std::time::{Duration, Instant}; /// Long enough for a filesystem event to travel and a read to happen, without /// reaching the idle interval that would make the assertion meaningless. -const SETTLE: Duration = Duration::from_millis(2_500); +pub(super) const SETTLE: Duration = Duration::from_millis(2_500); /// Wait for one snapshot, or fail. Errors count: what is being timed is the read, /// not what it found. -fn next_read(channel: &SnapshotChannel) -> SnapshotMsg { +pub(super) fn next_read(channel: &SnapshotChannel) -> SnapshotMsg { let deadline = Instant::now() + SETTLE; while Instant::now() < deadline { match channel.try_recv() { @@ -41,7 +44,7 @@ fn next_read(channel: &SnapshotChannel) -> SnapshotMsg { /// this is meant to remove right back in the way. Still far below /// `IDLE_READ_INTERVAL`, so the safety net cannot be what breaks the silence, /// and far below what the caller then waits for a read. -fn quiesce(channel: &SnapshotChannel) { +pub(super) fn quiesce(channel: &SnapshotChannel) { let quiet_for = MIN_READ_INTERVAL * 3; // Only reached if reads never stop, which no test here can cause; a failure // of the reader itself, not of the timing. @@ -59,7 +62,7 @@ fn quiesce(channel: &SnapshotChannel) { } /// How many snapshots arrive over `window`. -fn reads_during(channel: &SnapshotChannel, window: Duration) -> usize { +pub(super) fn reads_during(channel: &SnapshotChannel, window: Duration) -> usize { let deadline = Instant::now() + window; let mut reads = 0; while Instant::now() < deadline { diff --git a/src/runtime/terminal/scroll.rs b/src/runtime/terminal/scroll.rs index 7ab521d9..300dc22f 100644 --- a/src/runtime/terminal/scroll.rs +++ b/src/runtime/terminal/scroll.rs @@ -154,9 +154,8 @@ impl TerminalState { pub(super) fn sync_scroll_pane(&mut self, id: PaneId) { let offset = self.scroll.get(&id).copied().unwrap_or(0); let actual = match self.emulators.get_mut(&id) { - // The emulator clamps the offset to the actual scrollback - // buffer size internally, so we can pass the full request - // through and read back what was applied. + // The emulator clamps internally, so pass the full request through + // and read back what was applied. Some(emulator) => emulator.set_scroll_offset(offset), None => return, }; diff --git a/src/runtime/tree_watch.rs b/src/runtime/tree_watch.rs index 5e778138..47d517c2 100644 --- a/src/runtime/tree_watch.rs +++ b/src/runtime/tree_watch.rs @@ -131,7 +131,6 @@ impl TreeWatcher { /// was deleted between the listing and this call) is skipped, not retried, /// and never enters `watched`. pub fn sync(&mut self, workdir: &Path, desired: &BTreeSet) { - // Remembered so `drain_changed` can make event paths repo-relative. self.root = Some(workdir.to_path_buf()); let Some(debouncer) = self.debouncer.as_mut() else { // Inert watcher: track intent only so behaviour is observable in @@ -140,7 +139,6 @@ impl TreeWatcher { return; }; let watcher = debouncer.watcher(); - // Drop paths no longer desired. let stale: Vec = self.watched.difference(desired).cloned().collect(); for rel in stale { let abs = join_rel(workdir, &rel); @@ -149,7 +147,6 @@ impl TreeWatcher { let _ = watcher.unwatch(&abs); self.watched.remove(&rel); } - // Add newly desired paths. let fresh: Vec = desired.difference(&self.watched).cloned().collect(); for rel in fresh { let abs = join_rel(workdir, &rel); diff --git a/src/runtime/tree_watch_tests.rs b/src/runtime/tree_watch_tests.rs index 4bb92b3f..b7daf002 100644 --- a/src/runtime/tree_watch_tests.rs +++ b/src/runtime/tree_watch_tests.rs @@ -11,7 +11,6 @@ fn inert_watcher_tracks_desired_set_without_os_calls() { w.sync(root, &desired); assert_eq!(w.watched, desired); - // Reconcile down to just the root. let mut smaller = BTreeSet::new(); smaller.insert(String::new()); w.sync(root, &smaller); @@ -52,7 +51,6 @@ fn drain_changed_reports_the_directories_that_changed_and_clears() { BTreeSet::from(["src".to_string(), String::new()]), "the repo root is the empty relative path" ); - // Drained: a second poll with nothing new reports no change. assert!(w.drain_changed().is_empty()); } diff --git a/src/test_util.rs b/src/test_util.rs index 04a54a83..a815e64e 100644 --- a/src/test_util.rs +++ b/src/test_util.rs @@ -56,13 +56,31 @@ pub fn make_linked_worktree() -> (TempDir, TempDir, String) { (main, elsewhere, tree) } -/// A session serving `repos`, with no browser listener bound. +/// A session serving `repos`, with no browser listener bound, keeping its +/// preferences in `prefs_dir`. /// /// The state, not the server: everything the daemon serves hangs off this, so a /// test can drive a real session — catalog, terminal hubs and all — without a -/// TCP port. Never persists, so tests cannot touch the real +/// TCP port. The workspace file is never written, so tests cannot touch the real /// `~/.nightcrow/workspace.json`. -pub fn session_state(repos: &[String]) -> std::sync::Arc { +/// +/// `prefs_dir` is required, and must be the caller's own temporary directory. +/// The accent and the active project *are* preferences, so a session driven from +/// a test writes this file — and every session pointed at one path is one +/// session: a test would begin in the colour whichever test ran last chose, and +/// asking for that same colour is no change at all, which the watcher answers +/// with silence rather than a frame. Pass the directory that already holds the +/// test's socket and the file goes away with the test. +/// +/// This replaced a single fixed path outside any temporary directory, chosen +/// because it was expected to be unwritable. It is unwritable for an ordinary +/// user, which is what CI runs as; a test suite running as root — the default in +/// a bare container — created it and shared it, and the accent tests then +/// depended on each other's order across whole runs. +pub fn session_state( + repos: &[String], + prefs_dir: &Path, +) -> std::sync::Arc { std::sync::Arc::new(crate::web::viewer::server::ViewerState::new( crate::web::viewer::server::ViewerOptions { bind: "127.0.0.1".parse().unwrap(), @@ -72,9 +90,7 @@ pub fn session_state(repos: &[String]) -> std::sync::Arc= scroll_start && flat_idx < visible_end { lines.push(Line::from(Span::styled( hunk.header.as_str(), diff --git a/src/ui/file_view.rs b/src/ui/file_view.rs index 5409b558..c74a217c 100644 --- a/src/ui/file_view.rs +++ b/src/ui/file_view.rs @@ -68,8 +68,6 @@ impl FileViewState { self.error = None; } - /// Largest legal `scroll` value: one less than `line_count`, or 0 when - /// the file is empty. pub fn max_scroll(&self) -> usize { self.line_count().saturating_sub(1) } diff --git a/src/ui/splash.rs b/src/ui/splash.rs index 2f8cb9f9..6ef9249c 100644 --- a/src/ui/splash.rs +++ b/src/ui/splash.rs @@ -54,7 +54,6 @@ pub fn draw(frame: &mut Frame, state: &SplashState, accent: Color) { ); let logo_h = LOGO.len() as u16; - // logo + gap + tagline + gap + bar let content_h = logo_h + 1 + 1 + 1 + 1; let outer = Layout::default() @@ -93,7 +92,6 @@ pub fn draw(frame: &mut Frame, state: &SplashState, accent: Color) { inner[0], ); - // Tagline let version = env!("CARGO_PKG_VERSION"); frame.render_widget( Paragraph::new(Line::from(vec![ @@ -109,7 +107,6 @@ pub fn draw(frame: &mut Frame, state: &SplashState, accent: Color) { inner[2], ); - // Loading bar let filled = ((progress * BAR_WIDTH as f64) as usize).min(BAR_WIDTH); let empty = BAR_WIDTH - filled; let bar = format!("{}{}", "█".repeat(filled), "░".repeat(empty)); diff --git a/src/web/viewer/server/tests/mod.rs b/src/web/viewer/server/tests/mod.rs index 3c0530a1..4d3abd1c 100644 --- a/src/web/viewer/server/tests/mod.rs +++ b/src/web/viewer/server/tests/mod.rs @@ -20,8 +20,11 @@ pub(super) fn server(paths: &[String]) -> ViewerServer { } /// `prefs_dir` keeps preference writes inside a temp directory. Left `None` -/// the store still points at the real `~/.nightcrow/viewer.json`, so any -/// test that *writes* a preference must pass one. +/// the store points at a fixed path every caller shares, so any test that +/// *writes* a preference must pass one — the path is not a safe elsewhere. It is +/// only unwritable for an ordinary user; a suite running as root, which is the +/// default in a bare container, creates it and then every session left without a +/// directory reads back what the last one stored. pub(super) fn server_with( paths: &[String], hot: crate::config::AgentIndicatorConfig, diff --git a/src/web/viewer/terminal/hub_events.rs b/src/web/viewer/terminal/hub_events.rs index a609e3ad..e7e3abcf 100644 --- a/src/web/viewer/terminal/hub_events.rs +++ b/src/web/viewer/terminal/hub_events.rs @@ -186,6 +186,11 @@ impl Plugins { // one plugin must not be able to act through another's // opt-in. opted_in: self.owners.get(&pane).is_some_and(|name| name == plugin), + // The same lookup read the other way, which is a different + // question: an unclaimed pane may be asked for, one somebody + // else claimed may not. + watched_by_another: self.owners.get(&pane).is_some_and(|name| name != plugin), + may_watch_on_signal: self.watch_on_signal.contains(plugin), alive: backend.is_process_alive(pane), idle: slot.idle_for(now), launch_command: slot.launch.command.clone(), diff --git a/src/web/viewer/terminal/hub_plugins.rs b/src/web/viewer/terminal/hub_plugins.rs index bdafdda3..6d7b3042 100644 --- a/src/web/viewer/terminal/hub_plugins.rs +++ b/src/web/viewer/terminal/hub_plugins.rs @@ -6,10 +6,13 @@ //! command queue the worker already drains, and the only way out is //! [`crate::plugin::Guard`]. //! -//! A pane appears here **only** if its `[[startup_command]]` named a plugin by -//! hand. A pane a client opened has no association and never gains one, which is -//! what makes "an arbitrary shell is never plugin-controlled" a property of the -//! code rather than a promise. +//! A pane appears here only two ways: its `[[startup_command]]` named a plugin +//! by hand, or a plugin asked for it by quoting the pane's own token and the +//! guard allowed it (see `plugin::guard_watch`). Neither can be reached +//! by a plugin enumerating panes, because nothing ever tells a plugin what panes +//! exist — which is what keeps "an arbitrary shell is never plugin-controlled" a +//! property of the code. A shell stays untouched by doing what a shell does: +//! saying nothing to any plugin. use super::hub_helpers::PaneState; use crate::backend::{PaneId, PtyBackend}; @@ -88,6 +91,11 @@ pub(super) struct Plugins { pub(super) owners: HashMap, /// Each plugin's `allowed_resume_flags`, as the guard needs them. pub(super) allowed_flags: HashMap>, + /// The plugins whose config set `watch_on_signal`: those the operator allowed + /// to be given a pane they were never named by. Held as the set of names + /// rather than looked up in the config list, so the judgement reads it as a + /// hash probe on the same footing as every other fact it gathers. + pub(super) watch_on_signal: HashSet, pub(super) guard: Guard, pub(super) pending: HashMap, /// Panes whose plugin has already been told about the current quiet period, @@ -98,13 +106,17 @@ pub(super) struct Plugins { } impl Plugins { - /// Launch a host for every plugin that is enabled *and* that some startup - /// pane opted into. + /// Launch a host for every plugin that is enabled *and* has some pane it + /// could be given. /// /// Both conditions, because a host with no pane to watch is a child process - /// that can never be given anything to do. A plugin that will not launch is - /// logged and left out: its panes then behave exactly like unwatched ones, - /// so a broken plugin costs the operator a warning rather than a terminal. + /// that can never be given anything to do. `watch_on_signal` is the second + /// way to satisfy the first: such a plugin's panes are the ones that will + /// speak to it, so it has to be running *before* any of them does — waiting + /// for an opt-in that will never come would make the switch mean nothing. + /// A plugin that will not launch is logged and left out: its panes then + /// behave exactly like unwatched ones, so a broken plugin costs the operator + /// a warning rather than a terminal. pub(super) fn start(cwd: &str, configs: &[PluginConfig], startup: &[StartupCommand]) -> Self { let dir = crate::plugin::registry::default_plugins_dir() .inspect_err(|error| { @@ -113,16 +125,20 @@ impl Plugins { .ok(); let mut hosts = HashMap::new(); let mut allowed_flags = HashMap::new(); + let mut watch_on_signal = HashSet::new(); for cfg in configs { let opted_in = startup .iter() .any(|sc| sc.plugin.as_deref() == Some(cfg.name.as_str())); - if !cfg.enabled || !opted_in { + if !cfg.enabled || !(opted_in || cfg.watch_on_signal) { continue; } match PluginHost::spawn(cfg, dir.as_deref()) { Ok(host) => { allowed_flags.insert(cfg.name.clone(), cfg.allowed_resume_flags.clone()); + if cfg.watch_on_signal { + watch_on_signal.insert(cfg.name.clone()); + } hosts.insert(cfg.name.clone(), host); } Err(error) => tracing::warn!( @@ -136,6 +152,7 @@ impl Plugins { hosts, owners: HashMap::new(), allowed_flags, + watch_on_signal, guard: Guard::new(PANE_IDLE_THRESHOLD, RateLimits::default()), pending: HashMap::new(), idle_announced: HashSet::new(), diff --git a/src/web/viewer/terminal/hub_recovery.rs b/src/web/viewer/terminal/hub_recovery.rs index 189f5c41..c43b3c1d 100644 --- a/src/web/viewer/terminal/hub_recovery.rs +++ b/src/web/viewer/terminal/hub_recovery.rs @@ -12,6 +12,20 @@ use super::hub_plugins::Plugins; use crate::backend::{PaneId, PtyBackend, TerminalBackend}; use std::time::Instant; +/// Whether `pane`'s process could be put back if it ended. +/// +/// A relaunch reproduces the pane's original invocation, so a pane the host +/// launched no command in has nothing to reproduce — which is exactly the pane a +/// plugin is given when its occupant asks to be watched. The guard refuses such a +/// relaunch outright, so the hold that exists solely to make one possible must not +/// be taken out for it either: that hold lasts days, and it would be days spent +/// keeping a shell's slot alive for a request that can never be granted. +pub(super) fn is_relaunchable(backend: &PtyBackend, pane: PaneId) -> bool { + backend + .slot(pane) + .is_some_and(|slot| slot.launch.command.is_some()) +} + /// The `state` the hub itself sends when a pane's recovery is over without /// having succeeded — cancelled by a person, or given up on when the hold ran /// out. Every client reads it as "stop showing a deadline for this pane". @@ -57,7 +71,8 @@ impl TerminalHub { /// For a pane no plugin watches this is the long-standing path: destroy it /// and tell everyone. For a watched one the slot has to survive, because its /// token is the only thing a relaunch can reuse — so the process alone is let - /// go and the slot is held until the plugin acts or the window closes. + /// go and the slot is held until the plugin acts or the window closes. Unless + /// there is nothing to put back: see [`is_relaunchable`]. pub(super) fn pane_exited( &self, backend: &mut PtyBackend, @@ -71,14 +86,14 @@ impl TerminalHub { } // Where it sat, read before the removal below takes it out of the order. match self.pane_spot(pane) { - Some(spot) => { + Some(spot) if is_relaunchable(backend, pane) => { backend.release_process(pane); plugins.hold_for_relaunch(pane, spot, Instant::now()); plugins.pane_exited(backend, pane); } - // Not in the client-visible order, so there is nowhere to put a - // replacement and no reason to keep the slot alive for one. - None => { + // Nowhere to put a replacement, or nothing to put there: either way + // there is no reason to keep the slot alive for one. + _ => { plugins.pane_closed(backend, pane); plugins.forget(backend, pane); backend.destroy_pane(pane); diff --git a/src/web/viewer/terminal/hub_relaunch.rs b/src/web/viewer/terminal/hub_relaunch.rs index a5b20a76..199edf9b 100644 --- a/src/web/viewer/terminal/hub_relaunch.rs +++ b/src/web/viewer/terminal/hub_relaunch.rs @@ -79,12 +79,52 @@ impl TerminalHub { attempt, ); } + Ok(Approved::WatchPane { pane }) => { + self.watch_pane_for_plugin(backend, plugins, &plugin, pane) + } Ok(Approved::Log { level, message }) => log_plugin_line(&plugin, level, &message), Err(refused) => log_refusal(&plugin, &refused), } } } + /// Hand `pane` to `plugin` and announce it, so the plugin can start from the + /// same `PaneOpened` a configured pane begins with. + /// + /// Announcing is the whole point of the request: the association alone tells + /// the plugin nothing, and only a `PaneOpened` carries the generation every + /// later command has to name. The pane's history is not replayed — output + /// events carry fresh bytes only — so a plugin taken on mid-session sees the + /// pane from here forward, which is exactly what the signal that brought it + /// here is about. + fn watch_pane_for_plugin( + &self, + backend: &PtyBackend, + plugins: &mut Plugins, + plugin: &str, + pane: PaneId, + ) { + // The pane's name as the clients know it, so a plugin sees the same title + // whichever way it was given the pane. A client-opened pane has none. + let title = self.pane_spot(pane).and_then(|spot| spot.title); + if !plugins.adopt(pane, plugin) { + // Only reachable if the plugin's host died between the command being + // queued and this tick, since a dead host produces no commands. + tracing::debug!( + plugin = %plugin, + pane, + "viewer: nothing to hand a pane to; the plugin's host is gone" + ); + return; + } + tracing::info!( + plugin = %plugin, + pane, + "viewer: a plugin was given a pane by the token something inside it quoted" + ); + plugins.pane_opened(backend, pane, title.as_deref()); + } + /// Put a process back into the slot a plugin was holding. /// /// Only for a pane the hub is actually holding: that hold is the proof the @@ -214,9 +254,10 @@ fn log_plugin_line(plugin: &str, level: LogLevel, message: &str) { /// Log a refusal at the level that says whether anyone should look into it. /// /// A plugin decides asynchronously, so being late is ordinary traffic rather -/// than a fault: the pane moved on, or is not quiet yet. The rest mean the -/// plugin asked for something it was never allowed — a pane that is not its, an -/// oversized or control-laden payload, a flag the config does not list, or more +/// than a fault: the pane moved on, is not quiet yet, or was claimed by another +/// plugin first. The rest mean the plugin asked for something it was never +/// allowed — a pane that is not its, an oversized or control-laden payload, a +/// flag the config does not list, a bare shell it wanted to relaunch, or more /// attempts than the budget allows — and that is worth an operator's attention. /// Matched exhaustively on purpose, so a new refusal has to be classified rather /// than defaulting to silence. @@ -226,11 +267,14 @@ fn log_refusal(plugin: &str, refused: &Refused) { | Refused::StaleGeneration { .. } | Refused::PaneNotRunning { .. } | Refused::PaneBusy { .. } - | Refused::PaneStillRunning { .. } => true, + | Refused::PaneStillRunning { .. } + | Refused::PaneWatchedByAnother { .. } => true, Refused::NotOptedIn { .. } | Refused::InputTooLarge { .. } | Refused::ControlCharacter { .. } + | Refused::NoLaunchCommand { .. } | Refused::ResumeArgsRejected { .. } + | Refused::WatchNotAllowed { .. } | Refused::RateLimited { .. } => false, }; if ordinary { diff --git a/src/web/viewer/terminal/tests/mod.rs b/src/web/viewer/terminal/tests/mod.rs index ebc32486..e28cd414 100644 --- a/src/web/viewer/terminal/tests/mod.rs +++ b/src/web/viewer/terminal/tests/mod.rs @@ -4,6 +4,7 @@ mod behavior; mod plugin_rules; mod plugin_slots; +mod plugin_watch; mod plugins; mod recovery; mod scrollback_depth; diff --git a/src/web/viewer/terminal/tests/plugin_watch.rs b/src/web/viewer/terminal/tests/plugin_watch.rs new file mode 100644 index 00000000..dce3c059 --- /dev/null +++ b/src/web/viewer/terminal/tests/plugin_watch.rs @@ -0,0 +1,194 @@ +//! Being given a pane nobody opted in, driven against a real `PtyBackend`. +//! +//! The half that cannot be tested here is the token's journey: it reaches a +//! plugin through a provider's own hook, inside a pane, over a socket this crate +//! does not own. What *is* pinned here is everything the host decides once a token +//! comes back — which pane it names, whether the operator allowed it, and what +//! being given the pane does and does not entitle a plugin to. + +use super::plugin_rules::{COLS, LONG_RUNNING, PLUGIN, ROWS, opt_in, token_of}; +use super::plugins::{fixture, recorder}; +use crate::backend::{PaneId, PaneToken, PtyBackend, TerminalBackend}; +use crate::config::PluginConfig; +use crate::plugin::protocol::{PROTOCOL_VERSION, PluginCommand}; +use crate::plugin::{Approved, Refused}; +use crate::web::viewer::terminal::hub_plugins::Plugins; +use crate::web::viewer::terminal::hub_recovery::is_relaunchable; +use std::path::Path; +use std::time::Instant; + +/// The same recording plugin, with the operator's switch on. +fn signal_watcher(name: &str, log: &Path) -> PluginConfig { + PluginConfig { + watch_on_signal: true, + ..recorder(name, log) + } +} + +fn watch(token: &PaneToken) -> PluginCommand { + PluginCommand::WatchPane { + v: PROTOCOL_VERSION, + token: token.clone(), + } +} + +/// A pane a client opened: no title, and no command of its own. +fn bare_pane(backend: &mut PtyBackend) -> PaneId { + backend + .open_pane(ROWS, COLS, None) + .expect("open a bare pane") +} + +#[test] +fn a_plugin_that_watches_on_signal_is_launched_with_no_pane_opted_in() { + // The panes it exists for cannot opt in, so it has to be running before any of + // them says anything. `adopt` succeeding is how "it has a live host" is read. + let f = fixture(); + let mut plugins = Plugins::start(&f.cwd(), &[signal_watcher(PLUGIN, &f.log)], &[]); + + assert!(plugins.adopt(1, PLUGIN)); +} + +#[test] +fn a_watch_request_bearing_a_live_panes_token_is_approved_and_the_pane_handed_over() { + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start(&f.cwd(), &[signal_watcher(PLUGIN, &f.log)], &[]); + let pane = backend + .open_pane(ROWS, COLS, Some(LONG_RUNNING)) + .expect("open a pane"); + let token = token_of(&backend, pane); + assert_eq!(plugins.owner(pane), None, "nothing handed it over yet"); + + let approved = plugins + .judge(PLUGIN, watch(&token), &backend, Instant::now()) + .expect("a live pane's own token is the evidence"); + + assert_eq!(approved, Approved::WatchPane { pane }); + // The association is the caller's to record, which is what the hub does next. + assert!(plugins.adopt(pane, PLUGIN)); + assert_eq!(plugins.owner(pane), Some(PLUGIN)); + backend.destroy_pane(pane); +} + +#[test] +fn a_watch_request_from_a_plugin_without_the_switch_is_refused() { + // The identical request, refused purely because the operator did not ask for + // it: this is the only thing standing between a signal and a new association. + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start(&f.cwd(), &[recorder(PLUGIN, &f.log)], &[opt_in()]); + let pane = bare_pane(&mut backend); + let token = token_of(&backend, pane); + + let refused = plugins + .judge(PLUGIN, watch(&token), &backend, Instant::now()) + .expect_err("a plugin without the switch may not be given a pane"); + + assert!( + matches!(refused, Refused::WatchNotAllowed { .. }), + "{refused}" + ); + assert_eq!(plugins.owner(pane), None); + backend.destroy_pane(pane); +} + +#[test] +fn a_watch_request_for_a_pane_another_plugin_holds_is_refused() { + const OTHER: &str = "other"; + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start( + &f.cwd(), + &[ + signal_watcher(PLUGIN, &f.log), + signal_watcher(OTHER, &f.log), + ], + &[], + ); + let pane = bare_pane(&mut backend); + let token = token_of(&backend, pane); + assert!(plugins.adopt(pane, OTHER)); + + let refused = plugins + .judge(PLUGIN, watch(&token), &backend, Instant::now()) + .expect_err("one pane, one watcher"); + + assert!( + matches!(refused, Refused::PaneWatchedByAnother { .. }), + "{refused}" + ); + assert_eq!(plugins.owner(pane), Some(OTHER), "the holder keeps it"); + backend.destroy_pane(pane); +} + +#[test] +fn a_watch_request_for_a_token_this_host_never_minted_is_refused() { + // A helper from another nightcrow session on the same machine: the socket is + // per-user, so its tokens do reach us, and they must resolve to nothing. + let f = fixture(); + let backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start(&f.cwd(), &[signal_watcher(PLUGIN, &f.log)], &[]); + let stranger = PaneToken::new().expect("OS RNG"); + + let refused = plugins + .judge(PLUGIN, watch(&stranger), &backend, Instant::now()) + .expect_err("a token naming no pane of ours"); + + assert!(matches!(refused, Refused::UnknownPane { .. }), "{refused}"); +} + +#[test] +fn a_pane_handed_over_on_a_signal_can_never_be_relaunched() { + // The invariant that keeps late adoption from doing something worse than + // nothing: the pane is a shell, so a relaunch would restart the shell rather + // than the session the plugin was recovering. + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let mut plugins = Plugins::start(&f.cwd(), &[signal_watcher(PLUGIN, &f.log)], &[]); + let pane = bare_pane(&mut backend); + let token = token_of(&backend, pane); + assert!(plugins.adopt(pane, PLUGIN)); + // The exit the plugin would answer with a relaunch. + backend.release_process(pane); + + let refused = plugins + .judge( + PLUGIN, + PluginCommand::Relaunch { + v: PROTOCOL_VERSION, + token, + generation: 1, + resume_args: Vec::new(), + }, + &backend, + Instant::now(), + ) + .expect_err("a pane with no command of its own has nothing to relaunch"); + + assert!( + matches!(refused, Refused::NoLaunchCommand { .. }), + "{refused}" + ); + backend.retire_slot(pane); +} + +#[test] +fn an_exited_pane_with_no_command_is_not_worth_holding_a_slot_for() { + // The condition the hub branches on when a watched pane's process ends. A + // hold lasts days and exists only to make a relaunch possible, so a pane that + // can never be relaunched must take the closing path instead — otherwise a + // shell that exited would keep its slot for the whole window. + let f = fixture(); + let mut backend = PtyBackend::new(f.cwd()); + let bare = bare_pane(&mut backend); + let launched = backend + .open_pane(ROWS, COLS, Some(LONG_RUNNING)) + .expect("open a pane"); + + assert!(!is_relaunchable(&backend, bare)); + assert!(is_relaunchable(&backend, launched)); + + backend.destroy_pane(bare); + backend.destroy_pane(launched); +}