Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. Unhandled cache.put failure in quality.ts — src/libswamp/extensions/quality.ts:132

   In the quality generator, await deps.cache.put(hash, archiveBytes, {...}) is not wrapped in a try-catch. If the disk is full, permissions are denied, or any filesystem error occurs, this will throw an unhandled error that crashes the quality command after packaging succeeded but before scoring runs. By contrast, the push command (extension_push.ts:423-432) correctly wraps cache.put in a try-catch with a debug log.

   Breaking example: Run swamp extension quality on a filesystem where .swamp/cache/packages/ is on a read-only mount or quota-exceeded volume. The command crashes instead of gracefully continuing to score.

   Suggested fix: Wrap the cache.put call the same way push does:

   try {
     await deps.cache.put(hash, archiveBytes, { ... });
   } catch (cacheError) {
     ctx.logger.debug`Failed to write package cache (continuing): ${cacheError}`;
   }

2. packageJsonDir not passed in quality command — src/cli/commands/extension_quality.ts:124-149

   The quality command's prepareInput never sets packageJsonDir. The push command (extension_push.ts:233-265) has an entire block to detect package.json and resolve node_modules for extensions that use bare npm specifiers. The quality command skips all of this. For extensions that depend on package.json-based npm resolution, swamp extension quality will fail during bundling with a confusing module-not-found error, while swamp extension push succeeds.

   Breaking example: An extension with import Foo from "some-npm-package" in its entrypoint, a package.json with that dependency, and no deno.json. Push succeeds (finds package.json, sets packageJsonDir); quality fails.

   Suggested fix: Port the findPackageJson + bare-specifier detection block from extension_push.ts into extension_quality.ts, or extract it into a shared helper.

3. packageJsonPath always undefined in cache hash — src/cli/commands/extension_push.ts:290, src/cli/commands/extension_quality.ts:162

   Both commands hardcode packageJsonPath: undefined in the hash input. The PackageCacheHashInput interface has the field and computePackageCacheHash would include its contents if set. If a user changes a package.json dependency version (which affects bundling output), the cache hash stays the same and stale bytes are served. This is consistent between quality and push (both get the same wrong hash), but the cache contract — "any source change invalidates by construction" — is violated for package.json changes.

   Suggested fix: When packageJsonDir is set, resolve the package.json path and pass it in the hash input.

Low

1. toFileUrl misparses paths with #, ?, or unescaped % — src/domain/extensions/extension_rubric_scorer.ts:687-688

   new URL("file://" + abs) treats # as a fragment delimiter and ? as a query delimiter. A path like /tmp/my#project/model.ts would produce a URL with pathname /tmp/my and hash #project/model.ts, silently looking up the wrong deno doc node. In practice this requires # in the temp dir path (created with prefix: "swamp_quality_") or in the manifest entrypoint name, which is unlikely.

   Suggested fix: Use Deno's toFileUrl() from @std/path/to-file-url which handles special characters correctly.

2. platforms-one is a tautology — src/domain/extensions/extension_rubric_scorer.ts:347

   manifest.platforms.length === 0 || manifest.platforms.length >= 1 is always true for any non-negative number. This factor can never be "missing". The parity tests confirm this matches the server's behavior (the server also always earns this factor), so it's not a bug per se — but the condition is misleading. A reader might think it's possible to fail this factor. No fix needed if server parity is the priority.

Verdict

PASS — No critical or high severity issues. The medium findings are real functional gaps (cache.put crash, missing packageJson support in quality, stale cache on package.json changes) but are edge-case paths that won't affect the majority of users. The code is well-structured with thorough testing (especially the parity tests), proper path-traversal protection in collectEntrypoints, and appropriate temp-dir cleanup in finally blocks. The refactoring of push.ts to extract bundleAndArchive is clean and preserves behavior.

Code Review

Clean, well-layered PR with excellent test coverage (80 tests across unit and integration). DDD separation is solid: pure domain logic in extension_rubric_scorer.ts, repository-pattern cache in extension_package_cache.ts, orchestration in the libswamp generator, and decoupled presentation renderers.

Blocking Issues

None.

Suggestions

1. findDenoConfig duplication — extension_quality.ts and extension_push.ts each have their own findDenoConfig helper (the quality command's comment acknowledges this). Consider extracting to a shared utility in src/cli/ if a third caller ever appears.

2. Custom toFileUrl / encodeHex — extension_rubric_scorer.ts has a hand-rolled toFileUrl (new URL("file://" + abs).href) and extension_package_cache.ts has a hand-rolled encodeHex. The standard library provides toFileUrl from @std/path and encodeHex from @std/encoding/hex, which handle edge cases (paths with spaces/special chars, etc.) more robustly. Low risk since paths are temp-dir generated, but swapping in the std-lib versions would be more idiomatic.

3. packageJsonPath always undefined — Both the quality command and push command pass packageJsonPath: undefined in their cache hash inputs. If an extension project uses a package.json that affects the build, changes to it won't invalidate the cache. The field is in the interface for future use, but a comment noting the gap would help.

4. platforms-one is tautologically true — manifest.platforms.length === 0 || manifest.platforms.length >= 1 is always true (any array satisfies this). This mirrors the server's behavior (the factor is effectively a free point), but a one-line comment confirming the intentional parity would help future readers not "fix" it.

CLI UX Review

Blocking

None.

Suggestions

1. JSON mode emits two documents on failure (src/presentation/renderers/extension_quality.ts, JsonExtensionQualityRenderer.completed). When factors are missing the output is:

   • stdout: score JSON object, then the framework's error JSON object (two separate top-level documents)
   • The integration test acknowledges this with a custom parseFirstJson helper whose docstring explicitly says "exits non-zero in JSON mode, it prints the score object followed by an error object"

   A script doing result=$(swamp extension quality manifest.yaml --json); echo "$result" | jq .factors will get a jq parse error when any factor fails. The fix would be to buffer the score in the renderer and not console.log it in the completed handler, then let the action handler emit one final document (either the score on pass, or an error on failure). For now, the primary CI use case—check the exit code and ignore stdout—is unaffected.

2. repository-verified caveat is always printed in log mode (extension_quality.ts, LogExtensionQualityRenderer.completed). The note appears unconditionally after every scoring run, even on a 12/12 perfect score. Consider printing it only when repository-verified was earned (local structural check passed, server may still reject) rather than emitting it on every run including full-pass runs where it reads as a warning for something that didn't fail.

3. Minor inconsistency in log renderer (extension_quality.ts): packaging and scoring handlers use the call form logger.info("…") while cache_hit uses the template literal form logger.info\…``. One form or the other throughout is cleaner.

4. Command description "10 client-earnable factors" vs. 12 max points (extension_quality.ts, .description(...)): the description is accurate for factor count, but users who see 12/12 in output may wonder why it doesn't say 12. A parenthetical like "10 factors, 12 total points" in the description would head off confusion.

Verdict

PASS — both output modes are implemented, exit codes are correct, flag names are consistent with extension push and extension fmt, error messages are actionable, and the help text and examples are clear. Suggestions above are all minor and none block merge.