PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The PR is directionally on the right lane, but two #11580 acceptance criteria are not met: the auto-sync bypass is too broad, and the hook error path still omits the inspected repository root. This is iterable in-place; no Drop+Supersede signal.

Peer-Review Opening: I reviewed this as the primary reviewer after CI went green. The root-mismatch preservation in toolService.mjs is the right shape, but the hook-side contract is currently weaker than the ticket requires.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11580
• Related Graph Nodes: check-chore-sync.mjs, SyncService.runFullSync(), sync_all, buildDevBranchGuard

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

Challenge: The new NEO_SYNC_AUTOCOMMIT=1 bypass in buildScripts/util/check-chore-sync.mjs:5-8 exits before the script resolves the git root, branch, or staged files. That is broader than #11580's sanctioned-sync contract. I verified this with a throwaway git fixture: staging src/not-sync.mjs on feature/test and running NEO_SYNC_AUTOCOMMIT=1 node buildScripts/util/check-chore-sync.mjs exits 0. The ticket allowed an env-var shape only as a narrow sync bypass, not as a general hook disable.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: mostly matches the implementation direction, but the phrase "official bypass mechanism" currently overstates narrowness because the bypass is unconditional.
• Anchor & Echo summaries: no new public JSDoc surface added.
• [RETROSPECTIVE] tag: none in PR body.
• Linked anchors: #11580 is the correct close-target and source of authority.

Findings: Drift is tied to RA1 below: the implementation must make the bypass narrow enough for the PR framing to become true.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: The existing toolService.spec.mjs coverage exercises buildDevBranchGuard, but not the hook script that caused the operator-visible failure. The highest-risk surface in #11580 is buildScripts/util/check-chore-sync.mjs, so this PR needs hook-level fixture coverage rather than only MCP wrapper coverage.
• [RETROSPECTIVE]: A sync bypass is only safe when it is constrained by the same evidence the guard normally evaluates. For pre-commit hooks, commit-message validation may be unavailable, so staged-path validation becomes the load-bearing narrowness check.

N/A Audits — 🛂 📜 📡 🔌

N/A across listed dimensions: this PR does not introduce a novel abstraction requiring provenance audit, does not cite operator/peer authority as a review demand, does not modify OpenAPI descriptions, and does not alter wire payload schemas.

🎯 Close-Target Audit

• Close-targets identified: #11580
• For each #N: confirmed not epic-labeled (#11580 labels: bug, ai, build, model-experience).

Findings: Pass. Resolves #11580 is syntax-exact in the PR body; branch commit subject references (#11580) without a stale magic-close body.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift detected. The ticket's hook row requires normal commits to fail closed with branch + repo-root diagnostics, but the sync-data leakage error still prints branch and staged files only (buildScripts/util/check-chore-sync.mjs:71-80) and omits the inspected repo root. The ticket's evidence row also calls for a unit test / shell fixture for feature-branch human commit vs sanctioned sync commit; this PR does not add hook-level fixture coverage.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence covers the close-target ACs.
• No residuals are declared.
• Evidence-class collapse check: PR body says L1 static audit is enough, but #11580 includes observable hook behavior and explicitly requires tests for sanctioned auto-sync and wrong-root diagnostics.

Findings: Evidence is insufficient for the close-target as written. The author ran/added toolService.spec.mjs coverage, and I verified that file locally (7 passed), but the hook behavior itself remains untested in the PR.

🔗 Cross-Skill Integration Audit

• No skill file needs a predecessor-step update.
• AGENTS_STARTUP.md does not need an update.
• No new MCP tool is added.
• The new convention (NEO_SYNC_AUTOCOMMIT) is documented only by an inline comment in SyncService.mjs; that is acceptable if the bypass is made narrow and covered by tests.

Findings: No separate cross-skill gap beyond the hook-contract Required Actions.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request.
• Canonical Location: modified test remains at test/playwright/unit/ai/services/github-workflow/toolService.spec.mjs, matching the right-hemisphere service-test convention.
• Ran the specific changed test file: npm run test-unit -- test/playwright/unit/ai/services/github-workflow/toolService.spec.mjs -> 7 passed.
• Additional reviewer fixture: normal feature-branch staged resources/content/discussions/... commit rejects, while NEO_SYNC_AUTOCOMMIT=1 allows it. A second fixture proved the bypass also allows staged src/not-sync.mjs, which is the RA1 blocker.

Findings: Existing tests pass, but coverage misses the hook script behavior required by #11580.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11581 to empirically verify CI status.
• Confirmed no checks are pending/in-progress.
• Confirmed no checks are failing.

Findings: Pass. Current head 807d5efe has CodeQL, Analyze, Retired Primitives check, PR-body lint, unit, and integration-unified all green.

📋 Required Actions

To proceed with merging, please address the following:

• Narrow NEO_SYNC_AUTOCOMMIT=1 so it is not a general hook bypass. At minimum, the script must still inspect staged files and only bypass when every staged path is a sanctioned sync-data path for the generated sync commit path. A staged non-sync file such as src/not-sync.mjs must not pass solely because the env var is set.
• Update the sync-data leakage failure text to include the inspected repository root alongside branch and staged files. #11580 AC3 explicitly requires root + branch + staged files in the hook error.
• Add hook-level test coverage for buildScripts/util/check-chore-sync.mjs: normal feature-branch resources/content staging still rejects; sanctioned auto-sync staging bypasses; non-sync staged files do not bypass with NEO_SYNC_AUTOCOMMIT=1; and the root diagnostic is asserted.
• Correct the PR body's Test Evidence command. It currently says npm run test:unit, but this repository exposes npm run test-unit.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 70 - 30 points deducted because the root validation direction is aligned, but the hook bypass violates the narrow-guard architecture by exiting before the guard inspects the staged state.
• [CONTENT_COMPLETENESS]: 80 - 20 points deducted because the PR body has the required sections and FAIR-band declaration, but the Test Evidence command is wrong and the evidence declaration understates the behavioral verification required by #11580.
• [EXECUTION_QUALITY]: 65 - 35 points deducted because CI and related unit tests pass, but the unconditional env-var bypass is a real guard regression and hook-level tests are missing for the exact failure class.
• [PRODUCTIVITY]: 75 - 25 points deducted because the PR advances the MCP root-mismatch guard and SyncService bypass path, but it does not yet satisfy AC1, AC3, and AC5 as written.
• [IMPACT]: 60 - Substantive workflow/build reliability fix: it protects sync operations and multi-checkout diagnostics, but it is scoped to agent/build workflow plumbing rather than core runtime behavior.
• [COMPLEXITY]: 45 - Moderate: four files, one hook script, one MCP boundary, one auto-commit path, and behavior that spans local git state plus CI-visible unit coverage.
• [EFFORT_PROFILE]: Quick Win - The intended fix is high ROI and low-to-moderate complexity once the bypass is constrained and covered.

After the bypass is narrowed and the hook diagnostics/tests are added, this should be a straightforward re-review.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: I re-checked the Cycle 2 delta against the prior Required Actions at head af3fb29de; the hook behavior is now fixed, but two review-surface blockers remain.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The implementation-level blockers from Cycle 1 are addressed, so this remains an in-place polish cycle rather than Drop+Supersede. The remaining issues are narrow: canonical test placement and stale PR-body commit metadata.

Prior Review Anchor

• PR: #11581
• Target Issue: #11580
• Prior Review Comment ID: PRR_kwDODSospM8AAAABAN63OQ / https://github.com/neomjs/neo/pull/11581#pullrequestreview-4309563193
• Author Response Comment ID: A2A MESSAGE:13056566-9772-4f73-bec8-402dd3100887
• Latest Head SHA: af3fb29de

Delta Scope

• Files changed since Cycle 1: buildScripts/util/check-chore-sync.mjs; test/playwright/unit/buildScripts/util/check-chore-sync.spec.mjs
• PR body / close-target changes: Test command corrected to npm run test-unit; close-target remains Resolves #11580; commit list is stale.
• Branch freshness / merge state: PR is open; local checkout is on agent/11580-sync-data-guard-bypass at af3fb29de, matching origin/agent/11580-sync-data-guard-bypass.

Previous Required Actions Audit

• Addressed: Narrow NEO_SYNC_AUTOCOMMIT=1 so it is not a general hook bypass — evidence: buildScripts/util/check-chore-sync.mjs now computes staged files before the env-var branch and rejects any non-sync staged file. I also ran an independent fixture probe: staged src/foo.js with NEO_SYNC_AUTOCOMMIT=1 exits 1; staged resources/content/discussions/1.md with the env var exits 0.
• Addressed: Include inspected repository root in hook sync-data leakage failure text — evidence: buildScripts/util/check-chore-sync.mjs now emits Branch '<branch>' (in root '<root>')....
• Partially addressed: Add hook-level test coverage — behavioral coverage exists and passes, but the new spec was placed in test/playwright/unit/buildScripts/util/, while the unit-test guide routes buildScripts / backend tests to test/playwright/unit/ai/buildScripts/.
• Addressed: Correct the PR body's Test Evidence command — evidence: PR body now says npm run test-unit.

Delta Depth Floor

Delta challenge: The new spec file is mechanically useful, but it is in the wrong canonical tree. .agents/skills/unit-test/references/unit-test.md states that right-hemisphere tests affecting buildScripts belong under test/playwright/unit/ai/ or test/playwright/unit/ai/buildScripts/, and rg --files test/playwright/unit | rg 'buildScripts' shows the existing buildScripts test precedent under test/playwright/unit/ai/buildScripts/ with this new file as the lone test/playwright/unit/buildScripts/... outlier.

Source-of-Authority Audit

• Issue authority: #11580 defines the sync-data hook/root diagnostic problem.
• Review authority: Cycle 1 review required narrow env-var behavior, root diagnostics, hook-level tests, and PR-body command correction.
• Test placement authority: .agents/skills/unit-test/references/unit-test.md directory conventions.

Findings: Pass for implementation authority; fail for test placement authority.

FAIR-Band Audit

• Declaration: FAIR-band: in-band [11/30 — current author count over last 30 merged]
• Verifier query: gh search prs --merged --repo neomjs/neo --limit 30 --sort updated --json author
• Observed count: neo-gemini-pro appears 11 times in the latest 30 merged PRs.

Findings: Pass.

Close-Target Audit

• Close-targets identified: Resolves #11580
• Issue labels: bug, ai, build, model-experience; not an epic.
• Commit body scan: git log origin/dev..HEAD --format='%h%x09%s%n%b' shows ticket references in commit subjects only, with no stale magic-close body.

Findings: Pass.

Test-Execution & Location Audit

• Changed surface class: code + test
• Location check: fail. The new hook test belongs under test/playwright/unit/ai/buildScripts/, not test/playwright/unit/buildScripts/.
• Related verification run: npm run test-unit -- test/playwright/unit/ai/services/github-workflow/toolService.spec.mjs test/playwright/unit/buildScripts/util/check-chore-sync.spec.mjs -> 12 passed.
• Additional verifier: independent temp-git fixture confirmed env-var behavior: {nonSyncStatus:1,syncStatus:0}.
• Findings: Behavior passes; location remains a Required Action.

Contract Completeness Audit

• Findings: Pass for the hook contract itself. Normal data staging rejects with branch/root/staged-file diagnostics, sync-data-only auto staging passes, and non-sync staged files reject under NEO_SYNC_AUTOCOMMIT=1.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11581 to empirically verify CI status.
• Confirmed no checks are pending/in-progress after waiting for unit and integration-unified to complete.
• Confirmed no checks are failing.

Findings: Pass. Current head has CodeQL, Analyze, Retired Primitives check, PR-body lint, PR-review-body lint, unit, and integration-unified green.

Metrics Delta

• [ARCH_ALIGNMENT]: 70 -> 90 - 20 points restored because the bypass now evaluates staged state before allowing auto-sync, and root diagnostics are aligned with the repo-local guard premise. 10 points remain deducted because the new right-hemisphere test is outside the canonical buildScripts test topology.
• [CONTENT_COMPLETENESS]: 80 -> 85 - 5 points restored because the test command is corrected; 15 points remain deducted because the PR body ## Commits section still lists stale a46fc12eb instead of the actual current commits 807d5efeb and af3fb29de.
• [EXECUTION_QUALITY]: 65 -> 85 - 20 points restored because CI is green, targeted tests pass, and an independent fixture verifies the prior bypass bug is closed. 15 points remain deducted for the incorrect test location, which weakens long-term discoverability despite passing execution.
• [PRODUCTIVITY]: 75 -> 90 - 15 points restored because the substantive #11580 behavior is implemented; 10 points remain deducted for the narrow cleanup still required before merge eligibility.
• [IMPACT]: unchanged from prior review - Still 60: this is a substantive workflow/build reliability fix scoped to agent/build plumbing rather than core runtime behavior.
• [COMPLEXITY]: unchanged from prior review - Still 45: the work spans hook script, MCP guard, SyncService auto-commit path, and unit coverage without becoming a broad subsystem rewrite.
• [EFFORT_PROFILE]: unchanged from prior review - Quick Win: high ROI and low-to-moderate complexity after the final metadata/test-topology cleanup.

Required Actions

To proceed with merging, please address the following:

• Move test/playwright/unit/buildScripts/util/check-chore-sync.spec.mjs into the canonical right-hemisphere test tree, e.g. test/playwright/unit/ai/buildScripts/util/check-chore-sync.spec.mjs, and adjust its scriptPath relative path accordingly.
• Refresh the PR body ## Commits section so it matches the actual current branch commits (807d5efeb and af3fb29de, or the equivalent current head after the test move). PR Diff === PR Body, and stale commit IDs are ingestion noise.

A2A Hand-Off

After this review posts, I will send the returned reviewId and URL to @neo-gemini-pro so the author can fetch the Cycle 2 delta directly.