fix: use exact YAML line matching for agent job detection in integration tests#25780
fix: use exact YAML line matching for agent job detection in integration tests#25780
Conversation
…L line matching Agent-Logs-Url: https://github.com/github/gh-aw/sessions/592be6a0-7628-4748-8064-710d92bbac5e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates integration tests to detect the agent (or other) job definitions using exact YAML job-line matching, avoiding false positives where agent: appears in YAML comments (e.g., container image tags).
Changes:
- Switch job detection in multiple integration tests from substring matching to exact YAML job key markers (e.g.,
"\n agent:\n"/ exact line equality). - Adjust job-section extraction logic to start from the matched job key line and stop at the next top-level job key.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/trial_mode_test.go | Uses an exact YAML job marker to extract the agent job section for token assertions. |
| pkg/workflow/local_action_permissions_test.go | Uses an exact YAML job marker to locate and slice the target job section before validating permissions/checkout steps. |
| pkg/workflow/environment_test.go | Changes agent job detection to exact line equality instead of substring matching. |
| pkg/workflow/checkout_optimization_test.go | Uses an exact YAML job marker to bound the agent job section for checkout-step assertions. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
| // Extract the agent job section using exact YAML job marker | ||
| agentJobMarker := "\n agent:\n" | ||
| markerIdx := strings.Index(lockContentStr, agentJobMarker) | ||
| if markerIdx == -1 { | ||
| t.Fatalf("Agent job not found in compiled workflow") | ||
| } | ||
| agentJobStart := markerIdx + len("\n ") // point to "agent:\n" | ||
|
|
There was a problem hiding this comment.
This test reimplements job-section extraction with manual string indexing and hard-coded offsets (e.g., len("\n ")). There is already an extractJobSection(yamlContent, jobName) helper in pkg/workflow/compiler_test_helpers_test.go that extracts a job by YAML indentation, which would simplify this logic and reduce brittleness if formatting changes.
See below for a potential fix:
agentJobSection := extractJobSection(lockContentStr, "agent")
if agentJobSection == "" {
t.Fatalf("Agent job not found in compiled workflow")
}
| // Extract the agent job section using exact YAML job marker | ||
| agentJobMarker := "\n agent:\n" | ||
| markerIdx := strings.Index(lockContent, agentJobMarker) | ||
| if markerIdx == -1 { | ||
| t.Error("Expected agent job in trial mode") | ||
| return | ||
| } | ||
|
|
||
| agentJobStart := markerIdx + len("\n ") // point to "agent:\n" | ||
|
|
There was a problem hiding this comment.
The agent job section is extracted via a custom marker/index scan and magic offsets. Consider reusing the existing extractJobSection(yamlContent, jobName) helper from pkg/workflow/compiler_test_helpers_test.go to avoid duplicating YAML job-boundary logic and make the test less sensitive to minor formatting changes.
See below for a potential fix:
agentJobSection := extractJobSection(lockContent, "agent")
if agentJobSection == "" {
t.Error("Expected agent job in trial mode")
return
}
| // Verify the job exists and extract using exact YAML job marker | ||
| jobMarker := "\n " + tt.jobName + ":\n" | ||
| markerIdx := strings.Index(lockContentStr, jobMarker) | ||
| if markerIdx == -1 { | ||
| t.Errorf("Expected %s job to be present", tt.jobName) | ||
| return | ||
| } | ||
|
|
||
| // Extract the job section | ||
| jobStart := strings.Index(lockContentStr, jobMarker) | ||
| if jobStart == -1 { | ||
| t.Fatalf("%s job not found in compiled workflow", tt.jobName) | ||
| } | ||
| jobStart := markerIdx + len("\n ") // point to "jobName:\n" | ||
|
|
There was a problem hiding this comment.
This test duplicates YAML job extraction logic using string markers and manual scanning. There is an extractJobSection(yamlContent, jobName) helper in pkg/workflow/compiler_test_helpers_test.go that can extract tt.jobName's job body by indentation and would remove the need for the marker/index arithmetic here.
See below for a potential fix:
// Verify the job exists and extract its YAML section using the shared helper.
jobSection := extractJobSection(lockContentStr, tt.jobName)
if jobSection == "" {
t.Errorf("Expected %s job to be present", tt.jobName)
return
}
🧪 Test Quality Sentinel ReportTest Quality Score: 95/100✅ Excellent test quality
What This PR DoesAll four files are modified (not newly created). The PR refactors the agent job section detection in integration tests from a fragile substring search to an exact YAML job-line match:
This is a correctness improvement: the old approach could produce a false-positive "agent job found" match if any step or key within another job contained the string Test Classification Details
Observations (Non-Blocking)i️ Pre-existing redundant assertion in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 95/100. Test quality is excellent — 0% of modified tests are implementation tests (threshold: 30%). All 4 changed test functions are behavioral integration tests that verify the compiler's observable YAML output. The changes improve assertion accuracy by using exact YAML job-line matching instead of substring search.
Same class of bug fixed in #25778 — agent job extraction in integration tests uses
strings.Index(content, "agent:")which matches container image references in YAML comments (e.g.,ghcr.io/github/gh-aw-firewall/agent:0.25.18) instead of the actual job definition line.Changes
checkout_optimization_test.go,trial_mode_test.go,local_action_permissions_test.go: Use"\n agent:\n"(or"\n " + jobName + ":\n") as the marker to find the exact YAML job line instead of a substring matchenvironment_test.go: Useline == " agent:"exact match instead ofstrings.ContainsBefore:
After: