Skip to content

fix(acp): repair main's typecheck after the legacy journal-path removal - #26094

Merged
Jinwoo-H merged 1 commit into
mainfrom
fix/acp-restore-failed-journal-open-spy
Oct 7, 2026
Merged

Jinwoo-H merged 1 commit into
mainfrom
fix/acp-restore-failed-journal-open-spy

Conversation

@Jinwoo-H

@Jinwoo-H Jinwoo-H commented Oct 7, 2026 •

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

ELI5

Two changes landed on main at nearly the same time. One removed a function; the other added a test that pretends that function fails. Each passed CI alone, but together main no longer typechecks, so every open PR's "static analysis and typecheck" job is red. This updates the test.

What Changed

src/main/acp/acp-structured-host-restore-failed.test.ts (added in #25225) spied on JournalHostDatabase.legacyDirectoryFor to make the chat journal's open fail. #26038 deleted that method and moved its own tests to vi.spyOn(AgentSessionJournal.prototype, 'open').mockRejectedValueOnce(...). This test now uses the same spy, and drops the journalDatabase binding it no longer needs. No product code changes.

Why

It reuses the exact replacement #26038 chose for the same failure injection, so all the "journal open fails" tests use one seam.

Linked Issue

N/A (main-wide typecheck break, seen on #26088 and #26093).

Visual Proof

N/A, test-only change.

Testing

  • tsc --noEmit -p config/tsconfig.node.json clean.

  • vitest run src/main/acp/acp-structured-host-restore-failed.test.ts: 6/6 pass, including the two tests that use the injected failure.

  • oxlint / oxfmt clean.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

  • Not applicable

Notes

Test-only.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • N/A for visuals with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered
  • CI will cover lint/typecheck/test/build

…he restore-failed test

#26038 removed JournalHostDatabase.legacyDirectoryFor while #25225's test still
spied on it, so typecheck fails on main. Use the replacement spy #26038 gave
its own tests.
@coderabbitai

coderabbitai Bot commented Oct 7, 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: 13856c59-0bb8-48f1-b5f4-fe74e1419e15
📥 Commits

Reviewing files that changed from the base of the PR and between 0acf039 and 24be342.

📒 Files selected for processing (1)
  • src/main/acp/acp-structured-host-restore-failed.test.ts

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


📝 Walkthrough

Walkthrough

The failed-attach test now makes AgentSessionJournal.prototype.open reject once with journal path unavailable. It removes the previous failure injection through journalDatabase.legacyDirectoryFor. The later attach is still expected to reject with the same error.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 24be3

The test now exercises the intended journal-open failure and verifies that attachment rejects; no concrete merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: repairing the typecheck by updating the test after removal of the legacy journal-path method.
Description check ✅ Passed The description covers the change, motivation, testing, visual-proof status, and checklist. The Linked Issue section says N/A rather than linking an issue, but the rest of the description is complete …
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 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.

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