Skip to content

ci: close license evidence streams and audit supported Python versions - #241

Merged
jasonqinzhou merged 3 commits into
mainfrom
codex/pr59-review-followups-20260917
Sep 17, 2026
Merged

jasonqinzhou merged 3 commits into
mainfrom
codex/pr59-review-followups-20260917

Conversation

@jasonqinzhou

@jasonqinzhou jasonqinzhou commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

After #59 merged, Full CI failed both application-contract shards because the nightly license-evidence generator left its Cargo metadata file open. Close file, HTTP response, and archive-entry streams explicitly; exercise the same warnings-as-errors policy in Fast CI so resource leaks fail before the expensive matrix.

This also addresses the review comments posted after #59 merged:

  • Audit runtime dependency licenses across Python 3.11, 3.12, and 3.13 on amd64 and arm64, with distinct inventory artifacts.
  • Exercise the pinned pip-licenses consumer with permitted and forbidden metadata, plus workspace exclusion and conflicting-license evidence cases.
  • Verify the GitLab multipart handoff against release-automation revision 1aaaeae1f4a29345085b68bb460d09ee47cc4bb0; execute the real trigger script with a fake HTTP client to check fields, credentials, and HTTP failure propagation.
  • Correct the optional security-job diagram and document the conservative audit-tool inventory and existing public-log reporting policy.

Current validation at ef70c3da190f2ddb9567f8cc2b7f80109a8d4f5e, incorporating main c3cc3109: 220 workflow/qualification/release tests pass under root pytest.ini with warnings treated as errors. Fast CI is green, CodeRabbit substantively reviewed all six files at this head with no actionable findings, and current Full CI passed. Simone Chen approved this exact head. Squash-merged as 6c07147abccca1a28fa5db15fa8b2fa9b2af55cf; the merged tree was verified identical to the tested PR head.

Integration with the corrected #250 and #251 is conflict-free and passes 304 Python tests plus 132 subtests. Historical validation at 45f6c0c3 included 325 local repository-policy/workflow tests and all 54 Full CI jobs; those hosted results do not replace current-head CI. These checks do not certify a scheduled nightly or a live GitLab scan/publication.

Observed failure: https://github.com/ai-dynamo/aisimulate/actions/runs/35167311868

The release environment configuration prerequisite documented by #59 remains unchanged. No deployment approval or environment protection change is included.

Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 17, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 87bf8726-91aa-4a5a-bc7e-2862d0931642

📥 Commits

Reviewing files that changed from the base of the PR and between 45f6c0c and ef70c3d.

📒 Files selected for processing (6)
  • .github/workflows/ci.yml
  • .github/workflows/fast-ci.yml
  • .github/workflows/nightly-ci.yml
  • docs/ci.md
  • scripts/check_python_licenses.py
  • tests/test_ci_workflow_contracts.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • ai-dynamo/dynamo (manual)
  • ai-dynamo/aiconfigurator (manual)

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (5)
Only root workflows are active.

⚙️ CodeRabbit configuration file

Files:

  • .github/workflows/ci.yml
  • .github/workflows/nightly-ci.yml
  • .github/workflows/fast-ci.yml
Require coverage of the changed behavior and its negative or boundary cases.

⚙️ CodeRabbit configuration file

Files:

  • tests/test_ci_workflow_contracts.py
Check commands, defaults, supported runtimes, public names, and claims against executable behavior.

⚙️ CodeRabbit configuration file

Files:

  • docs/ci.md
Read REVIEW.md before commenting.

⚙️ CodeRabbit configuration file

Files:

  • tests/test_ci_workflow_contracts.py
  • docs/ci.md
  • scripts/check_python_licenses.py
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:

  • tests/test_ci_workflow_contracts.py
  • docs/ci.md
  • scripts/check_python_licenses.py
🪛 ast-grep (0.45.3)
tests/test_ci_workflow_contracts.py

[info] 1886-1894: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"workspace_members": list(workspace_members),
"packages": [
{"id": f"getrandom@{version}", "name": "getrandom", "version": version, "license": spdx}
for version, spdx in crates
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.

(use-jsonify)


[info] 1924-1924: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.

(use-jsonify)


[error] 2056-2071: Command coming from incoming request
Context: subprocess.run(
[
sys.executable,
"-m",
"piplicenses",
"--with-system",
"--packages",
"license-policy-fixture",
"--allow-only",
python_licenses.ALLOWED_LICENSES,
],
cwd=tmp_path,
env={**os.environ, "PYTHONPATH": str(tmp_path)},
text=True,
capture_output=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(subprocess-from-request)


[error] 2242-2260: Command coming from incoming request
Context: subprocess.run(
["bash", "-e", "-c", step["run"]],
cwd=tmp_path,
capture_output=True,
text=True,
env={
**os.environ,
"PATH": f"{fake_bin}:{os.environ['PATH']}",
"CAPTURE_REQUEST": str(capture),
"CURL_EXIT_CODE": "22" if failure == "http-error" else "0",
"GITLAB_TRIGGER_TOKEN": "" if failure == "missing-token" else "fixture-token",
"GITLAB_PIPELINE_URL": endpoint,
"WHEEL_VERSION": "0.12.0.dev20260917",
"GH_RUN_ID": "123456",
"GH_SHA": "a" * 40,
"SLACK_THREAD_TS": "1234567890.123456",
"SLACK_CHANNEL_ID": "fixture-channel",
},
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(subprocess-from-request)

scripts/check_python_licenses.py

[error] 43-48: Command coming from incoming request
Context: subprocess.run(
[*command, "--allow-only", ALLOWED_LICENSES],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(subprocess-from-request)

🪛 LanguageTool
docs/ci.md

[style] ~374-~374: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...EASE_TYPE|security/nightly| |NIGHTLY_TAG|nightly-YYYYMMDD-<first-seven-c...

(ADVERB_REPETITION_PREMIUM)

🔀 Multi-repo context ai-dynamo/dynamo, ai-dynamo/aiconfigurator

Linked repositories findings

ai-dynamo/dynamo

  • Existing compliance consumers require artifacts named compliance-<git-sha>-<container>; nightly builds explicitly use the non--nightly suffix for OSRB attachment and baseline lookup. [::ai-dynamo/dynamo::]
  • The upload action and audit workflow independently construct this same artifact name, so the PR’s new license-evidence artifacts should not reuse or alter this namespace. .github/actions/compliance-extract/action.yml:408-412; .github/workflows/shared-compliance-audit.yml:108-111. [::ai-dynamo/dynamo::]
  • No downstream references to pip-licenses or the new Python license matrix were found. [::ai-dynamo/dynamo::]

ai-dynamo/aiconfigurator

  • No license-audit, pip-licenses, or GitLab security-handoff consumers were found. The only GitLab reference was unrelated collector-pipeline documentation. [::ai-dynamo/aiconfigurator::]

📝 Summary

Risk: medium. Human attention should focus on:

  1. Nightly license audits across Python 3.11–3.13 and amd64/arm64.
  2. Explicit cleanup of Cargo metadata, HTTP response, ZIP entry, and CSV streams.
  3. GitLab security-trigger and GitHub Pages workflow contracts.

Changes

  • Pin pip-licenses==5.5.5 in CI contract jobs.
  • Treat Python warnings as errors in Fast CI contract tests.
  • Expand nightly license audits across supported Python versions and architectures.
  • Add contract tests for stream cleanup, license policy, matrix alignment, metadata conflicts, and GitLab trigger failures.
  • Document audit-tool inventory, public-log behavior, GitLab handoff requirements, and Pages dependencies.

Evidence

  • Fast CI and all 54 Full CI jobs passed.
  • The reported strict-warning regression suite passed 325 tests.
  • Integrated validation passed 304 Python tests plus 132 subtests, 12 workflow-script Node tests, Actionlint, and Ruff.
  • Additional reported checks covered whitespace, source headers, packaged legal files, and documentation links.
  • The requested repository diff produced no visible output, so the implementation details rely on the supplied change summary.
  • No current review severity counts were supplied. Bot review results do not constitute approval.

Missing evidence

  • No scheduled nightly run, live GitLab scan, or public publication is certified.
  • No deployment approval or environment-protection change is included.
  • Production behavior outside the reported CI and contract tests is not established.

Merge readiness

Technical validation is reported as passing. Merge readiness is incomplete because independent CODEOWNER approval is still required and GitHub reports REVIEW_REQUIRED.

Walkthrough

The change updates CI license enforcement, expands nightly license checks across Python versions and architectures, manages license-report resources explicitly, and documents and tests Pages and GitLab security-trigger contracts.

Changes

CI license and security contracts

Layer / File(s) Summary
License policy and audit contract
.github/workflows/ci.yml, .github/workflows/fast-ci.yml, scripts/check_python_licenses.py, tests/test_ci_workflow_contracts.py
CI installs pip-licenses==5.5.5, treats contract-test warnings as errors, documents audit scope, and validates the license policy.
Nightly license inventory and resource handling
.github/workflows/nightly-ci.yml, tests/test_ci_workflow_contracts.py
Nightly license checks cover Python 3.11–3.13 on both architectures. Cargo metadata and prior license artifacts use managed resources. Tests cover workspace exclusion, conflicting metadata, and cleanup.
Security trigger and Pages workflow contract
docs/ci.md, tests/test_ci_workflow_contracts.py
Pages trigger conditions and the GitLab security request contract are documented. Tests cover successful requests, missing credentials, HTTP errors, fields, endpoints, and generated tags.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to ef70c

The CI license and security-trigger changes have no identified merge-blocking risk.

🚥 Pre-merge checks | ✅ 8
✅ Passed checks (8 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cross-Layer Contract ✅ Passed No cross-layer contract failure is introduced. The changed surface is CI workflow and license-evidence plumbing, not a Rust, CLI, or public serialization API. The Python license pin is aligned in the …
Modeling And Data Evidence ✅ Passed PASS: The pull request changes CI workflows, license-audit handling, documentation, and workflow contract tests. It does not change formulas, model or performance data, selection logic for model/data …
Compatibility Boundaries ✅ Passed The PR changes only CI workflows, CI documentation, license-audit comments, and CI contract tests. No Python or Rust source, manifests, lockfiles, binding definitions, or public schemas changed. The u…
Review Evidence ✅ Passed The PR description names relevant checks and results, including the strict-warning test suite, Ruff, Actionlint, pip-licenses, hosted Fast CI, and hosted Full CI. It includes negative and boundary c…
Title check ✅ Passed The title precisely describes the main behavioral changes: explicit license-evidence stream closure and auditing across supported Python versions. It avoids vague terms.
Description check ✅ Passed The description is detailed and directly covers the problem, behavior changes, review scope, validation evidence, compatibility limits, and approval status. It does not reproduce every template headin…

Comment @coderabbitai help to get the list of available commands.

@jasonqinzhou
jasonqinzhou marked this pull request as ready for review September 17, 2026 01:00
@jasonqinzhou
jasonqinzhou requested review from a team as code owners September 17, 2026 01:00
@jasonqinzhou jasonqinzhou added the review-ready Ready for automated and human review label Sep 17, 2026

@jasonqinzhou jasonqinzhou left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex review at 45f6c0c against main 920453e: inspected the complete follow-up diff, six-way license matrix and artifact aggregation, real pip-licenses policy consumption, warnings-as-errors cleanup, and GitLab request/error paths. No remaining actionable finding in this diff. Local validation: 325 tests passed with warnings treated as errors, plus Ruff, Actionlint, legal/header and documentation checks. Full CI, current CodeRabbit, and independent CODEOWNER approval are still required. The existing release environment configuration remains a separate operational prerequisite.

@jasonqinzhou jasonqinzhou changed the title fix(ci): close compliance streams and finish PR 59 review corrections ci: close license evidence streams and audit supported Python versions Sep 17, 2026
@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

Current-head review is complete at 45f6c0c: Codex review found no remaining actionable issue, CodeRabbit reviewed all six changed files with no actionable comments, and Fast CI passed. The title now states the changed behavior. Admitting this exact commit to Full CI through the normal NVIDIA runner gate; independent CODEOWNER approval remains required.

@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

/ok to test 45f6c0c

@jasonqinzhou

Copy link
Copy Markdown
Contributor Author

Final validation at 45f6c0c: trusted pull-request/241 copy matches the PR head; Fast CI and all 54 Full CI jobs passed, including the amd64 and arm64 contract shards that failed after #59. CodeRabbit reviewed all six changed files with no actionable comments. Local strict-warning regression suite: 325 passed. Main remains 920453e and no code change followed review. Ready for independent CODEOWNER review; GitHub currently reports REVIEW_REQUIRED. Pavithra and the owning teams are requested. No deployment is approved by this validation.

Copy link
Copy Markdown
Contributor Author

Closeout dependency check at 45f6c0c: the corrected #250 (07a6b70) and #251 (fb2e93b) both combine cleanly with this PR. The integrated tree passes 304 Python tests plus 132 subtests under root pytest.ini warnings-as-errors, 12 workflow-script Node tests, actionlint, and Ruff. In particular, this PR eliminates the three pre-existing license-evidence stream warnings reproduced on the current main-derived #250/#251 branches. Existing exact-head Full CI is green. @pvijayakrish, the remaining gate here is independent CODEOWNER approval; this account authored #241 and cannot self-approve it. No bypass or merge has been performed.

Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>

@jasonqinzhou jasonqinzhou left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refreshed Codex review at ef70c3d after merging current main c3cc310. The final six-file PR diff is unchanged from the previously reviewed 45f6c0c; the merge only adds the current base's performance changes. No new actionable finding. Rechecked Python-version inventory identities, warning-sensitive resource closure, the real pip-licenses consumer, and GitLab request contract; 220 workflow/qualification/release tests pass under root pytest.ini warnings-as-errors at the new head. Prior hosted Full CI belongs to 45f6c0c and is historical evidence only; the updated head needs its own Fast/CodeRabbit/Full CI and independent CODEOWNER approval.

Copy link
Copy Markdown
Contributor Author

/ok to test ef70c3d

@jasonqinzhou
jasonqinzhou merged commit 6c07147 into main Sep 17, 2026
67 checks passed
@jasonqinzhou
jasonqinzhou deleted the codex/pr59-review-followups-20260917 branch September 17, 2026 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-ready Ready for automated and human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants