feat(heroku-to-aws): decision gate — decide is default, Generate is opt-in - #291
Conversation
…pt-in (P2-A)
Ports the gcp-to-aws decide-default pattern into heroku-to-aws so the decision
is the product and Generate is opt-in. Previously Heroku auto-advanced
Estimate -> Generate; now the post-Estimate gate offers [A] Done for now
(decide-complete) / [B] what-if workshop / [C] Generate, and Generate only runs
on explicit opt-in (run_mode == decide_and_execute or an explicit request).
- estimate.md: documented _advances_to exception (choice A sets current_phase:
complete instead of auto-advancing to generate)
- estimate-assemble.md: 3-option Decision Gate; workshop returns to the gate
- SKILL.md: decide-complete + resume rows; Generate-is-opt-in rule
- report-decision-core.md: shared decision-mode report renderer spec
- validate-heroku-migration-report.py: --mode {full,decision} arg + decision-mode
section rules (decision-cta instead of next-steps); +tests (13->15)
- generate-assemble.md/generate-report.md: report validator called with --mode
- run_mode added to the CANONICAL shared/state/phase-status.schema.json (both
plugins) + vendored via shared:sync — advances the shared-schema consolidation
- fixture heroku-decision-gate/after-decide-complete (decide-complete tuple:
current_phase=complete, run_mode=decide, generate=pending) + asserter
De-bundled from P2-B (artifact-inference state recovery) which stays separate.
Both plugin trees updated byte-identically (mod allowlisted namespace).
789db59 to
193d2d2
Compare
|
More fixes. 1. No resume for "gate presented, user hasn't picked A/C" (agreed — real state-machine gap)Confirmed by tracing the walk-away state (
Added the 2. Choice-A validator omitted
|
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Reviewed head 193d2d24603db0dbc85102d9b45dae53a0ec9a4b. Three P2 findings below apply to both plugin copies.
Validation: 15 report-validator tests and all 9 fixture asserters passed in each plugin; cross-plugin drift, frontmatter, vendored-shared checks, and git diff --check also passed. The findings were checked with targeted validator probes and traces of the documented DSL state transitions. No live migration was run.
…gate Resolves phase-status.schema.json conflicts (run_mode from this PR vs run_id/ owning_skill/initiated_by from awslabs#287, both additive — merged as sibling properties) in both the canonical shared/ schema and its vendored copies. generate.md / generate-assemble.md / SKILL.md merged cleanly (this PR's CONSENT GUARD composes with awslabs#287's rwx capability tier + Terraform policy gate, as anticipated in the PR description's rebase note).
Three review findings on the decision-gate PR, all in both plugin trees: 1. Handle populated run_mode when reopening the decision gate. workshop.md's Entry never reset current_phase/run_mode when a warm "what if"/"reprice" ask reopened a resolved gate (current_phase: complete, run_mode: decide or decide_and_execute) — so the retained run_mode survived workshop exit and SKILL.md's 'Gate-presented resume' rule (which requires run_mode absent to detect an unresolved gate) never fired; the interpreter fell through to current_phase and re-ran Estimate. Fixed by adding a re-entry step to workshop.md's Entry that resets current_phase to "estimate" and clears run_mode before proceeding. Also found and fixed the actual root cause of why this state could occur at all: workshop-assemble.md's "When exiting to Generate" section still unconditionally advanced current_phase straight to "generate" on exit, contradicting workshop-invariants.md § 2 (a skill that DEFINES a decision gate must re-present it, never auto-advance) and every other file in this PR (SKILL.md, estimate-assemble.md, workshop.md's own prose) which already say workshop exit returns to the gate. That file was simply never updated when the decision gate was added. Fixed to route back to the Decision gate, matching the gcp-to-aws pattern. Also fixed workshop.md's stale 'Decline without entering' section, which still referenced the old two-option Estimate offer and advanced straight to Generate. 2. Provide a decision-completion path after earlier generation. A workshop reprice after Generate had already run resets phases.generate to pending (existing _re_entry_guard) but never deleted the terraform/ and generation-*.json Generate wrote — so choosing Decision-gate option A always failed the decision-mode validator (which requires both absent), even with a perfectly valid decision-report.html, and there was no way to ever reach the decide-complete state again. Fixed workshop-refresh.md's Stale Generate guard to also delete the stale execution pack (terraform/, generation-*.json, MIGRATION_GUIDE.md, README.md, migration-report.html, report-validation-status.json) on user confirm — it's stale the moment Design/Estimate get overwritten by the reprice regardless of what the user chooses next, so removing it doesn't foreclose either later choice. 3. Validate the conditional decision-basis section. The validator required decision-basis structurally but never checked whether Estimate actually declared recommendation.decision_basis, so a report that silently dropped the section still returned REPORT_OK even with estimation-infra.json present via --migration-dir. Added a check mirroring the existing what-if-scenarios pattern: read estimation-infra.json, and if recommendation.decision_basis is present, require <section id="decision-basis">. Tests: 15 -> 18 in both trees (3 new: required-when-declared, not-required-when-absent, present-and-required-passes). Verification: pytest 18/18 both trees; cross-plugin-drift.ts OK (282 identical, 27 allowlisted); fixtures:assert PASS (9 asserters, both trees); frontmatter validator OK; dprint check clean; markdownlint 0 errors on touched files.
herosjourney
left a comment
herosjourney
left a comment
There was a problem hiding this comment.
Fixes for the three P2 findings (pushed as b3bf3de7)
All three fixed in both plugin trees, replied inline on each thread:
1. Handle populated run_mode when reopening the decision gate. Traced past the immediate symptom to the real root cause: workshop-assemble.md still unconditionally advanced current_phase straight to "generate" on exit — it was never updated when the decision gate was added, contradicting workshop-invariants.md § 2 and every other file this PR touches. Fixed the assembler to route back to the Decision gate (gcp-to-aws pattern), fixed a stale two-option reference in workshop.md's decline path, and added the mandatory re-entry reset (current_phase → "estimate", clear run_mode) to workshop.md § Entry.
2. Provide a decision-completion path after earlier generation. The Stale Generate guard reset phases.generate to pending but never removed what Generate had written, so Decision-gate choice A always failed the decision-mode validator (which requires terraform//generation-*.json absent) with no way back to decide-complete. Fixed the guard to delete the stale execution pack (terraform/, generation-*.json, MIGRATION_GUIDE.md, README.md, migration-report.html, report-validation-status.json) on user confirm — all of it is stale the moment the reprice overwrites Design/Estimate anyway.
3. Validate the conditional decision-basis section. The validator checked decision-basis structurally but never cross-referenced estimation-infra.json's recommendation.decision_basis, so a report that silently dropped the section still passed. Added a check mirroring the existing what-if-scenarios pattern; 3 new tests.
Verification (b3bf3de7, both trees)
pytest tests/test_validate_heroku_migration_report.py: 18 passed (15 → 18)cross-plugin-drift.ts: OK (282 identical, 27 allowlisted)mise run fixtures:assert: PASS (9 asserters, both trees) — including theheroku-decision-gategolden fixture, unaffectedmise run lint:frontmatter: OK (both trees)dprint check: cleanmarkdownlint-cli2: 0 errors on touched files
Also merged current main (this branch had fallen behind — main since merged #287, which touches the same generate.md/phase-status.schema.json files; resolved the schema conflicts by treating run_mode and #287's run_id/owning_skill/initiated_by as sibling additive properties).
@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 b3bf3de7641d9ffc2b592c7bbdd81ce0d225c5d3 against b30cb39be02d2eae78cb8c482f4018435d4dc988, including both plugin trees, the allowlisted state/interpreter differences, and all current discussion.
Requesting changes. The terminal decide-complete path and the ordinary missing decision-basis case are corrected, but the original threads still have substantiated follow-up cases: Generate re-entry retains old gate state, post-Generate decision completion misses cleanup paths, and the shipped commented-out basis placeholder passes validation. The new inline P1 finding covers destructive recursive cleanup of customer state and edits without deletion-specific consent or preservation.
Local validation: 18 report-validator tests and all 9 fixture asserters passed per plugin; cross-plugin drift (282 identical, 27 allowlisted), Heroku frontmatter, vendored-shared checks, and git diff --check passed. Additional real-CLI probes reproduced the remaining report failures/bypass; workflow results are traces of the documented DSL state transitions. GitHub independently reports all 9 check runs successful on this head. No deployment or live migration was run, and no customer migration outputs were deleted.
…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 a5f45b6fa1e9cb8156bc4f963a9567ca25d1be1b against ade57adaf9d776d18d316d28d3cee3f746582ced, including all four prior findings, current discussion, full relevant contracts, and both plugin mirrors.
The commented decision-basis finding is verified fixed. Requesting changes for the three remaining findings, detailed in their original threads: P1 deletion of customer edits in named generated files; P2 decision validation rejecting the Terraform directory the cleanup deliberately preserves; and P2 pending/in-progress Generate re-entry retaining the old current phase and execution consent. No duplicate inline findings were added.
Local validation: all 38 report-validator tests and 18 fixture-asserter runs passed, along with Heroku frontmatter, vendored-shared checks, cross-plugin drift (282 identical, 27 allowlisted), and git diff --check. Additional real-CLI probes confirmed the parser fix and retained-directory failure. Re-entry traces and the exact-name deletion model used isolated synthetic fixtures; no live migration, deployment, or deletion of customer outputs occurred. GitHub separately reports all 9 check runs successful on this head.
…ck presence Three new findings on the workshop re-entry / decision-gate machinery, one of which is a repeat P1 on the same file as the previous round. Root-caused all three to the same design flaw rather than patching each symptom again: the decision-mode validator's "pre-execution" check used raw filesystem presence (terraform/, generation-*.json) as its signal, when the real invariant is a STATE fact (.phase-status.json's phases.generate) that can diverge from the filesystem once a workshop reprice happens on a previously-executed run. 1. (repeat P1) Stop deleting anything Generate wrote on workshop re-entry, instead of trying to make the deletion list safer again. The previous round's fix deleted only named files (baseline.tf, variables.tf, etc.) instead of the whole terraform/ tree, but baseline.tf and variables.tf are exactly the two files generate-terraform.md tells customers to hand-edit (replace placeholder contact emails; comment out blocks to opt out of the baseline) -- they keep their generated filename after being customer-edited. _contributes: frontmatter identifies each fragment's ORIGINAL output path, not whether the path's CURRENT contents are still disposable, and no filename-based rule can tell those apart. workshop.md Entry step 2 no longer deletes anything; a prior execution pack (including customer edits to it) is left completely untouched by workshop re-entry. 2. Make the decision-mode validator's pre-execution check state-based instead of filesystem-based, which is what actually makes (1) possible without permanently blocking decision completion. Reading .phase-status.json's phases.generate value (completed/in_progress = genuinely pre-execution violated; pending or absent = fine regardless of what files exist) means a stale execution pack left over from before a workshop reprice no longer fails the decision report -- the check now reflects "has THIS decide-complete cycle gone through Generate", which is the actual contract report-decision-core.md was trying to express, not "does this directory happen to be empty." 3. Broaden workshop re-entry's current_phase/run_mode normalization to cover every state its own stale-detection guard fires on, not just the terminal current_phase == "complete" case. The guard already detected current_phase == "generate" with run_mode set and phases.generate pending or in_progress (choice C taken but Generate never completed) as stale, but the normalization step that resets current_phase/run_mode only ran for the complete-state case -- leaving those two states with a reset phases.generate but a STALE current_phase/run_mode, so a resumed session would select Generate again instead of reaching the unresolved Decision gate. Widened both the detection condition and the normalization step to cover all four states, and updated SKILL.md's Warm-start bullet to match. Verified (2) directly: 6 new unit-level cases against validate() with a real .phase-status.json, including the exact repro (stale terraform/ present, phases.generate pending -> must pass) and its inverse (phases.generate completed, no files present at all -> must still fail, since it's state that gates this, not files). Replaced the two existing tests that encoded the old file-presence contract with 4 new ones matching the corrected state-based contract -- 21/21 passing (up from 19). (1) and (3) are DSL/prose fixes with no executable test in this repo; verified by tracing the documented state transitions against both of the reviewer's exact repro states and confirming the golden heroku-workshop/heroku-decision-gate fixture asserters still pass. fixtures:assert (9 asserters), lint:frontmatter, cross-plugin- drift, dprint, markdownlint, git diff --check all clean in both plugin trees.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Re-reviewed ee6a59a98a76c06f7cb7afa23976dd715d2f7afe against ade57adaf9d776d18d316d28d3cee3f746582ced, including all prior threads and both plugin trees.
Verified fixes: workshop no longer deletes customer execution artifacts; pending/in-progress Generate re-entry clears current phase and execution consent correctly; the commented decision-basis regression remains fixed. The real decision CLI now accepts retained execution files with Generate pending and rejects completed/in-progress Generate.
Requesting changes for three P2 concerns: the report instructions/fixture verifier still enforce the old file-absence contract (follow-up in the original thread); existing corrupt state silently bypasses the new pre-execution check; and the advisor-only contextual-offers closing step was removed. The latter two have new inline comments.
Local validation: 42 report-validator tests and 18 fixture checks passed, plus frontmatter, vendored-shared checks, cross-plugin drift (283 identical, 26 allowlisted), and git diff --check. Targeted real-CLI probes reproduced the remaining state-validation and fixture-boundary failures. Re-entry results are explicit traces of the documented DSL, with synthetic execution-file hashes preserved; no live migration or deployment was run. GitHub separately reports all nine check runs successful on this head.
… check; fail closed on corrupt state; restore dropped advisor block
Three new findings on this PR:
1. Align the fixture asserter and shared renderer doc with the
state-based decision-mode contract landed in the prior commit. The
real CLI already correctly accepts phases.generate: "pending"
alongside a retained execution pack, but
fixtures/heroku-decision-gate/check_expected_decide.py still
asserted terraform/ and generation-*.json are absent by raw file
presence -- the stale pre-state-based-fix contract -- and
report-decision-core.md's own prose still said those files "do NOT
exist in decision mode" unconditionally. Removed both stale
assertions and added a sibling fixture
(heroku-decision-gate/retained-execution-pack, dispatched via a
thin check_expected_decide_with_retained_pack.py that reuses the
original asserter's exact logic) that specifically covers a valid
decide-complete run with a retained execution pack containing
customer-edited baseline.tf, hand-authored terraform.tfvars, and a
stale generation-warnings.json -- proving the golden CI path
actually exercises this case, not just the CLI in isolation.
2. Fail closed, not open, on a .phase-status.json that exists but is
corrupt. My prior fix's fail-open path treated a MISSING file and
an UNREADABLE/malformed file identically -- but
INTERPRETER.md's own State-file validation contract is explicit
that invalid JSON is a STOP condition ("do not proceed or guess"),
not evidence of anything. A truncated or empty existing state file
previously returned REPORT_OK for decision mode; it now fails with
a diagnostic pointing at that INTERPRETER.md section, while a
genuinely MISSING file (no run has ever tracked state here) still
correctly fails open.
3. Restore the advisor-only "Contextual offers (final step)" block in
advisor/plugins/aws-startup-advisor/skills/heroku-to-aws/SKILL.md,
accidentally dropped in an earlier commit on this PR. That block is
the advisor Heroku flow's mandatory closing step to check the
offers catalog and surface a qualifying partner offer -- it has no
migrate/ counterpart (migrate has no offers catalog), which is
exactly why cross-plugin-drift intentionally allowlists this file.
Restored verbatim from origin/main, verified the tail of the file
is now byte-identical to origin/main's version beyond my own
legitimate decision-gate edits earlier in the file, and confirmed
drift:check's allowlisted-difference count returns to 27 (was
dropped to 26 by the accidental removal).
Verified: (1) new GOLDEN asserter passes in both trees (10 asserters,
up from 9); confirmed it exercises a real retained-pack scenario, not
a vacuous pass. (2) added 2 regression tests (truncated JSON, empty
file) confirmed failing against the pre-fix code with the exact
REPORT_OK the reviewer reported, passing against the fix; existing
missing-file-fails-open control unchanged. 23/23 tests passing in both
trees (up from 21). (3) verified byte-for-byte against origin/main.
Full suite: fixtures:assert (10 asserters), cross-plugin-drift (282
identical, 27 allowlisted -- restored from the accidental 26), dprint,
markdownlint, lint:frontmatter, git diff --check all clean in both
trees.
…cache__ The Build check was failing on this PR (not a security check, despite the failure being flagged as such -- Bandit/Checkov/gitleaks/semgrep all passed): the fixtures:check task's gitignore scan (fixtures-check.ts) runs after fixtures:assert in the same CI job and flags ANY gitignored path present in the fixtures/ working tree, not just committed ones. check_expected_decide_with_retained_pack.py imports check_expected_decide as a module (`from check_expected_decide import main`) to reuse its assertions without duplicating them. On an interpreter that writes bytecode caches in-tree (CI's Linux Python 3.12 -- confirmed from the exact __pycache__/check_expected_decide. cpython-312.pyc path in the failure log), that import creates fixtures/heroku-decision-gate/__pycache__/, which .gitignore excludes -- so fixtures:check correctly flags it as "exists locally but will never be committed" and fails the build. This didn't reproduce on my local machine because macOS's system Python redirects sys.pycache_prefix to ~/Library/Caches/com.apple.python by default, masking the bug entirely in that environment. Verified the mechanism directly: forced sys.pycache_prefix = None (matching a standard Linux interpreter with no redirect) via a small runpy-based harness and reproduced __pycache__ being written by the pre-fix script, then confirmed it stops being written once sys.dont_write_bytecode = True is set before the import. Fixed by setting sys.dont_write_bytecode = True before importing check_expected_decide, in both plugin trees. No behavior change to the assertion logic itself -- confirmed both the retained-pack and original golden fixtures still pass via run-asserters.py (10/10 asserters), the report-validator test suite (23/23 in both trees), dprint, and git diff --check.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Completed the current-head re-review of fe5bb105e28f9e5660da1b804ca2e0b2ef757eb7 against ade57adaf9d776d18d316d28d3cee3f746582ced, including the post-e8cae60b bytecode-cache fix. All previously reported substantive findings are verified fixed; I found no new substantive issue. Approval is recommended for the human reviewer to submit.
Verified in both plugin copies: retained execution packs pass the real decision CLI and golden asserters; the original truncated/empty state-file cases fail with diagnostics; the advisor-only closing step is restored; workshop re-entry preserves customer edits and state while correctly resetting Generate consent/current phase; commented basis markup is still rejected. The isolated retained-pack wrapper also passes with python -E without writing bytecode or changing fixture files.
Local validation: 46 report-validator tests, 20 fixture-asserter runs, both fixture-integrity checks, Heroku frontmatter, vendored-shared checks, cross-plugin drift (282 identical, 27 allowlisted), and git diff --check passed. Workflow transition evidence is from explicit DSL traces and isolated synthetic fixtures, not a live migration or deployment. GitHub separately reports all nine check runs successful on this head.
This COMMENT records verification; it does not submit approval or dismiss earlier blocking reviews. Required approval and unresolved review conversations remain outstanding merge requirements.
…semble.md Non-blocking cleanup flagged in review: the parenthetical describing why --migration-dir is required for the decision-mode validator invocation still said the check verifies "no terraform/ / generation-*.json yet" -- the pre-state-based-fix framing. The actual check (fixed earlier in this PR) reads .phase-status.json's phases.generate value instead; raw file presence no longer matters, since a retained execution pack from a prior workshop-reprice cycle is expected to coexist with a fresh decision. Updated the wording in both plugin trees to describe the real check and point at report-decision-core.md for the full contract. No behavior change -- this is prose only. Verified: dprint, markdownlint, lint:frontmatter, cross-plugin-drift (282 identical, 27 allowlisted -- unchanged), fixtures:assert (10 asserters), git diff --check all clean in both trees.
Resolves a content conflict in validate-heroku-migration-report.py (both plugin trees). The two sides changed the SAME validator for independent features: - This branch (decision gate): added a `mode` parameter to validate() (`full` vs `decision`), MODE_REQUIRED_SECTION_ID, the other-mode terminal- section guard, and the .phase-status.json pre-execution check. - origin/main (PR awslabs#310 line): inserted the currency-formatting check (CENTS_RE / _RATE_SUFFIX_RE / _DecodedTextParser / _decoded_text / _validate_currency_formatting) plus its call in validate(). The features are complementary, so the resolution keeps BOTH: - Kept main's currency-formatting block verbatim. - Kept this branch's `def validate(html, migration_dir, mode="full")` signature (main's side had the older no-mode signature). - Added the missing `import re` (main's currency code needs it; this branch's import block never imported re). - The `errors.extend(_validate_currency_formatting(html))` call auto-merged cleanly into the decision-gate validate() body. - Mirrored the resolved file byte-identically across advisor/ and migrate/. Verification: heroku validator decision-gate suite 23 pass + currency suite 18 pass per tree; shared validate-migration-report suite 92 pass per tree; emit-plan-json 46 pass; drift:check OK (282 identical); fixtures:assert PASS (10 asserters); fmt:check clean; security:bandit 0 issues; lint:md 0 errors; git diff --check clean. Smoke-confirmed both features coexist: currency check fires in full mode ($25,684.89/mo), and --mode decision enforces decision-cta.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Completed the current-head review of ec8ca7cd22e87a9b1a371f1d28de363b944d3593 against ec681ba5d49b039dbf1ef9d87d3304205b824f3d, including the complete delta from the previously reviewed 7986b9c3 and both queued merges. No substantive findings remain; the approval recommendation is unchanged.
The merged currency checks work in both Heroku report modes without bypassing the decision lifecycle checks. Retained execution packs pass with Generate pending; completed/in-progress Generate and the original corrupt-state cases fail as intended. Commented decision-basis markup remains rejected. The preservation, consent/reset instructions, schemas, and fixtures are unchanged from the prior verified source. Currency helpers match the merged base, and the base-only plan writer introduces no changed lifecycle consumer in this PR. Intentional advisor/migrate differences remain intact.
Local validation passed: 266 report tests across both plugin copies, 150 real-CLI currency/lifecycle probes, 20 fixture-asserter runs, Heroku frontmatter, vendored-shared checks, cross-plugin drift (282 identical, 27 allowlisted), and git diff --check. Probes covered rendered/inert markup, monthly suffixes, block boundaries, rate operands in both orders, retained artifacts, malformed state, and combined currency/lifecycle failures. No live migration or deployment was run; no customer outputs were touched. GitHub separately reports all nine checks successful on this head.
An existing human APPROVED review is recorded on this exact head and is preserved. This is a COMMENT only; no approval, dismissal, or conversation resolution was submitted. Merge readiness remains separate because review conversations are still unresolved.
…ng-cache + phase-status conflicts) main advanced 39 commits (incl. awslabs#307 Bedrock lifecycle refresh merged as 39ca3d4, awslabs#291 heroku decision gate, awslabs#310/awslabs#314 report + plan-writer). Conflict resolution: - ai-model-lifecycle.md (gcp vendored, both trees): took OURS — same awslabs#307 facts, but at this PR's correct vendored/canonical path with the canonical header (main edited the old gcp-to-aws/references/shared/ path this PR deletes). - pricing-cache.md (gcp shared, both trees): hybrid — kept main's FRESHER lifecycle facts (Nova Canvas/Reel excluded; Nova Sonic past-EOL) but rewrote the path references from shared/ai-model-lifecycle.md -> vendored/ai/ai-model-lifecycle.md (this PR relocated the file; shared/ path would dangle). - phase-status.schema.json (shared + heroku vendored, both trees): took THEIRS (main's newer run_mode description prose); run_mode key preserved. - model-id-lint.py (both trees): updated the Haiku/Premier/Sonic allowlist paths from the deleted skills/gcp-to-aws/references/shared/ai-model-lifecycle.md to the canonical skills/shared/ai/ai-model-lifecycle.md (vendored copies inherit via canonicalize()) — main's linter didn't know this PR moved the file. Post-merge shared:sync re-propagated the shared phase-status schema into the azure vendored copy (canonical-staleness trap). Verified green both trees: shared:check OK, drift:check OK (398 identical), frontmatter 7 files, asserters 15 PASS, 92 validator unit tests pass, mise run build all 16 tasks clean (lint:model-ids OK, fmt clean, lint:md 0 errors, all security scanners clean). All six ai-model-lifecycle.md copies byte-identical.
2 participants
Problem
When someone runs a migration, the thing they actually want first is the decision: should I move to AWS, and what will it cost? Generating all the Terraform and migration scripts is the second step — and only worth doing once they've decided to go ahead.
gcp-to-awsalready works this way: after it estimates costs, it stops and shows a decision summary, and it only generates the infrastructure files if the user explicitly asks.heroku-to-awsdid the opposite — as soon as it finished estimating, it automatically barrelled ahead into generating everything, with no "do you want to proceed?" step.That's a problem for two reasons:
In code, for reviewers
gcp-to-awsstops at a Decision Gate and recordsrun_mode: "decide"+current_phase: "complete"(withphases.generate: "pending"), entering Generate only on explicit opt-in.heroku-to-awsinstead declared_advances_to: generateand its post-Estimate offer was only[A] workshop / [B] proceed toward Generate, with B settingcurrent_phase → generateunconditionally — norun_mode, no decision pack, no opt-in.Solution
Port the gcp decide-default into heroku-to-aws (translated to the DSL idiom):
estimate-assemble.md— the post-Estimate offer becomes a 3-option Decision Gate:[A] Done for now(decide-complete) /[B] Explore what-ifs(existing workshop sidebar) /[C] Generate Terraform and migration scripts. The workshop returns to the gate rather than falling through to Generate. A decide-complete resume offer handles the warm-start case.estimate.md— documents the_advances_to: generateexception: choice A overrides the default next-phase by settingcurrent_phase: "complete"(leavingphases.generate: "pending"), the same class of documented exception as the inner-workshop-repriceHANDOFF_OKskip. Choice C follows_advances_tonormally.generate.md— a CONSENT GUARD at the top of the phase (mirroring gcp): Generate runs only whenrun_mode == "decide_and_execute"or the current-turn message is an explicit Execute request (which setsrun_modefirst); otherwise it STOPs and re-presents the gate. Opt-in is enforced at the execution point, not just in SKILL.md prose.SKILL.md— decide-complete + resume rows and the "Generate is opt-in (HARD RULE)".report-decision-core.md+validate-heroku-migration-report.py— a shared decision-mode report renderer spec and a new--mode {full,decision}arg so the decision pack (decision-report.html+DECISION.md) validates in decision mode (decision-ctainstead ofnext-steps). Tests 13 → 15.run_modeadded to the canonicalshared/state/phase-status.schema.json(both plugins) + vendored viashared:sync— an optional enum["decide","decide_and_execute"]. This also advances the shared-schema consolidation (previouslyrun_modelived only in gcp's private prose schema).heroku-decision-gate/after-decide-complete(the decide-complete tuple:current_phase=complete,run_mode=decide,generate=pending) + asserter.Cross-skill payoff: Heroku now produces the same decide-complete tuple +
DECISION.mdthatllm-to-bedrock's Assess handoff (#290) keys on — so the AI-path handoff works identically regardless of source cloud, and Azure copies one decision-flow story.Scope: decision gate +
run_mode+ opt-in entry only. Deliberately de-bundled from P2-B (artifact-inference state recovery), which stays a separate PR — it modifies the shared canonicalINTERPRETER.mdand is an independent concern.Type of Change
Team Folder
advisor/migrate/solution-architecture/Verification
cross-plugin-drift.ts: OK (274 identical, 25 allowlisted — twins byte-identical mod the allowlisted namespace)test_validate_heroku_migration_report.py(both trees): 15 passed each (13 → 15; adds the--mode decisionsection rules)mise run fixtures:assert, both trees): PASS — 9 asserters, incl. the new goldenheroku-decision-gate/check_expected_decide.pyrun_mode=decide,current_phase=complete,generate=pending,DECISION.mdpresent; choice C →run_mode=decide_and_execute,current_phase=generate; workshop returns to the gatefrontmatter-validator/markdownlint/dprint: clean (both trees)mise run build(full composite incl.bandit/semgrep/gitleaks/checkov/grype): PASS, exit 0 against a fresh detached worktree at this commit. checkov 276/0; gitleaks no leaks; grype no vulnerabilities.Not run: no live cross-skill run; state transitions verified against constructed
.phase-status.jsonfixtures.Merge order (please read)
This PR restructures
heroku-to-awsgenerate.md/generate-assemble.md/SKILL.mdand adds a--modearg tovalidate-heroku-migration-report.py. The open PRs #287 (tf-policy-gate) and #288 (report-validation blocking) also touch those same files. This PR is the more foundational change (it establishes the decide-vs-execute flow those two refine), so please merge this before #287/#288 — those two should then rebase onto this. The overlap is small: #288 touches the same report-validator invocation line (the--mode fullargument) and both edit the Generate completion region. Verified P2-A does not depend on anything in #287/#288.Rebase note for whoever rebases #287 onto this: this PR adds a CONSENT GUARD at the top of
generate.mdand a "main-window exception" carve-out in its Scope Boundary. #287 (on its pre-P2-A base) also adds a Scope-Boundary "main-window exception" carve-out (for the tf policy reconcile) and its own main-window "Finish Generate" section. These compose fine logically — the consent guard runs first, then the policy reconcile, then the read-only gate — but on rebase, keep this PR's CONSENT GUARD (do not revert it) and merge the two Scope-Boundary carve-outs into one paragraph rather than leaving two duplicate "main-window exception" notes.Checklist
mise run buildlocally and it passes — verified against a fresh worktree at this branch's commit (see Verification)advisor/+migrate/migration skills)