Skip to content

fix(cli): render the provision command as a YAML block scalar - #220

Merged
javiertoledo merged 5 commits into
theam:mainfrom
cdotta:fix/provision-block-scalar
Sep 2, 2026
Merged

fix(cli): render the provision command as a YAML block scalar#220
javiertoledo merged 5 commits into
theam:mainfrom
cdotta:fix/provision-block-scalar

Conversation

@cdotta

@cdotta cdotta commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Closes part of #217.

facility-crew.yml and facility-codex.yml are unparseable on any repository where init detects no provision command, so neither agent trigger ever runs:

facility-crew.yml:196:27: could not parse as YAML:
mapping values are not allowed in this context [syntax-check]

196 | run: echo "facility: no provision command configured — the crew runs on a bare checkout...
                       ^~

The command is interpolated into a bare scalar (run: {{PROVISION_CMD}}), and the no-provision fallback text contains facility: . The colon-space ends the scalar.

This is not limited to the fallback — any provision command containing ": " breaks identically:

$ facility init --yes --provision='docker compose up -d && echo "db: ready"'
facility-crew.yml:202:45: could not parse as YAML: mapping values are not allowed in this context

Change

Render the command into a block scalar, matching what checksRun already does for the checks list — including for strings containing ::error::. The message text is unchanged; only its YAML context is.

Verification

  • actionlint on both rendered templates across three cases: no provision command, a command containing ": ", and an ordinary command. Clean in all three; the first two fail on main.
  • Full CLI test suite passes; node guards/run.mjs clean.
  • Also confirmed end to end on a real repository: with this change the workflows parse and the triggers run, where before the push produced two failed runs named by file path.

Deliberately not included

Having doctor parse the workflows it generated would have caught this, and would close the whole class rather than this instance — doctor --run-guards currently reports "Everything checkable checks out" on a repository whose primary workflow cannot be parsed. That is a larger change and I would rather agree the approach first. Detail in #217.

@javiertoledo

Copy link
Copy Markdown
Member

Reviewed this and pushed two commits to the branch (maintainer_can_modify). Both are extensions of your fix, not changes to its approach — provisionRun and the block scalar are exactly as you wrote them.

8226ddf — the same fix in the three remaining workflows.

init renders {{PROVISION_CMD}} into five workflows, not two. facility-review.yml:51, facility-address-review.yml:64 and facility-doctor.yml:112 kept the bare scalar, so a default facility init --yes on a repository where no provision command is detected still produced three documents GitHub cannot parse:

FAIL facility-address-review.yml: mapping values are not allowed here
FAIL facility-doctor.yml:         mapping values are not allowed here
FAIL facility-review.yml:         mapping values are not allowed here

All three indent run: by eight spaces exactly like the two you fixed, so PROVISION_RUN applied unchanged.

e66abe9 — a regression test.

Nothing in the suite failed when the block scalar was reverted; it stayed at the same pass/fail count either way. CONTRIBUTING asks that a change to generated YAML be covered by a test that parses and exercises the rendered workflow rather than checking text alone, so the test runs the real installer for three provision shapes — none detected (the ": "-bearing fallback), a command containing ": ", and an ordinary command — parses all eight rendered workflows, and asserts the parsed Provision step's run round-trips the command it was given. Reverting the block scalar now fails it with facility-address-review.yml must parse as YAML.

Asserting the parsed value rather than only that parsing succeeded is deliberate: it also catches a future rendering change that escapes or mangles the command quietly.

yaml is added as a root devDependency for this. It was already in pnpm-lock.yaml as a transitive dependency at the same version, so no new package enters the tree, and as a devDependency it leaves the CLI's no-runtime-dependency rule for what it vendors into adopter repositories untouched.

Verification: all eight workflows parse across all three provision shapes; @theagilemonkeys/facility suite 98 pass / 0 fail / 1 skip; biome check and node guards/run.mjs clean.

One thing deliberately left out, filed as #269: the command is also interpolated raw into the --append-system-prompt "…" shell string, so --provision='… echo "db: ready"' truncates the system prompt mid-argument. Pre-existing and a different context from the run: scalar, so it is not this PR's job — but worth noting that this PR makes it more reachable, since those commands now produce a workflow that parses and runs instead of failing loudly.

On your "deliberately not included": agreed that doctor parsing its own generated workflows is the fix for the whole class. The test added here closes the regression path for this instance; #217 is still the right place for the general one.

javiertoledo added a commit to cdotta/facility that referenced this pull request Sep 1, 2026
…erer

packages/core/src/render.ts renders the same templates as the CLI from its
own variable map, and that map carried PROVISION_CMD but not PROVISION_RUN.
Since the templates now interpolate the provision command through a block
scalar, core emitted a literal "{{PROVISION_RUN}}" where the CLI emits the
command, so the two renderers disagreed and every workflow core produced
carried an unsubstituted placeholder.

The byte-for-byte parity test caught it, which is what it is for. It was
failing before this branch added anything — at 4890d90 on facility-codex.yml,
and after the remaining three templates were converted on
facility-address-review.yml, which merely sorts earlier. It went unseen
because CI does not run on a fork pull request until a maintainer approves it.

provisionRun mirrors the CLI's implementation exactly, alongside the copy of
checksRun already kept in step the same way.

The parity test now runs three provision shapes instead of one: an ordinary
command, a command containing ": ", and no command at all so both sides take
the ": "-bearing fallback. The old single shape could only catch a raw
placeholder; the new ones also catch a future divergence in how either
renderer escapes or indents the command. All three fail if PROVISION_RUN is
removed again.

Addresses: CI minimum-node and verify on theam#220

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LyXT6m6npUH46VBuY1mPGM
@javiertoledo

Copy link
Copy Markdown
Member

CI ran on this branch for the first time — a fork PR's checks stay action_required until a maintainer approves them, so nothing had ever run here — and verify and minimum-node both failed. Pushed d2393e1 to fix it.

@facility/core renders these same templates from its own variable map, and that map had PROVISION_CMD but not PROVISION_RUN. So core emitted a literal {{PROVISION_RUN}} where the CLI emits the command, and matches the real CLI init output byte-for-byte failed.

Worth being clear that this was not introduced by the extra templates — it fails at your 4890d90 too, I checked out the commit and ran it:

4890d90  × matches the real CLI init output byte-for-byte
         AssertionError: .github/workflows/facility-codex.yml
e66abe9  × matches the real CLI init output byte-for-byte
         AssertionError: .github/workflows/facility-address-review.yml

Same defect; converting the other three templates only changed which file sorts first. packages/core/src/render.ts keeps a byte-identical copy of checksRun for exactly this reason, so provisionRun now sits beside it and the map gains the entry.

I also widened that parity test from one provision shape to three — an ordinary command, one containing ": ", and none at all so both sides take the ": "-bearing fallback. The single shape it had could only ever catch an unsubstituted placeholder; the new ones also catch a future divergence in how either renderer escapes or indents the command. All three fail if PROVISION_RUN is removed again.

Verification: @facility/core 41/41; CLI suite 98 pass / 0 fail / 1 skip; tsc --noEmit clean; biome check and node guards/run.mjs clean.

Two notes for whoever merges:

  • The branch is behind main, which has since landed 8889753 fix(toolchain)!: standardize on Node 24 LTS. That commit touches packages/cli/src/init.mjs and facility-codex.yml (node-version: 2224) in hunks several lines away from this PR's, so it should merge cleanly, but it wants an update before merge.
  • CI here tested the PR head directly rather than a merge commit, so a green run is not yet evidence about the merged result.

@javiertoledo

Copy link
Copy Markdown
Member

CI approved and re-run on d2393e1. The parity fix worked:

check before now
minimum-node fail pass
self-host-build pending pass
sandbox-e2e pass pass
commit-subjects / pull-request-title pass pass
verify fail fail — unrelated, see below

verify is now failing only on its dependency audit, not on anything in this branch. Lint, build, tests and guards all pass inside that job; it exits on two high-severity browserslist advisories (<=4.28.6, patched in 4.28.7), reached transitively through apps/docs → docusaurus → babel.

That is repo-wide and already owned by #267 ("floor browserslist at 4.28.7 for today's advisories"). #245's verify fails the same way right now. Nothing here can fix it, and adding a second override would duplicate #267 — so this PR is green on its own merits once #267 lands and this branch is updated.

The branch still wants that update anyway: main has moved on by 8889753 fix(toolchain)!: standardize on Node 24 LTS, which touches packages/cli/src/init.mjs and facility-codex.yml a few lines from this PR's hunks. Also worth noting CI tested the PR head directly rather than a merge commit, so these results do not speak for the merged tree.

cdotta and others added 4 commits September 2, 2026 18:08
facility-crew.yml and facility-codex.yml were unparseable on any repository
where init detected no provision command, so neither agent trigger ran:

  facility-crew.yml:196:27: could not parse as YAML: mapping values are not
  allowed in this context [syntax-check]

The command is interpolated into a bare scalar, and the no-provision fallback
text contains "facility: ". Any provision command containing ": " breaks the
same way, e.g. 'docker compose up -d && echo "db: ready"'.

Rendered into a block scalar, matching what checksRun already does for the
checks list. Message text is unchanged; only its YAML context is.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…kflow

The block scalar landed in facility-crew.yml and facility-codex.yml, but
init renders the same interpolation into three more workflows, which kept
the bare scalar and so kept the bug:

  facility-review.yml:51
  facility-address-review.yml:64
  facility-doctor.yml:112

A default `facility init --yes` on a repository where no provision command
is detected still produced three workflows GitHub cannot parse — the
fallback text contains "facility: ", and the colon-space ends the scalar:

  FAIL facility-address-review.yml: mapping values are not allowed here
  FAIL facility-doctor.yml:         mapping values are not allowed here
  FAIL facility-review.yml:         mapping values are not allowed here

All three sites indent `run:` by eight spaces exactly like the two already
fixed, so PROVISION_RUN applies unchanged.

Addresses: review finding — packages/cli/templates/workflows/*.yml

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LyXT6m6npUH46VBuY1mPGM
CONTRIBUTING requires that a change to generated YAML be covered by a test
that parses and exercises the rendered workflow rather than checking text
alone. Nothing failed when the block scalar was reverted, so the fix was
unprotected: the suite stayed at 94 pass / 3 pre-existing fail either way.

The test runs the real installer into a temporary repository for three
provision shapes — none detected (the ": "-bearing fallback), a command
containing ": ", and an ordinary command — parses all eight rendered
workflows, and asserts the parsed Provision step's `run` round-trips the
command it was given. Reverting the block scalar fails it with
"facility-address-review.yml must parse as YAML".

Asserting the parsed value, not just that parsing succeeded, is what stops
a future rendering change from escaping or mangling the command quietly.

`yaml` is added as a root devDependency. It was already in the lockfile as a
transitive dependency at the same version, so no new package enters the tree,
and being a devDependency it leaves the CLI's no-runtime-dependency rule for
what it vendors into adopter repositories untouched.

Addresses: review finding — packages/cli/test/init.test.mjs (absent coverage)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LyXT6m6npUH46VBuY1mPGM
…erer

packages/core/src/render.ts renders the same templates as the CLI from its
own variable map, and that map carried PROVISION_CMD but not PROVISION_RUN.
Since the templates now interpolate the provision command through a block
scalar, core emitted a literal "{{PROVISION_RUN}}" where the CLI emits the
command, so the two renderers disagreed and every workflow core produced
carried an unsubstituted placeholder.

The byte-for-byte parity test caught it, which is what it is for. It was
failing before this branch added anything — at 4890d90 on facility-codex.yml,
and after the remaining three templates were converted on
facility-address-review.yml, which merely sorts earlier. It went unseen
because CI does not run on a fork pull request until a maintainer approves it.

provisionRun mirrors the CLI's implementation exactly, alongside the copy of
checksRun already kept in step the same way.

The parity test now runs three provision shapes instead of one: an ordinary
command, a command containing ": ", and no command at all so both sides take
the ": "-bearing fallback. The old single shape could only catch a raw
placeholder; the new ones also catch a future divergence in how either
renderer escapes or indents the command. All three fail if PROVISION_RUN is
removed again.

Addresses: CI minimum-node and verify on theam#220

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LyXT6m6npUH46VBuY1mPGM
@javiertoledo
javiertoledo force-pushed the fix/provision-block-scalar branch from d2393e1 to f46e46e Compare September 2, 2026 17:10
@javiertoledo

Copy link
Copy Markdown
Member

Rebased onto main (39503f3) and re-ran CI. Everything is green.

check result
verify pass (5m6s)
minimum-node pass (1m51s)
self-host-build pass (11m7s)
sandbox-e2e / commit-subjects / pull-request-title pass

The verify failure I reported earlier is gone: it was the repo-wide browserslist audit, fixed on main by 689b7ff fix(deps): require patched browserslist. Nothing in this branch had to change for it.

The rebase replayed all four commits with no conflicts, even though main had moved ten commits ahead and touched six of the same files. Both sides survived intact — the five block scalars and core's PROVISION_RUN are still there, and 8889753's node-version: 2224 is present in init.mjs, render.ts and facility-codex.yml.

Verified locally after the rebase before pushing: pnpm install --frozen-lockfile clean, @facility/core 41/41, CLI suite 98 pass / 0 fail / 1 skip, tsc --noEmit, biome check and node guards/run.mjs all clean.

PR is now MERGEABLE / CLEAN and up to date with main, so CI is finally testing what would actually land.

Still open and deliberately not fixed here: #269, the shell-quoting truncation in --append-system-prompt. Pre-existing, different context, needs its own change.

@javiertoledo
javiertoledo merged commit 3c6e1be into theam:main Sep 2, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants