Skip to content

Fix targeted archived thread sync#120

Open
robinbraemer wants to merge 1 commit into
openclaw:mainfrom
robinbraemer:codex/targeted-thread-sync
Open

Fix targeted archived thread sync#120
robinbraemer wants to merge 1 commit into
openclaw:mainfrom
robinbraemer:codex/targeted-thread-sync

Conversation

@robinbraemer

Copy link
Copy Markdown

Summary

  • resolve requested channel/thread ids directly with GET /channels/{id} before falling back to broad thread-catalog discovery
  • prevent targeted messages --sync --channel <archived-thread> and sync --channels <archived-thread> from crawling unrelated guild thread parents
  • add a regression for syncing an archived thread id that is not already present in the local channel table

Minekube reproduction

Before this change, targeted sync for archived support thread 1522179061521186937 hung and cancelled while waiting on unrelated guild thread endpoints, e.g. channels/1067106460674314331/threads/active.

With this branch built locally:

perl -e 'alarm shift; exec @ARGV' 45 /tmp/discrawl-targeted-thread-sync \\\n  messages --sync \\\n  --guild 633708750032863232 \\\n  --channel 1522179061521186937 \\\n  --limit 200 \\\n  --include-empty\n```\n\nResult: completed in about 2 seconds, synced exactly one channel, wrote 2 messages, and did not crawl unrelated thread catalogs.\n\nThe Minekube support audit then passed:\n\n```text\nactionable_missing=0\nfatal_inaccessible_catalogs=0\nfatal_truncated_catalogs=0\nstatus=ok\n```\n\n## Verification\n\n- `go test ./internal/syncer -run 'TestSyncChannelSubsetFetchesRequestedArchivedThreadDirectly|TestSyncChannelSubsetExpandsRequestedForumThreads' -count=1`\n- `go test ./...`\n- `git diff --check`\n- local live targeted sync against Minekube archived support thread `1522179061521186937`\n

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed July 7, 2026, 11:44 AM ET / 15:44 UTC.

Summary
The branch adds direct GET /channels/{id} resolution for unresolved requested channel/thread IDs, updates the syncer client interface and fakes, adds an archived-thread regression test, and edits CHANGELOG.md.

Reproducibility: yes. Current main source shows unresolved requested thread IDs falling through to a broad parent-thread catalog crawl, and the PR body gives a concrete Minekube live before/after path for an archived thread ID.

Review metrics: 2 noteworthy metrics.

  • Diff surface: 129 added, 0 removed across 7 files. The functional change is small and additive, but one touched file is release-owned and should be removed from the branch.
  • Client API surface: 1 internal syncer Client method added. The new direct channel lookup requires the Discord adapter and syncer test fakes to stay aligned.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
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:

  • Remove the CHANGELOG.md hunk; keep the release-note context in the PR body or commit message.

Risk before merge

  • [P1] CHANGELOG.md is release-owned and is still edited by this normal PR; that hunk should be removed before merge.
  • [P1] I did not run the Go test suite in this read-only review, so validation rests on source inspection plus the PR body's reported Go and live runs.

Maintainer options:

  1. Decide the mitigation before merge
    Land the direct archived-thread lookup and regression coverage after removing the CHANGELOG.md hunk, keeping release-note context in the PR body or commit message.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] A narrow automated repair can remove only the release-owned CHANGELOG.md hunk without changing the functional sync fix.

Security
Cleared: The diff adds one Discord read API wrapper and tests, with no dependency, workflow, secret-handling, or supply-chain changes.

Review findings

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:5-9
Review details

Best possible solution:

Land the direct archived-thread lookup and regression coverage after removing the CHANGELOG.md hunk, keeping release-note context in the PR body or commit message.

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

Yes. Current main source shows unresolved requested thread IDs falling through to a broad parent-thread catalog crawl, and the PR body gives a concrete Minekube live before/after path for an archived thread ID.

Is this the best way to solve the issue?

Yes for the functional fix: direct channel resolution before broad discovery is the narrow path for an explicit archived thread target. The changelog edit is not part of that solution and should be removed.

Full review comments:

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:5-9
    This is still present from the previous ClawSweeper review. OpenClaw release notes are release-owned, so normal PRs should keep this context in the PR body or commit message instead of adding a CHANGELOG.md hunk.
    Confidence: 0.9

Overall correctness: patch is correct
Overall confidence: 0.86

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 9e8a981f3956.

Label changes

Label justifications:

  • P2: The PR addresses a real targeted archived-thread sync bug with limited blast radius in a CLI sync path.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix live-output proof from a Minekube targeted archived-thread sync plus an audit result showing status=ok.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix live-output proof from a Minekube targeted archived-thread sync plus an audit result showing status=ok.
Evidence reviewed

Acceptance criteria:

  • [P1] git diff --check.
  • [P1] go test ./internal/syncer -run 'TestSyncChannelSubsetFetchesRequestedArchivedThreadDirectly|TestSyncChannelSubsetExpandsRequestedForumThreads' -count=1.

What I checked:

  • Repository policy check: No AGENTS.md exists inside the target repository checkout, so no repo-local AGENTS policy applies. (9e8a981f3956)
  • Current main fallback behavior: Current main resolves requested channels from stored/top-level data, then crawls all thread-capable top-level parents when requested IDs remain unresolved, matching the broad-catalog behavior reported for uncached archived thread IDs. (internal/syncer/channel_catalog.go:62, 9e8a981f3956)
  • PR direct lookup implementation: The PR inserts appendDirectRequestedChannels before the broad thread-parent fallback and rejects a fetched channel if Discord reports it belongs to a different requested guild. (internal/syncer/channel_catalog.go:62, 9c10d697dc7e)
  • Regression coverage: The new test syncs an archived thread absent from the local channel table, asserts one direct channel lookup, and asserts no active or archived thread catalog calls are made. (internal/syncer/channel_catalog_test.go:126, 9c10d697dc7e)
  • Release-owned file still changed: The current PR head still adds a CHANGELOG.md entry under 0.11.5 - Unreleased, which is the same release-owned hunk flagged in the prior ClawSweeper review. (CHANGELOG.md:5, 9c10d697dc7e)
  • Real behavior proof: The PR body reports a local live Minekube targeted archived-thread sync completing in about 2 seconds with one channel, two messages, no unrelated catalog crawl, and an audit result of status=ok. (9c10d697dc7e)

Likely related people:

  • steipete: GitHub commit metadata shows this handle introduced the sync engine, targeted sync scoping, and targeted forum sync behavior that define the affected code path. (role: feature-history owner; confidence: high; commits: 26f9d0370520, ae5f51e4329b, 6f514415900d; files: internal/syncer/channel_catalog.go, internal/syncer/syncer.go, internal/discord/client.go)
  • solomonneas: GitHub commit metadata shows this handle added nearby archived-thread 403 tolerance coverage in the same thread-catalog reliability area. (role: adjacent area contributor; confidence: medium; commits: ea097000d10b; files: internal/syncer/channel_catalog_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.
Review history (1 earlier review cycle)
  • reviewed 2026-07-04T13:04:48.203Z sha 9c10d69 :: needs changes before merge. :: [P3] Remove the release-owned changelog entry

@robinbraemer
robinbraemer force-pushed the codex/targeted-thread-sync branch from c27e7ae to 9c10d69 Compare July 4, 2026 12:59
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. P2 Normal priority bug or improvement with limited blast radius. labels Jul 4, 2026
@clawsweeper

clawsweeper Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: Fix targeted archived thread sync This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant