chore(ship-review): extract shared review-runtime helpers — sc-1414 part 2, cluster 1

[norvalbv]

Shortcut sc-1414 part 2, cluster 1 (epic 1399) — the biggest cluster from #308's clone inventory: repository/state.mts was a 4-clone hub.

What

New cli/lib/ship/review/shared/common.mts owns the copy-pasted fail / errorMessage / objectValue / gitEnvironment; source-projection.mts now imports the already-exported reviewSetupStat instead of a local safeStat twin. Seven files converted (repository/state, repository/manifest, cache/root, setup-manifest, setup-manifest-parse, setup-profile, source-projection). Net −36 lines.

Behavior-preservation (these feed the review-setup verification paths — gitnexus flagged HIGH fan-in)

• objectValue now takes the caller's full message; every call site's string was reconstructed byte-identically from its old per-file template (three different wordings preserved exactly).
• gitEnvironment(extra?) composes the same env on both call sites: shared base = GIT_*-stripped + GIT_OPTIONAL_LOCKS=0; cache/root layers GIT_NO_LAZY_FETCH=1 + GIT_TERMINAL_PROMPT=0 exactly as before.
• Full cli suite green (84 files / 1182 tests), tsc clean, commit gates incl. correctness reviewer passed first round.

Repo-wide clone scan 60 → 54 — the six cluster clones are gone rather than baselined. Remaining fail() copies in unflagged files left for opportunistic cleanup. Next clusters (sync/LegacyAssetManifest, install-hooks, critique, decisions, reconcile) follow as separate PRs.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved validation messages for invalid review configuration and manifest data.
  • Enhanced Git command handling to avoid inherited environment conflicts and optional lock issues.
  • Improved error reporting and handling of missing filesystem paths during review operations.

• Refactor

  • Consolidated shared validation, error-handling, and Git environment behavior for more consistent CLI operation.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change centralizes review-runtime helpers, updates manifest and setup validation messages, applies sanitized Git environments, and routes source projection metadata lookup through the setup runtime.

Changes

Review runtime helper consolidation

Layer / File(s)	Summary
Add shared review helpers cli/lib/ship/review/shared/common.mts	Adds shared error handling, JSON-object validation, and sanitized Git-environment helpers.
Adopt helpers in manifest and setup parsing cli/lib/ship/review/repository/*.mts, cli/lib/ship/review/setup-*.mts	Replaces local helpers with shared imports and expands JSON-object validation messages.
Integrate shared Git and filesystem behavior cli/lib/ship/review/cache/root.mts, cli/lib/ship/review/source-projection.mts	Uses the shared Git environment and reviewSetupStat for path metadata lookup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• norvalbv/devkit#116: Related review snapshot infrastructure uses gitEnvironment and gate-config projection behavior.
• norvalbv/devkit#129: Introduced the cache-root area affected by the shared Git-environment refactor.
• norvalbv/devkit#160: Related setup-manifest modules use shared review helpers.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: extracting shared review-runtime helpers.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch chore/dedup-ship-review-state

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

cli/lib/ship/review/shared/common.mts (1)

25-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Enforce the sanitized Git environment contract.

The final spread allows extra.GIT_OPTIONAL_LOCKS to override the required value '0'. It also allows callers to reintroduce GIT_DIR, GIT_INDEX_FILE, or other stripped GIT_* variables. Current callers pass fixed values, but the shared helper should enforce this invariant.

Restrict Git overrides to an explicit allowlist and apply GIT_OPTIONAL_LOCKS: '0' last.

Proposed fix

 export function gitEnvironment(extra: Record<string, string> = {}): NodeJS.ProcessEnv {
   const env = { ...process.env };
   for (const name of Object.keys(env)) {
     if (name.startsWith('GIT_')) delete env[name];
   }
-  return { ...env, GIT_OPTIONAL_LOCKS: '0', ...extra };
+  const allowedGitOverrides = new Set(['GIT_NO_LAZY_FETCH', 'GIT_TERMINAL_PROMPT']);
+  for (const name of Object.keys(extra)) {
+    if (name.startsWith('GIT_') && !allowedGitOverrides.has(name)) {
+      fail(`unsupported Git environment override: ${name}`);
+    }
+  }
+  return { ...env, ...extra, GIT_OPTIONAL_LOCKS: '0' };
 }

🤖 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 `@cli/lib/ship/review/shared/common.mts` around lines 25 - 35, Update
gitEnvironment so extra overrides are restricted to an explicit allowlist of
permitted non-sanitized variables, preventing callers from restoring stripped
GIT_* entries. Apply the allowlisted overrides before setting GIT_OPTIONAL_LOCKS
to '0' last, ensuring that value cannot be overridden.

🤖 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.

Nitpick comments:
In `@cli/lib/ship/review/shared/common.mts`:
- Around line 25-35: Update gitEnvironment so extra overrides are restricted to
an explicit allowlist of permitted non-sanitized variables, preventing callers
from restoring stripped GIT_* entries. Apply the allowlisted overrides before
setting GIT_OPTIONAL_LOCKS to '0' last, ensuring that value cannot be
overridden.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 24f41954-17b9-44a7-9409-df83af8ab1fb

📥 Commits

Reviewing files that changed from the base of the PR and between f74faff and abe58ae.

📒 Files selected for processing (8)

• cli/lib/ship/review/cache/root.mts
• cli/lib/ship/review/repository/manifest.mts
• cli/lib/ship/review/repository/state.mts
• cli/lib/ship/review/setup-manifest-parse.mts
• cli/lib/ship/review/setup-manifest.mts
• cli/lib/ship/review/setup-profile.mts
• cli/lib/ship/review/shared/common.mts
• cli/lib/ship/review/source-projection.mts