Improve agent query workflows - #630
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
documentation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jonathanong
marked this pull request as ready for review
August 9, 2026 23:29
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jonathanong
force-pushed
the
agent/planner-query-ux
branch
from
August 9, 2026 23:53
eced13d to
e64e59d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
crates/no-mistakes/src/napi_api/tests/ci.rs (1)
90-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the N-API stderr contract.
This test only parses the returned JSON. It does not capture or assert stderr. An accidental
eprintln!inimpacted_checks_json_implwould still pass. Capture stderr around both empty-result calls and assert that it is empty.🤖 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 `@crates/no-mistakes/src/napi_api/tests/ci.rs` around lines 90 - 104, Update impacted_checks_json_classifies_empty_results_without_stderr_side_effects to capture stderr separately around both impacted_checks_json_impl calls, then assert each capture is empty while preserving the existing JSON code assertions.crates/no-mistakes/src/codebase/ts_source/facts/tests.rs (1)
202-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the path-qualified read error.
At Lines 202-209 and 405-408, the tests check only
failed to read. The production change addspath.display()to the message. A regression that removes the path would pass both tests.Assert that the operational error also contains
missing.display().to_string()in both fixture-backed tests.Suggested assertion shape
+ let expected_path = missing.display().to_string(); assert!(facts[&missing] .operational_error .as_deref() - .is_some_and(|error| error.contains("failed to read"))); + .is_some_and(|error| { + error.contains("failed to read") && error.contains(&expected_path) + }));Apply the same assertion to the second read-error check.
As per coding guidelines, when an error is found, create a regression test, preferably backed by a repository fixture.
Also applies to: 405-408
🤖 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 `@crates/no-mistakes/src/codebase/ts_source/facts/tests.rs` around lines 202 - 209, Strengthen the read-error assertions in both fixture-backed tests around the existing operational_error checks by also requiring the error to contain missing.display().to_string(). Preserve the existing “failed to read” assertion and apply the path-qualified check consistently at both locations.Source: Coding guidelines
crates/no-mistakes/src/codebase/queries/resolve_check/batch/tests.rs (1)
29-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing diagnostic case for the fatal-parse fallback.
This case sets
parse_error: Some("parser panicked"), sotarget_importsalways takes theSomearm atbatch.rsline 90. Theunwrap_or("parser panicked without a diagnostic")fallback stays untested. Add a case withfatal_parse_error: trueandparse_error: Noneto cover it.♻️ Proposed additional case
assert!(target_imports(&target, &fatal_parse) .unwrap_err() .to_string() .contains("failed to parse")); + + // A panicking parser records no diagnostic, so the message must still name the file. + let panicked = TsFactMap::from([( + target.abs_file.clone(), + TsFileFacts { + parse_error: None, + fatal_parse_error: true, + ..TsFileFacts::default() + }, + )]); + assert!(target_imports(&target, &panicked) + .unwrap_err() + .to_string() + .contains("parser panicked without a diagnostic"));As per coding guidelines: "Maintain 99% line and function coverage; refactor code that cannot reach the threshold instead of adding coverage exceptions."
🤖 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 `@crates/no-mistakes/src/codebase/queries/resolve_check/batch/tests.rs` around lines 29 - 40, Add a test case alongside the existing fatal-parse test in the batch query tests with fatal_parse_error set to true and parse_error set to None, then invoke target_imports and assert the resulting error contains “parser panicked without a diagnostic” to exercise the fallback in target_imports.Source: Coding guidelines
crates/no-mistakes/tests/cli_queries.rs (1)
177-202: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the single-file baseline shape alongside the batch shape.
This test proves the batch envelope for multiple files. No CLI test proves that one positional file still returns the historical single-file schema and not the batch envelope.
resolve_check_exit_codesat lines 155-175 readsallResolve, which both schemas expose, so it does not detect a regression to the batch shape.Add the complementary assertions so the pair covers both sides of the additive change.
♻️ Proposed addition
assert_eq!( value["results"] .as_array() .unwrap() .iter() .map(|result| result["file"].as_str().unwrap()) .collect::<Vec<_>>(), vec!["broken.ts", "consumer.ts"] ); + + // One positional file must keep the historical single-file schema. + let single = run(&[ + "resolve-check", + "broken.ts", + "--root", + root.to_str().unwrap(), + "--json", + ]); + let single = json(&single); + assert!(single["imports"].is_array()); + assert_eq!(single["file"], "broken.ts"); + assert!(single.get("results").is_none()); + assert!(single.get("unresolvedFiles").is_none()); }As per coding guidelines: "Additive CLI flags must not alter pre-existing report fields. Broader-scope additive analyses require fixture-backed parity tests for baseline fields with the flag disabled and enabled."
🤖 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 `@crates/no-mistakes/tests/cli_queries.rs` around lines 177 - 202, Extend the single-file CLI test, resolve_check_exit_codes, to assert the historical single-file JSON schema, including its baseline report fields and excluding batch-only fields such as results or unresolvedFiles. Keep the existing exit-code and allResolve assertions, and retain resolve_check_batch_is_deterministic_and_reports_partial_failure for validating the batch envelope.Source: Coding guidelines
packages/no-mistakes/query-types.d.ts (1)
105-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the batch options from the shared query options instead of re-declaring
rootandtsconfig.
ResolveCheckFilesOptionscopiesrootandtsconfigfromQueryFileOptions. The two declarations can drift. IfQueryFileOptionsgains a field, the batch form will not receive it, and the N-API layer will accept an option that the type rejects.♻️ Proposed refactor
/** Batch form of `resolveCheck`; `files` always returns the batch schema. */ -export interface ResolveCheckFilesOptions { - /** A nonempty list of TS/JS files to check. */ - files: [string, ...string[]]; - /** Mutually exclusive with `files`. */ - file?: never; - /** Project root. Defaults to the current working directory. */ - root?: string; - /** Path to tsconfig.json for alias resolution. Searched upward if omitted. */ - tsconfig?: string; -} +export type ResolveCheckFilesOptions = Omit<QueryFileOptions, "file"> & { + /** A nonempty list of TS/JS files to check. */ + files: [string, ...string[]]; + /** Mutually exclusive with `files`. */ + file?: never; +};Confirm that
QueryFileOptionsdeclares onlyfile,root, andtsconfigbefore applying this change.🤖 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 `@packages/no-mistakes/query-types.d.ts` around lines 105 - 114, Update ResolveCheckFilesOptions to derive its shared options from QueryFileOptions rather than redeclaring root and tsconfig, while preserving the batch-only files constraint and excluding the singular file field. First confirm QueryFileOptions contains only file, root, and tsconfig, then reuse its root and tsconfig declarations so future shared fields remain synchronized.
🤖 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 `@crates/no-mistakes/src/codebase/ts_source/facts/collect/file.rs`:
- Around line 13-20: Document parse_error as the legacy shared-error field and
preserve its existing consumer contract: retain populating both parse_error and
operational_error in the error branches of the file-facts collection flow, and
add documentation explaining this intentional duplication. Ensure downstream
parse_error counting/filtering and the related assertion remain consistent.
In `@crates/no-mistakes/src/napi_api/cli_parity_builders.rs`:
- Around line 7-15: Update the validation errors in the direct ownership checks
to explain the invalid option combination, identify the required correction, and
state the reason for each restriction: instruct callers to set framework because
direct ownership requires framework-specific ownership, and instruct them to
remove limitPercent, limitFiles, and globalConfigFallback because direct
ownership bypasses configured plan policy.
In `@crates/no-mistakes/src/napi_api/queries.rs`:
- Around line 145-157: Handle an explicitly provided empty files list separately
in the file-or-files validation near the existing match in
crates/no-mistakes/src/napi_api/queries.rs#L145-L157, returning an
agent-friendly error that names the empty list and states resolve-check requires
at least one path; retain the existing empty-path and missing-input behavior.
Add coverage in crates/no-mistakes/src/napi_api/tests_queries.rs#L56-L75 beside
resolve_check_json_files_rejects_empty_paths by calling resolve_check_json_impl
with {"files": []} and asserting the new error text.
In `@crates/no-mistakes/src/tests/plan/global_config.rs`:
- Around line 41-73: Update is_global_config_path to remove the hardcoded
Next.js, proxy, and middleware filename checks and instead obtain
framework-specific fallback paths from the configured analysis scope. Preserve
the existing explicit global-config matches and fallback selection semantics,
using the relevant configuration symbols already available to determine whether
a path is global.
In `@crates/no-mistakes/tests/cli_ci.rs`:
- Around line 116-133: Strengthen
impacted_checks_diagnose_empty_does_not_mask_failures by asserting that stderr
contains the expected invalid-configuration diagnostic or
invalid.no-mistakes.yml path, while preserving the existing failure-status,
empty-stdout, and no-note assertions.
In `@packages/no-mistakes/scripts/api.test.js`:
- Around line 473-480: Add async rejection tests alongside the existing
resolveCheck API tests, calling api.resolveCheck() with { files: [] } and with
both file and files populated. Assert that each call rejects, covering native
validation errors translated into Promise rejections while preserving the
existing valid-input coverage.
---
Nitpick comments:
In `@crates/no-mistakes/src/codebase/queries/resolve_check/batch/tests.rs`:
- Around line 29-40: Add a test case alongside the existing fatal-parse test in
the batch query tests with fatal_parse_error set to true and parse_error set to
None, then invoke target_imports and assert the resulting error contains “parser
panicked without a diagnostic” to exercise the fallback in target_imports.
In `@crates/no-mistakes/src/codebase/ts_source/facts/tests.rs`:
- Around line 202-209: Strengthen the read-error assertions in both
fixture-backed tests around the existing operational_error checks by also
requiring the error to contain missing.display().to_string(). Preserve the
existing “failed to read” assertion and apply the path-qualified check
consistently at both locations.
In `@crates/no-mistakes/src/napi_api/tests/ci.rs`:
- Around line 90-104: Update
impacted_checks_json_classifies_empty_results_without_stderr_side_effects to
capture stderr separately around both impacted_checks_json_impl calls, then
assert each capture is empty while preserving the existing JSON code assertions.
In `@crates/no-mistakes/tests/cli_queries.rs`:
- Around line 177-202: Extend the single-file CLI test,
resolve_check_exit_codes, to assert the historical single-file JSON schema,
including its baseline report fields and excluding batch-only fields such as
results or unresolvedFiles. Keep the existing exit-code and allResolve
assertions, and retain
resolve_check_batch_is_deterministic_and_reports_partial_failure for validating
the batch envelope.
In `@packages/no-mistakes/query-types.d.ts`:
- Around line 105-114: Update ResolveCheckFilesOptions to derive its shared
options from QueryFileOptions rather than redeclaring root and tsconfig, while
preserving the batch-only files constraint and excluding the singular file
field. First confirm QueryFileOptions contains only file, root, and tsconfig,
then reuse its root and tsconfig declarations so future shared fields remain
synchronized.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d188dbc-850c-4e4e-a2b0-a5045b77fa32
📒 Files selected for processing (59)
crates/no-mistakes/src/codebase/check_facts/file/program.rscrates/no-mistakes/src/codebase/queries/resolve_check.rscrates/no-mistakes/src/codebase/queries/resolve_check/batch.rscrates/no-mistakes/src/codebase/queries/resolve_check/batch/tests.rscrates/no-mistakes/src/codebase/queries/resolve_check/tests.rscrates/no-mistakes/src/codebase/queries/reverse.rscrates/no-mistakes/src/codebase/queries/reverse/build.rscrates/no-mistakes/src/codebase/queries/shared.rscrates/no-mistakes/src/codebase/queries/shared/targets.rscrates/no-mistakes/src/codebase/queries/shared/tests.rscrates/no-mistakes/src/codebase/ts_source/facts.rscrates/no-mistakes/src/codebase/ts_source/facts/collect/file.rscrates/no-mistakes/src/codebase/ts_source/facts/tests.rscrates/no-mistakes/src/impacted_checks.rscrates/no-mistakes/src/impacted_checks/generate.rscrates/no-mistakes/src/impacted_checks/generate/args.rscrates/no-mistakes/src/impacted_checks/tests.rscrates/no-mistakes/src/impacted_checks/tests/coverage.rscrates/no-mistakes/src/impacted_checks/tests/runner_isolation.rscrates/no-mistakes/src/napi_api/cli_parity_builders.rscrates/no-mistakes/src/napi_api/options_flow_tests.rscrates/no-mistakes/src/napi_api/queries.rscrates/no-mistakes/src/napi_api/tests/ci.rscrates/no-mistakes/src/napi_api/tests_planning.rscrates/no-mistakes/src/napi_api/tests_planning/direct_test_owner.rscrates/no-mistakes/src/napi_api/tests_queries.rscrates/no-mistakes/src/tests/args.rscrates/no-mistakes/src/tests/configured_plan.rscrates/no-mistakes/src/tests/configured_plan/direct_test_owner.rscrates/no-mistakes/src/tests/configured_plan/tests.rscrates/no-mistakes/src/tests/plan.rscrates/no-mistakes/src/tests/plan/global_config.rscrates/no-mistakes/src/tests/plan/run.rscrates/no-mistakes/src/tests/plan_output.rscrates/no-mistakes/src/tests/plan_output/explain.rscrates/no-mistakes/src/tests/plan_resources_tests.rscrates/no-mistakes/src/tests/prepared_plan.rscrates/no-mistakes/src/tests/prepared_plan/tests.rscrates/no-mistakes/src/tests/why.rscrates/no-mistakes/tests/cli_ci.rscrates/no-mistakes/tests/cli_queries.rscrates/no-mistakes/tests/cli_test_plan_resources.rscrates/no-mistakes/tests/cli_tests_impact.rscrates/no-mistakes/tests/cli_tests_impact/direct_owner_coverage.rsdocs/cli/README.mddocs/cli/impacted-checks.mddocs/cli/resolve-check.mddocs/cli/tests-plan.mddocs/node-api.mdpackages/no-mistakes/ci-types.d.tspackages/no-mistakes/index.d.tspackages/no-mistakes/query-types.d.tspackages/no-mistakes/scripts/api.test.jspackages/no-mistakes/test-types.d.tsskills/no-mistakes/SKILL.mdskills/no-mistakes/references/lightweight-queries.mdskills/no-mistakes/references/tests.mdtest-cases/codebase-analysis/queries/fixture/unsupported.jsontest-cases/impacted-checks/basic/src/style.css
jonathanong
dismissed
coderabbitai[bot]’s stale review
August 10, 2026 00:50
Addressed the actionable contract, validation, diagnostics, and coverage feedback in 3bef575; the pre-existing global-config convention request is documented as an out-of-scope follow-up in the Shepherd Journal.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
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 `@crates/no-mistakes/src/tests/args.rs`:
- Around line 119-126: Remove the parser-level requires and conflicts_with_all
constraints for direct-test-owner in
crates/no-mistakes/src/tests/args.rs:119-126, leaving validation to resolve_args
and using its positional invalid example such as tests plan vitest
--direct-test-owner. Update
crates/no-mistakes/tests/cli_tests_impact/direct_owner_coverage.rs:43-46 to
assert the detailed missing-framework error, and lines 58-59 to assert the
detailed policy-override conflict error.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51132523-d003-48df-b5eb-bac9ea0c877f
📒 Files selected for processing (21)
crates/no-mistakes/src/codebase/queries/resolve_check/batch.rscrates/no-mistakes/src/codebase/queries/resolve_check/batch/tests.rscrates/no-mistakes/src/codebase/ts_source/facts/collect/file.rscrates/no-mistakes/src/napi_api/cli_parity_builders.rscrates/no-mistakes/src/napi_api/queries.rscrates/no-mistakes/src/napi_api/tests_planning/direct_test_owner.rscrates/no-mistakes/src/napi_api/tests_queries.rscrates/no-mistakes/src/tests/args.rscrates/no-mistakes/src/tests/args/tests.rscrates/no-mistakes/src/tests/impact.rscrates/no-mistakes/src/tests/prepared_plan.rscrates/no-mistakes/src/tests/prepared_plan/tests.rscrates/no-mistakes/tests/cli_ci.rscrates/no-mistakes/tests/cli_tests_impact.rscrates/no-mistakes/tests/cli_tests_impact/direct_owner_coverage.rsdocs/cli/tests-impact.mddocs/cli/tests-plan.mddocs/node-api.mdpackages/no-mistakes/scripts/api.test.jspackages/no-mistakes/test-types.d.tsskills/no-mistakes/references/tests.md
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/no-mistakes/src/napi_api/tests_planning/direct_test_owner.rs
- crates/no-mistakes/tests/cli_ci.rs
- crates/no-mistakes/src/tests/prepared_plan.rs
- crates/no-mistakes/src/codebase/queries/resolve_check/batch/tests.rs
- crates/no-mistakes/src/napi_api/tests_queries.rs
- skills/no-mistakes/references/tests.md
- docs/cli/tests-plan.md
- packages/no-mistakes/test-types.d.ts
- crates/no-mistakes/src/napi_api/queries.rs
- crates/no-mistakes/src/codebase/ts_source/facts/collect/file.rs
- crates/no-mistakes/src/codebase/queries/resolve_check/batch.rs
- crates/no-mistakes/src/napi_api/cli_parity_builders.rs
- docs/node-api.md
jonathanong
dismissed
coderabbitai[bot]’s stale review
August 10, 2026 02:02
Removed redundant Clap constraints so pre-analysis validation returns actionable direct-owner remediation, corrected the positional framework example, and strengthened CLI regression assertions.
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
1 participant
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.
Summary
tests plan --format explainoutput and an explicit--direct-test-ownermode with async N-API/type parityimpacted-checksresults with stable structured reasons plus opt-in CLI diagnosticsresolve-checkrequests while preserving the single-file CLI and Node schemasDesign and performance
SourceStore, and union import-fact pass for batch resolutionValidation
cargo test --workspace --all-features --lib --bins --tests— 6,137 passedcargo test -p no-mistakes --libafter rebase — 5,228 passedcargo llvm-cov ... --fail-under-lines 99 --fail-under-functions 99— passednpm test— 465 passedno-mistakes check, 15 ast-grep architecture guards,cargo fmt,cargo clippy, oxfmt, and oxlint — passedFixes jonathanong/filaments#9209
Shepherd Journal