Skip to content

ADR-0007 schema ruling: is job-level concurrency: permitted in agent-ingress.yml? #2001

Closed
Closed
ADR-0007 schema ruling: is job-level concurrency: permitted in agent-ingress.yml?#2001
Labels
documentationDocumentation changesquestionFurther information is requested

Description

@don-petry
don-petry
opened on Oct 1, 2026

The decision needed

ADR-0007 enumerates the agent-ingress.yml schema as on: / permissions: / jobs: (with uses, with, secrets, if, permissions, name). It does not enumerate concurrency:. A ruling is needed before the pilot collapse can merge.

Raised by the #1729 collapse package — PR #1977, docs/initiatives/agent-ingress-collapse-markets.md §4 — which flagged it rather than deciding it.

Why it comes up

In petry-projects/markets, three of the five collapsing stubs (pr-auto-review.yml, pr-review.yml, ci-failure-analyst.yml) each carry a workflow-level concurrency: block. dev-lead.yml centralizes concurrency inside its reusable and has no caller block.

One collapsed workflow cannot host three distinct workflow-level groups without cross-role cancellation — role A's run would cancel role B's. So §3 of the package renders each as a job-level concurrency: block, which Actions supports and which preserves each role's grouping in isolation.

The two readings

  1. Allow job-level concurrency: as a declarative grouping key — it carries no steps:/run: and reaches for no repo state, so it is not logic under ADR-0001's thin-caller rule. This is what §3 assumes. Dropping it would regress the duplicate-run collapse the source stubs added ([LSP re-pilot placeholder] TypeScript / JavaScript (typescript-language-server / tsgo) — broodly / google-app-scripts / markets #1126).
  2. Forbid it as outside the enumerated schema, forcing concurrency grouping down into each reusable. Viable, but that is a reusable change per role and would gate the pilot on work in four other components.

Package recommendation

Ratify reading (1) with a one-line schema addendum to ADR-0007: concurrency is a declarative grouping key, not logic. Reading (2) is defensible but should not gate the pilot.

Why this blocks

The companion issue §10 ships to markets with a pre-merge gate item: "Confirm the §4 job-level concurrency ruling is ratified." Until this is decided and recorded, the pilot collapse cannot merge — and if it merged on the unratified assumption, a later reading (2) would invalidate the shipped ingress.

Acceptance criteria

  1. The ruling is recorded (ADR-0007 addendum, or a superseding ADR if the answer changes the thin-caller boundary).
  2. If reading (1): the if:-as-event-filter checker and validate-caller-inputs accept a job-level concurrency: block and still reject logic — with a test asserting both.
  3. If reading (2): the per-role reusable changes are enumerated as follow-ups and [Phase 3] Collapse the first repo's Class-1 event-driven stubs into agent-ingress.yml and rename the required checks #1729's §3 is corrected before the companion issue is filed.
  4. The decision is linked from [Phase 3] Collapse the first repo's Class-1 event-driven stubs into agent-ingress.yml and rename the required checks #1729 so the markets gate item can cite it.

Activity

  1. don-petry
    added
    needs-human-reviewFlagged by automated PR review agent
    on Oct 1, 2026

  2. don-petry commented on Oct 1, 2026

    @don-petry
    don-petry
    on Oct 1, 2026
    CollaboratorAuthor

    @petry-projects/solution-architect measure this against our ADRs and rule on it.

    The specific question: does ADR-0007's enumerated agent-ingress.yml schema permit a job-level concurrency: block, or does omitting it from the enumeration forbid it?

    What I would like from you, in order:

    1. A ruling — reading (1) allow, or reading (2) forbid — stated plainly.
    2. Grounded in the ADR boundary, not convenience: ADR-0001's thin-caller rule is that the caller tier holds no logic, and ADR-0007 loosens it only so a job-level if: may act as a pure event filter. Does a concurrency: group key sit inside that boundary (a declarative grouping key with no steps:/run: and no repo-state reach), or does enumerating the schema make the list exhaustive by construction?
    3. The consequence you weigh — forbidding it pushes concurrency into four reusables and gates the pilot; allowing it sets a precedent that the enumeration is permissive rather than exhaustive, which affects every future schema question, not just this one.
    4. The recorded form — an ADR-0007 addendum, or a superseding ADR if the answer moves the thin-caller boundary.

    Context is in the issue body above; the rendered ingress is §3 of docs/initiatives/agent-ingress-collapse-markets.md on PR #1977, with the if: provenance table in §3a.

    Please do not assume the package's recommendation is correct — it is the proposer's reading, and the precedent question in (3) is the part it does not weigh.

  3. don-petry commented on Oct 1, 2026

    @don-petry
    don-petry
    on Oct 1, 2026
    CollaboratorAuthor

    dev-lead is withholding action on this item.

    It is labeled needs-human-review (flagged for human review — this label is applied by automation as well as by people, so an item can become held without anyone noticing), so dev-lead will not pick it up while that label is present. This notice is posted once so the withhold is visible rather than looking like a stalled run.

    To re-enable automated pickup: remove the needs-human-review label.

  4. github-actions
    added
    questionFurther information is requested
    documentationDocumentation changes
    on Oct 1, 2026

  5. github-actions commented on Oct 1, 2026

    @github-actions
    github-actionsBot
    on Oct 1, 2026 – with GitHub Actions
    Contributor

    Thanks for writing this up so thoroughly, and for flagging the ADR-0007 schema gap before the pilot collapse merged. Classifying this as a question about the ADR plus a documentation follow-up (the addendum). Before a ruling can be recorded: is there any case where a job-level concurrency: block in agent-ingress.yml would need cancel-in-progress or a group expression that reads event or repo state (e.g. github.event.* context), or are all the groups in the markets stubs static or simple github.ref-style keys? That answer determines whether 'declarative grouping key, not logic' holds cleanly for the checker and validate-caller-inputs.

  6. don-petry
    removed
    needs-human-reviewFlagged by automated PR review agent
    on Oct 1, 2026

  7. don-petry commented on Oct 1, 2026

    @don-petry
    don-petry
    on Oct 1, 2026
    CollaboratorAuthor

    Answering the clarifying question with the actual rendered blocks, since it decides the ruling.

    No — none of the three is a static or github.ref-style key. All three read github.event.*, and two use conditional expressions. Verbatim from §3 of the package:

    # pr-auto-review
concurrency:
  group: >-
    ${{ (github.event_name == 'pull_request'
         && format('pr-auto-review-ready-check-pr-{0}', github.event.pull_request.number)) … }}

# pr-review
concurrency:
  group: >-
    pr-review-${{ (github.event.pull_request && github.event.pull_request.number) … }}

# ci-failure-analyst
concurrency:
  group: "ci-failure-analyst-${{ github.event.check_run.head_sha }}"
  cancel-in-progress: false

    So one block does set cancel-in-progress (explicitly false, preserving the source stub's behavior), and every group is event-payload-derived.

    Why I read that as supporting (1) rather than defeating it: ADR-0007's loosening for job-level if: already permits exactly this surface — github.event_name, github.event.action, and payload predicates — and draws its line at repo state ("anything reaching for repo state is logic and stays in the reusable"). None of these three reaches repo state: no API call, no label or ruleset lookup, no needs. context, no step output. The two conditionals select a group name per event shape, which is the same payload-predicate shape the ADR sanctions for if:.

    That suggests the ruling is narrower and more checkable than "allow concurrency:":

    A job-level concurrency: block is permitted, and its group expression is restricted to the same event-payload surface already permitted for if: — no repo-state reach, no needs./step outputs, no API calls. cancel-in-progress is a literal boolean.

    That keeps the enumeration permissive-but-bounded rather than open-ended, and it is enforceable: the #1725 if:-as-event-filter checker already implements exactly that predicate test and could be pointed at concurrency.group with no new logic.

    @petry-projects/solution-architect — with that evidence, please issue the ruling and name the recorded form (ADR-0007 addendum vs superseding ADR). The precedent question still stands and is the part I would most like your judgment on: does enumerating a schema make the list exhaustive by construction, or permissive-with-a-boundary? The answer governs every future ingress schema question, not just this one.

  8. don-petry commented on Oct 1, 2026

    @don-petry
    don-petry
    on Oct 1, 2026
    CollaboratorAuthor

    Solution Architect — structural advisory

    Governing ADR: ADR-0007 (One agent-ingress stub per repo), read with ADR-0001 (thin-caller boundary) and ADR-0000 (ADR process).

    Alignment: drifts, but the fix is small. ADR-0007's Decision says "The allowed schema is:" and lists the keys, then forbids "any logic outside the if: guard". The conditional concurrency.group expressions are logic outside the if: guard, so reading (1) needs a new recorded decision. Calling it an interpretation of ADR-0007 isn't enough.

    What I'd shore up (highest leverage first):

    • Precedent: an enumerated schema is exhaustive, because it is an allowlist. The freeze and caller-input tooling can only enforce a closed list. If the list were open with a boundary, the next keys someone wants (strategy:, env:, continue-on-error:) would each have to be argued case by case, and strategy.matrix is plainly logic. Each new key costs one ADR. ADR-0000 says that ceremony is the point.
    • Recorded form: a new ADR-0010. ADR-0000 makes accepted ADRs immutable, so an addendum to ADR-0007 isn't an option. ADR-0010 should extend ADR-0007 without superseding it, the same way ADR-0007 extended ADR-0001. ADR-0000 has no "amended-by" status, so note that gap in ADR-0010's Context.
    • Substance: allow it, with your bounded wording. group is limited to the payload surface already allowed in if:, and cancel-in-progress must be a literal boolean. This follows ADR-0007's own reason for one job per role: per-role state can't be merged into one shared value.
    • Add a collision rule: each group must carry the role name as a prefix and must not match any group the pinned reusable declares. Otherwise nested caller and reusable groups can block or cancel each other. The [Phase 2] Make caller_stub_freeze and validate-caller-inputs ingress-aware (job-scoped freeze + if:-as-event-filter check) #1725 checker should test this too.

    Escalate? no. This needs a human to accept the ADR; nothing here is unsafe.

    Advisory only — I comment, I do not change code. Ground: BMAD Method architect
    (bmad-agent-architect v6.8.0). Opt out on this item with the
    solution-architect:hands-off label.

  9. don-petry commented on Oct 1, 2026

    @don-petry
    don-petry
    on Oct 1, 2026
    CollaboratorAuthor

    Ruling received and accepted as the direction. Implementation filed as #2002 (labeled dev-lead), carrying all four decision elements as ACs: the bounded group surface, cancel-in-progress as a literal boolean, the role-prefix + no-reusable-collision rule, and the exhaustive-allowlist precedent with strategy:/env:/continue-on-error: explicitly not admitted. ADR-0010 will be authored proposed — human acceptance is the gate, per the advisory.

    Two corrections to my framing above, for the record: an ADR-0007 addendum is not available (ADR-0000 makes accepted ADRs immutable), and "permissive-with-a-boundary" was the wrong reading — the enumeration is an allowlist because the freeze and caller-input tooling can only enforce a closed list. The collision rule was not in my proposal at all and is a live case, since dev-lead centralizes concurrency inside its reusable.

    This issue stays open until ADR-0010 is accepted, so the #1729 §8a gate item has something ratified to cite.

  10. don-petry commented on Oct 10, 2026

    @don-petry
    don-petry
    on Oct 10, 2026
    CollaboratorAuthor

    Answered. The ruling (job-level concurrency: is permitted, bounded) was recorded as ADR-0010 (issue #2002; ADR accepted in #2036). It is in use in the markets collapse (combsmith/markets#513, merged 2026-10-10). Closing as completed.

  11. don-petry
    closed this as completedon Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationDocumentation changesquestionFurther information is requested

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions