Skip to content

Benchmark PR 1 - #11

Open
celmis-codereviewer wants to merge 1 commit into
cr-base-1from
cr-pr-1
Open

Benchmark PR 1#11
celmis-codereviewer wants to merge 1 commit into
cr-base-1from
cr-pr-1

Conversation

@celmis-codereviewer

Copy link
Copy Markdown

Benchmark reproduction of ai-code-review-evaluation#1

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

celmis-codereviewer

This comment was marked as outdated.

@celmis-codereviewer celmis-codereviewer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

CHANGES REQUESTED — blocking findings

Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.


def self.optimize(operation, from, to, width, height, opts={})
dim = dimensions(width, height)
def self.downsize(from, to, dimensions, opts={})

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: self.downsize defined on line 149 overwrites the definition at line 145, causing callers passing four arguments (from, to, max_width, max_height) to evaluate max_height as opts and raise a NoMethodError on line 155.

🔴 Method redefinition overwrites previous downsize method signature

Defining self.downsize(from, to, dimensions, opts={}) on line 149 redefines self.downsize(from, to, max_width, max_height, opts={}) from line 145. When callers invoke OptimizedImage.downsize with (from, to, max_width, max_height), max_height will be bound to opts, causing opts[:allow_animation] in optimize to raise a NoMethodError (or TypeError) because opts is an Integer rather than a Hash.

Suggested change
def self.downsize(from, to, dimensions, opts={})
def self.downsize(from, to, max_width, max_height, opts={})
dimensions = max_width.is_a?(String) ? max_width : "#{max_width}x#{max_height}"
opts = max_height if max_height.is_a?(Hash)
optimize("downsize", from, to, dimensions, opts)
end

agent: defect · rule: defect.dead-branch · confidence: 0.95

# allow users to upload large images that will be automatically reduced to allowed size
if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
attempt = 5
while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why: tempfile.size on line 66 queries the open file handle stat which is not updated when OptimizedImage.downsize modifies the file on disk at tempfile.path, causing the while loop to execute all 5 attempts regardless of file size reduction.

🟠 Stale tempfile.size check in image downsizing loop

tempfile.size inspects the open file descriptor held by the Ruby File/Tempfile instance. Because OptimizedImage.downsize invokes an external process (convert) that overwrites or replaces the file at tempfile.path on disk, tempfile.size continues to return the initial file size of the open file descriptor. As a result, tempfile.size > SiteSetting.max_image_size_kb.kilobytes will remain true across every iteration, forcing the loop to run all 5 attempts unnecessarily.

Suggested change
while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
while attempt > 0 && File.size(tempfile.path) > SiteSetting.max_image_size_kb.kilobytes

agent: defect · rule: defect.state-mutation · confidence: 0.90

@celmis-codereviewer

Copy link
Copy Markdown
Author

🤖 Code Review for PR #11

⚙ ADJUSTED — graph context partial (2 of 3 changed files): 1 of 3 changed files have no symbols in the index; 1 of them is not in that checkout at all (app/assets/javascripts/discourse/lib/utilities.js) — this PR's base is older than the indexed revision, so those files were renamed or deleted before it and no re-index can bring them back; there is nothing to fix.

CHANGES REQUESTED — blocking findings

Findings

  • 🔴 Critical: 1
  • 🟠 Error: 1

Scope

  • Files changed: 3
  • Lines: +20 / -12

Performance

  • Analysis time: 106.6s · agents: cve, structural, contract, security, defect · tokens: 14,876/24,146

Powered by Code Analyzer · context: tree-sitter graph + cve, structural, contract, security, defect

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.

2 participants