fix(share): validate imported tombstones

[steipete]

Summary

• Validate messages.deleted_at before either supported SQLite snapshot import path mutates the archive.
• Accept null, empty, and RFC3339Nano tombstones; reject non-string or malformed timestamps with an actionable field error.
• Leave unrelated tables untouched by this message-specific validation.

Extracted from the generic SQLite/tombstone hardening in #131. Thanks @GrantTheAssistant; contributor credit is preserved in the commit and changelog.

Proof

• go test ./... -count=1
• go test -race ./internal/share -count=1
• golangci-lint run ./...
• go vet ./...
• staticcheck ./...
• gosec -exclude=G101,G115,G202,G301,G304 ./...
• govulncheck ./...
• go mod verify
• go mod tidy -diff

The regression exercises valid nanosecond tombstones, null/empty compatibility, malformed timestamp rejection, non-string rejection, and table scoping. Full share integration tests exercise both import engines with the validator installed at their pre-insert boundary.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 39bfdd0763

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Validate tombstones on the incremental path too

This installs the new validator for full replacement imports, but the normal MergeIfChanged path still uses snapshot.ImportIncremental with a filter that only skips DMs, and importMergeSnapshotRow/upsertMergeSnapshotRow do not call validateSnapshotRow. When a changed-tail snapshot is applied through MergeIfChanged, a malformed or non-string messages.deleted_at can still be written to the archive before any error, so the main supported update path remains unprotected. Please run the same validation from the incremental filter or import row hook as well.

Useful? React with 👍 / 👎.

[clawsweeper]

Codex review: needs changes before merge. Reviewed July 17, 2026, 3:56 AM ET / 07:56 UTC.

Summary
Adds validation and normalization for imported messages.deleted_at values in replacement and legacy SQLite snapshot imports, with regression tests and a changelog entry.

Reproducibility: yes. at source level: apply a changed-tail snapshot containing a malformed or non-string messages.deleted_at through MergeIfChanged and verify that the current incremental callbacks accept it. A live reproduction was not run during this read-only review.

Review metrics: 2 noteworthy metrics.

• Import-path coverage: 2 validated, 1 missed. The routine changed-tail import remains able to bypass the validator.
• Patch surface: 3 files, +75/-1. The repair is small and focused despite the blocking path omission.

Root-cause cluster
Relationship: canonical
Canonical: #135
Summary: This PR is the focused landing candidate for the tombstone-validation portion extracted from a much broader closed proposal.

Members:

• partial_overlap: feat: add isolated Discord archive data plane #131 - The closed, unmerged proposal contained broader SQLite and tombstone hardening; this PR preserves only the focused imported-tombstone change.

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

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🌊 off-meta tidepool
Patch quality: 🦐 gold shrimp
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

• Wire validateSnapshotRow into the MergeIfChanged incremental path before any row mutation.
• [P2] Add an integration regression covering malformed and non-string tombstones through a changed-tail import and asserting the archive remains unchanged.

Next step before merge

• [P2] A narrow mechanical repair can extend the existing validator to the incremental importer and add focused regression coverage without requiring product or security judgment.

Security
Cleared: The diff adds bounded data validation without changing dependencies, execution sources, permissions, credentials, workflows, or other supply-chain surfaces.

Review findings

• [P1] Validate tombstones on the incremental import path — internal/share/share.go:503

Review details

Best possible solution:

Run one shared tombstone validator at the pre-mutation boundary used by replacement, legacy, and incremental snapshot engines, with a changed-tail regression proving malformed rows fail without altering the archive.

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

Yes at source level: apply a changed-tail snapshot containing a malformed or non-string messages.deleted_at through MergeIfChanged and verify that the current incremental callbacks accept it. A live reproduction was not run during this read-only review.

Is this the best way to solve the issue?

No. The validator itself is appropriately narrow, but installing it only on replacement and legacy imports leaves the normal incremental path inconsistent; all supported import engines should share the same pre-insert validation.

Full review comments:

• [P1] Validate tombstones on the incremental import path — internal/share/share.go:503
  The new checks are installed for replacement and legacy file imports, but routine updates use snapshot.ImportIncremental through MergeIfChanged; its merge/upsert callbacks never call validateSnapshotRow. A changed-tail snapshot can therefore still write a malformed or non-string messages.deleted_at, leaving the primary update workflow unprotected. Invoke the validator from the incremental filter or row hook and add a regression proving rejection occurs before mutation.
  Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: not found in the target repository.

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

Label changes

Label changes:

• add P2: This is a bounded archive-import integrity fix whose current patch misses one supported workflow but does not indicate an emergency or widespread outage.
• 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 contributor proof gate does not apply to this collaborator-authored PR; the supplied tests and green CI remain useful supplemental validation but do not cover the missed incremental path.

Label justifications:

• P2: This is a bounded archive-import integrity fix whose current patch misses one supported workflow but does not indicate an emergency or widespread outage.
• 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 contributor proof gate does not apply to this collaborator-authored PR; the supplied tests and green CI remain useful supplemental validation but do not cover the missed incremental path.

Evidence reviewed

Acceptance criteria:

• [P1] go test ./internal/share -count=1.
• [P1] go test -race ./internal/share -count=1.
• [P1] go test ./... -count=1.

What I checked:

• Replacement import validation: The PR adds validateSnapshotRow to the replacement import filter after direct-message rows are excluded. (internal/share/share.go:503, 39bfdd0763fb)
• Legacy file import validation: The PR also validates rows in importTableFile before values are passed to the insert statement. (internal/share/share.go:1668, 39bfdd0763fb)
• Incremental path remains uncovered: The existing review identified that MergeIfChanged uses snapshot.ImportIncremental, while importMergeSnapshotRow and upsertMergeSnapshotRow do not invoke the validator; the current head has not changed since that review. (internal/share/share.go:503, 39bfdd0763fb)
• Incremental updates are a supported primary workflow: Current documentation states that routine updates merge changed canonical shards and preserve destination-only rows, making the missed changed-tail path a supported user workflow rather than an unused edge case. (README.md:624, 91aa80d2f826)
• Historical incremental-import contract: The changelog records multiple shipped fixes specifically preserving incremental changed-tail snapshot imports, including compatibility with CrawlKit's safe changed-tail replacement plan. (CHANGELOG.md:284, 91aa80d2f826)
• Validation evidence: All reported GitHub checks passed, but the PR body supplies test and static-analysis commands rather than a changed-tail malformed-tombstone case demonstrating the missed incremental route. (internal/share/share_test.go:2278, 39bfdd0763fb)

Likely related people:

• steipete: Peter Steinberger authored prior share delta-import work, has maintained the repository's internal import area, and authored this focused validation patch. (role: share import feature owner; confidence: high; commits: b52eefaa400f, 39bfdd0763fb; files: internal/share/share.go, internal/share/share_test.go)

What the crustacean ranks mean

• 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
• 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
• 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
• 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
• 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
• 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
• 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.