Skip to content

fix: fail open on undetermined contacts auth#136

Open
cemendes wants to merge 1 commit into
openclaw:mainfrom
cemendes:fix/contacts-auth-fail-open
Open

fix: fail open on undetermined contacts auth#136
cemendes wants to merge 1 commit into
openclaw:mainfrom
cemendes:fix/contacts-auth-fail-open

Conversation

@cemendes
Copy link
Copy Markdown

@cemendes cemendes commented Jun 1, 2026

Summary

Avoid hanging imsg chats / imsg history when Contacts authorization is still .notDetermined in CLI usage.

Root cause

ContactResolver.create() currently awaits CNContactStore.requestAccess(for: .contacts) in the .notDetermined case.

In some CLI/headless contexts on macOS, that callback does not resolve promptly, which means commands that eagerly create a ContactResolver can appear to hang indefinitely even though the underlying Messages DB work already completed.

Fix

Fail open when Contacts auth is .notDetermined:

case .notDetermined:
  return NoOpContactResolver(contactsUnavailable: true)

This keeps core read paths working and only skips optional contact-name enrichment.

Why this is safe

  • chats and history still return useful output without contact names
  • existing behavior for .authorized, .denied, and .restricted is preserved
  • avoids blocking CLI read commands on a permission prompt path that may never complete

Manual verification

Before this change, on macOS 26.4.1 with Contacts auth still undecided:

  • imsg chats --limit 1 --json hangs
  • imsg history --chat-id 1090 --limit 1 --json hangs
  • imsg group --chat-id 1090 --json returns immediately
  • direct CNContactStore.requestAccess probe times out after 5s without callback

After this change:

  • imsg chats and imsg history should return normally
  • contact-name enrichment remains unavailable until Contacts access is explicitly granted through some other path

Verification in this branch

  • swift build
  • make test ⚠️ fails in this environment because the repo's tests import Testing but the module is unavailable here (no such module 'Testing')
  • make lint ⚠️ fails in this environment because swiftlint is not installed

Follow-up idea

Longer term, contact resolution should probably be opt-in (for example --resolve-contacts) rather than an implicit part of core read commands.

Closes #135

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

Codex review: needs real behavior proof before merge. Reviewed June 1, 2026, 5:54 PM ET / 21:54 UTC.

Summary
The branch changes ContactResolver.create() so .notDetermined Contacts authorization returns NoOpContactResolver instead of awaiting CNContactStore.requestAccess.

Reproducibility: yes. from source inspection: current main awaits ContactResolver.create() in chats and history, and the .notDetermined branch awaits requestAccess. The PR-introduced regression is also source-reproducible because the shared resolver is used by documented contact-name send/RPC paths.

Review metrics: 2 noteworthy metrics.

  • Diff size: 1 file changed, 5 added, 3 deleted. The patch is small, but the changed file is the shared Contacts resolver used by several commands.
  • Shared resolver call sites: 7 default call sites. The .notDetermined behavior change affects read commands plus send/RPC/nickname paths, not only the reported chats/history flow.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Narrow the no-prompt behavior so explicit contact-name resolution paths can still request or require Contacts authorization.
  • [P1] Add after-fix macOS CLI proof for imsg chats and imsg history under undecided Contacts authorization, redacting private handles or chat data.
  • [P1] Add focused regression coverage for the read fail-open path and for preserving contact-name send/RPC behavior.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR includes before-fix reproduction details and expected after behavior, but no after-fix terminal output, logs, screenshot, or recording showing the changed CLI behavior in a real macOS setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The global .notDetermined fail-open behavior would skip the Contacts prompt for documented contact-name sends, RPC sends, and nickname --local, so first-run users may lose name resolution until they grant Contacts manually outside the command.
  • [P1] The PR body describes expected after behavior, but it does not include observed after-fix macOS CLI output showing chats or history returning under undecided Contacts authorization.

Maintainer options:

  1. Scope no-prompt behavior to read paths (recommended)
    Add a resolver mode or command-level injection that skips Contacts prompts for chats and history enrichment while preserving authorization for send --to contact names, RPC send, and nickname --local.
  2. Accept the name-resolution behavior change
    Maintainers can intentionally accept global fail-open behavior if undecided Contacts authorization should no longer prompt for contact-name sends until users grant Contacts manually.

Next step before merge

  • [P1] Needs contributor and maintainer follow-up to narrow the resolver behavior and provide real after-fix macOS proof before merge.

Security
Cleared: The diff only changes a local Contacts authorization branch and does not touch dependencies, CI, secrets, release scripts, or other supply-chain surfaces.

Review findings

  • [P1] Preserve Contacts prompting for name-resolution paths — Sources/IMsgCore/ContactResolver.swift:74
Review details

Best possible solution:

Add an internal nonprompting Contacts resolver mode for optional read-path enrichment, while preserving an authorization/request path for explicit contact-name resolution flows.

Do we have a high-confidence way to reproduce the issue?

Yes, from source inspection: current main awaits ContactResolver.create() in chats and history, and the .notDetermined branch awaits requestAccess. The PR-introduced regression is also source-reproducible because the shared resolver is used by documented contact-name send/RPC paths.

Is this the best way to solve the issue?

No. The fail-open direction is right for optional read enrichment, but applying it inside the shared resolver is broader than the reported bug and removes the authorization path for contact-name resolution.

Full review comments:

  • [P1] Preserve Contacts prompting for name-resolution paths — Sources/IMsgCore/ContactResolver.swift:74
    This branch makes the shared .notDetermined Contacts case always return a no-op resolver. That fixes the read-command hang, but the same resolver is used by send --to contact-name resolution and RPC send, while the docs say contact names are resolved through Address Book with Contacts permission. With undecided permission, those flows now fall through with the literal name instead of getting a chance to authorize and resolve the contact. Scope the no-prompt behavior to read/enrichment paths or add an explicit nonprompting resolver mode.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 3eb6f414ab73.

Label changes

Label changes:

  • add P2: The PR addresses a bounded but user-visible hang in core read commands, with a compatibility blocker before merge.
  • add merge-risk: 🚨 compatibility: The diff changes first-run Contacts authorization behavior for documented contact-name resolution paths beyond the targeted read commands.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR includes before-fix reproduction details and expected after behavior, but no after-fix terminal output, logs, screenshot, or recording showing the changed CLI behavior in a real macOS setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: The PR addresses a bounded but user-visible hang in core read commands, with a compatibility blocker before merge.
  • merge-risk: 🚨 compatibility: The diff changes first-run Contacts authorization behavior for documented contact-name resolution paths beyond the targeted read commands.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR includes before-fix reproduction details and expected after behavior, but no after-fix terminal output, logs, screenshot, or recording showing the changed CLI behavior in a real macOS setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Current main awaits Contacts authorization in the reported path: ContactResolver.create() on current main still calls requestAccess(store:) for .notDetermined, which is the blocking path described by the linked issue. (Sources/IMsgCore/ContactResolver.swift:69, 3eb6f414ab73)
  • PR changes the shared resolver behavior: The proposed commit replaces the .notDetermined permission request with an unconditional unavailable no-op resolver. (Sources/IMsgCore/ContactResolver.swift:74, 8a63ccfa7537)
  • The resolver is shared beyond chats/history: There are 7 default ContactResolver.create call sites, including RPC startup, send, watch, search, chats/history, and local nickname lookup. (Sources/imsg/Commands/SendCommand.swift:55, 3eb6f414ab73)
  • Send contact-name behavior is documented: The send docs state that --to accepts a contact name resolved through Address Book and requiring Contacts permission, so globally returning a no-op resolver changes a documented path. (docs/send.md:21, 3eb6f414ab73)
  • Permissions docs scope Contacts to read and send name resolution: The permissions docs say Contacts protects name resolution in any read or send command, while skipped Contacts should leave resolved-name fields empty for JSON output. (docs/permissions.md:53, 3eb6f414ab73)
  • Feature history for Contacts resolver: Local blame ties the current request-access branch to the release source, and GitHub commit metadata for 5203f7461c49932da6c4f46d7876d9ffa3012cec shows the contact-name feature introduced ContactResolver.swift and direct-send name resolution. (Sources/IMsgCore/ContactResolver.swift:63, 5203f7461c49)

Likely related people:

  • steipete: Current blame for ContactResolver.create() points to the release source, and GitHub commit metadata shows the contact-name resolver feature was introduced by 5203f7461c49932da6c4f46d7876d9ffa3012cec. (role: feature introducer and recent area contributor; confidence: high; commits: 5203f7461c49, c3205e1361bf; files: Sources/IMsgCore/ContactResolver.swift, Sources/imsg/Commands/ChatsCommand.swift, Sources/imsg/Commands/HistoryCommand.swift)
  • jsindy: The contact-name resolution feature history credits this handle in the release changelog context and the same feature added the resolver and command-level name-resolution behavior. (role: feature co-author; confidence: medium; commits: 5203f7461c49; files: Sources/IMsgCore/ContactResolver.swift, Sources/imsg/ChatTargetResolver.swift, Sources/imsg/Commands/SendCommand.swift)
  • regaw-leinad: The contact-name resolution feature history credits this handle in the release changelog context for the feature that added Contacts-backed output and direct-send name resolution. (role: feature co-author; confidence: medium; commits: 5203f7461c49; files: Sources/IMsgCore/ContactResolver.swift, Sources/imsg/ChatTargetResolver.swift, Tests/imsgTests/ContactResolutionTests.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: imsg chats/history can hang when Contacts auth is not determined

1 participant