You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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/*.jsonvarschemaFS 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.
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).
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:
No defined steps — fatal StepError. Triggered when every mode list is empty.
Only check modes defined — fatal. At least one non-*-check mode must be present. Error names all the non-check modes.
Apply without check — fatal per-mode. For every non-interrupt non-check mode that has steps, the corresponding *-check mode must also have steps.
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.
UpgradeStep arguments — fatal. UpgradeStep declarations may not declare arguments (Python enforces this in __init__; in Go put it in NewUpgradeStep or Validate — pick one).
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}.
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.)
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).
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.
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.
Summary
Port agent/skyhook-agent/src/skyhook_agent/config.py and the cross-step validation rules from
Steps.validatein 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 asembed.FSso the agent binary stays a single static artifact.These two concerns are fused in Python (
config.loadcallsSteps.validateinternally) 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/steptypes).Motivation
Every other module that needs a parsed
config.jsonblocks on this. Without the cross-step rules (no-defined-steps, check-pair existence,UpgradeStepplacement, file-on-disk checks) the agent will happily start anapplycycle for a config whose check scripts do not exist or whoseUpgradeStepdeclares 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/configpackage (loader, schema FS, dump, migrate) andValidateininternal/step(cross-rules), wired together soconfig.Loadenforces both schema-level and cross-step constraints before returning.Proposed direction
1. Schema embedding
Build a registry that resolves
v1/skyhook-agent-schema.jsonand the$ref: "v1/step-schema.json"inside it — matching how Python'sreferencing.Registryworks. Copy the v1 schemas intointernal/config/schemas/v1/byte-for-byte (golden test catches drift).2. JSON Schema library choice
The Python
jsonschemapackage supports Draft 2020-12 with$refacross 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 forembed.FS. Recommended.github.com/xeipuuv/gojsonschema— older, widely used, may need$refURI 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 themodessection into typedstep.Step/step.UpgradeStepslices, then callstep.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
Migrateis a no-op for v1 → v1 today. Match Python's behavior of returning aValidationErrorfor unknown versions (don't silently pass through).5.
step.Validatecross-rulesIn
internal/step/validate.go, port the rules fromSteps.validate:StepError. Triggered when every mode list is empty.*-checkmode must be present. Error names all the non-check modes.interruptnon-check mode that has steps, the corresponding*-checkmode must also have steps.UpgradeStepplacement — fatal.UpgradeStepmay only appear underMode.UpgradeorMode.UpgradeCheck. Anywhere else is aStepErrornaming the offending step path and mode.UpgradeSteparguments — fatal.UpgradeStepdeclarations may not declare arguments (Python enforces this in__init__; in Go put it inNewUpgradeSteporValidate— pick one).error(Python usesValueError, notStepError; preserve the distinction in Go via two error types). Everystep.Pathmust resolve to a file under{rootDir}/{step.Path}.foo.sh, if nofoo_check.shexists in any check mode's step list, log a warning. Use the package logger (matching whatever [FEA]: Bootstrapagent/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.Loadto callstep.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:
schema_versionfails.Dumpproduces somethingLoadaccepts (round-trip).applywithoutapply-checkerror.UpgradeStepinapplymode → error.UpgradeStepwith arguments → error.Add a golden-bytes test ensuring the
Dumpoutput of a known-good package config matches the Pythondump()byte-for-byte (key order may matter — Pythonjson.dumpis insertion-ordered; Go'sencoding/jsonsorts map keys).Scope boundaries
In scope:
config.Load.Out of scope:
root_dir.teestreaming andrun_step#217).Acceptance criteria
agent/go/internal/config/schemas/v1/*.jsonare byte-identical copies of the Python schemas. CI lint catches drift via a golden test.Loadaccepts every fixture in agent/skyhook-agent/tests/ that the Pythonconfig.loadaccepts.Loadrejects every fixture the Pythonconfig.loadrejects, with the same field path / root cause.step.Validateerrors at every Python equivalent's failure point.Open questions
Validate(data)as a separate public function so thekubectl skyhookCLI can lint package configs without instantiating steps. Defer to a follow-up unless it falls out for free.UpgradeStep-with-arguments check live in aNewUpgradeStepfactory (Python's choice) orstep.Validate? Decide in the PR.References (codebase)
Steps.validate).Alternatives considered
Steps.validateis called fromconfig.Loadand reviewers would have to hold the seam across two PRs.Code of Conduct