[PR #7161] fix(environment): map id-list elements when a scalar set also changed #7686

Open
opened 2026-11-03 13:39:38 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/7161
Author: @mhamzasakhi
Created: 10/25/2026
Status: 🔄 Open

Base: main ← Head: fix/compare-id-list-mapping


📝 Commits (3)

  • 6f4ed55 fix(environment): map id-list elements when a scalar set also changed
  • 24a8510 fix(environment): pair id-list survivors by translated identity
  • 820fdb2 fix(environment): keep id-list order checks to matched survivors

📊 Changes

2 files changed (+162 additions, -11 deletions)

View changed files

📝 src/application/environment/environmentCompareDiff.test.ts (+93 -0)
📝 src/application/environment/environmentCompareDiff.ts (+69 -11)

📄 Description

Summary

When comparing an answer's scalar id list, a genuinely changed element made the comparison also report the unchanged ids as changed. Comparing live {"ids":[101,102]} with env {"ids":[1,9]} under the learned mapping 101→1, 102→2 reported both /ids/0 (101 vs 1) and the real /ids/1 (102 vs 9), even though 101→1 is the mapping, not a change.

This was surfaced by the automated review of #6530 (a P2 finding on environmentCompareDiff.ts) and reproduced at the source boundary; it is a concrete defect in the environment-compare engine. No separate issue is linked.

Problem and expected behavior

Differ.sameItems maps the whole scalar id list and, when the mapped set equals env, counts the renames and returns. Once one element genuinely changed, or the lengths differed, sameItems returned false and array fell to pairwise comparison. Those element comparisons were passed { own, clockKey } but not the id-list field, so scalar.mapsAsId was false and each element was compared raw.

Expected: each id-list element maps by the probe's mapping, so a renamed element matches (and counts as a normalised id) while only a genuinely different element is reported — with its original live value, not its mapped form.

Change and scope

  • For an id list, array pairs survivors by the environment identity they map to (#idListPairs over #translateIdList), then pairs any remaining live and environment items by position. This handles a survivor that shifted position when the set was shortened or changed (review follow-up). The translation is one owner shared with sameItems. Other arrays keep positional pairing.
  • Each element's comparison carries an idList flag derived with the same per-element rule sameItems uses (ID_LIST_FIELD on the field, or a learned state reference).
  • scalar.mapsAsId honors that flag, so the element maps through IdMap.env. A mapped match increments normalised.ids and reports nothing; a real difference falls through and is reported with the original values, exactly as a singular id field already is.
  • value forwards the flag.
  • Non-id arrays are unaffected: the flag is false unless the field is an id-list field or the element is a learned state reference.
  • The paired comparison passes the original live id to scalar, so a renamed survivor matches and is normalised while a genuine change reports its original value.
  • order is computed only from the identity-matched survivor pairs, not the positional fallbacks (review follow-up): a single surviving identity that shifts is a value replacement, not a reorder, so it no longer emits a spurious order. Two or more reordered survivors still report order.
  • Not included: object-keyed id comparison (object/keyedByLearnedIds) already maps correctly and is unchanged.

Contribution scope: an ordinary correctness fix within existing boundaries; a maintainer scope decision is not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: scalar id-list element comparison in Differ.array/Differ.scalar.

  • CLI and MCP/tool-catalog contract: no schema change. compare-application-versions / compare-web-captures now omit the false-positive id differences and count the renames.

  • Provider, bridge, target-format, or platform compatibility: none.

  • Evidence, artifact, provenance, or reconstruction contract: difference lists become accurate; normalised.ids counts the renamed-but-matching elements.

  • Generated metadata, package, or installation impact: none.

  • Read existing files or captured data.

  • Launch or attach to a process.

  • Inject instrumentation or run scripts/code.

  • Change process memory, registers, files, or configuration.

  • Contact network services.

  • Recover protected code or work around protections.

  • Documentation or usability only; no target interaction.

  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added to src/application/environment/environmentCompareDiff.test.ts, in "scalar sets of mapped ids":

    • "maps each id when one element genuinely changed, reporting only the real change": [101,102] vs [1,9] under 101→1,102→2 reports only /ids/1 (102 vs 9) and counts normalised.ids === 1.
    • "maps the surviving ids when the list length also differs": [101,102] vs [1] reports only the length difference and counts normalised.ids === 1.
    • "pairs a surviving id by its mapping when a removal shifts its position" (review follow-up): [101,102] vs [2] reports only the length difference, with no false /ids/0, and counts normalised.ids === 1.
    • "reports a replaced id as a value change, not an order change, when one survivor shifts" (review follow-up): [101,102] vs [2,9] reports only /ids/0 (101 vs 9), with no order.
    • "still reports an order change when two mapped survivors are reordered": [101,102,103] vs [2,1,99] reports order plus /ids/2.
  • Base reproduction (executed): with environmentCompareDiff.ts reverted, the new tests fail; the first commit's two tests produce an extra /ids/0, and the shifted-survivor test fails against the first commit (it still emitted a false /ids/0).

  • Additional cases checked at the source boundary (all correct after the change): a fully reordered mapped set reports only order; a non-id array (tags) reports only its real element change with no mapping; equal ids with no learned mapping report nothing.

  • Observed, derived, and inferred claims remain distinguishable.

  • Artifact identity, source provenance, and failed attempts remain preserved.

  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Run on Ubuntu 24.04 x64 with Node 24.22.1:

  • npm run test:local -- src/application/environment/environmentCompareDiff.test.ts: 70 passed. With the fix reverted: the two new tests fail.
  • npm run test:focused -- with every test file referencing environment compare or state (60 files): 1517 passed, 1 skipped, 0 failed. The heavy toolSchemaValidity boundary test (~30s, unrelated to this comparison change) timed out at its 30s limit under local load and passes in isolation; it is excluded from that count.
  • npm run check:fast: passed. knip reports nothing for the touched module.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Only false-positive id-list differences stop being reported; genuine differences and non-id arrays are unchanged.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable (pure comparison function). Linux only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new reads, execution, or network access.

Author readiness

Human owner: @mhamzasakhi

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/7161 **Author:** [@mhamzasakhi](https://github.com/mhamzasakhi) **Created:** 10/25/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/compare-id-list-mapping` --- ### 📝 Commits (3) - [`6f4ed55`](https://github.com/morluto/rea/commit/6f4ed55edc3be5280701a68d10147b2e00ee5aa1) fix(environment): map id-list elements when a scalar set also changed - [`24a8510`](https://github.com/morluto/rea/commit/24a851078c811ce4fa101ac3f2acbe22ba81649a) fix(environment): pair id-list survivors by translated identity - [`820fdb2`](https://github.com/morluto/rea/commit/820fdb2d9ce0a5a47688032f2377ddc6784bdc81) fix(environment): keep id-list order checks to matched survivors ### 📊 Changes **2 files changed** (+162 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `src/application/environment/environmentCompareDiff.test.ts` (+93 -0) 📝 `src/application/environment/environmentCompareDiff.ts` (+69 -11) </details> ### 📄 Description ## Summary When comparing an answer's scalar id list, a genuinely changed element made the comparison also report the *unchanged* ids as changed. Comparing live `{"ids":[101,102]}` with env `{"ids":[1,9]}` under the learned mapping `101→1`, `102→2` reported both `/ids/0` (101 vs 1) and the real `/ids/1` (102 vs 9), even though `101→1` is the mapping, not a change. This was surfaced by the automated review of #6530 (a P2 finding on `environmentCompareDiff.ts`) and reproduced at the source boundary; it is a concrete defect in the environment-compare engine. No separate issue is linked. ## Problem and expected behavior `Differ.sameItems` maps the whole scalar id list and, when the mapped set equals env, counts the renames and returns. Once one element genuinely changed, or the lengths differed, `sameItems` returned false and `array` fell to pairwise comparison. Those element comparisons were passed `{ own, clockKey }` but not the id-list field, so `scalar.mapsAsId` was false and each element was compared raw. Expected: each id-list element maps by the probe's mapping, so a renamed element matches (and counts as a normalised id) while only a genuinely different element is reported — with its original live value, not its mapped form. ## Change and scope - For an id list, `array` pairs survivors by the environment identity they map to (`#idListPairs` over `#translateIdList`), then pairs any remaining live and environment items by position. This handles a survivor that shifted position when the set was shortened or changed (review follow-up). The translation is one owner shared with `sameItems`. Other arrays keep positional pairing. - Each element's comparison carries an `idList` flag derived with the same per-element rule `sameItems` uses (`ID_LIST_FIELD` on the field, or a learned state reference). - `scalar.mapsAsId` honors that flag, so the element maps through `IdMap.env`. A mapped match increments `normalised.ids` and reports nothing; a real difference falls through and is reported with the original values, exactly as a singular id field already is. - `value` forwards the flag. - Non-id arrays are unaffected: the flag is false unless the field is an id-list field or the element is a learned state reference. - The paired comparison passes the original live id to `scalar`, so a renamed survivor matches and is normalised while a genuine change reports its original value. - `order` is computed only from the identity-matched survivor pairs, not the positional fallbacks (review follow-up): a single surviving identity that shifts is a value replacement, not a reorder, so it no longer emits a spurious `order`. Two or more reordered survivors still report `order`. - Not included: object-keyed id comparison (`object`/`keyedByLearnedIds`) already maps correctly and is unchanged. Contribution scope: an ordinary correctness fix within existing boundaries; a maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: scalar id-list element comparison in `Differ.array`/`Differ.scalar`. - CLI and MCP/tool-catalog contract: no schema change. `compare-application-versions` / `compare-web-captures` now omit the false-positive id differences and count the renames. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: difference lists become accurate; `normalised.ids` counts the renamed-but-matching elements. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added to `src/application/environment/environmentCompareDiff.test.ts`, in "scalar sets of mapped ids": - "maps each id when one element genuinely changed, reporting only the real change": `[101,102]` vs `[1,9]` under `101→1,102→2` reports only `/ids/1` (`102` vs `9`) and counts `normalised.ids === 1`. - "maps the surviving ids when the list length also differs": `[101,102]` vs `[1]` reports only the length difference and counts `normalised.ids === 1`. - "pairs a surviving id by its mapping when a removal shifts its position" (review follow-up): `[101,102]` vs `[2]` reports only the length difference, with no false `/ids/0`, and counts `normalised.ids === 1`. - "reports a replaced id as a value change, not an order change, when one survivor shifts" (review follow-up): `[101,102]` vs `[2,9]` reports only `/ids/0` (`101` vs `9`), with no `order`. - "still reports an order change when two mapped survivors are reordered": `[101,102,103]` vs `[2,1,99]` reports `order` plus `/ids/2`. - Base reproduction (executed): with `environmentCompareDiff.ts` reverted, the new tests fail; the first commit's two tests produce an extra `/ids/0`, and the shifted-survivor test fails against the first commit (it still emitted a false `/ids/0`). - Additional cases checked at the source boundary (all correct after the change): a fully reordered mapped set reports only `order`; a non-id array (`tags`) reports only its real element change with no mapping; equal ids with no learned mapping report nothing. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed Run on Ubuntu 24.04 x64 with Node 24.22.1: - `npm run test:local -- src/application/environment/environmentCompareDiff.test.ts`: 70 passed. With the fix reverted: the two new tests fail. - `npm run test:focused --` with every test file referencing environment compare or state (60 files): 1517 passed, 1 skipped, 0 failed. The heavy `toolSchemaValidity` boundary test (~30s, unrelated to this comparison change) timed out at its 30s limit under local load and passes in isolation; it is excluded from that count. - `npm run check:fast`: passed. `knip` reports nothing for the touched module. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only false-positive id-list differences stop being reported; genuine differences and non-id arrays are unchanged. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (pure comparison function). Linux only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new reads, execution, or network access. ## Author readiness Human owner: @mhamzasakhi - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#7686
No description provided.