feat!(action): simplify v2 flow to CLI incremental/full contract - #69
Conversation
![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: 363744fac4
ℹ️ 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".
| set -euo pipefail | ||
| AUTH_FILE="${RUNNER_TEMP}/openrouter-auth.json" | ||
| trap 'rm -f "$AUTH_FILE"' EXIT | ||
|
|
||
| _strip() { printf '%s' "$1" | tr -d '[:space:]' | sed -e 's/^"//;s/"$//' -e "s/^'//;s/'\$//"; } | ||
| # Cache keys reject some characters (model slugs carry '/'); sanitize for them. | ||
| _safe() { printf '%s' "$1" | tr -c 'A-Za-z0-9._-' '_'; } | ||
|
|
||
| KEY="$(_strip "$RAW_KEY")" | ||
| LICENSE="$(_strip "$RAW_LICENSE")" | ||
| PROXY_URL="$(_strip "$RAW_PROXY_URL")" | ||
| PROXY_URL="${PROXY_URL%/}" # no trailing slash; engine appends /chat/completions | ||
| AGENT_MODEL="$(_strip "$RAW_AGENT_MODEL")" | ||
| PARSING_MODEL="$(_strip "$RAW_PARSING_MODEL")" | ||
| umask 077 | ||
|
|
||
| # Three credential modes, in precedence order: | ||
| # 1. BYO key set -> talk to the provider directly (current behavior) | ||
| # 2. license_key set -> hosted proxy, bearer = the license | ||
| # 3. neither (zero-config) -> hosted proxy, bearer = a GitHub OIDC JWT | ||
| # Modes 2 & 3 force provider=openrouter and point the engine's | ||
| # OPENROUTER_BASE_URL at the proxy (written to cb-base-url). The proxy | ||
| # swaps in the real key, so no provider preflight here. | ||
| if [ -n "$KEY" ]; then | ||
| MODE="byokey" | ||
| elif [ -n "$LICENSE" ]; then | ||
| MODE="license" | ||
| else | ||
| MODE="oidc" | ||
| fi | ||
| echo "mode=$MODE" >> "$GITHUB_OUTPUT" | ||
| echo "Credential mode: $MODE" | ||
|
|
||
| # ── Hosted modes (license / oidc): provider is always OpenRouter via proxy ── | ||
| # The hosted tiers run on CodeBoarding's OpenRouter account. A loopback relay | ||
| # mints a fresh GitHub OIDC JWT for every engine request, then forwards it to | ||
| # the hosted proxy. This matters because an analysis can outlive a single OIDC | ||
| # JWT. To use a DIFFERENT provider, set llm_api_key (BYO-key mode, below). | ||
| if [ "$MODE" != "byokey" ]; then | ||
| if [ -z "$PROXY_URL" ]; then | ||
| echo "::error::proxy_url is empty but no llm_api_key was provided. Set llm_api_key, or restore proxy_url." | ||
| exit 1 | ||
| fi | ||
| # Warn if the user asked for a non-OpenRouter provider but gave no key: | ||
| # the hosted tier can only use OpenRouter, so llm_provider is ignored here. | ||
| PROVIDER_NORM="$(printf '%s' "$RAW_PROVIDER" | tr '[:upper:]' '[:lower:]' | tr -cd 'a-z0-9_')" | ||
| if [ -n "$PROVIDER_NORM" ] && [ "$PROVIDER_NORM" != "openrouter" ]; then | ||
| echo "::warning::llm_provider='$PROVIDER_NORM' is ignored on the free/license hosted tier (OpenRouter only). To use $PROVIDER_NORM, pass its key via llm_api_key." | ||
| fi | ||
| PROVIDER_ENV="OPENROUTER_API_KEY" | ||
| AGENT_MODEL="${AGENT_MODEL:-google/gemini-3-flash-preview}" | ||
| PARSING_MODEL="${PARSING_MODEL:-google/gemini-3.1-flash-lite-preview}" | ||
|
|
||
| # ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN are injected into the runner process | ||
| # env (NOT the `env` context) only when the job grants `id-token: write`. | ||
| # Pass them to the local relay through its inherited environment; it requests | ||
| # a new JWT per forwarded request instead of freezing one into the engine. | ||
| if [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ]; then | ||
| echo "::error::No GitHub OIDC token available. Add \`permissions: id-token: write\` to the job (the hosted tier — free and license — needs the OIDC token to identify your repository; an llm_api_key avoids the proxy entirely)." | ||
| exit 1 | ||
| fi | ||
| RELAY_READY="${RUNNER_TEMP}/cb-oidc-relay-port" | ||
| RELAY_PID_FILE="${RUNNER_TEMP}/cb-oidc-relay.pid" | ||
| RELAY_LICENSE_FILE="${RUNNER_TEMP}/cb-oidc-relay-license" | ||
| RELAY_LOG="${RUNNER_TEMP}/cb-oidc-relay.log" | ||
| rm -f "$RELAY_READY" "$RELAY_PID_FILE" "$RELAY_LICENSE_FILE" "$RELAY_LOG" | ||
| relay_args=(--upstream-base-url "$PROXY_URL" --ready-file "$RELAY_READY") | ||
| if [ "$MODE" = "license" ]; then | ||
| echo "::add-mask::$LICENSE" | ||
| printf '%s' "$LICENSE" > "$RELAY_LICENSE_FILE" | ||
| relay_args+=(--license-file "$RELAY_LICENSE_FILE") | ||
| fi | ||
| python3 "$ACTION_PATH/scripts/oidc_relay.py" "${relay_args[@]}" >"$RELAY_LOG" 2>&1 & | ||
| RELAY_PID=$! | ||
| printf '%s' "$RELAY_PID" > "$RELAY_PID_FILE" | ||
| for _ in $(seq 1 50); do | ||
| [ -s "$RELAY_READY" ] && break | ||
| kill -0 "$RELAY_PID" 2>/dev/null || break | ||
| sleep 0.1 | ||
| done | ||
| if [ ! -s "$RELAY_READY" ]; then | ||
| echo "::error::Failed to start the GitHub OIDC relay." | ||
| sed -n '1,20p' "$RELAY_LOG" || true | ||
| exit 1 | ||
| fi | ||
| RELAY_PORT="$(cat "$RELAY_READY")" | ||
| case "$RELAY_PORT" in *[!0-9]*|'') echo "::error::OIDC relay returned an invalid port."; exit 1 ;; esac | ||
| printf '%s' 'github-actions-oidc-relay' > "${RUNNER_TEMP}/cb-llm-key" | ||
| printf '%s' "$PROVIDER_ENV" > "${RUNNER_TEMP}/cb-provider-env" | ||
| printf '%s' "http://127.0.0.1:${RELAY_PORT}" > "${RUNNER_TEMP}/cb-base-url" | ||
| printf '%s' "$AGENT_MODEL" > "${RUNNER_TEMP}/cb-agent-model" | ||
| printf '%s' "$PARSING_MODEL" > "${RUNNER_TEMP}/cb-parsing-model" | ||
| if [ "$MODE" = "license" ]; then | ||
| echo "Using CodeBoarding license via a GitHub OIDC relay (token refreshed per request)." | ||
| else | ||
| echo "Using the free hosted tier via a GitHub OIDC relay (token refreshed per request)." | ||
| fi | ||
| exit 0 | ||
| if [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ]; then | ||
| echo "::error::Missing OIDC token. Add permissions: id-token: write." && exit 1 |
There was a problem hiding this comment.
The relay now unconditionally fails when the job lacks id-token: write, but both checked repository workflows grant no such permission and instead pass the now-removed llm_api_key input (.github/workflows/codeboarding.yml:123-126 and codeboarding-sync.yml:145-159). Consequently, every dogfood review and sync run fails here before analysis, and existing BYO-key consumers regress similarly; either retain the direct-key path or update the workflows and compatibility contract to mint OIDC tokens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
yea I am unsure how does the llm api key provision work now.
There was a problem hiding this comment.
Addressed. Direct/BYO keys are supported again and are mapped to the selected Core provider without requiring OIDC. Keyless OpenRouter mode alone selects hosted OIDC usage, and the README now documents both paths.
| ```mermaid | ||
| $(cat "${{ steps.review_render.outputs.diagram_md }}") | ||
| ``` |
There was a problem hiding this comment.
Action inputs under with.message are plain YAML strings and are not evaluated by a shell, so successful reviews post the literal text $(cat "/tmp/.../diagram.md") instead of a diagram. The generated file already contains its own Mermaid fences, so its contents should be placed into an output/body before invoking the comment action rather than wrapping a command substitution in another fence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. build-review-comment.sh reads the Mermaid file into a complete Markdown body, and the sticky-comment step receives that file through path; no shell expression is embedded in a YAML input.
| PR_TITLE="${{ inputs.sync_pr_title }} $(git rev-parse --short "$TARGET_SHA")" | ||
| PR_URL="$(gh pr list --repo "$REPO" --head "$SYNC_PR_BRANCH" --base "$TARGET_BRANCH" --state open --json url --jq '.[0].url // empty' || true)" | ||
| if [ -z "$PR_URL" ]; then | ||
| gh pr create --repo "$REPO" --head "$SYNC_PR_BRANCH" --base "$TARGET_BRANCH" --title "$PR_TITLE" --body "CodeBoarding sync PR for ${TARGET_BRANCH}." >/dev/null 2>&1 || true |
There was a problem hiding this comment.
When sync_strategy=pull_request, the branch push authenticates through the URL, but this step never exports GH_TOKEN or GITHUB_TOKEN for the subsequent gh pr list/create calls. GitHub does not automatically expose an action input as an environment variable, so normal callers that only provide push_token push the sync branch but fail to open its PR; all failures are swallowed and the step misleadingly reports committed=true. Restore GH_TOKEN: ${{ inputs.push_token }} on this step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. The delivery step now receives GH_TOKEN/GH_ENTERPRISE_TOKEN, and all rolling-PR lookup/create calls live in deliver-sync.sh under that authenticated environment. PR creation failures are no longer swallowed.
| candidate = Path(path) | ||
| if not candidate.is_absolute(): | ||
| candidate = Path(output_dir) / candidate | ||
| return candidate |
There was a problem hiding this comment.
When the CLI returns a relative path such as out/analysis.json, it is relative to the subprocess working directory (output_dir.parent), but this code prefixes output_dir and looks for out/out/analysis.json. The newly added success test exercises exactly this contract and fails for that reason; resolve relative paths against the same working directory used by _run_command so valid CLI output does not abort analysis.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 15ea6a8. Relative analysis_path values are now resolved against the subprocess working directory (output_dir.parent), matching the directory passed to Popen(cwd=...); the success test uses out/analysis.json to lock this contract.
| git config user.name "codeboarding-review[bot]" | ||
| git config user.email "codeboarding-review[bot]@users.noreply.github.com" | ||
| git add "$OUTPUT_DIR" 2>/dev/null || true |
There was a problem hiding this comment.
The render step writes docs/development/architecture.md, but this git add stages only .codeboarding. Therefore sync commits and rolling PRs never include the generated architecture document, even though the repository workflow explicitly treats it as generated output; add that path to the staged set alongside the output directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by reducing the v2 sync contract. Sync no longer renders or commits architecture Markdown; it persists only Core's analysis baseline artifacts and removes legacy generated documentation. There is therefore no architecture document to stage.
| if [ -n "$BASE_DIR" ] && git worktree list | awk '{print $1}' | grep -q "$BASE_DIR"; then | ||
| git worktree remove --force "$BASE_DIR" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
On the normal warm-baseline path, BASE_FOR_DIFF points to $BASE_DIR/.codeboarding/analysis.json, but this removes the base worktree before publishing that path to the render step. Removing the worktree deletes the referenced analysis file, so every review that successfully reuses a committed baseline subsequently fails with Review baseline missing; copy the baseline to scratch storage or defer worktree removal until after rendering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Base analysis is now copied/generated in action-owned base-state, separate from the base worktree. Removing or cleaning a worktree cannot invalidate the path consumed by the renderer.
| rm -rf "$OUTPUT_DIR" | ||
| mkdir -p "$OUTPUT_DIR" "$OUTPUT_DIR/health" |
There was a problem hiding this comment.
Sync now recursively deletes the entire .codeboarding directory before recreating generated files, so tracked user inputs such as .codeboarding/.codeboardingignore, health/.healthignore, and health/health_config.json are staged as deletions and removed from the repository. The README and sync workflow explicitly distinguish these inputs from generated artifacts; delete only owned generated files as the previous implementation did.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. install-sync.sh updates only Core's persisted-artifact manifest and explicitly preserves .codeboardingignore, health configuration, and other user-owned files. A focused regression test covers these files.
| if [ -d "$OUT" ]; then | ||
| rm -rf "$OUT" | ||
| mkdir -p "$OUT" | ||
| fi |
There was a problem hiding this comment.
In repository-analysis mode, --out is caller-controlled and may point to an existing directory, but this now deletes that directory wholesale before creating the harness workspace. A command such as run_local.sh --out /tmp/project-output ... destroys unrelated contents, and accidentally setting --out to the analyzed repository can delete the repository itself; clear only action-owned subdirectories or require an empty dedicated output path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 15ea6a8. The local harness now removes only its action-owned $OUT/work directory. Caller-owned files elsewhere under --out are preserved.
| N_CHANGED="$(jq -r '.n_changed' "$META")" | ||
| TRUNCATED="$(jq -r '.truncated | ascii_downcase' "$META")" | ||
| echo "diagram_md=$DIAGRAM_OUT" >> "$GITHUB_OUTPUT" | ||
| echo "n_changed=$N_CHANGED" >> "$GITHUB_OUTPUT" | ||
| echo "truncated=$TRUNCATED" >> "$GITHUB_OUTPUT" | ||
| echo "rendered=true" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
When the analyses are identical or the graph cannot be rendered within limits, diff_to_mermaid.py writes an empty file and reports rendered=false, but this step ignores that field and unconditionally emits rendered=true. The upload and comment steps then run with an empty diagram while the metadata may misleadingly report an auto-trimmed render; parse .rendered and gate publication on its actual value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. render-review.sh reads .rendered as a JSON boolean via tostring and fails with a clear message when no safe diagram can be rendered; publication does not claim success with an empty diagram.
| git config user.name "codeboarding-review[bot]" | ||
| git config user.email "codeboarding-review[bot]@users.noreply.github.com" | ||
| git add "$OUTPUT_DIR" 2>/dev/null || true |
There was a problem hiding this comment.
Cold-start syncs fail silently in repositories whose .gitignore excludes .codeboarding/: git add rejects the generated untracked files, stderr and the nonzero status are swallowed, and the following cached-diff check reports that architecture is unchanged. The previous implementation used git add -f -A for action-owned generated paths; retain forced staging so common cache-ignore rules do not disable baseline delivery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Delivery enumerates only action-owned generated paths and stages them with git add -f -A, so an ignored .codeboarding/ directory does not disable baseline commits.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9dc7b6a94
ℹ️ 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".
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
| python-version: '3.13' |
There was a problem hiding this comment.
With the default codeboarding_version still pinned to 0.13.4, selecting Python 3.13 makes the following pip install unable to resolve that release: its langchain-cerebras>=0.8 dependency is published for Python <3.13. The previous setup deliberately selected 3.12 for this constraint, so default sync and review runs now stop during installation before any analysis starts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. The action is pinned to Python 3.12 and installs codeboarding==0.13.5, matching the release's dependency constraints.
| esac | ||
| [ -n "$ISSUE_PR_URL" ] || skip "Not a pull request comment." | ||
|
|
||
| PR_JSON="$(gh api "$ISSUE_PR_URL")" |
There was a problem hiding this comment.
On every trusted issue_comment invocation, this is the first gh call, but the guard step no longer exports GH_TOKEN or GITHUB_TOKEN; passing github_token as a composite-action input does not create either environment variable. Consequently /codeboarding runs fail here with an authentication error instead of resolving the PR. The GitHub CLI environment documentation identifies GH_TOKEN and GITHUB_TOKEN as the authentication variables for API requests, so this step needs to map inputs.github_token to one of them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
can we somehow also extract these LoC to a script file, it is just way too much here to read.
There was a problem hiding this comment.
Addressed. The guard receives GH_TOKEN/GH_ENTERPRISE_TOKEN and normalizes GH_HOST; the slash-command API lookup is authenticated. The guard logic itself has also moved to scripts/action/guard.sh.
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: ${{ steps.guard.outputs.checkout_repo }} | ||
| path: target-repo | ||
| token: ${{ inputs.github_token }} | ||
| ref: ${{ steps.guard.outputs.checkout_ref || github.sha }} |
There was a problem hiding this comment.
For a pull request from a fork, checkout_ref is the fork's head SHA, but this checkout omits repository, so actions/checkout defaults to github.repository (the upstream base repository). That SHA is not on the upstream repository's normal branches or tags, causing checkout to fail before the later fork-aware fetch and analysis logic can run. Pass steps.guard.outputs.head_repo for review mode, as required by the documented checkout repository input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
tea forks are even failing now.
There was a problem hiding this comment.
Resolved by rejecting fork PRs before checkout. Same-repository reviews explicitly pass checkout_repo and the exact head SHA to actions/checkout; privileged fork checkout is intentionally unsupported.
| LICENSED_FLAG="--licensed"; MAX_DEPTH=10; TIER="licensed" | ||
| else | ||
| LICENSED_FLAG=""; MAX_DEPTH=3; TIER="free-tier" | ||
| RELAY_ARGS=(--upstream-base-url https://openrouter.ai/api/v1 --ready-file "$READY") |
There was a problem hiding this comment.
When OIDC or license authentication is used, the relay replaces the request's authorization header with a GitHub OIDC JWT (optionally suffixed with the CodeBoarding license), but this now forwards that bearer directly to OpenRouter. OpenRouter expects an OpenRouter API key and does not validate the CodeBoarding OIDC/license format; the relay is designed to target the hosted CodeBoarding proxy that validates the JWT and substitutes the real provider key. Thus every hosted-tier analysis reaches the model API with invalid credentials even after id-token: write is configured.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this seems legit of a comment
There was a problem hiding this comment.
Addressed and regression-tested. Hosted requests now go to the deployed CodeBoarding AWS Lambda URL, where GitHub OIDC is validated and the AWS-managed OpenRouter key is substituted. The Bloxstrap failure was the old branch sending the OIDC JWT directly to OpenRouter.
| id: sync_commit | ||
| if: always() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'sync' |
There was a problem hiding this comment.
If Render sync docs fails after analysis succeeds, this always() condition still runs the commit step. Because the failed render never publishes docs_dir, the step deletes the existing .codeboarding tree, ignores the empty markdown copy, copies only the analysis artifacts, and can commit and push the deletion of all rendered documentation despite the workflow already being failed. Gate this step on a successful sync_render outcome so a renderer failure cannot deliver a partial baseline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think rendering sync docs will stop rendering afte rmy comments but let's double check
There was a problem hiding this comment.
Resolved by removing sync-document rendering from v2. Delivery runs only after successful baseline analysis and installs only Core-owned analysis artifacts, so a renderer can no longer cause a partial documentation commit.
| --analysis "${{ steps.sync_analyze.outputs.analysis_path }}" \ | ||
| --output-dir "$DOC_DIR" \ | ||
| --repo-name "${{ github.repository }}" \ | ||
| --repo-ref "${{ steps.guard.outputs.target_sha }}" \ |
There was a problem hiding this comment.
On every sync, repo_ref is now only the raw commit SHA, although the unchanged render_docs API uses this value as the prefix for generated source-file links; the previous flow supplied a GitHub blob URL containing the repository, branch, and output path. The resulting markdown therefore contains relative links beginning with a SHA instead of links to repository source. Build the full https://github.com/<repo>/blob/<ref>/... prefix before calling the renderer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No longer applicable to the minimal v2 contract. Sync does not generate Markdown or source links; it versions only Core's persisted analysis state.
|
|
||
| See the [setup guide](https://github.com/CodeBoarding/CodeBoarding-action#more-usage) for the exact workflow snippet. | ||
| **Hosted webview:** [open artifact](${{ steps.upload_review_artifact.outputs.artifact-url }}) |
There was a problem hiding this comment.
For every successful review, artifact-url is the GitHub Actions artifact URL, not a CodeBoarding webview URL, so the link labeled “Hosted webview” opens or downloads the artifact instead of showing the interactive architecture view promised by the action. The previous flow constructed the URL from webview_base_url and the run metadata; retain that URL construction and keep the artifact URL as a separate download link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. The review body now has a CodeBoarding webview URL built from repository/PR/run metadata and a separately labeled artifact download URL.
| echo "target_sha=$GITHUB_SHA" >> "$GITHUB_OUTPUT" | ||
| echo "target_branch=$REF_NAME" >> "$GITHUB_OUTPUT" | ||
| echo "checkout_ref=$GITHUB_SHA" >> "$GITHUB_OUTPUT" | ||
| echo "commit_message=${{ inputs.commit_message }}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
When a caller customizes commit_message with shell syntax such as backticks or $(...), direct expression substitution places that text inside the generated Bash program, where it is executed rather than retained as commit-message text; a multiline message can also inject additional entries into GITHUB_OUTPUT. Treat the action input as data by mapping it through env and use the multiline output form if it must be forwarded to another step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by removing configurable commit messages from v2. The baseline commit message is a fixed action-owned constant, eliminating this expression-to-shell boundary and unnecessary API surface.
| import mermaid from 'https://cdn.jsdelivr.net/npm/mermaid@10/dist/mermaid.esm.min.mjs'; | ||
| <pre class=\"mermaid\">\n{graph}\n</pre> | ||
| <script type=\"module\"> | ||
| import mermaid from 'https://cdn.jsdelivr.net/npm/mermaid@10/dist/mermaid.esm.min.js'; |
There was a problem hiding this comment.
Every generated local preview imports mermaid.esm.min.js, but Mermaid 10 publishes the ESM bundle at dist/mermaid.esm.min.mjs; the new .js URL returns no module, leaving preview.html with an unrendered Mermaid source block. Restore the .mjs bundle path so the documented local preview works in the browser.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 15ea6a8. The local preview now imports Mermaid 10's mermaid.esm.min.mjs bundle.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ecc8286dc
ℹ️ 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".
| if [ "$HAS_BASELINE" = "true" ]; then | ||
| BASE_FOR_DIFF="$BASE_DIR/.codeboarding/analysis.json" | ||
| elif [ -n "$BASE_FULL_PATH" ] && [ -f "$BASE_FULL_PATH" ]; then | ||
| BASE_FOR_DIFF="$BASE_FULL_PATH" |
There was a problem hiding this comment.
When a committed baseline exists but incremental analysis returns requires_full_analysis=true—for example after an incompatible baseline-format upgrade—the step creates a fresh base analysis in BASE_FULL_PATH, but this branch still selects the original baseline merely because HAS_BASELINE remains true. After the head is analyzed using the rebuilt state, rendering therefore compares it with the stale baseline that triggered the fallback, which can fail or report spurious changes; prefer BASE_FULL_PATH whenever the base fallback ran.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed by simplifying review state handling. The base is analyzed into base-state; if incremental requests a rebuild, full analysis replaces that state. The rebuilt state is then both rendered as the base and copied to seed head analysis, so the stale baseline is never selected.
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
When the guard deliberately soft-skips an unsupported event, tag push, or bot-owned sync commit, this checkout and the following Python setup and package installation still run because none is conditioned on steps.guard.outputs.skip. A run documented as skipped can therefore spend several minutes downloading dependencies or even fail on checkout/PyPI problems; gate these preparation steps on skip != 'true' as the analysis steps are.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Checkout, Python setup, Core installation, authentication, and analysis are all gated on steps.guard.outputs.skip != 'true'.
| fi | ||
|
|
||
| PR_TITLE="${{ inputs.sync_pr_title }} $(git rev-parse --short "$TARGET_SHA")" |
There was a problem hiding this comment.
When a caller customizes sync_pr_title with shell syntax such as backticks, $(...), or a double quote, GitHub substitutes the input into this Bash program before execution, so the title can execute commands or break the step instead of remaining PR-title text. Pass this separate input through env and expand the environment variable inside the quoted assignment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by removing configurable rolling-PR titles. v2 uses the fixed title chore(codeboarding): sync analysis baseline, so an existing rolling PR remains accurate as its branch is updated.
| printf '%s' "$DIFF" > "$META" | ||
|
|
||
| N_CHANGED="$(jq -r '.n_changed' "$META")" | ||
| TRUNCATED="$(jq -r '.truncated | ascii_downcase' "$META")" |
There was a problem hiding this comment.
diff_to_mermaid.py serializes truncated as a JSON boolean, but ascii_downcase only accepts strings; with jq 1.7, even { "truncated": false } fails here with explode input must be a string. Consequently every review that reaches this step exits before publishing its render outputs, artifact, or success comment; read the boolean directly with jq -r '.truncated' or convert it using tostring first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Both truncated and rendered are converted with jq ... | tostring; no string-only filter is applied directly to JSON booleans.
| fi | ||
|
|
||
| PR_TITLE="${{ inputs.sync_pr_title }} $(git rev-parse --short "$TARGET_SHA")" | ||
| PR_URL="$(gh pr list --repo "$REPO" --head "$SYNC_PR_BRANCH" --base "$TARGET_BRANCH" --state open --json url --jq '.[0].url // empty' || true)" |
There was a problem hiding this comment.
When a contributor has an open fork PR whose head branch is also named codeboarding/sync and whose base matches TARGET_BRANCH, this bare --head lookup can return that cross-repository PR first. The action then skips creating a PR for the machine-owned branch it just pushed and reports the contributor's URL and number instead, allowing an accidental or deliberately named fork branch to block rolling-baseline delivery; retain the previous isCrossRepository == false filter when selecting the PR.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
there is way too much logic here which should be in scripts files and in general simplified we have 60 different if else.
There was a problem hiding this comment.
Addressed. Rolling-PR queries use the explicit base-repository owner and machine branch (OWNER:codeboarding/sync), which excludes identically named fork branches.
| : > "$LICENSE_FILE" | ||
| if [ -n "${LICENSE_KEY:-}" ]; then | ||
| printf '%s' "$LICENSE_KEY" > "$LICENSE_FILE" |
There was a problem hiding this comment.
On a multi-user self-hosted runner with the usual 0022 umask and a traversable runner temp directory, this creates license.txt as mode 0644, leaving the CodeBoarding license readable by other local users for the duration of the analysis—and potentially longer if the job is canceled before cleanup. Set umask 077 before creating the relay directory/files or explicitly chmod the license file to 0600, as the previous credential setup did.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Authentication setup applies umask 077 before creating the auth directory, provider key, license, PID, and relay state files.
|
/codeboarding |
1 similar comment
|
/codeboarding |
Architecture reviewStatus: 5 changed component(s) graph LR
n_Visual_Rendering_Engine["Visual Rendering Engine"]
n_Structural_Diff_Engine["Structural Diff Engine"]
n_Interaction_Orchestrator["Interaction Orchestrator"]
del_Analysis_Engine_Adapter["Analysis Engine Adapter"]
del_Telemetry_Feedback_Handler["Telemetry #38; Feedback Handler"]
n_Structural_Diff_Engine -- "Provides change-set data" --> n_Visual_Rendering_Engine
n_Interaction_Orchestrator -- "Consumes issue counts for UI" --> n_Visual_Rendering_Engine
n_Interaction_Orchestrator -- "Orchestrates structural comparison" --> n_Structural_Diff_Engine
del_Analysis_Engine_Adapter -- "Passes repository context for CTA generation" --> n_Interaction_Orchestrator
n_Visual_Rendering_Engine -- "Queries for architectural changes" --> n_Structural_Diff_Engine
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
class n_Visual_Rendering_Engine,n_Structural_Diff_Engine,n_Interaction_Orchestrator modified;
class del_Analysis_Engine_Adapter,del_Telemetry_Feedback_Handler deleted;
linkStyle 2 stroke:#0b5d23,stroke-width:2px;
linkStyle 3,4 stroke:#82071e,stroke-width:2px,stroke-dasharray:5 3;
Hosted webview: open artifact run 30995818058 · attempt 1 |
|
/codeboarding |
7 similar comments
|
/codeboarding |
|
/codeboarding |
|
/codeboarding |
|
/codeboarding |
|
/codeboarding |
|
/codeboarding |
|
/codeboarding |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce4e334208
ℹ️ 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".
| PR_URL="$(gh pr list --repo "$REPO" --head "$SYNC_PR_BRANCH" --base "$TARGET_BRANCH" --state open --json url --jq '.[0].url // empty' || true)" | ||
| if [ -z "$PR_URL" ]; then | ||
| gh pr create --repo "$REPO" --head "$SYNC_PR_BRANCH" --base "$TARGET_BRANCH" --title "$PR_TITLE" --body "CodeBoarding sync PR for ${TARGET_BRANCH}." >/dev/null 2>&1 || true |
There was a problem hiding this comment.
On the second and subsequent pull_request syncs, this lookup returns the existing PR URL, so the creation block is skipped and the newly computed PR_TITLE is never applied. The machine branch is still force-updated, but the open PR's title permanently advertises the target SHA from its first run, misleading reviewers about which source revision the rolling baseline represents; edit the existing PR's title after the lookup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by making the rolling PR title stable instead of embedding a source SHA. Existing and subsequent runs all describe the same rolling baseline purpose, while the branch content itself identifies the latest analyzed commit.
| LLM_API_KEY="$(printf '%s' "$LLM_API_KEY" | tr -d '[:space:]' | sed -e 's/^"//;s/"$//' -e "s/^'//;s/'\$//")" | ||
| LLM_API_KEY="${LLM_API_KEY#OPENROUTER_API_KEY=}" | ||
| echo "::add-mask::$LLM_API_KEY" | ||
| echo "OPENROUTER_API_KEY=$LLM_API_KEY" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
When llm_api_key is supplied, writing it to GITHUB_ENV makes the real API credential available to every subsequent step in the caller's job, including the artifact-upload and sticky-comment third-party actions, and the final cleanup never overwrites or unsets it. The previous flow dropped key material immediately after analysis; scope this environment variable to the analysis commands or clear it before entering the output phase so BYO credentials are not exposed beyond the engine.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Provider keys are stored in a mode-0700 action-owned auth directory, exported only by with-auth.sh around the analysis subprocess, and removed immediately afterward. They are never written to GITHUB_ENV or exposed to upload/comment actions.
| # retries: another push may land on target_branch while the analysis runs. | ||
| # Fail-open on final rejection — the next repository change regenerates. ---- | ||
| git commit -m "$COMMIT_MESSAGE" >/dev/null | ||
| AUTH_URL="https://x-access-token:${push_token}@github.com/${REPO}.git" |
There was a problem hiding this comment.
When this action runs on GitHub Enterprise Server, the repository lives under github.server_url, but the direct-push remote is hardcoded to github.com. Every otherwise successful sync therefore attempts to push OWNER/REPO to the public GitHub host and reports committed=false; construct the authenticated remote from ${{ github.server_url }} as the previous delivery flow did.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Sync remotes and review fetch URLs are built from github.server_url; GH_HOST and GH_ENTERPRISE_TOKEN are also supplied for GHES API operations.
|
/codeboarding |
Amp-Thread-ID: https://ampcode.com/threads/T-019fc956-723a-737d-a4ac-b0470f146cd4 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fc956-723a-737d-a4ac-b0470f146cd4 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fc956-723a-737d-a4ac-b0470f146cd4 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fc956-723a-737d-a4ac-b0470f146cd4 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fc956-723a-737d-a4ac-b0470f146cd4 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fc956-723a-737d-a4ac-b0470f146cd4 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fc956-723a-737d-a4ac-b0470f146cd4 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fc956-723a-737d-a4ac-b0470f146cd4 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fc956-723a-737d-a4ac-b0470f146cd4 Co-authored-by: Amp <amp@ampcode.com>
CodeBoarding reviewStatus: 6 changed components See the full change in CodeBoarding. graph LR
n_Visual_Rendering_Engine["Visual Rendering Engine"]
n_Structural_Diff_Engine["Structural Diff Engine"]
n_Interaction_Orchestrator["Interaction Orchestrator"]
n_Repository_Analysis_Artifact_Orchestrator["Repository Analysis #38; Artifact Orchestrator"]
del_Analysis_Engine_Adapter["Analysis Engine Adapter"]
del_Telemetry_Feedback_Handler["Telemetry #38; Feedback Handler"]
n_Structural_Diff_Engine -- "Provides change-set data" --> n_Visual_Rendering_Engine
n_Interaction_Orchestrator -- "Consumes issue counts for UI" --> n_Visual_Rendering_Engine
n_Repository_Analysis_Artifact_Orchestrator -- "Triggers documentation synchronization" --> n_Visual_Rendering_Engine
n_Repository_Analysis_Artifact_Orchestrator -- "Provides stateful analysis snapshots" --> n_Structural_Diff_Engine
del_Analysis_Engine_Adapter -- "Passes repository context for CTA generation" --> n_Interaction_Orchestrator
n_Visual_Rendering_Engine -- "Queries for architectural changes" --> n_Structural_Diff_Engine
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
class n_Repository_Analysis_Artifact_Orchestrator added;
class n_Visual_Rendering_Engine,n_Structural_Diff_Engine,n_Interaction_Orchestrator modified;
class del_Analysis_Engine_Adapter,del_Telemetry_Feedback_Handler deleted;
linkStyle 2,3 stroke:#0b5d23,stroke-width:2px;
linkStyle 4,5 stroke:#82071e,stroke-width:2px,stroke-dasharray:5 3;
|
ivanmilevtues
left a comment
ivanmilevtues
left a comment
There was a problem hiding this comment.
Lot's of comments to be addressed and answere
| raise SystemExit(f"Missing checkout directory: {checkout}") | ||
|
|
||
| if args.mode == "incremental": | ||
| requires_full, analysis_path, _ = run_incremental(checkout, output_dir) |
There was a problem hiding this comment.
Are we actually setting the sys variable so we know that this run is done via "github_action" and not just a "core"/"oss" call?
There was a problem hiding this comment.
No—the wrapper currently invokes the generic codeboarding CLI, whose entry point defaults CODEBOARDING_SOURCE to oss. The minimal fix is to set CODEBOARDING_SOURCE=github_action only around the authenticated analysis subprocess. That will be included in the simplification so telemetry identifies these runs correctly.
| pass | ||
|
|
||
|
|
||
| ROOT_ARTIFACTS = ( |
There was a problem hiding this comment.
We should have this list as constants somewhere in the core. Reuse them from there.
There was a problem hiding this comment.
Core 0.13.5 exposes these only as separate constants today: ANALYSIS_FILENAME and FINGERPRINT_FILENAME in utils, plus STATIC_ANALYSIS_PKL and STATIC_ANALYSIS_SHA in static_analyzer.analysis_cache. There is no single public persisted-artifact manifest, and codeboarding_version.json has no corresponding public constant. The clean direction is to add one public artifact manifest in Core and consume it here after bumping the engine pin; importing several internal modules from the action would create the wrong coupling.
| for name in ROOT_ARTIFACTS: | ||
| _replace_optional(analysis_dir / name, output_dir / name, stage_paths) | ||
|
|
||
| _replace_optional( |
There was a problem hiding this comment.
these should be constants also I think, best if they are taken from the core.
There was a problem hiding this comment.
health/health_report.json also has no public filename/path constant in Core 0.13.5. Whether this should be persisted at all depends on the minimal sync contract. If health output remains part of sync, Core should expose it through the same public artifact manifest; if sync is baseline-only, this path can disappear from the action entirely.
| out_path.write_text("\n\n".join(p.read_text(encoding="utf-8").rstrip() for p in files) + "\n", encoding="utf-8") | ||
|
|
||
|
|
||
| def main(argv: list[str] | None = None) -> int: |
There was a problem hiding this comment.
hmm do we really need this, I think we might not need it. I kind of want only these to be taken into account:
"fingerprint.json",
+
"static_analysis.pkl",
+
"static_analysis.sha",
+
"codeboarding_version.json",
The renderer I though is the thing which writes the comment in the PR itself is this true?
There was a problem hiding this comment.
render_sync_docs.py is not the PR-comment renderer. It calls Core’s documentation renderer to generate the versioned Markdown pages used by sync mode and optionally concatenates them into docs/development/architecture.md. Review comments are rendered separately by diff_to_mermaid.py and posted by the sticky-comment action. If the intended sync contract is only persistent analysis state, the entire sync-doc rendering path can be removed. One detail to confirm: incremental review/sync still needs analysis.json in addition to the four sidecars listed here.
| if git diff --cached --quiet; then | ||
| echo "::notice::Generated architecture is unchanged; nothing to commit." | ||
| reconcile_stale_sync_pr | ||
| echo "files_written=${#new_md[@]}" >> "$GITHUB_OUTPUT" | ||
| echo "::notice::Generated architecture is unchanged." | ||
| echo "files_written=0" >> "$GITHUB_OUTPUT" | ||
| echo "committed=false" >> "$GITHUB_OUTPUT" | ||
| exit 0 |
There was a problem hiding this comment.
yes but this is a bit of acorner case
| fi | ||
|
|
||
| PR_TITLE="${{ inputs.sync_pr_title }} $(git rev-parse --short "$TARGET_SHA")" | ||
| PR_URL="$(gh pr list --repo "$REPO" --head "$SYNC_PR_BRANCH" --base "$TARGET_BRANCH" --state open --json url --jq '.[0].url // empty' || true)" |
There was a problem hiding this comment.
there is way too much logic here which should be in scripts files and in general simplified we have 60 different if else.
| fi | ||
|
|
||
| PR_TITLE="${{ inputs.sync_pr_title }} $(git rev-parse --short "$TARGET_SHA")" | ||
| PR_URL="$(gh pr list --repo "$REPO" --head "$SYNC_PR_BRANCH" --base "$TARGET_BRANCH" --state open --json url --jq '.[0].url // empty' || true)" |
| # retries: another push may land on target_branch while the analysis runs. | ||
| # Fail-open on final rejection — the next repository change regenerates. ---- | ||
| git commit -m "$COMMIT_MESSAGE" >/dev/null | ||
| AUTH_URL="https://x-access-token:${push_token}@github.com/${REPO}.git" |
| if: failure() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.quota.outputs.exhausted == 'true' && steps.review_comment.outcome != 'success' | ||
| continue-on-error: true | ||
| uses: marocchino/sticky-pull-request-comment@v2 | ||
| WORK="${RUNNER_TEMP}/cb-review" |
There was a problem hiding this comment.
again huge script files.
There was a problem hiding this comment.
Addressed. Review analysis, sync delivery, authentication, rendering, artifact construction, and guard logic now live in separate focused scripts. The complete runtime implementation remains exactly 1,000 lines.
Move orchestration into focused scripts, reduce persisted sync output to Core analysis state, support direct providers with model overrides, and route hosted OIDC requests through the deployed AWS proxy. BREAKING CHANGE: v2 removes deprecated v1 inputs and generated sync documentation. Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15ea6a89be
ℹ️ 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".
| github_token: | ||
| description: 'Token used for comments and sync delivery.' | ||
| required: false | ||
| default: '.codeboarding' | ||
| output_format: | ||
| description: 'Sync mode: rendered docs format. Currently only .md is supported.' | ||
| default: ${{ github.token }} |
There was a problem hiding this comment.
When the checked-in sync workflow runs, .github/workflows/codeboarding-sync.yml still supplies the minted App credential as push_token, but the rewritten action only declares and reads github_token. GitHub therefore ignores the App token and delivery falls back to github.token, so baseline commits lose the intended CodeBoarding App attribution and rolling PRs created with the workflow token will not trigger downstream workflows as App-created PRs would. Update the workflow to pass the token through github_token.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0c7612c. The dogfood sync workflow now passes the minted App token through the action’s github_token input, preserving App attribution and App-token behavior for both push and pull-request delivery.
| if [ ! -s "$READY" ]; then | ||
| echo "::error::OIDC relay did not start." >&2 | ||
| cat "$LOG" >&2 || true | ||
| exit 1 |
There was a problem hiding this comment.
When the OIDC relay starts but never writes its ready file, this error path exits without killing the background PID or removing $RUNNER_TEMP/codeboarding-auth. With license_key configured, license.txt therefore remains available while the subsequent review-failure third-party action runs, and on a self-hosted runner the relay and credential files can persist beyond the failed step. Add failure cleanup for the relay process and authentication directory before exiting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0c7612c. The failed-start path now kills and waits for the relay process, then removes the authentication directory before exiting. This prevents the relay and license/credential files from surviving a failed setup.
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| cd "$CHECKOUT_DIR" | ||
| SYNC_BRANCH=codeboarding/sync |
There was a problem hiding this comment.
When a manual run selects a custom sync_pr_branch, the checked-in sync workflow still advertises and passes that input, but delivery now unconditionally uses codeboarding/sync. The workflow therefore accepts the user's branch choice while silently updating a different branch; either remove the obsolete workflow input or wire it through to this script.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0c7612c by removing the obsolete sync_pr_branch workflow input and pass-through. The minimal contract intentionally uses one fixed machine-owned branch, codeboarding/sync, so the workflow no longer advertises a selector the action ignores.
| if [ "$HEAD_MODE" != "incremental" ] || [ -z "$HEAD_PATH" ]; then | ||
| echo "::error::Could not parse head incremental output contract." >&2 | ||
| echo "$HEAD_OUTPUT" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
When incremental analysis returns the supported fallback response requires_full_analysis=true with no analysis_path, this validation exits before the full-analysis branch below can run. This is the exact contract accepted by analyze_repository.py for a missing or incompatible baseline, so the documented local pipeline fails on its first run or any required rebuild instead of falling back to run_full.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0c7612c. The local runner now accepts an empty analysis_path when requires_full_analysis=true, then executes and validates the full-analysis fallback. It still rejects an empty path when incremental analysis claims no fallback is needed.
| if [ -d "$BASE_DIR/.codeboarding" ] && [ -f "$BASE_DIR/.codeboarding/analysis.json" ]; then | ||
| cp -a "$BASE_DIR/.codeboarding/." "$HEAD_DIR/." | ||
| BASE_FOR_DIFF="$BASE_DIR/.codeboarding/analysis.json" |
There was a problem hiding this comment.
When the selected base commit contains a committed baseline that predates that commit, this path uses the stale analysis.json directly instead of incrementally updating it against the exact base checkout. The composite action refreshes the base first, but the documented local pipeline consequently attributes changes already present on the base branch to the head; run the base incremental/full flow before assigning BASE_FOR_DIFF.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0c7612c. The local review flow now seeds state from the committed baseline, refreshes it against the exact checked-out base SHA via incremental/full analysis, and only then uses that refreshed result for the visual diff. This matches the composite action and avoids attributing pre-existing base changes to the PR.
| for required in "${artifacts[@]:0:4}"; do | ||
| [ -f "$ANALYSIS_DIR/$required" ] || { echo "::error::Core did not produce $required."; exit 1; } | ||
| done |
There was a problem hiding this comment.
When Core produces a valid analysis.json and fingerprint.json but omits the optional static_analysis.pkl/.sha warm-start cache, this positional check treats all of the manifest's first four entries as mandatory and aborts delivery. A missing static cache only requires a cold static-analysis pass on the next run, so a successful analysis should still be committed; validate the genuinely required artifacts by name and copy or remove cache files conditionally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0c7612c. Installation now validates only Core’s required analysis artifact. Every persisted artifact from Core’s manifest is copied when present and stale tracked copies are removed when absent, so optional static-analysis caches no longer block a valid sync.
| except ImportError: | ||
| from static_analyzer.analysis_cache import STATIC_ANALYSIS_PKL, STATIC_ANALYSIS_SHA | ||
| from utils import ANALYSIS_FILENAME, FINGERPRINT_FILENAME | ||
| artifacts = (ANALYSIS_FILENAME, FINGERPRINT_FILENAME, STATIC_ANALYSIS_PKL, STATIC_ANALYSIS_SHA) | ||
| print(*artifacts, sep="\n") |
There was a problem hiding this comment.
When the pinned Core release uses this compatibility import path and the target repository already tracks the legacy .codeboarding/codeboarding_version.json, the filename is absent from artifacts. The installation loop therefore neither replaces nor removes nor stages that generated sidecar, leaving obsolete version metadata in every v2 baseline and potentially causing subsequent incremental runs to keep treating the state as incompatible. Include the legacy filename in the compatibility manifest so its absence in the analysis output removes the tracked copy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 0c7612c. The 0.13.7 compatibility manifest now includes codeboarding_version.json; because installation removes absent manifest artifacts, an existing legacy version sidecar is deleted and staged instead of remaining in the v2 baseline.
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
|
/codeboarding |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 281e85dd06
ℹ️ 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".
| for legacy in "$output"/*.md "$output/health/health_report.json" \ | ||
| "$CHECKOUT_DIR/docs/development/architecture.md"; do |
There was a problem hiding this comment.
When a repository already has a hand-written docs/development/architecture.md rather than a v1-generated copy, its first v2 sync unconditionally removes and stages that file, and the default push strategy commits the deletion directly to the target branch. Nothing here checks for a CodeBoarding-generated marker, so restrict legacy cleanup to files that can be identified as generated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 664ec59. Legacy Markdown is now deleted only when it contains CodeBoarding’s generated badge marker; hand-written .codeboarding/*.md and docs/development/architecture.md are preserved. The README and sync tests now document and verify that boundary.
| RENDERED="$(jq -r '.rendered | tostring' "$META")" | ||
| [ "$RENDERED" = true ] || { echo "::error::The architecture diff is too large to render."; exit 1; } |
There was a problem hiding this comment.
When both successful analyses contain zero components—for example in an empty repository or one with no supported source files—diff_to_mermaid.py returns rendered=false because there are no nodes, not because the graph exceeded a limit. This check consequently fails the review and posts a failure comment instead of reporting zero architectural changes; distinguish the no-components case from an oversized, unrenderable diff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 664ec59. The renderer now reports an explicit empty state separately from truncation, and the action accepts that state as a successful zero-component review while still failing genuinely oversized unrenderable diffs. The shell path is covered by a regression test.
| if base_component is not None and _owns_analysis_change( | ||
| base_component, | ||
| component, | ||
| changed_methods, | ||
| changed_files, | ||
| ): | ||
| component["diff_status"] = "modified" |
There was a problem hiding this comment.
When a top-level component is deleted and owns any removed method or file, _diff_components first creates a deleted ghost, but this assignment overwrites that status with modified because _owns_analysis_change sees the removed content in the base component. Consequently ordinary component removals are rendered and counted as modifications rather than deletions; only promote components whose existing diff status is unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 664ec59. Content-hash ownership now promotes only otherwise-unchanged components to modified; existing added, modified, and especially deleted statuses remain authoritative. A removed-method/deleted-component regression test verifies the result.
| from static_analyzer.analysis_cache import STATIC_ANALYSIS_PKL, STATIC_ANALYSIS_SHA | ||
| from utils import ANALYSIS_FILENAME, FINGERPRINT_FILENAME | ||
| artifacts = (ANALYSIS_FILENAME, FINGERPRINT_FILENAME, STATIC_ANALYSIS_PKL, STATIC_ANALYSIS_SHA) |
There was a problem hiding this comment.
When the analyzed repository contains a top-level utils.py or static_analyzer package, the pinned 0.13.5 compatibility path can import that repository code instead of CodeBoarding's modules: deliver-sync.sh invokes this script after cd "$CHECKOUT_DIR", and python3 - searches the current directory first. Common Python repositories can therefore fail manifest discovery—or supply unrelated artifact names—even though Core is installed correctly; run the import from an action-owned directory or resolve the installed distribution explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 664ec59. Core’s artifact constants are now imported with the action directory as the working directory rather than the analyzed checkout, preventing target constants.py, utils.py, or static_analyzer modules from shadowing the installed Core package. The sync test now runs from a target containing a deliberately failing constants.py.
| review_artifact_url: | ||
| description: 'URL of the uploaded review analysis artifact.' | ||
| value: ${{ steps.upload_review_artifact_dotcom.outputs.artifact-url || format('{0}/{1}/actions/runs/{2}', github.server_url, github.repository, github.run_id) }} |
There was a problem hiding this comment.
On GitHub Enterprise Server, the only upload step is skipped by its github.server_url == 'https://github.com' condition, but this fallback publishes the workflow-run page as review_artifact_url. The review comment then labels that same URL “download artifacts” even though the locally built artifact is discarded at job end, so GHES consumers receive a non-artifact URL; either upload through a GHES-compatible path or omit the output and download link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 664ec59. review_artifact_url and the comment’s artifact link now come only from the upload step. On GHES, where that step is skipped, the output remains empty and the comment keeps the workflow-run link without incorrectly labeling it as a downloadable artifact.
CodeBoarding review · failedSee the workflow logs. |
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
|
/codeboarding |
CodeBoarding review · failedSee the workflow logs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8e508d0ef
ℹ️ 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".
| if [ -n "${LLM_API_KEY:-}" ]; then | ||
| echo "::add-mask::$LLM_API_KEY" | ||
| printf '%s' "$LLM_API_KEY" > "$AUTH_DIR/provider-key" |
There was a problem hiding this comment.
When llm_api_key comes from a secret pasted with surrounding whitespace or quotes—or in the previously supported OPENROUTER_API_KEY=... form—this writes the wrapper characters verbatim and with-auth.sh exports them as part of the provider credential, causing provider authentication to fail. The prior credential path explicitly stripped these common wrappers, so normalize the value before storing it while retaining masking for both the raw and normalized forms.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 3bfc007. Direct credentials are normalized before storage: whitespace, matching surrounding quotes, and the selected provider’s <ENV>= prefix are stripped, then both raw and normalized forms are masked. A regression test covers a quoted OPENROUTER_API_KEY="…" paste.
| - name: Post review failure | ||
| if: failure() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.review_body.outcome != 'success' |
There was a problem hiding this comment.
When analysis and body construction succeed but the Post review comment step fails—for example because of a transient API error—failure() is true while steps.review_body.outcome is still success, so this fallback is skipped. The earlier progress comment consequently remains stuck on “analyzing” even though the job has failed; give the publishing step an ID and gate this fallback on that step's outcome instead of the body-building step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 3bfc007. The publishing step now has the review_comment ID, and the failure fallback checks that step’s outcome. It therefore replaces the progress comment when analysis, body construction, or final comment publication fails.
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bfc007b31
ℹ️ 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".
| target_branch: ${{ github.event_name == 'workflow_dispatch' && inputs.sync_strategy == 'pull_request' && 'main' || github.ref_name }} | ||
| sync_strategy: ${{ inputs.sync_strategy || 'push' }} | ||
| sync_pr_branch: ${{ inputs.sync_pr_branch || 'codeboarding/sync' }} | ||
| force_full: ${{ inputs.force_full || false }} |
There was a problem hiding this comment.
When this workflow is manually dispatched from a non-main ref with sync_strategy=pull_request, this block no longer supplies target_branch. The action consequently falls back to github.ref_name in scripts/action/guard.sh, opening the rolling baseline PR against the selected feature branch even though this workflow is designated as this repository's main-branch baseline writer. Pass target_branch: main for this manual delivery path.
Useful? React with 👍 / 👎.
|
/codeboarding |
CodeBoarding reviewStatus: 5 changed components See the full change in CodeBoarding. graph LR
n_Visual_Rendering_Engine["Visual Rendering Engine"]
n_Structural_Diff_Engine["Structural Diff Engine"]
n_Interaction_Orchestrator["Interaction Orchestrator"]
del_Analysis_Engine_Adapter["Analysis Engine Adapter"]
del_Telemetry_Feedback_Handler["Telemetry #38; Feedback Handler"]
n_Structural_Diff_Engine -- "provides annotated structural diff" --> n_Visual_Rendering_Engine
n_Interaction_Orchestrator -- "Consumes issue counts for UI" --> n_Visual_Rendering_Engine
del_Analysis_Engine_Adapter -- "Passes repository context for CTA generation" --> n_Interaction_Orchestrator
n_Visual_Rendering_Engine -- "Queries for architectural changes" --> n_Structural_Diff_Engine
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
class n_Visual_Rendering_Engine,n_Structural_Diff_Engine,n_Interaction_Orchestrator modified;
class del_Analysis_Engine_Adapter,del_Telemetry_Feedback_Handler deleted;
linkStyle 0 stroke:#7d4e00,stroke-width:2px;
linkStyle 2,3 stroke:#82071e,stroke-width:2px,stroke-dasharray:5 3;
|
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
|
/codeboarding |
CodeBoarding reviewStatus: 5 changed components See the full change in CodeBoarding. graph LR
n_Visual_Rendering_Engine["Visual Rendering Engine"]
n_Structural_Diff_Engine["Structural Diff Engine"]
n_Interaction_Orchestrator["Interaction Orchestrator"]
del_Analysis_Engine_Adapter["Analysis Engine Adapter"]
del_Telemetry_Feedback_Handler["Telemetry #38; Feedback Handler"]
n_Visual_Rendering_Engine -- "streams rendered diagrams and metadata" --> n_Interaction_Orchestrator
n_Structural_Diff_Engine -- "provides structural delta for visualization" --> n_Visual_Rendering_Engine
n_Structural_Diff_Engine -- "reports change status for CI gating" --> n_Interaction_Orchestrator
del_Analysis_Engine_Adapter -- "Passes repository context for CTA generation" --> n_Interaction_Orchestrator
n_Visual_Rendering_Engine -- "Queries for architectural changes" --> n_Structural_Diff_Engine
n_Interaction_Orchestrator -- "Consumes issue counts for UI" --> n_Visual_Rendering_Engine
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
class n_Visual_Rendering_Engine,n_Structural_Diff_Engine,n_Interaction_Orchestrator modified;
class del_Analysis_Engine_Adapter,del_Telemetry_Feedback_Handler deleted;
linkStyle 0,2 stroke:#0b5d23,stroke-width:2px;
linkStyle 1 stroke:#7d4e00,stroke-width:2px;
linkStyle 3,4,5 stroke:#82071e,stroke-width:2px,stroke-dasharray:5 3;
|
Amp-Thread-ID: https://ampcode.com/threads/T-019fdc90-d8c3-779f-92ce-ce9888aad94d Co-authored-by: Ivan Milev <ivanmilevtues@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0efc12eb39
ℹ️ 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".
| repository: ${{ steps.guard.outputs.checkout_repo }} | ||
| path: target-repo | ||
| fetch-depth: 0 | ||
| ref: ${{ steps.guard.outputs.checkout_sha }} | ||
| ref: ${{ steps.guard.outputs.checkout_ref }} | ||
| token: ${{ inputs.github_token }} |
There was a problem hiding this comment.
When a trusted collaborator invokes /codeboarding on a private fork PR, the guard sets checkout_repo to the fork, but the default github_token is scoped to the base repository and cannot clone that separate private repository. The newly advertised trusted-fork path therefore fails at checkout unless callers supply a broader PAT/App token; fetch the PR head through the base repository's pull-request ref instead.
Useful? React with 👍 / 👎.
| owned_files = set(base_by_file) | set(current_by_file) | ||
| return bool(owned_methods & changed_methods or owned_files & changed_files) |
There was a problem hiding this comment.
When a component owns a file whose file_methods entry has an empty methods list, _methods_by_file omits that file entirely, so owned_files cannot intersect the content-hash changes found in files. If such a file changes without being added or removed—for example a component-owned configuration or module-level file—the component remains unchanged and the architectural review silently misses the edit; derive owned file paths directly from file_methods rather than from the nonempty method map.
Useful? React with 👍 / 👎.
ivanmilevtues
changed the title
2 participants
Summary
changed_only,render_depth) as deprecated no-op flags.Scope
Action-only updates in
CodeBoarding-action.