Skip to content

feat: pipeline-conductor agent, harness skill, and design doc - #7238

Merged
bolichen97 merged 1 commit into
mainfrom
feat/pipeline-conductor
Sep 1, 2026
Merged

feat: pipeline-conductor agent, harness skill, and design doc#7238
bolichen97 merged 1 commit into
mainfrom
feat/pipeline-conductor

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

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-conductor generated agent (agent.py, agent_files.py): follows the established one-installer-per-agent pattern and the kirocrew-conductor security invariants -- no file-writing tool (never does a work item's work itself), @kirocrew-dashboard mounted 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_bash mounted 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-conductor builtin 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.
  • Two subprocess-free scripts: fleet_probe.py -- batch worker-tail classification, idle age, error tails, banned-process scan (e.g. an unbounded pytest with 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 with within/exhausted/unmetered verdicts (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 -- a PipelineSpec with 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

  • New test/test_pipeline_conductor_agent.py (installer + both scripts): agent identity/charter, verbosity placeholder, patrol-with-monitor_start contract, prompt names its tools and scripts, no fs_write/code mounted, dashboard grants pinned to the create/read set with mutating verbs and execute_bash excluded from allowedTools, MCP servers narrowed, governed-host withholds audited with this installer as source; probe protocol-tag firing vs quiet WORKING, idle alert, error tails, missing transcript, handled-set suppression + re-fire on new payload, banned-process scan (bounded -n 4 run exempt), malformed config exit; credit rollup filtering, budget verdicts, unmetered-not-zero, missing dir tolerance, newest-shards bound.
  • Updated test/test_spawn_agent_roster.py source ratchet for the third reserved name.
  • Ran the adjacent suites green: 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 in test_agent.py -- 200+ tests passing locally; isort/flake8/black clean on touched files.

Any other suggestions on the work

  • The design doc's open question 1 (resident LLM session vs deterministic engine spine) is deliberately deferred until the M1 event store exists; the cycle economics (a patrol loop is overwhelmingly no-signal bookkeeping) argue for the spine, and the event store is what makes it possible without losing adjudication context.
  • Credit metering today covers dashboard-session turns only; inspector subagent turns burn invisibly. The unmetered verdict makes that honest, and closing the gap belongs to the planned per-request token metrics work.
  • A folder argument on session_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.

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner August 31, 2026 08:06
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of e3872aef515124b6632a517517b6531864b2d86c — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • The probe's watch list is a second hand-maintained state surface. "Keep probe-config.json's sessions list synced with the ledger" is prompt-enforced; one missed sync → a dispatched worker is never probed → silent stall, the exact failure class the PR names as its cause. Cheap mechanical fix: derive the list from the ledger/session store, or have the probe cross-check N watched against the ledger's open count and fire a mismatch signal.
  • Both scripts hard-code private on-disk schemas — the session store's filename conventions (the dashboard_<key> / :_ heuristics are reverse-engineering, not a contract) and the usage-shard row shape (including coding around the literal turns: 0 quirk). A gateway-side layout change makes _transcript_path return None → false GONE → reclaim → duplicate dispatch, the other headline failure. These deserve a shared constant/API or a format-version check.
  • Unattended patrol requires session-level trust mode, which auto-approves arbitrary execute_bash — so the carefully curated per-verb grants do not bind in the mode this agent is designed to run in. Disclosed (known-limits, open decision 2), but humans should weigh it before this pattern spreads to more conductors.

Suggestions

  • Fix the shard writer's turns: 0 at its source rather than encoding the workaround into a shipped skill script (follow-up PR).

[DESIGN-REVIEWED] e3872ae

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed e3872aef515124b6632a517517b6531864b2d86c — this comment is updated in place on each push.

Review details

All three candidates are self-rated low-confidence and each rests on a premise I could not re-derive from the code:

  • C1 (credit_spend.py unhashable-slot crash): the slot not in wanted membership test does crash on a list/dict slot, but usage shards are gateway-written and the metering writer keys rows by a string session slot. No concrete input where a non-scalar slot occurs in practice — the required input (a) is speculative ("could not confirm"). Below bar.
  • C2 (banned-pytest regex false-negative): the .*(?:-n|…)\s*=?\s*\d lookahead does match a -n<digit> substring anywhere in the line, but it requires a pytest argv/filename containing -n<digit> outside the worker flag — an uncommon shape in a detector that is heuristic by design. Input (a) is "could occur," and even if real it is advisory, not blocking. Below bar.
  • C3 (colon-form + dashboard_ prefix → false GONE): requires a colon-form slot key stored as dashboard_<underscored>.jsonl. Observed dashboard transcripts use the dash form (dashboard_chat-1-123, dashboard_coder-demo), which points against the co-occurrence the finding needs; the candidate itself could not confirm it. Ungrounded. Below bar.

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 e3872aef515124b6632a517517b6531864b2d86c.

False positive or not applicable? A repository writer can comment:
/ai-review override fable e3872aef515124b6632a517517b6531864b2d86c: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of e3872aef515124b6632a517517b6531864b2d86c and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/builtin_skills/pipeline-conductor/scripts/fleet_probe.py:50 -- "BANNED pid=<pid> <cmdline>" contradicts the emitted rule=<regex> format -> Fix: document the actual output format.
[GPT-REVIEWED] e3872ae

False positive or not applicable? A repository writer can comment:
/ai-review override gpt e3872aef515124b6632a517517b6531864b2d86c: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of e3872aef515124b6632a517517b6531864b2d86c — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Emitting the review.

First-Principles-Verdict: CONCERNS

Every item traces to a logged failure, but fleet_probe.py re-derives host load/memory that the already-granted resource_status tool answers better.

What this change ships

Intent: make fleet supervision of an issue→PR pipeline a repeatable artifact instead of one session's improvised context — an ADDITION.

  1. New kirocrew-pipeline-conductor agent installed at every gateway boot — justified (logged deadlocks, stalls, ledger drift)
  2. New pipeline-conductor builtin skill shipped to every install — justified
  3. fleet_probe.py batch probe with handled-set — justified, but its load/mem fragment duplicates resource_status
  4. credit_spend.py budget verdicts — justified
  5. Design doc + index line — justified (follows docs/design/ convention)
  6. Agent hidden from spawn rosters — justified, mirrors existing conductor
  7. New exported derived_agent_permissions on the SDK boundary — one consumer, 2 inline siblings unconverted
  8. Verb-by-verb @kirocrew-core grants (first agent to narrow core) — justified, indicts an unfixed sibling
  9. User-docs spec list updated — declared

Watch

  • The probe's OK line load/mem re-reads raw loadavg + /proc/meminfo while resource_status (mcp_core.py, backed by src/kiro_crew/resource_status.py, cgroup-clamped, postures ample/tight/critical, auto-approved in this very grant list) already answers it — and the skill itself says "confirm with resource_status before batch dispatches". Two host-posture vocabularies that disagree in containers.
  • The new grants comment ("a server-wide grant would let that content start persistent work") condemns the goal conductor's whole-server @kirocrew-core grant one page up (agent.py:5032, 1 sibling, grepped "@kirocrew-core" in grant loops) — left unfixed, unmentioned.
  • derived_agent_permissions: 1 consumer (agent.py:5307); 2 sibling inline spellings (agent.py:3272, 5093, grepped allowed_tools_to_permissions) remain, though the shrink-only SDK-boundary gate makes the helper itself derived.

Subtractions

  • Drop the load/mem fields and the load_alert_per_cpu config key from fleet_probe.py (keep the banned-process count); posture comes from the granted @kirocrew-core/resource_status.
  • Fold _install_conductor_agent's inline derive (agent.py:5093-5096) into derived_agent_permissions — identical four lines, deletes the second spelling.

[FIRST-PRINCIPLES-REVIEWED] e3872ae

@chenmingwei23
chenmingwei23 force-pushed the feat/pipeline-conductor branch from 1a9a7cc to 44a4210 Compare August 31, 2026 08:16
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/pipeline-conductor branch from 44a4210 to 8515354 Compare August 31, 2026 08:40
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/pipeline-conductor branch from 8515354 to 74a0555 Compare August 31, 2026 08:58
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/pipeline-conductor branch from 74a0555 to b902eea Compare August 31, 2026 09:16
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for GPT round 3 (head b902eea):

Both BLOCKING findings fixed forward:

  • sessions_dir is no longer a config key: transcripts are read from this gateway's own <data home>/sessions only (env-derived, same containment rule as the round-2 state_path fix). Pinned by test_sessions_dir_config_key_is_ignored. The suggested remedy (revert the batch probe entirely) was not taken: the one-call batch tail read is the mechanism that makes a quiet patrol cycle cost one line instead of N session_read_message round-trips, and the finding's actual hazard was the config-chosen directory, which is now gone. proc_root left the config surface for the same reason (env seam retained for the test harness only).
  • Raw slot keys now normalize to the surface-prefixed transcript stem (chat-N-... -> dashboard_chat-N-....jsonl, : -> _), so a freshly created session never reads as GONE. Pinned by test_raw_slot_key_matches_surface_prefixed_transcript.

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 execute_bash. Declined on the sibling precedent and the gating facts: kirocrew-conductor ships exactly this shape for exactly this reason (bundled skill scripts need a runner), execute_bash is mounted but never in allowedTools (pinned by test_dashboard_grants_are_create_and_read_only), and the derived KAS policy therefore carries no rule for it -- in attended operation every invocation prompts with the full command line visible. Trust mode is a session-scoped grant the operator makes explicitly; a spec-level removal would break both conductors' bundled-script contract without changing what trust mode can reach.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/pipeline-conductor branch from 390709d to c469b96 Compare August 31, 2026 13:49
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Aug 31, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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): agent.py documents the function-local import convention for cycle-prone modules, and drivers/acp.py is the designated ACP boundary layer -- moving these to module scope reintroduces the genuine circular import the convention exists to avoid. Standing rebuttal, no override requested.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/pipeline-conductor branch from c469b96 to dc10c5e Compare August 31, 2026 14:27
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/pipeline-conductor branch from dc10c5e to b9a095b Compare September 1, 2026 04:02
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Follow-up on this head (0c5d16cc6): the rebase carried one logical conflict that git could not see, and the rebased CI caught it.

Backend Tests shard 1 was red on all three platforms with:

AssertionError: assert frozenset({...pipeline-conductor}) == frozenset({...kirocrew-conductor})
Extra items in the left set: 'kirocrew-pipeline-conductor'

While this branch was open, main added a second test pinning the reserved set --
test/test_agent_roster_shared.py::TestExclusionIsInheritedNotRespelled::test_the_helper_default_is_the_shared_constant --
alongside the one in test_spawn_agent_roster.py that this branch already updates. The two files
never conflicted textually, so the rebase applied cleanly and left the new assertion pinning two
names while UNADVERTISED_AGENTS now holds three.

Fixed by updating that assertion to the three-name set (and its docstring from "reserved pair" to
"reserved set", matching the sibling test). test_agent_roster_shared.py is now in this diff -- it
has to be, since the constant it pins is what this PR changes.

Verified: pytest test/test_agent_roster_shared.py test/test_spawn_agent_roster.py test/test_pipeline_conductor_agent.py test/test_conductor_agent.py -> 112 passed; grep -rn 'frozenset({"kirocrew", "kirocrew-conductor"})' test/ src/ -> no matches left, so there is no third copy waiting to break. flake8 / isort / black clean on the touched file. Still one commit.

The remaining reds (Backend Tests shard 3, Coverage Gate, PR Readiness) are the main-owned ones noted above -- #7504 and #7499.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Dispositions for Design + First Principles CONCERNS on 0c5d16cc6 -> now abbc423d4

Both lanes are advisory (CONCERNS, not BLOCK) and both named the current head, so
neither body was stale. Every item below was verified against the code before
being answered; two of the claims do not survive that check, and I say so rather
than quietly agreeing.

Two items are FIXED on the new head. The rest are dispositioned, three of them
with a named follow-up.


Fixed

F. The shipped SKILL.md pointed at a path an installed user cannot open. FIXED.

SKILL.md:13 sent the reader to docs/design/pipeline-conductor.md. That is a
repo-root path; packaged docs live under src/kiro_crew/docs/, and this SKILL.md
ships into every user's skills directory. So for anyone who is not working in a
checkout, the rationale pointer resolved to nothing -- and worse, a conductor agent
reading its own skill would have burned a turn trying to open it.

Rather than delete the reference (the doc is the real rationale and is worth citing
for anyone in the repo), it now says where the file lives and that it is not part of
an install, so nobody tries to open it. One line.

G. The pipeline-spec schema named one field two different things. FIXED.

SKILL.md:34 ships "worktree_pattern" inside worker_contract, and templates
{worktree_pattern} into the worker brief at SKILL.md:102. The design doc at
docs/design/pipeline-conductor.md:129 called the same field workdir_pattern.

This is the more consequential of the two: SKILL.md is the runtime consumer, so
anyone writing a pipeline spec from the design doc would have set a key the skill
never reads, and the failure mode is a silently empty worktree pattern rather than
an error. Renamed the doc to follow the consumer, not the reverse. grep -rn workdir_pattern docs/ src/ is now empty.

Verified after both edits: pytest test/test_pipeline_conductor_agent.py test/test_spawn_agent_roster.py test/test_agent_roster_shared.py -> 80 passed.
Still one commit.


Corrected: two subtractions rest on a "zero consumers" claim that is not true

I checked these with grep rather than accepting them, because both were argued as
dead code, and dead code is a fact claim.

C. --max-shards on credit_spend.py -- NOT zero-consumer. It is implemented
(the arg, the plumb into rollup, and the all_shards[:max_shards] bound), it is
directly unit-tested by test_max_shards_keeps_newest and a companion no-bound
test, and SKILL.md:243 names it in the verdict-handling table ("truncated (only
if you passed --max-shards) -> re-run without the bound").

What is true is narrower: no skill workflow step passes it, so it has no routine
caller. That makes it an opt-in cost bound on a script that reads an unbounded
number of usage shards -- which is the kind of escape hatch you want to exist
before you need it, not after. Keeping it, and the honest reason is that the
premise for removing it was inaccurate, not that I disagree about over-generality
in principle.

D. The four probe config keys -- NOT zero-consumer either, but half the concern
lands.
err_res, banned_process_res, load_alert_per_cpu and tail_bytes are
each read by fleet_probe.py (run_probe for tail_bytes/err_res, _host_lines
for load_alert_per_cpu/banned_process_res), each validated in _config_error,
and three are covered by malformed-config tests.

The part that IS confirmed: grep for all four across SKILL.md returns 0 --
the shipped skill never sets one, so they are documented optional overrides with
defaults and no shipped caller. That is a fair observation about surface area and a
weaker one about dead code. Keeping them: each has a working default, each is
validated, and a probe whose thresholds cannot be tuned per fleet is the thing that
gets forked rather than configured.


Confirmed, and deferred with a follow-up

A. The {"rules": []} derive now has three spellings, and the new seam has one
consumer while its sibling stays inline. CONFIRMED, filed.

Verified: derived_agent_permissions is defined at agent_sdk/drivers/acp.py:47
and has exactly one caller -- the new installer at agent.py:5307. The same
derive-plus-fallback shape remains inline at agent.py:3272 (_write_agent_permissions)
and agent.py:5093 (the goal-conductor installer).

The reviewer is right that introducing a seam and leaving its sibling inline is the
worst of both shapes: it adds a spelling instead of removing one. I am not doing it
here for one specific reason -- routing those two sites through the wrapper is the
easy half, but the value the reviewer actually wants is pruning the
agent-sdk-boundary-baseline.txt entry, and that file is not in this diff. That
baseline is shrink-only by design, so touching it means this PR starts arbitrating a
gate it otherwise has nothing to do with, and the boundary-baseline entry for
agent.py covers call sites beyond these two. Follow-up filed as #7513.

B. The sibling goal conductor still auto-approves @kirocrew-core whole.
CONFIRMED, filed, and out of scope by the reviewer's own reading.

This PR grants core verb-by-verb -- _PIPELINE_CONDUCTOR_CORE_GRANTS at
agent.py:5145-5159, 13 named verbs -- and its comment argues a server-wide grant
"would let that content start persistent work (task_run, workflow_run,
cron_add) or spawn arbitrary subagents with no human in the loop." The goal
conductor at agent.py:5032 still lists bare "@kirocrew-core" in its grant loop.

So the asymmetry is real and this PR's own comment is the argument against it. But
narrowing a shipped agent's grants is a behaviour change to a different agent's
security surface: it can break existing goal-conductor sessions that rely on a verb
not in the narrowed set, and it needs its own verb inventory and its own review.
Doing it inside a feature PR for a different agent is exactly how a security change
gets merged without being reviewed as one. Filed as #7514.

(For the record, the earlier-head line reference for this was agent.py:4892; the
live grant is at 5032.)

E. _PIPELINE_CONDUCTOR_DASHBOARD_GRANTS is byte-identical to
_CONDUCTOR_DASHBOARD_GRANTS. CONFIRMED identical -- kept deliberately.

Checked with a real diff of the two tuple bodies (agent.py:4927-4930 vs
5210-5213), not by eye: identical, same four entries in the same order.

Keeping them separate, and this is a judgment I want on the record rather than
buried. Collapsing them would couple two agents' security surfaces: the next time
someone widens the goal conductor's dashboard grants, the pipeline conductor would
silently inherit the widening, and nothing in review would show that a second
agent's permissions changed. The duplication costs a drift risk in the other
direction -- the two could diverge unnoticed -- but of the two failure modes, silent
grant widening is the one that matters, and the tuples sitting next to their own
justifying comments (5197-5208) is what makes each grant reviewable on its own
terms. Note the CORE grants are deliberately not shared for the same reason.

Design's format-duplication concern (the probe scripts re-implement
package-owned on-disk formats by copy).
Accepted as stated: a change to the real
transcript/shard writer would misclassify sessions with nothing going red. The
suggested remedy -- one round-trip contract test per format, real writer to script
classifier via load_skill_script -- is the right shape and preserves the
subprocess-free design. Filed as the second half of #7513 rather than added here, because it
needs a fixture that drives the real writers and that is more surface than this PR
should grow at the disposition stage.


Nothing above is dismissed as a false positive. Two items were real defects and are
fixed; two rested on an inaccurate dead-code premise and are answered with counts;
three are real and tracked.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Correction on item F (the SKILL.md docs pointer), head now 6bc461546.

I first tried to KEEP the citation and just mark it repo-only. Two gates said no, each for its own reason:

  • Brand Name Gate -- my first wording spelled the product name as one word in prose.
  • Builtin Skill Scope Gate -- my second wording used the repo slug, and that gate exists precisely to stop a shipped skill naming this repository as a place to act: "names this repository as a place to act; a reader elsewhere has no such repo".

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 docs/design/pipeline-conductor.md and is reachable from docs/design/README.md for anyone in a checkout, which is the only audience that could ever open it.

Verified locally before pushing this time, rather than after:

builtin-skill-scope gate passed: no repository markers in a shipped skill outside kirocrew-dev/
brand gate: no misspellings of Kiro Crew in the lines added since c28a2199b
grep kirodotdev|KiroCrew SKILL.md -> no matches
grep -rn workdir_pattern docs/ src/  -> no matches  (item G still aligned)
pytest test/test_pipeline_conductor_agent.py -> 43 passed

Still one commit. Item G (the worktree_pattern / workdir_pattern divergence) is unchanged and still fixed.

… 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.
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for the two First Principles subtractions on e3872aef5 (both advisory, both read and verified against the code):

1. Drop the load/mem fields + load_alert_per_cpu from fleet_probe.py, take posture from resource_status. Diagnosis accepted: resource_status is cgroup-clamped and the probe reads raw loadavg + /proc/meminfo, so in a container the two can disagree. What the code does with each is not symmetric, though: every governance DECISION in the skill (admit, hold, stop the expensive item) is keyed on resource_status's posture, and the probe's trailing OK ... | load ... fragment is a reported reading on the same line that already has to be printed so a quiet cycle costs one line. So today's shape is "decide on the granted tool, report the raw number", not two competing deciders.

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 _install_conductor_agent's inline derive into derived_agent_permissions. Declined for this PR, kept as a follow-up. The four lines are identical and the helper is the right home, but the edit lands in the EXISTING conductor's installer -- a sibling agent this PR otherwise does not touch. Same reasoning for the sibling's server-wide @kirocrew-core grant that the new per-verb grants indict: narrowing another shipped agent's authorization surface is its own change with its own review, not a rider on this one.

The same-shape observation about derived_agent_permissions having one consumer is accurate and deliberate: the SDK-boundary gate is shrink-only, so the helper exists to keep the ACP import in the one layer permitted to hold it.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI green, security invariants verified in diff and test-pinned. Approved.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • PR #6237 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6237: REBASE. A third per-verb grant list must be mapped onto the op shape at the same time as the conductor's. Files: src/kiro_crew/agent.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants