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

Inline comments:
In `@src/synthorg/api/auth/models.py`:
- Around line 60-66: The validator _validate_scoped_departments currently only
rejects scoped_departments when org_roles lacks OrgRole.DEPARTMENT_ADMIN; add
the symmetric check so that if OrgRole.DEPARTMENT_ADMIN is present in org_roles
then scoped_departments must be non-empty (raise ValueError with a clear message
otherwise), and update the same pattern in the analogous validator that
validates scoped_vaults (ensure DEPARTMENT_ADMIN implies a non-empty
scoped_vaults). Ensure both validators are model_validator(mode="after") methods
on the User model and reference OrgRole.DEPARTMENT_ADMIN,
scoped_departments/scoped_vaults and org_roles when enforcing the invariant.

In `@src/synthorg/api/controllers/agents.py`:
- Around line 188-207: The ETag pre-check in the controller is racy because it
runs before OrgMutationService acquires _org_lock; move the compare-and-set into
the locked mutation path by removing the external If-Match validation in
agents.py and instead pass the provided If-Match (or computed expected ETag
string) into AppState.org_mutation_service.update_agent(), then implement the
ETag verification inside update_agent while holding _org_lock using compute_etag
and check_if_match (or equivalent logic) so the check-and-write is atomic;
reference the controller's use of compute_etag/check_if_match and the service
method update_agent to locate where to change behavior.

In `@src/synthorg/api/controllers/company.py`:
- Around line 100-118: The If-Match precondition is checked before acquiring the
mutation lock, allowing a race; move the ETag verification into the mutation
critical section so it runs while the service holds its serialization lock.
Remove the current pre-lock block that reads request.headers.get("if-match") and
computing snapshot/cur_json/current_etag in the controller; instead either (A)
compute the snapshot and call check_if_match(if_match, current_etag, "company")
from inside org_mutation_service.update_company(...) while holding its internal
lock, or (B) change update_company to accept the incoming if_match (e.g., a
parameter named if_match) and have update_company call compute_etag(...) and
check_if_match(...) under the service's lock before applying changes. Ensure you
keep references to compute_etag, check_if_match, app_state.config_resolver, and
org_mutation_service.update_company when relocating the logic so the check
executes atomically with the mutation.

In `@src/synthorg/api/controllers/departments.py`:
- Around line 722-724: get_department() now lowercases the lookup but sibling
endpoints still do exact-name compares, causing inconsistent routing; update
get_department_health(), _require_department_exists(), and the ceremony-policy
resolution logic to perform case-insensitive matching the same way (prefer using
str.casefold() or .lower()) by normalizing the incoming department name and
comparing against dept.name.casefold()/lower() (or reuse the name_lower
variable) so all checks use the same normalized comparison.
- Around line 781-803: The If-Match ETag validation must be performed atomically
with the write to avoid a TOCTOU race: stop computing and checking the ETag
outside the mutation lock and instead move the validation into the mutation
path. Modify the call to app_state.org_mutation_service.update_department (or
add a new method like update_department_with_expected_etag) to accept the
expected ETag (computed with compute_etag) or the raw If-Match header, and
perform check_if_match while holding the service's internal _org_lock before
applying the update so the ETag validation and the write occur under the same
lock.

In `@src/synthorg/api/controllers/users.py`:
- Around line 446-457: The code is incorrectly allowing scoped_departments to be
changed when granting non-department-admin roles and leaving stale scopes when
department_admin is revoked; update the grant and revoke logic so
scoped_departments are only merged/updated when data.role ==
OrgRole.DEPARTMENT_ADMIN being added (use data.scoped_departments to merge into
user.scoped_departments to compute new_scoped) and prevent merging
scoped_departments for other roles, and when removing OrgRole.DEPARTMENT_ADMIN
ensure scoped_departments are cleared/removed from the user (i.e., when
computing new_roles/new_scoped on revocation remove department scopes), applying
the same fix in both the grant block (where new_roles/new_scoped are computed)
and the revoke block (lines referenced around where removal logic computes
roles/scopes).
- Around line 66-72: The GrantOrgRoleRequest model currently types
scoped_departments as tuple[str, ...], which allows blank/whitespace
identifiers; change its type to tuple[NotBlankStr, ...] and import NotBlankStr
from core.types so API input enforces the same non-blank invariant as the
persisted model, keeping the default value as () and leaving model_config
intact; update any tests or call sites that construct GrantOrgRoleRequest to
pass NotBlankStr-compatible strings if needed.

In `@src/synthorg/api/services/org_mutations.py`:
- Around line 487-491: The current check blocks deletion of any c-suite agent;
change it to only block the CEO. Replace the condition that uses existing.level
== SeniorityLevel.C_SUITE with a combined check that also verifies the job title
equals "CEO" (e.g., if existing.level == SeniorityLevel.C_SUITE and
existing.title == "CEO":) and only then raise the ConflictError with the same
message using name; if your model uses a different field for job title (e.g.,
existing.role or existing.position) use that field instead.
- Around line 121-134: The _validate_permutation method raises
ApiValidationError without logging; update it to log a warning/error with
context (entity, current_names, requested_names, and any relevant ids)
immediately before raising (use self.logger.warning or error consistently).
Apply the same pattern to all other places in this module that raise
ConflictError, NotFoundError, or ApiValidationError (the failure sites
referenced in the review: around the blocks covering lines 213-217, 256-258,
307-311, 375-381, 431-450, 519-520) so each raise is preceded by a log entry
including the mutation context to aid tracing.
- Around line 261-270: The current checks (e.g., in update_department() /
update_agent()) use "is not None" so a client-sent explicit null is ignored;
change the guards to detect whether the field was provided (not its value) by
using the model's presence tracking (e.g., for Pydantic use data.__fields_set__
or data.dict(exclude_unset=True), or fallback to checking field in
data.__dict__) and then assign updates["head"] = data.head (allowing None),
updates["ceremony_policy"] = data.ceremony_policy, updates["autonomy_level"] =
data.autonomy_level, and for teams convert to tuple(data.teams) only when the
field was provided; apply the same change pattern to the other occurrence (lines
~444-455).

In `@src/synthorg/persistence/sqlite/migrations.py`:
- Around line 76-77: Replace the dynamic import call with a normal module
import: add "import re" alongside the other imports in migrations.py and change
the _ALLOWED_COLUMN_RE definition to use re.compile(r"^[a-z_][a-z0-9_]*$"); this
updates the symbol _ALLOWED_COLUMN_RE to be created via the standard re module
rather than __import__("re").

In `@tests/unit/api/controllers/test_agent_mutations.py`:
- Around line 205-210: The test currently asserts a 409 when sending a stale
ETag via test_client.patch with headers {"If-Match": '"stale-etag-value000"'},
but the API contract mandates a 412 Precondition Failed on ETag mismatch; update
the assertion to assert resp.status_code == 412 and run the test suite to ensure
the controller handling (agent update path that checks If-Match/ETag) returns
412 for stale/mismatched ETags consistently.

In `@tests/unit/api/controllers/test_department_mutations.py`:
- Around line 50-76: Extend TestUpdateDepartment to cover the
optimistic-concurrency branch by adding tests that exercise the If-Match
precondition: create or fetch a department to obtain its current ETag header,
then perform a PATCH to "/api/v1/departments/{name}" with If-Match set to that
ETag and assert 200 and the updated value (matching case), and add a separate
test that sends a stale/incorrect If-Match value (e.g., "W/\"stale\"" or a
modified ETag) and asserts a 412 Precondition Failed response; reference the
existing test class TestUpdateDepartment and the same endpoints used in
test_update_department_happy_path to locate where to add these checks.

In `@web/src/pages/org-edit/DepartmentCreateDialog.tsx`:
- Around line 48-65: The form validation marks display_name required but the
handleSubmit payload omits it; update the handleSubmit function to include
display_name (e.g., pass display_name: form.display_name.trim()) in the object
sent to onCreate so the mutation receives the display name, keeping the existing
validation using setErrors and pct handling intact; ensure you trim display_name
like name and include it in the await onCreate({...}) call.

In `@web/src/pages/org-edit/DepartmentEditDrawer.tsx`:
- Around line 63-66: The Display Name edits are not being sent on save—update
the handleSave logic in DepartmentEditDrawer so the onUpdate call includes
display_name using the component's displayName state (e.g. await
onUpdate(department.name, { display_name: displayName.trim() || undefined,
budget_percent: Number.isFinite(pct) ? pct : undefined, ceremony_policy:
ceremonyPolicy })). Alternatively, if editing should be disabled, make the
Display Name input readOnly/disabled where displayName state is defined (lines
around its initialization and the input JSX) and remove editing affordance; pick
one approach and implement it consistently.

In `@web/src/pages/org/DeptAdminNode.stories.tsx`:
- Around line 8-32: The Wrapper function currently uses inline styles on the
container div (style={{ width: 400, height: 200 }}); replace that inline style
with Tailwind utility classes by changing the div to use className="w-[400px]
h-[200px]" so the visual fixture matches the codebase patterns while leaving
Wrapper, ReactFlowProvider, and the ReactFlow props (nodes, nodeTypes, etc.)
unchanged.

---

Outside diff comments:
In `@web/src/pages/org-edit/AgentEditDrawer.tsx`:
- Around line 151-156: The Status SelectField is editable but handleSave
excludes status from the onUpdate payload, so persistency is broken; either
include status in the update payload or make it read-only. Update handleSave
(the function calling onUpdate/UpdateAgentOrgRequest) to include form.status
(typed as AgentConfig['status']) in the payload sent to onUpdate, or replace the
editable SelectField with a non-editable display (remove SelectField usage and
render static text) and stop updating form via setForm for status. Ensure
references: SelectField, form.status, setForm, handleSave, onUpdate, and
UpdateAgentOrgRequest are adjusted consistently.

In `@web/src/pages/org-edit/AgentsTab.tsx`:
- Around line 83-92: DepartmentAgentsSection and the SortableAgentItem still
declare and accept a dragDisabled prop that is no longer used or passed from
parents; remove the dragDisabled property from the DepartmentAgentsSection props
interface, its parameter list, and any references inside
DepartmentAgentsSection, then remove the dragDisabled prop from the
SortableAgentItem component signature/props and any conditional logic that
checks it; finally clean up callers (the parent component that previously
forwarded dragDisabled) so they no longer pass the prop and update related type
annotations/imports to keep TS types consistent (look for symbols
DepartmentAgentsSection and SortableAgentItem to locate the code).

In `@web/src/pages/org/DepartmentGroupNode.tsx`:
- Around line 209-217: The "Add agent" chip rendered when showAddAgentButton is
true is styled as interactive but lacks a click handler and proper semantics;
replace the span with a focusable element (preferably a <button>) inside
DepartmentGroupNode and wire it to the handler that opens the agent-creation
flow (e.g., handleAddAgent or openAddAgentModal) and ensure it preserves
existing className, title/aria-label and icon (Plus). Add an onClick on that
button and, if needed, keyboard accessibility props (type="button") so the UI is
actionable and accessible.
- Around line 228-237: The "Add agent" chip rendered when !isEmpty &&
showAddAgentButton in DepartmentGroupNode is missing an onClick handler despite
having interactive styling; wire this span to the same agent-creation flow used
by the empty-state chip so both trigger the same behavior (e.g., call the
existing handler used elsewhere such as the component's create/add agent
function or openCreateAgent modal). Update the span in DepartmentGroupNode (the
block conditioned on !isEmpty && showAddAgentButton) to attach that same onClick
prop and ensure accessibility by adding a role/button or tabindex if needed.

In `@web/src/stores/company.ts`:
- Around line 106-122: The ReadonlySet orgMutationEvents is being recreated for
every WebSocket event; extract it to a module-level constant (e.g.,
ORG_MUTATION_EVENTS) and replace the inline declaration with a reference to that
constant to avoid repeated allocations and improve clarity; update the event
check to use ORG_MUTATION_EVENTS.has(event.event_type) and keep existing calls
to useCompanyStore.getState(), fetchCompanyData(), and fetchDepartmentHealths()
unchanged.

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

Inline comments:
In `@src/synthorg/api/controllers/company.py`:
- Around line 125-128: The response ETag is being computed from the partial
"updated" dict but the If-Match check validated against the full company
snapshot; change the compute_etag call in the PATCH response to hash the
canonical full company snapshot used for validation (either the merged/original
company object or the full company returned by update_company()), e.g.,
serialize that full snapshot with json.dumps(..., sort_keys=True) and pass it to
compute_etag (keeping the same salt/params), so the returned ETag matches the
If-Match source and won't cause spurious 412s.

In `@src/synthorg/api/controllers/users.py`:
- Around line 467-475: The persistence save calls for user mutations (the two
app_state.persistence.users.save(...) calls) are unguarded; wrap each in a
try/except that catches QueryError, log the failure at WARNING or ERROR with
context (include user.id, user.email, the mutation intent such as updated
org_roles/scoped_departments or which fields were changed, and the exception)
using the controller logger (e.g., logger.error(..., exc_info=True)), then
re-raise the exception; keep the existing constructs that create updated via
user.model_copy(update={...}) (variables like updated, now, new_roles,
new_scoped) and apply the same try/except logging pattern to the other save
block mentioned.

In `@src/synthorg/api/services/org_mutations.py`:
- Around line 266-268: In update_department(), before raising NotFoundError when
existing is None, add a warning log that includes the department name and any
relevant context (e.g., current user or request id if available) so operators
can trace the failure; call the module logger (or process logger used elsewhere)
with logger.warning(...) including f"Department {name!r} not found" and any
contextual fields, then raise NotFoundError(msg) as before.
- Around line 532-543: In delete_agent(), add warning logs before raising
errors: log a warning including the agent name (name) when existing is None
(before raising NotFoundError) and log a warning including the agent name and
existing.level/role when rejecting deletion of a C-suite CEO (existing,
SeniorityLevel.C_SUITE and existing.role) before raising ConflictError; use the
module/class logger (e.g., logger.warning) to record these details so the error
conditions are visible in logs.

In `@src/synthorg/persistence/sqlite/migrations.py`:
- Around line 106-109: The ALTER statement uses the variable definition
unchecked; add a defensive validation for the definition parameter in the same
routine (where table, column, definition are used) to reject dangerous tokens
(e.g., semicolons or SQL comment markers). Implement a simple regex test that
fails if definition contains ";" or "--" or "/*" or "*/" (or any other
disallowed characters/patterns you choose), and raise a ValueError with a clear
message when validation fails before calling db.execute; keep the existing
validations for table and column intact.

In `@src/synthorg/persistence/sqlite/user_repo.py`:
- Around line 64-78: The current deserialisation uses `data.get(...) or "[]"`
which treats empty strings as missing; change the logic in the deserialisation
block so you only substitute the default "[]" when the column is actually
missing or None (i.e., `data.get("org_roles") is None`), otherwise pass the raw
value through to json.loads to let empty strings/malformed payloads raise; apply
the same change for `"scoped_departments"`, preserving the subsequent validation
and assignments to `data["org_roles"] = tuple(OrgRole(r) for r in parsed_org)`
and `data["scoped_departments"] = tuple(parsed_dept)`.

In `@web/src/__tests__/pages/org-edit/AgentCreateDialog.test.tsx`:
- Around line 31-35: The current test only asserts the Create Agent button is
enabled which can miss broken submit wiring; update the test that uses
renderDialog() to simulate user behavior: fill the required form fields with
valid values (including extra whitespace), click the Create Agent button via
screen.getByRole('button', { name: /create agent/i }), and assert the mock
onCreate handler was called once with the trimmed payload (verify expected
properties and trimmed strings), ensuring the submit flow and trimming logic
work correctly.

In `@web/src/__tests__/pages/org-edit/AgentEditDrawer.test.tsx`:
- Around line 46-51: The test currently only checks that Save/Delete buttons are
enabled but not that their handlers run; update the test that calls
renderDrawer() to pass jest mock functions for onUpdate and onDelete (or access
the component's props/mocks used by renderDrawer), then simulate user clicks on
the buttons using userEvent.click (or fireEvent.click) targeting the elements
found via screen.getByRole('button', { name: /save/i }) and
screen.getByRole('button', { name: /delete/i }) and assert the onUpdate and
onDelete mocks were called (e.g., toHaveBeenCalled or toHaveBeenCalledTimes).
Ensure you reference renderDrawer, onUpdate, and onDelete when modifying the
test so the click interactions verify the actual handlers execute.

In `@web/src/__tests__/pages/org-edit/DepartmentCreateDialog.test.tsx`:
- Around line 27-30: The test currently only asserts the Create Department
button is enabled; extend it to exercise the submit path by using renderDialog()
to render the form, fill in required inputs with valid values (e.g., department
name and any other required fields), click the Create Department button
(getByRole('button', { name: /create department/i })), and assert that the
mockOnCreate handler (mockOnCreate or the prop passed into renderDialog) was
called with the expected payload; ensure you reference and call the same mock
used by renderDialog so the test fails if submit wiring is broken.

In `@web/src/api/endpoints/company.ts`:
- Around line 43-45: The updateCompany function currently returns a broad
Record<string, unknown>; change its return typing to preserve the company
contract (e.g., Promise<Partial<CompanyConfig>> or
Promise<UpdateCompanyResponse>) and update the apiClient.patch generic and
unwrap call accordingly so the response is typed (replace
ApiResponse<Record<string, unknown>> with ApiResponse<Partial<CompanyConfig>> or
ApiResponse<UpdateCompanyResponse> and change the function signature from
updateCompany(data: UpdateCompanyRequest): Promise<Record<string, unknown>> to
the chosen concrete type) to keep field-level types for callers.

In `@web/src/api/endpoints/users.ts`:
- Around line 15-18: The current GrantOrgRoleRequest allows invalid combinations
(e.g., role: "department_admin" without scoped_departments or other roles with
scoped_departments); replace it with a discriminated union keyed on role (using
the existing OrgRole values) so that the branch where role ===
"department_admin" requires scoped_departments: readonly string[] and every
other role variant either omits scoped_departments or has it typed as
never/undefined, ensuring the invariant is enforced at compile time under
TypeScript 6+ strict settings; update any callers to match the new union shape
and keep the original symbol name GrantOrgRoleRequest for compatibility.

---

Outside diff comments:
In `@web/src/__tests__/stores/company.test.ts`:
- Around line 208-217: The test doesn't assert that updateCompany triggers the
mandatory refetch nor that the rest of config survives the update; update the
test for useCompanyStore.updateCompany to assert mockGetCompanyConfig was called
(and/or calledOnce) after mockUpdateCompany resolves, and verify preserved
fields from mockConfig remain (e.g., check another key besides company_name).
Apply the same pattern to the tests exercising reorderAgents and
reorderDepartments: stub/mockGetCompanyConfig before calling
useCompanyStore.reorderAgents/reorderDepartments, assert the refetch was
invoked, and assert non-target parts of useCompanyStore.getState().config remain
unchanged so the full sync contract is enforced.

In `@web/src/pages/org-edit/AgentsTab.tsx`:
- Around line 235-241: The DepartmentAgentsSection component still defines and
forwards a dragDisabled prop to SortableAgentItem but no callers pass it; remove
the entire dragDisabled prop from DepartmentAgentsSection's props/interface and
stop forwarding it to SortableAgentItem (update the props definition and any
occurrences inside DepartmentAgentsSection where dragDisabled is referenced),
and ensure SortableAgentItem relies solely on its own disabled/drag behavior;
search for the symbol DepartmentAgentsSection and the prop name dragDisabled to
remove all references and adjust any TypeScript types accordingly.

In `@web/src/pages/org-edit/DepartmentsTab.tsx`:
- Around line 309-313: SortableDepartmentCard still declares a disabled prop in
its props interface although callers (e.g., the map usage shown and
DepartmentAgentsSection in AgentsTab) no longer pass it; remove the unused prop
to avoid confusion by deleting disabled from the SortableDepartmentCard
props/type definition and any default value or destructured parameter in the
SortableDepartmentCard component, and remove any internal branches that only run
when disabled is truthy (or replace them with the intended behavior). Ensure you
also remove any leftover disabled references in related helper types or exported
interfaces so the component signature matches actual usage (keep
SortableDepartmentCard, getAgentCount, and setEditDept unchanged).

In `@web/src/pages/org/DepartmentGroupNode.tsx`:
- Around line 228-237: The "Add agent" chip in DepartmentGroupNode (the span
with title "Add agent" and className including cursor-pointer) is styled as
interactive but has no click handler; either wire it to the same click handler
used for the other add button or make it non-interactive. Fix by adding the
appropriate onClick prop (e.g., call the existing addAgent/openAddAgentModal
function used elsewhere in DepartmentGroupNode) and keyboard accessibility
(onKeyPress/role="button" or use a button element) so behavior matches the other
add-agent control, or remove cursor-pointer/hover styles and the title if you
intend it to be a placeholder.
- Around line 209-217: The "Add agent" chip in DepartmentGroupNode (the span
rendered when showAddAgentButton is true) is styled as interactive but has no
onClick handler or prop; either add an onAddAgent callback prop to the component
props and types, thread it from parent, and attach it to the span's onClick (and
optionally keyboard handlers/role) to perform agent creation, or remove the
interactive classes (cursor-pointer and hover:bg-*) and title to make it
non-interactive—update the span rendering and the DepartmentGroupNode props
(e.g., onAddAgent) accordingly so the UI and behavior match.

In `@web/src/stores/company.ts`:
- Around line 112-123: Concurrent refreshes from updateFromWsEvent and direct
post-mutation refetch can race and let an older response overwrite newer store
state; modify the store to serialize or ignore stale completions by adding a
refresh key/version and checking it before applying fetched data. Specifically,
introduce a refreshToken/refreshVersion on the company store that you bump when
starting a fetch (used by fetchCompanyData/getCompanyConfig and
fetchDepartmentHealths) and pass or capture that token in each async result;
before calling set(...) or updating state in those methods, compare the captured
token to the current store token and discard the result if they differ, or
alternatively funnel both callers through a single sequencer helper (e.g., a
refreshCompanyData helper invoked by updateFromWsEvent and post-mutation paths)
to ensure only one in-flight refresh can complete and update the store.

---

Duplicate comments:
In `@src/synthorg/api/controllers/agents.py`:
- Around line 188-210: The If-Match ETag validation is done before acquiring the
organization lock, creating a TOCTOU race; move the compare-and-set into the
mutation path by passing the raw if_match (from request.headers.get("if-match"))
into app_state.org_mutation_service.update_agent and perform the compute_etag +
check_if_match logic inside update_agent while holding the _org_lock so the ETag
comparison and the write are atomic; update update_agent's signature and callers
accordingly and remove the pre-check in agents.py (references: compute_etag,
check_if_match, update_agent, _org_lock).

In `@src/synthorg/api/controllers/company.py`:
- Around line 100-118: The If-Match precondition is checked before acquiring the
mutation lock, allowing race conditions; move the entire ETag computation and
check (using request.headers.get("if-match"),
app_state.config_resolver.get_str/get_agents/get_departments, json.dumps,
compute_etag, and check_if_match) so it executes inside the critical section
immediately after update_company acquires its serialization/lock (i.e., within
the same code path where app_state.org_mutation_service.update_company runs
under the mutation lock) so the computed current_etag reflects the state guarded
by the lock and the check_if_match prevents stale updates.

In `@src/synthorg/api/controllers/departments.py`:
- Around line 722-728: The department name matching is inconsistent:
get_department() uses case-insensitive comparison but get_department_health()
and _require_department_exists() use exact matches, causing mixed-case paths to
sometimes 404; make all lookups consistent by normalizing names to a single case
(e.g., lower()) wherever departments are indexed or compared — update the
construction/usage of dept_by_name and the comparisons in
get_department_health() and _require_department_exists() to use name.lower() (or
always store keys as lowercased names) and ensure incoming name is normalized
before lookup so all endpoints use the same case-insensitive logic.
- Around line 781-803: The If-Match ETag check is done before acquiring the org
mutation lock, causing a TOCTOU race; move the validation into the locked
mutation path so the state used to compute the ETag cannot change before update.
Specifically, stop computing cur_etag and calling check_if_match in the
controller and instead perform the ETag validation inside the org mutation code
(e.g., inside org_mutation_service.update_department or the method that holds
_org_lock): compute_etag from the current department JSON and call
check_if_match while holding the same _org_lock that protects mutations, or
change update_department to accept the incoming If-Match header and perform the
check under that lock. Ensure you reference compute_etag and check_if_match when
relocating the logic so the controller no longer does the pre-lock validation.

In `@src/synthorg/api/controllers/users.py`:
- Around line 66-72: Change the scoped_departments field to use NotBlankStr so
blank/whitespace-only names are rejected at the API boundary: import NotBlankStr
from core.types and update GrantOrgRoleRequest.scoped_departments to type
tuple[NotBlankStr, ...] = () (and do the same update for the corresponding
response DTO's scoped_departments field declared below), keeping the existing
default and model_config.

In `@src/synthorg/api/services/org_mutations.py`:
- Around line 270-286: The update-building logic ignores explicit nulls because
it conditions assignments on both membership in data.model_fields_set and value
checks like "is not None"; change each conditional to only check membership in
data.model_fields_set (e.g., for "head", "autonomy_level", "ceremony_policy",
"teams", and "budget_percent") and assign the corresponding value from data
(allowing None) into the updates dict so a client-sent null clears the field;
keep the special handling for teams to convert to tuple when present (but still
assign tuple(data.teams) even if data.teams is None/empty as appropriate).
- Around line 500-502: The update_agent() code raises NotFoundError when
existing is None without logging context; before raising, log a clear message
including the agent name and any relevant identifiers (e.g., name, maybe org_id
if available) using the module logger or processLogger so failures are
auditable; specifically, add a log call just before the raise in the block that
checks existing (refer to update_agent, the existing variable and name) that
records the attempted update and that the agent was not found, then raise
NotFoundError as before.

In `@tests/unit/api/controllers/test_agent_mutations.py`:
- Around line 204-210: Update the test to expect the correct HTTP status for a
stale If-Match ETag: change the assertion on resp.status_code in the test block
that sends a stale ETag (the test_client.patch call producing resp) from 409 to
412 so the assertion checks for 412 Precondition Failed instead of 409.

In `@tests/unit/api/controllers/test_department_mutations.py`:
- Around line 50-76: Add optimistic-concurrency tests to the
TestUpdateDepartment test class: implement three new test methods that use the
test_client to (1) fetch the department to capture its ETag (GET
/api/v1/departments/{name}), then perform a PATCH with an outdated If-Match
header and assert a 412 Precondition Failed, (2) fetch the current ETag and
PATCH with If-Match set to that value and assert 200 and the budget_percent was
updated, and (3) perform a PATCH without any If-Match header and assert the
update succeeds (200) and budget_percent changes. Use the existing
TestUpdateDepartment class and test_client, target the same endpoint
"/api/v1/departments/{name}", and assert on response.status_code and
resp.json()["data"]["budget_percent"] to validate each scenario.

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

Inline comments:
In `@src/synthorg/api/app.py`:
- Around line 367-374: The code assumes list_users() returns users ordered by
created_at but the UserRepository protocol doesn't guarantee that; to fix, make
_maybe_promote_first_owner defensive by sorting the returned users by their
created_at ascending before selecting the first to promote. Update the logic in
_maybe_promote_first_owner to sort the users list by the User model's created_at
attribute (and keep the current promotion using model_copy and OrgRole.OWNER),
and also add or update the docstring for UserRepository.list_users to state that
implementations must return users ordered oldest-first by created_at (or at
minimum document the ordering expectation).

In `@src/synthorg/api/auth/middleware.py`:
- Around line 357-358: The code is inheriting db_user.org_roles and
scoped_departments into the AuthenticatedUser created by _resolve_api_key_user
which allows an API key (api_key.role) to escalate privileges; change
_resolve_api_key_user so that when constructing AuthenticatedUser it does NOT
copy db_user.org_roles or scoped_departments but instead derives
org_roles/scoped_departments from the api key (e.g., empty or explicitly stored
api_key.org_roles) or otherwise restrict them to the API key's allowed scope,
and/or update require_org_mutation to also validate api_key.role against the
minimum role required for the mutation; update tests to include cases where
api_key.role and db_user.org_roles differ to ensure no escalation.

In `@src/synthorg/api/controllers/users.py`:
- Around line 475-485: The error logging in the exception handler for
app_state.persistence.users.save is emitting the success event constant
API_USER_UPDATED; change this to a failure-specific event constant (e.g.,
API_USER_UPDATE_FAILED or another appropriate constant from
synthorg.observability.events.users) and update the logger.error call to use
that failure constant while preserving the structured fields (user_id, intent,
role, exc_info=True); do the same replacement for the similar logger.error block
referenced around the other occurrence (the block mentioned at lines 551-560) so
error-path logs are distinct from success events.
- Around line 432-476: The in-memory _CEO_LOCK around _get_user_or_404 and
app_state.persistence.users.save is insufficient for multi-instance concurrency;
replace the read-modify-write with a persistence-level atomic operation
(transaction or compare-and-swap) to update user.org_roles and
user.scoped_departments and enforce the "at least one owner" invariant.
Concretely: stop relying only on _CEO_LOCK and instead use an atomic update API
on app_state.persistence.users (e.g., a DB transaction or an
update_if_match/update_where that checks current org_roles or a
version/updated_at field) when modifying the User returned by _get_user_or_404
and when performing owner-revocation logic, ensure the owner-count check and the
write occur in the same transaction/conditional update so two concurrent
requests cannot both succeed and leave zero owners. Ensure the code paths that
add roles (org_roles/scoped_departments) and the paths mentioned around
owner-revocation use this atomic persistence method.

In `@src/synthorg/api/services/org_mutations.py`:
- Around line 317-325: The PATCH path is copying an unchecked ceremony_policy
into the persisted model; add validation to UpdateDepartmentRequest so
ceremony_policy, when present, is validated with
CeremonyPolicyConfig.model_validate() (or removed from the DTO). Implement a
field validator on UpdateDepartmentRequest that runs
CeremonyPolicyConfig.model_validate(data.ceremony_policy) and either returns the
validated dict or raises a validation error, then ensure the controller uses the
validated value when building updates (the code that calls model_copy/update and
the symbols to locate are UpdateDepartmentRequest, CeremonyPolicyConfig, and
model_copy in org_mutations.py).

In `@src/synthorg/api/state.py`:
- Around line 437-443: Add a has_org_mutation_service boolean property mirroring
the pattern used for other services so callers can check availability before
accessing org_mutation_service; implement has_org_mutation_service to return
True when self._org_mutation_service is present (or call the existing
_has_service helper if one exists) and False otherwise, keeping the name
has_org_mutation_service consistent with the org_mutation_service property and
referencing the backing attribute _org_mutation_service.

In `@src/synthorg/persistence/sqlite/migrations.py`:
- Around line 100-105: The allowlist validation currently raises ValueError for
bad table or column without logging; before each raise, emit a structured log at
WARNING or ERROR including the offending value and reason (e.g., "invalid table"
or "invalid column") using the same module-level logger used elsewhere in this
module, then raise the ValueError as before; apply this to the checks that
reference _ALLOWED_TABLES (log the table variable) and _ALLOWED_COLUMN_RE (log
the column variable) so migration failures produce a migration-event style log
with context before the exception is raised.

In `@tests/unit/api/controllers/test_company_mutations.py`:
- Around line 25-34: test_patch_company_observer_denied mutates the shared
test_client.headers with make_auth_headers("observer"), which can leak auth
state to other tests; instead, avoid mutating the shared fixture by creating a
fresh client or using a temp headers dict for the request (e.g., pass
headers=make_auth_headers("observer") into test_client.patch) or by copying
test_client.headers before updating and restoring it after the test; update
test_patch_company_observer_denied to use a request-scoped headers argument or a
new TestClient instance so test_client.headers is not permanently modified.
- Around line 52-58: The test expects a 201 but the reorder endpoint modifies
existing resources; update the controller that handles
"/api/v1/company/reorder-departments" (the reorder_departments handler) to
explicitly return status 200 OK (add status_code=200 on its route decorator or
return Response with status_code=200) and then change this test's assertion to
assert resp.status_code == 200 instead of 201 so REST semantics and tests are
aligned.

In `@tests/unit/api/controllers/test_user_org_roles.py`:
- Around line 70-99: Add a negative test that asserts the controller rejects
payloads containing "scoped_departments" when the role is not
"department_admin": create a new test (e.g.,
test_grant_non_department_admin_with_scopes_422) analogous to
test_grant_department_admin_without_scoped_departments_422 that seeds a user
with _seed_target_user, posts to f"/api/v1/users/{user.id}/org-roles" with json
{"role": "owner", "scoped_departments": ["eng"]}, and asserts resp.status_code
== 422 to cover the invariant that only department_admin may include
scoped_departments.

In `@web/src/__tests__/pages/org-edit/AgentEditDrawer.test.tsx`:
- Around line 80-87: The test currently performs fireEvent.click inside waitFor
which can cause multiple clicks; refactor the block so waitFor only performs
assertions or element queries and move the click outside it — use findAllByRole
or await screen.findAllByRole to get the buttons (or use waitFor to assert
existence), then locate the destructive button by checking
getAttribute('data-variant') === 'destructive' and call
fireEvent.click(destructiveButton) after the await; update references to
waitFor, screen.getAllByRole/findAllByRole, and fireEvent.click accordingly.

In `@web/src/__tests__/pages/org-edit/DepartmentEditDrawer.test.tsx`:
- Around line 61-69: The test currently only checks enabled state for
save/delete but should verify the click handlers run and avoid fragile indexing;
update the test that uses renderDrawer to pass or access mock handler functions
(e.g., onSave, onDelete) and replace deleteButtons[deleteButtons.length - 1]
with a robust query (like getByRole with an accessible name that identifies the
department delete or a data-testid on the department delete button). Then
simulate user clicks on the Save button (saveButton) and the department delete
button (deptDelete) and assert the corresponding mock handlers were called
(e.g., expect(onSave).toHaveBeenCalled(), expect(onDelete).toHaveBeenCalled()).

In `@web/src/__tests__/pages/org-edit/YamlEditorPanel.test.tsx`:
- Around line 30-34: The test currently checks that the Save YAML button is
disabled initially; extend the YamlEditorPanel test by simulating a user change
to the editor textarea (targeting the textarea element rendered by
YamlEditorPanel, e.g., via screen.getByRole('textbox') or querying the textarea)
using fireEvent.change or userEvent.type, then re-query the Save YAML button and
assert it is enabled; reference the test case name in this file and the
YamlEditorPanel component so you add the edit-and-assert step within the same
it('disables Save YAML button when form is not dirty', ...) test.

In `@web/src/__tests__/stores/company.test.ts`:
- Around line 261-265: The test currently only checks the department name which
doesn't prove the update path; change the assertions to verify a field that the
update actually mutates (e.g., budget_percent). After calling
useCompanyStore.getState().updateDepartment('engineering', { budget_percent: 50
}), assert that result.budget_percent === 50 and that
useCompanyStore.getState().config!.departments[0]!.budget_percent === 50 so both
the returned object (result) and the store state reflect the mutation; keep the
call to updateDepartment and replace the name assertions with these
budget_percent checks.

In `@web/src/pages/org/build-org-tree.ts`:
- Around line 364-372: The dept-admin graph node IDs are currently built only
from admin.id inside the loop over deptAdmins (DeptAdminInfo), which can produce
duplicate node IDs when the same user is admin of multiple departments; update
the adminNodeId construction (where adminNodeId is defined) to include the
department scope (matchedDept.name) so each emitted node ID is unique per
department (e.g., combine admin.id and matchedDept.name) and ensure any
references to adminNodeId in the nodes/push use the new department-scoped ID.

In `@web/src/pages/org/DeptAdminNode.tsx`:
- Around line 27-32: DeptAdminNode.tsx currently hardcodes the node width via
the class "w-[200px]" while build-org-tree.ts separately hardcodes width=200 and
height=70, which will drift; extract a single shared size definition (e.g.,
NODE_WIDTH and NODE_HEIGHT or a NODE_SIZE object) into a common module or a
shared export (create or use a shared constants file) and update DeptAdminNode
(replace "w-[200px]" and any implicit height assumptions) and build-org-tree.ts
(replace width=200 and height=70) to import and use those constants so both
components use the same source of truth.

In `@web/src/pages/org/OwnerNode.tsx`:
- Around line 12-13: The subtitle "You" in the OwnerNode component is hardcoded
and mislabels other owner nodes in multi-owner charts; update the OwnerNode
component to render "You" only when the node represents the current user (e.g.,
use an existing prop or context such as currentUserId or isCurrentUser) and for
other owner nodes render the appropriate identity label (e.g., owner.name,
owner.role, or nothing). Locate the OwnerNode React component and replace the
static "You" subtitle with a conditional that checks the node's identity (for
example: if props.isCurrentUser || owner.id === currentUserId then "You" else
owner.displayName/role).

In `@web/src/pages/OrgChartPage.tsx`:
- Line 749: Currently nodesDraggable is a global boolean
(nodesDraggable={viewMode === 'hierarchy'}) which enables drag for all node
types while your drag handlers (handleNodeDragStart and handleNodeDragStop) only
support agent nodes; change nodesDraggable to be conditional per-node so
dragging is enabled only when viewMode === 'hierarchy' AND the node's type is
'agent' (i.e., use a function or per-node predicate that checks node.type ===
'agent' together with viewMode) so the drag affordance matches the mutation
logic in handleNodeDragStart/handleNodeDragStop.

In `@web/src/stores/company.ts`:
- Around line 93-105: fetchCompanyData currently returns early on stale
responses (when get()._refreshVersion !== version) which lets callers like
updateCompany and reorderAgents treat a skipped refresh as success; change
fetchCompanyData to indicate whether it actually applied its result by returning
a boolean (or rejecting) instead of returning void: on the early "stale
response" paths in fetchCompanyData return false, on successful apply return
true, and on real errors keep throwing after setting error state; then update
callers (updateCompany, reorderAgents and other flows) to await fetchCompanyData
and verify it returned true (or retry/await the latest in-flight refresh) before
resolving their own promises so they don't consider a superseded refresh as a
successful sync. Ensure references to _refreshVersion are used exactly as in the
diff to detect staleness.

---

Outside diff comments:
In `@src/synthorg/api/controllers/departments.py`:
- Around line 903-929: The lookup is case-insensitive but later calls still use
the caller's casing; change the logic to bind the matched Department object from
dept_by_name (e.g., dept = dept_by_name[name.lower()]) after the existence check
and then use dept.name for filtering, aggregation and logging: call
_filter_agents_by_department(agents, dept.name), pass dept.name into
_assemble_department_health, and use dept.name in the
logger.debug/API_DEPARTMENT_HEALTH_QUERIED call; keep the NotFoundError behavior
when no match is found.

In `@web/src/pages/org-edit/AgentsTab.tsx`:
- Around line 235-241: DepartmentAgentsSection still declares a dragDisabled
prop and forwards it to SortableAgentItem although no parent passes it; remove
dragDisabled from DepartmentAgentsSection's props interface and delete any
forwarding/usage (the prop passed into SortableAgentItem), and update
SortableAgentItem usage/signature if it expects that prop so types remain
consistent; ensure no other components reference
DepartmentAgentsSection.dragDisabled and run TS checks to confirm the prop is
fully removed.

In `@web/src/stores/company.ts`:
- Line 63: The reorderAgents flow is sending orderedIds to apiReorderAgents
while the optimistic update uses a.id ?? a.name and the request body uses
agent_names, causing divergence when agents have ids; update reorderAgents (and
any callers) to normalize to ordered agent names before calling
apiReorderAgents: either change the public surface to accept orderedNames
(string[]) or, inside reorderAgents, map each id in orderedIds to the current
agent.name via the store lookup (using the agents collection referenced where
optimistic keys are computed) and pass that array as agent_names to
apiReorderAgents; ensure the optimistic update and the POST payload both use the
same name-based permutation (adjust references around reorderAgents,
apiReorderAgents, and the a.id ?? a.name logic).

---

Duplicate comments:
In `@src/synthorg/api/controllers/departments.py`:
- Around line 433-437: The department existence check currently compares
case-insensitively but does not return the canonical department identifier,
causing duplicate keys like "ENG" vs "eng" in ceremony-policy storage; modify
the loop in the function that calls app_state.config_resolver.get_departments()
so that when a matching department is found it returns the matched dept (or at
least dept.name) instead of returning None, and then propagate and use that
canonical dept.name in the helper functions _get_dept_ceremony_override,
_set_dept_ceremony_override and _clear_dept_ceremony_override (update callers to
accept the returned Department/name) so all reads/writes use the same canonical
key.

In `@src/synthorg/api/services/org_mutations.py`:
- Around line 505-506: The current check `if "autonomy_level" in fields_set and
data.autonomy_level is not None:` prevents clients from explicitly clearing
autonomy by sending null; change the logic so presence in `fields_set` alone
controls updating: when `"autonomy_level"` is in `fields_set`, assign
`updates["autonomy_level"] = data.autonomy_level` (allowing None) so an explicit
null clears the field; update the code around `fields_set`,
`data.autonomy_level`, and the `updates` dict in org_mutations.py accordingly.

In `@tests/unit/api/controllers/test_agent_mutations.py`:
- Around line 187-210: The test named test_stale_etag_returns_409 is asserting
the wrong status and has a misleading name; rename the test (e.g.,
test_stale_etag_returns_412 or test_if_match_miss_returns_412) and change the
assertion to expect HTTP 412 (Precondition Failed) instead of 409, updating the
assertion in the test_stale_etag_returns_409 function and any references to that
test name accordingly so the behavior and name match the If-Match contract.

In `@tests/unit/api/controllers/test_department_mutations.py`:
- Around line 77-90: The test_update_department_stale_etag test is asserting 409
for a stale If-Match value but should assert 412 Precondition Failed; update the
assertion to expect 412 (keep the request to "/api/v1/departments/eng" with
headers {"If-Match": '"stale-etag-value000"'} and the same payload) so the
optimistic-concurrency contract is validated correctly.