Pull requests are welcome, and several of FreeITSM's features started as one. This page is the checklist I review against. Every point on it is here because a real pull request (or my own code) got it wrong, so it is worth a read before you start - and worth giving to your AI coding assistant.

Each review is written up in full, so you can see the reasoning: Ticket workspace, task editor and ticket table (#211-#213) · Long forms in a task's Work view (#198) · REST custom fields, evidence and inventory (#195-#197) · Asset reconciliation, tags and labels (#164) · SSO JIT roles and profile sync (#155) · Checklists closure gating.

Every pull request gets one of three verdicts:

• Merge - it goes in as it is.
• Merge with fixes - your commit goes in under your name, and my fixes follow in a commit of my own. A review page like the ones above says what changed and why.
• Rewrite - the idea is right but the code would be risky to adapt. I write it myself, credit you, and close the PR.

Nothing is merged without being read line by line and run. That includes tests, which run against a real database.

1. Agree the design first. For anything bigger than a fix, open an issue. I write a design on the wiki, and the code follows it - including the names of fields, parameters and permissions. If the code needs to differ, say so in the PR so the design can be updated.
2. Base it on current main, and keep each PR to one feature. Three small PRs are far easier to review than one big one.
3. Upgrades must be invisible. Somebody upgrading should see no change in behaviour until they choose a new option. Real installs - including MSPs running many client companies - depend on this.

• Nothing runs from a URL unless it is meant to. Every file in tests/ and scripts/ starts with if (PHP_SAPI !== 'cli') { http_response_code(404); exit; }. CI fails without it (tests/web-exposure-guard.php). Files in includes/ only define functions and classes - opening one in a browser must do nothing.
• Check access before you write anything - module access, write access (Read only analysts change nothing), the company (multi-company installs), and for the REST API the key's permission. Before a file reaches the disk, not after.
• A REST permission does one job. Add a narrow resource or action (change_attachments.create) rather than borrowing a broad one (changes.update). The verbs are read, create, update, delete (and manage for configuration).
• Session endpoints check the CSRF token, and error messages never include database or exception text.

• Schema goes in two places by hand, and one by script: database/freeitsm.sql (new installs) and includes/db_verify_schema.php (Database Verification, existing installs). Then run php scripts/gen_db_verify_indexes.php - includes/db_verify_indexes.php is generated, and CI fails if it is edited by hand.
• Verification adds missing tables, columns and indexes - and nothing else. It does not widen a column, change a unique key or move data. If an existing install needs that, write an upgrade step that is safe to run twice and keeps every row.
• Every column that names an analyst goes in analystReferenceRegistry() (includes/analyst_lifecycle.php), with its kind (history, setup, live, ...). tests/analyst-lifecycle.php fails otherwise.

• One write path. If the UI already writes it, move that code into the service in includes/services/ and call it from both - never a second copy in the API.
• Own the transaction, or join the caller's. Side effects - workflows, emails, calendar sync, notifications - run after the commit, never inside it.
• A failed lookup is not an answer. Never turn an error into "no", "empty" or "not needed". Let it fail, or report it as unknown.
• Never swallow an error to make a test pass, and never return 200 for something you ignored - say what was ignored.
• Other people's rows are not yours. An importer touches only the rows its own source wrote. Before narrowing or widening a DELETE, find every writer of that table and check who relied on the old behaviour.

The tests run against the developer's own database, which is usually a copy of real data. So:

• Create what you need, with a ZZ/zz_ prefix, and delete it by the ids you created - never by name pattern or by "everything matching". A name can be truncated by a narrow column and never found again.
• Never go through a path that sends email, fires workflows or notifies people unless that is the thing being tested. Write the fixture row directly.
• Never change install settings. If you must, snapshot them and put them back in finally.
• Prove the test can fail. A control that fails on the old code is worth more than ten that pass.
• PHP and Chrome only - no Node.js, no npm. Every test runs as php tests/<name>.php. A test that needs a real browser drives Chrome over its DevTools socket from PHP: copy cdpEvaluate() from tests/task-form-embed.php (PR #198).
• Work on an install in a subfolder. FreeITSM is often at http://server/freeitsm/, not the web root. Build every URL from FREEITSM_TEST_URL or relative to the page - never fetch('/auth/login.php') or a pathname compared with === (#211-#213).
• A test that writes needs somewhere safe to write. The suites for #211-#213 take FREEITSM_TEST_DATABASE and refuse to run unless it matches the install's database - copy that guard. A disposable install is a git worktree under the web root with its own config.php and a database loaded from database/freeitsm.sql (recipe).
• Clean up API keys' rate-limit rows (api_key_rate_limits) along with the keys.

• New strings go in lang/en only. The other locales are translated together before each release.
• t() has no fallback argument, and existing English wording stays as it is - changing it breaks every translation of it.
• Buttons are one word where possible ("Save", "New", "Delete").

• Every operation is in api/v1/spec.json, and php api/v1/lib/openapi_check.php passes.
• Errors use apiError() with a stable code (device_ref_conflict), and the message says what to do next.
• Retries are safe: a client_reference (or similar) returns the original for the same content and a 409 for different content.

FreeITSM needs PHP 8.1, and CI checks every file parses on 8.1 - so nothing from 8.2 or later (readonly classes, true / null types, typed class constants). Within that:

• PHP 8 syntax only in files under an includes/ folder. match, enums, named arguments, ?->, readonly, never, constructor promotion, catch (X) without a variable - and Cap::X->value, which is syntax too. Never in a file the web server or cron runs directly, and never in includes/functions.php or includes/ssl.php. PHP reads a whole file before running any of it, so on an older PHP such a file is a parse error before includes/php_version_guard.php can say "FreeITSM needs PHP 8.1". PHP 8 functions - str_contains(), array_is_list() - are fine anywhere.
• A page loads config.php (or includes/functions.php) before any other includes/ file, so the guard runs first. A page that cannot - setup, the self-service portal - requires includes/php_version_guard.php as its first line.
• The PHP floor CI job and tests/php-version-guard.php check both. Why: Raising the PHP floor.

Where the code owns a closed list, it is an enum, and the value is what the database stores:

• Never compare a stored string with a case. $row['provider'] === MailboxProvider::Imap is silently false. Turn the string into a case first (fromMailbox(), Cap::from(), tryFrom()), then compare cases.
• Permissions: a guard takes the case - requireCapabilityJson(Cap::TICKETS_SLA) - and refuses a string. Anywhere else (manifests, arrays, HTML) write capKey(Cap::TICKETS_SLA). tests/cap-enum-usage.php checks every Cap:: in the codebase.
• A new PHP extension goes in docker/php-extensions.txt and the Dockerfile, or the image is built without it - tests/docker-php-extensions.php fails until it is.

• Match the file you are in. Keep the comments that explain why, and add a TRAP: comment wherever the obvious change would be wrong (grep -rn "TRAP:" lists them all).
• One statement per line, spaces around operators, a docblock that says why. $page=$string('page','1'); $size=... is rewritten before it is merged (#213).
• Rewriting a function? Carry its comments across unless they have become untrue.
• Wrapping a global (window.fetch, a function another file calls) gets the biggest comment in the file: nobody reading a fetch() elsewhere will guess it is being intercepted.

• Secure-context APIs are missing on plain HTTP. crypto.randomUUID(), navigator.clipboard and service workers exist only on HTTPS or localhost, and FreeITSM is often reached as http://helpdesk/ inside a network. Check window.isSecureContext or use something else - a randomUUID() in #212 would have stopped anyone leaving a ticket.
• Anything the browser hides, the server refuses. Hiding an Export button is a courtesy; the endpoint checks the setting on every request.
• Documentation goes on this wiki, not in a docs/ folder in the repo.

php -l <every changed PHP file>
php scripts/gen_db_verify_indexes.php --check
php tests/web-exposure-guard.php
php api/v1/lib/openapi_check.php          # if you touched the REST API
php tests/analyst-lifecycle.php           # if you added a column that names an analyst
php tests/cap-enum-usage.php              # if you touched a permission, a manifest or a settings page
php tests/php-version-guard.php           # pages load the guard first; with Docker, also runs PHP 7.4 and 8.4
php tests/docker-php-extensions.php       # if you used a PHP extension
FREEITSM_TEST_URL=http://localhost/your-site php tests/<your-suite>.php

Then run Database Verification (System → Database) on an install that already had data, and check that nothing changed that should not have.