feat: Add org-portability seam (load_org_profile + bare core-repos) - #43
Conversation
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
rubambiza
added
the
ready-for-ai-review
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Org-portability seam looks solid — load_org_profile precedence chain is correct, bare-name conversion in core-repos.txt is consistent with get_core_repos, and the REMAP-driven canonical_repo_for_dir loop handles malformed entries properly. All 3 commits are signed off and DCO passes.
1 nit on a misleading comment block.
Reviewed by clawgenti using github:pr-review
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
|
Addressed the comment-block nit in |
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
One new docs: commit since the last review — clarifies the hermetic-test comment in test-org-profile.sh. No functional change; rest of the diff is unchanged.
All checks pass. Ready for human review.
Reviewed by clawgenti using github:pr-review
| echo "ERROR: ORG could not be resolved (flag/env/profile all empty)" >&2 | ||
| return 1 | ||
| fi | ||
| FORK_OWNER="${FORK_OWNER_FLAG:-${FORK_OWNER:-${PROFILE_FORK_OWNER:-clawgenti}}}" |
There was a problem hiding this comment.
nit: The fallback clawgenti is org-specific. For full portability the profile should be required to set PROFILE_FORK_OWNER, or the default should be documented as rossoctl-deployment-only. Not a blocker — just worth a comment in the function doc or org.env.
There was a problem hiding this comment.
Addressed in commit 1423ad9 — documented the clawgenti fallback in the load_org_profile doc header as the rossoctl deployment default, and noted that other orgs should set PROFILE_FORK_OWNER (config/org.env already does). Kept it as a documented default rather than a hard requirement, per the approved design's four-fact model where only ORG has no built-in default.
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
One new docs: commit since the last review — documents the clawgenti FORK_OWNER fallback in the load_org_profile header, directly addressing the prior nit. No functional change; prior findings resolved.
All checks pass. Ready for human review.
Reviewed by clawgenti using github:pr-review
rubambiza
added
ready-for-human-review
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
Clean, well-scoped seam PR that introduces load_org_profile() and converts the core-repo list to bare names, with genuinely zero net behavior change until the follow-up rewire PR. The precedence resolution is correct and the PROFILE_-prefix clobber-safety design is sound: sourcing the profile cannot overwrite an env-provided value before resolution. Test coverage is excellent: the new test-org-profile.sh exercises all four precedence tiers, clobber-safety, defaults, and both fail-loud paths hermetically; test-core-repos.sh adds ORG-derived-owner, fail-loud-on-unset-ORG, and empty-REMAP-identity cases.
I verified the key claims: the referenced design doc exists on main, and pr-review-scanner.sh calls get_core_repos() with no load_org_profile/ORG set, so the "integration test is expected red on this branch, green with the rewire PR" disclosure is accurate. The canonical_repo_for_dir malformed-entry guard handles the no-colon and empty-basename cases correctly.
- Areas reviewed: Shell, config/data files, tests, docs (comments)
- Commits: 5, all signed-off, emoji + imperative subjects
- CI: DCO passing; integration test intentionally red on this branch by design (clearly disclosed)
Two non-blocking findings inline (1 suggestion, 1 nit). The dual-PR "do not deploy alone" contract is well documented. Nice work.
| FORK_OWNER="${FORK_OWNER_FLAG:-${FORK_OWNER:-${PROFILE_FORK_OWNER:-clawgenti}}}" | ||
| MAIN_REPO="${MAIN_REPO_FLAG:-${MAIN_REPO:-${PROFILE_MAIN_REPO:-$ORG/$ORG}}}" | ||
| REPOS_DIR="${REPOS_DIR_FLAG:-${REPOS_DIR:-${PROFILE_REPOS_DIR:-$HOME/$ORG}}}" | ||
| REMAP="${PROFILE_REMAP:-}" |
There was a problem hiding this comment.
suggestion: REMAP resolves from the profile only (${PROFILE_REMAP:-}), with no --flag/env tier, unlike ORG/FORK_OWNER/MAIN_REPO/REPOS_DIR, which all honor the full flag > env > profile > default precedence. The function's doc comment lists REMAP among the facts it "Sets," so the asymmetry is easy to miss: a caller who exports REMAP will have it silently overwritten (or blanked, if the profile omits PROFILE_REMAP). If profile-only is intentional for this transitional field, a one-line note here ("REMAP is profile-only by design, no flag/env override") would prevent a future surprise.
There was a problem hiding this comment.
Good catch on the asymmetry. Addressed in 0f8734a: added a note at the REMAP assignment that it is profile-only by design (transitional field, self-retires with #37, so it never earns a durable flag/env knob; an exported REMAP is ignored), and amended the doc header's "Sets" line to call out that the first four facts honor the full precedence chain while REMAP does not.
| *) echo "$dir_name" ;; | ||
| esac | ||
| local pair basename_part canon_part | ||
| for pair in ${REMAP:-}; do |
There was a problem hiding this comment.
nit: for pair in ${REMAP:-} relies on intentional word-splitting of the space-separated pairs, which is correct, but shellcheck will flag it as SC2086. A # shellcheck disable=SC2086 -- intentional split on space-separated pairs keeps a clean lint run and documents the intent.
There was a problem hiding this comment.
Done in 0f8734a — added # shellcheck disable=SC2086 -- intentional word-split on space-separated pairs above the loop.
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
3 participants
Summary
Introduces the org-portability seam — the machinery that lets the scanner/fixer suite target any GitHub org via one committed profile — with zero net behavior change. This is the first of two PRs; nothing calls the new machinery yet.
load_org_profile()inprogram-lib.shresolves four org-identity facts by precedence--flag > env > profile > builtin default:ORG(fail-loud if unset),FORK_OWNER(defaultclawgenti),MAIN_REPO(default$ORG/$ORG),REPOS_DIR(default$HOME/$ORG), plus a transitionalREMAP. Profile files assign onlyPROFILE_-prefixed keys so sourcing cannot clobber an env-provided value before precedence resolves.config/org.env— the default profile (rossoctl).config/core-repos.txt— converted to bare names;get_core_repos()now prepends$ORG.canonical_repo_for_dir()now reads$REMAP(data) instead of a hardcodedkagenti/kagenti-extensionscase.This PR changes the contract of
get_core_repos(): it now fails loud whenORGis unset. No consumer sources the loader yet — that wiring lands in the follow-up "rewire" PR. Deploying the seam by itself would break the scanners at runtime.The change is behavior-preserving only when paired with the rewire PR. They must land and deploy together.
As a direct consequence,
tests/test-pr-review-integration.shis expected to be red on this branch (pr-review-scanner.shhas no loader call yet); it passes withORGexported and is restored to green by the rewire PR. All other tests pass, including the newtests/test-org-profile.sh(precedence, clobber-safety, defaults, fail-loud) and the updatedtests/test-core-repos.sh(ORG-derived owner, fail-loud-on-unset-ORG, REMAP-driven canonical with empty-REMAP-identity).Testing
bash tests/test-org-profile.sh→ PASSbash tests/test-core-repos.sh→ PASSORG=rossoctl ... get_core_reposresolves exactly the 8 core repos (rossoctl/rossoctl…rossoctl/workload-harness).