Repository navigation
[avenger] Fix stale work-queue integration fixtures - #67585
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #67585 locally, but safeoutputs review submission was blocked by the harness: one attempt required unavailable approval and a second attempt was rejected by the shell guard. No GitHub write was emitted.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed for PR #67585: has_implementation_label=false and default_business_additions=13 (<=100 threshold, no custom .design-gate.yml). Neither Condition A nor Condition B is met.
|
|
🔬 Test Quality Sentinel is analyzing test quality on this pull request... |
There was a problem hiding this comment.
🟢 Approval recommended
The fixture updates consistently reflect existing production behavior without changing runtime code.
0 open findings
What changed in this PR
Updates stale work-queue integration fixtures to match current repository scheduling and dispatch behavior.
Changes:
- Expect AW-authorized dispatcher policies.
- Isolate legacy non-strict fixtures from repository configuration.
- Add shared imports and correct dispatch workflow basename assertions.
Security review found no production regression; changes are test-only.
| File | Description |
|---|---|
pkg/workflow/work_queue_persistent_memory_integration_test.go |
Links shared workflow imports into fixtures. |
pkg/workflow/work_queue_dispatch_identity_integration_test.go |
Isolates legacy explicit-principal compilation. |
pkg/workflow/work_queue_compilation_integration_test.go |
Updates strictness, isolation, and policy expectations. |
actions/setup/js/work_queue_control_adapter_checks.cjs |
Matches the native dispatch API’s basename behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — this is a well-targeted root-cause repair, not a symptom patch. Approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause verified, not papered over: The
GH_AW_WORK_QUEUE_POLICYassertion flips track a real production change —.github/workflows/aw.jsonnow configureswork_queue, whichwork_queue_repo_config.gouses to setAuthorization = "aw", causing the policy env var to always be emitted. The oldNotContainsassertions were simply stale, not something this PR is working around. compiler.gitRootisolation follows existing convention: Several other integration tests (compiler_repo_config_test.go,action_sha_validation_test.go,notify_comment_test.go) already setcompiler.gitRootto isolate fixtures from the real repo'saw.json. This PR applies the same pattern correctly to prevent the repo's scheduling config from leaking into legacy explicit-principal fixtures.path.posix.basename(WORKFLOW)fix is correct: Confirmed inwork_queue_native.cjs:135andwork_queue_provisioning.cjs:86that production dispatch code already strips to basename — the mock assertion was stale, and this change correctly re-aligns it while preserving all identity-mismatch assertions.- Symlink fixture for shared imports: Minimal, scoped fix to provide the
shared/workflow imports the linter-factory doc excerpts require; no production code touched.
Minor suggestions (non-blocking)
- Add a short inline comment explaining why the
GH_AW_WORK_QUEUE_POLICYexpectation flipped inwork_queue_compilation_integration_test.go(lines 95-96 and ~604-605), consistent with the explanatory comments already added elsewhere in this PR (e.g.// Keep coverage of legacy explicit profiles...). - Consider one additional nested-path case in the
.cjsfixture to make theposix.basenamestripping assertion more meaningfully load-bearing.
Positive Highlights
- ✅ Scope is tightly bounded — only test/fixture files touched, no production code or
.github/workflows/*.lock.ymlchanges. - ✅ PR description includes clear validation evidence (which test groups pass) and is transparent about unverified items (full lint/unit suite, Vitest timeout).
- ✅ Independently re-ran all five named integration test groups locally (
go test -tags=integration) — all pass.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 95.6 AIC · ⌖ 15.7 AIC · ⊞ 10.3K
Comment /matt to run again
| require.Equal(t, -1, mint) | ||
| } | ||
| require.NotContains(t, string(content), "GH_AW_WORK_QUEUE_POLICY:") | ||
| require.Contains(t, string(content), "GH_AW_WORK_QUEUE_POLICY:") |
There was a problem hiding this comment.
[/diagnosing-bugs] Good fix — this flips the assertion to match current production behavior where GH_AW_WORK_QUEUE_POLICY is now emitted whenever aw.json configures work_queue (confirmed via work_queue_repo_config.go:100 setting Authorization = "aw"). One nit: add a short comment above this assertion explaining why the expectation flipped (repo-level aw.json now always triggers AW-authorized policy emission), mirroring the explanatory comments you added at lines 198 and the dispatch-identity test. That keeps the next person who touches this fixture from re-flipping it by mistake.
@copilot please address this.
| assert.equal(dispatch.sender.run_attempt, 2); | ||
| assert.equal(parameters.ref, REF); | ||
| assert.equal(parameters.workflow_id, WORKFLOW); | ||
| assert.equal(parameters.workflow_id, path.posix.basename(WORKFLOW)); |
There was a problem hiding this comment.
[/diagnosing-bugs] Correct root-cause fix — verified work_queue_native.cjs:135 and work_queue_provisioning.cjs:86 already use posix.basename(destination.workflow) for dispatch calls, so this mock assertion was simply stale, not a workaround. Nice catch aligning the fixture with actual adapter behavior.
💡 Optional hardening
Consider adding one more case where WORKFLOW contains a nested path (e.g. shared/worker.lock.yml) to explicitly assert the basename-stripping behavior is intentional rather than incidental (since WORKFLOW is currently a flat .github/workflows/worker.lock.yml, basename happens to equal the filename and wouldn't fail even without posix.basename on the production side in simple cases).
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — Infrastructure
📊 Infrastructure Signals
Changes reviewed:
Verdict
|
There was a problem hiding this comment.
Test Quality Sentinel: fixture repair PR with no new behavioral tests. Existing tests correctly repaired. ✅
🧪 Test quality analysis by Test Quality Sentinel · copilot · haiku45 · 83.2 AIC · ⌖ 8.37 AIC · ⊞ 8.2K
Comment /review to run again
CI failure and repair
The first failed job in CI run 38100223336, Integration: Workflow Misc Part 2, exposed stale work-queue integration fixtures.
aw.jsonscheduling configuration. Production strict-mode behavior is unchanged.Only four test/helper files changed; no production code or
.github/workflows/files are included.Validation
TestWorkQueueDispatchCredentialActualCompilation,TestWorkQueueNamedProfileCompilation,TestWorkQueueDispatchCompilerConfiguration,TestWorkQueueCompiledPolicyAndLaunchIdentity, andTestWorkQueueFactoryDocumentationExcerptsCompile(including their subtests).make fmt,make lint-cjs(existing warnings, zero errors), andgit diff --check.make agent-report-progressbuilt the CLI, but the overall gate hit its 150-second budget after starting lint and impacted tests. It reported missinggolangci-lint; no installation was attempted. Full Go lint and unit-test completion are not claimed.Environment limits
Fetching
origin/mainfailed TLS certificate verification; verification was not bypassed. The available localorigin/mainwas already merged and matched the failed CI revision (fed2eb5). No valid steering issue number was supplied, so steering comments could not be queried. Thereport_progresstool is unavailable in this run; the repair is committed locally for safe-output publication.