Conversation
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
📝 WalkthroughWalkthroughThis PR adds a repo-remote alignment validation gate to prevent auto-mode execution and project health computation when a project's git remote origin mismatches an expected GitHub repository slug. The implementation provides startup sanity checking, project health warnings, and an environment variable opt-out for operators. ChangesRepo-Remote Alignment Validation
Sequence Diagram(s)sequenceDiagram
participant Client
participant AutoModeService
participant ProjectSettings
participant validateRepoSlug
participant GitCommand
Client->>AutoModeService: startAutoLoopForProject()
AutoModeService->>ProjectSettings: load expectedRepoSlug
AutoModeService->>validateRepoSlug: validateRepoSlug(projectPath, expectedRepoSlug)
validateRepoSlug->>GitCommand: git remote get-url origin
GitCommand-->>validateRepoSlug: remote URL
validateRepoSlug->>validateRepoSlug: extract slug, compare case-insensitive
alt slug mismatch + not skipped
validateRepoSlug-->>AutoModeService: valid=false, mismatchMessage
AutoModeService->>AutoModeService: throw error
AutoModeService-->>Client: Error: repo mismatch
else skipped or aligned
validateRepoSlug-->>AutoModeService: valid=true or skipped=true
AutoModeService->>AutoModeService: proceed with auto-mode
AutoModeService-->>Client: auto-mode started
end
sequenceDiagram
participant ProjectHealthService
participant checkRepoAlignment
participant validateRepoSlug
participant HealthStorage
ProjectHealthService->>checkRepoAlignment: checkRepoAlignment(projectPath)
checkRepoAlignment->>validateRepoSlug: validateRepoSlug(projectPath, expectedRepoSlug)
alt not configured
validateRepoSlug-->>checkRepoAlignment: notConfigured=true
checkRepoAlignment-->>ProjectHealthService: null
else alignment check runs
validateRepoSlug-->>checkRepoAlignment: result with valid/actual/expected
alt misaligned
checkRepoAlignment-->>ProjectHealthService: aligned=false
ProjectHealthService->>HealthStorage: set health to off-track
ProjectHealthService->>HealthStorage: persist
ProjectHealthService-->>ProjectHealthService: return early
else aligned
checkRepoAlignment-->>ProjectHealthService: aligned=true
ProjectHealthService->>ProjectHealthService: proceed to health derivation
end
end
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![protoquinn[bot]](https://avatars.githubusercontent.com/in/3282126?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA Audit — PR #4127 | [github] bug: project path → git remote misalignment corrupts cross-repo features
VERDICT: WARN
CI Status
- build: queued
- checks: queued
- test: queued
(CI not yet terminal — this is a non-blocking COMMENT. Re-request formal PASS/FAIL once checks are green.)
Diff Review
repo-remote-validation.ts(new): Core validation module — parses HTTPS and SSH GitHub remotes via two regexes, readsgit remote get-url originwith 10s timeout, non-fatal on failure. Escape hatchAUTOMAKER_SKIP_REPO_SLUG_CHECKdocumented.auto-mode-service.ts: Gate inserted before auto-mode loop — fetchesexpectedRepoSlugfrom settings, throwsErroron mismatch. Null-safe (?.catch(() => null)). Correctly skips whenexpectedRepoSlugis unset.project-health-service.ts:checkRepoAlignmentpublic method surfaces alignment to board UI; misaligned projects immediately markedoff-trackwithout waiting for other health factors.
Observations
- LOW:
extractRepoSlugsilently returnsnullfor non-GitHub remotes — no warning is emitted when the project uses a non-GitHub remote (e.g. GitLab, Bitbucket, or a local path). Sincenullcauses validation to skip, this is technically safe but could mask a configuration gap. Worth adding alogger.infofor visibility if desired, but not a blocker. - GAP: Clawpatch structural review unavailable (checkout cache rejected ref
main— likely base branch isdev). Diff-based review finds no correctness, security, or data-loss issues. Recommend a manual structural pass if the team has concerns about the exec call path. - GAP:
AUTOMAKER_SKIP_REPO_SLUG_CHECKbypass is env-var-only — no equivalent runtime flag surfaced to the board UI or settings file. If an operator needs to temporarily bypass for a misconfigured project, they must restart the server with the env var set. Acceptable for an MVP; askipReasonfield in project settings would be a future improvement.
Checks: 2 (CI pending)
Passed: 0 (awaiting CI)
Failed: 0
Gaps: 2
— Quinn, QA Engineer
|
Submitted COMMENT review on #4127. CI is still queued — formal PASS/FAIL will be determined on the next pass once checks are terminal-green. The diff is sound: the new |
There was a problem hiding this comment.
QA Audit — PR #4127 | [github] bug: project path → git remote misalignment corrupts cross-repo features
VERDICT: FAIL
CI Status
- ci-complete: failure ❌
- build: failure ❌
- checks: failure ❌
- test: failure ❌
- Close external code contributions: skipped
Diff Review
Adds a new repo-remote-validation.ts service that parses git remote origin (HTTPS + SSH) to extract owner/repo slugs. Wires this as a startup gate in auto-mode-service.ts (refuses to run when expectedRepoSlug mismatches the actual remote) and surfaces mismatches in project-health-service.ts by marking projects off-track. Escape hatch via AUTOMAKER_SKIP_REPO_SLUG_CHECK=1. The implementation logic is sound — regex patterns are correctly escaped, git calls use 10s timeouts, errors are non-fatal.
Observations
- CRITICAL: All CI checks (build, checks, test) have completed with
failurestate. This is a hard gate — code quality cannot be assessed until CI is green. The feature logic looks correct from the diff, but the PR cannot be approved with failing infrastructure. - GAP: Clawpatch structural review was unavailable (checkout cache could not resolve
mainas a SHA). Direct diff review found no correctness issues, but automated cross-file analysis of the new service was skipped. - LOW: Specific CI error messages are not accessible from the diff context. Please share build/checks/test output so the failures can be diagnosed.
Please resolve the CI failures and re-request review. The feature intent and implementation approach appear sound.
— Quinn, QA Engineer
|
Submitted |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/tests/unit/services/repo-remote-validation.test.ts (1)
1-198: 🛠️ Refactor suggestion | 🟠 MajorAdd integration coverage for the repo-remote wiring points (auto-mode refusal + project-health off-track)
apps/server/tests/integration/services/auto-mode-service.integration.test.tsexercisesstartAutoLoop(...)/loop start-stop, but there are no integration tests referencingAutoModeService.startAutoLoopForProject(...)or the repo-remote/compliance refusal path (the only repo-remote coverage found is unit-level inapps/server/tests/unit/services/repo-remote-validation.test.ts).apps/server/tests/integration/has no tests referencingProjectHealthService.computeAndUpdate(...)/checkRepoAlignment(...)/expectedRepoSlug/validateRepoSlug, nor any assertions onon-track/at-risk/off-track.Add (or link existing) integration tests for:
AutoModeService.startAutoLoopForProject()refusal when origin repo slug mismatchesexpectedRepoSlugProjectHealthService.computeAndUpdate()off-track short-circuit when repo alignment fails🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/tests/unit/services/repo-remote-validation.test.ts` around lines 1 - 198, The integration test suite lacks coverage for the repo-remote refusal and project-health off-track paths; add integration tests that call AutoModeService.startAutoLoopForProject(...) to assert it refuses/does not start when validateRepoSlug() reports a mismatch against expectedRepoSlug, and add tests that call ProjectHealthService.computeAndUpdate(...) (or its checkRepoAlignment(...) helper) to verify it short-circuits to "off-track" when validateRepoSlug() returns a mismatch (including cases of non-GitHub remote and git failure). Mock or stub the repo-remote validation to return controlled responses (valid, mismatch, unresolved) and assert the resulting behavior/state (auto-loop not started/refused, project health set to off-track and appropriate messages), ensuring tests exercise the same wiring points used by AutoModeService and ProjectHealthService rather than calling validateRepoSlug() unit helpers directly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/services/auto-mode-service.ts`:
- Around line 841-843: The current silent swallow of settings read errors via
.catch(() => null) lets projectSettings be null and bypass the repo-slug
alignment gate; change this to fail-closed by removing the silent catch so that
settingsService.getProjectSettings(projectPath) propagates errors (or explicitly
throw/log and rethrow) instead of returning null, ensuring the repo-slug check
in the auto-mode startup path runs only when settings were read successfully or
requires an explicit operator override flag; update code around projectSettings,
settingsService.getProjectSettings and the repo-slug alignment guard to handle
the propagated error path (or check an explicit override) so transient/corrupt
reads do not enable auto-mode.
- Around line 846-850: The call to buildRepoSlugMismatchMessage passes
slugValidation.actualSlug which is typed as string | null | undefined causing a
TS2345 error; ensure actualSlug is narrowed or defaulted to a non-undefined
string before calling buildRepoSlugMismatchMessage (e.g., coalesce undefined to
null or an empty string) so its type matches the function signature, and update
the call site in auto-mode-service.ts where buildRepoSlugMismatchMessage is
invoked; also inspect validateRepoSlug() and the settings read that uses
.catch(() => null) (which can make expectedRepoSlug undefined) and ensure
validateRepoSlug() treats a missing expectedRepoSlug as skipped/valid or
otherwise handle the missing value before building the mismatch message.
In `@apps/server/src/services/repo-remote-validation.ts`:
- Around line 30-33: The current regexes GITHUB_HTTPS_REMOTE_REGEX and
GITHUB_SSH_REMOTE_REGEX used by extractRepoSlug truncate repo names with dots
and miss ssh:// style remotes; update both regexes to use a repo capture that
allows dots (e.g. use [^/]+ for the repo segment instead of ([^/.]+)), make the
repo suffix optionally end with .git, and expand the SSH pattern to accept both
git@github.com:owner/repo and ssh://git@github.com/owner/repo forms (allow
either ":" or "/" after the host and an optional "ssh://" prefix). Ensure
extractRepoSlug continues to use these constants so it correctly returns
owner/repo slugs for dotted names and ssh:// remotes.
In `@apps/server/tests/unit/services/repo-remote-validation.test.ts`:
- Around line 53-73: The test suite for extractRepoSlug is missing regression
cases for dotted repository names and SSH-style GitHub remotes using the ssh://
scheme; add unit tests inside the describe('extractRepoSlug()') block that
assert extractRepoSlug('https://github.com/owner/repo.name.git') and
extractRepoSlug('https://github.com/owner/repo.name') return 'owner/repo.name',
and also assert extractRepoSlug('ssh://git@github.com/owner/repo.git') and
extractRepoSlug('ssh://git@github.com/owner/repo') return 'owner/repo'
(including trimmed variants), so extractRepoSlug handles dotted repo names and
the ssh:// GitHub remote form.
---
Outside diff comments:
In `@apps/server/tests/unit/services/repo-remote-validation.test.ts`:
- Around line 1-198: The integration test suite lacks coverage for the
repo-remote refusal and project-health off-track paths; add integration tests
that call AutoModeService.startAutoLoopForProject(...) to assert it refuses/does
not start when validateRepoSlug() reports a mismatch against expectedRepoSlug,
and add tests that call ProjectHealthService.computeAndUpdate(...) (or its
checkRepoAlignment(...) helper) to verify it short-circuits to "off-track" when
validateRepoSlug() returns a mismatch (including cases of non-GitHub remote and
git failure). Mock or stub the repo-remote validation to return controlled
responses (valid, mismatch, unresolved) and assert the resulting behavior/state
(auto-loop not started/refused, project health set to off-track and appropriate
messages), ensuring tests exercise the same wiring points used by
AutoModeService and ProjectHealthService rather than calling validateRepoSlug()
unit helpers directly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d7a66d8-3724-46fa-b472-8415c3e54178
📒 Files selected for processing (5)
apps/server/src/services/auto-mode-service.tsapps/server/src/services/project-health-service.tsapps/server/src/services/repo-remote-validation.tsapps/server/tests/unit/services/repo-remote-validation.test.tslibs/types/src/project-settings.ts
| const projectSettings = this.settingsService | ||
| ? await this.settingsService.getProjectSettings(projectPath).catch(() => null) | ||
| : null; |
There was a problem hiding this comment.
Settings read failures currently bypass the alignment gate (fail-open).
At Line 842, .catch(() => null) means a transient/corrupt settings read silently disables the repo-slug check and allows auto-mode startup. For this guard, fail-closed behavior is safer (or require explicit operator override).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/auto-mode-service.ts` around lines 841 - 843, The
current silent swallow of settings read errors via .catch(() => null) lets
projectSettings be null and bypass the repo-slug alignment gate; change this to
fail-closed by removing the silent catch so that
settingsService.getProjectSettings(projectPath) propagates errors (or explicitly
throw/log and rethrow) instead of returning null, ensuring the repo-slug check
in the auto-mode startup path runs only when settings were read successfully or
requires an explicit operator override flag; update code around projectSettings,
settingsService.getProjectSettings and the repo-slug alignment guard to handle
the propagated error path (or check an explicit override) so transient/corrupt
reads do not enable auto-mode.
| const message = buildRepoSlugMismatchMessage( | ||
| projectPath, | ||
| slugValidation.expectedSlug!, | ||
| slugValidation.actualSlug | ||
| ); |
There was a problem hiding this comment.
Fix build-breaking TS type mismatch for actualSlug
apps/server/src/services/auto-mode-service.ts (around lines 846-850): slugValidation.actualSlug is typed as string | null | undefined (actualSlug?: string | null), but buildRepoSlugMismatchMessage(..., actualSlug: string | null) rejects undefined, matching the CI TS2345 failure.
💡 Suggested fix
const message = buildRepoSlugMismatchMessage(
projectPath,
slugValidation.expectedSlug!,
- slugValidation.actualSlug
+ slugValidation.actualSlug ?? null
);Additionally, the settings read uses .catch(() => null); if that causes expectedRepoSlug to be missing, confirm validateRepoSlug() can’t mark the mismatch gate as “valid/skipped” in that case.
📝 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.
| const message = buildRepoSlugMismatchMessage( | |
| projectPath, | |
| slugValidation.expectedSlug!, | |
| slugValidation.actualSlug | |
| ); | |
| const message = buildRepoSlugMismatchMessage( | |
| projectPath, | |
| slugValidation.expectedSlug!, | |
| slugValidation.actualSlug ?? null | |
| ); |
🧰 Tools
🪛 GitHub Actions: PR Build Check / 1_build.txt
[error] 849-849: TypeScript (tsc) error TS2345: Argument of type 'string | null | undefined' is not assignable to parameter of type 'string | null'.
🪛 GitHub Actions: PR Build Check / build
[error] 849-849: TypeScript (tsc) error TS2345: Argument of type 'string | null | undefined' is not assignable to parameter of type 'string | null'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/auto-mode-service.ts` around lines 846 - 850, The
call to buildRepoSlugMismatchMessage passes slugValidation.actualSlug which is
typed as string | null | undefined causing a TS2345 error; ensure actualSlug is
narrowed or defaulted to a non-undefined string before calling
buildRepoSlugMismatchMessage (e.g., coalesce undefined to null or an empty
string) so its type matches the function signature, and update the call site in
auto-mode-service.ts where buildRepoSlugMismatchMessage is invoked; also inspect
validateRepoSlug() and the settings read that uses .catch(() => null) (which can
make expectedRepoSlug undefined) and ensure validateRepoSlug() treats a missing
expectedRepoSlug as skipped/valid or otherwise handle the missing value before
building the mismatch message.
| const GITHUB_HTTPS_REMOTE_REGEX = /https:\/\/github\.com\/([^/]+)\/([^/.]+)/; | ||
| /** Regex for SSH GitHub remotes: git@github.com:owner/repo.git */ | ||
| const GITHUB_SSH_REMOTE_REGEX = /git@github\.com:([^/]+)\/([^/.]+)/; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant sections
FILE="apps/server/src/services/repo-remote-validation.ts"
ls -l "$FILE" || (echo "Missing $FILE" && exit 1)
# Show around the regex definitions (lines 1-120 is usually safe)
nl -ba "$FILE" | sed -n '1,180p'
# Find all usages of the regex constants in this file
rg -n "GITHUB_(HTTPS|SSH)_REMOTE_REGEX" "$FILE"
# If the file is small, also dump rest to understand flow
wc -l "$FILE"
nl -ba "$FILE" | sed -n '180,400p' || trueRepository: protoLabsAI/protoMaker
Length of output: 227
🏁 Script executed:
#!/bin/bash
set -euo pipefailRepository: owner/repo
Length of output: 2026
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/server/src/services/repo-remote-validation.ts"
[ -f "$FILE" ] || { echo "Missing $FILE"; exit 1; }
echo "== File length =="
wc -l "$FILE"
echo "== Regex definitions area (lines 1-120) =="
awk 'NR>=1 && NR<=120 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo "== Find extractRepoSlug / usage of regexes =="
rg -n "extractRepoSlug|GITHUB_HTTPS_REMOTE_REGEX|GITHUB_SSH_REMOTE_REGEX|remote|slug" "$FILE"
echo "== Show extractRepoSlug implementation block (best-effort) =="
# If present, print around its definition
rg -n "function extractRepoSlug|extractRepoSlug\s*=" "$FILE" || true
# Print more context around possible match blocks
awk 'NR>=120 && NR<=320 {printf "%d\t%s\n", NR, $0}' "$FILE" 2>/dev/null || trueRepository: protoLabsAI/protoMaker
Length of output: 9850
Fix GitHub remote slug parsing to handle dotted repo names and ssh:// remotes.
- In
apps/server/src/services/repo-remote-validation.ts, the repo segment regex uses([^/.]+), which truncates repo names containing dots (e.g.my.repobecomesmy) causing false mismatches. - The SSH regex only matches
git@github.com:owner/repo(.git);ssh://git@github.com/owner/repo(.git)doesn’t match, soextractRepoSlug()returnsnulland the repo-slug gate gets skipped.
💡 Suggested fix
-const GITHUB_HTTPS_REMOTE_REGEX = /https:\/\/github\.com\/([^/]+)\/([^/.]+)/;
-const GITHUB_SSH_REMOTE_REGEX = /git@github\.com:([^/]+)\/([^/.]+)/;
+const GITHUB_HTTPS_REMOTE_REGEX =
+ /^https:\/\/(?:[^`@/`]+@)?github\.com\/([^/]+)\/([^/\s]+?)(?:\.git)?$/i;
+const GITHUB_SSH_REMOTE_REGEX =
+ /^(?:git@github\.com:|ssh:\/\/git@github\.com\/)([^/]+)\/([^/\s]+?)(?:\.git)?$/i;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/repo-remote-validation.ts` around lines 30 - 33, The
current regexes GITHUB_HTTPS_REMOTE_REGEX and GITHUB_SSH_REMOTE_REGEX used by
extractRepoSlug truncate repo names with dots and miss ssh:// style remotes;
update both regexes to use a repo capture that allows dots (e.g. use [^/]+ for
the repo segment instead of ([^/.]+)), make the repo suffix optionally end with
.git, and expand the SSH pattern to accept both git@github.com:owner/repo and
ssh://git@github.com/owner/repo forms (allow either ":" or "/" after the host
and an optional "ssh://" prefix). Ensure extractRepoSlug continues to use these
constants so it correctly returns owner/repo slugs for dotted names and ssh://
remotes.
| describe('extractRepoSlug()', () => { | ||
| it('extracts slug from HTTPS GitHub URL', () => { | ||
| expect(extractRepoSlug('https://github.com/owner/repo.git')).toBe('owner/repo'); | ||
| expect(extractRepoSlug('https://github.com/owner/repo')).toBe('owner/repo'); | ||
| }); | ||
|
|
||
| it('extracts slug from SSH GitHub URL', () => { | ||
| expect(extractRepoSlug('git@github.com:owner/repo.git')).toBe('owner/repo'); | ||
| expect(extractRepoSlug('git@github.com:owner/repo')).toBe('owner/repo'); | ||
| }); | ||
|
|
||
| it('returns null for non-GitHub URLs', () => { | ||
| expect(extractRepoSlug('https://gitlab.com/owner/repo.git')).toBeNull(); | ||
| expect(extractRepoSlug('ssh://git@internal.example.com/repo.git')).toBeNull(); | ||
| expect(extractRepoSlug('file:///local/path')).toBeNull(); | ||
| }); | ||
|
|
||
| it('trims whitespace', () => { | ||
| expect(extractRepoSlug(' https://github.com/owner/repo.git ')).toBe('owner/repo'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add parser regression tests for dotted repos and ssh:// GitHub remotes.
Current tests don’t lock behavior for repo names like owner/repo.name or ssh://git@github.com/owner/repo.git, which are important for this gate’s correctness.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/tests/unit/services/repo-remote-validation.test.ts` around lines
53 - 73, The test suite for extractRepoSlug is missing regression cases for
dotted repository names and SSH-style GitHub remotes using the ssh:// scheme;
add unit tests inside the describe('extractRepoSlug()') block that assert
extractRepoSlug('https://github.com/owner/repo.name.git') and
extractRepoSlug('https://github.com/owner/repo.name') return 'owner/repo.name',
and also assert extractRepoSlug('ssh://git@github.com/owner/repo.git') and
extractRepoSlug('ssh://git@github.com/owner/repo') return 'owner/repo'
(including trimmed variants), so extractRepoSlug handles dotted repo names and
the ssh:// GitHub remote form.
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — FAIL
code-review-structural · head a568b77f6be4 · formal
[review-synthesizer completed: workflow code-review-structural:report]
The verifier confirmed all four findings (none refuted, none uncertain), so the array keeps all four. Re-grading: finding 1 is re-severity'd from major to blocker — the verifier explicitly calls it "the real blocker" (the added suite deterministically fails, so CI is red on arrival), and the synthesizer is instructed to re-grade when a finder under-called it. Findings 2–4 remain minors, with finding 2's claim softened per the verifier (settingsService is a required constructor arg; failure logs a warning rather than being fully silent). No prior-requests block, so no dispositions JSON. Coverage cross-check: production files are touched and the array is non-empty; the one under-read spot is auto-mode-service.ts, which has no file-anchored finding (its gate changes are covered only indirectly via findings 2 and 4) — surfaced in the brief. The structural pass (code-review-structural:verify) completed, so no skipped-pass gap exists.
Overall risk is high: the PR is red on arrival — the added suite deterministically fails (finding 1), which blocks merge, and the new production behavior (checkRepoAlignment, the health → 'off-track' branch, the auto-loop refusal gate) ships with zero direct tests. Fix first: give the 'Concurrency & race guard' describe its own beforeEach that resets the hoisted h.failGit/h.remoteUrl state, since the earlier git-failure test leaves it set. The verifier confirmed all four findings byte-for-byte against the diff; it upgraded finding 1 from major to blocker and softened finding 2 — settingsService is a required constructor arg, so that defect is a JSDoc/contract contradiction plus a JS-runtime hazard that logs a warning, not a silent null. No refuted findings and no panel disagreement on substance. Coverage gap: auto-mode-service.ts's gate changes have no file-anchored finding — they are only covered indirectly via findings 2 and 4, so that file went under-read.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🔴 | blocker | apps/server/tests/unit/services/repo-remote-validation.test.ts:180 |
The 'Concurrency & race guard' tests deterministically fail because shared hoisted mock state h.failGit is left true by the preceding 'does NOT block when git … | confirmed |
| 🟡 | minor | apps/server/src/services/project-health-service.ts:114 |
checkRepoAlignment's JSDoc advertises a 'settings-service-free entry point' for the board UI or other services, but the implementation dereferences this.settin… | confirmed |
| 🟡 | minor | apps/server/src/services/repo-remote-validation.ts:168 |
mismatchMessage is dead output: validateRepoSlug builds it on mismatch, but the only production consumer of the result (the auto-mode gate) discards it and re-… | confirmed |
| 🟡 | minor | apps/server/src/services/project-health-service.ts:107 |
The new behavior this fix depends on is untested: the PR's suite covers only the pure helper module, with zero tests for the new public checkRepoAlignment() me… | confirmed |
findings JSON (machine-readable)
[
{
"file": "apps/server/tests/unit/services/repo-remote-validation.test.ts",
"line": 180,
"severity": "blocker",
"category": "tests",
"claim": "The 'Concurrency & race guard' tests deterministically fail because shared hoisted mock state h.failGit is left true by the preceding 'does NOT block when git command fails' test and is never reset (the concurrency describe has no beforeEach of its own), so readGitRemoteOrigin errors, actualSlug stays null, and expect(r.actualSlug).toBe('protoLabsAI/ava') fails.",
"evidence": " it('does NOT block when git command fails (no remote)', async () => {\n h.failGit = true;\n const result = await validateRepoSlug('/path', 'protoLabsAI/ava');\n expect(result.valid).toBe(true); // can't determine → don't block\n expect(result.actualSlug).toBeNull();\n });\n});\n\ndescribe('Concurrency & race guard: multiple parallel validations', () => {\n it('handles concurrent validations for same project without corruption', async () => {\n const n = 8;\n const promises = Array.from({ length: n }, () => validateRepoSlug('/path', 'protoLabsAI/ava'));\n const results = await Promise.all(promises);\n expect(results).toHaveLength(n);\n for (const r of results) {\n expect(r.valid).toBe(true);\n expect(r.actualSlug).toBe('protoLabsAI/ava');\n }\n });",
"verdict": "confirmed",
"note": "Re-read in the PR diff (new file +197 lines): the only beforeEach resetting h.failGit=false is scoped inside the validateRepoSlug() describe; the 'does NOT block when git command fails' test sets h.failGit=true with no reset (afterEach deletes only the env var), and the trailing 'Concurrency & race guard' describe has no beforeEach of its own, so the mock returns an Error, actualSlug stays null, and expect(r.actualSlug).toBe('protoLabsAI/ava') fails deterministically — evidence quote is byte-identical to the diff."
},
{
"file": "apps/server/src/services/project-health-service.ts",
"line": 114,
"severity": "minor",
"category": "cross-file",
"claim": "checkRepoAlignment's JSDoc advertises a 'settings-service-free entry point' for the board UI or other services, but the implementation dereferences this.settingsService unguarded — unlike the sibling auto-mode caller which guards `this.settingsService ? ... : null` — so on an instance without a wired SettingsService the documented query silently returns null from every call and never works, failing without error.",
"evidence": "JSDoc: \"This is a public, settings-service-free entry point so that the board UI or other services can query alignment without going through the full health compute pipeline.\" Implementation immediately below: \"const projectSettings = await this.settingsService\\n .getProjectSettings(projectPath)\" — and the sibling gate in auto-mode-service.ts guards the same dependency: \"const projectSettings = this.settingsService\\n ? await this.settingsService.getProjectSettings(projectPath).catch(() => null)\\n : null;\"",
"verdict": "confirmed",
"note": "All three quoted fragments are verbatim in the diff (JSDoc 'settings-service-free entry point'; unguarded `await this.settingsService.getProjectSettings(projectPath).catch(() => null)`; sibling guard `this.settingsService ? ... : null`). Caveat: constructor (read from main) declares settingsService a required, non-optional param, so the 'never works' scenario is JS-runtime-only, and the outer try/catch logs a warning rather than failing fully silently — the JSDoc/implementation contradiction and the missing null guard are real."
},
{
"file": "apps/server/src/services/repo-remote-validation.ts",
"line": 168,
"severity": "minor",
"category": "conventions",
"claim": "mismatchMessage is dead output: validateRepoSlug builds it on mismatch, but the only production consumer of the result (the auto-mode gate) discards it and re-invokes buildRepoSlugMismatchMessage with the same arguments, so the message string is computed twice and the field is never read by any caller.",
"evidence": " result.valid = false;\n result.mismatchMessage = buildRepoSlugMismatchMessage(\n projectPath,\n expectedRepoSlug,\n actualSlug\n );",
"verdict": "confirmed",
"note": "Evidence quote is verbatim in the diff. The auto-mode gate re-invokes `buildRepoSlugMismatchMessage(projectPath, slugValidation.expectedSlug!, slugValidation.actualSlug)` and never reads mismatchMessage; the health service reads only notConfigured/skipped/valid/expectedSlug/actualSlug; only the unit test reads the field (toBeDefined). New interface in a purely additive PR → no other consumers; field is production-dead and the message is computed twice."
},
{
"file": "apps/server/src/services/project-health-service.ts",
"line": 107,
"severity": "minor",
"category": "tests",
"claim": "The new behavior this fix depends on is untested: the PR's suite covers only the pure helper module, with zero tests for the new public checkRepoAlignment() method, the health -> 'off-track' branch, or the startAutoLoopForProject refusal gate that throws on mismatch.",
"evidence": " async checkRepoAlignment(projectPath: string): Promise<{\n aligned: boolean;\n expected?: string;\n actual?: string | null;\n } | null> {",
"verdict": "confirmed",
"note": "Method signature quote is verbatim in the diff. The PR adds exactly one test file (repo-remote-validation.test.ts) covering extractRepoSlug/readGitRemoteOrigin/validateRepoSlug/buildRepoSlugMismatchMessage only; no test exercises checkRepoAlignment(), the computeAndUpdate off-track branch, or the startAutoLoopForProject throw-on-mismatch gate (also made moot in practice by finding 1, where the added suite itself fails to run green)."
}
]1 finding(s) downgraded to uncertain: the code they quote as evidence does not appear in the file at the reviewed head, nor in this PR's patch for it. A finding that cannot be grounded does not gate a merge (issue #25) — it still stands for a human to judge.
apps/server/src/services/project-health-service.ts(minor) — quoted evidence not found at this head:this.settingsService ? ... : null
1 participant
Summary
[github] bug: project path → git remote misalignment corrupts cross-repo features
Situation
This feature was requested but PRD generation encountered an issue.
Problem
bug: project path → git remote misalignment corrupts cross-repo features
Bug
The local directory
/home/josh/dev/ava/contains a.automaker/project configured for ava-related work — but the git repo at that path points toprotoLabsAI/protoMaker, notprotoLabsAI/ava. Agents working features on the ava board e...Closes #4114
Created automatically by Automaker
Summary by CodeRabbit
New Features