CALIBER_LEARNINGS.md

Rama master · réplica de lectura del repositorio original.

# Caliber Learnings

Accumulated patterns and anti-patterns from development sessions.
Auto-managed by [caliber](https://github.com/caliber-ai-org/ai-setup) — do not edit manually.

- **[gotcha]** After the first `git push <empty-remote> master` to a brand-new empty GitHub repo, the repo's default branch stays empty string (`gh repo view OWNER/REPO --json defaultBranchRef` returns `{"defaultBranchRef":{"name":""}}`) until you explicitly `gh repo edit OWNER/REPO --default-branch master`. Without this, opening a PR from a feature branch may auto-target a non-existent base or fail outright. Always run `gh repo edit ... --default-branch master` immediately after the first master push to an empty repo. Also: when cloning an empty repo, `git symbolic-ref HEAD refs/heads/master` switches the local HEAD off the default `main` so the first commit lands on `master` and matches the GitHub default.
- **[gotcha]** `gh api -X DELETE repos/OWNER/REPO/git/refs/heads/<branch>` with a HEREDOC-style multi-line bash command sometimes returns an empty `{}` Bash response (no stdout, no error). Retry the same call on a single line and it prints the normal `{"message":"Reference does not exist","status":"422"}` for an already-gone branch, or success for one that exists. When `gh api` returns empty in a multi-line script, collapse to one-liner before assuming the API broke.
- **[pattern]** Branching with `feature-dev:code-reviewer` (or any non-default subagent type) — pass via `subagent_type: "feature-dev:code-reviewer"`. The default `oac:code-reviewer` is read-only (`Read, Glob, Grep`) so it cannot run verification commands like `gh`, `phpunit`, or `phpstan` itself; for review steps that must *re-run* CI bars to verify claims, spawn a richer agent type that has Bash access (e.g. the project's `feature-dev:code-reviewer` if defined, or `general-purpose`). Picking the wrong reviewer type silently leaves the verification bar unchecked.
- **[gotcha]** PHPUnit `--coverage-filter src/Foo` on the CLI does NOT change the "Code Coverage Report Summary" totals printed at the end — that summary always reflects the `<source>` block in `phpunit.xml`, not the runtime filter. To prove per-file coverage (e.g. "Version.php is 100%"), parse `coverage.xml` (Clover) directly: regex-extract the `<file name="...Version.php">...</file>` block and read its `<metrics statements="N" coveredstatements="M"/>` totals. Don't trust the summary line as evidence the filter worked.
- **[gotcha]** PSR-12's "Multi-line use declarations must define one parameter per line" rule (`PSR12.Functions.MultiLineArguments`) **applies to closure inheritance `use (...)` clauses too**, not just top-of-file `use SomeClass;` imports. A closure whose parameter list spans multiple lines must also break its `use ($a, $b)` clause one-per-line: `use (\n    $router,\n    $logger,\n): void {`. PHPCS reports this as a single ambiguous "Multi-line use declarations" error pointing at the `) use (...)` line — the message reads identically to a misordered `use` import block, so don't waste time scanning the imports section.
- **[gotcha]** `Workerman\MySQL\Connection::__construct` declares `@param int $port` (see `vendor/workerman/mysql/src/Connection.php` line 1697) — passing `(string) $connConfig['port']` fails PHPStan level 9 with `argument.type`. Cast to `(int)` even though the class stores it back into `settings['port']` untyped. `phlix-server` (the older repo) may still cast as `(string)` because PHPStan ran at a lower level there; for new code at level 9, always `(int)`.
- **[gotcha]** `Monolog\Handler\HandlerInterface` does NOT declare `setLevel()` — that method lives on `Monolog\Handler\AbstractHandler`. Calling `$handler->setLevel($level)` on a variable typed as `HandlerInterface` trips PHPStan level 9 `method.notFound`. **Fix:** every concrete Monolog handler (`RotatingFileHandler`, `StreamHandler`, etc.) accepts a `Level` in its constructor — pass it there instead of mutating post-construction, and the level config can be the source of truth. Don't type-narrow the variable to `AbstractHandler` just to silence the error.
- **[convention]** PHPCS `--standard=PSR12` in this repo enforces a 120-character line limit as a WARNING. Long `@param string $name Description...` docblock lines with vertical alignment of the description column easily exceed 120 chars on names like `$hostnameCandidates`. **Fix:** wrap the description onto the next line at the same column indent. `failOnWarning="true"` in `phpunit.xml` does NOT apply to phpcs output, but PR check pipelines often fail the suite on phpcs warnings, so treat warnings as errors for green CI. **Note:** existing pre-Phase-B files carry baseline `>120 chars` warnings that are NOT from your work — `phpcs src/` always emits ~6 baseline warnings even on a green branch. Only treat NEW warnings against files you touched as blockers.
- **[gotcha]** `class_alias()` does NOT work on PHP interfaces or enums of the wrong kind — calling it on a `Phlix\Plugins\Contract\LifecycleInterface` (an interface) is a silent no-op for `instanceof` checks. **Fix:** declare a bridge sub-interface in the old location: `namespace Phlix\Plugins\Contract; interface LifecycleInterface extends \Phlix\Shared\Plugin\LifecycleInterface {}`. Legacy implementers `implements LegacyLifecycleInterface` still satisfy `instanceof SharedLifecycleInterface` thanks to inheritance, in both directions. For `class_alias()` of enums, check `enum_exists()` separately — `class_exists($oldFqcn, false)` returns false for enums even when they exist.
- **[gotcha]** `composer validate --strict` reports `Publish errors: - description : The property description is required` against a repo's `composer.json` when it has `"type": "project"` without a `description` field. This is INTENTIONAL for `phlix` (project, not package) — DON'T add a fake `description` field to silence it. Use `composer validate` (no `--strict`) in CI/verify scripts for project repos; reserve `--strict` for the sibling phlix-shared/phlix-hub library packages. **However** for the library packages themselves (phlix-shared, phlix-hub), use `composer validate --strict --no-check-publish` in CI — `--no-check-publish` skips the `description`-required gate while still validating schema/dependency integrity, so the libraries can stay un-Packagisted (consumed via VCS repositories) without lying about being publishable. **Exception:** `phlix-hub`'s `composer.json` DOES carry a real `description`, so it passes plain `composer validate --strict` without `--no-check-publish` — adopt the same pattern if a new sibling repo should be Packagist-ready later.
- **[pattern]** **Psalm MixedAssignment from `array<string, mixed>` reads** — `$x = $config['key'] ?? $default;` where `$config` is `array<string, mixed>` triggers Psalm `MixedAssignment` at level 1 strictness because the read type is `mixed`. **Fix:** annotate the LHS with `/** @var mixed $x */` on the line BEFORE the assignment, then narrow with `is_string()` / `is_int()` / `is_array()` into a typed variable: `/** @var mixed $hostRaw */\n$hostRaw = $this->config['host'] ?? '0.0.0.0';\n$host = is_string($hostRaw) ? $hostRaw : '0.0.0.0';`. Same shape for every `?? $default` read from a `mixed`-valued array — `foreach`-with-`is_string($key)` filtering also needs `/** @var mixed $value */` on the foreach line plus `@psalm-suppress MixedAssignment` when the body re-assigns to a typed array slot.
- **[pattern]** **Psalm `UnresolvableInclude`** on dynamic `include $configPath` / `include self::$configPath` — Psalm cannot resolve runtime-computed paths. **Fix:** combine `@psalm-suppress UnresolvableInclude` with the `@var` annotation on the include result in a single docblock: `/**\n * @psalm-suppress UnresolvableInclude\n * @var array<string, mixed> $config\n */\n$config = include self::$configPath;`. This is the canonical shape used by both `LoggerFactory::get()` and `ConnectionPool::getConnection()` in `phlix-hub`.
- **[pattern]** **Psalm `MixedMethodCall` on `new $rawClass()`** — even after `is_string($rawClass)` + `class_exists($rawClass)` narrowing, Psalm still flags `new $rawClass()` because the constructor signature is unknown. The `/** @var class-string $rawClass */` hint above the `new` line is NOT enough on its own. **Fix:** `/**\n * @psalm-suppress MixedMethodCall\n * @var object $instance\n */\n$instance = new $rawClass();` — suppress the method-call warning AND type the result, in one docblock.
- **[gotcha]** **Workerman writes transient files into `public/`** when started via `php public/index.php start`: `public/workerman.<entryname>.pid` and `public/workerman.log` appear after every boot smoke test. Ensure `.gitignore` includes `public/workerman.*.pid` and `public/workerman.log` (NOT the generic `*.pid` — entry-named pid files vary by index filename). Without these entries, smoke-testing the Workerman entry mid-CI verification dirties the worktree and a careless `git add -A` ships the pid into the initial commit.
- **[pattern]** **Workerman boot-smoke pattern for CI verification:** `php public/index.php start >/tmp/hub-smoke.log 2>&1 &; PID=$!; sleep 2; curl -sS http://localhost:8800/health; kill $PID; wait $PID 2>/dev/null`. The 2-second sleep is non-negotiable — Workerman's bind takes ~1.5s on this machine. Without the `wait` the next bash invocation may inherit a half-dead child; with it, the smoke is fully synchronous. Send all stderr to a temp file rather than the terminal — Workerman's startup banner is verbose and clutters the verification report.
- **[pattern]** **Empty-GitHub-repo init flow** (when a repo is pre-created empty on GitHub, e.g. via `gh repo create OWNER/REPO --public --confirm` ahead of time, OR via the web UI): `git clone git@github.com:OWNER/REPO.git` (expect the "you appear to have cloned an empty repository" warning), scaffold all files, `git add -A`, `git commit -m "Initial release v0.1.0"`, `git push -u origin master`. The initial commit goes DIRECTLY to master — do NOT branch+PR for the first commit because there's no base branch to target. After this, the standard branch+PR ritual applies for every subsequent change. CI on master runs immediately after the first push and is the green-bar gate.
- **[gotcha]** `Read` on a file >1000 lines without explicit `offset`/`limit` has been observed to return an empty `{}` response even though the default limit is 2000 lines. Same empty `{}` for files that don't exist (e.g. `.git/packed-refs` in a fresh clone), AND for any path that is a DIRECTORY rather than a file (e.g. `Read /home/sites/phlix-hub/src` returns `{}` silently — use `Glob` with `path:` or `find -type f` to list directory contents). Same `{}` for any path that exists in the main worktree but NOT in the current `git worktree`'s checkout. When this happens, fall back to `wc -l` + `grep -n '^## '` to map sections, then `Read` with explicit `offset`/`limit` chunks — don't retry the unbounded read in a loop.
- **[gotcha]** `gh -R OWNER/REPO repo view --json X,Y` has been observed to return an empty `{}` response while the alternate form `gh repo view OWNER/REPO --json X,Y` returns the expected JSON. Same shape for `gh api -X DELETE repos/OWNER/REPO/git/refs/heads/<branch>` with a HEREDOC-style multi-line bash command — sometimes returns empty `{}` (no stdout, no error); retry on a single line and it prints normally. When `gh` returns empty in a multi-line script, collapse to one-liner or use positional-arg form before assuming the API broke.
- **[pattern]** **Sibling-repo metadata setup step (B.Xa)**: After a new `detain/phlix-*` repo is scaffolded and CI is green, set description + homepage + topic tags in one batched `gh repo edit` call: `gh repo edit OWNER/REPO --description "..." --homepage "https://phlix.media" --add-topic media-server --add-topic self-hosted --add-topic plex --add-topic jellyfin --add-topic php --add-topic php8 --add-topic workerman --add-topic ...` etc. GitHub accepts ~19 topics per repo. Confirm with `gh repo view OWNER/REPO --json name,description,homepageUrl,repositoryTopics`. The B.2a / B.4a / B.5a sibling-of-step pattern always immediately follows scaffold + CI-green.
- **[convention]** **Phlix sibling repos use a tracking-table migration runner** — `Phlix\<Pkg>\Common\Database\MigrationRunner` introduced in `phlix-hub` B.6 records every applied `NNN_name.sql` in a `migrations` tracking table (filename PRIMARY KEY) and skips already-applied files on re-run. **Don't model new migration code after the legacy `phlix-server/scripts/run-migrations.php`** — that legacy script blindly re-runs every `.sql` file every invocation and only survives because the SQL uses `CREATE TABLE IF NOT EXISTS`. For new sibling repos, copy the hub's `MigrationRunner` pattern: lex-ordered `*.sql` discovery, statement-split on `;`, single-line `--` comments stripped before split, parameterized `INSERT INTO migrations (filename) VALUES (?)` to record success, wrap each `applyFile()` failure in a `RuntimeException` that includes basename + raw statement.
- **[gotcha]** **PSR-12 file-level docblock must come BEFORE `declare(strict_types=1);`, immediately after `<?php`** — not after `declare`. PHPCS error reads `The file-level docblock must follow the opening PHP tag in the file header` (line points at the docblock, not the declare). Canonical order: `<?php` → blank line → `/** ... */` file docblock → blank line → `declare(strict_types=1);` → blank line → `use ...;` imports. Easy trap because most PHP code style guides emphasize `declare` placement but stay silent on file-docblock ordering relative to it. Class/method docblocks are unaffected — this only governs the FILE-level docblock at the top.
- **[gotcha]** **`phpcs.xml.dist` `<file>` blocks scope-restrict the run** — even though `./vendor/bin/phpcs --standard=PSR12 scripts/` *with an explicit path* lints `scripts/`, running `./vendor/bin/phpcs` with no args uses only the `<file>` entries (default just `<file>src</file>`). Add `<file>scripts</file>`, `<file>tests</file>` as needed so CI's bare `phpcs` invocation catches them too. **Asymmetric trap:** the CI workflow may invoke `phpcs --standard=PSR12 -n --colors src/` with an explicit `src/` path, in which case it doesn't read `phpcs.xml.dist`'s `<file>` blocks at all — local verification with `./vendor/bin/phpcs` (no args) and CI's `phpcs ... src/` can both pass while a third invocation that DOES use the config (e.g. someone running `phpcs scripts/`) catches new errors. When adding a new top-level directory, update BOTH `phpcs.xml.dist` AND every workflow file that uses an explicit path.
- **[gotcha]** **Psalm `RedundantConditionGivenDocblockType`** — annotating an array-returning query result with an *over-specific* docblock like `/** @var list<array{filename: string}>|null $rows */` makes subsequent `isset($row['filename']) && is_string($row['filename'])` runtime checks redundant per Psalm because the docblock already promised the structure. Two ways out: (a) drop the runtime narrowing if you trust the docblock (and the schema), or (b) **weaken the docblock to `/** @var mixed $rows */`** so the runtime checks remain meaningful and Psalm doesn't complain. The second is safer for `$db->query(...)` results because Workerman MySQL returns raw mixed arrays — promising `list<array{...}>` is a lie that breaks the moment the schema drifts. Use `/** @var mixed $rows */` + `is_array()` + per-row `is_array($row) && isset($row['key']) && is_string($row['key'])` narrowing.
- **[gotcha]** **PHPStan `arrayValues.list`** — calling `array_values($x)` where PHPStan already infers `$x` as `list<...>` is a flagged no-op: `Parameter #1 $array (list<string>) of array_values is already a list, call has no effect.` Common trip: `glob()` returns `list<string>|false` and `sort()` preserves listness, so `return array_values($files);` after `sort($files);` triggers the error. Just `return $files;` — PHPStan knows it's already a list. Only add `array_values()` when input has non-sequential numeric keys (e.g. after `array_filter()` without `ARRAY_FILTER_USE_KEY`).
- **[gotcha]** **MySQL Group Replication multi-primary forbids `ON DELETE CASCADE` foreign keys** — error reads `SQLSTATE[HY000]: General error: 3098 The table does not comply with the requirements by an external plugin.` on the FIRST insert into a child table (not at `CREATE TABLE` time, so the migrations apply cleanly but every integration test that inserts data fails). Diagnosis: `SHOW VARIABLES LIKE 'group_replication_enforce_update_everywhere_checks'` returns `ON` on a multi-primary cluster. The phlix-hub schema targets a **single-primary** deployment because CASCADE FKs are load-bearing. **Fix in integration tests:** add a `skipOnIncompatibleCluster()` guard at the top of `setUp()` that queries this variable and calls `self::markTestSkipped(...)` when `ON`. Don't try to rewrite the schema to avoid CASCADE — it's the schema's correct shape; just gate the integration suite. The local dev MySQL on this host (`174.138.179.252`) runs Group Replication multi-primary, so this skip will fire there but NOT in single-primary CI.
- **[pattern]** **Workerman MySQL named-vs-positional placeholders** — `Workerman\MySQL\Connection::query()` accepts both `?` positional with a list-style params array AND `:name` named placeholders with an assoc-array. **Prefer named placeholders for any query with 3+ params** — they read better and survive parameter reordering, which is the recurring cause of silent bind mismatches in long `INSERT` statements. For 1-2 param queries, either is fine. The mock pattern in unit tests must match the producer: if the production code uses `[":filename" => $file]`, the test mock must read `$params['filename']`, not `$params[0]`.
- **[pattern]** **`composer audit --no-dev`** is the canonical CI-bar security check in sibling repos — runs against the `require` block only, ignoring dev tooling. Use this when verifying a v0.x release. The output `No security vulnerability advisories found.` is the green signal; any non-empty advisory list blocks the release. Don't run plain `composer audit` (which includes dev deps) when verifying a runtime release — phpunit/phpstan/psalm pull in dev-time advisories that don't matter for production.