fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186)

[rapiercraft-forgedock]

Closes #194

ForgeDock verified handoff

• Run: run_2563b1c9-62f1-483d-9d16-908e6d6f6fdc
• Build Packet: art_0b7715f3-02db-4c2c-b319-971be93dda3c (durable artifact on the linked issue)
• Build Result: art_bbd701c8-d51e-4b86-a846-d843991ae008 (durable artifact on the linked issue)
• Verified head: e6686e3ffb089b677c9bdeeda7c8611b5f2991fd

Summary

Implemented strict deployment inventory reconciliation with one frozen diff snapshot, canonical path propagation, stale-plan protection, fail-closed cleanup, and focused regressions. Build, full test suite, and diff-check pass.

Changed paths

• src/workflows/review-pr/planner.test.ts
• src/workflows/review-pr/planner.ts
• src/workflows/review-pr/review-deployment.test.ts
• src/workflows/review-pr/review-existing.ts
• src/workflows/review-pr/review.test.ts
• src/workflows/review-pr/review.ts

Acceptance evidence

• passed — Deployment review rejects an empty or malformed diff inventory and any non-string, empty, duplicate, absolute, drive-qualified, traversal, glob, colon-containing, or control-character path; after slash and leading-./ normalization, the diff-header destination set equals both deployment.changedPaths and packet.payload.expectedPaths exactly, with no union or silent repair.: Added strict parseDeploymentDiffPaths/canonicalizeDeploymentPaths validation for both diff-header sides and canonical destination uniqueness; review.ts reconciles diff, evidence, and packet arrays exactly. Planner and deployment tests cover normalization and every rejection class. Controller-observed subject evidence: GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 body: ## Problem Deployment review trusts an unverified changed-path inventory Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-7-of-10:correctness-deployment-inventory-not-reconciled Reviewer sessions: 52bd70d0 Confidence: HIGH Severity: HIGH Controller disposition: blocking Scope disposition: in_scope Scope rationale: Reconciliating the froz…
• passed — The normal reviewDeploymentPullRequest path calls ForgeHost.getPullRequestDiff once for the pre-review frozen PR, passes that exact snapshot into reviewPullRequest, and uses the same snapshot for inventory validation, planReviewPanel, and scopedReviewDiff; a successive or alternate diff read cannot bind artifacts, topology, or reviewer content to another inventory.: Added the deployment diff snapshot handoff on DeploymentReviewEvidence; review-existing reads once, builds valid artifacts from that snapshot, and review.ts never rereads deployment diffs. DeploymentHost counting tests provide a successive alternate response and assert one read. Controller-observed subject evidence: GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 body: ## Problem Deployment review trusts an unverified changed-path inventory Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-7-of-10:correctness-deployment-inventory-not-reconciled Reviewer sessions: 52bd70d0 Confidence: HIGH Severity: HIGH Controller disposition: blocking Scope disposition: in_scope Scope rationale: Reconciliating the frozen…
• passed — The validated canonical inventory is the sole deployment path authority for createDeploymentReviewArtifacts, review-plan execution-group coverage, reviewer scopeManifestFor inputs, and consolidateReviewerFindings reviewedPaths/expectedPaths; a reusable prior plan is used only when its execution-group path union exactly covers the canonical inventory, otherwise it is replanned from the same snapshot before any reviewer starts.: Canonical paths feed deployment artifact creation, plan construction, deployment reviewer scope manifests, and consolidation; deployment-only reusable-plan coverage rejects stale topology. Core tests cover slash aliases, packet/evidence disagreement, and stale prior scopes. Controller-observed subject evidence: GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 body: ## Problem Deployment review trusts an unverified changed-path inventory Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-7-of-10:correctness-deployment-inventory-not-reconciled Reviewer sessions: 52bd70d0 Confidence: HIGH Severity: HIGH Controller disposition: blocking Scope disposition: in_scope Scope rationale: Reconciliating the frozen depl…
• passed — Every deployment inventory failure occurs before runtime capabilities or reviewer execution and produces no reviewer submission or wave comment, finding issue materialization/reconciliation, or ReviewVerdict append/publication; the run records a failure and any workspace allocated by the caller is cleaned up. Regression coverage covers omission, addition, empty, malformed, unsafe, duplicate, and packet/evidence disagreement cases.: Strict caller failures transition the created run to failed before runtime and remove the workspace; core reconciliation failures retain the existing fail-closed transition. Deployment and core tests assert zero tasks/publications/verdicts, failed runs, and cleanup across invalid inventories and mismatch cases. Controller-observed subject evidence: GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 body: ## Problem Deployment review trusts an unverified changed-path inventory Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-7-of-10:correctness-deployment-inventory-not-reconciled Reviewer sessions: 52bd70d0 Confidence: HIGH Severity: HIGH Controller disposition: blocking Scope disposition: in_scope **Scope ratio…
• passed — A clean exact-match deployment fixture can approve only after every canonical diff path is represented by the frozen execution-group shards and passed as the consolidated reviewed-path policy; existing bounded deployment diff handling and plan/sharding invariants remain intact.: Exact-match single- and multi-path deployment fixtures assert approval, complete execution-group scopes, canonical reviewer objectives, packet path count, and one diff read; existing bounded shard and scoped-diff tests remain passing. Controller-observed subject evidence: GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 body: ## Problem Deployment review trusts an unverified changed-path inventory Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-7-of-10:correctness-deployment-inventory-not-reconciled Reviewer sessions: 52bd70d0 Confidence: HIGH Severity: HIGH Controller disposition: blocking Scope disposition: in_scope Scope rationale: Reconciliating the frozen deployment inventory is directly required to…
• passed — The existing exact-head route checks remain authoritative around reviewer execution and verdict publication: head, branch, base, or state drift, cancellation, timeout, or an undrained/late reviewer result cannot produce an approving deployment verdict or bypass the validated inventory; the existing bounded late-result evidence behavior is preserved.: Route, CI, cancellation, timeout, drain, and late-result code paths were preserved; the existing deployment drift and review lifecycle regressions pass in the full test suite after inventory validation is placed before runtime setup. Controller-observed subject evidence: GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 labels: workflow:building, review-finding, needs-validation, priority:P1 | GitHub issue fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #194 body: ## Problem Deployment review trusts an unverified changed-path inventory Source: PR Deploy: staging → main #186 — Deploy: staging → main Reviewed SHA: 8aa2c7ae638699e9fdbcd27b37549ccba6993683 Run: run_72407589-e453-408e-afa1-8572c6ae712c Reviewers: correctness Source findings: correctness:review-correctness-part-7-of-10:correctness-deployment-inventory-not-reconciled Reviewer sessions: 52bd70d0 Confidence: HIGH Severity: HIGH Controller disposition: blocking Scope disposition: in_scope Scope rationale: Reconciliating the frozen deployment inventory is directly required to …

Verification

• passed — git diff --check — warning: in the working copy of 'src/workflows/review-pr/review-existing.ts', LF will be replaced by CRLF the next time Git touches it | warning: in the working copy of 'src/workflows/review-pr/review.test.ts', LF will be replaced by CRLF the next time Git touches it | warning: in the working copy of 'src/workflows/review-pr/review.ts', LF will be replaced by CRLF the next time Git touches it
• passed — C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js run build — > forgedock@1.8.0 build | > tsc -p tsconfig.json
• passed — C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js run docs:build — - generating sitemap... | ✓ generating sitemap... | build complete in 4.98s.
• passed — C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js test — # skipped 8 | # todo 0 | # duration_ms 84532.8843

Residual risks

• The controller-owned staging-review gate still performs its independent validation; no GitHub publication or workflow transition outside the assigned worktree was performed.

[rapiercraft-forgedock]

ForgeDock Review Evidence

One bounded projection for the complete frozen reviewer wave. The controller's consolidated Review Verdict remains authoritative.

• PR: fix: Deployment review trusts an unverified changed-path inventory (review finding — PR #186) #270
• Reviewed SHA: e6686e3ffb089b677c9bdeeda7c8611b5f2991fd
• Run: run_2563b1c9-62f1-483d-9d16-908e6d6f6fdc
• Frozen review plan: review-plan-3a4471a9d3be23b314f7
• Groups: 2 completed · 0 failed

Completed groups

review-correctness · correctness · completed

• Session lineage: 01a00951-22b2-791b-8546-7b248af028fd
• Summary: Found one blocking correctness gap in strict deployment diff path decoding: quoted UTF-8 filenames encoded with Git octal escapes are canonicalized to the wrong path.
• Findings:
  • HIGH · Strict deployment parser misdecodes Git quoted UTF-8 pathnames — src/workflows/review-pr/planner.ts:readGitHeaderToken; src/workflows/review-pr/review-existing.ts:102-123; src/workflows/review-pr/review.ts:299-304
    • Evidence: parseDeploymentDiffPaths sends quoted headers through readGitHeaderToken, whose octal branch appends String.fromCharCode(Number.parseInt(octal, 8)) for each escaped byte. A standard Git C-quoted UTF-8 header such as "a/src/\303\251-dir/file.ts" "b/src/\303\251-dir/file.ts" therefore yields src/é-dir/file.ts, not src/é-dir/file.ts, while normalizeDeploymentPath accepts the corrupted string. The normal deployment caller derives changedPaths from this parser, copies that list into packet.expectedPaths, and passes the same snapshot into reviewPullRequest, so all equality checks can pass with the wrong inventory; the resulting reviewer scope/plan/consolidation authority names the wrong path and a nested non-ASCII directory can receive the wrong read root.
    • Remediation: Decode Git C-quoted octal byte sequences as UTF-8 (and reject invalid byte sequences) before path normalization, for both header sides. Add a normal deployment regression using a quoted-octal Unicode filename, preferably under a Unicode directory, asserting the exact packet expectedPaths, execution-group scopes, reviewer scope manifest inputs, and consolidated reviewedPaths.

review-data · data · completed

• Session lineage: 01a00951-22db-751c-8141-ab2408f09c19
• Summary: Two blocking data-integrity gaps remain in deployment diff parsing: valid quoted headers are not represented by scoped diff sections, and Git UTF-8 octal escapes are decoded into incorrect path strings.
• Findings:
  • HIGH · Strictly accepted quoted diff headers disappear from deployment scoped diffs — src/workflows/review-pr/planner.ts:parseDiffSections and scopedReviewDiff
    • Evidence: parseDeploymentDiffPaths accepts quoted Git headers through readGitHeaderToken, including a valid form such as diff --git "a/src/a b.ts" "b/src/a b.ts". The downstream parseDiffSections regex only matches unquoted ^diff --git a/(.+?) b/(.+)$, so it produces no sections for that accepted header. scopedReviewDiff then calls boundInitialDiff with an empty section list; once the deployment diff exceeds the 30,000-character bound, it retains only the prefix rather than preserving every canonical file section. The plan still lists the paths, but later quoted-path files can be absent from reviewer content and a clean wave can approve without examining their hunks.
    • Remediation: Use the same strict Git-header tokenizer and decoded section representation for deployment planning and scopedReviewDiff, or fail closed when an accepted header cannot be sectionized. Add a large deployment regression with quoted path headers and assert every canonical file section remains available to its reviewer.
  • HIGH · Git C-style UTF-8 path escapes are canonicalized as mojibake — src/workflows/review-pr/planner.ts:readGitHeaderToken and normalizeDeploymentPath
    • Evidence: readGitHeaderToken converts each octal escape with String.fromCharCode(Number.parseInt(octal, 8)). Git C-style quoted paths encode UTF-8 bytes, so a valid header containing \303\251 for é is decoded as é, not é. normalizeDeploymentPath then accepts the wrong string. In the normal deployment caller this corrupted value becomes the artifact affectedSurfaces, packet expectedPaths, plan scope, reviewer manifest input, and consolidation policy; a reviewer finding anchored to the real src/é.ts path can therefore be treated as outside the reviewed inventory. A direct caller with the real path instead fails reconciliation, rejecting a valid deployment.
    • Remediation: Decode Git C-style escapes byte-wise and UTF-8-decode the resulting bytes before path validation; implement every Git control escape (or reject unsupported escapes) so no control character is silently converted to a printable character. Add quoted Unicode and control-escape regressions that assert the actual canonical destination path.

Failed groups

None.

[rapiercraft-forgedock]

ForgeDock · Review Verdict

Run run_2563b1c9-62f1-483d-9d16-908e6d6f6fdc · artifact art_732e5679-2532-496c-8f23-697abfec9a41

Disposition: request_changes · Reviewed SHA: e6686e3ffb089b677c9bdeeda7c8611b5f2991fd

Reviewer roles: correctness, data

Review plan

Risk: medium · Specialist group budget: 3
Identity: review-plan-3a4471a9d3be23b314f7 · Generation: 1 · Frozen: yes
Absolute budget: 2 logical sessions (3 specialist groups), 2 attempts/group

Required capabilities

• acceptance-correctness · score 1000 · mandatory evidence
• data-integrity · score 100

Execution groups

• review-correctness (correctness) · acceptance-correctness — mandatory intent, acceptance, target-authority, and correctness review
• review-data (data) · data-integrity — Build Packet declares persistence/encoding/schema risk; diff changes persisted or interoperable data semantics: src/workflows/review-pr/planner.test.ts, src/workflows/review-pr/planner.ts, src/workflows/review-pr/review-deployment.test.ts, src/workflows/review-pr/review.test.ts

Non-executing specialist roles

• security · score 0 · below-threshold — no qualifying evidence
• api-compatibility · score 0 · below-threshold — no qualifying evidence
• frontend · score 0 · below-threshold — no qualifying evidence
• infrastructure · score 0 · below-threshold — no qualifying evidence
• concurrency · score 0 · below-threshold — no qualifying evidence

Findings

• HIGH · Strictly accepted quoted diff headers disappear from deployment scoped diffs · BLOCKING · reviewers: data
  parseDeploymentDiffPaths accepts quoted Git headers through readGitHeaderToken, including a valid form such as diff --git "a/src/a b.ts" "b/src/a b.ts". The downstream parseDiffSections regex only matches unquoted ^diff --git a/(.+?) b/(.+)$, so it produces no sections for that accepted header. scopedReviewDiff then calls boundInitialDiff with an empty section list; once the deployment diff exceeds the 30,000-character bound, it retains only the prefix rather than preserving every canonical file section. The plan still lists the paths, but later quoted-path files can be absent from reviewer content and a clean wave can approve without examining their hunks.
  Location: src/workflows/review-pr/planner.ts:parseDiffSections and scopedReviewDiff
  Sources: data:review-data-quoted-header-section-parity
  Sessions: 01a00951-22db-751c-8141-ab2408f09c19
  Remediation: Use the same strict Git-header tokenizer and decoded section representation for deployment planning and scopedReviewDiff, or fail closed when an accepted header cannot be sectionized. Add a large deployment regression with quoted path headers and assert every canonical file section remains available to its reviewer.
• HIGH · Git C-style UTF-8 path escapes are canonicalized as mojibake · BLOCKING · reviewers: data
  readGitHeaderToken converts each octal escape with String.fromCharCode(Number.parseInt(octal, 8)). Git C-style quoted paths encode UTF-8 bytes, so a valid header containing \303\251 for é is decoded as é, not é. normalizeDeploymentPath then accepts the wrong string. In the normal deployment caller this corrupted value becomes the artifact affectedSurfaces, packet expectedPaths, plan scope, reviewer manifest input, and consolidation policy; a reviewer finding anchored to the real src/é.ts path can therefore be treated as outside the reviewed inventory. A direct caller with the real path instead fails reconciliation, rejecting a valid deployment.
  Location: src/workflows/review-pr/planner.ts:readGitHeaderToken and normalizeDeploymentPath
  Sources: data:review-data-git-octal-utf8-path-corruption
  Sessions: 01a00951-22db-751c-8141-ab2408f09c19
  Remediation: Decode Git C-style escapes byte-wise and UTF-8-decode the resulting bytes before path validation; implement every Git control escape (or reject unsupported escapes) so no control character is silently converted to a printable character. Add quoted Unicode and control-escape regressions that assert the actual canonical destination path.
• HIGH · Strict deployment parser misdecodes Git quoted UTF-8 pathnames · reviewers: correctness
  parseDeploymentDiffPaths sends quoted headers through readGitHeaderToken, whose octal branch appends String.fromCharCode(Number.parseInt(octal, 8)) for each escaped byte. A standard Git C-quoted UTF-8 header such as "a/src/\303\251-dir/file.ts" "b/src/\303\251-dir/file.ts" therefore yields src/é-dir/file.ts, not src/é-dir/file.ts, while normalizeDeploymentPath accepts the corrupted string. The normal deployment caller derives changedPaths from this parser, copies that list into packet.expectedPaths, and passes the same snapshot into reviewPullRequest, so all equality checks can pass with the wrong inventory; the resulting reviewer scope/plan/consolidation authority names the wrong path and a nested non-ASCII directory can receive the wrong read root.
  Location: src/workflows/review-pr/planner.ts:readGitHeaderToken; src/workflows/review-pr/review-existing.ts:102-123; src/workflows/review-pr/review.ts:299-304
  Sources: correctness:review-correctness-quoted-octal-path-decoding
  Sessions: 01a00951-22b2-791b-8546-7b248af028fd
  Remediation: Decode Git C-quoted octal byte sequences as UTF-8 (and reject invalid byte sequences) before path normalization, for both header sides. Add a normal deployment regression using a quoted-octal Unicode filename, preferably under a Unicode directory, asserting the exact packet expectedPaths, execution-group scopes, reviewer scope manifest inputs, and consolidated reviewedPaths.

Verification

Command	Status	Duration	Summary
git diff --check	passed	48 ms	warning: in the working copy of 'src/workflows/review-pr/review-existing.ts', LF will be replaced by CRLF the next time Git touches it | warning: in the working copy of 'src/workflows/review-pr/review.test.ts', LF will be replaced by CRLF the next time Git touches it | warning: in the working copy of 'src/workflows/review-pr/review.ts', LF will be replaced by CRLF the next time Git touches it
C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js run build	passed	9168 ms	> forgedock@1.8.0 build | > tsc -p tsconfig.json
C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js run docs:build	passed	6410 ms	- generating sitemap... | ✓ generating sitemap... | build complete in 4.98s.
C:\Program Files\nodejs\node.exe C:\Program Files\nodejs\node_modules\npm\bin\npm-cli.js test	passed	116161 ms	# skipped 8 | # todo 0 | # duration_ms 84532.8843