Repository navigation
PR review: ground findings in production telemetry, and check production after merge - #1274
Confidence 3/5 · 1 issue to address
🟡 Confidence 3/5 · needs attention
The read path I flagged is untested and its blast radius depends on how the review fills services; everything else in the changed set I could check is behaviour-preserving.
quality 90/100 · 1 warning · tests partial · risk medium · 2/2 new units observable
Grounds PR review in production telemetry: contract breaks, traffic weighting, open errors, ingest cost, verified dismissals, an optional blocking check, and a post-merge look at production from the alerting cron. The telemetry reads, the lease-guarded posting and the timestamp audit are sound; one read can miss the merging deploy.
PrReviewTelemetryServicereads production operations, attribute keys, metrics and errors per reviewtelemetryFindings/weighByTrafficfileTEL-01..03and raise busy-file notes to warningsPrReviewPostMergeService.runTickclaims due rows, finds the deploy and posts one follow-up commentmaple/no-date-parselint rule replacesDate.parsewithtimestampMs/parseWarehouseDateTime
Findings
🟠 Warning · F3 · deploymentsSince's alphabetical 50-row cap can drop the merge's version
correctness · packages/backend/src/services/pr-review/telemetry/PrReviewTelemetryService.ts:356
On the breaks-only path (telemetry/PrReviewTelemetryService.ts:351 passes [undefined], so no serviceName) one query reads every service's versions, and serviceDeploymentsQuery orders by serviceName ASC before taking limit: 50 (packages/query-engine/src/ch/queries/releases.ts:355-359). An organization with more than 50 version rows in the 48 h window therefore gets only the alphabetically first services' versions, so pickDeploy never sees the version carrying the merge; after 48 h the row settles no_deploy and no follow-up comment is ever posted. The same read also caps each service at its 20 newest versions, which drops the merge's own version for a service that deploys more than 20 versions in the window.
Read recency-ordered versions (or issue one call per service with `serviceName` set) instead of relying on the shared query's `serviceName ASC` order; raising `limit` only widens the alphabetical window.
Observability coverage: 2 of 2 changes observable
| Change | Kind | Observable | Evidence |
|---|---|---|---|
| PrReviewTelemetryService warehouse + database reads (operations, attribute keys, metrics, deployments, issue counts) | outbound warehouse/db read | yes | compiledQuery call sites plus Effect.withSpan on analyze and deploymentsSince (PrReviewTelemetryService.ts:335,384) |
| PrReviewPostMergeService.runTick post-merge examination on the alerting cron | background worker | yes | Effect.withSpan("PrReviewPostMergeService.runTick") and per-row maple.pr_review.post_merge.* attributes (PrReviewPostMergeService.ts:239,360) |
9f61fe0 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.
Annotations
maple-review-bot / Maple / review
correctness: `deploymentsSince`'s alphabetical 50-row cap can drop the merge's version
On the breaks-only path (`telemetry/PrReviewTelemetryService.ts:351` passes `[undefined]`, so no `serviceName`) one query reads every service's versions, and `serviceDeploymentsQuery` orders by `serviceName ASC` before taking `limit: 50` (`packages/query-engine/src/ch/queries/releases.ts:355-359`). An organization with more than 50 version rows in the 48 h window therefore gets only the alphabetically first services' versions, so `pickDeploy` never sees the version carrying the merge; after 48 h the row settles `no_deploy` and no follow-up comment is ever posted. The same read also caps each service at its 20 newest versions, which drops the merge's own version for a service that deploys more than 20 versions in the window.