Skip to content

Fixes2 - #11

Merged
wojciech-dron merged 4 commits into
masterfrom
fixes2
Mar 6, 2026
Merged

wojciech-dron merged 4 commits into
masterfrom
fixes2

Conversation

@wojciech-dron

Copy link
Copy Markdown
Owner

No description provided.

@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Privacy / prompt-injection risk:
SelectCvTemplateForOffer sends full CV template markdown content and offer description to an AI provider. Ensure this data is allowed to leave the system (PII/company confidential text), and consider limiting/sanitizing what is sent. Also note that untrusted offer descriptions can contain prompt-injection that may influence template selection; while the output is validated as a numeric ID, it can still cause incorrect selection decisions.

⚡ Recommended focus areas for review

Possible Issue

replace_text applies replacements against OriginalText and assigns the result to ModifiedText. This means multiple tool calls won't compose (each call effectively reverts to the original and reapplies only the latest replacement). Consider performing Contains/Replace on ModifiedText so iterative edits accumulate, and clarify whether you want to replace the first occurrence vs all occurrences.

[KernelFunction("replace_text")]
[Description("Replaces a chunk of text of the provided content with new text." +
    " Returns information if the text was not found or if the replacement was successful.")]
public string ReplaceText(
    [Description("The exact text chunk to find and replace")]
    string oldText,
    [Description("The new text to replace it with")]
    string newText)
{
    if (string.IsNullOrEmpty(OriginalText) || string.IsNullOrEmpty(oldText))
        return "No text to replace.";

    if (!OriginalText.Contains(oldText))
        return "[WARN] The specified text was not found in the content. No changes made.";

    ModifiedText = OriginalText.Replace(oldText, newText);

    return "Success";
}
Infinite Loop

The follow-up chat loops until the model includes the DoneSignal without a maximum number of iterations/turns. If the model never returns the signal (or returns empty content), the handler may loop indefinitely until cancellation. Add a bounded retry/turn limit and/or detect non-progress situations to fail fast with a useful error.

{
    var lastResponse = "";
    do
        await foreach (var response in agent.InvokeAsync(chatHistory, cancellationToken: ct))
            lastResponse = response.Message.Content ?? "";
    while (!lastResponse.Contains(DoneSignal));

    var chatItems = chatHistory.Select(ChatItem.From).ToList();
UI State Bug

isWorking is set to true before invoking the duplicate request, but it is not reset on the error path. If the response is an error and the method returns early, the modal can remain stuck in a "working" state (disabled UI/spinner). Ensure isWorking is reset via finally or before each early return after the async call.

isWorking = true;
StateHasChanged();

var message = new DuplicateCv.Request(OriginCvId, NewCvName, TargetOfferUrl);
var response = await MessageBus.InvokeAsync<ErrorOr<DuplicateCv.Response>>(message);

if (response.IsError)
{
    toasts.PushMessage(response.FirstError.Description, ToastType.Warning);
    return;

@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Navigate using the offer URL

The navigation query is currently interpolating the whole offer object, which will
likely produce an invalid URL and break routing. Navigate using the actual OfferUrl
value and escape it to avoid issues with special characters.

JobScraper.Web/Features/Cv/PrepareCvForOfferPage.razor.cs [202]

-navigationManager.NavigateTo($"/?offerUrl={offer}");
+navigationManager.NavigateTo($"/?offerUrl={System.Uri.EscapeDataString(offer.OfferUrl)}");
Suggestion importance[1-10]: 8

__

Why: Interpolating the whole offer object in NavigateTo($"/?offerUrl={offer}") will produce a broken/meaningless URL. Using offer.OfferUrl and escaping it fixes routing correctness, especially for URLs with special characters.

Medium
Apply replacements to current text

Replacements currently always apply to OriginalText, so multiple replace_text calls
will overwrite each other and lose prior edits. Apply replacements to ModifiedText
(the current working text) and check for matches there.

JobScraper.Web/Features/Cv/Logic/ReplaceTextPlugin.cs [32-40]

-if (string.IsNullOrEmpty(OriginalText) || string.IsNullOrEmpty(oldText))
+if (string.IsNullOrEmpty(ModifiedText) || string.IsNullOrEmpty(oldText))
     return "No text to replace.";
 
-if (!OriginalText.Contains(oldText))
+if (!ModifiedText.Contains(oldText, StringComparison.Ordinal))
     return "[WARN] The specified text was not found in the content. No changes made.";
 
-ModifiedText = OriginalText.Replace(oldText, newText);
+ModifiedText = ModifiedText.Replace(oldText, newText, StringComparison.Ordinal);
 
 return "Success";
Suggestion importance[1-10]: 7

__

Why: Using OriginalText for every replace_text call discards earlier edits, making multiple tool calls ineffective. Switching to operate on ModifiedText preserves cumulative modifications and better matches how the plugin is intended to be used.

Medium
Prevent infinite AI response loops

This can loop forever because chatHistory is never updated with the assistant
response, so repeated invocations may not progress. Invoke once, append the
assistant message to chatHistory, and fail fast if the response doesn't contain the
done signal to prevent hangs.

JobScraper.Web/Features/Cv/Logic/AiCvWithModifyConversation.cs [53-62]

 var lastResponse = "";
-do
-    await foreach (var response in agent.InvokeAsync(chatHistory, cancellationToken: ct))
-        lastResponse = response.Message.Content ?? "";
-while (!lastResponse.Contains(DoneSignal));
+await foreach (var response in agent.InvokeAsync(chatHistory, cancellationToken: ct))
+    lastResponse = response.Message.Content ?? "";
+
+if (!string.IsNullOrWhiteSpace(lastResponse))
+    chatHistory.AddAssistantMessage(lastResponse);
+
+if (!lastResponse.Contains(DoneSignal, StringComparison.Ordinal))
+    return new Response(false, null, chatHistory.Select(ChatItem.From).ToList());
 
 var chatItems = chatHistory.Select(ChatItem.From).ToList();
 var modifiedCv = replaceTextPlugin.GetModifiedText();
 
 return new Response(true, modifiedCv, chatItems);
Suggestion importance[1-10]: 4

__

Why: The current do/while can indeed spin forever if the agent never returns {DoneSignal}. However, the proposed fix removes the loop rather than bounding it (and only appends a single assistant message), which may reduce success for agents intended to iterate; a max-iteration guard + appending each assistant response would be a more accurate fix.

Low

@wojciech-dron
wojciech-dron merged commit 4e75fbb into master Mar 6, 2026
1 check passed
@wojciech-dron
wojciech-dron deleted the fixes2 branch March 9, 2026 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant