Skip to content

fix(portable): avoid archive churn from hydration checkpoints - #197

Closed
vincentkoc wants to merge 2 commits into
mainfrom
fix/runtime-checkpoint-identity-20260913
Closed

vincentkoc wants to merge 2 commits into
mainfrom
fix/runtime-checkpoint-identity-20260913

Conversation

@vincentkoc

Copy link
Copy Markdown
Member

What Problem This Solves

Fixes unnecessary portable archive updates when gitcrawl-store persists hydration cursors in its runtime database without changing source facts.

User Impact

Portable exports retain numeric hydration checkpoints for recovery, while cursor-only changes preserve the existing semantic artifact identity. This is a prerequisite for the separate gitcrawl-store checkpoint repair; it does not deploy a producer binary, migrate a live database, or publish a cloud archive.

Why This Change Was Made

Add only gitcrawl_store_hydration_progress to the existing exact local-bookkeeping exclusion. Unknown tables, source content, exact file hashes, and schema versions retain their existing behavior.

The runtime cache remains the durable cursor owner. A publisher may skip a cursor-only export, so the published checkpoint can lag the runtime cache.

Evidence

  • Independent review of exact signed head 1749b9632ca25f18dda94cfea1fdaf36dfe5dd0d: no findings.
  • Corrected regression failed against the byte-identical parent 223f23dd3b75d4c5c56bc0d85a9d685acd60bfc0 identity implementation, then passed with the repair.
  • Focused semantic and export identity tests passed: go test ./internal/portable -run 'TestSemanticArtifactIdentity|TestCurrentStateSemanticPolicy|TestExportSemanticIdentity|TestExport.*(Identity|Stable)|TestArtifactIdentity' -count=1.
  • Coverage includes both retained phase cursors, cursor-only no-op identity, meaningful source changes with the checkpoint table present, and an unknown same-prefix table remaining meaningful.
  • Formatting and git diff --check passed. Production delta: +3 lines (one policy entry and its comment); tests: +65 lines. No dependency changes.
  • Hosted Linux/macOS tests, Windows portable-filesystem proof, and documentation checks are pending on this draft.

@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. 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. labels Sep 13, 2026
@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 13, 2026, 10:11 AM ET / 14:11 UTC.

ClawSweeper review

What this changes

Exclude producer hydration checkpoints from portable archive identity calculations while retaining their rows in exported SQLite files, with regression tests and documentation.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 4 items remain

The change remains useful and is absent from main, but changing a shipped identity profile introduces a cross-version archive validation defect. This member-authored PR also requires explicit maintainer handling.

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

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The focused implementation and useful regression tests are limited by a concrete archive compatibility defect.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from the external-contributor proof gate. Supplied tests exercise the real export owner with fixture databases, but do not establish cross-version subscriber compatibility.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The MEMBER-authored PR is exempt from the external-contributor proof gate. Supplied tests exercise the real export owner with fixture databases, but do not establish cross-version subscriber compatibility.
Evidence reviewed 7 items Introduced change and main comparison: The complete pinned delta contains one exact-table exclusion, its tests, documentation, and changelog entry. Main retains this table in semantic identity; the proposed exclusion is not already implemented.
Normalization preserves exported checkpoints: Identity computation snapshots the finalized archive into a disposable database before applying DroppedTables. Export calculates the exact checksum separately, so the new exclusion does not delete checkpoint rows from the exported file.
Subscriber rejection path: validatePortableDBManifest recomputes current-state-semantic-v1 and rejects any differing artifactId even when the exact SHA matches. Both archive staging and subscriber refresh invoke this validator.
Findings 1 actionable finding [P1] Preserve compatibility with the shipped semantic identity profile
Security None None.

How this fits together

Gitcrawl exports runtime SQLite data into portable archives with an exact file checksum and a semantic identity used to avoid redundant publication. Subscribers recompute the declared identity when validating archives before adoption.

flowchart LR
  A[Runtime SQLite database] --> B[Portable export]
  B --> C[Archive retaining checkpoints]
  C --> D[Disposable normalized copy]
  D --> E[Semantic identity]
  C --> F[Exact checksum]
  E --> G[Subscriber validation]
  F --> G
Loading

Decision needed

Question Recommendation
Must checkpoint-bearing exports remain usable by existing subscribers, or may publication require a coordinated subscriber upgrade? Stage a compatible profile transition: Retain legacy validation and compatible publication until subscribers support a separately identified checkpoint-insensitive profile.

Why: Versioning the changed digest preserves an unambiguous contract, but choosing when publishers may emit it requires knowledge of deployed consumers.

Before merge

  • Preserve compatibility with the shipped semantic identity profile (P1) - Adding this exclusion changes the digest of every checkpoint-bearing archive while still declaring current-state-semantic-v1, which is already shipped in v0.9.6. An archive exported with the old policy is rejected by the new validatePortableDBManifest at internal/cli/portable_manifest.go:123-128; an older reader likewise computes the wrong digest for a newly exported archive. Exact SHA validation can pass while subscriber adoption fails. Preserve legacy profile validation and introduce the changed policy through an explicit compatible rollout, with old-export/new-reader and supported old-reader/new-export coverage. The added test only compares exports made by the same patched implementation.
  • Resolve merge risk (P1) - Checkpoint-bearing archives can become unreadable across producer/subscriber version boundaries despite intact file checksums; the deployed version mix and existing checkpoint-bearing archive population are not established.
  • Complete next step (P2) - Choose the compatibility transition, repair the shipped-profile mismatch, and add cross-version archive validation coverage before merge.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [P1] Preserve compatibility with the shipped semantic identity profile — internal/portable/identity.go:74-76
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth production +3 lines; tests +65 lines Production growth is narrowly justified by one explicit bookkeeping exclusion.

Merge-risk options

Maintainer options:

  1. Preserve the shipped identity contract (recommended)
    Separate the changed normalization policy from v1 and cover legacy archive reads plus the approved publisher/subscriber rollout.
  2. Pause publication support
    Keep this prerequisite pending until the producer rollout establishes which subscriber versions must remain compatible.

Technical review

Best possible solution:

Preserve validation of existing v1 archives and introduce checkpoint-insensitive identity through an explicitly versioned, compatibility-tested producer/subscriber transition.

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

Yes, source establishes the trigger: export a database containing the checkpoint table with the old identity policy, then validate its unchanged manifest using this branch. The digests differ and subscriber validation rejects it; this review did not execute that scenario.

Is this the best way to solve the issue?

No, the exact-table exclusion is appropriately narrow, but changing the meaning of a shipped profile without a compatibility transition is incomplete.

Full review comments:

  • [P1] Preserve compatibility with the shipped semantic identity profile — internal/portable/identity.go:74-76
    Adding this exclusion changes the digest of every checkpoint-bearing archive while still declaring current-state-semantic-v1, which is already shipped in v0.9.6. An archive exported with the old policy is rejected by the new validatePortableDBManifest at internal/cli/portable_manifest.go:123-128; an older reader likewise computes the wrong digest for a newly exported archive. Exact SHA validation can pass while subscriber adoption fails. Preserve legacy profile validation and introduce the changed policy through an explicit compatible rollout, with old-export/new-reader and supported old-reader/new-export coverage. The added test only compares exports made by the same patched implementation.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 223f23dd3b75.

Labels

Label changes:

  • add P2: This is a bounded archive-publication improvement without evidence of an ongoing urgent outage.
  • add merge-risk: 🚨 compatibility: Changing an existing named digest policy makes checkpoint-bearing archives fail validation across versions.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The MEMBER-authored PR is exempt from the external-contributor proof gate. Supplied tests exercise the real export owner with fixture databases, but do not establish cross-version subscriber compatibility.

Label justifications:

  • P2: This is a bounded archive-publication improvement without evidence of an ongoing urgent outage.
  • merge-risk: 🚨 compatibility: Changing an existing named digest policy makes checkpoint-bearing archives fail validation across versions.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The MEMBER-authored PR is exempt from the external-contributor proof gate. Supplied tests exercise the real export owner with fixture databases, but do not establish cross-version subscriber compatibility.

Evidence

What I checked:

  • Introduced change and main comparison: The complete pinned delta contains one exact-table exclusion, its tests, documentation, and changelog entry. Main retains this table in semantic identity; the proposed exclusion is not already implemented. (internal/portable/identity.go:76, 1749b9632ca2)
  • Normalization preserves exported checkpoints: Identity computation snapshots the finalized archive into a disposable database before applying DroppedTables. Export calculates the exact checksum separately, so the new exclusion does not delete checkpoint rows from the exported file. (internal/portable/identity.go:203, 1749b9632ca2)
  • Subscriber rejection path: validatePortableDBManifest recomputes current-state-semantic-v1 and rejects any differing artifactId even when the exact SHA matches. Both archive staging and subscriber refresh invoke this validator. (internal/cli/portable_manifest.go:123, 1749b9632ca2)
  • Shipped identity contract: Release v0.9.6 uses the same current-state-semantic-v1 profile name but does not exclude gitcrawl_store_hydration_progress. Thus a checkpoint-bearing database has different semantic digests across the released and proposed implementations. (internal/portable/identity.go:16, b72d4ae715d6)
  • Regression coverage and supplied validation: The added test checks two retained phase cursors, cursor-only identity stability, changing exact checksums, and meaningful source changes. The supplied complete PR body reports a failing parent regression and passing focused tests. Both baseline and subsequent exports in the new test use the patched implementation, so it does not exercise cross-version manifests. No tests were executed during this read-only review. (internal/portable/identity_test.go:161, 1749b9632ca2)
  • Feature-history routing: GitHub commit metadata identifies steipete as the author of the commit adding identity.go. Local main history also records subsequent portable subscriber work by Peter Steinberger. Deeper local blame/follow inspection encountered an unavailable historical object, so no additional introduction attribution is inferred. (internal/portable/identity.go:16, ff6b2b41861d)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

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

  • Preserve the shipped profile contract and establish the supported producer/subscriber upgrade sequence.
  • Add cross-version manifest validation coverage alongside the existing cursor-retention tests.

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.

@vincentkoc

Copy link
Copy Markdown
Member Author

Closing as an abandoned approach, not as fixed by #200.

The replacement store design derives hydration progress from native database facts and checkpoints, so it no longer needs gitcrawl_store_hydration_progress or this semantic-identity exclusion. The proposed exclusion would still change the shipped current-state-semantic-v1 digest and break cross-version manifest validation.

The native partial-sync repair is merged in #200. The corresponding store integration remains under qualification in https://github.com/openclaw/gitcrawl-store/pull/32; this closure does not claim that integration is merged or deployed.

@vincentkoc vincentkoc closed this Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant