fix: ignore confirmed dead LaunchServices records - #721
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
PR SummaryMedium Risk Overview Native full-BSD process-generation reads now surface Docs and changelog describe the stale-row rule; tests cover a real reaped child PID and uncertain identity transitions. Reviewed by Cursor Bugbot for commit 2b6bc23. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Codex review: needs maintainer review before merge. Reviewed September 12, 2026, 8:16 PM ET / September 13, 2026, 00:16 UTC (Revision 2). ClawSweeper reviewWhat this changesExclude stale macOS application records after two native lookups confirm the process is absent, restoring name and bundle targeting while retaining refusal for uncertain identities. Merge readiness✅ Ready for maintainer review This remains a useful fix: current main and v4.3.4 retain the reported failure mechanism. The scoped native proof supports the change, and no actionable correctness or security defect was found. Priority: P1 Review scores
Verification
How this fits togetherPeekaboo builds an application inventory from macOS discovery records and native process identities. The mutation planner uses that inventory to select an unambiguous application and retain its process generation for subsequent actions. flowchart TD
A[macOS application records] --> B[Native process lookup]
B --> C{Confirmed absent twice?}
C -->|Yes| D[Exclude stale record]
C -->|No| E[Validate identity and metadata]
E --> F[Complete inventory or refusal]
D --> F
F --> G[Exact application selection]
G --> H[Generation checked action]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Technical reviewBest possible solution: Keep confirmed native absence distinct from uncertainty while preserving complete-inventory selection and generation checks before application actions. Do we have a high-confidence way to reproduce the issue? Yes, from source: a listed PID returning ESRCH becomes an omission on current main, and the planner rejects the resulting partial inventory. Native before/after execution is recorded in the PR body, not independently run in this review. Is this the best way to solve the issue? Yes. Separating explicit native absence and requiring two agreeing observations repairs the reported failure without relaxing arbitrary-error handling or adding a competing resolution path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 4338bc31e6cd. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
A reaped process can remain in LaunchServices' application list. The native generation lookup returned
ESRCH, but Peekaboo collapsed that result into unknown metadata. One dead row then made the entire mutation inventory partial and refused otherwise unambiguous application names and bundle IDs.Preserve explicit native absence separately from permission denial and uncertain metadata. Exclude a row only after two consecutive failed full-BSD generation reads return
ESRCH. Partial reads, other errors, readable generations that disappear, and changing/reused identities still make the inventory partial. Existing denied-helper policy, read-only discovery, and generation-pinned explicit-PID targeting remain intact.Fixes #709.
Validation:
swift test --package-path Core/PeekabooAutomationKit --filter 'ApplicationInventoryTimeoutTests|SystemIdentityResolverTests': 27 tests passed after the fix, including native absence, uncertainty transitions, denied-helper identity changes, and timeout/cancellation coverage.ESRCH), andkill(pid, 0)also reportedESRCH.Unreleasedupdated. Hosted checks must pass on this exact head before merge.