diff --git a/.github/workflows/agent-mention-router.yml b/.github/workflows/agent-mention-router.yml index f14667a93..b922ba5ab 100644 --- a/.github/workflows/agent-mention-router.yml +++ b/.github/workflows/agent-mention-router.yml @@ -6,10 +6,6 @@ on: schedule: - cron: "*/5 * * * *" -concurrency: - group: review-agent-mention-router-${{ github.repository }} - cancel-in-progress: false - # Organization required-workflow rules do not propagate issue_comment events # into sibling repositories. Keep the workflow default read-only; each bounded # job declares only the writes it actually needs. @@ -28,6 +24,9 @@ jobs: contains(github.event.comment.body, '@cwl-noema-review') || contains(github.event.comment.body, '@opencode-agent') ) + concurrency: + group: review-agent-mention-router-local-${{ github.repository }} + queue: max runs-on: ubuntu-24.04 timeout-minutes: 5 permissions: @@ -70,6 +69,9 @@ jobs: if: >- github.repository == 'ContextualWisdomLab/.github' && github.event_name == 'schedule' + concurrency: + group: review-agent-mention-router-sweep-${{ github.repository }} + cancel-in-progress: false runs-on: ubuntu-24.04 timeout-minutes: 15 permissions: diff --git a/CHANGELOG.md b/CHANGELOG.md index bf30091dd..7901d7655 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Isolated trusted interactive review-agent mentions from scheduled organization sweeps with separate job-level concurrency groups; interactive requests use the bounded 100-pending `queue: max` contract while sweeps retain non-cancelling single-pending coalescing, leaving the durable exact-name artifact ledger as forwarding authority. - Bounded the Strix quality self-test's deterministic timeout fixtures to 3-second process and 5-second fake-sleep budgets so exact-head policy evidence completes inside the existing job limit without changing production Strix scanner timeouts, providers, credentials, or review semantics. - Allowed commas and ASCII parentheses in the bounded Strix changed-file path policy so legal tracked Packrat fixtures can receive exact-head security analysis, while rejecting raw `..` components before normalization and keeping controls, backslashes, whitespace ambiguity, and shell punctuation fail-closed. - Bound each review-agent invocation key to the wrapper's complete canonical payload, including the base branch and requesting actor; altered fields with a valid-format key now fail before durable-leader election or forwarding, and wrapper write permission is job-scoped. diff --git a/docs/doctoring/agent-mention-concurrency-isolation.md b/docs/doctoring/agent-mention-concurrency-isolation.md new file mode 100644 index 000000000..5dbf0a36f --- /dev/null +++ b/docs/doctoring/agent-mention-concurrency-isolation.md @@ -0,0 +1,124 @@ +# Review-agent mention concurrency isolation + +검토 기준일: **2026-08-07** + +## Incident + +Trusted `@cwl-noema-review` and review-only `@opencode-agent` comments can remain unacknowledged even though the protected-default-branch router is enabled. The failure occurs before model execution: the central workflow mixes two event classes in one workflow-level concurrency group. + +- interactive `issue_comment` routing has a five-minute job timeout; +- the organization-wide sweep is scheduled every five minutes and has a fifteen-minute job timeout. + +GitHub Actions documents that a concurrency group permits one running member. With the default `queue: single`, at most one additional run can be pending; a newer queued run replaces the existing pending run even when `cancel-in-progress` is false. A scheduled sweep can therefore replace a pending trusted comment before exact-head resolution, durable ledger claim, dispatch, or acknowledgement. + +This is a queue-configuration defect, not evidence that the model, credential, allowlist, or review result is invalid. + +## Fail-first evidence + +Direct-main replacement PR #825 starts from protected `main` `1131b1bbafb24e455fc8619cdf316813e8721861`. Exact RED head `a319d513a2f67b707737651a9eb7fdbfe4bc23c4` changed only `tests/test_agent_mention_workflow_contract.py` and required separate job-scoped queue contracts while the inherited workflow still had one shared workflow-level group. + +This replacement does not reuse predecessor PR #815 or stacked development PR #824 checks, reviews, approvals, or mergeability evidence. + +## Decision + +Move concurrency from the workflow to the two jobs and give each event class a separate group. + +```yaml +route-local-agent-mention: + concurrency: + group: review-agent-mention-router-local-${{ github.repository }} + queue: max + +sweep-organization-agent-mentions: + concurrency: + group: review-agent-mention-router-sweep-${{ github.repository }} + cancel-in-progress: false +``` + +GitHub currently documents `queue: max` as allowing up to 100 pending jobs or workflow runs in a concurrency group. Waiting members are processed serially; runs beyond the queue limit are rejected. GitHub also documents that `queue: max` cannot be combined with `cancel-in-progress: true`. + +The interactive route therefore retains every bounded pending trusted comment up to the platform queue limit instead of replacing the previous pending request. The sweep keeps the default single-pending behavior: a running sweep is not interrupted, but obsolete pending sweeps may coalesce. Local routes and scheduled sweeps use different concurrency groups, so scheduled work cannot replace interactive work. + +Concurrency is not the durable idempotency authority. Duplicate forwarding remains governed by the canonical invocation key, exact-key downstream concurrency, and immutable exact-name Actions artifact ledger written before authoritative forwarding. + +## Data and authority flow + +```mermaid +sequenceDiagram + participant M as Trusted maintainer + participant L as Local comment queue + participant S as Scheduled sweep queue + participant R as Central router + participant D as Exact-key downstream dispatcher + participant A as Durable artifact ledger + participant V as Review workflow + + M->>L: issue_comment exact mention + S->>R: bounded organization sweep + L->>R: ordered interactive request + R->>D: canonical invocation payload + SHA-256 key + D->>A: claim exact ledger name + alt first live claim + D->>V: forward once + R-->>M: receipt / acknowledgement + else existing claim + D-->>R: duplicate suppressed + end +``` + +A receipt proves routing/claim processing occurred. It is not an approval and does not weaken exact-head checks, branch protection, or expected-head merge rules. + +## Preserved security and privacy boundaries + +- No model provider, reviewer identity, token name, secret, repository allowlist, dispatch payload, or permission changes. +- `COPILOT_GITHUB_TOKEN` remains unused. +- Workflow-default permissions remain `contents: read`; only existing job-scoped writes remain. +- The local route still accepts only non-bot `OWNER`, `MEMBER`, or `COLLABORATOR` comments on pull requests in the central repository. +- The sweep retains the configured organization-token / OpenCode installation-token credential chain. +- Pull-request number, base branch, base SHA, current head SHA, requesting actor, source comment identifier, and requested agent remain bound into the canonical invocation key. +- The exact-name Actions artifact ledger remains the authority for idempotent forwarding. +- The ledger keeps the existing **30-day artifact retention** and contains bounded invocation metadata, not comment bodies, model output, credentials, or business payloads. + +The privacy alternative to masking is separation and minimization: this automation does not require business PII. It processes bounded GitHub control-plane metadata under repository authorization rather than copying business records into model prompts or artifacts. + +## CSAP / SOC 2 readiness evidence + +This repair improves availability and processing-integrity evidence without claiming certification. + +| Control concern | Evidence | +| --- | --- | +| Change management | Protected pull request and exact-head checks; qualifying independent review and post-integration evidence remain pending | +| Availability | Separate local/sweep groups, bounded job timeouts, bounded interactive queue | +| Processing integrity | Canonical invocation key, exact-name artifact claim, duplicate suppression | +| Least privilege | Existing job-scoped permissions and credential separation remain unchanged | +| Monitoring | Queue delay, receipt delay, sweep duration, dispatch count, duplicate-claim outcome | +| Incident response | Fail-first contract, this doctoring record, rollout/rollback criteria | +| Privacy | Metadata-only routing; no business payload or credential retained in ledger | + +## Monitoring and acceptance + +After protected merge: + +1. create a fresh exact-head `@opencode-agent` and/or `@cwl-noema-review` request; +2. require a durable receipt or acknowledgement before relying on downstream review evidence; +3. verify that scheduled sweep runs do not cancel or replace pending interactive routes; +4. monitor local queue delay, sweep duration, dispatch count, duplicate-ledger outcomes, and downstream conclusions; +5. alert when an eligible interactive request has no receipt within **10 minutes** of comment creation (a CWL operational alert threshold, not a GitHub SLA): this permits at most five minutes of queue delay plus the existing five-minute local execution timeout before operator investigation; +6. alert immediately on a queue-limit rejection, unexpected cancellation of an interactive route, or when the interactive queue approaches the documented 100-pending limit; +7. keep metrics finite-cardinality and exclude comment text, source diffs, tokens, and model responses. + +A downstream reviewer may still fail closed because credentials, providers, checks, or exact-head evidence are unavailable. That remains distinct from a routing queue failure. + +## Rollback + +Rollback must preserve interactive requests. Restoring the shared workflow-level group is not acceptable. A safe emergency degradation is to suspend the scheduled sweep while retaining the isolated local queue. Removing `queue: max` from the local group requires another independently reviewed durable queue that preserves every eligible invocation. + +## References (APA 7th) + +GitHub. (n.d.). *Concurrency*. GitHub Docs. Retrieved August 7, 2026, from https://docs.github.com/en/actions/concepts/workflows-and-actions/concurrency + +GitHub. (n.d.). *Control the concurrency of workflows and jobs*. GitHub Docs. Retrieved August 7, 2026, from https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency + +GitHub. (n.d.). *REST API endpoints for GitHub Actions artifacts*. GitHub Docs. Retrieved August 7, 2026, from https://docs.github.com/en/rest/actions/artifacts + +GitHub. (n.d.). *Store and share data with workflow artifacts*. GitHub Docs. Retrieved August 7, 2026, from https://docs.github.com/en/actions/tutorials/store-and-share-data diff --git a/tests/test_agent_mention_artifact_ledger.py b/tests/test_agent_mention_artifact_ledger.py index c527f4a6e..8eb86b65b 100644 --- a/tests/test_agent_mention_artifact_ledger.py +++ b/tests/test_agent_mention_artifact_ledger.py @@ -14,7 +14,7 @@ MODULE_PATH = ROOT / "scripts" / "ci" / "agent_mention_router.py" NOEMA_WORKFLOW = ROOT / ".github" / "workflows" / "agent-mention-noema-dispatch.yml" OPENCODE_WORKFLOW = ROOT / ".github" / "workflows" / "agent-mention-opencode-dispatch.yml" -DOC = ROOT / "docs" / "automation" / "review-agent-comment-invocation.md" +DOC = ROOT / "docs" / "doctoring" / "agent-mention-concurrency-isolation.md" UPLOAD_ARTIFACT_SHA = "043fb46d1a93c77aae656e7c1c64a875d1fc6a0a" diff --git a/tests/test_agent_mention_workflow_contract.py b/tests/test_agent_mention_workflow_contract.py index c5fc4cae5..7ed9aa71b 100644 --- a/tests/test_agent_mention_workflow_contract.py +++ b/tests/test_agent_mention_workflow_contract.py @@ -10,6 +10,23 @@ CHECKOUT_PIN = "actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1" +def _job_block(workflow: str, job_name: str, next_job_name: str | None) -> str: + """Return one top-level job block bounded by the following job.""" + jobs = workflow.split("\njobs:\n", 1)[1] + start = jobs.index(f" {job_name}:\n") + if next_job_name is None: + return jobs[start:] + end = jobs.index(f"\n {next_job_name}:\n", start) + return jobs[start:end] + + +def _concurrency_block(job: str) -> str: + """Return the exact job-scoped concurrency mapping before ``runs-on``.""" + start = job.index(" concurrency:\n") + end = job.index("\n runs-on:", start) + return job[start:end] + + def test_workflow_uses_local_event_and_central_sweep_with_job_scoped_writes() -> None: """The router is central-only, scheduled, and least-privileged.""" @@ -51,6 +68,31 @@ def test_workflow_uses_local_event_and_central_sweep_with_job_scoped_writes() -> assert "agent_mention_sweep.py" in sweep +def test_interactive_mentions_and_sweeps_have_independent_queue_contracts() -> None: + """Scheduled sweeps cannot replace a pending trusted comment invocation.""" + + text = WORKFLOW.read_text(encoding="utf-8") + header = text.split("\njobs:\n", 1)[0] + local_job = _job_block( + text, + "route-local-agent-mention", + "sweep-organization-agent-mentions", + ) + sweep_job = _job_block(text, "sweep-organization-agent-mentions", None) + + assert not any(line.startswith("concurrency:") for line in header.splitlines()) + assert _concurrency_block(local_job) == ( + " concurrency:\n" + " group: review-agent-mention-router-local-${{ github.repository }}\n" + " queue: max" + ) + assert _concurrency_block(sweep_job) == ( + " concurrency:\n" + " group: review-agent-mention-router-sweep-${{ github.repository }}\n" + " cancel-in-progress: false" + ) + + def test_quality_workflow_measures_exact_files_without_module_name_warnings() -> None: """Coverage includes the two script paths instead of treating paths as modules."""