feat(reports): assert key cost figures against estimation-infra.json - #289
Conversation
c815e25 to
8c80245
Compare
Follow-up review + fixes (pushed as
|
…(P1-C) The report validators checked structure, decision-UX, and a11y but never confirmed the rendered dollar figures matched the estimate — the HTML could drift from estimation-infra.json silently. This adds a numeric cross-check. - validate-migration-report.py (GCP) + validate-heroku-migration-report.py: new _validate_cost_figures — every data-cost-key-anchored dollar figure must equal the matching estimation-infra.json value (GCP: aws_monthly_balanced, current_monthly->current_costs.gcp_monthly, optional premium/optimized; Heroku: aws_monthly_balanced only — current-spend deferred, key unsettled). - Required keys fail CLOSED: when a required figure's JSON value exists and exec-costs is rendered, a MISSING anchor is a FAIL (an un-anchored wrong figure must not pass — the bug P1-C exists to catch). Gated on require_anchors so minimal --no-require-toc unit fixtures are not forced to anchor. Only untagged *illustrative* numbers and absent optional per-tier keys are skipped. - Safe int-coerce (non-whole-dollar JSON -> named FAIL, never a crash); nested markup stripped before compare; mismatch/no-$ = FAIL citing JSON path vs HTML. - Emitters (generate-artifacts-report.md rule 21, heroku generate-report.md) + docs describe the REQUIRED-anchor rule (round-2: the three author-facing lines that still said 'untagged skipped / missing anchor skipped' now state required anchors are mandatory; illustrative extras stay skipped). - migration-report-reference.html: anchors the balanced + current figures. - tests: mismatch->FAIL, match->PASS, no-estimate->skip (asserts code==0), untagged->skip, nested-markup, required-absent FAIL for BOTH balanced and current_monthly, non-numeric->named FAIL. 83 passed (77 GCP + 6 Heroku cost). Both plugin trees updated byte-identically. Heroku validator + generate-report.md also touched by the report-blocking PR (awslabs#288); land order noted in the PR.
8c80245 to
8279c59
Compare
|
Thanks — verified all four findings against Should-fix — emitter/docs described the old skip-on-missing rule (agreed)Confirmed the contradiction:
Nit 1 — GCP skip test asserted only the absence of a mismatch line (agreed)
Nit 2 — no GCP test for a missing
|
herosjourney
changed the title
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Reviewed commit 8279c59aec9d9d98167b2df5cda8a6f0f9ebd9f4. The 83 targeted tests pass in each plugin tree, and the cross-plugin drift check passes. Additional CLI probes reproduced three remaining issues in both mirrors: non-rendered or misplaced anchors satisfy the required-anchor check, nested markup is read incompletely, and the decision-report emitter does not define the newly mandatory anchors.
Please address the three inline findings in both plugin trees. For the decision-mode regression, the existing fixture with its matching Balanced estimate of $155 passes on the parent commit and fails on this head solely because the anchor is missing.
|
Thanks — I reproduced all three findings independently before replying (full CLI runs, not just reading the diff):
All three will be fixed in both trees: parse actual rendered elements (excluding comments) and require the anchors in |
Three issues found in review of the data-cost-key anchor check (P1-C),
fixed in both plugin trees:
1. Anchor scanning was a document-wide regex, so an anchor inside an HTML
comment or a correct anchor placed in decision-summary (while
exec-costs itself carried a different, unanchored figure) both passed.
Replaced the regex with a stdlib HTMLParser-based scan
(_CostAnchorParser), and scoped the required-anchor check to inside
<section id="exec-costs"> specifically, per generate-artifacts-report.md
rule 21's own text ("wrap ... in exec-costs").
2. The old regex captured an anchored element's text up to the first
"</" it saw, so nested markup split across child tags was misread in
both directions: a split "$"/amount inside nested spans failed as "no
dollar amount", and a trailing digit sitting outside an inner tag
passed with a truncated (silently wrong) figure. The new parser reads
through the anchor's own matching close tag by counting nested
open/close of the same tag name.
3. report-decision-core.md (loaded by both decision mode and full mode)
never carried the data-cost-key anchor rule that
generate-artifacts-report.md did, so decision-report.html was never
held to it. Added the anchor contract to the shared renderer spec, gave
the decision fixture its own estimation-infra.json, and wired
check_expected_decide.py to pass --estimation-infra so the fixture
asserter actually exercises the gate instead of skipping it fail-open.
Also re-anchored migration-report-reference.html's Balanced/current
figures inside exec-costs (they previously lived only in decision-summary,
which is the exact "correct anchor, wrong section" bug), and added 6
regression tests covering the comment, wrong-section, and both directions
of the nested-markup case.
Verified: 83/83 tests pass in each tree, drift 273 identical / 25
allowlisted (unchanged baseline), fixtures/lint/frontmatter/model-id/types
all clean, security scanners clean.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Re-reviewed a66ba2f1b4df88ad0870717694e396d2203b8b9c against base 0f067dfe8b4da3f9cca969f1c2022a75b4b7f154. Request changes remains warranted; follow-up evidence and fixes are in the three original threads, without duplicating them.
Verified improvements: the GCP comment/section/nested-text cases now behave correctly, and the decision renderer plus normal decision CLI enforce the anchor contract. Remaining issues: the Heroku CLI retains both original parser/anchor failures; GCP accepts a non-rendered template anchor; the decision-fixture CI asserter still disables the missing-anchor gate with --no-require-toc.
Local validation: 89 targeted tests pass per plugin (178 total); all 12 changed mirror pairs match after path normalization; cross-plugin drift passes (275 identical, 25 allowlisted); git diff --check passes. Additional adversarial CLI probes reproduced the remaining issues in both mirrors. GitHub CI is separately green at the reviewed head; it does not cover these failing cases.
Three P2 findings remained open after two review rounds, all in both plugin trees: 1. Port GCP's rendered-anchor HTMLParser to the Heroku validator. Heroku's _validate_cost_figures still used the original regex (data-cost-key=...> (.*?)</), so an anchor inside an HTML comment, or a correct anchor placed outside exec-costs while exec-costs itself carried a different unanchored figure, both returned REPORT_OK. Replaced the regex with the same _CostAnchorParser (HTMLParser-based, matching-close-tag depth counting) already used by validate-migration-report.py, and scoped the required-anchor check to specifically inside <section id="exec-costs"> (added a small _section_html helper for this, since the report-blocking PR that introduces one hasn't merged into this branch yet). Also closed the GCP parser's own remaining gap while fixing this: content inside a <template> subtree is inert (never rendered by a browser) but the HTMLParser still walks its tags, so an anchor placed inside <template> was standing in for a different, wrong, visible figure right next to it. Both parsers now track template depth and skip everything inside it. 2. (Folded into #1 above via the shared parser fix) split nested markup, trailing text outside an inner tag, and character references all now read correctly in the Heroku validator too, matching GCP's existing behavior. 3. Remove the --no-require-toc escape hatch from fixtures/gcp-decision-gate/check_expected_decide.py. That flag also sets require_anchors=False, which was silently defeating the mandatory data-cost-key="aws_monthly_balanced" anchor gate this asserter exists to enforce -- verified by stripping the anchor from the real golden fixture and confirming it still returned PASS through the dispatched asserter before this fix. The fixture has a genuine <nav class="toc">, so nothing about it actually needed the flag; removing it makes the asserter run the same checks the normal CLI does. Tests: GCP 77->78 (+1 <template> case); Heroku cost 6->11 (+5: comment, wrong-section, template, split-nested, trailing-text). Total for the changed path: 95 passed each tree (was 83). Verification: pytest 95/95 both trees; cross-plugin-drift.ts OK (281 identical, 27 allowlisted); fixtures:assert PASS (8 asserters, both trees); dprint check clean; markdownlint 0 errors. Manually reproduced all three reviewer repro cases against the pre-fix code to confirm they failed as described, then confirmed each fix resolves it.
herosjourney
left a comment
herosjourney
left a comment
There was a problem hiding this comment.
Fixes for the three remaining P2s (pushed as 998bef0e)
All three fixed in both plugin trees, replied inline on each thread:
1 & 2. Heroku validator never got the round-1 parser port. Both remaining gaps traced to the same cause: validate-heroku-migration-report.py was still on the original document-wide regex (data-cost-key=...>(.*?)</) that round 1 replaced in the GCP validator. Ported the same _CostAnchorParser (HTMLParser-based, matching-close-tag depth counting) to Heroku, and scoped the required-anchor check to inside <section id="exec-costs"> there too. This closes comments, wrong-section anchors, split-nested markup, trailing text, and character references for Heroku — matching GCP's existing behavior.
Also found and closed a parallel gap in the GCP parser itself while porting it: <template> subtree content is inert (never rendered) but HTMLParser still walks its tags, so an anchor placed inside <template> could stand in for a different, visible, wrong figure. Both parsers now track template depth and skip everything inside it.
3. Removed the --no-require-toc escape hatch from check_expected_decide.py. That flag also disables the required-anchor gate (require_anchors=False). Verified directly: stripped the anchor from the real fixture and ran it through this exact asserter — before the fix it returned PASS; after, FAIL with the missing-anchor message. The fixture has a genuine <nav class="toc">, so the flag wasn't needed in the first place.
Verification (998bef0e, both trees)
pytest tests/test_validate_migration_report.py tests/test_validate_heroku_migration_report_cost.py: 95 passed (was 83; +1 GCP<template>case, +5 Heroku: comment, wrong-section, template, split-nested, trailing-text)cross-plugin-drift.ts: OK (281 identical, 27 allowlisted)mise run fixtures:assert: PASS (8 asserters, both trees) — confirmed the stripped-anchor mutation now fails through the dispatched asserter, not just the raw CLIdprint check: cleanmarkdownlint-cli2: 0 errors
@leon1418 please re-review the current head when convenient.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Re-reviewed 998bef0e031555b1c2a2fe6a86e16e88ab721ea2 against base b30cb39be02d2eae78cb8c482f4018435d4dc988. All 48 prior adversarial CLI/asserter cases now behave correctly in both mirrors. The nested-text and decision-mode findings are verified fixed.
One P2 remains in the original anchor thread: section extraction can skip mandatory-anchor enforcement for equivalent HTML serialization. An unanchored $999 figure passes against estimate 112 with a single-quoted GCP cost-section ID, or whitespace before > in Heroku section end tags. Both cases reproduce in both plugin trees, with controls and earlier-commit comparisons. Request changes remains warranted until section recognition and anchor enforcement agree.
Local validation: 95 targeted tests pass per plugin (190 total); all 12 changed mirror pairs match after path normalization; drift passes (281 identical, 27 allowlisted); the registered decision-asserter dispatch accepts the golden fixtures and rejects missing-anchor mutations; git diff --check passes. GitHub's nine checks are separately green at this head. Follow-ups are in the three original threads; no duplicate findings were opened.
_section_html() in both validators matched section boundaries with a literal-syntax regex: - GCP's version required a literal `id="value"` (double-quoted only) — `<section id='exec-costs'>` (single-quoted, equally valid HTML) was never recognized. - Heroku's version accepted both quote styles for `id=` but matched the closing tag as the literal string `</section>` — `</section >` (trailing whitespace) or a newline before the `>` was never recognized either. Both cases silently defeated the required-cost-anchor check added in this PR: `if exec_costs_html is not None:` skipped the whole requirement whenever `_section_html` failed to extract the section, even though the section was genuinely present and would render fine in a browser. An unanchored, wrong cost figure inside a differently-spelled-but-valid exec-costs section passed validation. This is the same root cause as the heroku-to-aws report validator's opportunity-row bug (PR awslabs#288): regex against raw markup instead of parsing actual structure. Replaced both `_section_html` implementations with an `HTMLParser` subclass (`_SectionScopeParser`) that finds the target `<section id="...">` by parsed attributes (any legal spelling) and its true matching close tag by counting nested same-name tags (any legal spelling), rather than string-matching a specific serialization of either. Verified against both of the reviewer's exact repros (GCP: single-quoted id; Heroku: whitespace/newline before the closing `>`) plus every existing call site via the full pytest suite in both plugin trees — 99/99 passing (up from 97), with 4 new regression tests (2 GCP, 2 Heroku) that fail against the pre-fix code and pass against the fix.
…n-basis Three findings from the latest review round, all involving the decision-gate re-entry state machine and its validator: 1. Preserve customer state instead of recursively deleting terraform/ (P1, data-loss risk). workshop-refresh.md's Stale Generate guard deleted `terraform/` recursively on re-entry. That directory is not purely generated: generate-terraform.md tells users to create terraform.tfvars (gitignored, holds real values, never regenerated), and never emits *.tfstate/*.tfstate.backup or a .terraform/ provider cache -- those come from the user's own `terraform apply`/`terraform init` and are often the only local record of what's actually deployed. Recursive deletion destroyed all of that. Replaced with an explicit, named list of only the files each Generate fragment's own `_contributes:` frontmatter declares it writes -- every other file under terraform/ (tfvars, tfstate, .terraform/, or anything else the customer added) is now left untouched. 2. Move the stale-execution-pack cleanup to the single point every re-entry path actually passes through. The cleanup was gated behind workshop-refresh.md's own `phases.generate == "completed"` check, but workshop.md Entry step 2 already resets phases.generate to "pending" via reset_downstream_to_pending before workshop-refresh.md is ever reached -- so that guard's condition could never fire on the very re-entry it was meant to catch. Worse, the "Compare scenarios" loop branch never touches workshop-refresh.md at all, so it could return to the Decision gate with the old execution pack still on disk regardless. Moved the guard (detection + confirm + deletion) into workshop.md Entry step 2 itself, which every re-entry path (Apply & reprice, Compare scenarios, or exiting workshop without entering refresh) passes through before any branch is chosen. workshop-refresh.md's own step 2 now just documents that this already happened upstream, rather than re-deriving a check that can never fire. 3. Parse actual <section> elements for decision-basis, excluding comments. _section_counts() (the older, pre-awslabs#288-relocation copy of the Heroku report validator) matched <section id="..."> via a raw regex, so the unexpanded skeleton placeholder comment from generate-report.md -- `<!-- <section id="decision-basis"> when recommendation.decision_basis exists -->` -- was counted as a real, present section even though it renders nothing. A report that declared decision_basis but shipped the template comment verbatim returned REPORT_OK. Same root cause as PRs awslabs#288/awslabs#289 in this session: regex against raw markup instead of parsed structure. Replaced with an HTMLParser subclass (_SectionOpenTagCollector) that only counts real start tags -- comments are never re-tokenized as tags. Verified: (1) and (2) are DSL/prose fixes with no executable unit under test in this repo (the reviewer's own stated verification method for these files is tracing the documented state transitions, not a live agent run) -- checked for internal consistency between workshop.md and workshop-refresh.md and confirmed the golden heroku-workshop/heroku-decision-gate fixture asserters still pass. (3) is code: added a regression test using the exact placeholder text from generate-report.md, confirmed it fails against the pre-fix code (REPORT_OK when it should fail) and passes against the fix -- 19/19 tests passing in both trees. Full repo verification also green: fixtures:assert (9 asserters), cross-plugin-drift (282 identical, 27 allowlisted), node test suites, dprint, markdownlint, lint:frontmatter, git diff --check.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Re-reviewed 306256e375213026cedbbd9af4bf47efdcd11b5d against base ade57adaf9d776d18d316d28d3cee3f746582ced. The prior section-serialization bypass is fixed, and the previous nested-text, template and decision-asserter cases remain correct.
One P2 remains, documented in the original anchor thread: the new section parser decodes escaped text and then reparses it as HTML. A printed, escaped <span data-cost-key=...> example can therefore satisfy the required anchor while the actual total is an unanchored $999 against estimate 112. This reproduces through both provider CLIs in both mirrors; the previous head rejects the same reports. Preserve escaping when reconstructing section HTML, or avoid decoding/reparsing it.
Local checks: 99 targeted tests pass per plugin (198 total); all 48 earlier CLI/asserter probes and 40 HTML variants pass; exact prior quote/whitespace repros now fail correctly; registered decision-asserter dispatch passes golden fixtures and rejects missing-anchor mutations. All 12 changed mirror pairs match after path normalization; drift passes (281 identical, 27 allowlisted); git diff --check passes. GitHub's nine checks are separately green. Request changes remains warranted for the escaped-text regression; no duplicate finding thread was opened.
_SectionScopeParser decodes character references (convert_charrefs= True, needed so callers like the dollar-amount and prose checks see plain, matchable text) and appended handle_data's ALREADY-DECODED text directly into the returned section-HTML string. An escaped code example -- e.g. `<code><span data-cost-key="x">$112</span> </code>` -- decodes to the literal text `<span data-cost-key="x"> $112</span>`, which is indistinguishable, once appended, from a REAL <span> tag that was never actually in the source. Reparsing the returned string (as _validate_cost_figures does via _cost_anchor_matches) then finds a "real" anchor that satisfies the required-anchor gate even though the report contains no such anchor. Reproduced the reviewer's exact repro: remove the Balanced anchor from exec-costs, leave a visible wrong figure, and add an escaped <code>-block example containing the correct anchor+value -- returned REPORT_OK with estimate 112 on the pre-fix code in both providers. Root cause is specific to handle_data's re-emission, not the parser's decoding itself -- switching convert_charrefs=False was considered and rejected: several OTHER _section_html callers (e.g. _dollar_amount_present matching a dollar figure written as a numeric character reference) rely on decoded plain text, and disabling decoding globally would silently reintroduce entity-blindness there. Fixed by re-escaping only the three characters that make reparsed text look like markup (<, >, &) in handle_data's output -- every other decoded character (e.g. a literal "$" from &awslabs#36;) still passes through as plain, matchable text for callers that scan prose rather than reparsing structure. Removed the now-dead handle_entityref/ handle_charref overrides (they never fire with convert_charrefs=True). Verified: the exact repro no longer produces a false anchor match: a real anchor, a real anchor alongside decoded sibling text (e.g. "&" in prose), and even a double-escaped example all behave correctly. Confirmed _dollar_amount_present still matches a dollar figure written as &awslabs#36;13.00 (proving the fix didn't regress decoding for other callers). Added one regression test per provider using the reviewer's exact repro shape; confirmed both fail against the pre-fix code (REPORT_OK where REPORT_FAIL is expected) and pass against the fix. 101/101 tests passing in both trees (up from 99). fixtures:assert (8 asserters), cross-plugin-drift (281 identical, 27 allowlisted), dprint, markdownlint, git diff --check all clean.
|
Status check: all 3 review threads on this PR have a fix pushed as the latest comment as of
No comment is currently unaddressed on our end. Ready for another pass whenever convenient — flagging in case a status ping is useful rather than waiting on the review queue. |
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Completed the follow-up review of 460eb285839a996da02bf601b436f0d6e94ddfe4 against base ade57adaf9d776d18d316d28d3cee3f746582ced. All previous findings are verified fixed, with confirmation in their original threads. No remaining substantive findings from this review.
Local validation: 101 targeted tests pass per plugin (202 total); all 48 prior CLI/asserter probes and 40 HTML variants pass. The exact escaped-example regression is rejected, while real anchors, currency entities and other section consumers retain their behavior. The registered decision-asserter dispatch passes golden fixtures and rejects missing-anchor mutations. All 12 changed mirror pairs match after path normalization; cross-plugin drift passes (281 identical, 27 allowlisted); git diff --check passes. GitHub's nine checks are separately green at this head.
This is a verification COMMENT, not a formal approval. The existing blocking review has not been dismissed or otherwise cleared; approval remains a human action.
Resolves content conflicts in three files (both plugin trees). The two sides added independent, non-overlapping report-validation features to the same files: - This branch (awslabs#289): cost-figure crosscheck — asserts data-cost-key-anchored dollar figures match estimation-infra.json (_SectionScopeParser, _CostAnchorParser, _validate_cost_figures), plus HTML-escaping/section-scope parsing fixes and the "Cost-figure anchors" report-contract rule. - origin/main (PR awslabs#310 line): currency-formatting check — flags non-whole- dollar monthly figures (_DecodedTextParser, _validate_currency_formatting) and the "Whole-dollar monthly figures" report-contract rule. The features compose cleanly, so the resolution keeps BOTH everywhere: - validate-heroku-migration-report.py: kept both function blocks (cost-figure + currency); validate() already auto-merged to call both (errors.extend(_validate_cost_figures ...) and errors.extend(_validate_currency_formatting ...)). Added the return that closes _validate_cost_figures. - test_validate_migration_report.py: took the union of both sides' added test functions (20 cost-figure tests from awslabs#289 + 25 currency tests from main + the shared base tests = 112 total). Verified every test from each branch is present and none dropped; merged via function-level union since the line- based 3-way merge interleaved the additions. No base block was modified differently by both sides (additions are disjoint). - generate-artifacts-report.md: both added a report-contract rule "21"; kept awslabs#289's "Cost-figure anchors" as 21 and main's "Whole-dollar monthly figures" as 22. - Mirrored all three files byte-identically across advisor/ and migrate/. Verification: shared validator suite 112 pass per tree; heroku currency+cost suites 32 pass per tree; drift:check OK (281 identical); fixtures:assert PASS; fmt:check clean; security:bandit 0 issues; lint:md 0 errors; git diff --check clean. Smoke-confirmed both heroku behaviors fire (currency on $25,684.89/mo; cost mismatch on a data-cost-key figure disagreeing with the estimate JSON).
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Completed the startups-hybrid-v1 review of 8ef8f6088793468da92f67a968b206c1268bf66c against ec681ba5d49b039dbf1ef9d87d3304205b824f3d: OCR v1.12.5 delegation, one fresh general review, the parent repository review, and independent candidate verification. The evidence gate passed with all 24 changed files accounted for.
Three P2s remain: numeric comparison truncates values instead of following the emitter's rounding/precision rules; invisible anchors can satisfy the rendered-cost requirement; nested recognized anchors are silently omitted. A P3 follow-up concerns the decision asserter using the golden estimate for a different supplied run. The new rounding finding is inline; the other evidence is in the original threads without duplicating them.
The report/currency suites pass 144 tests per plugin. The parent independently reran all 92 general-review probe cases against the actual checkout and confirmed the candidates; passing standard tests did not cover them. All 12 changed mirror pairs match, drift and diff checks pass, and GitHub's nine checks are separately green. No approval, dismissal, branch change or deployment was performed.
…und not truncate
Closes three review findings on the cost-figure crosscheck (all the SAME class —
"the anchor collector matched structure the browser never renders, or compared at
the wrong precision"):
1. Invisible anchors satisfied the visible-figure requirement. `_CostAnchorParser`
excluded only <template>; an anchor inside <script>/<style> or a `hidden`
subtree still counted, so a hidden $112 could stand in for a visible $999.
Rewrote the parser around an element stack that tracks inert (script/style/
template) and hidden ancestry (sticky down the subtree) and never starts an
anchor — or collects text — inside one. Mirrors _DecodedTextRunParser's
inert-tag handling so the anchor collector and the currency text parser agree
on what "rendered" means.
2. Nested recognized anchors were silently dropped. While an outer anchor was
open, handle_starttag returned early, so a nested data-cost-key was never
validated. The stack now collects every anchor independently (inner emitted on
its own close, in document order) while an outer anchor's text still includes
its children's text.
3. Numeric comparison truncated instead of rounding. `str(int(expected))` turned
112.90 into 112, so a correctly rounded "$113" failed and a wrong "$112"
passed; the small-total cents exception was lost (0.40 -> 0). New
`_canonical_money` normalizes BOTH the rendered figure and the JSON figure to
the emitter's display rule (nearest dollar >= $2, two-decimal cents < $2), so
rule-19 rounding matches. Error message updated ("not a numeric dollar amount")
and the one test asserting the old wording updated.
Four regression tests added (invisible script anchor, hidden anchor, nested
recognized anchor, fractional-estimate rounding), each verified to fail against
the pre-fix parser and pass against the fix. Both plugin trees byte-identical.
Full validator suite 116 pass per tree; drift:check OK; fmt:check clean;
security:bandit clean.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Completed the memory-isolated startups-hybrid-v1 follow-up at 1da8957e25d23551d74e9bd9f8434d67913a06d8 against ec681ba5d49b039dbf1ef9d87d3304205b824f3d. OCR delegation, the fresh general review, parent review, initial-context/tool audit, candidate verification and the canonical evidence gate completed. All 24 changed files were accounted for.
The GCP standard rounding, inert-anchor and nested-anchor fixes are verified. Three P2 groups remain: display precision (Heroku truncation and the GCP $2 boundary), rendered visibility (Heroku and GCP hidden="false"), and independent nested-anchor collection in Heroku. Details and controls are in the existing precision, visibility and nesting threads. The acknowledged supplied-run asserter P3 remains a separate follow-up; the registered golden CI path still passes.
Parent validation passed 148 pytest tests per plugin. The independent reviewer completed its scoped direct tests/asserter checks after the two omitted unchanged stub fixtures were supplied as exact-head artifacts, and the parent independently reran its candidate probes. Passing suites do not cover the remaining failures. No approval, dismissal, implementation/branch change or thread resolution was performed. Merge conflicts are a separate landing blocker.
# Conflicts: # advisor/plugins/aws-startup-advisor/scripts/validate-heroku-migration-report.py # migrate/plugins/migration-to-aws/scripts/validate-heroku-migration-report.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
4 participants
Problem
The migration report is a stakeholder-facing summary — the page an executive reads to decide whether to approve the move. Its headline is the money: "$112/mo on AWS vs $165/mo today." That report is written by an AI from the cost estimate, and until now nothing checked that the dollar figures on the page actually match the estimate they claim to summarize.
So the numbers could quietly drift. The AI could render "$112/mo" while the underlying estimate said something else, and a wrong figure would reach the decision-maker with nothing to catch it. Our existing checks looked at the report's structure, layout, and accessibility — but never the accuracy of the numbers themselves.
In code, for reviewers
The report validators check structure, decision-UX, and accessibility, but never confirm the rendered dollar figures match
estimation-infra.json. Because the HTML is LLM-generated from the estimate, the headline figures can diverge from the artifact with no gate.Solution
A numeric cross-check that asserts each anchored cost figure equals the estimate — precise,
with no false-match against the many illustrative dollar amounts elsewhere in the report.
Emitter-anchored design (avoids false-matching appendix numbers). The
exec-costsfigures arewrapped in a
data-cost-keyattribute; the validator only asserts figures carrying that anchor:validate-migration-report.pyGCP,validate-heroku-migration-report.pyHeroku):new
_validate_cost_figures— for everydata-cost-key-anchored figure, the rendered dollars(normalized: strip
$, commas,/mo, cents) must equal the mappedestimation-infra.jsonvalue.aws_monthly_balanced→projected_costs.aws_monthly_balanced;current_monthly→
current_costs.gcp_monthly; optionalaws_monthly_premium/aws_monthly_optimized.aws_monthly_balancedonly. The Heroku current-spend key is not yet settled(
heroku_monthly_baselinevsheroku_monthly_estimated), so that comparator is a namedfollow-up, not guessed at here (matching the plan's "extend Heroku once the schema is mapped").
aws_monthly_balanced; GCP alsocurrent_monthly)whose JSON value exists, in a full report (
exec-costspresent, not a--no-require-tocminimal fixture), must carry its anchor — a missing anchor FAILs, so an un-anchored
wrong figure cannot slip through. Anchor-present-but-differs → FAIL (cites
JSON pathvsHTML excerpt). Anchored element with no$→ FAIL. A non-numeric JSON value → named FAIL,never a crash. Optional tiers / unknown keys / absent JSON value / no estimate → skip. Nested
markup (
<span data-cost-key=...><strong>$112</strong></span>) is read. It asserts therendered dollars match the artifact; it does not re-run any TCO computation.
generate-artifacts-report.mdrule 21, herokugenerate-report.md): instructemitting the
data-cost-keyanchors on the balanced + current-spend figures. The attribute isnot reader-visible text, so it does not violate the GCP "reader vocabulary" rule. The
author-facing prose (rule 21 closer, the Heroku intro, and the validator's
_COST_ANCHORScomment) now states the required anchors are mandatory — a missing required anchor is a
FAIL — while only untagged illustrative extras stay skipped, matching the code.
migration-report-reference.html): anchors the$112/$165figures.docs/report-quality.md): the numeric-check scope (listed fields only).Type of Change
Team Folder
advisor/migrate/solution-architecture/Verification
cross-plugin-drift.ts: OK (273 identical, 25 allowlisted — no new drift)test_validate_migration_report.py(both trees): 77 passed each, incl. the cost tests(mismatch → FAIL, match → PASS, no-estimate → skip asserting
code==0, untagged → skip,nested-markup, required-absent FAIL for both
aws_monthly_balancedandcurrent_monthly,non-numeric → named FAIL)
test_validate_heroku_migration_report_cost.py(both trees, new file): 6 cost tests pass_validate_cost_figurestoreturn []makestest_cost_figure_mismatch_failsfail (the test exercises the real code, not dead coverage)$112/mo,$1,415/mo,$ 82,$112.50all normalize correctlyfrontmatter-validator/fixtures-check(both trees): OK;markdownlint0 errors;dprintcleanmise run build(full composite incl.bandit/semgrep/gitleaks/checkov/grype):PASS, exit 0 — run against a fresh detached worktree at this branch's commit. checkov 276/0;
gitleaks no leaks; grype no vulnerabilities.
Not run locally: no live AWS /
terraform apply— the validator is a dependency-free static reader.Landing order vs #288
This PR and #288 (heroku report-validation blocking) both touch
scripts/validate-heroku-migration-report.pyand herokugenerate-report.md. The changes areadditive and non-overlapping in intent (this adds a cost check; #288 makes report validation
blocking + a11y). The Heroku cost tests are in a separate file
(
test_validate_heroku_migration_report_cost.py) to avoid colliding with the test file #288 adds.Whichever merges second resolves a small, mechanical conflict; no logic conflict.
Checklist
mise run buildlocally and it passes — verified against a fresh worktree at this branch's commit (see Verification)docs/report-quality.md, emitter authoring rules)advisor/+migrate/migration skills)