fix(FLEETMDM-001): CU-86akj32d7 48 review findings across 30 files - #165
flamingo[bot] wants to merge 30 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -1,3 +1,4 @@ | |||
| // >>> OPENFRAME(cisco-secure-client-bundle-id): Fork-specific migration to fix Cisco Secure Client bundle-id — openframe/docs/cisco-secure-client-bundle-id.md | |||
| package tables | |||
There was a problem hiding this comment.
🦩 🔴 New Go migration file lacks OPENFRAME sentinel comments for fork tracking
Wrapped the entire fork-specific migration logic (package declaration through the end of Up_20251031154558) with // >>> OPENFRAME(cisco-secure-client-bundle-id): ... — openframe/docs/cisco-secure-client-bundle-id.md and // <<< OPENFRAME(cisco-secure-client-bundle-id) sentinel comments, satisfying the fork-tracking requirement for new migration files under server/datastore/mysql/migrations/tables/.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20251031154558_ChangeCiscoSecureClientBundleId.go around line 1, review and complete this code-review fix: New Go migration file lacks OPENFRAME sentinel comments for fork tracking.
What the draft fix changed: Wrapped the entire fork-specific migration logic (package declaration through the end of `Up_20251031154558`) with `// >>> OPENFRAME(cisco-secure-client-bundle-id): ... — openframe/docs/cisco-secure-client-bundle-id.md` and `// <<< OPENFRAME(cisco-secure-client-bundle-id)` sentinel comments, satisfying the fork-tracking requirement for new migration files under `server/datastore/mysql/migrations/tables/`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| SELECT id | ||
| FROM software_installers | ||
| WHERE title_id = ? | ||
| AND extension = 'pkg' | ||
| `, bundleIdToTitleId["com.cisco.pkg.anyconnect.vpn"]) | ||
| if err != nil { | ||
| return err | ||
| return fmt.Errorf("querying software_installers with incorrect cisco title id: %w", err) | ||
| } | ||
| defer installerRows.Close() | ||
|
|
||
| var softwareInstallerIds []string | ||
| for installerRows.Next() { | ||
| var id string | ||
| if err := installerRows.Scan(&id); err != nil { | ||
| return err | ||
| return fmt.Errorf("scanning software_installers row: %w", err) | ||
| } | ||
| softwareInstallerIds = append(softwareInstallerIds, id) | ||
| } | ||
| if err := installerRows.Err(); err != nil { | ||
| return err | ||
| return fmt.Errorf("iterating software_installers rows: %w", err) | ||
| } | ||
|
|
||
| // Update software installers to point to correct title |
There was a problem hiding this comment.
🦩 🟠 Migration only reassigns installers with extension = 'pkg', but skips exe/msi/deb installers with the incorrect title
Removed the AND extension = 'pkg' filter from the SELECT id FROM software_installers WHERE title_id = ? query in Up_20251031154558, so all installers referencing the incorrect title id (regardless of extension) are repointed before the incorrect software_titles row is deleted, closing the orphaned foreign-key gap described in the finding.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20251031154558_ChangeCiscoSecureClientBundleId.go around line 65, review and complete this code-review fix: Migration only reassigns installers with extension = 'pkg', but skips exe/msi/deb installers with the incorrect title.
What the draft fix changed: Removed the `AND extension = 'pkg'` filter from the `SELECT id FROM software_installers WHERE title_id = ?` query in `Up_20251031154558`, so all installers referencing the incorrect title id (regardless of extension) are repointed before the incorrect `software_titles` row is deleted, closing the orphaned foreign-key gap described in the finding.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| @@ -22,20 +23,20 @@ func Up_20251031154558(tx *sql.Tx) error { | |||
| WHERE bundle_identifier IN ('com.cisco.pkg.anyconnect.vpn', 'com.cisco.secureclient.gui') | |||
| `) | |||
| if err != nil { | |||
There was a problem hiding this comment.
🦩 🟠 Bare error propagation without context wrapping in migration Up function
Replaced every bare return err in Up_20251031154558 with fmt.Errorf("<context>: %w", err), giving each failure point (title query/scan/iteration, insert, last-insert-id, installer query/scan/iteration, installer update, title delete) distinct contextual messages consistent with the rest of the migration batch's style; fmt was already imported so no new import was needed.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20251031154558_ChangeCiscoSecureClientBundleId.go around line 24, review and complete this code-review fix: Bare error propagation without context wrapping in migration Up function.
What the draft fix changed: Replaced every bare `return err` in `Up_20251031154558` with `fmt.Errorf("<context>: %w", err)`, giving each failure point (title query/scan/iteration, insert, last-insert-id, installer query/scan/iteration, installer update, title delete) distinct contextual messages consistent with the rest of the migration batch's style; `fmt` was already imported so no new import was needed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| SET title_id = ? | ||
| WHERE id = ? | ||
| `, bundleIdToTitleId["com.cisco.secureclient.gui"], softwareInstallerId); err != nil { | ||
| return err | ||
| return fmt.Errorf("updating software_installers title_id for id %s: %w", softwareInstallerId, err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 Migration deletes software_titles row that may still be referenced elsewhere (FK/orphan risk)
No schema-level defensive check was added for other tables (host_software, software_title_icons, vpp apps, cve associations) potentially referencing the deleted software_titles row; this finding is architectural/data-model in scope and cannot be safely resolved by a local code change without knowing the actual FK constraints and dependent tables. The safest in-file mitigation already applied (finding 2, removing the extension filter) reduces the likelihood of orphaned installer rows, but does not address other potential referencing tables — a complete fix would require inspecting the full schema and adding cleanup/reassignment logic for each dependent table, which is out of scope for a minimal fix in this file.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20251031154558_ChangeCiscoSecureClientBundleId.go around line 99, review and complete this code-review fix: Migration deletes software_titles row that may still be referenced elsewhere (FK/orphan risk).
What the draft fix changed: No schema-level defensive check was added for other tables (host_software, software_title_icons, vpp apps, cve associations) potentially referencing the deleted `software_titles` row; this finding is architectural/data-model in scope and cannot be safely resolved by a local code change without knowing the actual FK constraints and dependent tables. The safest in-file mitigation already applied (finding 2, removing the extension filter) reduces the likelihood of orphaned installer rows, but does not address other potential referencing tables — a complete fix would require inspecting the full schema and adding cleanup/reassignment logic for each dependent table, which is out of scope for a minimal fix in this file.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 30 low — review closely — react 👍/👎 to teach the reviewer
| @@ -1,5 +1,12 @@ | |||
| package redis | |||
There was a problem hiding this comment.
🦩 🔴 New fork file keyprefix.go lacks OPENFRAME sentinel wrapping despite being net-new fork logic outside openframe/ paths
Added an "OPENFRAME BEGIN"/"OPENFRAME END" sentinel comment block wrapping the entire file content (right after the package redis line and at the end of the file), explaining this is fork-only multi-tenant key-prefixing logic that must be preserved verbatim on upstream syncs. This addresses the finding's literal request ("adding OPENFRAME sentinel comments around the whole file") without moving the package, since relocating it to an openframe/-namespaced package would be a larger architectural change (it would require updating every caller across the datastore package) that I can't safely do blind to those call sites in this single-file fix. Risk: if the project's actual sentinel convention uses a different exact token/format (e.g. // OPENFRAME: single-line markers or a specific machine-parseable syntax) than what I invented here, tooling that scans for sentinels may not recognize this format — a complete fix would need the exact sentinel convention used elsewhere in the repo.
🤖 Prompt for AI agents
In server/datastore/redis/keyprefix.go around line 1, review and complete this code-review fix: New fork file keyprefix.go lacks OPENFRAME sentinel wrapping despite being net-new fork logic outside openframe/ paths.
What the draft fix changed: Added an "OPENFRAME BEGIN"/"OPENFRAME END" sentinel comment block wrapping the entire file content (right after the `package redis` line and at the end of the file), explaining this is fork-only multi-tenant key-prefixing logic that must be preserved verbatim on upstream syncs. This addresses the finding's literal request ("adding OPENFRAME sentinel comments around the whole file") without moving the package, since relocating it to an `openframe/`-namespaced package would be a larger architectural change (it would require updating every caller across the datastore package) that I can't safely do blind to those call sites in this single-file fix. Risk: if the project's actual sentinel convention uses a different exact token/format (e.g. `// OPENFRAME:` single-line markers or a specific machine-parseable syntax) than what I invented here, tooling that scans for sentinels may not recognize this format — a complete fix would need the exact sentinel convention used elsewhere in the repo.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| } | ||
|
|
||
| // >>> OPENFRAME(authz-platform-http-migration): re-export platform_http error types for backward compatibility during the authz->platform_http migration — openframe/docs/authz-platform-http-migration.md | ||
| // CheckMissing is the error to return when no authorization check was performed |
There was a problem hiding this comment.
🦩 🟠 authz.CheckMissing alias in shared upstream file lacks OPENFRAME sentinel comments
Wrapped the CheckMissing type alias and CheckMissingWithResponse var declaration at the end of server/authz/errors.go with // >>> OPENFRAME(authz-platform-http-migration): ... — openframe/docs/authz-platform-http-migration.md and // <<< OPENFRAME(authz-platform-http-migration) sentinel comments, so upstream sync tooling can identify this fork-specific migration shim.
🤖 Prompt for AI agents
In server/authz/errors.go around line 75, review and complete this code-review fix: authz.CheckMissing alias in shared upstream file lacks OPENFRAME sentinel comments.
What the draft fix changed: Wrapped the `CheckMissing` type alias and `CheckMissingWithResponse` var declaration at the end of server/authz/errors.go with `// >>> OPENFRAME(authz-platform-http-migration): ... — openframe/docs/authz-platform-http-migration.md` and `// <<< OPENFRAME(authz-platform-http-migration)` sentinel comments, so upstream sync tooling can identify this fork-specific migration shim.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| ? APP_CONTEXT_NO_TEAM_SUMMARY.name | ||
| : team.name; | ||
|
|
||
| // >>> OPENFRAME(FLEETMDM-001): fork-specific "fleet" rename of getTeamDisplayName | ||
| // added for ITokenFleet; reuses APP_CONTEXT_NO_TEAM_SUMMARY.name for both team | ||
| // and fleet display names. Preserve this block through upstream syncs. | ||
| export const getFleetDisplayName = (fleet: ITokenFleet) => | ||
| fleet.fleet_id === APP_CONTEXT_NO_TEAM_ID | ||
| ? APP_CONTEXT_NO_TEAM_SUMMARY.name | ||
| : fleet.name; | ||
| // <<< OPENFRAME(FLEETMDM-001) |
There was a problem hiding this comment.
🦩 🟠 TypeScript rename in shared frontend/interfaces/team.ts lacks OPENFRAME sentinel comments
In frontend/interfaces/team.ts, wrapped the fork-specific rename logic in // >>> OPENFRAME(FLEETMDM-001) / // <<< OPENFRAME(FLEETMDM-001) sentinel comments: (a) the APP_CONTEXT_ALL_TEAMS_SUMMARY constant whose name was changed from upstream "All teams" to "All fleets", and (b) the new getFleetDisplayName function (using ITokenFleet) that duplicates getTeamDisplayName's logic while reusing APP_CONTEXT_NO_TEAM_SUMMARY.name. getTeamDisplayName itself was left unwrapped since its body is unchanged from upstream. Risk/unverified: I inferred the ticket id FLEETMDM-001 from the finding text since no other sentinel convention was visible in this file; if the project's actual sentinel format differs (e.g. different ticket key or comment style used elsewhere in the repo), this should be aligned to match. No behavior was changed.
🤖 Prompt for AI agents
In frontend/interfaces/team.ts around line 153, review and complete this code-review fix: TypeScript rename in shared frontend/interfaces/team.ts lacks OPENFRAME sentinel comments.
What the draft fix changed: In `frontend/interfaces/team.ts`, wrapped the fork-specific rename logic in `// >>> OPENFRAME(FLEETMDM-001)` / `// <<< OPENFRAME(FLEETMDM-001)` sentinel comments: (a) the `APP_CONTEXT_ALL_TEAMS_SUMMARY` constant whose `name` was changed from upstream "All teams" to "All fleets", and (b) the new `getFleetDisplayName` function (using `ITokenFleet`) that duplicates `getTeamDisplayName`'s logic while reusing `APP_CONTEXT_NO_TEAM_SUMMARY.name`. `getTeamDisplayName` itself was left unwrapped since its body is unchanged from upstream. Risk/unverified: I inferred the ticket id `FLEETMDM-001` from the finding text since no other sentinel convention was visible in this file; if the project's actual sentinel format differs (e.g. different ticket key or comment style used elsewhere in the repo), this should be aligned to match. No behavior was changed.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| @@ -25,7 +25,7 @@ func (ds *Datastore) CountHostsInTargets(ctx context.Context, filter fleet.TeamF | |||
| // >>> OPENFRAME(mysql-multitenancy): fence target host resolution to this process's pinned team | |||
There was a problem hiding this comment.
🦩 🟠 OPENFRAME sentinel comment missing required doc link on mysql multitenancy fence
Appended the required trailing doc reference — openframe/docs/mysql-multitenancy.md to both OPENFRAME sentinel comment blocks (in CountHostsInTargets and HostIDsInTargets) in server/datastore/mysql/targets.go, satisfying the FLEETMDM-001 sentinel format. This does not create the referenced doc file itself (out of scope for this Go source file), so a complete fix would additionally require adding openframe/docs/mysql-multitenancy.md to the repository documentation tree.
🤖 Prompt for AI agents
In server/datastore/mysql/targets.go around line 25, review and complete this code-review fix: OPENFRAME sentinel comment missing required doc link on mysql multitenancy fence.
What the draft fix changed: Appended the required trailing doc reference `— openframe/docs/mysql-multitenancy.md` to both OPENFRAME sentinel comment blocks (in `CountHostsInTargets` and `HostIDsInTargets`) in `server/datastore/mysql/targets.go`, satisfying the FLEETMDM-001 sentinel format. This does not create the referenced doc file itself (out of scope for this Go source file), so a complete fix would additionally require adding `openframe/docs/mysql-multitenancy.md` to the repository documentation tree.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| const ServerSecretPrefix = "FLEET_SECRET_" | ||
|
|
||
| // >>> OPENFRAME(host-secrets): Fleet-internal host-scoped secrets support | ||
| // HostSecretPrefix is used for host-scoped secrets that are looked up by |
There was a problem hiding this comment.
🦩 🟠 HostSecretPrefix and host secret constants added to shared upstream file without OPENFRAME sentinel comments
Wrapped the HostSecretPrefix constant declaration and the HostSecretRecoveryLockPassword/HostSecretRecoveryLockPendingPassword/HostSecretMDMUnlockToken const block in server/fleet/secrets.go with // >>> OPENFRAME(host-secrets): Fleet-internal host-scoped secrets support and // <<< OPENFRAME(host-secrets) sentinel comments, marking the fork-specific additions so upstream merges can identify and preserve them. No other lines (imports, ServerSecretPrefix, MissingSecretsError) were touched.
🤖 Prompt for AI agents
In server/fleet/secrets.go around line 10, review and complete this code-review fix: HostSecretPrefix and host secret constants added to shared upstream file without OPENFRAME sentinel comments.
What the draft fix changed: Wrapped the `HostSecretPrefix` constant declaration and the `HostSecretRecoveryLockPassword`/`HostSecretRecoveryLockPendingPassword`/`HostSecretMDMUnlockToken` const block in `server/fleet/secrets.go` with `// >>> OPENFRAME(host-secrets): Fleet-internal host-scoped secrets support` and `// <<< OPENFRAME(host-secrets)` sentinel comments, marking the fork-specific additions so upstream merges can identify and preserve them. No other lines (imports, `ServerSecretPrefix`, `MissingSecretsError`) were touched.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| ) | ||
|
|
||
| // >>> OPENFRAME(clear-passcode-ref): fork-specific migration to support Android AMAPI clear-passcode commands | ||
| func init() { |
There was a problem hiding this comment.
🦩 🟠 New Fleet migration files lack OPENFRAME sentinel comments
Wrapped the fork-specific migration logic in 20260528211626_AddClearPasscodeRefToHostMDMActions.go with // >>> OPENFRAME(clear-passcode-ref): ... and // <<< OPENFRAME(clear-passcode-ref) sentinel comments around the entire migration body (init function through Down_20260528211626), per the finding's request that fork-specific Android AMAPI clear-passcode migration additions be marked for upstream-sync identification. This is a judgment call since no existing OPENFRAME sentinel convention/format is visible elsewhere in the provided material to confirm exact syntax expectations; a complete fix may require confirming the sentinel format against other files in the repo that already use it.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20260528211626_AddClearPasscodeRefToHostMDMActions.go around line 8, review and complete this code-review fix: New Fleet migration files lack OPENFRAME sentinel comments.
What the draft fix changed: Wrapped the fork-specific migration logic in `20260528211626_AddClearPasscodeRefToHostMDMActions.go` with `// >>> OPENFRAME(clear-passcode-ref): ...` and `// <<< OPENFRAME(clear-passcode-ref)` sentinel comments around the entire migration body (init function through Down_20260528211626), per the finding's request that fork-specific Android AMAPI clear-passcode migration additions be marked for upstream-sync identification. This is a judgment call since no existing OPENFRAME sentinel convention/format is visible elsewhere in the provided material to confirm exact syntax expectations; a complete fix may require confirming the sentinel format against other files in the repo that already use it.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 48 review findings across 30 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
server/datastore/mysql/migrations/tables/20251031154558_ChangeCiscoSecureClientBundleId.go:1server/datastore/mysql/migrations/tables/20251031154558_ChangeCiscoSecureClientBundleId.go:65server/datastore/mysql/migrations/tables/20251031154558_ChangeCiscoSecureClientBundleId.go:24server/datastore/mysql/migrations/tables/20251031154558_ChangeCiscoSecureClientBundleId.go:99server/datastore/redis/keyprefix.go:1server/datastore/redis/keyprefix.go:172server/datastore/redis/keyprefix.go:184server/datastore/redis/keyprefix.go:85server/mdm/maintainedapps/sync.go:32server/mdm/maintainedapps/sync.go:96server/mdm/maintainedapps/sync.go:134server/platform/endpointer/json_key_rewriter.go:1server/platform/endpointer/json_key_rewriter.go:84server/platform/endpointer/json_key_rewriter.go:195server/service/openframe/openframe-encryption-service.go:33server/service/openframe/openframe-encryption-service.go:42server/service/openframe/openframe-encryption-service.go:23ee/orbit/pkg/hostidentity/host_identity.go:99ee/orbit/pkg/hostidentity/host_identity.go:263server/datastore/mysql/migrations/openframe/20260626000001_ScopeHostIdentityUniqueToTeam.go:1server/datastore/mysql/migrations/openframe/20260626000001_ScopeHostIdentityUniqueToTeam.go:98server/datastore/mysql/migrations/tables/20260409153713_AddNameToNanoCommands.go:12server/datastore/mysql/migrations/tables/20260409153713_AddNameToNanoCommands.go:12server/mail/mfa.go:1server/mail/mfa.go:22server/datastore/mysqlredis/hosts.go:1server/datastore/mysqlredis/hosts.go:127charts/fleet/templates/job-migration.yaml:229server/service/microsoft_mdm_integration_test.go:1server/datastore/mysql/campaigns.go:83server/service/endpoint_campaigns.go:96server/fleet/authz.go:1server/mdm/apple/install_application.go:1charts/fleet/templates/deployment.yaml:223orbit/pkg/osquery/flags.go:38server/fleet/acme.go:1server/datastore/mysql/migrations/openframe/20260818000001_AddPoliciesOpenframeManagedColumn.go:25server/service/labels_util.go:74charts/fleet/templates/rbac.yaml:16server/datastore/mysql/query_results.go:50server/service/scim.go:1charts/fleet/values.yaml:132charts/fleet/values.yaml:334server/authz/errors.go:75frontend/interfaces/team.ts:153server/datastore/mysql/targets.go:25server/fleet/secrets.go:10server/datastore/mysql/migrations/tables/20260528211626_AddClearPasscodeRefToHostMDMActions.go:8What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
1446a072-096e-4294-8082-c7cadffe76deMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akj32d7 FleetMDM bulk review findings sweep (12 PRs)