feat(ownir): carry a real Roslyn source column from the extractor to SARIF - #318
Merged
Conversation
…ge half) Own.NET#317, child of #266. OwnAudit's `finding-occurrence/v1` anchors a physical finding on `path + startLine + startColumn`; own-check reports a line and nothing else, so every one of its findings anchors line-only. This is the bridge half of the fix: the fact path now carries a column end to end. The extractor does not emit one yet, so no behaviour changes for any existing input. RED FIRST `tests/test_ownir_column.py` was written before the implementation and is the executable specification: PYTHONUTF8=1 python3 tests/test_ownir_column.py before: 18 failed of 28 checks after: 0 failed of 30 checks The 10 that passed while red are the point of the contract - they pin what must NOT move: the flow-local anchor line, OWNIR_VERSION, and the human/GitHub/MSBuild renderings byte for byte. RED categories: Finding.column absent; SARIF region line-only; flow-local acquire column absent; seven invalid column shapes not rejected; two findings on one line collapsing to one; field order. THE CASE THAT DECIDES IT `check_facts` has two flow-local shapes and only one anchors on the diagnostic: * the special OWN025 branch (POOL005) reports at the VIEW site, `d.line`; * every other flow-local verdict, OWN001 among them, reports at the ACQUIRE site, `sub["line"]` - which `_lower_flow` carried over from the Roslyn `acquire` op via `acq_line`. The dedupe comment at ownir.py:2911 says so outright, and running `flow_leak_two_exits.facts.json` proves it: the acquire is at 105, both exits at 106, the finding lands on 105. So flow-local OWN001 is producer-solvable - its coordinate is a Roslyn fact, not a `.own` parse. An earlier reading of this file concluded the opposite and would have written off 23 corpus rows as unfixable; the test now pins the real behaviour so that reading cannot recur. FIVE HANDLE PATHS, NOT ONE A flow-local handle is minted in five places, and a column that travels only the obvious one would leave whole categories line-only: * `_lower_fn_params` (a param that became an owned obligation) * direct `acquire` * `alias_join` * fresh-returning call result * hoisted branch acquire - `_hoisted_branch_locals` now returns `(line, column, pool)`, with the column keyed on the SAME first-seen acquire the line came from, so a hoisted local can never carry one node's line and another node's column. NO MIXED COORDINATES OWN025 keeps `column=None` explicitly. Its line is the violation site; attaching the acquire's column would produce a well-formed SARIF coordinate pointing at a place that does not exist - the worst kind of wrong, because nothing downstream could detect it. A real column there needs the `.own` parser span, which is a non-goal. TWO ENTRY POINTS, TWO CONTRACTS `load()` is fail-loud: `0`, negative, `bool`, `str`, `float` and `list` all raise `OwnIRError`, and the walk recurses into `if`/`while` bodies because the hoisted branch acquire - the path most likely to be forgotten - lives nested. `check_facts()` degrades to None through `_as_col`, since it is called directly on un-validated facts. `bool` is excluded explicitly: `True` is an `int` and would otherwise read as column 1, the exact fabricated coordinate this contract refuses. DEDUPE AND SORT `column` joins the dedupe key for the same reason `line` is in it. Without it, two findings differing only in column collapse into one - the test caught exactly that. The sort key uses `f.column or 0` because sorting a mix of None and int raises on the first tie; a real column is >= 1, so 0 is an unambiguous "absent" that lives only in the key and never in the record. Schema: `column` is declared on the anchor path only - `resourceRecord`, `param` and every `flowOp` variant, which is exactly what `_check_flow_columns` validates. A first pass added it beside all 21 `line` fields, including `service`/`effect`/`binding`/`protocolEvent`; that was reverted, because a schema should declare what is implemented rather than what might be someday. OWNIR_VERSION stays 0: an optional field with a safe default is additive (spec/OwnIR.md §2). `Finding.column` is declared LAST, after `ignore_reason`, and a test asserts it - positional construction is used across this module and a field inserted mid-list silently re-binds every argument after it. Verified: 30/30 new contract; 282/282 ownir bridge; full Python suite (the new file is picked up by run_tests auto-discovery); all of it again under -O and under PYTHONUTF8=0 LC_ALL=C; all 21 existing fixtures load unchanged, and a column-less fact still emits `startLine` alone. NOT DONE IN THIS COMMIT: the extractor does not emit `column` yet (`frontend/roslyn/OwnSharp.Extractor/Program.cs`), so nothing produces one end to end; `spec/OwnIR.md` prose; the Rust `own-ir` round-trip run. `dotnet` is not available in this environment, so the extractor change cannot be compiled or tested here and is deliberately left out rather than committed unverified. Refs #317, #266. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
…round-trip Own.NET#317, on top of 8a26fbd. Nothing about behaviour changes here - this is the normative text for the field the bridge already carries, plus the evidence that the Rust `own-ir` crate preserves it. SPEC `spec/OwnIR.md` gains §4.1, and it states three rules rather than describing a field: 1. SAME NODE. The column belongs to the node the line came from. Pairing one node's line with another's column yields a well-formed coordinate pointing at a place that does not exist - worse than an absent field, because nothing downstream can detect it. Named concretely: the OWN025 (POOL005) verdict anchors on the view site, so it reports NO column rather than the acquire's. 2. NEVER INVENTED. Not 0, not 1, and not recovered by re-reading the source line - `Diagnostic._caret_col` does exactly that for its human caret, and it is a renderer heuristic, not a coordinate the analysis computed. 3. VALIDATED, NOT COERCED. `load()` is fail-loud and recurses into `if`/`while` bodies; `check_facts()` degrades to absent. `bool` is rejected explicitly because `True` is an `int` and would otherwise read as column 1. The §2 evolution table lists `column` beside `type`/`source_type` as additive, so `OWNIR_VERSION` stays 0, and §4/§5 mention it where `line` is introduced. RUST `cargo test -p own-ir`: 11 passed. The crate needed no change - `column` rides in the flattened `extra`, which is its documented contract - but "no change needed" and "no check needed" are different claims, and only one of them is true. `round_trips_every_python_fixture` already covers the new fixture by scanning the directory, but only implicitly: it proves whatever happens to be on disk. The added `optional_column_survives_the_round_trip` asserts the field by name and counts the eight columns, so the guarantee cannot quietly become vacuous if both sides drop the field together. THE FIXTURE, AND WHAT IT ACTUALLY PROVES `tests/fixtures/ownir/flow_column_anchors.facts.json` yields six findings, each carrying a column: the contract param that became an owned obligation (30:26), two direct acquires (40:13 and 40:44), the acquire nested in an `if` branch (44:21 - the hoisted path), the subscription (12:17) and the disposable field (21:9). The two acquires SHARE line 40 and differ only in column: that pair is what proves the column discriminates rather than decorating every record with the same number. A GAP FOUND IN MY OWN WORK Two of the five handle-minting paths - `alias_join` and the fresh-returning call result - were carried in code and covered by nothing. The fixture touches both but yields no finding from either, which would have read as coverage without being any. Both are now checked at Finding level in the contract test (34 checks, was 30), and the alias case records what was actually observed rather than what was hoped: an `alias_join` handle carries a column, but a finding still anchors at the SOURCE acquire in every probed shape, because the alias shares its obligation rather than creating a second one. If that ever changes, the check fails and someone decides deliberately which site should anchor. Verified: 34/34 column contract; 282/282 ownir bridge; full Python suite and again under -O; `cargo test -p own-ir` 11/11; `cargo clippy -p own-ir --tests` clean. STILL NOT DONE: the extractor does not emit a column (`frontend/roslyn/OwnSharp.Extractor/Program.cs`), so nothing produces one end to end. That is the next commit, and it cannot be compiled or tested in this environment - `dotnet` is unavailable - so it will be marked as such and verified by CI. Refs #317, #266. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
Own.NET#317, corrective on bf3cd28. No production change - the previous commit's alias_join check asserted the wrong thing, and this makes it assert the right one. THE CHECK WAS VACUOUS bf3cd28 claimed the `alias_join` path was covered. It observed only the finding: check([(f.line, f.column, f.event) for f in ali] == [(12, 7, "x")]) That proves the finding anchors at the SOURCE acquire. It says nothing about the alias handle, which is the thing the column actually travels through - delete the column from the alias handle and the check stays green. Coverage in appearance only, which is worse than no check, because it stops anyone looking. Now split into two, because they are two different claims: (a) the HANDLE keeps the alias site's own coordinate (13, 9), observed directly through `to_module`, which already returns `(Module, handles)` - no new production API needed for a test to see this; (b) the FINDING still anchors at the source acquire (12, 7), because the alias shares that obligation rather than creating a second one. Negative control, run rather than assumed: dropping `"column"` from the alias_join handle alone now fails with `alias_join handle lost its own coordinate` (1 failed of 36). Under the old check that same deletion was silent. RUST `byte-for-byte` in the new test's doc comment was wrong - it compares `serde_json::Value`, so the guarantee is value-for-value, which is what the rest of this file says and what the assertion actually performs. The eight-column count was replaced by eight JSON Pointer assertions naming each path. Re-serializing the whole document to a string inside every loop iteration to search it for `"column":` - after serde had already parsed it - was both wasteful and weak: it counted columns without caring which records kept them, so a future edit that moved one would still count to eight and pass. DOCSTRING The header claimed every check fails on `main@0ded835`. It does not, on purpose: 18 of 28 failed and 10 passed, and those 10 pin what must NOT move - the flow-local anchor line, OWNIR_VERSION, and the human/GitHub/MSBuild renderings. A contract made all-red on purpose would prove less. The text now says so. Verified: 36/36 column contract (was 34, +2 for the split alias claim); 282/282 ownir bridge; full Python suite and again under -O; `cargo test -p own-ir` 11/11; `cargo clippy -p own-ir --tests` clean. Next, unchanged: the extractor commit, marked reviewed-not-run locally because `dotnet` is unavailable here, then a draft PR for real compilation in CI. Refs #317, #266. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
Own.NET#317, the producer half. The bridge half (8a26fbd) carries a `column` from fact to SARIF `region.startColumn`; nothing produced one, so every own-check finding still anchored line-only. This is where the coordinate is created. ONE LOCATION AUTHORITY, NOT A PARALLEL HELPER `LineOf(node)` becomes a projection of `PosOf(node)`, which is `RangeOf(node).Start`, which is the single `GetLineSpan()` call. There is deliberately no `ColumnOf(node)`: two independent lookups are two chances to pass two different nodes, and the result would be a well-formed coordinate pointing at a place that does not exist - the worst kind of wrong, because nothing downstream can detect it. Every anchor record now does var pos = PosOf(<the exact node LineOf was called on>); ... line = pos.Line, column = pos.Column ... `FixSpanOf` is rewritten over `RangeOf` too, so the S0 fix block and the anchor records share one definition of "the position of this node". Its serialized shape is unchanged: same six properties, same order, same values. `SourcePos`/`SourceRange` are declared inside the existing `partial class Program` at the end of the file. Two types rather than one because `FixSpanOf` also serializes `end_line`/`end_column`, and a bare `SourcePos` would have forced it to keep its own `GetLineSpan()` - forking the second path this change exists to remove. THREE CARRIERS, NOT JUST THE EMISSION SITES Three places hold a coordinate across a distance, and a carrier that dropped the column would leave nothing to recover it from at the far end except a second lookup: * `dispoFieldLine` Dictionary<string,int> -> `dispoFieldPos` Dictionary<string,SourcePos> * `pooledFieldRent` Dictionary<string,int> -> Dictionary<string,SourcePos> * `rented` List<(string, int, bool)> -> List<(string, SourcePos, bool)> `releasedAt`, `helperReads`, `useLine` and `vline` are untouched on purpose. They are release/use/view sites - events on the path, not the record a finding anchors on - and a column there would pair one node's line with another node's column. WHERE THE COLUMN GOES: 18 RECORDS Seven handle-minting flow ops: the `using`-declaration acquire, the `using`-statement acquire, the direct acquire, `alias_join`, the fresh-returning `call` result, the synthetic disposed-field acquire, and the synthetic pooled-field acquire. Eleven `subs.Add` shapes: the four `+=` variants (one `PosOf(a.Left)` serving all four, so they cannot disagree), the unresolved subscription, the declared weak-subscribe wrapper, the disposable field with and without `[OwnIgnore]`, the dropped Rx token, the POOL001 rent, and the D1 local disposable. Nothing else gets one. `release`, `use`, `overspan`, `return`, `if`/`while` and the throw-edge ops keep their line alone. THE FIXTURE, AND WHY IT IS SHAPED LIKE THAT `frontend/roslyn/samples/ColumnAnchorsSample.cs` puts TWO anchor sites on ONE line in each family it covers: two `+=` on one line, two undisposed locals on one line. Every other sample in that directory puts one finding on one line, so a column that was always 1, or always the indentation, or always the first site's, would pass all of them. `tests/test_extractor_columns.py` runs the real extractor over it in both modes (default -> `local-disposable` records; `--flow-locals` -> `acquire` ops) and asserts each record's exact (line, column), plus separately that the pair does not share a column. It also asserts the fixture is tab-free plain ASCII first, because that is what makes the expected columns arithmetic on the text rather than a claim about the file's encoding. The expected values are (30, 9), (30, 31), (51, 13), (51, 45). They were read off the fixture's text - `Character + 1` is the 1-based index of the node's first character on an ASCII, tab-free line - and they are NOT verified against a real extractor run. `tests/check_fix_candidates_facts.py` gains the cross-check that needs no expected values at all: every subscription record's `column` must equal its own fix block's `span.start_column`. Those are computed in different code (`PosOf(a.Left)` versus `FixSpanOf(a)`), and an assignment expression starts exactly where its left side does, so agreement is required and disagreement means the anchor took its column from a node other than the one it names. That holds on every sample, every run, with nothing hardcoded. NOT DONE HERE, AND EXPECTED TO BE RED `tests/goldens/fix_candidates_off.golden.json` is a byte-for-byte parity gate against a frozen pre-S0 flag-off run of `FixCandidatesSample.cs`. All 23 of its subscription records now gain a `column`, so that gate WILL fail. It is left untouched rather than hand-edited: regenerating it correctly needs a real extractor run, and forging 23 coordinates by hand is exactly the fabrication this change is about refusing. The regeneration procedure is in `tests/goldens/README.md` and belongs after CI has produced genuine output. Reviewed statically; not compiled or executed locally because dotnet is unavailable. Exact extractor columns and golden parity require Own.NET CI verification. Verified locally (everything that does not need dotnet): full Python suite green and again under -O; 36/36 ownir column contract; 282/282 ownir bridge; cargo test -p own-ir 11/11; the new test skips cleanly with no dotnet and fails loudly under OWN_TIERB_REQUIRED=1; the fixture is ASCII and tab-free; brace and parenthesis balance in Program.cs is unchanged from HEAD. Refs #317, #266. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
A one-line whitespace fix in the generic resource-record finding path: the `column=` continuation was indented four columns past its siblings. Valid Python inside the parentheses, and it changes no behaviour - the surrounding suite is green before and after - but it reads as a nesting level that is not there. Refs #317. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
The `lint (ruff + mypy --strict)` job runs `ruff check .` over the whole tree with a pinned ruff 0.15.8, and eight findings in the two #317 test files would have failed it - six carried in from the bridge commit, two from the new extractor test. All of them are style, none change behaviour: * `test_ownir_column.py` - two import blocks reformatted to one-name-per-line, and three `# noqa` directives removed. Two were for `E402` on lines ruff does not flag (the module-level import follows a `sys.path` insert, which the config already allows); the third named `BLE001`, a rule this project does not enable. A `noqa` for a rule that never fires reads as "this line is known to be dodgy", which is a claim, not a suppression. * `test_extractor_columns.py` - dropped the unused `sys` import and replaced list concatenation in the extractor argv with unpacking. `ruff check .` is clean; the full Python suite and the 36-check column contract pass before and after, and again under -O. Refs #317. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
| // `src` is a parameter, so the source is `injected` and both are warnings. | ||
| public void SubscribeBoth(ColumnAnchorsSample src) | ||
| { | ||
| src.Alpha += OnAlpha; src.Beta += OnBeta; |
| // `src` is a parameter, so the source is `injected` and both are warnings. | ||
| public void SubscribeBoth(ColumnAnchorsSample src) | ||
| { | ||
| src.Alpha += OnAlpha; src.Beta += OnBeta; |
| // code and only one of them was on the obvious route. | ||
| public long TwoLeaksOneLine() | ||
| { | ||
| var first = new MemoryStream(); var second = new MemoryStream(); |
| // code and only one of them was on the obvious route. | ||
| public long TwoLeaksOneLine() | ||
| { | ||
| var first = new MemoryStream(); var second = new MemoryStream(); |
…trip Two CI gates that the local runs missed, both from the #317 bridge commit: `mypy --strict` (run by both the lint job and the pack job) rejected `_hoisted_branch_locals`. It returns `(line, column, pool)` since 8a26fbd but its annotation still said `dict[str, tuple[int, bool]]`, so the caller's three-way unpack was an error at ownir.py:1197 and the comprehension building it was an error at 2327. The annotation now matches what the function has actually returned all along - a real type error, caught by the checker rather than by a test, which is what strict mode is for. The local run had been `mypy ownlang tests`, which is not the invocation CI uses; bare `mypy` (config-scoped to ownlang, --strict) is, and it is what was run to verify this. `cargo fmt --check` rejected `roundtrip.rs`: one `assert_eq!` over the line limit and the JSON-pointer table's trailing comments aligned by hand. rustfmt's layout is now in place. `cargo fmt --check`, `cargo clippy --all-targets` and `cargo test` - the three steps the rust job runs - all pass. (`own-cfg` emits one clippy warning under rustc 1.97.1; it reproduces on origin/main, is not gated by that job, and is untouched here.) Refs #317. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds optional 1-based source columns across Roslyn extraction, OwnIR facts, diagnostics, SARIF output, schemas, and validation. It updates subscription, fix, disposable, pooling, and flow-local anchors while preserving compatibility when columns are absent. ChangesSource-column propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ColumnAnchorsSample
participant OwnSharpExtractor
participant OwnIR
participant SARIF
ColumnAnchorsSample->>OwnSharpExtractor: source nodes with line and column positions
OwnSharpExtractor->>OwnIR: OwnIR facts with optional column
OwnIR->>OwnIR: validate and propagate column
OwnIR->>SARIF: finding region with startLine and startColumn
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…olden The S0 byte-parity gate compares the extractor's flag-off facts for `FixCandidatesSample.cs` against a golden frozen at `ff21d4a`. #317 makes every anchor record carry a `column`, so flag-off output legitimately changed - the case `tests/goldens/README.md` calls "an unrelated extractor change intentionally alters this sample's facts". What changed in the golden: 23 lines, all insertions, one `"column"` per subscription record, between `"line"` and `"released"`. Nothing else moved. The gate still protects what it was built to protect - this is not a re-recording of the current extractor's behaviour, it is the `ff21d4a` output plus one documented additive field. WHY THIS IS NOT A BLIND REGENERATION CI's own failure printed the diff, and that diff is real extractor output. Twelve `(line, column)` pairs are legible in it: 123->48, 133->46, 154->13, 171->13, 187->13, 190->35, 199->13, 200->13, 212->38, 213->42, 237->13, 254->13. The values here were derived from the sample's source text - a subscription record anchors on the event access, so `column` is that node's 1-based index on its line - and the derivation reproduces all twelve, exactly, with no mismatch. It spans both the indented case (13) and five distinct mid-line cases (35, 38, 42, 46, 48), so it is not a constant that happens to agree. The other eleven follow the same rule. That is a validated derivation, not a measured artifact. The next green run of this gate is what turns it into one; until then the README says so in as many words. WHAT CI HAS ALREADY CONFIRMED (run 30442571356, job 90545103519) The extractor compiled, and every step before the golden diff passed: * "Extractor source columns (Own.NET#317)" - GREEN. The four statically-derived coordinates in `ColumnAnchorsSample.cs`, (30,9) (30,31) (51,13) (51,45), are what real Roslyn emits, in both extractor modes. * `check_fix_candidates_facts.py` - GREEN, including the new cross-check that every subscription record's `column` equals its own fix block's `span.start_column`. Two independently computed coordinates for one node, in agreement. * The diff itself is evidence: every hunk in it is a lone `+ "column": N`. The change is additive in fact, not only by intent. Refs #317, #266. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
The note added with the golden update said the eleven values not legible in the failing log were still awaiting confirmation. Run 30443181114 (job 90547085334) confirmed them: that gate is a BYTE diff, so a green run settles all 23 at once, not only the twelve that had been read off a log. Recording it, because a hedge that has since been resolved is a stale claim. Refs #317. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
PhysShell
marked this pull request as ready for review
July 29, 2026 11:49
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/ownir.schema.json (1)
100-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
columndescription into a shared$defsentry.The same
columnproperty (type/minimum/description) is duplicated verbatim across 11 sites (resourceRecord, param, and every flowOp variant). A future wording fix or constraint change risks silently desyncing across some of these.♻️ Proposed refactor
"$defs": { + "sourceColumn": { + "description": "1-based source column of the SAME node `line` anchors on. Optional and additive, so OWNIR_VERSION stays 0 (spec/OwnIR.md §2): a producer that does not report one omits it and no consumer substitutes a value — not 0, not 1, and never recovered by re-reading the source line. Rides to SARIF as region.startColumn, where the OwnAudit occurrence anchor reads it (Own.NET#317).", + "type": "integer", + "minimum": 1 + }, "resourceKind": { ... },Then each occurrence collapses to:
- "column": { "description": "1-based source column ... (Own.NET#317).", "type": "integer", "minimum": 1 }, + "column": { "$ref": "`#/`$defs/sourceColumn" },Also applies to: 167-167, 182-182, 194-194, 205-205, 216-216, 227-227, 238-238, 250-250, 264-264, 276-276
🤖 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 `@spec/ownir.schema.json` at line 100, Define one shared `$defs` entry for the `column` schema containing the existing description, integer type, and minimum of 1. Replace the duplicated inline `column` definitions in `resourceRecord`, `param`, and all flowOp variants with references to that shared definition, preserving the optional property behavior.tests/test_ownir_column.py (1)
80-94: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_write's temp files are never cleaned up.Every call to
_raises(8x in this run) leaves an orphaned*.facts.jsonfile behind —_writenever deletes what it creates.🧹 Proposed fix
def _raises(obj: dict) -> bool: + path = _write(obj) try: - load(_write(obj)) + load(path) except OwnIRError: return True except Exception: return False - return False + else: + return False + finally: + os.remove(path)🤖 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 `@tests/test_ownir_column.py` around lines 80 - 94, Update the test helper flow around _write and _raises so every temporary *.facts.json file is deleted after load completes, including when load raises an exception. Preserve _raises’s existing OwnIRError and generic-exception return behavior while ensuring cleanup occurs reliably.
🤖 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 `@tests/fixtures/ownir/flow_column_anchors.facts.json`:
- Around line 89-95: Update the sig field for the Factory.Create call in the
flow_column_anchors fixture to use the canonical empty parameter-type list for
this zero-argument invocation. Leave the callee, arguments, result, and
source-location fields unchanged.
---
Nitpick comments:
In `@spec/ownir.schema.json`:
- Line 100: Define one shared `$defs` entry for the `column` schema containing
the existing description, integer type, and minimum of 1. Replace the duplicated
inline `column` definitions in `resourceRecord`, `param`, and all flowOp
variants with references to that shared definition, preserving the optional
property behavior.
In `@tests/test_ownir_column.py`:
- Around line 80-94: Update the test helper flow around _write and _raises so
every temporary *.facts.json file is deleted after load completes, including
when load raises an exception. Preserve _raises’s existing OwnIRError and
generic-exception return behavior while ensuring cleanup occurs reliably.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ceea9bd-5e47-411b-8624-9c7a2cc1cea7
📒 Files selected for processing (13)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/ColumnAnchorsSample.csownlang/ownir.pyrust/crates/own-ir/tests/roundtrip.rsspec/OwnIR.mdspec/ownir.schema.jsontests/check_fix_candidates_facts.pytests/fixtures/ownir/flow_column_anchors.facts.jsontests/goldens/README.mdtests/goldens/fix_candidates_off.golden.jsontests/test_extractor_columns.pytests/test_ownir_column.py
…leanup
Three CodeRabbit findings, all verified against the code first and all real.
`tests/fixtures/ownir/flow_column_anchors.facts.json` recorded `"sig": "Create()"`
for a zero-argument call. `sig` is the canonical parameter-TYPE list, not a call
spelling: `CanonicalSig` in the extractor documents `""` for zero parameters, and
`_sig_key` builds `f"{name}({sig})"` - so the fixture's value would have produced the
key `Factory.Create(Create())`, which resolves to no `functions[]` record at all. The
verdict here does not change (nothing declares `Factory.Create`, so the call is
not-known-fresh either way, which is what the fixture's own doc says it tests), but a
fixture recording a shape the producer cannot emit is the same defect this PR is
about. The Rust round-trip scans this directory and would have carried it forward.
`spec/ownir.schema.json` had the `column` schema inlined 11 times, byte-identical.
Extracted to `$defs/sourceColumn` and referenced - the file already keeps 26 `$ref`s
against `$defs`, so this is the file's own convention, not a new one. The 11 copies
were a deliberate choice at the time (a first pass had reformatted the whole file, and
surgical insertions were the way back from that); the copies are still the wrong shape
now that they are all in place, because a wording or constraint change would have to
land in eleven places or silently desync.
`tests/test_ownir_column.py::_write` created a `tempfile.mkstemp` file that nothing
removed, so every `_raises` call - eight per run - orphaned one. Cleanup is in a
`finally`, not after the `load`: this helper exists to run inputs that RAISE, so the
raising path is every path.
Verified: `ruff check .` clean, `mypy` (strict) clean, full Python suite green and
again under -O, 36/36 column contract, `cargo fmt --check` clean, `cargo test -p
own-ir` 11/11 including `optional_column_survives_the_round_trip` over the amended
fixture, and a run of the column contract now leaves zero new files in TMPDIR.
Refs #317.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015de4MezSeUnZBoWq1fFU5M
Owner
Author
|
@coderabbitai review The follow-up pass on Generated by Claude Code |
|
✅ Action performedReview finished.
|
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
3 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.
Что и зачем
Own.NET#317 (дочерняя к #266): own-check не сообщал колонку вообще, поэтому каждый его finding якорился только по строке, а
finding-occurrence/v1в OwnAudit анкорит поpath + startLine + startColumn. PR проводит колонку насквозь: Roslyn-узел -> OwnIR-факт ->Finding-> SARIFregion.startColumn. Точность физического якоря, не coverageoccurrence_id— тот упирается в run identity, а не в колонку.PR был открыт как draft потому, что
dotnetв среде разработки недоступен и экстрактор локально не компилировался: проверка была отдана CI. Она состоялась — см. ниже.Тип изменения
Как проверено
Локально:
python tests/run_tests.py(и под-O),ruff check .,mypy(strict, конфиг-скоупownlang),tests/test_ownir_column.py36/36,tests/test_ownir.py282/282,cargo fmt --check+cargo clippy --all-targets+cargo test(own-ir 11/11, включаяoptional_column_survives_the_round_trip).Что подтвердил CI
Полностью зелёный прогон. Содержательно:
ColumnAnchorsSample.cs—(30,9) (30,31) (51,13) (51,45)— это то, что реально испускает Roslyn, в обоих режимах экстрактора.check_fix_candidates_facts.py— зелёный, включая новый кросс-чек:record.column == fix.span.start_columnдля каждой подписки. Две независимо вычисленные координаты одного узла совпали.fix_candidates_off.golden.json— то, что экстрактор действительно испускает.Как получен golden: первый прогон упал на нём ровно как ожидалось, и его дифф оказался данными — каждый hunk одиночная вставка
+ "column": N, и в нём читались двенадцать настоящих пар(line, column). Колонки выведены из текста сэмпла и сверены с этими двенадцатью (12 совпадений, 0 расхождений; покрыты и отступной случай 13, и пять разных середин строки — 35, 38, 42, 46, 48). Остальные одиннадцать подтвердил уже байтовый гейт. Вся эта хронология записана вtests/goldens/README.md, включая то, что файл — по-прежнему выводff21d4aплюс одно задокументированное аддитивное поле, а не перезапись поведения текущего экстрактора.Ревью
Три замечания CodeRabbit, все проверены по коду и все верные — исправлены в
fd30d20: неканоничныйsigнулевой арности в фикстуре ("Create()"вместо""дал бы ключFactory.Create(Create()), не резолвящийся ни во что); одиннадцать байт-идентичных инлайн-копий схемыcolumn(вынесены в$defs/sourceColumn— файл уже держит 26$refна$defs);_writeне удалял своиmkstemp-файлы.Устройство
Один источник координаты.
LineOf(node)теперь проекцияPosOf(node)=RangeOf(node).Start— единственный вызовGetLineSpan(). ПараллельногоColumnOf(node)намеренно нет: два независимых обращения — это две возможности передать два разных узла, а результат будет корректно оформленной координатой, указывающей на несуществующее место.FixSpanOfпереписан через тот жеRangeOf; его сериализуемая форма не изменилась — те же шесть полей, тот же порядок, те же значения.SourcePos/SourceRangeобъявлены внутри существующегоpartial class Programв конце файла.Три носителя координаты, а не только точки испускания:
dispoFieldLineсталdispoFieldPos— словарь, чьё значение теперьSourcePos, а неint;pooledFieldRent— то же; кортеж вrentedнесётSourcePosвместоint Line.releasedAt,helperReads,useLine,vlineне тронуты сознательно — это release/use/view-сайты, события на пути, а не запись, на которой якорится finding.18 записей получают колонку: 7 flow-операций, минтящих handle (два
using-acquire, прямой acquire,alias_join, результат fresh-возвращающегоcall, синтетические acquire для disposed-поля и pooled-поля), и 11 формsubs.Add(четыре варианта+=— одинPosOf(a.Left)на все четыре, чтобы они не могли разойтись; unresolved-подписка; declared weak-subscribe; disposable-поле с[OwnIgnore]и без; брошенный Rx-токен; POOL001-rent; D1 local-disposable). Остальные операции сохраняют только строку.OWN025 остаётся
column=Noneявно: его строка — сайт нарушения, а колонка acquire принадлежит другому узлу. Смешанные координаты — единственная ошибка, ради предотвращения которой всё это и сделано.Фикстура
frontend/roslyn/samples/ColumnAnchorsSample.csкладёт ДВА якорных сайта на ОДНУ строку в каждом семействе. Все остальные сэмплы в каталоге дают один finding на строку, поэтому колонка, всегда равная 1, или отступу, или первому сайту, прошла бы их все.OWNIR_VERSIONостаётся 0: опциональное поле с безопасным умолчанием аддитивно (spec/OwnIR.md§2).Четыре code-scanning алерта на строках 30 и 51 фикстуры — намеренные утечки, ради которых она и написана.
Связанные issue
Refs #317, #266. Не закрывает #317: после мержа нужно перемерить корпус двумя метриками (coverage
occurrence_id; coveragestartColumnпо продюсерам) с дифференциальным отчётом, отделяющим «та же строка + null -> та же строка + колонка» от любых других семантических изменений (вторая категория должна быть пустой).Чеклист
tests/test_extractor_columns.py— новый;tests/test_ownir_column.py— 36 проверок; кросс-чек вcheck_fix_candidates_facts.py; Rust round-trip)spec/OwnIR.md§4.1 (нормативный),spec/ownir.schema.json,tests/goldens/README.mdSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests