Skip to content

fix(teaching,cme): address late CodeRabbit review on #3614 - #3622

Merged
BigSimmo merged 4 commits into
mainfrom
fix/3614-coderabbit-followups
Oct 17, 2026
Merged

BigSimmo merged 4 commits into
mainfrom
fix/3614-coderabbit-followups

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Oct 17, 2026 •

Copy link
Copy Markdown
Owner

User description

Follow-up to #3614. This addresses the 6 CodeRabbit threads that were posted after #3614 auto-merged.

Fixes (5)

  1. Dual sign-off reads accept only dual sign-off rows. Rows must carry ratings.kind = "dual_sign_off", applied to the lookup by id and to both lists. Other registrar_assessments rows are never read as assessments or rewritten into one. New test covers this.
  2. Slot times must be ISO 8601 with an offset. startsAt/endsAt use z.string().datetime({ offset: true }).
  3. Booking listings without a role filter in the database. They return the caller's own slots (as supervisor or as trainee), plus open slots when a service is named, instead of scanning every booking.
  4. Part-time EPA minimum is a whole number. It is max(1, ceil(2 × FTE)), never a fraction.
  5. Registrar-only milestones can be edited and deleted. This landed in feat(cme,teaching,rag): wire claim-pack surface, registrar milestones, durable dual-sign-off, bookings, and chunking regex hoist (#GR72PF, #XNAHY2, #AD9JC6, #AW8SRK, #54N8ZN, #YETFBJ, #BGDVPD) #3614 itself (PATCH and DELETE on /api/cme/training/[id] fall back to registrar_training_milestones). It is listed here so all 6 threads are accounted for.

No change needed (1)

No sign-off records are created, changed or approved; these are code-level validation changes only.


Summary by cubic

Addresses the six CodeRabbit review threads posted after #3614 auto-merged with five code-level fixes; no sign-off records are created, changed, or approved.

Written for commit 126778e. Summary will update on new commits.

View guided diff Turn on auto-fix


CodeAnt-AI Description

Protect dual sign-off records and validate booking schedules

What Changed

  • Dual sign-off views and updates now accept only records created by the dual sign-off flow, leaving unrelated assessment records untouched.
  • Booking times must include a timezone offset, and listings without a role filter show the caller’s own slots plus open slots for the selected service.
  • Part-time rotation EPA targets now round up to a whole number, with a minimum of one.

Impact

✅ Unrelated assessment records stay unchanged
✅ Fewer timezone-ambiguous booking times
✅ Booking lists exclude other users’ slots
✅ Whole-number EPA targets for part-time rotations

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Summary by CodeRabbit

  • Bug Fixes
    • Booking creation now requires start and end times to include a time-zone offset.
    • Part-time rotations now receive an EPA minimum scaled to their FTE, with a minimum of one; full-time rotations and cases without a current rotation retain a minimum of two.
    • Assessment records and booking lists are now filtered and displayed more consistently, including when no role is specified.

- Dual sign-off reads accept only rows marked ratings.kind = dual_sign_off, so
  other registrar_assessments rows are never read or rewritten as assessments.
- Slot times must be ISO 8601 with an offset.
- Role-less booking listings are filtered in the database (own slots, plus open
  slots for a named service) instead of scanning every booking.
- Part-time EPA minimum rounds up to a whole number.
@supabase

supabase Bot commented Oct 17, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@BigSimmo
BigSimmo enabled auto-merge (squash) October 17, 2026 15:30

@greptile-apps greptile-apps Bot 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.

BigSimmo has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@codeant-ai

codeant-ai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

🤖 CodeAnt AI — Review Status

Status Commit Started (UTC) Finished (UTC)
✅ Reviewed your PR affe204 Oct 17, 2026 · 15:31 15:37

@codeant-ai

codeant-ai Bot commented Oct 17, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-17T15:33:08.417925Z affe204 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

Next included review available in 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 115 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: BigSimmo/PsychSift/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Team
  • Run ID: 6104ec53-2648-492c-8865-22b526c0e22b
📥 Commits

Reviewing files that changed from the base of the PR and between 9b0d028 and 126778e.

📒 Files selected for processing: 5
  • src/app/api/teaching/assessments/bookings/route.ts
  • src/lib/cme/training-assessments.ts
  • src/lib/teaching/assessments/dual-sign-off.ts
  • src/lib/teaching/assessments/supervisor-bookings.ts
  • tests/teaching-dual-sign-off-durable.test.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: BigSimmo/PsychSift/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Team
  • Run ID: 9baf717c-6fd0-49ba-aee3-ed4a8d62b6a1
📥 Commits

Reviewing files that changed from the base of the PR and between ebd0dd8 and 9b0d028.

📒 Files selected for processing: 5
  • src/app/api/teaching/assessments/bookings/route.ts
  • src/lib/cme/training-assessments.ts
  • src/lib/teaching/assessments/dual-sign-off.ts
  • src/lib/teaching/assessments/supervisor-bookings.ts
  • tests/teaching-dual-sign-off-durable.test.ts

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

📝 Walkthrough

The changes update booking slot datetime validation, role-less booking queries, dual sign-off assessment row classification, and the EPA minimum calculation for current part-time rotations.

Changes: Teaching booking behavior

Layer / File(s) Summary
Validate slot datetime inputs
src/app/api/teaching/assessments/bookings/route.ts
The create_slot action now requires startsAt and endsAt to be datetime strings with an explicit offset.
Query bookings without a role
src/lib/teaching/assessments/supervisor-bookings.ts
When no role is specified, the repository queries supervisor and trainee bookings separately. It queries open bookings when serviceId is supplied, then deduplicates results by slot ID and sorts by start time.

Dual sign-off assessment rows

Layer / File(s) Summary
Write and classify dual sign-off rows
src/lib/teaching/assessments/dual-sign-off.ts, tests/teaching-dual-sign-off-durable.test.ts
Written assessment payloads now include the required dual_sign_off discriminator. Durable lookups and lists accept rows with that discriminator or rows matching the defined legacy shape. The regression test checks that a foreign row returns 404 for GET and draft update and that its ratings remain unchanged.

EPA minimum calculation

Layer / File(s) Summary
Calculate the current-rotation minimum
src/lib/cme/training-assessments.ts
The current-rotation EPA minimum is now the ceiling of twice the FTE value, with a floor of one. The fallback minimum remains two.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 9b0d0

The supplied evidence identifies no remaining issue that should delay merging after normal checks.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Description check Warning The description provides a detailed summary of the five fixes and one deferred item, but it does not use the required Summary, Verification, Risk and rollout, and Notes sections. It also omits verific… Update the description to follow the repository template. Add the required Verification section with the checks run and decisive output. Add the Risk and rollout section with risk, rollback, and provider or production effects. Add the Notes…
Docstring Coverage Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly identifies a teaching and CME fix that addresses review feedback from #3614.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description provides a detailed summary of the five fixes and one deferred item, but it does not use the required Summary, Verification, Risk and rollout, and Notes sections. It also omits verification results and risk, rollback, and provider-effect details.

Resolution

Update the description to follow the repository template. Add the required Verification section with the checks run and decisive output. Add the Risk and rollout section with risk, rollback, and provider or production effects. Add the Notes section if applicable, and place the change summary under Summary.

✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@codeant-ai codeant-ai Bot added the size:M This PR changes 30-99 lines, ignoring generated files label Oct 17, 2026
@codeant-ai

codeant-ai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

🏁 CodeAnt Quality Gate Results

Commit: 126778ee
Scan Time: 2026-10-17 16:38:02 UTC

✅ Overall Status: PASSED

Quality Gate Details

Quality Gate Status Details
Secrets ✅ PASSED 0 secrets found
Duplicate Code ✅ PASSED 0.0% duplicated
SAST ✅ PASSED No security issues
Bugs ✅ PASSED Rating S: No bugs
IAC ✅ PASSED No IAC issues

View Full Results

@qodo-code-review

qodo-code-review Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Protect dual sign-off rows and tighten booking and EPA rules

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Restrict dual sign-off reads to tagged assessments, preventing updates to unrelated registrar
 rows.
• Require offset-aware slot times and query only relevant bookings for role-less listings.
• Round part-time EPA minimums up to whole numbers and test foreign-row protection.
Diagram

graph TD
  A["Dual sign-off API"] --> B["Assessment repository"] --> D[("Registrar assessments")]
  C["Booking API"] --> E["Booking repository"] --> D
  F["CME training view"] --> G["EPA minimum"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Separate assessment and booking tables
  • ➕ Enforces record separation at the schema level rather than through a ratings discriminator.
  • ➖ Requires a data migration and broader changes to existing repositories and queries.

Recommendation: Keep the tagged-row approach for this follow-up. It protects reads within the existing shared-table design without the migration and compatibility work of splitting storage.

Files changed (5) +56 / -10

Bug fix (4) +31 / -10
route.tsRequire offset-aware slot timestamps +2/-2

Require offset-aware slot timestamps

• Slot creation now validates start and end times as ISO 8601 datetimes with an offset instead of accepting any nonempty strings.

src/app/api/teaching/assessments/bookings/route.ts

training-assessments.tsRound prorated EPA minimums up +2/-1

Round prorated EPA minimums up

• The current rotation's EPA minimum is now the ceiling of twice its FTE, with a minimum of one. Rotations without a current match retain the default of two.

src/lib/cme/training-assessments.ts

dual-sign-off.tsLimit durable assessment reads to dual-sign-off rows +16/-7

Limit durable assessment reads to dual-sign-off rows

• New assessment rows carry a required dual-sign-off kind. Lookup and both listing paths reject other registrar assessment rows, while listings also filter by kind in the database.

src/lib/teaching/assessments/dual-sign-off.ts

supervisor-bookings.tsScope role-less booking listings in the database +11/-0

Scope role-less booking listings in the database

• Listings without a role combine the caller's supervisor and trainee bookings with open slots when a service is specified. Results are deduplicated and sorted without querying every booking.

src/lib/teaching/assessments/supervisor-bookings.ts

Tests (1) +25 / -0
teaching-dual-sign-off-durable.test.tsTest protection of unrelated registrar rows +25/-0

Test protection of unrelated registrar rows

• A production-mode regression test verifies that a foreign registrar assessment returns 404 for lookup and draft update, leaving its ratings unchanged.

tests/teaching-dual-sign-off-durable.test.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: affe204ae6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Previously saved sign-off assessments vanish after this fix 🔴 High 🐞 Bug ≡ Correctness
Description
assessmentRecordToRow only started writing ratings.kind = "dual_sign_off" in this PR; before it,
kind was optional and never set on write, so every dual sign-off assessment persisted in
production prior to this deploy has no kind field in its ratings JSON. isDualSignOffRow (and
the new ratings->>kind database filter in listForSupervisor/listForTrainee) now rejects any
row lacking that exact marker, so getRecord throws AssessmentNotFoundError for every
pre-existing assessment id and both list methods silently drop them from supervisor/trainee views,
with no migration or backfill adding the marker to historical rows.
Code

src/lib/teaching/assessments/dual-sign-off.ts[R760-764]

      throw assessmentDatabaseError(error);
    }

-    if (!data || isSupervisorBookingRow(data)) {
+    if (!data || !isDualSignOffRow(data)) {
      throw new AssessmentNotFoundError(id);
Relevance

●●● Strong

Strictly requiring a new marker breaks legacy persisted assessments; historical patterns favor
backward-compatible reads or migration.

PR-#2364
PR-#3460

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new strict marker check replaces a looser category-based check, while the only writer of the
marker is new code in this same PR; no SQL migration backfills ratings.kind on existing rows, so
historic rows fail the new predicate everywhere it's used (get, list-for-supervisor,
list-for-trainee).

src/lib/teaching/assessments/dual-sign-off.ts[234-240]
src/lib/teaching/assessments/dual-sign-off.ts[304-313]
src/lib/teaching/assessments/dual-sign-off.ts[754-766]
src/lib/teaching/assessments/dual-sign-off.ts[861-876]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR makes `ratings.kind = "dual_sign_off"` mandatory for a `registrar_assessments` row to be read as a dual sign-off assessment (via `isDualSignOffRow` and the new `ratings->>kind` database filter), but the marker was only ever written starting with this PR. Any dual sign-off assessment created before this deploy has no `kind` field and will now fail `getRecord` (404) and be silently excluded from `listForSupervisor`/`listForTrainee`.

## Fix Focus Areas
- src/lib/teaching/assessments/dual-sign-off.ts[304-313]
- src/lib/teaching/assessments/dual-sign-off.ts[754-766]
- src/lib/teaching/assessments/dual-sign-off.ts[861-876]
- src/lib/teaching/assessments/dual-sign-off.ts[884-899]

## Recommended Fix
Add a one-off migration/backfill script that sets `ratings.kind = 'dual_sign_off'` on every existing `registrar_assessments` row that is not a supervisor booking (e.g. rows where `ratings->>'kind'` is null and `category <> 'supervisor_review'`), run it before or atomically with this deploy, and only then rely on the strict `isDualSignOffRow` check. Alternatively, treat a null/missing `kind` as dual sign-off for a transition period (fallback to the pre-PR category-based heuristic) until the backfill confirms all rows are migrated.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗



Remediation recommended

No findings for this group


Informational

No findings for this group

Grey Divider

Context sources
❌ Tickets: failed to load — Ticket collection could not establish complete evidence
✅ Compliance rules (platform): 9 rules
✅ Skills: 8 invoked
  api-review
  testing-review
  supabase-postgres-best-practices
  supabase
  access
  api
  review
  security-review
✅ Cross-repo context — repo relationships
Review mode: Auto: ⚖️ Balanced: Multiple behavioral fixes affect validation, data filtering, queries, and assessment integrity.

Grey Divider

Tip of the day
💡 Did you know, you can see how risky a PR is at a glance with blast radius labels

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@codeant-ai

codeant-ai Bot commented Oct 17, 2026

Copy link
Copy Markdown

CodeAnt Nitpicks

1 code suggestion

1. Independent reads can straddle a booking change; the stale open duplicate is inserted last and overwrites the newer booked state in the returned map.

Race condition · src/lib/teaching/assessments/supervisor-bookings.ts:576-581

@codeant-ai

codeant-ai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

CodeAnt PR Risk: Low Risk

  • The PR appears safe to merge based on the changed code and supplied tests.
  • Dual sign-off reads accept marked rows or a narrowly recognized legacy shape, with tests for both legacy access and rejecting unrelated rows.
  • Booking timestamps now require an explicit timezone, and part-time EPA targets round up to a whole assessment.

Assessed commit: b185ab53e963

@qodo-code-review

qodo-code-review Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

No findings for this group


Remediation recommended

No findings for this group


Informational

No findings for this group

Grey Divider


Resolved findings

1. Existing assessments become inaccessible ✓ Resolved
Description
isDualSignOffRow now requires ratings.kind === "dual_sign_off", but assessmentRecordToRow did
not write that marker before this change. Previously stored dual sign-off rows therefore fail
getRecord, which blocks updates that depend on it, and are excluded from both supervisor and
trainee lists.
Code

src/lib/teaching/assessments/dual-sign-off.ts[312]

+    (row.ratings as Record<string, unknown>).kind === DUAL_SIGN_OFF_KIND,
Relevance

●● Moderate

Legacy compatibility concern is plausible, but same-day precedent favors strict marker-based
isolation.

PR-#3614

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The serializer only begins writing ratings.kind in this change, while getRecord rejects rows
without the exact marker and both list methods filter on it in their database queries and
client-side checks. No migration or backfill is provided to add the marker to previously written
rows.

src/lib/teaching/assessments/dual-sign-off.ts[232-250]
src/lib/teaching/assessments/dual-sign-off.ts[304-313]
src/lib/teaching/assessments/dual-sign-off.ts[760-766]
src/lib/teaching/assessments/dual-sign-off.ts[861-899]
supabase/migrations/20261013160000_teaching_assessments_and_milestones.sql[11-26]
src/lib/teaching/assessments/dual-sign-off.ts[754-765]
src/lib/teaching/assessments/dual-sign-off.ts[232-249]
src/lib/teaching/assessments/dual-sign-off.ts[858-899]
Skill: api

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Previously written dual sign-off assessments lack `ratings.kind`, so the new read guard and list filters make them inaccessible; updates cannot repair them because they first call `getRecord`.

## Fix Focus Areas
- src/lib/teaching/assessments/dual-sign-off.ts[232-249]
- src/lib/teaching/assessments/dual-sign-off.ts[304-313]
- src/lib/teaching/assessments/dual-sign-off.ts[754-766]
- src/lib/teaching/assessments/dual-sign-off.ts[858-899]

## Recommended Fix
Before enforcing the discriminator, add a one-time backfill that sets `ratings.kind = 'dual_sign_off'` only on rows reliably identifiable as products of the earlier dual sign-off flow; do not tag every row with a missing kind. Alternatively, temporarily accept reliably identified legacy rows in the read guard and list queries until the backfill has run everywhere. Add regression coverage confirming that legacy dual sign-off assessments remain readable and other assessment rows remain excluded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
❌ Tickets: failed to load — Ticket collection could not establish complete evidence
✅ Compliance rules (platform): 9 rules
✅ Skills: 8 invoked
  api-review
  security-review
  supabase
  supabase-postgres-best-practices
  testing-review
  access
  api
  review
✅ Cross-repo context — repo relationships
Review mode: Auto: ⚖️ Balanced: Multiple behavioral and data-access changes carry genuine review risk.

Grey Divider

Tip of the day
💡 Did you know, you can see how risky a PR is at a glance with blast radius labels

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 126778e

Results up to commit affe204 ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required

No findings for this group


Remediation recommended

No findings for this group


Informational

No findings for this group


Resolved findings

1. Existing assessments become inaccessible ✓ Resolved
Description
isDualSignOffRow now requires ratings.kind === "dual_sign_off", but assessmentRecordToRow did
not write that marker before this change. Previously stored dual sign-off rows therefore fail
getRecord, which blocks updates that depend on it, and are excluded from both supervisor and
trainee lists.
Code

src/lib/teaching/assessments/dual-sign-off.ts[312]

+    (row.ratings as Record<string, unknown>).kind === DUAL_SIGN_OFF_KIND,
Relevance

●● Moderate

Legacy compatibility concern is plausible, but same-day precedent favors strict marker-based
isolation.

PR-#3614

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The serializer only begins writing ratings.kind in this change, while getRecord rejects rows
without the exact marker and both list methods filter on it in their database queries and
client-side checks. No migration or backfill is provided to add the marker to previously written
rows.

src/lib/teaching/assessments/dual-sign-off.ts[232-250]
src/lib/teaching/assessments/dual-sign-off.ts[304-313]
src/lib/teaching/assessments/dual-sign-off.ts[760-766]
src/lib/teaching/assessments/dual-sign-off.ts[861-899]
supabase/migrations/20261013160000_teaching_assessments_and_milestones.sql[11-26]
src/lib/teaching/assessments/dual-sign-off.ts[754-765]
src/lib/teaching/assessments/dual-sign-off.ts[232-249]
src/lib/teaching/assessments/dual-sign-off.ts[858-899]
Skill: api

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Previously written dual sign-off assessments lack `ratings.kind`, so the new read guard and list filters make them inaccessible; updates cannot repair them because they first call `getRecord`.

## Fix Focus Areas
- src/lib/teaching/assessments/dual-sign-off.ts[232-249]
- src/lib/teaching/assessments/dual-sign-off.ts[304-313]
- src/lib/teaching/assessments/dual-sign-off.ts[754-766]
- src/lib/teaching/assessments/dual-sign-off.ts[858-899]

## Recommended Fix
Before enforcing the discriminator, add a one-time backfill that sets `ratings.kind = 'dual_sign_off'` only on rows reliably identifiable as products of the earlier dual sign-off flow; do not tag every row with a missing kind. Alternatively, temporarily accept reliably identified legacy rows in the read guard and list queries until the backfill has run everywhere. Add regression coverage confirming that legacy dual sign-off assessments remain readable and other assessment rows remain excluded.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo Fixer

✅ Committed (1) · ☑ Fixed (1)

Grey Divider

Commits pushed directly to this PR — no separate fix PR opened.

Process — 1 fixed
  • ☑ Fixed: Existing assessments become inaccessible

gitar-bot[bot]
gitar-bot Bot previously approved these changes Oct 17, 2026

@gitar-bot gitar-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gitar has auto-approved this PR (configure)

… and updatable

Covers Qodo's legacy-shape fallback for assessments written by #3614 before
ratings.kind existed; an update re-writes them with the marker.

@gitar-bot gitar-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gitar has auto-approved this PR (configure)

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit b185ab5

@gitar-bot

gitar-bot Bot commented Oct 17, 2026

Copy link
Copy Markdown
Contributor
Code Review ✅ Approved 1 closed / 1 findings

🔴 High risk · Discriminator-based assessment reads and updates, including legacy-row matching, could expose or rewrite unrelated registrar assessment records if rows are misclassified.

Dual sign-off reads and updates now require ratings.kind = "dual_sign_off", so other registrar_assessments rows are no longer read or rewritten. This also addresses the resolved finding that dual sign-off rows written under #3614 lacked kind and were vanishing. No open issues remain.

✅ 1 closed
✅ Bug: Dual sign-off rows written under #3614 lack kind, so they now vanish

📄 src/lib/teaching/assessments/dual-sign-off.ts:237 📄 src/lib/teaching/assessments/dual-sign-off.ts:308-313 📄 src/lib/teaching/assessments/dual-sign-off.ts:763 📄 src/lib/teaching/assessments/dual-sign-off.ts:864 📄 src/lib/teaching/assessments/dual-sign-off.ts:887
Under #3614, assessmentRecordToRow never set ratings.kind. The field was optional and nothing wrote it, so every dual sign-off row persisted since #3614 shipped has no kind. This PR now requires ratings->>kind = 'dual_sign_off' in both list queries and in isDualSignOffRow, which getRecord uses for get, update, submit and sign-off. Those existing assessments stop appearing in supervisor and trainee lists and return 404 on every action, so in-flight drafts and pending sign-offs are effectively orphaned. Nothing in the PR backfills them.

Suggested fix: add a migration that sets kind = 'dual_sign_off' on existing rows that are not bookings, or treat rows with no kind that match the dual sign-off payload shape as legacy dual sign-off rows.

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

Auto-approval Approved

For more verification details, comment gitar display:verbose.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Important

Your trial ends in 4 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 126778e

@BigSimmo
BigSimmo merged commit e6ead1e into main Oct 17, 2026
38 of 39 checks passed
@BigSimmo
BigSimmo deleted the fix/3614-coderabbit-followups branch October 17, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🟠 Medium blast radius size:M This PR changes 30-99 lines, ignoring generated files 🕐 20-40 Minutes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant