[FORGE-115] feat(skills,init): standardize worktree location on .forge/worktrees/#161
Merged
firatcand merged 5 commits intoMay 17, 2026
Merged
Conversation
…, anchor on git-common-dir
- Move worktree location from `../${PROJECT}-worktrees/${TICKET}/` to
`.forge/worktrees/${SANITIZED_ID}/` (inside repo, gitignored, project-relative).
Converges /pickup-task with the orchestrator path which has been here all along.
- Anchor on `git rev-parse --git-common-dir` instead of pwd/--show-toplevel so
invocations from inside an existing worktree don't nest the new one.
- Bash sanitization guard mirrors src/core/workspace.ts sanitizeIssueId rules
(length≤64, [A-Za-z0-9._-] only, no leading -, no /, no \\, no ..) and exits 1
on invalid input — skill produces a filesystem path, must not silently degrade.
- Hydration cp commands now read from REPO_ROOT, not pwd, so calls from a sibling
worktree don't silently no-op (was a latent footgun pre-this-change too).
- Update Next: output (cd .forge/worktrees/...) and "✓ Worktree created" example.
Part of FORGE-115 / P2.5-T19.
…ybrid)
`forge init` now writes tooling-exclude entries so eslint/prettier/tsc/vitest
don't recurse into worktrees that live inside the repo at `.forge/worktrees/`.
Hybrid strategy:
- Flat-ignore files (.eslintignore, .prettierignore): append `.forge/worktrees/`
via new exported helper `appendLineIfMissing(path, line)`. Idempotent —
records under `written` first run, `skipped` thereafter.
- Code configs (tsconfig.json, vitest.config.{ts,js,mjs,cjs}): WARN-ONLY.
Emit a copy-paste snippet to `.forge/init-warnings.md` rather than risk
corrupting the user's config (tsconfig.json may have // comments and
reject JSON.parse; vitest.config.* is TypeScript with no safe AST edit).
- All four targets detect-and-skip: silent no-op if the file doesn't exist.
Warnings sidecar reshape: `buildWarningsBody` now takes both `unverified[]`
and `toolingWarnings[]`, emits sections only for non-empty input. Existing
unverified-only callers still produce a single-section file (regex tests
matching `primary_host` etc. still pass).
13 new tests: appendLineIfMissing (4), appendToolingExcludes (6), end-to-end
scaffoldProject with all four tooling targets (1), sidecar merge (1), plus
existing tests updated for the new buildWarningsBody signature.
Decision: F2 (init scaffold aggressiveness) → hybrid (eslint/prettier append,
tsconfig/vitest warn-only) per user answer to plan AskUserQuestion.
Part of FORGE-115 / P2.5-T19.
Aligns user-facing docs + SPEC + CHANGELOG + BACKLOG with the new canonical worktree location landed in this branch's earlier commits. - docs/WORKTREES.md: directory-layout diagram now shows .forge/worktrees/ inside the project (not sibling); "convention" paragraph rewritten; command examples (worktree add/remove, cleanup) use the new path. - docs/QUICKSTART.md: daily-flow "cd" line updated. - docs/LIFECYCLE.md: /pickup-task "Outputs" description updated. - spec/SPEC.md §Module layout: comment on workspace.ts now states the canonical path; new "Worktree location convention" sub-section after the layout codeblock documents single-source-of-truth + forge init's hybrid tooling-exclude behavior. - plans/BACKLOG.md: sandbox-grant snippet (ergonomic enhancements item) references the new path so the future implementation matches reality. - CHANGELOG.md: new "Behavior change" entry under [Unreleased] with migration steps for v0.3.x adopters who already have sibling worktrees. Grep-purge verified: zero `../*-worktrees/` references remain across src/, skills/, templates/, docs/, spec/ — the only hit is the CHANGELOG migration note itself, which has to mention the old path to describe what changed. Part of FORGE-115 / P2.5-T19.
…e-safe glob
Addresses code-reviewer (1 BLOCK + improvements) and security-auditor
(1 HIGH + 1 MEDIUM + 1 LOW) on the FORGE-115 diff.
scaffold.ts:
- New `safeReadConfig(filePath)` helper: stat-then-read inside try/catch so
a concurrent delete between syscalls returns null instead of leaking
ENOENT (the bug class documented in docs/learnings/2026-Q2/
toctou-between-stat-and-read-leaks-raw-fs-errors.md). Enforces a
MAX_CONFIG_BYTES = 1 MiB cap so a symlink to /dev/zero or pathological
config can't stall init via unbounded readFileSync.
- `appendLineIfMissing` now routes through safeReadConfig (TOCTOU-safe) and
CRLF-normalizes before split('\n').includes(line) so Windows-checked-out
or autocrlf=true files don't double-append on re-run.
- `appendToolingExcludes` tsconfig.json + vitest.config.* reads go through
safeReadConfig (same TOCTOU + size-cap protection). False-positive caveat
on marker-substring detection in // comments documented in the function
body, not silently lurking.
skills/pickup-task/SKILL.md:
- Hydration cp commands switched from `cp "${REPO_ROOT}"/spec/*.md ...`
(glob expands after quoted variable → re-splits on whitespace if the
user's repo path contains a space, e.g. macOS "/Users/foo bar/...") to
`find "${REPO_ROOT}/spec" -maxdepth 1 -name '*.md' -exec cp {} ... \;`
which handles space-containing paths correctly.
Tests:
- New "CRLF idempotency" test asserts `\r\n`-separated files don't
double-append (regression for the BLOCK).
- New "size cap" test asserts files over 1 MiB are treated as missing
(regression for the MEDIUM).
890 pass / 0 fail (was 888). typecheck clean. build clean. pack clean.
Part of FORGE-115 / P2.5-T19.
…le-pin bash sanitizer Addresses two of three Codex IMPROVEMENT findings on the FORGE-115 diff. scaffold.ts safeReadConfig: - Treat EACCES, EISDIR, ELOOP as "unreadable" alongside ENOENT. Previously a directory at `cwd/.eslintignore`, a symlink loop, or a permission-denied read would surface a raw fs error and abort `forge init`. These files are optional probes — the right behavior is "no entry scaffolded for it", not "crash the whole flow". Codex caught the EISDIR path explicitly. - Inline `isFsError` / `isUnreadable` helpers keep both syscall catches DRY. - Docblock now enumerates every error class we tolerate and explicitly notes the remaining size-cap TOCTOU as deferred (not exploitable in the local-init threat model; tightening would require openSync+fstatSync+bounded-read). skills/pickup-task/SKILL.md sanitizer: - Wrap the [[ ... ]] tests in an LC_ALL=C scope (save/restore around the block) so the [A-Za-z0-9._-] character class has deterministic ASCII semantics in exotic locales. `[[` is a bash builtin so the VAR=val inline-prefix form doesn't apply — explicit save/restore is the correct pattern. Codex findings explicitly deferred (rationale in code comments): - Size-cap TOCTOU window — confidence 7, "not merge-blocking for a local init helper" (per Codex). Local-only threat model; complexity not warranted. - tsconfig comment false-positive — confidence 8. Plan explicitly rejected a JSONC parser. Worst case = no warning shown for path the user already has; never corrupts state. Documented in code comment. - Classic-Mac \r-only line endings — confidence 9 NIT. File format extinct. - BOM-prefixed first-line edge — confidence 9 NIT. Vanishingly rare for .eslintignore / .prettierignore; safe-fail. Codex BLOCK verdict: None (confidence 8). 890 pass / 0 fail. typecheck clean. build clean. Part of FORGE-115 / P2.5-T19.
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
1 participant
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
Closes FORGE-115 / P2.5-T19. Eliminates the accidental split where
/pickup-taskcreated worktrees at../${PROJECT}-worktrees/${TICKET}/while the orchestrator already used.forge/worktrees/<sanitized-id>/. After this lands, both paths converge on the orchestrator's pre-existing convention (inside-repo, gitignored, project-relative).skills/pickup-task/SKILL.md— worktree path moves to.forge/worktrees/${SANITIZED_ID}/. New bash sanitization guard mirrorssrc/core/workspace.tssanitizeIssueId(LC_ALL=C-pinned for deterministic ASCII semantics). Anchor switched frompwd/--show-topleveltogit rev-parse --git-common-dirso invocations from inside an existing worktree don't nest. Hydrationcpcommands rewritten asfind -execso repo paths containing spaces work correctly.src/cli/init/scaffold.ts— new exported helpersappendLineIfMissingandappendToolingExcludesimplement the hybrid scaffolding strategy:.eslintignore/.prettierignoreget a one-line append (idempotent under CRLF);tsconfig.json/vitest.config.*get a copy-paste snippet emitted to.forge/init-warnings.mdrather than auto-mutation (avoids corrupting JSONC tsconfigs or TS configs withextends/composite/references). NewsafeReadConfighelper enforces a 1 MiB cap and treats ENOENT/EISDIR/ELOOP/EACCES as "unreadable" so optional probes never crashforge init.docs/{QUICKSTART,WORKTREES,LIFECYCLE}.md,spec/SPEC.md §Module layout(new "Worktree location convention" sub-section),plans/BACKLOG.mdsandbox snippet, andCHANGELOG.md [Unreleased]"Behavior change" entry with adopter migration steps.Scope reality check (post-#160)
PR #160 already shipped the
.forge/worktree-task.jsonmarker contract,skills/_shared/worktree-guard.md, and the worktree-guard preflight on all task-scoped skills. The guard is path-agnostic — no changes were needed here. This PR is the remaining path-move + init scaffolding work.Acceptance criteria (all met)
/pickup-taskcreates worktrees at.forge/worktrees/<sanitized-id>/forge initwrites tooling-exclude entries per hybrid strategyspec/SPEC.md §Module layoutdocuments.forge/worktrees/as canonical../*-worktrees/references remain insrc/,skills/,templates/,docs/,spec/(sole hit: the CHANGELOG migration note describing what changed)forge doctor— explicitly out of scope (doesn't exist yet; T08/FORGE-99)Test plan
npm run typecheck— cleannpm test— 890 pass / 0 fail / 11 pre-existing skips. 14 new tests: 4×appendLineIfMissing(including CRLF-idempotency + 1 MiB size-cap regressions), 6×appendToolingExcludes(eslint/prettier append + tsconfig/vitest warn-only), 2×scaffoldProjectend-to-end + sidecar mergenpm run build— clean ESM + CJSnode dist/bin/forge.cjs --version→0.2.2npm run test:pack— 0 forbidden paths in 65-file tarballgitleaks detect— 0 secretsgrep -rn '\.\./.*-worktrees' src/ skills/ templates/ docs/ spec/— 0 hits/pickup-taskin a fresh adopter sandbox, confirm worktree lands at.forge/worktrees/<id>/forge initin a fixture with.eslintignore+tsconfig.jsonpresent, confirm hybrid scaffolding produces correctwritten/skipped/warnedclassificationReviews
Three review passes ran against this diff:
appendLineIfMissing) + 3 IMPROVEMENT (TOCTOU, comment false-positive, missing regression test). BLOCK fixed in4e491ff.toctou-between-stat-and-read-leaks-raw-fs-errors.md), 1 MEDIUM (unboundedreadFileSync), 2 LOW. HIGH+MEDIUM+LOW#1 fixed in4e491ff./codexper ETHOS principle [P1-T02] Define settings.yaml zod schema #6 — required for CRITICAL paths) — BLOCK: None (confidence 8). 3 IMPROVEMENT (broadensafeReadConfigto non-ENOENT errors; size-cap TOCTOU; tsconfig comment false-positive) + 2 NIT (classic-Mac\r, BOM, locale gap on bash sanitizer). Broaden-safeReadConfig+ LC_ALL=C fixed inc52d2c8. Others explicitly deferred with rationale in commit body and inline code comments.Decisions surfaced via AskUserQuestion (per ETHOS Confusion Protocol)
forge initscaffolding aggressiveness → Hybrid (append for flat-ignore, warn-only for code configs). Reason captured asDecision:line in commit21ad78c.4e491ff.🤖 Generated with Claude Code