Skip to content

fix(apps): refuse to publish the dependency ledger over an unreadable read - #8734

Open
DeryFerd wants to merge 1 commit into
kirodotdev:mainfrom
DeryFerd:fix/dependency-ledger-refuse-unreadable-read
Open

fix(apps): refuse to publish the dependency ledger over an unreadable read#8734
DeryFerd wants to merge 1 commit into
kirodotdev:mainfrom
DeryFerd:fix/dependency-ledger-refuse-unreadable-read

Conversation

@DeryFerd

@DeryFerd DeryFerd commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

~/.kiro/crew/dependency-ledger.json is the only record that an installed dependency is still referenced by an app. All three of its mutations — record_install, record_uninstall, and classify_and_clean_for_uninstall — read the whole file, mutate it, and write the whole file back, and all three stood on the lenient display reader that answers {} on OSError/json.JSONDecodeError. A transient EACCES/EIO or a truncated document read as empty and was then published back: record_install over every other app's refcount, classify_and_clean_for_uninstall as an outright wipe (its write is unconditional).

Why it matters

An emptied ledger does not break anything immediately, which is what makes it dangerous: every future uninstall classifies its dependencies as untracked and skips cleanup, so packages pile up on the host with no trace of why. This is the same defect class upstream closed four times in two weeks (#7620, #7788, #8084, #7618), each found by hand, and #7789 records why that discovery keeps being manual. The dependency ledger is the surviving instance of the class in apps/ proper, on a file every install and uninstall touches.

What changed (motivation → approach → change)

The diff is six files; the contract they implement is "a mutation never publishes over a read it could not do":

  • src/kiro_crew/apps/dependency_ledger.py_read_ledger_for_update() is the mutation base: only a MISSING file reads as empty; OSError propagates raw, corruption and a non-object root raise plain json.JSONDecodeError, and UnicodeDecodeError is folded into the refusal (it is a ValueError but not a JSONDecodeError, so unwrapped it would slip past every corruption clause at the callers). The display reader stays lenient — and gains the object-root check it lacked, so a non-object ledger degrades to empty with a loud log instead of crashing every lookup with AttributeError. A zero creation-style implausible parse is refused rather than answered with a value that would compare equal across processes.
  • src/kiro_crew/apps/routes.py — the uninstall handler pre-flights the ledger before anything destructive and aborts with a retryable 409 (code: "dependency_ledger_unreadable"), the same shape as its trust-grant and cron preconditions, offloaded to the executor. A keep_dependencies: true purge never touches the ledger and skips the pre-flight. An unreadable ledger DURING teardown (the onUninstall script is arbitrary code) degrades instead: the uninstall finishes without ledger cleanup and its log says so — failing there would strand a half-uninstalled app whose retry refuses at the pre-flight forever.
  • src/kiro_crew/apps/dependencies.pyclean_dependencies() returns (cleaned, unrecorded): a dependency whose ledger record was refused is reported as unrecorded, never counted as cleaned, because its ledger row still names this app and a "Cleaned N" claim over it would hide a ghost owner no future uninstall can remove. resolve_dependencies reports a refused record in a separate DependencyResult.unrecorded bucket, never in failed — which renders as "Failed to install N dependency(ies)" for a dependency that IS on disk.
  • test/test_dependency_ledger.py, test/test_dependencies.py, test/test_apps_uninstall_keep_specific.py — the refusal and flow tests, including on-disk-bytes-unchanged assertions.

No frontend change: the dashboard consumes the classification dict and the uninstall response, whose shapes are unchanged.

Tests

Fourteen new tests across three files (ledger refusals with byte-unchanged assertions, the 409 pre-flight and its keep_dependencies skip, the mid-teardown degradation, and both flow-surfacing paths). The four touched suites run 96 passed (49 + 13 + 34) plus the routes/hook suites at 237; the pre-existing tests are unchanged. flake8, isort, mypy (scoped), the formatting gates, the docs lint, and the comment-history gate pass.

Manual verification

With a truncated dependency-ledger.json: an app uninstall aborts with a 409 naming the code, the file's bytes survive for hand recovery, and after repairing the file the same request succeeds. In a REPL this is a two-minute check; with a healthy ledger everything behaves exactly as before, which the unchanged pre-existing tests pin.

Related Issues

Closes this class's remaining apps/ instance; the class tracker is #7805, and the merged readers are #7620, #7788, #8084, #7618. session_ledger.py's instance is deliberately out of scope — #6017 is already reworking that module's read/write plumbing, and bundling the two would collide over one file.

Checklist

  • Test-first: the refusal tests failed before the reader existed and pass after
  • Spec updated in the same commit (app-kit-platform.md §10/§11 carry the precondition and the refusal rules)
  • Flows state their own refusal contracts; no renderer can claim a bookkeeping state that does not exist

Pattern harvest

Rule candidate: review-prompt — a read whose result is the base of a whole-file rewrite must not share the display reader's leniency; only a missing file makes "empty is the truth" true, and UnicodeDecodeError must be folded into the corruption refusal because it is a ValueError but not a json.JSONDecodeError.
Rule candidate: testing conventions — a data-loss refusal test must assert the on-disk bytes are unchanged, not only that the call raised; an exception can still leave a partial write behind.
Rule candidate: review-prompt — when a flow performs an irreversible action and then records it, a refused record must surface in a bucket of its own (never merged into the success or failure lists), so no renderer can claim a bookkeeping state that does not exist.
Not generalizable: record_uninstall's silent early-return stays for a genuinely absent entry — that is "no such reference", not a failed read; the refusal is keyed to the read failing, never to the mutation being a no-op.## Problem / Motivation

~/.kiro/crew/dependency-ledger.json is the only record that an installed dependency is still referenced by an app. All three of its mutations — record_install, record_uninstall, and classify_and_clean_for_uninstall — read the whole file, mutate it, and write the whole file back, and all three stood on the lenient display reader that answers {} on OSError/json.JSONDecodeError. A transient EACCES/EIO or a truncated document read as empty and was then published back: record_install over every other app's refcount, classify_and_clean_for_uninstall as an outright wipe (its write is unconditional).

Why it matters

An emptied ledger does not break anything immediately, which is what makes it dangerous: every future uninstall classifies its dependencies as untracked and skips cleanup, so packages pile up on the host with no trace of why. This is the same defect class upstream closed four times in two weeks (#7620, #7788, #8084, #7618), each found by hand, and #7789 records why that discovery keeps being manual. The dependency ledger is the surviving instance of the class in apps/ proper, on a file every install and uninstall touches.

What changed (motivation → approach → change)

The diff is six files; the contract they implement is "a mutation never publishes over a read it could not do":

  • src/kiro_crew/apps/dependency_ledger.py_read_ledger_for_update() is the mutation base: only a MISSING file reads as empty; OSError propagates raw, corruption and a non-object root raise plain json.JSONDecodeError, and UnicodeDecodeError is folded into the refusal (it is a ValueError but not a JSONDecodeError, so unwrapped it would slip past every corruption clause at the callers). The display reader stays lenient — and gains the object-root check it lacked, so a non-object ledger degrades to empty with a loud log instead of crashing every lookup with AttributeError. A zero creation-style implausible parse is refused rather than answered with a value that would compare equal across processes.
  • src/kiro_crew/apps/routes.py — the uninstall handler pre-flights the ledger before anything destructive and aborts with a retryable 409 (code: "dependency_ledger_unreadable"), the same shape as its trust-grant and cron preconditions, offloaded to the executor. A keep_dependencies: true purge never touches the ledger and skips the pre-flight. An unreadable ledger DURING teardown (the onUninstall script is arbitrary code) degrades instead: the uninstall finishes without ledger cleanup and its log says so — failing there would strand a half-uninstalled app whose retry refuses at the pre-flight forever.
  • src/kiro_crew/apps/dependencies.pyclean_dependencies() returns (cleaned, unrecorded): a dependency whose ledger record was refused is reported as unrecorded, never counted as cleaned, because its ledger row still names this app and a "Cleaned N" claim over it would hide a ghost owner no future uninstall can remove. resolve_dependencies reports a refused record in a separate DependencyResult.unrecorded bucket, never in failed — which renders as "Failed to install N dependency(ies)" for a dependency that IS on disk.
  • test/test_dependency_ledger.py, test/test_dependencies.py, test/test_apps_uninstall_keep_specific.py — the refusal and flow tests, including on-disk-bytes-unchanged assertions.

No frontend change: the dashboard consumes the classification dict and the uninstall response, whose shapes are unchanged.

Tests

Fourteen new tests across three files (ledger refusals with byte-unchanged assertions, the 409 pre-flight and its keep_dependencies skip, the mid-teardown degradation, and both flow-surfacing paths). The four touched suites run 96 passed (49 + 13 + 34) plus the routes/hook suites at 237; the pre-existing tests are unchanged. flake8, isort, mypy (scoped), the formatting gates, the docs lint, and the comment-history gate pass.

Manual verification

With a truncated dependency-ledger.json: an app uninstall aborts with a 409 naming the code, the file's bytes survive for hand recovery, and after repairing the file the same request succeeds. In a REPL this is a two-minute check; with a healthy ledger everything behaves exactly as before, which the unchanged pre-existing tests pin.

Related Issues

Closes this class's remaining apps/ instance; the class tracker is #7805, and the merged readers are #7620, #7788, #8084, #7618. session_ledger.py's instance is deliberately out of scope — #6017 is already reworking that module's read/write plumbing, and bundling the two would collide over one file.

Checklist

  • Test-first: the refusal tests failed before the reader existed and pass after
  • Spec updated in the same commit (app-kit-platform.md §10/§11 carry the precondition and the refusal rules)
  • Flows state their own refusal contracts; no renderer can claim a bookkeeping state that does not exist

Pattern harvest

Rule candidate: review-prompt — a read whose result is the base of a whole-file rewrite must not share the display reader's leniency; only a missing file makes "empty is the truth" true, and UnicodeDecodeError must be folded into the corruption refusal because it is a ValueError but not a json.JSONDecodeError.
Rule candidate: testing conventions — a data-loss refusal test must assert the on-disk bytes are unchanged, not only that the call raised; an exception can still leave a partial write behind.
Rule candidate: review-prompt — when a flow performs an irreversible action and then records it, a refused record must surface in a bucket of its own (never merged into the success or failure lists), so no renderer can claim a bookkeeping state that does not exist.
Not generalizable: record_uninstall's silent early-return stays for a genuinely absent entry — that is "no such reference", not a failed read; the refusal is keyed to the read failing, never to the mutation being a no-op.## Problem / Motivation

~/.kiro/crew/dependency-ledger.json is the only record that an installed dependency is still referenced by an app. All three of its mutations — record_install, record_uninstall, and classify_and_clean_for_uninstall — read the whole file, mutate it, and write the whole file back, and all three stood on the lenient display reader that answers {} on OSError/json.JSONDecodeError. A transient EACCES/EIO or a truncated document read as empty and was then published back: record_install over every other app's refcount, classify_and_clean_for_uninstall as an outright wipe (its write is unconditional).

Why it matters

An emptied ledger does not break anything immediately, which is what makes it dangerous: every future uninstall classifies its dependencies as untracked and skips cleanup, so packages pile up on the host with no trace of why. This is the same defect class upstream closed four times in two weeks (#7620, #7788, #8084, #7618), each found by hand, and #7789 records why that discovery keeps being manual. The dependency ledger is the surviving instance of the class in apps/ proper, on a file every install and uninstall touches.

What changed (motivation → approach → change)

The diff is six files; the contract they implement is "a mutation never publishes over a read it could not do":

  • src/kiro_crew/apps/dependency_ledger.py_read_ledger_for_update() is the mutation base: only a MISSING file reads as empty; OSError propagates raw, corruption and a non-object root raise plain json.JSONDecodeError, and UnicodeDecodeError is folded into the refusal (it is a ValueError but not a JSONDecodeError, so unwrapped it would slip past every corruption clause at the callers). The display reader stays lenient — and gains the object-root check it lacked, so a non-object ledger degrades to empty with a loud log instead of crashing every lookup with AttributeError. A zero creation-style implausible parse is refused rather than answered with a value that would compare equal across processes.
  • src/kiro_crew/apps/routes.py — the uninstall handler pre-flights the ledger before anything destructive and aborts with a retryable 409 (code: "dependency_ledger_unreadable"), the same shape as its trust-grant and cron preconditions, offloaded to the executor. A keep_dependencies: true purge never touches the ledger and skips the pre-flight. An unreadable ledger DURING teardown (the onUninstall script is arbitrary code) degrades instead: the uninstall finishes without ledger cleanup and its log says so — failing there would strand a half-uninstalled app whose retry refuses at the pre-flight forever.
  • src/kiro_crew/apps/dependencies.pyclean_dependencies() returns (cleaned, unrecorded): a dependency whose ledger record was refused is reported as unrecorded, never counted as cleaned, because its ledger row still names this app and a "Cleaned N" claim over it would hide a ghost owner no future uninstall can remove. resolve_dependencies reports a refused record in a separate DependencyResult.unrecorded bucket, never in failed — which renders as "Failed to install N dependency(ies)" for a dependency that IS on disk.
  • test/test_dependency_ledger.py, test/test_dependencies.py, test/test_apps_uninstall_keep_specific.py — the refusal and flow tests, including on-disk-bytes-unchanged assertions.

No frontend change: the dashboard consumes the classification dict and the uninstall response, whose shapes are unchanged.

Tests

Fourteen new tests across three files (ledger refusals with byte-unchanged assertions, the 409 pre-flight and its keep_dependencies skip, the mid-teardown degradation, and both flow-surfacing paths). The four touched suites run 96 passed (49 + 13 + 34) plus the routes/hook suites at 237; the pre-existing tests are unchanged. flake8, isort, mypy (scoped), the formatting gates, the docs lint, and the comment-history gate pass.

Manual verification

With a truncated dependency-ledger.json: an app uninstall aborts with a 409 naming the code, the file's bytes survive for hand recovery, and after repairing the file the same request succeeds. In a REPL this is a two-minute check; with a healthy ledger everything behaves exactly as before, which the unchanged pre-existing tests pin.

Related Issues

Closes this class's remaining apps/ instance; the class tracker is #7805, and the merged readers are #7620, #7788, #8084, #7618. session_ledger.py's instance is deliberately out of scope — #6017 is already reworking that module's read/write plumbing, and bundling the two would collide over one file.

Checklist

  • Test-first: the refusal tests failed before the reader existed and pass after
  • Spec updated in the same commit (app-kit-platform.md §10/§11 carry the precondition and the refusal rules)
  • Flows state their own refusal contracts; no renderer can claim a bookkeeping state that does not exist

Pattern harvest

Rule candidate: review-prompt — a read whose result is the base of a whole-file rewrite must not share the display reader's leniency; only a missing file makes "empty is the truth" true, and UnicodeDecodeError must be folded into the corruption refusal because it is a ValueError but not a json.JSONDecodeError.
Rule candidate: testing conventions — a data-loss refusal test must assert the on-disk bytes are unchanged, not only that the call raised; an exception can still leave a partial write behind.
Rule candidate: review-prompt — when a flow performs an irreversible action and then records it, a refused record must surface in a bucket of its own (never merged into the success or failure lists), so no renderer can claim a bookkeeping state that does not exist.
Not generalizable: record_uninstall's silent early-return stays for a genuinely absent entry — that is "no such reference", not a failed read; the refusal is keyed to the read failing, never to the mutation being a no-op.

@DeryFerd
DeryFerd requested a review from a team as a code owner September 5, 2026 11:08
@DeryFerd
DeryFerd requested a review from cixuuz September 5, 2026 11:08
@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) — 🔴 changes requested (blocking)

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

2 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/apps/routes.py:1372 -- Ledger write failures leave permanent ghost owners
except (OSError, json.JSONDecodeError) as exc:
ENOSPC during ledger update -> exception is swallowed -> app files are removed while the unchanged ledger retains its ownership rows.
Anchor: residual/crash-data-loss-corruption
Fix: Do not finalize removal after a ledger write failure; durably preserve or queue the ownership repair first.

BLOCKING -- src/kiro_crew/apps/dependencies.py:220 -- Ledger failures produce successful but permanently untracked installs
except (OSError, json.JSONDecodeError) as exc:
Successful dependency install -> ledger write fails -> callers report success and finish installation, while future uninstall classifies the dependency as user-installed and skips it.
Anchor: residual/crash-data-loss-corruption
Fix: Fail or roll back the dependency installation when its ownership cannot be persisted.

[BLOCK-MERGE] d3eaa94
[GPT-REVIEWED] d3eaa94

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

Both findings are fenced (annotate-only); the adjudicable block is empty. I've read the diff and the base ledger code.

F1 — routes.py:1372 (mid-teardown ledger failure). Harm rung: MEDIUM/recoverable, not unbounded. Conditions: not keep_dependencies (patch routes.py:380), pre-flight read succeeded so ledger was healthy (patch:381–384), then classify_and_clean_for_uninstall raises during teardown (patch:433–439). On that path clean_dependencies is skipped, so the dependency is neither uninstalled nor re-recorded — the ledger keeps its exact bytes via atomic_write (dependency_ledger.py:212). The residual is a stale ownership row → a leaked (not removed-while-in-use) package, loudly logged and appended to uninstall_log (patch:449–458), reconciled by a manual repair or a reinstall+uninstall cycle. The finding's proposed fix (refuse after teardown) strands a half-uninstalled app whose retry refuses forever — the exact outcome the PR's documented design avoids. Ledger bytes are never lost or corrupted, and the consequence is visible and recoverable.

F2 — dependencies.py:220 (install ledger-write failure). Harm rung: MEDIUM/recoverable, not unbounded. Conditions: dep installs ok then record_install raises OSError/JSONDecodeError (patch dependencies.py:102–121). The dep is placed in both installed and unrecorded (never failed), logged as a warning; record_install uses atomic_write, so the prior ledger survives a failed write. Consequence: an installed-but-untracked package that a future uninstall skips as user-installed — a leak, surfaced in the unrecorded result field and reconciled idempotently by re-running install after repair. The finding's fix (fail/roll back a successful install on a transient EACCES) contradicts the install contract and adds its own failure modes. Visible, recoverable, no data loss.

Both describe the PR's deliberate, documented safe-direction behavior; the residual (a leaked package) is exactly what a human would accept, so a pre-drafted override rationale is warranted. Neither FLAG unblocks the merge.

[ADJUDICATION] d3eaa944ebea3bc1ab92710cc8ca824df978b69f total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] d3eaa944ebea3bc1ab92710cc8ca824df978b69f
[ADJUDICATION-FENCED] d3eaa944ebea3bc1ab92710cc8ca824df978b69f fenced=2 flagged=2
FLAG F1 src/kiro_crew/apps/routes.py:1372 -- Ledger uses atomic_write so its bytes survive intact; the residual is a stale ownership row → a leaked (never removed-while-in-use) package, loudly logged and recoverable, and the proposed refuse-mid-teardown fix would instead strand a half-uninstalled app whose retry refuses forever.
FLAG F2 src/kiro_crew/apps/dependencies.py:220 -- The install succeeds and is reported in `unrecorded` (not `failed`) with a warning; the ledger file is never corrupted (atomic_write), the outcome is a visible, recoverable leaked package re-recorded idempotently on retry, and rolling back a successful install on a transient EACCES contradicts the install contract.
[GPT-ADJUDICATED-FENCED] d3eaa944ebea3bc1ab92710cc8ca824df978b69f

🏷️ Fenced finding(s) machine-flagged as likely edge case

The security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)

  • F1 src/kiro_crew/apps/routes.py:1372 — Ledger uses atomic_write so its bytes survive intact; the residual is a stale ownership row → a leaked (never removed-while-in-use) package, loudly logged and recoverable, and the proposed refuse-mid-teardown fix would instead strand a half-uninstalled app whose retry refuses forever.
  • F2 src/kiro_crew/apps/dependencies.py:220 — The install succeeds and is reported in unrecorded (not failed) with a warning; the ledger file is never corrupted (atomic_write), the outcome is a visible, recoverable leaked package re-recorded idempotently on retry, and rolling back a successful install on a transient EACCES contradicts the install contract.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of d3eaa944ebea3bc1ab92710cc8ca824df978b69f 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 named, recurring data-loss class fixed at its root — the mutation base — with the pre-flight/degrade split correctly reasoned per flow.

The strict reader targets the actual cause (a display-lenient read as the base of a whole-file rewrite), not a symptom; the uninstall pre-flight vs. mid-teardown degradation asymmetry is the right shape (a refusal is only cheap before the irreversible steps, and the pre-flight's TOCTOU window is closed by the re-read under the lock); the API change is additive (unrecorded key, tuple return with all callers updated); spec updated in the same commit; a corrupt ledger has an in-band remedy (retryable 409 with repair instructions, keep_dependencies bypass).

Suggestions

  • The git-install path (registry.py:7066-7073) appends installed/failed/missing to log_lines but never unrecorded — the one flow where the new bucket stays invisible to the user; one line there completes the PR's own "flows state their refusal" contract.

[DESIGN-REVIEWED] d3eaa94

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🔴 BLOCK

Premise-level review of d3eaa944ebea3bc1ab92710cc8ca824df978b69f 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 evidence gathered. The strict reader, pre-flight, mid-teardown degradation, and uninstall-log surfacing all check out against the base tree; the one item that doesn't is the install-side unrecorded bucket, whose two real result consumers (routes.py:1490-1496, registry.py:7066-7073) were left untouched and whose to_dict serializer has zero callers.

First-Principles-Verdict: BLOCK

DependencyResult.unrecorded ships for a renderer that never receives it — zero consumers, and the publish-over fix is already complete without it.

Not justified as shipped

  • Item 7 (DependencyResult.unrecorded + its to_dict key) — zero counted consumers; both real consumers of the result enumerate installed/failed/missing only and neither is updated, so the stated "installed, unrecorded" rendering never happens.

What this change ships

Inventory (9 items) — 8 justified

Intent: stop a failed read of dependency-ledger.json from being written back as an empty ledger, silently erasing every app's dependency refcounts. FIX.

  1. A mutation on a corrupt/unreadable ledger now refuses instead of silently emptying it — justified
  2. Uninstall aborts up front with a retryable 409 (dependency_ledger_unreadable) before anything destructive — justified
  3. keep_dependencies: true uninstall skips that pre-flight — justified
  4. A ledger that goes bad mid-teardown degrades: uninstall finishes, cleanup skipped, log says so — justified
  5. Uninstall log reports uninstalled-but-unrecorded deps separately from "Cleaned N" — justified
  6. A refused install record logs a repair warning instead of counting the dep as failed — justified
  7. New DependencyResult.unrecorded field + to_dict "unrecorded" key — rides along, zero consumers
  8. Display reads of a non-object-root ledger degrade to empty with a log instead of crashing — rides along (declared; removes a real AttributeError)
  9. Spec §10/§11 updated in the same commit — justified (AGENTS.md mandate)

Blockers

  • Delete DependencyResult.unrecorded and its to_dict key. The description says "a renderer must be able to say 'installed, unrecorded' instead of 'failed to install'" — but grep DependencyResult yields 3 hits, all in dependencies.py (definition, annotation, constructor); to_dict() has 0 callers, and the only two resolve_dependencies consumers (routes.py:1490-1496 builds dep_info from installed/failed/missing manually; registry.py:7066-7073 reads the same three) never read it. The fix — strict reader, pre-flight, and the except-branch that keeps a refused record out of failed with a warning — removes the reported defect on its own; the field's zero option costs nobody anything. Keep the try/except and the warning; drop the field, the append, and the dict key. (The clean_dependencies tuple return is different: its unrecorded list has 1 counted consumer, the uninstall log at routes.py, and stays.)
    Clears when: the field is deleted, or a real consumer surfaces it (e.g. routes.py dep_info or registry.py log_lines).

Subtractions

  • Drop unrecorded from DependencyResult and its to_dict (0 consumers; greps above) — the except-branch and warning already carry the behavior.

[FIRST-PRINCIPLES-REVIEWED] d3eaa94

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

Both candidates fail the survival bar. Candidate 1 is a stale Returns: docstring block — documentation only, no observable runtime outcome (fails (c)), and in a category the pipeline owns. Candidate 2 is explicitly conceded by its own author to produce a correct result ("not wrong, just less informative") — the dep still appears under installed, so there is no observable wrong outcome (fails (c)).

I also checked the untouched test test_apps_uninstall_hook_registries.py:129, which still mocks clean_dependencies returning a bare [] (not the new tuple): its classify_and_clean_for_uninstall returns empty removable, so clean_dependencies is never called and the cleaned_deps, unrecorded_deps = ... unpack never runs — no breakage. No other production caller of clean_dependencies exists.

No findings.

[OPUS-REVIEWED] d3eaa94

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 5, 2026
@DeryFerd
DeryFerd force-pushed the fix/dependency-ledger-refuse-unreadable-read branch from 9d1f450 to e5e6d7a Compare September 5, 2026 12:17
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@DeryFerd
DeryFerd force-pushed the fix/dependency-ledger-refuse-unreadable-read branch from e5e6d7a to cd7f53f Compare September 5, 2026 13:08
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@DeryFerd
DeryFerd force-pushed the fix/dependency-ledger-refuse-unreadable-read branch from cd7f53f to ab2afda Compare September 5, 2026 14:00
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@DeryFerd

DeryFerd commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

On the remaining finding: the ENOSPC-on-write scenario it describes predates this PR — before it, the same write failure stranded the uninstall mid-flow (the exact shape the previous round asked to eliminate), so this change moved the failure mode rather than introducing one. Closing the loop entirely means a resumable uninstall (persist intent, delete files only after the ledger transition lands), which is uninstall-flow transactionality — a design change well beyond a ledger bug fix, and one that would apply to the merged readers in #7618/#8084 too. I'd propose tracking that as its own issue; this PR's scope stays "never publish over an unreadable read", which is now enforced at the mutation base, the pre-flight, and both flow boundaries.

@DeryFerd
DeryFerd force-pushed the fix/dependency-ledger-refuse-unreadable-read branch from ab2afda to cb72a6f Compare September 8, 2026 13:31
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 8, 2026
@github-actions github-actions Bot added 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 labels Sep 8, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) September 9, 2026 07:25
@bolichen97
bolichen97 disabled auto-merge September 9, 2026 07:59

@bolichen97 bolichen97 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.

Requesting changes at cb72a6f — the strict reader is right, but the PR ships more than it says and one stated guarantee is inverted.

  1. "The diff is two files" — it is six. src/kiro_crew/apps/routes.py +96/-12 adds _ledger_read_failure (~L1037), a new pre-flight returning status=500 / code="dependency_ledger_unreadable" before teardown (~L1181-1211) and a mid-teardown catch (~L1370-1392); src/kiro_crew/apps/dependencies.py +36/-3; plus two extra test files. That is undisclosed user-visible API behavior (a new error code, a new refusing precondition).
  2. record_uninstall's refusal is not "kept" — it is swallowed. Body: "a retriable uninstall error beats a ledger that never notices". Diff: dependencies.py ~L320-336 catches the refusal, logs, and still cleaned.append(dep_id), so the uninstall completes and routes.py logs "Cleaned N dependency(ies)" over an unrecorded ledger — leaving a row naming a now-removed app; a later app installing the same dep classifies it shared with a ghost owner and it is never cleaned. Pick one behavior and make body, code and tests agree.
  3. Spec not updated. docs/system-specs/modules/app-kit-platform.md §10 (~L643-679, the precondition chain) and the §11 table gain a new refusing precondition and a skip branch here; AGENTS.md requires the same-PR update.
  4. Status/shape mismatch. The pre-flight refusal is 500 with no retryable field, while the sibling precondition refusals in the same handler (trust grant, cron) use 409 + retryable: True; the dashboard shows a "server error" for a clean, nothing-changed refusal. Also result.installed and result.failed both contain the dep on ledger refusal (dependencies.py ~L219), which registry.py renders as "Failed to install N dependency(ies)" for a dependency that IS installed.

Good: dependency_ledger.py ~L226-278 strict reader, all three call sites swapped, refusal tests fail when a swap is reverted, 96/96 pass. Body test count (7) and "Manual verification" narrative describe an earlier revision — head adds 12 tests and the mid-teardown path reports success with a log line, not an error.

… read

All three mutations rewrite the whole ledger from what they read, and
they stood on the lenient display reader: a transient EACCES/EIO or a
truncated document read as empty and was then published back --
record_install over every other app's refcount, and
classify_and_clean_for_uninstall as an outright wipe of the only
record that an installed dependency is still referenced. The sidecar
lock serializes writers and says nothing about a read that failed.

_read_ledger_for_update is the mutation base: only a MISSING file
reads as empty; an unreadable or corrupt document (and a root that
parses but is not an object) propagates and the mutation is abandoned.
UnicodeDecodeError is folded into the refusal: it is a ValueError but
not a json.JSONDecodeError, so unwrapped it slips past corruption
clauses. The display reader stays lenient -- and gains the object-root
check it lacked, so a non-object ledger degrades to empty with a loud
log instead of crashing every lookup with AttributeError.

Every flow states its own refusal contract. The uninstall handler
pre-flights the ledger before anything destructive and aborts with a
retryable 409 (dependency_ledger_unreadable), the same shape as its
sibling preconditions; a keep_dependencies purge never touches the
ledger and skips the pre-flight. An unreadable ledger DURING teardown
(the script is arbitrary code) degrades instead: the uninstall finishes
without ledger cleanup and its log says so. Dependency cleanup splits
its result: a dep whose record was refused is reported as unrecorded,
never counted as cleaned -- its ledger row still names this app, and a
"Cleaned N" claim over it would hide a ghost owner no future uninstall
can remove. On the install side the refused dep lands in a separate
unrecorded bucket, never in `failed`, which renders as "Failed to
install" for a dependency that is on disk.

