hardening(p7): P0 sweep 2026-04-23 — sanitizer parity, ConfigureAwait, null-guards, ProviderName + 0.16.1#95
Conversation
…, null-guards, ProviderName; bump to 0.16.1 Fix #92: GeminiProvider.ExtractProductInfoAsync and AnalyzeReferenceLinkAsync now wrap all user-controlled text (d.Id, d.Text, url, html, context fields) with PromptSanitizer.EscapeIdentifierForPrompt / EscapeForPrompt, matching the existing GptService path (S-12 prompt-injection defence parity). Fix #93: ConfigureAwait(false) applied to every await expression across all 12 library source files (GptService.*, GeminiProvider, WebTools, FallbackSearchProvider). CA2007 promoted to error via .editorconfig + AnalysisLevel=latest-Recommended in Agency.csproj to prevent regression. P7-04 (#94): ArgumentException.ThrowIfNullOrWhiteSpace(conversationText) added at the top of GenerateTitleAsync on both GptService and GeminiProvider, matching the existing guard on systemPrompt. P7-07 (#94): ApiUsageEvent call sites in GptService.ReferenceLink (line 87) and GptService.StudioMint (line 120) changed from hardcoded "openai" to this.ProviderName so custom-endpoint providers (Groq, MiniMax, etc.) report correctly. Version bump: 0.16.0 → 0.16.1. Tests: 708 green (658 pre-existing + 50 new from this branch). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review @cyberchacha @MintSurf — friendly ping, this PR has been awaiting review for ~30 min. Summary:
708 tests pass, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5458d1a8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| var documentBlock = string.Join("\n\n", documents.Select(d => | ||
| $"--- DOCUMENT: {d.Id} ---\n{d.Text}\n--- END ---")); | ||
| $"--- DOCUMENT: {PromptSanitizer.EscapeIdentifierForPrompt(d.Id)} ---\n{PromptSanitizer.EscapeForPrompt(d.Text)}\n--- END ---")); |
There was a problem hiding this comment.
Keep Gemini document IDs canonical for provenance matching
Escaping d.Id here can change canonical IDs (for example & to &), but TryParseProductInfo later compares returned SourceDocuments to the original raw IDs via knownIds.Contains(...) without HTML-decoding. In calls where document IDs are URLs or other strings containing escapable characters, Gemini can legitimately echo the escaped ID it was shown, causing the match to fail and dropping provenance for valid extraction results. Please preserve round-trippable IDs (or decode before validation) so source attribution is not lost.
Useful? React with 👍 / 👎.
|
Thanks @chatgpt-codex-connector. Declining the change to keep OpenAI ↔ Gemini parity: The exact same On the round-trip concern: If IDs ever did contain HTML-special characters, the fix belongs in |
Summary
No commit was created and no follow-up PR was opened, because there were no file changes to commit. Checks
|
MintSurf
left a comment
There was a problem hiding this comment.
Review Summary
- Outcome: comment
- Summary: The hardening changes look directionally correct, but the PR bumps the NuGet version to 0.16.1 without the matching CHANGELOG entry that this repo treats as the authoritative consumer-facing release record. I also could not run the optional project checks here because
dotnetis not installed in this environment, and there is no*.sln/Node/Flutter manifest matching the requested check gates.
| <!-- NuGet Package --> | ||
| <PackageId>ShareInvest.Agency</PackageId> | ||
| <Version>0.16.0</Version> | ||
| <Version>0.16.1</Version> |
There was a problem hiding this comment.
Missing 0.16.1 changelog entry
This PR bumps the package version to 0.16.1, but CHANGELOG.md still tops out at 0.16.0. Per the repo instructions, every release needs a changelog entry and the latest entry is the authoritative source for NuGet consumers. Shipping the version bump without that release note leaves consumers and downstream repos without an accurate record of what 0.16.1 contains.
Useful? React with 👍 / 👎.
cyberchacha
left a comment
There was a problem hiding this comment.
Review Summary
- Outcome: approve
- Summary: Clean, well-scoped hardening PR. All four findings (sanitizer parity, ConfigureAwait, null-guards, ProviderName) are correctly implemented. Build passes with 0 errors and CA2007 enforcement active. All 708 tests green (including 50 new). Mechanical changes are consistent and complete across all call sites.
|
|
||
| <!-- Analysis: CA2007 (ConfigureAwait) as error to prevent regression --> | ||
| <AnalysisLevel>latest-Recommended</AnalysisLevel> | ||
| <TreatWarningsAsErrors>false</TreatWarningsAsErrors> |
There was a problem hiding this comment.
TreatWarningsAsErrors=false — verify intent
AnalysisLevel=latest-Recommended enables a broad set of analyzers. With TreatWarningsAsErrors=false, only CA2007 (via .editorconfig) is promoted to error — the remaining ~120 warnings (CA1848, CA1861, CA1305, etc.) stay as warnings. If that's intentional, this is fine. If the goal is to gradually tighten, consider adding a tracking issue to promote additional rules over time rather than leaving them as non-blocking.
Useful? React with 👍 / 👎.
|
|
||
| var documentBlock = string.Join("\n\n", documents.Select(d => | ||
| $"--- DOCUMENT: {d.Id} ---\n{d.Text}\n--- END ---")); | ||
| $"--- DOCUMENT: {PromptSanitizer.EscapeIdentifierForPrompt(d.Id)} ---\n{PromptSanitizer.EscapeForPrompt(d.Text)}\n--- END ---")); |
There was a problem hiding this comment.
Good: sanitizer parity now matches GptService path
Using EscapeIdentifierForPrompt for d.Id and EscapeForPrompt for d.Text correctly mirrors the defense-in-depth pattern from the OpenAI path. The <user_input> wrapping inside --- DOCUMENT --- delimiters is a clean layered approach.
Useful? React with 👍 / 👎.
Per repo convention every NuGet version bump must land with a matching CHANGELOG section. Added a Keep-a-Changelog entry for 0.16.1 covering: - Security: GeminiProvider PromptSanitizer parity (#92) - Changed: ConfigureAwait(false) mechanical rollout + CA2007 error-level (#93) - Fixed: GenerateTitleAsync null-guard (#94 P7-04) + ApiUsageEvent.ProviderName (#94 P7-07) - Notes: P5 Models.csproj bump to 0.16.1 + redeploy required Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed MintSurf P2 on P2 — missing CHANGELOG 0.16.1 entry: added a Keep-a-Changelog section for 0.16.1 covering Security (#92 GeminiProvider sanitizer parity), Changed (#93 ConfigureAwait + CA2007 error), Fixed (#94 P7-04 null-guard + P7-07 ProviderName), and a Notes row flagging the required Also declined the Codex P2 (Gemini ID escape parity) inline above — divergence from the OpenAI path would strictly worsen prompt-injection defense; fix belongs in @MintSurf — could I get your approval once CHANGELOG looks good? |
Summary
GeminiProvider.ExtractProductInfoAsync+AnalyzeReferenceLinkAsyncnow usePromptSanitizer.EscapeForPrompt/EscapeIdentifierForPrompt(parity with GptService path)ConfigureAwait(false)applied to every library await (mechanical, 12 files);CA2007promoted to error via.editorconfig+AnalysisLevel=latest-Recommendedto prevent regressionGenerateTitleAsyncnull-guard (ArgumentException.ThrowIfNullOrWhiteSpace(conversationText)) on bothGptServiceandGeminiProviderApiUsageEventproviderName now usesthis.ProviderNameinstead of hardcoded"openai"at ReferenceLink + StudioMint call sitesTest plan
GenerateTitleAsyncnull/empty/whitespace throwsArgumentExceptionon both providers (6 theory cases each)ApiUsageEventProviderName propagation (groq, minimax, custom)dotnet build -c Releaseclean with CA2007 enforcing (0 errors)dotnet testgreen: 708 total (658 pre-existing + 50 new)Closes #92, closes #93. Absorbs P7-04 + P7-07 from #94 (remaining sub-items tracked separately).
Post-merge: publish
ShareInvest.Agency.0.16.1.nupkgto nuget.org, then bumpModels.csprojin P5 to<PackageReference Include="ShareInvest.Agency" Version="0.16.1" />and redeploy P5.🤖 Generated with Claude Code