Repository navigation
fix: prescan must see Done-panel and archived stories (#90824) - #77
Open
ea-open-source wants to merge 9 commits into
Open
ea-open-source wants to merge 9 commits into
ea-open-source wants to merge 9 commits into
Conversation
`listStoryPage` never sent `include_done`, so both dedup prescans read the default story list. That list hides two classes of row: - Done-panel rows (EAT #25177) — a row frozen on a past iteration. A closed GitHub issue imports as an `accepted` story carrying `completed_at`, which lands it there. The rows a normal import creates most of are exactly the rows the prescan could not see, so a re-run re-created them and broke CONTRACT.md's re-import promise. - Archived rows (EAT #25174) — excluded by a separate default. An archived row still occupies its `(project, source, external_id)`, so the same duplication applies. `listStoryPage` now takes `includeDone` / `includeArchived`, both defaulting off so no other caller's query changes, and `prescanImported` / `prescanProvenance` both set both. `includeArchived` sends BOTH spellings: `archived=include` and the deprecated `include_archived=true`. A current server obeys the tri-state `archived` and ignores the alias; a deployment older than #25174 obeys the alias and ignores the unknown `archived` param. Neither struct denies unknown fields, so sending both is correct on both. CONTRACT.md's "Marker dedup" section now states the requirement, so the promise and the query cannot drift apart again. Tests, red before green: - `tests/client.test.js` asserts the exact query string, so a dropped flag fails, plus a guard that the flags stay off by default. - `tests/dedup.test.js` pins both flags at both prescan call sites. - `tests/direct.test.js` runs the bundled mock with a row frozen on a past iteration — once via the marker prescan, once via the provenance prescan — and with two archived rows, asserting the skip counts, not just the absence of a duplicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG
The mock's story create never assigned `iteration_id`, so no story it created was ever in its own Done panel. That is why the round-trip regression at tests/direct.test.js:66 stayed green while production duplicated every closed issue on a re-run. The real server places a forward create on the iteration window that holds its `completed_at` (`handlers/stories.rs`, the `forward_create` UPDATE), and its default story list hides a row frozen on a past window. The mock now mirrors that: when it stores a backdated `completed_at` on a non-iceboxed row, it assigns a past-window `iteration_id`. Measured, in the worktree, on the round-trip test alone: - pre-PR src/client.js + src/dedup.js + this mock change: FAIL (`rerun.importedStories` 1, expected 0 — the hidden row was re-imported). - pre-PR src/client.js + src/dedup.js + pre-PR mock: PASS — the gate could not fail for the defect it exists to catch. - head src/client.js + src/dedup.js + this mock change: PASS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG
… archived CONTRACT's dedup bullet said the list "hides two classes by default" and that the prescan "must see every row". The list query also pins `AND s.expired IS NULL` with no param to lift it, so the sentence was one class short and categorical where it could not be. - Scope the claim to the rows the list can still return, and say the two liftable classes are the whole opt-in set. - Name soft-deleted (`expired`) rows as the third class, with no opt-in, dormant today because the delete path hard-deletes. Re-creating a row the member deleted is the intended outcome. - Replace the archived justification. "An archived row still occupies its (project, source, external_id)" implied a conflict the schema does not raise: `story_import_provenance_idx` is deliberately NOT unique. The checkable reason is parity — the server importer's dedup preload filters neither archived, Done-panel nor `expired` rows, so the direct engine was the engine out of step. The "two and no more" claim is pinned, not just written: the prescan visibility test now compares the whole visibility option set instead of two keys. Measured — with a third flag added to PRESCAN_VISIBILITY the old assertion passes and the new one fails on the extra key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG
An archived row now suppresses its GitHub issue forever, and the run said only `skipped N (already imported)`. EAT hides archived rows from its default views, so the user finds no story on the board and no way to learn why the issue never came back. - Both prescan fieldsets request `archived`, and a run that skips an issue whose story is archived prints one warning with the count and the remedy. A live skip is not counted. - The mock's `fields=` allowlist gains `archived`, `archived_at` and `iteration_id` — the server has accepted all three since story #275 — and its read row derives `archived` from `archived_at` the way the real list and detail paths do. Compatibility, measured in the agile-tracker history: `archived` / `archived_at` entered the server's `fields=` allowlist in 0c1c5dc29 (2026-06-12, #275), and the visibility params this section reasons about arrived in 83e6fe384 (2026-06-22, #25174 + #25177). No deployment that can hide a row refuses the fieldset. That same commit pairing is why `include_done` needs no old-deployment argument, which CONTRACT now says. CONTRACT: the mock-allowlist divergence bullet claimed the published openapi.json omits the three fields; it has listed them since 2026-08-04, so the bullet was false before this change and is rewritten. The documented provenance fieldset was also one word short (`blocker_count`) and is corrected alongside the new `archived`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG
`projectHasStories` sent `?limit=1` with no visibility flags, so a project whose rows are all Done-panel or archived read as empty — which is the normal shape of a project a previous GitHub import filled. The "already has stories; import appends, it does not replace" warning never printed, on either engine. One `admitHiddenStories` helper now builds the flags for both call sites, so the prescan reads and the preflight probe cannot drift apart. The two exact-query-string assertions become sorted `[...searchParams]` pair comparisons. That keeps the whole-set strictness the panel asked to preserve and drops the coupling to `params.set` order. Mutation-measured on the full suite: - drop `include_archived`: 2 tests fail (was 1). - drop `archived=include`: 2 tests fail (was 1). - swap the order of the two: 1255 pass, 0 fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG
…string
"Both spellings, deliberately" was pinned only by an assertion on the
query string: the mock always honoured the tri-state, so nothing ran
against a server that obeys the alias alone.
`makeState({ archivedTriState: false })` now models a deployment older
than #25174 — `archived` is an unknown param there, so the mock neither
validates nor obeys it — and one archived prescan regression runs against
it.
Mutation-measured on the full suite:
- drop `include_archived`: 3 tests fail, including the new behavioural
one. Before, no behavioural test noticed.
- drop `archived=include`: the new test stays green, so the two spellings
are pinned independently rather than as a pair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG
Three reviewers raised it and one asked for it in writing. The provenance pass stays bounded by `import_source=github`; the marker pass cannot narrow that way, so it now pages the project's whole history and its request count grows with total history rather than with imported rows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG
The paged tests asserted calls[1].cursor and nothing else, so a regression that set the visibility flags on the first request only stayed green and still duplicated on a re-run. Raised by reviewer js-2 and dropped from the consolidation by mistake. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #90824 — Dedup prescan cannot see Done-panel stories, so a re-import duplicates them.
The defect
src/client.jslistStoryPagebuilt its query fromlimit,cursor,fields,import_source,import_external_idonly. It never sentinclude_done.Both dedup prescan paths ride that one call —
prescanImportedandprescanProvenanceinsrc/dedup.js. They are the only callers oflistStoryPageinsrc/, so the two call sites are the whole blast radius.The server hides two classes of row from the default story list (verified against agile-tracker
origin/main,platform/backend/src/handlers/stories.rs):include_done=true.A closed GitHub issue imports as an
acceptedstory carryingcompleted_at, which lands it on a past iteration. The rows a normal import creates most of are exactly the rows the prescan could not see, and a re-run re-created them. That breaks CONTRACT.md's re-import promise.The fix
listStoryPagetakesincludeDoneandincludeArchived. Both default off, so no other caller's query changes.prescanImportedandprescanProvenanceboth set both.Archived rows: included, deliberately
The story raises archived rows as a "consider". I decided to include them. An archived row still occupies its
(project, source, external_id), so the same duplication bug applies to it.includeArchivedsends both spellings,archived=includeandinclude_archived=true:archivedis the tri-state param (exclude|include|only, story #25174). A current server obeys it and ignores the deprecated alias.include_archivedis the DEPRECATED alias, honoured only whenarchivedis absent. A deployment older than #25174 obeys the alias and ignores the unknownarchivedparam.StoryFiltercarries nodeny_unknown_fields, so the unknown param cannot 400 an older deployment.Both spellings together are therefore correct on both.
Tests — red before green
Each test below failed on
origin/mainfor the right reason before the fix landed.tests/client.test.js— the existing provenance-filter test now asserts the exact query string, so a dropped flag fails rather than slipping past a per-key lookup. A second test guards that the flags stay off unless the caller asks.tests/dedup.test.js— pinsincludeDoneandincludeArchivedat both prescan call sites. They are separate call sites; a fix to one does not fix the other.tests/direct.test.js— three offline regressions against the bundled mock (src/mockserver.jsalready models the tri-statearchived, theinclude_archivedalias, and theinclude_donepast-iteration rule):skipped 1,importedStories 1, no duplicate row;skipped 1,importedStories 1, no duplicate row;skipped 2,importedStories 0, row count unchanged.Every one asserts the attempt shape (the skip counts), not only the absence of a duplicate, so a test cannot pass because nothing ran.
Gates
npm testℹ tests 1252 … ℹ pass 1251 … ℹ fail 0 … ℹ skipped 1npm run lintChecked 45 files in 130ms. No fixes applied. Found 1 warning. Found 1 info.npm run typechecktsc --noEmit, no outputThe 1 skip is the opt-in e2e test. The 1 warning + 1 info are pre-existing on
main(src/mockserver.jstooLong), untouched by this change.🤖 Generated with Claude Code
https://claude.ai/code/session_01VeP8156wLDqQqdMMjkjMHG