[WIP] Fix workflow failure for smoke cursor - #50455
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
/smoke-cursor |
|
🚀 smoke-cursor has started processing this pull request comment |
There was a problem hiding this comment.
Pull request overview
Adds secret validation for behavior-defined engines to catch missing Cursor credentials before execution.
Changes:
- Validates declared engine authentication secrets.
- Adds focused harness coverage.
- Regenerates the Cursor smoke workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/behavior_defined_engine.go |
Generates validation steps from engine auth bindings. |
pkg/workflow/behavior_defined_engine_harness_test.go |
Tests auth-secret validation behavior. |
.github/workflows/smoke-cursor.lock.yml |
Adds generated Cursor secret validation and failure reporting. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| documentationURL := "" | ||
| if behavior.Installation != nil { | ||
| documentationURL = behavior.Installation.DocumentationURL |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50455 does not have the implementation label and has only 32 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.9 AIC · ⌖ 10.8 AIC · ⊞ 7.1K
Comment /matt to run again
| return BuildDefaultSecretValidationStep(workflowData, secrets, e.definition.DisplayName, documentationURL) | ||
| } | ||
|
|
||
| func (e *BehaviorDefinedEngine) GetInstallationSteps(workflowData *WorkflowData) []GitHubActionStep { |
There was a problem hiding this comment.
[/diagnosing-bugs] The new secret-deduplication loop (lines 147–156) duplicates logic already in GetRequiredSecretNames. If GetRequiredSecretNames changes, GetSecretValidationStep silently diverges.
💡 Suggested fix
Replace the inlined loop with a call to the existing method:
secrets := e.GetRequiredSecretNames(workflowData)GetRequiredSecretNames already handles deduplication and the usesUniversalLLMConsumer branch, so the guard block at lines 133–138 can also be removed.
@copilot please address this.
| Tools: map[string]any{"github": map[string]any{}}, | ||
| }), "\n") | ||
| assert.Contains(t, step, "Validate TESTHARNESS_API_KEY secret") | ||
| assert.Contains(t, step, "TESTHARNESS_API_KEY: ${{ secrets.TESTHARNESS_API_KEY }}") |
There was a problem hiding this comment.
[/diagnosing-bugs] The test verifies that a secret validation step is generated when Installation is set, but there is no test for the root-cause path: Auth bindings present and Installation == nil (i.e. the Cursor case). Without this, the regression that triggered the PR is not caught.
💡 Suggested test
func TestBehaviorDefinedEngineGetSecretValidationStepNilInstallation(t *testing.T) {
def := newHarnessEngineDefinition()
def.Auth = []AuthBinding{{Role: "api-key", Secret: "CURSOR_API_KEY"}}
def.Behavior.Installation = nil // the regression scenario
engine, err := NewBehaviorDefinedEngine(def)
require.NoError(t, err)
step := engine.GetSecretValidationStep(&WorkflowData{
Tools: map[string]any{"github": map[string]any{}},
})
// Should still produce a step, not an empty one
assert.NotEmpty(t, step, "expected validation step even without Installation")
}This pins the fix and will catch future regressions at the unit-test level.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
|
There was a problem hiding this comment.
Review: Fix workflow failure for smoke cursor
The changes correctly address the nil Installation panic for non-universal engines (like Cursor) that have Auth bindings but no Installation documentation URL.
Key observations:
- The
behavior == nilguard is now separated from thebehavior.Installation == nilcheck — correct. - For non-universal engines, secrets are built directly from
e.definition.Auth, bypassing theInstallation-dependent path. - Empty
documentationURLis passed whenInstallationis nil.shellEscapeArg("")returns an empty string, so the script receives a trailing empty argument — acceptable, and the generated lock file confirms it works. - An existing review comment already flags the empty-URL concern at line 161.
- The new test covers the nil
Installation+Authbinding code path.
No additional blocking issues found. The fix is sound.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 31 AIC · ⌖ 13.8 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Verdict: Request changes
One confirmed blocking correctness bug: the empty-documentationURL fallback breaks validate_multi_secret.sh argument parsing for engines without an Installation block (e.g. Cursor), verified directly against the generated smoke-cursor.lock.yml.
💡 Details
Blocking issue: GetSecretValidationStep in pkg/workflow/behavior_defined_engine.go sets documentationURL := "" when behavior.Installation == nil. This empty string is silently dropped by shellJoinArgs/shellEscapeArg when building the run: command line (confirmed in the compiled smoke-cursor.lock.yml, which is missing the docs-URL argument entirely: validate_multi_secret.sh CURSOR_API_KEY Cursor). Because validate_multi_secret.sh uses positional indexing (${!#} for docs URL, ${@: -2:1} for engine name) to parse its args, the shifted arguments cause Cursor to be misparsed as the docs URL and CURSOR_API_KEY as the engine name, leaving zero secret names — which trips the script's own "at least one secret name is required" guard and fails the step unconditionally, regardless of whether CURSOR_API_KEY is actually configured. This is the exact class of failure the PR claims to fix, reintroduced by the empty-string fallback path. This must be fixed before merge (either supply a non-empty placeholder docs URL, or make shellJoinArgs always quote empty args).
Same theme independently flagged by both my own review and the pre-existing Copilot review comment on this line, reinforcing confidence.
No other blocking issues found; the deduping-secret loop and test coverage additions are reasonable.
🔎 Code quality review by PR Code Quality Reviewer · auto · 75.2 AIC · ⌖ 5.52 AIC · ⊞ 7.9K
Comment /review to run again
| return GitHubActionStep{} | ||
| } | ||
| return BuildDefaultSecretValidationStep(workflowData, secrets, e.definition.DisplayName, behavior.Installation.DocumentationURL) | ||
| documentationURL := "" |
There was a problem hiding this comment.
Empty documentationURL passed to the validation script silently produces an all-secrets-empty (not-empty!) trailing arg, mangling script argument parsing for engines like Cursor that have no Installation block.
💡 Explanation and fix
GetSecretValidationStep now falls through when behavior.Installation == nil, setting documentationURL := "" (lines 159-162). That empty string is forwarded to BuildDefaultSecretValidationStep → GenerateMultiSecretValidationStep, which appends it as the last positional shell arg via shellJoinArgs/shellEscapeArg. shellEscapeArg("") returns the bare empty string, which when joined with spaces and interpolated unquoted in the generated run: command simply disappears — verified in smoke-cursor.lock.yml line 244: ... validate_multi_secret.sh CURSOR_API_KEY Cursor (docs URL argument is missing entirely, not an empty string).
validate_multi_secret.sh requires $# -ge 3 and computes DOCS_URL="${!#}" (last arg) and ENGINE_NAME="${@: -2:1}" (second-to-last). With the docs URL silently dropped, the script instead parses Cursor as DOCS_URL and CURSOR_API_KEY as ENGINE_NAME — the secret name list becomes empty, so the script's own if [ "${#SECRET_NAMES[@]}" -eq 0 ] guard fires with Error: At least one secret name is required, always failing the validation step for Cursor regardless of whether CURSOR_API_KEY is set. This is the same class of bug the PR is meant to fix, just reintroduced via the empty string case.
Fix: don't pass an empty docs URL through to the script. Either fall back to a sensible default string (e.g. the engine's docs base URL or a placeholder like "N/A"), or change GenerateMultiSecretValidationStep/shellJoinArgs to always quote arguments (so an empty string round-trips as '') rather than relying on unquoted interpolation to be forgiving.
|
🎉 This pull request is included in a new release. Release: |
Uh oh!
There was an error while loading. Please reload this page.