Matt skills#167
Conversation
…kill documentation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a contributor-agent framework, project and organization guidance, automated organization-skill synchronization, expanded validation tooling, and Vitest coverage for SocialShareButton initialization, rendering, sharing, and cleanup. ChangesContributor Framework
Validation Tooling and Tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
no-issue-linked
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agent/core/architecture.md:
- Line 5: The glossary hyperlink in the architecture doc points outside the
repo, so update the GLOSSARY.md reference in the architecture content to use the
correct relative path from this file. Fix the markdown link target so it
resolves from .agent/core/architecture.md without walking above the repository
root, and keep the surrounding glossary/boundary wording unchanged.
In @.agent/core/code-mapping.md:
- Line 5: The glossary reference in the code-mapping document points too far up
the directory tree and resolves outside the repo. Update the Markdown link under
the bold terms note in the code-mapping content to use the correct relative path
to GLOSSARY.md, keeping the same reference text but fixing the target so it
renders properly.
In @.agent/core/edge-cases.md:
- Line 5: The glossary reference in edge-cases.md points too far up the
directory tree and will break from this location. Update the Markdown link in
the “Bold terms” sentence to use the correct relative path to GLOSSARY.md,
keeping the reference anchored from the same section so it resolves properly.
In @.agent/core/examples.md:
- Line 5: The glossary link in examples.md points outside the repo because the
relative path is one level too high. Update the Markdown link target in the
“Bold terms” reference to use the correct path from .agent/core/examples.md, and
keep the link text unchanged so it still points to GLOSSARY.md.
- Around line 80-85: The clipboard example is not valid JavaScript because it
uses top-level await in a copy-paste snippet. Update the example in the
clipboard block to be syntactically valid by wrapping the try/catch logic in an
async function (or another async context) while keeping the
navigator.clipboard.writeText call and this.triggerCallback usage intact.
In @.agent/info/operational-data.md:
- Line 5: The glossary link in operational-data.md points to the wrong location
and should be corrected to a repo-relative path. Update the existing Markdown
reference for GLOSSARY.md in this document to use the proper relative path from
this file so it resolves within the repository, and verify the link text still
matches the glossary definition reference.
In @.agent/instructions/deployment.md:
- Line 5: The glossary link in deployment.md points to the wrong location and
should be corrected. Update the markdown reference for GLOSSARY.md in that file
to use the repo-relative path from deployment.md, and verify the link target
matches the intended skills/GLOSSARY.md location. Keep the surrounding “Bold
terms” note unchanged and only fix the path.
In @.agent/instructions/setup.md:
- Line 5: The glossary link in setup.md points one directory too far up, so
update the reference to use the correct relative path. Fix the markdown link in
the bold terms note so it targets ../../skills/GLOSSARY.md instead of
../../../skills/GLOSSARY.md, keeping the rest of the instruction text unchanged.
- Around line 9-10: The prerequisite note in the setup instructions is
mismatched because the version check only covers npm while the text claims npm,
pnpm, and yarn are supported. Update the setup guidance in the instructions
document so it either mentions only npm with the existing `npm --version` check,
or add explicit version checks for pnpm and yarn alongside the current Node.js
check. Keep the wording aligned with the supported package managers and the
actual verification steps referenced in the setup section.
In @.agent/instructions/testing.md:
- Line 5: The glossary reference in testing.md points to the wrong location and
escapes the repository. Update the Markdown link for GLOSSARY.md in
.agent/instructions/testing.md to use the correct relative path from that file,
and verify the link target resolves within the repo when rendered.
- Around line 11-15: The command table in testing.md is missing the PR’s new
non-watch validation steps; update the documented workflow to include npm run
test and npm run check-size alongside the existing lint and format checks, while
leaving test:watch as optional. Make the change in the table that lists the
commands so the testing instructions match the available tooling and use the
existing command names exactly.
In `@AGENTS.md`:
- Line 6: The root-relative links in AGENTS.md are incorrect because the file
lives at the repository root, so the current ../skills/... references point
outside the repo. Update the glossary reference(s) to use skills/... instead,
and apply the same fix to the other affected lines in AGENTS.md so all links
resolve within the repository.
- Around line 25-35: The ordered list in AGENTS.md is continuing numbering from
earlier entries and will trigger MD029; update the list containing the skill
references so it restarts at 1. for these items or convert the section to
bullets, keeping the existing entries and links intact.
In `@docs/Roadmap.md`:
- Line 33: The Known Issues section still uses the old scoped package name in
the CSS import example, which is inconsistent with the updated package
reference. Update the import path example in Roadmap.md to use the new unscoped
package name `social-share-button-aossie`, matching the existing package name
reference already listed in the document.
In `@src/social-share-button.test.js`:
- Around line 5-131: The SocialShareButton test suite only covers basic
rendering/callbacks and is missing the required accessibility and async behavior
coverage. Add tests around the SocialShareButton class for modal open/close
flows that wait on the setTimeout-driven animation timing, verify Escape key
handling closes the modal, and assert ARIA labels/attributes on the share button
and modal close button. Also add focus-management tests for the open modal
(including focus trapping or focus returning behavior) so the suite matches the
test-file guidelines.
- Around line 116-130: The destroy test for SocialShareButton only verifies
cleanup inside the container, but it does not confirm that the modal created by
createModal() is removed from document.body. Update the test around
instance.destroy() in SocialShareButton to also assert that the modal element
appended to document.body is gone after destruction, alongside the existing
container and isDestroyed checks, so full teardown is covered.
- Around line 18-22: Add explicit global stub cleanup in the test teardown: the
afterEach in social-share-button.test.js currently calls vi.restoreAllMocks()
but does not clear globals created with vi.stubGlobal, so update that teardown
to also call vi.unstubAllGlobals() alongside the existing DOM reset. Keep the
change localized to the afterEach hook so the stubbed open global does not leak
into later tests.
- Around line 93-114: The SocialShareButton click test only checks the onShare
callback and misses the actual sharing side effect. Update the test in
SocialShareButtonClass to also assert the stubbed window.open was called once
with the expected Facebook share URL (and appropriate target/features if used)
after fbBtn.click(), while keeping the existing onShareSpy assertions to cover
both callback and share behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0841ff9-6863-4109-8061-862b06e2d243
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.agent/core/architecture.md.agent/core/code-mapping.md.agent/core/edge-cases.md.agent/core/examples.md.agent/info/operational-data.md.agent/instructions/deployment.md.agent/instructions/setup.md.agent/instructions/testing.md.clinerules.cursorrules.github/copilot-instructions.md.windsurfrulesAGENTS.mdCLAUDE.mddocs/Roadmap.mdpackage.jsonskills/social-share-button/SKILL.mdsrc/social-share-button.test.js
|
|
||
| > **Agent Instruction:** Do NOT guess or invent the project architecture. You MUST consult this file before adding features, creating files, or suggesting structural changes. If something isn't documented here, ask the contributor to check with maintainers — do NOT assume. | ||
| > | ||
| > **Bold terms** are defined in [GLOSSARY.md](../../../skills/GLOSSARY.md); look them up there for the full meaning. This file enforces the project's **architectural boundaries**. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the glossary link path.
../../../skills/GLOSSARY.md walks above the repo root from .agent/core/architecture.md, so this link will render broken. Use ../../skills/GLOSSARY.md here.
Suggested fix
-> **Bold terms** are defined in [GLOSSARY.md](../../../skills/GLOSSARY.md); look them up there for the full meaning.
+> **Bold terms** are defined in [GLOSSARY.md](../../skills/GLOSSARY.md); look them up there for the full meaning.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > **Bold terms** are defined in [GLOSSARY.md](../../../skills/GLOSSARY.md); look them up there for the full meaning. This file enforces the project's **architectural boundaries**. | |
| > **Bold terms** are defined in [GLOSSARY.md](../../skills/GLOSSARY.md); look them up there for the full meaning. This file enforces the project's **architectural boundaries**. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agent/core/architecture.md at line 5, The glossary hyperlink in the
architecture doc points outside the repo, so update the GLOSSARY.md reference in
the architecture content to use the correct relative path from this file. Fix
the markdown link target so it resolves from .agent/core/architecture.md without
walking above the repository root, and keep the surrounding glossary/boundary
wording unchanged.
|
|
||
| > **Agent Instruction:** This file contains specific errors previously made by AI agents in this project. You MUST check this file before writing new code to avoid repeating historical mistakes. This is a living document — maintainers add entries whenever an agent makes a significant mistake in a PR. | ||
| > | ||
| > **Bold terms** are defined in [GLOSSARY.md](../../../skills/GLOSSARY.md); look them up there for the full meaning. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the glossary link path.
../../../skills/GLOSSARY.md walks above the repo root from .agent/core/edge-cases.md, so this link will render broken. Use ../../skills/GLOSSARY.md here.
Suggested fix
-> **Bold terms** are defined in [GLOSSARY.md](../../../skills/GLOSSARY.md); look them up there for the full meaning.
+> **Bold terms** are defined in [GLOSSARY.md](../../skills/GLOSSARY.md); look them up there for the full meaning.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > **Bold terms** are defined in [GLOSSARY.md](../../../skills/GLOSSARY.md); look them up there for the full meaning. | |
| > **Bold terms** are defined in [GLOSSARY.md](../../skills/GLOSSARY.md); look them up there for the full meaning. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agent/core/edge-cases.md at line 5, The glossary reference in edge-cases.md
points too far up the directory tree and will break from this location. Update
the Markdown link in the “Bold terms” sentence to use the correct relative path
to GLOSSARY.md, keeping the reference anchored from the same section so it
resolves properly.
|
|
||
| > **Agent Instruction:** Follow the approved patterns below. If you find yourself writing something that looks like an anti-pattern, stop and reconsider. | ||
| > | ||
| > **Bold terms** are defined in [GLOSSARY.md](../../../skills/GLOSSARY.md); look them up there for the full meaning. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the glossary link path.
../../../skills/GLOSSARY.md walks above the repo root from .agent/core/examples.md, so this link will render broken. Use ../../skills/GLOSSARY.md here.
Suggested fix
-> **Bold terms** are defined in [GLOSSARY.md](../../../skills/GLOSSARY.md); look them up there for the full meaning.
+> **Bold terms** are defined in [GLOSSARY.md](../../skills/GLOSSARY.md); look them up there for the full meaning.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > **Bold terms** are defined in [GLOSSARY.md](../../../skills/GLOSSARY.md); look them up there for the full meaning. | |
| > **Bold terms** are defined in [GLOSSARY.md](../../skills/GLOSSARY.md); look them up there for the full meaning. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agent/core/examples.md at line 5, The glossary link in examples.md points
outside the repo because the relative path is one level too high. Update the
Markdown link target in the “Bold terms” reference to use the correct path from
.agent/core/examples.md, and keep the link text unchanged so it still points to
GLOSSARY.md.
| try { | ||
| await navigator.clipboard.writeText(url); | ||
| this.triggerCallback("copy", url); | ||
| } catch (err) { | ||
| console.error("Clipboard copy failed:", err); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,140p' .agent/core/examples.mdRepository: AOSSIE-Org/SocialShareButton
Length of output: 2503
Make the clipboard example valid JavaScript. .agent/core/examples.md shows await at top level in a JS code block, so it isn’t syntactically valid as written. Wrap it in an async function or remove the await if this is meant to be copy-pasteable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agent/core/examples.md around lines 80 - 85, The clipboard example is not
valid JavaScript because it uses top-level await in a copy-paste snippet. Update
the example in the clipboard block to be syntactically valid by wrapping the
try/catch logic in an async function (or another async context) while keeping
the navigator.clipboard.writeText call and this.triggerCallback usage intact.
| | ---------------------------------- | ------ | --------------------------------------------------------------- | | ||
| | CDN distribution (jsDelivr) | ✅ | `v1.0.3` | | ||
| | npm package | ✅ | Published as `@aossie-org/social-share-button` (scoped) | | ||
| | npm package | ✅ | Published as `social-share-button-aossie` (unscoped) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale package name reference in Known Issues section.
Line 33 was updated to social-share-button-aossie, but line 50 still references the old scoped name @aossie-org/social-share-button in the CSS import path example. Update line 50 to match the new unscoped package name for consistency.
🔧 Proposed fix for line 50
-- **CSS import path:** `import "`@aossie-org/social-share-button/src/social-share-button.css`"` — `src/` is not a valid published path. Must export CSS from `dist/` via a `package.json` `exports` field.
+- **CSS import path:** `import "social-share-button-aossie/src/social-share-button.css"` — `src/` is not a valid published path. Must export CSS from `dist/` via a `package.json` `exports` field.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | npm package | ✅ | Published as `social-share-button-aossie` (unscoped) | | |
| **CSS import path:** `import "social-share-button-aossie/src/social-share-button.css"` — `src/` is not a valid published path. Must export CSS from `dist/` via a `package.json` `exports` field. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/Roadmap.md` at line 33, The Known Issues section still uses the old
scoped package name in the CSS import example, which is inconsistent with the
updated package reference. Update the import path example in Roadmap.md to use
the new unscoped package name `social-share-button-aossie`, matching the
existing package name reference already listed in the document.
| describe("SocialShareButton Unit Tests", () => { | ||
| let container; | ||
|
|
||
| beforeEach(() => { | ||
| // Create a fresh container in the mocked DOM before each test | ||
| container = document.createElement("div"); | ||
| container.id = "share-container"; | ||
| document.body.appendChild(container); | ||
|
|
||
| // Mock window.open and window.location | ||
| vi.stubGlobal("open", vi.fn()); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| // Clean up DOM and global mocks | ||
| document.body.innerHTML = ""; | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it("should initialize with default options", () => { | ||
| const SocialShareButtonClass = window.SocialShareButton; | ||
| expect(SocialShareButtonClass).toBeDefined(); | ||
|
|
||
| const instance = new SocialShareButtonClass({ | ||
| container: "#share-container", | ||
| url: "https://test-url.com", | ||
| title: "Test Title", | ||
| }); | ||
|
|
||
| expect(instance.options.url).toBe("https://test-url.com"); | ||
| expect(instance.options.title).toBe("Test Title"); | ||
| expect(instance.options.theme).toBe("dark"); | ||
| }); | ||
|
|
||
| it("should render the share button inside container when showButton is true", () => { | ||
| const SocialShareButtonClass = window.SocialShareButton; | ||
|
|
||
| new SocialShareButtonClass({ | ||
| container: "#share-container", | ||
| showButton: true, | ||
| buttonText: "Share Now", | ||
| }); | ||
|
|
||
| const button = container.querySelector(".social-share-btn"); | ||
| expect(button).not.toBeNull(); | ||
| expect(button.textContent.trim()).toBe("Share Now"); | ||
| }); | ||
|
|
||
| it("should not render the button if showButton is false", () => { | ||
| const SocialShareButtonClass = window.SocialShareButton; | ||
|
|
||
| new SocialShareButtonClass({ | ||
| container: "#share-container", | ||
| showButton: false, | ||
| }); | ||
|
|
||
| const button = container.querySelector(".social-share-btn"); | ||
| expect(button).toBeNull(); | ||
| }); | ||
|
|
||
| it("should correctly generate share modal HTML with sharing platforms", () => { | ||
| const SocialShareButtonClass = window.SocialShareButton; | ||
|
|
||
| const instance = new SocialShareButtonClass({ | ||
| container: "#share-container", | ||
| platforms: ["facebook", "whatsapp", "twitter"], | ||
| url: "https://example.com", | ||
| }); | ||
|
|
||
| // Verify modal is constructed | ||
| expect(instance.modal).not.toBeNull(); | ||
|
|
||
| // Check if platforms are listed | ||
| const fbItem = instance.modal.querySelector( | ||
| '.social-share-platform-btn[data-platform="facebook"]' | ||
| ); | ||
| const waItem = instance.modal.querySelector( | ||
| '.social-share-platform-btn[data-platform="whatsapp"]' | ||
| ); | ||
| const twItem = instance.modal.querySelector( | ||
| '.social-share-platform-btn[data-platform="twitter"]' | ||
| ); | ||
|
|
||
| expect(fbItem).not.toBeNull(); | ||
| expect(waItem).not.toBeNull(); | ||
| expect(twItem).not.toBeNull(); | ||
| }); | ||
|
|
||
| it("should trigger onShare callback when a platform button is clicked", () => { | ||
| const SocialShareButtonClass = window.SocialShareButton; | ||
| const onShareSpy = vi.fn(); | ||
|
|
||
| const instance = new SocialShareButtonClass({ | ||
| container: "#share-container", | ||
| platforms: ["facebook"], | ||
| onShare: onShareSpy, | ||
| url: "https://example.com", | ||
| }); | ||
|
|
||
| const fbBtn = instance.modal.querySelector( | ||
| '.social-share-platform-btn[data-platform="facebook"]' | ||
| ); | ||
| expect(fbBtn).not.toBeNull(); | ||
|
|
||
| // Trigger click on the sharing button | ||
| fbBtn.click(); | ||
|
|
||
| expect(onShareSpy).toHaveBeenCalledTimes(1); | ||
| expect(onShareSpy).toHaveBeenCalledWith("facebook", "https://example.com"); | ||
| }); | ||
|
|
||
| it("should remove elements and listeners when destroy is called", () => { | ||
| const SocialShareButtonClass = window.SocialShareButton; | ||
|
|
||
| const instance = new SocialShareButtonClass({ | ||
| container: "#share-container", | ||
| showButton: true, | ||
| }); | ||
|
|
||
| expect(container.children.length).toBeGreaterThan(0); | ||
|
|
||
| instance.destroy(); | ||
|
|
||
| expect(container.children.length).toBe(0); | ||
| expect(instance.isDestroyed).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add accessibility and async behavior tests per test file review guidelines.
As per path instructions for test files, accessibility testing and async behavior testing should be included. The library has setTimeout-based animations (openTimeout, closeTimeout, feedbackTimeout per the source) and modal interactions that warrant async test coverage. Accessibility aspects like keyboard navigation (Escape to close modal), ARIA attributes, and focus management are also untested.
Consider adding tests for:
- Modal open/close behavior and animation timeouts
- Keyboard navigation (Escape key closing modal)
- ARIA label verification on the share button and modal close button
- Focus trapping within the modal when open
As per coding guidelines for **/*.test.{ts,tsx,js,jsx}: "Accessibility testing is included" and "Async behavior is properly tested".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/social-share-button.test.js` around lines 5 - 131, The SocialShareButton
test suite only covers basic rendering/callbacks and is missing the required
accessibility and async behavior coverage. Add tests around the
SocialShareButton class for modal open/close flows that wait on the
setTimeout-driven animation timing, verify Escape key handling closes the modal,
and assert ARIA labels/attributes on the share button and modal close button.
Also add focus-management tests for the open modal (including focus trapping or
focus returning behavior) so the suite matches the test-file guidelines.
Source: Path instructions
| afterEach(() => { | ||
| // Clean up DOM and global mocks | ||
| document.body.innerHTML = ""; | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use vi.unstubAllGlobals() for proper global stub cleanup.
vi.restoreAllMocks() restores mock implementations but does not unstub globals set via vi.stubGlobal. Add vi.unstubAllGlobals() to afterEach to prevent the open stub from leaking. While Vitest isolates test files by default, explicit cleanup is best practice.
♻️ Proposed fix
afterEach(() => {
// Clean up DOM and global mocks
document.body.innerHTML = "";
+ vi.unstubAllGlobals();
vi.restoreAllMocks();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterEach(() => { | |
| // Clean up DOM and global mocks | |
| document.body.innerHTML = ""; | |
| vi.restoreAllMocks(); | |
| }); | |
| afterEach(() => { | |
| // Clean up DOM and global mocks | |
| document.body.innerHTML = ""; | |
| vi.unstubAllGlobals(); | |
| vi.restoreAllMocks(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/social-share-button.test.js` around lines 18 - 22, Add explicit global
stub cleanup in the test teardown: the afterEach in social-share-button.test.js
currently calls vi.restoreAllMocks() but does not clear globals created with
vi.stubGlobal, so update that teardown to also call vi.unstubAllGlobals()
alongside the existing DOM reset. Keep the change localized to the afterEach
hook so the stubbed open global does not leak into later tests.
| it("should trigger onShare callback when a platform button is clicked", () => { | ||
| const SocialShareButtonClass = window.SocialShareButton; | ||
| const onShareSpy = vi.fn(); | ||
|
|
||
| const instance = new SocialShareButtonClass({ | ||
| container: "#share-container", | ||
| platforms: ["facebook"], | ||
| onShare: onShareSpy, | ||
| url: "https://example.com", | ||
| }); | ||
|
|
||
| const fbBtn = instance.modal.querySelector( | ||
| '.social-share-platform-btn[data-platform="facebook"]' | ||
| ); | ||
| expect(fbBtn).not.toBeNull(); | ||
|
|
||
| // Trigger click on the sharing button | ||
| fbBtn.click(); | ||
|
|
||
| expect(onShareSpy).toHaveBeenCalledTimes(1); | ||
| expect(onShareSpy).toHaveBeenCalledWith("facebook", "https://example.com"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
onShare test doesn't verify window.open is called with the correct share URL.
The test asserts the onShare callback is invoked but doesn't verify that window.open was called with the expected platform share URL. Since window.open is already stubbed in beforeEach, adding an assertion would strengthen coverage of the actual sharing behavior.
♻️ Proposed fix
expect(onShareSpy).toHaveBeenCalledTimes(1);
expect(onShareSpy).toHaveBeenCalledWith("facebook", "https://example.com");
+
+ // Verify the share window was opened
+ expect(window.open).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/social-share-button.test.js` around lines 93 - 114, The SocialShareButton
click test only checks the onShare callback and misses the actual sharing side
effect. Update the test in SocialShareButtonClass to also assert the stubbed
window.open was called once with the expected Facebook share URL (and
appropriate target/features if used) after fbBtn.click(), while keeping the
existing onShareSpy assertions to cover both callback and share behavior.
| it("should remove elements and listeners when destroy is called", () => { | ||
| const SocialShareButtonClass = window.SocialShareButton; | ||
|
|
||
| const instance = new SocialShareButtonClass({ | ||
| container: "#share-container", | ||
| showButton: true, | ||
| }); | ||
|
|
||
| expect(container.children.length).toBeGreaterThan(0); | ||
|
|
||
| instance.destroy(); | ||
|
|
||
| expect(container.children.length).toBe(0); | ||
| expect(instance.isDestroyed).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
destroy test doesn't verify modal removal from document.body.
createModal() appends the modal to document.body (per source line 131), but the destroy test only checks container.children.length. Add an assertion that the modal is also removed from document.body to verify full cleanup and prevent memory leaks.
♻️ Proposed fix
instance.destroy();
expect(container.children.length).toBe(0);
expect(instance.isDestroyed).toBe(true);
+ expect(document.body.querySelector(".social-share-modal-overlay")).toBeNull();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should remove elements and listeners when destroy is called", () => { | |
| const SocialShareButtonClass = window.SocialShareButton; | |
| const instance = new SocialShareButtonClass({ | |
| container: "#share-container", | |
| showButton: true, | |
| }); | |
| expect(container.children.length).toBeGreaterThan(0); | |
| instance.destroy(); | |
| expect(container.children.length).toBe(0); | |
| expect(instance.isDestroyed).toBe(true); | |
| }); | |
| it("should remove elements and listeners when destroy is called", () => { | |
| const SocialShareButtonClass = window.SocialShareButton; | |
| const instance = new SocialShareButtonClass({ | |
| container: "`#share-container`", | |
| showButton: true, | |
| }); | |
| expect(container.children.length).toBeGreaterThan(0); | |
| instance.destroy(); | |
| expect(container.children.length).toBe(0); | |
| expect(instance.isDestroyed).toBe(true); | |
| expect(document.body.querySelector(".social-share-modal-overlay")).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/social-share-button.test.js` around lines 116 - 130, The destroy test for
SocialShareButton only verifies cleanup inside the container, but it does not
confirm that the modal created by createModal() is removed from document.body.
Update the test around instance.destroy() in SocialShareButton to also assert
that the modal element appended to document.body is gone after destruction,
alongside the existing container and isDestroyed checks, so full teardown is
covered.
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
git-subtree-dir: skills/shared git-subtree-split: 10d080688e631c0d94de11f570d04b1433dcb20d
github-actions
Bot
added
backend
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agent/info/operational-data.md (1)
9-14: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPin production CDN endpoints to release tags.
The jsDelivr URLs currently omit a branch or tag, while
.agent/instructions/deployment.mddefines production delivery as a release tag. Update these endpoints to use the tagged production path, or explicitly label the current URLs as staging-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agent/info/operational-data.md around lines 9 - 14, Update the jsDelivr CDN JS and CSS entries in the operational data table to reference the production release-tag path defined by deployment instructions, rather than unpinned URLs; alternatively, explicitly label the existing unpinned endpoints as staging-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/sync-org-skills.yml:
- Around line 8-9: Move the contents: write permission from workflow-level
permissions to the sync job’s permissions under jobs.sync. Set the
workflow-level default to read-only while preserving write access for the
synchronization job that pushes updates.
- Around line 3-6: Add a workflow-level concurrency configuration near the
existing triggers in the sync workflow, using a stable group identifier and
setting cancel-in-progress to false. Ensure scheduled and manually dispatched
runs share this group so they queue rather than execute concurrently.
- Around line 15-21: Update the Checkout repository and Set up Python steps to
reference immutable full commit SHAs instead of the mutable actions/checkout@v4
and actions/setup-python@v5 tags, preserving the corresponding version comments
for readability.
In `@AGENTS.md`:
- Around line 10-31: Fix Markdown lint formatting in AGENTS.md around the
onboarding and workflow sections by removing extra spaces after heading markers
and adding blank lines before and after each heading. In
.agent/workflows/do-work.md around the command blocks, add blank lines
immediately before and after each fenced block; make no other content changes.
In `@org-wide-skills/contributor-onboarding/SKILL.md`:
- Line 6: Add a suitable top-level heading before the opening description in
org-wide-skills/contributor-onboarding/SKILL.md at lines 6-6 and apply the same
heading structure before the opening description in
org-wide-skills/project-template/SKILL.md at lines 6-6.
- Around line 15-18: Update the links in the contributor-onboarding entry-gate
instructions to use ../../.agent/... so they resolve to the repository-level
architecture, edge-cases, and setup files; apply the same root-relative
correction to the workflow link at the additionally affected location.
In `@org-wide-skills/GIT-DIS-AIPolicy/references/pr-issue-formatting.md`:
- Around line 64-84: Normalize Markdown formatting in
org-wide-skills/GIT-DIS-AIPolicy/references/pr-issue-formatting.md lines 64-84
by adding blank lines around headings and fenced examples, and adding
appropriate language tags to the example fences. Also update
org-wide-skills/mcp-integration/references/mcp-guide.md lines 5-8, 25-36, and
47-50 with missing blank lines around headings and fenced configuration blocks.
- Around line 29-32: Update the AI Assistance Disclosure template to expose a
dedicated Verification field alongside Tool and Scope. Preserve the existing
disclosure wording, but represent the manual verification status through the
explicit field required by the AI policy.
In `@org-wide-skills/GLOSSARY.md`:
- Around line 7-43: Update the heading hierarchy in GLOSSARY.md so the glossary
section uses H2 and each term uses H3 without skipping levels. Add a blank line
after every definition heading, including the existing entries through
Zero-Dependency Core, so the document passes Markdownlint.
In `@org-wide-skills/mcp-integration/references/mcp-guide.md`:
- Around line 60-63: Remove the duplicate “Accessibility Audits” entry from the
ordered list in the MCP guide, retaining one copy followed by “Responsive Design
Verification” and “Variant Visual Verification” with correct sequential
numbering.
In `@scripts/sync_org_skills.py`:
- Line 30: Update the logging calls in the sync script, including the messages
around the archive download and lines 59–64, to use lazy logger formatting with
`%s` placeholders and values passed as separate arguments. Remove the f-string
interpolation while preserving each message’s content and log level.
- Around line 40-64: Update the sync flow to copy all ORG_SKILLS entries into a
temporary staging directory before modifying TARGET_DIR. Treat any missing
expected item or copy failure as an error and abort without replacing the
existing target; only after every item is validated and copied successfully
should the staged directory atomically replace TARGET_DIR and report success.
- Around line 30-35: Update the archive download in the TemporaryDirectory block
to use a bounded request via urllib.request.urlopen with an explicit timeout,
then write the response contents to zip_path. Add explicit handling for download
failures so the sync operation logs the error and exits cleanly.
- Around line 25-35: Update sync_org_skills and SKILLS_REPO_ZIP to use an
immutable release, tag, or commit archive instead of main.zip, and verify the
downloaded zip against a trusted checksum or signature before extraction or
replacing TARGET_DIR. Preserve the existing temporary-download flow and abort
synchronization when verification fails.
---
Outside diff comments:
In @.agent/info/operational-data.md:
- Around line 9-14: Update the jsDelivr CDN JS and CSS entries in the
operational data table to reference the production release-tag path defined by
deployment instructions, rather than unpinned URLs; alternatively, explicitly
label the existing unpinned endpoints as staging-only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f318804-2f44-47b4-a118-6eccd01df034
📒 Files selected for processing (25)
.agent/core/architecture.md.agent/core/code-mapping.md.agent/core/edge-cases.md.agent/core/examples.md.agent/info/operational-data.md.agent/instructions/deployment.md.agent/instructions/setup.md.agent/instructions/testing.md.agent/workflows/do-work.md.github/workflows/sync-org-skills.ymlAGENTS.mdorg-wide-skills/GIT-DIS-AIPolicy/SKILL.mdorg-wide-skills/GIT-DIS-AIPolicy/references/ai-policy-rules.mdorg-wide-skills/GIT-DIS-AIPolicy/references/communication-templates.mdorg-wide-skills/GIT-DIS-AIPolicy/references/pr-issue-formatting.mdorg-wide-skills/GLOSSARY.mdorg-wide-skills/contributor-onboarding/SKILL.mdorg-wide-skills/mcp-integration/SKILL.mdorg-wide-skills/mcp-integration/references/mcp-guide.mdorg-wide-skills/project-template/SKILL.mdorg-wide-skills/project-template/references/microservice-standards.mdorg-wide-skills/project-template/references/nextjs-standards.mdorg-wide-skills/project-template/references/rest-standards.mdorg-wide-skills/project-template/references/web3-standards.mdscripts/sync_org_skills.py
| on: | ||
| schedule: | ||
| - cron: '0 0 * * *' | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Serialize scheduled and manual synchronization runs.
Without a concurrency group, a manual dispatch can overlap the scheduled run; both jobs may modify and push the same branch, causing conflicting commits or failed pushes. Add a workflow concurrency group with cancel-in-progress: false.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 3-6: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/sync-org-skills.yml around lines 3 - 6, Add a
workflow-level concurrency configuration near the existing triggers in the sync
workflow, using a stable group identifier and setting cancel-in-progress to
false. Ensure scheduled and manually dispatched runs share this group so they
queue rather than execute concurrently.
Source: Linters/SAST tools
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope write permission to the synchronization job.
The job needs contents: write to push updates, but declaring it at workflow scope grants that permission to every current or future job. Keep the workflow default read-only and grant contents: write only under jobs.sync.permissions.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 9-9: overly broad permissions (excessive-permissions): contents: write is overly broad at the workflow level
(excessive-permissions)
[warning] 9-9: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/sync-org-skills.yml around lines 8 - 9, Move the contents:
write permission from workflow-level permissions to the sync job’s permissions
under jobs.sync. Set the workflow-level default to read-only while preserving
write access for the synchronization job that pushes updates.
Source: Linters/SAST tools
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.11' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/sync-org-skills.yml"
echo "== file exists =="
git ls-files "$FILE"
echo
echo "== relevant lines =="
nl -ba "$FILE" | sed -n '1,120p'Repository: AOSSIE-Org/SocialShareButton
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/sync-org-skills.yml"
echo "== relevant lines =="
sed -n '1,120p' "$FILE" | cat -nRepository: AOSSIE-Org/SocialShareButton
Length of output: 1417
Pin the GitHub Actions in this workflow. actions/checkout@v4 and actions/setup-python@v5 are mutable tags; switch them to full commit SHAs and keep the version comments for readability.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 15-16: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 16-16: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 19-19: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/sync-org-skills.yml around lines 15 - 21, Update the
Checkout repository and Set up Python steps to reference immutable full commit
SHAs instead of the mutable actions/checkout@v4 and actions/setup-python@v5
tags, preserving the corresponding version comments for readability.
Source: Linters/SAST tools
| ### Onboarding & First Setup | ||
| Load when a contributor asks "how do I start", "what should I read", or is new to the project: | ||
| - [org-wide-skills/contributor-onboarding/SKILL.md](org-wide-skills/contributor-onboarding/SKILL.md) — Entry gate: reads local context, establishes policy, hands off to do-work. | ||
|
|
||
| ### Implementing a Feature or Bug Fix (Full Workflow) | ||
| Load when the contributor has an assigned issue and is ready to code: | ||
| - [.agent/workflows/do-work.md](.agent/workflows/do-work.md) — Canonical workflow: Explore → Code → Verify → PR. | ||
|
|
||
| ### Writing & Modifying Code (Reference Only) | ||
| Load individual files as needed during coding: | ||
| - [.agent/core/architecture.md](.agent/core/architecture.md) — Zero-dependency constraint and architectural boundaries. | ||
| - [.agent/core/code-mapping.md](.agent/core/code-mapping.md) — Directory layout (`src/`, `public/`, `landing-page/`). | ||
| - [.agent/core/examples.md](.agent/core/examples.md) — Approved code patterns vs anti-patterns. | ||
| - [org-wide-skills/project-template/SKILL.md](org-wide-skills/project-template/SKILL.md) — Shared architecture standards by stack. | ||
|
|
||
| ### Testing & Debugging | ||
| Load when running verification checks or fixing errors: | ||
| - [.agent/instructions/testing.md](.agent/instructions/testing.md) — ESLint, formatting, and visual browser verification steps. | ||
| - [.agent/core/edge-cases.md](.agent/core/edge-cases.md) — Historical agent mistakes and size-budget restrictions. | ||
| - [org-wide-skills/mcp-integration/SKILL.md](org-wide-skills/mcp-integration/SKILL.md) — Automated UI browser testing via MCP. | ||
|
|
||
| ### Pull Requests & Deployment |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix Markdown lint violations in contributor documentation.
Both documents contain formatting that prevents clean Markdown lint validation:
AGENTS.md#L10-L31: remove extra spaces after heading markers and surround headings with blank lines..agent/workflows/do-work.md#L34-L44: add blank lines before and after the fenced command blocks.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 10-10: Multiple spaces after hash on atx style heading
(MD019, no-multiple-space-atx)
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 14-14: Multiple spaces after hash on atx style heading
(MD019, no-multiple-space-atx)
[warning] 14-14: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 18-18: Multiple spaces after hash on atx style heading
(MD019, no-multiple-space-atx)
[warning] 18-18: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 25-25: Multiple spaces after hash on atx style heading
(MD019, no-multiple-space-atx)
[warning] 25-25: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 31-31: Multiple spaces after hash on atx style heading
(MD019, no-multiple-space-atx)
[warning] 31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
📍 Affects 2 files
AGENTS.md#L10-L31(this comment).agent/workflows/do-work.md#L34-L44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 10 - 31, Fix Markdown lint formatting in AGENTS.md
around the onboarding and workflow sections by removing extra spaces after
heading markers and adding blank lines before and after each heading. In
.agent/workflows/do-work.md around the command blocks, add blank lines
immediately before and after each fenced block; make no other content changes.
Source: Linters/SAST tools
| description: Org-level entry gate for all AOSSIE contributors. Use when a contributor asks "how do I start", "what should I read", or begins any contribution-related conversation. | ||
| --- | ||
|
|
||
| A skill serving as the org-wide entry gate for all contributors to AOSSIE repositories. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add top-level headings to both skill documents.
org-wide-skills/contributor-onboarding/SKILL.md#L6-L6: add a heading before the opening description.org-wide-skills/project-template/SKILL.md#L6-L6: add a heading before the opening description.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 6-6: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
📍 Affects 2 files
org-wide-skills/contributor-onboarding/SKILL.md#L6-L6(this comment)org-wide-skills/project-template/SKILL.md#L6-L6
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@org-wide-skills/contributor-onboarding/SKILL.md` at line 6, Add a suitable
top-level heading before the opening description in
org-wide-skills/contributor-onboarding/SKILL.md at lines 6-6 and apply the same
heading structure before the opening description in
org-wide-skills/project-template/SKILL.md at lines 6-6.
Source: Linters/SAST tools
| 1. **Accessibility Audits:** Analyze the accessibility tree, check color contrast ratios, and verify keyboard navigation. | ||
| 1. **Accessibility Audits:** Analyze the accessibility tree, check color contrast ratios, and verify keyboard navigation. | ||
| 2. **Responsive Design Verification:** Programmatically resize the viewport across mobile, tablet, and desktop widths to check for responsive layout breaks. | ||
| 3. **Variant Visual Verification:** Interact with different component states/props (e.g., hover, disabled, active, light/dark themes) and capture visual screenshots of each state. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the duplicated accessibility-audit step.
The same item appears on Lines 60 and 61, causing the ordered list to skip directly from item 1 to item 2 and presenting the same instruction twice.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 62-62: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
[warning] 63-63: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@org-wide-skills/mcp-integration/references/mcp-guide.md` around lines 60 -
63, Remove the duplicate “Accessibility Audits” entry from the ordered list in
the MCP guide, retaining one copy followed by “Responsive Design Verification”
and “Variant Visual Verification” with correct sequential numbering.
Source: Linters/SAST tools
| SKILLS_REPO_ZIP = "https://github.com/AOSSIE-Org/Skills/archive/refs/heads/main.zip" | ||
| TARGET_DIR = repo_root / "org-wide-skills" | ||
|
|
||
|
|
||
| def sync_org_skills(): | ||
| logger.info(f"Downloading org skills archive from {SKILLS_REPO_ZIP}...") | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| tmp_path = Path(tmp_dir) | ||
| zip_path = tmp_path / "skills.zip" | ||
| urllib.request.urlretrieve(SKILLS_REPO_ZIP, zip_path) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== AGENTS files ==\n'
find .. -name AGENTS.md -o -name agENTS.md 2>/dev/null | sed 's#^\./##' | sort
printf '\n== Root listing ==\n'
ls -la
printf '\n== Read AGENTS.md files (if present) ==\n'
for f in $(find . -name AGENTS.md | sort); do
echo "--- $f ---"
sed -n '1,260p' "$f"
doneRepository: AOSSIE-Org/SocialShareButton
Length of output: 5080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Candidate guidance files ==\n'
git ls-files | rg '(^|/)(AGENTS\.md|SKILL\.md|do-work\.md|testing\.md|edge-cases\.md|operational-data\.md|deployment\.md)$' || true
printf '\n== scripts/sync_org_skills.py outline ==\n'
ast-grep outline scripts/sync_org_skills.py --view expanded || true
printf '\n== scripts/sync_org_skills.py lines 1-140 ==\n'
sed -n '1,140p' scripts/sync_org_skills.pyRepository: AOSSIE-Org/SocialShareButton
Length of output: 3020
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print only the exact guidance files that exist in the repo.
for f in \
AGENTS.md \
.agent/workflows/do-work.md \
.agent/instructions/testing.md \
.agent/core/edge-cases.md \
.agent/info/operational-data.md \
.agent/instructions/deployment.md \
org-wide-skills/contributor-onboarding/SKILL.md \
org-wide-skills/mcp-integration/SKILL.md \
org-wide-skills/GIT-DIS-AIPolicy/SKILL.md
do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,260p' "$f"
fi
done
echo "--- scripts/sync_org_skills.py (numbered excerpt) ---"
cat -n scripts/sync_org_skills.py | sed -n '1,180p'Repository: AOSSIE-Org/SocialShareButton
Length of output: 23119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== References to sync_org_skills.py ==\n'
rg -n "sync_org_skills|org-wide-skills|Skills/archive/refs/heads/main.zip" . || true
printf '\n== sync_org_skills.py full file with line numbers ==\n'
cat -n scripts/sync_org_skills.pyRepository: AOSSIE-Org/SocialShareButton
Length of output: 4037
Pin the archive source and verify it before replacing org-wide-skills. (scripts/sync_org_skills.py:25-35) main.zip is mutable, so an upstream change or compromise can flow into tracked repo content. Use a pinned release/tag or commit and validate a checksum/signature before extraction.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 30-30: Logging statement uses f-string
(G004)
[error] 35-35: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/sync_org_skills.py` around lines 25 - 35, Update sync_org_skills and
SKILLS_REPO_ZIP to use an immutable release, tag, or commit archive instead of
main.zip, and verify the downloaded zip against a trusted checksum or signature
before extraction or replacing TARGET_DIR. Preserve the existing
temporary-download flow and abort synchronization when verification fails.
|
|
||
|
|
||
| def sync_org_skills(): | ||
| logger.info(f"Downloading org skills archive from {SKILLS_REPO_ZIP}...") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use lazy logging formatting.
Ruff flags the f-string logging calls at Lines 30, 59, 62, and 64. Pass values as logger arguments, for example logger.info("Copied %s", item), to satisfy G004 and avoid eager interpolation.
Also applies to: 59-64
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 30-30: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/sync_org_skills.py` at line 30, Update the logging calls in the sync
script, including the messages around the archive download and lines 59–64, to
use lazy logger formatting with `%s` placeholders and values passed as separate
arguments. Remove the f-string interpolation while preserving each message’s
content and log level.
Source: Linters/SAST tools
| logger.info(f"Downloading org skills archive from {SKILLS_REPO_ZIP}...") | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| tmp_path = Path(tmp_dir) | ||
| zip_path = tmp_path / "skills.zip" | ||
| urllib.request.urlretrieve(SKILLS_REPO_ZIP, zip_path) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AGENTS files =="
find .. -name AGENTS.md -print
echo
echo "== Root files =="
pwd
git ls-files | sed -n '1,120p'
echo
echo "== Target file and nearby context =="
python3 - <<'PY'
from pathlib import Path
p = Path("scripts/sync_org_skills.py")
print("exists:", p.exists())
if p.exists():
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 1 <= i <= min(len(lines), 120):
print(f"{i:4}: {line}")
PYRepository: AOSSIE-Org/SocialShareButton
Length of output: 6388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AGENTS.md =="
cat -n AGENTS.md
echo
echo "== sync-org-skills workflow =="
cat -n .github/workflows/sync-org-skills.yml
echo
echo "== related script references =="
rg -n "sync_org_skills|urlretrieve|timeout" .github/workflows scripts .agent org-wide-skills -g '!**/node_modules/**' || trueRepository: AOSSIE-Org/SocialShareButton
Length of output: 4489
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import inspect
import urllib.request
import urllib.error
print("urlretrieve signature:", inspect.signature(urllib.request.urlretrieve))
print("urlopen signature:", inspect.signature(urllib.request.urlopen))
src = inspect.getsource(urllib.request.urlretrieve)
print("\nurlretrieve source excerpt:")
for i, line in enumerate(src.splitlines(), 1):
if i <= 40:
print(f"{i:3}: {line}")
PYRepository: AOSSIE-Org/SocialShareButton
Length of output: 2033
Bound the archive download. urllib.request.urlretrieve() here has no per-call timeout, so a stalled fetch can hold the scheduled runner until the job times out. Switch to urlopen(..., timeout=...) (or equivalent) and handle download failures explicitly.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 30-30: Logging statement uses f-string
(G004)
[error] 35-35: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/sync_org_skills.py` around lines 30 - 35, Update the archive download
in the TemporaryDirectory block to use a bounded request via
urllib.request.urlopen with an explicit timeout, then write the response
contents to zip_path. Add explicit handling for download failures so the sync
operation logs the error and exits cleanly.
| extracted_dirs = [d for d in tmp_path.iterdir() if d.is_dir()] | ||
| if not extracted_dirs: | ||
| logger.error("Failed to extract skills archive.") | ||
| return | ||
| skills_root = extracted_dirs[0] | ||
|
|
||
| if TARGET_DIR.exists(): | ||
| shutil.rmtree(TARGET_DIR) | ||
| TARGET_DIR.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| copied_count = 0 | ||
| for item in ORG_SKILLS: | ||
| src = skills_root / item | ||
| dst = TARGET_DIR / item | ||
| if src.exists(): | ||
| if src.is_dir(): | ||
| shutil.copytree(src, dst) | ||
| else: | ||
| shutil.copy2(src, dst) | ||
| logger.info(f"Copied {item} -> org-wide-skills/{item}") | ||
| copied_count += 1 | ||
| else: | ||
| logger.warning(f"Item '{item}' not found in Skills repository.") | ||
|
|
||
| logger.info(f"Successfully synced {copied_count} org-wide skills to {TARGET_DIR}") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Stage and validate the complete sync before replacing org-wide-skills.
The script deletes TARGET_DIR at Line 47, then treats missing ORG_SKILLS entries as warnings and still returns success. A partial or renamed upstream archive can therefore silently delete existing skills; .github/workflows/sync-org-skills.yml will then commit those deletions. Copy into a staging directory, require every expected item, and replace the target only after the full sync succeeds.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 59-59: Logging statement uses f-string
(G004)
[warning] 62-62: Logging statement uses f-string
(G004)
[warning] 64-64: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/sync_org_skills.py` around lines 40 - 64, Update the sync flow to
copy all ORG_SKILLS entries into a temporary staging directory before modifying
TARGET_DIR. Treat any missing expected item or copy failure as an error and
abort without replacing the existing target; only after every item is validated
and copied successfully should the staged directory atomically replace
TARGET_DIR and report success.
…task-based guidance
…for project maintenance
1 participant
Addressed Issues:
Fixes #(issue number)
Screenshots/Recordings:
Additional Notes:
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Summary by CodeRabbit