fix: strip inherited class definitions from amends output by jdx · Pull Request #45 · jdx/pklr

jdx · 2026-03-24T01:08:37Z

Summary

When a module amends a base that defines classes (e.g., class Script), the class values were included in the JSON output at the top level
This broke consumers using serde's deny_unknown_fields (e.g., hk's Config struct)
Fix: remove inherited class definitions from base_obj during the amends class injection pass

Test plan

Added amends_strips_inherited_class_definitions test
All 226 existing tests pass

This PR was generated by Claude Code.

gemini-code-assist · 2026-03-24T01:08:41Z

Warning

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

When a module amends a base that defines classes (e.g., Config.pkl defining `class Script`), the class values were included in the base object at depth > 0 but never removed from the amending module's output at depth 0. This caused unexpected fields like "Script" to appear in the JSON output, breaking consumers that use deny_unknown_fields (e.g., hk's Config struct). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

greptile-apps · 2026-03-24T01:12:36Z

Greptile Summary

This PR fixes a bug where class definitions declared in an amended base module leaked into the JSON output of the amending module, breaking consumers that use serde's deny_unknown_fields. It also ships an unrelated new feature: a custom HTTP client API (set_http_client / eval_to_json_with_client) plus a reqwest re-export.

Key changes:

src/eval.rs: Adds base_obj.shift_remove(name) inside the amends class-injection pass to remove inherited class definitions from the base object before it is used as the amending module's starting state.
src/lib.rs: Introduces eval_to_json_with_client(path, Option<reqwest::Client>) and pub use reqwest so callers can configure proxy/CA settings without adding a direct reqwest dependency.
tests/: New base_with_class.pkl fixture and amends_strips_inherited_class_definitions test verify the fix.

Issues found:

The extends code path has the identical class-definition leak: eval_module at depth + 1 keeps class defs in its output, and class_names only tracks the current module's own class defs, so inherited ones are never stripped at depth 0.
package:// and HTTP amends paths load the base with eval_file/eval_module at depth + 1 but are excluded from the class injection pass (and thus from shift_remove), leaving the same potential leak for those URI schemes.
The PR bundles two unrelated concerns (the bug fix and the HTTP client feature) into a single change, which complicates bisecting and review.

Confidence Score: 3/5

Safe to merge for the local-file amends bug fix, but the same class-definition leak remains in the extends path and for non-local amends URIs.
The specific fix for local-file amends is correct and well-tested. However, the symmetrical extends code path has the same unaddressed bug (class defs from the extended base still appear in output), and package:// / HTTP amends are also unaffected by the fix. These are likely to surface as follow-up issues.
Pay close attention to src/eval.rs, particularly the extends block (lines 434–498) and the package:// / HTTP amends branches (lines 345–375).

Important Files Changed

Filename	Overview
src/eval.rs	Core fix: `shift_remove` correctly strips class definitions from `base_obj` in the local-file `amends` path. However, the same leak exists in the `extends` path and for `package://`/HTTP `amends`, which are not addressed.
src/lib.rs	Adds `eval_to_json_with_client` and re-exports `reqwest`. The logic is correct; `reqwest` re-export introduces a semver coupling concern if consumers also depend on reqwest directly.
tests/pkl_features.rs	New `amends_strips_inherited_class_definitions` test correctly validates the fix; coverage is limited to local-file amends only.
tests/fixtures/base_with_class.pkl	Minimal fixture defining `class Script` and a `name` property; sufficient to reproduce the reported bug.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["eval_module(module, path, depth)"] --> B{module.amends?}
    B -- "local file / file://" --> C["eval_file(base, depth+1)\n→ base_obj includes class defs\n(depth+1 ≠ 0, not stripped)"]
    B -- "http/https" --> D["eval_module(base, depth+1)\n→ base_obj includes class defs"]
    B -- "package://" --> E["eval_file(pkg_entry, depth+1)\n→ base_obj includes class defs"]
    B -- No --> Z

    C --> F["Class injection pass\n(local file only)"]
    F --> G["scope.set(name, defaults)\nbase_obj.shift_remove(name) ✅ FIX"]

    D --> H["No class injection pass\nclass defs remain in base_obj ⚠️"]
    E --> I["No class injection pass\nclass defs remain in base_obj ⚠️"]

    G --> Z
    H --> Z
    I --> Z

    Z["out = base_obj\n+ current module props"] --> AA{depth == 0?}
    AA -- Yes --> AB["strip OWN class_names\nstop own lambdas"]
    AA -- No --> AC["keep everything\n(for dotted access)"]
    AB --> OUT["Value::Object(out)"]
    AC --> OUT

    subgraph extends ["extends path (same leak)"]
      EX1["eval_module(base, depth+1)\n→ base_obj includes class defs"]
      EX2["scope.set class defs\nbut NO shift_remove ⚠️"]
      EX1 --> EX2
    end

Comments Outside Diff (3)

src/eval.rs, line 453-477 (link)

Same leak exists in extends code path

The fix correctly removes inherited class definitions from base_obj for amends, but the extends code path has the same bug. When a module uses extends, eval_module is called at depth + 1 (line 451), which means the base's class definitions are kept in its output (the if depth == 0 stripping at line 615 doesn't run). These class definitions end up in base_obj, but class_names (built at lines 529–536) only tracks the current module's own class defs — so inherited class defs from the extended base are never removed from out at depth 0.

A base module like:
```
class Script { linux: String? }
name = "base"
```
…extended by:
```
extends "base.pkl"
name = "child"
```
…would still emit "Script" in the JSON output. The same shift_remove pattern used for amends should be applied inside the extends class-injection loop.
src/eval.rs, line 356-375 (link)

package:// amends class definitions not stripped

For package:// amends that resolve to a zip entry (line 361), eval_file is called at depth + 1, so class definitions from the package base are included in base_obj (the depth-0 stripping doesn't apply). However, the class injection pass at lines 399–431 only handles !uri.starts_with("pkl:") && (!uri.contains("://") || uri.starts_with("file://")), so shift_remove is never called for package:// bases.

A package://-based amend whose module defines classes would still emit those classes in the JSON output. The same issue applies to HTTP amends (lines 345–355) where eval_module is called at depth + 1.

This may be lower-priority if package:// / HTTP amends with class definitions are uncommon in practice, but it's worth noting for completeness.
src/lib.rs, line 12 (link)

reqwest re-export creates a semver coupling hazard

Re-exporting reqwest is a handy pattern, but it binds consumers to the exact major version pklr uses (0.12). If a consumer also depends on reqwest and Cargo resolves two different minor/patch versions, any reqwest::Client built with the consumer's copy won't be accepted by pklr's set_http_client. The re-export is most useful when documented alongside a note that consumers should build their Client via pklr::reqwest::Client::builder() to avoid version mismatches.

_{Reviews (1): Last reviewed commit: "fix: strip inherited class definitions f..." | Re-trigger Greptile}

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>

…#48) ## 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 - [x] Added `amends_strips_inherited_class_definitions` test - [x] Added `extends_strips_inherited_class_definitions` test - [x] All 227 tests pass *This PR was generated by Claude Code.* --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

## 🤖 New release * `pklr`: 0.2.1 -> 0.2.2 (✓ API compatible changes) <details><summary>Changelog</summary> <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> </details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).

jdx force-pushed the fix/strip-inherited-classes branch from a39df41 to e46975a Compare March 24, 2026 01:12

jdx merged commit 513da23 into main Mar 24, 2026
6 checks passed

jdx deleted the fix/strip-inherited-classes branch March 24, 2026 01:32

jdx mentioned this pull request Mar 24, 2026

chore: release v0.2.2 #47

Merged

jdx mentioned this pull request Mar 24, 2026

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

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: strip inherited class definitions from amends output#45

fix: strip inherited class definitions from amends output#45
jdx merged 1 commit intomainfrom
fix/strip-inherited-classes

jdx commented Mar 24, 2026

Uh oh!

gemini-code-assist Bot commented Mar 24, 2026

Uh oh!

greptile-apps Bot commented Mar 24, 2026 •

edited

Loading

Comments Outside Diff (3)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jdx commented Mar 24, 2026

Summary

Test plan

Uh oh!

gemini-code-assist Bot commented Mar 24, 2026

Uh oh!

greptile-apps Bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Flowchart

Comments Outside Diff (3)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps Bot commented Mar 24, 2026 •

edited

Loading