Follow-up tracking issue for structural/DSL technical debt in the agent-advisor skill, raised by @icarthick during the review of #147. Per that review these are pre-existing debt, not regressions introduced by #147, and were explicitly deferred out of that PR to keep it focused. The two items the reviewer flagged as "fix here" (the dropped _check_phase_completed predecessor gates on clarify and generate) were addressed in #147 itself; the three below are the deferred ones.
Verified against the code at #147's head — all three reproduce as described.
1. No-op fragment + sibling-skill inline (migration-plan)
references/phases/migration-plan/migration-plan.md declares migration-plan-gcp-constraints.md as a _fragment (always-trigger) with zero _contributes, and the same file is also body-loaded at Step 2.5 — so it is double-loaded. It writes no artifact; it is a reference doc mislabeled as a work unit (the single-creator validator can't catch this — it only walks from _produces).
- Fix: relocate it to
references/migration-plan/, drop it from _fragments[], keep the Step 2.5 body-load. Not "add a _contributes" — that would fabricate a false contribution; the assembler is the real creator of migration-plan-injection.json.
- The larger gap: this phase inlines the entire gcp-to-aws backbone in prose, hard-coding gcp-to-aws's file layout in a path table with zero CI visibility — a gcp-to-aws refactor breaks agent-advisor silently. The DSL has no cross-skill-delegation construct; this motivates a
_delegates_to-style primitive. Likely warrants its own RFC.
2. .phase-status.json declared as a phase _produces
intake.md / intake-assemble.md declare (and gate on) the interpreter-owned state file .phase-status.json as a phase artifact under _produces. The run-root binding legitimizes the location, not the head being the state file's creator (cf. heroku, whose head produces its domain artifact, never the state file). Lower stakes now that bindings exist, but it still conflates run state with a deliverable.
3. No structural guards on the long backbone
agent-advisor has no _re_entry_guard / _forbids_files / _check_single_active_phase anywhere (verified: 0 occurrences, vs. ~20 across 7 phase files in heroku-to-aws). The chain is now long (intake → … → design → estimate → generate → migration-plan → poc); re-running an upstream phase silently staleifies expensive downstream artifacts. heroku guards exactly this. Adding the guard set across the backbone is a systematic enhancement, not a point fix.
Source: review comments on #147 by @icarthick (review 4735130775) and the inline thread on migration-plan.md.
Follow-up tracking issue for structural/DSL technical debt in the
agent-advisorskill, raised by @icarthick during the review of #147. Per that review these are pre-existing debt, not regressions introduced by #147, and were explicitly deferred out of that PR to keep it focused. The two items the reviewer flagged as "fix here" (the dropped_check_phase_completedpredecessor gates onclarifyandgenerate) were addressed in #147 itself; the three below are the deferred ones.Verified against the code at #147's head — all three reproduce as described.
1. No-op fragment + sibling-skill inline (migration-plan)
references/phases/migration-plan/migration-plan.mddeclaresmigration-plan-gcp-constraints.mdas a_fragment(always-trigger) with zero_contributes, and the same file is also body-loaded at Step 2.5 — so it is double-loaded. It writes no artifact; it is a reference doc mislabeled as a work unit (the single-creator validator can't catch this — it only walks from_produces).references/migration-plan/, drop it from_fragments[], keep the Step 2.5 body-load. Not "add a_contributes" — that would fabricate a false contribution; the assembler is the real creator ofmigration-plan-injection.json._delegates_to-style primitive. Likely warrants its own RFC.2.
.phase-status.jsondeclared as a phase_producesintake.md/intake-assemble.mddeclare (and gate on) the interpreter-owned state file.phase-status.jsonas a phase artifact under_produces. The run-root binding legitimizes the location, not the head being the state file's creator (cf. heroku, whose head produces its domain artifact, never the state file). Lower stakes now that bindings exist, but it still conflates run state with a deliverable.3. No structural guards on the long backbone
agent-advisorhas no_re_entry_guard/_forbids_files/_check_single_active_phaseanywhere (verified: 0 occurrences, vs. ~20 across 7 phase files in heroku-to-aws). The chain is now long (intake → … → design → estimate → generate → migration-plan → poc); re-running an upstream phase silently staleifies expensive downstream artifacts. heroku guards exactly this. Adding the guard set across the backbone is a systematic enhancement, not a point fix.Source: review comments on #147 by @icarthick (review 4735130775) and the inline thread on
migration-plan.md.