Skip to content

Commit 256da28

Browse files
rubenvdlindeclaude
andcommitted
fix(approval-routes): repair the parafering seam and unshadow the registry endpoint
Three HIGH defects from the rig re-proof. Unlabeled steps wedge every cross-app route (defect 2). The decision-stage schema requires a label, instantiate() wrote '' for steps without one, OpenRegister stored NULL, and the patch recording the FIRST sign-off then 400'd — dossiq's held routes carry no step labels, so the cross-app case always hit it. The schema keeps its requirement (the route timeline displays the label); instantiate() now derives one from the stage type and step number via the shared mapper (labelOf()). The ordering bug the 400 exposed is fixed too: record() writes the stage FIRST and appends the action row after, so a refused stage write no longer leaves an orphan action row per retry — pinned by a no-orphan test on exactly the legacy NULL-label shape. An idempotent post-migration repair step (RepairDecisionStageLabels, runAsSystem, fail-soft) backfills derived labels onto existing NULL-label stages; the orphan action rows are deliberately KEPT — they are the audit record of what the signer did. Conclusions were never announced (defect 3). provenanceOf() resolved the route with findAll(['id' => ...]), and a top-level id filter matches NOTHING in OpenRegister (identity lives in @self), so sourceApp resolved empty, every conclusion read as 'internal route' and dossiq never heard one. The store gains find() (get-by-uuid, RBAC-scoped), and the sweep of the whole class fixed three sites: the announcer's provenance, assertSubjectAccessible() (which refused every valid subject), and MandateDirectory::resolve() (which silently passed every LOCAL mandate as external, skipping the effective/window/delegate checks). AuditLogService's ['uuid' => ...] filter is NOT this class: it goes to OR's AuditTrailMapper where uuid is a real column. Every stateful test fake now returns zero rows for top-level id/uuid filters, like live OR — a fake that resolves the dead form agrees with the caller and cannot fail. The registry endpoint shipped dead (defect 5). decisionTypes#index (GET /api/v1/decision-types) was declared below the api#index wildcard (GET /api/v1/{resource}); Nextcloud matches in declaration order, so the wildcard answered 404 'Unknown resource' and the picker silently fell back to its 13 shipped types. The literal route now precedes the wildcard, and a new contract test loads the real route table and asserts EVERY literal route precedes every wildcard that matches it (verified red on the pre-fix table, naming exactly this route; no other literal is shadowed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 2458edc commit 256da28

16 files changed

Lines changed: 805 additions & 42 deletions

appinfo/info.xml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,19 @@ Vrij en open source onder de EUPL-1.2-licentie.
275275
the same shipped list, so nothing fails closed in the gap.
276276
-->
277277
<step>OCA\Decidiq\Repair\SeedDecisionTypes</step>
278+
<!--
279+
parafering-route-runtime label repair: instantiate() used to
280+
write '' for route steps without labels, OpenRegister stored
281+
NULL, and the required `label` then 400'd the patch recording
282+
every advance — cross-app routes wedged on their first sign-off.
283+
Backfills a derived label (stage type + step number) onto every
284+
stage stored without one, via the engine so the derivation is
285+
the same one instantiate() now uses. Idempotent, and it KEEPS
286+
the orphan approval-action rows the defect appended: they are
287+
the audit record of what the signer did. <post-migration> only —
288+
a fresh install has no stages to repair.
289+
-->
290+
<step>OCA\Decidiq\Migration\RepairDecisionStageLabels</step>
278291
</post-migration>
279292
<install>
280293
<!--

appinfo/routes.php

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,14 @@
243243
['name' => 'health#index', 'url' => '/api/health', 'verb' => 'GET'],
244244
['name' => 'health#status', 'url' => '/api/v1/health', 'verb' => 'GET'],
245245
['name' => 'health#statusOptions', 'url' => '/api/v1/health', 'verb' => 'OPTIONS'],
246+
// Decision-type registry (REQ-DCDH-002) — a LITERAL /api/v1 GET, so it
247+
// MUST precede the api#index wildcard below. Nextcloud matches in
248+
// declaration order; declared after the wildcard (where the rest of the
249+
// integration-hub group sits) the wildcard swallowed it and answered
250+
// 404 "Unknown resource", so the picker silently fell back to its 13
251+
// shipped types and an admin-added type never appeared. The exact trap
252+
// the write routes document further down.
253+
['name' => 'decisionTypes#index', 'url' => '/api/v1/decision-types', 'verb' => 'GET'],
246254
// CORS preflight for the whole v1 surface — must precede the catch-all GET.
247255
['name' => 'api#preflight', 'url' => '/api/v1/{resource}', 'verb' => 'OPTIONS', 'requirements' => ['resource' => '[a-z\-]+']],
248256
['name' => 'api#preflightItem', 'url' => '/api/v1/{resource}/{id}', 'verb' => 'OPTIONS', 'requirements' => ['resource' => '[a-z\-]+']],
@@ -272,8 +280,9 @@
272280
['name' => 'motionCoauthor#history', 'url' => '/api/motions/{id}/history', 'verb' => 'GET'],
273281

274282
// Integration hub endpoints — create-decision, outcome query, subscribe (REQ-DCDH-002..004).
283+
// decisionTypes#index belongs to this group but is declared ABOVE the
284+
// api#index wildcard, where a literal /api/v1 GET has to live.
275285
// @spec openspec/changes/decidesk-contract-decision-hub/tasks.md#phase-2
276-
['name' => 'decisionTypes#index', 'url' => '/api/v1/decision-types', 'verb' => 'GET'],
277286
['name' => 'integration#createDecision', 'url' => '/api/v1/decisions', 'verb' => 'POST'],
278287
['name' => 'integration#getOutcome', 'url' => '/api/v1/decisions/{id}/outcome', 'verb' => 'GET'],
279288
['name' => 'integration#subscribe', 'url' => '/api/v1/decisions/{id}/subscriptions', 'verb' => 'POST'],
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
<?php
2+
3+
/**
4+
* Decidiq repair step — backfill labels onto NULL-label decision stages.
5+
*
6+
* Cleans up after the parafering-seam label defect: instantiate() wrote '' for
7+
* steps without labels, OpenRegister stored NULL, and the required `label`
8+
* property then 400'd the patch recording every advance — a route held over
9+
* the cross-app seam (dossiq's routes carry no step labels) wedged on its
10+
* FIRST sign-off, appending an orphan approval-action row per attempt.
11+
*
12+
* The backfill itself lives in
13+
* {@see \OCA\Decidiq\Service\DecisionStageLabelRepair}, which derives every
14+
* label through the ONE shared mapper instantiate() uses, so the two cannot
15+
* drift. This step only supplies the repair context. The orphan action rows
16+
* are deliberately KEPT — they are the audit record of what the signer did;
17+
* see that class's docblock.
18+
*
19+
* Idempotent: a stage with a label is left alone, so a re-run repairs nothing.
20+
* Registered under <post-migration> only — a fresh install has no stages yet,
21+
* let alone broken ones.
22+
*
23+
* @category Migration
24+
* @package OCA\Decidiq\Migration
25+
*
26+
* @author Conduction Development Team <info@conduction.nl>
27+
* @copyright 2026 Conduction B.V.
28+
* @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
29+
*
30+
* @link https://conduction.nl
31+
*
32+
* SPDX-FileCopyrightText: 2026 Conduction B.V. <info@conduction.nl>
33+
* SPDX-License-Identifier: EUPL-1.2
34+
*
35+
* @spec openspec/changes/parafering-route-runtime/specs/parafering-route-runtime/spec.md
36+
*/
37+
38+
declare(strict_types=1);
39+
40+
namespace OCA\Decidiq\Migration;
41+
42+
use OCA\Decidiq\Service\SettingsService;
43+
use OCP\Migration\IOutput;
44+
use OCP\Migration\IRepairStep;
45+
use Psr\Container\ContainerInterface;
46+
use Psr\Log\LoggerInterface;
47+
use Throwable;
48+
49+
/**
50+
* Backfills derived labels onto stages stored with a NULL label. Idempotent.
51+
*
52+
* @spec openspec/changes/parafering-route-runtime/specs/parafering-route-runtime/spec.md
53+
*/
54+
class RepairDecisionStageLabels implements IRepairStep {
55+
56+
/**
57+
* Constructor.
58+
*
59+
* The repair service is resolved LAZILY through the container, never
60+
* constructor-injected: its store takes OpenRegister's facade by type, so
61+
* injecting it here would make this step — and with it `occ upgrade` —
62+
* fatal on an instance without openregister.
63+
*
64+
* @param SettingsService $settingsService Reports whether OpenRegister is usable.
65+
* @param ContainerInterface $container Resolves the repair service and OR's ObjectService.
66+
* @param LoggerInterface $logger Records what was repaired.
67+
*/
68+
public function __construct(
69+
private readonly SettingsService $settingsService,
70+
private readonly ContainerInterface $container,
71+
private readonly LoggerInterface $logger,
72+
) {
73+
}//end __construct()
74+
75+
/**
76+
* Repair-step label.
77+
*
78+
* @return string The label.
79+
*
80+
* @spec exclude Trivial repair-step label accessor.
81+
*/
82+
public function getName(): string {
83+
return 'Backfill labels onto Decidiq decision stages stored without one';
84+
}//end getName()
85+
86+
/**
87+
* Run the backfill.
88+
*
89+
* FAIL SOFT: a repair step that throws fails the whole `occ upgrade`, so
90+
* every failure here is logged and reported, never raised. RUN AS SYSTEM:
91+
* a repair step executes with no session, so without the scope
92+
* OpenRegister refuses the patches as 'Anonymous' — same measured trap as
93+
* the sibling migrations.
94+
*
95+
* @param IOutput $output Progress reporting.
96+
*
97+
* @return void
98+
*
99+
* @spec openspec/changes/parafering-route-runtime/specs/parafering-route-runtime/spec.md
100+
*/
101+
public function run(IOutput $output): void {
102+
if ($this->settingsService->isOpenRegisterAvailable() === false) {
103+
$output->info('OpenRegister unavailable — no stages to repair.');
104+
return;
105+
}
106+
107+
try {
108+
$objectService = $this->container->get('OCA\OpenRegister\Service\ObjectService');
109+
$repairService = $this->container->get('OCA\Decidiq\Service\DecisionStageLabelRepair');
110+
} catch (Throwable $e) {
111+
$output->warning('Could not resolve the stage-label repair: ' . $e->getMessage());
112+
return;
113+
}
114+
115+
try {
116+
// One system scope around the whole traversal. The repair's store
117+
// resolves the SAME shared ObjectService instance through the
118+
// container alias, so the scope set here covers its patches.
119+
$repaired = $objectService->runAsSystem(
120+
static fn (): int => (int)$repairService->repair()
121+
);
122+
$output->info(sprintf('Backfilled labels onto %d decision stage(s).', (int)$repaired));
123+
} catch (Throwable $e) {
124+
$this->logger->warning(
125+
'Decidiq: decision-stage label backfill skipped',
126+
['exception' => $e->getMessage()]
127+
);
128+
$output->warning('Decision-stage label backfill skipped: ' . $e->getMessage());
129+
}
130+
}//end run()
131+
}//end class

lib/Service/ApprovalRouteConclusionAnnouncer.php

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,12 @@ private function conclusionOf(array $stages): ?array {
195195
/**
196196
* The provenance pair of the route these stages came from.
197197
*
198+
* Resolved with find(), NOT findAll() with a top-level 'id' filter: that
199+
* filter runs against the object's own JSON properties, where no `id`
200+
* lives, so it matched NOTHING — every route's provenance resolved to
201+
* empty, read as "internal route, no producer waiting", and the
202+
* conclusion was silently skipped. dossiq never heard a single one.
203+
*
198204
* @param string $routeId The route id the stages back-reference.
199205
*
200206
* @return array{0: string, 1: string} [sourceApp, externalReference].
@@ -204,20 +210,15 @@ private function provenanceOf(string $routeId): array {
204210
return ['', ''];
205211
}
206212

207-
$rows = $this->store->findAll(schema: 'approval-route', filters: ['id' => $routeId]);
208-
foreach ($rows as $row) {
209-
$id = (string)($row['id'] ?? ($row['@self']['id'] ?? ''));
210-
if ($id !== $routeId) {
211-
continue;
212-
}
213-
214-
return [
215-
(string)($row['sourceApp'] ?? ''),
216-
(string)($row['externalReference'] ?? ''),
217-
];
213+
$row = $this->store->find(schema: 'approval-route', uuid: $routeId);
214+
if ($row === null) {
215+
return ['', ''];
218216
}
219217

220-
return ['', ''];
218+
return [
219+
(string)($row['sourceApp'] ?? ''),
220+
(string)($row['externalReference'] ?? ''),
221+
];
221222
}//end provenanceOf()
222223

223224
/**

lib/Service/ApprovalRouteService.php

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,11 @@ public function assertSubjectAccessible(string $subject, string $subjectSchema):
120120
throw new RuntimeException('A subject and its schema are required.');
121121
}
122122

123-
$found = $this->store->findAll(schema: $subjectSchema, filters: ['id' => $subject]);
124-
if ($found === []) {
123+
// Resolved with find(), NOT findAll() + a top-level 'id' filter: OpenRegister
124+
// applies filters to the object's own JSON properties and its identity
125+
// lives in `@self`, so the filter form matches nothing and reads as
126+
// "unreachable" for every subject that exists (dossiq#1686's class).
127+
if ($this->store->find(schema: $subjectSchema, uuid: $subject) === null) {
125128
throw new RuntimeException('This subject cannot be reached, so no route may be started on it.');
126129
}
127130
}//end assertSubjectAccessible()
@@ -176,7 +179,12 @@ public function instantiate(array $route, string $subject, string $subjectSchema
176179
// nothing would ever start it.
177180
'status' => $this->mapper->initialStatus(sequence: $sequence, firstSequence: $firstSequence),
178181
'decisionMakerType' => $this->mapper->decisionMakerType(step: $step),
179-
'label' => (string)($step['label'] ?? ''),
182+
// Derived when the step carries none: the schema REQUIRES a
183+
// label, an empty string stores as NULL, and a NULL label
184+
// 400s the patch that records the FIRST sign-off — after
185+
// the action row was already appended. Cross-app routes
186+
// (dossiq) never carry step labels, so they always hit it.
187+
'label' => $this->mapper->labelOf(step: $step, sequence: $sequence),
180188
'mandatory' => (bool)($step['mandatory'] ?? true),
181189
'decision' => $subject,
182190
'assignedPerson' => $this->mapper->assignedPerson(step: $step),
@@ -195,6 +203,15 @@ public function instantiate(array $route, string $subject, string $subjectSchema
195203
/**
196204
* Record an action and advance the route.
197205
*
206+
* THE STAGE WRITE COMES FIRST, the action row after. The old order appended
207+
* the action and THEN patched the stage, so a stage write that failed left
208+
* an orphan action row claiming a sign-off the route never took — and the
209+
* signer's retry appended another. With this order a failed stage write
210+
* throws before any action row exists, so the retry starts clean. The
211+
* inverse gap (stage advanced, action append failed) surfaces loudly: the
212+
* caller gets the throw, and a retry is refused by the guard because the
213+
* next stage names a different actor.
214+
*
198215
* @param array<string, mixed> $action The action: subject, step, actor, action, and optional fields.
199216
*
200217
* @return array<string, mixed> The recorded action.
@@ -222,11 +239,11 @@ public function record(array $action): array {
222239

223240
if ($verb === 'returned') {
224241
$this->guard->assertRequiredFields(verb: $verb, action: $action);
225-
// Validated BEFORE the append: a refused return must leave no
242+
// Validated BEFORE any write: a refused return must leave no
226243
// action row, the same promise every other refusal keeps.
227244
$this->guard->assertReturnTargetValid(action: $action, active: $active);
228-
$recorded = $this->appendAction(action: $action, stage: $active);
229245
$this->applyReturnVerb(action: $action, stages: $stages, active: $active);
246+
$recorded = $this->appendAction(action: $action, stage: $active);
230247
$this->projectTasks(subject: $subject);
231248

232249
return $recorded;
@@ -243,8 +260,8 @@ public function record(array $action): array {
243260
$this->guard->assertVerbFitsStage(stage: $active, verb: $verb);
244261
$this->guard->assertRequiredFields(verb: $verb, action: $action);
245262

246-
$recorded = $this->appendAction(action: $action, stage: $active);
247263
$this->completeAndAdvance(stages: $stages, active: $active, verb: $verb);
264+
$recorded = $this->appendAction(action: $action, stage: $active);
248265
$this->projectTasks(subject: $subject);
249266

250267
return $recorded;

lib/Service/ApprovalRouteStepMapper.php

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,38 @@ public function assignedBody(array $step): string {
149149
return (string)($step['actor'] ?? '');
150150
}//end assignedBody()
151151

152+
/**
153+
* The label a step's stage carries, derived when the step has none.
154+
*
155+
* The decision-stage schema REQUIRES a label: the route timeline displays
156+
* it, so a stage without one is not a valid stage. That requirement is
157+
* kept — the fix lives here instead, because a route held over the
158+
* cross-app seam (dossiq's parafering routes) carries no step labels at
159+
* all. Writing '' for those stored a NULL, and the patch path re-validates
160+
* the whole stage on every advance, so the FIRST sign-off on such a route
161+
* 400'd with "Property 'label' should be type 'string' but is 'null'".
162+
* instantiate() must produce a stage that validates, whoever sent the
163+
* route; a stage the engine itself cannot advance is not a stage.
164+
*
165+
* The fallback is mechanical — the stage type plus the step number, e.g.
166+
* "Endorsement (step 2)" — so the timeline still tells the signer which
167+
* step they are looking at.
168+
*
169+
* @param array<string, mixed> $step The route step.
170+
* @param int $sequence The stage's sequence.
171+
*
172+
* @return string A non-empty label.
173+
* @spec openspec/changes/parafering-route-runtime/specs/parafering-route-runtime/spec.md
174+
*/
175+
public function labelOf(array $step, int $sequence): string {
176+
$label = trim((string)($step['label'] ?? ''));
177+
if ($label !== '') {
178+
return $label;
179+
}
180+
181+
return ucfirst((string)$step['stageType']) . ' (step ' . $sequence . ')';
182+
}//end labelOf()
183+
152184
/**
153185
* The status a stage at this sequence starts in.
154186
*

0 commit comments

Comments
 (0)