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

[CrazyWillBear]

Closes #289, closes #290, closes #291 — the three review-fix follow-ups from the /my-review of PR #288, one commit each.

Context: PRD #286's migration is abandoned (Apple refused a second Apple ID), so the dedicated path ships unvalidated. That reframes these three — #291 is the one with live-user reach, firing on every interactive chief uninstall including single-user installs.

#291 — uninstall offered to delete an account it never created

_account_wanted was reached on every interactive uninstall. Answering yes ran sudo userdel --remove chief against whatever pre-existing account happened to carry that name, taking its home with it. Separately, execute(step)'s return code was discarded, so "system account chief removed." printed even when both steps failed.

Gated the whole branch on posture.chief_account(repo_dir / ACCOUNT_REPORT) — the question is only asked when this install's own report names an account that resolves to a real passwd entry. --remove-account on a single-user box is now a no-op. The account name comes from the report rather than a CLI default no caller ever passed, so the now-dead user parameter is gone.

#289 — grant_reason was weaker than the guarantee three docs make for it

It rejected only the literal home path, / and relative paths, so ~/.., /home and /Users all passed — chgrp -R chief /Users two keystrokes away. Now judged on the resolved path: refuse /, anything the home is inside, and any non-directory (which also catches the typo that previously aborted _run after the account and tree steps had landed).

Moved to dedicated_ask.py, its only caller — account.py goes 195 → 185, off the 200-line cap it was one edit from. Account name gated on ^[a-z_][a-z0-9_-]{0,31}$ before it reaches argv and Path("/Users") / user.

#290 — dual-store double-delivery of group chats

The two stores are polled independently and the twin dedup was off wholesale in dedicated mode, so a group both accounts are in held every message twice: two stranger rows, two monitor evaluations, two classifier calls, two wakes.

Now records every message in the window regardless — so whichever store reads it first primes the key — and suppresses only where a repeat is an artefact. Non-dedicated behavior is bit-identical.

Known trade: the key is (thread_key, sender, text) over 5s, so two genuinely identical group messages seconds apart collapse on the owner's-store path. That path is monitor-only; noted in a comment.

Not done

/etc is still grantable. #289 listed it, but the documented guarantee is about the home root, and /etc is an existing directory the owner typed deliberately. A system-path denylist is a different promise than the docs make — say so if you want it.

Validation

uv run pytest 790 passed, ruff check . and mypy . clean.

Six new tests, all failing before the fix. #290 and #291 are exercised against real fakes; #289's grant_reason is a pure function and fully unit-tested, but the chgrp it gates has still never run on real hardware — same caveat as the rest of the dedicated path.

