fix(review): keep runtime vocabulary out of persistent-data findings - #1081
fix(review): keep runtime vocabulary out of persistent-data findings#1081masatohoshino wants to merge 1 commit into
Conversation
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.
|
Codex review: needs maintainer review before merge. Reviewed August 23, 2026, 3:04 AM ET / 07:04 UTC. ClawSweeper reviewWhat this changesThe 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 readinessKeep 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 Review scores
Verification
Live VerificationCommand: Result: FAIL (partial) — step 2 Assertions:
How this fits togetherClawSweeper 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]
Decision needed
Why: The implementation is focused, but choosing which persistence signals may suppress automation is an enduring review-policy decision. Before merge
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. Merge-risk optionsMaintainer options:
Technical reviewBest 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. LabelsLabel justifications:
EvidenceWhat 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 (15 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review The current-head Docker-backed Crabbox proof is in the PR body — it was added at The body now carries a Real Behavior Proof — Docker-backed Crabbox, current
On the P1 about the retained positives being exercised through the production No code changed and the head is unchanged. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
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.
dataModelSurfacesFromPatchjoins every changed non-comment line into one bloband 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, anddataModelSurfaceReviewRequiredthen costs the PR pass eligibility(
reviewAutomationMarkersFromReport→verdict:needs-human), repair-loop andautomerge eligibility (
isRepairLoopPassReport), and publishes "Confirmmigration or upgrade compatibility proof before merge." The only escape hatch,
hasDataModelUpgradeProof, asks the author to write migration proof for amigration 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 twosurfaces — the reusable live-and-e2e checks workflow, and a package-acceptance
test file. Both are files in
openclaw/openclaw, both labelledvector/embedding metadata, and the trigger in the workflow is a bash variable namedmetadata.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 detectorout of
src/clawsweeper.tsintosrc/clawsweeper-change-detection.ts, so #983'sproduction hunk no longer applied. Its regressions still target
test/pr-surface-policy.test.ts, which is unchanged, so they port directly.This PR changes
dataModelChangeFromContextanddataModelSurfacesFromPatch,porting #983's approved semantics:
upgrade,doctor,repair,reindex,rehydratein changed text no longertrigger on their own (the
doctor/andmigrations/path hints areuntouched, so
src/doctor/repair.tsstill signals);JSON.parse|readFile|serialized|persistedserialized-stateregex 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;
cache (key|version|schema|namespace|ttl);dataModelPathHintso a name merelyending in
-repair.tsno longer reads asrepair.ts; the basename must now beexactly one of
migration|backfill|doctor|repair|upgrade+.ts/.js, and thedirectory branch of the same hint is unchanged.
Boundaries.
unknown-data-model-change,unknown-truncated-pull-files,truncation detection, and
pullFilesTruncatedfail-closed behavior areuntouched. Within
dataModelPathHintonly the migration filename branch isanchored; every directory branch and the two-factor corroboration in
dataModelTextMatchesPathHint/dataModelTextLooksLikePersistedShapeFieldareunchanged, 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 smallerchange, so it was implemented first and run against the same matrix. It loses
2 approved positives —
migrations/0001-init.sql(persistence outside thenormal source tree) and #983's retained
docs/storage.mdprose — and leaves6 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 matrixbelow.
isOpenClawSourcePathstays private insrc/pr-surface-stats.ts: the acceptedshape needs a test-path predicate, not a production-source allowlist, and that
module's
isOpenClawTestPathunder-excludes here (misses nestedtest/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.
JSON.parseline and thereadFilecall stays in unchanged context,changedPatchLinesnever sees the I/O line, so no surface is raised. Thiscannot 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
pathHintroute still covers persistence-locatedfiles. Residuals: a storage-path file whose only changed line carries no
corroborating token —
src/storage/snapshot-loader.tswithreturn JSON.parse(raw) as SessionStateV3;— goes clean; and so doeson-disk configuration persistence such as
src/config/io.write.tswritingJSON.stringify(config)toconfigPath, becauseconfigis deliberatelyabsent 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.tsdoing exactly that write and asserts it raises nosurface.
upgrade,doctor,repair,reindex,rehydratedropped from themigration 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(...)goesclean, because the
pathHintcorroboration list does not carry those wordseither; and a generic production file such as
src/runtime/compat.tswhoseonly changed line is
await upgrade(state);goes clean, since neither itspath nor its text now matches. The
doctor/,migrations/,backfill/,repair/andupgrade/path hints are untouched, sosrc/doctor/repair.tsstill signals.Restoring either means re-widening the content regex (re-opening the class this
PR closes) or widening
pathHintbeyond #983. Happy to add either if you wantthe boundary moved — I kept this PR to the shape you already approved.
Two further deltas, same origin, completing the list of four: bare
serialized/persistedprose no longer signals (#983 removed those words; its retained prose positives arecache versionandembedding dimension), and the migration filename branch nowrequires the basename to be exactly one of
migration|backfill|doctor|repair| upgrade+.ts/.js(its directory branch still matchesmigrations/,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
passplus automerge and repair-loop eligibility. Persistencechanges 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 negativesuites (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/main81c23bede7, after = this branch:The 2 "before" positives that were not exact are the two false extras #983
removed:
serialized state: scripts/config-fixture.tsandserialized state: docs/storage.md.Negatives, each a single changed file: workflow bash variable
metadata;workflow
vector-testsjob name; testmetadataassertion; fixture"dimension": 24;test/fixtures/vector-schema.tswithembeddingDimension;telemetry
metadataparameter;req.headers['x-request-metadata']; in-memorynew Map()cache;req.headers.upgrade !== "websocket"; API-responseJSON.parse; docs-onlyupgrade; a colocated*.test.tswithJSON.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 storageschema;
dimension/collectionundersrc/memory/→ vector/embeddingmetadata;
cacheVersionundersrc/cache/→ persistent cache schema;migrations/0001-init.sqlat repo root → database schema;extensions/…→serialized state; #983's four semantic positives; and the fail-closed set
(missing patch, truncated patch,
pullFilesTruncated) → theunknown-*markers.What is re-runnable from this diff, and what is not. The added regressions in
test/pr-surface-policy.test.tsencode a representative subset of the matrix —each is a
pullFilesentry with its patch and its expected surfaces — sonode --test test/pr-surface-policy.test.tsre-runs that subset against your ownbuild. 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 therejected production-path alternative all live in a scratch harness under
.artifacts/, whichAGENTS.mdkeeps out of the tree. Every one of them is asingle call you can paste:
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:
Feeding each result into
reviewAutomationMarkersFromReportasdata_model_changeand
data_model_surfacesreproduces the marker flip shown below.pnpm run checkexits non-zero. Static checks,format:check, all threebuild:*projects and all fourlint:*lanes pass; 13 tests fail intest/repair/*(git plumbing, dependency-setup process reaping, and a failure tocreate a coverage profile directory under the system temp dir). Those 13 fail
identically on a pristine
upstream/mainworktree 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-policyorchange-detection test fails. Upstream CI for this exact head is green,
including its own
pnpm checkrun, which independently confirms those 13failures 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
e23ba84401and the branchworktree has no modifications.
upstream/mainhas advanced since this branchwas 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/maintouches neither PR file, and that thebranch 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
dataModelChangeFromPullFilesForTestfromdist/clawsweeper-change-detection.js, feeding the realreviewAutomationMarkersFromReportandrenderReviewCommentFromReportfromdist/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 capturecrabbox-env.txt(which holdscrabbox version,crabbox doctor, the verbatimwarmupandstopoutput, and the artifact hash).local-container(Docker-backed)ubuntu:26.04cbx_252483d2f160crabbox-cs1081proof-138c0406(docker idb7de0b7f90a1at provisioning)/work/crabbox/cbx_252483d2f160/clawsweeperuid=1001(crabbox)— non-root;docker_socket=falseLinux 6.8.0-136-generic x86_64e23ba84401cbfe7827badc9ae72e24f17916bb1d. The transcript logsgit status : 1 modified pathsat run start andrestored to head : 3 modified pathsafter the base rebuild; the two tracked files are checked back to head before the run ends, and no commit is made.81c23bede7a805351bcbb1d5fde54ff278337535(PR parent)2026-08-09T06:01:19Z(30m idle timeout)Commands.
Inside the lease:
pnpm install --frozen-lockfile→pnpm run build→ fetch thelive carrier payload from the GitHub pull-files API → run the harness against the
built
dist.Build attribution (teeth). The
distunder test is built in-container fromthis head, not synced or stale:
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, head0d5398a9233014776b06d26252162cf5664535ca, 7 files, sha256f056dd85a8ceca09…).Workflow and test vocabulary no longer create persistent-data surfaces;
data_model_change=false; no migration-proof gate from this detector; thedetector-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:
ALTER TABLE … ADD COLUMNdatabase schema: src/db/schema.tsverdict:needs-human+ warningbackfill…(db)migration/backfill/repair:on both themigrations/*.sqlandsrc/doctor/backfill.tsverdict:needs-human+ warningwriteFileSync(statePath, JSON.stringify(…))serialized state: src/state/session-store.tsverdict:needs-human+ warningstate.storage.putdurable storage schema: src/gateway/worker/do.tsverdict:needs-human+ warningcacheVersionpersistent cache schema: src/cache/keys.tsverdict:needs-human+ warningembeddingDimension/document_idvector/embedding metadata: src/memory/vector-store.tsverdict:needs-human+ warning6/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
unknown-data-model-change: src/storage/session-state.tsverdict:needs-humanunknown-data-model-change: packages/database/schema.ts(plusdatabase schema)verdict:needs-humanpullFilesTruncatedunknown-truncated-pull-filesverdict:needs-human3/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, sha256d8a19f20cca10db368e9d99b3968a60cb936e34e05b6bdb36fc7776413c35134(21,879bytes), collected via
--artifact-globto.crabbox/runs/cbx_252483d2f160/. Contents and their hashes:Host-side Crabbox CLI output —
crabbox version, the post-runcrabbox doctorshowing
leases=0, the verbatimwarmupandstoplines, and this tarball'ssha256 — is retained beside it as
crabbox-env.txt.Cleanup.
crabbox stop --provider local-container --target linux --id cs1081proof→deleted lease=cbx_252483d2f160 server=… name=crabbox-cs1081proof-138c0406,and a subsequent
crabbox doctor --provider local-containerreportsleases=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
#119762remains open at thathead — 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
dataModelChangeFromPullFilesForTestfromsrc/clawsweeper-change-detection.ts, then the realreviewAutomationMarkersFromReportandrenderReviewCommentFromReport.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 buildon Node 24 in two worktrees —upstream/mainat81c23bede7and this branch — then both compiled detectorsover the same payload, each result fed into the same downstream consumer, so
the detector is the only variable.
Observed result.
The bracketed filename is elided only because it is an
openclaw/openclawcarrier 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 metadataon 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 from81c23bede7. The other identifiers belong to the carrier, not to this PR:0d5398a9233014776b06d26252162cf5664535cais its head and5198139710is itsClawSweeper comment.
Limitations / what was not tested. The BEFORE arm yields 3 surfaces the live
review does not show (the two
docs/…ones andserialized stateon the testfile); 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
pass→needs-humanand back — notthat the carrier PR becomes merge-ready, which also depends on
configSurfaceChangeFromContext, the security assessment, the proof status, andrequired 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
metadatain a telemetry signatureor
headers.upgradein the gateway any further.