Defer instruction-only thread reconfigure while background tasks are open - #1219
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request for security, code quality, performance, tests, and architectural duplication. |
| // live work over: defer the instruction-only reconfigure — the stored | ||
| // config stays stale, so it retries once the tasks settle. Explicit | ||
| // settings changes (e.g. a model switch) still apply immediately. | ||
| if (sameSettings && backgroundWorkState.hasOpenWorkForThread(args.threadId)) { |
There was a problem hiding this comment.
🚨 slopcop/review — Prettier rejects this changed line.
The format check also rejects runtime.lifecycle.test.ts at line 514. Please run Prettier on both changed files before merge. This note is non-blocking.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: A thread can start a long job. New instructions now wait until that job ends. The job stays alive.
Result: I found no blocking defect.
-
Format: One low-severity issue remains. Prettier rejects two changed lines. See the inline note.
-
Security: I found no security defect. A task can delay instruction updates indefinitely. The pull request defines this behavior. Permission changes still apply immediately.
-
Quality and architecture: The change uses the existing
RuntimeBackgroundWorkState. I found no duplicate mechanism. More tests for repeated defer and thread isolation would improve coverage. -
Performance: The new lookup uses the existing per-thread map. It adds no scan, queue, or timer.
-
Validation: The agent-runtime type check passed. The focused lifecycle tests passed 27 of 27. The background-state tests passed 5 of 5.
The full package run passed 838 tests. One unchanged stderr-tail test failed. The same test failed alone at runtime.process-lifecycle.test.ts:461.
A browser test does not apply because this daemon runtime change has no UI route.
Three GPT-5.6 workers reviewed the change in parallel. The internal GPT-5.6 gate returned APPROVE. I used only a comment review on GitHub.
2 participants
Fixes #1217.
Summary
When a turn arrives with instructions that differ from what the session was started with,
reconfigureThreadIfNeededissuesthread/resume. For claude-code that replaces the live CLI session: every running background task is killed with it, and the adapter settles them as interrupted — in the same second as the user's message.Instructions drift between turns without any user action: the Memory plugin renders the memory catalog into thread instructions, so any memory write armed the next message to silently kill all running background tasks;
AGENTS.mdedits and plugin dynamic instructions do the same.An instruction-only change is now deferred while the thread has open background tasks: the stored runtime config stays stale, so the reconfigure retries on the first turn after the tasks settle. Explicit execution-setting changes (e.g. a model switch) still apply immediately.
The change is daemon-local (no wire shape change), so
HOST_DAEMON_PROTOCOL_VERSIONis untouched. The new regression test reproduces the kill without the fix: a turn with changed instructions and an open background task previously emittedthread/resume, now it does not, and the deferred instructions apply once the task completes.