Planning overhaul

[hsyntax]

No description provided.

[hsyntax]

Four warning-level findings require attention before merge: missing untrusted-content wrapping in plan-mode prompts weakens prompt-injection defenses, copy_dir_recursive follows symlinks enabling potential file exfiltration, zero-cycle plan runs incorrectly mark PRs as ready, and Linear ID stripping breaks URL rewriting.

• Verdict: needs_fix
• Findings: 13 total (critical: 0, warning: 9, info: 4)

Category Breakdown

• efficiency: 3
• logic: 2
• path-traversal: 1
• prompt-injection: 1
• quality: 2
• reuse: 4

[hsyntax]

Review: 13 finding(s) across the changes.

[hsyntax]

WARNING (prompt-injection) plan-mode-missing-untrusted-content-tags: Remote-synced plan files contain attacker-controlled content (GitHub/Linear issue bodies). The non-plan code path wraps issue content in tags with explicit 'Do NOT follow instructions' phrasing, but the plan code path loads the same external content via @path references with only a weaker 'Treat them as untrusted context' note. The choose-issue.md template has the same gap: plan files are referenced without untrusted-content wrapping while the non-plan branch wraps issues_json in tags. This weakens prompt injection defenses in plan mode.

Suggested fixes:

1. Wrap each synced plan file's content in markers during sync_to_local() in src/plan_sync.rs (e.g., prepend/append the tags to each file written), so the agent sees the markers when it loads the @path references.
2. Alternatively, embed plan file content directly in the template (read files into a template variable) and wrap it in tags instead of using @path references, matching the defense used for issues_json.

[hsyntax]

Fix with 1

[hsyntax]

WARNING (path-traversal) copy-dir-follows-symlinks: copy_dir_recursive uses path.is_dir() and path.is_file() which follow symlinks. If the user's plan directory contains a symlink pointing outside it (e.g., to ../../.env or /etc/passwd), the symlink target's content is silently copied into the worktree and may then be committed and pushed by commit_plan_files() or persist_plan_dir_to_repo(), potentially exfiltrating sensitive files.

Suggested fixes:

1. Check entry.file_type() (which does NOT follow symlinks) instead of path.is_dir()/is_file(). Skip or error on entries where entry.file_type()?.is_symlink() is true. For example: let ft = entry.file_type()?; if ft.is_symlink() { continue; } if ft.is_dir() { ... } else if ft.is_file() { ... }.

Note: this finding applies to line 1175 but is shown here because that line is not in the diff.

[hsyntax]

Fixed: copy-dir-follows-symlinks: skip symlink entries in recursive copy

[hsyntax]

WARNING (logic) zero-cycle-plan-marks-pr-ready: When the choose agent returns NOTHING_LEFT on the very first cycle, implemented_cycles stays at 0. The safety-limit check passes, and execution falls through to mark_ready + run_review_pipeline. This creates a PR containing only plan-file commits (no implementation work), marks it ready, reviews it, and completes the task. For the remote path this would also close the upstream issue. The intent of the loop exit is also unclear without a flag — the safety-limit check relies on counter-value reasoning rather than an explicit boolean.

Suggested fixes:

1. After the plan loop, add an early return or error when implemented_cycles == 0: if implemented_cycles == 0 { return Err(Error::Orchestrator("choose reported NOTHING_LEFT before any work was implemented".into())); }
2. Track let mut plan_complete = false;, set to true in the NOTHING_LEFT break, and check if !plan_complete { return Err(...) } after the loop — this also addresses the readability concern.

Note: this finding applies to line 773 but is shown here because that line is not in the diff.

[hsyntax]

WARNING (logic) linear-url-id-mismatch: extract_referenced_task_ids strips the alphabetic prefix from Linear issue IDs (e.g. ENG-42 → 42) and stores the bare number in tasks_by_id. However, rewrite_issue_urls uses the full regex capture (ENG-42) to check membership in local_ids. Since local_ids is populated from tasks_by_id keys (bare numbers), Linear URLs will never match and will never be rewritten. Additionally, a Linear URL's bare numeric ID could collide with a GitHub issue number, causing fetch_sub_issues to retrieve an unrelated issue.

Suggested fixes:

1. Store the full Linear ID (ENG-42) as-is in extract_referenced_task_ids instead of stripping the prefix, so the ID space is consistent with rewrite_issue_urls. Adjust issue_filename accordingly.
2. Alternatively, restrict extract_referenced_task_ids to only match GitHub URLs (skip the Linear alternation) since LinearSource::fetch_sub_issues currently returns empty, avoiding cross-source ID collisions.

[hsyntax]

Store full linear ID

[hsyntax]

Fixed: linear-url-id-mismatch: scope URL crawling by tracker and fix linear rewrites

[hsyntax]

WARNING (efficiency) sync-write-reread-inefficiency: sync_to_local writes every task file to disk, then re-reads all files from disk to rewrite URLs, then calls list_plan_files a second time to populate the return value — three passes where one suffices. The local_ids HashSet is also built by cloning every key from tasks_by_id via a manual loop instead of using tasks_by_id.keys().cloned().collect() or passing a closure.

Suggested fixes:

1. Apply rewrite_issue_urls to the content string in memory before fs::write, collect paths into a Vec during the same loop, and return that Vec directly — eliminating the read-back pass and the second list_plan_files call.
2. Replace lines 80-83 with let local_ids: HashSet<String> = tasks_by_id.keys().cloned().collect(); or change rewrite_issue_urls to accept |id| tasks_by_id.contains_key(id) to avoid constructing a separate HashSet entirely.

[hsyntax]

WARNING (reuse) git-command-boilerplate: The pattern Command::new("git").args([...]).current_dir(path).output()... with status check and stderr wrapping is repeated 7 times across commit_plan_files, ensure_cycle_commit, git_head_oid, and push_branch. Each instance differs only in args and error-message prefix.

Suggested fixes:

1. Extract a fn run_git_checked(dir: &Path, args: &[&str], context: &str) -> Result<std::process::Output> helper that runs the command, checks exit status, and wraps stderr in Error::Orchestrator. Each call site becomes a one-liner.

Note: this finding applies to line 1191 but is shown here because that line is not in the diff.

[hsyntax]

Fixed: git-command-boilerplate: extract shared git command helpers

[hsyntax]

WARNING (reuse) test-choose-handler-duplication: The Choose-phase handler block (~20 lines) is copy-pasted across five AgentRunner implementations in integration tests (~120 lines total of near-identical code).

Suggested fixes:

1. Extract a shared fn default_choose_handler(task_id: &str, prompt: &str, working_dir: &Path) -> Result<RunResult> that encapsulates the nothing-left check, task.toml write, and RunResult construction. Each runner's Choose arm delegates to it.

Note: this finding applies to line 259 but is shown here because that line is not in the diff.

[hsyntax]

INFO (reuse) plan-slug-reimplements-slugify: The fallback branch in plan_slug re-implements slug-cleaning logic (lowercase, replace non-alnum with hyphens, trim hyphens) that WorktreeManager::slugify already provides.

Suggested fixes:

1. Replace lines 128-135 with let cleaned = WorktreeManager::slugify(&main_task.id); and keep the empty-check + gh- prefix logic.

[hsyntax]

Fixed: plan-slug-reimplements-slugify: reuse worktree slugify for id fallback

[hsyntax]

INFO (quality) validate-plan-dir-verbose: validate_local_plan_dir is 53 lines for a simple exists+is_dir+has-any-file check with manual entry iteration. It also returns Result<(), String> instead of the crate's error::Result<()>, inconsistent with the project's error convention.

Suggested fixes:

1. Rewrite to call plan_sync::list_plan_files after the exists/is_dir checks, error if the returned vec is empty. Change return type to rlph::error::Result<()> using Error::Orchestrator for error variants.