You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
cleanup: spec-to-eval decoupling, misplaced packages, dead engine code
Architecture review findings that are all "trim the fat" rather than
bugs. Batched here so they don't get lost.
Layering
spec imports lang/eval (internal/spec/converter.go:9). TypeConverter/ConvertContext/ConverterMap couple the core
interface package to the evaluator's value representation, so
everything touching spec (including all of render) transitively
depends on eval. It's a linker concern; move it to linker (or a
small adjacent package) and let step packages register there.
internal/render/template is not render. It's a generic text/template wrapper used by step/template to render target
file content, not CLI output. Move to a neutral home (e.g. internal/template).
target.Port/target.Mount are declarative config value
types pulled into link-time conversion (internal/linker/ convert.go:12-35, fields.go:93-95). They'd sit better in spec next to PkgSourceRef, which models the same idea.
New test/rules invariant: the CLAUDE.md package boundaries
(lang never imports engine/target, source never imports target,
spec never imports lang/eval, render never imports engine) as an
import-graph test. Would have caught the spec-to-eval edge when
it was introduced.
Dead code
scheduler.results (internal/engine/exec.go:88,216):
append-only, never read.
triggerBy (internal/engine/hooks.go:30,46,109): written
twice, never read.
capabilityTarget panic stubs (internal/engine/plan.go: 410-498): ~90 lines that only partially satisfy the target
sub-interfaces (missing Mkdir, ChmodRecursive, ChownRecursive), so target.Must's BUG panic fires anyway. A
bare Capabilities()-only struct behaves identically.
Vestigial scope params on emitEngineDiagnostic/ emitPlanDiagnostic (internal/engine/errors.go:165-184),
self-acknowledged in comments.
Consistency nits
container.Instance{} (internal/engine/registry.go:39)
violates the repo's own "avoid bare Instance" naming rule; only
step kind not named after its kind.
Duplicated helpers: resourceKindName
(engine/deploy_graph.go:277) vs resourceKindLabel
(engine/duplicate_promises.go:102); bespoke findCycle
(deploy_graph.go:160) next to generic detectCycles
(cycles.go:14).
planHooks/validateHooks iterate the Hooks map
(plan.go:312,360); multi-error diagnostic order varies run to
run.
engine.Resolve doc comment says "produces a Config from a
Config" (config.go:142).
spec's optional-interface count is ~11 plus engine-private
ones; add a doc table in spec/doc.go listing every optional
interface a step author can implement.
streamSink.stop() closes the producer-facing channel
(internal/render/cli/stream.go:74,133-138); an Emit after Flush is a send-on-closed-channel panic. Currently safe by
wiring; make it structural (closed flag or select-on-done).
cleanup: spec-to-eval decoupling, misplaced packages, dead engine code
Architecture review findings that are all "trim the fat" rather than
bugs. Batched here so they don't get lost.
Layering
specimportslang/eval(internal/spec/converter.go:9).TypeConverter/ConvertContext/ConverterMapcouple the coreinterface package to the evaluator's value representation, so
everything touching spec (including all of render) transitively
depends on eval. It's a linker concern; move it to
linker(or asmall adjacent package) and let step packages register there.
internal/render/templateis not render. It's a generictext/templatewrapper used bystep/templateto render targetfile content, not CLI output. Move to a neutral home (e.g.
internal/template).target.Port/target.Mountare declarative config valuetypes pulled into link-time conversion (
internal/linker/ convert.go:12-35,fields.go:93-95). They'd sit better inspecnext toPkgSourceRef, which models the same idea.test/rulesinvariant: the CLAUDE.md package boundaries(lang never imports engine/target, source never imports target,
spec never imports lang/eval, render never imports engine) as an
import-graph test. Would have caught the spec-to-eval edge when
it was introduced.
Dead code
scheduler.results(internal/engine/exec.go:88,216):append-only, never read.
triggerBy(internal/engine/hooks.go:30,46,109): writtentwice, never read.
capabilityTargetpanic stubs (internal/engine/plan.go: 410-498): ~90 lines that only partially satisfy the targetsub-interfaces (missing
Mkdir,ChmodRecursive,ChownRecursive), sotarget.Must's BUG panic fires anyway. Abare
Capabilities()-only struct behaves identically.emitEngineDiagnostic/emitPlanDiagnostic(internal/engine/errors.go:165-184),self-acknowledged in comments.
Consistency nits
container.Instance{}(internal/engine/registry.go:39)violates the repo's own "avoid bare
Instance" naming rule; onlystep kind not named after its kind.
resourceKindName(
engine/deploy_graph.go:277) vsresourceKindLabel(
engine/duplicate_promises.go:102); bespokefindCycle(
deploy_graph.go:160) next to genericdetectCycles(
cycles.go:14).planHooks/validateHooksiterate theHooksmap(
plan.go:312,360); multi-error diagnostic order varies run torun.
engine.Resolvedoc comment says "produces a Config from aConfig" (
config.go:142).spec's optional-interface count is ~11 plus engine-privateones; add a doc table in
spec/doc.golisting every optionalinterface a step author can implement.
streamSink.stop()closes the producer-facing channel(
internal/render/cli/stream.go:74,133-138); anEmitafterFlushis a send-on-closed-channel panic. Currently safe bywiring; make it structural (closed flag or select-on-done).