Skip to content

Fix libscrapli FFI logger level mismatch and resolve PR review comments - #17

Open
araustin01 with Copilot wants to merge 3 commits into
upstream-syncfrom
copilot/fix-github-actions-job
Open

araustin01 with Copilot wants to merge 3 commits into
upstream-syncfrom
copilot/fix-github-actions-job

Conversation

Copilot AI commented Sep 18, 2026

Copy link
Copy Markdown

The test / unit CI job was failing after bumping libscrapli to v0.0.1-rc.25.2, and PR #16 had 10 unresolved review comments flagging deadlock risks, panics, silently-ignored options, and broken backward compatibility.

Root cause: FFI logger level wire mismatch

libscrapli v0.0.1-rc.25.2 changed how the FFI options' logger_level byte is decoded (parseLoggerLevel in src/ffi-options.zig), remapping it to warn=0, trace=1, debug=2, info=3, critical=4, fatal=5. scrapligo was reusing scrapligologging.IntFromLevel — which encodes the internal severity ordering (trace=0...fatal=5) used for message filtering — to build this wire byte, so Warn (3) was sent and decoded as info:

// before: reused the internal severity-ordering function for the wire value
opts.loggerLevel = uint8(scrapligologging.IntFromLevel(o.LoggerLevel))

// after: dedicated mapping matching libscrapli's parseLoggerLevel
opts.loggerLevel = ffiLoggerLevelFromLevel(o.LoggerLevel)

ffiLoggerLevelFromLevel (internal/options.go) now owns this translation independently, leaving IntFromLevel/LevelFromInt untouched for their existing internal-filtering role.

Review comment fixes

  • cli/result.go: added Input() / InputAtIndex() accessors (previously Inputs became private with no way to read it back); rejected negative indexes in ResultAtIndex/ResultRawAtIndex instead of panicking.
  • internal/logging.go, internal/netconf.go, internal/recorder.go: dispatcher callbacks now release the read lock before invoking user code, avoiding a deadlock if the callback re-enters and needs the write lock; nil message pointers are now guarded instead of dereferenced.
  • options/cli.go: restored WithDefintionContent as a deprecated wrapper around the renamed WithDefinitionContent to preserve compatibility for existing callers.
  • cli/cli.go: ReplaceDefinition now sets the new definition path before calling loadDefinition, so the new definition is actually loaded instead of the stale one.
  • netconf/get_config.go: getFilterType/getDefaultsType now perform the same pointer conversion used elsewhere, so WithFilterType/WithDefaultsType reach libscrapli instead of being silently dropped.
  • util/file.go: LoadFileLines now returns scannerErr on scan failure instead of the unrelated (nil) err.

Co-authored-by: araustin01 <72994707+araustin01@users.noreply.github.com>
@araustin01
araustin01 added this pull request to stack #18 September 18, 2026 20:22
@araustin01
araustin01 marked this pull request as ready for review September 18, 2026 20:22
Copilot AI balanced review requested due to automatic review settings September 18, 2026 20:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Input() incorrectly collapses multiple empty inputs instead of preserving their newline-separated representation.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes FFI compatibility and addresses reliability/API regressions from PR #16.

Changes:

  • Corrects logger-level and NETCONF option encoding.
  • Hardens callbacks and result indexing.
  • Restores compatibility and fixes definition/file handling.
File summaries
File Description
util/file.go Returns scanner errors correctly.
options/cli.go Restores deprecated option alias.
netconf/get_config.go Forwards filter/default option values.
internal/recorder.go Prevents callback deadlocks and nil dereferences.
internal/options.go Adds correct FFI logger-level mapping.
internal/netconf.go Safely dispatches capability callbacks.
internal/logging.go Safely dispatches logging callbacks.
cli/result.go Adds input accessors and index validation.
cli/cli.go Loads replacement definitions correctly.
Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cli/result.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: araustin01 <72994707+araustin01@users.noreply.github.com>
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.

3 participants