Skip to content

fix(ci): repair main's line limits and a stale test import - #33675

Merged
Jinwoo-H merged 6 commits into
mainfrom
brennanb2025/main-fix-startup-host-lines
Oct 25, 2026
Merged

Jinwoo-H merged 6 commits into
mainfrom
brennanb2025/main-fix-startup-host-lines

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​159 $\color{#cf222e}{\Huge{\mathbf{−}}}$​316 $\color{#cf222e}{\Huge{\mathbf{−}}}$​157
Prod 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​241 $\color{#cf222e}{\Huge{\mathbf{−}}}$​11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​230

ELI5

Main's CI fails on every PR for three reasons. Root lint fails because two files exceed their line limits: #32334 grew the startup host test to 824 counted lines against 800, and #31602 grew the status feed to 301 against 300. Typecheck fails because #31602 added a test that imports launch persistence from a path that no longer exists. This PR repairs all three while preserving behavior and test coverage.

What Changed

Moved the child-report suite and current-generation readiness coverage into structured-agent-session-startup-attempt-reports.test.ts. Both test files reuse one test-support module for mocks, fixture setup, cleanup, and helpers. Every existing test body and assertion is unchanged. The extracted helper reads synchronous status without an unnecessary await, and exported mocks have explicit Mock annotations so their types remain portable.

Moved the status feed's child-work summary formatting into the existing structured-agent-session-status-child-work.ts module. The class method forwards to the extracted helper with the same inputs. The formatting logic, class signatures, and exports visible to callers are unchanged.

File Counted lines Limit
structured-agent-session-startup-attempt-host.test.ts 552 800
structured-agent-session-startup-attempt-reports.test.ts 126 800
structured-agent-session-startup-attempt-host-test-support.ts 209 300
structured-agent-session-status-feed.ts 292 300
structured-agent-session-status-child-work.ts 49 300

legacy-chat-rename-migration.test.tsx now imports readStructuredAgentLaunchRecord from @/lib/agent-session/launch/structured-agent-session-launch-persistence, where the module lives; its 8 tests pass.

Counts exclude blank and comment-only lines, matching the configured rule. No lint suppression, baseline change, configuration change, or runtime behavior change.

Why

The test split keeps fixtures in one module, and the status extraction places the formatting beside the existing child-work projection. Both files have headroom under their limits.

Linked Issue

Main regressions introduced by #32334 and #31602; no new issue needed for these mechanical repairs.

Visual Proof

N/A — test organization and behavior-preserving helper extraction only; no UI or interaction change.

Testing

  • Both startup test files passed through tc-gate on macOS: 2 files, 28 tests.
  • All eight explicitly named status-feed test files passed through tc-gate after the main sync and extraction: 8 files, 48 tests, including child-work broadcasts, retained names, ownership, stopping, and clock behavior.
  • Verified all original startup suite/test blocks remain verbatim after the split.
  • Formatted only changed files; normal commit hooks passed. The main-merge commit used the prescribed lint-staged --diff=MERGE_HEAD --concurrent false checks.
  • The rename-migration test passes on current head 04395e762e53b234b114479d711cb937ff195dae: 1 file, 8 tests. This includes the correction of the third main failure, the stale test import.
  • Current-head CI passed: static analysis and typecheck, all five unit-test shards, relay integration, cross-version wire compatibility, packaging, mobile verification, and WSL checks. QA is tests-only because the production change extracts identical formatting without changing a flow. No build or app run is required for these mechanical moves.

Agent skill upstream boundary

  • Not applicable; no upstream skill-installer source or data copied.

Checklist

  • Focused repairs for the two main lint failures and stale test import.
  • Cross-platform, SSH, mobile, and compatibility behavior unchanged.
  • Self-reviewed; all existing tests and assertions retained.

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

The change extracts shared fixtures and helpers for startup-attempt host tests, and adds a dedicated test file for startup option reports. It also adds a helper for deriving child-work status fields and updates the status feed to use it.

Priority: Low

Merge Risk: Moderate · up to ff47a

This test-only refactor has no production behavior change. The new shared test-support module fails typecheck because two exported mock constants lack explicit types, so CI stays red. Add the type annotations before merging.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
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.
Title check Passed The title clearly identifies the main changes: repairing CI line-limit failures and a stale test import.
Description check Passed The description covers the core of the change well. It has an ELI5 that explains the three main CI failures (two lint line-limit overruns and one stale import). It also has a What Changed section with…
Full details: Description check

Explanation

The description covers the core of the change well. It has an ELI5 that explains the three main CI failures (two lint line-limit overruns and one stale import). It also has a What Changed section with the file-level line counts, a Why section, and a Testing section that lists what was run. The Visual Proof section is marked N/A with a reason, and the agent skill boundary is checked. Two template items are weak. The Linked Issue section has no 'Fixes #' issue reference and says no new issue is needed, which the template's outside-contributor rule does not cover. The Checklist has no unchecked items, and the template's 'manually tested locally' box is not checked, though the testing done is described. The Notes list of cross-platform, SSH, mobile, security, and performance checks is addressed in the checklist. Overall the description is mostly complete.

Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · 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 brennanb2025 changed the title test(native-chat): split startup host tests to repair main lint fix(ci): repair main's startup test and status feed line limits Oct 25, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8f69d582-1595-428a-9880-b6fb03f8f03b
Commits

Reviewing files that changed from the base of the PR and between 3aec926 and ff47ab2.

Files selected for processing: 5
  • src/main/native-chat/agent-session-wire/structured-agent-session-startup-attempt-host-test-support.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-startup-attempt-host.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-startup-attempt-reports.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-status-child-work.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-status-feed.ts

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

@brennanb2025 brennanb2025 changed the title fix(ci): repair main's startup test and status feed line limits fix(ci): repair main's line limits and a stale test import Oct 25, 2026
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Main's CI was blocked by two files exceeding their line limits and a rename-migration test importing a module from its former location. This PR splits the startup tests and fixtures, moves identical child-work formatting beside its existing projection, and fixes that test import. User behavior is unchanged.

The full review completed one round with independent correctness and architecture reviewers. No new findings or fixes were needed. All 23 original startup test declarations (28 expanded cases), assertions, and fixture behavior are retained. The extracted formatter preserves input reads, property omission, ordering, provider IDs, and usage stripping. No suppression, baseline, configuration, or saved-data/wire change was added. The earlier exported-mock type issue was already fixed, and its outdated review thread is resolved. Nothing is deferred.

Verified: 28 startup tests, 48 status-feed tests, and 8 rename-migration tests passed across 11 explicitly named files. The production formatter remains identical to the tested extraction; later test-only commits preserve that evidence. QA is tests-only: no lifecycle, ownership, login, screen appearance, or user flow changes, so no app run or screenshot is required. Final reference comparison and the readiness checklist found no blockers or new scaling work; existing deployment-specific child-work reporting is an intended retained requirement.

CI is green on 04395e762e53b234b114479d711cb937ff195dae, including static analysis/typecheck and all five unit shards, relay integration, cross-version compatibility, packaging, mobile verification, and WSL checks. GitHub reports CLEAN and mergeable.

For Brennan's review: the formatting extraction and test import repair. No user-facing design decision is needed.

@Jinwoo-H
Jinwoo-H merged commit 5a4dd30 into main Oct 25, 2026
43 checks passed
@Jinwoo-H

Copy link
Copy Markdown
Contributor

@brennanb2025 I went ahead and merged this myself to unblock main's CI.

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.

2 participants