feat(learn): common build/test/dev/run/debug commands - #51
Conversation
qr learn now fills the developer loop into .qr/profile.json using language defaults overridden by package.json, Cargo.toml, pyproject, go.mod, Makefile, and Justfile — so qr do gets richer project context.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
Changesqr learn Profile Command Detection
Test environment isolation
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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
🤖 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 `@src/project_profile.rs`:
- Around line 306-327: The framework detection logic in the project profile
classifier is incorrectly treating plain main.py projects as FastAPI. Update the
framework selection branch in the function that builds the `framework` value so
that `main.py` without matching FastAPI/Flask dependency hints returns `None`
instead of `Some("fastapi")`; keep the existing `manage.py` and explicit
dependency-based detections unchanged.
- Around line 744-766: The Makefile parsing loop in the project profile logic is
misclassifying recipe lines and variable assignments as targets. In the
script-detection pass, update the trimming/guard logic in the raw line scan so
recipe body lines are excluded before any colon-based parsing, and tighten the
name filter in the same loop to reject Makefile assignment forms like := and ?=
in addition to plain =. Keep the fix localized around the line-processing code
that uses raw.lines(), split_once(':'), and insert_default(scripts, ...), so
only real target headers are learned as scripts.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f5c9f19-686b-47aa-bc47-5b2dc445d22c
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mdsrc/commands/do_cmd.rssrc/commands/learn.rssrc/project_profile.rstests/cli.rs
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if line.is_empty() || line.starts_with('#') || line.starts_with('\t') { | ||
| continue; | ||
| } | ||
| let Some(name) = line.split_once(':').map(|(n, _)| n.trim()) else { |
There was a problem hiding this comment.
WARNING: Makefile variable assignments using := are misidentified as targets.
A line like VERSION := 1.2.3 is split on the first :, yielding name = "VERSION". The filter below checks name.contains('='), but the = lives in the remainder after the colon, so it passes and a bogus make VERSION entry is added to scripts. Consider skipping the line when the remainder after : starts with = (a := assignment rather than a target rule).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| continue; | ||
| } | ||
| // Only treat as recipe if the line has a colon (recipe header). | ||
| if !line.contains(':') { |
There was a problem hiding this comment.
WARNING: Justfile assignments and set/export settings are misidentified as recipes.
Lines like foo := "bar", set shell := ["bash","-cu"], or export RUST_BACKTRACE := "1" contain a colon (from :=), so the line.contains(':') gate admits them and a bogus just foo / just set / just export entry is added to scripts. The colon check should distinguish a recipe header (name:) from a := assignment, e.g. by ensuring the matching : is not part of :=.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
The latest commit ( Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous Review Summaries (4 snapshots, latest commit 7c6e83a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7c6e83a)Status: 1 Issue Found | Recommendation: Address before merge Overview
The new commit ( Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit 51fbb16)Status: No Issues Found | Recommendation: Merge The previously reported warning on Files Reviewed (1 file)
Previous review (commit 9f8a6c3)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit d4a503e)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Reviewed by glm-5.2-short · Input: 28.8K · Output: 15.5K · Cached: 868K |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: d4a503ea68
ℹ️ 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".
| if is_node { | ||
| qualify_script_command(profile.package_manager.as_deref(), key, Some(body.as_str())) |
There was a problem hiding this comment.
When a Node project has no package.json script for a role but does have a Makefile/Justfile target, the merge step adds entries like build -> make build or build -> just build to scripts. This branch then treats every Node scripts entry as a package script and promotes it to npm run build/pnpm build, which fails because the package script still does not exist; preserve the merged command body for non-package entries instead of qualifying it.
Useful? React with 👍 / 👎.
| insert_default(scripts, "dev", "next dev"); | ||
| insert_default(scripts, "build", "next build"); | ||
| insert_default(scripts, "start", "next start"); | ||
| insert_default(scripts, "lint", "next lint"); |
There was a problem hiding this comment.
For Next.js 16 projects that omit a lint script, this fallback writes lint_command/scripts.lint as next lint. Next.js v16 removed that command (see https://nextjs.org/docs/app/guides/upgrading/version-16#next-lint-command), so in that scenario qr learn suggests a command that fails; only derive lint from an existing script/config or use ESLint/Biome directly.
Useful? React with 👍 / 👎.
| // Only treat as recipe if the line has a colon (recipe header). | ||
| if !line.contains(':') { | ||
| continue; | ||
| } | ||
| insert_default(scripts, name, &format!("just {name}")); |
There was a problem hiding this comment.
In Justfiles with top-level variables or aliases such as target := "main" or alias b := build (documented Just syntax), this check only requires any colon in the line and then inserts just target or just alias as if it were a recipe. If one of these names is test/build in a project without another role command, qr learn promotes a non-existent recipe to the top-level command; reject assignments/aliases or use just --summary-equivalent parsing.
Useful? React with 👍 / 👎.
| dev_command: Some("cargo run".into()), | ||
| run_command: Some("cargo run".into()), | ||
| debug_command: Some("RUST_BACKTRACE=1 cargo run".into()), |
There was a problem hiding this comment.
For Cargo libraries or virtual workspaces that have Cargo.toml but no runnable binary target, these new role commands still get set to cargo run, so qr learn records dev/run/debug commands that fail with Cargo's no-bin-target error. Only populate these run-oriented fields when the manifest or entry points show a binary target, otherwise leave them unset or use a non-running Cargo command.
Useful? React with 👍 / 👎.
| dev_command: Some("go run .".into()), | ||
| run_command: Some("go run .".into()), | ||
| debug_command: Some("go run -race .".into()), |
There was a problem hiding this comment.
For Go modules whose executable lives under cmd/ or whose root package is not main, the new dev/run/debug fields are still go run ./go run -race ., which fails from the module root even though go build ./... and go test ./... work. Since this code already detects cmd/ as an entry point, avoid emitting root go run . unless the root directory is actually runnable.
Useful? React with 👍 / 👎.
| let dev = format!("uvicorn {app} --reload"); | ||
| let run = format!("uvicorn {app}"); | ||
| let debug = format!("uvicorn {app} --reload --log-level debug"); |
There was a problem hiding this comment.
For any Python project with a root main.py but no FastAPI/Flask dependency, the existing heuristic still marks the framework as fastapi, and these new commands then record uvicorn main:app. A typical script-style main.py has no ASGI app, so the learned dev/run/debug commands fail; reserve the uvicorn defaults for projects that actually declare or expose an ASGI app, and let plain main.py use the fallback Python command.
Useful? React with 👍 / 👎.
| let prefix = match package_manager.as_deref() { | ||
| Some("uv") => Some("uv run"), | ||
| Some("poetry") => Some("poetry run"), | ||
| Some("pdm") => Some("pdm run"), | ||
| _ => None, |
There was a problem hiding this comment.
When a project has a Pipfile, detection now records package_manager = "pipenv", but this prefix table leaves the learned commands as bare pytest, ruff, or uvicorn. Pipenv's command reference describes run as running a command within the virtual environment (https://pipenv.pypa.io/en/latest/commands.html), so these commands can fail whenever the tools are installed only in the Pipenv environment; add pipenv run to the same prefix handling.
Useful? React with 👍 / 👎.
| if line.is_empty() || line.starts_with('#') || line.starts_with('\t') { | ||
| continue; | ||
| } | ||
| let Some(name) = line.split_once(':').map(|(n, _)| n.trim()) else { |
There was a problem hiding this comment.
Makefiles commonly use := for variable assignments (GNU make documents this assignment form at https://ftp.gnu.org/old-gnu/Manuals/make-3.79.1/html_chapter/make_6.html), but this parser splits at the colon and validates only the left side. A line like test := pytest is therefore recorded as make test; in a project without another test command, fill_role_commands_from_scripts promotes a non-existent target and the learned command fails.
Useful? React with 👍 / 👎.
Skip Makefile/Justfile assignments, avoid inventing next lint or FastAPI for plain main.py, preserve make/just bodies on Node, gate cargo/go run on real binaries, and prefix Pipenv commands. Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
…8f67 fix(learn): address PR #51 review findings
| let candidates = [ | ||
| root.join("src/bin").join(format!("{name}.rs")), | ||
| root.join("src/bin").join(name).join("main.rs"), | ||
| root.join("src/main.rs"), |
There was a problem hiding this comment.
WARNING: Explicit [[bin]] without a path resolves src/main.rs as a fallback candidate with no name == package_name guard
For an explicit [[bin]] name = "worker" (no path) where src/bin/worker.rs and src/bin/worker/main.rs are absent but src/main.rs exists, this third candidate resolves the bin's path to src/main.rs. That path is then pushed into explicit_paths, which makes the autobins block skip re-adding src/main.rs as the package-named bin (line 409), leaving bins == ["worker"] and emitting cargo run.
But Cargo does not use src/main.rs as the path for a non-package-name explicit bin: with no path it only looks at src/bin/{name}.rs then src/bin/{name}/main.rs, so cargo run would fail (can't find \worker` bin/ ambiguous). This contradicts the function's documented contract of emitting a run command "only when Cargo can choose one runnable binary without extra features or--bin." Guard the src/main.rsfallback ondeclared_name == package_name`.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/project_profile.rs (2)
1708-1727: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve quiet recipe headers.
@test:is still callable asjust test; dropping@here causes quiet recipes to be skipped when merging scripts.🤖 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 `@src/project_profile.rs` around lines 1708 - 1727, The recipe header parsing in project_profile should preserve quiet recipes like `@test`: instead of filtering them out during script merging. Update the logic around the name validation and insert_default path so the leading @ is stripped or otherwise handled before validation, while still allowing the recipe to be inserted under the callable name test. Use the existing first, name, and insert_default flow to make sure quiet recipe headers remain merged as scripts.
653-656: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate
go run -raceon race-detector support. Skip thedebugscript when the activeGoBuildContextisn’t cgo-enabled or the GOOS/GOARCH pair doesn’t support-race; otherwise some targets end up with an unusable command.🤖 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 `@src/project_profile.rs` around lines 653 - 656, The `debug` script is always being added with `go run -race`, but it should only be generated when the active `GoBuildContext` can actually support the race detector. Update the script insertion logic in the `root_is_main` block to check the current `GoBuildContext` for cgo support and a GOOS/GOARCH combination that allows `-race`, and skip inserting `debug` otherwise. Use the existing project/profile generation flow around `scripts.insert` so unsupported targets don’t receive an unusable command.
🤖 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.
Outside diff comments:
In `@src/project_profile.rs`:
- Around line 1708-1727: The recipe header parsing in project_profile should
preserve quiet recipes like `@test`: instead of filtering them out during script
merging. Update the logic around the name validation and insert_default path so
the leading @ is stripped or otherwise handled before validation, while still
allowing the recipe to be inserted under the callable name test. Use the
existing first, name, and insert_default flow to make sure quiet recipe headers
remain merged as scripts.
- Around line 653-656: The `debug` script is always being added with `go run
-race`, but it should only be generated when the active `GoBuildContext` can
actually support the race detector. Update the script insertion logic in the
`root_is_main` block to check the current `GoBuildContext` for cgo support and a
GOOS/GOARCH combination that allows `-race`, and skip inserting `debug`
otherwise. Use the existing project/profile generation flow around
`scripts.insert` so unsupported targets don’t receive an unusable command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 865c0730-4162-43a5-8881-a8553b59eaa0
📒 Files selected for processing (3)
src/ai/client.rssrc/config.rssrc/project_profile.rs
| let target_supported = match build_context.goos.as_str() { | ||
| "linux" => matches!( | ||
| build_context.goarch.as_str(), | ||
| "amd64" | "arm64" | "loong64" | "ppc64le" | "riscv64" | "s390x" |
There was a problem hiding this comment.
WARNING: loong64 is not a race-detector-supported target; recording go run -race . will fail.
Go's race detector (runtime/race) is only available on linux/amd64, linux/arm64, linux/ppc64le, linux/riscv64, linux/s390x, darwin/amd64, darwin/arm64, freebsd/amd64, netbsd/amd64, and windows/amd64. linux/loong64 is not in that documented set, yet with goos == "linux" and cgo_enabled == Some(true) this match arm returns true, so qr learn populates debug_command/scripts.debug with go run -race ., which fails with the race-detector-not-supported error this commit is meant to prevent.
| "amd64" | "arm64" | "loong64" | "ppc64le" | "riscv64" | "s390x" | |
| "amd64" | "arm64" | "ppc64le" | "riscv64" | "s390x" |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| let target_supported = match build_context.goos.as_str() { | ||
| "linux" => matches!( | ||
| build_context.goarch.as_str(), | ||
| "amd64" | "arm64" | "loong64" | "ppc64le" | "s390x" |
There was a problem hiding this comment.
WARNING: loong64 is not a race-detector-supported target; go run -race . still fails
Go's race detector officially supports only linux/amd64, linux/arm64, linux/ppc64le, and linux/s390x (plus amd64 on freebsd/netbsd/windows, and amd64/arm64 on darwin). linux/loong64 is not in that set, so go run -race . errors with -race is not supported on linux/loong64. This commit correctly removed riscv64 but kept loong64, so a debug command is still recorded for an unsupported target. Drop "loong64" from this linux match arm, and update the go_race_detector_support_matches_current_go_exceptions test that currently asserts loong64 is supported.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
2 participants
Summary
qr learnnow populates common developer-loop commands into.qr/profile.json:dev_command,run_command,debug_command, plus richerscriptsdefaults.package.jsonscripts, lockfile package managers, framework detection) and supplemented by Makefile / Justfile targets without clobbering existing keys..qr.tomlcan override the new fields; old profiles without them still load.Test plan
cargo testcargo clippy --all-targets --locked -- -D warningscargo fmt --all -- --checkcda Rust project →qr learn→ confirmdev/run/debugin profile and summarynext dev), notpnpm <missing>Summary by CodeRabbit
qr learnnow writes richer dev/run/debug command details to./.qr/profile.json, with language- and framework-aware defaults (and honors.qr.tomloverrides).qr learnsummary output now includes Dev/Run/Debug when available.main.pyno longer misclassified as FastAPI).cargo run/go run .are recorded when no runnable root exists.qr learnbehavior and output.