Implement plan validation and ledger-backed review core

[mchwang]

Problem and result

PR #1 established the design and v1 contract, but the repository had no executable foundation. This adds plan import/validation, pure suggestion transformations, read-only Git history loading, ledger-backed attribution, and approval/choice staleness. It now targets main and incorporates the merged design baseline 91fd2b4.

Implementation

• Retained versioned schemas and registry-selected v1 semantics, with strict JSON/YAML parsing, decoded duplicate-key detection, byte/depth limits, and version-specific conformance fixtures.
• Exact full-argv approval and deterministic tokenization. Required selected issue, stable repository/task/plan identity, typed base entries, and caller-supplied actual checkout path identity.
• Projected leaf/path operations, symlink lineage and declared-target overlap checks; gitlink authoring rejected. Runtime filesystem checks remain separate.
• Suggestions require the trusted request-time plan binding; Apply increments revision and stales sibling cards. Persistent IDs, replay protection, cancellation and atomic CAS belong to the store.
• Ledger-only ownership for text and non-text changes, explicit null owners remain foreign, and typed blob/commit metadata in file cards. Approvals and standalone choices bind to stable plan identity.
• Git reads are bounded and disable inherited Git environment/config, external helpers, lazy fetch, and transports; object alternates and symlinked object storage are rejected. Linear histories only.

Validation

• Final local npm test: 136 passing tests, including disposable real Git histories.
• npm run typecheck and git diff --check: pass.
• Baseline was 60 passing tests. The merged-contract regression suite first reproduced 18 failures; all now pass.
• Tests cover parsing boundaries, exact argv, registry copies/IDs, projected types and filesystem identity, delayed cross-plan suggestions, approval transfer, typed file metadata, and all prior history regressions.
• No agent execution, Docker, database, UI, or real-user go/no-go test ran: those phases do not exist in this slice.

Decisions and remaining work

core remains free of runtime I/O. Trusted context comes from the future runner/store, never from model output. The caller must supply actual filesystem identity and keep Git storage stable while reading. The pure suggestion function is not an HTTP endpoint or concurrency boundary.

See docs/implementation/build-step-1.md for API responsibilities and review history. #6 remains open for runtime link/occupancy audits, persistent suggestion identity/CAS, prompt and process budgets, and container enforcement. #2 and #3 remain the next approved build steps. Future typed symlink/gitlink authoring is tracked in #5 and #7.

Review status

The earlier foundation completed nine Copilot rounds; its last round had no new findings. This update changes the contract alignment substantially and requests a fresh review and current-head CI. Prior valid findings were fixed with regressions; declined Git alias/YAML claims and rename-card ownership interpretation are documented in the implementation notes. PR remains unmerged.

A local follow-up regression reproduced acceptance of C1 Unicode controls in paths/commands. The tokenizer and path checks now reject the full Unicode control category; the final suite is 136 passing tests with typecheck and whitespace checks passing.

Merged-contract review round 1

Fixed both stale README examples. Declined the scored-rename report because the regex already captures only the status letter; a new real-Git regression observed and successfully parsed a scored rename followed by another raw record before any parser change. All three threads answered and resolved. Final local suite: 136 passing tests, typecheck and whitespace checks pass. Fresh current-head review requested.

Merged-contract review round 2

This round reviewed earlier commit 9a2df1f. Declined both findings with executable probes: the fingerprint already includes the full shorthand item property (five independent field changes stale approval), and linkTarget calls the canonical path validator for every component (three .git/case/parent spellings reject rename while edit/delete repair passes). No code change was needed. Threads answered/resolved; requesting review of current head a8534cc.

Merged-contract review round 3

Fixed six findings after seven focused fixtures reproduced failures: bounded replay allocation/work and total runtime, shared Git subprocess deadline, metadata lineage through rename/deletion, non-directory link-target traversal, isolated unsafe-link repairs, and reverse-ancestry target overlap. Extended coverage to exact budgets, origin fanout, and text/empty/binary deletion cards. Final local suite: 132 tests, typecheck and whitespace checks pass. All six threads resolved and fresh review requested.

