Skip to content

test(context): skip symlink test without Windows privilege - #908

Merged
ZohaibHassan16 merged 4 commits into
semantica-agi:mainfrom
ssynb:test/windows-symlink-privilege
Aug 14, 2026
Merged

test(context): skip symlink test without Windows privilege#908
ZohaibHassan16 merged 4 commits into
semantica-agi:mainfrom
ssynb:test/windows-symlink-privilege

Conversation

@ssynb

@ssynb ssynb commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Skip the markdown symlink-safety test on Windows only when the OS reports that the current process lacks the privilege required to create a symbolic link.

Problem

test_markdown_export_rejects_symlink_without_touching_target fails before exercising AgentMemory.export() on Windows environments without Developer Mode or SeCreateSymbolicLinkPrivilege:

OSError: [WinError 1314] A required privilege is not held by the client

That is an environment capability failure, not a failure of the export safety behavior under test.

Changes

  • catch the Windows-specific winerror == 1314 from Path.symlink_to
  • skip only in that case
  • re-raise every other OSError, preserving genuine failures

Testing

uv run --no-project --python 3.12 --with pytest --with networkx --with numpy --with pydantic --with scipy --with scikit-learn --with pyyaml --with requests --with python-dateutil --with rdflib --with gensim pytest tests/context/test_agent_memory_markdown.py tests/context/test_context_retriever_hybrid.py tests/context/test_query_result_unwrap.py -q

Result: 73 passed, 1 skipped in 2.15s.

Also ran git diff --check.

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Skip Windows symlink test when symlink privilege is unavailable

🐞 Bug fix 🧪 Tests 🕐 Less than 10 minutes

Grey Divider

AI Description

• Skip the markdown symlink-safety test on Windows when symlink privilege is missing.
• Re-raise non-privilege symlink errors to avoid masking real failures.
• Preserve the original export safety assertions when symlink creation succeeds.
Diagram

graph TD
  T["Pytest symlink test"] --> S["Path.symlink_to()"] --> D{"WinError 1314 on win32?"}
  D -->|"yes"| K["pytest.skip()"]
  D -->|"no"| E["AgentMemory.export()"] --> R["ValueError asserted"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Capability probe fixture
  • ➕ Reusable across the test suite for any symlink-dependent tests
  • ➕ Keeps per-test bodies focused on assertions
  • ➖ Adds indirection/complexity for a single failing test today
  • ➖ Still needs Windows-specific error handling internally
2. pytest.mark.skipif(sys.platform == 'win32')
  • ➕ Simplest implementation; zero runtime branching
  • ➕ Eliminates Windows CI flakiness entirely
  • ➖ Over-skips: hides real regressions on Windows environments where symlinks are supported
  • ➖ Reduces platform coverage unnecessarily

Recommendation: The current approach is the best tradeoff: it skips only on the specific Windows privilege failure (WinError 1314) while preserving coverage and re-raising all other OSErrors so genuine setup/test regressions are not masked. A shared capability fixture could be considered later if more symlink tests appear.

Files changed (1) +7 / -1

Tests (1) +7 / -1
test_agent_memory_markdown.pySkip symlink safety test on Windows when symlink privilege is missing +7/-1

Skip symlink safety test on Windows when symlink privilege is missing

• Wraps symlink creation in a try/except to catch Windows-specific WinError 1314 and skip the test only in that case. All other OSError instances are re-raised so unexpected failures remain visible.

tests/context/test_agent_memory_markdown.py

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 11, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Informational

1. Magic Windows error code ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The new skip condition relies on the hard-coded WinError value 1314 without naming/documenting
what it represents, making the test intent harder to understand and maintain. This increases the
risk of accidental mis-edits (e.g., changing/removing the check) when troubleshooting Windows CI
failures.
Code

tests/context/test_agent_memory_markdown.py[R650-651]

+        if sys.platform == "win32" and error.winerror == 1314:
+            pytest.skip("Windows symlink creation requires an unavailable privilege")
Evidence
The added code introduces a platform-specific skip keyed on an unexplained literal 1314, which
obscures that this corresponds to the Windows “privilege not held” error for symlink creation.

tests/context/test_agent_memory_markdown.py[647-652]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test uses the magic number `1314` to detect the Windows symlink privilege error, but the meaning of this value is not obvious from the code.
## Issue Context
This is a readability/maintainability issue in the newly added Windows-specific skip logic.
## Fix Focus Areas
- tests/context/test_agent_memory_markdown.py[647-652]
## Suggested fix
- Define a local constant near the test (or near imports), e.g. `ERROR_PRIVILEGE_NOT_HELD = 1314`.
- Update the condition to compare against the named constant and optionally add a short comment like `# ERROR_PRIVILEGE_NOT_HELD`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread tests/context/test_agent_memory_markdown.py Outdated
@ZohaibHassan16

Copy link
Copy Markdown
Collaborator

Please address the Qodo finding , define a named constant for 1314 (e.g. _ERROR_PRIVILEGE_NOT_HELD = 1314) instead of the bare number.

Also sys is missing from the imports at the top of the file , add import sys alongside the existing import errno line otherwise the sys.platform check will raise a NameError on every platform.

@ssynb

ssynb commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in 7192ad6: added the named _ERROR_PRIVILEGE_NOT_HELD constant and updated the comparison. I also confirmed that import sys is present alongside import errno in the current PR head (it was already included in the original commit). Local verification on Python 3.12: 45 passed, 1 skipped; Black and isort checks pass.

@ZohaibHassan16
ZohaibHassan16 self-requested a review August 14, 2026 04:47
@ZohaibHassan16

Copy link
Copy Markdown
Collaborator

Thank you. LGTM

@ZohaibHassan16 ZohaibHassan16 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved.

@ZohaibHassan16
ZohaibHassan16 merged commit 09c4b1b into semantica-agi:main Aug 14, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants