fix(nickel): cure standards' own 20-file Nickel debt (#986) - #996
Merged
Merged
Conversation
`standards` publishes a Nickel gate it has never run against itself:
`ci-pipeline.yml` is `workflow_call:`-only and this repo has no self-caller, so
all 20 genuine `.ncl` files carry undetected debt. Measured with the exact
binary CI installs (nickel 1.18.0, sha256 9cba4dd6... verified on download):
format --check typecheck
baseline 20 fail 5 fail
after 0 fail 0 fail
Selection matches the gate's own pathspec: tracked `*.ncl` excluding
`*.k9.ncl` (20 files; the 16 k9 contracts are untouched).
WHAT WAS ACTUALLY WRONG
#986 described 2 bugs in 5 files. There were 6 distinct defects; 4 are fixed
here and 2 are reported below as follow-ups. Each was found only because the
previous one stopped masking it.
1. `%{ }` outside a string -- os_detect.ncl, 3 byte-identical copies.
`os = %{ "os" } | default = "linux"` is string-interpolation syntax used
where a contract belongs, so the file has NEVER parsed as Nickel. Cured as
`os | String | default = "linux"` -- the canonical idiom for an overridable
injected input, which is what the file's own comment ("detection via
environment introspection") and the existing `| default =` already intend.
Proven: defaults evaluate (os=linux -> Standard_PC -> nala_native) AND an
override works (`& { env.os | force = "darwin" }` -> Apple_Darwin).
2. Record contracts written with `:` -- config.ncl, 2 copies.
Nickel's own diagnostic names the cure. #986 quoted `present : Bool` as a
one-line fix; it is not. Fixing `present` revealed the same defect on
`context`, then on all of `WorkMetadata`, then `ProjectConfig`: a `|`-
annotated field disqualifies the enclosing literal from being a record
type, so every `:` in these schema records had to become `|`.
3. `std.enum.to_tag` does not exist in the pinned stdlib. Invisible to
`typecheck` because `to_json_ld` is annotated `-> _`. Cured with
`std.string.from_enum`, whose output is the same string.
4. Infinite recursion in the exported record. Nickel records are recursive, so
`validate_work = validate_work` is a self-reference, not a reference to the
`let` above it. Distinct aliases bound before the record; the exported field
names (the public API) are unchanged.
5. `include` is now a Nickel KEYWORD -- infra.ncl:386.
This one is not a code bug and is worth naming: the file typechecks rc=0
while `nickel format` fails to PARSE it, because the formatter's grammar
and the evaluator's disagree about a bare field named `include`. Quoting the
field name cures it. Proven semantics-preserving on a minimal reproducer:
bare and quoted forms `export` to byte-identical JSON.
6. Formatting: the remaining 15 files, `nickel format` (the auto-fix form) run
LOCALLY to produce this commit. CI only ever runs `nickel format --check`,
which is read-only, per the binding spec's non-destructive rule.
STILL BROKEN, DELIBERATELY NOT FIXED HERE
`config.ncl` typechecks and formats but still cannot `nickel export` whole,
which its own header advertises as its usage. Two pre-existing defects remain,
both needing a design decision rather than a bug fix:
- `std.record.merge` does not exist either, so `.environments` cannot
evaluate. `std.record.merge_all` exists, but the override pattern also
needs `| default` priorities on the base record or the merge conflicts.
- `.schemas` exports the contract records themselves, whose fields have
contracts but no definitions, so they are not exportable data at all.
`.config` and `.example` DO evaluate (rc=0), which is what proves defects 3
and 4 above are genuinely fixed rather than merely parsing.
Note this class: the gate checks format + typecheck only, so a file that
cannot evaluate would still have gone green. That is the same vacuous-gate
shape this campaign has been closing elsewhere.
VERIFIED BEFORE COMMIT
- nickel 1.18.0 downloaded and sha256-checked against the value pinned in
ci-pipeline.yml: OK
- format --check over all 20: 0 failures (was 20)
- typecheck over all 20: 0 failures (was 5)
- the 3 os_detect copies byte-identical to each other; the 2 config.ncl
copies byte-identical to each other (#986 AC2/AC3)
- `git diff --name-only -- '*.k9.ncl'` -> empty; no non-.ncl file touched
Closes #986 AC1-AC5. AC6 (a self-caller for ci-pipeline.yml, without which all
of the above can silently regress) follows as a separate PR, per owner ruling.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ji1bq3TypfycfUPAR7hSxR
Contributor
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (21)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`.machine_readable/REGISTRY.a2ml` is a DERIVED artefact: `build-registry.sh`
regenerates it from every tracked file under a spec home, and a gate fails the
build when the committed copy no longer matches the tree. The preceding commit
reformatted 20 `.ncl` files, 7 of them under `.machine_readable/contractiles/`
and 4 under `1-formats/k9/`, so the registry went stale by construction.
Generated, not hand-edited:
bash scripts/build-registry.sh # "Wrote ... (33 specs)."
This is the whole of the cure for BOTH reds on this PR, which shared one root
cause rather than being two faults:
- `Registry + topology in sync` -- `build-registry.sh --check` -> rc=1
- `Repo self-tests` -- 2 of 53 test files, namely
* `build-registry-test.sh` (2 of 9 controls; both clone the COMMITTED
tree, so they stay red until this artefact is committed, not merely
regenerated in the working tree)
* `wave3-scorecards-test.sh` (3 "claimed PASS but check exited 1"
entries, every one of whose `check:` line is literally
`bash scripts/build-registry.sh --check`)
Measured after regenerating: `--check` rc=0, `wave3-scorecards-test.sh` 9
passed / 0 failed. `build-registry-test.sh` needs this commit to exist before
its clone can see the cure.
The generator is proven deterministic by its own suite (two generations
byte-identical, no embedded timestamp), so this artefact is reproducible by
anyone running the command above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ji1bq3TypfycfUPAR7hSxR
|
This was referenced Sep 22, 2026
Merged
hyperpolymath
added a commit
that referenced
this pull request
Sep 22, 2026
…(30/30 startup failures) (#997) ## The fork-PR security gate has never run `.github/workflows/security-gate-pr-target.yml` is red on `main` and has been red on every one of its **last 30 runs — 30 of 30 failures**. Every one has `jobs=0`: it is a **startup death**, not a failing test. The workflow dies at parse time, before any job is created, so there are no logs to read and it was diagnosed from the file instead. This was found while verifying that #996 had not reddened `main`. It had not — this red is pre-existing and predates the merge by at least six commits. ## Two independent faults, either fatal on its own ### 1. An empty expression inside a `run:` block The comment explaining why a fork's branch name must never be interpolated into the script wrote the two-brace syntax out **literally**, as an *empty* expression. The runner substitutes expressions into the script as TEXT before bash sees it — **comments included** — and an empty one is a fatal workflow-parse error. > The security comment warning about the danger of interpolation was itself the > interpolation that killed the workflow. Cured by naming the syntax in prose, and adding a note at the site so the next reader knows why the literal must not come back. ### 2. `steps` context used at job level ```yaml if: steps.fork-check.outputs.is_fork != 'true' ``` `steps` does not exist at job level — only `github`, `inputs`, `needs` and `vars` do. Replaced with the exact value the `fork-check` step itself keys on (`HEAD_IS_FORK: github.event.pull_request.head.repo.fork`), so the condition is semantically identical to the evident intent: ```yaml if: github.event.pull_request.head.repo.fork != true ``` ## Verification | Check | Result | |---|---| | `yq` parse **before** the change | **rc=0** — the fault was never in the YAML layer, only the expression layer. A YAML linter cannot catch this class. | | `actionlint` before | 2 `[expression]` errors | | `actionlint` after | **0** `[expression]` errors | | Remaining `actionlint` findings | 11 `[shellcheck]` style infos, **pre-existing and untouched** | **Falsification test.** Across all 55 workflows in the repo, exactly **one** file contains a literal empty expression — this one — and it is the **only** workflow with `jobs=0`. Controls without it report `jobs=1`, including `settings-drift-detect.yml`, which is red but *ran*. Red alone does not imply startup death; the empty expression predicts it exactly. ## Scope and what to expect No behaviour changes for any passing job, because **no job has ever run**. The workflow is `pull_request_target`-triggered only, so it cannot affect `push` builds. It will take its **first real measurement on the next pull request** — so treat that first run as the gate's debut, not as a regression. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Ji1bq3TypfycfUPAR7hSxR --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #986 (AC1–AC5). AC6 follows separately — see the bottom.
The shape of the problem
standardspublishes the Nickel gate the estate runs, and has never run it against itself:ci-pipeline.ymlisworkflow_call:-only and this repo has no self-caller. So all 20 of its genuine.nclfiles carry undetected debt.Measured with the exact binary CI installs — nickel 1.18.0, downloaded and sha256-verified against the value pinned in
ci-pipeline.yml(9cba4dd6…) before anything was measured. The pinned toolchain is part of the verification: formatter output differs across versions, so formatting with the locally-installed 1.17.0 would have produced a PR that fails its own gate.format --checktypecheck58aa824)Selection matches the gate's own pathspec: tracked
*.nclexcluding*.k9.ncl→ 20 files. The 16 k9 contracts are untouched (git diff --name-only -- '*.k9.ncl'is empty).What was actually wrong
#986 described 2 bugs in 5 files. There were 6 distinct defects. Each surfaced only after the previous one stopped masking it, which is why the issue's diagnosis under-counted.
1.
%{ }used outside a string —os_detect.ncl, 3 byte-identical copies.os = %{ "os" } | default = "linux"is string-interpolation syntax where a contract belongs, so this file has never parsed as Nickel. Cured toos | String | default = "linux"— the canonical idiom for an overridable injected input, which is what the file's own comment and its existing| default =were already reaching for.Proven by evaluation, not just parsing: defaults give
os=linux → Standard_PC → nala_native, and an override (& { env.os | force = "darwin" }) givesApple_Darwin.2. Record contracts written with
:instead of|—config.ncl, 2 copies.#986 quoted
present : Boolas a one-line fix. It is not. A record literal only qualifies as a record type if every field is: Typewith no|metadata — so one| optionalfield disqualifies the whole record. Fixingpresentrevealed the same defect oncontext, then on all ofWorkMetadata, thenProjectConfig. Every:in these schema records had to become|.3.
std.enum.to_tagdoes not exist in the pinned stdlib. Invisible totypecheckbecauseto_json_ldis annotated-> _. Cured withstd.string.from_enum(same output string) — found by enumerating the real API withstd.record.fields, not by guessing.4. Infinite recursion in the exported record. A Nickel record is recursive: inside a record literal a field name shadows any outer binding of the same name, so
validate_work = validate_workis a self-reference, not a reference to theletabove it.nickel exportdied on exactly that. Cured by binding distinct aliases before the record; the exported field names (the public API) are unchanged.5.
includeis now a Nickel keyword —infra.ncl:386.Worth naming on its own, because it is a gate finding rather than a code bug: the formatter's grammar and the evaluator's disagree. The file
typechecks rc=0 whilenickel formatfails to parse it. Quoting the field name cures it. Proven semantics-preserving on a minimal reproducer — bareinclude =→ format rc=1 / typecheck rc=0;"include" =→ both rc=0; and both formsexportto byte-identical JSON.6. Formatting — the remaining 15 files.
nickel format(the auto-fix form) was run locally to produce this commit. CI only ever runsnickel format --check, which is read-only, per the binding non-destructive rule.What this does NOT claim
config.nclnow formats and typechecks, but still cannotnickel exportwhole, which its own header advertises as its usage. Two pre-existing defects are left deliberately unfixed because a correct fix is a design decision, not a bug fix, and I would rather not make it silently:.environments—std.record.mergedoes not exist either.std.record.merge_alldoes, but the override pattern also needs| defaultpriorities on the base record or the merge conflicts..schemas— it exports the contract records themselves, whose fields carry contracts but no definitions, so they are not exportable data at all (missing definition for 'name')..configand.exampledo evaluate (rc=0), and that is what proves defects 3 and 4 are genuinely fixed rather than merely parsing:--field examplegivesvalid=True, the citation renders,palimpsest:aiTraining: "prohibited".Also observed, not fixed:
'Apple_Darwinhas no explicit arm in os_detect'sdeployment_prioritymatch and falls through to_ => "container_first". Pre-existing logic gap.Note the class of defect this exposes. The gate runs
format --check+typecheckonly. A file that cannot evaluate at all would still go green. That is the same vacuous-gate shape this campaign has been closing elsewhere, and it is the strongest argument for AC6.Verified before opening
nickel 1.18.0sha256-checked against theci-pipeline.ymlpin: OKformat --checkover all 20: 0 failures (was 20)typecheckover all 20: 0 failures (was 5)os_detect.nclcopies byte-identical to each other; the 2config.nclcopies byte-identical to each other (AC2 / AC3) — checked by md5 group count after all edit rounds, because copies drift the moment you edit after copyinge3b9929(fix(ci): cure the AC4 currency gate's false positives, and wire it #990); zero file overlap, and the newly-wired AC4 currency gate runs clean over this tree (rc=0)AC6 follows separately
AC6 is a self-caller for
ci-pipeline.ymlon this repo — without which everything above can silently regress, since nothing currently checks it. Per owner ruling that lands as its own PR, after this one.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ji1bq3TypfycfUPAR7hSxR