feat(ambient)!: replace keyword/plan detection with orchestrator charter + plan handoff

[dean0x]

Summary

• Removes both old ambient detection paths: first-word keyword dispatch (implement, explore, research, debug, plan) and 3-marker plan detection (## Goal / ## Steps / ## Files). These added overhead and duplicated slash commands.
• Adds session-start-orchestrator — a SessionStart hook that injects a static ~200-token orchestrator charter as additionalContext at every session start, establishing the main model as a pure delegator and grading sub-agents by complexity (haiku → sonnet → opus → full workflows).
• Rewrites preamble to three clean behaviors: plan-handoff fast-path (Implement the following plan: prefix → devflow:implement), slash skip (no output), and a 2-line orchestrator reminder for all other prompts. Both hooks are git-repo-gated via a new sourced git-marker helper (pure bash, 64-level bounded walk, no subprocess).
• Both hooks share the same devflow ambient --enable/--disable toggle. Partial state (preamble present, orchestrator missing) is detected and reported by devflow ambient --status with a repair note.

Changes

• scripts/hooks/orchestrator-charter.md — static charter asset (≤4096 chars, enforced)
• scripts/hooks/session-start-orchestrator — new SessionStart hook; DEVFLOW_BG_UPDATER re-entrancy guard; fail-open
• scripts/hooks/preamble — rewritten; old keyword case statement and 3-marker detection removed
• scripts/hooks/git-marker — sourced helper: df_has_git_marker; bash 3.2 compatible
• src/cli/commands/ambient.ts — ORCHESTRATOR_HOOK_MARKER, partial-state detection/repair, updated messaging
• src/cli/commands/init.ts / uninstall.ts — updated descriptions
• Tests: 1881 passing; new describe('session-start-orchestrator', ...) block + full preamble — orchestrator charter mode rewrite; tests/integration/ambient-activation.test.ts updated

Breaking Changes

Both old detection paths are removed. Sessions that relied on keyword-prefixed prompts auto-routing workflows will no longer get that behavior — the orchestrator charter approach delegates routing to the model's judgment based on context, not string matching.

Reviewer Focus Areas

• scripts/hooks/session-start-orchestrator: verify it mirrors session-start-context structure (no set -e, fail-open, re-entrancy guard)
• scripts/hooks/preamble: confirm clean removal of shopt -s nocasematch, keyword case, and 3-marker grep — no residue
• scripts/hooks/git-marker: check the 64-level walk handles /, empty string, and .git-as-file correctly
• src/cli/commands/ambient.ts: hasAmbientHook (preamble-authoritative), hasOrchestratorHook, partial-state detection, idempotent addAmbientHook
• orchestrator-charter.md: content correctness — model tier routing, plan-handoff fallback bullet, ≤4096 char budget

Upgrade Note

After upgrading, run devflow init to register the new session-start-orchestrator hook. Existing installs with only the preamble hook are in partial state — devflow ambient --enable repairs it.

T-5 empirical outcome: pending live verification post-merge deploy.

Prefix re-validation (2026-07-04, pre-merge): The handoff prefix was re-confirmed live — this feature's own implementation session was a native "execute plan in new session" handoff at v2.1.198, and its first user prompt begins with the literal Implement the following plan:. Local transcript survey shows the prefix stable across v2.1.167→v2.1.198 with no alternative phrasing in any project; changelog/docs through v2.1.201 show no handoff-format change. Schema note: transcripts now carry the plan body in a top-level planContent field with origin: {"kind":"auto-continuation"} — message.content holds only the prefix. This is harmless to the fast-path (the match is prefix-anchored and the model still receives the full plan in context), but the origin tag is a plausible discriminator Claude Code could use to skip UserPromptSubmit for injected prompts — T-5 remains the definitive post-deploy test.

[dean0x]

Review Summary: PR #251 Code Comments

Review findings consolidated and deduplicated across 10 reports. Posting inline comments for ≥80% confidence blocking/should-fix issues.

Comment Strategy

• Inline comments: Blocking and should-fix findings at ≥80% confidence
• This summary: Lower-confidence suggestions and summary recommendations
• Deduplication: Performance + Reliability + TypeScript double-parse consolidated into one point

Key Blocking Issues (≥80% confidence)

1. README.md Lines 19 & 44 (CRITICAL, 97%) — Stale keyword/structured-plan prose
2. ambient.ts:226-238 (MEDIUM, 80%, Architecture) — Status observability gap for orchestrator-only state
3. ambient.ts:112-130 (MEDIUM, 80%, Complexity) — Duplicated hook-registration block
4. ambient.ts:227 (MEDIUM, 80%, Reliability) — Unguarded JSON.parse crashes on corrupt settings
5. KNOWLEDGE.md:33 (MEDIUM, 88%, Documentation) — Dispatch table contradicts code for empty prompts
6. shell-hooks.test.ts:637 (MEDIUM, 90%, Testing) — AC-F12 test mislabeled, doesn't verify claimed window
7. git-marker testing (MEDIUM, 88%, Testing) — No direct test for subprocess-purity invariant

Should-Fix Issues (≥80% confidence)

8. ambient.ts:88-137 (LOW, 80%, Regression) — addAmbientHook asymmetry: doesn't sweep classification hook
9. ambient.ts:113 (LOW, 80%, TypeScript) — Inline hasOrchestratorHook shadows module-level function
10. shell-hooks.test.ts:947 (LOW, 80%, Testing) — 4096-byte charter boundary (exactly 4096) untested

Lower-Confidence Suggestions (60-79%)

• Double JSON.parse optimization (65-88%) — Lines 227-229: parse once, pass object to both functions
• Three near-identical predicates (65%) — isAmbient, hasPreamble, hasAmbientHook each re-encode "is preamble present"
• Model-tier routing taxonomy duplication (60%) — haiku/sonnet/opus mapping in charter and preamble reminder
• Integration test coupled to hook output (70%) — systemPrompt hardcoded; should reference shell-hooks templates
• Non-git tests environmental dependency (65%) — F6a-F6b rely on TMPDIR having no .git ancestor

---

Recommendation: APPROVED_WITH_CONDITIONS
Address the README documentation and KNOWLEDGE.md dispatch-table contradictions (CRITICAL/MEDIUM) before merge. All other findings are fixable and improve robustness.

Generated by Claude Code devflow:comment-pr operation

[dean0x]

Status observability gap: Orchestrator-only state reports as disabled (MEDIUM, 80%)

When hasAmbientHook returns false (no preamble) but the orchestrator SessionStart hook exists, this line prints plain disabled with no hint about the stray hook.

The orchestrator hook fires every session and injects the charter, so the reported state contradicts observable behavior. The mirror partial-state case (preamble without orchestrator) correctly gets a partial hint on line 231.

Fix: Check for the orchestrator-only case in the else branch:

} else {
  if (hasOrchestratorHook(settingsContent)) {
    p.log.info(`Ambient mode: ${color.dim('disabled')} ${color.dim('(partial — stray orchestrator hook; run devflow ambient --disable to clean up)')}`);  
  } else {
    p.log.info(`Ambient mode: ${color.dim('disabled')}`);
  }
}

[dean0x]

Duplicated hook-registration block: SessionStart mirrors UserPromptSubmit (MEDIUM, 80%)

The SessionStart orchestrator add-block (lines 112-130) is structurally identical to the UserPromptSubmit preamble add-block (lines 92-110). Both perform the same some(... includes(MARKER)) check, then idempotent add.

This duplication creates a future maintenance burden. The feature KB warns: "Adding a third presence-gate separately… must be added to both functions [and] the partial-state detection logic." This duplication is exactly what makes that fragile.

Fix: Extract a helper to avoid keeping two copies in sync:

function ensureHook(
  settings: Settings, event: 'UserPromptSubmit' | 'SessionStart',
  marker: string, hookName: string, timeout: number, devflowDir: string,
): boolean {
  const present = settings.hooks?.[event]?.some((m) =>
    m.hooks.some((h) => h.command.includes(marker)));
  if (present) return false;
  settings.hooks ??= {};
  settings.hooks[event] ??= [];
  settings.hooks[event]!.push({
    hooks: [{ type: 'command', command: path.join(devflowDir, 'scripts', 'hooks', 'run-hook') + ` ${hookName}`, timeout }],
  });
  return true;
}
// Then:
if (ensureHook(settings, 'UserPromptSubmit', PREAMBLE_HOOK_MARKER, 'preamble', 5, devflowDir)) changed = true;
if (ensureHook(settings, 'SessionStart', ORCHESTRATOR_HOOK_MARKER, 'session-start-orchestrator', 10, devflowDir)) changed = true;

[dean0x]

Unguarded JSON.parse crashes on corrupt settings.json (MEDIUM, 80%)

This path calls hasAmbientHook(settingsContent) → JSON.parse(input) with no surrounding try/catch. If ~/.claude/settings.json is corrupt (trailing comma, etc.), this throws an uncaught exception and the CLI aborts with a raw stack trace.

The same function guards another parse 15 lines away (line 247-260) with a try/catch and graceful degradation. The primary paths (status/enable/disable) should do the same.

Fix: Parse once upfront and degrade gracefully:

let settingsContent: string;
try {
  settingsContent = await fs.readFile(settingsPath, 'utf-8');
} catch (err) {
  if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err;
  // ... handle ENOENT
}

