refactor(gst): make a target status answer its own questions - #356
refactor(gst): make a target status answer its own questions#356lamemustafa wants to merge 3 commits into
Conversation
What a status means was asked as literal comparisons in twenty-two modules. Nothing connected them, so each was an independent place to forget -- and when `not-generated` joined the union, five separate sites went on spelling the question `=== "not-filed"` and silently answered "no" for every period the portal declined to draft. `TARGET_STATUS_BEHAVIOUR` is a `Record` over the union, so adding a member does not compile until every question has been answered for it. That is the whole difference between this and the `Set` it replaces -- which sat directly beneath a comment explaining that a `Set` over a union is not exhaustiveness-checked, and had the defect that comment describes. Two questions are new because the code was asking them without naming them. `statedAbsence` is "is there a file to expect here?", which has one answer for both `not-filed` and `not-generated` even though a reader must keep them apart: one is a claim about the taxpayer, the other about the portal. `requiredEvidenceSignal` is the evidence a stored record claiming this status must carry, which two validators had each spelled out for `not-filed` alone. `isResolvedFullFiscalYearTargetStatus` and `needsExplicitFullFiscalYearRetry` are unchanged in behaviour and now read from the table.
…f it Each asked "is there a file to expect for this period?" and spelled it `status === "not-filed"`, so each answered "no, keep working" for a period the portal had already declined to draft: - evidence retained from a discarded run dropped declined periods entirely; - a year of declined periods was not recorded as having produced no ZIP; - restaging reset declined periods to `blocked`, sending the run back to periods the portal has answered; - two ledger validators accepted a stored `not-generated` record carrying no evidence that anything had been declined, while rejecting the equivalent `not-filed` record. Two more sites asked the complement -- "does this ledger still hold anything unresolved?" -- as hand-written arrays, and the two copies had drifted apart: `filed-returns-current-state.ts` listed seven members and `local-data.ts` six, omitting `cancelled`. A ledger the panel was still surfacing as the current run could therefore be cleared from under it by the local-data guard. Both now ask the question rather than enumerate an answer. `hasTerminalPositiveTarget` was `resolved || "manually-observed"`, which is what `holdsAnswer` means, so it says that instead.
…vers The invariants hold the table together: a stated absence resolves a target and stages nothing, anything resolved is an answer worth keeping, and every status lands in exactly one of active, resolved, or needs-a-decision. Both behaviour cases fail when their site is reverted to the literal it used to spell.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: dc67c929cd
ℹ️ 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".
|
The corrected consolidated candidate is #359, based directly on master. This PR is retained as draft with its commits and review history intact; its unchanged head is not independently merge-ready. The #359 body maps every original automated ask to specific source/test evidence or an explicit held follow-up. Authenticated runtime qualification remains required before merging the replacement. |
|
Superseded by #359. Its disposition register records the final status of this draft’s review asks. This draft is being closed; resolving its remaining threads marks them superseded, not fixed on this unchanged branch. |
1 participant
Summary
Makes a full-fiscal-year target status answer its own questions, and fixes the seven
sites that were asking those questions by naming individual members of the union.
Fifth in the stack, on top of #355. Runtime behaviour changes only where a declined
period (
not-generated) was previously mishandled.Root Cause / Decision Record
What was ambiguous. What a target status means was asked as literal comparisons
across twenty-two modules. Nothing connected them, so each was an independent place to
forget. When
not-generatedjoined the union, five sites went on spelling the questionstatus === "not-filed"and silently answered "no" for every period the portal declinedto draft; two more asked the complement as hand-written arrays that had drifted apart.
Why this is the smallest safe fix. The alternative is fixing each site as it is
reported, which is what produced four consecutive review rounds on this stack. A
Recordover the union makes the class unrepresentable: adding a member does not compile until
every question has been answered for it. The
Setit replaces sat directly beneath acomment explaining that a
Setover a union is not exhaustiveness-checked -- and hadexactly the defect that comment describes, which is the argument for a structure rather
than another warning.
What it closes. Structurally resolves four open findings on #354
(
r3985765507restaging,r3985765513discarded-run evidence,r3985765519no-ZIPsignal,
r3985765525's validation half) plus two latent defects nothing had reported:the diverging actionable/clearable lists, and a stored
not-generatedrecord acceptedwith no evidence behind it. Thread numbers are indicative -- see the table below once
review runs against this head.
Scope
filed-returns-contracts.ts(the table); seven call sites infiled-returns-full-fiscal-year-{summary,staging,run-state,validation}.ts,filed-returns-all-supported-full-fiscal-year-validation.ts,filed-returns-current-state.ts,local-data.ts.tests/connectors/filed-returns-target-status-behaviour.test.ts(invariants),tests/background/full-fiscal-year-declined-period-handling.test.ts(behaviour).complete/partial/...), which hasits own two hand-maintained copies;
TerminalStatusincore/contracts.ts, whichshares two spellings with this union and is a different vocabulary; and typing
safeSignalsbeyondstring[], which is the same class at 190 call sites and belongsin its own change.
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 runpnpm exec wxt buildnode scripts/verify-extension-package.mjs .output/chrome-mv3pnpm exec wxt zipnode scripts/verify-extension-zip.mjsnode scripts/write-release-provenance.mjsnode 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 --wait-head-review-ms 180000before merge/readiness claim; a missing Codex review blocks readiness:Artifact Evidence
dc67c92.Local gates on
dc67c92, quoted verbatim:Base (
e881215) was 179 files / 3353 tests, so this adds 2 files and 26 tests andremoves none. Both behaviour cases fail when their site is reverted to the literal it
used to spell; the mutation was run for each separately.
PR Review Follow-Up
Note for review. Two behaviour changes are deliberate and worth checking rather than
assuming:
local-data.tsnow refuses to clear local data when a target sits atcancelled,where it previously allowed it. That is the stricter side of a divergence between two
hand-written lists --
filed-returns-current-state.tscountedcancelledasactionable, so the panel could surface a ledger the clear would delete. Fail-closed
was chosen for a destructive action; say so if the looser reading was intended.
not-generatedtargets alone. Nothing was staged for them, sonothing needs restaging, and resetting them to
blockedsent the run back to periodsthe portal had already answered.
Screenshots
None -- no user-visible surface changed.