tests: add final-status execution tests (done token and fail status) #162
Conversation
… stub debug scripts; ensure LMMAgent is patched in module-level imports
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
…reserve modal & clock)
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gui_agents/s3/cli_app.py (2)
241-242: Potential TypeError:code[0]assumescodeis indexable.At this point in the code,
codecould be a string (fromagent.predict), and accessingcode[0]on a string returns the first character, not the expected behavior.While this may work incidentally (e.g.,
"next"[0]is"n"), the check"next" in code[0].lower()will fail for a string code like"next_step"becausecode[0]would be"n".🔎 Proposed fix: use normalized code_str
- if "next" in code[0].lower(): + if code_str and "next" in code_str.lower(): continue - if "wait" in code[0].lower(): + if code_str and "wait" in code_str.lower(): print("⏳ Agent requested wait...") time.sleep(5) continue
227-239: Remove thebreakat line 239—it terminates the agent loop prematurely on non-final steps, preventing normal code execution.When
execute_finalisNone(agent returns code without final status), the else block (lines 227–239) shows a "Task Completed" dialog and immediately breaks, making lines 241–294 unreachable. This disables all normal step execution: keyword checks ("next", "wait"), code execution, permission prompts, and trajectory updates are never reached for multi-step agent tasks.The break should be removed to allow the loop to continue to normal code execution. The "Task Completed" dialog also appears misplaced—it fires after every non-final step rather than only when the agent explicitly completes.
🧹 Nitpick comments (11)
tests/test_code_agent.py (1)
4-15: Consider removing unused timeout parameter.The
timeoutparameter inrun_bash_scriptis defined but never used in the mock implementation.🔎 Proposed fix
- def run_bash_script(self, code, timeout=30): + def run_bash_script(self, code): return {"status": "success", "output": code, "returncode": 0}Alternatively, if the parameter is required by the interface signature, add a comment or use
_timeoutto indicate it's intentionally unused..pre-commit-config.yaml (1)
1-20: Consider updating tool versions to the latest releases.All specified versions exist and are compatible with Python 3.11. However, newer versions are available: isort (latest 7.0.0), flake8 (latest 7.3.0), and pre-commit-hooks (latest 6.0.0). Updating these tools will ensure access to recent bug fixes and improvements.
requirements-dev.txt (1)
1-6: Pin dependency versions to ensure reproducible builds.Unpinned dependencies can lead to non-reproducible builds and unexpected breakage when maintainers release new major versions. Update the file with current stable versions:
-pytest -pillow -black -flake8 -isort -pre-commit +pytest==9.0.2 +pillow==12.0.0 +black==25.12.0 +flake8==7.3.0 +isort==7.0.0 +pre-commit==4.5.1Verify these versions are compatible with your project before applying.
tests/test_worker.py (1)
11-36: Consider extracting FakeLMMAgent to a shared test fixture.The
FakeLMMAgentclass is duplicated acrosstests/test_smoke.py(lines 51-77) and this file. Extracting it to a shared conftest.py or test utilities module would reduce duplication and ensure consistency.Proposed approach
Create
tests/conftest.pyortests/test_helpers.py:# tests/conftest.py import pytest class FakeLMMAgent: def __init__(self, engine_params=None, system_prompt=None, engine=None): self.messages = [] self.system_prompt = system_prompt or "You are a helpful assistant." self._response = "<thoughts>thinking</thoughts><answer>```python\nagent.wait(1.0)\n```</answer>" def set_response(self, response): """Allow tests to customize the response.""" self._response = response def reset(self): self.messages = [ { "role": "system", "content": [{"type": "text", "text": self.system_prompt}], } ] def add_system_prompt(self, prompt): self.system_prompt = prompt def add_message(self, text_content=None, image_content=None, role=None, **kwargs): self.messages.append( { "role": role or "user", "content": [{"type": "text", "text": text_content}], } ) def get_response(self, *args, **kwargs): return self._response @pytest.fixture def fake_llm_agent(): return FakeLMMAgentThen update both test files to import from the shared location.
scripts/run_agent_safe.py (1)
27-38: Consider restoring the original pyautogui.screenshot.Unlike
scripts/run_agent_open_calculator_safe.py(lines 67-90) which uses a try-finally block to restore patched functions, this script doesn't restorepyautogui.screenshot. While this may be acceptable for a standalone script, adding restoration would make the script more resilient if it's imported or reused.Proposed fix
def main(): + # Store original to restore later + original_screenshot = pyautogui.screenshot + # Patch screenshot to avoid real screen capture pyautogui.screenshot = fake_screenshot - agent = FakeAgent() - # scaled width/height used by run_agent (choose small values for quick run) - scaled_w, scaled_h = 320, 180 - - print("Starting safe agent run (FakeAgent). You should see one step and immediate completion.") - # For automated safe run disable interactive confirmation - cli_app.run_agent(agent, "Test instruction: do nothing", scaled_w, scaled_h, require_exec_confirmation=False) - print("Safe run finished.") + try: + agent = FakeAgent() + # scaled width/height used by run_agent (choose small values for quick run) + scaled_w, scaled_h = 320, 180 + + print("Starting safe agent run (FakeAgent). You should see one step and immediate completion.") + # For automated safe run disable interactive confirmation + cli_app.run_agent(agent, "Test instruction: do nothing", scaled_w, scaled_h, require_exec_confirmation=False) + print("Safe run finished.") + finally: + pyautogui.screenshot = original_screenshottests/test_smoke.py (1)
51-87: Code duplication: FakeLMMAgent is identical to test_worker.py.This is the same duplication noted in
tests/test_worker.py(lines 11-36). Consider extracting to a shared test fixture as recommended in that review.scripts/run_agent_open_calculator_real.py (1)
24-39: The return value semantics are correctly understood; consider aligning with the safe version's pattern.The analysis is accurate:
[code]executes and continues (next step calls predict again), while["done"]stops without executing. Your current two-step pattern (execute on step 1, return "done" on step 2) works correctly but differs from the safe version, which returns[code, "done"]to execute once and immediately stop.The safe version's approach (line 32 of run_agent_open_calculator_safe.py) returns the code with an explicit "done" status, causing
run_agentto execute the code and then exit via thedo_exit_afterlogic (line 297 of cli_app.py). For consistency with the safe version's one-step execution pattern, consider returning[code, "done"]on line 36 instead.scripts/run_agent_calc_and_compute_real.py (1)
55-68: Good use of try/finally for cleanup, but consider context manager pattern.The screenshot patching with try/finally is correct. For improved readability and robustness, consider extracting this into a context manager if this pattern is reused across multiple scripts.
🔎 Optional: context manager pattern
from contextlib import contextmanager @contextmanager def patched_screenshot(): original = pyautogui.screenshot pyautogui.screenshot = fake_screenshot try: yield finally: pyautogui.screenshot = original def main(): with patched_screenshot(): agent = RealComputeAgent() # ... rest of the codescripts/run_agent_create_website_real.py (2)
48-214: Large inline HTML reduces maintainability.The 160+ line HTML template embedded as a string is difficult to maintain and review. Consider moving this to a separate template file or using a heredoc-style approach with proper indentation.
That said, the HTML itself is well-structured with responsive design, accessibility attributes (
aria-hidden,role="dialog"), and proper Russian localization.
227-229: Movetimeimport to top of file.The import is already used at module level via
cli_appdependencies; having it imported insidemain()is inconsistent.🔎 Proposed fix
import os import platform +import time from PIL import Image import pyautoguiThen remove line 228:
- import timescripts/run_agent_open_sinoptik_real.py (1)
69-101: Late import ofcli_appinside function.For consistency with other scripts and to catch import errors early, move this import to the top of the file.
🔎 Proposed fix
import pyautogui +from gui_agents.s3 import cli_app + URL_CANDIDATES = [Then at line 75:
- from gui_agents.s3 import cli_app
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
results/restaurant_screenshot.pngis excluded by!**/*.pngresults/sinoptik_odessa.pngis excluded by!**/*.png
📒 Files selected for processing (23)
.github/workflows/ci.yml.gitignore.patches/0001-chore-add-tests-CI-linters-and-pre-commit-config.patch.patches/0002-ci-use-pre-commit-mirrors-for-flake8-and-add-basic-h.patch.pre-commit-config.yamlgui_agents/s3/cli_app.pyrequirements-dev.txtresults/restaurant/index.htmlscripts/_test_run_agent.pyscripts/run_agent_calc_and_compute_real.pyscripts/run_agent_create_website_real.pyscripts/run_agent_open_calculator_real.pyscripts/run_agent_open_calculator_safe.pyscripts/run_agent_open_sinoptik_real.pyscripts/run_agent_safe.pyscripts/run_debug_branch.pytests/test_cli_confirm_exec.pytests/test_code_agent.pytests/test_final_exec.pytests/test_final_statuses.pytests/test_smoke.pytests/test_utils_formatters.pytests/test_worker.py
🧰 Additional context used
🧬 Code graph analysis (13)
tests/test_code_agent.py (1)
gui_agents/s3/agents/code_agent.py (1)
extract_code_block(11-29)
tests/test_final_exec.py (1)
gui_agents/s3/cli_app.py (1)
run_agent(164-294)
scripts/_test_run_agent.py (5)
gui_agents/s3/cli_app.py (1)
main(297-460)scripts/run_agent_open_calculator_safe.py (1)
main(67-90)scripts/run_agent_open_sinoptik_real.py (1)
main(69-101)scripts/run_agent_safe.py (1)
main(27-38)scripts/run_debug_branch.py (1)
main(5-6)
tests/test_cli_confirm_exec.py (1)
gui_agents/s3/cli_app.py (1)
run_agent(164-294)
scripts/run_agent_open_sinoptik_real.py (1)
gui_agents/s3/cli_app.py (1)
run_agent(164-294)
scripts/run_agent_create_website_real.py (1)
gui_agents/s3/cli_app.py (1)
run_agent(164-294)
tests/test_worker.py (2)
gui_agents/s3/agents/agent_s.py (1)
AgentS3(48-94)tests/test_smoke.py (5)
FakeLMMAgent(51-77)reset(56-62)add_system_prompt(64-65)add_message(67-73)get_response(75-77)
tests/test_smoke.py (2)
tests/test_worker.py (5)
FakeLMMAgent(11-36)reset(16-22)add_system_prompt(24-25)add_message(27-33)get_response(35-36)gui_agents/s3/cli_app.py (1)
main(297-460)
tests/test_utils_formatters.py (1)
gui_agents/s3/utils/common_utils.py (1)
extract_agent_functions(169-179)
scripts/run_agent_safe.py (2)
scripts/run_agent_open_calculator_safe.py (3)
predict(20-32)fake_screenshot(39-40)main(67-90)gui_agents/s3/cli_app.py (2)
main(297-460)run_agent(164-294)
scripts/run_agent_open_calculator_real.py (1)
gui_agents/s3/cli_app.py (1)
run_agent(164-294)
scripts/run_agent_calc_and_compute_real.py (2)
scripts/run_agent_open_calculator_real.py (2)
reset(21-22)predict(24-39)gui_agents/s3/cli_app.py (1)
run_agent(164-294)
tests/test_final_statuses.py (5)
scripts/run_agent_open_sinoptik_real.py (1)
predict(36-43)scripts/run_agent_safe.py (1)
predict(16-19)tests/test_cli_confirm_exec.py (1)
predict(13-17)tests/test_final_exec.py (1)
predict(11-18)gui_agents/s3/cli_app.py (1)
run_agent(164-294)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
15-15: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 Ruff (0.14.10)
scripts/run_agent_open_calculator_safe.py
20-20: Unused method argument: observation
(ARG002)
tests/test_code_agent.py
14-14: Unused method argument: timeout
(ARG002)
tests/test_final_exec.py
11-11: Unused method argument: instruction
(ARG002)
11-11: Unused method argument: observation
(ARG002)
tests/test_cli_confirm_exec.py
13-13: Unused method argument: instruction
(ARG002)
13-13: Unused method argument: observation
(ARG002)
31-31: Unused lambda argument: code
(ARG005)
31-31: Unused lambda argument: desc
(ARG005)
51-51: Unused lambda argument: code
(ARG005)
51-51: Unused lambda argument: desc
(ARG005)
53-53: Unused lambda argument: prompt
(ARG005)
72-72: Unused lambda argument: code
(ARG005)
72-72: Unused lambda argument: desc
(ARG005)
74-74: Unused lambda argument: prompt
(ARG005)
gui_agents/s3/cli_app.py
154-154: Use of exec detected
(S102)
206-206: Do not catch blind exception: Exception
(BLE001)
221-221: Local variable final_exit_after_exec is assigned to but never used
Remove assignment to unused variable final_exit_after_exec
(F841)
263-263: Do not catch blind exception: Exception
(BLE001)
273-273: Do not catch blind exception: Exception
(BLE001)
scripts/run_agent_open_sinoptik_real.py
36-36: Unused method argument: instruction
(ARG002)
36-36: Unused method argument: observation
(ARG002)
46-46: Unused function argument: url
(ARG001)
52-52: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
55-55: String contains ambiguous Н (CYRILLIC CAPITAL LETTER EN). Did you mean H (LATIN CAPITAL LETTER H)?
(RUF001)
55-55: String contains ambiguous а (CYRILLIC SMALL LETTER A). Did you mean a (LATIN SMALL LETTER A)?
(RUF001)
64-64: Do not catch blind exception: Exception
(BLE001)
scripts/run_agent_create_website_real.py
44-44: Unused method argument: instruction
(ARG002)
44-44: Unused method argument: observation
(ARG002)
55-55: String contains ambiguous У (CYRILLIC CAPITAL LETTER U). Did you mean Y (LATIN CAPITAL LETTER Y)?
(RUF001)
101-101: String contains ambiguous У (CYRILLIC CAPITAL LETTER U). Did you mean Y (LATIN CAPITAL LETTER Y)?
(RUF001)
113-113: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
127-127: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
127-127: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
127-127: String contains ambiguous о (CYRILLIC SMALL LETTER O). Did you mean o (LATIN SMALL LETTER O)?
(RUF001)
127-127: String contains ambiguous у (CYRILLIC SMALL LETTER U). Did you mean y (LATIN SMALL LETTER Y)?
(RUF001)
127-127: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
127-127: String contains ambiguous е (CYRILLIC SMALL LETTER IE). Did you mean e (LATIN SMALL LETTER E)?
(RUF001)
128-128: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
129-129: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
141-141: String contains ambiguous У (CYRILLIC CAPITAL LETTER U). Did you mean Y (LATIN CAPITAL LETTER Y)?
(RUF001)
147-147: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
170-170: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
241-241: Do not catch blind exception: Exception
(BLE001)
tests/test_worker.py
12-12: Unused method argument: engine_params
(ARG002)
12-12: Unused method argument: engine
(ARG002)
27-27: Unused method argument: image_content
(ARG002)
27-27: Unused method argument: kwargs
(ARG002)
35-35: Unused method argument: args
(ARG002)
35-35: Unused method argument: kwargs
(ARG002)
76-76: Unpacked variable info is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
tests/test_smoke.py
13-13: Unused static method argument: image
(ARG004)
13-13: Unused static method argument: output_type
(ARG004)
48-48: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
52-52: Unused method argument: engine_params
(ARG002)
52-52: Unused method argument: engine
(ARG002)
67-67: Unused method argument: image_content
(ARG002)
67-67: Unused method argument: kwargs
(ARG002)
75-75: Unused method argument: args
(ARG002)
75-75: Unused method argument: kwargs
(ARG002)
122-122: Unpacked variable info is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
scripts/run_agent_safe.py
16-16: Unused method argument: observation
(ARG002)
scripts/run_agent_open_calculator_real.py
24-24: Unused method argument: observation
(ARG002)
scripts/run_agent_calc_and_compute_real.py
26-26: Unused method argument: observation
(ARG002)
tests/test_final_statuses.py
11-11: Unused method argument: instruction
(ARG002)
11-11: Unused method argument: observation
(ARG002)
24-24: Unused method argument: instruction
(ARG002)
24-24: Unused method argument: observation
(ARG002)
🔇 Additional comments (27)
scripts/run_debug_branch.py (1)
1-10: The no-op stub is correctly implemented.The file is properly formatted with clear documentation explaining the intentional stub-out to prevent accidental execution during test/CI runs. Verification confirms no production or test code depends on this debug script, making the stubbing approach safe.
scripts/_test_run_agent.py (1)
1-10: LGTM!The no-op test harness is well-documented and serves its purpose of preventing accidental execution during test runs. The implementation is straightforward and safe.
tests/test_final_exec.py (2)
6-18: LGTM!The
FakeAgentimplementation correctly simulates the final-status execution flow. The test validates that code with[code, "done"]status executes the code before exiting.Note: The static analysis warning about unused
instructionandobservationparameters is a false positive—these parameters are required by the agent interface signature.
21-34: LGTM!The test correctly verifies final execution behavior by:
- Creating a fake agent that returns code with "done" status
- Stubbing pyautogui.screenshot to avoid desktop dependencies
- Running the agent without user confirmation
- Asserting the output file exists with expected content
This aligns well with the PR objectives for testing final-status execution.
results/restaurant/index.html (1)
1-128: Unrelated file in PR.This restaurant landing page appears unrelated to the PR objectives, which focus on adding final-status execution tests. The PR description acknowledges this as an "unrelated site change."
Consider whether this file should be:
- Moved to a separate PR focused on site generation functionality
- Removed if it was included accidentally
- Kept if it's a test artifact that demonstrates agent-generated output
Could you clarify whether this file is intentionally included as a test artifact or should be moved to a separate PR?
tests/test_code_agent.py (1)
18-36: LGTM!The test functions provide good coverage for code extraction and execution:
test_extract_code_blockvalidates Python code block parsingtest_execute_code_pythonverifies Python execution via the controllertest_execute_code_bashverifies Bash execution via the controllerThe use of
DummyEnvControllerproperly isolates the tests from actual code execution.tests/test_worker.py (2)
39-46: Good practice: Comprehensive module-level patching.The module-level patching across
mllm_mod,_code_agent,_grounding, and_moduleensures that all import paths use the fake agent. This prevents subtle bugs from module-level imports.
76-82: LGTM: Test validates agent action output.The test correctly verifies that the agent returns a list of actions containing a wait/sleep instruction, which aligns with the FakeLMMAgent's canned response.
Note: The unused
infovariable (flagged by static analysis) could be prefixed with_if desired, but it's acceptable as-is.tests/test_utils_formatters.py (1)
1-15: LGTM: Clear and focused unit tests.The tests clearly validate the code extraction and function detection utilities. The test cases are straightforward and well-named.
tests/test_final_statuses.py (3)
6-16: LGTM: FailAgent correctly models fail status behavior.The agent returns
[code, "fail"]on the first call, which should trigger execution of the code followed by termination. This aligns with the cli_app.run_agent semantics described in the relevant code snippets.
19-29: LGTM: DoneTokenAgent models terminal token behavior.The agent returns
["done"]as the code string, which should be interpreted as a terminal token and skip execution. This correctly tests the semantic distinction between:
["done"](terminal token, no execution)[code, "done"](execute code once, then stop)
36-56: Well-structured integration tests for final-status behavior.Both tests correctly:
- Stub pyautogui.screenshot to avoid UI dependencies
- Set
require_exec_confirmation=Falsefor automated testing- Verify the expected file creation/non-creation behavior
The tests effectively validate the terminal token semantics introduced in this PR.
scripts/run_agent_safe.py (1)
12-19: LGTM: FakeAgent provides safe no-op behavior.The agent immediately returns a "done" token, preventing any code execution. This is appropriate for a safe testing script.
tests/test_smoke.py (4)
8-45: Excellent approach: Lightweight dependency mocking.The dummy modules for
pytesseractandpyautoguiallow tests to run without heavy external dependencies or GUI requirements. The minimal return values match expected interfaces while keeping tests fast.
48-48: The noqa directive is necessary despite static analysis warning.The
# noqa: E402comment is appropriate here because the imports on lines 81-87 intentionally occur after modifyingsys.modules. The Ruff warning about an unused noqa is a false positive in this context.
97-127: LGTM: Comprehensive smoke test validates core agent flow.The test correctly:
- Mocks all dependencies
- Instantiates the full agent stack (grounding + AgentS3)
- Verifies the predict output structure and content
This provides good confidence that the basic agent flow works end-to-end.
130-145: Good coverage: CLI help test ensures basic CLI functionality.The test verifies that the CLI can be imported with mocked dependencies and that
--helpexits cleanly with code 0. This catches import-time issues and basic argument parsing problems.scripts/run_agent_open_calculator_safe.py (3)
16-32: LGTM: Platform-aware agent with appropriate stubbing.The agent generates platform-specific code for opening Calculator and returns a final "done" status to terminate after one action. The platform detection logic correctly handles Windows, macOS, and Linux.
35-64: Excellent: Comprehensive stubbing prevents all side effects.The script stubs:
- pyautogui functions (hotkey, typewrite, press)
- os.system
- subprocess.run
This ensures the script is truly safe and prints actions instead of executing them. The stub implementations provide useful debugging output.
67-90: Good practice: Proper cleanup with try-finally.The main function correctly:
- Saves original functions before patching
- Patches all relevant functions
- Uses try-finally to ensure restoration
- Disables
require_exec_confirmationfor automated executionThis is a robust pattern for safe testing scripts.
scripts/run_agent_open_calculator_real.py (1)
46-59: LGTM: Proper setup and teardown for real execution.The main function:
- Appropriately warns about real execution (docstring line 7)
- Patches only the screenshot to keep execution lightweight
- Uses try-finally for proper cleanup
- Disables
require_exec_confirmationas user has pre-confirmed.patches/0002-ci-use-pre-commit-mirrors-for-flake8-and-add-basic-h.patch (1)
20-25: Configuration updates are sound; version 7.1.0 is stable.The changes look good—switching to pre-commit mirrors, updating to flake8 7.1.0 (released June 2024), and aligning the line length to 88 characters with Black's formatter. The ignore rules E203 and W503 are appropriate for Black compatibility.
tests/test_cli_confirm_exec.py (1)
5-17: LGTM — clean helper agent for testing.The
OneStepAgentcorrectly simulates a one-shot agent that returns executable code with a"done"status on the first call and a terminal"done"token on subsequent calls. This aligns with therun_agentsemantics where[code, "done"]triggers final execution.scripts/run_agent_calc_and_compute_real.py (1)
19-48: LGTM — well-structured agent for real desktop automation.The
RealComputeAgentcorrectly implements the agent interface with platform-specific Calculator launching and keystroke automation. The return of[code, "done"]on first call triggers the final execution path inrun_agent.gui_agents/s3/cli_app.py (2)
148-154: Appropriate centralization of code execution for testability.The
execute_codehelper correctly enables mocking in tests. The use ofexec()withglobals()is intentional here to allow agent-generated code to modify global state and persist imports.Note that this is inherently dangerous and relies on upstream confirmation checks for safety.
388-393: LGTM — new CLI flag for execution confirmation bypass.The
--allow_execflag is properly documented with a danger warning and correctly wired torequire_exec_confirmation.scripts/run_agent_open_sinoptik_real.py (1)
29-43: LGTM — simple agent implementation for browser automation.The agent correctly follows the pattern used by other real agent scripts in this PR, returning
[code, "done"]on first call to trigger final execution.
| - uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: "3.11" |
There was a problem hiding this comment.
Update to actions/setup-python@v5.
The actions/setup-python@v4 action runner is deprecated and too old to run on current GitHub Actions infrastructure.
🔎 Proposed fix
- - uses: actions/setup-python@v4
+ - uses: actions/setup-python@v5
with:
python-version: "3.11"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/setup-python@v4 | |
| with: | |
| python-version: "3.11" | |
| - uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.11" |
🧰 Tools
🪛 actionlint (1.7.9)
15-15: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 15 to 17: the workflow is using the
deprecated actions/setup-python@v4; update the runner to actions/setup-python@v5
by changing the uses entry to actions/setup-python@v5, keep the existing with:
python-version value (e.g., "3.11"), validate YAML syntax after the change, and
run the CI workflow to ensure no additional adjustments (like action input name
changes) are required.
| .DS_Store | ||
|
|
||
| # Local env file for secrets | ||
| .env |
There was a problem hiding this comment.
Duplicate .env entry.
The .env entry is already present at line 125 under the "Environments" section. This duplicate entry at line 167 is redundant.
🔎 Proposed fix
.DS_Store
-
-# Local env file for secrets
-.envThe existing entry at line 125 already covers environment files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .env | |
| .DS_Store |
🤖 Prompt for AI Agents
In .gitignore around line 167, the ".env" entry is duplicated (already present
at line 125 under "Environments"); remove the redundant ".env" line at 167 so
the file contains only the single environment entry at line 125.
| final_exit_after_exec = True | ||
| else: | ||
| execute_final = None | ||
| final_exit_after_exec = False |
There was a problem hiding this comment.
Unused variable: final_exit_after_exec is assigned but never read.
This variable is set but do_exit_after is used instead for the same purpose.
🔎 Proposed fix: remove unused variable
if status in ("done", "fail"):
# Execute the final code, then stop
execute_final = code_str
- # fall through to execution branch below
- final_exit_after_exec = True
+ # fall through to execution branch below; do_exit_after set below
else:
execute_final = None
- final_exit_after_exec = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final_exit_after_exec = True | |
| else: | |
| execute_final = None | |
| final_exit_after_exec = False | |
| if status in ("done", "fail"): | |
| # Execute the final code, then stop | |
| execute_final = code_str | |
| # fall through to execution branch below; do_exit_after set below | |
| else: | |
| execute_final = None |
🧰 Tools
🪛 Ruff (0.14.10)
221-221: Local variable final_exit_after_exec is assigned to but never used
Remove assignment to unused variable final_exit_after_exec
(F841)
🤖 Prompt for AI Agents
In gui_agents/s3/cli_app.py around lines 218 to 221, the local variable
final_exit_after_exec is assigned but never used (do_exit_after is used
instead); remove the final_exit_after_exec assignments and any other references
to that variable so the code only sets and checks do_exit_after, ensuring no
change to logic flow or behavior.
| def _default_chrome_paths(): | ||
| paths = [] | ||
| if platform.system().lower() == "windows": | ||
| paths.extend([ | ||
| r"C:\Program Files\Google\Chrome\Application\chrome.exe", | ||
| r"C:\Program Files (x86)\Google\Chrome\Application\chrome.exe", | ||
| ]) | ||
| elif platform.system().lower() == "darwin": | ||
| # macOS | ||
| paths.append("/Applications/Google Chrome.app/Contents/MacOS/Google Chrome") | ||
| else: | ||
| # common Linux paths | ||
| paths.extend([ | ||
| "/usr/bin/google-chrome", | ||
| "/usr/bin/google-chrome-stable", | ||
| "/snap/bin/chromium", | ||
| ]) | ||
| return paths |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Dead code: _default_chrome_paths is defined but never used.
This function duplicates the Chrome path logic that's already embedded in the agent's code string (lines 186-197). Either use this helper in the code string or remove it.
🔎 Proposed fix: remove unused function
-def _default_chrome_paths():
- paths = []
- if platform.system().lower() == "windows":
- paths.extend([
- r"C:\Program Files\Google\Chrome\Application\chrome.exe",
- r"C:\Program Files (x86)\Google\Chrome\Application\chrome.exe",
- ])
- elif platform.system().lower() == "darwin":
- # macOS
- paths.append("/Applications/Google Chrome.app/Contents/MacOS/Google Chrome")
- else:
- # common Linux paths
- paths.extend([
- "/usr/bin/google-chrome",
- "/usr/bin/google-chrome-stable",
- "/snap/bin/chromium",
- ])
- return paths
-
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _default_chrome_paths(): | |
| paths = [] | |
| if platform.system().lower() == "windows": | |
| paths.extend([ | |
| r"C:\Program Files\Google\Chrome\Application\chrome.exe", | |
| r"C:\Program Files (x86)\Google\Chrome\Application\chrome.exe", | |
| ]) | |
| elif platform.system().lower() == "darwin": | |
| # macOS | |
| paths.append("/Applications/Google Chrome.app/Contents/MacOS/Google Chrome") | |
| else: | |
| # common Linux paths | |
| paths.extend([ | |
| "/usr/bin/google-chrome", | |
| "/usr/bin/google-chrome-stable", | |
| "/snap/bin/chromium", | |
| ]) | |
| return paths |
🤖 Prompt for AI Agents
In scripts/run_agent_create_website_real.py around lines 17 to 34, the helper
function _default_chrome_paths is dead code and duplicates logic already present
in the agent's embedded code string; remove the entire function definition (and
any direct references if accidentally added elsewhere), or alternatively replace
the duplicated path logic inside the agent code string with a call to this
helper—preferably delete the unused helper to keep the file minimal; after
removal, run a quick grep for _default_chrome_paths to ensure no remaining
references and run the test/agent flow to confirm no regressions.
| def fetch_month_block(url): | ||
| tried = [] | ||
| for u in URL_CANDIDATES: | ||
| try: | ||
| # Ensure proper quoting for non-ascii parts | ||
| u_quoted = u if all(ord(c) < 128 for c in u) else quote(u, safe=':/') | ||
| resp = urllib.request.urlopen(u_quoted, timeout=10) | ||
| html = resp.read().decode('utf-8', errors='replace') | ||
| # Search for keywords | ||
| keywords = ['Погода на месяц', 'Погода на місяць', 'Месяц', 'на месяц', 'На місяць'] | ||
| for kw in keywords: | ||
| idx = html.find(kw) | ||
| if idx != -1: | ||
| start = max(0, idx-800) | ||
| end = min(len(html), idx+2400) | ||
| block = unescape(html[start:end]) | ||
| return (u, kw, block) | ||
| tried.append((u, 'no-keyword')) | ||
| except Exception as e: | ||
| tried.append((u, str(e))) | ||
| return (None, None, tried) |
There was a problem hiding this comment.
Bug: url parameter is ignored; function always iterates URL_CANDIDATES.
The function signature accepts a url parameter but never uses it, instead iterating over the global URL_CANDIDATES. This is misleading and the call at line 90 passes URL_CANDIDATES as the argument, which is incorrect usage.
🔎 Proposed fix: either use the parameter or remove it
Option 1: Use the parameter (preferred)
-def fetch_month_block(url):
+def fetch_month_block(urls):
+ """Fetch and parse month block from the first successful URL in the list."""
tried = []
- for u in URL_CANDIDATES:
+ for u in urls:Option 2: Remove the parameter
-def fetch_month_block(url):
+def fetch_month_block():
+ """Fetch and parse month block from URL_CANDIDATES."""
tried = []
for u in URL_CANDIDATES:🧰 Tools
🪛 Ruff (0.14.10)
46-46: Unused function argument: url
(ARG001)
52-52: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
55-55: String contains ambiguous Н (CYRILLIC CAPITAL LETTER EN). Did you mean H (LATIN CAPITAL LETTER H)?
(RUF001)
55-55: String contains ambiguous а (CYRILLIC SMALL LETTER A). Did you mean a (LATIN SMALL LETTER A)?
(RUF001)
64-64: Do not catch blind exception: Exception
(BLE001)
| # Ensure proper quoting for non-ascii parts | ||
| u_quoted = u if all(ord(c) < 128 for c in u) else quote(u, safe=':/') | ||
| resp = urllib.request.urlopen(u_quoted, timeout=10) |
There was a problem hiding this comment.
URL quoting may not encode Cyrillic paths correctly.
The condition all(ord(c) < 128 for c in u) checks for non-ASCII, but quote(u, safe=':/') with safe=':/' may not properly encode the Cyrillic path segments. For URLs like https://sinoptik.ua/погода-одесса/, you need to encode only the path portion.
🔎 Proposed fix: use proper URL encoding
+from urllib.parse import urlparse, quote as url_quote
- u_quoted = u if all(ord(c) < 128 for c in u) else quote(u, safe=':/')
+ # Properly encode non-ASCII path segments
+ parsed = urlparse(u)
+ encoded_path = url_quote(parsed.path, safe='/')
+ u_quoted = f"{parsed.scheme}://{parsed.netloc}{encoded_path}"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
52-52: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
In scripts/run_agent_open_sinoptik_real.py around lines 50 to 52, the current
check and call quote(u, safe=':/') may not correctly percent-encode Cyrillic
path segments; instead split the URL into components (scheme, netloc, path,
query, fragment), percent-encode only the path (and query if present) using
quote with safe='/' for path so segments are encoded but slashes preserved, then
rebuild the URL with urlunsplit (or urlunparse) and use that encoded URL for
urllib.request.urlopen with the same timeout.
| def test_execute_with_gui_confirmation(monkeypatch): | ||
| from gui_agents.s3 import cli_app | ||
|
|
||
| agent = OneStepAgent("print('hello')") | ||
|
|
||
| called = [] | ||
|
|
||
| def fake_execute(code_str): | ||
| called.append(code_str) | ||
|
|
||
| monkeypatch.setattr(cli_app, "execute_code", fake_execute) | ||
| monkeypatch.setattr(cli_app, "show_permission_dialog", lambda code, desc: True) | ||
|
|
||
| # Run with require_exec_confirmation=True | ||
| cli_app.run_agent(agent, "instr", 100, 100, require_exec_confirmation=True) | ||
|
|
||
| assert len(called) == 1 | ||
| assert "print('hello')" in called[0] |
There was a problem hiding this comment.
Tests may fail due to missing mocks for pyautogui.screenshot and time.sleep.
The run_agent function calls pyautogui.screenshot() and time.sleep() before reaching the execution path. Without mocking these, tests will attempt real screenshot capture (which may fail in CI) and introduce delays.
🔎 Proposed fix to add missing mocks
def test_execute_with_gui_confirmation(monkeypatch):
from gui_agents.s3 import cli_app
+ from PIL import Image
agent = OneStepAgent("print('hello')")
called = []
def fake_execute(code_str):
called.append(code_str)
monkeypatch.setattr(cli_app, "execute_code", fake_execute)
monkeypatch.setattr(cli_app, "show_permission_dialog", lambda code, desc: True)
+ monkeypatch.setattr(cli_app.pyautogui, "screenshot", lambda: Image.new("RGB", (100, 100)))
+ monkeypatch.setattr(cli_app.time, "sleep", lambda x: None)
# Run with require_exec_confirmation=True
cli_app.run_agent(agent, "instr", 100, 100, require_exec_confirmation=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_execute_with_gui_confirmation(monkeypatch): | |
| from gui_agents.s3 import cli_app | |
| agent = OneStepAgent("print('hello')") | |
| called = [] | |
| def fake_execute(code_str): | |
| called.append(code_str) | |
| monkeypatch.setattr(cli_app, "execute_code", fake_execute) | |
| monkeypatch.setattr(cli_app, "show_permission_dialog", lambda code, desc: True) | |
| # Run with require_exec_confirmation=True | |
| cli_app.run_agent(agent, "instr", 100, 100, require_exec_confirmation=True) | |
| assert len(called) == 1 | |
| assert "print('hello')" in called[0] | |
| def test_execute_with_gui_confirmation(monkeypatch): | |
| from gui_agents.s3 import cli_app | |
| from PIL import Image | |
| agent = OneStepAgent("print('hello')") | |
| called = [] | |
| def fake_execute(code_str): | |
| called.append(code_str) | |
| monkeypatch.setattr(cli_app, "execute_code", fake_execute) | |
| monkeypatch.setattr(cli_app, "show_permission_dialog", lambda code, desc: True) | |
| monkeypatch.setattr(cli_app.pyautogui, "screenshot", lambda: Image.new("RGB", (100, 100))) | |
| monkeypatch.setattr(cli_app.time, "sleep", lambda x: None) | |
| # Run with require_exec_confirmation=True | |
| cli_app.run_agent(agent, "instr", 100, 100, require_exec_confirmation=True) | |
| assert len(called) == 1 | |
| assert "print('hello')" in called[0] |
🧰 Tools
🪛 Ruff (0.14.10)
31-31: Unused lambda argument: code
(ARG005)
31-31: Unused lambda argument: desc
(ARG005)
🤖 Prompt for AI Agents
In tests/test_cli_confirm_exec.py around lines 20 to 37, the test calls
run_agent which invokes pyautogui.screenshot() and time.sleep() before executing
the agent; add monkeypatch stubs for pyautogui.screenshot (return a simple dummy
object or PIL Image) and time.sleep (no-op) at the top of the test so the CI
won't attempt a real screenshot or wait, then proceed to patch
cli_app.execute_code and cli_app.show_permission_dialog as already done.
| def test_execute_with_input_confirmation(monkeypatch): | ||
| from gui_agents.s3 import cli_app | ||
|
|
||
| agent = OneStepAgent("print('hi')") | ||
|
|
||
| called = [] | ||
|
|
||
| def fake_execute(code_str): | ||
| called.append(code_str) | ||
|
|
||
| monkeypatch.setattr(cli_app, "execute_code", fake_execute) | ||
| monkeypatch.setattr(cli_app, "show_permission_dialog", lambda code, desc: False) | ||
|
|
||
| monkeypatch.setattr(builtins, "input", lambda prompt="": "y") | ||
|
|
||
| cli_app.run_agent(agent, "instr", 100, 100, require_exec_confirmation=True) | ||
|
|
||
| assert len(called) == 1 | ||
| assert "print('hi')" in called[0] |
There was a problem hiding this comment.
Same missing mocks apply here.
This test also needs pyautogui.screenshot and time.sleep mocked to run reliably in CI environments.
🧰 Tools
🪛 Ruff (0.14.10)
51-51: Unused lambda argument: code
(ARG005)
51-51: Unused lambda argument: desc
(ARG005)
53-53: Unused lambda argument: prompt
(ARG005)
🤖 Prompt for AI Agents
In tests/test_cli_confirm_exec.py around lines 40 to 58 the test fails in CI
because it does not mock external calls; add mocks for pyautogui.screenshot and
time.sleep so the test doesn't perform real GUI or delay operations. Use
monkeypatch.setattr to replace pyautogui.screenshot with a no-op or a dummy
image-returning callable and monkeypatch.setattr(time, "sleep", lambda s: None)
(ensure the time module is imported in the test) before calling
cli_app.run_agent so the test runs reliably in CI.
| def test_decline_execution(monkeypatch): | ||
| from gui_agents.s3 import cli_app | ||
|
|
||
| agent = OneStepAgent("print('no')") | ||
|
|
||
| called = [] | ||
|
|
||
| def fake_execute(code_str): | ||
| called.append(code_str) | ||
|
|
||
| monkeypatch.setattr(cli_app, "execute_code", fake_execute) | ||
| monkeypatch.setattr(cli_app, "show_permission_dialog", lambda code, desc: False) | ||
|
|
||
| monkeypatch.setattr(builtins, "input", lambda prompt="": "n") | ||
|
|
||
| cli_app.run_agent(agent, "instr", 100, 100, require_exec_confirmation=True) | ||
|
|
||
| assert len(called) == 0 |
There was a problem hiding this comment.
Same missing mocks apply here.
Additionally, consider adding a test for require_exec_confirmation=False to verify that code executes without any confirmation prompts—this is the path used by the real agent scripts in this PR.
Do you want me to generate the additional test case for the require_exec_confirmation=False path?
🧰 Tools
🪛 Ruff (0.14.10)
72-72: Unused lambda argument: code
(ARG005)
72-72: Unused lambda argument: desc
(ARG005)
74-74: Unused lambda argument: prompt
(ARG005)
🤖 Prompt for AI Agents
In tests/test_cli_confirm_exec.py around lines 61 to 78, the test is missing
consistent mocks for confirmation-related functions and we should add a new test
for the require_exec_confirmation=False path; update the existing test to
explicitly monkeypatch cli_app.show_permission_dialog and builtins.input (as
done elsewhere) to simulate a user declining, and add a new test that
monkeypatches cli_app.execute_code to capture calls, ensures
show_permission_dialog and builtins.input are either not called or mocked, calls
cli_app.run_agent(..., require_exec_confirmation=False) and asserts execute_code
was invoked once with the agent code (verifying no confirmation prompt path).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
results/website_dressa_inspired/js/main.js (1)
1-38: LGTM for demo purposes.The implementation provides functional catalog, cart, and modal behavior suitable for an MVP demo. The static analysis parsing errors are false positives caused by Ukrainian characters in the product data.
For future iterations, consider:
- Replacing inline
onclickhandlers (line 15) with event delegation- Using a custom modal or toast instead of
alert()(line 22) for better UX- Adding try-catch around localStorage operations for private browsing compatibility
results/website_dressa_inspired/index.html (1)
27-30: Consider adding alt attributes for accessibility.The category card images are missing descriptive
altattributes, which impacts screen reader accessibility.🔎 Suggested accessibility improvements
- <div class="card"><img src="https://images.unsplash.com/photo-1520975925553-66f1d6a9c4b1?w=800&q=80"><h4>Сукні</h4></div> - <div class="card"><img src="https://images.unsplash.com/photo-1514996937319-344454492b37?w=800&q=80"><h4>Джинси</h4></div> - <div class="card"><img src="https://images.unsplash.com/photo-1541099649105-f69ad21f3246?w=800&q=80"><h4>Кардигани</h4></div> - <div class="card"><img src="https://images.unsplash.com/photo-1495121605193-b116b5b09d0b?w=800&q=80"><h4>Тренчі</h4></div> + <div class="card"><img src="https://images.unsplash.com/photo-1520975925553-66f1d6a9c4b1?w=800&q=80" alt="Сукні"><h4>Сукні</h4></div> + <div class="card"><img src="https://images.unsplash.com/photo-1514996937319-344454492b37?w=800&q=80" alt="Джинси"><h4>Джинси</h4></div> + <div class="card"><img src="https://images.unsplash.com/photo-1541099649105-f69ad21f3246?w=800&q=80" alt="Кардигани"><h4>Кардигани</h4></div> + <div class="card"><img src="https://images.unsplash.com/photo-1495121605193-b116b5b09d0b?w=800&q=80" alt="Тренчі"><h4>Тренчі</h4></div>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
results/website_dressa_inspired/screenshot_home.pngis excluded by!**/*.png
📒 Files selected for processing (10)
results/website_dressa_inspired/about.htmlresults/website_dressa_inspired/blog.htmlresults/website_dressa_inspired/catalog.htmlresults/website_dressa_inspired/contacts.htmlresults/website_dressa_inspired/css/style.cssresults/website_dressa_inspired/index.htmlresults/website_dressa_inspired/js/main.jsresults/website_dressa_inspired/product.htmlscripts/run_generate_dressa_inspired.pytests/test_website_structure.py
✅ Files skipped from review due to trivial changes (3)
- results/website_dressa_inspired/catalog.html
- results/website_dressa_inspired/product.html
- results/website_dressa_inspired/contacts.html
🧰 Additional context used
🪛 Biome (2.1.2)
results/website_dressa_inspired/js/main.js
[error] 6-6: expected , but instead found язаний
Remove язаний
(parse)
[error] 6-6: expected , but instead found кардиган
Remove кардиган
(parse)
[error] 6-6: expected , but instead found ',price:499,image:'
Remove ',price:499,image:'
(parse)
[error] 6-6: expected : but instead found https
Remove https
(parse)
[error] 6-6: expected , but instead found :
Remove :
(parse)
[error] 10-10: Expected a property, a shorthand property, a getter, a setter, or a method but instead found ']'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
🪛 Ruff (0.14.10)
scripts/run_generate_dressa_inspired.py
6-6: Avoid specifying long messages outside the exception class
(TRY003)
18-18: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (6)
results/website_dressa_inspired/about.html (1)
1-15: LGTM!The about page follows a consistent structure with other pages in the Dressa-inspired demo site and correctly links to shared assets.
results/website_dressa_inspired/index.html (1)
1-40: LGTM!The homepage structure is well-organized with proper navigation, hero section, category grid, and modal implementation suitable for the demo site.
results/website_dressa_inspired/blog.html (1)
1-15: LGTM!The blog page maintains consistency with the overall site structure and properly integrates with shared CSS/JS assets.
tests/test_website_structure.py (1)
1-22: LGTM!The test structure is clean and validates essential website components. The helper function correctly handles file loading with proper encoding, and the assertions verify the key structural elements of the demo site.
results/website_dressa_inspired/css/style.css (1)
1-21: LGTM!The stylesheet provides a clean, minimal design system with responsive behavior appropriate for the MVP demo site. The CSS variables, component styles, and modal implementation align well with the HTML structure.
scripts/run_generate_dressa_inspired.py (1)
1-19: Dependency is already documented.
pyautoguiis properly listed in bothsetup.py(install_requires) andrequirements.txt. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
results/website_dressa_inspired/data/products.json (1)
1-22: Image URL repetition across products.The data file uses only 5 distinct image URLs, each repeated for 4 different products. This pattern creates visual repetition in the catalog where products with different titles share identical images.
If this is placeholder data for testing, consider adding a comment or TODO. For production, each product should ideally have a unique representative image.
tests/test_products_json.py (2)
3-7: Consider validating JSON structure, not just string presence.The test checks for the substring
"id":1but doesn't validate that the file is valid JSON or that it contains the expected structure (array of objects with id, title, price, image fields).🔎 Proposed enhancement for robust JSON validation
+import json + def test_products_json_exists(): p = Path('results/website_dressa_inspired/data/products.json') assert p.exists(), 'products.json must exist' data = p.read_text(encoding='utf-8') - assert '"id":1' in data + products = json.loads(data) + assert isinstance(products, list), 'products.json must contain a list' + assert len(products) > 0, 'products list must not be empty' + assert products[0]['id'] == 1, 'first product must have id:1' + # Validate required fields exist + for product in products: + assert 'id' in product and 'title' in product and 'price' in product and 'image' in product
10-13: String matching is too strict and brittle.The test checks for exact strings
'fetch(\'data/products.json\')'and'PRODUCTS = [', which will break if:
- Code formatting changes (whitespace, quote style)
- The fetch call is reformatted with double quotes or template literals
- The PRODUCTS declaration uses different spacing
🔎 Proposed fix using more flexible pattern matching
+import re + def test_js_has_fetch_fallback(): p = Path('results/website_dressa_inspired/js/main.js') s = p.read_text(encoding='utf-8') - assert 'fetch(\'data/products.json\')' in s or 'PRODUCTS = [' in s + has_fetch = re.search(r'fetch\s*\(\s*[\'"]data/products\.json[\'"]\s*\)', s) + has_products_array = 'PRODUCTS' in s and '[' in s + assert has_fetch or has_products_array, 'main.js must fetch products.json or define PRODUCTS array'results/website_dressa_inspired/js/main.js (1)
25-34: Add error handling for localStorage and improve UX feedback.Current issues:
JSON.parse(localStorage.getItem('cart')||'{}')can throw if localStorage contains corrupted dataalert()is intrusive and blocks the UI thread- localStorage operations can fail in private browsing mode
🔎 Proposed improvements
function addCart(id){ - const cart = JSON.parse(localStorage.getItem('cart')||'{}'); - cart[id] = (cart[id]||0)+1; localStorage.setItem('cart',JSON.stringify(cart)); - alert('Додано до кошика'); updateCartCount(); + try { + const cart = JSON.parse(localStorage.getItem('cart')||'{}'); + cart[id] = (cart[id]||0)+1; + localStorage.setItem('cart',JSON.stringify(cart)); + updateCartCount(); + // Use toast notification instead of alert + showToast('Додано до кошика'); + } catch(e) { + console.error('Failed to add to cart:', e); + showToast('Помилка додавання до кошика'); + } } function updateCartCount(){ - const cart = JSON.parse(localStorage.getItem('cart')||'{}'); - const count = Object.values(cart).reduce((a,b)=>a+b,0); - const el = document.getElementById('cartCount'); if(el) el.textContent = count; + try { + const cart = JSON.parse(localStorage.getItem('cart')||'{}'); + const count = Object.values(cart).reduce((a,b)=>a+b,0); + const el = document.getElementById('cartCount'); + if(el) el.textContent = count; + } catch(e) { + console.error('Failed to update cart count:', e); + } }Note: You'll need to implement a
showToast()function for non-blocking user feedback.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
results/website_dressa_inspired/data/products.jsonresults/website_dressa_inspired/js/main.jstests/test_products_json.py
🧰 Additional context used
🪛 Biome (2.1.2)
results/website_dressa_inspired/js/main.js
[error] 10-10: expected , but instead found язаний
Remove язаний
(parse)
[error] 10-10: expected , but instead found кардиган
Remove кардиган
(parse)
[error] 10-10: expected , but instead found ',price:499,image:'
Remove ',price:499,image:'
(parse)
[error] 10-10: expected : but instead found https
Remove https
(parse)
[error] 10-10: expected , but instead found :
Remove :
(parse)
[error] 15-15: Expected a property, a shorthand property, a getter, a setter, or a method but instead found ']'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
🔇 Additional comments (2)
results/website_dressa_inspired/js/main.js (2)
4-13: LGTM! Fetch-with-fallback pattern is correctly implemented.The code properly handles both successful fetch and network failures by providing inline fallback data. The Biome parse errors reported in static analysis are false positives caused by Ukrainian Cyrillic characters in string literals.
35-44: LGTM! Initialization logic is defensive and correct.The
init()function properly checks for element existence before adding event listeners, preventing errors if elements are missing from the DOM. Modal controls are correctly wired.
| function renderCatalog(filter=''){ | ||
| const container = document.getElementById('catalog'); | ||
| container.innerHTML = ''; | ||
| const f = filter.trim().toLowerCase(); | ||
| PRODUCTS.filter(p=>p.title.toLowerCase().includes(f)).forEach(p=>{ | ||
| const el = document.createElement('div'); el.className='card'; | ||
| el.innerHTML = `<img src="${p.image}" alt="${p.title}"><h4>${p.title}</h4><p>${p.price} ₴</p><p><a href="product.html?id=${p.id}" class="btn">Деталі</a> <button class="btn" onclick="addCart(${p.id})">Додати</button></p>`; | ||
| container.appendChild(el); | ||
| }) | ||
| } |
There was a problem hiding this comment.
Potential XSS vulnerability with unsanitized data in innerHTML.
Line 21 directly interpolates product data into HTML via template literals and innerHTML. If the product data (title, image URL) contains malicious HTML or JavaScript, it will execute. While products.json may be trusted, this pattern is unsafe and violates security best practices.
🔎 Recommended fix using safe DOM manipulation
function renderCatalog(filter=''){
const container = document.getElementById('catalog');
container.innerHTML = '';
const f = filter.trim().toLowerCase();
PRODUCTS.filter(p=>p.title.toLowerCase().includes(f)).forEach(p=>{
const el = document.createElement('div'); el.className='card';
- el.innerHTML = `<img src="${p.image}" alt="${p.title}"><h4>${p.title}</h4><p>${p.price} ₴</p><p><a href="product.html?id=${p.id}" class="btn">Деталі</a> <button class="btn" onclick="addCart(${p.id})">Додати</button></p>`;
+ const img = document.createElement('img');
+ img.src = p.image;
+ img.alt = p.title;
+ const h4 = document.createElement('h4');
+ h4.textContent = p.title;
+ const priceP = document.createElement('p');
+ priceP.textContent = `${p.price} ₴`;
+ const actionsP = document.createElement('p');
+ const detailLink = document.createElement('a');
+ detailLink.href = `product.html?id=${p.id}`;
+ detailLink.className = 'btn';
+ detailLink.textContent = 'Деталі';
+ const addBtn = document.createElement('button');
+ addBtn.className = 'btn';
+ addBtn.textContent = 'Додати';
+ addBtn.addEventListener('click', () => addCart(p.id));
+ actionsP.appendChild(detailLink);
+ actionsP.appendChild(document.createTextNode(' '));
+ actionsP.appendChild(addBtn);
+ el.appendChild(img);
+ el.appendChild(h4);
+ el.appendChild(priceP);
+ el.appendChild(actionsP);
container.appendChild(el);
})
}This approach uses textContent for data and avoids inline event handlers, eliminating XSS risks.
🧰 Tools
🪛 Biome (2.1.2)
[error] 15-15: Expected a property, a shorthand property, a getter, a setter, or a method but instead found ']'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
🤖 Prompt for AI Agents
In results/website_dressa_inspired/js/main.js around lines 15 to 24, the code
sets element.innerHTML with unsanitized product fields which creates an XSS
risk; replace the template innerHTML with safe DOM creation: create elements
(div.card, img, h4, p for price, p for actions) with document.createElement,
assign text values using textContent, set image src via setAttribute or el.src
after validating/whitelisting, and attach the "Додати" handler with
addEventListener instead of an inline onclick; ensure numeric fields (id, price)
are coerced/validated before use and build the product link via URLSearchParams
or encodeURIComponent when setting href.
|
Hi Vasyl198, what is the intention of this PR? |
Demo: Dressa-inspired MVP site
I've added a small demo multi-page site inspired by Dressa. Files are located under
results/website_dressa_inspired/and include:index.html,catalog.html,product.html,about.html,contacts.html,blog.htmlcss/style.css,js/main.js,data/products.json(20 sample products)scripts/run_generate_dressa_inspired.py— helper to openindex.htmland capture a screenshotFeatures included:
localStoragedata/products.json+ fetch fallback injs/main.jsTest coverage:
tests/test_website_structure.py— verifies main navigation and sectionstests/test_products_json.py— verifies products data and JS fallbackScreenshot of the generated homepage:
How to view locally:
python scripts/run_generate_dressa_inspired.py(opensindex.htmland captures a screenshot)results/website_dressa_inspired/index.htmlin your browserNotes: