Skip to content

fix(reviews): share monitor evaluation results - #8664

Merged
iscekic merged 13 commits into
mainfrom
drain/review-monitor-refresh
Oct 27, 2026
Merged

iscekic merged 13 commits into
mainfrom
drain/review-monitor-refresh

Conversation

@iscekic

@iscekic iscekic commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Evidence

Evidence: demonstrated — a running local Next.js application and an isolated Docker PostgreSQL database exercised the monitor response contract at head ecc7e6f3355c9ccdd8210119539812e2aa9e5782. The migration chain applied on an empty database, and the detector window predicate resolved through the generated index.

Observed on this tree:

  • No credential and a mismatched bearer token each returned 401 with the unauthorized body; the configured bearer token returned 200 with a healthy body.
  • A second authorized call inside the shared interval returned a byte-identical payload with the same evaluation timestamp and produced zero detector statements in the PostgreSQL statement log; a control statement confirmed logging was active.
  • After the interval elapsed, a fresh evaluation ran (one statement per detector in the database log) and returned 503 with the expected alert for three synthetic recent reviews.
  • The full migration chain applied to an empty local database through drizzle:verify-bootstrap, and the generated index is valid and ready. The detector window predicate resolved to a bitmap index scan on it with sequential scans disabled. This verifies predicate compatibility, not production performance.

Changes

  • Configure the monitor through the existing server environment configuration.
  • Share each instance's evaluation result for the configured interval.
  • Provide the detector window index and preserve detector policy.
  • Document owner-operated provisioning, deployment, and rotation.
  • Merge the default branch, preserve its generated migration metadata, and regenerate this branch's index as 0307_review_window_index.sql.

Verification

Run on this follow-up's updated tree:

  • Frozen dependency installation passed.
  • pnpm drizzle generate reported no schema changes after the regeneration.
  • pnpm drizzle:verify-bootstrap applied the full chain to an empty local database.
  • The generated index is valid and ready, and the detector window predicate used it in the local query plan.
  • Database package suite: 50 tests passed. Database package and web application typechecks passed. Focused route and detector suites: 41 tests passed. Lint on the changed paths passed.
  • Local HTTP driver: unauthorized, healthy, cached, refreshed, and alert-response assertions all passed as described under Evidence.
  • No visual surface changed. The external production monitor was not exercised or modified.

Production prerequisites

Apply packages/db/src/migrations/0307_review_window_index.sql before application deployment. The owner must run pnpm web:env set CODE_REVIEW_HEALTH_SECRET and coordinate application deployment with the external monitor's bearer-header update. See ENVIRONMENT.md.

Proxy answers

The owner's proxy gave these answers in the owner's place.

  1. Question: Monitor contract: Plan decision: use a dedicated CODE_REVIEW_HEALTH_SECRET with Authorization: Bearer, preserve detector policy, add a 60-second per-instance shared evaluation, and document an operator-coordinated provisioning/deploy/monitor update. The monitor configuration is outside this repository, and this session will not change production. Which transport and rollout contract should this PR adopt?
    Answer: Bearer header; coordinated cutover
    Confidence: 0.60
  2. Question: Review recovery: The merge commit has green CI and no current-head Kilo Code Review check; the prior head's review was approved, and its generated detector migration matches byte-for-byte after index regeneration. How should I resolve the missing current-head review?
    Answer: Retry on review page
    Confidence: 0.85
  3. Question: Review fallback: The review page redirects this VM to sign-in, so I cannot use its retry action. The merge commit adds main plus the regenerated migration; its DDL is byte-identical to the reviewed migration, and the prior review summary is clean. How should I proceed?
    Answer: Use prior clean review
    Confidence: 0.80

@iscekic iscekic added drain Work item from the findings drain janitor:trust Janitor finding: trust boundary hardening labels Oct 22, 2026
@kilo-code-bot

kilo-code-bot Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental delta renames the redundant code-review window index migration to 0303_review_window_index after merged main's 0302_loud_victor_mancha, keeps the index DDL and snapshot prevId chain intact, and updates ENVIRONMENT.md to name the new file; the route, detector, and config files are unchanged since the previously approved review.

Assumptions: [INFERENCE] Migration apply and drizzle-kit generate were not executed in this read-only review. The large incremental range is dominated by merged main changes (environment snapshots, device auth, mobile/i18n, usage ingest, etc.) that this PR did not author; only the migration renumbering and the ENVIRONMENT.md sentence are new PR changes. Generated snapshot/journal contents were checked for chain and index consistency rather than line-reviewed. Note: the PR body's "Production prerequisites" section still names 0302_regular_maverick.sql, which no longer exists after the renumber; the in-repo deployment doc now points at the correct file. The external monitor bearer-header cutover and CODE_REVIEW_HEALTH_SECRET provisioning remain operator steps.

Verification

  • _journal.json keeps a contiguous 0..303 sequence with 0303_review_window_index appended after main's 0302_loud_victor_mancha, and the new when (1793014244373) is greater than every earlier entry, satisfying packages/db/src/migration-journal.test.ts.
  • 0303_snapshot.json prevId equals 0302_snapshot.json id (3ef93f9d-…), which in turn chains to 0301_snapshot.json id (ff5b8190-…); the 0302→0303 snapshot diff adds only idx_cloud_agent_code_reviews_started_at_effective plus the id/prevId fields.
  • The renamed migration SQL differs from the reviewed file only in --> statement-breakpoint placement; its three statements (COMMIT;, the concurrent index, BEGIN;) are unchanged, and its expression/predicate still matches startedReviewsCteSql in detectors.ts:66 and the index declaration in packages/db/src/schema.ts:5710.
  • idx_cloud_agent_code_reviews_started_at_effective appears only in the 0303 migration, its snapshot, and schema.ts; no stale 0302_regular_maverick file or reference remains, and ENVIRONMENT.md:28 names the file that exists.
  • The monitor route still uses a length-safe timingSafeEqual, a single-slot 60-second cache with inFlight cleared in finally, and Cache-Control: no-store; no unbounded cache or listener growth was introduced.
Files Reviewed (11 files)
  • .env.local.example
  • ENVIRONMENT.md
  • apps/web/src/app/api/code-reviews/up/route.ts
  • apps/web/src/app/api/code-reviews/up/route.test.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.test.ts
  • packages/db/src/migrations/0303_review_window_index.sql
  • packages/db/src/migrations/meta/0303_snapshot.json
  • packages/db/src/migrations/meta/0302_snapshot.json
  • packages/db/src/migrations/meta/_journal.json
  • packages/db/src/schema.ts
  • packages/web-shared/src/lib/config.server.ts
Previous Review Summaries (4 snapshots, latest commit e638da9)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit e638da9)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The new delta renames the redundant code-review window index migration to 0302_regular_maverick after merged main's 0300/0301, with byte-identical DDL, an intact snapshot prevId chain, a unique journal index sequence, and an index name still matching the detector query.

Assumptions: [INFERENCE] Migration apply and drizzle-kit generate were not executed in this read-only review. The large incremental range includes merged main changes (device auth, contributor credits, mobile/i18n, etc.) that this PR did not author; only the migration renumbering and PR body update are new PR changes. Generated snapshot/journal contents were checked for chain and index consistency rather than line-reviewed. The external monitor bearer-header cutover and CODE_REVIEW_HEALTH_SECRET provisioning remain operational steps documented in ENVIRONMENT.md.

Verification

  • _journal.json indexes are contiguous 0..302 with 0302_regular_maverick appended after main's 0300/0301 and monotonically increasing when.
  • 0302_snapshot.json prevId equals 0301_snapshot.json id, and the snapshot carries both main's new columns and the new index.
  • idx_cloud_agent_code_reviews_started_at_effective appears only in 0302_regular_maverick.sql, its snapshot, and packages/db/src/schema.ts; no orphaned 0300_secret_the_phantom.sql remains.
  • The migration SQL is a 100% rename of the previously reviewed file, and its expression/predicate still matches startedReviewsCteSql in detectors.ts.
Files Reviewed (11 files)
  • .env.local.example
  • ENVIRONMENT.md
  • apps/web/src/app/api/code-reviews/up/route.ts
  • apps/web/src/app/api/code-reviews/up/route.test.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.test.ts
  • packages/db/src/migrations/0302_regular_maverick.sql
  • packages/db/src/migrations/meta/0302_snapshot.json
  • packages/db/src/migrations/meta/_journal.json
  • packages/db/src/schema.ts
  • packages/web-shared/src/lib/config.server.ts

Previous review (commit 59a98ef)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental delta regenerates the redundant code-review window index on the merged tree as migration 0300, after main's 0299_lame_mojo, with an intact snapshot prevId chain, no duplicate index name, and unchanged detector/route code that still matches the index expression.

Assumptions: [INFERENCE] Migration apply was not executed in this read-only review. The generated SQL is identical to the previously reviewed migration apart from the file name, the index name appears in exactly one migration plus packages/db/src/schema.ts, and the journal/snapshot files are generated artifacts excluded from review. The 0299/environment-table changes in the incremental range come from merged main, not this PR, and were not treated as PR findings. The external monitor bearer-header cutover and CODE_REVIEW_HEALTH_SECRET provisioning remain operational steps documented in ENVIRONMENT.md.

Files Reviewed (11 files)
  • .env.local.example
  • ENVIRONMENT.md
  • apps/web/src/app/api/code-reviews/up/route.ts
  • apps/web/src/app/api/code-reviews/up/route.test.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.test.ts
  • packages/db/src/migrations/0300_secret_the_phantom.sql
  • packages/db/src/migrations/meta/0300_snapshot.json
  • packages/db/src/migrations/meta/_journal.json
  • packages/db/src/schema.ts
  • packages/web-shared/src/lib/config.server.ts

Previous review (commit 1c18d66)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental delta renumbers the redundant code-review window index from migration 0296 to 0299 and it now matches the merged journal, schema definition, and existing COMMIT; CREATE INDEX CONCURRENTLY ...; BEGIN; pattern with no duplicate index name.

Assumptions: [INFERENCE] Migration apply was not executed in this read-only review; the SQL is byte-identical to the previously reviewed migration apart from the file name, the index name appears in exactly one migration, and the snapshot/journal files are generated artifacts excluded from review.

Files Reviewed (11 files)
  • .env.local.example
  • ENVIRONMENT.md
  • apps/web/src/app/api/code-reviews/up/route.ts
  • apps/web/src/app/api/code-reviews/up/route.test.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.test.ts
  • packages/db/src/migrations/0299_tidy_grey_gargoyle.sql
  • packages/db/src/migrations/meta/0299_snapshot.json
  • packages/db/src/migrations/meta/_journal.json
  • packages/db/src/schema.ts
  • packages/web-shared/src/lib/config.server.ts

Previous review (commit 347dd40)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The monitor endpoint now uses a dedicated bearer secret with timing-safe comparison, a 60-second per-instance cache, and a matching partial expression index; the detector SQL rewrite preserves the prior window/fallback semantics.

Assumptions: [INFERENCE] The external monitor's bearer-header cutover and CODE_REVIEW_HEALTH_SECRET provisioning in production are operational steps outside the repository, as documented in ENVIRONMENT.md; stale monitor configuration would yield 401s. Migration execution was not run in this read-only review, but the COMMIT; CREATE INDEX CONCURRENTLY ...; BEGIN; pattern matches existing migrations (e.g. 0272, 0278), the snapshot diff is limited to the new index, and the index predicate/expression matches the detector query exactly. Promise.withResolvers used in tests is supported by the repo's Node >=24 engine.

Files Reviewed (11 files)
  • .env.local.example
  • ENVIRONMENT.md
  • apps/web/src/app/api/code-reviews/up/route.ts
  • apps/web/src/app/api/code-reviews/up/route.test.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.ts
  • apps/web/src/lib/code-reviews/alerting/detectors.test.ts
  • packages/db/src/migrations/0296_jittery_sersi.sql
  • packages/db/src/migrations/meta/0296_snapshot.json
  • packages/db/src/migrations/meta/_journal.json
  • packages/db/src/schema.ts
  • packages/web-shared/src/lib/config.server.ts

Reviewed by deepseek-flash-latest · Input: 85.7K · Output: 13.8K · Cached: 1.2M

Review guidance: REVIEW.md from base branch main

@iscekic iscekic added merge:human Substantive functional risk requires human review. intent:ok Janitor change confirmed as a real bug fix, not intended behavior needs:prod-action Prod prerequisite (env var, secret, firewall rule, config) needed before merge labels Oct 22, 2026
…refresh

# Conflicts:
#	packages/db/src/migrations/meta/0296_snapshot.json
#	packages/db/src/migrations/meta/_journal.json
@iscekic iscekic added merge:human Substantive functional risk requires human review. merge:blocked:judge-unavailable Risk assessment unavailable merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. and removed merge:human Substantive functional risk requires human review. merge:blocked:judge-unavailable Risk assessment unavailable merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. labels Oct 23, 2026
…refresh

# Conflicts:
#	packages/db/src/migrations/meta/0297_snapshot.json
#	packages/db/src/migrations/meta/_journal.json
@iscekic iscekic added merge:human Substantive functional risk requires human review. do-not-merge Do not merge this PR (yet) and removed merge:human Substantive functional risk requires human review. labels Oct 24, 2026
@iscekic iscekic added merge:human Substantive functional risk requires human review. and removed merge:human Substantive functional risk requires human review. labels Oct 24, 2026
@iscekic iscekic added merge:human Substantive functional risk requires human review. and removed merge:human Substantive functional risk requires human review. labels Oct 24, 2026
The default branch appended its own migration at the sequence number this
branch had reserved, so the branch entry is generated again at the next free
index. The generated DDL and index definition are unchanged; only the migrator
transaction boundaries follow the shared database conventions.
@iscekic iscekic removed the merge:human Substantive functional risk requires human review. label Oct 24, 2026
@iscekic

iscekic commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Both requests are addressed at the current head.

  • Merged the latest default branch and kept its journal and snapshots; the branch migration is regenerated at the next free sequence position after it.
  • The generated index DDL is unchanged and a follow-up drizzle-kit generate reports no schema drift.
  • Applied the full migration chain on a fresh database and re-ran the bootstrap check; the index exists with the expected definition, valid and ready.
  • Re-ran the detector window query plan check: the planner selects the new index with a bitmap index scan over the window condition.
  • Database package suite and the focused route and detector suites pass.
  • The PR body's production prerequisites now name the migration file.

@iscekic iscekic added merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:human Substantive functional risk requires human review. and removed merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head labels Oct 24, 2026
@iscekic iscekic added merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:human Substantive functional risk requires human review. and removed merge:human Substantive functional risk requires human review. merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head labels Oct 26, 2026
@iscekic

iscekic commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

bot: Updated the production prerequisites to name packages/db/src/migrations/0303_review_window_index.sql. The repository deployment documentation names the same file. The updated-head local application proof, fresh-database migration bootstrap, query-plan check, and focused test results are recorded in the PR body.

…refresh

# Conflicts:
#	packages/db/src/migrations/meta/0303_snapshot.json
#	packages/db/src/migrations/meta/_journal.json
@iscekic iscekic removed the merge:human Substantive functional risk requires human review. label Oct 27, 2026
@iscekic

iscekic commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

bot: The migration is regenerated against the current default branch after the latest merge.

  • Merged the latest default branch and kept its journal and snapshots; the branch migration is regenerated at the next free sequence position (0307_review_window_index.sql).
  • The generated index DDL is unchanged, and a follow-up drizzle-kit generate reports no schema drift.
  • Applied the full migration chain on a fresh database and passed the bootstrap check; the index exists with the expected definition, valid and ready.
  • The detector window query plan uses the new index in a bitmap index scan.
  • Database package suite (50) and the focused route and detector suites (41) pass; web and database package typechecks pass.
  • The local application run observed the expected monitor responses, including a shared evaluation across repeated calls and a refreshed evaluation after the interval; the body records the head and the observations.
  • The PR body's production prerequisites name packages/db/src/migrations/0307_review_window_index.sql.

@iscekic iscekic added the merge:human Substantive functional risk requires human review. label Oct 27, 2026
@iscekic
iscekic added this pull request to the merge queue Oct 27, 2026
@iscekic
iscekic removed this pull request from the merge queue due to a manual request Oct 27, 2026
@iscekic
iscekic added this pull request to the merge queue Oct 27, 2026
@iscekic iscekic removed the do-not-merge Do not merge this PR (yet) label Oct 27, 2026
Merged via the queue into main with commit 29c9152 Oct 27, 2026
88 of 89 checks passed
@iscekic
iscekic deleted the drain/review-monitor-refresh branch October 27, 2026 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drain Work item from the findings drain intent:ok Janitor change confirmed as a real bug fix, not intended behavior janitor:trust Janitor finding: trust boundary hardening merge:human Substantive functional risk requires human review. needs:prod-action Prod prerequisite (env var, secret, firewall rule, config) needed before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants