fix(core): idempotency pre-check без схемы — единый источник (live)

Код-ревью (раунд 5), п.1/6: pre-check брал схему из GET /instanceOperations/default/{opId},
а payload строился по живой схеме ?fields=cfsParams — два источника. default может
расходиться с живой => риск ложного пропуска modify.

Решение: pre-check вообще не запрашивает схему.
- modifierDesiredEqualsLive(ctx, uid, desired): сравнение по live-кодам
  (live[lower(code)]), единственный источник — state.params.
- Значения: похожи на JSON ({/[) — смысловое сравнение; иначе скалярное с
  нормализацией (null/"" -> ""; true/false без учёта регистра) — закрывает и
  регистр bool.
- Fail-safe сохранён: пусто/нет кода/ошибка live => modify выполняется.
- Удалены: fetchOperationSchemaByID, modifierValuesEqual, lookupLiveParam больше
  не участвует в pre-check (остаётся для досылки).
- Тесты переписаны (RawValuesEqual_Scalars/JSON, DesiredEqualsLive: 4 кейса).

Документация: TOOLS/ARCHITECTURE.md -> новый раздел «Modifier Idempotency»
(5 правил контракта); HISTORY — журнал раунда 5.

Проверено: go build ./... OK; go test ./internal/... -short PASS.

HISTORY/2026-09-30_core_and_modifiers_remediation.md

@@ -106,4 +106,39 @@ Required с пустым `data_type` (все получают `""`; угадыв
 
 **Осталось:** замер владельцем (`terraform plan` ×2 на `DEV_STAND/FullPipe`); B8 (`modifiers.yaml`).
 
+---
+
+## Раунд 5 — код-ревью (Opus) и его фиксы
+
+Промпт: `NOTES/20_prompts/prompt_for_opus_code_review_round5.md`. Opus нашёл 6 пунктов; критичные — 2.
+
+| # | Находка | Решение |
+|---|---|---|
+| 1 | **Регрессия B6:** pre-check стоял ПОСЛЕ `POST /instanceOperations` → при совпадении оставался «черновик» операции (pending). | Исправлено: pre-check до создания операции. |
+| 2 | Partial state: `resp.State.Set(&data)` мог записать unknown/computed → «invalid new value … unknown». | Исправлено: пишется только `id` (`SetAttribute`). |
+| 3 | 401 ретраится, но токен между попытками не обновляется. | Принято как есть; пояснено в комментарии `http.go`. |
+| 4 | `ImportState` vs `Read` — конфликта нет. | ок. |
+| 5 | `modifierDesiredEqualsCurrent` — мёртвый код (только тесты). | Удалено; тесты переведены на живые функции. |
+| 6 | Сравнение по `default`-схеме рискует ложным пропуском (ревизия п.1). | Исправлено: pre-check **без схемы** — только live-коды. |
+
+**Итоговый контракт pre-check (см. `TOOLS/ARCHITECTURE.md` → «Modifier Idempotency»):**
+1. pre-check выполняется **до** `POST /instanceOperations`;
+2. единственный источник — live (`state.params`), схема операции не запрашивается
+   (`default/{opId}` может расходиться с живой; живая доступна только после создания);
+3. поиск значения — по самому коду (`live[lower(code)]`);
+4. fail-safe: пусто/нет кода/ошибка live → modify выполняется;
+5. сравнение: похоже на JSON — смысловое (порядок ключей не значим), иначе — скалярное
+   с нормализацией (`null`/`""` → `""`; `true`/`false` без учёта регистра).
+
+Файлы: `core/modifier_compare.go` (новые `modifierRawValuesEqual`, `looksLikeJSON`,
+`normalizeRawScalar`, `modifierDesiredEqualsLive` без схемы), `core/operation_run_bycode.go`,
+`core/operation_cfs.go` (удалена `fetchOperationSchemaByID`), `core/client_test.go`,
+`core/modifier_compare_test.go`, `TOOLS/ARCHITECTURE.md`.
+
+Тесты: `TestRunInstanceOperationByIdempotent_SkipsWithoutCreatingOperation` (POST операции не
+вызывается при совпадении), `TestModifierRawValuesEqual_Scalars/JSON`,
+`TestModifierDesiredEqualsLive` (совпало / отличается / нет кода / live недоступен).
+Проверено: `go build ./...` OK, `go test ./internal/... -short` PASS.
+
+
 

TOOLS/ARCHITECTURE.md

@@ -134,6 +134,25 @@ Each operation has a kind:
   provider code nor the captured HAR contain `DELETE /instanceOperations/{uid}`.
 - Transient 401 is retried for GET (see `isRetryable`).
 
+### Modifier Idempotency (decided 2026-09-30)
+
+Modifiers may opt into an idempotency pre-check (`RunInstanceOperationUniversalByIdempotent`,
+used by `nubes_vc_nsxt_snat`). Rules:
+
+- The pre-check runs **before** `POST /instanceOperations`. Otherwise a skipped modify
+  leaves a created-but-never-run operation ("draft", pending) and the next call waits
+  for idle until timeout.
+- The **only** comparison source is the live instance state (`state.params` via
+  `instanceLiveParams`). The operation schema (`/instanceOperations/default/{opId}`,
+  `?fields=cfsParams`) is **deliberately not used**: before the operation exists only the
+  `default` schema is available, and it may diverge from the live one — comparing through
+  it risks a false skip. (Using the live schema would require creating the operation
+  first — which is exactly the regression above.)
+- Values are matched by the code itself (`live[lower(code)]`).
+- Fail-safe: empty live, code absent, or live read error → the modify runs (never skipped).
+- JSON-looking values (`{`/`[`) are compared semantically (key order ignored); scalars are
+  normalized (`null`/`""` → empty; `true`/`false` case-insensitive).
+
 ### Generated Code Resilience
 
 - **Partial state при ошибке create** (шаблон `instance.go`): если инстанс успел создаться

provider/internal/core/client_test.go

@@ -88,9 +88,6 @@ func TestRunInstanceOperationByIdempotent_SkipsWithoutCreatingOperation(t *testi
 		case r.Method == http.MethodGet && r.URL.Path == "/instances/inst-1":
 			w.Header().Set("Content-Type", "application/json")
 			_, _ = w.Write([]byte(`{"instance":{"instanceUid":"inst-1","serviceId":22,"explainedStatus":"running","isDeleted":false,"operationIsPending":false,"operationIsInProgress":false,"availableOperations":[{"svcOperationId":207,"operation":"modify"}],"state":{"params":{"ipSpaceName":"internet-ipv4-v1"}}}}`))
-		case r.Method == http.MethodGet && r.URL.Path == "/instanceOperations/default/207":
-			w.Header().Set("Content-Type", "application/json")
-			_, _ = w.Write([]byte(`{"svcOperation":{"cfsParams":[{"svcOperationCfsParamId":372,"code":"ipSpaceName","dataType":"string"}]}}`))
 		case r.Method == http.MethodPost && r.URL.Path == "/instanceOperations":
 			opPosts.Add(1)
 			w.WriteHeader(http.StatusCreated)

provider/internal/core/modifier_compare.go

@@ -7,30 +7,46 @@ import (
 	"terraform-provider-nubes/internal/core/jsonutil"
 )
 
-// modifierValuesEqual сравнивает одно значение с учётом типа параметра:
+// modifierRawValuesEqual сравнивает два значения БЕЗ схемы операции (dataType может быть
-//   - array*/map*/json — смысловое JSON-сравнение (порядок ключей не значим);
+// недоступен до создания операции, или отличаться между default- и live-схемой):
-//   - скаляры — нормализация через normalizeUniversalValueV6 и сравнение строками.
+//   - оба выглядят как JSON (`{`/`[`) — смысловое JSON-сравнение (порядок ключей не значим);
-func modifierValuesEqual(wanted, current string, param universalCfsParam) bool {
+//   - иначе — строковое сравнение с нормализацией null/""/true/false (без учёта регистра).
-	dataType := strings.ToLower(strings.TrimSpace(param.DataType))
+func modifierRawValuesEqual(wanted, current string) bool {
-	if strings.HasPrefix(dataType, "array") ||
+	w := strings.TrimSpace(wanted)
-		strings.HasPrefix(dataType, "map") ||
+	c := strings.TrimSpace(current)
-		strings.Contains(dataType, "json") {
+	if looksLikeJSON(w) && looksLikeJSON(c) {
-		return jsonutil.JSONStringsEquivalent(wanted, current)
+		return jsonutil.JSONStringsEquivalent(w, c)
 	}
-	return normalizeUniversalValueV6(wanted, param) == normalizeUniversalValueV6(current, param)
+	return normalizeRawScalar(w) == normalizeRawScalar(c)
+}
+
+func looksLikeJSON(v string) bool {
+	return strings.HasPrefix(v, "{") || strings.HasPrefix(v, "[")
+}
+
+// normalizeRawScalar приводит скаляр к каноническому виду без схемы:
+// null/"" -> "", true/false — к lower-case, остальное — trim.
+func normalizeRawScalar(v string) string {
+	switch {
+	case strings.EqualFold(v, "null"), v == `""`:
+		return ""
+	case strings.EqualFold(v, "true"):
+		return "true"
+	case strings.EqualFold(v, "false"):
+		return "false"
+	}
+	return v
 }
 
 // modifierDesiredEqualsLive — idempotency-pre-check по ЖИВЫМ параметрам инстанса
-// (state.params), а не по paramValue формы операции.
+// (state.params). Источник — ЕДИНСТВЕННЫЙ: live. Схема операции НЕ запрашивается (см.
+// ARCHITECTURE.md, «Modifier idempotency»): до создания операции доступна только
+// default-схема, а она может расходиться с живой — сравнение по ней рискует ложным
+// пропуском. Поиск значения — по самому коду (live keyed by lower(code)).
 //
-// Зачем: paramValue из cfsParams — это дефолт ФОРМЫ операции, а не состояние
+// Fail-safe: live пуст, код не найден или ошибка чтения live → (false, …): modify
-// инстанса (см. instanceLiveParams, HAR/edge_.har). Пропуск modify на основе
-// формы может быть ложным. Здесь источник — live.
-//
-// (true, nil) возвращается только если ВСЕ desired-поля найдены в live и совпали.
-// Любая неопределённость (пусто, поле не найдено, ошибка) → (false, …): modify
 // лучше выполнить лишний раз, чем пропустить нужный.
-func (c *UniversalClient) modifierDesiredEqualsLive(ctx context.Context, instanceUid string, desired map[string]string, cfsParams []universalCfsParam) (bool, error) {
+func (c *UniversalClient) modifierDesiredEqualsLive(ctx context.Context, instanceUid string, desired map[string]string) (bool, error) {
 	if len(desired) == 0 {
 		return false, nil
 	}
@@ -41,17 +57,12 @@ func (c *UniversalClient) modifierDesiredEqualsLive(ctx context.Context, instanc
 	if len(live) == 0 {
 		return false, nil
 	}
-	codeToParam := modifierCodeMap(cfsParams)
 	for code, wanted := range desired {
-		param, ok := codeToParam[strings.ToLower(strings.TrimSpace(code))]
+		current, ok := live[strings.ToLower(strings.TrimSpace(code))]
 		if !ok {
 			return false, nil
 		}
-		current, ok := lookupLiveParam(live, param)
+		if !modifierRawValuesEqual(wanted, current) {
-		if !ok {
-			return false, nil
-		}
-		if !modifierValuesEqual(wanted, current, param) {
 			return false, nil
 		}
 	}

provider/internal/core/modifier_compare_test.go

@@ -6,53 +6,46 @@ import (
 	"testing"
 )
 
-func TestModifierValuesEqual_Scalars(t *testing.T) {
+func TestModifierRawValuesEqual_Scalars(t *testing.T) {
 	tests := []struct {
 		name            string
 		wanted, current string
-		param           universalCfsParam
 		want            bool
 	}{
-		{"bool совпал", "false", "false", universalCfsParam{DataType: "boolean"}, true},
+		{"bool совпал", "false", "false", true},
-		{"bool расхождение", "true", "false", universalCfsParam{DataType: "boolean"}, false},
+		{"bool регистр", "True", "true", true},
-		{"int с пробелом", " 3 ", "3", universalCfsParam{DataType: "integer > 0"}, true},
+		{"bool расхождение", "true", "false", false},
-		{"int расхождение", "1", "3", universalCfsParam{DataType: "integer > 0"}, false},
+		{"int с пробелом", " 3 ", "3", true},
-		{"string совпал", "QoS-100Mbit", "QoS-100Mbit", universalCfsParam{DataType: "string"}, true},
+		{"int расхождение", "1", "3", false},
+		{"null и пустое", "null", "", true},
+		{"string совпал", "QoS-100Mbit", "QoS-100Mbit", true},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := modifierValuesEqual(tt.wanted, tt.current, tt.param); got != tt.want {
+			if got := modifierRawValuesEqual(tt.wanted, tt.current); got != tt.want {
-				t.Errorf("modifierValuesEqual() = %v, want %v", got, tt.want)
+				t.Errorf("modifierRawValuesEqual(%q, %q) = %v, want %v", tt.wanted, tt.current, got, tt.want)
 			}
 		})
 	}
 }
 
-func TestModifierValuesEqual_JSON(t *testing.T) {
+func TestModifierRawValuesEqual_JSON(t *testing.T) {
-	mapParam := universalCfsParam{DataType: "map-fixed"}
+	// JSON: порядок ключей не значим
-	// map-fixed: порядок ключей не значим
+	if !modifierRawValuesEqual(
-	if !modifierValuesEqual(
 		`{"mainDns":"81.22.46.22","ipAddrPool":"10.10.102.0/24"}`,
-		`{"ipAddrPool":"10.10.102.0/24","mainDns":"81.22.46.22"}`,
+		`{"ipAddrPool":"10.10.102.0/24","mainDns":"81.22.46.22"}`) {
-		mapParam) {
+		t.Errorf("JSON с разным порядком ключей должен считаться равным")
-		t.Errorf("map-fixed с разным порядком ключей должен считаться равным")
 	}
-
+	// JSON-массив: порядок элементов значим
-	arrParam := universalCfsParam{DataType: "array-map-fixed"}
+	if !modifierRawValuesEqual(`[{"name":"a"}]`, `[{"name":"a"}]`) {
-	// array-map-fixed: порядок элементов массива значим
+		t.Errorf("эквивалентные JSON-массивы должны быть равны")
-	if !modifierValuesEqual(`[{"name":"a","count":1},{"name":"b","count":2}]`, `[{"name":"a","count":1},{"name":"b","count":2}]`, arrParam) {
-		t.Errorf("array-map-fixed с тем же порядком должен быть равен")
 	}
-	if modifierValuesEqual(`[{"name":"a","count":1},{"name":"b","count":2}]`, `[{"name":"b","count":2},{"name":"a","count":1}]`, arrParam) {
+	if modifierRawValuesEqual(`[{"name":"a"},{"name":"b"}]`, `[{"name":"b"},{"name":"a"}]`) {
-		t.Errorf("array-map-fixed с другим порядком не должен быть равен")
+		t.Errorf("JSON-массивы с другим порядком не должны быть равны")
 	}
 }
 
 func TestModifierDesiredEqualsLive(t *testing.T) {
-	cfsParams := []universalCfsParam{
-		{SvcOperationCfsParamId: 372, Code: "ipSpaceName", DataType: "string"},
-	}
-
 	newClient := func(liveParams string) (*UniversalClient, func()) {
 		return makeTestClient(func(w http.ResponseWriter, r *http.Request) {
 			w.Header().Set("Content-Type", "application/json")
@@ -63,7 +56,7 @@ func TestModifierDesiredEqualsLive(t *testing.T) {
 	t.Run("совпало с live", func(t *testing.T) {
 		c, cleanup := newClient(`{"ipSpaceName":"internet-ipv4-v1"}`)
 		defer cleanup()
-		equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}, cfsParams)
+		equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"})
 		if err != nil {
 			t.Fatalf("unexpected err: %v", err)
 		}
@@ -75,12 +68,24 @@ func TestModifierDesiredEqualsLive(t *testing.T) {
 	t.Run("отличается от live", func(t *testing.T) {
 		c, cleanup := newClient(`{"ipSpaceName":"no-needed"}`)
 		defer cleanup()
-		equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}, cfsParams)
+		equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"})
 		if err != nil {
 			t.Fatalf("unexpected err: %v", err)
 		}
 		if equal {
-			t.Errorf("expected not equal when live differs from desired")
+			t.Errorf("expected not equal when live differs")
+		}
+	})
+
+	t.Run("код не найден в live — не пропускаем", func(t *testing.T) {
+		c, cleanup := newClient(`{"otherParam":"x"}`)
+		defer cleanup()
+		equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"})
+		if err != nil {
+			t.Fatalf("unexpected err: %v", err)
+		}
+		if equal {
+			t.Errorf("must not report equal when code is absent in live")
 		}
 	})
 
@@ -89,7 +94,7 @@ func TestModifierDesiredEqualsLive(t *testing.T) {
 			w.WriteHeader(http.StatusInternalServerError)
 		})
 		defer cleanup()
-		equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"}, cfsParams)
+		equal, err := c.modifierDesiredEqualsLive(context.Background(), "inst-1", map[string]string{"ipSpaceName": "internet-ipv4-v1"})
 		if err == nil {
 			t.Errorf("expected error when live is unavailable")
 		}
@@ -98,3 +103,4 @@ func TestModifierDesiredEqualsLive(t *testing.T) {
 		}
 	})
 }
+

provider/internal/core/operation_cfs.go

@@ -74,21 +74,3 @@ func (c *UniversalClient) fetchOperationCfsParams(ctx context.Context, opUid str
 	}
 	return def.SvcOperation.CfsParams, nil
 }
-
-// fetchOperationSchemaByID возвращает схему операции по svcOperationId БЕЗ создания
-// операции: GET /instanceOperations/default/{opId}.
-//
-// Зачем: idempotency pre-check обязан выполняться ДО POST /instanceOperations — иначе
-// при пропуске останется созданная-но-незапущенная операция («черновик», pending),
-// и следующий modify будет ждать idle до таймаута.
-func (c *UniversalClient) fetchOperationSchemaByID(ctx context.Context, opId int) ([]universalCfsParam, error) {
-	defResp, _, err := c.doRequest(ctx, "GET", fmt.Sprintf("/instanceOperations/default/%d", opId), nil)
-	if err != nil {
-		return nil, fmt.Errorf("не удалось получить схему операции %d: %w", opId, err)
-	}
-	var def universalOpDefaultResponse
-	if uerr := json.Unmarshal(defResp, &def); uerr != nil {
-		return nil, fmt.Errorf("не удалось разобрать схему операции %d: %w", opId, uerr)
-	}
-	return def.SvcOperation.CfsParams, nil
-}

provider/internal/core/operation_run_bycode.go

@@ -41,18 +41,12 @@ func (c *UniversalClient) runInstanceOperationByCode(ctx context.Context, instan
 		return fmt.Errorf("операция %s недоступна для экземпляра %s", action, instanceUid)
 	}
 
-	// Схема операции нужна и для idempotency pre-check, и для маппинга кодов.
-	// Получаем её ДО создания операции (GET /instanceOperations/default/{opId}): иначе
-	// при пропуске останется созданная, но незапущенная операция («черновик», pending).
-	schemaParams, err := c.fetchOperationSchemaByID(ctx, opId)
-	if err != nil {
-		return err
-	}
-
 	// Idempotency pre-check: если все desired уже равны LIVE-значениям инстанса —
-	// выходим, НЕ создавая операцию вовсе (источник — state.params, см. instanceLiveParams).
+	// выходим, НЕ создавая операцию вовсе. Схема операции здесь принципиально НЕ
+	// запрашивается: pre-check идёт по live-кодам (см. modifierDesiredEqualsLive и
+	// ARCHITECTURE.md «Modifier idempotency»).
 	if idempotent {
-		equal, liveErr := c.modifierDesiredEqualsLive(ctx, instanceUid, params, schemaParams)
+		equal, liveErr := c.modifierDesiredEqualsLive(ctx, instanceUid, params)
 		if liveErr == nil && equal {
 			return nil
 		}