Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent/go/internal/schema/schema.go`:
- Around line 93-110: The comparator currently discards parse errors from
parseVersionNumber and always returns -1/1, and ParseSchemaVersion returns
non‑canonical numeric inputs; update both to preserve canonical values and
consistent ordering: in the comparator (where
parseVersionNumber(a)/parseVersionNumber(b) are called) check and
propagate/handle parse errors instead of ignoring them—if either parse fails,
fall back to a deterministic lexical comparison of the SchemaVersion strings; in
ParseSchemaVersion call parseVersionNumber(normalized) and, when it succeeds,
convert the numeric result to the canonical SchemaVersion string (use a small
helper like formatSchemaVersion or similar) and return that canonical value, and
when parseVersionNumber fails return the parse error (don’t swallow it). Ensure
you reference and use parseVersionNumber and ParseSchemaVersion (and add
formatSchemaVersion if needed) so serialized values and comparator behavior stay
consistent.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent/go/internal/schema/schema_test.go`:
- Around line 80-83: Add two new test cases in schema_test.go targeting
ParseSchemaVersion: one that verifies canonicalization (call ParseSchemaVersion
with "v01" and "V002", assert no error and that returned value equals
SchemaVersion("v1") and SchemaVersion("v2") respectively) and another that
verifies rejection of non-positive versions (call ParseSchemaVersion with "v0"
and "v-1" and assert an error is returned; for "v0" also assert the error
message contains text like "must be >= v1"). Use the existing test style (It
blocks and Expect assertions) so they integrate with the current tests for
ParseSchemaVersion.

In `@agent/go/internal/step/regular_step.go`:
- Around line 54-59: The Encode method in RegularStep should wrap returned
errors with context instead of returning bare errors: when calling
s.Idempotence.Validate() (in RegularStep.Encode) wrap any returned error using
fmt.Errorf with a descriptive prefix like "validate idempotence: %w", and when
json.Marshal(s) fails wrap that error similarly (e.g. "marshal RegularStep:
%w"); keep the existing s.applyDefaults() call and ensure you import fmt if not
already imported.
- Around line 54-60: The Encode method on RegularStep must ensure the
discriminator is fixed: after calling s.applyDefaults() and validating
Idempotence (in RegularStep.Encode), set s.UpgradeStep = false unconditionally
before calling json.Marshal(s) so the marshalled payload cannot be coerced into
an upgrade step by a caller that previously set the exported field.

In `@agent/go/internal/step/step.go`:
- Around line 169-170: The call to u.Validate() inside Decode returns the bare
err (return nil, err) which breaks the repo's error-wrapping rule; update Decode
so that when u.Validate() fails you wrap the error with context (e.g., using
fmt.Errorf("upgrade step validation failed: %w", err)) before returning, and add
the fmt import if needed; locate the u.Validate() call in the Decode function in
step.go and replace the bare return with a wrapped error that references the
validation failure.

In `@agent/go/internal/step/upgrade_step_test.go`:
- Around line 103-143: Add a new unit case that constructs an UpgradeStep value
directly (not via NewUpgradeStep) and calls UpgradeStep.Encode() to verify the
wire contains the discriminator `"upgrade_step":true`; specifically, create an
UpgradeStep instance (using the UpgradeStep struct with RegularStep fields set
appropriately), call its Encode method and Expect the dumped bytes to
ContainSubstring(`"upgrade_step":true`), placing the test alongside the existing
Describe("UpgradeStep.Encode", ...) cases to pin ownership of the discriminator
independent of NewUpgradeStep.

In `@agent/go/internal/step/upgrade_step.go`:
- Around line 44-45: The call to u.Validate() in NewUpgradeStep returns a bare
err; update NewUpgradeStep to wrap that validation error with context using
fmt.Errorf and %w (e.g., include "NewUpgradeStep" or "upgrade step validation"
in the message) before returning so callers get call-site context; ensure fmt is
imported if not already.
- Around line 69-74: UpgradeStep.Encode currently just validates and delegates
to RegularStep.Encode, allowing a directly-constructed UpgradeStep to serialize
with the discriminator "upgrade_step": false; update UpgradeStep.Encode to
ensure the discriminator is forced to true before encoding by setting the
UpgradeStep-specific discriminator flag on the embedded RegularStep (or
otherwise set the field used by RegularStep.Encode that controls "upgrade_step")
and then call Validate and RegularStep.Encode (i.e. in UpgradeStep.Encode, set
the discriminator to true, call u.Validate(), then return
u.RegularStep.Encode()).

---

Duplicate comments:
In `@agent/go/internal/schema/schema.go`:
- Around line 126-136: ParseSchemaVersion currently returns the
lowercased/trimmed input (e.g., "v01") and doesn't enforce version >= 1; update
it to call parseVersionNumber(normalized) to obtain the integer n, validate n >=
1, and then return the canonical SchemaVersion using fmt.Sprintf("v%d", n)
(instead of returning normalized). Keep error wrapping the same when
parseVersionNumber fails and return an explicit error if n < 1. Ensure
references: ParseSchemaVersion, parseVersionNumber, SchemaVersion, and Latest
are used as described.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent/go/internal/step/upgrade_step.go`:
- Around line 72-74: UpgradeStep.Encode currently returns the bare error from
u.Validate(), losing call-site context; change the return to wrap the validation
error with context (e.g., return nil, fmt.Errorf("upgrade step validation
failed: %w", err)) so callers see which step failed, and add/import fmt if not
present; refer to UpgradeStep.Encode and u.Validate when making the change.

---

Duplicate comments:
In `@agent/go/internal/step/regular_step.go`:
- Around line 35-40: The RegularStep struct's OnHost bool causes direct literals
like RegularStep{Path:"foo.sh"} to encode "on_host": false breaking parity;
change OnHost to a pointer (*bool) on the struct (symbol: OnHost in type
RegularStep) and update RegularStep.Encode (and any defaulting logic tied to
Idempotence) to treat nil as "default true" when serializing, ensuring existing
defaulting at lines handling default values (previously skipping OnHost) still
applies; alternatively, prevent direct literal construction by making the struct
unexported and providing a constructor that sets the default, but prefer the
*bool + default-in-Encode approach for minimal disruption.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent/go/internal/step/regular_step.go`:
- Around line 34-49: The exported RegularStep struct currently has OnHost bool
which serializes false for zero-value literals and breaks Python parity; change
OnHost to a pointer (*bool) with the json tag `on_host,omitempty`, update
NewRegularStep to initialize OnHost to pointer-to-true, and modify applyDefaults
(and any code referencing OnHost) to treat nil as “unset” and set the pointer to
true when applying defaults; update tests to use NewRegularStep (or set &true)
where they construct RegularStep literals so the field is omitted/round-trips
correctly.

In `@agent/go/internal/step/step_test.go`:
- Around line 224-236: The test for RegularStep zero-value should assert the
OnHost behavior to document the wire discrepancy: update the test (the It block
that constructs bare := RegularStep{Path: "foo.sh"} and calls bare.Encode() /
Decode()) to either assert that the encoded wire form contains on_host:false or
assert that after round-trip the decoded rs.OnHost is false; reference
RegularStep, Encode, Decode and the OnHost field so reviewers can locate and
verify the added assertion.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@agent/go/internal/step/regular_step.go`:
- Around line 34-49: Change the OnHost field to a pointer so unset literals
don't serialize "on_host":false: update RegularStep.OnHost from bool to *bool
(and add `omitempty` to its json tag), update NewRegularStep to allocate and set
a true pointer, and modify applyDefaults (and any callers) to treat nil as
"unset" and set it to true when defaulting; run and adjust tests (e.g.,
regular_step_test.go) to construct or inspect the pointer/dereferenced value as
needed.