Skip to content

Multi-part plan green-gate gap + ci_only_failure verdict misuse kills pipeline runs #4190

Description

@Trecek

What happened

Implementation run impl-20260705-184231-936209 for #4189 (autoskillit fleet run CLI) was killed at the pre-merge fix step. Part A of a two-part plan registered the fleet run subcommand while deletion of the pre-existing deletion-guard test test_fleet_run_command_not_registered (tests/cli/test_fleet_list.py:100) was deferred to Part B — so Part A's local test gate could never pass. resolve-failures fixed 3 of 4 failures, then emitted verdict=ci_only_failure for the remaining locally-reproducible canary failure, and the recipe routed to release_issue_failure → register_clone_failure → escalate_stop.

Root cause — four stacked gaps (one per layer)

  1. make-plan (origin): the 500-line split trigger + "implementation vs. tests" boundary stranded the canary deletion in Part B. Multi-part rules (make-plan/SKILL.md lines 337–343) are scope-isolation only — no invariant that each part must leave task test-check green. The planner's own Registry Tracer flagged the canary CRITICAL (F1) pre-split; the split put the fix on the wrong side and no rule caught it.
  2. dry-walkthrough: read Part A's own admission ("task test-all passes with the single exception of test_fleet_run_command_not_registered") and recorded it under Verified, then stamped the plan "Implement as-is". It checks the test command name (SKILL.md lines 149–156), never the predicted outcome. No pipeline-gate compatibility check exists.
  3. resolve-failures: no verdict exists for a plan-acknowledged deferred failure. The session anchored on the weakened invariant "ci_only_failure MAY be emitted when fixes_applied >= 1…" and skipped the "…if the remaining failures are all CI-only" qualifier. Per the skill's own decision table (lines 186–196) every ci_only_failure row requires Local result = PASS — the canary fails locally (verified empirically in the worktree). It also exited the fix loop after 2 of 3 iterations via the tests-green reporting path (Step 4) instead of Step 5 (Report Failure).
  4. Recipe routing: the pre-merge fix step (implementation.yaml:661–698) is reachable only via a local test failure before any CI run — an honest ci_only_failure is mechanically impossible there. The route functions purely as an unconditional kill for misclassifications. The same trap exists in merge_gate_fix, implementation-groups.yaml, remediation.yaml, and research-implement.yaml.

Recurring regression (3rd generation)

Existing escape hatch never used

check_test_passed ignores xfailed counts (verified empirically: exit 0 + "45 xfailed" → gate passes; locked in by tests/server/test_tools_workspace.py:181). Marking the canary xfail(strict=True) in Part A would have passed the gate with forced cleanup semantics. No skill guidance mentions this mechanism.

Recommended remediation

  1. make-plan: add a multi-part CRITICAL rule — every part must independently pass task test-check; any pre-existing test invalidated by a part's changes must be updated/removed/xfail-bridged in that same part; split boundaries keep gate-prerequisites with the code that triggers them.
  2. dry-walkthrough: blocking FAIL when the plan text declares any test expected to fail after the part's implementation (detectable from a single part; no cross-part read required).
  3. resolve-failures: restore the hard invariant (give partial-fix-with-CI-only-remainder its own verdict if genuinely needed); mandate Step 5 (Report Failure) as the sole exit when tests remain red with iterations remaining; optionally add a plan_blocked verdict for plan-acknowledged deferred failures.
  4. Contracts/orchestrator: enum-constrain verdict in skill contracts (currently unconstrained type: string); reject or re-route ci_only_failure emitted from steps reachable only via local test failure — mechanical precondition validation instead of prose trust.
  5. Test hardening: pin the invariant paragraph in test_resolve_failures_ci_aware.py with an anchored, paragraph-scoped assertion so the next prose softening fails CI.

Artifacts

Investigation

Prior investigation completed interactively. See below for root cause analysis.

Investigation: Pipeline Failure on Issue #4189 — Part-Split Canary Test vs. Per-Part Test Gate

