Repository navigation
feat(tools): add project-apply + submodule-bump - #29
Merged
Merged
Conversation
Two small, independent ops tools. Both side-effectful (unlike repo-init
and project-spawn which are pure). Orchestrator-only.
## project-apply
Apply a files-map (from project-spawn or similar generators) to disk.
### Input
```ts
const ProjectApplyInput = z.object({
files: z.record(z.string(), z.string())
.describe('Map of relative file path to content.'),
baseDir: z.string().optional()
.describe('Directory to apply relative paths into. Defaults to cwd.'),
dryRun: z.boolean().optional()
.describe('Report what would happen, touch nothing.'),
force: z.boolean().optional()
.describe('Overwrite existing files. Default false.'),
});
```
### Output
```ts
const ProjectApplyOutput = z.object({
baseDir: z.string()
.describe('Resolved absolute base directory.'),
applied: z.array(z.string())
.describe('Relative paths actually written to disk.'),
skipped: z.array(z.object({
path: z.string(),
reason: z.enum(['exists', 'dry-run']),
})).describe('Paths not written with the reason.'),
});
```
### Handler
1. Run `safeParse` at the boundary (direct-invocation defense-in-depth;
project-spawn precedent).
2. Resolve baseDir. If undefined, use `process.cwd()`. Resolve to
absolute.
3. For each entry in files:
- Reject absolute paths and paths containing `..` segments. Reject
empty keys. Return `err('invalid-path', 'path: <why>', false)`
on first offender.
- If dry-run → push to skipped with reason `dry-run`.
- Else compute absolute target = path.resolve(baseDir, relPath).
Assert it starts with baseDir + path.sep (defense against
symlink tricks). Reject with `err('path-escape', …, false)` if
it escapes.
- If file exists and not force → push to skipped with reason
`exists`.
- Else: `mkdir -p` parent, `fs.writeFile` content (utf8), push
rel path to applied.
4. Return `ok({ baseDir, applied, skipped })`.
### Tests (tests/tools/project-apply.test.ts)
Use the existing fixture-repo harness for tmpdir scoping (or a minimal
local tmpdir helper — whichever is cleaner; don't extend the harness
unless it's cleaner).
Cases:
- P1: roles === ['orchestrator']
- P2: minimal input {files: {'a.txt': 'hello'}} in a tmpdir baseDir —
applied === ['a.txt'], file content matches, skipped empty
- P3: nested path {files: {'src/deep/file.ts': 'x'}} — mkdir -p
happened; file exists; applied lists the path
- P4: dry-run — nothing written, skipped lists every path with reason
'dry-run'
- P5: overwrite default refused — pre-create a.txt, apply without
force → skipped ['a.txt' reason:'exists'], original content
preserved
- P6: force overwrite — same but with force:true → applied, content
replaced
- P7: multiple files, mixed state — some new, some existing; verify
correct partitioning into applied/skipped
- N1: absolute path '/etc/passwd' → err('invalid-path', …)
- N2: '..' segment 'a/../b' → err('invalid-path', …)
- N3: empty key '' → err('invalid-path', …)
- N4: path escape via symlink — if worth testing; lower priority.
Skip if too fiddly.
## submodule-bump
Advance a git submodule's recorded pointer in the parent repo to a
given commit. Does NOT commit the parent — caller commits. This
composes: one tool does the mechanical bump, caller writes the
message.
### Input
```ts
const SubmoduleBumpInput = z.object({
submodulePath: z.string()
.describe('Path of the submodule inside the parent repo, e.g. repos/bitswell/loom-tools.'),
targetSha: z.string()
.describe('Full or short commit SHA to bump the submodule to.'),
remoteUrl: z.string().optional()
.describe('Override the upstream URL to fetch from. Defaults to the URL in .gitmodules.'),
parentDir: z.string().optional()
.describe('Parent repo root. Defaults to cwd.'),
});
```
### Output
```ts
const SubmoduleBumpOutput = z.object({
submodulePath: z.string(),
previousSha: z.string()
.describe('SHA previously recorded in the parent (pre-bump).'),
targetSha: z.string()
.describe('Full SHA now recorded in the parent (post-bump, post-resolution).'),
staged: z.boolean()
.describe('Whether the gitlink update is staged in the parent.'),
});
```
### Handler
1. safeParse.
2. Resolve parentDir (default cwd), verify it's a git repo
(`git -C <dir> rev-parse --git-dir`).
3. Verify submodulePath is tracked as a submodule:
`git -C <parent> config --file .gitmodules submodule.<path>.url`.
If missing, `err('not-a-submodule', …, false)`.
4. Resolve remoteUrl: use input if provided, else `.gitmodules` URL.
5. Read previous SHA from parent index:
`git -C <parent> ls-tree HEAD <path>` → parse the gitlink commit.
6. In the submodule:
- `git -C <submodule> fetch <remoteUrl> <targetSha>` — narrow fetch
to the specific sha (avoids pulling whole history if short).
- `git -C <submodule> checkout <targetSha>` (detached HEAD).
- Read resolved full SHA via `git -C <submodule> rev-parse HEAD`.
7. Stage the parent gitlink update:
`git -C <parent> add <path>`.
8. Return `ok({ submodulePath, previousSha, targetSha: resolvedSha,
staged: true })`.
Does NOT commit. The caller writes the message and trailers.
### Tests (tests/tools/submodule-bump.test.ts)
Build two fixture repos (inner = submodule, outer = parent) using
the fixture-repo harness or a small extension. Steps:
- createFixtureRepo() twice
- In inner, make two commits A, B
- In outer, `git submodule add <inner-path>` at commit A, commit it
- Apply submodule-bump({submodulePath: <path>, targetSha: B, parentDir: outer})
- Assert: previousSha === A; targetSha === B full sha; staged === true
- `git -C outer diff --cached <path>` shows the gitlink change
Negative cases:
- N1: submodulePath not tracked → err('not-a-submodule', …)
- N2: targetSha doesn't exist in remote → err (propagate fetch failure
with code 'fetch-failed' or 'invalid-sha')
- N3: parentDir not a git repo → err('not-a-repo', …)
Also test: when remoteUrl is passed explicitly, it overrides
.gitmodules (set up a fixture where the .gitmodules URL is wrong
and the override is right).
## Registry wiring (src/tools/index.ts)
Both tools registered in Phase 4 next to repoInitTool and
projectSpawnTool:
- `import { projectApplyTool } from './project-apply.js';`
- `import { submoduleBumpTool } from './submodule-bump.js';`
- Exports in Phase 4 export block
- `registry.register(projectApplyTool);`
- `registry.register(submoduleBumpTool);`
## mcp-server test fixture
Bump tool count (currently 30 on main) by +2 → 32. Add both names to
the expected names list. (Same pattern as project-spawn.)
## Reference files (read, do not modify)
- src/tools/project-spawn.ts — safeParse-at-boundary pattern
- src/tools/repo-init.ts — files-map shape (for project-apply's input)
- src/tools/submodule.ts — existing submodule tool (inspect for any
shared logic; likely nothing reusable — but don't duplicate trivially)
- src/tools/commit.ts — git exec patterns, error mapping
- tests/harness/fixture-repo.ts — tmpdir + git init helpers
- tests/tools/submodule.test.ts — for patterns, not duplication
## Constraints
- No PLAN.md or AGENT.json files. Plans (if any) in commit bodies
with Task-Status: PLANNING. Skip planning-phase entirely — task spec
is complete; start with IMPLEMENTING directly.
- Zod convention: .optional() without .default(); defaults in handler.
- `SafeString = z.string().regex(/^[^\n\r]*$/)` guard on any free-form
string that ends up in a file or git trailer. For this pair:
- project-apply: baseDir → not user-content; paths already validated
by path rules; file content is binary-tolerant so no regex on it.
- submodule-bump: submodulePath, targetSha, remoteUrl — reject
newlines on all three. targetSha has a tighter regex (hex only).
## Acceptance criteria
1. src/tools/project-apply.ts and src/tools/submodule-bump.ts exist
2. Both registered in src/tools/index.ts
3. tests/tools/project-apply.test.ts and
tests/tools/submodule-bump.test.ts pass
4. tests/runtime/mcp-server.test.ts updated for new tool count + names
5. `npm test` exits 0 (all existing tests still pass)
6. `npm run build` exits 0; dist/tools/project-apply.js and
dist/tools/submodule-bump.js emitted
Agent-Id: bitswell
Session-Id: 24c9a87b-5fd1-4fd4-83cf-b8e6f15bb73c
Task-Status: ASSIGNED
Assigned-To: moss
Assignment: project-apply-and-submodule-bump
Scope: src/tools/project-apply.ts src/tools/submodule-bump.ts tests/tools/project-apply.test.ts tests/tools/submodule-bump.test.ts src/tools/index.ts tests/runtime/mcp-server.test.ts
Dependencies: none
Budget: 150000
Starting implementation of the paired utility tools per ASSIGNED commit 55dde23. Skipping planning phase as the task spec is concrete. Agent-Id: moss Session-Id: 24c9a87b-5fd1-4fd4-83cf-b8e6f15bb73c Task-Status: IMPLEMENTING Heartbeat: 2026-04-19T00:00:00Z
Two orchestrator-only utility tools for post-generation ops: project-apply takes a files-map (relative path -> content), resolves it under a baseDir (defaults to cwd), and writes to disk. Honors dryRun (reports everything as skipped), force (overwrites existing), and defends against absolute paths, ".." segments, empty keys, and post-resolve baseDir escape. submodule-bump advances a recorded submodule pointer in the parent repo to a target SHA, without committing the parent. Verifies parentDir is a git repo, the submodulePath is in .gitmodules, reads the previous SHA from the parent index, narrow-fetches the target, checks out detached HEAD, reads the resolved full SHA, and stages the gitlink. remoteUrl may override the .gitmodules URL. Both apply safeParse at the handler boundary (project-spawn precedent); submodule-bump additionally pins free-form inputs with SafeString and restricts targetSha to hex. Registered in createDefaultRegistry; mcp-server name-list fixture updated (30 -> 32). Files-Changed: 6 Key-Finding: fixture-repo harness needed only a wrapper, not extension — createFixtureRepo() twice + `submodule add` produces a working inner/outer pair. The `submodule add` invocation in git 2.38+ requires `-c protocol.file.allow=always` because file:// transport is disabled by default. Agent-Id: moss Session-Id: 24c9a87b-5fd1-4fd4-83cf-b8e6f15bb73c Task-Status: COMPLETED Heartbeat: 2026-04-19T00:10:00Z
Suite passes (497/497), both tools wired and exported, but three
load-bearing defects surface under direct probing: a symlink-based
path-escape, uncaught FS errors from the handler, and a staged-flag
that lies when previous == target.
## Findings
### project-apply — REQUEST_CHANGES
1. Symlink-based path escape (critical). src/tools/project-apply.ts:100-107
The target check `path.resolve(baseDir, relPath).startsWith(baseDir + path.sep)`
is a pure string comparison on the unresolved path. If `baseDir`
contains a symlink (e.g. created by an earlier apply, by another
tool, or left by a generator), writes through that link escape the
sandbox. Reproducer: pre-create `<baseDir>/link-to-outside ->
/tmp/outside`; call with files `{"link-to-outside/gotcha.txt": "pwned"}`.
Result: file written to `/tmp/outside/gotcha.txt`, applied=["link-to-outside/gotcha.txt"],
no error. Fix: `realpath` the target's dirname after mkdir and
re-check the prefix, or refuse to traverse into any path segment
that is a symlink. The spec's N4 test was marked skippable; the
threat model nevertheless exists.
2. Uncaught handler crash on filesystem errors. src/tools/project-apply.ts:109-116
`fs.writeFile` is never wrapped in try/catch. Three reproducers:
- `force:true` over an existing directory → EISDIR escapes as an
unhandled rejection; the tool does not return an `err(...)`.
- Filename containing `\u0000` → Node throws
`TypeError: path must be ... without null bytes` before the
handler returns.
- EACCES / ENOSPC / EROFS on a real filesystem would do the same.
MCP server gets an uncaught exception instead of a structured
result. Fix: wrap the write (and the existence check) in try/catch
and map to `err('write-failed', e.message, false)` or similar.
3. `path-escape` error code is dead code AND misadvertised. src/tools/project-apply.ts:101-107
With `validateRelPath` already rejecting absolute paths and any
`..` segment, `path.resolve(baseDir, relPath)` cannot escape
lexically, so the `startsWith` branch is never taken in practice
(the one corner — target === baseDir — is special-cased). The
spec called this check a "defense against symlink tricks" — it
provides zero symlink defense (see finding 1). Either remove it
as misleading or replace with an actual realpath-based check.
4. Leading `./` is silently preserved in applied/skipped. src/tools/project-apply.ts:46-48
`validateRelPath` inspects segments but does not normalize. Input
`"./sub/x.txt"` writes to `<baseDir>/sub/x.txt` but reports
`applied: ["./sub/x.txt"]`. Not exploitable; surprising for a
caller that deduplicates by path.
5. Test coverage gaps. tests/tools/project-apply.test.ts:
- No test for the force-over-directory crash (finding 2).
- No symlink-traversal test (spec N4 punted; finding 1 now
justifies un-punting).
- No test for NUL in filename / NUL in content (content case
works; filename case crashes).
- No test for `skipped` ordering when input keys are numeric-ish
('1','2','3') — JS Object.entries is well-defined but worth
pinning to prevent regressions.
### submodule-bump — REQUEST_CHANGES
6. Short SHAs are not supported, contradicting both the spec and the
tool's own description. src/tools/submodule-bump.ts:47, 122-129
`git fetch <url> <sha>` interprets the argument as a ref, not a
commit; any short SHA returns `fatal: couldn't find remote ref`.
Reproducer: bump with `targetSha: <full>.slice(0,7)` on a file://
fixture → `fetch-failed`. The description field (line 47) says
"Full or short commit SHA" — delete "or short", or resolve short
SHAs client-side first (e.g. via an extra rev-parse after a
broader fetch), or document the limitation explicitly.
7. `staged: true` is returned even when nothing is staged.
src/tools/submodule-bump.ts:159-164
When previousSha === resolvedSha (bump-to-same-SHA), `git add`
succeeds but stages nothing. The tool reports staged=true. A
caller that relies on this flag to decide whether to commit will
create an empty commit. Fix: either compute staged from
`git diff --cached --quiet -- <path>` (exit code), or
`staged: previousSha \!== resolvedSha`.
8. `targetSha` regex is looser than spec. src/tools/submodule-bump.ts:9, 15
Regex is `/^[0-9a-f]+$/i` — accepts 1 char up to ∞. Thorn's
mandate asked for hex 7–40. A 1-char hex ("a") passes validation
and only fails at `fetch-failed`. Not a security issue (git
rejects it) but the Zod layer is meant to be the tight gate.
Suggest `/^[0-9a-f]{4,40}$/i` at minimum (git's ambiguous-short
threshold) or `{7,40}`.
9. `parentDir` is plain `z.string()` — no newline guard.
src/tools/submodule-bump.ts:21-24
The spec text listed only `submodulePath, targetSha, remoteUrl`
as SafeString, so technically in-spec, but inconsistent: a
newline in `parentDir` propagates into the error message
verbatim and would propagate into any trailer a caller composed
from the response. Applying SafeString here costs nothing and
matches the pattern.
10. `N2` fetch-failed returns `retryable: true`. src/tools/submodule-bump.ts:128
When the SHA simply doesn't exist in the remote (the test
scenario: `deadbeef...`), a retry will never succeed. Network
failures justify retryable; non-existent SHAs do not. Consider
parsing stderr for "couldn't find remote ref" / "not our ref"
and returning `retryable: false` for those; leave retryable for
transport errors.
11. Detached-HEAD aftermath is undocumented. src/tools/submodule-bump.ts:131-138
The submodule is left at a detached HEAD. That's probably fine
for the bump use case (the parent records the gitlink; the
working tree in the submodule is scratch) but the output schema
does not communicate it, and a caller that subsequently operates
inside the submodule will be surprised. Add a brief note in the
tool description.
12. Test harness relies on file:// transport — narrow-SHA fetches
only work on this transport. tests/tools/submodule-bump.test.ts:44-66
The `protocol.file.allow=always` hack plus file:// transport
bypasses the `uploadpack.allowReachableSHA1InWant` gate that
real HTTP/SSH remotes enforce. The tests therefore do not
exercise the common production failure mode: a correctly-typed
full SHA that the server refuses to serve. Not a blocker for
this PR, but worth flagging — consider adding a test that
disables `uploadpack.allow*` and asserts the `fetch-failed`
mapping still surfaces cleanly. (I ran this manually; with
file:// it still works because file transport ignores the
guard. Against a bare HTTP remote you'd see the real failure.)
### Cross-cutting — fine
- safeParse-at-boundary: both tools follow project-spawn precedent.
- SafeString on `submodulePath`: present.
- Role scoping: both `['orchestrator']`. Confirmed via
`projectApplyTool.definition.roles`.
- Registry wiring + mcp-server test fixture bumped to 32: correct.
- `dist/tools/project-apply.js` and `dist/tools/submodule-bump.js`
emitted.
- 497/497 tests pass.
## Required before approval
Fix 1 (symlink escape) and fix 2 (uncaught FS errors) — both are
genuine production hazards. Fix 6 (short SHA description) is one-
line docs or one-line code. Fix 7 (staged flag honesty) is a 2-line
change with a test. The rest are nice-to-have hardening.
Verdict: REQUEST_CHANGES
Agent-Id: thorn
Session-Id: 24c9a87b-5fd1-4fd4-83cf-b8e6f15bb73c-thorn
Review-Verdict: REQUEST_CHANGES
Heartbeat: 2026-04-19T20:38:26Z
Addressing thorn's REQUEST_CHANGES at 59f7b0d. Six required fixes plus two bundled nice-to-haves: project-apply - F1: realpath-check target's parent after mkdir to close symlink- based escape - F2: wrap existence check + write in try/catch, map to err( 'write-failed') - F3: retire the pure-lexical startsWith check (dead once F1 lands) - F4: normalize relative paths with path.posix.normalize before reporting; reject '.' submodule-bump - F5: tighten description; targetSha is full SHA only - F6: compute staged honestly from previousSha vs resolvedSha - F7: tighten regex to /^[0-9a-f]{7,40}$/i - F8: SafeString on parentDir for consistency Agent-Id: moss Session-Id: 24c9a87b-5fd1-4fd4-83cf-b8e6f15bb73c Task-Status: IMPLEMENTING Heartbeat: 2026-04-19T00:20:00Z
…honesty Addresses thorn's REQUEST_CHANGES at 59f7b0d. project-apply - F1: after `mkdir -p`, realpath the parent and re-check the prefix against a realpathed baseDir. A symlink placed inside baseDir (by an earlier apply or an external tool) can no longer redirect writes outside. Intra-baseDir symlinks still resolve normally. - F2: wrap the existence check and the write in try/catch; map any FS error (EISDIR from force-over-directory, EACCES, ENOSPC, NUL-in-filename thrown by Node) to `err('write-failed', …, false)`. The handler no longer leaks unhandled rejections. - F3: retire the lexical startsWith check; the realpath branch subsumes it and it provided zero symlink defense. - F4: normalize relative paths with path.posix.normalize before reporting; applied/skipped now carry canonical forms. Reject '.' explicitly. Retain the raw-segment '..' guard per spec so 'a/../b' is still rejected rather than silently collapsing to 'b'. - Reject NUL in path keys at the boundary instead of crashing. - Test matrix adds: F1 escaping symlink rejected; F1 intra-baseDir symlink permitted; F2 force-over-directory returns write-failed; F2 NUL-in-filename returns invalid-path; F4 './sub/x.txt' normalizes to 'sub/x.txt'; F4 '.' rejected; insertion order stability for numeric-ish keys. submodule-bump - F5: description now says full SHA; docstring no longer advertises short SHAs (`git fetch <url> <sha>` requires a ref/full SHA). - F6: `staged` is computed from previousSha \!== resolvedSha. Bumping to the same SHA reports staged=false honestly, so callers don't create empty commits. - F7: targetSha regex tightened to /^[0-9a-f]{7,40}$/i. A 1-char or 6-char "sha" now fails at the Zod gate, not downstream. - F8: parentDir promoted to SafeString for consistency with the other free-form fields. - Test matrix adds: F6 bump-to-same reports staged=false; F7 rejects 1-char and 6-char hex; F8 rejects newline in parentDir. Files-Changed: 4 Key-Finding: `fs.realpath` on macOS resolves `/tmp` to `/private/tmp`, so result.data.baseDir no longer equals `path.resolve(tmp)` in tests. Switched the P2 assertion to `await fs.realpath(tmp)` — the realpath-based escape check means the handler has to commit to the canonical form as authoritative. Agent-Id: moss Session-Id: 24c9a87b-5fd1-4fd4-83cf-b8e6f15bb73c Task-Status: COMPLETED Heartbeat: 2026-04-19T00:35:00Z
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two paired ops tools that together close the "scaffolder → disk → submodule pointer" loop opened by
project-spawn:project-apply— writes a{files: {path: content}}map to disk under a baseDir. Realpath-based sandbox, dry-run, force-overwrite opt-in, structurederr()on FS failures.submodule-bump— advances a git submodule's recorded pointer in the parent to a given commit. Fetches from the correct upstream (input override or.gitmodules), resolves the full SHA, stages the gitlink — does not commit. Caller composes the message.Both orchestrator-only, side-effectful, following the
project-spawnprecedent for ZodSafeString+safeParse-at-boundary defense-in-depth.What landed
src/tools/project-apply.ts— handler + schemas.src/tools/submodule-bump.ts— handler + schemas.tests/tools/project-apply.test.ts— 18 cases (minimal, nested, dry-run, force/no-force, multi-file, 4 invalid-path, write-failed on EISDIR + NUL, symlink escape refused, intra-baseDir symlink allowed, numeric-key order stability, normalization).tests/tools/submodule-bump.test.ts— 12 cases (happy path, previousSha read, remoteUrl override, 3 negative rule cases, targetSha regex edges, bump-to-same staged:false, parentDir newline rejection).src/tools/index.ts— both registered in Phase 4.tests/runtime/mcp-server.test.ts— tool count 30 → 32, both names added.Thorn review arc
Initial pass had three real defects: symlink-based path escape, uncaught FS errors crashing the handler, and a
staged: trueflag that lied whenpreviousSha === targetSha. All addressed:fs.realpathSyncon the resolved parent aftermkdir -p; intra-baseDirsymlinks still permitted.err('write-failed', …, false).stagedcomputed frompreviousSha \!== resolvedSha.targetShatightened to/^[0-9a-f]{7,40}$/i.parentDirguarded withSafeStringfor consistency.Final test result: 508/508 green.
npm run buildclean.Orchestration
Writer: moss. Reviewer: thorn (schema/edge stress, REQUEST_CHANGES addressed).
Follow-up
Submodule pointer bump in
bitswell/bitswellfollows in a separate PR (will usesubmodule-bumpitself once it's live in the next Claude Code session).Agent-Id: bitswell
Session-Id: 24c9a87b-5fd1-4fd4-83cf-b8e6f15bb73c