Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 587d06e The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (10)
Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThis change adds ImpactGate policies, a CLI that prepares baselines and reports impact scores, and a GitHub Actions workflow that runs analysis on pull requests and pushes to ChangesImpactGate analysis
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant ImpactGateScript as impact-gate.py
participant ImpactGateCLI as ImpactGate CLI
participant StepSummary as GitHub step summary
GitHubActions->>ImpactGateScript: Run prepare for the pinned base commit
ImpactGateScript->>ImpactGateCLI: Build or validate baseline data
GitHubActions->>ImpactGateScript: Run report for pull requests
ImpactGateScript->>ImpactGateCLI: Score both analysis scopes
ImpactGateScript->>StepSummary: Append the report
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds advisory impact reports that do not gate merges. No merge-blocking issue was found. Check cache behavior in hosted runs after merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 3.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 4 files. (6 skipped: 6 unsupported.)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a84761464
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| impact-gate==0.4.1 | ||
| lizard==1.23.0 |
There was a problem hiding this comment.
When PyYAML, Pygments, or pathspec publishes a new compatible release, this requirements file allows pip install to select it while the cache identity remains unchanged because identity() records only ImpactGate and Lizard versions. ImpactGate and Lizard declare those dependencies with open-ended constraints (ImpactGate metadata, Lizard metadata), so a PR can restore a baseline produced with different configuration or file-filtering code than the current scoring process, making the percentile report non-reproducible. Pin the complete resolved dependency set or include every measurement-affecting installed version in the cache identity.
Useful? React with 👍 / 👎.
tobyhede
left a comment
tobyhede
left a comment
There was a problem hiding this comment.
Review attestation (code-review, medium effort)
- Files reviewed:
.github/workflows/impact.yml,scripts/impact-gate.py,scripts/tests/test_impact_gate.py,scripts/__tests__/impact-workflow.test.mjs,.github/impact-gate/*(policies, requirements),.gitignore,docs/plans/2026-10-08-impact-gate-rollout.md - Models: Sonnet 5.5
Generated by Claude Code
| prepare(args, policy, head) | ||
| else: | ||
| report(args, policy) | ||
| except (subprocess.CalledProcessError, OSError, ValueError, KeyError, TypeError, |
There was a problem hiding this comment.
This except tuple is narrower than what the impact_gate library can raise. build_baseline and MeasureConfig.load can fail with a YAML parse error, a RuntimeError from the history walk, or a lizard failure. None of those are caught here. The job then dies with a bare traceback and no **ImpactGate analysis failed:** entry in the step summary, which is the visibility the test test_analysis_failure_is_not_hidden_by_summary is meant to guarantee. Consider catching Exception here, since main re-reports the error and returns non-zero anyway.
Generated by Claude Code
|
|
||
|
|
||
| def cli(*args: str) -> str: | ||
| return run(sys.executable, '-m', 'impact_gate', *args) |
There was a problem hiding this comment.
check_output doesn't capture stderr, so if impact_gate score exits non-zero the published message is only "returned non-zero exit status N". The tool's own error text goes to the job log and not to the step summary. Capturing stderr (for example stderr=subprocess.STDOUT, or subprocess.run with capture_output) and including it in the failure summary would make the report debuggable.
Generated by Claude Code
|
|
||
| concurrency: | ||
| group: impact-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
cancel-in-progress: true applies to main pushes too. If two merges land close together, the first run is cancelled before "Save main baselines" and no cache is written for that SHA. PRs based on it then fall back to an older ancestor cache through restore-keys, because valid_pair accepts any ancestor, and they are scored against a stale baseline. Consider cancel-in-progress: ${{ github.event_name == 'pull_request' }}.
Generated by Claude Code
| uses: actions/cache/save@0057852bfaa89a56745cba8c7296529d2fc39830 # v4 | ||
| with: | ||
| path: ${{ env.BASELINE_DIR }} | ||
| key: ${{ steps.restore.outputs.cache-primary-key }} |
There was a problem hiding this comment.
This save can fail permanently for a given SHA. If an exact-key cache exists but fails validation (policy or hash mismatch, corrupt files), REFRESH is false because cache-hit is true. prepare still rebuilds, and rebuilt=true triggers this save. GitHub refuses to overwrite the existing immutable key ("Unable to reserve cache"). The bad cache stays in place and every later run for that SHA rebuilds again. The key already hashes the policy files, so this is rare, but a corrupt entry never self-heals. Options are to delete the bad key first, or to add a run-attempt or ID suffix to the save key and match it through restore-keys.
Generated by Claude Code
| python scripts/impact-gate.py prepare --base "$BASE_REF" --cache-dir "$BASELINE_DIR" "${args[@]}" | ||
|
|
||
| - name: Save main baselines | ||
| if: github.event_name == 'push' && steps.prepare.outputs.rebuilt == 'true' |
There was a problem hiding this comment.
Pull-request runs never save a baseline. If no cache exists for the base (evicted after 7 days idle, or the main-push run was cancelled), each PR run rebuilds both scopes from scratch. The docs measure about 280 s locally for the pair, so a slow runner could use a large share of the 20-minute budget on every synchronize, and the result is discarded each time. A fallback that fails fast with a clear "no baseline yet" message, or a saved PR-side cache keyed on the base SHA, would avoid repeating this work.
Generated by Claude Code
2 participants
TL;DR: Adds advisory ImpactGate reports to PRs, scoring change impact with and without test files against Stack’s history.
Stack currently has no repository-calibrated change-impact report. This adds a read-only, advisory ImpactGate workflow with two job-summary reports: eligible source including test files, and eligible source excluding test files. Existing FTA/CRAP checks remain unchanged; impact scores do not gate merges or post PR comments.
The workflow pins ImpactGate and Lizard, builds matching baselines from 200 recent mainline entries, and caches them by measurement policy and target branch. The driver validates cached pairs, rebuilds corrupt or incompatible data, explicitly discloses seed-only grading, and preserves operational failures. Scope rules account for generated output and current/historical test layouts; reports disclose unsupported SQL/module extensions and remaining TypeScript parser defects.
Validation
Rollout
The first PR run can verify cold generation and rendered summaries. A main run saving the baseline pair and a subsequent PR restoring it remain to be verified after merge. Local timing does not establish hosted cache behavior. Evidence and reproduction details are recorded in
docs/plans/2026-10-08-impact-gate-rollout.md.Implements #1137; leave the issue open until hosted rollout verification is recorded.
Summary by CodeRabbit