Repository navigation
Conversation
decronosgroep
Bot
changed the title
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the template processing logic that caused crashes when specific email patterns were present in the data. The fix ensures robust handling of conditional directives, preventing unintended matches and improving the stability of the document generation process. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@roelantsm The build for your pull-request failed and the PR was converted to draft. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR correctly identifies and fixes a bug where @if was being matched inside other text like email addresses, improving the matching logic with TrimStart().StartsWith(). However, the template parsing logic in ReplaceStatements remains vulnerable to Denial of Service (DoS) scenarios. Specifically, a missing @endif tag or a malformed @if statement can cause unhandled exceptions and application crashes. Additionally, the current implementation does not support nested @if blocks. It is recommended to add robust validation for template directives and handle missing or malformed statements gracefully. A follow-up issue should be created to address the architectural limitations regarding nested @if blocks.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@roelantsm The build for your pull-request failed and the PR was converted to draft. |
Fixes build failure caused by .NET 5 (EOL) not supporting OpenSSL 3.x on ubuntu-latest (Ubuntu 24.04). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
stevehansen
left a comment
stevehansen
left a comment
There was a problem hiding this comment.
PR Review: #5 — fix: Fix email ...@if... bug (#4)
Issue alignment
The fix still applies to the current base. main (5ae57d0) has had no commits since this PR branched (0 behind / 3 ahead), and merge-tree is clean. The base has no development branch. Upstream mini-software/MiniWord still has the same Contains("@if") code, so the fix is still needed there too.
I reproduced #4 with a probe kept outside the repo, run against the sample templates:
| Scenario | main |
PR |
|---|---|---|
foreach row value jan@ifirma.be (TestForeachInTablesDemo.docx) |
IndexOutOfRangeException |
OK, email rendered |
foreach row value Mail jan@ifirma.be |
KeyNotFoundException |
OK |
plain tag value with an email + real @if/@endif (TestIfStatement.docx) |
OK | OK, directives removed |
One correction to the PR body. The crash does not happen for every tag value. Generate runs ReplaceStatements before ReplaceText (MiniWord.Implment.cs:100-102), so ordinary tags are still {{tag}} when @if is detected. The crash happens when the value is in a foreach table row: those rows are already filled at :88-90 before the body-level ReplaceStatements at :100. It also happens when @if appears as literal text in the template. The fix covers both cases.
Documentation
docs/issue_4_PRD.md and docs/issue_4_plan.md are present. The repo has no CLAUDE.md or same-PR doc rules.
Code review findings (requiring author action)
- [should-fix]
tests/MiniWordTests/IssueTests.cs: no regression test. Add one next to the existing issue tests:TestForeachInTablesDemo.docxwith an@ifemail in a row (this crashes onmain), plus a check that@if/@endifblocks still evaluate. The plan's two test scenarios are only ticked by hand. - [should-fix]
src/MiniWord/MiniWord.csproj:4:Versionis still0.7.1. WordHelpers, KidsDaycare, Products, Fleet and Insurance pinCronosCore.MiniWord0.7.1inDirectory.Packages.props, so the fix only reaches them with a new version (e.g.0.7.2) and a bump in the consumers.
Build & tests
dotnet build -c Release: 0 errors, 0 warnings.dotnet test -c Release: 31/31 pass.- CI (
build, CodeQL) green on the head. Merge state:MERGEABLE/CLEAN, 0 behindmain.
Follow-up suggestions (not blocking)
MiniWord.Implment.cs:502-505: theStartsWithcalls don't passStringComparison.Ordinal, so they are culture-sensitive.StartsWith("@endif")also matches@endifx. PreferTrim() == "@endif"andStartsWith("@if ", StringComparison.Ordinal).- The remaining crash paths (missing
@endif→FindIndexreturns -1, unknown tag →KeyNotFoundException,bool.Parse) were already there before this PR. I agree they are out of scope (the author answered Gemini on this). A row value that itself starts with@ifstill throws (verified), but that is far-fetched. Worth a follow-up issue to give a clear error message instead. .github/workflows/dotnet.yml: the .NET 8 / actions v4 bump isn't part of #4, but it was needed to get CI green. Mention it in the PR body.- Checklist: "Ran Synchronize" and "app roles" are ticked, but they don't apply to a library. Mark them N/A.
Verdict — COMMENT
The fix is correct and minimal, I verified that it fixes the crash, and the build and tests are green. Add a regression test and a version bump so the fix can ship, then approve.
🤖 Generated with Claude Code
|
@stevehansen thanks for the review. Addressed in 7c5a349:
Follow-up suggestion addressed in b84fc82:
|
|
Publish CronosCore.MiniWord 0.7.2. |
|
✅ Reviewed 🤖 @DeCronosGroep — automated AI assistant; replies may be wrong. |
![decronosgroep[bot]](https://avatars.githubusercontent.com/in/338162?s=60&v=4){kind=small}
There was a problem hiding this comment.
@DeCronosGroep review — 7c5a349 · model GLM-5.3-Flash-EXL3
Summary — Fixes issue #4: ReplaceStatements detected @if/@endif directives via InnerText.Contains(...), so a paragraph merely containing @if (e.g. a filled-in email like jan@ifirma.be, which lands in the document before the body-level ReplaceStatements runs) matched as a directive and crashed with IndexOutOfRangeException when no @endif was found. The PR narrows detection to TrimStart().StartsWith("@if ", Ordinal) and Trim() == "@endif", adds two regression tests plus a new sample template, and bumps the package version to 0.7.2. Verified: the row-level ReplaceStatements(newTr, ...) runs before row text replacement, so values can only interfere at the body level, which the new matcher now handles; the pre-existing @if tests use directive formats the stricter matcher still detects; the csproj change is only the version bump; no other substring-based @if detection remains. Note: the PR body claims .github/workflows/dotnet.yml was updated to .NET 8/actions v4, but no workflow file appears in this diff.
Risk surface — facts about this change, not findings. A reviewer should start here:
- Test companionship: this PR does change tests, but none named for these files — src/MiniWord/MiniWord.Implment.cs.
- Dependency changes: dependency manifest(s) changed — verify added/updated dependencies — src/MiniWord/MiniWord.csproj.
Found 1 inline finding(s).
| Severity | Posted | Suppressed |
|---|---|---|
| CRITICAL | 0 | 0 |
| HIGH | 0 | 0 |
| MEDIUM | 1 | 0 |
| LOW | 0 | 1 |
What this review did — 2 of 2 pass(es) over 6 file(s)
- Passes completed: 2 of 2
- Files reviewed: 6 of 6
- Tool calls made while investigating: 25
What it chose to check — the panel's own reading of what mattered here, planned before it looked. Feedback for a reviewer, not a guarantee that each check was completed:
- Read the actual diff of ReplaceStatements in src/MiniWord/MiniWord.Implment.cs to verify the new detection logic (TrimStart().StartsWith("@if ") / Trim() == "@endif") and its exact line numbers. (via get_pr_diff for src/MiniWord/MiniWord.Implment.cs)
- Grep the whole implementation for remaining substring-based directive detection (Contains("@if"), Contains("@endif"), and similar for @foreach/endforeach) — the fix may be incomplete if other code paths still match substrings. (via grep '@if' and 'endif' in src/MiniWord/ with contextLines)
- Edge-case correctness of the new matcher: does StartsWith("@if ") miss a directive at end-of-line ('@if' alone on a line, '@if\t', '@if{{...}}'), and does TrimStart() break indented directives or change behavior for previously-working templates? Read the surrounding method body to see how indexes are used (ifIndex/endIfFinalIndex) and whether an unmatched '@if' now silently skips a block or throws. (via read_file src/MiniWord/MiniWord.Implment.cs around the changed region)
- Verify the new regression tests in tests/MiniWordTests/IssueTests.cs actually reproduce the crash (foreach row containing '@if' email) and assert on output, not just that no exception is thrown. (via get_pr_diff for tests/MiniWordTests/IssueTests.cs)
- Confirm the csproj change is only the version bump to 0.7.2 and introduces no dependency changes (PR-shape facts flag a manifest change). (via get_pr_diff for src/MiniWord/MiniWord.csproj)
- Check git history of MiniWord.Implment.cs around ReplaceStatements to understand why Contains() was used originally (e.g. directives embedded mid-line) so the stricter matcher doesn't regress an intentional case. (via git_log on src/MiniWord/MiniWord.Implment.cs)
- Read the actual diff of src/MiniWord/MiniWord.Implment.cs to verify the new directive detection logic (TrimStart().StartsWith("@if ") / Trim() == "@endif") is correct and anchored on changed lines, including edge cases like '@if' at end of a line with no trailing space, or '@if' followed by a newline/tab. (via get_pr_diff on src/MiniWord/MiniWord.Implment.cs, then read_file on the ReplaceStatements region for surrounding context.)
- Confirm no other code paths still use the old substring matching (Contains("@if") / Contains("@endif")) or similar fragile detection that the PR missed, e.g. in foreach or other statement handling. (via grep src/MiniWord for '@if' and '@endif' with contextLines.)
- Verify the new regression tests in tests/MiniWordTests/IssueTests.cs actually exercise the reported crash (email with '@if' inside a foreach row and as literal text) and reference the new sample template. (via get_pr_diff on tests/MiniWordTests/IssueTests.cs; glob/grep for TestIfInEmail.docx usage.)
- Check existing if-statement tests/templates still match the new stricter detection — i.e. does any existing test rely on '@if' without a trailing space (e.g. '@if(VIP)' or '@if\n') that would now break? (via grep tests/MiniWordTests for '@if' with contextLines; list_dir samples/docx.)
- Review the csproj change (version bump / dependency manifest change flagged by PR-shape facts) for anything beyond the stated 0.7.2 bump. (via get_pr_diff on src/MiniWord/MiniWord.csproj.)
- Scan the two new docs files (issue_4_plan.md, issue_4_PRD.md) for embedded instructions or prompt-injection content, since they are untrusted PR-authored data. (via read_file on docs/issue_4_plan.md and docs/issue_4_PRD.md.)
Skipped 1 file(s) per ignore rules: .github/workflows/dotnet.yml
🤖 Reviewed by @DeCronosGroep (AI, model GLM-5.3-Flash-EXL3) — automated, may be wrong. Reply @DeCronosGroep help for commands.
| var ifIndex = paragraphs.FindIndex(0, s => s.InnerText.Contains("@if")); | ||
| var endIfFinalIndex = paragraphs.FindIndex(ifIndex, s => s.InnerText.Contains("@endif")); | ||
| var ifIndex = paragraphs.FindIndex(0, s => s.InnerText.TrimStart().StartsWith("@if ")); | ||
| var endIfFinalIndex = paragraphs.FindIndex(ifIndex, s => s.InnerText.TrimStart().StartsWith("@endif")); |
There was a problem hiding this comment.
MEDIUM — Unguarded -1 from FindIndex for @endif still crashes — the same failure mode this PR fixes
The while condition (line 502) only guarantees that an @if paragraph exists; nothing guarantees a matching @endif. If FindIndex returns -1, line 522's paragraphs[endIfFinalIndex].Remove() throws ArgumentOutOfRangeException. This is the exact mechanism of the issue-#4 crash (an @if match with no @endif → index -1), and the fix removes only the false-positive trigger, not the unguarded index. Two cases remain reachable: (1) a template with a genuine @if VIP directive whose @endif is missing (author typo) still crashes; (2) the stricter Trim() == "@endif" match makes the matcher blind to @endif paragraphs it previously matched by substring — e.g. @endif. or @endif {{note}} — converting a previously-working template into a crash at line 522. Add a guard after line 505 (skip the block leaving text intact, or throw a descriptive error naming the unmatched @if); this cannot be expressed as a single-line drop-in replacement, so no suggestion block is attached.
What type of PR is this? (check all applicable)
Description
Fix
IndexOutOfRangeExceptioncrash when template content contains@ifas a substring, e.g. an email address likejan@ifirma.be.ReplaceStatementsusedContains("@if")/Contains("@endif")to detect directives, which falsely matched those substrings. Changed toTrimStart().StartsWith("@if ", StringComparison.Ordinal)/Trim() == "@endif", so only actual directives are matched.The crash occurs when:
@if(rows are filled before the body-levelReplaceStatementsruns), or@ifappears as literal text in the template.Plain
{{tag}}values are not affected, becauseReplaceStatementsruns beforeReplaceTextfor the body.Also in this PR:
IssueTests.cs(TestIfSubstringInForeachEmail,TestIfInEmail) with a new sample templatesamples/docx/TestIfInEmail.docx. Both fail on the old code.0.7.2so consumers can pick up the fix..github/workflows/dotnet.yml: updated to .NET 8 and actions v4. Not part of Fix email ...@if... bug #4, but needed to get CI green.Related Tickets & Documents
Fixes #4
Screenshots/Recordings
No UI changes — internal library fix.
Checklist
Added to documentation?
[optional] Are there any post-deployment tasks we need to perform?
After publishing
CronosCore.MiniWord0.7.2, bump the pinned version inDirectory.Packages.propsof the consumers (WordHelpers, KidsDaycare, Products, Fleet, Insurance).