Refactor sitemap CSV import around a canonical resolved plan (#767)

[flesher]

Reviewable diff: +2135/-1594 across 6 files (excludes generated, test, and story files).

Summary

Site-map CSV import lets an operator upload a spreadsheet of sites, buildings, racks, and miners and have Proto Fleet reconcile the fleet's topology to match it. Previously each stage of that flow — the dry-run preview, the validation errors, the "did anything change since you previewed" token, and the final database writes — worked out from scratch what the CSV meant. Those four independent re-derivations drifted apart, which is where most of the reported edge-case bugs came from.

This PR makes the import resolve the CSV into one canonical plan first, and then has preview, validation, the commit token, and apply all read from that single plan. Same feature, same CSV format from the user's point of view; the internals now have a single source of truth instead of four.

How it works

The import runs in one direction: raw CSV → resolved plan → everything else.

1. Parse. The uploaded bytes are split into the four sections (SITE, BUILDING, RACK, MINER), each a list of rows.

2. Load a snapshot. The server reads what currently exists in the database: all sites, buildings, and racks for the org, plus the miners the uploader is allowed to see. Miners the uploader can't see but which still occupy a rack slot are kept in a separate "hidden members" list so they're never silently trampled or deleted.

3. Resolve references. Every parent link in the CSV is a single cell, read like this:

   • blank → unassigned
   • a bare number (e.g. building = 12) → the existing building with id 12
   • NAME:x (e.g. building = NAME:Warehouse B) → a building being created in this same import whose name is "Warehouse B"

   This replaces the old scheme where each link was two columns (a name and an id) that could disagree. Resolving also fills in implied ancestors (a rack that names a building inherits that building's site) and records the matched building's real id internally, so two different buildings that happen to share the same (site, name) stay distinct.

4. Build the plan. Each row becomes a resolved node (site / building / rack / miner) tagged with an action — none, create, or update — decided by comparing the canonical CSV row against the live row field by field. Alongside the nodes, a "topology view" describes the desired end state (CSV merged over the current snapshot) that the validators check against.

5. Validate, count, tokenize — all off the plan. Grid-position collisions, rack and building capacity, slot conflicts and bounds, and rename permissions all read the resolved nodes and the shared topology view (and fold in those hidden members). The preview change-counts come from the node actions. The commit token is a hash of the resolved plan plus the live snapshot fingerprint.

6. Apply. On commit, the token is re-checked (if the CSV or the live database changed since the dry-run, the token won't match and the commit is refused). Then a single database transaction walks the resolved nodes and performs each create/update, and — because "remove omitted rows" is now the only removal mode — deletes anything the CSV left out.

Example of the token guard: you dry-run and get a token. A teammate moves a rack in the UI before you hit commit. Your token no longer matches the fresh snapshot, so the commit is rejected and you're asked to dry-run again, rather than applying a stale plan.

flowchart TD
    CSV["Uploaded CSV"] --> Parse["Parse into SITE/BUILDING/RACK/MINER rows"]
    DB["Live database"] --> Snap["Load snapshot (org topology + visible miners + hidden members)"]
    Parse --> Resolve["Resolve references (single-cell: blank / id / NAME:x)"]
    Snap --> Resolve
    Resolve --> Plan["Resolved plan: nodes tagged none/create/update + topology view"]
    Plan --> Preview["Preview change counts"]
    Plan --> Validate["Validation (grid, capacity, slots, renames)"]
    Plan --> Token["Commit token = hash(plan + snapshot)"]
    Plan --> Apply["Apply: one transaction, walk nodes, delete omitted"]
    Apply --> DB

Loading

sequenceDiagram
    actor User
    participant Import as Import service
    participant DB
    User->>Import: Upload CSV (dry_run)
    Import->>DB: Load snapshot
    Import->>Import: Resolve plan, validate, count
    Import-->>User: Preview + commit token
    User->>Import: Commit (with token)
    Import->>DB: Reload snapshot
    Import->>Import: Recompute token
    alt token matches
        Import->>DB: Apply plan in one transaction
        Import-->>User: Success + changes
    else snapshot drifted
        Import-->>User: Rejected — dry-run again
    end

Loading

Areas of the code involved

Area / package / file	What changed	Why it matters for review
server/internal/domain/sitemap/resolved.go	New. The canonical plan: resolved node types, resolveReferences, resolvePlan, the topology view, action classification, and all validators that now read the plan.	This is the heart of the PR — the single source of truth everything else consumes.
server/internal/domain/sitemap/service.go	Rewritten around the plan: export is id-authoritative, preview/token/apply read the plan, and the old per-stage re-derivation helpers are deleted (net ~-900 lines).	Confirm preview, token, and apply all derive from the plan and that the deleted helpers have no remaining callers.
server/internal/domain/buildings/models/models.go	New helpers GridCapacity and RackCapacityExceeded — the building rack-capacity rule extracted so both the buildings domain and sitemap import share one implementation.	A previously duplicated invariant; check the two callers agree on semantics.
server/internal/domain/buildings/service.go	Uses the extracted capacity helper instead of its own inline arithmetic.	Behavior-preserving; verify the net-membership count passed in is unchanged.
client/.../SiteMapCsvImportModal.tsx	Removes the "leave omitted rows in place" radio option; the omission prompt now just confirms removal.	Matches the server: remove-omitted is the only committable behavior.
docs/plans/2026-07-22-767-sitemap-canonical-plan-tdd.md	New design/plan document.	Optional context for the refactor's phasing and out-of-scope items.

Key technical decisions & trade-offs

• Single reference cell (id or NAME:) instead of a name+id column pair. The old pair could contradict itself and needed reconciliation logic; one cell removes that whole class of ambiguity. Trade-off: existing entities must be referenced by id, and same-import creations by NAME: — bare names are a validation error.
• Id-precise building identity via an internal companion cell. Two buildings can legitimately share a (site, name) pair, so the resolved building id is carried alongside the canonical name to keep them distinct in grid/capacity/move checks. Alternative — keying purely on (site, name) — silently merged them.
• Remove-omitted is the only removal mode. "Leave in place" was dropped as a committable option (server and client). Simpler contract; the dead OMISSION_MODE_LEAVE_IN_PLACE proto enum value is left for a follow-up because proto regen is unreliable in this worktree.
• Commit token hashes the resolved plan, not the raw rows + preview summary. A token can no longer certify a topology that apply would resolve differently. Trade-off: token values change (they're opaque to clients, so this is safe).
• Extracted the rack-capacity invariant into the buildings domain rather than keeping sitemap's private copy, so the rule has one home. Kept the signature capacity-agnostic (aisles, racksPerAisle, resultingCount) so a future interactive-reparent fix (Rack reparent: over-capacity false rejection when a Manage-racks confirm both removes and reparents into a full building #778) can reuse it.

Testing & validation

• go build ./..., go vet, and gofmt -l clean; the full internal/domain/sitemap package test suite passes, as do the new buildings/models capacity tests.
• Behavior is asserted against the resolved plan so the previously-reported edge cases can't silently regress, including: renamed/moved buildings staying visible to the rack grid/capacity validators; id-disambiguation of duplicate (site, name) buildings; hidden rack members counted in slot/capacity/conflict checks; and remove-omitted never unassigning a hidden miner.
• Commit-token behavior is covered both ways: the token changes when the live snapshot drifts, and when a CSV edit changes the resolved plan even against an identical snapshot.
• Not covered: deletion of the now-dead OMISSION_MODE_LEAVE_IN_PLACE proto enum value (deferred — needs proto regen outside a worktree); no new end-to-end/integration test beyond the existing service-level suite.

[github-actions]

🔐 Codex Security Review

Note: This is an automated security-focused code review generated by Codex.
It should be used as a supplementary check alongside human review.
False positives are possible - use your judgment.

Scope summary

• Reviewed pull request diff only (fa57bd3fd3044e3800d1b3a91dff3422c4ef4898...1a919466ccc0041609b55d465cf9d17de0f14e97, exact PR three-dot diff)
• Model: gpt-5.5

💡 Click "edited" above to see previous reviews for this PR.

---

Review Summary

Overall Risk: MEDIUM

Findings

[MEDIUM] Contradictory parent references are silently rewritten

• Category: Reliability
• Location: server/internal/domain/sitemap/service.go:2815
• Description: During reference resolution, a RACK row first resolves the explicit site cell, but then a non-empty building cell wins and overwrites the site with the building's site without checking whether the explicit site disagreed. The same pattern exists for MINER rows when building or rack implies different ancestors. A CSV row can therefore say site A and building/rack under site B, and the importer will silently apply site B.
• Impact: Operators can import a contradictory CSV and move racks or miners to a different site/building than the row explicitly stated, which is especially risky in destructive remove-omitted imports and automated CSV editing workflows.
• Recommendation: Preserve the original explicit ancestor cells while resolving references. If a more-specific reference implies a site/building that conflicts with an explicitly supplied less-specific reference, return a validation error instead of overwriting the row.

[MEDIUM] LEAVE_IN_PLACE omission mode no longer produces a committable plan

• Category: Other
• Location: server/internal/domain/sitemap/service.go:1162
• Description: The build-plan early return now triggers for every omission mode except REMOVE_OMITTED. Because ImportSiteMapCsv then returns omission_choice_required whenever omissions exist and mode is not REMOVE_OMITTED, an API caller that explicitly sends OMISSION_MODE_LEAVE_IN_PLACE can no longer receive changes or a commit token. The proto enum still exposes this mode.
• Impact: Existing clients that relied on partial CSV imports while leaving omitted entities untouched are forced into a non-committable preview loop or must choose the destructive remove-omitted path. That is a behavioral regression in the public RPC contract.
• Recommendation: Either restore the old behavior for OMISSION_MODE_LEAVE_IN_PLACE by only requiring a choice for UNSPECIFIED, or remove/deprecate the enum and return a clear validation error when callers send it.

Notes

I reviewed .git/codex-review.diff as the authoritative scope. I did not find changed-hunk evidence of auth, SQL injection, command injection, cryptostealing/pool hijack, or protobuf wire-format issues. I could not run go test ./server/internal/domain/sitemap because the sandbox is read-only and Go could not create a module cache.

---

_{Generated by Codex Security Review |
Triggered by: @flesher |
Review workflow run}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 77b9f05038

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[Copilot]

Pull request overview

Refactors the sitemap CSV import flow to resolve the uploaded CSV + live snapshot into a single canonical “resolved plan” representation that downstream stages (preview, validation, commit token, apply) can consume consistently. This also extracts the building rack-capacity invariant into the buildings domain so both the buildings service and sitemap import share the same logic.

Changes:

• Introduces resolvedPlan / resolved node types plus plan-driven validation and change counting.
• Extracts rack-capacity helpers (GridCapacity, RackCapacityExceeded) into server/internal/domain/buildings/models and updates callers.
• Updates ProtoFleet’s import modal UX to remove the “leave omitted rows in place” option and treat omissions as a removal confirmation step; adds/updates characterization tests accordingly.

Reviewed changes

Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
server/internal/domain/sitemap/service_test.go	Updates sitemap import/export tests for the new single-cell reference model, plan-based tokening, and plan-driven apply helpers.
server/internal/domain/sitemap/resolved.go	Adds the canonical resolved-plan types and a large portion of plan-driven validation and change classification logic.
server/internal/domain/sitemap/resolved_test.go	Adds unit tests for resolved-plan identity classification, topology linking expectations, and omission-awareness behavior.
server/internal/domain/buildings/service.go	Switches capacity math to shared helpers and uses RackCapacityExceeded for the assignment guard.
server/internal/domain/buildings/models/models.go	Adds shared capacity helpers GridCapacity and RackCapacityExceeded.
server/internal/domain/buildings/models/models_test.go	Adds unit tests for the new capacity helpers.
docs/plans/2026-07-22-767-sitemap-canonical-plan-tdd.md	Adds a TDD documenting the canonical-plan refactor approach and constraints.
client/src/protoFleet/features/fleetManagement/components/SiteMapCsvImportModal.tsx	Removes the “leave omitted rows” option and updates the omission prompt copy to a removal confirmation flow.

[flesher]

🤖 Triaged both Codex Security Review findings:

Finding 1 — leave-in-place omission mode (MEDIUM): Working as intended. Remove-omitted is now the only committable omission mode (locked product decision, commit f3c2a7c). The commit path rejects any other mode, and the preview surfaces the omitted counts and requires explicit confirmation before removal. The dead OMISSION_MODE_LEAVE_IN_PLACE proto enum value is slated for deletion in a follow-up (proto regen is unreliable in this worktree, so the enum cleanup is tracked separately).

Finding 2 — silent parent-reference overwrite (MEDIUM): Fixed in 18421d4. This is intended precedence, not a silent bug: for a single row, a rack reference wins over a building reference, which wins over a site reference. The finding pointed at dead id-vs-name mismatch validation branches that could never fire under the single-reference-cell model. Those dead branches are now deleted and the rack-wins precedence is documented inline in validatePlacementConsistency.

Independent security review: LOW risk, no new vulnerabilities. Import stays inside a single RunInTx; the commit token is an optimistic-concurrency drift check recomputed and compared server-side. One pre-existing consideration noted — during import the plan reflects org-wide topology (sites/buildings/racks load org-wide, only miners are viewer-scoped), consistent with the accepted #520 decision; not changed here.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 18421d4f06

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[ankitgoswami]

nit: something like import_plan would have been clearer

[ankitgoswami]

I think we can simplify and DRY this up a bit, the helpers seem to be doing similar operations. Maybe call desiredBuildingList once, then iterate over the desiredBuildings and create the different maps (buildingsByKey, buildingsByLayoutID, buildingsByCapacityKey).
Can also pass buildingsByKey and buildingsByID to desiredRackMap so it can use them to resolve rack parents.

This way there will be fewer places to edit if we need to change the csv in the future