feat: batch-parallel per-unit waves with parallel foreground reviewers (2.5.67) - #617
Conversation
|
Reviewed at head 951fda8. The mechanism is sound and the engine facts the PR leans on all check out; the findings below are prose-shaped and none require engine changes, but 1 and 2 should block as written. Verified before the findings:
1. P1: carve non-autonomous code-generation out of wave eligibilityThe wave paragraphs apply to any
Fix: restrict the wave prose to the four inline design stages (or exclude 2. P1: sibling-unit resolution is unspecified; mixed-kind batches get wrong artifact setsThe engine resolves paths, kind-filtered produces, and the consumes present/absent split only for
This is closable in prose: 3. P2: the crash-recovery sentence overclaims; covered-but-unreviewed units reach the gate"A session that crashes mid-wave recovers via the same disk scan" is true for builds but not for reviews. Coverage is artifact-existence only, the gate flips to To be fair, the hole exists on the serial path today too: a crash between a unit's artifact write and its review skips that review identically. But a wave widens the exposure from one unit to a whole batch, and the recovery sentence claims a correctness it does not have. Cheap prose fix: the verdict is durable evidence (the 12a step 2 reviewer appends 4. P2: a builder that finishes its artifacts and returns a human-blocking question strands the questionIf a wave builder writes all of its produces AND returns a blocking question, the unit is covered, the engine never re-hands it, and the answer can no longer shape the artifact. The prose should require a blocked builder to withhold at least one required artifact (so the unit stays uncovered), or require the conductor to re-run that unit's body after the human answers. 5. P3: learnings and Q&A fidelityThe section 13 ritual surfaces the stage-level 6. Process
None of this questions the design itself: MAY-not-MUST, the unit-major exclusion, the reviewer-serialization split by harness capability, and the wave-as-loop-parallelization carve-out in section 5 are all consistent with the engine as shipped. Findings 1-4 are each a sentence or two of prose plus matching t244 pins. |
…aware sibling resolution (awslabs#617 review) Addresses review findings on PR awslabs#617 (a field engagement's request for batch-parallel per-unit waves). Both P1s were correctness bugs in the wave prose, not the engine: - P1: the wave paragraph applied to any gate:false per-unit directive, including non-autonomous code-generation. That's wrong on two counts - code-generation carries workspace_requires:true and writes into the shared workspace, so concurrent wave builders would collide in the working tree (the swarm path uses per-Unit worktrees for exactly this isolation; the wave floor has none); and code-generation's Step 3 Plan Approval is a mandatory hard stop in every execution mode, which contradicts the wave rule that blocking questions ride in the builder's return message instead of stopping. Wave eligibility is now restricted to the four inline per-unit design stages (functional-design, nfr-requirements, nfr-design, infrastructure-design); any workspace_requires:true stage is never wave-eligible. - P1: sibling-unit resolution for a wave was unspecified for mixed-kind batches. The wave prose said to take "units whose artifacts for THIS stage are not yet on disk" but never said how to derive a sibling's produce set - the engine's actual coverage predicate (unitCovered) filters required produces by the unit's kind via produces_kinds, so a kind exempt from every one of a stage's produces is vacuously covered and must never be a wave member. Naive name substitution into the one resolved directive is also wrong across kinds. The wave prose now specifies: cross- reference each candidate unit's bolt_dag.units[].kind against the stage's produces_kinds map to decide membership, and resolve each included sibling's own paths/produces/consumes from its own kind. - P2: the crash-recovery sentence overclaimed - disk-scan coverage is artifact-existence only, so a wave that crashes between builder completion and the reviewer pass could reach the gate with a covered-but-unreviewed unit. On crash re-entry the conductor now runs the §12a reviewer step for any wave unit that reads as covered but carries no `## Review` verdict yet, before presenting the gate. Full deterministic closure of this class of gap is awslabs#569's territory; this is the wave-specific instance of that same contract. - P2: a builder that writes all its produces AND returns a blocking question would strand the question (the unit reads covered and the engine never re-hands it). Fixed via the smaller of the two options: a blocked builder must withhold at least one required produce so its unit still reads as uncovered; the conductor re-hands the unit after the human answers, consistent with how the serial loop already treats an incomplete unit. - P3: the consolidated per-wave diary entry and any per-unit Q&A now explicitly carry the unit-level content verbatim, not a paraphrase. Applied to every surface that carries the wave paragraph: the shared core/aidlc-common/protocols/stage-protocol.md §3 contract, all five authored harness/*/skills/aidlc/SKILL.md files, and docs/reference/03-orchestrator.md, 04-stages/construction.md, and 13-runtime-graph.md. dist/ regenerated from the authored sources (no hand-edits). tests/unit/t249-per-unit-wave.test.ts (renamed from a rebase-time t244 collision) pins every load-bearing sentence above on every carrying surface, plus two grounding tests against real stage frontmatter: code-generation actually carries workspace_requires:true while the four inline design stages don't, and infrastructure-design's produces_kinds map actually vacuously exempts a spec-kind unit from every required produce while a service-kind unit stays fully required (apackeer's own example, grounded rather than only asserted in prose). No engine, hook, or state-format changes - conductor-prose only.
951fda8 to
21ef329
Compare
|
Thanks for the thorough review — rebased onto current 6 (process). Rebased onto 1 (P1, blocking) — carved non-autonomous code-generation out of wave eligibility. Confirmed: 2 (P1, blocking) — kind-aware sibling resolution. Added the missing specification: wave membership is now derived by cross-referencing each candidate unit's 3 (P2) — crash-recovery overclaim. Fixed: the recovery sentence now states the disk scan is artifact-existence only and proves nothing about review, and requires that on crash re-entry the conductor run the §12a reviewer step for any wave unit that reads as covered but whose primary artifact carries no 4 (P2) — stranded blocking question. Went with the withhold-artifact fix (the smaller change, and consistent with how the engine's coverage predicate already works): a wave builder with a human-blocking question MUST withhold at least one of the stage's required produces so its unit still reads as uncovered on the engine's disk scan, and — symmetrically — once a builder has written every required produce it MUST NOT raise a blocking question, since the engine would then read the unit as done and never re-hand it. The conductor re-hands an uncovered unit after the human answers, the same way the serial per-unit loop already treats an incomplete unit. Applied everywhere the wave paragraph lives; pinned in 5 (P3) — learnings/Q&A fidelity. Added the one-sentence fix: the conductor's consolidated per-wave diary entry now explicitly carries every unit's memory-note content verbatim, not a paraphrase, and the §13 learnings ritual (or a reviewer needing per-unit Q&A) may read the unit files directly wherever a verbatim source matters. Verification:
Force-pushed as |
|
Re-reviewed at head Verified before the findings:
New finding 1 (P1, blocking): the wave reads raw The wave prose (stage-protocol.md section 3 and each SKILL.md) instructs the conductor to "read This is not hypothetical; t215 test 6b ("a valid but outdated cached bolt_dag heals from the authored artifact") constructs exactly this state: the cached graph says
This also grazes the round-1 kind-membership fix: the sibling kinds are read from the same possibly-stale snapshot, so a kind edit that healed in memory is invisible to the wave. Suggested fix (prose-only, keeps the floor framing): make the serial fallback fire on staleness, not just absence. E.g.: before forming a wave, cross-check the snapshot against the directive: if New finding 2 (P2): the withhold-a-produce rule is kind-blind; on a mixed-kind unit the withheld artifact can be one the engine never checks. The new rule says a blocked builder "MUST withhold at least one of the stage's required produces so its unit still reads as uncovered". But coverage only checks produces applicable to the unit's kind: One-phrase fix on every surface (and the section 3 paragraph): "withhold at least one of the stage's required produces that applies to its unit's kind". Since t249 already pins the withhold sentence by regex, the pin needs the same update; and the existing grounding test for (g) could gain one assertion that the withheld-produce example uses a kind-applicable artifact. Smaller notes (non-blocking):
With finding 1's serial-on-mismatch rule and finding 2's one-phrase kind qualifier (plus the matching t249 pin updates), this is ready. |
|
Thanks, Jeff. Your @leandrodamascena, could you check how this should integrate with current
The core constraints still look sound: optional waves, the four inline design stages only, default stage-major execution, serialized reviewers where enforcement requires it, and no gate with reviews outstanding. If changes are needed, maintainers should handle the current- |
leandrodamascena
left a comment
There was a problem hiding this comment.
Reviewed against current v2. The wave direction is sound, but integration needs engine support before merge.
P1: Engine must emit wave membership
core/aidlc-common/protocols/stage-protocol.md:411 reads raw runtime-graph.json, while resolveBoltDag() may heal a stale DAG only in memory. This can leave directive.unit absent from the cached batches or concurrently dispatch units the authored DAG now declares dependent. Wave membership and complete per-unit directives should come from the engine’s healed snapshot.
P1: Reviews must settle before dependent batches
Crash recovery checks for ## Review, but completion requires a fresh REVIEW_COMPLETED receipt, and later artifact writes invalidate that receipt. The engine should keep the current DAG batch active until every applicable unit has a fresh terminal review receipt; otherwise a dependent batch can be built against design that the delayed review subsequently changes.
P1: Preserve current steering and context contracts
Every wave builder must receive the accumulated load-steering.rules_content bundle verbatim. Inline support perspectives and context_warnings must also remain effective. The current wave prose predates and does not preserve these current-v2 contracts.
P2: Resolve unit-local memory paths
Current directives expose the shared stage-level memory_path, while wave builders are prohibited from writing that diary. Engine-emitted wave entries need an explicit unit-local memory path; the parent directive should retain the stage diary consumed by the learnings tool.
P2: Make blocked-builder withholding kind-aware
Require withholding at least one required produce applicable to the unit’s kind. Withholding an inapplicable artifact still lets the engine mark the unit covered and strands the blocking question.
Recommended integration shape: add an optional engine-emitted wave containing complete per-unit entries derived from one healed BoltDagResolution, including resolved unit, consumes, consumes_absent, produces, unit memory path, and review state. Keep steering, context, and stage-level memory on the parent directive.
Verification passed locally: package parity, typecheck, lint, coverage registry, t249, t215, t208, and t205.
21ef329 to
ef2c4cd
Compare
…aware sibling resolution (awslabs#617 review) Addresses review findings on PR awslabs#617 (a field engagement's request for batch-parallel per-unit waves). Both P1s were correctness bugs in the wave prose, not the engine: - P1: the wave paragraph applied to any gate:false per-unit directive, including non-autonomous code-generation. That's wrong on two counts - code-generation carries workspace_requires:true and writes into the shared workspace, so concurrent wave builders would collide in the working tree (the swarm path uses per-Unit worktrees for exactly this isolation; the wave floor has none); and code-generation's Step 3 Plan Approval is a mandatory hard stop in every execution mode, which contradicts the wave rule that blocking questions ride in the builder's return message instead of stopping. Wave eligibility is now restricted to the four inline per-unit design stages (functional-design, nfr-requirements, nfr-design, infrastructure-design); any workspace_requires:true stage is never wave-eligible. - P1: sibling-unit resolution for a wave was unspecified for mixed-kind batches. The wave prose said to take "units whose artifacts for THIS stage are not yet on disk" but never said how to derive a sibling's produce set - the engine's actual coverage predicate (unitCovered) filters required produces by the unit's kind via produces_kinds, so a kind exempt from every one of a stage's produces is vacuously covered and must never be a wave member. Naive name substitution into the one resolved directive is also wrong across kinds. The wave prose now specifies: cross- reference each candidate unit's bolt_dag.units[].kind against the stage's produces_kinds map to decide membership, and resolve each included sibling's own paths/produces/consumes from its own kind. - P2: the crash-recovery sentence overclaimed - disk-scan coverage is artifact-existence only, so a wave that crashes between builder completion and the reviewer pass could reach the gate with a covered-but-unreviewed unit. On crash re-entry the conductor now runs the §12a reviewer step for any wave unit that reads as covered but carries no `## Review` verdict yet, before presenting the gate. Full deterministic closure of this class of gap is awslabs#569's territory; this is the wave-specific instance of that same contract. - P2: a builder that writes all its produces AND returns a blocking question would strand the question (the unit reads covered and the engine never re-hands it). Fixed via the smaller of the two options: a blocked builder must withhold at least one required produce so its unit still reads as uncovered; the conductor re-hands the unit after the human answers, consistent with how the serial loop already treats an incomplete unit. - P3: the consolidated per-wave diary entry and any per-unit Q&A now explicitly carry the unit-level content verbatim, not a paraphrase. Applied to every surface that carries the wave paragraph: the shared core/aidlc-common/protocols/stage-protocol.md §3 contract, all five authored harness/*/skills/aidlc/SKILL.md files, and docs/reference/03-orchestrator.md, 04-stages/construction.md, and 13-runtime-graph.md. dist/ regenerated from the authored sources (no hand-edits). tests/unit/t249-per-unit-wave.test.ts (renamed from a rebase-time t244 collision) pins every load-bearing sentence above on every carrying surface, plus two grounding tests against real stage frontmatter: code-generation actually carries workspace_requires:true while the four inline design stages don't, and infrastructure-design's produces_kinds map actually vacuously exempts a spec-kind unit from every required produce while a service-kind unit stays fully required (apackeer's own example, grounded rather than only asserted in prose). No engine, hook, or state-format changes - conductor-prose only.
|
Addressed the current- Review findings
Verification
I also attempted the Claude SDK live integration slice. This sandbox blocks writes to |
|
Hey @jstrunk pls check the last review. |
|
@jstrunk Now Leandro has approved, i will rebase and merge. Thank you again! |
…s (2.5.15) Implements the floor version of awslabs#610: on the default stage-major Construction walk, the conductor MAY process a gate:false per-unit design directive as a Bolt-DAG batch wave - one concurrent stage-body dispatch per uncovered batch unit, each running the stage's lead persona for that ONE unit, confined to construction/<unit>/<stage>/ (no stage-level diary; the conductor appends one consolidated entry per wave; human-blocking questions surface in the builder's return message). Reviewers run per wave unit at wave end - parallel FOREGROUND dispatches only where no reviewer-scope dispatch record is active; on enforcement harnesses (Claude Code, Kiro CLI, Codex, opencode) the single-file record serializes per-unit reviews. Every NOT-READY revision loop resolves within its iteration budget before the single stage gate - the wait is deferred, not skipped. Waves never apply under Construction Iteration: unit-major, and harnesses without a parallel dispatch primitive stay on the serial loop (MAY, not MUST). Conductor-prose only - no engine, hook, or dispatch-record changes: per-unit coverage is a stateless disk scan (nextUncoveredUnit), batch units are dependency-free with disjoint artifact dirs, and a crashed session mid-wave recovers via the same scan. - Wave paragraph in all five harness/*/skills/aidlc/SKILL.md, worded per each harness's native dispatch (Task / subagent crew / codex exec workers / task tool) - stage-protocol.md: section 3 harness-neutral wave paragraph + section 5 per-unit-wave carve-out (a wave parallelizes the engine's per-unit loop, not a communication topology; mode: inline's "supports are voices" folds into each builder's brief) - Docs: 03-orchestrator (per-unit wave in Construction Execution), 04-stages/construction.md (wave note), 13-runtime-graph.md (the conductor as a bolt_dag.batches consumer) - New prose pin tests/unit/t249-per-unit-wave.test.ts (HARNESS_MATRIX style; pins MAY-not-MUST, unit-major exclusion, write confinement, never-present-gate-with-outstanding-reviewer, and the enforcement serialization constraint on authored + dist surfaces) - Version 2.5.15, CHANGELOG entry, README badge; dist regenerated
…aware sibling resolution (awslabs#617 review) Addresses review findings on PR awslabs#617 (a field engagement's request for batch-parallel per-unit waves). Both P1s were correctness bugs in the wave prose, not the engine: - P1: the wave paragraph applied to any gate:false per-unit directive, including non-autonomous code-generation. That's wrong on two counts - code-generation carries workspace_requires:true and writes into the shared workspace, so concurrent wave builders would collide in the working tree (the swarm path uses per-Unit worktrees for exactly this isolation; the wave floor has none); and code-generation's Step 3 Plan Approval is a mandatory hard stop in every execution mode, which contradicts the wave rule that blocking questions ride in the builder's return message instead of stopping. Wave eligibility is now restricted to the four inline per-unit design stages (functional-design, nfr-requirements, nfr-design, infrastructure-design); any workspace_requires:true stage is never wave-eligible. - P1: sibling-unit resolution for a wave was unspecified for mixed-kind batches. The wave prose said to take "units whose artifacts for THIS stage are not yet on disk" but never said how to derive a sibling's produce set - the engine's actual coverage predicate (unitCovered) filters required produces by the unit's kind via produces_kinds, so a kind exempt from every one of a stage's produces is vacuously covered and must never be a wave member. Naive name substitution into the one resolved directive is also wrong across kinds. The wave prose now specifies: cross- reference each candidate unit's bolt_dag.units[].kind against the stage's produces_kinds map to decide membership, and resolve each included sibling's own paths/produces/consumes from its own kind. - P2: the crash-recovery sentence overclaimed - disk-scan coverage is artifact-existence only, so a wave that crashes between builder completion and the reviewer pass could reach the gate with a covered-but-unreviewed unit. On crash re-entry the conductor now runs the §12a reviewer step for any wave unit that reads as covered but carries no `## Review` verdict yet, before presenting the gate. Full deterministic closure of this class of gap is awslabs#569's territory; this is the wave-specific instance of that same contract. - P2: a builder that writes all its produces AND returns a blocking question would strand the question (the unit reads covered and the engine never re-hands it). Fixed via the smaller of the two options: a blocked builder must withhold at least one required produce so its unit still reads as uncovered; the conductor re-hands the unit after the human answers, consistent with how the serial loop already treats an incomplete unit. - P3: the consolidated per-wave diary entry and any per-unit Q&A now explicitly carry the unit-level content verbatim, not a paraphrase. Applied to every surface that carries the wave paragraph: the shared core/aidlc-common/protocols/stage-protocol.md §3 contract, all five authored harness/*/skills/aidlc/SKILL.md files, and docs/reference/03-orchestrator.md, 04-stages/construction.md, and 13-runtime-graph.md. dist/ regenerated from the authored sources (no hand-edits). tests/unit/t249-per-unit-wave.test.ts (renamed from a rebase-time t244 collision) pins every load-bearing sentence above on every carrying surface, plus two grounding tests against real stage frontmatter: code-generation actually carries workspace_requires:true while the four inline design stages don't, and infrastructure-design's produces_kinds map actually vacuously exempts a spec-kind unit from every required produce while a service-kind unit stays fully required (apackeer's own example, grounded rather than only asserted in prose). No engine, hook, or state-format changes - conductor-prose only.
ef2c4cd to
d38ca2d
Compare
Implements dependency-safe per-Unit design waves for the default stage-major Construction walk.
What
For functional-design, nfr-requirements, nfr-design, and infrastructure-design, the engine may emit
directive.wavefor the first unsettled Bolt-DAG batch. Each entry carries fully resolved Unit-local inputs, outputs, memory, build state, completion state, and paired-review progress. The conductor never reads or reconstructs the cached runtime graph.Current-v2 integration
BoltDagResolution.REVIEW_REQUESTED/REVIEW_COMPLETEDevidence.none, advisory, adversarial repair, unmatched-request retry, and terminal NOT-READY behavior follow the current review-class contract.aidlc-state.ts unit complete --waveverifies the live entry, fans Unit diary entries into the parent diary with deterministic deduplication, then emitsUNIT_COMPLETED.required_producespath. Optional or kind-inapplicable outputs cannot keep the Unit open.Compatibility and release metadata
v2.2.5.67.t278.Verification
bun run checkbun tests/gen-coverage-registry.ts --checkThe stronger future phases from the RFC, including plugin-extensible wave eligibility and background reviewer dispatch, remain out of scope.