-
Notifications
You must be signed in to change notification settings - Fork 174
feat: add sink-parity agent skill #1689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dcoric
wants to merge
3
commits into
finos:feat/postgres
Choose a base branch
from
dcoric:feat/sink-parity-skill
base: feat/postgres
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| --- | ||
| name: sink-parity | ||
| description: Keep the fs, mongo and postgres sink backends at feature parity when changing any of them | ||
| --- | ||
|
|
||
| Keep the sink backends at feature parity. | ||
|
|
||
| Read `AGENTS.md` first. It is the canonical project guide for this repository. | ||
|
|
||
| GitProxy persists its state through interchangeable sink backends: `src/db/file` (NeDB), `src/db/mongo`, and `src/db/postgres`. They all implement the `Sink` interface in `src/db/types.ts`, and deployments pick one via the `sink` config. A feature that exists in one backend but not the others is a bug waiting for whichever deployment uses the others. | ||
|
|
||
| Use this skill whenever a change touches any of: | ||
|
|
||
| - the `Sink` interface or the entity classes (`Repo`, `User`, push types) in `src/db/types.ts` | ||
| - any backend adapter under `src/db/file`, `src/db/mongo`, or `src/db/postgres` | ||
| - the migration framework (`src/db/migrations`) or the postgres schema | ||
|
|
||
| ## The contract | ||
|
|
||
| - `src/db/types.ts` is the single source of truth. A new `Sink` member or entity field is not done until all three backends implement it in the same change; do not leave a backend behind for a follow-up. | ||
| - `npm run check-types:server` enforces the interface structurally, but it cannot see semantic drift. The rest of this checklist exists for what the compiler cannot catch. | ||
|
|
||
| ## Adding or changing a Sink member | ||
|
|
||
| 1. Add the member to the `Sink` interface with a doc comment stating its semantics (ordering, case sensitivity, empty-result shape). | ||
| 2. Implement it in `src/db/file`, `src/db/mongo`, and `src/db/postgres`. Use the mongo implementation as the reference for behaviour unless the doc comment says otherwise. | ||
| 3. Export it from each backend's `index.ts` and wire the dispatcher in `src/db/index.ts`. | ||
| 4. Add unit tests for every backend, not just the one you started from. | ||
|
|
||
| ## Adding a field to an entity | ||
|
|
||
| 1. Update the class in `src/db/types.ts`. | ||
| 2. fs and mongo store documents whole, so writes usually pass new fields through automatically; verify reads return them. | ||
| 3. postgres maps fields to columns explicitly, so every layer must be updated by hand: | ||
| - schema: add the column (see the migration rules below) | ||
| - create: insert the field, applying the same defaults as mongo (for example `dateCreated`/`lastModified` are stamped with the current ISO time on create) | ||
| - update: extend the column allowlist; a field missing from the allowlist is dropped silently, and an update reduced to zero columns throws, which has already nearly shipped a startup crash (`populateRepoDates`) | ||
| - read: add the column to every select and to the row-to-entity mapping | ||
| 4. If mongo or fs bump `lastModified` (or similar) on a mutation, every backend must bump it on that mutation. | ||
|
|
||
| ## Postgres schema changes | ||
|
|
||
| - Schema changes are append-only migrations; never edit or reorder an entry that has shipped. | ||
| - Cross-backend logical migrations belong in `src/db/migrations` (registered in `registry.ts`) and run through the `Sink` hooks (`getAppliedMigrations`, `recordMigration`, `unrecordMigration`), so they must work against all three backends. | ||
| - `deriveCreatedAt` is best-effort by design: mongo derives a timestamp from the ObjectId, fs and postgres return `undefined` and callers fall back. Do not assume it returns a value. | ||
|
|
||
| ## Semantic parity rules | ||
|
|
||
| - Same defaults on create in every backend. | ||
| - Same case handling: usernames are lowercased on permission changes; name lookups are case-insensitive where mongo's are. | ||
| - Same projections: list endpoints must return the same field set from every backend, or UI behaviour diverges by deployment. | ||
| - Same error behaviour for invalid input (missing id, empty update). | ||
|
|
||
| ## Verify before pushing | ||
|
|
||
| ``` | ||
| npm run check-types:server | ||
| cross-env NODE_ENV=test npx vitest --run test/db | ||
| npm run lint | ||
| npm run format:check | ||
| ``` | ||
|
|
||
| Postgres integration tests (`npm run test:integration:postgres`) need a reachable PostgreSQL database; CI runs them in the dedicated lane. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be good to add a small testing section too:
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| description: Keep the fs, mongo and postgres sink backends at feature parity | ||
| --- | ||
|
|
||
| Follow @.agents/skills/sink-parity/SKILL.md. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the assumption here that usually a Mongo implementation will be added first (and the others must be filled in)? I feel this could be misleading if the agent takes it too literally...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering about what the "source of truth" should even be... The Mongo implementation is by default the "official" one, but it doesn't mean it'll always be the one to use as a template, especially if new Postgres functionality is added.