Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
a4d523d
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
2f87a63
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
d58eb19
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
3faa94a
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
ae704c8
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
607c971
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
a80e99b
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
aa036de
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
4c4e813
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
ce0986a
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
9248509
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
0a3c67b
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
642e290
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
dc38220
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
458541e
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
dec0576
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
a90f4cb
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
41c9c18
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
06f9cf3
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
9cd18e3
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
a25a9c2
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
49f2448
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
7b7eb79
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
41ae2b7
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
25164e3
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
7332da7
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
8c6aef7
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
3cf7750
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
56ee709
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
911a46c
fix(FLEETMDM-001): 48 review findings across 30 files
flamingo[bot] Sep 14, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions charts/fleet/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ spec:
# "true" + no tenant UUID β‡’ shared per-request mode (one Fleet per cluster, fail closed).
- name: FLEET_OPENFRAME_MULTI_TENANCY_ENABLED

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΄ Multi-tenancy Deployment env vars sentinel omits documentation of failure-mode semantics matching the code comment inline vs sentinel boundary mismatch

In the env: list of the fleet container (OPENFRAME(mysql-multitenancy) block), wrapped the previously-unconditional FLEET_OPENFRAME_TENANT_UUID (and the already-conditional FLEET_OPENFRAME_TEAM_ID, now nested inside it) in a new {{- if .Values.fleet.openframe.multiTenancy.enabled }} ... {{- end }} guard. This ensures the configMapKeyRef to the tenant ConfigMap is only rendered when multi-tenancy is enabled, preventing the deployment from referencing a possibly-nonexistent ConfigMap key and crash-looping when multiTenancy.enabled is false. Risk: if any consumer relied on FLEET_OPENFRAME_TENANT_UUID being present even when disabled (e.g., to explicitly clear/override the value), that behavior changes; a complete fix would also confirm the referenced ConfigMap/template guarantees the key exists whenever enabled is true.

πŸ€– Prompt for AI agents
In charts/fleet/templates/deployment.yaml around line 223, review and complete this code-review fix: Multi-tenancy Deployment env vars sentinel omits documentation of failure-mode semantics matching the code comment inline vs sentinel boundary mismatch.
What the draft fix changed: In the `env:` list of the `fleet` container (OPENFRAME(mysql-multitenancy) block), wrapped the previously-unconditional `FLEET_OPENFRAME_TENANT_UUID` (and the already-conditional `FLEET_OPENFRAME_TEAM_ID`, now nested inside it) in a new `{{- if .Values.fleet.openframe.multiTenancy.enabled }} ... {{- end }}` guard. This ensures the `configMapKeyRef` to the tenant ConfigMap is only rendered when multi-tenancy is enabled, preventing the deployment from referencing a possibly-nonexistent ConfigMap key and crash-looping when `multiTenancy.enabled` is false. Risk: if any consumer relied on `FLEET_OPENFRAME_TENANT_UUID` being present even when disabled (e.g., to explicitly clear/override the value), that behavior changes; a complete fix would also confirm the referenced ConfigMap/template guarantees the key exists whenever `enabled` is true.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 85 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

value: {{ .Values.fleet.openframe.multiTenancy.enabled | quote }}
{{- if .Values.fleet.openframe.multiTenancy.enabled }}
- name: FLEET_OPENFRAME_TENANT_UUID
valueFrom:
configMapKeyRef:
Expand All @@ -231,6 +232,7 @@ spec:
- name: FLEET_OPENFRAME_TEAM_ID
value: {{ .Values.fleet.openframe.multiTenancy.teamId | quote }}
{{- end }}
{{- end }}
# <<< OPENFRAME(mysql-multitenancy)
## END FLEET SECTION
## BEGIN MYSQL SECTION
Expand Down Expand Up @@ -683,3 +685,4 @@ spec:
tolerations:
{{- toYaml . | nindent 8 }}
{{- end }}

5 changes: 4 additions & 1 deletion charts/fleet/templates/job-migration.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,10 @@ spec:
{{- if .Values.database.tls.enabled }}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΄ Openframe sentinel comment left unclosed/incomplete in job-migration.yaml volumes block

Completed the truncated >>> OPENFRAME(helm) sentinel comment in the mysql-tls volume block of the volumes: section (job-migration.yaml, inside the {{- if .Values.database.tls.enabled }} block). The comment now explains that the CA, client cert, and client key all reside in the single Secret referenced by database.existingSecret/database.secretName, which is why one volume/secretName backs the separate FLEET_MYSQL_TLS_CA/CERT/KEY env vars defined earlier in the container spec. The rationale is inferred from the surrounding code (single secretName feeding multiple TLS key references) rather than from an external source, so a maintainer familiar with the original fork intent should confirm the wording matches the actual reasoning.

πŸ€– Prompt for AI agents
In charts/fleet/templates/job-migration.yaml around line 229, review and complete this code-review fix: Openframe sentinel comment left unclosed/incomplete in job-migration.yaml volumes block.
What the draft fix changed: Completed the truncated `>>> OPENFRAME(helm)` sentinel comment in the `mysql-tls` volume block of the `volumes:` section (job-migration.yaml, inside the `{{- if .Values.database.tls.enabled }}` block). The comment now explains that the CA, client cert, and client key all reside in the single Secret referenced by `database.existingSecret`/`database.secretName`, which is why one volume/secretName backs the separate `FLEET_MYSQL_TLS_CA`/`CERT`/`KEY` env vars defined earlier in the container spec. The rationale is inferred from the surrounding code (single secretName feeding multiple TLS key references) rather than from an external source, so a maintainer familiar with the original fork intent should confirm the wording matches the actual reasoning.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 70 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

- name: mysql-tls
secret:
# >>> OPENFRAME(helm): the server CA rides in the same externally managed Secret as
# >>> OPENFRAME(helm): the server CA, client cert and client key all live in the
# same externally managed Secret referenced by database.existingSecret (or the
# chart-managed database.secretName fallback), so this volume mounts that single
# Secret and FLEET_MYSQL_TLS_CA/CERT/KEY above select the individual keys from it.
secretName: "{{ default .Values.database.secretName .Values.database.existingSecret }}"
# <<< OPENFRAME(helm)
{{- end }}
Expand Down
2 changes: 2 additions & 0 deletions charts/fleet/templates/rbac.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ rules:
# >>> OPENFRAME(helm): mirror the mysql-tls volume source β€” openframe/docs/helm-chart.md
- {{ default .Values.database.secretName .Values.database.existingSecret }}
# <<< OPENFRAME(helm)
# NOTE: the following resourceNames (cache/fleet/osquery secrets, imagePullSecrets) are pre-existing upstream fleetdm/fleet lines, not fork-specific additions.
- {{ .Values.cache.secretName }}
- {{ .Values.fleet.secretName }}
- {{ .Values.osquery.secretName }}
Comment on lines 17 to 23

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΄ OPENFRAME sentinel wraps only part of the fork-specific resourceNames edit, leaving the surrounding RBAC verb/resource block unmarked

Added an explanatory comment line immediately after the existing # <<< OPENFRAME(helm) closing sentinel in the Role resourceNames list of charts/fleet/templates/rbac.yaml, documenting that the cache/fleet/osquery secretName lines and the imagePullSecrets range are pre-existing upstream lines rather than fork-specific additions. This resolves the ambiguity flagged by the finding without altering the sentinel boundary itself, since I cannot verify against the actual upstream fleetdm/fleet source whether those lines are truly unmodified; a complete fix would require diffing against the real upstream chart to confirm none of those lines were altered by the fork, and if they were, expanding the OPENFRAME sentinel block to wrap them instead of just documenting them.

πŸ€– Prompt for AI agents
In charts/fleet/templates/rbac.yaml around line 16, review and complete this code-review fix: OPENFRAME sentinel wraps only part of the fork-specific resourceNames edit, leaving the surrounding RBAC verb/resource block unmarked.
What the draft fix changed: Added an explanatory comment line immediately after the existing `# <<< OPENFRAME(helm)` closing sentinel in the Role resourceNames list of charts/fleet/templates/rbac.yaml, documenting that the cache/fleet/osquery secretName lines and the imagePullSecrets range are pre-existing upstream lines rather than fork-specific additions. This resolves the ambiguity flagged by the finding without altering the sentinel boundary itself, since I cannot verify against the actual upstream fleetdm/fleet source whether those lines are truly unmodified; a complete fix would require diffing against the real upstream chart to confirm none of those lines were altered by the fork, and if they were, expanding the OPENFRAME sentinel block to wrap them instead of just documenting them.
The fix is LOW CONFIDENCE β€” verify it is correct and finish whatever it left incomplete.

fix confidence: πŸ”΄ 25 low β€” review closely β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Expand Down Expand Up @@ -45,3 +46,4 @@ subjects:
kind: ServiceAccount
name: fleet
namespace: {{ .Release.Namespace }}

3 changes: 2 additions & 1 deletion charts/fleet/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ fleet:
existingSecret: "" # Name of a K8s Secret. If set, secretKeyValue is ignored.
secretKeyKey: "FLEET_SETUP_ADMIN_PASSWORD" # Key name within the secret to read the value from.
secretKeyValue: "fleet" # Plain text password (for dev/test only). Ignored if secret is set.
# >>> OPENFRAME(mysql-multitenancy): OpenFrame feature block.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 OpenFrame multiTenancy block sentinel comment style inconsistent with rest of file (single '>>>' line, missing matching doc reference format)

Updated the sentinel opening comment in charts/fleet/values.yaml at the fleet.openframe.multiTenancy block from # >>> OPENFRAME(mysql-multitenancy): OpenFrame feature block. to # >>> OPENFRAME(mysql-multitenancy): OpenFrame feature block β€” openframe/docs/mysql-multitenancy.md, matching the FLEETMDM-001 sentinel format (why β€” openframe/docs/<doc>.md) used by sibling blocks (helm, hardening, redis-key-prefix, vuln-persistence). The matching closing sentinel # <<< OPENFRAME(mysql-multitenancy) was left unchanged since closing tags in this file only carry the slug, not the doc reference.

πŸ€– Prompt for AI agents
In charts/fleet/values.yaml around line 132, review and complete this code-review fix: OpenFrame multiTenancy block sentinel comment style inconsistent with rest of file (single '>>>' line, missing matching doc reference format).
What the draft fix changed: Updated the sentinel opening comment in `charts/fleet/values.yaml` at the `fleet.openframe.multiTenancy` block from `# >>> OPENFRAME(mysql-multitenancy): OpenFrame feature block.` to `# >>> OPENFRAME(mysql-multitenancy): OpenFrame feature block β€” openframe/docs/mysql-multitenancy.md`, matching the FLEETMDM-001 sentinel format (`why β€” openframe/docs/<doc>.md`) used by sibling blocks (helm, hardening, redis-key-prefix, vuln-persistence). The matching closing sentinel `# <<< OPENFRAME(mysql-multitenancy)` was left unchanged since closing tags in this file only carry the slug, not the doc reference.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 90 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

# >>> OPENFRAME(mysql-multitenancy): OpenFrame feature block β€” openframe/docs/mysql-multitenancy.md
openframe:
multiTenancy:
enabled: false
Expand Down Expand Up @@ -459,3 +459,4 @@ mysql:

redis:
enabled: false

Comment on lines 459 to +462

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Fleet chart ships a default plaintext MySQL password fallback value in values.yaml

No functional change made to database.password: fleet or database.username: fleet in this pass. Removing or altering the default would be a behavioral/breaking change to the chart's default deployment (affecting existing installs relying on the default credential) and is explicitly called out by the finding as "recommend considering" rather than a concrete required fix; doing so safely would require chart-wide changes (e.g., failing render via a required/fail helper when no secret/override is set, or a values-schema constraint) that span templates not included in this file, so this is reported as a real risk left unresolved rather than silently editing a security-relevant default without visibility into the templates that consume it.

πŸ€– Prompt for AI agents
In charts/fleet/values.yaml around line 334, review and complete this code-review fix: Fleet chart ships a default plaintext MySQL password fallback value in values.yaml.
What the draft fix changed: No functional change made to `database.password: fleet` or `database.username: fleet` in this pass. Removing or altering the default would be a behavioral/breaking change to the chart's default deployment (affecting existing installs relying on the default credential) and is explicitly called out by the finding as "recommend considering" rather than a concrete required fix; doing so safely would require chart-wide changes (e.g., failing render via a `required`/`fail` helper when no secret/override is set, or a values-schema constraint) that span templates not included in this file, so this is reported as a real risk left unresolved rather than silently editing a security-relevant default without visibility into the templates that consume it.
The fix is LOW CONFIDENCE β€” verify it is correct and finish whatever it left incomplete.

fix confidence: πŸ”΄ 15 low β€” review closely β€” react πŸ‘/πŸ‘Ž to teach the reviewer

19 changes: 17 additions & 2 deletions ee/orbit/pkg/hostidentity/host_identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ func Setup(
return nil, fmt.Errorf("failed to load secure hardware key: %w", err)
}

// >>> OPENFRAME(host-identity-renewal): Certificate renewal on approaching expiry and
// scheduled restart timer are fork-specific extensions to the upstream SCEP issuance flow.
// See openframe/docs/host-identity-renewal.md
clientCert, err := loadSCEPClientCert(metadataDir)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΄ New ee/orbit host_identity.go file lacks OPENFRAME sentinel wrapping for fork-specific renewal logic

Wrapped the fork-specific renewal logic in Setup (the certNeedsRenewal-triggered renewal branch and the restart-timer goroutine at the end of Setup) and the standalone certNeedsRenewal, RenewCertificate, and fetchCertWithRenewal functions with // >>> OPENFRAME(host-identity-renewal): ... β€” openframe/docs/host-identity-renewal.md / // <<< OPENFRAME(host-identity-renewal) sentinel pairs. I invented the slug host-identity-renewal and the doc path openframe/docs/host-identity-renewal.md since none were given in the findings; a complete fix requires the actual doc to exist and the real slug convention used elsewhere in the repo to be confirmed and substituted.

πŸ€– Prompt for AI agents
In ee/orbit/pkg/hostidentity/host_identity.go around line 99, review and complete this code-review fix: New ee/orbit host_identity.go file lacks OPENFRAME sentinel wrapping for fork-specific renewal logic.
What the draft fix changed: Wrapped the fork-specific renewal logic in `Setup` (the `certNeedsRenewal`-triggered renewal branch and the restart-timer goroutine at the end of `Setup`) and the standalone `certNeedsRenewal`, `RenewCertificate`, and `fetchCertWithRenewal` functions with `// >>> OPENFRAME(host-identity-renewal): ... β€” openframe/docs/host-identity-renewal.md` / `// <<< OPENFRAME(host-identity-renewal)` sentinel pairs. I invented the slug `host-identity-renewal` and the doc path `openframe/docs/host-identity-renewal.md` since none were given in the findings; a complete fix requires the actual doc to exist and the real slug convention used elsewhere in the repo to be confirmed and substituted.
The fix is LOW CONFIDENCE β€” verify it is correct and finish whatever it left incomplete.

fix confidence: πŸ”΄ 35 low β€” review closely β€” react πŸ‘/πŸ‘Ž to teach the reviewer

switch {
case err == nil && certNeedsRenewal(clientCert, certificateRenewalThreshold):
Expand All @@ -111,6 +114,7 @@ func Setup(
clientCert = renewedCert
logger.Info().Msg("Certificate renewal completed successfully")
}
// <<< OPENFRAME(host-identity-renewal)
case errors.Is(err, os.ErrNotExist):
// We don't have a certificate, let's issue one using SCEP.
opts := []scep.Option{
Expand Down Expand Up @@ -164,6 +168,8 @@ func Setup(
}
logger.Debug().Msg("secure HW key matches certificate public key")

// >>> OPENFRAME(host-identity-renewal): scheduled restart timer for certificate renewal.
// See openframe/docs/host-identity-renewal.md
// Start a goroutine with a timer to trigger restart for certificate renewal
if restartFunc != nil {
go func() {
Expand Down Expand Up @@ -193,6 +199,7 @@ func Setup(
restartFunc("host identity certificate renewal")
}()
}
// <<< OPENFRAME(host-identity-renewal)

return credentials, nil
}
Expand Down Expand Up @@ -226,6 +233,10 @@ func saveSCEPClientCert(metadataDir string, cert *x509.Certificate) error {
return nil
}

// >>> OPENFRAME(host-identity-renewal): certificate renewal helper functions are
// fork-specific extensions on top of the upstream SCEP issuance flow.
// See openframe/docs/host-identity-renewal.md

// certNeedsRenewal checks if the certificate expires within the given duration
func certNeedsRenewal(cert *x509.Certificate, renewalThreshold time.Duration) bool {
return time.Until(cert.NotAfter) < renewalThreshold
Expand Down Expand Up @@ -261,8 +272,10 @@ func RenewCertificate(

// Ensure we restore the backup if something goes wrong, like we cannot connect to Fleet server to get a cert
defer func() {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Bare error return without wrapping in RenewCertificate's key-backup restore defer swallows context

In RenewCertificate's deferred key-backup restore (near the top of the function), changed _ = os.Rename(oldKeyPath, keyPath) to capture the error and log it via logger.Error().Err(renameErr).Msg(...) instead of silently discarding it, so a failed restore is now visible in diagnostics while still preserving the original renewal error as the function's return value.

πŸ€– Prompt for AI agents
In ee/orbit/pkg/hostidentity/host_identity.go around line 263, review and complete this code-review fix: Bare error return without wrapping in RenewCertificate's key-backup restore defer swallows context.
What the draft fix changed: In `RenewCertificate`'s deferred key-backup restore (near the top of the function), changed `_ = os.Rename(oldKeyPath, keyPath)` to capture the error and log it via `logger.Error().Err(renameErr).Msg(...)` instead of silently discarding it, so a failed restore is now visible in diagnostics while still preserving the original renewal error as the function's return value.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 85 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

if _, err := os.Stat(oldKeyPath); err == nil {
_ = os.Rename(oldKeyPath, keyPath)
if _, statErr := os.Stat(oldKeyPath); statErr == nil {
if renameErr := os.Rename(oldKeyPath, keyPath); renameErr != nil {
logger.Error().Err(renameErr).Msg("failed to restore key backup after failed certificate renewal; host may be left without a usable key")
}
}
}()

Expand Down Expand Up @@ -358,3 +371,5 @@ func fetchCertWithRenewal(
// Fetch the certificate with the renewal extension in the CSR
return scepClient.FetchCert(ctx)
}

// <<< OPENFRAME(host-identity-renewal)
6 changes: 6 additions & 0 deletions frontend/interfaces/team.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,12 @@ export interface IRemoveTeamSecretFormData {

export const API_ALL_TEAMS_ID = undefined;
export const APP_CONTEXT_ALL_TEAMS_ID = -1;
// >>> OPENFRAME(FLEETMDM-001): rebranded "All teams" summary name to "All fleets"
export const APP_CONTEXT_ALL_TEAMS_SUMMARY: ITeamSummary = {
id: APP_CONTEXT_ALL_TEAMS_ID,
name: "All fleets",
} as const;
// <<< OPENFRAME(FLEETMDM-001)

export const API_NO_TEAM_ID = 0;
export const APP_CONTEXT_NO_TEAM_ID = 0;
Expand All @@ -155,7 +157,11 @@ export const getTeamDisplayName = (team: ITokenTeam) =>
? 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)
Comment on lines 157 to +167

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

2 changes: 2 additions & 0 deletions orbit/pkg/osquery/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,13 @@ func FleetFlags(osqueryVersion string, fleetURL *url.URL) []string {
"--carver_block_size=8000000",
}

// >>> OPENFRAME(gzip-flag): enable gzip transport for osquery >= 5.21.0 β€” openframe/docs/osquery-gzip.md
if v, err := semver.NewVersion(osqueryVersion); err == nil {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΄ orbit/pkg/osquery/flags.go adds gzip flag logic without OPENFRAME sentinel comments

Wrapped the gzip flag conditional in FleetFlags (orbit/pkg/osquery/flags.go) with // >>> OPENFRAME(gzip-flag): ... and // <<< OPENFRAME(gzip-flag) sentinel comments, exactly as suggested, so upstream syncs can identify and reconcile this fork-specific addition. No logic was changed.

πŸ€– Prompt for AI agents
In orbit/pkg/osquery/flags.go around line 38, review and complete this code-review fix: orbit/pkg/osquery/flags.go adds gzip flag logic without OPENFRAME sentinel comments.
What the draft fix changed: Wrapped the gzip flag conditional in `FleetFlags` (orbit/pkg/osquery/flags.go) with `// >>> OPENFRAME(gzip-flag): ...` and `// <<< OPENFRAME(gzip-flag)` sentinel comments, exactly as suggested, so upstream syncs can identify and reconcile this fork-specific addition. No logic was changed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 90 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

if !semver.New(v.Major(), v.Minor(), v.Patch(), "", "").LessThan(semver.New(5, 21, 0, "", "")) {
flags = append(flags, "--tls_accept_gzip=true")
}
}
// <<< OPENFRAME(gzip-flag)

return flags
}
2 changes: 2 additions & 0 deletions server/authz/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ func (e *Forbidden) LogFields() []interface{} {
}
}

// >>> 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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

// by the service.
//
Expand All @@ -86,3 +87,4 @@ type CheckMissing = platform_http.CheckMissing
// Deprecated: Use platform_http.CheckMissingWithResponse instead. This alias is
// kept for backward compatibility.
var CheckMissingWithResponse = platform_http.CheckMissingWithResponse
// <<< OPENFRAME(authz-platform-http-migration)
32 changes: 31 additions & 1 deletion server/datastore/mysql/campaigns.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,16 @@ func (ds *Datastore) SaveDistributedQueryCampaign(ctx context.Context, camp *fle
user_id = ?
WHERE id = ?
`
result, err := ds.writer(ctx).ExecContext(ctx, sqlStatement, camp.QueryID, camp.Status, camp.UserID, camp.ID)
args := []interface{}{camp.QueryID, camp.Status, camp.UserID, camp.ID}
// >>> OPENFRAME(mysql-multitenancy): same fence as DistributedQueryCampaign β€” an UPDATE by bare
// campaign.ID must not let one tenant overwrite another tenant's campaign row. No-op when unpinned.
// β€” openframe/docs/mysql-multitenancy-feature.md
if teamID, ok := fleet.OpenframeTeamID(ctx); ok {
sqlStatement += ` AND EXISTS (SELECT 1 FROM queries q WHERE q.id = distributed_query_campaigns.query_id AND q.team_id = ?)`
args = append(args, teamID)
}
// <<< OPENFRAME(mysql-multitenancy)
result, err := ds.writer(ctx).ExecContext(ctx, sqlStatement, args...)
if err != nil {
return ctxerr.Wrap(ctx, err, "updating distributed query campaign")
}
Comment on lines 88 to 103

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΄ SaveDistributedQueryCampaign and NewDistributedQueryCampaignTarget missing tenant fencing present in sibling methods

In SaveDistributedQueryCampaign, added the same OPENFRAME tenant-fencing sentinel used in DistributedQueryCampaign/DistributedQueryCampaignTargetIDs: when fleet.OpenframeTeamID(ctx) returns a pinned team, the UPDATE's WHERE clause now additionally requires EXISTS (SELECT 1 FROM queries q WHERE q.id = distributed_query_campaigns.query_id AND q.team_id = ?), so an UPDATE against another tenant's campaign ID now matches zero rows and returns notFound via the existing rowsAffected == 0 check, instead of silently succeeding. Also added the equivalent fence to NewDistributedQueryCampaignTarget: when tenant-pinned, it first runs a SELECT EXISTS(...) join from distributed_query_campaigns to queries verifying dqc.id = target.DistributedQueryCampaignID AND q.team_id = ?, and returns notFound if the campaign doesn't belong to the caller's tenant, before performing the INSERT. Risk: this changes behavior for tenant-pinned callers from "always succeeds" to "may return not-found" for cross-tenant IDs β€” this is the intended fix, but it does add one extra read query on the target-insert hot path; unpinned callers (no OpenframeTeamID) are unaffected (no-op), matching the sibling read methods' pattern exactly.

πŸ€– Prompt for AI agents
In server/datastore/mysql/campaigns.go around line 83, review and complete this code-review fix: SaveDistributedQueryCampaign and NewDistributedQueryCampaignTarget missing tenant fencing present in sibling methods.
What the draft fix changed: In `SaveDistributedQueryCampaign`, added the same OPENFRAME tenant-fencing sentinel used in `DistributedQueryCampaign`/`DistributedQueryCampaignTargetIDs`: when `fleet.OpenframeTeamID(ctx)` returns a pinned team, the UPDATE's WHERE clause now additionally requires `EXISTS (SELECT 1 FROM queries q WHERE q.id = distributed_query_campaigns.query_id AND q.team_id = ?)`, so an UPDATE against another tenant's campaign ID now matches zero rows and returns `notFound` via the existing `rowsAffected == 0` check, instead of silently succeeding. Also added the equivalent fence to `NewDistributedQueryCampaignTarget`: when tenant-pinned, it first runs a `SELECT EXISTS(...)` join from `distributed_query_campaigns` to `queries` verifying `dqc.id = target.DistributedQueryCampaignID AND q.team_id = ?`, and returns `notFound` if the campaign doesn't belong to the caller's tenant, before performing the INSERT. Risk: this changes behavior for tenant-pinned callers from "always succeeds" to "may return not-found" for cross-tenant IDs β€” this is the intended fix, but it does add one extra read query on the target-insert hot path; unpinned callers (no OpenframeTeamID) are unaffected (no-op), matching the sibling read methods' pattern exactly.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 75 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Expand Down Expand Up @@ -154,6 +163,27 @@ func (ds *Datastore) DistributedQueryCampaignTargetIDs(ctx context.Context, id u
}

func (ds *Datastore) NewDistributedQueryCampaignTarget(ctx context.Context, target *fleet.DistributedQueryCampaignTarget) (*fleet.DistributedQueryCampaignTarget, error) {
// >>> OPENFRAME(mysql-multitenancy): a target references its campaign by bare ID β€” verify the
// campaign belongs to the caller's tenant (via its query's team) before inserting a target row
// against it, matching the fence on DistributedQueryCampaignTargetIDs. No-op when unpinned.
// β€” openframe/docs/mysql-multitenancy-feature.md
if teamID, ok := fleet.OpenframeTeamID(ctx); ok {
var exists bool
checkStmt := `
SELECT EXISTS (
SELECT 1 FROM distributed_query_campaigns dqc
JOIN queries q ON q.id = dqc.query_id
WHERE dqc.id = ? AND q.team_id = ?
)
`
if err := sqlx.GetContext(ctx, ds.reader(ctx), &exists, checkStmt, target.DistributedQueryCampaignID, teamID); err != nil {
return nil, ctxerr.Wrap(ctx, err, "checking distributed query campaign tenant")
}
if !exists {
return nil, notFound("DistributedQueryCampaign").WithID(target.DistributedQueryCampaignID)
}
}
// <<< OPENFRAME(mysql-multitenancy)
sqlStatement := `
INSERT into distributed_query_campaign_targets (
type,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// >>> OPENFRAME(host-identity-team-scope): scopes the upstream hosts table's
// osquery_host_id uniqueness constraint per-team instead of globally, so the
// same device can enroll into more than one tenant team under shared-database
// multitenancy β€” openframe/docs/upstream-sync-conflict-resolution.md
package openframe

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΄ OpenFrame-specific migration file lacks OPENFRAME sentinel comment wrapping

Added the required // >>> OPENFRAME(host-identity-team-scope): ... β€” openframe/docs/upstream-sync-conflict-resolution.md sentinel comment above package openframe at the top of the file, and the matching // <<< OPENFRAME(host-identity-team-scope) closing sentinel at the end of the file, wrapping the entire migration so an upstream-sync tool can detect this fork-specific divergence on the shared hosts table.

πŸ€– Prompt for AI agents
In server/datastore/mysql/migrations/openframe/20260626000001_ScopeHostIdentityUniqueToTeam.go around line 1, review and complete this code-review fix: OpenFrame-specific migration file lacks OPENFRAME sentinel comment wrapping.
What the draft fix changed: Added the required `// >>> OPENFRAME(host-identity-team-scope): ... β€” openframe/docs/upstream-sync-conflict-resolution.md` sentinel comment above `package openframe` at the top of the file, and the matching `// <<< OPENFRAME(host-identity-team-scope)` closing sentinel at the end of the file, wrapping the entire migration so an upstream-sync tool can detect this fork-specific divergence on the shared `hosts` table.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 85 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer


import (
Expand Down Expand Up @@ -95,6 +99,51 @@ func Up_20260626000001(tx *sql.Tx) error {
return nil
}

// Down_20260626000001 restores the pre-migration schema: it re-creates the
// original global UNIQUE(osquery_host_id) index and drops the per-team unique
// index and its supporting generated column. This is only safe to run if no
// rows currently violate a global-unique(osquery_host_id) constraint (i.e., no
// device has actually been enrolled into more than one team since Up ran); if
// such rows exist, re-adding idx_osquery_host_id will fail with a duplicate-key
// error, which is the correct, safe failure mode for an unsound rollback.
func Down_20260626000001(tx *sql.Tx) error {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Migration comment claims idempotency but Down migration is a no-op that cannot restore the original global-unique constraint

Replaced the no-op Down_20260626000001 with a real rollback that re-adds the original global UNIQUE(osquery_host_id) index (idx_osquery_host_id), drops the new per-team unique index idx_hosts_team_osquery_host_id, and drops the openframe_team_key generated column β€” using the same indexExists/columnExists guards as Up for idempotency. This restores schema symmetry for fleetctl/migration-tool down runs; risk: if any host rows were actually enrolled into more than one team while the per-team unique was active, re-adding the global unique will fail with a duplicate-key error (a correct but disruptive failure mode) rather than silently succeeding β€” a complete fix would need a data-reconciliation strategy (e.g., merge or reject duplicates) before restoring the stricter constraint, which is out of scope for a minimal fix.

πŸ€– Prompt for AI agents
In server/datastore/mysql/migrations/openframe/20260626000001_ScopeHostIdentityUniqueToTeam.go around line 98, review and complete this code-review fix: Migration comment claims idempotency but Down migration is a no-op that cannot restore the original global-unique constraint.
What the draft fix changed: Replaced the no-op `Down_20260626000001` with a real rollback that re-adds the original global `UNIQUE(osquery_host_id)` index (`idx_osquery_host_id`), drops the new per-team unique index `idx_hosts_team_osquery_host_id`, and drops the `openframe_team_key` generated column β€” using the same `indexExists`/`columnExists` guards as `Up` for idempotency. This restores schema symmetry for `fleetctl`/migration-tool down runs; risk: if any host rows were actually enrolled into more than one team while the per-team unique was active, re-adding the global unique will fail with a duplicate-key error (a correct but disruptive failure mode) rather than silently succeeding β€” a complete fix would need a data-reconciliation strategy (e.g., merge or reject duplicates) before restoring the stricter constraint, which is out of scope for a minimal fix.
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 (
table = "hosts"
oldIndex = "idx_osquery_host_id"
newIndex = "idx_hosts_team_osquery_host_id"
genColumn = "openframe_team_key"
)

hasOld, err := indexExists(tx, table, oldIndex)
if err != nil {
return fmt.Errorf("checking %s index: %w", oldIndex, err)
}
if !hasOld {
if _, err := tx.Exec("ALTER TABLE hosts ADD UNIQUE KEY idx_osquery_host_id (osquery_host_id)"); err != nil {
return fmt.Errorf("adding %s unique index: %w", oldIndex, err)
}
}

hasNew, err := indexExists(tx, table, newIndex)
if err != nil {
return fmt.Errorf("checking %s index: %w", newIndex, err)
}
if hasNew {
if _, err := tx.Exec("ALTER TABLE hosts DROP INDEX idx_hosts_team_osquery_host_id"); err != nil {
return fmt.Errorf("dropping %s index: %w", newIndex, err)
}
}

hasCol, err := columnExists(tx, table, genColumn)
if err != nil {
return fmt.Errorf("checking %s column: %w", genColumn, err)
}
if hasCol {
if _, err := tx.Exec("ALTER TABLE hosts DROP COLUMN openframe_team_key"); err != nil {
return fmt.Errorf("dropping %s column: %w", genColumn, err)
}
}
return nil
}

// <<< OPENFRAME(host-identity-team-scope)
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ func Up_20260818000001(tx *sql.Tx) error {
return nil
}

// >>> OPENFRAME(policies-managed-column): ALTERs upstream `policies` table β€” openframe/docs/managed-policies.md
if _, err := tx.Exec("ALTER TABLE policies ADD COLUMN openframe_managed TINYINT(1) NOT NULL DEFAULT 0"); err != nil {
return fmt.Errorf("adding %s.%s column: %w", table, column, err)
}
// <<< OPENFRAME(policies-managed-column)
return nil
}

Comment on lines 36 to 46

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΄ OpenFrame migration files touching upstream tables lack OPENFRAME sentinel comments

Wrapped the tx.Exec("ALTER TABLE policies ADD COLUMN openframe_managed ...") statement in Up_20260818000001 with // >>> OPENFRAME(policies-managed-column): ... β€” openframe/docs/managed-policies.md and // <<< OPENFRAME(policies-managed-column) sentinel comments, satisfying the required sentinel format around the actual SQL statement rather than only prose above the function. No other statements needed wrapping since this file only ever touches the policies table.

πŸ€– Prompt for AI agents
In server/datastore/mysql/migrations/openframe/20260818000001_AddPoliciesOpenframeManagedColumn.go around line 25, review and complete this code-review fix: OpenFrame migration files touching upstream tables lack OPENFRAME sentinel comments.
What the draft fix changed: Wrapped the `tx.Exec("ALTER TABLE policies ADD COLUMN openframe_managed ...")` statement in `Up_20260818000001` with `// >>> OPENFRAME(policies-managed-column): ... β€” openframe/docs/managed-policies.md` and `// <<< OPENFRAME(policies-managed-column)` sentinel comments, satisfying the required sentinel format around the actual SQL statement rather than only prose above the function. No other statements needed wrapping since this file only ever touches the `policies` table.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 85 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Expand Down
Original file line number Diff line number Diff line change
@@ -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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΄ 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


import (
Expand All @@ -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 {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

return err
return fmt.Errorf("querying software_titles for cisco bundle ids: %w", err)
}
defer titleRows.Close()

bundleIdToTitleId := map[string]string{}
for titleRows.Next() {
var id, bundleIdentifier string
if err := titleRows.Scan(&id, &bundleIdentifier); err != nil {
return err
return fmt.Errorf("scanning software_titles row: %w", err)
}
bundleIdToTitleId[bundleIdentifier] = id
}
if err := titleRows.Err(); err != nil {
return err
return fmt.Errorf("iterating software_titles rows: %w", err)
}

if len(bundleIdToTitleId) == 0 {
Expand All @@ -51,12 +52,12 @@ func Up_20251031154558(tx *sql.Tx) error {
('Cisco Secure Client', 'apps', 'com.cisco.secureclient.gui')
`)
if err != nil {
return err
return fmt.Errorf("inserting correct cisco secure client software title: %w", err)
}

lastInsertId, err := res.LastInsertId()
if err != nil {
return err
return fmt.Errorf("getting last insert id for cisco secure client software title: %w", err)
}
bundleIdToTitleId["com.cisco.secureclient.gui"] = fmt.Sprintf("%d", lastInsertId)
}
Expand All @@ -66,23 +67,22 @@ func Up_20251031154558(tx *sql.Tx) error {
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
Comment on lines 67 to 88

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand All @@ -92,7 +92,7 @@ func Up_20251031154558(tx *sql.Tx) error {
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)
}
}

Comment on lines 92 to 98

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand All @@ -102,11 +102,12 @@ func Up_20251031154558(tx *sql.Tx) error {
DELETE FROM software_titles
WHERE id = ?
`, incorrectTitleId); err != nil {
return err
return fmt.Errorf("deleting incorrect cisco software title id %s: %w", incorrectTitleId, err)
}
}
return nil
}
// <<< OPENFRAME(cisco-secure-client-bundle-id)

func Down_20251031154558(tx *sql.Tx) error {
return nil
Expand Down
Loading