fix(fsma-recall): fail closed on negative completed drill duration by PetrefiedThunder · Pull Request #3598 · regengine/RegEngine

PetrefiedThunder · 2026-10-25T07:04:04Z

Follow-up to #3592 (stacked on its branch; retarget to main once #3592 merges).

Problem

RecallDrill.sla_status returned MET for a COMPLETED drill whose completed_at precedes started_at (negative duration_seconds), because -N <= 24h.

Decision: BREACHED, not a new "invalid" status

SLAStatus is only consumed by being serialised (to_dict, fsma_recall/__init__.py) into API output with values met/at_risk/breached. Adding a new value would break consumers that switch on those three. Corrupt timing must never certify 24h compliance (21 CFR 1.1455(c)(1)), so it fails closed to the existing non-compliant status.

Change

models.py: duration is not None and 0 <= duration <= sla_limit.
services/graph/tests/test_recall_sla_negative_duration.py (data-free, timestamps only): -1s → BREACHED, -48h → BREACHED, 0s still MET. The two negative tests fail before the fix; 65 recall tests pass after.

Reviewer: DeepSeek (non-pi engine), per Lead.

Note

Low Risk
Narrow SLA classification fix for corrupt timestamps; only misclassified negative-duration completed drills change from met to breached.

Overview
Fixes a compliance bug where completed FSMA recall drills with completed_at before started_at could still report sla_status = met, because a negative duration_seconds satisfied <= 24h.

RecallDrill.sla_status now only treats a finished drill as compliant when duration is 0 <= duration <= 24h. Corrupt negative timings fail closed to breached, reusing the existing non-compliant enum instead of adding a new API value. Instant completions (0s) remain met.

Adds timestamp-only tests (-1s, -48h → breached; 0s → met).

^{Reviewed by Cursor Bugbot for commit eb35d96. Bugbot is set up for automated code reviews on this repo. Configure here.}

vercel · 2026-10-25T07:04:05Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
regengine	Ready	Preview	Oct 26, 2026 7:51am UTC

coderabbitai · 2026-10-25T07:04:12Z

No actionable comments were generated in the recent review.

Recent review info

Run configuration

Source mode: Auto
Repository: regengine/RegEngine
Branch: pi/recall-sla-negative-duration
File: .coderabbit.yaml
Commit: 759b856
Layers: Repository YAML
Review profile: ASSERTIVE
Plan: Advanced
Run ID: ccf9f6d9-1e8d-4f06-8f9a-7605f0e2ea88

Commits

Reviewing files that changed from the base of the PR and between eb35d96 and 759b856.

Files selected for processing: 1

services/graph/app/fsma_recall/models.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Walkthrough

Completed recall drills now receive MET status only when their duration is between zero and 24 hours, inclusive. Negative or missing durations receive BREACHED. Timestamp-based tests cover a one-second negative duration, a 48-hour negative duration, and zero duration.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Merge Risk: Minimal · up to 759b8

The change addresses negative drill durations without an established new compliance-status regression. It is ready to merge after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description check		The description clearly explains the problem, decision, implementation, compatibility impact, and test coverage. It omits the template's Type of Change and Checklist sections, but the provided informa…
Title check		The title clearly and concisely describes the primary change: preventing negative completed-drill durations from passing SLA validation.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description clearly explains the problem, decision, implementation, compatibility impact, and test coverage. It omits the template's Type of Change and Checklist sections, but the provided information is sufficient and mostly complete.

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.}

chatgpt-codex-connector · 2026-10-25T07:04:15Z

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-25T07:04:57.032597Z	`eb35d96`	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.

github-actions · 2026-10-25T07:22:06Z

PR Blocker Checklist

Head SHA: 759b856dc4e830928d70441abd55ab1f917a7d59

Status: Ready

All review threads resolved
No reviews requesting changes
No blocked review engines

PetrefiedThunder

REVIEW: PASS deepseek eb35d96

Reviewed by hand. The parent is ffd29de (#3592's head), so the diff vs main is #3592 plus this commit: models.py SLA check 0 <= duration <= sla_limit, plus the zero/negative tests.

Boundaries: 0 = MET, negative = BREACHED. 7 tests pass. With #3592's models.py, the 2 negative-duration tests fail.
The tests use no data (empty ids, timestamps only).
Merge after #3592 (stacked).

CI: all real checks green. Remaining failures are engine-review gates only (circular), plus Vercel Preview Built (deploy skipped by Ignored Build Step on non-frontend PRs, not a build failure): Engine Review Pass,Engine Review Pass,PR Blocker Checklist,Head SHA Check Guard,

PetrefiedThunder

REVIEW: PASS deepseek eb35d96

Reviewed by hand. The parent is ffd29de (#3592's head), so the diff vs main is #3592 plus this commit: models.py SLA check 0 <= duration <= sla_limit, plus the zero/negative tests.

Boundaries: 0 = MET, negative = BREACHED. 7 tests pass. With #3592's models.py, the 2 negative-duration tests fail.
The tests use no data (empty ids, timestamps only).
Merge after #3592 (stacked).

CI: all real checks green. Remaining failures are engine-review gates only (circular), plus Vercel Preview Built (deploy skipped by Ignored Build Step on non-frontend PRs, not a build failure): Engine Review Pass,Engine Review Pass,PR Blocker Checklist,Head SHA Check Guard,

CI: all real checks green. Remaining failures are engine-review gates only (circular), plus Vercel Preview Built (deploy skipped by Ignored Build Step on non-frontend PRs, not a build failure): Engine Review Pass,Engine Review Pass,visual-report (non-required),PR Blocker Checklist,Head SHA Check Guard,

PetrefiedThunder

REVIEW: PASS deepseek eb35d96

This follows up my earlier hand review. A dsh headless run on the full PR diff (merge-base..eb35d96) confirmed the PASS. Excerpt:

- `services/graph/app/fsma_recall/models.py:129` — the new condition `duration is not None and 0 <= duration <= sla_limit` correctly preserves `0s = MET`, `24h = MET`, `24h + 1s = BREACHED`, and now fails closed for negative durations.
- Verified behavior directly against both model versions: the parent returns `MET` for `-1s` and `-48h`; head returns `BREACHED`. The two negative-duration tests therefore meaningfully guard the regression.
- Ran the 7 new SLA tests at head: **7 passed**.
- The tests use only empty IDs and synthesized timestamps; no production data, fixtures, migrations, or tenant-scoped queries are touched. No tenant-scoping impact.

No concrete bugs found in the changed code or tests.

VERDICT: PASS

PetrefiedThunder

REVIEW: PASS grok eb35d96

services/graph/app/fsma_recall/models.py:123-130: completed drills are MET only when 0 <= duration <= 24h. Fixes two bugs: a 0 s completion was BREACHED via truthiness, and a negative duration (completed_at before started_at) is now fail-closed BREACHED rather than certifying compliance. No new enum value, so API consumers are unaffected.
Tests: test_recall_sla_negative_duration.py (-1 s, -48 h BREACHED; 0 MET) and test_recall_sla_zero_duration.py (0 MET, 24 h MET, 24 h+1 s BREACHED, missing completed_at uses elapsed). Timestamps only, empty ids; no invented data. Local: 7 passed.
CI: no code check failing; Engine Review Pass is the pending engine-review gate.

Posted with C.Lee's approval (12:37 AM PT).

# Conflicts: # services/graph/app/fsma_recall/models.py

PetrefiedThunder

REVIEW: PASS muse 759b856

Head checked: 759b856 (author Pi, branch pi/recall-sla-negative-duration).

Remerge-diff (git show --remerge-diff 759b856dc): the only conflict was in services/graph/app/fsma_recall/models.py, inside the SLA comparison. Since the old merge base, main's only change to models.py is ab2e59f (#3592, zero-second drills count as MET: duration is not None and duration <= sla_limit). The resolution duration is not None and 0 <= duration <= sla_limit keeps #3592's None check and its zero-inclusive behavior. No main change is dropped. The final diff against main for models.py is just that condition plus a comment.
Correctness: a completed drill with completed_at before started_at now returns BREACHED rather than MET, so it fails closed and adds no new enum value. Zero duration is still MET. Coverage: test_recall_sla_negative_duration.py tests -1s and -48h (BREACHED) and 0 (MET). test_recall_sla_zero_duration.py from main still passes.
Made-up data: none. The diff touches only models.py and one new test file. The test uses timestamps with empty ids and adds no CSV or sample data, and it doesn't touch /proof or the drill sample data.
Tests: I ran them in a temporary worktree at /Volumes/Extreme SSD/worktrees/muse-review-3598, using the existing venv from worktrees/ing-edi-wh-1017 (not the shared one), then removed the worktree. All 112 tests in the graph recall test files passed (test_fsma_recall, test_r3_recall_sla_24h, test_recall_contacts_csv, test_recall_deep_qa, test_recall_drill_create_api_key, test_recall_drill_metrics_2943, test_recall_sla_negative_duration, test_recall_sla_zero_duration). I excluded test_recall_drill_queue_isolation because 2 of its tests need the admin DATABASE_URL, which isn't set locally. That's an environment gap, not something this PR changed.
CI: when I checked, the full CI suite was running on this head, including PR Gate: Backend, Frontend and Workflow Contract. Head SHA Check Guard and PR Blocker Checklist had failed after 1s and need a re-run. Merge only once the required checks are green.

PetrefiedThunder added 2 commits October 24, 2026 22:34

fix(fsma-recall): count zero-second completed drills as SLA met

ffd29de

fix(fsma-recall): fail closed on negative completed drill duration

eb35d96

vercel Bot deployed to Preview October 25, 2026 07:06

View deployment

Base automatically changed from pi/recall-sla-zero-duration to main October 25, 2026 07:22

PetrefiedThunder commented Oct 25, 2026

View reviewed changes

PetrefiedThunder enabled auto-merge (squash) October 25, 2026 08:18

PetrefiedThunder commented Oct 25, 2026

View reviewed changes

PetrefiedThunder commented Oct 26, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into HEAD

759b856

# Conflicts: # services/graph/app/fsma_recall/models.py

vercel Bot deployed to Preview October 26, 2026 07:51

View deployment

PetrefiedThunder commented Oct 26, 2026

View reviewed changes

github-actions Bot added backend database labels Oct 26, 2026

PetrefiedThunder merged commit 206676d into main Oct 26, 2026
86 of 92 checks passed

PetrefiedThunder deleted the pi/recall-sla-negative-duration branch October 26, 2026 11:39

Navigation Menu

Repository navigation

fix(fsma-recall): fail closed on negative completed drill duration - #3598

fix(fsma-recall): fail closed on negative completed drill duration#3598
PetrefiedThunder merged 3 commits into
mainfrom
pi/recall-sla-negative-duration

PetrefiedThunder commented Oct 25, 2026 •

edited by cursor Bot

Loading

Uh oh!

vercel Bot commented Oct 25, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 25, 2026 •

edited

Loading

Uh oh!

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

edited

Loading

Uh oh!

github-actions Bot commented Oct 25, 2026 •

edited

Loading

Uh oh!

PetrefiedThunder left a comment

Uh oh!

PetrefiedThunder left a comment

Uh oh!

PetrefiedThunder left a comment

Uh oh!

PetrefiedThunder left a comment

Uh oh!

PetrefiedThunder left a comment

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

PetrefiedThunder commented Oct 25, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Decision: BREACHED, not a new "invalid" status

Change

Uh oh!

vercel Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

github-actions Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Blocker Checklist

Status: Ready

Uh oh!

PetrefiedThunder left a comment

Choose a reason for hiding this comment

Uh oh!

PetrefiedThunder left a comment

Choose a reason for hiding this comment

Uh oh!

PetrefiedThunder left a comment

Choose a reason for hiding this comment

Uh oh!

PetrefiedThunder left a comment

Choose a reason for hiding this comment

Uh oh!

PetrefiedThunder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

PetrefiedThunder commented Oct 25, 2026 •

edited by cursor Bot

Loading

vercel Bot commented Oct 25, 2026 •

edited

Loading

coderabbitai Bot commented Oct 25, 2026 •

edited

Loading

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

edited

Loading

github-actions Bot commented Oct 25, 2026 •

edited

Loading