Skip to content

fix(cli): do not block watch/search on undetermined Contacts - #238

Merged
steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/watch-search-contacts-skip
Aug 16, 2026
Merged

fix(cli): do not block watch/search on undetermined Contacts#238
steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/watch-search-contacts-skip

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

Merged #187 (issue #186) fixed headless imsg rpc so Contacts .notDetermined does not block forever. CLI watch and search still called ContactResolver.create() with the default .requestIfNeeded:

// WatchCommand.run / SearchCommand.run default factory
await ContactResolver.create()

When Contacts authorization is still .notDetermined, that awaits CNContactStore.requestAccess. In a headless (non-TTY) process the prompt can stay pending, so imsg watch and imsg search hang before they emit output. chats and history already use .skipIfNotDetermined (#136). imsg send still prompts, because name targets need Contacts.

Evidence

Prior art (same repo)

  • #136 added ContactsAccessPolicy.skipIfNotDetermined for imsg chats and imsg history.
  • #187 applied a TTY-aware rule to imsg rpc (stdin not a TTY -> skip; interactive TTY -> request). watch and search were left on the default.
  • ContactResolver.create(accessPolicy: .skipIfNotDetermined) already returns NoOpContactResolver(contactsUnavailable: true) without calling requestAccess.

Fix

The TTY rule now lives once on ContactsAccessPolicy.forStdin(isTTY:). RPC, watch, and search factories all use it. imsg send is unchanged.

Live headless search (after fix)

Environment: macOS 26.6.1 (Build 25G76), arm64, Contacts authorization notDetermined (raw 0), patched binary at commit 545aa3d, stdin from /dev/null (not a TTY). Fixture Messages DB via --db so the command reaches ContactResolver.create.

$ swift -e 'import Contacts; print(CNContactStore.authorizationStatus(for: .contacts).rawValue)'
0

$ ./bin/imsg search --query hello --limit 5 --db /tmp/imsg-watch-search-contacts-fixture.db < /dev/null
2026-08-15T21:17:18.121Z [recv] +15555550100: hello from fixture
process exit=0 elapsed_s=0.755

Search returned a fixture hit in under a second while Contacts stayed undetermined (no prompt hang). Headless imsg watch on the same DB stayed in its event loop (killed after 2.013s), so Contacts did not block startup.

Real behavior proof

  • Behavior or issue addressed: Headless imsg watch and imsg search must not block on an undetermined Contacts permission prompt. Interactive TTY still uses the prompt-capable path. imsg send still prompts.

  • Real environment tested: macOS 26.6.1 (Build 25G76), arm64, Contacts authorization status notDetermined (raw 0) on the host. Patched binary 545aa3d at /tmp/oc-impl-imsg-contacts/bin/imsg. Headless non-TTY stdin (/dev/null). Fixture chat.db so FDA is not required and the command still runs ContactResolver.create.

  • Exact steps or command run after this patch:

    swift -e 'import Contacts; print(CNContactStore.authorizationStatus(for: .contacts).rawValue)'
    make build ARCHES=$(uname -m)
    ./bin/imsg search --query hello --limit 5 --db /tmp/imsg-watch-search-contacts-fixture.db < /dev/null
  • Evidence after fix: terminal output from the headless search session above: imsg search --query hello printed the fixture message and exited 0 in 0.755s while Contacts stayed undetermined. Headless imsg watch on the same DB was still running after 2.013s (Contacts did not block startup).

  • Observed result after fix: watch and search no longer wait on requestAccess when stdin is not a TTY and Contacts is undetermined. Name enrichment remains optional until Contacts is already authorized (fixture used the raw handle +15555550100).

  • What was not tested: Live authorized-Contacts path on this host (authorization is notDetermined, not authorized). Interactive TTY prompt path was not exercised here (policy keeps .requestIfNeeded for TTY). imsg send was not changed. Real LaunchAgent plist install was not used; proof used equivalent non-TTY stdin.

Notes

  • Sibling of #187 / #136
  • Changelog entry under Unreleased / Fixes
  • Shared helper is ContactsAccessPolicy.forStdin(isTTY:) so the TTY rule is not copied in three places

Headless imsg watch and imsg search called ContactResolver.create()
with the default requestIfNeeded policy, so a .notDetermined Contacts
authorization could hang on the TCC prompt. Share the RPC TTY rule
(skipIfNotDetermined when stdin is not a TTY). send still prompts.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added 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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 15, 2026, 10:43 PM ET / August 16, 2026, 02:43 UTC.

ClawSweeper review

What this changes

The branch centralizes stdin TTY detection and makes headless search and watch skip an unresolved Contacts permission prompt while retaining interactive prompting.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

This PR remains necessary: current main still requests Contacts access before search and watch can run, while the branch scopes the established non-TTY fail-open policy to those read commands. The patch is focused, preserves interactive prompting and send, and has sufficient real macOS proof.

Priority: P2
Reviewed head: 5006f7ff2149d93ef2a0845dc41ad99bb6515a5c

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) A narrow, source-confirmed repair with deterministic policy coverage and direct macOS after-fix terminal proof.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR body provides a concrete after-fix macOS terminal transcript with Contacts still undetermined, non-TTY stdin, a fixture database, successful search output, and confirmed watch startup.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body provides a concrete after-fix macOS terminal transcript with Contacts still undetermined, non-TTY stdin, a fixture database, successful search output, and confirmed watch startup.
Evidence reviewed 5 items Current-main blocking path: Current main defaults ContactResolver.create to requestIfNeeded and awaits Contacts access when authorization is undetermined; search and watch both call that default factory before emitting output.
Focused branch repair: The PR moves the existing TTY decision into ContactsAccessPolicy, applies it to only search, watch, and the existing RPC caller, and leaves send on its prompt-capable default.
Regression coverage: The branch adds deterministic policy tests for the shared helper plus RPC, watch, and search TTY branches; existing core tests verify that the skip policy does not request Contacts.
Findings None None.
Security None None.

How this fits together

The imsg CLI reads and watches the local Messages database, then optionally resolves handle display names through Contacts. Contacts authorization is selected during command startup, before search results or a watch stream can be emitted.

flowchart LR
  A[CLI stdin] --> B[TTY detection]
  B --> C[Contacts access policy]
  C --> D[Contact resolver]
  D --> E[Search or watch command]
  E --> F[Message output or stream]
Loading

Before merge

  • Resolve merge risk (P1) - Merging intentionally changes non-TTY search and watch output under undetermined Contacts from waiting for a prompt to proceeding with raw handles rather than resolved names; interactive terminals retain the existing prompt path.
  • Complete next step (P2) - No repair lane is needed; the patch has no actionable review finding and only awaits ordinary maintainer merge review and the normal macOS check.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Implementation and test scope 6 files; production +36/-8, tests +39, release notes +4 The functional change is limited to the shared Contacts policy and three command entrypoints, with policy coverage for both TTY branches.

Merge-risk options

Maintainer options:

  1. Keep the non-TTY compatibility boundary (recommended)
    Merge the existing narrowly scoped policy so only headless stdin falls back to raw handles when Contacts permission is unresolved.

Technical review

Best possible solution:

Land the shared TTY policy so noninteractive read automation starts reliably while interactive use and explicit name-resolution flows keep their prompt-capable behavior.

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

Yes, source-reproducible: on macOS with Contacts authorization undetermined and non-TTY stdin, current main calls the default prompt-capable resolver before search or watch output. The PR also supplies a redacted live macOS terminal transcript showing the after-fix search result and watch startup.

Is this the best way to solve the issue?

Yes. Reusing the merged RPC TTY rule at the shared policy layer is the narrowest maintainable repair, and it leaves interactive prompting and send unchanged.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 268912be7189.

Labels

Label justifications:

  • P2: This fixes a bounded but real headless CLI reliability failure in two read workflows.
  • merge-risk: 🚨 compatibility: Noninteractive users with unresolved Contacts permission will receive raw handles instead of waiting for name resolution.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body provides a concrete after-fix macOS terminal transcript with Contacts still undetermined, non-TTY stdin, a fixture database, successful search output, and confirmed watch startup.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a concrete after-fix macOS terminal transcript with Contacts still undetermined, non-TTY stdin, a fixture database, successful search output, and confirmed watch startup.

Evidence

What I checked:

Likely related people:

  • SebTardif: Authored merged PR 187, which introduced the same non-TTY Contacts policy for RPC, and the current PR extends that established boundary. (role: prior merged feature author and recent area contributor; confidence: high; commits: 2fac78e96c1c; files: Sources/imsg/Commands/RpcCommand.swift, Tests/imsgTests/RpcCommandContactsPolicyTests.swift)
  • cemendes: Authored merged PR 136, which introduced the explicit Contacts access policy and fail-open behavior for earlier read commands. (role: initial Contacts-policy contributor; confidence: high; commits: 041b40686a6d; files: Sources/IMsgCore/ContactResolver.swift, Sources/imsg/Commands/ChatsCommand.swift, Sources/imsg/Commands/HistoryCommand.swift)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

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

History

Review history (2 earlier review cycles)
  • reviewed 2026-08-15T21:22:04.896Z sha 545aa3d :: needs changes before merge. :: [P2] Remove the release-owned changelog entry
  • reviewed 2026-08-15T21:56:06.742Z sha 63f566f :: needs maintainer review before merge. :: none

CHANGELOG.md is release-owned; keep the user-visible context
in the PR body.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

[P2] Remove the release-owned changelog entry

Dropped the Unreleased CHANGELOG.md edit on 63f566f. User-visible context stays in the PR body.

@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 15, 2026
@steipete
steipete merged commit 21eaaf0 into openclaw:main Aug 16, 2026
4 checks passed
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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants