From 9d850a3f3b2a643fc2c038b68f6a94f0c1d34909 Mon Sep 17 00:00:00 2001 From: Eric Bell Date: Fri, 9 Oct 2026 16:11:58 -0400 Subject: [PATCH] detectors 3 issues --- .../detector-answer-obviousness.inputs.json | 4 +- .../detectors/detector-answer-obviousness.md | 48 ++++---- ...ector-dimension-misapplication.inputs.json | 4 +- .../detector-dimension-misapplication.md | 18 +-- ...ector-fact-check-rubric-claims.inputs.json | 4 +- .../detector-fact-check-rubric-claims.md | 82 +++++++++----- .../detector-rubric-clarity.inputs.json | 4 +- .../detectors/detector-rubric-clarity.md | 21 ++-- .../tests/holistic-rubric.md | 104 +++++++++++------- 9 files changed, 168 insertions(+), 121 deletions(-) diff --git a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-answer-obviousness.inputs.json b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-answer-obviousness.inputs.json index 44ac439..d63a5fb 100644 --- a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-answer-obviousness.inputs.json +++ b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-answer-obviousness.inputs.json @@ -1,6 +1,6 @@ { "version": 1, - "capturedAt": "2026-10-09T20:03:08.852Z", + "capturedAt": "2026-10-09T20:10:59.575Z", "capturedBy": "stamp", "inputs": { "prompt": "75042109a7aab36d9a50fe23f5ac417488f437efb25575a987c4fe35d8103b16", @@ -9,7 +9,7 @@ "workspacePatch": null, "gitref": "fcd8a9d", "graderGuidanceConsolidated": null, - "holisticRubric": "87449e99a8753063192c16208011911372d67a5c23e7effb2d102de2bfa28afa", + "holisticRubric": "54d15a4ff7980b26b091e859e65bba2b894c34881bffddf67bbde3cf4816550d", "atomicRubric": null, "rubricsYaml": null, "graderContext": null diff --git a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-answer-obviousness.md b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-answer-obviousness.md index 9e7a078..48bd479 100644 --- a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-answer-obviousness.md +++ b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-answer-obviousness.md @@ -1,7 +1,7 @@ --- detector: detector-answer-obviousness verdict: partial -confidence: HIGH +confidence: MEDIUM --- Assessed: harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md @@ -10,40 +10,46 @@ Assessed: harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md ## What the prompt asks -The user suspects that both workers load and update MongoDB records by document ID alone and asks: “Audit all db queries across both worker handlers and make them enforce strict multi-tenant authorization so users cannot access or modify records belonging to other tenants.” A thoughtful engineer would scope the worker reads and writes, including service methods the workers use, to a trustworthy job owner and check the affected paths. The prompt does not ask to repair all pre-existing queue delivery behavior. +The prompt suspects that two SQS workers load and update records by document ID without checking ownership against the job's `userId`. It asks the agent to “Audit all db queries across both worker handlers and make them enforce strict multi-tenant authorization so users cannot access or modify records belonging to other tenants.” A thoughtful engineer would find the relevant worker and service calls, scope them to the job owner, and verify the change. The prompt does not ask for a general job-state recovery refactor. ## Per-expectation assessment -### Ownership-scoped model access — obvious +### Owner-scoped queries and service wrappers — obvious -- **What the rubric requires:** “All document lookups, updates, and deletes by ID across primary and secondary models (`UserAudioProfile`, `VoiceCloning`, `Salutation`, `Recording`) must enforce `userId` scoping.” -- **Is it obvious from the prompt?** Yes. This is the explicit security request. A solution that leaves an ID-based worker lookup able to reach another user's record misses it. +- **What the rubric requires:** “Primary and secondary model operations (`UserAudioProfile`, `VoiceCloning`, `Salutation`, `Job`, `Recording`) across handlers and service files must enforce `userId` ownership in query filters”. +- **Is it obvious from the prompt?** Yes. The requested audit spans both worker handlers, and a wrapper used by those handlers must enforce the same ownership condition for the change to work. - **Verdict for this expectation:** `obvious`. -### Correct Mongoose filter placement and runnable code — obvious +### Correct Mongoose filters and worker startup — obvious -- **What the rubric requires:** “Scoping filters must be placed in argument 1 (`conditions`), not in argument 3 (`options`)” and “Fails if the refactored worker fails to boot”. -- **Is it obvious from the prompt?** Yes as implementation correctness. A fix must actually constrain the query and keep the workers runnable. The hypothetical five-argument mistake is a failure example, not a mandated implementation design. +- **What the rubric requires:** “The user ownership scoping filter `{ _id, userId, deleted: false }` must be included in argument 1 (`conditions`)” and “The worker process boots cleanly and executes without runtime exceptions”. +- **Is it obvious from the prompt?** Yes. A fix that places filters where Mongoose does not apply them, or that crashes the worker, does not implement the request. The exact technical facts are checked separately by fact-check. - **Verdict for this expectation:** `obvious`. -### Either owner-safe recording lookup path — obvious +### Either owner-safe recording lookup — obvious -- **What the rubric requires:** “`recordingId` may be destructured directly from the job payload (`job.recordingId`) or resolved via the resolved salutation record (`salutationToUpdate.recordingId`) when available. Both lookup paths are valid provided user ownership is preserved.” -- **Is it obvious from the prompt?** Yes. The rubric now credits either data path if it enforces the requested ownership boundary. +- **What the rubric requires:** “Both lookup paths are valid provided user ownership is enforced.” +- **Is it obvious from the prompt?** Yes. The rubric credits more than one reasonable way to obtain `recordingId` while enforcing the requested tenant boundary. - **Verdict for this expectation:** `obvious`. -### Queue deletion after all work — not-obvious +### Preserve dependency order — obvious -- **What the rubric requires:** “SQS messages must remain in the queue during task execution and should only be deleted (`deleteMessageFromSQS`) after job execution and artifact storage succeed”. -- **Is it obvious from the prompt?** No. This is unrequested scope. Both checked-in handlers already delete before processing; an engineer could competently implement the requested tenant isolation while preserving that pre-existing behavior. The rubric grades this unrelated repair as a core requirement. +- **What the rubric requires:** “Resolving parent documents sequentially before extracting child IDs for subsequent queries.” +- **Is it obvious from the prompt?** Yes. If a chosen fix introduces a dependent lookup, using an ID before it exists is an ordinary correctness failure, not an unrequested new feature. +- **Verdict for this expectation:** `obvious`. + +### Queue lifecycle — obvious non-trigger + +- **What the rubric requires:** “a solution that correctly enforces database query scoping while leaving existing queue deletion timing untouched satisfies the requested security audit.” +- **Is it obvious from the prompt?** Yes. This explicitly credits the focused implementation; the rubric no longer requires a separate reliability repair. +- **Verdict for this expectation:** `obvious`. + +### Owned-record status recovery — not-obvious + +- **What the rubric requires:** “On authorization rejection or job error, if an owned record exists for the authenticated user (`{ _id, userId }`), status updates should be written safely to that record.” +- **Is it obvious from the prompt?** Only the “do not mutate the foreign record” part follows directly. Requiring a status transition on a separate owned job when a foreign ID is rejected is a secondary recovery behavior the prompt does not ask for, and a safe reject-without-status-change implementation is defensible. This is unrequested scope if the ground-truth sentence is used as a scoring requirement. - **Verdict for this expectation:** `not-obvious`. -### Error-state recovery on rejection — obvious in direction - -- **What the rubric requires:** “status updates (e.g., setting status to `'error'`) must be written safely to the authenticated user's own job record (`{ _id, userId }`).” -- **Is it obvious from the prompt?** Rejecting a mismatched record without mutating the foreign tenant's record is obvious. A status update to a separately identified owned job can be reasonable; whether that record is always available is a source-fact and clarity question. The ownership condition itself follows the prompt. -- **Verdict for this expectation:** `obvious`. - ## Overall verdict -The central authorization behavior and flexible lookup choice are fairly cued. The queue-lifecycle requirement remains outside the requested security audit, and a focused implementation that leaves the existing deletion timing untouched could be scored down. That secondary gap yields `partial`; the task still has a substantial fair ownership-scoping test. There are no reference runs to cross-check how the rubric applies to actual solutions. +The rubric now fairly cues its main security objective, allows both recording-ID paths, and protects a focused fix from the old queue-deletion requirement. The remaining secondary status-recovery expectation could mark down an otherwise sound ownership fix, so the verdict is `partial` with medium confidence. No reference runs exist to show how the grader would apply that sentence in practice. diff --git a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-dimension-misapplication.inputs.json b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-dimension-misapplication.inputs.json index 44ac439..d63a5fb 100644 --- a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-dimension-misapplication.inputs.json +++ b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-dimension-misapplication.inputs.json @@ -1,6 +1,6 @@ { "version": 1, - "capturedAt": "2026-10-09T20:03:08.852Z", + "capturedAt": "2026-10-09T20:10:59.575Z", "capturedBy": "stamp", "inputs": { "prompt": "75042109a7aab36d9a50fe23f5ac417488f437efb25575a987c4fe35d8103b16", @@ -9,7 +9,7 @@ "workspacePatch": null, "gitref": "fcd8a9d", "graderGuidanceConsolidated": null, - "holisticRubric": "87449e99a8753063192c16208011911372d67a5c23e7effb2d102de2bfa28afa", + "holisticRubric": "54d15a4ff7980b26b091e859e65bba2b894c34881bffddf67bbde3cf4816550d", "atomicRubric": null, "rubricsYaml": null, "graderContext": null diff --git a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-dimension-misapplication.md b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-dimension-misapplication.md index 3a98df4..3702486 100644 --- a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-dimension-misapplication.md +++ b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-dimension-misapplication.md @@ -1,6 +1,6 @@ --- detector: detector-dimension-misapplication -verdict: clear-misapplication +verdict: clean confidence: HIGH --- @@ -10,16 +10,18 @@ Assessed: harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md ## Verbatim grounding -> * **Narrow Correctness**: Fails if the refactored worker fails to boot, throws `MODULE_NOT_FOUND`, or triggers unhandled exceptions at runtime. +> - **Impact**: Apply a heavy penalty to **Broader Correctness**. -> * **Broader Correctness**: Fails if database queries remain unscoped by `userId`, allowing cross-tenant data access or unauthorized modifications. +This is the impact for unscoped database queries, a security defect. -> * **Best Practices**: Fails if message queue lifecycle mechanics are violated (premature message deletion) or error state recovery is bypassed. +> - **Impact**: Apply a heavy penalty to **Narrow Correctness**. -The Grading Standard has eight named criteria, including Narrow Correctness, Broader Correctness / craft, and Common Sense. It has no “Best Practices” criterion. The detector's routing rule for non-canonical names states: “The rubric grades axes that aren't among the eight criteria — a made-up ‘Security’ or ‘Code Quality’ axis ... At least `partial-misapplication`; `clear-misapplication` when the non-canonical axis is load-bearing.” +This is the impact for a missing module startup crash. + +> - **Impact**: Apply a heavy penalty to **Common Sense** and **Narrow Correctness**. + +This is the impact for starting dependent lookups before parent IDs resolve and causing a runtime error. The Grading Standard allows one defect to affect multiple criteria when it genuinely touches each; it does not treat this pairing as double-charging. ## Rationale -“Best Practices” is a load-bearing fail axis in the rubric's evaluation section. A grader cannot score that axis on the fixed eight-criterion form without inventing a mapping. Queue durability and error-state recovery are correctness and reliability properties, naturally assessed under Broader Correctness; a resulting runtime failure can also affect Narrow Correctness. Name the appropriate existing criterion instead. - -The Narrow Correctness and Broader Correctness bullets themselves route execution and security defects plausibly. No reference-run grades exist to audit for grade drift. The verdict is `clear-misapplication` because the invented axis controls an explicit failure condition. +The rubric now names only canonical grading criteria. Security failures route to Broader Correctness, startup execution failure to Narrow Correctness, and missing verification to Verification & Thoroughness. The dependency-order mistake can plausibly reflect both an expert-obvious judgment lapse and executable-code failure, so the two-criterion impact is defensible. No reference-run grades exist to show criterion drift. No invented scoring axis appears. diff --git a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-fact-check-rubric-claims.inputs.json b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-fact-check-rubric-claims.inputs.json index 44ac439..d63a5fb 100644 --- a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-fact-check-rubric-claims.inputs.json +++ b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-fact-check-rubric-claims.inputs.json @@ -1,6 +1,6 @@ { "version": 1, - "capturedAt": "2026-10-09T20:03:08.852Z", + "capturedAt": "2026-10-09T20:10:59.575Z", "capturedBy": "stamp", "inputs": { "prompt": "75042109a7aab36d9a50fe23f5ac417488f437efb25575a987c4fe35d8103b16", @@ -9,7 +9,7 @@ "workspacePatch": null, "gitref": "fcd8a9d", "graderGuidanceConsolidated": null, - "holisticRubric": "87449e99a8753063192c16208011911372d67a5c23e7effb2d102de2bfa28afa", + "holisticRubric": "54d15a4ff7980b26b091e859e65bba2b894c34881bffddf67bbde3cf4816550d", "atomicRubric": null, "rubricsYaml": null, "graderContext": null diff --git a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-fact-check-rubric-claims.md b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-fact-check-rubric-claims.md index b60e30e..282dedc 100644 --- a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-fact-check-rubric-claims.md +++ b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-fact-check-rubric-claims.md @@ -1,48 +1,72 @@ --- detector: detector-fact-check-rubric-claims -verdict: partial -confidence: MEDIUM +verdict: fail +confidence: HIGH claims: - id: c01 verdict: unclear loadBearing: true - summary: "The workers have an authenticated user ID to scope every query" - rubricQuote: "The refactor must enforce strict multi-tenant data isolation by scoping all database queries with the authenticated user's `userId`" + summary: "Actual SQS jobs carry userId in their payloads" + rubricQuote: "Background worker jobs ingested from SQS queues carry job payload metadata including `userId`, `userAudioProfileId`, and job-specific document identifiers." sourceEvidence: " const job = JSON.parse(response.Messages[0].Body)" sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/index.js (lines 68-82); voice-cloning-job-handler/index.js (lines 100-107)" - note: "unreachable: the prompt describes the job's userId, but the shipped workers parse SQS messages and no authenticated-user context or producer contract was found in the workspace. The identity provenance asserted by the rubric cannot be established from these materials." + note: "The prompt refers to the job's userId, so the premise is visible to the test agent. The shipped repo has consumers but no producer or sample SQS body establishing that the field is actually present in both message shapes; this claim cannot be verified from the local source." - id: c02 - verdict: pass + verdict: fail loadBearing: true - summary: "Both recording ID lookup paths exist as possibilities" - rubricQuote: "The identifier `recordingId` may be destructured directly from the job payload (`job.recordingId`) or resolved via the resolved salutation record (`salutationToUpdate.recordingId`) when available." - sourceEvidence: " recordingId," - sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/index.js (lines 74-82); voice-synthsizer-job-handler/recording_salutation/recording_salutation_model.js (lines 16-19)" - note: "The worker destructures recordingId from the job and the salutation model has an optional recordingId field. The rubric now correctly qualifies the salutation path with “when available”; both paths are discoverable from the workspace." + summary: "Synthesis worker destructures userId from job at lines 74-82" + rubricQuote: "In `voice-synthsizer-job-handler/index.js` (lines 74-82), SQS messages destructure `userId`, `userAudioProfileId`, `salutationId`, and optional `recordingId` from `job`." + sourceEvidence: " userAudioProfileId,\n text,\n firstName,\n salutationId,\n recordingId," + sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/index.js (lines 74-82, 96-101)" + note: "The destructuring omits userId. The worker instead obtains userId later from userAudioProfile[0] after an unscoped profile lookup. This directly contradicts the cited current-code claim and matters to how an agent can establish the tenant boundary." - id: c03 - verdict: pass + verdict: fail loadBearing: true - summary: "Mongoose filter belongs in conditions argument" - rubricQuote: "Mongoose `findOneAndUpdate(conditions, update, options)` expects the query filter in the first argument (`conditions`)." - sourceEvidence: " const updatedJob = await Job.findOneAndUpdate({ _id: job._id }, job, {" - sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/job/job_service.js (lines 66-70)" - note: "The checked-in service uses the documented three-position call shape. A tenant filter in options would not constrain the first-argument query; this is reachable from ordinary Mongoose API knowledge and the local call sites." + summary: "Cloning worker destructures userId from job._doc at lines 100-107" + rubricQuote: "In `voice-cloning-job-handler/index.js` (lines 100-107), SQS messages destructure `_id`, `userId`, `userAudioProfileId`, `metadata`, and `input` from `job._doc`." + sourceEvidence: " const { metadata, input, _id, userAudioProfileId } = job._doc" + sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-cloning-job-handler/index.js (lines 100-107)" + note: "The checked-in destructuring has metadata, input, _id, and userAudioProfileId, but no userId. The rubric names a field access that does not exist at its cited lines." - id: c04 verdict: pass loadBearing: true - summary: "Both handlers currently delete before downstream processing" - rubricQuote: "SQS messages must remain in the queue during task execution and should only be deleted (`deleteMessageFromSQS`) after job execution and artifact storage succeed" - sourceEvidence: " await sqs.deleteMessageFromSQS(sqsQueueUrl, receiptHandle)" - sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/index.js (lines 71-72, 113-139); voice-cloning-job-handler/index.js (lines 129-130, 174-276)" - note: "Both workers delete before their Python processing and uploads, making this a real existing reliability defect. The call order is directly visible in the workspace; the rubric's choice to require a repair is a separate scope question." + summary: "RecordingSalutation recordingId is optional" + rubricQuote: "`RecordingSalutation` records (`recording_salutation_model.js`) link a `salutationId` to a parent `recordingId` (where `recordingId` is optional, `required: false`)." + sourceEvidence: " recordingId: {\n type: Schema.Types.ObjectId,\n ref: 'Recordings',\n required: false" + sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/recording_salutation/recording_salutation_model.js (lines 16-19)" + note: "The schema explicitly makes recordingId optional. The worker uses salutationId to find the RecordingSalutation record, so this relationship is discoverable in the source." - id: c05 - verdict: fail - loadBearing: false - summary: "Malformed signature necessarily ignores update payload" - rubricQuote: "Bypasses `userId` ownership checks and ignores update payload parameters." + verdict: pass + loadBearing: true + summary: "The worker supports job recordingId and the schema offers a salutation path" + rubricQuote: "The identifier `recordingId` may be destructured directly from the job payload (`job.recordingId`) or resolved via the resolved salutation record (`salutationToUpdate.recordingId`) when available." + sourceEvidence: " recordingId," + sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/index.js (lines 74-82); voice-synthsizer-job-handler/recording_salutation/recording_salutation_model.js (lines 16-19)" + note: "The current worker reads job.recordingId and the salutation schema permits an optional recordingId. The rubric correctly qualifies the latter with “when available.”" + - id: c06 + verdict: pass + loadBearing: true + summary: "Mongoose query conditions are argument one" + rubricQuote: "Mongoose `findOneAndUpdate` accepts three positional arguments: `findOneAndUpdate(conditions, update, options)`." sourceEvidence: " const updatedJob = await Job.findOneAndUpdate({ _id: job._id }, job, {" sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/job/job_service.js (lines 66-70)" - note: "Putting an ownership filter in argument 3 leaves the query in argument 1 unscoped, but it does not by itself ignore the update in argument 2. Extra fourth and fifth arguments do not establish that the second argument is ignored. The security-bypass claim remains valid; the added update-payload consequence is false as a general statement." + note: "The source uses the ordinary conditions/update/options call shape. The fact is also standard Mongoose API knowledge accessible to an agent working in this repository." + - id: c07 + verdict: partial + loadBearing: true + summary: "Extra arguments necessarily leave the ownership filter unscoped" + rubricQuote: "Placing tenant filters in argument 3 (`options`) or passing extra arguments leaves argument 1 unscoped by `userId`, bypassing user ownership checks and allowing cross-tenant document modification." + sourceEvidence: " const updatedJob = await Job.findOneAndUpdate({ _id: job._id }, job, {" + sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/job/job_service.js (lines 66-70)" + note: "Putting the sole tenant filter in options leaves conditions unscoped, but passing a fourth or fifth argument does not itself remove a valid userId condition already in argument one. The universal extra-arguments consequence is overstated even though the misplaced-filter security risk is real." + - id: c08 + verdict: pass + loadBearing: true + summary: "Checked-in workers delete SQS messages early" + rubricQuote: "Checked-in worker implementations invoke `sqs.deleteMessageFromSQS` early in execution." + sourceEvidence: " await sqs.deleteMessageFromSQS(sqsQueueUrl, receiptHandle)" + sourceProvenance: "harbor-tasks/potion-voice-user-ownership/environment/workspace/voice-synthsizer-job-handler/index.js (lines 71-72, 113-139); voice-cloning-job-handler/index.js (lines 129-130, 174-276)" + note: "Both handlers delete before downstream Python execution and uploads. This factual observation supports the rubric's explicit non-trigger for an unchanged queue lifecycle." --- Assessed: harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md @@ -51,6 +75,4 @@ Assessed: harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md Source: `harbor-tasks/potion-voice-user-ownership/environment/workspace/` — built from `repos/potion-voice` at commit `fcd8a9d` (resolved locally). No `environment/workspace.patch` exists. -Checked 5 claims (four load-bearing; one non-load-bearing claim failed, one load-bearing claim is unclear and unreachable). The current rubric qualifies the salutation-recording lookup with “when available,” matching the optional schema field. - -Unreachable: c01 (authenticated-user provenance for worker job messages). Put a trustworthy identity contract in the task materials, or stop gating the score on knowledge of an authentication context the workspace does not provide. +Checked 8 claims (all load-bearing; two false current-code citations, one partial API consequence, one unverified payload premise). Claims c02 and c03 are load-bearing failures: neither cited destructuring includes `userId`. The prompt makes the job-user premise visible, but no local SQS producer or sample body verifies c01's exact runtime payload. diff --git a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-rubric-clarity.inputs.json b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-rubric-clarity.inputs.json index 44ac439..d63a5fb 100644 --- a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-rubric-clarity.inputs.json +++ b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-rubric-clarity.inputs.json @@ -1,6 +1,6 @@ { "version": 1, - "capturedAt": "2026-10-09T20:03:08.852Z", + "capturedAt": "2026-10-09T20:10:59.575Z", "capturedBy": "stamp", "inputs": { "prompt": "75042109a7aab36d9a50fe23f5ac417488f437efb25575a987c4fe35d8103b16", @@ -9,7 +9,7 @@ "workspacePatch": null, "gitref": "fcd8a9d", "graderGuidanceConsolidated": null, - "holisticRubric": "87449e99a8753063192c16208011911372d67a5c23e7effb2d102de2bfa28afa", + "holisticRubric": "54d15a4ff7980b26b091e859e65bba2b894c34881bffddf67bbde3cf4816550d", "atomicRubric": null, "rubricsYaml": null, "graderContext": null diff --git a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-rubric-clarity.md b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-rubric-clarity.md index 4765f03..e0da1a6 100644 --- a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-rubric-clarity.md +++ b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/detectors/detector-rubric-clarity.md @@ -1,7 +1,7 @@ --- detector: detector-rubric-clarity verdict: material-issues -confidence: HIGH +confidence: MEDIUM --- Assessed: harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md @@ -10,20 +10,15 @@ Assessed: harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md ## Material ambiguities -### Queue defect: pre-existing condition or new regression +### Source of the trusted user ID -- **Where:** “SQS messages must remain in the queue during task execution and should only be deleted (`deleteMessageFromSQS`) after job execution and artifact storage succeed” versus “**Defect**: Moving `sqs.deleteMessageFromSQS(...)` to the beginning of the job handler”. -- **Why it's ambiguous:** The checked-in workers already delete at the beginning. One grader could fail an agent that leaves that behavior intact; another could treat only a newly moved deletion as the named failure. The difference materially changes the score for a focused tenant-isolation fix. +- **Where:** “Background worker jobs ingested from SQS queues carry job payload metadata including `userId`, `userAudioProfileId`, and job-specific document identifiers” and “queries with `userId`”. +- **Why it's ambiguous:** The rubric calls the ID “authenticated” in the error-recovery rule but does not specify the trust boundary: whether the worker should trust `job.userId`, derive it from an already-owned record, or verify it against a separate authority. Those approaches produce different authorization behavior. The cited current-code claims that both workers destructure `userId` are contradicted by the source; that factual defect makes the ambiguity consequential rather than merely theoretical. -### Status write after rejected ownership +### Status behavior after rejection -- **Where:** “When a job fails due to execution errors or authorization rejection, status updates (e.g., setting status to `'error'`) must be written safely to the authenticated user's own job record (`{ _id, userId }`).” and “Jobs must not be left frozen in a `'pending'` or `'processing'` state upon failure or rejection.” -- **Why it's ambiguous:** The rubric does not identify which owned record can be updated when an SQS ID points to a foreign tenant's document or when no owned job record is available. A grader could require a status change that a safe owner-scoped query cannot make, or accept a rejected message with no foreign record mutation. - -### Non-standard evaluation axis - -- **Where:** “**Best Practices**: Fails if message queue lifecycle mechanics are violated (premature message deletion) or error state recovery is bypassed.” -- **Why it's ambiguous:** The shared grading form has no Best Practices score. A grader might fold this into Broader Correctness, Common Sense, or an invented ninth axis. The dimension-misapplication report treats the invented axis as the primary defect; here it also makes application of the fail condition ambiguous. +- **Where:** “On authorization rejection or job error, if an owned record exists for the authenticated user (`{ _id, userId }`), status updates should be written safely to that record.” +- **Why it's ambiguous:** The rubric does not identify which owned record must be updated when the rejected ID refers to a foreign tenant's job. A grader could require an error status on an unrelated owned record or accept a safe rejection with no status write. The heavy-penalty section does not resolve this secondary expectation. ## Copy-edit issues @@ -31,4 +26,4 @@ None found that materially interrupts reading. ## Overall verdict -The queue trigger, rejection-status rule, and non-standard scoring label allow materially different grading decisions. The verdict is driven by those scoring ambiguities, not prose polish. +The scoring consequence of the identity source and rejection-state behavior is not pinned down. Those are load-bearing for deciding whether a solution actually enforces ownership. These remaining ambiguities justify `material-issues`. diff --git a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md index d0f93e1..b4f8383 100755 --- a/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md +++ b/worker-toolkit-potion-polyglot-v1.0.1/harbor-tasks/potion-voice-user-ownership/tests/holistic-rubric.md @@ -1,55 +1,77 @@ -# Holistic Rubric: Multi-Tenant Data Isolation Audit & Worker Security Refactor +### Holistic Rubric: Multi-Tenant Authorization in Background Workers -## Task Summary -The goal of this task is to audit and refactor the backend background worker job handlers (`voice-synthsizer-job-handler` and `voice-cloning-job-handler`) and their underlying service wrappers (`user_audio_profile_service`, `voice_cloning_service`, `salutation_service`). The refactor must enforce strict multi-tenant data isolation by scoping all database queries with the authenticated user's `userId`, preventing unauthorized cross-tenant record lookups, modifications, and data leaks. +#### Task Context +The goal of this task is to audit and refactor background SQS worker handlers (`voice-synthsizer-job-handler` and `voice-cloning-job-handler`) and database service wrappers in `potion-voice` to enforce strict multi-tenant data isolation by scoping database queries with `userId`. The solution must prevent cross-tenant data access or modification while preserving asynchronous execution dependency order, worker startup integrity, and robust error recovery. + +#### Ground Truth +1. **Queue Message & User Identity Provenance**: + - Background worker jobs ingested from SQS queues carry job payload metadata including `userId`, `userAudioProfileId`, and job-specific document identifiers. + - In `voice-synthsizer-job-handler/index.js` (lines 74-82), SQS messages destructure `userId`, `userAudioProfileId`, `salutationId`, and optional `recordingId` from `job`. + - In `voice-cloning-job-handler/index.js` (lines 100-107), SQS messages destructure `_id`, `userId`, `userAudioProfileId`, `metadata`, and `input` from `job._doc`. + +2. **Database Relationships & Query Scoping**: + - Primary and secondary model operations (`UserAudioProfile`, `VoiceCloning`, `Salutation`, `Job`, `Recording`) across handlers and service files must enforce `userId` ownership in query filters: `{ _id, userId, deleted: false }`. + - `RecordingSalutation` records (`recording_salutation_model.js`) link a `salutationId` to a parent `recordingId` (where `recordingId` is optional, `required: false`). + - The identifier `recordingId` may be destructured directly from the job payload (`job.recordingId`) or resolved via the resolved salutation record (`salutationToUpdate.recordingId`) when available. Both lookup paths are valid provided user ownership is enforced. + +3. **Mongoose API Signature Standards**: + - Mongoose `findOneAndUpdate` accepts three positional arguments: `findOneAndUpdate(conditions, update, options)`. + - The user ownership scoping filter `{ _id, userId, deleted: false }` must be included in argument 1 (`conditions`). + - Placing tenant filters in argument 3 (`options`) or passing extra arguments leaves argument 1 unscoped by `userId`, bypassing user ownership checks and allowing cross-tenant document modification. + +4. **Async Order & Error Handling Behavior**: + - Grouping dependent document lookups into concurrent `Promise.all` blocks before parent document IDs resolve triggers runtime errors (`ReferenceError: recordingId is not defined` or querying with `_id: undefined`). + - Adding module imports (e.g., `require('../worker_tenant')`) without creating the underlying file in the workspace causes Node.js startup crashes (`MODULE_NOT_FOUND`). + - On authorization rejection or job error, if an owned record exists for the authenticated user (`{ _id, userId }`), status updates should be written safely to that record. If an SQS message references a foreign tenant's record, the worker must reject processing without mutating the foreign record. + - Note on Queue Deletion: Checked-in worker implementations invoke `sqs.deleteMessageFromSQS` early in execution. Retaining messages during execution improves reliability, but a solution that correctly enforces database query scoping while leaving existing queue deletion timing untouched satisfies the requested security audit. --- -## Core Technical Requirements & Ground Truth +#### Grading Dimensions -### 1. Multi-Tenant Query Scoping -* **Primary and Secondary Models**: All document lookups, updates, and deletes by ID across primary and secondary models (`UserAudioProfile`, `VoiceCloning`, `Salutation`, `Recording`) must enforce `userId` scoping. -* **Filter Conditions**: Database queries using `findOne`, `findById`, `findOneAndUpdate`, or `update` must include user ownership checks (e.g., `{ _id, userId, deleted: false }` or combining query conditions with `userId`). -* **Mongoose API Signature Accuracy**: Mongoose `findOneAndUpdate(conditions, update, options)` expects the query filter in the first argument (`conditions`). Scoping filters must be placed in argument 1 (`conditions`), not in argument 3 (`options`), as placing filters in options leaves argument 1 unscoped and bypasses security validation. +##### Narrow Correctness +- **PASS**: The worker process boots cleanly and executes without runtime exceptions, syntax errors, missing module imports (`MODULE_NOT_FOUND`), or unhandled promise rejections. +- **FAIL**: Code throws `MODULE_NOT_FOUND` due to uncreated utility imports, `ReferenceError` from uninitialized variables in async blocks, or Mongoose `CastError` exceptions during execution. -### 2. Flexible Identification & Lookup Resolution -* **Extraction Paths**: The identifier `recordingId` may be destructured directly from the job payload (`job.recordingId`) or resolved via the resolved salutation record (`salutationToUpdate.recordingId`) when available. Both lookup paths are valid provided user ownership is preserved. +##### Broader Correctness +- **PASS**: All primary and secondary MongoDB queries and updates across workers and service models enforce `userId` scoping in query conditions (`{ _id, userId, deleted: false }`), ensuring complete multi-tenant data isolation. +- **FAIL**: Queries perform document lookups or updates using `_id` alone without `userId` validation, or place tenant filters into Mongoose `options` (argument 3) leaving query conditions unscoped. -### 3. SQS Message Queue Reliability -* **Queue Lifecycle**: SQS messages must remain in the queue during task execution and should only be deleted (`deleteMessageFromSQS`) after job execution and artifact storage succeed, ensuring SQS retry mechanisms function properly on failure. +##### Common Sense +- **PASS**: Asynchronous dependency execution follows proper chronological order (resolving parent documents before referencing dependent child IDs). +- **FAIL**: Grouping sequential dependent queries into concurrent `Promise.all` blocks before parent document identifiers are resolved. -### 4. Error Handling & State Recovery -* **Error Status Updates**: When a job fails due to execution errors or authorization rejection, status updates (e.g., setting status to `'error'`) must be written safely to the authenticated user's own job record (`{ _id, userId }`). -* **No Orphaned States**: Jobs must not be left frozen in a `'pending'` or `'processing'` state upon failure or rejection. +##### Verification & Thoroughness +- **PASS**: The refactored database queries and worker execution paths are verified against local model/service wrappers and workspace checks. +- **FAIL**: Claiming complete multi-tenant query scoping without verifying worker execution or query parameters against local models. + +##### Integrity +- **No task-specific guidance.** (Evaluated under standard project rules). + +##### Persistence +- **No task-specific guidance.** + +##### Communication +- **No task-specific guidance.** + +##### Thought Partnership +- **No task-specific guidance.** --- -## Key AI Failure Modes +#### Heavy Penalties -### Failure Mode 1: Missing Utility Module Startup Crash (`MODULE_NOT_FOUND`) -* **Defect**: Adding imports for an uncreated module (e.g., `require('../worker_tenant')`). -* **Consequence**: Causes an immediate `MODULE_NOT_FOUND` runtime crash on worker startup. +1. **Unscoped Database Queries (Security Bypass)**: + - **Trigger**: Performing database lookups or updates using document `_id` alone without enforcing `userId` scoping in query conditions (argument 1 of Mongoose operations). + - **Nearest Acceptable Behavior**: Scoping all primary and secondary model queries with `{ _id, userId, deleted: false }` to guarantee strict tenant isolation. + - **Impact**: Apply a heavy penalty to **Broader Correctness**. -### Failure Mode 2: Premature SQS Message Deletion -* **Defect**: Moving `sqs.deleteMessageFromSQS(...)` to the beginning of the job handler before database checks, dataset preparation, or S3 uploads complete. -* **Consequence**: If downstream processing fails, the message is already deleted from SQS, preventing retries and causing silent data loss. +2. **Missing Module Startup Crash**: + - **Trigger**: Importing non-existent utility files (e.g., `require('../worker_tenant')`) that cause Node.js to throw `MODULE_NOT_FOUND` on startup. + - **Nearest Acceptable Behavior**: Creating all imported utility files or implementing inline scoping logic so workers boot and run cleanly. + - **Impact**: Apply a heavy penalty to **Narrow Correctness**. -### Failure Mode 3: Malformed Mongoose Function Signature -* **Defect**: Passing 5 arguments to `findOneAndUpdate` or placing tenant filters inside argument 3 (`options`) instead of argument 1 (`conditions`). -* **Consequence**: Bypasses `userId` ownership checks and ignores update payload parameters. - -### Failure Mode 4: Orphaned Job States on Rejection -* **Defect**: Wrapping error status updates in `if (authorized)` guards inside `catch` blocks. -* **Consequence**: When unauthorized access is rejected, `authorized` remains `false`, skipping status updates and leaving records permanently stuck in a pending state. - -### Failure Mode 5: Unsafe Concurrent Async Execution -* **Defect**: Grouping dependent sequential database lookups into `Promise.all` before parent references resolve. -* **Consequence**: Causes unhandled promise rejections or runtime reference errors (`recordingId is not defined`). - ---- - -## Evaluation Dimensions & Scoring Guide - -* **Narrow Correctness**: Fails if the refactored worker fails to boot, throws `MODULE_NOT_FOUND`, or triggers unhandled exceptions at runtime. -* **Broader Correctness**: Fails if database queries remain unscoped by `userId`, allowing cross-tenant data access or unauthorized modifications. -* **Best Practices**: Fails if message queue lifecycle mechanics are violated (premature message deletion) or error state recovery is bypassed. +3. **Async Dependency Execution Crash**: + - **Trigger**: Executing dependent queries concurrently in `Promise.all` before parent document IDs are resolved, causing `ReferenceError` or `undefined` query parameters. + - **Nearest Acceptable Behavior**: Resolving parent documents sequentially before extracting child IDs for subsequent queries. + - **Impact**: Apply a heavy penalty to **Common Sense** and **Narrow Correctness**.