Skip to content

fix(test): call isStructuredAgentSessionThinking with its { items } source (unbreaks main typecheck) - #25948

Closed
brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/main-live-turn-test-shape
Closed

brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/main-live-turn-test-shape

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​6 $\color{#cf222e}{\Huge{\mathbf{−}}}$​2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4
Prod 0 0 0 0

ELI5

Main's typecheck fails, and because of that, CI skips the test jobs on every pull request. Two PRs merged a few hours apart: #19221 added two test lines that call isStructuredAgentSessionThinking with a plain list of items, and #25751 changed that function to take { items, latestTurn }. Each was correct on its own, and git merged them without a conflict. This PR wraps those two calls in { items: … }, the shape every other test in the file already uses.

What Changed

  • src/shared/structured-agent-session-live-turn.test.ts: the two reasoning-state assertions pass { items: [...] } instead of a bare array (TS2741 at lines 42 and 44 on main b922342199b).
  • Test-only. Nothing users see changes, and the assertions mean the same thing.

Why

It's the smallest correct fix: the test now calls the function the way it is defined on main.

Linked Issue

N/A (main's typecheck fails since #19221 and #25751 crossed)

Visual Proof

N/A: a test-only fix; nothing visible changes.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

On this branch (main b922342199b plus this commit), the node and web typechecks report no error in this file. The only remaining errors are the stream-json, stream-chain and devalue modules missing from the local install, which CI installs. The file could not be run locally, because the shared install is missing main's new devalue dependency, so the vitest config can't load; CI runs it. oxlint --deny-warnings is clean.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Test-only. No security, platform, SSH, mobile or compatibility impact.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…ource

#19221 added two reasoning-state assertions that pass a bare item array, and
#25751 changed the function to take { items, latestTurn }. Each merged
cleanly, so main's typecheck fails with TS2741 and CI skips its tests for
every PR.
@coderabbitai

coderabbitai Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3b6048fc-5e78-4386-974c-523faf67a189
📥 Commits

Reviewing files that changed from the base of the PR and between b922342 and 96c3c0d.

📒 Files selected for processing (1)
  • src/shared/structured-agent-session-live-turn.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The isStructuredAgentSessionThinking tests now pass a session snapshot object containing an items array. The running reasoning case expects true, and the completed reasoning case expects false.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 96c3c

This test-only change aligns its inputs with the expected snapshot shape and does not alter product behavior. No actionable merge risk is identified; it is ready for normal checks.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the test fix and its purpose: pass the expected { items } source to isStructuredAgentSessionThinking to resolve the main typecheck failure.
Description check ✅ Passed The description explains the change, its reason, testing status, and lack of visual or platform impact. It also documents that local test execution was blocked by missing dependencies. The linked issu…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Fresh review of 96c3c0d8bd2: no findings. The two stale array arguments caused TS2741 after the reader changed its input shape. { items } fixes that; omitted latestTurn exactly preserves the old item-based scan, so running reasoning still expects true and completed reasoning false. No user-visible behavior changes. Searched src/ and tests/, including both other HostTurnSource readers: no remaining bare-array callers; nothing deferred.

Already superseded: current origin/main at 4c355161500 (#25949) contains the identical test-file fix.

CI settled: mobile verification passes. Both reds are inherited from the old main baseline: static analysis fails on the unchanged session-host file's 301-line lint violation; aggregate verification fails because that check failed and unit tests were skipped. No new reds. Desktop typechecking and unit tests never ran in CI. No local tests/typechecks were attempted; the shared install cannot load vitest because devalue is missing.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Superseded by #25949, which already fixed this TS2741 on main (merged 19:24Z). Closing.

@brennanb2025
brennanb2025 deleted the brennanb2025/main-live-turn-test-shape branch October 6, 2026 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant