Repository navigation
feat(review): warn when a profile agent source is behind its upstream - #698
Conversation
Profile agent sources are local checkouts, so a reviewer merged into a shared catalog does not run until that checkout is pulled, and nothing said so. Before a review, fetch each Git-backed source's upstream and warn with the fast-forward command when the checkout is behind. Note sources cr cannot check. The check never fails a review. Closes #697
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: c35ccb79715c
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 1 |
go:implementation-tests (1 finding)
Minor - internal/agentfreshness/agentfreshness.go:128
The suggested update command inserts the repository path without shell quoting. A checkout under a path containing spaces or shell metacharacters produces a command that fails or is interpreted differently when copied into a shell. Quote the path and upstream as shell arguments, and add a rendering test using a checkout path containing spaces.
structure:repo-health (1 finding)
Major - internal/agentfreshness/agentfreshness.go:113
A failed fetch with a last-fetched count of zero produces no notice, so an unreachable upstream is indistinguishable from a successfully verified current checkout. This defeats the freshness check precisely when the cached upstream may be stale. Emit a notice whenever fetching fails, including when the cached count is zero, and test that case.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (broad); scope: 6 files; examples:internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go,internal/cmd/reviewcmd/reviewcmd.go; 1 omitted; body-inspected: 6 files; examples:internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go,internal/cmd/reviewcmd/reviewcmd.go; 1 omitted; skipped: none; context (not coverage): 1 file; examples:internal/agents/agents.go; constraints: nonepolicies:conventions— complete (constrained); scope: 7 files; examples:README.md,internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go; 2 omitted; body-inspected: 7 files; examples:README.md,internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go; 2 omitted; skipped: none; constraints: Shared CLI standards were not available in the local review context. The optional local copy of the shared CLI standards was not available; findings are limited to conventions visible in the review context.structure:repo-health— complete (broad); scope: 6 files; examples:internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go,internal/cmd/reviewcmd/reviewcmd.go; 1 omitted; body-inspected: 6 files; examples:internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go,internal/cmd/reviewcmd/reviewcmd.go; 1 omitted; skipped: none; constraints: none
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 48s | gpt-6.1-terra | cr 0.10.357
| Field | Value |
|---|---|
| Model | gpt-6.1-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 48s wall · 1m 04s compute |
| Cost | unavailable |
| Tokens | 594.6k in / 3.7k out |
Per-workstream usage
orchestrator-selection— gpt-6.1-terra- In: 36.6k
- Out: 453
- Cache read: 30.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 8s
go:implementation-tests— gpt-6.1-terra- In: 95.0k
- Out: 656
- Cache read: 80.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 14s
policies:conventions— gpt-6.1-terra- In: 359.4k
- Out: 1.5k
- Cache read: 289.9k
- Cache create: unavailable
- Cost: unavailable
- Duration: 26s
structure:repo-health— gpt-6.1-terra- In: 46.8k
- Out: 503
- Cache read: 32.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 10s
orchestrator-rollup— gpt-6.1-terra- In: 56.8k
- Out: 623
- Cache read: 48.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 4s
| if err != nil || behind == 0 { | ||
| return Notice{}, false | ||
| } | ||
| notice := Notice{Kind: KindBehind, Source: source, RepoRoot: root, Upstream: upstream, Behind: behind} |
There was a problem hiding this comment.
A failed fetch with a last-fetched count of zero produces no notice, so an unreachable upstream is indistinguishable from a successfully verified current checkout. This defeats the freshness check precisely when the cached upstream may be stale. Emit a notice whenever fetching fails, including when the cached count is zero, and test that case.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in 8436ab9. A failed fetch now always produces a notice: a behind warning that names the fetch failure, or a note: saying cr couldn't confirm the source is current when the last fetch shows it current. Tested by TestCheckNotesUnreachableUpstreamWhenLastFetchIsCurrent.
There was a problem hiding this comment.
Thanks for addressing this and adding coverage for the current cached count.
Summary:
The author reports that failed fetches now always produce a notice, including when the cached upstream count is zero, with a test covering that case.
| } | ||
|
|
||
| func (n Notice) String() string { | ||
| switch n.Kind { |
There was a problem hiding this comment.
The suggested update command inserts the repository path without shell quoting. A checkout under a path containing spaces or shell metacharacters produces a command that fails or is interpreted differently when copied into a shell. Quote the path and upstream as shell arguments, and add a rendering test using a checkout path containing spaces.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in 8436ab9. The repo path and upstream in the update command are now quoted as POSIX shell words when they need it. Tested by TestBehindNoticeQuotesUpdateCommandArguments with a path containing a space and a single quote.
There was a problem hiding this comment.
Thanks for addressing the argument quoting and adding a test for a path with spaces and a single quote.
Summary:
The author reports that the update command now quotes the repository path and upstream as POSIX shell words when needed, with a rendering test covering a path containing a space and a single quote.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 8436ab952a26
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (broad); scope: 6 files; examples:internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go,internal/cmd/reviewcmd/reviewcmd.go; 1 omitted; body-inspected: 6 files; examples:internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go,internal/cmd/reviewcmd/reviewcmd.go; 1 omitted; skipped: none; constraints: nonepolicies:conventions— complete (broad); scope: 4 files; examples:README.md,internal/agentfreshness/agentfreshness.go,internal/app/runtime.go,internal/cmd/reviewcmd/reviewcmd.go; body-inspected: 4 files; examples:README.md,internal/agentfreshness/agentfreshness.go,internal/app/runtime.go,internal/cmd/reviewcmd/reviewcmd.go; skipped: none; constraints: Shared CLI standards and automation repositories were not available locally; the review used the supplied guidance and repository context.structure:repo-health— complete (broad); scope: 6 files; examples:internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go,internal/cmd/reviewcmd/reviewcmd.go; 1 omitted; body-inspected: 6 files; examples:internal/agentfreshness/agentfreshness.go,internal/agentfreshness/agentfreshness_test.go,internal/app/runtime.go,internal/app/runtime_test.go,internal/cmd/reviewcmd/reviewcmd.go; 1 omitted; skipped: none; constraints: none
2 PR discussion threads considered. 2 summarized; 2 resolved.
Completed in 45s | gpt-6.1-terra | cr 0.10.357
| Field | Value |
|---|---|
| Model | gpt-6.1-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-6.1-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 45s wall · 41s compute |
| Cost | unavailable |
| Tokens | 316.1k in / 2.3k out |
Per-workstream usage
orchestrator-selection— gpt-6.1-terra- In: 18.5k
- Out: 316
- Cache read: 12.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s
go:implementation-tests— gpt-6.1-terra- In: 71.1k
- Out: 507
- Cache read: 49.7k
- Cache create: unavailable
- Cost: unavailable
- Duration: 11s
policies:conventions— gpt-6.1-terra- In: 69.6k
- Out: 322
- Cache read: 56.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 9s
structure:repo-health— gpt-6.1-terra- In: 47.9k
- Out: 450
- Cache read: 24.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 10s
orchestrator-rollup— gpt-6.1-terra- In: 109.0k
- Out: 717
- Cache read: 94.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 4s
Closes #699 ## Problem A `cr review` rerun on #698 failed in `thread_analysis` before the provider started (`resume_llm`, 0 ms). It couldn't open `agent-logs/thread-analysis/<thread>.jsonl`: `no such file or directory`. The pipeline does create that directory (`prepareSelectionContext`), but only once, early in the run, while it is still empty. `llm.LaunchProcess` then opened the log with a bare `os.Create`, so a missing parent directory was fatal. The run's artifact timestamps show the directory was removed between being created and the thread analysis running. In the failing run, `agent-logs/` and `dossier/` were created at 15:54:4x–50, and the thread-analysis task started at 15:55:03. The `thread-analysis/` log dir on disk now was created later, at 15:55:48, by the `--fresh-session` rerun. The only code that deletes directories under the runs root is retention prune's `removeEmptyParents`, which removes every empty directory. Another `cr review` started on a different PR at the same time (15:55:09). I couldn't prove a prune got past the active-runs lock, so that part is a likely explanation, not a confirmed one. ## Fix Two changes, one per side of the failure: 1. **Opening the log.** `openSubprocessLog` now runs `MkdirAll` on the log's parent (0700, the same mode the pipeline uses) before it creates the file. Every subprocess task, fresh or resumed, then works no matter what happened to the directory after setup. 2. **The empty-dir sweep.** `removeEmptyParents` used to walk the whole runs root and remove every empty directory, including empty subdirs inside a live run's artifact tree. It now stops at attempt (`run-*`) dirs, so it removes only empty layout parents left behind by deleted runs. Unreferenced attempt dirs are still cleaned up by orphan removal, after its grace window. ## Tests - New subtest `TestSubprocessCodexSafetyModes/resume_creates_a_missing_log_directory`: resumes a codex session with `LogPath` under a missing `agent-logs/thread-analysis/`. Without the fix it fails with the same `open …: no such file or directory` error as the issue. With the fix it passes. - New `TestPruneKeepsEmptyArtifactSubdirsOfLiveRuns`: a live run with an empty `agent-logs/thread-analysis/` keeps it through `Prune`, and an empty layout parent is still removed. Without the fix the live run's dir is deleted. - `go test ./...` (3825 passed) and `make lint` (0 issues).
Closes #697
Summary
Profile agent sources are local directories. When one is a clone of a shared reviewer catalog, a reviewer merged there does not run until that clone is pulled, and
cr reviewgave no sign of it.Before each
cr review(live or dry-run), cr now checks every profile agent source:warning: agent source ~/catalog/.codereview/agents is 2 commits behind origin/main; reviewer changes merged there will not run. Update with: git -C /path/to/catalog merge --ff-only origin/mainIf the fetch fails, count against the last fetched upstream, and note the failed fetch even when that count is zero.
note:that cr cannot check whether it is current.The check never fails a review. Sources in the same checkout are fetched once, and missing sources are left to the existing loader errors.
cr respondand benchmarks skip the check: respond doesn't load the catalog, and a benchmark would fetch on every case.The check is in a new
internal/agentfreshnesspackage.app.Openruns it whenOpenRequest.CheckAgentSourceFreshnessis set, through aDependencies.CheckAgentSourcesseam that tests can fake.Testing
go test ./...andgolangci-lint runpass.internal/agentfreshnessuse real Git fixtures (a bare upstream plus clones) to cover: behind, current, fetch failure with a stale or current last fetch, shell quoting of the update command, unversioned, no upstream, missing sources, and dedupe.app.Openandcr reviewtests confirm only review asks for the check.origin/main.