test/docs: add canonical repo fixture builder and review hygiene#317
test/docs: add canonical repo fixture builder and review hygiene#317flyingrobots wants to merge 2 commits intomainfrom
Conversation
|
@coderabbitai follow-up plan: broader suite migration onto the canonical repo fixture builder is intentionally tracked in #316. This PR closes #311 and #315 by establishing the shared fixture substrate and codifying review hygiene; it does not try to migrate every existing repo-shaped suite in one shot. |
Summary by CodeRabbit
WalkthroughThis PR codifies PR review hygiene workflows across documentation and introduces a comprehensive repository fixture builder for tests. Review hygiene steps (resolving stale threads, documenting false positives) are now explicit pre-merge requirements. The git-warp upgrade (issue Changes
Sequence DiagramsequenceDiagram
actor Test as Test Suite
participant Builder as RepoFixtureBuilder
participant Fixture as RepoFixture
participant Base as Base Factory
participant Steps as Accumulated Steps
participant Overlay as Overlay Factory
Test->>Builder: repoFixture('name')
Builder->>Builder: new RepoFixtureBuilder(name)
Test->>Builder: .base(minimalDocsAndCodeBase)
Builder->>Builder: store base factory
Test->>Builder: .withFile(path, content)
Builder->>Steps: accumulate file write
Test->>Builder: .commit(message)
Builder->>Steps: accumulate commit
Test->>Builder: .overlay(withFeatureBranchOverlay)
Builder->>Builder: store overlay factory
Test->>Builder: .build()
Builder->>Fixture: create temp repo
Builder->>Fixture: init()
Fixture->>Fixture: git init + config
Builder->>Base: apply base factory
Base->>Fixture: writeFiles() + commit()
Builder->>Steps: apply accumulated steps
Steps->>Fixture: write, commit, remove
Builder->>Overlay: apply overlay factory
Overlay->>Fixture: branch, write, commit, merge
Builder-->>Test: return ready Fixture
Test->>Fixture: git commands, assertions
Test->>Fixture: cleanup()
Fixture->>Fixture: rm -rf temp
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes — Rationale: Heterogeneous changes spanning documentation (8 files, consistent patterns) and substantial test infrastructure (3 new helper modules totaling 379 LOC with public classes/methods, moderate logic density). Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Tip For best results, initiate chat on the files or code changes.
I'll record the migration strategy so future reviews don't incorrectly flag un-migrated suites as gaps in this PR. ✏️ Learnings added
🧠 Learnings used |
![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: 567c5250cf
ℹ️ 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".
| * @param {string} message | ||
| */ | ||
| async merge(ref, message) { | ||
| runGit(this.root, ['merge', '--no-ff', '-m', message, ref]); |
There was a problem hiding this comment.
RepoFixture.commit() intentionally fixes GIT_AUTHOR_DATE/GIT_COMMITTER_DATE, but RepoFixture.merge() runs git merge without those env vars (and does not advance the fixture clock), so any fixture using withFeatureBranchOverlay() gets wall-clock-dependent merge SHAs/timestamps. That makes fixture history non-reproducible across runs and can skew commit-order assumptions once a later deterministic commit has an older timestamp than the merge.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 48-50: Add a checklist item to the PR template to mechanically
enforce the new hygiene rule: update .github/pull_request_template.md to include
an explicit checkbox such as "Resolve stale review threads and document any
false positives" (or similar wording) in the pre-merge checklist so that the
CONTRIBUTING.md requirement (lines 48–50) is validated during reviews; ensure
the new item appears near existing checklist entries so reviewers must address
stale threads and false-positive documentation before merging.
In `@test/context.test.js`:
- Around line 24-26: The afterEach teardown should guard against repo being
undefined if beforeEach failed: in the afterEach async callback (the one calling
repo.cleanup()), check that the repo variable exists before invoking cleanup
(e.g., if repo) or use optional chaining to call cleanup only when repo is set;
ensure the same symbol names (repo and the afterEach callback) are used so
teardown won't throw a follow-on error hiding the original beforeEach failure.
In `@test/helpers/repo-fixture.js`:
- Around line 51-53: The resolve(relativePath) helper (and callers write() and
remove()) currently uses join(this.root, relativePath) which permits ../
traversal; change them to compute an absolute resolved path (using path.resolve
or path.resolve(this.root, relativePath)) and then validate that the resolved
path stays inside the fixture root (e.g., using path.relative(this.root,
resolvedPath) and rejecting if it starts with '..' or otherwise falls outside,
accounting for equality with this.root); if validation fails, throw an error and
do not perform the write/remove. Update resolve(), write(), and remove() to use
this validation check before touching the filesystem.
- Around line 159-161: The merge() helper is creating merge commits with
wall-clock metadata, breaking deterministic history; modify merge(ref, message)
to runGit with the same deterministic commit timestamp used by commit() (set
GIT_COMMITTER_DATE and GIT_AUTHOR_DATE to the deterministic value the fixture
uses) when invoking runGit(this.root, ['merge', '--no-ff', '-m', message, ref])
so the resulting merge commit metadata matches commit() and currentSha() remains
deterministic.
- Around line 239-257: The build() method currently leaks the temporary
directory created by mkdtemp when this.#base, any this.#steps, or this.#overlays
throws; wrap the sequence that initializes RepoFixture and then runs
this.#base/this.#steps/this.#overlays in a try/catch/finally (or try with
finally) so that if an error occurs you remove the created temporary root (the
value returned by mkdtemp) before rethrowing; reference sanitizeName, mkdtemp,
RepoFixture, and the this.#base / this.#steps / this.#overlays calls to locate
where to add the cleanup (use fs.rm or fs.promises.rm with recursive/force or an
equivalent cleanup helper) and ensure you only attempt cleanup if root was
created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35bc7b93-9f20-48a1-bedc-6c3362c4dbfb
📒 Files selected for processing (13)
AGENTS.mdCLAUDE.mdCONTRIBUTING.mdREADME.mdROADMAP.mddocs/README.mddocs/adr/ADR-0006.mddocs/design/git-warp-upgrade-audit.mdtest/context.test.jstest/helpers/repo-bases.jstest/helpers/repo-fixture.jstest/helpers/repo-overlays.jstest/repo-fixture.test.js
| 7. open the PR and process review feedback | ||
| 8. resolve stale review threads and document false positives before merge | ||
| 9. land it, then capture review-cycle learnings back into the backlog |
There was a problem hiding this comment.
New hygiene rule is documented but not mechanically enforced.
Lines 48–50 add the requirement, but the current .github/pull_request_template.md checklist still has no explicit item for stale-thread cleanup / false-positive documentation. This leaves compliance as “best effort” instead of a review gate.
🔧 Proposed enforcement update (PR template)
--- a/.github/pull_request_template.md
+++ b/.github/pull_request_template.md
@@
## Scope Control
- [ ] PR is single-purpose/cohesive (no unrelated refactors).
- [ ] Any non-essential refactor is split into separate PR(s) or explicitly justified.
+- [ ] All stale review threads are resolved before merge.
+- [ ] Any false-positive review findings are explicitly documented and linked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` around lines 48 - 50, Add a checklist item to the PR
template to mechanically enforce the new hygiene rule: update
.github/pull_request_template.md to include an explicit checkbox such as
"Resolve stale review threads and document any false positives" (or similar
wording) in the pre-merge checklist so that the CONTRIBUTING.md requirement
(lines 48–50) is validated during reviews; ensure the new item appears near
existing checklist entries so reviewers must address stale threads and
false-positive documentation before merging.
| afterEach(async () => { | ||
| await rm(tempDir, { recursive: true, force: true }); | ||
| await repo.cleanup(); | ||
| }); |
There was a problem hiding this comment.
Guard teardown when setup fails.
If beforeEach fails before repo assignment, teardown throws a follow-on error and hides the root failure.
Suggested fix
afterEach(async () => {
- await repo.cleanup();
+ if (repo) {
+ await repo.cleanup();
+ repo = undefined;
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterEach(async () => { | |
| await rm(tempDir, { recursive: true, force: true }); | |
| await repo.cleanup(); | |
| }); | |
| afterEach(async () => { | |
| if (repo) { | |
| await repo.cleanup(); | |
| repo = undefined; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/context.test.js` around lines 24 - 26, The afterEach teardown should
guard against repo being undefined if beforeEach failed: in the afterEach async
callback (the one calling repo.cleanup()), check that the repo variable exists
before invoking cleanup (e.g., if repo) or use optional chaining to call cleanup
only when repo is set; ensure the same symbol names (repo and the afterEach
callback) are used so teardown won't throw a follow-on error hiding the original
beforeEach failure.
| resolve(relativePath) { | ||
| return join(this.root, relativePath); | ||
| } |
There was a problem hiding this comment.
Block path escape outside fixture root.
join(this.root, relativePath) allows ../ traversal; write()/remove() can touch paths outside the temp repo.
Suggested fix
-import { dirname, join } from 'node:path';
+import { dirname, join, relative, resolve } from 'node:path';
@@
resolve(relativePath) {
- return join(this.root, relativePath);
+ const absolutePath = resolve(this.root, relativePath);
+ const rel = relative(this.root, absolutePath);
+ if (rel.startsWith('..') || rel === '') {
+ throw new Error(`Path escapes fixture root: ${relativePath}`);
+ }
+ return absolutePath;
}Also applies to: 59-63, 79-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helpers/repo-fixture.js` around lines 51 - 53, The resolve(relativePath)
helper (and callers write() and remove()) currently uses join(this.root,
relativePath) which permits ../ traversal; change them to compute an absolute
resolved path (using path.resolve or path.resolve(this.root, relativePath)) and
then validate that the resolved path stays inside the fixture root (e.g., using
path.relative(this.root, resolvedPath) and rejecting if it starts with '..' or
otherwise falls outside, accounting for equality with this.root); if validation
fails, throw an error and do not perform the write/remove. Update resolve(),
write(), and remove() to use this validation check before touching the
filesystem.
| async merge(ref, message) { | ||
| runGit(this.root, ['merge', '--no-ff', '-m', message, ref]); | ||
| return this.currentSha(); |
There was a problem hiding this comment.
Keep merge commits deterministic too.
merge() currently uses wall-clock commit metadata, which breaks deterministic-history guarantees provided by commit().
Suggested fix
async merge(ref, message) {
- runGit(this.root, ['merge', '--no-ff', '-m', message, ref]);
- return this.currentSha();
+ const timestamp = isoTimestamp(this.#commitIndex);
+ runGit(this.root, ['merge', '--no-ff', '-m', message, ref], {
+ env: {
+ GIT_AUTHOR_DATE: timestamp,
+ GIT_COMMITTER_DATE: timestamp,
+ },
+ });
+ this.#commitIndex += 1;
+ return this.currentSha();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async merge(ref, message) { | |
| runGit(this.root, ['merge', '--no-ff', '-m', message, ref]); | |
| return this.currentSha(); | |
| async merge(ref, message) { | |
| const timestamp = isoTimestamp(this.#commitIndex); | |
| runGit(this.root, ['merge', '--no-ff', '-m', message, ref], { | |
| env: { | |
| GIT_AUTHOR_DATE: timestamp, | |
| GIT_COMMITTER_DATE: timestamp, | |
| }, | |
| }); | |
| this.#commitIndex += 1; | |
| return this.currentSha(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helpers/repo-fixture.js` around lines 159 - 161, The merge() helper is
creating merge commits with wall-clock metadata, breaking deterministic history;
modify merge(ref, message) to runGit with the same deterministic commit
timestamp used by commit() (set GIT_COMMITTER_DATE and GIT_AUTHOR_DATE to the
deterministic value the fixture uses) when invoking runGit(this.root, ['merge',
'--no-ff', '-m', message, ref]) so the resulting merge commit metadata matches
commit() and currentSha() remains deterministic.
| async build() { | ||
| const prefix = sanitizeName(this.#name); | ||
| const root = await mkdtemp(join(tmpdir(), `gitmind-${prefix}-`)); | ||
| const repo = await new RepoFixture(this.#name, root).init(); | ||
|
|
||
| if (this.#base) { | ||
| await this.#base(repo); | ||
| } | ||
|
|
||
| for (const step of this.#steps) { | ||
| await step(repo); | ||
| } | ||
|
|
||
| for (const overlay of this.#overlays) { | ||
| await overlay(repo); | ||
| } | ||
|
|
||
| return repo; | ||
| } |
There was a problem hiding this comment.
Clean up temp repos on build failure.
If base/step/overlay throws, build() leaks the mkdtemp directory.
Suggested fix
async build() {
const prefix = sanitizeName(this.#name);
const root = await mkdtemp(join(tmpdir(), `gitmind-${prefix}-`));
- const repo = await new RepoFixture(this.#name, root).init();
-
- if (this.#base) {
- await this.#base(repo);
- }
-
- for (const step of this.#steps) {
- await step(repo);
- }
-
- for (const overlay of this.#overlays) {
- await overlay(repo);
- }
-
- return repo;
+ const repo = new RepoFixture(this.#name, root);
+ try {
+ await repo.init();
+
+ if (this.#base) {
+ await this.#base(repo);
+ }
+
+ for (const step of this.#steps) {
+ await step(repo);
+ }
+
+ for (const overlay of this.#overlays) {
+ await overlay(repo);
+ }
+
+ return repo;
+ } catch (error) {
+ await repo.cleanup();
+ throw error;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helpers/repo-fixture.js` around lines 239 - 257, The build() method
currently leaks the temporary directory created by mkdtemp when this.#base, any
this.#steps, or this.#overlays throws; wrap the sequence that initializes
RepoFixture and then runs this.#base/this.#steps/this.#overlays in a
try/catch/finally (or try with finally) so that if an error occurs you remove
the created temporary root (the value returned by mkdtemp) before rethrowing;
reference sanitizeName, mkdtemp, RepoFixture, and the this.#base / this.#steps /
this.#overlays calls to locate where to add the cleanup (use fs.rm or
fs.promises.rm with recursive/force or an equivalent cleanup helper) and ensure
you only attempt cleanup if root was created.
1 participant
PR Title
test/docs: add canonical repo fixture builder and review hygiene
Summary
test/context.test.jsonto the shared fixture substrate and add direct helper coverage#314and refresh roadmap/docs to reflect the completed git-warp prerequisite cycle#316to keep this PR cohesiveProblem Statement
mkdtemp + git init + git configboilerplate across repo-shaped tests, which makes Hill 1 test work slower, noisier, and less reusable.#314exposed as real merge friction.ADR Compliance (Required)
Relevant ADR(s)
Compliance Declaration
Exception Request (Required if deviating)
Architecture Laws Checklist (Hard Gates)
Canonical Truth & Context
--at,--observer,--trust) or deterministically defaulted.Determinism & Provenance
Artifact Hygiene
Contracts & Compatibility
Extension/Effects Safety (if applicable)
Scope Control
Backward Compatibility
Test Plan (Required)
Unit
npm test -- test/repo-fixture.test.js test/context.test.jsIntegration
npm testDeterminism
npm test -- test/repo-fixture.test.js test/context.test.jsContract/Schema
Policy Gates
Security / Trust Impact
Performance Impact
Observability / Debuggability
Operational Notes
#316Linked Issues / Milestones
Reviewer Quick Verdict Block (for maintainers)
MUST (Hard Gates)
SHOULD (Quality)
Verdict