Repository navigation
fix(ci): compare rebased copies against their PR base - #195
Conversation
Signed-off-by: Harry Lee <harrli@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (21)
🧰 Additional context used📓 Path-based instructions (4)Only root workflows are active.⚙️ CodeRabbit configuration file Files:
Require coverage of the changed behavior and its negative or boundary cases.⚙️ CodeRabbit configuration file Files:
Read REVIEW.md before commenting.⚙️ CodeRabbit configuration file Files:
Before making any change under: `python/aisimulate/src/aiconfigurator/generator/**` MUST read: `python/aisimulate/.claude/rules/generator-development.md` Before making any change under `python/aisimulate/collector/**` MUST read: `python/ais...📄 CodeRabbit inference engine (AGENTS.md) Files:
🪛 ast-grep (0.45.3)tests/test_ci_workflow_contracts.py[error] 584-601: Command coming from incoming request (subprocess-from-request) [error] 783-790: Command coming from incoming request (subprocess-from-request) 🔀 Multi-repo context ai-dynamo/dynamo, ai-dynamo/aiconfiguratorLinked repositories findingsai-dynamo/dynamo
ai-dynamo/aiconfigurator
📝 SummaryRisk: Medium. Human review should focus on:
Changed behavior:
Public contracts changed:
Technical evidence:
Merge readiness:
WalkthroughFull CI verifies and exports the trusted PR base SHA. Fast CI receives that SHA, fetches it when needed, and checks whitespace across the complete PR range. Contract tests cover stacked PR metadata, repository history, and fail-closed behavior. ChangesTrusted base CI flow
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The trusted base propagation and complete-range whitespace validation changes have no identified current-head merge risk. 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (7 passed)
Full details: Review EvidenceExplanation The description reports 206 passing contract tests, negative cases for rebases, stacked bases, malformed bases, API failure, and missing history, plus a parity statement and hosted Fast CI/Full CI statuses. The changed workflows show that root Fast CI is active for both pull requests and workflow calls, and Full CI is active for trusted-copy push branches. No inactive nested workflow is cited as passing hosted evidence. However, the description does not name exact commands or clearly classify local, fake-runner, parity, and production evidence, so the positive evidence requirement cannot be confirmed. Comment |
Harrilee
added
the
review-ready
ChatGPT Codex Connector
Arsene12358
left a comment
Arsene12358
left a comment
There was a problem hiding this comment.
Verdict: approve — no hard blockers. One P2 (F1) I'd like fixed or explicitly deferred before merge; S1–S4 are non-blocking notes.
Verification
I traced the change from the copy-branch push through verify-target → whitespace-base → the reusable Fast CI input → BASE_SHA → git diff --check, and re-ran the suites on 68433bad.
- Hosted evidence for the bug: run 34788614489 (
pull-request/121@69901699) ran the whitespace step withBASE_SHA=e1dff098…(the previous copy head) and failed withfatal: Invalid symmetric difference expression …(exit 128), which tookFast CI SuccessandFull CI Successdown with it. - Hosted evidence for the fix: the stacked copies used their own PR base, a value neither the old expression nor the
comparison-basefallback could produce — 34792674185 (pull-request/122) ran withBASE_SHA=7252deba…(#121's head) and 34792673388 (pull-request/190) withBASE_SHA=56ac9f86…(#122's head). Thepull-request/121run usedmain's tip because #121 still targetedmainwhen it started (it was retargeted 45 s later). This PR's own copy, 34792677350, finished green. - Local: the policy + contract + qualification suites collect 206 cases and all 11 new cases pass (also under bash 3.2). Ruff check/format,
git diff --check origin/main...HEAD, copyright and packaged-legal checks are clean; actionlint reports only the pre-existing SC2016 info in the readiness step (same onmain). - No regressions on the other entry points:
main/release/*pushes still compare againstevent.before(viacomparison-base), standalonepull_requestruns still usepull_request.base.sha(inputs.*is empty outsideworkflow_call), and the only new value that reaches a shell ispr_base, validated as 40-hex.
F1 (P2): the absent-base fetch cannot authenticate on the hosted runner
The description says "fetch absent base history before checking the complete PR range", and test_fast_ci_whitespace_fetches_an_absent_base_without_truncating_history[fetch-base-history] covers it. On GitHub that path fails every time it is reached: the repository is internal, the python-static checkout runs with persist-credentials: false (its log shows Removing auth unsetting http.https://github.com/.extraheader at the end of the checkout step), and git fetch --no-tags origin "${BASE_SHA}" carries no credentials. collector-check.yml runs the identical fetch and injects github.token as a basic-auth extraheader for exactly this reason (and that fetch succeeds in the same Full CI run). The new test passes only because its origin is a file:// path, which never asks for credentials.
This is not a regression — an absent base already failed with exit 128 — and in every hosted run so far the base was a branch tip already present in the fetch-depth: 0 clone, so the fetch never executed. But it is a shipped path that a test certifies and the description advertises, and the case where it matters (an orphaned .base.sha) is exactly when a clear outcome is needed. Two acceptable fixes: mirror collector-check.yml's token injection (inline suggestion on fast-ci.yml, plus a contract test pinning the header so it cannot silently disappear), or drop the fetch, the fetch-base-history parameterization, and the sentence in the description — for trusted copies the base is always reachable from a branch head, so the check stays correct without it.
Non-blocking notes
- S1 —
whitespace-basefalls back tocomparison-base, which ismain's tip for non-lifecycle runs (ci.yml L95-98).workflow_dispatchruns and the first push of a newrelease/*branch therefore now checkmain...targetinstead ofTARGET^..TARGET; for a dispatched stacked head that includes the parent PR's commits. Probably fine (tree-level diff, and every PR already passes this gate on itspull_requestevent), but it is undocumented — either note it or narrow the output tosteps.pr-target.outputs.base_shaso the other triggers keep their previous behavior. - S2 — the new helpers execute step scripts with
bash -c; GitHub runsshell: bashsteps with-eo pipefail. Verdicts coincide today (a failed fetch is followed by a failinggit diff), so this is a fidelity nit. - S3 — the body doesn't follow the PR template (Review map / risk level / Evidence fields / Tracking). REVIEW.md ties the Codex-review requirement to the declared tier and CodeRabbit rated this Medium; maintainers' call. The hosted runs above are the end-to-end evidence CodeRabbit said was missing and would be worth citing.
- S4 — after this merges, #121 needs to go back to
mainas planned (GitHub does it automatically if this branch is deleted on merge).
Signed-off-by: Harry Lee <harrli@nvidia.com>
|
@Arsene12358 Addressed F1 and S1–S2 in c08ebd5b, with details in each inline thread. Local CI/policy suites: 209 passed. Fast CI and CODEOWNERS passed on this exact commit; Full CI is queued. For S3, the description now follows the repository template, declares medium risk, and distinguishes current-head results from the earlier hosted runs. It also states the local missing-base test's transport limitation. For S4, the tracking section explicitly retains the post-merge step to ensure #121 targets main. This update only changes #195; the runtime stack has not been rebased or retargeted here. |
2 participants
Why and what changed
Trusted
pull-request/*copies trigger Full CI through push events. After a rebase,push.beforecan point to an unreachable previous copy head. The whitespace gate then fails withInvalid symmetric difference expression, preventing the runtime suites from starting.Resolve the originating PR's head and base together, retain exact-head verification, and pass the validated PR base to Fast CI. Fetch missing base history using a temporary, read-only GitHub-token header before checking the complete PR range. The token is not persisted. Other triggers keep their existing whitespace ranges: ordinary main/release pushes use
event.before; manual runs outside trusted-copy refs and new release branches retain the last-commit fallback. Collector and prediction comparison bases are unchanged.Review map
.github/workflows/ci.yml(verify-target→ Fast CI input), then.github/workflows/fast-ci.yml(BASE_SHA→ authenticated fetch → whitespace check). Tests are intests/test_ci_workflow_contracts.pyand the existing.github/codeowners/test_aisimulate_policy.pymock.base_shareusable-workflow input.persist-credentials: false, and full-range whitespace validation. Reverting this change restores the rebased-copy failure. Changes are limited to two workflows and two CI test files.Evidence
c08ebd5b8a338f8d6ed85b4117b213c092d3a642.python -m pytest -c /dev/null .github/codeowners/test_*.py tests/test_ci_workflow_contracts.py tests/test_ci_qualification.py -q -p no:cacheprovider --override-ini='addopts=' --override-ini='filterwarnings='— 209 passed.ruff check --config python/aisimulate/pyproject.toml tests/test_ci_workflow_contracts.py,ruff format --check --config python/aisimulate/pyproject.toml tests/test_ci_workflow_contracts.py,git diff --check,python3 scripts/check_copyright.py, andpython3 scripts/check_packaged_legal_files.pypassed.c08ebd5b; CODEOWNERS also passed.c08ebd5b. Previous revision68433badpassed on its trusted copy; that result does not certify this follow-up commit.68433bad14cde57a8cf1ab0ccae0aab29b6be1fe; new-head review pending.c08ebd5b8a338f8d6ed85b4117b213c092d3a642— author-side call-chain and regression review; reviewer recheck of the follow-up remains pending.Modeling or data provenance
N/A — no simulation model, performance data, or product-output changes.
Tracking
main. feat(replay): stabilize deterministic agentic runtime contract #121 currently targets this branch; feat(replay): report deterministic AgentX play outcomes #122 and feat(replay): qualify public AgentX M1 execution #190 remain stacked above it.