Skip to content

feat: add external archive provider sync - #86

Draft
steipete wants to merge 2 commits into
mainfrom
agent/external-archive-provider
Draft

feat: add external archive provider sync#86
steipete wants to merge 2 commits into
mainfrom
agent/external-archive-provider

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

Summary

  • add configured external archive providers through sync --source provider:<name>
  • ingest provider catalogs, messages, and checkpoints transactionally with workspace validation and source precedence
  • isolate scoped and bounded checkpoints, preserve historical event consistency, and optimize unchanged provider replays
  • document the JSONL protocol, configuration, retention, failure, and security contracts

Why

Existing Slack archives should be importable without coupling Slacrawl to another tool's database schema. The provider protocol keeps that boundary explicit while retaining Slacrawl's canonical storage and checkpoint behavior.

Safety

  • requires an absolute executable path
  • starts providers with a minimal environment plus an explicit allowlist
  • rejects cross-workspace records and unsafe source-rank ties
  • promotes completed full checkpoints only after valid output and a successful process exit

Verification

  • go test -count=1 ./...
  • go vet ./...
  • deadcode -test ./...
  • gofmt -l .
  • CI-equivalent CLI build and control-surface smoke test
  • go mod tidy with no dependency changes

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. feature: ✨ showcase ClawSweeper spotlight: unusually compelling feature idea for maintainer attention. 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: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 10, 2026
@clawsweeper

clawsweeper Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 31, 2026, 8:26 PM ET / August 1, 2026, 00:26 UTC.

ClawSweeper review

What this changes

The branch adds sync --source provider:&lt;name> to import external Slack archives from a configured local JSONL subprocess into Slacrawl’s canonical SQLite archive.

Merge readiness

⚠️ Ready for maintainer review - 6 items remain

This draft collaborator PR is still necessary: current main has no external archive-provider source, while the branch adds a new permanent configuration and JSONL subprocess contract. The implementation appears deliberate and extensively covered in source tests, but accepting trusted local executables and a core provider protocol requires maintainer product and security-boundary approval before merge.

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

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The branch is a coherent, well-tested draft, but the broad new protocol, execution boundary, and upgrade behavior still need maintainer validation.
Proof confidence 🌊 off-meta tidepool Not applicable: The author is a repository collaborator, so the external-contributor proof gate does not apply; the PR body lists automated checks but provides no inspectable live provider-import artifact in the supplied context.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The author is a repository collaborator, so the external-contributor proof gate does not apply; the PR body lists automated checks but provides no inspectable live provider-import artifact in the supplied context.
Evidence reviewed 5 items Current main does not implement the requested source: Current main accepts API, desktop, MCP, and all sync sources; it has no provider source parsing, provider configuration, or provider package.
Branch adds a real core execution boundary: The proposed provider sync starts the configured absolute executable without a shell and supplies a deliberately filtered environment; this is a new trusted-local-code contract rather than an existing integration being repaired.
Branch documents the permanent provider contract: The documentation defines provider naming, absolute executable paths, explicit environment allowlisting, source priority, checkpoint behavior, and the JSONL protocol, confirming that this is a new user-facing feature surface.
Findings None None.
Security Needs attention Confirm the trusted-provider execution boundary: Configured provider commands are launched as local processes and retain the caller’s filesystem and network permissions; the docs disclose this, but maintainers must explicitly accept that security contract before merging.

How this fits together

Slacrawl’s sync command collects Slack data from API, desktop, and MCP sources into a local SQLite archive with full-text search. This PR adds an explicit provider source that sends sync scope to a configured local executable, validates its JSONL records, and writes catalog data, messages, and checkpoints into that same archive.

flowchart LR
  A[Slacrawl configuration] --> B[Sync command]
  B --> C[Selected provider source]
  C --> D[Trusted local executable]
  D --> E[Validated JSONL records]
  E --> F[Transactional SQLite writes]
  F --> G[Checkpoints and FTS index]
  G --> H[Search and archive tools]
Loading

Decision needed

Question Recommendation
Should Slacrawl’s core permanently support configured external archive providers through the proposed trusted local JSONL subprocess protocol? Sponsor the provider protocol: Accept the explicit trusted-provider model, then require an end-to-end import and representative archive-upgrade validation before converting the draft into a merge candidate.

Why: The patch adds new configuration, a public protocol, and an intentional local-code execution boundary; source review can verify safeguards, but cannot determine whether that ongoing product and security contract belongs in core.

Before merge

  • Resolve security concern: Confirm the trusted-provider execution boundary - Configured provider commands are launched as local processes and retain the caller’s filesystem and network permissions; the docs disclose this, but maintainers must explicitly accept that security contract before merging.
  • Resolve merge risk (P1) - Merging creates a durable [[providers]] and JSONL protocol contract that third-party archive adapters may depend on, so future compatibility expectations need an explicit owner.
  • Resolve merge risk (P1) - A configured provider is intentionally trusted local code with the caller’s filesystem and network permissions; filtered environment forwarding does not sandbox it.
  • Resolve merge risk (P1) - The branch changes full-text-index maintenance for existing SQLite archives, so maintainers should confirm upgrade behavior and acceptable rebuild time on a representative existing archive before landing.
  • Complete next step (P2) - A maintainer must decide whether the permanent provider protocol and trusted-subprocess boundary belong in core before any repair or merge automation is appropriate.

Findings

  • [medium] Confirm the trusted-provider execution boundary — internal/provider/provider.go:163
Agent review details

Security

Needs attention: The diff deliberately adds a trusted local-executable boundary with documented environment filtering, but the permanent security model needs maintainer approval.

Review metrics

Metric Value Why it matters
Patch surface 21 files, 2,921 added, 104 removed The feature spans CLI parsing, configuration, provider ingestion, SQLite indexing, tests, and documentation, so review should consider the contract as a whole.

Merge-risk options

Maintainer options:

  1. Approve the trusted-provider contract (recommended)
    Accept the explicit subprocess and JSONL compatibility commitment after reviewing a redacted live import and an upgrade of an existing archive.
  2. Pause the core extension
    Do not merge the feature if maintainers do not want third-party provider compatibility or trusted local executable support in core.

Technical review

Best possible solution:

Keep provider imports explicit and opt-in, land the protocol only after maintainers accept the trusted-executable model and validate one representative existing archive upgrade plus an end-to-end provider run.

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

Not applicable: this PR proposes a new archive-import capability rather than reporting broken existing behavior. Source inspection confirms current main has no provider source to reproduce.

Is this the best way to solve the issue?

Unclear pending maintainer direction: the implementation is a coherent explicit-provider design, but only maintainers can decide whether its permanent subprocess and compatibility contract should live in core.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 53dd8d09b584.

Labels

Label changes:

  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The author is a repository collaborator, so the external-contributor proof gate does not apply; the PR body lists automated checks but provides no inspectable live provider-import artifact in the supplied context.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This is a substantial but opt-in feature proposal with bounded user impact and no demonstrated live regression.
  • merge-risk: 🚨 compatibility: The new provider configuration, provider:<name> source, JSONL protocol, checkpoint behavior, and FTS maintenance become upgrade and integration contracts.
  • merge-risk: 🚨 security-boundary: Configured provider executables run as trusted local code with the caller’s filesystem and network permissions despite environment filtering.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The author is a repository collaborator, so the external-contributor proof gate does not apply; the PR body lists automated checks but provides no inspectable live provider-import artifact in the supplied context.

Evidence

Security concerns:

  • [medium] Confirm the trusted-provider execution boundary — internal/provider/provider.go:163
    Configured provider commands are launched as local processes and retain the caller’s filesystem and network permissions; the docs disclose this, but maintainers must explicitly accept that security contract before merging.
    Confidence: 0.96

What I checked:

  • Current main does not implement the requested source: Current main accepts API, desktop, MCP, and all sync sources; it has no provider source parsing, provider configuration, or provider package. (internal/syncer/syncer.go:27, 53dd8d09b584)
  • Branch adds a real core execution boundary: The proposed provider sync starts the configured absolute executable without a shell and supplies a deliberately filtered environment; this is a new trusted-local-code contract rather than an existing integration being repaired. (internal/provider/provider.go:163, f3840362deaa)
  • Branch documents the permanent provider contract: The documentation defines provider naming, absolute executable paths, explicit environment allowlisting, source priority, checkpoint behavior, and the JSONL protocol, confirming that this is a new user-facing feature surface. (docs/configuration.md:190, f3840362deaa)
  • Branch remains ahead of current main: The PR head is a two-commit descendant of the checked-out main SHA, so its central feature has not been merged or released from this branch. (internal/provider/provider.go:125, f3840362deaa)
  • Adjacent feature ownership: Current source history attributes the existing MCP sync source to Peter Steinberger, and the current sync entry points are likewise attributed to that area of work. (internal/syncer/syncer.go:27, cbab369d8895)

Likely related people:

  • steipete: Introduced the existing MCP sync source and authored the current branch’s provider and replay work; current-main blame also places the sync entry path in this area. (role: recent sync and storage-area contributor; confidence: high; commits: cbab369d8895, f8e942414eb0, f3840362deaa; files: internal/syncer/syncer.go, internal/store/store.go, internal/provider/provider.go)
  • Vincent Koc: Feature history includes Git-backed archive synchronization and multi-workspace sync work that shares the archive ownership and storage boundaries extended here. (role: adjacent archive and storage contributor; confidence: medium; commits: bf82ff0e4ee4, 360ad4c23b39; files: internal/share/share.go, internal/store/store.go, internal/syncer/syncer.go)

Rank-up moves

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

  • Record a redacted end-to-end import from a representative provider executable.
  • Validate opening and searching a representative existing SQLite archive after the FTS alignment rebuild.
  • Obtain explicit maintainer sponsorship for the core provider protocol and trusted-executable model.

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 (18 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-31T01:18:49.491Z sha f384036 :: found issues before merge. :: [P3] Remove the release-owned changelog entries
  • reviewed 2026-07-31T04:05:15.028Z sha f384036 :: found issues before merge. :: [P3] Remove the release-owned changelog entries
  • reviewed 2026-07-31T08:42:43.478Z sha f384036 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T10:30:38.351Z sha f384036 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T13:34:44.031Z sha f384036 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T14:37:22.046Z sha f384036 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T16:01:37.676Z sha f384036 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-31T19:20:35.371Z sha f384036 :: needs maintainer review before merge. :: none

@steipete
steipete force-pushed the agent/external-archive-provider branch from 8dcd80d to f384036 Compare July 27, 2026 16:57
@clawsweeper clawsweeper Bot added 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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. 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. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 27, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat feature: ✨ showcase ClawSweeper spotlight: unusually compelling feature idea for maintainer attention. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. other P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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.

2 participants