Mirrors the merged corrupt-read refusal readers (kirodotdev#7805 class); the
app-kit-platform spec carries the precondition and the refusal rules.
@DeryFerd
DeryFerd force-pushed the fix/dependency-ledger-refuse-unreadable-read branch from cb72a6f to d3eaa94 Compare September 9, 2026 22:20
@DeryFerd

DeryFerd commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@bolichen97 All four points addressed and pushed as d3eaa944e:

  1. The body is rewritten — six files, the full refusal contract (pre-flight precondition, the 409 code, the mid-teardown degradation, the tuple split), and the current test counts.
  2. Picked report-not-folded: clean_dependencies returns (cleaned, unrecorded), a refused record is never counted as cleaned, and the uninstall log carries its own line for the unrecorded keys — no more "Cleaned N" over rows that still name the removed app.
  3. app-kit-platform.md §10 carries the ledger pre-flight as precondition 1 (the chain renumbered) plus the mid-teardown degrade branch; §11 carries the refusal rules.
  4. The pre-flight refusal is now 409 with retryable: true, matching the sibling preconditions. The installed∩failed overlap is gone: a refused install lands in a new DependencyResult.unrecorded bucket, so the registry summary says "Installed N" and never "Failed to install N" for a dependency that is on disk.

The stale "Manual verification" narrative and test counts went with the body rewrite.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 9, 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) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants