feat: bound public API surface + fix broken CLI surface (OSS P0)#47
feat: bound public API surface + fix broken CLI surface (OSS P0)#47
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes SARIF as a CLI output option, adds an ChangesPublic surface, CLI, command flow, and errors
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (args)
participant Main as main.rs
participant Lib as zift::run
participant Commands as commands::dispatch
participant Scanner as scanner::parser
CLI->>Main: Cli::parse()
Main->>Lib: zift::run(cli)
Lib->>Commands: dispatch "scan"
Commands->>Scanner: get_language / parse_source
alt supported
Scanner-->>Commands: Language (ok)
else unsupported
Scanner-->>Commands: UnsupportedLanguage error
end
Commands-->>Lib: command Result
Lib-->>Main: return Result
Main-->>CLI: print error & exit / success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (4)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully addresses the pre-release P0 items for making the repository public. The changes are well-implemented and achieve the stated objectives:
Key Changes Reviewed:
- API surface control: The
unstablefeature flag pattern correctly restricts internal modules while maintaining backward compatibility for stable APIs - Simplified entry point: The new
lib::run()function provides a clean public interface - SARIF removal: Properly removes the unimplemented SARIF format from CLI and command execution
- Error handling: The new
UnsupportedLanguageerror variant provides better type safety and consistent error messages
Summary:
All changes function correctly and no blocking issues were identified. The implementation follows Rust best practices and successfully prepares the codebase for public release.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/scanner/parser.rs (1)
19-20: ⚡ Quick winAdd a regression assertion for the typed unsupported-language error.
Now that this path returns
ZiftError::UnsupportedLanguage, the test should assert that exact variant (not justis_err()), so this contract doesn’t regress back to generic errors.Suggested test tightening
#[test] fn unsupported_language_returns_error() { - assert!(get_language(Language::CSharp, false).is_err()); + let err = get_language(Language::CSharp, false).unwrap_err(); + assert!(matches!(err, ZiftError::UnsupportedLanguage(Language::CSharp))); assert!(!is_language_supported(Language::CSharp)); }🤖 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/scanner/parser.rs` around lines 19 - 20, Update the test that currently only asserts is_err() to assert the exact error variant ZiftError::UnsupportedLanguage; call the parser function used in the test and replace the generic is_err() check with either assert!(matches!(result, Err(ZiftError::UnsupportedLanguage(_)))) or an explicit pattern match (e.g., if let Err(ZiftError::UnsupportedLanguage(lang)) = result { /* optional check on lang */ } else { panic!("expected UnsupportedLanguage") }), ensuring the test imports or references ZiftError::UnsupportedLanguage so the contract doesn't regress.
🤖 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/error.rs`:
- Around line 30-31: The error message on the UnsupportedLanguage variant
hardcodes a release version; update the message to be version-agnostic by
replacing the #[error("{0} is not yet supported in v0.1; support is planned for
v0.2")] attribute on UnsupportedLanguage(Language) with a neutral text (e.g.,
"{0} is not yet supported") or a message that points to docs/changelog for
roadmap, so the enum variant UnsupportedLanguage and its #[error(...)]
annotation no longer reference specific version numbers.
---
Nitpick comments:
In `@src/scanner/parser.rs`:
- Around line 19-20: Update the test that currently only asserts is_err() to
assert the exact error variant ZiftError::UnsupportedLanguage; call the parser
function used in the test and replace the generic is_err() check with either
assert!(matches!(result, Err(ZiftError::UnsupportedLanguage(_)))) or an explicit
pattern match (e.g., if let Err(ZiftError::UnsupportedLanguage(lang)) = result {
/* optional check on lang */ } else { panic!("expected UnsupportedLanguage") }),
ensuring the test imports or references ZiftError::UnsupportedLanguage so the
contract doesn't regress.
🪄 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
Run ID: cb61848d-8de0-48c9-afa7-8c8e57dc186a
📒 Files selected for processing (9)
AGENTS.mdCargo.tomldocs/DESIGN.mdsrc/cli.rssrc/commands/scan.rssrc/error.rssrc/lib.rssrc/main.rssrc/scanner/parser.rs
💤 Files with no reviewable changes (2)
- src/cli.rs
- src/commands/scan.rs
boorad
changed the title
|
@coderabbitai Fixed in 1ea81cc (nitpick from review body):
|
|
Tip For best results, initiate chat on the files or code changes.
|
jhawlwut
force-pushed
the
fix/oss-p0-surface
branch
from
8fba710 to
0a21025
Compare
Every internal module was pub, crystallising the entire codebase as a semver commitment the moment the repo goes public. Stable public surface (always pub): cli, error, types, rules, rego Internal only (private mod — binary reaches them through lib::run): commands, logging Internal, opt-in for integrators (pub only with --features unstable): config, deep, mcp, output, scanner Adds a top-level `pub fn run(cli: Cli) -> Result<()>` so the binary no longer needs to reach into private modules directly; main.rs is now a four-line shim. Pre-1.0 SemVer note: removing pub visibility from previously-public modules is a breaking library API change → feat! to cut 0.2.0.
Two advertised-but-broken CLI surfaces that generate day-one bug
reports:
1. `--format sarif` parsed cleanly then returned a runtime error.
Remove the OutputFormat::Sarif variant entirely; the match is now
exhaustive over Text and Json. SARIF can land as a proper feat: in
v0.2. AGENTS.md and docs/DESIGN.md updated to match.
2. get_language() returned ZiftError::General(String) for unsupported
languages (CSharp, Kotlin, Ruby, Php). Replace with the new typed
variant ZiftError::UnsupportedLanguage(Language) so callers can
pattern-match on the specific case and the error message is
consistent ("… is not yet supported in v0.1; support is planned for
v0.2").
…bility
Privatising `mcp` and `deep` (per PUBLIC-1) made clippy stop assuming
external crates use these items, surfacing four pre-existing dead_code
warnings:
- Candidate.kind / seed_category — read in tests + Debug, not in
--lib clippy targets
- error_code::INVALID_REQUEST — kept for JSON-RPC protocol completeness
- FrameError::Parse { raw } — retained for Debug-trace diagnostics
- HandlerError::Internal — matched in dispatch, reserved for future
handler paths
#[allow(dead_code)] is the minimum-scope fix; deletion would broaden
the PR.
…tures The four `tests/` integration tests reach into modules now gated behind the `unstable` feature (deep, mcp, scanner, config). Adding `required-features = ["unstable"]` so default `cargo test` skips them cleanly, and updating CI to run `cargo test --all-features` and `cargo clippy --all-features` so the full integration surface is still exercised in CI.
2 participants
What
Addresses the two code-side P0 items from the pre-release panel review that block flipping the repo public.
PUBLIC-1 — Bound the public crate API surface (
Cargo.toml,src/lib.rs,src/main.rs)Every module was
pub, crystallising the entire codebase as a semver commitment the moment the repo goes public. This commit draws a clear line:cli,error,types,rules,regopubcommands,loggingmod(private)lib::runconfig,deep,mcp,output,scannerpubonly with--features unstableA new top-level
pub fn run(cli: Cli) -> Result<()>replaces the three-import dispatch pattern inmain.rs, which is now a four-line shim.PUBLIC-2 — Fix advertised-but-broken CLI surface (
src/cli.rs,src/commands/scan.rs,src/error.rs,src/scanner/parser.rs)Two surfaces that parse cleanly then fail at runtime:
SARIF:
--format sarifwas accepted by clap, then returnedZiftError::General("SARIF output not yet implemented"). RemovedOutputFormat::Sarifentirely; thematchis now exhaustive overTextandJson. SARIF can land asfeat: add SARIF outputin v0.2.AGENTS.mdanddocs/DESIGN.mdupdated.UnsupportedLanguage:
get_language()returnedZiftError::General(String)for CSharp/Kotlin/Ruby/Php. Replaced with a new typed variantZiftError::UnsupportedLanguage(Language)so callers can pattern-match and the message is consistent across all surfaces.What's NOT in this PR (handled separately)
agent_cmdstderr warning +.gitignoreentry — planned for v0.1.x.Test plan
cargo test,cargo clippy -- -D warnings)zift --helpno longer showssarifas a valid format optionzift --format sarif .no longer compiles (variant removed)zift scan --language csharp .now reads "csharp is not yet supported in v0.1; support is planned for v0.2"cargo build --features unstableexposesconfig,deep,mcp,output,scannermodules to external consumersSummary by CodeRabbit
New Features
unstablefeature flag to opt into additional (non-stable) APIsRefactor
unstableChores