fix(beads-rust): unwrap br JSON envelope before filtering#366
Open
thunter009 wants to merge 3 commits intosubsy:mainfrom
Open
fix(beads-rust): unwrap br JSON envelope before filtering#366thunter009 wants to merge 3 commits intosubsy:mainfrom
thunter009 wants to merge 3 commits intosubsy:mainfrom
Conversation
Tool call display showed bare [read] and [apply_patch] with no arguments when input field names didn't match the hardcoded set. Add fallback that extracts the first useful string value from input, prioritizing paths. Also check event.part.input as fallback in OpenCode parser. Closes subsy#362 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The length guard prevented long string values (e.g. apply_patch bodies) from being considered for fallback display, causing blank output. The existing 120-char truncation already keeps output short. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
br list/ready --json now returns {"issues": [...]} instead of a bare
array. Add unwrapBrJson() helper that handles both formats.
Fixes subsy#365
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@thunter009 is attempting to deploy a commit to the plgeek Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
WalkthroughThe PR addresses tool-call display formatting with fallback logic for unknown fields, updates tool input event parsing with fallback support, and fixes br JSON output parsing to handle both bare arrays and envelope objects with an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
+ Coverage 47.35% 47.39% +0.04%
==========================================
Files 111 111
Lines 36446 36496 +50
==========================================
+ Hits 17258 17297 +39
- Misses 19188 19199 +11
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/agents/output-formatting.ts`:
- Around line 149-170: The extractFallbackDisplay function should skip
candidates that exceed the display cap instead of truncating and should
normalise values to a single line before evaluation; update
extractFallbackDisplay to, for each string property in input (using the existing
path-priority logic in extractFallbackDisplay), first take only the first line
(split on '\n' or '\r\n'), then ignore any candidate whose single-line length is
greater than the cap (120) rather than truncating, and only return the first
acceptable bestPath or bestShort; if none remain return undefined. Ensure you
still prefer path-like values (startsWith '/', './', or '~') and only perform
the single-line/length checks when selecting candidates.
In `@src/plugins/trackers/builtin/beads-rust/index.ts`:
- Around line 165-173: The function unwrapBrJson currently returns [] for
unknown payload shapes which hides contract drift; update unwrapBrJson to throw
a descriptive error instead of returning an empty array when the input doesn't
match the expected array or { issues: [...] } shape — include the unexpected
value (e.g., JSON.stringify(parsed)) and mention the expected shapes in the
error message so callers (and CI) will surface malformed/changed br payloads;
keep the existing Array.isArray(parsed) and issues-branch behavior and only
replace the final return [] with the thrown error referencing unwrapBrJson and
BrTaskJson.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8a5af67-f18d-42a0-8e34-93ee79335fc3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src/plugins/agents/builtin/opencode.tssrc/plugins/agents/output-formatting.test.tssrc/plugins/agents/output-formatting.tssrc/plugins/trackers/builtin/beads-rust/index.ts
Comment on lines
+149
to
+170
| function extractFallbackDisplay(input: Record<string, unknown>): string | undefined { | ||
| // Priority: look for path-like values first, then any short string | ||
| let bestPath: string | undefined; | ||
| let bestShort: string | undefined; | ||
|
|
||
| for (const [, value] of Object.entries(input)) { | ||
| if (typeof value !== 'string' || value.length === 0) continue; | ||
|
|
||
| // Path-like values get priority | ||
| if (value.startsWith('/') || value.startsWith('./') || value.startsWith('~')) { | ||
| if (!bestPath) bestPath = value; | ||
| } else if (!bestShort) { | ||
| bestShort = value; | ||
| } | ||
| } | ||
|
|
||
| const result = bestPath ?? bestShort; | ||
| if (!result) return undefined; | ||
|
|
||
| // Truncate if needed | ||
| return result.length > 120 ? result.slice(0, 120) + '...' : result; | ||
| } |
Contributor
There was a problem hiding this comment.
Fallback selection should skip oversized values and normalise multiline input.
extractFallbackDisplay currently truncates long values, but the intended behaviour is to avoid displaying long opaque payloads entirely. Also, preserving embedded newlines can break single-line tool-call rendering. Please normalise to first line and skip candidates beyond the display cap.
💡 Proposed fix
function extractFallbackDisplay(input: Record<string, unknown>): string | undefined {
// Priority: look for path-like values first, then any short string
let bestPath: string | undefined;
let bestShort: string | undefined;
for (const [, value] of Object.entries(input)) {
- if (typeof value !== 'string' || value.length === 0) continue;
+ if (typeof value !== 'string') continue;
+ const candidate = value.split(/\r?\n/, 1)[0]?.trim();
+ if (!candidate) continue;
+ if (candidate.length > 120) continue;
// Path-like values get priority
- if (value.startsWith('/') || value.startsWith('./') || value.startsWith('~')) {
- if (!bestPath) bestPath = value;
+ if (
+ candidate.startsWith('/') ||
+ candidate.startsWith('./') ||
+ candidate.startsWith('~') ||
+ /^[A-Za-z]:[\\/]/.test(candidate)
+ ) {
+ if (!bestPath) bestPath = candidate;
} else if (!bestShort) {
- bestShort = value;
+ bestShort = candidate;
}
}
- const result = bestPath ?? bestShort;
- if (!result) return undefined;
-
- // Truncate if needed
- return result.length > 120 ? result.slice(0, 120) + '...' : result;
+ return bestPath ?? bestShort;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function extractFallbackDisplay(input: Record<string, unknown>): string | undefined { | |
| // Priority: look for path-like values first, then any short string | |
| let bestPath: string | undefined; | |
| let bestShort: string | undefined; | |
| for (const [, value] of Object.entries(input)) { | |
| if (typeof value !== 'string' || value.length === 0) continue; | |
| // Path-like values get priority | |
| if (value.startsWith('/') || value.startsWith('./') || value.startsWith('~')) { | |
| if (!bestPath) bestPath = value; | |
| } else if (!bestShort) { | |
| bestShort = value; | |
| } | |
| } | |
| const result = bestPath ?? bestShort; | |
| if (!result) return undefined; | |
| // Truncate if needed | |
| return result.length > 120 ? result.slice(0, 120) + '...' : result; | |
| } | |
| function extractFallbackDisplay(input: Record<string, unknown>): string | undefined { | |
| // Priority: look for path-like values first, then any short string | |
| let bestPath: string | undefined; | |
| let bestShort: string | undefined; | |
| for (const [, value] of Object.entries(input)) { | |
| if (typeof value !== 'string') continue; | |
| const candidate = value.split(/\r?\n/, 1)[0]?.trim(); | |
| if (!candidate) continue; | |
| if (candidate.length > 120) continue; | |
| // Path-like values get priority | |
| if ( | |
| candidate.startsWith('/') || | |
| candidate.startsWith('./') || | |
| candidate.startsWith('~') || | |
| /^[A-Za-z]:[\\/]/.test(candidate) | |
| ) { | |
| if (!bestPath) bestPath = candidate; | |
| } else if (!bestShort) { | |
| bestShort = candidate; | |
| } | |
| } | |
| return bestPath ?? bestShort; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/agents/output-formatting.ts` around lines 149 - 170, The
extractFallbackDisplay function should skip candidates that exceed the display
cap instead of truncating and should normalise values to a single line before
evaluation; update extractFallbackDisplay to, for each string property in input
(using the existing path-priority logic in extractFallbackDisplay), first take
only the first line (split on '\n' or '\r\n'), then ignore any candidate whose
single-line length is greater than the cap (120) rather than truncating, and
only return the first acceptable bestPath or bestShort; if none remain return
undefined. Ensure you still prefer path-like values (startsWith '/', './', or
'~') and only perform the single-line/length checks when selecting candidates.
Comment on lines
+165
to
+173
| function unwrapBrJson(parsed: unknown): BrTaskJson[] { | ||
| if (Array.isArray(parsed)) { | ||
| return parsed as BrTaskJson[]; | ||
| } | ||
| if (parsed && typeof parsed === 'object' && 'issues' in parsed && Array.isArray((parsed as Record<string, unknown>).issues)) { | ||
| return (parsed as Record<string, unknown>).issues as BrTaskJson[]; | ||
| } | ||
| return []; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid silently treating unrecognised br payloads as “no tasks”.
At Line 172, falling back to [] for unknown shapes can hide CLI contract drift as a legitimate empty result. In practice this can quietly stop scheduling rather than surfacing a diagnosable failure.
💡 Proposed fix
function unwrapBrJson(parsed: unknown): BrTaskJson[] {
if (Array.isArray(parsed)) {
return parsed as BrTaskJson[];
}
- if (parsed && typeof parsed === 'object' && 'issues' in parsed && Array.isArray((parsed as Record<string, unknown>).issues)) {
- return (parsed as Record<string, unknown>).issues as BrTaskJson[];
+ if (parsed && typeof parsed === 'object') {
+ const issues = (parsed as Record<string, unknown>).issues;
+ if (Array.isArray(issues)) {
+ return issues as BrTaskJson[];
+ }
}
- return [];
+ throw new TypeError(
+ 'Unexpected br JSON shape: expected BrTaskJson[] or { issues: BrTaskJson[] }'
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function unwrapBrJson(parsed: unknown): BrTaskJson[] { | |
| if (Array.isArray(parsed)) { | |
| return parsed as BrTaskJson[]; | |
| } | |
| if (parsed && typeof parsed === 'object' && 'issues' in parsed && Array.isArray((parsed as Record<string, unknown>).issues)) { | |
| return (parsed as Record<string, unknown>).issues as BrTaskJson[]; | |
| } | |
| return []; | |
| } | |
| function unwrapBrJson(parsed: unknown): BrTaskJson[] { | |
| if (Array.isArray(parsed)) { | |
| return parsed as BrTaskJson[]; | |
| } | |
| if (parsed && typeof parsed === 'object') { | |
| const issues = (parsed as Record<string, unknown>).issues; | |
| if (Array.isArray(issues)) { | |
| return issues as BrTaskJson[]; | |
| } | |
| } | |
| throw new TypeError( | |
| 'Unexpected br JSON shape: expected BrTaskJson[] or { issues: BrTaskJson[] }' | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/trackers/builtin/beads-rust/index.ts` around lines 165 - 173, The
function unwrapBrJson currently returns [] for unknown payload shapes which
hides contract drift; update unwrapBrJson to throw a descriptive error instead
of returning an empty array when the input doesn't match the expected array or {
issues: [...] } shape — include the unexpected value (e.g.,
JSON.stringify(parsed)) and mention the expected shapes in the error message so
callers (and CI) will surface malformed/changed br payloads; keep the existing
Array.isArray(parsed) and issues-branch behavior and only replace the final
return [] with the thrown error referencing unwrapBrJson and BrTaskJson.
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.
Summary
br list --jsonnow returns{"issues": [...]}instead of a bare array.filter()directly on the parsed object, crashing withtasksJson.filter is not a functionunwrapBrJson()helper that handles both envelope and bare-array formatsgetTasks(),getEpics(), andgetNextTask()Test plan
ralph-tui run --tracker beads-rust --epic bd-3r0rinitializes without errorunwrapBrJsonwith both formatsFixes #365
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests