fix(ship): recover original published work after a lost checkpoint - #2794
Conversation
There was a problem hiding this comment.
Changes requested: Audited display histories consume the global ownership budget, blocking valid recovery and poisoning in-memory ownership scans.
Warning
Changes requested · head 3f41787 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — run-history.md canonical pull ownership: audit display bytes exhaust the owner budget | deploy/cloudflare-memory/worker.ts:4485 |
F1 invariant: Historical model prose, tool stdout and diagnostics must not consume the global ownership-scan budget. Standing audit attribution must remain bounded by canonical typed authority, while admission retains its complete per-run snapshot, digest, completeness checks and atomic rival lookup.
- SQLite adoption CAS with three prior audited producers and a fourth otherwise valid producer, each with approximately 1.4 MiB of complete events but only a small typed authority projection. → The fourth audit is not refused solely because irrelevant display events collectively exceed the 16 MiB ownership budget; full per-run audit checks and global rival admission still run.
- SQLite standing findPullOwners for either the audited repository or another repository after verbose audited histories have accumulated. → Only bounded authoritative evidence consumes ownership capacity; unrelated model prose and stdout cannot make a complete ownership lookup incomplete.
- SQLite ordinary unit, Main-task, recovery or effect admission invoking pullOwnershipRows alongside retained audited producers. → Valid admission is not blocked solely by historical display bytes, and real rival or incomplete authoritative evidence still refuses.
- SQLite claimed-to-posting and posting-to-bound audited CAS, including reconciliation after an uncertain PR-create response. → Canonical audit snapshot and digest remain unchanged and rechecked before the transaction's rival lookup without charging all other producers' display events to the global owner budget; uncertain create is not replayed.
- In-memory initial audited CAS near the ownership budget, where bindingRefusal substitutes the proposed audited unit only after pullOwnershipRows computes auditedRunIds. → The proposed state is accounted consistently before commit; a successful claim cannot make the next standing scan or posting CAS fail solely because its display events have newly entered the global budget.
- In-memory standing ownership lookup and subsequent ordinary binding or audited CAS writes with several verbose audited histories. → Use the same authoritative-byte accounting policy as SQLite; display-only history cannot poison every subsequent ownership scan.
- Either store encounters changed authoritative history, a missing event, a stale digest, a forged original spawn or an actual rival while using the bounded standing projection. → Refuse as before; excluding display bytes from global capacity must not weaken canonical evidence, full admission snapshot verification or rival ownership checks.
Full review
F1 — Major, high confidence. The new scan charges every audited producer’s complete event history—including model prose and tool stdout—to the global 16 MiB ownership budget. Four valid ~1.4 MiB histories consume ~16.8 MiB under the 3 * length accounting, even with tiny authority projections, so SQLite refuses the fourth audit. In-memory admission is worse: instanceStore.ts:352 counts only previously audited runs before substituting the proposed unit, allowing that claim to commit and making subsequent ownership lookups and posting CAS fail. This contradicts run-history.md:544, which explicitly excludes historical display bytes from ownership capacity. Separate complete per-run snapshot/digest verification from the transaction-local, bounded typed-authority scan in both stores; preserve completeness checks and rival refusal rather than increasing or removing the safety bound.
3f41787 to
2fe5b51
Compare
There was a problem hiding this comment.
LGTM: F1 is fixed; no major-or-higher defects remain in the current-head delta review.
Note
Approved · head 2fe5b51 · no findings
| Prior finding | Resolution | Evidence at this head |
|---|---|---|
| review:5422442802:F1 | fixed | Verified all seven prior cases at this head. deploy/cloudflare-memory/worker.ts historicalOwnerEvents/pullOwnershipRows derive compact typed evidence before global accounting while retaining raw per-run count, contiguous sequence and byte fences; preparation and initial/follow-on audited CAS use the complete canonical snapshot and full-event digest with an exact synchronous transaction recheck. The fourth verbose-history claim, standing lookups for same/other repositories, ordinary unit/Main-task/recovery/effect admission, and claimed-to-posting/posting-to-bound paths all consume this compact inventory instead of display bytes. src/core/coordinator/instanceStore.ts pullOwnershipRows substitutes the proposed audited unit before collecting audited run IDs and accounting, eliminating the initial-claim/standing-scan discrepancy; subsequent standing and ordinary/audited writes share compact accounting. historicalNativeAudit.ts preserves every causal tool field and validates malformed success flags; missing events, changed authoritative history, stale digests, forged spawn and real rivals still refuse. pullOwnership.ts preserves workspace owner attribution and custody guards, with main's diagnostics remaining observational. adminCoordinator.ts preserves the create-once posting state and exact uncertain-response reconciliation without reposting. The added verbose-history regression proofs in both adminCoordinator.test.ts and the actual SQLite runLedger.test.ts independently assert claims, standing reads, posting/binding, ordinary admission, changed authority and rival refusals. Source verified; tests/build and live adoption were not run during review. |
Full review
review:5422442802:F1 is resolved at 2fe5b51; no major-or-higher findings remain in the current-head delta.
|
Warning Polylane could not verify the production impact of this pull request. Checked the additive historical-native audit path ( Also considered · 2 refuted
Analysed against 7 cloud accounts and 1 repository
Polylane could not find the cloud resources this repository manages, so this review looked at the entire cloud account. Connect this repository to its resources and the next review will focus on exactly what this code deploys to. Polylane analysed Did this help? React 👍 or 👎 so the next review is sharper. |
An original unit can audit its native push history and open its draft PR when a restart lost publication evidence. The old settlement and private workspace remain unchanged. Ordinary adoption keeps its existing refusals.
Why: A restart can leave successful native pushes beside an unaccepted publication checkpoint. Remote branch equality cannot repair that evidence. Explicit original-owner audit checks canonical history, pinned commit identities and rival ownership before creating a new adoption receipt.
Where to look
Feedback wanted: Major delta review: close review:5422442802:F1; check compact authority, malformed fields, frozen digest/CAS, rivals and create-once reconciliation.
Risk: This adds an explicit recovery authority path. Missing, conflicting or incomplete evidence must refuse. No model run, budget renewal, old settlement rewrite or private workspace release is authorized.
Verified: 49 focused root tests and 2 real SQLite tests pass, plus scoped types, lint, format, consistency and test-loss guard. All 44 rendered captures retain their bytes. Live adoption remains unproved.
Decisions (4)
Validation (4 criteria)
For agents
Source proof only. Do not adopt a customer unit, replay a model run, fabricate an acknowledgement, alter historical caps or release a retained workspace during review. Current release and production acceptance are separate gates.
🤖 Generated with Claude Code