-
Notifications
You must be signed in to change notification settings - Fork 72
feat(project): implement add functionality. #2004
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
base: refactor
Are you sure you want to change the base?
Changes from all commits
7dc2d57
b575a30
bb093ca
d9138cd
53f7b5d
a80d21c
efd9c85
8025178
fae3141
77b169a
8722d84
ec740c6
98c9fd8
ee86add
10d631a
22444ca
52c734c
05f3268
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| import { ZodError, z } from "zod"; | ||
| import { PROJECT_TEMPLATES, type ProjectTemplate } from "../../handlers/project/types"; | ||
| import { HarnessSpecSchema } from "../../projectSchemas/harness"; | ||
| import { FsTreeNode } from "./fsTree"; | ||
| import type { AssetSource } from "./source"; | ||
| import { InputValidationError } from "../../errors/errors"; | ||
|
|
||
| type TemplateSpec = { | ||
| runtimes?: unknown[]; | ||
|
|
@@ -90,3 +93,30 @@ export async function createProjectTreeFromTemplate( | |
| FsTreeNode.createDirectory("app", [await FsTreeNode.fromAssetSource(src, assetDir, appDir)]), | ||
| ]); | ||
| } | ||
|
|
||
| const DEFAULT_HARNESS_SYSTEM_PROMPT = "You are a helpful assistant"; | ||
|
|
||
| export async function createHarnessTreeFromSpec( | ||
| spec: z.input<typeof HarnessSpecSchema>, | ||
| ): Promise<FsTreeNode> { | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const { systemPrompt, ...rest } = spec; | ||
| // strip system prompt such that markdown file is source of truth. | ||
| const parsed = parseHarnessSpec(rest); | ||
| return FsTreeNode.createDirectory(".", [ | ||
| FsTreeNode.createFile("harness.json", async () => json(parsed)), | ||
| FsTreeNode.createFile( | ||
| "system-prompt.md", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we making the changes on the cdk side as well? what we are losing here is that there is no way a user will be warned if they make changes in their local md file here if they don't update the value from harness.json.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, good callout, was not aware of that behavior. I feel like having two sources of system prompts might be unnecessary and confusing if we scaffold a |
||
| async () => spec.systemPrompt ?? DEFAULT_HARNESS_SYSTEM_PROMPT, | ||
| ), | ||
| ]); | ||
| } | ||
|
|
||
| function parseHarnessSpec(spec: z.input<typeof HarnessSpecSchema>) { | ||
| try { | ||
| return HarnessSpecSchema.parse(spec); | ||
| } catch (err) { | ||
| if (err instanceof ZodError) throw new InputValidationError(z.prettifyError(err)); | ||
| throw err; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,7 @@ export type CoreFetch = ( | |
| // full ClientConfig so callers can request any client customization (region, | ||
| // endpoint, ...). | ||
| export interface AwsClients { | ||
| control(config: ClientConfig): BedrockAgentCoreControlClient | ||
| control(config: ClientConfig): BedrockAgentCoreControlClient; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps a rebase issue, but this is failing ci on main https://github.com/aws/agentcore-cli/actions/runs/31832012548/job/94869692163. |
||
| data(config: ClientConfig): BedrockAgentCoreClient; | ||
| iam(config: ClientConfig): IAMClient; | ||
| // logs reads the CloudWatch Logs streams AgentCore writes batch-evaluation | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,9 @@ const TRACE: SessionTrace = { | |
| const RESULT: EvaluateResult = { | ||
| sessionsRequested: 1, | ||
| sessionsEvaluated: 1, | ||
| results: [{ evaluatorId: "Builtin.Helpfulness", value: 0.9 } as EvaluateResult["results"][number]], | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above. |
||
| results: [ | ||
| { evaluatorId: "Builtin.Helpfulness", value: 0.9 } as EvaluateResult["results"][number], | ||
| ], | ||
| }; | ||
|
|
||
| async function run(args: string[], configure?: (core: TestCoreClient) => void) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,15 @@ import type { | |
| AuthorizerConfiguration as SdkAuthorizerConfiguration, | ||
| HarnessEnvironmentArtifact, | ||
| HarnessEnvironmentProviderRequest, | ||
| HarnessGatewayOutboundAuth as SdkHarnessGatewayOutboundAuth, | ||
| HarnessMemoryConfiguration as SdkMemoryConfiguration, | ||
| HarnessModelConfiguration, | ||
| HarnessSkill as SdkHarnessSkill, | ||
| HarnessTool as SdkHarnessTool, | ||
| HarnessTruncationConfiguration as SdkTruncationConfiguration, | ||
| } from "@aws-sdk/client-bedrock-agentcore-control"; | ||
| import type { | ||
| HarnessGatewayOutboundAuth, | ||
| HarnessMemoryRef, | ||
| HarnessModel, | ||
| HarnessSkill, | ||
|
|
@@ -82,6 +84,11 @@ export const createAddHarnessHandler = (config: AddProjectResourceConfig) => | |
| "path to local dockerfile to use as the container image for the harness", | ||
| z.string().optional(), | ||
| ), | ||
| flag( | ||
| "vpc-id", | ||
| "VPC ID for Dockerfile builds in VPC mode (required when combining --dockerfile with VPC networking)", | ||
| z.string().optional(), | ||
| ), | ||
| ], | ||
| handle: async (ctx, flags) => { | ||
| if (!flags.name) | ||
|
|
@@ -136,7 +143,9 @@ export const createAddHarnessHandler = (config: AddProjectResourceConfig) => | |
| timeoutSeconds: flags["timeout-seconds"], | ||
| tags: parseJsonFlag<Record<string, string>>("tags", flags["tags"]), | ||
| networkMode: env?.networkMode, | ||
| networkConfig: env?.networkConfig, | ||
| networkConfig: env?.networkConfig | ||
| ? { ...env.networkConfig, vpcId: flags["vpc-id"] } | ||
| : undefined, | ||
| lifecycleConfig: env?.lifecycleConfig, | ||
| sessionStoragePath: env?.sessionStoragePath, | ||
| efsAccessPoints: env?.efsAccessPoints, | ||
|
|
@@ -146,11 +155,10 @@ export const createAddHarnessHandler = (config: AddProjectResourceConfig) => | |
| }; | ||
|
|
||
| const project = ctx.require(ProjectKey); | ||
| for await (const event of config.projectManager.addResource( | ||
| project, | ||
| "harness", | ||
| harnessConfig, | ||
| )) { | ||
| for await (const event of config.projectManager.addResource(project, { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is currently no way to add a Dockerfile-backed harness in VPC mode. The environment conversion carries subnets and security groups, but the harness schema also requires
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow great edge case find, was not aware vpc id was required with VPC + docker. Let me add the explicit flag for this and make it clear. |
||
| resourceType: "harness", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One part of the requested tool configuration gets lost before this call. For an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think @notgitika spotted the same in #1998 (comment). Was going to address as follow-up, but let me bring in here. |
||
| resourceConfig: harnessConfig, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Private Git skill auth has a field mismatch here. The SDK input gives us a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think adding it as a new field makes sense (with them being mutually exclusive). That allows us to support credentials from outside the project with minimal extra effort. We can comeback to supporting credentialName since its already in the schema. |
||
| })) { | ||
| config.io.stderr.write(`${event.message}\n`); | ||
| } | ||
|
|
||
|
|
@@ -233,6 +241,9 @@ function toTool(tool: SdkHarnessTool): HarnessTool { | |
| config: { | ||
| agentCoreGateway: { | ||
| gatewayArn: requireField(c.agentCoreGateway.gatewayArn, "agentCoreGateway.gatewayArn"), | ||
| outboundAuth: c.agentCoreGateway.outboundAuth | ||
| ? toOutboundAuth(c.agentCoreGateway.outboundAuth) | ||
| : undefined, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
@@ -263,6 +274,27 @@ function toTool(tool: SdkHarnessTool): HarnessTool { | |
| return { type: tool.type, name: tool.name }; | ||
| } | ||
|
|
||
| /** Converts an SDK HarnessGatewayOutboundAuth tagged union into the project-schema shape. */ | ||
| function toOutboundAuth(auth: SdkHarnessGatewayOutboundAuth): HarnessGatewayOutboundAuth { | ||
| if ("awsIam" in auth && auth.awsIam) return { awsIam: {} }; | ||
| if ("none" in auth && auth.none) return { none: {} }; | ||
| if ("oauth" in auth && auth.oauth) { | ||
| return { | ||
| oauth: { | ||
| providerArn: requireField(auth.oauth.providerArn, "outboundAuth.oauth.providerArn"), | ||
| scopes: requireField(auth.oauth.scopes, "outboundAuth.oauth.scopes"), | ||
| // SDK does not expose this type directly. | ||
| grantType: auth.oauth.grantType as Extract< | ||
| HarnessGatewayOutboundAuth, | ||
| { oauth: unknown } | ||
| >["oauth"]["grantType"], | ||
| customParameters: auth.oauth.customParameters, | ||
| }, | ||
| }; | ||
| } | ||
| throw new InputValidationError("unrecognized outboundAuth variant"); | ||
| } | ||
|
|
||
| /** Converts an SDK HarnessSkill tagged union into the project-schema shape. */ | ||
| function toSkill(skill: SdkHarnessSkill): HarnessSkill { | ||
| if ("path" in skill && skill.path) { | ||
|
|
@@ -277,7 +309,7 @@ function toSkill(skill: SdkHarnessSkill): HarnessSkill { | |
| path: skill.git.path, | ||
| auth: skill.git.auth | ||
| ? { | ||
| credentialName: requireField( | ||
| credentialArn: requireField( | ||
| skill.git.auth.credentialArn, | ||
| "skill.git.auth.credentialArn", | ||
| ), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we either implement the runtime branch here or leave runtime out of
AddResourceInputfor now? At the moment a runtime call falls through this switch, emits theUpdating project specmessage, writes the unchanged runtime list, and returns successfully. That silent success will be hard for callers to diagnose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine right? assuming we are implementing
add runtimeright after this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to be the next PR, but understand the current behavior is confusing. I'll have it throw early.