Repository navigation
fix(workspace): attach the engine for a message sent while the assistant is busy - #1461
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: 🔵 Low · up to
|
Codex review logContract: claims C1–C4 and residuals R1–R2 in the PR description. Up to 3 rounds; a round with no claim violation ends review.
cubic, first run after the ready flip, on cubic, second run, on Rebase onto main ( |
|
@codex review against the numbered claims (C1–C4) and the disclosed residuals (R1–R2) in this PR's description: report only a reproducible trace that violates a numbered claim. Instances of the disclosed residuals are not findings. A round with no claim violation ends review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ff8985620
ℹ️ 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review against the numbered claims (C1–C4) and the disclosed residuals (R1–R2) in this PR's description: report only a reproducible trace that violates a numbered claim. Instances of the disclosed residuals are not findings. The push since round 1 (bf485dc) changes only |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf485dc6e3
ℹ️ 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".
E2E: before and after, the same stepsSetup: a fresh project linked to a test workspace that declares Snowflake, Altimate, Jira and GitHub (60 tools); no Steps: send Run the bash command main ( No engine process was started. This PR (
|
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit bed0007)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit bed0007)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit 16d4969)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Review was read-only; tests were not executed. Previous review (commit bf485dc)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Review was read-only; tests were not executed. Reviewed by gemini-4-argon · Input: 85.3K · Output: 2.1K · Cached: 317.5K Review guidance: REVIEW.md from base branch |
|
@codex review — confirming round, scoped to the push since your last round (bf485dc..16d4969: the tracker runs the boundary for any request it has not seen, not only the newest, and a test assertion). Review only that change, against the numbered claims (C1–C4) and the disclosed residuals (R1–R3) in this PR's description: report only a reproducible trace in which that change violates a numbered claim. Instances of the disclosed residuals are not findings. A round with no claim violation ends review. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Consensus Code Review — altimate-code PR #1461fix(workspace): attach the engine for a message sent while the assistant is busy (head Code Review SummaryVerdict: APPROVE (no blockers; minor follow-ups) Claims verified by the whole panel:
Minor Issues
Design notes (documented trade-offs, not defects)
Nits
Positive Observations
Unintended Consequences
Rejected panel findings
Finding Attribution
Reviewed by 5 participants: Claude, GPT 5.3 Codex, DeepSeek V4 Pro, Grok 4.3, Qwen 3.6 Plus. GLM-5.2 was dropped mid-run on the user's instruction (relayed by a peer session) and replaced with Qwen 3.6 Plus. Convergence: 1 round. DeepSeek, Grok and Qwen approved. GPT asked for a reclassification, which was incorporated. |
sahrizvi
left a comment
There was a problem hiding this comment.
Verdict: approve with comments. No critical or major findings. The minor findings and nits are in the consensus review comment; please address them here or in a follow-up.
…ant is busy Such a message joins the running prompt loop, and the engine's turn boundary ran only on a loop's first catalog. It now runs on the first catalog of each user request, counted by lesson retrieval's rule (moved into `UserRequest.requestID`), so the loop's own messages keep the turn's pin. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRUhVhg6XwxSKJs1iTufiA
After a compaction that keeps a tail, the loop's history is not in id order (`filterCompacted` places the summary after the retained tail), so a positional scan could pick the retained request and skip a message queued during the compaction. Same rule as `MessageV2.latest`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRUhVhg6XwxSKJs1iTufiA
…y the newest A stamped compaction replay is newest by id but maps back to a request already seen, so checking only the newest could hide a message queued during the compaction. Every request in history is checked now, which also makes the order of history irrelevant. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRUhVhg6XwxSKJs1iTufiA
16d4969 to
bed0007
Compare
|
@sahrizvi, this needs your re-approval: rebasing onto main after #1431 and #1464 merged dismissed it. The only change from
Compare: The minors from your review go in one follow-up PR, after this merges. |
…ols came from The loop test now checks the linked-project notice is settled again at the queued request's boundary. It fails with the notice settled only at the first catalog. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRUhVhg6XwxSKJs1iTufiA
Consensus Re-Review — altimate-code PR #1461 (head
|
| Earlier finding | Author's response | Panel verdict |
|---|---|---|
Codex P1: picks the newest request by position, which filterCompacted reorders |
Fixed, then superseded by a full-history scan | Resolved (5/5) |
| cubic P1: a stamped replay hides a queued request | Fixed (16d49697d) |
Resolved (5/5) |
| cubic P2: assert the queued message was saved before releasing | Taken | Resolved (5/5) |
| Codex P2: client ids out of send order | Declined, recorded as R3 | Declined, reasoning holds (5/5); see the nuance under Minor 3 |
cubic P2: learnOriginalMessage is caller-controlled |
Declined: same session only, precedent | Declined, reasoning holds (5/5) |
kilo: release the gate in a finally |
Declined: test-only | Declined, reasoning holds (5/5) |
| kilo / earlier consensus Minor 1: queued subtask-only command | Deferred to follow-up (no reply on kilo's thread) | Deferral acceptable; see Minor 1 for what is still due here |
| Earlier consensus Minor 2: full-history rescan | Deferred | Deferral acceptable |
| Earlier consensus Minor 3: missing loop tests | Deferred | Deferral acceptable (partly improved by b6a3a6aade) |
| D1: mid-loop re-pin and re-link | Not addressed | Acceptable; document it (Minor 3) |
| D2: unstamped replay (R1) | Documented as R1 | Acceptable |
Nits: FilePart comment; poll diagnostic |
Deferred | Deferral acceptable (Grok re-noted the poll diagnostic) |
Positive Observations
- The rebase took the consistent resolution, and a test fails on the alternative.
requestIDis one rule shared by lesson retrieval and the engine boundary.- The tracker doesn't depend on history order, and each loop gets its own closure with no module state.
- The real-loop tests fail on main.
- Every earlier bot finding was answered with a commit or with reasoning, against a stated claims and residuals contract.
Missing Tests
All of these were raised before and deferred:
- a queued subtask-only command, in its real shape;
- validator retry and subtask summary through the real loop, expecting one boundary;
- R1, an unstamped replay through the real loop;
- two requests queued before one catalog.
Unintended Consequences
- A boundary is now reachable mid-loop:
prompt.ts:1463-1465. On each queued request this can change the system-prompt notice and the tool order, which misses the prompt cache. It can also re-link the directory engine under other sessions in the same directory. Announcements are deduplicated, so a repeat verdict stays quiet. Mitigation: document it in the residuals (Minor 3). Flagged by: DeepSeek V4 Pro, Qwen 3.6 Plus, Claude - Nothing else: GPT and Grok reported "None found — searched callers, tests, and downstream consumers". No public API, CLI or config contract changed.
Rejected panel findings
- Qwen F2, "tool-only user messages get no request": rejected. Tool parts live on assistant messages, so no user-message writer produces a tool-only user message.
- Qwen F4, "
requestIDlocal shadows the import": rejected. Qwen itself concluded no change is needed, and the import is namespaced asUserRequest.requestID.
Finding Attribution
| Issue | Origin | Type |
|---|---|---|
Queued /review test (turn-boundary.test.ts:67-69) models a type: "subtask" command as text; R4 not listed; kilo thread unanswered |
GPT 5.3 Codex, Qwen 3.6 Plus, DeepSeek V4 Pro, Claude | Consensus (previously raised; deferral accepted) |
Stale "first catalog" docstrings in datamate-manager-gate.ts turnNotice, engine-overlay.ts turnTools/pinTurnTools(firstCatalog), awareness.ts:147 |
Claude, DeepSeek V4 Pro | Unique (DeepSeek would rate NIT) |
| Residuals omit mid-loop notice change and re-pin (cache miss), re-link under other sessions, and the R3 mid-tool-call nuance | Qwen 3.6 Plus, DeepSeek V4 Pro, Claude | Consensus |
settledOutcome spy keyed on boundary count, applied module-wide (also read by precedence.ts:261) |
Qwen 3.6 Plus, Claude | Unique |
Rebase resolution if (boundary) workspaceNotice.settle(...) correct, and the new test guards it |
All 5 | Consensus (positive) |
| Minor 1 wording softened ("this PR or the follow-up") | GPT 5.3 Codex | Convergence fix |
Reviewed by 5 participants: Claude, GPT 5.3 Codex, DeepSeek V4 Pro, Grok 4.3, Qwen 3.6 Plus. Convergence: 1 round, all 4 external models APPROVE. Kimi fallback not needed.
sahrizvi
left a comment
There was a problem hiding this comment.
Approving after the consensus re-review. No critical or major findings remain; the minor items and nits are in #1461 (comment) and can go in a follow-up.
Issue for this PR
Closes #1460
Type of change
What does this PR do?
A message sent while the assistant is busy joins the running prompt loop instead of starting one. The engine's turn boundary (reconcile the engine, then catalog and pin the tools) ran only on a loop's first catalog, so that message kept the run's tools and an engine installed meanwhile never attached for it.
The boundary now runs on the first catalog of each user request in the loop, not of each user message. The loop writes user messages itself: the summary after a subtask, the validator retry, the compaction marker and the continue message. Re-running the boundary on those would re-pin engine tools mid-task, which the pin exists to prevent.
Precedent. What counts as a user request isn't new here. Lesson retrieval in the same loop already decides it (the
lessonsblock before the tool catalog inprompt.ts): a user message with a text part that is neither synthetic nor ignored, or a file part; a compaction replay stamped withmetadata.learnOriginalMessagestays the request it replays. This PR moves that rule unchanged intoUserRequest.requestIDand both callers use it, so the two can't drift apart.TurnBoundary.tracker()replaces thecataloguedflag. It runs the boundary on the loop's first catalog and on the first catalog after a request it hasn't seen. It checks every request in history, not just the newest, so neither a loop-written message nor a replay arriving after a queued one hides it.Claims
Residuals
.altimate-code/learn). Elsewhere, a replay after an overflow counts as a new request and runs the boundary once more, where the request restarts from the summary. With the engine unchanged, that reconcile changes nothing.Overlap: #1431 adds
if (firstCatalog) workspaceNotice.settle(sessionID)on a line this PR renames. Whichever merges second resolves it asif (boundary) …, so the linked-project notice also re-settles for a queued request.How did you verify your code works?
Codex rounds against these claims: review log.
Screenshots / recordings
Not a UI change.
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_01HRUhVhg6XwxSKJs1iTufiA
Summary by cubic
Fixes the workspace engine's turn boundary so a message sent while the assistant is busy attaches the engine on that message, instead of keeping the tools from the previous request.
UserRequest.requestID.Written for commit b6a3a6a. Summary will update on new commits.
Summary by CodeRabbit