From 9a47b5145110ba332079fc17b0bb9e2db652c08c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 10 Jul 2026 11:09:44 +0000 Subject: [PATCH 1/2] fix: require complete IDP scorecard update bodies Co-authored-by: Rohan Gupta --- src/registry/toolsets/idp.ts | 110 +++++++++++++++++++++--- tasks/todo.md | 16 ++++ tests/registry/scorecard-mutate.test.ts | 54 ++++++++++++ 3 files changed, 169 insertions(+), 11 deletions(-) diff --git a/src/registry/toolsets/idp.ts b/src/registry/toolsets/idp.ts index ffc33552..78135002 100644 --- a/src/registry/toolsets/idp.ts +++ b/src/registry/toolsets/idp.ts @@ -124,12 +124,48 @@ const requireObjectBody = (input: Record): Record; }; +const requireRecordField = ( + body: Record, + field: string, + message: string, +): Record => { + const value = body[field]; + if (!value || typeof value !== "object" || Array.isArray(value)) { + throw new Error(message); + } + return value as Record; +}; + +const requireStringField = ( + body: Record, + field: string, + message: string, +): string => { + const value = body[field]; + if (typeof value !== "string" || value.trim() === "") { + throw new Error(message); + } + return value; +}; + const buildScorecardMutateBody = (input: Record): Record => { const body = requireObjectBody(input); - if (!body.scorecard || typeof body.scorecard !== "object" || Array.isArray(body.scorecard)) { + requireRecordField( + body, + "scorecard", + 'scorecard is required. Pass body: { scorecard: {...}, checks?: [...] }. ' + + "Use harness_get(scorecard) to round-trip an existing scorecard.", + ); + return body; +}; + +const buildScorecardUpdateBody = (input: Record): Record => { + const body = buildScorecardMutateBody(input); + if (!Array.isArray(body.checks)) { throw new Error( - 'scorecard is required. Pass body: { scorecard: {...}, checks?: [...] }. ' + - "Use harness_get(scorecard) to round-trip an existing scorecard.", + "checks is required for scorecard updates because the endpoint performs a full replacement. " + + "Fetch the scorecard with harness_get and pass the complete { scorecard, checks } body. " + + "Pass checks: [] only when intentionally removing all checks.", ); } return body; @@ -137,12 +173,49 @@ const buildScorecardMutateBody = (input: Record): Record): Record => { const body = requireObjectBody(input); - if (!body.checkDetails || typeof body.checkDetails !== "object" || Array.isArray(body.checkDetails)) { - throw new Error( - 'checkDetails is required. Pass body: { checkDetails: {...} }. ' + - "Use harness_get(scorecard_check) to round-trip an existing check.", - ); + requireRecordField( + body, + "checkDetails", + 'checkDetails is required. Pass body: { checkDetails: {...} }. ' + + "Use harness_get(scorecard_check) to round-trip an existing check.", + ); + return body; +}; + +const buildScorecardCheckUpdateBody = (input: Record): Record => { + const body = buildScorecardCheckMutateBody(input); + const checkDetails = body.checkDetails as Record; + requireStringField(checkDetails, "identifier", "checkDetails.identifier is required for scorecard_check updates."); + requireStringField(checkDetails, "name", "checkDetails.name is required for scorecard_check updates."); + requireStringField(checkDetails, "description", "checkDetails.description is required for scorecard_check updates."); + const ruleStrategy = requireStringField( + checkDetails, + "ruleStrategy", + "checkDetails.ruleStrategy is required for scorecard_check updates because the endpoint performs a full replacement.", + ).toUpperCase(); + + switch (ruleStrategy) { + case "ALL_OF": + case "ANY_OF": + if (!Array.isArray(checkDetails.rules)) { + throw new Error( + "checkDetails.rules is required for ALL_OF/ANY_OF scorecard_check updates. " + + "Fetch the check with harness_get and pass the complete { checkDetails } body.", + ); + } + break; + case "ADVANCED": + requireStringField( + checkDetails, + "expression", + "checkDetails.expression is required for ADVANCED scorecard_check updates. " + + "Fetch the check with harness_get and pass the complete { checkDetails } body.", + ); + break; + default: + throw new Error("checkDetails.ruleStrategy must be one of ALL_OF, ANY_OF, or ADVANCED."); } + return body; }; @@ -180,6 +253,21 @@ const scorecardMutateBodySchema: BodySchema = { ], }; +const scorecardUpdateBodySchema: BodySchema = { + ...scorecardMutateBodySchema, + description: "ScorecardDetailsRequest — full replacement scorecard metadata and associated checks", + fields: scorecardMutateBodySchema.fields.map((field) => + field.name === "checks" + ? { + ...field, + required: true, + description: + "Required for full replacement updates: [{ identifier, weightage, custom? }]. Pass [] only to intentionally remove all checks.", + } + : field, + ), +}; + const scorecardCheckMutateBodySchema: BodySchema = { description: "CheckDetailsRequest — custom scorecard check definition", fields: [ @@ -387,12 +475,12 @@ export const idpToolset: ToolsetDefinition = { operationPolicy: { risk: "low_write", retryPolicy: "safe" }, pathParams: { scorecard_id: "scorecardIdentifier" }, skipScopeBodyInjection: true, - bodyBuilder: buildScorecardMutateBody, + bodyBuilder: buildScorecardUpdateBody, responseExtractor: passthrough, description: "Update an IDP scorecard (full replacement). Requires scorecard_id. " + "Pass the complete body from harness_get — { scorecard, checks }.", - bodySchema: scorecardMutateBodySchema, + bodySchema: scorecardUpdateBodySchema, }, }, }, @@ -463,7 +551,7 @@ export const idpToolset: ToolsetDefinition = { operationPolicy: { risk: "low_write", retryPolicy: "safe" }, pathParams: { check_id: "checkIdentifier" }, skipScopeBodyInjection: true, - bodyBuilder: buildScorecardCheckMutateBody, + bodyBuilder: buildScorecardCheckUpdateBody, responseExtractor: passthrough, description: "Update a scorecard check (full replacement). Requires check_id. " + diff --git a/tasks/todo.md b/tasks/todo.md index 6457f370..fba7c1aa 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -1,5 +1,21 @@ # Harness MCP Server — Task Tracking +## Critical Bug Investigation Automation (2026-07-10) +- [x] Baseline current branch and identify recent high-blast-radius commits +- [x] Trace recent behavioral changes through caller/downstream paths +- [x] Implement a minimal fix only if a concrete critical trigger is proven +- [ ] Run focused verification for any fix, or sanity checks for no-fix outcome +- [ ] Commit/push/open PR if fixed; otherwise report no critical bugs in Slack + +### Plan +- Treat commits after `v3.2.10` as the tight recent-change window, then include the adjacent v3.2.9..HEAD behavioral commits if the latest change depends on them. +- Prioritize DBOps path construction, Code file content scope handling, and new IDP write resources because they can break executions, file reads/writes, or mutate account/project entities. +- Require a concrete trigger scenario and caller-chain proof before editing runtime code; if confidence stays below the critical-bug bar, leave code unchanged and report the no-fix result. + +### Review +- Found a data-loss risk in the new IDP scorecard/check update paths: both operations are documented as full replacement, but the body builders accepted partial replacement bodies. A metadata-only scorecard update could omit `checks`, and a check rename/update could omit rule logic, allowing the backend to replace the resource with missing associations or logic. +- Fixed scorecard updates to require an explicit `checks` array, preserving intentional clears via `checks: []`; fixed scorecard check updates to require full `checkDetails` identity/metadata plus rule payloads based on `ruleStrategy`. + ## PR 569 Review Automation (2026-07-07) - [x] Read Slack trigger thread and confirm report context - [x] Inspect PR #569 diff, CI/review state, and affected code paths diff --git a/tests/registry/scorecard-mutate.test.ts b/tests/registry/scorecard-mutate.test.ts index 70f060f4..4df0b5f2 100644 --- a/tests/registry/scorecard-mutate.test.ts +++ b/tests/registry/scorecard-mutate.test.ts @@ -148,6 +148,19 @@ describe("scorecard mutate operations", () => { expect(call.body).toEqual(SAMPLE_SCORECARD_BODY); }); + it("update: rejects missing checks to avoid accidental full-replacement clears", async () => { + const mockRequest = vi.fn().mockResolvedValue({}); + const client = makeClient(mockRequest); + + await expect( + registry.dispatch(client, "scorecard", "update", { + scorecard_id: "production_readiness", + body: { scorecard: SAMPLE_SCORECARD_BODY.scorecard }, + }), + ).rejects.toThrow(/checks is required for scorecard updates/); + expect(mockRequest).not.toHaveBeenCalled(); + }); + it("bodyBuilder helpers match registry specs", () => { const createSpec = getOp("scorecard", "create"); const updateSpec = getOp("scorecard", "update"); @@ -155,6 +168,7 @@ describe("scorecard mutate operations", () => { expect(createSpec.skipScopeBodyInjection).toBe(true); expect(updateSpec.skipScopeBodyInjection).toBe(true); expect(createSpec.bodySchema?.fields.map((f) => f.name)).toEqual(["scorecard", "checks"]); + expect(updateSpec.bodySchema?.fields.find((f) => f.name === "checks")?.required).toBe(true); expect(createSpec.operationPolicy).toEqual({ risk: "low_write", retryPolicy: "do_not_retry" }); expect(updateSpec.operationPolicy).toEqual({ risk: "low_write", retryPolicy: "safe" }); }); @@ -213,6 +227,46 @@ describe("scorecard_check mutate operations", () => { expect(call.body).toEqual(SAMPLE_CHECK_BODY); }); + it("update: rejects rule-based checks without rules", async () => { + const mockRequest = vi.fn().mockResolvedValue({}); + const client = makeClient(mockRequest); + + await expect( + registry.dispatch(client, "scorecard_check", "update", { + check_id: "has_readme", + body: { + checkDetails: { + identifier: "has_readme", + name: "Has README", + description: "Component must have a README file", + ruleStrategy: "ALL_OF", + }, + }, + }), + ).rejects.toThrow(/checkDetails\.rules is required/); + expect(mockRequest).not.toHaveBeenCalled(); + }); + + it("update: rejects advanced checks without expression", async () => { + const mockRequest = vi.fn().mockResolvedValue({}); + const client = makeClient(mockRequest); + + await expect( + registry.dispatch(client, "scorecard_check", "update", { + check_id: "has_readme", + body: { + checkDetails: { + identifier: "has_readme", + name: "Has README", + description: "Component must have a README file", + ruleStrategy: "ADVANCED", + }, + }, + }), + ).rejects.toThrow(/checkDetails\.expression is required/); + expect(mockRequest).not.toHaveBeenCalled(); + }); + it("bodyBuilder helpers match registry specs", () => { const createSpec = getOp("scorecard_check", "create"); const updateSpec = getOp("scorecard_check", "update"); From 953789ba756cd159a1bda01d7465f928f6b4b34b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 10 Jul 2026 11:11:21 +0000 Subject: [PATCH 2/2] docs: record IDP update guard validation Co-authored-by: Rohan Gupta --- tasks/lessons.md | 5 +++++ tasks/todo.md | 5 +++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tasks/lessons.md b/tasks/lessons.md index 4f7cf3a5..8523db43 100644 --- a/tasks/lessons.md +++ b/tasks/lessons.md @@ -1,5 +1,10 @@ # Lessons Learned +## Docs Checks Need a Completed Build +- **Issue**: Running `pnpm docs:check` in parallel with `pnpm build` can fail with `ERR_MODULE_NOT_FOUND` for `build/registry/index.js` because docs generation imports compiled files. +- **Fix**: Run `pnpm build` to completion before `pnpm docs:check`; rerun docs checks after any stale/missing build failure. +- **Rule**: Do not parallelize docs checks with the build in this repo. The existing docs gotcha applies to `docs:check` as well as `docs:generate`. + ## Read Cache Signals Must Not Block Execute Paths - **Issue**: A remote pipeline `pipeline.get` response can report `cacheResponse.cacheState=STALE_CACHE` and old YAML from the read/UI cache, while pipeline execution is documented to fetch entities from Git for the selected pipeline branch. - **Fix**: Do not fail-close `harness_execute` based on `pipeline.get` cache metadata. Preserve explicit branch selection by sending `pipelineBranchName` for remote executions, and only block execution on signals from the execute path itself. diff --git a/tasks/todo.md b/tasks/todo.md index fba7c1aa..ee90b6eb 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -4,8 +4,8 @@ - [x] Baseline current branch and identify recent high-blast-radius commits - [x] Trace recent behavioral changes through caller/downstream paths - [x] Implement a minimal fix only if a concrete critical trigger is proven -- [ ] Run focused verification for any fix, or sanity checks for no-fix outcome -- [ ] Commit/push/open PR if fixed; otherwise report no critical bugs in Slack +- [x] Run focused verification for any fix, or sanity checks for no-fix outcome +- [x] Commit/push/open PR if fixed; otherwise report no critical bugs in Slack ### Plan - Treat commits after `v3.2.10` as the tight recent-change window, then include the adjacent v3.2.9..HEAD behavioral commits if the latest change depends on them. @@ -15,6 +15,7 @@ ### Review - Found a data-loss risk in the new IDP scorecard/check update paths: both operations are documented as full replacement, but the body builders accepted partial replacement bodies. A metadata-only scorecard update could omit `checks`, and a check rename/update could omit rule logic, allowing the backend to replace the resource with missing associations or logic. - Fixed scorecard updates to require an explicit `checks` array, preserving intentional clears via `checks: []`; fixed scorecard check updates to require full `checkDetails` identity/metadata plus rule payloads based on `ruleStrategy`. +- Verification passed: `pnpm exec vitest run tests/registry/scorecard-mutate.test.ts`, `pnpm typecheck`, `pnpm build`, `pnpm standards:check`, `pnpm docs:check`, and `pnpm test` (115 files / 2500 tests). An initial `pnpm docs:check` run failed because it was started in parallel before `pnpm build` had produced `build/`; rerunning after the successful build passed. ## PR 569 Review Automation (2026-07-07) - [x] Read Slack trigger thread and confirm report context