Skip to content

feat: reconcile generated prompt generations - #3317

Open
dzehnder wants to merge 4 commits into
mainfrom
issue-3267
Open

dzehnder wants to merge 4 commits into
mainfrom
issue-3267

Conversation

@dzehnder

@dzehnder dzehnder commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

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.

  • Generation writes stamp a UUID and canonical source on every prompt, reactivate re-emitted expired/deleted prompts, and reconcile only after all current prompts are persisted.
  • Reconciliation delegates to the atomic writer RPC and returns nested counts; threshold refusals return HTTP 409 without expiring candidates.
  • Expired prompts are hidden by default when the feature is enabled but remain queryable explicitly for audit and rollback.
  • expired remains server-owned and is not accepted as a public input status.
  • Region-removal checks and prompt statistics continue to operate on live statuses only.
  • CloudWatch metrics, structured logs, OpenAPI definitions, and the rollout/rollback runbook cover the new lifecycle.

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

  • Local: exercise legacy bare-array writes and generation-envelope writes; verify source validation, generation stamping, reactivation, default expired filtering, explicit expired queries, threshold refusal, and nested reconciliation counts.
  • Dev: after adobe/mysticat-data-service#1068 deploys, submit a generation envelope through the API and confirm the writer RPC response, HTTP 409 refusal path, logs, and ExpiredPromptCount metric.
  • Stage: deploy with filtering/reconciliation disabled, enable for a controlled brand/source, and verify stale prompts are hidden while explicit status=expired remains available.
  • Prod: deploy before DRS with the feature disabled; enable only after the coordinated DRS release is deployed, with the documented kill switch ready.

9. Deployment & merge order

  • adobe/mysticat-data-service#1068 — depends-on; the schema and writer RPC must be released before this API version.
  • adobe-rnd/llmo-data-retrieval-service#3275 — blocks; consumes this generation-envelope and reconciliation-response contract.
  • Ordered sequence: adobe/mysticat-data-service#1068 -> this PR -> adobe-rnd/llmo-data-retrieval-service#3275 -> enable the feature flag.

Change Management

cm-assessment: v1
changeType: standard
impact: unnoticeable
risk: minor
scope: multi-repo
relatedPRs: ["adobe/mysticat-data-service#1068", "adobe-rnd/llmo-data-retrieval-service#3275"]
rationale: "Adds the gated API contract for a coordinated data/API/DRS prompt-lifecycle rollout; it is additive, preserves legacy writes, and deploys after the database foundation."
recommendations: "Deploy data service, then API, then DRS; enable the feature flag only after contract verification and monitor reconciliation refusals and expired counts."
backout: "Disable prompt generation reconciliation, redeploy previous service releases, and leave additive database fields unused."

🤖 Generated with review-kit

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.11912% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/support/prompts-storage.js 97.87% 4 Missing ⚠️
src/controllers/brands.js 98.47% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

This PR will trigger a minor release when merged.

@dzehnder
dzehnder deployed to dev-branches September 18, 2026 10:50 — with GitHub Actions Active
@dzehnder

Copy link
Copy Markdown
Contributor Author

Review Action Plan

PR: #3317
Generated: 2026-09-18
Findings: 10 total — 4 implement / 3 pushback / 3 deferred

Implement

1. Add enabled-success and active-match generation tests

Files: test/controllers/brands.test.js, test/support/prompts-storage.test.js
Source: [] (ref_type: local_report)

Add a controller happy-path test for enabled reconciliation and a storage regression proving an existing active prompt is updated with the current generation_id. Current code does both, but only insert, pending, reactivation, and refusal paths are directly asserted.

Tests: targeted controller and storage tests.

2. Emit an independently countable refusal metric

Files: src/controllers/brands.js, test/controllers/brands.test.js
Source: [] (ref_type: local_report)

Emit a count-valued refusal event metric rather than relying on ExpiredPromptCount=0 with an Outcome=refused dimension. Assert the metric on 409 and the normal expiry metric on success.

Tests: metric assertions for success and refusal.

3. Correct refusal and threshold logging

Files: src/controllers/brands.js, test/controllers/brands.test.js
Source: [] (ref_type: local_report)

Log completed reconciliation only after the refusal branch, use a distinct refusal event, and warn when an invalid configured maximum fraction falls back to 0.9.

Tests: log-event assertions and invalid-config fallback coverage.

4. Clarify rollback and rollout documentation

Files: docs/serenity.md
Source: [] (ref_type: local_report)

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.

Pushback

1. Legacy write compatibility is already fixed

Sources: [] (ref_type: local_report)

Follow-up commit 3db1f72ec omits generation_id from legacy writes and retries legacy reads only for the precise missing-column error. Generation-envelope writes intentionally require the data-service migration and fail if deployment order is violated.

2. A 409 after successful writes is the designed refusal contract

Sources: [] (ref_type: local_report)

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 noise

Sources: [5728707639, 5728785293] (ref_type: issue_comment)

Patch coverage is 98.62% and semantic-release reports release type; neither comment identifies a correctness defect.

Deferred

1. Configure an operational alarm on the refusal metric

Reason: Alarm ownership and thresholds live in deployment/infrastructure configuration, not this API repository.
Target: coordinated production rollout for adobe-rnd/llmo-data-retrieval-service#3267.

2. Run the real DRS-through-API-through-Postgres scenario

Reason: Unit and repository integration tests cannot prove the deployed cross-repo envelope contract.
Target: dev/stage rollout gate before production enablement.

3. Assign backfill and customer-communication owners

Reason: This is a cross-team operational decision, not an API implementation detail.
Target: adobe-rnd/llmo-data-retrieval-service#3267 rollout checklist.


Reply go ahead to begin implementation.

@dzehnder dzehnder added the ai-triaged-pr-reviews PR reviews triaged by AI label Sep 18, 2026

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. [Important] Source validation runs after prompt writes - unregistered source values persist before assertPermittedSource executes - src/controllers/brands.js:727 (details inline)
  2. [Important] OpenAPI spec missing formal 409 response for reconciliation refusal - docs/openapi/prompts-v2-api.yaml:257 (details inline)
  3. [Important] Description references ignored status but V2Prompt enum is [active, pending, deleted, expired] - docs/openapi/prompts-v2-api.yaml:151 (details inline)
  4. [Important] No server-side guard prevents callers from writing status: 'expired' directly, bypassing reconciliation - src/support/prompts-storage.js:1082 (details inline)
  5. [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
  6. [Important] findPromptsBlockingRegionRemoval with excludeExpired: true is untested - the new .not('status', 'in', ...) branch has no storage-level coverage
Non-blocking (7): minor issues and suggestions
  • nit: promptGenerationMaxExpireFraction silently falls back to 0.9 on misconfigured env var - add a log.warn so operators can detect it - src/controllers/brands.js:135
  • nit: Pending prompt generation-touch inflates the updated count with different semantics than content updates - worth a code comment or a separate touched count
  • nit: Reconciliation object shape varies across success/disabled/refused ({refused, candidateCount, ...} vs {skipped, reason}) - a status discriminator field would help typed consumers
  • suggestion: Extract context.env || env into a single resolvedEnv binding at the top of createPromptsByBrand to reduce the three repeated lookups
  • nit: Structured log field run_id is inconsistent with the API field generationId and DB column generation_id - consider generation_id in 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.warn message 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 👎.

Comment thread src/controllers/brands.js
Comment thread docs/openapi/prompts-v2-api.yaml
Comment thread docs/openapi/prompts-v2-api.yaml
Comment thread src/support/prompts-storage.js
@MysticatBot MysticatBot added ai-reviewed Reviewed by AI complexity:high AI-assessed PR complexity: HIGH needs-human-review AI reviewer recommends a human read before merge labels Sep 18, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dzehnder

Copy link
Copy Markdown
Contributor Author

@codecov[bot]: Patch coverage is 98.62069% with 4 lines in your changes missing coverage.

Not implementing: this automated coverage status does not identify a correctness defect, and the review batch adds direct regressions for the requested lifecycle paths.

@dzehnder

Copy link
Copy Markdown
Contributor Author

@github-actions[bot]: This PR will trigger a minor release when merged.

Not implementing: this is release-status information, not a requested code change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dzehnder
dzehnder deployed to dev-branches September 18, 2026 16:00 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI ai-triaged-pr-reviews PR reviews triaged by AI complexity:high AI-assessed PR complexity: HIGH needs-human-review AI reviewer recommends a human read before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants