Skip to content

Commit f0aeebf

Browse files
detainclaude
detain
and
claude
committed
docs: point the agent docs at the contract harness
This package's test-writing guidance predated the shared contract harness and taught the reflection-only pattern the harness replaced -- assert a handler exists, is static, takes one parameter, and stop. That passes whether or not the handler works, and it is what a session reads before it reads any code. Adds the generated plugin-contract-tests skill, amends the older skills in place rather than rewriting them (they carry per-package knowledge written down nowhere else), and gives CLAUDE.md the always-on version. No test or skill was removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent c888b3c commit f0aeebf

5 files changed

Lines changed: 211 additions & 3 deletions

File tree

‎.claude/skills/kayako-api-function/SKILL.md‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,31 @@
11
---
22
name: kayako-api-function
3-
description: Adds a new procedural API function to `src/api.php` following the project's validation-first, try/catch-per-operation pattern. Initializes Kayako SOAP config, wraps each Kayako call in its own try/catch, returns status/status_text arrays. Use when user says 'add API function', 'new ticket operation', 'create endpoint in api.php', or 'add support function'. Do NOT use for modifying `src/Plugin.php` hook registration or for class-based API work.
3+
description: Adds a new procedural API function to `src/api.php` following the project's validation-first, try/catch-per-operation pattern. Initializes Kayako SOAP config, wraps each Kayako call in its own try/catch, returns status/status_text arrays. Use when user says 'add API function', 'new ticket operation', 'create endpoint in api.php', or 'add support function'. Do NOT use for modifying `src/Plugin.php` hook registration or for class-based API work. NOTE: for a plugin's contract/behavioral tests (tests/ContractTest.php, the shared harness, composer myadmin:scaffold-tests) use the plugin-contract-tests skill instead — this skill's reflection-only guidance predates that harness.
44
---
5+
<!-- myadmin-contract-harness-notice -->
6+
> ### ⚠️ Read this before the rest of the file
7+
>
8+
> This package is on the **shared plugin contract harness**. Parts of the guidance below
9+
> predate it and are now wrong in one specific way:
10+
>
11+
> **Any instruction here that a plugin's `getHooks()` / `getSettings()` / `getActivate()` /
12+
> `getDeactivate()` / `getQueue()` must not be *called* — that only its existence, visibility
13+
> or parameter count may be checked through `ReflectionClass` — no longer applies.** That rule
14+
> existed because those methods reference bare constants (`PRORATE_BILLING` and friends) that
15+
> only a live MyAdmin request defines, so calling them from a test used to fatal. The harness
16+
> defines them first. It then executes the handlers for real, in a process of its own.
17+
>
18+
> A reflection-only assertion passes whether or not the thing works: `getActivate()` can exist,
19+
> be public, be static, take one argument, and still fatal the moment it runs. Three real
20+
> production bugs in this fleet were sitting behind assertions of exactly that shape.
21+
>
22+
> **Use the `plugin-contract-tests` skill** for anything touching `tests/ContractTest.php`,
23+
> the contract inspectors, or `composer myadmin:scaffold-tests`.
24+
>
25+
> **Everything else in this file is still accurate and still applies** — this package's own
26+
> classes, its API wrappers, its fixtures, its bootstrap, and the reasons certain classes must
27+
> not be constructed. Nothing below has been removed.
28+
529
# kayako-api-function
630

731
## Critical

‎.claude/skills/phpunit-test/SKILL.md‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,31 @@
11
---
22
name: phpunit-test
3-
description: Writes PHPUnit 9 tests mirroring patterns in `tests/ApiFunctionsTest.php` and `tests/PluginTest.php`. Stubs MyAdmin globals in `setUpBeforeClass()`, uses `ReflectionFunction`/`ReflectionClass` for signature checks, tests validation failures before live calls. Use when user says 'write tests', 'add test coverage', 'test this function'. Do NOT use for integration tests requiring a live Kayako instance.
3+
description: Writes PHPUnit 9 tests mirroring patterns in `tests/ApiFunctionsTest.php` and `tests/PluginTest.php`. Stubs MyAdmin globals in `setUpBeforeClass()`, uses `ReflectionFunction`/`ReflectionClass` for signature checks, tests validation failures before live calls. Use when user says 'write tests', 'add test coverage', 'test this function'. Do NOT use for integration tests requiring a live Kayako instance. NOTE: for a plugin's contract/behavioral tests (tests/ContractTest.php, the shared harness, composer myadmin:scaffold-tests) use the plugin-contract-tests skill instead — this skill's reflection-only guidance predates that harness.
44
---
5+
<!-- myadmin-contract-harness-notice -->
6+
> ### ⚠️ Read this before the rest of the file
7+
>
8+
> This package is on the **shared plugin contract harness**. Parts of the guidance below
9+
> predate it and are now wrong in one specific way:
10+
>
11+
> **Any instruction here that a plugin's `getHooks()` / `getSettings()` / `getActivate()` /
12+
> `getDeactivate()` / `getQueue()` must not be *called* — that only its existence, visibility
13+
> or parameter count may be checked through `ReflectionClass` — no longer applies.** That rule
14+
> existed because those methods reference bare constants (`PRORATE_BILLING` and friends) that
15+
> only a live MyAdmin request defines, so calling them from a test used to fatal. The harness
16+
> defines them first. It then executes the handlers for real, in a process of its own.
17+
>
18+
> A reflection-only assertion passes whether or not the thing works: `getActivate()` can exist,
19+
> be public, be static, take one argument, and still fatal the moment it runs. Three real
20+
> production bugs in this fleet were sitting behind assertions of exactly that shape.
21+
>
22+
> **Use the `plugin-contract-tests` skill** for anything touching `tests/ContractTest.php`,
23+
> the contract inspectors, or `composer myadmin:scaffold-tests`.
24+
>
25+
> **Everything else in this file is still accurate and still applies** — this package's own
26+
> classes, its API wrappers, its fixtures, its bootstrap, and the reasons certain classes must
27+
> not be constructed. Nothing below has been removed.
28+
529
# PHPUnit Test
630

731
## Critical

‎.claude/skills/plugin-contract-tests/SKILL.md‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
---
2+
name: plugin-contract-tests
3+
description: Sets up, regenerates or debugs the shared MyAdmin plugin contract harness for this package — tests/ContractTest.php, the contract inspectors, and `composer myadmin:scaffold-tests`. Use when the user says 'add tests to this plugin', 'set up the harness', 'scaffold tests', 'why is ContractTest failing', or when deciding whether a contract failure is the plugin's fault or the harness's. Do NOT use for this package's own non-plugin classes — its other testing skills cover those.
4+
---
5+
# Plugin contract tests
6+
7+
The class under contract here is `Detain\MyAdminKayako\Plugin`.
8+
9+
## Critical
10+
11+
- **Never hand-write or hand-edit `tests/ContractTest.php`.** It is generated. Run
12+
`composer myadmin:scaffold-tests` from inside this repo; regenerate with `--force`. A hand
13+
edit is invisible to the next regeneration and to the next person.
14+
- **Never write a reflection-only test for the plugin class.** Asserting that a handler exists,
15+
is static and takes one parameter passes whether or not the handler works. Execute it. The
16+
harness has already done the hard part — priming the constants that used to make that
17+
impossible.
18+
- **Never delete an existing test to make room.** The harness is strictly additive: `ContractTest`
19+
runs *alongside* whatever this package already had. Duplicate coverage is the cheaper mistake.
20+
Removing anything is a question for the owner first — that is a standing rule on this fleet.
21+
- **Run the whole suite, never just `--filter ContractTest`.** The contract class primes constants
22+
and calls `register_module()`, neither of which can be undone, so it can change how this
23+
package's *other* tests behave. A filtered run cannot show that.
24+
- **`composer myadmin:scaffold-tests` does not exist in MyAdmin core.** Core sets
25+
`config.allow-plugins: false`, so Composer never activates the installer there and no
26+
`myadmin:*` command is registered. Run it from this repo.
27+
- This package declares no `$module`, which is correct for a `type=plugin` package and is asserted bidirectionally by A-7 — adding one without adding the module is a failure.
28+
29+
## Instructions
30+
31+
### Step 1 — regenerate, do not edit
32+
33+
```bash
34+
composer myadmin:scaffold-tests # plan only; writes nothing
35+
composer myadmin:scaffold-tests --write # create what is missing
36+
composer myadmin:scaffold-tests --force --write # also re-emit tests/ContractTest.php
37+
```
38+
39+
`CREATE` means a file is missing. `KEEP` means one exists and will not be touched. `DRIFT`
40+
means an existing `phpunit.xml.dist` is missing a setting the harness depends on.
41+
42+
If Composer deadlocks, this package still vendors installer `v2.0.2`, which predates Composer
43+
2's `PluginInterface` and fatals while activating — break it once with `composer update
44+
--no-plugins`.
45+
46+
### Step 2 — fix a reported DRIFT by hand
47+
48+
The three settings are load-bearing, not stylistic:
49+
50+
- `failOnWarning="true"` — several findings surface first as a PHP warning; without it PHPUnit
51+
prints the finding and exits 0.
52+
- `failOnRisky="true"` — a test asserting nothing because its subject would not load is risky,
53+
not passing.
54+
- `beStrictAboutOutputDuringTests="true"` — assertion B-15 (a plugin must not echo while its
55+
handlers run) is unenforceable without it.
56+
57+
### Step 3 — classify a failure before changing anything
58+
59+
This decides *which repository you touch*, so do it first:
60+
61+
| symptom | verdict | action |
62+
|---|---|---|
63+
| the plugin genuinely does the wrong thing — uses a variable before assigning it, constructs a class with the wrong arity, registers a requirement path that does not exist | **P-bug** | fix in this repo, on its own branch, with its own review. Do not bundle it into a test-scaffolding commit |
64+
| the harness accuses the plugin of something it did not do | **H-bug** | fix in `detain/myadmin-plugin-installer`, never here, and add the counter-test proving the inspector can still fail |
65+
| the blocker is the environment — a `require` of a path that only exists inside a MyAdmin checkout | neither | the inspector should *skip*, naming the blocker. If it fails instead, that is an H-bug |
66+
67+
Three H-bugs have shipped, and all three were the harness falsely accusing a plugin: a shadowed
68+
observer read as dead code (v2.1.1), a failed `require` read as the handler's own logic (v2.1.2),
69+
and a Windows path treated as relative so every package looked like it shipped no templates
70+
(v2.2.1). **Suspect the harness first** when a verdict changes depending on how the suite was
71+
launched, or when a finding fires on every package at once.
72+
73+
### Step 4 — if the generated file is wrong, change the generator
74+
75+
`src/Testing/Scaffold/ContractTestGenerator.php` in the installer is the single source of truth
76+
for all 66 generated copies. Fix it there, tag, then regenerate here.
77+
78+
## Three ordering rules the generated file encodes
79+
80+
They look like style. They are not.
81+
82+
1. **`primeConstants()` runs before the plugin class is mentioned at all.** A static property
83+
initializer can reference a bare constant — `$settings` holding
84+
`REPEAT_BILLING_METHOD => PRORATE_BILLING` is the common shape — and initializers run on class
85+
*load*, so even reading `::$type` fatals on an unprimed class.
86+
2. **The hook table is read through `TierA5HooksAreIdempotent::hookTable()`,** never a direct
87+
`getHooks()` call. A direct call is a second, independent answer to a question A-5 owns, and
88+
the two disagree for any plugin whose body touches a bare constant.
89+
3. **The table is evaluated exactly once.** Calling `getHooks()` twice asserts idempotence by
90+
accident and doubles whatever side effect the body has.
91+
92+
Plus `@runTestsInSeparateProcesses` + `@preserveGlobalState disabled`, always.
93+
94+
### Namespaced stubs
95+
96+
If this package ships a `tests/stubs.php` declaring helpers **inside the plugin's own
97+
namespace**, PHP binds the plugin's unqualified calls to those rather than to the harness's
98+
observers. Eight packages in the fleet do this. The harness detects the shadow and skips instead
99+
of accusing, but the assertion is then vacuous. Prefer forwarding such a stub into the harness
100+
over making it a no-op, so the observation still lands.
101+
102+
## Verify
103+
104+
```bash
105+
vendor/bin/phpunit
106+
```
107+
108+
Whole suite, green, before committing.
109+
110+
## Reference
111+
112+
- `docs/testing-harness.md` in `detain/myadmin-plugin-installer` — §1.5 scaffolding, §3 traps,
113+
§7 the P-bug/H-bug split, §11 the generated file.
114+
- `.claude/rules/plugin-tests.md` in MyAdmin core.

‎.claude/skills/plugin-hook/SKILL.md‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,31 @@
11
---
22
name: plugin-hook
3-
description: Adds a new event hook to src/Plugin.php — registers handler in getHooks(), implements the static method accepting GenericEvent, and wires up api_register/add_requirement/add_setting calls. Use when user says 'add hook', 'register new event', 'add plugin method', or needs to expose a new setting via getSettings(). Do NOT use for changes to src/api.php procedural functions.
3+
description: Adds a new event hook to src/Plugin.php — registers handler in getHooks(), implements the static method accepting GenericEvent, and wires up api_register/add_requirement/add_setting calls. Use when user says 'add hook', 'register new event', 'add plugin method', or needs to expose a new setting via getSettings(). Do NOT use for changes to src/api.php procedural functions. NOTE: for a plugin's contract/behavioral tests (tests/ContractTest.php, the shared harness, composer myadmin:scaffold-tests) use the plugin-contract-tests skill instead — this skill's reflection-only guidance predates that harness.
44
---
5+
<!-- myadmin-contract-harness-notice -->
6+
> ### ⚠️ Read this before the rest of the file
7+
>
8+
> This package is on the **shared plugin contract harness**. Parts of the guidance below
9+
> predate it and are now wrong in one specific way:
10+
>
11+
> **Any instruction here that a plugin's `getHooks()` / `getSettings()` / `getActivate()` /
12+
> `getDeactivate()` / `getQueue()` must not be *called* — that only its existence, visibility
13+
> or parameter count may be checked through `ReflectionClass` — no longer applies.** That rule
14+
> existed because those methods reference bare constants (`PRORATE_BILLING` and friends) that
15+
> only a live MyAdmin request defines, so calling them from a test used to fatal. The harness
16+
> defines them first. It then executes the handlers for real, in a process of its own.
17+
>
18+
> A reflection-only assertion passes whether or not the thing works: `getActivate()` can exist,
19+
> be public, be static, take one argument, and still fatal the moment it runs. Three real
20+
> production bugs in this fleet were sitting behind assertions of exactly that shape.
21+
>
22+
> **Use the `plugin-contract-tests` skill** for anything touching `tests/ContractTest.php`,
23+
> the contract inspectors, or `composer myadmin:scaffold-tests`.
24+
>
25+
> **Everything else in this file is still accurate and still applies** — this package's own
26+
> classes, its API wrappers, its fixtures, its bootstrap, and the reasons certain classes must
27+
> not be constructed. Nothing below has been removed.
28+
529
# plugin-hook
630

731
## Critical

‎CLAUDE.md‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,28 @@ myadmin_log('api', 'info', $e->getMessage(), __LINE__, __FILE__);
7979
| `KAYAKO_API_SECRET` | API authentication secret |
8080

8181
<!-- caliber:managed:pre-commit -->
82+
## Plugin contract harness
83+
84+
This package is on the shared contract harness from `detain/myadmin-plugin-installer`.
85+
`tests/ContractTest.php` is **generated** — run `composer myadmin:scaffold-tests` (add
86+
`--force --write` to re-emit it), never hand-edit it.
87+
88+
The harness **executes** the plugin: it defines the bare constants the class body references
89+
and then calls `getHooks()`, `getSettings()`, `getMenu()`, `apiRegister()` and — for
90+
`type=service` packages — the activate/deactivate/change-ip/queue handlers, for real.
91+
92+
**So do not write reflection-only tests for the plugin class.** Asserting a handler exists,
93+
is public, is static and takes one parameter passes whether or not the handler works; three
94+
production bugs in this fleet were sitting behind assertions of exactly that shape. Older
95+
guidance in this repo that says those methods must not be called predates the harness.
96+
97+
The harness is **additive**: it runs alongside this package's existing tests, and nothing is
98+
deleted to make room for it. Run the whole suite, never `--filter ContractTest` alone — the
99+
contract class primes constants and calls `register_module()`, neither of which can be undone.
100+
101+
See the `plugin-contract-tests` skill for the full workflow, and `docs/testing-harness.md` in
102+
the installer.
103+
82104
## Before Committing
83105

84106
**IMPORTANT:** Before every git commit, you MUST ensure Caliber syncs agent configs with the latest code changes.

0 commit comments

Comments
 (0)