Skip to content

fix: strip inherited class definitions from extends and remote amends#48

Merged
jdx merged 3 commits intomainfrom
fix/strip-classes-extends-and-remote
Mar 24, 2026
Merged

fix: strip inherited class definitions from extends and remote amends#48
jdx merged 3 commits intomainfrom
fix/strip-classes-extends-and-remote

Conversation

@jdx
Copy link
Copy Markdown
Owner

@jdx jdx commented Mar 24, 2026

Summary

Follow-up to #45 — the class-definition leak also affected:

  • extends (both local file and HTTP paths)
  • amends with HTTP and package:// URIs

Changes:

  • The amends class injection pass now resolves base source for all URI schemes (HTTP via http_cache, package:// via extracted dirs), not just local files
  • The extends path now calls base_obj.shift_remove() for each class definition, matching the amends fix

Test plan

  • Added amends_strips_inherited_class_definitions test
  • Added extends_strips_inherited_class_definitions test
  • All 227 tests pass

This PR was generated by Claude Code.

@gemini-code-assist
Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Follow-up to #45: the class-definition leak also affected:
- `extends` (both local file and HTTP)
- `amends` with HTTP and package:// URIs

The amends class injection pass now resolves base source for all URI
schemes (HTTP via http_cache, package:// via extracted dirs), not just
local files. The extends path now calls `base_obj.shift_remove()` for
each class definition, matching the amends fix.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jdx jdx force-pushed the fix/strip-classes-extends-and-remote branch from 69dbd4e to a5622ed Compare March 24, 2026 01:36
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Mar 24, 2026

Greptile Summary

This PR extends the class-definition stripping fix from #45 to cover amends with HTTP and package:// URIs and extends with HTTP URIs. It adds a second if let Some(uri) = &module.amends pass that resolves the base source for all URI schemes — reading from http_cache for HTTP, from the extracted zip directory for package:// Zip packages, and from disk for local files — and then calls shift_remove on base_obj for each discovered ClassDef entry. The same shift_remove call is added to the extends branches.

Key observations:

  • The new logic correctly handles the HTTP and local-file cases, matching the pattern from the pre-existing amends code.
  • In the package:// Zip branch of the injection pass, the zip URL is discarded (bound to _) and replaced with an unsafe find_map over all extracted package directories; this could read source from the wrong package when multiple packages contain a file at the same internal path. The zip URL is available from resolve_package_uri and should be used as the direct key into package_dirs.
  • The two new tests only cover local-file amends and extends; the HTTP and package:// code paths added by this PR have no test coverage.

Confidence Score: 3/5

  • Safe to merge for the common local-file case, but the package:// Zip path has a correctness bug and the new HTTP/package:// branches are untested.
  • The local-file fix is correct and tested. The HTTP fix looks correct (cache key matches what fetch_source stores). However, the Zip branch discards the known zip URL and uses find_map across all package directories, which can return content from the wrong package. Additionally, no tests cover the HTTP or package:// amends/extends paths that this PR claims to fix.
  • src/eval.rs — specifically the PackageSource::Zip arm in the amends class-injection block (around line 410).

Important Files Changed

Filename Overview
src/eval.rs Adds HTTP/package:// base-source resolution for the amends class-injection pass and shift_remove calls for both amends and extends; the Zip branch discards its known zip_url and uses an unsafe find_map fallback instead.
tests/pkl_features.rs Adds two new local-file tests that validate class stripping for amends and extends; HTTP and package:// paths mentioned in the PR description are not covered by tests.
tests/fixtures/base_with_class.pkl New fixture file with a class definition and a default property, used by the two new test cases.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[eval_module called] --> B{module.amends?}
    B -- yes --> C{URI scheme}
    C -- http/https --> D[fetch_source → eval_module → base_obj]
    C -- package:// --> E{PackageSource}
    E -- Direct --> F[fetch_source → eval_module → base_obj]
    E -- Zip --> G[extract_package_zip → eval_file → base_obj]
    C -- local/file:// --> H[read file → eval_file → base_obj]
    C -- pkl: --> I[skip]

    D & F & G & H --> J{Second pass: inject class defs}
    J -- http/https --> K[http_cache.get uri]
    J -- package:// Direct --> L[http_cache.get resolved_url]
    J -- package:// Zip --> M["find_map over ALL package_dirs ⚠️"]
    J -- local/file:// --> N[fs::read_to_string path]

    K & L & M & N --> O{parse source OK?}
    O -- yes --> P[for each ClassDef: eval + scope.set + base_obj.shift_remove ✅]
    O -- no --> Q[skip class injection]

    B -- no --> R{module.extends?}
    R -- local/file:// --> S[read + parse + eval_module → base_obj\nfor each ClassDef: shift_remove ✅]
    R -- http/https --> T[fetch_source + parse + eval_module → base_obj\nfor each ClassDef: shift_remove ✅]
    R -- package:// --> U[not handled — silently skipped]
Loading

Comments Outside Diff (1)

  1. tests/pkl_features.rs, line 952-988 (link)

    P2 No coverage for HTTP or package:// amends class stripping

    The two new tests only exercise local-file amends and local-file extends. The PR description explicitly calls out that this fix also covers:

    • amends with https:// / http:// URIs
    • amends with package:// URIs (both Direct and Zip sources)
    • extends with HTTP URIs

    None of those code paths have corresponding tests, so a regression in the HTTP-cache lookup or the zip-directory lookup would go undetected. Since network calls can be stubbed or mocked, adding at least an integration-style test for the HTTP amends path would significantly improve confidence in the fix.

    Fix in Claude Code

Fix All in Claude Code

Reviews (1): Last reviewed commit: "[autofix.ci] apply automated fixes" | Re-trigger Greptile

Comment thread src/eval.rs Outdated
Comment on lines +410 to +415
PackageSource::Zip(_, entry) => {
// For zip packages, read from the extracted directory
self.package_dirs
.values()
.find_map(|dir| std::fs::read_to_string(dir.join(entry)).ok())
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Zip URL discarded — wrong package directory may be read

The zip URL in the PackageSource::Zip arm is bound to _ and discarded. The code then iterates over all extracted package directories with find_map, returning the first one that contains a file at entry. Because package_dirs is already keyed by the zip URL (set in extract_package_zip), the correct directory is already known. If two different package:// URIs share an identical internal entry path, find_map will silently return content from the wrong package.

The fix is to bind the zip URL and use it directly for the lookup:

PackageSource::Zip(zip_url, entry) => {
    self.package_dirs.get(&zip_url).and_then(|dir| {
        std::fs::read_to_string(dir.join(entry)).ok()
    })
}

Fix in Claude Code

Addresses review feedback: the PackageSource::Zip arm was discarding the
zip URL and iterating all package_dirs with find_map, which could return
content from the wrong package when multiple packages share an internal
entry path. Now uses the zip URL directly as the HashMap key.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jdx jdx merged commit fa704fc into main Mar 24, 2026
6 checks passed
@jdx jdx deleted the fix/strip-classes-extends-and-remote branch March 24, 2026 01:43
@jdx jdx mentioned this pull request Mar 24, 2026
jdx added a commit that referenced this pull request Mar 24, 2026
## 🤖 New release

* `pklr`: 0.2.1 -> 0.2.2 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [0.2.2](v0.2.1...v0.2.2) -
2026-03-24

### Added

- add HTTP URL rewrite support
([#46](#46))

### Fixed

- strip inherited class definitions from extends and remote amends
([#48](#48))
- strip inherited class definitions from amends output
([#45](#45))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).
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.

1 participant