refactor(add): swap harness to accept project schema directly instead of api shape. - #2034
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2034 +/- ##
============================================
- Coverage 97.14% 97.09% -0.05%
============================================
Files 382 382
Lines 22884 22581 -303
============================================
- Hits 22231 21926 -305
- Misses 653 655 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ? toModelConfig(inputModelConfig) | ||
| : { provider: "bedrock" as const, modelId: "global.anthropic.claude-sonnet-4-6" }, | ||
| model: parseJsonFlag("model", flags["model"]) ?? { | ||
| provider: "bedrock" as const, |
| efsAccessPoints: env?.efsAccessPoints, | ||
| s3AccessPoints: env?.s3AccessPoints, | ||
| containerUri: artifact?.containerUri, | ||
| tags: parseJsonFlag("tags", flags["tags"]), |
There was a problem hiding this comment.
- Tags differ from the shared convention
This command currently accepts only:
--tags '{"team":"ml","env":"prod"}'
Other refactor-branch commands use parseTags, allowing both:
--tags team=ml env=prod
--tags '{"team":"ml","env":"prod"}'
They declare the flag as z.array(z.string()).optional() and convert the values into Record<string, string> with parseTags.
Should we support both? Even though the flags don't match in the imperative one, we should keep the tag conventions the same between both.
There was a problem hiding this comment.
I think to be consistent, we should swap it.
notgitika
left a comment
There was a problem hiding this comment.
OOS for here but when I was playing around with it I noticed it didn't have a parameterized help like we see in harness create --help. The JSON-valued flags are otherwise difficult to construct.
the help should document the harness spec shapes specifically, since they differ from the API shapes used by harness create.
can we discuss having that as a future improvement?
| flag("max-tokens", "max total output tokens per invocation", z.number().optional()), | ||
| flag("timeout-seconds", "max duration in seconds per invocation", z.number().optional()), | ||
| flag("tags", "tags to apply (JSON object of key/value strings)", z.string().optional()), | ||
| flag("tags", "tags as key=value (repeatable) or JSON object", z.array(z.string()).optional()), |
There was a problem hiding this comment.
Could we add --connections here? HarnessSpecSchema supports it but this handler currently doesn't
There was a problem hiding this comment.
AFAICT this isn't used in the old CLI so I left it out.
Upstream moved the per-resource `project add` tests out of the monolithic project.test.ts into colocated add/<resource>/index.test.ts suites (harness in aws#2034, online-eval in aws#2048). Move the memory tests to match, with the same locally-duplicated run/inProject helpers those suites use. project.test.ts is now identical to upstream/refactor again, so this PR no longer touches it. Also drops the DeserializationError, FsReadWriteJson and ReadWriteJson imports, left dead there once the harness tests that used them moved to add/harness/index.test.ts. No test content changed: 187 project tests still pass, now across 10 files instead of 9.
Problem
The existing convention of mirroring API shapes has some issues.
update/editin the future.Solution
swap to mirroring the schemas directly. This approach is simpler (removes ~300 lines of mapping code from harness) and will extend better to agentcore project resources that have diverged from the API.
mirroring the schemas with json blobs also reduces the amount of flags significantly.
The PR also ports the harness tests to their own file.
Testing
Existing Unit Tests ported over.
Helping screen becomes:
Notes
This does introduce a discoverability gap where customers need to know our project schemas, which means we'll need solid documentation here.