* fix(skill-importer): validate URL scheme and improve skills.sh handling * fix(skill-importer): enhance DNS resolution and SSRF protection in fetch URL handling * fix(url-safety): add allowed_dist parameter to check_outbound_url for flexible private blocking * test(skill-importer): add comprehensive tests for URL parsing and outbound checks * ensure newline at end of file in test_check_outbound_url_allows_public_ip * fix(skill-importer): improve TLS certificate handling in _get_checked function * fix(skill-importer): enhance _check_fetch_url to handle both hostnames and full URLs * fix(skill-importer): enhance parse_skill_source to support skills.sh URLs in path and netloc * fix(skill-importer): simplify skills.sh hostname check in parse_skill_source * fix(skill-importer): enhance parse_skill_source to identify skills.sh URLs in path and handle localhost/IP addresses * fix(skill-importer): enhance _resolve_and_check_url to validate all resolved IP addresses and prevent TOCTOU vulnerabilities * fix(skill-importer): enhance parse_skill_source to support schemeless GitHub and skills.sh URLs * fix(memory): resolve CodeQL URL sanitization warning and restore _check_fetch_url test alias * fix(memory): pin skill fetch sockets without rewriting URLs * fix(memory): reject unsupported skill wrapper hosts * refactor(url-safety): remove unused importer exception * test(memory): keep redirect regression hermetic * test(dns-rebinding): add test for _PinnedTransport to ensure connection to pinned IP * fix(skill-importer): enhance skills.sh support to extract GitHub links from page content * fix(skill-importer): improve URL scheme validation for GitHub and skills.sh links * fix(skills): reject unusable skill URLs instead of guessing Resolving a skills.sh link by scraping the first github.com URL out of the page body cannot work. Skill pages only ever link the repository root, never the skill's subdirectory, so every skill in a repo resolved to the same bundle: importing skills.sh/anthropics/skills/pdf walked the whole monorepo, saturated the 64-file cap, and installed algorithmic-art behind an ok:true response. Restore the redirect-target unwrap and fail with a message that says what to do instead. Also report the real reason a URL is rejected. The scheme check keyed off "://" appearing anywhere in the string, so a supplied-but-unusable URL came back as "URL is required", and a schemeless URL carrying "://" in its query was reported as an unsupported scheme. Key off the parsed scheme and let opaque schemes (mailto:, javascript:) and a schemeless host:port fall through to the host check. * test(skills): tighten the real-socket pinning regression The handler swallowed its own exceptions, so a failure inside it surfaced as a confusing assertion on the captured client address. Record the exception and assert on it, run the thread as a daemon, and close the listening socket from the test so a hang cannot outlive the run. Also drop the duplicate ipaddress import and the missing newline. * fix(skills): require exact GitHub skill URLs * test(skills): read complete pinned request headers --------- Co-authored-by: RaresKeY <158580472+RaresKeY@users.noreply.github.com> Co-authored-by: Léo <leograndcontact@gmail.com>
Test Suite Notes
Purpose
This file documents the shared test helpers and the review expectations that go
with them. The suite is being refactored incrementally, so this is a working
reference for that effort - not a claim that the suite is already fully
organized. Read it before adding a new helper or before reviewing a PR that
touches tests/helpers/.
For the broader rules - test taxonomy, determinism/isolation rules, the
behavioral-vs-source-text policy, and helper/factory extraction rules - see
TESTING_STANDARD.md. This file is the concrete helper
reference; that file is the standard the refactor works toward.
Running focused subsets (taxonomy markers)
tests/conftest.py tags every test at collection time with two markers derived
from its filename by tests/_taxonomy.py: an area_* marker (e.g.
area_security) and a finer sub_* marker (e.g. sub_owner_scope). This adds
markers only - it moves no files and changes no test behavior. Use them to run a
focused slice:
./venv/bin/python -m pytest -m area_security
./venv/bin/python -m pytest -m "area_services and sub_cookbook"
Areas are security, routes, services, cli, js, helpers, unit, and
uncategorized. Classification is conservative and token-based: a file that
matches no area keyword falls back to area_uncategorized with its filename as
the sub-area. The area_* names are registered in pyproject.toml; the dynamic
sub_* names are registered before collection by pytest_configure in
tests/conftest.py, so unknown-mark warnings still flag genuine typos.
For common focused runs, use tests/run_focus.py. It validates area and
sub-area names, accepts sub-areas with or without the sub_ prefix, and passes
extra pytest arguments after --:
./venv/bin/python tests/run_focus.py --area security
./venv/bin/python tests/run_focus.py --area services --sub-area cookbook
./venv/bin/python tests/run_focus.py --sub-area sub_cookbook
./venv/bin/python tests/run_focus.py --keyword taxonomy
./venv/bin/python tests/run_focus.py --last-failed
./venv/bin/python tests/run_focus.py --dry-run --area services --sub-area cookbook
./venv/bin/python tests/run_focus.py --area services -- --maxfail=1 -q
Fast lane and duration visibility
--fast runs the fast lane: the tests that are not marked slow (it adds the
marker expression not slow). It composes with --area/--sub-area using
and. Because no tests may be marked slow yet, --fast can initially match
the full focused selection; it becomes a real speed-up as slow marks are added
from duration evidence. Use it for quick local or reviewer feedback; it does not
replace broader focused or full-suite validation before merge.
--durations N and --durations-min FLOAT add pytest's slowest-test reporting
so you can see where time goes. They are reporting only and do not count as a
focus selector, so --durations must be combined with a real selector
(--area, --sub-area, --keyword, --last-failed, or --fast).
Use the project Python environment before running these commands. The examples
use the repo's documented ./venv/bin/python path so they do not accidentally
fall back to system Python.
./venv/bin/python tests/run_focus.py --fast
./venv/bin/python tests/run_focus.py --area services --fast
./venv/bin/python tests/run_focus.py --area services --durations 25
./venv/bin/python tests/run_focus.py --area services --fast --durations 25 --durations-min 0.05
The slow marker is opt-in. Mark a test slow only with duration evidence
(from --durations), not by guessing - see the fast-lane policy in
TESTING_STANDARD.md. --fast is for quick reviewer feedback and must not
replace the full suite before merge. A slow mark only excludes a test from the
fast lane; the test stays runnable directly, e.g.:
./venv/bin/python -m pytest tests/test_auth_config_lock_concurrency.py
./venv/bin/python -m pytest -m slow
Order-sensitivity reporting (report-only)
tests/run_order_report.py runs pytest with the collected test items shuffled
by a seeded RNG, to surface order-sensitive tests (hidden coupling through
shared import state, module caches, databases, etc.). It is report-only: it is
not wired into CI, adds no gate, and changes no normal pytest collection or
ordering - the shuffle exists only inside this runner. The seed is always
printed, and pytest targets/options go after a literal --:
./venv/bin/python tests/run_order_report.py --seed 123 -- tests/cli/ -q
./venv/bin/python tests/run_order_report.py -- tests/cli/ -q # generates and prints a seed
The same seed reproduces the same order when the reported working directory, pytest target arguments, and test environment are also the same. The runner prints all command arguments with shell-safe POSIX quoting and uses the invoking Python interpreter.
A generated-seed run starts with output like:
[order-report] working directory: /path/to/odysseus
[order-report] shuffling test order with seed 284734921
[order-report] reproduce from this working directory with the same test environment:
[order-report] reproduce with: /path/to/odysseus/venv/bin/python /path/to/odysseus/tests/run_order_report.py --seed 284734921 -- tests/cli/ -q
Run the printed command from the reported working directory to reproduce the same fixed-seed order:
[order-report] working directory: /path/to/odysseus
[order-report] shuffling test order with seed 284734921
[order-report] reproduce from this working directory with the same test environment:
[order-report] reproduce with: /path/to/odysseus/venv/bin/python /path/to/odysseus/tests/run_order_report.py --seed 284734921 -- tests/cli/ -q
Pytest output remains visible between the report header and footer. A failing run ends with pytest's normal failure report followed by:
FAILED tests/example_test.py::test_example - AssertionError
[order-report] seed 284734921: pytest exit code 1 (report-only; fix order-sensitive failures in separate scoped PRs)
Failures discovered this way are real isolation bugs: fix them in separate
scoped PRs - do not silence them with skip/xfail, and do not "fix" them by
depending on a particular order.
The runner propagates pytest's exit code, so it composes with normal local workflows; "report-only" means it is not a CI gate, not that failures are swallowed.
Core principles
- Keep PRs small and homogeneous: one kind of change per PR.
- Prefer explicit local setup over hidden global fixtures.
- Avoid expanding the root
conftest.pyunless absolutely necessary. - Do not mix file moves with logic changes in the same PR.
- Do not weaken tests with
skip/xfailjust to make CI pass. - Validate the focused files you changed, plus any neighboring or order-sensitive groups they interact with.
Helper conventions
The helpers below live under tests/helpers/. They exist to remove repeated
boilerplate that already appeared across multiple tests. Reach for one only when
your test matches its intended use; do not stretch a helper to cover a new case.
tests.helpers.cli_loader.load_script
Use when a test needs to import a script under scripts/ without repeating
SourceFileLoader / importlib.util boilerplate.
- Intended for script/CLI tests that load a single file from
scripts/. - Not for arbitrary package imports - use a normal
importfor those. - When migrating an existing test to it, keep the existing stubs and assertions
unchanged. Any
sys.modulesstubs the script needs at import time must still be injected (e.g. viamonkeypatch) before callingload_script.
tests.helpers.import_state.clear_module
Use when a test must drop one cached module and its parent-package attribute before a fresh import.
- Clears
sys.modules[name]. - Clears the parent-package attribute when present.
- Good replacement for local
sys.modules.pop(...)+delattr(parent, child)blocks.
tests.helpers.import_state.preserve_import_state
Use when a test temporarily installs stubs into sys.modules and needs
deterministic cleanup afterward.
- Context manager: restores both
sys.modulesentries and parent-package attributes on exit (normal or exception). - Useful around module-level stubs or temporary imports.
- Prefer narrow, explicit module names over broad ones.
tests.helpers.import_state.clear_fake_database_modules
Use only for the guarded fake/stub database cleanup pattern.
- Preserves a real-looking
core.database(one with a string__file__). - Removes a fake/stub
core.databaseand the relatedsrc.databasestate. - Do not use as a general database reset fixture.
tests.helpers.import_state.clear_fake_endpoint_resolver_modules
Use only for the guarded fake/stub src.endpoint_resolver cleanup pattern.
- Preserves real resolver modules (those with a truthy
__file__). - Evicts fake/stub resolver modules and the dependent route modules that were cached against them.
- Accepts explicit extra dependent module names to evict alongside the defaults.
tests.helpers.sqlite_db.make_temp_sqlite
Use for the repeated file-backed temp sqlite setup in tests.
- Only constructs
(SessionLocal, engine, tmpfile)from the repeated block. - Does not patch modules and does not clean up the temp file.
- The caller must bind
SessionLocalexplicitly onto whatever module the code under test reads, and must keep the returned objects alive. - Do not use it as a general DB fixture framework.
tests.helpers.db_stubs.make_core_db_stub
Use for small import-time core.database stubs with a placeholder
SessionLocal.
- Pass model names via
modelswhen MagicMock attributes are sufficient. - Pass
attributeswhen an import needs exact placeholder values. - Set
install_core_package=Trueonly when the test also needs a fake parentcoremodule stub. - Keep custom fake sessions and route-specific database behavior local.
What not to abstract yet
Some remaining patterns should stay as-is for now rather than being forced into helpers:
- Large mixed files such as security/review regression files.
- Broad setup-oriented
sys.modulesstub installers. - One-off custom module patching.
- Custom DB session, route, and app setup.
Validation expectations
Run validation locally before opening or approving a PR. Practical checks:
git diff --check- catch whitespace and conflict-marker errors../venv/bin/python -m py_compile <changed files>- confirm changed files compile.- Focused
./venv/bin/python -m pyteston the changed test files. ./venv/bin/python -m pyteston neighboring or order-sensitive test groups that share import state with the changed files.grepfor the old boilerplate when replacing it, to confirm no stragglers remain.- A fresh audit worktree when changing the helpers themselves, so stale
__pycache__or import state cannot mask a regression.
Current roadmap
- Import-state cleanup - complete.
- Document helper conventions (this file).
- Pilot the repeated import-time
core.databasestub helper. - Add further tiny helpers only when the repeated semantics are clear.
- Start low-risk file moves only after helper conventions are documented.
- Avoid moving high-risk security/route regression files first.