Use old inputs from the engine when available

[praneetloke]

🛑 INTEGRATION TESTING PENDING🛑

[praneetloke]

@claude review this PR.

[claude]

Claude finished @praneetloke's task in 1m 17s —— View job

---

PR Review: Use old inputs from the engine when available

Tasks:

• Gather context
• Read changed files and diff
• Analyze changes
• Post review feedback

---

Summary

This PR adapts the provider to take advantage of the Pulumi engine's SendsOldInputs / SendsOldInputsToDelete capabilities. When the engine advertises these capabilities (via ConfigureRequest), the provider no longer needs to manually stash inputs inside the output state blob using the __inputs sentinel key. Instead, it relies on the engine to supply old inputs directly through the OldInputs field of UpdateRequest, DeleteRequest, etc.

The approach is sound and well-aligned with how Pulumi providers should evolve to use engine-provided old inputs. A few observations below.

---

Observations & Issues

1. Create — output properties diverge based on engineSendsOldInputs (potential regression) [rest/provider.go:628-632]

if !p.engineSendsOldInputs {
    outputProperties, err = plugin.MarshalProperties(state.GetResourceState(outputsMap, inputs), state.DefaultMarshalOpts)
} else {
    outputProperties, err = plugin.MarshalProperties(resource.NewPropertyMapFromMap(outputsMap), state.DefaultMarshalOpts)
}

When engineSendsOldInputs is true, the Create response now returns only the raw API outputs without embedding inputs in __inputs. This is the correct behavior when the engine manages old inputs. However, when the response outputs don't contain properties that were provided as inputs (e.g., write-only fields, computed-only responses), the Pulumi state will silently lose those input values. The existing GetResourceState path merges inputs back in via the __inputs key, preserving them.

Consider: is there anything special that needs to happen for write-only inputs (like secrets) that the API never echoes back in responses, when the engine is managing old inputs?

2. Same pattern in Read (same concern) [rest/provider.go:836-841]

if !p.engineSendsOldInputs {
    outputProperties, err = plugin.MarshalProperties(state.GetResourceState(outputsMap, inputs), state.DefaultMarshalOpts)
} else {
    outputProperties, err = plugin.MarshalProperties(resource.NewPropertyMapFromMap(outputsMap), state.DefaultMarshalOpts)
}

The Read response must include Inputs separately (which is returned via inputsRecord), and Properties is for the raw state. This looks correct for the engine-sends-old-inputs path. No issue here aside from the write-only concern noted above.

3. Update — same pattern [rest/provider.go:991-995]

if !p.engineSendsOldInputs {
    outputProperties, err = plugin.MarshalProperties(state.GetResourceState(outputsMap, inputs), state.DefaultMarshalOpts)
} else {
    outputProperties, err = plugin.MarshalProperties(resource.NewPropertyMapFromMap(outputsMap), state.DefaultMarshalOpts)
}

The inline TODO comment (// TODO: Could this erase refreshed inputs that were previously saved in outputs state?) is still present and still unresolved. The concern it raises is real and worth tracking.

4. Update — GetOldInputs() call is unused [rest/provider.go:899-907]

oldInputs, _ := plugin.UnmarshalProperties(req.GetOldInputs(), state.HTTPRequestBodyUnmarshalOpts)
diff := oldInputs.Diff(inputs)

The engine-provided OldInputs is already being used here in the Update path to compute the diff. This is the primary purpose of the PR and it works well. However, the error from UnmarshalProperties is silently discarded (_). If GetOldInputs() returns a malformed struct, oldInputs will be nil and oldInputs.Diff(inputs) will panic.

Recommendation: Handle the error:

oldInputs, err := plugin.UnmarshalProperties(req.GetOldInputs(), state.HTTPRequestBodyUnmarshalOpts)
if err != nil {
    return nil, errors.Wrap(err, "unmarshal old inputs as propertymap")
}

Fix this →

5. getPathParamsMap — GetOldInputs still reads from the stashed __inputs key [rest/request.go:306]

oldInputs := state.GetOldInputs(properties)

GetOldInputs reads the __inputs sentinel key from the state blob. When engineSendsOldInputs is true, the state blob will no longer contain __inputs (since Create/Read/Update now skip GetResourceState). As a result, GetOldInputs will always return nil for new resources managed with the engine's old inputs. This is fine if the path params are always present in the current properties/inputs passed to getPathParamsMap. But the fallback path at line 358-365:

if oldInputs == nil {
    return nil, errors.Errorf("did not find value for path param %s in output props (old inputs was nil)", paramName)
}
property, ok = oldInputs[resource.PropertyKey(sdkName)]

...will always error with "old inputs was nil" for new resources when engineSendsOldInputs is true and the path param can't be found in the current properties. The caller should instead be passing the engine-provided old inputs (from req.GetOldInputs()) to the path param resolution logic, rather than relying on the stashed __inputs key. This may be an important correctness issue.

6. Tests hardcode SendsOldInputs: true [rest/transform_test.go:53-57, rest/provider_test.go:62-66]

Both test helper functions now set SendsOldInputs: true unconditionally. This means the tests only exercise the new code path (engineSendsOldInputs == true) and the old path (engineSendsOldInputs == false) loses test coverage. Consider adding test variants for the legacy path to avoid regressions.

7. Minor: maps.Copy in CreateGetRequest mutates the inputs argument [rest/request.go:108]

m := inputs
if currentState != nil {
    maps.Copy(m, *currentState)
}

resource.PropertyMap is a map type, so m := inputs is a reference copy. maps.Copy will mutate the caller's inputs map in place by merging currentState into it. This was present before this PR (it's not a new introduction) but is worth noting as a latent bug. If callers rely on inputs being unmodified after calling CreateGetRequest, they may see unexpected behavior.

---

Minor Notes

• The PR description calls out integration testing as pending, which is the right call given that the behavior change is conditional on engine capabilities that are hard to fully test with unit tests alone.
• The TODO at line 992 (// TODO: Could this erase refreshed inputs...) should probably be tracked as a GitHub issue rather than left as an in-code comment.

---

Verdict

The core approach is correct and well-targeted. The main risks are:

1. Silent error discard in Update (item 4 above) — easy fix, should not ship as-is.
2. getPathParamsMap fallback failure (item 5 above) — needs investigation to confirm whether this path is actually reachable when engineSendsOldInputs is true.
3. Test coverage gap for the legacy (engineSendsOldInputs == false) path.