Require explicit Elastic Beanstalk port and health settings

[amjadsy]

Problem

The Heroku-to-AWS skill hard-coded Elastic Beanstalk application port 5000 and health path /health. Those values are application-specific, so generated starter infrastructure could route traffic incorrectly or fail health checks while appearing ready to plan.

Solution

For Elastic Beanstalk designs only, generation now declares eb_application_port and eb_health_check_path as required string inputs with no defaults and passes them unchanged to the PORT and HealthCheckPath settings. The migration guide and README explain the required inputs, and the Generate completion gate fails closed if the variables or direct wiring are missing.

The synchronized Heroku skill surfaces are both updated. Fargate and EKS generation behavior remains unchanged.

Type of Change

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

Team Folder

• advisor/
• migrate/
• Other: root regression test and mise test wiring

The advisor/ and migrate/ files are the repository's synchronized copies of the same Heroku skill.

Validation

• mise run build
• mise run test
• mise run drift:check
• mise run lint:frontmatter
• Provider-free Terraform plan checks for missing required values and exact preservation of 4321 and /readyz
• Fresh Generate-phase skill replay confirming required variables have no defaults, supplied values are preserved unchanged, and Fargate/EKS are unaffected
• terraform fmt -check -recursive against the replayed output

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
• I have updated documentation if needed
• My changes are scoped to the Heroku skill's synchronized plugin surfaces and regression-test wiring

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[herosjourney]

Overall

The bug is real and worth fixing — hardcoded PORT=5000 and /health can produce infrastructure that looks ready but fails health checks at runtime. The execution is solid: both plugin copies updated in sync, fail-closed gate, and a genuinely thorough regression test. Requesting changes on three items below (1–3); two more are fine as follow-up issues.

1. BLOCKING — Extract these values instead of forbidding it; keep required tfvars as the fallback only

The PR explicitly forbids deriving the port and health path from the source repository, but Discover already has the evidence: the Dockerfile is a stated EB prerequisite (EXPOSE, ENV PORT) and the Procfile defines the web process. Throwing that away means every EB migration now ends with a failed terraform plan and a manual tfvars edit — even when the answer was sitting in the repo. The tell is that the PR has to downgrade the phase's own promise ("deployable artifacts" → "migration artifacts") to stay honest.

Please invert the design: extract the port from the Dockerfile/Procfile when possible, default the health path to / (EB's own default, and the same convention this skill's Fargate target groups already use), and surface both as detected/assumed values the user can override. Keep the required-no-default variables exactly as designed for the case where extraction finds nothing — that fail-closed behavior is right, it just shouldn't be the only path.

2. FIX IN THIS PR — Make the variables per-app

The EB environment resource is templated per app (aws_elastic_beanstalk_environment "<app_name>_<process_type>"), but eb_application_port / eb_health_check_path are singletons — two EB apps with different ports would share one pair. This also contradicts the naming convention documented a few lines below the new block in generate-terraform.md (<resource_type>_<heroku_app>_<attribute>), which the existing container_image_<app>_<process_type> tfvars entries already follow. Please switch to eb_application_port_<app> / eb_health_check_path_<app> and update the test assertions to match.

3. FIX IN THIS PR — Allow input validation

With type = string and validation explicitly forbidden, eb_application_port = "banana" plans cleanly and fails at deploy — exactly the late failure this PR is trying to eliminate. type = string is fine (EB setting values are strings), but please drop the "do not validate" instruction and add validation blocks: numeric 1–65535 for the port, nonempty for the path. "Preserve the value exactly" and "reject obvious garbage at plan time" are compatible goals.

4. FOLLOW-UP — Fargate docs still hardcode /health, and the new tests pin it

generate-docs.md still says Health check endpoint returns 200: https://{{ALB_DNS_NAME}}/health for Fargate verification while the Fargate target group actually health-checks / — and the new test asserts that hardcoded line stays. Scoping Fargate out of this PR is fine, but the eventual fix will now require a test change too. Filing as a follow-up; in the meantime consider softening that assertion so the follow-up isn't test-blocked.

Minor: the test hard-asserts terraform_version matches 1.13.x, which will break on every Terraform bump for no behavioral reason. A minimum-version check would age better. Not blocking.

What's great and should stay: the dual-plugin byte-identical sync test, the fail-closed Generate postcondition, the provider-free plan tests with sentinel preservation (4321, /readyz), and the docs updates.

[amjadsy]

@herosjourney

Thanks for the review. I updated the PR to use validated, per-app inputs for Beanstalk web services while excluding worker-only services. I kept automatic source extraction and a default / health path out of scope because the current discovery phase cannot establish those values reliably. Fargate health-check consistency can be a follow-up.

[leon1418]

[🤖 AI review 🤖]

Reviewed current head dbac3d413794495ef7445e6a32dbf192ff4c0ead. I found no additional mandatory code defects.

Verified:

• per-web-app Beanstalk port/path variables and worker guards are correctly scoped
• port/path validation rejects malformed values while preserving valid sentinels
• multi-app Terraform planning keeps independent values
• advisor/migrate copies remain byte-identical
• Fargate/EKS behavior is unchanged

Local validation passed: 12/12 targeted runtime-setting tests, 62/62 frontmatter tests in each plugin, cross-plugin drift (257 identical / 25 allowlisted), frontmatter lint, and TypeScript checks. GitHub CI is 8/8 green and the branch is mergeable.

Two previously reported non-blocking items remain: the test pins Terraform to 1.13.x, and Fargate verification docs still mention /health while the target group uses /. I did not duplicate those threads.

The existing CHANGES_REQUESTED review is still active because the author intentionally requires manual tfvars rather than detecting Dockerfile/Procfile values. That is an unresolved product/design decision, so I am commenting rather than approving. Code-wise this revision is clean; the PR is not merge-ready until that review decision is resolved.

[amjadsy]

@herosjourney
To close the remaining design question: PR231 intentionally does not infer port and health settings from Dockerfile or Procfile because those files cannot reliably establish both values for every application. This PR provides validated per-app inputs and fails safely instead of guessing. The staged source-review work beginning with PR239 and follow-up PRs will inspect application code and eventually feed reliable port and health findings into generation. If this scope is acceptable, could you re-review and approve?

[herosjourney]

Re-review, including the review thread

I re-verified the current PR head against every item raised across both review rounds - not just reading the responses, but checking the actual code.

Confirmed fixed, exactly as requested:

• Item 2 (per-app variables) - eb_application_port_web / eb_health_check_path_web, matching the resource_type_heroku_app_attribute convention used elsewhere in this file. Worker-only services correctly excluded ("Do not emit these variables for non-web Elastic Beanstalk services").
• Item 3 (input validation) - both variables now carry real validation blocks: numeric 1-65535 for the port, startswith("/") + length cap for the health path. type = string preserved as requested, so "preserve exactly" and "reject garbage at plan time" are both satisfied.
• Confirmed still clean: cross-plugin-drift.ts (259 identical, 25 allowlisted), and the two generate-terraform.md copies remain byte-identical between advisor/ and migrate/.

Still open, exactly as flagged (expected - these were labeled follow-up/non-blocking):

• Item 4 - generate-docs.md still hardcodes https://{{ALB_DNS_NAME}}/health for Fargate verification while the target group health-checks /. Confirmed unchanged.
• Minor - the test still hard-pins terraform_version to 1.13.x, which will break on every Terraform bump for no behavioral reason.

Item 1 (the BLOCKING item) - deliberately not implemented, with a stated reason

The design still forbids extraction: generate-terraform.md explicitly states "the generator has no evidence for either application-specific value" and requires manual tfvars for every EB app, with no Dockerfile/Procfile fallback attempted. The author's response (Aug 27) explains this is intentional - Discover currently can't reliably establish both values from Dockerfile/Procfile inspection, and that reliable extraction is being built as a separate, staged effort starting with #239.

I checked #239 directly: it's real, substantive groundwork (a 22-question data-only findings contract for a future source-code reviewer, explicitly scoped to feed Beanstalk compatibility decisions in follow-up PRs), not a placeholder or a deflection. So the "extraction is coming, just not in this PR" claim holds up against actual evidence, not just an assertion.

My read on the design disagreement: both positions are reasonable, but I'd side with landing this PR now rather than holding it for extraction. The core bug this PR fixes - hardcoded PORT=5000/health silently producing infrastructure that plans clean but fails health checks at runtime - is real and independent of whether the replacement values come from a human or from Discover. Fail-closed-with-required-tfvars is a strictly safer interim state than the current hardcoded default, and #239 shows the extraction path is being built deliberately rather than deferred indefinitely. Blocking this fix on that multi-PR effort landing first extends the window where the original bug is live in production use.

Recommendation: Approve. All three items originally requested as blocking/fix-in-this-PR are resolved (items 2, 3 fully; item 1 has a defensible, evidenced deferral rather than a silent drop), the two remaining items are correctly scoped as non-blocking follow-ups, and cross-plugin sync + validation still check out on the current head.

[herosjourney]

Re-verified all items from both review rounds against current code (details in comment above). Items 2 and 3 confirmed fixed exactly as requested; item 1's deferral is backed by real, evidenced follow-up work (#239). Approving.