feat(agents): agentic infrastructure - skills, subagents, hooks, and cross-tool config#1360
feat(agents): agentic infrastructure - skills, subagents, hooks, and cross-tool config#1360seriouslysean wants to merge 10 commits into
Conversation
…ork} workflow skills
- .gemini/settings.json: loads AGENTS.md as context, enables agents and plan mode, wires git-guardrails hook via BeforeTool/run_shell_command - .codex/config.toml: loads AGENTS.md, inherits core shell environment - .gitignore: track both configs, exclude *.local overrides
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared, tool-agnostic “agentic infrastructure” under .agents/, plus per-tool configuration for Claude/Gemini/Codex, and a Node-based git-guardrails hook with Vitest coverage to prevent destructive git operations and enforce main-branch safety.
Changes:
- Add
.agents/structure (skills, docs, reviewer subagents) as a cross-tool source of truth. - Add
git-guardrailshook (Claude + Gemini wiring) and a dedicatedtest:agentsVitest suite for hook logic. - Commit assistant configuration files (
.claude/settings.json,.gemini/settings.json,.codex/config.toml) and adjust.gitignoreto track shared config while ignoring local overrides.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds test:agents script to run .agents tool tests. |
| .gitignore | Stops ignoring whole assistant dirs; ignores only local override files/dirs. |
| .gemini/settings.json | Enables agents + runs guardrails before run_shell_command. |
| .codex/config.toml | Adds Codex project-doc fallback and feature toggles. |
| .claude/settings.json | Adds Claude allowlist permissions and PreToolUse hook configuration. |
| .claude/hooks/git-guardrails.sh | Adds a shim to invoke the Node guardrails script. |
| .agents/tools/vitest.config.mjs | Adds Vitest config for .agents/tools tests. |
| .agents/tools/git-guardrails.mjs | Implements command parsing + guardrail rules + hook output. |
| .agents/tools/tests/git-guardrails.spec.mjs | Adds Vitest coverage for guardrail rule behavior. |
| .agents/skills/ctl-vue-patterns/SKILL.md | Adds Vue/Pinia/Vuetify patterns skill. |
| .agents/skills/ctl-test-patterns/SKILL.md | Adds Cypress/Vitest testing patterns skill. |
| .agents/skills/ctl-sails-patterns/SKILL.md | Adds Sails.js backend patterns skill. |
| .agents/skills/ctl-plan-work/SKILL.md | Adds planning workflow skill (discover → plan → gate). |
| .agents/skills/ctl-github-create-pr/SKILL.md | Adds gh-based PR creation workflow skill. |
| .agents/skills/ctl-git-commit/SKILL.md | Adds conventional-commit workflow skill with guardrails. |
| .agents/skills/ctl-game-domain/SKILL.md | Adds game-domain glossary skill for consistent terminology. |
| .agents/skills/ctl-discover/SKILL.md | Adds discovery/grep recipes skill. |
| .agents/skills/ctl-code-review/SKILL.md | Adds review workflow skill + subagent dispatch instructions. |
| .agents/README.md | Documents .agents/ purpose, layout, and loading model. |
| .agents/docs/vue-patterns.md | Adds detailed Vue patterns doc referenced by the skill. |
| .agents/docs/testing.md | Adds detailed testing doc referenced by the skill. |
| .agents/docs/skill-conventions.md | Defines skill naming/frontmatter conventions. |
| .agents/docs/sails-patterns.md | Adds detailed Sails patterns doc referenced by the skill. |
| .agents/docs/github.md | Documents PR labeling/version-bump workflow + gh usage. |
| .agents/docs/game-domain.md | Adds expanded game-domain glossary. |
| .agents/docs/doc-format.md | Defines repo doc style conventions. |
| .agents/docs/commit-format.md | Documents conventional-commit format + repo rules. |
| .agents/agents/ctl-security-reviewer.md | Adds security reviewer subagent definition. |
| .agents/agents/ctl-performance-reviewer.md | Adds performance reviewer subagent definition. |
| .agents/agents/ctl-docs-reviewer.md | Adds docs reviewer subagent definition. |
| .agents/agents/ctl-architecture-reviewer.md | Adds architecture reviewer subagent definition. |
Comments suppressed due to low confidence (2)
.agents/tools/git-guardrails.mjs:112
execSync('git branch --show-current')runs in the current process CWD. If the hook is invoked from outside the repo (or from a different repo), branch detection will throw and the guardrail won't enforce commit/push rules. Consider running git with an explicitcwd/-Cderived fromCLAUDE_PROJECT_DIR/GEMINI_PROJECT_DIR(or the resolved repo root).
);
const branch = needsBranch
? execSync('git branch --show-current', { encoding: 'utf8' }).trim()
: null;
.agents/docs/testing.md:38
cy.loadGameFixtureis documented here ascy.loadGameFixture(fixture), but the actual Cypress command is defined asloadGameFixture(pNum, fixture, gameId = null)(seetests/e2e/support/commands.js). Please update the signature and example to match real usage (including the requiredpNum).
### `cy.loadGameFixture(fixture)`
Loads a specific game state. All fields are optional.
```js
cy.loadGameFixture({
p0Hand: [Card.ACE_OF_CLUBS, Card.SEVEN_OF_CLUBS],
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
itsalaidbacklife
left a comment
There was a problem hiding this comment.
Very cool! Finally made it through a full review. Looking forward to discussing
There was a problem hiding this comment.
I love this. If it can keep docs in sync, that will be a real win
| card.name // "7♣️" | ||
|
|
||
| // In Cypress selectors | ||
| '[data-player-hand-card=7_0]' // rank_suit |
There was a problem hiding this comment.
An aside: I've been wanting to refactor these selectors to replace 7_0 with 7H (the id) and then refactor the test references to use functions to get cards by id.
It would be exciting to see if the docs review can ensure that this reference is updated accordingly
|
|
||
| ### PR template | ||
|
|
||
| Fill the template completely. Link the PR to its issue with `Closes #N` in the body. |
There was a problem hiding this comment.
| Fill the template completely. Link the PR to its issue with `Closes #N` in the body. | |
| Fill the template completely. Link the PR to its issue with `Resolves #N` in the body. |
(Just to match the template)
|
|
||
| ## Scope | ||
|
|
||
| - Socket listener cleanup (listeners added in setup must be removed in teardown) |
There was a problem hiding this comment.
Maybe mention events in general?
| - Socket listener cleanup (listeners added in setup must be removed in teardown) | |
| - Socket and event listener cleanup (listeners added in setup must be removed in teardown) |
|
|
||
| In Sails controllers, `populate` calls are expensive. Flag: | ||
| - Unnecessary `populate` on fields not used in the response | ||
| - Missing `populate` that causes N+1 (accessing `game.p0.username` without populating `p0`) |
There was a problem hiding this comment.
This is neat and the poor man's typscript lol
|
|
||
| ## Full reference | ||
|
|
||
| `.agents/docs/testing.md` |
There was a problem hiding this comment.
Generally the overlap between the skill and docs versions of these files makes me think it might be better to have the skill simply @import the doc(s) to centralize the source of truth and reduce drift. That way the point of the skill is essentially frontmatter for scoping and managing discoverabiliry of interrelated docs.
The downside might be a reduced capacity to provide summarized docs for reduced token usage, but it's not clear to me how the agent would know when to escalate, and if it does read the full docs, it's parsing much of the same content redundantly. Penny for your thoughts
| describe('git-guardrails', () => { | ||
| describe('checkDestructive', () => { | ||
| it('allows a clean commit command', () => { | ||
| expect(checkDestructive('git commit -m "fix: thing"')).toBeUndefined(); |
There was a problem hiding this comment.
So you he guardrails return undefined when the action is allowed and otherwise a string describing the reason it's disallowed? Is that built into agent specs or do we have logic that informs the agents how to interpret the results of the guardrails?
| @@ -0,0 +1 @@ | |||
| ../.agents/agents No newline at end of file | |||
There was a problem hiding this comment.
Does this need '@import`?
There was a problem hiding this comment.
Unfortunately, I don't think this will work as intended. My understanding is that Claude code is specifically hard coded to look for skills in .claude and that it ignores the contents of the folder which are not in the official skill format, meaning this file will be ignored and the skills won't be properly discoverable.
It looks like the options are to
- Forgo generic skills and simply put these in .claude (do you use another agent personally? For myself I'd be content with just getting Claude configured well, though as an open source tool, the flexibility to be generic makes sense. This is my current leaning for simplicity's sake, but especially if you're interested in using another agent yourself, then we can look for better solutions
- We define symlinks from the specific agent directories to the genetic .agents directory using a post install script in package.json. This might be the most robust but also finicky depending on OS nonsense ie windows will be a PITA.
- We define some automation to duplicate whole files based on .agents as a source of truth, overwriting the contents of .claude etc on commit or some other hook. This feels the clunkiest but may be the most reliable method for actual cross agent, is agnostic doc sharing.
I lean towards ditching the agnostic approach and sticking with .claude. What do you think @seriouslysean ?
Issue number
No linked issue — developer experience infrastructure.
Please check the following
npm run test:unit3/3,npm run test:agents29/29)npm run lintclean).agents/README.mdand.agents/docs/are the documentation)Please describe additional details for testing this change
This PR adds agentic infrastructure — it has no effect on game behavior. Verification steps:
readlink .claude/skills→../.agents/skills; same for.claude/agents,.gemini/skills,.gemini/agentsmain:git commit --allow-empty -m "test"while onmainshould be denied with a clear messagemainbranch should be allowedAGENTS.md/CLAUDE.md/GEMINI.mdunchanged:git diff main -- AGENTS.md CLAUDE.md GEMINI.mdis emptyWhat's here vs what's next: The reference skills (
ctl-sails-patterns,ctl-vue-patterns,ctl-test-patterns,ctl-game-domain) are currently opt-in. A follow-up will convert these to.claude/rules/path-specific rules so they load automatically when a matching file is touched, without relying on keyword routing.