feat: add verification goal registry foundation (#187) - #328
Conversation
![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: fdc87012b7
ℹ️ 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".
| } | ||
| const projectRoot = await dependencies.bindProjectRoot(project) | ||
| const goals = await (dependencies.loadRegistry ?? loadVerificationGoalRegistry)(projectRoot) | ||
| return dependencies.store.importSnapshots(project.id, goals) |
There was a problem hiding this comment.
When projects.localPath changes concurrently after loadProject returns, this call persists definitions read from the old directory under a project that now points to the new directory; PUT /api/projects/:id can perform that update independently, and the snapshot records neither the bound root nor its revision, so its provenance cannot be recovered. Pass the loaded binding into the transaction, lock and re-read the project row before insertion, and abort if the authoritative binding changed.
Useful? React with 👍 / 👎.
| return typeof value === 'string' | ||
| && value.trim().length > 0 | ||
| && value.length <= maximumLength | ||
| && !SAFE_TEXT_CONTROL_CHARACTERS.test(value) |
There was a problem hiding this comment.
When a registry string contains an escaped unpaired surrogate such as "description":"\ud800", JSON.parse accepts it and this validation treats it as safe text, but PostgreSQL jsonb rejects the resulting invalid Unicode escape when the canonical definition is inserted. The registry therefore passes filesystem and contract validation only to fail later with a database error; reject unpaired surrogates here so the file receives the intended validation error before persistence.
Useful? React with 👍 / 👎.
1 participant
Source issue
Part of #187
Parent Epic: #184
Why this is next
The deterministic operation catalog (#201), canonical execution outcomes (#185 via PR #324), and capability reliability ledger (#186) are already merged. This starts #187 with its smallest safe implementation foundation.
What changed
.forge/verification-goals/*.jsonA stored snapshot is definition evidence only. It never means a goal ran or passed.
Architecture and security remediation
The review loop found and fixed these implementation blockers:
Commit
652eb14fixed the initial migration and registry-path findings. Commitfa43af2added pinned file/directory reads, membership re-attestation, symlink-rebind protection, and the shared migration-ledger helper. Commitfdc8701carries a validatedpath + device + inodeproject-root binding into the importer and adds deterministic no-storage regressions for both symlink and ordinary-directory replacement.Final architecture verdict
No blockers found in the inspected scope. High confidence in the reviewed architecture at exact head
fdc8701. In plain language: this is suitable for the bounded first slice—declarative goal definitions and immutable snapshots—and does not claim or introduce goal execution, pass/fail, scheduling, or reliability behavior. Review increases confidence; it is not proof that defects are impossible.Final Security/Adversarial verdict
No blockers found in the inspected scope. High confidence. All six deterministic root, registry-membership, symlink, and file-replacement attacks fail before storage.
One low-severity future boundary is recorded: the project-root binding is an identity token, not a directory handle held continuously across the importer handoff. That is acceptable here because repository definitions are strictly non-executable and the practical inode-reuse attack was not reproduced. A later execution slice must use an opaque live directory lease rather than reuse this snapshot-import primitive as an execution trust boundary.
Deliberately deferred
This PR does not add goal runs, execution or dispatch, manual-run API or UI, schedules or Redis jobs, canonical outcome writes, reliability ingestion, last-green or first-failure state, retries, notifications, autonomous repair, or issue #188 behavior.
Validation
Final-head evidence:
31823212940: passed at exact headfdc8701Remaining scope limits
Linux, NFS, and other filesystem-specific inode/timeout behavior was not separately exercised. Future API authorization, queueing, execution, and UI integration remain outside this PR and require their own architecture and security passes.