feat(metamorphic): bridge target — metamorphic testing of check_facts#46
Merged
Conversation
Add scripts/metamorphic_facts.py, the sibling of metamorphic.py one level down: instead of mutating .own source it mutates the OwnIR facts (the JSON the extractor emits) and asserts check_facts's diagnostics are invariant. The facts are sets of records, so reversing any record list or consistently renaming a component/service identifier cannot change which leaks exist — a moved verdict would mean the bridge is order/name-sensitive where it must not be. - Two sound transforms: reverse (top-level component/service/function lists, a component's resource list, a service's deps) and rename (a component/service identifier, consistently across the whole fact graph by exact-match replace). - Compared on the code multiset (same lesson the core harness learned from codex). - Result: all 18 committed fact fixtures + a captive-DI set are invariant, including DI001 (a singleton capturing a scoped service) under both service reordering and a consistent rename of the dependency graph. No bug found — the heavily-edited bridge (DI graph, finding dedup, source-lifetime tiering) is order/name-stable, which is the reassuring outcome. - Wired into the CI script-selftests step; docs/notes/metamorphic.md now covers both targets (core .own + bridge facts) and the OwnIR-fact follow-up is done. Also corrected a stale "(code, line)" mention left over from #45. metamorphic-facts 6/6, metamorphic 8/8, miner 15/15, oracle 24/24, full suite green, ruff + mypy --strict clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesBridge metamorphic test harness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~13 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a3a514b95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/metamorphic_facts.py (1)
93-97: ⚡ Quick winAvoid sharing original nested objects when building reverse variants.
Lines 96 and 104 reverse
subs/depstaken from the originalfacts, then assign them into the copied variant. That reintroduces aliases to the source graph and can taint later variants if any downstream path mutates input objects.Safe local fix
- v["components"][i]["subscriptions"] = list(reversed(subs)) + v_subs = v["components"][i].get("subscriptions") + if isinstance(v_subs, list): + v["components"][i]["subscriptions"] = list(reversed(v_subs)) yield (f"reverse components[{i}].subscriptions", v) ... - v["services"][i]["deps"] = list(reversed(deps)) + v_deps = v["services"][i].get("deps") + if isinstance(v_deps, list): + v["services"][i]["deps"] = list(reversed(v_deps)) yield (f"reverse services[{i}].deps", v)Also applies to: 101-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/metamorphic_facts.py` around lines 93 - 97, The code shares nested object references when assigning reversed subscriptions and dependencies to the copied variants. In the reversed components subscriptions block (around line 96) and the similar reversed component dependencies block (around 104-105), instead of assigning list(reversed(subs)) and list(reversed(deps)) directly, wrap them with copy.deepcopy to ensure the reversed lists contain independent copies of their elements. This prevents downstream mutations from tainting the original facts structure through shared object references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/metamorphic_facts.py`:
- Around line 66-77: The _rename function currently performs exact string
matching and replacement on all string values globally throughout the fact
graph, which causes false positives when unrelated semantic literals (like DI
lifetime values) happen to match an identifier name. Refactor the _rename
function to only apply string renaming when processing identifier-bearing fields
(such as component/service names, deps, source_type, etc.) within dictionary
structures, while leaving string values in other contexts untouched. This
requires checking the dictionary key name when iterating through dict.items()
and only calling _rename on values from identifier-bearing fields, otherwise
returning them unchanged.
- Around line 164-176: The function fails to report errors when input files
cannot be loaded or when no files are processed, allowing exit code 0 in failure
scenarios. In the exception handler that catches OSError and
json.JSONDecodeError (lines 164-166), increment the bad counter to track load
failures alongside violation failures. Additionally, add a check after the file
processing loop to ensure at least one file was successfully processed; if the
files list is empty, return a failure code. This ensures the final return
statement correctly propagates failure status for both unreadable inputs and
empty sweep sets.
---
Nitpick comments:
In `@scripts/metamorphic_facts.py`:
- Around line 93-97: The code shares nested object references when assigning
reversed subscriptions and dependencies to the copied variants. In the reversed
components subscriptions block (around line 96) and the similar reversed
component dependencies block (around 104-105), instead of assigning
list(reversed(subs)) and list(reversed(deps)) directly, wrap them with
copy.deepcopy to ensure the reversed lists contain independent copies of their
elements. This prevents downstream mutations from tainting the original facts
structure through shared object references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 953348a0-79ed-409b-bbdd-5da2a0fcc5a5
📒 Files selected for processing (3)
.github/workflows/ci.ymldocs/notes/metamorphic.mdscripts/metamorphic_facts.py
…ep exit Addresses codex + CodeRabbit review on #46. - codex: drive the real bridge entry — check_facts(load(path)). sweep and the selftest now load() each fact set (schema + version gate) instead of raw json.loads, so a schema-invalid file (e.g. a future ownir_version) is rejected at the gate, not silently swept as "invariant". New version-gate selftest case. - CodeRabbit (major): the rename is now key-aware — it touches only identifier sites (a name/source_type field, a deps entry), never a semantic literal, so a component/service named e.g. "scoped" no longer has its lifetime rewritten. New collision selftest case; all 18 fixtures stay invariant (the narrower rename is still complete for the schema's reference graph). - CodeRabbit (major): sweep returns non-zero on a load error or an empty input set (a sweep that evaluated nothing is no longer a false green). - CodeRabbit (nit): reverse_variants reverses the deep-copied variant's own list, not the original's, so a variant never aliases the source graph. metamorphic-facts 8/8, metamorphic 8/8, miner 15/15, oracle 24/24, full suite green, ruff + mypy --strict clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
scripts/metamorphic_facts.py— the sibling ofmetamorphic.py(#45) one level down. Instead of mutating.ownsource it mutates the OwnIR facts (the JSON the C# extractor emits) and assertscheck_facts's diagnostics are invariant. The facts are sets of records, so reversing any record list or consistently renaming a component/service identifier cannot change which leaks exist — a moved verdict would mean the bridge is order/name-sensitive where it must not be. Higher-signal than the core, since the bridge carries the incidental complexity (DI captive graph, finding dedup, source-lifetime tiering). dotnet-free.Transforms (both meaning-preserving)
deps. Independent records commute.deps/source_typemove with it) — alpha-equivalence over the fact graph.Compared on the multiset of diagnostic codes (the lesson the core harness learned from codex: a record's line is intrinsic, but order/name must not move a code).
Result
All 18 committed fact fixtures + a captive-DI set are invariant — including
DI001(a singleton capturing a scoped service), which holds under both service reordering and a consistent rename of the dependency graph. No bug found: the heavily-edited bridge (DI graph, dedup, tiering) is order/name-stable — the reassuring outcome.The analyzer is now metamorphically gated top to bottom, locally: core (
.own, 28 programs,metamorphic.py) + bridge (facts, 18 fixtures,metamorphic_facts.py), both--selftests in CI. The OwnIR-fact follow-up indocs/notes/metamorphic.mdis now done.Validation
metamorphic-facts6/6,metamorphic8/8, miner 15/15, oracle 24/24, full suite green,ruff+mypy --strictclean,ci.ymlparses. Also corrected a stale(code, line)mention left over from #45.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
New Features
metamorphic_factsCLI that runs invariance checks by generating transformed OwnIR fact variants and comparing diagnostic code outputs.Documentation
metamorphic_facts.pysweeps and selftests.Chores