Skip to content

fix: case-fold the swarm unit-name membership tests (2.5.12) - #637

Open
jstrunk wants to merge 1 commit into
awslabs:v2from
jstrunk:fix/621-swarm-unit-casing
Open

fix: case-fold the swarm unit-name membership tests (2.5.12)#637
jstrunk wants to merge 1 commit into
awslabs:v2from
jstrunk:fix/621-swarm-unit-casing

Conversation

@jstrunk

@jstrunk jstrunk commented Jul 22, 2026

Copy link
Copy Markdown

Fixes #621.

What

Case-folds the engine's swarm unit-name membership tests so an autonomous Construction run whose unit names are authored in uppercase (e.g. U-16 in unit-of-work-dependency.md) can advance batches, settle the stage, and pass the approval gates. Before this fix, next re-emitted invoke-swarm for the first batch forever after that batch had genuinely converged — the silent hang in #621.

Root cause (recap from the issue)

bolt_dag.batches preserves unit names exactly as authored, while the swarm referee (aidlc-swarm.ts) forces kebab-lower unit slugs — prepare/finalize reject non-kebab — so the SWARM_UNIT_CONVERGED rows carry u-16. converged.has("U-16") against a set holding "u-16" is always false, so every batch read as fully unconverged.

The fix — all four comparison sites, not just the hang

Since the issue was filed (against 2.2.17-era code), the ledger reader moved to aidlc-lib.ts and grew more consumers (including 2.5.5's per-unit review enforcement). Fixing only tryEmitSwarm would have moved the failure downstream: the engine would advance the batches, but the settle report, the approve-time artifact guard, and the per-unit review-receipt check would each then refuse for the same casing reason. So:

  • aidlc-lib.ts swarmConvergedUnits() — the shared reader stores lowercased Unit names (documented in its header comment).
  • aidlc-orchestrate.ts tryEmitSwarm() — folds the authored batch name at the membership test, and still emits the original-cased names in the invoke-swarm directive (the conductor's prepare re-derives the kebab slug from them).
  • aidlc-orchestrate.ts isSettledAutonomousSwarm() — the report-side settle evidence gate folds the DAG name.
  • aidlc-state.ts isSettledSwarmForArtifactGuard() — the approve-time artifact-guard exemption folds the DAG name.
  • aidlc-state.ts per-unit review-receipt check (reviewedUnits) — a swarm's reviews run in kebab-lower-slugged Bolt worktrees, so their REVIEW_COMPLETED rows carry the lower slug too; the set and both membership ops fold (add, artifact-invalidation delete, and the missing-units filter).

Per the issue's suggested fix, this is the comparison-site case-fold — not canonicalizing in computeBoltDag/parseBoltDag, which would change the stored graph shape other readers depend on.

Non-goal

aidlc-swarm.ts prepare --units U-16 still rejects the raw uppercase name loudly (Invalid --slug) and the conductor adapts by deriving the kebab slug — that loud path is unchanged. #621 is specifically the silent hang in the deterministic engine.

Changes

  • core/tools/aidlc-lib.ts, core/tools/aidlc-orchestrate.ts, core/tools/aidlc-state.ts — the case-folds above (comment-documented at each site).
  • tests/unit/t201-swarm-batch-advance.test.ts — new describe "authored-casing vs referee slug (issue Swarm batch never advances: bolt_dag unit-name casing vs forced-kebab SWARM_UNIT_CONVERGED slugs not case-folded #621)", 3 red-first cases: batch 1 converged under lowercase rows advances to batch 2 (["U-BENCH"] original-cased); a partial pass re-fans only the owed units original-cased; a fully-converged uppercase DAG settles and report --approved completes after per-unit reviews are recorded.
  • tests/integration/t185-stage-artifact-guard.test.ts — new settled-swarm exemption case: an uppercase-authored DAG whose rows carry kebab-lower slugs passes approve.
  • Version 2.5.12 + CHANGELOG entry + README badge; all dists regenerated. (Rebased once onto v2 after it advanced to 2.5.11 — the original 2.5.6 number this PR claimed was consumed upstream in the meantime; re-bumped past it.)

Testing

  • Red-first: all 4 new cases fail on the unfixed engine, pass with the fix.
  • Full smoke + unit tiers via tests/run-tests.sh: 178 files, 4399 assertions, 0 failures. Integration tier: pass.
  • bun scripts/package.ts --check, bun tests/gen-coverage-registry.ts --check, bun run typecheck, and bun run lint all clean.
  • bun run check (the exact command the repo's new v2 PR gate CI job runs) and bun scripts/ci-changelog-guard.ts (the new gate's changelog-deletion guard) both pass locally against this branch.

Context

Found and mitigated during a field engagement (a 3-unit parallel batch under autonomous mode hung at code-generation); the fix here extends that mitigation to the consumer sites added since.

Note on the Security Scanners signal

Security Scanners (.github/workflows/security-scanners.yml) triggers only on push/pull_request to main, plus a daily schedule — never on a PR against v2. So this PR cannot inherit that workflow's result regardless of #638's status; confirmed by reading the workflow's on: block rather than by absence of a run. It does fail on main itself today (e.g. run 29914525428): grype flags 17 pre-existing findings in scripts/aidlc-evaluator/uv.lock (pillow 12.2.0 — Dependabot #619 — plus mcp 1.23.3 and pydantic-settings 2.14.1), and gitleaks flags 7 deliberate fake credentials in test fixtures not yet in the .gitleaks.toml allowlist. Tracked in #638, orthogonal to this PR.

The checks that DO apply here are v2's new PR gate (ci.yml, added since this PR opened): contract checks (bun run check), smoke+unit tests, and a changelog-deletion guard. All three verified green locally against this branch (see Testing above).

bolt_dag.batches preserve unit names exactly as authored in
unit-of-work-dependency.md (often upper, e.g. "U-16"), but the swarm
referee forces kebab-lower unit slugs, so the SWARM_UNIT_CONVERGED and
per-unit REVIEW_COMPLETED audit rows carry "u-16". The engine compared
the two without case-folding, so a converged batch read as fully
unconverged and `next` re-emitted invoke-swarm for batch 1 forever; the
downstream settle evidence gate, artifact-guard exemption, and per-unit
review-receipt check would then refuse the approval for the same reason.

Lowercase the row-derived sets once at the readers and fold the
authored-DAG side at each membership test, while still emitting the
ORIGINAL-cased unit names in invoke-swarm directives (the conductor's
`prepare` re-derives the kebab slug from them):

- aidlc-lib.ts swarmConvergedUnits(): store lowercased Unit name.
- aidlc-orchestrate.ts tryEmitSwarm() + isSettledAutonomousSwarm():
  fold the batch/DAG name at the membership test.
- aidlc-state.ts isSettledSwarmForArtifactGuard() + the per-unit
  review-receipt check (reviewedUnits): fold both sides.

Tests: t201 gains an "authored-casing vs referee slug" describe (three
red-first cases: batch advance, partial re-fan original-cased, settle +
approved report); t185 gains an uppercase-authored-DAG exemption case.

Fixes awslabs#621.
@jstrunk
jstrunk force-pushed the fix/621-swarm-unit-casing branch from 5462408 to 5c23c01 Compare July 28, 2026 14:58
@jstrunk
jstrunk marked this pull request as ready for review July 28, 2026 15:21
@jstrunk jstrunk changed the title fix: case-fold the swarm unit-name membership tests (2.5.6) fix: case-fold the swarm unit-name membership tests (2.5.12) Jul 28, 2026
@apackeer

apackeer commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Thanks, Jeff. #621 is a real current-v2 bug, and this PR correctly identifies the downstream convergence, approval, and review comparisons that need to understand the worktree slug. I ran the focused t185/t201 slice from this head: both files passed, with 37 assertions.

There is one earlier boundary we need to include before integrating it. The current conductor passes directive.units unchanged to aidlc-swarm prepare, and prepare forwards each value unchanged to aidlc-worktree create --slug. U-16 is therefore rejected before any lowercase convergence row can be written. The new tests seed those rows directly, so they do not exercise that path; the current comment that the conductor re-derives the slug is not the shipped contract.

We also need to preserve authored unit identity independently from filesystem identity. The DAG currently accepts U-AUTH and u-auth as distinct names, while this patch folds both into one convergence/review key, so one row could satisfy both. The same global fold affects non-swarm per-unit reviews. The current-v2 integration should preserve authored names in DAGs, directives, audit, and results; derive and validate a lowercase worktree slug inside deterministic swarm tooling; reject units that collide after that mapping; and use shared comparison logic for legacy lowercase rows. A real prepare -> review -> finalize -> next/approve uppercase test plus a collision case should cover the boundary.

This branch now conflicts and overlaps heavily with current v2, so please do not spend time rebasing it. Maintainers can port the useful downstream fixes and integrate the complete authored-name/worktree-slug split. I suggest keeping this PR open until that integration lands, then closing it as incorporated.

@leandrodamascena, could you take another review pass and confirm this identity boundary and current-v2 integration direction before maintainers port the fix?

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