Skip to content

Contributable connector registry: isolate all connectors under connectors/, add pi-web connector - #455

Open
ashwin-pc wants to merge 5 commits into
opensearch-project:mainfrom
ashwin-pc:connector-registry
Open

Contributable connector registry: isolate all connectors under connectors/, add pi-web connector#455
ashwin-pc wants to merge 5 commits into
opensearch-project:mainfrom
ashwin-pc:connector-registry

Conversation

@ashwin-pc

@ashwin-pc ashwin-pc commented Aug 31, 2026

Copy link
Copy Markdown
Member

Makes agent connectors a contributable surface: every connector now lives in an isolated directory under connectors/ with its own README and tests, resolved through a single registry — so well-known connectors can grow over time without touching core.

Structure

  • connectors/<name>/ — one directory per connector: index.ts (implements the shared AgentConnector contract), README.md (target system, config, quirks), colocated tests.
  • connectors/index.ts / connectors/server.ts — name→factory registries; core resolves connectors only through them.
  • connectors/README.md — the contribution guide: interface contract, harvest/settlement expectations, evidence metadata conventions, how to add and test a connector.

Extracted (no behavior change)

AG-UI, REST, OpenAI-compatible, LangGraph, Subprocess, Claude Code, Kiro, Pi CLI, Strands, and Mock move from services/connectors/** into isolated directories. Legacy paths remain as deprecated re-export shims; regression tests prove existing configs and protocol strings resolve unchanged.

New: connectors/pi-web/

First contributed connector under the new model: drives a pi-web session end-to-end — session creation, settlement-based harvesting (polls session status until settled, so post-worker synthesis turns are never truncated), fixture-envelope resolution with integrity verification (#450), and numeric-timestamp handling.

@ashwin-pc
refactor: establish contributable connector registry
aa037c0 
Signed-off-by: ashwin pc <ashwinpc@amazon.com>
@ashwin-pc
test: preserve license headers in connector shims
5a132e2 
Signed-off-by: ashwin pc <ashwinpc@amazon.com>
@github-actions

github-actions Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit dc1c7fc)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 Security concerns

Sensitive information exposure:
The buildAuthEnv and buildAuthHeaders methods in BaseConnector (not shown in diff but referenced) may propagate API keys and AWS credentials into subprocess environments and HTTP headers. If debug logging is enabled, these values could be logged in plaintext. Ensure that debug output redacts sensitive fields like ANTHROPIC_API_KEY, AWS_SECRET_ACCESS_KEY, and Authorization headers.

✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Command Injection Risk

The health check spawns which with user-controlled command as an argument. While shell: false prevents shell metacharacter expansion, a malicious endpoint string could still be passed to which as a literal argument. Although which itself is unlikely to execute arbitrary code, this pattern is fragile. If the health check logic is ever refactored to use a different command or if which behavior varies across platforms, the risk increases. Consider validating that command contains only safe characters (alphanumeric, dash, underscore) before passing it to spawn.

 
async healthCheck(endpoint: string, auth: ConnectorAuth): Promise<boolean> {
  const command = endpoint || this.config.command;
  if (!command) return false;

  return new Promise((resolve) => {
    // shell: false to avoid metacharacter expansion on the command name.
    // `which` itself is invoked from PATH; a malicious endpoint string
    // (e.g. `kiro-cli; rm -rf ~`) would have been evaluated under shell:
    // true, but here it's passed as a literal argv slot to `which` which
    // will simply return non-zero for a name that doesn't exist on PATH.
    const proc = spawn('which', [command], { shell: false });
    proc.on('close', (code) => resolve(code === 0));
    proc.on('error', () => resolve(false));
  });
Path Traversal Risk

The fixture resolution checks that fixtureSource starts with fixturesDir followed by a separator, but this check can be bypassed on case-insensitive filesystems or via symlinks. An attacker who controls fixtureRef could craft a path like ../../../etc/passwd or use symlinks to escape the fixtures directory. The integrity check mitigates data tampering but does not prevent directory traversal. Consider using fs.realpathSync to resolve symlinks and normalize paths before the prefix check, or reject any fixtureRef containing .. segments.

 
if (fixtureRef) {
  const fixturesDir = resolve(config.fixturesDir || join(process.cwd(), "fixtures"));
  const fixtureSource = resolve(fixturesDir, fixtureRef);
  if (!fixtureSource.startsWith(`${fixturesDir}${sep}`)) {
    throw new Error(`Fixture resolves outside fixtures directory: ${fixtureRef}`);
  }
Unvalidated Fixture Integrity

When fixtureResolution is legacy-context, the connector uses a regex-extracted SHA-256 digest from a markdown comment as the integrity check. If the legacy manifest is missing or malformed, fixtureIntegrity is undefined and the integrity check is silently skipped. This allows an attacker who can modify the fixture directory to substitute arbitrary files without detection. The code should either require integrity for all fixtures or explicitly log a warning when integrity cannot be verified.

 
  fixtureRef = legacyFixture.value;
  const digest = legacyManifest?.value.match(/Whole-fixture SHA-256:\*\* `([a-f0-9]{64})`/)?.[1];
  fixtureIntegrity = digest ? `sha256:${digest}` : undefined;
  fixtureResolution = "legacy-context";
}
Config Mutation Side Effect

Lines 99-105 mutate this.config directly based on request.connectorConfig. Because the registry hands out a singleton connector instance shared by concurrent tasks, these mutations can leak between executions if two tasks run simultaneously. Although the comment on line 91 claims per-run state is reset, the config overrides are applied without a finally block to restore the original values. If an exception occurs before the method returns, the mutated config persists. This can cause one task's command, args, or env to bleed into another task's execution.

 
const cfgOverride = (request.connectorConfig || {}) as Partial<SubprocessConfig>;
if (cfgOverride.command !== undefined) this.config.command = cfgOverride.command as string;
if (cfgOverride.args !== undefined) this.config.args = cfgOverride.args as string[];
if (cfgOverride.env !== undefined) this.config.env = { ...(this.config.env || {}), ...(cfgOverride.env as Record<string, string>) };
if (cfgOverride.inputMode !== undefined) this.config.inputMode = cfgOverride.inputMode as any;
if (cfgOverride.outputParser !== undefined) this.config.outputParser = cfgOverride.outputParser as any;
if (cfgOverride.timeout !== undefined) this.config.timeout = cfgOverride.timeout as number;
if (cfgOverride.workingDir !== undefined) this.config.workingDir = cfgOverride.workingDir as string;

@github-actions

github-actions Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Code Suggestions ✨

Latest suggestions up to dc1c7fc

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Clean up temporary fixture directories after use

The temporary fixture directory is never cleaned up, leading to disk space
exhaustion over repeated benchmark runs. Add cleanup logic in a finally block or
return the temp path in metadata so the caller can clean it up.

connectors/pi-web/index.ts [236-238]

 fixtureTempPath = mkdtempSync(join(tmpdir(), "pi-web-benchmark-"));
-cpSync(fixtureSource, fixtureTempPath, { recursive: true, dereference: true });
-cwd = fixtureTempPath;
+try {
+  cpSync(fixtureSource, fixtureTempPath, { recursive: true, dereference: true });
+  cwd = fixtureTempPath;
+  // ... rest of execution
+} finally {
+  if (fixtureTempPath && !keepSession) {
+    rmSync(fixtureTempPath, { recursive: true, force: true });
+  }
+}
Suggestion importance[1-10]: 8

__

Why: Important resource management issue. The temporary directory created at line 236 is never cleaned up, leading to disk space accumulation over repeated runs. The suggestion to add cleanup logic in a finally block or conditional on keepSession is appropriate and addresses a real problem.

Medium
Clean up temporary test directories

The test creates a temporary directory but never cleans it up after execution. This
can lead to disk space accumulation over time, especially in CI environments. Add
cleanup logic using afterEach or a try-finally block to remove the temporary
directory.

connectors/pi-web/index.test.ts [92-114]

 it('rejects an envelope whose filesystem fixture fails integrity verification', async () => {
   const fixturesDir = mkdtempSync(join(tmpdir(), 'agent-health-pi-web-fixtures-'));
-  mkdirSync(join(fixturesDir, 'workspace'));
-  writeFileSync(join(fixturesDir, 'workspace', 'file.txt'), 'actual');
+  try {
+    mkdirSync(join(fixturesDir, 'workspace'));
+    writeFileSync(join(fixturesDir, 'workspace', 'file.txt'), 'actual');
 
-  await expect(new PiWebConnector().execute(
-    'http://pi-web.example',
-    {
-      testCase: {
-        ...testCase,
-        fixture: {
-          type: 'filesystem-workspace',
-          ref: 'workspace',
-          integrity: `sha256:${'0'.repeat(64)}`,
+    await expect(new PiWebConnector().execute(
+      'http://pi-web.example',
+      {
+        testCase: {
+          ...testCase,
+          fixture: {
+            type: 'filesystem-workspace',
+            ref: 'workspace',
+            integrity: `sha256:${'0'.repeat(64)}`,
+          },
         },
+        modelId: 'model',
+        connectorConfig: { fixturesDir },
       },
-      modelId: 'model',
-      connectorConfig: { fixturesDir },
-    },
-    { type: 'none' },
-  )).rejects.toThrow('Fixture integrity mismatch');
+      { type: 'none' },
+    )).rejects.toThrow('Fixture integrity mismatch');
+  } finally {
+    rmSync(fixturesDir, { recursive: true, force: true });
+  }
 });
Suggestion importance[1-10]: 7

__

Why: The test creates a temporary directory with mkdtempSync but never cleans it up, which can accumulate disk space over time. The suggestion to add cleanup logic using rmSync in a finally block is valid and improves resource management, though the impact is moderate since test runs are typically short-lived.

Medium
Extract duplicated session ID capture logic

The session_id capture logic is duplicated across multiple event handlers. Extract
this into a helper method to reduce code duplication and ensure consistent session
ID handling across all event types.

connectors/agui/aguiConverter.ts [179-181]

-if (typeof event.session_id === 'string' && event.session_id) {
-  this.sessionId = event.session_id;
+private captureSessionId(event: any): void {
+  if (typeof event.session_id === 'string' && event.session_id) {
+    this.sessionId = event.session_id;
+  }
 }
 
+// Then call: this.captureSessionId(event);
+
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies code duplication in session ID capture logic. However, the duplication only appears once in the visible diff (lines 179-181), so the impact is limited. Extracting to a helper method would improve maintainability but is not critical.

Low
Defer connector instantiation until needed

The function instantiates every connector immediately during registration, even if
they're never used. This wastes resources and increases startup time. Consider
storing the factories themselves and instantiating connectors lazily on first access
via get() or getForAgent().

connectors/registry.ts [141-153]

 export function registerConnectorFactories(
   factories: Readonly<Record<string, ConnectorFactory>>,
 ): void {
   for (const [name, factory] of Object.entries(factories)) {
-    const connector = factory();
-    if (connector.type !== name) {
-      throw new Error(
-        `Connector factory '${name}' returned connector type '${connector.type}'`,
-      );
-    }
-    connectorRegistry.register(connector);
+    connectorRegistry.registerFactory(name as ConnectorProtocol, factory);
   }
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion to defer connector instantiation is a valid optimization concern, but the current implementation is intentional for the startup registration pattern. The suggestion would require significant refactoring of ConnectorRegistryImpl to store factories instead of instances, and the performance impact of eager instantiation is likely negligible for the small number of connectors. The suggestion is correct but may not be worth the complexity.

Low
Restore mocked fetch after test

The test mocks fetch globally but never restores the original implementation. If
subsequent tests rely on real fetch behavior or other mocks, this can cause test
pollution. Use jest.restoreAllMocks() in an afterEach block or scope the mock to
this test only.

connectors/pi-web/index.test.ts [46-90]

 it('waits for recursive settlement before harvesting and keeps numeric timestamps', async () => {
   const calls: string[] = [];
-  jest.spyOn(global, 'fetch').mockImplementation(async (input, init) => {
+  const fetchSpy = jest.spyOn(global, 'fetch').mockImplementation(async (input, init) => {
     const url = String(input);
     const path = new URL(url).pathname;
     calls.push(`${init?.method || 'GET'} ${path}`);
     if (path === '/api/new-chat') return jsonResponse({ sessionId: 'session-1' });
     if (path === '/api/sessions/session-1/status') {
       return jsonResponse({
         sessionId: 'session-1',
         state: 'idle',
         settled: true,
         ...
       });
     }
     ...
   });
 
+  try {
+    // test body
+  } finally {
+    fetchSpy.mockRestore();
+  }
+});
+
Suggestion importance[1-10]: 3

__

Why: The test file already has afterEach(() => jest.restoreAllMocks()) at line 26, which restores all mocks including fetch after each test. The suggestion is technically correct about the importance of cleanup, but the issue is already addressed by the existing afterEach hook, making this suggestion redundant.

Low
Security
Strengthen path traversal protection with symlink resolution

The path traversal check has a subtle flaw: it doesn't normalize paths before
comparison, allowing bypasses via symlinks or .. sequences that resolve within
bounds after normalization. Use realpathSync to resolve symlinks before the boundary
check.

connectors/pi-web/index.ts [217-221]

 const fixtureSource = resolve(fixturesDir, fixtureRef);
-if (!fixtureSource.startsWith(`${fixturesDir}${sep}`)) {
+const realFixtureSource = realpathSync(fixtureSource);
+const realFixturesDir = realpathSync(fixturesDir);
+if (!realFixtureSource.startsWith(`${realFixturesDir}${sep}`)) {
   throw new Error(`Fixture resolves outside fixtures directory: ${fixtureRef}`);
 }
Suggestion importance[1-10]: 8

__

Why: This is a valid security concern. The current path traversal check at lines 217-221 doesn't resolve symlinks before validation, which could allow bypasses. Using realpathSync would strengthen the security boundary for fixture path validation.

Medium
Possible issue
Clear interval before resolving to prevent leaks

The interval continues running after resolve() is called, potentially causing memory
leaks or race conditions. Clear the interval immediately after resolving to prevent
further executions.

connectors/agui/sseStream.ts [167-174]

 const idleCheckInterval = setInterval(() => {
   const idleTime = Date.now() - lastEventTime;
   if (eventCount > 0 && idleTime > idleTimeoutMs) {
     debug('SSE', `Idle timeout: no events for ${idleTime}ms (threshold: ${idleTimeoutMs}ms)`);
+    clearInterval(idleCheckInterval);
     this.abort();
     resolve('idle_timeout');
   }
 }, 1000);
Suggestion importance[1-10]: 7

__

Why: Valid bug identification. The interval at lines 167-174 continues running after resolve('idle_timeout') is called, which could cause memory leaks. The interval is cleared in the finally block, but clearing it immediately before resolving is more robust and prevents potential race conditions.

Medium

Previous suggestions

Suggestions up to commit e49aab7
CategorySuggestion                                                                                                                                    Impact
General
Clean up temporary fixture directories

The temporary fixture directory is created but never cleaned up. If the connector
throws an error or the process crashes before cleanup, temporary directories
accumulate in tmpdir(). Wrap execution in a try-finally block to ensure cleanup even
on failure.

connectors/pi-web/index.ts [236-238]

 fixtureTempPath = mkdtempSync(join(tmpdir(), "pi-web-benchmark-"));
-cpSync(fixtureSource, fixtureTempPath, { recursive: true, dereference: true });
+try {
+  cpSync(fixtureSource, fixtureTempPath, { recursive: true, dereference: true });
+  cwd = fixtureTempPath;
+  // ... rest of execution
+} finally {
+  if (fixtureTempPath && !keepSession) {
+    rmSync(fixtureTempPath, { recursive: true, force: true });
+  }
+}
Suggestion importance[1-10]: 8

__

Why: Valid concern about resource leaks. The temporary directory created at line 236 is never cleaned up in the current code, which can accumulate disk usage over time. The suggestion to add cleanup in a finally block is a good practice for resource management.

Medium
Reset sessionId between runs

The sessionId is captured but never reset between runs. If the same converter
instance processes multiple events from different runs, the sessionId from a
previous run may leak into subsequent runs. Reset sessionId in handleRunStarted to
ensure isolation.

connectors/agui/aguiConverter.ts [179-181]

 if (typeof event.session_id === 'string' && event.session_id) {
   this.sessionId = event.session_id;
 }
 
+// In handleRunStarted method, add:
+this.sessionId = undefined;
+
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that sessionId is already reset in handleRunStarted (line 166: this.sessionId = null), making the proposed change redundant. However, the concern about state isolation is valid for understanding the code's behavior.

Medium
Clean up temporary test directories

The test creates a temporary directory but never cleans it up after execution. Add
cleanup logic using afterEach or a try-finally block to prevent test pollution and
resource leaks. Consider using rmSync with recursive: true to remove the temporary
directory.

connectors/pi-web/index.test.ts [92-114]

 it('rejects an envelope whose filesystem fixture fails integrity verification', async () => {
   const fixturesDir = mkdtempSync(join(tmpdir(), 'agent-health-pi-web-fixtures-'));
-  mkdirSync(join(fixturesDir, 'workspace'));
-  writeFileSync(join(fixturesDir, 'workspace', 'file.txt'), 'actual');
+  try {
+    mkdirSync(join(fixturesDir, 'workspace'));
+    writeFileSync(join(fixturesDir, 'workspace', 'file.txt'), 'actual');
 
-  await expect(new PiWebConnector().execute(
-    'http://pi-web.example',
-    {
-      testCase: {
-        ...testCase,
-        fixture: {
-          type: 'filesystem-workspace',
-          ref: 'workspace',
-          integrity: `sha256:${'0'.repeat(64)}`,
+    await expect(new PiWebConnector().execute(
+      'http://pi-web.example',
+      {
+        testCase: {
+          ...testCase,
+          fixture: {
+            type: 'filesystem-workspace',
+            ref: 'workspace',
+            integrity: `sha256:${'0'.repeat(64)}`,
+          },
         },
+        modelId: 'model',
+        connectorConfig: { fixturesDir },
       },
-      modelId: 'model',
-      connectorConfig: { fixturesDir },
-    },
-    { type: 'none' },
-  )).rejects.toThrow('Fixture integrity mismatch');
+      { type: 'none' },
+    )).rejects.toThrow('Fixture integrity mismatch');
+  } finally {
+    rmSync(fixturesDir, { recursive: true, force: true });
+  }
 });
Suggestion importance[1-10]: 7

__

Why: The test creates a temporary directory but never cleans it up, which can lead to test pollution and resource leaks. Adding cleanup logic is a valid improvement for test hygiene, though the impact is moderate since test environments are typically ephemeral.

Medium
Clear interval after timeout resolution

The setInterval callback resolves the promise but doesn't clear the interval itself.
After resolve('idle_timeout') is called, the interval continues firing every second
until the outer finally block clears it. Clear the interval immediately after
resolving to avoid unnecessary timer callbacks.

connectors/agui/sseStream.ts [167-174]

 idleCheckInterval = setInterval(() => {
   const idleTime = Date.now() - lastEventTime;
   if (eventCount > 0 && idleTime > idleTimeoutMs) {
     debug('SSE', `Idle timeout: no events for ${idleTime}ms (threshold: ${idleTimeoutMs}ms)`);
+    clearInterval(idleCheckInterval!);
     this.abort();
     resolve('idle_timeout');
   }
 }, 1000);
Suggestion importance[1-10]: 6

__

Why: The suggestion identifies a minor inefficiency where the interval continues firing after resolving the promise until the finally block clears it. While the finally block (line 239-241) does clean up the interval, clearing it immediately after resolution is a minor optimization that reduces unnecessary callbacks.

Low
Implement lazy connector instantiation

The function instantiates every connector immediately during registration, which may
be wasteful if connectors are never used. Consider lazy instantiation by storing
factories in the registry and creating connector instances only when get() or
getForAgent() is called. This improves startup performance and memory usage.

connectors/registry.ts [141-153]

-export function registerConnectorFactories(
-  factories: Readonly<Record<string, ConnectorFactory>>,
-): void {
-  for (const [name, factory] of Object.entries(factories)) {
-    const connector = factory();
-    if (connector.type !== name) {
-      throw new Error(
-        `Connector factory '${name}' returned connector type '${connector.type}'`,
-      );
-    }
-    connectorRegistry.register(connector);
-  }
+// Store factories instead of instances
+private factories: Map<ConnectorProtocol, ConnectorFactory> = new Map();
+
+registerFactory(type: ConnectorProtocol, factory: ConnectorFactory): void {
+  this.factories.set(type, factory);
 }
 
+get(type: ConnectorProtocol): AgentConnector | undefined {
+  if (!this.connectors.has(type) && this.factories.has(type)) {
+    const factory = this.factories.get(type)!;
+    const connector = factory();
+    this.connectors.set(type, connector);
+  }
+  return this.connectors.get(type);
+}
+
Suggestion importance[1-10]: 6

__

Why: Lazy instantiation could improve startup performance and memory usage by deferring connector creation until needed. However, the improved_code snippet is incomplete (missing the full implementation of registerConnectorFactories and the factory map initialization), and the current eager instantiation is acceptable for the typical number of connectors. The suggestion is valid but requires more work to implement correctly.

Low
Avoid redundant map lookups

The fallback logic attempts to retrieve DEFAULT_CONNECTOR_TYPE twice when a
connector is not found, which is inefficient. Cache the default connector lookup
result to avoid redundant map access. This also improves readability by making the
fallback path clearer.

connectors/registry.ts [69-98]

 getForAgent(agent: AgentConfigWithConnector): AgentConnector {
   // Handle mock:// endpoint prefix (legacy pattern)
   if (agent.endpoint.startsWith('mock://')) {
     const mockConnector = this.get('mock');
     if (mockConnector) {
       return mockConnector;
     }
     console.warn('[ConnectorRegistry] Mock connector not registered, falling back to default');
   }
 
   // Use explicit connector type if specified
   const connectorType = agent.connectorType ?? DEFAULT_CONNECTOR_TYPE;
   const connector = this.get(connectorType);
 
   if (!connector) {
+    const defaultConnector = this.get(DEFAULT_CONNECTOR_TYPE);
     console.error(
       `[ConnectorRegistry] Connector not found for type: ${connectorType}, ` +
       `falling back to ${DEFAULT_CONNECTOR_TYPE}`
     );
-    const defaultConnector = this.get(DEFAULT_CONNECTOR_TYPE);
     if (!defaultConnector) {
       throw new Error(
         `No connector registered for type '${connectorType}' and no default connector available`
       );
     }
     return defaultConnector;
   }
 
   return connector;
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that this.get(DEFAULT_CONNECTOR_TYPE) is called twice in the fallback path. However, the performance impact is minimal (a single map lookup), and the current code structure is clear. The improvement is valid but offers only marginal benefit.

Low
Security
Normalize paths before traversal check

The path traversal check uses sep but resolve() normalizes paths to the platform
separator. On Windows, a malicious fixtureRef like ../../../etc/passwd could bypass
this check if fixturesDir uses forward slashes. Normalize both paths before
comparison to prevent directory traversal attacks.

connectors/pi-web/index.ts [217-221]

-const fixtureSource = resolve(fixturesDir, fixtureRef);
-if (!fixtureSource.startsWith(`${fixturesDir}${sep}`)) {
+const normalizedFixturesDir = resolve(fixturesDir);
+const fixtureSource = resolve(normalizedFixturesDir, fixtureRef);
+if (!fixtureSource.startsWith(`${normalizedFixturesDir}${sep}`)) {
   throw new Error(`Fixture resolves outside fixtures directory: ${fixtureRef}`);
 }
Suggestion importance[1-10]: 3

__

Why: The existing code already uses resolve() on both fixturesDir (line 218) and the combined path (line 219), which normalizes paths. The suggestion's concern about Windows path separators is addressed by the existing implementation. The improvement is marginal.

Low
Suggestions up to commit 2d47613
CategorySuggestion                                                                                                                                    Impact
Security
Strengthen path traversal protection

The path traversal check is incomplete. An attacker can bypass it using encoded path
separators or symlinks. Use fs.realpathSync() to resolve the canonical path before
validation, ensuring the resolved path stays within the fixtures directory even
after following symlinks.

connectors/pi-web/index.ts [218-221]

 const fixtureSource = resolve(fixturesDir, fixtureRef);
-if (!fixtureSource.startsWith(`${fixturesDir}${sep}`)) {
+const canonicalSource = realpathSync(fixtureSource);
+const canonicalFixturesDir = realpathSync(fixturesDir);
+if (!canonicalSource.startsWith(`${canonicalFixturesDir}${sep}`)) {
   throw new Error(`Fixture resolves outside fixtures directory: ${fixtureRef}`);
 }
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical security vulnerability. The current path traversal check can be bypassed using symlinks, which could allow an attacker to access files outside the fixtures directory. Using realpathSync() to resolve canonical paths before validation is the correct mitigation.

High
Prevent memory exhaustion from unbounded accumulation

Unbounded string accumulation in argsAccumulator can cause memory exhaustion if a
malicious or buggy agent streams gigabytes of tool arguments. Enforce a reasonable
size limit (e.g., 1MB) and reject oversized payloads to prevent denial-of-service
via memory consumption.

connectors/agui/aguiConverter.ts [328-337]

 private handleToolCallArgs(event: ToolCallArgsEvent): TrajectoryStep[] {
   const toolState = this.activeTools.get(event.toolCallId);
   if (toolState) {
+    const MAX_ARGS_SIZE = 1024 * 1024; // 1MB limit
+    if (toolState.argsAccumulator.length + event.delta.length > MAX_ARGS_SIZE) {
+      throw new Error(`Tool args exceeded maximum size of ${MAX_ARGS_SIZE} bytes`);
+    }
     toolState.argsAccumulator += event.delta;
     debug('Converter', `Tool args delta accumulated (${toolState.argsAccumulator.length} chars total)`);
   }
   return [];
 }
Suggestion importance[1-10]: 8

__

Why: This identifies a legitimate security concern where unbounded string accumulation could lead to memory exhaustion. Adding a size limit on argsAccumulator is a reasonable safeguard against malicious or buggy agents streaming excessive data. The 1MB limit is practical for tool arguments.

Medium
General
Clean up temporary test directories

The test creates a temporary directory but never cleans it up, potentially leaving
filesystem artifacts after test execution. Add cleanup logic using afterEach or a
try-finally block to remove the temporary directory, preventing test pollution and
resource leaks.

connectors/pi-web/index.test.ts [92-114]

+let tempDir: string | undefined;
+
+afterEach(() => {
+  if (tempDir) {
+    rmSync(tempDir, { recursive: true, force: true });
+    tempDir = undefined;
+  }
+});
+
 it('rejects an envelope whose filesystem fixture fails integrity verification', async () => {
-  const fixturesDir = mkdtempSync(join(tmpdir(), 'agent-health-pi-web-fixtures-'));
-  mkdirSync(join(fixturesDir, 'workspace'));
-  writeFileSync(join(fixturesDir, 'workspace', 'file.txt'), 'actual');
+  tempDir = mkdtempSync(join(tmpdir(), 'agent-health-pi-web-fixtures-'));
+  mkdirSync(join(tempDir, 'workspace'));
+  writeFileSync(join(tempDir, 'workspace', 'file.txt'), 'actual');
 
   await expect(new PiWebConnector().execute(
     'http://pi-web.example',
     {
       testCase: {
         ...testCase,
         fixture: {
           type: 'filesystem-workspace',
           ref: 'workspace',
           integrity: `sha256:${'0'.repeat(64)}`,
         },
       },
       modelId: 'model',
-      connectorConfig: { fixturesDir },
+      connectorConfig: { fixturesDir: tempDir },
     },
     { type: 'none' },
   )).rejects.toThrow('Fixture integrity mismatch');
 });
Suggestion importance[1-10]: 7

__

Why: The test creates a temporary directory but never cleans it up, which can lead to filesystem pollution and resource leaks. While this is a valid concern for test hygiene, the impact is moderate since test environments are typically ephemeral and the OS will eventually clean up /tmp. Adding cleanup logic would improve test quality.

Medium
Add retry logic for network failures

The api helper lacks retry logic for transient network failures. A single network
hiccup will fail the entire benchmark run. Add exponential backoff retry for
idempotent operations (GET requests and safe POSTs) to improve reliability against
temporary connectivity issues.

connectors/pi-web/index.ts [249-258]

-const response = await fetch(`${baseUrl}${path}`, {
-  method,
-  headers: {
-    ...(token ? { Authorization: `Bearer ${token}` } : {}),
-    ...(body !== undefined ? { "Content-Type": "application/json" } : {}),
-  },
-  ...(body !== undefined ? { body: JSON.stringify(body) } : {}),
-  signal: AbortSignal.timeout(API_TIMEOUT_MS),
-});
+let lastError: Error | undefined;
+for (let attempt = 0; attempt < 3; attempt++) {
+  try {
+    const response = await fetch(`${baseUrl}${path}`, {
+      method,
+      headers: {
+        ...(token ? { Authorization: `Bearer ${token}` } : {}),
+        ...(body !== undefined ? { "Content-Type": "application/json" } : {}),
+      },
+      ...(body !== undefined ? { body: JSON.stringify(body) } : {}),
+      signal: AbortSignal.timeout(API_TIMEOUT_MS),
+    });
+    // ... existing response handling ...
+  } catch (error) {
+    lastError = error as Error;
+    if (attempt < 2) await delay(Math.pow(2, attempt) * 1000);
+  }
+}
+throw lastError;
Suggestion importance[1-10]: 6

__

Why: Adding retry logic would improve reliability against transient network failures. However, the suggestion's implementation is incomplete (missing response handling logic) and the impact is moderate since benchmark runs can be manually retried. The improvement is valuable but not critical.

Low
Defer connector instantiation until needed

The function instantiates every connector immediately during registration, which
could be expensive if connectors perform initialization work in their constructors.
Consider lazy instantiation by storing factories and creating connector instances
only when get() or getForAgent() is called, improving startup performance.

connectors/registry.ts [141-153]

-export function registerConnectorFactories(
-  factories: Readonly<Record<string, ConnectorFactory>>,
-): void {
-  for (const [name, factory] of Object.entries(factories)) {
-    const connector = factory();
-    if (connector.type !== name) {
-      throw new Error(
-        `Connector factory '${name}' returned connector type '${connector.type}'`,
-      );
+class ConnectorRegistryImpl implements ConnectorRegistry {
+  private connectors: Map<ConnectorProtocol, AgentConnector> = new Map();
+  private factories: Map<ConnectorProtocol, ConnectorFactory> = new Map();
+
+  register(connector: AgentConnector): void {
+    if (this.connectors.has(connector.type)) {
+      console.warn(`[ConnectorRegistry] Overwriting existing connector for type: ${connector.type}`);
     }
-    connectorRegistry.register(connector);
+    this.connectors.set(connector.type, connector);
   }
+
+  registerFactory(type: ConnectorProtocol, factory: ConnectorFactory): void {
+    this.factories.set(type, factory);
+  }
+
+  get(type: ConnectorProtocol): AgentConnector | undefined {
+    if (!this.connectors.has(type) && this.factories.has(type)) {
+      const connector = this.factories.get(type)!();
+      this.connectors.set(type, connector);
+    }
+    return this.connectors.get(type);
+  }
+  ...
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion proposes lazy instantiation to improve startup performance, which is a valid optimization. However, the current eager instantiation is intentional and simple—connectors are lightweight and the startup cost is negligible. The proposed change adds complexity (dual storage of factories and instances) without clear evidence of a performance problem. This is a premature optimization that trades simplicity for unproven gains.

Low
Restore mocked fetch implementation

The test mocks fetch globally but never restores the original implementation, which
could affect subsequent tests. While afterEach(() => jest.restoreAllMocks()) exists
at the suite level, explicitly restore mocks in this test or verify the afterEach
hook is properly scoped to prevent test interference.

connectors/pi-web/index.test.ts [46-90]

 it('waits for recursive settlement before harvesting and keeps numeric timestamps', async () => {
   const calls: string[] = [];
-  jest.spyOn(global, 'fetch').mockImplementation(async (input, init) => {
+  const fetchSpy = jest.spyOn(global, 'fetch').mockImplementation(async (input, init) => {
     const url = String(input);
     const path = new URL(url).pathname;
     calls.push(`${init?.method || 'GET'} ${path}`);
     if (path === '/api/new-chat') return jsonResponse({ sessionId: 'session-1' });
     if (path === '/api/sessions/session-1/status') {
       return jsonResponse({
         sessionId: 'session-1',
         state: 'idle',
         settled: true,
         pendingWakeups: 0,
         trackedWorkers: [{ id: 'worker-1', state: 'idle', settled: true }],
       });
     }
     if (path === '/api/messages') {
       return jsonResponse({
         messages: [{ role: 'assistant', text: 'complete', timestamp: '1712345678901' }],
       });
     }
     return jsonResponse({ ok: true });
   });
-  ...
+
+  try {
+    const result = await new PiWebConnector().execute(...);
+    ...
+  } finally {
+    fetchSpy.mockRestore();
+  }
 });
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies that fetch is mocked globally, but the test file already has afterEach(() => jest.restoreAllMocks()) at line 26, which will restore all mocks including fetch. The explicit try-finally restoration is redundant and adds unnecessary complexity. The existing afterEach hook is sufficient.

Low
Suggestions up to commit 5a132e2
CategorySuggestion                                                                                                                                    Impact
Security
Normalize paths before security check

The path traversal check may fail on Windows when comparing paths with different
separators. Normalize both paths using resolve() before comparison to ensure
consistent path validation across platforms.

connectors/pi-web/index.ts [218-221]

-if (!fixtureSource.startsWith(`${fixturesDir}${sep}`)) {
+const normalizedSource = resolve(fixtureSource);
+const normalizedDir = resolve(fixturesDir);
+if (!normalizedSource.startsWith(`${normalizedDir}${sep}`)) {
   throw new Error(`Fixture resolves outside fixtures directory: ${fixtureRef}`);
 }
Suggestion importance[1-10]: 7

__

Why: Valid security improvement for path traversal protection. The fixtureSource is already resolved at line 218, but normalizing fixturesDir ensures consistent comparison across platforms.

Medium
General
Extract duplicated session ID capture logic

The sessionId capture logic is duplicated across multiple event handlers. Extract
this into a helper method to reduce code duplication and ensure consistent session
ID handling across all event types.

connectors/agui/aguiConverter.ts [179-181]

-if (typeof event.session_id === 'string' && event.session_id) {
-  this.sessionId = event.session_id;
+private captureSessionId(event: any): void {
+  if (typeof event.session_id === 'string' && event.session_id) {
+    this.sessionId = event.session_id;
+  }
 }
 
+// Then call: this.captureSessionId(event);
+
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies code duplication in session ID capture logic. However, the duplication only appears once in the new file at lines 179-181, making the refactoring less impactful than claimed.

Low
Document temporary directory cleanup responsibility

The temporary fixture directory is created but never cleaned up, leading to disk
space leaks. Add cleanup logic in a finally block or return the temp path in
metadata so the caller can clean it up after the test completes.

connectors/pi-web/index.ts [236-238]

 fixtureTempPath = mkdtempSync(join(tmpdir(), "pi-web-benchmark-"));
-cpSync(fixtureSource, fixtureTempPath, { recursive: true, dereference: true });
-cwd = fixtureTempPath;
+try {
+  cpSync(fixtureSource, fixtureTempPath, { recursive: true, dereference: true });
+  cwd = fixtureTempPath;
+  // ... rest of execution
+} finally {
+  // Cleanup handled by caller via metadata.fixtureTempPath
+}
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about temp directory cleanup. However, the metadata already includes fixtureTempPath (line 377), and the improved_code doesn't actually add cleanup logic—it only adds a comment about caller responsibility.

Low
Clear interval after timeout resolution

The interval continues running even after resolving the promise, potentially causing
memory leaks. Clear the interval immediately after resolving to prevent resource
leaks in long-running processes.

connectors/agui/sseStream.ts [165-174]

 const idleTimeoutPromise = new Promise<string>((resolve) => {
   idleCheckInterval = setInterval(() => {
     const idleTime = Date.now() - lastEventTime;
     if (eventCount > 0 && idleTime > idleTimeoutMs) {
       debug('SSE', `Idle timeout: no events for ${idleTime}ms (threshold: ${idleTimeoutMs}ms)`);
+      if (idleCheckInterval) clearInterval(idleCheckInterval);
       this.abort();
       resolve('idle_timeout');
     }
   }, 1000);
 });
Suggestion importance[1-10]: 3

__

Why: The suggestion identifies a potential issue, but the interval is already cleared in the finally block at line 240-242, making this change redundant. The early clear would be a minor optimization.

Low

@codecov

codecov Bot commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.26301% with 238 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.48%. Comparing base (71311a4) to head (dc1c7fc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
connectors/pi-web/index.ts 45.56% 86 Missing and 43 partials ⚠️
connectors/claude-code/index.ts 82.77% 20 Missing and 16 partials ⚠️
connectors/subprocess/index.ts 91.44% 12 Missing and 4 partials ⚠️
connectors/base/index.ts 82.35% 7 Missing and 5 partials ⚠️
connectors/rest/index.ts 80.39% 3 Missing and 7 partials ⚠️
connectors/kiro/index.ts 81.63% 5 Missing and 4 partials ⚠️
connectors/agui/index.ts 87.80% 5 Missing ⚠️
connectors/registry.ts 88.63% 5 Missing ⚠️
connectors/strands/index.ts 95.65% 3 Missing and 2 partials ⚠️
services/evaluation/index.ts 73.33% 3 Missing and 1 partial ⚠️
... and 3 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
- Coverage   63.62%   63.48%   -0.14%     
==========================================
  Files         385      384       -1     
  Lines       31339    31571     +232     
  Branches     9394     9501     +107     
==========================================
+ Hits        19939    20044     +105     
- Misses       9464     9549      +85     
- Partials     1936     1978      +42
Flag Coverage Δ
e2e 45.70% <ø> (ø)
integration 42.99% <51.46%> (-0.62%) ⬇️
unit 68.14% <76.39%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
components/RunDetailsContent.tsx 58.23% <100.00%> (-0.31%) ⬇️
connectors/agui/aguiConverter.ts 80.45% <ø> (ø)
connectors/agui/payloadBuilder.ts 84.21% <ø> (ø)
connectors/agui/sseStream.ts 87.09% <ø> (ø)
connectors/index.ts 100.00% <100.00%> (ø)
connectors/openai-compatible/index.ts 100.00% <100.00%> (ø)
connectors/server.ts 100.00% <100.00%> (ø)
lib/config/loader.ts 79.38% <ø> (ø)
lib/constants.ts 77.41% <ø> (ø)
lib/index.ts 100.00% <100.00%> (ø)
... and 20 more

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ashwin-pc
fix(connectors): resolve aliases in bundled entrypoints
2d47613 
Signed-off-by: ashwin pc <ashwinpc@amazon.com>
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 2d47613

@ashwin-pc
merge main into connector-registry
e49aab7 
Signed-off-by: ashwin pc <ashwinpc@amazon.com>

# Conflicts:
#	CHANGELOG.md
@github-actions

github-actions Bot commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit dc1c7fc.

PathLineSeverityDescription
connectors/subprocess/index.ts245mediumAll parent process environment variables (process.env) are spread into the child process environment unconditionally. This means secrets such as ANTHROPIC_API_KEY, AWS_SECRET_ACCESS_KEY, and any other sensitive env vars present in the parent process are automatically inherited by every spawned subprocess agent, regardless of which connector or agent is executing. An agent that logs or exfiltrates its own environment would capture the host's secrets.
connectors/pi-web/index.ts291lowThe local working directory path (cwd) is sent to the remote pi-web server in the POST /api/new-chat request body. This discloses the host filesystem layout to the remote endpoint. While documented as intentional, it constitutes information leakage about the host environment to an external server.
connectors/claude-code/index.ts487lowOTEL_EXPORTER_OTLP_HEADERS from the host environment is forwarded verbatim into the Claude Code child process environment. This header commonly carries bearer tokens or API keys for the OTLP collector. Forwarding it to a subprocess extends the trust boundary and means any compromise of the subprocess could capture these credentials.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 1 | Low: 2

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit e49aab7

@ashwin-pc
test(connectors): disambiguate pi from pi-web
dc1c7fc 
Signed-off-by: ashwin pc <ashwinpc@amazon.com>
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit dc1c7fc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TackAdam TackAdam Awaiting requested review from TackAdam TackAdam is a code owner
@goyamegh goyamegh Awaiting requested review from goyamegh goyamegh is a code owner
@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner
@jasonlhamazon jasonlhamazon Awaiting requested review from jasonlhamazon jasonlhamazon is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ashwin-pc