feat(scheduling): wire workshop appointment booking#2017
Conversation
📝 WalkthroughWalkthroughScheduling navigation and localization are expanded, rehearsal appointments can be booked against selected resource calendars, calendar and onboarding entry points are updated, lazy routes are recognized by lint tooling, and dogfood deployment accepts a selected ref. ChangesScheduling experience
Lazy route lint support
Dogfood ref deployment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FrontOfficeRehearsal
participant bookAppointment
participant SchedulingDefinitionRoutes
participant IBookingService
User->>FrontOfficeRehearsal: select resource and review appointment
FrontOfficeRehearsal->>bookAppointment: submit appointment details
bookAppointment->>SchedulingDefinitionRoutes: POST scheduling appointment
SchedulingDefinitionRoutes->>IBookingService: validate and book appointment
IBookingService-->>SchedulingDefinitionRoutes: booking result
SchedulingDefinitionRoutes-->>FrontOfficeRehearsal: eventId and booked status
FrontOfficeRehearsal-->>User: show booking success
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
🧹 Nitpick comments (8)
tooling/preview-host/deploy-dogfood.ps1 (1)
66-69: 🩺 Stability & Availability | 🔵 Trivialchore (blocking): Add clean-checkout coverage for ref selection
Please add or link CI/operational coverage that exercises the script with
DOGFOOD_REFunset and set to an explicit fetched ref, verifies the resolved SHA, and proves failed fetch/checkout paths cannot reachStop-Service. The real deployment path and its offline/alternative test path should not be skip-gated, and this high-blast change needs an independent reviewer.As per path instructions, CI must exercise modified paths, verification must be clean-checkout reproducible, real and offline paths must be covered, and high-blast changes must not rely on self-assertion.
Also applies to: 113-131
🤖 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 `@tooling/preview-host/deploy-dogfood.ps1` around lines 66 - 69, Add clean-checkout CI or operational coverage for deploy-dogfood.ps1 that runs with DOGFOOD_REF unset and set to an explicit fetched ref, verifies the selected commit SHA, and confirms fetch or checkout failures cannot reach Stop-Service. Ensure both the real deployment path and offline/alternative test path execute without skip gating, make the verification reproducible from a clean checkout, and require an independent reviewer for this high-blast change.Source: Path instructions
apps/carrier/src/navigation/permissions.tsx (1)
92-92: 🔒 Security & Privacy | 🔵 Trivialchore [blocking]: Require independent review for this booking permission grant.
This permission exposes the scheduling surface, which is part of the CP-bearing booking flow. Please obtain independent reviewer sign-off and verify the builder-specific permission path before merge; the fully privileged navigation test does not prove this role mapping.
As per path instructions, CP-touching changes require an independent reviewer rather than self-assertion (PROC-A2).
🤖 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 `@apps/carrier/src/navigation/permissions.tsx` at line 92, Obtain independent reviewer sign-off for the scheduling:design permission change and verify the builder-specific permission path and its role-mapping test; do not rely solely on the fully privileged navigation test or self-assertion.Source: Path instructions
apps/carrier/src/i18n/locales/ru-RU.ts (1)
874-874: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion [non-blocking]: Keep one canonical scheduling label key.
everything-pack.ts:116androutes/scheduling.tsx:5-8usescheduling.route.title; this newnav.schedulingkey is not the contract for the added Workshop group. Remove it or update consumers to avoid label drift.As per path instructions, avoid hand-parallel duplicate copies of single-source things (PROC-A4).
🤖 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 `@apps/carrier/src/i18n/locales/ru-RU.ts` at line 874, Remove the duplicate nav.scheduling translation entry from the locale, and keep the existing scheduling.route.title key as the canonical label used by everything-pack.ts and routes/scheduling.tsx. Do not add or update consumers to use the duplicate key.Source: Path instructions
apps/carrier/src/i18n/locales/nl-NL.ts (1)
874-874: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion [non-blocking]: Keep one canonical scheduling label key.
everything-pack.ts:116androutes/scheduling.tsx:5-8usescheduling.route.title; this newnav.schedulingkey is not the contract for the added Workshop group. Remove it or update consumers to avoid label drift.As per path instructions, avoid hand-parallel duplicate copies of single-source things (PROC-A4).
🤖 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 `@apps/carrier/src/i18n/locales/nl-NL.ts` at line 874, Remove the duplicate nav.scheduling translation entry in the nl-NL locale and retain the canonical scheduling.route.title key used by everything-pack and routes/scheduling. Do not add or update consumers for nav.scheduling.Source: Path instructions
apps/carrier/src/navigation/__tests__/resolveNav.test.ts (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion [non-blocking]: Test the changed builder-role path.
This test resolves navigation with
FULL, so it only proves that scheduling appears for fully privileged users. Add a focused assertion usingNAV_ROLE_PERMISSIONS.builderto verify thatscheduling:designactually makes the scheduling item visible.As per path instructions, CI must exercise modified code paths (PROC-A1), with clean-checkout reproducible verification (PROC-A7).
🤖 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 `@apps/carrier/src/navigation/__tests__/resolveNav.test.ts` at line 58, Add a focused navigation test using NAV_ROLE_PERMISSIONS.builder, asserting that the scheduling:design permission makes the scheduling item visible. Keep the existing FULL-role coverage intact and ensure the new assertion exercises the modified builder-role path.Source: Path instructions
apps/carrier/src/i18n/locales/pt-PT.ts (1)
874-874: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion [non-blocking]: Keep one canonical scheduling label key.
everything-pack.ts:116androutes/scheduling.tsx:5-8usescheduling.route.title; this newnav.schedulingkey is not the contract for the added Workshop group. Remove it or update consumers to avoid label drift.As per path instructions, avoid hand-parallel duplicate copies of single-source things (PROC-A4).
🤖 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 `@apps/carrier/src/i18n/locales/pt-PT.ts` at line 874, Remove the newly added nav.scheduling translation from the pt-PT locale and keep scheduling.route.title as the canonical label used by everything-pack.ts and routes/scheduling.tsx. Do not add alternate consumer keys or duplicate scheduling labels.Source: Path instructions
apps/carrier/src/i18n/locales/pt-BR.ts (1)
874-874: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion [non-blocking]: Keep one canonical scheduling label key.
everything-pack.ts:116androutes/scheduling.tsx:5-8usescheduling.route.title; this newnav.schedulingkey is not the contract for the added Workshop group. Remove it or update consumers to avoid label drift.As per path instructions, avoid hand-parallel duplicate copies of single-source things (PROC-A4).
🤖 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 `@apps/carrier/src/i18n/locales/pt-BR.ts` at line 874, Remove the newly added nav.scheduling label from the locale and keep scheduling.route.title as the single canonical scheduling label used by everything-pack.ts and routes/scheduling.tsx. Do not add parallel scheduling label keys or update consumers to use the redundant key.Source: Path instructions
apps/carrier/src/scheduling/product/client.ts (1)
139-150: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: surface the server's rejection code instead of only the HTTP status
SchedulingRequestErroronly carriesresponse.status; the JSON body'scode(e.g.scheduling.appointment.slot_conflict,scheduling.appointment.no_availability,scheduling.appointment.resource_invalid) is discarded. The UI currently can only show one generic error message for every rejection reason, even though the booking service returns a specific, actionable reason.♻️ Suggested enhancement
export async function bookAppointment(input: { subjectId: string resource: string title: string startUtc: string endUtc: string }): Promise<{ eventId: string; status: 'booked' }> { const node = await requireNode() const response = await nodePost(node, '/api/local-node/scheduling/appointments', input) - if (!response.ok) throw new SchedulingRequestError(response.status) + if (!response.ok) { + const body = await response.json().catch(() => null) as { code?: string } | null + throw new SchedulingRequestError(response.status, body?.code) + } return response.json() as Promise<{ eventId: string; status: 'booked' }> }🤖 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 `@apps/carrier/src/scheduling/product/client.ts` around lines 139 - 150, Update bookAppointment to read the rejected response JSON and pass its server-provided code into SchedulingRequestError, while preserving the HTTP status and existing successful response behavior. Ensure rejection handling supports codes such as scheduling.appointment.slot_conflict, scheduling.appointment.no_availability, and scheduling.appointment.resource_invalid.
🤖 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 `@apps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsx`:
- Around line 88-108: The booking flow in book must reset booked to false before
starting a new booking attempt, so a prior successful appointment does not keep
the primary button disabled. Preserve setting booked to true after success and
the existing disabled condition.
- Around line 88-101: Reset or invalidate the checked result when subjectId
changes, matching the existing definitionId change behavior, so book() cannot
use stale result.contactName with the current subjectId. Ensure the review and
booking flow require a fresh check before calling bookAppointment.
- Around line 88-101: Update book() to parse startsAt as a wall-clock time in
selectedDefinition.timezone, matching check(), instead of constructing it with
new Date(startsAt). Use the timezone-aware parsed instant to calculate the
duration and populate startUtc and endUtc.
In `@apps/local-node-host/Health/SchedulingDefinitionRoutes.cs`:
- Around line 92-109: Update the request validation in the AppointmentRoute
handler to reject null, empty, or whitespace request.SubjectId alongside the
existing Title and time checks. Return the existing
scheduling.appointment.invalid 400 response before calling ParticipantRef.Party.
In `@apps/local-node-host/tests/Scheduling/SchedulingDefinitionRouteTests.cs`:
- Around line 233-255: The SchedulingDefinitionRouteTests coverage only verifies
denied and invalid appointment requests, not successful booking or overlap
rejection. Add tests alongside Appointment_booking_requires_scheduling_operate
and Appointment_booking_rejects_an_invalid_resource_without_writing that grant
SchedulingOperate, assert an available appointment returns 200 with status
"booked", then submit the same resource and overlapping slot again and assert
409 with code "scheduling.appointment.slot_conflict".
In `@tooling/label-continuity-lint/check.mjs`:
- Around line 102-107: Update the route scanning logic around lazyPathRe so each
path is paired only with its own lazy import, rather than searching an arbitrary
500-character slice that can reach the next route. Use a single route-entry
pattern that captures the path and its direct import together, or otherwise
parse route-object boundaries structurally before updating segToFile.
In `@tooling/preview-host/deploy-dogfood.ps1`:
- Around line 66-69: Update the config-copy logic near the dogfood deployment
flow to source dogfood-appsettings.Production.json from $Worktree instead of
$RepoRoot, keeping the template aligned with the selected $DeployRef and
deployed worktree revision.
- Around line 113-131: Update the worktree preparation flow around git fetch,
git checkout, and git worktree add to explicitly validate each command’s exit
status and terminate on failure. Ensure the script never proceeds to rev-parse
or logs $headSha when selecting $DeployRef fails, preventing a stale worktree
commit from being treated as current.
---
Nitpick comments:
In `@apps/carrier/src/i18n/locales/nl-NL.ts`:
- Line 874: Remove the duplicate nav.scheduling translation entry in the nl-NL
locale and retain the canonical scheduling.route.title key used by
everything-pack and routes/scheduling. Do not add or update consumers for
nav.scheduling.
In `@apps/carrier/src/i18n/locales/pt-BR.ts`:
- Line 874: Remove the newly added nav.scheduling label from the locale and keep
scheduling.route.title as the single canonical scheduling label used by
everything-pack.ts and routes/scheduling.tsx. Do not add parallel scheduling
label keys or update consumers to use the redundant key.
In `@apps/carrier/src/i18n/locales/pt-PT.ts`:
- Line 874: Remove the newly added nav.scheduling translation from the pt-PT
locale and keep scheduling.route.title as the canonical label used by
everything-pack.ts and routes/scheduling.tsx. Do not add alternate consumer keys
or duplicate scheduling labels.
In `@apps/carrier/src/i18n/locales/ru-RU.ts`:
- Line 874: Remove the duplicate nav.scheduling translation entry from the
locale, and keep the existing scheduling.route.title key as the canonical label
used by everything-pack.ts and routes/scheduling.tsx. Do not add or update
consumers to use the duplicate key.
In `@apps/carrier/src/navigation/__tests__/resolveNav.test.ts`:
- Line 58: Add a focused navigation test using NAV_ROLE_PERMISSIONS.builder,
asserting that the scheduling:design permission makes the scheduling item
visible. Keep the existing FULL-role coverage intact and ensure the new
assertion exercises the modified builder-role path.
In `@apps/carrier/src/navigation/permissions.tsx`:
- Line 92: Obtain independent reviewer sign-off for the scheduling:design
permission change and verify the builder-specific permission path and its
role-mapping test; do not rely solely on the fully privileged navigation test or
self-assertion.
In `@apps/carrier/src/scheduling/product/client.ts`:
- Around line 139-150: Update bookAppointment to read the rejected response JSON
and pass its server-provided code into SchedulingRequestError, while preserving
the HTTP status and existing successful response behavior. Ensure rejection
handling supports codes such as scheduling.appointment.slot_conflict,
scheduling.appointment.no_availability, and
scheduling.appointment.resource_invalid.
In `@tooling/preview-host/deploy-dogfood.ps1`:
- Around line 66-69: Add clean-checkout CI or operational coverage for
deploy-dogfood.ps1 that runs with DOGFOOD_REF unset and set to an explicit
fetched ref, verifies the selected commit SHA, and confirms fetch or checkout
failures cannot reach Stop-Service. Ensure both the real deployment path and
offline/alternative test path execute without skip gating, make the verification
reproducible from a clean checkout, and require an independent reviewer for this
high-blast change.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b68b4ee4-0359-47a8-9726-21cd3a7e2e1e
📒 Files selected for processing (37)
apps/carrier/src/first-run/genesisState.test.tsapps/carrier/src/first-run/genesisState.tsapps/carrier/src/i18n/locales/ar-AE.tsapps/carrier/src/i18n/locales/ar.tsapps/carrier/src/i18n/locales/bn.tsapps/carrier/src/i18n/locales/de-DE.tsapps/carrier/src/i18n/locales/en-GB.tsapps/carrier/src/i18n/locales/en-US.tsapps/carrier/src/i18n/locales/es-419.tsapps/carrier/src/i18n/locales/es-ES.tsapps/carrier/src/i18n/locales/fr-CA.tsapps/carrier/src/i18n/locales/fr-FR.tsapps/carrier/src/i18n/locales/hi-IN.tsapps/carrier/src/i18n/locales/it-IT.tsapps/carrier/src/i18n/locales/ja-JP.tsapps/carrier/src/i18n/locales/nl-NL.tsapps/carrier/src/i18n/locales/pt-BR.tsapps/carrier/src/i18n/locales/pt-PT.tsapps/carrier/src/i18n/locales/ru-RU.tsapps/carrier/src/i18n/locales/ur-PK.tsapps/carrier/src/i18n/locales/ur.tsapps/carrier/src/i18n/locales/zh-CN.tsapps/carrier/src/i18n/locales/zh-TW.tsapps/carrier/src/navigation/__tests__/resolveNav.test.tsapps/carrier/src/navigation/everything-pack.tsapps/carrier/src/navigation/fragments.tsxapps/carrier/src/navigation/permissions.tsxapps/carrier/src/routes/calendar.tsxapps/carrier/src/scheduling/product/FrontOfficeRehearsal.test.tsxapps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsxapps/carrier/src/scheduling/product/SchedulingHome.tsxapps/carrier/src/scheduling/product/client.tsapps/local-node-host/Health/HostedSchedulingApiEndpoint.csapps/local-node-host/Health/SchedulingDefinitionRoutes.csapps/local-node-host/tests/Scheduling/SchedulingDefinitionRouteTests.cstooling/label-continuity-lint/check.mjstooling/preview-host/deploy-dogfood.ps1
| async function book() { | ||
| if (!result || !selectedDefinition || !resource) return | ||
| setBooking(true) | ||
| setError(null) | ||
| try { | ||
| const start = new Date(startsAt) | ||
| const end = new Date(start.getTime() + selectedDefinition.durationMinutes * 60_000) | ||
| await bookAppointment({ | ||
| subjectId, | ||
| resource, | ||
| title: `${selectedDefinition.name} — ${result.contactName}`, | ||
| startUtc: start.toISOString(), | ||
| endUtc: end.toISOString(), | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
thought: booking uses live subjectId/resource but a stale result.contactName
If the user edits the contact selection after check() but before clicking Book, book()'s title still reads result.contactName from the earlier check, while the actual subjectId sent to bookAppointment reflects the new selection — the booked appointment's subject and the displayed review can diverge. Resetting result (as already done for definitionId) when subjectId changes would close this gap.
🤖 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 `@apps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsx` around lines 88
- 101, Reset or invalidate the checked result when subjectId changes, matching
the existing definitionId change behavior, so book() cannot use stale
result.contactName with the current subjectId. Ensure the review and booking
flow require a fresh check before calling bookAppointment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A10 'function validateLocalDateTime' apps/carrier/src
rg -n -A15 'export function DateTimeField' apps/carrier/srcRepository: Harborline-Software/shipyard
Length of output: 1374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- apps/carrier/src/scheduling/product/rehearsal.ts ---'
cat -n apps/carrier/src/scheduling/product/rehearsal.ts | sed -n '1,220p'
echo
echo '--- apps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsx ---'
cat -n apps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsx | sed -n '1,220p'Repository: Harborline-Software/shipyard
Length of output: 12544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- apps/carrier/src/scheduling/product/rehearsal.ts ---'
cat -n apps/carrier/src/scheduling/product/rehearsal.ts | sed -n '1,220p'
echo
echo '--- apps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsx ---'
cat -n apps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsx | sed -n '1,260p'Repository: Harborline-Software/shipyard
Length of output: 12544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' apps/carrier/src/scheduling/product/rehearsal.ts | nl -ba
printf '\n---\n'
sed -n '1,260p' apps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsx | nl -baRepository: Harborline-Software/shipyard
Length of output: 206
issue [blocking]: parse startsAt in the protocol timezone
check() treats the input as a wall-clock in selectedDefinition.timezone, but book() still does new Date(startsAt). A bare YYYY-MM-DDTHH:mm will be interpreted in the browser’s local timezone, so startUtc/endUtc can land on the wrong slot. Convert it with a timezone-aware parse before sending the appointment.
🤖 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 `@apps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsx` around lines 88
- 101, Update book() to parse startsAt as a wall-clock time in
selectedDefinition.timezone, matching check(), instead of constructing it with
new Date(startsAt). Use the timezone-aware parsed instant to calculate the
duration and populate startUtc and endUtc.
| async function book() { | ||
| if (!result || !selectedDefinition || !resource) return | ||
| setBooking(true) | ||
| setError(null) | ||
| try { | ||
| const start = new Date(startsAt) | ||
| const end = new Date(start.getTime() + selectedDefinition.durationMinutes * 60_000) | ||
| await bookAppointment({ | ||
| subjectId, | ||
| resource, | ||
| title: `${selectedDefinition.name} — ${result.contactName}`, | ||
| startUtc: start.toISOString(), | ||
| endUtc: end.toISOString(), | ||
| }) | ||
| setBooked(true) | ||
| } catch (cause: unknown) { | ||
| setError(cause instanceof Error ? cause.message : t('scheduling.errors.appointmentBook')) | ||
| } finally { | ||
| setBooking(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
issue: booking is permanently disabled after the first success
booked is set to true on a successful booking but is never reset back to false. The primary button's disabled condition (disabled={booking || booked || !resource}, line 146) means that once any booking succeeds, the Book button stays disabled for the rest of the session — the user cannot book a second appointment without leaving and re-entering this screen.
♻️ Suggested fix
function check() {
setChecking(true)
setResult(null)
+ setBooked(false)
const contact = contacts.find((item) => item.id === subjectId)Also applies to: 145-149
🤖 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 `@apps/carrier/src/scheduling/product/FrontOfficeRehearsal.tsx` around lines 88
- 108, The booking flow in book must reset booked to false before starting a new
booking attempt, so a prior successful appointment does not keep the primary
button disabled. Preserve setting booked to true after success and the existing
disabled condition.
|
|
||
| app.MapPost(AppointmentRoute, async (SchedulingAppointmentRequest request, CancellationToken ct) => | ||
| { | ||
| if (!authz.HasPermission(Permission.SchedulingOperate)) | ||
| return Denied(Permission.SchedulingOperate); | ||
| if (string.IsNullOrWhiteSpace(request.Title) || request.EndUtc <= request.StartUtc) | ||
| return Results.BadRequest(new { code = "scheduling.appointment.invalid" }); | ||
| var resource = ParseParticipant(request.Resource); | ||
| if (resource is null) | ||
| return Results.BadRequest(new { code = "scheduling.appointment.resource_invalid" }); | ||
|
|
||
| var outcome = await bookingService.Book( | ||
| NodeTenant.Resolve(activeTeam), resource, request.Title.Trim(), request.StartUtc, | ||
| request.EndUtc, LocalSchedulingActor, ParticipantRef.Party(request.SubjectId), ct: ct) | ||
| .ConfigureAwait(false); | ||
| return outcome.Success | ||
| ? Results.Ok(new { eventId = outcome.Event!.Id.Value, status = "booked" }) | ||
| : Results.Conflict(new { code = $"scheduling.appointment.{outcome.RejectionReason!.ToLowerInvariant()}" }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'static ParticipantRef Party($ARG) { $$$ }' --lang csharp packages/blocks-calendar
rg -n -A5 'static ParticipantRef Party' packages/blocks-calendarRepository: Harborline-Software/shipyard
Length of output: 826
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/blocks-calendar/Models/ParticipantRef.cs
printf '\n---\n'
rg -n -A6 -B6 'class PartyRef|record PartyRef|PartyRef\(' packages/blocks-calendar/ModelsRepository: Harborline-Software/shipyard
Length of output: 8995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A8 -B8 'class SchedulingAppointmentRequest|record SchedulingAppointmentRequest|SubjectId' apps/local-node-host packages/blocks-calendar
printf '\n---\n'
rg -n -A6 -B6 'UseExceptionHandler|app.Use|MapPost\(AppointmentRoute|Results\.BadRequest' apps/local-node-hostRepository: Harborline-Software/shipyard
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A20 -B10 'record SchedulingAppointmentRequest|class SchedulingAppointmentRequest|SchedulingAppointmentRequest\s*\(' apps/local-node-host/Health apps/local-node-host -g '*.cs'
printf '\n---\n'
rg -n -A8 -B8 'UseExceptionHandler|AddExceptionHandler|app\.UseExceptionHandler|exception handler|MapPost\(AppointmentRoute' apps/local-node-host -g '*.cs'Repository: Harborline-Software/shipyard
Length of output: 4649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A8 -B8 'UseExceptionHandler|AddExceptionHandler|UseDeveloperExceptionPage|exception handler|ProblemDetails|MapGroup\(".*Health|app\.MapPost\(AppointmentRoute' apps/local-node-host -g '*.cs'Repository: Harborline-Software/shipyard
Length of output: 2071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'Program.cs' apps/local-node-host
printf '\n---\n'
rg -n -A12 -B12 'UseExceptionHandler|AddExceptionHandler|UseDeveloperExceptionPage|app\.Use|MapPost\(AppointmentRoute|ArgumentException' apps/local-node-host/Program.cs apps/local-node-host -g '*.cs'Repository: Harborline-Software/shipyard
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
program=$(fd -a '^Program\.cs$' apps/local-node-host | head -n 1)
echo "$program"
printf '\n---\n'
cat -n "$program" | sed -n '1,260p'Repository: Harborline-Software/shipyard
Length of output: 16242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'UseExceptionHandler|AddExceptionHandler|UseDeveloperExceptionPage|DeveloperExceptionPage|ExceptionHandler|UseStatusCodePages|UseMiddleware<.*Exception' apps/local-node-host -g '*.cs'Repository: Harborline-Software/shipyard
Length of output: 166
issue [blocking]: validate SubjectId before calling ParticipantRef.Party
ParticipantRef.Party already rejects empty/whitespace ids, so a bad SubjectId here raises an ArgumentException and bubbles as a 500. Add it to the existing request validation and return the same 400 as the other invalid fields.
🤖 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 `@apps/local-node-host/Health/SchedulingDefinitionRoutes.cs` around lines 92 -
109, Update the request validation in the AppointmentRoute handler to reject
null, empty, or whitespace request.SubjectId alongside the existing Title and
time checks. Return the existing scheduling.appointment.invalid 400 response
before calling ParticipantRef.Party.
| [Fact] | ||
| public async Task Appointment_booking_requires_scheduling_operate() | ||
| { | ||
| var response = await _client.PostAsJsonAsync(SchedulingDefinitionRoutes.AppointmentRoute, | ||
| new { subjectId = "subject-1", resource = "party:resource-1", title = "Visit", | ||
| startUtc = "2026-07-13T13:00:00Z", endUtc = "2026-07-13T13:30:00Z" }); | ||
|
|
||
| Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task Appointment_booking_rejects_an_invalid_resource_without_writing() | ||
| { | ||
| _principal.Grant(Permission.SchedulingOperate); | ||
| var response = await _client.PostAsJsonAsync(SchedulingDefinitionRoutes.AppointmentRoute, | ||
| new { subjectId = "subject-1", resource = "unknown:resource-1", title = "Visit", | ||
| startUtc = "2026-07-13T13:00:00Z", endUtc = "2026-07-13T13:30:00Z" }); | ||
|
|
||
| Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); | ||
| var body = await response.Content.ReadFromJsonAsync<JsonElement>(); | ||
| Assert.Equal("scheduling.appointment.resource_invalid", body.GetProperty("code").GetString()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
issue: no test covers a successful booking or the no-double-booking rejection
The two new tests only cover permission-denial and invalid-resource inputs. The actual booking behavior this route introduces — a successful 200 booking, and rejection of a conflicting/overlapping slot (SLOT_CONFLICT → 409) — is never exercised.
As per path instructions, "Ensure CI actually exercises the modified code paths (PROC-A1)" and "Treat booking/concurrency/slotting as CP-5—preserve single-winner/uniqueness guarantees (no double-booking...)". Without a test that books once and then attempts a second overlapping booking, the no-double-booking guarantee for this new endpoint is unverified by CI.
Consider adding:
- A happy-path test asserting
200 OKwith{ status: "booked" }when availability exists. - A conflict test that books the same resource/slot twice and asserts the second call returns
409with ascheduling.appointment.slot_conflictcode.
🤖 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 `@apps/local-node-host/tests/Scheduling/SchedulingDefinitionRouteTests.cs`
around lines 233 - 255, The SchedulingDefinitionRouteTests coverage only
verifies denied and invalid appointment requests, not successful booking or
overlap rejection. Add tests alongside
Appointment_booking_requires_scheduling_operate and
Appointment_booking_rejects_an_invalid_resource_without_writing that grant
SchedulingOperate, assert an available appointment returns 200 with status
"booked", then submit the same resource and overlapping slot again and assert
409 with code "scheduling.appointment.slot_conflict".
Source: Path instructions
| const lazyPathRe = /path:\s*'([^']+)'\s*,\s*lazy:/g; | ||
| while ((rm = lazyPathRe.exec(src)) !== null) { | ||
| const routeBody = src.slice(rm.index, rm.index + 500); | ||
| const dynamicImport = routeBody.match(/import\(\s*'(\.\/routes\/[^']+)'\s*\)/); | ||
| if (dynamicImport) { | ||
| segToFile.set(rm[1], `apps/carrier/src/${dynamicImport[1].replace(/^\.\//, '')}.tsx`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
issue (blocking): Bind the import to the current route entry
The 500-character slice is not a route boundary. If a lazy entry uses a helper or lacks a direct import, this loop can consume the next route’s import() and map the wrong file to the current path. Capture the path and import from the same route pattern, or parse route-object boundaries structurally.
Proposed fix
- const lazyPathRe = /path:\s*'([^']+)'\s*,\s*lazy:/g;
- while ((rm = lazyPathRe.exec(src)) !== null) {
- const routeBody = src.slice(rm.index, rm.index + 500);
- const dynamicImport = routeBody.match(/import\(\s*'(\.\/routes\/[^']+)'\s*\)/);
- if (dynamicImport) {
- segToFile.set(rm[1], `apps/carrier/src/${dynamicImport[1].replace(/^\.\//, '')}.tsx`);
- }
- }
+ const lazyRouteRe =
+ /\{\s*path:\s*'([^']+)'\s*,\s*lazy:\s*(?:async\s*)?\(\s*\)\s*=>\s*import\(\s*'(\.\/routes\/[^']+)'\s*\)/g;
+ let lm;
+ while ((lm = lazyRouteRe.exec(src)) !== null) {
+ segToFile.set(lm[1], `apps/carrier/src/${lm[2].replace(/^\.\//, '')}.tsx`);
+ }📝 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 lazyPathRe = /path:\s*'([^']+)'\s*,\s*lazy:/g; | |
| while ((rm = lazyPathRe.exec(src)) !== null) { | |
| const routeBody = src.slice(rm.index, rm.index + 500); | |
| const dynamicImport = routeBody.match(/import\(\s*'(\.\/routes\/[^']+)'\s*\)/); | |
| if (dynamicImport) { | |
| segToFile.set(rm[1], `apps/carrier/src/${dynamicImport[1].replace(/^\.\//, '')}.tsx`); | |
| const lazyRouteRe = | |
| /\{\s*path:\s*'([^']+)'\s*,\s*lazy:\s*(?:async\s*)?\(\s*\)\s*=>\s*import\(\s*'(\.\/routes\/[^']+)'\s*\)/g; | |
| let lm; | |
| while ((lm = lazyRouteRe.exec(src)) !== null) { | |
| segToFile.set(lm[1], `apps/carrier/src/${lm[2].replace(/^\.\//, '')}.tsx`); | |
| } |
🤖 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 `@tooling/label-continuity-lint/check.mjs` around lines 102 - 107, Update the
route scanning logic around lazyPathRe so each path is paired only with its own
lazy import, rather than searching an arbitrary 500-character slice that can
reach the next route. Use a single route-entry pattern that captures the path
and its direct import together, or otherwise parse route-object boundaries
structurally before updating segToFile.
| # Internal dogfood may intentionally exercise an unmerged branch. Production-shaped use keeps the | ||
| # default origin/main; an operator can set DOGFOOD_REF to an explicit fetched remote ref. | ||
| $DeployRef = if ($env:DOGFOOD_REF) { $env:DOGFOOD_REF } else { 'origin/main' } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the deployment script and inspect the relevant sections with line numbers.
git ls-files tooling/preview-host/deploy-dogfood.ps1
wc -l tooling/preview-host/deploy-dogfood.ps1
sed -n '1,220p' tooling/preview-host/deploy-dogfood.ps1 | cat -n
# Find where the copied config file is sourced from and how $Worktree/$RepoRoot are used.
rg -n "\$Worktree|\$RepoRoot|dogfood-appsettings\.Production\.json|DOGFOOD_REF|origin/main" tooling/preview-host/deploy-dogfood.ps1 tooling/preview-host -SRepository: Harborline-Software/shipyard
Length of output: 18619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect nearby docs or scripts that describe dogfood deployment behavior.
git ls-files tooling/preview-host | sed -n '1,120p'
rg -n "dogfood-appsettings\.Production\.json|DOGFOOD_REF|preview-host|dogfood" -S README.md tooling _shared 2>/dev/null || trueRepository: Harborline-Software/shipyard
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the config template and the documented deployment contract around it.
cat -n tooling/preview-host/dogfood-appsettings.Production.json | sed -n '1,220p'
printf '\n--- DOGFOOD.md excerpt ---\n'
sed -n '170,190p;440,452p;476,486p' tooling/preview-host/DOGFOOD.md | cat -nRepository: Harborline-Software/shipyard
Length of output: 5492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether any script copies appsettings from the worktree instead of the repo root.
rg -n "appsettings\.Production\.json|Copy-Item .*dogfood-appsettings|Join-Path .*appsettings" tooling/preview-host -SRepository: Harborline-Software/shipyard
Length of output: 1238
issue (blocking): keep config on the same revision as the worktree
tooling/preview-host/deploy-dogfood.ps1:149-150 still copies dogfood-appsettings.Production.json from $RepoRoot. With a custom DOGFOOD_REF, that mixes binaries/web assets from one commit with config from another checkout. Copy the template from $Worktree, or pin the config source to the same ref explicitly, so deploys stay reproducible.
🤖 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 `@tooling/preview-host/deploy-dogfood.ps1` around lines 66 - 69, Update the
config-copy logic near the dogfood deployment flow to source
dogfood-appsettings.Production.json from $Worktree instead of $RepoRoot, keeping
the template aligned with the selected $DeployRef and deployed worktree
revision.
Source: Path instructions
| Write-Host "[deploy-dogfood] Fetching remote refs for $DeployRef..." | ||
| git fetch origin | ||
|
|
||
| if (Test-Path $Worktree) { | ||
| Write-Host "[deploy-dogfood] Updating existing worktree to origin/main..." | ||
| Write-Host "[deploy-dogfood] Updating existing worktree to $DeployRef..." | ||
| Push-Location $Worktree | ||
| git checkout --detach origin/main | ||
| git checkout --detach $DeployRef | ||
| Pop-Location | ||
| } else { | ||
| Write-Host "[deploy-dogfood] Creating worktree at $Worktree pinned to origin/main..." | ||
| git worktree add $Worktree origin/main | ||
| Write-Host "[deploy-dogfood] Creating worktree at $Worktree pinned to $DeployRef..." | ||
| git worktree add $Worktree $DeployRef | ||
| } | ||
| } | ||
| finally { | ||
| Pop-Location | ||
| } | ||
|
|
||
| $headSha = (git -C $Worktree rev-parse HEAD) | ||
| Write-Host "[deploy-dogfood] Building from origin/main @ $headSha" | ||
| Write-Host "[deploy-dogfood] Building from $DeployRef @ $headSha" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and surrounding logic.
git ls-files tooling/preview-host/deploy-dogfood.ps1
wc -l tooling/preview-host/deploy-dogfood.ps1
sed -n '1,220p' tooling/preview-host/deploy-dogfood.ps1Repository: Harborline-Software/shipyard
Length of output: 13726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for native-command error handling settings and any HEAD/ref validation.
rg -n "\$PSNativeCommandUseErrorActionPreference|\$ErrorActionPreference|git fetch|git checkout|git worktree add|rev-parse" tooling/preview-host/deploy-dogfood.ps1Repository: Harborline-Software/shipyard
Length of output: 473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the rest of the deployment flow around the git selection to see whether
# failures or mismatches are handled later.
sed -n '130,260p' tooling/preview-host/deploy-dogfood.ps1Repository: Harborline-Software/shipyard
Length of output: 9151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,260p' tooling/preview-host/deploy-dogfood.ps1Repository: Harborline-Software/shipyard
Length of output: 9151
🌐 Web query:
PowerShell 7 native command non-zero exit codes ErrorActionPreference Stop PSNativeCommandUseErrorActionPreference documentation
💡 Result:
In PowerShell 7, the variable $PSNativeCommandUseErrorActionPreference allows you to control how PowerShell handles non-zero exit codes from native (external) commands [1][2][3]. When this variable is set to $true, PowerShell will treat a non-zero exit code from a native command as a PowerShell error [2][4]. This error is then subject to the behavior defined by your current $ErrorActionPreference [2][3]. If you set $ErrorActionPreference to 'Stop', any native command that returns a non-zero exit code will trigger a script-terminating error, effectively halting execution [2][4]. Key implementation details: - Default Value: The variable defaults to $false [2]. - Scope: You can modify this setting locally within a script block or function to avoid affecting the global state [2][3]. - Specific Exceptions: Some native commands (e.g., robocopy) use non-zero exit codes for informational purposes rather than indicating failures [2][3]. In such cases, you should explicitly set the variable to $false before running those commands to prevent unintended script termination [2][3]. - Error Type: When triggered, PowerShell generates a NativeCommandExitException [5][6][4]. Note that because this error is subject to $ErrorActionPreference, using 'Stop' elevates it to a terminating error that can be handled using try/catch blocks [5][6][4]. Example Usage: $PSNativeCommandUseErrorActionPreference = $true $ErrorActionPreference = 'Stop' try { # This will throw a NativeCommandExitException if the command fails some-native-command.exe } catch { Write-Host "Command failed with exit code: $LASTEXITCODE" }
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/scripting/learn/shell/running-commands?view=powershell-7.6
- 2: https://github.com/MicrosoftDocs/PowerShell-Docs/blob/main/reference/7.5/Microsoft.PowerShell.Core/About/about_Preference_Variables.md
- 3: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_preference_variables?view=powershell-7.5
- 4: https://stackoverflow.com/questions/57468522/powershell-and-process-exit-codes
- 5: Native Command Error Handling PowerShell/PowerShell-RFC#277
- 6: Native error handling:
$PSNativeCommandUseErrorActionPreference = $trueunexpectedly causes non-terminating, not statement-terminating errors PowerShell/PowerShell#18368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any repo-wide PowerShell setting that would change native-command
# failure behavior, plus any invocation hints for pwsh/Windows PowerShell.
rg -n "\$PSNativeCommandUseErrorActionPreference|pwsh|powershell\.exe|PowerShell 7|PSNativeCommand" .Repository: Harborline-Software/shipyard
Length of output: 1156
issue (blocking): fail closed when Git cannot select the requested ref
git fetch, git checkout, and git worktree add still need explicit exit-code checks here; this script does not enable native-command failures as terminating errors. If one of those steps fails, $Worktree can stay on the previous commit and the later rev-parse HEAD will log stale code as if it matched $DeployRef.
🤖 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 `@tooling/preview-host/deploy-dogfood.ps1` around lines 113 - 131, Update the
worktree preparation flow around git fetch, git checkout, and git worktree add
to explicitly validate each command’s exit status and terminate on failure.
Ensure the script never proceeds to rev-parse or logs $headSha when selecting
$DeployRef fails, preventing a stale worktree commit from being treated as
current.
Sources: Path instructions, MCP tools
1 participant
What changed
Authority boundary
This is the existing node-local, single-resource booking authority. It requires configured resource availability and rejects closed or occupied slots. It does not claim multi-resource ReservationSet atomicity, distributed authority, notifications, or protocol publication.
Validation
Summary by CodeRabbit