Move ecosystem-neutral helpers out of Codex-named homes

[not-stbenjam]

Summary

Relocation-only refactor into the codex-plugin-rules feature branch, addressing the review note that ecosystem-neutral utilities lived in Codex-named modules. Behavior is byte-identical.

• safe_* filesystem wrappers → src/skillsaw/paths.py. safe_resolve, _safe_stat, safe_is_dir, safe_is_file, safe_exists, safe_is_symlink move out of formats/codex.py, and escapes_root moves out of rules/builtin/codex/_helpers.py, joining the pure path predicates already extracted there for exactly this reason. All import sites updated (context.py, lint_tree.py, docs/extractor.py, agentskills/marketplace/codex rules, tests); no re-export shims left behind.
• safe_display → new core module src/skillsaw/diagnostics.py with _redact_userinfo, _CONTROL_CHARS, _MAX_DISPLAY. The generic rules/builtin/hooks/json_valid.py no longer imports it cross-package from the codex rule helpers. rules/builtin/codex/_helpers.py now keeps only genuinely Codex-specific things (repo-type sets, KEBAB_CASE, strict-JSON helpers, path_problem).
• New helper contained_resolve(path, root) in paths.py names the reject-a-symlink-escape idiom (safe_resolve + is_relative_to, resolution failure → None).

contained_resolve conversions (exact-idiom sites only)

Converted (9): formats/codex.py codex_declared_paths + both checks in codex_manifest_is_contained; rules/builtin/agentskills/_helpers.py contained_skill_file; docs/extractor.py _scoped_prose_blocks; context.py _usable, _add (manifest dir), legacy plugin discovery, and both checks in single-skill discovery.

Deliberately left alone: boolean-return forms (lint_tree.py _contained/_contained_in_plugin, context.py _inside), sites that also resolve the root in the same guard (lint_tree.py _add_codex_block/_add_openai_metadata, openai_metadata.py), sites with different failure semantics (context.py existing-manifest check, marketplace_registration.py where an unresolvable manifest is not a reject), the explicit == root variant (context.py _contained), and the mixed-condition walk in _discover_skills_in_dir.

Ratchet test

tests/test_banned_apis.py AST-scans src/skillsaw/**/*.py for bare .resolve() calls and asserts the offending files are a subset of a frozen 29-file allowlist — existing debt is grandfathered per issue #463, new bare uses are banned, and a stale-entry check makes the ratchet tighten-only. Existing debt is intentionally not paid down here.

Verification

• make test: 3475 passed (full suite green, no test behavior changes beyond import-path updates)
• make lint clean; make update regenerated nothing
• Real-corpus canary over a local plugins checkout (--format json): 973 violations before, 973 after, violation sets identical, 0 violations from codex-* rules in both runs
• openshift-eng/ai-helpers run produces the identical result before and after (its single plugins-doc-up-to-date error is that repo's own docs drift, reproduced byte-for-byte with the base branch)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved repository and plugin discovery to reject paths that escape their expected directories.
  • Added safer handling for filesystem resolution failures, symlinks, and invalid paths.
  • Sanitized diagnostic messages by redacting credentials, removing control characters, and limiting displayed value length.

• Tests

  • Added safeguards to prevent unsafe path-resolution patterns from being reintroduced.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f1693566-5a86-4286-a12f-0788e20b79fa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR centralizes safe path resolution, containment, filesystem predicates, symlink checks, and diagnostic sanitization. Codex, plugin, marketplace, documentation, and agentskill discovery adopt the shared helpers, while tests enforce path-resolution usage and update helper imports.

Changes

Path safety and diagnostics

Layer / File(s)	Summary
Shared path-safety helpers src/skillsaw/paths.py, src/skillsaw/formats/codex.py, src/skillsaw/lint_tree.py	Adds safe resolution, containment, filesystem predicates, and symlink escape checks, then removes duplicated Codex implementations.
Diagnostic sanitization src/skillsaw/diagnostics.py, src/skillsaw/rules/builtin/codex/*, src/skillsaw/rules/builtin/hooks/json_valid.py	Adds bounded control-character and credential redaction handling and routes existing diagnostic formatting through safe_display.
Discovery containment integration src/skillsaw/context.py, src/skillsaw/docs/extractor.py, src/skillsaw/rules/builtin/agentskills/*, src/skillsaw/rules/builtin/marketplace/json_valid.py	Uses contained_resolve for repository, plugin, skill, marketplace, and scoped prose path validation.
Validation and API ratchets tests/test_banned_apis.py, tests/test_codex_rules.py	Adds AST checks for new bare Path.resolve() calls and updates tests to use centralized helpers.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Possibly related issues

• Retire the symlink-crash bug class: sweep bare Path.resolve() behind safe_resolve() and enforce with a banned-API test #463 — Covers the planned promotion of safe_resolve, containment handling, and banned-API enforcement implemented here.

Possibly related PRs

• stbenjam/skillsaw#347 — Overlaps the agentskill path-safety helper and import changes.
• stbenjam/skillsaw#451 — Overlaps Codex discovery containment and diagnostic display changes.

Suggested reviewers: stbenjam

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main refactor of moving shared helpers out of Codex-specific modules.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/helpers-home

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[not-stbenjam]

@codex review

[not-stbenjam]

@coderabbitai review

[not-stbenjam]

/review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[qodo-code-review]

PR Summary by Qodo

Move neutral path/diagnostic helpers into core modules

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Relocate filesystem-safe path helpers into skillsaw.paths and update imports.
• Extract safe_display into skillsaw.diagnostics for cross-ecosystem rule reuse.
• Add a ratchet test banning new bare Path.resolve() calls in src/skillsaw.

Diagram

graph TD
  CTX["context.py"] --> PATHS["paths.py"]
  FMT["formats/codex.py"] --> PATHS
  RULES_CODEX["codex rules"] --> PATHS & DIAG["diagnostics.py"]
  RULES_MISC["hooks/marketplace rules"] --> PATHS & DIAG
  TESTS["tests"] --> PATHS & DIAG

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep backward-compatible re-export shims

• ➕ Reduces churn for downstream/internal imports during transition
• ➕ Lowers risk of missed import updates
• ➖ Prolongs the anti-pattern of Codex-named modules being dependency hubs
• ➖ Creates long-lived ambiguity about the intended public import paths

2. Introduce a dedicated `skillsaw.fs`/`skillsaw.security` package

• ➕ Groups filesystem safety and redaction under a single, clearly-scoped namespace
• ➕ Easier to expand with future policy (e.g., other banned APIs) without bloating paths.py
• ➖ Adds another top-level module decision and migration overhead
• ➖ The current split (paths vs diagnostics) is already a clean separation

Recommendation: The PR’s approach is the right tradeoff: consolidate ecosystem-neutral helpers into core modules (skillsaw.paths, skillsaw.diagnostics) and update all call sites directly (no re-export shims). The added .resolve() ratchet test is a pragmatic guardrail to prevent regression while allowing incremental debt paydown.

Files changed (18) +353 / -235

Enhancement (2) +209 / -9

diagnostics.pyAdd core 'safe_display()' redaction/sanitization module +104/-0

Add core 'safe_display()' redaction/sanitization module

• Introduces 'safe_display()' and its helpers to safely echo untrusted manifest/config values in diagnostics. Implements bounded work/output, control-character replacement, and credential-shaped userinfo redaction.

src/skillsaw/diagnostics.py

paths.pyCentralize safe filesystem wrappers and add 'contained_resolve()' +105/-9

Centralize safe filesystem wrappers and add 'contained_resolve()'

• Expands the module from pure string predicates into the central home for non-throwing filesystem probes ('safe_resolve', 'safe_is_*') and adds 'contained_resolve(path, root)' for common containment checks. Moves 'escapes_root()' here to keep containment logic ecosystem-neutral.

src/skillsaw/paths.py

Refactor (14) +36 / -221

context.pyUse 'contained_resolve()' for containment checks during discovery +8/-14

Use 'contained_resolve()' for containment checks during discovery

• Updates imports to pull safe filesystem wrappers from 'skillsaw.paths'. Replaces repeated 'safe_resolve()+is_relative_to()' containment idioms with 'contained_resolve()' in plugin/skill discovery logic.

src/skillsaw/context.py

extractor.pySwitch prose scoping containment to 'contained_resolve()' +3/-3

Switch prose scoping containment to 'contained_resolve()'

• Moves imports of safe path helpers to 'skillsaw.paths'. Simplifies containment logic in prose block scoping by using 'contained_resolve()'.

src/skillsaw/docs/extractor.py

codex.pyRemove local safe_* wrappers; consume them from 'skillsaw.paths' +5/-64

Remove local safe_* wrappers; consume them from 'skillsaw.paths'

• Deletes the in-module implementations of 'safe_resolve' and safe stat wrappers. Updates Codex manifest path validation to use centralized helpers and converts exact containment idioms to 'contained_resolve()'.

src/skillsaw/formats/codex.py

lint_tree.pyRepoint safe path imports to 'skillsaw.paths' +1/-3

Repoint safe path imports to 'skillsaw.paths'

• Stops importing 'safe_is_dir/safe_is_file/safe_resolve' from 'formats.codex' and uses 'skillsaw.paths' instead. No behavioral changes intended.

src/skillsaw/lint_tree.py

_helpers.pyUse 'contained_resolve()' for skill-file containment validation +2/-3

Use 'contained_resolve()' for skill-file containment validation

• Replaces imports from 'formats.codex' with 'skillsaw.paths'. Simplifies containment checks by using 'contained_resolve()' and retains existing return semantics.

src/skillsaw/rules/builtin/agentskills/_helpers.py

unreferenced_files.pySwitch 'safe_resolve' import to 'skillsaw.paths' +1/-1

Switch 'safe_resolve' import to 'skillsaw.paths'

• Stops importing 'safe_resolve' from 'formats.codex' and imports it from 'skillsaw.paths' for consistency and neutrality.

src/skillsaw/rules/builtin/agentskills/unreferenced_files.py

_helpers.pyKeep Codex-specific helpers; move 'safe_display'/'escapes_root' out +2/-118

Keep Codex-specific helpers; move 'safe_display'/'escapes_root' out

• Removes 'safe_display' and its internal redaction machinery, replacing usage with imports from 'skillsaw.diagnostics'. Moves 'escapes_root' import to 'skillsaw.paths' so this module contains only Codex-specific constants and JSON/path validation helpers.

src/skillsaw/rules/builtin/codex/_helpers.py

marketplace_json_valid.pyImport 'safe_display'/'safe_exists' from core modules +2/-2

Import 'safe_display'/'safe_exists' from core modules

• Stops depending on Codex-private helpers for diagnostics display and on 'formats.codex' for existence checks. Uses 'skillsaw.diagnostics.safe_display' and 'skillsaw.paths.safe_exists'.

src/skillsaw/rules/builtin/codex/marketplace_json_valid.py

marketplace_registration.pyRewire registration rule to core 'paths' and 'diagnostics' helpers +3/-4

Rewire registration rule to core 'paths' and 'diagnostics' helpers

• Moves 'safe_display' import to 'skillsaw.diagnostics' and pulls 'safe_is_dir/safe_is_file/safe_resolve' from 'skillsaw.paths'. Removes reliance on 'formats.codex' as a cross-package utility provider.

src/skillsaw/rules/builtin/codex/marketplace_registration.py

openai_metadata.pyImport 'safe_display' from 'skillsaw.diagnostics' +2/-3

Import 'safe_display' from 'skillsaw.diagnostics'

• Eliminates Codex-private helper import for diagnostic display. Consolidates safe filesystem and path predicates imports under 'skillsaw.paths' and 'skillsaw.diagnostics'.

src/skillsaw/rules/builtin/codex/openai_metadata.py

plugin_json_valid.pyUse core 'safe_display' and safe filesystem wrappers +2/-2

Use core 'safe_display' and safe filesystem wrappers

• Moves 'safe_display' to 'skillsaw.diagnostics' and shifts 'safe_exists/safe_is_dir/safe_is_file' to 'skillsaw.paths'. Keeps Codex-specific validation logic unchanged.

src/skillsaw/rules/builtin/codex/plugin_json_valid.py

plugin_structure.pyUse 'skillsaw.diagnostics.safe_display' for Codex structure messages +3/-1

Use 'skillsaw.diagnostics.safe_display' for Codex structure messages

• Switches 'safe_display' import to the new diagnostics module while keeping Codex repo-type wiring unchanged.

src/skillsaw/rules/builtin/codex/plugin_structure.py

json_valid.pyStop importing Codex helpers; use 'skillsaw.diagnostics.safe_display' +1/-1

Stop importing Codex helpers; use 'skillsaw.diagnostics.safe_display'

• Removes an inappropriate dependency from generic hooks validation into Codex helper internals. Imports 'safe_display' from the new ecosystem-neutral diagnostics module.

src/skillsaw/rules/builtin/hooks/json_valid.py

json_valid.pyCentralize safe filesystem imports in 'skillsaw.paths' +1/-2

Centralize safe filesystem imports in 'skillsaw.paths'

• Moves 'safe_is_dir' and 'safe_resolve' imports out of 'formats.codex' and into 'skillsaw.paths'. Keeps marketplace validation behavior the same while reducing cross-ecosystem coupling.

src/skillsaw/rules/builtin/marketplace/json_valid.py

Tests (2) +108 / -5

test_banned_apis.pyAdd ratchet test banning new bare '.resolve()' in skillsaw source +101/-0

Add ratchet test banning new bare '.resolve()' in skillsaw source

• Introduces an AST-based scan over 'src/skillsaw/**/*.py' to detect attribute calls named 'resolve'. Enforces a frozen allowlist of existing offenders (tighten-only, with a stale-entry check) and exempts 'paths.py' where wrappers live.

tests/test_banned_apis.py

test_codex_rules.pyUpdate tests to import 'safe_display'/'escapes_root' from new homes +7/-5

Update tests to import 'safe_display'/'escapes_root' from new homes

• Repoints imports for 'escapes_root' to 'skillsaw.paths' and 'safe_display' (plus '_redact_userinfo') to 'skillsaw.diagnostics'. Keeps the redaction and containment behavior assertions intact.

tests/test_codex_rules.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 143 rules

✅ Skills: 5 invoked

  skillsaw-pr-review

  skillsaw-issue-solver

  skillsaw-pr-followup

  skillsaw-create-plugin

  skillsaw-review-panel

1. Removed codex.safe_exists API ✗ Dismissed 📜 Skill insight ≡ Correctness

Description

safe_exists() and safe_is_symlink() were removed from skillsaw.formats.codex without a
compatibility shim, which can break downstream code importing these helpers from the old location.
This is a backward-incompatible change without any deprecation path or migration guidance in-code.

Code

src/skillsaw/formats/codex.py[L106-113]

-def safe_exists(path: Path) -> bool:
-    """``path.exists()``, or ``False`` when the path cannot be stat'd."""
-    return _safe_stat(path, "exists")
-
-
-def safe_is_symlink(path: Path) -> bool:
-    """``path.is_symlink()``, or ``False`` when the path cannot be stat'd."""
-    return _safe_stat(path, "is_symlink")

Relevance

⭐⭐⭐ High

Team historically preserves legacy imports via shims/re-exports during refactors (PR #307, PR #8).

PR-#307
PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2315578 forbids backward-incompatible API removals without a deprecation path. The
PR deletes safe_exists()/safe_is_symlink() from skillsaw.formats.codex (diff), and the module
now imports only a subset of the moved helpers from skillsaw.paths, leaving those names
unavailable from the old import location.

src/skillsaw/formats/codex.py[14-18]
src/skillsaw/paths.py[109-116]
Skill: skillsaw-review-panel

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 PR relocates `safe_*` filesystem helpers out of `skillsaw.formats.codex`, and removes `safe_exists()` / `safe_is_symlink()` from that module. Any external callers importing `safe_exists` / `safe_is_symlink` from `skillsaw.formats.codex` will now fail at import-time.

## Issue Context
The compliance requirement is to avoid backward-incompatible changes (or provide a deprecation/migration path).

## Fix Focus Areas
- src/skillsaw/formats/codex.py[14-18]
- src/skillsaw/paths.py[109-116]

Suggested implementation direction:
- In `src/skillsaw/formats/codex.py`, re-export the moved helpers (at least `safe_exists` and `safe_is_symlink`) from `skillsaw.paths`.
- Optionally emit a `DeprecationWarning` (or add module-level doc comment) indicating the new canonical import path (`skillsaw.paths`).

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

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: faf896248c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve legacy imports for the moved path helpers

When an existing custom rule imports either previously exposed non-underscore helper (from skillsaw.formats.codex import safe_exists or safe_is_symlink), this import list no longer binds it, so custom-rule loading raises an ImportError and aborts the lint. The repository explicitly preserves legacy import locations for custom rules in tests/test_module_layering.py:9-14; keep compatibility re-exports here while making paths.py canonical.

AGENTS.md reference: AGENTS.md:L88-L89

Useful? React with 👍 / 👎.

[not-stbenjam]

Declining: skillsaw.formats.codex does not exist on main or in any release — it was introduced on this unreleased feature branch, so no external code can import from it. Moving the helpers to their intended home before first release is exactly why no deprecation shim is warranted.

[coderabbitai]

🧹 Nitpick comments (1)

src/skillsaw/paths.py (1)

66-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

State the resolved-root precondition in contained_resolve’s docstring.

contained_resolve resolves path but leaves root unchanged; callers pre-resolve here, but if the second argument is an unresolved symlinked directory, is_relative_to(root) can incorrectly reject contained paths. Make the docstring explicit that root must already be resolved.

🤖 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 `@src/skillsaw/paths.py` around lines 66 - 77, Update the contained_resolve
docstring to explicitly state that root must already be resolved before calling
the function, while preserving the existing behavior and examples.

🤖 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.

Nitpick comments:
In `@src/skillsaw/paths.py`:
- Around line 66-77: Update the contained_resolve docstring to explicitly state
that root must already be resolved before calling the function, while preserving
the existing behavior and examples.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 238a219f-f64c-473f-8983-51ff7e9fd186

📥 Commits

Reviewing files that changed from the base of the PR and between b6246c7 and faf8962.

📒 Files selected for processing (18)

• src/skillsaw/context.py
• src/skillsaw/diagnostics.py
• src/skillsaw/docs/extractor.py
• src/skillsaw/formats/codex.py
• src/skillsaw/lint_tree.py
• src/skillsaw/paths.py
• src/skillsaw/rules/builtin/agentskills/_helpers.py
• src/skillsaw/rules/builtin/agentskills/unreferenced_files.py
• src/skillsaw/rules/builtin/codex/_helpers.py
• src/skillsaw/rules/builtin/codex/marketplace_json_valid.py
• src/skillsaw/rules/builtin/codex/marketplace_registration.py
• src/skillsaw/rules/builtin/codex/openai_metadata.py
• src/skillsaw/rules/builtin/codex/plugin_json_valid.py
• src/skillsaw/rules/builtin/codex/plugin_structure.py
• src/skillsaw/rules/builtin/hooks/json_valid.py
• src/skillsaw/rules/builtin/marketplace/json_valid.py
• tests/test_banned_apis.py
• tests/test_codex_rules.py