fix(babysit): stop the gated ack contradicting itself at max_cycles=0

[bolichen97]

Fixes #8100.

Problem

When monitor_start armed a GATED loop with an explicit max_cycles=0 (documented as "unlimited"), the acknowledgement contradicted itself in one sentence. The gated cadence clause in src/kiro_crew/mcp_tools/control.py unconditionally rendered the {max_cycles or 0} cap counts delivered turns while the shared tail appended , with NO cycle cap for the same falsy value, so the agent read both a 0-cap and a no-cap.

This matters because the ack is what the model reads back to learn the loop's semantics, and the cycle cap is the runaway backstop. An ack that asserts both a cap and no cap leaves the agent unable to tell whether a bound exists on exactly the parameter that decides whether an unattended loop can spend without limit.

Fix

Gate the cadence clause on max_cycles being truthy, so the falsy case is described exactly once by the existing , with NO cycle cap tail:

"cost no turn"
+ (f" and the {max_cycles} cap counts delivered turns" if max_cycles else "")

The ungated branch was unaffected and left unchanged.

Which side was wrong, and why the ack moved rather than the engine

0 genuinely means unlimited here, on both of the surfaces that define it, so the ack was the wrong one:

• Engine — autonudge.py's cap check is if loop.max_cycles and loop.cycle_count >= loop.max_cycles, so a falsy cap is never enforced. A 0 loop runs until autonudge_stop, the STOP sentinel, or max_runtime_secs.
• Skill — the babysit skill documents max_cycles as a runaway backstop rather than a finish line, and says to "pass 0 for unlimited only when the user explicitly asks for an unbounded loop".

So the engine and the documented contract already agree that 0 is unlimited-on-purpose; only the ack disagreed with them. Making 0 refuse would be a behaviour change to a documented escape hatch, and making the message vaguer would hide the one fact the reader needs. Documented behaviour is therefore unchanged, and no spec or SKILL.md moves with this commit.

Class audit — one site, not an instance fix

Both clauses were grepped repo-wide (NO cycle cap, cap counts, max_cycles or 0):

Site	Renders a cap-counts clause?	State
mcp_tools/control.py:963 (gated cadence)	yes	fixed here
mcp_tools/control.py:969 (shared tail)	no — already if max_cycles else ", with NO cycle cap"	already correct
dashboard/session_directive_apply.py:238 (directive applier ack)	no such clause exists on that path	not affected

There is no second site that can render the contradiction, so this closes the class.

Test

test/test_monitor_start_ack.py gains two tests pinning both sides:

• test_a_gated_loop_with_zero_max_cycles_does_not_contradict_itself — an unlimited gated loop must positively declare NO cycle cap and must not also claim cap counts. Asserting the positive matters: mere silence would let a bounded-sounding ack ship for a loop that can spend without limit.
• test_a_gated_loop_with_a_truthy_max_cycles_states_the_cap — the counterpart, so the bounded branch is pinned from the other side and the two clauses can never both appear.

Confirmed the first fails without the fix and passes with it.

Pattern harvest

The two clauses describing one parameter sat ~6 lines apart in a single chained-+ expression, and only one of them had the truthiness guard. max_cycles or 0 reads as defensive normalisation, which is exactly why the missing guard survived review: it looks like the falsy case was handled, when it only coerced None into a value that then rendered as a false claim.

Rule candidate: when two fragments of one user-facing message describe the same parameter, every fragment must share the parameter's falsy guard — an x or 0 in a template is a rendering default, never evidence the empty case was considered. Too narrow to encode as a lint (there is no way to tell which fragments describe the same value), and the general form — "no hardcoded user-facing strings, one owner per limit" — is already in AGENTS.md; the durable defence is the test shape used here, asserting the message states the fact exactly once from both sides rather than merely asserting the absence of the wrong word.

Verification

• test/test_monitor_start_ack.py passes in full, plus the babysit / autonudge / monitor suites.
• isort, flake8, mypy --platform linux, scripts/check_black_formatting.py, scripts/check_builtin_skill_scope.py, scripts/docs-lint.sh, and test/test_security_posture.py clean.
• Rebased onto current main and squashed to one commit. The earlier three-dot range that two review lanes read as also containing a skills.py sync rework was stale-base noise: that work is main's own fix(skills): sync an installed builtin when only its scripts change #8050, this branch never carried it, and the range is now exactly the two files above (+33/-1).

Refs #7634.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 83e4e0b599058b4ea549157866ff111a20490919 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

One-line guard fixes the ack at the correct layer — engine and skill already define 0 as unlimited (verified at autonudge.py:2584) — pinned from both sides by tests.

[DESIGN-REVIEWED] 83e4e0b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 83e4e0b599058b4ea549157866ff111a20490919 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 83e4e0b

Verdict parsed from the review's SHA-scoped output markers for commit 83e4e0b599058b4ea549157866ff111a20490919.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 83e4e0b599058b4ea549157866ff111a20490919: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 83e4e0b599058b4ea549157866ff111a20490919 — 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. The fix, the class audit, the engine semantics, and the skill documentation all check out against the repository. Here is the review:

First-Principles-Verdict: PASS

A one-guard fix that makes the ack stop asserting a 0-cap and no-cap at once; nothing rides along, nothing new is added.

What this change ships

Intent: make the arming acknowledgement for an unlimited gated loop describe its cycle cap exactly once, so the model reading it knows no bound exists. This is a FIX.

1. A gated loop armed with max_cycles=0 no longer reads "the 0 cap counts delivered turns" alongside "NO cycle cap" — justified, and it fixes the mechanism (the ungated fragment), not the symptom.
2. Two tests pin both sides of the guard (zero declares no-cap positively; truthy states the cap and never denies it) — justified, part of the fix.

No new capability, config key, flag, or public surface is introduced, so there is nothing to consumer-count. The class audit in the description reproduces: grepping NO cycle cap|cap counts|max_cycles or 0 under src/ yields exactly three message sites — control.py:963 (fixed here), control.py:969 (already guarded), and session_directive_apply.py:238, whose gated cadence block (lines 248–251) carries no cap-counts clause — so zero unfixed siblings. The premise that 0 means unlimited is derived, not asserted: the engine's cap check at autonudge.py:2584 never enforces a falsy cap, and babysit/SKILL.md:598 documents 0 as the explicit-unbounded escape hatch, so correcting the ack rather than the engine is the right level. The bounded branch's apparent double mention ("cap counts delivered turns" + "stopping after N cycles") is disambiguation, not duplication — for a gated loop the cap counts delivered turns, not quiet cycles (autonudge.py:2656) — and it predates this change.

[FIRST-PRINCIPLES-REVIEWED] 83e4e0b

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 83e4e0b599058b4ea549157866ff111a20490919 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 83e4e0b

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 83e4e0b599058b4ea549157866ff111a20490919: <one-sentence reason>

[bolichen97]

• src/kiro_crew/skills.py:1373 — Linked destinations are traversed before the link guard — rebutted (not this PR's code)

BLOCKING -- src/kiro_crew/skills.py:1373 -- Linked destinations are traversed before the link guard
dest_newest = _tree_newest_mtime(dest_dir)
Linked builtin destination containing SKILL.md -> startup sync -> os.walk follows the root link -> out-of-root paths are recursively enumerated.
Fix: Treat linked destinations as update-due before either mtime walk.

This branch never touched skills.py. Both of its commits are reproducible per-file:

47618b4c6 fix(babysit): ...   src/kiro_crew/mcp_tools/control.py | 3 ++-
                              test/test_monitor_start_ack.py     | 14 +++++
58c23af84 test(babysit): ...  test/test_monitor_start_ack.py     | 17 +++++

The line the finding anchors on is main's own, from 5081f18f1 "fix(skills): sync an installed builtin when only its scripts change (#8050)". That commit landed after this branch's base 34a61d2d, which is the whole mechanism: git merge-base --is-ancestor 5081f18f1 34a61d2d is false, so a two-dot origin/main..HEAD comparison renders #8050's hunks into the branch's apparent diff even though no commit here wrote them. The three-dot diff against the merge base was always the two files above, +33/-1. The branch is now rebased onto current main, so the merge base is the main tip and two-dot and three-dot coincide — the file cannot appear in any comparison of this branch.

The mechanism is nonetheless real against main, and I checked rather than dismissing it. _tree_entries does guard links, but only ones it finds in dirnames beneath the root: it lstats each child, classifies S_ISLNK or is_link_or_junction as "link", and removes it from dirnames so the walk never descends. The root handed to os.walk is never subjected to that test, so a dest_dir that is itself a symlink or junction is enumerated into its target. What that yields is directory metadata only — names, sizes, modes, os.lstat mtimes — for an integer comparison; no file content is read, nothing reaches the model, and nothing is written into the target. The observable consequence is a wrong update_due.

Declining to fix it here is a scope judgement, not a waiver. This PR is a 33-line ack-string fix that cannot introduce or regress that path, and editing skills.py here is precisely the scope creep the First Principles lane blocks this PR for — the two blocking lanes would then demand opposite things. It belongs as its own issue against skills.py, where the fix (treat a linked destination as update-due before either mtime walk, matching how _tree_entries already classifies a linked child) can carry its own test.

[GPT-REVIEWED] 58c23af

[bolichen97]

• The skills.py sync change duplicates a landed main commit and is undeclared — fixed (rebased; the demanded subtraction was applied exactly)

Half this diff is an undeclared skills-sync rework that already landed on main as #8050 — the declared ack fix stands alone without it.
Subtraction: rebase onto current main so the PR carries only the control.py hunk and the two test_monitor_start_ack.py tests.

Done, and the remedy this lane asked for is the one applied. The branch is rebased onto current main, and the range is now exactly what the lane specified: src/kiro_crew/mcp_tools/control.py (+2/-1) and test/test_monitor_start_ack.py (+31). The two commits were also squashed to one, keeping headroom under the hygiene ceiling.

The lane's reading was right about the bytes and wrong about the authorship, and the distinction matters for the record. Every skills.py and test_builtin_skill_sync_safety.py hunk was byte-identical to main because main is where they came from — 5081f18f1 (#8050) landed after this branch's base 34a61d2d, so a two-dot origin/main..HEAD comparison renders them into the apparent diff. No commit on this branch ever wrote that file: 47618b4c6 touched control.py + the test, 58c23af84 touched only the test. The three-dot diff against the merge base was always +33/-1 across two files. So there was nothing to subtract, only a stale base to move — which is now moved, making the range unambiguous under either comparison.

The description mismatch the lane flagged is also gone. The body no longer claims only what it happens to touch by luck; it names the two files, states which side of the max_cycles=0 disagreement was wrong and why (the engine's cap check is if loop.max_cycles and ..., so a falsy cap is never enforced, and the babysit skill documents 0 as unlimited-on-request — the ack was the only surface disagreeing with both), and carries the repo-wide class audit showing session_directive_apply.py renders no cap-counts clause and so needs no matching change.

[FIRST-PRINCIPLES-REVIEWED] 58c23af

[NicholasRBowers]

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: fix with clear root cause -- the monitor_start ack rendered both the cap-counts clause and the NO-cycle-cap clause when max_cycles=0; the cadence clause now shares the tail's truthiness guard (message text only, no gating-logic change).