Skip to content

feat(report): add LLM analysis provenance - #556

Open
rng1995 wants to merge 18 commits into
mainfrom
naren/fix-NVCARPS-150-semantic-provenance
Open

rng1995 wants to merge 18 commits into
mainfrom
naren/fix-NVCARPS-150-semantic-provenance

Conversation

@rng1995

@rng1995 rng1995 commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

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_provenance contract 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

  • Record the configured and resolved adapters plus the provider proven by response telemetry.
  • Record resolved model and SkillSpector analyzer revision for every LLM-backed analyzer.
  • Record validated temperature, seed, and reasoning-effort requests, their sources, adapter support, and whether a real provider call established that they were forwarded.
  • Record safe Azure deployment/API-version routing without endpoints or credentials.
  • Mark LLM analysis as nondeterministic; static scans are not_applicable.
  • Use explicit states for provider defaults, partial/best-effort controls, invalid configuration, unknown configuration, and calls with no provider response evidence.
  • Sanitize the final fixed-field projection so prompts, endpoints, credentials, and arbitrary state cannot enter the public report.

Key files

  • src/skillspector/llm_provenance.py
  • src/skillspector/nodes/build_context.py
  • src/skillspector/nodes/report.py
  • src/skillspector/inference_usage.py
  • src/skillspector/providers/chat_models.py
  • Provenance, provider, report, and boundary regression tests

Validation

  • make lint format-check test-ci
    • Ruff lint and format checks passed.
    • 4,916 tests passed, 14 skipped, 4 expected failures, 38 deselected.
  • Focused provenance/report/provider coverage: 304 passed, 9 skipped.
  • pytest -m integration tests/ with live external agent CLIs excluded from PATH: 28 passed, 7 skipped.
  • Wheel and source distribution built successfully with local build dependencies.
  • twine check passed for both artifacts.
  • Independent contract review found no remaining issues after malformed-input and no-response cases were covered.

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

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
@rng1995
rng1995 force-pushed the naren/fix-NVCARPS-150-semantic-provenance branch from d43e3fa to de2920b Compare September 16, 2026 04:30

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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:

  1. Successful counter-less responses, including all current agent-CLI adapters, are removed before effective-provider provenance is derived. Those scans therefore report effective_adapter: unknown even though SkillSpector knows which provider answered.
  2. forwarded_to_client is 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.

Comment thread src/skillspector/llm_provenance.py Outdated
Comment thread src/skillspector/llm_provenance.py Outdated
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 chrisknvidia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Validated inline review findings.

Comment thread src/skillspector/providers/chat_models.py
Comment thread src/skillspector/llm_provenance.py
Comment thread src/skillspector/nodes/report.py Outdated
Comment thread src/skillspector/llm_provenance.py
Comment thread src/skillspector/llm_provenance.py Outdated
@rng1995

rng1995 commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

Completed the second review pass at head 1d3fea5. All five new findings are addressed and their threads are resolved:

  • record requested controls separately from normalized request-payload controls, including the gpt-5.4 temperature omission
  • preserve nv_inference controls
  • classify provenance from actual execution state
  • add sanitized package plus source revision identity without invoking Git
  • reject credential-shaped values across capture, inference usage, forwarded controls, and public reports

Validation:

  • 535 focused provider/provenance/report tests passed, 9 skipped
  • 5,826 full unit tests passed, 14 skipped, 4 expected xfails
  • full Ruff lint and format checks passed
  • git diff --check passed

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.

Comment thread src/skillspector/nodes/report.py Outdated
Comment thread src/skillspector/inference_usage.py Outdated

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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:

  1. report.py treats LLM enablement as execution. A verified all-not_applicable semantic 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.
  2. 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.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[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 yashrajp22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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"))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants