Skip to content

fix(pipeline-conductor): give NOPROGRESS the spec row it shipped without - #8808

Merged
iamwhatever merged 2 commits into
kirodotdev:mainfrom
LuisBrel:fix/noprogress-spec-row-8192
Sep 5, 2026
Merged

iamwhatever merged 2 commits into
kirodotdev:mainfrom
LuisBrel:fix/noprogress-spec-row-8192

Conversation

@LuisBrel

@LuisBrel LuisBrel commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Item 7 of #8192. NOPROGRESS landed in fleet_probe.py in #8035 and sits in the probe's _FIRING set, so the probe prints it — but the action table in pipeline-conductor/SKILL.md has no row for it. The conductor receives a fired line and looks it up against a table that answers every other tag, which is the issue's own thesis one level up: a lookup that returns nothing is indistinguishable from a lookup that says "do nothing".

Worse than a missing row on its own, the document still carried the procedure the tag replaced. The patrol-loop message told the conductor to "diff each fired line's i= against the recorded one", and the i= paragraph presented that two-cycle diff as the reader's job. The probe now holds both numbers and makes the comparison itself, so the document offered a second, weaker answer to a question already answered — and the weaker one is enforced by nothing, so it may simply never happen.

The general rule the issue states: whoever authorizes a new signal owns its spec row, because the contributor who implements the signal may not own the spec file.

What changed

src/kiro_crew/builtin_skills/pipeline-conductor/SKILL.md, three places:

  • A NOPROGRESS row in the probe action table. It states the firing condition (nothing produced since the last mark, and that mark at least one idle_alert_secs old), routes to an EFFECT check, never liveness — artifact, remote head, new commit — and then routes the absent-effect case on the line's own age, because two paths reach this tag and they do not mean the same thing. Within idle_alert_secs the transcript is warm, held alive by inbound traffic the session never answers, so the first move is not a nudge (a nudge is more of the input that produced the reading) and the ladder is entered at its Inspect step. Past idle_alert_secs the session is cold as well as unproductive and IDLE's ladder applies from the top: the classifier ranks the tag below the clock, but the suppression fallback substitutes it for an already-dispositioned report with no age test, so a cold line can carry it.
  • The patrol-loop message now names NOPROGRESS → check the EFFECT, never liveness in its tag enumeration, in place of the manual i= diff instruction.
  • The i= paragraph attributes the comparison to the probe and says never to diff two cycles by eye; i= is described as the corroborating number rather than the test.

No probe behaviour changed. This is the spec side only.

Why it matters

A tag with no row is not a cosmetic documentation gap here, because the conductor's dispositions are tag-keyed lookups against exactly this table. The two readings it could fall back to are both wrong in a specific way: treat it as IDLE and it nudges — which, on the warm path, is the input that produced the state — or treat it as unrecognized and the one signal that survives a self-deadlocked worker is dropped, since a worker that cannot produce a message also cannot produce any other tag.

Leaving the replaced manual procedure in place had its own cost. A conductor following it re-derives what the probe already computed, and the derivation lives in prose that nothing verifies, so the check that was moved into code for enforceability was still documented as optional human diligence.

Tests

New in test/test_pipeline_conductor_skill_contract.py:

  • test_every_firing_tag_has_a_row_in_the_action_table — loads the probe script and reads _FIRING from it rather than restating the tag list, so it fails for any future tag that ships without a row. That is the issue's general rule turned into a gate.
  • test_the_noprogress_row_routes_to_effect_and_not_to_a_nudge — the row must exist, must route to the effect check, and must route the absent-effect case on the printed age rather than describing the tag as warm-only.
  • test_the_probe_makes_the_no_progress_comparison_itself — the manual two-cycle diff must not be re-documented beside the tag.
  • test_the_loop_message_names_noprogress_instead_of_a_manual_diff — the armed patrol message is the list that actually runs; a tag missing there is an instruction never received.

Written before the source change and confirmed red for the right reason — the generic one reported probe tags with no row in the action table: ['NOPROGRESS'], exactly one tag.

Mutation-proved: with SKILL.md reverted to origin/main and the tests kept, 4 failed, 82 passed; with it restored, 86 passed.

pytest test/test_pipeline_conductor_skill_contract.py -n0 -q          -> 86 passed
pytest test/test_pipeline_conductor_skill_contract.py \
       test/test_pipeline_conductor_agent.py \
       test/test_pipeline_conductor_probe_roundtrip.py \
       test/test_pipeline_conductor_claim_preflight.py -n0 -q         -> 516 passed, 8 failed

The 8 are OSError: [WinError 1314] A required privilege is not held by the client from TestFleetProbe's symlink fixtures — unprivileged Windows cannot create a symlink. They are environmental and pre-existing; nothing in this change touches the probe or its fixtures.

Gates, from the worktree root: isort --check-only exit 0, flake8 exit 0, mypy src/kiro_crew/Success: no issues found in 1296 source files, scripts/check_black_formatting.pyblack gate passed over 2 changed files in scope.

The second commit is a review-driven correction, taken test-first like the first: the GPT lane pointed out that the original row's "only ... held WARM" claim skipped IDLE's nudge for a cold session. Checked against the source and it is right — _stalled_since_disposition is age-gated on the classifier path only, and the suppression fallback has no age test — so the row now routes on the printed age, with the new assertion added and confirmed red before the prose changed.

CI on d2ba2b1: all 66 checks pass, including Docs Lint, Fork PR Description, Backend Tests on Linux and Windows, E2E and the four fork-lane AI reviews (GPT no findings, Opus no findings, Design PASS, First Principles PASS).

Pattern harvest

Rule candidate: when a change adds a signal an agent branches on — a probe tag, a verdict word, an exit code — the same change adds its row to the table the agent looks it up in, and a test reads the emitting side's own constant so the next one cannot ship without it. The value is in reading the constant rather than restating the list: a hand-maintained list in the test is one more place the new signal has to be registered, and forgetting to register it is the exact failure being guarded. #8035 added the tag, its tests covered the tag's behaviour thoroughly, and every one of them passed while the conductor had nowhere to look the tag up.

Deliberately out of scope: items 1-6 of #8192. Item 1 is #8736. Item 5's separate complaint — that an expiring IDLE/NOPROGRESS mark re-fires on an unchanged digest — is untouched and stays open, so Refs rather than Closes.

Refs #8192

The probe fires NOPROGRESS and the action table it is read against had no row for it, so the conductor received a line with no defined action -- while the document still told it to diff each fired line's i= by hand, the comparison the tag replaced.
@LuisBrel
LuisBrel requested a review from a team as a code owner September 5, 2026 20:16
@LuisBrel
LuisBrel requested a review from dwu96 September 5, 2026 20:16
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed d2ba2b175f4f80c890793c6231100dfeab9f8932 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] d2ba2b1

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of d2ba2b175f4f80c890793c6231100dfeab9f8932 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real tag-keyed lookup gap, closed at the root: the generic gate reads _FIRING from the probe itself, so no future tag ships row-less.

Verified against base: fleet_probe.py fires NOPROGRESS on both the classifier path (fleet_probe.py:1045, age-ranked below the clock) and the suppression fallback (fleet_probe.py:1087, no transcript-age test), while the base SKILL.md has zero mentions of the tag — the two-path/route-on-age row matches the code, and removing the superseded manual i= diff eliminates the competing prose procedure rather than leaving two answers standing. Diff and description correspond exactly; no probe behavior changed.

[DESIGN-REVIEWED] d2ba2b1

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed d2ba2b175f4f80c890793c6231100dfeab9f8932 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] d2ba2b1

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of d2ba2b175f4f80c890793c6231100dfeab9f8932 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base tree. Producing the review now.

First-Principles-Verdict: PASS

A shipped probe tag finally gets the table row the conductor keys on, the replaced manual procedure is deleted, and a gate reads _FIRING so the gap cannot recur.

What this change ships

Intent: make the conductor's action table answer the NOPROGRESS tag the probe already fires, and retire the manual procedure that tag replaced. This is a FIX.

  1. The action table gains a NOPROGRESS row routing to an effect check, split on the line's age — justified (tag is in _FIRING, fleet_probe.py:372, with no row in base SKILL.md; both routing claims match fleet_probe.py:1035-1045 and 1081-1087)
  2. The patrol-loop message names NOPROGRESS in its tag list — justified (the list is the instruction that runs)
  3. The patrol-loop message drops "diff each fired line's i=" — justified subtraction (the probe computes it in _stalled_since_disposition, fleet_probe.py:620)
  4. The i= paragraph now attributes the comparison to the probe, forbids the by-eye diff — justified subtraction of a second, unenforced answer
  5. A test reads _FIRING from the probe script and fails any tag lacking a row — justified, cause-level (fixes "signal ships without its spec row" generally, not just this tag; helper load_skill_script pre-exists in 12 other test files)
  6. Two tests pin the row's effect-not-nudge routing and age split — justified, matches the file's existing prose-pinning convention
  7. A test pins the loop message's NOPROGRESS entry and refuses the old diff instruction — justified

Every item is the declared fix or its gate; nothing rides along, no new public surface, no config keys, and the row's two-path age claim was checked against the source rather than taken from the description.

[FIRST-PRINCIPLES-REVIEWED] d2ba2b1

The suppression fallback substitutes NOPROGRESS for an already-dispositioned report with no age test, so the tag is not warm-only and a cold line was being sent past the nudge it does need.
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 5, 2026
@iamwhatever
iamwhatever enabled auto-merge (squash) September 5, 2026 22:07

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: the pipeline-conductor probe fires a NOPROGRESS tag that SKILL.md never documented, so the conductor had no action row for it — this adds the row (check the EFFECT, never liveness; route on the line's own age) and replaces the by-eye two-cycle index diff the probe now performs itself, with a contract test pinning the row. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

@iamwhatever
iamwhatever merged commit 775e4e3 into kirodotdev:main Sep 5, 2026
78 of 84 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants