fix: validate remote observations before publishing files - #725
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 Ordinary captures with ROI behavior is unchanged in spirit: deferred snapshot commit, staged rollback, and ROI-specific validation still apply; shared preparation is generalized ( Adds serialized integration tests for publication ownership, changelog/docs notes, and trims a misleading long doc comment on Reviewed by Cursor Bugbot for commit e1c642d. Bugbot is set up for automated code reviews on this repo. Configure here. |
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. Reviewed September 12, 2026, 9:04 PM ET / September 13, 2026, 01:04 UTC (Revision 2). ClawSweeper reviewWhat this changesStages ordinary remote screenshots privately before validating and publishing them, preserves raw destinations for annotation-only and snapshot-only requests, and updates tests and documentation. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 3 items remain The publication fix remains necessary, but the previously reported Accessibility recovery regression is still present at the updated head. The contribution is useful and should remain open. Priority: P2 Review scores
Verification
How this fits togetherPeekaboo’s remote observation service sends capture requests to a local Bridge host and validates the returned screenshots and Accessibility evidence. It then exposes image files and snapshot references to CLI and API callers. flowchart TD
A[CLI or API capture request] --> B[Remote observation service]
B --> C[Bridge host capture]
C --> D[Private screenshot staging]
D --> E[Validate response and raster]
E --> F[Publish requested files]
C --> G[Host error]
G --> H[Delete staged files]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Preserve validated diagnostic rasters across host-thrown Accessibility failures while keeping rejected identity/content evidence private and unusable snapshots unpublished. Do we have a high-confidence way to reproduce the issue? Yes, from source: a combined observation with empty Accessibility evidence writes its raster and throws on the host; the patched remote client then deletes that staged raster. No runtime tests were executed during this read-only review. Is this the best way to solve the issue? Not yet: private staging is appropriate, but recovery must carry validated diagnostic evidence through the host error path before the implementation preserves the existing contract. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 01b8b3118043. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 703efd8. Configure here.
1 participant
Remote observations could overwrite a caller's screenshot before the client rejected inconsistent response evidence. Ordinary captures now use the same private staging boundary as ROI captures: validate identity, raster, and digest before publishing caller files. Annotation-only and snapshot-only requests retain verified image data without modifying the raw destination. Host-owned snapshot behavior and ROI rollback semantics remain intact.
Addresses the remaining premature-file-publication problem reported in #710. The separate screen/Accessibility attribution fix landed in #722. Removes a stale description above the actual AX delegation while touching that path.
Validation: 30 focused remote capture ownership tests passed, including new rejected-evidence tests that failed against the old implementation. Isolated Codex review found no actionable P0–P2 findings; lint, format, and docs checks passed. The shared-process Bridge fixtures are serialized to prevent competing short-deadline fixture hosts; their existing deadlines and assertions remain unchanged.
Native proof used a signed external consumer of
RemoteDesktopObservationServiceand two signed Bridge hosts, negotiating protocol 1.38. The old host reproduced the actual unexpected-application-evidence refusal. With the patched consumer, a rejected new destination remained absent and a rejected existing 21-byte sentinel remained unchanged. A valid host observation succeeded with 12 AX elements and a verified 1,617,793-byte raster. Images were removed after validation; only structural results were retained. This protects high-level remote caller destinations; it does not change the low-level Bridge API's publication contract.