From 9bdd34f73dc514d66c8710c52bbc4f05524e5403 Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer B (MiniMax)" Date: Tue, 23 Jun 2026 09:32:20 +0000 Subject: [PATCH 1/3] =?UTF-8?q?fix(sibling=20of=20core#3162):=20fail-CLOSE?= =?UTF-8?q?D=20on=20readStoredModelSecret=20decrypt/read=20error=20?= =?UTF-8?q?=E2=80=94=20rebased=20onto=20current=20main?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-applies the sibling fix on top of current main (c5316db2) which now includes the merge of #3165 (the provider fail-closed sibling). The rebase produced a non-conflicting change set because the two fail-closed halves touch different functions in the same file but the relevant hunk boundaries do not overlap: - readStoredModelSecret → (string, error), errors.Is(err, sql.ErrNoRows) → ("", nil) - readStoredProviderSecret → unchanged (already on main) - ensureConciergeModel → fails closed on ('', err) - ensureConciergeProvider → unchanged (already on main) Same scope as before: the function changes + a new test function (TestEnsureConciergeModel_FailsClosedOnReadError). No other files touched. PR#3166 (the original branch tip ca5f543d) is now superseded by this rebased tip. The PR description is unchanged; the SHAs in the title/branch will be updated by force-push. Tests ----- * TestEnsureConciergeModel_FailsClosedOnReadError - decrypt_error_on_existing_MODEL_row_fails_closed_(does_NOT_seed_default) - db_scan_error_(non-ErrNoRows)_on_MODEL_fails_closed - sql.ErrNoRows_(genuine_unset_MODEL)_proceeds_to_seed_(no_regression) - existing_stored_MODEL_is_respected_on_successful_read_(no_regression) * TestEnsureConciergeProvider_FailsClosedOnReadError (from main / PR#3165) - all four sub-tests continue to pass on the rebased tree Full handler suite passes (39.4s). go build + go vet clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../internal/handlers/platform_agent.go | 45 ++++++--- .../internal/handlers/platform_agent_test.go | 91 +++++++++++++++++++ 2 files changed, 123 insertions(+), 13 deletions(-) diff --git a/workspace-server/internal/handlers/platform_agent.go b/workspace-server/internal/handlers/platform_agent.go index 0cb6d249..386cf266 100644 --- a/workspace-server/internal/handlers/platform_agent.go +++ b/workspace-server/internal/handlers/platform_agent.go @@ -360,7 +360,10 @@ func (h *WorkspaceHandler) ensureConciergeModel(ctx context.Context, workspaceID // boot). Re-asserting on EVERY provision would silently revert the customer's // pick — exactly the platform-overriding-customer violation the SSOT // directive forbids. - if existing := readStoredModelSecret(ctx, workspaceID); existing != "" { + if existing, readErr := readStoredModelSecret(ctx, workspaceID); readErr != nil { + log.Printf("Provisioner: concierge %s MODEL read failed (failing closed, NOT seeding): %v", workspaceID, readErr) + return + } else if existing != "" { return // explicit model already set; never overwrite the customer's choice } @@ -392,23 +395,39 @@ func (h *WorkspaceHandler) ensureConciergeModel(ctx context.Context, workspaceID } } -// readStoredModelSecret returns the decrypted MODEL workspace_secret, or "" when -// none is stored (or on any read/decrypt error — treated as "unset" so a -// transient miss re-seeds rather than wedges). Used by ensureConciergeModel to -// decide seed-vs-respect. -func readStoredModelSecret(ctx context.Context, workspaceID string) string { +// readStoredModelSecret returns the decrypted MODEL workspace_secret. +// The second return value distinguishes the three observed states so the caller +// can fail closed rather than fail open on a transient error: +// +// - (value, nil): a secret is stored and decrypted successfully → caller +// respects the existing model and skips the seed. +// - ("", nil): no row exists for this workspace/key → caller may re-seed +// safely (this is the fresh-boot / cleared-secret case). +// - ("", error): the row exists (or the read otherwise succeeded) but the +// decryption failed → caller MUST NOT treat this as "unset" and MUST NOT +// fall back to seeding the platform default. Returning "" without the +// error used to silently overwrite the customer's model pick if the +// secret store later recovered: the seed path would re-fire on the next +// provision and the customer's choice would be lost without any error +// surfaced. Sibling of core#3162 (which closed the same fail-open shape +// on readStoredProviderSecret). +func readStoredModelSecret(ctx context.Context, workspaceID string) (string, error) { var stored []byte var version int - if err := db.DB.QueryRowContext(ctx, + err := db.DB.QueryRowContext(ctx, `SELECT encrypted_value, encryption_version FROM workspace_secrets WHERE workspace_id = $1 AND key = 'MODEL'`, - workspaceID).Scan(&stored, &version); err != nil { - return "" - } - dec, err := crypto.DecryptVersioned(stored, version) + workspaceID).Scan(&stored, &version) if err != nil { - return "" + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + return "", fmt.Errorf("readStoredModelSecret scan: %w", err) } - return string(dec) + dec, derr := crypto.DecryptVersioned(stored, version) + if derr != nil { + return "", fmt.Errorf("readStoredModelSecret decrypt: %w", derr) + } + return string(dec), nil } // conciergeProvider is the provider-registry NAME the concierge's declared diff --git a/workspace-server/internal/handlers/platform_agent_test.go b/workspace-server/internal/handlers/platform_agent_test.go index f0379bde..55e1cd81 100644 --- a/workspace-server/internal/handlers/platform_agent_test.go +++ b/workspace-server/internal/handlers/platform_agent_test.go @@ -1140,3 +1140,94 @@ func TestEnsureConciergeProvider_FailsClosedOnReadError(t *testing.T) { } }) } + +// TestEnsureConciergeModel_FailsClosedOnReadError is the sibling regression +// gate for the MODEL half of the fail-open shape (paired with +// TestEnsureConciergeProvider_FailsClosedOnReadError). Same shape as core#3162: +// a transient decrypt/read error on an existing MODEL row used to be collapsed +// into "" and treated as "unset", which would silently overwrite the customer's +// model pick if the secret store later recovered (the seed path would re-fire +// on the next provision and the customer's choice would be lost without any +// error surfaced). The fix returns the error; ensureConciergeModel MUST fail +// closed (return without seeding) so the next provision re-tries rather than +// losing the customer's pick. +func TestEnsureConciergeModel_FailsClosedOnReadError(t *testing.T) { + h := &WorkspaceHandler{} + const modelSelQuery = `SELECT encrypted_value, encryption_version FROM workspace_secrets WHERE workspace_id = \$1 AND key = 'MODEL'` + const secretInsert = `INSERT INTO workspace_secrets` + + t.Run("decrypt error on existing MODEL row fails closed (does NOT seed default)", func(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(modelSelQuery).WithArgs("ws-model-decrypt-fail"). + WillReturnRows(sqlmock.NewRows([]string{"encrypted_value", "encryption_version"}). + AddRow([]byte("corrupt-ciphertext-that-cannot-be-decrypted"), 0)) + // NO ExpectExec: the fail-closed path MUST NOT persist MODEL. + + env := map[string]string{} + h.ensureConciergeModel(context.Background(), "ws-model-decrypt-fail", env) + + if _, seeded := env["MODEL"]; seeded { + t.Errorf("transient decrypt error seeded MODEL=%q — would silently overwrite the customer's pick", env["MODEL"]) + } + if _, seeded := env["MOLECULE_MODEL"]; seeded { + t.Errorf("transient decrypt error seeded MOLECULE_MODEL=%q", env["MOLECULE_MODEL"]) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations (a MODEL INSERT means the fail-closed path leaked): %v", err) + } + }) + + t.Run("db scan error (non-ErrNoRows) on MODEL fails closed", func(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(modelSelQuery).WithArgs("ws-model-scan-fail"). + WillReturnError(fmt.Errorf("connection refused")) + // NO ExpectExec: fail closed. + + env := map[string]string{} + h.ensureConciergeModel(context.Background(), "ws-model-scan-fail", env) + + if _, seeded := env["MODEL"]; seeded { + t.Errorf("DB scan error seeded MODEL=%q", env["MODEL"]) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } + }) + + t.Run("sql.ErrNoRows (genuine unset MODEL) proceeds to seed (no regression)", func(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(modelSelQuery).WithArgs("ws-model-fresh-unset"). + WillReturnError(sql.ErrNoRows) + mock.ExpectExec(secretInsert). + WithArgs("ws-model-fresh-unset", sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + + env := map[string]string{} + h.ensureConciergeModel(context.Background(), "ws-model-fresh-unset", env) + + if env["MODEL"] != conciergeDeclaredModel { + t.Errorf("genuine unset did not seed MODEL=%q; got %q (env=%v) — regression on the seed path", conciergeDeclaredModel, env["MODEL"], env) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations (the seed path did not persist): %v", err) + } + }) + + t.Run("existing stored MODEL is respected on successful read (no regression)", func(t *testing.T) { + mock := setupTestDB(t) + mock.ExpectQuery(modelSelQuery).WithArgs("ws-model-existing"). + WillReturnRows(sqlmock.NewRows([]string{"encrypted_value", "encryption_version"}). + AddRow([]byte("anthropic:claude-opus-4-8"), 0)) + // NO ExpectExec: existing pick wins → no seed. + + env := map[string]string{} + h.ensureConciergeModel(context.Background(), "ws-model-existing", env) + + if _, seeded := env["MODEL"]; seeded { + t.Errorf("existing MODEL pick wrongly overwrote with default (env=%v)", env) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } + }) +} -- 2.52.0 From fa39418da33b47c540b5e0e440bb82bc76ce363b Mon Sep 17 00:00:00 2001 From: Hongming Home PC Agent Date: Tue, 23 Jun 2026 04:04:46 -0700 Subject: [PATCH 2/3] chore: re-trigger CI (Secret-scan stuck on #3166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No code change — empty commit to fire a clean synchronize and re-run the stuck Secret-scan / required checks on a stable head. Approved diff unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) -- 2.52.0 From 830934233dc6f1e6779481164f3c79d05b46a610 Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer B (MiniMax)" Date: Tue, 23 Jun 2026 12:44:01 +0000 Subject: [PATCH 3/3] chore: re-trigger CI (Secret-scan stuck) -- 2.52.0