Phase 16: Extract Farm.Modules.Inventory from Farm.Web.Api - #2081
Conversation
Add the Inventory module and test project scaffolding, wire both into farm-web.sln, and add the required InternalsVisibleTo seams before any file moves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bishop review finding: the dotnet-build job's per-project upload-artifact steps were missing the Farm.Modules.Inventory.Tests entry, so once the inventory/tests_inventory selector bucket selects that leg, the dotnet-test job's download step would fail. Mirrors the Farm.Modules.Identity.Tests block added in commit 61a5165. Also strips an incidental UTF-8 BOM that was introduced in RouteTableSnapshot.txt during the targeted assembly-qualifier substitution; the original file had none. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 14de1676-f3db-46ca-a91e-043b4963ea7a
|
Squad-Reviewer: bishop |
|
Squad-Reviewer: hicks |
|
Squad-Reviewer: vasquez |
FilamentFallbackGroupsController.cs moved from src/api/Controllers/ to src/modules/Farm.Modules.Inventory/Controllers/ as part of the Farm.Modules.Inventory extraction. The architecture guard's allowlist keys paths relative to the scan root the file is found under, so the entry must be updated to the new module-relative path or the presubmit guard (issue #1966 regression test) flags it as a new violation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 14de1676-f3db-46ca-a91e-043b4963ea7a
|
Squad-Reviewer: bishop |
|
Squad-Reviewer: hicks |
|
Squad-Reviewer: vasquez |
…rm-modules-inventory # Conflicts: # docs/CI.md # scripts/ci/select-dotnet-tests.sh # scripts/ci/tests/test-select-dotnet-tests.sh # src/api/Program.cs # src/tests/Farm.Web.Api.Tests/Startup/RouteTableSnapshot.txt
|
Squad-Reviewer: hicks |
|
Squad-Reviewer: bishop Read-only adversarial review (no build/test/install run). Re-reviewed the merge of Verified at this head:
Non-blocking (unchanged from prior rounds, do not gate this PR):
Evidence request (cannot verify read-only): the author's reported |
|
Squad-Reviewer: vasquez |
…ration #2082) Sibling phase Farm.Modules.Administration (#2082, issue #2042) merged into development while this PR's 3-way review was in flight, producing 11 conflicting files (a superset of the earlier Gcode #2078 resync). All conflicts are the "shared central registry" pattern already seen with the Gcode resync: two independently-extracted modules touching the same registration/CI plumbing with no actual code overlap. Resolved by unioning both sides everywhere: - src/api/Program.cs: register both InventoryApiModule and AdministrationApiModule via AddApiModules(...). - src/api/Farm.Web.Api.csproj, src/farm-web.sln: ProjectReference / solution membership for both modules' production + test projects. - src/tests/Farm.Web.Api.Tests/AssemblyInfo.ModuleTestsVisible.cs: InternalsVisibleTo for both Farm.Modules.Inventory.Tests and Farm.Modules.Administration.Tests. - scripts/docker/dockerfiles/Dockerfile{,.api}: COPY lines for both modules' csproj files. - .github/workflows/ci.yml: separate "Upload ... Tests build" artifact steps for both modules. - docs/CI.md: bucket-mapping table rows and prose for both modules. - scripts/ci/dotnet-test-manifest.json: separate manifest entries for Farm.Modules.Inventory.Tests and Farm.Modules.Administration.Tests. - scripts/ci/select-dotnet-tests.sh: bucket flags, classify_path cases, case-dispatch, full-build trigger condition, and test_names unions for both inventory and administration buckets. - scripts/ci/tests/test-select-dotnet-tests.sh: reconstructed interleaved case_inventory_*/case_administration_* test functions (git's diff3 merge interleaved these at overlapping line ranges since both sets of functions are structurally similar); registered both sets of case_* functions in the test runner list. Verified after resolution: dotnet build (0 errors), all 113 scripts/ci/tests/test-select-dotnet-tests.sh cases pass, all 170 Farm.Modules.Inventory.Tests pass, RouteTableSnapshot + NoClientsAll ArchitectureTests pass (2/2), dotnet-test-manifest.json is valid JSON, no conflict markers remain (git diff --check clean).
|
Squad-Reviewer: bishop |
|
Squad-Reviewer: hicks |
|
Squad-Reviewer: vasquez |
…les.Inventory The merge with origin/development (Phase 16, Farm.Modules.Inventory, PR #2081) moved FilamentCoverageControllerTests.cs out of Farm.Web.Api.Tests, whose GlobalUsings.cs global-using's Farm.Testing.Shared for the whole project. The new Farm.Modules.Inventory.Tests project lacks that global using, so the already-correctly-merged AppDbTestHelpers call site (renamed from TestInfrastructure.TestHelpers by this PR's own cohort-C move, commit 0a62464) failed to resolve. Add the explicit using directive to this one file; no other file references AppDbTestHelpers from Inventory.Tests.
…ase4 Resolve RouteTableSnapshot.txt conflict by regenerating the snapshot from the merged code after merging Phase 16 (Extract Farm.Modules.Inventory, #2081). Phase 16 re-qualified inventory controller routes to the Farm.Modules.Inventory assembly while this branch added six ProfileFamiliesController routes; both edited nearby lines in the sorted generated snapshot. Regenerated via the BuildRouteTable harness so the only delta versus origin/development is the six new ProfileFamilies routes, with Phase 16's inventory qualifiers intact. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
## Summary Phase 15 of epic #2019: extracts the OctoPrint-compat auth surface plus the NFC/camera device controllers into a new `Farm.Modules.Devices` module, following the established pilot pattern from Phase 8 (`Farm.Modules.SmartPlug`, #2036) and repeated in Phases 9/10/12/13 (Maintenance, Calibration, PrintQueue, Identity). Closes #2043 ## What moved (namespaces unchanged, move-first-rename-last) - `Services/OctoPrint/OctoPrintAuthService.cs` (`IOctoPrintAuthService` + `OctoPrintAuthService`) - `Authentication/OctoPrintApiKeyAuthenticationHandler.cs` - `Filters/OctoPrintApiKeyAttribute.cs` - `Controllers/NfcController.cs` - `Controllers/NfcDevicesController.cs` - `Controllers/CamerasController.cs` - `Controllers/CameraSnapshotsController.cs` - 2 test files: `OctoPrintAuthServiceTests.cs`, `OctoPrintApiKeyAttributeTests.cs` → `Farm.Modules.Devices.Tests` **Scope note:** the issue text says "6 controllers"; the actual final count is 4 (`NfcController`, `NfcDevicesController`, `CamerasController`, `CameraSnapshotsController`). `OctoPrintCompatController.cs` was explicitly out of scope and stays in `Farm.Web.Api`, unchanged. `AdminHomeAssistantController.cs` was originally moved here too, but Phase 14 (`Farm.Modules.Administration`, #2082) independently claimed the same controller and merged into `development` first; resolved during the `origin/development` merge by deferring to Administration, since only one module can own a given controller. As part of that merge, 4 security architecture guard tests in `Farm.Web.Api.Tests/Security/` that used `AdminHomeAssistantController` as their Devices-assembly "anchor" were re-anchored to `NfcController` instead (the prior anchor silently stopped covering Devices once the controller moved), and a previously-missing Administration self-anchor was added to all 4 (a Phase 14 gap discovered only because of this collision). Host-integration tests for the moved controllers (`CameraSnapshotsControllerTests`, `NfcDevicesControllerAuthenticationTests`, `RouteTableSnapshotTests`) remain in `Farm.Web.Api.Tests`, matching the same pattern used by every prior module phase. (`AdminHomeAssistantControllerTests` now belongs to Administration.) ## Acceptance criteria checklist - [x] `Farm.Modules.Devices.csproj` + `.Tests.csproj` added to `src/farm-web.sln`; depends on `Farm.Infrastructure` + `Farm.Backend.Plugin.Core` (transitively via infra, same as every other module) - [x] `InternalsVisibleTo` added in the first commit, before any file move - [x] Registered in `scripts/ci/dotnet-test-manifest.json` (pathPrefixes, dependsOnProjects, defaultFilter, leg) - [x] `scripts/ci/select-dotnet-tests.sh` + `scripts/ci/tests/test-select-dotnet-tests.sh` extended - [x] `scripts/ci/generate-codeql-slnf.sh` — no manual edit needed (derives its project list from the `.sln` automatically); `Farm.Modules.Devices.csproj` verified present - [x] `scripts/docker/dockerfiles/Dockerfile.api` COPY ordering updated - [x] `.github/workflows/ci.yml` upload-artifact step added for the new test leg - [x] Required CI job display names preserved; no new required contexts - [x] Route-table snapshot passes (assembly-tag-only delta: `Farm.Web.Api::` → `Farm.Modules.Devices::`) - [x] `OctoPrintApiKeyAuthenticationHandler` still registered as an auth scheme from the host — only the `using` changed; scheme name/registration point unaffected - [x] OctoPrint-compat routes unchanged (`OctoPrintCompatController` untouched) - [x] Total test count unchanged - [x] No `/api/*` contract change ## Validation ``` cd src dotnet build ./farm-web.sln -c Debug # 0 errors, no new warnings dotnet test ./tests/Farm.Modules.Devices.Tests/...csproj --no-build # 8/8 passed dotnet test .../Farm.Web.Api.Tests.csproj --filter "RouteTableSnapshot|CameraSnapshotsController|NfcDevicesControllerAuthentication|OctoPrintCompat" --no-build # passed dotnet test .../Farm.Web.Api.Tests.csproj --filter "AuthorizeRolesGateArchitectureTests|PermissionGrantPathTests|RoleToPermissionMigrationCompletenessTests|QueueEnqueuePermissionArchitectureTests" --no-build # passed dotnet format ./farm-web.sln --verify-no-changes # scoped to this task's files: clean bash scripts/ci/tests/test-select-dotnet-tests.sh # 116/116 passed (post-merge) bash scripts/ci/tests/test-dotnet-test-manifest.sh # passed ``` Branch merged three times with `origin/development` to pick up concurrently-landing phases: - First merge: concurrent Phase 13 (Identity) landed first. - Second merge (head SHA `679148955885a116f72e4548ff422868d72f0040`, fixed up at `ef996bc5777a1a1dca49306a42fe9a693022b8b3`): concurrent Phase 11 (Gcode, #2078) and Phase 14 (Administration, #2082) landed first, including the `AdminHomeAssistantController` scope collision described above. Fixed a bash syntax error (duplicated `if` line) left by manual conflict resolution in `scripts/ci/select-dotnet-tests.sh`, re-anchored the 4 security architecture guard tests as described above, removed a stale unresolvable `<see cref>` in `DevicesApiModule.cs`, and corrected stale "Home Assistant" wording in the module's `<Description>` and the CI selector's comments. - Third merge (head SHA `18649c087cd8930c790a231bec9b8514adcc39dd`, current head): concurrent Phase 16 (`Farm.Modules.Inventory`, #2081) landed. Conflicts confined to the same 3 files (`Program.cs` comment-only, `RouteTableSnapshot.txt` route-assembly reassignments, `select-dotnet-tests.sh` bucket wiring); again fixed a self-inflicted duplicate-`if`-line syntax error in the selector script during manual resolution, caught via `bash -n` and the selector's own 116-case unit suite before it was ever pushed. Re-validated after every merge: `dotnet build` clean (no new warnings), targeted test filters all passing, `Farm.Modules.Devices.Tests` 8/8 passing, `test-select-dotnet-tests.sh` 116/116 passing, `test-dotnet-test-manifest.sh` passing, `dotnet format --verify-no-changes` clean for every file this branch touches (pre-existing CHARSET/WHITESPACE errors elsewhere in the tree verified via `git log --follow` to predate this branch entirely). CodeQL: 0 open alerts on this PR's head (`gh api .../code-scanning/alerts?ref=refs/pull/2083/head` returns `[]`); CodeQL check itself passes. **Filed as a follow-up, explicitly out of scope for this PR:** #2088 — Phases 11 (Gcode) and 16 (Inventory) never added their modules to the 4 security architecture guard tests (`AuthorizeRolesGateArchitectureTests`, `RoleToPermissionMigrationCompletenessTests`, `PermissionGrantPathTests`, `QueueEnqueuePermissionArchitectureTests`), a pre-existing gap on `development` discovered during round-6 review of this PR's third sync merge, unrelated to the Devices extraction. ## Review Mandatory 3-way adversarial review completed and unanimous APPROVE across 6 rounds, most recently at head SHA `18649c087cd8930c790a231bec9b8514adcc39dd`: - Bishop (claude-opus-5) — found 2 blocking findings in round 1 (missing security-architecture-guard assembly entries; missing CI artifact upload step) and 4 blocking findings in round 4 (stale/unresolvable cref causing a new CS1574 warning; stale csproj Description; stale/truncated CI selector comments), all fixed. Round 6 (Inventory sync merge): re-verified the route-table merge mechanically (byte-for-byte diff against both parents) and the selector wiring; APPROVE. - Hicks (gpt-5.6-sol) — found 1 blocking finding in round 4 (missing Administration security-guard anchor in 2 of 4 files); fixed. Round 6: APPROVE, no new findings. - Vasquez (gemini-3.1-pro-preview) — found 1 blocking finding in round 4 (same missing Administration security-guard anchor, framed as a security-test-coverage regression); fixed. Round 6: initially raised the pre-existing Gcode/Inventory security-anchor gap (see #2088) as blocking; after verifying with evidence that the gap pre-dates this branch and originates from Phases 11/16's own PRs, agreed it was unrelated scope creep and switched to APPROVE. Verdicts posted as PR comments below in the canonical format, all naming head SHA `18649c087cd8930c790a231bec9b8514adcc39dd`. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c5b71999-3d38-4f5f-a016-e03da1b491b8
Summary
Extracts the parts-inventory / Spoolman vertical slice from
Farm.Web.Apiinto a newFarm.Modules.Inventoryproject (+.Tests), following the vertical-slice module-extraction pattern established by prior phases (seedocs/MODULE_MIGRATION_PATTERN.mdand Phase 13 #2076 for the most recent precedent).Closes #2044
What moved
SpoolmanController, ~1,497 LOC) +Infrastructure/PartsInventory/**(Spoolman, filament types, bins, parts, material clusters, tags, custom fields, collections, projects) intosrc/modules/Farm.Modules.Inventory/src/tests/Farm.Modules.Inventory.Tests//api/*contract change; no entity/EF migration changesParker gate checklist
Farm.Modules.Inventory.csproj+.Tests.csprojadded tosrc/farm-web.slnInternalsVisibleToentry added in the first commit, before any file movescripts/ci/dotnet-test-manifest.json,scripts/ci/select-dotnet-tests.sh)scripts/ci/tests/test-select-dotnet-tests.shextended (107/107 passing)scripts/ci/generate-codeql-slnf.sh— no edit needed; auto-discovers projects fromfarm-web.sln(verified locally:Farm.Modules.Inventory.csprojincluded,.Testsexcluded)scripts/docker/dockerfiles/Dockerfile.apiCOPY ordering updated.github/workflows/ci.yml— added the missing per-project "Upload Farm.Modules.Inventory.Tests build" artifact step (caught in review, see below)Validation performed
dotnet build ./farm-web.sln -c Debug→ 0 errors (pre-existing warnings only)dotnet testonFarm.Modules.Inventory.Tests→ 170 passeddotnet testfiltered toRouteTableSnapshotTests→ 1 passed (route table unchanged)dotnet format ./farm-web.sln --verify-no-changes→ failures are 100% pre-existing/unrelated (zero touch Inventory files)bash scripts/ci/tests/test-select-dotnet-tests.sh→ 107/107 passedorigin/developmentprior to review (already up to date)Review history
Commit
53e76181f(initial extraction) was reviewed in parallel by three independent models (Bishop/claude-opus-5, Hicks/gpt-5.6-sol, Vasquez/gemini-3.1-pro-preview). Bishop flagged one blocking finding:.github/workflows/ci.ymlwas missing the per-project "Upload Farm.Modules.Inventory.Tests build" artifact-upload step present for every other module (would break the dotnet-test job's download step once the inventory bucket runs). Fixed in3188aef2d, along with a non-blocking BOM cleanup inRouteTableSnapshot.txt. All three reviewers re-verified the incremental diff and unanimously APPROVE at head SHA3188aef2d5070aa6613afdb198d8f6f21480f34d.Non-goals
/api/*contract change