Skip to content

feat(heroku-to-aws): wire tf-best-practices policy gate into Generate - #287

Merged
icarthick merged 9 commits into
awslabs:mainfrom
herosjourney:feat/heroku-tf-policy-gate
Sep 17, 2026
Merged

icarthick merged 9 commits into
awslabs:mainfrom
herosjourney:feat/heroku-tf-policy-gate

Conversation

@herosjourney

@herosjourney herosjourney commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Problem

heroku-to-aws Generate emits Terraform but never runs it through the tf-best-practices policy
gate. gcp-to-aws has done this since its Generate phase shipped — before writing it invokes the
skill for authoring posture (Step 3.0), and after writing it runs fmt → init → validate → policy,
records the verdict in validation-report.json, and blocks Phase Completion on an unresolved
POLICY_FAIL. Heroku had no equivalent, so a Heroku user could generate Terraform with, say,
unencrypted ElastiCache or a plaintext-forwarding ALB and complete Generate with no signal.

#261 closed the largest security-parity gap (the baseline.tf account baseline) and its fail-closed
existence gate; it explicitly deferred the policy-gate wiring.

Solution

Ports the gcp-to-aws two-touchpoint pattern into heroku-to-aws, translated into the DSL idiom:

gcp-to-aws (prose) heroku-to-aws (this PR)
Step 3.0 authoring-posture invoke New Step 0 in generate-terraform.md (before writing; pass normalized compliance + aws_config)
Step 6 policy gate + validation-report.json Step 12 writes a best-effort validation-report.json in the v2 envelope; a main-window "Finish Generate" step runs the authoritative policy check + .tf fix-and-retry before the read-only gate (see "Where the gate actually runs")
Phase Completion prose gate validation-report.json in _produces, _check_file_exists, _validate_json, plus a read-only policy_status _assert in generate.md _postconditions
— generate-assemble.md states the gate is required, is read-only, and halts on failure (recovery = human edit + targeted re-run, not full Generate re-run)

Where the gate actually runs (important — corrects an earlier claim)

The Generate phase is dispatched under _exec._agent: rw, and per INTERPRETER.md the rw tier
excludes shell/Bash — a dispatched worker has file-only I/O and cannot run terraform or
validate-terraform-policy.py. So the work is split into three clearly-owned steps:

  • Worker (Step 12, dispatched, no shell): only writes a best-effort validation-report.json
    (status: "passed_degraded_offline", policy_status: "not_run") — it never self-certifies
    POLICY_OK.
  • "Finish Generate" (main window, BEFORE the gate): the interpreter finishes the leftover
    Generate work the shell-less worker could not do — runs validate-terraform-policy.py against
    $MIGRATION_DIR/terraform, applies the budget-3 .tf fix-and-retry loop on POLICY_FAIL, then
    merges the final verdict into validation-report.json. This is the only place .tf is edited
    for policy.
    status stays passed_degraded_offline (the main window runs only the policy
    stage, never fmt/init/validate); only policy_status / policy_violations[] change. The
    --json sidecar (validation-report.policy.json) is a temp merge input, deleted after merge.
  • _postconditions gate (main window, READ-ONLY): _validate_json + an _assert that
    policy_status == POLICY_OK. It runs no checker and edits nothing (per INTERPRETER.md
    § _postconditions, a gate never mutates artifacts to pass). A residual POLICY_FAIL or
    not_run fails closed → GATE_FAIL.

On inline-only hosts the whole phase runs in the main window, so the same finish-then-gate
sequence applies.

GATE_FAIL recovery: the retry budget is already spent in the finish step, so recovery is a
human edit of the named .tf sites + a targeted re-run of the checker — not a full Generate
re-run (re-dispatching re-authors terraform/ under the shell-less worker and would wipe manual
fixes).

Honest scoping of the guarantee: the file-existence + JSON-validity checks are mechanical
(_check_file_exists / _validate_json); policy_status == POLICY_OK is enforced by the
main-window re-run of the script in the finish step (authoritative), backed by a read-only
_assert. It is not a
CI-verified property of the artifact alone.

v2 schema

validation-report.json uses the real v2 envelope from references/terraform-validation.md:
$schema: "validation-report/v2", status ∈ {passed, passed_degraded_offline, …}, policy_status
∈ {POLICY_OK, POLICY_FAIL, not_run}, offline_fallback_used (bool), policy_violations[]. (An
earlier revision used a non-existent passed_degraded_offline boolean field — fixed.)

EB ALB caveat (static gate limitation, documented)

validate-terraform-policy.py inspects standalone aws_lb_listener blocks. An EB LoadBalanced
environment provisions its ALB from aws_elastic_beanstalk_environment setting blocks, which the
static checker does not read — so a pure-EB design passes the ALB rules vacuously. This is a
documented limitation (EB listener/TLS posture is authoring-only), noted in tf-best-practices/SKILL.md.

No change to the policy engine

validate-terraform-policy.py is source-agnostic and reused unchanged. No new rules. No baseline.tf
changes (#261 owns that).

Fixtures

Three Heroku-shaped POLICY_OK fixtures + one POLICY_FAIL, each with a matching test:

  • good-heroku-eb-loadbalanced — EB compute with a standalone ALB (real aws_lb_listener blocks
    the gate inspects) + RDS.
  • good-heroku-eb-only — pure EB LoadBalanced, no aws_lb — passes the ALB rules vacuously
    (documents the caveat above).
  • good-heroku-eb-singleinstance — public-subnet instance, 80/443 to the world, no ALB → POLICY_OK
    (web ports aren't flagged; only admin/datastore ports are).
  • bad-heroku-eb-elasticache — unencrypted ElastiCache + HTTP-forward ALB → POLICY_FAIL; the test
    asserts both rule IDs (elasticache_encryption_at_rest, alb_http_redirect) via --json.

Type of Change

  • New plugin/power/tool
  • Bug fix
  • Enhancement to existing content
  • Guardrail/CI update

Team Folder

  • advisor/
  • migrate/
  • solution-architecture/
  • Other: ___

Verification

  • cross-plugin-drift.ts: OK (279 identical, 25 allowlisted — no new drift)
  • test_validate_terraform_policy.py (both trees): 54 passed (2 new EB fixtures + the two-rule bad-fixture assertion)
  • frontmatter-validator (both trees): OK; fixtures-check.ts: OK both trees
  • markdownlint-cli2: 0 errors; dprint check: clean; tsc --noEmit: OK both trees
  • Fixture verdicts: good-heroku-eb-{loadbalanced,only,singleinstance} → exit 0; bad-heroku-eb-elasticache → exit 1
  • mise run build (full composite — lint, fmt:check, security incl. bandit/semgrep/gitleaks/checkov/grype): PASS, exit 0 — run against a fresh detached worktree at this branch's commit (not my working checkout, which has an untracked, unrelated migration-watchdog/ directory that fails an unrelated markdownlint check). checkov 276/0; gitleaks no leaks; grype no vulnerabilities.

Not run locally: no live AWS / no terraform apply — the policy stage is a dependency-free static
HCL reader; terraform init/validate degrade to passed_degraded_offline while the policy verdict
is recorded independently.

Checklist

  • I have read the CONTRIBUTING.md guidelines
  • My changes do not include hardcoded secrets, credentials, or internal-only content
  • I have run mise run build locally and it passes — verified against a fresh worktree at this branch's commit (see Verification)
  • I have updated documentation if needed (generate-*.md wiring, SKILL.md EB caveat, generated README artifact row)
  • My changes are scoped to my team's folder only (advisor/ + migrate/ migration skills)

Update — validation now runs in-fragment via a new rwx tier (supersedes the main-window reconcile above)

This update replaces the "Where the gate actually runs" mechanism described earlier in this PR.
The three-actor split (worker writes not_run → main-window "Finish Generate" reconcile → read-only gate) is removed. Pushed with the original author's sign-off.

Why the change

The earlier approach existed only to work around one constraint: Generate is dispatched at _exec._agent: rw, and the rw worker has no shell, so it couldn't run validate-terraform-policy.py — forcing the checker into main-window prose. But gcp-to-aws (the reference implementation this PR ports from) runs its Generate phase inline and validates in-fragment, because an inline phase has a shell. Heroku's difficulty was self-inflicted by dispatching Generate at rw.

Rather than un-dispatch Generate (losing _exec's context isolation) or smear enforcement across main-window prose the DSL can't see, this update closes the actual gap in the tier vocabulary.

What changed

  • New capability tier rwx (INTERPRETER.md, shared + vendored to all skills): rw + a scoped shell whose sole sanctioned use is running the read-only tf-best-practices policy checker (python3/uvx). It is not a git tier; where the host supports command scoping (e.g. Bash(python3:*)), it should be restricted, and the worker prompt binds that scope otherwise. Added to the closed tier vocab {ro, rw, rwx, git}, the frontmatter-validator enum, and the docs (02, 05).
  • New worker agents/generic-phase-worker-rwx.md (both trees): tools: Read, Grep, Glob, Write, Edit, Bash, with strong in-prompt scoping (checker only; no git, no network, no arbitrary shell).
  • generate.md: _exec._agent: rw → rwx; the "Finish Generate in the main window (policy reconcile)" section is deleted; the read-only _postconditions assert (policy_status == POLICY_OK, fail-closed) is unchanged.
  • generate-terraform.md Step 12: the dispatched rwx worker now runs the checker + budget-3 fix-and-retry in-fragment and writes the real verdict (no not_run placeholder), then deletes the temp --json verdict sidecar after merge.
  • generate-assemble.md: prose updated — the gate stays read-only; validation happened in-fragment.
  • discover stays rw (no shell): it is pure file work over untrusted repo input, exactly where a shell is least welcome.

DSL-alignment notes

  • Fragments still cannot carry _exec and validation is not a new phase — rwx keeps validation cohesive inside the Generate fragment where it belongs (immediately after the TF is authored), with no phase-count change and no downstream state/fixture churn.
  • Platform asymmetry: on inline-only hosts (Codex/Cursor, headless claude -p) there is no sub-agent, so the tier is inert and Generate runs inline with a shell anyway — the checker still runs. rwx is a least-privilege intent enforced where the harness can (interactive Claude Code), not a portable guarantee.

Verification

  • mise run build: PASS, exit 0 (markdownlint 0, dprint clean, checkov 276/0, gitleaks no leaks, grype clean) against a fresh worktree at this commit.
  • frontmatter-validator (both trees): OK; 63/63 validator unit tests (adds an rwx acceptance case).
  • cross-plugin-drift.ts: OK (282 identical, 25 allowlisted); shared:sync check: OK (vendored INTERPRETER.md copies re-synced).
  • test_validate_terraform_policy.py: 54 passed; heroku fixture asserters 8/8 (both trees).
  • Live end-to-end (interactive Claude Code): the dispatched rwx worker ran the checker in-fragment — POLICY_OK | checks=… EXIT=0 — and wrote validation-report.json. This exercised the real dispatched-worker path, not just the inline fallback.
  • Negative control: the checker returns POLICY_FAIL (exit 1) on a known-bad config (elasticache_encryption_at_rest, alb_http_redirect, alb_https_listener, correct file/line), and generate.md _postconditions asserts POLICY_OK else fail-closed — a FAIL cannot advance the phase. (Forcing the skill to emit bad TF and observe a live GATE_FAIL is not reachable — the skill authors clean TF by construction — so the gate side is verified by contract inspection plus the checker's own FAIL behavior.)

Known follow-ups

  • Touches the shared, vendored INTERPRETER.md — coordinate merge order with feat(heroku-to-aws): decision gate — decide is default, Generate is opt-in #291 (also modifies shared state / run_mode).
  • Observability gap (pre-existing, not introduced here): the gate asserts a value (policy_status == POLICY_OK), not an execution receipt. A future hardening would have the checker stamp a tamper-evident receipt (e.g. checker_version + a hash of the scanned .tf) that the _postcondition verifies, so a hand-written POLICY_OK could not pass.

@herosjourney
herosjourney requested review from a team as code owners September 12, 2026 21:32
@herosjourney
herosjourney force-pushed the feat/heroku-tf-policy-gate branch from 70cd8a7 to c0c6bf8 Compare September 12, 2026 22:01
@herosjourney

herosjourney commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Follow-up review + fixes (pushed as 0293584)

After the initial review I ran another review pass over this PR and found the gate design had real defects — a policy gate that could not actually execute, plus schema/wiring mistakes. All are now fixed. Structured by what the review found and how it was addressed:

Blocking issues found

1. The policy gate could not run where it was specified. Generate is dispatched under _exec._agent: rw, and INTERPRETER.md says rw excludes shell/Bash — so the worker I told to run terraform fmt/init/validate + validate-terraform-policy.py had no shell to do it, and an LLM-judged _assert over a worker-written policy_status was game-able (nothing stopped {"policy_status":"POLICY_OK"}).
→ Fix: the worker now only writes a best-effort validation-report.json (status: passed_degraded_offline, policy_status: not_run) and never self-certifies. The main-window completion gate (_postconditions, which per § _exec step 4 always runs in the main window — the only place with a shell) re-runs the policy checker, reconciles the real verdict, runs fix-and-retry, and GATE_FAILs on failure. Added _validate_json.

2. The completion gate told itself to repair .tf files. INTERPRETER.md § _postconditions forbids a gate from modifying artifacts, updating state, or advancing.
→ Fix: generate-assemble.md now halts and tells the user to re-run Generate. The .tf fix-and-retry loop lives only in the Terraform execution step, never at the gate.

3. validation-report.json used a field the v2 schema doesn't define (passed_degraded_offline as a boolean).
→ Fix: uses the real v2 envelope — status: "passed_degraded_offline" (a status value) + offline_fallback_used (bool) + the full field set ($schema, policy_status, policy_violations, …).

Should-fix issues found

4. The "good" EB fixture wasn't what EB emits, and the static gate can't see EB-managed ALBs. An EB LoadBalanced env provisions its ALB from aws_elastic_beanstalk_environment setting blocks; the checker only reads standalone aws_lb_listener, so a pure-EB design passes the ALB rules vacuously.
→ Fix: corrected the fixture comment; added good-heroku-eb-only (pure EB, no aws_lb — documents the vacuous pass) and good-heroku-eb-singleinstance (public-subnet, 80/443 open → POLICY_OK, web ports aren't flagged); documented the "EB ALBs are invisible to the static gate (authoring-only)" limitation in tf-best-practices/SKILL.md. A mutation probe confirms the vacuous pass is only because no listener exists — adding a bad standalone listener to eb-only flips it to POLICY_FAIL.

5. The bad-fixture test only asserted one of two violations.
→ Fix: it now parses --json and asserts both rule IDs (elasticache_encryption_at_rest and alb_http_redirect), so an alb_http_redirect regression can't hide.

6. The new artifact was missing from user-facing surfaces.
→ Fix: validation-report.json added to the generated README artifact table, the HANDOFF_OK list, and the generate.md Orientation.

Residual design nit found on re-review (also fixed)

7. The policy_status _assert was overloaded with an action. _assert is defined as a predicate the interpreter evaluates, but I'd written "run the script and reconcile the file" into the assert string.
→ Fix: split it — the _assert is now a pure predicate about the reconciled artifact ($schema v2 + policy_status == POLICY_OK), and the action (run checker → merge verdict → retry) lives in a dedicated "Completion gate (main window)" prose section the interpreter executes before evaluating the predicate. Also made no-shell hosts fail closed (policy_status stays not_run → the assert fails rather than certifying an un-run pass).

Verification (0293584, fresh detached worktree)

mise run build PASS (checkov 276/0, gitleaks no leaks, grype no vulns), cross-plugin-drift.ts OK (279 identical), test_validate_terraform_policy.py 54 passed both trees, all touched files twin-identical, EB fixtures verified (3 good → exit 0, bad → exit 1, mutation probe confirms the caveat). Not run locally: terraform apply (no live AWS) — the policy stage is a dependency-free static reader and runs offline.

@herosjourney
herosjourney force-pushed the feat/heroku-tf-policy-gate branch 2 times, most recently from 0293584 to f2afa2f Compare September 13, 2026 00:25
@herosjourney

herosjourney commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor Author

Verified additional issues against the interpreter model before changing anything; all four are now addressed in f2afa2f (both plugin trees, byte-identical). Structured as finding → what I changed.

1. Should-fix — retry ownership contradicted itself (agreed)

Confirmed: the three files told different stories, and putting the .tf fix-and-retry "as part of the _postconditions gate" violated INTERPRETER.md § _postconditions (a gate must not mutate artifacts to pass) — the same defect as the assembler version, just relocated. Adopted your split verbatim:

  • generate.md — renamed the section to "Finish Generate in the main window (policy reconcile) — BEFORE the gate." The checker run, the budget-3 .tf loop, and the verdict merge now live there, framed as leftover Generate work the shell-less rw worker could not do. This is the only place .tf is edited for policy.
  • _postconditions is now strictly read-only: _validate_json + an _assert that policy_status == POLICY_OK. The _assert text explicitly forbids running the checker, editing .tf, or editing the verdict.
  • generate-terraform.md Step 12 — the worker only writes the not_run placeholder and points at the "Finish Generate" step for authoritative execution.
  • generate-assemble.md — dropped "unresolved after the Step 12 fix-and-retry budget" and "re-run Generate so Step 12 can apply fix_hints." It now says the finish step already spent the budget; recovery is a human edit of the named .tf sites + a targeted re-run of validate-terraform-policy.py, not a full Generate re-run (re-dispatch re-authors terraform/ under the shell-less worker and wipes manual fixes — your point exactly).

2. Should-fix — status: "passed" without running fmt/init/validate (agreed)

Right — the main window only runs the policy stage; fmt/init/validate never execute there, so status: "passed" + offline_fallback_used: false would be a lie. The finish step now keeps status: "passed_degraded_offline" and offline_fallback_used: true, and only policy_status / policy_violations[] change on the policy re-run — matching the v2 rule that the policy verdict is recorded independently of the offline path.

3. Nit — Scope Boundary fought the Completion gate (agreed)

Carved the main-window policy reconcile out of the "Nothing else" forbid-list: the Scope Boundary now states that running the policy checker and applying its fix_hints to the generated terraform/ is part of producing the artifacts, so the "Finish Generate" step is in scope (design/estimate decisions stay final). An interpreter obeying the ALL-CAPS line no longer skips it.

4. Nit — leftover validation-report.policy.json (agreed)

Documented the --json output as a temp sidecar, merged then deleted in the finish step — not a second verdict and not in _produces.

Verification (f2afa2f, both trees)

  • cross-plugin-drift.ts: OK (279 identical, 25 allowlisted)
  • test_validate_terraform_policy.py: 54 passed
  • mise run build against a fresh detached worktree at this commit: PASS, exit 0 (checkov 276/0, gitleaks no leaks, grype no vulnerabilities, fail 0 across DSL suites)
  • Fail-closed property preserved: worker leaves not_run; if the finish step can't run (no shell) policy_status stays not_run and the read-only _assert fails closed.

No policy-engine changes; the split is entirely in the phase prose + the _assert.

herosjourney pushed a commit to herosjourney/startups that referenced this pull request Sep 13, 2026
The heroku-to-aws report validator was optional/non-blocking with zero tests,
and generate-report.md contradicted generate-assemble.md on whether it gates.
This makes it a required, blocking gate and hardens it to the report's own
decision-UX + accessibility contract (ported subset of the gcp-to-aws awslabs#223
checks, scoped to what the thin Heroku one-pager emits).

Round-2 review (host-placement): the validator invocation was in the dispatched
assembler/fragment (no shell under _exec: rw), so 'blocking' was prose on a path
that can't execute. Mirrors the awslabs#287 split: the python3 validator run + a
REPORT_OK assert now live in a main-window 'Finish Generate (report validation)'
step in generate.md that runs BEFORE the read-only _postconditions gate. On
REPORT_FAIL the step emits GATE_FAIL and pastes the validator errors[] verbatim
(so recovery is actionable, not a blind 're-run Generate'). The gate is
read-only; the assembler no longer claims to be 'the main window'.

- validate-heroku-migration-report.py: require cost-optimization; forbid
  badge-verdict-* pills; require verdict-headline when recommendation.outcome
  present; a11y subset (html lang, th scope, figure aria-label+figcaption)
- generate.md: 'Finish Generate' main-window validator step + read-only REPORT_OK
  _assert; Scope-Boundary carve-out for the main-window validate
- generate-report.md: points at the finish step; GATE_FAIL pastes errors[];
  skeleton gains a <th scope=col> example so agents copy the a11y pattern
- generate-assemble.md: read-only gate, no validator call, recovery = hand-edit
  + targeted re-run (not full Generate re-dispatch, which wipes the fix)
- tests: 13 -> 14 cases (add figure-with-aria-label-without-figcaption FAIL)
Deliberately not ported (would false-fail the thin report): decision-before-TOC,
single-h1, per-table caption, glossary/appendix set. Both plugin trees updated
byte-identically.
Ports the gcp-to-aws two-touchpoint pattern into heroku-to-aws, translated
into the DSL idiom: a before-write authoring-posture invoke (Step 0) and an
after-write policy gate. The shell-less rw worker leaves policy_status:not_run;
the main window finishes the policy run and reconciles the verdict.

Retry ownership (round-2 review): the checker run + budget-3 .tf fix-and-retry
loop + verdict merge live in a single main-window 'Finish Generate (policy
reconcile)' step that runs BEFORE _postconditions. The _postconditions gate is
strictly read-only (_validate_json + policy_status==POLICY_OK assert; no checker
run, no .tf edits, no verdict edits). status stays passed_degraded_offline
(fmt/init/validate never run in the main window); only policy_status/
policy_violations change. GATE_FAIL recovery is human .tf edits + a targeted
checker re-run, not a full Generate re-run (re-dispatch wipes edits). The
validation-report.policy.json --json output is a temp sidecar, merged then
deleted (not a second verdict). Scope Boundary carves out the main-window
policy reconcile as in-scope Generate work.

- generate.md: 'Finish Generate' pre-gate step; read-only policy _assert; Scope-Boundary carve-out
- generate-terraform.md: Step 12 worker writes not_run placeholder; points at the finish step
- generate-assemble.md: read-only gate; recovery = human edit + targeted re-run, not re-run Generate
- tf-best-practices/SKILL.md: consumers note includes heroku-to-aws
- 2 Heroku-shaped fixtures (POLICY_OK + POLICY_FAIL) + 2 tests; fixture count 23->25
Both plugin trees updated byte-identically.
@herosjourney
herosjourney force-pushed the feat/heroku-tf-policy-gate branch from f2afa2f to 24fc950 Compare September 13, 2026 00:43
@herosjourney

Copy link
Copy Markdown
Contributor Author

Thanks — agreed the retry-ownership split is right, and both wording nits were real (verified against f2afa2f). Fixed in 24fc950 so all three files tell one story:

  1. Step 12 "gate overwrites/re-derives" (generate-terraform.md) — the two lines that attributed the reconcile/overwrite to "the main-window gate" now say the "Finish Generate" step reconciles and overwrites the not_run placeholder, and the read-only _postconditions gate only reads it.

  2. Orientation (generate.md) — the validation-report.json description no longer says the completion gate reconciles it; it now says the main-window "Finish Generate" step reconciles it before the read-only gate reads it.

  3. Extra-protocol finish step / fail-closed — leaving it exactly as you say. The finish step is phase-body prose the main window executes; there's no INTERPRETER.md hook between "worker returns" and _postconditions, and skipping it fails closed (worker not_run + read-only assert). Encoding the retry loop as _postconditions would reintroduce the do-not-modify violation we just removed, so I won't.

Verification (24fc950, both trees): drift OK (279 identical, 25 allowlisted); test_validate_terraform_policy.py 54 passed; mise run build in a fresh detached worktree PASS exit 0 (checkov 276/0, gitleaks no leaks, grype no vulnerabilities). Wording-only change; no behavior or policy-engine change.

herosjourney pushed a commit to herosjourney/startups that referenced this pull request Sep 13, 2026
The heroku-to-aws report validator was optional/non-blocking with zero tests,
and generate-report.md contradicted generate-assemble.md on whether it gates.
This makes it a required, blocking gate and hardens it to the report's own
decision-UX + accessibility contract (ported subset of the gcp-to-aws awslabs#223
checks, scoped to what the thin Heroku one-pager emits).

Round-2 review (host-placement): the validator invocation was in the dispatched
assembler/fragment (no shell under _exec: rw), so 'blocking' was prose on a path
that can't execute. Mirrors the awslabs#287 split: the python3 validator run + a
REPORT_OK assert now live in a main-window 'Finish Generate (report validation)'
step in generate.md that runs BEFORE the read-only _postconditions gate. On
REPORT_FAIL the step emits GATE_FAIL and pastes the validator errors[] verbatim
(so recovery is actionable, not a blind 're-run Generate'). The gate is
read-only; the assembler no longer claims to be 'the main window'.

- validate-heroku-migration-report.py: require cost-optimization; forbid
  badge-verdict-* pills; require verdict-headline when recommendation.outcome
  present; a11y subset (html lang, th scope, figure aria-label+figcaption)
- generate.md: 'Finish Generate' main-window validator step + read-only REPORT_OK
  _assert; Scope-Boundary carve-out for the main-window validate
- generate-report.md: points at the finish step; GATE_FAIL pastes errors[];
  skeleton gains a <th scope=col> example so agents copy the a11y pattern
- generate-assemble.md: read-only gate, no validator call, recovery = hand-edit
  + targeted re-run (not full Generate re-dispatch, which wipes the fix)
- tests: 13 -> 14 cases (add figure-with-aria-label-without-figcaption FAIL)
Deliberately not ported (would false-fail the thin report): decision-before-TOC,
single-h1, per-table caption, glossary/appendix set. Both plugin trees updated
byte-identically.
herosjourney and others added 4 commits September 16, 2026 08:50
…n-fragment

Introduce a new '_exec._agent: rwx' tier (rw + a scoped shell for running the
tf-best-practices policy checker; no git) and point heroku-to-aws Generate at it,
so the dispatched Generate worker runs the Terraform policy checker + fix-and-retry
IN-FRAGMENT instead of via a main-window 'Finish Generate' reconcile step.

Removes the three-actor split awslabs#287 introduced (worker writes not_run placeholder ->
main-window checker+merge -> read-only gate). The worker now produces the real
policy verdict directly; generate.md _postconditions stays a read-only
policy_status == POLICY_OK assert (fail-closed on POLICY_FAIL/not_run).

- shared INTERPRETER.md: add rwx to the tier tables, the closed vocab {ro,rw,rwx,git},
  and the _exec example; clarify rwx softens the tier ORDERING (not a real boundary the
  platform enforced) and stays non-git. Re-vendored to all skills via shared:sync.
- agents/generic-phase-worker-rwx.md (both trees): rw + Bash with strong in-prompt
  scoping (checker only; no git, no network, no arbitrary shell) + host-permission note.
- frontmatter-validator: add rwx to EXEC_TIERS; +1 acceptance test (63 pass, both trees).
- docs 01/02/03/05: tier tables + 'why rw has no shell' updated; generation is no longer
  'pure file work' (it runs the checker), which is why generate uses rwx.
- discover stays rw (pure file work over untrusted input; no shell).

Gates: frontmatter OK (both), shared:check OK, drift OK (282 identical/25 allowlisted),
frontmatter tests 63/63 (both), tf-policy tests 54, asserters 8/8 (both).
Not run locally: markdownlint/dprint/tsc (mise CodeArtifact 401 in this env) — pure
node scripts run direct.
…mt/lint

- generate-terraform.md (both trees): Step 12 now names the --json verdict sidecar
  (validation-report.policy.json) explicitly and adds a cleanup bullet to DELETE it
  after merging into validation-report.json — restores the mainline behavior our
  rwx in-fragment rewrite had dropped (a live run left the sidecar behind).
- docs/05-exec-agent-dispatch.md: MD049 emphasis fixes (*x* -> _x_) on the rwx notes.
- dprint fmt normalized the rwx tier-table rows in INTERPRETER.md + docs; re-vendored.

Gates: mise run build PASS (exit 0; markdownlint 0, dprint clean, checkov 276/0,
gitleaks no leaks, grype clean). frontmatter OK both trees; drift OK (282 identical);
vendored-sync OK; frontmatter tests 63/63.

Negative control: the tf-best-practices checker returns POLICY_FAIL (exit 1) on a
known-bad terraform dir (elasticache_encryption_at_rest + alb_http_redirect +
alb_https_listener, correct file/line), and generate.md _postconditions asserts
policy_status == POLICY_OK else fail-closed — so a FAIL cannot advance the phase.

@herosjourney herosjourney left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed: head 3b51cc08 (base dd7096bd = current origin/main), 49 files (+1353/−151) — heroku-to-aws, tf-best-practices, agent-advisor (vendored INTERPRETER), shared (INTERPRETER + new rwx tier), frontmatter-validator tooling, docs. Both plugin trees.
Ran: mise run lint (drift, frontmatter validator, model-ids, types — all pass), mise run fixtures:check/fixtures:assert (92 json, 8 asserters, all PASS), mise run shared:check (OK), node --test frontmatter-validator suite (63/63 both trees), test_validate_terraform_policy.py (54/54), a direct invocation of the policy checker against the bad-heroku-eb-elasticache fixture (confirmed exit 1 with three real violations at correct file/line), security scans (bandit/gitleaks/checkov clean), git merge-tree vs origin/main (clean).
Did not run: no terraform apply / live AWS calls (the policy stage is a dependency-free static reader, by design); no end-to-end dispatched-worker run (no live agent harness in this environment).

Cross-plugin parity: PASS — drift:check reports 282 identical / 25 allowlisted (unchanged allowlist size). All newly-added/changed files (generic-phase-worker-rwx.md, the INTERPRETER edits, the fixture and validator changes) are byte-identical between advisor/ and migrate/.

Change narrative: wire the previously-inert tf-best-practices policy checker into heroku-to-aws Generate so bad Terraform (public DB, unencrypted cache, HTTP-only ALB) fails the phase instead of silently shipping → the first commit dispatched Generate at the rw tier, which structurally cannot run the checker, and used a worker-written verdict field an _assert trusted without independent re-verification → icarthick's two commits (490eb34, ef2bb7f) replaced that with a new rwx capability tier (rw + a scoped shell whose only sanctioned use is the checker), moved the checker invocation in-fragment into the same worker that writes the Terraform, and fixed a leftover cleanup-ownership inconsistency across three files. Body vs diff: matches — commit messages accurately describe what's in the diff, and I independently verified each claimed test count and gate result rather than trusting them.

Axis Stance One line
Parsimony clear rwx is the minimal new primitive needed; in-fragment execution is simpler than the three-actor split it replaces
Completeness advisory See finding below on gate self-attestation — not a defect, but worth naming explicitly
Security clear Worker's shell is prompt-scoped to one command, explicitly forbids git/network/injected instructions; checker itself is dependency-free and read-only
Alternatives advisory 2 named below
Contradictions clear The cleanup-ownership contradiction from the first pass is fixed; generate.md/generate-terraform.md/generate-assemble.md now tell one consistent story
Overlap clear No restated guidance; the rwx tier is defined once in shared INTERPRETER.md and vendored, not redefined per-skill
Cross-plugin parity clear See above
Propagation & data clear SSOT (shared/dsl/INTERPRETER.md) updated first; both vendored copies re-synced; frontmatter-validator's EXEC_TIERS array (the actual runtime check, not just a comment) updated and has a real acceptance test

Should-fix

  1. [Completeness / Propagation §3] The completion gate verifies a file the same worker wrote, not an independently re-run check. Detail inline on generate.md:64.

Nits

  1. The rwx worker prompt's host-enforcement claim is unverified. Detail inline on agents/generic-phase-worker-rwx.md.

Heads-up (out of diff, pre-existing — not blocking)

  • The EB-ALB blind spot the first commit documented (good-heroku-eb-loadbalanced's ALB is provisioned via aws_elastic_beanstalk_environment settings, which the static checker doesn't read, so it passes vacuously) is unchanged by icarthick's commits and remains a known, documented limitation in tf-best-practices/SKILL.md. Not a regression, just still open.

Alternatives (max 2)

  1. Independent re-verification in the main window instead of in-fragment. Keep the checker invocation in the main-window "Finish Generate" step (the original design) but give that step direct shell access instead of routing through a worker-written placeholder. Wins: the gate would independently re-derive the verdict rather than trust a worker-written field. Costs: reintroduces the three-actor split icarthick simplified away, and the main window doesn't have the Terraform content in its own context without re-reading files anyway. Recommendation: keep icarthick's in-fragment design — the complexity cost of the alternative outweighs closing an already-low-severity trust gap.
  2. Have the main-window gate spot-check by re-invoking the checker itself on POLICY_OK claims. Wins: closes finding 1 outright; the checker is fast/free so this is cheap. Costs: doubles checker invocations. Recommendation: worth considering as a follow-up, not this PR.

Verdict

Approve (posting as Comment since I'm the PR author) — nothing kind-checkable rises to blocking. The redesign genuinely fixes the two real defects from the first pass (a worker instructed to do something its capability tier forbade, and a completion gate that told itself to mutate artifacts), converges the policy gate's trust model to the same baseline every other assert in this file already uses rather than introducing a new hole, and is verified end-to-end (63/63 frontmatter tests, 54/54 policy tests, a live negative-control run, clean security scans, clean cross-plugin drift). Finding 1 is worth a follow-up but doesn't block. Nice work on the redesign, @icarthick — the in-fragment approach is materially simpler than what I shipped first.

Comment thread migrate/plugins/migration-to-aws/agents/generic-phase-worker-rwx.md Outdated
…soften rwx host-scoping claim

Review feedback from @herosjourney (both non-blocking):

1. [should-fix] generate.md _postconditions: the policy_status==POLICY_OK assert
   reads a verdict the same worker wrote; it trusts provenance, it does not
   independently re-derive. Added an explicit TRUST BOUNDARY clause to the _assert
   text so a future reader doesn't mistake it for verification — noting it's the
   same trust level as every other _postconditions assert in the file.

2. [nit] generic-phase-worker-rwx.md: the host command-scoping line
   (Bash(python3:*)) read as if such enforcement exists. Reworded so the in-prompt
   forbidden-actions list is stated as the real, always-present enforcement, and the
   host-level permission scoping is an OPTIONAL defense-in-depth hardening not known
   to be configured on any host today (do not assume it is in effect).

Both trees, byte-identical. Wording-only; no behavior/policy-engine change.
Gates: mise run build PASS (markdownlint 0, checkov 276/0, gitleaks/grype clean),
frontmatter OK both trees, drift OK (282 identical).
@icarthick

Copy link
Copy Markdown
Collaborator

Thanks for the thorough re-review and the approve, @herosjourney — and for independently re-running the gates rather than trusting the claims. Both of your findings are addressed in a823987 (both trees, byte-identical; wording-only, no behavior/policy-engine change):

  1. [should-fix] gate trust boundary — added an explicit TRUST BOUNDARY: clause to the policy_status == POLICY_OK _assert naming that the gate trusts the worker-written verdict's provenance rather than re-deriving it (same trust level as the other asserts here). Alternative 2 (gate re-invokes the checker to spot-check) left as a documented follow-up, not this PR.
  2. [nit] rwx host-scoping — reworded so the in-prompt forbidden-actions list is the real always-present enforcement, and host-level Bash(python3:*) scoping is optional defense-in-depth not known to be configured anywhere today (no longer reads as if it exists).

Heads-up on the rebase: after your main merge (3b51cc0) landed on the branch, I rebuilt my review-fix on top of the current head and pushed as a fast-forward (3b51cc0..a823987) — your main-merge is preserved, and the rwx tier survived it cleanly (present in shared INTERPRETER.md, EXEC_TIERS, both workers; drift OK 282 identical). That merge also pulled in #291's shared phase-status.schema.json / run_mode changes, so the merge-order coordination we flagged is partly resolved here — worth a glance to confirm no double-apply when both land on main.

Verification on the rebuilt base: mise run build PASS (markdownlint 0, checkov 276/0, gitleaks/grype clean), frontmatter 63/63 both trees, drift OK.

@herosjourney herosjourney left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at a823987 (previous: 3b51cc08). Delta: 1 commit, 4 files (2 files × 2 trees, byte-identical within each pair).

Prior finding Status at head
Should-fix — completion gate verifies a worker-written file without independently re-running the checker Fixed at a823987 — re-probed: generate.md's _assert now carries an explicit TRUST BOUNDARY: clause stating the gate trusts the worker's provenance claim rather than re-deriving the verdict, and names this as the same trust level every other _postconditions assert in the file already uses.
Nit — "the deployment SHOULD restrict this worker's Bash" read as an unverified host-enforcement claim Fixed at a823987 — re-probed: the worker prompt now states the in-prompt forbidden-actions list is the real, always-binding enforcement regardless of host, and reframes host-level command scoping as optional defense-in-depth "not known to be configured on any host today — do not assume it is in effect."

New in delta: none. Verified: both changed files still byte-identical between advisor/ and migrate/; cross-plugin-drift.ts unchanged (282 identical / 25 allowlisted); markdownlint clean.

Verdict: Approve — supersedes my prior Comment review. (Posting as Comment since GitHub blocks self-approval on my own PR.) Both findings closed with minimal, correctly-scoped edits; nothing outstanding.

@leon1418 leon1418 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[🤖 AI review 🤖]

Reviewed exact head af91763ee283dccc6f96945928b3254455a615a5 against base 658ca82a5aedd198442f752438c8e6e4c811a4aa, including the complete discussion, both plugin copies, the new rwx worker/dispatch contract, and validation-report producers and consumers.

Three P2 findings below: the mandatory gate rejects a supported CodePipeline permission that requires Resource: "*"; targeted recovery does not reconcile the canonical v2 report; and the policy scan precedes the EKS fragment, so its verdict does not cover the final Terraform directory. The first two concerns from my earlier local review remain reproducible after the rwx redesign. The EKS finding concerns scan ordering even when the worker faithfully runs the checker; it does not repeat the existing provenance-trust advisory.

Local validation: policy tests 54/54 per plugin; frontmatter tests 63/63 per plugin; both Heroku frontmatter validators and vendored-shared checks passed; cross-plugin drift passed (280 identical, 27 allowlisted); git diff --check passed. Directly compared all 24 changed mirror pairs: 20 byte-identical, with only plugin-name/table formatting and a blank-line difference in the other four. Reproduced the IAM and recovery failures in both copies, and exercised a local fragment-order model with a policy violation in a later eks.tf file.

GitHub CI separately: all nine reported checks currently pass for this head. No live AWS, Terraform apply, or end-to-end dispatched-agent execution was run during this review.

Signed-off-by: Logan Kleier <lkleier@amazon.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@herosjourney

Copy link
Copy Markdown
Contributor Author

All three requested P2 fixes are pushed in 8a91c97 and the review threads are resolved:

  • isolated the required elasticbeanstalk:CreateStorageLocation Resource = "*" statement, scoped the remaining EB actions, and added positive + mixed-action regressions
  • moved the authoritative policy run/report ownership to the assembler after conditional eks-generate, with an ordering regression
  • documented targeted recovery as sidecar → merge into canonical v2 envelope → delete sidecar → rerun read-only postconditions

Verification: both policy suites 57/57, mise run lint PASS, mise run build PASS, frontmatter 63/63 in both trees, drift/shared checks clean. @leon1418 please re-review the new head when convenient.

@icarthick
icarthick merged commit b30cb39 into awslabs:main Sep 17, 2026
9 checks passed
@herosjourney
herosjourney deleted the feat/heroku-tf-policy-gate branch September 17, 2026 21:39
herosjourney pushed a commit to herosjourney/startups that referenced this pull request Sep 18, 2026
…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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants