fix: normalize GitOps list results for search#152
Conversation
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
|
|
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
There was a problem hiding this comment.
- Findings ordered by severity
- Important —
src/registry/toolsets/gitops.ts:79-105,src/registry/toolsets/gitops.ts:245,src/registry/toolsets/gitops.ts:290,src/registry/toolsets/gitops.ts:618,src/registry/toolsets/gitops.ts:663,src/registry/toolsets/gitops.ts:712,src/registry/toolsets/gitops.ts:912,src/registry/toolsets/gitops.ts:955,src/registry/toolsets/gitops.ts:1025,src/registry/toolsets/gitops.ts:1066,src/tools/harness-list.ts:69-75,src/registry/extractors.ts:15-21,src/registry/extractors.ts:107-119,tests/registry/registry.test.ts:400-416
gitopsListExtract()fixesharness_searchby addingitems, but it does so withreturn { ...raw, items, total }, which makes the registry/public list contract leak GitOps transport keys likeapplications,clusters, etc. back out to callers. That breaks the repo's shared normalized-list pattern: existing extractors normalize to canonicalitems/totalshapes, whileharness_listonly compactsresult.items. So GitOps list results will now expose both a compactitemsarray and an un-compacted raw array, creating duplicated/inconsistent output and reintroducing API-specific leakage at the wrong abstraction layer. The new registry test hard-codes that leakage as expected behavior by assertingresult.applicationsexists.
- Open questions/assumptions
- Assumption: the intended contract for
registry.dispatch(..., "list", ...)remains the normalized list shape thatharness_list/harness_searchconsume (items+total), not a backward-compatible promise to preserve API-specific array keys. I did not find another consumer in this diff that needsapplicationsto remain public. - Open question: if preserving raw GitOps keys is intentional for compatibility, should
harness_listalso suppress or compact those aliases so the public tool output still stays canonical?
- Summary
- The PR fixes the immediate GitOps search regression, but it does not fully follow Sunil's architecture standards as written because it solves the issue by preserving API-specific list envelopes in the registry/output contract instead of keeping GitOps on the shared extractor/normalized-response path.
Sent by Cursor Automation: Sunil On Demand Architecture Review
There was a problem hiding this comment.
- Important:
src/registry/toolsets/gitops.ts
The PR fixes the immediate search bug, but it does so by expanding the shared registry/list contract to preserve raw GitOps transport keys (applications,clusters, etc.) alongsideitems/total. That conflicts with the shared extractor pattern insrc/registry/extractors.tsand with the documented list-output contract inAGENTS.md/CLAUDE.md, which standardize list responses around normalized fields. The new registry assertion intests/registry/registry.test.tsthen locks that GitOps-specific shape in as part of the public contract instead of keeping the compatibility concern at the caller boundary.
Open question / assumption:
- I’m assuming we do not intentionally guarantee raw GitOps array keys from
harness_list. If some downstream caller depends on them, that compatibility should be made explicit and handled above the registry layer rather than in the extractor itself.
Verification:
pnpm test tests/registry/registry.test.ts tests/tools/tool-handlers.test.tspnpm typecheck- PR CI:
build-and-testis green; smoke jobs were still running during review.
Summary:
- The functional bugfix looks good, but I don’t think the current shape fully follows Sunil’s architecture standards because it codifies an API-specific response contract in the shared registry layer.
Sent by Cursor Automation: Sunil On Demand Architecture Review
| (pagination ? numberField(pagination, "total") : undefined) ?? | ||
| items.length; | ||
|
|
||
| return { ...raw, items, total }; |
There was a problem hiding this comment.
This fixes harness_search, but return { ...raw, items, total } makes the shared registry contract GitOps-specific again. The extractor layer has generally normalized list responses to canonical fields (items, total, plus pagination metadata when needed) rather than preserving transport-specific keys, and the new registry test now hard-codes those raw keys as part of the contract. To stay aligned with Sunil's standards, I think this should normalize to the canonical list shape here and keep any backward-compatibility shim outside the registry layer.


Description
Normalize GitOps list responses so API-specific array keys like
applications,clusters, andrepositoriesalso exposeitems/totalfor generic consumers such asharness_search. Added regressions for scoped GitOps application search and direct list normalization.Type of Change
Checklist