feat: separate Nova and Marmot device groups - #1
Conversation
📝 WalkthroughWalkthroughThe agent now supports NOVA and MARMOT command groups, separate common and group command sources, persistent SHA-256 state, group-specific scripts, URL validation, and atomic state writes. Tests cover fetching, group changes, hash skips, and invalid groups. ChangesDevice Group Commands
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Listener
participant State
participant CommandSources
participant CommandProcessor
Listener->>State: load group and command state
Listener->>CommandSources: fetch common and group commands
CommandSources-->>Listener: return command text
Listener->>State: save hashes and timestamps
Listener->>CommandProcessor: process changed commands
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
tests/test_agent.py (3)
13-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister the patch cleanup with
addCleanup.
setUpstarts 13 patchers andtearDownstops them. If any line insetUpraises,tearDowndoes not run.builtins.openandos.systemthen stay patched for the rest of the session, and later tests fail in ways that hide the original error.Call
self.addCleanup(mock.patch.stopall)as the first statement ofsetUp.unittestruns registered cleanups even whensetUpfails.♻️ Proposed refactor
def setUp(self): + self.addCleanup(mock.patch.stopall) self.group_content = '' self.open_mock = mock.mock_open()- def tearDown(self): - mock.patch.stopall() -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent.py` around lines 13 - 50, Register mock.patch.stopall with self.addCleanup as the first statement in setUp, before any patcher is started, so all patches are cleaned up even when setup fails; keep tearDown unchanged unless necessary.
72-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the requested URLs, not only the response order.
self.mock_urlopen.side_effectreturns responses in call order and ignores the URL.test_nova_uses_common_and_novatherefore passes even iffetch_commands_for_grouprequested the MARMOT URL. The test does not verify group separation, which is the goal of this PR.
urllib.request.Requestis already patched asself.mock_request. Assert on its call arguments.♻️ Proposed refactor
self.assertEqual(agent.get_device_group(), 'NOVA') common, group = agent.fetch_commands_for_group('NOVA') self.assertEqual(common, 'COMMON_CMD\n') self.assertEqual(group, 'NOVA_CMD\n') + requested = [call.args[0] for call in self.mock_request.call_args_list] + self.assertTrue(requested[0].startswith(agent.COMMON_COMMAND_URL)) + self.assertTrue(requested[1].startswith(agent.GROUP_COMMAND_URLS['NOVA']))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent.py` around lines 72 - 88, Update test_nova_uses_common_and_nova and test_marmot_uses_common_and_marmot to assert the URLs passed through the patched self.mock_request, including the shared COMMON endpoint and the correct group-specific NOVA or MARMOT endpoint. Keep the existing response assertions, but verify request arguments so each test confirms fetch_commands_for_group selects the requested group URL rather than relying on response order.
8-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
SourceFileLoader.load_module()for future Python compatibility.
load_module()is deprecated and will fail on later Python releases. Useimportlib.util.spec_from_file_location(),importlib.util.module_from_spec(), register the module insys.modules, then callspec.loader.exec_module().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent.py` around lines 8 - 10, Replace the deprecated SourceFileLoader.load_module() usage in the test module-loading setup with importlib.util.spec_from_file_location(), module_from_spec(), sys.modules registration, and spec.loader.exec_module(). Preserve loading the existing agent module from agent_path under the same "agent" name.agent (4)
59-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the default state into one helper.
The default state dictionary appears three times. A new state key must then be added in three places. Extract a
default_state()helper and reuse it.♻️ Proposed refactor
+def default_state(): + return { + "device_group": "", + "common_command_hash": "", + "group_command_hash": "", + "last_processed_at": "", + } + + def load_state(): ensure_parent_dir(STATE_PATH) if not os.path.exists(STATE_PATH): - return { - "device_group": "", - "common_command_hash": "", - "group_command_hash": "", - "last_processed_at": "", - } + return default_state() try: with open(STATE_PATH, "r", encoding="utf-8") as f: data = json.load(f) - return { - "device_group": data.get("device_group", ""), - "common_command_hash": data.get("common_command_hash", ""), - "group_command_hash": data.get("group_command_hash", ""), - "last_processed_at": data.get("last_processed_at", ""), - } + state = default_state() + for key in state: + state[key] = data.get(key, "") + return state except Exception as e: log_error("load_state", e) - return { - "device_group": "", - "common_command_hash": "", - "group_command_hash": "", - "last_processed_at": "", - } + return default_state()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent` around lines 59 - 84, Extract the repeated default state dictionary from load_state into a default_state() helper. Replace the missing-file and exception fallback returns with calls to default_state(), while preserving the existing normalized return for successfully loaded data.
121-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
is_valid_urlhere.
fetch_textandis_valid_urlimplement the same check, butis_valid_urlalso rejects\nand\r. The two validators can then diverge. Callis_valid_urlfromfetch_textand keep one implementation. Move theis_valid_urldefinition abovefetch_text.♻️ Proposed refactor
- parsed = urllib.parse.urlparse(url) - if parsed.scheme not in ("http", "https") or not parsed.netloc: + if not is_valid_url(url): raise ValueError(f"Invalid URL: {url}") + parsed = urllib.parse.urlparse(url)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent` around lines 121 - 123, Move the is_valid_url definition above fetch_text, then update fetch_text to call is_valid_url(url) instead of duplicating urllib.parse.urlparse validation. Preserve the existing ValueError behavior and invalid-URL message while ensuring fetch_text uses the validator’s newline and carriage-return checks.
429-437: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFilter comment lines out of the command list.
lenh_all.txt,lenh_nova.txt, andlenh_marmot.txtall begin with#comment lines. Line 435 keeps them. The count printed on line 437 therefore overstates the number of real commands.lenh_nova.txtandlenh_marmot.txtcurrently contain only comments, so the agent reports 3 commands and runs none.The dispatch loop ignores unknown lines, so no incorrect command runs today. Skip lines that start with
#.♻️ Proposed refactor
- commands = [cmd.strip() for cmd in command_lines if cmd.strip()] - commands = [cmd.strip() for cmd in command_lines if cmd.strip()] + commands = [ + line.strip() + for line in command_lines + if line.strip() and not line.strip().startswith("#") + ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent` around lines 429 - 437, Update the command filtering in the block that builds commands from command_lines so entries whose trimmed text starts with “#” are excluded, while retaining non-empty command lines. Ensure the printed count in the commands discovery message reflects only executable commands and preserve the existing dispatch behavior.
140-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or consume
get_command_sources.
get_command_sourcesis not referenced in the repository. Keep it only if another caller or test will use it; otherwise removesources.append((group, GROUP_COMMAND_URLS[group]))andfetch_commands_for_groupfalls back toCOMMON_COMMAND_URL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent` around lines 140 - 144, Remove the unused get_command_sources function and its group-specific source assembly unless a repository caller or test requires it; ensure fetch_commands_for_group continues using COMMON_COMMAND_URL as its fallback when no group-specific source is consumed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent`:
- Around line 435-436: Remove the duplicate commands assignment in the command
parsing block, keeping only one list comprehension that strips and filters
command_lines.
- Around line 91-93: Update the temporary state-file write around json.dump and
os.replace to call f.flush() followed by os.fsync(f.fileno()) before leaving the
with block and renaming the file. Preserve the existing atomic replacement flow.
- Around line 105-107: Update get_device_group’s missing GROUP_PATH branch to
avoid calling log_error on every polling cycle; print the missing-file message
or otherwise report it only once, while preserving the existing None return
behavior.
- Around line 405-409: When the device group changes in the state-update block,
reset both stored command hashes, including group_command_hash and the
common-command hash, to empty strings before saving state so the new group’s
commands are evaluated.
- Around line 424-427: Move the save_state call out of the pre-dispatch section
and place it after the command dispatch loop completes, so hashes are persisted
only after all commands execute successfully. Keep the in-memory
common_command_hash, group_command_hash, and last_processed_at updates before
dispatch to preserve fetch-failure behavior.
- Around line 194-204: Update install_custom_script to reject script_url values
containing quote or backslash characters before constructing the Lua payload,
while preserving existing URL validation. Match install_group_script’s
cache-busting behavior when generating the fetched script URL so custom scripts
do not execute stale cached content.
- Line 426: Update the exception logging path that populates exception_to_log to
pass str(exception_obj) into the fixed-format JSON list, rather than the raw
exception object or unbound traceback text, so Agent_Error logging remains
JSON-safe.
In `@tests/test_agent.py`:
- Around line 95-113: Replace the self-asserting tests
test_hash_unchanged_skips_processing and test_group_change_resets_hash with
tests that exercise extracted helpers from the __main__ flow. Add
apply_group_change to update the device group, clear both hashes, and report
whether a change occurred; add should_skip to compare both hashes against state.
Update the main flow to use these helpers, then assert the unchanged-hash skip
behavior and group-change reset behavior through their return values and state
mutations.
---
Nitpick comments:
In `@agent`:
- Around line 59-84: Extract the repeated default state dictionary from
load_state into a default_state() helper. Replace the missing-file and exception
fallback returns with calls to default_state(), while preserving the existing
normalized return for successfully loaded data.
- Around line 121-123: Move the is_valid_url definition above fetch_text, then
update fetch_text to call is_valid_url(url) instead of duplicating
urllib.parse.urlparse validation. Preserve the existing ValueError behavior and
invalid-URL message while ensuring fetch_text uses the validator’s newline and
carriage-return checks.
- Around line 429-437: Update the command filtering in the block that builds
commands from command_lines so entries whose trimmed text starts with “#” are
excluded, while retaining non-empty command lines. Ensure the printed count in
the commands discovery message reflects only executable commands and preserve
the existing dispatch behavior.
- Around line 140-144: Remove the unused get_command_sources function and its
group-specific source assembly unless a repository caller or test requires it;
ensure fetch_commands_for_group continues using COMMON_COMMAND_URL as its
fallback when no group-specific source is consumed.
In `@tests/test_agent.py`:
- Around line 13-50: Register mock.patch.stopall with self.addCleanup as the
first statement in setUp, before any patcher is started, so all patches are
cleaned up even when setup fails; keep tearDown unchanged unless necessary.
- Around line 72-88: Update test_nova_uses_common_and_nova and
test_marmot_uses_common_and_marmot to assert the URLs passed through the patched
self.mock_request, including the shared COMMON endpoint and the correct
group-specific NOVA or MARMOT endpoint. Keep the existing response assertions,
but verify request arguments so each test confirms fetch_commands_for_group
selects the requested group URL rather than relying on response order.
- Around line 8-10: Replace the deprecated SourceFileLoader.load_module() usage
in the test module-loading setup with importlib.util.spec_from_file_location(),
module_from_spec(), sys.modules registration, and spec.loader.exec_module().
Preserve loading the existing agent module from agent_path under the same
"agent" name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68fbcde4-19e6-40f9-a567-bc2dacb58df6
📒 Files selected for processing (6)
.gitignoreagentlenh_all.txtlenh_marmot.txtlenh_nova.txttests/test_agent.py
| with open(tmp_path, "w", encoding="utf-8") as f: | ||
| json.dump(state, f, ensure_ascii=False, indent=2) | ||
| os.replace(tmp_path, STATE_PATH) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Flush and fsync the temporary file before the rename.
os.replace makes the rename atomic, but the data of tmp_path can still be in the page cache. The agent runs a REBOOT command, so an abrupt power cut can leave agent_state.json empty or truncated. The agent then re-runs the full command set. Call f.flush() and os.fsync(f.fileno()) before the file closes.
🛡️ Proposed fix
with open(tmp_path, "w", encoding="utf-8") as f:
json.dump(state, f, ensure_ascii=False, indent=2)
+ f.flush()
+ os.fsync(f.fileno())
os.replace(tmp_path, STATE_PATH)📝 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.
| with open(tmp_path, "w", encoding="utf-8") as f: | |
| json.dump(state, f, ensure_ascii=False, indent=2) | |
| os.replace(tmp_path, STATE_PATH) | |
| with open(tmp_path, "w", encoding="utf-8") as f: | |
| json.dump(state, f, ensure_ascii=False, indent=2) | |
| f.flush() | |
| os.fsync(f.fileno()) | |
| os.replace(tmp_path, STATE_PATH) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent` around lines 91 - 93, Update the temporary state-file write around
json.dump and os.replace to call f.flush() followed by os.fsync(f.fileno())
before leaving the with block and renaming the file. Preserve the existing
atomic replacement flow.
| if not os.path.exists(GROUP_PATH): | ||
| log_error("get_device_group", FileNotFoundError(GROUP_PATH)) | ||
| return None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not call log_error for the missing group file on every poll.
Two problems exist here.
- The listener loop calls
get_device_groupevery 15 seconds. Ifdevice_group.txtis absent, this appends an entry to/sdcard/Download/Agent_Error.txtevery 15 seconds. The log file grows without a bound on device storage. log_errorcallstraceback.format_exc(). Line 106 runs outside anexceptblock, so the recorded traceback isNoneType: None. The entry carries no diagnostic value.
Print the message instead, or log it only on the first occurrence.
🛡️ Proposed fix
if not os.path.exists(GROUP_PATH):
- log_error("get_device_group", FileNotFoundError(GROUP_PATH))
+ print(f"[!] get_device_group: missing {GROUP_PATH}")
return None📝 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.
| if not os.path.exists(GROUP_PATH): | |
| log_error("get_device_group", FileNotFoundError(GROUP_PATH)) | |
| return None | |
| if not os.path.exists(GROUP_PATH): | |
| print(f"[!] get_device_group: missing {GROUP_PATH}") | |
| return None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent` around lines 105 - 107, Update get_device_group’s missing GROUP_PATH
branch to avoid calling log_error on every polling cycle; print the missing-file
message or otherwise report it only once, while preserving the existing None
return behavior.
| if not is_valid_url(script_url): | ||
| log_error("install_custom_script", ValueError(f"Invalid URL: {script_url}")) | ||
| return | ||
| try: | ||
| os.makedirs(AUTOEXECUTE_DIR, exist_ok=True) | ||
| target_file = os.path.join(AUTOEXECUTE_DIR, "main_farm_script.lua") | ||
| if os.path.exists(target_file): | ||
| os.remove(target_file) | ||
| lua_body = f'loadstring(game:HttpGet("{script_url}"))()' | ||
| with open(target_file, "w", encoding="utf-8") as f: | ||
| f.write(lua_body + "\n") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject quote characters in script_url before the Lua interpolation.
script_url arrives from the remote Updatescript <link> command. is_valid_url accepts " and \, and line 202 embeds the value directly inside a Lua double-quoted string. A URL such as https://host/a")) print("x closes the string literal and appends arbitrary Lua to the autoexecute file. The command source is the same repository that ships the agent, so this is a hardening gap and not a direct external exploit. The fix is small.
install_group_script also appends a cache-busting token. install_custom_script does not, so Delta can execute a cached copy of the script.
🔒️ Proposed fix
def is_valid_url(url):
- if "\n" in url or "\r" in url:
+ if any(c in url for c in ('\n', '\r', '"', '\\')):
return False- lua_body = f'loadstring(game:HttpGet("{script_url}"))()'
+ sep = "&" if "?" in script_url else "?"
+ lua_body = f'loadstring(game:HttpGet("{script_url}{sep}t=" .. tostring(os.time())))()'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent` around lines 194 - 204, Update install_custom_script to reject
script_url values containing quote or backslash characters before constructing
the Lua payload, while preserving existing URL validation. Match
install_group_script’s cache-busting behavior when generating the fetched script
URL so custom scripts do not execute stale cached content.
| if group != state.get("device_group"): | ||
| print(f"[+] Thay đổi device group hoặc lần chạy đầu: {group}") | ||
| state["device_group"] = group | ||
| install_group_script(group) | ||
| save_state(state) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset the stored command hashes when the device group changes.
On a group change, the code updates device_group and installs the new script, but it keeps group_command_hash from the previous group. The stored hash then describes a different group's command file.
If the new group's command file produces the same hash as the previous group's stored hash, and the common file is unchanged, the check on line 420 skips the new group's commands. The device then never runs them.
tests/test_agent.py::test_group_change_resets_hash asserts that both hashes reset to '' on a group change. The production code does not perform that reset. The test builds its own dictionary, so it passes without exercising this path.
🐛 Proposed fix
if group != state.get("device_group"):
print(f"[+] Thay đổi device group hoặc lần chạy đầu: {group}")
state["device_group"] = group
+ state["common_command_hash"] = ""
+ state["group_command_hash"] = ""
install_group_script(group)
save_state(state)📝 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.
| if group != state.get("device_group"): | |
| print(f"[+] Thay đổi device group hoặc lần chạy đầu: {group}") | |
| state["device_group"] = group | |
| install_group_script(group) | |
| save_state(state) | |
| if group != state.get("device_group"): | |
| print(f"[+] Thay đổi device group hoặc lần chạy đầu: {group}") | |
| state["device_group"] = group | |
| state["common_command_hash"] = "" | |
| state["group_command_hash"] = "" | |
| install_group_script(group) | |
| save_state(state) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent` around lines 405 - 409, When the device group changes in the
state-update block, reset both stored command hashes, including
group_command_hash and the common-command hash, to empty strings before saving
state so the new group’s commands are evaluated.
| state["common_command_hash"] = common_hash | ||
| state["group_command_hash"] = group_hash | ||
| state["last_processed_at"] = datetime.datetime.utcnow().strftime("%Y-%m-%d %H:%M:%S UTC") | ||
| save_state(state) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the hashes after the commands run, not before.
save_state runs on line 427, before the dispatch loop on line 439. The REBOOT command on lines 468-473 restarts the device in the middle of that loop. Every command that follows REBOOT in the command file never runs. On restart the hashes already match, so the loop skips the set and never retries.
The same loss occurs for any crash or process kill during command execution.
Move the save_state call to after the dispatch loop. Keep the in-memory hash update where it is, so a fetch failure does not re-trigger the set.
🐛 Proposed fix
state["common_command_hash"] = common_hash
state["group_command_hash"] = group_hash
state["last_processed_at"] = datetime.datetime.utcnow().strftime("%Y-%m-%d %H:%M:%S UTC")
- save_state(state)Then persist after the dispatch loop completes:
print("\n[*] ĐÃ THỰC HIỆN XONG TOÀN BỘ LỆNH. CHỜ VIỆC TIẾP THEO...\n")
+ save_state(state)
time.sleep(15)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent` around lines 424 - 427, Move the save_state call out of the
pre-dispatch section and place it after the command dispatch loop completes, so
hashes are persisted only after all commands execute successfully. Keep the
in-memory common_command_hash, group_command_hash, and last_processed_at updates
before dispatch to preserve fetch-failure behavior.
|
|
||
| state["common_command_hash"] = common_hash | ||
| state["group_command_hash"] = group_hash | ||
| state["last_processed_at"] = datetime.datetime.utcnow().strftime("%Y-%m-%d %H:%M:%S UTC") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is datetime.datetime.utcnow deprecated in Python 3.12 and when will it be removed?
💡 Result:
Yes, datetime.datetime.utcnow is deprecated as of Python 3.12 [1][2]. There is currently no specific date or version scheduled for its removal [3]. It is listed among the APIs that will be removed in a "future version," but the Python development team has explicitly noted that there is no deadline for this removal [4][3]. While the official documentation recommends using datetime.now(datetime.UTC) as an alternative [1], it is important to note that this is not a direct, backwards-compatible replacement because it returns a timezone-aware datetime object, whereas utcnow returned a naive one [5][6][7]. If your application requires a naive datetime object to maintain existing behavior or compatibility, the recommended approach is to use datetime.now(datetime.UTC).replace(tzinfo=None) [5][7].
Citations:
- 1: https://docs.python.org/3/library/datetime.html
- 2: python/cpython@0b7fd8f
- 3: https://github.com/python/cpython/blob/main/Doc/deprecations/pending-removal-in-future.rst
- 4: Deprecate
utcnowandutcfromtimestamppython/cpython#103857 - 5: GH-103857: Deprecate utcnow and utcfromtimestamp python/cpython#103858
- 6:
utcnowdeprecation note is misleading python/cpython#118542 - 7: datetime utcnow deprecation leads to type confusion python/cpython#105544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files named agent/log_error matching context:\n'
fd -a '(^agent$|.*log_error.*)' . | sed 's#^\./##' | head -100
printf '\nSearch for dt.datetime.utcnow/getutcnow usage:\n'
rg -n "datetime\.datetime\\.utcn(ow|ow)\b|datetime\.datetime\.now\(datetime\.timezone\.utc\)|log_error" . --glob '!.*\.pyc' | head -200
printf '\nRelevant snippets:\n'
for f in $(rg -l "datetime\.datetime\.utcnow|def log_error" . --glob '!.*\.pyc'); do
echo "--- $f"
wc -l "$f"
rg -n -C 4 "datetime\.datetime\.utcnow|def log_error|last_processed_at" "$f" || true
done
printf '\nLanguage/runtime hints:\n'
fd -a '(^pyproject\.toml$|^setup\.py$|^setup\.cfg$|^requirements.*\.txt$|^Pipfile$|^poetry\.lock$|^Dockerfile$|\.python-version$)' . | sed 's#^\./##' | while read -r f; do
echo "--- $f"
sed -n '1,220p' "$f"
doneRepository: tinhpr9/Aotscript
Length of output: 3694
Replace non-decoded error logging with string representation.
traceback.format_exc() returns a string, but exception_to_log is later joined into a fixed JSON list, so unbound traceback text can break Agent_Error logging. Pass str(exception_obj) to the fixed-format log path instead of using the exception object as a JSON value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent` at line 426, Update the exception logging path that populates
exception_to_log to pass str(exception_obj) into the fixed-format JSON list,
rather than the raw exception object or unbound traceback text, so Agent_Error
logging remains JSON-safe.
| commands = [cmd.strip() for cmd in command_lines if cmd.strip()] | ||
| commands = [cmd.strip() for cmd in command_lines if cmd.strip()] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicate assignment on line 436.
Line 436 repeats line 435 exactly. The second statement recomputes the same list and discards the first.
🐛 Proposed fix
commands = [cmd.strip() for cmd in command_lines if cmd.strip()]
- commands = [cmd.strip() for cmd in command_lines if cmd.strip()]📝 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.
| commands = [cmd.strip() for cmd in command_lines if cmd.strip()] | |
| commands = [cmd.strip() for cmd in command_lines if cmd.strip()] | |
| commands = [cmd.strip() for cmd in command_lines if cmd.strip()] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent` around lines 435 - 436, Remove the duplicate commands assignment in
the command parsing block, keeping only one list comprehension that strips and
filters command_lines.
| def test_hash_unchanged_skips_processing(self): | ||
| state = { | ||
| 'device_group': 'NOVA', | ||
| 'common_command_hash': agent.calculate_sha256('A'), | ||
| 'group_command_hash': agent.calculate_sha256('B'), | ||
| 'last_processed_at': '' | ||
| } | ||
| self.assertEqual(state['common_command_hash'], agent.calculate_sha256('A')) | ||
| self.assertEqual(state['group_command_hash'], agent.calculate_sha256('B')) | ||
|
|
||
| def test_group_change_resets_hash(self): | ||
| state = {'device_group': 'NOVA', 'common_command_hash': 'old', 'group_command_hash': 'old'} | ||
| new_group = 'MARMOT' | ||
| self.assertNotEqual(new_group, state['device_group']) | ||
| state['device_group'] = new_group | ||
| state['common_command_hash'] = '' | ||
| state['group_command_hash'] = '' | ||
| self.assertEqual(state['common_command_hash'], '') | ||
| self.assertEqual(state['group_command_hash'], '') |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
These two tests assert their own local assignments.
test_hash_unchanged_skips_processing builds state and then asserts that state['common_command_hash'] equals the value assigned on line 98. test_group_change_resets_hash assigns '' to both hash keys and then asserts they are ''. Neither test calls any function from agent. Both always pass.
The skip decision lives on line 420 of agent, and the group-change handling lives on lines 405-409. Neither path is covered. agent does not reset the hashes on a group change, so the behavior that test_group_change_resets_hash describes is not implemented.
Extract the skip and group-change logic from the __main__ block into a function, then test that function.
♻️ Suggested direction in `agent`
def apply_group_change(state, group):
if group == state.get("device_group"):
return False
state["device_group"] = group
state["common_command_hash"] = ""
state["group_command_hash"] = ""
return True
def should_skip(state, common_hash, group_hash):
return (
common_hash == state.get("common_command_hash")
and group_hash == state.get("group_command_hash")
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_agent.py` around lines 95 - 113, Replace the self-asserting tests
test_hash_unchanged_skips_processing and test_group_change_resets_hash with
tests that exercise extracted helpers from the __main__ flow. Add
apply_group_change to update the device group, clear both hashes, and report
whether a change occurred; add should_skip to compare both hashes against state.
Update the main flow to use these helpers, then assert the unchanged-hash skip
behavior and group-change reset behavior through their return values and state
mutations.
Summary by CodeRabbit
New Features
Bug Fixes