Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

init remote sessions #212

Merged
merged 5 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions apps/ctrlshell/README.md

This file was deleted.

16 changes: 0 additions & 16 deletions apps/ctrlshell/src/payloads/session-create.ts

This file was deleted.

19 changes: 0 additions & 19 deletions apps/ctrlshell/src/payloads/session-input.ts

This file was deleted.

11 changes: 0 additions & 11 deletions apps/ctrlshell/src/payloads/session-output.ts

This file was deleted.

55 changes: 0 additions & 55 deletions apps/ctrlshell/src/routing.ts

This file was deleted.

17 changes: 0 additions & 17 deletions apps/ctrlshell/src/session-auditor.ts

This file was deleted.

27 changes: 0 additions & 27 deletions apps/ctrlshell/src/user-socket.ts

This file was deleted.

22 changes: 0 additions & 22 deletions apps/ctrlshell/src/utils.ts

This file was deleted.

2 changes: 1 addition & 1 deletion apps/ctrlshell/Dockerfile → apps/target-proxy/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,4 @@ RUN adduser --system --uid 1001 expressjs
USER expressjs

COPY --from=installer /app .
CMD ["node", "apps/webshell-router/dist/index.js"]
CMD ["node", "apps/target-proxy/dist/index.js"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

The CMD path is incorrect and needs to be updated

The verification shows that:

  1. The source code exists at apps/target-proxy/src/index.ts
  2. The tsconfig.json correctly configures output to dist directory
  3. The dist directory doesn't exist in the repository, as it's a build artifact
  4. There's no explicit build script in the package.json

The current CMD ["node", "apps/target-proxy/dist/index.js"] won't work because:

  • The build output should be in the container's working directory, not under apps/
  • The Dockerfile should copy built artifacts to /app/dist during the build stage

Suggested fix:

CMD ["node", "dist/index.js"]
🔗 Analysis chain

Verify the application entry point change

The CMD has been updated to use the new target-proxy application. Let's verify this aligns with the built artifacts.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the target-proxy build output and entry point
# Expected: Should find the index.js file in the target-proxy dist directory
# and confirm it's the main entry point in package.json

# Check if the dist directory and entry point exist in the repo
fd -t f "index.js" "apps/target-proxy/dist"

# Check the main entry point in package.json
fd -t f "package.json" "apps/target-proxy" -x jq -r '.main'

Length of output: 446


Script:

#!/bin/bash
# Let's verify the target-proxy application structure and entry point more thoroughly

# Check if the source file exists
fd "index.ts" "apps/target-proxy/src"

# Check package.json contents for build configuration
fd "package.json" "apps/target-proxy" -x cat

# Check turbo.json for build configuration
fd "turbo.json" . -x cat

# Check if there are any build scripts in the repository root
cat package.json | jq -r '.scripts | select(.build != null)'

Length of output: 3968


Script:

#!/bin/bash
# Let's check the build configuration and output directory

# Check if there's a build script in target-proxy's package.json that we might have missed
fd "package.json" "apps/target-proxy" -x jq -r '.scripts.build'

# Check if there's a tsconfig.json that might define the output directory
fd "tsconfig.json" "apps/target-proxy" -x cat

# Look for build-related configurations in parent directories
fd "tsconfig.json" "apps" -x cat
fd "tsconfig.json" . -x cat

# Check if the dist directory exists and what it contains
fd . "apps/target-proxy/dist"

Length of output: 9701

30 changes: 30 additions & 0 deletions apps/target-proxy/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Target Proxy Router

Simple router that redirects web terminal requests to instances and vis-versa.

### Sequence Diagram

```mermaid
sequenceDiagram
autonumber

participant AG as Agebt
participant PR as Proxy
participant CP as Ctrlplane
participant DE as Developer

opt Init Agent
AG->>PR: Connects to Proxy
PR->>CP: Register as target
AG->>PR: Sends heartbeat
end

opt Session
DE->>CP: Opens session
CP->>PR: Forwards commands
PR->>AG: Receives commands
AG->>PR: Sends output
PR->>CP: Sends output
CP->>DE: Displays output
end
```
File renamed without changes.
10 changes: 7 additions & 3 deletions apps/ctrlshell/package.json → apps/target-proxy/package.json
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
{
"name": "@ctrlplane/ctrlshell",
"name": "@ctrlplane/target-proxy",
"private": true,
"type": "module",
"scripts": {
"clean": "rm -rf .turbo node_modules",
"dev": "tsx watch --clear-screen=false src/index.ts",
"dev:t": "pnpm with-env tsx watch --clear-screen=false src/index.ts",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Script naming convention deviates from monorepo standards

The script naming dev:t in apps/target-proxy is inconsistent with other packages in the monorepo:

  • Most packages use the standard dev script name
  • While some packages use with-env, they maintain the standard dev script name
  • Other apps like webservice, event-worker, and jobs all use dev (not dev:t)

Recommendation:

  • Rename dev:t to dev to maintain consistency with the rest of the monorepo
  • Keep the with-env script as is, as it follows the existing pattern
🔗 Analysis chain

Verify environment setup and consider script naming convention

The changes introduce environment variable handling:

  1. The dev script has been renamed to dev:t - consider if this naming aligns with other packages in the monorepo
  2. A new with-env script loads variables from ../../.env

Also applies to: 10-10


Let me try another script to better verify the script naming patterns across packages.

This script will help us see the script names used across different packages to verify naming consistency. Let's analyze the results before making a final determination.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify environment setup and script naming consistency

# Check if .env file exists
echo "Checking for .env file..."
[ -f "../../.env" ] && echo "✓ .env file exists" || echo "✗ .env file missing"

# Check script naming patterns across packages
echo "Checking script naming patterns in other packages..."
fd "package.json" -x jq -r '.scripts | keys[]' {} \; | sort | uniq -c

Length of output: 1275


Script:

#!/bin/bash
# Check script naming patterns across packages
echo "Checking script naming in package.json files..."
fd "package.json" -x sh -c 'echo "=== $(dirname {}) ===" && cat {} | jq -r ".scripts | keys[]" 2>/dev/null' \;

Length of output: 2217

"lint": "eslint",
"format": "prettier --check . --ignore-path ../../.gitignore"
"format": "prettier --check . --ignore-path ../../.gitignore",
"with-env": "dotenv -e ../../.env --"
},
"dependencies": {
"@ctrlplane/db": "workspace:*",
"@ctrlplane/job-dispatch": "workspace:*",
"@ctrlplane/logger": "workspace:*",
"@ctrlplane/validators": "workspace:*",
"@t3-oss/env-core": "catalog:",
"cookie-parser": "^1.4.6",
"cors": "^2.8.5",
Expand Down
File renamed without changes.
File renamed without changes.
Original file line number Diff line number Diff line change
@@ -1,35 +1,90 @@
import type { IncomingMessage } from "http";
import type WebSocket from "ws";
import type { MessageEvent } from "ws";
import { v4 as uuidv4 } from "uuid";

import type { InsertTarget, Target } from "@ctrlplane/db/schema";
import type {
AgentHeartbeat,
SessionCreate,
SessionDelete,
SessionInput,
SessionOutput,
} from "./payloads";
import { agentHeartbeat, sessionOutput } from "./payloads";
SessionResize,
} from "@ctrlplane/validators/session";
import type { IncomingMessage } from "http";
import type WebSocket from "ws";
import type { MessageEvent } from "ws";

import { eq } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import * as schema from "@ctrlplane/db/schema";
import { upsertTargets } from "@ctrlplane/job-dispatch";
import { logger } from "@ctrlplane/logger";
import {
agentConnect,
agentHeartbeat,
sessionOutput,
} from "@ctrlplane/validators/session";

import { ifMessage } from "./utils";

export class AgentSocket {
static from(socket: WebSocket, request: IncomingMessage) {
if (request.headers["x-api-key"] == null) return null;
return new AgentSocket(socket, request);
static async from(socket: WebSocket, request: IncomingMessage) {
const agentName = request.headers["x-agent-name"]?.toString();
if (agentName == null) {
logger.warn("Agent connection rejected - missing agent name");
return null;
}

const apiKey = request.headers["x-api-key"]?.toString();
if (apiKey == null) {
logger.error("Agent connection rejected - missing API key");
throw new Error("API key is required.");
}

Comment on lines +29 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implement validation of the API key

The apiKey is extracted but not validated against any authentication mechanism. This could allow unauthorized access if the API key is incorrect or invalid.

Consider adding logic to validate the apiKey to ensure that only authorized agents can connect. Do you need assistance in implementing the API key validation?

const workspaceSlug = request.headers["x-workspace"]?.toString();
if (workspaceSlug == null) {
logger.error("Agent connection rejected - missing workspace slug");
throw new Error("Workspace slug is required.");
Comment on lines +23 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ensure consistent error handling for missing headers

The error handling for missing headers is inconsistent:

  • For x-agent-name, the code logs a warning and returns null.
  • For x-api-key and x-workspace, the code logs an error and throws an exception.

Consider making the error handling consistent. Here's a suggested refactor to throw exceptions for all missing headers:

 const agentName = request.headers["x-agent-name"]?.toString();
 if (agentName == null) {
-  logger.warn("Agent connection rejected - missing agent name");
-  return null;
+  logger.error("Agent connection rejected - missing agent name");
+  throw new Error("Agent name is required.");
 }

 const apiKey = request.headers["x-api-key"]?.toString();
 if (apiKey == null) {
   logger.error("Agent connection rejected - missing API key");
   throw new Error("API key is required.");
 }

 const workspaceSlug = request.headers["x-workspace"]?.toString();
 if (workspaceSlug == null) {
   logger.error("Agent connection rejected - missing workspace slug");
   throw new Error("Workspace slug is required.");
 }

Alternatively, if returning null is preferred, apply it consistently:

const agentName = request.headers["x-agent-name"]?.toString();
if (agentName == null) {
  logger.warn("Agent connection rejected - missing agent name");
  return null;
}

const apiKey = request.headers["x-api-key"]?.toString();
if (apiKey == null) {
  logger.error("Agent connection rejected - missing API key");
- throw new Error("API key is required.");
+ return null;
}

const workspaceSlug = request.headers["x-workspace"]?.toString();
if (workspaceSlug == null) {
  logger.error("Agent connection rejected - missing workspace slug");
- throw new Error("Workspace slug is required.");
+ return null;
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const agentName = request.headers["x-agent-name"]?.toString();
if (agentName == null) {
logger.warn("Agent connection rejected - missing agent name");
return null;
}
const apiKey = request.headers["x-api-key"]?.toString();
if (apiKey == null) {
logger.error("Agent connection rejected - missing API key");
throw new Error("API key is required.");
}
const workspaceSlug = request.headers["x-workspace"]?.toString();
if (workspaceSlug == null) {
logger.error("Agent connection rejected - missing workspace slug");
throw new Error("Workspace slug is required.");
const agentName = request.headers["x-agent-name"]?.toString();
if (agentName == null) {
logger.error("Agent connection rejected - missing agent name");
throw new Error("Agent name is required.");
}
const apiKey = request.headers["x-api-key"]?.toString();
if (apiKey == null) {
logger.error("Agent connection rejected - missing API key");
throw new Error("API key is required.");
}
const workspaceSlug = request.headers["x-workspace"]?.toString();
if (workspaceSlug == null) {
logger.error("Agent connection rejected - missing workspace slug");
throw new Error("Workspace slug is required.");
```
Option 2 (All return null):
```suggestion
const agentName = request.headers["x-agent-name"]?.toString();
if (agentName == null) {
logger.warn("Agent connection rejected - missing agent name");
return null;
}
const apiKey = request.headers["x-api-key"]?.toString();
if (apiKey == null) {
logger.error("Agent connection rejected - missing API key");
return null;
}
const workspaceSlug = request.headers["x-workspace"]?.toString();
if (workspaceSlug == null) {
logger.error("Agent connection rejected - missing workspace slug");
return null;
}

}

const workspace = await db.query.workspace.findFirst({
where: eq(schema.workspace.slug, workspaceSlug),
});
if (workspace == null) {
logger.error("Agent connection rejected - workspace not found");
return null;
}

const targetInfo: InsertTarget = {
name: agentName,
version: "ctrlplane/v1",
kind: "TargetSession",
identifier: `ctrlplane/target-agent/${agentName}`,
workspaceId: workspace.id,
};
const [target] = await upsertTargets(db, [targetInfo]);
if (target == null) return null;
return new AgentSocket(socket, request, target);
Comment on lines +41 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for database operations.

The database operations lack proper error handling. If the database query or upsert fails, it could throw unhandled exceptions.

Add try-catch blocks:

-    const workspace = await db.query.workspace.findFirst({
-      where: eq(schema.workspace.slug, workspaceSlug),
-    });
+    let workspace;
+    try {
+      workspace = await db.query.workspace.findFirst({
+        where: eq(schema.workspace.slug, workspaceSlug),
+      });
+    } catch (error) {
+      logger.error("Failed to query workspace", { error, workspaceSlug });
+      return null;
+    }
     if (workspace == null) {
       logger.error("Agent connection rejected - workspace not found");
       return null;
     }

     const targetInfo: InsertTarget = {
       name: agentName,
       version: "ctrlplane/v1",
       kind: "TargetSession",
       identifier: `ctrlplane/target-agent/${agentName}`,
       workspaceId: workspace.id,
     };
-    const [target] = await upsertTargets(db, [targetInfo]);
+    let target;
+    try {
+      [target] = await upsertTargets(db, [targetInfo]);
+    } catch (error) {
+      logger.error("Failed to upsert target", { error, targetInfo });
+      return null;
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const workspace = await db.query.workspace.findFirst({
where: eq(schema.workspace.slug, workspaceSlug),
});
if (workspace == null) {
logger.error("Agent connection rejected - workspace not found");
return null;
}
const targetInfo: InsertTarget = {
name: agentName,
version: "ctrlplane/v1",
kind: "TargetSession",
identifier: `ctrlplane/target-agent/${agentName}`,
workspaceId: workspace.id,
};
const [target] = await upsertTargets(db, [targetInfo]);
if (target == null) return null;
return new AgentSocket(socket, request, target);
let workspace;
try {
workspace = await db.query.workspace.findFirst({
where: eq(schema.workspace.slug, workspaceSlug),
});
} catch (error) {
logger.error("Failed to query workspace", { error, workspaceSlug });
return null;
}
if (workspace == null) {
logger.error("Agent connection rejected - workspace not found");
return null;
}
const targetInfo: InsertTarget = {
name: agentName,
version: "ctrlplane/v1",
kind: "TargetSession",
identifier: `ctrlplane/target-agent/${agentName}`,
workspaceId: workspace.id,
};
let target;
try {
[target] = await upsertTargets(db, [targetInfo]);
} catch (error) {
logger.error("Failed to upsert target", { error, targetInfo });
return null;
}
if (target == null) return null;
return new AgentSocket(socket, request, target);

}

private stdoutListeners = new Set<(data: SessionOutput) => void>();
readonly id: string;

private constructor(
private readonly socket: WebSocket,
private readonly request: IncomingMessage,
private readonly _: IncomingMessage,
public readonly target: Target,
) {
this.id = uuidv4();
this.socket.addEventListener(
this.target = target;
this.socket.on(
"message",
ifMessage()
.is(agentConnect, async (data) => {
await upsertTargets(db, [
{
...target,
config: data.config,
metadata: data.metadata,
version: "ctrlplane/v1",
},
]);
})
Comment on lines +70 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for target update.

The agentConnect handler lacks error handling for the database operation.

     .is(agentConnect, async (data) => {
+      try {
         await upsertTargets(db, [
           {
             ...target,
             config: data.config,
             metadata: data.metadata,
             version: "ctrlplane/v1",
           },
         ]);
+      } catch (error) {
+        logger.error("Failed to update target with agent data", {
+          error,
+          targetId: target.id,
+        });
+      }
     })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.is(agentConnect, async (data) => {
await upsertTargets(db, [
{
...target,
config: data.config,
metadata: data.metadata,
version: "ctrlplane/v1",
},
]);
})
.is(agentConnect, async (data) => {
try {
await upsertTargets(db, [
{
...target,
config: data.config,
metadata: data.metadata,
version: "ctrlplane/v1",
},
]);
} catch (error) {
logger.error("Failed to update target with agent data", {
error,
targetId: target.id,
});
}
})

.is(sessionOutput, (data) => this.notifySubscribers(data))
.is(agentHeartbeat, (data) => this.updateStatus(data))
.handle(),
Expand All @@ -50,15 +105,8 @@
console.log("status", data.timestamp);
}

createSession(username = "", shell = "") {
const createSession: SessionCreate = {
type: "session.create",
username,
shell,
};

this.send(createSession);

createSession(session: SessionCreate) {
this.send(session);
return this.waitForResponse(
(response) => response.type === "session.created",
);
Expand All @@ -80,7 +128,7 @@
return waitForResponse<T>(this.socket, predicate, timeoutMs);
}

send(data: SessionCreate | SessionDelete | SessionInput) {
send(data: SessionCreate | SessionDelete | SessionInput | SessionResize) {

Check failure on line 131 in apps/target-proxy/src/controller/agent-socket.ts

View workflow job for this annotation

GitHub Actions / Lint

'any' overrides all other types in this union type
return this.socket.send(JSON.stringify(data));
}
}
Expand Down
Loading
Loading