Date: 2026-07-05
Scope: Implementation pipeline run impl-20260705-184231-936209 for issue #4189 (autoskillit fleet run CLI). Failure at the fix step: resolve-failures returned verdict=ci_only_failure for a locally-failing, plan-acknowledged canary test, routing the run to release_issue_failure → escalate_stop.
Mode: Standard (extended: 9 subagents + direct empirical verification)

Summary

The run was killed by a stack of four independent gaps, one per pipeline layer, none of which is a "bug" in isolation. (1) Planning: make-plan split a 644-line plan at the mechanical 500-line trigger using an "implementation vs. tests" boundary, which stranded the removal of a pre-existing deletion-guard test (test_fleet_run_command_not_registered) in Part B while Part A registers the very command the guard forbids — guaranteeing a red test gate after Part A. make-plan has no rule that each part must independently pass the test gate. (2) Validation: dry-walkthrough read the plan's own admission that the test would stay red, recorded it under "Verified" as a correct factual statement, and stamped the plan — it has no pipeline-gate compatibility check. (3) Execution: resolve-failures fixed 3 of 4 failures, then faced a failure it correctly understood as plan-deferred but had no verdict for; it misapplied a previously-weakened invariant and emitted ci_only_failure for a failure that reproduces locally — definitionally wrong per its own decision table. (4) Routing: in the pre-merge fix step, which is only ever reached via a local test failure before any CI run, ci_only_failure can never be legitimate; its route is a hard-kill misclassification sink. The verdict misuse is a three-generation recurring regression (#1954 fixed → #3610 fix weakened the invariant → today's misread), and the regression test guarding the invariant is too broad to detect the weakening (empirically verified).

Root Cause

Layered, in causal order:

RC1 — make-plan split invariant gap (primary origin). The plan session's adversarial Registry Tracer correctly flagged the canary as CRITICAL (F1) and the fix (Step 4: delete the test) was added to the unsplit draft. When the 500-line rule forced a split, the planner categorized Step 4 as "test infrastructure" and moved it to Part B. The split rules in make-plan/SKILL.md (verified at lines 337–343) are exclusively about scope isolation — there is no "each part must leave the gate green" invariant, so no rule was violated. Part A's own Verification section (line 357 of the plan) then stated the failure as an accepted outcome. [SUPPORTED — SKILL.md text, plan files, plan session log]

RC2 — dry-walkthrough has no gate-compatibility check. The verify session explicitly wrote: "test_fleet_run_command_not_registered … is the single expected failure, exactly as the plan states (removal deferred to Part B)" — under its Verified section — then stamped the plan and recommended "Implement as-is." Its checklist checks the test command name, never the predicted test outcome. Its multi-part isolation rule ("MUST NOT read any other part files") also prevents cross-part reasoning, but no cross-part read is needed: the plan's own text declaring a post-part red test is detectable from Part A alone. [SUPPORTED — verify session JSONL, SKILL.md lines 88–156]

RC3 — resolve-failures verdict vocabulary gap + invariant misapplication. The fix session identified the canary as plan-deferred on its first reasoning turn, fixed the 3 unrelated failures (commits 999ef83c2, 58d5bbc30), re-ran tests (28,077 passed / 1 failed), then emitted ci_only_failure with zero deliberation over alternatives. Per the skill's own decision table (lines 186–196), every ci_only_failure row requires Local result = PASS; the post-fix invariant (lines 333–336) permits it with fixes_applied >= 1 only "if the remaining failures … are all CI-only." The canary fails locally — I verified this empirically in the worktree ('run' in subcommands → assertion False). The agent anchored on fixes_applied=2 >= 1 and skipped the CI-only qualifier. Secondary violations: it exited the fix loop after 2 of 3 allowed iterations with red tests, and used the Step 4 (tests-green) reporting path instead of Step 5 (Report Failure). [SUPPORTED — fix session JSONL, SKILL.md, empirical check]

RC4 — pre-merge ci_only_failure route is a semantic trap. The fix step (implementation.yaml:661–698) is reached exclusively after a local test_check failure; no CI has run and the CI context refs are empty in this path. A verdict meaning "failure only reproducible in CI" is therefore mechanically impossible if emitted honestly — the route ci_only_failure → release_issue_failure in this step functions only as an unconditional kill for misclassifications. The same trap exists in merge_gate_fix and in implementation-groups.yaml, remediation.yaml, research-implement.yaml. [SUPPORTED — recipe YAML, routing analysis]

Affected Components

  • src/autoskillit/skills_extended/make-plan/SKILL.md (lines 337–343): split trigger + multi-part rules; missing green-gate invariant [SUPPORTED]
  • src/autoskillit/skills_extended/dry-walkthrough/SKILL.md (lines 88–156): no test-gate outcome check; part-isolation rule [SUPPORTED]
  • src/autoskillit/skills_extended/resolve-failures/SKILL.md (lines 186–196 decision table; 323 verdict list; 333–336 weakened invariant) [SUPPORTED]
  • src/autoskillit/recipes/implementation.yaml (fix step 661–698; merge_gate_fix 718–757; next_or_done 820–829; kill chain 2352–2497) [SUPPORTED]
  • src/autoskillit/recipes/contracts/implementation.yaml (line ~131): verdict is unconstrained type: string, no enum [SUPPORTED]
  • src/autoskillit/execution/testing.py (check_test_passed, 243–265): binary gate; xfailed does not fail — empirically verified [SUPPORTED]
  • tests/skills/test_resolve_failures_ci_aware.py (line 292): DOTALL regex passes despite invariant weakening — verified by reading the regex against current SKILL.md text [SUPPORTED]
  • AutoSkillit repo tests/cli/test_fleet_list.py:100–103: the deletion-guard canary, added 7dda8dc81 (PR Implementation Plan: Fleet Restructure T3: CLI Split — fleet dispatch + fleet campaign #1242, 2026-04-25), no removal annotation, no pytest marker [SUPPORTED]

Data Flow

plan captures plan_parts → per part: verify (dry-walkthrough stamps) → create_impl_worktreeimplement (skill forbids running tests; canary failure invisible here by design) → test (test_checktask test-check, binary, local) → on failure check_test_fix_loopfix (resolve-failures emits free-string verdict) → on_result routing. Part A merges to the feature branch (mergeinter_part_push) before Part B begins (next_or_done), so the Part A test gate runs with zero awareness that Part B exists. The verdict string flows uncheck-ed (no enum, no precondition validation) into routing that terminates the run.

Test Gap Analysis

  1. The invariant regression test cannot detect its own subject regressing. test_ci_only_failure_requires_no_fix_applied matches (no fix|never.*fix|fixes_applied.*0).*ci_only_failure with re.DOTALL over the whole document; the Step 2d header text ("Applies ONLY when no fix was applied (fixes_applied == 0)") thousands of characters away from the weakened invariant keeps it green. The 476059f weakening shipped without any test failing. [SUPPORTED — verified against current text]
  2. No contract-level verdict validation. The skill contract declares verdict: type: string with no enum, so any string — including a semantically impossible one — passes validation and routes. [SUPPORTED]
  3. No recipe semantic rule checks that a verdict-gated route's verdict is satisfiable in its context (e.g., ci_only_failure routed from a step reachable only via local test failure). [SUPPORTED]
  4. No test exercises the multi-part red-gate scenario end-to-end (plan part declaring an expected failure → pipeline behavior). [SUPPORTED — absence via grep across recipes/server/skills]

Similar Patterns

  • xfail(strict=True) is the existing, proven bridge mechanism. check_test_passed ignores xfailed counts (empirically verified: check_test_passed(0, "…45 xfailed…") == True), the suite already uses xfail in 8+ files, and tests/server/test_tools_workspace.py:181 explicitly locks in that xfailed ≠ failure. Marking the canary xfail(strict=True, reason="removed in Part B") in Part A would have passed the gate, and strict=True forces cleanup. [SUPPORTED]
  • The canary pytest marker is a different concept ("development-time filter verification tests", pyproject line 106) — excluded from all task test-check variants but not intended for deletion guards. [SUPPORTED]
  • Deletion-guard precedent: the run_skill_retry guards were removed in later test-audit passes (3a43a0df0, 40ae65b80) — valid only for permanently-deleted symbols. For intentional re-registration (this case), the guard must die in the same commit that re-registers the name. Part A's commit 671e7e325 registered run and left the guard untouched. [SUPPORTED]

Design Intent Findings

Historical Context

This is a recurring pattern — at least the 4th manifestation of "verdict precondition not met → terminal routing kills pipeline":

Prior fixes were prose-layer (SKILL.md edits) guarded by regexes too broad to detect softening; no layer mechanically validates verdict preconditions. This is a recurring pattern — consider running /rectify for architectural immunity after resolving the immediate issue.

External Research

GitHub issue search (TalonT-Org/AutoSkillit) surfaced the historical issues above; issue #4189's body contains no mention of the canary or a two-part split — the split was purely a planning-session decision. No external library/web factors are implicated (failure is entirely internal orchestration logic).

Scope Boundary

Investigated: make-plan / review-approach / dry-walkthrough / implement-worktree-no-merge / resolve-failures skill texts; all five session logs of the failed run; implementation.yaml + implementation-groups.yaml routing; test_check/check_test_passed code paths (empirically); canary origin and precedents in the AutoSkillit repo; verdict invariant git lineage; prior related issues and fixes.
Not yet explored: whether the open scope of #3610 overlaps further; live multi-part runs at risk in remediation.yaml/research-implement.yaml (same routing shape, unexamined runs); Codex-backend variants of the fix path; the review_pr/audit_impl stages (never reached); the implementer's separate pre-commit rule violation ("pre-existing" contract-freshness blame) — noted but not chased.

Confidence Levels

All findings above marked inline. Every load-bearing claim (canary fails locally; xfailed passes the gate; ci_only_failure rows require local PASS; invariant weakened in 476059fbb; regression regex blind to the weakening; make-plan lacks green-gate rule) was verified directly by me — code read, diff read, or executed — not only by subagents: SUPPORTED. The only NEEDS-EVIDENCE item: why the fix agent skipped the third loop iteration (its transcript records no rationale; the early exit is observed fact, the motivation is inference).

Recommendations

Immediate (unblock #4189):

  1. Retry the issue with the plan corrected: move canary deletion (Part B Step 4) into Part A — the precedented same-commit removal for intentional re-registration — or, if the parts must stay as-is, add pytest.mark.xfail(strict=True, reason="canary removed in Part B") in Part A and delete the test in Part B. The preserved worktree's 3 commits are salvageable. Remove the fail label on retry.

Systemic (defense per layer, in priority order):
2. make-plan: add a fifth CRITICAL multi-part rule — every part must independently leave task test-check green; any pre-existing test invalidated by a part's changes must be updated/removed/xfail-bridged in that same part; split boundaries must keep gate-prerequisites with the code that triggers them.
3. dry-walkthrough: add a blocking check — if the plan text states any test is expected to fail after this part's implementation, emit Dry Walkthrough FAILED. Detectable from a single part; no cross-part read required.
4. resolve-failures: restore the hard invariant ("NEVER when fixes_applied >= 1"), giving the partial-fix-with-CI-only-remainder case its own verdict if genuinely needed; require Step 5 (Report Failure) as the sole exit when tests remain red and iterations remain; optionally add a plan_blocked verdict for plan-acknowledged deferred failures so the recipe can route it distinctly (diagnostic/human pause) instead of mislabeling.
5. Contract/orchestrator (the /rectify candidate): enumerate allowed verdicts in skill contracts (enum, not free string) and add a semantic rule or runtime check that ci_only_failure is rejected (or re-routed to re-diagnosis) when emitted from steps reachable only via local test failure — mechanical precondition validation instead of prose trust.
6. Test hardening: pin the invariant paragraph in test_resolve_failures_ci_aware.py with an anchored, paragraph-scoped assertion so the next prose softening fails CI.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugExisting behavior is brokenrecipe:remediationRoute: investigate/decompose before implementationstagedImplementation staged and waiting for promotion to main

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions