feat: surface missing coder_secret requirements on resolve-autostart#25081
Conversation
Docs preview📖 View docs preview for |
4e5a311 to
39f0526
Compare
| }) | ||
| return | ||
| } | ||
| response.SecretMismatch = secretMismatch |
There was a problem hiding this comment.
It feels like we could have a single function to evaluate autostart readiness that we call here. Something like EvaluateAutostartReadiness(version, owner, lastBuildParams) (ParameterMismatch, SecretMismatch, error). I'm inclined to tackle in a follow-up if other folks agree.
|
/coder-agents-review |
There was a problem hiding this comment.
This is a well-structured, cohesive PR. The new evaluateSecretMismatch function is cleanly parameterized, the error handling for forbidden callers and incomplete versions is deliberate, and the test coverage for HasSecretValidationDiagnostic (six edge cases) and the integration test (mismatch detected then cleared) are both genuine. The additive SecretMismatch field preserves backward compatibility with zero-value defaults.
Two P2 findings, six P3, two Nits. The headline items: the handler returning 500 on renderer failure discards the already-computed ParameterMismatch (the frontend gets nothing when it should at least get the parameter signal), and the evaluation function is private to coderd while the PR description claims it's a shared helper for the lifecycle executor follow-up in coderd/autobuild.
"The dashboard banner says 'secrets are fine' when the truth is 'couldn't check.'" (Zoro)
🤖 This review was automatically generated with Coder Agents.
8d44326 to
4ae5b5e
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Strong follow-up. Nine of ten R1 findings addressed in a single commit, all root-cause fixes. The handler error handling restructured from early-return 500 to switch/log-and-continue, preserving ParameterMismatch on renderer failure. EvaluateSecretMismatch exported to dynamicparameters/secrets.go with a proper logger parameter. Forbidden-caller test added. The result.Output == nil guard correctly replaced the dead result == nil check.
DEREM-9 (naming): panel unanimously accepts the author's defense (6/6). secret_mismatch parallels parameter_mismatch on the same struct. API-level symmetry outweighs cross-type vocabulary alignment.
Two open items remain: DEREM-8 (P3, the type assertion was moved but not changed to use ExtractDiagnosticExtra) and one new P3 from R2.
"Boring code gets silence. This code earned it." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
4ae5b5e to
56e643d
Compare
| defer renderer.Close() | ||
|
|
||
| result, diags := renderer.Render(ctx, ownerID, paramValues, IncludeSecretRequirements()) | ||
| if result.Output == nil || hasSecretValidationBlockerDiagnostic(diags) { |
There was a problem hiding this comment.
I think we should log or return something when result.Output == nil and the diagnostics have errors. I agree with treating this as unknown and leaving SecretMismatch false, but right now this path also drops the diagnostics, so a broken preview ends up looking the same as "no secret mismatch" with no log trail.
Could this either return an error so the caller's existing warning path fires, or log the diagnostics here before returning false?
There was a problem hiding this comment.
Not blocking but probably worth addressing 👍
There was a problem hiding this comment.
That's a fair point. There were there distinct cases being handled here. I added logging for each one.
Adds dynamicparameters.CheckSecretRequirements as a shared helper on top of the existing renderer, then wires it into the resolve-autostart handler so the dashboard can surface unsatisfied coder_secret requirements in a template alongside parameter mismatch. The lifecycle executor preflight will land in a follow-up that depends on this helper. The dashboard banner that consumes the new missing_secret_requirements field is also a follow-up. Generated with assistance from Coder Agents.
56e643d to
b931f01
Compare
Adds
dynamicparameters.EvaluateSecretMismatchas a shared helper on top of the existing renderer, then wires it into the resolve-autostart handler so the UI can surface unsatisfiedcoder_secretrequirements in a template alongside parameter mismatch.The lifecycle executor preflight will land in a follow-up that depends on this helper. The UI changes that consume the new
secret_mismatchfield is also a follow-up.Generated with assistance from Coder Agents.