Better data generation for fixture data#105
Open
jirwin wants to merge 3 commits into
Open
Conversation
WalkthroughAdds App as a first-class resource across client, connector, and demo data generation; extends the demo data generator with an organization hierarchy and multi-phase population flow; increases default demo user count from 10 to 200; and wires listing, paging, grant/revoke, event feeds, and group→app mappings for apps. Changes
Sequence Diagram(s)sequenceDiagram
participant Generator as Demo Generator
participant DB as Demo DB (client)
participant Connector as Connector ResourceSyncer
participant EventFeed as Connector EventFeed
participant API as Client API
Generator->>DB: build apps, groups, projects, roles, users, passwords
DB-->>Generator: persisted Apps (id, members, child_groups, timestamps)
Connector->>API: ListApps / ListAppsByUpdatedAt
API-->>Connector: Apps (with Members, ChildGroups) + nextPageToken
Connector->>Connector: build resources (App, Group mapping to App)
Connector->>EventFeed: emit app ResourceChangeEvents (OccurredAt = UpdatedAt)
EventFeed-->>Connector: streaming pages, Cursor/HasMore
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
pkg/client/data.go (1)
200-223: Consider precomputing manager emails to avoid O(n²) user generation.
getManagerEmailscans all users, and it is called once per user in Phase 5. This is fine at 200 users, but it scales poorly for larger fixture sizes.Also applies to: 423-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/data.go` around lines 200 - 223, getManagerEmail currently scans all users for each user (O(n²)); instead compute manager emails once and look them up: build a map keyed by department (and optionally separate for managers vs non-managers) during user generation by iterating users once and using getDepartment, isManagerTitle and getUserName to record the appropriate manager email(s), then change getManagerEmail to simply return from that precomputed map; update callers in Phase 5 to pass or access the precomputed mapping rather than triggering repeated scans in getManagerEmail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/client/data.go`:
- Around line 245-250: The hash calculation casts signed ints to uint32
unsafely; add a bounds check for negative inputs (if userIdx < 0 || groupIdx < 0
return false or handle appropriately), then perform the hash using wider signed
types: cast userIdx and groupIdx to int64 and compute h as int64 (use int64
constants like 2654435761 and 2246822519), do the mixing using int64 arithmetic
(or convert to uint64 after the negative check to perform bitwise shifts
safely), and finally compute the positive modulus against 100 before comparing
with coveragePct; update the block referencing userIdx, groupIdx, h, and
coveragePct accordingly to eliminate the signed-to-unsigned conversion.
---
Nitpick comments:
In `@pkg/client/data.go`:
- Around line 200-223: getManagerEmail currently scans all users for each user
(O(n²)); instead compute manager emails once and look them up: build a map keyed
by department (and optionally separate for managers vs non-managers) during user
generation by iterating users once and using getDepartment, isManagerTitle and
getUserName to record the appropriate manager email(s), then change
getManagerEmail to simply return from that precomputed map; update callers in
Phase 5 to pass or access the precomputed mapping rather than triggering
repeated scans in getManagerEmail.
Comment on lines
+245
to
+250
| h := uint32(userIdx)*2654435761 + uint32(groupIdx)*2246822519 | ||
| // Mix bits for better uniformity | ||
| h ^= h >> 16 | ||
| h *= 0x45d9f3b | ||
| h ^= h >> 16 | ||
| return int(h%100) < coveragePct |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine the shouldAssign function
fd -t f 'data.go' pkg/client/
# Read the specific lines around 245-250
sed -n '240,255p' pkg/client/data.goRepository: ConductorOne/baton-demo
Length of output: 438
🏁 Script executed:
# Get the full shouldAssign function
rg -n -B10 'func shouldAssign' pkg/client/data.go | head -20
# Also verify the exact problem with a more complete view
sed -n '235,260p' pkg/client/data.goRepository: ConductorOne/baton-demo
Length of output: 1331
🏁 Script executed:
# Check if there's a gosec configuration or CI configuration mentioning this
find . -name ".gosec*" -o -name "gosec.json" -o -name ".golangci.yml" -o -name "golangci.yml" 2>/dev/null | head -10
# Also search for G115 in the codebase or CI files
rg -i 'G115|gosec' --type yaml --type yml --type json -n . 2>/dev/null | head -20
# Check if there's a Makefile or script that runs gosec
fd -t f '(Makefile|\.mk|*\.sh)' | xargs rg -l 'gosec' 2>/dev/null || trueRepository: ConductorOne/baton-demo
Length of output: 2979
🏁 Script executed:
# Check the golangci.yml for gosec configuration
cat .golangci.yml | grep -A 20 'gosec'
# Let me also search for any G115 references in the codebase or issues
rg 'G115' . 2>/dev/null || echo "No G115 found"
# Let's verify what G115 is - search for "implicit" or "conversion" context around the issue
rg -i 'implicit.*conversion|narrowing' pkg/client/data.go || trueRepository: ConductorOne/baton-demo
Length of output: 438
🏁 Script executed:
# Let's search for the actual gosec rule to understand what issue we're dealing with
# G115 in gosec is typically about implicit type conversions
# Let's verify the exact line numbers and the cast operation
sed -n '245p' pkg/client/data.go
# Let's also check if there are any build/CI logs mentioning this issue
find . -name "*.md" -o -name "*.txt" | xargs rg -l 'G115\|narrowing\|uint32' 2>/dev/null | grep -v vendor | head -5Repository: ConductorOne/baton-demo
Length of output: 129
🏁 Script executed:
# Let's check gosec documentation - search for what rules gosec has
# and understand if G115 is about explicit or implicit conversions
# First, let's see if there's any CI configuration that runs gosec
find . -name "*.yml" -o -name "*.yaml" | grep -v vendor | xargs rg -l 'gosec' 2>/dev/null || true
# Check for GitHub workflows or CI files
fd -t f '(workflow|\.github)' 2>/dev/null | head -5
# Let's also check if this is a real issue by looking for gosec comments in the code
rg -i '#nosec|nolint.*gosec|G115' pkg/client/ 2>/dev/null || echo "No gosec directives found"Repository: ConductorOne/baton-demo
Length of output: 574
Fix G115 at line 245: Remove unsafe signed-to-unsigned int conversion in hash calculation.
The current code casts signed int parameters directly to uint32, which can produce undefined behavior when negative values are passed. This narrowing conversion is flagged by gosec and blocks CI.
Change the hash calculation to use int64 instead of uint32 to avoid signed-to-unsigned conversion issues, and add bounds checks for negative input values:
Proposed fix
+ if userIdx < 0 || groupIdx < 0 {
+ return false
+ }
// Knuth multiplicative hash for good distribution
- h := uint32(userIdx)*2654435761 + uint32(groupIdx)*2246822519
+ h := int64(userIdx)*2654435761 + int64(groupIdx)*2246822519
// Mix bits for better uniformity
h ^= h >> 16
h *= 0x45d9f3b
h ^= h >> 16
+ if h < 0 {
+ h = -h
+ }
return int(h%100) < coveragePct🧰 Tools
🪛 GitHub Actions: ci
[error] 245-245: G115: integer overflow conversion int -> uint32 (gosec)
🪛 GitHub Check: go-lint
[failure] 245-245:
G115: integer overflow conversion int -> uint32 (gosec)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client/data.go` around lines 245 - 250, The hash calculation casts signed
ints to uint32 unsafely; add a bounds check for negative inputs (if userIdx < 0
|| groupIdx < 0 return false or handle appropriately), then perform the hash
using wider signed types: cast userIdx and groupIdx to int64 and compute h as
int64 (use int64 constants like 2654435761 and 2246822519), do the mixing using
int64 arithmetic (or convert to uint64 after the negative check to perform
bitwise shifts safely), and finally compute the positive modulus against 100
before comparing with coveragePct; update the block referencing userIdx,
groupIdx, h, and coveragePct accordingly to eliminate the signed-to-unsigned
conversion.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/client/data.go (1)
987-1001:⚠️ Potential issue | 🟠 MajorNil
*dbResourcereturned alongsidetruein Phase 4 causes panic when Projects or Roles config is 0When
g.config.Projects == 0org.config.Roles == 0, the ScopedRole phase at lines 987-1001 returns(nil, true). The caller inpkg/client/client.go:230checks only the boolean flag and immediately enters a switch statement that accesses fields on the nil pointer (e.g.,dbResource.User,dbResource.ScopedRole), causing a panic. This occurs wheneverScopedRoles > 0but the Projects or Roles config is zero.Add a precondition check to skip the phase or return from Next() when the required config is not satisfied, avoiding the nil return.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/data.go` around lines 987 - 1001, The ScopedRole branch in Next() can return (nil, true) when g.config.Projects==0 or g.config.Roles==0 which causes panics; add a precondition in the ScopedRole generation (around dbResource/ScopedRole creation using g.currentScopedRole and g.config) to detect if g.config.Projects==0 or g.config.Roles==0 and in that case skip the phase (do not return a nil dbResource with true) — either increment g.currentScopedRole and continue to the next iteration or return (nil, false) so callers of Next() won’t treat a nil *dbResource as present; update the logic around dbResource, ScopedRole, g.currentScopedRole and the boolean return to ensure a non-nil *dbResource is only returned with true.
♻️ Duplicate comments (1)
pkg/client/data.go (1)
778-782:⚠️ Potential issue | 🔴 CriticalG115:
int → uint32conversion inshouldAssignstill unresolved (CI blocked).This is the same issue flagged in the previous review round. The
uint32(userIdx)anduint32(groupIdx)casts are unsafe for negative inputs and are blocking CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/data.go` around lines 778 - 782, The unsafe casts in shouldAssign occur because userIdx and groupIdx may be negative before uint32(...) conversion; add an explicit non-negative check at the start of shouldAssign (e.g., if userIdx < 0 || groupIdx < 0 { return false } or handle negatives according to desired semantics) and only then cast to uint32 for the hashing math; reference the shouldAssign function and the userIdx, groupIdx, and coveragePct variables when applying this guard so CI no longer flags the int→uint32 conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/client/data.go`:
- Line 502: Extract the repeated role strings into named constants (e.g., const
Contractor = "Contractor", ServiceAccount = "Service Account", SharedAccount =
"Shared Account") and replace every literal occurrence (for example the
conditional checking level in the if statement that uses "IC", "Senior IC", and
"Contractor") with the constants (use Contractor, ServiceAccount, SharedAccount)
to satisfy goconst; add the new const block near other package-level constants
and update all references throughout the file where those string literals
appear.
- Around line 942-944: The GroupAssignments slice is being populated with
possibly-duplicate entries because groupId(g.currentProject % totalGroups) can
equal groupId((g.currentProject * 10) % totalGroups); update the assignment
logic that builds GroupAssignments (referencing g.currentProject, totalGroups
and groupId) to deduplicate values before writing them (e.g., collect the two
computed ids into a small set or map, then convert back to a slice) so only
unique groupId values are stored and downstream uniqueness constraints are not
violated.
- Around line 704-706: In getRegionForUser, avoid the unsafe int→uint32
conversion for userIdx: ensure userIdx is non-negative or cast via uint before
converting to uint32 (e.g., validate userIdx >= 0 and then use
uint32(uint(userIdx))) so the expression that assigns h (currently: h :=
uint32(userIdx) * 2654435761) is safe; alternatively, add an explicit guard that
normalizes or rejects negative userIdx values before computing h and pct to
satisfy the G115 check.
- Around line 828-898: Service/shared accounts currently fall through into the
full member/admin assignment path; modify the logic so that when
pos.EmploymentType == "Service Account" || "Shared Account" and they pass the
initial non-universal dept check they do not continue through the standard
assignment/admin-evaluation block. Concretely: either wrap the large block
starting at "dept := pos.Department" through admin appends in an else branch of
the initial service/shared if, or after the service/shared check set a
short-circuit (e.g., append as member and continue) so service/shared accounts
never reach the isAdmin calculation (shouldAssign(...,2), isOverPrivileged, VP
check); reference g.positions / pos.EmploymentType, isUniversalApp,
shouldAssign, dept := pos.Department, isOverPrivileged, admins and members to
locate the code to change.
- Around line 1010-1018: The switch on pos.EmploymentType can be empty for tail
positions when buildOrgChart computed effectiveSpecial = 0; add a defensive
guard before that switch in the function that builds the user row (check
pos.EmploymentType == "" or strings.TrimSpace(pos.EmploymentType) == "") and
handle it explicitly (e.g., set fullName and email to empty strings or skip
calling getUserName) so the default branch does not pull incorrect names; keep
existing cases for "Service Account" and "Shared Account" and reference
serviceAccountDefs[g.currentUser-(g.config.Users-numSpecialAccounts)] and
sharedAccountDefs[g.currentUser-(g.config.Users-numSharedAccounts)] unchanged
for their cases.
---
Outside diff comments:
In `@pkg/client/data.go`:
- Around line 987-1001: The ScopedRole branch in Next() can return (nil, true)
when g.config.Projects==0 or g.config.Roles==0 which causes panics; add a
precondition in the ScopedRole generation (around dbResource/ScopedRole creation
using g.currentScopedRole and g.config) to detect if g.config.Projects==0 or
g.config.Roles==0 and in that case skip the phase (do not return a nil
dbResource with true) — either increment g.currentScopedRole and continue to the
next iteration or return (nil, false) so callers of Next() won’t treat a nil
*dbResource as present; update the logic around dbResource, ScopedRole,
g.currentScopedRole and the boolean return to ensure a non-nil *dbResource is
only returned with true.
---
Duplicate comments:
In `@pkg/client/data.go`:
- Around line 778-782: The unsafe casts in shouldAssign occur because userIdx
and groupIdx may be negative before uint32(...) conversion; add an explicit
non-negative check at the start of shouldAssign (e.g., if userIdx < 0 ||
groupIdx < 0 { return false } or handle negatives according to desired
semantics) and only then cast to uint32 for the hashing math; reference the
shouldAssign function and the userIdx, groupIdx, and coveragePct variables when
applying this guard so CI no longer flags the int→uint32 conversion.
| if totalUsers < 50 || userIdx < numCxO { | ||
| return false | ||
| } | ||
| if level != "IC" && level != "Senior IC" && level != "Contractor" { |
There was a problem hiding this comment.
Pipeline failure: extract "Contractor" (and "Service Account", "Shared Account") as named constants to fix the CI goconst error.
golangci-lint reports that the string literal "Contractor" appears 3+ times and the pipeline is blocked. The same lint rule applies to "Service Account" and "Shared Account", which have equally many occurrences.
🔧 Proposed fix
+const (
+ empTypeFullTime = "Full-time"
+ empTypeContractor = "Contractor"
+ empTypeServiceAccount = "Service Account"
+ empTypeSharedAccount = "Shared Account"
+)Then replace every matching string literal throughout the file (e.g., line 502, 636, 660, 831, 1010–1015, etc.) with the corresponding constant.
Also applies to: 636-637, 660-660
🧰 Tools
🪛 GitHub Actions: ci
[error] 502-502: golangci-lint error: string Contractor has 3 occurrences, make it a constant (goconst).
🪛 GitHub Check: go-lint
[failure] 502-502:
string Contractor has 3 occurrences, make it a constant (goconst)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client/data.go` at line 502, Extract the repeated role strings into named
constants (e.g., const Contractor = "Contractor", ServiceAccount = "Service
Account", SharedAccount = "Shared Account") and replace every literal occurrence
(for example the conditional checking level in the if statement that uses "IC",
"Senior IC", and "Contractor") with the constants (use Contractor,
ServiceAccount, SharedAccount) to satisfy goconst; add the new const block near
other package-level constants and update all references throughout the file
where those string literals appear.
Comment on lines
+704
to
+706
| h := uint32(userIdx) * 2654435761 | ||
| h ^= h >> 16 | ||
| pct := int(h % 100) |
There was a problem hiding this comment.
Fix G115: unsafe int → uint32 conversion in getRegionForUser.
Same gosec rule (G115) that blocks CI fires here. userIdx is always non-negative at its call sites, but the compiler does not guarantee that; add an explicit guard or cast through uint.
🐛 Proposed fix
func getRegionForUser(userIdx int) string {
- h := uint32(userIdx) * 2654435761
+ if userIdx < 0 {
+ userIdx = -userIdx
+ }
+ h := uint32(userIdx) * 2654435761 // `#nosec` G115 -- non-negative guard above
h ^= h >> 16
pct := int(h % 100)Alternatively, annotate with //nolint:gosec (or #nosec G115) if the project's lint config accepts that style.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| h := uint32(userIdx) * 2654435761 | |
| h ^= h >> 16 | |
| pct := int(h % 100) | |
| if userIdx < 0 { | |
| userIdx = -userIdx | |
| } | |
| h := uint32(userIdx) * 2654435761 // `#nosec` G115 -- non-negative guard above | |
| h ^= h >> 16 | |
| pct := int(h % 100) |
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 704-704:
G115: integer overflow conversion int -> uint32 (gosec)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client/data.go` around lines 704 - 706, In getRegionForUser, avoid the
unsafe int→uint32 conversion for userIdx: ensure userIdx is non-negative or cast
via uint before converting to uint32 (e.g., validate userIdx >= 0 and then use
uint32(uint(userIdx))) so the expression that assigns h (currently: h :=
uint32(userIdx) * 2654435761) is safe; alternatively, add an explicit guard that
normalizes or rejects negative userIdx values before computing h and pct to
satisfy the G115 check.
Comment on lines
+828
to
+898
| for i := 0; i < g.config.Users; i++ { | ||
| pos := g.positions[i] | ||
|
|
||
| if pos.EmploymentType == "Service Account" || pos.EmploymentType == "Shared Account" { | ||
| if !g.isUniversalApp(g.currentGroup) { | ||
| coverage, inDept := app.DeptCoverage[pos.Department] | ||
| if !inDept || !shouldAssign(i, g.currentGroup, coverage) { | ||
| continue | ||
| } | ||
| } | ||
| } | ||
|
|
||
| dept := pos.Department | ||
| coverage, inTargetDept := app.DeptCoverage[dept] | ||
|
|
||
| assign := false | ||
| if inTargetDept { | ||
| assign = shouldAssign(i, g.currentGroup, coverage) | ||
| } else if app.NoisePct > 0 { | ||
| assign = shouldAssign(i, g.currentGroup+1000, app.NoisePct) | ||
| } | ||
|
|
||
| // Level gate | ||
| if app.MinLevel != "" && assign { | ||
| userLvl := levelOrder[pos.Level] | ||
| minLvl := levelOrder[app.MinLevel] | ||
| if userLvl < minLvl { | ||
| // Acting managers bypass the manager-level gate | ||
| if app.MinLevel == "Manager" && isActingManager(i, g.config.Users, pos.Level) { | ||
| // allowed | ||
| } else { | ||
| assign = false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Region gate | ||
| if app.RegionOnly != "" && assign { | ||
| if pos.Region != app.RegionOnly { | ||
| assign = false | ||
| } | ||
| } | ||
|
|
||
| // Edge: transferred users retain old department access | ||
| if !assign && isTransferred(i, g.config.Users, pos.Department) { | ||
| oldDept := getTransferredFromDept(i, pos.Department) | ||
| if oldCov, ok := app.DeptCoverage[oldDept]; ok { | ||
| assign = shouldAssign(i, g.currentGroup, oldCov) | ||
| } | ||
| } | ||
|
|
||
| // Edge: over-privileged ICs gain random extra access | ||
| if !assign && isOverPrivileged(i, g.config.Users, pos.Level) { | ||
| assign = shouldAssign(i, g.currentGroup+3000, 30) | ||
| } | ||
|
|
||
| if assign { | ||
| members = append(members, userId(i)) | ||
|
|
||
| isAdmin := shouldAssign(i, g.currentGroup, 2) | ||
| if isOverPrivileged(i, g.config.Users, pos.Level) { | ||
| isAdmin = shouldAssign(i, g.currentGroup, 25) | ||
| } | ||
| if inTargetDept && levelOrder[pos.Level] >= levelOrder["VP"] { | ||
| isAdmin = true | ||
| } | ||
| if isAdmin { | ||
| admins = append(admins, userId(i)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Service/Shared accounts fall through to the full member-assignment path and can be elected as group admins.
When a service/shared account passes the early-gate check (lines 831-838) in a non-universal app, execution falls through to the standard path. Because there is no else or early continue after the service-account block, those accounts run through every downstream check including the 2% admin election (shouldAssign(i, g.currentGroup, 2), line 887) and the isOverPrivileged admin path (line 888-890). Service and shared accounts becoming group admins is almost certainly unintended.
♻️ Proposed fix — add an explicit `else` block for regular users
if pos.EmploymentType == "Service Account" || pos.EmploymentType == "Shared Account" {
if !g.isUniversalApp(g.currentGroup) {
coverage, inDept := app.DeptCoverage[pos.Department]
if !inDept || !shouldAssign(i, g.currentGroup, coverage) {
continue
}
}
+ // Service/shared accounts that reach here become plain members only.
+ members = append(members, userId(i))
+ continue
}
dept := pos.Department🧰 Tools
🪛 GitHub Check: go-lint
[failure] 831-831:
string Service Account has 3 occurrences, make it a constant (goconst)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client/data.go` around lines 828 - 898, Service/shared accounts currently
fall through into the full member/admin assignment path; modify the logic so
that when pos.EmploymentType == "Service Account" || "Shared Account" and they
pass the initial non-universal dept check they do not continue through the
standard assignment/admin-evaluation block. Concretely: either wrap the large
block starting at "dept := pos.Department" through admin appends in an else
branch of the initial service/shared if, or after the service/shared check set a
short-circuit (e.g., append as member and continue) so service/shared accounts
never reach the isAdmin calculation (shouldAssign(...,2), isOverPrivileged, VP
check); reference g.positions / pos.EmploymentType, isUniversalApp,
shouldAssign, dept := pos.Department, isOverPrivileged, admins and members to
locate the code to change.
Comment on lines
942
to
944
| groupId(g.currentProject % totalGroups), | ||
| groupId((g.currentProject * 10) % totalGroups), | ||
| }, |
There was a problem hiding this comment.
Project group assignments may contain duplicates.
When g.currentProject % totalGroups == (g.currentProject * 10) % totalGroups (e.g., every 10th project with the current ~90 appGroups), the same groupId is written twice into GroupAssignments. Downstream uniqueness constraints (e.g., the DB schema) may reject these rows.
♻️ Proposed dedup
- GroupAssignments: []string{
- groupId(g.currentProject % totalGroups),
- groupId((g.currentProject * 10) % totalGroups),
- },
+ GroupAssignments: func() []string {
+ a := groupId(g.currentProject % totalGroups)
+ b := groupId((g.currentProject * 10) % totalGroups)
+ if a == b {
+ return []string{a}
+ }
+ return []string{a, b}
+ }(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| groupId(g.currentProject % totalGroups), | |
| groupId((g.currentProject * 10) % totalGroups), | |
| }, | |
| GroupAssignments: func() []string { | |
| a := groupId(g.currentProject % totalGroups) | |
| b := groupId((g.currentProject * 10) % totalGroups) | |
| if a == b { | |
| return []string{a} | |
| } | |
| return []string{a, b} | |
| }(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client/data.go` around lines 942 - 944, The GroupAssignments slice is
being populated with possibly-duplicate entries because groupId(g.currentProject
% totalGroups) can equal groupId((g.currentProject * 10) % totalGroups); update
the assignment logic that builds GroupAssignments (referencing g.currentProject,
totalGroups and groupId) to deduplicate values before writing them (e.g.,
collect the two computed ids into a small set or map, then convert back to a
slice) so only unique groupId values are stored and downstream uniqueness
constraints are not violated.
Comment on lines
+1010
to
+1018
| switch pos.EmploymentType { | ||
| case "Service Account": | ||
| sa := serviceAccountDefs[g.currentUser-(g.config.Users-numSpecialAccounts)] | ||
| fullName = sa.Name | ||
| email = sa.Email | ||
| case "Shared Account": | ||
| sa := sharedAccountDefs[g.currentUser-(g.config.Users-numSharedAccounts)] | ||
| fullName = sa.Name | ||
| email = sa.Email |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the relevant code sections
head -100 pkg/client/data.go | tail -20Repository: ConductorOne/baton-demo
Length of output: 1869
🏁 Script executed:
# Get the total size of the file first
wc -l pkg/client/data.goRepository: ConductorOne/baton-demo
Length of output: 90
🏁 Script executed:
# Read lines around 1010-1018 to see the exact code
sed -n '1005,1025p' pkg/client/data.goRepository: ConductorOne/baton-demo
Length of output: 735
🏁 Script executed:
# Find buildOrgChart function to verify the position layout claims
rg -n "func.*buildOrgChart" pkg/client/data.go -A 2Repository: ConductorOne/baton-demo
Length of output: 176
🏁 Script executed:
# Find where effectiveSpecial is set
rg -n "effectiveSpecial\s*=" pkg/client/data.go -B 2 -A 2Repository: ConductorOne/baton-demo
Length of output: 199
🏁 Script executed:
# Find numSpecialAccounts and numSharedAccounts definitions
rg -n "numSpecialAccounts|numSharedAccounts" pkg/client/data.go -B 1 -A 1Repository: ConductorOne/baton-demo
Length of output: 853
🏁 Script executed:
# Find EmploymentType related constants
rg -n "empType|EmploymentType" pkg/client/data.go | head -30Repository: ConductorOne/baton-demo
Length of output: 576
🏁 Script executed:
# Find numCxO constant
rg -n "const numCxO" pkg/client/data.goRepository: ConductorOne/baton-demo
Length of output: 86
🏁 Script executed:
# Read the full buildOrgChart function to verify position layout
sed -n '558,680p' pkg/client/data.goRepository: ConductorOne/baton-demo
Length of output: 3001
🏁 Script executed:
# Check if there are other places using these special account indices
rg -n "serviceAccountDefs\[|sharedAccountDefs\[" pkg/client/data.goRepository: ConductorOne/baton-demo
Length of output: 299
🏁 Script executed:
# Verify the default config.Users value
rg -n "config\.Users|DefaultUsers" pkg/client/data.go | head -20Repository: ConductorOne/baton-demo
Length of output: 1304
🏁 Script executed:
# Check if there are other uses of pos.EmploymentType in the file
rg -n "pos\.EmploymentType|\.EmploymentType" pkg/client/data.go -B 1 -A 1Repository: ConductorOne/baton-demo
Length of output: 535
🏁 Script executed:
# Verify the suggested variable empTypeFullTime exists
rg -n "empTypeFullTime|\"Full-time\"" pkg/client/data.go | head -10Repository: ConductorOne/baton-demo
Length of output: 131
Add defensive guard when EmploymentType is empty to handle the edge case where totalUsers ≤ 13.
When config.Users ≤ 13 (numCxO + numSpecialAccounts), buildOrgChart sets effectiveSpecial = 0 and the service/shared account population loops never execute, leaving those tail positions with zero-value EmploymentType strings. The switch statement will then fall to the default case for those indices, producing incorrect names/emails from getUserName. Although this edge case is unreachable with the default 200 users, the guard below closes the hole:
🛡️ Suggested defensive guard
if g.currentUser < g.config.Users {
pos := g.positions[g.currentUser]
first, last := getUserName(g.currentUser)
+ // Sanity-check: if effectiveSpecial was 0, EmploymentType is empty;
+ // treat as Full-time rather than mis-indexing a special-account slice.
+ empType := pos.EmploymentType
+ if empType == "" {
+ empType = "Full-time"
+ }
var fullName, email string
- switch pos.EmploymentType {
+ switch empType {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client/data.go` around lines 1010 - 1018, The switch on
pos.EmploymentType can be empty for tail positions when buildOrgChart computed
effectiveSpecial = 0; add a defensive guard before that switch in the function
that builds the user row (check pos.EmploymentType == "" or
strings.TrimSpace(pos.EmploymentType) == "") and handle it explicitly (e.g., set
fullName and email to empty strings or skip calling getUserName) so the default
branch does not pull incorrect names; keep existing cases for "Service Account"
and "Shared Account" and reference
serviceAccountDefs[g.currentUser-(g.config.Users-numSpecialAccounts)] and
sharedAccountDefs[g.currentUser-(g.config.Users-numSharedAccounts)] unchanged
for their cases.
…ling - Added App struct to represent application entities. - Implemented methods for listing, retrieving, granting, and revoking access to apps in the client. - Introduced app event feed for tracking changes to app resources. - Enhanced group handling to associate child groups with their parent apps. - Updated resource types to include app resources for better integration with the connector.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
pkg/client/data.go (3)
1067-1070:⚠️ Potential issue | 🟡 MinorDeduplicate project
GroupAssignmentsto avoid repeated IDs.Line 1067 can emit the same group twice when both computed indices collide.
🔧 Proposed fix
if totalValid > 0 { - groupAssignments = []string{ - groupId(validGroups[g.currentProject%totalValid]), - groupId(validGroups[(g.currentProject*10)%totalValid]), - } + a := groupId(validGroups[g.currentProject%totalValid]) + b := groupId(validGroups[(g.currentProject*10)%totalValid]) + if a == b { + groupAssignments = []string{a} + } else { + groupAssignments = []string{a, b} + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/data.go` around lines 1067 - 1070, The groupAssignments creation can produce duplicate IDs when the two computed indices collide; update the logic around groupAssignments (the slice built from groupId(validGroups[...]) using g.currentProject and totalValid) to ensure unique entries by checking the second computed groupId against the first and only appending if different (or use a small dedup set before assigning), e.g., compute both candidate indices from g.currentProject and (g.currentProject*10), map them through groupId(validGroups[...]) and build groupAssignments with only distinct IDs so no repeated group appears.
914-921:⚠️ Potential issue | 🟠 MajorService/shared accounts can still be elevated to group admins.
After the check at Line 914, execution falls through to the normal admin election path at Line 969. Service/shared accounts should short-circuit as member-only.
🔧 Proposed fix
if pos.EmploymentType == "Service Account" || pos.EmploymentType == "Shared Account" { if !g.isUniversalApp(groupIdx) { coverage, inDept := app.DeptCoverage[pos.Department] if !inDept || !shouldAssign(i, groupIdx, coverage) { continue } } + members = append(members, userId(i)) + continue }Also applies to: 969-978
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/data.go` around lines 914 - 921, Service/shared accounts (pos.EmploymentType == "Service Account" || "Shared Account") are still falling through into the admin-election logic; ensure they are treated as member-only by short-circuiting them before the admin-election path. In the block that checks pos.EmploymentType (and uses g.isUniversalApp, app.DeptCoverage, shouldAssign), either immediately set the role to member and continue the loop or add an explicit continue to skip any subsequent admin-election code (the code that runs the admin election logic for the group) so service/shared accounts never reach the admin election logic.
767-770:⚠️ Potential issue | 🔴 CriticalFix unsafe
int -> uint32casts in hash helpers (G115).Line 768 and Line 842 still cast signed indices directly to
uint32. This matches the previous lint-blocking G115 finding.🔧 Proposed fix
func getRegionForUser(userIdx int) string { - h := uint32(userIdx) * 2654435761 + if userIdx < 0 { + return "Americas" + } + h := int64(userIdx) * 2654435761 h ^= h >> 16 + if h < 0 { + h = -h + } pct := int(h % 100) for _, r := range regionDistribution { if pct < r.Weight { return r.Name } } return "Americas" } @@ func shouldAssign(userIdx, groupIdx, coveragePct int) bool { if coveragePct >= 100 { return true } if coveragePct <= 0 { return false } - h := uint32(userIdx)*2654435761 + uint32(groupIdx)*2246822519 + if userIdx < 0 || groupIdx < 0 { + return false + } + h := int64(userIdx)*2654435761 + int64(groupIdx)*2246822519 h ^= h >> 16 h *= 0x45d9f3b h ^= h >> 16 + if h < 0 { + h = -h + } return int(h%100) < coveragePct }#!/bin/bash # Verify current signed->unsigned casts in hash helpers (G115 candidates). # Expected before fix: matches at getRegionForUser and shouldAssign. # Expected after fix: no direct uint32(userIdx/groupIdx) casts. rg -nP 'uint32\(\s*(userIdx|groupIdx)\s*\)' pkg/client/data.go sed -n '764,772p' pkg/client/data.go sed -n '835,847p' pkg/client/data.goAlso applies to: 842-846
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/data.go` around lines 767 - 770, Replace the unsafe direct signed->unsigned casts by first narrowing to a 32-bit signed type then to uint32: locate getRegionForUser and shouldAssign in pkg/client/data.go and change occurrences of uint32(userIdx) and uint32(groupIdx) to use uint32(int32(userIdx)) and uint32(int32(groupIdx)) respectively so the conversion is explicit and avoids the G115 unsafe cast; keep the rest of the hash computation (h ^= h >> 16, pct := int(h % 100), etc.) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/connector/apps.go`:
- Around line 159-170: The code calls parseAppID(entitlement.Id) but ignores the
parsed entitlement variant/slug and proceeds to GrantAppAccess/RevokeAppAccess;
update both grant and revoke paths to check the second return value from
parseAppID (e.g., entType or slug) and verify it equals the expected entitlement
variant (reject otherwise), and if it doesn't match return a clear error instead
of proceeding; locate parseAppID usage and the calls to o.client.GrantAppAccess
and o.client.RevokeAppAccess and add the validation and explicit error return in
those places.
In `@pkg/connector/groups.go`:
- Around line 371-377: The buildGroupToAppMapping function currently swallows
errors from o.client.ListApps and returns an empty map; change
buildGroupToAppMapping to return (map[string]*v2.ResourceId, error) instead of
only map so callers can handle failures, update the implementation to return the
error from o.client.ListApps when it occurs (instead of an empty map), and
adjust all callers of buildGroupToAppMapping to handle the returned error
(propagate or handle appropriately) so parent app links are not silently
dropped; references: function buildGroupToAppMapping and the call to
o.client.ListApps in pkg/connector/groups.go.
---
Duplicate comments:
In `@pkg/client/data.go`:
- Around line 1067-1070: The groupAssignments creation can produce duplicate IDs
when the two computed indices collide; update the logic around groupAssignments
(the slice built from groupId(validGroups[...]) using g.currentProject and
totalValid) to ensure unique entries by checking the second computed groupId
against the first and only appending if different (or use a small dedup set
before assigning), e.g., compute both candidate indices from g.currentProject
and (g.currentProject*10), map them through groupId(validGroups[...]) and build
groupAssignments with only distinct IDs so no repeated group appears.
- Around line 914-921: Service/shared accounts (pos.EmploymentType == "Service
Account" || "Shared Account") are still falling through into the admin-election
logic; ensure they are treated as member-only by short-circuiting them before
the admin-election path. In the block that checks pos.EmploymentType (and uses
g.isUniversalApp, app.DeptCoverage, shouldAssign), either immediately set the
role to member and continue the loop or add an explicit continue to skip any
subsequent admin-election code (the code that runs the admin election logic for
the group) so service/shared accounts never reach the admin election logic.
- Around line 767-770: Replace the unsafe direct signed->unsigned casts by first
narrowing to a 32-bit signed type then to uint32: locate getRegionForUser and
shouldAssign in pkg/client/data.go and change occurrences of uint32(userIdx) and
uint32(groupIdx) to use uint32(int32(userIdx)) and uint32(int32(groupIdx))
respectively so the conversion is explicit and avoids the G115 unsafe cast; keep
the rest of the hash computation (h ^= h >> 16, pct := int(h % 100), etc.)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b726e76e-c1aa-4b72-8d61-056c6b0e7318
📒 Files selected for processing (7)
pkg/client/client.gopkg/client/data.gopkg/connector/apps.gopkg/connector/connector.gopkg/connector/events.gopkg/connector/groups.gopkg/connector/resource_types.go
Comment on lines
+159
to
+170
| appID, _, err := parseAppID(entitlement.Id) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| userID := principal.Id.Resource | ||
|
|
||
| err = o.client.GrantAppAccess(ctx, appID, userID) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| return nil, nil, nil |
There was a problem hiding this comment.
Validate entitlement slug before app grant/revoke mutations.
Line 159 ignores the parsed entitlement type, and Line 173 revokes without checking slug. Unknown entitlement variants should be rejected explicitly.
🔧 Proposed fix
- appID, _, err := parseAppID(entitlement.Id)
+ appID, grantType, err := parseAppID(entitlement.Id)
if err != nil {
return nil, nil, err
}
+ if grantType != appAccessEntitlement {
+ return nil, nil, status.Errorf(codes.InvalidArgument, "unknown entitlement %q", grantType)
+ }
@@
func (o *appBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations.Annotations, error) {
appID := grant.Entitlement.Resource.Id.Resource
principalId := grant.Principal.Id
@@
+ if grant.Entitlement.Slug != appAccessEntitlement {
+ return nil, status.Errorf(codes.InvalidArgument, "unknown entitlement")
+ }
+
err := o.client.RevokeAppAccess(ctx, appID, principalId.Resource)Also applies to: 173-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connector/apps.go` around lines 159 - 170, The code calls
parseAppID(entitlement.Id) but ignores the parsed entitlement variant/slug and
proceeds to GrantAppAccess/RevokeAppAccess; update both grant and revoke paths
to check the second return value from parseAppID (e.g., entType or slug) and
verify it equals the expected entitlement variant (reject otherwise), and if it
doesn't match return a clear error instead of proceeding; locate parseAppID
usage and the calls to o.client.GrantAppAccess and o.client.RevokeAppAccess and
add the validation and explicit error return in those places.
Comment on lines
+371
to
+377
| // buildGroupToAppMapping builds a lookup from child group IDs to their parent app resource IDs. | ||
| func (o *groupBuilder) buildGroupToAppMapping(ctx context.Context) map[string]*v2.ResourceId { | ||
| result := make(map[string]*v2.ResourceId) | ||
| apps, err := o.client.ListApps(ctx) | ||
| if err != nil { | ||
| return result | ||
| } |
There was a problem hiding this comment.
Do not swallow ListApps failures in group→app mapping.
Line 375 currently hides failures and returns an empty mapping, which silently drops parent links in List/Get.
🔧 Proposed fix
- groupToApp := o.buildGroupToAppMapping(ctx)
+ groupToApp, err := o.buildGroupToAppMapping(ctx)
+ if err != nil {
+ return nil, nil, err
+ }
@@
- groupToApp := o.buildGroupToAppMapping(ctx)
+ groupToApp, err := o.buildGroupToAppMapping(ctx)
+ if err != nil {
+ return nil, nil, err
+ }
@@
-func (o *groupBuilder) buildGroupToAppMapping(ctx context.Context) map[string]*v2.ResourceId {
+func (o *groupBuilder) buildGroupToAppMapping(ctx context.Context) (map[string]*v2.ResourceId, error) {
result := make(map[string]*v2.ResourceId)
apps, err := o.client.ListApps(ctx)
if err != nil {
- return result
+ return nil, err
}
@@
- return result
+ return result, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connector/groups.go` around lines 371 - 377, The buildGroupToAppMapping
function currently swallows errors from o.client.ListApps and returns an empty
map; change buildGroupToAppMapping to return (map[string]*v2.ResourceId, error)
instead of only map so callers can handle failures, update the implementation to
return the error from o.client.ListApps when it occurs (instead of an empty
map), and adjust all callers of buildGroupToAppMapping to handle the returned
error (propagate or handle appropriately) so parent app links are not silently
dropped; references: function buildGroupToAppMapping and the call to
o.client.ListApps in pkg/connector/groups.go.
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
2 participants
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 by CodeRabbit
New Features
Chores