Repository navigation
fix(aws-startup-advisor): reject migration reports that skip the decision core - #396
Conversation
…sion core REPORT_OK treated section IDs as completeness, so a thin HTML report could pass without the verdict, flip conditions, or architecture section. The shared validator now checks those artifact-backed fields for GCP and Azure, and the Heroku validator checks the flip list and what-if columns. Co-authored-by: Cursor <cursoragent@cursor.com>
herosjourney
left a comment
herosjourney
left a comment
There was a problem hiding this comment.
Review: reject migration reports that skip the decision core
Verdict: Approve. Well-built, correctly fail-closed, no blocking issues — one minor consistency nit worth a look.
This is exactly the kind of validator hardening the plugin needs, done the way this family of bugs has taught us to do it. I traced the behavior rather than taking the summary at face value; it holds up.
What this change is really about
The report validators had a gap with a specific shape: a report could pass by having the right section IDs while omitting the content those sections render (verdict headline, hero metrics, flip conditions, specialist callout, architecture section, what-if columns). This PR asserts the content is actually there — the same class of "green check that doesn't mean correct" problem the earlier anchor-gate work hit.
Why I'm comfortable with it
The new checks are artifact-gated, which is the most important design decision here — and it's right. Every assertion fires only when the corresponding artifact data is present (if recommendation:, if flips:, if _design_has_clusters(...), outcome == "conditional_go"). A pre-extension estimate that never had would_flip_if or track_outcomes is not falsely rejected for lacking them. The biggest risk in tightening a validator is false positives on legitimate older output, and this structurally avoids it.
It fails closed, and proves it. The test file has real negative controls: strip "What would flip this" → fail, rename the specialist callout → fail, break the exec-architecture section ID → fail, plus two positive controls (reference + decision fixtures still pass). This is precisely the fail-closed evidence that was missing in the earlier validator bugs.
The require_toc gating is consistent with the known fail-open history. The decision-core checks run only when require_toc is set, with --no-require-toc kept as the documented escape hatch for minimal unit fixtures — coherent with how that flag's fail-open behavior was tightened previously.
The fuzzy matching is thoughtful. _word_shingles confirms a condition was rendered without demanding a verbatim copy of the artifact string — avoiding brittle exact-match failures while still catching an omitted condition.
Test-plan note: the one unchecked reviewer box — "confirm a report that drops 'What would flip this' while would_flip_if is populated returns REPORT_FAIL" — is already covered by test_missing_would_flip_list_fails. That box can be checked.
One minor nit (non-blocking)
The two sibling validators match the what-if columns with slightly different strictness:
- GCP
_what_if_column_errorsmatchesArchwith"arch" in text— a bare substring that also matches "architecture" / "search." - Heroku's matches
Archwith a word-boundaryre.search(r"\barch\b", text). - Both match
HAwith"ha" in text, satisfiable by any header word containing "ha" ("share", "shape", "chart").
The practical risk is a false negative (a table missing a real HA/Arch column could still pass because another header word contains the substring), not a false positive — so it won't wrongly reject good reports, which is why it's non-blocking. But since both validators enforce the same column contract, I'd align them on the stricter word-boundary form (\bha\b / \barch\b, keeping availability as the HA synonym). Cheap, and keeps the GCP/Heroku pair from drifting.
Scope / parity
The generate-step docs for all three skills (gcp/azure/heroku) are updated in the same PR to stop describing REPORT_OK as the sole quality gate, so docs match the new behavior — the right discipline (claim and enforcement land together). The appendix-config additions ("Your choice", "Source" columns) are consistent with the decision-core spec.
Summary
Closes a real "passes validation but omits the decision content" gap, with the correct fail-closed design and negative-control tests to prove it. Artifact-gating keeps it from rejecting legitimate older reports. The only thing I'd tidy is the GCP-vs-Heroku inconsistency in the HA/Arch header matching — a false-negative-only nit, worth aligning but not worth blocking on.
herosjourney
left a comment
herosjourney
left a comment
There was a problem hiding this comment.
Reviewed: head 432a3c8 (base 0d6167a), 11 files (+455/−14) — gcp-to-aws, azure-to-aws, heroku-to-aws, and the shared report validator.
Ran: pytest on test_validate_migration_report_decision_core.py and test_validate_heroku_migration_report.py (62 passed). Mutation probes on head: removing the flip heading fails; an HTML comment does not satisfy the check; removing metric-hero from the reference fixture fails; a flip list that exists only inside <template> still passes the shared validator and fails the Heroku validator. Sweep for "sole quality gate" under skills/: no remaining hits. git merge-tree --write-tree origin/main pr-396: clean.
Did not run: mise run build, bandit/semgrep/gitleaks, live AWS, an end-to-end model run. Merge-tree against open PRs #369, #381, #383, #384, #392 could not be computed in a shallow clone. Path overlap only: #369 also edits gcp-to-aws generate-artifacts-report.md; #381 and #384 also edit azure-to-aws generate.md.
Change narrative: A report could pass on section IDs alone, so the agent skipped report-decision-core.md. The fix adds artifact-gated checks to the existing validators and tells the generate steps that REPORT_OK is not the spec. Body vs diff: matches.
| Axis | Stance | One line |
|---|---|---|
| Parsimony | clear | The checks sit in the validators that already gate the report, not in a second script. |
| Completeness | finding | The shared flip check accepts text the browser will not show. |
| Security | clear | No credential, network, or IaC change. New checks fail closed on a missing heading. |
| Alternatives | advisory | One, below. |
| Contradictions | finding | Four Graviton sentences still say the validator is structural-only. |
| Overlap | advisory | Heroku grew a second, narrower what-if column check. |
| Cross-plugin parity | clear | Single tree; no allowlist change. |
| Propagation & data | clear | No new artifact field or enum. Negative case for an omitted heading is tested and pins the exit code. Stale "structural-only": 4, listed below. |
Should-fix
-
[Completeness] A flip list inside
<template>still passes the shared validator. —plugins/aws-startup-advisor/scripts/validate-migration-report.py:1770if isinstance(flips, list) and flips and "what would flip" not in summary_text:_plain_textkeeps the text of a<template>. On the reference fixture, moving the "What would flip this" block into<template>produced no flip error. The same input failsvalidate-heroku-migration-report.py, which already treats template content as not rendered.
Consequence: an agent writing HTML to satisfy the gate can hide the list and still getREPORT_OK.
Fix: before the decision-core text scan, dropscript,style, andtemplatesubtrees the way the Heroku decoded-text parser does. Add a regression next totest_missing_would_flip_list_failsthat puts the heading only in<template>and expects the flip error. -
[Contradictions] Graviton docs still say the validator is structural-only. —
skills/gcp-to-aws/references/shared/graviton.md:86(same sentence inazure-to-aws/references/shared/graviton.md:86; alsoschema-graviton.md:124in both skills)That validator is intentionally structural-only today, so this is a net-new check, not a modification of existing behavior.
generate-artifacts-report.md:728now says a report that omits the decision-core pieces fails. Both files load on a Graviton reprice followed by Generate.
Fix: keep the follow-up (noarchitecture_comparisondollar assertion yet). Delete "structural-only" / "structural/readability only" in the four files.estimate-infra.md:343has the softer form; the dollar half is still true, the "structural/readability gate" label is not.
Heads-up (out of diff, pre-existing)
report-decision-core.md Section 7 asks for Impact and Likelihood badge columns. The new check only requires a <table> in exec-risks. The reference fixture's risk table has neither badge class, so a badge check would fail the fixture. Follow-up once the fixture matches the spec.
Alternatives
- One what-if column checker imported by both validators — wins: the template fix and the tier columns cannot drift (Heroku's copy does not require Premium/Balanced/Optimized headers; the shared one does). Costs: Heroku's validator is standalone so a single-skill install does not import the plugin-root script. Recommendation: keep the copy; make the Heroku column set match its own table in
generate-report.md, including the three tiers.
Verdict
Comment — two should-fix items, no blocking defect. Commenting rather than approving: this review is from the same session that authored the pull request. Plugin code owners decide.
…ale 'structural-only' docs Address two should-fix review findings on PR aws#396: 1. [Completeness] The shared validator's _plain_text kept <template> content, so a 'What would flip this' block hidden inside <template> satisfied the flip check even though a browser never renders it (the Heroku validator already rejected this). _plain_text now strips HTML comments and inert script/style/template subtrees before the text scan, matching the Heroku decoded-text semantics. Added test_would_flip_list_only_in_template_fails as a regression (confirmed it fails against the pre-fix _plain_text). 2. [Contradictions] Four Graviton doc files + estimate-infra.md still described the report validator as 'structural-only' / 'structural/readability gate', which the new decision-core checks contradict. Reworded to 'checks structure and decision-core content but does not (yet) audit dollar figures' — preserving the accurate part (the architecture_comparison numeric assertion is still a tracked follow-up).
|
Addressed both should-fix findings from the review in 1. [Completeness] Flip list hidden inside 2. [Contradictions] "structural-only" docs corrected. Verification: 104 tests pass across the four report-validator suites ( Not changed: the Heroku validator already strips inert subtrees, so finding 1 was shared-validator-only. The two advisory items from the review (the HA/Arch substring matching, and the |
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Reviewed all 16 changed files at 3428cc66f96b639368fcd1c9e63ff17f874d5bc9 against 0d6167ad2e6dfb858098c4162fe74d04c975532f, including the unchanged report callers. OCR delegation, the repository review, one isolated independent general review, and the convergence evidence gate are complete.
Requesting changes for six reproduced P2 issues. Consolidated corrections (all paths are under plugins/aws-startup-advisor/):
- Short conditions: fix shared condition matching in
scripts/validate-migration-report.pyand its decision-core tests. Rendered one-to-three-word conditions must pass; omitted conditions must fail. - Azure design handoff: pass the existing design in
skills/azure-to-aws/references/phases/generate/generate-artifacts-report.md. Its documented command must reject missing architecture/specialist content when the design requires it; preserve AI-only use. - Empty flip lists: validate populated artifact-derived lists in both shared and Heroku validators and their tests. Empty/partial lists must fail; complete visible lists must pass.
- Heroku heading text: normalize decoded whitespace in the Heroku validator and test multiline, entity-separated and inline-formatted headings. Preserve missing/inert-content failures.
- Decision scenario handoff: add the directory to GCP
references/phases/estimate/estimate.mdandreferences/shared/report-decision-core.md; clarify the real-run recipe inreferences/shared/validate-migration-report.md. A two-scenario Decision report with incomplete headers must fail; retain zero/one-scenario and fixture behavior. - Inert headline/hero: check visible classed elements in the shared validator and its tests. Template-only elements must fail; visible elements must pass.
Validation: 104 focused tests passed under pytest; manifest, skill-parity, plugin-conformance, changed-Markdown lint and diff-whitespace checks passed. Additional local probes reproduced each issue with positive/negative controls. The independent reviewer separately executed 63 authored test functions directly because its pytest invocation was unavailable; that is not a second pytest run. Current-head GitHub checks were successful. No cloud or model-driven end-to-end migration was run.
…ditions, require populated flip list, ignore inert markup Three decision-core checks accepted the wrong thing: - conditional_go conditions of one to three words could never match: the condition collapsed to a single shingle while the summary was cut into four-word shingles, so the sets never intersected. _phrase_rendered now cuts both sides at the condition's own token width (capped at four). - "What would flip this" was satisfied by the heading alone; an empty or partial <ul> passed with a populated would_flip_if. The list that follows the heading is now parsed and every artifact entry must render as an item. - metric-hero / verdict-headline were regex-searched in the raw summary, so a class inside <template> counted. Class tokens are now read from rendered start tags only. _RenderedFragmentParser flattens the summary into rendered text runs, list items and class tokens, skipping script/style/template subtrees and comments, with inline tags kept as one phrase. Tests cover each repro: short rendered conditions pass, omitted short/long conditions fail, empty/partial/template flip lists fail, a complete list with inline markup and entities passes, template-only headline and hero fail, unquoted class attribute passes.
…e flip heading text, require populated list The would_flip_if check stripped tags with a regex and looked for the literal "what would flip": a heading written as "What\nwould\nflip this", "What would flip this" or "What <em>would</em> flip this" was rejected, and the heading over an empty <ul> was accepted. Port the shared validator's rendered-fragment parser: decoded, whitespace- collapsed visible text with inert subtrees and comments dropped and inline tags kept as one phrase. Match the heading on that text, then require every recommendation.would_flip_if entry to render as an item of the list that follows it. generate-report.md now states the populated-list rule. Tests: the four equivalent heading spellings pass; comment-only and template-only headings fail; empty and partial lists fail; complete list passes.
… and --migration-dir on the Decision path The validator reads only the artifacts named on its command line, so two documented callers never armed the checks this PR added: - azure-to-aws generate-artifacts-report.md Step 5 omitted --aws-design, so the architecture-section and specialist-callout checks ran with aws_design=None. A report missing both returned REPORT_OK on the documented command and REPORT_FAIL once --aws-design was added. The AI-only path still omits it (no aws-design.json exists there). - gcp-to-aws estimate.md choice A and the shared decision-core recipe (gcp and azure copies) omitted --migration-dir, so scenarios/index.json was never read and a two-scenario Decision report with Scenario/Balanced-only headers passed. The flag is now in the recipe and documented as required on real runs in shared/validate-migration-report.md; the reference fixture remains the only case that omits it. Check 26 in shared/validate-migration-report.md now describes the populated flip list, token-width condition matching and rendered-only markup rules.
…abel and lead-in flip lists, scope conditions to the checklist The flip-list matcher only recognised a heading run followed directly by <li>s. Two legitimate shapes failed closed: the heroku-decision-gate fixtures' <li>What would flip this: …</li> (reported as 'no list') and a lead-in <p> between the heading and the <ul> (reported as '0 of N missing' although every item was rendered). The parser now tags h1-h6 runs as headings; the matcher prefers a heading, then prose, then an inline-labelled item, skips lead-in prose until the first item, bounds the search at the next heading, and treats the labelled item's remainder plus its siblings as the list. Same change ported to the Heroku validator. The conditional_go condition scan is scoped to rendered list items (the spec renders conditions[] as a checklist, and the error already says so): a one-word condition was satisfied by that word anywhere in the summary prose and could never be reported as omitted.
…on fixtures; document accepted flip-list shapes The fixtures already render 'What would flip this' in DECISION.md and the report but their estimation-infra.json omitted would_flip_if, so check_expected_decide*.py never exercised the flip check. Check 26 and the Heroku report gate now state that the label may head the list or lead its first item, and that conditions match against list items only.
…n shared validator Addresses AI review finding on PR aws#396: the GCP Decision scenario-column check (--migration-dir required for scenarios/index.json) had test coverage in the Heroku validator's test file but none in test_validate_migration_report_decision_core.py. The behavior itself was already correct (fixed by prior commits on this branch); this adds the missing regression test.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖] Reviewed 3df26238711e77afea8968186119d333e0f1b48d against 1de6d7a92f4bfc114bc74eae3587619716328402 with the complete OCR-assisted hybrid review and convergence gate.
Four original concerns are verified fixed: short conditions, Azure design-argument wiring, normalized Heroku headings, and the GCP Decision scenario-directory wiring. Two P2 content guarantees remain incomplete; details and reproductions are in the original threads:
- Distinct flip conditions: a Database trigger alone satisfies both Database and Compute entries when they share a generic phrase. Preserve each trigger’s distinguishing content; the one-item omission must fail and the complete two-item control must pass.
- Visible decision content: headline/flip content under a native
hiddenancestor still passes. Exclude hidden elements and descendants from class/text/list collection, with hidden-negative and visible-positive controls.
Both corrections affect plugins/aws-startup-advisor/scripts/validate-migration-report.py and plugins/aws-startup-advisor/skills/heroku-to-aws/scripts/validate-heroku-migration-report.py, with coverage in their corresponding report test files. Preserve the existing short-text, inline-markup, lead-in, and template/comment behavior. I missed these exact boundary cases in the earlier review; they also passed at 3428cc6, so these remain the original completeness/visibility concerns, not regressions attributed to the fixes.
Validation: 132 focused tests passed; manifest, plugin parity, specification, changed Markdown, and whitespace checks passed. Independent current-head CLI probes reproduced both remaining issues. Build and CodeQL passed; Dependency Review is cancelled. Approval remains for the user. No source changes were made.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖] Reviewed 72559b3cf4f54c205af3f5702ec60b263bc1b948 against 1de6d7a92f4bfc114bc74eae3587619716328402. The complete hybrid review and convergence gate found four P2 issues: two remaining concerns and two earlier misses.
The reported one-item flip omission and inside-summary hidden cases are fixed. Their broader requirements remain partial. Consolidated corrections:
- Distinct artifact content: preserve each flip trigger’s distinguishing content in both validators; enough Database-only bullets must not stand in for an omitted Compute trigger. Apply the same guarantee to the shared
conditionschecklist, where one Database condition currently satisfies a missing Rollback prerequisite. Complete controls must pass and omitted-trigger/prerequisite controls must fail. - Semantic HTML entries: preserve each heading/list item across
<br>and nested paragraph markup. Equivalent complete content must pass; actual missing entries must fail. Correct both parser copies. - Outer hidden context: a hidden decision-summary section or an ancestor outside it must not supply visible content. Preserve that state across extraction in both validators, retaining current inner-hidden and visible controls.
Required paths: plugins/aws-startup-advisor/scripts/validate-migration-report.py, plugins/aws-startup-advisor/skills/heroku-to-aws/scripts/validate-heroku-migration-report.py, and their corresponding report test files. The checklist-only change belongs in the shared implementation. Keep the current short-text, inline-label, lead-in, legacy-artifact, and fixture behavior.
I missed the line-break and multiple-checklist cases in the previous review; both already reproduce at 3df2623, so they are not attributed to the latest fixes. Details are in two new inline comments and the two original threads, which remain open.
Validation: 141 report/currency/cost tests passed. Manifest, plugin parity, specification, changed Markdown and whitespace checks passed. Each finding was independently reproduced with controls against the current and previous heads. No source edits, approval, or review dismissal were performed.
Problem
validate-migration-report.pytreats a migration report as complete once the section IDs, TOC, and visual shell are present.REPORT_OKis described as the sole quality gate, so an agent writes HTML to satisfy the script and never loadsreport-decision-core.md.A report can then ship without the content that file requires: the verdict headline, hero metrics, the per-track line, the conditions checklist, "What would flip this", the specialist-engagement callout, the architecture section, a risk table, and the what-if columns (Region, HA, Compute, Arch). The same gap exists on all three migration skills. GCP and Azure share the plugin-root validator. Heroku has its own copy, which checks that a what-if section exists but not that the table has those columns, and it never checks
would_flip_if.Change
Shared validator (
scripts/validate-migration-report.py), used bygcp-to-awsandazure-to-aws. New checks fire only when the passed artifacts contain the data, so a pre-extension estimate is not rejected for fields it does not have:recommendationpresent →metric-heroand, whenoutcomeis set,verdict-headlinewould_flip_ifnon-empty → a "What would flip this" listtrack_outcomespresent → a "By track" lineconditional_gowithconditions→ the checklist shares a phrase with each artifact condition (a paraphrase passes; a missing condition does not)aws_servicestarting with "Deferred" → a specialist-engagement callout in the decision summaryclusterslist →<section id="exec-architecture">exec-assumptionsfollowsexec-risks;exec-risksis a tableappendix-config, when present, also has Your choice and Source columns--no-require-tocstill skips these, which is the existing escape hatch for minimal fixtures.Heroku validator (
skills/heroku-to-aws/scripts/validate-heroku-migration-report.py): "What would flip this" whenwould_flip_ifis non-empty, and the same Region / HA / Compute / Arch / Complexity columns when the what-if section is present.Generate steps for all three skills no longer call
REPORT_OKthe sole quality gate. The HTML is still rendered from the decision-core spec; the validator checks that it was.The reference fixture gains the "What would flip this" list its own
estimation-infra-reference.jsonalready declares.Tested
python3 -m pytest plugins/aws-startup-advisor/scripts/test_validate_migration_report_decision_core.py plugins/aws-startup-advisor/skills/heroku-to-aws/scripts/test_validate_heroku_migration_report.py plugins/aws-startup-advisor/skills/heroku-to-aws/scripts/test_validate_heroku_migration_report_currency.py plugins/aws-startup-advisor/skills/heroku-to-aws/scripts/test_validate_heroku_migration_report_cost.py— 103 passed.REPORT_OKwith their estimation JSON.exec-architecturewhile the artifacts contain that data returnsREPORT_FAIL.