fix: stop discarding the health report - #82
Conversation
CodeBoarding reviewStatus: 0 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_Visual_Rendering_Engine -- "Returns rendering metadata and diagram artifacts" --> n_Interaction_Orchestrator
n_Structural_Diff_Engine -- "Provides annotated diff model for visualization" --> n_Visual_Rendering_Engine
n_Interaction_Orchestrator -- "Triggers structural comparison via CLI" --> 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;
|
![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: fa37db9220
ℹ️ 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 [ -f "$health_source" ]; then | ||
| mkdir -p "$output/health" | ||
| cp "$health_source" "$health_target" |
There was a problem hiding this comment.
When a previous baseline already contains a health report but the current engine run produces none, analyze_sync copies that old report into ANALYSIS_DIR via seed_state before analysis starts (scripts/action/analyze.sh:88). This condition therefore sees the seeded file as fresh and copies it back, making the promised stale-removal branch unreachable after the first report exists. Remove the report from the seeded state before invoking Core, or otherwise track whether this run actually emitted it.
Useful? React with 👍 / 👎.
| - `static_analysis.pkl` | ||
| - `static_analysis.sha` | ||
| - `codeboarding_version.json` when emitted by Core | ||
| - `health/health_report.json` |
There was a problem hiding this comment.
Once sync commits this newly listed file, the workflow example below still omits .codeboarding/health/health_report.json from paths-ignore. In the documented sync_strategy: pull_request setup, merging the rolling PR can therefore trigger another full sync job because the merge commit is not necessarily authored with the bot email handled by guard.sh; that redundant run consumes analysis time and LLM quota even though it should find no baseline change. Add the generated health path to the sample ignore list, as the repository's own sync workflow already does.
Useful? React with 👍 / 👎.
| elif [ -e "$health_target" ]; then | ||
| rm -f "$health_target" | ||
| fi | ||
| printf '%s\n' "$health_target" |
There was a problem hiding this comment.
Whenever this path is installed and staged, deliver-sync.sh:73-75 still computes files_written using find -maxdepth 1 and a whitelist of only the root-level artifacts. The public action output and sync summary therefore undercount persisted artifacts by one whenever a health report exists, despite the updated documentation classifying it as persisted analysis state. Extend that counter to include health/health_report.json.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20f60b8c6b
ℹ️ 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 engine writes this next to the analysis it came from. Ship it when present | ||
| # so a pull request's warnings are readable without rerunning the analysis. | ||
| HEALTH_REPORT="$(dirname "$ANALYSIS_PATH")/health/health_report.json" | ||
| [ ! -f "$HEALTH_REPORT" ] || cp "$HEALTH_REPORT" "${RUNNER_TEMP}/cb-review-artifact/health_report.json" |
There was a problem hiding this comment.
When an incremental review is seeded from the merge-base state or a previous PR-chain run and Core produces no health report, the seeded health/health_report.json remains beside ANALYSIS_PATH, so this line uploads old findings as though they describe the current head. This is fresh evidence beyond the previously reported sync issue: analyze_review copies cached state into head_state before running incremental analysis, and this newly added artifact path now exposes that stale file. Remove the seeded report before analysis or track whether the current run emitted it.
Useful? React with 👍 / 👎.
health_report.json rewrites a timestamp on every run, so the existing -I filter is what stops committing the health report from turning every sync into a commit. That was incidental before and load-bearing now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The engine runs health checks on every analysis, full or incremental, and writes health_report.json beside the graph it produced. Nothing kept it: sync deleted the file instead of installing the fresh one, and review never uploaded it. The only surviving copy was inside the Actions cache, which has no download API, so neither the extension nor the webview could read health information at all. It has been that way since the v2 rewrite in 474d40a, which removed the file as a v1 leftover without noticing that the engine still produced it. Sync now installs the report it just generated, and removes a stale one when a run produced none, matching how the other optional artifacts are handled. Review ships it in the artifact next to the analyses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a86668c to
e12d41c
Compare
|
/codeboarding |
CodeBoarding reviewStatus: 0 changed components See the full change in CodeBoarding. Compared against the merge base: this branch is 1 commit behind graph LR
n_Visual_Rendering_Engine["Visual Rendering Engine"]
n_Structural_Diff_Engine["Structural Diff Engine"]
n_Interaction_Orchestrator["Interaction Orchestrator"]
n_Visual_Rendering_Engine -- "Returns rendering metadata and diagram artifacts" --> n_Interaction_Orchestrator
n_Structural_Diff_Engine -- "Provides annotated diff model for visualization" --> n_Visual_Rendering_Engine
n_Interaction_Orchestrator -- "Triggers structural comparison via CLI" --> 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;
|
1 participant
Stacked on #81 so the diff stays clean; GitHub retargets it to
mainwhen #81 merges.The problem
The engine runs health checks on every analysis, incremental included, and writes the result beside the graph. From a live run on #81:
Nothing kept it. Sync deleted
.codeboarding/health/health_report.jsoninstead of installing the fresh one, and review never uploaded it. The only surviving copy was inside the Actions cache, which has no download API, so neither the extension nor the webview could read health information at all.This regressed in
474d40a(the v2 rewrite), which removed the file as a v1 leftover without noticing the engine still produces it. The plumbing already assumed it would come back:codeboarding-sync.ymlstill lists.codeboarding/health/health_report.jsonunderpaths-ignoreso committing it cannot loop the workflow.The fix
Sync installs the report it just generated, and removes a stale one when a run produced none — the same copy-or-remove rule the other optional artifacts already follow, so a repo whose analysis stops emitting health data doesn't keep a frozen report forever. The installed path is printed, which is what the delivery step stages; an installed file that isn't printed would be silently left out of the commit, so there's a test for exactly that.
Review ships it in the artifact beside
analysis.jsonandbase_analysis.json, when present.User configuration under
health/(health_config.json,.healthignore) was never touched and still isn't.Also here: one portability fix
install-sync.shusedmapfile, which needs bash 4, and macOS ships bash 3.2 — so the script aborted withmapfile: command not foundlocally and its test had been failing on every macOS machine. Replaced with a read loop. That's in its own commit (ae62df2), and it's what made it possible to verify the health change locally rather than only in CI.Verification
test_installs_core_manifest_and_preserves_user_configuration)._run_health_reportis called frompre_analysis, which bothgenerate_analysisandgenerate_analysis_incrementalinvoke.Worth deciding separately
The artifact carries only the head health report. If the webview wants "this PR introduced N new findings", the base report would need to ship too — the same trade we made for
base_analysis.json. Left out here to keep the fix scoped; say the word and it's a two-line follow-up.🤖 Generated with Claude Code