[jimcadden]

Summary

The vendoring workflow was well-designed but undocumented for contributors (#17): the README never named vendor-engine.sh or the source-of-truth rules, CLAUDE.md carried no repo-topology guidance for LLM contributors, and nothing marked skills/repoman/scripts/ as generated output — inviting in-place edits of vendored scripts that bypass automation's tests and get overwritten on the next vendor.

This PR:

• Adds a ## Development section to the README with the three-repo source-of-truth map — automation = engine dev and testing, repoman = distribution and the canonical home for RepoMan-specific SKILL.mds, agent-skills = skills that work standalone as well as with RepoMan — folding in the former "How it is built" paragraph to avoid duplication.
• Adds a brief five-step Contributor workflow: placement of new work, testing in automation (bash 3.2, ### Requirements blocks for repoman-check.sh), keeping real state out of test runs (REPOMAN_CONFIG_FILE/REPOMAN_REPOS_FILE, --dry-run through repoman-run.sh), vendoring the engine (manifest rule, test-vendor-engine.sh, claude --plugin-dir smoke test), and PR rules (hooks, DCO, conventional title, one purpose).
• Adds a matching Repo Topology & Vendored Code section to CLAUDE.md phrased for LLM contributors: engine changes belong in automation, vendored scripts must never be edited in place, and the per-program skills/<program>/scripts/ copies are known-stale (Resync standalone scanner/fixer skills onto the current modular engine lib #5) and not authoritative.

Fixes #17

Assisted-By: Claude Code

[jimcadden]

Summary

Docs-only: a ## Development section in the README and a matching "Repo Topology & Vendored Code" section in CLAUDE.md. Most claims check out: the links and issue references resolve, tests/test-vendor-engine.sh passes, _vendor.json / repoman-run.sh <program> --dry-run / REPOMAN_CONFIG_FILE+REPOMAN_REPOS_FILE behave as described, and the "per-program copies are known-stale" note is real (e.g. skills/automation-health-dashboard/scripts/ lacks the doc-drift section the vendored copy has). One blocking defect: the local smoke-test command as written does not load this repo as a plugin. Three smaller notes on program registration and vendoring provenance.

Author: jimcadden (MEMBER — maintainer; same account as the reviewer, so this review is COMMENT)
Areas reviewed: docs (README, CLAUDE.md), shell-script claims, tests
Agent/IDE config (.claude/.vscode): none
Commits: 1, all signed-off: yes
CI status: passing (4/4)

Verdict: one must-fix (README:97); the rest are suggestions/nits. Once that line is corrected this is good to merge.

Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).

[jimcadden]

--plugin-dir loads a plugin: it looks for .claude-plugin/plugin.json (or a top-level plugin.json). This repo's root has only .claude-plugin/marketplace.json, and the CLI's own diagnostic for that case says a directory that lists plugins in a marketplace manifest is "not read here; with no plugin.json beside it the folder is not itself a plugin". claude plugin validate . also reports it as a marketplace manifest, not a plugin. So this line does not load the RepoMan plugin, and the "try the plugin" step silently does nothing useful.

A local smoke test that does work:

claude
# inside the session:
/plugin marketplace add <this-repo>
/plugin install repoman@rossoctl-repoman

(or the one-step form on Claude Code 2.1.275+: /plugin install repoman --marketplace <owner>/<repo>). I could not run claude end-to-end here (no credentials in this sandbox), so please either swap the command for the marketplace form or verify --plugin-dir actually loads this layout before keeping it.

[jimcadden]

Fixed in 2a0d0d3. Confirmed independently: the repo root has only .claude-plugin/marketplace.json (no plugin.json), and claude plugin validate . reports it as a marketplace manifest. Replaced with the verified CLI form: claude plugin marketplace add <this-repo> + claude plugin install repoman@rossoctl-repoman (both subcommands confirmed present in claude plugin --help).

Assisted-By: Claude Code

[jimcadden]

Step 1 covers the deterministic script and the SKILL.md, but a new program also needs repoman-side wiring that isn't engine code — #12 needed all of it:

• an entry in .claude-plugin/marketplace.json (without it the program can't be installed at all);
• a row in the README ## Programs table;
• the program → skill mapping in skills/repoman/SKILL.md (the _PROGRAM_TABLE/KNOWN_PROGRAMS registries are engine-side, so automation covers those).

As written, a contributor can follow this to the letter and still ship a skill nobody can install or a program /repoman cannot resolve. Worth naming those three explicitly.

[jimcadden]

Fixed in 2a0d0d3. Step 1 now names all three: the .claude-plugin/marketplace.json entry, the README ## Programs table row, and the program → skill mapping in skills/repoman/SKILL.md — with a note that these are repoman-side wiring, not engine code.

Assisted-By: Claude Code

[jimcadden]

nit: the manifest is duplicated — scripts/vendor-engine.sh's MANIFEST, and tests/test-vendor-engine.sh's own MANIFEST plus the hardcoded EXPECTED_SH/EXPECTED_FILES counts (tests/test-vendor-engine.sh:17-25). Adding a script without bumping the counts makes the regression test fail, so "added to the vendor manifest" undersells the step. Say "both the vendor manifest and its test fixture (including the counts)".

[jimcadden]

Fixed in 2a0d0d3. Step 4 now reads "added to both the vendor manifest and its test fixture in tests/test-vendor-engine.sh, including the expected counts".

Assisted-By: Claude Code

[jimcadden]

vendor-engine.sh stamps git -C "$SRC" rev-parse HEAD with no dirty-tree check (scripts/vendor-engine.sh:65), so vendoring an automation checkout with uncommitted changes — which CLAUDE.md:59 actively invites with "a checkout that has your fix" — records a source_sha that does not contain the vendored bytes. Since this section is specifically about provenance, add "from a clean checkout" next to the invocation (or have the script reject a dirty source; that fix belongs in automation).

[jimcadden]

Fixed in 2a0d0d3 — docs side: the README invocation and the CLAUDE.md bullet now both require a clean automation checkout (fix committed), with the reason stated (the stamp records the source HEAD SHA, so a dirty tree stamps a SHA that does not contain the vendored bytes). Agreed the stronger fix — vendor-engine.sh rejecting a dirty source — belongs in automation; want me to file that as an issue there?

Assisted-By: Claude Code

[jimcadden]

Filed as #20. One correction to the review's note: vendor-engine.sh lives in this repo (scripts/vendor-engine.sh), not in automation — it is not part of the engine manifest — so the fix lands here and the issue is filed on rossoctl/repoman.

Assisted-By: Claude Code

[clawgenti]

Re-review: docs-only revision

Re-reviewed at 2a0d0d3 (docs: Address review feedback on the contributor workflow). The one must-fix from the previous review is resolved: README now uses the verified marketplace form (claude plugin marketplace add <this-repo> then claude plugin install repoman@rossoctl-repoman) instead of the non-working --plugin-dir invocation.

Claims in the new ## Development section and the CLAUDE.md topology section were spot-checked against origin/main of this repo and of rossoctl/automation:

• scripts/vendor-engine.sh accepts --source/--dest, copies an explicit manifest (no glob), pre-validates the whole manifest before copying, and stamps the source HEAD SHA — matching the README's description.
• tests/test-vendor-engine.sh carries its own manifest plus EXPECTED_SH=26/EXPECTED_FILES=27, so "both the vendor manifest and its test fixture … including the expected counts" is accurate.
• REPOMAN_CONFIG_FILE/REPOMAN_REPOS_FILE are honored in org.sh/repoman-setup.sh; repoman-run.sh <program> [target...] [--dry-run] matches its usage.
• The ### Requirements block is parsed by repoman-requirements.sh; repoman-setup.sh writes the result into programs/<name>.json, which repoman-check.sh gates on.
• Marketplace name rossoctl-repoman and plugin repoman match .claude-plugin/marketplace.json; the program → skill table exists in skills/repoman/SKILL.md; docs/program-template.md exists in automation; issue #5 is open with the "known-stale" title.
• No remaining references to the removed "How it is built" anchor.

Author: jimcadden (CONTRIBUTOR — returning external)
Areas reviewed: docs (README.md, CLAUDE.md)
Agent/IDE config (.claude/.vscode): none
Commits: 2, all signed-off: yes
CI status: passing (3/3)

Verdict: no must-fix findings. One nit inline (the *.sh glob in CLAUDE.md omits the vendored doc-drift-check.py). Approval is left to a human maintainer because the author association is CONTRIBUTOR — this automated review does not approve.

Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).

[clawgenti]

nit: this glob only covers the shell scripts — skills/repoman/scripts/ also contains the vendored doc-drift-check.py (it is in vendor-engine.sh's extras and tests/test-vendor-engine.sh asserts on it). Since the point is to stop in-place edits of generated output, phrase this as the whole skills/repoman/scripts/ directory (or name the Python file too); as written an agent could read *.sh as leaving doc-drift-check.py editable.

[jimcadden]

Fixed in fbb31c0. The CLAUDE.md bullet now covers everything under skills/repoman/scripts/ — shell scripts and non-shell extras — and names doc-drift-check.py explicitly, so the glob can no longer be read as leaving it editable.

(Note: an earlier reply of mine landed on the wrong thread — the plugin-dir comment above — please disregard it there.)

Assisted-By: Claude Code

[clawgenti]

Re-review: provenance guard + docs revision

Re-reviewed at fbb31c0 (fix: Reject a dirty source tree in vendor-engine.sh). Since the previous review at 2a0d0d3, the branch adds the dirty-source guard, its regression tests, and the matching docs notes.

Verified at HEAD:

• tests/test-vendor-engine.sh passes 13/13 locally, including the new cases: dirty source rejected before any copy (dest left uncreated), and the --allow-dirty override copies successfully.
• Guard placement is correct — it runs before mkdir -p "$DEST", so a rejected run touches nothing.
• Manifest/stamp claims still hold: automation's scripts/ carries exactly the 26 shell scripts plus doc-drift-check.py, and skills/repoman/scripts/ holds those 27 plus _vendor.json — so the "everything under that directory is vendored output" wording is accurate and the old *.sh-glob nit is gone.
• Issue refs resolve: repoman#20 (provenance mismatch), #17 (this docs work), #5 (known-stale per-program copies).
• No references remain to the removed "How it is built" anchor; marketplace/plugin names, the scripts/hooks path, and the program → skill wiring all check out.
• No .claude/ or .vscode/ changes.

Author: jimcadden (CONTRIBUTOR — returning external)
Areas reviewed: shell (scripts/vendor-engine.sh), tests (tests/test-vendor-engine.sh), docs (README.md, CLAUDE.md)
Agent/IDE config (.claude/.vscode): none
Commits: 3, all signed-off: yes
CI status: passing (3/3 — DCO, shellcheck, verify-pr-title)

Verdict: no must-fix findings. One suggestion inline (the guard silently no-ops when git -C "$SRC" status itself fails). Approval is left to a human maintainer because the author association is CONTRIBUTOR — this automated review does not approve.

Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).

[clawgenti]

suggestion — the guard fails open: git -C "$SRC" status --porcelain 2>/dev/null produces empty output both when the tree is clean and when git cannot inspect it at all (not a git checkout, git missing, $SRC unresolvable). In that second case the guard passes and the run proceeds: the manifest pre-validation passes, every file is copied into $DEST, and only then does git rev-parse HEAD fail — leaving $DEST freshly overwritten with a stale or absent _vendor.json. That is the stamp/bytes mismatch this PR exists to prevent, reachable from a source path that isn't a git checkout (e.g. an exported tarball).

Since the stamp requires HEAD anyway, failing early on inspection failure is cheap:

if [ "$ALLOW_DIRTY" = false ]; then
  if ! DIRTY=$(git -C "$SRC" status --porcelain 2>&1); then
    echo "ERROR: cannot inspect $SRC for uncommitted changes (not a git checkout?)" >&2; exit 1
  fi
  [ -z "$DIRTY" ] || { echo "ERROR: source $SRC has uncommitted changes; commit or stash them so the provenance stamp matches the vendored bytes (or pass --allow-dirty)" >&2; exit 1; }
fi

Worth a regression case too: a source directory with every manifest file but no .git should fail before copying. Not blocking — the documented workflow points at a real checkout.

[rubambiza]

@jimcadden As the AI agents like to say "belt-and-suspenders". Worth fixing.

[jimcadden]

Fixed in 1606d1d. Went with the fail-early shape from the sibling thread (an up-front rev-parse --git-dir check before any copy) rather than tightening the status capture itself — once rev-parse succeeds, the existing --porcelain 2>/dev/null empty-output case can only mean clean, and the up-front form also covers --allow-dirty (the stamp needs a source HEAD regardless).

Your regression case is in: tests/test-vendor-engine.sh now builds a source with every manifest file but no .git and asserts it fails before copying (dest left uncreated), both with and without --allow-dirty.

Assisted-By: Claude Code

[rubambiza]

A few suggested changes and observations. AI-assisted review will follow.

[rubambiza]

I think it's about time to fix #5 before releasing v0.1.3. I have assigned it to my local Claude, but with the instruction/understanding that merging a PR for it is non-blocking for this one.

[rubambiza]

Update: #5 cannot land until this one merges because the mechanistic part of it to be done on repoman would also change the same set of files that this PR does.

[jimcadden]

Acknowledged — sequencing noted: this merges first, then #5 (its mechanistic repoman-side changes touch the same files as this PR). No action here.

Assisted-By: Claude Code

[rubambiza]

This will likely need to change during the development of #5 to include vendoring the scripts to the individual skills, too. Non-blocking comment; simply an observation after a recent investigation on the ordering of this repo vs #5 work.

[jimcadden]

Makes sense — this section describes the current single-dest vendoring, and #5 will extend it to the per-program skills/<program>/scripts/ copies when it lands. Leaving as-is for this PR.

Assisted-By: Claude Code

[rubambiza]

AI-assisted pass complementing the human review on this PR (which already covered the README/CLAUDE.md wording, the belt-and-suspenders fix at vendor-engine.sh:28, and the #5 ordering dependency). This pass focused on the code change — the new dirty-source provenance guard and its test — and does not re-raise those points.

The guard's intent is right (a dirty source would make _vendor.json stamp a commit that does not contain the copied bytes, repoman#20) and both the reject path and the --allow-dirty override are tested, including that a dirty reject leaves the dest uncreated. One non-blocking edge below.

§3.5a supply-chain gate: clean — no .claude/ or .vscode/ changes.

Author: jimcadden (MEMBER — maintainer)
Areas reviewed: Shell (vendor-engine.sh), Tests; docs deferred to the human pass
Agent/IDE config (.claude/.vscode): none
Commits: 3, all signed-off
CI status: passing (DCO, shellcheck, verify-pr-title)

Approving; the single finding is a suggestion, not a blocker.

Assisted-By: Claude Code

[rubambiza]

suggestion (non-blocking): the guard swallows git -C "$SRC" status --porcelain 2>/dev/null, so a non-git --source (a wrong path that exists, or a tarball extract) yields empty output and is treated as clean — the guard passes. The copy loop then writes the full engine into --dest, and only the later git rev-parse HEAD fails loud, leaving a half-vendored dest with no _vendor.json. Verified by running with a non-git source: 26 scripts copied, then ERROR: cannot read source HEAD sha, dest left behind.

Since the guard exists to keep the stamp honest, it should fail-closed when $SRC is not a git repo — e.g. an up-front git -C "$SRC" rev-parse --git-dir >/dev/null 2>&1 || { echo ... ; exit 1; } before any copy — rather than letting "can't determine cleanliness" read as "clean". Normal use (an automation checkout) is unaffected.

[jimcadden]

Fixed in 1606d1d, along the lines of your suggestion: an up-front git -C "$SRC" rev-parse --git-dir check now rejects an uninspectable source before anything is copied. It is deliberately unconditional — outside the --allow-dirty path — since the stamp needs a source HEAD either way, so --allow-dirty no longer postpones the same failure to after the copy loop.

Also added the regression case clawgenti asked for on the sibling thread: a source holding the full manifest but no .git fails (with and without --allow-dirty) leaving the dest uncreated. Verified your repro end-to-end: immediate error, 0 files copied.

Assisted-By: Claude Code