Conversation
…nd it's bestter practice to have null in database
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds support for a ChangesSchema Finding Zone Display Name Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 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
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@cmd/api/src/database/graphschema.go`:
- Around line 952-960: The CreateSchemaFinding function currently binds
zoneDisplayName as a plain string so empty string "" gets inserted instead of
NULL; modify the function to convert zoneDisplayName into a nullable value
(e.g., sql.NullString{String: zoneDisplayName, Valid: zoneDisplayName != ""})
and pass that nullable variable into the Raw query instead of zoneDisplayName so
empty titles are persisted as NULL (update/create sql import if missing); keep
the rest of the INSERT/Scan logic targeting model.SchemaFinding unchanged.
In `@cmd/api/src/database/upsert_schema_finding_integration_test.go`:
- Around line 88-89: The test compares args.zoneDisplayName /
args.newZoneDisplayName (string) directly to finding.ZoneDisplayName
(null.String), which can fail; update the assertions to first assert
finding.ZoneDisplayName.Valid (or assert the expected Valid state when
empty-string should map to null) and compare the string via
finding.ZoneDisplayName.ValueOrZero(); replace direct comparisons in the
upsert_schema_finding_integration_test assertions with
assert.True/False(finding.ZoneDisplayName.Valid) as appropriate and
assert.Equal(t, args.zoneDisplayName, finding.ZoneDisplayName.ValueOrZero())
(and likewise for args.newZoneDisplayName).
In `@packages/go/openapi/src/paths/attack-paths.attack-paths-findings.yaml`:
- Around line 158-160: The OpenAPI examples for the response schema in
attack-paths.attack-paths-findings.yaml are missing the newly added title
property; update every JSON example object tied to the findings/response schema
to include a "title" string that matches the description (e.g., a human-readable
finding title such as "Tier Zero" or "Privilege Zone" variant). Locate the
examples under the findings response examples in
attack-paths.attack-paths-findings.yaml and add the title field to each example
payload so the documented examples match the schema's title property.
- Around line 133-135: The CSV example's data rows do not match the header
columns
(severity,finding,title,finding_type,platform,environment_id,zone_id,zone_name,source_principal_id,source_principal_kind,source_principal_name,target_principal_id,target_principal_kind,target_principal_name,status,first_seen,last_seen):
fix by realigning each data row to have 17 comma-separated values matching those
header fields (insert empty placeholders for missing values like
zone_id/zone_name or source/target principal fields, or reorder values to their
correct header positions) so the example rows at the two lines conform exactly
to the header schema.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1c4c42f2-7e0b-4b61-87de-b2a4c2e30d35
📒 Files selected for processing (8)
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/migration/migrations/20260519000000_v9_add_schema_findings_title.sqlcmd/api/src/database/mocks/db.gocmd/api/src/database/upsert_schema_finding.gocmd/api/src/database/upsert_schema_finding_integration_test.gocmd/api/src/model/graphschema.gopackages/go/openapi/src/paths/attack-paths.attack-paths-findings.yaml
|
Actionable comments posted: 0 |
|
Actionable comments posted: 0 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Actionable comments posted: 0 |
Description
Problem Statement: The unified findings endpoint needed to return a human-readable title (e.g. "Tier Zero Write Owner") alongside the raw finding key (e.g. "T0WriteOwner") so the new Attack Paths table could display and sort/filter on it. Title resolution was split across two sources: a static title.md file first, then GetRemediationByFindingName().DisplayName as a fallback. Neither was tag-aware, meaning T0 and PZ customers could get the wrong label depending on context.
The ticket also surfaced two related questions: what is the intended difference between the short name (raw finding key) and the long name (display title), and why do
title.mdand the CUE-defined name differ post-OpenGraph.What I found: The short name is the stable programmatic identifier; the long name is the human-readable label shown in the UI. They diverged when OpenGraph introduced custom (non-built-in) findings whose display names live only in the database.
title.mdand the CUE name are not the same because CUE names are schema identifiers, whiletitle.mdwas added later as a UI-layer override. OpenGraph findings have no static file at all so their display name is entirely DB-driven.The best course of action for sort/filter was to canonicalize the title into the schema_findings table as display_name and zone_display_name, making it a first-class database column rather than a runtime-resolved string.
Fix
See Jira Comment: https://specterops.atlassian.net/browse/BED-8273?focusedCommentId=44430
Motivation and Context
Resolves BED-8273
Because the unified findings endpoint was returning raw finding keys (e.g. T0WriteOwner) instead of human-readable titles in some cases, and when titles were resolved they weren't tag-aware. T0 and PZ customers could get the wrong label for the zone they were viewing.
How Has This Been Tested?
Integration + Unit Tests
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores