docs: address review nits from #29

[shipyard-ci]

Follow-up to #29 (merged). Addresses the three P3 nits from that review. No behaviour change — docs and comments only, plus one binding rename.

Changes

docs/docker.md — stale PID 1 claim
The section opened with "Spacebot runs as PID 1" and then, a few lines later, explained that tini is the entrypoint. Straightforwardly self-contradictory. Now leads with the actual arrangement — tini is PID 1, spacebot is its direct child — and says why the distinction matters, since whichever process holds PID 1 is the one that inherits orphans. That is the whole reason spacebot's own reaper is conditional, so it's worth a sentence rather than a correction in passing.

src/process/reaper.rs — incomplete SAFETY comment
The comment justified WNOHANG (non-blocking, doesn't stall the runtime) but said nothing about the pointer, which is the part that actually needs justifying. Now states the invariant: status is a valid, properly aligned c_int stack local that outlives the call, and waitpid writes at most one c_int through it.

src/tools/shell.rs — _owned → _reaper_claim
The old name described what the binding was; the new one describes what it is for. The underscore still marks it as intentionally unused — it exists to be held, and dropping it early would reintroduce the leak.

Verification

Unit tests 10/10, clippy clean under -D warnings, rustfmt clean.

Staged only the three files above; the unrelated modified files in the shared checkout belong to a concurrent worker (spacedriveapp#224).

[shipyard-ci]

1. Summary

Documentation-only / comment-only changes following review feedback from #29. The docs now state that tini is PID 1 and spacebot is its direct child (rather than spacebot being PID 1), the waitpid SAFETY comment in reaper.rs is expanded to justify the pointer write, and the reaper-claim binding in shell.rs is renamed _owned → _reaper_claim with a clarified comment. No behavioral code changes.

2. Findings

P3 — Doc/comment claim about the claim guard's Drop behaviour changed meaning
src/tools/shell.rs:

+    // Tokio. Held until this function returns; dropping the claim also reaps
+    // the PID, which collects the child in the paths below where `wait()` times

The previous wording said dropping the guard "also sweeps" (i.e. a general orphan sweep); the new wording says it "reaps the PID" (i.e. a targeted reap_one). reaper.rs in this diff exposes both reap_orphans() and reap_one(pid), so these are materially different statements. Worth confirming which one the claim guard's Drop actually performs so the comment doesn't drift back out of date.

P3 — Narrowed condition in docs/docker.md drops a previously documented case

+  that runs only when spacebot *itself* holds PID 1, which happens if the
+  entrypoint is overridden.

This replaces "bare docker run without --init, or any deployment that bypasses the entrypoint". If the published image's ENTRYPOINT is tini, the new text is the more accurate one; but the word "only" makes it an absolute claim — anything else that lands spacebot at PID 1 (e.g. a derived image redefining ENTRYPOINT, or a k8s command: override) is arguably still "overriding the entrypoint", so consider softening to "e.g. if the entrypoint is overridden".

P3 — Line exceeds the file's ~80-col wrap

+grandchildren behind (`sh -c "cargo build"` exits, its `cargo`/`node`/build-script

~82 chars, while surrounding prose in the same paragraph wraps at 80. Trivial re-wrap.

Nothing blocking. The expanded waitpid SAFETY comment in reaper.rs is a genuine improvement: it now names the validity, alignment, lifetime and write-size properties, which is exactly what a SAFETY block should cover. The _reaper_claim rename keeps the leading underscore, so the binding is still held to end of scope (unlike a bare _) — the "Held until this function returns" comment is correct.

3. Security

No security impact. No changes to command construction, argument passing, authentication, or secret handling; waitpid(pid, &mut status, WNOHANG) call site is byte-for-byte unchanged, only its comment.

4. Verdict

APPROVE — documentation and comment accuracy improvements with no behavioral risk. The P3 items are optional follow-ups.

---

AI Review · Verdict: APPROVE · Diff-Score: 0.83
Routed as code_review (100%) → github_code_review_flow · View AI traces