Blind review: R12 trusted authority evolution - #405
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughДобавлены R12 trusted-base валидаторы для обычных PR и maintenance-переходов, сценарный harness, обновления CI-конформности, successor self-test и evidence-отчёты с результатами security-проверок. ChangesR12 trusted authority maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant AuthorityGuard
participant TrustedGit
participant Validators
participant Evidence
PullRequest->>AuthorityGuard: open, synchronize or maintenance label
AuthorityGuard->>TrustedGit: fetch trusted base and candidate refs
TrustedGit-->>AuthorityGuard: commits, trees, blobs and diff
AuthorityGuard->>Validators: run authority and maintenance checks
Validators-->>AuthorityGuard: PASS/FAIL verdict and details
AuthorityGuard->>Evidence: write and upload JSON artifact
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review\n\nPlease perform an independent adversarial review of this PR. Focus on behavioral edges (wrong JSON types, raw/normalized values, silent acceptance, subset/full truth), privileged workflow execution, Git races, and two-epoch liveness. Do not infer correctness from green tests. |
|
@gemini-code-assist review\n\nPlease act as a blind judge. Independently verify the security and liveness contract, inspect sibling/generic validation paths, and return severity-labeled file:line findings plus an explicit merge verdict. |
|
@coderabbitai review\n\nBlind focus: derive trust-boundary, recurring-epoch, strict-type, Git-ref/blob, and CI ownership findings independently. Please cite file:line and separate blockers from optional notes. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
REPO="thebtf/engram"
PR=405
echo "=== PR metadata ==="
gh pr view $PR --repo $REPO --json title,body,baseRefName,headRefName,files,additions,deletions,changedFiles 2>&1 | head -c 3000
echo "=== Files changed ==="
gh pr view $PR --repo $REPO --json files --jq '.files[].path' 2>&1
echo "=== Diff (first 2000 lines) ==="
gh pr diff $PR --repo $REPO 2>&1 | head -c 20000Length of output: 24505 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="thebtf/engram"
PR=405
mkdir -p /tmp/pr405
cd /tmp/pr405
for f in ".github/workflows/authority-guard.yml" ".github/workflows/test.yml"; do
echo "=== $f ==="
gh api "repos/$REPO/contents/$f?ref=1dc6b91c" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null | cat -n || echo "FAILED to fetch $f"
echo
doneLength of output: 111647 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="thebtf/engram"
for f in "scripts/production-gates/assert-pr-authority-guard.ps1" "scripts/production-gates/assert-pr-authority-maintenance.ps1"; do
echo "=== $f ==="
gh api "repos/$REPO/contents/$f?ref=1dc6b91c" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null | cat -n
echo
doneLength of output: 99412
Trust boundary (verified sound)
FindingsMEDIUM — Concurrency cancellation can grief an in-flight owner-approved maintenance run (liveness). MEDIUM — The "authority-guard" required-status binding is unverifiable from this diff. LOW — Protected-path prefix gap for plan-governance evidence files. INFO — Base ref is fetched by branch name, not pinned SHA. INFO — Verified structural claims (not just asserted)
Merge verdictNo exploitable privilege-escalation or trust-boundary bypass found in this pass — the design is fail-closed almost everywhere traced. The two MEDIUM items (label-driven cancellation griefing, and the branch-protection required-status dependency being unverifiable from code) should be explicitly acknowledged or ruled out before treating this as a fully closed security review; neither is a hard blocker to merging this PR itself, but the second one determines whether any of this machinery is actually load-bearing in production. ✅ Action performedReview finished.
|
![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: 1dc6b91c8c
ℹ️ 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".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the R12 authority maintenance and PR authority guard scripts, tests, and evidence files, along with support for 'replacement' transition kinds in plan path ownership. The review feedback focuses on improving the robustness of the PowerShell scripts, specifically by ensuring case-insensitive Git SHA comparisons, making YAML parsing and permissions checks resilient to comments and trailing whitespace, and maintaining consistent property access using helper functions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.agent/specs/release-gates-r12/evidence/release-gates/security-review.md (1)
62-64: 🔒 Security & Privacy | 🔵 TrivialЗафиксируйте зависимость от live branch protection.
Привязка имени required status внутри контракта не доказывает, что
mainдействительно требуетauthority-guardи запрещает административный bypass. Добавьте live API evidence или явно укажите это как residual operational risk.🤖 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 @.agent/specs/release-gates-r12/evidence/release-gates/security-review.md around lines 62 - 64, Update the maintenance transition evidence to verify via live branch-protection API data that main requires the authority-guard status and disallows administrative bypass; if this evidence cannot be provided, explicitly record the dependency as a residual operational risk alongside the bound required-status identity.
🤖 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
@.agent/specs/release-gates-r12/evidence/release-gates/R12-AUTHORITY-MAINTENANCE.tdd.json:
- Around line 30-32: Update the workflow_conformance evidence in
R12-AUTHORITY-MAINTENANCE.tdd.json so hostile_mutations_rejected matches the
current .github/workflows/test.yml value of 71. Regenerate or refresh the
artifact from the final conformance test rather than changing unrelated verdict
data.
In
@.agent/specs/release-gates-r12/evidence/release-gates/test-r12-authority-maintenance.ps1:
- Around line 372-375: Добавьте в test-r12-authority-maintenance.ps1 (372-375)
ordinary-сценарий, который через Reset-Fixture/Push-Head изменяет файл в
.agent/specs/release-gates-r12/evidence/plan-governance/ и проверяет отказ через
Invoke-Scenario с ожидаемой ошибкой authority. В security-review.md (58-61)
уберите абсолютное утверждение о защите plan-governance до исправления guard и
регенерации evidence.
In @.github/workflows/authority-guard.yml:
- Around line 17-19: Separate the maintenance-stage concurrency from the general
authority-guard group: update the workflow’s concurrency configuration so
maintenance-label runs use a distinct group key, while preserving cancellation
behavior within that maintenance group and preventing
labeled/unlabeled/synchronize runs from canceling an owner-approved maintenance
check.
In `@scripts/production-gates/assert-plan-path-ownership.ps1`:
- Around line 842-880: Привяжите replacement evidence к $requiredBase, а не
только к формату SHA: в ветке проверки replacement для REVISE_HOLD требуйте
точное равенство rejected_head_sha и requiredBase, а для
ROOT_SELECTED_EXACT_BASE — выбранной immutable identity (integration_sha или
predecessor_head_sha) и requiredBase. Обновите Invoke-DiffEpochAuthority, чтобы
replacement проходил те же строгие проверки базовой ревизии, что и rework.
Добавьте negative mismatch self-test, подтверждающий отклонение evidence с
несовпадающим base.
In `@scripts/production-gates/assert-pr-authority-guard.ps1`:
- Around line 119-124: Update Test-ProtectedPath in
scripts/production-gates/assert-pr-authority-guard.ps1 (lines 119-124) to treat
.agent/specs/release-gates-r12/evidence/plan-governance/ as protected from
ordinary PR changes. Update the corresponding namespace allowlist in
scripts/production-gates/assert-pr-authority-maintenance.ps1 (lines 148-151) to
permit it only as a bounded owner-approved maintenance change.
- Around line 151-167: Update the pending-envelope checks so
forbidden_final_paths are enforced for prefix matches as well as exact matches.
In scripts/production-gates/assert-pr-authority-guard.ps1:151-167, validate each
prefix-covered path against the pending contract’s forbidden_final_paths before
returning pending-prefix-envelope. In
scripts/production-gates/assert-active-candidate-path-authority.ps1:364-382,
apply the same forbidden-path intersection check to exact_prefixes, preserving
authorization only when no forbidden path is matched.
---
Nitpick comments:
In @.agent/specs/release-gates-r12/evidence/release-gates/security-review.md:
- Around line 62-64: Update the maintenance transition evidence to verify via
live branch-protection API data that main requires the authority-guard status
and disallows administrative bypass; if this evidence cannot be provided,
explicitly record the dependency as a residual operational risk alongside the
bound required-status identity.
🪄 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: CHILL
Plan: Pro
Run ID: 5a793f3a-7c13-468e-bf39-c66ef5c29786
📒 Files selected for processing (11)
.agent/specs/release-gates-r12/evidence/release-gates/R12-AUTHORITY-MAINTENANCE.tdd.json.agent/specs/release-gates-r12/evidence/release-gates/maintenance-simulation.json.agent/specs/release-gates-r12/evidence/release-gates/security-review.md.agent/specs/release-gates-r12/evidence/release-gates/test-r12-authority-maintenance.ps1.agent/specs/release-gates-r12/evidence/release-gates/windows-harness.json.github/workflows/authority-guard.yml.github/workflows/test.ymlscripts/production-gates/assert-active-candidate-path-authority.ps1scripts/production-gates/assert-plan-path-ownership.ps1scripts/production-gates/assert-pr-authority-guard.ps1scripts/production-gates/assert-pr-authority-maintenance.ps1
There was a problem hiding this comment.
Code Review
This pull request introduces the R12 trusted authority maintenance and PR authority guard framework, including new PowerShell validation scripts, simulation tests, and security reviews. Feedback on the changes highlights potential runtime errors in assert-active-candidate-path-authority.ps1 where direct property access on deserialized objects under strict mode can trigger PropertyNotFoundExceptions; using the Get-PropertyValue helper is recommended. Additionally, the YAML block parser in assert-pr-authority-maintenance.ps1 should be made more robust by replacing fragile exact string matching with regular expressions to handle trailing whitespace and comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
External-review repair is ready at exact head 22ec847. Please review the new delta and the full resulting security contract. Local evidence: AST/actionlint PASS; ledger 60 slices / 396 declarations / 0 errors; active authority PASS; plan governance PASS; workflow conformance 71 hostile mutations rejected; two consecutive isolated authority simulations PASS 22/22 each (4 accepts, 18 rejects, cleanup verified); staged gitleaks PASS. @coderabbitai review @codex review @gemini-code-assist review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="thebtf/engram"
PR=405
NEW_HEAD="22ec8476f65050919820f6bdff6cf8d3573ecf8e"
echo "=== PR current head ==="
gh pr view $PR --repo $REPO --json headRefOid,commits --jq '.headRefOid'
echo "=== Compare diff between previously reviewed commit and new head ==="
gh api "repos/$REPO/compare/1dc6b91c...$NEW_HEAD" --jq '.files[].filename' 2>&1 || echo "compare failed"
echo "=== Full diff for changed files (workflows + scripts) ==="
for f in ".github/workflows/authority-guard.yml" "scripts/production-gates/assert-pr-authority-guard.ps1" "scripts/production-gates/assert-pr-authority-maintenance.ps1"; do
echo "--- diff for $f ---"
gh api "repos/$REPO/compare/1dc6b91c...$NEW_HEAD" --jq ".files[] | select(.filename==\"$f\") | .patch" 2>&1 | head -c 20000
echo
doneLength of output: 31988 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="thebtf/engram"
NEW_HEAD="22ec8476f65050919820f6bdff6cf8d3573ecf8e"
for f in "scripts/production-gates/assert-active-candidate-path-authority.ps1" "scripts/production-gates/assert-plan-path-ownership.ps1"; do
echo "--- diff for $f ---"
gh api "repos/$REPO/compare/1dc6b91c...$NEW_HEAD" --jq ".files[] | select(.filename==\"$f\") | .patch" 2>&1 | head -c 12000
echo
done
echo "=== security-review.md diff (check acknowledgment of prior findings) ==="
gh api "repos/$REPO/compare/1dc6b91c...$NEW_HEAD" --jq '.files[] | select(.filename=="'".agent/specs/release-gates-r12/evidence/release-gates/security-review.md"'") | .patch' 2>&1 | head -c 6000Length of output: 21515
Fixes verifiedMEDIUM — concurrency griefing (resolved). MEDIUM — required-status/branch-protection binding (acknowledged, not a code defect). LOW — protected-path prefix gap (resolved). Additional hardening noted (beyond the prior review's asks)
Merge verdictNo new trust-boundary or privilege-escalation issue found in this delta. Both MEDIUM items and the LOW item from the previous pass are correctly resolved or explicitly and honestly flagged as a pending non-code activation step. The additional strict-typing and skeleton-diff hardening are net-positive and don't introduce new gaps I could find. From a code-review standpoint this is clear to merge; the one remaining item — actually enabling the |
There was a problem hiding this comment.
Code Review
This pull request implements the R12 trusted authority maintenance security specifications, including simulation tests, verification scripts, and updated contract auditing logic. The review feedback highlights several critical improvements for the PowerShell scripts: enforcing case-sensitive SHA and blob validation using -cnotmatch instead of -notmatch, allowing both [int] and [long] types to prevent failures on PowerShell Core 7.3+, checking for consecutive slashes in canonical path validation, and making YAML block parsing and workflow validation less fragile to minor formatting variations or comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22ec8476f6
ℹ️ 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".
|
R5 external-review repair is ready at exact head cc24740. Please review the new delta and the full resulting security/liveness contract independently. Evidence on the unchanged final tree: AST/actionlint PASS; ownership ledger 60 slices / 396 declarations / 0 errors; active authority PASS; R12 plan governance PASS; workflow conformance rejected all 71 hostile mutations; two consecutive isolated authority simulations PASS 24/24 each (4 accepts, 20 expected rejects, cleanup verified); evidence SHA map 14/14; staged gitleaks PASS. This revision explicitly reverses the earlier uppercase-SHA response after proving PowerShell matching is case-insensitive by default and now enforces canonical lowercase identities at runtime with -cnotmatch. Please do not infer correctness from the tests: re-check null/missing types, closed-world statuses, root-comment parsing, path aliases, privileged execution, recurring epochs, and exact workflow language. @coderabbitai review @codex review @gemini-code-assist review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="thebtf/engram"
PR=405
PREV_HEAD="22ec8476f65050919820f6bdff6cf8d3573ecf8e"
NEW_HEAD="cc247402f2309b53285c88367d9ca33696ff278f"
echo "=== confirm current PR head ==="
gh pr view $PR --repo $REPO --json headRefOid --jq '.headRefOid'
echo "=== files changed in delta ==="
gh api "repos/$REPO/compare/$PREV_HEAD...$NEW_HEAD" --jq '.files[].filename'
echo "=== full patches ==="
gh api "repos/$REPO/compare/$PREV_HEAD...$NEW_HEAD" --jq '.files[] | "--- " + .filename + " ---\n" + .patch' 2>&1 | head -c 60000Length of output: 55629 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="thebtf/engram"
NEW_HEAD="cc247402f2309b53285c88367d9ca33696ff278f"
echo "=== search for any remaining case-insensitive hex/sha matches in the 4 script files ==="
for f in "scripts/production-gates/assert-pr-authority-guard.ps1" "scripts/production-gates/assert-pr-authority-maintenance.ps1" "scripts/production-gates/assert-active-candidate-path-authority.ps1" "scripts/production-gates/assert-plan-path-ownership.ps1"; do
echo "--- $f ---"
gh api "repos/$REPO/contents/$f?ref=$NEW_HEAD" --jq '.content' | base64 -d | grep -nP '(?<!c)notmatch.*\[0-9a-f\]' || echo "none"
echo
done
echo "=== check full Get-OptionalArrayProperty usage sites in guard + maintenance scripts ==="
for f in "scripts/production-gates/assert-pr-authority-guard.ps1" "scripts/production-gates/assert-pr-authority-maintenance.ps1"; do
echo "--- $f ---"
gh api "repos/$REPO/contents/$f?ref=$NEW_HEAD" --jq '.content' | base64 -d | grep -n "Get-OptionalArrayProperty\|Get-OptionalStringArray"
echo
doneLength of output: 2762
Case-sensitivity reversal (verified correct and complete)PowerShell's
This is a legitimate, evidence-led reversal, not just a written retraction — good catch on your end. Other findings verified as correctly closed
Merge verdictNo new trust-boundary, privilege-escalation, or type-confusion issue found in this delta. The case-sensitivity fix is real and closes a genuine (if narrow) identity-comparison gap; the other four additions (root-comment parsing, double-slash paths, null-prefix filtering, closed-world pending status) are each backed by a corresponding hostile fixture that fails without the fix and passes with it. From a code-review standpoint this delta is clear. The two previously-flagged non-code items remain outstanding and unaffected by this delta: (1) confirming the ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
This pull request implements the R12/B17 trusted authority maintenance system, adding new validation scripts, test suites, and simulation logs, while updating existing path authority and plan ownership checks to support case-sensitive SHA validation and replacement transitions. The review feedback highlights several critical issues: an unsafe substring operation in the prefix matching logic of assert-pr-authority-guard.ps1 that could crash the script, missing System. prefixes for CultureInfo and DateTimeStyles types in multiple files causing runtime TypeNotFound exceptions, and unhandled Remove-Item failures in finally blocks that could mask primary exceptions during cleanup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc247402f2
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/production-gates/assert-active-candidate-path-authority.ps1 (1)
385-385: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winБаг:
$macroBoundложно истинен при отсутствующих/nullmember_slices/exact_r12_overrides.
@((Get-PropertyValue $pending 'member_slices')).Count— если свойство отсутствует или явноnull,Get-PropertyValueвозвращает скалярный$null, а@($null)в PowerShell создаёт массив из ОДНОГО элемента (Count=1), а не пустой массив (Count=0). Таким образом, pending-namespace сauthority_modeравным macro-константе, но без реальныхmember_slices/exact_r12_overrides, ложно проходит-gt 0проверку и обходит ветку "declares no bounded path" (строка 386).Прямой эксплойт через
assert-pr-authority-guard.ps1не подтверждён (тамFind-Authorizationне читаетmember_slices/macroBound), но это брешь в полноте структурной проверки самого R12-аудита — контракт с фиктивным macro-bound namespace без реальных полномочий проходит как валидный.Тот же паттерн уже правильно обёрнут через null-safe хелперы (
Get-OptionalStringArray) в остальных местах этой же функции — здесь стоит использовать тот же подход.🐛 Предлагаемое исправление
- $macroBound = [string](Get-PropertyValue $pending 'authority_mode') -ceq 'bound-plan-member-union-with-exact-r12-overrides' -and @((Get-PropertyValue $pending 'member_slices')).Count -gt 0 -and @((Get-PropertyValue $pending 'exact_r12_overrides')).Count -gt 0 + $macroBound = [string](Get-PropertyValue $pending 'authority_mode') -ceq 'bound-plan-member-union-with-exact-r12-overrides' -and @(Get-OptionalStringArray $pending 'member_slices').Count -gt 0 -and @(Get-OptionalStringArray $pending 'exact_r12_overrides').Count -gt 0🤖 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 `@scripts/production-gates/assert-active-candidate-path-authority.ps1` at line 385, Исправьте вычисление $macroBound в проверке authority_mode, заменив прямые вызовы Get-PropertyValue для member_slices и exact_r12_overrides на существующий null-safe helper Get-OptionalStringArray. Сохраните проверку непустых массивов через Count -gt 0, чтобы отсутствующие или null-свойства давали Count=0 и не считались bounded path.
🧹 Nitpick comments (2)
scripts/production-gates/assert-active-candidate-path-authority.ps1 (1)
407-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winПереиспользование автоматической переменной
$matches.
$matches— зарезервированное имя (заполняется оператором-match/-cmatch). Использование его как обычной переменной для результатаWhere-Object(как в этом же файле для R9-ветки, где для аналогичного кода используется$matchingPending, строка 565) создаёт риск скрытой порчи данных при будущих правках, если в этой же области видимости появится вызов-match. Статический анализ (PSScriptAnalyzer, PSAvoidAssignmentToAutomaticVariable) также это отмечает.♻️ Предлагаемое исправление
- $matches = @($pendingContracts | Where-Object { [string](Get-PropertyValue $_ 'slice') -ceq $probeSlice }) - if ($matches.Count -ne 1) { $errors.Add("pending probe slice '$probeSlice' has $($matches.Count) contracts, expected 1") } - elseif ($probeBase -cne [string](Get-PropertyValue $matches[0] 'base_anchor')) { $errors.Add("pending probe base '$probeBase' must equal frozen base anchor '$([string](Get-PropertyValue $matches[0] 'base_anchor'))'") } + $matchingPending = @($pendingContracts | Where-Object { [string](Get-PropertyValue $_ 'slice') -ceq $probeSlice }) + if ($matchingPending.Count -ne 1) { $errors.Add("pending probe slice '$probeSlice' has $($matchingPending.Count) contracts, expected 1") } + elseif ($probeBase -cne [string](Get-PropertyValue $matchingPending[0] 'base_anchor')) { $errors.Add("pending probe base '$probeBase' must equal frozen base anchor '$([string](Get-PropertyValue $matchingPending[0] 'base_anchor'))'") }(и аналогично заменить оставшиеся обращения к
$matches[0]на$matchingPending[0]внутри этого блока, строки 419-426).🤖 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 `@scripts/production-gates/assert-active-candidate-path-authority.ps1` around lines 407 - 431, Replace the local `$matches` variable in the pending-probe block with a non-reserved name such as `$matchingPending`, and update all references to it, including the count check and `$matchingPending[0]` accesses. Preserve the existing contract validation and audit behavior.Source: Linters/SAST tools
.agent/specs/release-gates-r12/evidence/release-gates/test-r12-authority-maintenance.ps1 (1)
197-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew-MaintenanceHead: корректно, но обратите внимание на алиасинг
exact_changes.
$manifest.exact_changes = $changes(строка 284) не создаёт копию — это та же ссылка, что и$maintenance.active_epoch.exact_changes. ЛюбаяManifestMutation, изменяющая элементыexact_changes(например, сценарийmaintenance-double-slash-path, строки 507-511), попадёт и в$successor.exact_changesпри JSON round-trip на строке 290, поскольку копирование происходит уже после мутации. Сейчас это не искажает итог теста (голова с испорченным путём отбраковывается и не мержится в main), но при добавлении новых сценариев, где манифест должен отличаться от фактического состояния эпохи, это может незаметно "протечь" в состояние следующей эпохи.♻️ Возможное исправление (глубокое копирование перед мутацией)
- exact_changes = $changes + exact_changes = ($changes | ConvertTo-Json -Depth 100 | ConvertFrom-Json -Depth 100)🤖 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 @.agent/specs/release-gates-r12/evidence/release-gates/test-r12-authority-maintenance.ps1 around lines 197 - 311, В функции New-MaintenanceHead устраните aliasing между manifest.exact_changes и maintenance.active_epoch.exact_changes: создайте независимую глубокую копию exact_changes до вызова ManifestMutation, а манифест собирайте из этой копии. Сохраните исходные изменения эпохи неизменными, чтобы последующее формирование successor через JSON round-trip не перенимало мутации манифеста.
🤖 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.
Outside diff comments:
In `@scripts/production-gates/assert-active-candidate-path-authority.ps1`:
- Line 385: Исправьте вычисление $macroBound в проверке authority_mode, заменив
прямые вызовы Get-PropertyValue для member_slices и exact_r12_overrides на
существующий null-safe helper Get-OptionalStringArray. Сохраните проверку
непустых массивов через Count -gt 0, чтобы отсутствующие или null-свойства
давали Count=0 и не считались bounded path.
---
Nitpick comments:
In
@.agent/specs/release-gates-r12/evidence/release-gates/test-r12-authority-maintenance.ps1:
- Around line 197-311: В функции New-MaintenanceHead устраните aliasing между
manifest.exact_changes и maintenance.active_epoch.exact_changes: создайте
независимую глубокую копию exact_changes до вызова ManifestMutation, а манифест
собирайте из этой копии. Сохраните исходные изменения эпохи неизменными, чтобы
последующее формирование successor через JSON round-trip не перенимало мутации
манифеста.
In `@scripts/production-gates/assert-active-candidate-path-authority.ps1`:
- Around line 407-431: Replace the local `$matches` variable in the
pending-probe block with a non-reserved name such as `$matchingPending`, and
update all references to it, including the count check and `$matchingPending[0]`
accesses. Preserve the existing contract validation and audit behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c85bdd96-ce9d-4e29-b13b-546b845ebca2
📒 Files selected for processing (14)
.agent/plans/2026-07-10-engram-production-ready-active-diff-contracts.json.agent/specs/release-gates-r12/evidence/plan-governance/gates-summary.json.agent/specs/release-gates-r12/evidence/plan-governance/test-r12-plan-governance.ps1.agent/specs/release-gates-r12/evidence/plan-governance/verification-summary.json.agent/specs/release-gates-r12/evidence/release-gates/R12-AUTHORITY-MAINTENANCE.tdd.json.agent/specs/release-gates-r12/evidence/release-gates/maintenance-simulation.json.agent/specs/release-gates-r12/evidence/release-gates/security-review.md.agent/specs/release-gates-r12/evidence/release-gates/test-r12-authority-maintenance.ps1.agent/specs/release-gates-r12/evidence/release-gates/windows-harness.json.github/workflows/authority-guard.ymlscripts/production-gates/assert-active-candidate-path-authority.ps1scripts/production-gates/assert-plan-path-ownership.ps1scripts/production-gates/assert-pr-authority-guard.ps1scripts/production-gates/assert-pr-authority-maintenance.ps1
🚧 Files skipped from review as they are similar to previous changes (2)
- .agent/specs/release-gates-r12/evidence/release-gates/security-review.md
- .agent/specs/release-gates-r12/evidence/release-gates/R12-AUTHORITY-MAINTENANCE.tdd.json
|
Final R6 repair is ready at exact head 6a942f8. Please perform one final independent full-delta review. R6 closes field-kind confusion (and moves the live IMAGE directory to exact_prefixes), anchors all R12 status classes to literal trusted-validator sets, rejects malformed prefixes before substring logic, and preserves primary errors across cleanup failures. Final byte-exact evidence on separate GUID fixture histories: two parallel isolated simulations PASS 24/24 each (4 accepts, 20 expected rejects; durations 401.433s and 401.417s), plan governance/active authority/ledger PASS (60 slices, 396 declarations, 0 errors), 71 workflow mutations rejected, AST/actionlint PASS, SHA map 14/14, gitleaks 0. Please inspect behavior rather than trusting those results. @coderabbitai review @codex review @gemini-code-assist review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="thebtf/engram"
PR=405
PREV_HEAD="cc247402f2309b53285c88367d9ca33696ff278f"
NEW_HEAD="6a942f80e8a9fba499af9e6d52898a3fa633c477"
echo "=== confirm current PR head ==="
gh pr view $PR --repo $REPO --json headRefOid --jq '.headRefOid'
echo "=== files changed in delta ==="
gh api "repos/$REPO/compare/$PREV_HEAD...$NEW_HEAD" --jq '.files[].filename'
echo "=== full patches ==="
gh api "repos/$REPO/compare/$PREV_HEAD...$NEW_HEAD" --jq '.files[] | "--- " + .filename + " ---\n" + .patch' 2>&1 | head -c 70000Length of output: 34287 🏁 Script executed: #!/bin/bash
set -euo pipefail
REPO="thebtf/engram"
SHA="6a942f80e8a9fba499af9e6d52898a3fa633c477"
echo "=== Normalize-AuthorityPath definition ==="
gh api "repos/$REPO/contents/scripts/production-gates/assert-active-candidate-path-authority.ps1?ref=$SHA" --jq '.content' | base64 -d | grep -n "function Normalize-AuthorityPath" -A 30
echo "=== other Substring usages on prefixes in same file ==="
gh api "repos/$REPO/contents/scripts/production-gates/assert-active-candidate-path-authority.ps1?ref=$SHA" --jq '.content' | base64 -d | grep -n "Substring\|forbidden\|exact_prefixes"
echo "=== test-r12-authority-maintenance.ps1 surrounding errorText/exit logic ==="
gh api "repos/$REPO/contents/.agent/specs/release-gates-r12/evidence/release-gates/test-r12-authority-maintenance.ps1?ref=$SHA" --jq '.content' | base64 -d | sed -n '480,600p'Length of output: 15555
Claims verified against codeField-kind confusion (resolved). In R12 status classes anchored to literal audited sets (resolved). Lines 315-320 of the same file now hard-code the expected Malformed prefixes rejected before substring logic (resolved, correctly scoped). Cleanup failures preserve primary errors and force FAIL (resolved). Both Other notes
Merge verdictAll four R6 claims are backed by real code changes with corresponding hostile self-tests, not just narrative in the evidence JSON. No new trust-boundary, privilege-escalation, or type-confusion defect found in this delta. The two previously-flagged non-code items remain outstanding and unaffected: (1) enabling the ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
This pull request implements the R12/B17 release-governance bootstrap and CI truth repair, introducing new PowerShell guard scripts, authority maintenance tests, and security reviews, while updating existing path authority and ownership scripts to enforce case-sensitive lowercase SHA checks. The review feedback highlights several critical PowerShell-specific issues where using the array subexpression operator @(...) on null or missing properties wraps $null in a single-element array. This behavior can bypass safety checks or cause fatal runtime crashes under strict mode when iterating and accessing properties on a null loop variable. It is recommended to use [array] casting or direct iteration to prevent these crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if ((Get-PropertyValue $pending 'release_accepted') -isnot [bool] -or [bool](Get-PropertyValue $pending 'release_accepted')) { $errors.Add("pending '$slice' must not claim release acceptance") } | ||
| [string[]]$exactPaths = @(Get-OptionalStringArray $pending 'exact_paths') | ||
| [string[]]$exactPrefixes = @(Get-OptionalStringArray $pending 'exact_prefixes') | ||
| $macroBound = [string](Get-PropertyValue $pending 'authority_mode') -ceq 'bound-plan-member-union-with-exact-r12-overrides' -and @((Get-PropertyValue $pending 'member_slices')).Count -gt 0 -and @((Get-PropertyValue $pending 'exact_r12_overrides')).Count -gt 0 |
There was a problem hiding this comment.
In PowerShell, the array subexpression operator @(...) wraps $null in an array of size 1 containing $null (i.e., @($null).Count is 1). If member_slices or exact_r12_overrides is null or missing, Get-PropertyValue returns $null, and @((Get-PropertyValue ...)).Count -gt 0 will incorrectly evaluate to True. This causes $macroBound to be True even when both properties are missing, bypassing the safety check on line 392.
Using [array] casting instead of @(...) safely returns a count of 0 for null values.
$macroBound = [string](Get-PropertyValue $pending 'authority_mode') -ceq 'bound-plan-member-union-with-exact-r12-overrides' -and ([array](Get-PropertyValue $pending 'member_slices')).Count -gt 0 -and ([array](Get-PropertyValue $pending 'exact_r12_overrides')).Count -gt 0
| $actualStatus = Get-OrdinalSignature $DiffEntries | ||
| $actualPaths = Get-OrdinalSignature $DiffEntries -PathsOnly | ||
| [string[]]$current = @($Contract.status_classes.current) | ||
| foreach ($candidate in @($Contract.candidates)) { |
There was a problem hiding this comment.
Using @($Contract.candidates) wraps a null value in an array of size 1 containing $null if candidates is missing or null. This causes the foreach loop to execute once with a null loop variable. Under Set-StrictMode -Version Latest (active on line 17), accessing properties on $null (e.g., $candidate.status_class) will throw a fatal runtime error and crash the script.
In PowerShell, foreach loops automatically handle null values safely (by skipping execution) and also handle single objects or arrays correctly without needing the @(...) wrapper. Removing @(...) prevents potential strict mode crashes.
foreach ($candidate in $Contract.candidates) {
| if ($candidateStatus -ceq $actualStatus) { return [pscustomobject][ordered]@{ kind='current-candidate'; slice=[string]$candidate.slice; status_class=[string]$candidate.status_class } } | ||
| } | ||
| [string[]]$pendingStatuses = @($Contract.status_classes.pending) | ||
| foreach ($pending in @($Contract.pending_namespaces)) { |
There was a problem hiding this comment.
Using @($Contract.pending_namespaces) wraps a null value in an array of size 1 containing $null if pending_namespaces is missing or null. This causes the foreach loop to execute once with a null loop variable. Under Set-StrictMode -Version Latest (active on line 17), accessing properties on $null (e.g., $pending.status_class) will throw a fatal runtime error and crash the script.
In PowerShell, foreach loops automatically handle null values safely (by skipping execution) and also handle single objects or arrays correctly without needing the @(...) wrapper. Removing @(...) prevents potential strict mode crashes.
foreach ($pending in $Contract.pending_namespaces) {
| if (($currentStatuses -join "`n") -cne ($expectedCurrentStatuses -join "`n")) { $errors.Add('R12 current status classes drifted from the audited closed-world set') } | ||
| if (($rejectedStatuses -join "`n") -cne ($expectedRejectedStatuses -join "`n")) { $errors.Add('R12 rejected status classes drifted from the audited closed-world set') } | ||
| if (($pendingStatuses -join "`n") -cne ($expectedPendingStatuses -join "`n")) { $errors.Add('R12 pending status classes drifted from the audited closed-world set') } | ||
| [object[]]$candidates = @((Get-PropertyValue $ContractObject 'candidates')) |
There was a problem hiding this comment.
If candidates is null, @((Get-PropertyValue ...)) wraps $null in an array of size 1, causing the subsequent foreach loop to execute once with a null candidate, which can lead to errors. Using [array] casting safely initializes an empty array when the property is null.
[object[]]$candidates = [array](Get-PropertyValue $ContractObject 'candidates')
| if ([string]::IsNullOrWhiteSpace([string](Get-PropertyValue $candidate 'plan_owner'))) { $errors.Add("candidate '$slice' has no plan owner") } | ||
| if ((Get-PropertyValue $candidate 'path_authority_eligible') -isnot [bool] -or -not [bool](Get-PropertyValue $candidate 'path_authority_eligible')) { $errors.Add("candidate '$slice' is not path-authority eligible") } | ||
| if ((Get-PropertyValue $candidate 'release_accepted') -isnot [bool] -or [bool](Get-PropertyValue $candidate 'release_accepted')) { $errors.Add("candidate '$slice' must not claim release acceptance") } | ||
| [object[]]$pathObjects = @((Get-PropertyValue $candidate 'paths')) |
There was a problem hiding this comment.
If paths is null, @((Get-PropertyValue ...)) wraps $null in an array of size 1, causing the subsequent foreach loop to execute once with a null path object. Using [array] casting safely initializes an empty array when the property is null.
[object[]]$pathObjects = [array](Get-PropertyValue $candidate 'paths')
| } | ||
| } | ||
| } | ||
| [object[]]$pendingContracts = @((Get-PropertyValue $ContractObject 'pending_namespaces')) |
There was a problem hiding this comment.
If pending_namespaces is null, @((Get-PropertyValue ...)) wraps $null in an array of size 1, causing the subsequent foreach loop to execute once with a null pending contract. Using [array] casting safely initializes an empty array when the property is null.
[object[]]$pendingContracts = [array](Get-PropertyValue $ContractObject 'pending_namespaces')
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a942f80e8
ℹ️ 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".
| } | ||
|
|
||
| $root = [System.IO.Path]::GetFullPath($Repository) | ||
| $artifactPath = [System.IO.Path]::GetFullPath((Join-Path $root $Artifact)) |
There was a problem hiding this comment.
When the workflow invokes this script with -Artifact $env:RUNNER_TEMP/r12-authority-maintenance.json, this Join-Path $root $Artifact rewrites that absolute runner-temp path under the checkout instead of leaving it in ${{ runner.temp }}; the PowerShell Join-Path docs show a child beginning with a separator is still appended to the parent (path\ + \childpath -> path\childpath). The simulation can therefore pass while writing the JSON to <repo>/<runner-temp>/..., and the following upload-artifact step that looks in ${{ runner.temp }} fails with if-no-files-found: error on pull_request runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/production-gates/assert-active-candidate-path-authority.ps1 (3)
692-692: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueПозиционные параметры при вызове
Invoke-R12ContractAuditв self-test.Static analysis отмечает использование позиционных параметров вместо именованных при нескольких вызовах
Invoke-R12ContractAuditв self-test-коде.Also applies to: 706-706, 712-712, 716-716, 721-721, 726-726
🤖 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 `@scripts/production-gates/assert-active-candidate-path-authority.ps1` at line 692, Update the self-test calls to Invoke-R12ContractAudit, including the assignments to r12WrongIdentityResult and the corresponding calls at the other referenced locations, to pass arguments using the function’s named parameter syntax instead of positional arguments. Preserve the existing argument values and test scenarios.Source: Linters/SAST tools
434-435: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winСмешение "not an ancestor" и настоящих git-ошибок в Probe.
git merge-base --is-ancestorвозвращает статус 1, если коммит не предок, и любой другой ненулевой статус сигнализирует об ошибке (например, невалидный SHA). Здесь любой ненулевой$LASTEXITCODEтрактуется одинаково как "pending probe base is not an ancestor of head", что маскирует настоящие git-ошибки неверным диагностическим сообщением.Чуть выше в этом же файле (строки 594-598, R9-ветка) это уже сделано правильно:
$ancestorExit -eq 1обрабатывается отдельно от прочих ненулевых кодов.♻️ Предлагаемое исправление по аналогии с R9-веткой
try { - & git -C $Repository merge-base --is-ancestor $probeBase $probeHead 2>$null - if ($LASTEXITCODE -ne 0) { throw 'pending probe base is not an ancestor of head' } + & git -C $Repository merge-base --is-ancestor $probeBase $probeHead 2>$null + $ancestorExit = $LASTEXITCODE + if ($ancestorExit -eq 1) { throw "pending probe base '$probeBase' is not an ancestor of head '$probeHead'" } + if ($ancestorExit -ne 0) { throw "git merge-base --is-ancestor failed with exit $ancestorExit" } [object[]]$entries = @(Get-GitDiffEntries $Repository $probeBase $probeHead)🤖 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 `@scripts/production-gates/assert-active-candidate-path-authority.ps1` around lines 434 - 435, Update the merge-base status handling near the pending probe validation so exit code 1 raises the existing “not an ancestor” diagnostic, while any other nonzero $LASTEXITCODE is propagated or reported as a genuine git error. Follow the established handling in the R9 branch around $ancestorExit, preserving success behavior for exit code 0.
688-728: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSelf-test не покрывает git-верификацию и Probe-путь R12.
Добавленные R12 self-test сценарии (строки 688-728) проверяют только identity/status/forbidden-path логику через прямые вызовы
Invoke-R12ContractAuditбез-VerifyGit/-RequireObjects/-Probe. Ветки, сверяющие живойgit diffс замороженным контрактом (строки 354-377), и Probe-логика сmerge-base --is-ancestor(строки 424-448), остаются непокрытыми тестами для revision 12 — в отличие от R9-контракта, для которого есть хотя бы$wrongBaseProbe(строка 750).🤖 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 `@scripts/production-gates/assert-active-candidate-path-authority.ps1` around lines 688 - 728, Расширьте R12 self-test вокруг Invoke-R12ContractAudit, добавив сценарии с -VerifyGit и -RequireObjects для проверки сверки живого git diff с замороженным контрактом, а также сценарии с -Probe для проверки merge-base --is-ancestor. Используйте валидный R12 контракт и намеренно изменённые base/объекты или probe-данные, проверяя ожидаемые FAIL-вердикты и соответствующие ошибки; добавьте аналог проверки успешного пути, если он предусмотрен существующим R9-паттерном вроде $wrongBaseProbe.
🤖 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
@.agent/specs/release-gates-r12/evidence/plan-governance/verification-summary.json:
- Line 19: Update the strict_authority_schema claim in verification-summary.json
so it does not present the active_contracts digest as a trusted-validator
SHA-256. Bind it to the validator artifact’s actual bytes and corresponding
digest, or rename the field to explicitly identify active_contracts if that is
the intended source; preserve the remaining schema-validation claims.
---
Nitpick comments:
In `@scripts/production-gates/assert-active-candidate-path-authority.ps1`:
- Line 692: Update the self-test calls to Invoke-R12ContractAudit, including the
assignments to r12WrongIdentityResult and the corresponding calls at the other
referenced locations, to pass arguments using the function’s named parameter
syntax instead of positional arguments. Preserve the existing argument values
and test scenarios.
- Around line 434-435: Update the merge-base status handling near the pending
probe validation so exit code 1 raises the existing “not an ancestor”
diagnostic, while any other nonzero $LASTEXITCODE is propagated or reported as a
genuine git error. Follow the established handling in the R9 branch around
$ancestorExit, preserving success behavior for exit code 0.
- Around line 688-728: Расширьте R12 self-test вокруг Invoke-R12ContractAudit,
добавив сценарии с -VerifyGit и -RequireObjects для проверки сверки живого git
diff с замороженным контрактом, а также сценарии с -Probe для проверки
merge-base --is-ancestor. Используйте валидный R12 контракт и намеренно
изменённые base/объекты или probe-данные, проверяя ожидаемые FAIL-вердикты и
соответствующие ошибки; добавьте аналог проверки успешного пути, если он
предусмотрен существующим R9-паттерном вроде $wrongBaseProbe.
🪄 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: CHILL
Plan: Pro
Run ID: c1927b45-d595-4751-a005-332a699e41cb
📒 Files selected for processing (12)
.agent/plans/2026-07-10-engram-production-ready-active-diff-contracts.json.agent/specs/release-gates-r12/evidence/plan-governance/gates-summary.json.agent/specs/release-gates-r12/evidence/plan-governance/test-r12-plan-governance.ps1.agent/specs/release-gates-r12/evidence/plan-governance/verification-summary.json.agent/specs/release-gates-r12/evidence/release-gates/R12-AUTHORITY-MAINTENANCE.tdd.json.agent/specs/release-gates-r12/evidence/release-gates/maintenance-simulation.json.agent/specs/release-gates-r12/evidence/release-gates/security-review.md.agent/specs/release-gates-r12/evidence/release-gates/test-r12-authority-maintenance.ps1.agent/specs/release-gates-r12/evidence/release-gates/windows-harness.jsonscripts/production-gates/assert-active-candidate-path-authority.ps1scripts/production-gates/assert-pr-authority-guard.ps1scripts/production-gates/assert-pr-authority-maintenance.ps1
🚧 Files skipped from review as they are similar to previous changes (7)
- .agent/specs/release-gates-r12/evidence/plan-governance/test-r12-plan-governance.ps1
- .agent/specs/release-gates-r12/evidence/release-gates/R12-AUTHORITY-MAINTENANCE.tdd.json
- .agent/specs/release-gates-r12/evidence/release-gates/maintenance-simulation.json
- .agent/specs/release-gates-r12/evidence/release-gates/windows-harness.json
- scripts/production-gates/assert-pr-authority-guard.ps1
- .agent/specs/release-gates-r12/evidence/release-gates/test-r12-authority-maintenance.ps1
- scripts/production-gates/assert-pr-authority-maintenance.ps1
| "two_pr_stale_loser": true, | ||
| "required_status_identity": "authority-guard plus strict JSON integer GitHub Actions integration_id 15368", | ||
| "strict_authority_schema": "the complete canonical authority manifest is bound to trusted-validator SHA-256 6665a80d30c2b45319b8c8adce05430c6f6e3913ded92313c4ebbb381f11c9f6; recursive hostile tests prove exact object keys, scalar types/values, array cardinality/order, and duplicate/case-alias rejection without requiring an unreachable predecessor Git object", | ||
| "strict_authority_schema": "the complete canonical authority manifest is bound to trusted-validator SHA-256 9776e67bb8b2bc93de8fca2ec9296f765e64ef3d2380f40b306ca2d6b193a67a; recursive hostile tests prove exact object keys, scalar types/values, array cardinality/order, and duplicate/case-alias rejection without requiring an unreachable predecessor Git object", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
for path in \
scripts/production-gates/assert-pr-authority-guard.ps1 \
scripts/production-gates/assert-pr-authority-maintenance.ps1 \
.agent/plans/2026-07-10-engram-production-ready-active-diff-contracts.json
do
printf '%s: ' "$path"
git show "HEAD:$path" | tr -d '\r' | sha256sum
doneRepository: thebtf/engram
Length of output: 478
🏁 Script executed:
set -euo pipefail
printf '\n[files]\n'
git ls-files \
.agent/specs/release-gates-r12/evidence/plan-governance/verification-summary.json \
.agent/specs/release-gates-r12/evidence/plan-governance/gates-summary.json \
.github/workflows/test.yml \
.github/workflows/authority-guard.yml \
scripts/production-gates/assert-pr-authority-guard.ps1 \
scripts/production-gates/assert-pr-authority-maintenance.ps1 \
.agent/plans/2026-07-10-engram-production-ready-active-diff-contracts.json
printf '\n[relevant excerpts]\n'
for f in \
.agent/specs/release-gates-r12/evidence/plan-governance/verification-summary.json \
.agent/specs/release-gates-r12/evidence/plan-governance/gates-summary.json \
.github/workflows/test.yml \
.github/workflows/authority-guard.yml
do
echo "--- $f"
nl -ba "$f" | sed -n '1,220p'
done
printf '\n[sha256 via python]\n'
python3 - <<'PY'
from pathlib import Path
import hashlib
paths = [
Path('scripts/production-gates/assert-pr-authority-guard.ps1'),
Path('scripts/production-gates/assert-pr-authority-maintenance.ps1'),
Path('.agent/plans/2026-07-10-engram-production-ready-active-diff-contracts.json'),
Path('.agent/specs/release-gates-r12/evidence/plan-governance/gates-summary.json'),
]
for p in paths:
data = p.read_bytes().replace(b'\r\n', b'\n')
print(f"{p}: {hashlib.sha256(data).hexdigest()}")
PYRepository: thebtf/engram
Length of output: 721
🏁 Script executed:
set -euo pipefail
printf '\n[verification-summary]\n'
sed -n '1,120p' .agent/specs/release-gates-r12/evidence/plan-governance/verification-summary.json | cat -n
printf '\n[gates-summary]\n'
sed -n '1,140p' .agent/specs/release-gates-r12/evidence/plan-governance/gates-summary.json | cat -n
printf '\n[test.yml]\n'
sed -n '70,190p' .github/workflows/test.yml | cat -n
printf '\n[authority-guard.yml]\n'
sed -n '1,170p' .github/workflows/authority-guard.yml | cat -n
printf '\n[sha256]\n'
python3 - <<'PY'
from pathlib import Path
import hashlib
for p in [
Path('scripts/production-gates/assert-pr-authority-guard.ps1'),
Path('scripts/production-gates/assert-pr-authority-maintenance.ps1'),
Path('.agent/plans/2026-07-10-engram-production-ready-active-diff-contracts.json'),
]:
data = p.read_bytes().replace(b'\r\n', b'\n')
print(p)
print(hashlib.sha256(data).hexdigest())
PYRepository: thebtf/engram
Length of output: 25331
Не смешивайте digest active_contracts с digest доверенного валидатора.
9776e67bb8b2bc93de8fca2ec9296f765e64ef3d2380f40b306ca2d6b193a67a совпадает с хэшем active-contracts JSON, а не с отдельным байтовым артефактом валидатора. Перепривяжите strict_authority_schema к правильным байтам или переименуйте поле, чтобы оно явно указывало на active_contracts.
🤖 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
@.agent/specs/release-gates-r12/evidence/plan-governance/verification-summary.json
at line 19, Update the strict_authority_schema claim in
verification-summary.json so it does not present the active_contracts digest as
a trusted-validator SHA-256. Bind it to the validator artifact’s actual bytes
and corresponding digest, or rename the field to explicitly identify
active_contracts if that is the intended source; preserve the remaining
schema-validation claims.
|
R7 checker repair is ready at exact head f50d86d. The delta closes the PowerShell @($null) false-green in macro authority, validates JSON-origin arrays and canonical members/paths, distinguishes merge-base exit 1 from Git errors, and corrects the mislabeled evidence digest. Root replay on the exact tree: selftest PASS; R12 plan governance PASS; Git-backed active audit PASS (9 candidates, 118 paths, 0 errors); isolated authority simulation PASS 24/24 (4 allows, 20 expected rejects, cleanup verified). Please independently inspect behavior, not the reported tests, with focus on wrong JSON types, missing/duplicate macro members, schema aliases, canonical paths, and ancestry error handling. @coderabbitai review @codex review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request updates the release-governance and authority-maintenance scripts by introducing a new 'replacement' transition kind, adding helper functions for JSON property validation, and implementing strict case-sensitive SHA checks. It also adds new scripts and test harnesses for R12 authority maintenance and PR authority guarding. The code review feedback identifies a potential bug where an undefined variable $predecessorCount is referenced, suggests using case-sensitive -cnotmatch operators for Git/SHA validation in the new replacement transition block for consistency, and recommends validating the $Remote parameter in both the PR authority guard and maintenance scripts to prevent potential Git option injection attacks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| elseif ($transitionKind -eq 'replacement' -and $currentIndex -ge 0) { | ||
| if ($currentIndex -eq 0) { $errors.Add("state epoch '$path' replacement transition has no predecessor") } | ||
| if ($requiredBase -notmatch '^[0-9a-fA-F]{40}$') { $errors.Add("state epoch '$path' replacement base must be a full SHA") } | ||
| if ($predecessorCount -ne $currentIndex) { $errors.Add("state epoch '$path' replacement predecessor evidence count is $predecessorCount, expected $currentIndex") } |
There was a problem hiding this comment.
Please verify if $predecessorCount is defined in the scope of the Get-EpochEvidenceErrors function. If it is not defined, accessing it here will return $null (or throw a runtime exception if Set-StrictMode is active), which would cause the validation to incorrectly fail or crash.\n\nIf it is not defined, you should use $predecessors.Count instead.
if ($predecessors.Count -ne $currentIndex) { $errors.Add("state epoch '$path' replacement predecessor evidence count is $($predecessors.Count), expected $currentIndex") }
| if ($requiredBase -notmatch '^[0-9a-fA-F]{40}$') { $errors.Add("state epoch '$path' replacement base must be a full SHA") } | ||
| if ($predecessorCount -ne $currentIndex) { $errors.Add("state epoch '$path' replacement predecessor evidence count is $predecessorCount, expected $currentIndex") } | ||
| $currentStatus = [string](Get-PropertyValue $Epoch 'current_owner_status') | ||
| $currentHead = [string](Get-PropertyValue $Epoch 'current_owner_head_sha') | ||
| if ([string]::IsNullOrWhiteSpace($currentStatus) -and $currentHead -notmatch '^[0-9a-fA-F]{40}$') { | ||
| $errors.Add("state epoch '$path' replacement lacks current-owner status or immutable head identity") | ||
| } | ||
| for ($ownerIndex = 0; $ownerIndex -lt [math]::Max(0, $currentIndex); $ownerIndex++) { | ||
| $expectedOwner = $owners[$ownerIndex] | ||
| $matches = @($predecessors | Where-Object { [string](Get-PropertyValue $_ 'owner') -ceq $expectedOwner }) | ||
| if ($matches.Count -ne 1) { $errors.Add("state epoch '$path' replacement predecessor '$expectedOwner' evidence count is $($matches.Count), expected 1"); continue } | ||
| $entry = $matches[0] | ||
| $checkerVerdict = [string](Get-PropertyValue $entry 'checker_verdict') | ||
| if ($checkerVerdict -ceq 'REVISE_HOLD') { | ||
| $checkerArtifact = [string](Get-PropertyValue $entry 'checker_artifact') | ||
| $checkerSha = [string](Get-PropertyValue $entry 'checker_sha256') | ||
| $rejectedHead = [string](Get-PropertyValue $entry 'rejected_head_sha') | ||
| $postReviewVerdict = [string](Get-PropertyValue $entry 'post_review_verdict') | ||
| $postReviewArtifact = [string](Get-PropertyValue $entry 'post_review_artifact') | ||
| $postReviewSha = [string](Get-PropertyValue $entry 'post_review_sha256') | ||
| $integrationSha = [string](Get-PropertyValue $entry 'integration_sha') | ||
| if ($checkerArtifact -notmatch '^\.agent/' -or $checkerSha -notmatch '^[0-9a-fA-F]{64}$') { $errors.Add("state epoch '$path' replacement predecessor '$expectedOwner' lacks exact rejecting-checker evidence") } | ||
| if ($postReviewVerdict -cne 'REVISE_HOLD' -or $postReviewArtifact -notmatch '^\.agent/' -or (-not [string]::IsNullOrWhiteSpace($postReviewSha) -and $postReviewSha -notmatch '^[0-9a-fA-F]{64}$')) { $errors.Add("state epoch '$path' replacement predecessor '$expectedOwner' lacks root post-review evidence") } | ||
| if ($rejectedHead -notmatch '^[0-9a-fA-F]{40}$') { $errors.Add("state epoch '$path' replacement predecessor '$expectedOwner' lacks rejected-head identity") } | ||
| if (-not [string]::IsNullOrWhiteSpace($integrationSha)) { $errors.Add("state epoch '$path' rejected replacement predecessor '$expectedOwner' must not claim integration") } | ||
| } | ||
| elseif ($checkerVerdict -ceq 'ROOT_SELECTED_EXACT_BASE') { | ||
| $integrationSha = [string](Get-PropertyValue $entry 'integration_sha') | ||
| $selectedHead = [string](Get-PropertyValue $entry 'predecessor_head_sha') | ||
| if ($integrationSha -notmatch '^[0-9a-fA-F]{40}$' -and $selectedHead -notmatch '^[0-9a-fA-F]{40}$') { $errors.Add("state epoch '$path' root-selected replacement predecessor '$expectedOwner' lacks an immutable integration/head identity") } | ||
| if ($integrationSha -notmatch '^[0-9a-fA-F]{40}$' -and [string](Get-PropertyValue $entry 'checker_artifact') -notmatch '^\.agent/') { $errors.Add("state epoch '$path' root-selected replacement predecessor '$expectedOwner' lacks the root selection artifact") } |
There was a problem hiding this comment.
In the new replacement transition validation block, several Git/SHA identity fields (such as $requiredBase, $currentHead, $checkerSha, $rejectedHead, $postReviewSha, $integrationSha, and $selectedHead) are validated using case-insensitive -notmatch with the pattern ^[0-9a-fA-F]{40}$ (or 64).\n\nTo maintain consistency with the rest of the codebase (which enforces a strict canonical lowercase contract for Git/SHA identities, as seen in the recent migration of Resolve-ExactCommit to -cnotmatch '^[0-9a-f]{40}$'), these checks should be updated to use case-sensitive -cnotmatch with lowercase [0-9a-f] patterns.
| $exitCode = 1 | ||
| $trustedRoot = $null | ||
| try { | ||
| if ($BaseSha -cnotmatch '^[0-9a-f]{40}$' -or $HeadSha -cnotmatch '^[0-9a-f]{40}$' -or $ExpectedValidatorGitBlob -cnotmatch '^[0-9a-f]{40}$') { throw 'base, head, and validator Git identities must be canonical lowercase full SHAs' } |
There was a problem hiding this comment.
The $Remote parameter is passed directly to git fetch without validation. If an attacker can control or manipulate the remote URL/name passed to this script, they could potentially perform a Git option injection attack by passing a value starting with a dash (e.g., --upload-pack=...).\n\nTo prevent this, consider validating that $Remote does not start with a dash.
if ($BaseSha -cnotmatch '^[0-9a-f]{40}$' -or $HeadSha -cnotmatch '^[0-9a-f]{40}$' -or $ExpectedValidatorGitBlob -cnotmatch '^[0-9a-f]{40}$') { throw 'base, head, and validator Git identities must be canonical lowercase full SHAs' }\n if ($Remote -like '-*') { throw 'remote must not start with a dash to prevent option injection' }
| $exitCode = 1 | ||
| $trustedRoot = $null | ||
| try { | ||
| if ($BaseSha -cnotmatch '^[0-9a-f]{40}$' -or $ExpectedValidatorGitBlob -cnotmatch '^[0-9a-f]{40}$' -or ($PSCmdlet.ParameterSetName -eq 'Transition' -and $HeadSha -cnotmatch '^[0-9a-f]{40}$')) { throw 'base, head, and validator Git identities must be canonical lowercase full SHAs' } |
There was a problem hiding this comment.
The $Remote parameter is passed directly to git fetch without validation. If an attacker can control or manipulate the remote URL/name passed to this script, they could potentially perform a Git option injection attack by passing a value starting with a dash (e.g., --upload-pack=...).\n\nTo prevent this, consider validating that $Remote does not start with a dash.
if ($BaseSha -cnotmatch '^[0-9a-f]{40}$' -or $ExpectedValidatorGitBlob -cnotmatch '^[0-9a-f]{40}$' -or ($PSCmdlet.ParameterSetName -eq 'Transition' -and $HeadSha -cnotmatch '^[0-9a-f]{40}$')) { throw 'base, head, and validator Git identities must be canonical lowercase full SHAs' }\n if ($Remote -like '-*') { throw 'remote must not start with a dash to prevent option injection' }
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f50d86de6a
ℹ️ 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".
| [string[]]$exactPaths = @(Get-OptionalStringArray $pending 'exact_paths') | ||
| [string[]]$exactPrefixes = @(Get-OptionalStringArray $pending 'exact_prefixes') | ||
| $macroBound = Test-R12MacroAuthority $pending $errors |
There was a problem hiding this comment.
When an R12 pending namespace uses the ordinary exact_paths/exact_prefixes fields, these lines still call Get-OptionalStringArray, which wraps any scalar property into a one-element list. A malformed contract such as "exact_paths": "internal/proxy/identity_test.go" is therefore treated as a valid bounded path and can PASS, so the checker only enforces JSON-origin arrays for the macro helper while leaving the other R12 pending authorities type-confusable.
Useful? React with 👍 / 👎.
| $memberSlicesValue = Get-JsonPropertyValue $Pending 'member_slices' | ||
| $valid = Test-RequiredCanonicalStringArray -Value $memberSlicesValue -Name 'member_slices' -Errors $Errors -MemberPattern '^[A-Z][A-Z0-9]*(?:-[A-Z0-9]+)*$' | ||
| $memberSlices = if ($valid) { @($memberSlicesValue) } else { @() } |
There was a problem hiding this comment.
This validates member_slices as a non-empty unique string array, but never checks that it equals the audited macro batch/scope-map membership. A contract can drop any MB1 member that has no exact override, or replace the list with just the override slices, and Test-R12MacroAuthority still returns true because the list is only used to vet override slice names; that makes macro_bound_plan_union a false positive for an incomplete macro authority set.
Useful? React with 👍 / 👎.
1 participant
Review brief
Description: establish the R12 trusted-base ordinary PR guard and owner-only self-reference-free control-plane maintenance path; repair CI database ownership and recurring epoch validation.
Requirements:
pull_request_targetexecution must use only exact trusted-base validator bytes and treat PR-head bytes as data;Base:
8279d221845ddc0c84007035266e00a79c4994e2Head:
1dc6b91cScope: this PR only (
review/release-integration-r1...work/prc-governance-a17-b17-sol).Risk focus: trusted/untrusted execution boundary, Git object/ref races, stale-label replay, self-reference/fixed-point traps, type confusion, duplicate path/status semantics, workflow permissions/secrets/actions, recurring-epoch liveness, CI false-green/false-red behavior.
Known limits: Tavily research route remained OAuth-blocked; current claims use official sources plus Context7 and Parallel. Application runtime/UI behavior is unchanged. The full project critical suite and exact dev-stand were already green on the base synthesis; this PR adds focused governance evidence and awaits independent PR review/CI.
Expected output: independently derive findings without relying on author conclusions; label severity; cite file and line; distinguish merge blockers from optional notes; explicitly test behavioral edges, not only structure or green tests.
Summary by CodeRabbit
Новые возможности
Исправления
Тестирование
Документация