Skip to content

fix(test): use the current provider handle shape in the submission-positions test (unbreaks main typecheck) - #25709

Merged
brennanb2025 merged 1 commit into
mainfrom
brennanb2025/main-journal-positions-test-type
Oct 5, 2026
Merged

brennanb2025 merged 1 commit into
mainfrom
brennanb2025/main-journal-positions-test-type

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 5, 2026 •

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

ELI5

Main's node typecheck fails. #25073 added a test that builds an agent-session identity with an old, retired shape for its provider handle, and the type checker rejects it. Every pull request that merges main inherits the failure. This PR changes the test to build the handle the current way, the same fix #25706 made in another test today.

What Changed

  • journal-submission-positions.test.ts builds its provider handle with codexProviderHandle('thread-1') instead of the retired { kind: 'codex', threadId: 'thread-1' } object literal (TS2353: kind does not exist on AgentSessionProviderHandle).
  • Test-only. Nothing users see changes.

Why

It's the same one-line change #25706 made for the stop-note test, using the shared helper that encodes the current handle shape.

Linked Issue

N/A (main's node typecheck fails since #25073)

Visual Proof

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

Testing

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

The changed test file and the node typecheck run on this branch; results are in the review comment.

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)

…sitions test

#25073 built the test's journal identity with the retired { kind, threadId }
provider handle, which the node typecheck rejects (TS2353). Same fix #25706
made in the stop-note test.
@coderabbitai

coderabbitai Bot commented Oct 5, 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: 514b7f79-414f-40a8-ae32-db2c68dd0a93
📥 Commits

Reviewing files that changed from the base of the PR and between 9def4b9 and b546f4f.

📒 Files selected for processing (1)
  • src/main/native-chat/agent-session-journal/journal-submission-positions.test.ts

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


📝 Walkthrough

Walkthrough

The test imports codexProviderHandle and uses codexProviderHandle('thread-1') to create its Codex provider handle. It continues to test rejected submission turn association and journal positions across snapshots, replay, and history pages.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to b546f

The test fixture now uses the shared Codex handle helper without changing the thread identity or existing assertions. No actionable merge risk remains.

🚥 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. It is specific and related to the main change.
Description check ✅ Passed The description explains the problem, the test-only change, the reason, visual impact, and testing. It also addresses the template checklist and notes that there is no user-visible change.
Linked Issues check ✅ Passed The only directly linked issue, #25706, is closed and provides historical context only. No active directly linked issue imposes coding requirements. The PR updates the test fixture to use `codexProvid…
Out of Scope Changes check ✅ Passed The PR changes one test fixture to use the current Codex provider-handle helper. This supports the stated typecheck fix. The change summary shows no unrelated production changes.
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…
✨ Finishing Touches 💡 1
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

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.

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

  • Use the current provider-handle helper — journal-submission-positions.test.ts now builds IDENTITY.providerHandle via codexProviderHandle('thread-1') instead of the retired { kind: 'codex', threadId: 'thread-1' } literal, which AgentSessionJournalIdentity.providerHandle: AgentSessionProviderHandle | null rejects (TS2353). No production surface.

Verified on b546f4f0: pnpm tc:node exits 0 and the file's 5 tests pass. The helper returns the correct in-memory shape ({ transport: 'codex-app-server', agent: 'codex', nativeId: 'thread-1' }), and it matches the same fix already applied to the stop-note test in #25706.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review: ready (test-only change)

Problem. Since #25073, main's node typecheck fails on one line. journal-submission-positions.test.ts:29 built its journal identity with the retired provider-handle literal { kind: 'codex', threadId }, which the identity's type no longer accepts (TS2353).

User-facing change. None. Only a test file changes. It now builds the handle with codexProviderHandle('thread-1'), the same fix #25706 made in the stop-note test. Because nothing users see changes, there was no architecture review and no app QA.

Verified

  • Re-baselined on origin/main 9def4b9. No other change has landed this fix.
  • A fresh reviewer confirmed two things:
    • The helper produces the current handle shape, and the journal still records it as the same Codex thread, so the test means what it did before.
    • No other leftover literal fails a type-checked project. Searched src/ and tests/, then checked each hit against the configs CI typechecks.
  • Node typecheck on this head: the TS2353 is gone and no new errors appear. CI's static analysis and typecheck passes.
  • The changed test passes (5/5), and lint is clean.

CI checks still red, all failing on main as well (not caused by this PR)

Deferred
Three tests/e2e/cross-version-wire/*.unit.test.ts files also carry the old literal: submission-positions-downgrade, agent-session-stop-event-downgrade and agent-session-newer-chat-refusal. They are outside the enforced typecheck, so they don't fail it.

Not verified
Whether the old literal in those three files changes their results. My local run of them failed only on archive-extraction timeouts. On this PR's CI, their job never got that far, because it stopped at the protocol check first.

@brennanb2025
brennanb2025 merged commit e43a9f7 into main Oct 5, 2026
28 of 31 checks passed
Fartown pushed a commit to Fartown/orca that referenced this pull request Oct 6, 2026
…ehind

Upstream b6c5f3f merged stablyai#25709 and an earlier fix for the same journal
test; both import codexProviderHandle, so upstream main fails typecheck and
lint. One import is removed here and the file is allowlisted until upstream
dedupes it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014bzZ8TEtvx8zdQnhDeNZAm
brennanb2025 added a commit that referenced this pull request Oct 6, 2026
…ssion-positions test (#25713)

Two fixes for the same provider-handle change (#22682 and #25709) each added the import,
so main fails tc:node with TS2300 (duplicate identifier).
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