From fda1fced3de12d28ea89c3aae89e00cebe799cae Mon Sep 17 00:00:00 2001 From: Mike Angstadt Date: Tue, 11 Aug 2026 23:25:59 -0500 Subject: [PATCH] fix(code): close three gaps in detect_spurious_complete ISS-5963 Layer 1. c3a4305 inverted the early return so a --prd run that claims COMPLETE with no plan.json is flagged PLAN_MISSING_AT_COMPLETION. Three verified gaps remained. - A zero-byte plan.json was still waved through: [[ -f ]] passes on it, jq yields nothing, and the pendingTasks checks then read 0 pending and call the run clean -- so the 0-byte artifact that is the incident's own evidence reported success. classify_plan_artifact now treats absent, zero-byte, and unparseable-JSON alike: no plan was produced. - An absent workspace failed CLOSED. "The artifact was not produced" and "the workspace is gone" are different facts; live-exit and boot-recovery reclaim the workdir right after finalization, so adjudicating a run whose directory has been deleted would flip a genuine success to FAILED with no repair path. Missing workdir now fails open. - The predicate was the --prd proxy, so REQUEST_CHANGES -- whose result bundle also requires plan.json -- was invisible, and EXECUTE was excluded only by the accident of not carrying a PRD. run_owes_plan_json now reads the command: PLAN and REQUEST_CHANGES owe a plan, EXECUTE and the rest are excluded by name, and an unknown command falls back to the --prd proxy so version skew never crashes or blocks in either direction. REQUEST_CHANGES is enforced only on the not-produced-at-all axis. The harness seeds plan.json before an amend, so presence proves nothing there; detecting "the amend produced nothing" needs a pre-run baseline the detector is not given, and that AC is left open rather than guessed. Testing: bash plugins/code/scripts/tests/test_spurious_complete.sh -- 19 pass, 0 fail. Each gap proven counterfactually by reverting its production line: gap 1 leaves 4 red (zero-byte, malformed, whitespace-only, seeded zero-byte all return {}), gap 2 leaves 1 red (deleted workdir reported PLAN_MISSING_AT_COMPLETION), gap 3 leaves 4 red (REQUEST_CHANGES invisible, EXECUTE falsely flagged). uv run pytest plugins/ green; bash -n clean. Risks: Low, and one-sided by design -- every new flag is scoped to a command whose declared bundle requires plan.json, and the two new not-flagged paths (absent workspace, EXECUTE by name) only remove false failures. The pre-existing shellcheck SC1073 in run-loop.sh is unchanged and unrelated. --- plugins/code/.claude-plugin/plugin.json | 2 +- plugins/code/scripts/run-loop.sh | 131 ++++++++++++++++-- .../scripts/tests/test_spurious_complete.sh | 90 ++++++++++++ 3 files changed, 212 insertions(+), 11 deletions(-) diff --git a/plugins/code/.claude-plugin/plugin.json b/plugins/code/.claude-plugin/plugin.json index 2e4c6cc..444d259 100644 --- a/plugins/code/.claude-plugin/plugin.json +++ b/plugins/code/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code", "description": "Code and planning framework plugin", - "version": "1.14.8", + "version": "1.14.9", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code/scripts/run-loop.sh b/plugins/code/scripts/run-loop.sh index 5b92ccb..08213e6 100755 --- a/plugins/code/scripts/run-loop.sh +++ b/plugins/code/scripts/run-loop.sh @@ -132,6 +132,78 @@ fail_loop_user_visible() { exit 1 } +# Loop commands whose result bundle declares plan.json REQUIRED. Mirrors the +# ResultBundle manifest that the cloud side validates against (PLAN and +# REQUEST_CHANGES both require plan.json). +# +# EXECUTE is deliberately absent, and that is the load-bearing part: its +# required artifact is execution-result.json, which is only written after a +# successful commit AND push, so a legitimate no-changes EXECUTE run ends +# without it. A blanket "required artifact missing => spurious" rule would fail +# every one of those runs. Listing the non-plan commands by name below keeps +# EXECUTE excluded on purpose rather than by the accident of it not carrying a +# --prd. +LOOP_COMMANDS_OWING_PLAN="PLAN REQUEST_CHANGES" +LOOP_COMMANDS_NOT_OWING_PLAN="EXECUTE CHAT EXPLORE REQUEST_PRD_CHANGES DECOMPOSE EVALUATE_PRD GENERATE_PRD EVALUATE_PLAN EVALUATE_CODE EVALUATE_FEATURE BOOTSTRAP MANUAL" + +# Normalize a command label to the canonical LoopCommand spelling. +# The desktop sets CLOSEDLOOP_COMMAND to the wire spelling (PLAN, EXECUTE, +# REQUEST_CHANGES); a local CLI run resolves it from --prompt instead, whose +# values are the prompt file names (plan-prompt, execute-prompt). Both map to +# the same command, so normalize rather than special-case the caller. +normalize_loop_command() { + local raw="${1:-}" + raw="${raw%-prompt}" + raw="${raw%_prompt}" + printf '%s' "$raw" | tr '[:lower:]-' '[:upper:]_' +} + +# Does this run owe a plan.json? Echoes nothing; returns 0 (owes) or 1 (does not). +# Args: $1 = command label (raw), $2 = prd_file +# +# Version skew, both directions: an unrecognized command -- an older desktop +# sending nothing, or a newer one sending a command this script has never heard +# of -- is not an error and never blocks. It falls back to the --prd proxy, +# which is exactly the behaviour before commands were modelled at all. +run_owes_plan_json() { + local command + command=$(normalize_loop_command "${1:-}") + local prd_file="${2:-}" + + case " $LOOP_COMMANDS_OWING_PLAN " in + *" $command "*) return 0 ;; + esac + case " $LOOP_COMMANDS_NOT_OWING_PLAN " in + *" $command "*) return 1 ;; + esac + + [[ -n "$prd_file" ]] +} + +# Classify the plan artifact at $1 as: missing | empty | unparseable | present. +# +# "Exists" is not "was produced". The incident evidence is literally a 0-byte +# plan.json: [[ -f ]] passes on it, jq yields nothing, and every pendingTasks +# check below reads 0 pending tasks and calls the run clean. A file that is +# absent, zero-byte, or not parseable JSON carries the same fact -- no plan was +# written -- and all three must be treated as unproduced. +classify_plan_artifact() { + local plan_file="$1" + if [[ ! -f "$plan_file" ]]; then + printf 'missing' + return + fi + if [[ ! -s "$plan_file" ]]; then + printf 'empty' + return + fi + if ! jq -e . "$plan_file" >/dev/null 2>&1; then + printf 'unparseable' + return + fi + printf 'present' +} + # Detect a spurious COMPLETE: the orchestrator's Phase 7 contract forbids # emitting COMPLETE when plan.json has pending tasks, but # it sometimes violates that contract -- typically when tasks are blocked by @@ -143,6 +215,10 @@ fail_loop_user_visible() { # {"subcode": "...", "message": "..."} when a violation is detected # {} otherwise # +# Args: $1 = workdir +# $2 = prd file (defaults to $PRD_FILE; tests pass it explicitly) +# $3 = command (defaults to $CLOSEDLOOP_COMMAND; ditto) +# # Caller is responsible for telemetry, cleanup, and invoking # fail_loop_user_visible. detect_spurious_complete() { @@ -152,9 +228,23 @@ detect_spurious_complete() { # draft a plan from a PRD, which is what makes a missing plan.json a broken # promise rather than a run that never owed one. local prd_file="${2-${PRD_FILE:-}}" + # Defaults to the global the desktop exports; passed explicitly by tests. + local command="${3-${CLOSEDLOOP_COMMAND:-}}" local plan_file="$workdir/plan.json" local state_file="$workdir/state.json" + # Fail OPEN when the workspace itself is gone. "The artifact was not produced" + # and "the workspace no longer exists" are different facts, and only the first + # one is evidence of a spurious completion. Live-exit and boot-recovery paths + # delete the temp workdir right after finalization, so adjudicating a run + # whose directory has already been reclaimed would flip a genuine success into + # a failure -- a permanent divergence that no re-run can repair. A missing + # workdir means "cannot judge", so judge nothing. + if [[ ! -d "$workdir" ]]; then + echo '{}' + return + fi + # Skip the check when the orchestrator emitted COMPLETE as part of an # AWAITING_USER_SEQUENCE hard stop (e.g., the Phase 1.1 plan review # checkpoint). In those cases pending tasks and open questions are @@ -170,12 +260,15 @@ detect_spurious_complete() { fi fi - if [[ ! -f "$plan_file" ]]; then - # A --prd run exists to produce plan.json. Claiming COMPLETE without one is - # the strongest spurious-completion signal there is, and this branch used to - # wave it through: the checks below only validate pendingTasks INSIDE an - # existing plan, so "no plan at all" -- the case that actually happens -- - # was the one case nothing could catch. + local plan_state + plan_state=$(classify_plan_artifact "$plan_file") + + if [[ "$plan_state" != "present" ]]; then + # A PLAN or REQUEST_CHANGES run exists to produce plan.json. Claiming + # COMPLETE without one is the strongest spurious-completion signal there is, + # and this branch used to wave it through: the checks below only validate + # pendingTasks INSIDE an existing plan, so "no plan at all" -- the case that + # actually happens -- was the one case nothing could catch. # # Observed: the orchestrator launched plan-draft-writer in the BACKGROUND, # said "Plan-draft-writer is running in the background. Waiting for @@ -185,12 +278,30 @@ detect_spurious_complete() { # nothing was examined"), and the run exited 0 having produced nothing. The # user got an implementation-plan artifact that looked done and was empty. # - # Scoped to runs that were asked for a plan: a run with no PRD never owed - # one, and is left alone. - if [[ -n "$prd_file" ]]; then + # Scoped per-command: a run that never owed a plan is left alone. + # + # REQUEST_CHANGES is included, with a known limit: the harness seeds + # plan.json before an amend run, so PRESENCE PROVES NOTHING there. This + # branch still catches "no plan at all" (including a seeded file truncated + # to zero bytes), but it cannot see "the amend ran and produced nothing" -- + # that needs a pre-run baseline the detector is not given. Do not read a + # clean result on a REQUEST_CHANGES run as proof the amend did work. + if run_owes_plan_json "$command" "$prd_file"; then + local plan_detail + case "$plan_state" in + empty) + plan_detail="plan.json is zero bytes -- the file exists but no plan was ever written into it" + ;; + unparseable) + plan_detail="plan.json is not parseable JSON -- the file exists but no usable plan was written into it" + ;; + *) + plan_detail="no plan.json was ever written" + ;; + esac jq -n -c \ --arg subcode "PLAN_MISSING_AT_COMPLETION" \ - --arg message "Loop emitted COMPLETE but no plan.json was ever written. The planning phase did not finish -- a background plan-draft-writer that is still running when the completion promise fires is abandoned. Inspect state.json and the loop output, then re-run /code:code to continue." \ + --arg message "Loop emitted COMPLETE but $plan_detail. The planning phase did not finish -- a background plan-draft-writer that is still running when the completion promise fires is abandoned. Inspect state.json and the loop output, then re-run /code:code to continue." \ '{subcode:$subcode,message:$message}' return fi diff --git a/plugins/code/scripts/tests/test_spurious_complete.sh b/plugins/code/scripts/tests/test_spurious_complete.sh index f1ea7f9..0e9e1e3 100755 --- a/plugins/code/scripts/tests/test_spurious_complete.sh +++ b/plugins/code/scripts/tests/test_spurious_complete.sh @@ -56,6 +56,11 @@ assert_not_spurious() { fi } +# The two-argument call sites below default the command to $CLOSEDLOOP_COMMAND, +# so an ambient value -- this suite may well be run from inside a live loop -- +# would silently decide the per-command branch for them. Clear it first. +unset CLOSEDLOOP_COMMAND PRD_FILE || true + # shellcheck source=/dev/null source "$RUN_LOOP" @@ -106,6 +111,91 @@ assert_subcode "pending tasks blocked by open questions still flagged" \ "PENDING_TASKS_BLOCKED_BY_QUESTIONS" rm -rf "$WORKDIR" +# --- A file that EXISTS but holds no plan is still an unproduced plan -------- +# The incident evidence is literally a 0-byte plan.json. [[ -f ]] passes on it, +# jq yields nothing, and the pendingTasks checks then read 0 pending tasks and +# call the run clean -- so the exact condition this guard exists to catch was +# reported as success. +WORKDIR=$(mktemp -d) +: > "$WORKDIR/plan.json" +assert_subcode "a zero-byte plan.json is treated as missing" \ + "$(detect_spurious_complete "$WORKDIR" "/tmp/prd.md")" \ + "PLAN_MISSING_AT_COMPLETION" +rm -rf "$WORKDIR" + +WORKDIR=$(mktemp -d) +printf '%s' '{"pendingTasks": [' > "$WORKDIR/plan.json" +assert_subcode "a plan.json that is not parseable JSON is treated as missing" \ + "$(detect_spurious_complete "$WORKDIR" "/tmp/prd.md")" \ + "PLAN_MISSING_AT_COMPLETION" +rm -rf "$WORKDIR" + +WORKDIR=$(mktemp -d) +printf '%s' ' ' > "$WORKDIR/plan.json" +assert_subcode "a whitespace-only plan.json is treated as missing" \ + "$(detect_spurious_complete "$WORKDIR" "/tmp/prd.md")" \ + "PLAN_MISSING_AT_COMPLETION" +rm -rf "$WORKDIR" + +# A run that never owed a plan is still left alone, empty file or not. +WORKDIR=$(mktemp -d) +: > "$WORKDIR/plan.json" +assert_not_spurious "a zero-byte plan.json on a run that owed no plan is not spurious" \ + "$(detect_spurious_complete "$WORKDIR" "")" +rm -rf "$WORKDIR" + +# --- An absent WORKSPACE is not an unproduced artifact: fail open ------------ +# Live-exit and boot-recovery delete the temp workdir right after finalization. +# Adjudicating a run whose directory is already reclaimed would flip a genuine +# success to FAILED, and no re-run repairs that. Missing workdir = cannot judge. +WORKDIR=$(mktemp -d) +rm -rf "$WORKDIR" +assert_not_spurious "an absent workspace fails open rather than reporting spurious" \ + "$(detect_spurious_complete "$WORKDIR" "/tmp/prd.md" "PLAN")" + +# --- Per-command contract: EXECUTE is excluded ------------------------------ +# EXECUTE's required artifact is execution-result.json, written only after a +# successful commit AND push, so a legitimate no-changes run ends without it -- +# and without a plan.json either. It must never be flagged. +WORKDIR=$(mktemp -d) +printf '%s' '{"phase":"Phase 7","status":"IN_PROGRESS"}' > "$WORKDIR/state.json" +assert_not_spurious "a legitimate no-changes EXECUTE run is not spurious" \ + "$(detect_spurious_complete "$WORKDIR" "" "EXECUTE")" +assert_not_spurious "EXECUTE is excluded by name even when a PRD was passed" \ + "$(detect_spurious_complete "$WORKDIR" "/tmp/prd.md" "EXECUTE")" +assert_not_spurious "the execute-prompt CLI spelling resolves to EXECUTE" \ + "$(detect_spurious_complete "$WORKDIR" "/tmp/prd.md" "execute-prompt")" +rm -rf "$WORKDIR" + +# --- Per-command contract: REQUEST_CHANGES owes a plan ---------------------- +# Its result bundle requires plan.json, so "no plan at all" is still caught, +# with or without a PRD. Presence, however, proves nothing on an amend run -- +# the harness seeds plan.json before it starts -- so a zero-byte seed must be +# caught too, and a clean result must not be read as proof the amend did work. +WORKDIR=$(mktemp -d) +assert_subcode "REQUEST_CHANGES with no plan.json is spurious even without a PRD" \ + "$(detect_spurious_complete "$WORKDIR" "" "REQUEST_CHANGES")" \ + "PLAN_MISSING_AT_COMPLETION" +: > "$WORKDIR/plan.json" +assert_subcode "REQUEST_CHANGES with a zero-byte seeded plan.json is spurious" \ + "$(detect_spurious_complete "$WORKDIR" "" "REQUEST_CHANGES")" \ + "PLAN_MISSING_AT_COMPLETION" +rm -rf "$WORKDIR" + +# --- Version skew, both directions ----------------------------------------- +# A command this script has never heard of (an older desktop sending nothing, a +# newer one sending a command added after this release) must not crash or block. +# It falls back to the --prd proxy, i.e. the behaviour before commands existed. +WORKDIR=$(mktemp -d) +assert_subcode "an unknown command with a PRD falls back to the --prd proxy" \ + "$(detect_spurious_complete "$WORKDIR" "/tmp/prd.md" "SOME_FUTURE_COMMAND")" \ + "PLAN_MISSING_AT_COMPLETION" +assert_not_spurious "an unknown command with no PRD is left alone" \ + "$(detect_spurious_complete "$WORKDIR" "" "SOME_FUTURE_COMMAND")" +assert_not_spurious "an empty command with no PRD is left alone" \ + "$(detect_spurious_complete "$WORKDIR" "" "")" +rm -rf "$WORKDIR" + echo echo "passed: $PASS_COUNT failed: $FAIL_COUNT" [[ "$FAIL_COUNT" -eq 0 ]]