chore: full SDK code review — findings catalog - #85
Draft
yaron-thenvoi wants to merge 7 commits into
Draft
Conversation
Triage document at review.md with per-area detail under review/. 5 Blockers, 71 Major, 85 Minor, 38 Nits. Not intended for merge — exists so the team can read and act on the findings via GitHub's rendered markdown. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Brings the review branch up to 1eb7bc9 so the review documents describe the code they are reviewing.
The review was cut at 763734d against @thenvoi/sdk@0.1.4. Since then main received 72 commits, 5 of which touched packages/sdk/src. This brings every finding, citation, and metric up to date with @band-ai/sdk@0.1.10. Rebrand (#150): - 198 legacy-brand occurrences rewritten across the 10 documents, keeping the identifiers the rename deliberately retained on the wire (thenvoiRoomId). - The repo's own c6/c7 brand guards scan tracked files, so the stale documents were failing them; they now pass. Citations: - All 427 file:line citations re-anchored by matching the cited line's text across the two trees rather than trusting line numbers. 76 moved. - 4 ranges in PhoenixChannelsTransport.ts were restructured rather than moved by #80 and were re-read and rewritten by hand. - All intra-review links and heading anchors re-validated. Findings that moved: - B4 (FernBandClientLike mismatch): Blocker -> Major. Resolved by dependency, not code — @band-ai/rest-client 0.0.118 now exposes the agentApi* namespaces the finding called phantom. Residual: 9 legacy namespaces now dead as the first branch of 13 ?? chains. - disconnect() mutating the map being iterated: fixed by #80. - Optional ws dependency typing: obsolete, and its recommendation is now harmful — #80 moved auth to an x-api-key handshake header that the native WebSocket constructor cannot set. - Connect timeout uses manual setTimeout: Major -> Minor, both concrete defects fixed. - Engine-version mismatch: Minor -> Major. node:sqlite does not load on the SDK's own declared Node floor (verified on 22.12). - M8 regressed: error classes outside BandSdkError 6 -> 7, console.warn outside the logger 2 -> 3. - Two premises corrected: assertCapability was never dead, and the B3 abort is guarded rather than unconditional. New findings: - B6: tests/c5-package-symbols.test.ts:420-421 asserts .release-hold exists, which 1eb7bc9 deleted. main has a red test. - Compile-proof tests spawn an extensionless .bin/tsc and do not run off POSIX; their negative assertions pass vacuously when it fails. - Part of the suite requires a prior build and does not say so. - One brand-guard assertion is line-ending sensitive. Checks re-run at 1eb7bc9: tsc --noEmit clean, eslint 0 errors / 12 warnings, vitest 871/898 passing (1 real defect, 20 test-infrastructure problems, 0 product-code regressions — triaged in the refresh log). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Folds the update into the findings themselves so the documents read as a review of the current tree. - Removes the "Refresh log" section and the "Refresh method" note from review.md, and the 36 per-finding refresh annotations. - Where a note only said "unchanged", it is dropped; the finding already said that. - Where a note recorded a real change, its substance moves into the finding's Observation / Impact / Fix in present tense: the nine dead REST namespaces, the AgentRuntime.stop entry guard, the onClose ambient gap, the three stopReconnectIfNoChannels call paths, the memory.ts export gap, the node:sqlite floor, the lint evidence for dead symbols. - B4 is no longer a Blocker, so its stub is removed from the Blockers list and the residual cleanup is filed as M21. - Test-suite state moves to a "Test suite state" section in build-tests-docs.md, where a build-and-tests review should carry it. - "At a glance" and the findings-count table are single current columns. - Two entries stay struck through on purpose, because acting on what they recorded would now cause harm: the ws dependency is load-bearing for handshake-header auth, and disconnect() is already the model pattern. Verified: 251 relative links resolve to existing files and headings, no duplicate heading slugs, 409 file:line citations in range with no inverted ranges, and the repo's c6/c7 brand guards pass on these documents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
B6 said the stale `.release-hold` assertion had gone unnoticed. It had not. On the pull request that deleted the marker, the `test` job concluded failure and annotated `c5-package-symbols.test.ts:421` with the exact assertion; the aggregate `ci-status` check also concluded failure. It was merged with both red. - Reworks B6 to state that, and to settle the fix direction: the commit message calls the marker stale and the merge went ahead, so the marker was deliberately removed and the assertion is what should go. - Adds a Major to build-tests-docs.md for the mechanism: the `main-branch-protection` ruleset is active with no bypass actors but its rules are only `deletion`, `non_fast_forward`, and `pull_request` — there is no `required_status_checks` rule, so a passing PR is not required. `ci.yml` also triggers on `pull_request` only, so the suite never runs against `main` after a merge and the failure stopped being reported. The `ci-status` aggregate job at `ci.yml:232-261` is already written and correct, so requiring it is one ruleset setting rather than new work. - Notes in "Test suite state" that CI results cannot be read as evidence about `main`'s tree. Counts: 200 -> 201 findings; build/tests/docs Major 12 -> 13. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yaron-thenvoi
force-pushed
the
chore/full-sdk-review
branch
from
August 25, 2026 23:41
1bd6143 to
ee38c5d
Compare
B4 was retired when the FernBandClientLike finding was resolved and moved to M21, leaving B1,B2,B3,B5,B6 with a hole in the sequence. Renumbers B5 (zod migration) to B4 and B6 (stale .release-hold assertion) to B5, and updates every heading, anchor, and cross-reference across review.md and review/build-tests-docs.md to match. Verified: all 256 relative links in the review documents still resolve.
Audited every pointer line under every B/M finding in review.md. Found four that didn't lead to a specific paragraph: - B5 had no area-file counterpart at all — its full Problem/Impact/Fix lived only in review.md. Added a proper finding for it in review/build-tests-docs.md (in the Blockers section, ahead of the merge-gate finding it explains the trigger for), and repointed review.md's B5 and the two build-tests-docs.md cross-references at it. - M8 (error-handling consistency) pointed at error-async.md's whole '### Major' section. Replaced with links to its four actual sub-findings: bare throws, stray error classes, dropped-context catches (error-async.md), and REST raw errors (network.md). - M14 (JSDoc) linked only the first of its three sub-findings (adapters). Added the other two (Logger/errors/runtime-config, contracts/protocols). - M15 (documentation drift) pointed at build-tests-docs.md's whole '### Major' section. Replaced with links to its five actual sub-findings (two CHANGELOG, three README/examples). Also dropped M10's secondary 'cast inventory' pointer, which resolved to the same generic section with no specific paragraph behind it — its primary link was already specific, so the secondary added a dead end with no content. Verified: every one of the 26 B/M findings now carries at least one pointer to a specific paragraph (not a section header), and all 268 relative links in the review documents resolve.
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
1 participant
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.
Overview
This PR adds a comprehensive code review of
packages/sdk/(@band-ai/sdk@0.1.10, reviewed atmain@1eb7bc9) — 201 findings across 9 areas (4 Blockers, 74 Major, 85 Minor, 38 Nits), with a top-level triage document linking to per-area detail files.This PR is not for merge. It exists so the team can read, discuss, and act on the findings. Each finding lives in a markdown file that GitHub renders inline; treat the PR as a viewing surface, not a code change.
mainis merged into the branch, so the diff is the review documents only — no source changes.How to read it
Read
review.mdin full — it includes detailed instructions on how the review is organized, what each area file contains, and how to navigate between the triage layer (the main file) and the per-finding detail (the files underreview/).Worth looking at first
One finding is not a design opinion but a live defect, and it is cheap to fix:
tests/c5-package-symbols.test.ts:420-421asserts a file that HEAD deleted.1eb7bc9removed.release-hold; the assertion that it exists was not removed with it, so the suite fails onmain's tree. CI reported this precisely on the PR that made the change —testred,ci-statusred, annotation naming line 421 — and it merged anyway.main-branch-protectionruleset requires a pull request but has norequired_status_checksrule, andci.ymltriggers onpull_requestonly so the suite never re-runs on the trunk. Theci-statusaggregate job already exists and is correct (ci.yml:232-261), so requiring it is one setting rather than new work.What was checked
packages/sdk/src/tree (every file read; counts grounded in direct text searches).tsc --noEmit— passes cleanly with peers installed, zero errors.eslint .— 0 errors, 12 warnings, several of which correspond to findings.vitest run— 898 tests across 96 files: 871 pass, 23 fail, 4 skipped. The failures are triaged inreview/build-tests-docs.md: 20 are test-infrastructure problems (16 of them one Windows portability bug), 2 are marginal timeouts, and 1 is the genuine.release-holdassertion above. No product-code regressions.file:linecitation in these documents was verified to point at the line it describes.What wasn't checked
Findings count