Skip to content

chore: bump parquet/squirreling deps and polish install wizard output#78

Open
philcunliffe wants to merge 1 commit into
masterfrom
chore/deps-and-wizard-polish
Open

chore: bump parquet/squirreling deps and polish install wizard output#78
philcunliffe wants to merge 1 commit into
masterfrom
chore/deps-and-wizard-polish

Conversation

@philcunliffe
Copy link
Copy Markdown
Contributor

Summary

Bundles dependency bumps with install-wizard output polish.

Dependencies

  • hyparquet-writer 0.15.4 → 0.15.6 (optionalDependencies + overrides)
  • squirreling 0.12.22 → 0.12.23 (dependencies + overrides)

npm install resolves cleanly; icebird's transitive copies dedupe to the same versions.

Install wizard readability

  • Prompts redraw in place: new clearOnResolve TUI option erases a prompt's frame when it settles so the next prompt redraws in the same spot instead of stacking below it. Row counting now measures physical rows using the terminal width, so wrapped frames clear correctly.
  • Skills block spacing: the installed skill … → lines in the npx install finale now have a leading and trailing blank line, separating them from the preceding client-attach output and the following backfill prompt. Guarded so no stray blank lines appear when nothing installs.

Before:

  base_url = http://127.0.0.1:8787/backend-api/codex
installed skill 'hypaware-query' → /Users/phil/.claude/skills/hypaware-query
installed skill 'hypaware-ignore' → /Users/phil/.claude/skills/hypaware-ignore
installed skill 'hypaware-query' → /Users/phil/.codex/skills/hypaware-query
Import local claude, codex history now (last 30 days)?

After:

  base_url = http://127.0.0.1:8787/backend-api/codex

installed skill 'hypaware-query' → /Users/phil/.claude/skills/hypaware-query
installed skill 'hypaware-ignore' → /Users/phil/.claude/skills/hypaware-ignore
installed skill 'hypaware-query' → /Users/phil/.codex/skills/hypaware-query

Import local claude, codex history now (last 30 days)?

Testing

  • npm test — 675 pass
  • npm run lint — clean
  • npm run typecheck — clean

🤖 Generated with Claude Code

- Update hyparquet-writer 0.15.4 -> 0.15.6 and squirreling 0.12.22 ->
  0.12.23 (dependencies, optionalDependencies, and overrides).
- Redraw TUI prompts in place via a clearOnResolve option so successive
  wizard prompts no longer stack below one another; measure physical rows
  using terminal width so wrapped frames clear correctly.
- Separate the skills-install block from surrounding wizard output with
  leading/trailing blank lines so the npx install finale is readable.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@philcunliffe
Copy link
Copy Markdown
Contributor Author

Dual-agent review — request_changes

  • Verdict: request_changes
  • Risk class: low
  • Reviewers: Codex (gpt-5.3-codex, reasoning=xhigh) + Claude (5-Sonnet scan → Haiku confidence scoring) + inline blast-radius
  • Head: 0f5293795bcba84c996519e4b500f923ad8d2421

What drives the verdict (verified)

Export default is now path-dependent. Codex flagged this as a major (high-confidence) and it was independently confirmed against the code:

  • The interactive wizard now hardcodes exportChoice = 'local-parquet' (walkthrough.js:711) — the "Where should HypAware export captured data?" picker was removed.
  • But non-interactive hyp init --source X (no --yes, no --export) still resolves to keep-local (core_commands.js:2333): flags.exportChoice ?? (flags.yes ? 'local-parquet' : 'keep-local').
  • The PR's own new comment states local-parquet is the default "so npx hypaware produces durable files out of the box" — yet the --source flag path doesn't honor that. This is the unfinished tail of the PR feat: autodetect installed client tools in the npx install wizard #76 advisory (non-TUI default divergence), partially fixed and relocated rather than unified.

The underlying question is a product-contract decision: should hyp init --source X (without --yes) default to local-parquet like the wizard now does, or stay conservative at keep-local? Resolving it in one place (or documenting the intended split) would clear the finding.

Heads-up unrelated to the verdict: the PR description only mentions "skills block spacing" for walkthrough.js, but the diff also removes the interactive export picker and swaps the backfill confirm()select(). Both verified correct — just worth noting in the description.

Codex minors (advisory, non-blocking)

  • Telemetry still logs an exports pick event when no prompt is shown, so analytics can't distinguish "user chose local-parquet" from "system defaulted" (walkthrough.js:711,730).
  • The new clearOnResolve cleanup branch (runtime.js:82) lacks an isolated resolve/cancel test (redraw row-counting is covered; the settle-erase isn't).

Fix validations — all correct

Codex confirmed the three stated fixes land: wrapped-frame de-duplication (physical-row counting + narrow-terminal regression test), prompt-frame stacking (clearOnResolve), and the "stuck" long-backfill transition (pre-run status line).

Risk capstone (inline)

Source Finding (severity, evidence) Intersects
Codex major — export default path-dependent (walkthrough.js:711 vs core_commands.js:2333) Targets + direct callers
Codex minor — defaulted-export telemetry ambiguity (walkthrough.js:711,730) Targets
Codex minor — clearOnResolve cleanup lacks direct test (runtime.js:82) Targets (TUI hot path)

risk_class=low: no concurrency/security/data-migration surface; the broad TUI-runtime change is verified + regression-tested; dep bumps are patch-level.

Codex review (full)

Fix Validations

Wrapped TUI frames duplicated while navigating long options

  • Status: correct
  • Evidence: src/core/cli/tui/runtime.js:99, runtime.js:218, test/core/cli/tui/runtime.test.js:324
  • Assessment: The redraw path now uses physical-row counting (with wrapping) instead of newline counting, and the regression test exercises a narrow terminal wrap case.

Prompt frames stacked between wizard questions

  • Status: correct
  • Evidence: runtime.js:82, walkthrough.js:429, walkthrough.js:456, walkthrough.js:522
  • Assessment: clearOnResolve is threaded through the runtime and enabled on walkthrough prompts, so settled prompt frames are explicitly cleared before the next step.

Wizard looked stuck during long backfill

  • Status: correct
  • Evidence: walkthrough.js:1261, walkthrough.js:1263
  • Assessment: A pre-run status line is now emitted before each provider import starts, making the transition out of the consent prompt visibly progress.

Findings

Contract & Interface Fidelity — major (high)

  • Evidence: walkthrough.js:711, core_commands.js:2333
  • Why it matters: Default export behavior is now path-dependent: interactive walkthrough defaults to local-parquet, while non-interactive hyp init --source ... still defaults to keep-local, creating divergent configs for equivalent source selections.
  • Suggested fix: Unify defaulting logic in one place (e.g. runPickerWalkthrough input normalization or runPickerInit) so omitted --export resolves consistently across interactive and flag-driven init flows.

Debuggability & Operability — minor (medium)

  • Evidence: walkthrough.js:711, walkthrough.js:730
  • Why it matters: Telemetry still logs an exports pick event even when no export prompt is shown, so analytics can no longer distinguish "user chose local-parquet" from "system defaulted local-parquet."
  • Suggested fix: Add explicit origin metadata (e.g. pick_origin=default|user) or emit a separate event for defaulted export selection.

Test Evidence Quality — minor (medium)

  • Evidence: runtime.js:82, test/core/cli/tui/runtime.test.js:294, runtime.test.js:324
  • Why it matters: The new clearOnResolve cleanup branch is behaviorally important but the added tests focus on row counting/redraw, not on resolve/cancel frame clearing itself.
  • Suggested fix: Add runtime tests that set clearOnResolve: true and assert cleanup emits cursor-up + clear on both resolve and cancel paths.

No Finding

Behavioral Correctness · Change Impact/Blast Radius · Concurrency, Ordering & State Safety · Error Handling & Resilience · Security Surface · Resource Lifecycle & Cleanup · Release Safety · Architectural Consistency

Claude review (full)

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

(5-Sonnet parallel scan + Haiku confidence scoring. Four sub-threshold candidates were filtered at the 0.80 gate: interactive export-picker removal — 50, deliberate code-documented decision; clearOnResolve cancel-erase UX — 50, ordering verified correct; missing clearOnResolve unit test — 25, coverage gap not required by CLAUDE.md; cancelledResult() returning keep-local — 25, dead value on the cancel path. The substance of the first is counted via Codex's major.)


Reports: .gc/pr-pipeline/reviews/pr-78/{codex,claude,risk,dual-review}.md · Ran manually (pr-pipeline rig suspended), faithful to mol-pr-dual-review.

🤖 Generated with Claude Code

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