[FORGE-97] Worker prompt template + render + guardrail-check verb (P2.5-T06)#168
Merged
Merged
Conversation
…drail-check verb (P2.5-T06)
Replaces the (uncommitted) 6-level precedence chain + drift event/question
protocol in spec/ORCHESTRATOR.md with the authority-by-field matrix locked
on 2026-05-17 PM (docs/plans/team-mode-minimum-architecture.md §2.1). Ships
the host-portable template, a pure renderer, and the CLI verb the worker
calls before writing to guardrail globs.
New files:
- templates/worker-prompt.template.md (237 lines) — authority-by-field
matrix + 5 worked examples + 70/30 escalation rule + heartbeat protocol
+ preflight wrapper + host-conditional blocks for Claude/Codex.
- src/orchestrator/render-worker-prompt.ts — pure renderer; allowlisted
{{TOKEN}} placeholders; strips other-host <!-- host: ... --> blocks.
- src/orchestrator/glob-match.ts — minimal **/*/literal glob matcher;
zero new deps.
- src/cli/orchestrate/guardrail-check.ts — new read-band verb
`forge orchestrate guardrail-check --path <p>`. Returns
{architectural, matched_glob, suggested_decision_key}. Best-effort
guardrail_checked event when --task + --attempt are supplied.
- test/unit/ ... — 4 new test files; 48 new tests.
Modifications:
- src/schemas/settings.ts — agents.preflight_globs (default = SPEC's
guardrail list).
- src/schemas/attempt.ts — guardrail_checked event type added to the
discriminated union.
- src/cli/orchestrate/index.ts — register guardrail-check verb in the
read-only band.
- src/cli/init/scaffold.ts — settings literal carries default
preflight_globs.
- test/unit/settings.schema.test.ts — 2 new tests (default expansion,
override).
- spec/ORCHESTRATOR.md — §Worker prompt template rewritten to describe
the new template contract; §Preflight wrapper rewritten to drop the
"mechanical wrapper" claim (forge can't intercept host tool calls);
amendment-header process note retired (this PR closed the trim).
11 architectural forks resolved via AskUserQuestion at /plan-task time
(see plans/tasks/FORGE-97.plan.md §7). Highlights:
- Disagreement → standard `forge orchestrate question` verb (no drift
machinery).
- Scope → template + minimal renderer (FORGE-98 will consume).
- Preflight → CLI verb + prompt-discipline + post-hoc audit, not a
mechanical wrapper (forge has no PreToolUse hook into Claude
Task/Codex subagents).
- Host portability → single template with <!-- host: claude --> blocks
stripped at render time.
- Placeholder syntax → {{TOKEN}} mustache against an allowlist.
- Test strategy → template-shape tests + renderer unit tests; integration
test (worker behaving on artifact disagreement) deferred to
FORGE-FOLLOWUP-B since it depends on FORGE-98 dispatch skill.
Follow-ups filed:
- FORGE-FOLLOWUP-A: post-hoc enforcement of guardrail-check calls in
complete.ts (cross-references events vs verdict.files_changed).
- FORGE-FOLLOWUP-B: end-to-end test of authority-collision question flow
(blocked on FORGE-98).
- FORGE-122: worktree-infra issue — 6 e2e/perf tests hardcode
${repoRoot}/node_modules/.bin/tsx; fails from worktree, passes from
main checkout. Unrelated to this PR.
Acceptance:
- typecheck clean; 1081/1098 tests pass (6 pre-existing worktree-infra
failures unrelated, filed as FORGE-122).
- npm run build clean; dist/bin/forge.cjs orchestrate --help shows
guardrail-check in the read-only band.
- No references to drift_event_id, routing_hint, --drift-event-id,
--type drift, "6-level", or "precedence chain" in the new template
or in the rewritten §Worker prompt template / §Preflight wrapper
sections.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…IMPROVEMENTs + 3 NITs) Multi-model review (code-reviewer + security-auditor + Codex per multi-model-review-catches-distinct-bug-classes.md) surfaced 13 findings across the FORGE-97 diff. All addressed in this commit; the original commit (d099a77) plus this one squash-merge into a single PR landing. BLOCKs - B1 [Codex] guardrail-check.ts:87 — symlink bypass: lexical `path.relative` containment treats a symlinked file inside the repo pointing to `/tmp` or `/etc` as inside-repo. Fix: `realpathSync` both the repoRoot and the longest-existing prefix of the target before computing the relative path. Refuse explicitly when the realpath escapes the repo. New helper `resolveRepoRelative` + `realpathOfLongestPrefix` (handles "target doesn't exist yet" without skipping symlink resolution on ancestors). - B2 [Codex] template + spec/ORCHESTRATOR.md — false enforcement claim. Prompt + SPEC asserted "`complete` cross-references guardrail events and rejects on skip" but that's FORGE-FOLLOWUP-A, not in this PR. Workers could skip `guardrail-check` today with zero penalty. Softened both surfaces to honest framing: a forthcoming release will ship the cross-reference; until then, calling guardrail-check is prompt-discipline only. - B3 [security-auditor] guardrail-check.ts:159 — unguarded FS probe via `--task`. A worker calling `--task ../../../etc --attempt foo` would cause `existsSync`/`readFileSync` to probe arbitrary filesystem paths before `appendAttemptEvent`'s validateIdSegment ran. Fix: `validateIdSegment(args.taskId, 'taskId')` and `validateIdSegment(args.attemptId, 'attemptId')` at the top of `appendGuardrailEvent`, before any `path.join`. Lease path now built via the validated `leaseFilePath` helper. IMPROVEMENTs - I1 [Codex] glob-match.ts:40 — `**` was compiled as raw `.*`, so `src/**/foo.ts` matched `src/barfoo.ts` (no segment boundary). Fix: `**/` → `(?:[^/]+/)*` (zero-or-more whole segments). Bare `**` at end → `.+` (one-or-more chars, no longer matches the empty trailing of `src/`). New test case: `src/**/foo.ts` matches `src/foo.ts` and `src/a/b/foo.ts` but NOT `src/barfoo.ts`. - I2 [Codex + code-reviewer] template Example 1 — said "tracker body owns scope," contradicting the matrix row that gives Tracker only execution metadata. Rewritten to use phases.yaml as the scope-carrying artifact (matches matrix). - I3 [Codex + code-reviewer] spec/ORCHESTRATOR.md:76 — stale `templates/worker-prompt.md` (old name) on a line this PR touched. Renamed to `worker-prompt.template.md` and added a reference to the renderer module. - I4 [security-auditor] guardrail-check.ts:121-133 — out-of-repo path leaked into response envelope (`../../etc/passwd` revealed repoRoot depth). Fix: explicit `INVALID_ARGS` rejection for any path whose relative starts with `..` or is absolute, before constructing the result envelope. The verb no longer echoes any out-of-repo path back. - I5 [code-reviewer + security-auditor] scaffold.ts:142-155 — hardcoded preflight_globs list duplicated the schema default. Future default-list updates would silently diverge from scaffolded YAML. Fix: omit the field from `toSettingsObject`; rely on zod's `.default()`. - I6 [code-reviewer] settings.ts:104 — `src/cli/migrate.ts` was dead in the default list because `src/cli/**` (earlier in the list) matches first. Removed. Comment in `glob-match.test.ts` already noted this. - I7 [code-reviewer] guardrail-check.ts:191 — silent catch on `appendAttemptEvent` violated CLAUDE.md "no silent catches." Replaced with a `process.stderr.write` warning. The first catch (lease parse failure) is intentional and documented. NITs - N1 [code-reviewer] guardrail-check.ts:125-126 — dead `?? null` / `?? ''` branches removed; `match.matched === true` guarantees `matchedGlob` is a defined string. - N2 [code-reviewer] glob-match.ts:81 — `compileGlob` now memoizes compiled regexes in a module-level Map. Cache hits return the cached RegExp; misses compile once and cache. Tiny test-only helper `__resetGlobMatchCacheForTests` keeps tests deterministic. - N3 [Codex] template tests:28 — added two semantic checks: (a) the Tracker matrix row must not claim ownership of "scope," (b) the preflight wrapper section must label complete-enforcement as "forthcoming"/"follow-up"/"planned" (catches future regressions of the B2 false-enforcement claim). Verification - typecheck clean. - 1089/1106 tests pass; 6 failures are pre-existing worktree-infra issues (FORGE-122). - `npm run build` clean; `dist/bin/forge.cjs orchestrate --help` shows `guardrail-check` in the read-only band. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Lands the host-portable worker prompt template every spawned forge worker subagent reads at task start, plus a pure renderer and a new CLI verb (
forge orchestrate guardrail-check) that workers call before writing to architecturally-sensitive paths.Replaces the uncommitted 6-level precedence chain + drift event/question protocol in
spec/ORCHESTRATOR.mdwith the authority-by-field matrix locked on 2026-05-17 PM (docs/plans/team-mode-minimum-architecture.md§2.1). Critical-path unlock for FORGE-98 (dispatch skill rewrite) and FORGE-99 (doctor).What changed
templates/worker-prompt.template.md(NEW, 237 lines) — Authority-by-field matrix + 5 worked examples + 70/30 escalation rule + heartbeat protocol + preflight wrapper + host-conditional blocks (<!-- host: claude --> ... <!-- /host -->).src/orchestrator/render-worker-prompt.ts(NEW) — Pure renderer;{{TOKEN}}placeholders validated against an allowlist; strips other-host blocks. Throws on unknown placeholders or unterminated blocks.src/orchestrator/glob-match.ts(NEW) — Minimal**/*/literal glob matcher. Zero new deps.**/enforces segment boundaries (sosrc/**/foo.tsdoesn't matchsrc/barfoo.ts). Compiled regexes memoized.src/cli/orchestrate/guardrail-check.ts(NEW) — Read-band verbforge orchestrate guardrail-check --path <p>. Realpath-based containment (rejects symlinks escaping the repo). Returns{architectural, matched_glob, suggested_decision_key}. Best-effortguardrail_checkedevent when--task+--attemptare supplied (validated viavalidateIdSegmentbefore any FS access).src/schemas/settings.ts—agents.preflight_globs: string[]with default list (10 patterns covering schemas/CLI/specs/CRITICAL/CLAUDE/AGENTS/package.json/phases.yaml).src/schemas/attempt.ts— Addedguardrail_checkedevent type to the discriminated union.spec/ORCHESTRATOR.md— §Worker prompt template rewritten to describe authority-by-field; §Preflight wrapper rewritten to drop the false "mechanical wrapper" claim (forge can't intercept host tool calls); amendment process-note retired.Why
The 2026-05-17 PM team-mode-minimum-architecture pivot dropped the 6-level chain + drift workflow. Without this rewrite the worker prompt would still encode the dropped design. Every worker dispatched by FORGE-98 (next ticket) needs the new template as its system prompt.
Architectural decisions (11 forks asked at /plan-task)
See
plans/tasks/FORGE-97.plan.md§7 for the full table. Highlights:forge orchestrate questionverb, no drift machinery.<!-- host: claude --> ... <!-- /host -->blocks; renderer strips other-host content before the prompt reaches the host.{{TOKEN}}against an allowlist (rejects${...}/<TOKEN>alternatives for safety/clarity).worker-prompt.template.md(matches the.template.mdconvention of the other 7 templates).Review pass
Multi-model review (per
multi-model-review-catches-distinct-bug-classes.md):codex exec) — 2 BLOCKs, 4 IMPROVEMENTs, 2 NITs.Zero overlap on BLOCKs across reviewers — confirms the multi-model thesis. All 13 findings addressed in commit
4ed1e6e:realpathSyncon repoRoot + target.completedoesn't actually cross-reference yet) → softened to "forthcoming follow-up."--task→validateIdSegmentat top ofappendGuardrailEventbefore anypath.join.**/segment-boundary fix.worker-prompt.mdfilename in ORCHESTRATOR.md.src/cli/migrate.tsremoved (dead behindsrc/cli/**).process.stderr.writewarning.?? null/?? ''branches.compileGlobmemoized.How to test
npm test— full suite passes from main checkout (1083/1083). From a.forge/worktrees/<id>checkout, 6 e2e/perf tests fail due to FORGE-122 (hardcoded${repoRoot}/node_modules/.bin/tsxdoesn't exist in worktrees — pre-existing, unrelated).npm run typecheck— clean.npm run build— clean.node dist/bin/forge.cjs orchestrate --helpshowsguardrail-checkin the read-only band.node dist/bin/forge.cjs orchestrate guardrail-check --path src/schemas/foo.ts --json(in any forge-initialized project) returns{architectural: true, matched_glob: "src/schemas/**", ...}./tmp/evil.ts, run guardrail-check against it → returnsINVALID_ARGS(does not leak path).{{TOKEN}}placeholders are substituted and host-blocks for the wrong host are stripped.Follow-ups filed at /plan-task time
guardrail-checkcalls incomplete.ts(cross-reference verdict'sfiles_changedagainstguardrail_checkedevents; markverdict_unverifiedon mismatch). Roughly 30 LOC; deferred to keep this PR'scomplete.tstouch zero.${repoRoot}/node_modules/.bin/tsx). Unrelated to this PR; surfaced during/review.Linked
docs/plans/team-mode-minimum-architecture.md🤖 Generated with Claude Code