fix(#1625): exclude agent working directories from git tracking - #1627
Conversation
Site previewPreview: https://39d4fccc-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsLow
Info
Previous runReviewFindingsHigh
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| echo "Changed files:" | ||
| echo "${CHANGED_FILES}" | sed 's/^/ /' | ||
|
|
||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
[high] correctness
Section 2b ("Strip agent working directories") detects agent artifacts and logs warnings, but never actually removes them from the commit. STRIPPED_FILES is set but never consumed — no git rm, git reset HEAD, or filtering of CHANGED_FILES follows. The defense-in-depth layer is inert.
Suggested fix: After the detection loop, use git rm --cached on each file in STRIPPED_FILES and amend the commit, or filter CHANGED_FILES to exclude them before the push step.
| } | ||
| if len(lines) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[low] security
excludeAgentWorkingDirs constructs a shell command via fmt.Sprintf with payload and repoDir interpolated into a single-quoted string. Safe with current hardcoded values and consistent with existing patterns, but fragile if a future agentWorkingDirExcludes entry contains a single quote.
Suggested fix: Consider writing patterns to a temp file and appending via cat, or escaping the payload.
| assert.NoError(t, validateLinuxBinary(binPath), "downloaded binary should be a valid Linux/amd64 ELF") | ||
| } | ||
|
|
||
| func TestAgentWorkingDirExcludes_ContainsKnownPatterns(t *testing.T) { |
There was a problem hiding this comment.
[low] correctness
Go tests verify the agentWorkingDirExcludes slice contents but do not test the excludeAgentWorkingDirs function itself. Shell command generation, error handling, and sandbox interaction are untested.
Suggested fix: Add a unit test that exercises excludeAgentWorkingDirs with a mock sandbox, verifying the generated command and error paths.
|
/fs-fix Resolve the conflicts |
|
/fs-fix |
Section 2b detected agent working directory artifacts but never removed them — STRIPPED_FILES was populated but not acted upon. Now uses git rm --cached to unstage artifacts and amends the commit, then rebuilds CHANGED_FILES so downstream steps (secret scan, pre-commit) operate only on legitimate files. Addresses review feedback on #1627 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: fullsend-fix <fullsend-code@users.noreply.github.com>
🔧 Fix agent — iteration 1 (human-triggered)Fixed the HIGH finding: post-code.sh section 2b now actually removes agent artifacts via git rm --cached and amends the commit, instead of just logging warnings. Disagreed with 2 LOW findings as out of scope for this bug-fix PR (shell injection risk with hardcoded values, and missing Go unit test for sandbox-dependent function). Fixed (1):
Disagreed (2):
Tests: passed Updated by fullsend fix agent |
| } | ||
| if len(lines) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[low] command-injection
excludeAgentWorkingDirs constructs a shell command via fmt.Sprintf with payload and repoDir interpolated into a single-quoted string. Currently safe with hardcoded values and consistent with the existing AGENTS.md exclude pattern (line 425), but fragile if a future agentWorkingDirExcludes entry contains a single quote.
Suggested fix: Write patterns to a temp file and append via cat, or use an exec-style API that avoids shell interpolation.
| assert.NoError(t, validateLinuxBinary(binPath), "downloaded binary should be a valid Linux/amd64 ELF") | ||
| } | ||
|
|
||
| func TestAgentWorkingDirExcludes_ContainsKnownPatterns(t *testing.T) { |
There was a problem hiding this comment.
[low] test-adequacy
Go tests verify the agentWorkingDirExcludes slice contents but do not test the excludeAgentWorkingDirs function itself. The function shell command generation, error handling, and sandbox interaction remain untested.
Suggested fix: Add a unit test for excludeAgentWorkingDirs that mocks sandbox.Exec and verifies the generated command string.
| @@ -1331,6 +1347,25 @@ func relOrAbs(base, path string) string { | |||
| return rel | |||
| } | |||
There was a problem hiding this comment.
[low] variable-construction-idiom
The loop copies agentWorkingDirExcludes into a new lines slice without transformation. Simplify to payload := strings.Join(agentWorkingDirExcludes, newline) and remove the lines variable.
| expected := []string{".agentready/", ".fullsend-workspace/"} | ||
| for _, pattern := range expected { | ||
| found := false | ||
| for _, exclude := range agentWorkingDirExcludes { |
There was a problem hiding this comment.
[low] test-assertion-style
Manual loop with found flag to check slice membership can be replaced with assert.Contains(t, agentWorkingDirExcludes, pattern) which is more idiomatic.
fullsend-ai-review
Bot
added
the
ready-for-merge
The code agent was committing its own working artifacts (e.g. .agentready/) into target repositories, causing review churn and requiring multiple fix iterations to clean up. Two-layer defense: 1. Harness-level (internal/cli/run.go): Add known agent working directory patterns (.agentready/, .fullsend-workspace/) to .git/info/exclude before the agent starts execution. This makes git completely ignore these directories — git status and git add will not see them. 2. Post-script (scripts/post-code.sh): Defense-in-depth check that detects and warns if agent artifacts somehow appear in the commit's changed files, as a safety net if the exclude mechanism is bypassed. The agentWorkingDirExcludes variable is a centralized list that can be extended as new agent working directories are identified. Closes #1625
Section 2b detected agent working directory artifacts but never removed them — STRIPPED_FILES was populated but not acted upon. Now uses git rm --cached to unstage artifacts and amends the commit, then rebuilds CHANGED_FILES so downstream steps (secret scan, pre-commit) operate only on legitimate files. Addresses review feedback on #1627 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: fullsend-fix <fullsend-code@users.noreply.github.com>
6c9b74d to
691782c
Compare
|
Review skipped — this PR is already merged. The Posted by fullsend post-review check |
Retro: PR #1627 — Exclude agent working directories from git trackingTimeline: Issue #1625 → code agent PR #1627 (05-28) → review CHANGES_REQUESTED with 1 HIGH + 2 LOW (05-28) → human approval + What went well:
Friction points:
Skipped proposals (already tracked):
Proposals filed
|
1 participant
The code agent was committing its own working artifacts (e.g. .agentready/) into target repositories, causing review churn and requiring multiple fix iterations to clean up.
Two-layer defense:
Harness-level (internal/cli/run.go): Add known agent working
directory patterns (.agentready/, .fullsend-workspace/) to
.git/info/exclude before the agent starts execution. This
makes git completely ignore these directories — git status
and git add will not see them.
Post-script (scripts/post-code.sh): Defense-in-depth check
that detects and warns if agent artifacts somehow appear in
the commit's changed files, as a safety net if the exclude
mechanism is bypassed.
The agentWorkingDirExcludes variable is a centralized list that can be extended as new agent working directories are identified.
Closes #1625
Post-script verification
agent/1625-exclude-agent-work-dirs)8480e16153a3f16c66b5484bfd4c76349a89933d..HEAD)