Skip to content

Harden issue-body auth association fallback#372

Draft
sepo-agent-app[bot] wants to merge 4 commits into
mainfrom
agent/implement-issue-358/codex-26677788726
Draft

Harden issue-body auth association fallback#372
sepo-agent-app[bot] wants to merge 4 commits into
mainfrom
agent/implement-issue-358/codex-26677788726

Conversation

@sepo-agent-app

Copy link
Copy Markdown

Summary

  • Retry weak issues event author associations before route policy runs.
  • Fall back to repository permission lookup and promote non-none permissions to COLLABORATOR.
  • Add diagnostics for raw, refreshed, permission fallback, and final associations.
  • Cover promotion, fallback failure, strong comment behavior, and true unauthorized NONE policy rejection.

Tests

  • npm --prefix .agent run build
  • node --test .agent/dist/__tests__/extract-context-cli.test.js
  • node --test .agent/dist/__tests__/triage.test.js
  • node --test .agent/dist/__tests__/docs-validation.test.js

Closes #358

@sepo-agent-app sepo-agent-app Bot added the agent Handled by the agent label May 30, 2026
@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

Comment thread .agent/src/__tests__/extract-context-cli.test.ts
@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

Comment thread .agent/src/cli/extract-context.ts Outdated
@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

Comment thread .agent/src/cli/extract-context.ts Outdated
@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

This comment has been minimized.

@sepo-agent-app

Copy link
Copy Markdown
Author

Sepo pushed fixes for this PR. Branch: agent/implement-issue-358/codex-26677788726. Requested by @sepo-agent.

  • Parsed repository permission responses with both permission and role_name, so role_name=triage now promotes weak requesters even when legacy permission=read.
  • Kept bare public permission=read untrusted.
  • Added regression coverage for permission=read plus role_name=triage and updated access-policy docs to match.
  • Verified with npm --prefix .agent run build, node --test .agent/dist/__tests__/extract-context-cli.test.js, node --test .agent/dist/__tests__/triage.test.js, and node --test .agent/dist/__tests__/docs-validation.test.js.

@sepo-agent-app

sepo-agent-app Bot commented May 30, 2026

Copy link
Copy Markdown
Author

Sepo is dispatching follow-up automation.

Source Next Target Round Status
fix-pr review PR #372 9 / 40 Dispatched

Reason: agent planner selected review: fix-pr succeeded and pushed the targeted triage role permission parser fix for open PR #372; the next useful automatic step is review of the updated PR head.

@sepo-agent-app

Copy link
Copy Markdown
Author

Rubrics Review

Total Score Verdict Rubrics Scored
100 PASS 7
Dimension Rubric Result Score Evidence
coding_workflow / generic Read docs and linked context first pass 6/6 PR #372 closes #358 and follows the linked issue plan: retry weak issue-body association, fall back to repo permission, add diagnostics, and preserve route policy.
coding_workflow / generic Reuse existing code pass 8/8 The change stays in .agent/src/cli/extract-context.ts, reusing existing event context, dispatch policy, and GitHub helper patterns instead of adding workflow-level auth logic.
coding_workflow / generic Surgical changes pass 8/8 The diff is limited to auth extraction, focused extract-context/triage regressions, and the matching access-policy documentation.
coding_style / generic Prefer simple implementations pass 7/7 The implementation uses small helpers, bounded retry constants, and direct permission parsing for permission/role_name without adding a broader auth subsystem.
coding_workflow / generic Separate decision gates from action authorization pass 8/8 Association normalization happens before dispatch policy, while applyDispatchPolicy still rejects true unauthorized NONE for explicit /implement.
coding_workflow / generic Surface stateful failures pass 7/7 The PR logs raw event association, issue refresh attempts/errors, permission fallback results, and final normalized association.
coding_workflow / generic Keep docs in sync pass 7/7 .agent/docs/customization/access-policy.md now documents the permission endpoint, trusted permission levels, public read exclusion, and issue-body retry behavior.

Notes

  • The current PR head includes the follow-up fixes from prior review rounds: stale-then-fresh retry coverage, public read preservation, and role_name=triage promotion.
  • Regression coverage now matches the requested narrow auth-hardening path, including retry resolution, permission fallback success/failure, strong comment fast path, and route-policy rejection for real unauthorized NONE.
  • No rubric-specific follow-up needed.

Findings

  • INFO: No blocking or warning findings against the applicable selected rubrics.

Final Rubric Verdict

PASS

@sepo-agent-app

Copy link
Copy Markdown
Author

AI Review Synthesis

Dual-agent review by Claude and Codex.
Requested by @lolipopshock.

Summary of PR/Issue

  • PR Harden issue-body auth association fallback #372 addresses issue Why is user auth still buggy? #358 by hardening Sepo’s weak issue-body mention authorization path.
  • GitHub can initially report stale weak author_association values on issues events, so the PR adds bounded live issue retries before route policy runs.
  • It also falls back to the repository permission endpoint, trusts triage, write, maintain, and admin, and continues rejecting public read alone.
  • The branch keeps route authorization unchanged, adds diagnostics, and updates tests/docs for the retry and permission fallback behavior.

Review

The current review artifacts support shipping the PR with no required branch changes. Claude noted one optional test-shape cleanup, while Codex and the rubrics review found no blocking or warning issues; synthesis treats that note as non-required because the dedicated role_name=triage regression covers the production path.

Issue Severity Description
No actionable issues INFO Current reviewers agree the earlier retry, public-read, and triage-role findings are fixed on PR head 66b199bb7e7e0db7cbb0d0b230245c3c99dd6634. No required branch-change work remains.

Progress

  • Earlier retry-resolution coverage is present.
  • Earlier public read over-promotion is fixed and covered.
  • Earlier role_name=triage masking is fixed and covered.
  • Resolved the old same-agent inline review thread PRRT_kwDOSOhEMM6F2zFE.
  • No new inline comments, replies, or minimizations were added.
  • Reviewer artifacts report build, extract-context tests, triage tests, and docs validation passing on the current PR head.

Recommended Next Step

NO_AUTOMATED_ACTION: No unresolved actionable branch changes remain; any remaining PR state decisions are maintainer/workflow metadata choices.

Final Verdict

SHIP

Action Items

  • No required branch-change work.

@lolipopshock

Copy link
Copy Markdown
Contributor

@sepo-agent /answer can you summarize what does this PR do? and actually test if this makes sense?

@sepo-agent-app

Copy link
Copy Markdown
Author

This PR addresses #358: an issue-body @sepo-agent /implement could be denied because GitHub initially reported the issue author as author_association: NONE, even when the same user later showed up correctly as COLLABORATOR on an issue comment.

What it changes:

  • Adds bounded retry for weak issue-body associations before route policy runs.
  • Falls back to GET /repos/{owner}/{repo}/collaborators/{username}/permission.
  • Promotes only trusted repo roles: triage, write, maintain, admin.
  • Explicitly does not trust public read alone, which is important for public repos.
  • Adds diagnostics for raw/refreshed/fallback/final association decisions.
  • Keeps route policy intact: a real unauthorized NONE /implement is still rejected.
  • Updates docs to match the new behavior.

Key code/docs:

  • Auth normalization:
    const TRUSTED_REPOSITORY_PERMISSIONS = new Set([
    "admin",
    "maintain",
    "write",
    "triage",
    ]);
    const ISSUE_ASSOCIATION_REFRESH_ATTEMPTS = 3;
    const ISSUE_ASSOCIATION_REFRESH_BACKOFF_MS = 100;
  • Permission parsing/fallback:
    function getRepositoryPermission(userLogin: string): RepositoryPermissionLookup {
    const login = String(userLogin || "").trim();
    if (!repository || !login) {
    return {
    login,
    permission: "",
    roleName: "",
    allowed: false,
    error: false,
    skippedReason: !repository ? "missing repository" : "missing login",
    };
    }
    try {
    const parsed = parseRepositoryPermissionResponse(gh([
    "api",
    `repos/${repository}/collaborators/${login}/permission`,
    "--jq",
    '{permission: (.permission // ""), role_name: (.role_name // "")}',
    ]));
    return {
    login,
    permission: parsed.permission,
    roleName: parsed.roleName,
    allowed: isTrustedRepositoryPermission(parsed.permission, parsed.roleName),
    error: false,
    skippedReason: "",
    };
    } catch {
    return {
    login,
    permission: "",
    roleName: "",
    allowed: false,
    error: true,
    skippedReason: "",
    };
    }
    }
  • Issue retry path:
    function refreshIssueAssociationWithRetry(
    association: string,
    issueNumber: string,
    ): string {
    if (
    eventName !== "issues" ||
    !repository ||
    !issueNumber
    ) {
    return normalizeAssociation(association) || association;
    }
    let resolved = normalizeAssociation(association) || association;
    for (let attempt = 1; attempt <= ISSUE_ASSOCIATION_REFRESH_ATTEMPTS; attempt += 1) {
    const refreshed = getIssueAssociation(issueNumber);
    logMentionAssociationDiagnostic(
    `refreshed issue association attempt ${attempt}/${ISSUE_ASSOCIATION_REFRESH_ATTEMPTS}: ${refreshed.association || "empty"}${refreshed.error ? " error=true" : ""}`,
    );
    resolved = refreshed.association || resolved;
    if (!isWeakMentionAssociation(resolved)) {
    return resolved;
    }
    if (attempt < ISSUE_ASSOCIATION_REFRESH_ATTEMPTS) {
    sleep(ISSUE_ASSOCIATION_REFRESH_BACKOFF_MS);
    }
    }
    return resolved;
  • Docs:
    ## Weak association normalization
    For mention triggers, the runtime trusts strong `author_association` values (`OWNER`, `MEMBER`, and `COLLABORATOR`) without another lookup. When GitHub reports a weaker value such as `NONE`, `CONTRIBUTOR`, `FIRST_TIMER`, or `FIRST_TIME_CONTRIBUTOR`, Sepo checks the triggering actor with `GET /repos/{owner}/{repo}/collaborators/{username}/permission` and treats `triage`, `write`, `maintain`, or `admin` from either `permission` or `role_name` as `COLLABORATOR` before route authorization. Public `read` permission alone is not trusted because it can apply to non-collaborators on public repositories. This applies to all supported mention surfaces, including issue and pull request bodies, discussion bodies and comments, issue comments, pull request review comments, and pull request reviews.
    Issue-body mentions from `issues` events also retry `author_association` from the live issue API a few times before the permission fallback. These checks cover cases where repo-scoped tokens cannot see private org membership through webhook `author_association`, but GitHub author association remains token- and visibility-dependent. The public default allowlist therefore still includes `CONTRIBUTOR` unless a repository configures a stricter `AGENT_ACCESS_POLICY`.

I tested PR head 66b199bb7e7e0db7cbb0d0b230245c3c99dd6634:

  • npm --prefix .agent run build passed
  • node --test .agent/dist/__tests__/extract-context-cli.test.js passed, 32/32
  • node --test .agent/dist/__tests__/triage.test.js passed, 45/45
  • node --test .agent/dist/__tests__/docs-validation.test.js passed, 4/4

I also ran the full .agent suite. It got 701/702 passing, with one deterministic failure in an unrelated hosted OIDC test:

  • hosted OIDC exchange retries transient broker HTTP responses
  • expected retry calls: 3
  • actual: 1
  • file: .agent/dist/__tests__/resolve-github-auth-oidc-shell.test.js

That failure is outside this PR’s touched auth-association files, so I would not treat it as evidence that this PR’s change is wrong. Based on the diff and focused regressions, the PR makes sense and covers the important edge cases, including stale retry resolution, public read not being over-trusted, role_name=triage, permission lookup failure, and true unauthorized NONE rejection.


codex | gpt-5.5/medium | high | Macmini-runner-5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Handled by the agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Why is user auth still buggy?

2 participants