Pi profile support, Codex family history, and Keycloak broker design - #3
Pi profile support, Codex family history, and Keycloak broker design#3brettheap wants to merge 10 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR adds isolated, multi-account Pi Coding Agent “profiles” aligned to existing provider identity manifests, introducing a canonical ppi launcher that sets PI_CODING_AGENT_DIR and integrates the new ~/.pi-profiles home into setup flows and container mounts.
Changes:
- Add Pi profile composition + setup scripts (
compose-pi-profiles.py,setup-pi-profiles.sh) and api-profile/ppilauncher. - Mount and surface
~/.pi-profilesin Wave/devBench container tooling and base images, and update setup UI/interactive setup messaging. - Add encrypted Pi credential escrow helper and accompanying credential-manager tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/wave-container-shell.sh | Mount ~/.pi-profiles and install pi-profile/ppi launcher inside Wave containers. |
| scripts/setup-windows-tools.sh | Improve Windows/WSL Pi installation flow and messaging for ppi. |
| scripts/setup-ui/src/utils/installers.ts | Update setup UI post-install guidance to use ppi login PROFILE. |
| scripts/setup-pi-profiles.sh | New host-side Pi profile tree setup and launcher symlinks. |
| scripts/setup-ai-profiles.sh | Generate pi-profiles.json from provider manifests and run Pi profile setup. |
| scripts/pi-credential-escrow | New encrypted escrow flow for Pi auth.json (backup/restore/check). |
| scripts/interactive-setup.sh | Detect Pi creds across standard + isolated homes; guide login via ppi. |
| scripts/compose-pi-profiles.py | Compose Pi profiles from provider manifests and retained launcher metadata. |
| README.md | Update Pi Terminal setup description to reference isolated ppi usage. |
| docs/pi-multi-account-profiles.md | New documentation for Pi multi-account profiles and escrow workflow. |
| docs/newBench.md | Add ~/.pi-profiles to standard devcontainer mount template. |
| docs/multi-provider-profiles.md | Document ppi alongside other provider profile launchers. |
| docs/MOUNTS-README.md | Document ~/.pi-profiles mount and purpose. |
| docker-compose.mounts.yml | Add standard mount for ~/.pi-profiles. |
| base-image/install-ai-clis.sh | Install Pi Coding Agent CLI in the base image. |
| base-image/files/pi-profile | New pi-profile launcher script with ppi alias behavior. |
| base-image/Dockerfile | Ship pi-profile and symlink ppi in the base image. |
| apps/credential-manager/test_pi_profiles.py | Add tests covering composition/idempotency/isolation and escrow round-trip behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ls "$config_dir"/{claude,openai,gemini,grok,glm}-profiles.json >/dev/null 2>&1; then | ||
| pi_profile_roots=( | ||
| --profile-root "claude=${CLAUDE_PROFILES_HOME:-$HOME/.claude-profiles}/profiles" | ||
| --profile-root "openai=${CODEX_PROFILES_HOME:-$HOME/.chatgpt-profiles}/profiles" | ||
| --profile-root "gemini=${GEMINI_PROFILES_HOME:-$HOME/.gemini-profiles}/profiles" | ||
| --profile-root "grok=${GROK_PROFILES_HOME:-$HOME/.grok-profiles}/profiles" | ||
| --profile-root "glm=${GLM_PROFILES_HOME:-$HOME/.glm-profiles}/profiles" | ||
| ) | ||
| python3 "$repo_dir/scripts/compose-pi-profiles.py" \ | ||
| --config-dir "$config_dir" \ | ||
| "${pi_profile_roots[@]}" \ | ||
| --output "$config_dir/pi-profiles.json" | ||
| fi |
| command -v jq >/dev/null 2>&1 || { echo "jq is required." >&2; exit 1; } | ||
| jq -e '.version == 1 and (.profiles | type == "array") and all(.profiles[]; (.name|length)>0 and (.email|length)>0 and (.family|length)>0 and ((.providers//[])|type=="array"))' "$manifest" >/dev/null | ||
|
|
| ln -sfn "$repo_dir/scripts/pi-profile" "$HOME/.local/bin/pi-profile" | ||
| ln -sfn "$repo_dir/scripts/pi-profile" "$HOME/.local/bin/ppi" |
| def add_profile(profiles: dict[str, dict], provider: str, raw: dict, source: pathlib.Path) -> None: | ||
| if not isinstance(raw, dict) or any(not raw.get(field) for field in ("name", "email", "family")): | ||
| raise ValueError(f"invalid {provider} profile in {source}") | ||
| normalized = { | ||
| "email": raw["email"], | ||
| "family": raw["family"], | ||
| "aliases": sorted(raw.get("aliases") or []), | ||
| } |
| pi) | ||
| echo -e "${CYAN}▶ Setting up Pi Coding Agent authentication...${NC}" | ||
| echo -e "${DIM}Pi uses /login inside its interactive terminal.${NC}" | ||
| read -p "Launch Pi now? [Y/n]: " launch_pi | ||
| echo -e "${DIM}Pi credentials are isolated by canonical profile through ppi.${NC}" | ||
| read -p "Launch a Pi profile login now? [Y/n]: " launch_pi | ||
| if [[ ! $launch_pi =~ ^[Nn] ]]; then | ||
| echo -e "${YELLOW}Run /login, choose a provider, then exit Pi when finished.${NC}" | ||
| pi | ||
| read -r -p "Pi profile name or alias: " pi_profile | ||
| echo -e "${YELLOW}Run /login, choose a provider, verify the matching identity, then exit Pi.${NC}" | ||
| ppi login "$pi_profile" | ||
| else | ||
| echo -e "${YELLOW}Skipped. Run 'pi' and enter /login anytime.${NC}" | ||
| echo -e "${YELLOW}Skipped. Run 'ppi login PROFILE' anytime.${NC}" | ||
| fi |
| pi_cli) | ||
| if command -v pi &>/dev/null; then | ||
| [[ -f "$HOME/.pi/agent/auth.json" ]] && echo "installed" || echo "needs creds" | ||
| if [[ -f "$HOME/.pi/agent/auth.json" ]] || find "$HOME/.pi-profiles/profiles" -mindepth 3 -maxdepth 3 -name auth.json -print -quit 2>/dev/null | grep -q .; then |
| elif command -v npm >/dev/null 2>&1; then | ||
| echo " Installing Pi Terminal in WSL/Linux for ppi and devBench use..." | ||
| wsl_npm="$(command -v npm)" | ||
| [[ -x /usr/bin/npm ]] && wsl_npm=/usr/bin/npm | ||
| if "$wsl_npm" install -g --ignore-scripts "$PI_NPM_PACKAGE"; then | ||
| echo " ✓ Pi Terminal installed for WSL/Linux" |
| ) | ||
| ' "$1" >/dev/null | ||
| } | ||
| json_hash() { jq -S -c . "$1" | sha256sum | awk '{print $1}'; } |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8528a21a98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| jq -r --arg name "$normalized" ' | ||
| .profiles[] | ||
| | select(((.name | ascii_downcase) == $name) or any(.aliases[]?; ascii_downcase == $name)) | ||
| | [.name,.email,.family,((.providers // []) | join(","))] | @tsv | ||
| ' "$manifest" | head -n 1 |
There was a problem hiding this comment.
In Wave/devBench containers I checked docker-compose.mounts.yml and scripts/wave-container-shell.sh: they bind-mount ~/.pi-profiles but not ~/.config/workbenches/pi-profiles.json. This launcher only reads $manifest for profile lookup, so ppi team001, ppi status team001, and ppi list fail inside those containers with a missing manifest even though setup-pi-profiles.sh wrote .profile.json under the mounted profile tree. Please fall back to $base/profiles/*/.profile.json as the Claude/Codex launchers do, or mount the manifest as well.
Useful? React with 👍 / 👎.
| pi | ||
| read -r -p "Pi profile name or alias: " pi_profile | ||
| echo -e "${YELLOW}Run /login, choose a provider, verify the matching identity, then exit Pi.${NC}" | ||
| ppi login "$pi_profile" |
There was a problem hiding this comment.
When a user selects Pi CLI in interactive setup without already having run the AI profile setup, this branch installs only the npm pi binary and then calls ppi, but this flow never creates the ppi symlink or Pi profile manifest. Accepting the prompt therefore fails instead of launching the newly installed CLI; provision ppi before this prompt, detect command -v ppi, or fall back to standard pi login when no profiles are configured.
Useful? React with 👍 / 👎.
| local_file="${PI_PROFILES_HOME:-$HOME/.pi-profiles}/profiles/$profile/agent/auth.json" | ||
| credential_ref="ai/secrets/pi/$profile.auth.sops.yaml" |
There was a problem hiding this comment.
When ai/source.json contains an authorized profile name with / or .., this interpolates it directly into both the local credential path and repository secret path, so backup/restore can read or write outside $PI_PROFILES_HOME/profiles or ai/secrets/pi. Restrict Pi profile names to a safe basename, or resolve and verify these paths before using them.
Useful? React with 👍 / 👎.
| if [[ "$check_only" == true ]]; then | ||
| docker exec --user "$container_user" --workdir "$workdir" "$container" "$shell_path" -lc \ | ||
| 'printf "%s\n" "wave-container-shell-ok"; whoami; pwd; command -v claude-profile; command -v pclaude; command -v codex-profile; command -v pcodex; command -v pgemini; command -v pgrok; command -v pglm; test -d "$HOME/.claude-profiles"; test -d "$HOME/.chatgpt-profiles"; test -d "$HOME/.gemini-profiles"; test -d "$HOME/.grok-profiles"; test -d "$HOME/.glm-profiles"' | ||
| 'printf "%s\n" "wave-container-shell-ok"; whoami; pwd; command -v claude-profile; command -v pclaude; command -v codex-profile; command -v pcodex; command -v ppi; command -v pgemini; command -v pgrok; command -v pglm; test -d "$HOME/.claude-profiles"; test -d "$HOME/.chatgpt-profiles"; test -d "$HOME/.pi-profiles"; test -d "$HOME/.gemini-profiles"; test -d "$HOME/.grok-profiles"; test -d "$HOME/.glm-profiles"' |
There was a problem hiding this comment.
For an existing Wave-created container from before this change, container_missing_required_mounts still omits /home/${container_user}/.pi-profiles, so the script does not call recreate_with_compose; this newly added check can fail and normal shells will not see host Pi profile credentials. Add the Pi profile directory to the required mount list so existing containers pick up the new mount.
Useful? React with 👍 / 👎.
| existing = profiles.get(raw["name"]) | ||
| if existing is None: | ||
| profiles[raw["name"]] = {"name": raw["name"], **normalized, "providers": [provider]} |
There was a problem hiding this comment.
When Pi profiles are composed from separate provider manifests, two different profile names can legally share an alias because those aliases only need to be unique within each provider. This code only keys by raw profile name, so the emitted pi-profiles.json can contain profiles like alpha and zeta both aliasing team; pi-profile then resolves ppi team by the first matching row, causing login/logout/run to target whichever profile sorts first. Please track case-insensitive names and aliases across the unified Pi manifest and reject collisions.
Useful? React with 👍 / 👎.
| fi | ||
| fi | ||
|
|
||
| [ "$windows_pi" = true ] && return 0 |
There was a problem hiding this comment.
In WSL, if a Windows pi.cmd already exists but the WSL/Linux npm install above fails or no WSL npm is available, this fallback still returns success with no Linux pi on PATH. The new ppi launcher and devBench containers invoke the Linux CLI, not the Windows shim, so setup can report Pi usable while isolated profile logins still fail; return an error or explicitly report a Windows-only install when WSL Pi could not be installed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
scripts/setup-ai-profiles.sh:54
- The Pi profile composition block only runs when all five provider manifests exist because
ls file1 file2 ...returns non-zero if any file is missing. This prevents generating pi-profiles.json when only a subset of provider manifests is present (which should be supported).
if ls "$config_dir"/{claude,openai,gemini,grok,glm}-profiles.json >/dev/null 2>&1; then
pi_profile_roots=(
--profile-root "claude=${CLAUDE_PROFILES_HOME:-$HOME/.claude-profiles}/profiles"
--profile-root "openai=${CODEX_PROFILES_HOME:-$HOME/.chatgpt-profiles}/profiles"
--profile-root "gemini=${GEMINI_PROFILES_HOME:-$HOME/.gemini-profiles}/profiles"
--profile-root "grok=${GROK_PROFILES_HOME:-$HOME/.grok-profiles}/profiles"
--profile-root "glm=${GLM_PROFILES_HOME:-$HOME/.glm-profiles}/profiles"
)
python3 "$repo_dir/scripts/compose-pi-profiles.py" \
--config-dir "$config_dir" \
"${pi_profile_roots[@]}" \
--output "$config_dir/pi-profiles.json"
fi
scripts/setup-pi-profiles.sh:71
- This script creates ~/.local/bin/ppi and ~/.local/bin/pi-profile symlinks pointing to
$repo_dir/scripts/pi-profile, but that file does not exist in the repository. The resulting launchers will be dangling symlinks.
ln -sfn "$repo_dir/scripts/pi-profile" "$HOME/.local/bin/pi-profile"
ln -sfn "$repo_dir/scripts/pi-profile" "$HOME/.local/bin/ppi"
scripts/interactive-setup.sh:2401
- The credential-setup flow unconditionally runs
ppi login ..., butitems_needing_creds+=("pi")is added based on the presence ofpi(or ~/.npm-global/bin/pi), notppi. On a fresh install wherepiexists butppihasn’t been installed yet, this will fail withppi: command not found.
if [[ ! $launch_pi =~ ^[Nn] ]]; then
read -r -p "Pi profile name or alias: " pi_profile
echo -e "${YELLOW}Run /login, choose a provider, verify the matching identity, then exit Pi.${NC}"
ppi login "$pi_profile"
scripts/setup-windows-tools.sh:162
wsl_npmis assigned withoutlocal, which leaks a global variable out of this function and can accidentally affect other logic in this script.
echo " Installing Pi Terminal in WSL/Linux for ppi and devBench use..."
wsl_npm="$(command -v npm)"
[[ -x /usr/bin/npm ]] && wsl_npm=/usr/bin/npm
if "$wsl_npm" install -g --ignore-scripts "$PI_NPM_PACKAGE"; then
| [[ -n "$profile" ]] || { echo "--profile is required" >&2; exit 2; } | ||
| [[ -r "$identity_file" ]] || { echo "Recovery identity is not readable" >&2; exit 1; } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7af38966c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| jq -r --arg name "$normalized" ' | ||
| .profiles[] | ||
| | select(((.name | ascii_downcase) == $name) or any(.aliases[]?; ascii_downcase == $name)) | ||
| | [.name,.email,.family,((.providers // []) | join(","))] | @tsv | ||
| ' "$manifest" | head -n 1 |
There was a problem hiding this comment.
In a devBench/Wave container, only ~/.pi-profiles is mounted (checked docker-compose.mounts.yml:45-46 and scripts/wave-container-shell.sh:177-178); ~/.config/workbenches/pi-profiles.json is not. Because ppi always queries $manifest here and setup-pi-profiles.sh stores the same metadata in each profile’s .profile.json, ppi team001, ppi login team001, and alias resolution fail with jq: Could not open file ... inside the advertised container flow unless the user manually mounts the config directory. Please fall back to the mounted .pi-profiles/profiles/*/.profile.json metadata like the other profile launchers.
Useful? React with 👍 / 👎.
| pi | ||
| read -r -p "Pi profile name or alias: " pi_profile | ||
| echo -e "${YELLOW}Run /login, choose a provider, verify the matching identity, then exit Pi.${NC}" | ||
| ppi login "$pi_profile" |
There was a problem hiding this comment.
In the fresh Pi CLI path, line 1908 adds pi to items_needing_creds as soon as the pi binary exists, but no code in that install branch creates ppi—that only happens when setup-pi-profiles.sh runs with a manifest. On a workstation where the user selected only Pi Terminal or has no profile manifest yet, answering yes reaches this line and aborts with ppi: command not found (or a missing manifest) instead of launching the previously working pi login flow. Please guard this with command -v ppi/profile setup or fall back to pi.
Useful? React with 👍 / 👎.
| fi | ||
| fi | ||
|
|
||
| [ "$windows_pi" = true ] && return 0 |
There was a problem hiding this comment.
When Windows Pi is already installed but WSL/Linux pi is missing, this fallback returns success even after the WSL npm install path above fails or is unavailable. This change makes WSL/Linux Pi the runtime used by ppi and devBench, so setup-windows-tools.sh pi_terminal can report success while leaving the actual ppi runtime unusable in WSL; please return failure unless command -v pi succeeds in WSL or the WSL install completed.
Useful? React with 👍 / 👎.
| docker exec --user "$container_user" --workdir "$workdir" "$container" "$shell_path" -lc \ | ||
| 'printf "%s\n" "wave-container-shell-ok"; whoami; pwd; command -v claude-profile; command -v pclaude; command -v codex-profile; command -v pcodex; command -v pgemini; command -v pgrok; command -v pglm; test -d "$HOME/.claude-profiles"; test -d "$HOME/.chatgpt-profiles"; test -d "$HOME/.gemini-profiles"; test -d "$HOME/.grok-profiles"; test -d "$HOME/.glm-profiles"' | ||
| 'printf "%s\n" "wave-container-shell-ok"; whoami; pwd; command -v claude-profile; command -v pclaude; command -v codex-profile; command -v pcodex; command -v ppi; command -v pgemini; command -v pgrok; command -v pglm; test -d "$HOME/.claude-profiles"; test -d "$HOME/.chatgpt-profiles"; test -d "$HOME/.pi-profiles"; test -d "$HOME/.gemini-profiles"; test -d "$HOME/.grok-profiles"; test -d "$HOME/.glm-profiles"' |
There was a problem hiding this comment.
The new --check assertion requires $HOME/.pi-profiles, but the stale-container detection list was not updated to require that mount before reusing an existing devcontainer. For users who already created a Wave container before this change, wave-container-shell.sh --check now fails here and normal launches still reuse a container without the new Pi profile mount; add .pi-profiles to the required mounts that trigger recreation.
Useful? React with 👍 / 👎.
| local_file="${PI_PROFILES_HOME:-$HOME/.pi-profiles}/profiles/$profile/agent/auth.json" | ||
| credential_ref="ai/secrets/pi/$profile.auth.sops.yaml" | ||
| secret_file="$repo_dir/$credential_ref" |
There was a problem hiding this comment.
profile is accepted from any matching entry in ai/source.json, but this code interpolates it directly into both the local auth path and the repository secret path. A malformed registry profile name containing / or .. (the source contract only requires a non-empty string) can make backup/restore read or write outside ~/.pi-profiles/profiles and ai/secrets/pi; validate the profile slug or use a declared, normalized credentialRef before constructing these paths.
Useful? React with 👍 / 👎.
| [[ "$(stat -c '%a' "$identity_file" 2>/dev/null || stat -f '%Lp' "$identity_file")" == 600 ]] || { echo "Recovery identity must have mode 0600" >&2; exit 1; } | ||
| command -v jq >/dev/null && command -v sops >/dev/null || { echo "jq and sops are required" >&2; exit 1; } | ||
|
|
||
| jq -e --arg profile "$profile" '[.profiles[]?[]? | select(.name == $profile)] | length > 0' "$repo_dir/ai/source.json" >/dev/null || { |
There was a problem hiding this comment.
This guard does not actually reject unknown profiles because jq -e exits successfully for the numeric value 0; with a missing profile the expression still returns 0, so backup/restore proceeds and can create ai/secrets/pi/NAME.auth.sops.yaml for a profile that is not in the registry. Pipe the count to > 0 (or use any(...)) so the check fails before touching local or escrowed credentials.
Useful? React with 👍 / 👎.
| existing = profiles.get(raw["name"]) | ||
| if existing is None: | ||
| profiles[raw["name"]] = {"name": raw["name"], **normalized, "providers": [provider]} |
There was a problem hiding this comment.
The composed manifest only keys profiles by the exact name here and never checks case-insensitive collisions between names and aliases. Since ppi resolves either a name or any alias and then takes the first match, cases like one profile aliasing another profile's name, or two providers contributing the same alias for different profile names, will route ppi login ALIAS to the wrong isolated Pi home; reject those collisions during composition.
Useful? React with 👍 / 👎.
| if jq -e 'index("claude") != null' <<<"$providers" >/dev/null; then | ||
| settings_tmp="$(mktemp "$agent_dir/.settings.XXXXXX.tmp")" | ||
| jq ' | ||
| .packages = (((.packages // []) | map(select(. != "npm:pi-claude-cli" and . != "npm:@ramarivera/pi-claude-cli" and . != "npm:@ramarivera/pi-claude-cli@0.3.1"))) + ["npm:@ramarivera/pi-claude-cli@0.3.1"]) | ||
| | .defaultProvider = "pi-claude-cli" |
There was a problem hiding this comment.
If a profile previously had the claude provider, this block writes pi-claude-cli as the default provider, but when a later manifest removes claude the setup simply skips the block and leaves those old settings in place. In that scenario ppi PROFILE still defaults to the Claude proxy even though ppi no longer exports CLAUDE_CONFIG_DIR for the profile, which can break Pi startup or fall back to an unintended Claude account; remove or recompute the Claude package/defaults when providers no longer contains claude.
Useful? React with 👍 / 👎.
Reviewer's GuideAdds isolated, per-identity Pi Coding Agent profiles driven by canonical provider manifests, with a new Sequence diagram for ppi login and Pi credential escrowsequenceDiagram
actor User
participant ppi
participant pi
participant pi_credential_escrow
participant Tenant_Credentials_repo
User->>ppi: ppi login PROFILE
ppi->>ppi: set PI_CODING_AGENT_DIR
ppi->>pi: start Pi terminal
User->>pi: /login
pi-->>User: provider auth flow
User-->>pi: complete authentication
User->>pi_credential_escrow: pi-credential-escrow backup --repo PATH --profile PROFILE --identity-file tenant-recovery.agekey
pi_credential_escrow->>Tenant_Credentials_repo: write ai/secrets/pi/PROFILE.auth.sops.yaml
pi_credential_escrow->>Tenant_Credentials_repo: read back and verify auth hash
pi_credential_escrow-->>User: backup verified
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="apps/credential-manager/test_pi_profiles.py" line_range="9" />
<code_context>
+class PiProfilesTest(unittest.TestCase):
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests verifying that standard `~/.pi/agent` credentials are preserved and never implicitly copied
The PR’s main requirement—preserving `~/.pi/agent` without implicit copying—is not covered by tests. Please add a test that sets a fake `HOME` with an existing `~/.pi/agent`, runs `setup-pi-profiles.sh` and `ppi` for a profile, and then verifies that:
- The profile’s agent path under `.pi-profiles/profiles/.../agent` differs from `~/.pi/agent`.
- No files from `~/.pi/agent` (e.g., `auth.json`) appear in the profile directory unless the test creates them explicitly.
This ensures the behavior promised in the PR description is actually enforced by tests.
```suggestion
REPO = pathlib.Path(__file__).parents[2]
def _run_cmd(args, cwd=None, env=None):
completed = subprocess.run(
args,
cwd=cwd,
env=env,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
check=True,
)
return completed
class PiProfilesAgentPreservationTest(unittest.TestCase):
def test_standard_agent_is_not_copied_into_profile(self):
with tempfile.TemporaryDirectory(dir="/tmp") as temporary:
root = pathlib.Path(temporary)
home = root / "home"
home.mkdir(parents=True)
# Create standard ~/.pi/agent with a fake auth.json
pi_dir = home / ".pi"
agent_dir = pi_dir / "agent"
agent_dir.mkdir(parents=True)
(agent_dir / "auth.json").write_text('{"token": "standard-agent-token"}')
# Set up fake HOME
env = os.environ.copy()
env["HOME"] = str(home)
# Run setup-pi-profiles.sh to initialize profiles support
setup_script = REPO / "apps" / "credential-manager" / "setup-pi-profiles.sh"
_run_cmd(["bash", str(setup_script)], cwd=str(REPO), env=env)
# Run `ppi` once to create a profile and its agent dir
profile_name = "test-profile"
ppi_bin = REPO / "apps" / "credential-manager" / "ppi"
_run_cmd(
[str(ppi_bin), "profiles", "use", profile_name],
cwd=str(REPO),
env=env,
)
# Resolve paths
profile_root = home / ".pi-profiles" / "profiles" / profile_name
profile_agent_dir = profile_root / "agent"
# Sanity: profile agent directory should exist
self.assertTrue(
profile_agent_dir.exists(),
"profile agent directory should be created",
)
# 1. The profile’s agent path must differ from the standard ~/.pi/agent path
self.assertNotEqual(
profile_agent_dir.resolve(),
agent_dir.resolve(),
"profile agent directory must not be the same as the standard ~/.pi/agent directory",
)
# 2. No files from ~/.pi/agent (e.g., auth.json) should appear in the profile directory
self.assertFalse(
(profile_agent_dir / "auth.json").exists(),
"auth.json from ~/.pi/agent must not be implicitly copied into the profile agent directory",
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| import unittest | ||
|
|
||
|
|
||
| REPO = pathlib.Path(__file__).parents[2] |
There was a problem hiding this comment.
suggestion (testing): Add tests verifying that standard ~/.pi/agent credentials are preserved and never implicitly copied
The PR’s main requirement—preserving ~/.pi/agent without implicit copying—is not covered by tests. Please add a test that sets a fake HOME with an existing ~/.pi/agent, runs setup-pi-profiles.sh and ppi for a profile, and then verifies that:
- The profile’s agent path under
.pi-profiles/profiles/.../agentdiffers from~/.pi/agent. - No files from
~/.pi/agent(e.g.,auth.json) appear in the profile directory unless the test creates them explicitly.
This ensures the behavior promised in the PR description is actually enforced by tests.
| REPO = pathlib.Path(__file__).parents[2] | |
| REPO = pathlib.Path(__file__).parents[2] | |
| def _run_cmd(args, cwd=None, env=None): | |
| completed = subprocess.run( | |
| args, | |
| cwd=cwd, | |
| env=env, | |
| stdout=subprocess.PIPE, | |
| stderr=subprocess.PIPE, | |
| text=True, | |
| check=True, | |
| ) | |
| return completed | |
| class PiProfilesAgentPreservationTest(unittest.TestCase): | |
| def test_standard_agent_is_not_copied_into_profile(self): | |
| with tempfile.TemporaryDirectory(dir="/tmp") as temporary: | |
| root = pathlib.Path(temporary) | |
| home = root / "home" | |
| home.mkdir(parents=True) | |
| # Create standard ~/.pi/agent with a fake auth.json | |
| pi_dir = home / ".pi" | |
| agent_dir = pi_dir / "agent" | |
| agent_dir.mkdir(parents=True) | |
| (agent_dir / "auth.json").write_text('{"token": "standard-agent-token"}') | |
| # Set up fake HOME | |
| env = os.environ.copy() | |
| env["HOME"] = str(home) | |
| # Run setup-pi-profiles.sh to initialize profiles support | |
| setup_script = REPO / "apps" / "credential-manager" / "setup-pi-profiles.sh" | |
| _run_cmd(["bash", str(setup_script)], cwd=str(REPO), env=env) | |
| # Run `ppi` once to create a profile and its agent dir | |
| profile_name = "test-profile" | |
| ppi_bin = REPO / "apps" / "credential-manager" / "ppi" | |
| _run_cmd( | |
| [str(ppi_bin), "profiles", "use", profile_name], | |
| cwd=str(REPO), | |
| env=env, | |
| ) | |
| # Resolve paths | |
| profile_root = home / ".pi-profiles" / "profiles" / profile_name | |
| profile_agent_dir = profile_root / "agent" | |
| # Sanity: profile agent directory should exist | |
| self.assertTrue( | |
| profile_agent_dir.exists(), | |
| "profile agent directory should be created", | |
| ) | |
| # 1. The profile’s agent path must differ from the standard ~/.pi/agent path | |
| self.assertNotEqual( | |
| profile_agent_dir.resolve(), | |
| agent_dir.resolve(), | |
| "profile agent directory must not be the same as the standard ~/.pi/agent directory", | |
| ) | |
| # 2. No files from ~/.pi/agent (e.g., auth.json) should appear in the profile directory | |
| self.assertFalse( | |
| (profile_agent_dir / "auth.json").exists(), | |
| "auth.json from ~/.pi/agent must not be implicitly copied into the profile agent directory", | |
| ) |
Codex profiles now link sessions, archived_sessions, history.jsonl, and session_index.jsonl to a per-family state directory so another login in the same trust family can resume the same conversations, while auth.json, config, caches, and SQLite state stay profile-local. Adopting an existing profile merges its portable state into the family without overwriting rollouts and retains a .pre-shared-state recovery copy. Add setup-multi-cli-codex-profiles.ps1 to sync registry-owned Multi-CLI profiles with the Windows Codex Desktop store for company families. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document that each personal login should use a unique family when its history must stay separate from other profiles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Onboarding now asks for the company login email first and defaults the company name to the email domain's first label (you@acme.com -> acme); the GitHub org in turn defaults to the company name. Both remain editable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document an optional shared Keycloak identity server plus a Python broker that gates the existing SOPS registries: Keycloak proves identity, the grant files stay authoritative for authorization, SOPS stays the at-rest store, and the broker holds one age identity per tenant so no decryption key reaches a workstation or agent. Cross-link it from the credential ownership doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
brettheap
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
scripts/pi-credential-escrow:48
json_hashrelies onsha256sum, which is not available by default on macOS (the repo already uses asha256sum/shasum -a 256fallback elsewhere). This makesbackupfail even whenjqandsopsare installed.
json_hash() { jq -S -c . "$1" | sha256sum | awk '{print $1}'; }
| $metadata = [ordered]@{ | ||
| name = $profile.name | ||
| email = $profile.email | ||
| family = $profile.family | ||
| aliases = @($profile.aliases) | ||
| managedBy = "workBenches" | ||
| } | ConvertTo-Json -Depth 4 |
| pi_cli) | ||
| if command -v pi &>/dev/null; then | ||
| [[ -f "$HOME/.pi/agent/auth.json" ]] && echo "installed" || echo "needs creds" | ||
| if [[ -f "$HOME/.pi/agent/auth.json" ]] || find "$HOME/.pi-profiles/profiles" -mindepth 3 -maxdepth 3 -name auth.json -print -quit 2>/dev/null | grep -q .; then | ||
| echo "installed" | ||
| else | ||
| echo "needs creds" | ||
| fi | ||
| else | ||
| echo "not installed" |
Sequence delivery so the riskiest assumptions (porting the escrow logic to Python, and the end-to-end authenticate -> authorize -> decrypt -> materialize loop) are proven first. Phase 0 builds the broker core with no Keycloak; phase 1 is the MVP vertical slice (real Keycloak, one tenant, pclaude); phases 2-4 add provider/tenant breadth, unattended agents, and broader cred kinds; phase 5 is hardening. Resolve the lease-semantics and federated-cloud open questions (opaque TTLs and no federation in v1, respectively). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
scripts/setup-ai-profiles.sh:46
- The
ls "$config_dir"/{claude,openai,gemini,grok,glm}-profiles.jsoncheck only succeeds when all five manifests exist. That prevents Pi profile composition when a user has only a subset of providers configured (even though the script already applies each manifest independently above). Switch this to an explicit “any manifest exists” check socompose-pi-profiles.pyruns whenever at least one provider manifest is present.
if ls "$config_dir"/{claude,openai,gemini,grok,glm}-profiles.json >/dev/null 2>&1; then
pi_profile_roots=(
--profile-root "claude=${CLAUDE_PROFILES_HOME:-$HOME/.claude-profiles}/profiles"
--profile-root "openai=${CODEX_PROFILES_HOME:-$HOME/.chatgpt-profiles}/profiles"
--profile-root "gemini=${GEMINI_PROFILES_HOME:-$HOME/.gemini-profiles}/profiles"
scripts/setup-pi-profiles.sh:71
- These symlinks point to
$repo_dir/scripts/pi-profile, but the repository only addsbase-image/files/pi-profilein this PR. As-is,setup-pi-profiles.shwill create broken launchers (pi-profile/ppi) on the host and the subsequentppi ...commands will fail.
ln -sfn "$repo_dir/scripts/pi-profile" "$HOME/.local/bin/pi-profile"
ln -sfn "$repo_dir/scripts/pi-profile" "$HOME/.local/bin/ppi"
| aliases = @($profile.aliases) | ||
| managedBy = "workBenches" | ||
| } | ConvertTo-Json -Depth 4 | ||
| Set-Content -LiteralPath (Join-Path $profileDir ".profile.json") -Value $metadata -Encoding utf8 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
scripts/setup-pi-profiles.sh:71
setup-pi-profiles.shinstalls theppi/pi-profilelaunchers by symlinking to$repo_dir/scripts/pi-profile, but that file doesn't exist in the repo (onlybase-image/files/pi-profileis present). This leaves users with a broken launcher after running setup.
ln -sfn "$repo_dir/scripts/pi-profile" "$HOME/.local/bin/pi-profile"
ln -sfn "$repo_dir/scripts/pi-profile" "$HOME/.local/bin/ppi"
scripts/pi-credential-escrow:48
json_hashpipes intosha256sum, but this script otherwise tries to be cross-platform (e.g.,stat -ffallback).sha256sumis not available by default on macOS, sobackup/restorecan fail even whenjq/sopsare installed.
json_hash() { jq -S -c . "$1" | sha256sum | awk '{print $1}'; }
scripts/interactive-setup.sh:275
pi_clistatus only checkscommand -v pi, but other parts of the script treat~/.npm-global/bin/pias an installed location. This can incorrectly report "not installed" after a successful npm install when the user's PATH doesn't include the npm-global bin dir yet.
if command -v pi &>/dev/null; then
| # Zsh history persistence. Mount the named volume as a directory; Docker named | ||
| # volumes are directories and cannot safely target the ~/.zsh_history file. | ||
| - ${BENCH_NAME:-bench}-zshhistory:/home/${USER}/.workbenches-history |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (3)
base-image/files/claude-profile:136
- Same issue in the
listpath:find "$base/profiles" ...errors when the directory is missing and will abortclaude-profile listunderset -eeven though listing should be a safe/no-op before setup.
done < <(find "$base/profiles" -mindepth 2 -type f -name .profile.json -print0)
base-image/files/codex-profile:79
- Same
find/set -eissue in thelistpath: this will abortcodex-profile listif$base/profileshasn’t been created yet.
done < <(find "$base/profiles" -mindepth 2 -type f -name .profile.json -print0)
scripts/setup-ai-profiles.sh:54
if ls "$config_dir"/{claude,openai,gemini,grok,glm}-profiles.json ...only succeeds when all five manifests exist.compose-pi-profiles.pyalready tolerates missing provider manifests, so this gate should trigger when any provider manifest exists; otherwise Pi profiles won’t be composed in common partial-provider setups.
| fi | ||
| fi | ||
| done | ||
| done < <(find "$base/profiles" -mindepth 2 -type f -name .profile.json -print0) |
| fi | ||
| fi | ||
| done | ||
| done < <(find "$base/profiles" -mindepth 2 -type f -name .profile.json -print0) |
| list) | ||
| jq -r '.profiles[] | [.name,.family,.email,((.aliases // [])|join(",")),((.providers // [])|join(","))] | @tsv' "$manifest" | ||
| ;; |
| fallback_row="$row" | ||
| fi | ||
| fi | ||
| done < <(find "$base/profiles" -mindepth 2 -type f -name .profile.json -print0) |
| claude_profile_depth="$candidate_depth" | ||
| fi | ||
| fi | ||
| done < <(find "$claude_profiles_root" -mindepth 1 -type d -name "$profile" -print0) |
| selected_depth="$depth" | ||
| fi | ||
| fi | ||
| done < <(find "$profile_root" -mindepth 1 -type d -name "$name" -print0) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
scripts/setup-ai-profiles.sh:42
setup-ai-profiles.shonly composes Pi profiles when all five provider manifests exist becauselsreturns non-zero if any path is missing. This means Pi profile generation can be silently skipped for partial setups (e.g., only claude/openai manifests present). Use a glob-aware check that succeeds when any of those manifests exists.
| log_info "Installing Herdr terminal workspace manager..." | ||
| herdr_installer="$(mktemp)" | ||
| if run_with_timeout "$RELEASE_DOWNLOAD_TIMEOUT" "Herdr installer download" \ | ||
| curl -fsSL --retry 3 --connect-timeout 10 --max-time 60 \ | ||
| -o "$herdr_installer" "$HERDR_INSTALL_URL"; then | ||
| if ! run_with_timeout "$RELEASE_DOWNLOAD_TIMEOUT" "Herdr install" \ | ||
| env HERDR_INSTALL_DIR=/usr/local/bin sh "$herdr_installer"; then | ||
| log_error "Herdr installation failed (continuing)" | ||
| fi | ||
| else | ||
| log_error "Herdr installer download failed (continuing)" | ||
| fi | ||
| rm -f "$herdr_installer" |
| local_file="${PI_PROFILES_HOME:-$HOME/.pi-profiles}/profiles/$profile/agent/auth.json" | ||
| credential_ref="ai/secrets/pi/$profile.auth.sops.yaml" | ||
| secret_file="$repo_dir/$credential_ref" |
| FAMILIES = ("opensoft", "medx", "personal") | ||
|
|
||
|
|
||
| def family_for(profile: dict) -> str: | ||
| email = str(profile.get("email", "")).lower() |
Summary
Six commits on top of
mainspanning the multi-account profile work and a new credential-broker design doc. Fast-forward overorigin/main(no divergence).Changes
sessions,archived_sessions,history.jsonl, andsession_index.jsonllink to a per-family state dir so another login in the same trust family can resume the same conversations; auth/config/caches/SQLite stay profile-local. Addssetup-multi-cli-codex-profiles.ps1to sync registry-owned Multi-CLI profiles with the Windows Codex Desktop store.you@acme.com→acme), GitHub org defaults to the company name; both editable.docs/keycloak-credential-broker.md) — an optional shared Keycloak identity server plus a Python broker that gates the existing SOPS registries: Keycloak proves identity, grant files stay authoritative for authorization, SOPS stays the at-rest store, and the broker holds one age identity per tenant so no decryption key reaches a workstation or agent. Cross-linked fromai-credential-ownership.md.Notes
🤖 Generated with Claude Code