Skip to content

fix(#2433): restore data consistency guard in EnsureOrgInMint#2436

Open
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/2433-restore-data-consistency-guard
Open

fix(#2433): restore data consistency guard in EnsureOrgInMint#2436
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/2433-restore-data-consistency-guard

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

PR #2331 removed the defense-in-depth cross-check that prevented silent clobbering of ALLOWED_ORGS on stale reads because it relied on org-scoped ROLE_APP_IDS keys which no longer exist in the role-only model. No replacement was added.

Restore the guard adapted for the role-only model: if ALLOWED_ORGS is empty but mintcore.RoleOnlyAppIDs() finds role-only entries in ROLE_APP_IDS, the mint has been bootstrapped and empty ALLOWED_ORGS indicates env var data loss. Return an error instead of silently writing only the new org (which would unenroll all existing orgs).

First-enrollment (both empty) and legacy-only keys (no role-only entries) proceed normally.

Updated TestEnsureOrgInMint_DerivesAllowedRolesWhenEmpty to set ALLOWED_ORGS to a non-empty value, since the test's purpose is verifying ALLOWED_ROLES derivation, not the empty-ALLOWED_ORGS path which is now correctly guarded.

Note: pre-commit could not run in sandbox (shellcheck download blocked by network policy). Post-script runs it authoritatively.


Closes #2433

Post-script verification

  • Branch is not main/master (agent/2433-restore-data-consistency-guard)
  • Secret scan passed (gitleaks — 8271187d48c6a25688945cbcdec4c8a7dabf800c..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

PR #2331 removed the defense-in-depth cross-check that prevented
silent clobbering of ALLOWED_ORGS on stale reads because it relied
on org-scoped ROLE_APP_IDS keys which no longer exist in the
role-only model. No replacement was added.

Restore the guard adapted for the role-only model: if ALLOWED_ORGS
is empty but mintcore.RoleOnlyAppIDs() finds role-only entries in
ROLE_APP_IDS, the mint has been bootstrapped and empty ALLOWED_ORGS
indicates env var data loss. Return an error instead of silently
writing only the new org (which would unenroll all existing orgs).

First-enrollment (both empty) and legacy-only keys (no role-only
entries) proceed normally.

Updated TestEnsureOrgInMint_DerivesAllowedRolesWhenEmpty to set
ALLOWED_ORGS to a non-empty value, since the test's purpose is
verifying ALLOWED_ROLES derivation, not the empty-ALLOWED_ORGS
path which is now correctly guarded.

Note: pre-commit could not run in sandbox (shellcheck download
blocked by network policy). Post-script runs it authoritatively.

Closes #2433
@github-actions

Copy link
Copy Markdown

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

@github-actions

Copy link
Copy Markdown

Site preview

Preview: https://59e94622-site.fullsend-ai.workers.dev

Commit: 9e3732d51e5b2208194572b746dbf56e7a142b72

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 18, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:47 PM UTC · Completed 4:58 PM UTC
Commit: 9e3732d · View workflow run →

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

  • [fail-open] internal/dispatch/gcf/provisioner.go:420json.Unmarshal error is silently discarded (_ = json.Unmarshal(...)). If ROLE_APP_IDS contains malformed JSON, roleAppIDMap stays nil, RoleOnlyAppIDs returns nil, and the guard is bypassed entirely. This is a fail-open on parse failure in a guard designed to detect data corruption — the exact scenario (garbled env vars from a stale read) where the guard should fire. See also: [error handling gap] and [architectural-coherence] findings at this location.
    Remediation: Check the error from json.Unmarshal. If ROLE_APP_IDS is non-empty but fails to parse, return an error (fail closed): if err := json.Unmarshal([]byte(raw), &roleAppIDMap); err != nil { return fmt.Errorf("data inconsistency: ROLE_APP_IDS is present but not valid JSON: %w", err) }

Low

  • [test-adequacy] internal/dispatch/gcf/provisioner_test.go — No test covers the case where ALLOWED_ORGS is empty and ROLE_APP_IDS contains non-empty but malformed JSON (e.g., {invalid). A test documenting the expected behavior for this edge case would be valuable, particularly given the unmarshal-error finding above.

  • [error-message-consistency] internal/dispatch/gcf/provisioner.go:417 — Error message uses multi-line string concatenation with + operator, inconsistent with all other fmt.Errorf calls in this file which use single-line format strings. Consider consolidating into a single-line format string.

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the review comment for full details.

// data loss (e.g., stale read from a diverged revision), not a first
// enrollment. Abort to prevent silently unenrolling all existing orgs.
if allowedOrgs == "" {
var roleAppIDMap map[string]string

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium] fail-open

json.Unmarshal error is silently discarded. If ROLE_APP_IDS contains malformed JSON, roleAppIDMap stays nil, RoleOnlyAppIDs returns nil, and the guard is bypassed. This is a fail-open on parse failure in a guard designed to detect data corruption.

Suggested fix: Check the error from json.Unmarshal. If ROLE_APP_IDS is non-empty but fails to parse, return an error (fail closed).

// Defense-in-depth: if ALLOWED_ORGS is empty but ROLE_APP_IDS has
// role-only entries, the mint has been bootstrapped and should have
// enrolled orgs. Empty ALLOWED_ORGS in this state indicates env var
// data loss (e.g., stale read from a diverged revision), not a first

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] error-message-consistency

Error message uses multi-line string concatenation with '+' operator, inconsistent with other fmt.Errorf calls in this file which use single-line format strings.

Suggested fix: Consolidate into a single-line format string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EnsureOrgInMint missing data consistency guard after role-only ROLE_APP_IDS migration

0 participants