Setting the execution bit only if it's not set

[andrej1991]

If it is run in an OpenShift pod then it fails, because OpenShift starts the pod with a random user. If the user has already execute permission then it is not needed to set it. Not setting the permissions can prevent the error in an OpenShift pod.

Summary by CodeRabbit

• Bug Fixes
  • Script execution setup is more conservative: scripts are only marked executable when needed. This reduces unnecessary permission changes, helps avoid permission-related warnings or conflicts in various environments, and improves reliability during runs.
• Chores
  • Minor adjustments to execution flow to better respect existing file permissions and minimize side effects.

[coderabbitai]

Walkthrough

Introduces a guard in script preparation to check if the script is already executable before applying chmod(755), modifying the control flow to conditionally set executable permissions.

Changes

Cohort / File(s)	Summary
Script execution guard lsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/script_runner.py	Replace unconditional chmod(755) with a check using os.access(path, os.X_OK) on the resolved path; only apply chmod if not already executable.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant SR as ScriptRunner
  participant OS as OS/Filesystem

  SR->>OS: Resolve script path
  SR->>OS: Check executability (os.access(path, X_OK))
  alt Already executable
    OS-->>SR: true
    note right of SR: Skip chmod
  else Not executable
    OS-->>SR: false
    SR->>OS: chmod(path, 0o755)
    OS-->>SR: permissions updated
  end
  SR-->>SR: Proceed with script run

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my whiskers, hop with glee,
A tiny guard for chmod’s spree—
“Check first!” I thump, then off I go,
If not exec, we make it so.
Small steps neat, permissions tight,
Carrot-commit lands just right. 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The provided title accurately summarizes the main change of conditionally setting the execution bit only when it’s not already set, making it clear and concise without extraneous detail.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

lsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/script_runner.py (1)

45-46: Drop redundant resolve() call before access check.

You already resolved the path a few lines up; handing the existing Path object to os.access avoids the second filesystem lookup and keeps things tidy. os.access happily accepts path-like objects.

-            if not os.access(str(script_path.resolve()), os.X_OK):
+            if not os.access(script_path, os.X_OK):

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c1e0377 and 2f363b4.

📒 Files selected for processing (1)

• lsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/script_runner.py (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-07-16T12:07:29.169Z

Learnt from: asamal4
PR: lightspeed-core/lightspeed-evaluation#19
File: lsc_agent_eval/tests/core/agent_goal_eval/test_script_runner.py:0-0
Timestamp: 2025-07-16T12:07:29.169Z
Learning: In the lsc_agent_eval package, the ScriptRunner class was modified to use absolute paths internally rather than documenting path normalization behavior, providing more predictable and consistent path handling.

Applied to files:

• lsc_agent_eval/src/lsc_agent_eval/core/agent_goal_eval/script_runner.py

[asamal4]

Thank you !!
@tisnik PTAL

[tisnik]

LGTM