docs(#34): define the Git and project-repository integration - #87
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Comment |
|
Risk Assessment: moderate (2/5) DetailsDocumentation-only PR with a large new spec file (927 lines) and minor updates to 7 existing docs; no security, CI, or dependency changes; high churn and fix history on existing files is typical for iterative spec refinement; issue scope aligns well with PR content and all acceptance criteria appear addressed. Previous runRisk Assessment: moderate (2/5) DetailsDocumentation-only PR with a large new spec file (927 lines) and minor updates to 7 existing docs; no security, CI, or dependency changes; high churn and fix history on existing files is typical for iterative spec refinement; issue scope aligns well with PR content and all acceptance criteria appear addressed. Previous run (2)Risk Assessment: moderate (2/5) DetailsDocumentation-only PR with a large new spec file (922 lines) and minor updates to 4 existing docs; no security, CI, or dependency changes; high churn and fix history on existing files is typical for iterative spec refinement; issue scope aligns well with PR content. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 10:53 AM UTC · Completed 11:14 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.87 |
|
Round 1 answered in T1, T9, Related Documents in three siblings — fixed in The five Low findings that were fixed: the header blockquote is trimmed to the shape One Low finding is not changed: editing the sentence in ADR-0001 that named the open question is what the house |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 11:51 AM UTC · Completed 12:33 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
|
Round 2 answered in T1, Fixed this round: That last one reverses my round 1 answer. The suggestion in this round was better than the inline rewrite I defended, so I took it. |
|
🤖 Finished Review · ✅ Success · Started 1:15 PM UTC · Completed 1:36 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.64 |
lukaskellerstein
added
fullsend-no-fix
JohnStrunk
left a comment
JohnStrunk
left a comment
There was a problem hiding this comment.
Overall, I like this. Only a few inconsistencies that I saw.
| @@ -0,0 +1,927 @@ | |||
| # ProtoBot: Git and Project-Repository Integration | |||
|
|
|||
| > Design document — draft, September 2026 | |||
There was a problem hiding this comment.
This should not be marked as a draft. If/when the PR merges it, it will be accepted.
There was a problem hiding this comment.
Fixed in 4714d2a: the header now reads "Design document — September 2026".
I left the word in the five sibling documents, since changing those is a separate call. Say if you want them in this PR.
|
|
||
| ### Selecting the paths | ||
|
|
||
| `protobot new` (the session-start command in the pending #28 |
There was a problem hiding this comment.
protobot is not a command, so there can be no protobot new. This is probably an ears-manager command of some sort.
There was a problem hiding this comment.
Fixed in c1fc52a. The sentence no longer names a command. "Selecting the
paths" now says the write goes through the ears-manager initialization
operation that "Project initialization" records as a dependency on #30.
The name came from docs/architecture/user-interaction-flow.md:102. Its
interface-type taxonomy gave protobot generate as the example for a CLI
interface, and that was the only place a protobot executable appeared in
the docs. The same commit changes that cell to "Build tool, code
generator". Every other example in that table names a kind of interface
rather than a command, so the row now matches its siblings.
Add docs/architecture/git-integration.md, the contract for turning governed specification changes into reviewable Git history. The document covers: - project identification through .protobot/project.yaml, including the canonical remote, the default branch and the declared review mode; - artifact-path selection, with one change set as one manifest file; - change-set branches named cs/<nnn>-<slug>, cut when change-set create runs and deleted after the merge; - commit content, message and trailer, and the rule against rewriting pushed history; - pull-request preparation, with the body rendered from change-set compare and impact; - approved specification state and registration in both modes; - a four-layer defence against ungoverned edits; - an allowlist of permitted Git operations; - IdeaBot material as input content only, never a dependency; - a failure table with a deterministic diagnostic and a safe retry per condition; and - a bare-repository fixture as the test plan for redhat-et#75. Every change reaches the default branch through a pull request, in every mode. Single-player differs only in that the author merges their own and no reviewer is required. This settles the conflict between overview.md, which allowed a direct push to main, and architecture.md, which made branch protection mandatory, and narrows the single-player wording in overview.md and components.md. ears-manager writes the projection class for a registered specification path. That closes the gap where the remedy for an unclassified path named a file the contract could not commit. Also links the new document from architecture.md, components.md and overview.md, marks the change-set half of the branch-naming open question resolved, points ADR-0001's deferred pull-request question at the answer, and fixes a broken Q13 anchor in architecture.md.
Run the AGENTS.md coverage check over the whole hierarchy against docs/architecture/git-integration.md. No inventory item is MISSING. Three fixes came out of it. - The document said ADR-0001 "leaves open" who posts the rendered pull-request summary. The previous commit already changed ADR-0001 to state the decision, so the tense read as a contradiction between the two files. It now says "left open" and points at the record. - Add the pull-request rule to the table of contents. It is the document's most load-bearing decision and it was not listed. - Register the manifest file name in the control-namespace table in components.md, so a reader of that file alone learns that a change set is one flat file named cs-<nnn>.yaml.
Seven of the nine bot findings are fixed. Two are answered in their threads without a change. - T2 Trim the header blockquote to the shape architecture.md uses. The long form repeated the Purpose and scope section. - T3, T4 "When the branch is created" now names the initialization exception, where the branch exists before change-set create, and says that change-set create records base_commit either way. - T5 The initialization commit holds three files, not two. projection.yaml carries the shared class of each registered path. Fixture step 1 expects it. - T6 repository.branch_prefix may not be wi/ or another Job Site namespace, and ears-manager check rejects one that is. - T7 An intent with no alphanumeric character falls back to the slug change-set. - T9 open-questions.md, user-interaction-flow.md and related-work.md now list the new sibling in Related Documents, so every hierarchy document points at it. Not changed: - T1 AGENTS.md, and its CLAUDE.md symlink, is a protected path. The brief for redhat-et#34 says not to edit it, and the PR body asks the maintainers whether contract documents belong in that list. - T8 The adr skill prescribes updating the sentence in an ADR that names a gap and linking the decision. The original wording is kept in the past tense.
Four of the five bot findings are fixed. One is answered again without a change. - T2 branch_prefix names wi/ as the only reserved prefix today. Round 1 added "or any other Job Site namespace", which no document enumerates, so ears-manager check could not evaluate it. A further reserved prefix has to be recorded in the Content Storage Model first. - T3 The dependency on redhat-et#30 is stated as a dependency, not as an instruction to that contract. The substance is unchanged. - T4 The initial Sketch records the merge commit that landed CS-001 on the default branch as base_commit. "The initialization commit" could have meant the commit on the init branch, which is a different hash from the one fixture step 2 asserts. - T5 ADR-0001 keeps its original sentence word for word, and the decision follows as a dated amendment. This reverses the round 1 answer: the bot's new suggestion is better than the inline rewrite, because the record stays readable as a record and the reader still learns the outcome. Not changed: - T1 AGENTS.md, and its CLAUDE.md symlink, is a protected path. The brief for redhat-et#34 says not to edit it, and the PR body asks the maintainers whether contract documents belong in that list. Same answer as round 1.
JohnStrunk requested changes with three findings. All three are fixed, plus two bot findings that describe undefined behavior rather than style. From the human review: - T1 The header no longer says draft. Merging the PR is what makes the document accepted. The five sibling documents still say it; changing those is a separate call. - T3 The document named no command that exists. There is no protobot executable, so the sentence now says the Drafting Table proposes the default layout at initialization and the user confirms it. The name came from the pending redhat-et#28 document, which uses protobot new and protobot resume in its session diagram, so PR redhat-et#81 needs the same correction; that is already open there as finding A5. - T4 The requirement store defaults to .protobot/requirements/, which keeps the files ears-manager manages together. components.md records the default next to the control namespace table, since the path sits inside .protobot/ and the table does not list it. The store is still a registry entry, so a project may point it elsewhere. From the bot review, fixed rather than deferred under the round 3 policy, because both leave behavior undefined and the push was happening anyway: - T5 The default-branch head is read from the local ref, after a fetch from the canonical remote. The two readings gave different hashes, and base_commit anchors the rest of the contract. - T6 The slug is cut at the last hyphen before position 40, or at exactly 40 characters when no hyphen precedes it. Every intent now yields one branch name. Not changed: - T2 The header-length finding names architecture.md as precedent itself and asks for nothing. - T7 The components.md row is a one-line summary that links to the full field table. Round 3: style only. - T8 AGENTS.md, and its CLAUDE.md symlink, is a protected path. The brief for redhat-et#34 says not to edit it, and the PR body asks the maintainers. Rebased onto upstream/main, 23 commits, no conflicts.
|
Round 3 answered in @JohnStrunk all three of your findings are fixed, each answered in its own thread. Two of them were factual errors rather than wording, so thank you for catching them. One open question for you is in the first thread: five sibling documents still say "draft" in their header, and I changed only this one. The review bot's findings for this round have no inline threads, so they are answered here. Fixed, although the round 3 policy would have deferred them. Both describe behavior the document left undefined, not style, and the push was happening anyway for the human review:
Not changed:
|
d51b872 to
4714d2a
Compare
|
🤖 Review · Commit: |
JohnStrunk's T3 said that protobot is not a command. The round 3 fix answered it in the text and named the pending redhat-et#28 document as the source of the name. Both parts were wrong. - git-integration.md no longer claims that no protobot executable exists. "Selecting the paths" now says the write goes through the ears-manager initialization operation that "Project initialization" records as a dependency on redhat-et#30. - user-interaction-flow.md gave `protobot generate` as the example for a CLI interface. That was the only place a protobot executable appeared in the docs, and it is where the name came from. The cell now reads "Build tool, code generator". Every other example in that table names a kind of interface in plain text rather than a command, so the row now matches its siblings. The redhat-et#28 draft on PR redhat-et#81 no longer uses protobot new or protobot resume, so the round 3 claim about it does not hold. Finding A5 there is marked outdated.
|
🤖 Review · Commit: |
|
PR #87 (docs(#34): git-integration.md, human-authored branch Proposals filed
|
|
🤖 Finished Retro · ✅ Success · Started 5:53 PM UTC · Completed 6:06 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.37 |
2 participants
What
Adds
docs/architecture/git-integration.md, the contract for issue #34:how the Drafting Table turns governed specification changes into
reviewable Git history.
It covers project identification, artifact-path selection, change-set
branches, commit content, pull-request preparation, approved
specification state, the permitted Git operations, ungoverned-edit
detection, IdeaBot seeding, failure behavior, and a bare-repository
fixture that #75 executes as its test plan.
Decisions worth a reviewer's attention
every mode. Single-player differs only in that the author merges
their own and no reviewer is required.
overview.mdandarchitecture.mddisagreed here: one allowed a direct push to main,the other made branch protection a mandatory enforcement layer. This
PR settles it, and narrows the single-player wording in
overview.mdand
components.md. Those two sentences come from the approvedSketch in Create ProtoBot's Initial Sketch: Vision and Architecture #26, so please confirm the change is acceptable.
ears-managerwrites the projection class for a registeredspecification path. No document assigned an owner for
.protobot/projection.yaml, so the remedy for an unclassified pathnamed a file that no component could commit. Every other entry in
that file stays reviewed project policy.
ears-manager change-set compareandimpact. ADR-0001 left openwhether CI or a Drafting Table action posts that summary. The ADR now
records the answer.
cs-<nnn>.yaml, in a flat.protobot/change-sets/folder. The manifest holds references, notcontent, so it stays small when the change is large.
cs/<nnn>-<slug>, cut whenchange-set createruns and deleted after the merge. This resolves thechange-set half of the open question "Branch naming and lifecycle" in
components.md. Thewi/half stays open and belongs to the JobSite.
A requirement recorded against #30
No
ears-managersubcommand writes.protobot/project.yamltoday, andneither subcommand table lists one. The document records that #30 must
define a project initialization operation, and says that fixture step 1
has no command to run until it exists.
Question for the maintainers
AGENTS.mdlists the specification-document hierarchy and is a protectedpath, so this PR does not touch it. Should contract documents like this
one and
drafting-table-ux.md(#28, PR #81) be added to that list?How it was tested
/spec-doccoverage check ran in write mode over the wholehierarchy. No inventory item is
MISSING: 7 components, 12interfaces, 13 constraints, 6 stores, 3 topologies and 8 security
rules, each with its source anchor. Seven hierarchy files and both
decision records were read end to end. One consistency mismatch was
found and fixed in the second commit.
links in every file this PR edits.
pre-commit run --all-filespasses.skillsawwas skipped locally,because it does not follow the skill symlinks inside a worktree. CI
runs it on the real tree.
Relationship to #28
PR #81 is still open. This document cites it as pending and does not
link to
drafting-table-ux.md, because that file is not onmainyet.Its authoritative mutation-ownership table names #34 for commit and
pull-request mechanics, which this document supplies. Its representative
transcript shows a change-set branch without the sequence number; this
document states that its own naming convention is the authoritative one,
so the two can be reconciled when #81 merges.
Closes #34