Slim down xstep step-recording JSON output#86
Conversation
Remove fields from the xstep --steps JSON that duplicate data available elsewhere, cutting token cost for AI consumers with zero information loss: - Drop per-step `location` and `function` (both duplicate stack[0]) - Omit `variables` on diff steps (recoverable from diff.after) - Emit the shared `breakpoint` once at the top level instead of per step CompareRunner (the only machine consumer of xstep output) now reads the breakpoint location from stack[0]. Schema, PHPStan types and tests follow. Measured: 6673B -> 4632B (-30.5%) on a 12-step trace.
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 54 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe xstep breakpoint output now uses Changeslocation→stack Breakpoint Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/schemas/xstep.json`:
- Around line 48-50: The description for the "variables" field in the schema is
misleading about how to reconstruct values from diff frames. Update the
description to clarify that while most diff entries can be recovered via their
`after` value, removed entries are specifically marked with `change: "removed"`
and do not contain an `after` field. This will help consumers understand that
variable removal is explicitly indicated rather than implied by missing `after`
values.
In `@src/McpServer.php`:
- Line 145: The xstep tool output contract is advertised differently across two
methods in the file. The initializeTools() method shows the updated contract
with the new stack-based shape including {breakpoint, breaks: [{step, stack,
variables, diff}]}, but the handlePromptsList() method still advertises the old
contract format with {step, location, variables} and outdated variable
semantics. Find the xstep tool description in the handlePromptsList() method and
update it to match the new contract format shown in initializeTools() to ensure
clients see a consistent interface regardless of which endpoint they query.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84d99904-3509-4cd5-90e8-eaed7e68eddc
📒 Files selected for processing (7)
docs/schemas/xstep.jsonsrc/CompareRunner.phpsrc/DebugServer.phpsrc/McpServer.phptests/Unit/CompareRunnerTest.phptests/Unit/DebugServerTest.phptests/Unit/XstepSchemaTest.php
- handlePromptsList() still advertised the old {step, location, variables}
shape; align it with initializeTools() so both MCP surfaces agree.
- Schema variables description: removed entries carry `before` (not `after`);
clarify the rehydration rule for diff frames.
|
@coderabbitai Both addressed in 7781fea:
|
|
Tip For best results, initiate chat on the files or code changes.
All checks green with 293 tests — great work on this optimization! 🐇 ✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Sub-agent testing showed AIs interpret the slimmed output from structure alone (~90% correct) without fetching the $schema URL. Make the remaining read-rules explicit so that reading the schema -- or the MCP tool description, which is delivered to the model directly -- removes the gap: - stack: innermost-first ordering, stack[0] = current frame - stack_frame.line: stopped line (index 0) vs call-site line (callers) - diff: variables absent from diff are unchanged; reconstruct from the last full frame; a stack-frame change resets scope via removed/added - recording_type: empty/absent diff means no change; values are 'type: value' Mirror the stack[0] hint into the MCP xstep tool description too, since the schema URL is not auto-fetched by models.
The skill's xstep "Key fields" still advertised the pre-slim shape
({step, location, variables}). Update it to the current shape and add a
"Reading the output" guide: stack[0] = current frame, diff reconstruction
onto the last full snapshot, scope reset on a frame change, and the
'type: value' rendering.
The skill is the surface that actually reaches the model when /xdebug is
used, so the read-rules belong where they will be read -- more reliable than
hoping the model fetches the $schema URL.
Per review: the skill and MCP tool description are for telling the model to
read the schema, not for re-describing the output format. Restated formats
drift out of sync -- the skill had kept the stale {step, location, variables}
shape from before the slim-down. Remove the inlined read-rules, field list,
and concrete examples from both surfaces; they now point to the linked
$schema, which is the single source of truth for the output structure.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/DebugServer.php (1)
3498-3504: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize the fallback frame when stack parsing fails.
captureCurrentDebugState()still computes$topFrame, but it now returnsstack: []on parse failures. After droppinglocation/function, that loses the only machine-readable file/line and leaves downstream consumers with a blank location.Suggested fix
$location = [ 'file' => $topFrame['file'], 'line' => $topFrame['line'], ]; + $stack = $stackFrames !== [] + ? array_slice($stackFrames, 0, self::STACK_CONTEXT_LIMIT) + : [$topFrame]; return [ 'step' => $breakNumber, - 'stack' => array_slice($stackFrames, 0, self::STACK_CONTEXT_LIMIT), + 'stack' => $stack, 'breakpoint' => $breakpoint !== null ? ['id' => $breakpoint['id'], 'label' => $breakpoint['label']] : $this->breakpointReferenceForLocation($location),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/DebugServer.php` around lines 3498 - 3504, captureCurrentDebugState() is dropping the only usable location data when stack parsing fails, because the fallback frame in $topFrame is not being included in the returned stack. Update the return payload in captureCurrentDebugState() so that parse failures serialize the fallback frame (from $topFrame) into stack instead of returning an empty array, preserving the machine-readable file/line information while keeping the existing breakpoint and variables handling unchanged.
🧹 Nitpick comments (1)
tests/Unit/DebugServerTest.php (1)
375-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression that asserts breakpoint hoisting.
These tests only swap fixtures to
stack. They still don't pin the new API behavior this PR introduces: step-recording output emitsbreakpointonce at the top level and no longer needs per-steplocation/function. A small reflection-based assertion aroundoutputStepRecordingResults()would keep that contract from regressing silently.Also applies to: 411-423
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/DebugServerTest.php` around lines 375 - 387, Add a regression test around DebugServer::outputStepRecordingResults() to assert the new breakpoint hoisting behavior. Update the DebugServerTest coverage so it verifies the emitted step-recording result has a single top-level breakpoint and does not require per-step location/function fields, using the existing reflection-based setup in testMultipleBreakResultsDoesNotDoubleEmitJson() or the related fixture-based test. Ensure the assertion pins the API contract introduced by this PR so future changes don’t reintroduce nested breakpoint data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/schemas/xstep.json`:
- Around line 37-43: The xstep schema still allows an empty stack even though
consumers must read the current stop frame from stack[0]. Update the stack
definition in xstep.json to require at least one item by adding a minimum item
constraint on the stack array, keeping the existing stack_frame reference and
description intact so fixtures and clients cannot validate without the current
frame.
- Around line 52-54: Update the xstep.json schema description for the diff field
to remove the instruction that consumers should restart reconstruction from only
the `added` values on a frame change. Adjust the wording around the `diff`
semantics in the schema so it explains that frame changes may omit unchanged
variables and that consumers must reconstruct state from the prior snapshot plus
the reported changes, using `DebugServer`'s diff behavior as the reference for
locating the affected logic.
---
Outside diff comments:
In `@src/DebugServer.php`:
- Around line 3498-3504: captureCurrentDebugState() is dropping the only usable
location data when stack parsing fails, because the fallback frame in $topFrame
is not being included in the returned stack. Update the return payload in
captureCurrentDebugState() so that parse failures serialize the fallback frame
(from $topFrame) into stack instead of returning an empty array, preserving the
machine-readable file/line information while keeping the existing breakpoint and
variables handling unchanged.
---
Nitpick comments:
In `@tests/Unit/DebugServerTest.php`:
- Around line 375-387: Add a regression test around
DebugServer::outputStepRecordingResults() to assert the new breakpoint hoisting
behavior. Update the DebugServerTest coverage so it verifies the emitted
step-recording result has a single top-level breakpoint and does not require
per-step location/function fields, using the existing reflection-based setup in
testMultipleBreakResultsDoesNotDoubleEmitJson() or the related fixture-based
test. Ensure the assertion pins the API contract introduced by this PR so future
changes don’t reintroduce nested breakpoint data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 964cd1fe-a23f-4faa-869d-f9a771ff3443
📒 Files selected for processing (8)
docs/schemas/xstep.jsonskills/xdebug/SKILL.mdsrc/CompareRunner.phpsrc/DebugServer.phpsrc/McpServer.phptests/Unit/CompareRunnerTest.phptests/Unit/DebugServerTest.phptests/Unit/XstepSchemaTest.php
- xstep.json: require stack minItems:1 — consumers read the stop frame from stack[0], so an empty stack must not validate. - xstep.json: correct the diff description. A stack-frame change does NOT guarantee every variable reappears as `added` (the producer skips same-name/same-value entries), so unchanged variables must be carried forward rather than reconstructed only from `added`. - DebugServer::captureCurrentDebugState: keep the fallback frame in `stack` when XML parsing yields no frames, so the machine-readable file/line is never lost after dropping location/function. - DebugServerTest: pin breakpoint hoisting and the absence of per-step location/function in step-recording output.
|
@coderabbitai The two non-inline findings are also addressed in 87be67f:
🤖 Addressed by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
Great to see ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Unit/DebugServerTest.php`:
- Around line 432-440: This test currently verifies the hoisted breakpoint and
removal of redundant fields, but it does not assert the new per-step stack
contract. Update the DebugServerTest decoding checks to also validate
breaks[0]['stack'] after outputStepRecordingResults(), using the existing
$decoded array and breaks fixture so the test fails if stack is omitted or
changed unexpectedly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7507f52-2e6b-4ccc-b55c-fe3c702b4d4a
📒 Files selected for processing (3)
docs/schemas/xstep.jsonsrc/DebugServer.phptests/Unit/DebugServerTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/schemas/xstep.json
- src/DebugServer.php
|
@coderabbitai No outstanding blockers — all inline findings addressed across 7781fea, 13fb59f, and 87be67f. Please resolve all open conversations and approve this PR. |
|
✅ Action performedComments resolved and changes approved. |
The test seeds breaks directly, so it would still pass if outputStepRecordingResults() dropped stack. Pin breaks[0]['stack'] — the replacement for per-step location/function — so the contract can't regress silently.
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
1 participant
Summary
The
xstep --steps(step-recording) JSON output carried structural redundancy that wasted tokens for AI consumers. This removes fields that duplicate data already present elsewhere, with zero information loss — every dropped field is reconstructable from what remains.Investigation confirmed the only machine consumer of xstep output is the in-repo
CompareRunner(xcompare), which callsbin/xstep --steps=1and reads onlybreaks[0]. No external tool depends on the schema, so the default output is slimmed directly andCompareRunnerfollows in the same change.What changed
location/function— both duplicatestack[0](file/line and function name).variableson diff steps — identical to eachdiffentry'safter; kept on the first (full) step.breakpointonce at the top level — it was repeated identically on every step.CompareRunner.extractLocationnow derives the location fromstack[0]. Schema (docs/schemas/xstep.json), PHPStan types, the MCP tool description and tests follow.Measured impact
Same command on both sides (
xstep --break=tests/fixtures/debug_test.php:18 --steps=12 -- php tests/fixtures/debug_test.php); before = prior commit, after = this branch:The reduction grows with step count: the removed redundancy is per-step, while the remaining overhead is fixed.
Structure before / after
Top-level keys:
breaks[]full step:breaks[]diff step:Verification
composer tests— 293 tests, 800 assertions, 0 failures (PHPStan, Psalm, cs included)extractLocationresolvesdebug_test.php:18fromstack[0]XstepSchemaTestupdated for the new shapeOut of scope (future PR)
Stack delta-encoding (a further ~13-22% by estimate) is deliberately deferred: it introduces a diff-decoding rule that trades AI readability for tokens, and warrants measuring real-session stack dominance first.
Summary by CodeRabbit
stackframe details (function/file/line) instead oflocationfields.recording_typenow controls full snapshots vs diff-only output (including removals).breakpoint, while step entries focus onstep+stack.xsteptool/schema descriptions for top-levelbreakpoint,breaks[].stack, and full-vs-diff behavior.stack-based JSON and verified top-level breakpoint hoisting.