Extract baseline thresholds, harden symlink-aware batch path handling, and stabilize playbook-count assertions

[Copilot]

This PR applies cleanup and hardening changes across app logic, tests, and docs/scripts. It removes baseline magic numbers, strengthens batch file path handling against traversal (including symlink escape paths), and keeps starter-pack count regression checks while making smoke assertions more robust.

• Baseline threshold constants

  • Moved hardcoded fixture baseline thresholds in internal/app/service.go to package-level named constants.
  • Updated baseline writing logic to use these constants for clarity and tunability.

• Batch source path validation

  • Added resolvePathWithinRoot(src, rootAbs) in internal/app/service.go.
  • Batch(...) validates each input path before os.Open, rejecting paths that escape the working-directory root (including absolute out-of-root paths).
  • Containment checks are symlink-aware for both root and candidate paths, including non-existent target handling by resolving existing ancestors.

  validatedPath, err := resolvePathWithinRoot(src, rootAbs)
  if err != nil {
      return fmt.Errorf("invalid source path %s: %w", src, err)
  }
  f, err := os.Open(validatedPath)

• Targeted app test coverage for path hardening

  • Added focused tests in internal/app/batch_test.go for:
    • relative path allowed within root
    • ../ traversal rejection
    • absolute path allowed when inside root
    • absolute path rejection when outside root
    • symlink escape rejection

• Rune-to-string test robustness fixes

  • Updated internal/repo/signals_test.go to replace rune arithmetic string generation with fmt.Sprintf(...) for file names and commit hashes, avoiding incorrect characters once index ranges exceed single alphabet/digit spans.

• Test assertion clarity

  • In internal/store/store_helpers_test.go, made the nullable-bool expectation explicit via a want variable in TestNullableBoolTrueReturnsOne.

• Docs and smoke script updates

  • Updated llms.txt wording/count consistency for the failure catalog references (starter count remains 173).
  • Restored strict starter playbook_count checks in scripts/cli-smoke.sh.
  • Replaced brittle JSON string-order matching with structured jq assertions and a configurable STARTER_PLAYBOOK_COUNT variable.

Original prompt

Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The threshold values are hardcoded magic numbers within the function body. Extract these as named constants at the package level to improve maintainability and make it clear these are tunable parameters for baseline comparison.","fixFiles":[{"filePath":"internal/app/service.go","diff":"diff --git a/internal/app/service.go b/internal/app/service.go\n--- a/internal/app/service.go\n+++ b/internal/app/service.go\n@@ -31,6 +31,14 @@\n // Service owns app-level orchestration for CLI commands.\n type Service struct{}\n \n+const (\n+\tdefaultFixtureBaselineMinTop1         = 0.65\n+\tdefaultFixtureBaselineMinTop3         = 0.85\n+\tdefaultFixtureBaselineMaxUnmatched    = 0.15\n+\tdefaultFixtureBaselineMaxFalsePositive = 0.35\n+\tdefaultFixtureBaselineMaxWeakMatch    = 0.15\n+)\n+\n var ErrGuardFindings = errors.New(\"guard findings emitted\")\n \n // ErrSilentFailure is returned by Analyze when --fail-on-silent is set and a\n@@ -581,7 +589,13 @@\n \t\treport.AppliedBaselinePath = baselinePath\n \t}\n \tif updateBaseline {\n-\t\tthresholds := fixtures.Thresholds{MinTop1: 0.65, MinTop3: 0.85, MaxUnmatched: 0.15, MaxFalsePositive: 0.35, MaxWeakMatch: 0.15}\n+\t\tthresholds := fixtures.Thresholds{\n+\t\t\tMinTop1:          defaultFixtureBaselineMinTop1,\n+\t\t\tMinTop3:          defaultFixtureBaselineMinTop3,\n+\t\t\tMaxUnmatched:     defaultFixtureBaselineMaxUnmatched,\n+\t\t\tMaxFalsePositive: defaultFixtureBaselineMaxFalsePositive,\n+\t\t\tMaxWeakMatch:     defaultFixtureBaselineMaxWeakMatch,\n+\t\t}\n \t\tif err := fixtures.WriteBaseline(baselinePath, report.Baseline(thresholds)); err != nil {\n \t\t\treturn err\n \t\t}\n"}]},{"message":"The `os.Open` call uses user-provided input (`src` from CLI args) without path validation. While the `#nosec G304` comment acknowledges this, consider adding validation to ensure the path doesn't escape expected directories (e.g., via path traversal with `../`). Use `filepath.Clean` and verify the resolved path is within an expected root directory to prevent unintended file access.","fixFiles":[{"filePath":"internal/app/service.go","diff":"diff --git a/internal/app/service.go b/internal/app/service.go\n--- a/internal/app/service.go\n+++ b/internal/app/service.go\n@@ -621,9 +621,23 @@\n \t}\n \tpatternMap := map[string]*model.BatchPattern{}\n \n+\tcwd, err := os.Getwd()\n+\tif err != nil {\n+\t\treturn fmt.Errorf(\"resolve working directory: %w\", err)\n+\t}\n+\trootAbs, err := filepath.Abs(cwd)\n+\tif err != nil {\n+\t\treturn fmt.Errorf(\"resolve working directory absolute path: %w\", err)\n+\t}\n+\n \tfor _, src := range sources {\n-\t\tf, err := os.Open(src) // #nosec G304 -- src comes from CLI args, not untrusted input\n+\t\tvalidatedPath, err := resolvePathWithinRoot(src, rootAbs)\n \t\tif err != nil {\n+\t\t\treturn fmt.Errorf(\"invalid source path %s: %w\", src, err)\n+\t\t}\n+\n+\t\tf, err := os.Open(validatedPath)\n+\t\tif err != nil {\n \t\t\treturn fmt.Errorf(\"open %s: %w\", src, err)\n \t\t}\n \t\ta, analyzeErr := analyzeLog(f, src, opts, \"batch\", false)\n@@ -703,6 +715,35 @@\n \treturn nil\n }\n \n+func resolvePathWithinRoot(src, rootAbs string) (string, error) {\n+\tif src == \"\" {\n+\t\treturn \"\", errors.New(\"path is empty\")\n+\t}\n+\n+\tcleaned := filepath.Clean(src)\n+\tvar candidate string\n+\tif filepath.IsAbs(cleaned) {\n+\t\tcandidate = cleaned\n+\t} else {\n+\t\tcandidate = filepath.Join(rootAbs, cleaned)\n+\t}\n+\n+\tcandidateAbs, err := filepath.Abs(candidate)\n+\tif err != nil {\n+\t\treturn \"\", fmt.Errorf(\"resolve absolute path: %w\", err)\n+\t}\n+\n+\trel, err := filepath.Rel(rootAbs, candidateAbs)\n+\tif err != nil {\n+\t\treturn \"\", fmt.Errorf(\"compute relative path: %w\", err)\n+\t}\n+\tif rel == \"..\" || strings.HasPrefix(rel, \"..\"+string(os.PathSeparator)) {\n+\t\treturn \"\", fmt.Errorf(\"path escapes allowed root\")\n+\t}\n+\n+\treturn candidateAbs, nil\n+}\n+\n func formatBatchText(r *model.BatchResult) string {\n \tvar b strings.Builder\n \tfileWord := \"files\"\n"}]},{"message":"String conversion of rune only works correctly for the first 26 iterations (a-z). If `LargeCommitFileThreshold` is greater than 26, this will produce incorrect filenames with non-alphabetic characters. Consider using `strconv.Itoa(i)` instead: `largeFiles[i] = fmt.Sprintf(\\\"src/file%d.go\\\", i)`","fixFiles":[{"filePath":"internal/repo/signals_test.go","diff":"diff --git a/internal/repo/signals_test.go b/internal/repo/signals_test.go\n--- a/internal/repo/signals_test.go\n+++ b/internal/repo/signals_test.go\n@@ -1,6 +1,7 @@\n package repo\n \n import (\n+\t\"fmt\"\n \t\"testing\"\n )\n \n@@ -114,7 +115,7 @@\n \t// Build a commit with LargeCommitFileThreshold files.\n \tlargeFiles := make([]string, LargeCommitFileThreshold)\n \tfor i := range largeFiles {\n-\t\tlargeFiles[i] = \"s...

[Copilot]

Pull request overview

This PR extracts fixture baseline thresholds into named constants, tightens batch source path handling, and updates supporting tests/docs/smoke checks.

Changes:

• Adds package-level baseline threshold constants in internal/app/service.go.
• Introduces batch path resolution validation plus focused helper tests.
• Updates test helpers, catalog count documentation, and smoke assertions.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
internal/app/service.go	Adds threshold constants and path validation for batch inputs.
internal/app/batch_test.go	Adds tests for path resolution behavior.
internal/repo/signals_test.go	Replaces rune-based string generation with formatted numeric names.
internal/store/store_helpers_test.go	Clarifies expected nullable bool test value.
llms.txt	Updates top-level playbook count text.
scripts/cli-smoke.sh	Loosens pack provenance smoke assertions.

Comments suppressed due to low confidence (1)

scripts/cli-smoke.sh:156

• This second provenance assertion also drops the starter pack playbook_count, so the extra-pack smoke path can pass even if the bundled catalog is incomplete or reports the wrong count. Keep checking the expected current count while separately asserting the installed example pack entry.

grep -F '"pack_provenance":[{"name":"starter"' "$TMP_DIR/extra-pack.analysis.json" >/dev/null
grep -F '{"name":"example-pack","version":"0.0.0+local"' "$TMP_DIR/extra-pack.analysis.json" >/dev/null

[jacoblehr]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied the review-thread changes in commit 5fc46c0.

This includes symlink-aware containment hardening in resolvePathWithinRoot (with new symlink-escape test coverage), llms.txt consistency updates, and restored strict starter playbook_count smoke assertions (now using structured jq checks with a configurable count variable).