Repository navigation
test: isolate Grok test home directories - #303
Conversation
|
@sfl review |
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![hemsoft-sfl-reviewer[bot]](https://avatars.githubusercontent.com/in/5406096?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Verdict: PASS • Round 1
Review done by claude-opus-5-5 (adversarial to codex)
This test-only change renames the Grok Home fixture's temp folder from codexbar-grok-{name}-{pid} to codexbar-grok-home-{name}-{pid}. The old name Home::with("activity") produced the same path as the grok_activity test folder codexbar-grok-activity-{pid}, and the new prefix keeps the two sets of folders apart. No Critical, High or Medium defect was found, so nothing blocks approval.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 1m 5s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
crates/codexbar-providers/src/grok.rs
- Context read:
crates/codexbar-providers/src/grok_activity.rscrates/codexbar-providers/src/lib.rscrates/codexbar-providers/src/*.rs (all 43 crate sources, searched for the `codexbar-grok` prefix)AGENTS.md
- Verified:
- Compare
base_commit.shaisd213f24be8c939bdbe345fce371cd7efd0e88ef3and the last commit is1c5b8cc1841f3f4383b01521e8bc47c6da010416: 1 ahead, 0 behind - The diff changes 1 file, adding 3 lines and removing 2: a doc comment and the
format!path inHome::with - At base,
Home::with("activity")inactivity_is_a_fallback_and_never_replaces_a_reported_quotabuiltcodexbar-grok-activity-{pid}, the same pathgrok_activitytestlocal_metadata_deduplicates_and_keeps_exact_seven_day_boundsbuilds and then removes - At head, all
Homenames (activity,quota-before-activity,renew,plan-without-pool,x-subscription,renew-403,other) sit undercodexbar-grok-home-, apart from thegrok_activitynamesempty,activityandbad - Within the crate, only
grok.rsandgrok_activity.rsuse thecodexbar-grokprefix. The fixed pathcodexbar-grok-nothing-heredoes not overlap thehome-namespace - Cleanup is unchanged: the
Drop for Homestill removes the sameself.0path it created - The production code paths in
grok.rs, including thegrok_activity::readcalls, are untouched
- Compare
- Not verified:
- The PR's claims of a failing run before the change and 20 passing runs after it on Windows; no code could be executed
- Collisions with temp folders created by other crates or by other processes on the same machine; only
codexbar-providerssources were searched - The status of CI checks
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 1c5b8cc1841f3f4383b01521e8bc47c6da010416 • SFL v0
|
@sfl review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 2
Since round 1
New: 0; still open (repeated): 0; resolved: 0.
Resolved means no matching finding in this review, not independent proof of a fix.
New (0)
- None.
Still open (repeated) (0)
- None.
Resolved (0)
- None.
Review done by claude-opus-5-5 (adversarial to codex)
The change only touches tests. It renames the Grok Home fixture's temp folder from codexbar-grok-{name}-{pid} to codexbar-grok-home-{name}-{pid}, so Home::with("activity") no longer resolves to the same folder as the codexbar-grok-activity-{pid} folder in grok_activity's test, and it updates the doc comment. Approve: I found no Critical, High, Medium or Low defect. The new prefix is disjoint from every other temp folder name in the crate that I checked.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 1m 18s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
crates/codexbar-providers/src/grok.rs
- Context read:
crates/codexbar-providers/src/grok_activity.rscrates/codexbar-providers/src/lib.rscrates/codexbar-providers/src (all other module sources, temp_dir usages)AGENTS.md
- Verified:
- The compare response's
base_commit.shais9df7787f0a68e8f23ee5235b89d1ad01014f6a97, its last commit SHA ise4a555b7250ba4b2abebc7ea2f88cd5817063e4b, status is ahead by 2 and behind by 0, and only one file changed (3 lines added, 2 removed) - PR #303's head and base SHAs match the reserved SHAs
- The base collision is real:
Home::with("activity")atgrok.rs:696producedcodexbar-grok-activity-{pid}, the same path asgrok_activity.rs:152, which that test removes withremove_dir_all - The new
codexbar-grok-home-prefix does not overlapgrok_activity'sempty,activityorbadfolders, norcodexbar-grok-nothing-hereatgrok.rs:858 - The
Home::withnames ingrok.rs(activity,quota-before-activity,renew,plan-without-pool,x-subscription,renew-403,other) are unique, so no twoHomefixtures share a folder - No other provider module or file under
crates/codexbar-providers/testsuses acodexbar-groktemp prefix - The
Dropcleanup and everyhome.0use derive from the new path, so cleanup still removes the folder each fixture created - No production code, dependency or configuration changed
- The
test:commit prefix follows the conventional-commit rule inAGENTS.md
- The compare response's
- Not verified:
- The PR's claims of failing-before and 20/20 passing-after runs, because executing tests is forbidden
- CI status and the merge commit's non-Grok content from main, because they are outside this diff's scope
- Collisions with temp folders created by other crates' test binaries running at the same time, which is unlikely given the per-process
pidsuffix but was not checked
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head e4a555b7250ba4b2abebc7ea2f88cd5817063e4b • SFL v0
Summary
Grok's
Hometest helper andgrok_activityused the same process-local temp folder. Parallel tests could delete each other's files and fail withAccess is deniedorNotSignedIn.Give
Homefixtures their owncodexbar-grok-home-prefix. Test names, assertions, cleanup and production behavior are unchanged.Acceptance Criteria
Homenamespace is disjoint fromgrok_activity'sempty,activityandbadfolders. The existing provider test names were checked for other collisions.Definition of Done
e4a555b7250ba4b2abebc7ea2f88cd5817063e4bin run 42116861363. Authenticated SFL Round 2 PASS and Codex clean, zero unresolved threads, zero formal approvals required.Validation
Initial local reproduction: base
d213f24be8c939bdbe345fce371cd7efd0e88ef3, candidate1c5b8cc1841f3f4383b01521e8bc47c6da010416.Current head
e4a555b7250ba4b2abebc7ea2f88cd5817063e4bmerges fixed main9df7787f0a68e8f23ee5235b89d1ad01014f6a97after fixture repair #305. The Grok diff against the new base is byte-identical to the initial validated diff. Formatting and diff checks passed after this update. All seven hosted CI checks and fresh SFL/Codex reviews passed for this head. The completed rust-metrics job log verifies actualMETRICS_BASE: 9df7787f0a68e8f23ee5235b89d1ad01014f6a97, which includes the fixture repair.cargo test -p codexbar-providers --lib --locked --no-run --message-format=json, then ran its resulting library test binary with the default parallel test harness. Each run used fresh synthetic HOME/USERPROFILE, AppData, XDG and temp directories.grok_activity::tests::local_metadata_deduplicates_and_keeps_exact_seven_day_boundspanicked atgrok_activity.rs:168withAccess is deniedwhile removing the shared folder.cargo fmt --all --check: pass.cargo clippy -p codexbar-providers --all-targets --locked -- -D warnings: pass.git diff --check: pass.The Rust test-harness documentation describes the shared-file interference this reproduces. No path-string-only test was added; the unchanged suite supplied the regression proof.
Risks and deferred work
Production code and dependencies are unchanged. Dependency repair #296 is separate. The original audit report and the other writer's lockfile bytes were preserved. Claude Opus 5.5 prepared the bounded source edit; the initiating GPT-6.1 Sol parent independently verified, packaged and validated it.
Closes #298
Merge evidence
Checked head
e4a555b7250ba4b2abebc7ea2f88cd5817063e4b; all seven GitHub Actions checks passed with integration ID 15368, authenticated SFL/Codex clean, zero unresolved threads and zero formal approvals required. The metrics log confirms repaired base9df7787f0a68e8f23ee5235b89d1ad01014f6a97. Guarded squash merge landed asa444e81a375fad4677b99a6fc09a56dd322ea424. Mini and Home main both fast-forwarded to that commit, with the audit report preserved.