fix: composed-workflow determinism + test-suite hardening (2.5.60) - #730
Conversation
0836c02 to
978c76a
Compare
leandrodamascena
left a comment
There was a problem hiding this comment.
Re-review of 978c76a5: CI is green and the focused tests pass, but the approval/routing contracts still have blockers.
-
P1 - a matched proposal can present and approve a different grid from the one that will run (
core/agents/aidlc-composer-agent.md:563,:576-580,:630-640). The composer copiessummaryfrom validation and builds the decision table from the mechanical/folded proposal, then may replace that proposal with the stock grid without revalidating or regenerating either surface. The new t275 scenario demonstrates the mismatch:bugfixplus two stages validates as 9 EXECUTE, but the <=2 match rule adopts stockbugfix, which has 7. The human can therefore approve counts/rows that are not the workflow subsequently started. The same contract also says mode is FINAL while still allowing edits; edits to a matched plan have no persistence path because matched approval writes no scope. Revalidate and rebuild summary/table after stock adoption, and convert edited stock plans to custom (or disallow edits that cannot be applied). -
P1 - incomplete grids are accepted as exact stock matches (
core/tools/aidlc-graph.ts:1019-1023,:1121-1133). Validation checks only supplied keys, and distance ignores missing proposal entries. I reproducedvalidate-gridwith{}: it returnsvalid: true, a zero-stage summary, and every stock scope atdiff: 0, withbugfixfirst alphabetically. Since the input is LLM-produced and the new number is the deterministic routing authority, require exactly the compiled stage set (or count missing stages as differences) before computing a match. -
P1 - the new mid-flow ask has contradictory response protocols (
harness/claude/skills/aidlc/SKILL.md:80,130; mirrored in all six harnesses). The centralaskdispatch contract andAskDirectivetype say the human answer goes back throughreport, while the new branch-specific instruction says route it directly and never usereport. This ask has no report result that can represent its three choices, so following the central table fails the flow. Give this question a typed response route/ask subtype and one authoritative continuation contract. -
P1 - rich new-work prose produces an offer without the scope required by its confirmation path (
core/tools/aidlc-orchestrate.ts:2965-2972).inferScopeFromText()returnssource: freeformfor no match or >5 words, and the question omits the inferred scope in that case. The harness then requiresnext --new-intent --scope <the scope the ask named>, but none was named. The ACP test explicitly calls the inferred scope non-deterministic. Always include the proposed scope (or an explicit compose route) in the directive rather than leaving the conductor to invent it. -
P2 - t163 can pass when a contender steals the live holder (
tests/integration/t163-reaper-steal-race.test.ts:108-119,:243-265). The live-holder case never createsreap-evidence.json; onlyseedStaleLock()does. If a broken implementation lets a contender acquire, that child throws while reading the missing file before printingWON. Child stderr/status are ignored,winnerEvidence()reports zero winners, and the replacement lock still makesexistsSync(lockDir)true, so both assertions pass. Seed the evidence state and fail on every nonzero/malformed child result. -
P2 - t238 no longer proves the conductor read the lead persona (
tests/integration/t238-user-stories-mob.sdk.test.ts:494-506).inline_context_pathsis only a path manifest emitted bybuildRunStageDirective; the skill explicitly requires the conductor to read those paths. Accepting manifest presence as an alternative to a Read makes the assertion pass even when the conductor ignores the persona. Keep behavioral evidence that the file content was actually loaded. -
P2 - t72 accepts inconsistent stage advancement (
tests/integration/t72-stage-reverse-engineering.test.ts:234-257). On an overshoot it permits RE to remain[-]or[?]and acceptsSTAGE_AWAITING_APPROVALas sufficient evidence that Current Stage moved later. Advancement should require RE[x]plusSTAGE_COMPLETED; otherwise a broken state transition passes as a timing variation. -
P2 - the ACP journey no longer proves the new-work offer was rendered (
tests/e2e/t-acp-kiro-journey-workspace.serial.test.ts:346-384). Turn 3a now asserts only no birth/no state mutation. Turn 3b itself explicitly says to start a second intent, so it can create one even if turn 3a rendered no question. Capture/assert the routing question or another observable offer surface before sending confirmation.
Verification on this head: 49 focused t114/t275 tests passed, both t163 tests passed, package parity, typecheck, lint, and git diff --check passed. The empty-grid reproduction above was run directly against the shipped graph tool.
|
@leandrodamascena Thanks for the detailed review. I pushed
Package parity, typecheck, lint, and diff checks are green. Please re-review when you have a chance. |
leandrodamascena
left a comment
There was a problem hiding this comment.
Re-review of 0f025c69: the previous eight findings have corresponding fixes, and focused checks pass. Three routing issues remain.
1. P1: stock adoption makes valid in-flight recomposition impossible
core/agents/aidlc-composer-agent.md:570-588 applies stock matching and full-grid adoption to all three composition moments, including in-flight. This conflicts with the in-flight contract at lines 57-65 and core/tools/aidlc-orchestrate.ts:840-847, where only pending, ahead-of-cursor suffix flips may be applied through recompose.
A deterministic example is a running feature workflow where the user asks to skip the still-pending market-research and team-formation stages. Strict validation succeeds with 30 EXECUTE / 2 SKIP and reports enterprise at distance 2, differing on exactly those stages. Step 7 then requires adopting the full stock grid, restoring both stages and producing a 32-stage proposal. The intended pending flips disappear. Approval then either invokes recompose without any flips, which fails its mandatory-argument guard, or attempts differences involving frozen stages, which recompose rejects.
The identical feature and enterprise grids also allow an in-flight feature workflow to be presented as a matched enterprise proposal even though recompose cannot change the workflow scope or its depth metadata.
Restrict stock matching/adoption to front/report composition. In-flight composition should preserve every frozen action and return only validated --skip/--add changes against the current effective plan. Add a deterministic test covering the existing t196 “drop two pending stages” scenario through the new match logic.
2. P2: composed scopes participate in the “stock” ranking
core/tools/aidlc-graph.ts:1015-1019 implements nearestStockScopes() by iterating every entry returned by loadScopeGrid().
That grid does not contain only stock scopes. mergeComposedScopes() explicitly preserves custom entries written by the composer (core/tools/aidlc-graph.ts:1411-1418), and Step 10 writes every approved custom scope into the same registry.
After the first custom scope is created, subsequent ARS and validation calls can rank it at diff: 0 or within the two-flip boundary. Step 7 will then treat that user-owned scope as prevalidated stock, set mode: matched, reuse its possibly task-specific name and depth, and skip the custom persistence path. Matching results therefore depend on which unrelated custom workflows happen to exist in the installation.
Build the candidate set from graph-authored/plugin-authored scopes, for example the fresh transpose before mergeComposedScopes(), or persist explicit scope provenance. Add a test that injects a composed grid entry and confirms it is excluded from nearest_stock.
3. P2: the final match rule has contradictory distance authorities
Several contracts say matching is based on the validator’s distance from the final proposal: core/agents/aidlc-composer-agent.md:566-568, core/knowledge/aidlc-composer-agent/composing.md:50-55, core/tools/aidlc-orchestrate.ts:865-866, and tests/unit/t275-validate-grid-nearest-stock.test.ts:9-12.
However, Step 7 at core/agents/aidlc-composer-agent.md:572-598 uses the smaller of the mechanical ARS screen’s stock distance before evidence-driven folds and the validator’s distance from the final proposal.
If the mechanical screen is two flips from stock but evidence-based folding moves the final proposal much farther away, Step 7 still forces the stock scope and discards every fold. There is no limit on how many final decisions can be erased; only the earlier mechanical distance is bounded.
This contradicts the knowledge contract’s “when no stock scope fits, synthesize; do not force a bad match” rule and leaves the LLM with two authoritative routing instructions. Depending on which instruction it follows, the same validated proposal can become either matched or custom.
Use validate-grid.nearest_stock from the final proposal as the sole routing authority. Keep the mechanical distance as advisory evidence if desired. If using the minimum is intentional, update the conductor, knowledge, and tests to state that contract explicitly and add a divergence test where the mechanical grid is near stock but the final folded proposal is not.
Verification
- Focused unit suite:
156 passed - Recompose suite:
17 passed - Reaper concurrency suite:
2 passed bun scripts/package.ts --check: passedbun run typecheck: passedbun run lint: exited successfully with three informational noticesgit diff --check: passedt72-stage-reverse-engineering: exceeded the five-minute local verification window; no pass/fail conclusion
0f025c6 to
5df77e3
Compare
|
@leandrodamascena Thanks for the follow-up review. I pushed
Added deterministic coverage for the t196 two-pending-stage reshape, mechanical/final distance divergence, composed-scope exclusion, emitted in-flight directives, and all six conductor skills. The branch is rebased onto current Verification: focused unit tests passed at Could you please re-review when you have a chance? |
leandrodamascena
left a comment
There was a problem hiding this comment.
Re-review of 5df77e38: the three previously reported routing issues are resolved. I found no additional blocking code findings.
Verification passed: 62 focused tests, typecheck, package parity, git diff --check, and remote CI.
Approved. The current merge conflict with v2 still needs resolution before merge.
5df77e3 to
8553a8d
Compare
Answers two independent reviews of PR awslabs#731. Grouped by the invariant each class violated rather than patched per finding, because a per-case fix in this file has repeatedly closed the named site and missed its sibling. CONTAINMENT. A committed symlink at `documentkb/.journal` let a plain `sync` recursively delete an external directory while printing "Up to date." and exiting 0; a symlinked `documentkb/<id>` let sync write outside the project; and the `aidlc/active-space` cursor was read unvalidated, so `..` escaped the `spaces/` jail. Containment is now re-checked per path component through one funnel, enforced by two independent layers: a biome `noRestrictedImports` override that makes a raw mutating `node:fs` import in this tool impossible, and a TypeScript-AST completeness property that treats an unrecognised fs binding as a mutator until a human classifies it. Mutations route through `ensureDirSync`/`renameIntoPlace`/`removeTreeSync`. Four earlier attempts enumerated known-good and failed open one level up (parameter names, then primitive names, then regex-parsed imports); t277's header records the source-code-level routes that remain out of reach of any in-repo check. ROW IDENTITY. Re-onboarding an edited file created two live rows for one path; now the live row at that path IS the identity, refreshed in place and reported as `edited`. Two identical-byte rows competing for one replacement had the winner decided by array order; digests are now resolved globally and the ambiguous case fails closed, requiring `rebind`. `content` and `summary.path` are bound canonically to the row's own id, so a spliced catalog can no longer cite one document while serving another's text. `removed_at` must be a non-empty ISO string, so a malformed tombstone cannot read as a live row. `onboard --intent` on an unchanged document applied the association it previously dropped. PUBLISHED CLAIMS. `rm -rf documentkb/` then `sync` was documented as a recovery; measured, a full wipe loses ids, tombstones and intent links. The claim is narrowed to index-only recovery in four places and pinned by a pair of tests -- one proving what survives an index loss, one proving what does not survive a tree wipe. `resolveIntentFlag` now accepts a record-dir name and a canonical UUID, so the remedy its own error message suggests works. The usage line lists all seven verbs. The audit-shard scoping gap is documented rather than fixed: document events land in the space shard, but an unscoped `readAllAuditShards` resolves through the active intent, so `--doctor --export` omits them once an intent exists. TEST HYGIENE. `t275` becomes `t286` (PR awslabs#730 ships a different t275). `t279` no longer mutates the shared checkout -- it works on a scratch copy, so a killed run orphans a temp dir instead of dirtying `dist/`. The t28/t81 audit-count prose now derives 85; both pins were already correct.
Summary
Test-suite hardening driven by two full 8-slice live gates on clean v2: every persistent red and flake observed was root-caused and fixed - four are product defects the gates exposed, the rest are test pins that raced live conductors or the box. Both gates are green on this branch's lineage, including the first fully-green live Claude TUI slice (19/0) on record.
Product fixes (each grounded in live trace evidence)
aidlc-graph.ts validate-gridreturnsnearest_stock(stock scopes ranked by grid distance from the proposal, reusing thearssubcommand's diff loop). The composer routes matched-vs-custom onnearest_stock[0].diff <= 2, a matched proposal adopts the stock grid verbatim, and conductor prose in all 5 harnesses forbids re-deriving the verdict. Before: the conductor could compare grids itself post-approval, decide "custom", and write a scope file +scope-grid.jsonentry the human never approved (observed 3/3 on clean v2).bugfix/security-patchscopes.--singlepre-generation summary stop now applies only when the stage body actually ran a file-backed Q&A; a first-scan reverse-engineering proceeds straight to artifacts. Before: non-interactive runs (Kiro ACP,codex exec) hung forever at a manufactured question (journey-workspace twins red on clean v2)./aidlcused to be silently discarded by the engine's happy path; conductors that skipped their own classification poured new-work descriptions into the active intent (observed: stage diary pollution, fabricated gate rejections, park attempts, hand state edits). The engine now answers with anasknaming the active work, the typed text, and three routes (continue / separate new work with its inferred scope / reshape). Flagged and verb forms are unaffected.Test-suite hardening
tests/logs/<stamp>-p<pid>, non-recursive mkdir): two same-second runners previously shared one dir and mutually deleted each other's_results(a concurrent sliced gate crashed ENOENT mid-run). Claude-gate discovery also tolerates transient sibling-planted test files vanishing betweenreaddirand read.reapStaleLockis out of scope; follow-up candidate.)inline_context_pathsdelivery or an explicit Read; the supports-not-read half of the mob contract is unchanged.DECISION_RECORDEDoptions row (verbatim "Nothing to add"/"Add a note") instead of paraphrase-sensitive answer wording; t73's artifact-ordering pin narrowed to generated deliverables (the audit hook legitimately logs the diary and questions file before the checkpoint).nearest_stockoutput shape and the diff<=2 boundary (the t193 scenario).Verification
Two full 8-slice gates (smoke+unit / integration / deterministic e2e / live Claude TUI / live Kiro ACP / live Kiro TUI / live codex exec / Kiro IDE skip-gate), plus per-fix solo and under-load reruns. Final gate: all 8 slices green; previously-red tests re-verified individually (t193 2x consecutive, ACP journey-workspace 4x consecutive, t163 solo x3 + 2-core torture runs, t72/t238/t240 solo + load). Evidence stamps under
tests/logs/are cited in the commit message.bun scripts/package.ts --check,bun run typecheck, and the coverage-registry check are green; CHANGELOG 2.5.60 + README badge bumped per the changelog policy.