Conversation
…onsorship feature
Extracts CLI construction into createProgram() (src/cli/program.ts) so scripts/generate-man.ts can introspect the Command tree without running the CLI, generates man/sorokeep.1, and wires build:man into npm run build. Per TegoLabs#384. Trimmed from the original PR before merging: - A full set of unrelated ".claude/agents/kfc/*", ".claude/settings/ kfc-settings.json", and ".claude/system-prompts/spec-workflow- starter.md" files - generic AI-agent workflow scaffolding with no connection to sorokeep or this issue. - getContractsByTag() and related changes to src/commands/status.ts / src/db/repositories.ts - that's issue TegoLabs#379's scope (--tag filtering), a different, unrelated feature. The PR's branch was also quite stale (predated the quiet-hours, contract_groups, rpc/client.ts any-type, and CI audit-scope fixes already on main), which produced a large raw diff and one real merge conflict in src/index.ts (this PR's createProgram() refactor vs. the already-merged --extension-jitter-ms option). Resolved by moving the jitter option registration into createProgram() so both are preserved.
…s#548) Relocates the misplaced test to tests/alerts/ (outside vitest.config.ts's include glob otherwise) and rewrites it against the current SlackChannel class / sendPagerDutyAlert function API - the old copy called a removed sendSlackAlert function.
…ted) Adds src/alerts/matrix.ts (real Matrix Client-Server API, token auth, room-scoped delivery) and its tests, per TegoLabs#310. The PR never registered the channel in builtins.ts, so `--type matrix` was unreachable from the CLI despite the sender being fully implemented and tested. Completed the registration (targetOption: "channel", since the sender's target is a Matrix room ID) plus the matching builtins.test.ts coverage, following the exact pattern used by every other lazily-imported channel (discord/telegram/opsgenie). Verified `alerts add --type matrix` end-to-end at the CLI.
… fixed + completed) Adds src/alerts/teams.ts (Adaptive Card payload, severity coloring per event type) and its tests, per TegoLabs#311. Two things fixed before merging: - CodeRabbit correctly flagged validateWebhookUrl()'s hostname check: `hostname.includes("webhook.office.com")` accepts any hostname that merely contains that substring, e.g. an attacker-controlled "x.webhook.office.com.evil.com" would pass, silently sending real alert content (contract IDs, TTL data) to an attacker-controlled server. Changed to an exact/suffix match on the real hostname, plus an explicit https-only check. Added regression tests for both. - The PR never registered the channel in builtins.ts, so `--type teams` was unreachable from the CLI. Completed the registration (targetOption: "url", matching webhook/discord's pattern) and the matching builtins.test.ts coverage.
…#585, fixed + completed) Adds src/alerts/email.ts (nodemailer SMTP transport, env/config token resolution, password redaction on error) and its tests, per TegoLabs#312. Fixed before merging: - Bumped nodemailer ^7.0.7 -> ^9.0.3 (major version, verified tsc still compiles clean and all tests pass unchanged): the pinned 7.x range had six high-severity advisories, including SMTP/CRLF command injection and an SSRF via the raw-message option. This is a production dependency, so `npm audit --omit=dev` would have failed on it. - The channel was registered in builtins.ts, but src/commands/alerts.ts still had a special-cased `--type email` branch printing "Email alerting is not yet implemented" *before* the registry was ever consulted - making the new channel completely unreachable from the CLI. Removed the special case. - Updated the one existing test that asserted the old "not implemented" behavior, and added a real success-path test (registers a config with --channel <email>). - Fixed a lint error (preserve-caught-error) the right way: the error handler already redacts the SMTP password from the thrown message, but attaching the raw caught error as `cause` would have smuggled the unredacted password back in via the cause chain. Redact the caught error's own message in place before using it as cause, so the guarantee holds through the whole chain.
…s#597, trimmed to scope) Adds src/alerts/googlechat.ts and registers it in builtins.ts, per TegoLabs#313. Registration and the sender itself were already correct. Trimmed from the original PR before merging: a bundled Grafana/Prometheus observability stack (devops/grafana/*, devops/prometheus/*, docker-compose. observability.yml, docs/observability.md) - unrelated to this issue, shared branch lineage with several other open PRs (TegoLabs#594, TegoLabs#595, TegoLabs#596) that also carry the identical bundle. Left tests/docker/docker-compose.test.ts untouched by reverting to main's version. Updated tests/alerts/builtins.test.ts for the 10th channel (the PR's branch predated matrix/teams/email, so its own copy of this file didn't know about them). Note for a follow-up: src/alerts/discord.ts has the same hostname- validation weakness fixed in TegoLabs#557 (`hostname.includes("discord")`, even looser than Teams's check) - pre-existing, not part of this PR, flagging separately.
…egoLabs#318) PR TegoLabs#568 referenced ChannelDefinition.maxRetries in dispatcher.ts but never added the field to the interface, so the branch failed to compile (TS2339). It also shipped a new dispatcher test that called registerAlertChannel("webhook", { maxRetries: 2 }) — a two-argument overload that doesn't exist on the real one-argument, throw-on-duplicate registerAlertChannel API, so the test could never have run against this codebase. - Add optional maxRetries/retryBackoffMs to ChannelDefinition (registry.ts); dispatcher.ts's lookup already existed from the merge and needed no further changes. - Give telegram a maxRetries: 3 default, matching the issue's own rationale (Telegram's Bot API rate limits are stricter than a generic webhook's) — every other channel keeps the global MAX_RETRY_COUNT default, preserving existing behavior exactly. - Rewrite the broken test to swap in a real ChannelDefinition override via the actual registry API, and restore the registry to normal built-in state afterward so it doesn't leak into other tests. Added a companion test asserting webhook has no override by default. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # package-lock.json
…nnels' into verify-555
…egoLabs#555) Issue TegoLabs#319's scope explicitly excludes the orphaned src/alerts/*.test.ts files ("reconciling those is phase-8's job, not this issue's") — they aren't in vitest.config.ts's include glob (tests/**/*.test.ts), so any tests added there never actually run. Keeping the docs update and the contract suite applied to webhook/slack, whose real test files live under tests/alerts/. Applying the contract suite to pagerduty/discord/ telegram is deferred to their dedicated orphan-reconciliation issues (TegoLabs#354-TegoLabs#356).
…to-preview-payload-without-sending-FIX' of https://github.com/veloura-dev/sorokeep into verify-582
…b/sorokeep into verify-536 # Conflicts: # src/commands/alerts.ts
…yStats (TegoLabs#536) The PR's src/db/repositories.ts shipped literal backslash-escaped backticks (\`...\`) instead of real template literals in the days-filter branch of getChannelDeliveryStats — invalid TypeScript syntax that made the entire file fail to parse (tsc reported ~30 cascading errors past this point, and CI's build-and-test check was correctly failing). Replaced with proper template literals. Also tightened the query params array from any[] to Array<number | string>. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds ChangesContract tag management
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant TagCommand
participant TagRepository
participant Database
CLI->>TagCommand: Run tag add or remove
TagCommand->>TagRepository: Mutate contract tag
TagRepository->>Database: Read and persist tags
Database-->>TagRepository: Return stored tags
TagRepository-->>TagCommand: Return resulting tag list
TagCommand-->>CLI: Print success or failure
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/repositories.ts`:
- Around line 174-190: Update addContractTag and removeContractTag to reject
trimmed tag values containing commas before parsing or persisting them, using
consistent validation and the repository’s existing error behavior. Preserve the
current handling of blank tags, and add coverage verifying comma-containing
inputs are rejected.
- Around line 174-190: Serialize the read-modify-write flow in addContractTag
and its counterpart removeContractTag by using an immediate write transaction
that holds the writer lock through tag parsing and persistence, preserving
existing validation and return behavior. Add a regression test using two
database connections that performs concurrent tag mutations and verifies neither
update is lost.
In `@tests/commands/tag.test.ts`:
- Around line 30-32: Update the afterEach cleanup in the tag tests to close the
per-test mockDb before calling vi.restoreAllMocks(), ensuring every database
created by beforeEach is deterministically released.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 0f9142ac-fad3-4be8-addd-ce0565591a2f
📒 Files selected for processing (6)
man/sorokeep.1src/cli/program.tssrc/commands/tag.tssrc/db/repositories.tstests/commands/tag.test.tstests/db/repositories.test.ts
📜 Review details
🔇 Additional comments (4)
src/db/repositories.ts (1)
145-165: LGTM!src/commands/tag.ts (1)
1-61: LGTM!src/cli/program.ts (1)
20-20: LGTM!Also applies to: 51-51
man/sorokeep.1 (1)
1-1: LGTM!Also applies to: 47-48
| export function addContractTag(db: Database.Database, contractId: string, tag: string): string[] { | ||
| const contract = getContract(db, contractId); | ||
| if (!contract) { | ||
| throw new Error(`Contract ${contractId} is not registered.`); | ||
| } | ||
|
|
||
| const normalizedTag = tag.trim(); | ||
| if (!normalizedTag) { | ||
| return parseTags(contract.tags); | ||
| } | ||
|
|
||
| const tags = parseTags(contract.tags); | ||
| if (!tags.includes(normalizedTag)) { | ||
| tags.push(normalizedTag); | ||
| writeTags(db, contractId, tags); | ||
| } | ||
| return tags; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject commas in a tag value.
Line 180 accepts a,b as one tag and Lines 185-188 persist it as a,b. A later parseTags() call treats that value as two tags. This also makes exact-match filtering and the displayed result inconsistent.
Reject comma-containing values after trimming. Use the same validation in addContractTag and removeContractTag. Add coverage for this input.
Proposed fix
+function normalizeTag(tag: string): string {
+ const normalizedTag = tag.trim();
+ if (normalizedTag.includes(",")) {
+ throw new Error("Tag values cannot contain commas.");
+ }
+ return normalizedTag;
+}
+
export function addContractTag(db: Database.Database, contractId: string, tag: string): string[] {
- const normalizedTag = tag.trim();
+ const normalizedTag = normalizeTag(tag);
// ...
}
export function removeContractTag(db: Database.Database, contractId: string, tag: string): string[] {
- const normalizedTag = tag.trim();
+ const normalizedTag = normalizeTag(tag);
// ...
}Also applies to: 200-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db/repositories.ts` around lines 174 - 190, Update addContractTag and
removeContractTag to reject trimmed tag values containing commas before parsing
or persisting them, using consistent validation and the repository’s existing
error behavior. Preserve the current handling of blank tags, and add coverage
verifying comma-containing inputs are rejected.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate repository file and related database usage"
git ls-files | rg '(^|/)repositories\.ts$|(^|/)db/|database|better-sqlite3|sql'
echo
echo "Show relevant section of src/db/repositories.ts"
sed -n '1,260p' src/db/repositories.ts | nl -ba
echo
echo "Search for tag-related functions and database interfaces"
rg -n "function (addContractTag|removeContractTag|writeTags|parseTags|getContract)|const (addContractTag|removeContractTag|writeTags|parseTags|getContract)|class Database|interface Database|type Database" src/db/repositories.tsRepository: AbdulmalikAlayande/sorokeep
Length of output: 1551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Show relevant section of src/db/repositories.ts"
sed -n '1,260p' src/db/repositories.ts
echo
echo "Search for tag-related functions in src/db/repositories.ts"
rg -n "addContractTag|removeContractTag|writeTags|parseTags|getContract|class Database|interface Database|type Database|transaction|writer" src/db/repositories.ts
echo
echo "Search database transaction/writer APIs"
rg -n "class Database|interface Database|type Database|writer|write|transaction|prepare|exec|get" src/dbRepository: AbdulmalikAlayande/sorokeep
Length of output: 34213
🌐 Web query:
better-sqlite3 transactions lock writer read-write locked concurrency read transaction writer lock documentation
💡 Result:
In better-sqlite3, as with SQLite generally, transaction and concurrency behavior is governed by the underlying SQLite locking model [1][2][3]. Concurrency Basics SQLite supports multiple simultaneous readers but only one writer at a time [1][4][3]. When using the default rollback journal mode, read and write operations can block each other [3]. To improve concurrency, it is standard practice in better-sqlite3 to enable Write-Ahead Logging (WAL) mode by executing db.pragma('journal_mode = WAL') [1][5]. WAL mode allows multiple readers to operate simultaneously with a single writer, as readers access the original database file while the writer appends changes to a separate WAL file [5][3]. Transaction Locks and SQLITE_BUSY The SQLITE_BUSY error occurs when the database cannot acquire the necessary lock due to concurrent activity [1][6]. 1. Deferred Transactions: By default, transactions in SQLite (and better-sqlite3) are 'deferred' [6]. They do not acquire a write lock until a write operation is actually performed [6]. If a transaction begins with a read and later attempts to perform a write, it must 'upgrade' its lock. If another connection has acquired a lock in the interim, this upgrade can cause a deadlock or SQLITE_BUSY error [7][8]. 2. Immediate Transactions: To prevent these upgrade-related deadlocks, you can use transactions that acquire the write lock immediately upon starting [7][8]. In better-sqlite3, transaction functions provide this capability (e.g., using.immediate or.exclusive variants) [9][7]. Best Practices for Concurrency - Enable WAL mode: This is the most effective way to allow concurrent read and write operations [5][4]. - Keep Transactions Short: Because SQLite serializes writes, long-running transactions hold the write lock for extended periods, blocking other writers and potentially leading to performance degradation [9][2][10]. - Use Proper Transaction Types: If a transaction will involve writing, use an 'immediate' transaction to acquire the write lock at the start, which avoids the risks associated with upgrading locks during the transaction [7][8]. - Handle SQLITE_BUSY: Even with optimizations, concurrent access can result in SQLITE_BUSY [1][4]. You should configure the database timeout (e.g., db = new Database('file.db', { timeout: 5000 })) to allow the process to wait for locks to be released before throwing an error [9][4]. It is generally advised not to mix manual transaction management (e.g., executing raw BEGIN/COMMIT commands) with the better-sqlite3.transaction helper, as this can lead to unpredictable behavior [9].
Citations:
- 1: SQLITE_BUSY with concurrent transactions is expected behavior? WiseLibs/better-sqlite3#1198
- 2: https://github.com/WiseLibs/better-sqlite3
- 3: https://www.sqlite.org/isolation.html
- 4: Multiprocess access to database WiseLibs/better-sqlite3#250
- 5: https://github.com/WiseLibs/better-sqlite3/blob/HEAD/docs/performance.md
- 6: https://sqlite.org/lockingv3.html
- 7: https://coldfusion-example.blogspot.com/2026/02/beyond-wal-optimizing-sqlite-for-high.html
- 8: https://github.com/sqlite/sqlite/blob/master/doc/wal-lock.md
- 9: https://github.com/WiseLibs/better-sqlite3/blob/HEAD/docs/api.md
- 10: Make transaction work with async callbacks. WiseLibs/better-sqlite3#1262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect tests for tag mutation coverage and two-connection tests"
sed -n '1,260p' tests/db/repositories.test.ts
echo
rg -n "addContractTag|removeContractTag|tag" tests/db src/db tests/db/repositories.test.ts
echo
echo "Inspect database open/PRAGMA configuration"
sed -n '1,80p' src/db/database.tsRepository: AbdulmalikAlayande/sorokeep
Length of output: 27966
Serialize contract tag mutations.
addContractTag and removeContractTag read the comma-separated tag list, update it in memory, then write back the full list. A concurrent tag update can overwrite the other update. Use an immediate write transaction that holds the writer lock through the read and write, or use a compare-and-swap update with retry. Add a two-connection regression test for concurrent tag changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/db/repositories.ts` around lines 174 - 190, Serialize the
read-modify-write flow in addContractTag and its counterpart removeContractTag
by using an immediate write transaction that holds the writer lock through tag
parsing and persistence, preserving existing validation and return behavior. Add
a regression test using two database connections that performs concurrent tag
mutations and verifies neither update is lost.
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the test database after each test.
beforeEach creates a new native SQLite database for every test. afterEach does not close it. Close mockDb before restoring mocks to release those resources deterministically.
Proposed fix
afterEach(() => {
+ mockDb.close();
vi.restoreAllMocks();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| }); | |
| afterEach(() => { | |
| mockDb.close(); | |
| vi.restoreAllMocks(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/commands/tag.test.ts` around lines 30 - 32, Update the afterEach
cleanup in the tag tests to close the per-test mockDb before calling
vi.restoreAllMocks(), ensuring every database created by beforeEach is
deterministically released.
43ad363 to
8692701
Compare
|
I don't have access to push to this repo, add me as collaborator so I can
push, or whats the issue I jave with this again.
Let me know, I will be happy to help finish it
…On Sat, 1 Aug 2026, 2:30 am coderabbitai[bot], ***@***.***> wrote:
***@***.***[bot]* requested changes on this pull request.
*Actionable comments posted: 3*
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In ***@***.***/db/repositories.ts`:
- Around line 174-190: Update addContractTag and removeContractTag to reject
trimmed tag values containing commas before parsing or persisting them, using
consistent validation and the repository’s existing error behavior. Preserve the
current handling of blank tags, and add coverage verifying comma-containing
inputs are rejected.
- Around line 174-190: Serialize the read-modify-write flow in addContractTag
and its counterpart removeContractTag by using an immediate write transaction
that holds the writer lock through tag parsing and persistence, preserving
existing validation and return behavior. Add a regression test using two
database connections that performs concurrent tag mutations and verifies neither
update is lost.
In ***@***.***/commands/tag.test.ts`:
- Around line 30-32: Update the afterEach cleanup in the tag tests to close the
per-test mockDb before calling vi.restoreAllMocks(), ensuring every database
created by beforeEach is deterministically released.
🪄 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*: ASSERTIVE
*Plan*: Pro Plus
*Run ID*: 0f9142ac-fad3-4be8-addd-ce0565591a2f
📥 Commits
Reviewing files that changed from the base of the PR and between 2914fe5
<2914fe5>
and b0a195b
<b0a195b>
.
📒 Files selected for processing (6)
- man/sorokeep.1
- src/cli/program.ts
- src/commands/tag.ts
- src/db/repositories.ts
- tests/commands/tag.test.ts
- tests/db/repositories.test.ts
📜 Review details 🔇 Additional comments (4)
src/db/repositories.ts (1)
145-165: LGTM!
src/commands/tag.ts (1)
1-61: LGTM!
src/cli/program.ts (1)
20-20: LGTM!
Also applies to: 51-51
man/sorokeep.1 (1)
1-1: LGTM!
Also applies to: 47-48
------------------------------
In src/db/repositories.ts
<#650 (comment)>
:
> +export function addContractTag(db: Database.Database, contractId: string, tag: string): string[] {
+ const contract = getContract(db, contractId);
+ if (!contract) {
+ throw new Error(`Contract ${contractId} is not registered.`);
+ }
+
+ const normalizedTag = tag.trim();
+ if (!normalizedTag) {
+ return parseTags(contract.tags);
+ }
+
+ const tags = parseTags(contract.tags);
+ if (!tags.includes(normalizedTag)) {
+ tags.push(normalizedTag);
+ writeTags(db, contractId, tags);
+ }
+ return tags;
*🗄️ Data Integrity & Integration* | *🟠 Major* | *⚡ Quick win*
*Reject commas in a tag value.*
Line 180 accepts a,b as one tag and Lines 185-188 persist it as a,b. A
later parseTags() call treats that value as two tags. This also makes
exact-match filtering and the displayed result inconsistent.
Reject comma-containing values after trimming. Use the same validation in
addContractTag and removeContractTag. Add coverage for this input.
Proposed fix
+function normalizeTag(tag: string): string {+ const normalizedTag = tag.trim();+ if (normalizedTag.includes(",")) {+ throw new Error("Tag values cannot contain commas.");+ }+ return normalizedTag;+}+
export function addContractTag(db: Database.Database, contractId: string, tag: string): string[] {- const normalizedTag = tag.trim();+ const normalizedTag = normalizeTag(tag);
// ...
}
export function removeContractTag(db: Database.Database, contractId: string, tag: string): string[] {- const normalizedTag = tag.trim();+ const normalizedTag = normalizeTag(tag);
// ...
}
Also applies to: 200-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In ***@***.***/db/repositories.ts` around lines 174 - 190, Update addContractTag and
removeContractTag to reject trimmed tag values containing commas before parsing
or persisting them, using consistent validation and the repository’s existing
error behavior. Preserve the current handling of blank tags, and add coverage
verifying comma-containing inputs are rejected.
------------------------------
*🗄️ Data Integrity & Integration* | *🟠 Major* | *⚡ Quick win*
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo "Locate repository file and related database usage"
git ls-files | rg '(^|/)repositories\.ts$|(^|/)db/|database|better-sqlite3|sql'
echoecho "Show relevant section of src/db/repositories.ts"
sed -n '1,260p' src/db/repositories.ts | nl -ba
echoecho "Search for tag-related functions and database interfaces"
rg -n "function (addContractTag|removeContractTag|writeTags|parseTags|getContract)|const (addContractTag|removeContractTag|writeTags|parseTags|getContract)|class Database|interface Database|type Database" src/db/repositories.ts
Repository: AbdulmalikAlayande/sorokeep
Length of output: 1551
------------------------------
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo "Show relevant section of src/db/repositories.ts"
sed -n '1,260p' src/db/repositories.ts
echoecho "Search for tag-related functions in src/db/repositories.ts"
rg -n "addContractTag|removeContractTag|writeTags|parseTags|getContract|class Database|interface Database|type Database|transaction|writer" src/db/repositories.ts
echoecho "Search database transaction/writer APIs"
rg -n "class Database|interface Database|type Database|writer|write|transaction|prepare|exec|get" src/db
Repository: AbdulmalikAlayande/sorokeep
Length of output: 34213
------------------------------
🌐 Web query:
better-sqlite3 transactions lock writer read-write locked concurrency read
transaction writer lock documentation
💡 Result:
In better-sqlite3, as with SQLite generally, transaction and concurrency
behavior is governed by the underlying SQLite locking model [1][2][3].
Concurrency Basics SQLite supports multiple simultaneous readers but only
one writer at a time [1][4][3]. When using the default rollback journal
mode, read and write operations can block each other [3]. To improve
concurrency, it is standard practice in better-sqlite3 to enable
Write-Ahead Logging (WAL) mode by executing db.pragma('journal_mode = WAL')
[1][5]. WAL mode allows multiple readers to operate simultaneously with a
single writer, as readers access the original database file while the
writer appends changes to a separate WAL file [5][3]. Transaction Locks and
SQLITE_BUSY The SQLITE_BUSY error occurs when the database cannot acquire
the necessary lock due to concurrent activity [1][6]. 1. Deferred
Transactions: By default, transactions in SQLite (and better-sqlite3) are
'deferred' [6]. They do not acquire a write lock until a write operation is
actually performed [6]. If a transaction begins with a read and later
attempts to perform a write, it must 'upgrade' its lock. If another
connection has acquired a lock in the interim, this upgrade can cause a
deadlock or SQLITE_BUSY error [7][8]. 2. Immediate Transactions: To prevent
these upgrade-related deadlocks, you can use transactions that acquire the
write lock immediately upon starting [7][8]. In better-sqlite3, transaction
functions provide this capability (e.g., using.immediate or.exclusive
variants) [9][7]. Best Practices for Concurrency - Enable WAL mode: This is
the most effective way to allow concurrent read and write operations
[5][4]. - Keep Transactions Short: Because SQLite serializes writes,
long-running transactions hold the write lock for extended periods,
blocking other writers and potentially leading to performance degradation
[9][2][10]. - Use Proper Transaction Types: If a transaction will involve
writing, use an 'immediate' transaction to acquire the write lock at the
start, which avoids the risks associated with upgrading locks during the
transaction [7][8]. - Handle SQLITE_BUSY: Even with optimizations,
concurrent access can result in SQLITE_BUSY [1][4]. You should configure
the database timeout (e.g., db = new Database('file.db', { timeout: 5000
})) to allow the process to wait for locks to be released before throwing
an error [9][4]. It is generally advised not to mix manual transaction
management (e.g., executing raw BEGIN/COMMIT commands) with the
better-sqlite3.transaction helper, as this can lead to unpredictable
behavior [9].
Citations:
- 1: WiseLibs/better-sqlite3#1198
- 2: https://github.com/WiseLibs/better-sqlite3
- 3: https://www.sqlite.org/isolation.html
- 4: WiseLibs/better-sqlite3#250
<WiseLibs/better-sqlite3#250>
- 5:
https://github.com/WiseLibs/better-sqlite3/blob/HEAD/docs/performance.md
- 6: https://sqlite.org/lockingv3.html
- 7:
https://coldfusion-example.blogspot.com/2026/02/beyond-wal-optimizing-sqlite-for-high.html
- 8: https://github.com/sqlite/sqlite/blob/master/doc/wal-lock.md
- 9: https://github.com/WiseLibs/better-sqlite3/blob/HEAD/docs/api.md
- 10: WiseLibs/better-sqlite3#1262
<WiseLibs/better-sqlite3#1262>
------------------------------
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo "Inspect tests for tag mutation coverage and two-connection tests"
sed -n '1,260p' tests/db/repositories.test.tsecho
rg -n "addContractTag|removeContractTag|tag" tests/db src/db tests/db/repositories.test.ts
echoecho "Inspect database open/PRAGMA configuration"
sed -n '1,80p' src/db/database.ts
Repository: AbdulmalikAlayande/sorokeep
Length of output: 27966
------------------------------
*Serialize contract tag mutations.*
addContractTag and removeContractTag read the comma-separated tag list,
update it in memory, then write back the full list. A concurrent tag update
can overwrite the other update. Use an immediate write transaction that
holds the writer lock through the read and write, or use a compare-and-swap
update with retry. Add a two-connection regression test for concurrent tag
changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In ***@***.***/db/repositories.ts` around lines 174 - 190, Serialize the
read-modify-write flow in addContractTag and its counterpart removeContractTag
by using an immediate write transaction that holds the writer lock through tag
parsing and persistence, preserving existing validation and return behavior. Add
a regression test using two database connections that performs concurrent tag
mutations and verifies neither update is lost.
------------------------------
In tests/commands/tag.test.ts
<#650 (comment)>
:
> + afterEach(() => {
+ vi.restoreAllMocks();
+ });
*🩺 Stability & Availability* | *🟡 Minor* | *⚡ Quick win*
*Close the test database after each test.*
beforeEach creates a new native SQLite database for every test. afterEach
does not close it. Close mockDb before restoring mocks to release those
resources deterministically.
Proposed fix
afterEach(() => {+ mockDb.close();
vi.restoreAllMocks();
});
📝 Committable suggestion
|
…, PR #650) Adds addContractTag()/removeContractTag() to src/db/repositories.ts, reusing the existing comma-separated tags column format (trimmed, exact-match values; empty set stored as NULL). Adding a tag that already exists is a no-op; removing one that isn't present is a no-op; removing the last tag resets the column to NULL. Adds src/commands/tag.ts: `sorokeep tag add <contract-id> <tag>` and `sorokeep tag remove <contract-id> <tag>`, registered in src/cli/program.ts. Reimplemented from PR #650 (muffti123) rather than merged directly: the fork was 100 commits / 1137 files behind current main. The actual new commit was clean, well-tested, and self-contained (repository functions, command file, and both test files touched nothing else) — reapplied nearly verbatim by hand, tightening two `catch (error: any)` blocks to `unknown` to match this session's earlier no-explicit-any cleanup. Note: a first attempt at this used `git diff main <fork-tip>` to patch src/db/repositories.ts, which incorrectly reverted unrelated content that had landed on main since the fork's stale base (audit-log/monitor additions) — caught via a failing tsc run before it was ever committed or pushed, and redone by hand instead. Verified with 19 new tests (11 repository, 8 command), full suite (114 files / 1471 tests), tsc --noEmit, lint, build, npm audit, and a manual smoke test confirming the "contract not registered" error path end-to-end via the built CLI.
|
Thanks for the detailed PR body and thorough test coverage — I implemented this directly on main (commit 7942674) rather than merging the PR since the branch was 100 commits / 1137 files behind current main. The design (addContractTag/removeContractTag, comma-separated tags format, no-op duplicate-add and missing-remove semantics, NULL on last-tag removal) was clean and reused nearly verbatim. One small change: tightened two Closing this PR since the work is captured on main now — appreciate the care taken here. |
Summary
Adds a
sorokeep tagcommand so tags can be added/removed on an already-watched contract without re-runningwatch(which would otherwise risk resetting other fields likepoll_interval_seconds).Each invocation mutates the contract's tag list and prints the resulting full tag list for confirmation.
Changes
src/db/repositories.tsaddContractTag(db, contractId, tag)andremoveContractTag(db, contractId, tag).tagsvalue, mutate the set, and write it back, reusing the serialization format settled by the companion--tagfilter issue: a comma-separated string with trimmed, exact-match values (an empty set is stored asNULL).Contract <id> is not registered.error for unregistered contracts.src/commands/tag.ts(new)sorokeep tag add <contract-id> <tag>andsorokeep tag remove <contract-id> <tag>subcommands, printed with commander-style confirmation output (Successfully added/removed tag "…" …, followed by the resultingTags:list, or(none)).src/cli/program.tstagcommand (registration lives here —src/index.tsdelegates tocreateProgram()).man/sorokeep.1tagcommand is documented.tests/db/repositories.test.ts— newContract Tagssuite covering add/remove, the duplicate-add no-op, the missing-remove no-op, whitespace trimming, clearing the last tag toNULL, unregistered-contract errors, and the exact-match filter format.tests/commands/tag.test.ts(new) — command-level coverage for both subcommands, including resulting-tag-list output and error handling for unregistered contracts.Edge cases handled
NULL, keeping parity with the--tagfilter's handling of empty tag sets.Testing
Also smoke-tested end-to-end against a real in-memory DB (add, duplicate add, multi-tag, remove, missing remove, remove-last →
NULL, unregistered error).Notes
db/schema.sqluntouched) — tags continue to live in the existingcontracts.tagscolumn.--tagfilter commit (f6fb27e) exists in history butgetContractsByTagis not currently present onmain; interop is guaranteed by the shared comma-separated serialization rather than a shared helper.Closes #380