Skip to content

fix(test): initialize pending prompt state in Claude retention fixture - #25095

Closed
nwparker wants to merge 1 commit into
mainfrom
nwparker/claude-retention-prompt-fixture
Closed

nwparker wants to merge 1 commit into
mainfrom
nwparker/claude-retention-prompt-fixture

Conversation

@nwparker

@nwparker nwparker commented Oct 3, 2026 •

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

ELI5

Main's Node typecheck fails because the Claude output-retention test omits the session's new pending-prompt state. Initialize that state so the existing test can compile again.

What Changed

The retention fixture supplies an empty instance of the existing Claude prompt registry alongside its child-work decoder and journal translator. Its 50 MiB output, digest, truncation and no-extra-join assertions remain intact.

Why

The updated child-work reader requires the registry to identify children waiting on permission. Giving this test the same session state keeps that contract checked; making the production field optional would conceal missing state in real sessions.

Linked Issue

Follow-up to #22634, contributed by @brennanb2025. This fixes the omitted fixture initialization after that merge.

Visual Proof

N/A — this initializes test state and changes no product interface.

Testing

  • I manually tested these changes locally
  • Existing tests updated

Exact main 752871b independently reproduces the missing-prompts Node type error. The one-file repair passes the Node typecheck, 19 tests across the retention and permission-request suites, full-file lint, formatting, all six changed-code checks and the official full-repository anti-slop scan. Tests use background launch mode and private HOME, XDG and temporary directories.

Review

Independent read-only review confirms that the empty registry has no pending requests or callbacks and all other 32,306 parent source entries remain exact. No production Claude or execution protocol changes are included.

Agent skill upstream boundary

  • Not applicable; no upstream skill-installer material copied.

Notes

The fixture uses the existing platform-independent registry. SSH, mobile and native input behavior are outside this test-state repair.

Checklist

  • This PR is small and focused
  • Before/after, mechanism and reasons explained
  • N/A with a reason for visual proof
  • Self-reviewed and independently reviewed
  • Cross-platform and SSH impact considered
  • Relevant local checks pass; CI covers remaining repository checks

@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

  • Initialize pending-prompt state in the retention fixture — the fixture now passes prompts: new ClaudePromptRegistry() alongside childWork and translator to drainClaudeChildWork, satisfying the session parameter's now-required prompts field.

The production contract in src/main/claude/claude-child-work-evidence.ts:146 picks 'childWork' | 'translator' | 'prompts', and claudeWaitingChildIds reads session.prompts.awaitsAnswer(...) (:127). ClaudePromptRegistry is exactly ClaudeSession.prompts (src/main/claude/claude-structured-session-state.ts:159), so the fixture state is the real type, not a stub. An empty registry makes awaitsAnswer always false, so draining adds no waiting children and the retention test's 50 MiB output, digest, truncation, and no-extra-join assertions still pin the same behavior.

Making the field optional instead would hide missing state in real sessions, so initializing it in the fixture is the right call for a test-only repair.

Verified locally: the focused suite passes (3 tests) and pnpm tc:node exits 0 against this head.

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

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

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: 971cb7bb-9671-4e52-9de6-d56e68a2d546
📥 Commits

Reviewing files that changed from the base of the PR and between 752871b and 6a83da3.

📒 Files selected for processing (1)
  • src/main/claude/claude-child-work-evidence-retention.test.ts

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


📝 Walkthrough

Walkthrough

The large-result retention test now imports ClaudePromptRegistry and provides a prompt registry when it calls drainClaudeChildWork.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 6a83d

This change only updates test setup; the large-result retention checks remain in place, with no production behavior change or actionable merge-blocking risk.

Architecture Summary

Architecture risk: 🔵 Low · up to 6a83d

The change affects 1 system.

Changed systems: src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in src/main/claude/claude-child-work-evidence-retention.test.ts: The test imports ClaudePromptRegistry for use when draining child work.
  • observed — Modified behavior in src/main/claude/claude-child-work-evidence-retention.test.ts: The drainClaudeChildWork test call now provides a prompt registry in addition to the decoder and translator.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: initializing pending prompt state in the Claude retention test fixture.
Description check ✅ Passed The description covers the change, reason, issue reference, visual proof, testing, review, and checklist. It is complete enough to explain the repair and its verification.
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
📝 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

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.

@nwparker

nwparker commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Closing this duplicate after #24952 merged both fixture repairs into main at 8cd9751963b3046e8393f2007c9104a56a63fa84. The retention fixture now initializes the existing prompt registry, and the notification test explicitly recreates an older tab without its host stamp while retaining the collision, subscription, completion and unread checks.

I verified the merged changes and all published checks passed or were skipped, including all five test shards and typechecks. Our separate fixture qualification remains preserved; no commit from this PR is claimed as merged. Thanks @OrcaWin for the merged repairs, and @brennanb2025 for the original tests in #22634 and #24518.

@nwparker nwparker closed this Oct 3, 2026
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