feat(manifest-analyzer): repo-walker onboarding discovery (F8 first cut)#215
Merged
Conversation
Add WalkRepo(ctx, root) in internal/manifestanalyzer plus a thin CLI mode
`--mode repo-walker <root>` (--format text|json). It walks a whole repo,
enumerates candidate GitTargets (render roots + plain KRM leaf folders),
classifies each folder's layout (plain | kustomize-single | kustomize-overlay
| refused-structural), runs the operator's own acceptance gate per candidate,
mirrors the GitTarget overlap check, and emits a text/JSON report.
Reports, does not propose: no CR/WatchRule generation, no `--mode discovery`
rename, and the repo-level `--policy refuse` gate is deferred (exit codes stay
simple). Per candidate: path, layout, acceptedByOperator, refusalReasons,
renderRoot, readScope, inferredNamespace, resources{rendered,editable,nonKrm},
overlapsWith; plus a repo summary (counts by layout, accepted/refused,
overlapConflicts, fleetRoot, unsupportedConstructs). The rendered-vs-editable
split makes the F2 overlay gap legible: an overlay renders base documents it
cannot yet edit (rendered=2, editable=0). The load-bearing
overlay-fan-out-needs-f2 (forward-looking) vs refused-structural (permanent
wall) distinction is preserved.
Golden corpus under testdata/repo-walker/{supported,unsupported}/<fixture>/
with sibling <fixture>.golden.json (UPDATE_GOLDEN=1 regenerates), including an
overlay-nested-base regression pinning the deduped rendered count and minimal
readScope (a nested out-of-subtree base must not double-count).
store.go: extract unsupportedKustomizeFeatureKeys() shared by the acceptance
gate and the walker's per-feature refusal detail (behavior-preserving).
docs(gitops-api): sync the F8 design doc to what shipped, add the
"discovery != support" containment thesis and the ladder-vs-wall framing,
reconcile refused-fleet-root/refused-out-of-band as repo-level vs candidate
layouts, and fold the open decisions into the open questions; add a
cross-reference from kustomize-support-boundary-and-product-model.md.
Validation: task lint and task test green (coverage within ratchet);
task test-e2e passed 55/55 (unaffected by the CLI-library/docs-only changes
made after that run).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
📝 WalkthroughWalkthroughAdds a new "repo-walker" whole-repository discovery mode: a ChangesF8 repo-walker feature
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/manifestanalyzer/repowalk.go (1)
345-372: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: memoize fully-visited dirs to avoid re-walking shared bases.
The
onPathset only guards cycles; a directory that has finished processing is removed fromonPath, so a diamond-shaped resources graph (multiple roots reaching a shared base that itself reaches further bases) re-walks the shared subtree once per inbound edge. For deeply-shared graphs this is exponential. The result set stays correct, so this is purely a defensive perf hardening for pathological/large repos.♻️ Add a done-set to short-circuit revisits
func reachedKustomizationDirs(rootDir string, kusts map[string]*kustomizationDoc) map[string]struct{} { reached := map[string]struct{}{} onPath := map[string]struct{}{} + done := map[string]struct{}{} var walk func(dir string) walk = func(dir string) { cur := kusts[dir] if cur == nil { return } if _, cycling := onPath[dir]; cycling { return } + if _, visited := done[dir]; visited { + return + } onPath[dir] = struct{}{} for _, entry := range cur.resources { target := cleanJoin(dir, entry) if target == "" { continue } if _, isKust := kusts[target]; isKust { reached[target] = struct{}{} walk(target) } } delete(onPath, dir) + done[dir] = struct{}{} } walk(rootDir) return reached }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/manifestanalyzer/repowalk.go` around lines 345 - 372, The recursive traversal in reachedKustomizationDirs only tracks the current path with onPath, so shared subtrees can be re-walked repeatedly in diamond-shaped graphs. Add a memoized done/visited set inside reachedKustomizationDirs to short-circuit already fully processed dirs, and check it before recursing from walk so shared bases are traversed once while preserving the existing cycle protection and reached results.
🤖 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
`@internal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize/app/kustomization.yaml`:
- Around line 6-9: The test fixture in the kustomize manifest contains a
hardcoded secret-looking literal that GitGuardian flags; update the
`secretGenerator` entry in the `kustomization.yaml` fixture to use a non-secret
placeholder while keeping the unsupported-feature path exercised. Prefer
changing the `literals` value under `secretGenerator` in this manifest to
something clearly fake like a placeholder token, or alternatively add a targeted
ignore for the `testdata/` fixtures if you want to keep the exact value.
---
Nitpick comments:
In `@internal/manifestanalyzer/repowalk.go`:
- Around line 345-372: The recursive traversal in reachedKustomizationDirs only
tracks the current path with onPath, so shared subtrees can be re-walked
repeatedly in diamond-shaped graphs. Add a memoized done/visited set inside
reachedKustomizationDirs to short-circuit already fully processed dirs, and
check it before recursing from walk so shared bases are traversed once while
preserving the existing cycle protection and reached results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4679fc32-edc1-4557-82e7-048caf1985aa
📒 Files selected for processing (46)
cmd/manifest-analyzer/main.gocmd/manifest-analyzer/main_test.godocs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.mddocs/design/gitops-api/kustomize-support-boundary-and-product-model.mdinternal/manifestanalyzer/repowalk.gointernal/manifestanalyzer/repowalk_test.gointernal/manifestanalyzer/store.gointernal/manifestanalyzer/testdata/repo-walker/supported/fleet-root.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/supported/fleet-root/apps/podinfo/deployment.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/fleet-root/clusters/prod/flux-system.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/fleet-root/infra/ingress-nginx/deployment.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/helm-release-document.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/supported/helm-release-document/app/helmrelease.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single/app/deployment.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single/app/kustomization.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/kustomize-single/app/service.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/no-krm.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/supported/no-krm/docs/README.mdinternal/manifestanalyzer/testdata/repo-walker/supported/no-krm/docs/notes.txtinternal/manifestanalyzer/testdata/repo-walker/supported/overlapping.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/supported/overlapping/apps/web/deployment.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/overlapping/apps/web/extra/configmap.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/prod/deployment.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/prod/service.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/test/deployment.yamlinternal/manifestanalyzer/testdata/repo-walker/supported/plain-per-env/apps/app/test/service.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/base/deployment.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/base/kustomization.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/base/service.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/overlays/acc/kustomization.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/overlays/prod/kustomization.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/base-overlays/overlays/test/kustomization.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/helm-inflation.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/unsupported/helm-inflation/app/kustomization.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/common/configmap.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/common/kustomization.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/deployment.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/base/kustomization.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/overlay-nested-base/overlays/test/kustomization.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize.golden.jsoninternal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize/app/deployment.yamlinternal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize/app/kustomization.yaml
Comment on lines
+142
to
+148
| // TestWalkRepo_NotADirectory covers the usage error path: a missing or non-directory | ||
| // root is an error, not an empty report. | ||
| func TestWalkRepo_NotADirectory(t *testing.T) { | ||
| if _, err := WalkRepo(context.Background(), filepath.Join("testdata", "does-not-exist")); err == nil { | ||
| t.Fatal("expected an error for a missing root") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test only covers the missing-path branch, not the file-as-root branch.
The test name TestWalkRepo_NotADirectory implies coverage of the !info.IsDir() error path in WalkRepo, but it only tests a non-existent path (the os.Stat error branch). The actual "not a directory" code path — passing a file as root — is not exercised.
🧪 Proposed fix: add a file-as-root subtest
func TestWalkRepo_NotADirectory(t *testing.T) {
- if _, err := WalkRepo(context.Background(), filepath.Join("testdata", "does-not-exist")); err == nil {
- t.Fatal("expected an error for a missing root")
+ t.Run("missing", func(t *testing.T) {
+ if _, err := WalkRepo(context.Background(), filepath.Join("testdata", "does-not-exist")); err == nil {
+ t.Fatal("expected an error for a missing root")
+ }
+ })
+ t.Run("file", func(t *testing.T) {
+ // Use this test file itself as a non-directory root.
+ if _, err := WalkRepo(context.Background(), "repowalk_test.go"); err == nil {
+ t.Fatal("expected an error for a file root")
+ }
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TestWalkRepo_NotADirectory covers the usage error path: a missing or non-directory | |
| // root is an error, not an empty report. | |
| func TestWalkRepo_NotADirectory(t *testing.T) { | |
| if _, err := WalkRepo(context.Background(), filepath.Join("testdata", "does-not-exist")); err == nil { | |
| t.Fatal("expected an error for a missing root") | |
| } | |
| } | |
| // TestWalkRepo_NotADirectory covers the usage error path: a missing or non-directory | |
| // root is an error, not an empty report. | |
| func TestWalkRepo_NotADirectory(t *testing.T) { | |
| t.Run("missing", func(t *testing.T) { | |
| if _, err := WalkRepo(context.Background(), filepath.Join("testdata", "does-not-exist")); err == nil { | |
| t.Fatal("expected an error for a missing root") | |
| } | |
| }) | |
| t.Run("file", func(t *testing.T) { | |
| // Use this test file itself as a non-directory root. | |
| if _, err := WalkRepo(context.Background(), "repowalk_test.go"); err == nil { | |
| t.Fatal("expected an error for a file root") | |
| } | |
| }) | |
| } |
Comment on lines
+6
to
+9
| secretGenerator: | ||
| - name: creds | ||
| literals: | ||
| - password=s3cret |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
GitGuardian flagged a hardcoded secret in this test fixture.
The secretGenerator literal password=s3cret is intentionally fake test data used to trigger the unsupported-feature detection, but it has triggered a real GitGuardian pipeline failure. This will block CI until remediated.
Options to resolve:
- Replace the value with a non-secret-looking placeholder (e.g.,
password=REDACTEDorkey=placeholder) that still exercises thesecretGeneratordetection path. - Add a GitGuardian ignore rule/match exclusion for the
testdata/directory.
Option 1 is simpler and keeps the fixture self-contained without scanner configuration changes.
🔐 Proposed fix: replace the flagged literal
secretGenerator:
- name: creds
literals:
- - password=s3cret
+ - placeholder=not-a-real-secret📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| secretGenerator: | |
| - name: creds | |
| literals: | |
| - password=s3cret | |
| secretGenerator: | |
| - name: creds | |
| literals: | |
| - placeholder=not-a-real-secret |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@internal/manifestanalyzer/testdata/repo-walker/unsupported/unsupported-kustomize/app/kustomization.yaml`
around lines 6 - 9, The test fixture in the kustomize manifest contains a
hardcoded secret-looking literal that GitGuardian flags; update the
`secretGenerator` entry in the `kustomization.yaml` fixture to use a non-secret
placeholder while keeping the unsupported-feature path exercised. Prefer
changing the `literals` value under `secretGenerator` in this manifest to
something clearly fake like a placeholder token, or alternatively add a targeted
ignore for the `testdata/` fixtures if you want to keep the exact value.
Source: Pipeline failures
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…easons, writer parity)
- rendered/editable now count only what the kustomization resources graph
renders (reachedResourceFilesFrom), never parked YAML a kustomization does
not reference; editable is the rendered subset physically in the candidate's
own subtree. Fixes rendered overstating "everything the candidate renders".
- refused plain / self-contained kustomize candidates now surface the
acceptance gate's issues as refusalReasons ({code, detail}) instead of a bare
acceptedByOperator:false, so the product sees WHY (duplicate, non-KRM,
foreign, mixed, unsupported nested kustomization, ...).
- repo-walker acceptance now uses WriterAllowlist (the live writer's allowlist,
incl. the operator .sops.yaml bootstrap config) for both the whole-repo store
and per-candidate acceptance, so a folder the writer would adopt is not
falsely reported refused.
- gate: add openapi and crds to unsupportedKustomizeFeatureKeys, aligning the
acceptance gate with the documented support boundary (they silently change
merge-key semantics; configurations was already refused).
Corpus: add overlay-parked-base (parked YAML excluded from rendered),
plain-nonkrm (gate refusal surfaced), and extend unsupported-kustomize with
openapi/crds; plus regression tests and an in-memory .sops.yaml acceptance test.
Docs: fence the report-contract example as jsonc (it carries an explanatory
comment); note the WriterAllowlist parity, gate-issue surfacing, and openapi/crds
refusal in F8's first-cut section.
Validation: task lint and task test green (coverage 75.2%); task test-e2e
passed 55/55 (the shared-gate openapi/crds change was exercised).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Review round 2 — addressed in
|
| Finding | Resolution |
|---|---|
M — rendered overstates the render |
rendered/editable now count only the kustomization resources graph (reachedResourceFilesFrom), never parked YAML a kustomization doesn't reference. New overlay-parked-base fixture (base holds an unreferenced parked.yaml) locks rendered: 1, not 2. |
| M — refused candidates lacked reasons | Refused plain / self-contained kustomize candidates now surface the gate's issues as refusalReasons ({code, detail}) — candidateAccepted → candidateAcceptance returning the full Acceptance. New plain-nonkrm fixture shows {code: "non-krm-yaml", detail: "values.yaml: …"}. |
| M-low — allowlist ≠ live writer | repo-walker now uses WriterAllowlist (incl. the operator .sops.yaml bootstrap config) for both the whole-repo store and per-candidate acceptance. In-memory .sops.yaml test asserts the folder is accepted (not refused non-KRM) and .sops.yaml isn't counted as nonKrm. |
Low — docs say openapi/crds refused, gate didn't |
Added openapi + crds to unsupportedKustomizeFeatureKeys (they silently change merge-key semantics). Extended unsupported-kustomize fixture — unsupportedConstructs now includes both. This is a shared-gate (operator) change, so e2e was re-run: 55/55 ✅. |
Low — json fence with // comment |
Fenced the report-contract example as jsonc (it carries one explanatory comment; the shipped report is strict JSON). |
Validation: task lint ✅ · task test ✅ (coverage 75.2%) · task test-e2e ✅ 55/55.
Pushed back on none this round — all five were fair. The only judgment call: openapi/crds is an operator-gate behavior change (not just repo-walker), included here because the walker's refused-structural detail reuses the same key list and the boundary doc already decided it; e2e re-run confirms no regression.
sunib
changed the title
This was referenced Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First cut of F8 repo-discovery / onboarding scan:
WalkRepo(ctx, root)ininternal/manifestanalyzerplus a thinmanifest-analyzer --mode repo-walker <repo>CLI (
--format text|json).It walks a whole repo, enumerates candidate
GitTargetsubtrees, classifies eachfolder's layout (
plain|kustomize-single|kustomize-overlay|refused-structural), runs the operator's acceptance gate per candidate, and emits atext/JSON report with a
resources { rendered, editable, nonKrm }split that makes theF2 overlay gap legible.
Reports, does not propose — no CR/
WatchRulegeneration, no--mode discoveryrename,
--policy refusedeferred (see the doc's non-goals).Design + rationale:
docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md.Validation:
task lint+task testgreen (coverage 75.2%);task test-e2e55/55.🤖 Generated with Claude Code