Repository navigation
Conversation
Signed-off-by: Srinivasa Dinakar Jainti <dinakarjs@gmail.com>
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
rajnisht7
left a comment
There was a problem hiding this comment.
the changes and direction looks good, isolation is solid, fresh venv, helper copied in, python -I, PYTHONPATH/PYTHONHOME stripped, and __file__ checked under sys.prefix. That cleanly stops source-tree import leakage. The behavioral checks (valid/invalid fixtures, golden event construction, version) are a clear step up from the old minimal smoke
One thing: the description says this exercises all six valid and seven invalid fixtures, but the harness only requires each category to be non-empty. both check_controls and the unit tests accept any count > 0, so a deleted/renamed/moved fixture still lets the smoke pass with reduced coverage. test_empty_controls_are_caught only catches the zero case
kindly tighten that so the claimed inventory is actually enforced, either exact counts (valid == 6, invalid == 7) or, better, the expected filenames, and add a regression test that a partially missing set is rejected.
A few smaller notes (non-blocking):
- The version regex on pyproject.toml works today but is brittle to whitespace/quoting changes.
- Unit tests cover the helper well; the full smoke_wheel.py path is only exercised when the script runs (CI does this on 3.12). That’s fine, just worth being clear about the layering.
- Controls come from the source checkout by design, so this proves installed behavior matches current source controls, not that the artifact embeds the exact same fixture set. The PR already scopes this correctly.
Prepared with OpenAI Codex. Signed-off-by: Srinivasa Dinakar Jainti <dinakarjs@gmail.com>
|
@rajnisht7 please re-review cb6c0d7. Exact fixture filenames now enforce all six valid/seven invalid controls, with missing, renamed and duplicate regressions. 164 tests and both installed wheel/sdist smoke paths pass; restoring the old helper fails all six new inventory subcases. This remains installed behavior against source controls, not embedded-fixture verification. Updated description records validation and limits. Prepared with OpenAI Codex. |
|
@rajnisht7 following up for your re-review and disposition of the fixture-inventory finding at cb6c0d7. The published correction enforces the exact six valid/seven invalid filenames and rejects partial, renamed or duplicate inventories. The description records 164 passing tests, both installed wheel/sdist smoke paths, and six inventory subcases that fail with the old helper restored. I have just rechecked this head: CI and CodeQL succeeded; the inapplicable Dependabot auto-merge workflow was skipped. Please confirm whether your requested change is satisfied or identify remaining corrections. Formal reviewer assignment returned 403, so this conversation comment is the re-review request. Prior local validation is cited, not a fresh suite run. Prepared with OpenAI Codex. |
Extend the wheel/sdist consumer smoke check to validate installed import origin/version, six valid and seven invalid source controls, and deterministic event construction against the shared Python/npm golden control. The helper runs with Python -I in a fresh environment outside the checkout.
The expected filenames are now enforced exactly. Missing, renamed, duplicate and empty controls fail; a nonempty subset can no longer silently reduce coverage.
Validation on Linux/Python 3.12: fresh hash-locked test/release dependencies; 164 tests pass with process-local OTEL_TRACES_SAMPLER=always_on. Built wheel and sdist each pass the full smoke path in separate fresh environments, including all 13 fixtures and golden construction. Six helper tests cover accept-all/reject-all, changed construction and inventory faults. Restoring the old helper makes all six partial/renamed/duplicate inventory subcases fail. Changed-file lint and whitespace pass.
Controls come from the source checkout: this proves installed behavior against source controls, not that artifacts embed those fixtures. Consumer installs resolve runtime dependencies afresh; this is not locked deployment certification. No SDK runtime/schema or release permission changes.
Prepared with OpenAI Codex. Signed-off-by: Srinivasa Dinakar Jainti dinakarjs@gmail.com.
Independent current-head review and hosted CI remain required; prior-head CI is historical.
Current head: cb6c0d7. Published tree d14d829251b65edf134ef918a4dbabd8668a64a2 matches the locally validated source tree.
Hosted validation update: all returned current-head PR workflows completed successfully (the inapplicable Dependabot auto-merge workflow was skipped). Relevant runs: workflow 41166473023, workflow 41166473025. This workflow query is not a complete branch-protection inventory. Independent re-review has been requested in the PR conversation and remains pending.