feat: move the ARS arithmetic into a deterministic ars subcommand (2.5.25) - #644
Conversation
e36911c to
a3b484b
Compare
Review (at a3b484b)Ran at the PR head in a clean worktree: Must-fix1. Rebase and re-bump: the version slot is stale, which is why GitHub reports conflicts. The branch declares 2.5.8 ( 2. Floating-point error can cross a composite band boundary. 3. Issue number in shipped code and data. Should-fix4. 5. Priors validation accepts a string cost. The check at 6. 7. Document the new subcommand. NitThe phase-gate screen (approval-handoff executes iff any other ideation stage does) diverges from two stock grids: On the four flagged design decisions1 (interpolated EV thresholds for costs 3 and 5): reasonable, and honestly disclosed as uncalibrated in the data file. 2 ( |
a3b484b to
e21b5cc
Compare
|
Thank you for the depth here — every finding reproduced at the old head, and all seven are addressed. New head is 1. Rebase and re-bump — done, at 2.5.13Rebased 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, 2. Floating-point band crossing — fixedReproduced exactly as you described: 3. Issue references in shipped code and data — removedGone from both 4.
|
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 Gates at the head: 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-fix1. Rename the test file: 2. A trailing Should-fix3. Pin the 4. Reject scores with more than two decimals. NotesThe 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 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. |
…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.
|
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 Must-fix 1 — renamed, and the slot moved againYou were right to flag that the number keeps moving: t251 is no longer free — #671 ( Renamed all four surfaces you listed: the file, its own header and Must-fix 2 — trailing flag rejected
One deliberate narrowing, and I want it visible rather than buried: I did not extend the guard to the The new test covers both shapes so the distinction is pinned, not incidental. Should-fix 3 — mirror pinned in both directions
Failures are paired with the slug so the diff names the stage that drifted. Should-fix 4 — finer-than-two-decimals rejectedRejected at the range check, via Scoped note: I applied it at both range checks — the CLI one at the parse screen and the What I did not doBoth 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 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
|
Review round 3 (at
|
|
Thank you — and for re-running the open-PR scan on 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
left a comment
There was a problem hiding this comment.
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-typewith 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):
- v2 has moved to 2.5.17 since this branch was cut, so it now conflicts on
core/tools/aidlc-version.ts/CHANGELOG.mdand 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. - CI has not run on this head (fork workflow runs pending approval); a maintainer "Approve and run" should go green before merge.
…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.
7464c0a to
84e7e87
Compare
|
Thanks — rebased onto v2 1. Version slot: 2.5.25, not 2.5.19I re-ran the open-PR scan the same way I did for the t-slot in round 2, reading each PR's actual
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
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 code, tests, priors, persona, docs and CHANGELOG prose are byte-identical to what you approved at 3. Gates at
|
…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.
…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.
…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.
…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.
…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.
Summary
Implements #618: the ARS arithmetic moves out of the composer persona's prose into a deterministic
arssubcommand onaidlc-graph.ts, with all weights and priors in the schema-versionedtools/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.
arssubcommand (core/tools/aidlc-graph.ts):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-typescreens out the stages whose compiledcondition:restricts them to the other kind of project (today Reverse Engineering, brownfield-only);--completedkeeps already-run stages EXECUTE and outranks that screen. Validation followsvalidate-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-stageprojectTypesthat mirrors a compiledcondition:restricting a stage to one kind of project.loadArsPriors()hard-validates the schema (weights sum to 1.0, bands tile 0–100 contiguously,costis a number or null,projectTypesnames 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 runsars.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-typedoes to the screen; the Cost Priors table is marked as documentation ofars-priors.json; Step 8a takes both gate tables from the tool'stablesoutput. The scoring guidance (signals, anchors, CodeKB-first discipline) is untouched.docs/guide/12-cli-commands.mdgains an### aidlc-graph arssection next tovalidate-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, nono-priorfallthrough), COMPLETED handling, and the exit-1 discipline including priors fault-injection via theAIDLC_ARS_PRIORSseam and a disabled-stage install viaAIDLC_STAGE_GRAPH.Design decisions to review
Four places where the issue's spec needed a concrete choice — flagging each for review:
_comment.incident-responseandfeedback-optimizationhave no row in the persona's cost-prior table. Rather than inventing costs, they shipcost: nulland 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.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 (stockbugfixbehaves this way). Known divergence: two stock grids,mvp(4 ideation EXECUTEs) andpoc(1), SKIP the gate anyway, so the mechanical screen carries a systematic +1 diff against both innearestScopes. 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.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.
Test Plan
t258→ 21/21.t68confirms the 2.5.25 version/CHANGELOG/badge sync;t190,t227,t239,t184,t213,t134,t55andgen-coverage-registrygreen alongside it (the coverage registry carries this branch'st258entry alongside v2'st244andt247).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-existinguseOptionalChainwarning ataidlc-doctor-bundle.ts:628this PR previously reported is gone from current v2).ars --iae 0.55 --csu 0.75 --ve 0.65 --r 0.5 --ua 0.55renders 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.73rendersraw 20.5 / total 21 / Focused;--project-type greenfield --csu 0.8SKIPsreverse-engineeringwith 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.