Skip to content

POK-91: Fix pluck's broken file ops and import-time crash#33

Merged
saheljalal merged 1 commit into
mainfrom
pok-91-fix-pluck-broken-file-ops
Jul 12, 2026
Merged

POK-91: Fix pluck's broken file ops and import-time crash#33
saheljalal merged 1 commit into
mainfrom
pok-91-fix-pluck-broken-file-ops

Conversation

@saheljalal

Copy link
Copy Markdown
Collaborator

Summary

pluck's characterization suite was failing (9 tests, POK-91), but that was only the visible tip of a tool that was non-functional across the board. This restores pluck to working order and adds tests that actually cover the tool's core path.

Three independent, committed-on-main bugs, all fixed here:

  1. Import-time crash (the reported symptom). import tomli_w was unguarded, so when the optional write-side dep was absent the entire module failed to import β€” taking down every unrelated command and, via the shared test loader, reporting all 9 pluck characterization tests as failing (AttributeError: module '_tool_pluck' has no attribute 'parse_path', etc.). This is exactly what happened in the POK-84 aikit QA venv (base + aikit deps + ruamel.yaml, but no tomli-w). Fixed by importing tomli_w lazily like ruamel.yaml: reading TOML uses stdlib tomllib, and writing TOML without tomli-w now raises a clean PluckError instead of crashing.

  2. HANDLERS name collision β†’ every file op broken. The command-dispatch dict HANDLERS (added when pluck was refactored onto scriptkit's sk.dispatch in 1f2b7a06) shadowed the pre-existing format-handler map of the same name. So handler_for() looked a ConfigFormat enum up in the command table and every get/set/copy/merge/diff crashed with an unhandled KeyError on all four formats. No test caught this. Renamed the format map to FORMAT_HANDLERS.

  3. TOML write str/bytes mismatch. tomli_w.dumps() returns str, but save()/dump_preview()/the get display path treated it as bytes (path.write_bytes(...) / .decode("utf-8")), so any TOML write or preview raised TypeError/AttributeError. Fixed to write text.

Why the suite didn't catch bugs 2 & 3

The characterization tests pinned only pure helpers + --help β€” never the dispatch β†’ handler_for β†’ load/save path, which is the whole tool. This PR adds end-to-end get/set round-trip tests across all four formats. They fail on the pre-fix tool (verified by reverting pluck and re-running) and pass after.

Verification

  • Full suite: 407 passed (was 9 failed / 390 passed; +8 new round-trip tests).
  • Manual: get/set/copy/diff/doctor/formats/--version all exercised across yaml Β· json Β· toml Β· env, including cross-format copy (TOML write).
  • Graceful degradation: without tomli-w, --help and the characterization suite still pass, TOML read works, and TOML write fails with a clean ❌ tomli-w is required to write TOML (pip install tomli-w) (no traceback).

Bumps pluck 1.1.0 β†’ 1.1.1 (PATCH β€” bug fixes, no interface change) with a CHANGELOG entry.

Closes POK-91

πŸ€– Generated with Claude Code

@saheljalal saheljalal left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review β€” POK-91: pluck import crash & broken file ops

Verdict: Approve, with one should-fix. The diagnosis is right and the three root causes are real, correctly fixed, and I reproduced the before/after behavior in a tomli_w-absent venv (the POK-84 environment).

Verified

  • Lazy tomli_w β€” module now imports clean without the dep (import OK, version 1.1.1, tomli_w is None True). The characterization loader no longer caches a half-initialized module. βœ…
  • HANDLERS β†’ FORMAT_HANDLERS split (pluck:588, pluck:597) β€” handler_for now indexes the format map; the command-dispatch HANDLERS at pluck:1106 is distinct. JSON/env set round-trips work end to end. βœ…
  • str/bytes fix (pluck:546, pluck:549, pluck:707) β€” tomli_w.dumps() returns str; write_text(..., encoding="utf-8") and .rstrip() on str are correct. βœ…
  • Write boundary you flagged β€” no write path assumes the dep at import. pluck set config.toml … without tomli-w fails with a clean PluckError (tomli-w is required to write TOML), rc=1, no traceback. βœ…

Should-fix

  • pluck:706 (show_value, TOML branch) β€” the default (non---json) pluck get config.toml key calls toml_writer().dumps(value), so a pure read requires the write dep. In the exact tomli-w-absent venv this issue came from, pluck get config.toml web.port errors instead of printing 8080, while --json works and the file load itself works. This contradicts the PR's own "reading TOML uses stdlib tomllib; only writing needs tomli_w" framing.
    • Why it matters: get is the most common read command; erroring on the default display for a supported read is surprising, and it's the precise scenario POK-91 targets.
    • Suggested fix: when tomli_w is None, degrade the TOML display to a writer-free rendering (reuse the JSON pretty-print, or str(value)) instead of raising β€” reads should never need the writer.

Nit (optional)

  • The new test_pluck_get_reads_value exercises only --json, so the default-display path above stays untested. A default-display assertion (or the fallback fix) would pin the read-without-writer path that this PR is fundamentally about.
  • Pre-existing, out of scope: EnvHandler.save (pluck:569–573) has an if/else whose two branches call set_key identically β€” dead branching, harmless. Not introduced here; noting only.

Nice work closing the test gap that let bugs #2/#3 ship invisibly β€” the dispatch β†’ handler_for β†’ load/save round-trips are exactly the coverage that was missing. CHANGELOG and the 1.1.0 β†’ 1.1.1 PATCH bump are accurate.

saheljalal added a commit that referenced this pull request Jul 3, 2026
Addresses the Code Reviewer's should-fix on PR #33: the default (non-JSON)
`pluck get config.toml key` display routed through `show_value`'s TOML branch,
which called the tomli-w writer β€” so a pure TOML *read* failed in the exact
tomli-w-absent venv this issue targets, contradicting the "reading needs no
writer" guarantee (reading uses stdlib tomllib).

`show_value` now renders a TOML value as TOML only when it can (value is a
mapping and tomli-w is present) and falls back to JSON otherwise. This also
fixes a latent crash the fix surfaced: `tomli_w.dumps()` needs a top-level
table, so displaying a bare scalar/list (`get config.toml a.scalar`) raised
`AttributeError: 'int' object has no attribute 'items'` even with the writer
installed β€” the round-trip tests missed it by reading via `--json`.

Tests: extended the get characterization test to also exercise the default
display path (not just `--json`), and added a test that shadows tomli-w as
unimportable to pin that a plain TOML `get` still works writer-free (fails on
the pre-fix code, passes after). Full suite 408 passed. Coalesced under the
existing uncommitted 1.1.1 changelog entry.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>
saheljalal added a commit that referenced this pull request Jul 3, 2026
POK-91: Fix pluck's broken file ops and import-time crash
@saheljalal

Copy link
Copy Markdown
Collaborator Author

Addressed in 164f996 (approved review's should-fix + nit).

Should-fix β€” show_value TOML branch: the default (non---json) get/diff TOML display no longer calls the writer. It renders as TOML only when the value is a mapping and tomli-w is present, else falls back to JSON β€” so pluck get config.toml web.port now prints 8080 in a tomli-w-absent venv (reading stays on stdlib tomllib). This also fixed a latent crash the change surfaced: tomli_w.dumps() requires a top-level table, so displaying a bare scalar/list (get config.toml a.scalar) raised AttributeError: 'int' object has no attribute 'items' even with the writer installed β€” the round-trips missed it by reading via --json.

Nit β€” untested default display: extended test_pluck_get_reads_value to exercise the default panel path (not just --json), and added test_pluck_get_toml_display_needs_no_writer, which shadows tomli_w as unimportable to pin the read-without-writer path (fails pre-fix, passes after).

EnvHandler dead branch: left as-is per your "pre-existing / out of scope / noting only".

Full suite 408 passed; CI green on 164f996. There were no inline review threads to mark resolved (the review is a single review body), so noting resolution here.

Addresses the Code Reviewer's should-fix on PR #33: the default (non-JSON)
`pluck get config.toml key` display routed through `show_value`'s TOML branch,
which called the tomli-w writer β€” so a pure TOML *read* failed in the exact
tomli-w-absent venv this issue targets, contradicting the "reading needs no
writer" guarantee (reading uses stdlib tomllib).

`show_value` now renders a TOML value as TOML only when it can (value is a
mapping and tomli-w is present) and falls back to JSON otherwise. This also
fixes a latent crash the fix surfaced: `tomli_w.dumps()` needs a top-level
table, so displaying a bare scalar/list (`get config.toml a.scalar`) raised
`AttributeError: 'int' object has no attribute 'items'` even with the writer
installed β€” the round-trip tests missed it by reading via `--json`.

Tests: extended the get characterization test to also exercise the default
display path (not just `--json`), and added a test that shadows tomli-w as
unimportable to pin that a plain TOML `get` still works writer-free (fails on
the pre-fix code, passes after). Full suite 408 passed. Coalesced under the
existing uncommitted 1.1.1 changelog entry.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>
@saheljalal saheljalal force-pushed the pok-91-fix-pluck-broken-file-ops branch from 164f996 to 690c89f Compare July 12, 2026 09:36
@saheljalal saheljalal merged commit c0826fa into main Jul 12, 2026
1 check passed
@saheljalal saheljalal deleted the pok-91-fix-pluck-broken-file-ops branch July 12, 2026 09:37
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