Asset Reconciliation Tags and Labels Review

Asset reconciliation, tags and labels: review notes (PR #164)

✅ Done - merged for 3.1.0 on 4 October 2026, with Sandy's four commits kept in the history. How the finished features work is in Asset reconciliation - Developer Guide and Asset tag numbering - Developer Guide; the labels are in QR asset labels - Developer Guide. What changed at merge time is below. This page is kept as the record of the review.

Written for Santhosh Srinivasan (Sandy), who built it, and for the AI coding assistant he works with. Each section follows the same shape - your code does X, the problem is Y, our approach is Z - and ends with a verdict: keep, change or leave out. The PR also asks 15 design questions; they are answered in their own section.

• Branch: srinivasansanthosh/freeitsm → feature/asset-lifecycle-enhancements
• Based on: 2.10.0 + 1 (0134fac2) - main is now 3.0.0
• Size: 4 commits, 29 files, +4,394 / -356
• How assets, QR labels and tags work today: QR asset labels - Developer Guide

---

Before anything else

This is a lot of careful work, and the problem it goes after is real. Hostname is not an identity: a laptop that gets renamed from DESKTOP-ABC1234 to LON-LT-042 turns up today as a brand-new asset, with none of its history. Fixing that properly is worth doing.

Things the branch does well, which should stay as they are:

• Reconciliation lives in the service layer. resolveAssetIdentity() decides, reconcileAsset() acts, and both the inventory agent and Intune call the same code. One set of rules instead of one per source is exactly right.
• Serial number is not made UNIQUE. The reasoning in the PR is the same reasoning the schema already uses for asset_tag (see the NULL trap ("The trap: per-company uniqueness cannot be a unique index")). Dirty discovery data is normal; a constraint that rejects it would break ingest.
• The placeholder-serial list. TO BE FILLED BY O.E.M. and friends are a real problem, and making the list editable is the right call.
• The tag sequence is genuinely safe under load. No MAX(asset_tag) + 1; the counter row is locked; the counter rolls back with a failed asset; a transaction the caller already opened is respected; GREATEST() stops an admin moving the counter backwards; numbers are never reused. All correct.
• The manual-tag lock checks its result and releases in finally. Many people forget one or the other.
• Labels now use publicBaseUrl(). The QR labels developer guide flagged the old messaging-only setting as something to clean up "next time settings are touched". This branch does it. 👍
• The logo upload reuses the safe-upload helpers (uploadStoreFile(), UPLOAD_TYPES_IMAGE, brandingPathIsSafe()), and deletes the old file only after the new settings are saved.
• The QR-token retry now only retries on a duplicate key, instead of swallowing every exception. Better than what was there.
• No existing English wording was changed - only new keys were added. That matters for the 20+ translations.
• The tests follow the house pattern for asset suites (CLI-only guard, a name prefix, sweep before and after) and even snapshot settings before touching them.
• The PR asks before locking things in. The design questions are the right questions.

Most of the changes below are about one rule: someone upgrading must see no difference until they choose to use the new options. FreeITSM has real installs - including MSPs running several client companies - and a change to how assets are matched is the kind that does damage quietly.

---

Don't worry about 3.0.0

The branch is based on 2.10.0; main is now 3.0.0. Please don't rebase or merge main into the branch - I'll do that when it comes in. Eight files overlap (db_verify.php, freeitsm.sql, db_verify_schema.php, db_verify_indexes.php, capabilities.php, asset-management/index.php, asset-management/settings/index.php and services/assets.php); none of them is hard.

One thing worth knowing for the rebase: includes/db_verify_indexes.php is now generated from freeitsm.sql by php scripts/gen_db_verify_indexes.php, and CI fails if it's edited by hand. Just add the index to freeitsm.sql and run the script.

---

Summary

#	Priority	Area	Verdict	In one line
1	🔴 Must	api/system/db_verify.php	Change	A stray } stops Database Verification parsing at all
2	🔴 Must	asset_history.analyst_id	Change	The NULL change never reaches an existing install, so automated history writes fail
3	🔴 Must	includes/intune.php	Change	Moved machines lose their Intune link and get a duplicate stub
4	🔴 Must	resolveAssetIdentity()	Change	A replacement laptop with the old one's name takes over the old record
5	🔴 Must	tests/*	Change	The tests delete real data by broad patterns
6	🟠 Should	Tag numbering	Change	Use the ticket-numbering format and counter, not prefix/padding/suffix
7	🟠 Should	Asset creation	Change	One place decides a new asset's tag; software inventory reconciles too
8	🟠 Should	Service layer	Change	ActorContext in, house helpers for audit, no raw exception text out
9	🟠 Should	Manual tags	Change	One rule for create and edit
10	🟠 Should	Permissions	Change	Labels get their own capability
11	🟢 Nice	Comments	Change	Put back the "why" comments that were removed
12	🟢 Nice	Translations	Change	t() has no fallback argument; label names are hard-coded English
13	🟢 Nice	docs/	Leave out	The documentation goes in the wiki
-	-	ActorContext::system(), publicBaseUrl() for labels, the label settings, the QR logo	Keep	See the sections above and Labels

---

1. Database Verification no longer parses

File: api/system/db_verify.php

Your code: near line 3782 the branch adds:

    }

The problem: that brace closes nothing, so the whole file is a parse error:

Parse error: Unmatched '}' in api/system/db_verify.php on line 3802

Database Verification is how every install gets new tables and columns, so on this branch nothing new would ever be created, on any install - including the branch's own asset_tag_sequences table.

Our approach: delete those three lines, then run php -l api/system/db_verify.php. Worth adding php -l on every changed file to the assistant's checklist (see the end).

Verdict: change.

---

2. asset_history.analyst_id - the NULL never reaches existing installs

Files: database/freeitsm.sql, includes/db_verify_schema.php, api/system/db_verify.php

Your code: changes asset_history.analyst_id from INT NOT NULL to INT NULL in both schema files, so that automated changes (an Intune rename, an agent report) can be recorded with no analyst. auditWrite() now passes NULL for an actor id of 0, and updateAssetHostname() writes NULL directly.

The problem: Database Verification adds missing columns; it does not change an existing column's definition. So on every install that already has asset_history - which is every install - the column stays NOT NULL. The first automated rename then fails:

SQLSTATE[23000]: Column 'analyst_id' cannot be null

and because updateAssetHostname() runs inside the agent's request, the system-info agent gets a 500 for every machine that has been renamed. Your tests pass because they run on a database built from the new freeitsm.sql.

This exact bug happened before, on ticket_audit (GitHub #120), which is why db_verify.php has a fixed shape for it.

Our approach: add a probe-then-MODIFY block next to the existing ticket_audit one (around line 224 of db_verify.php on main):

    // asset_history.analyst_id was NOT NULL, so an automated change - an Intune
    // rename, an agent report - had no way to be recorded (PR #164). Same
    // probe-then-MODIFY shape as ticket_audit above, and safe for the same
    // reason: every existing row has an analyst, so relaxing the rule cannot
    // invalidate one. The foreign key is unaffected - a NULL never violates it.
    try {
        $ahCol = $conn->prepare(
            "SELECT IS_NULLABLE FROM information_schema.columns
             WHERE table_schema = ? AND table_name = 'asset_history' AND column_name = 'analyst_id'"
        );
        $ahCol->execute([$dbName]);
        $ahRow = $ahCol->fetch(PDO::FETCH_ASSOC);
        if ($ahRow && strtoupper($ahRow['IS_NULLABLE']) === 'NO') {
            $conn->exec("ALTER TABLE `asset_history` MODIFY `analyst_id` INT NULL");
            $results[] = [
                'table'   => 'asset_history',
                'status'  => 'updated',
                'details' => ["analyst_id: NOT NULL → NULL (Intune and the inventory agent record changes no analyst made)"],
            ];
        }
    } catch (Exception $e) {
        // Non-fatal: asset history keeps working for people; automated entries stay broken.
    }

And please leave the foreign key exactly as it was (no ON DELETE SET NULL). Changing it in freeitsm.sql only changes fresh installs, so fresh and upgraded installs would quietly differ. ticket_audit kept its foreign key for the same reason.

The readers are already fine: api/assets/get_asset_history.php and the REST API both LEFT JOIN analysts, so a row with no analyst still shows.

Verdict: change (the idea is right; it needs the upgrade path).

---

3. Intune: a machine moved to another company loses its link

Files: includes/intune.php, asset-management/settings/index.php, asset-management/settings/manifest.php

Your code: adds an intune_company_id setting, and scopes the whole Intune link step to that one company. In step 1, every device that already has an asset_id is re-checked with isExplicitLink = true; if the linked asset is not in the configured company, the link is treated as stale - re-pointed to a match in the configured company, or cleared. Your Test J checks exactly this.

The problem: this is how MSPs use FreeITSM today, and it's written down in Multi-Tenancy - Developer Guide:

A single shared connection that pulls rows for several companies (a shared vCenter/Intune) can't derive one company from its credential ... Until then, such rows land in the Default company as "needs assigning."

So the workflow is: Intune creates the stub in Default, and an analyst moves it to the client company it belongs to (Moving an asset between companies). On this branch, with intune_company_id unset (which is every install after upgrade), the next sync sees that moved asset as "in the wrong company":

1. the Intune link is cleared,
2. step 2 finds no match in Default, and
3. a new stub is created in Default - a duplicate, which is exactly what this PR sets out to stop.

It repeats for every moved machine on the first sync after upgrade.

The underlying point: an existing intune_devices.asset_id was made by a person or by an earlier sync, and a person may since have moved the asset on purpose. Its company is not evidence that the link is wrong.

Our approach:

• Trust an existing link whatever company the asset is now in. Keep tier 1 as "the link is authoritative", and only drop a link when the asset no longer exists (the foreign key already does that).
• Use the company setting only for new decisions - which company a new stub goes in, and which company's assets a new serial/hostname match is looked for in. That keeps your isolation where it matters (an unlinked device can never grab another company's asset) without undoing an analyst's move.
• Unset means today's behaviour: new stubs in Default, as now.
• The rename check (updateAssetHostname()) already uses the asset's own company for the collision guard, which is correct and works for a moved asset.

A smaller point in the same function: step 1 now calls reconcileAsset() for every linked device on every sync - several queries each. A 5,000-device tenant does tens of thousands of queries to find the handful that were renamed. One query finds those directly:

SELECT d.id, d.asset_id, d.device_name
  FROM intune_devices d
  JOIN assets a ON a.id = d.asset_id
 WHERE d.device_name IS NOT NULL AND d.device_name <> ''
   AND LOWER(LEFT(d.device_name, 50)) <> LOWER(COALESCE(a.hostname, ''))

Verdict: change. Keep the company picker for new stubs; drop the "stale link" rule.

---

4. A replacement laptop with the old name takes over the old record

File: includes/services/assets.php (resolveAssetIdentity())

Your code: tier 2 looks the serial up. If it finds no asset with that serial, it falls through to tier 3 and matches on hostname.

The problem: the commonest hardware event on a service desk is a laptop refresh - and very often the new laptop is given the old one's name (LON-LT-042). The new machine reports a good serial nobody has seen, tier 2 finds nothing, tier 3 finds LON-LT-042... and the system-info update then writes the new serial over the old asset. The old laptop's record - its purchase date, warranty, handover history - now describes a different physical machine, and the old laptop (sitting in a cupboard for reuse) has no record at all.

This is the duplicate problem in reverse: one record for two machines, which is harder to spot and harder to untangle.

Our approach: a hostname match is only safe when it can't contradict the serial. If the incoming device has a usable serial and the hostname match already has a different usable serial, it is a different machine:

        // Tier 3: hostname - unless a usable serial already contradicts it.
        // A refreshed laptop is very often given the old one's name. If both
        // machines have real, different serials, the name is the only thing
        // they share, and matching on it would put the new serial on the old
        // laptop's record. That is a new asset.
        if ($hostname !== '') {
            $stmt = $conn->prepare("SELECT id, service_tag FROM assets WHERE hostname = ? AND tenant_id <=> ? LIMIT 1");
            $stmt->execute([$hostname, $tenantId]);
            $hit = $stmt->fetch(PDO::FETCH_ASSOC);
            if ($hit) {
                $theirs = (string)($hit['service_tag'] ?? '');
                $contradicts = $serviceTag !== '' && self::isUsableServiceTag($serviceTag, $ignoredTags)
                            && self::isUsableServiceTag($theirs, $ignoredTags)
                            && strcasecmp($theirs, $serviceTag) !== 0;
                if (!$contradicts) {
                    return ['asset_id' => (int)$hit['id'], 'matched_by' => 'hostname', 'ambiguous' => false];
                }
                return ['asset_id' => null, 'matched_by' => 'none', 'ambiguous' => false, 'hostname_reused' => true];
            }
        }

That leaves the important case working: an asset that was entered without a serial (by hand, or before the agent ran) still links by hostname, and the agent fills the serial in.

The new asset then needs a hostname that doesn't clash with the old record (hostname is unique per company). The simplest safe choice is to create it and flag it - hostname_reused => true in the result, a line in the agent's response, and an entry in the asset's history - so an analyst renames or retires the old record. Please add a test for it next to Test E.

Verdict: change.

---

5. The tests delete real data

Files: tests/asset-reconciliation.php, tests/asset-labels.php, tests/asset-tag-autogen.php

Your code: the suites use the house prefix-and-sweep pattern, which is right for asset tests (Developer Tests - Assets explains why: the asset services open their own transactions). But some of the sweeps are much wider than the rows the tests create:

$conn->exec("DELETE FROM assets WHERE hostname LIKE 'GAL-LTW-%' OR hostname LIKE 'ZZ-%' OR hostname LIKE 'COLLIDE-%'
             OR hostname LIKE 'DEFCO-%' OR hostname LIKE 'COMP%' OR hostname LIKE 'CLIENTX-%'");
$conn->exec("DELETE FROM tenants WHERE name LIKE 'ZZ-Tenant-%' OR name LIKE 'MSP-%'");

and tests/asset-labels.php deletes the install's real public_base_url and messaging_public_base_url settings, and every asset_label_* setting, before putting them back in finally.

The problem:

• hostname LIKE 'COMP%' matches COMPUTER01, COMPTA-PC, COMP-LT-07... on a real install, along with their history. name LIKE 'MSP-%' would delete a real company.
• The settings restore runs in finally, but a PHP fatal error skips finally. One crash mid-run and the install is left with no public address - every link in every email it sends from then on is broken, with no error anywhere.
• People run these suites on their own install. A test that can delete a real company is a test nobody can safely run.

Our approach:

• One prefix per suite, starting ZZ, and only that prefix in the sweeps - the table on Developer Tests - Assets lists the ones in use (ZZPD, ZZDH, zzimp...). For example ZZREC- for reconciliation, ZZTAG- (already used) and ZZLBL-. Test companies get the prefix too, and are deleted by the ids the test created, never by name.
• Don't change live settings to test a setting. TicketNumbering::withSettings() / forget() is the house pattern: the class takes an override for tests and previews, so nothing real is written. Give assetLabelSettings() and AssetTagsService the same.
• Your work makes the next step possible. Because createAsset() and generateNextAssetTag() now respect a transaction the caller already opened, most of the tag and reconciliation tests can run inside one transaction that is always rolled back - the pattern the newer suites use (see tests/people-scope.php). Only the rollback and lock tests need real commits.
• Production code shouldn't grow test switches. publicBaseUrl(PDO $conn, bool $resetCache = false) and tenantSetting(..., bool $clearCache = false) put a test concern into functions called on every request. Add a separate publicBaseUrlForget() / tenantSettingForget() instead, like TicketNumbering::forget().
• Add the three suites to the table on Developer Tests - Assets (I'll do that at merge).

Verdict: change.

---

6. Tag numbering: use the shape ticket numbering already has

Files: includes/services/asset_tags.php, asset_tag_sequences, the Asset Tags settings tab

Your code: five settings - prefix, suffix, padding, initial number, enabled - and a table keyed by tenant_id (0 for Default), locked with SELECT ... FOR UPDATE, skipping forward one number at a time (up to 1,000) past tags already in use.

The problem: none of it is wrong. But 3.0.0 shipped ticket numbering you choose the shape of (includes/ticket_numbering.php), and it solved the same problem in a way that also answers several of your design questions:

	This branch	Ticket numbering (3.0.0)
Format	prefix + padded number + suffix	one format string: INC-{YYYY}-{#####}
Rule-based parts (your Q10)	not possible without new settings	tokens: {COMPANY}, {TYPE}, {YYYY}, {MM}
Scope (your Q5)	per company, fixed	global / per_type / per_company setting
Counter	row per tenant, FOR UPDATE	counter_key + atomic upsert (LAST_INSERT_ID(next_value + 1))
Past a collision	+1, up to 1,000 tries	jumps, doubling the stride - a counter far behind is cleared in a few tries
Padding	1-12	{###} is a minimum width, never a limit

AST-{#####} is your prefix/padding/suffix in one setting, and the day someone asks for {COMPANY}-LT-{####} it simply works - no new settings and no migration of the old five. An admin who has already learnt ticket numbering recognises the screen.

Our approach:

• One setting, asset_tag_format, default AST-{#####}, plus asset_tag_start and a scope (per_company by default, as you have it).
• Counters in a table shaped like ticket_number_counters (counter_key such as asset:co12), claimed with the same upsert, wound forward with the same GREATEST() - which you already use in setNextSequenceNumber().
• Keep your rollback with the asset: the upsert inside the asset's transaction rolls back with it, exactly as your lock does now.
• If it helps, pull TicketNumbering::render()'s token handling out into a small shared function rather than copying it.

Verdict: change - your behaviour, ticket numbering's shape.

---

7. One place decides a new asset's tag

Files: includes/services/assets.php, includes/intune.php, api/external/system-info/submit/index.php, api/external/software-inventory/submit/index.php

Your code: "if auto-generation is on, generate a tag, then INSERT" is written four times - createAsset(), the system-info agent, the software-inventory agent and Intune - each with its own transaction handling.

The problem: four copies drift. The PR description says the reconciliation logic is centralised "so each source isn't implementing its own rules", and that's the right instinct - it just needs applying to creation too. And one source was missed: the software-inventory agent still matches on hostname only. If it reports before the system-info agent after a rename, it creates the duplicate this PR is meant to prevent.

Our approach:

• An AssetsService::createDiscoveredAsset(PDO $conn, ActorContext $ctx, array $fields, string $source) that owns the transaction, the tag and the audit row. The three intake paths call it instead of writing their own INSERT.
• The software-inventory agent calls reconcileAsset() too. It only sends a hostname, so it lands on tier 3, but it then follows the same rules (including section 4).

Verdict: change.

---

8. Service-layer conventions

File: includes/services/assets.php, includes/services/asset_tags.php, includes/service_context.php

Four small things, all about matching the rest of includes/services/ (Service Layer Architecture):

Your code	The house shape
reconcileAsset(PDO $conn, array $ids, ?int $tenantId, ...)	Service methods take ActorContext $ctx second, and the company comes from it or is checked against $ctx->companyScope. The agent paths build one with your new ActorContext::system().
updateAssetHostname() writes INSERT INTO asset_history ... itself	Use self::auditWrite(), so there's one place that writes asset history.
throw new ServiceError(..., 'Failed to generate asset tag: ' . $e->getMessage())	Don't pass a raw database message out to the caller; log it with error_log() and throw a plain one.
ActorContext::system() is inserted between fromApiKey()'s doc comment and the method	Move it above that doc comment, so each comment sits on its own method.

ActorContext::system() itself is a good addition and should stay - there wasn't one, and one place builds it by hand today (includes/domains/status_link.php).

Verdict: change (small).

---

9. Manual tags: one rule for create and edit

Files: includes/services/assets.php, api/assets/save_asset_tag.php

Your code: a typed-in tag on create is checked under a GET_LOCK named per company.

The problem: a tag can also be set or changed on an existing asset (api/assets/save_asset_tag.php, the tag box on the asset page), and that path checks without the lock. Generated tags don't take the lock either, so a generated AST-00005 and a typed AST-00005 can still both get through. The existing endpoint also names the clash ("That tag is already on LAPTOP-12"), where the new create path says only "already in use".

Our approach: one service method - for example AssetTagsService::assign(PDO $conn, ActorContext $ctx, int $assetId, string $tag) - takes the per-company lock, checks, writes and audits. Create, edit and generation all go through it. Keep the existing message that names the other asset.

Verdict: change.

---

10. Labels need their own capability

Files: includes/capabilities.php, asset-management/settings/manifest.php, api/assets/*label*

Your code: the Asset Tags tab and the Asset Labels tab both use Cap::ASSETS_TAGS.

The problem: capabilities are built from the manifests and keyed by the constant, and the first tab to claim one wins (capRegistry() skips a key it has already seen). So System → Roles shows one permission, "Configure asset tag auto-generation and numbering sequence", that silently also controls the label designer and its logo upload. They are different jobs - the person who designs a sticker is not necessarily the person allowed to change how every future asset is numbered.

Our approach: add const ASSETS_LABELS = 'assets.labels';, give the labels tab and its two endpoints that, and keep ASSETS_TAGS for numbering. ASSETS_RECONCILIATION is fine as it is.

Verdict: change.

---

11. Put back the "why" comments

Files: includes/asset_labels.php, includes/intune.php, includes/services/assets.php

Your code: alongside the real changes, several explanatory comments were removed:

Where	What the removed comment explained
assetEnsureToken()	why a QR token is minted on first print, not when the asset is created
assetIdForToken()	why the token's shape is checked before the database is asked
assetTagAvailable()	the NULL trap - why per-company uniqueness can't be a unique index
intuneLinkDevicesToAssets()	the multi-tenancy decision, and why names are truncated to 50
parseDate()	its doc comment

The problem: in this codebase those comments are load-bearing. They're the reason the next person - or the next AI assistant - doesn't "simplify" assetTagAvailable() into a unique index that silently does nothing. Removing them is how a fixed bug comes back.

Our approach: restore them, and update a comment where the behaviour has changed rather than deleting it (the Intune one becomes "an existing link is trusted; the company setting decides where new stubs go"). Also fix the doubled /** at the top of the intuneLinkDevicesToAssets() doc comment.

Verdict: change.

---

12. Translations

Files: asset-management/labels.php, includes/asset_labels.php, asset-management/settings/index.php, lang/en/asset-management.php

Your code: t('asset-management.labels.no_ids_heading', 'No Assets Selected').

The problem: t()'s second argument is placeholders (['n' => 3]), not fallback text - FreeITSM falls back to English by itself. A string there is quietly ignored, and eight of the keys used this way aren't in lang/en/asset-management.php, so the page would show the raw key, asset-management.labels.no_ids_heading. Separately, assetLabelStandardFields() and assetLabelPrintFields() hard-code 'Asset Tag', 'Serial' and so on, and the settings script has two English strings ('No logo uploaded', and the sample tag).

Our approach: add the eight keys to lang/en/asset-management.php and drop the second argument; give the field names keys (for example asset-management.labels.field.service_tag) and look them up with t(). New English keys are always fine - I translate them into the other languages in batches.

Verdict: change.

---

13. docs/asset-reconciliation.md

Your question: should the documentation be in the PR?

Our approach: thank you for writing it - the content is good. FreeITSM keeps user and developer documentation in this wiki, and docs/ holds only a few setup notes and design plans, so a file there would be read by nobody and drift. Please leave it out of the PR; I'll fold the content into the wiki pages below at merge time.

Verdict: leave out (the content moves to the wiki).

---

Labels and QR: what stays

The label work is in good shape and needs only the translation fixes above and the separate capability in section 10:

• Choosing and ordering fields (including custom fields), header, footer, field labels on or off - keep.
• The label logo, with the QR switched to error-correction level H and the logo held to about 22% on a white backing - keep. Level H recovers about 30% of the code, so a 22% logo leaves margin for a scuffed sticker. Please print one at the smallest size (65 per sheet, a 16 mm code) and scan it with an ordinary phone before it's final; that's the case most likely to fail.
• The asset tag is always printed - keep. It's what a person reads off the device.
• The four sheet layouts and the opaque QR token are unchanged - keep (see Q13).

---

Answers to the 15 questions

Reconciliation

1. Is "connector relationship → serial → hostname → new asset" the right hierarchy? Yes, with one condition: a hostname match must never contradict a good serial (section 4). And an existing connector link stays trusted after a person moves the asset to another company (section 3).

2. Should serial numbers stay reconciliation attributes rather than unique keys? Yes. It's the same reasoning FreeITSM already uses for asset_tag and hostname: a unique index can't hold for the Default company because of how MySQL treats NULL, and real discovery data has blanks, placeholders and genuine duplicates. Keep your ambiguity guard.

3. Are there other authoritative identifiers? Yes, two worth designing for later (not in this PR):

• The SMBIOS / hardware UUID. For virtual machines it's better than the serial - cloned VMs and some hypervisors report the same or an empty serial, but each VM has its own UUID. The inventory agent could send it, and it would slot in between tier 1 and tier 2.
• Each connector's own device id - Intune's device id (already tier 1 through intune_devices.asset_id), vCenter's instance UUID, Entra's device id. The pattern you used for Intune is the right one for each.

MAC addresses are not worth using: docks, USB adapters and Wi-Fi randomisation make them move between machines.

4. Should hostname remain the final fallback? Yes - an asset entered by hand usually has only a name, and that's how the agent finds it the first time. But only when it doesn't contradict a good serial (section 4). There's no need to switch hostname matching off per source.

Asset tags

5. Company-scoped sequences, or global / configurable? Company-scoped by default - asset tags are already unique per company, and MSP clients usually want their own numbering. Make the scope a setting, the way ticket numbering does (global / per_company), and it costs almost nothing (section 6).

6. Optional, or enforceable? Optional, and off after an upgrade, as you have it - an upgrade must change nothing until someone chooses to. "Enforce" isn't needed yet: with auto-generation on, every new asset already gets a tag unless someone types one.

7. Manual tags when auto-generation is on? Yes, allow them. Organisations arrive with stickers already on their equipment, and migrations bring old numbers. A typed tag is checked for uniqueness in the company like any other (section 9). One small touch worth adding: if a typed tag has the generated shape and a number ahead of the counter, wind the counter past it (GREATEST()), so the generator never has to skip over it later.

8. Should a tag be immutable once assigned? No. In FreeITSM the QR token is the asset's permanent physical identity - that's why it's opaque and separate from the tag (QR labels developer guide). A printed label keeps scanning to the right asset even if its tag changes. Tags should stay editable, with every change in the asset's history - which save_asset_tag.php already records.

9. Is bulk renumbering needed? Not now. Ticket numbering has renumbering because people migrate from other systems and want old references tidied. Asset tags are on physical stickers, so renumbering means re-labelling every device - it's rarely wanted and expensive when it is. Wait until someone asks.

10. Rule-based prefixes (by type, company, location)? Design for it now by using a format string with tokens instead of prefix/suffix (section 6) - {COMPANY} comes free. Don't build per-type or per-location rules yet. The format string means they can be added later without a migration.

Physical labels

11. Is choosing and ordering fields enough? Yes.

12. Is a full label designer needed? No. Your reasons in the PR are the right ones: physical dimensions, browser print scaling and long values make a designer a project of its own. Nobody has asked for one.

QR

13. Keep the opaque token? Yes. It's what lets the tag, hostname, user and location all change while every printed label still works, and it can't be guessed the way a database id can.

Testing and documentation

14. Is the test coverage and placement right? The coverage is good and tests/ is the right place. The changes are in section 5: narrow ZZ prefixes, no live settings changed, a rolled-back transaction wherever the code allows it, and no test switches in production functions.

15. Should the documentation be in the PR? Leave it out - the wiki is where FreeITSM's documentation lives, and I'll move the content there (section 13).

---

Checks before sending it back

For the coding assistant - run these first:

# 1. Every changed PHP file parses (this would have caught section 1)
git diff --name-only 0134fac2 -- '*.php' | xargs -n1 php -l

# 2. No sweep wider than its own prefix
git grep -n "DELETE FROM" -- tests/asset-reconciliation.php tests/asset-labels.php tests/asset-tag-autogen.php
#    every LIKE should be a ZZ... prefix this suite creates; no DELETE FROM tenants by name

# 3. No live install settings written by a test
git grep -n "system_settings" -- tests/asset-*.php

# 4. One place creates a discovered asset
git grep -n "INSERT INTO assets" -- includes/intune.php api/external/   # expect none

# 5. No string fallback passed to t()
git grep -n "t('[^']*', '" -- asset-management/labels.php   # expect none

And by hand:

1. Upgrade test: start from a 3.0.0 database, run Database Verification, and check asset_history.analyst_id is now nullable. Then rename a machine and let the agent report - it should update, not 500.
2. The MSP case: let Intune create a stub in Default, move it to another company, run a sync. The link should stay and no new stub should appear.
3. The laptop refresh: an asset ZZREC-LT-042 with serial AAA; the agent reports ZZREC-LT-042 with serial BBB. The old record must keep AAA.
4. Auto-generation off: after upgrade, a new asset gets no tag, exactly as before.

---

Wiki pages that change when this is merged

I'll update these at merge time, so they describe the code as merged rather than as reviewed:

Page	What changes
QR asset labels - Developer Guide	§3: labels now use publicBaseUrl() (the flagged clean-up is done). §6: configurable fields, header/footer, logo and error-correction H. New section: tag generation.
QR asset labels	Label settings and automatic tags, for the person setting them up
Multi-Tenancy - Developer Guide	The shared-Intune paragraph: new stubs can go to a chosen company; existing links survive a move
Moving an asset between companies - Developer Guide	"agent ingest matches on hostname" becomes serial, then hostname
Inventory agent	Renamed machines are recognised by serial; placeholder serials are ignored
Service Layer Architecture	ActorContext::system() for automated callers
Developer Tests - Assets	The three new suites and their prefixes
REST API: Assets	A created asset can come back with a generated asset_tag
A new Asset Reconciliation - Developer Guide	The tiers, the ambiguity guard, the placeholder list, the laptop-refresh rule - built from docs/asset-reconciliation.md

---

What happens next

Push the changes to the same branch. I'll bring it up to date with 3.0.0 and handle the changelog, translations and release notes - none of that is expected from a contributor.

If you disagree with any of these - especially sections 3, 4 and 6, which are judgement calls - say so on the PR. I'd rather hear the argument than have it quietly changed.

Thank you again, Sandy.

What changed at merge time

Every section above was worked through when the branch was merged. Where the merged code differs from what's written above, this table says so.

#	Done as described	Worth knowing
1	✅
2	✅	Automated history writes are also best-effort until Database Verification has run, so an agent report never fails over its own audit row.
3	✅	Unset, Intune matches across all companies, as before. Renaming assets from Intune is a setting (intune_sync_hostnames), off after an upgrade: on the development install 65 of 582 linked devices would otherwise have been renamed at the first sync.
4	✅	The ambiguity guard was narrowed too: two assets sharing a serial are only told apart by serial and name together, not by name alone.
5	✅	The suites were rewritten: reconciliation and tags run in a rolled-back transaction (the services now join one), labels only reads. tenantSettingForget() and publicBaseUrlForget() replace the test switches.
6	✅	TicketNumbering::render() is used directly rather than copied. The first retry after a taken number lands on the very next one, so a run of stickers has no needless gap.
7	✅	createDiscoveredAsset() writes asset_discovered, not asset_created: Database Verification's last_seen repair treats asset_created as proof a person made the record, and would otherwise blank last_seen on every newly discovered machine.
8-13	✅

---

Thank you, Sandy - recognising a renamed computer by its serial number is one of those things every asset register should do and few do well, and FreeITSM now does.