feat(cli): realize the extract/check/explain surface (explain in core, extract verb)#142
Merged
Merged
Conversation
…, extract verb) Completes the advertised three-verb CLI from the roslyn-tools note, split to where the architecture puts each — one checker, the C# tool only emits facts: - explain (core): `python -m ownlang explain OWN001 [DI002 ...]` prints what a code means, why it fires, and how to fix it; `--json <findings|sarif>` harvests every distinct code (code/ruleId keys) and explains them. New EXPLANATIONS catalogue + DI001-005 titles in diagnostics.py. Exit 2 on no-codes / all-unknown / unreadable JSON. Tests in tests/test_explain.py (folded into run_tests.py). - extract (C# tool): optional leading `extract` verb (bare form stays default) and `--out` as the long twin of `-o`, so `ownsharp-extract extract --project App.csproj --out facts.json` works as advertised. Backward compatible. - check: documented as the own-check.sh orchestrator (already chains extractor + core and takes a .csproj/.sln) rather than a second checker in C#. CI: extract-verb fact-parity (bare == --project == `extract --out`) + an explain smoke (code + --json harvest from a real SARIF). Docs (note + READMEs) updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzA6KGQVRzP52tBEKwJ3SW
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds an ChangesCLI surface updates
Sequence Diagram(s)sequenceDiagram
participant User
participant Main as ownlang.__main__.main()
participant Cmd as cmd_explain()
participant Diagnostics as ownlang.diagnostics
participant SARIF as SARIF JSON
User->>Main: python -m ownlang explain OWN001 --json findings.sarif
Main->>Cmd: route explain arguments
Cmd->>SARIF: read code and ruleId values
Cmd->>Diagnostics: look up TITLES and EXPLANATIONS
Cmd->>User: print explanation text and exit status
Review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (2)
ownlang/diagnostics.py (1)
97-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a long-form
EXPLANATIONS["DI001"]for parity.DI001is emitted by the bridge and asserted in CI, but unlikeDI002–DI005it has no long-form entry, soexplain DI001answers with only the one-line title via the fallback. SinceDI001is the umbrella "captive dependency" verdict a user is most likely to look up, a what/why/fix paragraph would round out the catalogue. Functional today thanks to the title fallback, so this is optional.🤖 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 `@ownlang/diagnostics.py` around lines 97 - 164, Add a long-form EXPLANATIONS entry for DI001 in diagnostics.py so it matches the DI002–DI005 pattern. Update the EXPLANATIONS mapping with a descriptive multi-sentence explanation for the DI001 captive-dependency verdict, including what it means and how to fix it, and keep the existing DI001 title behavior intact so explain DI001 returns the richer text instead of only the fallback..github/workflows/ci.yml (1)
1013-1015: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
|| truecan mask a SARIF-generation failure. Ifownir ... --format sariffails,$RUNNER_TEMP/proj.sarifis left empty/partial and the failure is swallowed; the nextgrepstill fails, but with a misleading "did not harvest OWN001" message rather than the real generation error. Consider dropping|| truehere (the previousproj-facts.jsonstep already validated the facts), so a genuine SARIF emit regression surfaces directly.🤖 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 1013 - 1015, The SARIF generation step is swallowing real failures because `ownir` is followed by `|| true`, which can leave `proj.sarif` empty or partial and produce a misleading later `grep` failure. Remove the `|| true` from the workflow command so a genuine emit regression fails immediately, and keep the existing `ownir`/`explain --json` check sequence to validate the SARIF output directly.
🤖 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 `@docs/notes/roslyn-tools-and-cli.md`:
- Around line 127-130: The example for the `ownlang explain --json` command is
pointing at the wrong artifact; update the docs so `explain` uses checker output
such as findings or SARIF JSON rather than `facts.ownir.json` from
`ownsharp-extract`. Keep the surrounding examples (`ownsharp-extract`,
`scripts/own-check.sh`, and `python -m ownlang explain`) but replace the
`--json` target with the checker-produced JSON source that contains the
diagnostic codes `explain` reads.
In `@frontend/roslyn/README.md`:
- Around line 44-49: The README example for `ownlang explain --json` points at
`facts.json`, but `explain --json` expects diagnostic codes from findings or
SARIF JSON. Update the example under the `extract`/`explain` description to
reference the checker’s findings/SARIF artifact instead, and keep the guidance
aligned with the `ownlang explain` command so users are directed to a valid JSON
input.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 1013-1015: The SARIF generation step is swallowing real failures
because `ownir` is followed by `|| true`, which can leave `proj.sarif` empty or
partial and produce a misleading later `grep` failure. Remove the `|| true` from
the workflow command so a genuine emit regression fails immediately, and keep
the existing `ownir`/`explain --json` check sequence to validate the SARIF
output directly.
In `@ownlang/diagnostics.py`:
- Around line 97-164: Add a long-form EXPLANATIONS entry for DI001 in
diagnostics.py so it matches the DI002–DI005 pattern. Update the EXPLANATIONS
mapping with a descriptive multi-sentence explanation for the DI001
captive-dependency verdict, including what it means and how to fix it, and keep
the existing DI001 title behavior intact so explain DI001 returns the richer
text instead of only the fallback.
🪄 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: fb886c6c-f8bb-4b5c-aec8-a205dfad049a
📒 Files selected for processing (8)
.github/workflows/ci.ymldocs/notes/roslyn-tools-and-cli.mdfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/README.mdownlang/__main__.pyownlang/diagnostics.pytests/run_tests.pytests/test_explain.py
…m, CI rc-aware (CodeRabbit) Three CodeRabbit review points on the explain/extract PR: - docs (Minor, correctness): `explain --json` harvests diagnostic *codes* (code/ ruleId), which `facts.ownir.json` (extractor facts) does not carry — the note + README examples now point `--json` at the checker's findings/SARIF output and call out the distinction. - diagnostics.py (Trivial): add a long-form EXPLANATIONS["DI001"] so the umbrella captive-dependency verdict gets a what/why/fix paragraph like DI002-DI005, instead of only the title fallback. - ci.yml (Trivial): the SARIF step's blanket `|| true` could mask a real emit regression. ownir exits 1 when findings exist (expected — the sample leaks), so capture the rc and tolerate <=1 while surfacing 2+ (a genuine error). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzA6KGQVRzP52tBEKwJ3SW
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ownlang/diagnostics.py`:
- Around line 104-105: The lifetime-fix guidance in diagnostics.py should not
present Func<T> as a default Microsoft.Extensions.DependencyInjection solution.
Update the relevant messages around the diagnostics text to recommend
IServiceScopeFactory and per-use scope creation as the primary fix, and only
mention Func<T> if it is clearly qualified as container-dependent or manually
registered. Make the wording consistent in the affected diagnostics entries so
it does not imply Func<T> is auto-resolved by the built-in container.
🪄 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: c98850af-ab3f-4afe-abb1-a22292eb76f4
📒 Files selected for processing (4)
.github/workflows/ci.ymldocs/notes/roslyn-tools-and-cli.mdfrontend/roslyn/README.mdownlang/diagnostics.py
✅ Files skipped from review due to trivial changes (1)
- frontend/roslyn/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- docs/notes/roslyn-tools-and-cli.md
…nc<T> (CodeRabbit) The built-in Microsoft.Extensions.DependencyInjection container does not auto-resolve Func<T> (it needs manual registration or a third-party container), so the DI001/DI002/DI003 explanation text no longer presents Func<T> as the default fix. Primary guidance is now IServiceScopeFactory + per-use scope creation; Func<T> is mentioned only as a registered/container-dependent option. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzA6KGQVRzP52tBEKwJ3SW
8 tasks
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.
Что и зачем
Follow-up to #139 (the roslyn-tools note's re-aimed "Next PR"). It completes the advertised three-verb CLI —
extract/check/explain— but split to where the architecture puts each, not bolted onto one binary. The "one checker" rule holds: the C# tool only emits facts; the core owns diagnostics.explain→ the core (python -m ownlang explain OWN001), next to the diagnostic catalogue. Prints what a code means, why it fires, and how to fix it.--json <findings|sarif>harvests every distinct code (code/ruleIdkeys) so you can explain exactly what a run produced. NewEXPLANATIONScatalogue +DI001–005titles indiagnostics.py.extract→ the C# tool gets an optional leadingextractverb (bare form stays the default) and--outas the long twin of-o, soownsharp-extract extract --project App.csproj --out facts.jsonworks as advertised. Backward compatible.check→ documented as theown-check.shorchestrator, which already chains extractor + core and (since feat(extractor): accept .csproj/.sln input (roslyn-tools CLI-first project resolution) #139) accepts a.csproj/.sln. Re-implementing a checker in C# stays the rejected path.Тип изменения
explaincommand +extractverbПроверка
explainis fully exercised locally (the core is Python): newtests/test_explain.py(folded intorun_tests.py) covers the text builder, exit-code contract (no-codes / all-unknown / unreadable JSON → 2),--jsonharvest from findings arrays and SARIF logs (de-duped, first-seen), and DI-catalogue coverage. Whole suite green.ruff+mypy --strictclean on the changed files.ownir … --format sarif→explain --json <sarif>surfacesOWN001.extractverb /--outbuilds only in CI (no localdotnet); awpf-extractorstep assertsbare == --project == extract --outat the OwnIR fact boundary, plus anexplainsmoke.Связанный issue
Нет — продолжение #139 /
docs/notes/roslyn-tools-and-cli.md.Чеклист
tests/run_tests.py, incl. newtest_explain) + ruff + mypy --strictexplainв ядре, C# только эмитит фактыNotes
A
System.CommandLinemigration of the C# tool (auto--help, validation) and--ref-dir-from-project-binauto-derivation are the remaining polish — deliberately deferred over a blind framework swap, since the extractor compiles only in CI here.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
ownlang explainto show diagnostic titles, why they appear, and suggested fixes, including bulk explanation via--jsonfrom findings/SARIF.extractto optionally accept a leadingextractverb and to support--out/-ofor the facts file.extract,check, andexplain.explainargument validation and exit-code behavior for known vs unknown codes.explaintest coverage and strengthened CI parity checks across project input modes.