fix(api): skip unsupported block-children errors instead of aborting sync#60
Open
oliver-mee wants to merge 1 commit into
Open
fix(api): skip unsupported block-children errors instead of aborting sync#60oliver-mee wants to merge 1 commit into
oliver-mee wants to merge 1 commit into
Conversation
…sync
GET /blocks/{id}/children returns a 400 validation_error when a page
contains a block type the integration cannot receive (seen with Notion
AI blocks: "Block type ai_block is not supported via the API for your
bot type."). This aborted the entire sync --source api run, regardless
of how many other pages were left to sync.
Classify this specific 400 as non-fatal: skip the unfetchable children
batch, record a warning, and continue. The page_blocks "complete" sync
marker and RetireSourcePageBlocksNotSyncedAt are both now gated on zero
warnings, so a page that synced cleanly before does not have its
previously-synced blocks retired the next time it also contains one
unfetchable block.
Adds isUnsupportedBlockChildrenError and threads a []string warnings
return through walkBlocksAt -> walkBlocks -> ingestPage -> Sync,
reusing the existing Summary.Warnings / writeAPIWarnings stderr path
already used for the forbidden-/users and restricted-resource cases.
Covered by three new tests:
- TestWalkBlocksSkipsUnsupportedBlockChildren
- TestIngestPageLeavesIncompleteWhenBlockChildrenUnsupported
- TestWalkBlocksDoesNotRetirePreviousBlocksOnPartialWalk
Fixes openclaw#59
|
Codex review: needs maintainer review before merge. Reviewed July 2, 2026, 9:49 AM ET / 13:49 UTC. Summary Reproducibility: yes. from source: current main returns any /blocks/{id}/children API error from walkBlocksAt through ingestPage and Sync, matching the linked ai_block 400 report. I did not reproduce against a live Notion workspace in this review. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this PR if maintainers accept the targeted warning predicate and normal Go checks pass; keep the change scoped to the API ingestion bug tracked at #59. Do we have a high-confidence way to reproduce the issue? Yes from source: current main returns any /blocks/{id}/children API error from walkBlocksAt through ingestPage and Sync, matching the linked ai_block 400 report. I did not reproduce against a live Notion workspace in this review. Is this the best way to solve the issue? Yes; the PR uses a narrow validation_error predicate, preserves warning visibility through the existing Summary.Warnings path, and avoids marking or retiring page blocks after a partial walk. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against 7754b9d416df. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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
|
2 tasks
clawsweeper
Bot
added
proof: sufficient
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.
Summary
GET /blocks/{id}/childrenreturns a 400validation_errorwhen a page contains a block type the API cannot return to this integration (seen with Notion AI blocks: "Block type ai_block is not supported via the API for your bot type."). That error propagated unhandled and aborted the entiresync --source apirun, the same failure shape as #37 and #39 but a different Notion block type triggering it.This classifies that specific 400 as non-fatal: skip the unfetchable children batch, record a warning (surfaced the same way existing warnings already are, via
Summary.Warnings/writeAPIWarningson stderr), and keep syncing the rest of the workspace. It also gates thepage_blocks"complete" marker andRetireSourcePageBlocksNotSyncedAton zero warnings: without that gate, a page that synced cleanly before would have its already-synced blocks retired the next run it also happens to contain one unfetchable block. An independent review pass (Claude Opus 4.8) caught that specific regression before this PR was opened, see Model used below.Linked issue
Fixes #59
Type of change
How to test
GOWORK=off go test ./...,go vet ./...,gofmt -l .all clean, including the 3 new tests (TestWalkBlocksSkipsUnsupportedBlockChildren,TestIngestPageLeavesIncompleteWhenBlockChildrenUnsupported,TestWalkBlocksDoesNotRetirePreviousBlocksOnPartialWalk).go build ./cmd/notcrawl, then runnotcrawl sync --source apiagainst a workspace with anai_blockpresent. I ran this against my own production workspace (7,573 pages, one page with anai_block) and confirmed the patched binary gets past the exact page the unpatched binary aborted on, printingwarning: Skipped children of block ... on page ...to stderr instead of dying.Checklist
Model used
Claude Sonnet 5 implemented this. A separate Claude Opus 4.8 pass independently reviewed the diff and caught a real bug: the first version only gated
SetSyncState(..., "complete")on the warnings, missing thatRetireSourcePageBlocksNotSyncedAtneeded the same gate, or a previously-clean page would lose its valid blocks the next time it also hit one unfetchable block. That fix is included here.🤖 Generated with Claude Code (Claude Sonnet 5)
🧑💻 Ideated, directed and reviewed by a human, @Omee11