From b9be8b179f3a32ef37eb9b4bbb5ab430984bb6d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:07:13 +0000 Subject: [PATCH 01/12] ci: remove legacy Droid workflows The legacy Droid Tag and Droid Auto Review workflows ran Factory-AI/droid-action@main, unpinned, on every PR or @droid mention. The opt-in factory-review and factory-mentions workflows replace them; keeping both would give two reviews per PR and two replies per mention once the replacements are enabled. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/droid-review-reusable.yml | 31 --------------------- README.md | 1 - 2 files changed, 32 deletions(-) delete mode 100644 .github/workflows/droid-review-reusable.yml diff --git a/.github/workflows/droid-review-reusable.yml b/.github/workflows/droid-review-reusable.yml deleted file mode 100644 index 8520d78..0000000 --- a/.github/workflows/droid-review-reusable.yml +++ /dev/null @@ -1,31 +0,0 @@ -name: Droid Auto Review (Reusable) - -on: - workflow_call: - secrets: - factory_api_key: - required: true - pull_request: - types: [opened] - -jobs: - droid-review: - runs-on: ubuntu-latest - permissions: - contents: write - pull-requests: write - issues: write - id-token: write - actions: read - steps: - - name: Checkout repository - uses: actions/checkout@v5 - with: - fetch-depth: 1 - - name: Run Droid Auto Review - uses: Factory-AI/droid-action@main - with: - factory_api_key: ${{ secrets.factory_api_key }} - automatic_review: true - automatic_security_review: true - allowed_bots: "*" diff --git a/README.md b/README.md index 89f4f92..896c034 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,6 @@ This repository is public. It holds policy, credential names, and procedures onl | `.github/workflows/factory-review.yml`, `factory-mentions.yml`, `factory-ci-steward.yml` | This repository's copies of the review, mention, and CI Steward workflows that each JHD repository receives. | | `.github/workflows/factory-triage.yml`, `droid-wiki-refresh.yml` | New-issue classification without implementation, and the official default-branch wiki refresh with a rolling reviewable PR. | | `.github/workflows/validate.yml` | Runs the policy lint and tests as the `Automation configuration` workflow. | -| `.github/workflows/droid-review-reusable.yml` | Existing reusable review workflow. It stays in place until its replacement is proven. | ## States From 9b498e4a574c94ac07506c69b15c28a869f6eabd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:12:51 +0000 Subject: [PATCH 02/12] docs: drop the reusable review workflow from the README intro Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 896c034..3abbfc9 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # factory-droid-workflows -Policy, documentation, and prompt templates for Jason Holt Digital (JHD) software-delivery automation across 13 repositories. It also holds the reusable Droid review workflow. +Policy, documentation, and prompt templates for Jason Holt Digital (JHD) software-delivery automation across 13 repositories. This repository is public. It holds policy, credential names, and procedures only. Secret values, private issue text, licensed source content, logs, traces, and recordings stay out of it. From 3c847c9018390199e3744e61487850d6f602107a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:29:18 +0000 Subject: [PATCH 03/12] ci: review each PR once and refresh the wiki monthly Factory Review now runs when a PR opens ready for review or a draft is marked ready, not on every push. A successful review adds the droid-reviewed label, and a labelled PR is skipped; comment `@droid review` to ask for another review. The review checks out the live PR head, so a push between the event and the run is still covered. Droid Wiki Refresh runs at 06:17 UTC on the 1st of each month instead of on every push to main, and can still be started by hand. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/droid-wiki-refresh.yml | 12 +- .github/workflows/factory-review.yml | 61 ++++----- README.md | 2 +- docs/OPERATIONS.md | 6 +- docs/PROVENANCE.md | 6 +- tests/test_workflow_authorization.cjs | 163 +++++++---------------- 6 files changed, 93 insertions(+), 157 deletions(-) diff --git a/.github/workflows/droid-wiki-refresh.yml b/.github/workflows/droid-wiki-refresh.yml index a6dcec6..54d1e72 100644 --- a/.github/workflows/droid-wiki-refresh.yml +++ b/.github/workflows/droid-wiki-refresh.yml @@ -1,8 +1,12 @@ name: Droid Wiki Refresh +# Monthly, not on every push to main: each run is a paid Droid session, and +# a monthly incremental refresh keeps the wiki current enough. Run it by hand +# from the Actions tab when the wiki needs refreshing sooner. on: - push: - branches: [main] + schedule: + - cron: '17 6 1 * *' # 06:17 UTC on the 1st of each month + workflow_dispatch: # The job that runs /wiki is read-only: the agent reads the whole repository # and the pages restored from the unmerged rolling branch, so it must not be @@ -12,8 +16,8 @@ permissions: contents: read # One refresh at a time for the whole repository: the wiki is per repo, so -# the group must not vary by branch or run. Pushes during a run queue only -# the newest commit instead of starting parallel full generations. +# the group must not vary by branch or run. A manual run started during a +# scheduled one waits instead of starting a parallel full generation. concurrency: group: droid-wiki-refresh cancel-in-progress: false diff --git a/.github/workflows/factory-review.yml b/.github/workflows/factory-review.yml index 90045ff..3ca9c8e 100644 --- a/.github/workflows/factory-review.yml +++ b/.github/workflows/factory-review.yml @@ -1,33 +1,39 @@ # Based on Factory-AI/droid-action v10 native automatic review and security modes. +# +# One automatic review per PR: when it opens ready for review, or when a draft +# becomes ready. Later pushes do not start another review; the droid-reviewed +# label marks a PR as reviewed. To review again, comment `@droid review` +# (factory-mentions.yml), or remove the label and mark the PR ready again. name: Factory Review on: pull_request: - types: [opened, synchronize, reopened, ready_for_review, edited, converted_to_draft, closed] + # converted_to_draft and closed only cancel a review still in progress. + types: [opened, reopened, ready_for_review, converted_to_draft, closed] permissions: {} concurrency: - group: factory-review-${{ github.event.action == 'edited' && !github.event.changes.base && format('edit-{0}', github.run_id) || github.event.pull_request.number }} + group: factory-review-${{ github.event.pull_request.number }} cancel-in-progress: true jobs: authorize: if: >- vars.JHD_FACTORY_REVIEW_ENABLED == 'true' && - (github.event.action != 'edited' || github.event.changes.base) && + !contains(github.event.pull_request.labels.*.name, 'droid-reviewed') && github.event.pull_request.draft == false && github.event.pull_request.head.repo.full_name == github.repository && (github.event.sender.type == 'User' || github.event.sender.login == 'factory-droid[bot]') runs-on: ubuntu-latest permissions: - actions: read pull-requests: read outputs: eligible: ${{ steps.check.outputs.eligible }} + head_sha: ${{ steps.check.outputs.head_sha }} steps: - - name: Check current head and completed reviews + - name: Check the PR and whether it was already reviewed id: check uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 env: @@ -36,18 +42,17 @@ jobs: script: | const { owner, repo } = context.repo; const expected = context.payload.pull_request; - if (context.payload.action === 'edited' && !context.payload.changes?.base) { - core.info('Skipping an edit that did not change the PR base.'); - return; - } const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: expected.number, }); if (pr.state !== 'open' || pr.draft || pr.head.repo?.full_name !== `${owner}/${repo}` || - pr.head.sha !== expected.head.sha || pr.base.ref !== expected.base.ref) { - core.info('Skipping a fork, draft, closed PR, or superseded head/base.'); + core.info('Skipping a fork, draft, closed PR, or superseded base.'); + return; + } + if (pr.labels?.some(label => label.name === 'droid-reviewed')) { + core.info('This PR already has its Factory review. Comment `@droid review` to review it again.'); return; } const actors = new Set([context.actor, process.env.TRIGGERING_ACTOR]); @@ -65,27 +70,7 @@ jobs: return; } } - const { data } = await github.rest.actions.listWorkflowRuns({ - owner, repo, workflow_id: 'factory-review.yml', - event: 'pull_request', head_sha: pr.head.sha, - status: 'success', per_page: 100, - }); - for (const run of data.workflow_runs) { - if (run.id === context.runId) continue; - if (!run.pull_requests?.some(candidate => - candidate.number === pr.number && - candidate.base?.ref === pr.base.ref && - candidate.base?.sha === pr.base.sha)) continue; - const jobs = await github.paginate( - github.rest.actions.listJobsForWorkflowRun, - { owner, repo, run_id: run.id, per_page: 100 }, - ); - if (jobs.some(job => job.name === 'Advisory review and security' && - job.conclusion === 'success')) { - core.info('This PR head and base already completed Factory review.'); - return; - } - } + core.setOutput('head_sha', pr.head.sha); core.setOutput('eligible', 'true'); review: @@ -114,7 +99,7 @@ jobs: - name: Check out the reviewed head without saved credentials uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 with: - ref: ${{ github.event.pull_request.head.sha }} + ref: ${{ needs.authorize.outputs.head_sha }} fetch-depth: 1 persist-credentials: false @@ -129,3 +114,13 @@ jobs: security_block_on_high: "false" allowed_bots: factory-droid show_full_output: "false" + + - name: Mark the PR as reviewed + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 + with: + script: | + await github.rest.issues.addLabels({ + owner: context.repo.owner, repo: context.repo.repo, + issue_number: context.payload.pull_request.number, + labels: ['droid-reviewed'], + }); diff --git a/README.md b/README.md index 3abbfc9..8c7429f 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ Checks that were not run are reported as `NOT_RUN` with a reason. Ticket-to-PR and recurring source Intake are **deferred**, not scheduled implementation work. Their retained registrations stay paused and cannot perform their former tasks. -Review, PR commands, Triage and CI Steward are opt-in. The official wiki workflow starts on default-branch pushes as soon as its setup PR merges; merging it is an activation decision. +Review, PR commands, Triage and CI Steward are opt-in. The official wiki workflow runs monthly (06:17 UTC on the 1st) once its setup PR merges, and can be started by hand from the Actions tab. Live status is in the issue titled `JHD automation status` in the private `Jason-Holt-Digital/jhd-skills` repository. diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index 9eec3ed..3998b34 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -2,14 +2,14 @@ ## Agreed operating model -Code Review, AutoWiki, issue Triage, and bounded CI repair run on events, not polling schedules. Weekly Health and Digest and monthly Readiness run as local native Factory automations on the Mac mini. Ticket-to-PR and recurring source Intake are deferred. +Code Review, issue Triage, and bounded CI repair run on events, not polling schedules. AutoWiki runs monthly as a GitHub Actions schedule, or by hand from the Actions tab. Weekly Health and Digest and monthly Readiness run as local native Factory automations on the Mac mini. Ticket-to-PR and recurring source Intake are deferred. These frequencies are JHD choices for a low-volume organization, not Factory Readiness requirements. | Automation | Starts when | Required outcome | |---|---|---| -| Code Review | New eligible PR head | One current-head review with a security pass; no duplicate review for the same PR/head/base identity. | -| AutoWiki | Push to the default branch | Refresh the Factory wiki and open or update one reviewable `factory/droid-wiki` PR containing generated pages and metadata. | +| Code Review | Eligible PR opened ready for review, or a draft marked ready | One review with a security pass per PR. The `droid-reviewed` label marks it done; later pushes do not start another review. Comment `@droid review` for another one. | +| AutoWiki | Monthly, 06:17 UTC on the 1st, or a manual run | Refresh the Factory wiki and open or update one reviewable `factory/droid-wiki` PR containing generated pages and metadata. | | Issue Triage | New GitHub issue | One comment classifying it, suggesting related work and a human route. No implementation or ready label. | | CI Steward | An allowlisted deterministic CI workflow fails | Diagnose it; at most one retry and two fix commits, with ten invocations per PR. | | Health | Weekly | At most one justified maintenance PR across the organization. No work if no useful fix exists. | diff --git a/docs/PROVENANCE.md b/docs/PROVENANCE.md index 0141d1f..3a30177 100644 --- a/docs/PROVENANCE.md +++ b/docs/PROVENANCE.md @@ -14,10 +14,10 @@ The review, mention, Steward and Triage wrappers reference commits, with version ## Corrections to the official templates -- Add `synchronize` to the pull request triggers, so each new head is reviewed. +- Review once per PR: trigger on `opened`, `reopened` and `ready_for_review`, not `synchronize`, and add a `droid-reviewed` label after a successful review. `@droid review` asks for another review. - List trusted bots by name. Never use a wildcard. -Review, mentions, Triage and CI Steward are opt-in through `JHD_FACTORY_REVIEW_ENABLED`, `JHD_FACTORY_MENTIONS_ENABLED`, `JHD_FACTORY_TRIAGE_ENABLED`, `JHD_FACTORY_STEWARD_ENABLED`, and `JHD_FACTORY_STEWARD_FIX_ENABLED`. AutoWiki has the official default-branch push trigger; merging its setup PR is activation, not a gated dry run. +Review, mentions, Triage and CI Steward are opt-in through `JHD_FACTORY_REVIEW_ENABLED`, `JHD_FACTORY_MENTIONS_ENABLED`, `JHD_FACTORY_TRIAGE_ENABLED`, `JHD_FACTORY_STEWARD_ENABLED`, and `JHD_FACTORY_STEWARD_FIX_ENABLED`. AutoWiki replaces the official default-branch push trigger with a monthly schedule and a manual run; merging its setup PR is activation, not a gated dry run. ## Official action @@ -29,7 +29,7 @@ The workflows use the official upstream `Factory-AI/droid-action` v10 at the pin - Issue Triage: 13 classification-only workflows using the official pinned `Factory-AI/droid-action/base-action`, with restricted local tools and a separate validated comment job. This is JHD configuration, not the scheduled Slack Triage package. - AutoWiki: twelve official-template workflows added to existing setup PRs; Mythic's existing PR #1921 already has the current template and is reused unchanged. All restore the rolling wiki, preserve hidden metadata, exclude media, and separate read-only generation from write-permission publication. - CI Steward: six repositories with existing deterministic CI, watching only those workflows. The list is in `docs/OPERATIONS.md`. -- Review rejects stale PR head SHAs and changed base branch refs. A base branch that advances to a new commit does not suppress review. Completed-review deduplication uses the live PR head SHA, base ref, and base SHA. The workflow cancels superseded runs and skips a matching completed review. +- Review checks out the live PR head and rejects changed base branch refs. A PR with the `droid-reviewed` label is not reviewed again. Converting the PR to a draft or closing it cancels a review in progress. - Opt-in jobs on the setup PRs are skipped. Hosted Factory execution remains unverified. ## Added repositories diff --git a/tests/test_workflow_authorization.cjs b/tests/test_workflow_authorization.cjs index 476bc5c..63545df 100644 --- a/tests/test_workflow_authorization.cjs +++ b/tests/test_workflow_authorization.cjs @@ -30,7 +30,7 @@ function eventContext(kind) { const payload = { repository: { default_branch: 'main' }, sender: { login: 'writer', type: 'User' }, - action: kind === 'review' ? 'synchronize' : 'created', + action: kind === 'review' ? 'opened' : 'created', pull_request: pr, }; if (kind === 'mentions') { @@ -60,7 +60,7 @@ function eventContext(kind) { async function authorize(kind, options = {}) { const context = options.context || eventContext(kind); const pr = options.pr || pullRequest(); - const result = { outputs: {}, failures: [], infos: [], permissions: [], jobs: [], pulls: [] }; + const result = { outputs: {}, failures: [], infos: [], permissions: [], pulls: [] }; const github = { rest: { repos: { @@ -83,21 +83,6 @@ async function authorize(kind, options = {}) { return { data: pr }; }, }, - actions: { - async listWorkflowRuns(params) { - assert.equal(params.workflow_id, 'factory-review.yml'); - assert.equal(params.head_sha, pr.head.sha); - assert.equal(params.event, 'pull_request'); - assert.equal(params.status, 'success'); - return { data: { workflow_runs: options.runs || [] } }; - }, - listJobsForWorkflowRun() { throw new Error('Use github.paginate for jobs'); }, - }, - }, - async paginate(method, { run_id }) { - assert.equal(method, github.rest.actions.listJobsForWorkflowRun); - result.jobs.push(run_id); - return options.jobs?.[run_id] || []; }, }; const core = { @@ -121,7 +106,10 @@ function evaluate(expression, context, vars = {}, scope = {}) { changes: {}, comment: { user: {} }, review: { user: {} }, ...context.payload, }; - return vm.runInNewContext(expression.replaceAll('pull_requests[0].number', 'pull_requests?.[0]?.number'), { + const script = expression + .replaceAll('pull_requests[0].number', 'pull_requests?.[0]?.number') + .replaceAll('labels.*.name', 'labels?.map(label => label.name)'); + return vm.runInNewContext(script, { github: { event, actor: context.actor, event_name: context.eventName, repository: 'jhd/project', run_id: context.runId, @@ -167,16 +155,15 @@ for (const kind of ['mentions', 'review', 'ci-steward']) { } } - for (const state of kind === 'review' ? ['fork', 'closed', 'draft', 'stale'] : []) { + for (const state of kind === 'review' ? ['fork', 'closed', 'draft'] : []) { test(`${kind}: rejects ${state} PR`, async () => { const pr = pullRequest(); if (state === 'fork') pr.head.repo.full_name = 'outside/project'; if (state === 'closed') pr.state = 'closed'; if (state === 'draft') pr.draft = true; - if (state === 'stale') pr.head.sha = 'newer-head'; const result = await authorize(kind, { pr }); denied(result); - assert.match(result.infos[0], /fork|superseded/); + assert.match(result.infos[0], /fork|draft|closed/); }); } @@ -293,60 +280,34 @@ for (const body of ['@droid', '@droid review', '@droid fill', '@droid security', }); } -function reviewRun(id = 99) { +test('review: reviews the live head when the PR moved since the event', async () => { const pr = pullRequest(); - return { id, head_sha: pr.head.sha, pull_requests: [{ number: pr.number, base: pr.base }] }; -} - -for (const mismatch of ['other PR', 'base ref', 'base SHA', 'no associations', 'missing associations', 'missing base']) { - test(`review: previous success with ${mismatch} does not suppress review`, async () => { - const run = reviewRun(); - if (mismatch === 'other PR') run.pull_requests[0].number = 41; - if (mismatch === 'base ref') run.pull_requests[0].base.ref = 'release'; - if (mismatch === 'base SHA') run.pull_requests[0].base.sha = 'old-base'; - if (mismatch === 'no associations') run.pull_requests = []; - if (mismatch === 'missing associations') delete run.pull_requests; - if (mismatch === 'missing base') delete run.pull_requests[0].base; - const result = await authorize('review', { - runs: [run], jobs: { 99: [{ name: 'Advisory review and security', conclusion: 'success' }] }, - }); - assert.equal(result.outputs.eligible, 'true'); - assert.deepEqual(result.jobs, []); - }); -} + pr.head.sha = 'newer-head'; + const result = await authorize('review', { pr }); + assert.equal(result.outputs.eligible, 'true'); + assert.equal(result.outputs.head_sha, 'newer-head'); +}); -test('review: same PR/head/base successful review deduplicates', async () => { - const result = await authorize('review', { - runs: [reviewRun()], - jobs: { 99: [{ name: 'Advisory review and security', conclusion: 'success' }] }, - }); +test('review: a PR labelled droid-reviewed is not reviewed again', async () => { + const context = eventContext('review'); + context.payload.pull_request.labels = [{ name: 'droid-reviewed' }]; + assert.equal(jobCondition('review', context), false); + const pr = pullRequest(); + pr.labels = [{ name: 'droid-reviewed' }]; + const result = await authorize('review', { pr }); denied(result); assert.deepEqual(result.failures, []); - assert.deepEqual(result.jobs, [99]); - assert.match(result.infos[0], /already completed/); + assert.match(result.infos[0], /already has its Factory review/); + assert.match(result.infos[0], /@droid review/); }); -for (const jobs of [ - [], [{ name: 'authorize', conclusion: 'success' }], - [{ name: 'Advisory review and security', conclusion: 'skipped' }], - [{ name: 'Advisory review and security', conclusion: 'failure' }], -]) { - test(`review: no completed successful review job (${JSON.stringify(jobs)}) remains eligible`, async () => { - const result = await authorize('review', { runs: [reviewRun()], jobs: { 99: jobs } }); - assert.equal(result.outputs.eligible, 'true'); - assert.deepEqual(result.jobs, [99]); - }); -} - -test('review: skips current run and continues past unrelated candidates', async () => { - const unrelated = reviewRun(98); - unrelated.pull_requests[0].number = 41; - const result = await authorize('review', { - runs: [reviewRun(100), unrelated, reviewRun()], - jobs: { 99: [{ name: 'Advisory review and security', conclusion: 'success' }] }, - }); - denied(result); - assert.deepEqual(result.jobs, [99]); +test('review: other labels do not block the first review', async () => { + const context = eventContext('review'); + context.payload.pull_request.labels = [{ name: 'enhancement' }]; + assert.equal(jobCondition('review', context), true); + const pr = pullRequest(); + pr.labels = [{ name: 'enhancement' }]; + assert.equal((await authorize('review', { pr })).outputs.eligible, 'true'); }); test('review: rejects a superseded base ref', async () => { @@ -355,59 +316,35 @@ test('review: rejects a superseded base ref', async () => { denied(await authorize('review', { pr })); }); -test('review: base SHA drift remains eligible and deduplicates against the live base', async () => { - const pr = pullRequest(); - pr.base.sha = 'new-base'; - const oldRun = reviewRun(); - const liveRun = reviewRun(98); - liveRun.pull_requests[0].base.sha = pr.base.sha; - const jobs = { - 99: [{ name: 'Advisory review and security', conclusion: 'success' }], - 98: [{ name: 'Advisory review and security', conclusion: 'success' }], - }; - const eligible = await authorize('review', { pr, runs: [oldRun], jobs }); - assert.equal(eligible.outputs.eligible, 'true'); - assert.deepEqual(eligible.jobs, []); - const completed = await authorize('review', { pr, runs: [oldRun, liveRun], jobs }); - denied(completed); - assert.deepEqual(completed.jobs, [98]); - assert.match(completed.infos[0], /already completed/); +test('review: pushes and edits do not start a review', () => { + const types = workflows.review.match(/^ types: \[(.*)\]$/m); + assert.ok(types); + const actions = types[1].split(',').map(action => action.trim()); + assert.deepEqual(actions, ['opened', 'reopened', 'ready_for_review', 'converted_to_draft', 'closed']); }); -function reviewGroup(context) { +test('review: lifecycle events share one cancellation group per PR', () => { const match = workflows.review.match(/^ group: (.*)\n/m); assert.ok(match); - return match[1].replace(/\$\{\{(.*?)\}\}/g, (_, expression) => evaluate(expression, context)); -} - -test('review: title/body edits neither authorize nor cancel an active review', async () => { - const current = eventContext('review'); - for (const changes of [{ title: { from: 'old' } }, { body: { from: 'old' } }]) { - const edit = eventContext('review'); - edit.payload.action = 'edited'; - edit.payload.changes = changes; - edit.runId = 101; - assert.equal(jobCondition('review', edit), false); - denied(await authorize('review', { context: edit })); - assert.notEqual(reviewGroup(edit), reviewGroup(current)); - assert.notEqual(reviewGroup(edit), reviewGroup({ ...edit, runId: 102 })); - } -}); - -test('review: base retargets authorize and share head/lifecycle cancellation group', async () => { + const group = context => match[1].replace(/\$\{\{(.*?)\}\}/g, (_, expression) => evaluate(expression, context)); const context = eventContext('review'); - const group = reviewGroup(context); - context.payload.action = 'edited'; - context.payload.changes = { base: { ref: { from: 'release' }, sha: { from: 'old-base' } } }; - assert.equal(jobCondition('review', context), true); - assert.equal((await authorize('review', { context })).outputs.eligible, 'true'); - assert.equal(reviewGroup(context), group); - for (const action of ['opened', 'synchronize', 'reopened', 'ready_for_review', 'converted_to_draft', 'closed']) { + const first = group(context); + for (const action of ['reopened', 'ready_for_review', 'converted_to_draft', 'closed']) { context.payload.action = action; - assert.equal(reviewGroup(context), group); + context.runId += 1; + assert.equal(group(context), first); } }); +test('review: labels the PR only after the review step', () => { + const review = jobBlock('review', 'review'); + const reviewStep = review.indexOf('Run native deep review and security'); + const labelStep = review.indexOf('Mark the PR as reviewed'); + assert.ok(reviewStep > 0 && labelStep > reviewStep); + assert.match(review.slice(labelStep), /labels: \['droid-reviewed'\]/); + assert.match(review, /ref: \$\{\{ needs\.authorize\.outputs\.head_sha \}\}/); +}); + for (const kind of ['review', 'ci-steward']) { test(`${kind}: preserves Factory bot trust for fix pushes`, async () => { const context = eventContext(kind); From f1f9a1337a318b3b3791324e96b93cb67c153f6c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:35:46 +0000 Subject: [PATCH 04/12] ci: create the droid-reviewed label and keep wiki runs on main Adding a label that does not exist yet can fail, so the review job now creates droid-reviewed when the repository lacks it. A manual wiki run started from another branch would document unmerged code, so generation runs only on the default branch. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/droid-wiki-refresh.yml | 2 + .github/workflows/factory-review.yml | 16 ++++++-- tests/test_workflow_authorization.cjs | 49 +++++++++++++++++++++++- 3 files changed, 63 insertions(+), 4 deletions(-) diff --git a/.github/workflows/droid-wiki-refresh.yml b/.github/workflows/droid-wiki-refresh.yml index 54d1e72..db56470 100644 --- a/.github/workflows/droid-wiki-refresh.yml +++ b/.github/workflows/droid-wiki-refresh.yml @@ -29,6 +29,8 @@ env: jobs: wiki-refresh: + # A manual run from another branch would document unmerged code. + if: github.ref_name == github.event.repository.default_branch runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/factory-review.yml b/.github/workflows/factory-review.yml index 3ca9c8e..04e818c 100644 --- a/.github/workflows/factory-review.yml +++ b/.github/workflows/factory-review.yml @@ -119,8 +119,18 @@ jobs: uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 with: script: | + const { owner, repo } = context.repo; + const name = 'droid-reviewed'; + try { + await github.rest.issues.getLabel({ owner, repo, name }); + } catch (error) { + if (error.status !== 404) throw error; + await github.rest.issues.createLabel({ + owner, repo, name, color: '5319e7', + description: 'Factory review done; comment @droid review for another', + }); + } await github.rest.issues.addLabels({ - owner: context.repo.owner, repo: context.repo.repo, - issue_number: context.payload.pull_request.number, - labels: ['droid-reviewed'], + owner, repo, issue_number: context.payload.pull_request.number, + labels: [name], }); diff --git a/tests/test_workflow_authorization.cjs b/tests/test_workflow_authorization.cjs index 63545df..ea40f02 100644 --- a/tests/test_workflow_authorization.cjs +++ b/tests/test_workflow_authorization.cjs @@ -341,7 +341,7 @@ test('review: labels the PR only after the review step', () => { const reviewStep = review.indexOf('Run native deep review and security'); const labelStep = review.indexOf('Mark the PR as reviewed'); assert.ok(reviewStep > 0 && labelStep > reviewStep); - assert.match(review.slice(labelStep), /labels: \['droid-reviewed'\]/); + assert.match(review.slice(labelStep), /labels: \[name\]/); assert.match(review, /ref: \$\{\{ needs\.authorize\.outputs\.head_sha \}\}/); }); @@ -492,3 +492,50 @@ for (const [kind, job] of [['mentions', 'mentions'], ['ci-steward', 'steward']]) } }); } + +async function runLabelStep(labelExists) { + const review = jobBlock('review', 'review'); + const step = review.slice(review.indexOf('Mark the PR as reviewed')); + const match = step.match(/ script: \|\n((?:^ .*\n|^\n)+)/m); + assert.ok(match, 'Missing label step script'); + const calls = []; + const github = { + rest: { + issues: { + async getLabel(params) { + calls.push(['get', params.name]); + if (!labelExists) throw Object.assign(new Error('Not Found'), { status: 404 }); + return { data: {} }; + }, + async createLabel(params) { calls.push(['create', params.name]); }, + async addLabels(params) { calls.push(['add', params.issue_number, params.labels]); }, + }, + }, + }; + const context = eventContext('review'); + await vm.runInNewContext(`(async () => {\n${match[1].replace(/^ /gm, '')}\n})()`, { github, context }); + return JSON.parse(JSON.stringify(calls)); +} + +test('review: creates the droid-reviewed label when the repository lacks it', async () => { + assert.deepEqual(await runLabelStep(false), [ + ['get', 'droid-reviewed'], ['create', 'droid-reviewed'], ['add', 42, ['droid-reviewed']], + ]); + assert.deepEqual(await runLabelStep(true), [ + ['get', 'droid-reviewed'], ['add', 42, ['droid-reviewed']], + ]); +}); + +test('wiki: generation runs only on the default branch', () => { + const wiki = readFileSync(join(__dirname, '../.github/workflows/droid-wiki-refresh.yml'), 'utf8'); + const job = wiki.split('\n wiki-refresh:\n')[1]; + assert.ok(job); + const condition = job.match(/^ if: (.*)$/m); + assert.ok(condition); + for (const [ref, expected] of [['main', true], ['feature', false]]) { + const value = vm.runInNewContext(condition[1].replaceAll('github.', 'github_.'), { + github_: { ref_name: ref, event: { repository: { default_branch: 'main' } } }, + }); + assert.equal(value, expected); + } +}); From 013c0043f8ebe1c6064d66144bc1b2d138cff791 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:37:35 +0000 Subject: [PATCH 05/12] ci: restart an unlabelled review on push and label only the reviewed head Without the synchronize trigger, a push during the first review left the new commits labelled as reviewed but unseen. Pushes now cancel and restart the review until the droid-reviewed label exists, and the label step skips when the head moved. The base-ref check is dropped so a retarget before the run still gets reviewed. The operations doc now describes the monthly wiki schedule, manual runs and GitHub's 60-day inactivity pause. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/factory-review.yml | 25 ++++++++++++++------- docs/OPERATIONS.md | 8 +++---- docs/PROVENANCE.md | 4 ++-- tests/test_workflow_authorization.cjs | 31 +++++++++++++++++++-------- 4 files changed, 45 insertions(+), 23 deletions(-) diff --git a/.github/workflows/factory-review.yml b/.github/workflows/factory-review.yml index 04e818c..edd3561 100644 --- a/.github/workflows/factory-review.yml +++ b/.github/workflows/factory-review.yml @@ -1,15 +1,16 @@ # Based on Factory-AI/droid-action v10 native automatic review and security modes. # -# One automatic review per PR: when it opens ready for review, or when a draft -# becomes ready. Later pushes do not start another review; the droid-reviewed -# label marks a PR as reviewed. To review again, comment `@droid review` -# (factory-mentions.yml), or remove the label and mark the PR ready again. +# One automatic review per PR. A successful review adds the droid-reviewed +# label, and every later event on a labelled PR is skipped. Until then a push +# cancels the review in progress and starts it again on the new head, so the +# reviewed code is the code that gets the label. To review again, comment +# `@droid review` (factory-mentions.yml), or remove the label. name: Factory Review on: pull_request: # converted_to_draft and closed only cancel a review still in progress. - types: [opened, reopened, ready_for_review, converted_to_draft, closed] + types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] permissions: {} @@ -46,9 +47,8 @@ jobs: owner, repo, pull_number: expected.number, }); if (pr.state !== 'open' || pr.draft || - pr.head.repo?.full_name !== `${owner}/${repo}` || - pr.base.ref !== expected.base.ref) { - core.info('Skipping a fork, draft, closed PR, or superseded base.'); + pr.head.repo?.full_name !== `${owner}/${repo}`) { + core.info('Skipping a fork, draft, or closed PR.'); return; } if (pr.labels?.some(label => label.name === 'droid-reviewed')) { @@ -117,10 +117,19 @@ jobs: - name: Mark the PR as reviewed uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 + env: + REVIEWED_SHA: ${{ needs.authorize.outputs.head_sha }} with: script: | const { owner, repo } = context.repo; const name = 'droid-reviewed'; + const { data: pr } = await github.rest.pulls.get({ + owner, repo, pull_number: context.payload.pull_request.number, + }); + if (pr.head.sha !== process.env.REVIEWED_SHA) { + core.info('The PR moved during the review; the next run reviews the new head.'); + return; + } try { await github.rest.issues.getLabel({ owner, repo, name }); } catch (error) { diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index 3998b34..df98ada 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -8,7 +8,7 @@ These frequencies are JHD choices for a low-volume organization, not Factory Rea | Automation | Starts when | Required outcome | |---|---|---| -| Code Review | Eligible PR opened ready for review, or a draft marked ready | One review with a security pass per PR. The `droid-reviewed` label marks it done; later pushes do not start another review. Comment `@droid review` for another one. | +| Code Review | Eligible PR, until its first review succeeds | One review with a security pass per PR. A push during the review restarts it on the new head. The `droid-reviewed` label marks it done, and labelled PRs are skipped. Comment `@droid review` for another one. | | AutoWiki | Monthly, 06:17 UTC on the 1st, or a manual run | Refresh the Factory wiki and open or update one reviewable `factory/droid-wiki` PR containing generated pages and metadata. | | Issue Triage | New GitHub issue | One comment classifying it, suggesting related work and a human route. No implementation or ready label. | | CI Steward | An allowlisted deterministic CI workflow fails | Diagnose it; at most one retry and two fix commits, with ten invocations per PR. | @@ -60,7 +60,7 @@ Use the current official `/install-wiki` template, including fixed repository-wi Mythic's existing setup is PR #1921. It already contains these settings and is reused unchanged. The other twelve repositories receive the same template in their existing setup PRs. Do not open a duplicate Mythic wiki workflow. -**Merging a wiki setup workflow activates its default-branch push trigger.** It has no JHD enable variable. Wait for restored Factory quota and approval of that activation before merging those PRs. This preparation does not merge any PR. +**Merging a wiki setup workflow activates its monthly schedule** (06:17 UTC on the 1st). It has no JHD enable variable. For an immediate refresh, run it by hand from the Actions tab on the default branch; a run started from any other branch generates nothing. GitHub disables scheduled workflows in a public repository after 60 days without repository activity; re-enable the workflow from the Actions tab when that happens. `FACTORY_API_KEY` already exists at organization scope for all repositories. No `FACTORY_WIKI_PR_TOKEN` was present in organization or repository secret metadata at preparation time. Do not reuse or widen `RELEASE_PLEASE_TOKEN`. @@ -106,7 +106,7 @@ The other seven receive no CI Steward workflow. Steward does not watch AI, relea | Issue Triage | `JHD_FACTORY_TRIAGE_ENABLED` unset or `false` | `true`, one pilot first | | CI Steward diagnosis | `JHD_FACTORY_STEWARD_ENABLED` unset or `false` | `true` | | CI Steward fixes | `JHD_FACTORY_STEWARD_FIX_ENABLED` unset or `false` | `true` after repair proof | -| AutoWiki | Setup PR unmerged, or workflow disabled | Merge explicitly activates default-branch pushes | +| AutoWiki | Setup PR unmerged, or workflow disabled | Merge activates the monthly schedule and manual runs | Review, mentions and Steward use the unchanged official `Factory-AI/droid-action` v10 at the pinned commit in `docs/PROVENANCE.md`. Factory App access and credential availability are required but do not prove hosted execution. Keep all repository `automerge` values false. @@ -120,7 +120,7 @@ Verify available Factory quota before a paid execution. Do not activate on a gue 2. Confirm the affected repository can actually start GitHub Actions jobs after the CI budget change. A zero-step billing failure proves nothing about Factory or Sentry credentials. 3. Canary Code Review on one same-repository PR/head. Confirm a real posted review, then cut over that repository's legacy responder. 4. Canary one issue classification. Expect one bounded comment, no implementation or label changes, and no second comment on rerun. -5. Approve AutoWiki activation and its publishing credential/permission path. Merge one wiki setup and observe one rolling wiki PR, then an incremental update with preserved metadata. +5. Approve AutoWiki activation and its publishing credential/permission path. Merge one wiki setup, start one manual run on the default branch, and observe one rolling wiki PR, then an incremental update with preserved metadata. 6. Canary CI Steward with one eligible nonproduction failure. Verify diagnosis before separately enabling bounded repairs. 7. Canary Digest against the known private status issue, Health against one named repository, and Readiness against one named repository. Keep their schedules paused during each manual canary. 8. Resume only the three recurring automations whose outcomes passed and whose activation is approved. Leave Ticket-to-PR and Intake deferred. diff --git a/docs/PROVENANCE.md b/docs/PROVENANCE.md index 3a30177..161cdd9 100644 --- a/docs/PROVENANCE.md +++ b/docs/PROVENANCE.md @@ -14,7 +14,7 @@ The review, mention, Steward and Triage wrappers reference commits, with version ## Corrections to the official templates -- Review once per PR: trigger on `opened`, `reopened` and `ready_for_review`, not `synchronize`, and add a `droid-reviewed` label after a successful review. `@droid review` asks for another review. +- Review once per PR: a successful review adds the `droid-reviewed` label, and labelled PRs are skipped. Until then a push restarts the review on the new head. `@droid review` asks for another review. - List trusted bots by name. Never use a wildcard. Review, mentions, Triage and CI Steward are opt-in through `JHD_FACTORY_REVIEW_ENABLED`, `JHD_FACTORY_MENTIONS_ENABLED`, `JHD_FACTORY_TRIAGE_ENABLED`, `JHD_FACTORY_STEWARD_ENABLED`, and `JHD_FACTORY_STEWARD_FIX_ENABLED`. AutoWiki replaces the official default-branch push trigger with a monthly schedule and a manual run; merging its setup PR is activation, not a gated dry run. @@ -29,7 +29,7 @@ The workflows use the official upstream `Factory-AI/droid-action` v10 at the pin - Issue Triage: 13 classification-only workflows using the official pinned `Factory-AI/droid-action/base-action`, with restricted local tools and a separate validated comment job. This is JHD configuration, not the scheduled Slack Triage package. - AutoWiki: twelve official-template workflows added to existing setup PRs; Mythic's existing PR #1921 already has the current template and is reused unchanged. All restore the rolling wiki, preserve hidden metadata, exclude media, and separate read-only generation from write-permission publication. - CI Steward: six repositories with existing deterministic CI, watching only those workflows. The list is in `docs/OPERATIONS.md`. -- Review checks out the live PR head and rejects changed base branch refs. A PR with the `droid-reviewed` label is not reviewed again. Converting the PR to a draft or closing it cancels a review in progress. +- Review checks out the live PR head and labels the PR only if that head has not moved. A PR with the `droid-reviewed` label is not reviewed again. A push, converting the PR to a draft, or closing it cancels a review in progress. - Opt-in jobs on the setup PRs are skipped. Hosted Factory execution remains unverified. ## Added repositories diff --git a/tests/test_workflow_authorization.cjs b/tests/test_workflow_authorization.cjs index ea40f02..0854ee8 100644 --- a/tests/test_workflow_authorization.cjs +++ b/tests/test_workflow_authorization.cjs @@ -30,7 +30,7 @@ function eventContext(kind) { const payload = { repository: { default_branch: 'main' }, sender: { login: 'writer', type: 'User' }, - action: kind === 'review' ? 'opened' : 'created', + action: kind === 'review' ? 'synchronize' : 'created', pull_request: pr, }; if (kind === 'mentions') { @@ -163,7 +163,7 @@ for (const kind of ['mentions', 'review', 'ci-steward']) { if (state === 'draft') pr.draft = true; const result = await authorize(kind, { pr }); denied(result); - assert.match(result.infos[0], /fork|draft|closed/); + assert.match(result.infos[0], /fork, draft, or closed/); }); } @@ -310,17 +310,17 @@ test('review: other labels do not block the first review', async () => { assert.equal((await authorize('review', { pr })).outputs.eligible, 'true'); }); -test('review: rejects a superseded base ref', async () => { +test('review: a base retarget before the run still reviews the PR', async () => { const pr = pullRequest(); pr.base.ref = 'different-base'; - denied(await authorize('review', { pr })); + assert.equal((await authorize('review', { pr })).outputs.eligible, 'true'); }); -test('review: pushes and edits do not start a review', () => { +test('review: pushes restart an unlabelled review; edits do not start one', () => { const types = workflows.review.match(/^ types: \[(.*)\]$/m); assert.ok(types); const actions = types[1].split(',').map(action => action.trim()); - assert.deepEqual(actions, ['opened', 'reopened', 'ready_for_review', 'converted_to_draft', 'closed']); + assert.deepEqual(actions, ['opened', 'synchronize', 'reopened', 'ready_for_review', 'converted_to_draft', 'closed']); }); test('review: lifecycle events share one cancellation group per PR', () => { @@ -329,7 +329,7 @@ test('review: lifecycle events share one cancellation group per PR', () => { const group = context => match[1].replace(/\$\{\{(.*?)\}\}/g, (_, expression) => evaluate(expression, context)); const context = eventContext('review'); const first = group(context); - for (const action of ['reopened', 'ready_for_review', 'converted_to_draft', 'closed']) { + for (const action of ['synchronize', 'reopened', 'ready_for_review', 'converted_to_draft', 'closed']) { context.payload.action = action; context.runId += 1; assert.equal(group(context), first); @@ -493,7 +493,7 @@ for (const [kind, job] of [['mentions', 'mentions'], ['ci-steward', 'steward']]) }); } -async function runLabelStep(labelExists) { +async function runLabelStep(labelExists, liveHead = 'head-sha') { const review = jobBlock('review', 'review'); const step = review.slice(review.indexOf('Mark the PR as reviewed')); const match = step.match(/ script: \|\n((?:^ .*\n|^\n)+)/m); @@ -501,6 +501,9 @@ async function runLabelStep(labelExists) { const calls = []; const github = { rest: { + pulls: { + async get() { return { data: { head: { sha: liveHead } } }; }, + }, issues: { async getLabel(params) { calls.push(['get', params.name]); @@ -513,7 +516,10 @@ async function runLabelStep(labelExists) { }, }; const context = eventContext('review'); - await vm.runInNewContext(`(async () => {\n${match[1].replace(/^ /gm, '')}\n})()`, { github, context }); + const core = { info: message => calls.push(['info', message]) }; + await vm.runInNewContext(`(async () => {\n${match[1].replace(/^ /gm, '')}\n})()`, { + github, context, core, process: { env: { REVIEWED_SHA: 'head-sha' } }, + }); return JSON.parse(JSON.stringify(calls)); } @@ -539,3 +545,10 @@ test('wiki: generation runs only on the default branch', () => { assert.equal(value, expected); } }); + +test('review: does not label a PR whose head moved during the review', async () => { + const calls = await runLabelStep(true, 'newer-head'); + assert.equal(calls.length, 1); + assert.equal(calls[0][0], 'info'); + assert.match(calls[0][1], /moved during the review/); +}); From cda9d054e0d9bc6ed5ddce9ca90813fb0242b1d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:40:12 +0000 Subject: [PATCH 06/12] Review CI Steward fix pushes on labelled PRs; note the mentions switch The droid-reviewed label skipped Steward fix commits, breaking the CI Steward promise that every fix push is reviewed. A push from factory-droid[bot] now bypasses the label. Document that @droid review needs JHD_FACTORY_MENTIONS_ENABLED. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/factory-review.yml | 12 ++++++++---- docs/OPERATIONS.md | 2 +- docs/PROVENANCE.md | 2 +- tests/test_workflow_authorization.cjs | 13 +++++++++++++ 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/.github/workflows/factory-review.yml b/.github/workflows/factory-review.yml index edd3561..659f64b 100644 --- a/.github/workflows/factory-review.yml +++ b/.github/workflows/factory-review.yml @@ -3,8 +3,10 @@ # One automatic review per PR. A successful review adds the droid-reviewed # label, and every later event on a labelled PR is skipped. Until then a push # cancels the review in progress and starts it again on the new head, so the -# reviewed code is the code that gets the label. To review again, comment -# `@droid review` (factory-mentions.yml), or remove the label. +# reviewed code is the code that gets the label. A CI Steward fix pushed by +# factory-droid[bot] is still reviewed on a labelled PR. To review again, +# comment `@droid review` (factory-mentions.yml, which needs +# JHD_FACTORY_MENTIONS_ENABLED), or remove the label. name: Factory Review on: @@ -22,7 +24,8 @@ jobs: authorize: if: >- vars.JHD_FACTORY_REVIEW_ENABLED == 'true' && - !contains(github.event.pull_request.labels.*.name, 'droid-reviewed') && + (!contains(github.event.pull_request.labels.*.name, 'droid-reviewed') || + github.event.sender.login == 'factory-droid[bot]') && github.event.pull_request.draft == false && github.event.pull_request.head.repo.full_name == github.repository && (github.event.sender.type == 'User' || @@ -51,7 +54,8 @@ jobs: core.info('Skipping a fork, draft, or closed PR.'); return; } - if (pr.labels?.some(label => label.name === 'droid-reviewed')) { + if (context.actor !== 'factory-droid[bot]' && + pr.labels?.some(label => label.name === 'droid-reviewed')) { core.info('This PR already has its Factory review. Comment `@droid review` to review it again.'); return; } diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index df98ada..c484e86 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -8,7 +8,7 @@ These frequencies are JHD choices for a low-volume organization, not Factory Rea | Automation | Starts when | Required outcome | |---|---|---| -| Code Review | Eligible PR, until its first review succeeds | One review with a security pass per PR. A push during the review restarts it on the new head. The `droid-reviewed` label marks it done, and labelled PRs are skipped. Comment `@droid review` for another one. | +| Code Review | Eligible PR, until its first review succeeds | One review with a security pass per PR. A push during the review restarts it on the new head. The `droid-reviewed` label marks it done, and labelled PRs are skipped, except CI Steward fix pushes by `factory-droid[bot]`. Comment `@droid review` for another one; that needs `JHD_FACTORY_MENTIONS_ENABLED`. | | AutoWiki | Monthly, 06:17 UTC on the 1st, or a manual run | Refresh the Factory wiki and open or update one reviewable `factory/droid-wiki` PR containing generated pages and metadata. | | Issue Triage | New GitHub issue | One comment classifying it, suggesting related work and a human route. No implementation or ready label. | | CI Steward | An allowlisted deterministic CI workflow fails | Diagnose it; at most one retry and two fix commits, with ten invocations per PR. | diff --git a/docs/PROVENANCE.md b/docs/PROVENANCE.md index 161cdd9..3ed00a9 100644 --- a/docs/PROVENANCE.md +++ b/docs/PROVENANCE.md @@ -14,7 +14,7 @@ The review, mention, Steward and Triage wrappers reference commits, with version ## Corrections to the official templates -- Review once per PR: a successful review adds the `droid-reviewed` label, and labelled PRs are skipped. Until then a push restarts the review on the new head. `@droid review` asks for another review. +- Review once per PR: a successful review adds the `droid-reviewed` label, and labelled PRs are skipped, except CI Steward fix pushes by `factory-droid[bot]`. Until then a push restarts the review on the new head. `@droid review` asks for another review when mentions are enabled. - List trusted bots by name. Never use a wildcard. Review, mentions, Triage and CI Steward are opt-in through `JHD_FACTORY_REVIEW_ENABLED`, `JHD_FACTORY_MENTIONS_ENABLED`, `JHD_FACTORY_TRIAGE_ENABLED`, `JHD_FACTORY_STEWARD_ENABLED`, and `JHD_FACTORY_STEWARD_FIX_ENABLED`. AutoWiki replaces the official default-branch push trigger with a monthly schedule and a manual run; merging its setup PR is activation, not a gated dry run. diff --git a/tests/test_workflow_authorization.cjs b/tests/test_workflow_authorization.cjs index 0854ee8..83ba8b1 100644 --- a/tests/test_workflow_authorization.cjs +++ b/tests/test_workflow_authorization.cjs @@ -301,6 +301,19 @@ test('review: a PR labelled droid-reviewed is not reviewed again', async () => { assert.match(result.infos[0], /@droid review/); }); +test('review: a CI Steward fix push on a labelled PR is still reviewed', async () => { + const context = eventContext('review'); + context.actor = 'factory-droid[bot]'; + context.payload.sender = { login: context.actor, type: 'Bot' }; + context.payload.pull_request.labels = [{ name: 'droid-reviewed' }]; + assert.equal(jobCondition('review', context), true); + const pr = pullRequest(); + pr.labels = [{ name: 'droid-reviewed' }]; + const result = await authorize('review', { context, pr, triggeringActor: context.actor }); + assert.equal(result.outputs.eligible, 'true'); + assert.deepEqual(result.permissions, []); +}); + test('review: other labels do not block the first review', async () => { const context = eventContext('review'); context.payload.pull_request.labels = [{ name: 'enhancement' }]; From 2227342daf826b78e2c6552cfd20187d6c762806 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:42:48 +0000 Subject: [PATCH 07/12] Say that removing the label makes the next push eligible Removing droid-reviewed sends an unlabeled event, which the workflow does not listen to, so the removal alone starts no review. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/factory-review.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/factory-review.yml b/.github/workflows/factory-review.yml index 659f64b..407a94a 100644 --- a/.github/workflows/factory-review.yml +++ b/.github/workflows/factory-review.yml @@ -6,7 +6,8 @@ # reviewed code is the code that gets the label. A CI Steward fix pushed by # factory-droid[bot] is still reviewed on a labelled PR. To review again, # comment `@droid review` (factory-mentions.yml, which needs -# JHD_FACTORY_MENTIONS_ENABLED), or remove the label. +# JHD_FACTORY_MENTIONS_ENABLED), or remove the label so the next push gets +# reviewed. name: Factory Review on: From d8aa5a1f375b2765d33bf110075245ce9828bdee Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:43:26 +0000 Subject: [PATCH 08/12] Treat a label created by a concurrent review as success Two first reviews finishing together can both see the label missing; the second createLabel returns 422 and used to stop the step before addLabels, leaving a reviewed PR unmarked. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/factory-review.yml | 13 +++++++++---- tests/test_workflow_authorization.cjs | 15 +++++++++++++-- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/.github/workflows/factory-review.yml b/.github/workflows/factory-review.yml index 407a94a..6cff741 100644 --- a/.github/workflows/factory-review.yml +++ b/.github/workflows/factory-review.yml @@ -139,10 +139,15 @@ jobs: await github.rest.issues.getLabel({ owner, repo, name }); } catch (error) { if (error.status !== 404) throw error; - await github.rest.issues.createLabel({ - owner, repo, name, color: '5319e7', - description: 'Factory review done; comment @droid review for another', - }); + try { + await github.rest.issues.createLabel({ + owner, repo, name, color: '5319e7', + description: 'Factory review done; comment @droid review for another', + }); + } catch (error) { + // Another PR's review created it first. + if (error.status !== 422) throw error; + } } await github.rest.issues.addLabels({ owner, repo, issue_number: context.payload.pull_request.number, diff --git a/tests/test_workflow_authorization.cjs b/tests/test_workflow_authorization.cjs index 83ba8b1..4dea30b 100644 --- a/tests/test_workflow_authorization.cjs +++ b/tests/test_workflow_authorization.cjs @@ -520,10 +520,15 @@ async function runLabelStep(labelExists, liveHead = 'head-sha') { issues: { async getLabel(params) { calls.push(['get', params.name]); - if (!labelExists) throw Object.assign(new Error('Not Found'), { status: 404 }); + if (labelExists !== true) throw Object.assign(new Error('Not Found'), { status: 404 }); return { data: {} }; }, - async createLabel(params) { calls.push(['create', params.name]); }, + async createLabel(params) { + calls.push(['create', params.name]); + if (labelExists === 'race') { + throw Object.assign(new Error('already_exists'), { status: 422 }); + } + }, async addLabels(params) { calls.push(['add', params.issue_number, params.labels]); }, }, }, @@ -545,6 +550,12 @@ test('review: creates the droid-reviewed label when the repository lacks it', as ]); }); +test('review: labels the PR when another review created the label first', async () => { + assert.deepEqual(await runLabelStep('race'), [ + ['get', 'droid-reviewed'], ['create', 'droid-reviewed'], ['add', 42, ['droid-reviewed']], + ]); +}); + test('wiki: generation runs only on the default branch', () => { const wiki = readFileSync(join(__dirname, '../.github/workflows/droid-wiki-refresh.yml'), 'utf8'); const job = wiki.split('\n wiki-refresh:\n')[1]; From fb09d8c9fb4cdd79975e54a20afdeb23fe59c0b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:44:50 +0000 Subject: [PATCH 09/12] Confirm the label exists after a rejected create A 422 can also mean an invalid request, so look the label up again and let that lookup fail if the label is still missing. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/factory-review.yml | 4 +++- tests/test_workflow_authorization.cjs | 14 +++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/.github/workflows/factory-review.yml b/.github/workflows/factory-review.yml index 6cff741..8594a44 100644 --- a/.github/workflows/factory-review.yml +++ b/.github/workflows/factory-review.yml @@ -145,8 +145,10 @@ jobs: description: 'Factory review done; comment @droid review for another', }); } catch (error) { - // Another PR's review created it first. + // Another PR's review may have created it first; any other + // 422 leaves the label missing and the lookup throws. if (error.status !== 422) throw error; + await github.rest.issues.getLabel({ owner, repo, name }); } } await github.rest.issues.addLabels({ diff --git a/tests/test_workflow_authorization.cjs b/tests/test_workflow_authorization.cjs index 4dea30b..94e66aa 100644 --- a/tests/test_workflow_authorization.cjs +++ b/tests/test_workflow_authorization.cjs @@ -520,12 +520,15 @@ async function runLabelStep(labelExists, liveHead = 'head-sha') { issues: { async getLabel(params) { calls.push(['get', params.name]); - if (labelExists !== true) throw Object.assign(new Error('Not Found'), { status: 404 }); + const created = labelExists === 'race' && calls.some(call => call[0] === 'create'); + if (labelExists !== true && !created) { + throw Object.assign(new Error('Not Found'), { status: 404 }); + } return { data: {} }; }, async createLabel(params) { calls.push(['create', params.name]); - if (labelExists === 'race') { + if (labelExists === 'race' || labelExists === 'invalid') { throw Object.assign(new Error('already_exists'), { status: 422 }); } }, @@ -552,10 +555,15 @@ test('review: creates the droid-reviewed label when the repository lacks it', as test('review: labels the PR when another review created the label first', async () => { assert.deepEqual(await runLabelStep('race'), [ - ['get', 'droid-reviewed'], ['create', 'droid-reviewed'], ['add', 42, ['droid-reviewed']], + ['get', 'droid-reviewed'], ['create', 'droid-reviewed'], ['get', 'droid-reviewed'], + ['add', 42, ['droid-reviewed']], ]); }); +test('review: fails when label creation is rejected and the label is still missing', async () => { + await assert.rejects(runLabelStep('invalid'), { status: 404 }); +}); + test('wiki: generation runs only on the default branch', () => { const wiki = readFileSync(join(__dirname, '../.github/workflows/droid-wiki-refresh.yml'), 'utf8'); const job = wiki.split('\n wiki-refresh:\n')[1]; From 79b33a07e96362966af5fab027e06aa36eae06eb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:45:34 +0000 Subject: [PATCH 10/12] Run the wiki refresh only from the default branch, not a same-named tag Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/droid-wiki-refresh.yml | 2 +- tests/test_workflow_authorization.cjs | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/droid-wiki-refresh.yml b/.github/workflows/droid-wiki-refresh.yml index db56470..6e03770 100644 --- a/.github/workflows/droid-wiki-refresh.yml +++ b/.github/workflows/droid-wiki-refresh.yml @@ -30,7 +30,7 @@ env: jobs: wiki-refresh: # A manual run from another branch would document unmerged code. - if: github.ref_name == github.event.repository.default_branch + if: github.ref_type == 'branch' && github.ref_name == github.event.repository.default_branch runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 diff --git a/tests/test_workflow_authorization.cjs b/tests/test_workflow_authorization.cjs index 94e66aa..cb1cf7b 100644 --- a/tests/test_workflow_authorization.cjs +++ b/tests/test_workflow_authorization.cjs @@ -570,9 +570,11 @@ test('wiki: generation runs only on the default branch', () => { assert.ok(job); const condition = job.match(/^ if: (.*)$/m); assert.ok(condition); - for (const [ref, expected] of [['main', true], ['feature', false]]) { + for (const [ref, type, expected] of [ + ['main', 'branch', true], ['feature', 'branch', false], ['main', 'tag', false], + ]) { const value = vm.runInNewContext(condition[1].replaceAll('github.', 'github_.'), { - github_: { ref_name: ref, event: { repository: { default_branch: 'main' } } }, + github_: { ref_name: ref, ref_type: type, event: { repository: { default_branch: 'main' } } }, }); assert.equal(value, expected); } From 5f48052ad929f47e48d6c557d42b1383a40696fa Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:49:04 +0000 Subject: [PATCH 11/12] Cut the review-once change back to what was asked for Restore main's review workflow and add only the droid-reviewed skip (with the CI Steward exception) and one addLabels call, which creates the label when missing. Drop the label bookkeeping, head-move guard, trigger rewrite and tag check that came from bot review rounds. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/droid-wiki-refresh.yml | 7 +- .github/workflows/factory-review.yml | 94 ++++---- docs/OPERATIONS.md | 8 +- docs/PROVENANCE.md | 5 +- tests/test_workflow_authorization.cjs | 267 +++++++++++------------ 5 files changed, 183 insertions(+), 198 deletions(-) diff --git a/.github/workflows/droid-wiki-refresh.yml b/.github/workflows/droid-wiki-refresh.yml index 6e03770..bbe6c7c 100644 --- a/.github/workflows/droid-wiki-refresh.yml +++ b/.github/workflows/droid-wiki-refresh.yml @@ -1,8 +1,7 @@ name: Droid Wiki Refresh -# Monthly, not on every push to main: each run is a paid Droid session, and -# a monthly incremental refresh keeps the wiki current enough. Run it by hand -# from the Actions tab when the wiki needs refreshing sooner. +# Monthly, not on every push to main: each run is a paid Droid session. +# Run it by hand from the Actions tab when the wiki needs refreshing sooner. on: schedule: - cron: '17 6 1 * *' # 06:17 UTC on the 1st of each month @@ -29,8 +28,6 @@ env: jobs: wiki-refresh: - # A manual run from another branch would document unmerged code. - if: github.ref_type == 'branch' && github.ref_name == github.event.repository.default_branch runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/factory-review.yml b/.github/workflows/factory-review.yml index 8594a44..67d243c 100644 --- a/.github/workflows/factory-review.yml +++ b/.github/workflows/factory-review.yml @@ -1,30 +1,26 @@ # Based on Factory-AI/droid-action v10 native automatic review and security modes. # -# One automatic review per PR. A successful review adds the droid-reviewed -# label, and every later event on a labelled PR is skipped. Until then a push -# cancels the review in progress and starts it again on the new head, so the -# reviewed code is the code that gets the label. A CI Steward fix pushed by -# factory-droid[bot] is still reviewed on a labelled PR. To review again, -# comment `@droid review` (factory-mentions.yml, which needs -# JHD_FACTORY_MENTIONS_ENABLED), or remove the label so the next push gets -# reviewed. +# One review per PR: a successful review adds the droid-reviewed label, and +# labelled PRs are skipped, except CI Steward fix pushes, which +# factory-ci-steward.yml promises are reviewed. Comment `@droid review` for +# another review. name: Factory Review on: pull_request: - # converted_to_draft and closed only cancel a review still in progress. - types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] + types: [opened, synchronize, reopened, ready_for_review, edited, converted_to_draft, closed] permissions: {} concurrency: - group: factory-review-${{ github.event.pull_request.number }} + group: factory-review-${{ github.event.action == 'edited' && !github.event.changes.base && format('edit-{0}', github.run_id) || github.event.pull_request.number }} cancel-in-progress: true jobs: authorize: if: >- vars.JHD_FACTORY_REVIEW_ENABLED == 'true' && + (github.event.action != 'edited' || github.event.changes.base) && (!contains(github.event.pull_request.labels.*.name, 'droid-reviewed') || github.event.sender.login == 'factory-droid[bot]') && github.event.pull_request.draft == false && @@ -33,12 +29,12 @@ jobs: github.event.sender.login == 'factory-droid[bot]') runs-on: ubuntu-latest permissions: + actions: read pull-requests: read outputs: eligible: ${{ steps.check.outputs.eligible }} - head_sha: ${{ steps.check.outputs.head_sha }} steps: - - name: Check the PR and whether it was already reviewed + - name: Check current head and completed reviews id: check uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 env: @@ -47,17 +43,18 @@ jobs: script: | const { owner, repo } = context.repo; const expected = context.payload.pull_request; + if (context.payload.action === 'edited' && !context.payload.changes?.base) { + core.info('Skipping an edit that did not change the PR base.'); + return; + } const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: expected.number, }); if (pr.state !== 'open' || pr.draft || - pr.head.repo?.full_name !== `${owner}/${repo}`) { - core.info('Skipping a fork, draft, or closed PR.'); - return; - } - if (context.actor !== 'factory-droid[bot]' && - pr.labels?.some(label => label.name === 'droid-reviewed')) { - core.info('This PR already has its Factory review. Comment `@droid review` to review it again.'); + pr.head.repo?.full_name !== `${owner}/${repo}` || + pr.head.sha !== expected.head.sha || + pr.base.ref !== expected.base.ref) { + core.info('Skipping a fork, draft, closed PR, or superseded head/base.'); return; } const actors = new Set([context.actor, process.env.TRIGGERING_ACTOR]); @@ -75,7 +72,27 @@ jobs: return; } } - core.setOutput('head_sha', pr.head.sha); + const { data } = await github.rest.actions.listWorkflowRuns({ + owner, repo, workflow_id: 'factory-review.yml', + event: 'pull_request', head_sha: pr.head.sha, + status: 'success', per_page: 100, + }); + for (const run of data.workflow_runs) { + if (run.id === context.runId) continue; + if (!run.pull_requests?.some(candidate => + candidate.number === pr.number && + candidate.base?.ref === pr.base.ref && + candidate.base?.sha === pr.base.sha)) continue; + const jobs = await github.paginate( + github.rest.actions.listJobsForWorkflowRun, + { owner, repo, run_id: run.id, per_page: 100 }, + ); + if (jobs.some(job => job.name === 'Advisory review and security' && + job.conclusion === 'success')) { + core.info('This PR head and base already completed Factory review.'); + return; + } + } core.setOutput('eligible', 'true'); review: @@ -104,7 +121,7 @@ jobs: - name: Check out the reviewed head without saved credentials uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5 with: - ref: ${{ needs.authorize.outputs.head_sha }} + ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 1 persist-credentials: false @@ -120,38 +137,13 @@ jobs: allowed_bots: factory-droid show_full_output: "false" + # Adding a missing label creates it. - name: Mark the PR as reviewed uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 - env: - REVIEWED_SHA: ${{ needs.authorize.outputs.head_sha }} with: script: | - const { owner, repo } = context.repo; - const name = 'droid-reviewed'; - const { data: pr } = await github.rest.pulls.get({ - owner, repo, pull_number: context.payload.pull_request.number, - }); - if (pr.head.sha !== process.env.REVIEWED_SHA) { - core.info('The PR moved during the review; the next run reviews the new head.'); - return; - } - try { - await github.rest.issues.getLabel({ owner, repo, name }); - } catch (error) { - if (error.status !== 404) throw error; - try { - await github.rest.issues.createLabel({ - owner, repo, name, color: '5319e7', - description: 'Factory review done; comment @droid review for another', - }); - } catch (error) { - // Another PR's review may have created it first; any other - // 422 leaves the label missing and the lookup throws. - if (error.status !== 422) throw error; - await github.rest.issues.getLabel({ owner, repo, name }); - } - } await github.rest.issues.addLabels({ - owner, repo, issue_number: context.payload.pull_request.number, - labels: [name], + ...context.repo, + issue_number: context.payload.pull_request.number, + labels: ['droid-reviewed'], }); diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index c484e86..ca084c6 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -2,13 +2,13 @@ ## Agreed operating model -Code Review, issue Triage, and bounded CI repair run on events, not polling schedules. AutoWiki runs monthly as a GitHub Actions schedule, or by hand from the Actions tab. Weekly Health and Digest and monthly Readiness run as local native Factory automations on the Mac mini. Ticket-to-PR and recurring source Intake are deferred. +Code Review, issue Triage, and bounded CI repair run on events, not polling schedules. AutoWiki runs monthly, or by hand from the Actions tab. Weekly Health and Digest and monthly Readiness run as local native Factory automations on the Mac mini. Ticket-to-PR and recurring source Intake are deferred. These frequencies are JHD choices for a low-volume organization, not Factory Readiness requirements. | Automation | Starts when | Required outcome | |---|---|---| -| Code Review | Eligible PR, until its first review succeeds | One review with a security pass per PR. A push during the review restarts it on the new head. The `droid-reviewed` label marks it done, and labelled PRs are skipped, except CI Steward fix pushes by `factory-droid[bot]`. Comment `@droid review` for another one; that needs `JHD_FACTORY_MENTIONS_ENABLED`. | +| Code Review | Eligible PR without the `droid-reviewed` label | One review with a security pass per PR; the label marks it done. CI Steward fix pushes are still reviewed. Comment `@droid review` for another one (needs `JHD_FACTORY_MENTIONS_ENABLED`). | | AutoWiki | Monthly, 06:17 UTC on the 1st, or a manual run | Refresh the Factory wiki and open or update one reviewable `factory/droid-wiki` PR containing generated pages and metadata. | | Issue Triage | New GitHub issue | One comment classifying it, suggesting related work and a human route. No implementation or ready label. | | CI Steward | An allowlisted deterministic CI workflow fails | Diagnose it; at most one retry and two fix commits, with ten invocations per PR. | @@ -60,7 +60,7 @@ Use the current official `/install-wiki` template, including fixed repository-wi Mythic's existing setup is PR #1921. It already contains these settings and is reused unchanged. The other twelve repositories receive the same template in their existing setup PRs. Do not open a duplicate Mythic wiki workflow. -**Merging a wiki setup workflow activates its monthly schedule** (06:17 UTC on the 1st). It has no JHD enable variable. For an immediate refresh, run it by hand from the Actions tab on the default branch; a run started from any other branch generates nothing. GitHub disables scheduled workflows in a public repository after 60 days without repository activity; re-enable the workflow from the Actions tab when that happens. +**Merging a wiki setup workflow activates its monthly schedule.** It has no JHD enable variable. Wait for restored Factory quota and approval of that activation before merging those PRs. This preparation does not merge any PR. `FACTORY_API_KEY` already exists at organization scope for all repositories. No `FACTORY_WIKI_PR_TOKEN` was present in organization or repository secret metadata at preparation time. Do not reuse or widen `RELEASE_PLEASE_TOKEN`. @@ -120,7 +120,7 @@ Verify available Factory quota before a paid execution. Do not activate on a gue 2. Confirm the affected repository can actually start GitHub Actions jobs after the CI budget change. A zero-step billing failure proves nothing about Factory or Sentry credentials. 3. Canary Code Review on one same-repository PR/head. Confirm a real posted review, then cut over that repository's legacy responder. 4. Canary one issue classification. Expect one bounded comment, no implementation or label changes, and no second comment on rerun. -5. Approve AutoWiki activation and its publishing credential/permission path. Merge one wiki setup, start one manual run on the default branch, and observe one rolling wiki PR, then an incremental update with preserved metadata. +5. Approve AutoWiki activation and its publishing credential/permission path. Merge one wiki setup, start one manual run, and observe one rolling wiki PR, then an incremental update with preserved metadata. 6. Canary CI Steward with one eligible nonproduction failure. Verify diagnosis before separately enabling bounded repairs. 7. Canary Digest against the known private status issue, Health against one named repository, and Readiness against one named repository. Keep their schedules paused during each manual canary. 8. Resume only the three recurring automations whose outcomes passed and whose activation is approved. Leave Ticket-to-PR and Intake deferred. diff --git a/docs/PROVENANCE.md b/docs/PROVENANCE.md index 3ed00a9..869a9cf 100644 --- a/docs/PROVENANCE.md +++ b/docs/PROVENANCE.md @@ -14,7 +14,8 @@ The review, mention, Steward and Triage wrappers reference commits, with version ## Corrections to the official templates -- Review once per PR: a successful review adds the `droid-reviewed` label, and labelled PRs are skipped, except CI Steward fix pushes by `factory-droid[bot]`. Until then a push restarts the review on the new head. `@droid review` asks for another review when mentions are enabled. +- Add `synchronize` to the pull request triggers, so each new head is reviewed until the first review succeeds. +- Review once per PR: a successful review adds the `droid-reviewed` label, and labelled PRs are skipped except CI Steward fix pushes. - List trusted bots by name. Never use a wildcard. Review, mentions, Triage and CI Steward are opt-in through `JHD_FACTORY_REVIEW_ENABLED`, `JHD_FACTORY_MENTIONS_ENABLED`, `JHD_FACTORY_TRIAGE_ENABLED`, `JHD_FACTORY_STEWARD_ENABLED`, and `JHD_FACTORY_STEWARD_FIX_ENABLED`. AutoWiki replaces the official default-branch push trigger with a monthly schedule and a manual run; merging its setup PR is activation, not a gated dry run. @@ -29,7 +30,7 @@ The workflows use the official upstream `Factory-AI/droid-action` v10 at the pin - Issue Triage: 13 classification-only workflows using the official pinned `Factory-AI/droid-action/base-action`, with restricted local tools and a separate validated comment job. This is JHD configuration, not the scheduled Slack Triage package. - AutoWiki: twelve official-template workflows added to existing setup PRs; Mythic's existing PR #1921 already has the current template and is reused unchanged. All restore the rolling wiki, preserve hidden metadata, exclude media, and separate read-only generation from write-permission publication. - CI Steward: six repositories with existing deterministic CI, watching only those workflows. The list is in `docs/OPERATIONS.md`. -- Review checks out the live PR head and labels the PR only if that head has not moved. A PR with the `droid-reviewed` label is not reviewed again. A push, converting the PR to a draft, or closing it cancels a review in progress. +- Review rejects stale PR head SHAs and changed base branch refs. A base branch that advances to a new commit does not suppress review. Completed-review deduplication uses the live PR head SHA, base ref, and base SHA. The workflow cancels superseded runs and skips a matching completed review. - Opt-in jobs on the setup PRs are skipped. Hosted Factory execution remains unverified. ## Added repositories diff --git a/tests/test_workflow_authorization.cjs b/tests/test_workflow_authorization.cjs index cb1cf7b..30140d7 100644 --- a/tests/test_workflow_authorization.cjs +++ b/tests/test_workflow_authorization.cjs @@ -60,7 +60,7 @@ function eventContext(kind) { async function authorize(kind, options = {}) { const context = options.context || eventContext(kind); const pr = options.pr || pullRequest(); - const result = { outputs: {}, failures: [], infos: [], permissions: [], pulls: [] }; + const result = { outputs: {}, failures: [], infos: [], permissions: [], jobs: [], pulls: [] }; const github = { rest: { repos: { @@ -83,6 +83,21 @@ async function authorize(kind, options = {}) { return { data: pr }; }, }, + actions: { + async listWorkflowRuns(params) { + assert.equal(params.workflow_id, 'factory-review.yml'); + assert.equal(params.head_sha, pr.head.sha); + assert.equal(params.event, 'pull_request'); + assert.equal(params.status, 'success'); + return { data: { workflow_runs: options.runs || [] } }; + }, + listJobsForWorkflowRun() { throw new Error('Use github.paginate for jobs'); }, + }, + }, + async paginate(method, { run_id }) { + assert.equal(method, github.rest.actions.listJobsForWorkflowRun); + result.jobs.push(run_id); + return options.jobs?.[run_id] || []; }, }; const core = { @@ -106,10 +121,7 @@ function evaluate(expression, context, vars = {}, scope = {}) { changes: {}, comment: { user: {} }, review: { user: {} }, ...context.payload, }; - const script = expression - .replaceAll('pull_requests[0].number', 'pull_requests?.[0]?.number') - .replaceAll('labels.*.name', 'labels?.map(label => label.name)'); - return vm.runInNewContext(script, { + return vm.runInNewContext(expression.replaceAll('pull_requests[0].number', 'pull_requests?.[0]?.number').replaceAll('labels.*.name', 'labels?.map(label => label.name)'), { github: { event, actor: context.actor, event_name: context.eventName, repository: 'jhd/project', run_id: context.runId, @@ -155,15 +167,16 @@ for (const kind of ['mentions', 'review', 'ci-steward']) { } } - for (const state of kind === 'review' ? ['fork', 'closed', 'draft'] : []) { + for (const state of kind === 'review' ? ['fork', 'closed', 'draft', 'stale'] : []) { test(`${kind}: rejects ${state} PR`, async () => { const pr = pullRequest(); if (state === 'fork') pr.head.repo.full_name = 'outside/project'; if (state === 'closed') pr.state = 'closed'; if (state === 'draft') pr.draft = true; + if (state === 'stale') pr.head.sha = 'newer-head'; const result = await authorize(kind, { pr }); denied(result); - assert.match(result.infos[0], /fork, draft, or closed/); + assert.match(result.infos[0], /fork|superseded/); }); } @@ -280,82 +293,119 @@ for (const body of ['@droid', '@droid review', '@droid fill', '@droid security', }); } -test('review: reviews the live head when the PR moved since the event', async () => { +function reviewRun(id = 99) { const pr = pullRequest(); - pr.head.sha = 'newer-head'; - const result = await authorize('review', { pr }); - assert.equal(result.outputs.eligible, 'true'); - assert.equal(result.outputs.head_sha, 'newer-head'); -}); + return { id, head_sha: pr.head.sha, pull_requests: [{ number: pr.number, base: pr.base }] }; +} -test('review: a PR labelled droid-reviewed is not reviewed again', async () => { - const context = eventContext('review'); - context.payload.pull_request.labels = [{ name: 'droid-reviewed' }]; - assert.equal(jobCondition('review', context), false); - const pr = pullRequest(); - pr.labels = [{ name: 'droid-reviewed' }]; - const result = await authorize('review', { pr }); +for (const mismatch of ['other PR', 'base ref', 'base SHA', 'no associations', 'missing associations', 'missing base']) { + test(`review: previous success with ${mismatch} does not suppress review`, async () => { + const run = reviewRun(); + if (mismatch === 'other PR') run.pull_requests[0].number = 41; + if (mismatch === 'base ref') run.pull_requests[0].base.ref = 'release'; + if (mismatch === 'base SHA') run.pull_requests[0].base.sha = 'old-base'; + if (mismatch === 'no associations') run.pull_requests = []; + if (mismatch === 'missing associations') delete run.pull_requests; + if (mismatch === 'missing base') delete run.pull_requests[0].base; + const result = await authorize('review', { + runs: [run], jobs: { 99: [{ name: 'Advisory review and security', conclusion: 'success' }] }, + }); + assert.equal(result.outputs.eligible, 'true'); + assert.deepEqual(result.jobs, []); + }); +} + +test('review: same PR/head/base successful review deduplicates', async () => { + const result = await authorize('review', { + runs: [reviewRun()], + jobs: { 99: [{ name: 'Advisory review and security', conclusion: 'success' }] }, + }); denied(result); assert.deepEqual(result.failures, []); - assert.match(result.infos[0], /already has its Factory review/); - assert.match(result.infos[0], /@droid review/); + assert.deepEqual(result.jobs, [99]); + assert.match(result.infos[0], /already completed/); }); -test('review: a CI Steward fix push on a labelled PR is still reviewed', async () => { - const context = eventContext('review'); - context.actor = 'factory-droid[bot]'; - context.payload.sender = { login: context.actor, type: 'Bot' }; - context.payload.pull_request.labels = [{ name: 'droid-reviewed' }]; - assert.equal(jobCondition('review', context), true); - const pr = pullRequest(); - pr.labels = [{ name: 'droid-reviewed' }]; - const result = await authorize('review', { context, pr, triggeringActor: context.actor }); - assert.equal(result.outputs.eligible, 'true'); - assert.deepEqual(result.permissions, []); -}); +for (const jobs of [ + [], [{ name: 'authorize', conclusion: 'success' }], + [{ name: 'Advisory review and security', conclusion: 'skipped' }], + [{ name: 'Advisory review and security', conclusion: 'failure' }], +]) { + test(`review: no completed successful review job (${JSON.stringify(jobs)}) remains eligible`, async () => { + const result = await authorize('review', { runs: [reviewRun()], jobs: { 99: jobs } }); + assert.equal(result.outputs.eligible, 'true'); + assert.deepEqual(result.jobs, [99]); + }); +} -test('review: other labels do not block the first review', async () => { - const context = eventContext('review'); - context.payload.pull_request.labels = [{ name: 'enhancement' }]; - assert.equal(jobCondition('review', context), true); - const pr = pullRequest(); - pr.labels = [{ name: 'enhancement' }]; - assert.equal((await authorize('review', { pr })).outputs.eligible, 'true'); +test('review: skips current run and continues past unrelated candidates', async () => { + const unrelated = reviewRun(98); + unrelated.pull_requests[0].number = 41; + const result = await authorize('review', { + runs: [reviewRun(100), unrelated, reviewRun()], + jobs: { 99: [{ name: 'Advisory review and security', conclusion: 'success' }] }, + }); + denied(result); + assert.deepEqual(result.jobs, [99]); }); -test('review: a base retarget before the run still reviews the PR', async () => { +test('review: rejects a superseded base ref', async () => { const pr = pullRequest(); pr.base.ref = 'different-base'; - assert.equal((await authorize('review', { pr })).outputs.eligible, 'true'); + denied(await authorize('review', { pr })); }); -test('review: pushes restart an unlabelled review; edits do not start one', () => { - const types = workflows.review.match(/^ types: \[(.*)\]$/m); - assert.ok(types); - const actions = types[1].split(',').map(action => action.trim()); - assert.deepEqual(actions, ['opened', 'synchronize', 'reopened', 'ready_for_review', 'converted_to_draft', 'closed']); +test('review: base SHA drift remains eligible and deduplicates against the live base', async () => { + const pr = pullRequest(); + pr.base.sha = 'new-base'; + const oldRun = reviewRun(); + const liveRun = reviewRun(98); + liveRun.pull_requests[0].base.sha = pr.base.sha; + const jobs = { + 99: [{ name: 'Advisory review and security', conclusion: 'success' }], + 98: [{ name: 'Advisory review and security', conclusion: 'success' }], + }; + const eligible = await authorize('review', { pr, runs: [oldRun], jobs }); + assert.equal(eligible.outputs.eligible, 'true'); + assert.deepEqual(eligible.jobs, []); + const completed = await authorize('review', { pr, runs: [oldRun, liveRun], jobs }); + denied(completed); + assert.deepEqual(completed.jobs, [98]); + assert.match(completed.infos[0], /already completed/); }); -test('review: lifecycle events share one cancellation group per PR', () => { +function reviewGroup(context) { const match = workflows.review.match(/^ group: (.*)\n/m); assert.ok(match); - const group = context => match[1].replace(/\$\{\{(.*?)\}\}/g, (_, expression) => evaluate(expression, context)); - const context = eventContext('review'); - const first = group(context); - for (const action of ['synchronize', 'reopened', 'ready_for_review', 'converted_to_draft', 'closed']) { - context.payload.action = action; - context.runId += 1; - assert.equal(group(context), first); + return match[1].replace(/\$\{\{(.*?)\}\}/g, (_, expression) => evaluate(expression, context)); +} + +test('review: title/body edits neither authorize nor cancel an active review', async () => { + const current = eventContext('review'); + for (const changes of [{ title: { from: 'old' } }, { body: { from: 'old' } }]) { + const edit = eventContext('review'); + edit.payload.action = 'edited'; + edit.payload.changes = changes; + edit.runId = 101; + assert.equal(jobCondition('review', edit), false); + denied(await authorize('review', { context: edit })); + assert.notEqual(reviewGroup(edit), reviewGroup(current)); + assert.notEqual(reviewGroup(edit), reviewGroup({ ...edit, runId: 102 })); } }); -test('review: labels the PR only after the review step', () => { - const review = jobBlock('review', 'review'); - const reviewStep = review.indexOf('Run native deep review and security'); - const labelStep = review.indexOf('Mark the PR as reviewed'); - assert.ok(reviewStep > 0 && labelStep > reviewStep); - assert.match(review.slice(labelStep), /labels: \[name\]/); - assert.match(review, /ref: \$\{\{ needs\.authorize\.outputs\.head_sha \}\}/); +test('review: base retargets authorize and share head/lifecycle cancellation group', async () => { + const context = eventContext('review'); + const group = reviewGroup(context); + context.payload.action = 'edited'; + context.payload.changes = { base: { ref: { from: 'release' }, sha: { from: 'old-base' } } }; + assert.equal(jobCondition('review', context), true); + assert.equal((await authorize('review', { context })).outputs.eligible, 'true'); + assert.equal(reviewGroup(context), group); + for (const action of ['opened', 'synchronize', 'reopened', 'ready_for_review', 'converted_to_draft', 'closed']) { + context.payload.action = action; + assert.equal(reviewGroup(context), group); + } }); for (const kind of ['review', 'ci-steward']) { @@ -506,83 +556,28 @@ for (const [kind, job] of [['mentions', 'mentions'], ['ci-steward', 'steward']]) }); } -async function runLabelStep(labelExists, liveHead = 'head-sha') { - const review = jobBlock('review', 'review'); - const step = review.slice(review.indexOf('Mark the PR as reviewed')); - const match = step.match(/ script: \|\n((?:^ .*\n|^\n)+)/m); - assert.ok(match, 'Missing label step script'); - const calls = []; - const github = { - rest: { - pulls: { - async get() { return { data: { head: { sha: liveHead } } }; }, - }, - issues: { - async getLabel(params) { - calls.push(['get', params.name]); - const created = labelExists === 'race' && calls.some(call => call[0] === 'create'); - if (labelExists !== true && !created) { - throw Object.assign(new Error('Not Found'), { status: 404 }); - } - return { data: {} }; - }, - async createLabel(params) { - calls.push(['create', params.name]); - if (labelExists === 'race' || labelExists === 'invalid') { - throw Object.assign(new Error('already_exists'), { status: 422 }); - } - }, - async addLabels(params) { calls.push(['add', params.issue_number, params.labels]); }, - }, - }, - }; +test('review: a labelled PR is skipped unless CI Steward pushed', () => { const context = eventContext('review'); - const core = { info: message => calls.push(['info', message]) }; - await vm.runInNewContext(`(async () => {\n${match[1].replace(/^ /gm, '')}\n})()`, { - github, context, core, process: { env: { REVIEWED_SHA: 'head-sha' } }, - }); - return JSON.parse(JSON.stringify(calls)); -} - -test('review: creates the droid-reviewed label when the repository lacks it', async () => { - assert.deepEqual(await runLabelStep(false), [ - ['get', 'droid-reviewed'], ['create', 'droid-reviewed'], ['add', 42, ['droid-reviewed']], - ]); - assert.deepEqual(await runLabelStep(true), [ - ['get', 'droid-reviewed'], ['add', 42, ['droid-reviewed']], - ]); -}); - -test('review: labels the PR when another review created the label first', async () => { - assert.deepEqual(await runLabelStep('race'), [ - ['get', 'droid-reviewed'], ['create', 'droid-reviewed'], ['get', 'droid-reviewed'], - ['add', 42, ['droid-reviewed']], - ]); + context.payload.pull_request.labels = [{ name: 'droid-reviewed' }]; + assert.equal(jobCondition('review', context), false); + context.payload.sender = { login: 'factory-droid[bot]', type: 'Bot' }; + assert.equal(jobCondition('review', context), true); + context.payload.pull_request.labels = [{ name: 'enhancement' }]; + context.payload.sender = { login: 'writer', type: 'User' }; + assert.equal(jobCondition('review', context), true); }); -test('review: fails when label creation is rejected and the label is still missing', async () => { - await assert.rejects(runLabelStep('invalid'), { status: 404 }); +test('review: a successful review labels the PR', () => { + const steps = jobSteps('review', 'review'); + const last = steps[steps.length - 1]; + assert.match(last, /name: Mark the PR as reviewed/); + assert.match(last, /labels: \['droid-reviewed'\]/); + assert.doesNotMatch(last, /^ if:/m); }); -test('wiki: generation runs only on the default branch', () => { +test('wiki: runs monthly or by hand, not on push', () => { const wiki = readFileSync(join(__dirname, '../.github/workflows/droid-wiki-refresh.yml'), 'utf8'); - const job = wiki.split('\n wiki-refresh:\n')[1]; - assert.ok(job); - const condition = job.match(/^ if: (.*)$/m); - assert.ok(condition); - for (const [ref, type, expected] of [ - ['main', 'branch', true], ['feature', 'branch', false], ['main', 'tag', false], - ]) { - const value = vm.runInNewContext(condition[1].replaceAll('github.', 'github_.'), { - github_: { ref_name: ref, ref_type: type, event: { repository: { default_branch: 'main' } } }, - }); - assert.equal(value, expected); - } -}); - -test('review: does not label a PR whose head moved during the review', async () => { - const calls = await runLabelStep(true, 'newer-head'); - assert.equal(calls.length, 1); - assert.equal(calls[0][0], 'info'); - assert.match(calls[0][1], /moved during the review/); + assert.match(wiki, /^ schedule:\n - cron: '17 6 1 \* \*'/m); + assert.match(wiki, /^ workflow_dispatch:$/m); + assert.doesNotMatch(wiki, /^ push:/m); }); From 0c0be80c0a4c820f4c0c2c20850c82a56e8b4f1f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 23 Oct 2026 10:55:32 +0000 Subject: [PATCH 12/12] Create the droid-reviewed label before adding it GitHub's add-labels docs do not promise that a missing label is created, so create it first and treat 422 (already exists) as success. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017U3iFTj2nxsbhbdBRHKFzi --- .github/workflows/factory-review.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/factory-review.yml b/.github/workflows/factory-review.yml index 67d243c..5a1da9e 100644 --- a/.github/workflows/factory-review.yml +++ b/.github/workflows/factory-review.yml @@ -137,11 +137,15 @@ jobs: allowed_bots: factory-droid show_full_output: "false" - # Adding a missing label creates it. - name: Mark the PR as reviewed uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 with: script: | + try { + await github.rest.issues.createLabel({ ...context.repo, name: 'droid-reviewed', color: '5319e7' }); + } catch (error) { + if (error.status !== 422) throw error; // 422: the label already exists + } await github.rest.issues.addLabels({ ...context.repo, issue_number: context.payload.pull_request.number,