Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

web/src/pages/settings/ceremony-policy/strategies/TaskDrivenConfig.tsx (1)

40-40: ⚠️ Potential issue | 🟡 Minor

Enforce bounds on numeric inputs per documented constraints.

The isFinite checks are present, but the hints specify bounds ("min: 1" for completions, "1-100" for percentage) that aren't enforced. Users can enter 0 or negative values.

💡 Suggested bounded updates

- onChange={(e) => { const val = Number(e.target.value); if (Number.isFinite(val)) onChange({ ...config, every_n_completions: val }) }}
+ onChange={(e) => {
+   const val = Number(e.target.value)
+   if (!Number.isFinite(val) || val < 1) return
+   onChange({ ...config, every_n_completions: Math.floor(val) })
+ }}
...
- onChange={(e) => { const val = Number(e.target.value); if (Number.isFinite(val)) onChange({ ...config, sprint_percentage: val }) }}
+ onChange={(e) => {
+   const val = Number(e.target.value)
+   if (!Number.isFinite(val)) return
+   const bounded = Math.min(100, Math.max(1, val))
+   onChange({ ...config, sprint_percentage: bounded })
+ }}

Also applies to: 51-51

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/strategies/TaskDrivenConfig.tsx` at
line 40, The numeric inputs currently only check Number.isFinite but don't
enforce documented bounds; update the onChange handlers for the TaskDrivenConfig
inputs (the handler that reads e.target.value and calls onChange with
config.every_n_completions and the handler for config.top_percent) to validate
and clamp the parsed value to the allowed ranges (every_n_completions: minimum
1; top_percent: range 1–100) before invoking onChange, and ignore or coerce
invalid values so the config never receives 0, negative, or out-of-range
percentages.

web/src/pages/settings/ceremony-policy/strategies/BudgetDrivenConfig.tsx (2)

10-11: ⚠️ Potential issue | 🟠 Major

Guard unknown config values before array/number usage.

Direct casts from unknown can throw at render time if persisted data is malformed. If config.budget_thresholds is a string or object, thresholds.join(...) on line 17 will throw.

💡 Suggested hardening

-  const thresholds = (config.budget_thresholds as number[]) ?? [25, 50, 75, 100]
-  const transitionPct = (config.transition_threshold as number) ?? 100
+  const rawThresholds = config.budget_thresholds
+  const thresholds =
+    Array.isArray(rawThresholds) &&
+    rawThresholds.every((n): n is number => typeof n === 'number' && Number.isFinite(n))
+      ? rawThresholds
+      : [25, 50, 75, 100]
+
+  const rawTransition = config.transition_threshold
+  const transitionPct =
+    typeof rawTransition === 'number' && Number.isFinite(rawTransition) ? rawTransition : 100

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/strategies/BudgetDrivenConfig.tsx`
around lines 10 - 11, The code casts unknown config values directly to
number[]/number (the thresholds and transitionPct variables in
BudgetDrivenConfig.tsx), which can throw at render time; update the logic that
sets thresholds and transitionPct to first validate types: for thresholds use
Array.isArray(config.budget_thresholds) and coerce/map each entry to a number
(filtering out non-numeric values) and fall back to [25,50,75,100] if validation
fails, and for transitionPct check typeof config.transition_threshold ===
"number" (or parse to number safely) and default to 100 on NaN/invalid input;
ensure downstream usage (e.g., thresholds.join(...)) is only called on the
validated array.

---

33-33: ⚠️ Potential issue | 🟡 Minor

Enforce numeric bounds before persisting transition_threshold.

The hint specifies "(1-100)" but the handler only checks isFinite, allowing out-of-range values like 0 or 500.

💡 Suggested bounded update

-        onChange={(e) => { const val = Number(e.target.value); if (Number.isFinite(val)) onChange({ ...config, transition_threshold: val }) }}
+        onChange={(e) => {
+          const val = Number(e.target.value)
+          if (!Number.isFinite(val)) return
+          const bounded = Math.min(100, Math.max(1, val))
+          onChange({ ...config, transition_threshold: bounded })
+        }}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/strategies/BudgetDrivenConfig.tsx` at
line 33, The onChange handler for transition_threshold in BudgetDrivenConfig.tsx
currently only checks Number.isFinite and allows out-of-range values; update the
handler referenced (the onChange arrow updating config.transition_threshold) to
parse the input (e.g., const val = Number(e.target.value)), enforce bounds
1..100 (either return early if val < 1 || val > 100 or clamp: val =
Math.min(100, Math.max(1, val))), and then call onChange({ ...config,
transition_threshold: val }) only with the validated/clamped numeric value to
ensure the stored transition_threshold is always within 1–100.

web/src/pages/settings/ceremony-policy/DepartmentOverridesPanel.tsx (1)

38-45: ⚠️ Potential issue | 🟠 Major

Fresh department overrides still start from the wrong baseline.

Line 45 seeds a new draft with { strategy: 'task_driven' }. Because Lines 60-63 later spread effectivePolicy, the first edit to threshold, velocity calculator, or strategy_config will persist task_driven even when the department was inheriting calendar, hybrid, etc. Seed from the resolved policy instead, or keep the draft empty and derive the displayed strategy from the resolved value until the user explicitly changes it.

Also applies to: 60-63, 115-121

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/DepartmentOverridesPanel.tsx` around
lines 38 - 45, The new-draft seeding in handleInheritChange (which calls
setLocalDraft({ strategy: 'task_driven' })) incorrectly fixes the strategy to
task_driven and gets persisted when the component later spreads effectivePolicy
(see the setLocalDraft use around effectivePolicy at lines ~60-63 and the other
draft creation at ~115-121); change this so a fresh draft is seeded from the
resolved effectivePolicy (use effectivePolicy.strategy and
effectivePolicy.strategy_config/thresholds as the initial values) or leave the
draft null/empty and derive displayed values from effectivePolicy until the user
makes an explicit change; update clearPolicy, setLocalDraft calls in
DepartmentOverridesPanel/handleInheritChange and the other draft-creation sites
to follow one of these two approaches so the initial persisted edit doesn't
overwrite an inherited strategy.

