Skip to content

feat(server): add authenticated instance shutdown - #182

Closed
Lint111 wants to merge 1 commit into
Ark0N:masterfrom
Lint111:agent/split-secure-instance-shutdown
Closed

feat(server): add authenticated instance shutdown#182
Lint111 wants to merge 1 commit into
Ark0N:masterfrom
Lint111:agent/split-secure-instance-shutdown

Conversation

@Lint111

Copy link
Copy Markdown
Contributor

Summary

  • Add a confirmed “Shut down Codeman” action for desktop and tablet headers.
  • Require CODEMAN_PASSWORD authentication in single-user mode and an administrator in multi-user mode.
  • Stop the owning systemd/launchd supervisor when one is verified; otherwise use the existing graceful server stop path.
  • Preserve tmux sessions so they can be recovered after restart.
  • Stop reconnect loops after an accepted shutdown and keep terminal keyboard focus out of the confirmation dialog.

Security model

The route fails closed in three places:

  1. Passwordless single-user network access receives 403.
  2. Non-admin multi-user accounts receive 403.
  3. Supervisor shutdown is accepted only when the detected Codeman unit/job reports the current process as its owner. Nested previews cannot stop a parent service.

System-level supervisor actions also fail closed when administrator access is unavailable.

Scope

This is the secure instance-shutdown slice extracted from #173. It includes the orchestration port, supervisor detection, route, confirmation UI, header policy, documentation, and focused tests. It does not depend on the mobile terminal controls.

The destructive header action stays hidden on phone widths, including wide-phone touch layouts through 480px.

Validation

  • npx vitest run --config config/vitest.config.ts test/instance-shutdown.test.ts test/routes/system-routes.test.ts test/mobile-header-buttons-policy.test.ts: 82 passed
  • npx vitest run --config test/mobile/vitest.config.ts test/mobile/header-buttons.test.ts: 4 passed
  • npm run check:frontend-syntax: 26 files passed
  • npm run build: passed
  • Commit formatting hook: passed
  • git diff --check: passed

@Lint111
Lint111 marked this pull request as ready for review July 29, 2026 17:30
CopilotAI review requested due to automatic review settings July 29, 2026 17:30

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Ark0N

Ark0N commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Thank you for this, and I'm sorry to give it a "no" after the care that clearly went in. I want to be straight with you about why rather than let it sit open indefinitely.

The engineering here is genuinely good. Gating on the unit's actual MainPID so a preview launched from a service-owned shell cannot stop its parent is exactly the check I would have asked for, the cgroup and launchctl print parsing are split into pure, testable functions, and the three fail-closed paths are the right three. If I were taking this feature, this is close to the implementation I would want.

My objection is to the feature, not to the code.

1. It is a one-way door, and the way back is not in the product.

The UI can stop the server, but nothing in the UI can start it again, because the thing that would serve that UI is what just stopped. The recommended production posture (and mine) is a loopback bind reached through tailscale serve or a tunnel. So the success state of this button is "you can no longer reach this machine's Codeman," and recovery requires SSH or physical access. On a headless box, which is a setup I actively recommend and run, that is a lockout with no in-product remedy. A control whose happy path removes your only remote access to the tool needs a very strong justification, and I don't think shutdown clears that bar.

2. The mechanism exists to defeat a guarantee I consider core.

Restart=always and launchd KeepAlive are not incidental packaging, they are the durability promise. Codeman's whole pitch is agents that survive: 24+ hour autonomous runs, respawn cycling, sessions that outlive crashes and restarts. This feature's central mechanism is reaching out from a web request to stop the supervisor that provides that. I would rather the answer to "how do I stop Codeman" stay "the same way you stop any other service on that machine" than build a supported path for the app to shut off its own keeper.

3. It would be dead on the deployment it most needs to be safe on.

The route requires CODEMAN_PASSWORD in single-user mode. I checked my own production unit: no CODEMAN_PASSWORD is set, because the documented and recommended setup is a loopback bind fronted by Tailscale, where the tunnel is the authentication. So on the primary supported posture this ships a header button that returns 403 forever. The users who would get a working remote kill switch are the ones who set a password, which is the LAN-exposed configuration, and that is the population I would least like to hand one to.

4. It adds a privileged surface without adding a capability.

Anyone who can authenticate to Codeman already has shell on that box, by construction. They installed it, and they run agents with --dangerously-skip-permissions there. For them, systemctl --user stop codeman-web already exists and is shorter than opening a browser. So the feature grants its authorized users nothing they lacked, while making systemctl stop and launchctl bootout reachable from an HTTP route and raising what an auth bypass is worth. Codeman has already shipped one auth-bypass class of bug (DNS rebinding plus a text/plain CSRF, fixed in 0.9.5). Given that history, I want the ceiling on "what can a request that slips the gate actually do" to stay low, and this raises it meaningfully for a convenience.

5. It is a second mechanism in an area that already has one.

Self-update solves the closely related "act on the process I am currently running in" problem, and it is deliberately awkward about it: a detached script that outlives the restart, plus a status file the browser polls across the connection drop. This introduces a different in-process approach to supervisor manipulation alongside it. Two subtly different mechanisms in the most hazardous corner of the codebase is a maintenance cost I would rather not take on.

None of that is a knock on your work, and please don't read it as one. Points 1 through 4 would apply to any implementation of this feature, which is why I would rather decline the idea cleanly than ask you to keep revising toward something I'm not going to merge.

For what it's worth, the parts I would happily look at as standalone contributions: the MainPID ownership check is a nice primitive on its own if there is ever another reason to know which unit owns this process, and the header button policy test addition is the right way to use that guard.

Closing this one. Thanks again for the time you put into it, and for splitting #173 up in the first place, that made it much easier to evaluate on its own terms.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Lint111@Ark0N