fix(apps): refuse to publish the dependency ledger over an unreadable read - #8734
fix(apps): refuse to publish the dependency ledger over an unreadable read#8734DeryFerd wants to merge 1 commit into
Conversation
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed 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 BLOCKING -- src/kiro_crew/apps/dependencies.py:220 -- Ledger failures produce successful but permanently untracked installs [BLOCK-MERGE] 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: F2 — dependencies.py:220 (install ledger-write failure). Harm rung: MEDIUM/recoverable, not unbounded. Conditions: dep installs 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. 🏷️ Fenced finding(s) machine-flagged as likely edge caseThe 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.)
|
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of 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 ( Suggestions
[DESIGN-REVIEWED] d3eaa94 |
First Principles Review (Fable 5, fork) — 🔴 BLOCKPremise-level review of 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 First-Principles-Verdict: BLOCK
Not justified as shipped
What this change shipsInventory (9 items) — 8 justifiedIntent: stop a failed read of
Blockers
Subtractions
[FIRST-PRINCIPLES-REVIEWED] d3eaa94 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsBoth candidates fail the survival bar. Candidate 1 is a stale I also checked the untouched test No findings. [OPUS-REVIEWED] d3eaa94 |
9d1f450 to
e5e6d7a
Compare
e5e6d7a to
cd7f53f
Compare
cd7f53f to
ab2afda
Compare
|
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. |
ab2afda to
cb72a6f
Compare
bolichen97
left a comment
There was a problem hiding this comment.
Requesting changes at cb72a6f — the strict reader is right, but the PR ships more than it says and one stated guarantee is inverted.
- "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 returningstatus=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). 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 stillcleaned.append(dep_id), so the uninstall completes androutes.pylogs "Cleaned N dependency(ies)" over an unrecorded ledger — leaving a row naming a now-removed app; a later app installing the same dep classifies itsharedwith a ghost owner and it is never cleaned. Pick one behavior and make body, code and tests agree.- 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. - Status/shape mismatch. The pre-flight refusal is
500with noretryablefield, while the sibling precondition refusals in the same handler (trust grant, cron) use409+retryable: True; the dashboard shows a "server error" for a clean, nothing-changed refusal. Alsoresult.installedandresult.failedboth contain the dep on ledger refusal (dependencies.py~L219), whichregistry.pyrenders 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.
cb72a6f to
d3eaa94
Compare
|
@bolichen97 All four points addressed and pushed as
The stale "Manual verification" narrative and test counts went with the body rewrite. |
Problem / Motivation
~/.kiro/crew/dependency-ledger.jsonis the only record that an installed dependency is still referenced by an app. All three of its mutations —record_install,record_uninstall, andclassify_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{}onOSError/json.JSONDecodeError. A transientEACCES/EIOor a truncated document read as empty and was then published back:record_installover every other app's refcount,classify_and_clean_for_uninstallas 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;OSErrorpropagates raw, corruption and a non-object root raise plainjson.JSONDecodeError, andUnicodeDecodeErroris folded into the refusal (it is aValueErrorbut not aJSONDecodeError, 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 withAttributeError. 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 retryable409(code: "dependency_ledger_unreadable"), the same shape as its trust-grant and cron preconditions, offloaded to the executor. Akeep_dependencies: truepurge never touches the ledger and skips the pre-flight. An unreadable ledger DURING teardown (theonUninstallscript 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.py—clean_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_dependenciesreports a refused record in a separateDependencyResult.unrecordedbucket, never infailed— 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_dependenciesskip, 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
app-kit-platform.md§10/§11 carry the precondition and the refusal rules)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
UnicodeDecodeErrormust be folded into the corruption refusal because it is aValueErrorbut not ajson.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.jsonis the only record that an installed dependency is still referenced by an app. All three of its mutations —record_install,record_uninstall, andclassify_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{}onOSError/json.JSONDecodeError. A transientEACCES/EIOor a truncated document read as empty and was then published back:record_installover every other app's refcount,classify_and_clean_for_uninstallas 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;OSErrorpropagates raw, corruption and a non-object root raise plainjson.JSONDecodeError, andUnicodeDecodeErroris folded into the refusal (it is aValueErrorbut not aJSONDecodeError, 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 withAttributeError. 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 retryable409(code: "dependency_ledger_unreadable"), the same shape as its trust-grant and cron preconditions, offloaded to the executor. Akeep_dependencies: truepurge never touches the ledger and skips the pre-flight. An unreadable ledger DURING teardown (theonUninstallscript 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.py—clean_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_dependenciesreports a refused record in a separateDependencyResult.unrecordedbucket, never infailed— 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_dependenciesskip, 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
app-kit-platform.md§10/§11 carry the precondition and the refusal rules)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
UnicodeDecodeErrormust be folded into the corruption refusal because it is aValueErrorbut not ajson.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.jsonis the only record that an installed dependency is still referenced by an app. All three of its mutations —record_install,record_uninstall, andclassify_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{}onOSError/json.JSONDecodeError. A transientEACCES/EIOor a truncated document read as empty and was then published back:record_installover every other app's refcount,classify_and_clean_for_uninstallas 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;OSErrorpropagates raw, corruption and a non-object root raise plainjson.JSONDecodeError, andUnicodeDecodeErroris folded into the refusal (it is aValueErrorbut not aJSONDecodeError, 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 withAttributeError. 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 retryable409(code: "dependency_ledger_unreadable"), the same shape as its trust-grant and cron preconditions, offloaded to the executor. Akeep_dependencies: truepurge never touches the ledger and skips the pre-flight. An unreadable ledger DURING teardown (theonUninstallscript 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.py—clean_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_dependenciesreports a refused record in a separateDependencyResult.unrecordedbucket, never infailed— 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_dependenciesskip, 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
app-kit-platform.md§10/§11 carry the precondition and the refusal rules)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
UnicodeDecodeErrormust be folded into the corruption refusal because it is aValueErrorbut not ajson.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.