morluto/rea

Fork 0

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

[PR #5597] fix(evidence): stop offering overwrite for outputs that never overwrite #6986

New issue

Open

opened 2026-11-01 21:44:30 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:44:30 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5597
Author: @mpratsaki
Created: 10/20/2026
Status: 🔄 Open

Base: main ← Head: fix/evidence-output-overwrite-guidance

📝 Commits (6)

d4b12d8 fix(evidence): stop offering overwrite for outputs that never overwrite
0110822 fix(il2cpp): declare that Ghidra seed output never overwrites
3140f8c fix(reconstruction): dedupe residual unknowns with uniqueSorted
a6380ee fix(hopper): pass the host distribution into install-host tests
b0ff528 test: align inherited fixtures with current analysis contracts
60934f3 test(ida): assert search recovery instead of diagnostic prose

📊 Changes

18 files changed (+150 additions, -48 deletions)

View changed files

📝 src/application/EvidenceBundleCommands.ts (+9 -3)
📝 src/application/EvidenceBundleFiles.ts (+2 -1)
📝 src/application/JsonFiles.ts (+14 -9)
📝 src/application/LinuxHopper.test.ts (+9 -5)
📝 src/application/LinuxHopperPackageRuntime.test.ts (+19 -4)
📝 src/application/ReconstructionObligationCandidates.ts (+4 -4)
📝 src/application/binary/AnalysisSnapshotFiles.ts (+6 -1)
📝 src/application/pcap/PcapAnalysisService.test.ts (+6 -0)
📝 src/domain/analysisErrorPresentation.ts (+4 -1)
📝 src/domain/analysisErrorProjection.test.ts (+23 -0)
📝 src/domain/browserScenarioDiff.test.ts (+1 -0)
📝 src/domain/evidenceErrors.ts (+10 -0)
📝 src/ida/IdaSessionClient.test.ts (+3 -4)
📝 src/il2cpp/Il2cppGhidraSeedWriter.ts (+1 -1)
📝 src/server/registerSessionRecordTools.ts (+1 -1)
📝 tests/boundary/cli/desktopApplicationCli.test.ts (+15 -3)
📝 tests/boundary/filesystem/evidenceBundleFiles.test.ts (+14 -2)
📝 tests/boundary/filesystem/jsonOutputOwnership.test.ts (+9 -9)

📄 Description

Summary

CLI summary delivery (--evidence_output) never replaces an existing bundle, but its refusal told callers to "Choose another path or allow overwrite" in both message and remediation.action. inspect-artifact (and every other --evidence_output command) then rejects the suggested --overwrite with Unknown flag: --overwrite. The refusal now gives recovery the operation actually offers; commands that do support overwrite keep their guidance.

Fixes #5408

Problem and expected behavior

Trigger: run inspect-artifact <zip> --detail summary --evidence_output <file> --json twice. The second run correctly refuses and leaves the bundle unchanged, but recommends an overwrite that the command does not support.

Violated invariant: recovery guidance must name an action available to the failing operation (docs/user-visible-errors.csv, EvidenceFileError row).

Change and scope

Root cause: the shared writers (writeTextParts, writeTextFile, writeEvidenceBundle) took overwrite: boolean, so "the caller did not request replacement" and "this operation can never replace" were the same false, and the shared exists message assumed the first.

The writers now take an OverwritePolicy ("replace" | "refuse" | "never"), and the exists refusal records the policy that produced it (EvidenceFileError.overwrite).
publishCliAnalysisEvidence declares "never"; the message becomes "Evidence file already exists, and this operation never overwrites it. Choose another path and try again." remediation.action falls back to the same text.
evidence-export, MCP export_evidence_bundle, and snapshot saves map their existing boolean overwrite input to "replace"/"refuse" and keep "Choose another path or allow overwrite."
Update after merging current main (a5f982de8): main added Il2cppGhidraSeedWriter, a new writeTextParts caller whose --ghidra-seeds file is documented as never replaced. The typed policy flagged it; it now passes "never" (separate commit e8132c1d0). The only merge conflict was an import line in desktopApplicationCli.test.ts.
Update after merging main 2452efa92 (6e4cef98f): #6221 moved delivery publication into publishEvidenceDelivery (src/application/EvidenceBundleCommands.ts), which called persistEvidenceBundleCommand with false. That helper now takes an OverwritePolicy; delivery declares "never" and evidence-export maps --overwrite to "replace"/"refuse".
CI fix (8863d12e8): concluding that merge ran lint-staged's oxlint --fix over incoming files and rewrote two files from main this PR never touched (src/main/deferredRuntime.ts, src/domain/flutter/flutterBuildAnalysis.ts), failing format:check and advertisedPatternsVMode.test.ts. Both are restored byte-for-byte from the merged main; the branch now differs from main only in this PR's files.
Not included: no new --overwrite option for --evidence_output; projected details are unchanged.

Contribution scope: ordinary fix within existing boundaries; maintainer scope decision not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: the atomic writer in src/application/JsonFiles.ts, which constructs the exists refusal; the CLI delivery helper declares the never policy.
Contribution scope fit and linked maintainer decision: fix within existing boundaries; not applicable.
CLI and MCP/tool-catalog contract: only the message/remediation.action text for --evidence_output collisions changes. Codes, categories, details, inputs, and the tool catalog are unchanged.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: unchanged; existing files remain protected and partial observations are still retained.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.

Expected effects:

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 or updated:
- tests/boundary/cli/desktopApplicationCli.test.ts: the existing public-CLI collision step asserted only a non-zero exit. It now asserts code, category, details, the exact message and remediation.action, and that the published bundle bytes are unchanged.
- src/domain/analysisErrorProjection.test.ts: refuse keeps the overwrite hint and never omits it.
- evidenceBundleFiles / jsonOutputOwnership boundary tests: call sites updated to the policy type with unchanged semantics.
Base reproduction: on the base revision, inspect-artifact printed "Evidence file already exists. Choose another path or allow overwrite." and --overwrite returned Unknown flag: --overwrite. With the delivery helper temporarily set back to "refuse", the strengthened CLI test fails with exactly that message diff.

User-visible CLI output (after), second inspect-artifact run:

{
  "code": "invalid_request",
  "category": "invalid_input",
  "message": "Evidence file already exists, and this operation never overwrites it. Choose another path and try again.",
  "remediation": { "action": "Evidence file already exists, and this operation never overwrites it. Choose another path and try again." },
  "details": { "operation": "write", "reason": "exists", "path": "…" }
}

The existing bundle stayed byte-identical, and a fresh path succeeded. Control: an evidence-export collision still says "…or allow overwrite", and evidence-export --overwrite still succeeds.

Remaining proof gaps: macOS not run locally (CI covers it); the other --evidence_output commands share the changed helper and are covered at the source level, with only inspect-artifact and inspect-desktop-application exercised through the CLI.

For evidence-bearing changes:

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

Linux (Ubuntu 24.04, Node 24.18.0):

npm run test:focused -- src/domain/analysisErrorProjection.test.ts tests/boundary/filesystem/evidenceBundleFiles.test.ts tests/boundary/filesystem/jsonOutputOwnership.test.ts tests/boundary/cli/desktopApplicationCli.test.ts: 4 files, 94 tests passed.
npm run check:fast: 2/2 tasks successful (uncached).
npm run test:changed: 211/212 files, 3277 passed, 4 skipped, 1 failed. The failure was PlaywrightElectronActiveProvider.test.ts > reports an expired final state grace period…, a vi.waitFor timing check unrelated to this change; rerun alone it passed 26/26.

Linux (Ubuntu 24.04, Node 24.18.0), after merging main 2452efa92 (head 6e4cef98f):

Same 5 focused test files: 5 passed, 122 passed, 0 skipped.

Windows 11, after merging main 2452efa92 (head 6e4cef98f):

npm run test:focused -- the 4 changed test files plus tests/boundary/il2cpp/il2cppGhidraSeeds.test.ts: 5 files, 119 passed, 3 skipped (platform-specific).
npm run check:fast: passed.

Windows 11, after merging main a5f982de8 (head e8132c1d0):

npm run test:focused -- the 4 files above plus tests/boundary/il2cpp/il2cppGhidraSeeds.test.ts and tests/acceptance/il2cppMetadataCli.test.ts: 6 files, 121 passed, 4 skipped (platform-specific).
npm run check:fast (pre-push, uncached): passed.

Windows 11 (Node 24.18.0):

Same focused tests: 92 passed, 2 skipped (POSIX-only).
npm run check:fast: passed.
Issue reproduction script before and after (outputs above).
npm run test:changed: 57 failures in 14 files. The same 57 fail on the base revision 1417bf943 (POSIX fixture paths such as /usr/bin/hopper); none are introduced by this change.

Compatibility, safety, and release

Breaking changes or migration steps: none. Internal writer signatures changed; all callers were updated.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (no provider involved); Linux and Windows as above.
Package or release metadata impact: none.
Security, privacy, process, or containment review: existing files remain protected; no new write authority.

CI follow-up: corrected inherited test fixtures to declare unresolved packet-tool identity and current-version storage fingerprints. Removed the duplicate IDA diagnostic-prose assertion; the supervision regression now checks the exact recovered search row after a deadline failure, while RegexMatcher owns diagnostic coverage. The three originally failing files passed 102 tests on each branch; the final IDA/RegexMatcher batch passed 117 tests on the evidence-output branch. Typecheck/lint passed. Hosted CI on the new heads remains pending.

Author readiness

Human owner: @mpratsaki

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/5597 **Author:** [@mpratsaki](https://github.com/mpratsaki) **Created:** 10/20/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/evidence-output-overwrite-guidance` --- ### 📝 Commits (6) - [`d4b12d8`](https://github.com/morluto/rea/commit/d4b12d8b9325042ab23126ae0a1e455961d6a3f5) fix(evidence): stop offering overwrite for outputs that never overwrite - [`0110822`](https://github.com/morluto/rea/commit/011082201dc5c4509daa2542596e31834c0afa3e) fix(il2cpp): declare that Ghidra seed output never overwrites - [`3140f8c`](https://github.com/morluto/rea/commit/3140f8c0026d8f70cf5a355f9397f47b2834fb49) fix(reconstruction): dedupe residual unknowns with uniqueSorted - [`a6380ee`](https://github.com/morluto/rea/commit/a6380ee11f90a508b425f23b5aa84b478698af05) fix(hopper): pass the host distribution into install-host tests - [`b0ff528`](https://github.com/morluto/rea/commit/b0ff528837d9fdf62ad917147d855e14699dabf0) test: align inherited fixtures with current analysis contracts - [`60934f3`](https://github.com/morluto/rea/commit/60934f3836c2e1fc65dec56704808a30cbd1fd13) test(ida): assert search recovery instead of diagnostic prose ### 📊 Changes **18 files changed** (+150 additions, -48 deletions) <details> <summary>View changed files</summary> 📝 `src/application/EvidenceBundleCommands.ts` (+9 -3) 📝 `src/application/EvidenceBundleFiles.ts` (+2 -1) 📝 `src/application/JsonFiles.ts` (+14 -9) 📝 `src/application/LinuxHopper.test.ts` (+9 -5) 📝 `src/application/LinuxHopperPackageRuntime.test.ts` (+19 -4) 📝 `src/application/ReconstructionObligationCandidates.ts` (+4 -4) 📝 `src/application/binary/AnalysisSnapshotFiles.ts` (+6 -1) 📝 `src/application/pcap/PcapAnalysisService.test.ts` (+6 -0) 📝 `src/domain/analysisErrorPresentation.ts` (+4 -1) 📝 `src/domain/analysisErrorProjection.test.ts` (+23 -0) 📝 `src/domain/browserScenarioDiff.test.ts` (+1 -0) 📝 `src/domain/evidenceErrors.ts` (+10 -0) 📝 `src/ida/IdaSessionClient.test.ts` (+3 -4) 📝 `src/il2cpp/Il2cppGhidraSeedWriter.ts` (+1 -1) 📝 `src/server/registerSessionRecordTools.ts` (+1 -1) 📝 `tests/boundary/cli/desktopApplicationCli.test.ts` (+15 -3) 📝 `tests/boundary/filesystem/evidenceBundleFiles.test.ts` (+14 -2) 📝 `tests/boundary/filesystem/jsonOutputOwnership.test.ts` (+9 -9) </details> ### 📄 Description ## Summary CLI summary delivery (`--evidence_output`) never replaces an existing bundle, but its refusal told callers to "Choose another path or allow overwrite" in both `message` and `remediation.action`. `inspect-artifact` (and every other `--evidence_output` command) then rejects the suggested `--overwrite` with `Unknown flag: --overwrite`. The refusal now gives recovery the operation actually offers; commands that do support overwrite keep their guidance. Fixes #5408 ## Problem and expected behavior Trigger: run `inspect-artifact <zip> --detail summary --evidence_output <file> --json` twice. The second run correctly refuses and leaves the bundle unchanged, but recommends an overwrite that the command does not support. Violated invariant: recovery guidance must name an action available to the failing operation (`docs/user-visible-errors.csv`, EvidenceFileError row). ## Change and scope Root cause: the shared writers (`writeTextParts`, `writeTextFile`, `writeEvidenceBundle`) took `overwrite: boolean`, so "the caller did not request replacement" and "this operation can never replace" were the same `false`, and the shared `exists` message assumed the first. - The writers now take an `OverwritePolicy` (`"replace" | "refuse" | "never"`), and the `exists` refusal records the policy that produced it (`EvidenceFileError.overwrite`). - `publishCliAnalysisEvidence` declares `"never"`; the message becomes "Evidence file already exists, and this operation never overwrites it. Choose another path and try again." `remediation.action` falls back to the same text. - `evidence-export`, MCP `export_evidence_bundle`, and snapshot saves map their existing boolean `overwrite` input to `"replace"`/`"refuse"` and keep "Choose another path or allow overwrite." - Update after merging current `main` (a5f982de8): `main` added `Il2cppGhidraSeedWriter`, a new `writeTextParts` caller whose `--ghidra-seeds` file is documented as never replaced. The typed policy flagged it; it now passes `"never"` (separate commit e8132c1d0). The only merge conflict was an import line in `desktopApplicationCli.test.ts`. - Update after merging `main` 2452efa92 (6e4cef98f): #6221 moved delivery publication into `publishEvidenceDelivery` (`src/application/EvidenceBundleCommands.ts`), which called `persistEvidenceBundleCommand` with `false`. That helper now takes an `OverwritePolicy`; delivery declares `"never"` and `evidence-export` maps `--overwrite` to `"replace"`/`"refuse"`. - CI fix (8863d12e8): concluding that merge ran lint-staged's `oxlint --fix` over incoming files and rewrote two files from `main` this PR never touched (`src/main/deferredRuntime.ts`, `src/domain/flutter/flutterBuildAnalysis.ts`), failing `format:check` and `advertisedPatternsVMode.test.ts`. Both are restored byte-for-byte from the merged `main`; the branch now differs from `main` only in this PR's files. - Not included: no new `--overwrite` option for `--evidence_output`; projected `details` are unchanged. Contribution scope: ordinary fix within existing boundaries; maintainer scope decision not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: the atomic writer in `src/application/JsonFiles.ts`, which constructs the `exists` refusal; the CLI delivery helper declares the `never` policy. - Contribution scope fit and linked maintainer decision: fix within existing boundaries; not applicable. - CLI and MCP/tool-catalog contract: only the `message`/`remediation.action` text for `--evidence_output` collisions changes. Codes, categories, `details`, inputs, and the tool catalog are unchanged. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: unchanged; existing files remain protected and partial observations are still retained. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. Expected effects: - [ ] 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. - [x] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: - `tests/boundary/cli/desktopApplicationCli.test.ts`: the existing public-CLI collision step asserted only a non-zero exit. It now asserts code, category, `details`, the exact `message` and `remediation.action`, and that the published bundle bytes are unchanged. - `src/domain/analysisErrorProjection.test.ts`: `refuse` keeps the overwrite hint and `never` omits it. - `evidenceBundleFiles` / `jsonOutputOwnership` boundary tests: call sites updated to the policy type with unchanged semantics. - Base reproduction: on the base revision, `inspect-artifact` printed `"Evidence file already exists. Choose another path or allow overwrite."` and `--overwrite` returned `Unknown flag: --overwrite`. With the delivery helper temporarily set back to `"refuse"`, the strengthened CLI test fails with exactly that message diff. - User-visible CLI output (after), second `inspect-artifact` run: ```json { "code": "invalid_request", "category": "invalid_input", "message": "Evidence file already exists, and this operation never overwrites it. Choose another path and try again.", "remediation": { "action": "Evidence file already exists, and this operation never overwrites it. Choose another path and try again." }, "details": { "operation": "write", "reason": "exists", "path": "…" } } ``` The existing bundle stayed byte-identical, and a fresh path succeeded. Control: an `evidence-export` collision still says "…or allow overwrite", and `evidence-export --overwrite` still succeeds. - Remaining proof gaps: macOS not run locally (CI covers it); the other `--evidence_output` commands share the changed helper and are covered at the source level, with only `inspect-artifact` and `inspect-desktop-application` exercised through the CLI. For evidence-bearing changes: - [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 Linux (Ubuntu 24.04, Node 24.18.0): - `npm run test:focused -- src/domain/analysisErrorProjection.test.ts tests/boundary/filesystem/evidenceBundleFiles.test.ts tests/boundary/filesystem/jsonOutputOwnership.test.ts tests/boundary/cli/desktopApplicationCli.test.ts`: 4 files, 94 tests passed. - `npm run check:fast`: 2/2 tasks successful (uncached). - `npm run test:changed`: 211/212 files, 3277 passed, 4 skipped, 1 failed. The failure was `PlaywrightElectronActiveProvider.test.ts > reports an expired final state grace period…`, a `vi.waitFor` timing check unrelated to this change; rerun alone it passed 26/26. Linux (Ubuntu 24.04, Node 24.18.0), after merging `main` 2452efa92 (head 6e4cef98f): - Same 5 focused test files: 5 passed, 122 passed, 0 skipped. Windows 11, after merging `main` 2452efa92 (head 6e4cef98f): - `npm run test:focused --` the 4 changed test files plus `tests/boundary/il2cpp/il2cppGhidraSeeds.test.ts`: 5 files, 119 passed, 3 skipped (platform-specific). - `npm run check:fast`: passed. Windows 11, after merging `main` a5f982de8 (head e8132c1d0): - `npm run test:focused --` the 4 files above plus `tests/boundary/il2cpp/il2cppGhidraSeeds.test.ts` and `tests/acceptance/il2cppMetadataCli.test.ts`: 6 files, 121 passed, 4 skipped (platform-specific). - `npm run check:fast` (pre-push, uncached): passed. Windows 11 (Node 24.18.0): - Same focused tests: 92 passed, 2 skipped (POSIX-only). - `npm run check:fast`: passed. - Issue reproduction script before and after (outputs above). - `npm run test:changed`: 57 failures in 14 files. The same 57 fail on the base revision `1417bf943` (POSIX fixture paths such as `/usr/bin/hopper`); none are introduced by this change. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Internal writer signatures changed; all callers were updated. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (no provider involved); Linux and Windows as above. - Package or release metadata impact: none. - Security, privacy, process, or containment review: existing files remain protected; no new write authority. CI follow-up: corrected inherited test fixtures to declare unresolved packet-tool identity and current-version storage fingerprints. Removed the duplicate IDA diagnostic-prose assertion; the supervision regression now checks the exact recovered search row after a deadline failure, while RegexMatcher owns diagnostic coverage. The three originally failing files passed 102 tests on each branch; the final IDA/RegexMatcher batch passed 117 tests on the evidence-output branch. Typecheck/lint passed. Hosted CI on the new heads remains pending. ## Author readiness Human owner: @mpratsaki - [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 - [ ] 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 added the

pull-request

label

2026-11-01 21:44:30 +00:00

bsakaguchi referenced this issue

2026-11-03 13:38:36 +00:00

[PR #6984] [MERGED] fix(cli): exclude inherited values from Markdown rows #7546

bsakaguchi referenced this issue

2026-11-03 13:38:38 +00:00

[PR #6986] [MERGED] fix(javascript): bound cyclic mutation captures by their paths #7548

bsakaguchi referenced this issue

2026-11-03 13:38:38 +00:00