Skip to content

fix: harden greenfield + repair paths (2nd-pass review — 2×P1, 2×P2)#45

Merged
agjs merged 1 commit into
mainfrom
fix/greenfield-hardening
Jun 22, 2026
Merged

fix: harden greenfield + repair paths (2nd-pass review — 2×P1, 2×P2)#45
agjs merged 1 commit into
mainfrom
fix/greenfield-hardening

Conversation

@agjs

@agjs agjs commented Jun 22, 2026

Copy link
Copy Markdown
Owner

Why

A second-pass review of the greenfield/repair paths (shipped in 0.20.0 via #44) found four real issues — two security/correctness P1s and two P2s. This is the hardening pass before the feature is settled.

Fixes

[P1] Contract persistence trusted model-controlled feature IDs as paths.
writeContract did join(dir, \${feature.id}.md`), and the planner accepted any string id — so with TSFORGE_CONTRACT=1a feature id like../../../README escaped the state dir (reproduced in a temp repo). Now: feature ids are validated kebab-case (isFeatureId) at **both** parse (plan.ts) and load (state.ts), so a path-like id is dropped; writeContractadditionally derives the filename from abasenamewith a safe"feature"` fallback (defence-in-depth).

[P1] Rollback left newly-created binary/asset files behind.
restoreFiles tombstoned through resolveScopeFiles, which intentionally filters out .svg/images/fonts for prompt safety — so a failed repair that created icon.svg reported "reverted" while leaving it on disk (reproduced). Added resolveScopeFilesForRollback (uncapped, ignored-dirs-only, binary-inclusive); the snapshot's existed-set and the tombstone scan use it, while text content is still captured via the filtered resolver (writing back .text() of a binary would corrupt it).

[P2] Greenfield --browser resolved relative paths from the launcher cwd, not --dir.
Normal gate checks run inside --dir, but greenfield's in-process renderCheck did not. Relative --browser is now resolved against args.dir.

[P2] Accept-rate metrics undercounted multi-file reverts.
One reverted event subtracted only 1, but a failed batch may roll back N edits — making /trace optimistic. The reverted event now carries a batch mutation count (review-repair + quality tally edit/create events during the attempt); analyzeEvents subtracts event.count (default 1), and count round-trips through the ledger.

Verification

  • bun run validate green — 1421 pass / 0 fail.
  • New regression tests for each fix; both P1s flip-and-confirm-failed (svg-tombstone and path-traversal-id).
  • Targets the same merged feature; no behavior change outside the greenfield/repair paths.

…nd-pass review)

[P1] Contract path traversal via model-controlled feature ids: feature ids are
  now validated kebab-case at parse (plan.ts) AND load (state.ts) via isFeatureId,
  so '../../README' is dropped; writeContract derives the filename from a
  basename with a safe fallback as defence-in-depth.
[P1] Rollback left created binary/asset files behind: the prompt-facing resolver
  skips .svg/images/fonts, so tombstoning missed them. Added
  resolveScopeFilesForRollback (uncapped, ignored-dirs-only, binary-inclusive);
  file-snapshot uses it for the existed set + tombstone scan, while content is
  still captured from the text-only resolver.
[P2] Greenfield --browser path now resolves against --dir, not the launcher cwd.
[P2] Accept-rate undercounted multi-file reverts: the reverted event now carries
  a batch mutation count (review-repair + quality tally edit/create events);
  metrics subtract event.count (default 1); count round-trips through the ledger.

Full validate green (1421 pass).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several enhancements and bug fixes: it resolves relative --browser paths against the run directory, tracks the exact mutation count of reverted batches to improve accept-rate metric accuracy, ensures rollback operations can detect and clean up newly created binary assets, and adds strict validation for feature IDs to prevent path traversal vulnerabilities. The reviewer's feedback suggests a valuable performance optimization in file-snapshot.ts to combine the separate text and binary file traversals into a single pass, reducing disk I/O by 50% in large workspaces, which subsequently allows for cleaning up the unused textFiles helper and its associated import.

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.

Comment thread packages/core/src/loop/file-snapshot.ts
Comment thread packages/core/src/loop/file-snapshot.ts
Comment thread packages/core/src/loop/file-snapshot.ts
@agjs agjs merged commit 8493579 into main Jun 22, 2026
8 checks passed
@agjs agjs deleted the fix/greenfield-hardening branch June 22, 2026 13:33
agjs added a commit that referenced this pull request Jun 22, 2026
Collapse the two scope traversals into one binary-inclusive walk, classifying
text vs binary in-memory via a new exported isBinaryPath. Removes the unused
textFiles/existedSet helpers and the resolveScopeFiles import. Behavior
unchanged (tombstone + rewrite tests green).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant