Phase 2 Milestone A batch — T-013 / T-014 / T-015 / T-010 / T-016 (T-030 blocked)#83
Open
cemililik wants to merge 8 commits into
Open
Phase 2 Milestone A batch — T-013 / T-014 / T-015 / T-010 / T-016 (T-030 blocked)#83cemililik wants to merge 8 commits into
cemililik wants to merge 8 commits into
Conversation
Maintainer merged the batch to main 2026-04-25 after two rounds of PR review (commits 5a633cd, d397feb). Updates Status frontmatter on all 6 task files (T-011, T-012, T-025, T-026, T-027, T-029) and rewrites the current.md Active / Recently closed sections to reflect the merge. Refs: T-011, T-012, T-025, T-026, T-027, T-029 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Nightly drift audit comparing each tenant's applied migration head per module against the platform assembly's known head. Drift rows land in a new public-schema platform_migration_drift table; an aggregate MigrationDriftDetectedIntegrationEvent is published at sweep end for ops alerting. - IModuleMigration gains a default-impl GetMigrationHeadsAsync so Phase 1.5 modules without EF migrations are no-op while Phase 2 modules override to wire EF Core's GetMigrations + GetAppliedMigrationsAsync. - Tenant gains nullable LastMigrationStartedAtUtc, stamped by MigrationRunner via direct UPDATE (same pattern as MarkTenantMigrationFailedAsync). Job suppresses drift for tenants whose migration started within the last 2h (rolling-deploy window per migration-orchestration runbook §3.3). - 6 integration tests cover drift recorded, no-drift skipped, suppression in/out of window, terminated-tenant exclusion, and one module throwing without aborting the sweep. AC4 deviation: spec says "via outbox" but no platform-level outbox infrastructure exists yet (existing outbox is per-tenant). Job uses IEventBus directly; drift evidence still lives in platform_migration_drift so worst case is missed alert, not lost data. Follow-up task should introduce a public-schema outbox. Refs: T-013, ADR-0002, ADR-0003 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Daily revocation-list fetcher with RSA-SHA256 signature verification and atomic local cache. Offline / air-gapped deployments skip HTTP entirely and load the bundled file at deploy time. - IRevocationListProvider exposes synchronous IsRevoked(licenseId) for hot-path verification call sites; FileRevocationListProvider keeps an in-memory FrozenSet snapshot, swapped via Interlocked.Exchange. - RevocationListVerifier is pure (no I/O) — signs/verifies over a canonical alphabetical-property JSON projection so issuer and verifier produce byte-identical input. - RevocationListFetchJob fetches with 30s timeout, three jittered exponential retries (1s/5s/30s ±25%), atomically renames stage→cache via File.Move(overwrite:true) (POSIX rename(2) / Windows ReplaceFile). Signature failure: cache retained, LogError fires the on-call alert. - Offline mode skips HTTP, reloads from bundled file only. - 11 tests cover: 6 verifier cases (valid, tampered entries, tampered issued-at, garbage sig, wrong key, empty round-trip) + 5 job cases (persist, tampered-retains-cache, unchanged-no-op, offline skips HTTP, missing public key aborts). Refs: T-015, ADR-0003, ADR-0023 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Polling-loop + SIGHUP-short-circuit hot-reload watcher per ADR-0030. Works uniformly across K8s projected Secret volumes (where inotify silently no-ops), bare-metal Linux, and Windows. Replaces no-op static license loading with a production-ready hot-swap pipeline. - LicenseSnapshot record carries the validated in-memory state (LicenseId, Tier, ValidFromUtc/ValidUntilUtc, Modules, LoadedAtUtc). - ILicenseProvider.Current exposes the snapshot to hot-path callers; InMemoryLicenseProvider swaps via Interlocked.Exchange + Volatile.Read. - ILicenseValidator pluggable; default JsonLicenseValidator parses JSON and checks expiry. Signature verification deferred to NMP track. - LicenseReloadService : BackgroundService runs the polling loop (default 30s, range 5-600 enforced by IValidateOptions). Each tick hashes the file (SHA-256) and short-circuits when unchanged. - PosixSignalRegistration on SIGHUP cancels the polling sleep so operators can `kubectl exec + kill -HUP 1` for instant reload after uploading a new .lic file. - Validation failure: previous snapshot retained, LicenseRefreshFailed IntegrationEvent fires (admin alert path); success emits LicenseRefreshedIntegrationEvent. - 7 tests cover: file-missing no-op, fresh valid file, file replacement (old → new), unchanged file no-op, garbage rollback, expired rollback, bad-then-good recovery. ADR-0030 deviation: polling interval read via IOptions rather than IConfigurationResolver. The resolver is tenant-scoped (ITenantContext required) but this service is tenant-less. Range still enforced; follow-up could extend IConfigurationResolver with a platform-only resolution path. Refs: T-014, ADR-0030, ADR-0003, ADR-0023 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Milestone A scaffolding slice for the cross-module Article 17 sweep per ADR-0026. The indexed-path GDPR handler from T-004 catches entries matching EntityType=Contact AND EntityId=<contactId>; this PR closes the gap for contact references embedded inside other modules' audit JSON payload columns. - IContactReferenceLocator (Nexora.SharedKernel.Abstractions.Audit): module-owned redaction interface. Each module returns redacted JSON for its own payload shape, or null when nothing matches. Avoids the false-positive risk of full-table regex scans rejected in ADR-0026. - ContactPayloadScanJob (Hangfire maintenance queue): iterates every registered locator, applies redaction per audit row, appends a gdpr_erasure_scan summary entry with per-module counts. - ContactGdprDeletedIntegrationEventHandler now enqueues the scan job AFTER its inbox commit so a job failure cannot lose the broker ack. - AuditEntry gains ApplyLocatorRedaction(before, after, changes) for per-column locator output (asymmetric to RedactPayloadForGdpr which replaces all three with one marker). - ContactReferenceLocatorCoverageTests asserts every module emitting a ContactId-bearing IIntegrationEvent ships a locator implementation. Exempt list documents Identity (ID-only events) and Contacts itself. - 5 unit tests (no-locators no-op, locator matches and redacts, only matching module touched, idempotent on re-run) + 2 architecture tests. - SPEC §11.1 + audit-coverage standard §3 updated with the obligation. Deferred to Milestone C: 10M-row performance benchmark (needs seeded fixture harness) and per-module locators (CRM lands with T-030 Portal Extension pilot; Subscription/Finance/Fundraising land with their own phase tasks). Refs: T-010, ADR-0026, ADR-0008 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Backend slice of the progressive-rollout feature-flag service. Phase 2
Milestone C scope; admin React page deferred as a follow-up so this PR
stays focused.
- IFeatureFlagService (Nexora.SharedKernel.Abstractions.FeatureFlags):
IsEnabledAsync(flagKey, userId?, ct), SetAsync, GetAllAsync. The
optional userId param is the dimension future percentage-rollout
backends (LaunchDarkly) hash on; default backend ignores it.
- TenantConfigFeatureFlagService: default impl backed by the existing
ITenantConfiguration. Stores flags under feature_flags.{key}; reads
share storage + invalidation + audit with the rest of tenant config.
Direct DbContext query in GetAllAsync to avoid N round-trips for the
admin listing.
- /api/v1/identity/feature-flags endpoints (GET list, GET single,
PUT toggle) under the new identity.feature_flags.manage permission
(Platform-scope — tenant admins cannot flip flags; preserves the
rollout contract that flags are operator levers, not tenant settings).
- en/tr lockeys added for the new permission display string.
- 7 tests cover absent-flag default-off, set→read round-trip, true→false
flip, prefix-only listing, prefix isolation (bare key not visible),
userId pass-through, empty-key validation.
LaunchDarkly backend wiring is a single-line registration swap — call
sites (IFeatureFlagService) stay unchanged. Filed as a Phase 3 NMP
track follow-up when the SaaS deployment configures the integration.
Refs: T-016, ADR-0023
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Picked up T-030 (Portal Extension CRM pilot) as the last task in the Milestone A sweep and discovered four prerequisite gaps that the task's own Technical notes section says MUST be resolved as precursor work: 1. No CRM backend module exists (entity / DbContext / queries needed to power the lead-history tab + Recent leads widget). 2. No Contacts 360° host shell in nexora-portal — the existing ContactDetailPage lives in nexora-admin which has no ModuleSlot / ModuleTabs scaffolding. 3. Slot-naming collision: T-030 AC asks for `dashboard.sidebar` (dotted) but PortalModuleManifest already declares `dashboard-sidebar` (hyphenated) as a SectionPosition; convergence is an ADR-0017 amendment, not in-PR work. 4. No backend manifest-assembly endpoint / no module.manifest.yaml schema artefact / no validate-manifests CI job. Per CLAUDE.md ADR-first / no-ad-hoc-introduction discipline and the task's own "file a precursor task on the Portal Framework module rather than ad-hoc-introducing them in this PR" note, T-030 is paused at status Blocked. Recommended precursor sequence: T-030d — slot-naming reconciliation + ADR-0017 amendment T-030e — Portal Framework backend module + manifest endpoint + CI T-030b/c — host shell for contact-360° T-030a — CRM backend skeleton T-030 itself resumes once the precursors land. Sweep summary: T-013, T-014, T-015, T-010, T-016 all shipped to In Review across this sweep; T-030 alone is Blocked. Maintainer review needed to (a) approve the In Review work, (b) confirm the precursor sequence + file task IDs before T-030 resumes. Refs: T-030 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (41)
WalkthroughBu PR, GDPR silme işleri için iletişim yükü taraması, lisans yükleme/iptal mekanizması ve platform-seviyesi göç sapması denetimi ekler. İlgili görev belgelerini (T-029–T-030) güncelleştirerek Milestone A kapanışını kaydeder ve Milestone B/C çalışmalarını erteleyerek tamamlama durumlarını belirler. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 dakika Gerekçe: Değişiklikler lisanslama (yük doğrulaması, imza doğrulaması, dosya yoklama), GDPR denetim taraması (modüle ait locator mekanizması), platform göç sapması denetimi (multi-modül karşılaştırması, bastırma penceresi) ve özellik bayrakları (kiracı konfigürasyonu ile destekleme) gibi yoğun mantığa sahip birden fazla bağımsız alan kapsar. Yaklaşık 15 yeni dosya ve 11 test dosyası, değişken soyutlama modellerini ve eşdeğer uç tanımlamalarını doğrulamayı gerektirir. Lisanslama işleri için atomik dosya yazma, işaret işlemesi (SIGHUP) ve uygun hata yayını; göç sapması işi için ki-verilmiş sorgu filtreleme ve bastırma mantığı başta olmak üzere kontrol akışı çok satırlı ve bağlamsal olarak denetim açısından yoğundur. Test kapsamı kapsamlı ancak her alan kendi redaksiyonunu ister. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 2 critical |
🟢 Metrics 226 complexity · 20 duplication
Metric Results Complexity 226 Duplication 20
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several platform-level infrastructure enhancements, including a cross-module PII scan for GDPR compliance, a migration drift detection job, a feature flag service, and a hot-reloading licensing system. The review feedback identifies a potential memory risk in the PII scan job and suggests using a streaming approach to prevent OOM errors. Other recommendations include standardizing on DateTimeOffset for licensing timestamps to avoid ambiguity and hardening the canonicalization logic for signature verification to ensure cross-platform consistency.
Comment on lines
+85
to
+101
| var entries = await dbContext.AuditEntries | ||
| .Where(e => e.TenantId == parameters.TenantId && e.Module == locator.ModuleName) | ||
| .ToListAsync(ct); | ||
|
|
||
| foreach (var entry in entries) | ||
| { | ||
| ct.ThrowIfCancellationRequested(); | ||
| var redactedBefore = locator.Redact(entry.BeforeState, parameters.ContactId); | ||
| var redactedAfter = locator.Redact(entry.AfterState, parameters.ContactId); | ||
| var redactedChanges = locator.Redact(entry.Changes, parameters.ContactId); | ||
|
|
||
| if (redactedBefore is null && redactedAfter is null && redactedChanges is null) | ||
| continue; // nothing to redact in this entry | ||
|
|
||
| entry.ApplyLocatorRedaction(redactedBefore, redactedAfter, redactedChanges); | ||
| redactedThisModule++; | ||
| } |
There was a problem hiding this comment.
Materializing all audit entries for a module via ToListAsync poses a significant memory risk (OOM) for tenants with high activity, even within a single month's partition. Since the scan must iterate through every row to check the JSON payload, it should use a streaming approach via AsAsyncEnumerable() and AsNoTracking() to keep the memory footprint constant for non-matching rows.
var entries = dbContext.AuditEntries
.AsNoTracking()
.Where(e => e.TenantId == parameters.TenantId && e.Module == locator.ModuleName)
.AsAsyncEnumerable();
await foreach (var entry in entries.WithCancellation(ct))
{
var redactedBefore = locator.Redact(entry.BeforeState, parameters.ContactId);
var redactedAfter = locator.Redact(entry.AfterState, parameters.ContactId);
var redactedChanges = locator.Redact(entry.Changes, parameters.ContactId);
if (redactedBefore is null && redactedAfter is null && redactedChanges is null)
continue; // nothing to redact in this entry
entry.ApplyLocatorRedaction(redactedBefore, redactedAfter, redactedChanges);
dbContext.AuditEntries.Update(entry);
redactedThisModule++;
}| public required int Version { get; init; } | ||
|
|
||
| /// <summary>UTC timestamp the bundle was issued. Used to detect stale rolls.</summary> | ||
| public required DateTime IssuedAtUtc { get; init; } |
There was a problem hiding this comment.
Using DateTime for UTC timestamps can lead to ambiguity during serialization/deserialization if the Kind is not correctly preserved (e.g., Unspecified vs Utc). Using DateTimeOffset is the recommended practice for absolute points in time to ensure consistency across different environments and during signature verification.
public required DateTimeOffset IssuedAtUtc { get; init; }| /// <summary>One revoked license — append-only inside the bundle.</summary> | ||
| public sealed record RevokedLicenseEntry( | ||
| string LicenseId, | ||
| DateTime RevokedAtUtc, |
There was a problem hiding this comment.
Consider using DateTimeOffset instead of DateTime to avoid timezone ambiguity and ensure consistent behavior during signature verification.
DateTimeOffset RevokedAtUtc,
Comment on lines
+18
to
+30
| public required DateTime ValidFromUtc { get; init; } | ||
|
|
||
| /// <summary>UTC timestamp the license expires. Past-expiry licenses fail validation.</summary> | ||
| public required DateTime ValidUntilUtc { get; init; } | ||
|
|
||
| /// <summary>Module slugs the license entitles. Empty list means platform-only.</summary> | ||
| public required IReadOnlyList<string> Modules { get; init; } | ||
|
|
||
| /// <summary>UTC timestamp the snapshot was loaded from disk — operators read this for freshness diagnostics.</summary> | ||
| public required DateTime LoadedAtUtc { get; init; } | ||
|
|
||
| /// <summary>True when <c>now</c> is within <c>[ValidFromUtc, ValidUntilUtc]</c>.</summary> | ||
| public bool IsActive(DateTime utcNow) => utcNow >= ValidFromUtc && utcNow <= ValidUntilUtc; |
There was a problem hiding this comment.
Use DateTimeOffset for all absolute timestamps to avoid DateTimeKind ambiguity and ensure correct comparisons regardless of the server's local timezone.
/// <summary>UTC timestamp the license becomes valid.</summary>
public required DateTimeOffset ValidFromUtc { get; init; }
/// <summary>UTC timestamp the license expires. Past-expiry licenses fail validation.</summary>
public required DateTimeOffset ValidUntilUtc { get; init; }
/// <summary>Module slugs the license entitles. Empty list means platform-only.</summary>
public required IReadOnlyList<string> Modules { get; init; }
/// <summary>UTC timestamp the snapshot was loaded from disk — operators read this for freshness diagnostics.</summary>
public required DateTimeOffset LoadedAtUtc { get; init; }
/// <summary>True when <c>now</c> is within <c>[ValidFromUtc, ValidUntilUtc]</c>.</summary>
public bool IsActive(DateTimeOffset utcNow) => utcNow >= ValidFromUtc && utcNow <= ValidUntilUtc;
Comment on lines
+77
to
+84
| var canonical = new | ||
| { | ||
| entries = bundle.Entries | ||
| .Select(e => new { e.LicenseId, e.Reason, RevokedAtUtc = e.RevokedAtUtc.ToUniversalTime() }) | ||
| .ToArray(), | ||
| issuedAtUtc = bundle.IssuedAtUtc.ToUniversalTime(), | ||
| version = bundle.Version, | ||
| }; |
There was a problem hiding this comment.
The canonicalization logic relies on the property order of an anonymous type and default DateTime serialization. This is fragile as System.Text.Json does not guarantee property order for all types, and DateTime formatting can vary (e.g., fractions of seconds). Consider constructing the canonical string manually with sorted keys and a fixed date format (e.g., ISO 8601 with fixed precision) to ensure the signature remains valid across different platforms or library versions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 56
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/analysis/tasks/phase-2/T-011.md (1)
4-9:⚠️ Potential issue | 🟡 MinorDone geçişine eşlik eden status log girdisi eksik.
Satır 84-85'teki "In Review" girdisi, Done geçişini "integration-test follow-up lands with first real EF migration" koşuluna bağlamış. Ancak status log'da
2026-04-25 — Donegirdisi yok ve Phase 2'nin ilk EF migration'ı bu PR'da gönderilmiyor (T-012'nin kendi log'u "scanner currently matches zero files" diyor — hâlâ EF migration üretilmiş değil). İki seçenekten biri uygulanmalı:
- Promosyon koşulu gerçekten karşılanmadıysa T-011'i
In Reviewolarak bırakın; veya- Promosyonun bilinçli olduğunu (ör. "integration test ilk EF migration ile gelecek; T-011 unit kapsamıyla Done sayılıyor") kayıt altına alan bir status log girdisi ekleyin.
Komiteye / arşive bakanlar için bu fark belirleyici; sessiz "Done" geçişi diğer task'lerin (T-029, T-025) status log konvansiyonuyla da uyumsuz.
📝 Önerilen status log eki
**Status:** Done @@ - Status: In Review; awaiting maintainer to promote to Done after integration-test follow-up lands with first real EF migration. + +- 2026-04-25 — Done — maintainer — Promoted on the Phase 2 Milestone A sweep. Integration-test gate explicitly deferred to land alongside the first real EF migration (tracked under the migration-orchestration runbook §2 follow-ups); unit coverage (lock-key determinism + distinctness, failure factory, stack-trace truncation) deemed sufficient for Done given there is currently nothing to migrate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/analysis/tasks/phase-2/T-011.md` around lines 4 - 9, Bu görevin "Done" olarak işaretlenmesiyle ilgili tutarsızlığı gider: ya T-011'in üst kısmındaki durum listesini ve son log girdilerini "In Review" olarak geri al (mevcut "In Review" girdisini koru) ya da status log'a açık bir kayıt ekle ki Done geçişi kasıtlı olsun (ör. "2026-04-25 — Done (promosyon bilinçli; ilk EF migration T-012'de eklenecek; integration-test follow-up planlı)") — düzenlenecek yer T-011 belge başlığındaki Status/Last updated bölümü ve sayfadaki mevcut "In Review" status log girdisi.tests/Nexora.Modules.Audit.Tests/Infrastructure/ContactGdprDeletedIntegrationEventHandlerTests.cs (1)
85-163:⚠️ Potential issue | 🟠 Major
IBackgroundJobClientenqueue davranışı için mock doğrulaması eksik.
backgroundJobClientenjekte ediliyor ancakReceived()veyaDidNotReceive()çağrısıyla doğrulanmıyor. T-010 idempotency sözleşmesinin kritik kısmı — replay'de ikinci birContactPayloadScanJobenqueue edilmemesi — bu test tarafından sessizce denetlenmiyor. Veritabanı seviyesinde audit satırlarını sayıyoruz, fakat arka planda enqueue edilen iş sayısını doğrulayan hiç bir assertion yok.tests/**standardına göre: değişen kod yolları için coverage zorunlu.İlgili testleri (
StartContactImportTests,RequestGdprDeleteTests,StartContactExportTests) inceledik: tümübackgroundJobClient.Received(1).Create(Arg.Any<Hangfire.Common.Job>(), Arg.Any<Hangfire.States.IState>())deseni kullanarak Hangfire enqueue çağrılarını doğruluyor. Aynı pattern burada da uygulanmalı.♻️ Önerilen ek doğrulamalar
// Inbox must contain the event id. var inboxCount = await _dbContext.InboxMessages.CountAsync(m => m.EventId == `@event.EventId`); inboxCount.Should().Be(1); + + // Scan job enqueued exactly once after the first run. + backgroundJobClient.Received(1).Create( + Arg.Any<Hangfire.Common.Job>(), + Arg.Any<Hangfire.States.IState>()); // Act — second run (idempotency) await handler.HandleAsync(`@event`, CancellationToken.None); _dbContext.ChangeTracker.Clear(); var afterSecondRun = await _dbContext.AuditEntries.ToListAsync(); afterSecondRun.Should().HaveCount(4, "idempotent — no new compliance record appended on replay"); var gdprCount = afterSecondRun.Count(e => e.Operation == "gdpr_erasure"); gdprCount.Should().Be(1); + + // Replay must NOT enqueue a second scan job — same idempotency contract. + backgroundJobClient.Received(1).Create( + Arg.Any<Hangfire.Common.Job>(), + Arg.Any<Hangfire.States.IState>()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Nexora.Modules.Audit.Tests/Infrastructure/ContactGdprDeletedIntegrationEventHandlerTests.cs` around lines 85 - 163, The test currently injects backgroundJobClient into ContactGdprDeletedIntegrationEventHandler but never asserts Hangfire enqueue behavior; add assertions to verify ContactPayloadScanJob is enqueued exactly once on first HandleAsync and not enqueued on the second (idempotent) run by using NSubstitute's Received/DidNotReceive pattern against backgroundJobClient.Create(…) similar to other tests; reference backgroundJobClient and ContactGdprDeletedIntegrationEventHandler (and the job type ContactPayloadScanJob or the Hangfire.Job passed) to ensure you assert backgroundJobClient.Received(1).Create(Arg.Any<Job>(), Arg.Any<IState>()) after the first act and backgroundJobClient.Received(1).Create(...) or backgroundJobClient.DidNotReceive().Create(...) appropriately after the second run.src/Nexora.Infrastructure/Migrations/MigrationRunner.cs (1)
90-144: 🧹 Nitpick | 🔵 TrivialLGTM — best-effort stamping mantığı doğru kurulmuş, ama 0-row durumu sessiz geçiyor.
Aynı session üzerinde advisory lock ile birlikte UPDATE çalıştırılması, soft-delete + Terminated guard'ları ve
MarkTenantMigrationFailedAsyncile aynı dar exception ailesi (NpgsqlException/DbException) — modül assembly bağımlılığı yaratmadan istenen davranışı veriyor.Küçük bir gözlem:
ExecuteNonQueryAsync0 etkilenen satır döndürürse (ör.Idmevcut değil veya tenant henüzTerminated) kimse uyarı görmüyor. Drift suppression penceresi sessizce devre dışı kalmış olur. Best-effort olduğu için critical değil, ama operatöre sinyal vermek isteniyorsa bir LogDebug yeterli olur:♻️ Önerilen küçük iyileştirme
cmd.Parameters.AddWithValue("tenantId", tenantGuid); - await cmd.ExecuteNonQueryAsync(ct); + var affected = await cmd.ExecuteNonQueryAsync(ct); + if (affected == 0) + { + logger.LogDebug( + "MigrationRunner: stamp affected 0 rows for tenant {TenantId} (likely Terminated or absent); drift-suppression window will not apply.", + tenantGuid); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Nexora.Infrastructure/Migrations/MigrationRunner.cs` around lines 90 - 144, StampMigrationStartedAsync currently ignores ExecuteNonQueryAsync's return value so a 0-row update (tenant missing/terminated) is silent; after calling cmd.ExecuteNonQueryAsync(ct) in StampMigrationStartedAsync capture the returned affectedRows and if affectedRows == 0 emit a logger.LogDebug (include tenantGuid and brief context that LastMigrationStartedAtUtc was not updated) so operators get a low-importance signal; keep this inside the existing try block and use the same logger and CancellationToken ct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/analysis/tasks/phase-2/T-013.md`:
- Line 53: Create a concrete follow-up task file for the AC4 deviation by adding
a stub task named something like "T-013a — platform_outbox_messages +
OutboxProcessor scope" and update T-013 to reference it; the stub should
identify the work (schema public.platform_outbox_messages, OutboxProcessor
polling/consumption, migration of platform-level publishes from IEventBus to
outbox, testing and rollout plan), mention the current mitigation
(platform_migration_drift retention) and acceptance criteria, and include a link
back from the T-013 status line to T-013a so the recommended follow-up is not
forgotten.
In `@docs/analysis/tasks/phase-2/T-014.md`:
- Line 7: Milestone alanı ile PR başlığı/özeti arasında uyumsuzluk var;
doğrulama yapıp tek doğru kaynağa göre düzeltin: eğer Roadmap/roadmap tek
kaynaktaysa ilgili frontmatter içindeki "Milestone" değerini "A" olarak
düzeltin, aksi halde PR özetindeki batch tanımını "Milestone B" şeklinde
güncelleyin; ayrıca aynı kontrolü T-013 ve T-016 için de uygulayıp hepsinin aynı
kaynakta tutarlı olmasını sağlayın ve değişiklikleri yaptığınız yerdeki
frontmatter "Milestone" alanı ve PR başlığı/özetini eşitleyin.
- Around line 36-38: Create a new ADR that supersedes ADR-0030's "Implementation
shape" paragraph: title it (e.g. "ADR-00xx: Platform-scoped configuration
resolution for tenant-less hosted services — supersedes ADR-0030 §Implementation
shape"), reference ADR-0030 and T-014, state the deviation (LicenseReloadService
uses IOptions<LicenseReloadOptions> instead of IConfigurationResolver), record
the technical rationale (IConfigurationResolver is request-scoped while hosted
LicenseReloadService is a tenant-less singleton) and the enforced constraints
(polling range validated via IValidateOptions), and add a link/back-reference to
DOCUMENTATION_STANDARDS.md and the implementation commit so readers can trace
the change.
In `@docs/analysis/tasks/phase-2/T-016.md`:
- Line 24: The IFeatureFlagService registration currently living in
IdentityModule.ConfigureServices should be moved out of the Identity module:
keep the contract in Nexora.SharedKernel.Abstractions.FeatureFlags, relocate the
concrete TenantConfigFeatureFlagService implementation and its DI registration
into Nexora.Infrastructure (or a new Platform module) so the Identity module
only consumes the shared interface; update IdentityModule to remove any direct
service lifetime/registration and instead resolve IFeatureFlagService from DI;
add a short ADR (ADR-00xx “Feature-flag service ownership”) documenting the
ownership/name and reasoning and reference the moved types (IFeatureFlagService,
TenantConfigFeatureFlagService, IdentityModule.ConfigureServices) in the ADR.
In `@docs/analysis/tasks/phase-2/T-025.md`:
- Around line 4-9: The task T-025 currently shows Status: Done despite the ACs
(see the "Integration test (Testcontainers Postgres): … assert tables dropped +
row deleted" noted around the AC section and the "Deferred" note later) not
being met; either set Status back to "In Review" and open a follow-up task
(e.g., a Testcontainers harness task) to track the missing integration test, or
keep "Done" but append a clear status log entry that documents which specific
ACs are unmet (reference the Integration test AC), cites the "Deferred"
justification, and points to the follow-up tracking task (or T-011) for
auditors; update the status log in T-025.md accordingly so the file reflects the
chosen option.
In `@docs/analysis/tasks/phase-2/T-030.md`:
- Line 134: Update the introductory sentence that currently reads "three
prerequisite gaps" to "four prerequisite gaps" so the preface matches the four
blockers listed under the blocker subheading (blockers 1–4) and the status log
entry that mentions "four blockers documented above"; locate and edit the phrase
in the T-030 document (the preface sentence around "three prerequisite gaps") to
make the count consistent with the blocker list and the status log.
In `@docs/modules/tier-1-core/audit/SPEC.md`:
- Around line 437-449: Add an inline Mermaid sequence diagram to the SPEC.md
section for "11.1 Cross-module PII payload scan" that visually documents the new
sub-flow: participants (ContactGdprDeletedIntegrationEventHandler → indexed
redaction commit → ContactPayloadScanJob [maintenance queue] → each
IContactReferenceLocator redaction → append gdpr_erasure_scan summary), ensuring
it follows DOCUMENTATION_STANDARDS.md (Mermaid inline, not external image) and
include labels for modules/locators and the idempotency note so tests
(ContactReferenceLocatorCoverageTests) and readers can quickly map the sequence
to ContactPayloadScanJob, IContactReferenceLocator implementations, and the
gdpr_erasure_scan summary.
- Line 437: Başlık numara çakışmasını düzeltmek için yeni "### 11.1 Cross-module
PII payload scan (T-010, ADR-0026)" başlığını "11.2" olarak güncelleyin (başlığı
"### 11.2 Cross-module PII payload scan (T-010, ADR-0026)" yapın) ve gerekliyse
diğer 11.x başlıklarla hiyerarşi uyumu için başlık seviyesini "##" olarak
değiştirin; mevcut çakışan başlık referansını bulmak için "[AuditMask]
Attribute..." içeren "## 11.1" başlığını kontrol edin ve içindekiler/tablo
referanslarının (TOC) doğru güncellendiğinden emin olun.
In `@docs/roadmap/current.md`:
- Around line 17-19: Dosyada iki kez aynı "## Pending decisions" başlığı
(özellikle "T-030 precursor ordering" bloğu ve ADR/genel statü bloğu) nedeniyle
anchor çakışması ve belirsizlik oluşuyor; ya üstteki "T-030 precursor ordering"
başlığını "Pending decisions — T-030 precursors" gibi spesifik bir başlıkla
yeniden adlandır veya her iki bloğu tek bir "## Pending decisions" başlığı
altında toplayıp içeriği "### T-030 precursors" ve "### ADR statüleri" gibi alt
başlıklara ayır (başlık metinlerini güncelle ve varsa bu bölümlere dışarıdan
verilen bağlantıların yeni anchor'lara işaret ettiğini doğrula).
In
`@src/Modules/Nexora.Modules.Audit/Infrastructure/Jobs/ContactPayloadScanJob.cs`:
- Around line 85-110: The current ContactPayloadScanJob loads all matching
AuditEntries into memory with ToListAsync and commits redactions with a separate
SaveChangesAsync before calling AppendSummaryAuditEntryAsync, which can leave
redactions persisted without the required summary row; change the flow to stream
and batch-process entries via auditEntries.AsAsyncEnumerable() (or paginated
queries) and apply locator.Redact + entry.ApplyLocatorRedaction in bounded
batches, calling SaveChangesAsync per batch only when changes exist, and wrap
the entire operation including AppendSummaryAuditEntryAsync in a single
IDbContextTransaction (or perform one final SaveChangesAsync inside that
transaction) so the redactions and the gdpr_erasure_scan summary are committed
atomically; ensure cancellation token ct is honored between batches and that
batching logic keeps memory usage bounded to satisfy job time/idempotency
constraints.
In `@src/Modules/Nexora.Modules.Identity/Api/FeatureFlagEndpoints.cs`:
- Around line 25-31: The list endpoint currently returns
ApiEnvelope<IReadOnlyDictionary<string,bool>> while the single-flag endpoints
(MapGet "/{flagKey}" and MapPut "/{flagKey}") return
ApiEnvelope<FeatureFlagDto>, causing inconsistent wire shapes; change the GET
"/" handler (the MapGet("/") lambda that calls IFeatureFlagService.GetAllAsync)
to return a collection of FeatureFlagDto (e.g.
ApiEnvelope<IEnumerable<FeatureFlagDto>> or
ApiEnvelope<IReadOnlyList<FeatureFlagDto>>), mapping each dictionary entry into
FeatureFlagDto so the list and single endpoints share the same DTO shape and
avoid breaking client mappings.
- Around line 34-52: Add request guards in the MapGet and MapPut handlers to
validate the route param and body before calling the service: verify that
flagKey is not null/empty/whitespace in the MapGet handler (before calling
IFeatureFlagService.IsEnabledAsync) and in the MapPut handler, check that
flagKey is valid and that the [FromBody] SetFeatureFlagRequest body is not null
(and that body.Enabled is accessible); if validation fails return a 400 response
using the API envelope with a lockey_ message key (do not hardcode user-facing
strings). Ensure these checks live at the top of the lambdas mapped by
group.MapGet and group.MapPut so IsEnabledAsync and IFeatureFlagService.SetAsync
are only called with validated inputs.
- Around line 19-53: The three minimal lambda endpoints inside
MapFeatureFlagEndpoints (the group.MapGet("/", group.MapGet("/{flagKey}"),
group.MapPut("/{flagKey}")) lack XML documentation and explicit
[ProducesResponseType] declarations; extract each lambda into a named static
handler method (e.g., GetAllFeatureFlags, GetFeatureFlagByKey, SetFeatureFlag)
or apply [EndpointSummary]/[EndpointDescription] attributes, add /// <summary>
and /// <response code="..."> XML docs for each public endpoint and annotate
each handler with all expected status codes (200, 400/422 for validation, 401,
403, 404 for not found/authorization) using [ProducesResponseType(typeof(...),
StatusCodes.StatusXXX)] referencing FeatureFlagDto,
ApiEnvelope<IReadOnlyDictionary<string,bool>>, and SetFeatureFlagRequest as
appropriate so OpenAPI picks up the responses and documentation.
In `@src/Modules/Nexora.Modules.Identity/Domain/Entities/Tenant.cs`:
- Around line 129-138: MarkMigrationStarted içinde geçirilen utcNow
parametresinin DateTimeKind'ını doğrulayın: sadece DateTimeKind.Utc kabul edin;
aksi halde ArgumentException/ArgumentOutOfRangeException fırlatın ve hata
metninde hangi parametrenin yanlış olduğunu belirtin; uygulamada benzer korumayı
kullanan örnek olarak AuditableEntity.MarkAsDeleted desenini takip edin ve sonra
yalnızca doğrulanmış utcNow ile LastMigrationStartedAtUtc'ı atayın (fonksiyon:
MarkMigrationStarted, alan: LastMigrationStartedAtUtc).
In `@src/Modules/Nexora.Modules.Identity/IdentityModule.cs`:
- Around line 121-131: The three platform-scope jobs
(PurgeUninstalledModulesJob,
Infrastructure.Jobs.PlatformAuditMigrationDriftJob.RegisterRecurringSchedule,
and
Nexora.Infrastructure.Licensing.RevocationListFetchJob.RegisterRecurringSchedule)
are being registered inside IdentityModule.ConfigureJobs which creates a module
boundary smell; extract these registrations into a new module (e.g.,
PlatformOpsModule or PlatformSchedulerModule) that implements IModule and owns
platform-level recurring schedules, move the calls to that module's
ConfigureJobs, and update composition root to register the new module instead of
putting platform jobs in IdentityModule; also add a short note to current.md or
create an ADR recording this boundary decision.
In
`@src/Modules/Nexora.Modules.Identity/Infrastructure/Configurations/TenantConfiguration.cs`:
- Around line 24-25: The TenantConfiguration currently maps
LastMigrationStartedAtUtc via builder.Property(t => t.LastMigrationStartedAtUtc)
without specifying the PostgreSQL timezone-aware type; update the mapping in
TenantConfiguration (where builder.Property(t => t.LastMigrationStartedAtUtc) is
defined) to explicitly set the column type to timestamptz (e.g., add
.HasColumnType("timestamptz")) and ensure the nullability matches the model
(nullable if the CLR property is nullable) so the database column preserves
UTC/timezone information.
In
`@src/Modules/Nexora.Modules.Identity/Infrastructure/Jobs/PlatformAuditMigrationDriftJob.cs`:
- Line 43: Replace the hard-coded "maintenance" queue literal with the shared
JobQueues.Maintenance constant to keep queue names consistent and avoid silent
breakage: update the [Queue("maintenance")] attribute on the
PlatformAuditMigrationDriftJob class and the RegisterRecurringSchedule call
(both in PlatformAuditMigrationDriftJob) to use JobQueues.Maintenance, and add
the appropriate using/import if the JobQueues symbol is in another namespace;
mirror the approach used by ContactPayloadScanJob.
In `@src/Nexora.Host/appsettings.json`:
- Around line 26-37: Ek yapılandırma doğrulaması ve taşınabilir varsayılanlar
eksik: ekle RevocationListOptions için bir
IValidateOptions<RevocationListOptions> boot-time validator (örn. validate
OfflineMode==false => PublicKeyPem != null) ve kaydı
RevocationListFetchJob.ExecuteAsync içinde job başlamadan önce başarısız olacak
şekilde yapılandır; ayrıca appsettings.json içindeki
Licensing.Revocations.CacheFilePath ve Licensing.Reload.LicenseFilePath gibi
mutlak Linux yolları yerine taşınabilir bir varsayılan sağlayın (ör. geliştirici
ortamı için appsettings.Development.json ile override veya varsayılanları
göreli/path-temelli hale getirme) ve ayar anahtarlarını (PublicKeyPem,
CacheFilePath, LicenseFilePath) güncelleyerek test/CI geliştirici makinelerinde
yazma hatalarını önleyin.
In `@src/Nexora.Host/DevelopmentSeed.cs`:
- Around line 689-704: The ALTER TABLE statement in DevelopmentSeed.cs that adds
"LastMigrationStartedAtUtc" is executed under a schema-specific search_path and
therefore currently alters identity_tenants in the tenant schema; change the
statement to explicitly target the public schema (i.e., alter
public.identity_tenants) so it matches the production MigrationRunner.cs update
behavior and what PlatformAuditMigrationDriftJob expects; update the ALTER TABLE
entry that references "LastMigrationStartedAtUtc" to use the public schema
prefix to ensure the column is created in public.identity_tenants.
In `@src/Nexora.Infrastructure/FeatureFlags/TenantConfigFeatureFlagService.cs`:
- Around line 71-74: The code in TenantConfigFeatureFlagService currently slices
row.Key with AsSpan(...).ToString() and manually trims quotes from row.Value
before bool.TryParse, which causes an unnecessary allocation and fragile JSON
handling; replace the key slicing with C# range syntax (e.g.,
row.Key[FlagKeyPrefix.Length..]) to avoid the pointless AsSpan/ToString and
parse the stored value via the JSON contract instead of manual trimming by using
JsonSerializer.Deserialize<bool>(row.Value) (or nullable-safe variant) prior to
assigning result[bareKey] so the parsing is robust to stored JSON primitives or
quoted strings.
In `@src/Nexora.Infrastructure/InfrastructureServiceRegistration.cs`:
- Around line 253-265: RevocationListOptions is not validated at startup and
LicenseReloadOptions is missing ValidateOnStart(); create a
RevocationListOptionsValidator implementing
IValidateOptions<Licensing.RevocationListOptions> and register it, and change
the service registration to follow the DomainEventChannelOptions pattern by
using
services.AddOptions<Licensing.RevocationListOptions>().BindConfiguration(configuration.GetSection(Licensing.RevocationListOptions.SectionName)).ValidateOnStart()
(and similarly call .ValidateOnStart() for Licensing.LicenseReloadOptions after
AddLicenseReload()), ensuring AddSingleton registrations for
Licensing.FileRevocationListProvider and IRevocationListProvider remain
unchanged.
In `@src/Nexora.Infrastructure/Licensing/FileRevocationListProvider.cs`:
- Around line 68-85: The code can NRE when bundle.Entries is null before
creating the Snapshot; ensure you guard bundle.Entries before calling
bundle.Entries.Select(...).ToFrozenSet(): check if bundle.Entries is null (or
treat it as empty) immediately after confirming bundle is not null and before
constructing the Snapshot, log an error via logger.LogError (including
_opts.CacheFilePath and bundle.IssuedAtUtc) and return false (or use
Enumerable.Empty for safe conversion), so Snapshot(..) only receives a non-null
sequence; reference bundle.Entries, Volatile.Read(ref _snapshot) and the
Snapshot(...) construction when applying the fix.
- Around line 44-93: ReloadFromDiskAsync is not protected against concurrent
Hangfire executions and also uses a catch (Exception ex) when (...) pattern; add
the [DisableConcurrentExecution] attribute to the RevocationListFetchJob class
so Hangfire will not run multiple instances concurrently, and replace the
combined catch (Exception ex) when (ex is JsonException or IOException) in
ReloadFromDiskAsync with two explicit catches (catch JsonException ex) and
(catch IOException ex) that each log the error and return false; keep the
existing logic that reads the file, compares IssuedAtUtc and uses
Interlocked.Exchange on _snapshot.
In `@src/Nexora.Infrastructure/Licensing/JsonLicenseValidator.cs`:
- Around line 30-58: Deserialize the license JSON using a JsonSerializerOptions
instance with PropertyNameCaseInsensitive = true when calling
JsonSerializer.Deserialize<JsonLicenseFile>(fileBytes.Span) so camelCase keys
map correctly; add a pre-validation that checks file.ValidFromUtc against nowUtc
and return
Task.FromResult(LicenseValidationResult.Invalid("lockey_licensing_validation_not_yet_active",
$"License not active until {file.ValidFromUtc:O}")) when nowUtc <
file.ValidFromUtc; and add XML documentation comments to the public
JsonLicenseFile record and its public properties (LicenseId, Tier, ValidFromUtc,
ValidUntilUtc, Modules) to satisfy the "Add XML documentation on all public
types and methods" guideline.
In `@src/Nexora.Infrastructure/Licensing/LicenseReloadOptions.cs`:
- Around line 17-26: LicenseReloadOptions currently has DataAnnotation
attributes ([Required], [Range]) while the project also implements the same
checks in LicenseReloadOptionsValidator — pick one source of truth and remove
the duplicate. Option A (prefer DI data-annotation validation): delete or
unregister LicenseReloadOptionsValidator, ensure the options are registered with
services.AddOptions<LicenseReloadOptions>().Bind(...).ValidateDataAnnotations().ValidateOnStart()
so [Required]/[Range] are enforced at startup. Option B (prefer custom
validator): remove the [Required] and [Range] attributes from the
LicenseReloadOptions class and keep LicenseReloadOptionsValidator as the single
validator (leave XML comments for docs). Update registrations accordingly so
only the chosen validation path is active.
- Line 29: The XML summary for LicenseReloadOptions contains a typo "Boots-time"
— update the <summary> text in the LicenseReloadOptions XML documentation to
read "Boot-time" (or "Startup") instead so IntelliSense/OpenAPI show the
corrected wording; locate the summary comment above the LicenseReloadOptions
type and replace "Boots-time validation enforcing the ADR-0030 polling-interval
range." with the corrected phrasing.
In `@src/Nexora.Infrastructure/Licensing/LicenseReloadService.cs`:
- Around line 75-101: The loop's temporary "using var interruptCts" can race
with the PosixSignalRegistration handler that captures and later calls Cancel on
that CTS, causing ObjectDisposedException; replace the per-iteration disposable
CTS pattern with a long-lived field-backed CTS that is safely swapped and
disposed after swap, or at minimum ensure the handler calls Cancel inside a
try/catch that ignores ObjectDisposedException; specifically update the code
that creates and swaps _interruptCts (the Interlocked.Exchange/CompareExchange
usage around interruptCts) so you allocate a persistent/new CTS into
_interruptCts atomically, dispose the previous CTS only after the atomic swap,
and/or wrap the handler's call to Interlocked.Exchange(ref _interruptCts,
null)?.Cancel() in a defensive try/catch to swallow ObjectDisposedException
(refer to symbols _interruptCts, Interlocked.Exchange,
Interlocked.CompareExchange, and the PosixSignalRegistration callback).
- Around line 117-141: The validator.ValidateAsync(bytes, ct) call can throw
specific parsing/crypto/format exceptions and these should be handled to avoid
crashing ExecuteAsync and to publish a LicenseRefreshFailedIntegrationEvent;
move the _lastHash assignment so it is updated only after successful validation,
and wrap the ValidateAsync(bytes, ct) call in a try block that catches named
exceptions (e.g., JsonException, CryptographicException, FormatException,
InvalidOperationException) (avoid catch(Exception)); on any caught exception log
a warning including _opts.LicenseFilePath and the exception, publish
LicenseRefreshFailedIntegrationEvent, and return without crashing the hosted
service.
In `@src/Nexora.Infrastructure/Licensing/RevocationListFetchJob.cs`:
- Line 61: The static field JitterSource = Random.Shared raises a Codacy "weak
random" warning but it's only used for non-crypto jitter in
RevocationListFetchJob; either suppress the warning and document it with a short
comment (e.g., "// not used for cryptography — jitter only") or remove the
indirection entirely by replacing uses of JitterSource with direct calls to
Random.Shared.NextDouble(); update references in RevocationListFetchJob (and the
other occurrence at the same file) accordingly so the code uses Random.Shared
directly or includes the suppression comment/pragma next to the JitterSource
declaration.
- Around line 88-93: Add startup options validation for RevocationListOptions by
implementing IValidateOptions<RevocationListOptions> (e.g., a
RevocationListOptionsValidator) that throws an OptionsValidationException when
OfflineMode is false and PublicKeyPem is null/whitespace; register this
validator with DI so misconfiguration fails at application start rather than
when RevocationListFetchJob runs. Update or remove the runtime check in
RevocationListFetchJob (the string.IsNullOrWhiteSpace(_opts.PublicKeyPem) block)
to avoid duplicate handling once startup validation is in place.
- Around line 152-168: PersistAtomicallyAsync currently leaves tempPath orphaned
if File.Create/JsonSerializer.SerializeAsync/File.Move throws; wrap the
write+move sequence in a try/finally (or try/catch/finally) around the block
that creates and writes to tempPath (references: tempPath, _opts.CacheFilePath,
PersistAtomicallyAsync, File.Create, JsonSerializer.SerializeAsync, File.Move)
and in the finally check File.Exists(tempPath) and attempt File.Delete(tempPath)
(or log any delete failures) unless the move succeeded, so failed attempts do
not leave leftover .tmp files.
- Around line 119-134: The retry loop currently treats all HttpRequestException
as transient, causing wasted retries for non-transient 4xx errors; update the
catch around GetFromJsonAsync<RevocationListBundle> to inspect the
HttpRequestException.StatusCode (from the caught ex in the catch block for
HttpRequestException) and immediately stop retrying for non-transient codes
(anything not 5xx, 408, or 429) by logging an error and rethrowing or breaking
out so the outer logic handles it, while preserving existing retry behavior for
transient codes; reference the HttpClient created by
httpClientFactory.CreateClient(HttpClientName), the call to
GetFromJsonAsync<RevocationListBundle>(_opts.FetchUrl, ct), and the logger used
for attempt logging to implement this short-circuit.
- Around line 26-31: The XML comment claims the download is "fsynced, then
File.Move'd", but the code only calls FileStream.FlushAsync() (see FlushAsync
usage around the write path in RevocationListFetchJob) which does not guarantee
an fsync; fix this by either (A) changing the FileStream creation used to write
the temporary cache file to open with FileOptions.WriteThrough (or otherwise
perform an explicit filesystem sync) so that the data is durable before calling
File.Move, or (B) update the XML docblock at the top (the paragraph describing
atomic file rename) to remove the "fsynced" claim and accurately describe that
the code only FlushAsyncs and relies on the cache rollover strategy; reference
the FileStream construction, FlushAsync call, and the subsequent
File.Move(overwrite: true) rename when making the change.
In `@src/Nexora.Infrastructure/Licensing/RevocationListOptions.cs`:
- Line 21: RevocationListOptions currently hardcodes CacheFilePath to a
POSIX-only path; update the RevocationListOptions.CacheFilePath handling so the
default is platform-aware (e.g., build the path with Path.Combine and
Environment.GetFolderPath(Environment.SpecialFolder.*) for Windows/dev machines)
or else default to null/empty and require/validate an override from
configuration, and update any consumers to handle the new behavior; change the
RevocationListOptions class and its CacheFilePath initialization accordingly.
- Around line 8-52: Add startup validation for RevocationListOptions by
implementing IValidateOptions<RevocationListOptions> (e.g. create
RevocationListOptionsValidator) that checks FetchUrl is non-empty/valid URI when
OfflineMode is false, CacheFilePath points to a writable path or non-empty
string, FetchTimeout is positive, and RetryDelays does not contain TimeSpan.Zero
or negative values; register it in DI with
services.AddSingleton<IValidateOptions<RevocationListOptions>,
RevocationListOptionsValidator>() and ensure options are validated on startup
with services.AddOptions<RevocationListOptions>().ValidateOnStart().
In `@src/Nexora.Infrastructure/Licensing/RevocationListVerifier.cs`:
- Around line 77-87: The canonical byte producer in RevocationListVerifier.cs
uses DateTime.ToUniversalTime() on values with Kind=Unspecified which yields
non-deterministic offsets across issuer/verifier; update the code that builds
the canonical object (the anonymous object using bundle.IssuedAtUtc and
e.RevokedAtUtc) to ensure timestamps are true UTC: either change the domain
types to DateTimeOffset (preferred) or, if keeping DateTime, enforce/validate
Kind explicitly before serialization (e.g., add an AssertUtc helper and call it
for bundle.IssuedAtUtc and each e.RevokedAtUtc), and apply the same fix to
LicenseSnapshot usage so canonical bytes are deterministic across environments.
- Around line 16-25: CanonicalJsonOptions currently relies on runtime defaults
that may change; explicitly pin all serializer-affecting settings on the
CanonicalJsonOptions static (e.g., set PropertyNamingPolicy = null,
DictionaryKeyPolicy = null (already set), PropertyNameCaseInsensitive = false,
Encoder = System.Text.Encodings.Web.JavaScriptEncoder.Default or a chosen
deterministic encoder, JsonNumberHandling =
System.Text.Json.Serialization.JsonNumberHandling.Strict, DefaultIgnoreCondition
= System.Text.Json.Serialization.JsonIgnoreCondition.Never, and any converters
you depend on) so the canonical wire format produced by RevocationListVerifier
remains stable across .NET or global-scope changes; update the
CanonicalJsonOptions initializer to explicitly assign these properties.
- Around line 39-62: The try/catch in RevocationListVerifier's signature
verification path misses ArgumentException thrown by RSA.ImportFromPem, so add
handling for ArgumentException (alongside the existing FormatException and
CryptographicException) and return false on that exception; locate the block
using ComputeCanonicalBytes(bundle), Convert.FromBase64String(bundle.Signature),
RSA.ImportFromPem(publicKeyPem) and the rsa.VerifyData(...) call, and add either
a dedicated catch (ArgumentException) { return false; } or combine it with the
cryptographic catch (e.g., catch (ArgumentException) or a multi-type/when
filter) so all PEM/parse/crypto errors cause the method to return false.
In `@src/Nexora.Infrastructure/Migrations/MigrationDrift.cs`:
- Line 1: Bu dosya (MigrationDrift.cs / MigrationDrift model representing
platform_migration_drift) lives under a path that will be mistaken for EF Core
migrations; move the file out of the Migrations/ folder into a distinct folder
(e.g., MigrationDrift/ or Entities/ or MigrationLogging/) and update its
namespace accordingly (change the namespace declaration that currently uses
Nexora.Infrastructure.Migrations to match the new folder, e.g.,
Nexora.Infrastructure.MigrationDrift or Nexora.Infrastructure.Entities) so it no
longer triggers EF Core migration discovery and avoids confusion with real
migrations.
In `@src/Nexora.Infrastructure/Migrations/MigrationDriftLogDbContext.cs`:
- Around line 34-37: The index definition e.HasIndex(d => new { d.TenantId,
d.DetectedAtUtc }) in MigrationDriftLogDbContext should explicitly mark
DetectedAtUtc as DESC to match the documented hot lookup ORDER BY DetectedAtUtc
DESC; update the index fluent call to include an explicit descending
specification (use the EF Core/Npgsql IsDescending overload with a boolean array
where TenantId is false and DetectedAtUtc is true) so the index intent is
expressed and the query planner can target the DESC ordering.
In `@src/Nexora.SharedKernel/Abstractions/FeatureFlags/IFeatureFlagService.cs`:
- Line 12: The XML <see cref="Configuration.ITenantConfiguration"/> and <see
cref="Configuration.IConfigurationResolver"/> references in IFeatureFlagService
are resolving to the current namespace and causing CS1574; fix by either adding
the missing using for Nexora.SharedKernel.Abstractions.Configuration at the top
of the file or replace those cref attributes with fully-qualified names (e.g.,
Nexora.SharedKernel.Abstractions.Configuration.ITenantConfiguration and
Nexora.SharedKernel.Abstractions.Configuration.IConfigurationResolver) so the
compiler can resolve the referenced types.
In `@src/Nexora.SharedKernel/Abstractions/Licensing/ILicenseValidator.cs`:
- Around line 27-51: The LicenseValidationResult record allows external callers
to set Snapshot and error fields via public init setters, violating the "never
both, never neither" invariant; change the API so only the factories produce
instances: make property setters non-public (e.g., private init) for Snapshot,
ErrorLocalizationKey, and ErrorReason and keep the Valid and Invalid static
factories as the only public constructors, or replace this custom type with the
established Result<LicenseSnapshot> pattern used elsewhere; ensure
LicenseValidationResult (or its replacement) enforces that either Snapshot is
set XOR error fields are set and remove any public mutability that permits
creating invalid states.
In `@src/Nexora.SharedKernel/Abstractions/Licensing/LicenseSnapshot.cs`:
- Around line 17-30: The DateTime properties ValidFromUtc, ValidUntilUtc and
LoadedAtUtc claim to be UTC but don't enforce DateTime.Kind, leading to
incorrect comparisons in IsActive(DateTime utcNow); update the model to either
(a) change these properties to DateTimeOffset (recommended) or (b)
validate/normalize Kind to Utc in the init accessors for ValidFromUtc,
ValidUntilUtc and LoadedAtUtc (throw or convert if Kind != Utc) and ensure
IsActive compares against utcNow.ToUniversalTime() or a DateTimeOffset; also add
a short XML contract note on IsActive about whether the upper bound (<=) is
intentionally inclusive.
In `@src/Nexora.SharedKernel/Abstractions/Licensing/RevocationListBundle.cs`:
- Around line 41-45: The positional record RevokedLicenseEntry lacks XML
documentation required by the coding guideline: either add <param name="...">
XML docs for each positional parameter (LicenseId, RevokedAtUtc, Reason)
explaining formats and semantics (e.g., that LicenseId corresponds to
LicenseSnapshot.LicenseId indexing key and whether Reason is free-text or an
enum-like code) or convert RevokedLicenseEntry to a property-init record/class
and add full XML summaries and <param> tags on its public properties; ensure the
docs explicitly state the expected format/constraints for Reason and the linkage
to LicenseSnapshot.LicenseId to match the documentation style used by
RevocationListBundle.
In `@src/Nexora.SharedKernel/Abstractions/MultiTenancy/IModuleMigration.cs`:
- Around line 26-27: The default implementation of
IModuleMigration.GetMigrationHeadsAsync returns MigrationHeadsReport.Empty which
will hide drift for Phase 2 modules that use EF migrations; update the module
registration/startup path (e.g., where modules are registered or inside
IModule.OnStartupAsync) to detect modules that contain EF migrations (for
example by checking for a Migrations artifact/metadata or the presence of
migration types) and assert that those modules override
IModuleMigration.GetMigrationHeadsAsync; if a module has EF migrations but did
not override GetMigrationHeadsAsync (interface default), throw a
startup/registration exception with a clear message so Phase 2 modules cannot be
silently misconfigured; also update unit tests (e.g., StubModuleMigration,
ThrowingModuleMigration) to cover the new validation.
In `@src/Nexora.SharedKernel/Domain/Events/LicenseRefreshedIntegrationEvent.cs`:
- Around line 30-40: The ErrorReason field of
LicenseRefreshFailedIntegrationEvent is being populated with raw exception text
in JsonLicenseValidator (e.g., the $"Malformed license JSON: {ex.Message}"
call); change that code to pass a fixed, classified string (e.g.,
"MalformedLicenseJson" or "Malformed license JSON") instead of ex.Message so the
integration event contains only the safe classification, while preserving the
full exception via logger.LogWarning(ex, ...) for ops; update the code path in
JsonLicenseValidator where Invalid(...) is called with the exception to use this
fixed string for ErrorReason.
In
`@src/Nexora.SharedKernel/Domain/Events/MigrationDriftDetectedIntegrationEvent.cs`:
- Around line 11-23: The XML <remarks> and the record properties are
inconsistent: MigrationDriftDetectedIntegrationEvent declares TenantCount and
DriftRowCount but the remarks mention a `{tenantCount, moduleCount}` aggregate;
decide which is correct and make them match. If alerts require a module-level
aggregate, add a required int ModuleCount { get; init; } to
MigrationDriftDetectedIntegrationEvent and update any serialization/tests;
otherwise, change the <remarks> text to reference `{tenantCount, driftRowCount}`
so the documentation matches the existing TenantCount and DriftRowCount
properties.
In `@tests/Nexora.Architecture.Tests/ContactReferenceLocatorCoverageTests.cs`:
- Around line 49-50: Test
Modules_PublishingContactIdEvents_RegisterAtLeastOneLocator asserts only that a
type deriving from IContactReferenceLocator exists in the module assembly but
does not verify it is registered in DI; either rename the test to
Modules_PublishingContactIdEvents_DefineAtLeastOneLocator to reflect it only
checks type presence, or change the gate to build a real IServiceCollection/Host
(invoke the module's DI registration code) and assert the service provider can
resolve IContactReferenceLocator so you verify actual DI registration rather
than mere type existence.
- Around line 56-66: Get rid of fragile Assembly.GetTypes() calls that can throw
ReflectionTypeLoadException: wrap each asm.GetTypes() usage (the queries
building event types and locator types that start from moduleAssemblies and the
similar query inside the test
Locators_LiveInModuleAssemblies_NotInSharedKernel_OrAudit) in a safe loader that
catches ReflectionTypeLoadException and falls back to ex.Types.Where(t => t !=
null) (or add a helper GetLoadableTypes(Assembly) and use that), then continue
to filter by typeof(IIntegrationEvent)/HasContactIdProperty/ExtractModulePrefix
and typeof(IContactReferenceLocator) as before so null entries are skipped and
the coverage gate no longer flakes.
In
`@tests/Nexora.Infrastructure.Tests/FeatureFlags/TenantConfigFeatureFlagServiceTests.cs`:
- Around line 110-115: Replace the xUnit Assert.ThrowsAsync usage in the
IsEnabledAsync_ThrowsOnEmptyKey test with FluentAssertions: create an
asynchronous action that calls svc.IsEnabledAsync("") and assert it throws
ArgumentException using FluentAssertions' ThrowAsync<TException>() API (e.g.,
via FluentActions.Invoking or assigning a Func<Task>), keeping the test named
IsEnabledAsync_ThrowsOnEmptyKey and using the existing
CreateService/svc.IsEnabledAsync symbols.
In `@tests/Nexora.Infrastructure.Tests/Licensing/LicenseReloadServiceTests.cs`:
- Around line 157-160: Dispose içinde doğrudan Directory.Delete(_tempDir,
recursive: true) çağrısı IO hatası fırlatırsa temizliği bozar; Dispose metodunu
(sınıf: LicenseReloadServiceTests, üye: Dispose, alan: _tempDir) güncelle ve
Directory.Delete çağrısını try/catch bloğuna alarak IOException ve
UnauthorizedAccessException gibi hataları yakala, kısa bir bekleme + bir iki
deneme ile yeniden dene (best-effort cleanup) ve sonunda hatayı yutarak testler
arası temp dizin birikimini önle.
- Around line 164-175: The test class LicenseReloadServiceTests declares
IDisposable but lacks an implementation; add a public void Dispose() method on
the LicenseReloadServiceTests class that calls Directory.Delete(_tempDir,
recursive: true) to clean up the temporary directory created in the constructor,
ensuring resources are removed after tests run while leaving the existing
WriteLicense method behavior unchanged.
In `@tests/Nexora.Infrastructure.Tests/Licensing/RevocationListFetchJobTests.cs`:
- Around line 37-130: Rename the test methods that currently begin with
"ExecuteAsync_" to reflect the actual SUT method "RunAsync" so they follow the
Method_Scenario_ExpectedResult convention; specifically rename
ExecuteAsync_ValidBundle_PersistsToDisk_AndUpdatesProvider,
ExecuteAsync_TamperedBundle_RetainsExistingCache,
ExecuteAsync_UnchangedBundle_IsNoOp,
ExecuteAsync_OfflineMode_SkipsHttpAndReloadsCache, and
ExecuteAsync_NoPublicKey_AbortsWithoutTouchingCache to start with "RunAsync_"
(keeping the rest of each name intact), update any references or attributes
accordingly, and ensure test class
Nexora.Infrastructure.Tests.Licensing.RevocationListFetchJobTests still compiles
and the tests run.
In
`@tests/Nexora.Modules.Audit.Tests/Infrastructure/Jobs/ContactPayloadScanJobTests.cs`:
- Around line 159-170: The current assertion (firstTotal +
secondTotal).Should().Be(1) can pass for the wrong sequence; instead assert each
pass individually so the second is a no-op: parse summaries into firstTotal and
secondTotal (as already done) and replace the combined sum assertion with two
assertions: firstTotal.Should().Be(1) and secondTotal.Should().Be(0) (using the
existing summaries, firstTotal and secondTotal variables) to ensure the first
run redacted one item and the second run did nothing.
In
`@tests/Nexora.Modules.Identity.Tests/Infrastructure/Jobs/PlatformAuditMigrationDriftJobTests.cs`:
- Around line 58-105: Add a new test that verifies running the migration-drift
job twice produces the expected append-only behavior: call CreateJob(...) with
the same StubModuleMigration and run RunAsync twice (using
PlatformAuditMigrationDriftParams), then assert _driftDb.Drifts contains two
rows for the same tenant/module with the same heads and that
_eventBus.PublishAsync was invoked twice with
MigrationDriftDetectedIntegrationEvent; locate references to CreateJob,
PlatformAuditMigrationDriftParams, _driftDb.Drifts and _eventBus.PublishAsync to
implement the assertions and ensure the test pins the expected append-only
outcome for idempotency regression protection.
---
Outside diff comments:
In `@docs/analysis/tasks/phase-2/T-011.md`:
- Around line 4-9: Bu görevin "Done" olarak işaretlenmesiyle ilgili tutarsızlığı
gider: ya T-011'in üst kısmındaki durum listesini ve son log girdilerini "In
Review" olarak geri al (mevcut "In Review" girdisini koru) ya da status log'a
açık bir kayıt ekle ki Done geçişi kasıtlı olsun (ör. "2026-04-25 — Done
(promosyon bilinçli; ilk EF migration T-012'de eklenecek; integration-test
follow-up planlı)") — düzenlenecek yer T-011 belge başlığındaki Status/Last
updated bölümü ve sayfadaki mevcut "In Review" status log girdisi.
In `@src/Nexora.Infrastructure/Migrations/MigrationRunner.cs`:
- Around line 90-144: StampMigrationStartedAsync currently ignores
ExecuteNonQueryAsync's return value so a 0-row update (tenant
missing/terminated) is silent; after calling cmd.ExecuteNonQueryAsync(ct) in
StampMigrationStartedAsync capture the returned affectedRows and if affectedRows
== 0 emit a logger.LogDebug (include tenantGuid and brief context that
LastMigrationStartedAtUtc was not updated) so operators get a low-importance
signal; keep this inside the existing try block and use the same logger and
CancellationToken ct.
In
`@tests/Nexora.Modules.Audit.Tests/Infrastructure/ContactGdprDeletedIntegrationEventHandlerTests.cs`:
- Around line 85-163: The test currently injects backgroundJobClient into
ContactGdprDeletedIntegrationEventHandler but never asserts Hangfire enqueue
behavior; add assertions to verify ContactPayloadScanJob is enqueued exactly
once on first HandleAsync and not enqueued on the second (idempotent) run by
using NSubstitute's Received/DidNotReceive pattern against
backgroundJobClient.Create(…) similar to other tests; reference
backgroundJobClient and ContactGdprDeletedIntegrationEventHandler (and the job
type ContactPayloadScanJob or the Hangfire.Job passed) to ensure you assert
backgroundJobClient.Received(1).Create(Arg.Any<Job>(), Arg.Any<IState>()) after
the first act and backgroundJobClient.Received(1).Create(...) or
backgroundJobClient.DidNotReceive().Create(...) appropriately after the second
run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02bcbb51-2d79-46cf-869b-b68e2bf4d6d2
📒 Files selected for processing (58)
docs/analysis/tasks/phase-1.5/T-029.mddocs/analysis/tasks/phase-2/T-010.mddocs/analysis/tasks/phase-2/T-011.mddocs/analysis/tasks/phase-2/T-012.mddocs/analysis/tasks/phase-2/T-013.mddocs/analysis/tasks/phase-2/T-014.mddocs/analysis/tasks/phase-2/T-015.mddocs/analysis/tasks/phase-2/T-016.mddocs/analysis/tasks/phase-2/T-025.mddocs/analysis/tasks/phase-2/T-026.mddocs/analysis/tasks/phase-2/T-027.mddocs/analysis/tasks/phase-2/T-030.mddocs/modules/tier-1-core/audit/SPEC.mddocs/roadmap/current.mddocs/standards/audit-coverage.mdsrc/Clients/nexora-admin/src/locales/en/identity.jsonsrc/Clients/nexora-admin/src/locales/tr/identity.jsonsrc/Modules/Nexora.Modules.Audit/Domain/Entities/AuditEntry.cssrc/Modules/Nexora.Modules.Audit/Infrastructure/IntegrationEvents/ContactGdprDeletedIntegrationEventHandler.cssrc/Modules/Nexora.Modules.Audit/Infrastructure/Jobs/ContactPayloadScanJob.cssrc/Modules/Nexora.Modules.Identity/Api/FeatureFlagEndpoints.cssrc/Modules/Nexora.Modules.Identity/Domain/Entities/Tenant.cssrc/Modules/Nexora.Modules.Identity/IdentityModule.cssrc/Modules/Nexora.Modules.Identity/Infrastructure/Configurations/TenantConfiguration.cssrc/Modules/Nexora.Modules.Identity/Infrastructure/Jobs/PlatformAuditMigrationDriftJob.cssrc/Nexora.Host/DevelopmentSeed.cssrc/Nexora.Host/appsettings.jsonsrc/Nexora.Infrastructure/FeatureFlags/TenantConfigFeatureFlagService.cssrc/Nexora.Infrastructure/InfrastructureServiceRegistration.cssrc/Nexora.Infrastructure/Licensing/FileRevocationListProvider.cssrc/Nexora.Infrastructure/Licensing/InMemoryLicenseProvider.cssrc/Nexora.Infrastructure/Licensing/JsonLicenseValidator.cssrc/Nexora.Infrastructure/Licensing/LicenseReloadOptions.cssrc/Nexora.Infrastructure/Licensing/LicenseReloadService.cssrc/Nexora.Infrastructure/Licensing/RevocationListFetchJob.cssrc/Nexora.Infrastructure/Licensing/RevocationListOptions.cssrc/Nexora.Infrastructure/Licensing/RevocationListVerifier.cssrc/Nexora.Infrastructure/Migrations/MigrationDrift.cssrc/Nexora.Infrastructure/Migrations/MigrationDriftLogDbContext.cssrc/Nexora.Infrastructure/Migrations/MigrationRunner.cssrc/Nexora.SharedKernel/Abstractions/Audit/IContactReferenceLocator.cssrc/Nexora.SharedKernel/Abstractions/FeatureFlags/IFeatureFlagService.cssrc/Nexora.SharedKernel/Abstractions/Licensing/ILicenseProvider.cssrc/Nexora.SharedKernel/Abstractions/Licensing/ILicenseValidator.cssrc/Nexora.SharedKernel/Abstractions/Licensing/IRevocationListProvider.cssrc/Nexora.SharedKernel/Abstractions/Licensing/LicenseSnapshot.cssrc/Nexora.SharedKernel/Abstractions/Licensing/RevocationListBundle.cssrc/Nexora.SharedKernel/Abstractions/MultiTenancy/IModuleMigration.cssrc/Nexora.SharedKernel/Domain/Events/LicenseRefreshedIntegrationEvent.cssrc/Nexora.SharedKernel/Domain/Events/MigrationDriftDetectedIntegrationEvent.cstests/Nexora.Architecture.Tests/ContactReferenceLocatorCoverageTests.cstests/Nexora.Infrastructure.Tests/FeatureFlags/TenantConfigFeatureFlagServiceTests.cstests/Nexora.Infrastructure.Tests/Licensing/LicenseReloadServiceTests.cstests/Nexora.Infrastructure.Tests/Licensing/RevocationListFetchJobTests.cstests/Nexora.Infrastructure.Tests/Licensing/RevocationListVerifierTests.cstests/Nexora.Modules.Audit.Tests/Infrastructure/ContactGdprDeletedIntegrationEventHandlerTests.cstests/Nexora.Modules.Audit.Tests/Infrastructure/Jobs/ContactPayloadScanJobTests.cstests/Nexora.Modules.Identity.Tests/Infrastructure/Jobs/PlatformAuditMigrationDriftJobTests.cs
| - **Status:** In Review | ||
| - **Owner:** maintainer (autonomous agent) | ||
| - **Phase:** phase-2 | ||
| - **Milestone:** B |
There was a problem hiding this comment.
Milestone alanı PR kapsamı ile çelişiyor.
Bu dosyada Milestone: B yazılı, ancak PR başlığı/özeti T-014'ü “Phase 2 Milestone A batch” altında ele alıyor. Roadmap’in tek kaynağı olarak hangisi doğruysa diğeri güncellenmeli (ya frontmatter'daki Milestone: A olmalı ya da PR özetindeki batch tanımı düzeltilmeli). Aynı uyumsuzluk T-013 ve T-016 için de söz konusu — taşıma mı yoksa tipo mu olduğu netleştirilmeli.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/analysis/tasks/phase-2/T-014.md` at line 7, Milestone alanı ile PR
başlığı/özeti arasında uyumsuzluk var; doğrulama yapıp tek doğru kaynağa göre
düzeltin: eğer Roadmap/roadmap tek kaynaktaysa ilgili frontmatter içindeki
"Milestone" değerini "A" olarak düzeltin, aksi halde PR özetindeki batch
tanımını "Milestone B" şeklinde güncelleyin; ayrıca aynı kontrolü T-013 ve T-016
için de uygulayıp hepsinin aynı kaynakta tutarlı olmasını sağlayın ve
değişiklikleri yaptığınız yerdeki frontmatter "Milestone" alanı ve PR
başlığı/özetini eşitleyin.
Comment on lines
36
to
+38
| - 2026-04-24 — Not started — [ADR-0030](../../../decisions/0030-license-hot-reload-mechanism.md) **Accepted**: chose polling loop + SIGHUP short-circuit over pure FileSystemWatcher because `inotify` silently no-ops under K8s projected Secret volumes (the prod deployment target). T-014 is now unblocked — implementation shape locked by the ADR's "Implementation shape" bullets (LicenseReloadService : IHostedService, polling via `platform.license.reload_interval_seconds` resolver key, `PosixSignalRegistration` for SIGHUP, Interlocked.Exchange snapshot swap). AC items in this file remain the correctness bar; see ADR-0030 for the mechanism rationale. | ||
| - 2026-04-25 — In Progress — Implementation started. | ||
| - 2026-04-25 — In Review — Implementation complete; 7 tests pass. **ADR-0030 deviation:** the ADR specifies polling interval resolved via `IConfigurationResolver` under `platform.license.reload_interval_seconds`. Implementation uses `IOptions<LicenseReloadOptions>` instead because `IConfigurationResolver` is request-scoped (resolves the tenant from `ITenantContextAccessor`) and the reload service is a tenant-less singleton hosted service. The 5–600s range is still enforced (via `IValidateOptions<T>`); operators set the interval via `Licensing:Reload:PollingIntervalSeconds` in appsettings. Recommended follow-up: extend `IConfigurationResolver` with a platform-only resolution path, then port this back if the maintainer prefers strict ADR alignment. |
There was a problem hiding this comment.
ADR-0030 sapması yeni bir ADR ile dökümante edilmeli (immutability kuralı).
ADR-0030 “Accepted” durumda ve IConfigurationResolver üzerinden platform.license.reload_interval_seconds çözümünü zorunlu kılıyor. Implementation bunun yerine IOptions<LicenseReloadOptions> kullanıyor — sebep teknik olarak savunulabilir (resolver request-scoped, hosted service singleton/tenant-less). Ancak status log içinde sapma not düşmek yetmez: DOCUMENTATION_STANDARDS.md gereği kabul edilmiş ADR'lar düzenlenmez, supersede eden yeni bir ADR açılmalı (örn. ADR-00xx “Platform-scoped configuration resolution for tenant-less hosted services — supersedes ADR-0030 §Implementation shape”). Aksi halde gelecekteki bir okuyucu için ADR-0030 hâlâ otorite, kod ise sessizce sapmış oluyor.
As per coding guidelines: "If any ADR is needed for significant cross-module technical changes, follow the ADR template and immutability rule: don't edit accepted ADRs—supersede with a new ADR."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/analysis/tasks/phase-2/T-014.md` around lines 36 - 38, Create a new ADR
that supersedes ADR-0030's "Implementation shape" paragraph: title it (e.g.
"ADR-00xx: Platform-scoped configuration resolution for tenant-less hosted
services — supersedes ADR-0030 §Implementation shape"), reference ADR-0030 and
T-014, state the deviation (LicenseReloadService uses
IOptions<LicenseReloadOptions> instead of IConfigurationResolver), record the
technical rationale (IConfigurationResolver is request-scoped while hosted
LicenseReloadService is a tenant-less singleton) and the enforced constraints
(polling range validated via IValidateOptions), and add a link/back-reference to
DOCUMENTATION_STANDARDS.md and the implementation commit so readers can trace
the change.
| - [ ] Tests. | ||
| - [x] `IFeatureFlagService` abstraction with `IsEnabledAsync(flagKey, userId?, ct)` + `SetAsync` + `GetAllAsync`. The optional `userId` parameter is the dimension future percentage-rollout backends can hash on; default backend ignores it. | ||
| - [x] Default implementation: `TenantConfigFeatureFlagService` reads/writes through `ITenantConfiguration` under the `feature_flags.{key}` namespace. | ||
| - [x] DI is set up so a future `LaunchDarkly` backend swaps the registration without touching call sites — the `IFeatureFlagService` registration in `IdentityModule.ConfigureServices` is the single replacement point. |
There was a problem hiding this comment.
Mimari endişe: feature-flag servisi kimlik modülüne ait olmamalı.
IFeatureFlagService registration'ı IdentityModule.ConfigureServices içinde tek değiştirme noktası olarak tanımlanmış. Feature-flag platform-genelinde (cross-cutting) bir kapasitedir; Identity modülünün domain'ine değil. Yarın LaunchDarkly arka ucuna geçişte Identity modülünün build artifact'ı bunu zorlamamalı. Bu yerleşim ADR-0023 ve modül bağımsızlığı ilkesiyle gerilim üretiyor (No direct references to other modules — use SharedKernel interfaces or integration events).
Önerilen alternatif: kontrat Nexora.SharedKernel.Abstractions.FeatureFlags altında zaten var; TenantConfigFeatureFlagService ve registration Nexora.Infrastructure (ya da yeni bir Platform module) içine taşınmalı. Yönetim endpoint'leri kalabilir ama servis hayatı Identity'ye bağlı olmamalı. Sapma önemli olduğu için kısa bir ADR (örn. ADR-00xx “Feature-flag service ownership”) yararlı olur.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/analysis/tasks/phase-2/T-016.md` at line 24, The IFeatureFlagService
registration currently living in IdentityModule.ConfigureServices should be
moved out of the Identity module: keep the contract in
Nexora.SharedKernel.Abstractions.FeatureFlags, relocate the concrete
TenantConfigFeatureFlagService implementation and its DI registration into
Nexora.Infrastructure (or a new Platform module) so the Identity module only
consumes the shared interface; update IdentityModule to remove any direct
service lifetime/registration and instead resolve IFeatureFlagService from DI;
add a short ADR (ADR-00xx “Feature-flag service ownership”) documenting the
ownership/name and reasoning and reference the moved types (IFeatureFlagService,
TenantConfigFeatureFlagService, IdentityModule.ConfigureServices) in the ADR.
Comment on lines
+19
to
+53
| public static void MapFeatureFlagEndpoints(this IEndpointRouteBuilder endpoints) | ||
| { | ||
| var group = endpoints.MapGroup("/feature-flags") | ||
| .RequireAuthorization("identity.feature_flags.manage"); | ||
|
|
||
| // GET /feature-flags — list every flag set on the current tenant. | ||
| group.MapGet("/", async ( | ||
| IFeatureFlagService flags, | ||
| CancellationToken ct) => | ||
| { | ||
| var all = await flags.GetAllAsync(ct); | ||
| return Results.Ok(ApiEnvelope<IReadOnlyDictionary<string, bool>>.Success(all)); | ||
| }); | ||
|
|
||
| // GET /feature-flags/{key} — read a single flag's effective value. | ||
| group.MapGet("/{flagKey}", async ( | ||
| string flagKey, | ||
| IFeatureFlagService flags, | ||
| CancellationToken ct) => | ||
| { | ||
| var enabled = await flags.IsEnabledAsync(flagKey, userId: null, ct); | ||
| return Results.Ok(ApiEnvelope<FeatureFlagDto>.Success(new FeatureFlagDto(flagKey, enabled))); | ||
| }); | ||
|
|
||
| // PUT /feature-flags/{key} — set a single flag for the current tenant. | ||
| group.MapPut("/{flagKey}", async ( | ||
| string flagKey, | ||
| [FromBody] SetFeatureFlagRequest body, | ||
| IFeatureFlagService flags, | ||
| CancellationToken ct) => | ||
| { | ||
| await flags.SetAsync(flagKey, body.Enabled, ct); | ||
| return Results.Ok(ApiEnvelope<FeatureFlagDto>.Success(new FeatureFlagDto(flagKey, body.Enabled))); | ||
| }); | ||
| } |
There was a problem hiding this comment.
[ProducesResponseType] ve /// <response> XML doc'ları eksik — DOCUMENTATION_STANDARDS ve CODING_STANDARDS ihlali.
src/Modules/**/Api/** kuralı net: “Tum olasi HTTP status code'lar [ProducesResponseType] ile belirtilmeli” ve “/// <summary>, /// <response> XML doc'lari olmali”. Üç endpoint de yalnızca Results.Ok(...) dönüyor ve OpenAPI çıktısı bu sebeple eksik üretilecek (200, 400/422 — flagKey doğrulama hatası, 401, 403, 404 — bilinmeyen flag durumu, beklenen response code'lar).
♻️ Önerilen düzeltme (örnek GET /{flagKey} için)
- // GET /feature-flags/{key} — read a single flag's effective value.
- group.MapGet("/{flagKey}", async (
+ // GET /feature-flags/{key} — read a single flag's effective value.
+ group.MapGet("/{flagKey}", async (
string flagKey,
IFeatureFlagService flags,
CancellationToken ct) =>
{
var enabled = await flags.IsEnabledAsync(flagKey, userId: null, ct);
return Results.Ok(ApiEnvelope<FeatureFlagDto>.Success(new FeatureFlagDto(flagKey, enabled)));
- });
+ })
+ .WithName("GetFeatureFlag")
+ .Produces<ApiEnvelope<FeatureFlagDto>>(StatusCodes.Status200OK)
+ .ProducesProblem(StatusCodes.Status400BadRequest)
+ .ProducesProblem(StatusCodes.Status401Unauthorized)
+ .ProducesProblem(StatusCodes.Status403Forbidden);Aynı pattern'i diğer iki endpoint (GET / ve PUT /{flagKey}) için tekrarlayın; ayrıca her endpoint delegate'inin üstüne /// <summary> + /// <response code="200">...</response> XML doc'ları eklenmeli (minimal API'lerde XML doc'ları lambda dışında bir helper method'a taşımak gerekir; ya [EndpointSummary]/[EndpointDescription] attribute'larını kullanın ya da delegate'leri named static method'lara çıkartın).
As per coding guidelines: "XML documentation her public endpoint'te ZORUNLU" ve "Tum olasi HTTP status code'lar [ProducesResponseType] ile belirtilmeli".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Modules/Nexora.Modules.Identity/Api/FeatureFlagEndpoints.cs` around lines
19 - 53, The three minimal lambda endpoints inside MapFeatureFlagEndpoints (the
group.MapGet("/", group.MapGet("/{flagKey}"), group.MapPut("/{flagKey}")) lack
XML documentation and explicit [ProducesResponseType] declarations; extract each
lambda into a named static handler method (e.g., GetAllFeatureFlags,
GetFeatureFlagByKey, SetFeatureFlag) or apply
[EndpointSummary]/[EndpointDescription] attributes, add /// <summary> and ///
<response code="..."> XML docs for each public endpoint and annotate each
handler with all expected status codes (200, 400/422 for validation, 401, 403,
404 for not found/authorization) using [ProducesResponseType(typeof(...),
StatusCodes.StatusXXX)] referencing FeatureFlagDto,
ApiEnvelope<IReadOnlyDictionary<string,bool>>, and SetFeatureFlagRequest as
appropriate so OpenAPI picks up the responses and documentation.
…batch
Documentation
- T-013: filed T-013a (platform_outbox_messages + OutboxProcessor scope)
as the AC4 deviation follow-up; T-013 status log now references it.
T-013a stub task created with schema + DI + processor scope ACs and
a rollout plan.
- T-025: status log entry filed at PR-review time documenting the
unmet Testcontainers integration-test AC; promotion-to-Done was
intentional (shared deferral with T-011/T-026/T-027).
- T-011: status log entry filed similarly — Done was promoted on the
unit-test coverage; Testcontainers regression test moves to the
shared harness when it lands.
- T-030: "three" → "four" prerequisite gaps so the preface matches
the Blockers list and status log.
- T-014 / T-016: appended PR-review notes deferring the requested
superseding ADR + ownership ADR/relocation as separate work
packages (rationale recorded in-line in each task's status log).
- SPEC.md §11.1 collision fixed (renumbered cross-module PII section
to §11.2) + Mermaid sequence diagram added for the scan flow.
- current.md: duplicate "## Pending decisions" headings disambiguated
("— T-030 precursors" / "— ADR statuses").
- README phase-2 task table picks up T-013a.
Code — simple
- PlatformAuditMigrationDriftJob + RevocationListFetchJob: hardcoded
"maintenance" replaced with JobQueues.Maintenance constant.
- LicenseReloadOptions: "Boots-time" → "Startup-time" typo; the
duplicate IValidateOptions implementation removed in favour of
DataAnnotations + ValidateOnStart at registration site.
- IFeatureFlagService: cref errors fixed (added the missing
Configuration namespace import).
- TenantConfiguration.LastMigrationStartedAtUtc maps to timestamptz.
- DevelopmentSeed ALTER TABLE explicitly schema-qualifies
public.identity_tenants to match MigrationRunner.
- MigrationDriftLogDbContext: index marks DetectedAtUtc as descending.
- FeatureFlagEndpoints GET / now returns IReadOnlyList<FeatureFlagDto>
(matches the single-flag DTO shape) and validates flagKey/body before
calling the service; new lockeys added in en/tr.
- ILicenseValidator.LicenseValidationResult setters → private init so
the Valid/Invalid factories are the only public constructors and the
XOR invariant stays intact.
- MigrationDriftDetectedIntegrationEvent <remarks> ↔ properties match
({tenantCount, driftRowCount}, not moduleCount).
- RevokedLicenseEntry positional record gains XML docs per coding
guideline.
- LicenseSnapshot DateTime properties normalise Kind=Utc on init via
AsUtc; IsActive comparisons stay deterministic.
- Tenant.MarkMigrationStarted argument-asserts DateTimeKind.Utc.
Code — medium / safety
- TenantConfigFeatureFlagService.GetAllAsync: range-syntax key trim +
JsonSerializer.Deserialize<bool> instead of manual quote-trim;
malformed rows are skipped, not thrown.
- RevocationListVerifier: serializer options pinned (encoder, naming
policy, number handling, ignore condition) for forward-compat against
.NET upgrades; AsUtc helper canonicalises DateTime.Kind so issuer +
verifier emit byte-identical "Z"-terminated timestamps; catches
ArgumentException too (RSA.ImportFromPem throws this for malformed
PEM labels alongside CryptographicException + FormatException).
- JsonLicenseValidator: PropertyNameCaseInsensitive=true so camelCase
payloads deserialise correctly; ValidFromUtc pre-active check fires
before ValidUntilUtc expiry; XML docs added on JsonLicenseFile fields;
ErrorReason for malformed-JSON is now a fixed classification string
("MalformedLicenseJson") rather than raw exception text — full
exception still flows through logger.LogWarning.
- RevocationListFetchJob: PersistAtomicallyAsync wraps write+move in
try/finally that best-effort deletes orphaned .tmp files; FileStream
opened with FileOptions.WriteThrough so the XML doc's "fsynced" claim
is now honoured (POSIX O_DSYNC / Windows FILE_FLAG_WRITE_THROUGH);
HttpRequestException with non-transient (4xx except 408/429) status
short-circuits the retry loop; JitterSource carries a "not
cryptography" comment; [DisableConcurrentExecution(timeoutInSeconds: 300)]
added.
- FileRevocationListProvider: catch JsonException and IOException
separately for clearer logs; null bundle.Entries guard before
Snapshot construction (no NRE on tampered bundle).
- LicenseReloadService: race fix on _interruptCts disposal — atomic
swap returns the previous CTS, dispose-after-swap, SIGHUP handler
swallows ObjectDisposedException defensively. ValidateAsync wrapped
in try/catch for JsonException / CryptographicException /
FormatException / InvalidOperationException; _lastHash assignment
moved AFTER validation so a transient validator exception doesn't
silently pin the bad bytes as the new baseline.
- ContactPayloadScanJob: streaming + batched (BatchSize=500) loop so
memory stays bounded for large audit tables; per-batch
ChangeTracker.Clear; whole sweep wrapped in
IDbContextTransaction (relational only) so redactions and the
gdpr_erasure_scan summary commit atomically. ORDER BY suppressed on
InMemory provider (its non-generic Comparer fails on DateTimeOffset
even though the type is IComparable).
- MigrationRunner.StampMigrationStartedAsync: emits LogDebug when the
UPDATE matches 0 rows (terminated / soft-deleted / never-provisioned
tenant) so operators see why drift suppression won't engage.
- RevocationListOptions: added IValidateOptions implementation +
AddOptions(...).Bind(...).ValidateOnStart() registration so
misconfigured trust anchor / non-positive timeouts / negative retry
delays fail host boot.
Tests
- RevocationListFetchJobTests: ExecuteAsync_* renamed to RunAsync_*
(matches the SUT entry method).
- LicenseReloadServiceTests.Dispose: best-effort retry loop swallows
IO/UnauthorizedAccessException so a stray cleanup error doesn't
abort the test class.
- TenantConfigFeatureFlagServiceTests: Assert.ThrowsAsync replaced
with FluentAssertions ThrowAsync<T>() to match the project's
assertion style.
- ContactPayloadScanJobTests: idempotency-pass assertion split into
firstTotal=1 / secondTotal=0 instead of the combined sum check.
- PlatformAuditMigrationDriftJobTests: new test pins append-only
behaviour — running the job twice produces two drift rows (same
tenant/module/heads) and two events, guarding against a future
"skip if recorded" optimisation that would lose the second
observation.
- ContactGdprDeletedIntegrationEventHandlerTests: asserts the
background scan job is enqueued exactly once (via NSubstitute on
IBackgroundJobClient.Create) on the first HandleAsync run and stays
at one after the idempotent second run.
- ContactReferenceLocatorCoverageTests: GetLoadableTypes wrapper
catches ReflectionTypeLoadException so a partial-load on one type
doesn't flake the architecture sweep; existing
"_RegisterAtLeastOneLocator" test renamed to
"_DefineAtLeastOneLocator" since it only verifies type presence
(DI-registration assertion is out of scope for arch tests).
Deferred (recorded in task status logs):
- Superseding ADR for ADR-0030 §"Implementation shape" (T-014).
- Feature-flag service ownership ADR + relocation (T-016).
- Platform-jobs split out of IdentityModule into a Platform module.
- MigrationDrift folder rename (EF discovery uses [Migration]
attribute, not folder name; rename is purely cosmetic and would
ripple namespace changes across multiple files).
- IModuleMigration runtime-detection of EF migrations + override
assertion (interface lacks the runtime context to do this without
per-module reflection that's better placed in an architecture test).
Refs: T-010, T-013, T-013a, T-014, T-015, T-016, T-025, T-030
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2 Milestone A autonomous-agent sweep. Five tasks ship to In Review; one (T-030) is paused at Blocked with a precursor sequence filed in its task status log. No regressions across the full backend test suite (37 new tests added, all green; pre-existing Docker/Testcontainers failures unchanged).
Tasks
platform:audit-migration-driftHangfire job8bd645cIEventBuspublish (no platform outbox infra yet); drift evidence still inplatform_migration_drift8d272f3LicenseServicehot-reload watcher8788adeIOptionsinstead of resolver (resolver is tenant-scoped)324eba90bcec0481669a2Test plan
/api/v1/identity/feature-flags(T-016) once a Postgres + Hangfire stack is up.Documentation updates
docs/modules/tier-1-core/audit/SPEC.md§11.1 — cross-module PII scan sectiondocs/standards/audit-coverage.md§3 — locator obligation notedocs/analysis/tasks/phase-2/T-013.md, T-014.md, T-015.md, T-010.md, T-016.md, T-030.md — status logs, AC checkboxes, deviation notesdocs/roadmap/current.md— Active / Prior Active / Blocked / Pending decisions sections🤖 Generated with Claude Code
Summary by CodeRabbit
Sürüm Notları
Yeni Özellikler
Geliştirmeler