fix: preserve wrapped CLI help in API docs#1826
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Package TarballHow to installgh release download pr-1826-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.24.2.tgz |
|
@strandly-the-agent review |
strandly-the-agent
left a comment
There was a problem hiding this comment.
TL;DR — changes requested: the wrapped-help fix works, but the shared renderer can corrupt Optional … prose and the new regression test is not part of any automated test target.
✅ Reviewed 642f2f5; direct Node test 1/1, Python compile, build, and end-to-end generation (6 groups / 30 entries / 139 params) passed.
🔴 format:check still fails on the new test file; CI already identifies the formatting fix.
ai-review failure occurred during runner setup before review execution; the remaining build/test/security checks pass.
Review breakdown
- Correctness/docs: one deterministic cross-source wording regression.
- Test quality: the parser regression test passes directly but is absent from package/CI test targets.
- Suppressed: the memory-strategy wording concern did not reproduce against real inputs; no other escaping or parser defect was found.
No API or model-context pass was needed: these are internal documentation-generation scripts.
| """Normalize recurring parameter-description style issues.""" | ||
| text = normalize_style(text).strip() | ||
| substitutions = ( | ||
| (r"^Optional\b", "An optional"), |
There was a problem hiding this comment.
🟡 This global rewrite breaks valid plural and mass-noun descriptions from the renderer's other supported sources. I reproduced Optional tags. → An optional tags. and Optional additional task metadata → An optional additional task metadata; every parameter reaches this rule via render_params(). Could we preserve Optional, or make the rewrite noun-aware, and add an exact renderer test for those cases plus the intended {aws} output?
|
|
||
| import { parseHelp } from "./extract-cli-model.mjs"; | ||
|
|
||
| test("parseHelp preserves wrapped argument and option descriptions", () => { |
There was a problem hiding this comment.
🟡 The regression test passes with node --test, but no repository test target runs it: the unit project only discovers src/**/*.test.ts(x), and the package test scripts invoke Vitest. I confirmed Vitest reports this file as “No test files found.” Please wire it into a CI-invoked script/workflow or convert/move it into the discovered suite so this regression remains enforced.
|
Claude Security Review: no high-confidence findings. (run) |
Summary
Addresses generated documentation feedback from Amazon CR-290947859.
Testing
node --test scripts/extract-cli-model.test.mjs