feat: pipeline-conductor agent, harness skill, and design doc - #7238
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound M0 productization of a proven pattern, but the deterministic probe rests on prompt-synced state and reverse-engineered private storage formats. Watch
Suggestions
[DESIGN-REVIEWED] e3872ae |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsAll three candidates are self-rated low-confidence and each rests on a premise I could not re-derive from the code:
Nothing in the two bundled scripts or the installer surfaced a grounded, reachable, in-diff defect at the required bar during falsification. No findings. [OPUS-REVIEWED] e3872ae Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- src/kiro_crew/builtin_skills/pipeline-conductor/scripts/fleet_probe.py:50 -- False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence gathered. Emitting the review. First-Principles-Verdict: CONCERNS Every item traces to a logged failure, but What this change shipsIntent: make fleet supervision of an issue→PR pipeline a repeatable artifact instead of one session's improvised context — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] e3872ae |
1a9a7cc to
44a4210
Compare
44a4210 to
8515354
Compare
8515354 to
74a0555
Compare
74a0555 to
b902eea
Compare
|
Disposition for GPT round 3 (head b902eea): Both BLOCKING findings fixed forward:
Advisory: host-wide banned-process scan. Reporting is host-wide by design -- the load-forensics question is "what is crushing this host", and a fleet-scoped scan cannot answer it. The ACTION is ownership-gated one layer up: the skill's governance section instructs the conductor to act only on fleet-owned processes (platform processes and legitimate targeted runs exempt), and since round 2 the BANNED line carries pid + matched rule only, so the conductor must positively identify the owner before any stop. This mirrors the proven fleet-run behavior (a platform skill-loader process appeared in the sweep and was correctly left untouched). Advisory: remove |
390709d to
c469b96
Compare
|
Round-9 disposition: the three credit_spend blockers are fixed on this head (verdict precedence exhausted > any-unmetered > truncated > within; corrupt matched rows degrade completeness; non-finite/non-positive budgets exit 2), each pinned by a test. The function-local-import finding is re-noted as previously dispositioned (rounds 3-4): |
c469b96 to
dc10c5e
Compare
dc10c5e to
b9a095b
Compare
|
Rebased onto current main after the 13-PR merge batch put this branch in conflict; the conflict is resolved and the PR is MERGEABLE again. The remaining reds on this head are main-owned and reproduce on PRs with disjoint diffs, so they are not actionable here:
Per house rule the main-owned fixes are not being folded in here. Once main heals I will rebase to cut a fresh merge ref and re-run, rather than re-triggering against a stale one. |
|
Follow-up on this head (
While this branch was open, main added a second test pinning the reserved set -- Fixed by updating that assertion to the three-name set (and its docstring from "reserved pair" to Verified: The remaining reds ( |
Dispositions for Design + First Principles CONCERNS on
|
|
Correction on item F (the SKILL.md docs pointer), head now I first tried to KEEP the citation and just mark it repo-only. Two gates said no, each for its own reason:
So the gates were telling me the reviewer had prescribed the right fix and I had talked myself out of it. The pointer is now simply dropped -- the sentence reads "Every rule below closes a named failure mode." and stops. The rationale still lives in Verified locally before pushing this time, rather than after: Still one commit. Item G (the |
… skill, and design doc A dedicated kirocrew-pipeline-conductor generated agent (mirroring the kirocrew-conductor security invariants: no file-writing tool, dashboard and core verbs auto-approved individually, create/read only), a pipeline-conductor builtin skill carrying the fleet operating procedure (probe cycle, work-order brief, intervention ladder, adjudication and override protocol, resource-posture flow control, per-item credit budgets), two subprocess-free probe scripts (fleet_probe.py with a handled-set state file and containment-checked transcript reads, credit_spend.py with within/exhausted/unmetered verdicts), and docs/design/pipeline-conductor.md recording the architecture and the PipelineSpec template seams for running the same conductor on any repository and campaign type.
|
Disposition for the two First Principles subtractions on 1. Drop the load/mem fields + That said, a reported number nobody may decide on is exactly the kind of surface this review exists to question, and deleting it is a pure subtraction. I am not folding it into this revision because the head is green with readiness passed after ten review rounds and a push re-rolls the non-deterministic lanes for a decorative field; the maintainer merging this gets the call, and if he wants it gone it is a two-line delete plus the config key. 2. Fold The same-shape observation about |
bolichen97
left a comment
There was a problem hiding this comment.
CI green, security invariants verified in diff and test-pinned. Approved.
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
What is the problem?
KiroCrew's issue-fixing automation is three open-loop pipelines (issue -> new PR, red PR -> green, green PR -> merge): scanners and dispatchers stand worker sessions up and then nothing watches them. Every failure class we have logged traces to that gap -- duplicate dispatch ending in mutual-yield deadlocks, silent stalls discovered only by the 90-minute heartbeat reaper, every exception (reviewer deadlock, main-owned CI red, wrong-premise issue) terminating in a human excavating logs, and hand-edited progress state drifting from GitHub reality.
The Pipeline Conductor closes that gap: a long-lived supervisor session per pipeline that picks up queued items, stands up one worker session per item, probes the fleet with one script call per cycle, independently verifies claimed greens, adjudicates blocked items under a published override protocol, intervenes on looping or stalled workers, governs host resources and per-item credit budgets, and digests each verified green for the human. Running this control plane ad hoc from a default-agent chat session works but does not repeat: the rules live in one session's context, the ledger is hand-edited markdown that drifts, and handled-signal tracking degenerates into hand-grown exclusion lists.
Why this issue matters to the user
The fleet pattern is how a maintainer turns a large backlog into merged PRs with the human appearing exactly twice -- the merge click and genuine design decisions. Without a productized conductor, every campaign re-improvises the harness, repeats the already-paid-for failures (double dispatch, ledger drift, host overload from full test suites), and burns the operator's attention on bookkeeping instead of decisions. And because today's pipelines are KiroCrew-shaped one-offs, none of this is reusable on another repository or another campaign type.
How our fix solves it
Symptom -> root cause: fleet failures happen because no standing entity owns supervision, and supervision was unrepeatable because its rules lived in one session's context. So this PR makes the supervisor a first-class, regenerable artifact -- an agent spec plus a skill -- with the deterministic bookkeeping demoted to scripts:
kirocrew-pipeline-conductorgenerated agent (agent.py,agent_files.py): follows the established one-installer-per-agent pattern and thekirocrew-conductorsecurity invariants -- no file-writing tool (never does a work item's work itself),@kirocrew-dashboardmounted whole but auto-approved verb by verb (create/read granted;session_send/session_stop/move gated; unattended operation uses the same session-level trust grant the worker sessions already require),execute_bashmounted but never auto-approved, KAS policy derived from the filtered grant list, withheld grants audited. Hidden from the spawn roster (subagent.py) like the other conductor.pipeline-conductorbuiltin skill: the full operating procedure -- idempotent pickup (state check + store verdict + open-PR/worktree overlap), the work-order brief whose every clause closes an observed failure mode, the one-call-per-cycle probe with an action table, independent green verification (check-runs collapsed per lane, head SHA pinned, job logs not conclusions), an intervention ladder for looping/stalled workers (nudge -> bounded read-only inspector subagent -> rule: sharpened re-dispatch / adjudicate / open-issue descope / reclaim), the adjudication + override protocol, resource-posture flow control (ample/tight/critical -> dispatch/backoff/stop), per-item credit budgets with burn review, steering-as-mode-change, and merge cleanup + reconciliation.fleet_probe.py-- batch worker-tail classification, idle age, error tails, banned-process scan (e.g. an unboundedpytestwith no-n), and host load in one call, with a handled-set state file replacing the run's hand-grown grep exclusions;credit_spend.py-- per-item credit rollups from the usage shards withwithin/exhausted/unmeteredverdicts (absent metering reads as unknown, never as zero).docs/design/pipeline-conductor.md: the architecture, the lessons-to-rules table, and the bigger-picture template vision -- aPipelineSpecwith five named seams (work-source adapter, verifier adapter, protocol vocabulary as data, adjudication policy as data, per-repo identity per Auto Triage Pipeline: make the pipeline a per-repository object under Issue Radar #6221) so the same conductor can run any repository and campaign type; M1-M3 milestones (event store, adjudication queue/SLAs, baking stage + sagas) are specified there and deliberately not shipped here.What tests we did
test/test_pipeline_conductor_agent.py(installer + both scripts): agent identity/charter, verbosity placeholder, patrol-with-monitor_startcontract, prompt names its tools and scripts, nofs_write/codemounted, dashboard grants pinned to the create/read set with mutating verbs andexecute_bashexcluded fromallowedTools, MCP servers narrowed, governed-host withholds audited with this installer as source; probe protocol-tag firing vs quietWORKING, idle alert, error tails, missing transcript, handled-set suppression + re-fire on new payload, banned-process scan (bounded-n 4run exempt), malformed config exit; credit rollup filtering, budget verdicts, unmetered-not-zero, missing dir tolerance, newest-shards bound.test/test_spawn_agent_roster.pysource ratchet for the third reserved name.test_conductor_agent.py,test_agent_spec_preflight.py,test_builtin_skill_packaging.py,test_builtin_skill_scope.py(which caught and forced repo-agnostic example config in the skill),test_brand_name_gate.py,test_spawn_audit.py, and the owned-files sweep tests intest_agent.py-- 200+ tests passing locally;isort/flake8/blackclean on touched files.Any other suggestions on the work
unmeteredverdict makes that honest, and closing the gap belongs to the planned per-request token metrics work.folderargument onsession_create(session_create should accept a folder so filing is atomic with creation #6118) would remove the one remaining approval prompt in the attended dispatch path.