fix(output): add missing surface field to test Finding constructors#59
Conversation
#54 and #55 were merged ~30s apart on main. #55 added a new `surface` field to `Finding` (with `#[serde(default)]` on the field but no default on the struct itself), and #54 was merged without rebasing onto post-#55 main. The new field is required at construction time, so the test helpers in `src/output/json.rs` and `src/output/text.rs` introduced by #54 no longer compile against the post-#55 struct shape. Both PRs passed CI on their own branches because each was tested against the main they branched from — neither saw the other's changes. No textual conflict, but a semantic one: `cargo build` still succeeds (lib target only references the field via serde), but `cargo clippy --all-targets` and `cargo test` fail with E0063 on the lib-test target. Fix is mechanical: add `surface: Surface::Backend` to the two test helpers, matching the convention already used elsewhere in the codebase (src/scanner/matcher.rs, src/rego/grouping.rs, etc). Followup: the repo ruleset on `main` doesn't include a `required_status_checks` rule, so neither "CI must pass" nor "branches must be up to date before merging" is currently enforced — that's why this slipped through. Tracked separately.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR updates test fixtures in JSON and text output modules to construct ChangesTest Fixture Updates for Surface Field
Estimated Code Review Effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly Related PRs
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 docstrings
Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix correctly resolves the compilation error by adding the missing surface field to both test helpers. The choice of Surface::Backend as the default value is appropriate and matches the pattern used elsewhere in the codebase. The changes are minimal and targeted, addressing the semantic merge conflict without introducing any defects.
The PR is ready to merge.
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}
1 participant
Summary
cargo clippy --all-targets:error[E0063]: missing field \surface` in initializer of `types::Finding`insrc/output/json.rs:118andsrc/output/text.rs:131`.surfacefield toFinding; feat(output): promote externalization percentage to headline #54 was merged without rebasing onto post-feat: scanner output follow-ups (surface, snippet fallback, enforcement_points) #55 main, so the test helpers it introduced no longer compile.surface: Surface::Backendto the two affected test helpers, matching the convention used elsewhere in the codebase.Note that
cargo build(lib target) still succeeds — the field is only required when the test target is built, which is why the failure shows up undercargo clippy --all-targets/cargo testbut not a plain build.Followup (separate)
The repo ruleset on
mainhas norequired_status_checksrule, so neither "CI must pass" nor "Require branches to be up to date before merging" is enforced today. That's why this slipped through. Will address in a separate change.Test plan
cargo fmtcargo clippy --all-targets -- -D warningscleancargo testpassesSummary by CodeRabbit