Skip to content

feat: add guarded contact-card probes#166

Closed
omarshahine wants to merge 2 commits into
openclaw:mainfrom
omarshahine:feat/contact-card
Closed

feat: add guarded contact-card probes#166
omarshahine wants to merge 2 commits into
openclaw:mainfrom
omarshahine:feat/contact-card

Conversation

@omarshahine

@omarshahine omarshahine commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Important caveat

This PR does not make native contact-card sharing work on lobster. Live proof showed the private controller path can block Messages.app, so the implementation deliberately fails closed: status reports unavailable and share returns a clear error instead of attempting a send.

Summary

  • add guarded imsg contact-card status|share --chat <guid> commands that expose the attempted surface without claiming share support is available
  • expose RPC contacts.shouldShareContact and contacts.shareContactCard, with shareContactCard returning unavailable on current macOS/lobster proof
  • fail closed on current macOS instead of invoking IMNicknameController.sharedController, which live proof showed can block Messages.app
  • add a retained GCD timer beside the existing v2 inbox NSTimer so bridge RPC requests are consumed reliably after injection
  • remove the release-owned changelog edit; release-note context stays in this PR body

Redacted runtime proof on lobster

Proof collected on lobster with branch-local artifacts from feat/contact-card after relaunching Messages.app with the branch dylib. Raw logs stay local because they include chat GUIDs and local identifiers.

  • branch head: f45875d
  • runtime proof directory: /Users/lobster/imsg-pr-proofs/2026-07-08/166-contact-card/run-20260708-220445
  • test chat: Lobster Test Group (chat row 22; GUID redacted in PR text)
  • bridge launch: ./bin/imsg launch --dylib "$PWD/bin/imsg-bridge-helper.dylib"
  • IPC proof: manual v2 ping request was consumed after adding the retained GCD inbox timer and returned {"success":true,"data":{"pong":true}}
  • status proof: imsg contact-card status --chat <redacted> --json exited 0 and returned available:false, controller_class_available:1, controller_available:false, should_share:null, status_probe_invoked:false, and all known contact-card selectors as false
  • fail-closed share proof: imsg contact-card share --chat <redacted> --json exited 1 with Contact-card sharing unavailable: private controller probing is disabled because it can block Messages.app
  • behavior before hardening: live branch proof showed contact-card status could time out after resolving the chat because IMNicknameController.sharedController probing blocked inside Messages.app

Redacted status output:

{
  "chat": "Lobster Test Group",
  "chat_id": 22,
  "chat_guid": "<redacted>",
  "controller_class_available": 1,
  "controller_available": false,
  "available": false,
  "should_share": null,
  "status_probe_invoked": false,
  "status_probe_reason": "selector availability only; private status selectors are not invoked",
  "probe_reason": "IMNicknameController.sharedController probing is disabled because it can block Messages.app",
  "selectors": {
    "sharingStateForChat:": false,
    "shouldShareMyNameAndPhotoWithChat:": false,
    "setShouldShareNickname:forChat:": false,
    "sharingStateForChatGUID:": false,
    "shouldShareNameAndPhotoForChat:": false,
    "shareNicknameWithChat:": false,
    "shareNameAndPhotoWithChat:": false,
    "shouldShareNicknameWithChat:": false,
    "shareMyNameAndPhotoWithChat:": false,
    "setShouldShareMyNameAndPhoto:forChat:": false
  }
}

Redacted share output:

{
  "chat": "Lobster Test Group",
  "chat_id": 22,
  "chat_guid": "<redacted>",
  "success": false,
  "error": "Dylib error: Contact-card sharing unavailable: private controller probing is disabled because it can block Messages.app"
}

Verification

  • make build ✅ on lobster for f45875d
  • swift format lint --recursive Sources Tests TestsLinux
  • git diff --check
  • make lint ⚠️ blocked after swift-format by local SwiftLint/SourceKit crash: Loading sourcekitdInProc.framework/Versions/A/sourcekitdInProc failed
  • make test ⚠️ blocked before project tests run on this host: Tests/IMsgCoreTests/BridgeHelperLocatorTests.swift:2:8: error: no such module 'Testing'
  • GitHub CI ✅ for f45875d (macos, linux-read-core, and Socket checks passed)

@clawsweeper

clawsweeper Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 7:35 PM ET / 23:35 UTC.

Summary
The branch adds guarded contact-card CLI/RPC/bridge probes that report selector status and fail closed for sharing, plus a retained v2 inbox dispatch timer, docs, skill guidance, and tests.

Reproducibility: not applicable. as a user bug. The PR body provides redacted live output and the diff gives a high-confidence patch review path, but I did not run local runtime tests in this read-only review.

Review metrics: 3 noteworthy metrics.

  • Public bridge surface: 1 CLI command, 2 RPC methods, 2 bridge actions added. The PR expands user-facing and programmatic bridge vocabulary, so product acceptance matters before merge.
  • Changed files: 12 files changed, +286/-2. The diff crosses Objective-C injected runtime code, Swift CLI/RPC routing, docs, a bundled skill, and tests.
  • Injected helper runtime change: 1 retained dispatch timer added. The Messages.app-injected helper is the highest-risk runtime surface touched by the PR.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • none.

Risk before merge

  • [P1] The PR advertises a new contact-card CLI/RPC surface while current live proof shows sharing intentionally fails closed, so maintainers need to accept that partial product contract before merge.
  • [P1] The injected Messages helper gains a retained dispatch timer alongside the existing v2 inbox timer; the PR has live proof, but normal CI cannot fully prove Messages.app runtime availability.

Maintainer options:

  1. Accept the guarded helper path (recommended)
    Merge after a maintainer explicitly accepts the fail-closed contact-card API contract and the additional injected-helper timer as acceptable runtime risk.
  2. Ask for current-head runtime proof
    Require a fresh redacted live run from the latest head if maintainers want the retained timer and selector-introspection changes proven together before merge.
  3. Pause contact-card core surface
    Close or hold this PR if a fail-closed private-API bridge surface is not ready for imsg core.

Next step before merge

  • [P2] Manual review is needed because the remaining action is product and runtime-risk acceptance, not a narrow automated repair.

Maintainer decision needed

  • Question: Should imsg core expose imsg contact-card and contacts.*ContactCard as built-in bridge surfaces while current macOS proof shows contact-card sharing is unavailable and must fail closed?
  • Rationale: This is a new privacy-sensitive private-API bridge surface rather than a repair of existing documented behavior, and the implementation deliberately advertises a partial surface while avoiding the blocking private controller path.
  • Likely owner: steipete — This handle is tied to the current bridge selector-probe baseline and is the best available product-direction owner from inspected history.
  • Options:
    • Accept guarded core surface (recommended): Keep the proposed CLI/RPC shape and treat fail-closed unavailability as the supported initial contact-card behavior.
    • Defer or narrow the surface: Keep the PR open or ask for status-only/external exploration until maintainers want contact-card probing in core.

Security
Cleared: No concrete security or supply-chain issue was found; the private-API bridge surface is handled as product-direction and availability risk.

Review details

Best possible solution:

If maintainers accept the partial contract, land the guarded status/share surface with fail-closed behavior and treat future real sharing support as a separate runtime-proven change.

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

Not applicable as a user bug. The PR body provides redacted live output and the diff gives a high-confidence patch review path, but I did not run local runtime tests in this read-only review.

Is this the best way to solve the issue?

Unclear pending maintainer product direction. The fail-closed implementation is safer than invoking the blocking controller, and the remaining question is whether core should expose this partial surface now.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes redacted live output from a real Messages.app bridge run showing v2 ping consumption, contact-card status, and fail-closed share behavior; screenshots or logs should still redact chat GUIDs and local identifiers.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This is a bounded advanced bridge feature with live proof and passing CI, but it still needs maintainer acceptance before merge.
  • merge-risk: 🚨 availability: The PR changes the injected Messages helper and v2 inbox polling path, where normal CI cannot fully prove crash, stall, or runtime availability behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes redacted live output from a real Messages.app bridge run showing v2 ping consumption, contact-card status, and fail-closed share behavior; screenshots or logs should still redact chat GUIDs and local identifiers.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted live output from a real Messages.app bridge run showing v2 ping consumption, contact-card status, and fail-closed share behavior; screenshots or logs should still redact chat GUIDs and local identifiers.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its CLI naming, focused changeset, verification, and macOS permission guidance apply to this PR review. (AGENTS.md:1, d9bd9c754d50)
  • Current main lacks this surface: A source search on current main found only existing nickname/contact-card introspection wording, not a contact-card command, contact-card bridge action, or contacts.*ContactCard RPC implementation. (Sources/imsg/Commands/BridgeIntroCommands.swift:277, d9bd9c754d50)
  • PR adds public bridge vocabulary: The PR merge-base diff touches 12 files with 286 insertions and 2 deletions across CLI, RPC, bridge protocol, injected helper, docs, skill guidance, and tests. (ffdcb7eba272)
  • CLI command routing: ContactCardCommand requires --chat, maps status/share to the two new BridgeAction cases, and emits either JSON data or a compact human summary. (Sources/imsg/Commands/ContactCardCommand.swift:5, ffdcb7eba272)
  • Guarded injected-helper behavior: The helper now uses class-level selector introspection for IMNicknameController, marks the private status probe as not invoked, and always fails share with the explicit unavailable error rather than calling the blocking controller path. (Sources/IMsgHelper/IMsgInjected.m:4236, ffdcb7eba272)
  • Runtime availability-sensitive change: The PR adds and retains a dispatch_source_t timer beside the existing NSTimer for v2 inbox polling and cancels it during injected cleanup. (Sources/IMsgHelper/IMsgInjected.m:4695, ffdcb7eba272)

Likely related people:

  • steipete: Blame and commit metadata tie this handle to the current bridge selector-probe baseline, nickname bridge surface, and release baseline that this PR extends. (role: recent area contributor and release-baseline owner; confidence: high; commits: dea78a9e9c49, fed89d6b4f9d; files: Sources/IMsgHelper/IMsgInjected.m, Sources/imsg/Commands/BridgeIntroCommands.swift, Sources/IMsgCore/IMsgBridgeProtocol.swift)
  • omarshahine: Prior merged history ties this handle to the BlueBubbles-inspired IMCore bridge, macOS 26 bridge fixes, and native poll bridge/RPC surfaces that this PR extends. (role: recent bridge contributor; confidence: high; commits: c56c24d488ef, 2d7b506d1736, 617d941f7b1e; files: Sources/IMsgHelper/IMsgInjected.m, Sources/IMsgCore/IMsgBridgeProtocol.swift, Sources/imsg/RPCServer.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.
Review history (6 earlier review cycles)
  • reviewed 2026-07-08T06:45:52.421Z sha 798a1fc :: needs real behavior proof before merge. :: [P1] Invoke status selectors with the correct return type | [P3] Leave release notes to the release process
  • reviewed 2026-07-09T05:11:22.826Z sha f45875d :: found issues before merge. :: [P3] Restore the release-owned changelog header
  • reviewed 2026-07-09T05:16:45.677Z sha f45875d :: found issues before merge. :: [P3] Restore the release-owned changelog header
  • reviewed 2026-07-09T05:35:06.595Z sha f45875d :: found issues before merge. :: [P3] Restore the 0.12.4 changelog header
  • reviewed 2026-07-09T05:41:01.669Z sha f45875d :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T20:30:35.421Z sha f45875d :: found issues before merge. :: [P2] Report selector availability before returning unavailable | [P3] Preserve the release-owned changelog header

@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: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jul 8, 2026
@omarshahine

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 9, 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.

@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: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed 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. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 9, 2026
@omarshahine omarshahine changed the title feat: add contact-card sharing feat: add guarded contact-card probes Jul 9, 2026
@omarshahine

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@omarshahine
omarshahine marked this pull request as ready for review July 9, 2026 20:24
@clawsweeper clawsweeper Bot added 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: 🐚 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. labels Jul 9, 2026
@omarshahine

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 9, 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.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. 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 9, 2026
@clawsweeper clawsweeper Bot added the status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. label Jul 9, 2026
@steipete

Copy link
Copy Markdown
Collaborator

Thanks @omarshahine. I’m closing this surface rather than shipping permanent endpoints that cannot perform or observe their advertised action. At ffdcb7e, status deliberately never invokes sharing state (should_share: null, status_probe_invoked: false), while share always returns unavailable. This probes Messages Name & Photo internals, not a vCard/contact attachment.

The watcher finding is useful, but it should be a separate change. This branch adds a second 100 ms main-thread inbox scan beside the existing one—up to 20 directory scans per second—and still cannot rescue a genuinely blocked main thread. A follow-up should use one watcher, measure idle CPU/I/O and request latency, and include exact-head injected-bridge proof.

@steipete

Copy link
Copy Markdown
Collaborator

@omarshahine — I'm sorry. I closed this too categorically and should have discussed your intent with you first. The branch had implementation problems, but I conflated “this patch should not merge as-is” with “this feature does not belong in imsg.” You were extending the BlueBubbles-compatible Messages Name & Photo surface, and your fail-closed live work exposed real selector and bridge-liveness problems.

I've rebuilt the feature as #173. It fixes the existing nickname path, uses the verified Name & Photo selector family, keeps the watcher finding separate as a measured follow-up, and exposes sharing only as an explicit request. The PR is draft while I finish receiver-visible proof. I preserved your authorship and would especially value your review of the intended contract and sender-handle routing.

Thanks for pushing this forward, and sorry again for shutting down the conversation instead of separating the useful findings from the patch shape.

@omarshahine
omarshahine deleted the feat/contact-card branch July 18, 2026 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. 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.

2 participants