#1252 Keep a skill's own sections out of injected guidance

packages/agents/content/__tests__/injection-point-placement.unit.test.ts

@@ -0,0 +1,192 @@
+import { readdir, readFile } from 'node:fs/promises';
+import path from 'node:path';
+
+import { describe, expect, it } from 'vitest';
+
+import { expandIncludes } from '../../src/lib/directive-expander.ts';
+import { isUnderTestDirectory } from '../../src/lib/fs-helpers.ts';
+
+// An include and a guidance hook both splice content that brings its own headings, so a host heading deeper than the
+// injected content's shallowest one renders as a subsection of the injection rather than of the host body. For a hook
+// the parent is worse than misattributed: it is whichever rulebook the local binding supplied.
+//
+// The scan reads source rather than rendered output. Once includes expand, an inlined partial is byte-identical to the
+// text around it, and no pass over the result can tell a host heading from an injected one.
+const CONTENT_ROOT = new URL('../', import.meta.url).pathname;
+
+// Mirrors of the production grammars in `directive-expander.ts` and `guidance-hooks.ts`: each occupies a full line and
+// tolerates surrounding whitespace, and self-close is tested before open so a path ending in `/` reads as a self-close.
+const CLOSE_INCLUDE_REGEX = /^[ \t]*<!--[ \t]*\/include[ \t]*-->[ \t]*$/;
+const FENCE_REGEX = /^\s*(`{3,}|~{3,})/;
+const HEADING_REGEX = /^(#{1,6})\s/;
+const HOOK_DIRECTIVE_REGEX = /^[ \t]*<!--[ \t]*guidance-hook:[ \t]*.*?[ \t]*-->[ \t]*$/;
+const OPEN_INCLUDE_REGEX = /^[ \t]*<!--[ \t]*include:[ \t]*(\S+)[ \t]*-->[ \t]*$/;
+const SELF_CLOSE_INCLUDE_REGEX = /^[ \t]*<!--[ \t]*include:[ \t]*(\S+?)[ \t]*\/[ \t]*-->[ \t]*$/;
+
+/** Level a bound rulebook's title lands at: `fillGuidanceHooks` demotes every heading in the fill by one. */
+const HOOK_FILL_LEVEL = 2;
+
+/** A heading the host body declares itself, outside any fence or slot region. */
+interface Heading {
+  readonly kind: 'heading';
+  readonly lineNumber: number;
+  readonly level: number;
+  readonly text: string;
+}
+
+/** A directive and the level its injected content starts at, against which a following host heading is judged. */
+interface Injection {
+  readonly kind: 'injection';
+  readonly lineNumber: number;
+  readonly shallowestLevel: number;
+  readonly text: string;
+}
+
+/** One source line that carries structure, paired with the 1-based line it occupies. */
+interface LiveLine {
+  readonly lineNumber: number;
+  readonly text: string;
+}
+
+type Token = Heading | Injection;
+
+const levelByPartial = new Map<string, number | undefined>();
+
+describe('injection-point placement', () => {
+  it('no directive reparents the section following it', async () => {
+    const violations: Array<string> = [];
+    for (const relativePath of await listContentMarkdown()) {
+      violations.push(...(await findViolations(relativePath)));
+    }
+
+    expect(
+      violations,
+      `These directives inject content whose headings adopt the section below them:\n  ${violations.join('\n  ')}\n` +
+        `A host heading following a directive must sit at or above the injected content's shallowest heading level. ` +
+        `Promote the section, or move the directive below it.`,
+    ).toEqual([]);
+  });
+});
+
+// region | Helpers
+
+/**
+ * Reports each directive in one body whose next host heading is deeper than the content it injects, rendered as one
+ * line per violation. Two directives sharing a following heading are both reported: each is independently misplaced.
+ */
+async function findViolations(relativePath: string): Promise<ReadonlyArray<string>> {
+  const violations: Array<string> = [];
+  const open: Array<Injection> = [];
+
+  for (const token of await readStructure(relativePath)) {
+    if (token.kind === 'injection') {
+      open.push(token);
+      continue;
+    }
+    for (const injection of open) {
+      if (token.level > injection.shallowestLevel) {
+        violations.push(
+          `${relativePath}:${injection.lineNumber} ${injection.text} (injects h${injection.shallowestLevel}) ` +
+            `adopts ${relativePath}:${token.lineNumber} ${token.text}`,
+        );
+      }
+    }
+    open.length = 0;
+  }
+
+  return violations;
+}
+
+/** Returns every Markdown file under `content/`, as paths relative to it. */
+async function listContentMarkdown(): Promise<ReadonlyArray<string>> {
+  const entries = await readdir(CONTENT_ROOT, { recursive: true });
+  return entries.filter((entry) => entry.endsWith('.md') && !isUnderTestDirectory(entry)).toSorted();
+}
+
+/**
+ * Yields the lines of a body that carry structure, skipping every line inside a fenced block. A `#` inside a fence is
+ * content, so the fence is tracked rather than each line matched in isolation. A fenced directive is skipped for a
+ * different reason: the expander tracks no fences and still expands it, but the fence turns the headings it injects
+ * into literal text, which adopts nothing.
+ */
+function* readLiveLines(body: string): Generator<LiveLine> {
+  let openFence: string | undefined;
+  for (const [index, text] of body.split('\n').entries()) {
+    const fence = FENCE_REGEX.exec(text)?.[1];
+    if (openFence !== undefined) {
+      if (fence !== undefined && fence[0] === openFence[0] && fence.length >= openFence.length) {
+        openFence = undefined;
+      }
+      continue;
+    }
+    if (fence !== undefined) {
+      openFence = fence;
+      continue;
+    }
+    yield { lineNumber: index + 1, text };
+  }
+}
+
+/**
+ * Returns the shallowest level a partial contributes, or `undefined` when it contributes nothing a following section
+ * could nest under. A hook the partial declares counts at the fill level: hooks resolve after includes expand, so such
+ * a hook fills inside the host and can splice shallower than the partial's own headings. Memoized, since one partial
+ * reaches many consumers.
+ */
+async function readPartialLevel(partialPath: string): Promise<number | undefined> {
+  if (!levelByPartial.has(partialPath)) {
+    const expanded = await expandIncludes(partialPath, CONTENT_ROOT);
+    let shallowest: number | undefined;
+    for (const { text } of readLiveLines(expanded)) {
+      const level = HOOK_DIRECTIVE_REGEX.test(text) ? HOOK_FILL_LEVEL : HEADING_REGEX.exec(text)?.[1]?.length;
+      if (level !== undefined && (shallowest === undefined || level < shallowest)) {
+        shallowest = level;
+      }
+    }
+    levelByPartial.set(partialPath, shallowest);
+  }
+  return levelByPartial.get(partialPath);
+}
+
+/**
+ * Reads one body's headings and injection points in source order. Lines between an open include directive and its
+ * `<!-- /include -->` are slot content bound for the partial's `<!-- children -->`, not structure of this body, and a
+ * partial that places that placeholder inside a fence turns them into example text — so they contribute no token.
+ */
+async function readStructure(relativePath: string): Promise<ReadonlyArray<Token>> {
+  const filePath = path.join(CONTENT_ROOT, relativePath);
+  const body = await readFile(filePath, 'utf8');
+  const tokens: Array<Token> = [];
+  let inSlot = false;
+
+  for (const { lineNumber, text } of readLiveLines(body)) {
+    if (inSlot) {
+      inSlot = !CLOSE_INCLUDE_REGEX.test(text);
+      continue;
+    }
+
+    const partial = SELF_CLOSE_INCLUDE_REGEX.exec(text)?.[1] ?? OPEN_INCLUDE_REGEX.exec(text)?.[1];
+    if (partial !== undefined) {
+      inSlot = !SELF_CLOSE_INCLUDE_REGEX.test(text);
+      const shallowestLevel = await readPartialLevel(path.resolve(path.dirname(filePath), partial));
+      if (shallowestLevel !== undefined) {
+        tokens.push({ kind: 'injection', lineNumber, shallowestLevel, text: text.trim() });
+      }
+      continue;
+    }
+
+    if (HOOK_DIRECTIVE_REGEX.test(text)) {
+      tokens.push({ kind: 'injection', lineNumber, shallowestLevel: HOOK_FILL_LEVEL, text: text.trim() });
+      continue;
+    }
+
+    const level = HEADING_REGEX.exec(text)?.[1]?.length;
+    if (level !== undefined) {
+      tokens.push({ kind: 'heading', lineNumber, level, text: text.trim() });
+    }
+  }
+
+  return tokens;
+}
+
+// endregion | Helpers

packages/agents/content/_partials/README.md

@@ -37,6 +37,14 @@ Self-close is matched before open so that a path with a trailing slash is read c
 
 The `<!-- children -->` placeholder is a partial-side directive. It appears at most once per partial. If a partial has no `<!-- children -->` and the caller provides slot content, the expander throws `slot-without-children`. If a partial has `<!-- children -->` and the caller provides no slot content (bare self-close, or empty open/close pair), the placeholder line is removed and surrounding lines join verbatim.
 
+## Directive placement
+
+A host heading following a directive, deeper than the shallowest heading the injection contributes, renders as a subsection of the injected content rather than of the host body. Place every directive where the next host heading sits at or above that level. The `codeassembly-content-specification` rulebook carries the rule an author follows, under "Injection-point placement"; this section carries the level computation behind it.
+
+The deciding level is what the splice contributes, not a fixed `##`. A partial contributes its headings as authored -- `subagents/_partials/review-writes-scaffold.md` opens at `###`, so the `###` sections following it are its correct siblings -- and `##` for any guidance hook it declares, since hooks resolve after includes expand and so fill inside the host. A hook the host declares contributes `##` on its own, a bound rulebook's title being demoted one level to land there.
+
+Slot content is the caller's own text and contributes nothing here: a heading passed into a partial's `<!-- children -->` is authored in the host beside the section that follows it, so its nesting is already visible where it is written. `content/__tests__/injection-point-placement.unit.test.ts` enforces the rule.
+
 ## Path resolution
 
 Include paths are resolved relative to the directive-bearing file's directory in the source tree. The resolved target must remain inside `packages/agents/content/` (lexical containment is checked, not symlink resolution). Out-of-tree references throw `out-of-tree`.

packages/agents/content/guidance/rulebooks/codeassembly-content-specification.md

@@ -144,3 +144,9 @@ Behavioural rules that govern an agent's output -- the recommendation gradient,
 Treat that restatement as load-bearing redundancy, not duplication. A DRY-driven refactor that strips the skill-local pointers and leaves only the global rule removes the mechanism by which the global rule takes effect. _(Enforced by `action-item-reinforcement.unit.test.ts` and `spec-inlining.unit.test.ts`.)_
 
 Where a step's guidance is a matter of local taste rather than library doctrine -- a user's code-style preferences, a project's own glossary -- neither restatement above fits: a pointer sends the agent away to fetch the rule, and an inlined partial fixes one answer for every consumer at authoring time. Declare a guidance hook instead, `<!-- guidance-hook: <name> -->`, and leave the slot for a `codeassembly.yaml` to bind per project or per machine. An unbound hook contributes nothing to deployed output, so declaring one is safe wherever nothing fills it. A rulebook written for that slot declares `delivery: hook`, which records the route and lets `sync` report a binding and a delivery that disagree. The directive grammar is specified in `content/_partials/README.md` and the binding syntax in `packages/agents/README.md`. _(Convention; not enforced.)_
+
+## Injection-point placement
+
+Injected content brings its own headings: a partial's as authored, and a guidance-hook fill's demoted one level, so a bound rulebook's title lands at `##`. A host heading following a directive that is deeper than the injected content's shallowest heading renders as a subsection of the injection rather than of the host -- and for a hook, under whichever rulebook the local binding supplied, so one body reads differently on two machines.
+
+Place every directive where the next host heading sits at or above that level. Where a section would otherwise nest, promote it or move the directive below it. The level that decides is what the injection contributes, not a fixed `##`: a partial opening at `###` and declaring no hook legitimately takes `###` siblings after it. _(Enforced by `injection-point-placement.unit.test.ts`.)_

packages/agents/content/skills/respond-to-review/SKILL.md

@@ -148,11 +148,11 @@ The disposition conflates two decisions: whether the change belongs (substance),
 
 > T1: ACCEPT. `createApiKey` is exported from the public `api/keys.ts` surface; the codebase's other public-API entry points (`createUser`, `createOrganization`) all validate their inputs at entry. The missing guard violates the established public-API invariant, so the change belongs. Timing is decided separately: The storage refactor's scope is otherwise tight, and adding the guard pulls in test fixtures unrelated to the refactor's purpose. The guard ships in a follow-up, and the follow-up ticket is filed now (per `create-tickets-immediately` guidance), not held as a maybe.
 
-<!-- guidance-hook: implementation-preferences -->
-
 <!-- include: ../../_partials/comment-discipline.md / -->
 
-### Writing code after a review
+<!-- guidance-hook: implementation-preferences -->
+
+## Writing code after a review
 
 Implementing an ACCEPTed finding puts you mid-conversation with the reviewer, and that is when the voice leaks. A comment must not narrate the change, retell the reviewer's concern, or cite a finding or acceptance-criterion ID.
 

packages/agents/content/skills/revise-comments/SKILL.md

@@ -32,7 +32,7 @@ Apply the comment-discipline audit to a target file set, editing comments in pla
 
 <!-- include: ../../_partials/comment-discipline.md / -->
 
-### File-level carve-outs
+## File-level carve-outs
 
 The carve-outs above apply to comments. Two file-level rules govern which lines this skill may touch at all:
 

packages/agents/content/skills/testing-conventions/SKILL.md

@@ -68,7 +68,7 @@ Examples:
 
 <!-- include: ../../_partials/comment-discipline.md / -->
 
-### Test files
+## Comments in test files
 
 Test files are governed the same as source. Test names already communicate intent and assertions communicate the check, so everything outside the test-comment carve-out is over-commenting.