feat(conductor): one code owner for the ledger item entry (#5912)

[NicholasRBowers]

Problem / Motivation

The goal-conductor's only compaction-surviving state is the per-work-item entry it writes into the session ledger's artifacts map — and that entry format had no code owner. SKILL.md specified it as prose plus a worked example, and the model re-derived the encoding every patrol cycle. Review of PR #5652 caught two real defects this produced: an acceptance spec that lived only in model context (lost on compaction), and an entry documented as a nested JSON object, which dashboard/handlers/session_ledger.py rejects with artifacts_not_string_map — so nothing persisted, silently.

Why it matters

A conductor that loses an item's acceptance spec cannot evaluate, steer, or resume that item after compaction — the goal silently degrades into orphaned child sessions. Both observed failure modes are silent: the ledger truncates an oversized value (corrupting the stored JSON) rather than rejecting it, and a non-string value never persists at all. A format only enforced by prose will keep regressing.

What changed (motivation → approach → change)

The prose example was the specification, and models drift from prose — so the format needs one code owner the conductor calls instead of re-deriving bytes. Added as a sibling script (not an accept_eval.py mode) to keep the evaluator single-purpose:

• src/kiro_crew/builtin_skills/goal-conductor/scripts/ledger_entry.py — stdlib-only CLI codec, JSON on stdin/stdout, structured errors (never a crash), exit 2 only for malformed invocation:
  • encode: fields → the single-line JSON string the ledger accepts; rejects unknown fields, an unknown status (a synonym like "done" would silently read as active and never rotate), and a value the ledger would truncate.
  • decode: stored value → structured fields + terminal/complete flags; structured error on malformed input, including RecursionError on pathologically nested JSON.
  • validate: enforces the ledger's real bounds before a write (value ≤ 2000, key ≤ 128, ≤ 32 entries, value-is-a-string) — mirrored from session_ledger.py and drift-guarded by test against the ledger's own constants.
  • rotate: collapses terminal entries to a one-line outcome, drops oldest-terminal-first only when over the entry cap, never drops an active item; docstring pins the whole-map write-back contract (the ledger merges, so a partial write-back would invert age-out order).
  • Retry semantics in the status vocabulary: a failed acceptance check keeps status: running with a fails counter (the three-strikes stop's durable state); fail means finally given up. This prevents rotation from destroying the spec/session/cursor an item still being retried needs.
• SKILL.md — the worked byte-format example is gone; a new "ledger item-entry codec" section routes encode/decode/validate/rotate through the script. The WHEN-to-write prose stays; the HOW is now helper invocations.
• agent.py — the conductor system prompt and installer docstring now name both bundled scripts as the shell grant's purpose.
• test/test_conductor_agent.py — the doc-ratchet test now pins the codec-owned contract instead of the removed worked example, and _load_evaluator uses the shared load_skill_script helper (the hand-rolled exec_module was leaving __pycache__ residue beside the checked-in script).

Tests

test/test_conductor_ledger_entry.py (46 tests):

• Bounds drift-guard: codec constants asserted equal to session_ledger._MAX_TEXT / _MAX_ARTIFACTS (and the key clamp literal), so a ledger-side change fails this test instead of silently drifting.
• encode/decode round-trip, determinism, optional fields; rejection of missing/unknown fields, bool-as-int round, unknown status, and values the ledger would truncate.
• decode structured errors: non-string value (the artifacts_not_string_map shape), non-JSON, non-object, missing status, deep-nesting RecursionError; unknown fields preserved for forward compatibility.
• validate: every bound at and past its edge.
• rotate: terminal collapse, idempotence, oldest-terminal-first drops under the cap, never-drop-active (structured cap_exceeded_all_active), opaque/malformed entries preserved, and a running+fails retrying item surviving rotation whole.
• CLI process contract: round-trip, domain errors exit 0, usage/malformed-stdin/JSON-array exit 2 with the cause on stdout, no traceback.
• End-to-end: an encoded entry survives a real session_ledger.record write and reads back byte-identical.

Manual verification

N/A — unit coverage sufficient: the codec is a pure JSON transform with a subprocess CLI contract, both exercised directly; the end-to-end test writes through the real ledger store.

Related Issues

Fixes #5912

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Why no screenshot: backend skill script + SKILL.md prose only; nothing renders in the browser.

[NicholasRBowers]

Pre-push local review dispositions (GPT lane, judged commit 0042ed3; fixes amended into 1e2456f):

• fail-is-terminal destroys retry state — fixed

  A failed acceptance check must not collapse the entry while retries remain; the three-strikes stop needs the spec/session/cursor.

  encode now enforces a status vocabulary (running|waiting|pass|fail) with retry semantics stated in code and SKILL.md: a failed check keeps status: running with a fails counter; fail means finally given up. Locked by test_rotation_preserves_a_failing_but_retrying_item and test_encode_accepts_fails_counter.

• RecursionError on deeply nested stored JSON crashes decode — fixed

  decode promises a structured error on malformed input but caught only JSONDecodeError.

  RecursionError now returns a structured not_json error; locked by test_decode_deeply_nested_json_is_a_structured_error.

• shell interpolation / python3-on-Windows: expose the codec as a typed MCP tool instead of execute_bash — rebutted

  The proposed remedy is a redesign of the skill's established invocation surface, out of proportion to this PR's purpose.

  The codec uses the exact invocation pattern the acceptance evaluator established when it merged in PR feat(conductor): ship the conductor agent + acceptance-evaluator skill as built-ins #5652 (stdin JSON piped to a bundled script via the conductor's audited, approval-gated execute_bash grant); the interpolated values are conductor-authored spec fields, not arbitrary end-user text, and the codec itself spawns no subprocess. Moving bundled-script invocation to a typed MCP tool is a legitimate design question for the skill as a whole (evaluator included) and belongs to its own change, not this one.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 16ec8b8465465f4e61b73f3da8349271856a8796 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 16ec8b8

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 16ec8b8465465f4e61b73f3da8349271856a8796 — 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: session_ledger.py:370-373 does silently clamp values and blindly age out the oldest 32, load_skill_script and the three-strikes stop (SKILL.md:208) pre-exist, and no prior codec exists for this format. My one real finding is the validate mode.

First-Principles-Verdict: CONCERNS

validate is a second spelling of enforcement encode and rotate already perform — a whole mode, and an approval prompt, for checks that cannot fire when the skill's own rules are followed.

What this change ships

Intent: stop the conductor's compaction-surviving ledger entry from being silently lost or corrupted, by giving its format one code owner — a FIX (two defects observed on PR #5652) delivered as an addition.

1. Conductor encodes ledger entries via a bundled codec, never hand-written JSON — justified
2. decode returns structured fields plus terminal/complete flags — justified
3. validate mode re-checks bounds encode/rotate already enforce — duplicate of its own siblings
4. rotate collapses/drops terminal entries, never active ones — justified (ledger age-out is status-blind, session_ledger.py:372-373)
5. Persisted fails counter — justified (pre-existing three-strikes stop, SKILL.md:208, was context-only)
6. Strict four-status vocabulary, synonyms rejected — justified
7. New required patrol step: pre-dispatch rotate of the combined map — justified, declared
8. One extra approval prompt per codec call per cycle — declared cost
9. Conductor system prompt widens the shell grant to both scripts — necessary rider
10. Evaluator test loader swapped to shared load_skill_script — rides along; is itself a deletion

Watch

• Counted validate's four violation codes: value_too_long duplicates encode's refusal, too_many_entries duplicates rotate's cap handling (which also fixes it), and key_too_long / value_not_string are unreachable when entries come from encode under item-<n> keys — its only job is catching disobedience of the "never hand-write" rule the same document imposes.
• The bounds mirror + drift-guard exists only because session_ledger._clamp (session_ledger.py:370) truncates silently; a loud rejection there would protect every artifacts writer, not just this one. Genuinely wider than this PR — accepted-and-deferred, not a demand.

Subtractions

• Drop mode_validate/validate_artifacts in ledger_entry.py and the SKILL.md "validate" bullet; let encode and rotate remain the two enforcement points, saving one approval-prompt category per cycle.

[FIRST-PRINCIPLES-REVIEWED] 16ec8b8

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound codec, but enforcement is still prose: nothing mechanical stops a drifting conductor from hand-writing entries, and the ledger's silent clamp remains untouched.

Watch

• The PR's own premise — "A format only enforced by prose will keep regressing" — applies one level up: the instruction to call ledger_entry.py is itself only SKILL.md prose. A conductor that skips the codec (post-compaction, or after instruction drift) hand-writes a value, and the ledger still silently truncates oversized JSON / ages out active entries — the exact loss class this PR targets. The drift surface is much smaller (invoke a named script vs. re-derive an escaped byte format), so this is acceptable, but the class is narrowed, not closed. A ledger-side loud failure (reject or log on truncation/age-out) is the follow-up that would close it for every writer.
• Bounds are mirrored constants, drift-guarded only by an in-repo test. A user's installed skill copy can skew from an upgraded backend ledger, at which point validate passes writes the ledger clamps — inherent to bundled-skill versioning, but this is the first script whose correctness depends on lockstep with a package-side constant.

Suggestions

• encode is per-entry while the skill's own cost discipline demands batching ("each invocation costs one approval prompt"); a dispatch round of N items costs N approval prompts. A batch-encode mode (list in → map out) would follow the evaluator's established one-call-per-cycle design.

[DESIGN-REVIEWED] 16ec8b8

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 16ec8b8465465f4e61b73f3da8349271856a8796 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 16ec8b8

Verdict parsed from the review's SHA-scoped output markers for commit 16ec8b8465465f4e61b73f3da8349271856a8796.

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

[NicholasRBowers]

• Rotation can still age out an active item (SKILL.md rotate trigger) — fixed in 16ec8b8

  A 32-entry map with an older active item plus a new dispatch would be capped by the LEDGER, whose age-out is status-blind, evicting the active item's durable state.

  The rotate contract now has two named trigger points: on a terminal verdict, and before any dispatch write that would push the map past the cap — run on the combined current-plus-new map (SKILL.md dispatch step 5 + the codec-section rotate bullet + the mode_rotate docstring). A cap_exceeded_all_active result at dispatch time means do-not-dispatch. Locked by test_pre_dispatch_rotation_of_combined_map_protects_active_items: 31 active + 1 terminal + 1 new dispatch → the terminal entry drops, all active items and the new item survive, and the result fits the cap so the ledger's own eviction never runs.

[NicholasRBowers]

Dispositions for Design Review CONCERNS (verdict on 16ec8b8):

• Enforcement is still prose; a ledger-side loud failure would close the class for every writer — needs-a-decision

  A conductor that skips the codec hand-writes a value, and the ledger still silently truncates / ages out.

  Agreed the class is narrowed, not closed — that is this PR's declared scope. The closing move you name lives in session_ledger.py and changes the clamp contract for EVERY artifacts writer: rejecting would break existing callers that rely on best-effort clamping; logging is safe but adds noise on a hot path. Which of those (reject / warn-log / keep silent) the ledger should do is a maintainer ruling on a shared contract, not something this skill-scoped PR should decide as a side effect — @NicholasRBowers, if you want the loud-clamp change, say which shape and it can be its own small PR.

• Bounds are mirrored constants; an installed skill copy can skew from an upgraded backend — rebutted (accepted constraint, no code change)

  The first script whose correctness depends on lockstep with a package-side constant.

  Inherent to bundled-skill versioning, as noted. Two mitigations already in the diff: the drift-guard test pins repo lockstep, and skew degrades gracefully — a stale-bounds codec at worst lets a write through that the ledger then clamps, which is exactly the pre-PR status quo, never worse.

• Suggestion: batch-encode mode — rebutted (proportionality; deferred until usage shows need)

  A dispatch round of N items costs N approval prompts.

  N is bounded by the skill's own concurrency discipline ("two or three items per round"), and per-item re-encodes at patrol time happen at different moments (cursor advances item-by-item), so a batch mode only helps the initial dispatch write — 1-2 prompts saved per round. Worth adding if real goals show prompt fatigue; not worth re-arming every reviewer on a clean head now.

[NicholasRBowers]

Dispositions for First Principles CONCERNS (verdict on 16ec8b8):

• validate duplicates enforcement encode and rotate already perform; drop it — rebutted (keeping the code)

  Its only job is catching disobedience of the "never hand-write" rule the same document imposes.

  That is precisely its job, and this round's Design Review names why it is reachable: the "never hand-write" rule is itself prose, so a map's provenance is not guaranteed — post-compaction recovery, instruction drift, or an entry written by an older skill copy all produce entries encode never authored. encode/rotate enforce at AUTHORING time on values they create; validate is the one diagnostic for a map the current session did not author, and the only mode that checks keys at all. On cost: it is opt-in ("run it before session_ledger_record when in doubt"), not a per-cycle step, so it adds no steady-state approval prompt. Removing it would delete the recovery path one lane in this same round argues is the residual risk.

• The bounds mirror exists only because session_ledger._clamp truncates silently; a loud rejection there would protect every writer — needs-a-decision (same question as the Design lane)

  Genuinely wider than this PR — accepted-and-deferred, not a demand.

  Agreed on scope. Whether the ledger's clamp contract becomes loud (reject vs warn-log vs unchanged) affects every artifacts writer and is put to the maintainer in the Design-lane disposition above rather than filed as an unowned issue.