fix: align GitOps update identifier guidance#181
Conversation
Co-authored-by: rohan.gupta <rohan.gupta@harness.io>
Co-authored-by: rohan.gupta <rohan.gupta@harness.io>
|
|
Co-authored-by: rohan.gupta <rohan.gupta@harness.io>
There was a problem hiding this comment.
Stale comment
src/registry/toolsets/gitops.ts: thegitops_application.updatecontract is still internally inconsistent. This PR fixes the prose description to sayresource_idis the app name andagent_idbelongs inparams, butharness_describealso surfacesbodySchema.description, and that structured metadata still saysapp_name (path)is passed viaparams. Under the repo’s architecture guidance, required fields/body format should live in structured metadata, so agents can still receive conflicting update instructions after this change.Assumptions:
- I treated
harness_describeoutput as part of the architecture contract becauseCLAUDE.mdexplicitly prefers structured agent guidance (bodySchema,executeHint, etc.) over prose-only instructions.Verification:
pnpm test tests/registry/gitops.test.tspnpm typecheckpnpm testSent by Cursor Automation: Sunil On Demand Architecture Review
There was a problem hiding this comment.
- Important —
src/registry/toolsets/gitops.ts,tests/registry/gitops.test.ts:gitops_applicationset.updatenow tells agents to passresource_id='<appset_id>', but the runtime still does not consumeappset_idfor this update path.harness_updateonly copiesresource_idintoinput.appset_id, and this spec never mapsappset_idinto the request; the real identifier remainsbody.applicationset.metadata.uidplusparams.agent_id. That leaves the changed guidance out of sync with actual generic update behavior, and the new ApplicationSet test does not catch it. - Important —
tests/registry/gitops.test.ts: the added regression coverage bypasses the sharedharness_updatelayer by callingRegistry.dispatch()withapp_name/appset_idalready populated. Because the bug being fixed is about genericresource_idsemantics, these tests would stay green ifsrc/tools/harness-update.tsregressed. A tool-level test throughregisterUpdateTool()is needed to prove the user-facing contract.
Open questions / assumptions:
- I assumed keeping
harness_update.resource_idmandatory is intentional. If so, shouldgitops_applicationset.updatevalidateresource_id === metadata.uid, or should the guidance explicitly saymetadata.uidis authoritative andresource_idis only present because of the shared tool schema?
Brief summary:
- I found two important issues. Local verification after installing dependencies:
pnpm test tests/registry/gitops.test.ts,pnpm test tests/tools/tool-handlers.test.ts,pnpm typecheck, andpnpm buildall passed.
Sent by Cursor Automation: Sunil On Demand Architecture Review
| " 2. Modify the fields you need\n" + | ||
| " 3. harness_update(resource_type='gitops_applicationset', resource_id='account.myagent',\n" + | ||
| " body={applicationset:{metadata:{name:'my-appset', uid:'<identifier>'}, spec:{...project:'<project>'...}}})\n\n" + | ||
| " 3. harness_update(resource_type='gitops_applicationset', resource_id='<appset_id>',\n" + |
There was a problem hiding this comment.
Does resource_id actually need to be <appset_id> here? harness_update maps it into input.appset_id, but this update spec never reads appset_id back out (no path/query mapping, and bodyBuilder() only validates body.applicationset.metadata.uid). I spot-checked the built registry with two different resource_id values and got identical PUT /gitops/api/v1/applicationset requests, so this guidance is still stricter than the runtime contract.
| const mockRequest = vi.fn().mockResolvedValue({ applicationset: { metadata: { name: "demo-set" } } }); | ||
| const client = makeClient(mockRequest); | ||
|
|
||
| await registry.dispatch(client, "gitops_applicationset", "update", { |
There was a problem hiding this comment.
This test never goes through harness_update; it calls Registry.dispatch() with appset_id already set, so it does not verify the generic resource_id -> appset_id mapping the PR is documenting. That matters here because the runtime currently ignores appset_id for ApplicationSet update and keys entirely off metadata.uid + agent_id, so this stays green even if the user-facing contract is still wrong.
|
@thisrohangupta I have already made the fix in PR
|


Description
Corrects GitOps update guidance so
resource_idmatches the generic nested-resource convention (app_name/ ApplicationSet UUID) andagent_idis passed throughparams. Adds focused registry coverage for the guidance and dispatch wiring.Type of Change
Checklist