Fixes for IDP tools#159
Conversation
|
|
thisrohangupta
left a comment
There was a problem hiding this comment.
The Checks are failing can you review?
There was a problem hiding this comment.
Stale comment
Found 3 warnings against Sunil's architecture standards:
src/registry/toolsets/idp.ts:idp_workflow.executehas a metadata/runtime mismatch.workflow_detailsis mandatory inbodyBuilder(), but it is not represented inbodySchema.fields, soharness_describeexposes an incomplete contract and the shared required-field validation cannot catch the real missing input. This repo's standards prefer structured agent guidance over prose-onlyactionDescription.src/registry/toolsets/idp.ts:scorecard.listnow stops forwardingpage/sizeand claims the endpoint has no pagination.harness_liststill accepts pagination inputs, so callers now get silent no-op behavior instead of consistent shared-list semantics or a fail-loud error.- Public docs are no longer in sync with the runtime surface.
pnpm build && pnpm docs:checkfails withREADME.md is stale, and theidp_scoresplit leaves staleget/pagination docs inREADME.mdanddocs/testing/idp_score/*.Assumptions / questions:
- I assumed the existing scorecard and
idp_scoredocs still reflect the intended public contract. If the underlying IDP APIs changed, those docs need to be updated in the same PR.- I did not see focused tests covering the new
idp_workflow.executerequest-shaping path, so there is residual risk around IDP response/body contract assumptions.Checks run:
gh pr checks 159pnpm buildpnpm testpnpm typecheckpnpm build && pnpm docs:check(fails:README.md is stale)Sent by Cursor Automation: Sunil On Demand Architecture Review
There was a problem hiding this comment.
Stale comment
Re-ran the architecture review on the latest head. The earlier
docs:checkfailure is fixed, but I still found 4 issues against Sunil's standards:
idp_workflow.executestill promises local validation of requiredspec.parameters[], but the implementation only parses auth-token refs fromspec.steps[]and never checks missing non-auth params before sending the request.workflow_detailsis still required only in prose/custom code whilebodySchemamarks it optional, soharness_describeexposes an incomplete execute contract.- The new IDP scope helpers synthesize placeholder scopes such as
account.org.projectwhen explicit org/project scope is requested without ids instead of failing locally with a clear missing-scope error.- Public docs are still partially out of sync with the runtime surface:
README.mdstill showsidp_scoreasget-capable and omitsidp_score_summary, and the updatedidp_scoretest plan/report document an unsupported top-levelentity_identifiercall shape forharness_list.Assumptions:
- I assumed the published
harness_listschema insrc/tools/harness-list.tsis the supported contract for top-level inputs.Checks run:
pnpm install --frozen-lockfilepnpm typecheckpnpm testpnpm build && pnpm docs:checkSent by Cursor Automation: Sunil On Demand Architecture Review
| } | ||
| } | ||
|
|
||
| const requestBody = { identifier, values }; |
There was a problem hiding this comment.
The action contract now says values are validated against required workflow_details.yaml spec.parameters, but bodyBuilder() never inspects spec.parameters[] before returning { identifier, values }. Today we only auto-fill auth-token refs from spec.steps[], so missing required non-auth params will fail downstream in IDP instead of locally. That conflicts with the repo's fail-loud preference and makes harness_describe promise behavior we don't implement.
| { | ||
| name: "workflow_details", | ||
| type: "object", | ||
| required: false, |
There was a problem hiding this comment.
workflow_details is mandatory for this action to build the request, but the structured schema marks it required: false. Because Registry.executeSpec() only uses bodySchema.fields for shared missing-field validation, harness_describe exposes an incomplete contract and callers get a late custom throw from bodyBuilder() instead of the standard schema error. Per the repo's "prefer data over prose" rule, this should be modeled structurally rather than only described in prose.
| case "project": | ||
| if (orgId && projectId) scopes = `account.${orgId}.${projectId}`; | ||
| else if (orgId) scopes = `account.${orgId}.project`; | ||
| else scopes = "account.org.project"; |
There was a problem hiding this comment.
When a caller explicitly picks scope_level=org or project without the matching IDs, this builds placeholder scopes like account.org / account.org.project and punts the error to the IDP API. Sunil's standards prefer fail-loud local validation over synthetic identifiers. Can we throw here (and in the mirrored idp_entity pathBuilder above) unless the needed org_id / project_id is present?
| | TC-idp_score-009 | Edge | List with page beyond data | `harness_list(resource_type="idp_score", page=9999)` | Returns empty list | | ||
| | TC-idp_score-010 | Edge | List with size=1 | `harness_list(resource_type="idp_score", size=1)` | Returns exactly 1 score | | ||
| | TC-idp_score-001 | List | List scorecard scores for an entity | `harness_list(resource_type="idp_score", filters={"entity_identifier":"default/Component/my-service"})` | Returns overall_score plus scorecard score items for the entity | | ||
| | TC-idp_score-002 | List | List scorecard scores using top-level entity_identifier | `harness_list(resource_type="idp_score", entity_identifier="default/Component/my-service")` | Returns overall_score plus scorecard score items for the entity | |
There was a problem hiding this comment.
harness_list's public schema only exposes resource-specific inputs via filters / params; entity_identifier is not a declared top-level input in src/tools/harness-list.ts. Documenting the top-level form here makes the test plan rely on an undocumented path, while the supported filters={"entity_identifier": ...} form is already covered by TC-idp_score-001. The test report/README should stay aligned with the published tool contract.
There was a problem hiding this comment.
Stale comment
- Important —
src/registry/toolsets/idp.ts:idp_workflow.executestill promises local validation of requiredworkflow_details.yaml.spec.parameters[], butbodyBuilder()only inspectsspec.steps[]for auth-token refs and never rejects missing non-auth required params before POSTing to/v2/workflows/execute. This is still not fail-loud, and the runtime behavior is looser than the published execute contract.- Important —
src/registry/toolsets/idp.ts:workflow_detailsis still required inbodyBuilder()/actionDescription, butbodySchema.fieldsmarks it optional.harness_describetherefore advertises the wrong contract, and the shared required-field validation path cannot catch the real missing input.- Important —
src/registry/toolsets/idp.ts: the newscope_levelhelpers still synthesize placeholders such asaccount.org,account.org.project, andaccount.<org>.projectwhen explicit org/project scope is requested without the corresponding ids. That turns a missing-scope input into an opaque remote lookup instead of a clear localorg_id/project_iderror. The same bug appears in bothidp_entity.listandidp_workflow.list.- Important —
docs/testing/idp_score/test_plan.md,docs/testing/idp_score/test_report.md,README.md: docs/runtime parity is still off. The changedidp_scoredocs still show unsupported top-levelentity_identifiercalls forharness_list, andpnpm build && pnpm docs:checkstill fails becauseREADME.mdis stale (idp_scoreis still shown as get-capable andidp_score_summaryis omitted).Previously reported issue that does appear fixed on the current head:
scorecard.listis forwardingpage/sizeagain, so the earlier pagination regression there is gone.Sent by Cursor Automation: Sunil On Demand Architecture Review
| ); | ||
| } | ||
|
|
||
| const refs = extractAuthParamRefs(yamlStr); |
There was a problem hiding this comment.
actionDescription says this call validates the required spec.parameters[], but this implementation only inspects spec.steps[] via extractAuthParamRefs(). Missing non-auth required parameters will still fall through to the remote /v2/workflows/execute call instead of failing locally. Please parse workflow_details.yaml.spec.parameters here and reject any required non-auth keys missing from values before building the request.
| { | ||
| name: "workflow_details", | ||
| type: "object", | ||
| required: false, |
There was a problem hiding this comment.
workflow_details is required above (if (!wfDetails) throw ...) and in actionDescription, but the published bodySchema still marks it optional. Because registry preflight only enforces bodySchema.required, harness_describe exposes the wrong execute contract and the shared validation path cannot catch the missing field. Mark this as required or relax the runtime requirement so metadata and behavior stay in sync.
| case "org": | ||
| scopes = orgId ? `account.${orgId}` : "account.org"; | ||
| break; | ||
| case "project": | ||
| if (orgId && projectId) scopes = `account.${orgId}.${projectId}`; | ||
| else if (orgId) scopes = `account.${orgId}.project`; | ||
| else scopes = "account.org.project"; |
There was a problem hiding this comment.
This still synthesizes placeholder scopes like account.org, account.org.project, and account.${orgId}.project when callers request explicit org/project scope without supplying the needed ids. That converts a local configuration/input error into an opaque remote lookup and violates the repo's fail-loud scope handling. Please throw here (and in the duplicated workflow pathBuilder below) when scope_level requires org_id/project_id that are unavailable.
| | TC-idp_score-009 | Edge | List with page beyond data | `harness_list(resource_type="idp_score", page=9999)` | Returns empty list | | ||
| | TC-idp_score-010 | Edge | List with size=1 | `harness_list(resource_type="idp_score", size=1)` | Returns exactly 1 score | | ||
| | TC-idp_score-001 | List | List scorecard scores for an entity | `harness_list(resource_type="idp_score", filters={"entity_identifier":"default/Component/my-service"})` | Returns overall_score plus scorecard score items for the entity | | ||
| | TC-idp_score-002 | List | List scorecard scores using top-level entity_identifier | `harness_list(resource_type="idp_score", entity_identifier="default/Component/my-service")` | Returns overall_score plus scorecard score items for the entity | |
There was a problem hiding this comment.
The public harness_list contract does not expose entity_identifier as a top-level argument; resource-specific fields are documented under filters/params in src/tools/harness-list.ts. This example therefore drifts from the runtime/tool-schema surface. Please move this case under filters (or params if that is the intended escape hatch), and make the same fix in test_report.md.
There was a problem hiding this comment.
Current HEAD is in better shape than the earlier revisions, but I still found 3 important issues in src/registry/toolsets/idp.ts against the repo standards in CLAUDE.md / AGENTS.md:
-
Important —
src/registry/toolsets/idp.ts:90-96, 383-389
idp_entity.listandidp_workflow.liststill synthesize placeholder scopes likeaccount.org,account.org.project, andaccount.${orgId}.projectwhen callers explicitly requestscope_level="org"/"project"without the required identifiers. That turns a local input/config problem into an opaque remote lookup instead of failing loudly with a clearorg_id/project_idvalidation error. -
Important —
src/registry/toolsets/idp.ts:466-499, 503-513, 531-535
idp_workflow.executestill does not validate requiredworkflow_details.yaml.spec.parameters[]before POSTing to/v2/workflows/execute. The implementation only parses auth-token refs fromspec.steps[], so missing non-auth required inputs are deferred to the remote API. That conflicts with both the published execute contract and the repo’s fail-loud validation standard. -
Important —
src/registry/toolsets/idp.ts:444-447, 518-523
workflow_detailsis still a hard runtime requirement, but the structuredbodySchemapublishes it as optional. Becauseharness_describesurfacesbodySchemaand shared validation only knows about structured fields, the real execute contract is still encoded partly in ad hoc prose/custom throws instead of accurate structured metadata.
Previously reported issues that do appear fixed on current HEAD:
src/registry/toolsets/idp.ts:187-190now forwardspage/sizeagain forscorecard.list, so the earlier pagination regression is gone.- The earlier repo-check failure no longer reproduces here:
pnpm typecheck,pnpm build && pnpm docs:check, andpnpm test -- --runInBandall pass. workflow_detailsis now present inbodySchema(earlier it was missing entirely), but that is only a partial fix because the required flag is still wrong, so I kept the contract-alignment finding above.
Sent by Cursor Automation: Sunil On Demand Architecture Review
| case "project": | ||
| if (orgId && projectId) scopes = `account.${orgId}.${projectId}`; | ||
| else if (orgId) scopes = `account.${orgId}.project`; | ||
| else scopes = "account.org.project"; |
There was a problem hiding this comment.
When callers explicitly ask for scope_level="org" / "project" without the matching ids, this builds placeholder scopes like account.org, account.org.project, and account.${orgId}.project instead of failing locally. That violates the repo’s fail-loud rule and makes the error depend on a remote lookup. Please throw a clear org_id / project_id validation error here, and make the mirrored idp_workflow.list pathBuilder below do the same.
| ); | ||
| } | ||
|
|
||
| const refs = extractAuthParamRefs(yamlStr); |
There was a problem hiding this comment.
actionDescription / bodySchema say values are validated against required workflow_details.yaml.spec.parameters[], but this code only parses auth-token refs from spec.steps[] and then POSTs whatever values contains. Missing non-auth required params still fail downstream in IDP instead of locally, which conflicts with the repo’s fail-loud validation standard. Please parse spec.parameters here and reject missing required non-auth keys before building the request.
| { | ||
| name: "workflow_details", | ||
| type: "object", | ||
| required: false, |
There was a problem hiding this comment.
workflow_details is still a hard runtime requirement (if (!wfDetails) throw ... above), but the structured schema publishes it as optional. Since harness_describe surfaces bodySchema and registry preflight only understands structured fields, this keeps the real execute contract in ad hoc prose/custom errors instead of data. Please make the structured contract and runtime requirement match, either by adding explicit pre-bodyBuilder validation for auxiliary execute inputs like workflow_details, or by removing the runtime requirement.
There was a problem hiding this comment.
Re-ran the architecture review on the current head. Local validation is green (pnpm install --frozen-lockfile, pnpm typecheck, pnpm build && pnpm docs:check, pnpm test -- --runInBand), but I still found 4 important issues against Sunil's standards:
idp_entity.listandidp_workflow.liststill synthesize placeholder scopes likeaccount.org,account.org.project, andaccount.<org>.projectwhen callers request explicit org/project scope without the needed ids. That turns a missing-scope input into an opaque remote query instead of a fail-loud localorg_id/project_iderror.idp_workflow.executestill promises local validation of requiredworkflow_details.yaml spec.parameters[], but the implementation only inspectsspec.steps[]for auth-token refs and never rejects missing non-auth required params before POSTing.workflow_detailsis still runtime-required but modeled as optional in the structured execute schema, soharness_describeadvertises the wrong contract and the shared schema-based validation path cannot catch the real missing field.- Docs/runtime parity is still off.
docs/testing/idp_score/*still documents an unsupported top-levelentity_identifiershape forharness_list, andREADME.mdstill showsidp_scoreas get-capable while omittingidp_score_summaryfrom the IDP surface.
Previously reported issue that does appear fixed on the current head: scorecard.list is forwarding pagination inputs again, and the repo checks are now passing.
Sent by Cursor Automation: Sunil On Demand Architecture Review
| case "org": | ||
| scopes = orgId ? `account.${orgId}` : "account.org"; | ||
| break; | ||
| case "project": | ||
| if (orgId && projectId) scopes = `account.${orgId}.${projectId}`; | ||
| else if (orgId) scopes = `account.${orgId}.project`; | ||
| else scopes = "account.org.project"; |
There was a problem hiding this comment.
When the caller explicitly requests scope_level="org" or scope_level="project", falling back to placeholders like account.org / account.org.project is not a valid substitute for the missing identifiers. Under this repo's Fail Loudly rule, this should reject locally with a clear org_id / project_id error instead of turning a missing-input bug into an opaque remote lookup.
The same placeholder-scope pattern is duplicated again in idp_workflow.list below.
| const refs = extractAuthParamRefs(yamlStr); | ||
| const values = { ...((b.values as Record<string, unknown> | undefined) ?? {}) }; | ||
|
|
||
| for (const ref of refs.apikeyRefs) { | ||
| if (values[ref] === undefined) values[ref] = "user.token"; | ||
| } | ||
|
|
||
| if (refs.apiKeySecretRefs.length > 0) { | ||
| const userSupplied = | ||
| (b.api_key_secret as string | undefined) ?? | ||
| (input.api_key_secret as string | undefined); | ||
| const fallback = input[CONFIG_API_KEY] as string | undefined; | ||
| const keyValue = userSupplied || fallback; | ||
| if (!keyValue) { | ||
| throw new Error( | ||
| "Missing apiKeySecret. This workflow has a step with an apiKeySecret input but no api_key_secret was provided and HARNESS_API_KEY is not configured. Pass apiKeySecret in the body.", | ||
| ); | ||
| } | ||
|
|
||
| for (const ref of refs.apiKeySecretRefs) { | ||
| if (values[ref] === undefined) values[ref] = keyValue; | ||
| } | ||
| } | ||
|
|
||
| const requestBody = { identifier, values }; |
There was a problem hiding this comment.
The execute contract and actionDescription both say values should be validated against required workflow_details.yaml spec.parameters, but this builder only extracts auth-token refs from spec.steps[] and auto-injects those values. Missing non-auth required params still flow through to /v2/workflows/execute, so the implementation is looser than the published contract and no longer fails loudly before the write.
| { | ||
| name: "workflow_details", | ||
| type: "object", | ||
| required: false, |
There was a problem hiding this comment.
workflow_details is runtime-required a few lines above, but the structured schema still marks it optional here. Because harness_describe and the shared required-field validation path both derive from bodySchema, agents see an inaccurate execute contract and the missing field is only caught by custom bodyBuilder() logic. That goes against the repo rule to prefer structured metadata over prose-only guidance.
| | TC-idp_score-009 | Edge | List with page beyond data | `harness_list(resource_type="idp_score", page=9999)` | Returns empty list | | ||
| | TC-idp_score-010 | Edge | List with size=1 | `harness_list(resource_type="idp_score", size=1)` | Returns exactly 1 score | | ||
| | TC-idp_score-001 | List | List scorecard scores for an entity | `harness_list(resource_type="idp_score", filters={"entity_identifier":"default/Component/my-service"})` | Returns overall_score plus scorecard score items for the entity | | ||
| | TC-idp_score-002 | List | List scorecard scores using top-level entity_identifier | `harness_list(resource_type="idp_score", entity_identifier="default/Component/my-service")` | Returns overall_score plus scorecard score items for the entity | |
There was a problem hiding this comment.
The shared harness_list contract does not expose resource-specific fields like entity_identifier at the top level. src/tools/harness-list.ts only merges resource-specific values from filters and params, so this example is documenting an unsupported invocation shape. The same mismatch is repeated in docs/testing/idp_score/test_report.md.


Description
get_entityType of Change
Checklist