Skip to content

fix(skills): reject unexpected resource files - #243

Closed
SebastienElet wants to merge 8 commits into
mainfrom
fix/237-skill-directory-files
Closed

fix(skills): reject unexpected resource files#243
SebastienElet wants to merge 8 commits into
mainfrom
fix/237-skill-directory-files

Conversation

@SebastienElet

@SebastienElet SebastienElet commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Description

  • Define the canonical physical-entry allowlist for skill roots, agents metadata, and eval files.
  • Make skill-manager doctor inspect every file and directory on a quiescent canonical skill tree and fail closed when the tree or Git ignore rules cannot be audited.
  • Reject every Git-ignored entry, including below open resource directories, plus unexpected empty directories, symbolic links, FIFOs, sockets, and other non-regular entries.
  • Require tool-owned runtime data to live outside both skill collections; the doctor checker has no package dependency and runs with Bun auto-installation disabled.
  • Cover ignored paths under assets/, malformed Git evidence, canonical-root symlinks, files impersonating resource directories, unsupported entry types, unreadable directories, accepted eval layouts, and strict native policy parsing.
  • Split three independent Oxlint scenarios after a GitHub runner exposed their shared 15-second test timeout; no retry, skip, or timeout increase was added.

Useful Links

How to Test

  • bun --config=/dev/null --no-env-file tooling/lint-typescript.ts
  • bun --config=/dev/null --no-env-file run typecheck
  • SCRAPLING_DOCKER_SMOKE=1 bun --config=/dev/null --no-env-file test --timeout 15000
  • bun run format:typescript:check
  • prettier --check on the skill-manager files
  • bun --no-install harness/skills/skill-manager/scripts/check-resource-files.ts for all 24 canonical skills

Local evidence: macOS 26.6.2; 371 tests passed, 4 explicitly disabled Docker/network/real-CLI integration tests, 0 failed; lint, typecheck, TypeScript formatting, skill-manager formatting, and all 24 canonical physical audits passed.

Checklist

  • Changes have been tested locally
  • Documentation has been updated
  • No breaking changes

Enforce the canonical tracked-file policy in skill-manager doctor and cover the false-negative paths.

Closes #237
Include non-ignored untracked files in the doctor boundary while preserving ignored runtime artifacts.

Refs #237
Keep each Oxlint process under its own suite timeout instead of aggregating three independent scenarios.
Keep only gitignored runtime artifacts outside the doctor audit.

Refs #237
@SebastienElet

Copy link
Copy Markdown
Owner Author

Independent verdict — approved

Review of PR #243 on ddb66e271136ddd5eeda20b6ae7bcb82a5dacf82 against main@90934215, not stacked. GitHub reports a clean merge state and every CI job passed; no open task, conflict, or attachment was observed.

Authenticated local validation on this exact head: TypeScript lint 0 errors/0 warnings; typecheck 0 errors; 359/362 tests passed, 3 skipped, 0 failed, 1,635 assertions; TypeScript format checked 110 files; Prettier and CSpell passed. This macOS 26.6.2 evidence covers tracked and non-ignored-untracked paths, accepted eval variants, fail-closed Git behavior, and the Docker smoke; GitHub CI supplies Ubuntu/macOS jobs.

Approved on this head with no blocking or non-blocking finding.

@SebastienElet

Copy link
Copy Markdown
Owner Author

Repair record

Exact head reviewed and repaired: 104ac0cc558a5784a5101bc2f7e2a6f89a5288d0.

Repaired

  • Replaced the Git-enumerated audit with a recursive physical inventory, so ignored paths, empty directories, symlinks, FIFOs, sockets, and unreadable subtrees cannot disappear from the doctor result.
  • Made every Git-ignored physical entry fail, including entries below open assets/, references/, and scripts/ directories; malformed or unavailable Git evidence fails closed.
  • Rejected regular files impersonating allowed root directories and symlinks passed as canonical skill roots.
  • Removed the checker’s package dependency and disabled Bun auto-installation; the skill neither creates nor provisions runtime artifacts.
  • Updated conventions and index synchronization so runtime directories are never tolerated inside either canonical skill collection.
  • Replaced Zod policy parsing with strict dependency-free validation after the full-install oracle proved that provisioning would couple doctor to an incompatible Homebrew Bun version.

Proof

  • macOS 26.6.2, Bun 1.4.0: 371 passed, 4 explicitly disabled Docker/network/real-CLI integrations, 0 failed across 375 tests.
  • macOS 26.6.2: TypeScript lint, typecheck, TypeScript and skill-manager formatting, 22 deployment tests, and all 24 canonical physical/Git-ignore audits passed.
  • GitHub Actions: all 28 exact-head checks passed, including the 10-minute full installation and replay run.

Not repaired

  • Concurrent mutation during an audit remains outside the explicitly quiescent-tree contract. Tools are forbidden to write runtime data into skills, and any entry present before or after mutation is rejected, but the checker does not claim an atomic filesystem snapshot.

@SebastienElet

Copy link
Copy Markdown
Owner Author

Independent verdict — approved

Re-review of PR #243 on exact head 104ac0cc558a5784a5101bc2f7e2a6f89a5288d0 against main@90934215, not stacked. GitHub reports the branch mergeable and clean; all 28 exact-head checks passed, including macOS and Ubuntu deployment, TypeScript, text, shell, CodeGraph, and the 10-minute full installation replay.

Authenticated local validation on macOS 26.6.2 with Bun 1.4.0: TypeScript lint and typecheck passed; 371/375 tests passed, 4 explicitly disabled integrations skipped, 0 failed, 1,659 assertions; TypeScript formatting checked 115 files; Prettier and all 24 canonical skill audits passed. A separate clean-environment review confirmed the checker runs without package.json, a lockfile, or node_modules.

The prior review findings are closed: every ignored physical entry fails even under open resource directories, canonical-root symlinks and root-name files fail, Git evidence is parsed fail-closed, and doctor has no runtime package dependency or auto-install path. The repair trail is recorded in the exact-head repair comment.

Approved with no blocking finding. Non-blocking limitation: socket rejection was exercised manually rather than by a dedicated automated regression; the generic unsupported-entry path is covered automatically.

@SebastienElet
SebastienElet deleted the fix/237-skill-directory-files branch August 27, 2026 05:58
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.

Refuser les fichiers non admis dans les répertoires d’un skill

1 participant