Skip to content

[FEA]: Config loader with embedded JSON schemas + Steps.validate #214

Description

@rice-riley

Summary

Port agent/skyhook-agent/src/skyhook_agent/config.py and the cross-step validation rules from Steps.validate in agent/skyhook-agent/src/skyhook_agent/step.py (lines ~168–215) to Go in a single PR. The v1 JSON schemas under agent/skyhook-agent/src/skyhook_agent/schemas/v1/ ship as embed.FS so the agent binary stays a single static artifact.

These two concerns are fused in Python (config.load calls Steps.validate internally) and fusing them in the Go port keeps the wiring visible to a single reviewer instead of strung across two PRs. Combined size is ~340 LOC of source + tests.

Depends on #213 (the internal/step types).

Motivation

Every other module that needs a parsed config.json blocks on this. Without the cross-step rules (no-defined-steps, check-pair existence, UpgradeStep placement, file-on-disk checks) the agent will happily start an apply cycle for a config whose check scripts do not exist or whose UpgradeStep declares arguments — producing confusing mid-rollout failures.

This PR also forces an early decision on the JSON-schema library, the one new external dependency the agent introduces.

Feature description

A new internal/config package (loader, schema FS, dump, migrate) and Validate in internal/step (cross-rules), wired together so config.Load enforces both schema-level and cross-step constraints before returning.

Proposed direction

1. Schema embedding

//go:embed schemas/v1/*.json
var schemaFS embed.FS

Build a registry that resolves v1/skyhook-agent-schema.json and the $ref: "v1/step-schema.json" inside it — matching how Python's referencing.Registry works. Copy the v1 schemas into internal/config/schemas/v1/ byte-for-byte (golden test catches drift).

2. JSON Schema library choice

The Python jsonschema package supports Draft 2020-12 with $ref across multiple files. Pick one Go equivalent and call it out as the one new pattern this PR introduces:

  • github.com/santhosh-tekuri/jsonschema/v6 — Draft 2020-12 native, supports custom URI loaders for embed.FS. Recommended.
  • github.com/xeipuuv/gojsonschema — older, widely used, may need $ref URI gymnastics.
  • github.com/kaptinlin/jsonschema — newer, Draft 2020-12.

Document the choice in the PR description per the AGENTS.md "new patterns must be called out" rule.

3. Loader API

In internal/config:

  • Load(data []byte, stepRootDir string) (*Config, error) — JSON Schema validate against the embedded v1 schemas, migrate to latest, validate again, then materialize the modes section into typed step.Step / step.UpgradeStep slices, then call step.Validate(...) (see §5 below).
  • Dump(packageName, packageVersion, rootDir string, steps map[step.Mode][]step.Step, expectedConfigFiles []string) (*Config, error) — always emits the latest schema version.
  • Migrate(*Config) (*Config, error) — no-op for v1, but the function exists so future schema versions slot in cleanly.

4. Migration shim

Migrate is a no-op for v1 → v1 today. Match Python's behavior of returning a ValidationError for unknown versions (don't silently pass through).

5. step.Validate cross-rules

In internal/step/validate.go, port the rules from Steps.validate:

  1. No defined steps — fatal StepError. Triggered when every mode list is empty.
  2. Only check modes defined — fatal. At least one non-*-check mode must be present. Error names all the non-check modes.
  3. Apply without check — fatal per-mode. For every non-interrupt non-check mode that has steps, the corresponding *-check mode must also have steps.
  4. UpgradeStep placement — fatal. UpgradeStep may only appear under Mode.Upgrade or Mode.UpgradeCheck. Anywhere else is a StepError naming the offending step path and mode.
  5. UpgradeStep arguments — fatal. UpgradeStep declarations may not declare arguments (Python enforces this in __init__; in Go put it in NewUpgradeStep or Validate — pick one).
  6. Step file exists on disk — fatal error (Python uses ValueError, not StepError; preserve the distinction in Go via two error types). Every step.Path must resolve to a file under {rootDir}/{step.Path}.
  7. Missing check pair (warning, not error) — for each non-check step foo.sh, if no foo_check.sh exists in any check mode's step list, log a warning. Use the package logger (matching whatever [FEA]: Bootstrap agent/go/ module + port pure-data types #213 standardized on). The check-pair pairing rule needs the suffix logic from Python: split on the last . if present, append _check (and the extension if any), otherwise just append _check.

Wire config.Load to call step.Validate(parsedSteps, stepRootDir) after schema validation succeeds. Do this in two places — same as Python does: pre-migration and post-migration. (For v1 this is a no-op equivalent, but the seam is what matters.)

6. Tests

Port both agent/skyhook-agent/tests/test_config.py and the validation-focused cases from agent/skyhook-agent/tests/test_steps.py:

  • Valid v1 config loads.
  • Missing required fields fail with a structured error pointing at the field path.
  • Bogus schema_version fails.
  • Dump produces something Load accepts (round-trip).
  • All-empty error.
  • Only-check-modes error.
  • apply without apply-check error.
  • UpgradeStep in apply mode → error.
  • UpgradeStep with arguments → error.
  • Missing file on disk → error listing the missing paths.
  • Check-pair-missing → warning logged, not error.

Add a golden-bytes test ensuring the Dump output of a known-good package config matches the Python dump() byte-for-byte (key order may matter — Python json.dump is insertion-ordered; Go's encoding/json sorts map keys).

Scope boundaries

In scope:

  • Schema validation, embedded schemas, load/dump/migrate API.
  • Cross-step validation rules wired into config.Load.

Out of scope:

Acceptance criteria

  • agent/go/internal/config/schemas/v1/*.json are byte-identical copies of the Python schemas. CI lint catches drift via a golden test.
  • Load accepts every fixture in agent/skyhook-agent/tests/ that the Python config.load accepts.
  • Load rejects every fixture the Python config.load rejects, with the same field path / root cause.
  • step.Validate errors at every Python equivalent's failure point.
  • Check-pair warnings show up in test logs; they are not errors.
  • Schema FS is embedded — no external schema file required at runtime.
  • The new JSON Schema dependency is explicitly justified in the PR description.

Open questions

  • Migration semantics: do we ever expect a v0 → v1 migration to retroactively land in this codebase, or is v1 the floor? (Python treats unknown versions as fatal.)
  • Whether to expose Validate(data) as a separate public function so the kubectl skyhook CLI can lint package configs without instantiating steps. Defer to a follow-up unless it falls out for free.
  • Should the UpgradeStep-with-arguments check live in a NewUpgradeStep factory (Python's choice) or step.Validate? Decide in the PR.
  • Promote the check-pair "warning" to an error? Python's behavior is warn-only and the operator does not block on it. Recommend: keep warn-only for parity; revisit after cutover.

References (codebase)

Alternatives considered

  • Hand-rolled schema validation instead of JSON Schema. Rejected — error messages would diverge from Python's, and we lose the schema as a versioned artifact for package authors.
  • Folding cross-step rules into JSON Schema. Rejected — JSON Schema cannot express filesystem existence or cross-array structural constraints cleanly.
  • Keeping config and validate as separate PRs. Rejected — Steps.validate is called from config.Load and reviewers would have to hold the seam across two PRs.

Code of Conduct

  • I agree to follow Skyhook's Code of Conduct.

Metadata

Metadata

Assignees

Labels

component/agentSkyhook agent (package executor)

Fields

No fields configured for Enhancement.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions