PoC polish: gallery, rustc-style diagnostics, real .NET compile in CI, real-world corpus#6
Merged
Merged
Conversation
Make the value visible in a minute. examples/gallery/ holds ten tiny, real-shaped programs -- each trips exactly one ownership/borrow diagnostic and carries the real C# analog in a comment (forgot Dispose() on an error path -> OWN001, ArrayPool.Return while a Span is live -> OWN008, returning a Span over a stackalloc -> OWN015, ownership laundered through an opaque call -> OWN040, ...), plus one clean happy-path. tests/test_gallery.py pins every file to the code it advertises and prints the gallery table, so the demo can never drift from what the checker actually does; it runs as part of the suite. README gets a "what it catches" section.
Make errors readable at a glance. `check`/`emit` now render each diagnostic as
a `file:line:col` header, the offending source line with a gutter, and a caret
under the named identifier:
examples/gallery/05_dispose_while_view_live.own:9:13: error: [OWN008] cannot release 'b' while it is borrowed
9 | release b; // freeing the backing store while `view` is alive
^
The column/caret is derived at render time from the source line and the
identifier the message names (whole-word match, so 'a' lands on the argument in
`Hash(a)`, not the 'a' inside `Hash`); it falls back to the plain header when it
can't be resolved. No AST/CFG threading, so nothing in the checker pipeline
changes -- this is presentation only. Added a render smoke to the gallery test.
The PoC sandbox has no .NET SDK, so the golden lowering was only "verified by construction." GitHub Actions does have .NET, so add a `dotnet-golden` job: set up the SDK, check the `process` method in examples/golden_arraypool/ Program.cs is still byte-for-byte what `ownlang emit buffer.own` produces (verify_emit.py), then drop it into a console project and `dotnet run` it. This closes the "not actually compiled/executed" caveat for the showcase example -- on every push the real compiler confirms the ArrayPool Rent/Return + try/finally lowering compiles and runs. (The sync guard is testable locally; the build/run step is exercised only in CI.)
Prove the checker maps onto real bugs, not synthetic ones. corpus/real-world/
holds reductions of real .NET ArrayPool/Dispose ownership bugs, each as
before.cs / after.cs / case.own / expected-diagnostics.txt / notes.md:
* arraypool-use-after-return -> OWN002 (rented buffer Returned, then a slice
of it still read -- the classic dotnet/runtime buffer-pooling bug)
* arraypool-double-return -> OWN003 (same array Returned twice;
dotnet/runtime#33767)
tests/test_corpus.py runs every case.own and asserts its diagnostics match the
expected file, wired into the suite -- if the checker ever stops catching one of
these patterns, CI goes red.
Honesty is in notes.md and the corpus README: case.own is a hand reduction of
the C# pattern, not C# the checker ingested (no C# front-end yet), so it shows
the ownership *logic* maps onto real bugs. README also lists what is NOT
expressible (Span over-clear, exception-path-only leaks, concurrency, async).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 rustc-style pretty diagnostic rendering ( ChangesGallery Fixtures, Corpus, Pretty Diagnostics, and CI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
corpus/real-world/arraypool-use-after-return/notes.md (1)
14-19: ⚡ Quick winAdd language identifier to fenced code block.
The code block starting at line 14 should specify a language identifier for proper syntax highlighting and lint compliance.
📝 Proposed fix
-``` +```text $ python -m ownlang check corpus/real-world/arraypool-use-after-return/case.own case.own:14:14: error: [OWN002] borrow 'quotient' after it was released 14 | BuildResult(quotient); // read after return -> OWN002🤖 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 `@corpus/real-world/arraypool-use-after-return/notes.md` around lines 14 - 19, The fenced code block in the notes.md file that contains the shell command output and error message is missing a language identifier. Add a language identifier (such as "text" or "bash") immediately after the opening triple backticks of this code block to enable proper syntax highlighting and meet lint compliance requirements for the markdown file.Source: Linters/SAST tools
tests/test_corpus.py (1)
74-75: ⚡ Quick winClose file handles explicitly.
The files are opened but never closed, creating a resource leak. Use context managers to ensure proper cleanup.
♻️ Proposed fix
- want = sorted(w for w in open(exp, encoding="utf-8").read().split() if w) - got = sorted(set(_codes(open(own, encoding="utf-8").read()))) + with open(exp, encoding="utf-8") as f: + want = sorted(w for w in f.read().split() if w) + with open(own, encoding="utf-8") as f: + got = sorted(set(_codes(f.read())))🤖 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_corpus.py` around lines 74 - 75, The file handles opened in the expressions for both the `want` variable assignment and the `got` variable assignment are not being closed, causing a resource leak. Wrap each `open()` call with a context manager (using a with statement) to ensure the files are properly closed after reading. For the `want` assignment, use a with statement to open the `exp` file and read its contents. For the `got` assignment, use a with statement to open the `own` file and pass its contents to the `_codes()` function. This ensures both file handles are automatically closed when the context manager exits.
🤖 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 @.github/workflows/ci.yml:
- Around line 54-58: The GitHub Actions in the workflow are using version tags
(v4, v5, v4) instead of being pinned to specific commit SHAs, which is a
security best practice. Additionally, the actions/checkout action should include
persist-credentials: false to reduce token exposure risk. Replace the version
tag references for actions/checkout@v4, actions/setup-python@v5, and
actions/setup-dotnet@v4 with their full commit SHA pins, and add the
persist-credentials: false configuration to the actions/checkout step to
minimize security vulnerabilities.
In `@examples/gallery/08_stack_buffer_escapes.own`:
- Around line 1-8: The code in the make function uses Buffer.stack(64) but the
Buffer resource type is never declared in the module. Add a resource declaration
for Buffer at the module level (after the module Gallery declaration and before
the make function definition) that specifies the acquire, rent, release, and
give operations, following the pattern shown in the referenced
09_untracked_call.own file, so that the parser recognizes Buffer as a valid
resource type and the OWN015 diagnostic can be properly triggered.
In `@examples/golden_arraypool/verify_emit.py`:
- Around line 24-39: The verifier is applying `.strip()` to lines when
extracting the process method (line 32: out.append(l.strip())), which normalizes
whitespace and allows formatting drift to pass despite the intent to verify
verbatim, byte-for-byte matching. Remove the `.strip()` call from the line
append operation so that the original formatting is preserved exactly. Apply the
same fix to the other location where this pattern occurs (lines 46-50 as noted
in the comment), removing any `.strip()` normalization there as well. Adjust the
final filtering logic if needed to handle unstripped lines correctly.
- Around line 26-27: The loop variable `l` (lowercase L) is ambiguous and
violates Ruff E741. Rename this variable to a clearer name like `line` at all
three locations in examples/golden_arraypool/verify_emit.py: in the generator
expression at lines 26-27, in the loop at lines 31-33, and in the expression at
line 46. Replace all instances of the ambiguous `l` variable with the new
descriptive name consistently across all three occurrences.
In `@README.md`:
- Around line 89-94: The fenced code block in README.md starting at line 89 is
missing a language identifier, which triggers the MD040 markdown linting rule.
Add a language tag "text" to the opening backticks of this code block that
contains the command-line output example showing the ownlang error message.
In `@tests/test_corpus.py`:
- Around line 86-87: The success-count formula in the print statement uses
len(rows) - len(fails) which is incorrect because when a case has a missing
file, it is added to fails but never added to rows (due to the continue
statement), causing undercounting of successes. Fix this by explicitly tracking
two values: the number of cases actually checked/runnable (those that don't hit
the missing file continue) and the number of those that matched their expected
diagnostics. Then update the print statement to use these tracked counts instead
of the subtraction formula.
In `@tests/test_gallery.py`:
- Line 103: The `max()` call in the line that calculates `width` will raise a
`ValueError` if `rows` is empty (when all files in MANIFEST are missing). Add a
`default` parameter to the `max()` function call to provide a fallback value
when the rows iterable is empty, making the code more robust against this edge
case.
- Around line 112-113: The success count calculation in the print statement at
lines 112-113 incorrectly includes "render_pretty" failures (from _render_smoke)
in the set of failed gallery examples. Filter the fails iterable when building
the set to only include failures that correspond to actual gallery filenames,
not the "render_pretty:" prefixed failures. This ensures that _render_smoke
failures do not decrement the gallery example success count.
---
Nitpick comments:
In `@corpus/real-world/arraypool-use-after-return/notes.md`:
- Around line 14-19: The fenced code block in the notes.md file that contains
the shell command output and error message is missing a language identifier. Add
a language identifier (such as "text" or "bash") immediately after the opening
triple backticks of this code block to enable proper syntax highlighting and
meet lint compliance requirements for the markdown file.
In `@tests/test_corpus.py`:
- Around line 74-75: The file handles opened in the expressions for both the
`want` variable assignment and the `got` variable assignment are not being
closed, causing a resource leak. Wrap each `open()` call with a context manager
(using a with statement) to ensure the files are properly closed after reading.
For the `want` assignment, use a with statement to open the `exp` file and read
its contents. For the `got` assignment, use a with statement to open the `own`
file and pass its contents to the `_codes()` function. This ensures both file
handles are automatically closed when the context manager exits.
🪄 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: 84197ffd-07f0-4a31-b5fe-8e90664ff0c6
📒 Files selected for processing (29)
.github/workflows/ci.ymlREADME.mdcorpus/real-world/README.mdcorpus/real-world/arraypool-double-return/after.cscorpus/real-world/arraypool-double-return/before.cscorpus/real-world/arraypool-double-return/case.owncorpus/real-world/arraypool-double-return/expected-diagnostics.txtcorpus/real-world/arraypool-double-return/notes.mdcorpus/real-world/arraypool-use-after-return/after.cscorpus/real-world/arraypool-use-after-return/before.cscorpus/real-world/arraypool-use-after-return/case.owncorpus/real-world/arraypool-use-after-return/expected-diagnostics.txtcorpus/real-world/arraypool-use-after-return/notes.mdexamples/gallery/00_ok_clean.ownexamples/gallery/01_leak_on_error_path.ownexamples/gallery/02_use_after_release.ownexamples/gallery/03_double_release.ownexamples/gallery/04_use_after_move.ownexamples/gallery/05_dispose_while_view_live.ownexamples/gallery/06_exclusive_while_shared.ownexamples/gallery/07_use_after_handoff.ownexamples/gallery/08_stack_buffer_escapes.ownexamples/gallery/09_untracked_call.ownexamples/golden_arraypool/verify_emit.pyownlang/__main__.pyownlang/diagnostics.pytests/run_tests.pytests/test_corpus.pytests/test_gallery.py
Comment on lines
+54
to
+58
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.13" | ||
| - uses: actions/setup-dotnet@v4 |
There was a problem hiding this comment.
Pin GitHub Actions by commit SHA and disable checkout credential persistence.
Line 54 / Line 55 / Line 58 are unpinned action refs, and Line 54 should set persist-credentials: false to reduce token exposure risk.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 54-54: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 54-54: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 55-55: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 58-58: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ci.yml around lines 54 - 58, The GitHub Actions in the
workflow are using version tags (v4, v5, v4) instead of being pinned to specific
commit SHAs, which is a security best practice. Additionally, the
actions/checkout action should include persist-credentials: false to reduce
token exposure risk. Replace the version tag references for actions/checkout@v4,
actions/setup-python@v5, and actions/setup-dotnet@v4 with their full commit SHA
pins, and add the persist-credentials: false configuration to the
actions/checkout step to minimize security vulnerabilities.
Source: Linters/SAST tools
Owner
Author
There was a problem hiding this comment.
Skipping for now (deliberate). SHA-pinning every action is a repo-wide policy choice that's better maintained by Dependabot than hand-pinned in one PR, and these jobs only checkout + run tests — no push, no secrets — so the credential-persistence exposure is minimal. Happy to do a dedicated hardening pass (full SHA pins + persist-credentials: false) if you'd like that as a standing policy.
Generated by Claude Code
There was a problem hiding this comment.
@PhysShell, that's a reasonable call — Dependabot's actions ecosystem update is exactly the right layer for maintaining SHA pins at scale, and the exposure here really is low without push permissions or secrets in scope.
Would you like me to open a follow-up GitHub issue to track the dedicated hardening pass (full SHA pins + persist-credentials: false across all workflow jobs)? That way it won't get lost once the PR merges.
✏️ Learnings added
Learnt from: PhysShell
Repo: PhysShell/Own.NET PR: 6
File: .github/workflows/ci.yml:54-58
Timestamp: 2026-06-14T16:16:25.503Z
Learning: In the PhysShell/Own.NET repository, SHA-pinning of GitHub Actions (e.g., actions/checkout, actions/setup-python, actions/setup-dotnet) is treated as a repo-wide policy decision to be handled by Dependabot rather than hand-pinned in individual PRs. Adding `persist-credentials: false` to checkout steps is also deferred to a dedicated hardening pass, not enforced per-PR. Do not flag unpinned action refs or missing persist-credentials as blocking issues in this repo.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Legit fixes: - verify_emit.py: compare the process() method byte-for-byte (drop the whitespace normalization that allowed drift); rename ambiguous `l` -> `line` (Ruff E741); close file handles via `with`. - tests/test_corpus.py, tests/test_gallery.py: close file handles via `with`; fix the success-count math (track checked/matched; don't count a render_pretty smoke failure as a gallery-example failure); `max(..., default=0)`. - README.md + corpus notes.md: add `text` language tag to fenced blocks (MD040). - Short docstrings on the new test helpers / Diagnostic.render (coverage). Intentionally skipped (with reason): - gallery 08 "missing `resource Buffer`": false positive. `Buffer.stack(64)` is built-in buffer-intent syntax, not a resource method; the file produces exactly OWN015 (asserted by test_gallery). - Pinning CI actions to commit SHAs: a repo-policy/Dependabot decision, overkill for this PoC (the jobs check out + run tests, they don't push or use secrets). Suite green: analysis 124/124, gallery 10/10, corpus 2/2, fuzzer clean.
The gallery summary used `len(rows) - len(example_fails)`, but a missing file is appended to `fails` and `continue`d (never added to `rows`), so subtracting it from `rows` undercounted by one — the same class of bug CodeRabbit caught in test_corpus.py but which the gallery's render_pretty-only fix didn't cover. Track `matched` explicitly (an example counts only when it's found AND its codes match), report `matched/len(MANIFEST)`. Verified: a missing file now reports 10/11 (was 9/11), and a render-smoke failure with all examples passing reports 10/10 (was 9/10), both still returning non-zero.
Update the 'where it cheats' section: the golden lowering is no longer only 'verified by construction' -- the dotnet-golden CI job compiles and runs it with the real .NET compiler. Add a known-issue item: CI actions are pinned by tag, not commit SHA, and don't set persist-credentials: false (zizmor flags this); deliberately deferred to Dependabot / a dedicated hardening pass rather than hand-pinned per PR, since the jobs only checkout + run tests (no push, no secrets).
PhysShell
pushed a commit
that referenced
this pull request
Jun 15, 2026
This was referenced Jun 16, 2026
PhysShell
pushed a commit
that referenced
this pull request
Jul 3, 2026
Both reviewers converged; all corrections are factual/design refinements (no forks): - Crate DAG: own-codegen is a SIBLING of own-diagnostics (both consume own-cfg/own-analysis), not chained through it. Codegen is verdict-independent (AST/CFG-driven, matching Python codegen.generate), so the old own-diagnostics -> own-codegen edge would have forced diagnostics to re-export solver internals. Added a fitness function locking codegen !-> diagnostics (and !-> analysis for now). (Codex P2, CodeRabbit #1) - Oracle: do NOT reuse scripts/oracle_compare.py as the parity oracle — it is cross-tool fuzzy (leak-only, +-N line tolerance, coarse severity) and would mask off-by-one/label/subject/exit-status divergences. Spec a new exact harness over status+stdout+stderr+SARIF/JSON, with an exit/crash gate first, exact set-equality incl. evidence label text, and intra-tie ordering. (Codex P1, CodeRabbit #4) - CFG seam does not exist yet: python cfg prints human text, not JSON. Add+freeze a canonical cfg --format json before the ratchet uses that seam; added as migration step 0. (Codex P2) - State: arena+CoW likely wins this procedural workload; bench largest real function, wall-clock + RSS. Prior art: clippy lint-pass registry, prusti-viper encoding boundary. Repo-layout revisit trigger. (CodeRabbit #2/#3/#5/#6)
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.
Brings the ownership checker to a presentable, trustworthy PoC. Four commits:
1. "What it catches" gallery (
examples/gallery/) — ten tiny, real-shaped programs, each tripping exactly one ownership/borrow diagnostic with the real C# analog in a comment (forgotDispose()on an error path → OWN001,ArrayPool.Returnwhile aSpanis live → OWN008, returning aSpanover astackalloc→ OWN015, ownership laundered through an opaque call → OWN040, …).tests/test_gallery.pypins every file to its code so the demo can't drift.2. rustc-style diagnostics —
check/emitnow printfile:line:col, the offending source line, and a caret under the named identifier:Column/caret are derived at render time from the source + the identifier the message names (whole-word match). Presentation only — no AST/CFG threading, the checker pipeline is untouched.
3. Real .NET compile in CI (
dotnet-goldenjob) — the sandbox has no SDK, so the golden lowering was only "verified by construction". GitHub Actions does: set up .NET, check theprocessmethod inProgram.csis still byte-for-byte whatownlang emitproduces, thendotnet runit. Closes the "not actually compiled/executed" caveat — already passing on this branch's push.4. Self-checking real-world corpus (
corpus/real-world/) — reductions of real .NET ArrayPool bugs, each as before.cs / after.cs / case.own / expected-diagnostics.txt / notes.md:arraypool-use-after-return→ OWN002 (rented buffer Returned, then a slice still read)arraypool-double-return→ OWN003 (same array Returned twice; Do not double-return arrays to ArrayPool dotnet/runtime#33767)tests/test_corpus.pyasserts each case still trips its documented code. Honesty (in notes.md + corpus README):case.ownis a hand reduction of the C# pattern, not C# the checker ingested — it shows the ownership logic maps onto real bugs. The README also lists what is NOT expressible (Span over-clear, exception-path-only leaks, concurrency, async).Full suite green locally (analysis 124/124, gallery 10/10, corpus 2/2, fuzzer clean); all five CI jobs green on the branch push, including the new real-compiler job.
Generated by Claude Code
Summary by CodeRabbit
file:line:col, source context, and caret highlighting.