Repository navigation
fix(dreamer): forward task fallbackModels & timeout, increase subagent maxSteps and verify batch size - #783
izhangzhihao wants to merge 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Waiting for This PR needs a linked issue with the |
|
Thanks for the PR. We're in a focused stretch on other work right now, so this is queued and we'll review it properly when that lifts. To help us review, could you share what you ran into? This PR changes three separate things:
Knowing which of these fixes a failure you saw, and which is tuning, will let us take the parts that fit quickly. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/plugin/src/agents/hidden-agent-registrations.ts">
<violation number="1" location="packages/plugin/src/agents/hidden-agent-registrations.ts:131">
P1: This raises `dreamer-docs` to 400 steps, but OpenCode 2 still stops it at 60. `AGENT_STEPS` in `v2/hooks/hidden-child.ts` keeps `DREAMER_DOCS_AGENT` and `DREAMER_MEMORY_MAPPER_AGENT` at 60, and the step guard throws once `steps > cap`. Update those entries to 400 too, or the new cap has no effect on that harness.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
View guided diff | Re-trigger cubic
| maxSteps: 60, | ||
| // Documentation investigations can span many files in larger repositories. | ||
| // Keep the read-only proposal loop bounded while allowing it to finish. | ||
| maxSteps: 400, |
There was a problem hiding this comment.
P1: This raises dreamer-docs to 400 steps, but OpenCode 2 still stops it at 60. AGENT_STEPS in v2/hooks/hidden-child.ts keeps DREAMER_DOCS_AGENT and DREAMER_MEMORY_MAPPER_AGENT at 60, and the step guard throws once steps > cap. Update those entries to 400 too, or the new cap has no effect on that harness.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/plugin/src/agents/hidden-agent-registrations.ts, line 131:
<comment>This raises `dreamer-docs` to 400 steps, but OpenCode 2 still stops it at 60. `AGENT_STEPS` in `v2/hooks/hidden-child.ts` keeps `DREAMER_DOCS_AGENT` and `DREAMER_MEMORY_MAPPER_AGENT` at 60, and the step guard throws once `steps > cap`. Update those entries to 400 too, or the new cap has no effect on that harness.</comment>
<file context>
@@ -126,9 +126,9 @@ export function buildHiddenAgentRegistrations(args: {
- maxSteps: 60,
+ // Documentation investigations can span many files in larger repositories.
+ // Keep the read-only proposal loop bounded while allowing it to finish.
+ maxSteps: 400,
overrides: args.dreamerOverrides,
// Lock so a user override can't add the memory surface back.
</file context>
Here is the context and evidence behind each change: --- 1. `fallbackModels` & `timeoutMinutes` forwarding (Bug Fix — high priority) We configured `timeout_minutes: 120` and fallback models (e.g., `gemini-3.8-flash-high` falling back to `claude-opus-4-6-thinking`) under `dreamer.pi.tasks.*`. However: 1. **Timeout ignored:** Tasks doing broad sweeps (`verify-broad`, `map-memories`) were consistently killed at exactly 30 minutes, logging:pi subagent timed out after 1800000ms — saw 112 events ... (model was emitting events but no terminal stopReason reached) pi assistant produced empty text (provider error: Connection error.) |
There was a problem hiding this comment.
🟡 Changes recommended
Nested fallback retries undermine budget accounting, and existing verification and OpenCode finalization tests remain incompatible with the new limits.
3 open issues
What changed in this PR
This PR updates Dreamer task configuration forwarding in Pi and increases investigation limits and verification batch sizes.
Changes:
- Forward task-level fallback models and timeouts to the Pi subagent runner.
- Raise documentation and memory-mapper step limits from 60 to 400.
- Increase verification batches from 5 to 10 memories and update related tests.
| File | Description |
|---|---|
packages/plugin/src/features/magic-context/dreamer/verify.ts |
Increase verification batch size and revise timing comments. |
packages/plugin/src/features/magic-context/dreamer/verify-batches.test.ts |
Update batch, budget, progress, and replay expectations. |
packages/plugin/src/agents/hidden-agent-registrations.ts |
Raise documentation and mapper step limits. |
packages/plugin/src/agent-registration-drift.test.ts |
Assert the new step limits. |
packages/pi-plugin/src/subagent-runner.ts |
Update a comment to describe the configured step cap. |
packages/pi-plugin/src/subagent-runner.test.ts |
Update mapper finalization expectations and provider fixture names. |
packages/pi-plugin/src/dreamer/index.ts |
Forward task fallback models and configured timeouts. |
packages/pi-plugin/src/dreamer/index.test.ts |
Assert forwarded fallback and timeout options. |
packages/e2e-tests/tests/dreamer-mapper-step-cap-pi.test.ts |
Exercise 70 reads below the increased mapper limit. |
Review effort: Balanced
| // give the subprocess a generous ceiling so the signal is the | ||
| // authority (not a second, conflicting wall-clock here). | ||
| timeoutMs: 30 * 60 * 1000, | ||
| fallbackModels: taskConfig?.fallbackModels, |
| maxSteps: 60, | ||
| // A map/verify batch can require many file lookups across larger repositories. | ||
| // Keep the investigation bounded while allowing complex batches to finish. | ||
| maxSteps: 400, |
| */ | ||
|
|
||
| export const VERIFY_BATCH_SIZE = 5; | ||
| export const VERIFY_BATCH_SIZE = 10; |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/pi-plugin/src/dreamer/index.ts">
<violation number="1" location="packages/pi-plugin/src/dreamer/index.ts:623">
P2: This change leaves the existing Dreamer forwarding test expecting the old nested fallback array, so the focused Pi Dreamer test now fails on both runner calls. Update that test to assert that the outer retry owns the fallback chain and the runner receives no nested fallbacks, while retaining its failure-path coverage.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| @@ -628,10 +621,7 @@ function createPiDreamerClient( | |||
| userMessage, | |||
| model: perTaskModel, | |||
| fallbackModels: undefined, | |||
There was a problem hiding this comment.
P2: This change leaves the existing Dreamer forwarding test expecting the old nested fallback array, so the focused Pi Dreamer test now fails on both runner calls. Update that test to assert that the outer retry owns the fallback chain and the runner receives no nested fallbacks, while retaining its failure-path coverage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/pi-plugin/src/dreamer/index.ts, line 623:
<comment>This change leaves the existing Dreamer forwarding test expecting the old nested fallback array, so the focused Pi Dreamer test now fails on both runner calls. Update that test to assert that the outer retry owns the fallback chain and the runner receives no nested fallbacks, while retaining its failure-path coverage.</comment>
<file context>
@@ -620,7 +620,7 @@ function createPiDreamerClient(
userMessage,
model: perTaskModel,
- fallbackModels: taskConfig?.fallbackModels,
+ fallbackModels: undefined,
timeoutMs: (taskConfig?.timeoutMinutes ?? 30) * 60 * 1000,
cwd: dreamSession.directory,
</file context>
| @@ -628,10 +621,7 @@ function createPiDreamerClient( | |||
| userMessage, | |||
| model: perTaskModel, | |||
| fallbackModels: undefined, | |||
There was a problem hiding this comment.
Setting fallbackModels to undefined fixes the nested retries, but shared curate validation retries Pi pseudo-tool-call text with the fallback model still expects the runner to receive a fallback array. That assertion at index.test.ts:1237 now fails.
Update it to expect undefined. Keep the model-order checks to verify that the shared executor still tries the fallback.
Approved issue: #784
Harness coverage (required)
For each entry select: covered / not applicable — surface does not exist there / NOT covered.
Summary
fallbackModelsandtimeoutMinutesto the Pi subagent runner, honoring configured fallbacks and timeouts.dreamer-docsanddreamer-memory-mappermaxStepsfrom 60 to 400.VERIFY_BATCH_SIZEfrom 5 to 10.Testing
fallbackModelsandtimeoutMinutesare passed through to the runner.git diff --checkpassed.fix #784
Update the stale fallback assertion before merging so the Pi Dreamer tests pass.
Findings
Summary
The PR forwards task timeouts to Pi, raises the documentation and memory-mapper step limits to 400, and increases verification batches to ten memories.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Task model and fallbacks] --> B[Shared retry loop] B --> C[Pi runner: one model per attempt] C --> D[Return result and token spend] D --> BReviews (2) · Last reviewed commit: "Update packages/pi-plugin/src/dreamer/in..." · Reviewed by Greptile