Docs updated in the same change: docs/OPERATIONS.md (uninstall's account question) and docs/SECURITY.md (the grant claim now states what is actually enforced).

[CrazyWillBear]

Review findings addressed — 71d15da

/my-review (opus, xhigh) came back CHANGES REQUESTED: 1 high, 5 medium, 3 low. All nine are handled. Done-check green: 793 passed, ruff + mypy + shellcheck clean.

The high was a regression this branch introduced, and the reviewer was right that no test covered the combination:

Sev	Finding	Resolution
high	--purge-data rmtree's data/, then the new gate reads data/account-setup out of it — so --purge-data --remove-account --yes kept the account and reported none ever existed. main removed it.	Report read before the purge. Regression test.
medium	/etc and other system roots still grantable; chmod -R g+rwX /etc is root via sudoers, contradicting SECURITY.md's no-escalation bullet	grant_reason now refuses anything not strictly inside the owner's home — which is the question the wizard actually asks, and subsumes every earlier clause
medium	Dedup only held when chief's store polled first; independent cursors mean the owner's copy can land a tick ahead, and the window can't break the tie (measured on the row's own date, identical in both stores)	RecentDedup.see records which store logged a sighting and returns it; cross-store repeats suppressed either order, only mine→mine stays exempt. Regression test for the owner-first order.
medium	install.sh:96 unconditional rm -f of the report — any --single-user, non-interactive, or declined re-run stranded a real account, making --remove-account a permanent no-op	Written aside via mktemp, moved into place only on a create/adopt answer
medium	Test fixtures wrote the developer's real username into a record driving sudo userdel --remove; inert only because every call site passes a recording executor, while the default is a live subprocess.run	nobody
medium	Gate accepts mode=existing, so an adopted account is still removable — contradicting the new "created by this install" doc claim	Doc fixed, behavior kept (owner's call): removal stays explicit opt-in and interactively confirmed, and OPERATIONS.md now says created or adopted, spelling out that --remove-account on an existing install deletes a user predating chief
low	groupdel --force ran after a failed userdel, and exit 0 read as success	Stops at the first failure, returns 1. Regression test.
low	_name docstring named the wrong escape (.., and useradd on a macOS sysadminctl path)	Corrected — it guards an absolute name swallowing the Path("/Users") / user join
low	Dedup comment stated the opposite side of the trade from what #290 asked for	Now states the collapse cost, including the one-to-one cross-store case

Worth recording: the reviewer noted #290's implementation is stronger than the issue text specified — the issue's literal "run the dedup for non-mine rows only" would never have primed the window from chief's store and would have fixed nothing.

[CrazyWillBear]

Verification round — 54e9f54

Three agents (opus on the high, opus on the 5 mediums, sonnet on the 3 lows) adversarially verified 71d15da. Six of nine confirmed fixed; one was half-done, one was a regression I introduced, and they turned up four defects nobody had looked for. All addressed. 796 passed, ruff + mypy + shellcheck clean.

The one that mattered: M2 was fixed wrong

The verifier simulated my dedup against the real RecentDedup:

dedicated, group both accounts are in, someone sends "ok" twice 2s apart:
  chief r1  deliver
  owner r1  DROP    (correct — but rewrites the stored mine flag to False)
  chief r2  DROP    <- genuine second message, LOST. main delivered it.

I had traded a double-delivery for silent message loss, and my own comment claiming "only a mine→mine repeat stays exempt" was false in exactly the two-store configuration the fix exists for.

The key was wrong, not the bookkeeping. (thread_key, sender, text) cannot separate one message read from two stores from two people typing the same thing — and imessage_store.py already recorded the fact that makes them separable: self-DM twins carry different guids. So:

• twins (same text, different guid) → the text heuristic, now restored to main's semantics verbatim for the non-dedicated path
• both stores (same message, same guid) → exact, no heuristic, no false collapse

Read skew can't defeat it either, since the window is measured on the row's own date, which is identical in both copies. Single-store installs never match the new check. And if guids turn out not to match across two Apple IDs, it degrades to a double delivery — never a lost message. New test test_a_genuine_repeat_in_a_shared_group_is_not_swallowed covers what regressed.

Found by the verifiers, not in the original review

Sev	Defect	Fix
medium	mktemp with no template would likely fail on BSD/macOS — the only platform the dedicated path runs on. Needs an interactive tty, so no CI job reaches the line and shellcheck can't see it.	Takes a template
medium	chief account defaulted --report to None and the launcher forwards none, so the documented alternative to re-running install.sh recorded nothing — and after 1c75a13 that's an account chief can never remove	Defaults to data/account-setup
medium	uninstall --purge-data with the account kept (the default) destroys the only record of it, stranding it permanently — the same failure class this PR fixed for install.sh	Says so, with the manual command
low	Report landed 0600 owner-only after the mv; the tree chown had already run, so chief couldn't read its own install record	chmod 0644
low	--keep-account had no test on either side of this change	Added
low	SECURITY.md still described the old grant rule; the stale useradd claim survived in a test docstring after being fixed in the code	Both corrected

Confirmed genuinely fixed

M1 — verified against a built symlink tree: symlinked parents are caught (.resolve() walks the chain), both sides resolved so a symlinked home still admits its own subdirectories, and macOS firmlinks don't break legitimate grants. M4, M5 (clause-by-clause against the code), L1 (return traced through to the process exit code), L3. The high was verified empirically by running the new test's body against the pre-fix module loaded from 9305028.

Non-dedicated mode was verified byte-for-byte equivalent to 77338af — the single-user install has no regression from any of this.

Known and accepted

$HOME=/ would let /etc through (is_relative_to("/") is true for everything); bind mounts defeat resolve() but need root already. Neither is a regression, both need a machine already lost.