1
0
Эх сурвалжийг харах

fix review findings: close wrapped-expression, alias, and binding-pattern gaps

Codex round-2 review found three adjacent fail-open shapes:

- Wrapped function expressions escaped classification: parentheses,
  as/satisfies casts, and non-null assertions are now peeled before the
  arrow/function test (initializers and default exports), and a
  single-call-signature type literal counts as the surface signature.
  A literal mixing call/construct signatures with anything else is
  refused outright — no single signature to hold the tags against.
- The blanket 'export import X = N.member' skip was unsound (the target
  can be a non-exported namespace member no walk visits): an alias now
  documents itself.
- The heritage extra-parameter duty missed binding-pattern extras,
  which no base declaration can name: they now trigger the standard
  binding-pattern violation.

Five new negative-path tests pin the closed shapes; module doc and RFC
updated.
Tianyi Cui 2 сар өмнө
parent
commit
eaac3b58a4

+ 3 - 3
docs/rfc/implemented/process/2026-07-06-export-surface-jsdoc-gate.md

@@ -13,12 +13,12 @@ A new gate, `scripts/verify-export-jsdoc.ts` (`pnpm run verify-export-jsdoc`, wi
 The contract by declaration kind:
 
 - Every exported name needs JSDoc with non-empty description prose.
-- Function-like exports (function declarations; consts with function initializers or an INLINE function-type annotation; non-identifier function default exports) follow the full function contract. A const whose declarator is annotated with a NAMED type (`export const f: Handler = …`) defers the signature contract to that type's own declaration and `@returns` stays optional; an inline `(x: T) => U` annotation is the surface signature itself and gets the full contract.
+- Function-like exports (function declarations; consts with function initializers or an INLINE callable annotation; non-identifier function default exports) follow the full function contract, with wrapper expressions (parentheses, `as`/`satisfies` casts, non-null assertions) peeled before classifying. A const whose declarator is annotated with a NAMED type (`export const f: Handler = …`) defers the signature contract to that type's own declaration and `@returns` stays optional; an inline `(x: T) => U` annotation or single-call-signature literal is the surface signature itself and gets the full contract, and a literal mixing call/construct signatures with anything else is refused outright (no single signature to hold the tags against — extract a named type).
 - Exported classes need class-level prose; public methods (statics included — reachable on the exported name) follow the function contract; public properties and accessors need prose (a get/set pair is covered by the getter). Overload implementations are exempt — the signatures carry the docs.
 - Exported interfaces, type aliases, and enums need prose on the declaration; member-level enforcement is deliberately deferred (the highest-value member surface — seam service classes — is already under the cordis gate).
 - Exported namespaces recurse (inside an ambient `declare` namespace every member exports implicitly); the namespace itself needs prose only when it does not merge with a documented same-name declaration (the Config-namespace idiom documents the plugin once).
-- `declare module` / `declare global` bodies, `export … from` re-export statements, and `export import X = N.member` aliases are skipped: an augmentation is not an export of the package, and a re-exported or aliased definition is checked where it is defined.
-- Everything else fails CLOSED: `export =` is refused outright, and an exported statement kind the dispatch does not recognize is itself a violation — no export form can pass unchecked by omission.
+- `declare module` / `declare global` bodies and `export … from` re-export statements are skipped: an augmentation is not an export of the package, and a re-exported definition is checked where it is defined. An `export import X = N.member` alias documents ITSELF — its target may be a non-exported namespace member no walk visits, so the "definition owns the doc" rationale does not hold for it.
+- Everything else fails CLOSED: `export =` is refused outright, parameters the base never names keep their `@param` duty even as binding patterns, and an exported statement kind the dispatch does not recognize is itself a violation — no export form can pass unchecked by omission.
 
 Three exemption families keep the gate from demanding boilerplate, in the spirit of the cordis gate's `this`/`next` exemptions (documenting an exempt name anyway is allowed; only absence goes unchecked):
 

+ 52 - 1
packages/core/agent/tests/verify-export-jsdoc.spec.ts

@@ -329,12 +329,45 @@ describe('verify-export-jsdoc fail-closed forms (review round 1)', () => {
     ])
   })
 
-  it('skips an export-import alias (the aliased definition owns the doc)', () => {
+  it('requires an export-import alias to document itself (its target may be unwalked)', () => {
     expect(collectExportJsdocViolations(make(
       '/** Holder. */\nexport namespace N {\n  /** The value. */\n  export const x = 1\n}\nexport import y = N.x\n',
+    ))).toEqual([expect.stringMatching(/exported alias 'y' .* has no JSDoc\./)])
+    expect(collectExportJsdocViolations(make(
+      'namespace N {\n  export const x = 1\n}\n/** Alias surfacing the internal counter. */\nexport import y = N.x\n',
     ))).toEqual([])
   })
 
+  it('classifies wrapped function initializers and default exports (parens, satisfies)', () => {
+    expect(collectExportJsdocViolations(make(
+      'type Fn = (x: number) => number\n/** Wrapped. */\nexport const f = (((x: number): number => x)) satisfies Fn\n',
+    ))).toEqual([
+      expect.stringMatching(/exported const 'f' .* is missing @param x\./),
+      expect.stringMatching(/exported const 'f' .* is missing @returns \(return type: number\)\./),
+    ])
+    expect(collectExportJsdocViolations(make(
+      'type Fn = (x: number) => number\n/** Wrapped. */\nexport default (((x: number): number => x * 2) satisfies Fn)\n',
+    ))).toEqual([
+      expect.stringMatching(/default export .* is missing @param x\./),
+      expect.stringMatching(/default export .* is missing @returns \(return type: number\)\./),
+    ])
+  })
+
+  it('treats a single-call-signature type literal as the surface signature', () => {
+    expect(collectExportJsdocViolations(make(
+      '/** Maps. */\nexport declare const f: { (x: number): number }\n',
+    ))).toEqual([
+      expect.stringMatching(/exported const 'f' .* is missing @param x\./),
+      expect.stringMatching(/exported const 'f' .* is missing @returns \(return type: number\)\./),
+    ])
+  })
+
+  it('refuses a hybrid callable type literal instead of narrowing the check', () => {
+    expect(collectExportJsdocViolations(make(
+      '/** Hybrid. */\nexport declare const f: { (x: number): number; flush: () => void }\n',
+    ))).toEqual([expect.stringMatching(/exported const 'f'.*callable type literal is not gate-classifiable; extract a named type/)])
+  })
+
   it('refuses an export-equals assignment instead of failing open', () => {
     expect(collectExportJsdocViolations(make(
       'const x = 1\nexport = x\n',
@@ -393,4 +426,22 @@ export class Impl extends Base {
 }
 `))).toEqual([])
   })
+
+  it('flags a binding-pattern parameter an override adds beyond the base', () => {
+    expect(collectExportJsdocViolations(make(`
+/** Seam. */
+export abstract class Base {
+  /**
+   * Do it.
+   * @param x - input.
+   * @returns output.
+   */
+  abstract run(x: number): number
+}
+/** Impl. */
+export class Impl extends Base {
+  override run(x: number, { verbose }: { verbose?: boolean } = {}): number { return verbose ? x : -x }
+}
+`))).toEqual([expect.stringMatching(/exported class method 'Impl.run' .* is a binding pattern/)])
+  })
 })

+ 79 - 25
scripts/verify-export-jsdoc.ts

@@ -14,16 +14,20 @@
  * - Every exported name needs JSDoc with non-empty description prose (prose
  *   ends at the first block tag, standard JSDoc semantics).
  * - A function-like export (function declaration, a const with a function
- *   initializer or an INLINE function-type annotation, or a non-identifier
+ *   initializer or an INLINE callable annotation, or a non-identifier
  *   function default export) additionally needs a non-empty `@param` per
  *   parameter (`this` receiver annotations exempt; a stale `@param` errors)
  *   and a non-empty `@returns` unless the return type is `void` /
- *   `Promise<void>`. The walk classifies returns syntactically, so the return
- *   type must be ANNOTATED — except a const whose declarator is annotated
- *   with a NAMED type (e.g. `export const f: Handler = …`), where that type's
- *   own declaration owns the signature contract and `@returns` stays
- *   optional; an inline `(x: T) => U` annotation is the surface signature
- *   itself and gets the full contract.
+ *   `Promise<void>`. Wrapper expressions (parentheses, `as` / `satisfies`
+ *   casts, non-null assertions) are peeled before classifying. The walk
+ *   classifies returns syntactically, so the return type must be ANNOTATED —
+ *   except a const whose declarator is annotated with a NAMED type (e.g.
+ *   `export const f: Handler = …`), where that type's own declaration owns
+ *   the signature contract and `@returns` stays optional; an inline
+ *   `(x: T) => U` annotation or single-call-signature literal is the surface
+ *   signature itself and gets the full contract, and a literal mixing
+ *   call/construct signatures with anything else is refused (extract a named
+ *   type).
  * - An exported class needs class-level JSDoc; its public methods (static
  *   included — they are reachable on the exported name) follow the function
  *   contract, and public properties and accessors need description prose (on
@@ -56,10 +60,12 @@
  * - Overload groups: each overload signature carries its own docs; the
  *   implementation signature is exempt (callers never see it).
  * - Skipped: `declare module` / `declare global` augmentation bodies (the
- *   cordis gate's turf; an augmentation is not an export of the package),
- *   re-export statements with a module specifier (`export … from`) and
- *   `export import X = N.member` aliases — the defining module is walked on
- *   its own, and external definitions are not ours to document.
+ *   cordis gate's turf; an augmentation is not an export of the package) and
+ *   re-export statements with a module specifier (`export … from`) — the
+ *   defining module is walked on its own, and external definitions are not
+ *   ours to document. An `export import X = N.member` alias documents
+ *   ITSELF (its target may be a non-exported namespace member no walk
+ *   visits, so a skip would fail open).
  * - Everything else fails CLOSED: `export =` is refused outright, and an
  *   exported statement kind the dispatch does not recognize is itself a
  *   violation, so no export form can pass unchecked by omission.
@@ -115,6 +121,43 @@ function thisReceiver(p: ts.ParameterDeclaration): boolean {
   return ts.isIdentifier(p.name) && p.name.text === 'this'
 }
 
+/**
+ * Peel wrapper expressions that carry no surface of their own — parentheses,
+ * `as` / `satisfies` / angle-bracket casts, non-null assertions — so a
+ * wrapped function expression is still classified as function-like.
+ * @param e - the expression to unwrap.
+ * @returns the innermost non-wrapper expression.
+ */
+function unwrapExpression(e: ts.Expression): ts.Expression {
+  let inner = e
+  while (
+    ts.isParenthesizedExpression(inner) || ts.isAsExpression(inner) || ts.isSatisfiesExpression(inner)
+    || ts.isNonNullExpression(inner) || ts.isTypeAssertionExpression(inner)
+  ) inner = inner.expression
+  return inner
+}
+
+/**
+ * Classify a declarator's type annotation for the function contract: an
+ * inline function type or a type literal that is EXACTLY one call signature
+ * is the surface signature itself; a literal mixing call/construct
+ * signatures with anything else cannot be classified syntactically and is
+ * refused (fail closed — extract a named type); everything else is a plain
+ * value shape.
+ * @param type - the declarator's type annotation.
+ * @returns the signature to check, 'refuse' for an unclassifiable callable literal, or null for a non-callable shape.
+ */
+function callableAnnotation(type: ts.TypeNode): ts.SignatureDeclarationBase | 'refuse' | null {
+  if (ts.isFunctionTypeNode(type)) return type
+  if (!ts.isTypeLiteralNode(type)) return null
+  const signatures = type.members.filter(m => ts.isCallSignatureDeclaration(m) || ts.isConstructSignatureDeclaration(m))
+  if (signatures.length === 0) return null
+  if (signatures.length === 1 && type.members.length === 1 && signatures[0] !== undefined && ts.isCallSignatureDeclaration(signatures[0])) {
+    return signatures[0]
+  }
+  return 'refuse'
+}
+
 /**
  * The heritage-member exemption for one class member. When the member's name
  * is declared by an `extends`/`implements` heritage type, the seam declaration
@@ -242,11 +285,12 @@ function checkClass(cls: ts.ClassDeclaration, name: string, w: Walk): void {
       const where = `exported class method '${name}.${mname}' (${pointer(w.rel, w.sf, m)})`
       if (exemption !== null) {
         // The heritage declaration owns prose and @returns; parameters the
-        // base never names are new surface and keep their @param duty.
+        // base never names — including binding patterns, which no base
+        // declaration can name — are new surface and keep their @param duty.
         const base = exemption.baseParams
         const inBase = (p: ts.ParameterDeclaration): boolean =>
           base !== null && ts.isIdentifier(p.name) && base.has(p.name.text.replace(/^_+/, ''))
-        if (base !== null && m.parameters.some(p => ts.isIdentifier(p.name) && p.name.text !== 'this' && !inBase(p))) {
+        if (base !== null && m.parameters.some(p => !thisReceiver(p) && !inBase(p))) {
           const { params } = parseTags(rawJsDoc(w.text, m))
           checkParams(where, 'export', m.parameters, params, w.sf,
             p => thisReceiver(p) || inBase(p), w.violations)
@@ -316,13 +360,19 @@ function checkDecl(
       const name = ts.isIdentifier(d.name) ? d.name.text : d.name.getText(w.sf)
       if (prefix === '' && PROTOCOL_EXPORTS.has(name)) continue // cordis plugin-protocol slot
       const where = `exported const '${prefix}${name}'${at(d)}`
-      const init = d.initializer
-      if (d.type !== undefined && ts.isFunctionTypeNode(d.type)) {
-        // An INLINE function-type annotation is the surface signature itself:
-        // its parameters and result need docs right here. (A NAMED reference
+      const annotation = d.type !== undefined ? callableAnnotation(d.type) : null
+      const init = d.initializer !== undefined ? unwrapExpression(d.initializer) : undefined
+      if (annotation === 'refuse') {
+        // A literal mixing call/construct signatures with other members (or
+        // overloading them) has no single signature the walk can hold the
+        // tags against — fail closed rather than silently narrow the check.
+        w.violations.push(`${where}: its callable type literal is not gate-classifiable; extract a named type and document it there.`)
+      } else if (annotation !== null) {
+        // An INLINE callable annotation is the surface signature itself: its
+        // parameters and result need docs right here. (A NAMED reference
         // type carries its docs at the type's own declaration instead.)
-        checkFunctionLike(where, raw, d.type.parameters, d.type.type, false, w)
-      } else if (init && (ts.isArrowFunction(init) || ts.isFunctionExpression(init))) {
+        checkFunctionLike(where, raw, annotation.parameters, annotation.type, false, w)
+      } else if (init !== undefined && (ts.isArrowFunction(init) || ts.isFunctionExpression(init))) {
         // A named declarator type annotation (`const f: Handler = …`) hands
         // the return contract to the named type; the arrow's own annotation is
         // still checked when it is the only signature the reader has.
@@ -354,7 +404,11 @@ function checkDecl(
     return
   }
   if (ts.isImportEqualsDeclaration(stmt)) {
-    return // alias re-export (`export import X = N.member`): the aliased definition owns the doc, like `export … from`
+    // An alias (`export import X = N.member`) is a distinct exported name and
+    // its target may be a non-exported namespace member no walk ever visits,
+    // so a blanket skip would fail open — the alias documents itself.
+    checkDescribed(`exported alias '${prefix}${stmt.name.text}'${at(stmt)}`, rawJsDoc(w.text, stmt), w)
+    return
   }
   // Fail CLOSED: an exported statement kind this dispatch does not recognize
   // must never pass silently — the gate's whole promise is that unchecked
@@ -422,11 +476,11 @@ function checkScope(statements: readonly ts.Statement[], prefix: string, w: Walk
         continue
       }
       const where = `default export (${pointer(w.rel, w.sf, stmt)})`
-      if (ts.isIdentifier(stmt.expression)) {
-        for (const decl of byName.get(stmt.expression.text) ?? []) check(decl)
-      } else if (ts.isArrowFunction(stmt.expression) || ts.isFunctionExpression(stmt.expression)) {
-        const fn = stmt.expression
-        checkFunctionLike(where, rawJsDoc(w.text, stmt), fn.parameters, fn.type, false, w)
+      const expr = unwrapExpression(stmt.expression)
+      if (ts.isIdentifier(expr)) {
+        for (const decl of byName.get(expr.text) ?? []) check(decl)
+      } else if (ts.isArrowFunction(expr) || ts.isFunctionExpression(expr)) {
+        checkFunctionLike(where, rawJsDoc(w.text, stmt), expr.parameters, expr.type, false, w)
       } else {
         checkDescribed(where, rawJsDoc(w.text, stmt), w)
       }