fix(ci): unblock Full Lifecycle Tests workflow#592
Conversation
Weekly scheduled run failed with `pytest: error: unrecognized arguments: --timeout=1800` because pytest-timeout was passed to pytest but never declared as a dev dependency. - Add pytest-timeout>=2.3.0 to [project.optional-dependencies].dev - Scope lifecycle job to `environment: staging` so it can resolve ANTHROPIC_API_KEY (currently only present on the staging env, not at repo level), otherwise conftest skips the entire suite. - Capture pytest stdout/stderr to artifacts/pytest.log via tee and use --basetemp inside the workspace, so failure artifacts upload even when pytest fails to start.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe lifecycle CI job targets the staging environment; pytest output is saved to ChangesLifecycle Workflow Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Review: fix(ci): unblock Full Lifecycle Tests workflowThis is a clean, well-targeted CI fix. The root cause diagnosis is correct and the changes are minimal and focused. What works wellRoot cause fix is correct. Adding
Artifact path fix (
One thing to verify before mergingThe Confirm: does the repo have an environment named Minor notes (non-blocking)
Overall: LGTM pending confirmation that the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/lifecycle.yml:
- Around line 89-97: The workflow step named "Upload test artifacts on failure"
currently uses a mutable tag actions/upload-artifact@v4; replace that mutable
tag with the corresponding full commit SHA for actions/upload-artifact to pin
the action for supply-chain security. Locate the step with name "Upload test
artifacts on failure" and the uses line referencing actions/upload-artifact@v4
and change it to uses: actions/upload-artifact@<FULL_COMMIT_SHA> (the specific
commit SHA from the actions/upload-artifact repository release you trust).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd53c37a-e120-45bf-bf1d-91122d4a8846
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.github/workflows/lifecycle.ymlpyproject.toml
| - name: Upload test artifacts on failure | ||
| if: failure() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: lifecycle-failure-${{ github.run_id }} | ||
| path: | | ||
| /tmp/pytest-*/ | ||
| artifacts/ | ||
| retention-days: 7 | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Verify unpinned GitHub Actions references in workflow files.
# Expected: no matches after pinning to full 40-char commit SHAs.
rg -n --glob '.github/workflows/*.yml' --glob '.github/workflows/*.yaml' \
'^\s*uses:\s*[^@]+@(?!(?:[a-f0-9]{40})$).+'Repository: frankbria/codeframe
Length of output: 326
🏁 Script executed:
#!/bin/bash
# First, find and examine the lifecycle.yml file
echo "=== Finding workflow files ==="
fd -e yml -e yaml '.github/workflows'
echo -e "\n=== Checking lifecycle.yml content (lines 85-100) ==="
if [ -f ".github/workflows/lifecycle.yml" ]; then
sed -n '85,100p' ".github/workflows/lifecycle.yml"
else
echo "lifecycle.yml not found"
fi
echo -e "\n=== Checking all uses: statements in workflow files ==="
rg -n 'uses:\s*' '.github/workflows/' -t yaml | head -20
echo -e "\n=== Checking specifically for action versions ==="
rg -n 'uses:\s*.*@' '.github/workflows/' -t yamlRepository: frankbria/codeframe
Length of output: 5889
Pin actions/upload-artifact to a full commit SHA.
Line 91 uses a mutable tag (@v4), which weakens supply-chain security and provenance guarantees.
Suggested fix
- - name: Upload test artifacts on failure
- if: failure()
- uses: actions/upload-artifact@v4
+ - name: Upload test artifacts on failure
+ if: failure()
+ uses: actions/upload-artifact@<FULL_COMMIT_SHA>🧰 Tools
🪛 zizmor (1.25.2)
[error] 91-91: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/lifecycle.yml around lines 89 - 97, The workflow step
named "Upload test artifacts on failure" currently uses a mutable tag
actions/upload-artifact@v4; replace that mutable tag with the corresponding full
commit SHA for actions/upload-artifact to pin the action for supply-chain
security. Locate the step with name "Upload test artifacts on failure" and the
uses line referencing actions/upload-artifact@v4 and change it to uses:
actions/upload-artifact@<FULL_COMMIT_SHA> (the specific commit SHA from the
actions/upload-artifact repository release you trust).
The lifecycle CLI test passed `--execute` to `cf work batch run`, but that flag only exists on `cf work start` (single task). Batch always executes (use `--dry-run` to preview). The flag was silently broken since the test was added (bdb6686) because the lifecycle suite was never green in CI — the pytest-timeout dep error masked it. Fixes the AssertionError surfaced by the first non-skipped lifecycle run after the dep fix: Error: No such option: --execute
Response to review feedbackTest failure on dispatched lifecycle runThe first non-skipped lifecycle run (now possible after the pytest-timeout fix) surfaced a latent test bug:
CodeRabbit — SHA-pin
|
|
Claude finished @frankbria's task in 38s —— View job Updated Assessment
All open questions from the initial review are now resolved. Here's the updated status:
SHA-pinning deferral — reasonable. Pinning one action in isolation while ~20 others across 7 workflows remain on mutable tags creates inconsistency without meaningful security improvement. A repo-wide pinning pass with Dependabot automation is the right approach. Final diff assessmentThe three-commit stack is clean:
LGTM. This is ready to merge. |
|
Follow-up review (continuing from prior comments): 1) Missed item - the --execute flag removal from cf work batch run in test_cli_lifecycle.py is correct; --execute belongs on cf work start per CLI_WIREFRAME, not batch run. Good fix bundled here. 2) Still-open: confirm the staging GitHub environment exists with ANTHROPIC_API_KEY set and no required-reviewer protection, or workflow_dispatch runs will block on approval. 3) CodeRabbit pinning note (non-blocking): actions/upload-artifact@v4 can be SHA-pinned for supply-chain hardening but does not need to block this PR. Status: ready to merge once staging environment is confirmed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/lifecycle/test_cli_lifecycle.py`:
- Around line 68-72: The test calls cf("work", "batch", "run", ...) but does not
assert the command succeeded; capture the result of that cf invocation and
assert its exit code indicates success before proceeding to check task statuses.
Specifically, assign the call to a variable (e.g., result = cf(...)) and assert
on result.exit_code (or result.returncode depending on cf’s return object)
equals 0 (or use result.ok) immediately after the cf("work", "batch", "run",
...) call in tests/lifecycle/test_cli_lifecycle.py so failures fail fast and do
not allow later IN_PROGRESS checks to mask regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a21f2f7-e9c2-4b30-9d02-89641d9760ac
📒 Files selected for processing (1)
tests/lifecycle/test_cli_lifecycle.py
| cf( | ||
| "work", "batch", "run", | ||
| "--all-ready", "--execute", "--engine", "react", | ||
| "--all-ready", "--engine", "react", | ||
| timeout=1800, | ||
| ) |
There was a problem hiding this comment.
Assert batch execution success before checking task statuses.
Line 68-72 runs cf work batch run without validating the exit code. If that command fails, the later IN_PROGRESS check can still pass and hide a real regression.
Suggested fix
- cf(
+ run_result = cf(
"work", "batch", "run",
"--all-ready", "--engine", "react",
timeout=1800,
)
+ assert run_result.returncode == 0, (
+ f"cf work batch run failed (exit {run_result.returncode}).\n"
+ f"stdout tail:\n{run_result.stdout[-1000:]}\n"
+ f"stderr:\n{run_result.stderr[-500:]}"
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lifecycle/test_cli_lifecycle.py` around lines 68 - 72, The test calls
cf("work", "batch", "run", ...) but does not assert the command succeeded;
capture the result of that cf invocation and assert its exit code indicates
success before proceeding to check task statuses. Specifically, assign the call
to a variable (e.g., result = cf(...)) and assert on result.exit_code (or
result.returncode depending on cf’s return object) equals 0 (or use result.ok)
immediately after the cf("work", "batch", "run", ...) call in
tests/lifecycle/test_cli_lifecycle.py so failures fail fast and do not allow
later IN_PROGRESS checks to mask regressions.
`cf tasks generate` creates tasks in BACKLOG status by design (the CLI prints next-step hints suggesting `cf tasks set status READY --all` to promote them). The lifecycle fixture skipped this step, so `cf work batch run --all-ready` returned "No READY tasks found" and exited 0 — the test passed the batch-run assertion but the agent never actually built anything, and acceptance checks correctly flagged the missing csv_stats.py. Add the promotion step to initialized_workspace and update the docstring (which still referenced the no-longer-valid --execute flag).
Follow-up Review (conftest.py / test_cli_lifecycle.py changes)Building on my previous review — the latest commit adds fixture-level task promotion and drops the
|
1 participant
Summary
pytest-timeout>=2.3.0to dev dependencies — fixespytest: error: unrecognized arguments: --timeout=1800(root cause of the failing weekly run 25981293079)environment: stagingso it can resolveANTHROPIC_API_KEY(currently only set on the staging env, not at repo level). Without this, even after the timeout fix every test would be silently skipped bytests/lifecycle/conftest.py.tee artifacts/pytest.logand use--basetemp=artifacts/pytestso the failure-artifact upload step yields something useful (previous/tmp/pytest-*/path was always empty when pytest itself failed to start).Test plan
gh workflow run lifecycle.yml --ref fix/lifecycle-ci-pytest-timeout -f mode=cli -f model=haiku--timeout=1800is accepted by pytestANTHROPIC_API_KEYresolves (no "all tests skipped" outcome)Notes
tasks/node24-migration-plan.md(not in this PR — kept local) for follow-up on the Node.js 20 deprecation warning surfaced by the same run.Summary by CodeRabbit
Chores
Tests