feat(panel): record why cleanup happened and what a period actually got - #212
feat(panel): record why cleanup happened and what a period actually got#212lamemustafa wants to merge 5 commits into
Conversation
Closes #209 and #208. **#209.** Three routes reached cleanup -- a confirmed download, a run that produced no ZIP, and a legacy staging cleared on upgrade -- and all three wrote one `cleaned`. The origin was the only durable evidence that the ZIP reached the browser, and the transition discarded it, so a delivered run read as merely captured once the panel reopened and the transient signal was gone. Cleanup now keeps its origin: `cleaned-after-download`, `cleaned-without-export`, `cleaned-legacy`. The delivery claim reads that instead of inferring backwards from a value that had already thrown the answer away. Plain `cleaned` is retained so a ledger written before the split still parses; it carries no origin, so those runs stay indeterminate and read as captured, which is what they did before and is the safe direction for a claim about a file. Every existing `=== "cleaned"` became a check that silently stopped matching three quarters of its runs, so they go through one `isCleanedZipPhase` predicate rather than a comparison repeated per call site. **#208.** A multi-artifact period reaches `downloaded` when one artifact staged and another was explicitly unavailable, because an unavailable artifact is a resolved outcome -- so `saved` claimed the whole selection for a period that had part of it. It now reads `Partly saved`, kept distinct from `Needs review` because an artifact the portal never offered is not a fault a re-run corrects. The issue expected this to need a new persisted field. It does not: `filed-return-artifact-unavailable:<TYPE>` survives durable signal parsing, so a terminal target already carries it. The derivation was reading the run's step, which carries the signal for whichever period is in flight and would have marked the whole year partial because one period was. Counted separately in the status line. A partly saved period is in none of the other three counts, so without its own clause it would leave the header saying "1 of 2 saved" with the second period unaccounted for anywhere on the line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`completeFullFiscalYearLedger` had no test at all, so the value it writes -- now the only durable record of whether the ZIP reached the browser -- could be changed without a single failure. Covered per route, plus the no-pending-phase case that must not be guessed into a delivery. The evidence fixture built its targets behind a blanket cast and omitted every field the outcome did not read, so it never exercised the record the runtime is given: the first derivation to read one of those fields threw on all fifteen tests in the file at once. The targets are faithful now. Each addition mutation-checked: reverting the origin-carrying write, the partly-saved branch, or the status-line clause fails one test each, with the rest of the file passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… claim `(target.safeSignals ?? []).some(...)` reads as caution and is the opposite. Absent signals would mean no evidence of a missing artifact, which resolves to `saved` -- a fully saved period -- on the one field that decides whether the period was complete. That is "could not determine" answering "matches", on the claim the column exists to keep honest. It was also the odd one out: `hasLegacyRetainedStaging`, ten lines above in the same file, dereferences the same field unguarded. The field is required by the type, set by every construction path, and its absence fails ledger validation, so neither can be reached with a real record. A malformed one throwing is diagnosable; a malformed one silently reading as saved is not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review Requesting a review of head The change closes #209 and #208. Two things a reviewer may want to look at first:
|
`markFullFiscalYearCleanupPending` defaulted its phase argument to `downloaded-cleanup-pending` -- the one value that asserts the ZIP reached the browser. One call site relied on that default: retrying the OPFS discard of an already-completed same-scope ledger. So a run that had cleaned as `cleaned-legacy` or `cleaned-without-export` was re-marked onto the delivery route, and completion then wrote `cleaned-after-download` for files that were never exported. That is the overclaim the origin split exists to prevent, reintroduced through the transition meant to preserve it -- and it was invisible because the assertion was made by a default nobody had to type. Two changes, because either alone leaves the mechanism: - The default is gone. A phase that asserts a delivery must be chosen, not inherited by a caller that did not consider the argument. - That call site derives its route from the phase the ledger already reached, via `cleanupPendingPhaseFor`, which never upgrades. A pre-split `cleaned` carries no origin and takes the non-delivery route, so it keeps reading as captured exactly as it did before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers all four starting phases through the retry route and back out of completion. Collapsing `cleanupPendingPhaseFor` to the delivery route fails this test and only this test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c51f20929
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| flowStep.safeSignals.includes("full-fiscal-year-zip-downloaded") || | ||
| zipPhaseProvesDelivery(ledger.zipPhase) |
There was a problem hiding this comment.
This change makes the new persisted cleanup phase durable proof that a ZIP reached the browser, but the commit's Verification section explicitly leaves the live browser check outstanding. Because this affects runtime and download evidence, exercise the actual authenticated portal download, cleanup, and panel-reopen path before treating the change as ready; synthetic tests cannot confirm that the live signal-to-phase transition matches the portal flow.
AGENTS.md reference: AGENTS.md:L75-L77
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted, blocking, and left open deliberately — this is not something I can close.
The gate is correct and it is the one I recorded against myself in the Verification section rather than checked. AGENTS.md L75-77 requires a live authenticated run for runtime and download-evidence changes, and this change is squarely both: it makes a persisted phase the durable proof that a ZIP reached the browser.
Your reasoning is the part worth agreeing with explicitly. Synthetic tests here assert that given a pending phase, completion writes the right terminal one — they say nothing about whether the live flow actually reaches downloaded-cleanup-pending rather than one of its siblings on a real portal download. That transition is the whole claim, and it is the only part of it no fixture can reach.
It needs an authenticated session, so it is the maintainer's action rather than mine. Recorded in the PR body as the outstanding gate; the PR should not merge until it is done.
What the run has to establish, in order:
- A full-year download completes and the ZIP reaches the browser.
- Reopen the panel. Every period that was saved still reads
Saved, notCaptured— this is the A delivered full-year run reads as captured after the panel reopens #209 regression, and before this branch it failed here. - A period where the portal offered one selected format and not another reads
Partly saved, and the status line accounts for it separately. - A run that produces no ZIP, and a retained legacy staging cleared on upgrade, both still read
Capturedafter reopen. These are the two siblings that must not be promoted.
Step 4 matters most: promoting either of those is the overclaim the whole change exists to prevent, and a defect in this branch did exactly that until 40ea54d.
One correction to the finding's framing, which does not change the disposition: the cleanup phase is not a new persisted field. zipPhase already persisted; it gained three values so that cleanup stops discarding which route it came from. Widening that enum was raised and decided before it was made, per the ask-first rule.
1 participant
Summary
Closes #209 and #208. Both are defects in the per-period evidence column from
#207: one understates a delivered run, the other overstates an incomplete
period.
Root Cause / Decision Record
#209 — a delivered run read as captured after the panel reopened. Three
routes reached cleanup: a confirmed download, a run that produced no ZIP, and a
legacy staging cleared on upgrade. All three wrote one
cleaned. That originwas the only durable evidence the ZIP reached the browser, and the transition
discarded it, so the claim had to fall back on a signal that exists only on the
step which observed the download and is absent from the step a re-summarisation
builds.
Cleanup now keeps its origin —
cleaned-after-download,cleaned-without-export,cleaned-legacy— chosen over adding azipDeliveredboolean because it fixesthe collapse itself rather than routing around it, and because the three-way
ambiguity in
zipPhasewould otherwise stay in place for every other reader.It widens an existing enum rather than adding a field, and persists one bit
about the run: no taxpayer data.
Plain
cleanedis retained so a ledger written before the split still parses.It carries no origin, so those runs stay indeterminate and read as captured —
what they did before, and the safe direction for a claim about a file.
#208 — a period read Saved when part of the selection never arrived. A
multi-artifact target reaches
downloadedwhen one artifact staged and anotherwas explicitly unavailable, because an unavailable artifact is a resolved
outcome.
Partly savedis kept distinct fromNeeds reviewdeliberately: anartifact the portal never offered is not a fault a re-run corrects, and routing
it to review would send someone looking for a problem that is not theirs.
The issue expected a new persisted field and it needed none.
filed-return-artifact-unavailable:<TYPE>survivesparseDurableFiledReturnsSignals,so a terminal target already carries it — the derivation was reading the run's
step, which holds the signal for whichever period is in flight and would have
marked the whole year partial because one period was.
Smallest safe fix. Neither change adds a persisted field. The one enum
widening was raised and decided before it was made, per the
AGENTS.mdask-first rule.
Scope
filed-returns-contracts.ts(phase split,partly-saved, twopredicates),
filed-returns-full-fiscal-year-ledger.ts(origin-carryingwrite),
filed-returns-full-fiscal-year-summary.ts(delivery and partialderivation), four readers routed through
isCleanedZipPhase,popup/target-evidence.tsxandstyles/panel.css.delivery cases in the evidence tests; header-count and row cases in the panel
tests; the evidence fixture made faithful.
in README or store copy.
change to what persists beyond the one enum widening described above.
Pack Workflow Preflight
pnpm workflow:preflightwas run before editing/push, or the skip reason is documented.Sanchika Adoption Gate
@sanchika/*packages or copied Sanchika guidance, Iread
sanchika/docs/adoption-pack.mdin the coordinated parent worktree.and records the Sanchika commit or copied guidance used.
../sanchika,sanchika/packages/*/src, or parentsource paths.
Privacy And Data-Flow Impact
Sensitive Surface Review
Chrome Web Store Impact
docs/PUBLICATION_READINESS.mdis checked.Verification
pnpm install --frozen-lockfilepnpm audit --audit-level highpnpm exec wxt preparepnpm exec prettier --check .pnpm exec eslint . --max-warnings 0pnpm exec tsc --noEmitpnpm exec vitest run—Tests 2066 passed (2066),Test Files 123 passed (123)pnpm exec wxt buildnode scripts/verify-extension-package.mjs .output/chrome-mv3pnpm exec wxt zip(release-only; not run)node scripts/verify-extension-zip.mjs(release-only; not run)node scripts/write-release-provenance.mjs(release-only; not run)node scripts/verify-github-release-assets.mjs --tag <tag> --zip <zip> --checksum <sha256> --provenance <json>when release assets existnode scripts/publish-chrome-web-store.mjs --zip .output/<zip> --provenance .output/pack-release-provenance.v1.json --publisher-id <id> --dry-run truegit diff --checkpnpm review:gate -- --strict-head-review --required-review-author chatgpt-codex-connectorbefore merge, with a missing Codex review recorded as an audit gap if reported. Dropping--allow-missing-head-reviewis deliberate — see Documented review-gate commands are satisfiable without a reviewer #211, where--strict-head-reviewalone turned out to be satisfiable by one's own thread replies.Live authenticated run outstanding — this PR must not merge until it is done.
Raised by review as P1 and accepted; the thread is left open deliberately,
because it needs an authenticated session and is the maintainer's action.
AGENTS.mdL75-77 requires a live run for runtime and download-evidencechanges, and this is both. The synthetic tests assert that given a pending
phase, completion writes the right terminal one; they say nothing about whether
the live flow reaches
downloaded-cleanup-pendingrather than a sibling on areal portal download. That transition is the entire claim and the only part of
it no fixture can reach.
What the run has to establish, in order:
Saved, notCaptured— the A delivered full-year run reads as captured after the panel reopens #209 regression, which fails here before this branch.
Partly saved, with the status line accounting for it separately.both still read
Capturedafter reopen.Step 4 matters most: promoting either sibling is the overclaim the change exists
to prevent, and a defect in this branch did exactly that until 40ea54d.
Artifact Evidence
PR Review Follow-Up
Two things worth a reviewer's attention rather than left implicit:
completeFullFiscalYearLedgerhad no test at all, so the value it writescould be changed without a single failure — and that value is now the only
durable record of delivery. Covered per route in 7e54ad1.
ascast and omittedevery field the outcome did not read. It therefore proved nothing about the
record the runtime is given: the first derivation to read one of those fields
threw on all fifteen tests in the file at once. The targets are faithful now.
Self-review found a defect in this PR's own fix, and it is the more serious of
the two things found this way.
markFullFiscalYearCleanupPendingdefaultedits phase argument to
downloaded-cleanup-pending— the one value that assertsthe ZIP reached the browser. One call site relied on that default: retrying the
OPFS discard of an already-completed same-scope ledger. So a run that had cleaned
as
cleaned-legacyorcleaned-without-exportwas re-marked onto the deliveryroute, and completion then wrote
cleaned-after-downloadfor files that werenever exported.
The origin split exists to prevent exactly that claim, and this reintroduced it
through the transition meant to preserve it. It was invisible because the
assertion was made by a default nobody had to type. Fixed in 40ea54d by removing
the default outright and deriving that call site's route from the phase the
ledger already reached, since either change alone leaves the mechanism in place.
I reached this by asking whether
downloaded-cleanup-pendingshould also countas delivery evidence. Tracing its writers to answer that is what surfaced the
third one — the check was worth running even though the answer turned out to be
no.
The smaller one, recorded rather than quietly amended:
the first version of
targetMissedAnArtifactread(target.safeSignals ?? []),which resolves absent signals to no missing artifact and therefore to
saved.That is "could not determine" answering "matches" on the field that decides
whether a period was complete, and it was the odd one out — the function ten
lines above dereferences the same field unguarded. Fixed in a third commit.
Each behavioural addition is mutation-checked — reverting the origin-carrying
write, the
partly-savedbranch, or the status-line clause fails exactly onetest each with the rest of its file passing.
Screenshots
None. The change is a new row label, glyph and status-line clause; the rendered
markup is asserted in
tests/popup/target-evidence.test.tsx. A live panelscreenshot is part of the outstanding browser check noted above.
DCO
Signed-off-by:trailers. Every commit onmaster does, because the trailer is applied at squash merge — see
9218160,f615c1c,cd3a4db. Left unchecked rather than claimed, sincethe box describes the branch commits; chore(governance): make the DCO checkbox mechanically satisfiable, or drop it #109 tracks making this box
mechanically satisfiable or dropping it.