feat: Requirement edits lack optimistic-concurrency protection - #101
Merged
Conversation
…t across requirement-related tests
… version lifecycle
…lifecycle documentation
…ling in contributor guide
…rrency control - Added `revision_token` column to `requirement_versions` table to manage optimistic concurrency. - Updated database schema documentation to reflect the new column and its constraints. - Enhanced TypeORM entity to include `revisionToken`. - Created migration to add `revision_token` to the existing `requirement_versions` table. - Updated various schemas in the server code to include `revisionToken` in requirement version outputs and mutations. - Modified tests to ensure migrations are correctly registered and executed.
…d for requirement editing
…and baseRevisionToken
…d in RequirementForm
…d across requirement editing logic
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds optimistic-concurrency for requirement edits by introducing per-version Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Edit Client
participant API as API Route
participant Service
participant DAL
participant DB
User->>UI: Open edit (load latest)
UI->>API: GET requirement history
API->>Service: getRequirement / history
Service->>DAL: getLatestVersionLite / getRequirementById
DAL->>DB: SELECT latest version (includes revision_token)
DB-->>DAL: latest version + revision_token
DAL-->>Service: version with revisionToken
Service-->>API: requirement
API-->>UI: requirement (includes baseVersionId, baseRevisionToken)
User->>UI: Submit edits (includes baseVersionId, baseRevisionToken)
UI->>API: PUT /requirements/[id] with baseVersionId, baseRevisionToken, changes
API->>Service: manageRequirement(edit, payload)
Service->>DAL: editRequirement(id, baseVersionId, baseRevisionToken, changes)
DAL->>DB: BEGIN TRANSACTION (SERIALIZABLE)
DAL->>DB: SELECT ... WITH (UPDLOCK, HOLDLOCK) to lock latest
DB-->>DAL: current latest version + revision_token
alt base matches latest
DAL->>DB: UPDATE requirement_versions SET ..., revision_token=NEWID() WHERE id=@latestId AND revision_token=CONVERT(uniqueidentifier, `@baseRevisionToken`)
DB-->>DAL: rowsAffected = 1
DAL->>DB: COMMIT
DAL-->>Service: updated version (new revisionToken)
Service-->>API: 200 OK with updated data
API-->>UI: 200 OK
else mismatch (stale)
DAL->>DB: ROLLBACK
DAL->>DB: SELECT latest snapshot
DB-->>DAL: latest snapshot
DAL-->>Service: throw conflictError(reason='stale_requirement_edit', latest=...)
Service-->>API: 409 Conflict with structured payload
API-->>UI: 409 Conflict + latest snapshot
UI->>User: Show stale-edit alert (view latest / reload)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
+ Coverage 56.80% 57.35% +0.54%
==========================================
Files 257 257
Lines 16880 17013 +133
Branches 6376 6554 +178
==========================================
+ Hits 9589 9757 +168
+ Misses 7178 7143 -35
Partials 113 113
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
typeorm/migrations/0002_requirement_version_revision_token.mjs (1)
1-8: Use a UNIQUE constraint here to mirror the entity metadata.
lib/typeorm/entities/requirement-version.tsmodelsuq_requirement_versions_revision_tokenas a unique constraint, but this migration creates a unique index with the same name. SQL Server treats those as different schema objects, so future metadata-based diffs can think the constraint is still missing.Suggested fix
const UP_STATEMENTS = [ - "IF COL_LENGTH(N'dbo.requirement_versions', N'revision_token') IS NULL\nBEGIN\n ALTER TABLE [requirement_versions] ADD [revision_token] uniqueidentifier NULL;\n UPDATE [requirement_versions] SET [revision_token] = NEWID() WHERE [revision_token] IS NULL;\n ALTER TABLE [requirement_versions] ALTER COLUMN [revision_token] uniqueidentifier NOT NULL;\n ALTER TABLE [requirement_versions] ADD CONSTRAINT [df_requirement_versions_revision_token] DEFAULT NEWID() FOR [revision_token];\n CREATE UNIQUE INDEX [uq_requirement_versions_revision_token] ON [requirement_versions] ([revision_token]);\nEND", + "IF COL_LENGTH(N'dbo.requirement_versions', N'revision_token') IS NULL\nBEGIN\n ALTER TABLE [requirement_versions] ADD [revision_token] uniqueidentifier NULL;\n UPDATE [requirement_versions] SET [revision_token] = NEWID() WHERE [revision_token] IS NULL;\n ALTER TABLE [requirement_versions] ALTER COLUMN [revision_token] uniqueidentifier NOT NULL;\n ALTER TABLE [requirement_versions] ADD CONSTRAINT [df_requirement_versions_revision_token] DEFAULT NEWID() FOR [revision_token];\n ALTER TABLE [requirement_versions] ADD CONSTRAINT [uq_requirement_versions_revision_token] UNIQUE ([revision_token]);\nEND", ] const DOWN_STATEMENTS = [ - "IF EXISTS (SELECT 1 FROM sys.indexes WHERE name = N'uq_requirement_versions_revision_token' AND object_id = OBJECT_ID(N'dbo.requirement_versions')) DROP INDEX [uq_requirement_versions_revision_token] ON [requirement_versions];", + "IF EXISTS (SELECT 1 FROM sys.key_constraints WHERE name = N'uq_requirement_versions_revision_token' AND parent_object_id = OBJECT_ID(N'dbo.requirement_versions')) ALTER TABLE [requirement_versions] DROP CONSTRAINT [uq_requirement_versions_revision_token];", "IF EXISTS (SELECT 1 FROM sys.default_constraints WHERE name = N'df_requirement_versions_revision_token' AND parent_object_id = OBJECT_ID(N'dbo.requirement_versions')) ALTER TABLE [requirement_versions] DROP CONSTRAINT [df_requirement_versions_revision_token];", "IF COL_LENGTH(N'dbo.requirement_versions', N'revision_token') IS NOT NULL ALTER TABLE [requirement_versions] DROP COLUMN [revision_token];", ]As per coding guidelines, "Always pass an explicit
nameto TypeORM@Index/@Uniquedecorators and to migration SQLCREATE INDEX/ALTER TABLE … ADD CONSTRAINT."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typeorm/migrations/0002_requirement_version_revision_token.mjs` around lines 1 - 8, The migration creates a UNIQUE INDEX named uq_requirement_versions_revision_token but the entity (lib/typeorm/entities/requirement-version.ts) models it as a UNIQUE constraint, so change the migration to create a UNIQUE constraint instead: in UP_STATEMENTS replace the CREATE UNIQUE INDEX line with an ALTER TABLE [requirement_versions] ADD CONSTRAINT [uq_requirement_versions_revision_token] UNIQUE ([revision_token]); and in DOWN_STATEMENTS remove the DROP INDEX line and add a conditional DROP CONSTRAINT for uq_requirement_versions_revision_token (use sys.objects/sys.constraints or IF EXISTS (SELECT 1 FROM sys.objects WHERE name = N'uq_requirement_versions_revision_token' ...) ALTER TABLE [requirement_versions] DROP CONSTRAINT [uq_requirement_versions_revision_token];) so the constraint name exactly matches the entity metadata.lib/dal/requirements.ts (4)
880-908: DuplicatedrequirementAreaIdupdate across both edit branches.The same
UPDATE requirements SET requirement_area_id =@0WHERE id =@1`` block now appears in the draft path (L880-885) and the published-creates-new-draft path (L903-908). Extracting a small helper inside the transaction keeps the SQL definition in one place and makes future column additions to that side-effect (e.g., updated_at) easier.♻️ Sketch
+ const updateAreaIfRequested = async () => { + if (data.requirementAreaId == null) return + await tx.query( + `UPDATE requirements SET requirement_area_id = `@0` WHERE id = `@1``, + [data.requirementAreaId, requirementId], + ) + } @@ - if (data.requirementAreaId != null) { - await tx.query( - `UPDATE requirements SET requirement_area_id = `@0` WHERE id = `@1``, - [data.requirementAreaId, requirementId], - ) - } + await updateAreaIfRequested() @@ - if (data.requirementAreaId != null) { - await tx.query( - `UPDATE requirements SET requirement_area_id = `@0` WHERE id = `@1``, - [data.requirementAreaId, requirementId], - ) - } + await updateAreaIfRequested()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/dal/requirements.ts` around lines 880 - 908, The update of requirement_area_id is duplicated in both edit branches; refactor by extracting a small helper inside the transaction (e.g., an inner async function or local helper like updateRequirementArea) that takes tx, requirementId, and data.requirementAreaId and performs the tx.query(`UPDATE requirements SET requirement_area_id = `@0` WHERE id = `@1``, [...]); replace both occurrences in the branches (the block near insertVersionJoinsSqlServer/current.id branch and the block before getNextVersionNumberSqlServer) with calls to that helper so the SQL is defined in one place and can be extended (e.g., adding updated_at) centrally.
755-795: Add explicit return types to the new helpers.
normalizeBaseVersionId,normalizeBaseRevisionToken, andstaleRequirementEditErrorrely on inference. As per coding guidelines (lib/**/*.ts: "Explicitly specify return types for all functions in utilities"), each should declare its return type so refactors elsewhere can't silently widen the contract.♻️ Proposed signatures
-function normalizeBaseVersionId(value: number | null | undefined) { +function normalizeBaseVersionId(value: number | null | undefined): number { @@ -function normalizeBaseRevisionToken(value: string | null | undefined) { +function normalizeBaseRevisionToken(value: string | null | undefined): string { @@ -function staleRequirementEditError( - requirementId: number, - baseVersionId: number, - latestVersionId: number, -) { +function staleRequirementEditError( + requirementId: number, + baseVersionId: number, + latestVersionId: number, +): ReturnType<typeof conflictError> {As per coding guidelines: "Explicitly specify return types for all functions in utilities".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/dal/requirements.ts` around lines 755 - 795, The three helpers lack explicit return types; add them: declare normalizeBaseVersionId(value: number | null | undefined): number, normalizeBaseRevisionToken(value: string | null | undefined): string, and staleRequirementEditError(requirementId: number, baseVersionId: number, latestVersionId: number): ReturnType<typeof conflictError> (or the specific error type your project uses) so their contracts are explicit and future refactors can't widen returns; update the function signatures for normalizeBaseVersionId, normalizeBaseRevisionToken, and staleRequirementEditError accordingly.
826-839: Consider running the stale-edit check before the lifecycle status checks.If a stale editor's base is mismatched AND the requirement has since moved to
REVIEW/ARCHIVED, the user currently sees'Cannot edit a requirement in Review status'/'Cannot edit an archived requirement…'instead of the stale-edit conflict that the PR added the refresh/compare UI flow for. That's still technically correct, but it bypasses the newreason: 'stale_requirement_edit'payload (withlatestVersionId) the UI is now wired to handle. Surfacing the stale conflict first gives consistent UX whenever the user's base is out-of-date, regardless of where the requirement subsequently landed.♻️ Suggested ordering
- if (current.statusId === STATUS_REVIEW) { - throw conflictError('Cannot edit a requirement in Review status') - } - if (current.statusId === STATUS_ARCHIVED) { - throw conflictError( - 'Cannot edit an archived requirement — restore it first', - ) - } if ( current.id !== baseVersionId || current.revisionToken !== baseRevisionToken ) { throw staleRequirementEditError(requirementId, baseVersionId, current.id) } + if (current.statusId === STATUS_REVIEW) { + throw conflictError('Cannot edit a requirement in Review status') + } + if (current.statusId === STATUS_ARCHIVED) { + throw conflictError( + 'Cannot edit an archived requirement — restore it first', + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/dal/requirements.ts` around lines 826 - 839, Move the stale-edit validation to run before lifecycle status checks: instead of first checking current.statusId against STATUS_REVIEW/STATUS_ARCHIVED, first compare current.id and current.revisionToken with baseVersionId and baseRevisionToken and throw staleRequirementEditError(requirementId, baseVersionId, current.id) when they mismatch; keep the existing status checks (throwing conflictError for STATUS_REVIEW and STATUS_ARCHIVED) but execute them only after the stale check so the stale_requirement_edit payload (including latestVersionId) is consistently returned to the UI.
691-691:editedAtis selected and mapped but never read.
VersionLite.editedAt, the newedited_at AS editedAtcolumn ingetLatestVersionLite, and the corresponding map at L738 are not consumed by any caller (editRequirement,deleteDraftVersion,transitionStatus). Per the commit history,editedAtwas the original optimistic-concurrency precondition before the switch torevision_token; this looks like a leftover. Either remove it to keep the lite read minimal, or leave a brief comment explaining the intentional retention.Also applies to: 717-717, 738-738
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/dal/requirements.ts` at line 691, VersionLite.editedAt is an unused leftover: remove the editedAt property from the VersionLite type and stop selecting/mapping edited_at AS editedAt in getLatestVersionLite and its map (the mapping around the current editedAt assignment), since callers (editRequirement, deleteDraftVersion, transitionStatus) do not read it; alternatively, if you intentionally want to keep it, add a brief comment near the VersionLite declaration and the getLatestVersionLite mapping noting it's retained for historical/compatibility reasons — pick one approach and update any tests or usages accordingly.tests/unit/mcp-http.test.ts (2)
639-697: Edit-mock test pair correctly exercises the new precondition contract.
accepts normReferenceIds in manage_requirementnow passesbaseRevisionToken/baseVersionIdand asserts they reachmanageRequirement, matching the newsuperRefinerequirement inlib/mcp/server.ts(lines 821-846). The companionrejects the old references field …test still validates strict() unknown-field rejection — note that with the newsuperRefine, this request also lacksbaseVersionId/baseRevisionToken, so multiple validation errors will be raised; the/unrecognized/iregex still matches because.strict()issues are surfaced alongsidesuperRefineones. Behavior is fine, but if a future Zod change reorders/short-circuits errors, this test could become brittle. Consider including the base fields here so the test is unambiguously about the unknownreferencesfield.♻️ Suggested tightening so the test isolates the unknown-field rejection
arguments: { operation: 'edit', uniqueId: 'INT0001', requirement: { + baseRevisionToken: '11111111-1111-4111-8111-111111111111', + baseVersionId: 10, description: 'Updated description', references: [1], }, }, name: 'requirements_manage_requirement',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-http.test.ts` around lines 639 - 697, The second test "rejects the old references field in manage_requirement due to strict schema" is currently ambiguous because it also omits baseRevisionToken/baseVersionId and thus triggers the superRefine precondition; update that test to include the required baseRevisionToken and baseVersionId in the request body so the failure is isolated to the unknown "references" field. Locate the test by the it() description or the tool name "requirements_manage_requirement" and add the same baseRevisionToken/baseVersionId values used in the "accepts normReferenceIds in manage_requirement" test so the assertion that content[0]?.text matches /unrecognized/i only reflects the strict() unknown-field rejection.
221-303: Consider splitting the omnibus tool-schema assertion test.This single test block now asserts on input/output schemas for six different tools (
requirements_manage_requirement,requirements_query_catalog,requirements_get_requirement,requirements_transition_requirement,requirements_list_improvement_suggestions,requirements_manage_improvement_suggestion,requirements_generate_requirements) plus performs the original resource-read flow. When any one schema drifts, the failure name will be misleading ('lists the MCP tools and serves the requirement detail resource') and the developer has to scan ~80 assertions to find the actual mismatch.For "Chill" mode this isn't a blocker, but extracting per-tool
it(...)blocks (or adescribe('tool schemas', ...)group) would localize failures and document each tool's contract independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-http.test.ts` around lines 221 - 303, The test "lists the MCP tools and serves the requirement detail resource" bundles schema assertions for many tools which obscures failures; split it into focused tests (or a describe('tool schemas') group) by creating separate it blocks that each locate the specific tool (e.g., requirements_manage_requirement, requirements_query_catalog, requirements_get_requirement, requirements_transition_requirement, requirements_list_improvement_suggestions, requirements_manage_improvement_suggestion, requirements_generate_requirements) and move only that tool's input/output/description assertions into that block, leaving the original resource-read flow (the requirement detail assertions) as its own test; ensure each new test still calls createClient() or reuses the setup to obtain tools and uses the same symbol lookups (tools.tools.find(...) for the tool.name) so failures are localized per tool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/skills/run-spec-audit/references/scrutiny-areas.md:
- Around line 60-71: Update the Scenario 11 heading in QUALITY.md so it exactly
matches the test name string used in functional.test.ts: replace the current
title-case heading text with the exact test name "Scenario 11: stale draft edits
are rejected before replacing latest content" to ensure verbatim equality with
the test selector.
In `@components/RequirementForm.tsx`:
- Around line 244-249: The stale-conflict UI assumes staleConflict.latest exists
and fires onRefreshLatest() without awaiting or error handling; update the
rendering and actions to be resilient by: only render the "View latest" button
and build latestConflictHref when staleConflict?.latest is present (use
latestConflictVersion and latestConflictTarget references) and otherwise hide or
disable that action; introduce an isRefreshing state flag, await
onRefreshLatest() (wrap in try/catch) and set isRefreshing to disable
conflicting controls (disabled={isRefreshing}) and show inline loading feedback
(e.g., {isRefreshing ? t('loading') : t('view latest')}); apply the same pattern
for the other stale-conflict controls referenced in the same component (the
block around lines 347-385).
In `@docs/database-schema.md`:
- Line 1286: The Mermaid Index Relationship Diagram is missing a self-edge for
the new unique index uq_requirement_versions_revision_token; update the
diagram's graph LR to add a self-edge from the requirement_versions node (RV)
back to itself using the same convention as other unique indexes, e.g. RV --
"uq_requirement_versions_revision_token\n(revision_token)" --> RV so the new
index appears in the diagram alongside the existing unique indexes.
In `@docs/version-lifecycle-dates.md`:
- Around line 51-54: Update the stale paragraph that instructs callers to send
edited_at as the optimistic concurrency token: change it to describe the new
contract using baseVersionId and baseRevisionToken as the precondition tokens.
Reference the implemented normalized fields (baseVersionId and
baseRevisionToken) and remove or replace any mention of edited_at so the doc
matches the behavior enforced by normalizeBaseVersionId /
normalizeBaseRevisionToken and the rest of the codebase.
In `@lib/dal/requirements.ts`:
- Line 815: Add explicit return type annotations to the three helper functions:
annotate normalizeBaseVersionId(...) with : number, annotate
normalizeBaseRevisionToken(...) with : string, and annotate
staleRequirementEditError(...) with the same return type as conflictError()
(i.e., the return type of conflictError). Update each function signature to
include these explicit return types so TypeScript enforces the expected types.
In `@tests/unit/requirements-id-route.test.ts`:
- Around line 24-32: The mocked toHttpErrorPayload used in the tests returns
status 400 and leaves code undefined for plain Errors, but production maps plain
Errors to { code: 'internal', status: 500 } in lib/requirements/service.ts;
update the mock (toHttpErrorPayload) so it defaults to status: err.status ?? 500
and code: err.code ?? 'internal' (i.e., set the fallback for code to 'internal'
and the fallback for status to 500) to align test failure-path assertions with
production behavior.
---
Nitpick comments:
In `@lib/dal/requirements.ts`:
- Around line 880-908: The update of requirement_area_id is duplicated in both
edit branches; refactor by extracting a small helper inside the transaction
(e.g., an inner async function or local helper like updateRequirementArea) that
takes tx, requirementId, and data.requirementAreaId and performs the
tx.query(`UPDATE requirements SET requirement_area_id = `@0` WHERE id = `@1``,
[...]); replace both occurrences in the branches (the block near
insertVersionJoinsSqlServer/current.id branch and the block before
getNextVersionNumberSqlServer) with calls to that helper so the SQL is defined
in one place and can be extended (e.g., adding updated_at) centrally.
- Around line 755-795: The three helpers lack explicit return types; add them:
declare normalizeBaseVersionId(value: number | null | undefined): number,
normalizeBaseRevisionToken(value: string | null | undefined): string, and
staleRequirementEditError(requirementId: number, baseVersionId: number,
latestVersionId: number): ReturnType<typeof conflictError> (or the specific
error type your project uses) so their contracts are explicit and future
refactors can't widen returns; update the function signatures for
normalizeBaseVersionId, normalizeBaseRevisionToken, and
staleRequirementEditError accordingly.
- Around line 826-839: Move the stale-edit validation to run before lifecycle
status checks: instead of first checking current.statusId against
STATUS_REVIEW/STATUS_ARCHIVED, first compare current.id and
current.revisionToken with baseVersionId and baseRevisionToken and throw
staleRequirementEditError(requirementId, baseVersionId, current.id) when they
mismatch; keep the existing status checks (throwing conflictError for
STATUS_REVIEW and STATUS_ARCHIVED) but execute them only after the stale check
so the stale_requirement_edit payload (including latestVersionId) is
consistently returned to the UI.
- Line 691: VersionLite.editedAt is an unused leftover: remove the editedAt
property from the VersionLite type and stop selecting/mapping edited_at AS
editedAt in getLatestVersionLite and its map (the mapping around the current
editedAt assignment), since callers (editRequirement, deleteDraftVersion,
transitionStatus) do not read it; alternatively, if you intentionally want to
keep it, add a brief comment near the VersionLite declaration and the
getLatestVersionLite mapping noting it's retained for historical/compatibility
reasons — pick one approach and update any tests or usages accordingly.
In `@tests/unit/mcp-http.test.ts`:
- Around line 639-697: The second test "rejects the old references field in
manage_requirement due to strict schema" is currently ambiguous because it also
omits baseRevisionToken/baseVersionId and thus triggers the superRefine
precondition; update that test to include the required baseRevisionToken and
baseVersionId in the request body so the failure is isolated to the unknown
"references" field. Locate the test by the it() description or the tool name
"requirements_manage_requirement" and add the same
baseRevisionToken/baseVersionId values used in the "accepts normReferenceIds in
manage_requirement" test so the assertion that content[0]?.text matches
/unrecognized/i only reflects the strict() unknown-field rejection.
- Around line 221-303: The test "lists the MCP tools and serves the requirement
detail resource" bundles schema assertions for many tools which obscures
failures; split it into focused tests (or a describe('tool schemas') group) by
creating separate it blocks that each locate the specific tool (e.g.,
requirements_manage_requirement, requirements_query_catalog,
requirements_get_requirement, requirements_transition_requirement,
requirements_list_improvement_suggestions,
requirements_manage_improvement_suggestion, requirements_generate_requirements)
and move only that tool's input/output/description assertions into that block,
leaving the original resource-read flow (the requirement detail assertions) as
its own test; ensure each new test still calls createClient() or reuses the
setup to obtain tools and uses the same symbol lookups (tools.tools.find(...)
for the tool.name) so failures are localized per tool.
In `@typeorm/migrations/0002_requirement_version_revision_token.mjs`:
- Around line 1-8: The migration creates a UNIQUE INDEX named
uq_requirement_versions_revision_token but the entity
(lib/typeorm/entities/requirement-version.ts) models it as a UNIQUE constraint,
so change the migration to create a UNIQUE constraint instead: in UP_STATEMENTS
replace the CREATE UNIQUE INDEX line with an ALTER TABLE [requirement_versions]
ADD CONSTRAINT [uq_requirement_versions_revision_token] UNIQUE
([revision_token]); and in DOWN_STATEMENTS remove the DROP INDEX line and add a
conditional DROP CONSTRAINT for uq_requirement_versions_revision_token (use
sys.objects/sys.constraints or IF EXISTS (SELECT 1 FROM sys.objects WHERE name =
N'uq_requirement_versions_revision_token' ...) ALTER TABLE
[requirement_versions] DROP CONSTRAINT
[uq_requirement_versions_revision_token];) so the constraint name exactly
matches the entity metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: caf7338f-913c-44ec-b7fd-128175b891c2
📒 Files selected for processing (35)
.github/instructions/mcp-tool-contract.instructions.md.github/skills/run-spec-audit/references/scrutiny-areas.mdapp/[locale]/requirements/[id]/edit/edit-requirement-client.tsxapp/[locale]/requirements/requirements-client.tsxapp/api/requirements/[id]/route.tscomponents/RequirementForm.tsxcspell.jsoncdocs/database-schema.mddocs/lifecycle-workflow.mddocs/mcp-server-contributor-guide.mddocs/mcp-server-user-guide.mddocs/version-lifecycle-dates.mdlib/dal/requirements.tslib/mcp/server.tslib/requirements/list-view.tslib/requirements/service.tslib/requirements/types.tslib/typeorm/entities/requirement-version.tsmessages/en.jsonmessages/sv.jsonscripts/__tests__/db-sqlserver-admin.test.mjsscripts/db-sqlserver-admin.mjstests/quality/QUALITY.mdtests/quality/functional.test.tstests/unit/edit-requirement-client.test.tsxtests/unit/mcp-http.test.tstests/unit/requirement-detail-client.test.tsxtests/unit/requirement-form.test.tsxtests/unit/requirements-client.test.tsxtests/unit/requirements-dal.test.tstests/unit/requirements-id-route.test.tstests/unit/requirements-service.test.tstests/unit/version-detail-client.test.tsxtypeorm/migrations/0001_initial_sqlserver.mjstypeorm/migrations/0002_requirement_version_revision_token.mjs
…versions migration
…with baseVersionId and baseRevisionToken
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/mcp-http.test.ts (2)
221-378: Optional: extract a small helper for the repeated tool-lookup boilerplate.The eight
tool schemascases all repeatcreateClient → listTools → tools.find(...) → close. A tiny helper would tighten the suite and reduce drift if more tools are added.♻️ Sketch
+ async function getTool(name: string) { + const { client, transport } = await createClient() + const { tools } = await client.listTools() + const tool = tools.find(t => t.name === name) + return { + tool, + tools, + cleanup: async () => { + await client.close() + await transport.close() + }, + } + }Each case then becomes
const { tool, cleanup } = await getTool('...'); ...; await cleanup().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-http.test.ts` around lines 221 - 378, The tests repeat createClient → listTools → tools.find(...) → close boilerplate across the "tool schemas" suite; add a small async helper (e.g., getTool) that calls createClient(), awaits client.listTools(), finds the tool by name (using tools.find), and returns { tool, cleanup } where cleanup closes both client and transport; replace each test's repeated sequence with a call to getTool('requirements_...') and await cleanup() at the end to keep behavior identical while removing duplication.
108-124: Consider asserting the rotatedrevisionTokenis surfaced byrequirements_manage_requirement.The mock now exposes a
revisionTokenondetail.versions[0], but no test in this file asserts it is propagated through the MCP tool output for the manage flow (only the schema-text check at line 318 coverstransition_requirement). Given the whole point of the PR is the rotation/contract, a focused assertion that the manage tool output (or its outputSchema) advertisesrevisionTokenwould prevent silent regressions in the contract surfaced to MCP clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-http.test.ts` around lines 108 - 124, Add an assertion in the manage-flow test that the rotated revisionToken from the manageRequirement mock is propagated in the MCP response/outputSchema: when using the mocked manageRequirement (the vi.fn() mock with detail.versions[0].revisionToken), assert that the API response for requirements_manage_requirement (or the test helper that validates outputSchema for the manage flow) includes detail.versions[0].revisionToken (or that the returned outputSchema advertises a revisionToken field for the manage operation) so the rotated token is explicitly verified and cannot regress.docs/database-schema.md (1)
198-198: AddUKmarker onrevision_tokenin the erDiagram for consistency.Other single-column unique constraints in this diagram are annotated with
UK(e.g.email UK,prefix UK,unique_id UK). Sincerevision_tokenis covered byuq_requirement_versions_revision_token, it should follow the same convention.📝 Proposed change
- text revision_token "uniqueidentifier" + text revision_token UK "uniqueidentifier"As per coding guidelines, "When any database schema, migration, or seed change is made, update
docs/database-schema.md: Entity-Relationship Diagram (Mermaid erDiagram)…".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/database-schema.md` at line 198, The ER diagram entry for the column revision_token should be marked as unique to match the constraint uq_requirement_versions_revision_token; update the mermaid erDiagram line that currently reads "text revision_token 'uniqueidentifier'" to include the UK marker (e.g., "text revision_token UK 'uniqueidentifier'") so it follows the same convention used for other single-column unique constraints like email UK and prefix UK.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/database-schema.md`:
- Line 198: The ER diagram entry for the column revision_token should be marked
as unique to match the constraint uq_requirement_versions_revision_token; update
the mermaid erDiagram line that currently reads "text revision_token
'uniqueidentifier'" to include the UK marker (e.g., "text revision_token UK
'uniqueidentifier'") so it follows the same convention used for other
single-column unique constraints like email UK and prefix UK.
In `@tests/unit/mcp-http.test.ts`:
- Around line 221-378: The tests repeat createClient → listTools →
tools.find(...) → close boilerplate across the "tool schemas" suite; add a small
async helper (e.g., getTool) that calls createClient(), awaits
client.listTools(), finds the tool by name (using tools.find), and returns {
tool, cleanup } where cleanup closes both client and transport; replace each
test's repeated sequence with a call to getTool('requirements_...') and await
cleanup() at the end to keep behavior identical while removing duplication.
- Around line 108-124: Add an assertion in the manage-flow test that the rotated
revisionToken from the manageRequirement mock is propagated in the MCP
response/outputSchema: when using the mocked manageRequirement (the vi.fn() mock
with detail.versions[0].revisionToken), assert that the API response for
requirements_manage_requirement (or the test helper that validates outputSchema
for the manage flow) includes detail.versions[0].revisionToken (or that the
returned outputSchema advertises a revisionToken field for the manage operation)
so the rotated token is explicitly verified and cannot regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb6d97df-d079-4884-9dfc-d84010c7a26b
📒 Files selected for processing (11)
components/RequirementForm.tsxdocs/database-schema.mddocs/version-lifecycle-dates.mdlib/dal/requirements.tstests/quality/QUALITY.mdtests/quality/functional.test.tstests/unit/mcp-http.test.tstests/unit/requirement-form.test.tsxtests/unit/requirements-dal.test.tstests/unit/requirements-id-route.test.tstypeorm/migrations/0002_requirement_version_revision_token.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
- typeorm/migrations/0002_requirement_version_revision_token.mjs
- tests/unit/requirement-form.test.tsx
- docs/version-lifecycle-dates.md
- tests/unit/requirements-id-route.test.ts
- tests/unit/requirements-dal.test.ts
Co-authored-by: Copilot <copilot@github.com>
…n integration tests
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/mcp-http.test.ts (1)
199-211: Optional: ensure cleanup on listTools failure.If
client.listTools()throws, the helper returns without callingclient.close()/transport.close(), leaking the in-memory transport for the rest of the test run. Wrapping the call in try/catch (or usingafterEachregistration) would make the helper resilient if a future tool registration regression is ever introduced.♻️ Proposed fix
async function getTool(name: string) { const { client, transport } = await createClient() - const tools = await client.listTools() - const tool = tools.tools.find(tool => tool.name === name) - - return { - cleanup: async () => { - await client.close() - await transport.close() - }, - tool, + const cleanup = async () => { + await client.close() + await transport.close() + } + try { + const tools = await client.listTools() + const tool = tools.tools.find(tool => tool.name === name) + return { cleanup, tool } + } catch (error) { + await cleanup() + throw error } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-http.test.ts` around lines 199 - 211, The helper getTool should guarantee cleanup of the in-memory client/transport if client.listTools() throws; wrap the listTools call in a try/finally (or try/catch that rethrows) so that client.close() and transport.close() are always awaited in the finally block; update the getTool function to acquire client/transport, perform listTools inside try and return tool in the try, but always run the cleanup calls in finally to avoid leaking the transport on failures..github/workflows/quality-checks.yml (1)
30-31: Pin the dotenv-linter installer in CI and consider caching the binary.Same supply-chain/reproducibility concern as the Dockerfile: this curls
install.shfrommasterintosudo shon every PR/push run, so a change in the upstream installer or its targeted release flips CI behavior without a code change in this repo. Pinning by tag (and optionally caching the resulting binary keyed on that tag) makes the gate deterministic and faster.♻️ Pin install.sh by release tag
- - name: Install dotenv-linter - run: curl -sSfL https://raw.githubusercontent.com/dotenv-linter/dotenv-linter/master/install.sh | sudo sh -s -- -b /usr/local/bin + - name: Install dotenv-linter + env: + DOTENV_LINTER_VERSION: v3.3.0 + run: | + curl -sSfL "https://raw.githubusercontent.com/dotenv-linter/dotenv-linter/${DOTENV_LINTER_VERSION}/install.sh" \ + | sudo sh -s -- -b /usr/local/bin "${DOTENV_LINTER_VERSION}" + dotenv-linter --versionAlso worth considering: keep the version aligned with the devcontainer Dockerfile so local and CI runs produce identical results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/quality-checks.yml around lines 30 - 31, The CI step "Install dotenv-linter" currently curls install.sh from master; change it to fetch a pinned release (replace the installer URL to reference a specific tag/release) so the workflow is deterministic, and update the step to verify the release (e.g., checksum or GPG) if available; optionally add an actions/cache entry keyed by the pinned tag and runner OS to cache the downloaded dotenv-linter binary so future runs restore the cached binary instead of re-downloading; ensure the pinned tag matches the version used in the devcontainer to keep local and CI versions aligned..devcontainer/Dockerfile (1)
24-26: Pin the dotenv-linter installer (supply‑chain hardening, reproducible builds).The installer is fetched from
masterand piped intoshwith no version pin and no checksum verification, so any future change toinstall.shor its referenced release will silently land in the devcontainer image (and in CI on every run). For a tool wired intonpm run check, a sudden CLI/flag change can also break the gate.Recommend pinning to a tagged release (and ideally verifying the resulting binary), e.g.:
♻️ Pin to a specific dotenv-linter release
-# Install dotenv-linter for env file checks. -RUN curl -sSfL https://raw.githubusercontent.com/dotenv-linter/dotenv-linter/master/install.sh | \ - sh -s -- -b /usr/local/bin +# Install dotenv-linter for env file checks (pinned for reproducibility). +ARG DOTENV_LINTER_VERSION=v3.3.0 +RUN curl -sSfL "https://raw.githubusercontent.com/dotenv-linter/dotenv-linter/${DOTENV_LINTER_VERSION}/install.sh" | \ + sh -s -- -b /usr/local/bin "${DOTENV_LINTER_VERSION}" && \ + dotenv-linter --version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/Dockerfile around lines 24 - 26, The current RUN step piping install.sh from master is unpinned and unsafe; change the install to fetch a specific dotenv-linter release and verify it before installing: replace the curl of https://.../install.sh with a download of the installer or release asset for a specific tag (e.g., a GitHub release tag), validate the checksum/signature for that release, and then install the binary into /usr/local/bin; update the RUN that references install.sh so it uses the tagged URL and checksum verification logic to ensure reproducible, supply‑chain hardened builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.devcontainer/Dockerfile:
- Around line 24-26: The current RUN step piping install.sh from master is
unpinned and unsafe; change the install to fetch a specific dotenv-linter
release and verify it before installing: replace the curl of
https://.../install.sh with a download of the installer or release asset for a
specific tag (e.g., a GitHub release tag), validate the checksum/signature for
that release, and then install the binary into /usr/local/bin; update the RUN
that references install.sh so it uses the tagged URL and checksum verification
logic to ensure reproducible, supply‑chain hardened builds.
In @.github/workflows/quality-checks.yml:
- Around line 30-31: The CI step "Install dotenv-linter" currently curls
install.sh from master; change it to fetch a pinned release (replace the
installer URL to reference a specific tag/release) so the workflow is
deterministic, and update the step to verify the release (e.g., checksum or GPG)
if available; optionally add an actions/cache entry keyed by the pinned tag and
runner OS to cache the downloaded dotenv-linter binary so future runs restore
the cached binary instead of re-downloading; ensure the pinned tag matches the
version used in the devcontainer to keep local and CI versions aligned.
In `@tests/unit/mcp-http.test.ts`:
- Around line 199-211: The helper getTool should guarantee cleanup of the
in-memory client/transport if client.listTools() throws; wrap the listTools call
in a try/finally (or try/catch that rethrows) so that client.close() and
transport.close() are always awaited in the finally block; update the getTool
function to acquire client/transport, perform listTools inside try and return
tool in the try, but always run the cleanup calls in finally to avoid leaking
the transport on failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7452e8ee-b337-4595-8b5f-cd8c0484d906
📒 Files selected for processing (13)
.devcontainer/.env.example.devcontainer/Dockerfile.env.development.env.example.env.sqlserver.ci.env.sqlserver.example.github/copilot-instructions.md.github/workflows/integration-tests.yml.github/workflows/quality-checks.ymlCONTRIBUTING.mddocs/database-schema.mdpackage.jsontests/unit/mcp-http.test.ts
💤 Files with no reviewable changes (1)
- .github/workflows/integration-tests.yml
✅ Files skipped from review due to trivial changes (6)
- .github/copilot-instructions.md
- .env.sqlserver.example
- .devcontainer/.env.example
- CONTRIBUTING.md
- .env.development
- .env.example
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.sqlserver.ci
- docs/database-schema.md
…ags and filtering
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/quality-checks.yml (1)
49-69: Consider running the npm script directly to keep CI and local in lockstep.The PR-failure comment tells contributors to reproduce by running
npm run dotenv:check(line 111), but CI shells out to thedotenv-linter/action-dotenv-linter@v3action which ships its owndotenv-linterbinary independent of the version pulled in bynpm ci. That can cause "passes locally, fails in CI" (or vice versa) when the two versions drift, and it forces every flag/exclude change to be made in two places (here andpackage.json).Since the npm script already encodes the exact same flags and exclusions, calling it directly removes the duplication and the version-skew risk.
♻️ Proposed simplification
- - name: Run dotenv-linter root check - uses: dotenv-linter/action-dotenv-linter@v3 - with: - dotenv_linter_flags: >- - --exclude .env.sqlserver - --exclude .env.local - --exclude .env.*.local - filter_mode: nofilter - fail_level: any - - - name: Run dotenv-linter devcontainer check - uses: dotenv-linter/action-dotenv-linter@v3 - with: - dotenv_linter_flags: >- - .devcontainer - --recursive - --exclude . - --exclude .devcontainer/.env - --exclude .devcontainer/elevated/.env - filter_mode: nofilter - fail_level: any + - name: Run dotenv-linter check + run: npm run dotenv:checkIf keeping the action is intentional (e.g., for inline PR annotations via
filter_mode/fail_level), feel free to dismiss — but please confirmdotenv-linteris also installed/pinned consistently with whatnpm run dotenv:checkexpects so contributors aren't chasing version drift.Also applies to: 111-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/quality-checks.yml around lines 49 - 69, The workflow uses the dotenv-linter GitHub Action steps named "Run dotenv-linter root check" and "Run dotenv-linter devcontainer check" which call the action's bundled binary causing potential version drift vs the repo npm script; replace those steps to run the repository npm scripts (e.g., run npm ci then npm run dotenv:check for the root check, and the equivalent npm script for the devcontainer check such as npm run dotenv:check:devcontainer or npm run dotenv:check -- <devcontainer args>) so CI executes the same pinned version and flags as the local npm script; ensure Node is setup (actions/setup-node) and workspace is installed before running the npm scripts.tests/unit/mcp-http.test.ts (2)
199-208: Optional: cache the tools list across schema sub-tests.
getToolopens/closes a fresh MCP client + transport and re-callslistTools()for every assertion (7 times across the newtool schemasdescribe). It works, but you can cut it down to a single client per describe with abeforeAll/afterAlland a memoizedtools.toolsarray. Given Chill review, just flagging as optional cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-http.test.ts` around lines 199 - 208, Refactor to avoid recreating the MCP client for every assertion by moving client/transport setup into a describe-level beforeAll and teardown into afterAll: call createClient() once, call client.listTools() once and store the resulting tools array in a memoized variable, then update getTool(name) to return tools.find(t => t.name === name) using that cached array; ensure afterAll awaits client.close() and transport.close(); reference createClient, client.listTools, getTool, client.close, and transport.close.
233-248: Inconsistent setup and missing finally cleanup in this sub-test.The other six sub-tests under
tool schemasgo throughgetTool, but this one inlinescreateClient()and closes the client/transport at the end without atry/finally. If any of theexpect(...)calls fail, the client and HTTP transport leak into the next test, which can cause cascading failures or hangs. Either route this throughgetTool(returning the full tool list) or wrap the body intry/finally.♻️ Possible cleanup
- it('lists the core MCP tools', async () => { - const { client, transport } = await createClient() - const tools = await client.listTools() - - expect(tools.tools.map(tool => tool.name)).toEqual( - expect.arrayContaining([ - 'requirements_get_requirement', - 'requirements_manage_requirement', - 'requirements_query_catalog', - 'requirements_transition_requirement', - ]), - ) - - await client.close() - await transport.close() - }) + it('lists the core MCP tools', async () => { + const { client, transport } = await createClient() + try { + const tools = await client.listTools() + expect(tools.tools.map(tool => tool.name)).toEqual( + expect.arrayContaining([ + 'requirements_get_requirement', + 'requirements_manage_requirement', + 'requirements_query_catalog', + 'requirements_transition_requirement', + ]), + ) + } finally { + await client.close() + await transport.close() + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-http.test.ts` around lines 233 - 248, This test inlines createClient() and closes client/transport without a try/finally, causing resource leaks on assertion failure; update the test to either call the existing getTool helper to obtain the client/transport and tool list, or wrap the createClient() usage in a try/finally so client.close() and transport.close() are always called (ensure you reference createClient(), client.close(), transport.close(), or getTool where appropriate and move the await client.close() and await transport.close() into the finally block or replace the body with a call to getTool that returns the full tool list).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/quality-checks.yml:
- Around line 59-69: Remove the stray "--exclude ." token from the
dotenv_linter_flags for the "Run dotenv-linter devcontainer check" step: open
the action block that uses dotenv-linter/action-dotenv-linter@v3 (the step named
"Run dotenv-linter devcontainer check") and edit the dotenv_linter_flags value
to delete the "--exclude ." entry so the flags list matches the package.json
dotenv:check script and only excludes the intended .devcontainer/.env and
.devcontainer/elevated/.env files.
---
Nitpick comments:
In @.github/workflows/quality-checks.yml:
- Around line 49-69: The workflow uses the dotenv-linter GitHub Action steps
named "Run dotenv-linter root check" and "Run dotenv-linter devcontainer check"
which call the action's bundled binary causing potential version drift vs the
repo npm script; replace those steps to run the repository npm scripts (e.g.,
run npm ci then npm run dotenv:check for the root check, and the equivalent npm
script for the devcontainer check such as npm run dotenv:check:devcontainer or
npm run dotenv:check -- <devcontainer args>) so CI executes the same pinned
version and flags as the local npm script; ensure Node is setup
(actions/setup-node) and workspace is installed before running the npm scripts.
In `@tests/unit/mcp-http.test.ts`:
- Around line 199-208: Refactor to avoid recreating the MCP client for every
assertion by moving client/transport setup into a describe-level beforeAll and
teardown into afterAll: call createClient() once, call client.listTools() once
and store the resulting tools array in a memoized variable, then update
getTool(name) to return tools.find(t => t.name === name) using that cached
array; ensure afterAll awaits client.close() and transport.close(); reference
createClient, client.listTools, getTool, client.close, and transport.close.
- Around line 233-248: This test inlines createClient() and closes
client/transport without a try/finally, causing resource leaks on assertion
failure; update the test to either call the existing getTool helper to obtain
the client/transport and tool list, or wrap the createClient() usage in a
try/finally so client.close() and transport.close() are always called (ensure
you reference createClient(), client.close(), transport.close(), or getTool
where appropriate and move the await client.close() and await transport.close()
into the finally block or replace the body with a call to getTool that returns
the full tool list).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af569697-42b0-45cd-a45b-c249f37426f0
📒 Files selected for processing (4)
.github/workflows/quality-checks.ymldocs/arkitekturbeskrivning-kravhantering.mdpackage.jsontests/unit/mcp-http.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/arkitekturbeskrivning-kravhantering.md
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/mcp-http.test.ts (2)
251-253:getToolis synchronous but awaited at every call site.
getToolreturns the array element directly, yet callers useconst queryTool = await getTool('…')(lines 267, 280, 297, 316, 328, 339, 354). It works becauseawaiton a non-Promise is a no-op, but it misleads readers into thinking schema lookup is async. Either drop theawaits or makegetToolasyncfor consistency.♻️ Proposed fix (drop the awaits)
- const queryTool = await getTool('requirements_query_catalog') + const queryTool = getTool('requirements_query_catalog')(Apply the same to the other six call sites.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-http.test.ts` around lines 251 - 253, getTool is synchronous but is awaited at its call sites; remove the unnecessary awaits (e.g., change const queryTool = await getTool('…') to const queryTool = getTool('…')) at all call sites that reference getTool so the code accurately reflects a synchronous schema lookup (do this for each occurrence where getTool is currently awaited).
116-132: Fixture formanageRequirement.detail.versions[0]omitsid.The MCP edit contract instructs clients to read
requirement.versions[0].id(asbaseVersionId) andrequirement.versions[0].revisionTokenfrom outputs of read/write tools (seelib/mcp/server.ts:685-698and the description text asserted at lines 309-312). This mock returns onlyrevisionTokenandversionNumber, so the fixture diverges from the contract being advertised. The output schema isz.record(z.string(), z.unknown())so tests still pass, but a future test that exercises the round-trip (baseVersionIdtaken from a previous edit response) would silently getundefined. Recommend including a representativeidso fixtures stay aligned with the documented client flow.♻️ Proposed fix
detail: { uniqueId: 'INT0001', versions: [ { + id: 10, revisionToken: '22222222-2222-4222-8222-222222222222', versionNumber: 2, }, ], },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mcp-http.test.ts` around lines 116 - 132, The mock for manageRequirement in the test fixture omits the version id expected by the MCP edit contract; update the mocked manageRequirement response so manageRequirement.mockResolvedValue includes an id field on detail.versions[0] (e.g., add detail.versions[0].id) so that code paths reading requirement.versions[0].id (used as baseVersionId) receive a representative value; keep the existing revisionToken and versionNumber fields intact when updating the fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/quality-checks.yml:
- Around line 52-55: The dotenv_linter_flags entry currently uses a glob-like
pattern `--exclude .env.*.local` which dotenv-linter does not support; replace
that pattern by listing each concrete filename to exclude (e.g., add `--exclude
.env.production.local`, `--exclude .env.development.local`, etc.) under the
dotenv_linter_flags block so the linter skips the intended files; verify
repository-specific `.local` filenames (or .gitignore) and add each explicit
`--exclude <filename>` line instead of the glob.
In `@tests/unit/mcp-http.test.ts`:
- Around line 229-249: The inner beforeAll calls createClient() before the outer
beforeEach has seeded the service mock; move the mock seeding into the inner
beforeAll so serviceState.getService.mockReturnValue(createFakeService()) runs
before createClient() (and thus before handleRequirementsMcpRequest/listTools),
ensuring createKravhanteringMcpServer and createClient use the fake service;
keep or remove the outer beforeEach seeding (avoid double-mocking) and ensure
cleanup remains in afterAll.
---
Nitpick comments:
In `@tests/unit/mcp-http.test.ts`:
- Around line 251-253: getTool is synchronous but is awaited at its call sites;
remove the unnecessary awaits (e.g., change const queryTool = await getTool('…')
to const queryTool = getTool('…')) at all call sites that reference getTool so
the code accurately reflects a synchronous schema lookup (do this for each
occurrence where getTool is currently awaited).
- Around line 116-132: The mock for manageRequirement in the test fixture omits
the version id expected by the MCP edit contract; update the mocked
manageRequirement response so manageRequirement.mockResolvedValue includes an id
field on detail.versions[0] (e.g., add detail.versions[0].id) so that code paths
reading requirement.versions[0].id (used as baseVersionId) receive a
representative value; keep the existing revisionToken and versionNumber fields
intact when updating the fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7307f7c-45a3-4de8-b977-567c0a15dd34
📒 Files selected for processing (4)
.github/workflows/quality-checks.ymldocs/arkitekturbeskrivning-kravhantering.mdpackage.jsontests/unit/mcp-http.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/arkitekturbeskrivning-kravhantering.md
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
This was referenced Apr 26, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
lib/dal/requirements.ts,
lib/requirements/service.ts
idonly.A second editor can submit stale data and silently overwrite the first
editor's changes. The request payload has no
edited_at/version preconditionand the API has no
409 Conflictresponse shape for stale edits.the current
edited_atvalue or a new integer row version in edit requests.Apply the precondition in the
UPDATE, return409 Conflictwith the latestsnapshot on mismatch, and teach the UI to show a refresh/compare prompt.
Screenshots (if applicable)
Related Issues
Type of Change
Testing
npm run checkpasses locallyChecklist
Checklist
This change is