fix: fail open on undetermined contacts auth by cemendes · Pull Request #136 · openclaw/imsg

cemendes · 2026-06-01T21:46:54Z

Summary

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

Root cause

ContactResolver.create() awaited CNContactStore.requestAccess(for: .contacts) whenever Contacts auth was undecided. In CLI/headless contexts that callback can fail to resolve promptly, so read commands that only need optional contact-name enrichment can appear to hang even after Messages DB work finished.

Fix

Add an explicit ContactsAccessPolicy with the existing default behavior preserved as requestIfNeeded.
Use skipIfNotDetermined only for chats and history, so those read commands fail open without contact-name enrichment.
Keep send, RPC, bridge intro, and other explicit contact/name-resolution paths on the default requesting resolver.
Add resolver tests for both branches: skip policy does not request Contacts, default policy still does.
Add an 0.11.1 changelog entry thanking @cemendes.

Closes #135.

Verification

swift test --filter ContactResolver passed: 4 tests.
swift test --filter 'ChatHistory|ContactResolver|sendCommandResolvesUniqueContactName|sendCommandRejectsAmbiguousContactName|accountNickname' passed: 26 tests, including send/name-resolution coverage.
make build passed and produced bin/imsg plus bin/imsg-bridge-helper.dylib; only existing helper deprecation warnings were emitted.
Contacts status on this Mac was .notDetermined (CNContactStore.authorizationStatus(for: .contacts).rawValue == 0).
Live proof under .notDetermined: bin/imsg chats --limit 1 --json returned within a 10s alarm; sanitized output showed one chat and no contact_name field.
Live proof under .notDetermined: bin/imsg history --chat-id <id> --limit 1 --json returned within a 10s alarm; sanitized output showed one message and no sender_name field.
make lint exited 0; repo has existing warning-level file length/line length violations, no serious findings.
make test passed: 323 tests.

Maintainer note

Local autoreview was attempted but the review engine returned empty output after a long-running child process was stopped, so it is not counted as proof here. The patch was manually reviewed against all ContactResolver.create call sites: only chats and history opt into no-prompt behavior; send/RPC/name paths still use the default requesting behavior.

clawsweeper · 2026-06-01T21:47:58Z

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:

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.
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.

steipete · 2026-06-03T21:11:25Z

@clawsweeper re-review

Pushed maintainer fixup at a783922.

What changed since the blocked review:

The global .notDetermined fail-open was narrowed behind ContactsAccessPolicy.
Only chats and history use skipIfNotDetermined.
send, RPC, bridge intro, and other explicit contact/name-resolution flows still use default requestIfNeeded behavior.
Added resolver regression tests for both policies.
Added live proof on this Mac with Contacts status .notDetermined (rawValue == 0): both bin/imsg chats --limit 1 --json and bin/imsg history --chat-id <id> --limit 1 --json returned within a 10s alarm, with sanitized output showing no contact-name enrichment.

Local proof: make build, focused Swift tests, make lint (exit 0, existing warnings only), and make test (323 tests) passed.

clawsweeper · 2026-06-03T21:11:28Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26913316255
Updated: 2026-06-03T21:13:36.099Z

fix: fail open on undetermined contacts auth

8a63ccf

fix: avoid Contacts prompt for read commands

a783922

steipete merged commit 041b406 into openclaw:main Jun 3, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fail open on undetermined contacts auth#136

fix: fail open on undetermined contacts auth#136
steipete merged 2 commits into
openclaw:mainfrom
cemendes:fix/contacts-auth-fail-open

cemendes commented Jun 1, 2026 •

edited by steipete

Loading

Uh oh!

clawsweeper Bot commented Jun 1, 2026 •

edited

Loading

Uh oh!

steipete commented Jun 3, 2026

Uh oh!

clawsweeper Bot commented Jun 3, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cemendes commented Jun 1, 2026 • edited by steipete Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

Fix

Verification

Maintainer note

Uh oh!

clawsweeper Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jun 3, 2026

Uh oh!

clawsweeper Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cemendes commented Jun 1, 2026 •

edited by steipete

Loading

clawsweeper Bot commented Jun 1, 2026 •

edited

Loading

clawsweeper Bot commented Jun 3, 2026 •

edited

Loading