Skip to content

fix(git-sync): validate push access where GitHub enforces it (#2107) - #3018

Merged
vybe merged 3 commits into
devfrom
feature/2107-git-write-probe
Sep 25, 2026
Merged

vybe merged 3 commits into
devfrom
feature/2107-git-write-probe

Conversation

@dolho

@dolho dolho commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Nothing checked that an agent's git credential can write. A fine-grained PAT with Contents: read-only passed every check and then failed every auto-sync for the agent's whole life (64 in the reported case, remote: Write access to repository not granted.):

  • git ls-remote talks to upload-pack, so it only proves read access.

  • The REST permissions.push field reports the user's role on the repo, not what a fine-grained token was granted.

  • git_service.probe_push_access(repo, pat) asks the side GitHub enforces: a git push --dry-run --porcelain <url> HEAD:refs/heads/__trinity_write_probe from an empty scratch repo on the backend host. The PAT rides git_auth_env (an http.extraHeader in the child's env), never argv. The dry run negotiates auth and permission and creates nothing. Result: ok / denied (with GitHub's own line) / transient.

  • Creation: agents that will auto-push (the _git_auto_sync_baked predicate: working-branch mode or fork-to-own, with a PAT) are probed. denied → 400 naming GitHub's reason and the fix (fine-grained: Contents: Read and write; classic: repo; or source mode), before any branch is reserved or container is created. transient → logged, non-blocking (the PAT path's existing policy). Source-mode (pull-only) agents aren't probed.

  • Sync health: when the error is a refused push (git_service.is_push_denied), the sync_failing item is titled "Git token can't push", says it won't recover on its own, and carries context.cause = "push_denied" plus a remediation. Other failures keep the generic item.

Not in this PR

Changes

  • src/backend/services/git_service/provisioning.py: probe_push_access, is_push_denied, _PUSH_DENIED_PATTERNS (re-exported from the package)
  • src/backend/services/agent_service/crud.py: _validate_push_access, plus the will_push gate at the create call site
  • src/backend/services/sync_health_service.py: the named sync_failing item
  • Tests:
    • tests/unit/test_2107_push_access_probe.py (17, new): real git against a local fake smart-HTTP server; covers the 403 with GitHub's text, a valid receive-pack advertisement (nothing POSTed), 5xx and unreachable. It also asserts the token arrives as an Authorization header and is absent from git's argv, and covers the classifier table.
    • test_1484_create_agent_characterization.py: +4 through the full create_agent_internal. A refused push gives a 400 with no container and no branch reserved; a transient failure doesn't block; source mode is never probed. The harness's _git_auto_sync_baked is now the faithful predicate instead of a truthy MagicMock.
    • test_sync_health_service.py: +2 on the real DB harness (named push-denied item; ordinary failure unchanged).
  • Docs: github-sync.md (probe section + error table), git-sync-health.md (alert branch)

Test Plan

  • New and touched suites green with random ordering (2107 17, 1484 49, sync_health_service 47, 1028 package surface, ent123, fork_to_own, 1595_sync_health_signals, ent109_repo_binding)
  • Mutation: with the fix reverted, 20 of the 23 new tests fail. The 3 that stay green are behaviour-preserving (transient doesn't block, source mode isn't probed, ordinary failure unchanged).
  • Against real GitHub, using a token (user repo scope) that can write one repo and not another:
    • octocat/Hello-World → ('denied', 'remote: Permission to octocat/Hello-World.git denied to <user>.')
    • Abilityai/trinity → ('ok', '')
    • ls-remote afterwards shows no __trinity_write_probe ref, so the dry run created nothing.
    • The fine-grained read-only text (Write access to repository not granted.) comes from the issue's observed data. It's covered by the fake-server test; I had no such token to reproduce it live.
  • lint_sys_modules, lint_root_test_placement clean

Fixes #2107

🤖 Generated with Claude Code

A Contents: read-only fine-grained PAT passed every check (ls-remote
talks to upload-pack; REST permissions.push is the user's role, not the
token's grant) and then failed every auto-sync for the agent's life.

- git_service.probe_push_access: git push --dry-run of a throwaway ref
  from an empty scratch repo, PAT via git_auth_env (never argv); asks
  receive-pack, creates nothing. ok / denied / transient.
- creation: agents that will auto-push (_git_auto_sync_baked) are probed;
  denied -> 400 naming GitHub's reason and the fix, before any branch is
  reserved or container created; transient -> logged, non-blocking.
- sync health: a sync_failing item whose error is a refused push is
  titled 'Git token can't push', says it won't recover on its own, and
  carries cause=push_denied + remediation.

Fixes #2107

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comment thread src/backend/services/git_service/provisioning.py Fixed
…2107)

CodeQL py/clear-text-logging-sensitive-data: the line comes from a child
that held the token. It is still returned (scrubbed) to the caller.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-probe

# Conflicts:
#	tests/registry.json
#	tests/unit/test_1484_create_agent_characterization.py

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: batch validated on train/20260925-1608 (#3027, full suite green).

@vybe
vybe merged commit c36873f into dev Sep 25, 2026
24 checks passed
vybe pushed a commit to harimaruthachalam/trinity that referenced this pull request Sep 29, 2026
…nity-enterprise#705)

Create-time kind: agent (default) | deployment, on POST /api/agents and
MCP create_agent. An explicit source_mode always wins.

An agent gets a working branch + auto_sync_enabled + freeze-on-sync-
failure, but only when the Abilityai#2107 push probe says its token can push to
that repo. A deployment, an ephemeral ghost, a tokenless create, a
refused or unverifiable probe all stay pull-only, with the reason on the
create response's git_mode (never on the /ws broadcast). A template
someone else owns therefore never receives an agent's branches (the
ent#162 class); Cornelius, built from a shared public upstream, is
pinned pull-only. Fork-to-own gets the trio.

Existing agents are not flipped. Operator runbook for migrating a live
agent: docs/migrations/AGENT_WORKING_BRANCH_DEFAULT_2026-09.md.

Stacked on Abilityai#3016, Abilityai#3017, Abilityai#3018 (+Abilityai#3015): merges only after them.

Related to Abilityai/trinity-enterprise#705

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
vybe pushed a commit that referenced this pull request Oct 2, 2026
…rise#703) (#3021)

* feat(git-sync): the container pulls origin on its own (trinity-enterprise#703)

Invariant G3: human and fleet work reaches the agent within a bound.
Nothing inside the container ever pulled; the 2026-09-24 audit found
agents up to 31 commits behind.

- agent server: a pull loop beside the push loop, gated per cycle on
  the owner's pull_sync_enabled (read live, GIT_SYNC_PULL fallback),
  interval GIT_SYNC_PULL_INTERVAL_SECONDS (defaults to the push one).
  Under _REPO_LOCK, never while an execution runs (checked again right
  before the tree is touched). Fast-forward, or rebase aborted on
  conflict. Uncommitted edits are stashed explicitly and a pull that
  collides with them is undone - not --autostash, which strands them
  in the stash while reporting success.
- pull_sync_enabled + last_pull_at/_status/behind_after_pull on both
  migration tracks; backfill on only where auto-sync is on; new github
  agents get it at creation (source mode included), GIT_SYNC_PULL env
  derived from the DB flag alone at recreate.
- GET/PUT /api/agents/{name}/git/pull-sync; sync-health persists the
  pull outcome (bounded); Settings -> Git sync gains the toggle.

Stacked on #3020 (+#3015-#3018): merges after it.

Related to Abilityai/trinity-enterprise#703

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix(schema): keep the FK strip working on agent_git_config for PostgreSQL (trinity-enterprise#703)

The inline comment on `pull_sync_enabled` sat between the comma and the
FOREIGN KEY clause. `_PG_TABLE_SUBS` strips that clause only when it directly
follows the comma, so the clause survived without its REFERENCES and
PostgreSQL rejected the table ("syntax error at or near ')'"), failing 18
requires_postgres tests in schema-parity. The comment moves above the column.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix(git-sync): pull cycle never strands local work, counts queued turns, brings main to working branches (trinity-enterprise#703)

PR #3021 review:
- _with_stash wraps every post-stash step: a git child that times out
  (run_registered raises TimeoutExpired) goes back to the pre-pull HEAD
  and re-applies the stash; when that is impossible the error says the
  edits are kept in `git stash`.
- reset --hard's return code is checked. A failed undo stops with an
  error naming it instead of leaving UU files behind silently.
- Both cycles refuse to run over unmerged paths. The push cycle checks
  before `git add -A`, which would stage conflict markers and push them
  to origin as a successful sync.
- The execution gate counts register_pending entries (#2433), so a turn
  queued on the chat lock or the headless pool is busy, not idle.
- The pull reaps stale lock litter first, as the push cycle does; a
  pull-only agent has no push cycle to do it.
- sync-state gains last_pull_error streak fields: consecutive pull
  failures / skips and last_successful_pull_at.

Ruling on the intent question: a trinity/* working branch only ever
pulled itself, so human pushes to main never arrived (G3). The cycle
now also merges origin/main (via _get_pull_branch) into the working
branch — a merge, not a rebase, since the branch is already pushed; a
conflict is aborted and recorded.

Nine real-repo tests, all red before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* feat(git-sync): persist pull health; renumber to 0080_pull_sync; docs + UI copy (trinity-enterprise#703)

- Alembic 0076_pull_sync -> 0080_pull_sync <- 0079_telegram_group_context
  (one head, 81 revisions); SQLite entry re-appended after dev's.
- The same migration pair adds agent_sync_state.last_pull_error,
  last_successful_pull_at, consecutive_pull_failures and
  consecutive_pull_skips, persisted by the sync-health poller (bounded,
  coerced; a success clears the error). ent#706/#707 read these, so they
  land here rather than as a second migration on the same table.
- agent-runtime.md says three loops; the flow and requirement describe the
  main merge, the queued-turn gate, the unmerged-path refusal, what
  behind_after_pull measures, and that auto-sync agents already rebase in
  the push cycle.
- Settings copy no longer hard-codes 15 minutes
  (GIT_SYNC_PULL_INTERVAL_SECONDS) and no longer claims the push cycle
  never touches the tree while the agent works.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(git-sync): pull records merge conflicts as conflicts; rebases keep merges (trinity-enterprise#703)

PR #3021 re-review:
- A conflict merging main was recorded as "Auto-merging <file>": git
  prints it on stdout with an empty stderr. Unmerged paths are read
  before the abort; the error is "diverged: merge conflict with main
  (<files>)", and a failed merge --abort is named.
- `git rebase` flattened the merge the pull made into copies of main's
  commits; both cycles now rebase with --rebase-merges.
- A failed strict ahead/behind count fails the pull instead of reading
  as up to date.
- A timed-out step is reset to the pre-pull HEAD even with nothing
  stashed; a reset blocked by the killed child's index.lock says so.
- "Never runs while the agent is working" is check-then-act: the copy
  and docs now say "never starts" and document the admission window.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(git-sync): pull-sync write is person-only; classify the agent's read

#2996's route census (merged to dev) requires every new route to be
classified. The agent's pull loop reads GET .../git/pull-sync with its
own key each cycle (AGENT_CALLABLE); the PUT is a setting write, so it
takes Depends(require_person) like the other #2996 settings.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(git-sync): the push and pull cycles share the repo lock without starving each other (#3021 review)

The two background loops started together on one interval and took
_REPO_LOCK non-blocking, so the loser skipped silently every tick and
the effective pull bound could quietly double. Background cycles now
wait up to GIT_SYNC_LOCK_WAIT_SECONDS (default 120 s) for the lock, and
the pull loop's first tick is half an interval after the push loop's.
Operator endpoints keep their immediate 409.

The pull's reset --hard undo re-checks that no execution started
meanwhile; if one did, the tree is left as it is (conflict markers block
the next push and pull) instead of discarding the turn's writes.
merge --abort runs only when MERGE_HEAD exists, so a merge that refused
to start no longer records "There is no merge to abort".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* merge-train: narrow the "never discards local work" claims to what _safe_to_reset guarantees (#3021) — mechanical, per the merge-train note on the PR

The undo paths no longer reset over a registered execution, but writes outside
the process registry (Files API, docker exec, web terminal) made during the
integrate window are not protected, as the author's e6cb8b5 note concedes.
Wording only: two git.py docstrings, the auto_sync.py comment,
requirements/github.md and the GitSyncSettingsPanel.vue copy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-authored-by: Andrii Pasternak <44377756+AndriiPasternak31@users.noreply.github.com>
Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants