Repository navigation
feat(#160): implement the Source Control Manager - #166
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: redhat-et/ProtoBot/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Comment |
ReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
🤖 Finished Review · ✅ Success · Started 10:37 AM UTC · Completed 11:05 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $10.92 |
|
Round 1, the findings without a thread:
Also in 875297a, from a local review: the design names CI-skip tokens and the |
|
🤖 Review · Commit: |
|
Round 2. The branch is rebased onto
From my own check of the design against #34, ADR-0002, and #30, also fixed in cc8d61f:
|
875297a to
cc8d61f
Compare
|
🤖 Review · Commit: |
Add the source-control-manager Go module: the SCM core, the Drafting Table face as a dual-era MCP server over stdio, the approved-state read face as the approved-merge CLI subcommand, the same operations as CLI subcommands, the gh host adapter, and the renderer for commit messages and pull-request bodies. The golden fixture of the design replays against the built binary over MCP, with stubs for gh and ears-manager; the hosted face is out of scope. A CI job runs format, lint, vet, the tests, and the static build.
T2: the root README lists both Go components and the SCM install target, and links the SCM design. T3: architecture.md and the SCM design record the choice of Go and the MCP Go SDK. T4: the check for text that GitHub acts on folds Unicode spaces and drops format characters, and commit and publish also check the rendered subject, message, and title. T5: branch_init asks the upstream remote with git ls-remote for the initialization branch; redhat-et#34's Fetch row names that read. T6: publish refuses an empty title, a title over 256 characters, and a body over 65,536 characters before the push. Also from a local review: the design names CI-skip tokens and the refresh merge message among the refused text, a missing remote HEAD names git remote set-head, and the fixture tables list the new checks.
T1: commit maps artifact.digest_mismatch by record_id and project.store_digest_mismatch by its store_digests field to the artifact or store path, so the discard route never names project.yaml. T2: publish's list mode names every change-set path it cannot compare, a directory included, as an uncommitted change. T3: a create or update that fails after the push with an open host outcome reports mutation unknown and retry reconcile, and the details name the branch and the pull request. T4: the project walk refuses a .protobot that is a symbolic link or not a directory with PROJECT_UNREADABLE. T5: a push that fails in transport reports mutation unknown and retry reconcile. T6: a failed read after the refresh merge reports mutation unknown. T7: every fetch runs with an explicit refspec and an empty --refmap, so no configured refspec moves a local branch or a tag. T8: a digest mismatch in any structured store refuses the commit; only a registered artifact outside the file set stays a warning. T9: the fixture holds redhat-et#34's ninth negative check, a structured record changed outside ears-manager, and the ears-manager stub keeps store_digests. T10: redhat-et#30's change-set show row lists the records the change set touches, not directory registry entries. T12: the design says which store entries the store digest covers. T13: redhat-et#34's Stage row lists the structured records, and its Fetch row names remote-tracking refs. T14: the hosted rule covers the configured stores and the store digests. T15: the transcript's 4-check step carries the redhat-et#108 diagnostic shape, and the driver compares it. T16: the checks that only the Go driver asserts are in their own table. T17: the design orders publish's host-failure outcomes by what the host did. T18: a store mismatch names the untracked entries that the discard leaves. Also from the spec-doc check: refresh's failure after its merge commit, redhat-et#34's discard route and project-root exception, the reasons a push is rejected, and the redhat-et#108 diagnostic shape in redhat-et#33's fixture.
cc8d61f to
6a00754
Compare
|
Correction to the replies above: after they were posted, |
|
🤖 Review · Commit: |
JohnStrunk
left a comment
JohnStrunk
left a comment
There was a problem hiding this comment.
B1 is blocking: the SCM trusts remote-tracking refs that can remain stale after remote branch deletion, including the default branch used for publish, refresh, approval, and initialization.
T1: every fetch runs with --prune, so a branch the remote deleted leaves no remote-tracking ref behind, and repo_state, publish, refresh, approved_merge and branch_init read refs that the same call refreshed. Two fixture checks cover a deleted change-set branch and a deleted default branch. Also from the spec-doc check: publish's step 5 and the BASE_NOT_ON_DEFAULT row name a remote with no default branch, repo_state's default_branch field can be null, refresh and approved_merge say what they do in that state, redhat-et#34 says that a prune of remote-tracking refs is no write to a branch, and the stale-tracking-ref fixture row says why it now passes.
|
🤖 Review · Commit: |
T1: loading project.yaml applies the branch-namespace rules of the branch_init request, so a configured default_branch inside branch_prefix, or either field in the reserved wi/ namespace, is PROJECT_UNREADABLE. Without it, such a default branch reads as a change-set branch and the ref policy permits commit and publish to write it. Unit tests cover each refusal, and a fixture check rewrites project.yaml and calls commit and publish. Also from the spec-doc check: the request now refuses the bare branch wi and a prefix below wi/, as the loader does, so both paths hold the same rule; redhat-et#34's default_branch and branch_prefix rows state the namespace rules and name their enforcer; and the fixture check asserts the local and remote refs it claims.
|
🤖 Review · Commit: |
|
PR #166 (redhat-et/ProtoBot) implemented the Source Control Manager, closing issue #160, and merged successfully. The issue had been correctly triaged as blocked; once its blockers cleared, a human (lukaskellerstein) implemented it directly on a non-agent branch rather than the code agent being dispatched — no evidence this was a defect, just a human taking the work. The main finding: of ~6 review-dispatch events triggered by the PR's push history, only the very first automated review run completed (round 1, high-quality: it caught a genuine High-severity Unicode-whitespace bypass of the closing-keyword guard, plus valid stale-doc and stale-ref findings, and one Low finding the author reasonably overrode with a tracked follow-up in #167). Every subsequent review run was auto-cancelled (5 runs) or skipped (1 run) because the author pushed fix commits faster than the review agent could finish, and the This is a well-documented, actively open problem upstream, not a new gap: fullsend-ai/fullsend#1014 ("Debounce review dispatch on rapid synchronize events") is the canonical issue, with #4960, #7314, and #7107 as near-duplicates, and #7521 pinpointing the exact mechanical defect confirmed here — the concurrency group isn't keyed on commit SHA. Several prior "Evidence for #1014" issues already document the same pattern on other PRs (e.g. #3694, #5210, #4693, #3752, #4414, #4635, #5106). Given this density of existing coverage, I'm not filing a new proposal; this comment serves as fresh evidence for #1014: on PR #166, the missing debounce caused 5 review-run cancellations plus 1 skip across ~11 hours of fix cycles, and specifically let 2 confirmed security-relevant bugs go unreviewed by automation until human reviewers caught them. No other systemic issues were identified in this workflow. |
|
🤖 Finished Retro · ✅ Success · Started 2:40 PM UTC · Completed 2:49 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.19 |
3 participants
What changed
A new Go module,
source-control-manager/, implements the Source ControlManager that
docs/architecture/source-control-manager.mddefines.source-control-manager, with the SCM core as alibrary inside it. It is a static Go binary, like
ears-manager, so itadds no runtime to contributor machines (Environmental Constraints). The
MCP SDK is the official Go SDK, v1.8.0.
(
serve --face drafting-table):repo_state,branch_init,branch_resume,commit,publish, andrefresh, in that fixed order.A modern client gets revision 2026-07-28, and a client that opens with
initializegets 2025-11-25, with byte-identical results.approved-merge.hooks directory of the SCM's own,
core.fsmonitoroff, literalpathspecs, and no submodule recursion.
commitstages into a privateindex, compares every staged blob and mode with the content that
checksaw, writes the commit with
git commit-tree, and moves the branch witha compare-and-swap
git update-ref. Pushes run without force and withouttags. The remote and push-URL checks refuse userinfo and redirected
pushes.
gh, with--repoon every call andGH_REPOandGH_HOSTcleared.Change-Set:trailer, therefresh merge message, and the pull-request title and body. Record values
sit in code spans or fenced blocks. Closing keywords, mentions, and
CI-skip tokens are refused in every line the SCM writes outside code.
golangci-lint, vet, the tests, and the static build.docs/architecture.mdand the SCM design record the choice of Go andthe MCP Go SDK, and the design names every text that
UNSAFE_TEXTrefuses: closing keywords, mentions, CI-skip tokens, and a
pull-request title or body that GitHub would not take.
ears-manageris not implemented yetThe SCM reads the specification only through the
ears-managerCLI of #30.The
ears-managerbinary in this repository is the storage foundation of#115; its command set does not exist yet. The SCM needs:
change-set show(with--at)branch_initchange-set compare,impactpublish(the pull-request body)checkwith theartifact.digest_mismatchcodecommitproject init,projection.yamlentriescommitWithout them, every operation that reads
ears-managerfails closed withSPEC_TOOL_FAILED, and nothing changes.The tests use a stub that speaks #30's envelopes, as the design allows
("
ears-manager, or its recording stub with #30's golden envelopes"). Where#30 leaves a field name open, the SCM names what it reads, in
internal/ears/ears.go:change-set showdata:change_set(the manifest),status(
proposedorapproved),manifest_path, andpaths, the file set withthe manifest included.
check: status 4 withartifact.digest_mismatchdiagnostics that carrypath; status 5 for an incomplete or stale impact assessment.change-set compare:beforeandaftervalues for a revised artifact,so a content-only change updates the pull-request body (fixture row
scm-7).
.protobot/projection.yaml: apathslist ofpathandclassentries.Once the command set exists, the same replay runs against the real binary:
Today that run stops at step 1, because
project initanswers nothing.The run against the real binary is tracked in #167, blocked by #110,
#112, and #113.
Out of scope
The hosted face behind the Gate, as #160 states.
serve --transport streamable-httpexits non-zero and never listens. The 12fixture rows of the hosted face are skipped.
How it was tested
internal/goldenreplaysdocs/architecture/fixtures/source-control-manager-golden.jsonlagainstthe built binary: it starts
serve, calls the tools as an MCP client withno harness and no model, and compares every result, with
<sha:NAME>bound on first sight. It covers Define the Single-Player Git Integration #34's eight steps, Define the Single-Player Git Integration #34's eight negative
checks, and every SCM check except the hosted rows.
working tree, the remote, and the host unchanged.
one;
repo_stateis byte-stable; the trace context is copied; the CLI andMCP results are equal; planted hooks and
fsmonitordo not run, but dorun for an ordinary
git commit; a tag namedorigin/maincannot steerrefreshor the fast-forward; protected names in another letter case arerefused; no result holds the remote URL or the planted token.
validation, and the host classes.
go test ./...passes on Go 1.25.12 and 1.26.5.golangci-lintv2.13.2finds 0 issues.
gofmtand pre-commit are clean.for security, found 17 distinct problems; 15 are fixed, and the 2 left are
listed below.
Known limits
approved_mergereads at most the 1,000 newest merged pull requests.checkruns canget content that
checknever saw into the commit. The design acceptsthis under "one session per checkout"; the full fix is
check --at <new commit>before the ref moves.repo_statereturnsPROJECT_NOT_FOUND.materializerandreconcilerroles ofapproved-mergedo the sameread and are not configurable.
Protected path
.github/workflows/ci-workflow.yamlgains the SCM's CI job. #160 authorizesit: its acceptance criteria require the fixture to replay green, and CI is
where it replays. A maintainer must approve this change.
Closes #160