Skip to content

fix: generic criterion matching returned nil for CF-typed attributes#19

Merged
steipete merged 3 commits into
openclaw:mainfrom
dalsoop:fix/generic-criterion-matching
Jul 18, 2026
Merged

fix: generic criterion matching returned nil for CF-typed attributes#19
steipete merged 3 commits into
openclaw:mainfrom
dalsoop:fix/generic-criterion-matching

Conversation

@dalsoop

@dalsoop dalsoop commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Problem

Generic criteria fetched values through Attribute<Any>, whose typed conversion could return nil for real Core Foundation-backed accessibility values. A window with a readable AXTitle therefore failed an exact AXRole + AXTitle lookup.

Fix

Read the raw AX attribute and normalize it through AXorcist's existing ValueUnwrapper, retaining the typed accessor as a fallback. This preserves string, boolean, number, and AXValue semantics while fixing generic matching. Add focused automation regression coverage and an Unreleased changelog entry thanking @dalsoop.

The contributor branch is refreshed onto current main with a regular merge, preserving both contributor commits and credit without rewriting branch history.

Live behavior proof

The refreshed head was built locally, then exercised against a newly built AppKit fixture signed with the repository maintainer's Developer ID. The fixture was live at bundle identifier org.openclaw.AXorcistProbe with a real window titled AXorcist Probe.

$ codesign --verify --deep --strict --verbose=2 AXorcistProbe.app
AXorcistProbe.app: valid on disk
AXorcistProbe.app: satisfies its Designated Requirement

$ axorc find --app org.openclaw.AXorcistProbe --role AXWindow --title "AXorcist Probe"
Role: AXWindow, Title: 'AXorcist Probe'
exit=0

$ axorc find --app org.openclaw.AXorcistProbe --role AXWindow --title "Wrong Probe Title"
Error: FTE: Not found C=[AXRole:AXWindow, AXTitle:Wrong Probe Title] from application root Role: AXApplication, Title: 'AXorcistProbe', Max depth visited = 2 of 10, Nodes visited = 7
exit=1

This negative control changed only the requested title while keeping the live app and role fixed.

Verification

  • swift build
  • swift test --filter ElementSearchTests (the automation suite compiled and was correctly skipped without the automation opt-in)
  • Source-blind behavior contract: exact-title lookup passed; wrong-title control passed; no blockers
  • Structured autoreview on the refreshed diff: no actionable findings
  • Exact candidate head: e5863f35e3326f0c1e284fe3cb19148cf4e5d176
  • Exact-head CI: run 29660139857 passed build, safe tests, lint, and release packaging

performGenericAttributeMatch fetched the actual value via
element.attribute(Attribute<Any>(key)), which returns nil for CF-typed
values. Every generic criterion (AXTitle, AXValue, ...) therefore never
matched: a locator like {AXRole: AXWindow, AXTitle: <exact title>}
failed even when the window title matched exactly.

Read the raw attribute value first and fall back to the generic path.
@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. labels Jul 18, 2026
@clawsweeper

clawsweeper Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 18, 2026, 4:41 PM ET / 20:41 UTC.

Summary
The branch normalizes raw accessibility attribute values before generic criterion matching, adds an exact AXTitle automation regression test, and documents the fix in the unreleased changelog.

Reproducibility: yes. with medium confidence from source and supplied live evidence: the old generic accessor could yield no value for Core Foundation-backed attributes, while the PR body shows a real exact-title lookup and changed-title negative control. The reviewer environment could not independently execute the macOS accessibility fixture.

Review metrics: 1 noteworthy metric.

  • Patch scope: 3 files affected; 30 additions, 1 deletion. The implementation is narrowly scoped to the matcher, one automation regression, and release-note context.

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:

  • Wait for the in-progress build-and-lint check on e5863f35e3326f0c1e284fe3cb19148cf4e5d176 to finish successfully.

Risk before merge

  • [P1] The exact-head build-and-lint check is still in progress, so merge should wait for that normal validation gate to complete successfully.

