Skip to content

fix: reject unsafe sync skill names - #88

Merged
andrei-hasna merged 2 commits into
mainfrom
fix/pr87-sync-name-containment
Aug 9, 2026
Merged

fix: reject unsafe sync skill names#88
andrei-hasna merged 2 commits into
mainfrom
fix/pr87-sync-name-containment

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • reject malformed explicitly named sync skills before any source or target path is constructed
  • add a regression proving traversal-shaped input performs no write outside the selected agent skills directory
  • preserve the valid named bundled fallback added by fix: align todos-plan with deployment doctrine #87

Why

The independent review of #87 reproduced a P1 unsafe mutation: ../../skills/todos-plan was restored as a raw name after normalization failed, then reused in filesystem joins. The landed code wrote outside .codewith/skills.

Verification

  • focused regression before fix: 16 pass, 1 fail
  • focused regression after fix: 17 pass, 0 fail
  • bun run typecheck: exit 0
  • bun run test: exit 0; 1076 pass, 1 skip, 0 fail; 6629 expect calls
  • git diff --cached --check: exit 0
  • shield review: exit 0; no security issues found in staged changes

Fixes the blocking finding recorded on #87.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #88 @ 74cce4c — lens: named-containment-remediation, reviewer /root/hotfix_reviewer (1 of 1)

Focused scope: the named path-containment P1 from #87 and direct regressions only.

Blocking P0/P1 findings:

  • P1 — invalid names are rejected after corpus resolution. syncSkillsToAgents() calls listPortableSkills() before normalizeRequested(). Corpus resolution may run the package's legacy-layout migration, so malformed input can still cause a filesystem write before the command rejects it.

Runtime control, exit 0:

  • THROW: Invalid skill name '../../skills/todos-plan'...
  • MIGRATED_BEFORE_REJECTION: true
  • TRAVERSAL_TARGET_WRITTEN: false

Required focused remedy:

  • Normalize and reject requested names before corpus lookup or migration. Add a regression with a legacy corpus entry proving malformed input creates no installed/ copy. Preserve the valid named bundled fallback.

Independent commands:

  • bun test src/lib/agent-sync.test.ts --timeout 30000 — exit 0; 17 pass, 0 fail, 60 expect calls.
  • bun test src/lib/todos-plan-contract.test.ts --timeout 30000 — exit 0; 3 pass, 0 fail.
  • bun run typecheck — exit 0.
  • git diff --check b1135a2901abdb133194dad6f8dd3deab2106cb9 74cce4c23f262860a2ff3027e603477e2a8de3d3 — exit 0.

Non-blocking follow-ups: none.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

Remediation cycle 1 pushed at 129b9821095a2cd88c47e6731b7e1c2ee0176791.

The invalid-name check now runs before corpus resolution, so legacy migration cannot write before rejection. The regression first reproduced the migration (16 pass, 1 fail) and now proves no installed/ copy or agent target is written (17 pass, 0 fail). Direct todos-plan contract: 3 pass, 0 fail; typecheck exit 0; full declared test gate: 1076 pass, 1 skip, 0 fail; staged security scan exit 0.

The same reviewer is re-checking only the named blocking defect and direct regressions.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #88 @ 129b982 — lens: named-containment-remediation, reviewer /root/hotfix_reviewer (1 of 1)

Focused re-review scope: the previously named migration-before-rejection blocker, its fix, and direct regressions only.

Independent runtime control, exit 0:

  • MIGRATED_BEFORE_REJECTION: false
  • TRAVERSAL_TARGET_WRITTEN: false
  • AGENT_TARGET_WRITTEN: false

The invalid traversal input still throws Invalid skill name, and the valid named bundled fallback remains functional.

Validation:

  • bun test src/lib/agent-sync.test.ts --timeout 30000 — exit 0; 17 pass, 0 fail, 61 expect() calls.
  • bun test src/lib/todos-plan-contract.test.ts --timeout 30000 — exit 0; 3 pass, 0 fail.
  • bun run typecheck — exit 0.
  • git diff --check b1135a2901abdb133194dad6f8dd3deab2106cb9 129b9821095a2cd88c47e6731b7e1c2ee0176791 — exit 0.

Blocking P0/P1 findings: none.

Non-blocking follow-ups: none.

@andrei-hasna
andrei-hasna merged commit 86c7058 into main Aug 9, 2026
2 checks passed
@andrei-hasna
andrei-hasna deleted the fix/pr87-sync-name-containment branch August 9, 2026 00:27
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.

1 participant