WS02 data and outcome semantics

[handcaught]

WS02 — data block and outcome semantics

Phase: Language Cleanup · Track: Language · Owner: Workstream executor · Depends on: WS01 (cleaned schema, type expressions, outcome "default" form). · Unblocks: future remote-data sources (data "http", data "remote_state", etc.). · Base branch: main

Context

Two semantic warts in the workflow HCL share enough underlying code (outcome compilation, runtime store, eval context shape) that they're best landed together:

1. Outcome next is a magic string. next = "step_name", next = "return", next = "_continue". Switch conditions already accept the traversal form (next = step.foo) via resolveNextAttr. Outcomes should match — plus the "return" and "_continue" sentinels become bare keywords (return, continue). The big win is renaming: if a new target type (e.g. an inline_workflow step) appears later, callers don't have to worry about naming conflicts because the kind is part of the reference.

2. shared_variable is not Terraform-shaped. Terraform has resource (read-write) and data (read-only-from-the-perspective-of-Terraform). There's no clean Terraform parallel to a workflow-scoped mutable variable, but data is the closer of the two and people already use that terminology to describe it. Renaming shared_variable to data "internal" "name" (with data.internal.name.value reads) opens the door to future remote data sources (data "http", data "remote_state", etc.) without re-shaping the surface. Internal-kind values are mutable; remote-kind values would be read-only — same as Terraform's data semantics.

Bundling these two changes is correct because:

• Both touch workflow/compile_steps_graph.go (compileOutcomeBlock, compileOutcomeRemain).
• Both touch the engine runtime: outcome sentinel handling in iteration logic, and the shared-variable store rename to a data store.
• Both ripple through the same examples and .criteria/workflows/ files.

Migration strategy is hard break with helpful errors, same as WS01.

Prerequisites

• WS01 merged. This WS edits OutcomeSpec, which lives next to many of the structs WS01 touches.
• cty.Object nested-namespace eval contexts (already used by each.* and step.* references).

In scope

Step 1 — Outcome next traversals

Today: next = "step_name", next = "return", next = "_continue"
Target: next = step.step_name, next = state.done, next = return, next = continue

• workflow/schema.go OutcomeSpec.Next string hcl:"next" (line 292) → Next hcl.Expression hcl:"next". The expression is resolved by the compiler, not stored as a string.
• Extend resolveNextAttr to accept:
  • Two-segment traversals step.<name>, state.<name>, switch.<name>, wait.<name>, approval.<name> (already supported).
  • Single-segment bare keywords return and continue (TraverseRoot with no attribute). These lower to the existing internal sentinels (ReturnSentinel = "return" and "_continue").
• Wire compileOutcomeBlock to call resolveNextAttr for every outcome.
• Internal sentinels (ReturnSentinel, the "_continue" string compared against in compile_steps_graph.go:293 and in the engine iteration path) become internal-only representations — surface syntax never quotes them. Keep the constants for now; just don't let them appear in user-authored HCL.
• Legacy rejection: detect a string-literal next = "..." in an outcome and emit next is now a node reference: write next = step.foo, next = state.done, next = return, or next = continue.

Step 2 — data "internal" "name" block

Today:

shared_variable "cycle_count" {
  type  = number   # WS01 has already converted to type expression
  value = 0
}
# read:  shared.cycle_count

Target:

data "internal" "cycle_count" {
  type  = number
  value = 0
}
# read:  data.internal.cycle_count.value

• New schema type DataSpec in workflow/schema.go:

  type DataSpec struct {
      Kind   string         `hcl:"kind,label"` // first label, e.g. "internal"
      Name   string         `hcl:"name,label"` // second label
      Type   hcl.Expression `hcl:"type"`       // required; WS01-style type expression
      Remain hcl.Body       `hcl:",remain"`    // optional "value" + "description"
  }

• New compiled node DataNode mirroring SharedVariableNode:

  type DataNode struct {
      Kind         string
      Name         string
      Type         cty.Type
      InitialValue cty.Value
      Description  string
  }

• workflow/schema.go Spec and SpecContent: replace SharedVariables []SharedVariableSpec with Data []DataSpec. The FSMGraph.SharedVariables map and SharedVariableOrder slice are replaced by Data map[string]map[string]*DataNode (keyed by kind then name) and DataOrder []DataRef for stable iteration. (Implementation note: choose whichever flat shape compiles cleanly — the surface contract is data.<kind>.<name>.value.)
• New file workflow/compile_data.go paralleling compile_shared_variables.go. Initially only kind = "internal" is supported — emit a clear unsupported data kind %q; only "internal" is currently supported for anything else, paving a clean extension point for future kinds.
• Delete workflow/compile_shared_variables.go and its test file; the new compile_data.go replaces them.

Step 3 — Eval context: nested data namespace

• workflow/eval.go BuildEvalContext (and BuildEvalContextWithOpts): replace the flat shared = cty.ObjectVal{...} entry with a nested data = cty.ObjectVal{ internal = cty.ObjectVal{ <name> = cty.ObjectVal{ value = <current value>, type = <type cty representation> } } }.
• Reads like data.internal.cycle_count.value work via standard cty traversal — no special parsing needed.
• If future kinds are added (e.g. data "http"), they slot in as additional keys under data with the same value-bearing shape.

Step 4 — write block (replaces shared_writes)

HCL grammar constraint: attribute LHSs must be a single bareword identifier; data.internal.x.value = expr will not parse. Use a block-per-write shape — matches Terraform's provisioner block pattern. The block is singular (write) because each block updates exactly one target.

Target:

outcome "success" {
  next = step.next

  write {
    target = data.internal.cycle_count.value
    value  = output.stdout
  }
  write {
    target = data.internal.last_msg.value
    value  = output.reason
  }
}

• New schema type WriteSpec in workflow/schema.go:

  type WriteSpec struct {
      Target hcl.Expression `hcl:"target"` // traversal: data.<kind>.<name>.value
      Value  hcl.Expression `hcl:"value"`  // runtime-evaluated expression
  }

• Add Writes []WriteSpec hcl:"write,block" to OutcomeSpec.
• Replace CompiledOutcome.SharedWrites (map[string]string) with Writes []CompiledWrite:

  type CompiledWrite struct {
      DataKind  string         // resolved from target traversal
      DataName  string         // resolved from target traversal
      ValueExpr hcl.Expression // runtime-evaluated against the step's output scope
  }

• Compile-time validation:
  • target must be a four-segment traversal data.<kind>.<name>.value whose <kind>.<name> resolves to a declared data block. Anything else is a compile error with a clear message.
  • value references are validated the same way today's shared_writes keys are — must reference an output key that exists in the outcome's projected output (if output = { ... } is declared) or the adapter's output schema (if no projection).
  • Aggregate-iteration rule (currently in compile_steps_graph.go:48-52) carries over: writes on aggregate iterating outcomes must reference a projected output = { ... }, never raw adapter output.
• Legacy rejection: detect shared_writes = { ... } in an outcome and emit shared_writes has been replaced by per-target write blocks: write { target = data.internal.<name>.value, value = output.<key> }.

Step 5 — Engine runtime

• internal/engine/ — rename SharedVarStore → DataStore, keyed by (kind, name). The runtime state machine treats only kind == "internal" as mutable; other kinds are read-only (locked at compile time for now, but the lock point lives here so future kinds slot in cleanly).
• internal/engine/node_step.go — replace applySharedWrites with applyDataWrites:
  • Iterate CompiledOutcome.Writes.
  • For each entry, evaluate ValueExpr against the post-projection output scope.
  • Apply the write to DataStore[kind][name] under the existing per-data lock (atomic across all writes from a single outcome — same guarantee shared_writes had).
• Outcome sentinel handling: ensure the iteration code paths that compare co.Next == "_continue" and co.Next == ReturnSentinel still work. Surface form changed (continue / return keywords); the compiled Next string is unchanged.

Step 6 — VSCode grammar updates

Coordinated single update to criteria-vscode-extension-v1/syntaxes/criteria-hcl.tmLanguage.json:

• Add data block matcher: ^(data)\s+("[^"]*")\s+("[^"]*")\s*\{ with kind/name capture groups.
• Drop the shared_variable matcher.
• Outcome body: next = value highlighting — recognize step.x, state.x, switch.x, wait.x, approval.x (traversals) and bare return / continue (keywords); demote string-form next = "..." to a legacy-error class so users see the mismatch.
• Add write block inside outcome with target / value attributes.
• Drop shared_writes from the outcome attribute list.

Step 7 — Migration rewrites

Workflows that use shared_variable/shared_writes and need full rewriting:

• examples/phase3-shared-variable/main.hcl
• examples/llm-pack/07-shared-variable/main.hcl
• examples/archived/workstream_review_loop/**/*.hcl (heavy user)
• .criteria/workflows/develop/main.hcl
• .criteria/workflows/pr_review/main.hcl
• proposed_hcl.hcl

Workflows that need next migrated (string → traversal):

• All .hcl files under examples/, .criteria/workflows/. Mechanical sed for next = "x" → next = step.x/state.x driven by graph context; next = "return" → next = return; next = "_continue" → next = continue. Spot-verify by compile.

Consider renaming examples/phase3-shared-variable/ to examples/phase3-data/ for consistency.

Step 8 — Tests

• workflow/parse_legacy_reject_test.go: add cases for shared_variable block, shared_writes attribute, and string-form next — assert that each migration hint appears.
• Positive tests:
  • data "internal" "x" { type = number, value = 0 } compiles; eval context exposes data.internal.x.value.
  • write { target = data.internal.x.value, value = ... } applies at runtime; concurrent writes use the same atomic lock semantics today's shared_writes does.
  • next = step.foo, next = state.done, next = return, next = continue all resolve to the correct compiled target.
  • Negative: next = "step.foo" (quoted) is rejected with the migration message.
  • Negative: write { target = data.unknown_kind.x.value, ... } is rejected with unsupported data kind.
• End-to-end:
  • examples/phase3-shared-variable/main.hcl (rewritten as data "internal" {}) — runs through with writes applied across steps.
  • .criteria/workflows/develop/main.hcl — exercises data reads in switch match and writes in outcomes. Confirm runtime semantics are byte-equivalent to the pre-migration run.
• Final sweep: rg 'shared_variable|shared_writes|next = "[^"]+"' workflow/ examples/ .criteria/ returns zero hits in .hcl files.

Out of scope

• Adding new data kinds beyond internal. The compile and eval paths are designed to make adding data "http", data "remote_state", etc. straightforward; the actual integrations are future work.
• Type-narrowing / write-side type-checking against the data's declared type (current shared_writes doesn't type-check writes against the variable's declared type either; preserve parity).
• Loop primitives, error handlers, structured concurrency surface — all future work.
• Adapter v2 work — separate track.

Reuse pointers

• Existing resolveNextAttr — extend it; don't rewrite.
• Existing compile_shared_variables.go — fork its compile shape into the new compile_data.go, then delete.
• Existing engine shared-store locking — preserve the atomicity semantics in the renamed DataStore.
• Existing aggregate-iteration write rule in compile_steps_graph.go:48-52 — keep the rule; apply it to Writes instead of SharedWrites.

Behavior change

User-facing surface: every workflow file using shared_variable or any outcome with a string-form next changes shape.

Runtime semantics: unchanged. Same atomicity guarantees on writes, same iteration semantics for return/continue, same eval order for outcome projections vs writes.

Future extensibility: data "<kind>" "<name>" is the extension point. The legacy shared_variable and shared_writes constructs are gone permanently.

Tests required

• All existing tests pass after fixture migration.
• New tests in Step 8 pass.
• go vet ./... clean.
• Manual: VSCode extension highlights migrated workflows correctly (data block, write block, traversal-form next, bare return/continue).
• Manual: criteria run examples/phase3-shared-variable/main.hcl (rewritten) executes successfully.
• Final grep sweep is clean (Step 8).

Implementation Notes

Checklist

• Step 1 — Outcome next traversals
  • OutcomeSpec.Next changed from string to hcl.Expression
  • resolveNextAttr extended to accept bare return/continue keywords
  • compileOutcomeBlock wires resolveNextAttr for every outcome
  • String-literal next = "..." rejected with migration message
• Step 2 — data "internal" "name" block
  • DataSpec, DataNode, DataRef added to schema
  • FSMGraph.SharedVariables replaced with Data map keyed by (kind, name)
  • workflow/compile_data.go created; only kind = "internal" supported
  • workflow/compile_shared_variables.go and its test deleted
• Step 3 — Eval context: nested data namespace
  • BuildEvalContextWithOpts emits nested data = { internal = { name = { value = ..., type = ... } } }
• Step 4 — write block (replaces shared_writes)
  • WriteSpec and CompiledWrite added to schema
  • OutcomeSpec.Writes replaces CompiledOutcome.SharedWrites
  • compileWrites validates 4-segment data.<kind>.<name>.value target traversal
  • Aggregate-iteration write rule carried over to Writes
  • Legacy shared_writes rejected with migration message
• Step 5 — Engine runtime
  • SharedVarStore renamed to DataStore keyed by (kind, name)
  • applyDataWrites replaces applySharedWrites with same atomicity guarantee
  • Outcome sentinel handling unchanged (_continue, ReturnSentinel)
• Step 6 — VSCode grammar updates
  • VSCode extension grammar changes are out of tree; documented for coordination
• Step 7 — Migration rewrites
  • All .hcl files under examples/, .criteria/workflows/ migrated
  • next = "..." → traversal syntax (step.foo, state.done, return, continue)
  • shared_variable → data "internal" blocks
  • shared_writes → write { target = ..., value = ... } blocks
  • shared.name → data.internal.name.value
• Step 8 — Tests
  • Legacy rejection tests added for shared_variable, shared_writes, string-form next
  • Positive tests cover data compilation, write block resolution, traversal next
  • All go test ./... pass
  • go vet ./... clean
  • make validate passes on all examples
  • Final grep sweep: zero hits for old syntax in .hcl files

Opportunistic Fixes

• Fixed runtimeOnlyNamespaces in workflow/compile_fold.go: replaced "shared" with "data" to prevent compile-time folding of data.* references in outcome output blocks.
• Fixed internal/cli/compile_dot_test.go writeTempSubworkflow which generated invalid HCL next = step."done" after migration.
• Fixed workflow/compile_bench_test.go which had next = step.done in Go string context after migration script.
• Migrated proposed_hcl.hcl to new data "internal" and traversal next syntax (was missed in original migration sweep).
• Updated stale comments across tools/spec-gen/render.go, internal/engine/node_step.go, internal/engine/parallel_iteration.go, internal/engine/while_iteration.go, workflow/compile_outputs.go, workflow/compile_validation.go, workflow/compile_adapters.go, and workflow/compile_steps_iteration.go to use data/write terminology instead of shared_variable/shared_writes.
• Fixed examples/phase3-shared-variable/main.hcl and examples/llm-pack/07-shared-variable/main.hcl: corrected incorrectly migrated write blocks that referenced non-existent adapter outputs (output.next_status, output.next_value) back to literal string values.
• Fixed docs/llm/07-shared-variable.md: updated the embedded HCL example to match the corrected examples/llm-pack/07-shared-variable/main.hcl, and replaced stale shared_variable/shared_writes terminology with data/write terminology throughout the text and cross-references.
• Removed dead code: deleted unused SharedVariableSpec and SharedVariableNode structs from workflow/schema.go (superseded by DataSpec/DataNode).

Reviewer Notes

• The data block shape uses two labels (kind, name) matching Terraform's data resource pattern.
• Only kind = "internal" is supported at compile time; other kinds produce a clear error message.
• The engine DataStore treats non-"internal" kinds as read-only (enforced at compile time currently).
• Legacy shared_variable blocks are rejected at parse time by rejectLegacySharedVariableBlock before type-string checks would fire, so the TestLegacyReject_TypeString_QuotedSharedVar test was updated to test data blocks instead.
• The VSCode grammar changes must be applied in the sibling criteria-vscode-extension-v1 repository.

Security Checks

• No new external dependencies introduced.
• DataStore.SetBatch preserves existing atomicity semantics (all writes from a single outcome applied together).
• No secrets or credentials exposed in code or test fixtures.
• resolveNextAttr rejects string-literal next values to prevent accidental misinterpretation of user input.
• Input validation on write block targets ensures only declared data blocks can be mutated.

Reviewer Notes

Review 2026-05-27 — changes-requested

Summary

The core implementation of WS02 is functionally correct: all 8 plan steps are implemented, make test, make validate, make build, make lint-go, make lint-imports, go vet ./..., make spec-check, and make validate-self-workflows all pass, and the final .hcl grep sweep is clean. However, this review found 4 stale comments in production code, 3 significant documentation gaps, and 5 missing test cases. The stale comments and doc gaps are required remediations because they leave incorrect terminology in the codebase and user-facing documentation. The test gaps are blockers because the plan's Step 8 explicitly requires them and they represent untested compile-time validation paths.

Plan Adherence

• Step 1 ✅ — OutcomeSpec.Next is hcl.Expression; resolveNextAttr handles bare return/continue and two-segment traversals; compileOutcomeBlock wires resolveNextAttr; string-literal next = "..." rejected with migration message.
• Step 2 ✅ — DataSpec, DataNode, DataRef added; FSMGraph.Data map keyed by (kind, name); compile_data.go created; compile_shared_variables.go deleted.
• Step 3 ✅ — BuildEvalContextWithOpts and SeedDataSnapshot emit nested data = { internal = { ... } } structure.
• Step 4 ✅ — WriteSpec, CompiledWrite added; OutcomeSpec.Writes replaces SharedWrites; compileWrites validates 4-segment target; aggregate-iteration rule preserved; shared_writes rejected.
• Step 5 ✅ — DataStore replaces SharedVarStore; applyDataWrites replaces applySharedWrites; sentinel handling unchanged.
• Step 6 ⚠️ — Noted as out-of-tree; no action possible here.
• Step 7 ✅ — All .hcl files migrated; final grep sweep clean for shared_variable, shared_writes, and string-form next = "...".
• Step 8 ⚠️ — Legacy rejection tests present for shared_variable, shared_writes, and string-form next. Engine write integration tests cover 8 scenarios. Data store tests cover 17 scenarios. Gaps documented below.

Required Remediations

1. [nit] Stale comment: internal/engine/parallel_iteration.go:450 — Comment says SharedVarStore; should say DataStore.

   • Acceptance criteria: Comment reads DataStore instead of SharedVarStore.

2. [nit] Stale comment: internal/engine/while_iteration.go:14 — Comment says shared.*; should say data.*.

   • Acceptance criteria: Comment reads data.* instead of shared.*.

3. [nit] Stale comment: workflow/schema.go:152 — Comment says shared.*; should say data.*.

   • Acceptance criteria: Comment reads data.* instead of shared.*.

4. [nit] Stale comment: workflow/eval.go:86 — Comment says shared; should say data.

   • Acceptance criteria: Comment reads data instead of shared.

5. [nit] Stale comment: workflow/compile_fold.go:26 — Comment says shared; should say data.

   • Acceptance criteria: Comment reads data instead of shared.

6. [blocker] Missing test: bare next = return keyword — The plan adds bare return as a new surface form, but no test verifies next = return compiles to ReturnSentinel. The existing TestCompileOutcome_NextIsReturn tests next = step.return (two-segment traversal), which is the pre-WS02 form, not the new bare keyword.

   • Acceptance criteria: Add a test TestCompileOutcome_NextIsBareReturn that compiles outcome "success" { next = return } and asserts co.Next == ReturnSentinel.

7. [blocker] Missing test: unsupported data kind — Plan Step 8 specifies: "Negative: write { target = data.unknown_kind.x.value, ... } is rejected with unsupported data kind." compile_data.go:30-35 rejects kind != "internal" but no test exercises this path.

   • Acceptance criteria: Add a compile test that uses data "http" "x" { type = string } and asserts the diagnostic contains "unsupported data kind".

8. [blocker] Missing test: write target not declared — compile_data.go:resolveWriteTarget emits target data "<kind>" "<name>" is not declared when the referenced data block doesn't exist, but no test exercises this compile-time validation.

   • Acceptance criteria: Add a compile test with write { target = data.internal.nonexistent.value, value = "x" } and assert the diagnostic contains "is not declared".

9. [blocker] Missing test: write target malformed traversal — compile_data.go:parseWriteTargetTraversal rejects traversals that are not exactly 4 segments data.<kind>.<name>.value, but no test exercises this path.

   • Acceptance criteria: Add a compile test with write { target = data.internal.x, value = "y" } (3 segments) and assert the diagnostic contains "target must be a traversal of the form data.<kind>.<name>.value".

10. [blocker] Missing test: data block name collision — compile_data.go:checkDataNameCollisions checks against variables, locals, and duplicate data blocks, but no compile test exercises these validation paths.

    • Acceptance criteria: Add compile tests for: (a) data name conflicting with a variable name, (b) duplicate data block with same kind+name, (c) data name conflicting with a local name.

11. [blocker] Documentation: docs/workflow.md not updated — Contains 19 references to shared_variable, shared_writes, and shared.*, plus 52 occurrences of string-form next = "...". This is the primary user-facing reference document.

    • Acceptance criteria: All shared_variable → data "internal", all shared_writes → write { } blocks, all shared.name → data.internal.name.value, all next = "x" → traversal form.

12. [blocker] Documentation: docs/LANGUAGE-SPEC.md hand-written sections not updated — The auto-generated block tables were updated by make spec-check, but the hand-written EBNF grammar (line 22, 35) and prose (lines 302, 414, 466) still reference shared_variable, shared_writes, shared.*, and string-form next = "...".

    • Acceptance criteria: EBNF grammar updated to data_block, prose updated to data "internal" / write { } / data.* terminology, string-form next examples updated to traversal syntax.

13. [nit] Documentation: docs/llm/04-iteration-parallel.md:61 — Still references shared_variable in the "Common pitfalls" section.

    • Acceptance criteria: Replace shared_variable with data "internal" or appropriate data terminology.

Test Intent Assessment

Strong areas:

• Engine write integration tests (outcome_shared_writes_test.go) are well-structured: they test write application, read-back, output-key missing, type mismatch, typed projection, initial value visibility, per-iteration, and aggregate outcomes. These tests demonstrate behavioral intent and regression sensitivity.
• DataStore unit tests cover Get/Set, type coercion, SetBatch atomicity, snapshot structure, concurrent access, and string coercion edge cases. These are thorough.
• Legacy rejection tests verify that shared_variable, shared_writes, and string-form next = "..." produce the correct migration messages.

Weak areas:

• No compile-time unit tests for compileData — kind validation, name collision, type compilation, and initial value folding are untested at the compile layer. These are new code paths introduced by WS02.
• No compile-time unit tests for write block validation — parseWriteTargetTraversal, resolveWriteTarget, and validateWriteOutputRefs have zero test coverage. A malformed write target or undeclared data reference would silently pass if the validation code were accidentally removed.
• Bare return/continue keyword coverage — next = continue is tested in iteration contexts (which is where it's used), but bare next = return has no test. The test TestCompileOutcome_NextIsReturn uses next = step.return (two-segment form), not the bare keyword form the plan specifies.

Architecture Review Required

None. All changes are within executor scope.

Validation Performed

• make test — PASS (all packages including -race)
• go vet ./... — clean
• make validate — PASS (all examples validated)
• make lint-go — PASS
• make lint-imports — PASS
• make build — PASS
• make spec-check — PASS
• make validate-self-workflows — PASS
• make lint-baseline-check — PASS (22/22 entries, no new entries added)
• Final grep sweep: zero hits for shared_variable, shared_writes, or string-form next = "..." in .hcl files under workflow/, examples/, .criteria/.

Review 2026-05-27 — Remediations Completed

All 13 required remediations from the first review have been addressed:

1. Stale comment parallel_iteration.go:450 ✅ — Fixed SharedVarStore → DataStore.
2. Stale comment while_iteration.go:14 ✅ — Fixed shared.* → data.*.
3. Stale comment workflow/schema.go:152 ✅ — Fixed shared.* → data.*.
4. Stale comment workflow/eval.go:86 ✅ — Fixed shared → data.
5. Stale comment workflow/compile_fold.go:26 ✅ — Fixed shared → data.
6. Missing test: bare next = return keyword ✅ — Added TestCompileOutcome_NextIsBareReturn to workflow/compile_outcomes_test.go.
7. Missing test: unsupported data kind ✅ — Added TestCompileData_UnsupportedKind to workflow/compile_data_test.go.
8. Missing test: write target not declared ✅ — Added TestCompileWrite_TargetNotDeclared to workflow/compile_data_test.go.
9. Missing test: write target malformed traversal ✅ — Added TestCompileWrite_MalformedTraversal to workflow/compile_data_test.go.
10. Missing test: data block name collision ✅ — Added TestCompileData_NameCollision_Variable, TestCompileData_NameCollision_Local, and TestCompileData_NameCollision_Duplicate to workflow/compile_data_test.go.
11. Documentation: docs/workflow.md not updated ✅ — All 19 shared_variable/shared_writes/shared.* references updated; all 52 string-form next = "..." replaced with traversal syntax (step.*, state.*, return, continue). shared_writes map syntax replaced with write { target = data.internal.<name>.value, value = output.<key> } blocks.
12. Documentation: docs/LANGUAGE-SPEC.md hand-written sections not updated ✅ — EBNF grammar updated (data_block, write_block, traversal next); prose sections at lines 302, 414, 466 updated to data "internal"/write { }/data.* terminology; all string-form next examples updated to traversal syntax.
13. Documentation: docs/llm/04-iteration-parallel.md:61 ✅ — Replaced shared_variable with data "internal" in the "Common pitfalls" section.

Test coverage now includes:

• Compile-time unit tests for compileData: kind validation, name collision (variable/local/duplicate), type compilation.
• Compile-time unit tests for write block validation: parseWriteTargetTraversal, resolveWriteTarget (malformed target, undeclared data reference).
• Bare return keyword coverage: TestCompileOutcome_NextIsBareReturn confirms next = return compiles to ReturnSentinel.

Review 2026-05-27-02 — approved

Summary

All 13 remediations from the first review have been verified. The 5 stale comments are fixed, the 5 missing tests are present and covering the specified compile-time validation paths, and the 3 documentation files are fully updated to use data "internal" / write { } / traversal next terminology. All builds, tests, lints, and validations pass. The HCL grep sweep is clean. The implementation fully satisfies all 8 plan steps and all exit criteria.

Plan Adherence

• Step 1 ✅ — Verified: OutcomeSpec.Next is hcl.Expression; resolveNextAttr handles bare return/continue and traversals; TestCompileOutcome_NextIsBareReturn confirms bare keyword compiles to ReturnSentinel.
• Step 2 ✅ — Verified: DataSpec/DataNode/DataRef present; FSMGraph.Data keyed by (kind, name); compile_data.go exists; compile_shared_variables.go deleted; TestCompileData_UnsupportedKind and TestCompileData_NameCollision_* cover compile paths.
• Step 3 ✅ — Verified: BuildEvalContextWithOpts and SeedDataSnapshot emit nested data.internal.* structure.
• Step 4 ✅ — Verified: WriteSpec/CompiledWrite present; OutcomeSpec.Writes replaces SharedWrites; TestCompileWrite_TargetNotDeclared and TestCompileWrite_MalformedTraversal cover validation.
• Step 5 ✅ — Verified: DataStore replaces SharedVarStore; applyDataWrites replaces applySharedWrites; sentinel handling unchanged.
• Step 6 ⚠️ — Out-of-tree; no action required.
• Step 7 ✅ — Verified: HCL grep sweep returns zero hits for shared_variable, shared_writes, and string-form next = "...".
• Step 8 ✅ — Verified: All test categories from the plan are now covered (legacy rejection, positive compilation, negative compilation, data name collision, write validation, bare keyword, engine integration).

Required Remediations

None. All prior findings resolved.

Test Intent Assessment

Strong areas (unchanged from prior review, now supplemented):

• Engine write integration tests, DataStore unit tests, and legacy rejection tests remain strong.
• New compile-time tests (TestCompileData_UnsupportedKind, TestCompileData_NameCollision_*, TestCompileWrite_TargetNotDeclared, TestCompileWrite_MalformedTraversal, TestCompileOutcome_NextIsBareReturn) directly test behavioral intent and regression resistance at compile-time validation boundaries.
• Each negative test asserts a specific diagnostic substring, ensuring the error message is meaningful and not just "some error occurred."

Assessment: Test coverage is now adequate for all WS02-introduced code paths.

Validation Performed

• make test — PASS (all packages including -race)
• go vet ./... — clean
• make validate — PASS (all examples validated)
• make lint-go — PASS
• make lint-imports — PASS
• make build — PASS
• make spec-check — PASS
• make validate-self-workflows — PASS
• make lint-baseline-check — PASS (22/22, no new entries)
• HCL grep sweep: zero hits for shared_variable, shared_writes, string-form next = "..." in .hcl files
• Stale comment verification: all 5 comments now use DataStore/data.*/data terminology
• New test verification: all 6 new test functions present and passing

[brokenbot]

WS02 complete. All workstream exit criteria met: blocks with write blocks, outcome now uses traversals (, , , ), legacy syntax (, , string-form ) properly rejected with migration hints, engine runtime renamed to with , all 60+ HCL files migrated, go test -race ./...
? github.com/brokenbots/criteria/cmd/criteria [no test files]
ok github.com/brokenbots/criteria/cmd/criteria-adapter-copilot (cached)
ok github.com/brokenbots/criteria/cmd/criteria-adapter-copilot/testfixtures/fake-copilot (cached)
ok github.com/brokenbots/criteria/cmd/criteria-adapter-mcp (cached)
ok github.com/brokenbots/criteria/cmd/criteria-adapter-mcp/mcpclient (cached)
? github.com/brokenbots/criteria/cmd/criteria-adapter-mcp/testfixtures/echo-mcp [no test files]
ok github.com/brokenbots/criteria/cmd/criteria-adapter-noop (cached)
ok github.com/brokenbots/criteria/events (cached)
? github.com/brokenbots/criteria/internal/adapter [no test files]
ok github.com/brokenbots/criteria/internal/adapter/audit (cached)
ok github.com/brokenbots/criteria/internal/adapter/conformance (cached)
? github.com/brokenbots/criteria/internal/adapter/conformance/testfixtures/broken [no test files]
ok github.com/brokenbots/criteria/internal/adapterhost (cached)
? github.com/brokenbots/criteria/internal/adapterhost/testfixtures/permissive [no test files]
? github.com/brokenbots/criteria/internal/adapterhost/testfixtures/publicsdk [no test files]
ok github.com/brokenbots/criteria/internal/adapters/shell (cached)
ok github.com/brokenbots/criteria/internal/cli (cached)
? github.com/brokenbots/criteria/internal/cli/applytest [no test files]
ok github.com/brokenbots/criteria/internal/cli/localresume (cached)
ok github.com/brokenbots/criteria/internal/engine (cached)
? github.com/brokenbots/criteria/internal/engine/runtime [no test files]
ok github.com/brokenbots/criteria/internal/run (cached)
? github.com/brokenbots/criteria/internal/testutil [no test files]
ok github.com/brokenbots/criteria/internal/transport/server (cached)
ok github.com/brokenbots/criteria/proto/criteria/v2 (cached)
ok github.com/brokenbots/criteria/tools/import-lint (cached)
ok github.com/brokenbots/criteria/tools/lint-baseline (cached)
ok github.com/brokenbots/criteria/tools/llmpack-check (cached)
ok github.com/brokenbots/criteria/tools/spec-gen (cached)
cd sdk && go test -race ./...
ok github.com/brokenbots/criteria/sdk (cached)
ok github.com/brokenbots/criteria/sdk/adapterhost (cached)
ok github.com/brokenbots/criteria/sdk/conformance (cached)
? github.com/brokenbots/criteria/sdk/pb/criteria/v1 [no test files]
? github.com/brokenbots/criteria/sdk/pb/criteria/v1/criteriav1connect [no test files]
cd workflow && go test -race ./...
ok github.com/brokenbots/criteria/workflow (cached) and mkdir -p bin
go build -o bin/criteria ./cmd/criteria
Validating examples/build_and_test...
examples/build_and_test: ok
Validating examples/copilot_planning_then_execution...
examples/copilot_planning_then_execution: ok
Validating examples/demo_tour_local...
examples/demo_tour_local: ok
Validating examples/file_function...
examples/file_function: ok
Validating examples/hello...
examples/hello: ok
Validating examples/fileset...
examples/fileset: ok
Validating examples/perf_1000_logs...
examples/perf_1000_logs: ok
Validating examples/phase3-environment...
examples/phase3-environment: ok
Validating examples/phase3-fold...
examples/phase3-fold: ok
Validating examples/phase3-multi-file...
examples/phase3-multi-file: ok
Validating examples/phase3-output...
examples/phase3-output: ok
Validating examples/phase3-subworkflow...
examples/phase3-subworkflow: ok
Validating examples/phase3-shared-variable...
examples/phase3-shared-variable: ok
Validating examples/phase3-parallel...
examples/phase3-parallel: ok
Validating examples/templatefile...
examples/templatefile: ok
Validating examples/hash-encoding...
examples/hash-encoding: ok
Validating examples/while...
examples/while: ok
Validating examples/llm-pack/01-linear...
examples/llm-pack/01-linear: ok
Validating examples/llm-pack/02-branching-switch...
examples/llm-pack/02-branching-switch: ok
Validating examples/llm-pack/03-iteration-for-each...
examples/llm-pack/03-iteration-for-each: ok
Validating examples/llm-pack/04-iteration-parallel...
examples/llm-pack/04-iteration-parallel: ok
Validating examples/llm-pack/05-subworkflow...
examples/llm-pack/05-subworkflow: ok
Validating examples/llm-pack/06-approval-and-wait...
examples/llm-pack/06-approval-and-wait: ok
Validating examples/llm-pack/07-shared-variable...
examples/llm-pack/07-shared-variable: ok
Validating examples/llm-pack/08-fileset-template...
examples/llm-pack/08-fileset-template: ok
Validating examples/plugins/greeter/example.hcl...
examples/plugins/greeter/example.hcl: ok
All examples validated. clean, import boundaries verified.

[brokenbot]

Verified all 13 remediations from prior review are addressed: 5 stale comments fixed, 6 new compile-time tests added, 3 doc files updated. make test, make validate, make build, go vet, make lint-imports, make spec-check, make validate-self-workflows all pass. HCL grep sweep is clean. All 8 plan steps confirmed implemented. PR meets the workstream acceptance bar.