fix(space): artifact shape reader polish — evolution evidence + list_artifacts kind filter - #2319
Conversation
…st_artifacts kind filter extractArtifactDetail now checks canonical shape fields (url/text/recommendation) before legacy ones, so fresh link/note/decision writes surface in evolution evidence (migrated rows still resolve via pr_url/summary). list_artifacts post-filters legacy type:'pr'/'review' by data.kind so they no longer over-return unrelated link/decision kinds; result/progress and shape-name queries stay unfiltered.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThis PR delivers two targeted reader fixes deferred from the generic artifact-shapes PR (#2313):
Confidence Score: 5/5Safe to merge — both fixes are narrowly scoped reader-only changes with no writes or schema mutations, and the migration-boundary edge case is explicitly documented and covered by a dedicated test. Both changes are read-path only: Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/daemon/src/lib/space/evolution-scope-service.ts | Prepends canonical shape fields to extractArtifactDetail's key scan; function exported for unit testing. Logic and field ordering are correct. |
| packages/daemon/src/lib/space/tools/node-agent-tools.ts | Adds kindFilter post-filter for pr/review legacy type queries; narrow, correct, and well-commented. |
| packages/daemon/tests/unit/5-space/agent/node-agent-tools.test.ts | Adds comprehensive list_artifacts test suite: mixed-kind seeding, per-legacy-type assertions, kind-less migration boundary case explicitly documented and tested. |
| packages/daemon/tests/unit/5-space/evolution-scope-service.test.ts | Adds extractArtifactDetail unit tests covering new canonical fields, priority ordering, legacy fallback, and null/empty guard cases. |
Reviews (2): Last reviewed commit: "test(space): cover kind-less link/decisi..." | Re-trigger Greptile
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (z.ai)
Model: glm-5.1 | Client: NeoKai | Provider: z.ai
Recommendation: REQUEST_CHANGES — one P2 (test gap) + 2 open Greptile threads to resolve before merge.
Both fixes are correct, well-scoped, and well-tested. Verified independently.
extractArtifactDetail — canonical fields (url/text/recommendation) now probed before legacy fields, so fresh link/note/decision writes surface in evolution evidence. Migrated rows still resolve (normalizeLinkData carries url; result→decision keeps summary). The one behavior change — a decision carrying both recommendation and summary now returns the verdict — is intentional and correct (the verdict is the primary signal). Clean. New export passes knip (used by the test).
list_artifacts kind filter — correct and tight: kindFilter applies only to pr/review; result, progress, direct shape-name queries, and no-type stay unfiltered. I confirmed the new tests genuinely catch the bug by running them against the pre-fix source (origin/dev): without the filter {type:'pr'} returns 3 links (not 1) and {type:'review'} returns 2 decisions (not 1). Restored after; tree clean, tests green (213 pass), oxlint/tsc/knip all clean.
Greptile P1 (kind filter "silently drops" kind-less rows) — reviewed and dismissed as unfounded. The backfill migration at migrations.ts:11290-11299 sets kind:'pr' for pr→link, kind:'review' for review→decision, and infers kind from URL fields for result→link. So every semantically-PR/review row carries the right kind; the only kind-less link/decision rows are legitimately-not-PR/not-review direct shape writes, which should be excluded. No silent regression (Greptile itself rates this 4/5 "safe to merge with awareness").
P2 — add a kindless-link exclusion test (endorsing Greptile's second thread; anchored comment posted). The suite only covers the kind-always-written path. Add a test asserting a link written without kind is excluded from { type:'pr' } (and the kind-less decision analog for { type:'review' }). Documents the intentional trade-off at the migration boundary and guards against a future change that wrongly includes kindless rows.
Before merge: resolve the 2 open Greptile threads — dismiss P1 with the migration-backfill note above; P2 is addressed by the new test.
Note: the local bun run check check:test-quality blip in provider-registry.test.ts is pre-existing on origin/dev, outside this diff, and not in PR CI — not a concern for this PR.
…eview filters Documents the intentional narrowing at the migration boundary: a directly- written link/decision with no kind tag is legitimately not a PR/review (the backfill migration only tags pr/review/result-derived rows), so the legacy type:'pr'/'review' filters exclude them while direct shape queries return them.
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (z.ai)
Model: glm-5.1 | Client: NeoKai | Provider: z.ai
Recommendation: APPROVE. Zero findings remain — P2 addressed, all threads resolved.
The kindless-row exclusion test (commit 3a855bc) is correct and is a genuine guard: it writes a kind-less link and decision, asserts {type:'pr'}/{type:'review'} exclude them while direct {type:'link'}/{type:'decision'} still return them. Without the kind filter the {type:'pr'} assertion would fail (the 1 kindless link would be returned), so this locks the behavior in. Passes locally (179 tests).
Verified on the new head: 0 unresolved review threads; CI green (Lint/Knip/Typecheck, Web, Greptile, all unit shards, and most online shards pass — rpc-4 and space-2 still in-flight, neither touched by this test-only change). PR is OPEN/MERGEABLE, targets dev.
Both core fixes from the prior round stand correct and well-tested: extractArtifactDetail now probes canonical shape fields first, and list_artifacts post-filters pr/review by data.kind. Ready to squash-merge (do not delete branch).
…link Address review feedback on #2345: - CodingArtifactProfile.onGateDataCommitted now reads review_url from the current send_message payload (messageData) instead of the merged gate state. The prior form would spuriously record an extra review round when a later send updated only comment_urls while a previous round's review_url lingered in the gate state. Adds a regression test for the follow-up-send case. - Fullstack QA success now records the PR as a first-class `link kind:'pr'` (consistent with the other reviewer nodes and robust to legacy-field removal) alongside the terminal `decision` outcome, instead of burying pr_url inside the decision data. - Rebase onto dev: drop the list_artifacts legacy kind-filter and its tests added in #2319 (they re-introduced `pr`/`review` kind names into core, which PR C removes); list_artifacts now filters by shape only.
Two reader fixes deferred from #2313 (the generic artifact-shapes PR), now that it has merged to dev.
extractArtifactDetail(evolution evidence) read only legacy fields, so fresh shape writes never surfaced — it now checks the canonical shape fieldsurl/text/recommendationfirst (link/note/decision), with legacy fields as fallback so migrated rows still resolve.list_artifactsmapped legacytype:'pr'→linkandtype:'review'→decisionbut didn't filter ondata.kind, so{type:'pr'}over-returned issue/preview/doc links and{type:'review'}over-returned gate decisions. It now post-filters those two by their legacy semantic kind;result(overloaded),progress, and shape-name queries stay unfiltered.Covered by new unit tests in both files; the list_artifacts tests fail without the kind filter (verified).