Fix comment and quoting gaps in the workflow checker

docs/workflows.md

@@ -1,49 +1,51 @@
 # Workflow convention
 
-Every GitHub Actions workflow and composite action in the studio's repositories follows these rules, so a workflow in one repository reads like one in any other and the same mistakes are caught everywhere. The `Checks / Workflows` check enforces them on every pull request: actionlint for syntax and shell scripts, zizmor for security, and `scripts/check-workflows.mjs` for the rest. The few rules that need judgement are kept in review and marked as such.
+Every GitHub Actions workflow and composite action in the studio's repositories follows these rules, so a workflow in one repository reads like one in any other and the same mistakes are caught everywhere. The `Checks / Workflows` check enforces them on every pull request: actionlint for syntax and shell scripts, zizmor for security, and `scripts/check-workflows.mjs` for the rest. Rules that no tool checks are kept in review and marked as such.
 
 ## Files
 
 - Workflows end in `.yml` and start without a `---` document marker.
 - Top-level keys come in this order: `name`, `on`, `permissions`, `concurrency`, `defaults`, `env`, `jobs`.
 - `on` is a map. A flow list holds at most one item (`branches: [main]`); two or more items go in a block list.
-- The files stay inside the plain subset of YAML the checker reads: block style with 2-space indentation, no anchors or aliases, no flow maps.
+- The files stay inside the plain subset of YAML the checker reads: block style with 2-space indentation, no anchors or aliases, no flow maps. The checker reports anchors, aliases and flow maps; indentation and values continued on the next line are kept in review.
 
 ## Permissions and secrets
 
-- A workflow grants no more than reading by default: `permissions` is always set at the top, normally to `contents: read`. Write scopes go on the one job that needs them.
-- No code from a pull request runs with a write scope. A job that runs on pull requests holds one only if every step that uses it is skipped on pull requests, or if the job checks nothing out and its steps are pinned actions that act on the pull request itself. The first case covers today's single build-and-push jobs of the publish workflows, until they move to a shared publish workflow that splits building from pushing; the second covers labeling pull requests.
+- A workflow grants no more than reading by default: `permissions` is always set at the top, normally to `contents: read`.
+- Write scopes go on the one job that needs them (review).
+- No code from a pull request runs with a write scope (review). A job that runs on pull requests holds one only if every step that uses it is skipped on pull requests, or if the job checks nothing out and its steps are pinned actions that act on the pull request itself. The first case covers today's single build-and-push jobs of the publish workflows, until they move to a shared publish workflow that splits building from pushing; the second covers labeling pull requests.
 - Secrets are passed only on events other than `pull_request`, except upload tokens that can only add reports, such as Codecov's (review).
-- Checkouts set `persist-credentials: false`; a job that pushes keeps the credentials and says why in a comment.
-- Every action is pinned by its full commit SHA with the release in a comment, `@<sha> # vX.Y.Z`. Renovate keeps the pins current.
+- Checkouts set `persist-credentials: false`; a job that pushes keeps the credentials and says why in a comment (the comment: review).
+- Every action is pinned by its full commit SHA with the release in a comment, `@<sha> # vX.Y.Z` (the comment: review). Renovate keeps the pins current.
 
 ## Concurrency and shell
 
-- A new push to a pull request cancels the run it replaces: every workflow that runs on `pull_request` sets `concurrency` with `group: ${{ github.workflow }}-${{ github.ref }}` and `cancel-in-progress: ${{ github.event_name == 'pull_request' }}`.
+- A new push to a pull request cancels the run it replaces: every workflow that runs on `pull_request` sets `concurrency` with `group: ${{ github.workflow }}-${{ github.ref }}` and `cancel-in-progress: ${{ github.event_name == 'pull_request' }}` (the values: review).
 - A failing command fails its step, including inside a pipe: every workflow with a `run` step sets `defaults.run.shell: bash`. Naming bash explicitly turns on `pipefail`; the implicit default shell does not.
-- `working-directory` lives in `defaults.run`: at workflow level when every job shares it, otherwise per job. A step sets its own only when it alone differs. Action inputs such as `with: working-directory` are not affected.
+- Steps do not set their own `shell`; a step that needs another shell starts it from bash, as in `pwsh -File script.ps1`.
+- `working-directory` lives in `defaults.run`: at workflow level when every job shares it, otherwise per job. A step sets its own only when it alone differs. Action inputs such as `with: working-directory` are not affected (review).
 
 ## Jobs
 
 - Every job has a `name` and a `timeout-minutes`, about two to three times its usual duration. A job that calls a reusable workflow cannot set a timeout and is exempt.
 - A job that is a required status check keeps its name, because renaming it leaves pull requests waiting for a check that never reports (review).
-- A value several steps use goes in the job's `env` once.
+- A value several steps use goes in the job's `env` once (review).
 
 ## Steps
 
-- Every step has a sentence-case `name` as its first key, and `run` comes last.
+- Every step has a `name` as its first key, in sentence case (review), and `run` comes last.
 - A step runs one command that lives in the repository: a package script, `uv run …`, or a script file. Shell longer than a few lines moves into a script, so local runs and CI run the same thing and shellcheck sees it (review).
 - `if` conditions are written without `${{ }}`, and use `!cancelled()` instead of `always()`.
 - No `${{ }}` inside `run`: values reach the shell through `env`, so nothing from the event payload is spliced into a script.
 
 ## Spacing and quoting
 
 - Exactly one empty line separates top-level keys, jobs and steps; there is none right before `steps`. A job with `services` puts them right before `steps`, with one empty line before and after them.
-- Plain strings go unquoted; patterns such as globs and cron expressions, and strings YAML requires quoted, use double quotes.
+- Plain strings go unquoted; patterns such as globs and cron expressions, and strings YAML requires quoted, use double quotes. The checker reports single quotes; which strings need quotes is kept in review.
 
 ## Composite actions
 
-The same rules apply to `action.yml` files, except that GitHub requires `shell` on every `run` step there.
+The same rules apply to `action.yml` files, except that GitHub requires `shell` on every `run` step there. actionlint does not read `action.yml` files, so scripts in composite actions get no shellcheck: they stay at one command or move into a script file (review).
 
 ## Calling the shared checks
 

scripts/settings.mjs

@@ -17,7 +17,6 @@ export function desiredSettings(baseline, override) {
   return desired;
 }
 
-// A "removed" or "detached" status still names the configuration the repo had.
 // GraphQL names of the merge settings, keyed by their REST names.
 const MERGE_FIELDS = {
   allow_squash_merge: "squashMergeAllowed",
@@ -34,7 +33,8 @@ export const MERGE_QUERY = `query($owner: String!, $name: String!) { repository(
 
 // Merge settings come from GraphQL: the REST repository leaves them out for tokens without contents write, such as the read-only drift check's.
 export function currentSettings({ repository, merge, workflow, securityFixes, configuration }) {
-  const applies = configuration !== null && ["attached", "enforced"].includes(configuration.status);
+  // A "removed" or "detached" status still names the configuration the repo had.
+  const applies =configuration !== null && ["attached", "enforced"].includes(configuration.status);
   const mergeSettings = merge ? Object.fromEntries(Object.entries(MERGE_FIELDS).map(([rest, graphql]) => [rest, merge[graphql]])) : {};
   return {
     repository: { ...repository, ...mergeSettings },

scripts/workflow-style.mjs

@@ -1,13 +1,16 @@
 // Checks a workflow or composite action against the workflow convention (docs/workflows.md).
 // The convention keeps files to a small YAML subset (2-space block style, no anchors, no flow
-// maps, flow lists of at most one item), so an indentation-based reader is enough; anything
-// outside that subset is reported rather than parsed.
+// maps, flow lists of at most one item), so an indentation-based reader is enough. Anchors,
+// aliases, flow maps and longer flow lists are reported; other constructs outside the subset,
+// such as plain scalars continued on the next line, are not detected and may be misread.
 
 const KEY = /^(?:"([^"]+)"|([A-Za-z_][\w.-]*)):(?:\s+(.*))?$/;
 const BLOCK_SCALAR = /^[|>][+-]?\d*\s*(#.*)?$/;
 const TOP_LEVEL_ORDER = ["name", "run-name", "on", "permissions", "concurrency", "defaults", "env", "jobs"];
 
-const withoutComment = (value) => value.replace(/\s+#.*$/, "");
+// KEY has already consumed the space after the colon, so a value that is only a comment starts with "#".
+const withoutComment = (value) => value.replace(/(^|\s+)#.*$/, "");
+const unquoted = (value) => value?.replace(/^"(.*)"$|^'(.*)'$/, "$1$2");
 
 function parse(text) {
   const lines = text.split("\n");
@@ -211,19 +214,19 @@ function checkWorkflowRules(root, kinds, report) {
   const permissions = child(root, "permissions");
   if (!permissions) report(1, "missing top-level permissions");
   else {
-    if (permissions.value === "write-all") report(permissions.line, "top-level permissions must be read or none");
-    for (const scope of keys(permissions)) if (scope.value === "write") report(scope.line, "top-level permissions must be read or none");
+    if (unquoted(permissions.value) === "write-all") report(permissions.line, "top-level permissions must be read or none");
+    for (const scope of keys(permissions)) if (unquoted(scope.value) === "write") report(scope.line, "top-level permissions must be read or none");
   }
 
   const on = child(root, "on");
   if (on) {
-    if (on.value) report(on.line, "on must be a map");
+    if (on.value || on.children.some((entry) => entry.isItem)) report(on.line, "on must be a map");
     else if (child(on, "pull_request") && !child(root, "concurrency")) report(on.line, "missing concurrency for pull_request");
   }
 
   const jobs = keys(child(root, "jobs"));
   const hasRun = jobs.some((job) => (child(job, "steps")?.children ?? []).some((step) => child(step, "run")));
-  if (hasRun && child(child(child(root, "defaults"), "run"), "shell")?.value !== "bash") report(1, "missing defaults.run.shell: bash");
+  if (hasRun && unquoted(child(child(child(root, "defaults"), "run"), "shell")?.value) !== "bash") report(1, "missing defaults.run.shell: bash");
 
   jobs.forEach((job, index) => {
     if (index > 0 && blanksBetween(kinds, jobs[index - 1].endLine, job.line) !== 1) report(job.line, "one empty line between jobs");

scripts/workflow-style.test.mjs

@@ -140,6 +140,26 @@ test("on must be a map", () => {
     { line: 3, message: "flow lists hold at most one item" },
     { line: 3, message: "on must be a map" },
   ]);
+  const blockList = doc(header.slice(0, 2), "on:", "  - pull_request", header.slice(4), job, step);
+  assert.deepEqual(check(blockList), [{ line: 3, message: "on must be a map" }]);
+  const unindented = doc(header.slice(0, 2), "on:", "- pull_request", header.slice(4), job, step);
+  assert.deepEqual(check(unindented), [{ line: 3, message: "on must be a map" }]);
+});
+
+test("a comment after a key is not its value", () => {
+  assert.deepEqual(check(doc(header.slice(0, 2), "on: # triggers", header.slice(3), job, step)), []);
+  const unchecked = ["    - shell: python", "      run: echo a"];
+  const expected = [
+    { line: 23, message: "no per-step shell" },
+    { line: 23, message: "step has no name" },
+  ];
+  assert.deepEqual(check(doc(header, job.slice(0, 4), "    steps:", unchecked)), expected);
+  assert.deepEqual(check(doc(header, job.slice(0, 4), "    steps: # build", unchecked)), expected);
+});
+
+test("quoted values compare like plain ones", () => {
+  assert.deepEqual(check(doc(header.slice(0, 14), '    shell: "bash"', header.slice(15), job, step)), []);
+  assert.deepEqual(check(doc(header.slice(0, 6), '  contents: "write"', header.slice(7), job, step)), [{ line: 7, message: "top-level permissions must be read or none" }]);
 });
 
 test("job name and timeout", () => {