rule_owner mapping: failure transperancy + logging configurable

[Imat00]

Fixed

• Owner import with change_action = 'I' remapped all rules without removing first, colliding with idx_rule_owner_removed_is_null_unique — the on_conflict clause only covers pk_rule_owner, where created differs. Still-active pairs are now dropped before inserting, centrally for all sources.
• RunIncremental caught failures, returned true and breaked — a single broken import blocked every import behind it. Failures are now reported and the loop continues.
• A full reinitialize matching no rule aborted before the cleanup, leaving obsolete mappings active and showing an error. An empty result is valid: it now replaces the state and raises an alert.

Added

• AlertCode.RuleOwnerMapping (52) for failed imports, fallback to a full rebuild, an empty mapping result and a deviation from a full recalculation. Duplicates suppressed while an alert is open.
• Config setting ruleOwnerMappingLogLevel (five levels, default Warning) for the per-rule messages. Per-run summary, import failures and alerts are always logged.
• Run history in the config entry ruleOwnerMappingRunHistory, shown at monitoring/rule_owner_mapping: does the incremental mapping produce the same state as a full recalculation, which rules differ, and what caused the difference. No schema change — the previous state comes from the returning of setAllActiveRuleOwnersRemoved.

Not in this PR

• Atomic incremental writes — removal commits before the insert. Self-healing, needs a DB function. Issue filed.
• Locking between UI and middleware — the full reinitialize runs in the UI process, both guards are process local. Issue filed; worth doing before further UI work, the run history it would poll already exists.
• EditOwner.razor triggers a rebuild on owner network or active-state changes without marking it deliberate, so it currently reports as drift. Being reworked separately, together with owners created in the UI never entering the incremental mapping.

closes: #4806, #5244

[tpurschke]

Review round 11 — task/develop-rule_owner-alertAndVisibleChangesPart2

Reviewed with Claude Opus 5 (1M context), model id claude-opus-5[1m], through the fwo-review-pr skill at standard depth. Both passes (correctness/quality and security) were run inline against this skill's checklists.

Scope of this round, as requested: the last commit only — d26fa9a9..372465c8, four files, +85/−7. That commit is the fix for F25 and F26. The rest of the PR was reviewed in rounds 1 to 10 and was not re-swept here; see What this round did not check.

Findings

#	Criticality	Confidence	State	Subject
F4	🟡 medium	★★★ high	☑️ accepted	Run history stored in a config row that weaker roles can read
F7	🟡 medium	★★★ high	☑️ accepted	The full reinitialize materializes the entire rule_owner table in middleware memory
F21	🟡 medium	★★★ high	✅ fixed	The containment guard does not notice a fourth mapping source
F22	🟡 medium	★★★ high	✅ fixed	The guard's substring match cannot tell a top-level predicate from a nested one
F24	🟡 medium	★★★ high	✅ fixed	A deliberate change that had no effect is explained on the page as a dropped source-switch list
F25	🟡 medium	★★★ high	✅ fixed	The pending-change note never expires, so it silences the next real drift instead of its own rebuild
F27	🟡 medium	★★★ high	🟥 new	The run that drops an expired note still tells the operator the incremental mapping missed the change
F20	🔵 low	★★★ high	✅ fixed	The fake's custom-field filter does not mirror the query its comment cites
F23	🔵 low	★★★ high	✅ fixed	The new help page contains trailing whitespace
F26	🔵 low	★★★ high	✅ fixed	RaiseAlert's documented invariant is contradicted by the backlog fallback and by its own test

F1–F3, F5, F6, F8–F19 were verified fixed in rounds 2 to 9 and are dropped here only for the ten-row limit; none of them is touched by the commit reviewed this round.

---

Recommendations

Worth doing

• F27 — the drop decision already exists in TakeOverPendingChanges; carry it onto the run so AlertMappingDrift and the monitoring page can say "a mapping setting saved on date was never applied by a rebuild, so this difference may be its effect" instead of asserting a cause the run has just decided it cannot know. Small, and it closes the last gap in the F25 story rather than opening a new one. Not a merge blocker: the alert is raised on a real difference that does deserve attention, it just names the wrong culprit on one path.

Residual risk — F4 and F7 remain accepted as documented, and the missing lock between the UI process and the middleware on the history config row stays deferred with an issue filed. New this round, small: the expiry window itself is not pinned by a test. RunAsync_ShouldReportDrift_WhenTheSavedChangeWasNeverAppliedForTooLong sits at kPendingChangesMaxAge + 1 minute and the three positive tests sit at age ≈ 0, so nothing exercises a note just inside the window. Both branches are covered, so this is a thin edge rather than a gap. Unchanged from earlier rounds: nothing exercises end to end an importer run driving the incremental mapping, failing and being repaired by the fallback against a real database.

---

New finding

F27 — The run that drops an expired note still tells the operator the incremental mapping missed the change — medium

TakeOverPendingChanges now makes an explicit judgement and writes it to the log:

if (DateTime.UtcNow - history.PendingChangesRecordedAt > kPendingChangesMaxAge)
{
    Log.WriteWarning(kLogMessageTitle, "Dropping the rule_owner mapping change note of " +
        $"{history.PendingChangesRecordedAt:u}: no rebuild applied it within {kPendingChangesMaxAge.TotalDays} days, " +
        "so a difference of this run is judged on its own.");
    return;
}

"Judged on its own" then means IndicatesDrift returns true, and AlertMappingDrift asserts a cause:

await RaiseAlert($"Full rule_owner reinitialize {run.ControlId} added {run.AddedCount} and removed {run.RemovedCount} mappings " +
    $"although no import was pending. The incremental mapping missed these changes. See config key '…' for the affected rules and owners.");

The run is recorded with TriggeredByChange = false and Changes empty, so the monitoring page shows it as Drift in red with nothing named as the cause. Everything that would explain it stayed in the middleware log line above.

I checked which paths actually reach this, because most of them cannot: IndicatesDrift requires DiffMeaningful, so the backlog fallback and the repeated-failure repair are both out — they run with imports pending and the page correctly shows ImportsPending. The one path that reaches it is the manual Recalculate button, and it is the likely one:

• the admin changes the modelled marker, RecordPendingChanges stamps the note, the rebuild fails, they see E5502 and leave it;
• more than seven days later they come back. Saving again records nothing and triggers nothing — the editor compares against the stored settings, which already carry the new marker, and ruleOwnerRebuildPending lives in the browser circuit that is long gone. Recalculate is the only thing that still applies the change;
• that rebuild produces the whole effect of the never-applied marker change, with an empty backlog. The note that explains it is dropped one line earlier, and the operator is told the incremental mapping missed these changes and pointed at the affected rules.

The expiry itself is right — silence is worse than a wrong attribution, which is why F25 asked for the bound. What is wrong is only that the alert and the run keep asserting the attribution the code has just discarded the evidence for. TakeOverPendingChanges holds both the note and the run at that moment, so recording the drop on the run costs one field.

---

F25 and F26 — verified fixed

F25. RuleOwnerMappingRunHistoryData.PendingChangesRecordedAt is stamped by RecordPendingChanges on every save that records a note, checked in TakeOverPendingChanges against the new kPendingChangesMaxAge (7 days), and reset by Store together with the note. I checked the three things that decide whether this actually holds:

• the positive side is still covered — RunAsync_ShouldNotReportDrift_WhenAChangeWasSavedButItsRebuildNeverCompleted and the two neighbouring tests go through RecordPendingChanges rather than seeding raw JSON, so they stamp the note and exercise the non-expired branch; the new RunAsync_ShouldReportDrift_WhenTheSavedChangeWasNeverAppliedForTooLong covers the expired one and asserts all four consequences, including that the note is dropped rather than carried forward;
• an unstamped note fails safe — PendingChangesRecordedAt is JsonIgnore(WhenWritingDefault), so a history written by an intermediate build of this branch deserializes to DateTime.MinValue, which is older than any window and is therefore dropped rather than trusted. That is the right direction, and it needs no upgrade handling because nothing outside this branch has ever written the key;
• the comparison is sound — both sides are UTC (DateTime.UtcNow written, ISO-8601 Z read back as Kind.Utc), and a seven-day window absorbs any clock skew between a separately hosted UI and middleware.

Re-stamping the whole note on a later save is deliberate and documented, so an admin who keeps retrying keeps the note alive; that is the intended reading of "a retried rebuild days later still counts as the one applying the change".

F26. The invariant sentence is replaced by an accurate one: HandleFailedImports keys off the run history, AlertFullReinitFallback has no state to key off and repeats while the rebuild it falls back to keeps failing, its description is kept constant so the open alert stays one row, and the acknowledged rows accumulate. That matches what RunAsync_ShouldLeaveOneStandingAlert_WhenTheBacklogFallbackRepeats asserts. Documentation only; no behaviour changed.

documentation/revision-history.md describes the seven-day bound and why it exists, in the 9.5.3 section that already covers this area.

Checks that produced nothing, on this commit

• Security pass — the commit adds no endpoint, no query, no schema change, no auth or Hasura permission change, and no new input surface; the only new stored value is a UTC timestamp. That timestamp goes into the ruleOwnerMappingRunHistory config row, whose exposure tier is the accepted F4 — it records when an admin last changed a mapping setting, which is no more revealing than the mapping source name, marker and custom-field keys already stored there by PendingChanges and checked against F4 in round 4, so the accepted tier does not widen. No secret, credential or token reaches source, log or alert; the new log line carries a timestamp and a day count.
• CODING_GUIDELINES.md — kPendingChangesMaxAge carries the k prefix and is static readonly because TimeSpan cannot be const; TimeSpan.FromDays(7) sits inside that named constant, so it is not a magic number. No method added or grown past 100 lines (TakeOverPendingChanges gains one guard and stays at three branches), no parameter or complexity limit approached, no file near 1000 lines, no dead or commented-out code. XML docs on both new members. The [new RuleOwnerMappingChange { … }] in the new test is a property initializer inside an object initializer, not an inline array argument — the same form rounds 4 to 9 accepted throughout these fixtures.
• Localization, help, release notes, schema — the commit adds no user-visible string, so no fworch-texts.sql or Help change is due; whats_new_facts already covers the feature and this is an internal refinement of it; no SQL, no migration, no upgrade path affected, product_version unchanged at 9.5.3.

What this round did not check

Stated so the next round knows what was scanned and what was not:

• Only d26fa9a9..372465c8 was reviewed. The rest of the PR surface — the services, the monitoring page, both settings pages, the GraphQL contracts, the full localization and help sweep — carries the verdicts of rounds 1 to 10 and was not re-examined, except where reading this commit required it (Store, IndicatesDrift, AlertMappingDrift, OwnerMappingSourceHandler, and the four tests that touch PendingChanges).
• I did not build the solution or run the unit tests locally. On head 372465c8 SonarQube passes — 0 new issues, 0 accepted issues, 0 security hotspots, 88.3% coverage on new code, 0.0% duplication — which covers compilation and the test run; Test install with ansible-core 2.18, GitGuardian, Python Code Check, Launcher guard and the test matrix pass; the second Test install job was still running.

---

Method

• Depth — standard. Not escalated: the reviewed commit is four files and 85 added lines, with no schema, migration, auth or permission change.
• Delegated to a reduced model tier — none this round, and that is a deliberate choice rather than an omission. The delegation this skill calls for pays off when a mechanical check fans out over many files; with four changed files the guideline, localization, help and version checks were faster to run directly than to brief, and every one of them was performed on the primary model.
• Kept on the primary model — everything: reading the commit, verifying F25 and F26 against the current tree, the reachability analysis that turned F27 from a candidate into a finding (three of the four paths into IndicatesDrift turned out not to reach it), the security pass, the rating, the numbering against F1–F26 and this comment.
• Reconciliation — the full comment history was read through gh api graphql, not gh pr view --json comments, which silently returns only the first few comments on this PR. The highest number previously used is F26, so this round's finding is F27. No review threads and no submitted reviews exist on this PR.
• Usage budget — this environment exposes no usage or quota indicator to the agent and no per-request token log I could attribute to this review, so the 400k ceiling for a standard review could not be measured; I am not reporting a number I did not observe. The proxy limits were enforced instead and all held: 0 sub-agent dispatches of the 6 allowed, standard depth with no escalation, reads confined to the reviewed commit plus the callers and tests it implicates, no whole-tree scans, and no pass or fetch repeated.
• Detail prose is in this comment rather than in inline comments, as the repository's review authority permits only a single top-level comment.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud