Skip to content

feat: move the ARS arithmetic into a deterministic ars subcommand (2.5.25) - #644

Merged
apackeer merged 3 commits into
awslabs:v2from
iuryeng:feat/618-ars-subcommand
Jul 30, 2026
Merged

feat: move the ARS arithmetic into a deterministic ars subcommand (2.5.25)#644
apackeer merged 3 commits into
awslabs:v2from
iuryeng:feat/618-ars-subcommand

Conversation

@iuryeng

@iuryeng iuryeng commented Jul 22, 2026

Copy link
Copy Markdown

Summary

Implements #618: the ARS arithmetic moves out of the composer persona's prose into a deterministic ars subcommand on aidlc-graph.ts, with all weights and priors in the schema-versioned tools/data/ars-priors.json. Same five component scores in, same proposal numbers out — reproducible, auditable, and runnable without an LLM. Closes #618.

Per the issue's "Explicitly unchanged" section: this is a who-computes swap. The proposal schema and the three-block gate render are untouched, nothing deterministic routes on the composite (it stays advisory), and no conductor/SKILL.md edits were needed.

Changes

1. ars subcommand (core/tools/aidlc-graph.ts):

bun <harness>/tools/aidlc-graph.ts ars --iae <s> --csu <s> --ve <s> --r <s> --ua <s> [--completed <csv>] [--project-type <t>]

Prints a JSON ArsResult: the weighted composite (raw + rounded) with its band, per-component LOW/MED/HIGH bands at the shipped 0.30/0.70 boundaries, the per-stage EV screen against the cost-prior table, nearest stock scopes by grid diff count, and the two gate tables pre-rendered as markdown (tables.arsScores, tables.stageDecisions). The weighted sum is normalized at a fixed precision before rounding, so a composite that is exactly a half-point lands in the band the documented formula gives rather than one below it. --project-type screens out the stages whose compiled condition: restricts them to the other kind of project (today Reverse Engineering, brownfield-only); --completed keeps already-run stages EXECUTE and outranks that screen. Validation follows validate-grid's typo discipline: scores outside [0,1], unknown stage slugs in --completed, and priors-schema violations all exit 1 with a naming error — never a silent fallback.

2. tools/data/ars-priors.json (schemaVersion 1): component weights (0.20/0.30/0.25/0.15/0.10), band boundaries, the stage cost-prior table, the EV thresholds, and an optional per-stage projectTypes that mirrors a compiled condition: restricting a stage to one kind of project. loadArsPriors() hard-validates the schema (weights sum to 1.0, bands tile 0–100 contiguously, cost is a number or null, projectTypes names known types, every stage entry names a compiled-graph slug). Slug validation runs against the unfiltered graph, so an install whose plugin selection disables a core stage still runs ars.

3. Composer persona diet (core/agents/aidlc-composer-agent.md): §2.3 now instructs score-the-components → run the tool → copy its numbers verbatim (the formula stays as documentation of the data file) and states what --project-type does to the screen; the Cost Priors table is marked as documentation of ars-priors.json; Step 8a takes both gate tables from the tool's tables output. The scoring guidance (signals, anchors, CodeKB-first discipline) is untouched. docs/guide/12-cli-commands.md gains an ### aidlc-graph ars section next to validate-grid.

4. Unit tests (tests/unit/t258-ars-subcommand.test.ts, 21 tests): pins the arithmetic against the persona's own worked example (0.55/0.75/0.65/0.50/0.55 → 63 / Comprehensive, byte-matching the persona's Table 1 composite row), the 0.29/0.30/0.69/0.70 band edges, the composite 20/21 band edge plus an exact half-point that only rounds correctly under the normalization, the project-type screen in both directions and its COMPLETED precedence, the priors schema (every graph stage covered, no no-prior fallthrough), COMPLETED handling, and the exit-1 discipline including priors fault-injection via the AIDLC_ARS_PRIORS seam and a disabled-stage install via AIDLC_STAGE_GRAPH.

Design decisions to review

Four places where the issue's spec needed a concrete choice — flagging each for review:

  1. EV thresholds for costs 3 and 5. The persona ships anchors only for costs 1 (> 0), 2 (> 0.2), and 4 (> 0.4). The data file linearly interpolates/extends: cost 3 → 0.3, cost 5 → 0.5, documented as uncalibrated in the file's _comment.
  2. incident-response and feedback-optimization have no row in the persona's cost-prior table. Rather than inventing costs, they ship cost: null and the screen reports them as "not numerically screenable — human judgment at the gate". Happy to assign costs instead if you'd rather close the table gap here.
  3. approval-handoff (phase gate) resolves deterministically: EXECUTE iff any other ideation-phase stage screens EXECUTE/COMPLETED — matching "Always at the ideation→inception boundary" while letting a fully-folded ideation phase fold its gate too (stock bugfix behaves this way). Known divergence: two stock grids, mvp (4 ideation EXECUTEs) and poc (1), SKIP the gate anyway, so the mechanical screen carries a systematic +1 diff against both in nearestScopes. That is a pre-existing tension between the persona prose and the stock grids rather than something this PR introduces, and the output labels the screen a starting signal — but it is worth reconciling in one direction or the other, and I am happy to open it separately.
  4. Nearest-scope diffs compare against the tool's mechanical screen grid (the structural stages default SKIP, flagged as judgment calls). The composer's folded grid may differ — the output labels this a starting signal, not the proposal. Table 2 is likewise the mechanical screen; the persona instructs folds to update only the rows they change, keeping every untouched row verbatim.

User experience

Before: the composer persona multiplied five weighted scores in prose — the same component scores could render different composites and band labels across runs, and a lean-grid-inclined model could nudge the composite without any single component looking wrong. After: the composer cites evidence for five scores and runs one command; every downstream number is computed, byte-stable, and pinned by tests. This also creates the calibration seam the issue names as the goal: the scoring function now runs without an LLM against a labeled fixture corpus, and the priors can be tuned in one data file.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

  • I have reviewed the contributing guidelines
  • I have performed a self-review of this change
  • Changes have been tested
  • Changes are documented

Test Plan

  • t258 → 21/21. t68 confirms the 2.5.25 version/CHANGELOG/badge sync; t190, t227, t239, t184, t213, t134, t55 and gen-coverage-registry green alongside it (the coverage registry carries this branch's t258 entry alongside v2's t244 and t247).
  • bun scripts/package.ts --check → green for all 5 harnesses (the priors file and regenerated persona/tool ship in every dist).
  • bun run typecheck → clean across all 3 tsconfigs.
  • bun run lint → clean (the pre-existing useOptionalChain warning at aidlc-doctor-bundle.ts:628 this PR previously reported is gone from current v2).
  • Manual smoke: ars --iae 0.55 --csu 0.75 --ve 0.65 --r 0.5 --ua 0.55 renders the persona's documented example exactly (composite row | **Composite ARS (advisory)** | - | **63 / 100** | **Comprehensive** |, raw: 62.75); --ve 0.03 --r 0.83 --ua 0.73 renders raw 20.5 / total 21 / Focused; --project-type greenfield --csu 0.8 SKIPs reverse-engineering with the condition-agreement reason; out-of-range and unknown-slug inputs exit 1.

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

@apackeer

Copy link
Copy Markdown
Contributor

Review (at a3b484b)

Ran at the PR head in a clean worktree: bun scripts/package.ts --check green for all 5 harnesses, bun run typecheck clean, and the full smoke+unit tier green (176 files, 0 failures; t244 13/13). The implementation follows #618's spec closely, the "explicitly unchanged" contract holds (no SKILL.md edits, proposal schema untouched, nothing deterministic routes on the composite), and the test file is strong: the persona worked-example pin, the no-no-prior ratchet, and the AIDLC_ARS_PRIORS fault-injection seam are all the right instincts. The findings below are logic/contract issues the current tiers don't pin; each was reproduced against the PR head.

Must-fix

1. Rebase and re-bump: the version slot is stale, which is why GitHub reports conflicts. The branch declares 2.5.8 (core/tools/aidlc-version.ts, the CHANGELOG heading, the README badge), but v2 has since shipped 2.5.8, 2.5.9, and 2.5.10. A merge-tree against v2 shows the conflicts are exactly the version trio (CHANGELOG.md, README.md, aidlc-version.ts plus its five dist copies); everything else auto-merges, and the t244 slot and coverage-ratchet baselines are still clean on v2. Rebase, re-bump to 2.5.11 (or whatever is current at merge time), rename the CHANGELOG heading to match, update the badge, and re-run bun scripts/package.ts. The PR title still says 2.5.6.

2. Floating-point error can cross a composite band boundary. core/tools/aidlc-graph.ts:2152-2153 rounds the raw IEEE sum directly. For --iae 0 --csu 0 --ve 0.03 --r 0.83 --ua 0.73 the exact composite is 20.5 (0.75 + 12.45 + 7.3), which should round to 21 / Focused; the tool emits raw: 20.499999999999996, total: 20, label: "Near-direct". The output is deterministic, but it disagrees with the documented formula computed exactly, and the disagreement lands on the one thing the gate render bolds: the band label. Normalize before rounding (round the raw sum to a fixed precision first, or compute in scaled-integer space) and add a half-point boundary case to t244; the current band-edge tests use uniform scores, where the accumulated error is too small to bite. The same fix cleans up the "raw": 62.74999999999999 noise in the JSON output.

3. Issue number in shipped code and data. core/tools/aidlc-graph.ts:1967 and :2346 carry (#618) in comments, and tools/data/ars-priors.json embeds (#618) in its _comment, which ships verbatim into every user's project via all five dists. Repo convention keeps issue refs in commit messages and PR bodies; in source and shipped strings, describe the behavior instead ("the deterministic ARS arithmetic" already does the job). The t244 header comment has one too.

Should-fix

4. --project-type is validated and echoed but never applied, and the unmodified screen can contradict a stage's own execution condition. With --project-type greenfield --csu 0.8 the screen emits reverse-engineering EXECUTE - reduces CSU=0.80 > threshold 0.4 (cost 4), while the stage frontmatter says Execute when project is brownfield. ... Skip for greenfield projects. (core/aidlc-common/stages/inception/reverse-engineering.md:5). Since the persona instructs the composer to keep untouched Table 2 rows verbatim, the mechanical screen can ship a row that contradicts the compiled condition. Either wire the flag into the screen (greenfield forces reverse-engineering to SKIP, plus any other condition-driven stages) or drop the flag until it does something; echo-only invites the belief that it changes the result.

5. Priors validation accepts a string cost. The check at core/tools/aidlc-graph.ts:2105 is String(st.cost) in evThresholds, so "cost": "1" passes (exit 0) and the string leaks into the output JSON's cost fields, violating the declared number | null contract on ArsPriors/ArsScreenRow. Verified via the AIDLC_ARS_PRIORS seam. Add a typeof st.cost === "number" || st.cost === null guard before the threshold-key check, plus a fault-injection line in t244.

6. ars hard-fails on installs with disabled plugin stages. computeArs validates every priors.stages slug against loadGraph(), which filters out stages marked enabled: false (the plugin-selection recompile marks disabled nodes exactly that way). On an install whose selection disables any core stage, the shipped priors still name all 32 slugs, so every ars invocation exits 1. Reproduced by pointing AIDLC_STAGE_GRAPH at a graph copy with one stage marked enabled: false: ars priors: stages.market-research is not in the compiled stage graph. The stale-data check is right in spirit; validate against the unfiltered graph (or tolerate known-but-disabled slugs) while still rejecting truly unknown ones. The reverse direction is already handled gracefully: a plugin-added stage with no priors entry gets a no-prior SKIP row.

7. Document the new subcommand. docs/guide/12-cli-commands.md catalogs the deterministic CLI tools and has a dedicated section for aidlc-graph validate-grid; ars needs a matching ### aidlc-graph ars entry in the same change, per the docs policy.

Nit

The phase-gate screen (approval-handoff executes iff any other ideation stage does) diverges from two stock grids: mvp (4 ideation EXECUTEs) and poc (1) both SKIP the gate, so the mechanical screen carries a systematic +1 diff against them in nearestScopes. That is a pre-existing tension between the persona prose and the stock grids, not this PR's bug, and the output correctly labels the screen a starting signal; worth a line under design decision 3.

On the four flagged design decisions

1 (interpolated EV thresholds for costs 3 and 5): reasonable, and honestly disclosed as uncalibrated in the data file. 2 (cost: null over invented costs): the right call; "not numerically screenable" beats a made-up prior. 3 (phase-gate resolution): sound logic, see the nit above. 4 (mechanical screen as the nearest-scope baseline): correct framing, and keeping fold-changed rows as the only deviations from the tool's Table 2 preserves auditability.

@iuryeng
iuryeng force-pushed the feat/618-ars-subcommand branch from a3b484b to e21b5cc Compare July 25, 2026 14:34
@iuryeng iuryeng changed the title feat: move the ARS arithmetic into a deterministic ars subcommand (2.5.6) feat: move the ARS arithmetic into a deterministic ars subcommand (2.5.13) Jul 25, 2026
@iuryeng

iuryeng commented Jul 25, 2026

Copy link
Copy Markdown
Author

Thank you for the depth here — every finding reproduced at the old head, and all seven are addressed. New head is e21b5cc3, rebased onto v2 (257b43a3).

1. Rebase and re-bump — done, at 2.5.13

Rebased and re-bumped to 2.5.13 rather than 2.5.12: #646 already claims 2.5.12 on its open head, so taking that slot here would just move the conflict. CHANGELOG heading renamed and repositioned above 2.5.11, README badge updated, aidlc-version.ts plus the five dist copies regenerated, PR title corrected. Your conflict read was exact — the version trio and its dists — with one addition since your review: tests/unit/gen-coverage-registry.test.ts picked up t247, so the registry now carries both t244 and t247.

2. Floating-point band crossing — fixed

Reproduced exactly as you described: --iae 0 --csu 0 --ve 0.03 --r 0.83 --ua 0.73 summed to 20.499999999999996 and printed total: 20, label: "Near-direct", against an exact 20.5 → 21 → Focused. The weighted sum is now normalized to a fixed precision (1e-9 — roughly five orders of magnitude above the ~1e-14 error that accumulates at composite magnitudes, so it cannot swallow a real input digit) before Math.round. Same invocation now prints raw: 20.5, total: 21, label: "Focused", and the persona example prints raw: 62.75 instead of 62.74999999999999. t244 pins the half-point case and the clean raw; the pre-existing uniform-score band edges stay as they were.

3. Issue references in shipped code and data — removed

Gone from both aidlc-graph.ts sites, the _comment in ars-priors.json, and the t244 header; each now describes the behavior instead. grep -rn "#618" core/ harness/ tests/ docs/ plugins/ returns nothing.

4. --project-type — wired, not dropped

Kept the flag and gave it teeth, since the stage's own compiled condition is the thing the screen must not contradict. Priors entries gained an optional projectTypes array mirroring a condition: that restricts a stage to one kind of project — today only reverse-engineering: ["brownfield"]. practices-discovery names both types in its condition but runs on either, so it stays unrestricted. With --project-type greenfield, the row now reads:

| 2.1 | reverse-engineering | SKIP | project is greenfield - the stage's compiled condition restricts it to brownfield projects |

Precedence: --completed still wins (the stage demonstrably ran); without the flag the output is byte-identical to before. I deliberately did not parse the free-prose condition: string — a structured mirror is auditable and fails loudly under schema validation if it drifts, prose matching would silently rot.

5. String cost — guarded

typeof st.cost === "number" || st.cost === null now runs before the threshold-key lookup, and the error names the stage and the type it received. Fault-injection line added to t244 via the AIDLC_ARS_PRIORS seam, plus one for an unknown projectTypes value.

6. Disabled plugin stages — tolerated

Priors slugs now validate against loadStageGraphAll() (unfiltered) while the screen still walks loadGraph() (enabled only): an install that disables a core stage runs ars normally and simply carries no row for that stage. A slug missing from the unfiltered graph is still stale data and still exits 1. t244 covers both directions — one graph copy with market-research marked enabled: false (exit 0, no market-research row, other stages intact) and one with the slug renamed (exit 1, message unchanged).

7. Docs — added

docs/guide/12-cli-commands.md has an ### aidlc-graph ars section next to validate-grid: flags, the priors data file as the source of every constant, the exit-1 discipline, and two examples. The composer persona also documents what --project-type does now that it does something.

Nit — phase gate vs the mvp / poc grids

Recorded under design decision 3 in the PR body: the screen's rule carries a systematic +1 diff against both stock grids in nearestScopes, pre-existing tension between the persona prose and the grids rather than something this PR introduces. Left as-is here — the screen is the starting signal and the fold step owns the final grid — but it is worth reconciling in one direction, and I am happy to open it as its own issue if you agree.

Validation at e21b5cc3

bun scripts/package.ts --check green on all five harnesses; bun run typecheck and bun run lint clean (the useOptionalChain warning at aidlc-doctor-bundle.ts:628 I flagged earlier is no longer present on current v2, so that offer is moot); t244 17/17; t68, t190, t227, t239, t184, t213, t134, t55 and gen-coverage-registry green alongside it. I develop on Windows, where a known-unrelated subset of the suite fails on path-separator and short-path assumptions, so the full-tier number is yours to confirm — the affected files above were all run individually and are green.

@apackeer

Copy link
Copy Markdown
Contributor

Review round 2 (at e21b5cc)

Re-verified everything at the new head in a clean worktree: all seven round-1 findings are genuinely fixed, and each fix was reproduced against the dist tree, not just read. Half-point case now prints raw: 20.5, total: 21, Focused and the persona example a clean 62.75 (the actual Step 8a scores render 63 / Comprehensive byte-exact); grep "#618" across core, tests, and docs returns nothing; --project-type greenfield SKIPs reverse-engineering with the condition named in the reason, --completed outranks it, and I confirmed reverse-engineering is the only condition-restricted stage (practices-discovery's condition names both types but runs on either, so leaving it unrestricted is right); the injected string cost and projectTypes: ["bluefield"] both exit 1 naming the stage; a graph with market-research disabled runs clean with 31 rows while a renamed slug still exits 1; and the docs section is in place next to validate-grid.

Gates at the head: bun scripts/package.ts --check green for all five harnesses, bun run typecheck and bun run check clean, smoke+unit tier 178 files / 4405 assertions, 0 failures. A git merge-tree against the current v2 tip (207db2e) is conflict-free. CI itself never ran on this head - both workflow runs sit at action_required awaiting maintainer approval, so the numbers above are from a local run.

Two new must-fixes, both artifacts of the world moving after your 07-25 rebase plus one edge the round-1 fix left open:

Must-fix

1. Rename the test file: t244 now collides with a file that landed on v2 after your rebase. #596 merged on 07-27 and brought tests/unit/t244-scope-matrix-doc-sync.test.ts. Because the filenames differ, git auto-merges clean - the merged tree silently carries TWO t244-* files, --filter "t244" matches both forever, and nothing guards t-number uniqueness (I checked run-tests.ts, the registry generator, and t68). Slot picture on open PR heads today: t248 is claimed twice (#613, #660), t249 by #617, t250 by #616, so t251 is the first visibly free number - worth re-checking when you rename, this keeps moving. The rename touches the filename, the file's own header, the entry in gen-coverage-registry.test.ts (line 883 on your head), and the three path-keyed rows in tests/.coverage-registry.json (regenerate rather than hand-edit).

2. A trailing --project-type with no value is silently ignored. bun ... ars --iae 0 --csu 0.8 --ve 0 --r 0 --ua 0 --project-type exits 0 with projectType: null and reverse-engineering EXECUTE - the caller believes the greenfield screen ran and it did not. That contradicts the CHANGELOG's own "never a silent fallback" line, and the asymmetry is one screen apart in the same handler: --completed rejects exactly this shape ("requires a comma-separated value", aidlc-graph.ts:2443) while --project-type only guards the flag-as-value case (:2450; --project-type --completed ... correctly exits 1). Mirror the --completed guard and add the trailing-flag case to the renamed test. (A fully misspelled flag like --project_type greenfield is also silently ignored, but that is CLI-wide behavior - validate-grid treats strays the same way and only the two sensor tools reject unknown flags - so strict arg parsing belongs to a separate issue for the whole dispatcher, not this PR.)

Should-fix

3. Pin the projectTypes mirror to the compiled condition it mirrors. Your structured-mirror rationale is sound for runtime, but today the link is enforced by nothing: validation at :2139 is enum-only, so editing reverse-engineering's condition: (or adding a new project-restricted stage) leaves the priors silently stale and can recreate the exact contradiction finding 4 fixed. The compiled stage-graph.json carries the condition prose verbatim, so the guard is cheap and lives in the test, not in runtime parsing: assert both directions in the ARS test file - every priors entry with projectTypes corresponds to a stage whose condition names that restriction, and every stage whose condition matches the brownfield/greenfield-restriction phrasing has a mirror entry. Same two-direction drift discipline as t124/t132. The current greenfield pin would not catch a removed or newly-added restriction.

4. Reject scores with more than two decimals. --iae 0.299 renders | Intent Ambiguity | IAE | 0.30 | LOW | against the documented LOW < 0.30, and --csu 0.4004 produces the self-contradictory reason reduces CSU=0.40 > threshold 0.4. The banding itself is computed on the exact value (correct); only the toFixed(2) render (:2193) lies about it. The persona's rubric works entirely in two-decimal scores, so the clean fix is to reject finer input at the existing range check - it matches the [0.00, 1.00] vocabulary and the exit-1 discipline, and keeps table and band incapable of disagreeing.

Notes

The 2.5.13 slot (skipping 2.5.12, which #646 claims) is fine - t68 tolerates gaps - though the contested-slot picture keeps shifting (#658 and #664 also sit on 2.5.12 today, #661 on 2.5.14), so re-check at merge time as usual. And on the phase-gate divergence you recorded under design decision 3: agreed it is pre-existing and correctly labeled; a separate issue to reconcile the persona prose with the mvp/poc grids would be welcome.

Findings 2-4 here came out of a second independent verification pass over the round-1 fixes; each was reproduced at e21b5cc before being written up.

iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Jul 29, 2026
…4 slot

Review round 2 on awslabs#644.

1. Test file renamed t244 -> t258. awslabs#596 merged on 07-27 and brought
   tests/unit/t244-scope-matrix-doc-sync.test.ts; the filenames differ, so git
   auto-merges clean and the merged tree would silently carry two t244-* files
   with `--filter "t244"` matching both forever. t251, the first free slot at
   review time, has since been claimed by awslabs#671, so this takes t258 - free on
   v2 and unclaimed by every open PR as of today. The coverage registry was
   regenerated, not hand-edited.

2. A trailing `--project-type` with no value now exits 1 instead of falling
   through to unset. Ignoring it silently reported EXECUTE for a stage the
   caller believes the greenfield screen excluded, contradicting the entry's
   own "never a silent fallback" line. Same shape as the --completed guard one
   screen up; a flag-as-value keeps its more specific enum message, which
   names what it read.

3. Scores finer than two decimals are rejected at the existing range check.
   Banding reads the exact value while the tables render toFixed(2), so
   --iae 0.299 printed "0.30 | LOW" against the documented LOW < 0.30, and
   --csu 0.4004 produced the self-contradictory reason "reduces CSU=0.40 >
   threshold 0.4". The rubric works in two decimals throughout.

4. The priors `projectTypes` mirror is pinned to the compiled condition in
   both directions: every mirror answers to a condition that skips the other
   project type, and every such condition has a mirror. Schema validation is
   enum-only, so editing a stage's condition alone would have left the priors
   stale and recreated the contradiction the mirror exists to prevent.
   practices-discovery, which names both types to describe per-type behaviour,
   is asserted NOT to count as a restriction.

t258 grows 17 -> 21 tests.
@iuryeng

iuryeng commented Jul 29, 2026

Copy link
Copy Markdown
Author

Thank you — the second independent pass caught two real holes, and finding #1 in particular is the kind of thing only someone re-checking the merged world would see. All four items are addressed at 7464c0ab.

Must-fix 1 — renamed, and the slot moved again

You were right to flag that the number keeps moving: t251 is no longer free#671 (t251-sensor-stdout-noise.test.ts) claimed it since your review. I walked all 69 open PRs for added tests/**/t<N>* files and the picture right now is t248 (#613, #654, #660, #670), t249 (#617, #653), t250 (#616, #669), t251 (#671), t252 (#667, #672, #673), t253/t254 (#673), t255/t256 (#674), t257 (#675). So this takes t258, which is free on v2 and unclaimed by every open PR today.

Renamed all four surfaces you listed: the file, its own header and describe blocks (plus the two mkdtemp prefixes), the entry in gen-coverage-registry.test.ts (re-sorted into alphabetical position, between t247 and t27), and the three path-keyed rows in tests/.coverage-registry.json — regenerated with bun tests/gen-coverage-registry.ts, not hand-edited. .coverage-ratchet.json is unchanged, as expected: the rename moves no counts.

Must-fix 2 — trailing flag rejected

--project-type with no value now exits 1 with --project-type requires a value (brownfield or greenfield).

One deliberate narrowing, and I want it visible rather than buried: I did not extend the guard to the startsWith("--") half that --completed also covers. That case already exits 1 through the enum check with a message that names what it read (must be brownfield or greenfield (got "--completed")), which is strictly more informative than "requires a value" — and you verified that path as correct in round 1. Mirroring the guard literally would have replaced a good message with a vaguer one. If you would rather have the two flags symmetric down to the wording, say so and I will align them.

The new test covers both shapes so the distinction is pinned, not incidental.

Should-fix 3 — mirror pinned in both directions

projectTypes is now checked against the compiled condition prose in the test, not at runtime:

  • Restriction rule: a condition restricts a stage when it says to skip the other project type outright. Naming both types to describe per-type behaviour is not a restriction — practices-discovery is asserted not to count, which pins the exact judgment you made in round 1.
  • Direction 1: every priors entry with projectTypes answers to a restricting condition and names the same type.
  • Direction 2: every restricting condition has its mirror entry.
  • Anchor first: reverse-engineering must resolve to brownfield before either loop runs — otherwise a regex that stopped matching would let both directions pass vacuously over two empty sets, which is the failure mode a two-direction guard is most prone to.

Failures are paired with the slug so the diff names the stage that drifted.

Should-fix 4 — finer-than-two-decimals rejected

Rejected at the range check, via Number(v.toFixed(2)) !== v — which avoids the v * 100 float trap (0.29 * 100 is 28.999999999999996). --iae 0.299 and --csu 0.4004 now exit 1 naming the flag; 0, 0.07, 0.29, 0.3 and 1 stay legal, pinned in the test.

Scoped note: I applied it at both range checks — the CLI one at the parse screen and the computeArs one — because the existing [0.00, 1.00] check is already duplicated across those two, and guarding only the CLI would leave the in-process seam (which the tests and any direct importer use) able to produce the contradictory table.

What I did not do

Both of your separate-issue observations are untouched here: strict unknown-flag parsing for the whole dispatcher, and reconciling the persona's phase-gate prose with the mvp/poc grids. Happy to file either (or both) as their own issue if you want them tracked — I did not want to widen this PR to reach them.

The only change beyond the four items is one CHANGELOG bullet: it claims "never a silent fallback", and that claim was incomplete while these two inputs were silently accepted, so the line now names them.

Gates at 7464c0ab

bun scripts/package.ts --check green on all five harnesses; bun run check clean (three tsconfigs + biome, 541 files); t258 21/21 (was 17), gen-coverage-registry green, t68 green. Version 2.5.13 is still free — origin/v2 sits at 2.5.11. Same caveat as before: this is a local Windows run and the fork's CI still sits at action_required, so I have not seen the v2 gate itself execute on this head.

@apackeer

Copy link
Copy Markdown
Contributor

Review round 3 (at 7464c0ab)

All four round-2 findings are addressed.

  • The ARS test is consistently renamed to t258 (file/header/describes/temp prefixes/coverage registry), the registry is regenerated and fresh, and a current scan of open PR file lists finds no other t258 claim.
  • A trailing --project-type now exits 1; the flag-as-value path still exits 1 with the more specific enum error.
  • projectTypes is pinned to compiled conditions in both directions, with the Reverse Engineering anchor preventing a vacuous pass and Practices Discovery explicitly excluded.
  • Scores finer than two decimals are rejected in both the CLI parser and computeArs; legal 0/1/2-decimal values remain accepted.

I reproduced the two new CLI failures directly and reviewed the generated dist copies. Local gates at this head: t258 21/21; coverage-registry + t68 40/40; bun scripts/package.ts --check green for all five harnesses; bun run check clean; smoke+unit 178 files / 4,409 assertions / 0 failures. Merge-tree against current origin/v2 (207db2ea) is conflict-free. GitHub still reports no checks on the branch, so these are local results.

No further code findings.

One merge-order note: #657 also currently declares 2.5.13. This PR claimed 2.5.13 at f3ebc1d9 on 07-25 14:09 UTC; #657 rebumped to it later at ea2bff63 on 07-25 20:30 UTC, so I am not assigning that collision to this PR now. Whichever one lands second must apply the documented rebase/re-bump conflict trap.

@iuryeng

iuryeng commented Jul 29, 2026

Copy link
Copy Markdown
Author

Thank you — and for re-running the open-PR scan on t258 yourself rather than taking my word for the slot, which is exactly the check that caught t251 moving under me.

Noted on the 2.5.13 overlap with #657, and I appreciate you checking the claim timestamps instead of assigning the collision by default. Either merge order works for me: if #657 lands first I will rebase, re-bump to whatever slot is free at that moment, and reposition the CHANGELOG heading the same day. There is nothing here worth holding that PR for, so please take them in whatever order suits the queue.

@apackeer apackeer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving at 7464c0a. Round-3 verification confirmed all four round-2 asks are addressed:

  • Test renamed to t258 (file, header, describes, mkdtemp prefixes, gen-coverage-registry entry, and the 3 regenerated registry rows all consistent; t258 slot re-confirmed free).
  • Trailing --project-type with no value now exits 1; flag-as-value still exits 1 with the enum error.
  • Two-direction projectTypes vs compiled-condition drift pin, anchored on Reverse Engineering so the check cannot pass vacuously, with Practices Discovery explicitly excluded.
  • More than 2 decimal places rejected in both the CLI parser and computeArs; 0/1/2dp accepted.

Local gates at this head: t258 21/21, coverage-registry + t68 40/40, bun scripts/package.ts --check green on all dists, bun run check clean, smoke+unit 178 files / 4409 assertions / 0 failures.

Two mechanical items before merge (not code findings):

  1. v2 has moved to 2.5.17 since this branch was cut, so it now conflicts on core/tools/aidlc-version.ts / CHANGELOG.md and needs a rebase with a re-bump past 2.5.17 (2.5.18 is already claimed by an open PR, so 2.5.19 is the safe next slot), plus the matching CHANGELOG heading rename and README badge.
  2. CI has not run on this head (fork workflow runs pending approval); a maintainer "Approve and run" should go green before merge.

iuryeng added 3 commits July 29, 2026 23:35
…5.25)

The entropy-scoring composer computed the ARS composite, bands, EV
screen, and gate tables in persona prose - the same five component
scores could render different numbers across runs. `aidlc-graph.ts ars`
now owns every downstream number, reading weights, band boundaries,
stage cost priors, and EV thresholds from the schema-versioned
tools/data/ars-priors.json; the persona scores the components from
evidence, runs the tool, and copies its output. Unit tests pin the
arithmetic, the 0.29/0.30/0.69/0.70 band edges, the priors schema, and
the invalid-input discipline.

Closes awslabs#618
… (review response)

Review findings on the ars subcommand, each reproduced at the PR head first:

* The composite rounded the raw IEEE sum. --ve 0.03 --r 0.83 --ua 0.73 is
  exactly 20.5 by the documented formula but sums to 20.499999999999996, so
  the tool reported 20 / "Near-direct" where exact arithmetic gives 21 /
  "Focused" - a disagreement landing on the one figure the gate table bolds.
  The sum is now normalized at a precision far above the accumulated error
  before rounding, which also clears the 62.74999999999999 noise from `raw`.
* --project-type was validated and echoed but never applied, so a greenfield
  run could emit `reverse-engineering EXECUTE` while that stage's compiled
  condition says "Skip for greenfield projects". Stages a condition restricts
  to one kind of project now carry `projectTypes` in the priors and screen
  out on the other kind; --completed still outranks that screen, since the
  stage already ran.
* `String(cost) in evThresholds` accepted the STRING "1", which then leaked
  into the result JSON and broke the `number | null` contract ArsPriors and
  ArsScreenRow both declare. cost is type-checked before the lookup now.
* The priors validated their slugs against loadGraph(), which filters out
  stages a plugin selection marked `enabled: false`; on such an install the
  shipped priors still name all 32 slugs, so every `ars` call exited 1.
  Validation moves to the unfiltered graph - a slug missing from THAT graph
  is still stale data and still throws.
* Issue references dropped from the shipped source, the priors data file, and
  the test header; the behavior is described instead.
* docs/guide/12-cli-commands.md gains an `aidlc-graph ars` section alongside
  validate-grid, and the composer persona now documents what --project-type
  does to the screen.

t244 grows the half-point boundary case, the project-type screen in both
directions plus the COMPLETED precedence, the string-cost and unknown
projectTypes fault injections, and a disabled-stage install that must still
screen (with the renamed-slug case pinning that unknown slugs still fail).
17 tests; package --check green on all five harnesses, typecheck and lint
clean.
…4 slot

Review round 2 on awslabs#644.

1. Test file renamed t244 -> t258. awslabs#596 merged on 07-27 and brought
   tests/unit/t244-scope-matrix-doc-sync.test.ts; the filenames differ, so git
   auto-merges clean and the merged tree would silently carry two t244-* files
   with `--filter "t244"` matching both forever. t251, the first free slot at
   review time, has since been claimed by awslabs#671, so this takes t258 - free on
   v2 and unclaimed by every open PR as of today. The coverage registry was
   regenerated, not hand-edited.

2. A trailing `--project-type` with no value now exits 1 instead of falling
   through to unset. Ignoring it silently reported EXECUTE for a stage the
   caller believes the greenfield screen excluded, contradicting the entry's
   own "never a silent fallback" line. Same shape as the --completed guard one
   screen up; a flag-as-value keeps its more specific enum message, which
   names what it read.

3. Scores finer than two decimals are rejected at the existing range check.
   Banding reads the exact value while the tables render toFixed(2), so
   --iae 0.299 printed "0.30 | LOW" against the documented LOW < 0.30, and
   --csu 0.4004 produced the self-contradictory reason "reduces CSU=0.40 >
   threshold 0.4". The rubric works in two decimals throughout.

4. The priors `projectTypes` mirror is pinned to the compiled condition in
   both directions: every mirror answers to a condition that skips the other
   project type, and every such condition has a mirror. Schema validation is
   enum-only, so editing a stage's condition alone would have left the priors
   stale and recreated the contradiction the mirror exists to prevent.
   practices-discovery, which names both types to describe per-type behaviour,
   is asserted NOT to count as a restriction.

t258 grows 17 -> 21 tests.
@iuryeng
iuryeng force-pushed the feat/618-ars-subcommand branch from 7464c0a to 84e7e87 Compare July 30, 2026 02:38
@iuryeng iuryeng changed the title feat: move the ARS arithmetic into a deterministic ars subcommand (2.5.13) feat: move the ARS arithmetic into a deterministic ars subcommand (2.5.25) Jul 30, 2026
@iuryeng

iuryeng commented Jul 30, 2026

Copy link
Copy Markdown
Author

Thanks — rebased onto v2 38bf086b (2.5.17) and re-bumped. New head: 84e7e87a. PR is MERGEABLE again.

1. Version slot: 2.5.25, not 2.5.19

I re-ran the open-PR scan the same way I did for the t-slot in round 2, reading each PR's actual AIDLC_VERSION diff rather than its title — several titles are stale relative to their own bump, so titles undercount the claims. Everything from 2.5.18 through 2.5.24 is already taken:

slot claimed by that PR's title still says
2.5.18 #669 2.5.15
2.5.19 #671 2.5.16
2.5.20 #672 2.5.17
2.5.21 #673 2.5.18
2.5.22 #674 2.5.19
2.5.23 #675 2.5.20
2.5.24 #676 2.5.21

So 2.5.25 is the first free slot above v2's 2.5.17 — the same rule #667 followed when it skipped 2.5.13–2.5.16 to land on 2.5.17. To be clear this is a claim in a moving queue, not a preference: if you'd rather have this land ahead of #669/#671, say so and I'll move it to 2.5.19 (or any slot you name) in one push.

2. What the re-bump touched

core/tools/aidlc-version.ts plus the five dist copies, the README badge, and the CHANGELOG heading, which also moved above the 2.5.17 block.

One judgment call inside the heading rename: I also moved its date, 2026-07-25 → 2026-07-29. The file is reverse-chronological (t68's own header documents it that way), so leaving a 07-25 entry above a 07-29 one would have broken that. #667's heading is dated by authoring day rather than merge day (merged 07-30 UTC, dated 07-29), so 07-29 follows the same convention. Happy to put 07-25 back if you read the heading rename as version-only.

Two things I changed beyond the literal ask, both stale facts rather than polish — tell me if you'd rather I revert either:

  • the commit subject and PR title still said 2.5.13;
  • the PR body still described the test as t244-ars-subcommand.test.ts with 17 tests, left over from before round 2's rename. It now says t258 / 21.

The code, tests, priors, persona, docs and CHANGELOG prose are byte-identical to what you approved at 7464c0ab. Verified rather than asserted: comparing this PR's own diff before (257b43a3..7464c0ab) and after (38bf086b..84e7e87a), the file set is identical and exactly 8 files' patches differ — CHANGELOG.md, README.md, core/tools/aidlc-version.ts and the 5 dist version files. The other 25 files' patches hash the same. Within CHANGELOG.md, the 2.5.25 block body is byte-identical to the old 2.5.13 block; only the heading line changed.

3. Gates at 84e7e87a

  • bun scripts/package.ts --check → green on all five dists. Worth noting: I resolved the 5 dist version-file conflicts by hand during the rebase, and the drift guard confirms they came out byte-identical to what the packager generates.
  • bun run check → clean (typecheck across all 3 tsconfigs; biome 543 files, no fixes).
  • t258 21/21, t68 + gen-coverage-registry 40/40 → 61 tests / 1135 assertions / 0 failures, the same counts you reported.
  • t258 re-confirmed free after the rebase: v2 now carries t244 and t252, and the open PRs claim t248–t257, so there's no rename churn this round.
  • merge-tree against v2 → clean.

One thing I can't hand you: a full-suite number. On my Windows box the runner aborts at smoke on t05 (--all --debug defaults live TUI coverage unless AIDLC_TUI_LIVE is explicit), and that failure reproduces identically on a clean detached origin/v2 worktree — 16 pass / 1 fail, same case — so it's environmental and pre-existing rather than something this branch introduced. Your Linux run at the previous head stays the authority for the 178-file / 4409-assertion number, and this round's delta is confined to the 8 files above.

4. On your item 2

Agreed, and still true: CI has never executed on any head of this PR — the fork's workflow runs sit in action_required waiting on a maintainer's "Approve and run". That one's yours.

@apackeer
apackeer merged commit 2dabae2 into awslabs:v2 Jul 30, 2026
5 checks passed
iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Jul 30, 2026
…lided t244 slot

v2 merged awslabs#644 (2.5.25) and, earlier, awslabs#596's t244-scope-matrix-doc-sync.test.ts
while this branch was in review, colliding on two fronts. Per the conflict-trap
convention this branch, as the second to merge, rebases and re-bumps.

- CHANGELOG heading renamed 2.5.12 -> 2.5.27, placed above v2's 2.5.25 entry;
  aidlc-version.ts and the README badge follow. 2.5.26 is claimed by awslabs#645, so
  2.5.27 is the first free slot above v2.
- t244-source-freshness-receipts.test.ts -> t259-source-freshness-receipts.test.ts.
  v2 now ships t244-scope-matrix-doc-sync.test.ts; two files sharing a t-number
  auto-merge cleanly while leaving `--filter t244` matching both forever, and
  nothing enforces t-number uniqueness. t248-t257 are claimed by open PRs and
  t258 shipped with awslabs#644, so t259 is the first free slot. The header comment,
  six describe() blocks, five mkdtemp prefixes and the gen-coverage-registry
  entry follow the filename.
- Coverage registry and ratchet regenerated with bun tests/gen-coverage-registry.ts
  rather than hand-edited.
iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Jul 30, 2026
…lided t244 slot

While this branch was in review v2 merged awslabs#644 (2.5.25) and awslabs#645 (2.5.26), and
earlier awslabs#596's t244-scope-matrix-doc-sync.test.ts, colliding on two fronts. Per
the conflict-trap convention this branch, as the later one, rebases and re-bumps.

- CHANGELOG heading renamed 2.5.12 -> 2.5.27 and placed above v2's 2.5.26 entry;
  aidlc-version.ts and the README badge follow. 2.5.27 is the first free slot
  above v2, and no open PR claims it.
- t244-source-freshness-receipts.test.ts -> t259-source-freshness-receipts.test.ts.
  v2 now ships t244-scope-matrix-doc-sync.test.ts; two files sharing a t-number
  auto-merge cleanly while leaving `--filter t244` matching both forever, and
  nothing enforces t-number uniqueness. t248-t257 are claimed by open PRs and
  t258 shipped with awslabs#644, so t259 is the first free slot. The header comment,
  six describe() blocks, five mkdtemp prefixes and the gen-coverage-registry
  entry follow the filename.
- Coverage registry and ratchet regenerated with bun tests/gen-coverage-registry.ts
  rather than hand-edited.

No behavior change: every source, doc and test file is byte-identical to the
reviewed head modulo the t244 -> t259 rename.
iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Jul 31, 2026
…lided t244 slot

While this branch was in review v2 merged awslabs#644 (2.5.25) and awslabs#645 (2.5.26), and
earlier awslabs#596's t244-scope-matrix-doc-sync.test.ts, colliding on two fronts. Per
the conflict-trap convention this branch, as the later one, rebases and re-bumps.

- CHANGELOG heading renamed 2.5.12 -> 2.5.27 and placed above v2's 2.5.26 entry;
  aidlc-version.ts and the README badge follow. 2.5.27 is the first free slot
  above v2, and no open PR claims it.
- t244-source-freshness-receipts.test.ts -> t259-source-freshness-receipts.test.ts.
  v2 now ships t244-scope-matrix-doc-sync.test.ts; two files sharing a t-number
  auto-merge cleanly while leaving `--filter t244` matching both forever, and
  nothing enforces t-number uniqueness. t248-t257 are claimed by open PRs and
  t258 shipped with awslabs#644, so t259 is the first free slot. The header comment,
  six describe() blocks, five mkdtemp prefixes and the gen-coverage-registry
  entry follow the filename.
- Coverage registry and ratchet regenerated with bun tests/gen-coverage-registry.ts
  rather than hand-edited.

No behavior change: every source, doc and test file is byte-identical to the
reviewed head modulo the t244 -> t259 rename.
iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Aug 3, 2026
…lided t244 slot

While this branch was in review v2 merged awslabs#644 (2.5.25) and awslabs#645 (2.5.26), and
earlier awslabs#596's t244-scope-matrix-doc-sync.test.ts, colliding on two fronts. Per
the conflict-trap convention this branch, as the later one, rebases and re-bumps.

- CHANGELOG heading renamed 2.5.12 -> 2.5.27 and placed above v2's 2.5.26 entry;
  aidlc-version.ts and the README badge follow. 2.5.27 is the first free slot
  above v2, and no open PR claims it.
- t244-source-freshness-receipts.test.ts -> t259-source-freshness-receipts.test.ts.
  v2 now ships t244-scope-matrix-doc-sync.test.ts; two files sharing a t-number
  auto-merge cleanly while leaving `--filter t244` matching both forever, and
  nothing enforces t-number uniqueness. t248-t257 are claimed by open PRs and
  t258 shipped with awslabs#644, so t259 is the first free slot. The header comment,
  six describe() blocks, five mkdtemp prefixes and the gen-coverage-registry
  entry follow the filename.
- Coverage registry and ratchet regenerated with bun tests/gen-coverage-registry.ts
  rather than hand-edited.

No behavior change: every source, doc and test file is byte-identical to the
reviewed head modulo the t244 -> t259 rename.
iuryeng added a commit to iuryeng/aidlc-workflows that referenced this pull request Aug 6, 2026
…lided t244 slot

While this branch was in review v2 merged awslabs#644 (2.5.25) and awslabs#645 (2.5.26), and
earlier awslabs#596's t244-scope-matrix-doc-sync.test.ts, colliding on two fronts. Per
the conflict-trap convention this branch, as the later one, rebases and re-bumps.

- CHANGELOG heading renamed 2.5.12 -> 2.5.27 and placed above v2's 2.5.26 entry;
  aidlc-version.ts and the README badge follow. 2.5.27 is the first free slot
  above v2, and no open PR claims it.
- t244-source-freshness-receipts.test.ts -> t259-source-freshness-receipts.test.ts.
  v2 now ships t244-scope-matrix-doc-sync.test.ts; two files sharing a t-number
  auto-merge cleanly while leaving `--filter t244` matching both forever, and
  nothing enforces t-number uniqueness. t248-t257 are claimed by open PRs and
  t258 shipped with awslabs#644, so t259 is the first free slot. The header comment,
  six describe() blocks, five mkdtemp prefixes and the gen-coverage-registry
  entry follow the filename.
- Coverage registry and ratchet regenerated with bun tests/gen-coverage-registry.ts
  rather than hand-edited.

No behavior change: every source, doc and test file is byte-identical to the
reviewed head modulo the t244 -> t259 rename.
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