REST Inventory and Evidence PR Review

REST custom fields, change evidence and inventory: review notes (PRs #195, #196, #197)

✅ Done - all three merged for 3.12.0 on 20 October 2026. Each PR went in as its own commit under @Dr4inder's name, followed by a review commit of mine. How the finished features work: custom asset fields, change evidence, inventory reports, and the design with its as-built notes. This page is the record of the review.

Written for @Dr4inder, who built all three, and for anyone - or any AI coding assistant - contributing to FreeITSM next. Each finding has the same shape: your code does X, the problem is Y, the approach now is Z, then a verdict: keep, change or leave out. The lessons that keep coming back across reviews are collected in Contributing.

PR	What	Verdict	Commits
#195	Custom asset fields on POST / PATCH /assets	Merge with fixes (small)	f0e17d3 + 66f6cc5
#196	Evidence attachments on a change	Merge with fixes (small)	370a6a3 + 31221a9
#197	Source-aware inventory import	Merge with fixes (substantial rework)	506b8d6 + e4ba975

---

Before anything else

All three were good pull requests, and much better than average. They came with the design already agreed, each part small enough to review on its own, and with tests. Things that should stay exactly as they are, and are worth copying:

• One write path. #195 sends custom fields through AssetFieldsService, the path the UI and the spreadsheet importer use. #196 moved the upload out of the UI endpoint into ChangesService::uploadAttachment(), and the UI now calls it too. Nothing was written twice.
• Atomic, and honest about it. Core and custom fields commit together (#195), and a failed upload leaves no file behind and no row (#196). A whole report or nothing (#197). The transaction handling is careful: each service owns its transaction and refuses to run inside someone else's.
• Authorisation before anything is written. #196 refuses a read-only analyst and an unknown change before the file reaches the disk.
• Narrow permissions. change_attachments and asset_inventory.create let an integration do one thing. An evidence key cannot edit, approve or delete a change.
• Retries that check content. Reusing an upload reference with a different file is a 409 (#196), and so is a different section at the same observation time (#197). A retry is never mistaken for a match.
• Path traversal handled properly. The download resolves realpath() and checks the result is inside the attachments folder, and the test proves it by pointing a row at ../../config.php.
• The latent software collision found and fixed. The table's unique key (device, application) could not hold the same application from two sources, despite the code assuming it could. #197 widens it to (device, application, source) with an upgrade that keeps every row - 40,146 on the install it was tested on.
• Every test file is guarded (CLI only, then 404), so nothing can be run from a browser.

---

PR #195 - custom asset fields

#	Finding	Verdict
1	Unknown top-level keys still answered 200	Change
2	The warranty calendar sync ran inside the transaction	Change
3	No positive test for a type change	Change
4	The test cleaned up by hostname	Change

1. Unknown keys. Your code validates everything inside fields, as agreed. The problem: the agreed answer to decision 1 was also to name the top-level keys a write does not use, and those still came back as a plain 200, so a typo looked like a saved change. Now: AssetsService::ignoredKeys() lists them and the response carries "ignored": ["colour"]. They are still accepted, so older scripts keep working.

2. Warranty sync. Your code moved syncWarranty() inside the update's transaction. The problem: it re-syncs the warranty calendar for the whole estate and swallows its own errors. Inside the transaction it holds the asset's locks for all of that, and a half-done sync commits with the asset. Now: it runs after the commit, as createAsset() already did.

3. Type change. The tests proved that a field of the wrong type is refused. They did not prove the case the design promised: changing the type and setting a field of the new type in the same request. Now: there is a test for exactly that, plus a check that the old type's field is then refused.

4. Cleanup. The test deleted its assets by hostname. The problem: on an install whose hostname column is narrower than the schema says (it happens - Database Verification adds missing columns but never widens them), MySQL quietly shortens the name. The cleanup found nothing, and the rows were left in a real database. Now: it records every id it creates and deletes by id. Clean up by what you created, never by what you think it is called.

---

PR #196 - change evidence

#	Finding	Verdict
1	external_reference where the design says client_reference	Change
2	No GET ?client_reference=	Change
3	The test fired real workflows	Change
4	Per-analyst uniqueness of the reference	Keep

1. The name. Your code called the retry key external_reference. The problem: the agreed design, and the task-form submissions built the same week (#190), call it client_reference. Two names for one idea in one API is something every integrator would trip over. Now: client_reference everywhere - the column, the parameter, the response and the spec. It was free to change, because nothing had been released.

2. "Did it arrive?" The design promised GET /changes/{id}/attachments?client_reference=…, so a pipeline whose upload timed out can ask before sending again. Now: it exists. The answer is the match or an empty list, never an error.

3. The test. Your test created its change with ChangesService::createChange(). The problem: that fires the real change.created workflows. On a development database that is a copy of somebody's live data, those workflows send real emails and open real tickets. Now: the test inserts the change row directly. A test never goes through a path that notifies, emails or runs automation, unless that is what it tests.

4. Kept: uniqueness per change and per analyst is stricter than the design asked for, and better. Two integrations can never collide on a reference.

Also tidied: the three operations sit in the spec's Change comments, history & CAB section beside their neighbours, and the test helper passes query strings through.

---

PR #197 - inventory import

This was the big one, and the most carefully built: versioned input, strict validation, a per-section watermark and a content fingerprint. It was also where the contract and the existing code needed the most alignment, as @Dr4inder said it would.

#	Finding	Verdict
1	🔴 The agent stopped replacing Intune's stand-in volumes	Change
2	🔴 Ownership per asset let one integration clear another's rows	Change
3	🔴 One bad section refused the whole report	Change
4	Delete-and-reinsert on every report	Change
5	Field names differ from the agreed design	Change
6	No way to unlink a device	Change
7	New applications skipped the workflow trigger	Change
8	Analyst columns missing from the leaver map	Change
9	The upgrade test altered real tables	Change
10	22 hand-written translations	Leave out
11	db_verify_indexes.php edited by hand	Change
12	Fingerprint instead of a 30-day report_id	Keep
13	The rest: namespace for stored sources	Keep
-	Physical drives as a section; a sources panel on the asset page	Not yet - follow-ups

1. 🔴 Agent regression. Your code narrowed the agent's volume delete to source = 'agent', so it would leave REST rows alone. That intent is right. The problem: the agent used to replace Intune's volumes too, and they rely on that. Intune only writes a stand-in volume for machines the agent does not report (intuneSyncDisks() skips any asset with agent rows). With the narrowed delete, a machine Intune saw first would show its Intune volume beside the agent's for ever. Now: source IN ('agent', 'intune'), with a TRAP: comment, and a new check in tests/agent-empty-report-guard.php. That check fails on the PR's version and passes on the fix. When you narrow a delete, find every writer of that table first and ask who relied on the wider one.

2. 🔴 Ownership. Your code bound each (asset, source) to the analyst who first wrote it. The problem: the binding was per asset. A second integration that sent the same source name could not touch assets the first had reached, but on every asset the first had not reached yet, it became the owner. It could then send a complete empty section and clear that source's rows as they arrived. That is exactly what @Dr4inder's own clarification 1 asked to prevent. Now: a source name belongs to one analyst per company, in asset_inventory_source_owners. A rotated key acts as the same analyst, so it keeps the source; anyone else gets 403 source_owner. Built-in names (agent, intune, ...) are refused outright.

3. 🔴 Partial success. Your code refused the whole report if any section was invalid or conflicted. The problem: the agreed contract (design, and @Dr4inder's clarification 3) was the reverse. The good sections apply, the failed ones are named, and a retry must not be blocked by its first failure. Now: each section runs inside its own savepoint. A failure rolls back that section alone and records no watermark, so sending the same report again retries only the failed sections, and the applied ones answer replayed. 200 when nothing failed, 207 when some did, 422 all_sections_failed when none applied. Problems with the report as a whole (the envelope, ownership, a device linked elsewhere) still refuse all of it.

4. Reconciliation. Your code deleted the source's rows and inserted the new list each time. The problem: every row got a new id on every report, and the response could not say what had changed. Now: rows are matched by identity (the application, or your external_id) and updated in place, new ones are added, and - for a complete section only - the missing ones are removed. The response gives added, updated and removed counts per section.

5. Names. source_id / external_device_id became source / source_device_ref, as in the design. source_id also reads like a foreign key. Item fields keep the tables' own names (display_name, mac_address), which the read endpoints already use - a sensible choice, kept.

6. Unlinking. The design made re-linking a deliberate act: DELETE /assets/{id}/inventory/{source} first. The PR had no way to do it, so a reimaged machine's device reference was stuck on its old asset. Now: the endpoint exists, behind asset_inventory.delete, and it removes that source's rows from the asset and writes to the history. Making it work meant one small change to the REST router: path parameters that are not numbers now arrive as strings. Every route before this one used ids only.

7. Workflow trigger. Your code added unseen applications to the catalogue, as the agent and Intune do. The problem: they also fire software.application_discovered, and people build workflows on it. Now: it fires, after the commit, and only for applications whose section actually applied.

8. Leaver map. Every column that names an analyst must be in analystReferenceRegistry(), or deleting a test analyst fails. Now: source owners are setup (whoever runs that integration now has to take the name over) and section importers are history. The same check showed nine columns from my own work this month that were missing as well. They are fixed in the same commit (changelog #2489).

9. The upgrade test. Its header said no persistent data was modified, and the software table was indeed shadowed by a temporary copy. The problem: the upgrade function also widens two real tables and adds two foreign keys, and nothing shadows those. Now: the test refuses to run until Database Verification has done the upgrade, so on the real tables it can only be a no-op.

10. Translations - leave out. New strings go into lang/en only. The other locales are translated in one pass before each release, checked by native-speaker rules and a glossary. Hand-written entries get in the way of that.

11. Generated file. includes/db_verify_indexes.php is generated from freeitsm.sql by php scripts/gen_db_verify_indexes.php, and CI fails when the two disagree. Change the SQL and run the script.

12. Kept: the fingerprint. The design asked for a report_id remembered for 30 days. Your observation time plus a content fingerprint, per section, does the same job and never expires. That answers @Dr4inder's worry about replay protection ending with a cache. The design page now says so.

13. Kept: rest:. Storing REST rows as rest:<name> means they can never be mistaken for the agent's or Intune's, whatever name is sent.

Not one implementation - yet. The design wanted the agent and the API to share one inventory implementation. The REST path shares the catalogue rules and the trigger, but the agent's own code is unchanged apart from fix 1. Moving the busiest write path in FreeITSM onto new code deserves a release of its own.

---

What changed at merge time

All of it is in the three review commits listed at the top. Tests at merge:

Suite	Result
tests/asset-api-fields.php	28 passed
tests/change-api-attachments.php	21 passed, plus a manual check that the app's own upload still works through the shared service
tests/asset-api-inventory.php	34 passed (rewritten for the contract above)
tests/asset-inventory-upgrade.php	5 passed
tests/agent-empty-report-guard.php	14 passed, with a control that fails on the PR's agent change
tests/analyst-lifecycle.php	21 passed
api/v1/lib/openapi_check.php, schema drift, web-exposure guards	pass

Related pages

• Contributing - the lessons from this and earlier reviews, in one place
• API inventory and evidence - design
• Earlier reviews: Asset reconciliation, tags and labels (#164) · SSO JIT roles and profile sync (#155) · Checklists closure gating