Skip to content

library: FetchLibrary swallows json.Unmarshal errors — API shape changes fail silent instead of loud #27

Description

@stozo04

Problem

internal/template/library.go (FetchLibrary) discards the error on every decode of an API response:

_ = json.Unmarshal(tabsEnv.Data, &tabs)
...
_ = json.Unmarshal(grpEnv.Data, &groups)
...
_ = json.Unmarshal(detEnv.Data, &details)

All three ignore unmarshal failures. If Speediance changes a response shape (the standing risk for a reverse-engineered API — see the spoofed fingerprint in internal/api/types.go and the "if an endpoint breaks… internal/api is the single place to patch" note in endpoints.go), the affected slice stays empty and the catalog silently comes back partial or empty with a 0 exit code. For a tool whose value proposition is fidelity, a silent wrong/empty result is the worst failure mode: a downstream agent or push flow then resolves exercise ids against an incomplete catalog with no signal that anything broke.

A related, lower-severity instance is in internal/config/config.go (applyEnv): a malformed SPEEDIANCE_DEVICE_TYPE is silently dropped (if n, err := strconv.Atoi(v); err == nil { … } with no else), so a typo'd env value falls back to the default without a word.

Context / why it's currently like this

This is deliberate — the decodes mirror the Python tool's permissiveness (resilience over strictness, so a single odd record doesn't abort the whole catalog). That tradeoff is defensible; the issue is that it's applied at the whole-response level, where it hides structural breakage rather than tolerating a single bad row.

Suggested direction (not prescriptive)

  • At minimum, capture the error and surface it on the warn logger (c.logger/a.logger) so a shape change is visible on stderr without failing the command — e.g. if err := json.Unmarshal(...); err != nil { logger.Warn("library tabs decode failed", "err", err) }.
  • Stronger: distinguish "genuinely empty catalog" from "decode failed / zero items parsed from a non-empty body" and return an error (or non-zero exit) in the latter case, so CI/agents notice.
  • Keep per-row tolerance where it's intentional (skip a bad action, keep the rest) but don't let a top-level decode failure masquerade as an empty result.
  • applyEnv's device_type: a one-line logger.Warn on a non-integer value would remove the silent fallback.

Notes

  • Per CLAUDE.md / .claude/CLAWHUB_STANDARDS.md, any behavior change here should ship with a guard test in the same PR (a fixture with a deliberately malformed library response asserting it warns / errors instead of returning []).
  • Docs-only audit context: surfaced while reviewing the --json output contract; no user-visible output shape changes, only failure semantics.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions