feat(git): go-git v6, so Azure DevOps is now also supported - #297
Merged
Conversation
Azure DevOps rejects any protocol-v0 upload-pack request whose capability list omits multi_ack, with HTTP 400 "TF401041: Clients must support multi-ack." go-git v5 keeps MultiACK and MultiACKDetailed in transport.UnsupportedCapabilities and deletes them from the server's advertisement as it parses, so the capability is never requested and every fetch against ADO fails (#288). v6 implements the capability (go-git#1204), and upstream then deleted their own ADO workaround example saying it "works out of the box". This takes v6.0.0-alpha.5 -- the latest tag, and identical to upstream main -- rather than PR #292's fallback to a bundled system git binary, which measured at +723 MB of image and left the CRITICAL image-scan gate blind to git, OpenSSH and OpenSSL because they arrive as loose files with no package database. The reasoning, the measurements and the four options are in docs/design/azure-devops-multi-ack.md. The blast radius of the ADO problem is one call, repo.Fetch. CheckRepo and listRemoteRefs read only the ref advertisement, and PushAtomic speaks receive-pack, which has no multi_ack at all. The tests assert exactly that, and they passed on v5. Red-first, and it needs no Azure DevOps tenant: canonical git's own upload-pack advertises multi_ack, so git-http-backend behind a proxy that enforces ADO's rule is a faithful simulator for both halves -- the proxy reproduces the rejected request, the real backend reproduces the multi-ACK response v5 also cannot parse. The v2 opt-in header is stripped so a v2-capable client cannot sidestep the capability under test. TestADO_SmartFetch_RequiresMultiAck fails on v5 with ADO's exact 400 and passes on v6. The API migration: - transport.AuthMethod is gone; auth is functional options. A credential now travels as []gitclient.Option, and git.Credential keeps the concrete value alongside so the Secret-key-to-auth-field mapping stays assertable -- the options are closures and cannot be inspected. - transport.NewEndpoint + client.NewClient + NewReceivePackSession become transport.ParseURL + gitclient.New(opts).Handshake. - AdvertisedReferences becomes GetRemoteRefs, returning a slice. - ReceivePack becomes Session.Push. The atomic push keeps its guarantee unchanged: one session serves both the advertisement and the push, and PushRequest.Commands takes the same *packp.Command, so the server-side Old/New compare-and-swap is verbatim. v6 negotiates report-status itself and returns a rejected command as the error from Push, so the separate status inspection collapses into one check, and Atomic is now a first-class field. Two settings v6 reads from the environment that v5 ignored, both failing closed, and neither visible to a unit test: - commit.gpgSign, merged across system, global and local scope, is consulted whenever CommitOptions.Signer is nil, and refuses the commit when set with no signer registered. Any host or image with it set would break every commit we make. PinExplicitSigningPolicy writes the local value false at init: our signing policy comes from the GitProvider, not from ambient config. - HostKeyAlgorithms is derived by reading ~/.ssh/known_hosts and /etc/ssh/ssh_known_hosts whenever ClientConfig returns it empty, even when a HostKeyCallback was supplied, and hard-fails when neither file exists. The controller image is distroless with neither, so every SSH remote would have failed in production regardless of the credential. ssh.KeyAuth now always populates the list, from the pinned known_hosts when there is one and a modern default set otherwise. Found by the e2e suite against a real Gitea; no unit test can reach it, because the fallback lives in the transport's connect rather than in ClientConfig. TestBranchWorker_ConcurrentOperations moves to a real git server. v5's file:// transport spawned the real git-receive-pack; v6 runs go-git's in-process one, whose updateReferences never compares cmd.Old and just sets the reference, so over file:// every racing push wins and the test was passing vacuously with 2 commits instead of 4. Any future test of the compare-and-swap must avoid file://. Closes #288 Refs #292 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR migrates Git handling from go-git v5 to v6, introduces structured credential and SSH authentication handling, updates fetch and push transport flows, adds Azure DevOps multi-ACK simulator and live/e2e coverage, and documents the migration decision and setup. ChangesGit transport migration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
internal/controller/ssh_test.go (1)
349-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the split-out test after the function under test.
The subject is
extractCredential, soTestExtractCredential_HTTPAndAnonymousmatches the convention;TestCredentials_...refers to no existing function.As per coding guidelines: "name tests
TestFunctionName_Scenario(t *testing.T)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/ssh_test.go` around lines 349 - 352, Rename the split-out test function from TestCredentials_HTTPAndAnonymous to TestExtractCredential_HTTPAndAnonymous so its name follows the function-under-test convention for extractCredential while preserving the existing test behavior.Source: Coding guidelines
internal/controller/gitprovider_controller_test.go (1)
59-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two passphrase specs are now identical and neither exercises a passphrase.
Both blocks supply the same unencrypted key plus
ssh-passphrase: "", andsshPassphraseininternal/git/credentials.goonly readsssh-password/password— sossh-passphraseis never consulted and both specs reduce to the no-passphrase case already covered at Lines 39-57. Worth either dropping one and pointing the other at a recognized key, or asserting something passphrase-specific.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/gitprovider_controller_test.go` around lines 59 - 114, Remove the duplicate empty-passphrase coverage in the SSH credential specs around generateTestSSHKey and extractCredential, or replace one test with a recognized ssh-password/password key and an encrypted SSH key. Ensure the remaining test exercises behavior specific to the supported passphrase lookup rather than repeating the unencrypted no-passphrase case.internal/git/credentials_test.go (2)
91-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
x, ok := v, v != nilis a leftover of the removed type assertion.Now that
Credentialexposes typed fields,require.NotNilreads better and the extraokvariable disappears.♻️ Proposed simplification
- basic, ok := auth.Basic, auth.Basic != nil - require.True(t, ok) - assert.Equal(t, "u", basic.Username) - assert.Equal(t, "p", basic.Password) + require.NotNil(t, auth.Basic) + assert.Equal(t, "u", auth.Basic.Username) + assert.Equal(t, "p", auth.Basic.Password)- token, ok := auth.Bearer, auth.Bearer != nil - require.True(t, ok) - assert.Equal(t, "gho_token", token.Token) + require.NotNil(t, auth.Bearer) + assert.Equal(t, "gho_token", auth.Bearer.Token)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/credentials_test.go` around lines 91 - 108, In the Basic and bearer token test cases, replace the `x, ok := v, v != nil` patterns with direct typed-field variables and `require.NotNil` assertions. Remove the redundant `ok` variables while preserving the existing username, password, and token assertions.
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest names still reference the pre-migration functions.
These now exercise
CredentialFromSecretData/credentialFromSecret, so theTestAuthFromSecretData_*andTestGetAuthFromSecret_*names point at the thin wrappers rather than the code under test.As per coding guidelines: "name tests
TestFunctionName_Scenario(t *testing.T)".Also applies to: 69-69, 83-83, 247-247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/credentials_test.go` at line 50, Rename the affected tests in internal/git/credentials_test.go to follow the functions they directly exercise: replace the outdated TestAuthFromSecretData_* and TestGetAuthFromSecret_* prefixes with CredentialFromSecretData and credentialFromSecret respectively, while preserving each scenario suffix.Source: Coding guidelines
internal/git/git_atomic_push.go (1)
198-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the exported v6 API changes.
Both updated exported functions lack Go doc comments.
internal/git/git_atomic_push.go#L198-L203: add aPushAtomiccomment describing its compare-and-swap push contract and client-option authentication.internal/git/git_smart_fetch.go#L25-L30: add aSmartFetchcomment describing its fetch/result behavior and client-option authentication.As per coding guidelines, “add godoc comments for all exported identifiers.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/git_atomic_push.go` around lines 198 - 203, Add Go doc comments for the exported functions PushAtomic in internal/git/git_atomic_push.go (lines 198-203) and SmartFetch in internal/git/git_smart_fetch.go (lines 25-30). Document PushAtomic’s compare-and-swap push contract and client-option authentication, and document SmartFetch’s fetch/result behavior and client-option authentication.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/INDEX.md`:
- Line 88: Update the Azure DevOps multi-ACK entry in the documentation index to
reflect that Option A is decided and implemented rather than decision needed.
Also increment the “Sixteen other open items” count to seventeen, preserving the
rest of the entry’s content.
In `@internal/git/git_atomic_push.go`:
- Line 40: In the atomic-push flow around transport.ParseURL, validate that
remote.Config().URLs contains at least one entry before accessing URLs[0].
Return the existing reconciliation error type or path for a configured origin
with no URLs, and add a negative test covering this empty-URLs case without
changing behavior for valid remotes.
In `@internal/git/git.go`:
- Around line 640-642: Update PrepareBranch so PinExplicitSigningPolicy(repo)
runs after both repository acquisition paths, including when tryOpenExistingRepo
returns an existing checkout, while preserving error propagation. Add a
regression test covering an already-existing checkout with ambient
commit.gpgSign=true and verifying unsigned commits proceed successfully.
In `@internal/ssh/auth_test.go`:
- Around line 142-152: Strengthen the “with a pinned known_hosts” subtest by
asserting cfg.HostKeyAlgorithms differs from defaultHostKeyAlgorithms(), proving
the pin lookup supplied the algorithms rather than fallback. Also cover a
non-22-port request if supported by req to validate the hostWithPort(req) lookup
key.
---
Nitpick comments:
In `@internal/controller/gitprovider_controller_test.go`:
- Around line 59-114: Remove the duplicate empty-passphrase coverage in the SSH
credential specs around generateTestSSHKey and extractCredential, or replace one
test with a recognized ssh-password/password key and an encrypted SSH key.
Ensure the remaining test exercises behavior specific to the supported
passphrase lookup rather than repeating the unencrypted no-passphrase case.
In `@internal/controller/ssh_test.go`:
- Around line 349-352: Rename the split-out test function from
TestCredentials_HTTPAndAnonymous to TestExtractCredential_HTTPAndAnonymous so
its name follows the function-under-test convention for extractCredential while
preserving the existing test behavior.
In `@internal/git/credentials_test.go`:
- Around line 91-108: In the Basic and bearer token test cases, replace the `x,
ok := v, v != nil` patterns with direct typed-field variables and
`require.NotNil` assertions. Remove the redundant `ok` variables while
preserving the existing username, password, and token assertions.
- Line 50: Rename the affected tests in internal/git/credentials_test.go to
follow the functions they directly exercise: replace the outdated
TestAuthFromSecretData_* and TestGetAuthFromSecret_* prefixes with
CredentialFromSecretData and credentialFromSecret respectively, while preserving
each scenario suffix.
In `@internal/git/git_atomic_push.go`:
- Around line 198-203: Add Go doc comments for the exported functions PushAtomic
in internal/git/git_atomic_push.go (lines 198-203) and SmartFetch in
internal/git/git_smart_fetch.go (lines 25-30). Document PushAtomic’s
compare-and-swap push contract and client-option authentication, and document
SmartFetch’s fetch/result behavior and client-option authentication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7284c3a-49bf-4bf7-8920-5974222c51d9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (53)
docs/INDEX.mddocs/design/azure-devops-multi-ack.mdgo.modinternal/controller/gitprovider_controller.gointernal/controller/gitprovider_controller_test.gointernal/controller/ssh_test.gointernal/git/acceptance_gate_test.gointernal/git/ado_multiack_test.gointernal/git/bootstrapped_repo_template.gointernal/git/branch_worker.gointernal/git/branch_worker_metrics_test.gointernal/git/branch_worker_split_test.gointernal/git/branch_worker_test.gointernal/git/commit.gointernal/git/commit_executor.gointernal/git/commit_executor_test.gointernal/git/commit_request_attach_test.gointernal/git/credentials.gointernal/git/credentials_test.gointernal/git/fieldpatch_flush_test.gointernal/git/git.gointernal/git/git_atomic_push.gointernal/git/git_atomic_push_test.gointernal/git/git_operations_test.gointernal/git/git_smart_fetch.gointernal/git/helpers.gointernal/git/helpers_test.gointernal/git/inplace_edit_test.gointernal/git/inplace_overrides_test.gointernal/git/known_placement_bugs_test.gointernal/git/kustomize_delete_test.gointernal/git/kustomize_oracle_test.gointernal/git/patches_test.gointernal/git/placement_metrics_test.gointernal/git/placement_test.gointernal/git/plan_flush.gointernal/git/plan_flush_test.gointernal/git/prune_mode_test.gointernal/git/render_fidelity_test.gointernal/git/render_scope_test.gointernal/git/resync_flush.gointernal/git/resync_flush_test.gointernal/git/resync_heal_test.gointernal/git/resync_push_test.gointernal/git/secret_write_test.gointernal/git/signing.gointernal/git/signing_test.gointernal/git/source_form_test.gointernal/git/types.gointernal/git/write_boundary_precondition_test.gointernal/manifestanalyzer/gittargetignore.gointernal/ssh/auth.gointernal/ssh/auth_test.go
Contributor
Author
e2e green
against a real in-cluster Gitea over The regression test asserts the property that matters (the algorithm list is never empty, under both host-key policies) rather than re-asserting credential shape. Caveat worth carrying into review
Full status: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…address the review
Four review findings, all real:
- PrepareBranch pinned the signing policy only on repositories it CREATED.
Worker clones live on a volume across restarts, and a repository made
before the pin existed is the common case on upgrade -- either would hit
go-git v6's "cannot auto-sign commit" the first time an ambient
commit.gpgSign is true. The pin now covers the reuse path too, which is
where it matters most.
- getPushSession indexed remote.Config().URLs[0] unguarded. go-git rejects
a URL-less remote in its own validation, but a hand-edited .git/config
can still present one, and indexing it panics rather than fails.
- The pinned-known_hosts subtest could not tell a pin hit from the default
fallback, because the default set also carries the RSA algorithms. It now
asserts the list is narrower than the default and excludes ed25519, which
only the pin can produce.
- docs/INDEX.md still said "decision needed" after the design record moved
to decided-and-built, and undercounted its own list by one.
Separately, and found by porting #292's Azure DevOps examples: the credential
those examples document did not work. ADO sends a PAT as HTTP basic auth with
the token as the password and ignores the username, so its documented Secret
carries an empty username -- and firstSecretValue treats an empty value as an
absent key, so the basic-auth branch never fired and the Secret was refused
with "does not contain valid authentication data". Pre-existing on v5 too.
The password is what carries the credential, so it is what we branch on now. A
username with no password stays an error, because that one is a real mistake.
Verified against a real Azure DevOps repository, which is what the new opt-in
tests are for. Both skip themselves without a credential, so CI is unchanged:
- internal/git/ado_live_test.go walks the branch-resolution contract against
the real remote in the order the code implements it -- empty repository
resolves to nothing, an absent target falls back to the default, a present
target wins while the default is still fetched as a safety net -- and ends
on the negotiating fetch, the one request go-git v5 cannot make.
- test/e2e/ado_e2e_test.go proves the operator mirrors a live ConfigMap into
a real ADO repository. It reads the result back with canonical git rather
than our own library, so the assertion does not depend on the code under
test.
docs/azure-devops-getting-started.md is written from that e2e recipe, and says
plainly why the connectivity check can pass on a release where every fetch
fails: the advertisement is a different request that never needed multi_ack.
Refs #288
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Pushed e67adb8: all four inline review comments fixed, plus a bug the ported examples surfaced. The four review findingsAll valid. The signing-policy one was the most consequential — the pin covered only repositories The A bug the Azure DevOps examples surfacedPorting the ADO examples from #292 found that the credential those examples document did not work. ADO sends a PAT as HTTP basic auth with the token as the password and ignores the username, so its documented Secret carries an empty The password is what carries the credential, so it is what we branch on now. A username with no password stays an error. Verified against real Azure DevOps@sunib supplied a scratch ADO repository, so the belief the local simulator encodes is now checked against the real thing. Two opt-in layers, both skipping themselves without a credential, so CI is unchanged:
On the "Out of Scope Changes" warningI would push back on splitting these out. The signing pin and the SSH |
… at a time I probed ADO with a bare `want <sha>` carrying no capability list, got HTTP 200, and concluded that the six-year-old bug reports were wrong. They are not. That shape is the one row ADO accepts, and no real client sends it. Running go-git v5.19.1 against the same repository reproduced the reported failure immediately -- and not on a fetch, on the CLONE: CLONE FAILED: unexpected client error: unexpected requesting ".../git-upload-pack" status code: 400 The rule, measured one request shape at a time against a real repository: want <sha> -> 200 want <sha> side-band-64k ofs-delta agent=... -> 400 TF401041 want <sha> side-band-64k -> 400 TF401041 want <sha> agent=git/2.39.5 -> 400 TF401041 want <sha> multi_ack side-band-64k ofs-delta -> 200 want <sha> multi_ack_detailed side-band-64k -> 200 So the trigger is a capability list that omits multi_ack, not the absence of negotiation, and either capability satisfies it. v5 filters both out of the advertisement before deciding what to ask for, so every request it sends is the rejected shape. The doc keeps the wrong turn rather than quietly correcting it, because the failure mode is worth naming: probing with a hand-rolled request tests the request you built, not the behaviour you are attributing to it, and contradicting a long-standing external bug report needs a reproduction with the real client rather than a curl that disagrees. Also records what canonical git advertises (which is why it never notices this at all, and why git-http-backend is a usable stand-in), that receive-pack has no multi_ack whatsoever so pushing was never affected on any version, and the sources behind each claim. TestADOLive_StillRequiresMultiAck turns the premise into a canary: it asserts ADO STILL rejects the capability-list-without-multi_ack shape, and fails loudly with "GOOD NEWS, NOT A BUG" if Microsoft ever fixes it. Verified against the real server, which answers: TF401041: The Git protocol sent is not as expected (Clients must support multi-ack.). The public-fixture idea is dropped -- Azure DevOps no longer allows new public projects -- so the live tests stay opt-in and PAT-gated, and never run in CI. E2E_ADO_EMPTY_REPO_URL names a repository that stays empty, which is the only way to cover the empty-repository contract more than once: the main fixture seeds itself on first run. Refs #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/azure-devops-getting-started.md`:
- Around line 144-146: Update the documentation wording around
E2E_ADO_EMPTY_REPO_URL to clarify that the variable should point to a repository
that remains empty, rather than implying the variable itself must be unset or
empty; preserve the explanation that nothing writes to this repository and it
supports repeated empty-repository contract coverage.
In `@docs/facts/azure-devops-multi-ack-requirement.md`:
- Around line 148-150: Update the simulator description to say it rejects any
upload-pack POST without either capability, multi_ack or multi_ack_detailed.
Preserve the explanation that the simulator is stricter than ADO and
intentionally keeps the gating logic simple.
In `@internal/git/ado_live_test.go`:
- Around line 375-386: Bound the raw HTTP request in the canary test by
replacing the background context used around the request with a context carrying
a finite timeout, and ensure that timeout is released appropriately. Apply this
to the request created in the canary flow before http.DefaultClient.Do so
unreachable or hanging Azure DevOps calls terminate cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9100ef7c-7437-4b87-b632-68730b82fea8
📒 Files selected for processing (3)
docs/azure-devops-getting-started.mddocs/facts/azure-devops-multi-ack-requirement.mdinternal/git/ado_live_test.go
configuration.md had grown a full Azure DevOps walkthrough inline -- the Secret, the GitProvider, Entra, SSH, and the multi_ack background -- duplicating azure-devops-getting-started.md a screen below the GitProvider example. github-setup-guide.md already establishes the pattern: per-provider setup is its own page, and configuration.md points at it. So the section goes, and what stays is the single thing that surprises people, placed where it bites: a note under the credentials-Secret auth-keys table, which is exactly where a reader learns HTTP basic means username + password and needs to know Azure DevOps is the exception that carries only a password. Also wires E2E_ADO_EMPTY_REPO_URL through the getting-started guide, and names TestADOLive_StillRequiresMultiAck as a canary there, so someone hitting a red build knows a failure means Microsoft fixed their end rather than that something broke. Verified against the real fixtures at dev.azure.com/configbutler/tests: all four live tests pass, including the empty-repository case against a repository that stays empty and the canary, which reports TF401041: The Git protocol sent is not as expected (Clients must support multi-ack.). and the operator-level e2e spec mirrors a live ConfigMap into the repository (1 passed, 0 failed). Refs #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cted themselves
Three review findings, all valid:
- The canary had no deadline on either half: context.Background() and
http.DefaultClient's zero Timeout. An unreachable or mid-response Azure
DevOps would hang the test rather than reaching the skip its error path
already intends. Bounded at 30s.
- "E2E_ADO_EMPTY_REPO_URL must stay empty" reads as an instruction to unset
the variable, two lines below the command that sets it. It is the
REPOSITORY it names that must stay empty.
- The facts page claimed the simulator "rejects any upload-pack POST without
multi_ack", which contradicted its own measured table two screens above:
the check matches the multi_ack PREFIX, so multi_ack_detailed satisfies it,
exactly as ADO does. Verified rather than assumed -- "multi_ack" is a
substring of "multi_ack_detailed", and the simulator's test is
bytes.Contains.
The third is the one worth noticing: a page whose entire purpose is to record
what was measured had a summary sentence disagreeing with its own table.
Refs #288
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback. The guide read as a tour rather than a path a newcomer can
follow, and advertised two credential types nobody has tested against Azure
DevOps.
- SSH and Entra bearer tokens are no longer presented as setup steps. They
use the same Secret keys as any other provider and probably work exactly
as they do for GitHub, but nothing here has exercised them against ADO, so
they are named once and labelled untested rather than walked through.
- `kubectl create namespace` is now in the flow. The Secret command failed
without it.
- The branch is a `<branch>` placeholder used in both resources, instead of
a hard-coded `main` the reader had no reason to think was a choice.
- The PAT step says which organization, to set an expiry, to copy the token
when shown, and that it inherits its user's repository permissions.
- security-model.md claimed HTTP basic auth requires `username`, which
contradicts the Azure PAT contract this branch introduced. It now says the
password is what selects basic auth, and that ADO omits the username.
Both pages are shorter: 155 -> 125 lines for the guide, and the facts page
loses the essay while keeping the measurements and sources.
One review point is not applied, because the evidence contradicts it: audit
delivery is NOT a prerequisite for the final ConfigMap to produce a commit.
Attribution is what needs audit; writes do not. The chart defaults to
configured-author with attribution disabled, and test/e2e/ado_e2e_test.go sets
up no audit at all yet passes by reading the committed manifest back out of
Azure DevOps. The guide now says this explicitly, since the reviewer is
unlikely to be the last person to assume otherwise.
Refs #288
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s carve-out The docs insisted on leaving `username` out, which overstates what the code does and framed a general rule as provider-specific. Read back: nothing in CredentialFromSecretData mentions Azure DevOps. It branches on `password`, and passes whatever `username` it found straight through. So all three shapes work -- `password` alone, `username: ""` with a password, and a real username with a password -- and only a username WITHOUT a password is an error. Measured against a real ADO repository, `CheckRepo` is accepted with the username set to "", "pat", "anything-at-all", and the account's own name. ADO genuinely ignores it, so "do not add a username" was advice for a problem that does not exist. configuration.md now states the rule where the auth-keys table is, since that table said HTTP basic needs `username` + `password` and thereby contradicted it: the password is what selects basic auth, the username is optional, and an empty value is the same as an absent key to the credential reader. That last point is the only genuinely surprising part, and it is a property of firstSecretValue rather than of Azure DevOps. The getting-started guide drops the warning and says the username is optional and ignored, which is both shorter and true. A test now covers the case that had none: a supplied username must survive into the credential. The three ADO-shaped cases were already covered; the ordinary one was not, which is how the docs drifted from the code unnoticed. Refs #288 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/facts/azure-devops-multi-ack-requirement.md`:
- Around line 130-133: Update the simulator note to describe the check as
matching the raw “multi_ack” substring rather than a prefix, including the
multi_ack_detailed form. Keep the existing explanation of the simulator’s
stricter behavior and its acceptance of the bare-want shape unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fc86b96-af1f-4476-bcbb-67d05fe39941
📒 Files selected for processing (7)
docs/README.mddocs/azure-devops-getting-started.mddocs/configuration.mddocs/facts/azure-devops-multi-ack-requirement.mddocs/security-model.mdinternal/git/ado_live_test.gointernal/git/credentials_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/configuration.md
- docs/azure-devops-getting-started.md
- internal/git/credentials_test.go
Comment on lines
+130
to
+133
| The simulator is deliberately stricter than ADO: it rejects any `upload-pack` POST carrying neither | ||
| `multi_ack` nor `multi_ack_detailed` — its check matches the `multi_ack` prefix, so either satisfies | ||
| it, exactly as ADO does — including the bare-`want` shape ADO accepts. That difference does not matter for what it | ||
| gates, and the strictness is what keeps it simple. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file existence / size =="
ls -l docs/facts/azure-devops-multi-ack-requirement.md || true
wc -l docs/facts/azure-devops-multi-ack-requirement.md || true
echo "== relevant section =="
sed -n '1,180p' docs/facts/azure-devops-multi-ack-requirement.md | cat -n
echo "== search simulator implementation mentions =="
rg -n --hidden --glob '!vendor/**' --glob '!dist/**' --glob '!build/**' 'multi_ack|upload-pack|Contains' .Repository: ConfigButler/gitops-reverser
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("docs/facts/azure-devops-multi-ack-requirement.md")
text = p.read_text() if p.exists() else ""
print("contains 'prefix' or 'prefix' in relevant document:")
for i, line in enumerate(text.splitlines(), 1):
if "prefix" in line.lower() or "substring" in line.lower() or "bytes.Contains" in line:
print(f"{i}: {line}")
# Behavioral probe for Go semantics implied by bytes.Contains:
# raw substring would treat any string containing "multi_ack" as matching,
# including false-positive keys like "nomulti_ack", which a token-aware prefix/prefix-list check should not.
samples = [
"multi_ack",
"multi_ack_detailed",
"multi_ack_detailed_and_something",
"nomulti_ack",
"multi_ack_in_middle",
"wantedmulti_ack",
]
needle = "multi_ack"
for s in samples:
print(f"{s!r}: contains={needle in s}, plausible_single_token_boundary={s.count('_') == 1 and s.startswith(needle) and s[9:10] in (' ','\t','>', '<', '/', '?', '&') and (len(s)==9 or s[9:][0].isupper())}")
PYRepository: ConfigButler/gitops-reverser
Length of output: 677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== multi_ack simulator / ADO proxy candidates =="
rg -n --hidden --glob '!vendor/**' --glob '!dist/**' --glob '!build/**' \
'(multi_ack_detailed|multi_ack|UploadPack|git-upload-pack|400|TF401041|upload-pack)' internal test pkg cmd docs | \
head -n 200
echo "== focused filenames =="
git ls-files | rg '(^|/)(ado_multiack|multiack|multi-ack|multi_ack).*\.go$|ado|multi_ack'Repository: ConfigButler/gitops-reverser
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
needle = "multi_ack"
samples = [
"multi_ack",
"multi_ack_detailed",
"multi_ack_detailed_and_something",
"nomulti_ack",
"multi_ack_in_middle",
"wantedmulti_ack",
]
for s in samples:
print(f"{s!r}: contains={needle in s}")
PYRepository: ConfigButler/gitops-reverser
Length of output: 378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod go version =="
sed -n '1,40p' go.mod 2>/dev/null || true
echo "== ADO multi_ack files by name =="
git ls-files | rg -in 'multi[ _-]?ack|mack|ado.*multi|multi.*ado' || true
echo "== focused exact matches for multi_ack_detailed literal =="
rg -n --hidden --glob '!vendor/**' --glob '!dist/**' --glob '!build/**' 'multi_ack_detailed|multi_ack' internal cmd test docs --glob '*.go' --glob '!*.svg' --glob '!*.cast' | head -n 200Repository: ConfigButler/gitops-reverser
Length of output: 5527
Replace “prefix” with substring in the simulator note.
The simulator contract uses bytes.Contains(body, []byte("multi_ack")), including multi_ack_detailed, so calling this a prefix match overstates the check’s precision. Document the raw multi_ack substring behavior or tighten the simulator.
🧰 Tools
🪛 LanguageTool
[style] ~132-~132: To elevate your writing, try using an alternative expression here.
Context: ...ant` shape ADO accepts. That difference does not matter for what it gates, and the strictness i...
(MATTERS_RELEVANT)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/facts/azure-devops-multi-ack-requirement.md` around lines 130 - 133,
Update the simulator note to describe the check as matching the raw “multi_ack”
substring rather than a prefix, including the multi_ack_detailed form. Keep the
existing explanation of the simulator’s stricter behavior and its acceptance of
the bare-want shape unchanged.
sunib
changed the title
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Fixes #288 by moving to go-git v6 (
v6.0.0-alpha.5), which implements themulti_ackcapabilityAzure DevOps insists on. Alternative to #292, which reaches for a system
gitbinary instead; oncego-git#1204 landed, upstream deleted their own ADO workaround example saying it "works out of the box".
multi_ackis in every v6 tag, and the churn in the fourteen packages we import has settled — 96 → 39→ 1 → 9 exported removals per alpha, the one breaking wave being the transport rewrite in
spring. v5 is still released alongside (
v5.19.2on alpha.5's day), so backing out stays possible.What v6 changes
The API moves were mechanical. The interesting part is that v6 reads ambient system state that v5
ignored, and fails closed — twice, in places no unit test can reach.
transport.AuthMethod[]client.Option— closures, so not inspectableNewReceivePackSession→AdvertisedReferences→ReceivePackHandshake→GetRemoteRefs→Push, same*packp.Commandcommit.gpgSignSigneris nil; refuses the commit if set with no signerHostKeyAlgorithmsknown_hostseven when a callback is set; hard-fails if absentfile://transportgit-receive-packcmd.OldThe one that would have shipped broken. v6 loads
~/.ssh/known_hostsand/etc/ssh/ssh_known_hoststo deriveHostKeyAlgorithmswheneverClientConfigreturns it empty —including when we already supplied a
HostKeyCallback— and fails the connection when neither fileexists. Our image is distroless with neither, so every SSH remote would have failed regardless of the
credential.
ssh.KeyAuthnow always populates the list: from the pinnedknown_hostswhen there isone, a modern default set otherwise.
Unit tests cannot see this. The fallback lives in the transport's
connect, not inClientConfig, sobuilding a credential and asserting on it passes cleanly — two of mine did. The e2e SSH spec against a
real Gitea is what caught it.
commit.gpgSignis the same shape: any machine or image with it set globally breaks every commit wemake.
PinExplicitSigningPolicywrites the repo-local valuefalse, so our signing policy comes fromthe GitProvider rather than from whatever gitconfig the process can see.
file://stopped being a real server, which silently weakened a test. v6's in-processreceive-packonly checks that a ref exists and then sets it — it never comparescmd.Old— so racingpushes all win, and
TestBranchWorker_ConcurrentOperationswas passing with 2 commits where it asserts4. It now runs against a real git server, and anything asserting the compare-and-swap must avoid
file://. Probably worth reporting upstream.The atomic push itself is unchanged: one session still serves both the advertisement and the push, and
PushRequest.Commandstakes the same*packp.Command, so the server-sideOld/Newcompare-and-swapis verbatim. v6 also negotiates
report-statusitself and returns a rejected command as the error fromPush, collapsing our separate status inspection into one check, andAtomicis now a first-classfield.
The test, without an Azure DevOps tenant
Nobody here has an ADO org, so the failure is reproduced locally. Canonical git's own
upload-packadvertises
multi_ack, which makesgit-http-backendbehind a proxy enforcing ADO's rule a faithfulsimulator for both halves: the proxy reproduces the rejected request, the real backend reproduces the
multi-ACK response v5 also cannot parse. The v2 opt-in header is stripped so a v2-capable client cannot
sidestep the capability under test.
TestADOSimulator_IsFaithfulTestADO_CheckRepo_NeedsNoNegotiationTestADO_PushAtomic_NeedsNoMultiAckTestADO_SmartFetch_RequiresMultiAckTF401041The two middle ones passing on v5 locate the bug: only
repo.Fetchwas ever affected.CheckReporeads just the ref advertisement, and
receive-packhas nomulti_ackat all.Background and the options considered:
docs/design/azure-devops-multi-ack.md.I also tested it live with a PAT and it works, and even attribution is shown nicely in ADO as well:
Type of Change
Testing
task linttask testtask test-e2e(default leg): 79 passed, 22 skipped, 0 failedtask test-image-refreshlocally: 6 passed, 0 failed — the CI shard's first failure was abring-up flake (apiserver
EOFduringflux-operator install, zero Ginkgo reports, no spec ran)Related Issues
Closes #288
Alternative to #292
🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests