fix: isolate git credential helper chain for scoped repos - #2188
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesGit credential configuration now tracks origins and values, classifies managed state, validates ownership, verifies updates, and rolls back failures. Git credential configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CredentialHelper
participant GitConfig
participant GitExecutable
CredentialHelper->>GitConfig: call SetHelper or UnsetHelper with gitHTTPURL and appID
GitConfig->>GitExecutable: read origin-tagged helper and useHttpPath values
GitExecutable-->>GitConfig: return configuration state
GitConfig->>GitExecutable: update or remove managed configuration
GitConfig->>GitExecutable: verify readback or restore prior state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
github-actions
Bot
added
the
size/L
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2188 +/- ##
==========================================
+ Coverage 75.85% 76.02% +0.16%
==========================================
Files 958 966 +8
Lines 101701 103017 +1316
==========================================
+ Hits 77150 78319 +1169
- Misses 18684 18746 +62
- Partials 5867 5952 +85 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8a767566b62657af82f9ee6dffb3ca6f08e9ce26🧩 Skill updatenpx skills add larksuite/cli#fix/apps-git-credential-helper-chain -y -g |
b65948a to
e8dd45d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shortcuts/apps/gitcred/gitconfig.go (1)
70-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the immediate second read.
Lines 63 and 70 read the configuration twice with no write between them. The comparison at line 74 therefore cannot detect a concurrent change that the readback at line 91 would miss. It only adds two more
git configsubprocesses perSetHelpercall.If the intent is to detect external changes, the readback at lines 87-93 already provides that. If the intent is different, add a comment that states it.
♻️ Proposed simplification
if classifyManagedState(snapshot, canonical) == managedNone { return gitConfigNotOwnedError(normalizedURL) } - current, err := readCredentialConfig(ctx, normalizedURL) - if err != nil { - return err - } - if !reflect.DeepEqual(current, snapshot) { - return gitConfigChangedError(normalizedURL) - }🤖 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 `@shortcuts/apps/gitcred/gitconfig.go` around lines 70 - 76, Remove the redundant immediate read and comparison around readCredentialConfig in SetHelper, including the current/snapshot DeepEqual check and gitConfigChangedError return. Retain the later readback comparison after the configuration write as the sole external-change check.shortcuts/apps/git_credential_test.go (1)
1234-1258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared fake-git setup.
installAppsFakeGitUnsetUseHTTPPathFailurerepeats every step ofinstallAppsFakeGit:exec.LookPath("git"), the temp bin directory, the script write, the isolated global config, and the threet.Setenvcalls. Only the shellcasepattern differs. A change to the isolation setup now has to be applied in two places.♻️ Proposed refactor
+// installAppsFakeGitWithCase installs a git wrapper on PATH that delegates to +// the real git, except for argv matching failCase, and isolates global config. +func installAppsFakeGitWithCase(t *testing.T, failCase string) { + t.Helper() + realGit, err := exec.LookPath("git") + if err != nil { + t.Fatalf("find real git: %v", err) + } + dir := t.TempDir() + script := fmt.Sprintf("#!/bin/sh\n%sexec %q \"$@\"\n", failCase, realGit) + if err := os.WriteFile(filepath.Join(dir, "git"), []byte(script), 0o700); err != nil { + t.Fatalf("write fake git: %v", err) + } + globalConfig := filepath.Join(t.TempDir(), "global.config") + if err := os.WriteFile(globalConfig, nil, 0o600); err != nil { + t.Fatalf("write isolated global git config: %v", err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("GIT_CONFIG_GLOBAL", globalConfig) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") +} + +func installAppsFakeGit(t *testing.T, failUseHTTPPathExit int) { + t.Helper() + failCase := "" + if failUseHTTPPathExit != 0 { + failCase = fmt.Sprintf("case \"$*\" in\n *\"useHttpPath true\"*) exit %d ;;\nesac\n", failUseHTTPPathExit) + } + installAppsFakeGitWithCase(t, failCase) +} + +func installAppsFakeGitUnsetUseHTTPPathFailure(t *testing.T, exitCode int) { + t.Helper() + installAppsFakeGitWithCase(t, fmt.Sprintf("case \"$*\" in\n *\"--unset-all\"*\"useHttpPath\"*) exit %d ;;\nesac\n", exitCode)) +}🤖 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 `@shortcuts/apps/git_credential_test.go` around lines 1234 - 1258, Extract the shared fake-git setup from installAppsFakeGitUnsetUseHTTPPathFailure and installAppsFakeGit into a reusable helper that handles git lookup, temporary script installation, isolated global config creation, and all PATH/GIT_CONFIG environment setup. Keep the failure helper’s distinct shell case pattern and exit-code behavior, while having both helpers reuse the common setup.
🤖 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 `@shortcuts/apps/gitcred/gitconfig_test.go`:
- Around line 208-214: Update the malformed-output test cases around
parseOriginValues to retain the returned error and validate it with the existing
assertProblemSubtype helper, asserting the InternalError metadata includes
errs.SubtypeExternalTool. Also verify the underlying cause is preserved through
errs.ProblemOf, while keeping the existing non-nil error assertion behavior.
---
Nitpick comments:
In `@shortcuts/apps/git_credential_test.go`:
- Around line 1234-1258: Extract the shared fake-git setup from
installAppsFakeGitUnsetUseHTTPPathFailure and installAppsFakeGit into a reusable
helper that handles git lookup, temporary script installation, isolated global
config creation, and all PATH/GIT_CONFIG environment setup. Keep the failure
helper’s distinct shell case pattern and exit-code behavior, while having both
helpers reuse the common setup.
In `@shortcuts/apps/gitcred/gitconfig.go`:
- Around line 70-76: Remove the redundant immediate read and comparison around
readCredentialConfig in SetHelper, including the current/snapshot DeepEqual
check and gitConfigChangedError return. Retain the later readback comparison
after the configuration write as the sole external-change check.
🪄 Autofix
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: 6f74e3dd-d6ba-4a2b-a4b9-1e81a11309d6
📒 Files selected for processing (5)
shortcuts/apps/git_credential_test.goshortcuts/apps/gitcred/gitconfig.goshortcuts/apps/gitcred/gitconfig_test.goshortcuts/apps/gitcred/gitcred_test.goshortcuts/apps/gitcred/helper.go
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
e8dd45d to
64206ac
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Good catch — done in 64206ac. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
shortcuts/apps/gitcred/gitconfig_test.go (1)
788-797: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the problem category in
assertProblemSubtype.The helper checks only
problem.Subtype. Every error-path test in this file routes through it, so no test pins the error category. A change that kept the subtype but moved the error to a different category would still pass.Add the expected category as a parameter, or assert it inline where the category is fixed for the call site.
♻️ Proposed refactor
-func assertProblemSubtype(t *testing.T, err error, subtype errs.Subtype) { +func assertProblem(t *testing.T, err error, category errs.Category, subtype errs.Subtype) { t.Helper() if err == nil { - t.Fatalf("error = nil, want subtype %s", subtype) + t.Fatalf("error = nil, want category %s subtype %s", category, subtype) } problem, ok := errs.ProblemOf(err) - if !ok || problem.Subtype != subtype { - t.Fatalf("problem = %#v, ok = %v, want subtype %s", problem, ok, subtype) + if !ok || problem.Category != category || problem.Subtype != subtype { + t.Fatalf("problem = %#v, ok = %v, want category %s subtype %s", problem, ok, category, subtype) } }
Paramis not available onProblem, so do not assert it here. If a call site needsParam, useerrors.Aswith*errs.ValidationErrorinstead.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam)". Based on learnings: "errs.ProblemOf(err)returns(*errs.Problem, bool)and the returnedProblemcontains only problem-level fields ... it does not haveParam."🤖 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 `@shortcuts/apps/gitcred/gitconfig_test.go` around lines 788 - 797, Update assertProblemSubtype to accept the expected errs.Category and assert problem.Category alongside problem.Subtype via errs.ProblemOf. Update every call site to provide its expected category; do not assert Param on errs.Problem, using errors.As with *errs.ValidationError only where a call site specifically requires Param.Sources: Coding guidelines, Learnings
🤖 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 `@shortcuts/apps/gitcred/gitconfig_test.go`:
- Around line 395-396: Update the helper script contents in
TestGlobalGitConfigResetFirstHelperStopsGlobalFill to drain stdin with cat
>/dev/null before executing printf, for both globalHelper and scopedHelper.
Preserve their existing logging behavior and executable permissions so approve
and erase operations complete without broken-pipe failures.
- Around line 254-285: Update TestWritableGlobalOrigin to set USERPROFILE
alongside HOME for Windows variants, using the test home directory so
vfs.UserHomeDir() resolves consistently through os.UserHomeDir(). Keep the
existing environment setup and non-Windows fixtures unchanged.
- Around line 596-603: Update the nonCanonical path construction in the test to
build the “./global.config” suffix using string(filepath.Separator) instead of
hardcoded forward slashes, while preserving the intentionally non-canonical path
used for GIT_CONFIG_GLOBAL.
---
Nitpick comments:
In `@shortcuts/apps/gitcred/gitconfig_test.go`:
- Around line 788-797: Update assertProblemSubtype to accept the expected
errs.Category and assert problem.Category alongside problem.Subtype via
errs.ProblemOf. Update every call site to provide its expected category; do not
assert Param on errs.Problem, using errors.As with *errs.ValidationError only
where a call site specifically requires Param.
🪄 Autofix
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: 29289f4f-917c-4274-9fcb-b514a0d5e5c4
📒 Files selected for processing (5)
shortcuts/apps/git_credential_test.goshortcuts/apps/gitcred/gitconfig.goshortcuts/apps/gitcred/gitconfig_test.goshortcuts/apps/gitcred/gitcred_test.goshortcuts/apps/gitcred/helper.go
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/apps/gitcred/gitcred_test.go
- shortcuts/apps/gitcred/helper.go
- shortcuts/apps/git_credential_test.go
- shortcuts/apps/gitcred/gitconfig.go
64206ac to
97c69cd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/apps/gitcred/gitconfig_test.go (1)
817-831: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the failure-injection wrapper fail loudly if it never matches.
The script matches the exact positional form
git config --global <useHttpPath key> true. The three rollback tests depend on this match to inject exit code 23.If
gitConfigSetlater changes its argument order or adds a flag such as--replace-all, the branch stops matching. The wrapper then forwards the call to the real git,SetHelpersucceeds, andassertExternalToolExitreports a generic "want git exit code 23" failure that does not indicate the harness itself went stale. The--unset-allbranch has the same coupling.Record a marker file when the injection branch fires, then assert that the marker exists. The tests then distinguish "rollback behaviour regressed" from "the wrapper no longer intercepts the call".
♻️ Proposed refactor
script := `#!/bin/sh if [ "$1" = "config" ] && [ "$2" = "--global" ] && [ "$3" = "$GIT_TEST_USE_PATH_KEY" ] && [ "$4" = "true" ]; then + : > "$GIT_TEST_INJECTED_MARKER" if [ "$GIT_TEST_EXTERNAL_CHANGE" = "1" ]; then "$GIT_TEST_REAL_GIT" config --global --add "$GIT_TEST_HELPER_KEY" '!external-change' fi exit 23 fiSet
GIT_TEST_INJECTED_MARKERin the helper, return its path, and check it in each rollback test:markerPath := installGitConfigUseHTTPPathFailure(t, url, false, false) // ... after SetHelper if _, err := os.Stat(markerPath); err != nil { t.Fatalf("failure injection never fired; the git wrapper no longer matches the real command: %v", err) }🤖 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 `@shortcuts/apps/gitcred/gitconfig_test.go` around lines 817 - 831, Update the failure-injection helper around installGitConfigUseHTTPPathFailure to create and expose a marker path via GIT_TEST_INJECTED_MARKER, and have the wrapper write the marker when the exit-23 interception branch executes. Return that path to each rollback test and assert it exists after SetHelper, so unmatched git argument changes fail with an explicit harness error; apply equivalent coverage to the --unset-all injection branch as needed.
🤖 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 `@shortcuts/apps/gitcred/gitconfig_test.go`:
- Around line 476-482: The redaction assertion in the rollback error test checks
the already-validated static hint instead of the complete error chain. Keep the
exact rollbackFailureHint assertion, but change the sensitive-rollback-detail
check to inspect err.Error() so leaked stderr content in the error message
causes the test to fail.
- Around line 20-21: Update the test fixture helpers in gitconfig_test.go to use
standard filesystem APIs: replace vfs.MkdirAll and vfs.WriteFile in
writeTestFileMode with os.MkdirAll and os.WriteFile, and replace all remaining
test-only vfs.ReadFile calls with os.ReadFile while preserving the existing
fs.ErrNotExist handling.
---
Nitpick comments:
In `@shortcuts/apps/gitcred/gitconfig_test.go`:
- Around line 817-831: Update the failure-injection helper around
installGitConfigUseHTTPPathFailure to create and expose a marker path via
GIT_TEST_INJECTED_MARKER, and have the wrapper write the marker when the exit-23
interception branch executes. Return that path to each rollback test and assert
it exists after SetHelper, so unmatched git argument changes fail with an
explicit harness error; apply equivalent coverage to the --unset-all injection
branch as needed.
🪄 Autofix
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: 2dd906f5-5b70-4347-b4b1-c0a5504cd9cd
📒 Files selected for processing (5)
shortcuts/apps/git_credential_test.goshortcuts/apps/gitcred/gitconfig.goshortcuts/apps/gitcred/gitconfig_test.goshortcuts/apps/gitcred/gitcred_test.goshortcuts/apps/gitcred/helper.go
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/apps/gitcred/helper.go
- shortcuts/apps/git_credential_test.go
- shortcuts/apps/gitcred/gitconfig.go
- shortcuts/apps/gitcred/gitcred_test.go
97c69cd to
9b40210
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/apps/gitcred/gitconfig_test.go (1)
259-262: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the redundant
globalassignment.Lines 259-262 replace
globalEnvwith itself when non-empty; setglobal := tc.globalEnvand skip the conditional.🤖 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 `@shortcuts/apps/gitcred/gitconfig_test.go` around lines 259 - 262, In the test setup around tc.globalEnv, remove the conditional reassignment and initialize global directly from tc.globalEnv. Preserve the existing value unchanged for both empty and non-empty inputs.
🤖 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.
Nitpick comments:
In `@shortcuts/apps/gitcred/gitconfig_test.go`:
- Around line 259-262: In the test setup around tc.globalEnv, remove the
conditional reassignment and initialize global directly from tc.globalEnv.
Preserve the existing value unchanged for both empty and non-empty inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0470350a-a93c-4b63-8a92-9ac7d9010542
📒 Files selected for processing (5)
shortcuts/apps/git_credential_test.goshortcuts/apps/gitcred/gitconfig.goshortcuts/apps/gitcred/gitconfig_test.goshortcuts/apps/gitcred/gitcred_test.goshortcuts/apps/gitcred/helper.go
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/apps/gitcred/helper.go
- shortcuts/apps/git_credential_test.go
- shortcuts/apps/gitcred/gitconfig.go
- shortcuts/apps/gitcred/gitcred_test.go
bca8ce3 to
cab0ee4
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@shortcuts/apps/gitcred/gitconfig.go`:
- Around line 455-467: Update repositionLarkSection and its
SetHelper/rollbackIfUnchanged flow to preserve every key in the URL-scoped
credential section when repositioning, including keys not represented by Helpers
or UseHTTPPath; capture the full section before removal and restore untracked
entries alongside the canonical helper values. If faithful preservation cannot
be implemented, refuse repositioning with the repository’s typed validation
error rather than deleting unsupported keys.
- Around line 424-425: Update the scopedKey construction near genericKey so it
does not lowercase the full gitCredentialKey result: preserve the URL subsection
exactly as returned by gitCredentialKey while lowercasing only the section and
final key component as required. Add a regression test covering a URL with an
uppercase path segment and verify the helper update/revert flow succeeds without
triggering gitConfigHelperOrderUnsafeError.
In `@shortcuts/apps/gitcred/lock.go`:
- Around line 84-92: Update lockGlobalConfig to resolve the origin path to an
absolute form after removing the file: prefix and before applying
safeLockNameChars and constructing the lock filename, ensuring relative
GIT_CONFIG_GLOBAL values map to the same cross-process lock regardless of
working directory.
🪄 Autofix
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: 3282f622-e0ac-409c-83a5-4edd6ac41fd1
📒 Files selected for processing (6)
shortcuts/apps/git_credential_test.goshortcuts/apps/gitcred/gitconfig.goshortcuts/apps/gitcred/gitconfig_test.goshortcuts/apps/gitcred/gitcred_test.goshortcuts/apps/gitcred/helper.goshortcuts/apps/gitcred/lock.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/apps/gitcred/helper.go
- shortcuts/apps/git_credential_test.go
- shortcuts/apps/gitcred/gitcred_test.go
554467a to
ddbc98e
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
shortcuts/apps/gitcred/helper.go (1)
147-147: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider falling back to the validated
appIDwhen the record has none.Both call sites read the app ID from the stored record.
UnsetHelpernow callsvalidate.ResourceName(appID, "appID"), so an empty or stalerecord.AppIDmakes cleanup fail and only surfaces aConfigWarning; the URL-scoped helper stays in the global Git config.
previouscomes fromcurrentAppRecord(appID)andrecordscomes fromFindByAppID(appID, ...), so the validatedappIDparameter is the same value for every record. Use it when the record field is empty.♻️ Proposed change (Line 147)
- if err := m.GitConfig.UnsetHelper(ctx, previous.GitHTTPURL, previous.AppID); err != nil { + previousAppID := previous.AppID + if previousAppID == "" { + previousAppID = appID + } + if err := m.GitConfig.UnsetHelper(ctx, previous.GitHTTPURL, previousAppID); err != nil { result.ConfigWarning = err.Error() }Also applies to: 178-178
🤖 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 `@shortcuts/apps/gitcred/helper.go` at line 147, Update both UnsetHelper call sites in the cleanup flow to use the validated appID parameter when previous.AppID or the corresponding record AppID is empty, while preserving non-empty stored IDs. Ensure cleanup passes this fallback consistently to GitConfig.UnsetHelper so URL-scoped helpers are removed.shortcuts/apps/git_credential_test.go (1)
1205-1232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the real-git wrappers for platform and git availability.
Both helpers now require the real
gitbinary and a POSIX shell. Two gaps follow:
exec.LookPath("git")failure callst.Fatalf, so the whole test fails on an environment without git. The previous synthetic wrapper had no such dependency. Uset.Skipfor a missinggit, and skip on Windows, asgitconfig_test.godoes for its shell wrappers.- The
case "$*"patterns depend on the exact argv join.installAppsFakeGitmatchesuseHttpPath true, which holds only while the code callsgit config --global <key> true. If the argument form changes, the wrapper stops injecting the failure and the test still passes while asserting the warning path. Match the discrete arguments instead, asinstallGitConfigUseHTTPPathFailureingitconfig_test.godoes.♻️ Proposed guard
func installAppsFakeGit(t *testing.T, failUseHTTPPathExit int) { t.Helper() + if runtime.GOOS == "windows" { + t.Skip("test wrapper is a POSIX shell script") + } realGit, err := exec.LookPath("git") if err != nil { - t.Fatalf("find real git: %v", err) + t.Skipf("real git is not available: %v", err) }Also applies to: 1234-1257
🤖 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 `@shortcuts/apps/git_credential_test.go` around lines 1205 - 1232, Update installAppsFakeGit and the related real-git wrapper helper to skip on Windows and use t.Skip when exec.LookPath("git") cannot find git, matching the platform guards in gitconfig_test.go. Replace the case "$*" substring matching for useHttpPath true with discrete-argument matching equivalent to installGitConfigUseHTTPPathFailure, so failure injection remains tied to the intended arguments rather than their joined representation.shortcuts/apps/gitcred/gitconfig.go (1)
95-101: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider dropping the duplicate ownership read.
Lines 88 and 95 call
readCredentialConfigtwice with no write between them, and both reads happen underlockGlobalConfig. The comparison at Line 99 can only detect an external write that lands between two adjacent reads. The readback at Line 135 already covers divergence after the writes. Each extra read costs twogit configsubprocesses perSetHelpercall.If the double read is intentional for a specific race, add a comment that names it.
🤖 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 `@shortcuts/apps/gitcred/gitconfig.go` around lines 95 - 101, Remove the second readCredentialConfig call and its current-vs-snapshot comparison from the SetHelper flow, since no write occurs between the reads and the later readback already verifies post-write divergence. Preserve the initial ownership/configuration read and existing error handling; only retain the duplicate read if a documented race requires it, with a comment naming that race.
🤖 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 `@shortcuts/apps/gitcred/gitconfig.go`:
- Around line 501-509: Update sectionHasUntrackedKeys to pass Git’s
--no-includes option to the global config --get-regexp command, ensuring the
scan only examines the editable global file and matches the scope affected by
--remove-section.
In `@shortcuts/apps/gitcred/lock.go`:
- Around line 84-91: Wrap the error returned by acquireFileLock in
lockGlobalConfig with errs.NewInternalError using errs.SubtypeStorage, a
descriptive lock-acquisition message, and WithCause(err). Preserve the existing
directory-creation handling and return the typed error so SetHelper and
UnsetHelper receive consistent metadata.
---
Nitpick comments:
In `@shortcuts/apps/git_credential_test.go`:
- Around line 1205-1232: Update installAppsFakeGit and the related real-git
wrapper helper to skip on Windows and use t.Skip when exec.LookPath("git")
cannot find git, matching the platform guards in gitconfig_test.go. Replace the
case "$*" substring matching for useHttpPath true with discrete-argument
matching equivalent to installGitConfigUseHTTPPathFailure, so failure injection
remains tied to the intended arguments rather than their joined representation.
In `@shortcuts/apps/gitcred/gitconfig.go`:
- Around line 95-101: Remove the second readCredentialConfig call and its
current-vs-snapshot comparison from the SetHelper flow, since no write occurs
between the reads and the later readback already verifies post-write divergence.
Preserve the initial ownership/configuration read and existing error handling;
only retain the duplicate read if a documented race requires it, with a comment
naming that race.
In `@shortcuts/apps/gitcred/helper.go`:
- Line 147: Update both UnsetHelper call sites in the cleanup flow to use the
validated appID parameter when previous.AppID or the corresponding record AppID
is empty, while preserving non-empty stored IDs. Ensure cleanup passes this
fallback consistently to GitConfig.UnsetHelper so URL-scoped helpers are
removed.
🪄 Autofix
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: 5b7053ac-ce6f-4c99-8008-b8fbb5a31cbc
📒 Files selected for processing (6)
shortcuts/apps/git_credential_test.goshortcuts/apps/gitcred/gitconfig.goshortcuts/apps/gitcred/gitconfig_test.goshortcuts/apps/gitcred/gitcred_test.goshortcuts/apps/gitcred/helper.goshortcuts/apps/gitcred/lock.go
Scoped repos created by Miaoda apps failed day-2 `git pull/push`
authentication because a stale, higher-priority credential helper (e.g.
macOS osxkeychain) served cached credentials instead of the lark-cli
helper. This isolates the URL-scoped credential helper chain and closes
correctness gaps found in review:
1. Helper-order isolation. Git's empty-helper ("") reset only clears
helpers that parse BEFORE the lark-cli section; a generic
credential.helper (or one from a later [include]) that parses AFTER it
still participated in get/store/erase. SetHelper now verifies, via the
faithful parse-order oracle (`git config --includes --show-origin -z
--list`), that the lark-cli helper is last in fill order. If not, it
repositions the section to the end of the writable file; if a later
helper lives in a file we must not edit, it fails closed with a
FailedPrecondition error and restores the prior state.
2. Concurrent read-modify-write. The read-modify-write of the writable
global config file is serialized across lark-cli processes by a new
cross-process lockGlobalConfig, ordered after lockApp and before
lockURL. That lock cannot stop an unrelated (non-lark-cli) process, so
the helper rewrite also no longer clears the whole key: it deletes only
the values observed in the ownership snapshot by exact match, leaving a
helper a third party inserted during the write window in place. The
readback then diverges from the expected state and SetHelper fails
closed with the foreign value preserved, instead of a whole-key
--unset-all silently deleting it.
3. Recoverable teardown. UnsetHelper now deletes useHttpPath before the
helper list so a mid-teardown failure leaves a lark-cli-recoverable
residue (never a useHttpPath-only orphan that blocks re-init), and
removes only lark-cli values from a mixed list, reporting that a
third-party helper remains rather than silently no-op'ing. The state
taxonomy gains Foreign/Partial/Mixed to drive these paths.
ddbc98e to
8a76756
Compare
Scoped repos created by Miaoda apps failed day-2 `git pull/push`
authentication because a stale, higher-priority credential helper (e.g.
macOS osxkeychain) served cached credentials instead of the lark-cli
helper. This isolates the URL-scoped credential helper chain and closes
correctness gaps found in review:
1. Helper-order isolation. Git's empty-helper ("") reset only clears
helpers that parse BEFORE the lark-cli section; a generic
credential.helper (or one from a later [include]) that parses AFTER it
still participated in get/store/erase. SetHelper now verifies, via the
faithful parse-order oracle (`git config --includes --show-origin -z
--list`), that the lark-cli helper is last in fill order. If not, it
repositions the section to the end of the writable file; if a later
helper lives in a file we must not edit, it fails closed with a
FailedPrecondition error and restores the prior state.
2. Concurrent read-modify-write. The read-modify-write of the writable
global config file is serialized across lark-cli processes by a new
cross-process lockGlobalConfig, ordered after lockApp and before
lockURL. That lock cannot stop an unrelated (non-lark-cli) process, so
the helper rewrite also no longer clears the whole key: it deletes only
the values observed in the ownership snapshot by exact match, leaving a
helper a third party inserted during the write window in place. The
readback then diverges from the expected state and SetHelper fails
closed with the foreign value preserved, instead of a whole-key
--unset-all silently deleting it.
3. Recoverable teardown. UnsetHelper now deletes useHttpPath before the
helper list so a mid-teardown failure leaves a lark-cli-recoverable
residue (never a useHttpPath-only orphan that blocks re-init), and
removes only lark-cli values from a mixed list, reporting that a
third-party helper remains rather than silently no-op'ing. The state
taxonomy gains Foreign/Partial/Mixed to drive these paths.
Co-authored-by: TRAE CLI <traecli@bytedance.com>
2 participants
Summary
apps +git-credential-initpreviously configured a single URL-scoped Gitcredential helper, which left other global helpers participating in
get/store/erasefor the repo. This isolates the credential helper chainso only lark-cli handles credentials for the configured repository URL.
Changes
the helper list, then add the lark-cli helper) together with
useHttpPath=true, so earlier global helpers no longer participate for therepo URL.
third-party, mixed, cross-origin, or include-sourced values fail closed
without modifying user config.
on a mid-write failure (unless an external change is detected).
relative
GIT_CONFIG_GLOBALstill matches.Review-driven correctness fixes
Three gaps were found in review and fixed in this PR:
"") resetonly clears helpers that parse before the lark-cli section; a generic
credential.helper(or one from a later[include]) that parses after itstill participated in
get/store/erase.SetHelpernow verifies, usingthe faithful parse-order oracle (
git config --includes --show-origin -z --list), that the lark-cli helper is last in fill order. If it is not, thesection is repositioned to the end of the writable global file; if a
competing helper lives in a file we must not edit,
SetHelperfails closedwith a
FailedPreconditionerror and restores the prior state — it neverleaves a written-but-ineffective config.
--add. Theoptimistic read followed by an unconditional
--unset-allcould drop aconcurrent write.
SetHelper/UnsetHelpernow serialize theread-modify-write of the writable global config file under a new
cross-process
lockGlobalConfig(ordered afterlockApp, beforelockURL).UnsetHelpernow deletesuseHttpPathbefore the helper list, so amid-teardown failure leaves a lark-cli-recoverable residue rather than a
useHttpPath-only orphan that would block re-init. For a helper list thatmixes lark-cli and third-party values, it removes only the lark-cli values
and reports (non-fatally) that a third-party helper remains, instead of
silently no-op'ing. The managed-state taxonomy gains
Foreign/Partial/Mixedto drive these paths.Residual / known limitation
lockGlobalConfigserializes concurrent lark-cli writers on the same globalconfig file. It does not serialize non-lark-cli writers (e.g. a user running
git configconcurrently); such a race is detected by the post-write readback,which fails closed with
FailedPreconditionrather than silently clobbering theforeign change.
Test Plan
make unit-testgo vet ./...,gofmt,go mod tidy(no changes)go test ./shortcuts/apps/gitcred/... -racerollback, cross-origin/include-source refusal, later-helper isolation and
fail-closed, concurrent-writer serialization, recoverable teardown,
mixed-owned cleanup, real
git credential fill, and app-id shell-quotingRelated Issues
N/A
Summary by CodeRabbit
Bug Fixes
Tests