refactor(gst): make an unbound refusal impossible to construct - #357
lamemustafa wants to merge 3 commits into
Conversation
Recording a refusal resolves a target outright: the period is answered, the run advances, and no artifact ever follows to corroborate it. The visible page is therefore the whole of the evidence, and a refusal read from a page that was never checked against the target is a wrong answer that looks exactly like a right one. Three emitters each learned that separately -- one reported defect at a time, across three pull requests -- and nothing stopped a fourth from not learning it. The check is no longer something an emitter remembers: `declinedArtifactStep` takes proof as its first argument, and the only values of that type come from a binder in this module. The brand is a type-only symbol exported nowhere, so no object literal written elsewhere satisfies it. A deliberate double assertion would still get through; this makes the check impossible to forget, which is how all four defects happened, not impossible to circumvent on purpose. Two duplicates go with it. The GSTR-2B safe message and its retry wording were written out in both emitters, identical in both, with nothing able to contradict them -- no test compares one emitter's copy with another's. They are now a `Record` over the declined signals, so a third refusal cannot be registered without deciding what a reader is told and what they are offered. The binding signals register by derivation. Writing one by hand is how seven periods a live run had correctly answered came back as needs-review: one unregistered token rejects the entire array it travels in, and a refusal is a terminal step whose signals are persisted.
No behaviour change intended. Each emitter already ran a guard and built its own result; each now presents the guard's proof and asks for the result to be built. The flow path keeps its extra step: a stale panel is not merely refused there, it is navigated away from, because the run still needs the period it asked for. `filed-returns-durable-status` reads the declined wording from the same table rather than importing a constant that said it a second time.
The guarantee is a compile-time one, so the test is too: `tsc --noEmit` fails if either forgery below stops being an error. Removing the brand makes the `@ts-expect-error` unused, which is the mutation that proves it holds.
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: 319d4ece0b
ℹ️ 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 recording a portal refusal without checking the visible page impossible to
construct, rather than something three emitters each had to remember.
Sixth in the stack, on top of #356. No behaviour change intended: every emitter already
ran the guard. This removes the possibility that a fourth one does not.
Root Cause / Decision Record
What failed. Recording a refusal resolves a target outright -- the period is
answered, the run advances, and no artifact ever follows to corroborate it. The visible
page is therefore the whole of the evidence. Four separate defects across this stack were
one shape: a refusal read from a page nobody had checked against the target. Each was
found only after the previous fix shipped, because each fix reached the instance and not
the class.
Why this is the smallest safe fix. The alternative is what has been happening: a
reviewer finds the next emitter, one round at a time.
declinedArtifactStepnow takesproof as its first argument, and the only values of that type come from a binder in the
same module. The brand is a type-only symbol exported nowhere, so no object literal
written elsewhere satisfies it.
Honest limit. A deliberate
as unknown as VisibleTargetBindingstill gets through.This makes the check impossible to forget -- which is how all four defects happened --
not impossible to circumvent on purpose. That is stated in the module comment too.
What it closes. No open thread directly; it removes the class behind
#353
r3985770792, #354r3985765483, #355r3985794277, and the original #355observation-path defect, so a fifth instance cannot be written.
Scope
filed-returns-declined-artifact.ts; the three emitters infiled-returns-post-click-blocked-state.tsandgstr2b-flow.ts;filed-returns-durable-status.tsreads the wording from the same table;filed-returns-durable-signals.tsregisters the binding signals by derivation.tests/connectors/filed-returns-declined-artifact.test.ts.safeSignalsbeyondstring[](190 call sites, 40modules) -- the same class one level wider, and 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
319d4ec.Local gates on
319d4ec, quoted verbatim:Base (
dc67c92) was 181 files / 3379 tests. An earlier run of the same suite showed 19failures in
tests/scripts/publish-review-gate-check.test.ts; that file spawnssubprocesses and flakes under parallel load on this machine -- it was reproduced on an
untouched base earlier in this stack, and this diff does not touch it.
The guarantee is verified at compile time, so its test is too. The test file carries
two
@ts-expect-errorforgeries;tsc --noEmitfails if either stops being an error.Removing the brand makes one of them unused and
tscreportsTS2578: Unused '@ts-expect-error' directive-- that is the mutation proving the brand iswhat enforces the rule, not the comment above it.
PR Review Follow-Up
Two things worth checking rather than assuming:
(
gstr2b-visible-period-verified/filed-gstr1-detail-period-verified), so thedurable record says how the refusal was checked.
filed-gstr1-detail-period-verifiedis new. It is registered by derivation --
REFUSAL_BINDING_SIGNALSis spread into thedurable allowlist -- because writing one by hand is exactly how seven correctly
answered periods came back as needs-review earlier in this stack.
in both. They are now one
Recordover the declined signals, so a third refusal cannotbe registered without deciding what a reader is told.
Screenshots
None -- no user-visible surface changed.