workstream: complete workstreams/language_cleanup/WS01-mechanical-sch…#162
Conversation
…ema-cleanup.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
handcaught
left a comment
There was a problem hiding this comment.
Review
Steps 1–5 are structurally sound. The isStringLiteralExpr fix (TemplateExpr branch), resolveEnvironmentExpr helper, and stdlib registration are all correct. Legacy rejection tests using hclsyntax.ParseConfig are properly constructed and cover both the negative and positive forms. The LANGUAGE-SPEC regeneration is clean and the stdlib section accurately reflects what is registered. Migration of all .hcl files is complete.
However, three user-facing defects were introduced by Step 4 that will actively produce bad outcomes for users, and three additional stale comments need cleaning up. Verdict: changes-requested.
docs/workflow.md:364 — Code example teaches rejected syntax
The sentence reads:
To bind a subworkflow to an environment, set it on the subworkflow declaration:
subworkflow "inner" { environment = "shell.ci" }.
environment = "shell.ci" is the now-rejected quoted-string form. Any reader who follows this will get a parse error. Fix to:
subworkflow "inner" { environment = shell.ci }
docs/workflow.md:1705 — Code example in subworkflow block uses rejected syntax
environment = "shell.ci" # optional: bind callee to a declared environmentSame issue — quoted string is now rejected. Fix to:
environment = shell.ci # optional: bind callee to a declared environmentdocs/workflow.md:214 — Prose implies quoted-string form
The backtick span reads:
environment = "<type>.<name>"
The double-quotes inside the code span imply the value is a quoted string. Since Step 4 changed this to a bare traversal, the description should be:
environment = <type>.<name>
workflow/compile_step_target.go:191 — Diagnostic instructs users to write rejected syntax
Detail: `Set environment on the subworkflow declaration instead: subworkflow "<name>" { environment = "<type>.<name>" }.`,A user who hits this error, reads the diagnostic, and follows the advice will write environment = "shell.ci" — which the parser immediately rejects with a second error. The diagnostic must use the bare-traversal form:
Detail: `Set environment on the subworkflow declaration instead: subworkflow "<name>" { environment = <type>.<name> }.`,The comment on line 173 also says environment = "shell.ci" — update to environment = shell.ci.
workflow/schema.go:98 — Stale comment references removed labeled form
// Spec is the parsed (but unvalidated) HCL workflow document. After workstream
// 17, the `workflow "<name>" { ... }` block is header-only; all content blocksThe label was removed by this workstream. Update to workflow { ... } (no label).
workflow/parser.go:24 — Stale comment references removed labeled form
// Parse decodes HCL source into a Spec. The workflow "name" { ... } block is
// header-only in the new format;Update to The workflow { ... } block is header-only;.
Missing positive compile-level test for adapter environment traversal
compile_subworkflows_test.go has a test asserting sw.Environment == "shell.ci" for the subworkflow path. There is no equivalent test for the adapter path — that adapter "shell" "ci" { environment = shell.ci } compiles to AdapterNode.Environment == "shell.ci". Add one test in compile_adapter_config_test.go or a new file.
All other aspects (schema struct changes, test coverage, LANGUAGE-SPEC accuracy, migration completeness, lint/vet/spec-check) are approved.
…, adapter env test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
handcaught
left a comment
There was a problem hiding this comment.
All 8 items from the previous review are resolved:
docs/workflow.md:214— bare traversal form, quotes removed ✅docs/workflow.md:364—environment = shell.ci(no quotes) ✅docs/workflow.md:1705—environment = shell.ci(no quotes) ✅compile_step_target.go:191— diagnostic now showsenvironment = <type>.<name>✅compile_step_target.go:173— comment updated to bare-traversal form ✅schema.go:98—workflow { ... }(label removed) ✅parser.go:24—workflow { ... }(label removed) ✅TestAdapterEnvironmentTraversalResolvesadded incompile_adapter_config_test.go— parses and compilesadapter "copilot" "bot" { environment = shell.ci }, assertsAdapterNode.Environment == "shell.ci"✅
Approved.
Under -race the multiple-reconnect path can exceed 10s on slower CI runners. Bump to 20s and add progress logging so future flakes are easier to diagnose.
* language_cleanup: scope two workstreams for HCL Terraform-shaping (#160) Adds WS01 (mechanical schema cleanup: workflow{}/policy nesting, type expressions, outcome "default" block, environment traversals, cty stdlib functions, VSCode grammar) and WS02 (data block + outcome semantics: next traversals, return/continue keywords, data "internal" replaces shared_variable, write block replaces shared_writes). Targets main; merges into adapter-v2 once the language phase closes. For architecture review before implementation begins. Co-authored-by: Dave Sanderson <dave@brokenbots.net> * workstream: complete workstreams/language_cleanup/WS01-mechanical-sch… (#162) * workstream: complete workstreams/language_cleanup/WS01-mechanical-schema-cleanup.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * workstream: complete workstreams/language_cleanup/WS01-mechanical-schema-cleanup.md * fix: address PR review feedback - bare traversal docs, stale comments, adapter env test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: increase TestClientReconnectMultipleFailures timeout for CI runners Under -race the multiple-reconnect path can exceed 10s on slower CI runners. Bump to 20s and add progress logging so future flakes are easier to diagnose. --------- Co-authored-by: Dave <dave@brokenbots.dev> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Dave Sanderson <dave@brokenbots.net> Co-authored-by: Dave Sanderson <dave@nullop.io> Co-authored-by: Dave <dave@brokenbots.dev> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Mechanical schema cleanup: reshape HCL to align with Terraform conventions (steps 1-4 of the language cleanup workstream).
What changed
workflow {}header reshape:workflow "name" {}(label) →workflow { name = "..." }. Top-levelpolicy {}moved insideworkflow { policy { ... } }.type = "string"(string literal) →type = string(type expression). Unlockslist(string),object({...}),any, etc.default_outcome→outcome "default" {}: Attribute replaced by named outcome block carrying its ownnext/output/writes.environment = "shell.ci"(quoted) →environment = shell.ci(bare traversal) on workflow/adapter/subworkflow.All legacy forms emit helpful migration hints via the existing
parse_legacy_reject.gopattern.Test evidence
TestLegacyReject_WorkflowLabel,TestLegacyReject_PolicyBlock_TopLevel,TestLegacyReject_TypeString_Quoted(variable/shared_var/output),TestLegacyReject_DefaultOutcomeAttr,TestLegacyReject_EnvironmentString_QuotedOn*(workflow/step/adapter/subworkflow) — all pass withhclsyntax.ParseConfig(catchesTemplateExprvsLiteralValueExprbug).TestPositive_NestedPolicy(verifiesHeader.Policy.MaxTotalSteps),TestPositive_TypeExpressions(6 type kinds compile to correctcty.Type),TestPositive_DefaultOutcomeBlock(verifiesStepNode.DefaultOutcomeis non-nil with correctName/Next).make test(all packages incl.-race) ✅ ·make build✅ ·make validate✅ ·make lint-imports✅ ·make lint-go✅ ·make spec-check✅ ·make test-conformance✅Out of scope (deferred to WS02)
Workstream:
workstreams/language_cleanup/WS01-mechanical-schema-cleanup.mdReview: approved 2025-05-25-03 (all 8 prior blockers resolved; no remaining issues)