Skip to content

Deepen the Trusted account rule behind its own module #862

Closed

Description

@JacobStephens2

What to build

Move the Trusted account rule out of the GitHub adapter into a domain module of its own, behind a small reads interface, so that every edge case of the rule can be tested in memory. What users see does not change. Today the rule is spread through the GitHub adapter's gh calls, and the only way to test it is the gh fake. The rule covers:

  • the author check;
  • which ready-for-agent labelling counts;
  • for a Ticket, the later of its own and its Spec's trusted labellings;
  • untrusted edits since a time;
  • an edit whose account GitHub no longer knows;
  • the repository owner shortcut.

Four callers use the rule:

  • the Pickup run's Ready issue search;
  • a Spec run before it starts each Ticket's Run;
  • a direct Run's warning;
  • Delivery's check before each session.

They keep the same verdicts, messages and failure behaviour.

Acceptance criteria

  • A domain trust module owns the rule. Its interface is three checks: issue trust, Ticket trust (Ticket plus its Spec), and the first untrusted edit since an optional time. It also owns the UntrustedIssue and IssueText types and the authorAssociation test that session issue hand-over shares.
  • The module reads GitHub only through a Reads interface with four reads:
    • the author's association;
    • the issue's ready-for-agent labellings, oldest first, each with an optional actor and a time;
    • the issue's edits of one IssueText, each with an optional actor and a time;
    • whether a login is a collaborator on the repository or a member of the organization that owns it.
  • The GitHub adapter implements Reads and holds only queries, paging, parsing and the membership status reading (204 means yes, 404 means no, anything else is an error). It no longer decides trust.
  • The rule's order and laziness are kept: the author first, then the latest labelling, then edits, body before title, stopping at the first failure. The owner check is case-insensitive and needs no read. A login once found trusted is not looked up again within one check.
  • The four callers call the trust module with the GitHub adapter. The Ready issue search keeps its own Reads::issue_trust, which forwards to the module, so its in-memory tests stay as they are.
  • Inline unit tests use an in-memory Reads adapter and cover:
    • an untrusted author;
    • no labelling at all;
    • a latest labelling with no actor;
    • a latest labelling by an untrusted actor, even after an earlier trusted one;
    • a latest trusted labelling that supersedes an earlier untrusted one;
    • edits at or before the labelling, which are ignored;
    • an edit after it, by a trusted account, by an untrusted account, or by an unknown account;
    • a title edit as well as a body edit;
    • a Ticket that uses the later of its own and its Spec's trusted labellings, or every edit when neither has one;
    • the case-insensitive owner;
    • no repeated membership read for the same login;
    • a read error, which propagates and never grants trust.
  • Integration tests that only vary the rule move to those unit tests. Two kinds stay:
    • one test per caller showing the wiring (Pickup passes an untrusted issue over, a Spec run stops before an untrusted Ticket, Delivery stops on an untrusted edit, a direct Run warns and still delivers);
    • the tests that cover adapter reading (paging, an unknown editor's null login, a membership failure).
  • docs/code-navigation.md's "Trusted account" row names the trust module as the owner and the GitHub adapter's trust reads as the adapter.
  • cargo test passes, and cargo clippy and cargo fmt --check are clean.

Blocked by

None (can start immediately)

Decisions

  • Settle the rule in a domain module, not the adapter. The rule is security-critical domain policy, and today it is woven through gh calls with no unit tests. Moving it concentrates the knowledge in one module (locality) and gives four callers one interface (leverage).
  • Use a Reads interface with two adapters, GitHub and in-memory, rather than gathering every fact up front. Early returns and per-login membership lookups stay lazy: no events or edits are read for an untrusted author, and no title edits are read after a failing body edit. That keeps today's gh traffic and the order of the checks. The two adapters make this a real seam, not a hypothetical one.
  • Read edits per IssueText (body, then title). This keeps the existing short-circuit and keeps the two GraphQL queries apart in the adapter.
  • The owner shortcut and the trusted-login cache belong to the rule. The membership endpoints and the organization lookup belong to the adapter. The owner comes from the issue URL and is a rule fact. Which endpoints prove membership is a GitHub fact.
  • The cache lasts for one top-level check. Today it lasts only within one edit scan. Widening it to the whole check changes no verdict, saves repeated lookups and gives the rule one place to remember trusted logins.
  • Keep the GitHub side's file as the adapter's trust reads. Its queries and the membership reading stay there, so history follows the adapter code.
  • Keep the Ready issue search's own Reads::issue_trust. Its in-memory search tests depend on that seam. Only its GitHub implementation changes.
  • Leave the Pickup run's Claim history out of scope. It reads label history from the GraphQL timeline, while trust reads it from REST events. Unifying those reads is a separate change with its own paging risks.
  • Replace tests rather than layer them. Rule variants move to unit tests at the module's interface. Integration tests stay only for each caller's wiring and the adapter's reading.
  • No GLOSSARY.md or ADR change. "Trusted account" already names the concept, and the move is easy to reverse.

Architecture review

Strength: Strong · Dependencies: mock

Files: src/github/trust.rs, src/github.rs, src/github/session_issue.rs, src/ready.rs, src/run.rs, src/spec_run.rs, src/delivery.rs, docs/code-navigation.md

Before

flowchart LR
  P[Ready issue search] --> G
  S[Spec run] --> G
  R[Run start] --> G
  D[Delivery] --> G
  subgraph G[GitHub adapter]
    direction LR
    rule[Trusted account rule] -.-> gh[gh calls]
  end
  F[gh fake] --> G
  classDef leak stroke:#dc2626,stroke-width:2px;
  class rule leak
Loading

After

flowchart LR
  P[Ready issue search] --> T
  S[Spec run] --> T
  R[Run start] --> T
  D[Delivery] --> T
  T[Trusted account module] --> Reads((Reads))
  Reads --> G[GitHub adapter]
  Reads --> M[In-memory adapter]
  classDef deep stroke-width:3px;
  class T deep
Loading

Problem: the Trusted account rule lives inside the GitHub adapter, mixed in with gh calls, so its edge cases can only be tested through the gh fake.

Solution: a domain trust module owns the rule over a four-read Reads interface, with the GitHub adapter and an in-memory adapter behind it.

Wins

  • Locality: the trust rule sits in one module
  • Tests hit one interface, in memory
  • The adapter shrinks to reads and parsing
  • Leverage: four callers, one rule
  • Integration tests keep only the wiring

Activity

  1. added
    needs-triageMaintainer needs to evaluate this issue
    ready-for-agentFully specified, ready for an AFK agent
    architect-planAn Architect plan: the Spec or Ticket an Architecture review published
    in-progressA Claim: thirdshift has taken this issue
    and removed
    needs-triageMaintainer needs to evaluate this issue
    ready-for-agentFully specified, ready for an AFK agent
    on Oct 20, 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

    architect-planAn Architect plan: the Spec or Ticket an Architecture review published

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions