feat(cli): run ast-grep against the committed sg config - #73
Conversation
thecodedrift
added
the
skip-changeset
9cf8674 to
655179e
Compare
thecodedrift
removed
the
skip-changeset
thecodedrift
changed the title
3f6ea8f to
e773da4
Compare
eeed898 to
796423c
Compare
On a stack, one changeset describes the whole change and lives on the bottom PR — the one targeting `main`, which is where the check has to pass. Branches above inherit that file, so it is never *added* in a child's own diff and every mid-stack PR failed. Labelling them `skip-changeset` records a deliberate "ships no release note," which is false, so the workflow bypasses on the base ref instead. The `on: branches: [main]` filter does not hold for stacked PRs in practice — the workflow still runs on a PR based on a feature branch — so the base is re-checked inside the step rather than trusted. Observed on #80, #81, and #73, all of which ran and failed the check with a non-main base.
On a stack, one changeset describes the whole change and lives on the bottom PR — the one targeting `main`, which is where the check has to pass. Branches above inherit that file, so it is never *added* in a child's own diff and every mid-stack PR failed. Labelling them `skip-changeset` records a deliberate "ships no release note," which is false, so the workflow bypasses on the base ref instead. The `on: branches: [main]` filter does not hold for stacked PRs in practice — the workflow still runs on a PR based on a feature branch — so the base is re-checked inside the step rather than trusted. Observed on #80, #81, and #73, all of which ran and failed the check with a non-main base.
1993325 to
cb835f1
Compare
…laim Review pushed back on three statements, and two of them were wrong. `propagate_stack.py` does return you to the branch you started on — normal completion, rebase conflict, balloon guard, and push failure all check it out before returning. Only a failure to check out a *child* (exit 6, typically that branch being held by a worktree) returns early without it. That is the path that actually bit, so the warning now names it instead of describing the script's normal behavior. The `branches: [main]` claim is reframed rather than dropped. It is documented to filter on the base branch, and the guard should be unreachable — but #73, #80, and #81 each produced a failing `Require a changeset` check run with an `openspec/partition-engine-*` base, and a workflow that never triggers produces no check run at all. The comment now cites that evidence and presents the check as a defensive guard, without teaching that Actions filters are unreliable in general. Also replaces the "structural failures are acceptable" guidance with the opposite rule: a mid-stack PR that is red is incomplete, and the fix belongs in it. That advice let this stack sit on 20 failures that turned out to be two real defects.
…l target `branches: [main]` reads as "only PRs whose base is main," and filtered that way when a PR had one base. Under GitHub's stacked-PR support a stacked PR targets `main` eventually, so the filter matches the eventual target and these workflows run on mid-stack PRs too — which is why #73, #80, and #81 each produced a failing `Require a changeset` run with an `openspec/partition-engine-*` base. That makes the base-ref guard required rather than defensive, and generalizes: a workflow whose correctness depends on "is this the PR that merges to main" has to establish that itself. Corrects the OpenSpec archive-check section too, which told readers a stacked PR would not run that check at all.
On a stack, one changeset describes the whole change and lives on the bottom PR — the one targeting `main`, which is where the check has to pass. Branches above inherit that file, so it is never *added* in a child's own diff and every mid-stack PR failed. Labelling them `skip-changeset` records a deliberate "ships no release note," which is false, so the workflow bypasses on the base ref instead. The `on: branches: [main]` filter does not hold for stacked PRs in practice — the workflow still runs on a PR based on a feature branch — so the base is re-checked inside the step rather than trusted. Observed on #80, #81, and #73, all of which ran and failed the check with a non-main base.
…laim Review pushed back on three statements, and two of them were wrong. `propagate_stack.py` does return you to the branch you started on — normal completion, rebase conflict, balloon guard, and push failure all check it out before returning. Only a failure to check out a *child* (exit 6, typically that branch being held by a worktree) returns early without it. That is the path that actually bit, so the warning now names it instead of describing the script's normal behavior. The `branches: [main]` claim is reframed rather than dropped. It is documented to filter on the base branch, and the guard should be unreachable — but #73, #80, and #81 each produced a failing `Require a changeset` check run with an `openspec/partition-engine-*` base, and a workflow that never triggers produces no check run at all. The comment now cites that evidence and presents the check as a defensive guard, without teaching that Actions filters are unreliable in general. Also replaces the "structural failures are acceptable" guidance with the opposite rule: a mid-stack PR that is red is incomplete, and the fix belongs in it. That advice let this stack sit on 20 failures that turned out to be two real defects.
…l target `branches: [main]` reads as "only PRs whose base is main," and filtered that way when a PR had one base. Under GitHub's stacked-PR support a stacked PR targets `main` eventually, so the filter matches the eventual target and these workflows run on mid-stack PRs too — which is why #73, #80, and #81 each produced a failing `Require a changeset` run with an `openspec/partition-engine-*` base. That makes the base-ref guard required rather than defensive, and generalizes: a workflow whose correctness depends on "is this the PR that merges to main" has to establish that itself. Corrects the OpenSpec archive-check section too, which told readers a stacked PR would not run that check at all.
cb835f1 to
a8e2c30
Compare
|
Claude finished @thecodedrift's task in 4m 2s —— View job Review: PR #73 — run ast-grep against the committed sg config
SummaryThe core change is well-scoped: Three issues found, none blocking correctness of the shipped behavior: 1. 2. Stale/inaccurate doc comment newly added in 3. Test flakiness risk in Nothing else stood out — the Branch: |
a0d12ea to
ab597f1
Compare
ab597f1 to
c53f866
Compare
c53f866 to
ac4b725
Compare
ac4b725 to
2024781
Compare
2024781 to
bd6d473
Compare
bd6d473 to
2024781
Compare
2024781 to
14b8fd5
Compare
`check` and `rule verify` now point ast-grep at `.taskless/sg/sgconfig.yml` — committed, authored once, read as-is. Nothing is generated on the check path, which is what makes the config the source of truth rather than a reconstruction of whatever happened to be on disk that run: a `ruleDirs` entry a human or the generator added is honoured, and a test proves it. `generateSgConfig` is not gone, only narrowed. Rule sets with no committed config of their own still need one written for them — the pre-migration `.taskless/rules/` layout and the materialized `.run/` set — so its `rulesDirectory` / `testDirectory` options remain load-bearing for those. `resolveSgConfigPath` is the one place that decides which of the two a rule set gets. Also covers the missing-binary path end to end: the bundled CLI, run from outside the workspace with an empty PATH, reports `ast-grep binary not found` and exits 1 rather than failing obscurely at spawn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ding Twelve help recipes, the package README, and the `.taskless/README.md` that migration 0001 writes still told agents and humans to look in `.taskless/rules/` and `.taskless/runtime-rules/`. They now name the engine directories, and the scaffolded README explains the partition itself — that the directory picks the engine and each holds that tool's own native config. Closes out the change's quality gates: typecheck, lint, and test are clean (421 passed), and `check` output was confirmed unchanged across the relayout on this repo's own rule set — same findings, same positions, same exit code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The constant lived in `scan.ts`, which made `filesystem/sgconfig.ts` import the spawn machinery to read one string. `engines.ts` imports nothing of ours but the error type, so both layers can reach it there without pulling in the other's dependencies. Typing ENGINE_LAYOUTS with `satisfies` narrows `sg.configFile` to its literal, which retires the `?? "sg/sgconfig.yml"` fallback that duplicated the same path.
Also corrects the cli-check delta, which filed two rewrites of existing requirements under ADDED. The archive refused it — both headers already exist in the spec — so they move to MODIFIED, which is what they always were. The changeset lives at the base of the stack, not here: Require Changeset runs only on PRs based on `main` and reads the PR's own diff, so the bottom PR is where it has to be. This branch inherits it.
The test asserted that no ast-grep exists anywhere reachable, but handed the child a copy of `process.env`. `NODE_PATH` and `HOME` both feed module resolution — node consults `$HOME/.node_modules` — so on CI the resolver found a binary, the scan ran, and the assertion failed against the *findings* path: exit 1 with empty stderr, which is indistinguishable from the error path on the exit code alone. The child now gets a minimal environment, and the failure message reports what went to stdout so the two paths can be told apart.
14b8fd5 to
92f513e
Compare
…erateSgConfig note Both reviewers flagged the same contradiction: the requirement updated by this change says the committed `sg/sgconfig.yml` is read and no config is generated at check time, while requirements a few lines above still described `.taskless/rules/` and "SHALL proceed to generate sgconfig.yml". The earlier requirements now describe the engine directory, and keep the pre-migration layout as the compatibility case it actually is — including a scenario for the unmigrated rule set, which was previously implied by nothing. The `generateSgConfig` note was also wrong, in the doc comment and in the archived tasks.md. It claimed two callers needed it: the legacy layout and the materialized `.run/` run set. Only the first is real — the runtime narrow writes its own sgconfig.yml in `rules/runtime/narrow.ts` and never calls generateSgConfig. That claim came from an answer I accepted on the strength of a test that only ever exercised the legacy split. Also hardens `runCli` in the committed-config tests: it shelled out to a PATH-resolved `node` while a test in the same file already used `process.execPath`, and it read `code` as a number when a signal death gives null, which would have reported a killed process as exit 0.
Both of the issues I can act on are fixed, and the second one was a genuine catch. The spec inconsistency is corrected — the earlier requirements now describe the The That claim originated in an answer I accepted because a test asserted the generated-vs-committed split. That test only ever exercised the legacy path, so it could not have caught the — AI Coding Agent |
Both findings addressed and resolved inline: the — AI Coding Agent |
Stack (root → tip):
checkandrule verifynow run ast-grep against the committed.taskless/sg/sgconfig.ymlinstead of writing an ephemeral config on every run.resolveSgConfigPathis the single place that decision is made.generateSgConfigis narrowed rather than deleted. Two rule sets still have no committed config of their own and need one generated: the pre-0004.taskless/rules/layout, and the materialized.run/run set. Both pass their directories explicitly, so those options are load-bearing for exactly those callers — a test asserts the split in both directions, that no config is written when onlysg/rules exist and one is written for the legacy layout.Also updates the twelve
help/*.txtrecipes, the CLI README, and the.taskless/README.mdwritten by migration0001, which now describes the partition rather than listing the old flatrules/andrule-tests/.Two smaller things ride along.
COMMITTED_SG_CONFIGmoves toengines.tsbeside the layout it derives from, sofilesystem/sgconfig.tsno longer imports thespawnmachinery inscan.tsto read one string; typingENGINE_LAYOUTSwithsatisfiesnarrowssg.configFileto its literal and retires a fallback that duplicated the same path. And thecli-checkspec delta filed two rewrites of existing requirements underADDED— the archive refused it, so they move toMODIFIED.Tip of a merge-down stack — #79 → #80 → #81 → this. The units are only correct together, so none reaches
mainalone: this merges down into #81, that into #80, into #79, and #79 makes the single protected merge. The OpenSpec change is archived here, on the tip, exactly once.Verification worth noting, since the interesting one is a substitute: the full suite is 421 passing, and the old and new check paths were run against an identical seed of this repo's own
.taskless/plus real sources, producing the same two findings — same rule, file, line, column, severity, message — and the same exit code. That baseline is a reproduction of what the old path ran, not the published binary, becausepnpm dlxis blocked in this environment.Fixes OSS-24
Built on top of #79
Migration
0004moves.taskless/to an engine-partitioned layout: ast-grep rules go tosg/rules/andsg/rule-tests/, the runtime tree toruntime/rules/andruntime/rule-tests/, andvale/is scaffolded but inert until the Vale engine lands. Files move without their bytes being touched, so runtime rule signatures survive the migration.Also anchors the
sgconfig.ymlgitignore pattern to.taskless/. The pattern was unanchored, so it would match a same-named file anywhere in the tree.This PR carries the OpenSpec change docs for the whole effort, including the spec deltas the later PRs implement.
This is the bottom of a merge-down stack — the units are only correct together, since this migration relocates rules that nothing yet knows how to find. Nothing here reaches
mainon its own: the PRs above merge down into this branch, and the accumulated result is what merges once. The change directory stays unarchived here by design; the archive gate skips non-tip PRs and the tip archives it.Refs OSS-24
Built on top of #80
The directory a rule sits in under
.taskless/is its engine. Dispatch reads the path and never parses a rule file to decide who owns it, so adding an engine is a directory, not a discriminator.planEngineDispatchresolves which engine directories are present; an unrecognized directory is ignored rather than guessed at, so a.taskless/written by a newer CLI degrades to running the engines this one understands instead of handing a rule to the wrong parser.Two compatibility behaviors are deliberate and worth reviewing as such:
0004.taskless/rules/still runs as ast-grep. An unmigrated checkout, or a producer that keeps naming the old path, executes rather than being silently ignored. Both sources are scanned and findings de-duplicated on the match itself, since a rule id can legitimately exist in only one of them.rules[].contentis documented as an ast-grep definition — so absence means ast-grep, permanently, not for a migration window. An unrecognized engine is the opposite case: it means the payload is newer than the CLI, so it throws and writes nothing rather than filing the rule where the wrong parser will read it.Part of a merge-down stack (see the base branch). Not independently shippable: rules have moved but runtime discovery still reads the old path until the next PR.
Refs OSS-24
Built on top of #81
Runtime discovery now reads
.taskless/runtime/rules/, deriving the path fromENGINE_LAYOUTS.runtimerather than repeating the string. Only the search path moves —runtime-check.test.tsandruntime-harness.test.tsseed the engine layout and otherwise pass unmodified, which is the evidence that rule semantics are untouched.This is what closes the loop on directory-based dispatch: a rule under
runtime/rules/can now actually reach the harness, so both engines are asserted end to end. A new test files a runtime-shaped capture undersg/rules/and confirms it stays static — the directory decides, not the rule's shape.Runtime deliberately gets no legacy tolerance. The pre-
0004fallback covers.taskless/rules/only. Since0004moves the runtime tree andcheckmigrates before discovering, a leftoverruntime-rules/is never a second source. A test asserts this, so the asymmetry with ast-grep is visible rather than accidental.Part of a merge-down stack (see the base branch).
Refs OSS-24