Skip to content

[#217] feat: tag-driven workflows — opt-in automation on tagged issues - #474

Open
rucka wants to merge 14 commits into
mainfrom
feature/US-217-tag-driven-workflows
Open

[#217] feat: tag-driven workflows — opt-in automation on tagged issues#474
rucka wants to merge 14 commits into
mainfrom
feature/US-217-tag-driven-workflows

Conversation

@rucka

@rucka rucka commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

PR Information

PR Title: [#217] feat: tag-driven workflows — opt-in automation on tagged issues
Story/Epic: #217 (epic #212 — Supervised automation)
Type: Feature
Priority: High
Assignee: @rucka
Labels: risk:red

Summary

What Changed

A team declares tag ⇒ workflow in .pair/adoption/tech/automation.md (## Workflows), and pair run --card <id> --card-tags <list> becomes the entry point a trigger calls: it routes that card to the workflow its tag names, and does nothing at all on every card nobody tagged.

Piece Where
## Workflows schema + reader workflow-mapping.ts · schema owned by the KB guideline, mirrored to the dataset
Routing core — trigger → eligibility → route → run dispatch.ts (decideDispatch): pure function of card + observed tags + policy + installed-skill probe
Entry point parser.ts (--card, --card-tags), handler.ts
Audit trail + one-run-per-card lock dispatch-audit.ts, card-lock.ts
Reference host adapter (GitHub Actions, documented only) KB github-automation.md

Why This Change

Automation must be opt-in per card, not per run: ## Eligibility (#216) answers which cards an unattended run may pick up; nothing answered what runs on a card once a trigger fires on it. R4.4 · Spec G10 · D18, D21.

Story Context

User Story: As a team, we want to declare a tag→workflow mapping in adoption, so that an unattended trigger routes each tagged card to the workflow its tag names — and does nothing on every card nobody tagged.

AC Where it is proven
AC1 — a tagged, eligible card runs its mapped workflow cli.e2e.test.ts BOARD rows 301/304 — real project dir, real lock, real audit file
AC2 — an untagged card runs nothing, and the skip is logged BOARD rows 302/303/305 + the negative no-start/no-route assertions
AC3 — the run start is recorded on the issue DISPATCH-RECORD: stdout line the host adapter posts; start only, never skip/end
AC4 — no ## Workflows ⇒ clean exit, no default workflow runs nothing at all when the project declares no mapping
AC5 — no classification criteria in the routing core (grep-verifiable) routing-purity.test.ts — the executable form of the DoD line

Changes Made

Implementation Details

  • Host-agnostic core. decideDispatch is told the card's labels; it never reads a tracker, holds no credentials, makes no network call. Adding a code host is a new thin adapter.
  • The order is normative — mapping → eligibility → routing. An ineligible card is skipped before its tags are looked at.
  • A dispatched card IS the run's scope, under the routed workflow's own argument name (--root for pair-loop, --story for pair-process-plan-tasks) and nothing displaces it: --root, --skill and --prompt are all refused alongside --card, and the handler reads the dispatched card before config.scope.root.
  • The mappable set is the KB catalog (DISPATCHABLE_WORKFLOWSpair-loop, pair-process-plan-tasks), a declaration of its own, asserted set-EQUAL to the guideline's catalog table. Two things do not put a skill in it, and each has its own refusal and its own message: knowing how it spells its scope (pair-next — a dispatch would take the card's lock and post a record for a run that only prints a recommendation), and being able to be scoped at all (pair-process-refine-story — see below).
  • A workflow that needs a human in the room is not mappable. A dispatch runs under --autonomous with nobody watching, holding the card's lock, on a card already carrying a public DISPATCH-RECORD: comment. A workflow whose own steps require an explicit human decision has two outcomes there and both are worse than not running: a stall until the per-iteration timeout, or an agent that answers its own gate. pair-process-refine-story is the concrete exclusion (its phase 0 is the R3.11 alignment gate); it keeps its SKILL_PARAMETERS row, because --skill pair-process-refine-story --root <card> is a legitimate hand-driven run. A conformance guard reads each catalogued workflow's own SKILL.md, so re-adding a row fails a test rather than shipping. ADR-024 item 8.
  • Fail-safe in one direction. No ## Workflows ⇒ "no mapping declared", clean exit. Uninstalled workflow, uncatalogued workflow, unscopable workflow, or a multi-tag card with no Precedence:HALT with an adoption-fix message, before eligibility and routing — whole board, deliberately.
  • One run per card. Exclusive mkdir lock under working_path, taken after every refusal and before anything spawns, released in a finally. A burst's second trigger is skipped, never queued, and the skip prints the holder's path and age.
  • Every decision is auditable or the run fails. appendAuditLine throws by design, so each of the three writes has a message an operator can act on: a crash writes outcome=crashed; a start that cannot be written is reported as a dispatch that never began, not as a crashed run; a skip — the commonest outcome on a board — names the card and states that nothing was spawned, instead of surfacing a bare EACCES.
  • The reference adapter provisions the binary it calls, and calls it by the name it publishes. Nothing on ubuntu-latest has heard of the CLI, so the shipped job installs it (setup-node + npm i -g @foomakers/pair-cli) before the dispatch step; the engine and its credentials stay the adopter's step, stated in the pre-flight. The step invokes pair-cli run@foomakers/pair-cli publishes exactly one bin, pair-cli, and ADL 2026-08-25 forbids a pair alias, so an earlier revision of this PR that installed the package and typed pair run shipped a step that was command not found/127 on every runner. That is now proven by a real install, not by a string match — see the Reviewer Guide.

Files Changed

  • Added: workflow-mapping.ts, dispatch.ts, dispatch-audit.ts, card-lock.ts (+ a test file each), routing-purity.test.ts, scripts/smoke-tests/scenarios/github-dispatch-adapter.sh, ADR-024, two ADLs
  • Modified: parser.ts, handler.ts, invocation.ts, perimeter.ts, metadata.ts, automation-policy.ts, resolve-skill.ts, policy-sections.ts, cli.e2e.test.ts; KB automation-policy.md / github-automation.md / README.md (dataset + mirror); architecture.md, way-of-working.md, llms.txt; docs adoption-files.mdx, commands.mdx, unattended-delivery.mdx, guidelines-catalog.mdx; conformance automation-eligibility.test.ts, github-automation.test.ts

40 files · +4848 / −173.

Services to Release

  • apps/pair-cli: pair-cli run gains --card / --card-tags and the whole dispatch path. Every existing invocation is byte-identical — the dispatch branch is entered only when --card is passed.

Testing

Test Coverage

  • Unit: every safety property is a tested module — untagged-never, eligibility-before-routing, no-silent-choice, one-run-per-card, the four refusals, and each of the three audit-write failure messages (crash / unwritten start / unwritten skip, the last including the lock-held skip, which must NOT release the holder's lock).
  • End-to-End: cli.e2e.test.ts drives five triggers at ONE real project directory through the command registry, with only the engine spawn injected — real lock, real appended audit file. The BOARD fixture is the assertion set (spawned prompts, audit lines, negative checks, DISPATCH-RECORD: list, released locks all iterate it), plus the burst case, the no-mapping board and the --root-refused case.
  • Corpus-pinned: invocation.test.ts reads the skills' real ## Arguments tables and the KB catalog table — the driver cannot spell an argument a skill does not declare, cannot dispatch a workflow the catalog does not name, and the catalog cannot name a workflow whose SKILL.md declares a human-judgment gate.
  • KB content is executed, not regexed: github-automation.test.ts parses the yaml the guideline ships and RUNS each run: step under the flags GitHub documents. The stubbed binary's name is now read from the document (invokedCommand()), never hardcoded — a stub named after an assumption is what let a dead step pass — and one case asserts that name equals the sole bin key declared by BOTH producers of the published manifest (apps/pair-cli/package.json and the bin: { … } literal in package-manual.sh).
  • The provisioned-artifact boundary is never stubbed: the new smoke scenario scripts/smoke-tests/scenarios/github-dispatch-adapter.sh (in CI_TESTS, OFFLINE_SAFE=true) runs the REAL package-manual.sh and create-registry-tgz.sh, npm install --global --prefix <clean temp prefix> of the resulting tarball, then executes the dispatch step extracted from the shipped guideline under env -i with only that prefix, node and the system bins on PATH — over BOTH the dataset copy and the root mirror. It asserts bin/pair-cli exists, bin/pair does not, the exit is not 127, and the installed CLI's own no mapping declared line appears (proof the real binary ran, not merely that some name resolved). Required by ADL 2026-08-31 — review baseline and provisioned-artifact contract.

Test Results

pnpm quality-gate                   →  exit 0, 22/22 turbo tasks
  (ts:check, test, lint, workflows:test, format:check, gate:composition,
   hygiene:check, smoke-modes:check, docs:staleness, skills:conformance, dup:check)
pnpm smoke-tests --ci               →  exit 0, 21/21 scenarios (incl. github-dispatch-adapter.sh)
pnpm --filter @pair/website e2e     →  39 passed
pair-cli: 1987 tests · knowledge-hub conformance: 4035 tests (2250 in the two files this story owns)

Pre-merge tiering: disabled in way-of-working.md, so the full suite is the CI-parity set regardless of the risk:red tag.

Branch is merged up to origin/main at e7d375bf (merge commit 5460fc48; one .pair/llms.txt decision-log conflict, resolved as the union of both sides).

Re-review scope (please read before starting). Per ADL 2026-08-31, the baseline is reviewedHead = e4b42cc8 (the last completed review). A re-review verifies the previously open findings plus git diff e4b42cc8...origin/feature/US-217-tag-driven-workflows and the directly changed producer/consumer boundary — it is not a rescan of the accumulated PR.

Deliberately out of scope, and already carded: apps/website/content/docs/** still writes bare pair <cmd> throughout (commands.mdx alone has ~40 lines). That mechanical rename, and the docs-staleness INVOCATION_PREFIX widening that keeps it fixed, are story #449's per ADL 2026-08-25. Renaming part of it here would leave one page internally inconsistent and pre-empt that story; the guideline fixed in this PR is the one that ships an executable, copy-verbatim artifact.

Testing Strategy

  • Happy path: eligible + mapped card ⇒ mapped workflow, scoped to itself, start and end in the trail, DISPATCH-RECORD: on stdout.
  • Edge cases: unlabelled card (--card-tags "" — data, not a malformed flag), eligible-but-unmapped, mapped-but-ineligible, two mapped tags with/without Precedence:, trigger burst, lock released mid-probe, dangling lock symlink, a ## Workflows section pasted with its markdown fence.
  • Error handling: engine crash ⇒ outcome=crashed then rethrow, lock still released; crash + unwritable audit ⇒ one error carrying both messages, engine error as cause; unwritable audit on the START ⇒ "nothing was spawned", no false crash claim; unwritable audit on a SKIP ⇒ the card and "nothing was spawned", not a bare filesystem error.

Quality Assurance

Review Areas

  • dispatch.ts — the order (mapping → eligibility → routing) and the HALTs. assertWorkflowsScopable deliberately has two messages: an uncatalogued workflow is a mapping to change, an unscopable one is a catalog entry with no argument row — telling the first it needs an argument row sends a maintainer to add one that exists.
  • invocation.tsSKILL_PARAMETERS vs DISPATCHABLE_WORKFLOWS: two tables answering two different questions ("how does this skill spell its scope" vs "may a tag route a card here"). pair-process-refine-story is in the first and not the second, on purpose.
  • handler.ts driveDispatchedCard — lock → audit → drive → release ordering, the started flag, and recordSkip/recordCrash keeping their messages true about what did and did not happen.
  • parser.ts resolveDispatch — what is refused alongside --card, and why --filter is not.
  • KB automation-policy.md — the schema bullet, the catalog table and the "needs a human in the room" rule must state ONE rule; three conformance guards hold them together, one of them against the catalogued skills' own SKILL.md.

Documentation

  • KB (dataset + regenerated .pair mirror): automation-policy.md ## Workflows schema, HALT list, the workflow catalog with its "how the dispatched card reaches it" column and the § "A workflow that needs a human in the room is not mappable"; github-automation.md reference adapter (five steps — checkout, setup-node, install, dispatch, record) + pre-flight.
  • Docs site: concepts/adoption-files, reference/cli/commands, tutorials/unattended-delivery, reference/guidelines-catalog.
  • Decisions: ADR-024 (adoption mapping + pure routing core + thin host adapter; the refusals that keep the trail true; item 8 — only a workflow that can finish unattended is mappable), ADL — atomicity primitives use node:fs directly, ADL — an empty --card-tags is an observation.

Risk Assessment

Risk Impact Probability Mitigation
A broken mapping line HALTs dispatch for the whole board High Low Deliberate and documented in the guideline, the adapter pre-flight, commands.mdx and ADR-024 — surfacing it only on the card carrying that tag would make the failure depend on which trigger fired first
Tag-driven automation does not cover refinement Med Accepted, not worked around (ADR-024 trade-off): the only Draft→Ready path requires a human, so refinement stays hand-driven. An unattended path past that gate would be a change to D24, not to this ADR
The per-card lock is filesystem-local Med Med Stated in six places: on ephemeral runners the HOST concurrency group is the cross-job guard; the lock guards one working area
Nothing reaps a stale lock Med Low The skip prints the holder's path and age, and the KB pre-flight documents clearing it. A TTL was rejected — a lock expiring while its run is alive re-creates the race
Labels are as fresh as the trigger that passed them Low Med Accepted: re-reading them needs the tracker client ADR-024 exists to avoid, and the invoked skill re-checks eligibility per iteration

Reviewer Guide

Testing the Changes

git checkout feature/US-217-tag-driven-workflows
pnpm install
pnpm quality-gate
pnpm --filter @pair/pair-cli exec vitest run src/commands/run src/cli.e2e.test.ts
pnpm --filter @pair/knowledge-hub exec vitest run src/conformance/automation-eligibility.test.ts src/conformance/github-automation.test.ts

Key Test Scenarios

  1. Routed card: pair run --card 217 --card-tags "auto-dev,risk:green" --dry-run against a project declaring auto-dev ⇒ pair-loop — prints the route, spawns nothing.
  2. Opt-in boundary: the same command with --card-tags "" — skipped cleanly, exit 0, skip logged.
  3. Refusals: --card 217 --root 300; a mapping naming pair-next or pair-process-review; a mapping naming pair-process-refine-story (installed, scopable, still refused — "the KB catalog does not name as mappable"); a ## Workflows section pasted inside its fence. Each refused with the file and the value to fix.

Dependencies & Related Work

rucka added 7 commits August 30, 2026 18:31
- Sweep bounded contract surfaces before re-review\n- Preserve every accepted finding\n\nRefs: #220, #441
- KB schema (dataset + mirror): seventh section of tech/automation.md —
  `<tag> ⇒ <workflow>` entries + optional `Precedence:`, tag as opaque
  routing key (D18), untagged ⇒ never, absent section ⇒ no mapping (opt-in,
  never an error), eligibility before routing, read-time + routing-time HALTs
- workflow-mapping.ts: parses/validates the section; unknown-workflow and
  multi-tag rules deliberately left to routing (they need board + skill set)
- policy-sections.ts: section/HALT/label primitives extracted from
  automation-policy.ts so both readers of the file share one answer
- conformance guard extended over dataset + mirror
- Task: T1 — Mapping schema in adoption + validation

Refs: #217
- dispatch.ts: agnostic routing core. mapping → eligibility → routing, in
  that order (BR3); untagged/unmapped/ineligible ⇒ reported skip, never a
  default workflow (AC2); no `## Workflows` ⇒ "no mapping declared", clean
  exit (AC4); uninstalled workflow and undecidable multi-tag ⇒ HALT
- `pair run --card <id> --card-tags <list>`: the entry point a host trigger
  calls (ADR-021 tier 2). Both values are host DATA, content-checked at
  parse; --skill/--prompt alongside --card refused (one answer, not two)
- a routed card invokes the MAPPED workflow (source: mapping) and scopes
  the run with --root <card>; a skip spawns nothing
- Task: T2 — Dispatcher: trigger → eligibility check → route → run

Refs: #217
- dispatch-audit.ts: start/skip/end appended to the `## Audit Location`
  file; the start record also printed as `DISPATCH-RECORD:` — the line the
  trigger's host adapter posts on the card, so the driver holds no tracker
  credentials (AC3)
- card-lock.ts: exclusive per-card lock (atomic mkdir under working_path).
  A trigger burst is skipped and logged, never queued; release is
  unconditional, so a crash never parks a card
- handler: lock -> audit start -> run -> audit end -> release; a dry run
  writes nothing and locks nothing
- ADR-024 (tag-driven dispatch: adoption mapping, agnostic core, host
  adapter) + ADL on testing OS atomicity against the OS; architecture.md and
  way-of-working.md updated
- Task: T3 — Audit trail (start/end/skip records)

Refs: #217
…erator surfaces

- KB `## Workflows` gains the catalog a maintainer needs to write a mapping:
  which shipped skills are workflows (pair-loop, refine-story, plan-tasks),
  the precedence pair that keeps a just-refined card from HALTing, and why a
  second tag never means a second intensity of the same workflow (D18)
- github-automation.md: the reference trigger adapter — a GH Actions job on
  `issues: [labeled]` calling `pair run --card/--card-tags`, labels passed as
  DATA (never re-fetched), the token scoped to the adapter, and the
  DISPATCH-RECORD line posted by IT, not by the driver. Says plainly that
  `concurrency:` is a host economy and the per-card lock is the guard
- docs site: `run` gains both flags + a Tag-driven dispatch section (the four
  fail-towards-not-running properties, the two HALTs, the audit split);
  adoption-files documents the sixth section; the unattended-delivery tutorial
  gains Option D — push instead of pull, a tag starts the run
- conformance guard extended over all of it, dataset + mirror, including that
  every workflow the catalog names is a skill this repo actually ships
- Task: T4 — KB workflow examples + docs site

Refs: #217
…it caught

- cli.e2e.test.ts: five triggers at ONE real project (routed, unlabelled,
  eligible-but-unmapped, multi-tagged, mapped-but-ineligible), driven through
  the command registry with only the engine spawn injected. Real lock, real
  audit file: each dispatch hands state to the next through the filesystem,
  which is the only thing the module suites (lock + audit injected) cannot show.
  Plus the burst — a second trigger re-entering WHILE the first run holds the
  card — and the no-mapping board
- BUG the e2e found: `--card-tags ""` — what every host adapter renders for an
  UNLABELLED issue — was refused as a malformed flag, so AC2's own case exited 1
  instead of skipping cleanly, and the opt-in boundary was unreachable through
  the entry point. Failing parser test first, then the fix: an empty value is
  the observation "no labels"; a hole INSIDE a list still HALTs. ADL +
  way-of-working record the observation-vs-intent rule; CLI/docs/adapter say it
  where a reader meets the flag
- routing-purity.test.ts: the DoD's grep, mechanised — no tier vocabulary, no
  classification import, no shape inspection and no regex in the routing core;
  every `tags.*` call allowlisted to membership and reporting (D18)
- llms.txt regenerated: ADR-024 and both ADLs were missing from the index
- Task: T5 — End-to-end test on a populated board (tagged/untagged/multi-tag)

Refs: #217
- `driveDispatchedCard`/`record` each took 5 positionals and tripped eslint
  `max-params` (max 4). The four the first took describe ONE routed card, so
  they become a `DispatchedCard` interface; `record`'s trailing `event` +
  optional `outcome` become the one options object every call site already
  reads as a pair
- No behaviour change: same lock → start → drive → end → release order, same
  audit records, same stdout line
- Task: T2/T3 — lint gate green on the dispatcher and its audit wiring

Refs: #217
@rucka rucka added the risk:red Classification: high risk tier label Aug 30, 2026
@rucka rucka self-assigned this Aug 30, 2026
@rucka rucka added the pr-state:to-be-reviewed PR state: awaiting review / gate label Aug 30, 2026
@rucka

rucka commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict

risk:red · cost:orangeCHANGES-REQUESTED — the agnostic core is correct and well tested (117 unit tests green, AC1–AC4 all met in code), but the reference GitHub adapter the story ships as its trigger is broken in three ways: it fails the job on exactly the untagged case AC2 says is silent, it swallows a HALT exit code, and it claims a concurrency guarantee the per-card lock cannot deliver on an ephemeral runner.

Open findings: 8.

PR: [#474] · Author: Gianluca Carucci · Reviewer: independent reviewer · Date: 2026-08-30 · Story: US-217 · Type: feature

Classification matrix — per dimension
Dimension Tier Source Note
Service/domain criticality green Criticality Table apps/pair-cli listed Low; confirmed against the diff
Change/diff risk yellow diff footprint 7 new modules + 6 modified in one package, additive, no contract break; confirmed
Business impact red subdomain class core — Collaborative Workflow (routing is the central capability); confirmed
Security relevance red path heuristic dispatch keyed off externally mutable labels; the opt-in boundary is an authorization control. Confirmed, not raised — the boundary is enforced in a pure, tested function and every untrusted value is content-checked at parse time
Coupling balance not assessed volatility unrated excluded from max (§3.1)

Tier = max(assessed) = risk:red, unchanged. Cost = orange, unchanged. Review value confirms refinement; nothing raised.

Assessments

Security — Input validation

Verdict: green — every value crossing the boundary (--card, each --card-tags entry, each mapping tag and workflow name) is content-checked before it reaches a path segment or an agent prompt.

Details
  • --cardidentifierText at parse time and isSafeId again in card-lock.ts:43 where the path is actually built (defence at the point of use, not only at the edge).
  • Each --card-tags entry → isSafePromptText; an empty entry inside the list HALTs (parser.ts), an empty whole value is data (ADL recorded).
  • ## Workflows tag → assertLabelValue (the same rule ## Eligibility gets, now one implementation in policy-sections.ts rather than two — a real improvement carried by this PR).
  • Workflow name → isSafeId, because it is both spliced into an invocation and used as a path segment.
  • Audit values are newline-flattened (dispatch-audit.ts:110), so no field can forge a second trail line.

Security — Output handling

Verdict: green — the only outputs are a key=value audit line and a stdout DISPATCH-RECORD: line, both single-line by construction and built from validated fields.

Security — Authentication

Verdict: not applicable — the driver deliberately holds no credentials; the token stays in the adapter (ADR-024 §3).

Security — Authorization

Verdict: yellow — the boundary itself is right (untagged ⇒ never, eligibility before routing, no default workflow, all unit-tested), but the documented deployment of it weakens the concurrency half — see Major 3.

Details
  • decideDispatch evaluates mapping → eligibility → routing in that order, verified by dispatch.test.ts:87-101.
  • Stale-label trust is an accepted, recorded trade-off (ADR-024 § Trade-offs) — not a silent one.
  • The lock's cross-host limitation is recorded in the same section, then contradicted by github-automation.md:211-213.

Security — Introduced vulnerabilities

Verdict: green — 0 introduced, 0 pre-existing.

Cost

Verdict: cost:orange — unchanged; per-trigger agentic runs, ceilinged by eligibility + lock + stop predicate.

Architecture (Coupling)

Verdict: not assessed — /pair-capability-assess-coupling not run (volatility unrated).

Bug fix — Red test before fix

Verdict: not applicable — not a bug fix. (The --card-tags "" defect found during development is documented as fixed test-first, and parser.test.ts carries the case.)

Details

Findings by severity

Critical (must fix before merge)

  • none.

Major (must fix before merge)

  • .pair/knowledge/guidelines/collaboration/automation/github-automation.md:251 (+ dataset mirror) — [ -n "$record" ] && gh issue comment … is the last command of the step, so when nothing was dispatched the compound returns 1 and the step exits 1. GitHub's default run shell is bash -e {0}. Concrete case: an untagged issue is labelled → pair run exits 0 with no DISPATCH-RECORD: line → record="" → the job goes red. That is exactly the case the same file's "Untagged is not a case the adapter has to handle" paragraph promises is silent, and AC2's whole point ("nothing runs on that issue"). Every unmapped label edit on the board produces a failed workflow run and a failure notification. Evidence, reproduced: printf 'record=""\n[ -n "$record" ] && echo posted\n' | bash -eexit 1. Fix: if [ -n "$record" ]; then gh issue comment "$CARD" --body "$record"; fi.
  • .pair/knowledge/guidelines/collaboration/automation/github-automation.md:239 (+ dataset mirror) — pair run … | tee dispatch.log runs under bash -e without pipefail (GH only adds -eo pipefail when the step declares shell: bash), so the pipeline's status is tee's. Concrete case: ## Workflows maps auto-dev ⇒ pair-loop and pair-loop is not installed → decideDispatch HALTs, pair run exits 1 → the job reports success. The adoption-fix HALT that the design deliberately chose over a silent fallback never reaches a human, and the team's cards stop being processed with a green tick on every run. Evidence, reproduced: printf 'false | tee /dev/null\n' | bash -eexit 0; same script under bash -eo pipefailexit 1. Fix: add shell: bash to the step (or set -o pipefail as its first line).
  • .pair/knowledge/guidelines/collaboration/automation/github-automation.md:211-213 (+ dataset mirror) — the concurrency: comment states the host group is "NOT the safety guard: the per-card lock inside pair run is what guarantees one run per card, and it holds even when two triggers come from two different workflows, two repositories, or a manual invocation on the box." In this workflow that is false, and it directly contradicts ADR-024's own recorded limitation ("The lock is filesystem-local: two runners on different machines sharing no working area can still collide"). Each ubuntu-latest job does a fresh actions/checkout, so <working_path>/automation/locks/ is always empty — the lock always succeeds. Concrete case: a team reads that sentence and adds a second trigger (workflow_dispatch, or an issue_comment job) outside the pair-dispatch-<issue> group; both jobs acquire the lock on their own runners and two agents drive card 217 on the same branch and PR — the precise race the guard exists for. Fix: state that on ephemeral runners the host concurrency group is the cross-job guard (which is why cancel-in-progress: false matters), and that the per-card lock covers collisions within one working area — the persistent-daemon deployments of the tutorial's Options A–C. Cite ADR-024's limitation instead of contradicting it.

Minor (must fix before merge — same bar as Major, just lower impact)

  • apps/pair-cli/src/commands/run/handler.ts:278-286 — a throw from driveRun releases the lock in finally but writes no end record. The guideline this PR ships states "Every dispatch decision — start, skip, end — is appended to the run's ## Audit Location file". Concrete case: the engine spawn rejects mid-run → the trail holds … event=start card=217 tag=auto-dev workflow=pair-loop and nothing after it; the operator reading the trail the next morning (the module docstring's stated use) cannot distinguish a crashed run from one still in flight, and the lock is gone so the filesystem gives no second signal. handler.test.ts:688 asserts lock events only, so nothing catches it. Fix: catchrecord(… { event: 'end', outcome: 'crashed' }) → rethrow, and extend that test to assert the record.
  • apps/pair-cli/src/commands/run/handler.ts:271join(context.workingArea, 'automation/locks') re-hardcodes the path that card-lock.ts:22 owns as the non-exported LOCK_DIRECTORY. Concrete case: someone relocates the lock directory in card-lock.ts; the burst-skip message keeps naming the old path, so the operator is sent to a directory that does not exist — defeating the exact purpose card-lock.ts:26 states for it ("printed, so a stale lock is findable without guessing"). No test compares the two. Fix: export LOCK_DIRECTORY (or have the acquirer return the intended path) and use it at the call site.
  • apps/pair-cli/src/commands/run/card-lock.ts:40-83 — a stale lock has no recovery path and no operator documentation. finally covers exceptions, not SIGKILL, OOM, or a host job timeout. Concrete case: on a persistent runner (the tutorial's Options A–C — a VPS daemon, the deployment where the lock actually is the guard) the daemon is killed mid-run; <working_path>/automation/locks/217/ survives forever; every later trigger on card 217 prints run-in-progress, exits 0, and appends a skip line — automation is permanently and silently off for that card, with no alert, and github-automation.md's "Before wiring the trigger" checklist never mentions looking. holder.json already records pid and acquiredAt but nothing reads them. Fix: surface the holder's age in the skip detail (the data is already written) and add the one-line manual-clear instruction to the KB's operator checklist.
  • apps/website/content/docs/tutorials/unattended-delivery.mdx:35, 251, 273 — three stale numeric claims left behind by this PR. :35 still says "Six independent sections" while the same PR updates the KB to "seven sections"; :251 introduces ## Workflows as "a sixth section", a number Step 1's own list already spends; :273 calls the shipped GitHub Actions job "20 lines" when the snippet it points at is ~40. Concrete case: a reader who followed Step 1 believes they already have six sections, reaches Option D, and cannot tell which section they are being asked to add. Fix: renumber to seven / "a seventh section", and drop or correct the line count.

Questions (informational, never blocking)

  • apps/pair-cli/src/commands/run/dispatch.ts:72assertWorkflowsInstalled runs before the eligibility and routing checks, so one mapping line naming an uninstalled workflow HALTs every trigger on the whole board, including cards that are ineligible or carry no mapped tag. The comment argues this deliberately ("finding out only on the card that happens to carry that tag would make the failure depend on which trigger fired first"), and I agree with the reasoning — flagging only because combined with Major 2 the blast radius is invisible: every job goes green while nothing is dispatched anywhere. Worth confirming this is the intended blast radius once Major 2 is fixed.
  • apps/pair-cli/src/commands/run/handler.ts:317 — only start is emitted as a DISPATCH-RECORD: line, so only starts can be posted on the card; skip and end reach the audit file only. AC3 requires the run start on the issue, so this meets the AC, and suppressing a comment per skipped label edit is clearly the right call for noise. But T3 in the story's task breakdown reads "Audit trail (start/end/skip comments on issues)". Confirm the narrower reading is deliberate — if so it is worth one sentence in the guideline, since it is the kind of thing a future adapter author will re-litigate.
Positive feedback
  • The purity property is mechanized, not asserted: routing-purity.test.ts bans tier vocabulary, classification imports, shape inspection and regexes in the routing core, and allowlists every method callable on a tags collection. That is the DoD line "grep-verifiable: no classification criteria in workflow code" turned into a build failure, and it is the single best thing in this PR.
  • policy-sections.ts is the right extraction at the right moment — the label rules now have one implementation instead of two, and the diff shows the messages are byte-identical, so the refactor is verifiably behaviour-preserving.
  • The e2e suite drives five triggers against one real project directory through the real command registry with only the engine spawn injected — real lock, real audit file. That is the "populated test board" the story's DoD asks for, not a mock of one.
  • Both ADLs record decisions that would otherwise look like violations (node:fs in a FileSystemService codebase; one flag that accepts empty). ADR-024 records the two rejected options and the four limitations of the chosen one, including the lock's cross-host scope — which is why Major 3 reads as a doc slip rather than a missed risk.
Functionality & requirements (AC coverage)
  • AC1 (mapping routes each card to its workflow) — workflow-mapping.ts + dispatch.ts; dispatch.test.ts:26-47, handler.test.ts:498, e2e :147.
  • AC2 (no mapped tag ⇒ nothing runs; grep-verifiable absence of default-on) — dispatch.ts:92-98, routing-purity.test.ts, dispatch.test.ts:49-86, e2e :147. Met in the core; the shipped adapter turns this case red — Major 1.
  • AC3 (single entry point; run start recorded on the issue) — pair run --card; DISPATCH-RECORD: + adapter post. Met in the core; the posting step is Major 1.
  • AC4 (no mapping section ⇒ "no mapping declared", clean exit) — dispatch.ts:66-68, handler.test.ts:562, e2e :205.
  • BR3 (eligibility before routing) — dispatch.ts:74-89, dispatch.test.ts:87-101.
  • Edge cases: unknown workflow ⇒ HALT; two mapped tags without precedence ⇒ HALT; burst ⇒ skip not queue. All three tested.
Testing & quality gates
  • Coverage: one suite per module + purity guard + e2e; HALT triggers enumerated 1:1 against the guideline's list.
  • Edge/error scenarios tested (including "releases the lock even when the run throws").
  • Quality gates: PASS — I re-ran the new suites at 12846737: routing-purity 7, workflow-mapping 20, parser 56, dispatch 16, dispatch-audit 10, card-lock 8 → 117 passed. (handler.test.ts did not run in my sandbox for an environment reason — an adm-zip resolution failure from the way I linked node_modules into a detached worktree — not a PR defect.)
  • Gap: no test asserts the end record on the throwing path (Minor 1).
Adoption compliance
  • Degradation level: 1 — no deviation from adopted patterns.
  • ADR-024 + two ADLs present and substantive; architecture.md and way-of-working.md updated to match. No new dependency, so tech-stack.md is untouched correctly.
  • KB ↔ dataset mirror verified: README.md and github-automation.md byte-identical; automation-policy.md differs only on four pre-existing /setup-harness lines untouched by this PR — the new ## Workflows section mirrors byte-for-byte.
  • .pair/adoption/tech/automation.md deliberately left without a ## Workflows section, so this repo exercises the absent-section default. Consistent with the ADR's stated Adoption Impact.
Tech debt
  • The lock is filesystem-local and cannot coordinate across machines. Recorded in ADR-024 as an accepted limitation and out of this story's isolation model — surfaced, not blocking. Note that Major 3 is not this: it is the KB claiming the opposite of what the ADR records.
Documentation
  • KB schema, reference adapter, website concepts/reference/tutorial, llms.txt, guidelines catalog — all updated, with conformance tests guarding the normative claims.
  • Three doc defects above (Major 1–3 in the adapter, Minor 4 in the tutorial).
Performance & deployment
  • No hot path touched; one extra section parse per run over a file already read.
  • Rollback is trivial: absent ## Workflows is the shipped default, so the feature is off until a project opts in.

…ecord a crash owes

Review round 1 on PR #474 — 9 findings, all in place.

- Adapter shell: `if [ -n "$record" ]` (a trailing `&&` under `bash -e` reddened
  the job on every untagged label edit) and `shell: bash` (GitHub's implicit
  shell has no pipefail, so a HALT behind `| tee` reported green). Executed, not
  grepped: `github-automation.test.ts` parses the shipped workflow and runs each
  step under the flags GitHub uses, against stub `pair`/`gh`.
- The concurrency comment claimed the per-card lock guards cross-job runs. On
  ephemeral runners it cannot — fresh checkout, fresh working area. The group IS
  the guard there; the lock guards one working area (ADR-024). Corrected in the
  KB, the tutorial, commands.mdx and architecture.md.
- A thrown run wrote no `end`: the trail stopped at `start` and read like a run
  still in flight. Now `outcome=crashed`, then rethrow.
- A held lock reports the holder the acquirer named + how long it has held it;
  `LOCK_DIRECTORY` is exported instead of re-spelled at the call site. Stale
  locks are silent-forever otherwise; clearing them is in the KB pre-flight.
- Confirmed and written down: only `start` is posted on the card, and an
  uninstalled workflow HALTs the whole board, not one tag.
- Tutorial: seven sections / the seventh, line count dropped.

Refs: #217
…w declares

Round 2, 5 findings.

`--root <card>` was rendered at every routed workflow. Two of the three the KB
catalog recommends declare only `$story`, and both pick the highest-priority
Draft story themselves when unscoped: `auto-refine ⇒ pair-process-refine-story`
on card 304 refined a DIFFERENT card while the audit trail and the on-issue
`DISPATCH-RECORD:` both said 304. `SKILL_PARAMETERS` now carries each catalogued
workflow's own spelling, the corpus test pins it against the skills' `##
Arguments` tables, and a mapping naming a workflow the driver cannot scope HALTs
before eligibility and routing.

`skillAcceptsFilter` became `filterDeliveryFor`: a card-scoped workflow neither
takes `--filter` nor reads `## Eligibility`, so the label is dropped instead of
printed as a perimeter nothing applies.

`acquireCardLock` leaked a raw ENOENT when the holder released between the
EEXIST and the stat — the burst it exists for. It retries the exclusive create
once; a path that exists for mkdir and for nothing else is named as a broken
working area.

A crash whose `end` record cannot be written no longer replaces the engine error
with the filesystem one: both in the message, the run error as `cause`.

The e2e board fixture is now what the assertions read — routes, trail, records
and locks all iterate the rows, so an added card is a checked card.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UQJzGMhRqBRRboxMrRqFPP
…e mappable set

Review round 3 — 5 findings, all fixed in place.

- `--root` is refused alongside `--card`: `--card 217 --root 300` drove the agent
  over 300 while the trail, the lock and the on-issue record all named 217.
  The handler reads the dispatched card first too, so a non-parser caller cannot
  reach it either.
- an unwritable audit destination on the START record is reported as a dispatch
  that never began, not as a crashed run whose trail stops at `event=start` —
  both of which were false.
- `DISPATCHABLE_WORKFLOWS` is its own declaration, asserted EQUAL to the KB
  catalog; `pair-next` is scopable by hand and no longer mappable.
- the `## Workflows` schema bullet carries the catalog restriction; ADR-024
  item 1 drops the "no workflow catalog ever lives in code" clause item 7
  contradicts.
- `github-automation.md` loses its duplicated `## TODO` stub tail (dataset +
  regenerated mirror), guarded by execution.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UQJzGMhRqBRRboxMrRqFPP
…e runner must be given the binary

- `pair-process-refine-story` out of the catalog and `DISPATCHABLE_WORKFLOWS`: its own
  SKILL.md makes explicit human alignment a prerequisite, so a dispatch either stalls to the
  timeout holding the card lock, or answers its own gate. Guard reads catalogued skills SKILL.md.
- reference adapter installs the CLI it calls (setup-node + npm i -g); guard runs the shipped
  step on a PATH with no `pair` and asserts 127.
- skip-path audit write wrapped like recordCrash: names the card and that nothing was spawned.
- a `## Workflows` line pasted with its fence reports the wrapper, not a grammar failure.
rucka added 3 commits August 31, 2026 17:47
Anchor re-reviews; prove provisioned artifacts.
…iven-workflows

# Conflicts:
#	.pair/llms.txt
…ven by a real install

- reference GH Actions adapter installed `@foomakers/pair-cli` then invoked `pair run`; the
  package publishes ONE bin, `pair-cli` (ADL 2026-08-25) — the dispatch step was 127 on any
  runner. Every representation moved: yaml `run:`, both yaml comments, the runner-does-not-ship
  paragraph, the `command not found` example, the decision table, the by-hand line, the
  provisioning bullet, and automation-policy's entry-point invocation. Both mirrors identical.
- new smoke scenario github-dispatch-adapter.sh (in CI_TESTS, OFFLINE_SAFE): real
  package-manual.sh + create-registry-tgz.sh, `npm install -g` into a clean prefix, then the
  shipped step run under `env -i` with only that prefix on PATH — nothing stubbed at the
  boundary (ADL 2026-08-31). RED before the fix with `pair: command not found`.
- conformance guard stops naming the binary: `invokedCommand()` reads it from the document,
  every stub is built from that, and a new case pins it to the sole bin key declared by BOTH
  producers of the published manifest. Round 4 passed only because it stubbed `pair`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-state:to-be-reviewed PR state: awaiting review / gate risk:red Classification: high risk tier

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant