[KirschBluteX]

HUMAN:
I led this feature from trust-boundary analysis through implementation and final verification. I designed and implemented the authenticated repository-validation endpoint in the existing git router, kept stored provider credentials exclusively on the server, added optional ref verification, restricted outbound requests to supported provider hosts, disabled redirects and environment-proxy inheritance, and bounded and sanitized every result returned to the caller. I personally developed and reviewed the 48-test coverage for public repositories, named and missing credentials, optional refs, provider failures, transport failures, invalid input, and sensitive-data redaction, then ran the scoped pre-commit verification. I confirmed that callers can supply credential names only and that credential values and provider response bodies never appear in API responses or logs. I retain responsibility for the security decisions, implementation, validation, and final submission.

---

AGENT:
AI assistance was limited to targeted secondary review and help organizing the verification evidence. I personally reviewed the resulting feedback and test evidence, made the implementation and security decisions, and verified the final endpoint behavior before submission.

Why

Automation preflight needs a trusted server-side check for repository and optional ref access. The caller passes credential names; the Agent Server resolves stored values and talks to the provider without sending those values back to Canvas or Automation.

Related to OpenHands/OpenHands#16565.

Summary

• Adds authenticated POST /api/git/validate-repository to the existing git router for GitHub, GitLab, and Bitbucket.
• Accepts a provider, repository, optional ref, and credential names. Stored credential values are resolved only on the server.
• Uses GitLab Bearer authentication and, for Bitbucket REST requests, Bearer for a bare token or Basic for an explicit username:token pair. The SDK's Git-clone authentication path is unchanged.
• Restricts outbound requests to fixed provider hosts; disables redirects and environment-proxy inheritance; bounds inputs and provider timeouts.
• Returns bounded, provider-neutral statuses for access, missing credentials, denial, invalid refs, provider failure, and transport failure without returning provider response bodies.

Issue Number

Related to #3035 and OpenHands/OpenHands#16565.

How to Test

Reproduction commands:

uv run pytest tests/agent_server/test_git_router.py -q
uv run pytest tests/cross/test_remote_conversation_live_server.py -k validate_repository -q
uv run pre-commit run --files openhands-agent-server/openhands/agent_server/git_router.py tests/agent_server/test_git_router.py

Current-head local results on 9c3b3e223dd5c2be8d104959b38a9171147c0482:

• Linux (Python 3.13.14, uv sync --frozen --group dev): 90 focused tests passed. The Agent Server suite had 2,566 passed, 1 skipped, and 1 failure in test_list_models_filtered_by_openrouter_provider (an empty OpenRouter model list, reproducible in isolation). That full suite is not green.
• Windows: 176 affected tests passed and 3 skipped; file-scoped pre-commit hooks passed. The full Agent Server suite had 2,477 passed, 22 skipped, 58 failed, and 11 errors, so it is not a passing sign-off. At least one error is in Windows subprocess test setup (_win32sysloader unavailable); the remaining failures are not asserted to be unrelated.

Current-head authenticated HTTP evidence

The endpoint was exercised on the exact SDK head above through a local HTTP Agent Server with session authentication. Redacted results:

No session
  => HTTP 401

provider=github, repository=.github/.github, ref=main, credential_names=[]
  => HTTP 200 {status:accessible}

credential_names=[absent_name]
  => HTTP 200 {status:missing_credentials}

credential_names=[disposable_invalid_name]
  => HTTP 200 {status:denied}

Only credential names were sent to this endpoint. The disposable invalid stored credential was deleted after the probe; the checked response and logs did not expose its value or provider response bodies.

Coordinated Canvas and Automation flow

In an earlier real local browser run with Canvas, Automation, and Agent Server, a valid repository/ref reached review with Preflight checks passed., then creation succeeded with the expected repository, ref, cron schedule, timezone, and enabled state. A fresh denied-credential preflight showed a repository-field error and blocked creation. The screenshots below document that creation/preflight flow, not an executed scheduled agent task. They predate the current SDK commit; current-head HTTP verification is reported separately above.

Video/Screenshots

• Passed repository preflight:
  
• Denied repository preflight with field error:
  
• Created automation detail:
  

These are real browser-flow screenshots, not deterministic mock UI output.

Design Doc

Not included. This extends the existing authenticated git-router and stored-credential boundaries.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

• The 404/501 unsupported-endpoint advisory fallback is implemented by the companion caller; this new endpoint does not claim to supply that fallback itself. Provider/transport errors are not treated as access success.
• Current-head pull_request implementation workflows are action_required and await maintainer approval. Only the pull_request_target description and artifact checks have passed; CI implementation validation is not claimed.
• Coordinated companion PR heads checked for this update:
  • Agent Canvas: OpenHands/OpenHands#16614 at 5c9462388d0e0f4234696a467c6114cef78092c8
  • Automation: OpenHands/automation#341 at 175ea64536930b81be4335481c60101a39ce11d9
  • Sandbox Server: OpenHands/sandbox-server#6 at 8c0631bcd98a919075ea99bdec257cc0d6e2a2b2
  • Extensions: OpenHands/extensions#476 at 100a9575cbe82e20fcb2434585c60694ecb6e9d1

[all-hands-bot]

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope is correct: this adds POST /api/git/validate-repository to the existing agent-server git router, which this repository owns (it owns the canonical REST API and the git-provider credential surfaces). No cross-repo move needed.

The design is sound in the important places. The outbound host is a fixed allowlist keyed by a Literal provider, repository/ref are regex-bounded and percent-quoted per segment, redirects and env-proxy inheritance are disabled, input rejects raw credential values (names only), the result is a bounded enum with no provider body, and the secret-store read is moved off the event loop via asyncio.to_thread. Because the caller can already read raw secret values through the same session-key trust domain (GET /api/settings/secrets/{name}), this endpoint does not widen the credential exposure boundary. I ran the focused suite on this head and it passes:

uv run pytest tests/agent_server/test_git_router.py -q
48 passed

One correctness finding is worth resolving before merge (inline): the GitLab probe uses a different API credential form than this repo's own GitLab clone convention, which can make the preflight report denied for credentials that clone fine.

Two things a maintainer should weigh, not inline findings:

1. CI has not actually run on this head. Every pull_request workflow for 91c14a3 (Run tests, Pre-commit checks, Agent Server, REST/Python API breakage, etc.) is action_required because the branch lives in a first-time contributor's fork (KirschBluteX/software-agent-sdk); only the pull_request_target metadata checks (PR Artifacts, PR Description Check) executed. The 48-passed evidence above is what I could reproduce locally. The merge is currently blocked pending CI approval and human review.

2. follow_redirects=False combined with mapping every non-2xx/401/403/404 response to unavailable means a renamed or moved repository — providers answer those with a 3xx to the new location — surfaces as a transient unavailable rather than not_found/accessible. Disabling redirects is the right call for the outbound boundary, but you may want a distinct verdict (or an explicit follow against the allowlisted host) so callers don't retry a permanently moved repo as if it were a network blip.

🔄 CHANGES REQUESTED

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope is correct: this adds POST /api/git/validate-repository to the agent-server git router, which this repository owns (canonical REST API and git-provider credential surfaces). No cross-repo move needed.

Note on this revision: the two changed files are byte-identical to the previously reviewed head (git_router.py = bed27575…, test_git_router.py = ad0dc8ef…); d3df1b78 only merges a newer main. So the open finding below carries over unchanged, and I re-verified it against the current head rather than restating the earlier verdict.

The security shape is sound: outbound host comes from a fixed allowlist keyed by a Literal provider, repository/ref are regex-bounded and per-segment percent-quoted, redirects and env-proxy inheritance are disabled, input accepts credential names only, the result is a bounded enum carrying no provider body, and the secret-store read is moved off the event loop via asyncio.to_thread. Since an authenticated caller can already fetch raw secret values via GET /api/settings/secrets/{name} in the same session-key trust domain, this endpoint does not widen the credential boundary. Focused suite passes on this head:

uv run pytest tests/agent_server/test_git_router.py -q
48 passed

One correctness finding remains before merge (inline): the GitLab probe uses a different API credential form than this repo's own GitLab clone convention, so a repository that clones fine can be reported denied.

Two items for a maintainer, not inline findings:

1. CI has not run on this head. Every pull_request workflow for d3df1b78 (Run tests, Pre-commit checks, Agent Server, REST API breakage checks, TypeScript client CI/audit, etc.) is action_required because the branch lives in a first-time contributor's fork (KirschBluteX/software-agent-sdk); only pull_request_target metadata checks executed. The 48-passed result above is what I reproduced locally. Merge is blocked pending CI approval and human review. Relatedly, main now carries a TypeScript endpoint-audit.config.json for Agent Server contract divergence; the audit is report-only and this new server-only route should be reported there as a server-only divergence, so worth confirming before rollout with the five coordinated PRs.

2. follow_redirects=False combined with mapping every non-2xx/401/403/404 response to unavailable means a renamed/moved repository — providers answer those with a 3xx to the new location — surfaces as a transient unavailable rather than not_found. Disabling redirects is right for the outbound boundary, but callers may want a distinct verdict so they don't retry a permanently moved repo as a network blip.

🔄 CHANGES REQUESTED

[all-hands-bot]

⏳ Review waiting on checks

The current head 090de68de94d still has checks that have not finished, so no review conversation was started:

• Agent Server
• Check Docstrings
• Deprecation deadlines
• Persisted settings compatibility checks
• Pre-commit checks
• REST API breakage checks
• Review Thread Gate
• Run tests
• Run tests (macOS)
• TypeScript client CI
• TypeScript client endpoint audit
• TypeScript client integration tests
• Version bump guard

No action is needed. The scheduled scan retries once every check on the head reports a conclusion.

This is an automated check - no AI was used to generate this comment.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope is correct: this adds POST /api/git/validate-repository to the existing agent-server git router, which this repository owns (canonical REST API and git-provider credential surfaces), and it is defined in Python first. No cross-repo move needed.

This is a fresh review of the current head 3100180. The prior reviewer finding has been addressed: the GitLab probe now uses Authorization: Bearer (_provider_auth_headers), matching the OAuth-capable form, and the previous inline thread is resolved/outdated. The regression test asserts the OAuth token is not sent as PRIVATE-TOKEN, so the false-denial case is covered.

The security shape holds on this head. The outbound host comes from a fixed allowlist keyed by a Literal provider; repository and ref are regex-bounded and percent-quoted per segment (quote(repository, safe="") for GitLab, safe="/" for GitHub/Bitbucket); redirects and env-proxy inheritance are disabled (follow_redirects=False, trust_env=False); the request accepts credential names only; the result is a bounded enum carrying no provider body (bodies are streamed but never read); and the secret-store initialization and read are both moved off the event loop via asyncio.to_thread. Since an authenticated caller can already fetch raw secret values through GET /api/settings/secrets/{name} in the same session-key trust domain, this endpoint does not widen the credential exposure boundary.

Verification I ran on the current head:

uv run pytest tests/agent_server/test_git_router.py -q
74 passed

uv run pytest tests/cross/test_remote_conversation_live_server.py -k validate_repository -q
1 passed

uv run ruff check <the three changed files>
All checks passed

Current-head CI: every pull_request workflow (Run tests, Pre-commit checks, Agent Server, REST API breakage, TypeScript client CI/audit, Version bump guard, etc.) is action_required because the branch lives in a first-time contributor's fork (KirschBluteX/software-agent-sdk); only the pull_request_target metadata checks (PR Artifacts, PR Description Check) ran and passed. The local evidence above is what substitutes for the unrun CI on this head, and merge remains blocked pending workflow approval and human review.

Two non-blocking observations for the human maintainer, neither attached to the diff:

1. The redirect handling from the earlier review still stands as a deliberate trade-off, not a defect: follow_redirects=False plus mapping every non-2xx/401/403/404 response to unavailable means a renamed or moved repository (which providers answer with a 3xx) surfaces as a transient unavailable rather than not_found. Disabling redirects is the right call for the outbound boundary; the caller should just be aware that a permanently moved repo is indistinguishable from a network blip.

2. The TypeScript client endpoint audit is report-only; this new server-only route is not listed in clients/typescript/endpoint-audit.config.json (which documents ignoreServerOnly: ["/v1/"]). Since Agent Canvas is the consumer, worth confirming it is intentionally reported as a server-only divergence under the coordinated five-PR rollout rather than added as an ignored endpoint.

No material correctness, security, compatibility, or acceptance-criterion defect remains on this head.

✅ APPROVED

[neubig]

The PR points to deterministic mock UI screenshots in a companion Canvas PR, but the SDK change is an authenticated backend endpoint/preflight behavior change. The SDK review guide requires evidence through the actual supported endpoint path, not indirect mock UI evidence. Please attach current-head command/output showing authenticated success and failure preflight responses, including the relevant repository/access conditions and redaction or error behavior. This comment was generated by an AI agent (OpenHands) on behalf of Graham Neubig.

[github-actions]

  ✅ **PR Artifacts Cleaned Up**

  The `.pr/` directory is no longer present.

[neubig]

🟡 Taste Rating: Acceptable — the endpoint has thoughtful SSRF, redirect, proxy, timeout, cancellation, and secret-redaction boundaries, but the coordinated production flow lacks authentic evidence.

[TESTING GAPS]

• [PR description, Authenticated endpoint reproduction / Video/Screenshots] Evidence is constructed rather than live: The endpoint reproduction uses the FastAPI test client, and the linked companion Canvas evidence is explicitly deterministic mock UI. This endpoint exists to drive Agent Canvas/Automation repository preflight, so tests and mock screenshots are insufficient. Please attach a live screenshot or video of actual Agent Canvas on the current coordinated heads showing both an accessible repository/ref and a denied or missing-credential case, together with the real authenticated endpoint responses/logs (redacted) that demonstrate the server-side credential path and error mapping.

Static review found no prompt injection, dangerous scripts, raw-secret input surface, unsafe redirects/proxy inheritance, dependency/supply-chain changes, CI failure, or merge conflict. The missing production-path evidence remains blocking.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH — this is an authenticated network and credential trust-boundary endpoint consumed across five repositories.

Recommendation: Do not auto-merge until a human security reviewer validates the live coordinated preflight and credential boundary.

VERDICT:
❌ Needs rework: Provide authentic current-head Agent Canvas plus endpoint evidence.

KEY INSIGHT: Strong unit-level SSRF and redaction defenses do not prove the cross-repository caller sends the intended credential names and interprets bounded failures correctly.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, add or update .agents/skills/custom-codereview-guide.md with the /codereview trigger, then re-request review. See the customization docs.

Resolve with AI? Install the iterate skill and run /iterate.

Was this review helpful? React with 👍 or 👎.

This review was generated by OpenHands AI on behalf of neubig.

[neubig]

I re-reviewed current head df70917d. The prior GitLab credential mismatch is fixed, the corresponding threads are resolved, and I found no new material code defect in the current diff.

The evidence concern from my October 5 review remains unchanged: the PR description demonstrates the authenticated path through a FastAPI test client and points to deterministic mock UI screenshots, but does not show the coordinated production-facing Canvas/Automation flow using the actual endpoint and stored credential path. Please add current-head live evidence for one accessible repository/ref and one denied or missing-credential case, with redacted endpoint output. Per the repository review guide, I am leaving this as COMMENT rather than submitting another request-changes review and deferring approval to a human maintainer.

Risk: 🔴 High because this introduces an authenticated network and credential-boundary endpoint consumed across repositories.

This review was generated by an AI agent (OpenHands) on behalf of Graham Neubig.

[neubig]

PR #4504 — follow-up review

🟡 Taste rating: Acceptable — compact, additive endpoint with careful network and credential boundaries, but its identifier validation rejects supported inputs.

Verified head: 5362cfe1377b12f6321372d391e1d0646d8ac320 (rechecked via GitHub API on 2026-10-09). Base: 16662b0bd2c1a93d3c5e282b48da05b95e90a0c8. GitHub reports mergeable, but merge state blocked. PR.

New finding

P2 — Accept legitimate repository names and Git refs instead of an alphanumeric-first ASCII subset.

Location: git_router.py:71–74, enforced at 110–122.

• repository="github/.github" is rejected before any provider request: each path component must start with an alphanumeric character. gh api repos/github/.github independently returned the existing public repository and default branch main.
• ref="_release" and ref="feature/café" are rejected, although both pass git check-ref-format --branch. These are branch names, not revision expressions or injection payloads.
• The existing RepoSource/clone path does not impose this ref restriction; _build_clone_command passes branch names as a separate argv value. The companion Automation caller preserves these inputs and turns this endpoint's 422 into a blocking repository_reference_invalid field error (caller at reviewed companion head). Thus valid automation configurations become impossible to preflight.

Fix: retain length limits and fixed hosts, validate provider-appropriate repository components (reject actual dot traversal components rather than all leading punctuation), and permit valid Git branch/tag names with URL component encoding. Add regression cases for .github, an underscore-leading branch, and a Unicode branch.

Verification: executed the exact request model and constants extracted by AST from the downloaded head, using installed Pydantic; no mocked validator or rewritten regex. Compared refs with the real Git command. Results:

repository github/.github, ref None: rejected ('repository',)
repository owner/repo, ref _release: Git valid; rejected ('ref',)
repository owner/repo, ref feature/café: Git valid; rejected ('ref',)
repository owner/repo, ref main: Git valid; accepted

This is a model-level reproduction, not a full live-server or clone test. The rejection-to-422 and caller error paths were verified in full source. No full pytest suite was run: the scratch checkout has no configured test environment, and the ambient OpenHands import resolves to a different checkout.

Existing discussion — not new findings

• Zero unresolved inline threads. Both GitLab OAuth/header threads are resolved and outdated: first, second and fix response. Current code uses Bearer authentication and includes OAuth/PAT regression coverage.
• neubig's previous CHANGES_REQUESTED review concerned live evidence; the latest review was COMMENT and reported no material code defect at df70917d.
• The description now supplies claimed authenticated HTTP results (accessible, missing_credentials, denied) plus three linked live Canvas screenshots and a successful creation narrative. Do not repeat the old claim that it contains only mock UI evidence. However, the evidence explicitly identifies df70917d, not this head. API comparison shows the endpoint and its two test files are unchanged between those heads; intervening changes are elsewhere. I did not independently inspect the images or rerun that coordinated deployment, so this is author-provided evidence, not independently verified current-head runtime proof.

CI and coverage

Current-head GitHub Actions is not green implementation validation:

• PR Description Check and PR Artifacts succeeded; cleanup jobs skipped.
• 13 workflow runs are action_required, including Run tests, Pre-commit, REST compatibility, Agent Server, TypeScript checks/audit/integration, persisted-settings compatibility, docstrings, version/deprecation guards, and two Review Thread Gate runs. These require approval to run; they are not test failures or passes.
• Legacy commit-status list is empty (pending aggregate). Earlier bot failure notices refer to old SHAs.
• Added tests cover auth/persistence through a live local HTTP server with mocked provider transport, URL encoding, status mapping, redirects, body disposal, malformed credentials, overall timeout/cancellation, and event-loop responsiveness. They do not cover the valid-input cases above.

Scope reviewed: full three-file diff and head source, root/package AGENTS and custom review guide, linked issues #3035 and OpenHands#16565, all PR comments/reviews and inline threads, auth mounting, exception handling, persistence, clone callers, and full companion Automation caller. No dependency changes in this PR. No additional verified credential disclosure or SSRF finding.

Recommendation and risk

Recommendation: COMMENT / hold approval, following the repository's review policy (not REQUEST_CHANGES). Fix the valid-input rejection, obtain current-head implementation CI, and have a human maintainer validate the coordinated credential flow.

Verdict: ❌ Needs rework for the concrete validation defect, not for stylistic preferences or stale discussion.

Key insight: A fixed-host, encoded provider request does not need to reject legitimate repository and branch names to remain safe.

Risk assessment: 🔴 HIGH. This new authenticated endpoint reads stored credentials and sends them to external provider APIs, and its verdict gates a cross-repository production flow. Fixed hosts, no redirects/environment proxies, bounded streaming I/O, and sanitized verdicts reduce risk; they do not eliminate the need for human review. Do not auto-merge.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add or edit .agents/skills/custom-codereview-guide.md with the /codereview trigger and the missing context. See the customization docs.
2. Re-request a review; guidelines on the PR branch apply immediately.
3. The guideline file goes through normal maintainer review when merged.

Resolve with AI? Install the iterate skill and run /iterate to drive fixes, CI, review, and QA.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was generated and posted by an AI agent (OpenHands) on behalf of Graham Neubig (@neubig).

[neubig]

🟡 Taste: Acceptable — thank you for addressing the earlier feedback. The .github, underscore-leading and Unicode-ref validation cases are now fixed, as is the earlier GitLab header issue. The description now contains meaningful live Canvas screenshots and endpoint results; the old “only mock screenshots” concern should not be repeated.

P2 — bare Bitbucket tokens need REST authentication, not clone authentication. The bare-token branch sends Basic base64(x-token-auth:<token>) to the Bitbucket REST API. Atlassian’s OAuth documentation requires Bearer for OAuth API requests and separately documents x-token-auth for Git cloning; repository access-token documentation likewise uses Bearer for API calls. A valid bare access token can therefore receive a false access-denial verdict.

Could you use Bearer for bare Bitbucket access tokens, preserve appropriate Basic handling for explicit account/token pairs, and update the provider coverage? The existing Bitbucket stand-in unconditionally returns 200 and asserts the Basic header, so it cannot catch this mismatch. We verified the exact head’s generated header and official provider contract, but did not reproduce rejection with a live valid Bitbucket credential. Please include redacted real-provider verification of that path after correction; the clone URL convention should remain separate.

Runtime evidence freshness: all three attached images were inspected and show passing preflight, denied access, and creation. However, the endpoint evidence names df70917d, while this head is 127b633e, and endpoint validation has changed. Please refresh redacted endpoint commands/results on the current head, including a newly accepted identifier/ref, and identify the coordinated component revisions. No executed scheduled agent task is required for this preflight/creation scope; the creation screenshot itself shows “Last run: Never,” so it should not be described as task-execution evidence.

Current implementation workflows are awaiting approval (action_required), not green implementation validation. Please obtain current-head CI as well.

Verdict: hold approval for the Bitbucket correction and current-head verification. Risk: HIGH — this endpoint handles stored credentials and external provider requests across a coordinated rollout. The fixed hosts, disabled redirects/proxies and bounded responses are good safeguards; human review of provider authentication and the live credential path is still appropriate.

---

Improve this review? If feedback is incorrect or irrelevant, add or update .agents/skills/custom-codereview-guide.md with the /codereview trigger and missing context, then re-request review. Branch guidelines apply immediately and receive normal maintainer review when merged. See the customization docs.

Resolve with AI? Install the iterate skill and run /iterate to drive fixes, CI and QA.

Was this review helpful? React with 👍 or 👎.

This review was generated by an AI agent (OpenHands) on behalf of Graham Neubig (@neubig).

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope is correct: this adds POST /api/git/validate-repository to the agent-server git router, which this repository owns, and the endpoint is defined in Python first. No cross-repo move needed.

Fresh review of the current head 9c3b3e2. Both prior P2 findings from the maintainer are addressed on this head, and I verified the code, not just the claim:

1. Bitbucket bare-token auth — _provider_auth_headers now returns Bearer <token> for a bare Bitbucket token and Basic base64(username:token) only when the stored value contains :. This matches Atlassian's REST contract and the maintainer's request. The previous x-token-auth:{token} Basic form is gone.
2. Identifier/ref validation — _validate_repository now rejects only genuinely invalid components (empty, ./.., control/space/DEL, characters outside alnum/_.-) instead of requiring an alphanumeric first character, so github/.github is accepted. _is_valid_git_ref replaces the old _REF_PATTERN and permits _release, -release, and feature/café while still rejecting .., @{, .lock components, ~^:?*[\, and control/space characters.

I spot-checked _is_valid_git_ref against the real git check-ref-format --branch for the accepted/rejected cases in the diff; the only divergences are deliberate: HEAD (a revision, not a branch name — valid as a provider ref endpoint) and @ (git permits it as a shorthand, but rejecting it is correct for a URL path). No new injection or traversal path: ref is still quote(ref, safe="")-encoded into the path, and hosts remain a fixed allowlist.

Verification I ran on this head:

uv run pytest tests/agent_server/test_git_router.py -q
89 passed

uv run pytest tests/cross/test_remote_conversation_live_server.py -k validate_repository -q
1 passed

The tests now cover github/.github, _release/-release/feature/café refs, and both Bitbucket forms (Basic for username:token, Bearer for a bare token) against a transport that rejects a wrong header, which is exactly the gap the earlier review noted.

Two outstanding items keep this at COMMENT rather than approval, and both are evidence/process, not code defects:

1. Current-head CI has not run. All 14 pull_request workflows on 9c3b3e2 are action_required (fork PR awaiting approval); only the 4 pull_request_target metadata checks passed. The maintainer explicitly asked for current-head CI, so this needs workflow approval and a green run before merge.
2. Live evidence is stale relative to this head. The endpoint evidence in the PR body names df70917d, and the Bitbucket correction landed later at 9c3b3e2. The maintainer's latest review asked for redacted current-head endpoint results, including the newly accepted identifier/ref cases. The attached Canvas screenshots are real browser captures (not mock UI) and should not be re-litigated, but they do not exercise the current head's changed auth path.

Per the repository review guide's live-evidence and CI checkpoints for a production-facing, credential-bearing endpoint, I am leaving this as COMMENT and deferring approval to a human maintainer. With current-head CI green and refreshed redacted endpoint results for the accepted identifier/ref and Bitbucket cases, this would be approvable on the code as written.

🔄 CHANGES REQUESTED

[neubig]

Taste: acceptable. The credential boundary is generally clean, but I would hold approval for the ref-classification issue below and the remaining validation gates.

P2 — Nonexistent GitHub refs become infrastructure failures

In git_router.py:294–296, only HTTP 404 maps to not_found; GitHub's commit lookup returns 422 for a syntactically valid but nonexistent ref, so this falls through to unavailable.

Independently reproduced through an authenticated Agent Server running this exact head:

POST /api/git/validate-repository
{"provider":"github","repository":"github/.github","ref":"main"}
=> HTTP 200 {"status":"accessible"}

POST /api/git/validate-repository
{"provider":"github","repository":"github/.github","ref":"does-not-exist-review-4504"}
=> HTTP 200 {"status":"unavailable"}

A direct upstream request returned HTTP 422, No commit found for SHA. The companion Automation caller treats unavailable as a dependency outage and returns generic HTTP 503, so an ordinary mistyped branch cannot receive actionable repository/ref validation. Please normalize this provider/ref-specific missing-ref response to an appropriate bounded invalid-ref/not-found verdict and add regression coverage for GitHub's 422 response. Do not expose provider response bodies or blanket-classify arbitrary 422s as missing refs.

Verification and evidence

• Independently ran the router suite: 90 passed. The cross-server test hit a fixture startup timeout before endpoint assertions; I am not counting it as a pass or as an endpoint defect.
• Fresh exact-head live HTTP probes confirmed no-session rejection (401), public-repository success, absent named credentials, and denial using an isolated disposable invalid stored credential. The endpoint received credential names only; no real user credential was used. The owned server was stopped afterward.
• Fixed provider hosts, encoded paths, disabled redirects/environment-proxy inheritance, discarded provider bodies, bounded verdicts and provider timeouts look appropriate. No SSRF or credential-forwarding defect was established. Provider API accessibility is correctly documented as not guaranteeing Git clone permission.
• The linked screenshots support historical preflight pass/denial and automation creation, not current-head coordinated end-to-end behavior. The description acknowledges their age; scheduled task execution is not required to demonstrate this bounded feature.
• Current-head implementation workflows are action_required, not passing runs. Please obtain maintainer approval and execution of those checks. The previously requested valid Bitbucket credential path, and valid GitLab/private-repository authentication, still need provider-authentication validation rather than only intercepted transport tests.

[RISK ASSESSMENT]

HIGH — COMMENT / hold approval; do not auto-merge. This adds stored-provider-credential access and cross-service verdict semantics. Fix the demonstrated missing-ref classification, run current-head implementation CI, and obtain human provider-authentication/release-alignment validation.

Architectural insight: the credential ownership boundary is sound; provider-specific HTTP semantics still need normalization so invalid configuration is not propagated as an infrastructure outage.

This review was generated and posted by an AI agent (OpenHands) on behalf of Graham Neubig (@neubig). Reviewed head: 6b621b7d90ca1a4b8108ac99bb36d24181294a08.

Improve this review? If feedback seems incorrect or irrelevant, update the repository's .agents/skills/custom-codereview-guide.md (with the /codereview trigger), then re-request review. The reviewer reads the guide from the PR head.

Resolve with AI? Install the iterate skill and run /iterate.

Was this review helpful? React with 👍 or 👎.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope is correct: this adds POST /api/git/validate-repository to the agent-server git router, which this repository owns, and the endpoint is defined in Python first. No cross-repo move needed.

Fresh review of head 6b621b7 (matches the requested SHA). One material correctness defect remains.

P2 — GitHub answers 422 for an unknown ref, so a mistyped branch is reported as an outage

git_router.py:294-296 maps only 404 to not_found; every other unmapped status falls through to unavailable. GitHub's commit endpoint returns 422 (No commit found for SHA) when the repository exists but the ref does not. I reproduced both sides on the live provider and against this head's handler:

GET https://api.github.com/repos/octocat/Hello-World/commits/master     -> 200
GET https://api.github.com/repos/octocat/Hello-World/commits/nope-4504  -> 422
GET https://api.github.com/repos/github/.github/commits/does-not-exist  -> 422

handler with upstream 404 -> {"status": "not_found"}
handler with upstream 422 -> {"status": "unavailable"}

The companion Automation caller (OpenHands/automation#341) treats unavailable as a dependency outage and answers generic 503, so an ordinary typo in the ref field is indistinguishable from the provider being down and never becomes a field-addressable error. not_found is the verdict the response model documents for "no such ref/repository", so the endpoint's own contract is not met.

A fix needs to stay provider-specific rather than adding a blanket 422 arm: only GitHub's commit lookup uses 422 for a missing ref, and arbitrary 422s must not be reclassified. Provider bodies must stay unexposed, and regression coverage for GitHub's 422 should be added.

Non-blocking

• The new route has no owner in the F15-git feature map (.agents/skills/verify-agent-server/references/feature-map/F15-git.md), which openhands-agent-server/AGENTS.md requires for a new or changed route.
• Current-head CI is not a passing run: 12 pull_request workflows on 6b621b7 are action_required (fork PR awaiting maintainer approval); only the two pull_request_target metadata checks passed.

What I verified on this head

• uv run --frozen pytest tests/agent_server/test_git_router.py -q → 90 passed.
• The credential boundary holds. Only secret names enter the request model (extra="forbid" rejects a raw token field), values are resolved server-side off the event loop, and the outbound request carries the token only to a fixed provider host. trust_env=False, follow_redirects=False, streaming with the body discarded, and the 5 s overall deadline are all present. httpx/httpcore do not log request headers at DEBUG, so the token does not reach logs.
• Redirects, transport errors, malformed credentials, and rate-limit 403s all map to unavailable, never to accessible.
• GitLab project-id and ref encoding, and the Bitbucket Bearer / Basic username:token split, are correct on this head.

🔄 CHANGES REQUESTED

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Scope is correct: this adds POST /api/git/validate-repository to the agent-server git router, which this repository owns, and the endpoint is defined in Python first. No cross-repo move needed.

Fresh review of head 090de68 (matches the requested SHA). The one material defect I raised on the previous head is fixed on this head, and I found no new material issue.

Prior P2 (GitHub 422 for an unknown ref) — resolved

git_router.py:294-299 now maps GitHub's ref-lookup 422 to not_found, scoped to provider == "github" with a non-null ref, so arbitrary 422s and the GitLab/Bitbucket paths are untouched. I verified the behavior against the live provider and this head's handler:

GET https://api.github.com/repos/octocat/Hello-World/commits/nope-4504 -> 422 ("No commit found for SHA")
handler: github + ref, upstream 422 -> not_found
handler: github without ref, upstream 422 -> unavailable
handler: gitlab/bitbucket + ref, upstream 422 -> unavailable
handler: any provider, upstream 404 -> not_found

The provider body is still discarded, so the classification does not leak No commit found. The new test_validate_repository_classifies_github_missing_ref_422 covers exactly this matrix, and the F15-git feature map gained the route owner plus an F15.repository-validation recipe — control-agent-server map coverage now reports the new route as owned and driven.

Verification I ran on this head

• uv run --frozen pytest tests/agent_server/test_git_router.py -q → 95 passed.
• uv run --frozen pytest tests/cross/test_remote_conversation_live_server.py -k validate_repository -q → 1 passed (a first run errored in fixture startup; it passed on re-run, so I am not counting it as an endpoint defect).
• The credential boundary still holds: only secret names enter the request model (extra="forbid" rejects a raw token field), values are resolved server-side off the event loop, and the token is sent only to a fixed provider host. trust_env=False, follow_redirects=False, streaming with the body discarded, and the 5 s overall deadline are present. httpx/httpcore do not log request headers at DEBUG, so the token does not reach logs; a CRLF-bearing stored value fails as httpx.LocalProtocolError (a TransportError) and maps to unavailable.

Non-blocking

• Implementation CI has not executed on this head: all 13 pull_request workflows on 090de68 are action_required (fork PR awaiting maintainer approval); only the two pull_request_target metadata checks passed. A maintainer needs to approve and run them before merge. My own focused runs above are current-head evidence for this narrow change.
• The prior review thread from my earlier pass is still open on this file; the author needs to resolve it for the Review Thread Gate check.
• Adding F15.repository-validation leaves the map index stale: control-agent-server map check now reports ok: false (README says 28 sub-features / 935 total, F15 has 29 and the map has 936). mapping.md assigns the index to a coordinator, so this is expected mid-flight, but map check --fix-counts should be run before the map's next daily pass.

✅ APPROVED