Skip to content

Architectural review: create_chronics hook vs VILLASnode design patterns#1007

Draft
Copilot wants to merge 1 commit intohook-timeseries-chronix-conversionfrom
copilot/sub-pr-991
Draft

Architectural review: create_chronics hook vs VILLASnode design patterns#1007
Copilot wants to merge 1 commit intohook-timeseries-chronix-conversionfrom
copilot/sub-pr-991

Conversation

Copy link

Copilot AI commented Mar 19, 2026

The create_chronics implementation was added as a hook, but this conflicts with VILLASnode's hook architecture. This PR adds a code review comment explaining the mismatch.

Architectural issues identified

  • Hook contract violated: create_chronics never implements process(Sample*) — all logic runs in start(), bypassing the per-sample pipeline entirely
  • Batch semantics in a streaming API: Hooks are designed for O(1) per-sample transforms; create_chronics reads all files into memory and performs bulk columnar restructuring
  • Tight node coupling: Uses dynamic_cast<NodeCompat*> + getData<struct file>() to access file node internals — silently breaks with any other node type
  • Invasive file node changes: multi_file_mode, READ_ALL, and the file_samples map were added solely to support this hook, not as general-purpose improvements

Better fit

A Format plugin (lib/formats/) handles exactly this responsibility — serializing/deserializing batches of samples to/from structured file representations. Alternatively, a standalone converter tool outside VILLASnode keeps concerns cleanly separated without modifying core node types.


📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

Copilot AI changed the title [WIP] Add create_chronics hook for timeseries conversion Architectural review: create_chronics hook vs VILLASnode design patterns Mar 19, 2026
Copilot AI requested a review from stv0g March 19, 2026 11:31
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.

2 participants