feat(review): relay the engine's early-exit verdict, a machine-readable comment line, and the platform link in the progress comment - #131
Conversation
…the comment with a machine-readable line A pull request whose changed files are all outside the analysed scope (docs, configuration, CI, tests the ignore file excludes, a language the engine does not read) cannot have moved the architecture, yet it cost a full run and its comment's "0 changed components" was whatever the diff of two graphs happened to say. A new step lists the pull request's files (the checkout is the head alone, so from the API) and counts them against the engine's own rules, imported from the installed engine: its extension map and its ignore manager over the checked-out repository. When none is analysed and a base graph is at hand, the analysis step is skipped, the base is published as the head, the diff renders empty, and the comment says "0 changed components (no analysed file changed)". A file list that cannot be read, an engine that cannot be imported or a missing base all mean "run as usual". Every review comment now ends with an HTML comment carrying the platform URL, the changed-component count, the file counts and the head sha, so the web platform's dashboard can stop parsing the prose and the mermaid block. The artifact's metadata.json carries scope_skipped, changed_files and analysed_files, as strings like every other field there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CodeBoarding reviewStatus: 1 changed component See the full change in CodeBoarding. graph LR
n_action_scripts["action_scripts"]
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_action_scripts modified;
|
![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: ab75e3572c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # The checkout is the head commit alone, so the changed files come from the API, not git. | ||
| files="$(gh api "repos/${REPOSITORY}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null)" || { |
There was a problem hiding this comment.
When a PR renames an analyzed file such as src/a.py to docs/a.md, the API's filename is the destination while the source is reported separately as previous_filename (GitHub's List pull request files schema); forwarding only filename makes this loop classify the rename as out of scope and reuse the base unchanged, so the removed component is never reported. Include rename source paths, or conservatively run the engine for renamed files whose old or new path is analyzed.
AGENTS.md reference: AGENTS.md:L5-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this seems like quite a corner case ;d.
But i think we should probably not short circuit the analysis run here, but rather have the core engine recognize when there is nothing to be done for the incremental and effectively be done instnatnly. (which i think at the moment won't be the case because we have to do the full setup if nothing else...)
| if [ -f "${BASE_DIR:-}/analysis.json" ]; then | ||
| base="${BASE_DIR}/analysis.json" | ||
| elif [ -f "${CHECKOUT_DIR}/.codeboarding/analysis.json" ]; then | ||
| base="${CHECKOUT_DIR}/.codeboarding/analysis.json" |
There was a problem hiding this comment.
When no matching published base exists, this directly treats the checkout's committed analysis.json as the merge-base graph, bypassing the compatibility and catch-up work in analyze_review. That file can describe an older commit or a different engine, ignore file, model, or depth—for example after changing depth_cap or .codeboardingignore, the configuration-keyed artifact lookup misses but this fallback still succeeds—so a docs-only PR can publish a stale graph as both base and head. Restrict the shortcut to a configuration-compatible graph for the actual merge base, or run the normal base derivation first.
AGENTS.md reference: AGENTS.md:L8-L10
Useful? React with 👍 / 👎.
| - name: Analyze pull request | ||
| id: review_analyze | ||
| if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' | ||
| if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.scope.outputs.skip != 'true' |
There was a problem hiding this comment.
If the fetched base has less than the review retention remaining, fetch_base sets renew=true, but a successful scope shortcut skips this step and therefore never emits publish_base=true; the later publish condition stays false while the review artifact still records the old fetched artifact ID and does not inline the base. That base can expire before the 14-day review artifact that references it, so the skipped path must republish or inline an expiring base.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| # The checkout is the head commit alone, so the changed files come from the API, not git. | ||
| files="$(gh api "repos/${REPOSITORY}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null)" || { |
There was a problem hiding this comment.
For pull requests exceeding the endpoint's 3,000-file maximum, --paginate cannot retrieve the omitted files, but this command treats the returned subset as complete; if that subset contains only docs/config while an omitted file is analyzed, the action skips the engine and reports a false zero. GitHub documents the cap for this endpoint (List pull request files), so compare the count with the PR's changed_files total or fall back to analysis whenever the listing may be truncated.
AGENTS.md reference: AGENTS.md:L5-L7
Useful? React with 👍 / 👎.
| ACTION_PATH: ${{ github.action_path }} | ||
| GH_TOKEN: ${{ inputs.github_token }} | ||
| GH_ENTERPRISE_TOKEN: ${{ inputs.github_token }} | ||
| GH_HOST: ${{ github.server_url }} |
There was a problem hiding this comment.
On GitHub Enterprise Server, github.server_url includes the scheme, but GH_HOST expects a hostname (GitHub CLI environment manual); unlike guard.sh and fetch-state.sh, this new path does not strip https://. The CLI consequently constructs an invalid address such as https://https//github.example.com/..., the caught request failure makes every GHES review analyze normally, and the advertised scope shortcut never works there. Normalize GH_HOST before this gh api call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think I am fine with this. But technically speaking we should be good even without a special case. In that case our iterative analysis should be instant as there are no changes or not really?
| changed=0 | ||
| analysed=0 | ||
| skip=false | ||
|
|
||
| emit() { | ||
| printf 'changed_files=%s\nanalysed_files=%s\nskip=%s\n' "$changed" "$analysed" "$skip" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
When the PR-files request fails, or when the Python scope calculation errors after the file list was read, the fallback calls emit while both counters still hold their initialized zero values. The engine then runs normally, but the new comment marker and artifact metadata claim changed_files=0 analysed_files=0, which is indistinguishable to machine consumers from a genuine empty result and can corrupt dashboard reporting. Emit empty/explicitly unknown values on fallback, or at least populate the known changed-file count before invoking the classifier.
Useful? React with 👍 / 👎.
ivanmilevtues
left a comment
ivanmilevtues
left a comment
There was a problem hiding this comment.
I am fine with this change, I think it would be better if we don't short circuit things here but rather keep this w/o business logic and just a wrapper around the core. Meaning that the core would just not do anything in the case of incremental which has nothing affected.
Here my expectation was that we would modify the inprogress message to include the PR link from the get go so that you can immedietly open the platform even while wiating for the analysis to complete.
As the latest advancements of the grouping I am not completely sure if the core would in fact be instantly done with the incremental or not (for sure we will be setting up the env which takes some time) so this said I think we should proly just merge this and maybe at some point we will have to make it a bit more proper.
| parser.add_argument("--repo-root", required=True, help="The checked-out repository, where the ignore file lives.") | ||
| args = parser.parse_args() | ||
| paths = [line.strip() for line in sys.stdin.read().splitlines() if line.strip()] | ||
| print(json.dumps(count_analysed(paths, Path(args.repo_root)))) |
There was a problem hiding this comment.
unsure what will happenw ith this print proly better to use a logger?
| ACTION_PATH: ${{ github.action_path }} | ||
| GH_TOKEN: ${{ inputs.github_token }} | ||
| GH_ENTERPRISE_TOKEN: ${{ inputs.github_token }} | ||
| GH_HOST: ${{ github.server_url }} |
There was a problem hiding this comment.
I think I am fine with this. But technically speaking we should be good even without a special case. In that case our iterative analysis should be instant as there are no changes or not really?
| # The checkout is the head commit alone, so the changed files come from the API, not git. | ||
| files="$(gh api "repos/${REPOSITORY}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename' 2>/dev/null)" || { |
There was a problem hiding this comment.
this seems like quite a corner case ;d.
But i think we should probably not short circuit the analysis run here, but rather have the core engine recognize when there is nothing to be done for the incremental and effectively be done instnatnly. (which i think at the moment won't be the case because we have to do the full setup if nothing else...)
…ciding it here The engine already skips re-detailing when an incremental finds no cluster or membership deltas (CodeBoarding #605 took that path today with no model call), so the scope pre-check that listed the pull request's files and decided the same thing in bash is gone, and the action is a wrapper again. What stays is the relay: the artifact step reads `metadata.incremental_unchanged` from the analysis the engine wrote (CodeBoarding #608) and carries it into the review artifact's metadata and, through an output, into the comment, whose status line says "(nothing analysed changed)" and whose machine-readable line says `unchanged=true`. An analysis written before the field existed reads as false, the safe direction. The progress comment now carries the platform link from the start, so a pull request can be opened there while the run is still going. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Svilen-Stefanov
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16427fd019
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if [ "${UNCHANGED:-false}" = true ]; then | ||
| STATUS="${STATUS} (nothing analysed changed)" |
There was a problem hiding this comment.
On a later run of an existing PR, analyze_review can seed the incremental analysis from that PR's previous head (scripts/action/analyze.sh:203-216), so incremental_unchanged=true only means the latest push added no analyzed changes. If the PR first changes three components and then receives a docs-only commit, the merge-base diff still reports N_CHANGED=3, but this condition produces 3 changed components (nothing analysed changed) and the marker advertises unchanged=true. Derive the PR-level verdict from the rendered base-to-head diff, or at minimum require N_CHANGED to be zero before setting the status and marker.
Useful? React with 👍 / 👎.
| # its early exit (no cluster or membership deltas, nothing re-detailed, no model consulted), | ||
| # so the comment's "0 changed components" is a decided fact rather than a model's word. An | ||
| # analysis written before the field existed reads as false, which is the safe direction. | ||
| UNCHANGED="$(jq -r 'if .metadata.incremental_unchanged == true then "true" else "false" end' "$ANALYSIS_PATH" 2>/dev/null || echo false)" |
There was a problem hiding this comment.
action.yml:362 still installs codeboarding==0.14.4, while this change depends on the subsequent Core #608 change to add metadata.incremental_unchanged; under the pinned release this lookup therefore maps the missing field to false on every run, so the advertised early-exit status and artifact verdict never activate despite the tests synthesizing the field. Bump the engine pin to a release containing that Core change and update the mirrored engine version before shipping this feature.
AGENTS.md reference: AGENTS.md:L12-L16
Useful? React with 👍 / 👎.
… the diff's zero agree The engine's early exit says the clusters and their membership held. A body-only edit keeps them while moving method hashes, which the diff counts as a modified component; and a run seeded from the pull request's previous head takes the exit for a docs-only push on top of real changes. Either way the diff is not zero, and the comment must not say nothing changed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c66b36f61e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # and the diff's zero together, and neither alone. | ||
| STATUS="${N_CHANGED} changed ${COMPONENT_NOUN}" | ||
| VERDICT_UNCHANGED=false | ||
| if [ "${UNCHANGED:-false}" = true ] && [ "$N_CHANGED" = "0" ]; then |
There was a problem hiding this comment.
When an incremental reports incremental_unchanged=true but the base-to-head diff contains only a relation delta, N_CHANGED remains 0, so this condition publishes unchanged=true and “nothing analysed changed” even though the diagram contains a changed edge. The current fix still checks only the component count; scripts/diff_to_mermaid.py deliberately returns a separate changed=true for relation-only diffs (covered by test_changed_flag_relation_only), but render-review.sh drops that value. Relay that flag and require it to be false before declaring the review unchanged.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…' file hashes
The engine records a whole-file content hash for every file a component owns. The render step
already holds the base and head analyses, so it counts the analysed files whose hash differs,
including added and removed ones, and reports the count next to the component count. At zero no
byte of analysed code changed, and the comment says so; a non-zero component count at the same
time is the analysis grouping the same code differently, and the comment says that too.
This replaces relaying an engine flag (CodeBoarding #608, now closed). The flag compared a run
against its own seed, so a re-run seeded from the pull request's previous head, a body-only edit
and clustering noise each made it disagree with what the pull request did. The hash count
compares against the merge base, needs no engine change and no engine release, and refuses to
count ("unknown") when an analysis has a file without a hash.
The machine-readable line carries it as analysed_files_changed=<n|unknown>, and the review
artifact's metadata as analysed_files_changed.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44ab29791c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not base_files or not current_files: | ||
| return None |
There was a problem hiding this comment.
When a PR deletes the last analysed source file, the head analysis legitimately has "files": {}, but this condition treats that empty index as unavailable and returns None. The artifact and comment therefore report analysed_files_changed=unknown instead of counting the removed files; similarly, a repository with no analysed files on either side never receives the zero-change verdict. Check whether the files field is absent or invalid rather than whether its mapping is empty.
Useful? React with 👍 / 👎.
2 participants
What
Reworked after review (Ivan): the action decides nothing about scope itself, and needs no engine change to say a pull request changed nothing analysed.
analysed_files_changed. At zero no byte of analysed code changed. It isunknownwhen either analysis has a file without a hash, because an edit to such a file would be invisible.**Status:** N changed components (no analysed file changed)at zero. If the component count is not zero at the same time, that is the analysis grouping the same code differently, and the comment says so in one sentence instead of presenting it as a change. Every review comment ends with<!-- codeboarding: platform_url=… changed=… analysed_files_changed=… head=… -->for readers that should not parse the prose or the diagram; the web platform reads the verdict from this line only. The status line's shape is unchanged.metadata.jsoncarriesanalysed_files_changed(a string, like every field there).This replaces relaying an engine flag from CodeBoarding #608, now closed. That flag compared a run against its own seed, so a re-run seeded from the pull request's previous head, a body-only edit and clustering noise each made it disagree with what the pull request did. The hash count compares against the merge base and works with the engine pinned today, so there is no engine release or pin bump to wait for.
Checked on real review artifacts with the installed engine:
analysed_files_changed=0for CodeBoarding #605 (docs) and CodeBoarding-webview #154 (agent guide), and8for CodeBoarding-webview #153 (dashboard code).What it looks like
Nothing visible changes in the webview. The comment on a docs-only pull request:
Tests
tests/test_diff_to_mermaid.py(4 new): the same code grouped differently changes no file; every edited, added or removed file counts; an analysis without a file index or with an unhashed file gives no count; the count rides on the script's machine-readable summary.tests/test_review_comment.py(5): the status line and platform link, the marker as the last line, the zero wording, a regrouped component count, and a changed analysed file getting no verdict even at zero components.tests/test_review_artifact_metadata.py(2): the count recorded as given, andunknownwhen the render step could not make it.test_sync_without_baseline_uses_configured_depth_directly, which fails onmaintoo on macOS bash 3.2 (${var,,}); CI's Linux bash has it. Black clean.🤖 Generated with Claude Code