Align service deadline checks with tracking window - #17
Conversation
intended-outcome: Make the operator deadline check reflect the 14-day implementation-tracking contract without customer delivery or refund promises, while preserving Day 0 compatibility and pause handling. verify: npm run check, npm test, npm run ci, git diff --check, and focused Grok review all pass; sgscan reports only pre-existing warnings. Co-Authored-By: Claude <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughThe deadline checker now monitors 14-day implementation-tracking windows. The window extends for overlapping pauses. The checker reports tracking status, missing state, missing Day 0, and attention conditions. ChangesImplementation tracking window monitoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ClientRecords
participant CheckServiceDeadlines
participant ServiceState
participant TrackingWindow
ClientRecords->>CheckServiceDeadlines: client folders
CheckServiceDeadlines->>ServiceState: load service state
ServiceState->>TrackingWindow: implementation acceptance and pauses
TrackingWindow-->>CheckServiceDeadlines: tracking-window end
CheckServiceDeadlines-->>ClientRecords: status and attention result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check-service-deadlines.mjs`:
- Around line 42-46: Update loadClients so a missing service-day0.json does not
return null or get removed by filter(Boolean); retain the client entry with an
empty Day 0 value, allowing assess to emit no-day0 and return exit code 1.
In `@scripts/lib/service-artifacts.mjs`:
- Around line 106-109: Update the pause-interval validation in the helper
containing the start/stop Date.parse logic to reject intervals where stop is
earlier than start, in addition to invalid timestamps. Throw the existing
invalid-interval error before calculating the Math.max/Math.min duration, while
preserving valid interval handling.
In `@scripts/test-service-engine.mjs`:
- Around line 447-451: Add a serviceTrackingWindowEndAt test assertion for a
pause whose endedAt precedes startedAt, expecting the helper to reject it with
the appropriate validation error. Update the helper’s pause-interval validation
so inverted intervals throw rather than being silently excluded, while
preserving valid pause handling.
- Around line 2189-2191: Update loadClients to retain client directories even
when service-day0.json is absent, allowing the checker to report the client and
return attention status. In the noDay0Id fixture, create only the client
directory and remove the aw call that creates service-day0.json with an empty
object, so the test exercises the missing-file case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 12c79571-6c04-4f3d-b472-72f6f160e8dc
📒 Files selected for processing (3)
scripts/check-service-deadlines.mjsscripts/lib/service-artifacts.mjsscripts/test-service-engine.mjs
| const path = join(dir, entry.name, "service-day0.json") | ||
| if (!existsSync(path)) return null | ||
| return {id: entry.name, day0: readJson(path)} | ||
| return {id: entry.name, folder: join(dir, entry.name), day0: readJson(path)} | ||
| }) | ||
| .filter(Boolean) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retain clients that have no Day 0 file.
When service-day0.json is absent, this code returns null and removes the client. The checker then cannot emit no-day0 or return exit code 1 for that client. Keep the directory in loadClients and use an empty Day 0 value so assess reports the missing record.
Proposed fix
.map((entry) => {
const path = join(dir, entry.name, "service-day0.json")
- if (!existsSync(path)) return null
- return {id: entry.name, folder: join(dir, entry.name), day0: readJson(path)}
+ return {
+ id: entry.name,
+ folder: join(dir, entry.name),
+ day0: existsSync(path) ? readJson(path) : {},
+ }
})
- .filter(Boolean)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const path = join(dir, entry.name, "service-day0.json") | |
| if (!existsSync(path)) return null | |
| return {id: entry.name, day0: readJson(path)} | |
| return {id: entry.name, folder: join(dir, entry.name), day0: readJson(path)} | |
| }) | |
| .filter(Boolean) | |
| const path = join(dir, entry.name, "service-day0.json") | |
| return { | |
| id: entry.name, | |
| folder: join(dir, entry.name), | |
| day0: existsSync(path) ? readJson(path) : {}, | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-service-deadlines.mjs` around lines 42 - 46, Update loadClients
so a missing service-day0.json does not return null or get removed by
filter(Boolean); retain the client entry with an empty Day 0 value, allowing
assess to emit no-day0 and return exit code 1.
| const start = Date.parse(pause.startedAt) | ||
| const stop = Date.parse(pause.endedAt) | ||
| if (Number.isNaN(start) || Number.isNaN(stop)) throw new Error("tracking pause interval is invalid") | ||
| return total + Math.max(0, Math.min(end, stop) - Math.max(anchor, start)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject pause intervals that end before they start.
Date.parse accepts both timestamps when endedAt precedes startedAt. The Math.max expression then treats the invalid interval as zero duration. The deadline checker consumes this helper directly, so malformed persisted data can report an earlier tracking-window end.
Proposed fix
const start = Date.parse(pause.startedAt)
const stop = Date.parse(pause.endedAt)
if (Number.isNaN(start) || Number.isNaN(stop)) throw new Error("tracking pause interval is invalid")
+ if (stop < start) throw new Error("tracking pause interval is invalid")
return total + Math.max(0, Math.min(end, stop) - Math.max(anchor, start))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const start = Date.parse(pause.startedAt) | |
| const stop = Date.parse(pause.endedAt) | |
| if (Number.isNaN(start) || Number.isNaN(stop)) throw new Error("tracking pause interval is invalid") | |
| return total + Math.max(0, Math.min(end, stop) - Math.max(anchor, start)) | |
| const start = Date.parse(pause.startedAt) | |
| const stop = Date.parse(pause.endedAt) | |
| if (Number.isNaN(start) || Number.isNaN(stop)) throw new Error("tracking pause interval is invalid") | |
| if (stop < start) throw new Error("tracking pause interval is invalid") | |
| return total + Math.max(0, Math.min(end, stop) - Math.max(anchor, start)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/service-artifacts.mjs` around lines 106 - 109, Update the
pause-interval validation in the helper containing the start/stop Date.parse
logic to reject intervals where stop is earlier than start, in addition to
invalid timestamps. Throw the existing invalid-interval error before calculating
the Math.max/Math.min duration, while preserving valid interval handling.
| eq(serviceTrackingWindowEndAt("2026-07-13T10:00:00.000Z"), "2026-07-27T10:00:00.000Z") | ||
| eq(serviceTrackingWindowEndAt("2026-07-13T10:00:00.000Z", [{reason: "Client access delay", startedAt: "2026-07-15T10:00:00.000Z", endedAt: "2026-07-17T10:00:00.000Z", durationMs: 172800000}]), "2026-07-29T10:00:00.000Z") | ||
| eq(serviceTrackingWindowEndAt("2026-07-13T10:00:00.000Z", [{reason: "Pre-acceptance delay", startedAt: "2026-07-11T10:00:00.000Z", endedAt: "2026-07-12T10:00:00.000Z", durationMs: 86400000}]), "2026-07-27T10:00:00.000Z") | ||
| eq(serviceTrackingWindowEndAt("2026-07-13T10:00:00.000Z", [{reason: "Straddling delay", startedAt: "2026-07-25T10:00:00.000Z", endedAt: "2026-07-28T10:00:00.000Z", durationMs: 259200000}]), "2026-07-30T10:00:00.000Z") | ||
| thr(() => serviceTrackingWindowEndAt("not-a-date"), /implementation acceptance timestamp is invalid/) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add coverage for an inverted pause interval.
The new tests reject an invalid acceptance timestamp. They do not reject a pause where endedAt is before startedAt. Add this assertion with the validation fix so the helper cannot silently exclude malformed pauses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-service-engine.mjs` around lines 447 - 451, Add a
serviceTrackingWindowEndAt test assertion for a pause whose endedAt precedes
startedAt, expecting the helper to reject it with the appropriate validation
error. Update the helper’s pause-interval validation so inverted intervals throw
rather than being silently excluded, while preserving valid pause handling.
| const noDay0Id = "550f5a54-84aa-7ae0-a1fd-4da350490005" | ||
| md(join(mixedDir, "clients", noDay0Id), {recursive: true}) | ||
| aw(join(mixedDir, "clients", noDay0Id, "service-day0.json"), {}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test a missing Day 0 file.
This fixture creates service-day0.json with {}. It does not test the missing-record case. loadClients currently skips a client directory when that file is absent, so the checker does not report the client or return attention status.
Create the client directory without service-day0.json after updating loadClients to retain that client.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-service-engine.mjs` around lines 2189 - 2191, Update loadClients
to retain client directories even when service-day0.json is absent, allowing the
checker to report the client and return attention status. In the noDay0Id
fixture, create only the client directory and remove the aw call that creates
service-day0.json with an empty object, so the test exercises the missing-file
case.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e257cb78b4
ℹ️ 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".
| if (toRefund <= 0) status = "REFUND-DUE" | ||
| else if (toTarget <= 0) status = "TARGET-MISSED" | ||
| else if (toTarget <= WARN_WITHIN_DAYS) status = "due-soon" | ||
| if (state.state === "tracking-14-day" && state.implementationAcceptedAt) { |
There was a problem hiding this comment.
When a tracking review returns needs-info, transitionFor creates the valid state {state: "needs-info", resumeState: "tracking-14-day"} while preserving implementationAcceptedAt; this interval is not excluded from active tracking unless a separate Day 0 pause is recorded. This condition therefore falls through to tracking-not-started, and an overdue client can produce exit code 0 until the information request is resolved. Treat a needs-info state whose resumeState is tracking-14-day as an active tracking window.
Useful? React with 👍 / 👎.
| const windowEnd = serviceTrackingWindowEndAt(state.implementationAcceptedAt, day0.pauseHistory || []) | ||
| const toEnd = businessDaysBetween(asOf, windowEnd) | ||
| let status = "on-track" | ||
| if (toEnd <= 0) status = "TRACKING-OVERDUE" |
There was a problem hiding this comment.
If implementation is accepted on a weekend, the 14-calendar-day window also ends on a weekend; before that end instant, businessMillisecondsBetween returns zero because the remaining interval contains no business time. For example, acceptance at Saturday 2026-07-18 10:00Z and checking at 09:00Z on Saturday 2026-08-01 produces toEnd === 0, so this line reports TRACKING-OVERDUE one hour early. Determine overdue from the actual timestamps and use business-day distance only for the warning/display calculation.
Useful? React with 👍 / 👎.
1 participant
Intended outcome
Make the operator deadline check reflect the 14-day implementation-tracking contract without customer delivery or refund promises, while preserving Day 0 compatibility and pause handling.
Verification
/home/nish/.local/bin/test-gate npm run check— passed (126 checks)/home/nish/.local/bin/test-gate npm test— passed (126 checks)/home/nish/.local/bin/test-gate npm run ci— passed (126 checks)git diff --check— passedsgscan— only pre-existing warnings; no diff findingsResidual seven-day wording in separate client-facing surfaces is follow-up scope, not introduced by this PR.
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit