Fix IDP full-replacement update safeguards

src/registry/toolsets/idp.ts

@@ -124,25 +124,98 @@ const requireObjectBody = (input: Record<string, unknown>): Record<string, unkno
   return body as Record<string, unknown>;
 };
 
+const requireRecordField = (
+  body: Record<string, unknown>,
+  field: string,
+  message: string,
+): Record<string, unknown> => {
+  const value = body[field];
+  if (!value || typeof value !== "object" || Array.isArray(value)) {
+    throw new Error(message);
+  }
+  return value as Record<string, unknown>;
+};
+
+const requireStringField = (
+  body: Record<string, unknown>,
+  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<string, unknown>): Record<string, unknown> => {
   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<string, unknown>): Record<string, unknown> => {
+  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;
 };
 
 const buildScorecardCheckMutateBody = (input: Record<string, unknown>): Record<string, unknown> => {
   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<string, unknown>): Record<string, unknown> => {
+  const body = buildScorecardCheckMutateBody(input);
+  const checkDetails = body.checkDetails as Record<string, unknown>;
+  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. " +

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.

tasks/todo.md

@@ -1,5 +1,22 @@
 # 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
+- [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.
+- 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`.
+- 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
 - [x] Inspect PR #569 diff, CI/review state, and affected code paths

tests/registry/scorecard-mutate.test.ts

@@ -148,13 +148,27 @@ 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");
 
     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");