-
Notifications
You must be signed in to change notification settings - Fork 1
Codex/sdk 0 2 34 full audit #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughUpgrades Anthropic Claude Agent SDK to v0.2.34, renames admin-only tool Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkerPool as Worker-Pool
participant Agent as Claude-Agent-SDK
Note over Client,WorkerPool: Request includes permissionMode
Client->>WorkerPool: startTask(permissionMode)
alt permissionMode == "bypassPermissions"
WorkerPool->>Agent: query(..., allowDangerouslySkipPermissions: true)
else
WorkerPool->>Agent: query(...)
end
Agent-->>WorkerPool: response
WorkerPool-->>Client: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/web/package.json`:
- Line 42: package.json currently lists `@hookform/resolvers`@^5.2.2 alongside
zod@^4.1.5 which is incompatible; update dependencies so zod and the resolver
match: either downgrade zod to a v3 release (e.g. ^3.25.0) or upgrade
`@hookform/resolvers` to a version that supports Zod v4, then run install and
rebuild; update any imports/uses in SubdomainDeployForm.tsx that call
zodResolver to match the chosen resolver version and ensure TypeScript types
resolve correctly.
In `@docs/reports/sdk-upgrades/anthropic-sdk-0.2.34-audit.md`:
- Around line 6-9: The audit file contains committed absolute local filesystem
paths (e.g. "/Users/here/.codex/worktrees/61f8/...") that leak environment
details; open docs/reports/sdk-upgrades/anthropic-sdk-0.2.34-audit.md, find the
string occurrences like `/Users/here/.codex/worktrees/61f8/alive/...` and
replace them with relative repo paths (for example `alive/package.json`,
`apps/web/package.json`, `packages/tools/package.json`,
`packages/worker-pool/package.json`) or a sanitized placeholder (e.g.
`<repo-root>/alive/...`) so the document no longer contains user-specific
absolute paths; apply the same replacement for the other occurrences later in
the file.
In `@docs/reports/sdk-upgrades/anthropic-sdk-0.2.34-changelog-span.md`:
- Line 47: The changelog line contains a typo ("messsages"); update the text in
the docs entry that currently reads "Fixed structured outputs to handle empty
assistant messsages" to "Fixed structured outputs to handle empty assistant
messages" so the changelog and any references (the changelog line string) use
the correct spelling.
- Around line 60-67: Remove the duplicate bullet about the
CLAUDE_CODE_ENABLE_TASKS env var from the 0.2.20 section and keep it only under
0.2.19; in the 0.2.20 block (the lines mentioning
CLAUDE_CODE_ADDITIONAL_DIRECTORIES_CLAUDE_MD and parity with Claude Code
v2.1.20) leave only the additionalDirectories/CLAUDE.md support and the parity
note, ensuring the env var CLAUDE_CODE_ENABLE_TASKS appears solely in the 0.2.19
changelog entry.
In `@templates/site-template/user/package.json`:
- Line 17: The package.json currently lists "@hookform/resolvers": "^5.2.2"
which has a strict peer on react-hook-form@7.55.0; update the react-hook-form
entry to exactly "7.55.0" (or alternatively downgrade `@hookform/resolvers` to a
compatible v5 release that matches your installed react-hook-form) so the peer
dependency aligns and avoids pnpm strict-resolution warnings—look for the
"react-hook-form" and "@hookform/resolvers" entries in package.json and make the
versions consistent across those entries (also apply the same change wherever
react-hook-form is declared in this template).
docs/reports/sdk-upgrades/anthropic-sdk-0.2.34-changelog-span.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/reports/sdk-upgrades/anthropic-sdk-0.2.34-changelog-span.md`:
- Around line 181-184: Update the parity note under the "## 0.1.65" heading:
change the line that reads "Updated to parity with Claude Code v2.0.66" to
"Updated to parity with Claude Code v2.0.65" so the stated Claude Code version
matches the 0.1.65 entry (locate the "## 0.1.65" header and the following parity
bullet to make the edit).
🧹 Nitpick comments (3)
docs/reports/sdk-upgrades/anthropic-sdk-0.2.34-audit.md (2)
24-24: Hyphenate “task system” consistently.Consider “task-system” → “task-system” (already hyphenated) but “compacting related” could read smoother as “compacting-related,” and the line currently reads a bit clunky. Small wording polish for clarity.
50-51: Minor grammar tweak for clarity.“due Unix socket” → “due to Unix socket” to improve readability.
docs/reports/sdk-upgrades/anthropic-sdk-0.2.34-changelog-span.md (1)
126-134: Consider consistent date formatting across all version entries.Only versions 0.2.0 (line 126) and 0.1.77 (line 131) include release dates, while all other entries omit them. For improved consistency and easier reference, consider either:
- Adding dates to all version entries, or
- Documenting why dates are included only for certain versions (e.g., major releases)
This would help readers quickly understand the release timeline across the full version span.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/reports/sdk-upgrades/anthropic-sdk-0.2.34-changelog-span.md`:
- Around line 1-196: The changelog is missing entries for published SDK
versions; add minimal entries for versions 0.1.62, 0.1.70, 0.1.76, 0.2.1, 0.2.2,
0.2.13, 0.2.24, and 0.2.28 by inserting new "## <version>" headings (e.g., "##
0.1.62") with a short bullet like "- Updated to parity with Claude Code vX.Y.Z"
or a brief note describing the change (if known), keeping formatting consistent
with existing headings such as "## 0.2.34" and surrounding entries like "##
0.1.63" and "## 0.2.31".
🧹 Nitpick comments (1)
docs/reports/sdk-upgrades/anthropic-sdk-0.2.34-changelog-span.md (1)
1-196: Consider adding release dates for other versions.Only versions 0.2.0 and 0.1.77 include release dates. Adding dates to the other versions would improve the changelog's utility for understanding the release timeline and evaluating upgrade urgency.
FlowgladProvider's bundled @tanstack/react-query creates a duplicate QueryClientProvider context. When the provider is skipped (E2E tests, unauthenticated users), useBilling() throws "No QueryClient set" and crashes the entire settings page. - Add BillingErrorBoundary + SafeBilling wrapper in GeneralSettings - Skip FlowgladProvider for Playwright tests (window.PLAYWRIGHT_TEST) - Mock billing/user/tokens APIs in settings E2E test - Use gotoChat instead of gotoChatFast for reliable navigation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds placeholder entries for 0.1.62, 0.1.70, 0.1.76, 0.2.1, 0.2.2, 0.2.13, 0.2.24, 0.2.28 as flagged by CodeRabbit review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/components/settings/tabs/GeneralSettings.tsx`:
- Around line 83-90: SafeBilling currently passes children(BILLING_NOT_LOADED)
as the BillingErrorBoundary fallback which renders outside the protected
subtree, so a throw inside the render-prop would escape the boundary; change the
fallback to a safe static node (e.g., null or a minimal placeholder) or wrap the
fallback render-prop call in its own error boundary so both paths are protected
— update SafeBilling to ensure BillingErrorBoundary's fallback does not directly
invoke children, or wrap children(BILLING_NOT_LOADED) in an additional
BillingErrorBoundary (references: SafeBilling, BillingErrorBoundary,
BillingProvider, BILLING_NOT_LOADED, children).
🧹 Nitpick comments (2)
apps/web/components/settings/tabs/GeneralSettings.tsx (2)
36-60: Consider freezingBILLING_NOT_LOADEDto prevent accidental mutation.This module-level object is intended as a static fallback sentinel. Since nothing should ever mutate it,
Object.freezewould guard against accidental writes.🧊 Proposed change
-const BILLING_NOT_LOADED: FlowgladContextValues = { +const BILLING_NOT_LOADED: FlowgladContextValues = Object.freeze({ loaded: false, errors: null, ... -} +}) as FlowgladContextValuesNote: If the
FlowgladContextValuestype is incompatible withReadonly, you can keep the current form — the risk is low since this is module-scoped and only passed into render-prop children.
62-75: Class component is acceptable here — but consider adding an error-reset mechanism.Error boundaries require class components in React (no hook equivalent exists), so this doesn't violate the spirit of the "functional components only" guideline. However, once
hasErrorflips totrue, the boundary never recovers — the user is stuck on the fallback until the component is fully unmounted and remounted. If billing initializes lazily or recovers after a transient failure, the user won't benefit.A common pattern is to reset on a key change:
♻️ Proposed reset mechanism
-class BillingErrorBoundary extends Component<{ children: ReactNode; fallback: ReactNode }, { hasError: boolean }> { +class BillingErrorBoundary extends Component< + { children: ReactNode; fallback: ReactNode; resetKey?: string | number }, + { hasError: boolean } +> { state = { hasError: false } + static getDerivedStateFromProps( + props: { resetKey?: string | number }, + state: { hasError: boolean; prevResetKey?: string | number }, + ) { + if (props.resetKey !== state.prevResetKey) { + return { hasError: false, prevResetKey: props.resetKey } + } + return null + } static getDerivedStateFromError(): { hasError: boolean } { return { hasError: true } }Then in
SafeBillingyou can pass aresetKey(e.g., a timestamp or route-change counter) to allow recovery.
The render-prop fallback executed outside the boundary's protected subtree, so a bug in the children function would propagate uncaught. Using null simply hides the credits section when billing fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary by CodeRabbit
New Features
Updates
Documentation