Skip to content

fix(workflow): built-in factory matches today's team skills; gate since: run_start - #69

Closed
itaior wants to merge 1 commit into
mainfrom
fix/factory-builtin-matches-skills
Closed

fix(workflow): built-in factory matches today's team skills; gate since: run_start#69
itaior wants to merge 1 commit into
mainfrom
fix/factory-builtin-matches-skills

Conversation

@itaior

@itaior itaior commented Nov 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Why

The built-in factory workflow had drifted from the team skills it drives. As a result, a factory run could pass every step and gate, push nothing, open no MR, and still report success.

Built-in factory (2.4.x – 2.7.0) Team skills today Effect
Publish helpers looked up in .agents/skills/factory-mission/scripts/bash Moved to factory-runbook/scripts/bash run-branch, publish-* and open-mr were skipped while the step still completed (skipped … (factory-mission helpers not installed)): nothing pushed, no MR
Intent steps ran command: factory-product / factory-architect Removed, replaced by factory-intent (ADR-435) The intent steps ran a command that no longer exists

ADR-435 calls out this risk ("stale invocations of removed skills"). This file was missed.

Change

src/workflow/builtins.mjs:

  • One intent step (ADR-435): intent (command: factory-intent, both lanes), then one publish-intent, then gate-product, run-branch-architecture and gate-architecture.
    • The gate IDs are unchanged, and both gates link steps.publish-intent.output.mr_url.
    • publish-architecture is removed. run-branch-architecture stays between the two gates: a run parked at gate-product can resume in a fresh clone on the base branch (restore only writes run state, never the working tree), and gate-architecture diffs the working tree.
  • Helper lookup: HELPER_DIRS = [factory-runbook, factory-mission]. Each helper is found in the first directory that has it.
  • Fail closed: a helper missing from both directories, or a bash that can't start, fails its step, and the reason goes to stderr, so it shows up in the step's error. It's no longer reported as skipped with the step completed.

src/workflow/gate-context.mjs, steps/gate.mjs: a new gate mode, context.since: run_start.

  • It lists records changed since the run started, ignoring later approvals.
  • If the run start is no longer an ancestor of HEAD, it falls back to the merge-base.
  • Why it's needed: with one intent step, the ADRs are written before gate-product is approved, so gate-architecture with since: last_approved would list none of them. gate-architecture now uses run_start; gate-product keeps last_approved.

Tests

  • R3 (workflow-gate-context-3): an ADR written by the intent step before gate-product is still listed at gate-architecture after the product approval. The test fails with last_approved (checked).
  • Helpers:
    • no helpers → the run fails at run-branch, and no agent step runs;
    • open-mr missing → publish-intent fails with open-mr.sh not found;
    • helpers only in factory-runbook, or only in factory-mission → both work.
  • run_start: validation accepts it and still rejects unknown modes; a later approval is ignored; the merge-base fallback works.
  • Fixtures updated for the new step IDs, plus no-op factory-runbook helpers for stub runs (tests/fixtures/factory-helpers.mjs).
  • 843/843 node --test; tests/e2e-node-gate.mjs --skip-agent passes.
  • workflow validate factory against a workspace with today's team skills: valid, 9 steps.
  • The O8 resume test asserts run-branch-architecture ran and HEAD is back on the run branch before gate-architecture.

Compatibility

  • Older team-skills installs (helpers still in factory-mission) keep working through the fallback.
  • Parked runs resume on their own saved copy of workflow.yml, so a run parked on the old layout keeps the old step IDs.
  • A workspace with no factory skills now fails at run-branch instead of completing a run that did nothing. That's intended.

🤖 Generated with Claude Code

@itaior @claude
fix(workflow): built-in factory matches today's team skills; gate sin…
1fabdc1 
…ce: run_start

The built-in `factory` workflow had drifted from the team skills it drives:
- the intent steps ran `factory-product` and `factory-architect`, which
  ADR-435 replaced with `factory-intent`;
- the publish helpers were looked up in factory-mission/scripts/bash, while
  the team skills moved them to factory-runbook. A missing helper was
  "skipped" and the step still completed, so a factory run could push
  nothing, open no MR and still report success.

Now:
- one `intent` step (`command: factory-intent`, both lanes), one
  `publish-intent`, then `gate-product` and `gate-architecture` (ids
  unchanged, both linking the intent MR);
- helpers are found in factory-runbook first, factory-mission second; a
  helper missing from both, or a bash that cannot start, fails its step;
- new gate `context.since: run_start` (changes since the run started,
  ignoring later approvals; merge-base fallback when the run start is not
  an ancestor). gate-architecture uses it, because the ADRs are written
  before gate-product is approved and `last_approved` would hide them.

Tests: R3 now covers that case (an ADR written before gate-product is still
listed at gate-architecture; it fails under `last_approved`); the missing-
helper tests assert the step fails with the reason; factory-runbook vs
factory-mission lookup; three direct `run_start` tests. 843/843 pass; the
node e2e gate passes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kanfil
kanfil reviewed Nov 2, 2026
View reviewed changes

@kanfil kanfil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factory-review verdict: merge-ready (PR #69)

Reviewed revision: head=1fabdc1b772c35bec2a633c4d602c4a76bb04216, base=4e215d61d17720c7e2aa68fd5ac6009485602de6, merge-base=4e215d61d17720c7e2aa68fd5ac6009485602de6 (reviewed from isolated detached checkout).

Scope: 10 files, +219/−91 across src/workflow/builtins.mjs, src/workflow/gate-context.mjs, src/workflow/steps/gate.mjs, and associated test suites/fixtures.

Pass results (REVIEW.md)

Pass Severity Result
bugs (null/undefined, unhandled errors, resource leaks) Important ✅ PASS — fail-closed helper resolution replaces silent skips; run_start gate resolution correctly falls back to merge-base when HEAD has diverged. Full suite (862/862 assertions across 36 suites) passes green.
security (injection, secret exposure, auth bypass) Important ✅ PASS — shell command generation in helperRun safely serializes directory and file arguments with JSON encoding; no secrets exposed.
spec-compliance (PRD/AD coverage) Important ✅ PASS — satisfies ADR-435 (intent lane merge) and ADR-469 (factory-intent cutover in adlc-cli builtin).
quality (naming, dead code, docs) Nit ✅ PASS — Keep-a-Changelog updated, backwards-compatibility fallback to factory-mission preserved for older workspace installs.

Notes on Architectural Coupling

While this PR is technically sound and necessary to unblock live runs, the underlying design of embedding the factory workflow YAML and inline bash runners inside src/workflow/builtins.mjs is the root cause of this drift. Future work should decouple workflow definitions from CLI binaries by having skills manifest workflow.yml into .adlc/workflows/<id>/workflow.yml during skill installation.

Findings: 0 Important, 0 Nit. Verdict: merge-ready — awaiting human code-owner sign-off.

@kanfil kanfil added the validation Passed automated factory-review validation; awaiting human approval label Nov 2, 2026
@itaior itaior assigned itaior and unassigned itaior Nov 3, 2026
@itaior itaior mentioned this pull request Nov 3, 2026
Open
@itaior itaior closed this Nov 3, 2026
itaior added a commit to tikalk/adlc-team-skills that referenced this pull request Nov 3, 2026
@itaior @claude
fix(factory-runbook): address review — layout-independent helpers, ru…
14c18b4 
…n branch between gates

Review findings on c1a6dd4:
- Important 1: shell steps named `.agents/skills/…`, an install path that
  is per agent (.claude/skills, …) and absent in this repo. They now call
  "${ADLC_WORKFLOW_DIR}/scripts/bash/run-helpers.sh" (the engine exports
  the workflow's directory and keeps it on resume), failing with a reason
  when it is unset. Copying the workflow elsewhere must copy the skill dir;
  written down in SKILL.md and the workflow header.
- Important 3: a run parked at gate-product can resume in a fresh clone on
  the base branch, where gate-architecture would diff no ADRs. Restored
  `run-branch-architecture` between the two gates.
- Nit 4: stale "skipped without the helper" comment fixed.
- Nit 5: version floor named: adlc-cli newer than 2.7.0
  (tikalk/adlc-cli#69).
- Nit 6: run-helpers.ps1 has behavioural tests (skip without pwsh); the
  gate-match test documents `since` as a third intended difference.
- Nit 7: SKILL.md states the file runs only by path until shipped
  workflows are installed into .adlc/workflows/<id>/.

Tests: no shell step may name an install path; every shell step's real
run line, run from a copy of the skill under .agents/skills and under
.claude/skills, finds its helpers; without ADLC_WORKFLOW_DIR it fails with
a reason. A hard-coded path fails 4 tests; dropping
run-branch-architecture fails the layout test. Engine run (adlc-cli#69) from
a .claude/skills layout: run-branch → intent → publish-intent →
gate-product, resume → run-branch-architecture → gate-architecture.

Important 2 (the skipped → fail reversal of the run-branch amendment) is
recorded in the decision-record repository, outside this one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kanfil kanfil kanfil left review comments

Assignees

No one assigned

Labels

validation Passed automated factory-review validation; awaiting human approval

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@itaior @kanfil