let settings: Settings;
try {
  settings = JSON.parse(settingsContent);
} catch (err) {
  p.log.error(`~/.claude/settings.json is not valid JSON: ${(err as Error).message}`);
  if (options.status) return;
  // ... handle for enable/disable
}

const enabled = hasAmbientHook(settings);
const hasOrchestrator = hasOrchestratorHook(settings);

Both predicates already accept Settings objects, removing the redundant double-parse at lines 227/229 (also addresses the performance optimization concern).

[dean0x]

Dispatch table contradicts code for empty prompts (MEDIUM, 88%)

The KB dispatch table row reads: Anything else (including empty-after-strip) | 2-line orchestrator delegation reminder

But the actual preamble code handles empty-after-strip as its own first branch that emits nothing (silent exit 0):

if [ -z "$HEAD" ]; then
  dbg "EXIT: empty prompt"          # no json_prompt_output — silent
elif [[ "$HEAD" == "Implement the following plan:"* ]]; then ...
elif [[ "$HEAD" == "/"* ]]; then ...
else
  json_prompt_output "..."          # reminder only here
fi

Since this KB is the feature knowledge base that future agents/developers will trust as the model of this hook, the inaccuracy will mislead work.

Fix: Split the table so empty-after-strip is its own row:

| First word begins Implement the following plan: | Fixed devflow:implement handoff directive |
| First char is / | Silent exit 0 (slash commands pass through) |
| Empty after whitespace strip | Silent exit 0 (no output) |
| Anything else | 2-line orchestrator delegation reminder |

[dean0x]

AC-F12 test is mislabeled and under-asserts the 256-byte window bound (MEDIUM, 90%)

The test name and comment claim it verifies "prefix beyond 256-byte window", but the input ' '.repeat(300) + 'Implement the following plan: …' doesn't exercise that path.

300 leading spaces make HEAD="${PROMPT:0:256}" all-whitespace, then the leading-whitespace strip collapses HEAD to empty. The hook exits via [ -z "$HEAD" ] (empty-prompt branch) — output is 0 bytes. So the key assertion expect(out).not.toContain('plan handoff') passes only because out === ''; it never tests the actual 256-byte content window.

The genuinely window-bounded case (256+ non-whitespace bytes before the prefix) is unexercised. I verified: 'a'.repeat(260) + 'Implement the following plan: …' fires the reminder (prefix pushed past window), which is the behavior this test claims to verify but doesn't.

Fix: Split into two precise tests:

1. Whitespace-only head → assert expect(out).toBe('') (this is what AC-F12 actually tests)
2. New test with non-whitespace padding past byte 256 → assert the reminder template fires, not HANDOFF_TEMPLATE (this is the real window-bound verification)

[dean0x]

Additional Inline Comments Posted

Five high-priority inline comments have been posted on this PR covering the most critical blocking and should-fix findings:

1. ambient.ts:236 — Status observability gap for orchestrator-only state (MEDIUM, 80%)
2. ambient.ts:130 — Duplicated hook-registration block needs extraction (MEDIUM, 80%)
3. ambient.ts:227 — Unguarded JSON.parse crashes on corrupt settings (MEDIUM, 80%)
4. KNOWLEDGE.md:33 — Dispatch table contradicts code for empty prompts (MEDIUM, 88%)
5. shell-hooks.test.ts:637 — AC-F12 test mislabeled, under-asserts (MEDIUM, 90%)

Remaining Findings (≥80% confidence, not yet posted as inline)

• git-marker test coverage (MEDIUM, 88%) — No direct test for subprocess-purity invariant; add suite
• addAmbientHook asymmetry (LOW, 80%) — Doesn't sweep stale session-start-classification hook on enable/repair
• ambient.ts:113 inline shadow (LOW, 80%) — Local hasOrchestratorHook shadows module-level function
• shell-hooks.test.ts:947 (LOW, 80%) — 4096-byte charter boundary (exactly 4096) untested; add boundary case

Summary Assessment

Status: APPROVED_WITH_CONDITIONS

Blockers to address before merge:

• README.md lines 19 & 44 (CRITICAL, 97%) — stale keyword/structured-plan prose
• architecture.ts status observability (MEDIUM, 80%) — orchestrator-only state reporting
• KNOWLEDGE.md dispatch table (MEDIUM, 88%) — empty-prompt contradiction

Should-fix items (safe to defer post-merge):

• Duplicated hook registration → extract helper
• Unguarded JSON.parse → parse upfront with try/catch
• AC-F12 test split → verify actual 256-byte window
• git-marker test coverage → add direct tests
• Minor TypeScript/Regression/Testing items

All findings detailed in the corresponding inline comments and initial summary comment.

---

Review completed by Claude Code devflow:comment-pr