Skip to content

fix(kimaki): stop a failed system prompt patch from blocking service start - #570

Merged
chubes4 merged 1 commit into
mainfrom
fix/kimaki-global-tree-ownership
Sep 3, 2026
Merged

fix(kimaki): stop a failed system prompt patch from blocking service start#570
chubes4 merged 1 commit into
mainfrom
fix/kimaki-global-tree-ownership

Conversation

@chubes4

@chubes4 chubes4 commented Sep 3, 2026

Copy link
Copy Markdown
Member

The bug is one missing ||

post-upgrade.sh sets set -euo pipefail (line 50) and is wired as ExecStartPre with no - prefix, so any non-zero exit inside it stops the service from starting at all.

Pass 2 invoked node unguarded:

SYSTEM_MESSAGE_FILE="$system_message_file" node <<'NODE'
...
NODE
local patch_exit=$?          # <- unreachable under set -e
if [[ "$patch_exit" -eq 0 ]]; then
  ...
else
  echo "...WARNING: managed Kimaki system prompt patch failed..."   # <- never printed
fi

The failure handling was written correctly and could never executeset -e terminated the script at the node line, before $? was ever read.

Why that matters in practice

The reachable shape is routine, not exotic:

npm install -g kimaki     runs as root -> dist/system-message.js is root:root
rendered unit             User=$SERVICE_USER (non-root)
next service start        ExecStartPre patches a root-owned file as that user
                          -> EACCES -> exit 1 -> systemd refuses to start

One install sat down for five days across ~29,900 restart attempts (2026-08-28 → 2026-09-02), recovering only when a kimaki version bump happened to let root re-patch the file out of band. Four user requests were silently dropped in that window.

The journal is the proof that the branch never ran — node's stack trace goes straight to systemd's failure line, with neither the warning nor the closing summary in between:

kimaki-config: persistent skill source available at /opt/kimaki-config/skills
node:fs:2430
Error: EACCES: permission denied, open '/usr/local/lib/node_modules/kimaki/dist/system-message.js'
...
Node.js v22.22.1
kimaki.service: Control process exited, code=exited, status=1/FAILURE

Why this fix and not a chown

This file had already settled the rule for exactly this hazard, twice. try_remove_package_path (Pass 1):

This script runs as ExecStartPre, as the SERVICE user, with set -e and no - prefix on the unit directive — so any non-zero exit here blocks the service from starting at all. The package dir is root-owned […] An unguarded rm -rf therefore turns a supported install shape into a service that will not boot […] Not being able to perform them is worth a warning, never a failed start.

And the obsolete-plugin removal in Pass 3 repeats it: "Same reasoning as try_remove_package_path."

Pass 2 was the only one of the three not following the rule. So this makes it follow the rule rather than introducing a new mechanism. Chowning the npm tree at install time was the other candidate, and it is strictly more machinery guarding a narrower case — it fixes the installs we provision, and does nothing for a service that dies because some other root process ran npm install -g. The invariant that actually holds is the one this file already states: mutating the package directory is best-effort.

Changes

  • || patch_exit=$? on the node invocation, which activates the existing branch.
  • An unwritable target is reported separately from a genuinely broken patch, and names the remedy. "Root installed it, a service user runs it" is the supported shape, not a defect, and the operator needs to know to re-run as root rather than to go hunting.
  • Two regression tests in the existing kimaki-system-message-patch.sh (already registered in shell.yml).

Verification

Both new cases fail against the unfixed script and pass against the fixed one — baselined, not assumed:

# unfixed
PASS: Kimaki system prompt patch supports known signatures
FAIL: post-upgrade.sh exited 2 when the prompt patch failed; ExecStartPre must not block service start

# fixed, as non-root
PASS: Kimaki system prompt patch supports known signatures
PASS: a failed Kimaki system prompt patch warns without blocking service start
PASS: an unwritable Kimaki system-message.js warns without blocking service start
  • Case 1 (signature not found) is permission-free so it is deterministic for every caller including root.
  • Case 2 is the exact outage shape and is skipped when the caller is root, since root bypasses file permissions. CI runs as runner, so it executes there.
  • bash -n clean on both files; the repo's syntax job covers this.

Scope

Deliberately not addressed here: the affected install's nightly workflow re-armed this every night by running npm install -g kimaki as root without re-patching. That is fixed separately in that repo (chubes4/h44-lacrosse#3). This PR is the reason a mistake like that could take a host down instead of merely leaving a prompt unpatched.

…start

post-upgrade.sh runs with set -e and is wired as ExecStartPre with no '-'
prefix, so any non-zero exit inside it prevents kimaki.service from
starting at all. Pass 2 invoked node unguarded, which meant the failure
branch immediately below it was unreachable: the shell exited before $?
could be read. The error handling was written but could never run.

The reachable shape of that is routine. 'npm install -g kimaki' runs as
root and rewrites dist/system-message.js root-owned, while the rendered
unit runs as a non-root SERVICE_USER. The next start of the service then
tries to patch a root-owned file as that user, gets EACCES, and systemd
refuses to start. One install sat down for five days across roughly
29,900 restart attempts before a version bump happened to let root
re-patch the file. The journals for those attempts show node's stack
trace followed directly by systemd's failure line, with neither the
warning nor the closing summary printed - the branch never ran.

Passes 1 and 3 already settled the rule for this file: mutating the
root-owned package directory is best-effort hygiene, and being unable to
do it is worth a warning, never a failed start. try_remove_package_path
says so explicitly, and the obsolete-plugin removal repeats it. Pass 2
was the only one not following it, so this makes it follow it rather than
adding a new mechanism.

An unwritable target is now reported separately from a genuinely broken
patch, and names the remedy, since 'root installed it, a service user
runs it' is the expected shape rather than a defect.

Tests pin both: a patch that fails must leave the script exiting 0 and
reaching its summary, and the exact unwritable-target case. Both fail
against the unfixed script (exit 2 and exit 1); the unwritable case is
skipped when the caller is root, which bypasses file permissions.
@chubes4
chubes4 merged commit e73b3bf into main Sep 3, 2026
67 checks passed
Sign up for free to 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.

1 participant