Skip to content

feat: add exact owner-directed thread purges - #227

Merged
steipete merged 1 commit into
mainfrom
worker/round7-purge-threads
Oct 1, 2026
Merged

steipete merged 1 commit into
mainfrom
worker/round7-purge-threads

Conversation

@steipete

@steipete steipete commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Owner-directed removal needs to delete archived content and retry work while keeping later crawls from restoring it. This replaces the stacked proposal in #217 with a bounded native-local purge-threads command, retaining Hannes Rudolph's ownership and exclusion design.

Preview purge-threads owner/repo --numbers 123,456 --json, then pass the returned plan_id to --apply. The ID binds the archive, exact selected identities, and selected stored rows. Apply revalidates that plan inside the same transaction that removes content/history/derived records and retry work and installs durable exclusions. Shared actor profiles and unrelated conversations remain intact. Owner removal stays separate from provider deletion and successful recovery.

Size: +1,467/-22 across 21 files versus the stacked #217 at +7,431/-99 across 49 files (80% fewer added lines). The original removal-only commit was +1,509/-16; this candidate also adds content-bound plan identity and stronger rollback/collection tests.

Scope cuts: no portable runtime-mirror mutation or refresh ownership path, no arbitrary request-ID option, no idempotent reapply mode, no speculative blob cleanup, and one concise documentation section. Unsafe blob, shared cluster/workflow, ambiguous-identity and reusable-native-ID targets are refused. Portable publication with local exclusions is refused. Existing REST, GraphQL, analytics discovery/recovery, identity enrichment and store write paths respect the exclusions.

Credits: adapted from #217 by @hannesrudolph (Hannes Rudolph); his contribution is retained in the squash attribution and changelog.

Validation: synthetic archive regression tests cover exact selection, stable/stale plans, second-target failure rollback, content/revision/retry removal, shared actor and peer preservation, collection suppression, late receipts, unsafe targets, migrations, and CLI ownership locking. make check passed on one AWS Crabbox lease after all 412 source-file hashes were verified; isolated Codex autoreview is clean through P2. Exact-head CI passed for d4c395d822f4e2baba853748a53334ac237d8c3f: Linux/macOS Go, Windows and Docs, Docker, secret scanning, Socket and CodeQL. Full gate coverage: 85.6%.

Also prepares 0.15.0 release notes with an empty Unreleased section. Release dispatch is held while Apple's notarization preflight returns HTTP 403; no tag is created by this PR.

Adapt the ownership and exclusion design from #217 into a bounded native-local command with content-bound plan identities and atomic apply. Prepare 0.15.0 notes; release remains gated on Apple's notarization agreement.

Co-authored-by: Hannes Rudolph <49103247+hannesrudolph@users.noreply.github.com>
@clawsweeper

clawsweeper Bot commented Oct 1, 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 P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Oct 1, 2026
@clawsweeper

clawsweeper Bot commented Oct 1, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed October 1, 2026, 9:32 AM ET / 13:32 UTC.

ClawSweeper review

What this changes

Adds a preview-and-apply command that permanently removes selected conversations from Gitcrawl’s local archive and prevents later collection from restoring them.

Regression provenance

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

Merge readiness

⛔ Blocked before merge - 4 items remain

This remains useful work absent from current main, but the new export refusal can consume the active archive without producing an artifact. The narrower replacement retains that blocking defect from the related proposal.

Priority: P0
Reviewed head: d4c395d822f4e2baba853748a53334ac237d8c3f

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The focused implementation and regression coverage are useful, but the archive-consuming failure prevents a safe landing.
Proof confidence 🌊 off-meta tidepool Not applicable: The supplied repository state identifies a member author, so the ordinary contributor runtime-proof gate is exempt. Reported Crabbox make check results cover synthetic purge regressions, not an observed consuming-export refusal. Schema-15 upgrade compatibility is supported by the additive migration and focused preservation test; no runtime execution was performed in this review.
Patch quality 🧂 unranked krab (1/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The supplied repository state identifies a member author, so the ordinary contributor runtime-proof gate is exempt. Reported Crabbox make check results cover synthetic purge regressions, not an observed consuming-export refusal. Schema-15 upgrade compatibility is supported by the additive migration and focused preservation test; no runtime execution was performed in this review.
Evidence reviewed 9 items Pinned change ownership: The introduced delta is db5f6bc..d4c395d. The verified test merge has that main revision followed by the exact PR head as parents; its export and consume-source implementations are unchanged.
Introduced publication refusal: PrunePortablePayloads now rejects any archive containing owner exclusions. This is the introduced trigger for the consume-source failure.
Export handoff and cleanup: Export moves the source into staging at lines 197–201, reaches PrunePortablePayloads at line 258, and removes staging on an error before artifact commit at lines 162–165. An excluded archive therefore loses its active source path and produces no artifact.
Findings 1 actionable finding [P1] Reject excluded archives before consuming the source
Security None None.

How this fits together

Gitcrawl collects GitHub conversations into a local SQLite archive used for search, analytics, and clustering. The purge command changes that archive and its collection policy, which portable export also consumes.

flowchart LR
A[Selected conversations] --> B[Preview exact removal]
B --> C[Validate and apply transaction]
C --> D[Archive and durable exclusions]
E[GitHub collection] --> F[Check exclusions]
F --> D
D --> G[Portable export or refusal]
Loading

Before merge

  • Reject excluded archives before consuming the source (P1) - With an owner exclusion present, portable export --consume-source first renames the source into staging, then reaches this new rejection through PrunePortablePayloads. Export’s failure cleanup deletes staging, so the command produces no artifact and requires restoring the external backup. Although later consume-source failures intentionally discard staging, this predictable policy refusal should be checked under the exclusive source lock before the rename. Add a consuming-export regression that verifies rejection preserves the source bytes.
  • Resolve merge risk (P1) - An archive containing owner exclusions can be moved and deleted by consuming export before publication is refused, leaving recovery dependent on the external backup.
  • Complete next step (P2) - Check owner exclusions under the exclusive source lock before consuming the archive, and add a regression proving refusal preserves its exact bytes.
  • Improve patch quality - Preflight owner exclusions before the consuming rename and add a regression proving refusal preserves the original archive.

Findings

  • [P1] Reject excluded archives before consuming the source — internal/store/portable.go:104-105
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Introduced code growth production +711/−12; tests +724/−10; docs and changelog +32 The production growth implements the stated bounded purge capability, with comparable regression coverage.

Merge-risk options

Maintainer options:

  1. Refuse before consuming (recommended)
    Check owner exclusions while holding the source’s exclusive SQLite lock and prove that rejection preserves the original file.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Preflight owner exclusions under the consume-source exclusive SQLite lock before renaming the source, retain the pruning guard, and add a regression proving excluded archives are refused without changing source bytes or producing an artifact.

Technical review

Best possible solution:

Reject excluded archives under the consume-source lock before moving them, while preserving the existing post-handoff failure contract.

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

Yes, from source: export a closed, rollback-journal archive containing an owner exclusion with --consume-source; the new guard fails after rename and cleanup deletes staging. This review did not execute that path.

Is this the best way to solve the issue?

No. The bounded native purge is useful, but its publication refusal must occur before consuming the source; a locked preflight is a focused repair.

Full review comments:

  • [P1] Reject excluded archives before consuming the source — internal/store/portable.go:104-105
    With an owner exclusion present, portable export --consume-source first renames the source into staging, then reaches this new rejection through PrunePortablePayloads. Export’s failure cleanup deletes staging, so the command produces no artifact and requires restoring the external backup. Although later consume-source failures intentionally discard staging, this predictable policy refusal should be checked under the exclusive source lock before the rename. Add a consuming-export regression that verifies rejection preserves the source bytes.
    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 db5f6bc22908.

Labels

Label changes:

  • add P0: The new refusal can delete the entire active archive during consuming export without producing a replacement artifact.
  • add merge-risk: 🚨 compatibility: The new publication restriction breaks an established consuming-export workflow after its destructive handoff.
  • add merge-risk: 🚨 other: Active archive deletion and backup-dependent recovery are merge risks outside the specific owned risk categories.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The supplied repository state identifies a member author, so the ordinary contributor runtime-proof gate is exempt. Reported Crabbox make check results cover synthetic purge regressions, not an observed consuming-export refusal. Schema-15 upgrade compatibility is supported by the additive migration and focused preservation test; no runtime execution was performed in this review.

Label justifications:

  • P0: The new refusal can delete the entire active archive during consuming export without producing a replacement artifact.
  • merge-risk: 🚨 compatibility: The new publication restriction breaks an established consuming-export workflow after its destructive handoff.
  • merge-risk: 🚨 other: Active archive deletion and backup-dependent recovery are merge risks outside the specific owned risk categories.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The supplied repository state identifies a member author, so the ordinary contributor runtime-proof gate is exempt. Reported Crabbox make check results cover synthetic purge regressions, not an observed consuming-export refusal. Schema-15 upgrade compatibility is supported by the additive migration and focused preservation test; no runtime execution was performed in this review.

Evidence

Acceptance criteria:

  • [P1] go test ./internal/portable ./internal/store ./internal/cli.
  • [P1] go test ./...

What I checked:

  • Pinned change ownership: The introduced delta is db5f6bc..d4c395d. The verified test merge has that main revision followed by the exact PR head as parents; its export and consume-source implementations are unchanged. (internal/store/portable.go:99, d4c395d822f4)
  • Introduced publication refusal: PrunePortablePayloads now rejects any archive containing owner exclusions. This is the introduced trigger for the consume-source failure. (internal/store/portable.go:104, d4c395d822f4)
  • Export handoff and cleanup: Export moves the source into staging at lines 197–201, reaches PrunePortablePayloads at line 258, and removes staging on an error before artifact commit at lines 162–165. An excluded archive therefore loses its active source path and produces no artifact. (internal/portable/export.go:197, d4c395d822f4)
  • Existing consume-source contract: Consume-source deliberately requires a verified external backup and discards consumed staging after later failures. Existing tests assert that behavior. The concern is the new, predictable policy refusal occurring after handoff; recovery still requires restoring that backup. (docs/portable-stores.md:316, d4c395d822f4)
  • Related proposal: Add owner-directed archive removal and durable exclusions #217 remains open and draft. This PR explicitly narrows and adapts its owner-removal design; the related review identified the same export-order defect, which independent inspection confirms here. Its older schema-13 concern does not describe the latest released baseline.
  • Atomic removal coverage: Apply recomputes the content-bound plan inside its deletion transaction. Tests cover stale content, rollback after a second-target failure, peer preservation, collection suppression, and direct publication refusal; the publication test does not exercise consuming export. (internal/store/thread_exclusions.go:435, d4c395d822f4)

Likely related people:

  • Ayaan Zaidi: Raw commit d99af2e adds internal/portable/consume_unix.go:71 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: d99af2ed7b3b; files: internal/portable/consume_unix.go)
  • Hannes Rudolph: Raw commit 84ed753 adds internal/store/analytics_integrity.go:51 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 84ed753d157f; files: internal/store/analytics_integrity.go)

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.

@steipete
steipete merged commit e8448c1 into main Oct 1, 2026
19 checks passed
@steipete
steipete deleted the worker/round7-purge-threads branch October 1, 2026 13:35
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. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant