Repository navigation
Add firehose-operation-review skill for DevOps Agent - #109
Conversation
…very streams against the Well-Architected Framework across seven pillars (Security, Reliability, Performance, Service Quotas, Cost Optimization, Operational Excellence, Sustainability) using Firehose, CloudWatch, and Service Quotas read APIs, and produces a severity-ranked report artifact. Wires the skill into the aws-operation-review custom agent.
github-actions
Bot
added
the
needs-evals
|
firehose-operation-review skill for DevOps Agent |
…l tool access The CI-required evals/structure|best-practices|functional layout is produced only by the internal skill-eval tool, which isn't available to this contributor (no merged skill ships that layout yet). A full aws-samples/sample-agent-skill-eval run is committed (overall 0.8994, passed). Requesting exemption pending internal tool access; will migrate to the required layout once available.
github-actions
Bot
removed
the
needs-evals
udid-aws
added
the
enforce-evals
github-actions
Bot
added
the
needs-evals
|
Hi @udid-aws , thanks for checking. Is there any issue to address? |
|
@shinutharol2ext, thank you for the contribution proposal. A few elaborations:
On the What the check asks for is the internal skill evaluation tool's output under I know all this is a bit inconvenient. The tool still isn't public, yet we want to start enforcing consistent skill evaluation, hence we found this intermediate solution until we publish the skill evaluation tool in this repo. Once the tool is public, it'll be much easier as we would public the usage guidelines in the repo, and anyone will be able to use it |
|
@udid-aws : please check. (26-09-24 22:28:00) <0> [/workplace/stharolz/git-release-folder/tools-for-devops-agent] ✓ structure: 12/12 passed, 0 skipped (score: 100%) — PASSED Best-practices evaluation complete (v11) Per-iteration results: Results: /workplace/stharolz/git-release-folder/tools-for-devops-agent/skills/firehose-operation-review/evals/best-practices/v11 ✓ Summaries: 3 eval(s), overall written Functional evaluation complete (v3) Trigger test results: Expected output test results: Assertion test results: Assertions to rewrite: Comparison test results (quality): Comparison test results (runtime): Output consistency test results: Results: /workplace/stharolz/git-release-folder/tools-for-devops-agent/skills/firehose-operation-review/evals/functional/v3 Full evaluation: PASSED |
…t with the internal devops-agent skill-eval tool output under evals/structure, evals/best-practices, and evals/functional, as required by the enforce-evals check on PR aws#109.
github-actions
Bot
removed
the
needs-evals
|
Hey — the CloudWatch metrics tables are duplicated in SKILL.md Step 3 and references/metrics-thresholds.md. Please make HTTP endpoint check flags "non-HTTPS → HIGH," but Firehose only accepts HTTPS endpoints at create time — the finding is PartitionCountExceeded is treated as a 1/0 gauge in the checklist and thresholds but as Max = 1 in metrics-thresholds. Confirm Can you please take a look at those comments and respond. |
|
Thanks for reviewing @kalyanjanaki 1/ in the current revision metrics-thresholds.md has the CloudWatch metric tables and the Iceberg-table throughput info. Skill 3 lists which metric groups to pull and points to metrics-thresholds.md as the catalog (loaded via read_skill_resource). 2/Sure I will correct as suggested for the HTTP endpoint check -- "INFO (verify HTTPS )". i will update in the next revision. 3/ "PartitionCountExceeded -> This metric indicates if you are exceeding the partition count limit. It emits 1 or 0 based on whether limit is breached or not" Hence i took it as a boolean. Ref : https://docs.aws.amazon.com/firehose/latest/dev/monitoring-with-cloudwatch-metrics.html. i hope i don't need to change it. |
|
Looks good to be merged. @ams-thakkar Please get it merged to main. |
Firehose only accepts https:// endpoint URLs (HttpEndpointConfiguration.Url pattern https://.*, enforced at create time), so a non-HTTPS endpoint is unreachable. Rephrase the checklist item from 'non-HTTPS -> HIGH' to an INFO that confirms HTTPS and refocuses on secure access-key storage and VPC/PrivateLink. Bump to v1.4.1; record review responses on metric-table duplication and PartitionCountExceeded semantics in the CHANGELOG.
|
2nd point in review -- fixed in the commit #7599a40 |
Thanks for the SME feedback. I'll contact if I have any questions. I still need review the skill evaluation tool results, so holding on with merging until I finish, and I'll post here feedback |
|
@udid-aws - any updates? |
Sorry, not yet. I have backlog of some other PRs to review, so haven't got to yours yet |
|
@udid-aws - any updates? |
…facts Replace eight real AWS account IDs that leaked into the functional eval run artifacts under evals/functional/ with the standard placeholder 123456789012. The IDs appeared in CloudFormation stack ARNs and the captured agent journals/outputs from the environment that produced the run. Also stop tracking the per-run _metadata.json (Agent Space IDs, stack ARNs, environment account IDs): add a gitignore rule for functional/*/_metadata.json and remove the committed file from the index. The file stays on disk locally. All affected JSON remains valid.
|
1/ Replaced eight AWS account IDs that was in the functional eval run artifacts under evals/functional/ with the standard placeholder 123456789012. 2/Also stopped adding the per run |
ams-thakkar
left a comment
ams-thakkar
left a comment
There was a problem hiding this comment.
What needs to change
1. Add one eval covering the v1.4.1 severity change, then re-run the functional suite.
The substance of this PR is the HTTP-endpoint check moving HIGH → INFO, and nothing in the eval suite touches it: grep -ci on evals/evals.json returns 0 for both HttpEndpoint and https. I am being asked to accept a severity reclassification on the strength of a CHANGELOG argument alone, and one eval pins it permanently.
Write it recall-style, like firehose-review-severity-definitions. That matters: the three recall-shaped evals graded 3/3 on every iteration, while half the runs of the one behavioural eval came back ungradable (more on that below), so this framing is the one that will actually produce a verdict. Something close to:
{
"id": "firehose-review-http-endpoint-severity",
"task_type": "chat",
"prompt": "Using the Firehose operational review skill, what severity does it assign to the HTTP endpoint destination check, and why doesn't it flag a non-HTTPS endpoint URL? No account access required.",
"should_trigger": true,
"expected_output": "Identifies the HTTP endpoint destination check as INFO, explains that Firehose only accepts https:// endpoint URLs so a non-HTTPS endpoint is not a reachable state to flag, and redirects the check to secure storage and rotation of the endpoint access key plus a VPC/PrivateLink path for private or cross-account endpoints.",
"assertions": [
"The HTTP endpoint destination check is identified as INFO severity",
{ "text": "INFO is named", "evaluator": "regex", "pattern": "INFO" },
"The answer explains that Firehose only accepts https:// endpoint URLs, so a non-HTTPS endpoint is not a reachable state to flag",
"The answer redirects the check to secure storage or rotation of the endpoint access key",
"The answer mentions a VPC or PrivateLink path for private or cross-account endpoints"
]
}Only the functional results need regenerating (v3 → v4) — the structure and best-practices runs are versioned independently and are about the skill files, not the eval set, so leave those alone.
2. llms.txt has no entry for this skill. Every peer has exactly one line — grep -c returns 1 for aws-health-events, database-rds-devops and bedrock-adoption-readiness, and 0 for firehose-operation-review. Add it to the Available Skills section in the existing - [Name](skills/<name>/SKILL.md): description format.
Nits
- The PR description still describes the pre-
7599a40daHTTP-endpoint behaviour, so the merge commit would read as if the check were still HIGH. Non-blocking. - Trigger-eval assertion 4 reads "DataFreshness or delivery-lag", but the grader appears to want the literal CamelCase token. Non-blocking, and more a grader issue than yours.
Thanks
Nice work on v1.4.1. The HTTP-endpoint correction is the right call, and the CHANGELOG explains why the old finding was unreachable — the HttpEndpointConfiguration.Url API pattern is https://.*, so a non-HTTPS endpoint cannot be configured in the first place — rather than just asserting the severity change. Asking for an eval on it is about locking that reasoning in, not doubting it. The negative evals are the strongest part of the suite: five of them, 3/3 each, and two are genuinely hard near-misses (tuning Kinesis Data Streams shards, and "review my Amazon Bedrock workload for best practices" — same verb, same framing, adjacent service).
I have taken over the skill-evaluation review from @udid-aws. What I verified myself at 27223fa7f:
- Identifier scanner: exit 0, 0 findings, down from 50 before the scrub in
04d1de41b. Eval validator: PASS under enforced--skill, withexemptions.jsondeleted andenforce-evalsapplied, so nothing is being waived. - All three result sets come from one session on 2026-09-24 (22:28 to 22:38Z). Structure 100. Best-practices consistency 88 with all three iterations at 100, zero failed, zero errored.
mkdocs build --strictclean,version: "1.4.1"matches the CHANGELOG,_metadata.jsoncorrectly untracked. - The apparent scope regression in the trigger eval is not real. Assertion 5 reads with_skill 0/2 against without_skill 1/1, so I read the journal records. Both with-skill answers state region, stream count and account inline; the baseline answer that passed states region and stream count but never the account. The grader marked the stronger answers false, at
mediumconfidence on both arms. Assertion 4 is the same story — the iteration-2 with-skill answer writesDataFreshnessverbatim and still graded false. Nothing for you to fix in either. - The one legitimate failure is assertion 2 in iteration 1, where the chat summary is ordered by severity and the per-pillar tables went into the saved report artifact instead. Defensible behaviour, not a defect.
- Only 3 of the 6 trigger runs graded at all — the
2/2and1/1denominators are the knownextract_final_responsebug, where a complete answer in atextblock scores as "no final response". Not this PR's problem, but it is why I am steering the new eval toward the recall shape. - I checked the
without_skillarm for contamination after seeing aload_skillrecord on the baseline side offirehose-review-pillarsiteration 1. It was a failed attempt —Skill not found: firehose-operational-review, the agent guessingoperationalwhere the skill isoperation— so the baseline is clean. - On @kalyanjanaki's third point: the Firehose CloudWatch metrics doc says
PartitionCountExceeded"emits 1 or 0 based on whether limit is breached or not". The skill's= 1comparison is correct as written, and it already usesPartitionCountseparately where it needs the count. No change needed — noting it so nobody corrects it later on the strength of that comment.
Add the eval and the llms.txt line and I will merge.
…s, llms.txt entry Address PR aws#109 review feedback: - Add firehose-review-http-endpoint-severity functional eval pinning the v1.4.1 HTTP-endpoint HIGH -> INFO reclassification (recall-style) - Regenerate functional eval results (v4) against the updated eval set - Add the missing firehose-operation-review entry to llms.txt - Record the above under the 1.4.1 CHANGELOG entry and correct its date to the actual landing date (2026-09-29)
|
Thank you for the detailed review @ams-thakkar. Your feedback addressed in the latest commit. 1 - Addressed the two review items (commit 860242d, just pushed):
2 - Ran the eval tool:
|
ams-thakkar
left a comment
ams-thakkar
left a comment
There was a problem hiding this comment.
Approving. Both items landed, and the eval does exactly what I asked it to.
What needs to change
Nothing blocking.
Nits
- One follow-up that is not yours to fix: the eval journals carry some IAM principal identifiers that the repo's identifier scanner does not currently look for, which is why it passed clean here and why your earlier scrub in
04d1de41bhad no reason to catch them. A maintainer-side cleanup will take care of it along with a scanner improvement, so the next contributor does not hit the same gap. Nothing for you to do. - The trigger eval still loses one run per arm to the
extract_final_responsebug, so its numbers remain the soft ones in the set. Also not yours.
Thanks
Good turnaround, and you added the eval as specified rather than approximating it — same recall-style framing as firehose-review-severity-definitions, which is the thing that made it gradable. You also caught the 1.4.1 date being wrong (09-24 → 09-29) and dated the follow-up entry separately, which I had not asked for, and kept v3 alongside v4 so the earlier run stays auditable.
What I verified myself at 860242dce:
- The new eval works.
firehose-review-http-endpoint-severitygrades 15/15 with the skill against 0/15 without, 3/3 on every assertion with all three iterations graded and nothing ungradable. I read the journal rather than trusting the score: the answer names INFO, explains that theHttpEndpointConfiguration.Urlpattern ishttps://.*so a non-HTTPS endpoint is not a reachable state, and redirects to access-key storage and the VPC/PrivateLink path. The baseline arm fails honestly — it reports no such skill configured — so the separation is real and not an artifact. The v1.4.1 reclassification is now pinned. - The re-run settled the trigger-eval question retrospectively, in your favour. With no change to the skill, assertion 4 went 0/2 to 2/2 at
highconfidence, and assertion 5's apparent scope regression inverted outright — the skill arm now passes and the baseline now fails. That is better evidence than my reading of the journals last pass, and it confirms neither was a defect. Trigger overall moved 7/12 to 9/12; aggregate with-skill went 90% to 95%, against 13% without. - Everything else held:
pillarsstill 21/21,severity-definitionsstill 18/18, all five negative evals still 3/3 on non-trigger. Structure and best-practices results untouched, as intended. llms.txtentry is in the Available Skills section with an accurate description. Eval validator PASS under enforced--skill, no exemptions file. Identifier scanner exit 0. Merges onto currentmainwith zero conflicts,mkdocs build --strictzero warnings, and the generated catalog lists the skill among 34.
Merging now. Thanks for staying with this one through three passes.
Description
Add a read-only Amazon Data Firehose operational-review skill that assesses delivery streams against the AWS Well-Architected Framework across seven pillars — Security, Reliability, Performance, Service Quotas, Cost Optimization, Operational Excellence, and Sustainability — producing severity-rated findings (CRITICAL/HIGH/MEDIUM/LOW/INFO) and a consolidated report artifact (firehose-review---.md). Discovers streams and config via firehose:List/Describe*, pulls AWS/Firehose CloudWatch metrics and DescribeAlarms, and compares observed throughput against servicequotas:GetServiceQuota and the *PerSecondLimit metrics. Six-step workflow (scope → discovery → metrics → analysis → report → findings-over-time diff on recurring runs).
This is the Firehose domain of the operation-review skill family and plugs into the existing aws-operation-review custom agent. It handles per-destination correctness for S3, Redshift, OpenSearch (Service/Serverless), Splunk, HTTP endpoint, Snowflake, and Iceberg — including the Iceberg "one JSON object per record" landing caveat (delivery metrics + S3 error prefix are necessary-but-not-sufficient; reconcile IncomingRecords vs. actual table rows), Redshift COPY failure modes, and dynamic-partitioning key-extraction failures. Judgment thresholds are single-sourced and concrete (e.g. "sustained" = ≥3 consecutive datapoints; "stalled" = non-decreasing DataFreshness across ≥6×5-min datapoints; quota Warning/Critical = 75%/90%) so runs are consistent; security findings use objective signals (data-classification tags, factual IAM *-scope statements) rather than subjective judgment. Strictly read-only — no data-plane record puts, no reading of delivered content or transform Lambda code, no mutating calls.
Evals
evals.json + eval_queries.json committed, plus a full aws-samples/sample-agent-skill-eval run (audit 96/100 Grade A, functional Grade B, trigger 100% 5/5, overall 0.8994, passed: true) in evals/. The validate-skill-evals layout (evals/structure|best-practices|functional/) is produced only by the internal skill-eval tool, which isn't available to me yet — no merged skill in the repo currently ships that layout — so exemptions.jsonrequests a temporary exemption for those three types, to be migrated once the internal tool is available. Requesting maintainer review of the exemption.
Description
Type of change
Testing
License confirmation