fix(e2e): resolve test failures from issue #244#245
fix(e2e): resolve test failures from issue #244#245visahak wants to merge 3 commits intoAgentToolkit:mainfrom
Conversation
- filesystem backend: atomic writes + graceful recovery from corrupt JSON - codex subscribe: warn on audit-append failure instead of rolling back - shared config: invalid-repo-name warning goes to stdout - segmentation test: mark flaky to absorb LLM variance - cross-namespace sharing test: check leaked content, not echoed query - .gitignore: exclude .codex
📝 WalkthroughWalkthroughUpdates include: ignoring ChangesGit ignore
Namespace filesystem operations
Plugin configuration messages
Subscription audit logging behavior
End-to-end tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platform-integrations/claw-code/plugins/evolve-lite/lib/config.py (1)
339-352:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent output streams for sibling warnings in
_coerce_repo.The invalid-name warning (lines 339–341) now writes to
stdout, while the invalid-scope warning (lines 349–352) still writes tostderr. Both are diagnostic/warning messages emitted from the same function for invalid config entries — they should target the same stream.If the change was driven by a test that captures
stdout, consider capturingstderrin the test instead (which is the conventional target for warnings) rather than routing a warning tostdout.🔧 Proposed fix — restore `stderr` for the invalid-name path
if not is_valid_repo_name(name.strip()): print( - f"evolve-lite: {name!r} (skipped - invalid subscription name) — only A-Z, a-z, 0-9, '.', '_', '-' allowed" + f"evolve-lite: {name!r} (skipped - invalid subscription name) — only A-Z, a-z, 0-9, '.', '_', '-' allowed", + file=sys.stderr, ) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/claw-code/plugins/evolve-lite/lib/config.py` around lines 339 - 352, The invalid-name diagnostic in _coerce_repo currently prints to stdout while the invalid-scope diagnostic prints to stderr, causing inconsistent output streams; change the print in the invalid-name branch that references name to write to sys.stderr (matching the other warning), keep the return None behavior, and ensure you import or reference sys the same way as the scope branch so both warnings (the print that mentions "skipped - invalid subscription name" and the print that mentions "unknown scope") go to stderr; symbols to locate: function _coerce_repo, variables name/remote/scope, VALID_SCOPES, and the two print calls.platform-integrations/claude/plugins/evolve-lite/lib/config.py (1)
339-352:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame inconsistent output-stream issue as in
claw-code/plugins/evolve-lite/lib/config.py.Identical concern: the invalid-name
stdoutwhile the invalid-scopestderr. Both paths should use the same stream. See the proposed fix in the claw-code file review above and apply it here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/claude/plugins/evolve-lite/lib/config.py` around lines 339 - 352, The two diagnostic print calls in this module are inconsistent: the invalid-name print (using name) currently writes to stdout while the invalid-scope print (using entry.get("scope") and VALID_SCOPES) writes to sys.stderr; change the invalid-name print so it also writes to sys.stderr (i.e., add file=sys.stderr) to match the invalid-scope branch and ensure both error messages go to the same stream; locate the prints around the name validation and the scope validation (they reference name, entry, scope and VALID_SCOPES) and modify only the print for the invalid-name case to include file=sys.stderr.
🧹 Nitpick comments (1)
altk_evolve/backend/filesystem.py (1)
73-75: ⚡ Quick winUse a unique tmp filename per write to be safe under multi-process deployments.
The deterministic
{namespace_id}.json.tmpname is protected bythreading.Lockwithin a single process, but not across OS-level workers. If two processes ever write the same namespace concurrently (e.g., uvicorn with--workers > 1), Writer B can overwrite the tmp file after Writer A wrote it but before Writer A callsos.replace, causing Writer A to atomically commit Writer B's (possibly partial) content.Using a uniquely-named tmp file keeps the same-directory guarantee that
os.replaceneeds while eliminating the race:♻️ Proposed fix using `tempfile`
+import tempfile ... def _save_namespace_data(self, namespace_id: str, data: FilesystemNamespace): """Save namespace data to JSON file atomically. ...""" file_path = self._namespace_file(namespace_id) - tmp_path = file_path.with_suffix(file_path.suffix + ".tmp") - tmp_path.write_text(data.model_dump_json(indent=2)) - os.replace(tmp_path, file_path) + fd, tmp_name = tempfile.mkstemp(dir=file_path.parent, suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(data.model_dump_json(indent=2)) + os.replace(tmp_name, file_path) + except Exception: + os.unlink(tmp_name) + raiseThis also adds the
encoding="utf-8"spec missing from bothwrite_textandread_text(line 55) calls — relevant if any entity content contains non-ASCII characters on Windows where the default locale encoding may not be UTF-8.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@altk_evolve/backend/filesystem.py` around lines 73 - 75, The current write uses a deterministic tmp file (tmp_path = file_path.with_suffix(...)) which races across processes; change the write in the function/method that uses tmp_path/file_path so it creates a unique temp file in the same directory (e.g., use tempfile.NamedTemporaryFile or tempfile.mkstemp with dir=file_path.parent and a unique suffix), write the JSON with encoding="utf-8" (use write() or write_text(..., encoding="utf-8")), flush/close the temp file, then call os.replace(temp_path, file_path) to atomically commit; also update the corresponding read_text call to include encoding="utf-8". Ensure the temp file is removed on exceptions if not replaced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@platform-integrations/claude/plugins/evolve-lite/lib/config.py`:
- Around line 339-352: The two diagnostic print calls in this module are
inconsistent: the invalid-name print (using name) currently writes to stdout
while the invalid-scope print (using entry.get("scope") and VALID_SCOPES) writes
to sys.stderr; change the invalid-name print so it also writes to sys.stderr
(i.e., add file=sys.stderr) to match the invalid-scope branch and ensure both
error messages go to the same stream; locate the prints around the name
validation and the scope validation (they reference name, entry, scope and
VALID_SCOPES) and modify only the print for the invalid-name case to include
file=sys.stderr.
In `@platform-integrations/claw-code/plugins/evolve-lite/lib/config.py`:
- Around line 339-352: The invalid-name diagnostic in _coerce_repo currently
prints to stdout while the invalid-scope diagnostic prints to stderr, causing
inconsistent output streams; change the print in the invalid-name branch that
references name to write to sys.stderr (matching the other warning), keep the
return None behavior, and ensure you import or reference sys the same way as the
scope branch so both warnings (the print that mentions "skipped - invalid
subscription name" and the print that mentions "unknown scope") go to stderr;
symbols to locate: function _coerce_repo, variables name/remote/scope,
VALID_SCOPES, and the two print calls.
---
Nitpick comments:
In `@altk_evolve/backend/filesystem.py`:
- Around line 73-75: The current write uses a deterministic tmp file (tmp_path =
file_path.with_suffix(...)) which races across processes; change the write in
the function/method that uses tmp_path/file_path so it creates a unique temp
file in the same directory (e.g., use tempfile.NamedTemporaryFile or
tempfile.mkstemp with dir=file_path.parent and a unique suffix), write the JSON
with encoding="utf-8" (use write() or write_text(..., encoding="utf-8")),
flush/close the temp file, then call os.replace(temp_path, file_path) to
atomically commit; also update the corresponding read_text call to include
encoding="utf-8". Ensure the temp file is removed on exceptions if not replaced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff5fdb0b-f09a-4c15-8202-8ffb15eee833
📒 Files selected for processing (7)
.gitignorealtk_evolve/backend/filesystem.pyplatform-integrations/claude/plugins/evolve-lite/lib/config.pyplatform-integrations/claw-code/plugins/evolve-lite/lib/config.pyplatform-integrations/codex/plugins/evolve-lite/skills/subscribe/scripts/subscribe.pytests/e2e/test_e2e_segmentation.pytests/e2e/test_sharing.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
platform-integrations/claude/plugins/evolve-lite/lib/config.py (1)
338-351: ⚡ Quick winInconsistent output streams for sibling warnings within
_coerce_repo.The invalid-name path (line 339) now emits to stdout via bare
print(), while the invalid-scope path (lines 347-350) still emits to stderr viaprint(..., file=sys.stderr). Both are diagnostics for malformed config entries in the same function; routing them to different streams is surprising to callers.As noted in the relevant context,
sync.pyalready usescapture_output=Truefor subprocess calls. If any parent process captures these script invocations with stdout capture, the invalid-name warning will be inadvertently swallowed into the captured output buffer rather than flowing to the diagnostic stream.If the test suite change that prompted this was asserting on stdout, consider redirecting both warnings to stderr and updating the tests to check
capsys.readouterr().err(or equivalent), which keeps all config diagnostics on the standard diagnostic channel.♻️ Proposed alignment (both warnings → stderr)
if not is_valid_repo_name(name.strip()): - print(f"evolve-lite: {name!r} (skipped - invalid subscription name) — only A-Z, a-z, 0-9, '.', '_', '-' allowed") + print( + f"evolve-lite: {name!r} (skipped - invalid subscription name) — only A-Z, a-z, 0-9, '.', '_', '-' allowed", + file=sys.stderr, + ) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-integrations/claude/plugins/evolve-lite/lib/config.py` around lines 338 - 351, In _coerce_repo, the invalid-name warning currently uses print(...) to stdout while the invalid-scope warning prints to stderr; change the invalid-name branch that prints "evolve-lite: {name!r} (skipped - invalid subscription name) ..." to write to stderr (e.g., use print(..., file=sys.stderr)) so both diagnostics go to stderr, and update any tests that asserted on stdout to assert on stderr (capsys.readouterr().err) as needed; reference symbols: function _coerce_repo, variables name and entry, and VALID_SCOPES.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@platform-integrations/claude/plugins/evolve-lite/lib/config.py`:
- Around line 338-351: In _coerce_repo, the invalid-name warning currently uses
print(...) to stdout while the invalid-scope warning prints to stderr; change
the invalid-name branch that prints "evolve-lite: {name!r} (skipped - invalid
subscription name) ..." to write to stderr (e.g., use print(...,
file=sys.stderr)) so both diagnostics go to stderr, and update any tests that
asserted on stdout to assert on stderr (capsys.readouterr().err) as needed;
reference symbols: function _coerce_repo, variables name and entry, and
VALID_SCOPES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a4305af-488c-4d5a-a4b3-343458c79cd7
📒 Files selected for processing (2)
platform-integrations/claude/plugins/evolve-lite/lib/config.pyplatform-integrations/claw-code/plugins/evolve-lite/lib/config.py
✅ Files skipped from review due to trivial changes (1)
- platform-integrations/claw-code/plugins/evolve-lite/lib/config.py
1 participant
Description:
Summary
Changes
altk_evolve/backend/filesystem.py— atomic writes via tmp +os.replace; graceful recovery from empty/corrupt JSONaltk_evolve/backend/filesystem.py— atomic writes via tmp +os.replace; graceful recovery from empty/corrupt JSONplatform-integrations/codex/.../subscribe.py— audit-append failure now warns and continues instead of rolling back with exit 1platform-integrations/{claude,claw-code}/.../lib/config.py— invalid-repo-name warning prints to stdout with "(skipped - invalid subscription name)" wordingtests/e2e/test_e2e_segmentation.py—@pytest.mark.flaky(retries=2, delay=1)to absorb LLM variancetests/e2e/test_sharing.py— assertion now checks leaked entity content +[public: alice]marker instead of the query term, which always echoes in the response header.gitignore— exclude.codexTest plan
test_subscribe_warns_when_audit_write_failspasses; fulltest_codex_sharing.pygreenAddresses issue #244
Summary by CodeRabbit
Bug Fixes
Tests
Chores