Pre-L6, an env entry with a dotted key (e.g. {"weird.key": ...}) produced
diagnostic paths like 'env.weird.key' — ambiguous between the
single key 'weird.key' and a nested env.weird.key. Authors reading
admin-actions-parse output had to deduce the boundary.
joinPath helper renders identifier-shaped keys unbracketed
(parent.child) and bracket-quotes anything else
(parent["weird.key"]). Wired into parseEnv where env names are
unconstrained YAML — the worst offender. Other callers can adopt
incrementally.
Local pathIdentRe rather than reusing the L2 polish-bundle's
canonical identRe so this cleanup PR is independent of #67. Once
both land we can dedupe.
Adds a 'Where the flag lives' subsection so future readers don't
have to deduce the single-source-of-truth from grep. References
the L5 cleanup that dropped the duplicate field.
Pre-L5 workflow.Value had a Tainted bool field plus a Tainted()
constructor — both unused. The parser only ever called V() (which
zeroed Tainted) so the field was always false. Two different
structs in two different packages both named Value claimed to own
the taint contract; the architecture doc has always pointed at
expr.Value.Tainted as load-bearing.
Single source of truth now: workflow.Value carries Raw only;
expr.Value carries the taint flag. Constructor V() preserved with
trimmed signature (no Tainted to zero).
Audit found we relied on go.yaml.in/yaml/v3's built-in defense
without acknowledging it. Reading the source: yaml.v3 has its own
ratio-based check (alias count > 100 AND decode count > 1000 AND
ratio above the size-scaled threshold) but the constants are baked
in and not configurable.
Our 100-alias hard cap fires on documents the ratio defense ignores
(small files where the decode count is too low to trigger). Both
layers are real defense; document the relationship so the next
maintainer knows what 'we own' vs 'the library owns'.
Adds an identRe = ^[A-Za-z_][A-Za-z0-9_-]*$ check at parse time for
job keys (parseJob) and step ids (parseStep when set). The regex
mirrors the DB-side CHECK constraints in 0043 (workflow_jobs_job_key_format)
and 0044 (workflow_steps_step_id_format) so authors get the error
at workflow-lint time instead of at INSERT time during S41b dispatch.
Empty step id is allowed (it's an optional field — the schema
constraint is 'step_id = '' OR matches regex').
Pre-L1 the parser used 'n.Value == "true"' at three sites — strict
string match. YAML 1.2 booleans True and TRUE silently parsed to
false. Authors who wrote 'cancel-in-progress: True' got no warning
and the opposite of the documented behavior at runtime.
New parseBool helper accepts the canonical forms (true/True/TRUE,
false/False/FALSE, plus 1/0 and t/f as strconv.ParseBool does) and
surfaces a parse-time diagnostic on anything else — including
'yes'/'no' which are valid YAML 1.1 but not 1.2.
Updated call sites:
- DispatchInput.Required (parseDispatchInputs)
- Concurrency.CancelInProgress (parseConcurrency)
- Step.ContinueOnError (parseStep)
Pre-fix the lexer iterated byte-by-byte and called unicode.IsLetter
on a rune cast from a single byte — only handles ASCII + Latin-1
correctly. Multi-byte UTF-8 sequences either fed leading bytes that
happened to test as letters (false-positive identifier chars) or
fell into the default arm with a confusing 'unexpected character'
error pointing at a continuation byte.
Replaces the main loop and lexIdent with utf8.DecodeRuneInString
walks. isIdentStart/isIdentChar now take rune. Single-byte ASCII
operators (==, !=, &&, ||) keep their fast path since they can't
overlap with UTF-8 continuation bytes (which are >= 0x80).
Surfaces invalid UTF-8 with a precise 'invalid UTF-8 at offset N'
error instead of letting it through.
Quality fix, not a security one: the byte-level lexer failed
closed (rejected). Closes the door consistently and gives authors
correct diagnostics.
Replaces the documentation-only contract with a pointer to the
typed event constructors. Documents the closed-door discipline for
adding fields (edit constructor + doc + *_FlowsThroughEvaluator
test in the same PR; reviewer-required note) and the v1→v2 break
boundary for renames/removals.
Four shape tests assert the documented top-level keys + nested
shape for each trigger constructor (push/pull_request/schedule/
workflow_dispatch). Two end-to-end tests pipe a constructor's
output through expr.Eval against canonical reference paths like
`shithub.event.pull_request.head.ref` and the github.* alias
counterpart, asserting both the value AND the load-bearing
Tainted=true flag.
If you're here because a shape test failed: that's intentional
friction. Update the doc + the test in the same PR as the
constructor change.
The shithub.event v1 schema was previously documentation-only —
docs/internal/actions-schema.md pinned the field layout but no
code enforced it. Audit S41a-L4 flagged this before S41b's trigger
pipeline writes the first event_payload row, since once data is in
the jsonb column the schema is sticky.
New package internal/actions/event with one constructor per trigger:
- event.Push(ref, before, after, hc HeadCommit)
- event.PullRequest(action, number, title, head, base, userLogin)
- event.Schedule()
- event.WorkflowDispatch(inputs map[string]string)
Each takes typed inputs (so adding a field is visible in the
function signature, reviewer-required by repo norms) and returns
map[string]any (what expr.evalEventPath consumes; also pgx-encodable
to jsonb without further transformation, so S41b passes the result
straight into InsertWorkflowRun).
The closed-door discipline matches the expression evaluator's
namespace + function allowlists: adding a field requires editing
the constructor, the doc, AND the corresponding *_FlowsThroughEvaluator
test in the same PR.
Same shared-schema-dir pattern as the prior round in S41a — every
package that runs sqlc against internal/migrationsfs/migrations sees
the new step_with field in its locally-generated WorkflowStep struct.
Captures the regeneration so 'sqlc generate' is idempotent.
Wires the new step_with jsonb column into the CRUD path. The column
is appended at the end of SELECT/RETURNING lists to match Postgres'
actual column order (ALTER TABLE in 0050 appended), so sqlc returns
the existing WorkflowStep struct directly instead of synthesizing a
new Row type.
ALTER TABLE workflow_steps ADD COLUMN step_with jsonb NOT NULL
DEFAULT '{}'::jsonb. Closes the gap between the parser's
Step.With map[string]Value field and the schema, which had no
column to persist it. S41b/c can now INSERT a step with the
forwarded action-input bag intact.
Default empty object so any pre-migration rows backfill harmlessly
(none in production yet — S41b hasn't started). Same shape as
step_env.
The original doc text described a parse-time rewrite layer feeding
shithub.* into downstream consumers. That's not what shipped — H1
the audit found dialect.go was dead code. The actual implementation
lives in evalRef and is per-reference at eval time. Update the
section to reflect that, document the scope-narrow behavior (only
shithub-equivalent fields route through), and call out the load-
bearing taint preservation.
