morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #4100] [MERGED] fix(artifacts): never count occurrences on an unknown duplicate path as unchanged #6058

New issue

Closed

opened 2026-10-31 01:11:32 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:11:32 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4100
Author: @kaoru0822-kitauji
Created: 10/15/2026
Status: ✅ Merged
Merged: 10/15/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/3956-duplicate-path-unchanged-count

📝 Commits (1)

d953d1f fix(artifacts): never count occurrences on an unknown duplicate path as unchanged

📊 Changes

2 files changed (+18 additions, -8 deletions)

View changed files

📝 src/domain/artifactComparison.ts (+11 -8)
📝 tests/boundary/filesystem/artifactComparison.test.ts (+7 -0)

📄 Description

Summary

Fixes #3956. Artifact comparison over-reported summary.unchanged when an inventory contained duplicate logical paths. One unknown change row stands for every occurrence on a duplicate path, but the summary subtracted only one per row. The remaining duplicate occurrences were reported as affirmatively unchanged. Now only occurrences on paths with no left-side change count as unchanged.

Problem and expected behavior

summarize (src/domain/artifactComparison.ts) computed unchanged as left occurrence count − removed − changed − contradiction − unknown, counting change rows. Change rows are keyed by logical_path, and duplicate paths cannot be paired, so they produce a single unknown row with null occurrence IDs.

I reproduced this with the issue's script: a CLI inventory of a ZIP whose central directory repeats x.txt, compared with itself over stdio MCP:

inventory	main	this PR
`.`, `neighbor.txt`, `x.txt`, `x.txt` (duplicate)	`unchanged: 3`, `unknown: 1`	`unchanged: 2`, `unknown: 1`
`.`, `neighbor.txt`, `x.txt`, `y.txt` (control)	`unchanged: 4`, `unknown: 0`	unchanged

Only . and neighbor.txt are affirmatively unchanged, as the issue expects. summary.unknown keeps its existing per-path aggregation; the issue does not require a policy change there.

Change and scope

summarize now counts left occurrences whose logical_path has no change row other than added (added rows have no left occurrence).
For inventories without duplicate paths the result is the same as before, because each non-added row then matches exactly one left occurrence.
The old Math.max(0, …) clamp is gone: the new count cannot go negative.

Contract and boundary impact

Semantic owner and earliest changed stage: the comparison summary, in summarize.
Contribution scope fit and linked maintainer decision: a bug fix within existing behavior; not applicable.
CLI and MCP/tool-catalog contract: schema unchanged.
Provider, bridge, target-format, or platform compatibility: provider-independent.
Evidence, artifact, provenance, or reconstruction contract: unchanged no longer counts occurrences whose pairing is unknown.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.

Evidence and regression coverage

Tests added or updated: tests/boundary/filesystem/artifactComparison.test.ts, in the existing duplicate-ZIP pages test, now asserts exact summaries.
- Differing archives: the root changes too, so unchanged: 1, unknown: 1.
- The issue's case, the duplicate inventory compared with itself: unchanged: 2, unknown: 1.
- The test fails on main (unchanged: 2 where 1 is expected) and passes with the fix.
Real data, built CLI inventory plus stdio MCP compare_artifacts, main vs this PR:

comparison	occurrences	main	this PR
`KeepingYouAwake.app` vs itself	410 / 410	`unchanged 404, unknown 6`, 47 ms, 9,041-byte reply	identical, 50 ms, 9,041 bytes
`KeepingYouAwake.app` vs `XLD.app`	410 / 625	`unchanged 1, added 616, removed 401, changed 8`, 68 ms, 1,118,237 bytes	identical, 68 ms, 1,118,237 bytes

The summaries agree on real bundles without duplicate paths, and timing is unchanged. The new count is a single pass over the left occurrences.

Remaining proof gaps: no real third-party archive with duplicate central-directory names was available, so the duplicate case is covered by the issue's crafted ZIP and the test fixture.

Validation performed

npx vitest run tests/boundary/filesystem/artifactComparison.test.ts tests/boundary/filesystem/artifactComparisonSummary.test.ts tests/boundary/filesystem/artifactDirectoryIdentity.test.ts tests/boundary/mcp/artifactMcp.test.ts src/application/artifacts src/domain/reconstructionVerification.test.ts src/domain/analysisView src/domain/artifactInventoryEvidence.test.ts src/artifacts/inventory — 16 files, 216 passed.
The extended duplicate test against main: 1 failed, as expected.
npm run check:fast — passed.
The issue's MCP repro and the real-bundle comparisons above.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: macOS app bundle inventories.
Package or release metadata impact: none.
Security, privacy, process, or containment review: pure summary computation.

Author readiness

Human owner:

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.

🤖 Generated with Claude Code

_{🔄 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/4100 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/15/2026 **Status:** ✅ Merged **Merged:** 10/15/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/3956-duplicate-path-unchanged-count` --- ### 📝 Commits (1) - [`d953d1f`](https://github.com/morluto/rea/commit/d953d1f9edb21c54e9aed34e5d648f235a68969b) fix(artifacts): never count occurrences on an unknown duplicate path as unchanged ### 📊 Changes **2 files changed** (+18 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/artifactComparison.ts` (+11 -8) 📝 `tests/boundary/filesystem/artifactComparison.test.ts` (+7 -0) </details> ### 📄 Description ## Summary Fixes #3956. Artifact comparison over-reported `summary.unchanged` when an inventory contained duplicate logical paths. One `unknown` change row stands for every occurrence on a duplicate path, but the summary subtracted only one per row. The remaining duplicate occurrences were reported as affirmatively unchanged. Now only occurrences on paths with no left-side change count as unchanged. ## Problem and expected behavior `summarize` (`src/domain/artifactComparison.ts`) computed `unchanged` as left occurrence count − removed − changed − contradiction − unknown, counting change rows. Change rows are keyed by `logical_path`, and duplicate paths cannot be paired, so they produce a single `unknown` row with null occurrence IDs. I reproduced this with the issue's script: a CLI inventory of a ZIP whose central directory repeats `x.txt`, compared with itself over stdio MCP: | inventory | main | this PR | |---|---|---| | `.`, `neighbor.txt`, `x.txt`, `x.txt` (duplicate) | `unchanged: 3`, `unknown: 1` | **`unchanged: 2`**, `unknown: 1` | | `.`, `neighbor.txt`, `x.txt`, `y.txt` (control) | `unchanged: 4`, `unknown: 0` | unchanged | Only `.` and `neighbor.txt` are affirmatively unchanged, as the issue expects. `summary.unknown` keeps its existing per-path aggregation; the issue does not require a policy change there. ## Change and scope - `summarize` now counts left occurrences whose `logical_path` has no change row other than `added` (`added` rows have no left occurrence). - For inventories without duplicate paths the result is the same as before, because each non-added row then matches exactly one left occurrence. - The old `Math.max(0, …)` clamp is gone: the new count cannot go negative. ## Contract and boundary impact - Semantic owner and earliest changed stage: the comparison summary, in `summarize`. - Contribution scope fit and linked maintainer decision: a bug fix within existing behavior; not applicable. - CLI and MCP/tool-catalog contract: schema unchanged. - Provider, bridge, target-format, or platform compatibility: provider-independent. - Evidence, artifact, provenance, or reconstruction contract: `unchanged` no longer counts occurrences whose pairing is unknown. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/filesystem/artifactComparison.test.ts`, in the existing duplicate-ZIP pages test, now asserts exact summaries. - Differing archives: the root changes too, so `unchanged: 1, unknown: 1`. - The issue's case, the duplicate inventory compared with itself: `unchanged: 2, unknown: 1`. - The test fails on main (`unchanged: 2` where 1 is expected) and passes with the fix. - Real data, built CLI inventory plus stdio MCP `compare_artifacts`, main vs this PR: | comparison | occurrences | main | this PR | |---|---|---|---| | `KeepingYouAwake.app` vs itself | 410 / 410 | `unchanged 404, unknown 6`, 47 ms, 9,041-byte reply | identical, 50 ms, 9,041 bytes | | `KeepingYouAwake.app` vs `XLD.app` | 410 / 625 | `unchanged 1, added 616, removed 401, changed 8`, 68 ms, 1,118,237 bytes | identical, 68 ms, 1,118,237 bytes | The summaries agree on real bundles without duplicate paths, and timing is unchanged. The new count is a single pass over the left occurrences. - Remaining proof gaps: no real third-party archive with duplicate central-directory names was available, so the duplicate case is covered by the issue's crafted ZIP and the test fixture. ## Validation performed - `npx vitest run tests/boundary/filesystem/artifactComparison.test.ts tests/boundary/filesystem/artifactComparisonSummary.test.ts tests/boundary/filesystem/artifactDirectoryIdentity.test.ts tests/boundary/mcp/artifactMcp.test.ts src/application/artifacts src/domain/reconstructionVerification.test.ts src/domain/analysisView src/domain/artifactInventoryEvidence.test.ts src/artifacts/inventory` — 16 files, 216 passed. - The extended duplicate test against main: 1 failed, as expected. - `npm run check:fast` — passed. - The issue's MCP repro and the real-bundle comparisons above. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: macOS app bundle inventories. - Package or release metadata impact: none. - Security, privacy, process, or containment review: pure summary computation. ## Author readiness Human owner:  - [ ] 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:11:32 +00:00

closed this issue
added the
pull-request
label

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#6058

No description provided.

Rows
Columns