Skip to content

test(provisioner/terraform): add script-order conversion fixtures - #31038

Open
geokat wants to merge 1 commit into
mainfrom
george/script-order/15-add-script-ordering-plan-and-state-test-fixtures
Open

test(provisioner/terraform): add script-order conversion fixtures#31038
geokat wants to merge 1 commit into
mainfrom
george/script-order/15-add-script-ordering-plan-and-state-test-fixtures

Conversation

@geokat

@geokat geokat commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Add reproducible Terraform plan, state, and graph fixtures covering
startup and shutdown rules, explicit and inferred phases, nested and
empty modules, count, for_each, overlapping rules, and unordered
scripts.

Verify that plan conversion validates rules without attaching runtime
dependencies, while post-apply conversion produces the expected
ordered graphs and per-script dependencies. Check diagnostics in both
paths.

Add selector coverage for all matching managed for_each instances.

Refs:
https://linear.app/codercom/issue/PLAT-552

@geokat
test(provisioner/terraform): add script-order conversion fixtures
7b5b606 
Add reproducible Terraform plan, state, and graph fixtures covering
startup and shutdown rules, explicit and inferred phases, nested and
empty modules, `count`, `for_each`, overlapping rules, and unordered
scripts.

Verify that plan conversion validates rules without attaching runtime
dependencies, while post-apply conversion produces the expected
ordered graphs and per-script dependencies. Check diagnostics in both
paths.

Add selector coverage for all matching managed `for_each` instances.

Refs:
https://linear.app/codercom/issue/PLAT-552
@linear-code

linear-code Bot commented Oct 19, 2026

Copy link
Copy Markdown

PLAT-552

@geokat

geokat commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

/coder-agents-review Please limit the review to P0, P1, and P2 issues only.

@coder-agents-review

coder-agents-review Bot commented Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Chat: Review posted | View chat
Requested: 2026-10-19 23:54 UTC by @geokat

Review history
  • R1 (2026-10-20): 9 reviewers, COMMENT. Review

deep-review v0.13.0 | Round 1 | 8e76aec..7b5b606

Last posted: Round 1, no findings, COMMENT. Review

Finding inventory

Finding inventory: PR #31038

Findings

# Sev Status Location Summary Round Reviewer Posted
CRF-1 P3 Dropped by orchestrator (below author-requested P0-P2 threshold) generate.sh:15 Fixtures outside canonical generator; no staleness check, unresolvable provider constraint, no CODER_* unset R1 Netero No
CRF-2 Note Dropped by orchestrator (below author-requested P0-P2 threshold) scriptorderconvert_internal_test.go:20 count = 0 module declaration coverage is only indirect (NoError) R1 Netero No
CRF-3 P3 Dropped by orchestrator (below author-requested P0-P2 threshold) scriptorderconvert_internal_test.go:76 Plan-step dependency loop checks only the 4 root scripts; module scripts are absent from plan conversion and the test does not record it R1 Pariston P3, Knov Note No
CRF-4 Nit Dropped by orchestrator (below author-requested P0-P2 threshold) scriptorderconvert_internal_test.go:20 Test comment narrates the test instead of the Terraform shapes it depends on R1 Gon No
CRF-5 Nit Dropped by orchestrator (below author-requested P0-P2 threshold) generate.sh:6 Header restates terraform init "$@" instead of naming the argument to pass R1 Gon No
CRF-6 Note Dropped by orchestrator (below author-requested P0-P2 threshold) generate.sh:22 state.dot is byte-identical to plan.dot by design (-type=plan matches executor.graph) R1 Gon No
CRF-7 OOS Out of scope (body) resources.go:1166 Plan-path dependsOnAgent drops module-declared scripts from agents R1 Mafuuu, Knov, Pariston No

Law analysis

Round 1. Effective LOC +2234 (1852 Terraform-generated, 382 hand-written). Head 7b5b606. Verdict: Don't split. Enforcement: Advisory.

Contested and acknowledged

Round log

Round 1

Full panel (round 1). Focused (Go tests and Terraform fixtures only): ging-go, ryosuke, pariston, mafuuu, bisky, komugi, gon, leorio, wildcard knov. Netero: 1 P3, 1 Note. Law: don't split.

Cross-check: Gon rated CRF-4 and CRF-5 P2. Keep-argument: AGENTS.md requires comments to describe behavior, and CRF-4's comment partly narrates the test. Downgraded to Nit because both comments are accurate and the change is wording only, with no behavior or reader-misleading consequence. CRF-3 converges Pariston (P3) and Knov (Note); higher wins at P3. Keep-at-P3 argument: require.Nil(order) and the Empty loop cannot fail with the current SOURCE_PLAN branch, and the loop examines 4 of 8 scripts. Not raised higher because the assertions act as a regression guard: if plan conversion began attaching dependencies, the root coder_script.work scripts would fail the loop. The root cause (CRF-7) is pre-PR behavior in resources.go, already recorded by testdata/resources/module-script/converted_state.plan.golden (contains no module script). Knov's additional claim that the suffix match confuses root and module addresses was checked against plan.dot node names ("[root] module.bootstrap.coder_script.start (expand)") and is incorrect: the ] prefix in the suffix excludes module-qualified nodes. The PR author asked for P0-P2 findings only; no finding reached P2, so none are posted inline. Verified both new tests pass at 7b5b606 and the >= 2.20.0 provider constraint matches no registry release (latest 2.20.0-pre1).

Reviewed against 8e76aec..7b5b606.

About deep-review

CRF = Coder Review Finding (P0-P4, Nit, Note)

Reviewer Focus
Bisky tests
Chopper ops/errors
Churn-guard change verification
Ging language modernization
Gon naming
Hisoka edge cases
Killua perf
Kite change integrity
Knov contracts
Knuckle SQL
Komugi flake/determinism
Kurapika security
Law decomposition
Leorio docs
Luffy product
Mafu-san process
Mafuuu contracts
Melody dispatch/pairing
Meruem structural
Nami frontend
Netero mechanical checks
Pariston premise testing
Pen-botter product gaps
Razor verification
Robin duplication
Ryosuke Go arch
Takumi concurrency
Zoro shape

🤖 Managed by Coder Agents.

coder-agents-review[bot]
coder-agents-review Bot reviewed Oct 20, 2026
View reviewed changes

@coder-agents-review coder-agents-review Bot 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.

This PR adds Terraform-generated script-order fixtures, a plan and state conversion test that uses them, and a for_each selector case. The review found no P0, P1, or P2 issues. As requested, lower-severity findings are not posted. Both new tests pass at 7b5b606.

Out of scope (needs a ticket or explicit acceptance by a human):

  • provisioner/terraform/resources.go:1166: on the plan path, dependsOnAgent matches only a direct coder_script.<name> to coder_agent graph edge, so module-declared scripts are attached to no agent. Plan-source conversion of this PR's fixture keeps 4 of its 8 scripts, and the existing testdata/resources/module-script plan golden also has no module script.

🤖 This review was automatically generated with Coder Agents.

@geokat
geokat marked this pull request as ready for review October 20, 2026 00:13
@geokat
geokat requested a review from BobbyHo October 20, 2026 00:14

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BobbyHo BobbyHo Awaiting requested review from BobbyHo
1 more reviewer
@coder-agents-review coder-agents-review[bot] coder-agents-review[bot] left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@geokat geokat

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@geokat