fix: handle SecretVar JSON objects without value field in UnmarshalJSON

[BearTS]

Summary

Fixes a bug in SecretVar.UnmarshalJSON where JSON objects that do not contain a "value" field (e.g., {"env_var":"MY_KEY","from_env":true}) were not being parsed correctly as the structured compat format. The previous logic used sonic.Get to check for a "value" key, which caused the object to fall through to plain-string handling instead of the struct deserialization path.

Changes

• Replaced the sonic.Get(data, "value") existence check with a direct byte-level inspection (bytes.TrimSpace) to detect whether the input is a JSON object ({). This ensures any JSON object — with or without a "value" field — is routed through the secretVarCompat struct unmarshaling path.
• Added a test case covering the from_env format without a "value" field to prevent regression.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

go test ./core/schemas/... -run TestSecretVar_UnmarshalJSON_BackwardCompat

Expected: all subtests pass, including the new from_env without value field case which verifies that env_var/from_env objects without a "value" key correctly resolve the environment variable and set the ref to env.MY_KEY.

Screenshots/Recordings

N/A

Breaking changes

• Yes
• No

Related issues

N/A

Security considerations

This change affects how secret variable references are resolved from environment variables. The fix ensures env-backed secrets are correctly identified and resolved rather than silently falling back to treating the raw JSON as a plain string value, which could have led to secrets being mishandled or unresolved.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ca365c3e-b5b4-47f2-85a9-f7f6e186fcc2

📥 Commits

Reviewing files that changed from the base of the PR and between f111e1c and 77c2e80.

📒 Files selected for processing (2)

• core/schemas/secretvar.go
• core/schemas/secretvar_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• core/schemas/secretvar_test.go
• core/schemas/secretvar.go

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Prevented object-style secret parsing from running on non-object JSON payloads, avoiding incorrect interpretation when the input isn’t a { ... } object.
  • Strengthened backward-compatible handling for legacy env_var/from_env inputs when the value field is missing, ensuring consistent secret reference normalization and resolved value.
• Tests
  • Added tests covering legacy formats without value, verifying both unmarshalling and NewSecretVar behavior match expected normalized references and resolved values.

Walkthrough

SecretVar JSON parsing now requires an object-shaped payload before reading compatibility fields, and tests cover legacy env_var/from_env input without a value field.

Changes

SecretVar JSON parsing

Layer / File(s)	Summary
JSON shape guard core/schemas/secretvar.go	NewSecretVar and SecretVar.UnmarshalJSON trim leading whitespace and only read secret-object fields when the payload starts with {.
Backward-compatibility tests core/schemas/secretvar_test.go	Tests cover legacy env_var/from_env input without value, verifying the normalized raw reference, secret state, and resolved value for both paths.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main bug fix in SecretVar JSON unmarshaling.
Description check	✅ Passed	The description matches the template well and includes the required summary, changes, testing, security, and checklist sections.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 06-26-fix_less_strict_unmarshalling_for_secret_var

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

_{Comment @coderabbitai help to get the list of available commands.}

[BearTS]

• fix: handle SecretVar JSON objects without value field in UnmarshalJSON #4723 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Confidence Score: 5/5

Safe to merge — the change is a targeted two-line fix in a single schema file with matching regression tests for both affected entry points.

The fix correctly addresses the routing logic in both UnmarshalJSON and NewSecretVar. The first-byte { check is a well-established idiom, gracefully degrades (falls through to the plain-string path) when sonic.Unmarshal fails, and is covered by new table-driven tests that exercise the exact broken format. No concurrency, streaming, or provider-interface concerns are in scope.

No files require special attention beyond the already-reviewed core/schemas/secretvar.go.

Important Files Changed

Filename	Overview
core/schemas/secretvar.go	Replaces sonic.Get(…, "value").Exists() guard with a first-byte { check in both UnmarshalJSON and NewSecretVar so that JSON objects lacking a "value" field (e.g. from_env format) are correctly routed through struct deserialization; introduces bytes import.
core/schemas/secretvar_test.go	Adds regression test from_env without value field inside TestSecretVar_UnmarshalJSON_BackwardCompat and a new top-level TestNewSecretVar_FromEnvWithoutValueField covering the exact JSON format fixed by this PR.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["UnmarshalJSON / NewSecretVar called"] --> B["strconv.Unquote data → val"]
    B --> C{"sonic.Valid(data)?"}
    C -- No --> G
    C -- Yes --> D{"bytes.TrimSpace(data)[0] == '{'?"}
    D -- No --> G["Plain-string path\n(env. / vault. prefix or literal)"]
    D -- Yes --> E["sonic.Unmarshal into secretVarCompat"]
    E -- fail --> G
    E -- ok --> F{"Which fields are set?"}
    F -- "SecretType != empty" --> H["New format: use explicit type + ref"]
    F -- "Ref != empty" --> I["Infer type from ref prefix"]
    F -- "FromEnv && EnvVar != empty" --> J["Backward-compat: build env.VAR ref,\nresolve from OS env"]
    F -- "Val == EnvVar starts with env." --> K["Legacy format: resolve from OS env"]
    F -- "none match" --> L["Resolve via SecretTypeEnv/Vault\nor leave Val as-is"]
    H & I & J & K & L --> M["return nil ✓"]
    G --> N["vault. → LookupVault\nenv. → os.LookupEnv\nother → plain Val"]
    N --> M

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["UnmarshalJSON / NewSecretVar called"] --> B["strconv.Unquote data → val"]
    B --> C{"sonic.Valid(data)?"}
    C -- No --> G
    C -- Yes --> D{"bytes.TrimSpace(data)[0] == '{'?"}
    D -- No --> G["Plain-string path\n(env. / vault. prefix or literal)"]
    D -- Yes --> E["sonic.Unmarshal into secretVarCompat"]
    E -- fail --> G
    E -- ok --> F{"Which fields are set?"}
    F -- "SecretType != empty" --> H["New format: use explicit type + ref"]
    F -- "Ref != empty" --> I["Infer type from ref prefix"]
    F -- "FromEnv && EnvVar != empty" --> J["Backward-compat: build env.VAR ref,\nresolve from OS env"]
    F -- "Val == EnvVar starts with env." --> K["Legacy format: resolve from OS env"]
    F -- "none match" --> L["Resolve via SecretTypeEnv/Vault\nor leave Val as-is"]
    H & I & J & K & L --> M["return nil ✓"]
    G --> N["vault. → LookupVault\nenv. → os.LookupEnv\nother → plain Val"]
    N --> M

Loading

_{Reviews (4): Last reviewed commit: "fix: less strict unmarshalling for secre..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (1)

core/schemas/secretvar_test.go (1)

117-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a whitespace-prefixed object case.

SecretVar.UnmarshalJSON now trims leading whitespace before deciding whether to take the compat-object path, but this new subtest still starts with {, so the new guard itself never gets exercised.

Suggested addition

 	t.Run("from_env without value field", func(t *testing.T) {
 		input := `{"env_var":"MY_KEY","from_env":true}`
 		var sv SecretVar
 		if err := sv.UnmarshalJSON([]byte(input)); err != nil {
 			t.Fatalf("UnmarshalJSON failed: %v", err)
 		}
 		if sv.GetRawRef() != "env.MY_KEY" {
 			t.Errorf("expected ref %q, got %q", "env.MY_KEY", sv.GetRawRef())
 		}
 		if !sv.IsFromSecret() {
 			t.Error("expected IsFromSecret=true")
 		}
 		if sv.Val != "resolved-value" {
 			t.Errorf("expected Val=%q, got %q", "resolved-value", sv.Val)
 		}
 	})
+
+	t.Run("from_env without value field with leading whitespace", func(t *testing.T) {
+		input := " \n\t" + `{"env_var":"MY_KEY","from_env":true}`
+		var sv SecretVar
+		if err := sv.UnmarshalJSON([]byte(input)); err != nil {
+			t.Fatalf("UnmarshalJSON failed: %v", err)
+		}
+		if sv.GetRawRef() != "env.MY_KEY" {
+			t.Errorf("expected ref %q, got %q", "env.MY_KEY", sv.GetRawRef())
+		}
+		if !sv.IsFromSecret() {
+			t.Error("expected IsFromSecret=true")
+		}
+		if sv.Val != "resolved-value" {
+			t.Errorf("expected Val=%q, got %q", "resolved-value", sv.Val)
+		}
+	})

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/schemas/secretvar_test.go` around lines 117 - 132, The new
whitespace-trimming branch in SecretVar.UnmarshalJSON is not covered by the
current compat-object test because the input starts directly with an object
brace. Update the SecretVar.UnmarshalJSON test in secretvar_test.go by adding a
subtest that uses leading whitespace before the JSON object, and verify the same
env-based resolution behavior through GetRawRef, IsFromSecret, and Val so the
trim-before-compat-object path is actually exercised.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@core/schemas/secretvar_test.go`:
- Around line 117-132: The new whitespace-trimming branch in
SecretVar.UnmarshalJSON is not covered by the current compat-object test because
the input starts directly with an object brace. Update the
SecretVar.UnmarshalJSON test in secretvar_test.go by adding a subtest that uses
leading whitespace before the JSON object, and verify the same env-based
resolution behavior through GetRawRef, IsFromSecret, and Val so the
trim-before-compat-object path is actually exercised.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7ddbb647-c10e-413d-a7ef-bd9649513d3d

📥 Commits

Reviewing files that changed from the base of the PR and between 5e42852 and 56077d1.

📒 Files selected for processing (2)

• core/schemas/secretvar.go
• core/schemas/secretvar_test.go

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

core/schemas/secretvar.go (1)

50-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use the unquoted candidate consistently in NewSecretVar.

Line 51 checks val after strconv.Unquote, but Line 61 still unmarshals value. That means a quoted secret object string can pass the new { guard and still miss the compat path because sonic.Unmarshal sees a JSON string, not the unquoted object payload.

Suggested fix

-	if sonic.Valid([]byte(value)) {
-		if trimmed := bytes.TrimSpace([]byte(val)); len(trimmed) > 0 && trimmed[0] == '{' {
+	candidate := []byte(val)
+	if sonic.Valid(candidate) {
+		if trimmed := bytes.TrimSpace(candidate); len(trimmed) > 0 && trimmed[0] == '{' {
 			type secretVarCompat struct {
 				Val        string     `json:"value"`
 				Ref        string     `json:"ref"`
 				SecretType SecretType `json:"type"`
@@
-			if err := sonic.Unmarshal([]byte(value), &raw); err == nil {
+			if err := sonic.Unmarshal(candidate, &raw); err == nil {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@core/schemas/secretvar.go` around lines 50 - 61, In NewSecretVar, the compat
JSON-object check and unmarshal must both use the same unquoted candidate value.
Update the logic around the trimmed `{` guard so it consistently operates on the
unquoted string (the same value checked after strconv.Unquote) instead of mixing
val and value, and ensure the sonic.Unmarshal call parses that unquoted payload.
Keep the backward-compat handling in the secretVarCompat path intact while
making the candidate selection consistent.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@core/schemas/secretvar.go`:
- Around line 50-61: In NewSecretVar, the compat JSON-object check and unmarshal
must both use the same unquoted candidate value. Update the logic around the
trimmed `{` guard so it consistently operates on the unquoted string (the same
value checked after strconv.Unquote) instead of mixing val and value, and ensure
the sonic.Unmarshal call parses that unquoted payload. Keep the backward-compat
handling in the secretVarCompat path intact while making the candidate selection
consistent.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 47b7e333-a2e7-4361-86e9-eff767e5f2fd

📥 Commits

Reviewing files that changed from the base of the PR and between 56077d1 and f7128a9.

📒 Files selected for processing (2)

• core/schemas/secretvar.go
• core/schemas/secretvar_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• core/schemas/secretvar_test.go

[BearTS]

Fixes #4319

The merge-base changed after approval.

The merge-base changed after approval.