feat(dynamic)!: dynamic-build streamlining — delta reviews, honest findings/coverage, wave hardening, skill re-entrancy fixes#252
Conversation
…run removal
Implements all 9 changes to the dynamic-build MDS command sources:
C1: Delta-scoped review cycles — cycle 1 = full branch diff; cycles 2+ = DELTA REVIEW
of fix commits only (reviewBaseSha..HEAD). preFixSha recorded before fixes.
C2: Review-failure honesty — structured reviewer result contract (reviewed: true);
dead reviewer → retry once → coverageGaps; staggered spawning in chunks of 5;
early-exit only when allFindings.length === 0 && coverageGaps.length === 0;
engine_output_schema gains reviewCoverage + extended escalation type enum.
C3: Batched fix Coders — same-file batches, max 5 findings per batch, independent
batches in parallel; findings disposition two-bucket (FIXED / SURVIVING);
fixedFindings[] added to schema; Synthesizer prompt uses "Findings disposition" heading.
C4: Gate-2 fix-and-continue — evalVerdict/testerVerdict set to "FAIL-FIXED" after fix,
no re-evaluation by design; removes the doctrine-vs-cadence contradiction.
C5: Wave hardening — reader tier upgraded to agentType "Designer" (opus); vacuous-truth
rule (ALWAYS ready); empty-ready-set guard re-asks once; cascade quarantine doctrine;
per-ticket try/catch wave sketch in dynamic-build.mds ("never kills the wave").
C6: Build-cost doctrine — step 0 pre-load Monitor; exit-code honesty; bounded polling
with re-arm; NEVER wrapped in sh -c/bash -c; Cheapest-sufficient validation;
One build gate per phase.
C7: Remove --dry-run from dynamic-build/plan/tickets/wave (argument-hints + detect steps);
retained in dynamic-profile; applies ADR-003 (leave-the-end-state).
C8: Run-unique node --check scratch path — /tmp/df-wf-check-<meta.name>-<epoch-seconds>.js
instead of fixed /tmp/df-wf-check.js.
C9: Engine invariant 6 — No unauthorized GitHub side-effects.
All 29 build-mds tests pass. Simplifier/Scrutinizer exactly 2× each in compiled output.
C10: Add guard line to all 16 shared agents stating that frontmatter
skills are already active and the Skill tool must never be re-invoked
for them. Observed failure mode (PF-002): an agent that gets the
're-entrancy guard string' back treats it as a terminal instruction and
returns with 0 tool uses while the workflow reports success.
Substantive edits:
- reviewer.md: change Skill() invocation to Read file instruction
(`~/.claude/skills/devflow:{FOCUS}/SKILL.md`); rephrase Apply
Decisions from "Follow the skill" to "Apply the algorithm (already
loaded)"; update focus-table header and frontmatter description.
- bug-analyzer.md: rephrase two preloaded-skill body references from
"Follow the ... skill" to "Apply the ... algorithm (already loaded)".
- designer.md: update frontmatter description ("preloaded mode skills");
rephrase boundary item to "Applying the preloaded mode skill".
- coder.md: remove devflow:boundary-validation and devflow:testing from
domain Skill() invocation lists (both are frontmatter-preloaded);
extend graceful-skip wording with "or returns 'already running'".
C6 mirror: update coder.md "Long-running commands" section to match
_engine.mds build_execution_doctrine() — adds step 0 (pre-load Monitor),
direct-invocation rule (NEVER wrap in sh -c/bash -c), exit-code honesty
note, bounded polling (ONE Monitor, re-arm ≤2×, then escalate), and
"one build gate per phase" doctrine.
code-review.mds: update Reviewer prompt to "Read the pattern skill file
~/.claude/skills/devflow:{focus}/SKILL.md — do not use the Skill tool".
Docs: add mutually-exclusive rule + guard-string failure signature to
skills-architecture.md; add anti-pattern #7 to agent-design.md.
Structural invariant verified: no agent Skill-invokes a frontmatter skill.
All tests pass: 27/27 skill-references + 29/29 build-mds.
…al guard Regression tests pinning the Phase 1–2 prose changes: build-mds.test.ts §12 — greps on compiled dynamic-build.md: - C1: DELTA REVIEW + reviewBaseSha delta-scope tracking - C2: reviewer result contract (reviewed: true, coverageGaps, chunk/stagger) - C3: Findings disposition replaces old survivingFindings raw dump - C4: FAIL-FIXED verdict label - C5: wave hardening (ALWAYS ready, cascade, never kills the wave, Designer reader) - C6: build doctrine (Cheapest-sufficient, One build gate per phase, NEVER wrapped, re-arm) - C8: run-unique scratch path (/tmp/df-wf-check.js fixed name gone) - C9: No unauthorized GitHub side-effects build-mds.test.ts §13 — --dry-run removal (C7): - dynamic-build/plan/tickets/wave must NOT contain --dry-run - dynamic-profile still contains --dry-run (untouched per plan) skill-references.test.ts: - Structural invariant (PF-002 guard): for every shared/agents/*.md, no Skill(skill="devflow:NAME") where NAME is in the agent's own frontmatter skills — frontmatter skills are pre-activated, re-invocation is a bug - Updated stale comment at Format 3 to reflect Phase 2 read-the-file design Co-Authored-By: Claude <noreply@anthropic.com>
…ents Remove `// C1:`–`// C4:` transition markers from dynamic-build.mds workflow script — these encoded which forensic change added a line, not what the code does. The descriptive text after the prefix is preserved and capitalised. Matches the project principle "leave the end-state, not the transition." Also remove "After Phase 2 redesign:" from the skill-references test comment for the same reason, and correct "§10's beforeAll" to "Earlier describe blocks" in build-mds.test.ts (§11 also compiles the file, making §10-only attribution inaccurate). Tests: 67 pass, 0 fail.
The fix phase overwrote survivingFindings each cycle (survivingFindings = notAddressed), but the delta-review model only re-reviews fix commits — an unaddressed finding from an earlier cycle can never re-surface, so overwriting silently dropped it and let overallVerdict falsely read PASS. Accumulate instead, matching the doctrine definition 'survivingFindings = only findings NOT addressed'.
…ew base _engine.mds review_loop(): add one sentence stating that when the pre-fix SHA from a prior cycle is missing/unrecorded, the review scope falls back to the wider full-branch base rather than a narrower delta. dynamic-build.mds: harden reviewScope construction — guard !reviewBaseSha so a falsy base never interpolates the string "null" into a delta prompt; falls back to full-branch scope instead (matches the doctrine).
Summary: PR #252 Code ReviewStatus: CHANGES_REQUESTED — 1 blocking issue, 4 should-fix items, several informational findings. Inline Comments SummaryPosted 6 consolidated inline comments for high-confidence findings (≥80%):
Should-Fix Items (60-79% confidence)Fix-batch failure detection reads
Delta-review silently re-inflates on unpinned Git-agent SHA (Performance 74%)
Reviewer roster hardcoded to 8 focuses — doctrine says use budget-scaled sizing (Performance 80%)
Review-loop closure does ~8 things in ~140 lines (Complexity 80% MEDIUM)
Build-execution doctrine duplicated across
Informational / Pre-Existing IssuesUntrusted GitHub issue bodies flow into Designer reader (Security MEDIUM, Pre-existing)
Reviewer.md hardcoded to user-scope skill path (Regression MEDIUM, Intentional trade-off)
Prose pins ≠ behavior tests (Testing LOW, informational)
Verified Sound (No Action)These were deep-checked and hold up:
Scores by Reviewer
Recommendation: CHANGES_REQUESTEDBefore Merge:
StrengthsThe C10 re-entrancy guard, wave hardening, FAIL-FIXED verdict, and the overall streamlining design are well-reasoned and thoroughly verified. The test suite (67/67 pass) is solid. The breaking changes (--dry-run removal, reviewed: true contract, FAIL-FIXED enum) are complete and residue-free. Once the C3 concurrency issue is fixed, this PR delivers genuine cost reduction (delta-review, dead-reviewer gating, findings disposition batching) without regression. Posted by: Claude Code Review Agent (devflow:git) |
…den wave skeleton
F1: Rewrite fix-batching to strictly group one file per set of sub-batches,
chunked at MAX_BATCH=5 within that file. Sub-batches for the SAME file
run sequentially (never two Coders editing the same file concurrently);
sub-batches for DISTINCT files run in parallel(). Findings with no file
field each get a singleton group. Adds 'file' field to reviewer result
contract and engine_output_schema survivingFindings/fixedFindings.
Deletes the false "(same-file batches already grouped)" comment.
Updates _engine.mds:review_loop doctrine and KNOWLEDGE.md:106 to match.
F2: Fix disposition check from '!result?.failed' (field Coder never returns)
to 'result?.status === "fixed"'. Pin the fix-Coder return contract to
{"status":"fixed"|"blocked","commitShas":[...],"unresolved":[...]} so
soft-failure can never be counted as FIXED and silently drop findings.
F3: Pin Git agent return to 'Return exactly: {"sha":"<40-hex>"}' and add a
40-hex regex validation before accepting preFixSha; fall back to the
wider prior base on invalid shape, never narrow the review window.
Soften _engine.mds:85 from "full-branch base" to "wider prior base"
(the un-advanced scope from the previous cycle — more accurate).
F4: Assign MAX_ROUNDS = Math.max(10, remainingTickets.length * 2 + 5) in
the wave skeleton (was referenced but never assigned — ReferenceError
on first run). Inline the empty-ready-set re-ask guard: re-ask once
with vacuous-truth rule on an empty ready set, break on second empty
read (deadlock). Tolerate both engineResult.verdict and .overallVerdict
in the merge gate check (SINGLE skeleton exposes overallVerdict).
F7: Wrap ticket data flowing into the wave Designer reader prompt inside
<untrusted-issue-body> containment markers with a one-line data-only
note. Add matching instruction to _wave.mds Step 1 documenting when
and how to apply the markers for issue bodies.
applies ADR-005 applies ADR-003 avoids PF-002
Co-Authored-By: Claude <noreply@anthropic.com>
…onsistency + install-scope safety PR #252 changed reviewer.md from Skill(skill="devflow:{FOCUS}") to Read ~/.claude/skills/devflow:{FOCUS}/SKILL.md. This was an over-correction: {FOCUS} resolves to runtime-selected non-frontmatter skills (security, architecture, etc.), which are PF-002-safe for Skill-tool invocation. The Read-by-path approach also hardcodes ~/.claude which breaks project/local-scope installs where skills live elsewhere. Reverts the four focus-skill-loading lines to use the Skill tool — matching coder.md and researcher.md which kept Skill() for their runtime-selected domain skills. The PF-002 re-entrancy guard line (C10) is preserved intact. avoids PF-002 (Skill tool on {FOCUS} is safe — {FOCUS} is never a frontmatter skill) applies ADR-003 (clean end-state: no contradictory "do NOT use the Skill tool" instruction alongside the restored Skill() call) Structural guard: npx vitest run tests/skill-references.test.ts — 28/28 pass
…den PF-002 guard to plugin agents
F6a: add positive presence guards to C8 test — toContain('df-wf-check-') and
toContain('run-unique scratch file') — so the test fails on deletion or rename
of the scratch-path mechanism, not just on reintroduction of the old fixed name.
F6b: give §12 (doctrine C1–C9) and §13 (--dry-run removal) their own beforeAll
that spawns build-mds.ts, mirroring §10/§11. Eliminates intra-file ordering
dependency: isolated/filtered vitest runs now self-build instead of ENOENT.
F6c: extend the PF-002 structural re-entrancy guard from shared/agents/*.md to
also cover tracked plugin agents (git ls-files 'plugins/*/agents/*.md'). Catches
re-entrant Skill() reintroductions in plugins/devflow-plan/agents/designer.md
which is hand-maintained and was modified in lockstep with shared/agents/designer.md
by this PR. Avoids PF-002.
Remove unnecessary `rawSha` intermediate variable (used once, immediately after assignment) and the redundant `typeof rawSha === "string"` guard (the 40-hex regex already rejects non-strings via ToString coercion). Behavior is identical — invalid shapes still fall back to reviewBaseSha.
| ``` | ||
| Agent(subagent_type="Reviewer", run_in_background=false): | ||
| "Review focusing on {focus}. Load the pattern skill for your focus from the Focus Areas table. | ||
| "Review focusing on {focus}. Read the pattern skill file ~/.claude/skills/devflow:{focus}/SKILL.md — do not use the Skill tool. |
There was a problem hiding this comment.
why are we not instructing to load using the skill tool? why read... we need to use the skill tool. all over to be honest.
There was a problem hiding this comment.
Agreed — fixed in 6991302, and this thread is the right place for the full context. The Read was a workaround for PF-002, a real observed failure: an agent that both preloads a skill via frontmatter skills: and invokes it via the Skill tool gets the re-entrancy guard string (devflow:X already running), treats it as a terminal instruction, and silently returns zero work while the workflow counts it as success. The earlier fix kept the preloads and prose-banned re-invocation — hence all the 'already active / do not re-invoke' notes, and this one spot that swapped the Skill call for a file Read.
6991302 resolves PF-002 the other way, structurally: no agent declares skills: frontmatter anymore; the Skill tool is the single loading mechanism, invoked at runtime at the point of need. Nothing is preloaded, so the guard can never fire, and every prohibition note is gone. This prompt now instructs Skill(skill="devflow:{focus}"), and the ~/.claude/skills/... availability Read-probe above it is deleted (skills install universally, and the hardcoded path broke project-scope installs). The PF-002 structural guard in tests/skill-references.test.ts now also fails CI if any agent reintroduces a skills: frontmatter block.
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in 80c68b0 — the reviewer prompt now instructs Skill(skill="devflow:{focus}"), matching reviewer.md, and the ~/.claude/skills Read-probe availability check is removed (skills install universally; that path also assumed user-scope installs). Frontmatter preloading is unchanged; the agent-note cleanup landed separately in 1506669.
|
|
||
| **Handle autonomously:** | ||
| - Loading assigned skill file | ||
| - Applying the preloaded mode skill (do NOT re-invoke via Skill tool) |
There was a problem hiding this comment.
why do we need this do not re-invoke? what if preload fails?
There was a problem hiding this comment.
Exactly the right question — under preload-plus-prohibition there was no answer to 'what if preload fails'. Fixed in 6991302: nothing is preloaded; the Designer explicitly loads its mode skill via the Skill tool (devflow:gap-analysis or devflow:design-review per MODE). A failed load is now a visible failed tool call in the transcript instead of a silent gap.
There was a problem hiding this comment.
Fixed in 1506669 — the parenthetical is deleted; the bullet now just reads 'Applying the preloaded mode skill'. On 'what if preload fails': frontmatter skills are injected by the Claude Code runtime at agent spawn; if one isn't installed the runtime skips it and the agent runs without that content — nothing hangs. And invoking a skill that isn't loaded simply loads it; the re-entrancy guard string only appears when it already is active. So no prohibition or fallback prose is needed in either direction.
|
|
||
| You are a proactive bug finding agent. Your focus area is specified in the prompt. You hunt for real bugs — not style issues — using a 5-step methodology that combines static analysis findings with semantic code understanding. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
again.. why do we need this note?
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
| - **STATIC_FINDINGS** (optional): Pre-computed static analysis output (Semgrep/Snyk/CodeQL results). Only provided to security analyzer. `(none)` for other focus types. | ||
| - **DECISIONS_CONTEXT** (optional): Compact index of active ADR/PF entries. `(none)` when absent. Use `devflow:apply-decisions` to Read full bodies on demand. | ||
| - **FEATURE_KNOWLEDGE** (optional): Pre-computed feature area context. Follow `devflow:apply-feature-knowledge`. | ||
| - **FEATURE_KNOWLEDGE** (optional): Pre-computed feature area context. Apply the `devflow:apply-feature-knowledge` algorithm (already loaded). |
There was a problem hiding this comment.
again... how do we know its already loaded, what if it isn't why are these notes "already loaded" needed.
There was a problem hiding this comment.
Fixed in 6991302 — the '(already loaded)' assumption is gone. The instruction is now load-then-apply: invoke Skill(skill="devflow:apply-decisions") (or apply-feature-knowledge) and apply its algorithm. Nothing is assumed to be pre-active.
There was a problem hiding this comment.
Fixed in 1506669 — the '(already loaded)' asides are deleted; the sentences now just say to apply the algorithm. The skills are injected at spawn via frontmatter; if one were ever missing, a Skill call would simply load it, so the aside carried no information.
| ## Apply Decisions | ||
|
|
||
| Follow the `devflow:apply-decisions` skill to scan the `DECISIONS_CONTEXT` index, Read full ADR/PF bodies on demand, and cite `applies ADR-NNN` / `avoids PF-NNN` inline in findings. Skip when `DECISIONS_CONTEXT` is `(none)`. | ||
| Apply the `devflow:apply-decisions` algorithm (already loaded) — scan the `DECISIONS_CONTEXT` index, Read full ADR/PF bodies on demand, and cite `applies ADR-NNN` / `avoids PF-NNN` inline in findings. Skip when `DECISIONS_CONTEXT` is `(none)`. |
There was a problem hiding this comment.
Fixed in 6991302 — the '(already loaded)' assumption is gone. The instruction is now load-then-apply: invoke Skill(skill="devflow:apply-decisions") (or apply-feature-knowledge) and apply its algorithm. Nothing is assumed to be pre-active.
There was a problem hiding this comment.
Fixed in 1506669 — '(already loaded)' aside deleted.
|
|
||
| You are an autonomous implementation specialist working on a feature branch. You receive a task with an execution plan from the orchestrator and implement it completely, including testing and committing. You operate independently, making implementation decisions without requiring approval for each step. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
why did we add these type of instructions all over...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| 2. **Load domain skills**: Before any analysis, invoke the Skill tool for each domain skill matching DOMAIN hint. If a Skill invocation fails, skip that skill and continue — domain skills are optional enhancements, not required for task completion. | ||
| - `backend` (TypeScript): `Skill(skill="devflow:typescript")`, `Skill(skill="devflow:boundary-validation")` | ||
| 2. **Load domain skills**: Before any analysis, invoke the Skill tool for each domain skill matching DOMAIN hint. If a Skill invocation fails or returns 'already running', skip that skill and continue — domain skills are optional enhancements, not required for task completion. |
There was a problem hiding this comment.
who said domain skills are optional enhancement? why do we need to give this way out. we can't just say nothing, to either direction. just instruct the model to load whats relevant to the task..
There was a problem hiding this comment.
Removed in 6991302 — no more 'optional enhancements' escape hatch and no more fail-and-skip clause. Step 2 is now a plain directive: load the skills matching the language and stack of the code being touched, with the available inventory listed as guidance.
There was a problem hiding this comment.
Fixed in 80c68b0 — the 'optional enhancements / skip and continue' hedge is gone. Step 2 is now a plain directive: load the domain skills matching the language and stack of the code being touched.
| - `backend` (Rust): `Skill(skill="devflow:rust")` | ||
| - `frontend`: `Skill(skill="devflow:react")`, `Skill(skill="devflow:typescript")`, `Skill(skill="devflow:accessibility")`, `Skill(skill="devflow:ui-design")` | ||
| - `tests`: `Skill(skill="devflow:testing")`, `Skill(skill="devflow:typescript")` | ||
| - `tests`: `Skill(skill="devflow:typescript")` |
There was a problem hiding this comment.
what? why for tests we load devflow typescript?
There was a problem hiding this comment.
Gone in 6991302 — the rigid DOMAIN→skill mapping table (including the nonsensical tests→typescript row) is removed. The instruction is now to match skills to the language/stack of the code actually being touched.
There was a problem hiding this comment.
Removed in 80c68b0 — the tests→typescript mapping row is gone.
|
|
||
| **Handle autonomously:** | ||
| - Loading assigned skill file | ||
| - Applying the preloaded mode skill (do NOT re-invoke via Skill tool) |
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Fixed in 1506669 — '(do NOT re-invoke via Skill tool)' deleted from the Boundaries bullet in both designer copies. Frontmatter preloading unchanged.
| validates, or applies anything on your behalf. The only executables you call are the three | ||
| ledger ops below. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. Dream's case was the degenerate one: its only frontmatter skill (apply-decisions) was never used by its body — it curates the ledger directly — so the entry was deleted outright. No note, no load instruction needed.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are an alignment validation specialist. You ensure implementations match the original request and execution plan. You catch missed requirements, scope creep, and intent drift. You report misalignments with structured details for the Coder agent to fix - you never fix code yourself. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are a Git/GitHub operations specialist. You handle all git and GitHub API interactions based on the operation specified. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| # Knowledge Agent | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. This agent's restricted tools: list didn't even include the Skill tool — the preload was the only mechanism, which is exactly the fragility you flagged. Skill is now in its tools list, skills: frontmatter is gone, and it loads what it needs explicitly at runtime.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are a multi-type research agent. You receive a research type, dynamically load the domain-specific research skill, execute the research methodology from that skill, and produce structured findings. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are an issue resolution specialist. You validate review issues and implement fixes with risk-proportional care. You fix everything that can be fixed safely. Tech debt is the absolute last resort — only for issues requiring complete architectural redesign. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are a universal code review agent. Your focus area is specified in the prompt. You dynamically load the pattern skill for your focus area, then apply the 6-step review process from `devflow:review-methodology`. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
| ## Apply Decisions | ||
|
|
||
| Follow the `devflow:apply-decisions` skill to scan the `DECISIONS_CONTEXT` index, Read full ADR/PF bodies on demand, and cite `applies ADR-NNN` / `avoids PF-NNN` inline in findings. Skip when `DECISIONS_CONTEXT` is empty or `(none)`. | ||
| Apply the `devflow:apply-decisions` algorithm (already loaded) — scan the `DECISIONS_CONTEXT` index, Read full ADR/PF bodies on demand, and cite `applies ADR-NNN` / `avoids PF-NNN` inline in findings. Skip when `DECISIONS_CONTEXT` is empty or `(none)`. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Fixed in 1506669 — '(already loaded)' aside deleted.
|
|
||
| You are a meticulous self-review specialist. You evaluate implementations against the 9-pillar quality framework and fix issues before handoff to Simplifier. You run in a fresh context after Coder completes, ensuring adequate resources for thorough review and fixes. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are an expert code simplification specialist focused on enhancing code clarity, consistency, and maintainability while preserving exact functionality. Your expertise lies in applying project-specific best practices to simplify and improve code without altering its behavior. You prioritize readable, explicit code over overly compact solutions. This is a balance that you have mastered as a result of your years as an expert software engineer. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are a codebase orientation specialist. You use `npx rskim` exclusively for code exploration — never Grep, Glob, or manual file searches. Your output gives implementation agents a clear map of relevant files, functions, and integration points. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. This agent's restricted tools: list didn't even include the Skill tool — the preload was the only mechanism, which is exactly the fragility you flagged. Skill is now in its tools list, skills: frontmatter is gone, and it loads what it needs explicitly at runtime.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are a synthesis specialist. You combine outputs from multiple parallel agents into clear, actionable summaries. You operate in six modes: exploration, planning, review, bug-analysis, design, and research. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are a scenario-based QA specialist. You design and execute acceptance tests that verify implementation behavior from the user's perspective. You test what was asked for, not implementation details. You report results with evidence — you never fix code yourself. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. This agent's restricted tools: list didn't even include the Skill tool — the preload was the only mechanism, which is exactly the fragility you flagged. Skill is now in its tools list, skills: frontmatter is gone, and it loads what it needs explicitly at runtime.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
|
|
||
| You are a validation specialist that runs build and test commands to verify code correctness. You discover validation commands from project configuration, execute them in order, and report structured results. You never fix issues - you only report them for other agents to fix. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
| You are a design analysis specialist. You detect gaps and anti-patterns in design documents, specifications, and implementation plans before implementation begins. Your mode and focus determine which skill you load and which analysis you perform. | ||
| You are a design analysis specialist. You detect gaps and anti-patterns in design documents, specifications, and implementation plans before implementation begins. Your mode and focus determine which preloaded skill applies and which analysis you perform. | ||
|
|
||
| The skills listed in your frontmatter are already active — never invoke the Skill tool for any of them; if a Skill call returns a guard string like 'already running', ignore it and proceed with your work. |
There was a problem hiding this comment.
same.... why is this important...
There was a problem hiding this comment.
Removed in 6991302. These notes were the mitigation for PF-002 (preloaded skill + Skill-tool invocation → re-entrancy guard string → silent zero-work bail). That's now resolved structurally instead: skills: frontmatter is gone from every agent and all skill loading is explicit runtime Skill-tool invocation, so the guard can never fire. Full context on the commands/code-review.mds thread.
There was a problem hiding this comment.
Deleted in 1506669. The note was belt-and-braces over PF-002, which is already handled structurally: no agent body instructs invoking a frontmatter-preloaded skill, and a structural test fails CI if that combination is reintroduced. Frontmatter skills: preloading is unchanged.
…ads, Skill tool everywhere Resolve PF-002 structurally instead of by prohibition prose. Agents no longer declare skills: frontmatter; every skill is loaded at runtime via the Skill tool at the point of need. With nothing preloaded, the skill re-entrancy guard can never fire. - shared/agents (16) + plugins/devflow-plan/agents/designer.md: remove skills: frontmatter and the "already active — never invoke the Skill tool" notes; add per-agent Skill-tool load instructions with when-conditions; fix "(already loaded)" asides to load-then-apply - coder.md: drop the "optional enhancements" hedge and the DOMAIN mapping table (incl. tests->typescript); plain directive to load the skills matching the language/stack of the code being touched - designer.md (both copies): mode skill loaded via Skill tool per MODE; scrub all "preloaded" language - knowledge/skimmer/tester: add Skill to restricted tools lists - code-review.mds: reviewer prompt loads pattern skill via Skill tool; remove the skill-availability Read-probe (skills install universally); worktree-support pointer -> Skill tool (also resolve.mds) - tests: PF-002 guard now also forbids any skills: frontmatter in agents; command files pinned to zero skill install-path references - docs: CLAUDE.md, agent-design, skills-architecture, and dynamic-workflow-engine KNOWLEDGE.md updated to the invoke-only rule Addresses PR #252 skill-loading review threads.
…er preloads, Skill tool everywhere" This reverts commit 6991302.
- code-review.mds reviewer prompt: load pattern skill via
Skill(skill="devflow:{focus}") instead of Read-by-install-path,
matching reviewer.md (24e26a3)
- code-review.mds: remove the skill-availability Read-probe — skills
install universally, and the ~/.claude path assumed user-scope;
conditional reviewers spawn from the file-type table alone
- coder.md step 2: plain directive to load domain skills matching the
language/stack of the code being touched (drops the "optional
enhancements, skip and continue" hedge); remove the tests→typescript
mapping row
Addresses PR #252 threads on code-review.mds:200, coder.md:63, coder.md:70.
Remove the identical "already active — never invoke the Skill tool" note from all 17 agent files, plus the "(already loaded)" asides (bug-analyzer, reviewer) and designer's "(do NOT re-invoke via Skill tool)" parenthetical. Frontmatter skills: preloading is unchanged. PF-002 protection is structural and sufficient without the prose: no agent body instructs invoking a frontmatter-preloaded skill, and the skill-references structural test fails CI if that combination is ever reintroduced. Addresses the remaining PR #252 skill-note review threads.
|
|
||
| This is LLM judgment — the agent reads like a person would, not a graph algorithm. | ||
|
|
||
| **Rationale for Designer tier:** a haiku-tier reader once quarantined 10 independent tickets as "blocked" because nothing had merged yet. Dependency reasoning requires the judgment of a senior reviewer, not a fast-read pass. |
There was a problem hiding this comment.
This is also a weird comment, why not just instruct to use opus because dependency reasoning requires the high level judgment - why do we need this weird haiku example...
There was a problem hiding this comment.
Fixed in 834fea5 — the line now just states the rule: 'use opus — dependency reasoning requires high-level judgment, not a fast-read pass.' Anecdote gone.
| - `backend` (Python): `Skill(skill="devflow:python")` | ||
| - `backend` (Rust): `Skill(skill="devflow:rust")` | ||
| - `frontend`: `Skill(skill="devflow:react")`, `Skill(skill="devflow:typescript")`, `Skill(skill="devflow:accessibility")`, `Skill(skill="devflow:ui-design")` | ||
| - `tests`: `Skill(skill="devflow:testing")`, `Skill(skill="devflow:typescript")` |
There was a problem hiding this comment.
Why was the tests removed? the testing skill should stay..
tests:Skill(skill="devflow:testing")
There was a problem hiding this comment.
The testing skill was never removed — devflow:testing is in coder's frontmatter skills: and is preloaded on every run (the Coder writes tests in every task, so it's always in context). What 80c68b0 removed was the tests→typescript row. Re-adding the row as Skill(skill="devflow:testing") would invoke an already-preloaded skill — the PF-002 re-entrancy combination the structural test blocks. Per discussion: leaving it as is; step 2's directive already covers loading the language skill of the code under test.
1 participant
Summary
Six forensic defects in the dynamic-build engine were identified and fixed:
reviewBaseShaand scope toDELTA REVIEW: fix commits onlyon every cycle after the first.reviewed: true+filesExamined+findings; a result missingreviewed: trueis DEAD and retried once, then recorded as acoverageGapsentry (which blocks PASS).JSON.stringify(survivingFindings)with no grouping or cap. Replaced with a structured two-bucket "Findings disposition" section and amax 5 findings per batchbatching rule.FAIL-FIXEDso the run report is honest about which gates found issues that were fixed vs truly passed.--dry-runfrom dynamic-build/plan/tickets/wave argument-hint and detect steps; retained in dynamic-profile (it controls profile generation, not dry runs)./tmp/df-wf-check.jsreused the same filename across runs, tripping write guards. Replaced with/tmp/df-wf-check-<name>-<epoch>.js— unique every run.'already running'). Added the guard line to all 16 agents and removed the two preloaded skills from coder.md's domain-skill invocation list.Breaking Changes
--dry-runremoved from/dynamic-build,/dynamic-plan,/dynamic-tickets,/dynamic-wave(was non-functional per forensics — the flag was listed in argument-hint but no code path read it).FAIL-FIXEDverdict label: reviewers and downstream consumers that matched against"PASS" | "FAIL"must now handle the third value.{ focus, reviewed: true, filesExamined, findings }— a result withoutreviewed: trueis now treated as DEAD (was silently accepted before).Test Plan
toContain/not.toContaingreps on compileddynamic-build.mdpinning C1–C9 exact prose.--dry-runabsence in 4 dynamic hosts, presence indynamic-profile.md.shared/agents/*.md, zeroSkill(skill="devflow:NAME")where NAME is in the agent's frontmatter skills — permanent regression guard for PF-002.npx vitest run tests/build-mds.test.ts tests/skill-references.test.ts tests/build.test.ts).Out of Scope
Per user direction, the following are deliberately excluded from this PR: