Egg/issue 2474/work - #2595
Merged
Merged
Conversation
PR #2556 shipped Parts B, C, D (drop docker runtime, delete tests/functional, retire test-e2e.yml). This draft analyses the remaining work: - Part A: wire test-integration.yml into PR CI (currently orphan workflow) - Part E: promote ScriptedProvider to public + add 8 k3s regression tests - Part F: CLAUDE.md / docs/guides/testing.md notes pointing agents at the tier Recommends Option B (E first, A+F follow-up) so the new gate's first run covers the regression categories that motivated #2474. Surfaces 7 multi-choice decisions and 6 feedback questions via egg-contract.
slice-1 (Part E): promote ScriptedProvider + 8 k3s regression tests
under integration_tests/regression/.
slice-2 (Part A): stage .github-staging/workflows/{test,test-integration}.yml
for human git mv pre-merge; new integration: job folded into Test/aggregate;
required-from-day-1 (decision-3).
slice-3 (Part F): CLAUDE.md + docs/guides/testing.md updates; depends on
slice-1. No specific test filenames per HITL Q6.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Parts B/C/D shipped in PR #2556. Remaining: Part A (wire test-integration.yml into PR CI as required check), Part E (promote ScriptedProvider + 8 k3s regression tests), Part F (docs). 3-slice DAG: slice-1 (E) and slice-2 (A) in parallel, slice-3 (F) depends on slice-1. Captures key design choices (ScriptedProvider lands at shared/egg_harness/testing/, workflow_call into test.yml keeps Test/aggregate as canonical required-check name, E.8 uses kubectl-logs scrape of gateway audit_log) plus 8 risks for risk_analyst and seed acceptance criteria for task_planner. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Eleven risks identified across CI reliability, scope, test implementability, and compatibility. Overall MEDIUM; three areas flagged for human review (R1 flake-fallback posture, R3 scope-expansion HITL escape valve, R5 E.8 push-counting mechanism).
…2586) * slice-2(#2474): stage wired-in CI workflows under .github-staging/ Stage the wired-in version of `.github/workflows/test.yml` and the flake-hardened version of `.github/workflows/test-integration.yml` under `.github-staging/workflows/`. coder is gateway-blocked from writing under `.github/`, so the human reviewer performs the `git mv` from `.github-staging/workflows/*` to `.github/workflows/*` pre-merge. The orchestrator surfaces this as a Pre-merge Obligation on the slice-2 PR body. test.yml changes vs. HEAD: - New `integration:` job sibling to `unit:` and `security:`, invoking `./.github/workflows/test-integration.yml` (post-move path). - `timeout-minutes: 30` on the `integration:` job (caller). - `aggregate:` `needs:` updated to `[unit, security, integration]` and the aggregate check now flags integration failure. - Required-from-day-1 per decision-3 of #2474. Canonical required-check name stays `Test / aggregate`. test-integration.yml changes vs. HEAD (HITL Q1 flake guards): - "Import images into k3s" wrapped in a 3-attempt retry loop with short backoff between attempts. - Per-job `timeout-minutes: 30` on the `integration` job as defense in depth (mirrors the caller-level timeout). - New `if: failure()` step captures `kubectl get events --all-namespaces -o yaml` plus pod logs from `egg-system` / `egg-test-agents`, then uploads both as a `k3s-debug` workflow artifact (`actions/upload-artifact@v4`). - Existing kubectl wait calls already have `--timeout=120s`; verified. Acceptance: - `.github-staging/workflows/test.yml` parses; `jobs.aggregate.needs` is `[unit, security, integration]` and `jobs.integration.uses` references `./.github/workflows/test-integration.yml`. - `.github-staging/workflows/test-integration.yml` parses; has retry on image-import, `--timeout=` on `kubectl wait` calls, and an on-failure artifact-upload step for `k3s-debug-events.yaml` plus `k3s-debug-pods.log`. Closes task-2-1, task-2-2 of #2474. * slice-2(#2474): address reviewer_code_holistic NACK Three fixes from the v1 NACK on commit 6e402e2: (1) BLOCKING: replace `kubectl logs --selector=""` with a per-pod enumeration loop in the on-failure debug-collection step. `kubectl logs` requires an explicit pod name or non-empty label selector — empty selector is a kubectl error, not an "all pods" primitive — so the previous formulation would have captured zero pod logs in the `k3s-debug-pods.log` artifact and silently defeated HITL Q1's flake-triage guarantee. (2) NON-BLOCKING (recommended): drop `name: Aggregate Test Results` on the aggregate job in test.yml so GitHub renders the check as `Test / aggregate` (matching the canonical required-check name documented in decision-3 / manual_steps / architect output). With the previous override, the admin's pre-merge `Test / aggregate` typed into Branch protection would have silently desync'd from the workflow's `Test / Aggregate Test Results` rendered name and blocked all PRs. (3) NON-BLOCKING (defense-in-depth): switch `set -e` to `set -eo pipefail` in the image-import retry loop. Without pipefail, a transient `docker save` failure on the left side of the pipe could be masked by `k3s ctr images import -` returning 0 on empty stdin, falsely reporting success and short-circuiting the retry. All three found by reviewer_code_holistic's pass-4 (silent-fallback hunt) and pass-2 (doc↔code symmetry) on slice-2 v1. Re-propose with these fixes. * slice-2 tests(#2474): assert staged workflow YAML structural invariants Add tests/config/test_slice_2_staging_workflows.py with 11 structural assertions over the slice-2 staged workflow YAMLs at .github-staging/workflows/test.yml and .github-staging/workflows/test-integration.yml. Test classes: TestStagedTestYmlStructure (7 tests) — `test.yml`: * integration job exists as sibling of unit/security * integration.uses references './.github/workflows/test-integration.yml' (the post-`git mv` path, not the staged path) * integration job has `timeout-minutes: 30` * aggregate.needs == {unit, security, integration} * aggregate's check_all_passed script inspects `needs.integration.result` so a red integration tier fails the aggregate (matching the canonical `Test / aggregate` required check from decision-3) * workflow_call output `passed` preserved for downstream callers * concurrency block (group + cancel-in-progress) preserved TestStagedTestIntegrationYmlFlakeGuards (4 tests) — `test-integration.yml`: * `Import images into k3s` step body wraps a retry loop (HITL-Q1 image-import flake guard) * every `kubectl wait --for=` invocation carries an explicit `--timeout=` flag (HITL-Q1 deadline guard) * an `if: failure()` step captures `kubectl get events --all-namespaces`, pod logs, and uploads them via `actions/upload-artifact@v4` with name `k3s-debug` (HITL-Q1 on-failure triage artifact) * workflow_call trigger preserved so the staged test.yml's integration job can call into it All 11 tests skip cleanly when the staged files are absent (e.g. on `main` before slice-2 lands) so the unit suite stays green for the pipeline's pre-slice-2 history. * slice-2 review(#2474): address PR #2586 feedback Address the egg-reviewer feedback on PR #2586: - Blocking: `_build_github_staging_manual_step` now detects existing targets in `.github/` and emits `git rm <target>` before `git mv` so the rendered procedure actually runs. `git mv` refuses to overwrite an existing destination, so the historic template that always emitted the plain form broke for replacement scenarios (e.g. restaging an existing workflow) — `fatal: destination exists`. Adds a regression test that exercises both the new-target and replacement-target paths. - Non-blocking #2: `tests/config/test_slice_2_staging_workflows.py` → `tests/config/test_workflows_structure.py`, with fixtures that prefer `.github-staging/workflows/<file>` when present and fall back to `.github/workflows/<file>`. The same structural invariants now guard the production CI configuration in perpetuity instead of skipping forever once the human reviewer performs the `git mv`. - Non-blocking #3: skip messages broadened to describe the actual failure mode (no workflow file found in either location). - Non-blocking #5: `test_concurrency_block_preserved` now asserts the group expression references `github.head_ref` so a regression that silently flipped concurrency to `github.run_id` (one group per run = no PR concurrency at all) is caught instead of slipping through. - Non-blocking #6: inline comment on `set -o pipefail` in the image-import retry rewritten to describe the actual mechanism (propagation into the `if` test result + suspension of `set -e` inside the conditional), not "propagates to the loop condition". Author: egg <egg@localhost> * slice-2 review(#2474): fix aggregate gate; sharpen test; tighten artifact * slice-2 review(#2474): close comment-masking gap; cover all aggregates Addresses the v3 NACK on PR #2586: (1) BLOCKING: `test_aggregate_fails_on_red_tier` did not actually catch removal of the `exit 1` statement. The previous regex captured the failure branch via `passed=false …(?=else)` and then asserted `"exit 1" in failure_branch` as a plain substring — which was silently satisfied by the warning comment line immediately above the real statement (the comment contains the literal text "exit 1" in backticks). A future developer who "cleaned up" the comment-less statement while leaving the comment behind would have bypassed the guard, and the canonical `Test / aggregate` required-for-merge gate would have become non-functional again — the exact regression class this test exists to prevent. Replace the substring check with a standalone-statement anchored regex: re.search(r"^\s*(exit\s+1|false)\s*$", failure_branch, re.MULTILINE) The `^` / `$` line anchors via MULTILINE require the exit (or false) to be the entire content of a line, so an "exit 1" inside a comment does not satisfy the assertion. Honors the docstring's "exit 1 (or false)" parenthetical by accepting either form. (2) Promoted the failure-branch check to a module-level parametrized test `test_aggregate_failure_branch_exits_nonzero` that covers all three aggregate gates simultaneously — `test.yml::aggregate`, `test-integration.yml::aggregate`, AND `lint.yml::aggregate`. The latter two had the same one-line bug and the same fix in this PR but had no structural coverage; a future regression that removed `exit 1` from any one of them is now caught by the same suite. (3) Split the integration-result check off into its own narrowly scoped `test_aggregate_check_inspects_integration_result` so the test.yml-specific assertion (a red integration tier reaches the failure branch) and the universal assertion (failure branch exits non-zero) are independently named and independently actionable. Verification: deleted ONLY the `exit 1` line from `.github-staging/workflows/test.yml` (leaving the warning comment intact) and re-ran the parametrized test — it fails for the `test.yml` case with the new assertion message, while `test-integration.yml` and `lint.yml` continue to pass. Restored the file and confirmed the full 14-test suite passes. Also updated the PR body to surface the sibling `.github/workflows/lint.yml` direct fix (was not previously called out in the body) and to mention the new parametrized coverage. Author: egg <egg@localhost> --------- Co-authored-by: egg <egg@example.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.