[Kb2uka]

Summary

Companion PR: #90. Both target main independently.

Splits the contact saving half of #79 for independent review. Adds only contact-save to the confined key. Review an unknown sender, enter a name, confirm the fields, and save a new Mac contact. The helper checks exact and regional duplicates and reads the saved card back before reporting success.

Includes the formatted-number and native Contacts predicate fallback fixes reported by @ianswope. Contact saving requires Contacts write access and does not require Accessibility or Swift. Setup and recovery after a lost response are documented in docs/SAVE-CONTACT.md.

Review fixes (2026-09-18)

Rebased onto current main (35 commits). The three findings from @nixfred's review:

1. contact-save.ts bypassed shimPath(). Fixed: it now resolves through
shimPath('contact-save', home) like every other caller, and contact-save
joins the ShimTool union.

The guard test that exists for exactly this — "no spawner hard-codes ~/bin" —
did not catch it. Its regex matched only double-quoted "bin", "imsg" and the
tools imsg/contacts; the regression was written 'bin','contact-save'.
Widened to every quote style and all five shim tools, it then found the same
regression already on main, in two files this PR does not otherwise touch:

• contact-details.ts:24 (exact-card details, from 386d9d1)
• contact-vcard.ts:91 (vCard export, from ab3e2ee)

Both are fixed here, because leaving them would leave the widened guard red.
Happy to split them into their own PR if you would rather review them apart
from the contact save.

2. scripts/blip-setup reverted main's install loop. Resolved in the
rebase itself rather than patched afterwards: main's mkdir -p "$bin_dir" /
target="$bin_dir/$t" loop, the bin_dir / prev_bin_dir / BLIP_BIN_DIR
handling, the header comment and the generated bridge.conf banner are all
main's; contact-save is added to the tool list. Same for README.md and
docs/PRIVACY.md, which keep main's bin_dir= wording. ci.yml takes main's
py_compile line with bridge/mac/contact-save appended — as you noted, if
#103 lands first this is the line to take from main again.

3. phone_matcher() reached into imsg's private names. imsg now names
parse_phone and same_number as the supported entry point for this, and
contact-save consumes those. A sync-bridge.sh pull that renames either
private function now breaks imsg's own import loudly, instead of silently
turning every regional duplicate check into "unavailable".

I kept one implementation rather than copying the helpers: a copy would let the
regional rules drift from the bridge's own, which is the failure that made this
check necessary in the first place. Say the word if you would rather have the
copy and take the duplication.

Named, as asked, rather than left implicit

• contact-save.js:85 writes through the deprecated ABAddressBook while
  reading through CNContactStore. It fails closed if sharedAddressBook
  returns null, so a macOS that removes the framework degrades to
  "unavailable" rather than corrupting anything. That is the right failure
  direction, and it is still a bet on an API Apple has been retiring for a
  decade. If it goes, the create path needs a CNSaveRequest replacement.
• contact-save.js:53: if adapter.read() throws after
  saveAndReturnError already committed, the caller sees
  {ok:false, code:'unknown'} while the card exists. The code comment says
  "everything after this point may already have committed" and
  docs/SAVE-CONTACT.md covers recovery. Worst case is a stray card the user
  deletes by hand; there is no automatic retry, by design.

Fail-then-pass

Finding 1, bun test bin-dir.test.ts contact-save.test.ts:

===== BEFORE =====            ===== AFTER =====
    "f": "contact-save.ts",    32 pass, 0 fail
-   "hit": false,
+   "hit": true,
(fail) no spawner hard-codes ~/bin
(fail) the save spawns the shim where bin_dir= put it
 30 pass, 2 fail

Finding 3, python3 bridge/mac/test_contact_save.py:

===== BEFORE =====                          ===== AFTER =====
FAIL: test_phone_matcher_uses_the_          Ran 13 tests — OK
      bridge_public_entry_point
AssertionError: AttributeError not raised
Ran 13 tests — FAILED (failures=1)

Gates after the rebase

Gate	Result
bun test	609 pass, 0 fail, 7637 expect() calls, 19 files
bridge/mac/test_contact_save.py	13 OK (11 + 2 new)
bridge/mac/test_phone_match.py	27 OK
bridge/mac/contacts_test.py	16 OK
bridge/mac/test_read_verification.py	9 OK
bridge/mac/test_blip_check.py	4 OK
CI py_compile line	OK
bash -n, shellcheck -S warning	clean

Still no live Contacts mutation claimed: there is no Mac in this loop and the
create path is unchanged by these fixes.

Proof

Fresh captures of the real QML component on a remote Linux host, with synthetic data and a stub helper. These verify rendering and are not evidence of native Mac mutation.

Form	Review	Saved

The native helper carries forward the implementation from #79, with a correction that distinguishes unavailable Contacts storage from denied permission in both inspection and creation. The regression failed before and passed after the correction. Regressions failed before and passed after preventing a busy save from being closed, refreshing candidates on Back, clearing stale errors on Edit, and distinguishing a missing helper or offline bridge from an uncertain save. The form was rendered again after the review fixes. Synthetic regressions cover formatted identities, denied access, exact and regional duplicates, failed native predicates, failed fallback, and saved-card verification. Earlier live card creation and cleanup evidence is recorded in #79; this split does not claim a new live Contacts mutation.

Gates

Below describes the pre-rebase revision (70da7fc). For the current head see
"Gates after the rebase" above: the TypeScript count moved 551 -> 609 with main's
35 commits and the new regression.

551 TypeScript tests and 96 Python tests passed on a remote Linux host. On macOS, 31 Python tests and JavaScript compilation passed. Python compilation, Bash syntax, ShellCheck, and diff whitespace checks passed. Both remote hosts tested the same verified Git bundle. Hosted CI requires maintainer approval.

Review rounds

Round	Seat A	Seat B	Accepted	Rejected	Needs human
1	3/5 (13)	4/5 (2)	7	8	0
2	3/5 (6)	4/5 (2)	5	2	1
3	4/5 (4)	5/5 (0)	0	4	0
4	3/5 (6)	5/5 (0)	1	5	0
5	4/5 (2)	5/5 (0)	0	2	0

Stopped at the five round limit. Accepted defects were fixed and covered by regressions. The final two comments were checked against the actual permission guidance and the passing native fixture tests; no open implementation findings remain after arbitration. The first attempt from Seat A in round 2 returned no verdict and was rerun successfully.

Needs a human

Maintainer review of the contact creation and confirmation flow, plus approval for the fork workflow. Closing the containing window can lose an in-flight save result; check Contacts before trying again, as documented in docs/SAVE-CONTACT.md. The form itself prevents closing or going back while a save is busy. No merge or deployment is included.

factory: rounds=5 evidence=A,B,D

[nixfred]

Same note as on #90: we publish a weekly update crediting contributors and tag by X handle where we can verify one, and I could not find one published for you. Reply with the handle if you want to be tagged, or say you would rather not and we will credit you as Kb2uka.

[nixfred]

Reviewed this properly, and the short version is that I want it. It is the safest contact-write surface of anything open on this repo.

Context first, since it changes where this PR sits: I closed #90 today, as a product decision rather than a quality one, and explained there why. That makes this the surviving half of the split I asked you for, so it is worth finishing rather than parking.

What I checked and what holds up:

• Create-only, with no edit, merge or delete path. That is the right scope for a first contact-write tool.
• The two-phase design under one flock, with inspect checking duplicates through CNContactStore predicates and an ABAddressBook scan as fallback, then a create phase that reads the card back by unique id before reporting success. Verifying from the store rather than from the return code is the same discipline that makes imsg-read trustworthy.
• valid_request() requiring the selected handle to be one the user actually typed.
• 551 bun tests pass here, plus test_contact_save.py (11) and contacts_test.py (16). The coverage includes denied access, exact and regional duplicates, a failed native predicate, a failed fallback, and saved-card verification. You also do not claim a live Contacts mutation was exercised, which is the honest thing to say from a Linux CI.

Three things before it can land, in order of how much they matter:

1. contact-save.ts:46 is a bin_dir= regression. It calls join(process.env.HOME ?? homedir(), 'bin', 'contact-save'). Every other TS caller on main now resolves through shimPath(): collector.ts, thread.ts, avatar.ts, fetch.ts, search.ts, send-file.ts, contact-review.ts, contact-search.ts. Anyone with bin_dir= set gets a broken save. Convert it to shimPath("contact-save", home).

2. scripts/blip-setup reverts main's install loop. The branch puts back $HOME/bin in place of main's mkdir -p "$bin_dir" / target="$bin_dir/$t" loop, and drops the bin_dir / prev_bin_dir / BLIP_BIN_DIR handling from the header comment and the generated bridge.conf banner. The correct resolution is to keep main's loop and simply add contact-save to the tool list.

3. bridge/mac/contact-save:99-107 phone_matcher() reaches into the vendored imsg's private _parse_phone and _same_number. A sync-bridge.sh pull that renames either one turns every regional duplicate check into "unavailable" silently. Either copy the two helpers, or make them a supported entry point in imsg rather than private ones.

ci.yml, README.md and docs/PRIVACY.md also conflict, but those are text-only. Note ci.yml collides with #103's rework of the same py_compile line, so if #103 lands first, take main's line and append bridge/mac/contact-save.

Two things I am not asking you to change, but want named in the PR rather than left implicit:

