fix(specialist): cap max specialist nesting depth

[euxaristia]

Summary

Adds a hard cap on specialist (Task tool) nesting depth, as a defense-in-depth safeguard.

Related to #470. Not closing it: as explained below, today's normal specialist/swarm path can't actually reach this recursively, so this cap is a future-proof guard rather than a fix for an active exploit of the recursive chain #470 describes.

Changes

• Add maxSpecialistDepth = 8 in internal/specialist/exec.go.
• Executor.Run now rejects a call whose CurrentDepth is already at or past the cap with a clear error, before any specialist work starts. (Fixed from an earlier off-by-one: the check must trigger at CurrentDepth >= maxSpecialistDepth, not >, since this call is what launches the child at CurrentDepth+1 — checking only > let a parent already at the cap start one more level before the guard caught it.)

Why this is defense-in-depth, not an active-exploit fix

Per jatmn's review: today, a specialist (or swarm member) can't actually reach this code path recursively. Every specialist/swarm child is launched with --tag specialist (internal/specialist/exec.go, the only place --tag is set on a spawned child), and shouldRegisterExecSpecialistTools (internal/cli/app.go) refuses to register the Task tool whenever tag == "specialist", regardless of --auto high — already covered by the existing exec_test.go case asserting a specialist child does not get the Task tool. So a specialist cannot itself call Task to spawn a grandchild, and CurrentDepth can't structurally exceed 1 through today's normal flow.

The cap is still worth having: it's cheap, matches the issue's suggested fix (hard MaxDepth with a clear error), and protects against a future change to that tag-gating logic silently reopening the recursive path. It is not closing an actively exploitable bug today.

Test plan

• TestRunRejectsDepthExceedingMax and TestRunRejectsDepthAtMax in internal/specialist/exec_test.go, covering both the already-past-cap case and the exact-boundary case the off-by-one fix addresses. TestRunRejectsDepthAtMax is now a table covering both a fresh call and a Resume call, since the guard sits before the fresh/resume split in Run.
• go build ./...
• go vet ./internal/specialist/...
• go test ./internal/specialist/... -count=1 (all pass)

This was split out of #468 at reviewer request (Vasanthdev2004 asked for the bundled fixes to be split into independent PRs).

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Added a maximum cap on nested specialist execution depth to prevent overly deep task chains.
  • Runs with a current depth at or beyond the limit now fail fast with a clear, depth-related error.
• Tests
  • Extended coverage to verify rejection when depth exceeds the maximum.
  • Added boundary tests for both fresh runs and resume runs when depth equals the maximum.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ffe05924-a350-4d1a-8b33-81d0ef09f1db

📥 Commits

Reviewing files that changed from the base of the PR and between 087e3c9 and c89032f.

📒 Files selected for processing (2)

• internal/specialist/exec.go
• internal/specialist/exec_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/specialist/exec.go
• internal/specialist/exec_test.go

---

Walkthrough

Adds a hard maximum specialist nesting depth in Executor.Run and rejects runs at or beyond that limit. Tests cover both over-limit and at-limit inputs.

Changes

Depth limit enforcement

Layer / File(s)	Summary
Depth cap enforcement internal/specialist/exec.go	Adds maxSpecialistDepth = 8 and rejects CurrentDepth >= maxSpecialistDepth in Executor.Run.
Depth boundary tests internal/specialist/exec_test.go	Adds coverage for CurrentDepth > maxSpecialistDepth and CurrentDepth == maxSpecialistDepth, including fresh and resume paths.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• specialist: no hard maximum depth limit on Task / nested specialists #470: Matches the same Executor.Run depth-handling path by adding a hard nesting cap and tests for the new rejection cases.

Possibly related PRs

• Gitlawb/zero#113: Modifies the same specialist execution flow that feeds Executor.Run, which this change now constrains with a nesting cap.

Suggested reviewers: Vasanthdev2004, gnanam1990

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: capping specialist nesting depth.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Vasanthdev2004]

Clean little guard, and it's the specialist-depth split from #468 done exactly as asked. Cap's at 8, the check fires before any specialist work starts, and the comparison is right — it rejects only past the cap, so legit nesting up to it still works. And the cap is actually reachable: CurrentDepth propagates through --depth, so a runaway chain does hit it rather than this being dead code. Test covers it, CodeRabbit's clean. Closes #470. Approving.

[jatmn]

I found an issue that needs to be addressed before this is ready.

Findings

• [P2] Stop launching the over-cap specialist child
  internal/specialist/exec.go:222
  The new guard rejects only when the parent run's CurrentDepth is already greater than maxSpecialistDepth, but the child process is launched with --depth CurrentDepth+1 a few lines later. A specialist currently at depth 8 therefore still passes this check and starts another specialist session at depth 9, so work begins beyond the advertised maximum before the next Task call is rejected. Please reject when the next child would exceed the cap, and cover that boundary for both fresh and resumed specialist runs.

• [P2] Demonstrate the recursive Task path before closing #470
  internal/cli/app.go:984
  The PR body says this closes #470 because "a chain of specialists recursively spawning further specialists via --depth" can recurse indefinitely, but the normal child path appears to suppress that chain: BuildArgs/BuildResumeArgs always launch the child with --tag specialist, and shouldRegisterExecSpecialistTools returns false for that tag, with an existing --list-tools test asserting that a specialist child does not get the Task tool even under --auto high. Please either add evidence/tests for the reachable recursive Task path this cap is meant to stop, or update the PR/issue-closing claim so #470 is not closed by a guard that does not cover the reported behavior.

[jatmn]

Thanks for the update. I rechecked the previously discussed paths and found one issue that still needs to be addressed.

Findings

• [P2] Do not auto-close #470 from this defense-in-depth guard
  PR description
  The code now fixes the off-by-one by rejecting CurrentDepth >= maxSpecialistDepth, so the first review item is addressed. The PR body also now agrees with the second review item that today's normal specialist/swarm child path cannot recursively reach Task because child sessions are launched with --tag specialist and shouldRegisterExecSpecialistTools suppresses Task for that tag. However, the body still says Closes #470, which would close the issue describing an actually recursive Task chain even though the PR now says this is only a future-proof guard and not an active-exploit fix. Please complete the prior review request by removing the closing keyword from this PR, or add concrete evidence/tests for a currently reachable recursive path that this cap actually stops.

• [P3] Cover the resume boundary that was requested
  internal/specialist/exec_test.go:348
  The previous review asked for the over-cap boundary to be covered for both fresh and resumed specialist runs. TestRunRejectsDepthAtMax does verify the shared pre-branch guard today, but it only passes a plain prompt and never exercises the Resume parameters, so a future refactor could move the guard into the fresh path and still leave the requested resume boundary uncovered. Please add the explicit resume-at-cap case as well, or otherwise make the test table prove both fresh and resume calls reject before launching a child.

[jatmn]

Thanks for the update. I rechecked the previously discussed paths and found one issue that still needs to be addressed.

Findings

• [P2] Remove the closing keyword from the commit history too
  548cf478
  The PR body now correctly says this is only related to #470 and is not closing it, but the first commit in this PR still contains Closes #470. That leaves the same issue-closing risk in the branch metadata: if this is merged in a way that preserves the original commit message, #470 can still be closed even though the current PR description says this defense-in-depth guard is not an active fix for the recursive Task path. Please complete the prior review request by rewriting/removing that closing keyword from the PR's commits, or add concrete evidence/tests for a currently reachable recursive path that this cap actually stops.

[euxaristia]

Fixed: the closing keyword is gone from the commit history. The first commit was amended (548cf47 -> 498afca) so Closes #470 reads Related to #470, not closing it, matching the PR description. No code changed, just the message. Force-pushed to c89032f.

@jatmn ready for re-review.

[jatmn]

Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.

@Vasanthdev2004

[kevincodex1]

LGTM