Skip to content

Add aws-ecs-operations-review skill - #42

Merged
ams-thakkar merged 4 commits into
aws:mainfrom
shyamkulkarni:feature/aws-ecs-operations-review
Sep 24, 2026
Merged

ams-thakkar merged 4 commits into
aws:mainfrom
shyamkulkarni:feature/aws-ecs-operations-review

Conversation

@shyamkulkarni

Copy link
Copy Markdown
Contributor

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:

  • Resiliency & HA (REL)
  • Observability (OBS)
  • Security (SEC)
  • Operations (OPS)
  • Performance (PERF)
  • Additional Analysis (ADD)

Key features

  • Strictly read-only — uses only describe* / list* / get* APIs (ECS, CloudWatch, IAM, Application Auto Scaling, ELB, ECR, EC2, GuardDuty, Compute Optimizer); remediations are drafted for human approval, never applied
  • 7-day CloudWatch metrics baseline per service
  • Compute-platform-aware check applicability (Fargate, Fargate Spot, EC2 ASG capacity providers, ECS Managed Instances, ECS Anywhere)
  • Per-pillar PASS/FAIL/N/A scorecards with evidence and severity
  • Recommended CloudWatch alarm thresholds for IDR onboarding
  • Prioritized, remediation-linked report artifact per service
  • Graceful degradation: non-Tier-1 access errors mark dependent checks N/A and the review continues

Contents

  • SKILL.md with frontmatter metadata (version, author, agent types)
  • README.md with usage docs and non-production disclaimer
  • CHANGELOG.md
  • references/ — checks index, per-pillar check definitions, alarm thresholds, report format
  • evals/ — functional evals (evals.json) and trigger tests (eval_queries.json)
  • .skilleval.yaml for Agent Skill Eval
  • llms.txt entry added

Testing

  • Skill structure follows the repo contribution guidelines (SKILL.md + references + evals)
  • Eval queries and functional evals included under evals/

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.
@udid-aws udid-aws added ci-sweep Temporary: force a validate-skill-evals run and removed ci-sweep Temporary: force a validate-skill-evals run labels Sep 21, 2026
Comment thread skills/ecs-operation-review/references/pillars/operations.md
Comment thread skills/ecs-operation-review/references/pillars/security.md
Comment thread skills/aws-ecs-operations-review/references/pillars/security.md Outdated
@LearningNewbie

Copy link
Copy Markdown
Contributor

Thanks for adding this ECS operations-review skill. The overall direction is valuable and the structure is genuinely solid.
Credit where due first. The skill already covers a strong operations-review baseline:

  • All 6 pillars (REL / OBS / SEC / OPS / PERF / ADD) with per-check ✓/✗/N/A grading, evidence, severity, and remediation links.
  • A strictly read-only contract : only describe*/list*/get*, remediations drafted not applied.
  • A well-formed API tier dependency chain (Tier-1 describeServices → downstream) with graceful access-limitation handling.
  • A real coverage gate + review-common crosswalk, so no pillar or check silently drops.
  • Capacity-provider depth and a recommended-CloudWatch-alarms deliverable for IDR onboarding.

Validated the skill end-to-end against two live seeded services in sandbox account:

  • Fargate (launchType-only): correct platform resolution, every seeded fault caught, inapplicable checks kept N/A (not false FAILs), access-denied handled without fabricating. Solid base.

  • EC2 + ASG capacity provider + ALB: platform resolved as EC2-CP; the capacity-provider checks that are N/A on Fargate now run and grade correctly REL12 (managed termination protection disabled) and PERF9 (targetCapacity=100, zero warm headroom) both fired on seeded faults; REL14 / OPS6 / OPS7 / SEC4 ran and passed; the LB path activated (SEC20 FAIL on an HTTP listener, REL6/8/13 graded); SEC3 caught the bridge network mode; ADD7 fired and correctly linked the MI-migration recommendation to the PERF9 headroom problem. Well-configured items (circuit breaker, multi-AZ, Container Insights, scoped SG, IMDSv2) correctly passed - not fail-all.

Coverage gaps (checks worth adding)

  • ECS Exec logging / audit trail - Today OPS3 flags whether Exec is disabled in prod, but not whether - when enabled, sessions are logged (executeCommandConfiguration.logging to CloudWatch/S3).
  • SEC5 (readonlyRootFilesystem: true) needs an ECS Exec caveat.readonlyRootFilesystem is incompatible with ECS Exec - recommending it unconditionally will break Exec where it's in use. Add a one-line caveat to the SEC5 recommendation.

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 ams-thakkar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-review custom agent enumerates four skills in its SYSTEM_PROMPT.md and ECS isn't among them, and this PR touches nothing under custom-agents/. #93 registered its skill with the agent in the same PR; worth matching, but a follow-up is fine.
  • Frontmatter author: kulkshya is an Amazon alias; the convention asks for the GitHub username (shyamkulkarni).
  • The CHANGELOG uses bare ## 2.4.0 headings 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 ams-thakkar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ams-thakkar
ams-thakkar merged commit 7787c29 into aws:main Sep 24, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants