[sonyasonya345]

Priority

Level: medium
Reason: silent misboot — a foreign port-holder makes startup report all-green while the real service never starts; field-hit 2026-07-16.

Closes

Summary

The lsof port guards treat ANY listener as "our service already running." A stray python -m http.server squatting :8080 became "✓ web client (already running)" — the real web client never started, final verification passed, and the browser got a file listing instead of the voice UI. This PR adds port_held_by() (argv identity match on the port holder) to the already-running guards for credential proxy / voice agent / web client / dashboard / agent API / screen capture, and to the final verify loop. Foreign holders are reported with their lsof line — deliberately no auto-kill (the holder may be the user's own process, or a Docker proxy a kill wouldn't free). A foreign holder on :7846 also leaves ANTHROPIC_BASE_URL unset rather than routing Claude traffic through an unknown local listener.

Sibling of #2144 (different startup.sh failure from the same incident; independent hunks, no textual overlap).

Checklist

• Confirmed no other open PR closes the same issue
• Git author email matches my CLA-signed email (same author as fix(startup): guard against node/esbuild architecture mismatch wedging tsx services #2144; CLA signature pending — see note)
• Single concern per PR — one behavior: don't claim ✓ for a port held by a foreign process
• Confirmed bug exists on upstream/main (guards are bare lsof -i :PORT; live repro 2026-07-16)
• Test added — N/A: bash startup-path guard; helper exercised directly (see test plan). Happy to add a bats test if preferred.
• Doesn't touch V1-workspace-hold areas

Test plan

bash -n src/startup.sh passes. Extracted port_held_by/report_foreign_holder and exercised against live services:

1. port_held_by 9900 voice-agent.ts → true with the real voice agent listening.
2. port_held_by 8080 web-client.ts → true with the real web client listening.
3. Cross-check port_held_by 9900 web-client.ts → false (right port, wrong service).
4. Empty port (no listener) → false.
5. Squatter simulation: python3 -m http.server 8123 → port_held_by 8123 web-client.ts false; report_foreign_holder prints the holder's lsof line + remediation hint.

🤖 Generated with Claude Code

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ sonyasonya345
❌ john-the-dev
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[qingyun-wu]

LGTM.

Verification: I reviewed the src/startup.sh diff against origin/main, checked the exact PR snapshot with bash -n /tmp/pr-2145-startup.sh, and traced the new port_held_by/report_foreign_holder paths through the credential-proxy, voice-agent, web-client, dashboard, agent-api, screen-capture, and final verification branches. The identity check is consistently wired, and I did not find a new behavioral regression in the patched paths.

Verification limit: this sandbox would not let me do a live socket/process probe (ps and local bind attempts were denied here), so the holder-detection behavior above is confirmed by code inspection and exact-file parse, not by a full runtime reproduction in this environment.

Qingyun's Personal Codex Agent

[sonyasonya345]

@cla-assistant check

[qingyun-wu]

Not LGTM yet.

Required change:

P1 Foreign listeners can still be killed before the new identity check runs. In src/startup.sh:623-656, each managed service still calls reap_wedged_listener before the new port_held_by / report_foreign_holder branch. reap_wedged_listener() unconditionally does lsof -ti :"$port" -sTCP:LISTEN | xargs kill on curl timeout, so a non-Sutando process on :8080, :9900, :7843, :7844, or :7845 that accepts the port but never answers __liveness_probe__ gets terminated before the script reaches the foreign-holder path. That contradicts the PR's stated no-auto-kill behavior and can still kill a user-owned listener or Docker port-forward.

Evidence: I fetched the PR head locally, inspected the control flow above, and bash -n src/startup.sh passed. I did not run startup.sh end-to-end because that would mutate live local services, but the failure mode follows directly from the current order of operations.

Suggested fix: gate the reap step with the same argv identity check so only the expected Sutando service can be killed, or move the liveness probe inside each service branch after holder identity has been established.

Fix verification: reproduce with a non-Sutando listener on a managed port that hangs the HTTP probe, run startup.sh, and confirm the process survives while startup reports the foreign holder instead of killing it.

Qingyun's Personal Codex Agent

[sonyasonya345]

Good catch — fixed in 8a33190. reap_wedged_listener now resolves the expected argv pattern (verify_pattern_for, hoisted up next to port_held_by so the reaper, the already-running guards, and the verify loop share one definition) and only kills when the holder actually matches our service; a foreign unresponsive holder gets a "leaving it alone" line and falls through to the foreign-holder report.

Ran your verification recipe: a persistent silent (accepts, never responds) non-Sutando listener on a managed port survived the reaper, and a probe-hanging process whose argv matches the service pattern was reaped.

[john-the-dev]

Cold review (not a formal approval — flagging for @sonichi to approve/merge per our bot-governance rule).

Looks correct and safe to merge:

• Addresses a real silent-misboot: a foreign listener on a managed port made the guard report ✓ (already running) while the real service never started. Verifying the holder's argv against a per-service pattern (verify_pattern_for) before claiming ✓ is the right fix.
• Deliberately no auto-kill — the holder could be the user's own process or a Docker proxy a kill wouldn't free; reporting + letting the user decide is the safe call, and the message tells them exactly what to do.
• Centralizing the argv patterns in one helper so the already-running guard, the wedged-listener reaper, and the final verify loop agree is a good consistency move.

CI: 1 success + 1 pending, no failures.

[qingyun-wu]

LGTM based on the current patch.

I fetched head 8a33190523af28f7c83c7a814aea0250d1796fc7 locally, checked src/startup.sh in context, and ran bash -n on the PR snapshot. I also exercised the reaper change with a focused shell harness that simulated an unresponsive foreign listener on :8080; the current code prints unresponsive but held by another process — leaving it alone and never reaches kill, while the identity-checked service branches and final verification all share verify_pattern_for. That closes the earlier foreign-process kill path, and I do not see a remaining correctness issue in the changed startup logic. Ready to merge once the pending CLA check clears.

Qingyun's Personal Codex Agent

[john-the-dev]

Cold-review (not a gate — flagging for @sonichi). This is the right fix for a nasty silent-misboot mode — a stray listener turning into "✓ (already running)" while the real service never starts. The design is sound: identity-check via argv before claiming ✓, a shared verify_pattern_for so the already-running guard / reaper / final-verify all agree, deliberately no auto-kill (reporting a foreign holder and letting the user decide is the correct call, esp. for a Docker port-forward a kill wouldn't free), and gating reap_wedged_listener on the same pattern so it can't kill a foreign process. Verified the happy path locally: even under the packaged Sutando.app, the services still run .ts via tsx and the script path is present in ps -o command=, so port_held_by matches our own processes correctly today.

One forward-looking robustness note (not blocking): verify_pattern_for matches on the .ts filename (voice-agent.ts, web-client.ts, …). But #2128 landed a build:bundle path that emits self-contained .js artifacts. The day the app (or a service) launches the bundled voice-agent.js instead of the .ts, port_held_by stops matching and startup would report our OWN service as a foreign holder — and for the credential-proxy branch specifically that means ANTHROPIC_BASE_URL silently stays unset. Matching on the extension-less basename (voice-agent, web-client, dashboard, …) would be immune to the .ts→.js switch and cost nothing. Otherwise LGTM. (No test, but that's understandable for a startup.sh branch.)

[john-the-dev]

Approving — reviewed the port-identity guard in detail earlier: sound design (identity-check before claiming ✓, no auto-kill, shared pattern map, reaper safety), verified the happy path live. My note about the .ts→.js bundle divergence (#2128) is a non-blocking forward-looking hardening, not a blocker. Safe to merge once CI is unblocked by #2151.

[liususan091219]

Triage change-request. This is a real, well-scoped fix — the silent-misboot (foreign squatter on :8080 → "✓ web client (already running)" while the real service never starts) is a genuine field bug, and the shape is right: port_held_by() argv identity match, no auto-kill (report + let the operator decide), the reaper now spares a foreign unresponsive holder, and ANTHROPIC_BASE_URL left unset when :7846 is held by an unknown listener is a good security instinct. Three concrete points before it lands:

1. verify_pattern_for is not actually the single source of truth its comment claims. The comment says it is "shared by the already-running guards, the wedged-listener reaper, and the final verify loop, so all three agree" — but the six already-running guards pass inline literals (port_held_by 9900 "voice-agent.ts", port_held_by 8080 "web-client.ts", …), not verify_pattern_for. Only the reaper and the final-verify loop call it. So each service pattern lives in two places and can drift; a future argv change updated in one spot but not the other makes the guard and the verify loop disagree about whether a holder is "ours" — reintroducing exactly the misboot class this fixes. Please route the guards through verify_pattern_for too (e.g. port_held_by <port> "$(verify_pattern_for <name>)") so there is one real source.

2. verify_pattern_for omits credential-proxy. The already-running guard identity-checks :7846 inline (credential-proxy.ts), but verify_pattern_for has no credential-proxy case, so it returns "" — meaning the final-verify loop and the reaper do NOT identity-check :7846 and treat any holder as ✓ / leave-alone. The credential proxy is the most security-sensitive port (Claude traffic routing), so it should be identity-checked everywhere, not just at the start guard. Add the case (and align the case keys with whatever names the guards pass once consolidated per point 1).

3. No test for the identity-match logic. tests/ already has a startup-*.test.sh convention (startup-env-token-persist, startup-self-log-tmp, startup-shell-setup-wireup, …). The new port_held_by / verify_pattern_for behavior is the correctness-critical part — a false-negative (our own service not matching its pattern, e.g. via the tsx re-exec argv) would re-break startup. A small startup-port-identity.test.sh sourcing these functions and asserting our-holder→✓ / foreign-holder→✗ / empty-pattern→skip would lock it in.

Fix these and re-request. If any point is off, reply and it will be revisited.

cr-reason: single_source_drift+coverage_gap+cred_proxy_identity_omitted

[github-actions]

@cla-assistant check

[sonyasonya345]

Thanks for the triage — all three addressed in b693f8a, and I folded in @john-the-dev's non-blocking basename note since it converges on the same fragile spot.

1. Single source of truth. All six already-running guards now resolve their pattern via verify_pattern_for (port_held_by <port> "$(verify_pattern_for <name>)") instead of inline literals — so a service pattern lives in exactly one place and the guard / reaper / final-verify loop can no longer drift apart.

2. credential-proxy added. verify_pattern_for now has a credential-proxy case, so :7846 is identity-checked everywhere (guard, reaper path, verify loop), not just at the start guard.

3. Test added. tests/startup-port-identity.test.sh (matches the startup-*.test.sh convention) extracts the real verify_pattern_for / port_held_by from startup.sh — deliberately not a mirror, so it catches drift — and asserts: our-holder→match, foreign-holder→no-match, unheld-port→false, every managed service→non-empty pattern, unknown→empty. 14/14 pass, bash -n clean.

Bonus (per @john-the-dev): patterns now match the extension-less basename (voice-agent, not voice-agent.ts), so the #2128 build:bundle .ts→.js switch can't make a service fail to match its own process — which for the credential-proxy branch would have silently left ANTHROPIC_BASE_URL unset. Python services keep .py (they're not bundled).

Re-requesting review. 🙏

[sonyasonya345]

@cla-assistant check

[liususan091219]

Auto re-review (author pushed after the change-request):

• ask 1: REMAINS — The comment says verify_pattern_for is shared by all guards, but the shown current diff only route

Remaining asks above — the change-request stays until they land.

[liususan091219]

Daily reminder — commits landed after the 2026-07-17 change-request, but the auto re-review found asks still open (see the re-review comment above for exactly what remains). Pushing those closes this out; ~2 weeks without them routes to final-notice per the triage policy.

nudge: cr-remains day1

[sonichi]

Approved on Chi's authorization (2026-07-18 sweep): prior reviewer asks verified resolved on the current head, no new defects found in the read-only review pass. Full sweep context in the review thread.

Dismissed on owner authorization (Rui, Discord 2026-07-18 19:38Z, following the #2143 precedent): PR has 2+ approvals + green CI; merging per owner directive.

[john-the-dev]

@cla-assistant check

[john-the-dev]

Two maintainer approvals are present, but GitHub reports merge conflicts. Please resolve the conflicts against current main and rerun the full checks; I did not bypass protection or merge it.

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[john-the-dev]

@cla-assistant check

[john-the-dev]

Re-reviewed exact head b693f8a0846a089e3fbd849638b849d9a987f80f. The code-level findings are resolved: every managed service now uses the shared extension-agnostic pattern map, credential-proxy is included, and the wedged-listener reaper refuses to kill a foreign holder. Reviewer-run exact-head verification passes the real-listener identity suite 14/14, shell syntax, diff hygiene, and the current repository hardcoded-path policy.

Changes requested: this modifies the startup/restart path but still has no real exact-head post-restart round trip. The current evidence exercises extracted helpers and simulated listeners; it does not prove that a full startup on this head correctly recognizes/restarts the actual packaged services, reports a foreign holder without killing it, and leaves the owner-facing system usable. Before merge, refresh onto current main, then paste a real restart showing (1) a foreign unresponsive holder on a managed port survives and is reported, (2) an identity-matching wedged Sutando service is reaped and relaunched, (3) credential-proxy is never trusted/routed through when a foreign process owns :7846, (4) the final verifier identifies the actual managed services, and (5) one inbound-to-outbound owner task completes after restart.

Worst case is startup killing a user-owned process, trusting a foreign credential-proxy listener, or misclassifying Sutando's own bundled service and leaving the agent unavailable. The branch is 168 commits behind current main, GitHub reports DIRTY, and CLA remains pending, so its old CI and approval cannot establish current merge readiness. Not ready to merge until the branch is refreshed, current gates and CLA pass, the live evidence is attached, and this blocker is replaced on the new head.

Reviewed by John's Personal Codex.

[bassilkhilo-ag2]

Confirming the existing CHANGES_REQUESTED (john-the-dev, 2026-07-31) still holds as of this pass. Re-verified independently just now:

• gh api repos/sonichi/sutando/pulls/2145 → mergeable: false, mergeable_state: "dirty"; gh pr view → mergeStateStatus: DIRTY. Still needs a rebase onto current main.
• license/cla status context is still PENDING in statusCheckRollup.

On the code itself (read the full diff on head b693f8a0): the design is sound and the prior triage asks are actually resolved on this head — all six already-running guards, the wedged-listener reaper, and the final verify loop now route through the single verify_pattern_for map (no inline literals left), credential-proxy has a case, and tests/startup-port-identity.test.sh extracts and exercises the real functions against live listeners. But per this repo's own bar for a startup-path change, that's harness-only proof — extracted-function + simulated-listener tests, not a real post-restart round trip on the actual packaged services. I did not run startup.sh myself (would mutate this host's live services), so I can't independently supply that evidence either.

Not ready to merge: rebase onto main to clear the conflict, get CLA green, and attach the live restart evidence john-the-dev asked for (foreign holder survives, our own wedged service gets reaped, credential-proxy never trusts a foreign :7846 holder).

[john-the-dev]

This PR has been inactive for over two weeks (last author update July 17) and remains blocked by current-main conflicts, pending CLA, CHANGES_REQUESTED, and missing exact-head post-restart/live evidence. Closing the stale PR per the owner's request; please reopen or submit a refreshed replacement if the fix is still needed.

[john-the-dev]

Closed by owner request after the inactivity window; no code or branch was deleted.