feat: git sync foundation (Plan A of issue #16)#133
Merged
Conversation
Adds the 'git' credential type with token, ssh_key, and username fields, plus a RequireAtLeastOne validator ensuring token or ssh_key is always present. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements the first CRUD method on the repo.Store type. Create validates name and poll_interval before touching the database, inserts the row, emits a repo.added audit event inside the same transaction, and returns the persisted Repo struct. Four integration tests cover the happy path, duplicate name rejection, and both validation guards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Clarify that Plan A ships registry + CLI only (sync engine lands later) - Rewrite --prune flag help to state the positive behavior - Mark webhook_secret as sensitive in Repo struct - Reject repo URLs that embed credentials inline Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds GitOps repository management: CLI commands (repos add/list/status/remove), persistent Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (mantle repos)
participant Store as Repo Store
participant DB as Postgres
participant Audit as Audit subsystem
CLI->>Store: Create/List/Get/Update/Delete request
Store->>DB: Begin TX / Execute SQL
DB-->>Store: Row(s) or error
Store->>Audit: Emit audit event (within same TX)
Store->>DB: Commit TX
DB-->>Store: Commit ack
Store-->>CLI: Return result / formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
packages/engine/internal/db/migrations/019_git_repos.sql (1)
32-33: Partial index onenabledalone is likely low-value.Since
enableddefaults toTRUE, the partial predicateWHERE enabled = TRUEwill match nearly every row, so this index won't offer much selectivity over a seq scan for the sync engine's "find enabled repos" query in Plan B. A composite(team_id, enabled) WHERE enabled = TRUE(or just relying onidx_git_repos_teamplus a runtime filter) is usually the shape that actually gets used by the planner. Worth revisiting when Plan B defines the actual sync scan query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/db/migrations/019_git_repos.sql` around lines 32 - 33, The partial index idx_git_repos_enabled on git_repos(enabled) WHERE enabled = TRUE is low-value because enabled defaults to TRUE; replace or change it to a composite partial index that includes team_id (e.g., idx_git_repos_team_enabled on git_repos(team_id, enabled) WHERE enabled = TRUE) so the planner can use team_id + enabled for the “find enabled repos” queries, or remove the redundant idx_git_repos_enabled and rely on idx_git_repos_team plus a runtime enabled filter; update the migration to create the composite index (or drop the partial index) accordingly.packages/engine/internal/config/config.go (1)
36-36: No env-var binding forgit_sync.*keys.All other new config sections added in this repo (storage, retention, engine., auth.oidc.) have explicit
v.BindEnvcalls inLoad.git_sync.reposdoesn't — which is fine given it's a list (Viper can't meaningfully bindMANTLE_GIT_SYNC_REPOS), but flagging for awareness so this doesn't get called out as an inconsistency in Plan B review. A short code comment noting "list-valued; file-only" would preempt that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/config/config.go` at line 36, The GitSyncConfig field (GitSync GitSyncConfig `mapstructure:"git_sync"`) lacks env-var bindings in Load; add a short explanatory comment either above the GitSyncConfig type or next to the GitSync field in config.go stating that git_sync.repos is list-valued and therefore intentionally only read from files (Viper cannot bind list env vars), so no v.BindEnv call is required—this keeps Load()’s omission of v.BindEnv for git_sync.* explicit and avoids reviewer confusion.packages/engine/internal/cli/root.go (1)
48-60:reposlikely belongs outside the "Workflow Lifecycle" group.
mantle repos add|list|status|removeis a registry/admin surface, not part of thevalidate → plan → apply → runlifecycle that the "workflow" group is scoped to (per the help text on line 16 and CLAUDE.md). Grouping it withenvworks, but consider either placing it underadminalongsidesecrets/teams/users, or adding a dedicatedgitopsgroup so the help output stays legible as Plans B and C add more surface area. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/root.go` around lines 48 - 60, The repos commands (newReposCommand) are grouped under the "workflow" addToGroup call but belong to admin/gitops surface, so remove newReposCommand from the addToGroup(..., "workflow", ...) call and instead register it with a more appropriate group (e.g., an "admin" or "gitops" addToGroup invocation alongside newEnvCommand or the secrets/teams/users group); locate the addToGroup call that currently lists newValidateCommand/newPlanCommand/... and take newReposCommand out of that list and add it to the chosen separate group registration so help output reflects correct command grouping.packages/engine/internal/secret/types_test.go (1)
106-123: LGTM; optionally add an empty-value case.The positive/negative coverage is sufficient. Consider also asserting that
{"token": "", "ssh_key": ""}fails, sinceValidatetreats empty strings as unset — this pins that behavior against future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/secret/types_test.go` around lines 106 - 123, Update the TestGitCredentialType_ValidateRequiresTokenOrSSHKey test to include an additional negative case that passes both "token" and "ssh_key" with empty string values and asserts ct.Validate returns an error; locate the test by name (TestGitCredentialType_ValidateRequiresTokenOrSSHKey) and the Validate call on the ct value returned by GetType("git") and add an assertion similar to the existing negative case but using map[string]string{"token": "", "ssh_key": ""} to ensure empty strings are treated as unset.packages/engine/internal/config/config_test.go (1)
500-504: Consider asserting the remaining parsed fields.The YAML sets
branch,path,poll_interval, andprune, but the assertion only checksName,URL,Branch,Credential, andAutoApply. Adding checks forPath,PollInterval, andPrunewould guard against a regression in any onemapstructuretag (e.g., a future rename/typo onpoll_interval).♻️ Proposed tightening
- if r.Name != "acme" || r.URL != "https://github.com/acme/workflows.git" || - r.Branch != "main" || r.Credential != "github-pat" || !r.AutoApply { - t.Errorf("parsed repo: %+v", r) - } + if r.Name != "acme" || r.URL != "https://github.com/acme/workflows.git" || + r.Branch != "main" || r.Path != "/" || r.PollInterval != "60s" || + r.Credential != "github-pat" || !r.AutoApply || !r.Prune { + t.Errorf("parsed repo: %+v", r) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/config/config_test.go` around lines 500 - 504, The test currently only checks some fields of cfg.GitSync.Repos[0] (variable r); extend the assertion to also validate r.Path, r.PollInterval and r.Prune to match the YAML and prevent regressions in mapstructure tags; update the test around cfg.GitSync.Repos[0] (where r is defined) to assert these fields (and use t.Errorf/t.Fatalf as appropriate) so the parsed repo includes Path, PollInterval and Prune with the expected values.packages/engine/internal/cli/repos_test.go (2)
119-121: Consider asserting no stray content in empty-state output.
strings.Containspasses as long as(no repos)appears anywhere, so a stray table header wouldn't fail this test. Given the CLI prints either(no repos)or the table (mutually exclusive), a stricter check (e.g.,strings.TrimSpace(stdout.String()) == "(no repos)") would catch regressions where both paths get executed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/repos_test.go` around lines 119 - 121, The test currently uses strings.Contains on stdout.String() which allows stray output; change the assertion to require the trimmed stdout exactly equals "(no repos)" by comparing strings.TrimSpace(stdout.String()) to that literal so the test fails if a header or extra content is present, and keep the t.Errorf to print the full stdout on mismatch (referencing stdout.String()).
82-94: Optional: extract the "seed one repo via add" block into a helper.The same inline
repos add acme ...seeding block is repeated verbatim acrossTestReposList_ShowsRegisteredRepos,TestReposStatus_ShowsDetails,TestReposRemove_RequiresYesFlag, andTestReposRemove_DeletesRow. A smallseedRepo(t, cfg, name)helper would reduce noise and make intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/repos_test.go` around lines 82 - 94, Extract the repeated "seed one repo via add" block into a helper function (e.g., seedRepo(t *testing.T, cfg Config, name string)) that constructs NewRootCommand(), sets its stderr buffer, prepares args like {"repos","add", name, "--url", "https://example.com/a.git", "--credential", "pat", "--database-url", cfg.Database.URL"}, calls root.Execute(), and fails the test on error using t.Fatalf with the stderr contents; replace the four inline blocks in TestReposList_ShowsRegisteredRepos, TestReposStatus_ShowsDetails, TestReposRemove_RequiresYesFlag, and TestReposRemove_DeletesRow with calls to seedRepo to reduce duplication and clarify intent while keeping behavior identical.packages/engine/internal/cli/repos.go (1)
98-102: Optional: addLong/Exampletostatusandlistfor parity withadd/remove.
statusandlistonly setShort, whileaddandremoveincludeLong(andaddalso includesExample). Small polish for a consistent--helpsurface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/repos.go` around lines 98 - 102, Add the missing detailed help text to the repo commands so CLI help is consistent: update newReposStatusCommand and the repos list command (e.g., newReposListCommand) to set Long (a longer description) and optionally Example fields similar to newReposAddCommand/newReposRemoveCommand (and match the Example used by add if appropriate). Ensure the Long string describes the command behavior in more detail than Short and, for parity with add, include an Example demonstrating typical usage of the status and list commands.packages/engine/internal/repo/types.go (1)
19-21: Optional: reject trailing-/_in repo names.
^[a-z0-9][a-z0-9_-]*$accepts values likeacme-orfoo_. Given the stated intent to embed names into log lines, metric labels, and URL path segments without escaping, tightening the tail of the pattern avoids awkward boundary characters.♻️ Suggested tightening
-var validRepoNamePattern = regexp.MustCompile(`^[a-z0-9][a-z0-9_-]*$`) +var validRepoNamePattern = regexp.MustCompile(`^[a-z0-9]([a-z0-9_-]*[a-z0-9])?$`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/repo/types.go` around lines 19 - 21, The current validRepoNamePattern allows repo names to end with '-' or '_' (e.g., "acme-"); update the regexp assigned to validRepoNamePattern so the last character must be an alphanumeric to reject trailing hyphens/underscores. Replace the current pattern (`regexp.MustCompile(\`^[a-z0-9][a-z0-9_-]*$\`)`) with a tightened pattern that requires the final character be [a-z0-9] while still allowing single-character names (for example by using a grouped optional middle segment), and update the variable validRepoNamePattern accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/engine/internal/config/config.go`:
- Around line 71-92: Load currently parses cfg.GitSync.Repos but doesn't run the
repo validators; update the Load function to iterate over cfg.GitSync.Repos
(GitSyncConfig / GitSyncRepo) and for each entry call the existing validators
repo.ValidateName, repo.ValidatePollInterval and validateURL (or the repo
package equivalent) and return an error if any validation fails so Load fails
fast; if importing internal/repo causes a cycle, either lift the validators to a
lower-level package or inline at minimum the URL credential rejection logic here
before returning the parsed config.
In `@packages/engine/internal/db/migrations/019_git_repos.sql`:
- Around line 11-30: Update the git_repos table definition so the team_id
foreign key uses an explicit ON DELETE RESTRICT; specifically modify the team_id
column definition (symbol: team_id in table git_repos) to read REFERENCES
teams(id) ON DELETE RESTRICT (keeping the existing DEFAULT
'00000000-0000-0000-0000-000000000001' and NOT NULL), so the column becomes
something like team_id UUID NOT NULL REFERENCES teams(id) ON DELETE RESTRICT
DEFAULT '00000000-0000-0000-0000-000000000001'.
In `@packages/engine/internal/repo/store.go`:
- Around line 166-172: The Update method (Store.Update) currently writes
p.Branch and p.Path even when empty; fix by either validating non-empty
p.Branch/p.Path and returning an error, or by preserving existing values when
incoming fields are empty: fetch the current repo (e.g., via s.Get(ctx, name) or
the repository getter used in this file), and before persisting, set branch :=
p.Branch; if branch=="" then branch = existing.Branch (same for path := p.Path
-> existing.Path); keep existing validation for PollInterval and Credential
as-is and ensure the final values written come from these resolved branch/path
variables.
- Around line 183-194: The Update SQL and Scan in the tx.QueryRowContext call
(function/method Update, variables tx, r) currently omit last_sync_sha,
last_sync_at, last_sync_error, and webhook_secret from the RETURNING list and
Scan, so returned *Repo values lose those fields; update the RETURNING clause to
include last_sync_sha, last_sync_at, last_sync_error, webhook_secret exactly as
Get does and extend the Scan to accept corresponding destinations (including
using the same nullable scanners/nullable types or helpers that Get uses) so
Update returns a fully populated Repo matching Get's column set.
- Around line 257-272: The validateURL function currently only rejects URLs with
a password but allows username-only userinfo (e.g., GitHub PAT in the username);
change validateURL to reject any non-empty u.User (i.e., if u.User != nil return
an error directing operators to use the Credential field/--credential) so no
credentials can be embedded in git_repos.url, and update the test
TestStore_Create_AllowsURLWithUsernameOnly to expect rejection (or adjust any
callers that relied on username hints); alternatively, if you choose to strip
userinfo instead of rejecting, ensure the strip happens before
persisting/auditing (where git_repos.url is set) so credentials never land in
storage.
---
Nitpick comments:
In `@packages/engine/internal/cli/repos_test.go`:
- Around line 119-121: The test currently uses strings.Contains on
stdout.String() which allows stray output; change the assertion to require the
trimmed stdout exactly equals "(no repos)" by comparing
strings.TrimSpace(stdout.String()) to that literal so the test fails if a header
or extra content is present, and keep the t.Errorf to print the full stdout on
mismatch (referencing stdout.String()).
- Around line 82-94: Extract the repeated "seed one repo via add" block into a
helper function (e.g., seedRepo(t *testing.T, cfg Config, name string)) that
constructs NewRootCommand(), sets its stderr buffer, prepares args like
{"repos","add", name, "--url", "https://example.com/a.git", "--credential",
"pat", "--database-url", cfg.Database.URL"}, calls root.Execute(), and fails the
test on error using t.Fatalf with the stderr contents; replace the four inline
blocks in TestReposList_ShowsRegisteredRepos, TestReposStatus_ShowsDetails,
TestReposRemove_RequiresYesFlag, and TestReposRemove_DeletesRow with calls to
seedRepo to reduce duplication and clarify intent while keeping behavior
identical.
In `@packages/engine/internal/cli/repos.go`:
- Around line 98-102: Add the missing detailed help text to the repo commands so
CLI help is consistent: update newReposStatusCommand and the repos list command
(e.g., newReposListCommand) to set Long (a longer description) and optionally
Example fields similar to newReposAddCommand/newReposRemoveCommand (and match
the Example used by add if appropriate). Ensure the Long string describes the
command behavior in more detail than Short and, for parity with add, include an
Example demonstrating typical usage of the status and list commands.
In `@packages/engine/internal/cli/root.go`:
- Around line 48-60: The repos commands (newReposCommand) are grouped under the
"workflow" addToGroup call but belong to admin/gitops surface, so remove
newReposCommand from the addToGroup(..., "workflow", ...) call and instead
register it with a more appropriate group (e.g., an "admin" or "gitops"
addToGroup invocation alongside newEnvCommand or the secrets/teams/users group);
locate the addToGroup call that currently lists
newValidateCommand/newPlanCommand/... and take newReposCommand out of that list
and add it to the chosen separate group registration so help output reflects
correct command grouping.
In `@packages/engine/internal/config/config_test.go`:
- Around line 500-504: The test currently only checks some fields of
cfg.GitSync.Repos[0] (variable r); extend the assertion to also validate r.Path,
r.PollInterval and r.Prune to match the YAML and prevent regressions in
mapstructure tags; update the test around cfg.GitSync.Repos[0] (where r is
defined) to assert these fields (and use t.Errorf/t.Fatalf as appropriate) so
the parsed repo includes Path, PollInterval and Prune with the expected values.
In `@packages/engine/internal/config/config.go`:
- Line 36: The GitSyncConfig field (GitSync GitSyncConfig
`mapstructure:"git_sync"`) lacks env-var bindings in Load; add a short
explanatory comment either above the GitSyncConfig type or next to the GitSync
field in config.go stating that git_sync.repos is list-valued and therefore
intentionally only read from files (Viper cannot bind list env vars), so no
v.BindEnv call is required—this keeps Load()’s omission of v.BindEnv for
git_sync.* explicit and avoids reviewer confusion.
In `@packages/engine/internal/db/migrations/019_git_repos.sql`:
- Around line 32-33: The partial index idx_git_repos_enabled on
git_repos(enabled) WHERE enabled = TRUE is low-value because enabled defaults to
TRUE; replace or change it to a composite partial index that includes team_id
(e.g., idx_git_repos_team_enabled on git_repos(team_id, enabled) WHERE enabled =
TRUE) so the planner can use team_id + enabled for the “find enabled repos”
queries, or remove the redundant idx_git_repos_enabled and rely on
idx_git_repos_team plus a runtime enabled filter; update the migration to create
the composite index (or drop the partial index) accordingly.
In `@packages/engine/internal/repo/types.go`:
- Around line 19-21: The current validRepoNamePattern allows repo names to end
with '-' or '_' (e.g., "acme-"); update the regexp assigned to
validRepoNamePattern so the last character must be an alphanumeric to reject
trailing hyphens/underscores. Replace the current pattern
(`regexp.MustCompile(\`^[a-z0-9][a-z0-9_-]*$\`)`) with a tightened pattern that
requires the final character be [a-z0-9] while still allowing single-character
names (for example by using a grouped optional middle segment), and update the
variable validRepoNamePattern accordingly.
In `@packages/engine/internal/secret/types_test.go`:
- Around line 106-123: Update the
TestGitCredentialType_ValidateRequiresTokenOrSSHKey test to include an
additional negative case that passes both "token" and "ssh_key" with empty
string values and asserts ct.Validate returns an error; locate the test by name
(TestGitCredentialType_ValidateRequiresTokenOrSSHKey) and the Validate call on
the ct value returned by GetType("git") and add an assertion similar to the
existing negative case but using map[string]string{"token": "", "ssh_key": ""}
to ensure empty strings are treated as unset.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15a2d429-ec50-42f7-81db-3f8efc443c80
📒 Files selected for processing (13)
CLAUDE.mdpackages/engine/internal/audit/audit.gopackages/engine/internal/cli/repos.gopackages/engine/internal/cli/repos_test.gopackages/engine/internal/cli/root.gopackages/engine/internal/config/config.gopackages/engine/internal/config/config_test.gopackages/engine/internal/db/migrations/019_git_repos.sqlpackages/engine/internal/repo/store.gopackages/engine/internal/repo/store_test.gopackages/engine/internal/repo/types.gopackages/engine/internal/secret/types.gopackages/engine/internal/secret/types_test.go
This was referenced Apr 18, 2026
GitHub PATs are valid as a username in HTTPS URLs (user@host), so the previous check (hasPassword only) left a bypass. Now any u.User != nil is rejected. Renamed TestStore_Create_AllowsURLWithUsernameOnly to _RejectsURLWithUsernameOnly and flipped the assertion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… clause The Update method was returning a partial Repo that silently dropped last_sync_sha/at/error and webhook_secret. The RETURNING clause now mirrors Get's SELECT list, and the subsequent scan populates the nullable fields using sql.NullString/NullTime with the same pattern as Get. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… empty Empty Branch/Path in UpdateParams previously overwrote the stored value with an empty string. Now a CASE expression in the SQL keeps the stored value when the caller passes ''. This is equivalent to Option B (preserve-on-empty) without a second round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without an explicit action the FK defaulted to NO ACTION (functionally RESTRICT but implicit). Adding RESTRICT makes the intent unambiguous: deleting a team with registered repos is a hard error, not silent cascade. Operators must remove repos before removing the team. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Load now iterates cfg.GitSync.Repos and validates each entry: - repo.ValidateName for the name field - url is required and must not embed userinfo (same rule as store.Create) - repo.ValidatePollInterval when poll_interval is non-empty No import cycle: internal/repo production code does not import internal/config (only repo's _test.go does, which is excluded from the dependency graph). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… enabled) Rename idx_git_repos_enabled to idx_git_repos_team_enabled and make it a composite (team_id, enabled) partial index. Queries that filter by team and enabled status now hit a single index instead of intersecting two. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…group; comment GitSync env binding - repos status and repos list were missing Long descriptions and Example blocks; add both, styled to match repos add. - newReposCommand belongs with admin/gitops infra config, not the workflow lifecycle group; move it to the admin group in root.go. - Add inline comment on GitSyncConfig.Repos explaining why no BindEnv calls are present (Viper cannot bind list-valued env vars). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-string git cred case
- TestLoadConfig_GitSyncRepos was only asserting Name, URL, Branch,
Credential, AutoApply. Add explicit checks for Path, PollInterval,
and Prune so the full parsed struct is verified.
- TestGitCredentialType_ValidateRequiresTokenOrSSHKey gains a case
for {"token": "", "ssh_key": ""} — present-but-empty values must
also fail the RequireAtLeastOne check.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- TestReposList_EmptyState: replace strings.Contains with an exact strings.TrimSpace equality check so stray whitespace or extra output would surface as a failure rather than passing silently. - Extract the repeated four-line "seed a repo via add" block into a seedRepo(t, cfg, name) helper; replace all four inline copies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d underscores Change from ^[a-z0-9][a-z0-9_-]*$ to ^[a-z0-9]([a-z0-9_-]*[a-z0-9])?$ so that names like "my-" or "repo_" are rejected. The updated pattern requires the last character to be alphanumeric while still permitting single-character names (pure alnum). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/engine/internal/config/config.go (1)
360-382: Missingcredentialfail-fast check — config path is looser than CLI.
Store.Create(store.go lines 59-61) rejects an emptyCredential, butLoadhere does not. An operator can commit amantle.yamlwith agit_sync.reposentry that omitscredential, and it will parse cleanly today — the error surfaces only when Plan B tries to materialize it. The same fail-fast reasoning that addedValidateName/URL/ValidatePollIntervalhere applies tocredential.Optionally, detecting duplicate names upfront would also be cheap insurance (the
git_repos_team_name_keyUNIQUE constraint catches it at DB time, but not until materialization).♻️ Suggested
for i, r := range cfg.GitSync.Repos { if err := repo.ValidateName(r.Name); err != nil { return nil, fmt.Errorf("git_sync.repos[%d]: %w", i, err) } if r.URL == "" { return nil, fmt.Errorf("git_sync.repos[%d] (%q): url is required", i, r.Name) } if parsed, err := url.Parse(r.URL); err != nil { return nil, fmt.Errorf("git_sync.repos[%d] (%q): invalid url: %w", i, r.Name, err) } else if parsed.User != nil { return nil, fmt.Errorf("git_sync.repos[%d] (%q): url must not embed credentials", i, r.Name) } + if r.Credential == "" { + return nil, fmt.Errorf("git_sync.repos[%d] (%q): credential is required", i, r.Name) + } if r.PollInterval != "" { if err := repo.ValidatePollInterval(r.PollInterval); err != nil { return nil, fmt.Errorf("git_sync.repos[%d] (%q): %w", i, r.Name, err) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/config/config.go` around lines 360 - 382, The git_sync.Repos validation currently checks Name, URL, and PollInterval but does not fail-fast when a repo's Credential is empty; add a check in the same loop over cfg.GitSync.Repos to return an error if r.Credential == "" (e.g., fmt.Errorf("git_sync.repos[%d] (%q): credential is required", i, r.Name)) so behavior matches Store.Create's rejection of empty Credential; optionally also detect duplicate repo names by tracking seen names in a map and returning a similar error if a duplicate is found to catch the UNIQUE constraint earlier.packages/engine/internal/cli/repos.go (2)
130-135:SHAwill render blank whenLastSyncSHAis empty butLastSyncAtis set.If a future sync path writes
last_sync_atwithout a SHA (e.g., a sync that failed after receiving the timestamp),statusprintsLast Sync: 2026-04-20 ... UTC (SHA ). Low-impact since Plan A never populates these, but a guard here would be cheap insurance against surprises in Plan B:♻️ Suggested
if r.LastSyncAt != nil { - fmt.Fprintf(out, "Last Sync: %s (SHA %s)\n", - r.LastSyncAt.UTC().Format("2006-01-02 15:04:05 UTC"), r.LastSyncSHA) + ts := r.LastSyncAt.UTC().Format("2006-01-02 15:04:05 UTC") + if r.LastSyncSHA != "" { + fmt.Fprintf(out, "Last Sync: %s (SHA %s)\n", ts, r.LastSyncSHA) + } else { + fmt.Fprintf(out, "Last Sync: %s\n", ts) + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/repos.go` around lines 130 - 135, The current output always appends " (SHA %s)" even when r.LastSyncSHA is empty, producing "SHA " blanks; in the printing logic in repos.go that checks r.LastSyncAt (the fmt.Fprintf call that uses r.LastSyncAt.UTC().Format(...) and r.LastSyncSHA), change the formatting to conditionally include the " (SHA ...)" suffix only when r.LastSyncSHA is non-empty (e.g., check r.LastSyncSHA != "" and use one fmt.Fprintf variant with the SHA or another without it) so the status line omits the empty SHA token.
33-46: Cleanup closure dropsdatabase.Close()error.Silently discarding the close error is fine for the CLI happy path, but it does hide failures like "broken pipe during tx commit flush" during debugging. Consider logging at debug level:
♻️ Suggested
- cleanup := func() { database.Close() } + cleanup := func() { + if err := database.Close(); err != nil { + slog.Debug("closing repo store db", "error", err) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/repos.go` around lines 33 - 46, The cleanup closure in newRepoStore currently ignores the error returned by database.Close(); change the cleanup function to capture the Close() error and log it at debug level instead of discarding it. Locate newRepoStore and its cleanup closure, call database.Close() into a variable, and emit a debug log including that error (use the package's existing logger from context/config if available, otherwise use the standard logger) so shutdown errors like "broken pipe during tx commit flush" are visible when debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/engine/internal/repo/store.go`:
- Around line 278-293: The validateURL function currently rejects any URL with
u.User != nil which incorrectly blocks legitimate SSH URLs like
ssh://git@host/..., so change the logic in validateURL to only treat embedded
userinfo as an error for HTTP(S) schemes (check u.Scheme == "http" || u.Scheme
== "https") while allowing SSH schemes (e.g., "ssh") to have a username; apply
the identical change to the corresponding validator in config.go (the
validateURL/URL validation function there), and add unit tests covering
ssh://git@host/path.git and ssh://host/path.git to ensure both forms are
accepted while HTTP(S) URLs with user:pass remain rejected.
---
Nitpick comments:
In `@packages/engine/internal/cli/repos.go`:
- Around line 130-135: The current output always appends " (SHA %s)" even when
r.LastSyncSHA is empty, producing "SHA " blanks; in the printing logic in
repos.go that checks r.LastSyncAt (the fmt.Fprintf call that uses
r.LastSyncAt.UTC().Format(...) and r.LastSyncSHA), change the formatting to
conditionally include the " (SHA ...)" suffix only when r.LastSyncSHA is
non-empty (e.g., check r.LastSyncSHA != "" and use one fmt.Fprintf variant with
the SHA or another without it) so the status line omits the empty SHA token.
- Around line 33-46: The cleanup closure in newRepoStore currently ignores the
error returned by database.Close(); change the cleanup function to capture the
Close() error and log it at debug level instead of discarding it. Locate
newRepoStore and its cleanup closure, call database.Close() into a variable, and
emit a debug log including that error (use the package's existing logger from
context/config if available, otherwise use the standard logger) so shutdown
errors like "broken pipe during tx commit flush" are visible when debugging.
In `@packages/engine/internal/config/config.go`:
- Around line 360-382: The git_sync.Repos validation currently checks Name, URL,
and PollInterval but does not fail-fast when a repo's Credential is empty; add a
check in the same loop over cfg.GitSync.Repos to return an error if r.Credential
== "" (e.g., fmt.Errorf("git_sync.repos[%d] (%q): credential is required", i,
r.Name)) so behavior matches Store.Create's rejection of empty Credential;
optionally also detect duplicate repo names by tracking seen names in a map and
returning a similar error if a duplicate is found to catch the UNIQUE constraint
earlier.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22e9adbd-952a-4278-87a1-8ccd402d9664
📒 Files selected for processing (10)
packages/engine/internal/cli/repos.gopackages/engine/internal/cli/repos_test.gopackages/engine/internal/cli/root.gopackages/engine/internal/config/config.gopackages/engine/internal/config/config_test.gopackages/engine/internal/db/migrations/019_git_repos.sqlpackages/engine/internal/repo/store.gopackages/engine/internal/repo/store_test.gopackages/engine/internal/repo/types.gopackages/engine/internal/secret/types_test.go
✅ Files skipped from review due to trivial changes (4)
- packages/engine/internal/config/config_test.go
- packages/engine/internal/db/migrations/019_git_repos.sql
- packages/engine/internal/cli/repos_test.go
- packages/engine/internal/repo/store_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/engine/internal/secret/types_test.go
- packages/engine/internal/cli/root.go
Comment on lines
+278
to
+293
| // validateURL rejects URLs that embed any userinfo (user or user:pass). | ||
| // GitHub PATs are valid as a username in HTTPS URLs, so even a bare | ||
| // "user@host" form must be rejected. Operators must store credential | ||
| // material in a "git" secret and reference it via the Credential field, | ||
| // never inline in the URL, so we don't risk persisting or displaying | ||
| // tokens that live in git_repos.url. | ||
| func validateURL(raw string) error { | ||
| u, err := url.Parse(raw) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid url %q: %w", raw, err) | ||
| } | ||
| if u.User != nil { | ||
| return fmt.Errorf("repo url must not embed credentials — use the --credential flag instead") | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the two SSH URL forms behave differently under net/url.
cat <<'EOF' > /tmp/ssh_url_probe.go
package main
import (
"fmt"
"net/url"
)
func main() {
for _, raw := range []string{
"ssh://git@github.com/acme/repo.git",
"git@github.com:acme/wf.git",
"https://user:pass@github.com/acme/repo.git",
"https://token@github.com/acme/repo.git",
} {
u, err := url.Parse(raw)
fmt.Printf("%-55s err=%v scheme=%q host=%q user=%v path=%q\n",
raw, err, u.Scheme, u.Host, u.User, u.Path)
}
}
EOF
go run /tmp/ssh_url_probe.go 2>/dev/null || echo "go toolchain not available in sandbox; maintainer should run locally"
# Also check if the repo's tests already cover ssh:// form:
rg -nP 'ssh://' -g '*.go'Repository: dvflw/mantle
Length of output: 252
🏁 Script executed:
# Read the exact validateURL implementation from store.go
cat -n packages/engine/internal/repo/store.go | sed -n '278,293p'Repository: dvflw/mantle
Length of output: 827
🏁 Script executed:
# Search for tests of validateURL function
rg -n 'validateURL|TestValidateURL' packages/engine/internal/repo/ -g '*.go'Repository: dvflw/mantle
Length of output: 337
🏁 Script executed:
# Check config.go around lines 372-376
cat -n packages/engine/internal/repo/config.go | sed -n '370,380p'Repository: dvflw/mantle
Length of output: 127
🏁 Script executed:
# Re-run the URL parsing test to see scp-like behavior
cat > /tmp/complete_url_test.go << 'EOF'
package main
import (
"fmt"
"net/url"
)
func main() {
urls := []string{
"ssh://git@github.com/acme/repo.git",
"git@github.com:acme/wf.git",
"https://user:pass@github.com/acme/repo.git",
"https://token@github.com/acme/repo.git",
}
for _, raw := range urls {
u, err := url.Parse(raw)
user := ""
if u.User != nil {
user = u.User.String()
}
fmt.Printf("URL: %-55s | Scheme: %-6s | User: %-20s | Host: %-20s\n",
raw, u.Scheme, user, u.Host)
}
}
EOF
python3 << 'PYEOF'
import subprocess
result = subprocess.run(['go', 'run', '/tmp/complete_url_test.go'], capture_output=True, text=True)
if result.returncode == 0:
print(result.stdout)
else:
print("Go not available in sandbox. Checking URL parsing reference...")
# Parse URLs using Python to show the behavior
from urllib.parse import urlparse
for raw in ["ssh://git@github.com/acme/repo.git", "git@github.com:acme/wf.git"]:
parsed = urlparse(raw)
print(f"Python parse {raw}: scheme={parsed.scheme} netloc={parsed.netloc} user={parsed.username}")
PYEOFRepository: dvflw/mantle
Length of output: 276
🏁 Script executed:
# Search for any existing tests related to repo URL validation
fd -e '_test.go' packages/engine/internal/repo/ -x grep -l 'validateURL\|URL.*test\|ssh://' {}Repository: dvflw/mantle
Length of output: 481
🏁 Script executed:
# Search for config.go anywhere in the repo
find . -name 'config.go' -type f 2>/dev/null | head -20Repository: dvflw/mantle
Length of output: 99
🏁 Script executed:
# Check repos.go line 58 for the example
fd -type f -name 'repos.go' | xargs cat -n | sed -n '50,70p'Repository: dvflw/mantle
Length of output: 226
🏁 Script executed:
# Search for tests covering URL validation or git repo URLs
fd -type f -name '*_test.go' packages/engine/ | xargs grep -l 'URL\|url' 2>/dev/null | head -5Repository: dvflw/mantle
Length of output: 226
🏁 Script executed:
# Check if there's a test file for store.go
fd -type f -name 'store_test.go' packages/engine/Repository: dvflw/mantle
Length of output: 226
🏁 Script executed:
# Check config.go at correct location around line 372
cat -n packages/engine/internal/config/config.go | sed -n '370,380p'Repository: dvflw/mantle
Length of output: 663
🏁 Script executed:
# Search for repos.go and check the example
find . -name 'repos.go' -type f 2>/dev/null -exec cat -n {} \; | sed -n '50,70p'Repository: dvflw/mantle
Length of output: 988
🏁 Script executed:
# Search for URL validation tests more carefully
find . -name '*_test.go' -path '*/engine/internal/repo/*' 2>/dev/nullRepository: dvflw/mantle
Length of output: 101
🏁 Script executed:
# Look for any test of validateURL or git repo URLs
rg 'validateURL|ssh://.*github' packages/engine/ --type goRepository: dvflw/mantle
Length of output: 440
🏁 Script executed:
# Check store_test.go for validateURL tests
cat -n packages/engine/internal/repo/store_test.go | grep -A 10 -B 2 'validateURL\|TestValidate\|ssh://'Repository: dvflw/mantle
Length of output: 38
🏁 Script executed:
# Get a broader view of store_test.go to see what's tested
wc -l packages/engine/internal/repo/store_test.go
head -100 packages/engine/internal/repo/store_test.go | cat -nRepository: dvflw/mantle
Length of output: 3559
🏁 Script executed:
# Search for any test covering SSH URLs in the entire engine package
rg 'ssh://|git@github' packages/engine/ --type go -B 2 -A 2Repository: dvflw/mantle
Length of output: 590
validateURL rejects legitimate ssh://git@host/... URLs.
Blanket-rejecting any URL with u.User != nil blocks the standard SSH URL form ssh://git@github.com/acme/repo.git, where git is the SSH login name (not a credential). url.Parse sets u.User = url.User("git") for that input, so it fails this validator.
The scp-like shorthand git@github.com:acme/wf.git (shown in the add command's example) happens to survive because url.Parse treats the whole string as an opaque path without populating User — creating inconsistent behavior across two forms of the same SSH URL.
This same overly-broad validation exists in packages/engine/internal/config/config.go lines 374–375, so both must be fixed together.
Consider allowing well-known SSH login names (git, hg) or, more robustly, only rejecting userinfo for HTTP(S) schemes where credential-in-URL is the actual attack surface:
🛡️ Suggested fix
func validateURL(raw string) error {
u, err := url.Parse(raw)
if err != nil {
return fmt.Errorf("invalid url %q: %w", raw, err)
}
- if u.User != nil {
- return fmt.Errorf("repo url must not embed credentials — use the --credential flag instead")
- }
+ // Credentials-in-URL is an HTTP(S) attack vector; SSH URLs legitimately
+ // carry a login name (e.g., ssh://git@github.com/...).
+ if u.User != nil {
+ scheme := strings.ToLower(u.Scheme)
+ if scheme == "http" || scheme == "https" {
+ return fmt.Errorf("repo url must not embed credentials — use the --credential flag instead")
+ }
+ if _, hasPwd := u.User.Password(); hasPwd {
+ return fmt.Errorf("repo url must not embed credentials — use the --credential flag instead")
+ }
+ }
return nil
}Apply the same logic to packages/engine/internal/config/config.go lines 372–376 and add test cases for both ssh://git@host/path.git and ssh://host/path.git forms.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/internal/repo/store.go` around lines 278 - 293, The
validateURL function currently rejects any URL with u.User != nil which
incorrectly blocks legitimate SSH URLs like ssh://git@host/..., so change the
logic in validateURL to only treat embedded userinfo as an error for HTTP(S)
schemes (check u.Scheme == "http" || u.Scheme == "https") while allowing SSH
schemes (e.g., "ssh") to have a username; apply the identical change to the
corresponding validator in config.go (the validateURL/URL validation function
there), and add unit tests covering ssh://git@host/path.git and
ssh://host/path.git to ensure both forms are accepted while HTTP(S) URLs with
user:pass remain rejected.
The config → repo → auth chain caused `go test` to fail in both ./internal/auth and ./internal/repo with "import cycle not allowed in test". Fix by inlining validateGitSyncName, validateGitSyncPollInterval, and validateGitSyncURL as unexported helpers in config.go, removing the import edge from config → repo. The canonical exported symbols in internal/repo/types.go are unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai this is ready for re-review |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/engine/internal/config/config_test.go (1)
471-514: Add negative-path tests for the newgit_sync.reposvalidators.The added test exercises only the happy path. The new
validateGitSyncName,validateGitSyncURL, andvalidateGitSyncPollIntervalhelpers — which are explicitly flagged in the source as "keep in sync with canonical version" — have no negative coverage here. Given the duplication, a regression (e.g., the 10s minimum drifting or the userinfo check being silently removed) would go undetected. Consider table-driven negative cases:🧪 Suggested additional test
func TestLoadConfig_GitSyncRepos_ValidationErrors(t *testing.T) { cases := []struct { name string repo string wantErr string }{ {"missing url", `- name: acme credential: c`, "url is required"}, {"embedded creds", `- name: acme url: https://user:pw@github.com/a/b.git credential: c`, "must not embed credentials"}, {"poll too small", `- name: acme url: https://github.com/a/b.git credential: c poll_interval: 1s`, "below 10s minimum"}, {"bad name", `- name: BadName! url: https://github.com/a/b.git credential: c`, "invalid repo name"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "mantle.yaml") yaml := "version: 1\ngit_sync:\n repos:\n " + tc.repo + "\n" require.NoError(t, os.WriteFile(path, []byte(yaml), 0600)) cmd := newTestCommand() _ = cmd.Flags().Set("config", path) _, err := Load(cmd) require.Error(t, err) assert.Contains(t, err.Error(), tc.wantErr) }) } }Also consider renaming to
TestLoad_GitSyncReposto match the existingTestLoad_*convention in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/config/config_test.go` around lines 471 - 514, Add table-driven negative tests for git_sync.repos validation by creating a new test (or renaming to TestLoad_GitSyncRepos to match convention) that constructs malformed repo YAML cases and asserts Load(cmd) returns an error containing the expected message; cover cases exercising validateGitSyncName, validateGitSyncURL (embedded credentials and missing url), and validateGitSyncPollInterval (below 10s) so regressions in those helpers are caught, and reuse newTestCommand and setting the "config" flag as in the existing TestLoadConfig_GitSyncRepos.packages/engine/internal/audit/audit.go (1)
65-68: Optional: consider aligning verbs with other audit actions.Other domain lifecycle actions in this file use
created/updated/deleted(e.g.,environment.created/updated/deleted,user.created/deleted,credential.created/deleted), whereas the new repo actions useadded/removed. The mix is harmless but queries filtering by verb (action LIKE '%.created') will not pick uprepo.added. If the intent is to mirror the CLI verbs (repos add/remove), leave as-is; otherwise considerrepo.created/repo.deletedfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/audit/audit.go` around lines 65 - 68, Change the repo audit action verbs to match existing domain lifecycle verbs: replace ActionRepoAdded and ActionRepoRemoved with names using "created" and "deleted" (e.g., repo.created/repo.deleted) so they align with other actions like environment.created/updated/deleted; update the ActionRepoAdded and ActionRepoRemoved constant values (and any references) to the new strings while keeping the same constant identifiers to minimize callsite changes.packages/engine/internal/db/migrations/019_git_repos.sql (1)
19-19: Optional: no referential integrity betweencredentialand the credentials table.
credential TEXT NOT NULLstores a credential name by convention but there's no FK. Deleting a credential from under agit_reposrow will leave a dangling reference that Plan B's sync engine will only discover at resolution time. If the credentials table has a stablenamecolumn with a unique constraint per team, consider either:
- Adding
FOREIGN KEY (team_id, credential) REFERENCES credentials(team_id, name) ON DELETE RESTRICT, or- Documenting the intentional decoupling (e.g., supporting forward references from
mantle.yamlwhere a credential may not yet exist).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/db/migrations/019_git_repos.sql` at line 19, The git_repos table defines credential TEXT NOT NULL but has no referential integrity to the credentials table, which can leave dangling names when a credential is deleted; either add a foreign key constraint like FOREIGN KEY (team_id, credential) REFERENCES credentials(team_id, name) ON DELETE RESTRICT to the git_repos definition (ensuring team_id and credentials.name are uniquely constrained) or explicitly document that git_repos.credential is intentionally decoupled (e.g., to allow forward references from mantle.yaml) and update tests and migration comments to reflect the chosen approach.packages/engine/internal/config/config.go (2)
416-463: Duplicated validators are a drift hazard; consider lifting them out of both packages.The comments acknowledge these are copies of
repo.ValidateName/repo.ValidatePollInterval/repo.validateURLkept in sync manually. A future tightening (e.g., bumping the 10s minimum, or narrowing the name regex) will now need two edits in two files with no compile-time guarantee they stay aligned.If the cycle is
config → repo → authat test time, a light refactor is to extract these pure validators into a leaf package (e.g.,internal/repo/validateorinternal/gitvalidate) that neitherinternal/authnorinternal/repo's heavy deps pull in. Bothinternal/configandinternal/repocan then depend on the leaf. Not a blocker for this PR — flagging so it doesn't rot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/config/config.go` around lines 416 - 463, The duplicated validator logic (gitSyncNamePattern, validateGitSyncName, validateGitSyncPollInterval, validateGitSyncURL) should be moved into a small shared leaf package (e.g., internal/repo/validate or internal/gitvalidate) and both packages should call the single canonical functions (e.g., ValidateName, ValidatePollInterval, ValidateURL) instead of keeping local copies; create the new package with the same validation behavior and signatures, update packages/engine/internal/config to import and use those functions, and switch packages/engine/internal/repo to call the shared validators (replacing repo.ValidateName/ValidatePollInterval/validateURL or delegating to them) so future changes are made in one place and the import cycle is avoided by keeping the new package dependency-free.
455-463: Minor:validateGitSyncURLaccepts strings that aren't really URLs.
url.Parseis very permissive — it will happily returnnilerror for inputs like"not a url at all"(scheme and host both empty) because they're syntactically valid references. So the current check catches the userinfo case but won't reject, e.g.,url: ~/typo/workflows.git. For defense-in-depth on the config surface, consider also requiring a non-empty scheme and host:♻️ Proposed tightening
u, err := url.Parse(raw) if err != nil { return fmt.Errorf("invalid url %q: %w", raw, err) } + if u.Scheme == "" || (u.Host == "" && u.Scheme != "ssh") { + // ssh-style git URLs (git@host:path) don't parse as net/url with a host; + // treat this as a hint that more-sophisticated validation belongs in the + // repo package — this is just the config-level fail-fast pass. + } if u.User != nil { return fmt.Errorf("repo url must not embed credentials — use the Credential field instead") }Note that
scp-style git SSH URLs (git@github.com:acme/repo.git) don't parse as net/url with a meaningfulHost, so any stricter check needs to accommodate that form. Leaving as-is is also reasonable given the store runs the authoritative check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/config/config.go` around lines 455 - 463, The validateGitSyncURL function accepts inputs that url.Parse treats as valid references but lack scheme/host (e.g., "not a url"), so tighten validation: after parsing in validateGitSyncURL, require either a non-empty u.Scheme and u.Host OR that the raw string matches an scp-style SSH git pattern (e.g., user@host:repo) — if neither condition holds return an error; keep the existing u.User check to reject embedded credentials. This ensures normal URL form is enforced while still allowing git scp-style URLs.packages/engine/internal/cli/repos_test.go (1)
19-58: Nit:reposCtxreturns a context no caller consumes.Every caller uses
_, cfg := reposCtx(t)and the tests drive config into the command via the--database-urlflag (not via the returned ctx). The doc comment also claims the ctx is "sonewRepoStorecan load it", butnewRepoStorereads fromcmd.Context(), so this is misleading. Consider dropping the unused return (and theconfig.WithContextline) or actually using the ctx in the test flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/repos_test.go` around lines 19 - 58, The helper reposCtx returns a context that callers never use (they only use the returned *config.Config via cfg and pass the DB URL via flags), so remove the unused context to avoid misleading documentation and dead code: update reposCtx to not construct or return config.WithContext/bg (remove the config.WithContext call and the context return value) and adjust all callers to call reposCtx(t) for only the *config.Config (or alternatively, if you prefer keeping the ctx, change the tests that call reposCtx to use the returned ctx when constructing the command so newRepoStore can read from cmd.Context()); reference reposCtx, newRepoStore, and config.WithContext to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/engine/internal/config/config.go`:
- Around line 360-380: The GitSync repo validation loop (iterating
cfg.GitSync.Repos) currently checks Name, URL and PollInterval but does not
enforce Credential, which will violate the DB NOT NULL column; update the loop
to validate that each repo's Credential (r.Credential) is non-empty and return a
clear error like "git_sync.repos[%d] (%q): credential is required" when
missing—add this check alongside the existing
validateGitSyncName/validateGitSyncURL/validateGitSyncPollInterval calls so
failures are caught in Load() rather than at persistence time.
In `@packages/engine/internal/db/migrations/019_git_repos.sql`:
- Line 26: The webhook_secret column in the git_repos migration is being created
as plaintext TEXT; change the migration so secrets are not stored in cleartext
by either (a) removing webhook_secret and instead adding a reference (e.g.,
reuse the existing credential TEXT pattern on git_repos to point at an entry in
the credentials table) or (b) persist an encrypted blob using the engine
master-key mechanism (same approach used in internal/secret/store.go for
credential data) so values are encrypted at rest; update any code that
writes/reads git_repos.webhook_secret to use the credential reference or the
encrypt/decrypt helpers in internal/secret/store.go accordingly.
---
Nitpick comments:
In `@packages/engine/internal/audit/audit.go`:
- Around line 65-68: Change the repo audit action verbs to match existing domain
lifecycle verbs: replace ActionRepoAdded and ActionRepoRemoved with names using
"created" and "deleted" (e.g., repo.created/repo.deleted) so they align with
other actions like environment.created/updated/deleted; update the
ActionRepoAdded and ActionRepoRemoved constant values (and any references) to
the new strings while keeping the same constant identifiers to minimize callsite
changes.
In `@packages/engine/internal/cli/repos_test.go`:
- Around line 19-58: The helper reposCtx returns a context that callers never
use (they only use the returned *config.Config via cfg and pass the DB URL via
flags), so remove the unused context to avoid misleading documentation and dead
code: update reposCtx to not construct or return config.WithContext/bg (remove
the config.WithContext call and the context return value) and adjust all callers
to call reposCtx(t) for only the *config.Config (or alternatively, if you prefer
keeping the ctx, change the tests that call reposCtx to use the returned ctx
when constructing the command so newRepoStore can read from cmd.Context());
reference reposCtx, newRepoStore, and config.WithContext to locate the changes.
In `@packages/engine/internal/config/config_test.go`:
- Around line 471-514: Add table-driven negative tests for git_sync.repos
validation by creating a new test (or renaming to TestLoad_GitSyncRepos to match
convention) that constructs malformed repo YAML cases and asserts Load(cmd)
returns an error containing the expected message; cover cases exercising
validateGitSyncName, validateGitSyncURL (embedded credentials and missing url),
and validateGitSyncPollInterval (below 10s) so regressions in those helpers are
caught, and reuse newTestCommand and setting the "config" flag as in the
existing TestLoadConfig_GitSyncRepos.
In `@packages/engine/internal/config/config.go`:
- Around line 416-463: The duplicated validator logic (gitSyncNamePattern,
validateGitSyncName, validateGitSyncPollInterval, validateGitSyncURL) should be
moved into a small shared leaf package (e.g., internal/repo/validate or
internal/gitvalidate) and both packages should call the single canonical
functions (e.g., ValidateName, ValidatePollInterval, ValidateURL) instead of
keeping local copies; create the new package with the same validation behavior
and signatures, update packages/engine/internal/config to import and use those
functions, and switch packages/engine/internal/repo to call the shared
validators (replacing repo.ValidateName/ValidatePollInterval/validateURL or
delegating to them) so future changes are made in one place and the import cycle
is avoided by keeping the new package dependency-free.
- Around line 455-463: The validateGitSyncURL function accepts inputs that
url.Parse treats as valid references but lack scheme/host (e.g., "not a url"),
so tighten validation: after parsing in validateGitSyncURL, require either a
non-empty u.Scheme and u.Host OR that the raw string matches an scp-style SSH
git pattern (e.g., user@host:repo) — if neither condition holds return an error;
keep the existing u.User check to reject embedded credentials. This ensures
normal URL form is enforced while still allowing git scp-style URLs.
In `@packages/engine/internal/db/migrations/019_git_repos.sql`:
- Line 19: The git_repos table defines credential TEXT NOT NULL but has no
referential integrity to the credentials table, which can leave dangling names
when a credential is deleted; either add a foreign key constraint like FOREIGN
KEY (team_id, credential) REFERENCES credentials(team_id, name) ON DELETE
RESTRICT to the git_repos definition (ensuring team_id and credentials.name are
uniquely constrained) or explicitly document that git_repos.credential is
intentionally decoupled (e.g., to allow forward references from mantle.yaml) and
update tests and migration comments to reflect the chosen approach.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b602348-312d-4fc4-9f7c-01fcbd419596
📒 Files selected for processing (13)
CLAUDE.mdpackages/engine/internal/audit/audit.gopackages/engine/internal/cli/repos.gopackages/engine/internal/cli/repos_test.gopackages/engine/internal/cli/root.gopackages/engine/internal/config/config.gopackages/engine/internal/config/config_test.gopackages/engine/internal/db/migrations/019_git_repos.sqlpackages/engine/internal/repo/store.gopackages/engine/internal/repo/store_test.gopackages/engine/internal/repo/types.gopackages/engine/internal/secret/types.gopackages/engine/internal/secret/types_test.go
Inline (required): - config.go: validate that each git_sync.repos entry has a non-empty credential; the DB has credential NOT NULL, so a missing value would fail at persistence time with a cryptic constraint error - 019_git_repos.sql: drop webhook_secret column — storing an HMAC secret as plaintext TEXT is a security risk; Plan C will add a proper migration that stores it as an encrypted credential reference - store.go, types.go: remove webhook_secret from RETURNING/SELECT queries, scans, and the Repo struct to match the updated schema Nitpicks: - audit.go: align repo action verbs with the rest of the domain: repo.added → repo.created, repo.removed → repo.deleted (constant identifiers unchanged to minimise callsite churn) - repos_test.go: remove unused context return from reposCtx; callers already discarded it with _ and pass the DB URL via --database-url flags - config_test.go: add TestLoad_GitSyncRepos_Validation with table-driven negative cases covering invalid name, missing url, embedded credentials, url with no scheme/host, missing credential, and poll_interval below 10s - config.go: tighten validateGitSyncURL to reject strings that url.Parse accepts as relative references but lack both scheme and host, unless they match the scp-style SSH format (git@host:path) - 019_git_repos.sql: add comment explaining why credential is a plain name reference rather than a FK — prevents registration before the credential exists, which is a common IaC bootstrapping pattern https://claude.ai/code/session_01MqwQDFpdQTksPDHrMi4cKB
Collaborator
Author
|
@coderabbitai those issues have been addressed |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[approve] 🧠 Learnings used |
5 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
2 participants
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.
Summary
Plan A of 3 for issue #16 (GitOps sync, milestone v0.5.0). Ships the foundation — no sync behavior yet; that's Plan B.
gitcredential type with token/ssh_key/username fields and at-least-one-of validationgit_repostable (migration 019) andinternal/repoCRUD store with transactional audit emissiongit_sync.reposblock tomantle.yaml(parsed-only; startup reconciliation lands with Plan B)mantle repos add | list | status | removeCLI commandsrepo.added,repo.updated,repo.removedaudit eventsPlan breakdown
repos sync.POST /hooks/git/<id>), REST API (POST /api/v1/repos), metrics.What operators can do today
mantle repos add acme --url https://github.com/acme/wf.git --credential github-patmantle repos list,mantle repos status acmemantle repos remove acme --yesmantle.yamlundergit_sync.repos:(parsed, not yet materialized — Plan B)Last-sync fields (
last_sync_sha,last_sync_at,last_sync_error,webhook_secret) are in the schema but stay null until Plan B/C populate them.Pre-push review fixes
Four reviewer agents ran before pushing (Technical Writer, Product Manager, Legal Compliance, Reality Checker). Consolidated fixes landed in 431f822:
--pruneflag help to describe the positive-default behaviorWebhookSecretfield onRepostruct with a SECURITY commenthttps://user:pass@host/...)Test plan
go test ./...frompackages/engine— all packages greengo test ./internal/repo/ -v— 13 tests pass (CRUD + validation + URL sanitization)go test ./internal/cli/ -v— 6TestRepos*CLI tests passgo test ./internal/config/ -v—TestLoadConfig_GitSyncRepospassesgo build ./cmd/mantle && ./mantle repos --helpsurfaces all four subcommandsmantle repos addpersists a row;mantle repos listshows it;mantle repos statusprints detail;mantle repos remove --yesdeletes and emitsrepo.removedaudit eventCloses part of #16. Plans B and C follow.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Config
Security / Credentials
Database
Tests
Documentation