fix(ledger): report the data a state read discards instead of losing it silently

[leonlaiyc]

Problem / Motivation

_coerce_state folds whatever is on disk into a well-typed ledger record. Six
of its rules throw caller data away, and none of them said so:

rule	what is discarded
_clamp(..., _MAX_TEXT)	any text past 2000 chars, on goal, phase, next, and the timestamps
_clamp inside tried[] / events[]	the same, per entry field
tried[-_MAX_TRIED:]	the oldest approaches past 50
events[-_MAX_EVENTS:]	the oldest progress lines past 100
len(arts) < _MAX_ARTIFACTS	artifacts past 32 — in insertion order, so which 32 survive is arbitrary from the writer's point of view

This is not a display-time trim. record() opens with
state = _read_state_unlocked(dir_path) and closes by writing the record back,
so the first read that overflows a cap persists the shortened version. The
data is then gone from disk permanently, and nothing anywhere says it existed.

The same function's neighbour is already loud about exactly this failure at a
coarser grain:

if path.stat().st_size > _MAX_STATE_BYTES:
    logger.warning("ledger state file over size ceiling; treating as absent")

So the module discards a whole file with a warning and discards half its
contents in silence.

This is the residual the reviewer named twice on merged #5970:

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.

…the ledger still silently truncates oversized JSON / ages out active
entries — the exact loss class this PR targets. A ledger-side loud failure
(reject or log on truncation/age-out) is the follow-up that would close it for
every writer.

Why it matters

The ledger is the conductor's memory of what it has already tried. The two
biggest caps are the two that hurt most when they fire quietly:

• tried ages out the oldest approaches. Those are precisely the ones the
  agent is most likely to repeat, because nothing in its context says it tried
  them. A silent cap turns "the ledger stops it looping" into "the ledger loops
  it back to the start".
• artifacts drops by insertion order once past 32, so the writer cannot even
  predict which ones it lost.

Because the write-back persists the trim, the loss is unrecoverable and leaves
no trace — no log line, no counter, nothing in the record itself. An operator
debugging a conductor that keeps re-trying a rejected approach has no way to
discover that its history was truncated three cycles ago.

Of the two remedies the reviewer offered, this takes the second. Rejecting
an oversized write would change acceptance semantics for every caller and needs
a product decision about what a writer should do when refused. Logging needs
none: it changes nothing about what is stored and makes the existing loss
diagnosable. The stored values are byte-identical before and after this diff,
and a test asserts that.

What changed (motivation → approach → change)

The root cause is that the caps were written as expressions (value[:limit],
items[-cap:], len(arts) < cap) whose discard is invisible at the point of
use. Making it visible means tallying at each site and reporting once:

• _clamp_tallied(value, limit, label, lost) sits beside _clamp, which
  stays pure for the write-side callers that do not participate in a read's
  tally. It records a truncation only when it actually shortened a string the
  caller supplied in full.
• The tried, events and artifacts caps record a count rather than one
  line per entry, so a record 40 artifacts over the limit produces one clause,
  not forty.
• _coerce_state emits a single WARNING at the end naming every bound it hit,
  widest loss first, tagged with the ledger directory name — which the module
  documents as a non-decodable fold, so it identifies the record without
  revealing the slot key.
• The artifacts loop is restructured so the wrong-type skip and the cap skip
  are separate branches. They were one condition, which is why the cap drop had
  nowhere to be counted.

Deliberately not counted: a wrong-typed field reset to its default. That is
the coercion contract this function documents ("known fields with the wrong type
are reset to their defaults"), and a newer writer's forward-compatible field
would otherwise produce a warning on every read for behaving exactly as
specified.

docs/system-specs/features/session-work-ledger.md §8 is updated in the same
commit per AGENTS.md, beside the existing over-ceiling entry it now parallels.

Tests

New TestCoercionIsLoudAboutLoss in test/test_session_ledger.py, driving
_coerce_state directly and reading caplog:

• test_an_oversized_field_is_named — the field is named, and the value is
  still trimmed to exactly _MAX_TEXT.
• test_aged_out_entries_are_counted — tried and events seven over their
  caps report x7 each, and the kept lengths are unchanged.
• test_dropped_artifacts_are_counted — five over the cap reports x5.
• test_one_line_per_read_not_one_per_field — three oversized events collapse
  to events[].text truncated x3 inside a single warning.

Three controls, which pass both before and after, because they are what
makes this safe rather than what proves the defect:

• test_a_clean_record_logs_nothing — the one that makes it shippable. Without
  it, a WARNING on every ledger read would be noise.
• test_a_wrong_typed_value_is_not_reported_as_loss — pins the deliberate
  exclusion above, and that the values still reset as documented.
• test_the_report_is_self_limiting_across_a_record_cycle — the answer to
  "won't this warn forever?". It seeds a real ledger, corrupts the file with an
  oversized goal, and asserts the next record() warns once and the one
  after that warns not at all, because the write-back removed the overflow.

Red-before, measured against pristine origin/main production code
(1fab46224) with the new tests in place — 5 failed / 46 passed / 1 skipped:

AssertionError: the discard was silent (warnings: [])      ×4
AssertionError: the overflow read must be reported once

The three controls pass on main, as controls should.

Green-after: 51 passed / 1 skipped in that module; 115 passed / 1 skipped
across test_session_ledger.py, test_conductor_ledger_entry.py and
test_capability_ledger.py.

flake8, isort, mypy and black are clean on both files, and
scripts/check_black_formatting.py / scripts/check_subprocess_encoding.py
both pass on the real origin/main...HEAD scope.

After rebasing over the merged system-spec audit (#6946), the loss-reporting contract is integrated into the current compact State record and bounded writes section rather than restoring the superseded proposal-era document. scripts/docs_lint.py --test, the baseline-aware Black gate, the subprocess-encoding gate, and git diff --check pass on the rebased 3-file scope.

Manual verification

N/A — unit coverage sufficient: the change is a log line derived from data the
function already computes, and the tests assert both the message and that the
coerced values are unchanged.

Related Issues

Residual named by the reviewer on merged #5970 (feat(conductor): one code owner for the ledger item entry (#5912)). No separate issue was filed. #4371 also
touches test/test_session_ledger.py, in a different area of the file — a
textual conflict is possible, a semantic one is not.

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

Contribution License Agreement

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 7745c925fe4fac1eab460547ee83b2650b836f1b 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 checks done. The base corroborates the cited residual (test_conductor_ledger_entry.py:9 — "the ledger CLAMPS rather than rejects" — with the bounds mirror + drift-guard at lines 55-66), the read-side warning parallels the existing _serialize_bounded write-budget warning rather than duplicating it, and the MCP route schema already loudly rejects most oversized writer fields — but not artifacts values, and record()'s own clamp sites stay silent. That drives the verdict.

First-Principles-Verdict: CONCERNS

The cited #5970 residual names the write-side _clamp ("protect every artifacts writer"); this instruments only read coercion, which in-tree writers' pre-clamped output never triggers.

What this change ships

Intent: make the ledger's read-time coercion report the caller data it permanently discards, instead of trimming and persisting it silently — a FIX (observability of an existing loss), with checkable provenance: the drift-guard at test/test_conductor_ledger_entry.py:9,55-66 exists "because the ledger CLAMPS rather than rejects".

Inventory (5 items)

1. A ledger read that overflows any text/tried/events/artifacts bound now logs one aggregated warning with per-bound counts — justified
2. The warning names the ledger directory read (new source arg, mirroring _serialize_bounded's) — justified
3. Wrong-typed fields still reset silently, excluded from the report — justified
4. Artifacts loop split into wrong-type skip vs cap skip; kept set byte-identical — justified
5. Spec paragraph beside the existing over-ceiling entry, same commit — justified

Watch

Point patch on the loss class it cites. record() still silently truncates and persists at 8 sites (session_ledger.py:372,374,377,382,383,392,399,409; grepped _clamp(), and FieldSpec("artifacts", dict) (validation.py:1139) leaves artifacts values route-unguarded — so the exact writer the quoted residual names still loses data with no line, and the drift-guard that "exists only because _clamp truncates silently" keeps its reason to exist. The new _clamp_tallied already fits those sites; the read-side coverage shipped here fires only on out-of-band/foreign files.
Clears when: record()'s truncation sites report through the same tally, or a linked issue records the write half as deferred.

[FIRST-PRINCIPLES-REVIEWED] 7745c92

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 7745c92

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

The base module confirms the PR's claims: _serialize_bounded already emits the parallel one-line write-side loss warning, and record() persists the coerced state. One design gap: record() itself trims tried/artifacts/events to their caps before every write (session_ledger.py:387,401,410), so on files written by this codebase the disk never holds over-cap data — meaning the new coercion warnings fire only on foreign, corrupt, or version-skewed files, while the motivating scenario (conductor's oldest tried approaches aging out silently) happens at record()'s own append trims and stays silent.

Design-Verdict: CONCERNS

The loud-about-loss report lands on the rare read path; the motivating loss — record() aging out the oldest tried — stays silent.

Watch

• The in-tree writer trims to every cap before writing (record() at session_ledger.py:387, :401, :410), so _coerce_state never sees overflow on its own files — the new warnings fire only on external/version-skewed data. The PR's headline harm ("an operator debugging a conductor that keeps re-trying a rejected approach has no way to discover its history was truncated") is produced by record()'s steady-state append trims, which this diff leaves unreported, so "close it for every writer" is not yet delivered.
  Clears when: record()'s own age-out sites (at minimum the low-frequency tried/artifacts trims) report loss the same way, or the PR description narrows its claim to foreign/skewed files.

Suggestions

• Tally tried/artifacts age-out inside record() — those fire only on explicit appends, so a one-line warning there is as low-noise as _serialize_bounded's and covers the actual conductor-loop harm; leave the per-cycle events tail trim silent.

[DESIGN-REVIEWED] 7745c92

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 7745c92

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• This PR is PARTIALLY_COVERED with PR #7631. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #6017: REBASE. PR #7631 implements the write-side half of the same goal and is already merged; it neither implements nor obsoletes the read-side coercion caps this PR reports. Its only effect on this PR is the test-file adjacency conflict it created. Files: src/kiro_crew/session_ledger.py, test/test_session_ledger.py, docs/system-specs/features/session-work-ledger.md.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[chenmingwei23]

Kiro Crew [operator: chenmingwei23]: This PR has been inactive for 7+ days with failing CI. I've assessed the blockers and they appear resolvable -- I'll push fixes directly to this branch as a co-author.

Assessment: Merge conflict with main. GPT 5.6 review passes clean; the other review lanes only 'could not complete' (transient model errors, not findings). The one substantive red is Gateway Tests (macOS), and the failing test (TestAdoptedDriftRecheck::test_the_recheck_is_rate_limited_to_its_own_interval) is unrelated to this ledger-coercion change and ran on a memory-starved runner -- a pre-existing flake, not your regression. Plan: rebase onto main, re-run the macOS lane, confirm it clears.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

[bolichen97]

Rebased onto main 8534cbf75 by a maintainer as part of the 2026-09-08 open-PR audit.

Conflicts and resolutions:

• test/test_session_ledger.py: both sides added a new "loud about loss" test class in the same spot. Kept both, in order: main's write-side TestBoundedWriteIsLoudAboutLoss (from fix(ledger): report the history a bounded write discards #7631), then your read-side TestCoercionIsLoudAboutLoss unchanged.
• docs/system-specs/features/session-work-ledger.md: docs: refresh prompts, skills and docs against the shipped code #8905 moved this spec to docs/system-specs/modules/session-work-ledger.md; git followed the rename and your paragraph landed there. I did not touch main's neighbouring sentence that calls a read-side clamp recoverable, which your change contradicts. Worth reconciling.
• src/kiro_crew/session_ledger.py: merged with no conflict.

Gates run locally on changed files: black, isort, flake8, and pytest on test_session_ledger.py, test_work_ledger.py, test_conductor_ledger_entry.py (227 passed).

Please review the resolution. The maintainer push makes the maintainer the last pusher, so under the repo's last-push rule a second approver is needed. Reply if anything looks wrong.