docs(conductor): rfc v3 -- shared board visibility and conductor-gated worker communication - #9394
Conversation
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Solid RFC revision with verified code claims, but the visibility rule "a sibling never sees prose" is false for WatchThe Phase 3b sibling digest breaks the mask rule it claims to enforce. The Visibility model says "a sibling sees pointers and enumerations, never prose" and the threat row asserts steering is "Prevented by the mask: … no free-text field" — but Suggestions
[DESIGN-REVIEWED] 669410c |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
…d worker communication Re-characterises the work ledger as one shared board per conductor, masked by identity, and promotes the three shipped phases from "proposed" to "as implemented". Direction change. v1 and v2 described a two-party record: one conductor, one worker, one item each, and a worker that could see nothing but its own row. The shipped store is already a board -- every item lives in one conductor directory -- so v3 names it that and writes the mask down. A new Visibility model section tabulates every field against four readers (conductor, this item's worker, a sibling worker, Crew page user) and gives the three rules that generate the table: a sibling sees pointers and enumerations but never prose, no reader but the conductor sees a session key, and a worker never learns the goal. Two new phases follow from that. Phase 3b adds a `siblings` digest to `work_brief` -- item_id, title, status, artifacts, last_report_at, open items only -- so item B can rebase on item A's branch without a round trip through the conductor. Phase 5 adds conductor-gated worker communication: `work_request` lets a worker ask for relay or a channel, recorded as an event on its OWN square; `work_ledger_record action=channel` writes a bounded, expiring pairwise channel; `work_message` writes a body into the target's inbox only while that channel covers the pair. Nothing on the path enqueues a prompt, and Phase 5's exit criteria assert the absence of that call directly, because that absence is what distinguishes a channel from `session_send` granted downward. Fact correction. The RFC's earlier framing said `session_send` is "withheld from workers", which reads as an allowlist decision. It is structural: `kirocrew-worker` is `build_agent_config()` plus `@kirocrew-work`, and `session_send` lives on the opt-in `kirocrew-dashboard` server that a worker spec never mounts. A worker has `send_message` / `send_notification`, which address a person rather than a session's turn queue, so the ledger is its only channel to its conductor. Also: Phase 1/2 sections rewritten as implemented with the two places the code corrected the design (`_coalesce_progress`, `MAX_RECORD_BYTES` reading as absent); a Phase 2.5 section recording the isolation walk-back and its met exit criteria; the agent-spec section corrected to the shipped grants; Q2 closed (the ledger gate wins, because a worker reports its PR anyway) and Q6 closed (consecutive progress reports coalesce); Q7, Q8 and Q9 opened for sibling summary visibility, a conductor as a channel end, and cross-level requests. The index row moves from `draft`/"Nothing" to `partial` with what is and is not on main. Tests: none -- documentation only. docs-lint, brand-name, changelog-history and focus-cue gates all run with their BASE_REF exported and pass.
9689d31 to
669410c
Compare
|
Triage review (comment only, no verdict). RFC v3 re-characterises the conductor work ledger as one shared board per conductor masked by identity, adds a Visibility model table plus Phase 3b (sibling digest) and Phase 5 (conductor-gated worker channels), and promotes the three shipped phases from proposed to as-implemented; the sibling README moves the index row from draft to partial. Pure documentation: zero source files touched, and the diff matches its design-doc-gate: Rule A (docs/request-for-change/), also Rule D on README.md and Rule C on rfc-*.md, docs-dominant with no code in the diff. The document specifies trust-boundary and inter-session-communication design (worker channels, identity masks, session_send withholding), which is exactly the class of decision an agent approval cannot stand in for. Not approved and not rejected by the pipeline: it needs a human to agree to the design. Evaluated at head SHA 669410c. |
bolichen97
left a comment
There was a problem hiding this comment.
Read the whole diff and checked every code claim in it against origin/main. The as-implemented rewrite of Phases 1/2/2b is accurate — read_work_brief's seven keys, accept_batch composing from acceptance alone, the exact grant sets in _LEDGER_CONDUCTOR_WORK_GRANTS / _WORKER_WORK_GRANTS, _narrow_conductor_mcp_servers(work=…), kirocrew-dashboard being opt_in, probes/__init__.py mapping only gh-pr, _coalesce_progress and MAX_RECORD_BYTES — all verified, and no internal references leaked.
One blocking factual problem, and it is in the new document's central claim rather than at its edges.
§Visibility model — "a sibling sees pointers and enumerations, never prose" is false for artifacts. The same section calls artifacts "a string→string map of pointers", but the shipped store validates only type and size: work_ledger.py:962-985 accepts MAX_ARTIFACT_KEYS=16 keys of MAX_ARTIFACT_VALUE_CHARS=512 each — roughly 8 KB of worker-chosen free text, with no shape constraint making a value a pointer. Phase 3b puts artifacts into the sibling digest, so worker-authored prose crosses exactly the boundary that excluding summary and gating the Phase 5 channels exist to close.
That makes the threat row "A worker reads a sibling's prose and is steered by it … Prevented by the mask … no free-text field" wrong for the same reason, and the stated defence ("adds no attack surface that artifacts did not already carry within one item") misstates the reader set: within one item the readers are the conductor and that item's own worker, not siblings. Phase 3b's exit criteria pin key presence, not value shape, so nothing would catch this at implementation time either.
This is the same finding Design Review raised as 🟡 CONCERNS on head 669410cd, still without a disposition. Two ways out, and the choice is a design decision, not an editorial one: constrain artifacts values to a pointer shape (URL / path / id) and say so in Phase 3b's exit criteria, or drop the "never prose" guarantee and treat artifacts as prose that the mask deliberately lets through, with the threat row rewritten to match.
Separately, a nit: the body says "re-audited at 7fa305f35" and "Every code claim added in this revision was read at 7fa305f35", but the diff writes audited-at: c02cdd67c and the doc text says c02cdd67c. 7fa305f35 is a real commit but not the audit SHA the artifact claims.
Not approving on the design itself — per the design-doc-gate triage this specifies a trust boundary and an inter-session communication channel, so it needs a human owner to agree to the design rather than a review bot or me.
bolichen97
left a comment
There was a problem hiding this comment.
Requesting changes, formalising the finding I left in my earlier comment on this PR.
The document's central guarantee is false as written. §Visibility model states "a sibling sees pointers and enumerations, never prose" and calls artifacts "a string→string map of pointers", but the shipped store validates only type and size: work_ledger.py:962-985 accepts MAX_ARTIFACT_KEYS=16 keys of MAX_ARTIFACT_VALUE_CHARS=512 each — about 8 KB of worker-chosen text, with nothing constraining a value to a pointer shape. Phase 3b puts artifacts in the sibling digest, so worker-authored prose crosses exactly the boundary that excluding summary and gating the Phase 5 channels exist to close.
Two consequences that also need correcting:
- The threat row "A worker reads a sibling's prose and is steered by it … Prevented by the mask … no free-text field" is wrong for the same reason.
- The stated defence — "adds no attack surface that
artifactsdid not already carry within one item" — misstates the reader set. Within one item the readers are the conductor and that item's own worker; Phase 3b widens that to siblings, which is the whole change.
Phase 3b's exit criteria pin key presence, not value shape, so nothing catches this at implementation time either.
Either fix closes it, and the choice is a design decision rather than an editorial one: constrain artifacts values to a pointer shape (URL / path / id) and say so in Phase 3b's exit criteria, or drop the "never prose" guarantee, treat artifacts as prose the mask deliberately lets through, and rewrite the threat row to match. Design Review raised the same thing as 🟡 CONCERNS on head 669410cd and it still has no disposition.
Separately, a nit worth fixing in the same pass: the body says "re-audited at 7fa305f35" and "Every code claim added in this revision was read at 7fa305f35", but the diff writes audited-at: c02cdd67c and the doc text says c02cdd67c. 7fa305f35 is a real commit but not the audit SHA the artifact claims.
For the record, everything else in the as-implemented rewrite checks out against origin/main: read_work_brief's seven keys, accept_batch composing from acceptance alone, the exact grant sets in _LEDGER_CONDUCTOR_WORK_GRANTS and _WORKER_WORK_GRANTS, _narrow_conductor_mcp_servers(work=…), kirocrew-dashboard being opt_in, probes/__init__.py mapping only gh-pr, _coalesce_progress and MAX_RECORD_BYTES, and goal-conductor/SKILL.md plus ledger_entry.py being untouched. Phases 3/3b/4/5 describe behaviour that does not exist yet but are labelled "not started", which is correct RFC practice. No leaked internal references.
Problem / Motivation
docs/request-for-change/rfc-conductor-work-ledger.mdis at v2.1 and says two things that are no longer true.It says
status: draftand "Nothing in this document exists on main", but three phases have merged: the store (#8855), the four tools and their routes (#9152), and the isolation into a separatekirocrew-ledger-conductorspec (#9277). A reader acts on that status field, and the index row still says "Nothing".It also describes a two-party record — one conductor, one worker, one item each, and a worker that can see nothing but its own row. The store that shipped is not that: every item of one conductor lives in one directory, which is a shared board. Because the document never said so, it also never wrote down who may read which field, and the one real need that follows from a board — item B building on item A's output — had no answer except the conductor reading A's summary and retyping it at B.
Why it matters
Two costs, one for readers and one for the next phase.
A
draftRFC whose first three phases are on main sends every future reader to the wrong place: they read a proposal and go looking for code that is already there, or they trust "Nothing" and duplicate it.docs/request-for-change/README.mdstates that statuses are verified against the code, so a stale row is a broken promise of that file specifically.And Phase 3 onward now has to be designed against a visibility model nobody wrote.
work_briefreturns seven keys today; whether an eighth may carry a sibling'ssummary, or a session key, or the conductor's goal is currently a judgement call per PR rather than a rule. The same gap makes worker-to-worker help unimplementable: the only sanctioned answer is "route it through the conductor's prose", which is the interpretation cost this RFC exists to remove.What changed (motivation → approach → change)
Goal: make the document describe the shipped board, and give the remaining phases a rule to design against.
Approach: re-characterise rather than re-scope. The ledger is one board per conductor, masked by identity — the conductor sees every square, a worker sees its own square whole and its siblings only as pointers. Everything else in v3 falls out of that one sentence, so no new mechanism is introduced that the mask does not already justify.
What the diff does:
§Visibility modeltabulating every field against four readers (conductor / this item's worker / a sibling worker / Crew page user), plus the three rules that generate the table: a sibling sees pointers and enumerations but never prose, no reader but the conductor sees a session key, and a worker never learns the goal.work_briefgainssiblings:item_id,title,status,artifacts,last_report_at, open items only. Nosummary, noworker_session_key, noacceptance, noverdict, nodecision. The concrete use is item B rebasing on item A'sartifacts.pr. Read-only, pointers and enums only, so it adds no surfaceartifactsdid not already carry inside one item.work_request(kind: relay | channel,targeta siblingitem_id,text≤ 500) appends arequestevent to the requester's own square and wakes the conductor; the target is a field, not a destination. The conductor answers either by relaying withsession_send— zero new code, one approval click per message — or by writing a channel withwork_ledger_record action=channel(ttl_secs≤ 86400,max_messages≤ 50), after whichwork_messagelets the pair exchange bodies directly. A message lands as an event on the target's item and is read as data throughwork_brief's newinbox. Every request and every message is an event on the board the conductor already reads once a cycle, which is the point: the manager does not have to be in the conversation to know what the conversation was.request,channel_open,channel_close,message), a derivedunread_inbox, and achannels/directory whose pair id is two server-minteditem_ids sorted and joined — so no model-supplied string reaches a path component, which is what lets these two tools take anitem_idargument at all.session_send. The earlier text said it is "withheld from workers", which reads as an allowlist decision that could be reversed by adding a line. It is structural:kirocrew-workerisbuild_agent_config()plus@kirocrew-work, andsession_sendlives on theopt_inkirocrew-dashboardserver a worker spec never mounts. A worker's only tools that reach a human aresend_message/send_notification, which address a person, not a session's turn queue. The ledger is a worker's only channel to its conductor.work_message-vs-session_senddistinction as categorical rather than a matter of degree:session_sendreachesenqueue_or_run_prompt, so its body runs as the target's user-role turn under the target's grants — lateral prompt injection.work_messagewrites a JSON field, and Phase 5's exit criteria patchenqueue_or_run_promptto fail the test if it is called at all, because "we did not wire delivery" is exactly the property a later convenience patch would quietly add._coalesce_progress;MAX_RECORD_BYTESreading an over-large file as absent) and the follow-up that the cross-platform locking argument turned out to need (fix(work-ledger): stop the lock-file open from truncating on Windows #9237). A new Phase 2.5 section records the isolation walk-back and its met exit criteria, and§Agent spec changesis corrected to the grants actually on main — three refs onkirocrew-ledger-conductorincludingwork_brief, and no work-ledger grant onkirocrew-conductororkirocrew-pipeline-conductor.progressreports coalesce, as shipped. Q7, Q8, Q9 opened for siblingsummaryvisibility (currently no), a conductor as one end of a channel (currently no), and cross-level requests in a two-level conductor (currently no).revision: v3,status: partial,implementation-prs: [8855, 9152, 9277], re-audited at7fa305f35. The index row moves fromdraft/"Nothing" topartialwith what is and is not on main.One thing is deliberately not claimed: Phase 3 is not a precondition for use.
kirocrew-ledger-conductoras shipped already runs a goal on a plain timer, paying one turn per quiet tick. Phase 3 is a cost optimisation on that loop, and half of the merge criterion for folding the two conductors back together — not the thing that makes the ledger usable.Tests
N/A — documentation only. No code changes, so nothing to pin with a test. The design's own testable claims live in each phase's exit criteria, and Phase 5's are written as assertions rather than intentions (the
enqueue_or_run_promptabsence, the byte-identical target after arequest, the distinctchannel_expired/channel_exhaustedcodes).Manual verification
All four diff-scoped gates run with their
BASE_REFexported, since running them bare reverts to full-tree report mode and always exits 0:sh scripts/docs-lint.sh→ All documentation checks passed (303 files). The 14 dead-identifier and 1 stale-baseline findings are report-only and pre-existing in unrelated files.BRAND_BASE_REF=$(git merge-base HEAD origin/main) python3 scripts/check_brand_name.py→ clean.CHANGELOG_BASE_REF=... python3 scripts/check_changelog_history.py→ 8 shipped sections intact.FOCUS_BASE_REF=... python3 scripts/check_focus_cue.py→ clean for this diff (4 pre-existing frontend elements reported, not gated).Every code claim added in this revision was read at
7fa305f35, not carried over from the previous revision:work_ledger.py's vocabularies and caps,read_work_brief's seven keys,accept_batch's composition fromacceptancealone,mcp_work.py's four tool definitions, the_reaches_a_channelre-check after the thread hop inhandlers/work_ledger.py,_MANAGED_MCP_SERVERS' twoopt_inentries,_LEDGER_CONDUCTOR_WORK_GRANTS/_WORKER_WORK_GRANTS,_narrow_conductor_mcp_servers'workparameter, and_WORKER_SYSTEM_PROMPT.Related Issues
Follows #8842 (v1), #8920 (v2). Documents the design behind #8855, #9152 and #9277.