refactor: ttyd reaper, argv daemon launch, url dedup, keep-alive, retries (#101 P2)

[axisrow]

Summary

Six P2 refactoring-debt fixes from the multi-agent code review in #101, plus NOTE comments for the deferred TOCTOU finding (#8).

• Fix CSP blocking eval in TTYD and add debug logging #7 — dead ttyd processes not reaped without traffic (manager.py, app.py). Added a background daemon reaper thread; zombies were only reaped lazily inside list_terminals()/get_terminal(), so with no list/get/health traffic a defunct process pinned its PID slot forever. Start/stop wired into main() + the signal handler.
• CSRF token: investigate suspected additional bug #9 — run_as_hapi interpolated args into sh -c (entrypoint.sh). Routed droid/ao/hapi startup through the argv-safe run_as_hapi_argv (a latent root-context command-injection surface). | tee → redirect (argv can't express a pipe, matching the tunnel helpers), inline env via env NAME=VAL; the unused string run_as_hapi is removed.
• Document design decisions from PR review cycles #12 — relay-URL built by a drifted bash reimplementation (entrypoint.sh). Delegated the legacy /home/hapi/url writer to the same ttydproxy.views.build_hapi_url_from_runtime the dashboard uses (bash took the FIRST relay URL + left the token un-encoded; views takes the LAST + quote()s both).
• Sync ~/.claude config and sessions to container #13 — per-request allocation in the HTTP proxy (proxy.py). HOP_BY_HOP_HEADERS is now a frozenset, the HTML skip-set is precomputed, and resp.getheaders() is read once (was twice per response). A keep-alive connection pool was intentionally not added — a shared HTTPConnection across ThreadingHTTPServer workers would introduce a concurrency bug.
• Sync GitHub CLI auth (~/.config/gh) to container #14 — duplicated JS helpers across assets (app/assets/*.html). Exported window.__containsFiles / window.__sendToTTYD from the injected tab_fix_script and delegated to them from the parent page + virtual keyboard, removing byte-identical containsFiles copies and three hard-coded socket-discovery + '0' ttyd-prefix sites.
• Sync SSH keys to container #15 — network fetches with no retry (Dockerfile). Wrapped the Node/ttyd/cloudflared/chisel curl downloads and the ao git-fetch + Hermes git-clone/pip steps in the CLAUDE.md retry loop, previously applied only to npm.

#101 findings closed

• Fix CSP blocking eval in TTYD and add debug logging #7 (zombie ttyd not reaped) — resource-leak, PLAUSIBLE
• CSRF token: investigate suspected additional bug #9 (run_as_hapi sh -c injection) — security, PLAUSIBLE
• Document design decisions from PR review cycles #12 (relay-URL double parsing) — reuse, PLAUSIBLE
• Sync ~/.claude config and sessions to container #13 (no keep-alive / per-request alloc) — efficiency, PLAUSIBLE
• Sync GitHub CLI auth (~/.config/gh) to container #14 (JS helper duplication) — reuse, PLAUSIBLE
• Sync SSH keys to container #15 (curl/git without retry) — conventions, PLAUSIBLE

Deferred: #8 (port-reuse TOCTOU) — narrow window (usually ECONNREFUSED→502), not a security boundary. NOTE comments added in app.py + manager.py pointing at a per-terminal lease/refcount as the proper fix, tracked separately.

New/changed env vars

None.

Ports / volumes

No changes.

Tests

New: reaper tests in test_ttyd_manager.py, TestResponseHeaderEfficiency in test_proxy_http.py, TestDockerNetworkRetries + a droid-daemon injection test + relay-URL delegation tests in test_shell_scripts.py, __sendToTTYD delegation in the vkbd JS tests. Full pytest tests/ and npm test green; bash -n entrypoint.sh clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KgvJhzd8gEhXVE4MVu18nn

[axisrow]

🔍 Local review (cycle 1)

Reviewed locally (Claude subagent + Codex companion), no bots pinged. Both reviewers ran; findings triaged against the real code + git show main:entrypoint.sh. bash -n entrypoint.sh clean; 176 Python + 4 JS tests pass.

Verdict	Reviewer	Finding	Location
IRRELEVANT	codex	Root opens hapi-controlled log path (>>) before privilege drop	entrypoint.sh:497
SKIP	claude	| tee → >> changes truncate to append (stale fallback URL + log growth)	entrypoint.sh:493-497

Both findings stem from the \| tee "$LOG" → >>"$LOG" 2>&1 change in the argv refactor; neither is a required FIX:

• C1 (IRRELEVANT — pre-existing, no incremental exposure): True that cmd >>"$F" & opens $F in the root outer shell before runuser drops privileges. But every daemon that gained a root >> redirect already has a root touch "$LOG" + chown on the exact same path one line above — and those lines are unchanged from main (hapi server 493-494, droid 298-299, ao 337-338). Root already creates/opens and re-owns the target through any hapi-planted symlink there; the >> follows the identical symlink root already dereferenced. The root-context run_as_hapi_argv … >>"$LOG" pattern itself pre-exists in main (the SSH-tunnel launch). No new attacker-reachable target introduced by this PR. (The absence of a symlink guard on these log paths is a real but pre-existing gap, out of scope here.)

• C2 (SKIP — real mechanism, negligible severity + optional hardening): tee (no -a) truncated server.log at startup; >> appends, and nothing else truncates it, so on the persistent volume old relay URLs persist. cleanup_runner_state removes settings.json (token stays fresh) but not server.log. However the dashboard link is built live on every request via views.build_hapi_url_from_runtime (last-wins over the trailing 8 KiB), which self-heals to the new URL+token within seconds of hapi appending; only the static /home/hapi/url fallback file can go briefly stale, and only in a degraded state (live files missing). Unbounded server.log growth is minor (reads are tail-bounded). Optional one-char hardening: replace touch "${HAPI_SERVER_LOG}" with : > "${HAPI_SERVER_LOG}" to restore the old truncate-at-startup semantics and cap growth — I'll apply this in the cleanup pass.

Verified correct (both reviewers): reaper thread (no double-clean race — removal happens under lock; daemon thread catches exceptions; start_reaper idempotent; stop_reaper joins; correctly wired in main/signal_handler), argv env-passing (nested env AO_PORT=…/HAPI_RELAY_FORCE_TCP=… reaches the child; stdbuf/&/$! preserved), python relay delegation (PYTHONPATH=/app matches COPY app/ /app/; -f-gated; None-safe; captured + chown'd), proxy.py frozenset + single getheaders() (byte-equivalent), JS __containsFiles/__sendToTTYD dedup (identical behavior + pre-load fallback), and the 7 Dockerfile retry loops (break-on-success, hermes clone rm -rf before retry, syntactically valid in case arms).

Applying the optional C2 truncate hardening in the cleanup pass; merge is left to the maintainer.

[axisrow]

Applied the C2 cleanup in 1ad695c: the three daemon logs (server/droid/ao) are now : > "$LOG" (truncate-or-create) instead of touch, restoring the old | tee truncate-at-startup semantics now that the launches append via >>. This closes the narrow stale-fallback-URL window on server.log and caps unbounded log growth on the persistent volume. Full pytest tests/ green. Local review cycle for #104 is complete.

[axisrow]

Rebased onto merged main (3552a8b, with #102 P0 + #103 P1 squash-merged). Now MERGEABLE.

Conflicts (2, both resolved by union — kept the P1 change from main AND layered the P2 change on top, nothing reverted):

• app/ttydproxy/proxy.py — HOP_BY_HOP_HEADERS: fix: proxy reliability + sync-guard dedup + .env.example sync (#101 P1) #103 added "cookie" (as a plain set); refactor: ttyd reaper, argv daemon launch, url dedup, keep-alive, retries (#101 P2) #104 made it a frozenset and added _SKIP_RESPONSE_HEADERS_HTML. Resolved to frozenset({… , "cookie"}) + _SKIP_RESPONSE_HEADERS_HTML. Verified at runtime: cookie and authorization in the frozenset; _SKIP_RESPONSE_HEADERS_HTML inherits HOP + content-security-policy.
• tests/unit/test_proxy_http.py — both PRs appended a new test class before if __name__: fix: proxy reliability + sync-guard dedup + .env.example sync (#101 P1) #103 TestBuildTtydHeaders (cookie stripping), refactor: ttyd reaper, argv daemon launch, url dedup, keep-alive, retries (#101 P2) #104 TestResponseHeaderEfficiency (single getheaders/frozenset). Kept both.

Auto-merged cleanly (verified, not silently mismerged):

• app/ttydproxy/app.py — fix(security): drop PASSWORD_SECRET from proxy env + add slowloris read-timeout (#101 P0) #102 timeout attr + fix: proxy reliability + sync-guard dedup + .env.example sync (#101 P1) #103 _check_auth redirect (removed-user branch → 302) + refactor: ttyd reaper, argv daemon launch, url dedup, keep-alive, retries (#101 P2) #104 reaper wiring & Terminal loads slowly after creation (502 on first connect) #8 NOTE all coexist.
• entrypoint.sh — fix(security): drop PASSWORD_SECRET from proxy env + add slowloris read-timeout (#101 P0) #102 unset PASSWORD_SECRET + refactor: ttyd reaper, argv daemon launch, url dedup, keep-alive, retries (#101 P2) #104 run_as_hapi_argv (0 string run_as_hapi left) + Document design decisions from PR review cycles #12 python relay-URL delegation + : > log truncation.
• tests/unit/test_shell_scripts.py — test additions from all three PRs present (ENV_EXAMPLE const intact).

Verification (all green): pytest tests/ → 441 passed; npm test → 15 passed (assets touched); bash -n entrypoint.sh + bash -n bin/clihost-sync.sh clean.

Not merging — leaving that to the maintainer once CI shows green.