Maintainer options:

  1. Decide the mitigation before merge
    Retain the narrow raw-value normalization and regression test, then merge after the exact-head build check confirms the refreshed branch remains healthy.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] No concrete automated repair is needed; the remaining action is ordinary human review plus completion of the exact-head build check.

Security
Cleared: The three-file Swift test/source/changelog diff introduces no credential, dependency, workflow, permission, artifact-execution, or supply-chain change.

Review details

Best possible solution:

Retain the narrow raw-value normalization and regression test, then merge after the exact-head build check confirms the refreshed branch remains healthy.

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

Yes, with medium confidence from source and supplied live evidence: the old generic accessor could yield no value for Core Foundation-backed attributes, while the PR body shows a real exact-title lookup and changed-title negative control. The reviewer environment could not independently execute the macOS accessibility fixture.

Is this the best way to solve the issue?

Yes. Reusing the established ValueUnwrapper at the generic-matching boundary, while retaining the existing typed-accessor fallback, is narrower and less duplicative than adding special cases for individual Accessibility attributes.

AGENTS.md: found, but no applicable review policy affected this item.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides inspectable terminal output from the exact candidate head, including a successful real AppKit fixture lookup and a changed-title negative control; redact any private identifiers if this evidence is refreshed.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit. Replaced prior rating: 🦪 silver shellfish.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: This is a concrete generic accessibility-search bug with limited blast radius and a focused proposed repair.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit. Replaced prior rating: 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Sufficient (terminal): The PR body provides inspectable terminal output from the exact candidate head, including a successful real AppKit fixture lookup and a changed-title negative control; redact any private identifiers if this evidence is refreshed.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides inspectable terminal output from the exact candidate head, including a successful real AppKit fixture lookup and a changed-title negative control; redact any private identifiers if this evidence is refreshed.
Evidence reviewed

What I checked:

  • Raw-value normalization: The changed generic matcher unwraps element.rawAttributeValue(named: key) through the existing ValueUnwrapper, then falls back to the previous typed accessor, directly addressing Core Foundation-backed values that the generic accessor can fail to convert. (Sources/AXorcist/Search/SingleCriterionMatching.swift:199, 1517bf5756cb)
  • Focused regression coverage: The added automation test queries a live TextEdit window with both AXRole and its observed AXTitle, then verifies the returned title matches the locator criterion. (Tests/AXorcistTests/ElementSearchTests.swift:11, 2a66df009dfc)
  • After-fix behavior proof: The PR body includes copied terminal output from the exact candidate head showing a real AppKit fixture succeeds for its exact window title and fails for a changed-title negative control while retaining the same application and role. (e5863f35e332)
  • Current review state: The branch is reported mergeable against current main; its only reported check, build-and-lint, remains in progress for the exact head. (e5863f35e332)

Likely related people:

  • steipete: Authored the follow-up commit that added the regression coverage and user-facing documentation for this matcher path, making them the strongest available routing contact in the provided history. (role: recent area contributor; confidence: medium; commits: 2a66df009dfc, e5863f35e332; files: Sources/AXorcist/Search/SingleCriterionMatching.swift, Tests/AXorcistTests/ElementSearchTests.swift, CHANGELOG.md)
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 (3 earlier review cycles)
  • reviewed 2026-07-18T06:42:16.124Z sha 1517bf5 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T20:18:00.765Z sha 2a66df0 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T20:29:49.101Z sha 2a66df0 :: needs real behavior proof before merge. :: none

Add regression coverage for exact AXTitle matching and document the user-visible fix.

Co-authored-by: jeonghan.yun <devops@ranode.net>
@steipete

Copy link
Copy Markdown
Collaborator

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 18, 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 rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 18, 2026
@steipete

Copy link
Copy Markdown
Collaborator

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 18, 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:

@steipete
steipete merged commit bdfc723 into openclaw:main Jul 18, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants