Repository navigation
fix(python): inspect imported initialization callables - #897
yashrajp22 wants to merge 2 commits into
Conversation
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Value and readiness
The code is ready from static review: imported bindings loaded during initialization now retain their selected callable bodies. The PR description explicitly requests a release-inclusion hold; preserve that hold until inclusion is requested.
Material findings
No required code changes found. The description explicitly says this release correction should remain unmerged until release inclusion is requested.
PIC tradeoffs
Loaded module values and uncertain callbacks can conservatively leave analysis incomplete; that is disclosed and preferable to granting a false safe ownership proof. Unused imports and selected harmless helpers retain their narrower treatment.
Verification and gaps
Reviewed the complete diff, description, linked issue material, all previous reviews/comments/replies and pinned-head surrounding source at c04b9c37adca05b93f95e2737fa7afc9e1c60841. Traced alias binding, dotted imports, initialization stubs, recursive inspection and existing file/depth/node budgets. Reviewed all 30 new static regression cases, including the ledger checks and harmless controls. No GitHub status checks are reported for this head; author-reported test runs are not independently reproduced. No contributor code, tests, imports, build steps or scanner invocations were executed during this review.
An imported setup function can replace a harmless output function with a shell executor. The release branch currently skips that imported function body, allowing the scan to report SAFE with complete analysis when it should retain CAUTION and incomplete analysis.
This change follows imported names used during module initialization, including aliases, callbacks, defaults and decorators. It reuses the existing bounded import analysis and preserves harmless setup, unused imports and selected safe helpers beside unrelated functions.
This PR targets the v2.13.1 release branch because the affected import checks were added there while integrating #849. It is a separate correction for #852 and should remain unmerged until release inclusion is requested.
Known precision limit: reading a value from a module with unrelated shell-capable functions, or retaining uncertain nested callback code, can now leave analysis incomplete. The module-constant case was reproduced separately. Narrowing these cases requires additional attribute and control-flow analysis.
Validation: 30 new regression cases; 1,742 targeted tests passed in source and fresh installed-wheel runs. All 13 focused skills passed across base/head and source/wheel (52 CLI scans), including unsafe setup returning CAUTION/incomplete and harmless setup staying SAFE/complete. Fresh wheel/sdist package checks and Ruff lint/format passed. All 12 catalog skills also matched across base/head and source/wheel (48 more CLI scans). These were offline checks with no LLM provider. CodeRabbit and Greptile completed final-commit reviews with no new findings; the earlier conservative precision findings remain documented above.