Skip to content

feat(planner): generate plans as validated task files instead of a reply - #2588

Merged
integry merged 8 commits into
mainfrom
feat/file-based-plan-generation
Sep 28, 2026
Merged

integry merged 8 commits into
mainfrom
feat/file-based-plan-generation

Conversation

@integry

@integry integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner

Stacked on the fix/plan-generation-integrity PR (PR A): merge that first. GitHub retargets this PR to main once A merges.

Why

Plan 027d8f35's regeneration (Opus 5.5, 129,562 output tokens) was cut at the per-message output limit. Only the last 3,542 characters reached the parser, and a malformed "task" was saved. As long as the plan is taken from the final chat message, any plan larger than one message is at risk.

What

  • File contract: the planning agent writes one JSON task object per file: tasks/001.json, tasks/002.json, and so on, in plan order. It then runs node validate-plan.mjs --tasks tasks and fixes the files it names until the validator exits 0. Each task is a separate tool call, so neither the reply nor any single Write has to hold the whole plan.
  • Validator (planValidation.ts, still one source of truth): an additive --tasks <dir> mode assembles the files in name order into plan.json. Errors name the file (task 2 (tasks/002.json) has no non-empty "body"). Without the flag, output is unchanged.
  • Runner (planFileAgent.ts):
    • A taskFiles option. ProPR reads the task files back and assembles and validates them with its own copy of the validator.
    • It reads only regular files, never following a symlink out of the workspace, with bounds on file count and size.
    • It raises the task's turn limit to 200: this host and .env.example ship CLAUDE_MAX_TURNS=10, and a granular plan needs a turn per task.
    • A new AgentTaskOptions.maxTurns field, which Claude uses; agents without a turn limit ignore it.
  • Generation (planFileGeneration.ts): the existing planner prompt (fullContext) plus the file contract, which replaces the reply format. It hooks into callLLMForPlan after the token check, estimation and trace update. Granularity enforcement still applies.
  • Lazy loading: the agent registry, model aliases and log helpers now load on use in the runner, so importing planning modules (and their tests) opens no database or queue connections.

Mode, default and fallback

  • PROPR_PLAN_GENERATION_MODE: file (default) or response (the previous reply parsing).
  • Why file is the default:
    • It removes the whole class of failures from truncated or malformed replies.
    • Every agent type (claude, codex, opencode, antigravity, vibe) already runs implementation tasks with file tools through executeTask.
    • Every agent image is built on node:22, so the validator runs everywhere.
  • Cost: each tool turn re-reads the (cached) planner context. For large contexts that means more cache-read tokens and somewhat longer runs than a single reply. response stays available as a switch.
  • Fallback:
    • Only a PlanFileAgentUnavailableError falls back to response for that run: the workspace couldn't be created, no agent could be resolved, or executeTask threw before producing anything.
    • An agent that ran but wrote an invalid or incomplete plan fails the generation with the validator's errors. Regenerating as a reply would double a potentially 15-minute run and hit the same model limits.
    • UsageLimitError propagates unchanged, so requeueing still works.

Tests

  • test/planFileGeneration.test.ts (12 tests) covers:
    • task-file assembly and per-file errors
    • an empty tasks directory
    • unchanged plain-mode messages
    • the fake-agent end-to-end path through callLLMForPlan, including granularity enforcement
    • a workspace validator tampered with and a symlinked task file, both ignored
    • "ran but wrote nothing" treated as a failure, not a fallback
    • executeTask throwing leading to fallback, with usage limits propagated
    • mode selection and the prompt contract
  • test/planGenerationJsonRepair.test.ts is pinned to response mode, since it covers reply parsing.
  • Passing: planner, refinement, trace isolation, llmLogger, agent registry and reasoning-level suites (166 tests), plus a core typecheck of the changed files and eslint.

Not verified

  • No real agent run of file mode yet: CI and the tests use a fake agent. The first real generation should be watched, especially with Codex, OpenCode, Antigravity and Vibe, to confirm each follows the one-file-per-task contract and runs the validator.
  • Docs: docs/docs/features/planning.md and docs/docs/operations/configuration-reference.md (PROPR_PLAN_GENERATION_MODE, PROPR_PLAN_WORKSPACE_ROOT).

🤖 Generated with Claude Code

proprdev and others added 2 commits September 28, 2026 22:18
The planning agent now writes one JSON task per file under tasks/ in a
scratch workspace and runs `node validate-plan.mjs --tasks tasks` until
it passes; ProPR re-assembles and re-validates the files with its own
validator before accepting the plan. The plan no longer depends on the
agent's final chat message, which is cut at the per-message output
limit for very large plans, and no single tool call has to hold the
whole plan.

- validate-plan.mjs gains an additive --tasks mode that assembles task
  files in name order and names the file behind each error.
- runPlanFileAgent gains a task-file contract, reads back only regular
  files (never a symlink out of the workspace), bounds what it reads,
  raises the task's turn limit (Claude ships CLAUDE_MAX_TURNS=10) and
  reports setup/agent failures as PlanFileAgentUnavailableError; usage
  limits keep their type.
- PROPR_PLAN_GENERATION_MODE=file (default) | response. Only an
  unavailable agent falls back to the reply path; an invalid plan fails.
- Heavy modules (agent registry, model aliases, log helpers) load on
  use, so planning modules and their tests stay free of connections.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…pair tests

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@integry
integry force-pushed the feat/file-based-plan-generation branch from 02ad806 to 6fd0b5d Compare September 28, 2026 20:19
Base automatically changed from fix/plan-generation-integrity to main September 28, 2026 21:38
@integry integry added the AI label Sep 28, 2026
@integry

integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner Author

/merge

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

🔀 Resolved merge conflicts from main into feat/file-based-plan-generation in commit 052c07f

Conflicting Files

  • packages/core/src/services/taskPlanning/planValidation.ts

Resolution Summary

Resolved the conflict in packages/core/src/services/taskPlanning/planValidation.ts.

  • Conflict: The PR added task-file-aware diagnostics, while main replaced approximate content matching with structural validation of tasks, fields, and ordering.
  • Resolution: Kept main’s stronger validation and incorporated the PR’s filename-aware error labels. Preserved incremental task-file assembly.
  • Reason: This retains file-based generation while preventing repairs from dropping, duplicating, reordering, or altering task content.

Refreshed the generated JavaScript containing the same conflict. The repository-wide scan found no remaining unresolved conflict markers; remaining matches are intentional strings or Markdown separators.

Validation passed: TypeScript checking, JavaScript syntax checks—including the embedded validator—and all three focused test files using demo mode.

Git could not mark the file resolved in its index because creating index.lock returned Permission denied. The file contents are resolved; no permission repairs were attempted.


🤖 Resolution Details

  • Model: gpt-6-astra
  • Time: 3m 13s

View Task Execution


System-triggered merge conflict resolution

Automatically resolved merge conflicts after merging target branch into PR branch.
Resolved conflicts in:
- packages/core/src/services/taskPlanning/planValidation.ts

PR: #2588
Model: gpt-6-astra
@integry

integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner Author

/review astra

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ AI Code Review Complete requested by @integry

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

🔍 AI Code Review — astra

Overall Evaluation

The file-based approach addresses reply truncation, but the PR needs changes before merge: it can still accept partial plans, read outside the workspace, and incorrectly restart generation after an agent has produced output.

✅ Independent validation — ProPR validates copied task contents using its own validator, so editing the workspace validator cannot bypass schema checks.

✅ Granularity preserved — The new generation path applies enforceGranularity before returning.

This review is based on static tracing of the supplied code; no commands were run. Current checks show 18 passed, 5 pending, and none failed.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F1: 🔴 File bounds silently discard tasks

  • Required behavior: File-based generation must reject incomplete output rather than save a partial plan; read limits must not silently remove generated tasks.

  • Evidence: packages/core/src/services/taskPlanning/planFileAgent.ts, readTaskFiles and readWorkspaceFile.

    1. An agent writes 201 valid task files, or writes two tasks with one file exceeding 8 MiB.
    2. slice(0, MAX_TASK_FILES) discards task 201; independently, readWorkspaceFile returns null for the oversized file and the caller omits it.
    3. The remaining tasks pass validatePlanTaskFiles, and runPlanFileAgent returns them as the generated plan.
    4. The caller receives a successful but incomplete plan, with no indication that generated tasks were dropped.

    The private validator only sees the filtered files, so it cannot detect the loss. The turn limit does not enforce file count, and the prompt does not establish either size or count restrictions. static trace: both discard paths and subsequent successful validation are explicit in the supplied implementation.

  • Minimum fix: Reject generation with a clear diagnostic when the file count exceeds the limit or any matching task file cannot be accepted, instead of validating a reduced set. Cover both count overflow and oversized-file cases.

F2: 🔴 Failed execution can save an unfinished plan

  • Required behavior: An agent that ran but produced an incomplete plan must fail generation rather than save the completed prefix.

  • Evidence: packages/core/src/services/taskPlanning/planFileAgent.ts, runPlanFileAgent, handling of result.success.

    1. The generation agent writes a complete tasks/001.json.
    2. Execution fails before it writes the remaining tasks, returning { success: false, error: 'execution failed' }. The supplied routing implementation explicitly permits unsuccessful results to reach this caller.
    3. The runner uses the failure only to construct diagnostic text, then validates the existing file.
    4. Its fields pass validation, so the runner returns the one-task prefix as a successful plan.

    Schema validation establishes that individual files are complete objects; it does not establish that generation finished. The existing unsuccessful-agent test writes no files, so it does not exercise this sequence. static trace: the unsuccessful result is never used to reject an otherwise valid task-file set.

  • Minimum fix: Reject unsuccessful task-file generation even when the files already written pass schema validation. Add a regression where an agent writes one valid task and then returns an unsuccessful result.

F3: 🔴 Symlinked task directory escapes the workspace

  • Required behavior: The runner must read only regular files within the workspace and never follow a symlink outside it.

  • Evidence: packages/core/src/services/taskPlanning/planFileAgent.ts, readTaskFiles and readWorkspaceFile.

    1. The agent replaces the writable tasks directory with a symlink to an outside directory containing a valid task JSON file.
    2. readdir(directory) follows that directory symlink.
    3. lstat(workspace/tasks/001.json) follows the intermediate tasks symlink and reports the final target as a regular file.
    4. readFile reads the outside file, and its contents can be returned as a plan.

    Checking the final file with lstat rejects a symlink at the file itself, but does not reject a symlink in its parent path. The existing test covers only a symlinked task file. static trace: the task directory is never checked before enumeration or reads; no concurrent replacement is needed for this sequence.

  • Minimum fix: Reject a symlinked task directory and verify that task reads remain within the actual workspace. Add a regression that replaces tasks itself with a symlink to an outside directory.

F4: 🔴 Exceptions after output incorrectly trigger fallback

  • Required behavior: Response fallback is allowed only when execution throws before producing anything; an agent that has produced invalid or incomplete output must fail without restarting generation.

  • Evidence: packages/core/src/services/taskPlanning/planFileAgent.ts, the executeTask catch; packages/core/src/services/taskPlanning/planFileGeneration.ts, tryGeneratePlanWithFiles.

    1. An agent writes a task file and subsequently throws a non-usage-limit error.
    2. The catch unconditionally converts that error into PlanFileAgentUnavailableError, without inspecting whether output exists.
    3. Workspace cleanup deletes the generated files.
    4. tryGeneratePlanWithFiles returns null, causing callLLMForPlan to run a second generation through reply parsing.

    This discards already-produced work and repeats a potentially expensive generation using the output mechanism this PR is intended to avoid. Preserving UsageLimitError does not protect other failures after output. static trace: every other execution rejection takes this branch regardless of workspace contents. proposed regression: use the supplied fake-agent seam to write a task and then throw; assert that reply generation is not called.

  • Minimum fix: Before classifying an execution exception as unavailable, establish that no plan output was produced. If output exists, propagate a planning failure without response fallback, while preserving usage-limit propagation.

Suggestions

These are optional follow-ups and are not sent to /fix.

S1: 🟢 Exercise real agent integrations

The fake-agent tests verify orchestration, but do not establish that each supported CLI follows the file contract in its actual container. A small real generation per agent would improve rollout confidence, particularly because file mode becomes the default. This is optional integration evidence, separate from the demonstrated code defects above.

Score

The approach and independent validation are sound foundations, but four correctness and boundary defects remain. Passing checks do not cover these failure sequences, and five checks are still pending.

Score: 5/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 1m 6s
  • Tokens: 55,004 (52,982 in / 2,022 out)
  • Cost: $0.63

View Task


💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

🤖 Review by ProPR

@integry

integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner Author

/fix

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ Applied the requested follow-up changes in commit 8c44459

Addressed 1 AI review comment (IDs: 5879704313)

Addressed findings F1, F2, F3, F4

Summary of Changes

Implemented F1–F4 only:

  • Reject excessive file counts and unacceptable task files.
  • Reject unsuccessful generation even when valid tasks exist.
  • Reject symlinked task directories and files.
  • Prevent fallback after output or an inconclusive output check; preserve usage-limit errors.

Added focused regressions and checked shared paths and callers. All 57 relevant tests passed, plus core typecheck, ESLint, and diff checks. No visual preview needed.


🤖 Implementation Details

  • Model: GPT-6 Astra (high)
  • Requested By: @integry
  • Time: 5m 15s
  • Tokens: 435,360 (427,940 in / 7,420 out)
  • Cost: $1.35

Undo Changes • View Task Execution


💡 ProPR Slash Commands
Command Description Example
/merge Merge target branch into this PR and resolve conflicts /merge
/review Request an AI code review /review or /review claude-sonnet
/fix Implement fixes for issues found by /review /fix or /fix address the null check issue
/switch Change the AI model for this PR /switch claude-opus
/use Override the model for a single follow-up run /use claude-sonnet
/ultrafix Loop review→fix cycles until score goal is met /ultrafix or /ultrafix goal=8 max=10

🤖 Generated by ProPR
Processing comment ID: 5879713126✓

Implemented F1–F4 only:

- Reject excessive file counts and unacceptable task files.
- Reject unsuccessful generation even when valid tasks exist.
- Reject symlinked task directories and files.
- Prevent fallback after output or an inconclusive output check; preserve usage-limit errors.

Added focused regressions and checked shared paths and callers. All 57 relevant tests passed, plus core typecheck, ESLint, and diff checks. No visual preview needed.

PR: #2588
Comment by: @integry (ID: 5879713126)
Model: gpt-6-astra
@integry

integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner Author

/review astra

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ AI Code Review Complete requested by @integry

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

🔍 AI Code Review — astra

Overall Evaluation

The PR addresses reply truncation with incremental task files and independent validation. It needs changes before merge because synthetic routing retries can mix tasks from different generation attempts.

✅ Independent validation — ProPR validates copied task contents with its own validator, rejecting malformed tasks even when the workspace validator is modified.

✅ Controlled fallback — Execution failures after output exists do not trigger reply regeneration; thrown usage-limit errors retain their identity.

This review is based on the supplied code; no commands were run. Current checks show 19 passed, four pending, and no failures.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F5: 🔴 Isolate task files across routing retries

  • Required behavior: A saved plan must contain the successful generation attempt’s tasks, without silently including output from an unsuccessful attempt.

  • Evidence: packages/core/src/services/taskPlanning/planFileAgent.ts, runPlanFileAgent and readTaskFiles; packages/core/src/services/taskPlanning/planValidation.ts, task-file assembly.

    1. File generation uses a synthetic routing session. Its first physical agent writes valid 001.json, 002.json, and 003.json, then returns a retryable failure.
    2. The supplied SyntheticRoutingSession.executeTask implementation retries another physical agent with the same options and therefore the same workspace. The workspace is prepared only before entering that routing call.
    3. The second agent produces a complete two-task plan by writing 001.json and 002.json. Nothing clears the first attempt’s 003.json. Running the validator succeeds because all three files contain structurally valid tasks.
    4. The second execution returns success. runPlanFileAgent reads every .json file and returns all three tasks, including the abandoned attempt’s task. The caller receives a contaminated plan for subsequent persistence.

    The success check observes only the routing session’s final result. Neither validator tracks which attempt produced each file, and the supplied fake-agent tests do not exercise multiple physical attempts sharing a workspace. Verification provenance — static trace: the changed workspace lifecycle and assembly logic, together with the supplied routing retry loop. Proposed regression: fail an initial three-file attempt, succeed with a two-file replacement attempt, and assert that only the replacement tasks are returned.

  • Minimum fix: Give each physical generation attempt an empty task directory or isolated workspace, and validate only the successful attempt’s output. Apply the isolation between internal routing attempts, not merely before calling the routing session.

Suggestions

These are optional follow-ups and are not sent to /fix.

S2: 🟢 Exercise real agent adapters

A monitored smoke run across the supported agents would verify workspace access, tool permissions, validator execution, and turn-limit behavior. The fake-agent coverage verifies orchestration well, but cannot establish those adapter behaviors. This is optional integration confidence work, separate from the demonstrated retry defect.

Score

The implementation has useful validation and failure-path coverage, but accepting stale tasks across routing retries is a plan-integrity blocker. The supplied checks show no failures, with four test shards still pending.

Score: 6/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 50s
  • Tokens: 60,213 (58,734 in / 1,479 out)
  • Cost: $0.56

View Task


💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

🤖 Review by ProPR

@integry

integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner Author

/fix

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ Applied the requested follow-up changes in commit 9e07c8a

Addressed 1 AI review comment (IDs: 5879872693)

Addressed finding F5

Summary of Changes

Implemented F5: each physical routing attempt gets a fresh workspace, and only the final attempt’s output is validated. Earlier output still prevents inappropriate response fallback.

Added regressions for retry contamination, empty replacements, late writes, and preparation failures.

All 75 relevant tests passed, along with core typecheck and ESLint. No visual preview needed.


🤖 Implementation Details

  • Model: GPT-6 Astra (high)
  • Requested By: @integry
  • Time: 4m 35s
  • Tokens: 485,144 (479,287 in / 5,857 out)
  • Cost: $1.18

Undo Changes • View Task Execution


💡 ProPR Slash Commands
Command Description Example
/merge Merge target branch into this PR and resolve conflicts /merge
/review Request an AI code review /review or /review claude-sonnet
/fix Implement fixes for issues found by /review /fix or /fix address the null check issue
/switch Change the AI model for this PR /switch claude-opus
/use Override the model for a single follow-up run /use claude-sonnet
/ultrafix Loop review→fix cycles until score goal is met /ultrafix or /ultrafix goal=8 max=10

🤖 Generated by ProPR
Processing comment ID: 5879884452✓

…workspace, and only the final attempt’s output is validated. Earlier output still prevents inappropriate response fallback.

Implemented F5: each physical routing attempt gets a fresh workspace, and only the final attempt’s output is validated. Earlier output still prevents inappropriate response fallback.

Added regressions for retry contamination, empty replacements, late writes, and preparation failures.

All 75 relevant tests passed, along with core typecheck and ESLint. No visual preview needed.

PR: #2588
Comment by: @integry (ID: 5879884452)
Model: gpt-6-astra
@integry

integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner Author

/ultrafix

@propr-dev propr-dev Bot added the ultrafix label Sep 28, 2026
@propr-dev

propr-dev Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

🔄 Ultrafix loop started (goal: 8/10, max cycles: 10)

First action: /review

💡 Tip: Remove the ultrafix label from this PR to stop further ultrafix cycles.

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ AI Code Review Complete requested by @integry

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

🔍 AI Code Review — codex:gpt-6-astra

Overall Evaluation

The PR addresses reply truncation with per-task files and independent validation. It needs a localized correction to failure propagation before merge.

✅ Independent validation — ProPR assembles copied task files using its own validator, so editing the workspace validator cannot authorize invalid output.

✅ Retry isolation — Each routed attempt receives a fresh workspace, with tests covering abandoned files, late writes, and cleanup.

The supplied current-head checks report 24 passed and no failures or pending checks. This review used the supplied code only; no commands were run.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F6: 🔴 Preserve explicit cancellation before fallback

  • Required behavior: Explicit cancellation must remain terminal; it must not be classified as file-agent unavailability and restart generation through the response path.

  • Evidence: packages/core/src/services/taskPlanning/planFileAgent.ts, runPlanFileAgent execution catch; packages/core/src/services/taskPlanning/planFileGeneration.ts, tryGeneratePlanWithFiles; packages/core/src/services/taskPlanning/llmCalling.ts, response fallback.

    1. A generation agent throws ExecutionAbortedError before writing output. The supplied agent contract supports task cancellation through taskId, which this runner passes.
    2. SyntheticRoutingSession.executeTask recognizes that error as non-retryable and propagates it without selecting another agent.
    3. The new runner catch preserves only UsageLimitError. With no output present, it converts the cancellation into PlanFileAgentUnavailableError.
    4. tryGeneratePlanWithFiles consequently returns null, and callLLMForPlan invokes runLightweightLLMAnalysis for another generation attempt instead of propagating cancellation.

    The routing layer’s terminal-error protection is undone by the outer catch. Output inspection establishes whether files exist, not whether another invocation is authorized. The observable defect is dispatching the response-generation fallback after explicit cancellation; whether that downstream invocation independently rejects cancellation is not shown in the supplied context.

    static trace: verified against the supplied catch branches and isNonRetryableSyntheticFailure. Existing tests cover generic execution errors and usage limits, but not explicit cancellation.

  • Minimum fix: Propagate explicit cancellation and other terminal execution errors before classifying failures as unavailability. Add a regression asserting cancellation retains its identity and does not call response generation.

Suggestions

These are optional follow-ups and are not sent to /fix.

S3: 🟢 Exercise real agent adapters

A monitored smoke generation with each supported adapter would supplement the fake-agent coverage by checking workspace access, validator execution, and completion reporting. This is an optional integration follow-up: the supplied review evidence does not establish an adapter-specific defect.

S4: 🟢 Document practical plan limits

Document the 200-file ceiling and allow turn-budget headroom for validation when describing maximum plan sizes. The current boundary test writes 200 files directly through a fake agent, so it does not demonstrate that an agent can write that many tasks and finish validation within 200 turns. This is optional operational clarification.

Score

The implementation has substantial focused coverage and passing current-head checks. The cancellation-to-fallback conversion requires a small but important correction.

Score: 6/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 55s
  • Tokens: 60,867 (59,237 in / 1,630 out)
  • Cost: $0.57

View Task


💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

🤖 Review by ProPR

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ Applied the requested follow-up changes in commit 56f739c

Addressed 1 AI review comment (IDs: 5880209569)

Addressed finding F6

Summary of Changes

Implemented F6 only. Explicit cancellation and other terminal execution errors retain their identity before output inspection, preventing response fallback. The shared repair path is covered too.

Added 12 regressions. All 69 targeted tests passed, along with core typecheck, ESLint, and diff checks.

No visual preview needed for this backend change.


🤖 Implementation Details

  • Model: GPT-6 Astra (high)
  • Requested By: @propr-ultrafix
  • Time: 3m 41s
  • Tokens: 556,343 (552,367 in / 3,976 out)
  • Cost: $1.13
  • Subscription usage: Weekly +1%

View Task Execution


💡 ProPR Slash Commands
Command Description Example
/merge Merge target branch into this PR and resolve conflicts /merge
/review Request an AI code review /review or /review claude-sonnet
/fix Implement fixes for issues found by /review /fix or /fix address the null check issue
/switch Change the AI model for this PR /switch claude-opus
/use Override the model for a single follow-up run /use claude-sonnet
/ultrafix Loop review→fix cycles until score goal is met /ultrafix or /ultrafix goal=8 max=10

🤖 Generated by ProPR

…al execution errors retain their identity before output inspection, preventing response fallback. The shared repair path is covered too.

Implemented F6 only. Explicit cancellation and other terminal execution errors retain their identity before output inspection, preventing response fallback. The shared repair path is covered too.

Added 12 regressions. All 69 targeted tests passed, along with core typecheck, ESLint, and diff checks.

No visual preview needed for this backend change.

PR: #2588
Comment by: @propr-ultrafix (ID: 0)
Model: gpt-6-astra
@integry

integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner Author

/review astra

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ AI Code Review Complete requested by @integry

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

🔍 AI Code Review — astra

Overall Evaluation

The PR addresses reply truncation with incremental task files and independent validation. It needs a localized correction before merge: response fallback reuses an exhausted synthetic routing session.

✅ Independent validation — ProPR validates copied task contents with its own validator, rejecting incomplete objects and workspace-validator tampering.

✅ Attempt isolation — Each routed execution receives a fresh workspace, preventing abandoned task files from contaminating a successful retry.

✅ Failure coverage — Added tests cover invalid output, file bounds, symlinks, cancellation, and retry isolation.

This review is based on static inspection; no commands were run. The supplied current checks show zero failures and eight pending checks.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F7: 🔴 Reset routing for response fallback

  • Required behavior: When file execution throws before producing output, the advertised response fallback must be able to invoke response generation.

  • Evidence: packages/core/src/services/taskPlanning/llmCalling.ts:100, callLLMForPlan; packages/core/src/services/taskPlanning/planFileAgent.ts, runPlanFileAgent error handler.

    1. Generation uses a real synthetic routing session whose eligible members encounter retryable execution failures before writing any output.
    2. SyntheticRoutingSession.executeTask marks each selected member attempted, clears failed selections, and eventually throws when no eligible members remain.
    3. runPlanFileAgent establishes that all workspaces are empty and converts the failure to PlanFileAgentUnavailableError. tryGeneratePlanWithFiles returns null, selecting response fallback.
    4. callLLMForPlan passes the same opts.routingSession to runLightweightLLMAnalysis. That session still excludes every attempted member; subsequent selection throws again without allowing a response invocation.

    Users receive a failed generation despite reaching the explicitly supported fallback branch. The route cannot try response generation even if the preceding transport failure was transient. Workspace isolation and output validation do not reset routing eligibility. Existing fallback tests use stateless fake sessions, so they do not exercise this state transition.

    static trace: The supplied routing implementation retains attemptedMemberIds across calls and rejects those members in inspectMember; its fork() method creates a distinct logical call. No regression test was executed.

  • Minimum fix: When falling back after an unavailable file execution, use a fresh routing session, such as routingSession.fork(), for response generation. Add a regression using the real routing loop that exhausts file execution without output and verifies a subsequent response invocation.

Suggestions

These are optional follow-ups and are not sent to /fix.

S5: 🟢 Exercise real agent adapters

The fake-agent tests establish orchestration behavior but do not verify that real adapters follow the file contract in the scratch workspace. A monitored smoke generation for each supported adapter would strengthen rollout confidence, particularly around workspace access and validator execution. This is optional integration validation, not a demonstrated adapter defect.

Score

The implementation has strong validation and retry-isolation coverage, but the synthetic-routing fallback defect requires correction. Current checks have no reported failures, with eight still pending.

Score: 6/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 51s
  • Tokens: 59,708 (58,197 in / 1,511 out)
  • Cost: $0.60

View Task


💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

🤖 Review by ProPR

@integry

integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner Author

/fix

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ Applied the requested follow-up changes in commit 5b61103

Addressed 1 AI review comment (IDs: 5880299679)

Addressed finding F7

Summary of Changes

Implemented F7 only: response fallback now forks the routing session, restoring member eligibility after file execution exhausts the pool.

Regressions cover thrown and returned failures, response retries, and explicit response mode. Related callers revealed no additional occurrences.

Validation: 76 tests passed; core typecheck passed; lint reported only a test-file length warning. No visual preview needed.


🤖 Implementation Details

  • Model: GPT-6 Astra (high)
  • Requested By: @integry
  • Time: 4m 10s
  • Tokens: 622,756 (617,502 in / 5,254 out)
  • Cost: $1.39

Undo Changes • View Task Execution


💡 ProPR Slash Commands
Command Description Example
/merge Merge target branch into this PR and resolve conflicts /merge
/review Request an AI code review /review or /review claude-sonnet
/fix Implement fixes for issues found by /review /fix or /fix address the null check issue
/switch Change the AI model for this PR /switch claude-opus
/use Override the model for a single follow-up run /use claude-sonnet
/ultrafix Loop review→fix cycles until score goal is met /ultrafix or /ultrafix goal=8 max=10

🤖 Generated by ProPR
Processing comment ID: 5880340697✓

…g session, restoring member eligibility after file execution exhausts the pool.

Implemented F7 only: response fallback now forks the routing session, restoring member eligibility after file execution exhausts the pool.

Regressions cover thrown and returned failures, response retries, and explicit response mode. Related callers revealed no additional occurrences.

Validation: 76 tests passed; core typecheck passed; lint reported only a test-file length warning. No visual preview needed.

PR: #2588
Comment by: @integry (ID: 5880340697)
Model: gpt-6-astra
@integry

integry commented Sep 28, 2026

Copy link
Copy Markdown
Owner Author

/review astra

@propr-dev

propr-dev Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ AI Code Review Complete requested by @integry

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

🔍 AI Code Review — astra

Overall Evaluation

The PR replaces reply-based plan delivery with individually written task files and host-side validation. The implementation is coherent and merge-ready within the reviewed scope, conditional on the remaining checks passing.

✅ Independent validation — ProPR assembles copied task files using its own validator, so modifying the workspace validator cannot authorize malformed tasks.

✅ Retry isolation — Each physical routing attempt receives a separate workspace, preventing abandoned tasks from contaminating a successful replacement plan.

✅ Controlled fallback — Output from failed attempts prevents response fallback, while explicit thrown cancellation errors retain their identity.

This was a static review of the supplied code; no commands or tests were run. The authoritative status reports 20 passed checks, three pending, and no failures.

Merge blockers

No merge blockers.

Suggestions

These are optional follow-ups and are not sent to /fix.

S6: 🟢 Exercise real agent delivery

Run a representative file-mode generation through the supported physical adapters, including a plan requiring more than ten turns. The fake-agent tests cover orchestration extensively, but cannot establish actual CLI adherence to the file contract or scratch-workspace compatibility. This is useful integration follow-up rather than a demonstrated code blocker.

S7: 🟢 Clarify repair documentation

In docs/docs/features/planning.md, qualify the earlier paragraph about the default coding agent repairing malformed model JSON as response-mode behavior. The new default instead asks the planning agent to fix its task files. Clarifying that distinction would make the two descriptions consistent without changing runtime behavior.

Score

Strong validation and regression coverage support merging within scope. The remaining checks are pending, and physical-agent behavior has not been demonstrated by the supplied tests.

Score: 8/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 42s
  • Tokens: 62,232 (61,098 in / 1,134 out)
  • Cost: $0.56

View Task


💡 Next step: Comment /fix to address every F# merge blocker, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them, and they never relax a merge blocker.

🤖 Review by ProPR

@integry
integry merged commit a4fbd81 into main Sep 28, 2026
42 checks passed
@integry
integry deleted the feat/file-based-plan-generation branch September 28, 2026 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants