From db9beba20060c71e97b58d0817c9ff002eb9adc1 Mon Sep 17 00:00:00 2001 From: satyaborg Date: Tue, 14 Jul 2026 16:27:20 +1000 Subject: [PATCH 1/2] feat: enforce spec proof obligations --- README.md | 2 +- devloop | 133 +++++++++++++----- scripts/devloop_test.sh | 100 ++++++++++++- skills/devloop-review/SKILL.md | 19 ++- skills/devloop-spec/SKILL.md | 53 ++++++- .../devloop-spec/references/spec-template.md | 23 ++- 6 files changed, 275 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 21838cd..95b4626 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ When you pick a spec from the interactive menu, devloop uses your configured run ## Specs -A good spec is short, concrete, and verifiable. Start from [`skills/devloop-spec/references/spec-template.md`](skills/devloop-spec/references/spec-template.md). The bundled `devloop-spec` skill can also render a sibling HTML companion with [`skills/devloop-spec/scripts/render.sh`](skills/devloop-spec/scripts/render.sh). +A good spec is short, concrete, and verifiable. It includes a repository-grounded implementation map, invariants, credible failure modes, and a proof map linking every obligation to test evidence. Start from [`skills/devloop-spec/references/spec-template.md`](skills/devloop-spec/references/spec-template.md). The bundled `devloop-spec` skill can also render a sibling HTML companion with [`skills/devloop-spec/scripts/render.sh`](skills/devloop-spec/scripts/render.sh). Strict mode is on by default: specs need `## Acceptance criteria`, and reviews must pass both the spec gate and engineering quality gate. diff --git a/devloop b/devloop index e4ff81c..583c66c 100755 --- a/devloop +++ b/devloop @@ -2881,22 +2881,24 @@ run_devloop() { local spec_text spec_text="$(cat "$spec")" - local criteria_file + local criteria_file obligations_file criteria_file="$(mktemp "${TMPDIR:-/tmp}/devloop-criteria.XXXXXX")" parse_criteria "$spec" > "$criteria_file" + obligations_file="$(mktemp "${TMPDIR:-/tmp}/devloop-obligations.XXXXXX")" + spec_obligations "$spec" > "$obligations_file" local criteria_count criteria_count="$(line_count "$criteria_file")" if ! lint_spec_file "$spec" "$spec_text" "$criteria_count" "$strict"; then STATUS="spec-error" printf '%s\n' "$SPEC_LINT_ERROR" >&2 - rm -f "$criteria_file" + rm -f "$criteria_file" "$obligations_file" return 2 fi event_gate "acceptance criteria" "$criteria_count" "$criteria_count found" SOURCE_REPO="$(git -C "$PWD" rev-parse --show-toplevel 2>/dev/null)" || { printf '%s\n' "not inside a git repository" >&2 - rm -f "$criteria_file" + rm -f "$criteria_file" "$obligations_file" return 2 } local source_branch @@ -2911,7 +2913,7 @@ run_devloop() { STATUS="preflight-error" event_done "preflight" false "$PREFLIGHT_ERROR" printf 'preflight failed: %s\n' "$PREFLIGHT_ERROR" >&2 - rm -f "$criteria_file" + rm -f "$criteria_file" "$obligations_file" return 2 fi @@ -2925,7 +2927,7 @@ run_devloop() { else printf 'naming failed: %s\n' "$naming_error" >&2 fi - rm -f "$criteria_file" + rm -f "$criteria_file" "$obligations_file" return 2 fi local slug="$WORK_SLUG" @@ -2935,7 +2937,7 @@ run_devloop() { if [ "$use_worktree" = true ]; then event_step "worktree" "creating worktree" repo="$(create_worktree "$SOURCE_REPO" "$WORK_TYPE" "$WORK_BREAKING" "$WORK_SLUG")" || { - rm -f "$criteria_file" + rm -f "$criteria_file" "$obligations_file" return 2 } event_done "worktree" true "$repo" @@ -3014,7 +3016,7 @@ run_devloop() { local coder_log=".devloop/logs/$slug-r$pass-coder.log" local coder_id="coder-$pass" event_step "$coder_id" "pass $pass/$max: $(agent_label "$coder") implementing" - if run_agent "$coder" "$repo" "$repo/$coder_session" "$repo/$coder_log" "$(coder_prompt "$run_spec" "$TRACK" "$pass" "$strict" ".devloop/reviews/$slug-r$((pass - 1)).md" "$criteria_file" "$pr_review_context")" "$coder_id"; then + if run_agent "$coder" "$repo" "$repo/$coder_session" "$repo/$coder_log" "$(coder_prompt "$run_spec" "$TRACK" "$pass" "$strict" ".devloop/reviews/$slug-r$((pass - 1)).md" "$obligations_file" "$pr_review_context")" "$coder_id"; then event_done "$coder_id" true "done" else if [ "$RUN_TIMED_OUT" = true ]; then STATUS="timeout"; else STATUS="coder-error"; fi @@ -3087,7 +3089,7 @@ run_devloop() { local reviewer_log=".devloop/logs/$slug-r$pass-reviewer.log" local reviewer_id="reviewer-$pass" event_step "$reviewer_id" "pass $pass/$max: $(agent_label "$reviewer") reviewing" - if run_agent "$reviewer" "$repo" "$repo/$reviewer_session" "$repo/$reviewer_log" "$(review_prompt "$coder" "$run_spec" "$TRACK" "$base" "$pass" "$review" "$slug" "$max" "$criteria_file" "$strict")" "$reviewer_id"; then + if run_agent "$reviewer" "$repo" "$repo/$reviewer_session" "$repo/$reviewer_log" "$(review_prompt "$coder" "$run_spec" "$TRACK" "$base" "$pass" "$review" "$slug" "$max" "$obligations_file" "$strict")" "$reviewer_id"; then event_done "$reviewer_id" true "done" else if [ "$RUN_TIMED_OUT" = true ]; then STATUS="timeout"; else STATUS="reviewer-error"; fi @@ -3116,7 +3118,7 @@ run_devloop() { verdict="$(parse_verdict "$repo/$review")" if [ "$verdict" = "ACCEPT" ]; then event_gate "pass $pass review verdict" 1 "$verdict" - if [ "$strict" = true ] && ! has_passing_matrix "$repo/$review" "$criteria_count"; then + if [ "$strict" = true ] && ! has_passing_matrix "$repo/$review" "$obligations_file"; then STATUS="unclear" elif [ "$strict" = true ] && ! has_passing_quality_matrix "$repo/$review"; then STATUS="unclear" @@ -3161,7 +3163,7 @@ run_devloop() { fi print_result - rm -f "$criteria_file" "$initial_dirty" + rm -f "$criteria_file" "$obligations_file" "$initial_dirty" return 0 } @@ -3237,7 +3239,7 @@ lint_spec_file() { local spec_text="$2" local criteria_count="$3" local strict="$4" - local title type + local title type duplicate_obligation SPEC_LINT_ERROR="" title="$(printf '%s\n' "$spec_text" | sed -nE 's/^#[[:space:]]+(.+)/\1/p' | head -n 1)" if [ -z "$(clean_report_text "$title")" ]; then @@ -3254,6 +3256,11 @@ lint_spec_file() { ;; esac fi + duplicate_obligation="$(duplicate_spec_obligation_identifier "$spec")" + if [ -n "$duplicate_obligation" ]; then + SPEC_LINT_ERROR="duplicate spec obligation identifier: $duplicate_obligation" + return 1 + fi if [ "$strict" = true ] && [ "$criteria_count" -eq 0 ]; then SPEC_LINT_ERROR="strict mode requires ## Acceptance criteria" return 1 @@ -3495,6 +3502,52 @@ criteria_block() { if [ "$count" -eq 0 ]; then printf '%s\n' "No parsed acceptance criteria."; fi } +parse_labeled_obligations() { + local file="$1" + awk ' + BEGIN { inside = 0 } + /^[[:space:]]*###[[:space:]]+Failure and edge cases[[:space:]]*$/ { inside = 1; next } + /^[[:space:]]*##[[:space:]]+Invariants[[:space:]]*$/ { inside = 1; next } + /^[[:space:]]*#+[[:space:]]+/ { inside = 0; next } + inside { + line = $0 + gsub(/^[[:space:]]+|[[:space:]]+$/, "", line) + sub(/^[-*][[:space:]]+/, "", line) + gsub(/\*\*/, "", line) + if (line ~ /^F[0-9]+[[:space:]]*(\(|:)/ || line ~ /^I[0-9]+[[:space:]]*:/) { + match(line, /^(F|I)[0-9]+/) + identifier = substr(line, RSTART, RLENGTH) + detail = substr(line, RLENGTH + 1) + sub(/^[[:space:]]*:[[:space:]]*/, "", detail) + sub(/^[[:space:]]+/, "", detail) + printf "%s: %s\n", identifier, detail + } + } + ' "$file" +} + +spec_obligations() { + local spec="$1" + local count=0 + local criterion + while IFS= read -r criterion; do + count=$((count + 1)) + printf 'AC%s: %s\n' "$count" "$criterion" + done < <(parse_criteria "$spec") + parse_labeled_obligations "$spec" +} + +duplicate_spec_obligation_identifier() { + local spec="$1" + spec_obligations "$spec" | awk -F ':' ' + { + identifier = $1 + if (duplicate == "" && seen[identifier]++) duplicate = identifier + } + END { if (duplicate != "") print duplicate } + ' +} + base_branch() { local repo="$1" local out @@ -4125,10 +4178,10 @@ coder_prompt() { local pass="$3" local strict="$4" local previous="$5" - local criteria_file="$6" + local obligations_file="$6" local pr_review_context="${7:-}" - local criteria - criteria="$(criteria_block "$criteria_file")" + local obligations + obligations="$(cat "$obligations_file")" local strict_block="" if [ "$strict" = true ]; then strict_block=" @@ -4146,11 +4199,11 @@ You are implementing against an approved spec. Spec: $spec Track: $track Pass: $pass -Acceptance criteria: -$criteria$strict_block +Spec obligations (acceptance criteria, invariants, and failure modes): +$obligations$strict_block Tasks: 1. Read the spec. -2. Implement the smallest working change satisfying the acceptance criteria. +2. Implement the smallest working change satisfying every spec obligation. 3. Append "## Pass $pass - implement" to $track with changed files, design tradeoffs, verification, and residual risk. Constraints: @@ -4173,13 +4226,14 @@ Spec: $spec Track: $track $review_context Pass: $pass -Acceptance criteria: -$criteria$strict_block +Spec obligations (acceptance criteria, invariants, and failure modes): +$obligations$strict_block Tasks: 1. Read the review context. 2. Fix each finding or explain why it is wrong in the track. -3. Re-run relevant tests. -4. Append "## Pass $pass - fix" to $track with per-finding outcomes. +3. Confirm every spec obligation remains satisfied. +4. Re-run relevant tests. +5. Append "## Pass $pass - fix" to $track with per-finding outcomes. EOF fi } @@ -4193,10 +4247,10 @@ review_prompt() { local output="$6" local slug="$7" local max="$8" - local criteria_file="$9" + local obligations_file="$9" local strict="${10}" - local criteria priors review_skill strict_rule - criteria="$(criteria_block "$criteria_file")" + local obligations priors review_skill strict_rule + obligations="$(cat "$obligations_file")" priors="$(list_reviews "$slug" "$pass" "$max")" review_skill="$ROOT_DIR/skills/devloop-review/SKILL.md" strict_rule="" @@ -4213,8 +4267,8 @@ Base: $base Pass: $pass Prior reviews: $priors -Acceptance criteria: -$criteria +Spec obligations (acceptance criteria, invariants, and failure modes): +$obligations Output path: $output Skill: use the installed devloop-review skill. Bundled skill path, for fallback only: $review_skill @@ -4231,9 +4285,11 @@ Verdict: ## Acceptance matrix -| Criterion | Status | Implementation evidence | Test evidence | +| Obligation | Status | Implementation evidence | Test evidence | | --- | --- | --- | --- | | AC1 | | | | +| F1 | | | | +| I1 | | | | ## Engineering quality matrix @@ -4271,10 +4327,11 @@ Verdict: Rules: - The verdict line must appear verbatim. -- ACCEPT requires every acceptance criterion PASS with concrete implementation evidence and concrete test evidence.$strict_rule +- Include exactly one row for every listed spec obligation. Omit sample identifiers that are not present in the spec. +- ACCEPT requires every acceptance criterion, invariant, and failure mode PASS with concrete implementation evidence and concrete test evidence.$strict_rule - ACCEPT requires a complete Engineering quality matrix. Reject if any engineering quality row is FAIL. - Flag a silent decision when the diff makes a tradeoff, default choice, migration choice, or risk acceptance that is not recorded in the spec or track. -- Flag scope drift when the diff changes behavior, public API, dependencies, files, or architecture outside the acceptance criteria, or includes a broad refactor not needed for the spec. +- Flag scope drift when the diff changes behavior, public API, dependencies, files, or architecture outside the spec's declared scope or is not needed to satisfy any listed obligation, or includes a broad refactor not needed for the spec. - Flag a missing test when behavior changed without targeted test evidence, even if the full suite passed. - Use UNCLEAR only when spec ambiguity prevents a defensible ACCEPT or REJECT, and put the exact question in Notes. - For ACCEPT: Findings and Fix instructions bodies are "None". @@ -4411,15 +4468,21 @@ parse_verdict() { has_passing_matrix() { local file="$1" - local count="$2" - local i=1 + local obligations_file="$2" + local obligation identifier found + found=false grep -Eq '^##[[:space:]]+Acceptance matrix[[:space:]]*$' "$file" || return 1 - while [ "$i" -le "$count" ]; do - if ! grep -Eiq "^\|[[:space:]]*AC${i}[[:space:]]*\|[[:space:]]*PASS[[:space:]]*\|" "$file"; then + while IFS= read -r obligation; do + identifier="${obligation%%:*}" + if ! printf '%s\n' "$identifier" | grep -Eq '^(AC|F|I)[0-9]+$'; then return 1 fi - i=$((i + 1)) - done + found=true + if ! grep -Eiq "^\|[[:space:]]*${identifier}[[:space:]]*\|[[:space:]]*PASS[[:space:]]*\|" "$file"; then + return 1 + fi + done < "$obligations_file" + [ "$found" = true ] } has_passing_quality_matrix() { diff --git a/scripts/devloop_test.sh b/scripts/devloop_test.sh index 3bfec90..e7e0742 100755 --- a/scripts/devloop_test.sh +++ b/scripts/devloop_test.sh @@ -118,6 +118,7 @@ ok "spec skill path" spec_skill_text="$(cat "$REPO_ROOT/skills/devloop-spec/SKILL.md")" spec_template_text="$(cat "$REPO_ROOT/skills/devloop-spec/references/spec-template.md")" +review_skill_text="$(cat "$REPO_ROOT/skills/devloop-review/SKILL.md")" contains "$spec_skill_text" "Mermaid" "spec skill Mermaid guidance" contains "$spec_skill_text" "scripts/render.sh" "spec skill renderer" contains "$spec_skill_text" "HTML Companion" "spec skill renderer" @@ -127,8 +128,30 @@ contains "$spec_skill_text" "Do not convert a conversation, artifact bundle, or contains "$spec_skill_text" "If interactive and gaps remain, do not draft yet" "spec skill gap interview" not_contains "$spec_skill_text" "offer to interview for only those gaps after producing the first draft" "spec skill gap interview" contains "$spec_skill_text" "/.devloop/specs/YYYY-MM-DD-.md" "spec skill repo output" +contains "$spec_skill_text" "Discover repository facts before user questions." "spec skill discovery discipline" +contains "$spec_skill_text" "Facts are discovered from evidence; decisions are asked of the user." "spec skill discovery discipline" +contains "$spec_skill_text" "Build a change map" "spec skill defect model" +contains "$spec_skill_text" "State the invariants" "spec skill defect model" +contains "$spec_skill_text" "Enumerate relevant failure modes at every changed boundary and state transition." "spec skill defect model" +contains "$spec_skill_text" "Map every acceptance criterion, invariant, and failure mode to specific test or observable evidence." "spec skill verification matrix" +contains "$spec_skill_text" "Name the highest-risk review focus areas" "spec skill review focus" +contains "$spec_skill_text" "Record every behavior-affecting default, decision, and assumption" "spec skill decision record" contains "$spec_template_text" '```mermaid' "spec template Mermaid fence" contains "$spec_template_text" "flowchart LR" "spec template Mermaid syntax" +contains "$spec_template_text" "## Implementation map" "spec template implementation map" +contains "$spec_template_text" "## Invariants" "spec template invariants" +contains "$spec_template_text" "### Failure and edge cases" "spec template failure modes" +contains "$spec_template_text" "### Proof obligations" "spec template verification map" +contains "$spec_template_text" "## Review focus" "spec template review focus" +not_contains "$spec_skill_text" "1. **AC1:**" "spec skill positional acceptance numbering" +not_contains "$spec_template_text" "1. **AC1:**" "spec template positional acceptance numbering" +not_contains "$spec_skill_text" "**F1" "spec skill plain failure labels" +not_contains "$spec_skill_text" "**I1" "spec skill plain invariant labels" +not_contains "$spec_template_text" "**F1" "spec template plain failure labels" +not_contains "$spec_template_text" "**I1" "spec template plain invariant labels" +contains "$review_skill_text" "Check every spec obligation independently" "review skill obligations" +contains "$review_skill_text" 'Every acceptance criterion, invariant, and failure mode is `PASS`.' "review skill obligations" +contains "$review_skill_text" "Do not flag work required solely by an invariant or failure mode as scope drift." "review skill scope drift" ok "spec Mermaid diagram guidance" contains "$(cat "$REPO_ROOT/README.md")" "\`devloop --create-pr \`" "README PR mode" @@ -249,6 +272,12 @@ not_contains "$(cat "$renderer_output")" "mermaid.esm.min.mjs" "spec renderer wi expected_renderer_output="$(cd -P "$(dirname "$regular_renderer_fixture")" >/dev/null 2>&1 && pwd)/$(basename "${regular_renderer_fixture%.md}.html")" equals "$renderer_output" "$expected_renderer_output" "spec renderer output path" +template_renderer_fixture="$work/spec-template.md" +cp "$REPO_ROOT/skills/devloop-spec/references/spec-template.md" "$template_renderer_fixture" +template_renderer_output="$("$renderer_script" "$template_renderer_fixture")" +not_contains "$(cat "$template_renderer_output")" "**F1" "spec template rendered failure labels" +not_contains "$(cat "$template_renderer_output")" "**I1" "spec template rendered invariant labels" + mermaid_renderer_fixture="$work/spec-render-mermaid-fixture.md" cat > "$mermaid_renderer_fixture" <<'SPEC' --- @@ -428,6 +457,13 @@ criteria_file="$work/criteria.md" cat > "$criteria_file" <<'MARKDOWN' # Spec +## Behavior +### Failure and edge cases +- F1 (Empty input): The command reports the error without changing state. + +## Invariants +- I1: State changes only after validation succeeds. + ## Acceptance criteria 1. First thing - Second thing @@ -436,6 +472,46 @@ cat > "$criteria_file" <<'MARKDOWN' Ignore me MARKDOWN equals "$(parse_criteria "$criteria_file")" $'First thing\nSecond thing' "parse_criteria" +equals "$(spec_obligations "$criteria_file")" $'AC1: First thing\nAC2: Second thing\nF1: (Empty input): The command reports the error without changing state.\nI1: State changes only after validation succeeds.' "spec_obligations" +obligations_file="$work/obligations.md" +spec_obligations "$criteria_file" > "$obligations_file" + +duplicate_failure_spec="$work/duplicate-failure.md" +cat > "$duplicate_failure_spec" <<'MARKDOWN' +# Duplicate Failure + +## Behavior +### Failure and edge cases +- F1 (Empty input): Reject the input. +- F1 (Invalid input): Reject the input without changing state. + +## Acceptance criteria +1. The command validates input. +MARKDOWN +if lint_spec_file "$duplicate_failure_spec" "$(cat "$duplicate_failure_spec")" 1 true; then + fail "lint_spec_file accepted duplicate failure identifier" +fi +equals "$SPEC_LINT_ERROR" "duplicate spec obligation identifier: F1" "duplicate failure identifier" + +duplicate_invariant_spec="$work/duplicate-invariant.md" +cat > "$duplicate_invariant_spec" <<'MARKDOWN' +# Duplicate Invariant + +## Invariants +- I1: State changes only after validation. +- I1: Failed writes leave existing state intact. + +## Acceptance criteria +1. The command updates valid state. +MARKDOWN +if lint_spec_file "$duplicate_invariant_spec" "$(cat "$duplicate_invariant_spec")" 1 true; then + fail "lint_spec_file accepted duplicate invariant identifier" +fi +equals "$SPEC_LINT_ERROR" "duplicate spec obligation identifier: I1" "duplicate invariant identifier" + +template_criteria_file="$work/spec-template-criteria.md" +parse_criteria "$REPO_ROOT/skills/devloop-spec/references/spec-template.md" > "$template_criteria_file" +equals "$(criteria_block "$template_criteria_file")" $'AC1: \nAC2: ' "spec template criteria numbering" review_file="$work/review.md" cat > "$review_file" <<'MARKDOWN' @@ -449,6 +525,8 @@ Verdict: ACCEPT | --- | --- | --- | --- | | AC1 | PASS | code path | test | | AC2 | PASS | behavior | test | +| F1 | PASS | guarded failure path | failure test | +| I1 | PASS | validation boundary | state test | ## Engineering quality matrix @@ -463,17 +541,33 @@ Verdict: ACCEPT | Operational safety | PASS | no partial update | MARKDOWN equals "$(parse_verdict "$review_file")" "ACCEPT" "parse_verdict" -has_passing_matrix "$review_file" 2 || fail "has_passing_matrix rejected passing matrix" +has_passing_matrix "$review_file" "$obligations_file" || fail "has_passing_matrix rejected passing matrix" has_passing_quality_matrix "$review_file" || fail "has_passing_quality_matrix rejected passing matrix" sed 's/| AC2 | PASS |/| AC2 | FAIL |/' "$review_file" > "$work/review-fail.md" -if has_passing_matrix "$work/review-fail.md" 2; then fail "has_passing_matrix accepted failing matrix"; fi +if has_passing_matrix "$work/review-fail.md" "$obligations_file"; then fail "has_passing_matrix accepted failing criterion"; fi +sed 's/| F1 | PASS |/| F1 | FAIL |/' "$review_file" > "$work/review-failure-fail.md" +if has_passing_matrix "$work/review-failure-fail.md" "$obligations_file"; then fail "has_passing_matrix accepted failing failure mode"; fi +sed 's/| I1 | PASS |/| I1 | FAIL |/' "$review_file" > "$work/review-invariant-fail.md" +if has_passing_matrix "$work/review-invariant-fail.md" "$obligations_file"; then fail "has_passing_matrix accepted failing invariant"; fi sed 's/| Maintainability | PASS |/| Maintainability | FAIL |/' "$review_file" > "$work/review-quality-fail.md" if has_passing_quality_matrix "$work/review-quality-fail.md"; then fail "has_passing_quality_matrix accepted failing matrix"; fi -review_prompt_text="$(review_prompt codex "$criteria_file" ".devloop/tracks/test.md" main 1 ".devloop/reviews/test-r1.md" test 5 "$criteria_file" true)" +coder_prompt_text="$(coder_prompt "$criteria_file" ".devloop/tracks/test.md" 1 true "" "$obligations_file")" +contains "$coder_prompt_text" "Spec obligations (acceptance criteria, invariants, and failure modes):" "coder prompt obligations" +contains "$coder_prompt_text" "F1: (Empty input): The command reports the error without changing state." "coder prompt failure mode" +contains "$coder_prompt_text" "I1: State changes only after validation succeeds." "coder prompt invariant" +contains "$coder_prompt_text" "satisfying every spec obligation" "coder prompt contract" + +review_prompt_text="$(review_prompt codex "$criteria_file" ".devloop/tracks/test.md" main 1 ".devloop/reviews/test-r1.md" test 5 "$obligations_file" true)" contains "$review_prompt_text" "Skill: use the installed devloop-review skill." "review prompt" contains "$review_prompt_text" "Bundled skill path, for fallback only: $REPO_ROOT/skills/devloop-review/SKILL.md" "review prompt" contains "$review_prompt_text" "Engineering quality matrix" "review prompt" +contains "$review_prompt_text" "Spec obligations (acceptance criteria, invariants, and failure modes):" "review prompt obligations" +contains "$review_prompt_text" "F1: (Empty input): The command reports the error without changing state." "review prompt failure mode" +contains "$review_prompt_text" "I1: State changes only after validation succeeds." "review prompt invariant" +contains "$review_prompt_text" "ACCEPT requires every acceptance criterion, invariant, and failure mode PASS" "review prompt contract" +contains "$review_prompt_text" "outside the spec's declared scope or is not needed to satisfy any listed obligation" "review prompt scope drift" +not_contains "$review_prompt_text" "outside the acceptance criteria" "review prompt scope drift" findings_a="$work/findings-a.md" findings_b="$work/findings-b.md" diff --git a/skills/devloop-review/SKILL.md b/skills/devloop-review/SKILL.md index 2adc24a..ff5c3aa 100644 --- a/skills/devloop-review/SKILL.md +++ b/skills/devloop-review/SKILL.md @@ -17,8 +17,8 @@ Review against two gates: 2. Engineering gate: the implementation is correct, maintainable, and fits the codebase without avoidable structural debt. -Do not approve merely because the acceptance criteria pass. A change can pass -the spec gate and still fail review because it makes the codebase worse. +Do not approve merely because the spec obligations pass. A change can pass the +spec gate and still fail review because it makes the codebase worse. ## Gather Context @@ -46,9 +46,10 @@ unrelated follow-up territory out of the devloop pass. ## Spec Gate -Check every acceptance criterion independently: +Check every spec obligation independently: -- Does the implementation satisfy this exact criterion? +- Does the implementation satisfy this exact acceptance criterion, invariant, + or failure mode? - Is there concrete code or behavior evidence? - Is there concrete test or command evidence? - Did the change drift beyond the spec? @@ -57,8 +58,12 @@ Check every acceptance criterion independently: - If this pass fixes a prior review finding, was verification rerun after the fix instead of reused from before the code changed? -Reject when a criterion fails, evidence is vague, scope drift is meaningful, or -missing tests leave changed behavior underverified. +Reject when an obligation fails, evidence is vague, scope drift is meaningful, +or missing tests leave changed behavior underverified. + +Do not flag work required solely by an invariant or failure mode as scope drift. +Scope drift is work outside the spec's declared scope or not needed to satisfy +any listed obligation. ## Engineering Gate @@ -101,7 +106,7 @@ Treat these as reject-level findings unless clearly justified: Use `ACCEPT` only when both gates pass: -- Every acceptance criterion is `PASS`. +- Every acceptance criterion, invariant, and failure mode is `PASS`. - Every engineering quality row is `PASS` or `N/A`. - Findings and fix instructions are `None`. - Any tradeoff, risk, or default choice is recorded in the track or is genuinely diff --git a/skills/devloop-spec/SKILL.md b/skills/devloop-spec/SKILL.md index b45e1e5..83ea3b2 100644 --- a/skills/devloop-spec/SKILL.md +++ b/skills/devloop-spec/SKILL.md @@ -38,6 +38,30 @@ Do not convert a conversation, artifact bundle, or notes directly into a spec ju When interactive, ask one question at a time until the gate passes. When the environment cannot ask questions, write a marked draft only if the caller explicitly needs best-effort non-interactive output; otherwise stop with the questions that must be answered. +## Planning Discipline + +During spec discovery and interview, do not implement the requested product or code change. + +- Discovery before interview: Discover repository facts before user questions. Inspect relevant commands, paths, symbols, ownership boundaries, tests, and local conventions whenever a repository is available. +- Facts vs decisions: Facts are discovered from evidence; decisions are asked of the user. Do not ask the user to restate facts that are cheap to verify. +- User decisions: Ask one user decision at a time and include your recommended answer with the concrete tradeoff. +- Dependency order: Walk parent decisions before downstream details. +- Settled context: If supplied context is already settled, synthesize it directly without re-interviewing. +- Follow-up threshold: Only ask a follow-up when the answer could change scope, behavior, acceptance criteria, failure handling, test seams, or the implementation slice. + +## Defect Model + +Treat the spec as the shared defect-prevention contract for the implementer and reviewer. Before drafting: + +1. Trace the affected behavior from its input or trigger through state changes and external interactions to the observable result. +2. Build a change map naming the repository paths, symbols, ownership boundaries, and existing contracts expected to change or be reused. Separate verified repository facts from proposed design. +3. State the invariants that must remain true across success, failure, retry, and interruption. +4. Enumerate relevant failure modes at every changed boundary and state transition. Check invalid, empty, and boundary inputs; partial failure and interruption; dependency errors and timeouts; retries, duplicates, and idempotency; ordering and concurrency; permissions, security, and privacy; migrations and compatibility; cleanup, rollback, and recovery. Include only credible risks for this change. +5. Identify the highest stable existing test seam. Prefer one seam when practical; if none covers the change, name the smallest appropriate new seam. +6. Map every acceptance criterion, invariant, and failure mode to specific test or observable evidence. Group identifiers when one test proves the same behavior, but leave no obligation without evidence or an explicit reason that verification is manual or unavailable. +7. Name the highest-risk review focus areas and the adjacent ownership boundary the reviewer should inspect for sibling instances of the same bug class. +8. Record every behavior-affecting default, decision, and assumption in Notes. Do not leave the implementer or reviewer to infer a silent choice. + ## Source Resolution Resolve the source material before drafting: @@ -115,30 +139,44 @@ flowchart LR - In: - Out: +## Implementation map +1. `` / ``: + ## Behavior ### Happy path 1. 2. -### Edge cases -- : +### Failure and edge cases +- F1 (): + +## Invariants +- I1: ## Acceptance criteria 1. ## Test plan +### Proof obligations +- AC1, I1: +- F1: + +### Commands - Red: - Green: - Full: - Coverage: <100% coverage command, or why unavailable> +## Review focus +- + ## Constraints - Must: - Avoid: - Existing convention: ## Notes - + ```` - `created` is today's date. @@ -147,8 +185,13 @@ flowchart LR - The subtitle is a plain one-sentence line directly under the H1. - The Mermaid schematic is optional but preferred when it clarifies architecture, data flow, ownership, or before/after behavior. Omit it if it would be decorative or speculative. - In Mermaid flowchart node labels, quote labels containing `|`. Use `Node["A | B"]`, not `Node[A | B]`. -- Under `## Behavior`, use `### Happy path` and `### Edge cases` H3 headings, not plain `Happy path:` labels. -- Acceptance criteria must be singular, verifiable, and observable. +- The implementation map names expected ownership boundaries and existing contracts, not incidental line-by-line edits. Every entry must be grounded in repository evidence or clearly labeled as proposed design. +- Under `## Behavior`, use `### Happy path` and `### Failure and edge cases` H3 headings, not plain labels. +- Number failure modes with plain labels such as `F1` and invariants with plain labels such as `I1`. Acceptance criteria are numbered positionally by their list order; refer to them as `AC1`, `AC2`, and so on in the proof map, but do not write an `AC` label inside criterion text. +- Acceptance criteria must be singular, verifiable, and observable. Do not restate implementation steps as acceptance criteria. +- The test-plan proof map must cover every `AC`, `I`, and `F` identifier exactly once or explain why its evidence is manual or unavailable. Combine identifiers when one proof covers them without merely restating the requirements. +- Keep Review focus to the one to three highest-risk boundaries or bug classes. It guides adversarial review but does not limit the reviewer's engineering-quality pass. +- Record material defaults, decisions, assumptions, dependencies, and risks in Notes. Omit empty categories. - Include concrete paths, commands, APIs, and behaviors when the context provides them. ## Gaps diff --git a/skills/devloop-spec/references/spec-template.md b/skills/devloop-spec/references/spec-template.md index 136da50..1fb3228 100644 --- a/skills/devloop-spec/references/spec-template.md +++ b/skills/devloop-spec/references/spec-template.md @@ -24,29 +24,44 @@ flowchart LR - In: - Out: +## Implementation map +1. `` / ``: + ## Behavior ### Happy path 1. 2. -### Edge cases -- : -- : +### Failure and edge cases +- F1 (): +- F2 (): + +## Invariants +- I1: +- I2: ## Acceptance criteria 1. 2. ## Test plan +### Proof obligations +- AC1, I1, F1: +- AC2, I2, F2: + +### Commands - Red: - Green: - Full: - Coverage: <100% coverage command, or why unavailable> +## Review focus +- + ## Constraints - Must: - Avoid: - Existing convention: ## Notes - + From 64f01898deb5009df4379530e430705324d47a5e Mon Sep 17 00:00:00 2001 From: satyaborg Date: Tue, 14 Jul 2026 16:34:19 +1000 Subject: [PATCH 2/2] fix: escape review skill assertion --- scripts/devloop_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/devloop_test.sh b/scripts/devloop_test.sh index e7e0742..982c0ee 100755 --- a/scripts/devloop_test.sh +++ b/scripts/devloop_test.sh @@ -150,7 +150,7 @@ not_contains "$spec_skill_text" "**I1" "spec skill plain invariant labels" not_contains "$spec_template_text" "**F1" "spec template plain failure labels" not_contains "$spec_template_text" "**I1" "spec template plain invariant labels" contains "$review_skill_text" "Check every spec obligation independently" "review skill obligations" -contains "$review_skill_text" 'Every acceptance criterion, invariant, and failure mode is `PASS`.' "review skill obligations" +contains "$review_skill_text" "Every acceptance criterion, invariant, and failure mode is \`PASS\`." "review skill obligations" contains "$review_skill_text" "Do not flag work required solely by an invariant or failure mode as scope drift." "review skill scope drift" ok "spec Mermaid diagram guidance"