address second round feedbac

Author: rezrah

packages/mcp/src/brand/resolve-brand-project/resolve-brand-project.test.ts

@@ -1,4 +1,4 @@
-import {mkdirSync, mkdtempSync, rmSync, writeFileSync} from 'node:fs'
+import {mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync} from 'node:fs'
 import {tmpdir} from 'node:os'
 import {join} from 'node:path'
 
@@ -7,7 +7,7 @@ import {resolveBrandProject} from './resolve-brand-project.js'
 describe('resolveBrandProject', () => {
   const directories: string[] = []
   const temporaryWorkspace = () => {
-    const directory = mkdtempSync(join(tmpdir(), 'primer-brand-project-'))
+    const directory = realpathSync(mkdtempSync(join(tmpdir(), 'primer-brand-project-')))
     directories.push(directory)
     return directory
   }
@@ -64,6 +64,19 @@ describe('resolveBrandProject', () => {
     expect(resolveBrandProject(workspace, join(workspace, 'apps', 'site')).reason).toBe('invalid-brand-project')
   })
 
+  it('rejects a requested package symlink that resolves outside the workspace', () => {
+    const workspace = temporaryWorkspace()
+    const externalPackage = temporaryWorkspace()
+    writePackage(externalPackage, {'@primer/react-brand': '^0.73.0'})
+    mkdirSync(join(workspace, 'apps'), {recursive: true})
+    symlinkSync(externalPackage, join(workspace, 'apps', 'site'), 'dir')
+
+    expect(resolveBrandProject(workspace, 'apps/site')).toMatchObject({
+      projectDir: null,
+      reason: 'invalid-brand-project',
+    })
+  })
+
   it('reports when no declaring package or workspace root is available', () => {
     const workspace = temporaryWorkspace()
     writePackage(workspace, {react: '^19.0.0'})

packages/mcp/src/brand/resolve-brand-project/resolve-brand-project.ts

@@ -1,5 +1,5 @@
-import {readdirSync, readFileSync} from 'node:fs'
-import {isAbsolute, join, relative, resolve} from 'node:path'
+import {readdirSync, readFileSync, realpathSync} from 'node:fs'
+import {isAbsolute, join, relative, resolve, sep} from 'node:path'
 
 export type BrandProjectResolution = {
   projectDir: string | null
@@ -37,15 +37,30 @@ export function resolveBrandProject(workspaceDir: string | null, requestedProjec
 
   if (!workspaceDir) return {projectDir: null, candidates: [], reason: 'no-workspace-root'}
 
+  let canonicalWorkspaceDir: string
+  try {
+    canonicalWorkspaceDir = realpathSync(workspaceDir)
+  } catch {
+    return {projectDir: null, candidates: [], reason: 'no-workspace-root'}
+  }
+
+  const isWithinWorkspace = (candidate: string): boolean => {
+    const relativeCandidate = relative(canonicalWorkspaceDir, candidate)
+    return relativeCandidate !== '..' && !relativeCandidate.startsWith(`..${sep}`) && !isAbsolute(relativeCandidate)
+  }
+
   if (requestedProjectDir) {
     if (isAbsolute(requestedProjectDir)) {
       return {projectDir: null, candidates: [], reason: 'invalid-brand-project'}
     }
-    const candidate = resolve(workspaceDir, requestedProjectDir)
-    const relativeCandidate = relative(workspaceDir, candidate)
-    const isOutsideWorkspace = relativeCandidate.startsWith('..') || isAbsolute(relativeCandidate)
-    return !isOutsideWorkspace && declaresBrand(candidate)
-      ? {projectDir: candidate, candidates: [candidate]}
+    let canonicalCandidate: string
+    try {
+      canonicalCandidate = realpathSync(resolve(canonicalWorkspaceDir, requestedProjectDir))
+    } catch {
+      return {projectDir: null, candidates: [], reason: 'invalid-brand-project'}
+    }
+    return isWithinWorkspace(canonicalCandidate) && declaresBrand(canonicalCandidate)
+      ? {projectDir: canonicalCandidate, candidates: [canonicalCandidate]}
       : {projectDir: null, candidates: [], reason: 'invalid-brand-project'}
   }
 
@@ -64,7 +79,7 @@ export function resolveBrandProject(workspaceDir: string | null, requestedProjec
       visit(join(directory, entry.name))
     }
   }
-  visit(workspaceDir)
+  visit(canonicalWorkspaceDir)
   candidates.sort()
 
   if (candidates.length === 1) return {projectDir: candidates[0] ?? null, candidates}

packages/mcp/src/brand/update-agents-md/update-agents-md.test.ts

@@ -1,4 +1,4 @@
-import {mkdtempSync, readFileSync, rmSync, writeFileSync} from 'node:fs'
+import {existsSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync} from 'node:fs'
 import {tmpdir} from 'node:os'
 import {join} from 'node:path'
 
@@ -77,6 +77,27 @@ describe('updateAgentsMd', () => {
     expect(updateAgentsMd(wrongCaseDir)).toMatchObject({action: 'skipped', reason: 'incorrect-filename-case'})
   })
 
+  it('does not read or write AGENTS.md through symbolic links', () => {
+    const projectDir = temporaryProject()
+    const externalDir = temporaryProject()
+    const externalFile = join(externalDir, 'instructions.md')
+    writeFileSync(externalFile, '# External instructions\n')
+    symlinkSync(externalFile, join(projectDir, 'AGENTS.md'))
+
+    expect(updateAgentsMd(projectDir)).toMatchObject({action: 'skipped', reason: 'unsafe-agents-path'})
+    expect(readFileSync(externalFile, 'utf8')).toBe('# External instructions\n')
+  })
+
+  it('does not create an external file through a dangling AGENTS.md symlink', () => {
+    const projectDir = temporaryProject()
+    const externalDir = temporaryProject()
+    const externalFile = join(externalDir, 'future-instructions.md')
+    symlinkSync(externalFile, join(projectDir, 'AGENTS.md'))
+
+    expect(updateAgentsMd(projectDir)).toMatchObject({action: 'skipped', reason: 'unsafe-agents-path'})
+    expect(existsSync(externalFile)).toBe(false)
+  })
+
   it('skips when no project root is available', () => {
     expect(updateAgentsMd(null)).toEqual({action: 'skipped', path: null, reason: 'no-project-root'})
   })

packages/mcp/src/brand/update-agents-md/update-agents-md.ts

@@ -1,10 +1,10 @@
-import {existsSync, readFileSync, readdirSync, writeFileSync} from 'node:fs'
-import {join} from 'node:path'
+import {lstatSync, readFileSync, readdirSync, realpathSync, writeFileSync} from 'node:fs'
+import {isAbsolute, join, relative, sep} from 'node:path'
 
 export type AgentsMdUpdateResult = {
   action: 'created' | 'updated' | 'unchanged' | 'skipped'
   path: string | null
-  reason?: 'no-project-root' | 'incorrect-filename-case' | 'malformed-managed-block'
+  reason?: 'no-project-root' | 'incorrect-filename-case' | 'malformed-managed-block' | 'unsafe-agents-path'
 }
 
 /** Create or update the managed Primer Brand instructions in a project's root AGENTS.md. */
@@ -24,17 +24,42 @@ ${endMarker}`
 
   if (!projectDir) return {action: 'skipped', path: null, reason: 'no-project-root'}
 
-  const agentMdCustomSetupPath = join(projectDir, filename)
-  const caseInsensitiveMatch = readdirSync(projectDir).find(entry => entry.toLowerCase() === filename.toLowerCase())
+  let canonicalProjectDir: string
+  try {
+    canonicalProjectDir = realpathSync(projectDir)
+  } catch {
+    return {action: 'skipped', path: null, reason: 'no-project-root'}
+  }
+
+  const agentMdCustomSetupPath = join(canonicalProjectDir, filename)
+  const caseInsensitiveMatch = readdirSync(canonicalProjectDir).find(
+    entry => entry.toLowerCase() === filename.toLowerCase(),
+  )
   if (caseInsensitiveMatch && caseInsensitiveMatch !== filename) {
     return {action: 'skipped', path: agentMdCustomSetupPath, reason: 'incorrect-filename-case'}
   }
 
-  if (!existsSync(agentMdCustomSetupPath)) {
+  const fileStats = lstatSync(agentMdCustomSetupPath, {throwIfNoEntry: false})
+  if (fileStats?.isSymbolicLink()) {
+    return {action: 'skipped', path: agentMdCustomSetupPath, reason: 'unsafe-agents-path'}
+  }
+
+  if (!fileStats) {
     writeFileSync(agentMdCustomSetupPath, `${block}\n`, 'utf8')
     return {action: 'created', path: agentMdCustomSetupPath}
   }
 
+  let canonicalAgentsPath: string
+  try {
+    canonicalAgentsPath = realpathSync(agentMdCustomSetupPath)
+  } catch {
+    return {action: 'skipped', path: agentMdCustomSetupPath, reason: 'unsafe-agents-path'}
+  }
+  const relativeAgentsPath = relative(canonicalProjectDir, canonicalAgentsPath)
+  if (relativeAgentsPath === '..' || relativeAgentsPath.startsWith(`..${sep}`) || isAbsolute(relativeAgentsPath)) {
+    return {action: 'skipped', path: agentMdCustomSetupPath, reason: 'unsafe-agents-path'}
+  }
+
   const existing = readFileSync(agentMdCustomSetupPath, 'utf8')
   const eol = existing.includes('\r\n') ? '\r\n' : '\n'
   const normalizedBlock = block.replaceAll('\n', eol)

packages/mcp/src/review/rules.test.ts

@@ -232,6 +232,12 @@ describe('primer_brand_review rules', () => {
     expect(ruleIds(review('<Heading size="4">Heading</Heading>'))).not.toContain('component-text-default-size')
   })
 
+  it('does not apply component-owned text guidance to application components', () => {
+    expect(ruleIds(review('<PricingCard.Heading size="small">Pro</PricingCard.Heading>'))).not.toContain(
+      'component-text-default-size',
+    )
+  })
+
   it('reports which approved brand components were imported', () => {
     const used = brandComponentsUsed("import {Hero, CTABanner} from '@primer/react-brand'", makeCatalog())
     expect(used.map(component => component.name).sort()).toEqual(['CTABanner', 'Hero'])

packages/mcp/src/review/rules.ts

@@ -441,15 +441,18 @@ const headingExplicitSize: Rule = {
 /** Compound components provide context-appropriate text sizes; explicit overrides should be exceptional. */
 const componentTextDefaultSize: Rule = {
   id: 'component-text-default-size',
-  run(code) {
+  run(code, catalog) {
+    const knownSubcomponents = new Set(catalog.components.flatMap(component => component.subcomponents))
     const findings: Finding[] = []
     for (const match of code.matchAll(
       /<([A-Z][A-Za-z0-9]*)\.(Heading|Subheading|Description|Label|Eyebrow|Text)\b[^>]*(?:^|\s)size\s*=/g,
     )) {
+      const qualifiedName = `${match[1]}.${match[2]}`
+      if (!knownSubcomponents.has(qualifiedName)) continue
       findings.push({
         severity: 'warning',
         rule: this.id,
-        message: `\`${match[1]}.${match[2]}\` already has the right default \`size\` for its component. Remove \`size\` unless the brief requires an override.`,
+        message: `\`${qualifiedName}\` already has the right default \`size\` for its component. Remove \`size\` unless the brief requires an override.`,
         evidence: evidence(match[0]),
       })
     }

packages/mcp/src/test-utils/catalog.ts

@@ -61,6 +61,13 @@ export function makeCatalog(overrides: Partial<Catalog> = {}): Catalog {
           },
         ],
       },
+      {
+        name: 'Card',
+        module: '@primer/react-brand',
+        subcomponents: ['Card.Heading', 'Card.Description'],
+        props: [],
+        examples: [],
+      },
       {name: 'Pillar', module: '@primer/react-brand', subcomponents: [], props: [], examples: []},
       {name: 'SectionIntro', module: '@primer/react-brand', subcomponents: [], props: [], examples: []},
       {name: 'River', module: '@primer/react-brand', subcomponents: [], props: [], examples: []},

packages/mcp/src/tools/primer-brand-setup/primer-brand-setup.ts

@@ -168,6 +168,9 @@ function build(id: FrameworkId, ctx: ToolContext, agentsMd: SetupAgentsMdResult)
         if (agentsMd.reason === 'malformed-managed-block') {
           return 'Skipped `AGENTS.md`: the Primer Brand managed markers are malformed or duplicated; repair or remove that block, then rerun setup.'
         }
+        if (agentsMd.reason === 'unsafe-agents-path') {
+          return 'Skipped `AGENTS.md`: the existing path is a symbolic link or resolves outside the selected package. Replace it with a regular file inside the package, then rerun setup.'
+        }
         return 'Skipped `AGENTS.md`: no project root was detected.'
     }
   })()