Merged-contract review round 4

Addressed the summary-only metadata reuse finding from review 5287702535. A real binary deletion/recreation regression reproduced lost earlier ownership; updates now merge existing path evidence before storing/propagating it. Owned and foreign deletion cases both pass, preserving Ambiguous/Unplanned classification. Final local suite: 134 tests, typecheck and whitespace checks pass. Fresh review requested.

Merged-contract review round 5

Reproduced the Git graft ancestry bypass using an unrelated root commit. The reader now rejects graft/shallow metadata before ancestry reads and disables grafts in child processes. Common-directory discovery covers linked-worktree metadata. Added graft/shallow regressions and clarified stable-metadata caller responsibility. Final local suite: 136 tests, typecheck and whitespace checks pass. Thread resolved and fresh review requested.

Final verification for the merged-contract update

Current head 611aedd targets main. Six additional Copilot rounds completed; review 5287900922 covers this exact head and reports Findings: None, with no inline comments or actionable summary details. All 24 review threads are resolved. The generic overview still says unspecified concerns remain, so this is not represented as formal approval.

Final rerun: 136 tests pass, typecheck passes, git diff --check passes, and the worktree is clean. Both current-head CI runs pass. Valid findings were fixed with reproduced regressions; three false positives (scored rename parsing, full item serialization, and .git target checking) were declined with executable evidence. PR remains unmerged. Runtime/store integration remains tracked in #6; ledger persistence (#2) is next in the approved order.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical Git alias execution risk and moderate attribution and choice-validation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Disable repository Git aliases to prevent arbitrary command execution · New
• Use segment-specific paths when checking scope after renames · New

What changed in this PR

Implements the first executable review-core slice: plan validation, safe suggestions, read-only Git history, ledger-backed attribution, and approval tracking.

Changes:

• Adds plan import, schema validation, command parsing, and suggestions.
• Adds linear Git history loading and change attribution.
• Adds approval/choice staleness handling.
• Adds tests, documentation, strict TypeScript configuration, dependencies, and CI.

File	Changes	Review notes
tsconfig.json	Strict TypeScript configuration	—
test/​plan.test.ts	Plan validation and suggestion tests	—
test/​history.test.ts	Git attribution and approval tests	—
README.md	Library usage and limitations	—
package.json	Scripts and dependencies	—
package-lock.json	Locked dependency versions	—
git/​history.ts	Read-only Git history adapter	Critical (1 vote): Local Git aliases can execute arbitrary shell commands; use sanitized configuration with aliases disabled for every subcommand.
docs/​implementation/​build-step-1.md	Implementation decisions and validation	—
core/​plan.ts	Plan validation and suggestions	—
core/​linking.ts	Ledger-backed change attribution	Moderate (2 votes): Scope text segments using the path actually changed, retaining both paths only for rename/file-card evidence. Moderate (1 vote): Preserve replayed rename provenance when rename detection falls back to delete/add deltas. Moderate (1 vote): Include moved owners when calculating replacement ownership so rename ancestry remains ambiguous where appropriate. Moderate (1 vote): Replay path deltas from a pre-commit snapshot before applying staged updates to handle swaps and rotations correctly.
core/​approvals.ts	Approval fingerprints and choices	Moderate (1 vote): Validate that accept has a null item and assign has a non-null item; reject malformed or future actions rather than treating them as assignments.
.gitignore	Generated-file exclusions	—
.github/​workflows/​ci.yml	Node CI workflow	—

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in Git isolation, attribution, and approval handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Isolate Git environment from external repository settings · New

Resolved since last review (2)

• Disable repository Git aliases to prevent arbitrary command execution
• Use segment-specific paths when checking scope after renames

Previously missed (1)

In code that hasn't changed since last review

Preserve rename provenance through deletion

core/​linking.ts:86

Metadata provenance is only stored under the immediately previous path. For a history a.txt -> b.txt owned by P1 followed by deleting b.txt owned by P2, the final base-to-head diff is a deletion of a.txt; this lookup then reports the file-change card as P1, while the actual deletion (and its text removals) belongs to P2. Carry metadata provenance through rename path ancestry (and merge the delete owner) before classifying the final card; add a rename-then-delete regression.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect repository isolation, history bounds, attribution, and approval state.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Sanitize inherited Git environment variables · New

Resolved since last review (1)

• Isolate Git environment from external repository settings

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved provenance, repository-isolation, and aggregate-memory findings block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Replacement evidence drops rename provenance · New
• Git alternates allow reads from external object stores · New

Resolved since last review (1)

• Sanitize inherited Git environment variables

[Copilot]

Copilot review overview

🟡 Changes recommended

Seven final review findings remain unresolved, including one critical aggregate-memory issue in git/history.ts.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Aggregate blob memory can exceed response limit · New

Resolved since last review (2)

• Git alternates allow reads from external object stores
• Replacement evidence drops rename provenance

[Copilot]

Copilot review overview

🟡 Changes recommended

Six unresolved findings remain, including one critical repository object-storage isolation issue.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Symlinked object storage bypasses alternates validation · New

Resolved since last review (1)

• Aggregate blob memory can exceed response limit

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved Git rename parsing, attribution, and target-validation issues remain, along with documentation nits.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 2

Open (3)

• Handle similarity scores when parsing rename statuses · New
• Update importPlan example to use the current PlanContext fields · New
• Pass checkout identity to linkHistory in the example · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect approval staleness, repository safety, parsing, history attribution, and documentation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Include complete plan items in approval fingerprints · New
• Reject symlink targets containing .git path components · New

Resolved since last review (3)

• Handle similarity scores when parsing rename statuses
• Pass checkout identity to linkHistory in the example
• Update importPlan example to use the current PlanContext fields

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in plan validation, Git history handling, and ledger linking.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 6

Open (6)

• Bound line and object allocation during blob replay · New
• Preserve rename metadata lineage for final deletion cards · New
• Reject non-directory intermediate components in link targets · New
• Reject link and target overlaps through unsafe symlink paths · New
• Detect reverse ancestry overlaps with retained link targets · New
• Enforce one overall deadline across readHistory Git subprocesses · New

Resolved since last review (2)

• Reject symlink targets containing .git path components
• Include complete plan items in approval fingerprints

[Copilot]

Copilot review overview

🔵 Needs a closer look

Four unresolved moderate findings remain in core/linking.ts, core/parse-v1.ts, core/plan.ts, and git/history.ts.

Review effort: Lite
Findings: None

Resolved since last review (6)

• Enforce one overall deadline across readHistory Git subprocesses
• Detect reverse ancestry overlaps with retained link targets
• Reject link and target overlaps through unsafe symlink paths
• Reject non-directory intermediate components in link targets
• Preserve rename metadata lineage for final deletion cards
• Bound line and object allocation during blob replay

Previously missed (1)

In code that hasn't changed since last review

Preserve metadata when deleted paths are recreated

core/​linking.ts:152

The global metadata map is overwritten when a path is deleted and later recreated (or when a new file reuses a previously deleted path). A final binary/mode/rename card for that path then loses the earlier deletion owner's metadata and can be shown only under the recreating commit, unlike the line-level attribution. Merge any existing path evidence into the value before storing it so reused paths retain all contributing owners.

[mchwang]

Addressed the summary-only finding in review 5287702535. A binary delete/recreate regression first retained only P2; metadata updates now merge existing path evidence before storing and propagating it. Owned deletion plus recreation retains P1/P2 as Ambiguous, and foreign deletion remains Unplanned. Final suite: 134 tests passing; typecheck and git diff --check pass.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved deterministic approval serialization and Git graft-handling issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Reject grafts when validating commit ancestry · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

Three unresolved moderate findings remain in core/plan.ts.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Reject grafts when validating commit ancestry