feat(heroku-to-aws): clarify extraction + assumption sheet, decision outcomes, breadcrumbs (gcp parity) - #180
Conversation
…outcomes, breadcrumbs — parity with gcp-to-aws
mise run build failed at fmt:check on two clarify/estimate tables. Content unchanged (git diff -w shows only separator-row widths).
There was a problem hiding this comment.
[🤖 AI review 🤖]
Review: PR #180 — feat(heroku-to-aws): clarify extraction + assumption sheet, decision outcomes, breadcrumbs (gcp parity)
Scope reviewed: All 7 changed files (clarify-assemble, clarify-interview, design-assemble, estimate-assemble, estimate-cost-engine, estimate.md phase def, generate-assemble). Cross-referenced against the GCP estimate-infra.md decision-outcome schema for parity. Checked sibling PRs #181–186 for file conflicts.
Summary
This is a well-structured parity back-port: extraction rules, assumption sheet gate, additive outcome enum, breadcrumbs, and de-biased "stay" path — all adapted to Heroku's flat-preferences schema and fragment DSL rather than blindly copied. The design decisions are sound (tier-derived = signal not requirement, rarity guard on defer, lead-with-what-IS-established). Schema extension is additive with proper backward-compat note. Hard trigger reduction from 3 (GCP) to 2 (Heroku) is correctly justified (no BigQuery-class specialist gate). One substantive finding, one notable discrepancy vs. description, and two nits below.
Findings
1. Optional: PR description claims generate-report.md renders outcome_label — but no change to that file exists in this diff or on the branch.
The description says: "report renders the outcome as a typography-first headline (no colored pill badges)" — but generate-report.md on this branch still contains zero references to outcome, outcome_label, decision_basis, or would_flip_if. The decision-summary section spec (line 52) still just says "Verdict / path label, complexity, one-sentence next action."
This means the new schema fields are written to estimation-infra.json but never consumed in the report. Either: (a) the report rendering belongs in a follow-up PR (should be stated in "Out of scope"), or (b) it was accidentally left out of this PR.
2. Nit: breadcrumb says "Phase 2 of 6" but the system has 5 user-visible phases + 1 optional (Feedback).
The breadcrumb in clarify-assemble.md says "Phase 2 of 6 complete" and counts Feedback as phase 6. This is reasonable but may confuse users since Feedback is explicitly optional and the estimate offer text says "Phase 4 of 6… optional Feedback." Consistent, but calling the total "6" when one is optional is a UX choice worth noting.
3. Nit: clarify-assemble.md schema example shows "inventory_clarifications": {} (empty object) but the text describes its populated shape (e.g. {"database_ha": "plan:premium-0"}).
Minor — the example is for the schema skeleton, and populated examples live in the extraction rules table. Fine as-is.
Merge interactions with #181–186
- #181–184 are all
gcp-to-awschanges with zero heroku file overlap. No conflict. - #185 and #186 both touch
heroku-to-aws/references/vendored/workshop/workshop-invariants.md(shared file) — modifying the workshop exit logic to support a "post-Estimate decision gate." This PR does NOT touch that file, so no textual conflict. However, theestimate-assemble.mdworkshop offer text in THIS PR ("Before Generate, want to see…") assumes workshop exits directly to Generate — which #185/#186 change to re-present a decision gate instead. Merge order: #180 should land before #185/#186 (or simultaneously), since #185/#186 expect theoutcomeschema this PR introduces. If #185/#186 land first, the workshop-invariants text would reference a decision gate whose schema doesn't exist yet.
Merge recommendation
✅ Recommend merge once the generate-report.md gap is acknowledged (follow-up is fine — just add it to "Out of scope" or file it as a noted follow-up). The code health improvement is clear: extraction de-duplicates user effort, assumption sheets give transparency, de-biased stay path removes an artificial gate, and the schema extension is additive with backward compat.
mergeStateStatus: BEHIND — needs rebase onto main before merge (no conflicts expected given file scope).
CI: No status checks configured on this repo. Tests reported passing in PR description (57 pass + 81 pytest + 10 asserters).
Merge order: #180 → #185 → #186 (outcome schema must exist before decision-gate PRs land).
| **Presenting a defer (lead with what IS established):** open with what the assessment did determine — "AWS can host this stack; the AWS-side estimate is $X–$Y/mo" — before naming the one missing piece of evidence and how to obtain it. Never present defer as "no answer." | ||
|
|
||
| ### Persist recommendation to estimation-infra.json | ||
|
|
There was a problem hiding this comment.
[🤖 AI review 🤖]
Optional: The PR description states: "report renders the outcome as a typography-first headline (no colored pill badges, meaning never carried by color alone) with an end-of-summary decision-basis block" — but generate-report.md is unchanged on this branch and still has no reference to outcome, outcome_label, decision_basis, or would_flip_if.
The new schema fields are written here but never consumed in report rendering. Is this intentional for a follow-up (if so, suggest adding to "Out of scope"), or was it accidentally omitted from this PR?
Severity: worth acknowledging — the report currently won't surface the new decision vocabulary to stakeholders reading the HTML output.
Close the generate-report gap called out in review: consume the v2 outcome / decision_basis / would_flip_if fields with a typography-first verdict (no color-only pills). Lead the Estimate chat recommendation with outcome_label, and show a populated inventory_clarifications example in the clarify schema skeleton. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the review findings: 1. 2. Phase N of 6: Left as-is — same backbone+optional-Feedback convention as gcp-to-aws breadcrumbs; the "optional Feedback" wording in the Remaining line already flags optionality. 3. |
2 participants
Summary
The problem, in plain English: The same plugin now treats a GCP user and a Heroku user very differently. A GCP user gets shown what their Terraform already answered before being asked anything, sees every assumption with its cost consequence, gets a verdict that can honestly say "go, once you confirm X" — and always knows which phase they're in. A Heroku user gets none of that: the interview asks about database HA even though the Postgres plan tier in their own inventory already says whether an HA follower exists, applies defaults silently, can only recommend "stay on Heroku" if cost is literally the customer's only concern, and hands back a verdict with no stated basis. This PR back-ports the gcp-to-aws usability improvements (merged as #170–#176) to heroku-to-aws, adapted to its flat preferences schema and fragment DSL — it does not blindly copy the GCP text.
Changes (all under
skills/heroku-to-aws/; Vercel and GCP untouched, no canonicalskills/shared/edits):clarify-interview.md): answer fromheroku-resource-inventory.jsonbefore asking — Q1 region from Private Space region (detected) or Common Runtimeus/eu(proposed suggestion only), Q6 database HA from Postgres plan tier (standard-*= no follower,premium-*/private-*/shield-*= HA), Q7 Redis HA from plan tier, Q12b containerization from app stack. With an honesty rule the GCP flow didn't need: plan tier says what the customer HAS, not what they NEED — tier-derived rows show their source so users can correct target posture vs current posture.clarify-interview.md): detected + assumed tables with consequence lines, numbered menu actions (Confirm all / Change a setting / Ask about X / Ask everything), plain-language corrections accepted, structured-question-tool note with chat fallback. Batches then contain only unresolved questions, capped at 4 per turn with progress lines.estimate-cost-engine.mdPart 8,estimate.mdasserts): additiverecommendation.outcome(go | conditional_go | defer_for_evidence | stay) beside the untouchedpath. Trigger table adapted to Heroku: hard triggers are GovCloud-class compliance ambiguity and cost-only motivation with zero spend signal (nobilling_profile, spend declined) — there is no BigQuery-analog specialist gate here, so only two hard triggers, and the rarity guard applies ("defer is expected to be RARE; when in doubt, conditional_go"). Soft triggers: tier-derived/defaulted HA, defaulted migration approach on a large database, stale pricing. "Stay on Heroku" de-biased — reachable on any decisive factor, not only "cost is the sole metric." Defer must lead with what IS established.estimate-assemble.md,generate-report.md): chat verdict leads withoutcome_label, conditions listed,would_flip_ifcloses; report renders the outcome as a typography-first headline (no colored pill badges, meaning never carried by color alone) with an optionaldecision-basisblock (measured / assumed / unknown) when v2 fields exist.clarify-assemble.md,design-assemble.md,estimate-assemble.md,generate-assemble.md): "Phase N of 6 complete — Remaining: …" onHANDOFF_OKpaths only. Workshop offer rewritten benefit-first with a data-justified scenario hint (e.g. tier-derived HA → suggest single-AZ comparison).clarify-assemble.md):metadata.questions_skipped_extracted+metadata.inventory_clarificationsadded;sourcesgains"extracted". Flat preferences shape preserved — no wrapped-constraint migration.Why
Post-series re-review found cross-skill parity was the largest remaining usability gap: heroku's interview re-asks what its own inventory knows, and its recommendation vocabulary predates the honesty improvements. Follows the same review-hardened decisions as the GCP series: additive outcome enum, tiered defer triggers with rarity guard, assumptions at the end of the report summary, no color-only meaning.
Test plan
mise run lint:md— 0 errors (855 files)mise run lint:frontmatter— OK (10 phase files; heroku DSL asserts validated, including the new outcome assert)mise run shared:check— OK (no canonical/vendored edits)mise run test— 57 pass, 0 failpython3 -m pytest tests/— 81 passed;tools/run-asserters.py— PASS (10 asserters incl. heroku workshop)outcome_labelheadline + decision-basis when v2 fields presentOut of scope