Repository navigation
feat: save the diagram to an analysis branch of its own - #142
Svilen-Stefanov wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
CodeBoarding reviewStatus: 0 changed components (no analysed file changed) 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;
|
2012a60 to
32fdc42
Compare
![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: db578a35b9
ℹ️ 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".
| elif [ "$BRANCH_DISTANCE" -eq 0 ]; then | ||
| REQUIRES_FULL=false base_source=saved base_from_sha="$BRANCH_SOURCE" catchup_commits=0 |
There was a problem hiding this comment.
When the baseline branch contains an entry for the merge base, this path accepts it after checking only depth_cap and skips the compatibility run. Baseline artifacts are otherwise keyed by CFG_HASH, which also covers the engine version, provider, endpoint, region, and model inputs; after an engine upgrade or when sync and review use different models, the review therefore compares an old-configuration base with a newly generated head and can report spurious architecture changes. Store and validate the configuration identity with each branch entry, or run the same compatibility check used for non-exact entries before treating it as saved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a65300. Each analysis-branch commit now carries a CodeBoarding-Config: <cfg hash> trailer (also in source.json), and an entry only counts when it matches this run's CFG_HASH, for the exact entry, ancestor entries and the sync tip alike. Entries found only under another configuration make the full run's reason "incompatible". With no CFG_HASH nothing on the branch is trusted, same as artifact reuse.
| if [ -n "$tip" ]; then | ||
| git fetch -q --depth=1 "$REMOTE" "refs/heads/$branch" | ||
| parent=(-p "$tip") |
There was a problem hiding this comment.
If baseline_branch already names any ordinary branch other than target_branch—for example a pre-existing codeboarding/baseline, develop, or the old codeboarding/sync branch—this unconditionally makes its tip the parent of a commit whose tree contains only .codeboarding/. The subsequent fast-forward push succeeds and makes every other file disappear from that branch, rather than creating the promised orphan branch. Verify that an existing tip is a recognized CodeBoarding baseline before parenting it, and fail without pushing when it is not.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a65300. Before building on an existing tip, delivery checks it has a CodeBoarding-Source trailer and that its tree is only .codeboarding/; otherwise it fails without pushing and says to pick an unused analysis_branch. Test: test_an_existing_branch_that_is_not_an_analysis_branch_is_never_written.
| # Never forced: the parent is the tip just read, so this only ever fast-forwards. | ||
| if git push -q "$REMOTE" "$commit:refs/heads/$branch"; then |
There was a problem hiding this comment.
The target branch is checked by a separate fetch at the start of the loop, but this push updates only the baseline ref. If target_branch advances after that fetch and before this push, the push still succeeds and publishes an analysis of the old BASE_SHA as the latest baseline, contrary to the stale-result guarantee; the queued newer run may not repair it if that run later fails or is cancelled. The expected target SHA needs to be validated in the same remote transaction as the baseline update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this one as is, now documented in COMMIT_STRATEGY.md. Git can't make the push conditional on another ref without also updating it, and the outcome is benign: the entry's trailer still names the older commit it describes, so no reader takes it for newer (reviews match by source sha), and the run queued for the newer commit replaces it. If that run fails, the next push fixes it.
db578a3 to
f5c9e48
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5c9e48f22
ℹ️ 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".
| "rules": [ | ||
| { "type": "deletion" }, | ||
| { "type": "non_fast_forward" } |
There was a problem hiding this comment.
In repositories where contributors may push branches but the main branch requires reviewed PRs, this ruleset still lets any contributor create or fast-forward codeboarding/baseline; these rules block only deletion and non-fast-forward updates. Review and sync runs later pass that branch's .codeboarding/static_analysis.pkl through restore_baseline to the engine, which loads pickle state in a privileged workflow, so a contributor can bypass code review with a crafted baseline commit. Add creation and update restrictions, leaving only the intended app/token as a bypass actor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a65300 (now docs/analysis-branch-ruleset.json): it adds creation and update rules, so only the bypass actor can write. The bypass is GitHub Actions (15368), since that's what the default github.token pushes as, and the docs explain the pickle risk and say to make a GitHub App the only bypass actor when sync pushes with one, since any workflow can use github.token.
| mkdir -p "$scratch" "$state" | ||
| git -C "$CHECKOUT_DIR" archive "$commit" .codeboarding | tar -x -C "$scratch" || return 1 | ||
| rm -f "$scratch/.codeboarding/source.json" | ||
| cp -a "$scratch/.codeboarding/." "$state/" |
There was a problem hiding this comment.
When a repository switches from push to branch, $state is initially seeded with the old committed .codeboarding directory, and this overlay leaves any generated files absent from the branch snapshot untouched. Optional artifacts such as static_analysis.sha, codeboarding_version.json, or a health report can therefore come from the old code-branch baseline while analysis.json and the pickle come from the baseline branch, and that mixed state is passed to incremental analysis. Clear the generated state before copying the snapshot, then restore only the current checkout's user configuration as seed_from_ancestor does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| tip="$(git ls-remote "$REMOTE" "refs/heads/$branch" | awk '{print $1; exit}')" | ||
| parent=() | ||
| if [ -n "$tip" ]; then | ||
| git fetch -q --depth=1 "$REMOTE" "refs/heads/$branch" |
There was a problem hiding this comment.
If another sync advances the baseline branch after ls-remote records tip but before this depth-one fetch, the fetch downloads the new tip while parent still names the old one. When that old commit is not already in the checkout—such as with force_full, or when the branch had already moved during analysis—git commit-tree -p "$tip" fails before the push and the advertised retry logic can run. git fetch -h describes --depth as producing shallow history, so derive tip from the fetched FETCH_HEAD or fetch the recorded SHA explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a65300. ls-remote is only used to tell whether the branch exists; the parent is now FETCH_HEAD from the depth-1 fetch.
| if [ "$SYNC_STRATEGY" = branch ]; then | ||
| [ -n "${BASELINE_BRANCH:-}" ] || fail "baseline_branch is required with sync_strategy: branch." | ||
| # The baseline branch holds only analysis; a workflow that also fires on it must not analyze it. | ||
| [ "$REF_NAME" != "$BASELINE_BRANCH" ] || skip "Ignoring a push to the baseline branch $BASELINE_BRANCH." | ||
| [ "$target_branch" != "$BASELINE_BRANCH" ] || fail "target_branch must differ from baseline_branch." |
There was a problem hiding this comment.
If baseline_branch contains an invalid Git refname, such as one with .., a space, or a trailing dot, this guard accepts it because it checks only that the value is nonempty. The analysis lookup then silently treats the failed fetch as a missing baseline and can perform a full paid analysis, after which delivery fails with an invalid refspec and misreports the unchanged empty tip as a branch-rule rejection. Validate refs/heads/$BASELINE_BRANCH with Git's refname validator in the guard so this configuration fails before checkout and analysis.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a65300. guard.sh runs git check-ref-format refs/heads/$ANALYSIS_BRANCH in both modes and fails before checkout. Test: test_a_branch_name_git_cannot_use_fails_before_anything_runs.
ivanmilevtues
left a comment
ivanmilevtues
left a comment
There was a problem hiding this comment.
I mentally looked over the thing here. Looked through the logic and it makes sense and looks fine. no majore issues found from prompting just wording again + takea look at the codex comments. (there are some P1s)
| description: 'Sync delivery method: push, pull_request, or branch (an orphan branch of its own, see baseline_branch).' | ||
| required: false | ||
| default: 'push' | ||
| baseline_branch: |
There was a problem hiding this comment.
maybe the word baseline is not very intuitive for our users as we call this syncing, so I suppose maybe we should name it syncing_branch
There was a problem hiding this comment.
Agreed on dropping "baseline". Renamed in 7a65300 to analysis_branch, default codeboarding/analysis. I avoided sync_branch because sync_strategy: pull_request already pushes to a branch called codeboarding/sync, so the two would be easy to mix up. analysis also matches .codeboarding/analysis.json. The webview side (#202) follows the same names.
| with: | ||
| mode: sync | ||
| llm: hosted | ||
| target_branch: main |
There was a problem hiding this comment.
target_branch, is this the target in tehms of where the sync will happen or in terms of which branch we will sync with, unsure that wording is clear again.
i think that most of these things will be read by ppl or even more by their agents so proly descriptive and somewhat clear names are worth investing in.
There was a problem hiding this comment.
Good catch, it was ambiguous, and under branch the old description ("Branch updated by sync mode") was just wrong. I kept the name since it's an existing v1 input and renaming would break every current workflow, but the description now says it's the code branch sync analyzes: with push / pull_request the analysis is also committed to it, with branch it's only read. Same wording in the README table and the new section.
sync_strategy: branch saves the analysis to an orphan branch in the same repository, codeboarding/analysis unless analysis_branch names another, one fast-forward commit per sync, and never writes to the target branch. Each commit carries CodeBoarding-Source and CodeBoarding-Config trailers, also recorded in .codeboarding/source.json. Reviews read their base from the branch after an exact artifact and a baseline committed at the merge base: an entry for the merge base is reused, an entry for an ancestor is caught up. Only entries made under this run's configuration count, so a different engine version or model is never reused as is. Sync continues from the tip under the same rule, replacing the generated state wholesale and keeping only the checkout's user config. Delivery refuses an existing branch that is not an analysis branch (no trailer, or anything besides .codeboarding/), builds on the tip it fetched rather than the one ls-remote saw, and the guard rejects an analysis_branch git cannot use before anything runs. The importable ruleset now restricts creating and updating the branch to its bypass actor, since sync and review load a pickle from it. target_branch's description now says it is the code branch sync analyzes, written only by push and pull_request. The README carries a prompt for moving an existing setup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
32fdc42 to
3ca8dc1
Compare
f5c9e48 to
7a65300
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Svilen-Stefanov
changed the title
…emetry target_branch becomes synced_branch, sync_strategy becomes save_baseline_to (synced_branch | pull_request | baseline_branch), and the unreleased analysis_branch becomes baseline_branch with default codeboarding/baseline. The released names keep working through old_api_migrator.sh, the only reader of deprecated inputs. The base_analysis_method output and the review comment's base line and timers are removed. Engine runs carry CODEBOARDING_RUN_ID tagged base, head or sync, so the engine's own telemetry shows how each base was obtained. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 608b37dd48
ℹ️ 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".
| # save_baseline_to has a default, so only a value other than it is a conflict. | ||
| [ "$save_baseline_to" = synced_branch ] || fail "Set save_baseline_to only; sync_strategy is its deprecated name." |
There was a problem hiding this comment.
When a migrating workflow explicitly sets save_baseline_to: synced_branch but accidentally retains sync_strategy: pull_request, both inputs are set, yet this condition treats the new value as merely its default and silently lets the deprecated input overwrite it. The run then opens a rolling PR instead of committing to the explicitly requested branch, contrary to the documented guarantee that setting both names fails. Use an empty action-input default and apply synced_branch only after conflict detection, or otherwise preserve whether the new input was explicitly supplied.
Useful? React with 👍 / 👎.
2 participants
Stacked on #140 (
feat/ancestor-base-seed), which is stacked on #139. Review and merge those first.What changed and why
Section 5 of the base-provenance contract: save the diagram to a branch of its own, so a team gets a saved diagram without committing generated files to its code branch. New setups will default to it through the webview's setup template (CodeBoarding-webview#202); this PR only adds the capability. Existing repositories keep whatever strategy they use; the README has a prompt to paste into a coding agent to move one.
sync_strategy: branchand a newanalysis_branchinput (defaultcodeboarding/analysis). The first sync creates the branch as an orphan. Each sync adds one commit with the same.codeboarding/files sync commits today plus.codeboarding/source.json(schema,source_branch,source_sha,generated_at,engine_version,config), titledchore(codeboarding): diagram of main @<sha7>withCodeBoarding-Source: <sha>andCodeBoarding-Config: <cfg hash>trailers. Engine output is never edited.target_branchkeeps its name (renaming it would break every v1 workflow), but its description now says what it is: the code branch sync analyzes, whichpushandpull_requestalso write to andbranchonly reads.deliver-sync.sh) builds the commit with a scratch index, never touches the target branch (no.gitattributeseither), and pushes fast-forward only, never forced, onto the tip it fetched. It refuses an existing branch that is not an analysis branch (tip without the trailer, or holding anything besides.codeboarding/), so pointinganalysis_branchat a code branch fails instead of emptying it. Races: a target that moved during analysis drops the result (as today); an analysis branch moved by a concurrent sync is built on once; a re-run for the same commit adds nothing. A push refused while the tip did not move fails with a message naming the bypass-actor fix..codeboardingignoreand health config, then runs incrementally. Otherwise it falls through to the ancestor-artifact lookup from feat: catch up the review base from the nearest saved ancestor #140, then full, and delivery recreates the branch.CodeBoarding-Configmatches this run's configuration hash count. An exact entry isreusedwith no engine run; an ancestor entry is caught up and reportedincremental. Entries found only under another configuration make a full run's reason "incompatible". A base read from the branch is published under the merge base's artifact name.branch; ignores pushes to the analysis branch itself; rejects ananalysis_branchthatgit check-ref-formatrefuses, in both modes, before anything is analyzed.docs/COMMIT_STRATEGY.md"The analysis branch" (what lives where, how sync writes and reviews read it, the configuration rule, the non-atomic target check, what a deleted branch means, why to protect it);docs/analysis-branch-ruleset.jsonto import. It blocks creating, updating, deleting and force-pushingcodeboarding/analysisfor everyone but GitHub Actions (integration 15368, what the defaultgithub.tokenpushes as); the docs say to make a GitHub App the only bypass actor when sync pushes with one.Since the first review
baseline_branch→analysis_branch,codeboarding/baseline→codeboarding/analysis;target_branchdescribed rather than renamed.base_analysis_method/base_analysis_reason.What it looks like
This PR changes nothing visible in the web platform by itself; CodeBoarding-webview#202 reads the branch. On GitHub, the new branch's commits read:
How it was tested
tests/test_analysis_branch.pyagainst real local remotes:source.json(includingconfig) and both trailers, and leavesmainalone. The second sync appends a fast-forward commit, and a same-commit re-run adds nothing. A pre-receive hook refusing the branch produces the plain message. A deleted branch is recreated as a new orphan. A target that moved keeps the branch unchanged. An existing code branch under the analysis branch's name is refused and left untouched.reusedand an older one isincremental(1 commit caught up), from a depth-1 clone. An entry under another configuration is never reused (full, incompatible), and without a configuration hash no entry is trusted. A missing branch falls back tofull.pushreplaces the old committed state but keeps.codeboardingignore; a tip under another configuration is not continued from.target_branchequal to the analysis branch.python -m unittest discover -s tests: 239 tests, all pass locally (macOS). Black 25.9.0 via the pre-commit hook; shellcheck on all action scripts.🤖 Generated with Claude Code