fix: replace manifest.txt help mechanism with embedded user docs directory

[robertolopezlopez]

User description

Follow-up to CLI-1474 / PR #6884 (CLI-1596): replace the generated, committed manifest.txt with go:embed of help/cli-commands, removing the drift-prone manifest artifact and the helpdocs-manifest / git add manifest.txt workflow.

Why this is necessary: GitBook sync updates help/cli-commands/ without triggering a build. A separate committed manifest could drift from the synced markdown and break help routing or CI until someone remembered to regenerate it.

What changed technically:

• cliv2/internal/helpdocs now uses //go:embed cli-commands and walks embedded .md basenames (same lookup logic as before), via a CommandHelp type instead of package-level state.
• helpdocs-manifest is removed; _helpdocs-prepare / _helpdocs-clean copy help/cli-commands/*.md into the embed tree at build time only, then remove copied .md files afterward. make -C cliv2 test and lint do not run the copy step.
• Committed cli-commands/do-not-delete keeps the embed directory present for tooling (go list, gopls) between builds; cliv2/.gitignore ignores transient copied markdown under internal/helpdocs/cli-commands/.
• Tests inject help lookup instead of relying on compile-time embed: helpdocs and helprouting use CommandHelpFromRepo() (reads help/cli-commands/ from disk) or a small in-memory fixture when that directory is unavailable. No TestMain sync and no skip guards for bare go test.

Help routing behavior is unchanged: documented commands still get legacy GitBook markdown help; undocumented ones still get Cobra help. Production wiring uses helpdocs.DefaultCommandHelp().HasUserDoc from main.go.

Pull Request Submission Checklist

• Follows CONTRIBUTING guidelines
• Commit messages are release-note ready, emphasizing what was changed, not how.
• Includes detailed description of changes
• Contains risk assessment (Low)
• Highlights breaking API changes (if applicable) — N/A
• Links to automated tests covering new functionality
• Includes manual testing instructions (if necessary)
• Updates relevant GitBook documentation (PR link: ___) — N/A (GitBook is source of truth)
• Includes product update to be announced in the next stable release notes — N/A (internal build/routing plumbing)

What does this PR do?

• Deletes cliv2/internal/helpdocs/manifest.txt and manifest sync test.
• Embeds help/cli-commands markdown via go:embed instead of a basename list.
• Replaces helpdocs-manifest Makefile target with transient _helpdocs-prepare / _helpdocs-clean around build only.
• Introduces injectable CommandHelp and wires HasUserDoc through helprouting.Router and main.go.
• Updates CONTRIBUTING.md for the new workflow.

Where should the reviewer start?

1. cliv2/internal/helpdocs/command_help.go + embed.go — lookup + compile-time embed (behavior should match pre-change)
2. cliv2/internal/helpdocs/fixture.go — disk/fixture fallback for tests
3. cliv2/Makefile — _helpdocs-prepare / _helpdocs-clean on build target only
4. cliv2/internal/helprouting/helprouting.go — injected HasUserDoc on Router

How should this be manually tested?

make -C cliv2 test
make build BUILD_MODE=public

./binary-releases/snyk-macos-arm64 help test        # documented → GitBook help
./binary-releases/snyk-macos-arm64 help agent-scan  # undocumented → Cobra help
./binary-releases/snyk-macos-arm64 secrets test --help | head -5    # Cobra Usage:

Verify post-build working tree: cliv2/internal/helpdocs/cli-commands/ contains only do-not-delete (no copied .md).

What's the product update that needs to be communicated to CLI users?

None. User-facing help content and routing behavior are unchanged.

Risk assessment

Low — same routing algorithm; only the embed data source, build sync, and test wiring changed.

Any background context you want to provide?

go:embed cannot reference repo-root help/cli-commands/ from cliv2/internal/helpdocs/ (no .. paths). The Makefile copies markdown into the package tree at build time; tests read the repo directory directly so they stay in sync with GitBook PRs without a prepare step.

What are the relevant tickets?

• CLI-1596
• CLI-1596

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[robertolopezlopez]

/describe

[snyk-pr-review-bot]

PR Description updated to latest commit (bd242f1)

[robertolopezlopez]

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Regex mismatch risk 🟡 [minor]
The nonDocChars regex [^a-zA-Z0-9-] strips all characters except alphanumeric and hyphens. If GitBook or the sync process produces filenames containing underscores or other symbols (which are common in some documentation systems), HasUserDoc will fail to find the file even if it exists in the embed. While the current tests use hyphens, this creates a tight coupling between the sync script's naming convention and the Go code that isn't explicitly enforced.

var nonDocChars = regexp.MustCompile(`[^a-zA-Z0-9-]`)

Silent WalkDir failure 🟡 [minor]
In docFilesFromEmbed, the error returned by the inner function of fs.WalkDir is returned to WalkDir, but the call to WalkDir itself on line 25 has its return value ignored (_ = ...). If the embedded filesystem is corrupted or inaccessible for an unexpected reason, the docFiles map will be silently incomplete, causing the CLI to fall back to the legacy help mechanism without any internal log or warning indicating a filesystem error.

_ = fs.WalkDir(fsys, root, func(_ string, d fs.DirEntry, err error) error {
	if err != nil || d.IsDir() || !strings.HasSuffix(d.Name(), ".md") {
		return err
	}
	files[d.Name()] = struct{}{}
	return nil
})

📚 Repository Context Analyzed

This review considered 14 relevant code sections from 11 files (average relevance: 0.80)

• help/README.md (lines 1-19)
• cliv2/pkg/core/main.go (2 segments, lines 209-412)
• cliv2/internal/embedded/cliv1/dummy_embedded_legacy_cli.go (lines 1-23)
• package.json (lines 1-229)
• scripts/upgrade-snyk-go-dependencies.go (lines 1-149)
• cliv2/scripts/prepare_licenses.go (lines 1-189)
• package-lock.json (lines 6-266)
  ... and 4 more file(s)

🤖 Repository instructions applied (from AGENTS.md)

Regex mismatch risk

this is just mimicking the previous .ts behavior

Silent WalkDir failure

this failure would just happen in build time, not runtime

[PeterSchafer]

Question: does this directory need to go into gitignore

[robertolopezlopez]

mmmmh good one, adding - but you get the reasoning behind this file, right?

[octavian-snyk]

Could we perhaps ignore the directory contents entirely in case an interrupted make build/test leaves a dirty tree such that they cannot be committed by accident?

[PeterSchafer]

This is what I meant, gitignore the whole directory and just have this one file.

but you get the reasoning behind this file, right?
I think this file is here so that a build would work even without using make build for it?!

[robertolopezlopez]

ok! for sure

[octavian-snyk]

Could we perhaps make the top level /help a go module itself, so we don't need this copy?

[robertolopezlopez]

Unfortunately not and this is why we bring up this Makefile wizardry. Root reason: it is off the cliv2/go.mod file tree. This is enforced by the directive itself: Patterns may not contain ‘.’ or ‘..’ or empty path elements

[robertolopezlopez]

and we cannot move help/ without extra trouble because there is a git hook which pushes directly to help/cli-commands/

[octavian-snyk]

what's the purpose of _helpdocs-prepare in lint?

[robertolopezlopez]

Leftover from previous approach - cleaning up

[octavian-snyk]

Could we test the logic against an in-memory fstest.MapFS to avoid the copy-delete process?

[robertolopezlopez]

I would prefer to mirror tHe Makefile#_Helpdocs-prepare. fstest.MapFS would only test against a made up FS tree, not the current state of help/cli-commands/*.md. I might add some extra test with fstest.MapFS - but probably this is redundant.

Please share your thoughts :-)

[robertolopezlopez]

I ended up applying this

[PeterSchafer]

Suggestion: all seems unnecessary and risky to include unwanted filesystem content

[PeterSchafer]

Suggestion: to be sure we start from an empty dir, let's just remove all file please.

[octavian-snyk]

Question: Now that we have _helpdocs-prepare and _helpdocs-clean. Could it be possible just to generate the manifest.txt with the filenames, embed that, and then remove it?

Will the contents of .md files end up being embedded twice in the binary as //go:embed cli-commands doesn't include strictly just the file names, and I believe they're also embedded by the legacy TS version?

[robertolopezlopez]

We are not generating the manifest.txt due to the user-docs hook issue; that's the whole purpose of this PR.

I do not follow your second comment :-)

[octavian-snyk]

On the manifest point, I think we're aligned that a committed manifest.txt is the problem: because of the weekly sync-cli-help-to-user-docs action, the manifest could drift.

What I was wondering is whether a transient manifest (generated in _helpdocs-prepare, embedded for compile, removed in _helpdocs-clean, never committed) would still have that problem. It feels like it would solve the same drift issue, just with a smaller Go-side embed than copying all the .md files. Does that match your thinking, or am I missing something about why even a build-time-only manifest is undesirable?

On the second part, Go's //go:embed cli-commands pulls in the full markdown files (with content), but HasUserDoc only uses their filenames for routing.

Mostly curious whether I'm reasoning about the transient-manifest alternative correctly, or if there's a constraint I'm not seeing. Happy to resolve the thread either way once I've understood your take. 🙂

[robertolopezlopez]

But what's the point of generating a manifest.txt in _helpdocs-prepare. I think that's adding extra complexity:

1. Iterate over all user-docs
2. Create manifest.txt
3. At the end, removing the manifest

Vs what we have here. embed.FS is basically an iterable construct which does not imply any complexity other than iterating it.

I do not think it brings any advantage. If you could name any real, tangible advantage we may be in the same line ;-)

[octavian-snyk]

With this approach we're embedding the docs content twice in the shipped binary: Go embeds the full .md files but only uses filenames for routing, and the legacy TS binary already embeds the same content for rendering. With a transient manifest we'd only have the filenames on the Go side. The difference isn't big though 😅 .

[robertolopezlopez]

You may be right on one side: yes, double embed. The TS bundle will be there for long time I believe.

Your proposal may work and shrink the Go side; but reopens the initial approach in CLI-1596 and adds more problems to the PR, which is already large.

Planned scope: preparing+cleaning changes, embed, getting a green pipeline (heh), multiple new review threads.

Do you really think it is worth to start changing the whole PR? :)

[octavian-snyk]

Fair point. Thank you 😀

Approval already given. I believe you can merge this PR.

[robertolopezlopez]

thank youuuu

[PeterSchafer]

just to add a closing thought here, we will be removing the TS implementation in the future, so the double embed is only temporary.

[PeterSchafer]

Question: can we test against synthetic data rather then relying on this pre-test step

[robertolopezlopez]

_helpdocs-prepare needs to be run before go test, so the test binary embeds the real .md (compile time vs run time). So we test with real world data :-)

Does this address your question, or what do you really mean with synthetic data?

[PeterSchafer]

I think I meant what @octavian-snyk meant with their comment.

[PeterSchafer]

Question: do we really/still need this?

[octavian-snyk]

nitpick: For readability it might be better to integrate these small files (embed.go, and export_test.go) into other relevant files; perhaps command_help.go, and fixture.go, respectively.

Other than this, LGTM :)

[robertolopezlopez]

ack :-) about embed.go: the Go convention is that //go:embed sits in a minimal file. I may merge it with command_help.go if you prefer but it is getting dangerously large IMHO and this is a sleepery road ;-)

[robertolopezlopez]

in snyk you can find similar examples https://github.com/snyk/policy-engine/blob/main/rego/embed.go

[robertolopezlopez]

about export_test.go: export_help.go is a kind of standard for test only package helpers. Example: os/export_test.go, fmt/export_test.go, etc.

[octavian-snyk]

TIL, thanks for the explanation!

I noticed CommandHelpFromRepo is already used directly in a couple of test files (helprouting_test.go:233 and externally at main_test.go:770), while CommandHelpForTest is only used internally at command_help_test.go:17. Do we need the extra re-export via CommandHelpForTest?

[PeterSchafer]

Suggestion: Maybe use https://pkg.go.dev/golang.org/x/tools/godoc/vfs/mapfs and remove this New function.

[robertolopezlopez]

it is deprecated! from your own link :-)

[PeterSchafer]

Question: Why not surface the error?

[robertolopezlopez]

ok!

[PeterSchafer]

Issue: please see the comment above!!! the fips import needs to be the first one

[PeterSchafer]

@robertolopezlopez besides the remaining comments, I think this looks now very good! Thank you for the refactoring!

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Broad Regex Pattern 🟡 [minor] The nonDocChars regex strips all characters except a-zA-Z0-9-. While this matches the current naming convention, if a future command contains other characters (like a period or underscore) that are allowed in filenames but stripped here, the lookup will fail to locate the corresponding .md file. Adding a comment or link to the corresponding TypeScript findHelpFile implementation would improve maintainability. var nonDocChars = regexp.MustCompile(`[^a-zA-Z0-9-]`)

📚 Repository Context Analyzed This review considered 27 relevant code sections from 15 files (average relevance: 0.96) help/README.md (lines 1-19) cliv2/scripts/prepare_licenses.go (lines 1-189) cliv2/internal/embedded/cliv1/dummy_embedded_legacy_cli.go (lines 1-23) cliv2/internal/embedded/cliv1/embedded_binary_template.txt (lines 1-22) cliv2/pkg/core/main.go (lines 210-413) cliv2/internal/cliv2/cliv2.go (lines 1-273) cliv2-private/cmd/cliv2/main.go (lines 1-16) ... and 8 more file(s)

🤖 Repository instructions applied (from AGENTS.md)