Skip to content

Commit dd7c96d

Browse files
JoviDeCroockclaude
andcommitted
Fix second-round review findings in agents validation and static analysis
- Run validateAgentsConfig() outside the VALIDATE_MANIFEST guard: Vite compiles import.meta.env.DEV to false in production server/edge bundles, which stripped the security validation and let a typo'd policy fail open again. - Mask comments before locating the manifest capabilities block so a block-commented example cannot shadow the live registry in verify. - Resolve the default-exported capability to the module-scope binding (brace-depth 0), not a shadowed inner declaration. - Make template-literal scanning interpolation-aware so a nested template in run() no longer truncates extraction and silently privatizes an exposed capability. - Extend the typegen collision guard to basename collisions between the declaration and capabilities outputs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 9c3a515 commit dd7c96d

6 files changed

Lines changed: 145 additions & 12 deletions

File tree

packages/capabilities/src/static.ts

Lines changed: 63 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,15 @@ function findDefaultExportedCallParen(searchable: string): number {
5454
// `=` inside `=>`.
5555
const decl = new RegExp(
5656
`\\b(?:const|let|var)\\s+${id}\\b[^;]*?=\\s*defineCapability\\s*(?:<[^(]*?>)?\\s*\\(`,
57-
).exec(searchable);
58-
if (decl && decl.index != null) {
59-
return decl.index + decl[0].length - 1;
57+
"g",
58+
);
59+
// The default export refers to the MODULE-scope binding; a shadowed
60+
// declaration inside a function must not win. Prefer the match at brace
61+
// depth 0.
62+
for (const match of searchable.matchAll(decl)) {
63+
if (match.index != null && braceDepthAt(searchable, match.index) === 0) {
64+
return match.index + match[0].length - 1;
65+
}
6066
}
6167
}
6268

@@ -68,6 +74,20 @@ function findDefaultExportedCallParen(searchable: string): number {
6874
return -1;
6975
}
7076

77+
/**
78+
* Brace/paren/bracket nesting depth at `index` in an already comment- and
79+
* string-masked source. Depth 0 means module scope.
80+
*/
81+
function braceDepthAt(searchable: string, index: number): number {
82+
let depth = 0;
83+
for (let cursor = 0; cursor < index; cursor += 1) {
84+
const char = searchable[cursor];
85+
if (char === "{" || char === "(" || char === "[") depth += 1;
86+
else if (char === "}" || char === ")" || char === "]") depth -= 1;
87+
}
88+
return depth;
89+
}
90+
7191
/** Local binding name of a module's default export, or null. */
7292
function defaultExportLocalName(searchable: string): string | null {
7393
const idMatch = /export\s+default\s+([A-Za-z_$][A-Za-z0-9_$]*)\b/.exec(searchable);
@@ -307,6 +327,7 @@ function findQuotedObjectProperty(source: string, key: string): number | null {
307327
/** Index of the closing quote of the string starting at `start`. */
308328
function findStringEnd(source: string, start: number): number {
309329
const quote = source[start];
330+
if (quote === "`") return findTemplateEnd(source, start);
310331
for (let index = start + 1; index < source.length; index += 1) {
311332
const char = source[index];
312333
if (char === "\\") {
@@ -318,6 +339,45 @@ function findStringEnd(source: string, start: number): number {
318339
return -1;
319340
}
320341

342+
/**
343+
* Index of the closing backtick of the template literal starting at `start`.
344+
* Tracks `${ ... }` interpolations (including nested strings and templates
345+
* inside them) so an inner backtick or `}` does not end the template early.
346+
*/
347+
function findTemplateEnd(source: string, start: number): number {
348+
for (let index = start + 1; index < source.length; index += 1) {
349+
const char = source[index];
350+
if (char === "\\") {
351+
index += 1;
352+
continue;
353+
}
354+
if (char === "`") return index;
355+
if (char === "$" && source[index + 1] === "{") {
356+
let depth = 1;
357+
index += 2;
358+
while (index < source.length && depth > 0) {
359+
const inner = source[index];
360+
if (inner === "\\") {
361+
index += 2;
362+
continue;
363+
}
364+
if (inner === '"' || inner === "'" || inner === "`") {
365+
const end = findStringEnd(source, index);
366+
if (end === -1) return -1;
367+
index = end + 1;
368+
continue;
369+
}
370+
if (inner === "{") depth += 1;
371+
else if (inner === "}") depth -= 1;
372+
index += 1;
373+
}
374+
if (depth > 0) return -1;
375+
index -= 1;
376+
}
377+
}
378+
return -1;
379+
}
380+
321381
function findMatchingBrace(source: string, start: number, open: string, close: string): number {
322382
let depth = 0;
323383
for (let index = start; index < source.length; index += 1) {

packages/capabilities/test/static.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,39 @@ describe("capability static extraction", () => {
7474
expect(extractDefineCapabilityArgs(source)).toContain('title: "Only call"');
7575
});
7676

77+
it("resolves the module-scope binding, not a shadowed inner declaration", () => {
78+
const source = `
79+
function make() {
80+
const cap = defineCapability({ title: "Inner helper", run() {} });
81+
return cap;
82+
}
83+
const cap = defineCapability({ title: "Module scope", run() {} });
84+
export default cap;
85+
`;
86+
87+
const args = extractDefineCapabilityArgs(source);
88+
expect(args).toContain('title: "Module scope"');
89+
expect(args).not.toContain('title: "Inner helper"');
90+
});
91+
92+
it("does not truncate at a nested template literal in run()", () => {
93+
const source = `
94+
export default defineCapability({
95+
title: "Templates",
96+
run({ input }) {
97+
const inner = \`prefix \${\`nested \${input.name}\`} suffix\`;
98+
return { message: inner };
99+
},
100+
effect: "read",
101+
expose: { http: true },
102+
});
103+
`;
104+
105+
const args = extractDefineCapabilityArgs(source);
106+
expect(args).toContain('effect: "read"');
107+
expect(args).toContain("expose:");
108+
});
109+
77110
it("does not truncate at a brace inside a regex literal", () => {
78111
const source = `
79112
export default defineCapability({

packages/cli/src/commands/typegen.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,11 +132,15 @@ export async function runTypegen(options: TypegenOptions): Promise<TypegenResult
132132
"so the generated capability types would never apply. Pick a different --capabilities-out.",
133133
);
134134
}
135-
if (capabilitiesPath === declarationPath) {
135+
if (
136+
capabilitiesPath === declarationPath ||
137+
outputsCollide(declarationPath, capabilitiesPath) ||
138+
outputsCollide(capabilitiesPath, declarationPath)
139+
) {
136140
throw new Error(
137-
`Capabilities output ${options.capabilitiesOut} is the same file as the declaration output ` +
138-
`${options.declarationOut}; the two generated files would overwrite each other. ` +
139-
"Pick a different --capabilities-out.",
141+
`Capabilities output ${options.capabilitiesOut} collides with the declaration output ` +
142+
`${options.declarationOut} — identical paths overwrite each other, and TypeScript drops ` +
143+
"a .d.ts input that sits next to a same-named .ts file. Pick a different --capabilities-out.",
140144
);
141145
}
142146
const outputs = [

packages/cli/src/manifest.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -106,11 +106,15 @@ export function extractRegistryEntries(
106106
source: string,
107107
key: string,
108108
): { name: string; path: string }[] {
109-
const block = findNamedBlock(source, key, "{", "}");
110-
if (!block) return [];
111-
// Mask comments before matching so commented-out registrations are not
109+
// Mask comments BEFORE locating the block so a block-commented example
110+
// (`/* capabilities: { ... } */`) cannot be selected instead of the live
111+
// registry, and commented-out registrations inside the live block are not
112112
// treated as registered (mirrors the analyzer in @pracht/capabilities).
113-
const inner = maskComments(source.slice(block.openIndex + 1, block.closeIndex));
113+
// Masking preserves offsets, so slicing the masked source is safe.
114+
const masked = maskComments(source);
115+
const block = findNamedBlock(masked, key, "{", "}");
116+
if (!block) return [];
117+
const inner = masked.slice(block.openIndex + 1, block.closeIndex);
114118
const entries: { name: string; path: string }[] = [];
115119
// Keys may be bare identifiers (shells, middleware) or quoted strings —
116120
// capability names like "notes.search" require quoting.

packages/cli/test/verification-capabilities.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,32 @@ describe("collectCapabilityChecks", () => {
118118
);
119119
});
120120

121+
it("reads the live registry when a block-commented example precedes it", () => {
122+
const checks: Check[] = [];
123+
collectCapabilityChecks(
124+
createProject({
125+
capability: capabilitySource(COMPLETE_FIELDS),
126+
middlewareBlock: [
127+
" /*",
128+
" capabilities: {",
129+
' "notes.fake": () => import("./capabilities/notes-fake.ts"),',
130+
" },",
131+
" */",
132+
].join("\n"),
133+
}),
134+
checks,
135+
);
136+
137+
// The commented example must not shadow the live registry: the real
138+
// capability is analyzed (its OK check appears) and the fake one is not.
139+
expect(checks.map((check) => check.message)).toContainEqual(
140+
expect.stringContaining("declares a complete exposed contract"),
141+
);
142+
expect(checks.map((check) => check.message)).not.toContainEqual(
143+
expect.stringContaining("notes.fake"),
144+
);
145+
});
146+
121147
it("does not count a commented-out registration", () => {
122148
const checks: Check[] = [];
123149
collectCapabilityChecks(

packages/framework/src/app.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,9 +181,15 @@ export function resolveApp(app: PrachtApp): ResolvedPrachtApp {
181181
);
182182
}
183183
}
184-
validateAgentsConfig(app.agents);
185184
}
186185

186+
// Security validation, deliberately OUTSIDE the VALIDATE_MANIFEST guard:
187+
// Vite compiles `import.meta.env.DEV` to `false` in production server/edge
188+
// bundles, which would strip a dev-only check and let a typo'd policy
189+
// (e.g. "requre") silently fail open at dispatch. This runs once per
190+
// manifest resolution, so the cost is negligible.
191+
validateAgentsConfig(app.agents);
192+
187193
for (const node of app.routes) {
188194
flattenRouteNode(app, node, inherited, routes);
189195
}

0 commit comments

Comments
 (0)