From d861af2193c77165cfebb38a1ed66497e5b65767 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Kr=C3=A1l?= Date: Mon, 29 Jun 2026 17:08:41 +0200 Subject: [PATCH 1/2] feat(agents): let read-only CR agents use git worktrees for parallel review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit argos and athena may now opt into an isolated read-only git worktree to run their parallel CR pass when isolation is needed; daidalos cleans every CR worktree up after the run/merge so the repository stays clean. The writing path (talos) still never uses worktrees and concurrent writers still serialise on the single shared working tree via the write-lock — only the read-only CR pass gains the explicit-request opt-in of @rules/git/general.mdc Worktrees / Workspaces, which carries no write-lock and never contends with the writing path. Documented in agents/daidalos.md, agents/argos.md, agents/athena.md Parallel review worktree, mirrored in docs/agents.md, with the worktree pinning test in tests/Installer/AgentsTest.php updated to the new policy. --- CHANGELOG.md | 2 ++ agents/argos.md | 9 +++++++++ agents/athena.md | 9 +++++++++ agents/daidalos.md | 15 +++++++++++---- docs/agents.md | 6 +++--- tests/Installer/AgentsTest.php | 30 ++++++++++++++++++++++++------ 6 files changed, 58 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7663ed1..3f34712 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to `cursor-rules` will be documented in this file. ## [Unreleased] +- 📝 **Changed**: the read-only **code-review agents (`argos`, `athena`) may now opt into an isolated git worktree** to run their parallel CR pass when isolation is genuinely needed, and **`daidalos` cleans every CR worktree up** after the run / merge so the repository and codebase stay clean. This refines the earlier *git worktrees are forbidden across the whole workflow* policy: the **writing path (`talos`) still never uses worktrees** and concurrent writers still serialise on the single shared working tree via the write-lock — only the read-only CR pass gains the explicit-request opt-in of `@rules/git/general.mdc` *Worktrees / Workspaces*, which carries no write-lock and never contends with the writing path. Each CR agent creates the worktree with `git worktree add` (a throwaway read-only review checkout — never edits / commits / pushes there), records its path in the handoff, and either hands it to `daidalos` for removal in cleanup (step 7: `git worktree remove` + `git worktree prune`, no `--force`) or removes it itself when run standalone. Documented in `agents/daidalos.md` *Concurrency & the working-tree write-lock* (now *Worktrees — writing path stays on the shared tree, read-only CR may isolate*) + step 6/7, `agents/argos.md` / `agents/athena.md` *Parallel review worktree*, and mirrored in `docs/agents.md`. Per user request — *CR agenti smí používat git worktrees pro paralelní code review; Daidalos po dokončení a merge vše uklidí, aby v repository nebyl bordel*. + - ✨ **Added**: new **`laravel-authorization-review`** skill (#716) — a Laravel-native authorization / IDOR (broken object-level authorization, BOLA) reviewer for use in Laravel code review. Adapted from the open-source `laravel-authorization-review` skill, it walks the authorization chain of every HTTP route (middleware → `authorize`/policy/gate → Eloquent query scoping → API Resource output), anchors **every** finding to real `php artisan route:list --json` output plus a cited `file:line`, classifies by confidence (High / Medium / Low), maps each finding onto the repo CR severity scale (Critical / Moderate / Minor) per `@rules/code-review/general.mdc`, and produces a per-route coverage map. Read-only / advise-only — never edits code. Ships `references/auth-patterns.md` (how Laravel authz legitimately appears across middleware, policies, gates, and `route:list --json`), `references/public-by-design.md` (routes meant to be unauthenticated), and `templates/report.md` (coverage-map report template). Complements `laravel-security` (broad 7-area audit) and `security-review` by going deep on the authorization lane. Catalogued under `skills/laravel-authorization-review/`; README skill count bumped 61 → 62. - 📝 **Changed**: `daidalos` now processes **every task it resolves from the issue tracker strictly sequentially — one source at a time, never fanning out in parallel** — and **git worktrees are forbidden** across the whole workflow. The former *Parallel analysis fan-out* (multiple `metis` runs in one message) is replaced by *Sequential processing of multiple sources*: a single request that resolves to several issues finishes one source's run completely before starting the next, ordered by the dependency-aware plan (or oldest-first). The concurrency / write-lock section drops every isolated-worktree escape — concurrent full-delivery runs serialise on the single shared working tree, and a run that finds a live lock holder waits and retries rather than spinning up a worktree. Documented in `agents/daidalos.md` *Concurrency & the working-tree write-lock* + *Sequential processing of multiple sources* and mirrored in `docs/agents.md`. The intra-task `argos` ‖ `athena` security/quality CR pass stays parallel — it is two reviews of one task, not two tasks. Per user request — *všechny úkoly z issue trackeru řešit systematicky a nikoliv paralelně; výslovně zakázat git worktrees*. diff --git a/agents/argos.md b/agents/argos.md index 33778e1..9270a2d 100644 --- a/agents/argos.md +++ b/agents/argos.md @@ -36,6 +36,14 @@ You accept one **source** for the review, in this order of preference: When the caller passes a **shared brief path** (`.claude/run/.md`), it is the run's shared memory — **read it first** as the authoritative context (resolved source, gathered data, work-breakdown plan, and every prior specialist's handoff) so you don't re-derive what is already there. When you finish, **append your handoff section** to it via `Bash` (`cat >> "$BRIEF" <<'EOF' … EOF`: `### argos — CR done` plus the result you return) so the next specialist inherits it. Because you may run in parallel with `athena` on the same brief, **guard the append with the per-brief append lock** (`tries=0; until mkdir "$BRIEF.lock" 2>/dev/null; do sleep 0.2; tries=$((tries+1)); [ "$tries" -gt 50 ] && rm -rf "$BRIEF.lock"; done; cat >> "$BRIEF" …; rmdir "$BRIEF.lock"`) so the two handoffs never interleave and a crashed holder never deadlocks the peer — see `agents/daidalos.md` *Shared task brief* → *Parallel handoff sharing*. Appending to this git-ignored scratch file is the **only** write you perform — your read-only stance on source, tests, and config is unchanged. Delete any temporary files you created during this run (except memory files) per `@rules/compound-engineering/general.mdc` *Temporary-file hygiene*. +## Parallel review worktree (optional) + +Because you and `athena` run as two concurrent CR passes, you **may run your review in an isolated read-only git worktree** when you need to avoid contending with the shared working tree (for example, a writing run is still touching the tree, or the two CR passes would otherwise step on each other). This is the explicit-request opt-in of `@rules/git/general.mdc` *Worktrees / Workspaces*, which `daidalos` grants to the CR pass — it is **not** a default; stay in the current tree unless isolation is genuinely needed. + +- Create it with `git worktree add ` where `` is the PR head you are reviewing. This is the only filesystem write you make beyond the shared-brief append, and it adds **no** change to tracked files, branches, or history — your read-only stance on source, tests, and config is unchanged. You **read** in the worktree; you never edit, commit, push, or merge there. +- **Record the worktree path in your handoff** (and in the shared-brief append) so `daidalos` removes it during its cleanup (step 7 of `agents/daidalos.md`) — this is how it keeps the repository clean after the run / merge. +- When you run **standalone** (no `daidalos` orchestrating the cleanup), remove your own worktree after the review: verify it is not the active tree and has no uncommitted changes (never `--force`), then `git worktree remove ` followed by `git worktree prune`. + ## Output — handoff to the caller Your final message is returned to the caller as the result, so make it a clean handoff: @@ -47,5 +55,6 @@ Your final message is returned to the caller as the result, so make it a clean h - **Source:** link to the originating tracker item (GitHub issue / JIRA ticket / Bugsnag error), or `none` for the no-source fallback. - **Counts:** Critical / Moderate / Minor. - **Assignment conformance:** `conformant` / `N gap(s)` / `no linked issue`. +- **Worktree:** the path of any review worktree you created (so `daidalos` removes it in cleanup), or `none` when you reviewed in the shared tree. Hand the next agent everything it needs to act (apply fixes, merge) without re-deriving where the review lives. Stop after the handoff — applying fixes or merging is a different agent's job. diff --git a/agents/athena.md b/agents/athena.md index 9e253be..5c508cd 100644 --- a/agents/athena.md +++ b/agents/athena.md @@ -89,6 +89,14 @@ Since both run concurrently, `argos` does not see your handoff during its own ru When the caller passes a **shared brief path** (`.claude/run/.md`), it is the run's shared memory — **read it first** as the authoritative context (resolved source, gathered data, work-breakdown plan, and every prior specialist's handoff) so you don't re-derive what is already there. When you finish, **append your handoff section** to it via `Bash` (`cat >> "$BRIEF" <<'EOF' … EOF`: `### athena — Security CR done` plus the result you return) so the next specialist inherits it. Because you run in parallel with `argos` on the same brief, **guard the append with the per-brief append lock** (`tries=0; until mkdir "$BRIEF.lock" 2>/dev/null; do sleep 0.2; tries=$((tries+1)); [ "$tries" -gt 50 ] && rm -rf "$BRIEF.lock"; done; cat >> "$BRIEF" …; rmdir "$BRIEF.lock"`) so the two handoffs never interleave and a crashed holder never deadlocks the peer — see `agents/daidalos.md` *Shared task brief* → *Parallel handoff sharing*. Appending to this git-ignored scratch file is the **only** write you perform — your read-only stance on source, tests, and config is unchanged. Delete any temporary files you created during this run (except memory files) per `@rules/compound-engineering/general.mdc` *Temporary-file hygiene*. +## Parallel review worktree (optional) + +Because you and `argos` run as two concurrent CR passes, you **may run your security review in an isolated read-only git worktree** when you need to avoid contending with the shared working tree (for example, a writing run is still touching the tree, or the two CR passes would otherwise step on each other). This is the explicit-request opt-in of `@rules/git/general.mdc` *Worktrees / Workspaces*, which `daidalos` grants to the CR pass — it is **not** a default; stay in the current tree unless isolation is genuinely needed. It applies to the post-implementation **security review mode** only — the pre-implementation security-analysis mode reviews no diff. + +- Create it with `git worktree add ` where `` is the PR head you are reviewing. This is the only filesystem write you make beyond the shared-brief append, and it adds **no** change to tracked files, branches, or history — your read-only stance is unchanged. You **read** in the worktree; you never edit, commit, push, or merge there. +- **Record the worktree path in your handoff** (and in the shared-brief append) so `daidalos` removes it during its cleanup (step 7 of `agents/daidalos.md`) — this is how it keeps the repository clean after the run / merge. +- When you run **standalone** (no `daidalos` orchestrating the cleanup), remove your own worktree after the review: verify it is not the active tree and has no uncommitted changes (never `--force`), then `git worktree remove ` followed by `git worktree prune`. + ## Output — handoff to the caller Your final message is returned to the caller as the result, so make it a clean handoff: @@ -100,5 +108,6 @@ Your final message is returned to the caller as the result, so make it a clean h - **Source:** link to the originating tracker item (GitHub issue / JIRA ticket / Bugsnag error), or `none`. - **Counts:** Critical / Moderate / Minor. - **Skills run:** which of the four security skills executed (and which were skipped with reason, e.g. "laravel-security skipped — not a Laravel project"). +- **Worktree:** the path of any review worktree you created (so `daidalos` removes it in cleanup), or `none` when you reviewed in the shared tree. Hand the next agent (`talos` to implement an analysis, `argos` / `daidalos` to act on a CR) everything it needs without re-deriving the findings. Stop after the handoff — implementing fixes, consolidating CR results, and publishing summaries are other agents' jobs. diff --git a/agents/daidalos.md b/agents/daidalos.md index 98eb309..1716469 100644 --- a/agents/daidalos.md +++ b/agents/daidalos.md @@ -49,13 +49,15 @@ Brief layout: ## Concurrency & the working-tree write-lock -**No git worktrees (hard rule).** This workflow **never uses git worktrees**. You do not create one, you do not request one, and you never instruct any specialist (`talos` included) to run in an isolated worktree. Every run — yours and every concurrent `daidalos` — operates on the **single shared git working tree** of the project. There is no isolated-worktree escape for running writing work in parallel; concurrent writers serialise on the write-lock below instead. This is intentionally **stricter** than `@rules/git/general.mdc` *Worktrees / Workspaces* (which permits an explicit-request opt-in): daidalos removes that opt-in for its own workflow, so a writing run never gains a worktree and always serialises on the shared tree. +**Worktrees — writing path stays on the shared tree, read-only CR may isolate.** **The writing path never uses git worktrees**: you never create one for, request one for, or instruct `talos` to run in an isolated worktree. Every writing run — yours and every concurrent `daidalos` — operates on the **single shared git working tree** of the project, and there is no isolated-worktree escape for the writing path; concurrent writers serialise on the write-lock below instead. + +The **read-only code-review agents (`argos`, `athena`) may use a git worktree** to run their reviews in parallel when needed — this is the explicit-request opt-in of `@rules/git/general.mdc` *Worktrees / Workspaces*, which this workflow grants to the CR pass only. Because the CR agents never write to the tree, an isolated read-only checkout lets the two parallel CR passes review a stable diff without contending with the writing path or each other. A CR worktree is a throwaway review checkout: the agent reads in it and never edits / commits / pushes there, it records the worktree path in its handoff, and **`daidalos` removes it during cleanup (step 7)** so nothing orphaned survives the run. Multiple top-level `daidalos` runs can be launched against the **same project at the same time** (interactively). Because worktrees are forbidden, they all share **one git working tree**. Two runs that both **write** to that tree corrupt each other: `talos` checks out a branch, edits files, and may `git stash` / `reset` — pulling the tree out from under another run and colliding on uncommitted edits. A run that only **reads** the tree (analysis-only → `metis`) is safe to overlap. The fix is a **scope-conditioned write-lock**: only the writing scope (full-delivery → dispatch `talos`) takes the lock and serialises; the read-only scope (analysis-only → `metis`) takes nothing and stays parallel. The scope signal already exists — it is the classification you make in step 3 (`analysis-only` vs `full-delivery`). -- **Keyed to the working-tree toplevel.** The lock lives at `.claude/run/.daidalos-write.lock` (a **directory**), inside the same git-ignored `.claude/run/` of the current toplevel as the shared brief. Because worktrees are forbidden, every run resolves to the **same** toplevel and the **same** lock, so concurrent full-delivery runs always contend on it and **serialise** on the shared tree — there is no isolated-worktree toplevel that could take a non-conflicting lock and slip past the serialisation. +- **Keyed to the working-tree toplevel.** The lock lives at `.claude/run/.daidalos-write.lock` (a **directory**), inside the same git-ignored `.claude/run/` of the current toplevel as the shared brief. Because the writing path never uses worktrees, every full-delivery run resolves to the **same** toplevel and the **same** lock, so concurrent writing runs always contend on it and **serialise** on the shared tree — there is no isolated-worktree toplevel that a writing run could take to slip past the serialisation (the read-only CR worktree carries no write-lock and never contends here). - **Atomic acquire.** Create the lock with `mkdir .claude/run/.daidalos-write.lock` — `mkdir` is atomic and fails when the directory already exists, so it doubles as a test-and-set. On success, write the holder inside it (`PID`, the run's `source-slug`, and an ISO-8601 timestamp). - **Stale reclaim.** When `mkdir` fails, read the holder's `PID` and probe it with `kill -0 "$PID"`. If the probe fails, the holder is a dead run, the lock is **stale** — `rm -rf` it and re-acquire. If the probe succeeds, a live full-delivery run holds this working tree: **do not proceed into `talos`**. - **Release.** Remove the lock (`rm -rf .claude/run/.daidalos-write.lock`) during cleanup in step 7 — on the final report **and** on any `Blocked` stop — right next to the existing `rm -f "$BRIEF"`, so no orphan lock survives the run. @@ -82,7 +84,7 @@ Only the full-delivery path acquires and releases the lock. An analysis-only run - **Security-focused task → analyse with `athena` (security-risk analysis, on demand).** When the task is primarily a security fix, security hardening, or vulnerability remediation, or the assignment carries a cyber-security question, **dispatch `athena` through the Task tool** in its **security analysis mode** with the resolved source **and the shared brief path**. `athena` scopes the security risk and publishes a remediation plan artifact (`Security analysis done`), which you feed to `talos` for implementation in step 5; `athena` then runs again as the security CR after `talos` (step 6). For a task that mixes security with substantial general design, dispatch **both** `metis` (general analysis) and `athena` (security analysis) — both plans feed `talos`. `athena`'s analysis mode is dispatchable only when she is registered (see the registration note in step 6); when she is not registered, note *„athena není registrována — security analýza běží inline v metis → security-review"* and fall back to `metis` with a security-focused brief. Do not duplicate `@skills/analyze-problem` or the security skills — `athena` owns them. - **Ambiguous, large, multi-interpretation, high-impact, or the user wants a plan first** → **dispatch `metis` through the Task tool** with the resolved source **and the shared brief path**, and feed its `Analysis done` handoff (the plan link) as the context for step 5. When the work was decomposed in step 1, the same `metis` dispatch also prepares each issue's independently deliverable assignment and fills its `## Dependencies` / ordering — via the ordered Implementation steps of `@skills/analyze-problem` and the `## Dependencies` template field of `@skills/create-issues-from-text` — `daidalos` does not re-derive ordering itself. Do not duplicate those skills' rules. 5. **Implement (talos).** **Acquire the working-tree write-lock first** (per *Concurrency & the working-tree write-lock*): `mkdir .claude/run/.daidalos-write.lock`, writing the holder (`PID`, `source-slug`, ISO timestamp) inside on success. If `mkdir` fails, probe the holder's `PID` with `kill -0` — reclaim a stale lock (`rm -rf` then re-acquire) when the probe fails, but when a **live** holder is found, **do not dispatch `talos`**: stop with **`Blocked: jiný daidalos zápisový běh drží tento working tree`** and the remediation (wait for the holder to finish and retry — git worktrees are forbidden in this workflow, so there is no parallel-isolation escape). Only the full-delivery path takes this lock — the analysis-only branch of step 3 (`metis`) never acquires it, so independent read-only runs stay free to overlap (this is cross-run overlap-safety on the shared tree, not a licence to fan out a single request's sources — those are processed sequentially per *Sequential processing of multiple sources*). With the lock held, drive the implementation: **Dispatch `talos` through the Task tool** on the subject **with the shared brief path** (and the metis plan, if any). `talos` delegates to `@skills/resolve-issue` — which runs an **implementer pre-PR self-check**: `code-review` + `security-review` as a self-validation pass over its own diff (**not** the authoritative code review — that role is `argos`'s alone), iterating until no Critical/Moderate remain, then opens the pull request **as a Draft** (per `@rules/git/general.mdc` *Draft pull requests* — it is not yet ready to merge because your step-6 review-and-fix loop runs after it). Wait for the `Impl done` handoff (PR link). Do not run those reviews yourself. If `talos` returns `Blocked: sandbox denied file write` instead, **stop and escalate that blocker to the user with its remediation** (enable subagent file writes — see `docs/agents.md` *Troubleshooting — subagent file writes blocked*); never implement the change yourself in your own context to work around the denial (`@rules/compound-engineering/general.mdc` *Blocked delegation is a hard stop*). After receiving the `Impl done` handoff from `talos`, **dispatch `apollon` through the Task tool** with the PR's diff context **and the shared brief path** for a fast scoped validation pass (see `agents/apollon.md` *Fast scoped validation mode*). If `apollon` returns `Blocked`, **stop and escalate** to `talos` with the details — do not proceed to the review-and-fix loop on a blocked scoped validation. -6. **Review-and-fix loop (talos ↔ argos ‖ athena) to convergence.** **Dispatch `argos` through the Task tool** on the opened PR **with the shared brief path** (code quality / architecture / optimisation CR). In parallel, **dispatch `athena` through the Task tool** on the same PR **with the shared brief path** (security CR — when `athena` is registered; see registration note below). Both are independent Task invocations dispatched by `daidalos` — `argos` does **not** dispatch `athena` (one-level nesting rule). Both write their handoffs to the shared brief under the per-brief append lock (see *Shared task brief* → *Parallel handoff sharing*). Because they run concurrently, neither sees the other's handoff during its own run: **wait for both the `CR done` and the `Security CR done` handoffs to return and land in the brief (the barrier), then `argos` consolidates `athena`'s security findings with its own from the now-complete brief** — `argos` re-reads it before publishing and driving the fix loop, and you re-dispatch `argos` on the complete brief if it finished before `athena`. `argos` (via `process-code-review` / `code-review-github`) is the `argos` ↔ `talos` loop: it re-runs the review, applies fixes, and **iterates until `criticalCount + moderateCount == 0`** (`maxIterations = 5`). You do not drive the iteration — the specialist and its skill own it; you only read the returned handoff. +6. **Review-and-fix loop (talos ↔ argos ‖ athena) to convergence.** **Dispatch `argos` through the Task tool** on the opened PR **with the shared brief path** (code quality / architecture / optimisation CR). In parallel, **dispatch `athena` through the Task tool** on the same PR **with the shared brief path** (security CR — when `athena` is registered; see registration note below). When you dispatch the two CR passes in parallel, tell each in its dispatch prompt that it **may run its review in an isolated read-only git worktree** when it needs to avoid contending with the shared working tree (per *Worktrees — writing path stays on the shared tree, read-only CR may isolate* above), and that it must **record any worktree path in its handoff** so you can remove it during cleanup (step 7). Both are independent Task invocations dispatched by `daidalos` — `argos` does **not** dispatch `athena` (one-level nesting rule). Both write their handoffs to the shared brief under the per-brief append lock (see *Shared task brief* → *Parallel handoff sharing*). Because they run concurrently, neither sees the other's handoff during its own run: **wait for both the `CR done` and the `Security CR done` handoffs to return and land in the brief (the barrier), then `argos` consolidates `athena`'s security findings with its own from the now-complete brief** — `argos` re-reads it before publishing and driving the fix loop, and you re-dispatch `argos` on the complete brief if it finished before `athena`. `argos` (via `process-code-review` / `code-review-github`) is the `argos` ↔ `talos` loop: it re-runs the review, applies fixes, and **iterates until `criticalCount + moderateCount == 0`** (`maxIterations = 5`). You do not drive the iteration — the specialist and its skill own it; you only read the returned handoff. - **Athena registration:** dispatch `athena` only when she is registered as a dispatchable subagent (installer must have copied `agents/athena.md` to `.claude/agents/`). When `athena` is not registered, note *„athena není registrována — security běží inline v code-review-github → security-review"* and continue with the standard `argos` dispatch only. - **No argos→athena nesting:** `argos` does **not** dispatch `athena` — both are dispatched by `daidalos` at the same level. This is the one-level nesting rule in practice. - **Convergence gate:** the run is "done" only when the handoff reports the loop exited with **0 Critical + 0 Moderate** (Minor does not block). Security Critical findings from `athena` count toward this gate. On convergence, `process-code-review` promotes the PR out of Draft (`gh pr ready`) so it is ready to merge — a still-Draft PR means the loop has not converged and must never be merged (`@rules/git/general.mdc` *Draft pull requests*). @@ -95,7 +97,12 @@ Only the full-delivery path acquires and releases the lock. An analysis-only run - **Co instruovat v dispatch promptu:** sdílená cesta briefu, odkaz na PR/zdroj zadání, zvolený režim (light/full), instrukce *„publikuj konsolidovanou zpětnou vazbu (Summary of changes + How to test) do zdroje zadání přes `@skills/pr-summary` v jazyce z briefu `## Language`; headlinem komentáře bude 'Hotovo — co se změnilo a jak otestovat'"*, a instrukce jazyka (dle briefu `## Language`). - **Fallback — bez trackeru:** pokud se jedná o described-task běh bez linked tracker (detekuješ v briefu `## Source`, kde není GitHub issue URL, GitHub PR URL ani JIRA klíč), do dispatch promptu přidej: *„Zdroj zadání není tracker — nepublikuj komentář; vrať totéž shrnutí jako součást svého handoffu."* Apollon pak vrátí shrnutí v handoffu a ty ho předáš uživateli v chatu. - **Závislost na registraci:** `apollon` musí být registrovaný jako dispatchnutelný subagent (viz `docs/agents.md` *Distribution* — installer kopíruje `agents/` do `.claude/agents/`). Pokud dispatch selže (apollon není registrován), pokračuj k finálnímu reportu bez post-konvergenčního shrnutí a uveď v reportu „apollon není registrován — shrnutí v chatu chybí". -7. **Record durable lessons per agent role, report to the user, then clean up the brief.** Once converged, run a **per-agent write loop** via `@skills/record-project-memory/SKILL.md` — for each agent role that genuinely participated in this run (derivable from the handoff log in the shared brief), invoke the skill once with that role's converged context, the PR link as `SOURCE_REF`, and the role's name as `AGENT_ROLE`. Each invocation writes role-tagged entries to the target project's `docs/memory/PROJECT_MEMORY.md` (a run that surfaced nothing durable for a given role writes nothing for that role). Run the loop in participation order: `metis` (if dispatched) → `talos` → `argos` → `apollon` (if dispatched) → `daidalos` (orchestration lessons last). For any role that wasn't active in this run, skip it. You may **dispatch `talos` once more** to carry out the write loop if the skill context works better inside a subagent; alternatively run `@skills/record-project-memory/SKILL.md` inline for each role. Only **after** all role passes are complete (or skipped), summarise the result directly to the user (see handoff below), then remove the shared brief (`rm -f "$BRIEF"`) and release the write-lock acquired in step 5 (`rm -rf .claude/run/.daidalos-write.lock`) — the brief is scratch memory for the run, not a kept artifact, and the lock must not outlive the run that took it. Merging stays a separate, explicit step — do **not** auto-merge unless the user asked for the full merge chain. **You never merge anything without a code review from the CR agents, and you always request that review before any merge.** Before any merge you must hold a converged code review on the PR's **final diff** — `argos` reported the `argos` ↔ `talos` loop exited with **0 Critical + 0 Moderate** on the exact commits being merged: +7. **Record durable lessons per agent role, report to the user, then clean up the brief.** Once converged, run a **per-agent write loop** via `@skills/record-project-memory/SKILL.md` — for each agent role that genuinely participated in this run (derivable from the handoff log in the shared brief), invoke the skill once with that role's converged context, the PR link as `SOURCE_REF`, and the role's name as `AGENT_ROLE`. Each invocation writes role-tagged entries to the target project's `docs/memory/PROJECT_MEMORY.md` (a run that surfaced nothing durable for a given role writes nothing for that role). Run the loop in participation order: `metis` (if dispatched) → `talos` → `argos` → `apollon` (if dispatched) → `daidalos` (orchestration lessons last). For any role that wasn't active in this run, skip it. You may **dispatch `talos` once more** to carry out the write loop if the skill context works better inside a subagent; alternatively run `@skills/record-project-memory/SKILL.md` inline for each role. Only **after** all role passes are complete (or skipped), summarise the result directly to the user (see handoff below), then **clean up after the run so no mess survives in the repository or the codebase**: + - **Remove every CR worktree** that `argos` / `athena` created for the parallel review (their handoffs record the paths). For each, verify it is not the active working tree and has no uncommitted changes — never pass `--force` — then `git worktree remove ` followed by `git worktree prune` to clear stale metadata, mirroring `@rules/git/general.mdc` *Worktrees / Workspaces* and the post-merge step of `@skills/merge-github-pr/SKILL.md`. Skip a worktree that no longer exists. + - **Remove the shared brief** (`rm -f "$BRIEF"`) and **release the write-lock** acquired in step 5 (`rm -rf .claude/run/.daidalos-write.lock`) — the brief is scratch memory for the run, not a kept artifact, and the lock must not outlive the run that took it. + - **After a merge, leave no orphans behind.** When this run ends in a merge (the user asked for the merge chain), `@skills/merge-github-pr/SKILL.md` already deletes the merged branch, removes any opt-in worktree, and returns the tree to the default branch; confirm that happened and additionally `git worktree prune` plus remove any temporary `.md` / scratch files the run created (per `@rules/compound-engineering/general.mdc` *Temporary-file hygiene*) so the repository and codebase are left clean. + + Merging stays a separate, explicit step — do **not** auto-merge unless the user asked for the full merge chain. **You never merge anything without a code review from the CR agents, and you always request that review before any merge.** Before any merge you must hold a converged code review on the PR's **final diff** — `argos` reported the `argos` ↔ `talos` loop exited with **0 Critical + 0 Moderate** on the exact commits being merged: - When the merge follows a full run, the convergence gate of step 6 already produced that review — proceed only if it reported 0 Critical + 0 Moderate. - When the merge is requested on its own (an existing PR you did not just take through step 6), or the diff changed after the last review (new commits pushed → the prior review is stale), **first dispatch `argos` through the Task tool** to run the review-and-fix loop on the current diff to convergence (step 6), and only then merge. Never merge on a stale, partial, or absent code review. diff --git a/docs/agents.md b/docs/agents.md index 5bc7d3d..ebd903d 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -138,11 +138,11 @@ The handoff above is the *return* channel. For the *forward* channel — passing ## Concurrency — working-tree write-lock -Several top-level `daidalos` runs can target the **same project at once** (interactively). **Git worktrees are forbidden in this workflow**, so every run shares **one git working tree** and two runs that both write to it would corrupt each other's checkout and uncommitted edits. `daidalos` guards this with a **scope-conditioned write-lock**, and processes the sources of a single request **sequentially, never fanning out**: +Several top-level `daidalos` runs can target the **same project at once** (interactively). **The writing path never uses git worktrees**, so every writing run shares **one git working tree** and two runs that both write to it would corrupt each other's checkout and uncommitted edits. `daidalos` guards this with a **scope-conditioned write-lock**, and processes the sources of a single request **sequentially, never fanning out**. The read-only code-review agents (`argos`, `athena`) **may** opt into a throwaway read-only worktree for their parallel review — they carry no write-lock, so they never contend here, and `daidalos` removes any CR worktree during its post-run cleanup: - **Read-only runs overlap.** An analysis-only run (dispatching `metis`) — including a decomposition run that produces issues instead of a PR — never modifies the working tree, so it takes **no** lock — any number of independent analysis or decomposition runs overlap freely, with each other and with a writing run. When a single request resolves multiple sources, they are still processed **one at a time** (no parallel fan-out); when multiple linked issues exist, `daidalos` plans a dependency-aware resolve order (reading `## Dependencies` from each issue) that takes precedence over oldest-first when issues are interlinked. -- **Writing runs serialise.** A full-delivery run (dispatching `talos`) acquires a lock before the dispatch and runs one at a time. A second writing run that finds a live holder stops with `Blocked` and a remediation (**wait for the holder to finish and retry** — worktrees are forbidden, so there is no isolated-worktree escape to run in parallel) instead of dispatching `talos` into another run's changes. -- **Keyed to the toplevel.** The lock is a directory at `.claude/run/.daidalos-write.lock` inside the current toplevel's git-ignored `.claude/run/`. Because worktrees are forbidden, every run resolves to the same toplevel and the same lock, so concurrent full-delivery runs always serialise on the shared tree. Acquire is atomic (`mkdir`), a stale lock from a crashed run is reclaimed via a `kill -0` PID probe, and the lock is released on the final report and on any `Blocked` stop. See `agents/daidalos.md` *Concurrency & the working-tree write-lock* for the mechanism. +- **Writing runs serialise.** A full-delivery run (dispatching `talos`) acquires a lock before the dispatch and runs one at a time. A second writing run that finds a live holder stops with `Blocked` and a remediation (**wait for the holder to finish and retry** — the writing path takes no worktree, so there is no isolated-worktree escape to run writing work in parallel) instead of dispatching `talos` into another run's changes. +- **Keyed to the toplevel.** The lock is a directory at `.claude/run/.daidalos-write.lock` inside the current toplevel's git-ignored `.claude/run/`. Because the writing path never uses worktrees, every full-delivery run resolves to the same toplevel and the same lock, so concurrent writing runs always serialise on the shared tree. Acquire is atomic (`mkdir`), a stale lock from a crashed run is reclaimed via a `kill -0` PID probe, and the lock is released on the final report and on any `Blocked` stop. See `agents/daidalos.md` *Concurrency & the working-tree write-lock* for the mechanism. ## Subagents of an agent diff --git a/tests/Installer/AgentsTest.php b/tests/Installer/AgentsTest.php index 4299377..465208c 100644 --- a/tests/Installer/AgentsTest.php +++ b/tests/Installer/AgentsTest.php @@ -315,16 +315,34 @@ expect($content)->toContain('classify **each one independently**'); }); -test('daidalos forbids git worktrees and serialises concurrent writers on the single shared working tree', function (): void { +test('daidalos keeps the writing path on the shared tree but lets read-only CR agents isolate in a worktree, and cleans them up', function (): void { $packageDir = dirname(__DIR__, 2); $content = (string) file_get_contents($packageDir . '/agents/daidalos.md'); - // Explicit hard rule: no git worktrees anywhere in the workflow. - expect($content)->toContain('No git worktrees (hard rule)'); - expect($content)->toContain('never uses git worktrees'); - // Concurrent writers serialise on the single shared tree — no isolated-worktree escape. + // The writing path (talos) still never uses worktrees — concurrent writers serialise on the shared tree. + expect($content)->toContain('The writing path never uses git worktrees'); expect($content)->toContain('single shared git working tree'); - expect($content)->toContain('there is no isolated-worktree toplevel'); + expect($content)->toContain('there is no isolated-worktree escape for the writing path'); + // Read-only CR agents may isolate in a worktree for parallel review. + expect($content)->toContain('read-only code-review agents (`argos`, `athena`) may use a git worktree'); + // Daidalos owns worktree cleanup so the repo stays clean after the run / merge. + expect($content)->toContain('git worktree remove'); + expect($content)->toContain('git worktree prune'); +}); + +test('the read-only CR agents document an optional parallel-review worktree they hand back for daidalos cleanup', function (): void { + $packageDir = dirname(__DIR__, 2); + + foreach (['argos', 'athena'] as $agent) { + $content = (string) file_get_contents($packageDir . '/agents/' . $agent . '.md'); + // The CR agent may isolate its review in a read-only worktree when needed. + expect($content)->toContain('Parallel review worktree'); + expect($content)->toContain('git worktree add'); + // It hands the path back so daidalos removes it during cleanup. + expect($content)->toContain('Record the worktree path in your handoff'); + // Standalone runs clean up after themselves. + expect($content)->toContain('git worktree remove'); + } }); test('agents directory ships the apollon test-engineer subagent with required frontmatter', function (): void { From 559f2bb81c4e0831a2efbb6bb09572bbaccf8043 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Kr=C3=A1l?= Date: Mon, 29 Jun 2026 17:12:04 +0200 Subject: [PATCH 2/2] docs(daidalos): align leftover worktree-forbidden prose with the CR opt-in Two restatements still said worktrees were forbidden workflow-wide, which contradicted the new read-only CR worktree opt-in in the same file. Scope both to the writing path, consistent with docs/agents.md. Self-CR (PR #728): resolves 2 Moderate consistency findings. --- agents/daidalos.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agents/daidalos.md b/agents/daidalos.md index 1716469..c247134 100644 --- a/agents/daidalos.md +++ b/agents/daidalos.md @@ -53,7 +53,7 @@ Brief layout: The **read-only code-review agents (`argos`, `athena`) may use a git worktree** to run their reviews in parallel when needed — this is the explicit-request opt-in of `@rules/git/general.mdc` *Worktrees / Workspaces*, which this workflow grants to the CR pass only. Because the CR agents never write to the tree, an isolated read-only checkout lets the two parallel CR passes review a stable diff without contending with the writing path or each other. A CR worktree is a throwaway review checkout: the agent reads in it and never edits / commits / pushes there, it records the worktree path in its handoff, and **`daidalos` removes it during cleanup (step 7)** so nothing orphaned survives the run. -Multiple top-level `daidalos` runs can be launched against the **same project at the same time** (interactively). Because worktrees are forbidden, they all share **one git working tree**. Two runs that both **write** to that tree corrupt each other: `talos` checks out a branch, edits files, and may `git stash` / `reset` — pulling the tree out from under another run and colliding on uncommitted edits. A run that only **reads** the tree (analysis-only → `metis`) is safe to overlap. +Multiple top-level `daidalos` runs can be launched against the **same project at the same time** (interactively). Because the writing path never uses worktrees, every writing run shares **one git working tree**. Two runs that both **write** to that tree corrupt each other: `talos` checks out a branch, edits files, and may `git stash` / `reset` — pulling the tree out from under another run and colliding on uncommitted edits. A run that only **reads** the tree (analysis-only → `metis`) is safe to overlap. The fix is a **scope-conditioned write-lock**: only the writing scope (full-delivery → dispatch `talos`) takes the lock and serialises; the read-only scope (analysis-only → `metis`) takes nothing and stays parallel. The scope signal already exists — it is the classification you make in step 3 (`analysis-only` vs `full-delivery`). @@ -83,7 +83,7 @@ Only the full-delivery path acquires and releases the lock. An analysis-only run - **Clear, well-specified, low-risk** → skip analysis; go to step 5. (`talos`/`resolve-issue` still runs its own internal specificity gate.) - **Security-focused task → analyse with `athena` (security-risk analysis, on demand).** When the task is primarily a security fix, security hardening, or vulnerability remediation, or the assignment carries a cyber-security question, **dispatch `athena` through the Task tool** in its **security analysis mode** with the resolved source **and the shared brief path**. `athena` scopes the security risk and publishes a remediation plan artifact (`Security analysis done`), which you feed to `talos` for implementation in step 5; `athena` then runs again as the security CR after `talos` (step 6). For a task that mixes security with substantial general design, dispatch **both** `metis` (general analysis) and `athena` (security analysis) — both plans feed `talos`. `athena`'s analysis mode is dispatchable only when she is registered (see the registration note in step 6); when she is not registered, note *„athena není registrována — security analýza běží inline v metis → security-review"* and fall back to `metis` with a security-focused brief. Do not duplicate `@skills/analyze-problem` or the security skills — `athena` owns them. - **Ambiguous, large, multi-interpretation, high-impact, or the user wants a plan first** → **dispatch `metis` through the Task tool** with the resolved source **and the shared brief path**, and feed its `Analysis done` handoff (the plan link) as the context for step 5. When the work was decomposed in step 1, the same `metis` dispatch also prepares each issue's independently deliverable assignment and fills its `## Dependencies` / ordering — via the ordered Implementation steps of `@skills/analyze-problem` and the `## Dependencies` template field of `@skills/create-issues-from-text` — `daidalos` does not re-derive ordering itself. Do not duplicate those skills' rules. -5. **Implement (talos).** **Acquire the working-tree write-lock first** (per *Concurrency & the working-tree write-lock*): `mkdir .claude/run/.daidalos-write.lock`, writing the holder (`PID`, `source-slug`, ISO timestamp) inside on success. If `mkdir` fails, probe the holder's `PID` with `kill -0` — reclaim a stale lock (`rm -rf` then re-acquire) when the probe fails, but when a **live** holder is found, **do not dispatch `talos`**: stop with **`Blocked: jiný daidalos zápisový běh drží tento working tree`** and the remediation (wait for the holder to finish and retry — git worktrees are forbidden in this workflow, so there is no parallel-isolation escape). Only the full-delivery path takes this lock — the analysis-only branch of step 3 (`metis`) never acquires it, so independent read-only runs stay free to overlap (this is cross-run overlap-safety on the shared tree, not a licence to fan out a single request's sources — those are processed sequentially per *Sequential processing of multiple sources*). With the lock held, drive the implementation: **Dispatch `talos` through the Task tool** on the subject **with the shared brief path** (and the metis plan, if any). `talos` delegates to `@skills/resolve-issue` — which runs an **implementer pre-PR self-check**: `code-review` + `security-review` as a self-validation pass over its own diff (**not** the authoritative code review — that role is `argos`'s alone), iterating until no Critical/Moderate remain, then opens the pull request **as a Draft** (per `@rules/git/general.mdc` *Draft pull requests* — it is not yet ready to merge because your step-6 review-and-fix loop runs after it). Wait for the `Impl done` handoff (PR link). Do not run those reviews yourself. If `talos` returns `Blocked: sandbox denied file write` instead, **stop and escalate that blocker to the user with its remediation** (enable subagent file writes — see `docs/agents.md` *Troubleshooting — subagent file writes blocked*); never implement the change yourself in your own context to work around the denial (`@rules/compound-engineering/general.mdc` *Blocked delegation is a hard stop*). After receiving the `Impl done` handoff from `talos`, **dispatch `apollon` through the Task tool** with the PR's diff context **and the shared brief path** for a fast scoped validation pass (see `agents/apollon.md` *Fast scoped validation mode*). If `apollon` returns `Blocked`, **stop and escalate** to `talos` with the details — do not proceed to the review-and-fix loop on a blocked scoped validation. +5. **Implement (talos).** **Acquire the working-tree write-lock first** (per *Concurrency & the working-tree write-lock*): `mkdir .claude/run/.daidalos-write.lock`, writing the holder (`PID`, `source-slug`, ISO timestamp) inside on success. If `mkdir` fails, probe the holder's `PID` with `kill -0` — reclaim a stale lock (`rm -rf` then re-acquire) when the probe fails, but when a **live** holder is found, **do not dispatch `talos`**: stop with **`Blocked: jiný daidalos zápisový běh drží tento working tree`** and the remediation (wait for the holder to finish and retry — the writing path takes no worktree, so there is no parallel-isolation escape). Only the full-delivery path takes this lock — the analysis-only branch of step 3 (`metis`) never acquires it, so independent read-only runs stay free to overlap (this is cross-run overlap-safety on the shared tree, not a licence to fan out a single request's sources — those are processed sequentially per *Sequential processing of multiple sources*). With the lock held, drive the implementation: **Dispatch `talos` through the Task tool** on the subject **with the shared brief path** (and the metis plan, if any). `talos` delegates to `@skills/resolve-issue` — which runs an **implementer pre-PR self-check**: `code-review` + `security-review` as a self-validation pass over its own diff (**not** the authoritative code review — that role is `argos`'s alone), iterating until no Critical/Moderate remain, then opens the pull request **as a Draft** (per `@rules/git/general.mdc` *Draft pull requests* — it is not yet ready to merge because your step-6 review-and-fix loop runs after it). Wait for the `Impl done` handoff (PR link). Do not run those reviews yourself. If `talos` returns `Blocked: sandbox denied file write` instead, **stop and escalate that blocker to the user with its remediation** (enable subagent file writes — see `docs/agents.md` *Troubleshooting — subagent file writes blocked*); never implement the change yourself in your own context to work around the denial (`@rules/compound-engineering/general.mdc` *Blocked delegation is a hard stop*). After receiving the `Impl done` handoff from `talos`, **dispatch `apollon` through the Task tool** with the PR's diff context **and the shared brief path** for a fast scoped validation pass (see `agents/apollon.md` *Fast scoped validation mode*). If `apollon` returns `Blocked`, **stop and escalate** to `talos` with the details — do not proceed to the review-and-fix loop on a blocked scoped validation. 6. **Review-and-fix loop (talos ↔ argos ‖ athena) to convergence.** **Dispatch `argos` through the Task tool** on the opened PR **with the shared brief path** (code quality / architecture / optimisation CR). In parallel, **dispatch `athena` through the Task tool** on the same PR **with the shared brief path** (security CR — when `athena` is registered; see registration note below). When you dispatch the two CR passes in parallel, tell each in its dispatch prompt that it **may run its review in an isolated read-only git worktree** when it needs to avoid contending with the shared working tree (per *Worktrees — writing path stays on the shared tree, read-only CR may isolate* above), and that it must **record any worktree path in its handoff** so you can remove it during cleanup (step 7). Both are independent Task invocations dispatched by `daidalos` — `argos` does **not** dispatch `athena` (one-level nesting rule). Both write their handoffs to the shared brief under the per-brief append lock (see *Shared task brief* → *Parallel handoff sharing*). Because they run concurrently, neither sees the other's handoff during its own run: **wait for both the `CR done` and the `Security CR done` handoffs to return and land in the brief (the barrier), then `argos` consolidates `athena`'s security findings with its own from the now-complete brief** — `argos` re-reads it before publishing and driving the fix loop, and you re-dispatch `argos` on the complete brief if it finished before `athena`. `argos` (via `process-code-review` / `code-review-github`) is the `argos` ↔ `talos` loop: it re-runs the review, applies fixes, and **iterates until `criticalCount + moderateCount == 0`** (`maxIterations = 5`). You do not drive the iteration — the specialist and its skill own it; you only read the returned handoff. - **Athena registration:** dispatch `athena` only when she is registered as a dispatchable subagent (installer must have copied `agents/athena.md` to `.claude/agents/`). When `athena` is not registered, note *„athena není registrována — security běží inline v code-review-github → security-review"* and continue with the standard `argos` dispatch only. - **No argos→athena nesting:** `argos` does **not** dispatch `athena` — both are dispatched by `daidalos` at the same level. This is the one-level nesting rule in practice.