Review feedback from @trek-e — three blocking issues and one style fix:
1. **Symlink escape guard** — Added validatePath() call on the resolved
global skill path with allowAbsolute: true. This routes the path
through the existing symlink-resolution and containment logic in
security.cjs, preventing a skill directory symlinked to an arbitrary
location from being injected. The name regex alone prevented
traversal in the literal name but not in the underlying directory.
2. **5 new tests** covering the global: code path:
- global:valid-skill resolves and appears in output
- global:invalid!name rejected by regex, skipped without crash
- global:missing-skill (directory absent) skipped gracefully
- Mix of global: and project-relative paths both resolve
- global: with empty name produces clear warning and skips
3. **Explicit empty-name guard** — Added before the regex check so
"global:" produces "empty skill name" instead of the confusing
'Invalid global skill name ""'.
4. **Style fix** — Hoisted require('os') and globalSkillsBase
calculation out of the loop, alongside the existing validatePath
import at the top of buildAgentSkillsBlock.
All 16 agent-skills tests pass.
* refactor(tests): standardize to node:assert/strict and t.after() per CONTRIBUTING.md
- Replace require('node:assert') with require('node:assert/strict') across
all 73 test files to enforce strict equality (no type coercion)
- Replace try/finally cleanup blocks with t.after() hooks in core.test.cjs
and hooks-opt-in.test.cjs per the test lifecycle standards
- Utility functions in codex-config and security-scan retain try/finally
as that is appropriate for per-function resource guards, not lifecycle hooks
Closes#1674
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* perf(tests): add --test-concurrency=4 to test runner for parallel file execution
Node.js --test-concurrency controls how many test files run as parallel child
processes. Set to 4 by default, configurable via TEST_CONCURRENCY env var.
Fixes tests at a known level rather than inheriting os.availableParallelism()
which varies across CI environments.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(security): allowlist verify.test.cjs in prompt-injection scanner
tests/verify.test.cjs uses <human>...</human> as GSD phase task-type
XML (meaning "a human should verify this step"), which matches the
scanner's fake-message-boundary pattern for LLM APIs. This is a
false positive — add it to the allowlist alongside the other test files
that legitimately contain injection-adjacent patterns.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Add agent_skills config section that maps agent types to skill directory
paths. At spawn time, workflows load configured skills and inject them
as <agent_skills> blocks in Task() prompts, giving subagents access to
project-specific skill files.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
