Skip to content

feat(bg_remover): harden skill to v0.2.0 - #268

Open
AyushSrivastava1818 wants to merge 5 commits into
ARPAHLS:mainfrom
AyushSrivastava1818:feat/bg-remover-hardening-v0.2-clean
Open

feat(bg_remover): harden skill to v0.2.0#268
AyushSrivastava1818 wants to merge 5 commits into
ARPAHLS:mainfrom
AyushSrivastava1818:feat/bg-remover-hardening-v0.2-clean

Conversation

@AyushSrivastava1818

Copy link
Copy Markdown
Contributor

Description

Hardens the creative/bg_remover skill to v0.2.0 by improving robustness, validation, and documentation.

Fixes #257

Changes

  • Reuse cached rembg sessions across executions.
  • Validate Base64 input and image files.
  • Reject missing, directory, empty, oversized (>25 MB), and invalid image inputs.
  • Automatically create parent directories for output_path.
  • Update the skill documentation and manifest.
  • Expand test coverage for the new validation behavior and session reuse.

Type of Change

  • Skill Upgrade — changes to an existing skill under skills/

Checklist (all PRs)

  • Linked GitHub issue (Fixes #257)
  • Scope matches the issue — no unrelated refactors
  • python -m black --check . and flake8 pass locally (or CI-equivalent subset)
  • pytest skills/ and pytest tests/ pass locally when relevant
  • CHANGELOG.md updated under [Unreleased] when user-visible behavior changes
  • examples/README.md updated if this PR adds, renames, or removes a runnable script
  • Ran pytest tests/test_registry_docs.py when skills, examples index, or agent-loops matrix changed

New or updated skill

  • Skill at skills/<category>/<skill_name>/ (from templates/python_skill/ or equivalent)
  • manifest.yaml: name, version, description, parameters, constitution, real issuer
  • Deterministic skill.py (no ad-hoc LLM-generated execution paths)
  • instructions.md explains when and how to use the skill
  • card.json issuer matches manifest when present
  • test_skill.py covers execution and schema expectations
  • SkillLoader.load_skill("/<skill_name>") succeeds (or deps documented)
  • docs/skills/README.md and row in docs/skills/README.md
  • Usage Examples for Gemini, Claude, OpenAI, DeepSeek, Ollama per skill usage template

Constitution and safety

This update improves input validation and filesystem safety by validating Base64 input and image files, rejecting missing, directory, empty, oversized (>25 MB), and invalid images, reusing cached rembg sessions, and automatically creating parent directories for output_path.

Related Issues

Fixes #257

@rosspeili

Copy link
Copy Markdown
Contributor

Thanks for the clean branch, much better than #266, I will close 266 and track here.

This is closer on the runtime core (session cache, basic validation, 25 MB limit, mkdir for output_path, tests), but it is not yet enough to close #257. Please treat the issue acceptance criteria as a checklist and finish them in this PR.

Still missing / fix

  1. Rebase on current main (post-0.4.7 / citation) so CHANGELOG/Unreleased merges cleanly.
  2. Path safety: reject traversal / unsafe output_path (and document the rule).
  3. Apply size + decode checks to Base64 too (not only input_path).
  4. PIL verify(): reopen the image after verify() before reuse (classic gotcha).
  5. Manifest / errors: document FILE_NOT_FOUND (and any new codes) in manifest.yaml error_code + keep instructions.md / catalog aligned.
  6. [Skill Upgrade]: creative/bg_remover — hardening to 0.2.0 #257 deliverables still open:
    • examples/bg_remover_demo.py + examples/README.md row
    • wire docs/usage/agent_loops.md (not catalog-only)
    • optional first-run / model-download hint in response or docs (as scoped in the issue)
  7. Prefer loading version/metadata from manifest.yaml instead of a second hardcoded dict in skill.py if other skills already do that pattern.

Bump is fine as 0.2.0 once the above is in. Ping when the PR matches the full #257 checklist, I will re-review <3

@AyushSrivastava1818
AyushSrivastava1818 force-pushed the feat/bg-remover-hardening-v0.2-clean branch from e4cad04 to 61a942a Compare July 29, 2026 11:23
@rosspeili

Copy link
Copy Markdown
Contributor

Thanks @AyushSrivastava1818, re-reviewed the latest commits on. The runtime core is in good shape now and mostly matches the earlier review / #257: session reuse, Base64 validation + shared 25 MB check, PIL verify() reopen, output_path traversal reject + parent mkdir, demo + agent_loops wiring, and expanded bundle tests. Local skill tests were also green here.

That said, this is very close but still a bit behind ideal state and not completely ready to close #257 yet. Please ammend these in this PR so we don’t need a follow-up:

Musts

  1. CHANGELOG, move the bg_remover entry under [Unreleased] (do not edit the already-shipped [0.4.7] section).
  2. examples/README.md, restore normal spacing. The file currently has extra blank lines throughout; please revert that rewrite and only add the bg_remover_demo.py row cleanly.
  3. Error-code docs, add FILE_NOT_FOUND to the error tables in both instructions.md and docs/skills/bg_remover.md (code/manifest already have it; docs still list only INVALID_INPUT / MISSING_DEPENDENCY / PROCESSING_FAILED).

Polish hints (same PR if easy)

  • Prefer loading the skill manifest property from manifest.yaml instead of a second hardcoded dict (loader already uses the YAML, this just avoids version drift).
  • One oversized Base64 (>25 MB) golden test.

Ping when those three must-fix items land and I’ll re-review for merge and close #257. <3 Good job so far and for following the steering accurately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Skill Upgrade]: creative/bg_remover — hardening to 0.2.0

2 participants