web/src/pages/settings/ceremony-policy/strategies/EventDrivenConfig.tsx (1)

10-10: ⚠️ Potential issue | 🟡 Minor

Sanitize debounce_default to the documented range before persisting it.

Line 19 filters out NaN, but it still stores 0, negatives, decimals, and arbitrarily large values (Number('') is 0); Line 10 also lets a persisted NaN bounce back into the control. That breaks the 1-10000 contract for this field.

Proposed fix

-  const debounceDefault = (config.debounce_default as number) ?? 5
+  const rawDebounceDefault = config.debounce_default
+  const debounceDefault =
+    typeof rawDebounceDefault === 'number' && Number.isFinite(rawDebounceDefault)
+      ? Math.min(10000, Math.max(1, Math.trunc(rawDebounceDefault)))
+      : 5
...
-        onChange={(e) => { const val = Number(e.target.value); if (Number.isFinite(val)) onChange({ ...config, debounce_default: val }) }}
+        onChange={(e) => {
+          const val = Number(e.target.value)
+          if (!Number.isFinite(val)) return
+          onChange({
+            ...config,
+            debounce_default: Math.min(10000, Math.max(1, Math.trunc(val))),
+          })
+        }}

Also applies to: 15-19

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/strategies/EventDrivenConfig.tsx` at
line 10, Sanitize config.debounce_default both when reading into the control and
before persisting: coerce it to a finite number, clamp it to the documented
range 1–10000, and convert to an integer (e.g., floor or round) so decimals, 0,
negatives, NaN and huge values are normalized. Update the places using
debounceDefault (the read: const debounceDefault = (config.debounce_default as
number) ?? 5 and the save/filter logic that currently only filters NaN) to apply
this clamp-and-integer coercion so the component never displays or stores values
outside 1..10000.

web/src/pages/settings/ceremony-policy/strategies/CalendarConfig.tsx (1)

20-21: ⚠️ Potential issue | 🟡 Minor

Reject invalid duration_days values before updating shared state.

Line 38 blocks NaN, but it still stores 0, negatives, decimals, and Number('') === 0; Line 21 also lets an already-invalid NaN render back as "NaN". That bypasses the documented 1-90 contract for this field.

Proposed fix

-  const durationDays = (config.duration_days as number) ?? 14
+  const rawDurationDays = config.duration_days
+  const durationDays =
+    typeof rawDurationDays === 'number' && Number.isFinite(rawDurationDays)
+      ? Math.min(90, Math.max(1, Math.trunc(rawDurationDays)))
+      : 14
...
-        onChange={(e) => { const val = Number(e.target.value); if (Number.isFinite(val)) onChange({ ...config, duration_days: val }) }}
+        onChange={(e) => {
+          const val = Number(e.target.value)
+          if (!Number.isFinite(val)) return
+          onChange({
+            ...config,
+            duration_days: Math.min(90, Math.max(1, Math.trunc(val))),
+          })
+        }}

Also applies to: 34-38

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/strategies/CalendarConfig.tsx` around
lines 20 - 21, The duration_days value must be validated/sanitized before
updating shared state: instead of assigning durationDays from
(config.duration_days as number) ?? 14 or allowing
Number('')/NaN/decimals/0/negatives to pass through, validate in the component's
initializer and in the change handler (e.g., where durationDays is read and
where you call the shared state updater such as setConfig/updateConfig). Accept
only integers between 1 and 90 (use Number.isInteger(parseInt(value, 10)) or
equivalent after parsing), reject or ignore updates that produce
NaN/0/negative/decimal/empty-string, and keep the previous valid value when an
invalid input is attempted; also ensure the rendered input never receives "NaN"
by coalescing invalid config.duration_days to a safe default or the last valid
value before rendering.

src/synthorg/api/controllers/departments.py (1)

515-547: ⚠️ Potential issue | 🟠 Major

Settings-backed department reads should be validated before returning 200.

This helper returns any dict stored under dept_ceremony_policies[department_name] before confirming the department still exists, and it never re-validates that dict with CeremonyPolicyConfig.model_validate(). A stale or malformed settings entry can therefore come back as a successful read instead of the 404/503 this endpoint advertises. As per coding guidelines, validate at system boundaries (user input, external APIs, config files).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/synthorg/api/controllers/departments.py` around lines 515 - 547, The
settings-backed value returned by _get_dept_ceremony_override must be validated
before returning: first confirm the department exists (call the same
department-existence check used elsewhere) before using the policies mapping
from _load_dept_policies_json, and if a non-None dict is found run
CeremonyPolicyConfig.model_validate() on it; if validation raises, convert to
ServiceUnavailableError (or rethrow per existing error semantics), and if the
department does not exist raise NotFoundError instead of returning the settings
value. Ensure you still treat None as the explicit "inherit" sentinel.

src/synthorg/settings/definitions/coordination.py (1)

168-199: ⚠️ Potential issue | 🟠 Major

Wire the override-map settings into YAML too.

dept_ceremony_policies and ceremony_policy_overrides are the only ceremony-policy keys in this block without yaml_path, so those two levels can only exist in DB state. That breaks the feature’s YAML/code parity and the documented precedence chain for settings resolution. Based on learnings, settings use runtime-editable persistence with precedence: DB > env > YAML > code defaults.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/synthorg/settings/definitions/coordination.py` around lines 168 - 199,
The two SettingDefinition entries for dept_ceremony_policies and
ceremony_policy_overrides are missing yaml_path so they cannot be set via YAML;
update their SettingDefinition instances (the ones with
key="dept_ceremony_policies" and key="ceremony_policy_overrides" under
SettingNamespace.COORDINATION) to include an appropriate yaml_path string
(matching the existing YAML naming convention used by other ceremony policy
settings) so the settings honor YAML precedence (DB > env > YAML > code
defaults) and restore YAML/code parity; keep type=SettingType.JSON and other
fields unchanged.

web/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsx (1)

153-163: ⚠️ Potential issue | 🟠 Major

Saving one policy across six independent writes is still non-atomic.

web/src/stores/settings.ts:126-165 sends each updateSetting() as a separate request and patches store state per key. If one request fails partway through, the backend is left with a mixed ceremony policy that this page cannot roll back or reconstruct reliably.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsx` around lines
153 - 163, The current handleSave performs six independent updateSetting calls
(updateSetting('coordination', 'ceremony_strategy', ...), etc.), which is
non-atomic and can leave the backend in a partially-updated state; change to a
single atomic update by creating and calling a batch update API or store helper
(e.g., add/update a function like updateSettingsBatch or
updateCoordinationSettings) that accepts the entire ceremony policy payload
(strategy, strategy_config, velocity_calculator, auto_transition,
transition_threshold, ceremony_policy_overrides) and sends one request, and only
patch the local settings store after that single request succeeds; update
handleSave to call this new batch method (and remove the Promise.all multi-call
logic) so the six keys are saved atomically and the UI state is only updated on
success.

web/src/pages/settings/ceremony-policy/CeremonyListPanel.tsx (1)

41-47: ⚠️ Potential issue | 🟠 Major

Per-ceremony overrides still force a concrete strategy and still cannot edit strategy_config.

Turning override on seeds { strategy: 'task_driven' }, so a row that only wants to override threshold or auto-transition immediately stops inheriting strategy. The row also never renders StrategyConfigPanel, so milestone/calendar/etc. specific config cannot be viewed or changed at this level.

Also applies to: 87-103

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/CeremonyListPanel.tsx` around lines 41
- 47, handleInheritChange currently forces a concrete strategy ({ strategy:
'task_driven' }) when turning inheritance off, which prevents per-ceremony rows
from editing strategy_config and from remaining strategy-agnostic; change
handleInheritChange to call onOverrideChange(name, {}) or otherwise create an
override object that preserves existing override fields (merge with existing
override if available) instead of setting strategy, and ensure the codepath that
decides to render StrategyConfigPanel checks for the presence of an override
object (e.g., override !== null) rather than a concrete strategy so
StrategyConfigPanel can render/edit strategy_config; apply the same fix to the
duplicate logic at the other block (the similar handler around the 87-103
range).

src/synthorg/api/controllers/ceremony_policy.py (1)

347-373: ⚠️ Potential issue | 🟠 Major

Resolved-policy lookup still hides broken department override settings.

This helper converts settings/JSON failures—and non-dict department values—into _SETTINGS_NOT_FOUND, then _fetch_department_policy() falls back to config. The resolved-policy endpoint can therefore report YAML/default sources even when the settings-backed override exists but is unreadable or corrupt.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/synthorg/api/controllers/ceremony_policy.py` around lines 347 - 373, The
current broad except in _fetch_department_policy (around the call to
app_state.settings_service.get and json.loads(entry.value)) swallows JSON/typing
failures and returns _SETTINGS_NOT_FOUND so the caller falls back to defaults;
change the error handling to only treat a missing settings service/entry as
not-found and do not convert JSONDecodeError/TypeError/other parsing errors into
_SETTINGS_NOT_FOUND—log those errors (include exc_info) and re-raise (or return
an explicit error response) so corrupt/unreadable override data is surfaced;
update the try/except around json.loads(entry.value) and the handling of
policies (the policies variable and department_name lookup) accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

web/src/pages/settings/ceremony-policy/strategies/BudgetDrivenConfig.tsx (1)

13-15: ⚠️ Potential issue | 🟠 Major

Harden transition_threshold normalization on read.

typeof config.transition_threshold === 'number' still accepts NaN/Infinity and out-of-range values. That can render invalid state ("NaN"/unexpected number) before the user edits the field.

Suggested fix

-  const transitionPct = typeof config.transition_threshold === 'number'
-    ? config.transition_threshold
-    : 100
+  const rawTransition = config.transition_threshold
+  const transitionPct =
+    typeof rawTransition === 'number' && Number.isFinite(rawTransition)
+      ? Math.min(100, Math.max(1, Math.round(rawTransition)))
+      : 100

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/strategies/BudgetDrivenConfig.tsx`
around lines 13 - 15, The current normalization for transitionPct accepts
NaN/Infinity and out-of-range values; change the read for
config.transition_threshold (used to compute transitionPct in
BudgetDrivenConfig.tsx) to first coerce/parse a numeric value (if needed), then
verify it's finite (Number.isFinite) and clamp it into the 0–100 range, falling
back to 100 when the value is invalid; update the transitionPct assignment to
use this validated/clamped value so the component never receives NaN/Infinity or
values outside 0–100.

src/synthorg/api/controllers/departments.py (1)

542-562: ⚠️ Potential issue | 🟠 Major

Corrupt per-department entries still get masked as inheritance.

Once department_name is present in policies, any non-None value here is persisted state. Returning None for an invalid dict or unexpected type makes the GET path look like “inherit” and lets the dashboard overwrite unreadable data on the next save. This should raise ServiceUnavailableError instead. As per coding guidelines, validate at system boundaries (user input, external APIs, config files).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/synthorg/api/controllers/departments.py` around lines 542 - 562, The
current branch in departments.ceremony_policy.get masks corrupt persisted
entries as inheritance by returning None; instead detect invalid stored values
(when policies[department_name] is a dict that fails
CeremonyPolicyConfig.model_validate or any unexpected type) and raise
ServiceUnavailableError so callers know the stored state is unreadable. Adjust
the exception handling around CeremonyPolicyConfig.model_validate to use proper
tuple-catching (except (MemoryError, RecursionError): raise) and on other
exceptions log the invalid stored override (using logger.warning as shown) and
then raise ServiceUnavailableError (not return None); also handle non-dict
unexpected types similarly by logging and raising ServiceUnavailableError.

web/src/pages/settings/ceremony-policy/DepartmentOverridesPanel.tsx (1)

34-45: ⚠️ Potential issue | 🟠 Major

Seed new department drafts from the resolved policy, not {}.

When a department currently inherits a non-default policy, policy ?? {} makes the editor open as task_driven with an empty strategy config. The first edit is then made against the wrong strategy/config panel, even though the effective department policy may be something else. Clone the resolved department policy as the initial draft here instead of starting from an empty object.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/DepartmentOverridesPanel.tsx` around
lines 34 - 45, The toggle handler seeds new drafts from the wrong source: change
the else branch in handleInheritChange so it seeds setLocalDraft with the
resolved/effective policy (the one used by effectivePolicy) rather than policy
?? {}; i.e., clone effectivePolicy (not a shared reference) when calling
setLocalDraft to initialize the editor (use structured clone or
{...effectivePolicy} / JSON methods) and keep clearPolicy(dept.name) for the
inherit path.

docs/design/page-structure.md (1)

336-336: ⚠️ Potential issue | 🟡 Minor

Expand this route description to match the page.

This row still reads like an editor-only page. It omits the resolved-policy/source-badge flow and velocity-calculator behavior that are documented above, so the routing table is still incomplete here. Based on learnings, update the relevant docs/design/ page to reflect the new reality.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/design/page-structure.md` at line 336, Update the
`/settings/coordination/ceremony-policy` row description to match the actual
page behavior: replace the editor-only wording with a summary that includes the
resolved-policy/source-badge flow and the velocity-calculator behavior (how
resolved policies are surfaced, source badges indicate origin, and how
velocity-calculator affects metrics/overrides), and ensure the routing table
entry for `/settings/coordination/ceremony-policy` lists strategy selection,
department and per-ceremony overrides plus the resolved-policy/source-badge flow
and velocity-calculator effects so the docs/design/page-structure.md reflects
the real page functionality.

web/src/pages/settings/ceremony-policy/CeremonyListPanel.tsx (1)

39-48: ⚠️ Potential issue | 🟠 Major

Per-ceremony override editing is still incomplete.

For inherited ceremonies, this row initializes from {} and falls back to task_driven, so the user never sees the actual effective strategy/config they are overriding. The expanded editor also omits StrategyConfigPanel, which makes strategy_config impossible to view or edit for strategies like event_driven, external_trigger, and milestone_driven. Pass the resolved ceremony policy into this row, seed new overrides from it, and reuse StrategyConfigPanel here.

Also applies to: 88-103

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/CeremonyListPanel.tsx` around lines 39
- 48, The per-ceremony override row currently initializes overrides from {} and
uses a fallback 'task_driven', so users can't see the effective policy or edit
strategy_config; update CeremonyListPanel to compute a resolvedPolicy (policy
merged with defaults) and pass that resolvedPolicy as the initial value into the
row and to the expanded editor; change handleInheritChange and the
onOverrideChange calls to seed new overrides from resolvedPolicy (not {} or
policy ?? {}) so inherited rows show effective values, and reuse
StrategyConfigPanel inside the expanded editor so strategy_config for strategies
like event_driven/external_trigger/milestone_driven is viewable and editable;
apply the same fix to the other override-initialization instance that mirrors
this logic.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

web/src/pages/settings/ceremony-policy/strategies/ThroughputAdaptiveConfig.tsx (1)

16-41: ⚠️ Potential issue | 🟠 Major

Enforce numeric bounds before persisting config values.

Lines 20, 29, and 38 store raw Number(e.target.value) without NaN checks or range validation, despite hints specifying (1-100) and (2-100) constraints. This allows invalid values to propagate to the store.

🐛 Proposed fix to add validation and clamping

       <InputField
         label="Velocity Drop Threshold (%)"
         type="number"
         value={String(dropPct)}
-        onChange={(e) => onChange({ ...config, velocity_drop_threshold_pct: Number(e.target.value) })}
+        onChange={(e) => {
+          const val = Number(e.target.value)
+          if (!Number.isFinite(val)) return
+          onChange({ ...config, velocity_drop_threshold_pct: Math.min(100, Math.max(1, Math.round(val))) })
+        }}
         disabled={disabled}
         hint="Trigger ceremony when velocity drops by this percentage (1-100)"
       />

       <InputField
         label="Velocity Spike Threshold (%)"
         type="number"
         value={String(spikePct)}
-        onChange={(e) => onChange({ ...config, velocity_spike_threshold_pct: Number(e.target.value) })}
+        onChange={(e) => {
+          const val = Number(e.target.value)
+          if (!Number.isFinite(val)) return
+          onChange({ ...config, velocity_spike_threshold_pct: Math.min(100, Math.max(1, Math.round(val))) })
+        }}
         disabled={disabled}
         hint="Trigger ceremony when velocity spikes by this percentage (1-100)"
       />

       <InputField
         label="Measurement Window (tasks)"
         type="number"
         value={String(window)}
-        onChange={(e) => onChange({ ...config, measurement_window_tasks: Number(e.target.value) })}
+        onChange={(e) => {
+          const val = Number(e.target.value)
+          if (!Number.isFinite(val)) return
+          onChange({ ...config, measurement_window_tasks: Math.min(100, Math.max(2, Math.round(val))) })
+        }}
         disabled={disabled}
         hint="Rolling window of task completions for rate calculation (2-100)"
       />

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@web/src/pages/settings/ceremony-policy/strategies/ThroughputAdaptiveConfig.tsx`
around lines 16 - 41, The three InputField onChange handlers in
ThroughputAdaptiveConfig.tsx currently write raw Number(e.target.value) into
config (velocity_drop_threshold_pct, velocity_spike_threshold_pct,
measurement_window_tasks) allowing NaN/out-of-range values; update each handler
to parse the input safely (e.g. Number or parseInt), guard against NaN (fall
back to previous value or a sensible default), and clamp to the allowed ranges:
velocity_drop_threshold_pct and velocity_spike_threshold_pct to 1–100, and
measurement_window_tasks to 2–100, then call onChange with the validated/clamped
value so only valid numbers are persisted.

web/src/components/ui/policy-source-badge.stories.tsx (1)

13-23: ⚠️ Potential issue | 🟠 Major

Add required state stories for this shared UI component.

The file documents source variants, but it still lacks the mandatory state set (hover/loading/error/empty).

Proposed story additions

 export const Default: Story = {
   args: { source: 'default' },
 }
+
+export const Hover: Story = {
+  args: { source: 'project' },
+  parameters: { pseudo: { hover: true } },
+}
+
+export const Loading: Story = {
+  args: { source: 'default' },
+  parameters: {
+    docs: { description: { story: 'Loading state representation in parent container.' } },
+  },
+}
+
+export const Error: Story = {
+  args: { source: 'default' },
+  parameters: {
+    docs: { description: { story: 'Error-state representation in parent container.' } },
+  },
+}
+
+export const Empty: Story = {
+  args: { source: 'default' },
+  parameters: {
+    docs: { description: { story: 'Empty/no-data state representation in parent container.' } },
+  },
+}

As per coding guidelines, “When creating new shared components… create accompanying `.stories.tsx` with all states (default, hover, loading, error, empty).”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/components/ui/policy-source-badge.stories.tsx` around lines 13 - 23,
The story file lists Project, Department and Default stories but is missing the
required state stories (hover, loading, error, empty) for the shared
PolicySourceBadge component; add four new Story exports named Hover, Loading,
Error, and Empty that target the same component and set appropriate args/props
(e.g., source values and a loading boolean or error message prop) and any
necessary decorators or interaction/play setups to simulate hover state so the
component renders each required state consistently with the existing
Project/Department/Default stories.

web/src/components/ui/inherit-toggle.stories.tsx (1)

13-23: ⚠️ Potential issue | 🟠 Major

Add the required shared-component state stories (hover/loading/error/empty).

This story currently covers only default variants and still misses the mandatory state set for shared UI components.

Proposed story additions

 export const Disabled: Story = {
   args: { inherit: true, onChange: () => {}, disabled: true },
 }
+
+export const Hover: Story = {
+  args: { inherit: true, onChange: () => {} },
+  parameters: { pseudo: { hover: true } },
+}
+
+export const Loading: Story = {
+  args: { inherit: true, onChange: () => {}, disabled: true },
+  parameters: {
+    docs: { description: { story: 'Loading/locked interaction state.' } },
+  },
+}
+
+export const Error: Story = {
+  args: { inherit: false, onChange: () => {} },
+  parameters: {
+    docs: { description: { story: 'Error-state usage example in parent form context.' } },
+  },
+}
+
+export const Empty: Story = {
+  args: { inherit: true, onChange: () => {} },
+  parameters: {
+    docs: { description: { story: 'Empty/default container state example.' } },
+  },
+}

As per coding guidelines, “When creating new shared components… create accompanying `.stories.tsx` with all states (default, hover, loading, error, empty).”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/components/ui/inherit-toggle.stories.tsx` around lines 13 - 23, Add
the missing shared-component state stories (hover, loading, error, empty) to the
existing inherit-toggle.stories.tsx so the shared UI component covers all
required states; create new Story exports named Hover, Loading, Error, and Empty
alongside the existing Inherit, Override, and Disabled exports, each supplying
appropriate args/props (e.g., inherit true/false, disabled where needed) and, if
necessary, a play/parameters/decorator to simulate hover state and to present
loading/error/empty visuals consistent with the component’s props or state
handlers (reference the Story type and the existing Inherit/Override/Disabled
exports to match pattern and naming).

web/src/pages/settings/ceremony-policy/StrategyChangeWarning.tsx (1)

17-29: ⚠️ Potential issue | 🟡 Minor

Add live-region semantics for the dynamic warning banner.

Because this element appears conditionally, Line 17 should expose polite status semantics so assistive tech announces the change consistently.

♿ Suggested fix

-    <div className="flex items-start gap-2 rounded-md border border-warning/30 bg-warning/5 p-card">
+    <div
+      className="flex items-start gap-2 rounded-md border border-warning/30 bg-warning/5 p-card"
+      role="status"
+      aria-live="polite"
+      aria-atomic="true"
+    >

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/StrategyChangeWarning.tsx` around
lines 17 - 29, The dynamic warning banner in StrategyChangeWarning should expose
live-region semantics so assistive tech announces it; update the top-level
container div (the element rendering the AlertTriangle and text) to include
live-region attributes (e.g., role="status" and aria-live="polite", and
optionally aria-atomic="true") so changes are announced consistently when the
component renders/changes.

web/src/pages/settings/ceremony-policy/StrategyPicker.tsx (1)

28-28: ⚠️ Potential issue | 🟡 Minor

Guard select values instead of force-casting to CeremonyStrategyType.

Line 28 trusts arbitrary SelectField output via cast. Add a runtime type guard against CEREMONY_STRATEGY_TYPES before invoking onChange.

🔧 Proposed fix

+const isCeremonyStrategyType = (v: string): v is CeremonyStrategyType =>
+  CEREMONY_STRATEGY_TYPES.includes(v as CeremonyStrategyType)
+
 export function StrategyPicker({ value, onChange, disabled }: StrategyPickerProps) {
   return (
@@
-        onChange={(v) => onChange(v as CeremonyStrategyType)}
+        onChange={(v) => {
+          if (isCeremonyStrategyType(v)) onChange(v)
+        }}
         disabled={disabled}
       />

#!/bin/bash
set -euo pipefail

echo "== SelectField onChange contract =="
SELECT_FILE="$(fd -i '^select-field\.tsx$' web/src/components/ui | head -n1)"
rg -n -C3 'onChange' "$SELECT_FILE"

echo
echo "== Current StrategyPicker cast site =="
rg -n -C3 'onChange=\{\(v\) => onChange\(v as CeremonyStrategyType\)\}' web/src/pages/settings/ceremony-policy/StrategyPicker.tsx

echo
echo "== Strategy type domain =="
rg -n -C2 'type CeremonyStrategyType|CEREMONY_STRATEGY_TYPES' web/src/api/types.ts web/src/utils/constants.ts

Based on learnings: “TypeScript strict mode must pass type checking” and “Validate at system boundaries (user input, external APIs, config files)”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/StrategyPicker.tsx` at line 28,
Replace the unsafe cast in StrategyPicker's SelectField onChange handler with a
runtime guard: check that the incoming value exists in the
CEREMONY_STRATEGY_TYPES set/array (or matches a predicate for
CeremonyStrategyType) before calling the onChange prop; if it passes, call
onChange(value as CeremonyStrategyType), otherwise ignore or handle the invalid
input (e.g., log or no-op). Locate the handler where onChange={(v) => onChange(v
as CeremonyStrategyType)} and use CEREMONY_STRATEGY_TYPES and
CeremonyStrategyType to validate the value first.

web/src/pages/org-edit/DepartmentCeremonyOverride.tsx (2)

41-44: ⚠️ Potential issue | 🟠 Major

Move render-time state sync into useEffect.

Line 43 updates state during render; this should be effect-driven to avoid render-phase updates.

♻️ Suggested fix

-import { useCallback, useRef, useState } from 'react'
+import { useCallback, useEffect, useState } from 'react'
@@
-  const prevHasOverrideRef = useRef(hasOverride)
-
-  // Sync expanded state when override status changes externally
-  if (prevHasOverrideRef.current !== hasOverride) {
-    prevHasOverrideRef.current = hasOverride
-    setExpanded(hasOverride)
-  }
+  useEffect(() => {
+    setExpanded(hasOverride)
+  }, [hasOverride])

#!/bin/bash
# Verify render-phase state updates in this component
rg -n "setExpanded\\(|prevHasOverrideRef|useEffect\\(" web/src/pages/org-edit/DepartmentCeremonyOverride.tsx

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/org-edit/DepartmentCeremonyOverride.tsx` around lines 41 - 44,
The render-phase update of state should be moved into an effect: inside the
DepartmentCeremonyOverride component, remove the direct render-time block that
checks prevHasOverrideRef.current !== hasOverride and instead create a useEffect
that depends on hasOverride; within that effect compare
prevHasOverrideRef.current to hasOverride, set prevHasOverrideRef.current =
hasOverride and call setExpanded(hasOverride) so state updates occur in an
effect (use the existing prevHasOverrideRef, hasOverride and setExpanded
identifiers).

---

58-69: ⚠️ Potential issue | 🟠 Major

Strategy change can erase strategy_config with no editor to recover it.

Changing strategy clears strategy_config, but this component does not expose a strategy-config editor, so data can be lost and cannot be edited here.

Also applies to: 94-134

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/org-edit/DepartmentCeremonyOverride.tsx` around lines 58 - 69,
handleStrategyChange currently clears strategy_config unconditionally which can
silently erase user data because this component does not provide an editor for
strategy_config; instead, preserve existing strategy_config unless you have an
editor here or the strategy truly requires a reset. Update handleStrategyChange
(and the similar block at 94-134) to only update strategy and
velocity_calculator (using STRATEGY_DEFAULT_VELOCITY_CALC[s]) while keeping
strategy_config: policy?.strategy_config, or add a clear only behind an explicit
confirmation flag passed to onChange; ensure you still short-circuit when s ===
policy?.strategy.

web/src/pages/settings/ceremony-policy/CeremonyListPanel.tsx (1)

91-107: ⚠️ Potential issue | 🟠 Major

Per-ceremony overrides still cannot edit strategy_config.

The row renders StrategyPicker + PolicyFieldsPanel only, so strategy-specific config is not editable for ceremony-level overrides.

🔧 Suggested direction

 import { StrategyPicker } from './StrategyPicker'
+import { StrategyConfigPanel } from './StrategyConfigPanel'
 import { PolicyFieldsPanel } from './PolicyFieldsPanel'
@@
           {hasOverride && (
             <div className={cn('space-y-3 pl-2 border-l-2 border-accent/20')}>
               <StrategyPicker
                 value={strategy as CeremonyStrategyType}
                 onChange={handleStrategyChange}
                 disabled={saving}
               />
+              <StrategyConfigPanel
+                strategy={strategy as CeremonyStrategyType}
+                config={(policy?.strategy_config ?? {}) as Record<string, unknown>}
+                onChange={(c) => onOverrideChange(name, { ...policy, strategy_config: c })}
+                disabled={saving}
+              />
               <PolicyFieldsPanel

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/CeremonyListPanel.tsx` around lines 91
- 107, The ceremony-level override UI only renders StrategyPicker and
PolicyFieldsPanel so the strategy-specific configuration (strategy_config) is
not editable; update the conditional block that renders when hasOverride is true
to also render the component that edits strategy_config (the same one used for
global policy strategy-specific options) and wire its value and change handler
into the override flow by passing policy?.strategy_config (or a sensible
default) and calling onOverrideChange(name, { ...policy, strategy_config:
updated }) from the strategy-config component’s onChange; ensure the
StrategyPicker, PolicyFieldsPanel, and the strategy-config editor use the same
saving/disabled (saving) state and the existing name/onOverrideChange handlers
so ceremony overrides persist correctly.

web/src/pages/settings/ceremony-policy/PolicyFieldsPanel.tsx (1)

73-77: ⚠️ Potential issue | 🟠 Major

Enforce the documented threshold bounds before persisting.

Line 73-Line 77 should clamp/validate to 0.01..1.0; currently any finite number is accepted.

🔧 Suggested fix

               onChange={(e) => {
-                const val = Number(e.target.value)
-                if (Number.isFinite(val)) {
-                  onTransitionThresholdChange(val)
-                }
+                const val = e.target.valueAsNumber
+                if (!Number.isFinite(val)) return
+                onTransitionThresholdChange(Math.min(1.0, Math.max(0.01, val)))
               }}

Based on learnings: Validate at system boundaries (user input, external APIs, config files).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/PolicyFieldsPanel.tsx` around lines 73
- 77, The onChange handler in PolicyFieldsPanel.tsx currently passes any finite
number to onTransitionThresholdChange; change it to validate and clamp the
parsed value into the documented range 0.01..1.0 before calling
onTransitionThresholdChange (e.g., parse Number(e.target.value), ignore NaN,
then clamp with Math.max(0.01, Math.min(1.0, parsed))). Ensure only the
clamped/validated value is passed to onTransitionThresholdChange so out-of-range
inputs cannot be persisted.

web/src/pages/settings/ceremony-policy/strategies/TaskDrivenConfig.tsx (1)

21-22: ⚠️ Potential issue | 🟡 Minor

Harden numeric reads against NaN/Infinity.

Line 21 and Line 22 accept non-finite numbers from malformed persisted config. Guard with Number.isFinite(...) before using values.

💡 Suggested fix

-  const everyN = typeof config.every_n_completions === 'number' ? config.every_n_completions : 5
-  const pct = typeof config.sprint_percentage === 'number' ? config.sprint_percentage : 50
+  const everyN =
+    typeof config.every_n_completions === 'number' &&
+    Number.isFinite(config.every_n_completions)
+      ? config.every_n_completions
+      : 5
+  const pct =
+    typeof config.sprint_percentage === 'number' &&
+    Number.isFinite(config.sprint_percentage)
+      ? config.sprint_percentage
+      : 50

Based on learnings: Validate at system boundaries (user input, external APIs, config files).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/strategies/TaskDrivenConfig.tsx`
around lines 21 - 22, The numeric reads for everyN and pct currently only check
typeof and can accept NaN/Infinity; update the assignments for everyN and pct to
guard with Number.isFinite(config.every_n_completions) and
Number.isFinite(config.sprint_percentage) respectively, falling back to the
defaults 5 and 50 when the value is not a finite number (keep the identifiers
everyN and pct and the referenced properties config.every_n_completions and
config.sprint_percentage).

web/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsx (1)

188-209: ⚠️ Potential issue | 🟠 Major

Non-atomic save may leave ceremony policy partially updated on failure.

The save handler issues six independent updateSetting calls via Promise.all. If one request fails after others succeed, the backend is left with a mixed ceremony policy that the UI cannot roll back. The comment at lines 186-187 acknowledges the limitation, but this remains a reliability concern for production use.

Consider implementing a batch update endpoint or a transactional settings API to ensure atomicity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/pages/settings/ceremony-policy/CeremonyPolicyPage.tsx` around lines
188 - 209, The handler handleSave performs six independent updateSetting calls
which can leave partial updates on failure; change the save to a single atomic
operation by creating and using a batch/transactional API (e.g., add a new
backend endpoint like batchUpdateSettings or extend updateSetting to accept an
object) that accepts all coordination keys at once (ceremony_strategy,
ceremony_strategy_config, ceremony_velocity_calculator,
ceremony_auto_transition, ceremony_transition_threshold,
ceremony_policy_overrides) and call that from handleSave; on success call
addToast and fetchResolvedPolicy, on failure show the error toast and avoid
partial commits by relying on the backend transaction semantics.

web/src/stores/ceremony-policy.ts (1)

64-72: ⚠️ Potential issue | 🟡 Minor

fetchActiveStrategy still does not manage loading state.

Unlike fetchResolvedPolicy (lines 54-61), this action does not set loading: true before the request or loading: false after. If the UI relies on loading to show a spinner while fetching both resolved policy and active strategy, users will see inconsistent loading indicators.

🐛 Proposed fix to add loading state management

   fetchActiveStrategy: async () => {
+    set({ loading: true, activeStrategyError: null })
     try {
       const active = await ceremonyApi.getActiveStrategy()
-      set({ activeStrategy: active })
+      set({ activeStrategy: active, loading: false })
     } catch (err) {
-      set({ activeStrategyError: getErrorMessage(err) })
+      set({ activeStrategyError: getErrorMessage(err), loading: false })
     }
   },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/src/stores/ceremony-policy.ts` around lines 64 - 72, fetchActiveStrategy
omits updating the shared loading flag, causing inconsistent UI; update the
fetchActiveStrategy action to mirror fetchResolvedPolicy by setting loading:
true before initiating ceremonyApi.getActiveStrategy(), preserve existing error
handling, and ensure loading: false is set after completion (use a finally
block) so activeStrategy, activeStrategyError, and loading are all correctly
updated; target the fetchActiveStrategy function in ceremony-policy.ts to
implement this change.

src/synthorg/api/controllers/departments.py (2)

584-591: 🧹 Nitpick | 🔵 Trivial

Cross-worker race condition documented but unresolved.

The comment correctly notes that _dept_policy_lock only serializes within a single process. In multi-worker deployments, concurrent PUT/DELETE requests can still race and cause last-write-wins data loss. This is a known limitation that should be tracked for future resolution.

Consider tracking this as technical debt. Solutions include:

• Settings service compare-and-swap (CAS) API with version tokens
• Per-department storage keys instead of a single JSON blob
• Distributed locking (Redis, etc.) if multi-worker deployment is planned

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/synthorg/api/controllers/departments.py` around lines 584 - 591, The
comment notes a cross-worker race with the module-level asyncio.Lock
(_dept_policy_lock) protecting the dept_ceremony_policies JSON blob but doesn't
track it as technical debt or provide an actionable plan; add a
TODO/technical-debt marker in the file near _dept_policy_lock that creates a
tracked issue (or links to an existing ticket) for implementing a cross-worker
solution and list accepted remediation options (settings-service CAS with
version tokens, per-department storage keys instead of a single JSON blob, or
distributed locking like Redis), and update any relevant handler names
(PUT/DELETE handlers that modify dept_ceremony_policies) to include a short
inline note pointing to that ticket so future maintainers know this must be
addressed for multi-worker deployments.

---

491-513: ⚠️ Potential issue | 🟡 Minor

_save_dept_policies_json does not wrap exceptions in ServiceUnavailableError.

While _load_dept_policies_json wraps exceptions in ServiceUnavailableError when raise_on_error=True (lines 473-476), _save_dept_policies_json lets settings_service.set() exceptions propagate raw. This can expose internal error details and produce inconsistent 500 responses instead of the expected 503.

🛡️ Proposed fix to wrap save exceptions

 async def _save_dept_policies_json(
     app_state: AppState,
     policies: dict[str, Any],
 ) -> None:
     ...
     if not app_state.has_settings_service:
         msg = "Settings service not available"
         logger.warning(API_SERVICE_UNAVAILABLE, service="settings")
         raise ServiceUnavailableError(msg)
-    await app_state.settings_service.set(
-        "coordination",
-        "dept_ceremony_policies",
-        json.dumps(policies, separators=(",", ":")),
-    )
+    try:
+        await app_state.settings_service.set(
+            "coordination",
+            "dept_ceremony_policies",
+            json.dumps(policies, separators=(",", ":")),
+        )
+    except MemoryError, RecursionError:
+        raise
+    except Exception as exc:
+        logger.warning(
+            API_REQUEST_ERROR,
+            endpoint="departments.ceremony_policy.save",
+            error="failed to persist dept_ceremony_policies",
+            exc_info=True,
+        )
+        msg = "Failed to persist department ceremony policies"
+        raise ServiceUnavailableError(msg) from exc

As per coding guidelines, all error paths must log at WARNING or ERROR with context before raising.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/synthorg/api/controllers/departments.py` around lines 491 - 513, The save
path in _save_dept_policies_json currently lets exceptions from
app_state.settings_service.set bubble up; catch any Exception around the await
app_state.settings_service.set(...) call, log a warning using
API_SERVICE_UNAVAILABLE with service="settings" and the error context, and then
raise a ServiceUnavailableError with a clear message (matching the existing
pattern used in _load_dept_policies_json). Ensure you reference
_save_dept_policies_json, app_state.settings_service.set,
API_SERVICE_UNAVAILABLE and ServiceUnavailableError when making the change.