From 543743bbc1981cecc0a82fbbd7841fe78250bc0b Mon Sep 17 00:00:00 2001 From: AirCoding Date: Mon, 1 Jun 2026 09:49:16 +0800 Subject: [PATCH] Detailed design: resolve nine P2 cross-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply all remaining P2-level repairs identified by the four-model cross-review (DeepSeek, MIMO 2.5 Pro, GPT-5.5 Pro, Opus 4.8) and verify by regression: - P2-01 Architecture Designer gate sequence: add a dedicated §19.4 sequence diagram covering trigger → impact assessment → result class (silent_continue/requires_user_confirmation/requires_replan/ reject_or_escalate) → doc update via ToolRegistry → Scheduler consumption. Renumber Debug knowledge capture to §19.5. - P2-02 CLI catalog command class ownership: replace the prose inventory with an explicit class-to-subcommand table covering RunCommand, InitCommand, DoctorCommand, ProviderCommand, E2ECommand, ReleaseCommand, ResumeCommand, CompactCommand, HistoryCommand, SessionListCommand, RestoreCommand. - P2-03 agent.started projection wording: clarify the two-step domain update (starting upon spawn intent, running on handshake ack within the same event commit) and what carries the final state row at commit time. - P2-04 WorkspaceManager vs. Scheduler responsibility split: add a policy/mechanism responsibility matrix to §7.5 making Scheduler the sole policy owner (strategy choice, conflict resolution) and WorkspaceManager the sole mechanism owner (materialize, merge, cleanup, lifecycle events). - P2-05 EventStore.project boundary wording: split §5.4 into Table A (pure projection inside events_session.db transaction) and Table B (projection intent + post-commit outbox/compensation by owning service), removing the “write external DB via owner” phrasing from the in-transaction projection table. - P2-06 memory.promoted two-phase semantics: rewrite §18.4 to document the intent vs. committed phases with payload markers, retry behaviour, and parity with debug.record.created. - P2-07 docs task type closure: add a TaskType → WorkerRole mapping table in §8.3 and a design decision recording that `docs` is a formal TaskType handled by ExecutorRole with docs-scoped TaskScope.write_area; events and verification follow the execute pipeline with type='docs' as the domain-level differentiator. - P2-08 AgentType vs. runtime roles: add a top-level table in §2 pinning AgentType to worker child-process roles only and naming the runtime-resident roles (main, architecture_designer, scheduler) plus their prompt sources and LLM-use flags. - P2-09 contracts package file set: replace the IMPL note in §3 with a frozen file-set decision plus an overview §4 symbol-group → code-view §3 file mapping; new files require an ADR. Regression confirms: §19 sequence count is now 5; §17 CLI table covers every catalog command; §5.4 splits projection vs. outbox; §18.4 documents memory.promoted phase semantics; §8.3 lists docs under ExecutorRole; §2 names runtime roles; §3 freezes the contracts file set. Co-Authored-By: Claude Opus 4.7 --- .../architecture/system-detailed-design.md | 231 ++++++++++++++++-- 1 file changed, 207 insertions(+), 24 deletions(-) diff --git a/AirPlan/docs/architecture/system-detailed-design.md b/AirPlan/docs/architecture/system-detailed-design.md index 9a8e0cf..e3d89c3 100644 --- a/AirPlan/docs/architecture/system-detailed-design.md +++ b/AirPlan/docs/architecture/system-detailed-design.md @@ -88,6 +88,25 @@ Ownership summary (frozen by code-view §11 State Ownership): | model config | `ProviderManager` | runtime/Doctor read via API | | project rules/context | `ContextAssembler`/`ProjectStore` | workers get excerpts | +**Worker `AgentType` vs. runtime roles** (contracts §5 `AgentType`, overview §10): + +The frozen `AgentType` union covers only worker child-process roles: +`executor | reviewer | debugger | compactor | experience_miner`. + +Runtime-resident roles are *not* members of `AgentType` and never appear in +`WorkerResult.agent_type`, `agents.agent_type`, or `PromptLayerLoader.load_role(role)`: + +| Runtime role | Implementing class | LLM use | Prompt source | +|---|---|---|---| +| `main` | `MainAgent` (§14.1) | Yes, via `ProviderManager` | Built-in resource (loaded directly by `MainAgent`, see §10.2) | +| `architecture_designer` | `ArchitectureDesigner` (§14.2) | Yes, via `ProviderManager` | Built-in resource (loaded directly by `ArchitectureDesigner`, see §10.2) | +| `scheduler` | `Scheduler` (§7.1) | No (pure orchestration) | N/A | + +This split keeps the contract `AgentType` enum stable and avoids accidentally giving runtime +roles a worker `WorkerResult` shape. Where event payloads need to identify a runtime role +(e.g. `EventSource.kind` in §22.1), they use a separate `"main" | "architecture_designer" | +"scheduler"` literal set defined in `event.ts`, never `AgentType`. + ## 3. Contracts Package Detailed Design `packages/contracts` is type-only. It contains zero runtime logic, only `export interface`, @@ -137,6 +156,25 @@ platform.ts → cross-platform tier enums referenced by Doctor (from cross-p above; the binding requirement is that the exported symbol set equals the contract set. This design keeps the code-view §3 list as canonical and treats overview §4 as the superset note. +**Frozen file-set decision (P2-09)**: For V1.0.0 Alpha, the canonical contracts package file set is +exactly the 16 files listed above (frozen by code-view §3). Overview §4 symbol groups that do not +have a matching dedicated file are merged into the existing files per the mapping below; no new +`.ts` files are added in the contracts package without an ADR. + +| Overview §4 symbol group | Canonical file (code-view §3) | +|---|---| +| `storage.ts` symbols (TransactionManager, Repository facades) | merged into `task.ts` (TransactionManager) and per-domain files | +| `scheduler.ts` symbols (SchedulerWavePlan, SchedulerRunResult) | `task.ts` | +| `workers.ts` symbols (WorkerRole, WorkerRuntime, IPC payloads) | `ipc.ts` + `worker-result.ts` | +| `context.ts` symbols (PromptLayer, PromptLayerLoader, ContextAssembler) | `runtime.ts` (`ContextPack`) + dedicated `context.ts` only if needed (otherwise inlined in `runtime.ts`) | +| `projection.ts` symbols | `ui.ts` | +| `doctor.ts` symbols (DoctorService, DoctorRunInput/Output) | `platform.ts` (cross-platform tier types) + dedicated `doctor.ts` only if file size warrants split | +| `knowledge.ts` symbols (DebugKnowledgeStore, LearnedMemoryStore) | inlined into `artifact.ts` / dedicated file only if needed | +| `diagnostics.ts` symbols (Diagnostic, semantic_signature types) | inlined into `tool.ts` / dedicated file only if needed | + +The barrel `index.ts` exports the full union. Any future split into additional files requires an ADR +under `docs/architecture/adr/` and a synchronized update to code-view §3. + ## 4. Storage and Repositories Module: `packages/runtime/src/storage/`. Classes: `DatabaseManager`, `MigrationRunner`, @@ -325,17 +363,20 @@ append-only (rule 5) — `EventStore` never rewrites prior route entries. ### 5.4 Durable projection map `project(event, tx)` switches on `event.type` and applies exactly the domain update fixed by -event-registry §3. The full map (no event may project differently): +event-registry §3. To make the boundary between pure intra-transaction projection and +post-commit outbox/compensation work explicit, the map is split into two tables. -| Event type | Domain update | +**Table A — Pure projection within `EventStore.append` transaction (no external writes):** + +| Event type | Domain update (in `events_session.db` transaction) | |---|---| | `session.created` | insert `sessions` | | `session.archived` / `session.deleted` | update `sessions.status` | | `user.message.created` | insert `messages` | | `assistant.message.started` | upsert `message_drafts` (status=streaming) | | `assistant.message.created` | insert `messages` + delete matching `message_drafts` | -| `assistant.message.failed` | `message_drafts.status=error` or failure artifact | -| `agent.started` | insert `agents` (starting/running) | +| `assistant.message.failed` | `message_drafts.status=error` or failure artifact ref | +| `agent.started` | insert `agents` row with `status='starting'` upon spawn intent, then update to `status='running'` on the same event's commit when WorkerProcess handshake has succeeded (`workers` ack); domain row carries final state at commit time | | `agent.completed/failed/lost/cancelled` | update `agents.status` | | `task.created` | insert `tasks` (+ optional `task_dependencies`) | | `task.started` | `tasks.status=running`, set started/agent/workspace; insert `task_attempts` | @@ -351,10 +392,10 @@ event-registry §3. The full map (no event may project differently): | `command.started` | insert `command_runs` | | `command.completed` | update exit_code, artifact refs, diagnostics, duration | | `command.failed` | update exit_code when available + failure artifacts | -| `artifact.created` | insert `artifacts` (after temp→rename) | +| `artifact.created` | insert `artifacts` (after temp→rename completed before commit) | | `diagnostic.created` | insert `diagnostics` | | `evidence.created` | insert `evidence_refs` | -| `context.compaction.requested` | insert compaction task if accepted | +| `context.compaction.requested` | insert compaction task row if accepted | | `context.compaction.started` | mark compaction task running | | `context.compaction.completed` | mark compaction task complete (no summary row) | | `context.compaction.failed` | mark compaction task failed/blocked | @@ -362,18 +403,39 @@ event-registry §3. The full map (no event may project differently): | `permission.decision.recorded` | append (optional future projection table) | | `permission.prompt.requested/resolved` | append (optional UI projection) | | `doctor.*` | append (+ optional report artifact / command rows) | -| `requirement.changed` | append; mark impacted tasks when Scheduler applies | +| `requirement.changed` | append (Scheduler applies impacted-task marking on consumption) | | `architecture.plan.updated` | append + plan/artifact refs | -| `architecture.impact.completed` | append; Scheduler consumes | +| `architecture.impact.completed` | append (Scheduler consumes on EventBus subscription) | | `workspace.created` | insert `workspaces` | | `workspace.merge.started` | append + mark merge in progress (metadata) | | `workspace.merge.completed` | `workspaces.status=merged`, set merged_at | | `workspace.merge.conflicted` | `workspaces.status=conflicted` | | `workspace.cleaned` | `workspaces.status=cleaned` | | `memory.candidate.created` | append | -| `memory.promoted` | append + write rules/skill/learned-memory.db via owner (outbox §18.4) | -| `memory.archived` | append + mark memory inactive via owner | -| `debug.record.created` | insert/update debug-records.db (outbox §18.4) + append session event | +| `memory.archived` | append (mark memory inactive in session-side mirror if any) | + +**Table B — Projection + post-commit outbox/compensation (cross-DB or external write):** + +These events project a session-side intent row inside the same transaction, then drive a +follow-up external write via the outbox model (§18.4, runtime-semantics §6.3-§6.4). The +external write is performed by the *owning service* after EventStore.commit, never inside +`project(event, tx)`. + +| Event type | Projection step (in transaction) | Outbox step (post-commit) | Owner | +|---|---|---|---| +| `memory.promoted` | append durable event recording promotion intent | write `rules/`, `skills/`, or `learned-memory.db` row | `LearnedMemoryStore` / `RuleStore` | +| `memory.archived` (when external mirror exists) | append durable event | mark external memory inactive | `LearnedMemoryStore` | +| `debug.record.created` | append durable session event | insert/update row in `debug-records.db` | `DebugKnowledgeStore` | + +Rules: +- `project(event, tx)` itself never opens external DBs or files; it only writes + `events_session.db` rows (events, drafts, domain projections). +- After successful commit, `EventBus.publish(event)` fires; outbox-aware subscribers + (`LearnedMemoryStore`, `DebugKnowledgeStore`, ...) then perform the external write and + emit a completion event (`memory.promoted` completion artifact, `debug.record.created` + status update) per §18.4. +- On restart, recovery scans pending outbox intents in the session DB and retries + external writes (runtime-semantics §6.4). ### 5.5 EventBus @@ -536,6 +598,24 @@ Strategies (db-schema §16, scheduler-state-machine §MERGING): `main` (no merge GC retention follows overview §15 (active until merge/cancel; merged 7d; abandoned 3d; cleaned keeps DB row). +**Responsibility split — Scheduler vs. WorkspaceManager** (overview §10.5, scheduler-state-machine §4/§MERGING): + +| Responsibility | Owner | +|---|---| +| Decide *whether* a task needs an isolated workspace | Scheduler (via `WavePlanner.assign_workspace`) | +| Decide *which strategy* (`main`/`worktree`/`isolated_copy`) | Scheduler (via `WavePlanner.assign_workspace`) | +| Encode the decision into a `WorkspacePlan` | Scheduler (in `SchedulerWavePlan`) | +| Materialize the workspace on disk (clone/copy/setup) | WorkspaceManager (`create_workspace`) | +| Apply merge/patch back to project root | WorkspaceManager (`merge_workspace`) | +| Detect/report merge conflicts | WorkspaceManager (emits `workspace.merge.conflicted`) | +| Decide *how* to resolve a conflict | Scheduler (consumes conflicted event; may re-plan or escalate to Main Agent) | +| Cleanup/GC of abandoned/cleaned workspaces | WorkspaceManager (`cleanup_workspace`, driven by Scheduler heartbeat / Doctor) | +| Persist workspace lifecycle events | WorkspaceManager (durable events) → EventStore projection updates `workspaces` row | + +WorkspaceManager is a pure executor of Scheduler decisions: it never plans concurrency, never decides +which task occupies which workspace, and never re-plans on conflict. The Scheduler is the sole policy +owner; WorkspaceManager is the sole mechanism owner. + ### 7.6 AgentMonitor ```text @@ -639,6 +719,28 @@ Each role's `run` ends by returning a `WorkerResult` with the matching `fallback_allowed` (runtime-semantics §9.4, overview §10.9). Self-escalation returns `blocked` with a `BlockerReport` rather than improvising (scope-escalation §9). +**TaskType → WorkerRole mapping** (contracts §9 `TaskType`, db-schema §`tasks.type` closed enum): + +| TaskType | Owning WorkerRole | Worker `agent_type` | Output contract | Notes | +|---|---|---|---|---| +| `execute` | `ExecutorRole` | `executor` | `ExecutorResult` | Code-changing work; scoped project writes | +| `review` | `ReviewerRole` | `reviewer` | `ReviewerResult` | Read-only audit/review | +| `debug` | `DebuggerRole` | `debugger` | `DebuggerResult` | Scoped writes when assigned | +| `compact` | `CompactorRole` | `compactor` | `CompactorResult` | Summary/artifact writes only | +| `mine_experience`| `ExperienceMinerRole` | `experience_miner` | `ExperienceMinerResult` | Candidate/rule/skill writes when assigned | +| `docs` | `ExecutorRole` | `executor` | `ExecutorResult` | Documentation-only task; `TaskScope.write_area` restricted to docs paths (overview §10.9); same events as `execute`: `task.created/started/completed`, `tool.*` for fs writes, `artifact.created` for doc artifacts | + +**Design decision — `docs` task type closure**: `docs` is a formal `TaskType` (contracts §9, +db-schema §`tasks.type`). It is *not* a separate WorkerRole; it reuses `ExecutorRole` with a +documentation-scoped `TaskScope.write_area` (typically `docs/`, `AirPlan/docs/`, README, ADR/C4 +folders) and an empty or docs-only `VerificationPolicy.commands`. All persistent events for a +`docs` task are identical to an `execute` task: `task.created` → `task.started` (with +`agent_type='executor'`) → `tool.*` for file edits → `artifact.created` for documentation +artifacts → `task.completed` with `ExecutorResult`. The `tasks.type='docs'` field is the only +domain-level differentiator, used by Scheduler for routing/policy (e.g. lighter verification, +no test runs) and by Architecture Designer to detect doc-only changes that may still trigger +the architecture gate when they touch ADR/C4 files. + ### 8.4 Execution discipline enforcement (Executor/Debugger) Read-before-edit and exact-edit are enforced at the tool layer, not just by prompt (§9.4); the role @@ -1133,13 +1235,29 @@ Module: `packages/cli/src/` (code-view §8). `CliEntrypoint.main(argv)` routes t ```text class CliEntrypoint { +main(argv): Promise } class RuntimeFactory { +create(options): Promise } -commands: RunCommand, InitCommand, DoctorCommand, ProviderCommand (read-only), E2ECommand, - ReleaseCommand, plus catalog: resume, compact, history, session list, restore (overview §14) ``` +Command class inventory (frozen mapping, code-view §8, overview §14): + +| Command class | Subcommand(s) | Notes | +|---|---|---| +| `RunCommand` | `run [project]` | Default entry; spawns RuntimeApp + TUI | +| `InitCommand` | `init` | First-run wizard, project bootstrap | +| `DoctorCommand` | `doctor [--fix|--bundle]` | Wraps `DoctorService` (§16.1) | +| `ProviderCommand` | `provider list`, `provider current` | Read-only; no runtime model switch | +| `E2ECommand` | `e2e` | Local end-to-end harness, no CI hooks | +| `ReleaseCommand` | `release` | Build/package release artifacts | +| `ResumeCommand` | `resume [--session SID]` | Catalog: resume last/specific session | +| `CompactCommand` | `compact [--session SID]` | Catalog: trigger compaction task | +| `HistoryCommand` | `history [--session SID]` | Catalog: render past sessions/events | +| `SessionListCommand` | `session list` | Catalog: enumerate sessions | +| `RestoreCommand` | `restore ...` | Catalog: git-backed backup restore (overview §15, runtime-semantics §19) | + `ProviderCommand` is read-only (`provider list`/`current`); there is no runtime provider/model switch -command (overview §14). `restore` supports file/time/session granularities over the git-backed backup -repo (overview §15, runtime-semantics §19). +command (overview §14). `RestoreCommand` supports file/time/session granularities over the git-backed +backup repo (overview §15, runtime-semantics §19). All CLI command classes route side effects through +`RuntimeApp` services (Scheduler, SessionManager, DoctorService, ArtifactStore) and never bypass +`ToolRegistry` or `PermissionEngine`. ## 18. Cross-Cutting Designs @@ -1207,16 +1325,37 @@ Writes to project-level DBs (`debug-records.db`, `learned-memory.db`) or externa outbox model (runtime-semantics §6.3-§6.4, overview §8.3): ```text -1. Insert durable session event recording intent/request -2. Insert/update session domain row with pending/external status where applicable -3. Perform external DB/file operation through owning service -4. Emit durable completed/failed event with evidence -5. On restart, recovery scans pending external intents and reconciles +1. Insert durable session event recording intent/request (e.g. memory.promoted intent payload, + debug.record.created intent payload). Domain row in session DB marks status="pending_external". +2. Commit the session-side transaction (events_session.db). +3. After commit, EventBus delivers the event to the owning service (LearnedMemoryStore / + DebugKnowledgeStore). The service performs the external DB/file operation. +4. On success, the owning service emits a durable completion event (memory.promoted completion + payload with artifact_ref, debug.record.created status="committed") that updates the + session-side row from pending_external to committed. +5. On external failure, the service emits a durable failure event; recovery retries from step 3 + based on the pending intent row. +6. On restart, recovery scans pending external intents and resumes step 3. ``` -Example: `memory.promoted` → session event (step 1) → `LearnedMemoryStore.insert` (step 3) → -`memory.promoted` completion evidence (step 4). If step 3 fails, the session event remains and -recovery retries or marks failed. +**`memory.promoted` two-phase semantics** (event-registry §3, runtime-semantics §6.3): + +The single `memory.promoted` event type carries two semantically distinct phases distinguished by +its payload: + +| Phase | Payload marker | Meaning | When emitted | +|---|---|---|---| +| Intent | `phase: "intent"`, no `artifact_ref`/`memory_id` yet | The system has decided to promote a candidate; external write is pending | Step 1 above, inside session-side transaction | +| Completion | `phase: "committed"`, includes `artifact_ref` / `memory_id`, `target_store` | External store now holds the promoted memory | Step 4 above, after external write succeeds | + +The intent event makes the promotion durable even if the runtime crashes before the external +write. The completion event closes the outbox loop and exposes the resulting memory ID/artifact +to downstream readers. If the external write fails permanently, a `memory.candidate.created` +follow-up may re-queue the promotion, or a Doctor task surfaces it for user attention; the +intent event itself remains in the log for audit. + +`debug.record.created` follows the same two-phase pattern (intent → committed) with payload +field `phase`. ### 18.5 Security invariants @@ -1361,7 +1500,51 @@ Scheduler collects result Original messages preserved for backtracking ``` -### 19.4 Debug knowledge capture +### 19.4 Architecture Designer review gate + +Triggered when a Worker result, plan change, or detected drift touches contracts, schema, +events, package boundaries, security, or runtime semantics (scope-escalation §4, overview §10.7). + +```text +Trigger source (Worker/Scheduler/Main Agent) + │ detects: contract/schema/event change | boundary cross | ADR/C4 drift + ▼ +Main Agent.dispatch_architecture_review(change) + │ build ImpactRequest { change, affected_refs, evidence_refs } + ▼ +ArchitectureDesigner.assess_impact + │ load L4 architecture pack (plan/ADR/C4) via ContextAssembler + │ ProviderManager.complete (read-only analysis, no fs/shell writes) + │ classify result: + │ silent_continue → no event mutation, log advisory + │ requires_user_confirmation → architecture.impact.completed (durable) + │ + permission.prompt.requested via Main Agent + │ requires_replan → architecture.impact.completed (durable) + │ + Scheduler replan trigger + │ reject_or_escalate → architecture.impact.completed (durable) + │ + escalate to user / block tasks + ▼ +ArchitectureDesigner.update_architecture_docs (only if confirmed) + │ DocumentUpdate applied via tool layer (ToolRegistry + PermissionEngine) + │ architecture.plan.updated (durable) + artifact refs to plan/ADR/C4 + ▼ +Scheduler consumes architecture.impact.completed + │ mark impacted tasks via requirement.changed propagation + │ wave replan or task cancellation per result class + ▼ +Main Agent resumes; downstream tasks proceed with refreshed L4 context +``` + +Notes: +- The gate never replaces Reviewer (overview §10.7); it adds an architecture-level check. +- ArchitectureDesigner has no direct fs/shell access; all writes go through `ToolRegistry` + under `PermissionEngine` (contracts §13, §23 rule "tools → side effect only via PermissionEngine"). +- Result class strings come from scope-escalation §4 (`silent_continue`, `requires_user_confirmation`, + `requires_replan`, `reject_or_escalate`). +- All gate decisions land as durable `architecture.impact.completed`; subsequent doc edits land + as `architecture.plan.updated` (event-registry §3). + +### 19.5 Debug knowledge capture ```text ExecutorRole fails with build error