Skip to content

fix(rpc): do not block startup on undetermined Contacts auth - #187

Open
SebTardif wants to merge 4 commits into
openclaw:mainfrom
SebTardif:fix/rpc-contacts-skip-undetermined
Open

fix(rpc): do not block startup on undetermined Contacts auth#187
SebTardif wants to merge 4 commits into
openclaw:mainfrom
SebTardif:fix/rpc-contacts-skip-undetermined

Conversation

@SebTardif

@SebTardif SebTardif commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Update (2026-07-26)

Scoped Contacts fail-open to noninteractive stdin only (ClawSweeper P1):

  • stdin not a TTY → .skipIfNotDetermined (headless LaunchAgent/pipe)
  • interactive TTY → .requestIfNeeded (preserve prompt path)

Tests: RpcCommandContactsPolicyTests (headless + interactive).

What Problem This Solves

imsg rpc initializes Contacts before starting the JSON-RPC server:

let contacts = await ContactResolver.create()  // default: .requestIfNeeded
let server = RPCServer(..., contactResolver: contacts)
try await server.run()

When Contacts authorization is still .notDetermined, requestIfNeeded awaits CNContactStore.requestAccess. In a headless Aqua-domain LaunchAgent that prompt can remain pending indefinitely. The process stays alive but never accepts JSON-RPC, so methods such as chats.list, messages.history, and send.rich hang before the server starts.

Issue #186 reproduces this on macOS with imsg 0.12.3+ and the same call path on current main.

Evidence

Prior art (same repo)

  • #136 already added ContactsAccessPolicy.skipIfNotDetermined and applied it to imsg chats and imsg history so CLI list/read commands fail open. imsg rpc was left on the default when RpcCommand was later extracted (commit b5b7464, 2026-07-11).
  • ContactResolver.create(accessPolicy: .skipIfNotDetermined) already returns NoOpContactResolver(contactsUnavailable: true) without calling requestAccess.

Fix

Wire RPC startup to the same fail-open policy:

static let startupContactsAccessPolicy: ContactsAccessPolicy = .skipIfNotDetermined
// ...
await ContactResolver.create(accessPolicy: startupContactsAccessPolicy)

Interactive CLI send paths keep the default prompt-if-needed behavior. Authorized installations still load contacts normally on RPC start (existing ContactResolver authorized branch; unchanged).

Live headless transcript (after fix)

Environment: macOS 26.5.2, Contacts authorization notDetermined (CNContactStore.authorizationStatus(for: .contacts) = 0), built binary at commit f46cec8, non-TTY subprocess (LaunchAgent-equivalent headless stdio). Fixture Messages DB via --db so startup reaches ContactResolver.create (real Messages path would also pass FDA when granted).

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

$ ./bin/imsg rpc --db /tmp/imsg-187-fixture-chat.db
>>> {"jsonrpc":"2.0","id":"p1","method":"chats.list","params":{"limit":5}}
>>> {"jsonrpc":"2.0","id":"p2","method":"messages.history","params":{"chat_id":1,"limit":2}}
<<< {"jsonrpc":"2.0","id":"p1","result":{"chats":[{"id":1,"guid":"iMessage;-;+15555550100","identifier":"+15555550100","name":"Proof Contact","participants":["+15555550100"],"service":"iMessage","is_group":false,"unread_count":0,"last_message_at":"2023-03-08T20:26:40.000Z"}]}}
<<< {"jsonrpc":"2.0","id":"p2","result":{"messages":[{"id":1,"chat_id":1,"text":"hello from fixture","sender":"+15555550100","participants":["+15555550100"],"is_from_me":false,"is_read":true,"created_at":"2023-03-08T20:26:40.000Z",...}]}}
process exit=0 elapsed_s=0.039 responses=2

Server accepted two methods in under 50ms while Contacts remained undetermined (no prompt hang). Participant handles left as fixture placeholders (+15555550100).

Real behavior proof

  • Behavior or issue addressed: Headless imsg rpc must start the JSON-RPC server without blocking on an undetermined Contacts permission prompt (hang for LaunchAgents / non-interactive hosts).

  • Real environment tested: macOS 26.5.2 (Build 25F84), arm64, Contacts authorization status notDetermined (raw 0) on the host. Patched binary f46cec8 at /tmp/imsg-186/bin/imsg. Headless non-TTY stdio subprocess (equivalent to LaunchAgent / gateway child). Fixture chat.db so FDA is not required and startup still runs ContactResolver.create.

  • Exact steps or command run after this patch:

    swift -e 'import Contacts; print(CNContactStore.authorizationStatus(for: .contacts).rawValue)'
    # prints 0 (notDetermined)
    make build ARCHES=$(uname -m)
    ./bin/imsg rpc --db /tmp/imsg-187-fixture-chat.db
    # write one JSON-RPC line per request on stdin; read responses on stdout
  • Evidence after fix: terminal output from the headless RPC session above: chats.list and messages.history both returned result objects with exit 0 in 0.039s while Contacts stayed undetermined.

  • Observed result after fix: RPC no longer waits on requestAccess when Contacts is undetermined. The server reaches RPCServer.run(), accepts methods, and returns JSON-RPC results. Name enrichment remains optional until Contacts is already authorized (fixture used chat display_name / handles only).

  • What was not tested: Live authorized-Contacts path on this host (authorization is notDetermined, not authorized). Existing ContactResolver authorized branch is unchanged; interactive CLI send paths still use .requestIfNeeded. Real LaunchAgent plist install was not used; proof used an equivalent non-TTY stdio child process.

Notes

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 20, 2026
@clawsweeper

clawsweeper Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed July 26, 2026, 9:23 PM ET / July 27, 2026, 01:23 UTC.

ClawSweeper review

What this changes

This PR makes imsg rpc skip an undecided Contacts permission prompt only when stdin is noninteractive, while retaining prompt-capable Contacts setup for interactive terminals.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep this PR open as the active candidate fix for the headless RPC startup hang described by #186. The scoped noninteractive policy preserves the interactive Contacts prompt path that the earlier review required; the remaining concrete cleanup is removing the release-owned CHANGELOG.md entry before merge.

Priority: P1
Reviewed head: b8bcf52b8f944f34121f0e8ae4ee90d6cc71a8f6

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The PR has focused implementation, passing platform checks, and useful live headless proof; removing the release-owned changelog edit is the remaining merge cleanup.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The PR body supplies a redacted real macOS non-TTY terminal transcript showing two successful JSON-RPC calls while Contacts authorization is undetermined; the final scoped policy retains that same headless branch.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body supplies a redacted real macOS non-TTY terminal transcript showing two successful JSON-RPC calls while Contacts authorization is undetermined; the final scoped policy retains that same headless branch.
Evidence reviewed 5 items Scoped RPC policy: The PR head chooses requestIfNeeded for interactive stdin and skipIfNotDetermined for noninteractive stdin, then supplies that resolver before starting the JSON-RPC server.
Focused regression coverage: The added policy suite covers both relevant modes: headless stdin selects the non-blocking resolver policy and interactive stdin retains the permission-request policy.
Prior Contacts-policy provenance: The merged predecessor #136 introduced skipIfNotDetermined for read-oriented commands while retaining prompt-capable behavior elsewhere; this PR narrows the RPC extension to noninteractive use rather than changing all RPC sessions.
Findings 1 actionable finding [P3] Remove the release-owned changelog entry
Security None None.

How this fits together

imsg rpc opens the Messages database and creates a Contacts resolver before it starts its stdin/stdout JSON-RPC server. The resolver supplies optional contact-name enrichment to RPC methods, so its authorization policy determines whether a headless host can begin serving requests.

flowchart LR
  A[RPC stdin and stdout] --> B[RPC startup]
  B --> C{Is stdin a TTY?}
  C -->|Interactive| D[Request Contacts access if needed]
  C -->|Headless| E[Use unavailable Contacts resolver]
  D --> F[JSON-RPC server]
  E --> F
  F --> G[Chat history and send methods]
Loading

Before merge

  • Remove the release-owned changelog entry (P3) - CHANGELOG.md is release-owned for normal PR review. Keep this PR's user-visible summary in its body or commit message and leave release-note selection to the release process.
  • Resolve merge risk (P1) - Merging changes behavior for noninteractive RPC callers with undecided Contacts authorization: they will start promptly but operate without Contacts-backed name enrichment until authorization is granted through an interactive path.
  • Resolve merge risk (P1) - The contributor-owned CHANGELOG.md edit conflicts with the release-owned changelog policy and should not be carried by this normal PR.

Findings

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:26
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 3 files affected; 52 added, 1 removed The implementation is focused on RPC startup policy, a small regression suite, and one release-owned documentation edit.
Mode coverage 2 policy cases added The tests explicitly protect both the headless fail-open path and the interactive prompt-capable path.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #186
Summary: This PR is the active implementation candidate for the concrete headless RPC startup failure tracked by the canonical issue; the earlier Contacts-policy PR is related prior art but addressed different entrypoints.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Keep the scoped compatibility boundary (recommended)
    Remove the release-owned changelog edit, then merge the noninteractive-only behavior so LaunchAgents avoid the native permission wedge while interactive terminals keep their existing prompt path.
  2. Retain prompt behavior everywhere
    Do not merge this policy change if maintainers require every RPC invocation, including headless ones, to attempt Contacts authorization before serving requests.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Remove the `CHANGELOG.md` entry; keep the user-visible release context in the PR body and preserve the scoped Contacts policy and tests.

Technical review

Best possible solution:

Land the scoped headless fail-open policy with its two-mode tests after removing the CHANGELOG.md entry; retain the user-visible release context in the PR body or commit message for release owners.

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

No current-main live reproduction was run in this review. The supplied report and PR body provide a concrete macOS non-TTY reproduction path, and the PR head directly targets that startup policy branch.

Is this the best way to solve the issue?

Yes, with one release-process cleanup. Limiting the non-blocking resolver to noninteractive stdin is the narrowest approach because it fixes LaunchAgent and pipe startup while retaining interactive Contacts authorization behavior.

Full review comments:

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:26
    CHANGELOG.md is release-owned for normal PR review. Keep this PR's user-visible summary in its body or commit message and leave release-note selection to the release process.
    Confidence: 0.98

Overall correctness: patch is correct
Overall confidence: 0.87

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P1: A headless RPC process can remain alive while accepting no requests, blocking core chat, history, and send RPC workflows.
  • merge-risk: 🚨 compatibility: Noninteractive RPC sessions with undetermined Contacts permission will intentionally stop prompting and may lack contact-name enrichment until authorization is granted elsewhere.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body supplies a redacted real macOS non-TTY terminal transcript showing two successful JSON-RPC calls while Contacts authorization is undetermined; the final scoped policy retains that same headless branch.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies a redacted real macOS non-TTY terminal transcript showing two successful JSON-RPC calls while Contacts authorization is undetermined; the final scoped policy retains that same headless branch.

Evidence

Acceptance criteria:

  • [P1] make lint.
  • [P1] make test.

What I checked:

  • Scoped RPC policy: The PR head chooses requestIfNeeded for interactive stdin and skipIfNotDetermined for noninteractive stdin, then supplies that resolver before starting the JSON-RPC server. (Sources/imsg/Commands/RpcCommand.swift:14, b8bcf52b8f94)
  • Focused regression coverage: The added policy suite covers both relevant modes: headless stdin selects the non-blocking resolver policy and interactive stdin retains the permission-request policy. (Tests/imsgTests/RpcCommandContactsPolicyTests.swift:8, b8bcf52b8f94)
  • Prior Contacts-policy provenance: The merged predecessor fix: fail open on undetermined contacts auth #136 introduced skipIfNotDetermined for read-oriented commands while retaining prompt-capable behavior elsewhere; this PR narrows the RPC extension to noninteractive use rather than changing all RPC sessions. (Sources/IMsgCore/ContactResolver.swift, 041b40686a6d)
  • Real headless behavior proof: The PR body includes a macOS non-TTY terminal transcript with Contacts authorization still undetermined where chats.list and messages.history returned JSON-RPC results in 0.039 seconds. The final scoped policy retains that same non-TTY branch. (f46cec854821)
  • Current PR validation: The supplied PR state reports successful macos and linux-read-core checks at the current head; the last commit adds conditional Darwin/Glibc imports for isatty portability. (Sources/imsg/Commands/RpcCommand.swift:5, b8bcf52b8f94)

Likely related people:

  • cemendes: Their merged Contacts fail-open work established the access-policy split that this PR extends to noninteractive RPC startup. (role: introduced Contacts access-policy behavior; confidence: high; commits: 041b40686a6d; files: Sources/IMsgCore/ContactResolver.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Remove the CHANGELOG.md entry and keep the release context in the PR body or commit message.

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 (5 earlier review cycles)
  • reviewed 2026-07-20T07:23:44.923Z sha f46cec8 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-20T21:20:02.417Z sha 3600199 :: found issues before merge. :: [P1] Preserve interactive Contacts authorization for RPC
  • reviewed 2026-07-20T22:02:22.021Z sha 3600199 :: found issues before merge. :: [P1] Preserve a prompt-capable interactive RPC path
  • reviewed 2026-07-26T13:13:02.999Z sha 6e36348 :: needs changes before merge. :: [P3] Remove the release-owned changelog entry
  • reviewed 2026-07-26T13:18:16.510Z sha 6e36348 :: needs changes before merge. :: [P3] Remove the release-owned changelog entry

@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Added live headless imsg rpc request/response transcript with Contacts authorization notDetermined (host status raw 0). Patched binary accepted chats.list and messages.history with JSON-RPC results in 0.039s (no Contacts prompt hang). Fixture DB via --db so startup reaches ContactResolver; non-TTY stdio child is LaunchAgent-equivalent. PR body Evidence + Real behavior proof sections updated.

@clawsweeper

clawsweeper Bot commented Jul 20, 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 repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added 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. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 21, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

The ClawSweeper review raises a fair point: making skipIfNotDetermined unconditional for all RPC changes the contract that PR #136 set up (fail-open for chats/history reads, but prompt-capable for RPC).

Two options:

  1. Scope to headless (ClawSweeper recommended): apply skipIfNotDetermined only when the RPC startup detects a noninteractive/headless context, and keep the prompt path for interactive sessions. Add coverage for both modes.
  2. Accept all-RPC fail-open: document that RPC no longer requests Contacts authorization at startup, which means contact-name resolution may be unavailable until the user grants access through another command.

@cemendes, since you introduced the Contacts policy split in #136, could you weigh in on which approach fits the intended design? Happy to implement whichever direction you prefer.

@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Scoped fail-open Contacts policy to noninteractive stdin (isatty). Interactive TTY keeps requestIfNeeded. Policy unit tests added for both modes.

@clawsweeper

clawsweeper Bot commented Jul 26, 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 repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

imsg rpc used ContactResolver.create() with the default
requestIfNeeded policy, so a headless LaunchAgent could hang forever
waiting on CNContactStore.requestAccess before the JSON-RPC server
started. Use skipIfNotDetermined (same as chats/history) so RPC starts
with optional name enrichment disabled until Contacts is already
authorized.

Closes openclaw#186

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
ClawSweeper P1: unconditional skipIfNotDetermined changed the RPC
authorization contract for interactive callers. Keep requestIfNeeded when
stdin is a TTY; use skipIfNotDetermined only for headless pipes/LaunchAgents.

Adds pure policy tests for both modes.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif
SebTardif force-pushed the fix/rpc-contacts-skip-undetermined branch from 3600199 to 6e36348 Compare July 26, 2026 13:09
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary 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 Jul 26, 2026
Unconditional import Darwin breaks linux-read-core (no Darwin module).
Match SecurePath and use canImport Darwin/Glibc for isatty/STDIN_FILENO.

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

Copy link
Copy Markdown
Contributor Author

Linux CI regression fixed: unconditional import Darwin broke linux-read-core. Now uses the same #if canImport(Darwin) / Glibc pattern as SecurePath.

linux-read-core and macos are green on head b8bcf52.

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. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary 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.

rpc: avoid blocking startup on an undetermined Contacts permission

1 participant