Skip to content

fix(test): remove tower and watch load flakes - #363

Merged
elkaix merged 1 commit into
mainfrom
fix/tower-watch-load-flakes
Oct 2, 2026
Merged

elkaix merged 1 commit into
mainfrom
fix/tower-watch-load-flakes

Conversation

@elkaix

@elkaix elkaix commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

[skip changeset] Test and restore-ordering fix only; nothing users can see.

Summary by CodeRabbit

  • Bug Fixes
    • Tower handoffs now wait for the exit process to complete when another active session owns the tower, helping ensure ownership changes finish before reconciliation continues.
  • Tests
    • Improved watcher test timing by waiting for expected file events rather than relying on a fixed delay.
    • Expanded tower replay and restore test coverage across additional scenarios.

Await the foreign-reconcile tower exit during restore so ownership
release cannot outlive the service, stub the logger in every forked
tower test, and poll for chokidar signal events instead of sleeping.
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: PyModel/pythinker-code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3229c8db-8a8b-45eb-9994-89a52fb64f27
📥 Commits

Reviewing files that changed from the base of the PR and between f1fbb9c and 791eb57.

📒 Files selected for processing (3)
  • packages/agent-core-v2/src/features/tower/towerService.ts
  • packages/agent-core-v2/src/human/test/utils/watch.test.ts
  • packages/agent-core-v2/test/features/tower/towerService.test.ts

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


📝 Walkthrough

Walkthrough

The tower reconciliation path now awaits exit when another present session owns the tower. Tower tests add log-service stubs to secondary instantiation services. Two watcher tests poll for file-created events instead of using fixed delays.

Changes

Tower reconciliation and tests

Layer / File(s) Summary
Tower reconciliation and test setup
packages/agent-core-v2/src/features/tower/towerService.ts, packages/agent-core-v2/test/features/tower/towerService.test.ts
The foreign-tower path awaits exit('foreign-reconcile'). Replay and restore tests add log-service stubs to secondary instantiation services.

Watcher tests

Layer / File(s) Summary
Watcher event polling
packages/agent-core-v2/src/human/test/utils/watch.test.ts
The native-watch fallback and Linux signal-mode tests poll for the file-created event instead of waiting 300 ms.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 791eb

The restore-ordering fix and test updates have no established merge-blocking risk. Merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 791eb

The change tightens restore ordering without adding access or authority. Ownership remains session-guarded, but complete cleanup under failure and repeated restoration is only partially established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The assessed ownership operation targets the tower workspace resolved from the restoring session's working directory. It cannot clear a different persisted session owner's identity through the checked release operation.

Trust Boundaries and Controls

  • observed — Identity is checked at two distinct points: the dispatcher rejects agent-domain events whose agent identity does not match its lifecycle context, and TowerStore release requires the persisted owner to match the releasing session under the state lock.

Resilience and Maintainability Implications

  • inferred — The new await is not an atomic state-and-authority barrier. Exit still discards the event-dispatch promise, while repeated restoration can queue dispatches until after restore hooks. TowerModeExit clears local active, owner, and base state when applied, but ownership-release completion does not itself prove that application. This limitation predates the PR; no worsening was established.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is largely incomplete. It does not provide the required requirement or bug, reproduction steps, root cause, code changes, behavior-change table, or checklist responses. Complete the required sections: identify the bug or requirement, add reproduction steps, explain the root cause and whether the fix is fundamental or a workaround, describe the code changes, document observable behavior with affected module…
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the required conventional-commit prefix, uses imperative wording, stays within 72 characters, and accurately describes the test and restore-ordering fixes.
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 3…
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.
Full details: Description check

Resolution

Complete the required sections: identify the bug or requirement, add reproduction steps, explain the root cause and whether the fix is fundamental or a workaround, describe the code changes, document observable behavior with affected modules and test coverage, and complete the checklist. State why no changeset and documentation update are needed.

  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

@pkg-pr-new

pkg-pr-new Bot commented Oct 2, 2026

Copy link
Copy Markdown
pnpm dlx https://pkg.pr.new/@pymodel/pythinker-code@791eb57
npx https://pkg.pr.new/@pymodel/pythinker-code@791eb57

commit: 791eb57

@elkaix
elkaix merged commit ecc2c3b into main Oct 2, 2026
27 checks passed
@elkaix
elkaix deleted the fix/tower-watch-load-flakes branch October 2, 2026 23:07
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