[factory] p12: node-targeted placement + reject-and-reconcile#1141
Conversation
Factory issue p12. Autonomous build via relayflows squad loop.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds ChangesSDK Node Placement Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a83d124b61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export * from './normalize.js'; | ||
| export { RelaycastMessagingClient, type RelaycastMessagingOptions } from './relaycast.js'; | ||
| export { | ||
| RelayPlacementError, |
There was a problem hiding this comment.
The root AGENTS.md requires curating CHANGELOG.md [Unreleased] as PRs land. This commit exports new public SDK surface (RelayPlacementError plus placement.spawn and related types), but CHANGELOG.md has no placement.spawn/RelayPlacementError entry (checked with rg -n "placement\.spawn|RelayPlacement" CHANGELOG.md), so the release notes will omit this user-visible API. Please add a concise Added bullet under [Unreleased].
Useful? React with 👍 / 👎.
| if (placement.capability.startsWith('spawn:') && typeof payload.cli !== 'string') { | ||
| payload.cli = placement.capability.slice('spawn:'.length); |
There was a problem hiding this comment.
When input.input already contains a string cli, this keeps it even though node eligibility was checked against capability. For example, capability: 'spawn:claude' with input: { cli: 'codex' } can select a node that only advertises spawn:claude but then invoke the broker spawn action with cli: 'codex' (the broker SpawnParams in crates/broker/src/types.rs:160-164 uses cli to choose the harness). Please overwrite cli from the spawn: capability or reject mismatches before invoking.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/sdk/src/messaging/placement.test.mts (1)
247-390: ⚡ Quick winUse deterministic timers for queue-drain tests to reduce CI flake risk.
These tests depend on tight wall-clock sleeps (e.g., 35ms vs 25ms polling). Under slower CI scheduling, that margin can intermittently fail.
Suggested approach
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +beforeEach(() => { + vi.useFakeTimers(); +}); + +afterEach(() => { + vi.useRealTimers(); +}); // inside queue/drain tests - await new Promise((resolve) => setTimeout(resolve, 35)); + await vi.advanceTimersByTimeAsync(35);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/messaging/placement.test.mts` around lines 247 - 390, Replace the wall-clock sleeps in these four test cases with deterministic Jest fake timers to eliminate CI flake risk. For each test (the ones starting with "queues a targeted offline node", "queues a targeted node that does not map the repo", "reconciles an unmapped repo", and "queues when no eligible node is live"), add jest.useFakeTimers() at the beginning and replace each await new Promise((resolve) => setTimeout(resolve, 35)) call with jest.advanceTimersByTime(35) to deterministically advance time. After each test completes, call jest.useRealTimers() to restore real timers. This removes the dependency on tight wall-clock timing margins and makes the tests robust across varying CI scheduling speeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sdk/src/messaging/relaycast.ts`:
- Around line 1154-1166: The error code unmapped_repo is declared as part of the
public RelayPlacementError type union but is never actually thrown anywhere in
the placement.spawn logic, making it an unreachable code path. Either remove
unmapped_repo from the exported RelayPlacementError union type if it should not
be a valid error code, or identify where unmapped_repo conditions should be
detected (likely when repo mapping fails) and add the logic to throw a
RelayPlacementError with code unmapped_repo in those cases. Also check the
location referenced in the comment (lines 266-267) as the same issue applies
there.
- Around line 1308-1320: The reconcilePlacement and logPlacement methods call
user-provided callbacks (input.onReconcile, input.log, and this.placementLog)
without error handling. If these callbacks throw or reject, the entire
placement.spawn operation fails unnecessarily since the core node selection
logic has already succeeded. Wrap each caller-provided callback invocation in
try-catch blocks to isolate any errors they throw, allowing the placement to
complete successfully even if the observability callbacks fail. This applies to
the await input.onReconcile call in reconcilePlacement and the input.log and
this.placementLog calls in logPlacement.
---
Nitpick comments:
In `@packages/sdk/src/messaging/placement.test.mts`:
- Around line 247-390: Replace the wall-clock sleeps in these four test cases
with deterministic Jest fake timers to eliminate CI flake risk. For each test
(the ones starting with "queues a targeted offline node", "queues a targeted
node that does not map the repo", "reconciles an unmapped repo", and "queues
when no eligible node is live"), add jest.useFakeTimers() at the beginning and
replace each await new Promise((resolve) => setTimeout(resolve, 35)) call with
jest.advanceTimersByTime(35) to deterministically advance time. After each test
completes, call jest.useRealTimers() to restore real timers. This removes the
dependency on tight wall-clock timing margins and makes the tests robust across
varying CI scheduling speeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eea16c22-7703-4f82-8ae6-17da57f73c31
📒 Files selected for processing (9)
.workflow-artifacts/factory-p12-node-placement/claude-fix.md.workflow-artifacts/factory-p12-node-placement/claude-review.md.workflow-artifacts/factory-p12-node-placement/self-reflection.md.workflow-artifacts/factory-p12-node-placement/signoff.mdpackages/sdk/src/messaging/index.tspackages/sdk/src/messaging/placement.test.mtspackages/sdk/src/messaging/relaycast.tspackages/sdk/src/messaging/types.tspackages/sdk/src/messaging/vitest.placement.config.mts
…e-placement # Conflicts: # packages/sdk/src/messaging/relaycast.ts # packages/sdk/src/messaging/types.ts
Resolve merge with main (node `live` optionality) and apply codex/CodeRabbit review feedback on node-targeted placement: - CHANGELOG: add the `[Unreleased] Added` entry for `placement.spawn` / `RelayPlacementError` (codex P1). - placementActionInput: reject a spawn whose explicit `input.cli` does not match the `spawn:<cli>` capability instead of silently dispatching the wrong harness (codex P2); throws RelayPlacementError(capability_mismatch). - placement.spawn fail/expiry: emit `RelayPlacementError.code = 'unmapped_repo'` when the failure reason is an unmappable repo, so the public code is reachable (CodeRabbit major). - reconcilePlacement/logPlacement: wrap caller-provided onReconcile/log hooks in try/catch so a throwing observability sink can't break a valid placement (CodeRabbit major). - Keep main's optional `RelayNode.live` (placement checks are already null-safe). - Remove factory build artifacts (.workflow-artifacts/factory-p12-node-placement) and the unrelated bot-authored memory/INCIDENT-*.md from the PR diff. - Tests: add cli-mismatch reject, cli-match overwrite, unmapped_repo fail-fast, and throwing-onReconcile isolation cases (15 placement tests pass). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Targeted placement by node name/self, repo-path-aware eligibility, reject-and-reconcile for unmapped repos. Minimal #1056 §6 slice; no least-loaded scheduler.
Factory issue p12 (node-placement). Built autonomously via the relayflows factory squad loop (standard review).
Spec: factory/planning/linear-issue-factory-fleet-p12-node-placement.md
Review