Repository navigation
feat: keep the diagram on a baseline branch of its own - #142
Svilen-Stefanov wants to merge 1 commit 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. Base: caught up 4 commits from feat/ancestor-base-seed @a7675d5, 32 s · changes 9 s 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
sync_strategy: branch writes the analysis to an orphan branch, codeboarding/baseline by default (the new baseline_branch input), and never writes the target branch. Each sync adds one fast-forward commit with the same .codeboarding/ files plus .codeboarding/source.json, titled "chore(codeboarding): diagram of <branch> @<sha7>" with a CodeBoarding-Source trailer. A target that moved during analysis drops the result as today; a branch moved by a concurrent sync is built on once; a push refused while the tip did not move fails with the branch rule and the fix. Sync seeds from the branch tip; without one it seeds from a saved ancestor or runs full, and recreates the branch. Reviews read the branch after an exact artifact and a baseline committed at the merge base: a blobless fetch of its newest 100 commits, matched by trailer against the merge base's first-parent history. An entry for the merge base is saved; one for an ancestor is caught up and reported as ancestor. A base read from the branch is published under the merge base's name. Docs cover the layout, a deleted branch, and an importable ruleset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![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 👍 / 👎.
| 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 👍 / 👎.
| # 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 👍 / 👎.
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 👍 / 👎.
| 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 👍 / 👎.
| 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 👍 / 👎.
| 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 👍 / 👎.
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
| 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.
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: keep the diagram on 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 (separate webview PR); this PR only adds the capability. Existing repositories keep whatever strategy they use. Migrating between the two is deferred to https://github.com/CodeBoarding/CodeBoarding-webview/issues/199.
sync_strategy: branchand a newbaseline_branchinput (defaultcodeboarding/baseline). 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), titledchore(codeboarding): diagram of main @<sha7>with aCodeBoarding-Source: <sha>trailer. Engine output is never edited.deliver-sync.sh) builds the commit with a scratch index, never touches the target branch (no.gitattributeseither), and pushes fast-forward only, never forced. Races: a target that moved during analysis drops the result (as today); a baseline branch moved by a concurrent sync is built on once; a re-run for the same commit adds nothing. A push refused while the branch tip did not move fails with:GitHub refused the push to codeboarding/baseline, most likely because a branch rule protects it. Add the CodeBoarding app as a bypass actor for codeboarding/baseline in the repository's rulesets, or set sync_strategy: push.base_source=saved; an ancestor entry is caught up incrementally and reported asancestorwith its catch-up count.base_from_shais the source sha. A base read from the branch is published under the merge base's artifact name.branch; ignores pushes to the baseline branch itself.docs/COMMIT_STRATEGY.md"The baseline branch" (what lives where, how sync writes and reviews read it, both bounds, what a deleted branch means, private repos need a paid plan for rulesets);docs/baseline-branch-ruleset.jsonto import (blocks deletion and non-fast-forward oncodeboarding/baseline, CodeBoarding Review app id 4021464 as bypass actor).What it looks like
This PR changes nothing visible in the web platform by itself; the webview PR that reads the branch comes separately. On GitHub, the new branch's commits read:
How it was tested
tests/test_baseline_branch.pyagainst real local remotes: first sync creates an orphan withsource.jsonand the trailer and leavesmainalone; the second sync appends a fast-forward commit; 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; a review resolvessavedfrom an exact branch entry andancestor(catch-up 1) from an older one, from a depth-1 clone; a missing branch falls back to computed; sync continues from the branch tip; guard accepts the strategy and skips pushes to the branch.python -m unittest discover -s tests: 226 tests, all pass locally (macOS). Black 25.9.0 via the pre-commit hook; shellcheck on all action scripts.🤖 Generated with Claude Code