Conversation
…arkdown lint Replace xml.etree.ElementTree with defusedxml.ElementTree in all Python scripts to address Bandit B314/B405 and Semgrep XXE findings. Add nosec suppression to subprocess import in drawio_url.py (browser URL opener only). Fix 106 markdownlint errors (MD022/MD031/MD032 blank lines, MD001 heading levels) across 7 reference files and SKILL.md. Trim SKILL.md from 515 to 495 lines to satisfy the 500-line SKILL001 limit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mayakost
requested review from
a team,
krokoko,
scottschreckengaust and
theagenticguy
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
mayakost
changed the title
Move detailed XML generation rules and layout guidelines from SKILL.md (494 lines) into references/xml-generation-rules.md and references/layout-guidelines.md. SKILL.md is now 219 lines, well under the 300-line warning threshold enforced by SKILL001.
Run dprint fmt on 7 files (JSON and Markdown) that were not conforming to the project's formatting rules.
…sues - Replace defusedxml with stdlib xml.etree.ElementTree in all 5 Python scripts and validate-drawio.sh (XML is self-generated, no XXE risk) - Remove pip3 auto-install from PostToolUse hook - Remove unused import sys in fix_step_badges.py - Remove unused style variable in post_process_drawio.py fix_legend_size - Add explanatory comment to except block in post_process_drawio.py
…ples Delete 5 files (-2039 lines): - example-diagrams.md (803 lines): outdated XML examples conflicting with .drawio refs - aws-diagram-guidelines.md (536 lines): internal wiki copy, all rules in style-guide.md - xml-structure.md (209 lines): merged unique parts into group-styles.md + layout-guidelines.md - xml-generation-rules.md (186 lines): trimmed and renamed to xml-rules.md (89 lines) - aws4-shapes.md (231 lines): split into aws4-shapes-services.md (69) + aws4-shapes-resources.md (100) All reference files now under 100-line DESIGN_GUIDELINES limit. All shape names preserved in the split files (zero shape deletions). All cross-references updated to point to new file names.
- Remove eager "Study" paragraph that loaded ~135k tokens upfront - Move reference loading into Step 3 where references are actually needed - Add example selection table: agent picks 1-2 most relevant .drawio examples per diagram type (reduces ~95k to ~10-18k tokens) - Update cross-references to renamed files (xml-rules.md, aws4-shapes-services.md) - SKILL.md: 226 lines (under 300 limit)
…s for dark mode - Remove background="#FFFFFF" from mxGraphModel XML template in SKILL.md - Change rule from "ALWAYS set background=#FFFFFF" to "NEVER set background" - Change edge label rule from labelBackgroundColor=#ffffff to labelBackgroundColor=none in layout-guidelines.md and xml-rules.md - Both white backgrounds break dark mode adaptive contrast
- New references/general-icons.md: maps 40+ non-AWS technologies (PostgreSQL, Docker, Kubernetes, CoreML, HuggingFace, GitHub, etc.) to draw.io shapes with category-appropriate tint colors - Mode A codebase analysis now detects non-AWS tech when no AWS infrastructure files are found (Dockerfiles, DB configs, ML frameworks) - Mixed architectures supported: AWS icons for AWS services, general icons for non-AWS, same layout rules for both - Step 3 loads general-icons.md only when non-AWS services are present
PR Review FindingsI reviewed this PR and found 8 issues + 1 additional dark mode bug. All have been addressed in 5 new commits on this branch. Findings Addressed
Additional Fix
New Feature
Commits
|
Keep all new plugins from main (amazon-location-service, aws-amplify, aws-serverless, databases-on-aws, migration-to-aws) and apply our diagram skill additions to deploy-on-aws (description, keywords, tags, version bump to 1.2.0).
There was a problem hiding this comment.
Semgrep OSS found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
- Restore defusedxml.ElementTree imports (CI scanners require it) - Replace silent pip3 auto-install with informative error message when defusedxml is missing — no more modifying user's environment - Add scripts/requirements.txt declaring defusedxml>=0.7.1 - Create plugin README.md per DESIGN_GUIDELINES (prerequisites, skills, MCP servers, installation, examples) - Document defusedxml dependency in post-processing.md - Fix 20 markdownlint errors (MD022, MD029, MD031, MD032, MD060) - Apply dprint formatting to table separators
plugins/deploy-on-aws/skills/aws-architecture-diagram/references/general-icons.md
Dismissed
Show dismissed
Hide dismissed
Update: PR Review Fixes + Dependency DocumentationAll 8 review findings have been addressed, along with additional improvements discovered during testing. Review Findings Fixed
defusedxml Dependency HandlingThe CI pipeline (bandit B314 / semgrep) requires What changed: The hook no longer silently runs python3 -c "import defusedxml" 2>/dev/null || {
echo '{"systemMessage": "Missing required dependency: defusedxml. Install it with: pip3 install defusedxml>=0.7.1"}'
exit 0
}The dependency is documented in three places:
Additional Improvements
|
There was a problem hiding this comment.
Pull request overview
Adds a new aws-architecture-diagram skill to the deploy-on-aws plugin, plus a validation/post-processing pipeline, to generate AWS reference-style architecture diagrams as draw.io XML using AWS4 icon libraries.
Changes:
- Introduces the
aws-architecture-diagramskill with extensive style/template/reference materials and example diagrams. - Adds a PostToolUse hook and Python tooling to post-process and validate
.drawiooutputs (nesting fixes, icon color fixes, badge overlap fixes, legend sizing, preview URL generation). - Updates plugin README + marketplace/plugin manifests and bumps
deploy-on-awsversion to1.2.0.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/deploy-on-aws/skills/aws-architecture-diagram/SKILL.md | New skill definition and end-to-end workflow for diagram generation + validation/export. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/xml-templates-structure.md | Core XML scaffolding templates (title, users, containers, legend, badges). |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/xml-templates-examples.md | XML snippet examples for edges, waypoints, groups, annotations. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/xml-rules.md | Structural rules/constraints for generating valid draw.io XML. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/style-guide.md | Canonical styling rules (fonts, colors, dark mode, legend/badges). |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/post-processing.md | Documents the automatic fixer/validator pipeline run by the hook. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/layout-guidelines.md | Layout/spacing/routing guidance to match AWS reference diagrams. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/group-styles.md | Copy/paste style strings for AWS boundary groups and edge styles. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/general-icons.md | Mapping guidance for non-AWS technologies to generic AWS4 shapes. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/example-sketch.drawio | Example sketch-mode diagram demonstrating “hand-drawn” styling. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/example-multi-region-active-active.drawio | Example multi-region reference architecture diagram. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/example-agentcore.drawio | Example AgentCore diagram + auxiliary-services patterns. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/diagram-templates-basic.md | Ready-to-use “basic” architecture layout patterns. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/diagram-templates-advanced.md | Advanced patterns (multi-region/hybrid) + sizing guidance. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/cli-export.md | Draw.io desktop CLI export reference (png/svg/pdf). |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/aws4-shapes-services.md | Registry-style list of allowable AWS4 service icon shape names. |
| plugins/deploy-on-aws/skills/aws-architecture-diagram/references/aws4-shapes-resources.md | Registry-style list of allowable AWS4 sub-resource/misc shape names. |
| plugins/deploy-on-aws/scripts/validate-drawio.sh | PostToolUse hook entrypoint: runs post-processing + validation + preview URL. |
| plugins/deploy-on-aws/scripts/requirements.txt | Python dependency pin for the validation pipeline. |
| plugins/deploy-on-aws/scripts/lib/validate_drawio.py | XML validator (structure, IDs, edges, AWS4 shapes) + “compliance” warnings. |
| plugins/deploy-on-aws/scripts/lib/post_process_drawio.py | Unified post-processing runner chaining all fixers. |
| plugins/deploy-on-aws/scripts/lib/fix_step_badges.py | Iterative badge overlap resolver for on-diagram step badges. |
| plugins/deploy-on-aws/scripts/lib/fix_nesting.py | Fixes Region nesting (container=1 → container=0) + re-parenting/offsetting. |
| plugins/deploy-on-aws/scripts/lib/fix_icon_colors.py | Corrects icon fill colors + container tint/strokes + renames broken shapes. |
| plugins/deploy-on-aws/scripts/lib/drawio_url.py | Generates app.diagrams.net preview URLs by compressing/encoding XML. |
| plugins/deploy-on-aws/README.md | Plugin-level docs updated to include the new diagram skill + prerequisites. |
| plugins/deploy-on-aws/hooks/hooks.json | Registers PostToolUse hook to run the draw.io validator on Edit/Write. |
| plugins/deploy-on-aws/.claude-plugin/plugin.json | Updates plugin description/keywords and bumps version to 1.2.0. |
| .claude-plugin/marketplace.json | Updates marketplace metadata (keywords/tags/version/description). |
| 2. Read `references/style-guide.md` for colors, fonts, and dark mode | ||
| 3. Read `references/xml-templates-structure.md` for XML code blocks | ||
| 4. Read `references/layout-guidelines.md` for spacing and edge routing | ||
| 5. Select 1-2 example `.drawio` files from the table below and study them for edge routing and layout patterns |
There was a problem hiding this comment.
The skill instructions are internally contradictory: Step 3 tells the agent to read/study example .drawio files, but the “Important Rules” section later says “Do NOT read existing .drawio files as reference”. This will lead to inconsistent agent behavior depending on which instruction it follows. Please reconcile (either allow reading the curated examples under references/, or remove the earlier instruction to study .drawio examples).
| 5. Select 1-2 example `.drawio` files from the table below and study them for edge routing and layout patterns | |
| 5. Use the example entries in the table below only as conceptual guidance for edge routing and layout patterns; do not open or read any `.drawio` files as reference. |
| ```xml | ||
| <mxCell id="legend-line-styles-group" style="group;fontFamily=Helvetica;fillColor=light-dark(#F5F5F5,#29393B);strokeColor=#666666;" value="" vertex="1" parent="legend-panel"> | ||
| <mxGeometry height="133" width="456" as="geometry" /> | ||
| </mxCell> | ||
| ``` |
There was a problem hiding this comment.
In the “Line Styles Box” template, the legend-line-styles-group cell uses parent="legend-panel", but there’s no legend-panel defined in the surrounding templates (the legend container is legend-outer/legend-container). This makes the snippet unusable as-is and will likely result in broken XML when copied. Please update the parent to a defined legend container ID consistent with the other templates/examples.
…d post-processor bugs - Reject hardcoded background attribute on mxGraphModel (breaks dark mode) - Reject bare <mxGraphModel> without <mxfile> wrapper per skill contract - Lower font size threshold from 11px to 10px to match skill's font hierarchy - Remove stale fontColor=#232F3E warning (redundant with style guide) - Fix legend-panel parent ID typo in xml-templates-structure.md - Fix .upper() bug in badge skip condition (dead code) - Replace blunt "Do NOT read .drawio" rule with Reference Priority section - Remove duplicate developer_tools entry in aws4-shapes.json that shadowed xray
Code reviewFound 8 issues across our review and Copilot's findings. Pushed fixes in commit 7742c33. Fixes applied (7742c33):
Also fixed: Noted but not changed:
Copilot finding F3 (background color) note: Copilot claimed "SKILL.md says never set background #FFFFFF" — at the time of Copilot's review, SKILL.md actually said "ALWAYS set background #FFFFFF." Both the SKILL.md and the validator were stale. We've now aligned both: no hardcoded background attribute allowed. Generated with Claude Code |
…on, and resource exhaustion - Replace JSON escaping fallback with safe static message (CWE-838) - Use importlib.util for sibling imports instead of sys.path manipulation (CWE-426) - Fix defusedxml ET.indent() bug by using stdlib for write-time formatting - Replace bare except with specific exception types in hook JSON parser (CWE-396) - Add 2MB file size guard to URL generator before reading into memory (CWE-400)
…() import The xml.etree.ElementTree import is only used for indent() (write-time formatting), not XML parsing. All parsing uses defusedxml. Add inline suppression for Bandit B405 and Semgrep XXE rules to match the existing pattern in drawio_url.py.
…) import The specific rule ID nosemgrep didn't match because Semgrep uses a composite rule ID. Bare # nosemgrep suppresses all rules on the line. The justification is in the comment: only used for indent(), not parsing.
Drop the indent() call and StdET import that triggered Bandit B405 and Semgrep XXE rules. XML indentation is cosmetic — draw.io and agents handle non-indented XML fine. Added a comment noting that a custom indent helper can be added later if human-readable XML is needed.
Prevent prompt injection via crafted draw.io cell attributes that flow through the PostToolUse hook into agent systemMessage context. Adds _sanitize_attr() which strips control characters, collapses whitespace, restricts to safe characters, and truncates at 80 chars. Applied to all user-controlled attribute values in error/warning messages.
…s, and info leakage (#119) Follow-on security hardening for the architecture diagram skill (PR#103): - Add 2MB file size checks before XML parsing in validate_drawio.py and post_process_drawio.py (matching existing drawio_url.py guard) - Add iterparse-based XML depth (50) and element count (50,000) pre-flight check to prevent C-stack overflow from deeply nested files - Reject symlinks in post_process_drawio.py before write-back to prevent symlink-follow write attacks - Wrap fixer pipelines in top-level try/except to prevent unhandled exception tracebacks from leaking file paths and source code into the hook systemMessage (stderr captured via 2>&1) - Add timeout wrappers (10s/5s) around all Python subprocess calls in validate-drawio.sh to prevent runaway processes from blocking the hook Co-authored-by: Scott Schreckengaust <345885+scottschreckengaust@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
scottschreckengaust
dismissed stale reviews from theagenticguy, MichaelWalker-git, and themself
via
575876c
| # Not a drawio file, exit silently (hook compatibility) | ||
| sys.exit(0) | ||
|
|
||
| path = Path(file_path) |
Check failure
Code scanning / Semgrep OSS
Semgrep Finding: ai.ai-best-practices.hooks-path-traversal.hooks-path-traversal.hooks-path-traversal-python Error
Summary
aws-architecture-diagramskill to thedeploy-on-awsplugin that generates validated AWS architecture diagrams as draw.io XML using official AWS4 icon librariesdeploy-on-awsplugin version from 1.1.0 to 1.2.0 and updates marketplace/plugin manifests with new keywords and descriptionTest plan
claude --plugin-dir ./plugins/deploy-on-awsand trigger the skill by asking to generate an AWS architecture diagram.drawiofile opens correctly in draw.io desktop and renders with proper AWS4 icons, step badges, and category containers.drawiofiles and catches malformed XMLmise run lint:manifeststo validate updated marketplace.json and plugin.jsonmise run lint:cross-refsto validate cross-references between manifestsBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.