fix(install): the three #288 review follow-ups — #289, #290, #291

docs/OPERATIONS.md

@@ -132,7 +132,16 @@ and `secrets/`; without `--purge-data` it explicitly says both were kept. It the
 asks about the **system account** separately — `--remove-account` /
 `--keep-account` are the non-interactive answers, and keeping is the default,
 because chief's home holds its own message store and the dedicated Apple ID's
-whole conversation lives there.
+whole conversation lives there. The question is only asked when
+`data/account-setup` records the account this install set chief up with —
+created *or* adopted (`mode=existing`) — and that account still resolves to a
+real passwd entry: a single-user install is never offered the deletion, and
+`--remove-account` there is a no-op rather than a `userdel` aimed at whatever
+pre-existing account happens to be named `chief`. An adopted account is
+removable the same way a created one is, so on an `existing` install
+`--remove-account` deletes a user that predates chief, home and all. The report
+survives re-runs that skip or decline the account offer; only a fresh
+create/adopt answer replaces it.
 
 ## The service
 

docs/SECURITY.md

@@ -312,8 +312,13 @@ What the boundary buys:
   just closed (`install/account.py`).
 - **The owner picks what chief may reach.** The wizard asks two questions —
   directories to read, directories to write — defaulting to **none**, applied
-  as group permissions. The home directory root is never offered
-  (`grant_reason`).
+  as group permissions. Only directories strictly *inside* the owner's home
+  can be granted — judged on the resolved path, so neither `~/..` nor a
+  symlink out smuggles anything in, and the home root itself, `/`, and every
+  system root are refused along with it (`chmod -R g+rwX /etc` would hand
+  chief group-write on `sudoers`, which is the escalation the bullet above
+  rules out). A directory that does not exist is refused too, rather than left
+  to fail its own step mid-plan (`install/dedicated_ask.py`, `grant_reason`).
 
 ### What the boundary does not cover
 

install.sh

@@ -93,20 +93,34 @@ fi
 CHIEF_USER=""
 CHIEF_HOME=""
 ACCOUNT_REPORT="$REPO_DIR/data/account-setup"
-rm -f "$ACCOUNT_REPORT"
 if [ "$SINGLE_USER" = 1 ]; then
   say "skipping the dedicated-account offer (--single-user)"
 elif [ "$NON_INTERACTIVE" = 1 ] || [ ! -r /dev/tty ]; then
   say "dedicated account: not offered (no terminal) — chief runs as you"
 else
   say "dedicated system account"
+  # Written aside and moved over the real report only on an account answer.
+  # Re-runs are idempotent and the offer can be skipped (--single-user, no
+  # tty) or declined, and none of those mean the account an earlier run set up
+  # has gone away — but `chief uninstall` reads this file to decide whether
+  # there is an account to remove at all, so erasing it strands one.
+  # With a template, not bare: BSD mktemp (macOS — the platform this targets)
+  # requires one, and this line is only reached on an interactive run, which
+  # no CI job makes.
+  FRESH_REPORT=$(mktemp "${TMPDIR:-/tmp}/chief-account.XXXXXX")
   uv run python -m chief.install account \
-    --tree "$REPO_DIR" --report "$ACCOUNT_REPORT" < /dev/tty
+    --tree "$REPO_DIR" --report "$FRESH_REPORT" < /dev/tty
   # -E, not BRE alternation: BSD grep (macOS — the platform this targets) does
   # not understand \(a\|b\), and a silent no-match installs the wrong mode.
-  if grep -qE '^mode=(create|existing)$' "$ACCOUNT_REPORT" 2>/dev/null; then
+  if grep -qE '^mode=(create|existing)$' "$FRESH_REPORT" 2>/dev/null; then
+    mv "$FRESH_REPORT" "$ACCOUNT_REPORT"
+    # mktemp makes it 0600 owner-only, and the tree chown already ran, so
+    # without this chief cannot read its own install record.
+    chmod 0644 "$ACCOUNT_REPORT"
     CHIEF_USER=$(sed -n 's/^user=//p' "$ACCOUNT_REPORT")
     CHIEF_HOME=$(sed -n 's/^home=//p' "$ACCOUNT_REPORT")
+  else
+    rm -f "$FRESH_REPORT"
   fi
 fi
 

src/chief/adapters/imessage.py

@@ -70,6 +70,7 @@ def __init__(
         # or it deadlocks that thread until the card times out.
         self._resolve_approval = resolve_approval
         self._dedup = RecentDedup()
+        self._guids = RecentDedup()
         self._task: asyncio.Task[None] | None = None
         self._fifo = ThreadFifo(on_message, restart)
         self._stores = [Store(db_path, RowCursor(cursor_path))]
@@ -132,7 +133,9 @@ async def poll_once(self) -> None:
         scope = frozenset() if self._dedicated else self._owner_handles
         for store in self._stores:
             rows = await asyncio.to_thread(store.fetch, scope)
-            for rowid, sender, text, from_me, group_chat, in_self, date in rows:
+            for rowid, sender, text, from_me, group_chat, in_self, date, guid in (
+                rows
+            ):
                 store.advance(rowid)
                 if not store.mine and sender in self._self_handles:
                     # Chief's own reply, seen from the owner's side as an
@@ -141,6 +144,13 @@ async def poll_once(self) -> None:
                 message = self._map(sender, text, from_me, group_chat, in_self)
                 if message is None:
                     continue
+                # A group chat both accounts are in holds every message twice,
+                # once per store, under different rowids. Same message, so
+                # same guid — which the text key cannot tell apart from two
+                # people typing "ok". Cheap in the single-store case: a guid
+                # is only ever read once, so nothing matches.
+                if guid and self._guids.is_duplicate((guid,), date):
+                    continue
                 if not self._dedicated and self._dedup.is_duplicate(
                     (message.thread_key, message.sender, message.text), date
                 ):

src/chief/adapters/imessage_store.py

@@ -35,7 +35,8 @@
     "MAX(CASE WHEN (chat.style IS NOT NULL AND chat.style != 45) "
     "OR chat.room_name IS NOT NULL THEN chat.chat_identifier END) AS group_chat, "
     "MAX(CASE WHEN self_chat.mid IS NOT NULL THEN 1 ELSE 0 END) AS in_self, "
-    "message.attributedBody AS body, message.date AS date "
+    "message.attributedBody AS body, message.date AS date, "
+    "message.guid AS guid "
     "FROM message JOIN handle ON message.handle_id = handle.ROWID "
     "LEFT JOIN chat_message_join ON chat_message_join.message_id = message.ROWID "
     "LEFT JOIN chat ON chat.ROWID = chat_message_join.chat_id "
@@ -70,6 +71,13 @@ class RecentDedup:
     the same text (owner types it again minutes on) from being swallowed. State
     is in-memory: a fresh boot re-primes from the store cursor, never replaying
     an already-delivered twin.
+
+    Also used, keyed on ``guid`` alone, for the other duplicate: one message
+    present in both stores. That one is exact rather than heuristic — the two
+    copies *are* the same message, so they carry the same guid, where self-DM
+    twins carry different ones. Read skew between the stores cannot defeat it
+    either: the window is measured on the row's own date, which is identical
+    in both.
     """
 
     def __init__(self, window_ns: int = DEDUP_WINDOW_NS) -> None:
@@ -121,7 +129,7 @@ def text_of(text: object, body: object) -> str:
 
 
 #: One polled row: (rowid, sender, text, from_me, group_chat, in_self, date).
-PolledRow = tuple[int, str, str, int, str | None, int, int]
+PolledRow = tuple[int, str, str, int, str | None, int, int, str]
 
 
 def fetch_rows(
@@ -143,6 +151,7 @@ def fetch_rows(
             (
                 int(r[0]), str(r[1]), text_of(r[2], r[6]), int(r[3]),
                 None if r[4] is None else str(r[4]), int(r[5]), int(r[7]),
+                str(r[8] or ""),
             )
             for r in cur.fetchall()
         ]

src/chief/install/account.py

@@ -30,7 +30,6 @@
     "Step",
     "account_plan",
     "default_home",
-    "grant_reason",
     "grant_steps",
 ]
 
@@ -96,15 +95,6 @@ def _git_steps(tree: Path, user: str, email: str) -> tuple[Step, ...]:
     )
 
 
-def grant_reason(path: Path, home: Path) -> str | None:
-    """Why this directory may not be granted, or ``None`` if it may."""
-    if not path.is_absolute():
-        return f"{path} is not an absolute path"
-    if path in (home, Path("/")):
-        return f"{path} is a home or filesystem root — grant a subdirectory"
-    return None
-
-
 def grant_steps(
     *,
     group: str = DEFAULT_GROUP,

src/chief/install/cli.py

@@ -8,6 +8,7 @@
 from pathlib import Path
 
 from chief.install.lifecycle import DEFAULT_LAUNCHER
+from chief.install.posture import ACCOUNT_REPORT
 
 
 def build_parser() -> argparse.ArgumentParser:
@@ -24,7 +25,14 @@ def build_parser() -> argparse.ArgumentParser:
     account.add_argument("--non-interactive", action="store_true")
     account.add_argument("--tree", type=Path, default=None)
     account.add_argument(
-        "--report", type=Path, default=None, help="write key=value facts here"
+        # Defaulted, not None: `chief account` is the documented alternative to
+        # re-running install.sh (docs/OPERATIONS.md), and uninstall reads this
+        # file to decide there is an account at all. Written nowhere, the
+        # account it just made is one chief can never remove.
+        "--report",
+        type=Path,
+        default=ACCOUNT_REPORT,
+        help="write key=value facts here",
     )
     service_install = sub.add_parser(
         "service-install", help="install + start the autostart service"

src/chief/install/dedicated_ask.py

@@ -4,18 +4,44 @@
 inspected and pinned without a terminal or a machine to change.
 """
 
+import re
 from dataclasses import dataclass, field
 from pathlib import Path
 
-from chief.install.account import DEFAULT_USER, grant_reason
+from chief.install.account import DEFAULT_USER
 from chief.install.session import password_conflict
 from chief.install.wizard_io import MIN_PASSWORD_LENGTH, WizardIO
 
 CREATE = "create"
 EXISTING = "existing"
 DECLINED = "declined"
+ACCOUNT_NAME = re.compile(r"^[a-z_][a-z0-9_-]{0,31}$")
 
-__all__ = ["CREATE", "DECLINED", "EXISTING", "Answers", "ask"]
+__all__ = ["CREATE", "DECLINED", "EXISTING", "Answers", "ask", "grant_reason"]
+
+
+def grant_reason(path: Path, home: Path) -> str | None:
+    """Why this directory may not be granted, or ``None`` if it may.
+
+    The question asked is which of *your* directories chief may reach, and
+    strictly-inside-your-home is that question's own answer — which is also
+    what makes it the whole check. It refuses the home root and ``/``, ``~/..``
+    and any other route out (judged on the *resolved* path, since the grant is
+    a recursive, irreversible ``chgrp``), and every system root: ``chmod -R
+    g+rwX /etc`` hands chief group-write on ``sudoers`` and is root by another
+    name, which the no-escalation promise in docs/SECURITY.md rules out.
+
+    A path that does not exist is refused here rather than left to fail its own
+    step, which aborts the plan after the account and tree steps have landed.
+    """
+    if not path.is_absolute():
+        return f"{path} is not an absolute path"
+    target, root = path.resolve(), home.resolve()
+    if target == root or not target.is_relative_to(root):
+        return f"{path} is not inside {root} — grant a directory of your own"
+    if not target.is_dir():
+        return f"{path} is not an existing directory"
+    return None
 
 
 @dataclass(frozen=True)
@@ -53,6 +79,19 @@ def _password(io: WizardIO, user: str) -> str:
         return password
 
 
+def _name(io: WizardIO) -> str:
+    """The name reaches argv unquoted and is joined onto the home root, where
+    an *absolute* one swallows the join whole: `Path("/Users") / "/etc"` is
+    `/etc`, i.e. `sysadminctl -addUser … -home /etc`."""
+    while True:
+        user = (
+            io.prompt(f"  account name [{DEFAULT_USER}]: ").strip() or DEFAULT_USER
+        )
+        if ACCOUNT_NAME.match(user):
+            return user
+        io.say("  lowercase letters, digits, _ and - only — try again.")
+
+
 def _dirs(io: WizardIO, verb: str, home: Path) -> tuple[Path, ...]:
     raw = io.prompt(
         f"  directories chief may {verb}, space-separated (empty = none): "
@@ -81,9 +120,7 @@ def ask(io: WizardIO, *, home: Path) -> Answers:
     if answer in ("n", "no"):
         io.say("account: declined — chief runs as you, exactly as before.")
         return Answers(DECLINED)
-    user = (
-        io.prompt(f"  account name [{DEFAULT_USER}]: ").strip() or DEFAULT_USER
-    )
+    user = _name(io)
     choice = EXISTING if answer in ("e", "existing") else CREATE
     password = _password(io, user) if choice == CREATE else ""
     io.say("Which of your directories may chief reach? Default is none.")

src/chief/install/lifecycle.py

@@ -12,9 +12,10 @@
 import httpx
 
 from chief.config import load_config
-from chief.install.account import DEFAULT_GROUP, DEFAULT_USER
+from chief.install.account import DEFAULT_GROUP
 from chief.install.account_steps import remove_steps
 from chief.install.dedicated import StepRunner, default_step_runner
+from chief.install.posture import ACCOUNT_REPORT, chief_account
 from chief.install.service import ServiceManager
 
 DEFAULT_LAUNCHER = Path.home() / ".local" / "bin" / "chief"
@@ -52,7 +53,6 @@ def uninstall(
     assume_yes: bool,
     remove_account: bool = False,
     keep_account: bool = False,
-    user: str = DEFAULT_USER,
     group: str = DEFAULT_GROUP,
     confirm: Callable[[str], str] = input,
     say: Callable[[str], None] = print,
@@ -62,7 +62,8 @@ def uninstall(
 
     The dedicated system account is only removed when asked for — its home
     holds chief's own message store. ``--remove-account`` / ``--keep-account``
-    are the non-interactive answers; without either, uninstall asks.
+    are the non-interactive answers; without either, uninstall asks. A
+    single-user install has no such account, and is never asked.
     """
     if purge_data and not assume_yes:
         answer = confirm(
@@ -71,6 +72,10 @@ def uninstall(
         if answer.strip().lower() not in ("y", "yes"):
             say("aborted — nothing removed.")
             return 1
+    # Read before the purge: --purge-data deletes data/, which is where the
+    # report lives, and a run told to remove the account would then find no
+    # record of one and report that it never existed.
+    account = chief_account(repo_dir / ACCOUNT_REPORT)
     service.uninstall()
     launcher.unlink(missing_ok=True)
     say("service + launcher removed.")
@@ -80,15 +85,35 @@ def uninstall(
         say("data + secrets removed.")
     else:
         say("data + secrets kept (pass --purge-data to remove them).")
-    if not keep_account and _account_wanted(
-        remove_account, assume_yes, user, confirm
+    # Only this install's own report names the account it set chief up with; a
+    # bare `chief` in passwd may be someone else's, and userdel --remove takes
+    # the home with it. No report, no question and no steps.
+    if account is None:
+        say("no dedicated system account is recorded for this install.")
+    elif keep_account or not _account_wanted(
+        remove_account, assume_yes, account[0], confirm
     ):
-        for step in remove_steps(service.platform, user, group):
-            say(f"  {step.description}")
-            execute(step)
-        say(f"system account {user} removed.")
+        say(
+            "system account kept "
+            f"(pass --remove-account to delete {account[0]})."
+        )
+        if purge_data:
+            # The purge just took data/account-setup with it, and that is the
+            # only record of the account. Nothing here can remove it after
+            # this, so name the manual commands while they are still useful.
+            say(
+                f"  its record is gone with data/ — remove {account[0]} by "
+                f"hand if you meant to: userdel --remove {account[0]}"
+            )
     else:
-        say(f"system account kept (pass --remove-account to delete {user}).")
+        for step in remove_steps(service.platform, account[0], group):
+            say(f"  {step.description}")
+            if execute(step).returncode != 0:
+                # Stop: groupdel --force after a failed userdel takes the group
+                # out from under an account that is still there.
+                say(f"system account {account[0]} could not be removed.")
+                return 1
+        say(f"system account {account[0]} removed.")
     return 0