[PR #5250] [CLOSED] fix(managed): keep operand-free instructions in CIL anchors #6838

Closed
opened 2026-11-01 21:43:24 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5250
Author: @akushonkamen
Created: 10/19/2026
Status: ❌ Closed

Base: main ← Head: fix/5147-managed-anchors-operand-free


📝 Commits (1)

  • fd8e098 fix(managed): keep operand-free instructions in CIL anchors

📊 Changes

3 files changed (+88 additions, -9 deletions)

View changed files

📝 src/dotnet/ManagedMemberInspector.test.ts (+70 -1)
📝 src/dotnet/ManagedMethodBodyReader.ts (+11 -8)
📝 tests/boundary/providers/managed/managedMemberComparisonFromMetadata.test.ts (+7 -0)

📄 Description

Summary

inspect-managed-members dropped every operand-free instruction from body.anchors, so branch comparison constants like the 4 in ldloc.s 4; ldc.i4.4; bne.un were unrecoverable from the result. Fixes #5147.

Problem and expected behavior

src/dotnet/ManagedMethodBodyReader.ts built body.anchors with a .filter((instruction) => instruction.operandKind !== "none"). Operand-free opcodes (ldc.i4.N, dup, pop, ldarg.N, ldloc.N, stloc.N, ret, …) vanished from anchors while opcode_counts and the normalized decode hash still counted them — on the issue's example (token 0x060029f5), instruction_count 208 but only 132 anchors. Expected behavior, per the maintainer endorsement on the issue: anchors retain every decoded instruction, so a branch reports the constant it compares against.

Change and scope

Anchors now cover every decoded instruction: the operandKind !== "none" filter is removed in src/dotnet/ManagedMethodBodyReader.ts and a comment records why operand-free anchors must stay. No schema change is needed (managedInstructionAnchorSchema already allows operand_kind: "none"). Consumers are unaffected: call edges and field accesses (ManagedMemberRows.ts), ldstr literal collection (ManagedUserStrings.ts), and MethodSpec resolution (ManagedMethodSpecs.ts) all filter anchors by opcode or operand kind and guard null operands; the decode budget charges anchors by retained JSON size, so bounds still hold. Intentionally not included: sister issue #5146 and any documentation rewrite (docs never described anchors as operand-only).

Follow the contribution scope. Maintainer scope decision: not applicable — ordinary fix within the existing managed-metadata inspection boundary; no new provider, execution, injection, or protection recovery.

Contract and boundary impact

  • Semantic owner and earliest changed stage: ManagedMethodBodyReader (managed CIL body decode) — the producing boundary for anchors.
  • Contribution scope fit and linked maintainer decision: not applicable, see above.
  • CLI and MCP/tool-catalog contract: inspect-managed-members output gains anchor entries for operand-free instructions; schema unchanged (operand_kind enum already includes "none").
  • Provider, bridge, target-format, or platform compatibility: none — pure decode-side change.
  • Evidence, artifact, provenance, or reconstruction contract: normalized_il_sha256 and il_sha256 unchanged (already covered all instructions); structuralKey comparison hashes now span the same anchor set they already spanned via opcode_counts.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • 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: new regression test in src/dotnet/ManagedMemberInspector.test.ts ("managed method body anchors") builds the IL ldloc.s 4; ldc.i4.4; bne.un.s +0; ret and asserts the exact anchors including { il_offset: 2, opcode: "ldc.i4.4", operand_kind: "none", operand: null } and the ret anchor; existing anchor expectations updated to the complete lists in src/dotnet/ManagedMemberInspector.test.ts and tests/boundary/providers/managed/managedMemberComparisonFromMetadata.test.ts.
  • Base reproduction or other evidence: executed. Red on base 3ea0d9489 — the new test failed with anchors missing the ldc.i4.4 and ret entries; green after the one-line filter removal.
  • User-visible CLI/MCP output (if applicable): body.anchors now lists operand-free instructions alongside operand-bearing ones.
  • Remaining proof gaps: the full --project boundary suite was attempted twice but did not finish within a 900 s budget on a machine running several concurrent builds; the relevant managed subsets (below) and the DMG file whose failure fragment appeared under that load (nativeDmgArtifactReader.test.ts, 48/48 in isolation) all pass. On CI, the PR's red lanes (Test shard (1-5/5), Portable tests) fail on setup/acceptance/provider tests (Gemini workspace trust, HOME fallback, kuna verifier) with no managed or anchor test among them; the base commit 3ea0d948 fails the same lanes on its own main CI run (39254713979, plus Real Apple artifact E2E, recorded_crash, darwin native baselines), so those lanes are pre-existing red, not introduced here. The PR readiness check stays red until the human owner completes the Author readiness declarations below — intentionally left to a human per the template.

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

  • npx vitest run src/dotnet/ManagedMemberInspector.test.ts -t "operand-free" — red before the fix (anchors lacked ldc.i4.4, ret); passes after.
  • npx vitest run src/dotnet — 14 files, 344/344 passed.
  • npx vitest run tests/boundary/providers/managed src/domain/managed — 10 files, 118/118 passed.
  • npx vitest run src/dotnet tests/boundary/providers/managed src/domain/managed — 24 files, 462/462 passed.
  • npx vitest run tests/boundary/providers/native/nativeDmgArtifactReader.test.ts — 48/48 passed in isolation.
  • npm run typecheck — clean after a standalone npm ci (the shared node_modules was missing openpgp).
  • npm run check:fast — typecheck + lint, 2/2 tasks successful.
  • npx oxlint / oxfmt --check on the three touched files — no new findings (baseline files already carry pre-existing warnings).
  • CI on this PR: Static checks, Source tests, Build package, Build bundled Windows x64 native controls, Native package baseline (linux x64), Classify changes, Conventional PR title, Greptile Review all pass; the failing lanes match pre-existing main failures (see Remaining proof gaps).

Compatibility, safety, and release

  • Breaking changes or migration steps: none. body.anchors grows (output contract change endorsed in the issue); schema already accepted the shape, so older readers of newer output are unaffected and newer readers of older Evidence see the previous lists as-is.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable — no provider interaction; covered by unit/boundary tests.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none — read-only decode of already-opened artifact bytes.

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.

🔄 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/5250 **Author:** [@akushonkamen](https://github.com/akushonkamen) **Created:** 10/19/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/5147-managed-anchors-operand-free` --- ### 📝 Commits (1) - [`fd8e098`](https://github.com/morluto/rea/commit/fd8e0983ce8993e198bf3625cd7fe4eaf642f137) fix(managed): keep operand-free instructions in CIL anchors ### 📊 Changes **3 files changed** (+88 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `src/dotnet/ManagedMemberInspector.test.ts` (+70 -1) 📝 `src/dotnet/ManagedMethodBodyReader.ts` (+11 -8) 📝 `tests/boundary/providers/managed/managedMemberComparisonFromMetadata.test.ts` (+7 -0) </details> ### 📄 Description ## Summary `inspect-managed-members` dropped every operand-free instruction from `body.anchors`, so branch comparison constants like the `4` in `ldloc.s 4; ldc.i4.4; bne.un` were unrecoverable from the result. Fixes #5147. ## Problem and expected behavior `src/dotnet/ManagedMethodBodyReader.ts` built `body.anchors` with a `.filter((instruction) => instruction.operandKind !== "none")`. Operand-free opcodes (`ldc.i4.N`, `dup`, `pop`, `ldarg.N`, `ldloc.N`, `stloc.N`, `ret`, …) vanished from anchors while `opcode_counts` and the normalized decode hash still counted them — on the issue's example (token `0x060029f5`), `instruction_count` 208 but only 132 anchors. Expected behavior, per the maintainer endorsement on the issue: anchors retain every decoded instruction, so a branch reports the constant it compares against. ## Change and scope Anchors now cover every decoded instruction: the `operandKind !== "none"` filter is removed in `src/dotnet/ManagedMethodBodyReader.ts` and a comment records why operand-free anchors must stay. No schema change is needed (`managedInstructionAnchorSchema` already allows `operand_kind: "none"`). Consumers are unaffected: call edges and field accesses (`ManagedMemberRows.ts`), ldstr literal collection (`ManagedUserStrings.ts`), and MethodSpec resolution (`ManagedMethodSpecs.ts`) all filter anchors by opcode or operand kind and guard null operands; the decode budget charges anchors by retained JSON size, so bounds still hold. Intentionally not included: sister issue #5146 and any documentation rewrite (docs never described anchors as operand-only). Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Maintainer scope decision: not applicable — ordinary fix within the existing managed-metadata inspection boundary; no new provider, execution, injection, or protection recovery. ## Contract and boundary impact - Semantic owner and earliest changed stage: `ManagedMethodBodyReader` (managed CIL body decode) — the producing boundary for anchors. - Contribution scope fit and linked maintainer decision: not applicable, see above. - CLI and MCP/tool-catalog contract: `inspect-managed-members` output gains anchor entries for operand-free instructions; schema unchanged (`operand_kind` enum already includes `"none"`). - Provider, bridge, target-format, or platform compatibility: none — pure decode-side change. - Evidence, artifact, provenance, or reconstruction contract: `normalized_il_sha256` and `il_sha256` unchanged (already covered all instructions); `structuralKey` comparison hashes now span the same anchor set they already spanned via `opcode_counts`. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [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 or updated: new regression test in `src/dotnet/ManagedMemberInspector.test.ts` ("managed method body anchors") builds the IL `ldloc.s 4; ldc.i4.4; bne.un.s +0; ret` and asserts the exact anchors including `{ il_offset: 2, opcode: "ldc.i4.4", operand_kind: "none", operand: null }` and the `ret` anchor; existing anchor expectations updated to the complete lists in `src/dotnet/ManagedMemberInspector.test.ts` and `tests/boundary/providers/managed/managedMemberComparisonFromMetadata.test.ts`. - Base reproduction or other evidence: executed. Red on base `3ea0d9489` — the new test failed with anchors missing the `ldc.i4.4` and `ret` entries; green after the one-line filter removal. - User-visible CLI/MCP output (if applicable): `body.anchors` now lists operand-free instructions alongside operand-bearing ones. - Remaining proof gaps: the full `--project boundary` suite was attempted twice but did not finish within a 900 s budget on a machine running several concurrent builds; the relevant managed subsets (below) and the DMG file whose failure fragment appeared under that load (`nativeDmgArtifactReader.test.ts`, 48/48 in isolation) all pass. On CI, the PR's red lanes (`Test shard (1-5/5)`, `Portable tests`) fail on setup/acceptance/provider tests (Gemini workspace trust, HOME fallback, kuna verifier) with no managed or anchor test among them; the base commit `3ea0d948` fails the same lanes on its own `main` CI run (39254713979, plus `Real Apple artifact E2E`, `recorded_crash`, darwin native baselines), so those lanes are pre-existing red, not introduced here. The `PR readiness` check stays red until the human owner completes the Author readiness declarations below — intentionally left to a human per the template. 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 - `npx vitest run src/dotnet/ManagedMemberInspector.test.ts -t "operand-free"` — red before the fix (anchors lacked `ldc.i4.4`, `ret`); passes after. - `npx vitest run src/dotnet` — 14 files, 344/344 passed. - `npx vitest run tests/boundary/providers/managed src/domain/managed` — 10 files, 118/118 passed. - `npx vitest run src/dotnet tests/boundary/providers/managed src/domain/managed` — 24 files, 462/462 passed. - `npx vitest run tests/boundary/providers/native/nativeDmgArtifactReader.test.ts` — 48/48 passed in isolation. - `npm run typecheck` — clean after a standalone `npm ci` (the shared `node_modules` was missing `openpgp`). - `npm run check:fast` — typecheck + lint, 2/2 tasks successful. - `npx oxlint` / `oxfmt --check` on the three touched files — no new findings (baseline files already carry pre-existing warnings). - CI on this PR: `Static checks`, `Source tests`, `Build package`, `Build bundled Windows x64 native controls`, `Native package baseline (linux x64)`, `Classify changes`, `Conventional PR title`, `Greptile Review` all pass; the failing lanes match pre-existing `main` failures (see Remaining proof gaps). ## Compatibility, safety, and release - Breaking changes or migration steps: none. `body.anchors` grows (output contract change endorsed in the issue); schema already accepted the shape, so older readers of newer output are unaffected and newer readers of older Evidence see the previous lists as-is. - Real Hopper/Ghidra, browser, or OS coverage: not applicable — no provider interaction; covered by unit/boundary tests. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none — read-only decode of already-opened artifact bytes. ## Author readiness <!-- The human owner completes these declarations before requesting review. The PR author is the owner by default. A different owner must explicitly accept ownership on this PR. AI agents must leave these declarations to a human to dictate to them or there could be catastrophic consequences for many users of REA. Draft PRs may leave them unchecked. These declarations record initial readiness when opening the PR for review; later pushes do not require a new human sign-off. Keep the description and validation evidence current as the work changes. See CONTRIBUTING.md#pull-request-ownership-and-readiness. --> Human owner: <!-- @GitHub-handle --> - [ ] 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 <!-- Draft PRs may leave pending items unchecked. Resolve material scope and authority questions before integration. --> - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:43:24 +00:00
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#6838
No description provided.