Skip to content

Name the NetBox row a recorded merge rejection is about - #207

Merged
captainpacket merged 1 commit into
forwardnetworks:mainfrom
sujeito-operator:name-the-row-a-merge-rejection-is-about
Aug 15, 2026
Merged

Name the NetBox row a recorded merge rejection is about#207
captainpacket merged 1 commit into
forwardnetworks:mainfrom
sujeito-operator:name-the-row-a-merge-rejection-is-about

Conversation

@sujeito-operator

Copy link
Copy Markdown
Contributor

Closes the second half of #206.

What an operator sees

The field report in #206 quotes this record verbatim:

Merge for ipam.ipaddress failed (ValidationError) violating primary-ip-reassignment-blocked. Recorded and skipped: re-running cannot change a NetBox validation rejection, so the baseline was promoted over this row.

Accurate, and unactionable — resolving it means editing the offending row in NetBox, and
one ipam.ipaddress row out of ~4,000 devices is being talked about with nothing saying
which. After this patch the same record ends:

… so the baseline was promoted over this row. Affected NetBox row: pk 900002.

raw_data gains row_pk and row alongside the existing diagnosis keys.

The identity was never missing

_record_failed already holds the CollapsedChange, whose key is (label_lower, pk)
— the same key[1] the authoritative-delete guard logs in the clear a few frames below.
It just was not passed to record(), which took model_string and exc and nothing
else. record() gains pk and change_data, both optional.

It names the pk, not the address — deliberately

#206 asks for the address, "and ideally the device and interface". I did not do that, and
the reason is your own invariant rather than an oversight:

  • diagnostic_shape keeps field names and discards their values;
  • redacted_message_shape exists for the same reason;
  • _MergeIssueRecorder's own comment says the key values a Postgres DETAIL line embeds
    "are deliberately still not captured".

An address, a device name and an interface name are customer values, and ingestion issues
reach support bundles. The NetBox pk is a surrogate you assigned; it resolves to the
object page, which is exactly where the edit has to happen, and it discloses nothing. So
it answers "which row" completely and leaves the boundary intact.

If you want the value as well, that is one function. _row_identity is the only place
to change, and MergeRowIdentityTests.test_no_customer_value_reaches_the_message_or_the_shape
plus the two assertNotIn pairs in the recorder tests are the assertions that would need
to be retired — knowingly, which is the point of putting them there.

The Drift Summary half of #206 is not addressed here

I confirmed the diagnosis and stopped. EXACT_COMPARISON is defined and never produced:
_dependency_dry_run_payload hardcodes workload_upper_bound
(views.py:507-508, :563) and drift_report.py:136,176 gates on the constant, with
all(...) across models, so one upper-bound model disables the whole report.

Your fix direction offers two answers — compute a real per-model comparison, or stop
presenting a drift measurement on a path that cannot produce one and relabel the panel.
That is a product decision. Picking one uninvited on a first contribution seemed worse
than leaving it, so please keep #206 open for it; I am happy to take whichever
direction you choose.

Compatibility

pk defaults to None and _row_identity returns None for it, so a caller with no row
in hand — the orchestration paths that record a whole-model failure — gets the message it
always had, byte for byte. MergeIssueRecorderTest.test_module_bay_failures_are_recorded_as_blocking_merge_issues
still asserts "Merge for dcim.modulebay failed (Exception)." by equality, unchanged, and
is the regression test for that path. New raw_data keys are disjoint from the diagnosis
keys. No migration — raw_data is already a JSONField.

The trailing sentence is appended rather than folded into the prefix, because
safe_operation_failure writes the head that recovered_classifiers reads back out.

docs/03_Plans/active/2026-08-15-name-the-merge-rejected-row.md is in the diff because
scripts/check_harness.py requires a plan for a high-risk surface; it failed on the change
without one.

What I verified, and what I could not

Ran and green: black 25.9.0 --check, flake8 7.3.0, scripts/check_harness.py,
scripts/check_sensitive_content.py. _row_identity is exercised against your real
diagnostic_shape — no pk leaves the message untouched, an int pk renders, a UUID pk
survives, a DELETE with no postchange_data records no shape, and no customer value
reaches either surface.

I could not run invoke test. It needs NetBox, PostgreSQL and Redis, and I do not have
that runtime — so the four tests added here are written against the suite's existing
fixtures and conventions but have not been executed. Please treat CI, not this PR body, as
the authority on them.

Tests added: MergeRowIdentityTests (4, SimpleTestCase),
test_the_skipped_row_names_the_netbox_object_to_edit (end-to-end through sync_merge
with the field-reported primary-ip-reassignment-blocked), and
test_a_recorded_row_names_its_pk_and_records_its_shape (pins the exact message and
raw_data).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants