fix: triage pass — cross-workspace collisions, attachment loss, and text width - #131
Merged
Conversation
The #130 fix covered channels but left two boundaries fatal. A user shared across workspaces (Enterprise Grid IDs are org-wide, and Slack Connect surfaces external members) aborted Sync in the user loop, after channels, DMs and every message had already committed but before the checkpoint was written, so the run was recorded as never having happened. A message in a shared channel owned by another workspace propagated out of HandleEventsAPIEvent and terminated the socket-mode loop, killing tail. Demote both to skip-with-warning, matching the desktop importer, and cover the history and thread upserts on the same path.
Three defects in the attachment path: - readLimit: MaxBytes+1 overflowed to negative for math.MaxInt64, the natural way to ask for no cap. io.LimitReader then returned EOF at once, so every file was stored as a successfully fetched empty file. - A symlinked media root is a supported setup for a large cache on another volume, and fetch always wrote through one, but every read path rejected it, so purge and publish failed permanently. Resolve the root once and keep refusing symlinks inside the tree. - publish treated an unsafe media path as an absent file and could emit a manifest claiming no media at all, wiping attachments for subscribers. Also clamp fetch errors on a rune boundary and keep the extension for fully non-Latin filenames, which safeFilename strips to bare 'png'.
trimTo sliced bytes, so a message containing emoji or CJK was cut mid-rune into invalid UTF-8 and wide text was truncated far earlier than the limit implies. renderTable measured widths on the already-colorized cell, and formatScalar wraps empty, nil and bool values in ANSI codes, so those invisible bytes padded the column while the header used its plain length. Promote go-runewidth to a direct dependency; it was already in the module graph via bubbletea.
The tombstone-propagation SQL existed in both the v6 migration and the share import path and had already drifted: the migration overwrote deletion_reason unconditionally while the import preserved an existing value. store.BackfillDeletedSubordinates now owns the statement with the preserving semantics and both callers use it. media.LocalPath/RepoPath were byte-identical twins, and share duplicated the symlink-rejecting containment walk that media.cachedRegularFile implements. One containedJoin plus an exported media.VerifiedFile now carry both invariants; the share-local copy and its error sentinel are gone. Also drop the dead schemaPragmas const, a dead store flagged by staticcheck, and stamp Docker builds with a real version.
ExtractMentions html-unescaped the whole message before matching, so a user literally typing <@u123> — which Slack transmits as <@u123> — became indistinguishable from a real mention and landed in message_mentions across every ingest path. normalizeMessageText already parses tokens before entity decoding (ac6d350) for exactly this reason; mentions were left behind. Match on the escaped text and unescape only the display label. Flips a test that asserted the buggy behavior.
ChannelThreadRoots' reply-existence probe had no index on thread_ts, so it scanned the whole channel per message — O(channel-size squared), measured at minutes-to-hours for a 50k-message channel and 0.04s with the index. message_events had no (channel_id, ts) index either, making per-message event deletes a full scan of the event log. Schema v7 adds both; migration verified against a copy of a real v6 archive. replaceUserMentions recompiled the same per-target regex for every rendered row; the compiled patterns are now cached.
The hourly repair sweep made plain HTTP calls whose failures were fatal: one network blip during the tick killed a long-running tail daemon with a nonzero exit. Repair failures now warn and retry next interval; store errors still surface through the event-handler path. The socket-mode goroutine ran slack-go's Run(), which is RunContext(context.TODO()) — the websocket never observed Tail's context and outlived it. The runner interface now takes ctx.
… cap - main now installs signal.NotifyContext: Ctrl-C/SIGTERM cancel the context so sync/tail/watch unwind through their cleanup paths instead of being hard-killed; all the existing ctx plumbing was unreachable. - runTailTargets raced a buffered error against the canceled context in one select; a scratch -race harness showed 14% of failures reported 'context canceled' instead of the real workspace error. The error channel is now drained with priority. - sync --since documents 'slack ts or RFC3339' but only the MCP backend normalized RFC3339; the API path passed the raw string to Slack's oldest param. Normalized once at the CLI boundary, with a clear error for junk values. - analytics trends --weeks materializes one bucket per week per channel row, so an absurd count was an OOM kill; capped at 520.
mcpclient called cmd.Wait concurrently with the stdout read loop; Wait closes the pipe, so a server that writes its final response and exits immediately could have that response turned into a decode error. Wait now runs after the reader drains, with a bounded post-kill timeout so a grandchild holding the pipe cannot hang Close. The provider request write also moved to a goroutine — a request larger than a pipe buffer against a provider that emits early output could deadlock both processes — and a provider that dies before reading its request now gets its captured stderr attached to the error instead of a bare EPIPE.
Collaborator
Author
|
Pushed a second wave from a full-codebase deep review (4 parallel review passes: bugs, performance, duplication, interactive/live surfaces — every finding verified before fixing): Bugs
Performance
Dedupe
Proof: full suite, Known follow-ups deliberately not in this PR: batching the per-message transactions in the non-provider ingest paths (~3.4x measured headroom), the double-pass point queries in |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Codex review: needs changes before merge. Reviewed August 6, 2026, 7:38 PM ET / 23:38 UTC. ClawSweeper reviewWhat this changesThe PR fixes multi-workspace Slack sync collisions, media-cache data loss paths, Unicode CLI rendering, cancellation/subprocess handling, and archive lookup performance. Merge readinessKeep this collaborator PR open: current main still has the user-collision failure, and the branch adds a coherent set of regression-tested reliability fixes. The functional patch appears correct; remove the release-owned changelog edits before merge. Priority: P2 Review scores
Verification
How this fits togetherSlacrawl ingests Slack workspaces into a local SQLite archive, then serves that archive through sync, tail, media, search, share, and CLI commands. The changed code handles incoming Slack data, archive persistence, cached attachments, and command output. flowchart LR
A[Slack workspaces] --> B[Sync and tail ingestion]
B --> C[Archive store]
C --> D[Media cache]
C --> E[Search and CLI output]
C --> F[Share export]
D --> F
Before merge
Findings
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. Technical reviewBest possible solution: Remove the release-owned changelog entries, then land the tested reliability fixes after normal maintainer review. Do we have a high-confidence way to reproduce the issue? Yes, by seeding a user or message under another workspace and running bot sync or event ingestion; current main’s direct user upsert establishes the failing path and the branch adds focused regression tests. Is this the best way to solve the issue? Yes, after removing the release-owned changelog edits: the fixes preserve existing collision integrity while handling legitimate shared-workspace inputs at the ingestion boundary. Full review comments:
Overall correctness: patch is correct AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against 2fdce12b5057. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What 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
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Triage pass over
main. Eight confirmed defects, each with a regression test that fails without the fix.Sync: cross-workspace collisions (completes #130)
#130 demoted cross-workspace channel collisions to a skip, but two other boundaries stayed fatal:
Sync's user loop calledUpsertUserbare. Enterprise Grid user IDs are org-wide and Slack Connect surfaces external members, so a second workspace's sync died after channels, DMs and every message had committed but before the checkpoint was written — the run was recorded as never having happened.tail. The upsert error propagated out ofHandleEventsAPIEventthroughhandleSocketModeEventtoTail, terminating the socket-mode loop. fix: skip cross-workspace channel collisions during bot sync #130 made this more likely, since the shared channel now permanently stays owned by the first workspace.Both are now skip-with-warning, matching the desktop importer. The history and thread upserts on the same path are covered too.
Media: silent attachment loss
MaxBytes+1overflowed.math.MaxInt64is the natural "no cap" value; incrementing it for the over-limit probe wrapped negative,io.LimitReaderreturned EOF immediately, and every attachment was written as a successfully fetched empty file —sha256(""), size 0, statusfetched, no error anywhere.purgepermanently. Parking a large attachment cache on another volume behind a symlink is ordinary, andfiles fetchhas always written through one — but every read path (list,remove,share) rejected it.purge --forcecommitted the database transaction and then failed post-commit cleanup on every subsequent run. The root is now resolved once; symlinks inside the tree are still refused, and the existing escape test still passes.publishcould emit a media-less manifest.errUnsafeMediaPathwas treated as "file absent", so after the repo'smedia/had already been wiped, every row was skipped and the manifest simply claimed the archive has no attachments — silently losing the entire media set for subscribers on their next--mediaimport.clearUnmanifestedImportedFileMediaswallowed every non-ErrNoRowsquery error.CLI: byte lengths used where display width was meant
trimTosliced bytes, sosearch/messagesoutput cut multi-byte runes in half and emitted invalid UTF-8; CJK text was also truncated to roughly a third of the intended width.renderTablemeasured column widths on the colorized cell.formatScalarwraps empty,niland bool values in ANSI codes, so those invisible bytes padded the column while the header used its plain length — every table with an empty or non-ASCII cell was misaligned.写真.png) lost their extension entirely, so a publishedmedia/tree served them asapplication/octet-stream.Proof
go build,go test ./...,go test -race ./...,go vet,govulncheck(no vulnerabilities),make lint,make smoke,make snapshot,make tidy-check fmt-check— all clean. Codex autoreview: clean, no accepted findings.Each fix was verified to fail before the change: the two sync tests, both render tests, and the media tests were run against the reverted logic and observed failing.
One existing test changed rather than being deleted:
TestPurgeCommandReportsPostCommitCleanupFailureused a symlinked media root as its failure trigger, which is exactly the wedge being fixed. It now points at a genuinely broken cache (a root symlinked to a regular file) and still asserts that post-commit cleanup failure is reported.go-runewidthmoves from indirect to direct; it was already in the module graph via bubbletea, same version.