8.0

.claude/agent-memory/code-improvement-reviewer/MEMORY.md

@@ -0,0 +1,69 @@
+# image-bundle Code Review Memory
+
+## Project Snapshot
+- Symfony 8 / PHP 8.5 bundle, no `src/` directory — PSR-4 root is the package root.
+- Namespace: `ChamberOrchestra\ImageBundle`
+- Second review pass completed: 2026-02-17 (init branch, after first-pass fixes applied)
+
+## Architecture
+- Request → ImageController → FilterService → CacheManager / DataManager → FilterManager → CacheManager store → 301 redirect
+- Binary layer: LoaderInterface (FileSystemLoader, StreamLoader) → LocatorInterface → BinaryInterface / FileBinaryInterface
+- Filter layer: FilterConfiguration (holds all named filter configs), FilterManager (applies processors + post-processors via ServiceLocator)
+- Cache layer: CacheManager (central), ResolverInterface (WebPathResolver, CacheResolver decorator), Signer (HMAC)
+- Twig: ImageExtension / ImageRuntime (lazy runtime)
+- Events: FileRemoveSubscriber listens to ChamberOrchestra\FileBundle\Events\PostRemoveEvent (EventSubscriber/ dir, not EventListener/)
+
+## Key Issues Found (second-pass — post-fix state)
+
+### Critical
+1. **CwebpPostProcessor wrong MIME type guard** (`CwebpPostProcessor.php:44–47`): Skips processing unless mime IS webp, but cwebp converts FROM jpeg/png/etc TO webp. Logic is inverted — webp input is a no-op; any non-webp image is silently passed through uncompressed.
+2. **`CacheResolver::remove()` deletes wrong cache key** (`CacheResolver.php:73–74`): Deletes `sanitizeCacheKeyPart($path)` instead of `generateCacheKey($path, $filter)`. Passes only path, no filter — mismatches the 4-part key generated by `generateCacheKey()`, so the cache entry is never actually removed.
+3. **`DataManager::getDefaultImageUrl()` uses falsy check** (`DataManager.php:109`): `if ($config['default_image'])` — if the default_image is the string "0" or empty string, it's falsily skipped even though it was set.
+4. **`Signer::sign()` uses PHP `serialize()` for config hash input** (`Signer.php:24`): serialize() output varies with object insertion order. Two arrays with identical keys/values but different insertion order produce different HMAC strings, breaking URL validation for runtime configs built programmatically in different orders.
+
+### High
+5. **`AbstractFilesystemResolver::store()` not atomic** (`AbstractFilesystemResolver.php:48–53`): Uses bare `file_put_contents()` — no temp-file+rename pattern. Concurrent requests writing the same path will produce torn files (partial write visible to readers).
+6. **`FilterManager::save()` format fallback swallows `output` config** (`FilterManager.php:106–113`): reads `$config['format']` but the `output` array (from YAML config) stores `format` as `$config['output']['format']`. The top-level `$config['format']` key is only populated when `OutputProcessor` mutates `$config` via the `&$config` reference. If `output` processor is not in the chain, `$config['output']['format']` is ignored and `$binary->getFormat()` is used instead — subtly wrong for e.g. force-convert-to-webp configs that only define `output.format` without the `output` processor.
+7. **`CacheManager::remove()` calls `getRuntimePrefix($path)` but path may include leading slash** (`CacheManager.php:119`): `remove()` receives `$path` from `FileRemoveSubscriber::$event->resolvedUri` with no normalization, but `Signer::hash()` strips leading slashes with `ltrim`. However `CacheManager::resolve()` normalizes via `str_contains` check — the raw path passed to `remove()` may differ from the stored runtime-prefix key.
+8. **`WebPathResolver::remove()` ignores filter** (`WebPathResolver.php:59–66`): `getFilePath($path)` is called with no `$filter` argument (defaults to `''`), so it removes `{webRoot}/{cachePrefix}/{path}` rather than `{webRoot}/{cachePrefix}/{filter}/{path}`. The cache file is stored with filter in path but removed without it — cache entries are never deleted.
+9. **`InterlaceProcessor` accepts any string as mode** (`InterlaceProcessor.php:16–20`): No validation of `$options['mode']` value against allowed Imagine constants (INTERLACE_NONE, INTERLACE_LINE, INTERLACE_PLANE, INTERLACE_PARTITION). An invalid mode string is passed directly to Imagine's `interlace()`, which may throw a cryptic exception or silently be ignored depending on driver.
+10. **`FileSystemLoaderFactory` uses `sprintf` with no format string** (`FileSystemLoaderFactory.php:22`): `new Reference(\sprintf($config['locator']))` — `sprintf` with a single argument string passes through unchanged but if the locator string contained `%` characters it would throw. Should be `new Reference($config['locator'])` directly.
+11. **`AbstractFilesystemResolver` `makeFolder()` infinite recursion risk** (`AbstractFilesystemResolver.php:80–91`): Recursive `makeFolder()` call with no base-case guard on filesystem root `/`. If `$dir` resolves to `/` on a misconfigured server, `dirname('/')` returns `/` and the call recurses infinitely until stack overflow.
+
+### Medium
+12. **`BinaryMimeTypeGuesser::isBinary()` false-negatives on text images** (`BinaryMimeTypeGuesser.php:40–43`): `ctype_print()` returns `true` for SVG content (all printable characters) so SVG is treated as non-binary and `guessMimeType()` returns null, causing DataManager to throw "mime type not guessed".
+13. **`CacheResolver::saveToCache()` double `getItem()` call** (`CacheResolver.php:155–178`): Fetches `$indexKey` item twice (lines 156 and 173) within the same request — the second fetch can return a stale value from the PSR-6 pool if the pool does internal deferred saves. Should reuse the `$index` item object.
+14. **`ImageRuntime::fit()` and `fill()` cast null to 0** (`ImageRuntime.php:25,38`): `(int) null === 0`, so passing `null` for width or height silently becomes 0. The `AbstractResizeProcessor` normalizer then throws `RuntimeException('Width and Height can not be 0 simultaneously')` when both are null. The type hint `int|null` implies null is valid but it is not — parameters should be `int` with defaults.
+15. **`FilterConfiguration` `$filters` property is `protected`** (`FilterConfiguration.php:11`): There is no subclass in the codebase, and `protected` breaks the immutability expectation. Should be `private` or the class should be `readonly`.
+16. **`DataManager::getLoader()` uses falsy check** (`DataManager.php:62`): `$config['loader'] ?: $this->defaultLoader` — if a filter's loader is configured as `"0"` (pathological but possible), it falls back to the default loader silently.
+17. **`WebPathResolver::getBaseUrl()` is dead code** (`WebPathResolver.php:87–110`): The method is defined `protected` and never called from anywhere in the class or any subclass. Should be removed or actually wired up to `resolve()`.
+18. **`CacheResolver::setDefaultOptions()` is dead code** (`CacheResolver.php:206–209`): Calls `$this->configureOptions()` — duplicate of the constructor flow and never called externally. Leftover from a refactor.
+19. **`AbstractResizeProcessor::create()` always pastes image onto canvas** (`AbstractResizeProcessor.php:84–96`): Even when image size exactly equals canvas size (`doApply` returned the exact target size), a full canvas + paste operation is performed. This doubles memory use unnecessarily for the common no-upscale case.
+20. **`FitProcessor::doApply()` visibility is `public` but `FillProcessor::doApply()` is `protected`** (`FitProcessor.php:13`, `FillProcessor.php:15`): Inconsistent visibility for the same abstract method. The interface contract in `AbstractResizeProcessor` declares `abstract protected` — `FitProcessor` widens it to `public`, which PHP allows but it breaks Liskov substitution expectations and the abstract declaration.
+21. **`OutputProcessor` allows only `png`, `jpg`, `webp` formats but `FilterManager::save()` calls `image->get($filteredFormat)`** (`OutputProcessor.php:13`, `FilterManager.php:113`): Imagine supports `gif`, `bmp`, `tiff` etc., but these are silently rejected by the validator, making the config `format: gif` fail with an opaque OptionsResolver validation error.
+22. **`Signer` signs path with `ltrim` but controller passes `urldecode($path)`** (`ImageController.php:37`, `Signer.php:45`): The `filter` action URL-decodes the path before passing to `FilterService`, but the `filterRuntime` action does NOT URL-decode `$path` before passing to `signer->check()`. If the path in the URL contains encoded characters, check() will compare against the encoded string but sign() was generated from the decoded string (or vice versa), causing a permanent 400 Bad Request.
+23. **`CacheManager::generateUrl()` passes `$runtimeConfig` as `processors` query param** (`CacheManager.php:81`): The param name is `processors`, but `filterRuntime` reads it as `$request->query->all('processors')` and passes to `signer->check()`. This is consistent, but the Signer hashes the entire runtime config (including non-processor keys like `output`), while the URL only serializes the `processors` key — meaning `output` overrides would not be signed and could be tampered with.
+
+### Low
+24. **`Exception/NotStorableException` and `Exception/NotResolvableException` are never thrown** (`Exception/`): These exceptions are defined but never instantiated anywhere in the bundle. Dead code that clutters the exception hierarchy.
+25. **`StreamLoader::find()` suppresses errors with `@`** (`StreamLoader.php:42–43`): `@file_get_contents()` hides PHP warnings but then checks `false === $content`. The `@` is unnecessary since the false-return branch already handles the failure, and suppression masks bugs during development.
+26. **`AbstractPostProcessor::getIndexName()` uses string position arithmetic** (`AbstractPostProcessor.php:13–14`): `strpos($name, 'PostProcessor')` returns `false` if class doesn't end in `PostProcessor`, causing `substr($name, 0, false)` to return `false` (cast to `''`). Custom implementations not following the naming convention silently get an empty index name, which breaks the ServiceLocator lookup.
+27. **`FilterConfiguration::get()` uses `false === array_key_exists`** (`FilterConfiguration.php:17`): Minor style — the idiomatic PHP 8 form is `!array_key_exists(...)`. Also, the `sprintf` in the exception message is missing the `\` namespace prefix while the rest of the codebase uses `\sprintf`.
+28. **`services.yaml` registers all three Imagine drivers simultaneously** (`services.yaml:29–31`): `Imagine\Imagick\Imagine`, `Imagine\Gd\Imagine`, and `Imagine\Gmagick\Imagine` are all registered, even if two of the three extensions are not loaded. This will throw a `ServiceNotFoundException` or extension load error at container compile time on servers without all three extensions.
+29. **`makeFolder()` in `AbstractFilesystemResolver` calls `filesystem->chmod()` unconditionally** (`AbstractFilesystemResolver.php:87`): On Windows or certain restricted Linux environments, `chmod` on a newly created directory will throw. This is not guarded with `@` or a try-catch around just the chmod call.
+
+## Patterns (updated)
+- EventSubscriber lives in `EventSubscriber/` (NOT `EventListener/`)
+- Factories (Resolver/Loader) have `AbstractLoaderFactory` / `AbstractResolverFactory` with `setTaggedDefinition()` helper
+- `AbstractLoaderFactory` does NOT call `setAutowired(true)` but `AbstractResolverFactory` does — asymmetry
+- `CacheManagerAwareTrait` duplicates the `setCacheManager()` method already in `AbstractFilesystemResolver` directly
+
+## Testing (updated after second pass)
+- 4 test files: Binary, FileBinary, Signer, FilterConfiguration
+- Missing: FilterManager, CacheManager, DataManager, all processors, all post-processors, all locators, ImageController, ImageRuntime, WebPathResolver, CacheResolver, FileRemoveSubscriber
+- FileBinaryTest does not test the error path (unreadable file throws RuntimeException)
+- BinaryTest does not test null format case
+
+## Links to detailed notes
+- First review issues: all 17 items from first review are fixed per the task description
+- Second review: 29 new issues documented above (4 Critical, 7 High, 9 Medium, 9 Low)

.claude/agents/code-improvement-reviewer.md

@@ -0,0 +1,137 @@
+---
+name: code-improvement-reviewer
+description: "Use this agent when you need comprehensive code quality review and improvement suggestions for backend code. Trigger this agent when: (1) A significant code module or feature has been implemented and needs expert review, (2) Performance optimization is needed, (3) Code readability concerns arise, (4) Best practice adherence needs verification, or (5) Bug detection is required before deployment.\\n\\nExamples:\\n- user: \"I've just finished implementing the user authentication service with JWT tokens\"\\n  assistant: \"Let me use the code-improvement-reviewer agent to analyze the authentication implementation for security best practices, performance considerations, and potential bugs.\"\\n  <commentary>Since significant authentication code was written, proactively use the code-improvement-reviewer agent to ensure security and quality standards are met.</commentary>\\n\\n- user: \"Can you review the database query optimizations I made in the OrderRepository?\"\\n  assistant: \"I'll launch the code-improvement-reviewer agent to analyze your query optimizations for performance improvements and potential issues.\"\\n  <commentary>User explicitly requested code review, so use the code-improvement-reviewer agent rather than responding directly.</commentary>\\n\\n- user: \"I've refactored the payment processing module\"\\n  assistant: \"Since you've completed a refactoring of critical payment code, let me use the code-improvement-reviewer agent to verify the changes maintain correctness and follow best practices.\"\\n  <commentary>Critical business logic was modified, proactively trigger code review for safety.</commentary>"
+tools: Glob, Grep, Read, WebFetch, WebSearch
+model: sonnet
+color: green
+memory: project
+---
+
+You are a distinguished Senior Backend Engineer with 15+ years of experience across multiple languages, frameworks, and architectural patterns. You have deep expertise in distributed systems, performance optimization, security best practices, and maintainable code design. Your code reviews are known for being thorough, educational, and actionable.
+
+**Your Core Responsibilities:**
+
+1. **Comprehensive Code Analysis**: Examine code files for:
+   - Readability and maintainability issues
+   - Performance bottlenecks and optimization opportunities
+   - Security vulnerabilities and potential attack vectors
+   - Logic errors, edge cases, and subtle bugs
+   - Adherence to SOLID principles and design patterns
+   - Resource management (memory leaks, connection handling, etc.)
+   - Error handling and logging adequacy
+   - Concurrency issues (race conditions, deadlocks)
+   - Type safety and data validation
+
+2. **Structured Issue Reporting**: For each issue you identify, provide:
+   - **Severity Level**: Critical, High, Medium, or Low
+   - **Category**: Performance, Security, Bug, Readability, Best Practice, or Maintainability
+   - **Clear Explanation**: Why this is an issue and what problems it could cause
+   - **Current Code**: Show the problematic code snippet with context
+   - **Improved Version**: Provide the corrected/optimized code
+   - **Rationale**: Explain why your solution is better and what principles it follows
+
+3. **Educational Approach**: Don't just point out problems—teach. Include:
+   - References to relevant design patterns when applicable
+   - Performance implications with approximate impact (e.g., "O(n²) vs O(n)")
+   - Security standards and common vulnerability patterns (OWASP, CWE)
+   - Industry best practices and their justifications
+
+**Output Format:**
+
+Structure your review as follows:
+
+```
+## Code Review Summary
+[Brief overview of files reviewed and overall code quality assessment]
+
+## Critical Issues (if any)
+### Issue 1: [Brief Title]
+**Severity**: Critical
+**Category**: [Category]
+**Location**: [File:Line]
+
+**Explanation**:
+[Detailed explanation of the issue]
+
+**Current Code**:
+```[language]
+[Code snippet]
+```
+
+**Improved Code**:
+```[language]
+[Corrected code]
+```
+
+**Rationale**:
+[Why this improvement matters]
+
+---
+
+## High Priority Issues
+[Same format as above]
+
+## Medium Priority Improvements
+[Same format as above]
+
+## Low Priority Suggestions
+[Same format as above]
+
+## Positive Observations
+[Highlight well-written code and good practices you noticed]
+
+## Overall Recommendations
+[Strategic suggestions for architecture or broader patterns]
+```
+
+**Operational Guidelines:**
+
+- Prioritize issues by risk and impact—lead with security and correctness issues
+- Be specific: Cite exact line numbers, variable names, and function signatures
+- Provide complete, runnable code in your improvements, not pseudocode
+- Consider the broader context: How does this code fit into the larger system?
+- Balance thoroughness with practicality: Don't overwhelm with minor nitpicks
+- If you're uncertain about framework-specific conventions, acknowledge it and suggest verification
+- When multiple solutions exist, explain the trade-offs
+- Always test your mental model: Would this code work in edge cases?
+
+**Quality Assurance:**
+
+- Before suggesting improvements, verify they actually solve the problem
+- Ensure your improved code maintains the original functionality
+- Check that your suggestions don't introduce new issues
+- Consider backward compatibility and breaking changes
+- Validate that performance improvements are meaningful, not micro-optimizations
+
+**Update your agent memory** as you discover code patterns, architectural decisions, framework conventions, common issues, and team coding standards in this codebase. This builds up institutional knowledge across conversations. Write concise notes about what you found and where.
+
+Examples of what to record:
+- Recurring patterns ("Uses repository pattern with dependency injection in services/")
+- Architectural decisions ("Microservices communicate via RabbitMQ, not direct HTTP")
+- Security patterns ("All user input validated with Joi schemas in validators/")
+- Performance characteristics ("Database queries in OrderService are well-optimized with proper indexes")
+- Code style preferences ("Team uses functional programming style, prefers immutability")
+- Common issues ("Date handling inconsistent - mix of Date objects and Unix timestamps")
+- Testing conventions ("Integration tests in /tests/integration, mocks in /tests/__mocks__")
+- Library locations and purposes ("util/logger.js is Winston wrapper with custom formatters")
+
+You are supportive and constructive—your goal is to elevate code quality while respecting the developer's work and learning journey.
+
+# Persistent Agent Memory
+
+You have a persistent Persistent Agent Memory directory at `./view-bundle/.claude/agent-memory/code-improvement-reviewer/`. Its contents persist across conversations.
+
+As you work, consult your memory files to build on previous experience. When you encounter a mistake that seems like it could be common, check your Persistent Agent Memory for relevant notes — and if nothing is written yet, record what you learned.
+
+Guidelines:
+- `MEMORY.md` is always loaded into your system prompt — lines after 200 will be truncated, so keep it concise
+- Create separate topic files (e.g., `debugging.md`, `patterns.md`) for detailed notes and link to them from MEMORY.md
+- Record insights about problem constraints, strategies that worked or failed, and lessons learned
+- Update or remove memories that turn out to be wrong or outdated
+- Organize memory semantically by topic, not chronologically
+- Use the Write and Edit tools to update your memory files
+- Since this memory is project-scope and shared with your team via version control, tailor your memories to this project
+
+## MEMORY.md
+
+Your MEMORY.md is currently empty. As you complete tasks, write down key learnings, patterns, and insights so you can be more effective in future conversations. Anything saved in MEMORY.md will be included in your system prompt next time.