fix: harden 042 requirements gate

This commit is contained in:
Ahmed Darrazi 2026-01-10 00:54:40 +01:00
parent a5ef9961b4
commit 85e4bd75f8
7 changed files with 165 additions and 36 deletions

View File

@ -7,6 +7,7 @@
use App\Support\Enums\RelationshipType; use App\Support\Enums\RelationshipType;
use Illuminate\Support\Arr; use Illuminate\Support\Arr;
use Illuminate\Support\Collection; use Illuminate\Support\Collection;
use Illuminate\Support\Facades\Log;
class DependencyExtractionService class DependencyExtractionService
{ {
@ -16,11 +17,12 @@ class DependencyExtractionService
* *
* @param array<string, mixed> $policyData * @param array<string, mixed> $policyData
*/ */
public function extractForPolicyData(InventoryItem $item, array $policyData): void public function extractForPolicyData(InventoryItem $item, array $policyData): array
{ {
$warnings = [];
$edges = collect(); $edges = collect();
$edges = $edges->merge($this->extractAssignedTo($item, $policyData)); $edges = $edges->merge($this->extractAssignedTo($item, $policyData, $warnings));
$edges = $edges->merge($this->extractScopedBy($item, $policyData)); $edges = $edges->merge($this->extractScopedBy($item, $policyData));
// Enforce max 50 outbound edges by priority: assigned_to > scoped_by > others // Enforce max 50 outbound edges by priority: assigned_to > scoped_by > others
@ -57,13 +59,15 @@ public function extractForPolicyData(InventoryItem $item, array $policyData): vo
['metadata', 'updated_at'] ['metadata', 'updated_at']
); );
} }
return $warnings;
} }
/** /**
* @param array<string, mixed> $policyData * @param array<string, mixed> $policyData
* @return Collection<int, array<string, mixed>> * @return Collection<int, array<string, mixed>>
*/ */
private function extractAssignedTo(InventoryItem $item, array $policyData): Collection private function extractAssignedTo(InventoryItem $item, array $policyData, array &$warnings): Collection
{ {
$assignments = Arr::get($policyData, 'assignments'); $assignments = Arr::get($policyData, 'assignments');
if (! is_array($assignments)) { if (! is_array($assignments)) {
@ -93,19 +97,15 @@ private function extractAssignedTo(InventoryItem $item, array $policyData): Coll
], ],
]; ];
} else { } else {
// Unresolved/unknown target → mark missing $warning = [
$edges[] = [ 'type' => 'unsupported_reference',
'tenant_id' => (int) $item->tenant_id, 'policy_id' => (string) ($policyData['id'] ?? $item->external_id),
'source_type' => 'inventory_item',
'source_id' => (string) $item->external_id,
'target_type' => 'missing',
'target_id' => null,
'relationship_type' => RelationshipType::AssignedTo->value,
'metadata' => [
'raw_ref' => $assignment, 'raw_ref' => $assignment,
'last_known_name' => null, 'reason' => 'unsupported_assignment_target_shape',
],
]; ];
$warnings[] = $warning;
Log::info('Unsupported reference shape encountered', $warning);
} }
} }

View File

@ -108,6 +108,7 @@ private function executeRun(InventorySyncRun $run, Tenant $tenant, array $normal
$errors = 0; $errors = 0;
$errorCodes = []; $errorCodes = [];
$hadErrors = false; $hadErrors = false;
$warnings = [];
try { try {
$typesConfig = $this->supportedTypeConfigByType(); $typesConfig = $this->supportedTypeConfigByType();
@ -186,8 +187,11 @@ private function executeRun(InventorySyncRun $run, Tenant $tenant, array $normal
// Extract dependencies if requested in selection // Extract dependencies if requested in selection
$includeDeps = (bool) ($normalizedSelection['include_dependencies'] ?? true); $includeDeps = (bool) ($normalizedSelection['include_dependencies'] ?? true);
if ($includeDeps) { if ($includeDeps) {
$warnings = array_merge(
$warnings,
app(\App\Services\Inventory\DependencyExtractionService::class) app(\App\Services\Inventory\DependencyExtractionService::class)
->extractForPolicyData($item, $policyData); ->extractForPolicyData($item, $policyData)
);
} }
} }
} }
@ -198,7 +202,9 @@ private function executeRun(InventorySyncRun $run, Tenant $tenant, array $normal
'status' => $status, 'status' => $status,
'had_errors' => $hadErrors, 'had_errors' => $hadErrors,
'error_codes' => array_values(array_unique($errorCodes)), 'error_codes' => array_values(array_unique($errorCodes)),
'error_context' => null, 'error_context' => [
'warnings' => array_values($warnings),
],
'items_observed_count' => $observed, 'items_observed_count' => $observed,
'items_upserted_count' => $upserted, 'items_upserted_count' => $upserted,
'errors_count' => $errors, 'errors_count' => $errors,
@ -207,11 +213,14 @@ private function executeRun(InventorySyncRun $run, Tenant $tenant, array $normal
return $run->refresh(); return $run->refresh();
} catch (Throwable $throwable) { } catch (Throwable $throwable) {
$errorContext = $this->safeErrorContext($throwable);
$errorContext['warnings'] = array_values($warnings);
$run->update([ $run->update([
'status' => InventorySyncRun::STATUS_FAILED, 'status' => InventorySyncRun::STATUS_FAILED,
'had_errors' => true, 'had_errors' => true,
'error_codes' => ['unexpected_exception'], 'error_codes' => ['unexpected_exception'],
'error_context' => $this->safeErrorContext($throwable), 'error_context' => $errorContext,
'items_observed_count' => $observed, 'items_observed_count' => $observed,
'items_upserted_count' => $upserted, 'items_upserted_count' => $upserted,
'errors_count' => $errors + 1, 'errors_count' => $errors + 1,

View File

@ -38,11 +38,23 @@
$name = $edge['metadata']['last_known_name'] ?? null; $name = $edge['metadata']['last_known_name'] ?? null;
$targetId = $edge['target_id'] ?? null; $targetId = $edge['target_id'] ?? null;
$display = $name ?: ($targetId ? ("ID: ".substr($targetId,0,6)."") : 'Unknown'); $display = $name ?: ($targetId ? ("ID: ".substr($targetId,0,6)."") : 'Unknown');
$missingTitle = 'Missing target';
if (is_string($name) && $name !== '') {
$missingTitle .= ". Last known: {$name}";
}
$rawRef = $edge['metadata']['raw_ref'] ?? null;
if ($rawRef !== null) {
$encodedRef = json_encode($rawRef);
if (is_string($encodedRef) && $encodedRef !== '') {
$missingTitle .= '. Ref: '.\Illuminate\Support\Str::limit($encodedRef, 200);
}
}
@endphp @endphp
<li class="flex items-center gap-2 text-sm"> <li class="flex items-center gap-2 text-sm">
<span class="fi-badge">{{ $display }}</span> <span class="fi-badge" title="{{ is_string($targetId) ? $targetId : '' }}">{{ $display }}</span>
@if ($isMissing) @if ($isMissing)
<span class="fi-badge fi-badge-danger" title="Missing target">Missing</span> <span class="fi-badge fi-badge-danger" title="{{ $missingTitle }}">Missing</span>
@endif @endif
</li> </li>
@endforeach @endforeach

View File

@ -13,7 +13,7 @@ ## Constitution Gates
- [x] Tenant isolation: all reads/writes tenant-scoped - [x] Tenant isolation: all reads/writes tenant-scoped
- [x] Automation is idempotent & observable: unique key + upsert + run records + stable error codes - [x] Automation is idempotent & observable: unique key + upsert + run records + stable error codes
- [x] Data minimization & safe logging: no secrets/tokens; avoid storing raw payloads outside allowed fields - [x] Data minimization & safe logging: no secrets/tokens; avoid storing raw payloads outside allowed fields
- [ ] No new tables for warnings; warnings persist on InventorySyncRun.error_context.warnings[] (T009) - [x] No new tables for warnings; warnings persist on InventorySyncRun.error_context.warnings[]
## Functional Requirements Coverage ## Functional Requirements Coverage
@ -26,12 +26,12 @@ ## Functional Requirements Coverage
## Non-Functional Requirements Coverage ## Non-Functional Requirements Coverage
- [x] NFR-001 Idempotency: re-running extraction does not create duplicates; updates metadata deterministically - [x] NFR-001 Idempotency: re-running extraction does not create duplicates; updates metadata deterministically
- [ ] NFR-002 Unknown reference shapes handled gracefully: warning recorded in run metadata; does not fail sync; no edge created for unsupported types (T008, T009, T036) - [x] NFR-002 Unknown reference shapes handled gracefully: warning recorded in run metadata; does not fail sync; no edge created for unsupported types
## Tests (Pest) ## Tests (Pest)
- [x] Extraction determinism + unique key (re-run equality) - [x] Extraction determinism + unique key (re-run equality)
- [ ] Missing edges show “Missing” badge and safe tooltip (T019, T021) - [x] Missing edges show “Missing” badge and safe tooltip
- [x] 50-edge limit enforced and truncation behavior is observable (if specified) - [x] 50-edge limit enforced and truncation behavior is observable (if specified)
- [ ] Tenant isolation for queries and UI (T012) - [x] Tenant isolation for queries and UI
- [x] UI smoke: relationship-type filter limits visible edges - [x] UI smoke: relationship-type filter limits visible edges

View File

@ -127,3 +127,62 @@ public function request(string $method, string $path, array $options = []): Grap
$count = InventoryLink::query()->where('tenant_id', $tenant->getKey())->count(); $count = InventoryLink::query()->where('tenant_id', $tenant->getKey())->count();
expect($count)->toBe(50); expect($count)->toBe(50);
}); });
it('persists unsupported reference warnings on the sync run record', function () {
$tenant = Tenant::factory()->create();
$this->app->bind(GraphClientInterface::class, function () {
return new class implements GraphClientInterface
{
public function listPolicies(string $policyType, array $options = []): GraphResponse
{
return new GraphResponse(true, [[
'id' => 'pol-warn-1',
'displayName' => 'Unsupported Assignment Target',
'assignments' => [
['target' => ['filterId' => 'filter-only-no-group']],
],
]]);
}
public function getPolicy(string $policyType, string $policyId, array $options = []): GraphResponse
{
return new GraphResponse(true);
}
public function getOrganization(array $options = []): GraphResponse
{
return new GraphResponse(true);
}
public function applyPolicy(string $policyType, string $policyId, array $payload, array $options = []): GraphResponse
{
return new GraphResponse(true);
}
public function getServicePrincipalPermissions(array $options = []): GraphResponse
{
return new GraphResponse(true);
}
public function request(string $method, string $path, array $options = []): GraphResponse
{
return new GraphResponse(true);
}
};
});
$svc = app(InventorySyncService::class);
$run = $svc->syncNow($tenant, [
'policy_types' => ['deviceConfiguration'],
'categories' => [],
'include_foundations' => false,
'include_dependencies' => true,
]);
$warnings = $run->error_context['warnings'] ?? null;
expect($warnings)->toBeArray()->toHaveCount(1);
expect($warnings[0]['type'] ?? null)->toBe('unsupported_reference');
expect(InventoryLink::query()->where('tenant_id', $tenant->getKey())->count())->toBe(0);
});

View File

@ -3,6 +3,7 @@
use App\Filament\Resources\InventoryItemResource; use App\Filament\Resources\InventoryItemResource;
use App\Models\InventoryItem; use App\Models\InventoryItem;
use App\Models\InventoryLink; use App\Models\InventoryLink;
use App\Models\Tenant;
use Illuminate\Support\Str; use Illuminate\Support\Str;
it('shows zero-state when no dependencies and shows missing badge when applicable', function () { it('shows zero-state when no dependencies and shows missing badge when applicable', function () {
@ -27,10 +28,16 @@
'target_type' => 'missing', 'target_type' => 'missing',
'target_id' => null, 'target_id' => null,
'relationship_type' => 'assigned_to', 'relationship_type' => 'assigned_to',
'metadata' => ['last_known_name' => null], 'metadata' => [
'last_known_name' => 'Ghost Target',
'raw_ref' => ['example' => 'ref'],
],
]); ]);
$this->get($url)->assertOk()->assertSee('Missing'); $this->get($url)
->assertOk()
->assertSee('Missing')
->assertSee('Last known: Ghost Target');
}); });
it('direction filter limits to outbound or inbound', function () { it('direction filter limits to outbound or inbound', function () {
@ -109,3 +116,32 @@
->assertSee('Scoped Target') ->assertSee('Scoped Target')
->assertDontSee('Assigned Target'); ->assertDontSee('Assigned Target');
}); });
it('does not show edges from other tenants (tenant isolation)', function () {
[$user, $tenant] = createUserWithTenant();
$this->actingAs($user);
/** @var InventoryItem $item */
$item = InventoryItem::factory()->create([
'tenant_id' => $tenant->getKey(),
'external_id' => (string) Str::uuid(),
]);
$otherTenant = Tenant::factory()->create();
// Same source_id, but different tenant_id: must not be rendered.
InventoryLink::factory()->create([
'tenant_id' => $otherTenant->getKey(),
'source_type' => 'inventory_item',
'source_id' => $item->external_id,
'target_type' => 'missing',
'target_id' => null,
'relationship_type' => 'assigned_to',
'metadata' => ['last_known_name' => 'Other Tenant Edge'],
]);
$url = InventoryItemResource::getUrl('view', ['record' => $item], tenant: $tenant);
$this->get($url)
->assertOk()
->assertDontSee('Other Tenant Edge');
});

View File

@ -5,6 +5,7 @@
use App\Models\Tenant; use App\Models\Tenant;
use App\Services\Inventory\DependencyExtractionService; use App\Services\Inventory\DependencyExtractionService;
use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Str; use Illuminate\Support\Str;
uses(RefreshDatabase::class); uses(RefreshDatabase::class);
@ -29,8 +30,11 @@
$svc = app(DependencyExtractionService::class); $svc = app(DependencyExtractionService::class);
$svc->extractForPolicyData($item, $policyData); $warnings1 = $svc->extractForPolicyData($item, $policyData);
$svc->extractForPolicyData($item, $policyData); // re-run, should be idempotent $warnings2 = $svc->extractForPolicyData($item, $policyData); // re-run, should be idempotent
expect($warnings1)->toBeArray()->toBeEmpty();
expect($warnings2)->toBeArray()->toBeEmpty();
$edges = InventoryLink::query()->where('tenant_id', $tenant->getKey())->get(); $edges = InventoryLink::query()->where('tenant_id', $tenant->getKey())->get();
expect($edges)->toHaveCount(4); expect($edges)->toHaveCount(4);
@ -43,7 +47,7 @@
expect($tuples->count())->toBe(4); expect($tuples->count())->toBe(4);
}); });
it('handles unsupported references by creating missing edges', function () { it('handles unsupported references by recording warnings (no edges)', function () {
$tenant = Tenant::factory()->create(); $tenant = Tenant::factory()->create();
/** @var InventoryItem $item */ /** @var InventoryItem $item */
@ -59,11 +63,20 @@
], ],
]; ];
$svc = app(DependencyExtractionService::class); Log::spy();
$svc->extractForPolicyData($item, $policyData);
$edge = InventoryLink::query()->first(); $svc = app(DependencyExtractionService::class);
expect($edge)->not->toBeNull();
expect($edge->target_type)->toBe('missing'); $warnings = $svc->extractForPolicyData($item, $policyData);
expect($edge->target_id)->toBeNull(); expect($warnings)->toBeArray()->toHaveCount(1);
expect($warnings[0]['type'] ?? null)->toBe('unsupported_reference');
expect($warnings[0]['policy_id'] ?? null)->toBe($item->external_id);
expect(InventoryLink::query()->count())->toBe(0);
Log::shouldHaveReceived('info')
->withArgs(fn (string $message, array $context) => $message === 'Unsupported reference shape encountered'
&& ($context['type'] ?? null) === 'unsupported_reference'
&& ($context['policy_id'] ?? null) === $item->external_id)
->once();
}); });