Conversation
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
d43e3fa to
de2920b
Compare
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Manual Review Needed — code changes are required, and this PR is authored by the authenticated reviewer, so this is a COMMENT review rather than a self-requested-changes review.
Reviewed head: 296df4097e78fbb4e9781e09d5591403836c19f8
Two provenance claims are not yet supported by the recorded evidence:
- Successful counter-less responses, including all current agent-CLI adapters, are removed before effective-provider provenance is derived. Those scans therefore report
effective_adapter: unknowneven though SkillSpector knows which provider answered. forwarded_to_clientis inferred from a captured environment value, adapter support, and any provider response. A response does not prove the earlier captured value was the value supplied when the client was later constructed.
Please preserve sanitized provider-response observations independently of token counters and record the actual sanitized controls used at client construction/invocation. Add report-level regression coverage for a successful counter-less/CLI response and for configuration changing between capture and client construction.
Merge gates also open:
- GitHub reports merge conflicts (
CONFLICTING/DIRTY); rebase and re-review the resulting head. - GitHub reports no CI/check results for this head.
- Independent approval is required because the author and authenticated reviewer are both
rng1995.
Priority: P0 — customer-facing semantic provenance must be trustworthy for reproducibility analysis.
Preserve counter-less provider response evidence, derive forwarded sampling controls from constructed clients, and merge the current main branch. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
chrisknvidia
left a comment
There was a problem hiding this comment.
Validated inline review findings.
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
…rovenance' into pr556-clean Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
…rovenance' into pr556-clean Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
|
Completed the second review pass at head 1d3fea5. All five new findings are addressed and their threads are resolved:
Validation:
GitHub reports the PR mergeable with no conflicts. Exact-head CI is running after the latest automatic main refresh; independent approval is still required because the authenticated account is the PR author. |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Manual Review Needed — code changes remain required
Re-reviewed current head a02d6d71abb692988183d23b28e976dc66a319c4 against the prior marked review, all resolved and current threads, the complete provenance/provider/report diff, report-level tests, and current checks.
The original counter-less response and construction-evidence findings are addressed, as are the earlier adapter-support, source-revision, and preflight-disabled paths. Two current P1 correctness/security findings remain:
report.pytreats LLM enablement as execution. A verified all-not_applicablesemantic pass with no calls is still reported as nondeterministic execution with unknown configuration. Derive execution from attempt/response evidence or verified analyzer outcomes and pin the no-work report case.- The generic credential heuristic remains fail-open for authorization/JWT-shaped values and overbroad for valid long model IDs. Use field-specific validation or opaque identifiers and add serialized-report regressions proving secrets cannot escape while legitimate model identifiers remain useful.
The authenticated reviewer rng1995 is also the PR author, so this COMMENT cannot request changes or approve. Five exact-head checks pass and test-unit is still running; the unresolved findings, independent approval requirement, and GitHub BLOCKED state prevent merge.
Priority: P0 — public provenance must not misstate execution or expose credential-like inputs.
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Manual Review Needed — no remaining required code change found
Re-reviewed current head 9523e3314028118f2ac885325ab8cb56464faf48 after the final synchronization with settled main d162d9b343e559be13df8ebba093df3bc9d58c90. I inspected the complete provenance diff, the authored remediation delta, prior resolved findings/threads, report execution state, credential/model boundaries, and the merged-main interaction.
Both outstanding P1 findings are resolved. Provenance execution now requires observed attempts or provider-response evidence, preserving not_applicable / not_applied for verified no-work semantic passes while retaining failed attempts and counter-less successes. Public fields now use field-specific validation: reasoning effort is an enum, Azure deployment/API version have narrow grammars, authorization and compact JWT/JWE shapes are rejected, and legitimate long hyphenated model identifiers remain observable. Report-level tests exercise environment capture, raw-state revalidation, serialized non-leakage, and valid-model preservation. I found no remaining required code, test, documentation, security, or compatibility changes.
The authenticated reviewer rng1995 is also the PR author, so this COMMENT is not an independent approval. GitHub currently reports no checks and mergeStateStatus=UNSTABLE; exact-head CI and independent approval remain required.
Priority: P0 — trustworthy, non-secret semantic provenance is a public reporting boundary.
yashrajp22
left a comment
There was a problem hiding this comment.
One remaining reasoning-effort provenance mismatch is reproduced on this head; details are inline.
Validation: fresh base/head wheels and installed-package identity checks; 20 baseline and 50 HEAD test cases passed. All 48 offline corpus scans matched across source/wheel and base/head after accounting for the additive metadata. Existing partial-coverage results remain partial. SDK checks use mocked HTTP responses, so this does not establish live-provider behavior or overall security accuracy.
| output_config = output_config if isinstance(output_config, Mapping) else {} | ||
| for name in selected: | ||
| if name == "reasoning_effort": | ||
| controls[name] = payload.get(name, output_config.get("effort")) |
There was a problem hiding this comment.
Could you also read the Responses API’s nested reasoning.effort here? With SKILLSPECTOR_PROVIDER=openai, SKILLSPECTOR_MODEL=gpt-5.4-pro, and SKILLSPECTOR_REASONING_EFFORT=high (temperature/seed unset), the locked SDK sends reasoning={"effort":"high"} to /v1/responses. I reproduced that with a mock HTTP response, but the report then shows forwarded_to_client: null and controls_partially_forwarded. Reading the nested value and adding a Responses regression case would keep the provenance aligned with the request.
Summary
NVCARPS-150 tracks inconsistent LLM-backed scan results that could not be explained from the report. The existing report showed findings and token usage, but not the resolved provider/model configuration or sampling controls used by each semantic analyzer.
This change adds a strict, additive
metadata.llm_provenancecontract and explicitly reports whether LLM analysis can be reproduced deterministically.Root cause
SkillSpector resolved provider, model, and optional sampling configuration at runtime but did not retain that configuration in scan output. Provider sampling controls also do not guarantee identical generated output. As a result, two successful scans could differ in semantic findings without enough evidence to distinguish a configuration change from normal provider variation.
Solution
nondeterministic; static scans arenot_applicable.Key files
src/skillspector/llm_provenance.pysrc/skillspector/nodes/build_context.pysrc/skillspector/nodes/report.pysrc/skillspector/inference_usage.pysrc/skillspector/providers/chat_models.pyValidation
make lint format-check test-cipytest -m integration tests/with live external agent CLIs excluded fromPATH: 28 passed, 7 skipped.twine checkpassed for both artifacts.Compatibility and risk
The JSON change is additive and does not alter finding or scoring behavior. Existing consumers can ignore the new metadata. Sampling values remain optional, and the report does not claim deterministic output even when controls are present. Seed validation is limited to the portable signed 64-bit range and invalid values fail before model construction.
Rollback is a straight revert of this commit. A downstream SkillEvaluator change will retain and export this provenance after the SkillSpector version is updated.
Tracking: NVCARPS-150; related public report: #334