Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This PR will trigger a minor release when merged. |
Review Action PlanPR: #3317 Implement1. Add enabled-success and active-match generation testsFiles: Add a controller happy-path test for enabled reconciliation and a storage regression proving an existing active prompt is updated with the current Tests: targeted controller and storage tests. 2. Emit an independently countable refusal metricFiles: Emit a count-valued refusal event metric rather than relying on Tests: metric assertions for success and refusal. 3. Correct refusal and threshold loggingFiles: Log completed reconciliation only after the refusal branch, use a distinct refusal event, and warn when an invalid configured maximum fraction falls back to Tests: log-event assertions and invalid-config fallback coverage. 4. Clarify rollback and rollout documentationFiles: Document that rollback reactivates rows removed by a reconciliation run but does not restore prior mutable field values, that the DRS flag is environment-wide rather than source-specific, and that safe backlog cleanup is performed by controlled full-source reruns. Tests: documentation-only. Pushback1. Legacy write compatibility is already fixedSources: Follow-up commit 2. A 409 after successful writes is the designed refusal contractSources: The atomic RPC refuses only stale-row expiry; the current generation has already been safely upserted. Returning counts with HTTP 409 is the issue's hard-refusal behavior, not a partial write bug. 3. Drop automated status comments as non-review noiseSources: Patch coverage is 98.62% and semantic-release reports release type; neither comment identifies a correctness defect. Deferred1. Configure an operational alarm on the refusal metricReason: Alarm ownership and thresholds live in deployment/infrastructure configuration, not this API repository. 2. Run the real DRS-through-API-through-Postgres scenarioReason: Unit and repository integration tests cannot prove the deployed cross-repo envelope contract. 3. Assign backfill and customer-communication ownersReason: This is a cross-team operational decision, not an API implementation detail. Reply go ahead to begin implementation. |
There was a problem hiding this comment.
Hey @dzehnder,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - six addressable issues across input validation, API contract, and test coverage.
Complexity: HIGH - large diff, API surface change.
Changes: Adds generation-aware prompt writes with atomic reconciliation, expired-prompt filtering, and a fraction guard to the v2 brand prompts API (7 files).
Note: Recommend a human read before merge - this change modifies a shared contract (docs/openapi/*) and the conventions reviewer flagged possible spec divergence (heuristic). The bot review is a complement to, not a replacement for, a human read here.
Must fix before merge
- [Important] Source validation runs after prompt writes - unregistered source values persist before
assertPermittedSourceexecutes -src/controllers/brands.js:727(details inline) - [Important] OpenAPI spec missing formal 409 response for reconciliation refusal -
docs/openapi/prompts-v2-api.yaml:257(details inline) - [Important] Description references
ignoredstatus but V2Prompt enum is [active, pending, deleted, expired] -docs/openapi/prompts-v2-api.yaml:151(details inline) - [Important] No server-side guard prevents callers from writing
status: 'expired'directly, bypassing reconciliation -src/support/prompts-storage.js:1082(details inline) - [Important] Missing controller test for the enabled-reconciliation happy path (RPC returns
refused=false, 201 with reconciliation counts) - the primary feature path has no end-to-end controller coverage - [Important]
findPromptsBlockingRegionRemovalwithexcludeExpired: trueis untested - the new.not('status', 'in', ...)branch has no storage-level coverage
Non-blocking (7): minor issues and suggestions
- nit:
promptGenerationMaxExpireFractionsilently falls back to 0.9 on misconfigured env var - add alog.warnso operators can detect it -src/controllers/brands.js:135 - nit: Pending prompt generation-touch inflates the
updatedcount with different semantics than content updates - worth a code comment or a separatetouchedcount - nit: Reconciliation object shape varies across success/disabled/refused (
{refused, candidateCount, ...}vs{skipped, reason}) - astatusdiscriminator field would help typed consumers - suggestion: Extract
context.env || envinto a singleresolvedEnvbinding at the top ofcreatePromptsByBrandto reduce the three repeated lookups - nit: Structured log field
run_idis inconsistent with the API fieldgenerationIdand DB columngeneration_id- considergeneration_idin logs for grep-ability - suggestion: The upsert write path intentionally omits
withMissingGenerationIdFallback(deploy-order dependency on data-service schema) - add a code comment explaining this conscious asymmetry with the read paths - nit: The 409 refusal test asserts
log.warnmessage string but not the structured fields (org, brand, source, run_id) that operators use for incident triage -test/controllers/brands.test.js:1224
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 10m 7s | Cost: $11.66 | Commit: 3db1f72ec1bd0a7c15249af3a91e2f9c7adf73de
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Not implementing: this automated coverage status does not identify a correctness defect, and the review batch adds direct regressions for the requested lifecycle paths. |
Not implementing: this is release-status information, not a requested code change. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1. Abstract
Adds generation-aware prompt writes and reads to the v2 brand prompts API, including atomic stale-generation reconciliation, expired filtering, refusal responses, and operational telemetry.
2. Reasoning
Generated prompt pipelines need to retire prompts that disappear from a later full source run without exposing destructive status control to public callers. adobe-rnd/llmo-data-retrieval-service#3267 requires the API layer to validate the service-only generation contract, invoke the guarded database reconciliation, and keep legacy prompt writes compatible.
3. High-level overview of the changes
The existing bare-array write contract remains unchanged, while authenticated service callers can submit a strict full-generation envelope.
expiredremains server-owned and is not accepted as a public input status.4. Required information
6. Affected / used mysticat-workspace projects
mysticat-data-service: provides the generation column, expired status, and atomic reconciliation RPC consumed by this PR.llmo-data-retrieval-service: will send strict full-generation envelopes and consume nested reconciliation counts.7. Additional information outside the code
OpenAPI rendering reports only the repository's existing warning set. A real API-to-RPC integration run is deferred because the API integration environment currently pins a released data-service image that does not yet contain adobe/mysticat-data-service#1068.
8. Test plan
ExpiredPromptCountmetric.status=expiredremains available.9. Deployment & merge order
Change Management
🤖 Generated with review-kit