Skip to content

fix(review): avoid false persistent data-model findings - #983

Draft
vincentkoc wants to merge 1 commit into
mainfrom
fix/data-model-classifier
Draft

fix(review): avoid false persistent data-model findings#983
vincentkoc wants to merge 1 commit into
mainfrom
fix/data-model-classifier

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member

What Problem This Solves

Fixes an issue where OpenClaw pull requests with runtime repair, telemetry, cache-usage, or test-fixture vocabulary could be reported as persistent data-model changes even when they did not alter stored state. This produced misleading merge risk in reviews such as openclaw/openclaw#115869.

Why This Change Was Made

The detector now keeps persistence matching tied to explicit storage operations, semantic schema language, and production data-model paths while excluding test-only surfaces. It preserves real schema, migration, cache, vector, serialized-state, compound-path, prose, and production snapshot signals.

User Impact

Maintainers get fewer false stored-data warnings without losing review gates for actual persistence or migration changes.

Evidence

  • pnpm run build && node --test test/pr-surface-policy.test.ts: 28/28 passed.
  • Focused regressions cover production snapshot schemas, prose forms such as cache version and embedding dimension, and compound identifiers such as checkpointFilePath.
  • git diff --check: clean.
  • Fresh autoreview: clean, 0 accepted/actionable findings.

Real Behavior Proof

Claim: the detector no longer classifies the Code Mode reliability patch as a stored data-model change while retaining concrete persistence findings.

Exercised surface: dataModelChangeFromPullFilesForTest, using the complete 118-file GitHub pull-file fixture from openclaw/openclaw#115869.

Command and environment: built @openclaw/clawsweeper on Node 26, then executed the compiled detector 50 times over the captured 118-file fixture.

Observed result: { "change": false, "surfaces": [] }; p50 3.77 ms, p90 3.87 ms, max 7.53 ms. The prior published review reported 12 unrelated data-model surfaces.

Artifact or trace: focused test output and benchmark summary are included in the task handoff; CI will rerun repository gates on this exact branch.

Limits: this PR fixes classifier precision only. Complete pull-file materialization for PRs above the compact review limit remains a separate change.

Combined Integration Proof - July 31, 2026

Both exact merge orders were tested from base 16f01b29508632ab3a904087191e39b20968ab68:

  • 983 -> 984: clean cherry-picks; 66/66 focused tests passed.
  • 984 -> 983: clean cherry-picks; 66/66 focused tests passed.
  • Both orders produced identical Git tree 609478921e5b04e2b1844001bfe81ca713f4f099, with 6 files changed, 560 insertions, and 25 deletions.
  • git diff --check 16f01b29508632ab3a904087191e39b20968ab68..HEAD passed in both worktrees.
  • Live GitHub pagination for OpenClaw PR 115869 returned pages of 100 and 18 files, exactly 118 total.
  • Both orders produced the same compiled-detector output: { "config": { "change": false, "keys": [] }, "data": { "change": false, "surfaces": [] } }.
  • Positive persistence guards remained green in both orders, including schema, serialized-state, cache, vector metadata, production snapshot, compound persisted-path, missing-patch, and truncated-input fixtures.
  • Complete-file guards also remained green in both orders: exact count, unique filenames, stable base/head snapshot, fetch failure, and the 3,000-file API ceiling all fail closed.

Commands used direct tsc plus node --test test/context.test.ts test/pr-surface-policy.test.ts test/review-prompt-context.test.ts; no dependency reconciliation occurred in the proof worktrees.

Maintainer Decision - July 31, 2026

Approved: retain the narrower production-path and explicit-signal classifier boundary. The demonstrated Code Mode false positive is not useful merge-risk signal, while the positive schema, migration, serialized-state, cache, vector, compound-path, missing-patch, and truncated-input guards preserve the conservative cases that matter. No broader vocabulary fallback is requested.

@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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jul 31, 2026
@clawsweeper

clawsweeper Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed August 2, 2026, 5:03 PM ET / 21:03 UTC.

ClawSweeper review

What this changes

The branch narrows persistent data-model detection and adds regressions so runtime, telemetry, and test-only vocabulary does not create stored-data merge-risk findings.

Merge readiness

Blocked by patch quality or review findings - 6 items remain

Keep this member-authored PR open, but do not merge it as-is: its classifier edits target the pre-modularization monolith, while current review reports invoke the unchanged classifier in src/clawsweeper-change-detection.ts. The intended false-positive reduction remains useful, but the existing P1 finding must be resolved on the active module after rebasing.

Priority: P2
Reviewed head: 89e434454729baea1d490fbbb3e3e2094c5bb1af

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The behavior proof is detailed, but the submitted patch is not merge-ready because it modifies an inactive implementation after the main-branch modularization.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The PR body provides after-fix compiled-detector output over the complete 118-file fixture and states that positive persistence guards remained green; the proof is credible for the intended branch behavior, though it does not cure the current-main porting defect.
Patch quality 🧂 unranked krab (1/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The PR body provides after-fix compiled-detector output over the complete 118-file fixture and states that positive persistence guards remained green; the proof is credible for the intended branch behavior, though it does not cure the current-main porting defect.
Evidence reviewed 5 items Active classifier remains unchanged: Current main executes dataModelChangeFromContext from the focused change-detection module. It still scans every candidate path and retains the broad runtime terms (repair, doctor, JSON.parse, readFile, cache, metadata) that this PR intends to narrow.
Live report path uses the focused module: The report renderer imports and calls dataModelChangeFromContext from src/clawsweeper-change-detection.ts, so modifying only the historical monolith cannot change generated review findings on current main.
Refactor provenance: Commit 8fd1140e7aa97790513afaab43eba4dd1085f85a modularized ClawSweeper after this branch's base, and dccfa528f0387431807818ba43823310b5e08f51 continued the entrypoint decomposition; the active classifier is therefore outside the branch's edited file.
Findings 1 actionable finding [P1] Port the classifier changes to the active module
Security None None.

How this fits together

ClawSweeper reads pull-request file paths and patches to classify configuration and persistent-data changes before rendering a durable review report and merge-risk labels. The data-model classifier feeds the report renderer, which turns detected surfaces into maintainer-facing review guidance.

flowchart LR
  A[GitHub pull-file metadata] --> B[Pull-request context]
  B --> C[Data-model classifier]
  C --> D[Detected persistence surfaces]
  D --> E[Review report renderer]
  E --> F[Merge-risk guidance]
  G[Focused regression fixtures] --> C
Loading

Before merge

  • Port the classifier changes to the active module (P1) - Current main renders data-model findings through src/clawsweeper-change-detection.ts, but this hunk edits the pre-refactor src/clawsweeper.ts copy. Resolving the dirty branch without moving these rules and regressions leaves the live classifier unchanged, so the reported false positives persist.
  • Resolve merge risk (P1) - The branch is dirty against current main; a conflict resolution that preserves only the old src/clawsweeper.ts edits would land no change to the live classifier and leave false persistent-data findings unchanged.
  • Resolve merge risk (P1) - This changes review automation’s merge-risk classification, so the port needs focused positive and false-positive regression coverage on the active module before merge.
  • Complete next step (P2) - Do not merge the dirty draft; the PR owner needs to rebase and port the already-identified P1 repair to the current classifier module, then refresh proof and review.
  • Improve patch quality - Rebase and port the classifier rules and regressions to src/clawsweeper-change-detection.ts.
  • Improve patch quality - Rerun the focused live-module proof and obtain a fresh review for the rebased head.

Findings

  • [P1] Port the classifier changes to the active module — src/clawsweeper.ts:13768-13787
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 2 files changed; 160 additions, 15 deletions Both changed files are pre-refactor surfaces, while current main routes the live classifier through a separate module.

Merge-risk options

Maintainer options:

  1. Port the fix onto the active classifier (recommended)
    Rebase onto current main, move the semantic and test-path changes into src/clawsweeper-change-detection.ts, and rerun focused classifier proof before maintainer review.
  2. Pause the obsolete branch
    If the active-module port is not pursued, close this draft and reopen a narrow replacement PR against current main when the false positive still needs correction.

Technical review

Best possible solution:

Rebase the PR and apply the approved narrow production-path and explicit-persistence-signal boundary to src/clawsweeper-change-detection.ts, with regressions that exercise the live exported test helper and preserve conservative unknown/truncated cases.

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

Yes—source-reproducible with high confidence: current main’s report renderer calls the unchanged classifier in src/clawsweeper-change-detection.ts, while this branch edits only the retired monolith.

Is this the best way to solve the issue?

No. The approved narrower boundary is a reasonable solution, but this branch must move it and its regressions to the active classifier module before it can solve the live behavior.

Full review comments:

  • [P1] Port the classifier changes to the active module — src/clawsweeper.ts:13768-13787
    Current main renders data-model findings through src/clawsweeper-change-detection.ts, but this hunk edits the pre-refactor src/clawsweeper.ts copy. Resolving the dirty branch without moving these rules and regressions leaves the live classifier unchanged, so the reported false positives persist.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix compiled-detector output over the complete 118-file fixture and states that positive persistence guards remained green; the proof is credible for the intended branch behavior, though it does not cure the current-main porting defect.

Label justifications:

  • P2: The PR addresses misleading review automation with a bounded maintainer impact, but the current branch does not update the live path.
  • merge-risk: 🚨 automation: The change affects deterministic review classification and can leave incorrect merge-risk reports in place if it is resolved without porting to the active module.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🐚 platinum hermit and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body provides after-fix compiled-detector output over the complete 118-file fixture and states that positive persistence guards remained green; the proof is credible for the intended branch behavior, though it does not cure the current-main porting defect.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix compiled-detector output over the complete 118-file fixture and states that positive persistence guards remained green; the proof is credible for the intended branch behavior, though it does not cure the current-main porting defect.

Evidence

What I checked:

Likely related people:

  • Peter Steinberger: Peter authored the commits that moved the classifier into the focused module currently used by report rendering, making him the strongest current-main history contact for the required port. (role: modularization author and recent area contributor; confidence: high; commits: 8fd1140e7aa9, dccfa528f038; files: src/clawsweeper-change-detection.ts, src/clawsweeper-report-rendering.ts)

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 (17 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-01T17:32:50.113Z sha 89e4344 :: found issues before merge. :: [P1] Port the classifier changes to the active module
  • reviewed 2026-08-01T21:23:04.329Z sha 89e4344 :: found issues before merge. :: [P1] Port the classifier changes to the active module
  • reviewed 2026-08-01T22:37:41.519Z sha 89e4344 :: found issues before merge. :: [P1] Port the classifier changes to the active module
  • reviewed 2026-08-02T07:46:30.752Z sha 89e4344 :: needs changes before merge. :: [P1] Port the classifier changes to the active module
  • reviewed 2026-08-02T08:54:37.725Z sha 89e4344 :: found issues before merge. :: [P1] Port the classifier changes to the active module
  • reviewed 2026-08-02T12:47:28.070Z sha 89e4344 :: found issues before merge. :: [P1] Port the classifier changes to the active module
  • reviewed 2026-08-02T15:25:51.850Z sha 89e4344 :: needs changes before merge. :: [P1] Port the classifier changes to the active module
  • reviewed 2026-08-02T17:35:07.231Z sha 89e4344 :: found issues before merge. :: [P1] Port the classifier changes to the active module

@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
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:

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 31, 2026
@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. labels Jul 31, 2026
@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

1 similar comment
@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@vincentkoc

Copy link
Copy Markdown
Member Author

Maintainer decision for head 89e4344: accept the narrower persistent-data classifier boundary. Keep the explicit production-path and persistence-signal positives and the added false-positive regressions; ordinary runtime/test vocabulary must not create a persistent-data merge gate.

@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed 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. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 1, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. proof: sufficient Contributor real behavior proof is sufficient. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant