docs(adr): amend ADR-130's approval-surface constraint and gate it (#800)

## Description

ADR-130 constraint 3 said `agent_approve` was "doubly unreachable for
non-admins today". It named two surfaces, both behind admin gates — and
**both still are**: `nrllm_runs` is `access => 'admin'`
(`Modules.php:299-301`) and the Playground is too (`:280-282`) with
`denyNonAdmin()` on its resume path.

**So the premise did not expire. A third surface arrived.**
`nrllm_aitasks` is `access => 'user'` and registers
`approve`/`submitInput` (`:342-344`, `:375-381`), so the enumeration
stopped being exhaustive against a module that did not exist when it was
written.

That distinction is the useful part. "The premise expired" reads as bad
luck. **"A constraint that enumerates surfaces is only as durable as the
enumeration"** is a failure mode a reviewer can guard against — and it
is the one that bit. The record even predicted its own trigger ("becomes
exercisable with the editing module") and the prediction left no mark
anywhere a change had to pass.

Which record made it reachable was **established, not assumed**: `git
log -S"nrllm_aitasks"` returns exactly one commit, `2ee16dfd feat:
editor-facing AI Tasks module (ADR-131)`; `git log
-S"AgentRunController::class"` returns that and the earlier admin-only
module; and ADR-131's own Consequences already said it in prose —
*"`agent_approve` through the shared approvals inbox (previously doubly
unreachable for non-admins)"*. The author saw the consequence and wrote
it down. Only the two lifecycle fields were missed. They are written
now, from both ends.

## What the constraint says instead

**Not module access alone.** Both the module tick in `be_groups` and the
grant are required, and neither substitutes for the other (ADR-131
decision 2).

Then three checks on the approve path, in the order `ResumeCoordinator`
evaluates them — which is not the order I first wrote, and the code was
allowed to decide:

1. `mayActOnRun()` (`:205`) — may this actor act on this run at all
2. ADR-172's `requiresSecondApprover` (`:222-226`) — optional, refuses
an approval from the run's own initiator
3. ADR-133's `approverRefusal()` (`:314`) — withholds the release of a
write the approver could not run themselves

Only the third releases the run, because only it had claimed anything.
`submitInput`'s own gates are recorded **beside** the list rather than
merged into it: it has no four-eyes check, and its `submitterRefusal`
examines every pending call rather than only those declaring a write.

## A prediction in prose is what failed, so the fix is a test

`ApprovalSurfaceInventoryTest` holds three lists: which modules register
the approval actions and under which `access`; which classes under
`Classes/Controller` reach `AgentRuntime::approve()`/`submitInput()`;
and that every AJAX route on
`resumeAction`/`submitInputAction`/`approveAction` opens with
`denyNonAdmin()`. It then requires ADR-130 to **name each module the
scan finds**, matched as a whole identifier.

That matching detail is not cosmetic. An earlier draft used
`assertStringContainsString`, and a module named `nrllm_run` satisfied
it through the existing `nrllm_runs` — a substring collision that let a
fourth surface through. A second control found worse: a fourth module
under a *different* controller was invisible to all three assertions and
the suite stayed fully green. Both holes are closed; each guard was then
seen to fail on the defect it exists for.

**The ADR states what the three lists do not cover**, because they do
not cover everything: a class outside `Classes/Controller`, exposed
under a fourth action name, still passes. That was tried and it stayed
green. Saying so is the point — the check exists to make this record's
enumeration falsifiable, not to become a second registry. An earlier
draft of this very paragraph also planted the literal `nrllm_run` into
the text as an example, which would itself have satisfied the naming
check for a module of that name; no example is given now, deliberately.

## Citation rot, found and fixed

Eleven line-anchored citations in ADR-169 and ADR-171 pointed at ranges
this amendment moved. One predated it: **ADR-171 cited
`ResumeCoordinator.php:204` for the `mayActOnRun` check, and that line
is blank** — the four-eyes commit inserted a guard above it four days
ago. The personas record rotted within a day of merging.

All eleven are recomputed. The general problem is filed as #793: 74 such
citations exist and `AdrLifecycleTest` checks `:ref:` targets but not
these.

## Related Issue

Closes #787 — whose own framing this change corrects; the correction is
a comment on the issue.

Also filed while sweeping: #791 (ADR-119 counts twelve submodules, there
are fourteen plus one outside), #792 (ADR-108/094 count 40/41 builtins,
there are 46), #793 (line-anchored citations rot unchecked).

## Type of Change

- [x] Documentation update
- [x] New test (a mechanical gate for this defect class)

## Deliberately not done

ADR-130 **constraint 4** is expired too — `tasks_manage` "does not exist
yet", and the editing module shipped without it. Its amender is ADR-169,
which is `:Status: Proposed`. The lifecycle forbids a proposal writing
`:Amends:`, so that amendment is owed by whichever change accepts
ADR-169. Pre-empting it would break the same rule in the other
direction.

## Gates

`ApprovalSurfaceInventoryTest` OK (4 tests, 79 assertions),
`AdrLifecycleTest` OK (6 tests, 1187 assertions), all three repo checks
exit 0.

`rector -n` at the CI-pinned PHP 8.2 did not run — `.Build` here is
resolved at 8.4 and `platform_check.php` fatals before it starts. One
new PHP file reaches CI without that gate having run locally.

## Checklist

- [x] My code follows the project's coding standards
- [x] I have added a test that proves the gate works, and watched each
assertion fail
- [x] I have updated the documentation accordingly
- [x] `CHANGELOG.md` entry under the existing `### Changed`
- [x] Both ends of the amend link written, verified by
`AdrLifecycleTest`
- [x] My changes generate no new warnings

Author: CybotTM

CHANGELOG.md

@@ -65,6 +65,28 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Changed
 
+- **ADR-130's constraint 3 is amended by ADR-131** (`#787`). The record called
+  `agent_approve` doubly unreachable for non-admins because it named two
+  approval surfaces and both were admin-gated. Both still are: `nrllm_runs` is
+  `access => 'admin'` and the Playground's resume path still calls
+  `denyNonAdmin()`. The enumeration was what went out of date — the editor
+  module `nrllm_aitasks` is `access => 'user'` and registers `approve` and
+  `submitInput`, so a non-admin whose group has that module ticked and who holds
+  the grant decides other users' runs today (both switches are required). The
+  amendment states the three bounds that apply on top of module access — the
+  module tick is required but bounds nothing on its own — in the order
+  `ResumeCoordinator::approve()` evaluates them: `mayActOnRun()`, the opt-in
+  four-eyes refusal (ADR-172), and the approver tool gate (ADR-133) that
+  withholds a write the approver could not run themselves. That list is the
+  approve path; `submitInput` is recorded next to it with the same first and
+  last gate and no four-eyes check (ADR-150). The stale citations ADR-169 and
+  ADR-171 carried into the same records are corrected with it. A new
+  `ApprovalSurfaceInventoryTest` pins which modules register `approve` /
+  `submitInput` under which `access`, which classes under `Classes/Controller`
+  reach the approval runtime, and that ADR-130 names each module it finds — so a
+  new approval module cannot be registered without editing the record. The ADR
+  states what the three lists do not cover. No behaviour changes.
+
 - **One MCP operation now has one timeout, not one per HTTP request** (`#773`,
   ADR-170). A tool call against an MCP server is three requests — the protocol
   handshake, its confirmation, then `tools/call` — and each carried a full

Documentation/Adr/Adr130BackendUserGrants.rst

@@ -4,8 +4,10 @@
 ADR-130: Capability grants for backend users
 ===========================================
 
-:Status: Accepted
+:Status: Accepted (constraint 3 enumerated the approval surfaces and the
+   enumeration was not exhaustive — see :ref:`ADR-131 <adr-131>`)
 :Date: 2026-08-06
+:Amended: 2026-08-06 by :ref:`ADR-131 <adr-131>`
 
 Context
 =======
@@ -75,12 +77,136 @@ Named constraints, each verified against the code
    reachable for grant holders **now**, without any UI — the intended end
    semantics, bounded by the per-user budget. This ADR must not be read as
    "inert until the editing module ships".
-3. **``agent_approve`` is doubly unreachable for non-admins today** — both
-   human approval surfaces sit behind admin gates (the AgentRun module's
-   ``access => admin``, the Playground's ``denyNonAdmin()``). The
-   ``mayActOnRun()`` branch is real, tested code, but only becomes
-   exercisable for non-admins with the editing module. Stated here so
-   nobody reads it as an already-active control.
+3. **``agent_approve`` is reachable for non-admins** — amended, see
+   :ref:`ADR-131 <adr-131>`. This record called the grant doubly
+   unreachable because it named two human approval surfaces and both sat
+   behind admin gates.
+
+   Both gates still hold. ``nrllm_runs`` is still ``'access' => 'admin'``
+   in ``Configuration/Backend/Modules.php``, and the Playground's
+   ``resumeAction()`` and ``submitInputAction()`` still open with
+   ``denyNonAdmin()``. The editing module added a *third* surface that
+   did not exist when this was written: ``nrllm_aitasks`` is
+   ``'access' => 'user'`` and registers ``AgentRunController::approve``
+   and ``submitInput``. A non-admin whose group has that module ticked
+   **and** who holds the grant therefore decides other users' suspended
+   runs today — which is what the grant is for. Both switches are
+   required, and neither substitutes for the other
+   (:ref:`ADR-131 <adr-131>` decision 2): the module tick alone grants no
+   execution, and the grant alone reaches no surface.
+
+   This record named its own expiry trigger. The sentence that stood here
+   said the ``mayActOnRun()`` branch "only becomes exercisable for
+   non-admins with the editing module" — and then the editing module
+   shipped, with nothing carrying that prediction into the change that
+   fired it. So the failure is not that the trigger went unforeseen; it
+   is that a foreseen trigger left no mark anywhere a change had to pass.
+   **A constraint that enumerates surfaces is only as durable as the
+   enumeration.** "Both surfaces are gated" is a claim about a list, and
+   a list stops being complete the moment someone registers the next
+   entry, whether or not the record predicted it. Nothing in the codebase
+   held the list.
+
+   A constraint phrased against the *check* is falsifiable where one
+   phrased against the *inventory* is not — but "no approval action sits
+   outside an admin gate" is not the check to write, because this record
+   deliberately made it false. What distinguishes an intended non-admin
+   surface from an accidental one is the inventory written down and
+   asserted, which
+   ``Tests/Unit/Configuration/ApprovalSurfaceInventoryTest.php`` now
+   does. It holds three lists, and each covers a
+   bounded scope rather than the whole idea of an approval surface. It
+   pins which modules register ``approve``/``submitInput`` and under
+   which ``access``. It pins which classes *under* ``Classes/Controller``
+   reach :php:`AgentRuntime::approve()` or
+   :php:`AgentRuntime::submitInput()`. And it requires every AJAX route
+   targeting one of three named actions — ``resumeAction``,
+   ``submitInputAction``, ``approveAction`` — to open with
+   ``denyNonAdmin()``. It then requires this paragraph to name each
+   module the scan finds, matched as a whole identifier rather than as a
+   substring, so a name that merely extends one already written here
+   does not satisfy it. No example of such a name is given, deliberately:
+   writing one into this paragraph would be enough to satisfy the check
+   for it.
+
+   A fourth surface fails whichever of the three it falls inside.
+   Registered as a module, it fails the module list and this paragraph's
+   naming check — under any controller, not only ``AgentRunController``.
+   Written as a class under ``Classes/Controller`` that calls the
+   runtime, it fails the controller list before it is registered
+   anywhere. Exposed as an AJAX route on one of the three named actions,
+   it fails unless that route guards itself. Whichever list it broke has
+   to be edited; the module door additionally requires this text. Each
+   of those failures was produced on purpose, by registering a fourth
+   surface of that shape and watching the assertion fail, before this
+   paragraph was allowed to claim it.
+
+   What none of the three catches, stated so nobody reads more into them
+   than they hold: an approval-calling class outside ``Classes/Controller``,
+   exposed under an action name that is not one of the three. That
+   combination was tried and the suite stayed green. Widening the scan to
+   every class in ``Classes`` would close it and was not done here — the
+   check exists to make the enumeration in this paragraph falsifiable, not
+   to become a second registry.
+
+   What bounds the grant is therefore not module access alone. Three
+   checks apply to a single approval, in the order
+   :php:`ResumeCoordinator::approve()` evaluates them. **This list
+   describes the approve path only** — ``submitInput`` is the other
+   action this constraint names, and its gates are set out after the
+   list rather than merged into it, because the middle check does not
+   exist there and a merged list of three would be false for it:
+
+   - :php:`AiActorContext::mayActOnRun()` decides whether this actor may
+     act on this run at all — owner, admin, or ``agent_approve`` holder.
+     A refusal is ``RunAccessDeniedException``.
+   - Only on a configuration with ``require_second_approver`` set, an
+     *approval* (never a denial) from the run's own initiator is refused,
+     admin and grant holder alike (:ref:`ADR-172 <adr-172>`). This is
+     opt-in and narrows who may release one particular run
+     (``ResumeCoordinatorFourEyesGateTest``).
+   - :php:`approverRefusal()` resolves the approver's live backend user
+     and asks :php:`ToolCallPolicyInterface::decide()` about every
+     pending call that declares a write, throwing
+     :php:`ApproverNotPermittedException` on a denial
+     (:ref:`ADR-133 <adr-133>`). Unlike the previous check this one is
+     not opt-in: ``approve()`` calls it for every approval decision, and
+     only a *denial* skips it, because a denial executes nothing. Two
+     things scope it — it looks at pending calls that declare a write, so
+     a read-only turn passes it unexamined, and the ``requiresAdmin()``
+     axis it leans on is one the ``tools.dataClassEnforcement: observe``
+     switch does not relax (that switch governs the trust-zone axis
+     only). This is what keeps the grant from becoming a write
+     escalation: the grant admits the *decision*, ADR-133 withholds the
+     *release* of a write the approver could not run themselves
+     (``ResumeCoordinatorApproverGateTest``).
+
+   None of the three consumes the turn. The first two throw above the
+   ``claimResume()`` call, so the run is never claimed and simply stays
+   ``WAITING_FOR_APPROVAL``; the third claimed the state to read it and
+   therefore calls ``release()`` before throwing. ``AgentRunController``
+   turns the latter two into a flash and a redirect, so a run one grant
+   holder may not release stays decidable by someone who may — the two
+   gate tests named above assert exactly that, each re-reading the run
+   and finding it ``WAITING_FOR_APPROVAL``.
+
+   None of the three shrinks the set of runs the grant admits a decision
+   on: that set is ``mayActOnRun()``'s answer, and the other two act on
+   the *outcome* of a decision already admitted. That is why they are
+   named here and amend nothing about the grant itself.
+
+   ``submitInput()`` is gated the same way minus the middle check. It
+   opens with the same :php:`AiActorContext::mayActOnRun()` call and the
+   same ``AGENT_APPROVE`` scope, and it ends at
+   :php:`submitterRefusal()` (:ref:`ADR-150 <adr-150>`), the sibling of
+   :php:`approverRefusal()` against the submitter's live backend user.
+   One rule differs: it asks the policy about **every** pending call
+   rather than only those declaring a write, because an input-requiring
+   tool declares no write — the input and approval markers are mutually
+   exclusive at registration — so a write filter would select nothing
+   and the gate would be decorative. There is no four-eyes check on that
+   path: :ref:`ADR-172 <adr-172>` refuses a self-*approval*, and
+   supplying input is not one.
 4. **``tasks_manage`` does not exist yet.** The list/wizard actions have
    no per-action gate to migrate (they are module-gated only), and the
    trait's JSON 403 body is the wrong shape for HTML module actions. The

Documentation/Adr/Adr131EditorModule.rst

@@ -6,6 +6,8 @@ ADR-131: The editor-facing module
 
 :Status: Accepted
 :Date: 2026-08-06
+:Amends: :ref:`ADR-130 <adr-130>` (constraint 3: this module is a third
+   approval surface its enumeration did not have)
 
 Context
 =======

Documentation/Adr/Adr169RecordManagementUsesTypo3Permissions.rst

@@ -17,7 +17,7 @@ Context
 
 Issue `#691` declines to add a ``MANAGE`` grant case until a management surface
 exists, under :ref:`ADR-130 <adr-130>`'s rule that a case arrives together with
-its enforcement point (``Adr130BackendUserGrants.rst:32-36``). That rule exists
+its enforcement point (``Adr130BackendUserGrants.rst:34-38``). That rule exists
 because :ref:`ADR-023 <adr-023>` shipped backend-group checkboxes that gated
 nothing and :ref:`ADR-117 <adr-117>` had to remove them: "a control that has to
 be labelled 'has no effect' is worse than its absence"
@@ -243,8 +243,8 @@ acceptable grant of ``allowed_groups``.
 -------------------
 
 ``tasks_manage`` is reserved in three places: ADR-130 named constraint 4
-(``Adr130BackendUserGrants.rst:84-88``), ADR-131's "what stays out"
-(``Adr131EditorModule.rst:67-68``) and the enum docblock
+(``Adr130BackendUserGrants.rst:199-203``), ADR-131's "what stays out"
+(``Adr131EditorModule.rst:69-70``) and the enum docblock
 (``Classes/Domain/Enum/BackendUserGrant.php:24-27``). `#691` asks for something
 wider — providers, models, configurations and tasks.
 
@@ -277,7 +277,7 @@ first; old routes keep working through kept identifiers plus
 an actor-scoped run viewport, but the record states that "'Own runs' for editors
 is mostly the approver's view", that agent runs "are currently started from
 admin surfaces", and that the ownership filter "matters the moment any non-admin
-path starts runs" (``Adr131EditorModule.rst:69-72``). A personal run history is
+path starts runs" (``Adr131EditorModule.rst:71-74``). A personal run history is
 what that surface will become, not what it is.
 
 What has changed is the count and the constraint. ADR-119 describes twelve
@@ -287,7 +287,7 @@ registered outside it under ``parent => 'web'``
 file). It sits there for a verified platform reason: the module menu drops every
 top-level module whose own access check fails, so a child of the admin-only
 ``nrllm`` (``:53``) would be invisible to non-admins
-(``Adr131EditorModule.rst:29-33``). A management surface has that same
+(``Adr131EditorModule.rst:31-35``). A management surface has that same
 constraint.
 
 **Recommendation: reopen ADR-119 — recommended here, not done here.** Without

Documentation/Adr/Adr171PersonasTheCodeAlreadyAssumes.rst

@@ -111,8 +111,8 @@ per-task, per-category or per-group scoping, and no task ownership field.
 
 **Code.** ``BackendUserGrant.php:47-52``; the grant branch in
 :php:`AiActorContext::mayActOnRun()` (``:229``) — "deliberately the only scope
-with a grant equivalent" (``Adr130:57-60``); the write side at
-``ResumeCoordinator.php:204`` and ``:634``; the list viewport at
+with a grant equivalent" (``Adr130:59-62``); the write side at
+``ResumeCoordinator.php:205`` and ``:650``; the list viewport at
 ``AgentRunController.php:267``. It sits in no recommended preset: "granting it is
 an explicit trust decision"
 (``Documentation/Administration/Permissions.rst:55-59``).
@@ -216,7 +216,7 @@ one of ours. No third-party integrator is evidenced anywhere.
 
 **Code.** None, by design: "a case is only added TOGETHER with its consumer (a
 grant nothing reads is worse than none)" (``BackendUserGrant.php:24-27``);
-reserved again at ``Adr130:84-88`` and ``Adr131:67-68``.
+reserved again at ``Adr130:199-203`` and ``Adr131:69-70``.
 
 **Human (unvalidated).** Curates the AI catalogue for their own team while keys
 and endpoints stay administrator-only.
@@ -234,14 +234,20 @@ that should be retired.
 Contradictions found
 ====================
 
-**A. A control ADR-130 said was inactive went live, and the record still says it
-is not.** ``Adr130BackendUserGrants.rst:78-83``: "``agent_approve`` is **doubly
-unreachable for non-admins today** — both human approval surfaces sit behind
-admin gates … only becomes exercisable for non-admins with the editing module.
-Stated here so nobody reads it as an already-active control." It shipped:
-``Modules.php:375-381`` registers ``AgentRunController::approve`` and
-``submitInput`` in ``nrllm_aitasks``, ``'access' => 'user'`` (``:344``). A
-non-admin deciding another user's write is an active control. Nothing filed it.
+**A. A control ADR-130 said was inactive went live** — RESOLVED, `#787`.
+ADR-130 constraint 3 read "``agent_approve`` is **doubly unreachable for
+non-admins today** — both human approval surfaces sit behind admin gates …
+only becomes exercisable for non-admins with the editing module. Stated here so
+nobody reads it as an already-active control." It shipped: ``Modules.php:375-381``
+registers ``AgentRunController::approve`` and ``submitInput`` in
+``nrllm_aitasks``, ``'access' => 'user'`` (``:344``). A non-admin who holds the
+grant and whose group has that module ticked decides another user's write today.
+ADR-130 now carries ``:Amended:`` and a rewritten constraint 3
+(``Adr130BackendUserGrants.rst:80-198``), and
+``Tests/Unit/Configuration/ApprovalSurfaceInventoryTest.php`` pins the module
+inventory, so a new approval *module* cannot be registered without ADR-130
+naming it. It does not cover every shape an approval surface could take;
+ADR-130 says which.
 
 **B. One codebase, two answers to "may this person use this model".** The Editor
 Action Center refuses a user outside the configuration's allowed groups —
@@ -253,9 +259,11 @@ module** does not: ``TaskExecutionService.php:63`` calls
 configuration, returned unchecked; the method has no actor to check with. A task
 pinned to a restricted configuration bypasses that restriction.
 
-**C. The bound that justifies the grant is opt-in.** ``Adr130:55-56`` and
-``Permissions.rst:41-43``: "The per-user budget pre-flight … bounds what a grant
-holder can spend." ``BudgetService.php:76-79``: with no ``tx_nrllm_user_budget``
+**C. The bound that justifies the grant is opt-in.** ``Adr130:57-58``: "The
+per-user budget pre-flight … bounds what a grant holder can spend."
+``Permissions.rst:43-45`` states it in its own words, not this one's — "Every run
+is pre-flighted against the user's own usage budget and attributed to them."
+``BudgetService.php:76-79``: with no ``tx_nrllm_user_budget``
 record for that uid, an inactive one, or one with no limit set, the check returns
 ``allowed()``. The budget is per user and opt-in; the grant is per group.
 Ticking ``tasks_use`` for a group without creating budget records ships an
@@ -281,7 +289,7 @@ operator" that is a **non-admin** holding one grant, and says so: "identical to
 "Each case maps to exactly one enforcement point — there is no wildcard"; the
 ``TASKS_USE`` docblock names two. There are nine, across task execution, a module
 index, the Editor Action Center and a record context menu. That is a role-ladder
-rung, which ``Adr130:24`` promised the design would not grow.
+rung, which ``Adr130:26`` promised the design would not grow.
 
 .. _adr-171-invented:
 
@@ -292,12 +300,12 @@ These exist because a gate exists, not because anyone does the job. A reviewer
 should read this section first.
 
 - **The approver, as a distinct person.** The gate is real and tested. But
-  ``Adr130:57-60`` calls it "**deliberately** the only scope with a grant
+  ``Adr130:59-62`` calls it "**deliberately** the only scope with a grant
   equivalent" — a design choice, not an observed org chart. Until
   contradiction A shipped, no non-admin could reach it at all. We built the
   separation before meeting anyone who wants it.
-- **The input submitter.** ``ResumeCoordinator.php:634`` calls the *same*
-  predicate as ``:204``, so submitting is the approver's grant.
+- **The input submitter.** ``ResumeCoordinator.php:650`` calls the *same*
+  predicate as ``:205``, so submitting is the approver's grant.
   :ref:`ADR-150 <adr-150>` reasons about them as a distinct actor with a
   different danger model — and no production tool can produce one:
   ``InputPauseCoverageTest::INPUT_REQUIRING_TOOLS = []`` pins the empty list,
@@ -368,7 +376,7 @@ What TYPO3 gives us and what it does not
 
 TYPO3 gives **roles**: three enforcement primitives, all of which nr_llm uses.
 ``admin`` is the whole default posture. ``user`` on a module means one tick in a
-group's module list and nothing more — ``Adr131:34-39`` records the verified trap
+group's module list and nothing more — ``Adr131:36-41`` records the verified trap
 that ``'user,group'`` denies everyone. ``systemMaintainer`` we never check but
 depend on.