feat: use YAML for harness project configuration - #2297
Conversation
Keep YAML configuration failures user-classified, validate project prompt references before literal-domain validation, and reject FIFO sources without blocking. Build the generated CDK app before tests to prevent missing or stale executable output.
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice migration from harness.json to harness.yaml. The design is careful and the test coverage is thorough — I especially appreciated:
- The FIFO / symlink-race hardening in
readPrompt(nonblockingO_RDONLY+ post-openstat), and the subprocess-bounded test. - The YAML alias / shared-node test in
harnessConfig.test.ts("does not read misplaced prompt keys or overwrite shared YAML aliases") — the shallow-clone-on-descent in the reader threads that needle correctly. - The obsolete-JSON diagnostic in both readers, and the migration guidance in
README.mdcovering the copiedagentcore/cdkapp. - Running the same reader test suite against both the CLI copy (
src/io/harnessConfig.ts) and the vended CDK copy (src/assets/cdk/io/harnessConfig.ts), which mitigates drift between the near-duplicate files. - The
HarnessAuthoringSchemasplit that keepsfile://references out ofHarnessSpecSchemawhile still surfacing empty-file://errors before any files are written. test("reports schema errors with the YAML path before creating export output", ...)asserts export output isn't created on failure.
Nothing worth blocking on. A couple of small non-blocking notes for future consideration:
- The CLI and CDK reader files are nearly line-for-line duplicates (only the error-wrapping differs). The shared cross-runner test loop makes drift detectable, but a longer-term refactor to share source (e.g., via
@aws/agentcore-cdk) would be nicer. readFile(configPath, "utf8")has no size guard on the YAML itself (only the referenced prompt files are capped at 1 MiB). Low risk for author-controlled input, but a cheapstatguard would be consistent with the prompt-file policy.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2297 +/- ##
============================================
+ Coverage 97.20% 97.21% +0.01%
============================================
Files 607 607
Lines 40904 40988 +84
============================================
+ Hits 39760 39848 +88
+ Misses 1144 1140 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
TextDecoder removes the leading BOM by default. Set ignoreBOM alongside fatal in both readers to preserve prompt contents without weakening UTF-8 validation. Cover main, summary, and conventional prompts plus BOM-only rejection through the shared reader suite.
|
Claude Security Review: no high-confidence findings. (run) |
| outboundAuth: { awsIam: {} }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Any reason why we're not using handlebars like the other templates? I also don't see a test asserting YAML output anywhere. Without either of these, I can't tell what the generated YAML would look like.
jariy17
left a comment
There was a problem hiding this comment.
Pretty good, see if we can use yaml and jinja to simplify the code here.
| ]; | ||
|
|
||
| /** Serializes supplied values separately from inactive examples, so examples cannot become defaults. */ | ||
| export class HarnessYamlRenderer { |
There was a problem hiding this comment.
Can't we use jinja to generate the yaml file?
| ] as const; | ||
|
|
||
| /** Project-file I/O only; callers validate the resolved data with their HarnessSpecSchema. */ | ||
| export class HarnessConfigReader { |
There was a problem hiding this comment.
Can't we use a library like 'yaml' to convert the yaml into a typescript object
There was a problem hiding this comment.
nevermind, I see we need to process another file for the system prompt.
| ] as const; | ||
|
|
||
| /** Project-file I/O only; callers validate the resolved data with their HarnessSpecSchema. */ | ||
| export class HarnessConfigReader { |
There was a problem hiding this comment.
Same as the comments above.
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
| "dependencies": { | ||
| "@aws/agentcore-cdk": "0.1.0-alpha.53", | ||
| "aws-cdk-lib": "~2.266.0", | ||
| "aws-cdk-lib": "~2.269.0", |
There was a problem hiding this comment.
why you are bumping the aws-cdk-lib?
AlexanderRichey
left a comment
There was a problem hiding this comment.
This looks solid. One thing I noticed is that we removed the systemPrompt: file://system-prompt.md syntax, which I liked since it makes it explicit where the system prompt content is coming from. Can we restore that in a follow up?
Also wanted so show some examples of this kind of commented file pattern:
Summary
harness.jsonfiles with commentedharness.yamlconfiguration.yaml.stringifyfor dynamic values. A complete default-output snapshot replaces fragment-level presentation assertions.system-prompt.mdfile. InlinesystemPrompttext overrides that file for deployment and CLI export; summary prompts are inline text.Generated harness.yaml
Breaking change: Project harness configuration now uses
harness.yaml;harness.jsonis no longer supported.Release dependency: The thin app requires the CDK
refactorAPI plus CDK #383. Before shipping, publish that CDK build and sync the generated app's package pin to the release containing both; verification below uses a local refactor build, not the older CDKmain.Testing
refactorthroughde54554996bbf7f7c0be948d162cb248d753049f; 3,386 CLI tests passed, with six snapshots. Typecheck, lint, formatting, secrets checks, and build passed.agentcore project build. Separate synthesis checks confirmed fixed-file loading and inline override.aws-cdk-lib ~2.269.0, satisfying the CDK refactor's existing peer requirement.Observed service limitation: AWS runtime creation rejected a summary prompt with a trailing newline. The successful smoke run used a single-line summary; the CLI does not trim prompt contents.