• contact-save.js:85 writes through the deprecated ABAddressBook while reading through CNContactStore. It fails closed if sharedAddressBook returns null, so a future macOS that removes the framework degrades to "unavailable" instead of corrupting anything. That is the right failure direction, but it is a bet on an API Apple has been retiring for a decade. Say so in the PR body.
• contact-save.js:53: if adapter.read() throws after saveAndReturnError already committed, the caller sees {ok:false, code:'unknown'} while the card exists. Your own comment says "everything after this point may already have committed" and docs/SAVE-CONTACT.md covers recovery, so this is handled honestly. Worst case is a stray card the user deletes by hand.

The real cost of this feature is the one your docs/SECURITY.md already states plainly: Contacts write TCC is granted to the SSH bridge binary, not per-tool, so it widens what every present and future bridge tool can do. I am fine with that trade for a create-only path that verifies from the store, and I would not be fine with it for a delete path.

Take it out of draft when the rebase is done and I will merge it.

[Kb2uka]

@StudioXRadio — tag away.

Rebasing onto current main now and working the three findings: shimPath() for contact-save.ts, restoring main's blip-setup install loop with contact-save added to the tool list, and cutting the dependency on imsg's private phone helpers. I'll name the ABAddressBook trade-off and the post-commit read case in the PR body, and take main's ci.yml line plus contact-save if #103 lands first. Un-drafting when it's green.

On #90 — appreciated, and no hard feelings. I'd rather have the decision right than early. Happy to come back with Undo Send on the terms you set now that #36 is closed.

[Kb2uka]

Rebased and out of draft. All three findings are addressed, CI is green, and it is mergeable clean.

One thing to see before you merge, since it is scope you did not ask for. Fixing finding 1 meant widening the existing no spawner hard-codes ~/bin guard — it had missed the regression because it only matched double-quoted "bin", "imsg" and the tools imsg|contacts, and mine was written 'bin','contact-save'. Widened, it immediately failed on two files already on main: contact-details.ts:24 and contact-vcard.ts:91, both join(HOME,'bin','contacts'). With bin_dir= set, exact-card details and vCard export were broken the same way the save was. I fixed both here rather than narrow the guard back down, but say the word and I will pull them into their own PR.

On finding 3 I kept one implementation rather than copying the helpers: imsg now names parse_phone/same_number as the entry point, so a re-vendor rename breaks its own import loudly instead of going quiet. A copy would let the regional rules drift from the bridge's, which is the failure that made the check necessary. Happy to take the duplication instead if you prefer it.

[nixfred]

Merged as 347ef54. Thank you @Kb2uka.

This one crossed a line we had written down as an invariant, that contact review is read-only and nothing is ever written to Contacts, so I read the write path before putting it to Fred rather than reviewing the description. What decided it:

• Create only. The helper builds a fresh person, addRecord, saveAndReturnError. There is no delete and no modify-existing path in the file at all.
• It refuses duplicates, then verifies. It reads the saved card back and compares every field before reporting success, and it says plainly that a failure after the commit point may still have created the contact. That honesty is worth more than a cleaner-looking success path.
• The card has to be the person you are looking at. The conversation handle must equal the phone or email being saved, both are format- and length-bounded, control and bidi characters rejected, and confirmed: true required.
• Fields cross on bounded stdin, never argv.
• It fails closed. Without the Contacts grant the feature reports unavailable and nothing changes.

Fred's call, made on that basis. bun test 617 pass, 11 Mac bridge suites OK including your test_contact_save.py. Not exercised live: creating a real contact, which needs the prompt answered at the Mac.

Your three conflicts were your own doing in the best way: the bin_dir regression you found in contact-details.ts and contact-vcard.ts shipped on main first as 4134bbf so it would not wait on this review. I kept the union of the two guard regexes, yours for backticks and mine for the word boundary, and kept shimPath('contacts') whose default home is exactly what your explicit argument passes.

Separately: you have no X handle I can verify and we credit contributors by handle in a weekly public update. Reply with one if you want to be tagged, or say you would rather not and we will credit you as Kb2uka.

[Kb2uka]

Thanks for merging it, and for reading the write path itself rather than the description — that is the review I would want on anything that crosses an invariant, and the five points you landed on are exactly the ones I would have wanted checked.

Good call shipping the contact-details.ts / contact-vcard.ts fixes to main as 4134bbf so they did not wait on this review, and taking the union of the two guard regexes rather than either one alone.

On the credit: yes, I would like to be tagged. The handle is @StudioXRadio (https://x.com/StudioXRadio). I posted it earlier in this thread on the 18th, so it may just have been buried above the merge.

Happy to come back with Undo Send on the terms you set now that #36 is closed.