Add aws-ecs-operations-review skill - #42
Conversation
Add a comprehensive Amazon ECS operations review skill covering 6 review pillars (Resiliency & HA, Observability, Security, Operations, Performance, Additional Analysis) using read-only AWS APIs. Includes a 7-day CloudWatch metrics baseline, per-pillar PASS/FAIL/N/A scorecards, recommended alarm thresholds for IDR onboarding, and a prioritized remediation-linked report artifact. Ships with references, evals, and skill-eval config per the contribution guidelines.
|
Thanks for adding this ECS operations-review skill. The overall direction is valuable and the structure is genuinely solid.
Validated the skill end-to-end against two live seeded services in sandbox account:
Coverage gaps (checks worth adding)
NOTE:When I ran this, the agent's conversational summary compressed a few confirmed ✗ findings (PERF9, SEC3, ADD7) that were correctly graded in the scorecard. That's likely just chat-summarization, not the report artifact - but worth confirming the Prioritized action plan lists every ✗ (it already says it should). If it does, ignore this. |
ams-thakkar
left a comment
There was a problem hiding this comment.
What needs to change
compute-optimizer is not granted by the DevOps Agent's managed policy, and this PR adds no policy for it. references/ and SKILL.md call computeoptimizer.getECSServiceRecommendations. I parsed all 912 actions in the live AIDevOpsAgentAccessPolicy v10 and there is no compute-optimizer entry of any kind — every other API this skill uses is covered (ecs:Describe*, ec2:Describe*, elasticloadbalancing:Describe*, ecr:Describe*, application-autoscaling:Describe*, guardduty:GetDetector + List*, logs:Describe*, cloudwatch:Describe* + GetMetricStatistics, and the three iam: role-policy reads). So that check will return AccessDenied under the agent's own role. Per step 9 of the contributing conventions it needs a gated parameter, condition, and inline policy in cloudformation/devops-agent-skill-policies.yaml plus a SkillPolicySummary line — sagemaker-ops-review in #93 is the pattern for a single added action. Alternatively drop the check. Relatedly, README.md:46 says the AWS managed ReadOnlyAccess policy "is sufficient" — true, but it's the wrong frame for this repo and far broader than least privilege; every other skill states coverage against AIDevOpsAgentAccessPolicy and gates any gap.
The strict docs build fails. deploy-docs.yml:41 runs mkdocs build --strict. Building main with only this skill overlaid:
WARNING - Doc file 'skills/aws-ecs-operations-review.md' contains a link 'evals/TESTING.md', but the target 'skills/evals/TESTING.md' is not found among documentation files.
WARNING - Doc file 'skills/aws-ecs-operations-review.md' contains a link 'references/report-format.md', but the target 'skills/references/report-format.md' is not found among documentation files.
Aborted with 2 warnings in strict mode!
Both are the relative .md links at README.md:104 and README.md:108; no other warnings, so main is clean today and this is the change that breaks the deploy. Linking to the directory rather than the file avoids it, which is why merged skill READMEs carry no relative .md links.
@LearningNewbie's three findings are all still open. The only commit on this branch, db7403e2, is dated 9 August — six weeks before the 23 September review — and nothing has been pushed since. I re-checked each against the current head and all three reproduce: references/pillars/operations.md:1 reads # Pillar: Operations (OPS1-OPS7) while the table defines OPS8, and checks.md:35,54 plus SKILL.md:51,76 all say OPS1-OPS8; references/pillars/security.md:1 reads SEC1-SEC19 while the table defines SEC20, against SEC1-SEC20 in the same four places. On the third, security.md:29 marks SEC19 GuardDuty Runtime Monitoring Applies To: All, and the skill does model Managed Instances as a distinct applicability value — MI is declared in CHANGELOG.md:13, used in resiliency.md:24, and operations.md:7 already excludes MI for OPS6/OPS7 — so the vocabulary to express the exclusion exists and SEC19 isn't using it. I'm not independently confirming the GuardDuty/MI support boundary; that's @LearningNewbie's call as the domain reviewer, and it needs an explicit answer rather than being left as All.
Frontmatter version and CHANGELOG disagree. SKILL.md declares version: "1.0.0"; the CHANGELOG's top entry is ## 2.4.0. They have to match.
Settle the skill name before this merges. aws-ecs-operations-review diverges from the family it joins on two axes: none of eks-operation-review, rds-operation-review, bedrock-operation-review, or agentcore-ops-review carries an aws- prefix, and all use the singular. ecs-operation-review would match. The directory name is also the frontmatter name, the llms.txt path, and the published docs URL, so renaming after merge is churn across all three — cheap to settle now.
Nits
Non-blocking, fine as follow-ups:
- The
aws-operation-reviewcustom agent enumerates four skills in itsSYSTEM_PROMPT.mdand ECS isn't among them, and this PR touches nothing undercustom-agents/. #93 registered its skill with the agent in the same PR; worth matching, but a follow-up is fine. - Frontmatter
author: kulkshyais an Amazon alias; the convention asks for the GitHub username (shyamkulkarni). - The CHANGELOG uses bare
## 2.4.0headings where the convention and other skills use## [x.y.z] - YYYY-MM-DD.
Thanks
The check design is the strongest part of this. Six pillars with stable IDs, a common-checks-coverage.md crosswalk that actually demonstrates the review-common baseline is covered rather than asserting it, and a tiered call graph where iam.getRolePolicy (Tier 4b) only fires after the Tier 4a policy listing tells you there's an inline policy to fetch — that's real thought about call volume, not a flat list of APIs. Modelling Managed Instances as a first-class applicability value alongside Fargate and EC2-ASG-CP, and marking OPS2/OPS6/OPS7 N/A per platform, is the kind of precision that keeps a review from emitting confident nonsense. Requiring an Access Limitations section listing every check that couldn't be evaluated is the right failure mode, and README.md:5 committing to no mutating calls with remediations drafted for human approval is the right boundary.
Verified myself: merges clean into main with no conflicts; the full API surface against live AIDevOpsAgentAccessPolicy v10 as described above; name matches the directory and the description is 591 characters; all three JSON files parse and .skilleval.yaml parses; extensions are limited to md/json/yaml; llms.txt:26 carries the entry; the two docs-build warnings and the version mismatch quoted above; and each of the three review findings reproducing at head.
On evals: this ships evals.json, eval_queries.json, TESTING.md and a fixture, but not the structure/, best-practices/, functional/ results layout, so the check reports WARN and exits 0 under PRS_PREDATING_CHECK while failing on merit. That waiver is legitimate given this opened in August, and I'm not asking for a backfill since no skill on main has the full layout yet — noting it so the decision is visible.
My review is scoped to repository mechanics, the IAM surface, and the read-only boundary; ECS correctness is @LearningNewbie's. Happy to re-review once the compute-optimizer permission, the docs build, the version, and the three open findings are sorted, and the naming is settled.
- Add OPS9 ECS Exec audit logging check (70 baseline checks, up from 69): when enableExecuteCommand is true, grade the cluster's executeCommandConfiguration.logging via ecs.describeClusters with include=[CONFIGURATIONS]; N/A when Exec is disabled - Narrow SEC19 GuardDuty Runtime Monitoring applicability to Fargate/EC2 - Runtime Monitoring does not support ECS Managed Instances; add the MI N/A rule to the compute-platform note - Add SEC5 caveat: readonlyRootFilesystem is incompatible with ECS Exec, note the tradeoff where Exec is in use - Fix pillar headers to match their tables: operations.md OPS1-OPS7 -> OPS1-OPS9, security.md SEC1-SEC19 -> SEC1-SEC20
- Rename aws-ecs-operations-review -> ecs-operation-review to match the skill family convention (no aws- prefix, singular operation); directory, frontmatter name, and llms.txt entry updated - Gate the Compute Optimizer permission in CloudFormation: AIDevOpsAgentAccessPolicy grants no compute-optimizer actions, so add EnableEcsOperationReview parameter, condition, inline policy with compute-optimizer:GetECSServiceRecommendations, and a SkillPolicySummary line; reframe the README permissions section around AIDevOpsAgentAccessPolicy coverage instead of ReadOnlyAccess - Fix the strict docs build: remove the two relative .md links in README.md that fail mkdocs build --strict - Align frontmatter version with the changelog (2.5.0) and set author to the GitHub username - Adopt the [x.y.z] - YYYY-MM-DD changelog heading convention Also merges upstream/main to pick up the current policy template and docs pipeline.
ams-thakkar
left a comment
There was a problem hiding this comment.
All five items are resolved at 10172e41. The compute-optimizer gap now has a gated inline policy (EnableEcsOperationReview → SkillEcsOperationReview → compute-optimizer:GetECSServiceRecommendations, read-only, SkillPolicySummary updated), and README.md is reframed against AIDevOpsAgentAccessPolicy with PERF8 degrading to N/A when the policy isn't attached — the ReadOnlyAccess recommendation is gone. The rename to ecs-operation-review is consistent across the directory, frontmatter name, llms.txt, and the zip command, and it now matches the eks/rds/bedrock family. Nice touch documenting the rename in the CHANGELOG.
@LearningNewbie's three findings are all closed: operations.md:1 reads OPS1-OPS9 against nine defined IDs, security.md:1 reads SEC1-SEC20 against twenty, both agreeing with checks.md and SKILL.md in all four places, and SEC19 is now Applies To: Fargate, EC2 with an explicit "N/A for Managed Instances" note. Version is 2.5.0 in both frontmatter and the CHANGELOG top, and the CHANGELOG picked up the bracketed-and-dated heading format and the GitHub username along the way.
Verified: merges clean with zero conflicts; the CFN template parses with the parameter, condition, and conditioned resource all present and the action read-only; mkdocs build --strict passes with zero warnings, where the previous revision aborted on two; all three JSON files and .skilleval.yaml parse; extensions limited to md/json/yaml; description 591 characters; validate-skill-evals passes as WARN under the predating-PR waiver.
Approving as scoped maintainer approval — repository mechanics, the IAM surface against live AIDevOpsAgentAccessPolicy v10, and the read-only boundary. ECS correctness, including the GuardDuty Managed Instances boundary, rests on @LearningNewbie's domain review. One follow-up whenever convenient: registering this skill with the aws-operation-review custom agent, which still enumerates four skills without ECS.
Summary
Adds a new ECS Operations Review skill (
skills/aws-ecs-operations-review) that performs a comprehensive Amazon ECS operations review across 6 review pillars:Key features
describe*/list*/get*APIs (ECS, CloudWatch, IAM, Application Auto Scaling, ELB, ECR, EC2, GuardDuty, Compute Optimizer); remediations are drafted for human approval, never appliedContents
SKILL.mdwith frontmatter metadata (version, author, agent types)README.mdwith usage docs and non-production disclaimerCHANGELOG.mdreferences/— checks index, per-pillar check definitions, alarm thresholds, report formatevals/— functional evals (evals.json) and trigger tests (eval_queries.json).skilleval.yamlfor Agent Skill Evalllms.txtentry addedTesting
evals/