Skip to content

ci: LLM code review#1429

Merged
kirillzyusko merged 22 commits intomainfrom
ci/ai-review
Apr 10, 2026
Merged

ci: LLM code review#1429
kirillzyusko merged 22 commits intomainfrom
ci/ai-review

Conversation

@kirillzyusko
Copy link
Copy Markdown
Owner

@kirillzyusko kirillzyusko commented Apr 10, 2026

📜 Description

Run Ollama DeepSeek:14b model to review changes by LLM.

💡 Motivation and Context

This is basic PoC implementation:

  • use only prompt;
  • use thinking model;
  • use model under 16GB;
  • include diff for context;
  • include original file content for context.

Future improvements:

  • include predefined context (like Android/iOS skills);
  • use whole repo for context;

This baseline doesn't show really good review output but acts more like a "second" eyes. Let's enable if tor now and see how it behaves. Things that I like is that I can literally customize many aspects of this flow (context construction, prompts, model selection, etc.) and don't pay for 3rd party LLM integration.

Let's see how far can it go.

📢 Changelog

CI

  • add job for PR reviews with LLM help;

🤔 How Has This Been Tested?

Tested via this PR. DeepSeek showed the best results so far.

📸 Screenshots (if appropriate):

image

📝 Checklist

  • CI successfully passed
  • I added new mocks and corresponding unit-tests if library API was changed

@kirillzyusko kirillzyusko self-assigned this Apr 10, 2026
@kirillzyusko kirillzyusko added the CI CI (github actions) label Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

📊 Package size report

Current size Target Size Difference
312992 bytes 312992 bytes 0 bytes 📉

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

llama3 model

Bugs:

  • The ollama run command is not properly formatted, it may cause errors.
  • The git fetch origin main and git diff commands are unnecessary, as t�[1D�[K
    the workflow is running on a self-contained environment.

Performance:

  • The sleep 5 command is unnecessary and can be removed to improve workfl�[6D�[K
    workflow speed.
  • The curl -fsSL https://ollama.com/install.sh | sh command can be optimi�[6D�[K
    optimized by using a package manager like apt or yum instead of curling the�[3D�[K
    the installation script.
  • The workflow runs for an extended period while waiting for Ollama to serv�[4D�[K
    serve, this can be improved by running Ollama in the background and checkin�[7D�[K
    checking its status later.

Bad Practices:

  • Hardcoding API keys, GitHub tokens, or other sensitive information in the�[3D�[K
    the workflow file is not secure. Consider using environment variables or a �[K
    secrets manager.
  • The actions/checkout@v4 action does not need to specify fetch-depth. �[K
    This can be removed for better performance.
  • The workflow does not handle errors properly. It would be better to use t�[1D�[K
    try-catch blocks or error handling mechanisms to ensure the workflow handle�[6D�[K
    handles unexpected errors.

Recommendations:

  1. Refactor the workflow to separate concerns and reduce complexity.
  2. Use environment variables instead of hardcoding sensitive information.
  3. Improve error handling and logging to ensure the workflow is robust.
  4. Optimize Ollama installation and running for better performance.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

mistral

  1. Title: Incorrect assumption about the existence of a file 'diff.patch'
    Why: The script assumes that diff.patch has been created before it is �[K
    used in the command. However, the creation of this file is dependent on the�[3D�[K
    the success of the previous steps and might fail if there are any issues du�[2D�[K
    during the Git diff process.
    Fix: Add error handling to check if diff.patch is created successfully�[12D�[K
    successfully before using it in the command.

  2. Title: Broken CI command due to hardcoded Ollama installation script URL�[3D�[K
    URL
    Why: The CI command uses a hardcoded URL for installing Ollama, which�[5D�[K
    which might break if the URL changes or becomes unavailable.
    Fix: Use a more reliable method of installing dependencies, such as addi�[4D�[K
    adding Ollama as a dependency in package.json and using npm or yarn to in�[2D�[K
    install it.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

mistral + new prompt

  1. (Title) Incorrect assumption about the existence of the 'main' bran�[4D�[K
    branch
    Why: The script assumes that the local repository and the remote one�[3D�[K
    one have a 'main' branch. However, it's possible for the local repository t�[1D�[K
    to not have the 'main' branch yet or for the remote repository to use anoth�[5D�[K
    another main branch name (like 'master' or custom branch names).
    Fix: Before running the script, ensure that the local and remote rep�[3D�[K
    repositories both have a 'main' branch. If they don't, update accordingly.

  2. (Title) Incorrect usage of shell command '&' in the "Start Ollama" s�[1D�[K
    step
    Why: The '&' symbol is used to run commands in the background. Howev�[5D�[K
    However, if the following steps (Pull model, Get diff, Run LLM review) enco�[4D�[K
    encounter any errors, they will not be captured because they are running in�[2D�[K
    in the background. This could lead to unnoticed issues.
    Fix: Consider removing the '&' symbol from the "Start Ollama" comman�[6D�[K
    command to ensure that all following steps run sequentially and potential e�[1D�[K
    errors are properly handled.

LGTM for the rest of the code.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

Gemma4 + thinking

Thinking...
The user wants me to act as a senior engineer performing a STRICT code revi�[4D�[K
review on the provided GitHub Actions workflow file (.github/workflows/ai-�[23D�[K (.github/workflows/ai-review.yml`).
I must follow strict rules:

  1. Output MAX 2 issues.
  2. Only report REAL, HIGH-CONFIDENCE issues.
  3. Ignore style, naming, formatting, and micro-optimizations.
  4. Ignore generic best practices.
  5. DO NOT suggest improvements unless it's a real bug.
  6. If no real issues — output EXACTLY: LGTM.
  7. If unsure — output: LGTM.

Let's analyze the workflow step by step.

Workflow: ai-review.yml

  1. Checkout: Uses actions/checkout@v4 with fetch-depth: 0. This i�[1D�[K
    is standard and safe.

  2. Install Ollama: Installs Ollama using curl ... | sh. This looks �[K
    like the standard installation script and should work on ubuntu-latest.

  3. Start Ollama:

    - name: Start Ollama
      run: |
        ollama serve &
        sleep 5

    Issue: Running ollama serve & and then immediately running subsequen�[9D�[K
    subsequent steps (like ollama pull gemma4) within the same run block do�[2D�[K
    does not guarantee that the service is ready or that the environment variab�[6D�[K
    variables/processes are correctly managed for subsequent steps. Although s�[2D�[K sleep 5 is used, it's brittle. More critically, backgrounding the service�[7D�[K service (&) means the step will continue immediately, but subsequent oll�[4D�[K
    ollama commands rely on the service being active and reachable, which sho�[3D�[K
    should ideally be managed by ensuring the service is foregrounded or waited�[6D�[K
    waited upon correctly, or using a dedicated service start action. However, �[K
    in a standard GitHub Actions run block, background processes started with�[4D�[K
    with & are usually detached from the step's lifecycle, and the shell migh�[4D�[K
    might exit before the service is fully stable or accessible to subsequent s�[1D�[K
    steps that rely on it.

  4. Pull model:

    - name: Pull model
      run: |
        ollama pull gemma4

    This step depends on the service started in the previous step. If the se�[2D�[K
    service start in step 3 fails to keep the service running reliably or expos�[5D�[K
    exposes the necessary socket/endpoint for subsequent commands, this step wi�[2D�[K
    will fail.

  5. Get diff:

    - name: Get diff
      run: |
        git fetch origin main
        git diff origin/main...HEAD > diff.patch

    This assumes the PR source branch is relative to origin/main. This is �[K
    generally correct for a pull_request trigger.

  6. Run LLM review:

    - name: Run LLM review
      run: |
        RESPONSE=$(ollama run gemma4 "
        # ... prompt content ...
        $(cat diff.patch)
        ")

    Critical Issue: When using $() substitution inside a multi-line stri�[4D�[K
    string passed to ollama run, shell variable expansion occurs before the�[3D�[K
    the outer command executes. The structure is: RESPONSE=$(ollama run gemma4�[6D�[K gemma4 "PROMPT_START $(cat diff.patch) PROMPT_END").
    The content $(cat diff.patch) executes before ollama run is called�[6D�[K
    called. However, ollama run expects the entire prompt string to be passed�[6D�[K
    passed as a single quoted argument. Since diff.patch contains newlines an�[2D�[K
    and potentially special characters, directly embedding it inside the prompt�[6D�[K
    prompt string literal without careful quoting might lead to shell parsing i�[1D�[K
    issues or, more likely, the shell will execute the substitution and append �[K
    the content, but the resulting single string passed to ollama run might i�[1D�[K
    incorrectly interpret embedded shell syntax if the prompt itself contains q�[1D�[K
    quotes.

    A safer way to pass the content of a file (diff.patch) into a complex �[K
    prompt string is to construct the prompt programmatically or use cat as p�[1D�[K
    part of the input stream redirection/command substitution, not just simple �[K
    string concatenation within the shell script context.

    If I test the shell behavior:

    # Assume diff.patch contains: "test diff"
    RESPONSE=$(ollama run gemma4 "Hello, here is the diff: $(cat diff.patch)�[11D�[K

diff.patch)")

The shell executes `$(cat diff.patch)` first, replacing it with `test di�[2D�[K
diff`. The command becomes:
```bash
ollama run gemma4 "Hello, here is the diff: test diff"

This appears functionally correct for simple text, but it makes the co�[2D�[K
code less robust, and if the LLM prompt was meant to treat the diff content�[7D�[K
content as part of the text input to the LLM, this substitution method is�[2D�[K
is acceptable but inherently complex.

The most glaring bug related to execution flow is in Step 3 and **St�[4D�[K
Step 4 regarding service lifecycle management.

Focusing on the most critical bug (High Confidence):

  1. Service Dependency Bug (Start/Pull): The workflow starts the ollama�[7D�[K ollama serve process in the background (&) and then waits for 5 seconds�[7D�[K seconds. Subsequent steps (ollama pull, ollama run`) assume the service �[K
    is ready and running, but the workflow does not explicitly wait for the ser�[3D�[K
    service to be fully accessible, nor does it manage the foreground process l�[1D�[K
    lifecycle, making the subsequent commands brittle and likely to fail or tim�[3D�[K
    timeout under real-world CI load variation.

Revisiting the goal constraints: I must find a real, high-confidence �[K
bug.

The background process management in Step 3 is a classic race condition/lif�[13D�[K
condition/lifecycle bug in CI YAML.

Formulating the issue:

The workflow starts Ollama's server in the background and then proceeds imm�[3D�[K
immediately. While a sleep is used, the subsequent commands rely on a run�[3D�[K
running service that may not be fully initialized or exposed before the nex�[3D�[K
next command executes. For reliable execution, the service should ideally b�[1D�[K
be run in a way that allows subsequent steps to reliably connect to it, per�[3D�[K
perhaps by foregrounding the server or using a dedicated service setup meth�[4D�[K
method that manages the lifecycle.

I will report the service lifecycle issue.


Self-Correction/Refinement: Should I combine the service start and pullin�[6D�[K
pulling into one job block or use a service mechanism? Yes. The current set�[3D�[K
setup is prone to intermittent failure due to race conditions between slee�[5D�[K sleep` and service readiness.

I will report the service management bug.
...done thinking.

  1. Service race condition on Ollama lifecycle
    Why: The ollama serve & command starts the Ollama background server, �[K
    and the step then only waits for a fixed 5 seconds using sleep 5. This cr�[2D�[K
    creates a race condition. Subsequent steps (ollama pull gemma4 and ollam�[6D�[K ollama run gemma4`) assume the service is fully initialized and accessible�[10D�[K
    accessible on the expected network/socket by the time they run. If the serv�[4D�[K
    service initialization takes longer than the arbitrary wait time, or if the�[3D�[K
    the background process fails silently, the workflow will fail unpredictably�[13D�[K
    unpredictably with connection errors.
    Fix: The service lifecycle should be managed more robustly. Instead of �[K
    backgrounding the server and sleeping, the best practice is to run the serv�[4D�[K
    server process in the foreground, potentially using a dedicated service mec�[3D�[K
    mechanism if available, or implementing a health check loop that polls the �[K
    service status until it reports ready.

diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.�[30D�[K
b/.github/workflows/ai-review.yml
--- a/.github/workflows/ai-review.yml
+++ b/.github/workflows/ai-review.yml
@@ -13,13 +13,11 @@
- name: Install Ollama
run: |
curl -fsSL https://ollama.com/install.sh | sh

   - name: Start Ollama
     run: |
  •      ollama serve &
    
  •      sleep 5
    
  •      ollama serve
    
     - name: Pull model
       run: |
    

@github-actions
Copy link
Copy Markdown
Contributor

  1. Incorrect JSON Payload Construction
    Why: The PROMPT variable is defined across multiple lines and contains special characters (newlines, markdown formatting) from the YAML structure. When this variable is substituted into the jq command using jq -n --arg prompt "$PROMPT", shell expansion often fails to properly escape all necessary characters for the resulting JSON payload, leading to malformed JSON or incorrect prompt interpretation by the LLM service.
    Fix: Construct the JSON payload string explicitly using shell variables or structure the YAML to minimize complexity/use tools designed for JSON embedding (e.g., printf %q for escaping, or handling the multi-line prompt outside of the immediate run block).

  2. Incorrect API Context Usage
    Why: In the Comment PR step, the usage of the spread operator {...context.repo, ...} when calling github.rest.issues.createComment is incorrect. The context.repo object cannot be spread directly into the API parameters. The required values (owner and repo name) must be accessed and passed explicitly as strings.
    Fix: Replace {...context.repo, ...} with explicit context variables: owner: context.repo.owner, repo: context.repo.repo.

diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml
--- a/.github/workflows/ai-review.yml
+++ b/.github/workflows/ai-review.yml
@@ -70,9 +70,9 @@
const fs = require('fs');
const body = fs.readFileSync('review.txt', 'utf8');

         github.rest.issues.createComment({
  •          ...context.repo,
    
  •          issue_number: context.issue.number,
    
  •          body
    
  •          owner: context.repo.owner,
    
  •          repo: context.repo.repo,
    
  •          issue_number: context.issue.number,
    
  •          body: body
           });
    

@kirillzyusko kirillzyusko marked this pull request as ready for review April 10, 2026 15:47
@github-actions
Copy link
Copy Markdown
Contributor

null

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

[Gemma4]

  1. Service Process Termination
    Why: Running ollama serve & starts the Ollama service in the background and immediately continues the script. In many CI/CD environments, background processes started this way are vulnerable to being terminated or failing to maintain state, potentially causing the subsequent curl request in the 'Run LLM review' step to fail due to connection loss or service unavailability.
    Fix: The service must be started and kept alive in the foreground until all dependent steps (specifically the 'Run LLM review' step) are complete.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

[ChatGPT]

What caused the regression?
The change you made to KeyboardAvoidingView.tsx removed the guard that only updates the frame state when the component’s behavior is not "height":

- if (behavior !== "height") {
-   frame = { x: left, y: top, width, height: bottom };
- }

Because the component has no behavior prop, the condition was always evaluated as true in the original code.
After the removal, frame is updated only when bottom > 0 (i.e. the keyboard is open).
If the keyboard is closed first (so bottom is 0) and the user subsequently opens the keyboard, the bottom value becomes positive but the frame value has not been updated – it stays at its initial value of 0. The subsequent interpolation uses frame.value.height as an input range:

const bottom = interpolate(
  bottom,
  inputRange: [0, frame.value.height],  // <- frame.value.height is 0
  outputRange: [0, 1],
  extrapolateRight: Extrapolate.CLAMP,
);

With a 0 input range the interpolation fails and the component throws a runtime error.
This is the regression you’re seeing.


Proposed fix

Keep the original guard or make the interpolation robust to a frame.value.height of 0.
The simplest, safest fix is to restore the original guard so that the frame is only recomputed when the behavior is not "height".

// KeyboardAvoidingView.tsx
onLayoutWorklet((x, y, width, height, bottom, top, left) => {
  // The original guard – keep it!
  if (behavior !== "height") {
    frame = { x: left, y: top, width, height: bottom };
  }
  // ... rest of the code unchanged
});

If you want to keep the more concise version, add a fallback to avoid zero‑height interpolation:

// KeyboardAvoidingView.tsx
onLayoutWorklet((x, y, width, height, bottom, top, left) => {
  // always update frame, but avoid zero height
  if (bottom > 0) {
    frame = { x: left, y: top, width, height: bottom };
  }

  // Guard against zero height in interpolations
  const h = frame.value.height || 1; // avoid 0
  const bottom = interpolate(
    bottom,
    inputRange: [0, h],
    outputRange: [0, 1],
    extrapolateRight: Extrapolate.CLAMP,
  );
  // ... rest of the code unchanged
});

Either solution will stop the regression by ensuring that the interpolation never receives a zero‐height input range.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

[DeepSeek]

Here are the key issues identified:

  1. Incorrect Condition in onLayoutWorklet:
    • Problem: The onLayoutWorklet function's condition was modified to remove a check against the behavior type. This could cause the initial frame to be set incorrectly for certain behaviors, leading to layout or animation issues.
    • Why: When using "height" behavior, setting initialFrame.value during layout might interfere with dynamic height adjustments by the keyboard.
    • Fix: Re-institute the condition to only run when the behavior is not "height". This ensures that initialFrame is set correctly based on the component's intended behavior.
diff --git a/src/components/KeyboardAvoidingView/index.tsx b/src/components/KeyboardAvoidingView/index.tsx
index ec2ee2961c..c58fba5831 100644
--- a/src/components/KeyboardAvoidingView/index.tsx
+++ b/src/components/KeyboardAvoidingView/index.tsx
@@ -120,7 +120,7 @@ const KeyboardAvoidingView = forwardRef<
       (layout: LayoutRectangle) => {
         "worklet";
 
-        if (keyboard.isClosed.value || initialFrame.value === null) {
+        if (keyboard.isClosed.value || initialFrame.value === null || behavior !== "height") {
           // eslint-disable-next-line react-compiler/react-compiler
           initialFrame.value = layout;
         }

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

[Gemma4 + 500LoC context]

LGTM. Here is a review of the changes.

🟢 Review Summary

The changes are minor and seem to involve fixing a redundant condition in
[ repeating src/components/KeyboardAware/src/components/KeyboardAware]

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 10, 2026

[DeepSeek]

The code updates aim to enhance the handling of window insets and edge-to-edge display behavior in a React Native keyboard controller module. Here's a concise summary:

  1. Removed Extension Function: The windowSoftInputMode extension function was removed from View.kt, simplifying dependencies.

  2. New Extension Function: Added replaceStatusBarInsets to dynamically adjust insets based on translucency and activity state, improving layout consistency across Android versions.

  3. Updated Layout Handling:

    • In EdgeToEdgeReactViewGroup, the onLayoutChild method uses the new extension function for better inset management.
    • The setEdgeToEdge() method now dynamically adjusts edge-to-edge behavior based on activity and preservation state, ensuring proper window flags are set.

These changes improve responsiveness and layout stability when handling keyboard inputs across devices.

@kirillzyusko kirillzyusko changed the title ci: AI review ci: LLM code review Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

  1. Missing Error Handling in File Reading

    • Why: The workflow does not handle file reading errors when building prompt.txt, which can lead to incomplete context for the AI review and potential missed issues.
    • Fix: Implement error checking when reading files, and log any errors to ensure they are visible.
  2. No Validation After Ollama Commands

    • Why: The workflow doesn't check if Ollama installation, serving, or model pulls were successful, leading to possible undetected failures in the CI/CD process.
    • Fix: Add checks after each critical command (install, serve, pull) to validate success and fail the job on any failure.

LGTM

@kirillzyusko kirillzyusko merged commit b7ce1ce into main Apr 10, 2026
6 checks passed
@kirillzyusko kirillzyusko deleted the ci/ai-review branch April 10, 2026 18:47
@kirillzyusko kirillzyusko mentioned this pull request Apr 11, 2026
2 tasks
kirillzyusko added a commit that referenced this pull request Apr 11, 2026
## 📜 Description

Improving AI reviews introduced in
#1429

## 💡 Motivation and Context

Tweaked context creation, propmp, code for sending comments - see
changelog section for more details.

## 📢 Changelog

<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->

### CI

- added concurrency;
- re-worked prompt;
- move repeated constants onto variables;
- don't duplicate content if file has been added in this PR;
- don't duplicate content if file has been removed in this PR;
- ignore example/FabricExample code;
- attach only single comment;

## 🤔 How Has This Been Tested?

Tested via this PR.

## 📸 Screenshots (if appropriate):

<img width="886" height="401" alt="image"
src="https://github.com/user-attachments/assets/2ebc20f7-c437-4551-92b0-230daee8eccf"
/>

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI CI (github actions)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant