Skip to content

fix(review): keep runtime vocabulary out of persistent-data findings - #1081

Open
masatohoshino wants to merge 1 commit into
openclaw:mainfrom
masatohoshino:fix/persistent-data-classifier-recut
Open

fix(review): keep runtime vocabulary out of persistent-data findings#1081
masatohoshino wants to merge 1 commit into
openclaw:mainfrom
masatohoshino:fix/persistent-data-classifier-recut

Conversation

@masatohoshino

@masatohoshino masatohoshino commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Related: #983

What Problem This Solves

Fixes an issue where OpenClaw pull requests that change no stored state are
publicly told to produce migration proof before merge.

dataModelSurfacesFromPatch joins every changed non-comment line into one blob
and runs its content regexes over that blob without reference to the file
path
. Ordinary runtime, test, fixture, telemetry, docs, and GitHub Actions
vocabulary — metadata, cache, upgrade, JSON.parse, vector, dimension,
repair — is therefore enough to raise a persistent data-model surface.

A false hit is not cosmetic. It sets data_model_change, and
dataModelSurfaceReviewRequired then costs the PR pass eligibility
(reviewAutomationMarkersFromReportverdict:needs-human), repair-loop and
automerge eligibility
(isRepairLoopPassReport), and publishes "Confirm
migration or upgrade compatibility proof before merge."
The only escape hatch,
hasDataModelUpgradeProof, asks the author to write migration proof for a
migration that does not exist.

Live carrier: openclaw/openclaw#119762. Its ClawSweeper
comment openclaw/openclaw#119762 (comment),
at carrier head 0d5398a9, reports a persistent data-model change on two
surfaces — the reusable live-and-e2e checks workflow, and a package-acceptance
test file. Both are files in openclaw/openclaw, both labelled vector/embedding metadata, and the trigger in the workflow is a bash variable named
metadata
.

Why This Change Was Made

This recuts the accepted persistent-data classifier boundary onto the active
post-#993 change-detection module.

#983 fixed this exact root cause and
carries a recorded maintainer decision: keep the narrower persistent-data
classifier boundary; ordinary runtime/test vocabulary must not create a
persistent-data merge gate.
It was closed during queue cleanup, not rejected
"nothing against the fix itself … reopen or recut against current main and it
will get a fast review."

It went stale for a mechanical reason:
#993 (9ab7ed402) moved the detector
out of src/clawsweeper.ts into src/clawsweeper-change-detection.ts, so #983's
production hunk no longer applied. Its regressions still target
test/pr-surface-policy.test.ts, which is unchanged, so they port directly.

This PR changes dataModelChangeFromContext and dataModelSurfacesFromPatch,
porting #983's approved semantics:

  • exclude test, fixture, and snapshot paths from the data-model candidate set;
  • narrow the migration content regex to migration and backfill words, so bare
    upgrade, doctor, repair, reindex, rehydrate in changed text no longer
    trigger on their own (the doctor/ and migrations/ path hints are
    untouched, so src/doctor/repair.ts still signals);
  • replace the bare JSON.parse|readFile|serialized|persisted serialized-state
    regex with explicit storage APIs or a proximity window (±2 changed lines
    around each changed line, so a 5-line span) requiring file I/O and JSON
    and a persistence noun;
  • narrow the cache regex to cache (key|version|schema|namespace|ttl);
  • narrow the vector regex to compound persisted identifiers;
  • anchor the migration filename branch of dataModelPathHint so a name merely
    ending in -repair.ts no longer reads as repair.ts; the basename must now be
    exactly one of migration|backfill|doctor|repair|upgrade + .ts/.js, and the
    directory branch of the same hint is unchanged.

Boundaries. unknown-data-model-change, unknown-truncated-pull-files,
truncation detection, and pullFilesTruncated fail-closed behavior are
untouched. Within dataModelPathHint only the migration filename branch is
anchored; every directory branch and the two-factor corroboration in
dataModelTextMatchesPathHint / dataModelTextLooksLikePersistedShapeField are
unchanged, so a strong path hint still establishes a persistent-data surface
outside the normal source tree. No new vocabulary, no
schema change, no consumer change: report front matter, orchestration readers,
renderer, and automation markers all consume {change, surfaces} unchanged.

Rejected alternative, measured not assumed. Gating the content branch behind
a ^(src|ui|packages|extensions)/ production-path predicate is the smaller
change, so it was implemented first and run against the same matrix. It loses
2 approved positives
migrations/0001-init.sql (persistence outside the
normal source tree) and #983's retained docs/storage.md prose — and leaves
6 negatives still signalling, every production-source case. Reproduce by
wrapping the compiled detector so content-derived surfaces are dropped on paths
failing that predicate, keeping unknown-* markers, and re-running the matrix
below.

isOpenClawSourcePath stays private in src/pr-surface-stats.ts: the accepted
shape needs a test-path predicate, not a production-source allowlist, and that
module's isOpenClawTestPath under-excludes here (misses nested test/
segments, fixtures/, __snapshots__/, .snap).

Known waived trade-offs

Two review findings were raised against this branch. Both were waived as
properties of the boundary #983 established rather than defects introduced here,
and both residual costs are stated so they are trades, not hidden gaps.

  1. One-sided serialized-state detection. When a hunk changes only the
    JSON.parse line and the readFile call stays in unchanged context,
    changedPatchLines never sees the I/O line, so no surface is raised. This
    cannot be separated from the required negative "API-response JSON.parse" —
    at the changed-lines level they are the same input, and fix(review): avoid false persistent data-model findings #983 chose the
    negative. The two-factor pathHint route still covers persistence-located
    files. Residuals: a storage-path file whose only changed line carries no
    corroborating token — src/storage/snapshot-loader.ts with
    return JSON.parse(raw) as SessionStateV3; — goes clean; and so does
    on-disk configuration persistence such as src/config/io.write.ts writing
    JSON.stringify(config) to configPath, because config is deliberately
    absent from the persistence-noun list. That second one is not an oversight:
    fix(review): avoid false persistent data-model findings #983 pinned it with a test this PR ports verbatim, which lists
    scripts/config-fixture.ts doing exactly that write and asserts it raises no
    surface.
  2. upgrade, doctor, repair, reindex, rehydrate dropped from the
    migration content regex.
    fix(review): avoid false persistent data-model findings #983 removed them; its PR body enumerates the
    preserved signal set without them. Residuals: a file inside migrations/
    whose only changed lines are reindex(...) / rehydrateState(...) goes
    clean, because the pathHint corroboration list does not carry those words
    either; and a generic production file such as src/runtime/compat.ts whose
    only changed line is await upgrade(state); goes clean, since neither its
    path nor its text now matches. The doctor/, migrations/, backfill/,
    repair/ and upgrade/ path hints are untouched, so
    src/doctor/repair.ts still signals.

Restoring either means re-widening the content regex (re-opening the class this
PR closes) or widening pathHint beyond #983. Happy to add either if you want
the boundary moved
— I kept this PR to the shape you already approved.

Two further deltas, same origin, completing the list of four: bare serialized /
persisted prose no longer signals (#983 removed those words; its retained prose positives are cache version and embedding dimension), and the migration filename branch now
requires the basename to be exactly one of migration|backfill|doctor|repair| upgrade + .ts/.js (its directory branch still matches migrations/,
backfill/, doctor/, repair/, upgrade/).

User Impact

Maintainers stop seeing migration-proof demands on PRs that touch only
workflows, tests, fixtures, docs, telemetry, or ordinary runtime code, and those
PRs regain pass plus automerge and repair-loop eligibility. Persistence
changes still gate across every case in the matrix below — schema, migrations,
durable storage, serialized state, cache versioning, vector metadata — except
for the four semantic deltas named above, which are #983's approved narrowing
rather than a change this PR introduces.

Evidence

Tests added: 7 regressions in test/pr-surface-policy.test.ts — two negative
suites (workflow, test and fixture vocabulary; runtime repair, telemetry and
cache vocabulary) and five positive guards — four ported from #983, plus one new
case for persistence outside the normal source tree. node --test test/pr-surface-policy.test.ts
30 subtests pass, 0 fail (23 pre-existing, unchanged).

Teeth check. Reverting only the production hunk and rebuilding fails 4 of the
7 added tests. Reverting only the candidate-path filter fails 1. The other 3
are positive-preservation guards: they pass before and after by design, and fail
if the narrowing overshoots.

Detector matrix, compiled dataModelChangeFromPullFilesForTest, before =
upstream/main 81c23bede7, after = this branch:

before after
negatives clean 1/13 13/13
positives signalling 15/15 15/15
positives with the exact expected surfaces 13/15 15/15
lost positives 0

The 2 "before" positives that were not exact are the two false extras #983
removed: serialized state: scripts/config-fixture.ts and serialized state: docs/storage.md.

Negatives, each a single changed file: workflow bash variable metadata;
workflow vector-tests job name; test metadata assertion; fixture
"dimension": 24; test/fixtures/vector-schema.ts with embeddingDimension;
telemetry metadata parameter; req.headers['x-request-metadata']; in-memory
new Map() cache; req.headers.upgrade !== "websocket"; API-response
JSON.parse; docs-only upgrade; a colocated *.test.ts with JSON.parse;
plus #983's own five-file false-positive set.

Positives, with the surfaces they must keep: ALTER TABLE → database schema;
src/db/migrations/*.ts → database schema; writeFileSync(statePath, JSON.stringify(...)) → serialized state; state.storage.put → durable storage
schema; dimension/collection under src/memory/ → vector/embedding
metadata; cacheVersion under src/cache/ → persistent cache schema;
migrations/0001-init.sql at repo root → database schema; extensions/…
serialized state; #983's four semantic positives; and the fail-closed set
(missing patch, truncated patch, pullFilesTruncated) → the unknown-* markers.

What is re-runnable from this diff, and what is not. The added regressions in
test/pr-surface-policy.test.ts encode a representative subset of the matrix —
each is a pullFiles entry with its patch and its expected surfaces — so
node --test test/pr-surface-policy.test.ts re-runs that subset against your own
build. The rest of the matrix is not in the diff: the runtime
header-metadata negative, several single-file positives such as
src/state/session-store.ts, #983's full five-file negative set, and the
rejected production-path alternative all live in a scratch harness under
.artifacts/, which AGENTS.md keeps out of the tree. Every one of them is a
single call you can paste:

const m = await import("./dist/clawsweeper-change-detection.js");
// negative: a bash variable named `metadata` in a workflow
m.dataModelChangeFromPullFilesForTest({
  repo: "openclaw/openclaw",
  pullFiles: [{
    filename: ".github/workflows/release.yml",
    patch: '@@\n+metadata=".artifacts/pkg/candidate.json"\n+if [[ ! -f "$metadata" ]]; then',
  }],
});
// before: { change: true, surfaces: ["vector/embedding metadata: .github/workflows/release.yml"] }
// after:  { change: false, surfaces: [] }

// positive: a persisted JSON state write still gates
m.dataModelChangeFromPullFilesForTest({
  repo: "openclaw/openclaw",
  pullFiles: [{
    filename: "src/state/session-store.ts",
    patch: "@@\n+  writeFileSync(statePath, JSON.stringify({ version: 3, turns }));",
  }],
});
// before and after: { change: true, surfaces: ["serialized state: src/state/session-store.ts"] }

The carrier A/B is not reproducible from this diff alone — it needs the live
payload and a build of the base as well as of this branch:

gh api "repos/openclaw/openclaw/pulls/119762/files?per_page=100" --paginate > files.json
git worktree add ../base 81c23bede7 && (cd ../base && pnpm install && pnpm run build)
pnpm run build
node --input-type=module -e '
  import { readFileSync } from "node:fs";
  const files = JSON.parse(readFileSync("files.json", "utf8"));
  const arms = [["before", "../base/dist"], ["after", "./dist"]];
  for (const arm of arms) {
    const m = await import(arm[1] + "/clawsweeper-change-detection.js");
    console.log(arm[0], JSON.stringify(
      m.dataModelChangeFromPullFilesForTest({ repo: "openclaw/openclaw", pullFiles: files })));
  }'

Feeding each result into reviewAutomationMarkersFromReport as data_model_change
and data_model_surfaces reproduces the marker flip shown below.

pnpm run check exits non-zero. Static checks, format:check, all three
build:* projects and all four lint:* lanes pass; 13 tests fail in
test/repair/* (git plumbing, dependency-setup process reaping, and a failure to
create a coverage profile directory under the system temp dir). Those 13 fail
identically on a pristine upstream/main worktree in the same sandbox —
baseline 3209 tests / 13 fail, this branch 3216 tests / 13 fail, same test names
— so none is attributable to this change. No pr-surface-policy or
change-detection test fails. Upstream CI for this exact head is green,
including its own pnpm check run, which independently confirms those 13
failures belong to the local sandbox rather than to the patch. CodeQL for this
exact head is green as well.

git diff --check: clean.

Real Behavior Proof — Docker-backed Crabbox, current head

The repository-required production-path validation. No code changed for this
follow-up
— the reviewed head is unchanged at e23ba84401 and the branch
worktree has no modifications. upstream/main has advanced since this branch
was cut, but every commit since touches neither file in this PR — the blobs
for both are identical between the PR parent and current main — so the branch was
deliberately not rebased. (Inside the lease the checkout does carry untracked build output from
the two builds — the transcript reports 1 then 3 such paths — but the two tracked
files are checked back to head before the run ends and no commit was made.)

Provenance note: the two retained files below evidence everything inside the
lease. Two facts are host-side and independently checkable rather than part of
the artifact: that current upstream/main touches neither PR file, and that the
branch worktree has no modifications.

Claim. At the exact PR head, the compiled production classifier clears the
real false-positive carrier, while six sampled persistence positives — one per
surface the classifier can emit — and all three fail-closed cases still gate.
(The broader 15-positive boundary is covered by the detector matrix above and by
the committed regressions, not by this sample.)

Exercised surface. Compiled dataModelChangeFromPullFilesForTest from
dist/clawsweeper-change-detection.js, feeding the real
reviewAutomationMarkersFromReport and renderReviewCommentFromReport from
dist/clawsweeper-runtime.js. The classifier is not copied or reimplemented —
the harness imports the built artifact.

Environment. Every value below is traceable to one of two retained files:
the in-container transcript crabbox-run.log, or the host-side CLI capture
crabbox-env.txt (which holds crabbox version, crabbox doctor, the verbatim
warmup and stop output, and the artifact hash).

Field Value
Backend Crabbox local-container (Docker-backed)
Crabbox 0.40.0
Image ubuntu:26.04
Lease cbx_252483d2f160
Container hostname crabbox-cs1081proof-138c0406 (docker id b7de0b7f90a1 at provisioning)
Workdir /work/crabbox/cbx_252483d2f160/clawsweeper
User uid=1001(crabbox) — non-root; docker_socket=false
Docker engine 29.4.2
Kernel Linux 6.8.0-136-generic x86_64
Resources 8 vCPU, 46.9 GiB RAM, 381 GiB free
Node v24.19.0
pnpm 11.10.0
tsc 7.0.2
Git head in container e23ba84401cbfe7827badc9ae72e24f17916bb1d. The transcript logs git status : 1 modified paths at run start and restored to head : 3 modified paths after the base rebuild; the two tracked files are checked back to head before the run ends, and no commit is made.
Base observed 81c23bede7a805351bcbb1d5fde54ff278337535 (PR parent)
Lease expiry at provisioning 2026-08-09T06:01:19Z (30m idle timeout)
Proof run 2026-08-09T05:37:03Z → 05:37:09Z, from the transcript (toolchain and deps provisioned earlier in the same lease)

Commands.

crabbox doctor  --provider local-container   # retained capture is post-release,
                                             # showing leases=0
crabbox warmup  --provider local-container --slug cs1081proof
crabbox run     --provider local-container --id cs1081proof --no-hydrate \
                --artifact-glob '.artifacts/crabbox-proof/*' -- '<proof script>'
crabbox stop    --provider local-container --target linux --id cs1081proof

Inside the lease: pnpm install --frozen-lockfilepnpm run build → fetch the
live carrier payload from the GitHub pull-files API → run the harness against the
built dist.

Build attribution (teeth). The dist under test is built in-container from
this head, not synced or stale:

dist mtime   2026-08-09 05:37:04Z   sha256 74ef3b45b97cd891…  (head build)
base dist                            sha256 d2be75cc2aca1899…  (base build)
isOpenClawDataModelTestPath   head 2   base 0     # symbol added by this PR
hasJsonFilePersistence        head 2   base 0     # symbol added by this PR
reindex (old content regex)   head 0   base 1     # token removed by this PR

The base arm is produced in the same container by checking the two files back to
the parent commit, rebuilding, and restoring — so both arms come from the same
toolchain and differ only by this PR's diff.

1. Real false-positive carrier — openclaw/openclaw#119762

Payload fetched live inside the container (state open, head
0d5398a9233014776b06d26252162cf5664535ca, 7 files, sha256 f056dd85a8ceca09…).

BEFORE (base 81c23bede7)          AFTER (head e23ba84401)
  data_model_change : true          data_model_change : false
  surfaces          : 5             surfaces          : 0
    migration/backfill/repair: docs/reference/full-release-validation.md
    persistent cache schema:   docs/reference/full-release-validation.md
    serialized state:          test/scripts/package-acceptance-workflow.test.ts
    vector/embedding metadata: .github/workflows/openclaw-live-and-e2e-checks-reusable.yml
    vector/embedding metadata: test/scripts/package-acceptance-workflow.test.ts
  downstream        : verdict:needs-human   downstream : verdict:pass
  stored-data section rendered : true       rendered   : false
  public warning    : "Persistent data-model change detected: …
                       Confirm migration or upgrade compatibility
                       proof before merge."                 : (none)

Workflow and test vocabulary no longer create persistent-data surfaces;
data_model_change=false; no migration-proof gate from this detector; the
detector-only downstream verdict becomes pass.

2. Retained true positives — 6/6 still gate

Each case goes through the compiled detector and then the real automation path
with no upgrade proof recorded. All are single changed files except the
migration case, which is two:

Case Surface produced Downstream
SQL/DDL ALTER TABLE … ADD COLUMN database schema: src/db/schema.ts verdict:needs-human + warning
migration path + backfill…(db) migration/backfill/repair: on both the migrations/*.sql and src/doctor/backfill.ts verdict:needs-human + warning
serialized durable state writeFileSync(statePath, JSON.stringify(…)) serialized state: src/state/session-store.ts verdict:needs-human + warning
durable object state.storage.put durable storage schema: src/gateway/worker/do.ts verdict:needs-human + warning
persistent cache cacheVersion persistent cache schema: src/cache/keys.ts verdict:needs-human + warning
persisted embeddingDimension / document_id vector/embedding metadata: src/memory/vector-store.ts verdict:needs-human + warning

6/6 in the AFTER arm and 6/6 in the BEFORE arm — identical, so this PR loses
no positive.

3. Fail-closed — 3/3 still gate

Case Marker retained Downstream
missing patch on a likely persistent path unknown-data-model-change: src/storage/session-state.ts verdict:needs-human
truncated patch unknown-data-model-change: packages/database/schema.ts (plus database schema) verdict:needs-human
pullFilesTruncated unknown-truncated-pull-files verdict:needs-human

3/3 in both arms. No unknown-* case fails open.

Focused regressions at the same head, inside the same container: node --test test/pr-surface-policy.test.ts → 30 subtests pass, 0 fail.

Teeth. Both arms run the same harness over the same inputs in the same
container; the only variable is which compiled artifact is loaded — the head
build or the parent build. Across that single variable, positives (6/6) and
fail-closed (3/3) are identical, and the carrier is the only observation
that moves. So the proof isolates exactly the behavior this PR changes rather
than exercising unrelated paths.

Artifact. cbx_252483d2f160-artifacts.tgz, sha256
d8a19f20cca10db368e9d99b3968a60cb936e34e05b6bdb36fc7776413c35134 (21,879
bytes), collected via --artifact-glob to
.crabbox/runs/cbx_252483d2f160/. Contents and their hashes:

e685eacf8b7667e8…  after.json          structured AFTER observations
a70feaa1c7cae6ba…  before.json         structured BEFORE observations
dab8e936553486374… harness.mjs         the harness itself
f056dd85a8ceca09…  pr119762-files.json live carrier payload as fetched
bcff3038d4819a7a…  proof.log           full transcript

Host-side Crabbox CLI output — crabbox version, the post-run crabbox doctor
showing leases=0, the verbatim warmup and stop lines, and this tarball's
sha256 — is retained beside it as crabbox-env.txt.

Cleanup. crabbox stop --provider local-container --target linux --id cs1081proofdeleted lease=cbx_252483d2f160 server=… name=crabbox-cs1081proof-138c0406,
and a subsequent crabbox doctor --provider local-container reports leases=0.
Both lines are in crabbox-env.txt.

Limits. Egress is open in this lease, so the carrier payload was fetched
live; the run is therefore reproducible only while #119762 remains open at that
head — the exact payload is retained in the artifact for replay. The lease is a
local Docker container on one host rather than an org-brokered backend, so this
proves classifier and automation behavior, not scheduling or distribution.
The downstream arm synthesises a report whose other gates (security cleared,
review complete) are held constant so the detector is the only variable; it does
not prove any PR becomes merge-ready.


Earlier host A/B proof (retained)

Claim. On the live payload of openclaw/openclaw#119762
the detector stops reporting persistent-data surfaces, and the false signal alone
no longer drives the report to needs-human.

Exercised surface. Compiled dataModelChangeFromPullFilesForTest from
src/clawsweeper-change-detection.ts, then the real
reviewAutomationMarkersFromReport and renderReviewCommentFromReport.

Scenario / fixture. The live 7-file pull-file payload of that PR from the
GitHub pull-files API at head 0d5398a9233014776b06d26252162cf5664535ca
3 workflow files, 1 docs page, 1 script, 2 test files, zero persistent production
code.

Command and environment. pnpm run build on Node 24 in two worktrees —
upstream/main at 81c23bede7 and this branch — then both compiled detectors
over the same payload, each result fed into the same downstream consumer, so
the detector is the only variable.

Observed result.

===== BEFORE (upstream/main 81c23bede7) =====
  data_model_change : true
  surfaces (5):
      - migration/backfill/repair: docs/reference/full-release-validation.md
      - persistent cache schema: docs/reference/full-release-validation.md
      - serialized state: test/scripts/[package-acceptance-workflow test]
      - vector/embedding metadata: .github/workflows/openclaw-live-and-e2e-checks-reusable.yml
      - vector/embedding metadata: test/scripts/[package-acceptance-workflow test]
  downstream markers: verdict:needs-human
  "Stored data model" section rendered: true

===== AFTER (this branch) =====
  data_model_change : false
  surfaces (0):
  downstream markers: verdict:pass
  "Stored data model" section rendered: false
  public warning   : (none)

The bracketed filename is elided only because it is an openclaw/openclaw
carrier path, not a file in this repository; the unedited text is in
ClawSweeper's own comment on the carrier.

The two surfaces ClawSweeper actually published on that PR — vector/embedding metadata on the workflow and on the test file — both appear in the BEFORE arm,
so the reproduction is causally aligned with the live review rather than merely
similar.

Freshness. Captured at this branch head e23ba84401, cut from
81c23bede7. The other identifiers belong to the carrier, not to this PR:
0d5398a9233014776b06d26252162cf5664535ca is its head and 5198139710 is its
ClawSweeper comment.

Limitations / what was not tested. The BEFORE arm yields 3 surfaces the live
review does not show (the two docs/… ones and serialized state on the test
file); the docs patch is 41.7 KB and was almost certainly truncated out of the
live review context. This PR is scoped to the detector, and that gap does not
affect the two surfaces that do match. The A/B isolates the detector-owned
transition only: the synthesized report holds the other gates (security cleared,
proof sufficient, review complete) constant and favourable, so it proves the
false data-model signal alone flips passneeds-human and back — not
that the carrier PR becomes merge-ready, which also depends on
configSurfaceChangeFromContext, the security assessment, the proof status, and
required CI, none of which this patch changes. This change is limited to the
boundary #983 established; intentionally out of scope is the read-and-write
context classifier redesign needed to narrow metadata in a telemetry signature
or headers.upgrade in the gateway any further.

Recut the persistent-data classifier boundary accepted on openclaw#983 onto the
active post-openclaw#993 change-detection module.

The content regexes in dataModelSurfacesFromPatch ran over the joined
patch text with no reference to the file path, so ordinary runtime,
test, fixture, telemetry, docs, and workflow vocabulary - metadata,
cache, upgrade, JSON.parse, vector, dimension, repair - was enough to
raise a persistent data-model surface. That set data_model_change, cost
the pull request pass and automerge eligibility, and published a
migration-proof demand on changes that touch no stored state.

Exclude test, fixture, and snapshot paths from the candidate set, narrow
the migration, cache, and vector content regexes, require explicit
storage APIs or nearby file I/O plus JSON plus a persistence noun for
serialized state, and anchor the migration filename path hint.

pathHint, unknown-data-model-change, unknown-truncated-pull-files,
truncation detection, and pullFilesTruncated fail-closed behavior are
unchanged.
@masatohoshino
masatohoshino marked this pull request as ready for review August 9, 2026 05:19
@clawsweeper clawsweeper Bot added 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. 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 Aug 9, 2026
@clawsweeper

clawsweeper Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 23, 2026, 3:04 AM ET / 07:04 UTC.

ClawSweeper review

What this changes

The PR narrows persistent-data detection and adds regression coverage so ordinary workflow, test, fixture, telemetry, runtime, and documentation terms do not trigger migration-proof automation gates.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep open for a maintainer decision: current main still uses the broad classifier, while this PR intentionally trades some isolated persistence detections for eliminating false migration-proof gates.

Priority: P2
Reviewed head: e23ba84401cbfe7827badc9ae72e24f17916bb1d
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The patch and proof are strong, with the remaining question being an intentional automation-policy trade-off.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR body records current-head Docker-backed Crabbox proof of both cleared false positives and retained persistence and fail-closed gates.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body records current-head Docker-backed Crabbox proof of both cleared false positives and retained persistence and fail-closed gates.
Evidence reviewed 6 items Current-main behavior: Current main still evaluates every changed-file candidate and uses broad migration, serialized-state, cache, and vector vocabulary, so the central false-positive class remains present.
Proposed classifier boundary: The PR filters test and fixture paths, narrows content signals, and requires explicit or nearby file-backed JSON persistence before emitting serialized-state surfaces.
Automation impact: The report builder invokes the classifier, and a detected surface renders the migration-or-upgrade-proof warning consumed by review automation.
Findings None None.
Security None None.

Live Verification

Command: node --test test/pr-surface-policy.test.ts

Result: FAIL (partial) — step 2 expect_output data model detector ignores workflow, test, and fixture vocabulary: expected terminal output was not visible within 30 seconds: "data model detector ignores workflow, test, and fixture vocabulary"

node --test test/pr-surface-policy.test.ts
runner@runnervm76f27:/tmp/clawsweeper-live-proof-1081-R4O528/target$ node --test test/pr-surface-policy.test.ts
node:internal/modules/esm/resolve:271
    throw new ERR_MODULE_NOT_FOUND(
          ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/tmp/clawsweeper-live-proof-1081-R4O528/target/dist/clawsweeper.js' imported from /tmp/clawsweeper-live-proof-
1081-R4O528/target/test/pr-surface-policy.test.ts
    at finalizeResolution (node:internal/modules/esm/resolve:271:11)
    at moduleResolve (node:internal/modules/esm/resolve:865:10)
    at defaultResolve (node:internal/modules/esm/resolve:992:11)
    at #cachedDefaultResolve (node:internal/modules/esm/loader:701:20)
    at #resolveAndMaybeBlockOnLoaderThread (node:internal/modules/esm/loader:721:38)
    at ModuleLoader.resolveSync (node:internal/modules/esm/loader:759:56)
    at #resolve (node:internal/modules/esm/loader:683:17)
    at ModuleLoader.getOrCreateModuleJob (node:internal/modules/esm/loader:603:35)
    at ModuleJob.syncLink (node:internal/modules/esm/module_job:163:33)
    at ModuleJob.link (node:internal/modules/esm/module_job:253:17) {
  code: 'ERR_MODULE_NOT_FOUND',
  url: 'file:///tmp/clawsweeper-live-proof-1081-R4O528/target/dist/clawsweeper.js'
}

Node.js v24.19.0
✖ test/pr-surface-policy.test.ts (114.706876ms)
ℹ tests 1
ℹ suites 0
ℹ pass 0
ℹ fail 1
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 121.223657

✖ failing tests:

test at test/pr-surface-policy.test.ts:1:1
✖ test/pr-surface-policy.test.ts (114.706876ms)
  'test failed'
runner@runnervm76f27:/tmp/clawsweeper-live-proof-1081-R4O528/target$ node --test test/pr-surface-policy.test.ts
node:internal/modules/esm/resolve:271
    throw new ERR_MODULE_NOT_FOUND(
          ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/tmp/clawsweeper-live-proof-1081-R4O528/target/dist/clawsweeper.js' imported from /tmp/clawsweeper-live-proof-
1081-R4O528/target/test/pr-surface-policy.test.ts
    at finalizeResolution (node:internal/modules/esm/resolve:271:11)
    at moduleResolve (node:internal/modules/esm/resolve:865:10)
    at defaultResolve (node:internal/modules/esm/resolve:992:11)
    at #cachedDefaultResolve (node:internal/modules/esm/loader:701:20)
    at #resolveAndMaybeBlockOnLoaderThread (node:internal/modules/esm/loader:721:38)
    at ModuleLoader.resolveSync (node:internal/modules/esm/loader:759:56)
    at #resolve (node:internal/modules/esm/loader:683:17)
    at ModuleLoader.getOrCreateModuleJob (node:internal/modules/esm/loader:603:35)
    at ModuleJob.syncLink (node:internal/modules/esm/module_job:163:33)
    at ModuleJob.link (node:internal/modules/esm/module_job:253:17) {
  code: 'ERR_MODULE_NOT_FOUND',
  url: 'file:///tmp/clawsweeper-live-proof-1081-R4O528/target/dist/clawsweeper.js'
}

Node.js v24.19.0
✖ test/pr-surface-policy.test.ts (63.31553ms)
ℹ tests 1
ℹ suites 0
ℹ pass 0
ℹ fail 1
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 69.80553

✖ failing tests:

test at test/pr-surface-policy.test.ts:1:1
✖ test/pr-surface-policy.test.ts (63.31553ms)
  'test failed'
runner@runnervm76f27:/tmp/clawsweeper-live-proof-1081-R4O528/target$

Assertions:

  • FAIL expect_output: data model detector ignores workflow, test, and fixture vocabulary

How this fits together

ClawSweeper scans changed OpenClaw pull-request files for persistent-data signals and records matching surfaces in its review report. Those surfaces produce a migration-proof warning and can prevent automated pass, repair, or merge paths.

flowchart LR
  A[Changed pull-request files] --> B[Persistent-data classifier]
  B --> C[Detected data surfaces]
  C --> D[Review report]
  D --> E[Migration-proof warning]
  E --> F[Automation verdict and merge gates]
Loading

Decision needed

Question Recommendation
Should ClawSweeper accept the documented false-negative boundary for isolated persistence-shaped edits in order to stop false migration-proof gates on ordinary runtime and test changes? Accept the narrower boundary: Merge the PR and retain path hints plus explicit storage signals as the conservative fallback.

Why: The implementation is focused, but choosing which persistence signals may suppress automation is an enduring review-policy decision.

Before merge

  • Resolve merge risk (P1) - Merging intentionally allows some isolated JSON/file persistence or configuration-write edits without a migration-proof gate; maintainers must accept that precision trade-off for automation.
  • Complete next step (P2) - A maintainer must explicitly accept the deliberate automation-gate coverage trade-off before this otherwise focused PR can land.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +15 net; tests +190 A small classifier change is paired with focused positive and negative surface coverage.

Root-cause cluster

Relationship: canonical
Canonical: #1081
Summary: This PR is the active recut of the previously unmerged persistent-data classifier proposal.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Accept the documented classifier boundary (recommended)
    Approve the stated false-negative trade-off and merge with the new focused regressions protecting retained data-model signals.
  2. Retain broader gating
    Keep the existing detector until maintainers choose a narrower alternative that preserves the desired isolated persistence cases.

Technical review

Best possible solution:

Keep persistent-data gating tied to concrete storage operations and path corroboration, with an explicit maintainer record accepting the documented false-negative boundary.

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

Yes—source-reproducible: current main applies broad vocabulary regexes to every changed-file candidate, and the PR supplies focused inputs that exercise the resulting false-positive class.

Is this the best way to solve the issue?

Unclear: the implementation directly removes the proven false positives and preserves several positive guards, but the intentional loss of some isolated persistence signals needs maintainer policy acceptance.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This is a bounded automation-classification bug affecting review friction rather than runtime availability or data integrity.
  • merge-risk: 🚨 automation: Detected surfaces feed review verdict markers and pass or merge eligibility.
  • 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 (terminal): The PR body records current-head Docker-backed Crabbox proof of both cleared false positives and retained persistence and fail-closed gates.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body records current-head Docker-backed Crabbox proof of both cleared false positives and retained persistence and fail-closed gates.

Evidence

What I checked:

Likely related people:

  • Martin Cleary: Available current-main blame attributes the classifier lines to the local history boundary commit. (role: current-main source provenance; confidence: medium; commits: d389e6addf4d; files: src/clawsweeper-change-detection.ts)
  • steipete: The linked merged refactor moved this detector into the module this PR now changes. (role: adjacent refactor author; confidence: medium; commits: 8fd1140e7aa9; files: src/clawsweeper-change-detection.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Record maintainer acceptance of the documented false-negative boundary before merge.

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 (15 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-09T13:59:49.170Z sha e23ba84 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T14:33:10.495Z sha e23ba84 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T15:36:49.651Z sha e23ba84 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T15:59:50.706Z sha e23ba84 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T18:04:15.860Z sha e23ba84 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T19:11:14.640Z sha e23ba84 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T22:50:52.144Z sha e23ba84 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-12T06:24:16.330Z sha e23ba84 :: needs maintainer review before merge. :: none

@masatohoshino

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

The current-head Docker-backed Crabbox proof is in the PR body — it was added at
2026-08-09T06:23:07Z, about two minutes before this review started
(06:25:17Z), so I think the scan read the previous revision.

The body now carries a Real Behavior Proof — Docker-backed Crabbox, current
head
section with the fields the checklist asks for:

  • provider local-container, image ubuntu:26.04, Crabbox 0.40.0, Docker 29.4.2
  • lease cbx_252483d2f160, container crabbox-cs1081proof-138c0406, uid=1001
  • artifact cbx_252483d2f160-artifacts.tgz, sha256
    d8a19f20cca10db368e9d99b3968a60cb936e34e05b6bdb36fc7776413c35134, plus the
    per-file hashes inside it
  • head e23ba84401cbfe7827badc9ae72e24f17916bb1d built in-container, with the
    build attributed to that head rather than a synced dist
  • limits, and a cleanup record (leases=0 after release)

On the P1 about the retained positives being exercised through the production
path: that is section 2 of the proof. Six persistence cases — SQL/DDL, migration
plus backfill, serialized durable state, durable object storage, cache version,
and persisted vector identifier — go through the compiled
dataModelChangeFromPullFilesForTest and then the real
reviewAutomationMarkersFromReport, and each still emits its surface and still
returns verdict:needs-human when no upgrade proof is recorded. The three
fail-closed cases (missing patch, truncated patch, pullFilesTruncated) keep
their unknown-* markers and also stay needs-human.

No code changed and the head is unchanged.

@clawsweeper

clawsweeper Bot commented Aug 9, 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 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: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed 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: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 9, 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: 🐚 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.

1 participant