coilyco-flight-deck/umbra

Watch

fix(specverb, opcore): let a shadow carry the leaf's optional fields, correctly typed #327

Merged

coilysiren merged 1 commit from aos/1105-optional-args into main

2026-08-27 04:38:24 +00:00

coilyco-ops commented

2026-08-27 04:36:15 +00:00

Owner

Closes #326 and #328. coilyco-flight-deck/agentic-os#1105 restores the full flag set on top of it.

The behaviour

An input without required may legitimately be absent, but an args entry bound to it resolved through an error, and buildCallRequest turned that into a UserError that killed the whole call. Observed on a real verb, with nothing modified:

$ aosguard ops forgejo action-run list coilyco-flight-deck umbra
aosguard: call 1 (list): action arg "limit": $limit is not set (an optional input that was not supplied)

So "optional" held only while nothing bound it. The moment an args block referenced it, it was required in practice, and the error told the caller to supply an input the guardfile had called optional.

Why it matters beyond the message

A mount shadow replaces the generated leaf on the CLI, so it can expose only the fields it declares, and it could only declare them as required. agentic-os#1105 dropped --assignee, --assignees, --closed, --due_date, --milestone and --ref from issue create for exactly this reason: carrying them would have forced every filing to pass all six.

Adding a guard cost the leaf's optional surface. That is a tax on every consumer that shadows a leaf.

The fix, and why it does not fail open

ResolveArg now wraps ErrUnsetOptional, and buildCallRequest skips that arg so the field is simply absent from the request.

The obvious worry is a typo'd $refrence silently vanishing instead of erroring. It cannot, because that is already caught one layer earlier:

http/specverb/action_call.go  validateCallArgRef
    "arg %q references $%s, which no `input` declares"

Build-time validation rejects a bare $name that no input declares, so any reference reaching runtime is a declared input. $step.field is a separate path and still fails when its step is unbound.

The dry plan resolves the same way, so --dry-run no longer prints a ${placeholder} for something the live call drops.

Scope, and the one thing deliberately left alone

Spec dialect only. An execverb step takes positional argv tokens, where dropping one would shift every token after it and could silently weaken an argN guard. So ResolveArgDry keeps its original signature and the execverb path is byte-for-byte unchanged; the new UnsetInputRef helper is what lets specverb opt in without touching it.

That is the part worth reviewing hardest - the first draft did change both paths, and it should not have.

Verification

make test (34 packages, no failures), make vet, make lint (0 issues), make tidy, godoc-current.txt regenerated. code-comments reports 18 violations on main unchanged; this branch adds none.

Two new tests: the omitted field absent from the live body rather than sent empty or as a literal placeholder, and the dry plan carrying no placeholder for it.

Second commit: scalar typing (#328)

Carrying the optional fields is only half of it. BindSlice lowers array elements to the declared element type (#315, #319), but Bind did not do the same for a scalar, so a body field declared integer got the raw CLI string:

body:
    milestone: "16"      <- quoted; the field is an int64

boolean had the same shape. That is #316's defect one level up, and Forgejo's answer to a wrong type is frequently a 200 that changes nothing - the whole subject of coilyco-flight-deck/agentic-os#1047.

CoerceScalar mirrors CoerceItems: integer, number and boolean parse to their JSON type, anything else stays a string, and a value that does not parse is refused rather than sent. Body fields only, since path and query values are strings on the wire.

Without this, #326 would have handed consumers a way to carry --milestone that silently sent the wrong type - a worse outcome than not carrying it. The two belong together.

Two more tests: an integer field reaching the wire as a JSON number, and a non-integer refused before the write.

Closes #326 and #328. `coilyco-flight-deck/agentic-os#1105` restores the full flag set on top of it. ## The behaviour An `input` without `required` may legitimately be absent, but an `args` entry bound to it resolved through an error, and `buildCallRequest` turned that into a `UserError` that killed the whole call. Observed on a real verb, with nothing modified: ``` $ aosguard ops forgejo action-run list coilyco-flight-deck umbra aosguard: call 1 (list): action arg "limit": $limit is not set (an optional input that was not supplied) ``` So "optional" held only while nothing bound it. The moment an `args` block referenced it, it was required in practice, and the error told the caller to supply an input the guardfile had called optional. ## Why it matters beyond the message A mount shadow **replaces** the generated leaf on the CLI, so it can expose only the fields it declares, and it could only declare them as required. agentic-os#1105 dropped `--assignee`, `--assignees`, `--closed`, `--due_date`, `--milestone` and `--ref` from `issue create` for exactly this reason: carrying them would have forced every filing to pass all six. Adding a guard cost the leaf's optional surface. That is a tax on every consumer that shadows a leaf. ## The fix, and why it does not fail open `ResolveArg` now wraps `ErrUnsetOptional`, and `buildCallRequest` skips that arg so the field is simply absent from the request. The obvious worry is a typo'd `$refrence` silently vanishing instead of erroring. It cannot, because that is already caught one layer earlier: ```go http/specverb/action_call.go validateCallArgRef "arg %q references $%s, which no `input` declares" ``` Build-time validation rejects a bare `$name` that no `input` declares, so any reference reaching runtime is a declared input. `$step.field` is a separate path and still fails when its step is unbound. The dry plan resolves the same way, so `--dry-run` no longer prints a `${placeholder}` for something the live call drops. ## Scope, and the one thing deliberately left alone Spec dialect only. An execverb step takes **positional argv tokens**, where dropping one would shift every token after it and could silently weaken an `argN` guard. So `ResolveArgDry` keeps its original signature and the execverb path is byte-for-byte unchanged; the new `UnsetInputRef` helper is what lets specverb opt in without touching it. That is the part worth reviewing hardest - the first draft did change both paths, and it should not have. ## Verification `make test` (34 packages, no failures), `make vet`, `make lint` (0 issues), `make tidy`, `godoc-current.txt` regenerated. `code-comments` reports **18 violations on `main` unchanged**; this branch adds none. Two new tests: the omitted field absent from the live body rather than sent empty or as a literal placeholder, and the dry plan carrying no placeholder for it. --- ## Second commit: scalar typing (#328) Carrying the optional fields is only half of it. `BindSlice` lowers array elements to the declared element type (#315, #319), but `Bind` did not do the same for a scalar, so a body field declared `integer` got the raw CLI string: ``` body: milestone: "16" <- quoted; the field is an int64 ``` `boolean` had the same shape. That is #316's defect one level up, and Forgejo's answer to a wrong type is frequently a 200 that changes nothing - the whole subject of `coilyco-flight-deck/agentic-os#1047`. `CoerceScalar` mirrors `CoerceItems`: `integer`, `number` and `boolean` parse to their JSON type, anything else stays a string, and a value that does not parse is **refused rather than sent**. Body fields only, since path and query values are strings on the wire. Without this, #326 would have handed consumers a way to carry `--milestone` that silently sent the wrong type - a worse outcome than not carrying it. The two belong together. Two more tests: an integer field reaching the wire as a JSON number, and a non-integer refused before the write.

coilyco-ops added 1 commit

2026-08-27 04:36:15 +00:00

fix(specverb): drop an omitted optional arg instead of failing the call

ci / lint (pull_request) Successful in 36s

Details

ci / secrets (pull_request) Successful in 8s

Details

ci / test (pull_request) Successful in 48s

Details

60eb0d8475

Closes #326.

## The behaviour

An `input` without `required` may legitimately be absent, but an `args` entry
bound to it resolved through an error, and `buildCallRequest` turned that into a
UserError that killed the whole call. Observed on a real verb:

    $ aosguard ops forgejo action-run list coilyco-flight-deck umbra
    aosguard: call 1 (list): action arg "limit": $limit is not set
              (an optional input that was not supplied)

So "optional" held only while nothing bound it. The moment an `args` block
referenced it, it was required in practice.

## Why it matters

A mount shadow replaces the generated leaf, so it can expose only the fields it
declares, and it could only declare them as required. agentic-os#1105 dropped
six of `issue create`'s optional flags for exactly this reason: carrying them
would have forced every filing to pass all six. Adding a guard cost the leaf's
optional surface.

## The fix, and why it does not fail open

`ResolveArg` now wraps `ErrUnsetOptional`, and `buildCallRequest` skips that arg
so the field is simply absent. A typo cannot hide behind this:
`validateCallArgRef` already rejects a bare `$name` no `input` declares at build
time, so any reference reaching runtime is a declared input. `$step.field` is
unaffected and still fails when its step is unbound.

The dry plan resolves the same way, so `--dry-run` no longer prints a
`${placeholder}` for something the live call drops.

## Scope

Spec dialect only. An execverb step takes positional argv tokens, where dropping
one would shift every token after it and could silently weaken an `argN` guard,
so `ResolveArgDry` keeps its signature and that path is untouched. The new
`UnsetInputRef` helper is what lets specverb opt in without changing it.

## Verification

`make test` (34 packages, no failures), `make vet`, `make lint` (0 issues),
`make tidy`, `godoc-current.txt` regenerated, and `code-comments` reports 18 on
`main` unchanged. Two new tests: the omitted field absent from the live body
rather than sent empty, and the dry plan carrying no placeholder for it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>
Agent-Role: platform

coilysiren merged commit 39b76ec95a into main

2026-08-27 04:38:24 +00:00

coilysiren deleted branch aos/1105-optional-args

2026-08-27 04:38:25 +00:00

coilysiren referenced this pull request from a commit

2026-08-27 04:38:27 +00:00

fix(specverb): drop an omitted optional arg instead of failing the call (#327)

coilyco-ops changed title from ~~fix(specverb): drop an omitted optional arg instead of failing the call~~ to fix(specverb, opcore): let a shadow carry the leaf's optional fields, correctly typed

2026-08-27 04:40:55 +00:00

coilyco-ops referenced this pull request

2026-08-27 04:45:14 +00:00

fix(opcore): give an action's scalar arg the type its field declares #329

coilyco-ops commented

2026-08-27 04:45:43 +00:00

Author

Owner

Correction to this PR's own description: it closed #326 only, not #328.

The body was edited to cover a second commit adding CoerceScalar, but the merge here took the branch at 60eb0d8, one commit earlier. 39b76ec on main carries ErrUnsetOptional and nothing else - confirmed by reading the merged tree rather than the diff summary:

$ git show origin/main:pkg/stepflow/stepflow.go | grep -c ErrUnsetOptional
3
$ git show origin/main:http/opcore/binder.go | grep -c CoerceScalar
0

#328 is still open and carries the scalar-typing half on its own, as #329. Recording it here so nobody reads this PR's description and assumes an integer body field is already typed - it is not, until #329 lands.

Correction to this PR's own description: it closed **#326 only**, not #328. The body was edited to cover a second commit adding `CoerceScalar`, but the merge here took the branch at `60eb0d8`, one commit earlier. `39b76ec` on `main` carries `ErrUnsetOptional` and nothing else - confirmed by reading the merged tree rather than the diff summary: ``` $ git show origin/main:pkg/stepflow/stepflow.go | grep -c ErrUnsetOptional 3 $ git show origin/main:http/opcore/binder.go | grep -c CoerceScalar 0 ``` #328 is still open and carries the scalar-typing half on its own, as #329. Recording it here so nobody reads this PR's description and assumes an integer body field is already typed - it is not, until #329 lands.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

coilyco-flight-deck/umbra!327

No description provided.

Rows
Columns