Sign up
View stargazersSign up

fix(server): inherit parent lifecycle when creating subagents#20074

open9
main
GitHub fork: fix/subagent-creation-lifecycle
opened24d agobyQQuicksaver
4 files changed
View on GitHubTwo-way sync enabledPR last imported 13d ago

A native subagent discovered after its parent was archived or deleted could appear active. A restored parent could also leave a newly discovered child with a stale archive timestamp.

New native subagents inherit the parent’s persisted lifecycle while creation is serialized with parent commands. Existing children keep their saved lifecycle, forks remain independent, and explicitly deleted children stay deleted. Unarchiving a parent does not cascade to children already discovered while archived; those children can be unarchived separately. Fresh app-owned delegation rejects archived or deleted parents through the existing command-receipt path; accepted and rejected request replays retain their recorded results. Self-parent offers avoid acquiring the same non-reentrant lock twice.

Original contribution by @Quicksaver in #20074. Maintainer takeover preserves contributor commits, merges main, consolidates the tests into existing module suites, simplifies locking, and adds the self-parent guard.

Verification at 5f2abc7b44:

  • PR-specific new cases reduced 11 → 5. Deleted the 290-line one-off suite and added 249 lines to two existing test suites.
  • 47 tests pass across ProviderEventIngestor and Orchestrator.control-reads, including real-Git branch correction, persisted parent lifecycle, restoration, fork independence, explicit child deletion, metadata replay, Deferred-synchronized archive/delete races, and delegation receipts.
  • Reverting inheritance fails seven assertions/cases; removing parent serialization fails both races on persisted child lifecycle; removing the delegation gate fails both rejection cases. Deleted-parent delegation already rejects on main because deletion cancels its run; this change gives the explicit lifecycle error through the same receipt path.
  • Scoped lint/format and server package typecheck pass. Two existing warnings remain in unchanged Orchestrator code.
  • One independent full-diff review round (Opus and Sol) found no code blockers; each independently reran all 47 tests successfully. Latest-head CI is green, including all eight server shards, lint, build/typecheck, and aggregate Check.

Proof is headless service-level execution with in-memory SQLite, actual orchestration planning/commit/receipts, keyed locks, and temporary Git repositories. The provider effect worker is disabled to isolate persistence. No live provider CLI, MCP transport, integrated client, remote connection, macOS, Windows, iOS, Android, or Electron desktop run is claimed. This server-only change has no visual UI diff. Previous-head evidence is superseded by the results above. The PR remains unmerged.

Models: GPT-6-Astra (tests), GPT-6.1-Sol (implementation), GPT-6-Sol and Claude Opus 5.5 (review). Harnesses: Codex and Claude Code through T3 Code.

Checks

20 successful · 8 skipped
All checks passed
c5e91fb
MmacroscopeappBot24d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR not approvable — The fix changes production lifecycle propagation and adds a parent-state gate that can prevent subagent workflows, alongside cross-thread locking to address races. Focused tests cover the scenarios, but the workflow-gating and concurrency behavior merit human review.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot24d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between d77df163bea8983e6843bd7e24871a4986ddcd62 and c5e91fb122924fd9b2b29cc5d53a02be003435d6.

External image Files selected for processing: 4
  • apps/server/src/orchestration-v2/Orchestrator.control-reads.test.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/orchestration-v2/ProviderEventIngestor.test.ts
  • apps/server/src/orchestration-v2/ProviderEventIngestor.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

The orchestrator now rejects delegated-task requests when the parent thread is archived or deleted. New subagent thread creation inherits lifecycle timestamps from its parent, and creation events acquire the parent lock before the child lock. Tests cover lifecycle inheritance, command replay, and archive/delete races.

Changes: Subagent lifecycle

Layer / File(s)Summary
Subagent lifecycle inheritance
apps/server/src/orchestration-v2/ProviderEventIngestor.ts, apps/server/src/orchestration-v2/ProviderEventIngestor.test.ts		New subagent threads inherit the parent’s archive timestamp and inherit its deletion timestamp when the child has none. Creation events lock the parent before the child. Tests cover inherited lifecycle states, forks, and archive/delete races.
Delegated-task lifecycle guard
apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/orchestration-v2/Orchestrator.control-reads.test.ts		Delegated-task dispatch rejects requests when the parent is archived or deleted. Tests cover accepted and rejected command replay, and delegation after unarchiving.

Priority: External image Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested labels: bugfix

Suggested reviewers: t3dotgg, juliusmarminge

Merge Risk: External image Minimal · up to c5e91

The supplied evidence identifies no issue requiring a fix before merge; normal validation remains appropriate.

Security architecture risk: External image Low · up to d77df

The change improves lifecycle consistency and prevents new delegation under archived or deleted parents. The new nested locking can block affected threads if a provider supplies self-referential lineage, although an externally exploitable path has not been established.

Retained concerns

  • Low · reliability · inferred: The new lock nesting assumes distinct parent and child identities without checking them before acquisition. An app_thread.created event whose subagent parentThreadId equals its own id reacquires the same non-reentrant lock and cannot reach normalization, commit, or ordinary completion. Commands sharing that thread key remain blocked until ingestion is interrupted. This is a conditional failure-containment concern; ordinary adapter construction limits reachability, and direct external control was not established.
Security review details

Security Blast Radius

  • inferred — The demonstrated lock-failure mechanism affects ingestion and commands sharing the selected thread key. Broader tenant, credential, environment, or data-store compromise is not established by this mechanism.

Security Findings and Attack Paths

  • inferred — A self-parent adapter event would enter the newly added parent lock and then wait on the same child key before any projection lookup. This condition is new relative to the single-lock base path, but evidence does not establish that an ordinary external user can cause a production adapter to emit it.

Trust Boundaries and Controls

  • observed — Normal subagent construction takes parent identity from the current parent thread rather than an arbitrary parent-ID parameter. Claude and Codex derive child IDs from provider-native identities. These controls limit malformed-lineage reachability, although the lineage schema itself does not require parent and child IDs to differ.

Resilience and Maintainability Implications

  • observed — The keyed lock releases its bookkeeping through acquire-use-release when the enclosing effect exits. That supports cleanup after interruption, but does not make same-key nested acquisition reentrant or allow it to complete normally.

Hardening Proposals

  • proposed — Reject self-parent lineage before acquiring nested locks, and define how invalid or cyclic provider lineage is contained without stranding thread-command keys.
Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Description checkWarningThe description explains the problem, change, and verification in detail, but it does not provide the required scope and approval information. It does not link a triaged issue or explicit maintainer a…Add a link to the triaged issue or discussion with the maintainer’s explicit approval of the direction and scope. If no prior approval is needed, explain why this is a small, focused fix of an obvious bug.
Passed Passed checks: 3 passed
Check nameStatusExplanation
Title checkPassedThe title clearly summarizes the main change: new subagents inherit their parent’s lifecycle.
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, change, and verification in detail, but it does not provide the required scope and approval information. It does not link a triaged issue or explicit maintainer approval, or explain why this qualifies as a small, focused fix that needs no prior approval.

Resolution

Add a link to the triaged issue or discussion with the maintainer’s explicit approval of the direction and scope. If no prior approval is needed, explain why this is a small, focused fix of an obvious bug.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Added 3 commits

  • fix(server): inherit parent lifecycle when creating subagents7e4b202
  • test(server): adapt lifecycle fixtures to native branch rechecksbcae563
  • test(server): assert lifecycle setup before branch correctiond77df16
JJulius Marminge20d ago

Taking over this one as maintainer for batch scheduler-tasks-03. I will preserve the contributor commits, merge main, reduce the lifecycle tests into existing module suites, rebuild the implementation against current conventions, and run independent parallel reviews. This batch will not merge the PR.

Added 6 commits

  • Merge remote-tracking branch 'origin/main' into fix/subagent-creation-lifecyclea004212
  • test(server): consolidate subagent lifecycle regressionse8a659c
  • fix(server): avoid self-parent subagent creation deadlocks5f2abc7
  • Merge remote-tracking branch 'origin/main' into pr-200743f5f407
  • merge: bring main into subagent lifecycle fix2633ac1
  • test(server): isolate subagent lifecycle casesc5e91fb
Mmaria13d ago

Note

Written by gpt-6.1-sol on behalf of Maria

updated the original branch at c5e91fb122924fd9b2b29cc5d53a02be003435d6, preserving @Quicksaver's commits and julius's earlier takeover. merged current main without rebasing or force-pushing.

the production fix already expressed the intent simply, so i kept it: newly discovered native children inherit persisted parent lifecycle under the parent lock; fresh app-owned delegation rejects archived/deleted parents; saved children, forks and command receipts keep their behavior. removed the obsolete workspace test matrix that main replaced and consolidated lifecycle coverage into three parameterized cases with distinct child ids. no new test files.

verification:

  • main production code fails all seven selected lifecycle/delegation regression cases; restored production code passes all 47 focused tests.
  • blacksmith vp run check:changed passes formatting, lint, both knip checks, graphql validation, server/scripts typechecks and 48 tests across three existing files.
  • all 28 latest-head ci checks are green. two independent full-diff reviews approve this exact head. coderabbit's latest review reports no actionable code findings.
  • the retained low self-parent warning refers to d77df; the current parentThreadId !== thread.id guard prevents acquiring the same lock twice, and the metadata test exercises that path.

unverified: live provider/client integration, remote transports, desktop and native mobile. the dev server started successfully, then was stopped. no rendering paths changed and no screenshots are claimed. testbox stopped.

scope: maria explicitly requested this focused takeover and preservation of the original intent. this comment records implementation authorization, not final approval of the lifecycle policy. the existing macroscope human-approval request and coderabbit scope-description warning remain for maria's review. #20072 separately covers propagation to already-created descendants; it is outside this pr.