Docs: add CONTRIBUTING.md guide, ADRs, E2E sandbox, and onboarding tests - #222
Conversation
|
@dunnidev Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesContributor Onboarding Documentation and Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/adr/ADR-006-in-memory-sqlite-for-testing.md`:
- Line 33: Correct the ADR so it matches the actual behavior in
src/db/database.ts: remove the claim that getDatabaseForTesting enables WAL
mode, since the implementation only enables foreign keys and initializes the
schema, and fix the statement about export status because getDatabaseForTesting
is explicitly exported. Update the description in the ADR section that
references getDatabaseForTesting to reflect the real implementation and avoid
contradictory claims.
In `@docs/e2e-sandbox.md`:
- Line 21: The e2e sandbox is pinning two different
stellar/quickstart:soroban-dev image digests in the same workflow, which should
be standardized. Update the references in docs/e2e-sandbox.md so the same pinned
digest is used consistently at both locations, using the existing quickstart
image constant/reference to keep local and scripted setup reproducible.
In `@tests/docs/onboarding.test.ts`:
- Around line 167-173: The ADR completeness test in the onboarding suite is too
permissive because it accepts files with either “### Consequences” or “##
Validation” instead of requiring both. Update the assertion in the test that
iterates over adrFiles so it checks for both sections in each ADR, using the
existing readFile, hasConsequences, and hasValidation logic in the test block.
Keep the test name aligned with the stricter contract and make the expectation
fail unless both required sections are present.
- Around line 285-288: The README-to-CONTRIBUTING test in onboarding.test.ts
only checks for the raw string and can pass even if it is not a real link.
Update the existing “README.md link to CONTRIBUTING.md is valid” test to parse
the README content and assert that the CONTRIBUTING reference is an actual
resolvable Markdown link target, using the existing readFile and path/project
root setup in the test suite. Focus the fix around the README.md assertion logic
so it verifies the link destination rather than simple text presence.
🪄 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: 8d7d3221-998b-4521-9afa-1050def9eb16
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CONTRIBUTING.mddocs/adr/ADR-001-use-sqlite-for-local-storage.mddocs/adr/ADR-002-use-esm-modules.mddocs/adr/ADR-003-use-commander-js-for-cli.mddocs/adr/ADR-004-polling-daemon-architecture.mddocs/adr/ADR-005-use-typescript-over-rust.mddocs/adr/ADR-006-in-memory-sqlite-for-testing.mddocs/e2e-sandbox.mdstdouttests/docs/onboarding.test.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.0)
tests/docs/onboarding.test.ts
[warning] 12-12: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(p, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🪛 LanguageTool
docs/adr/ADR-006-in-memory-sqlite-for-testing.md
[style] ~58-~58: Specify a number, remove phrase, use “a few”, or use “some”
Context: ...le cleanup, etc.). These are covered by a small number of integration tests in `tests/utils/confi...
(SMALL_NUMBER_OF)
CONTRIBUTING.md
[style] ~35-~35: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...first. If there's no issue for what you want to do, open one and describe the change be...
(REP_WANT_TO_VB)
🪛 markdownlint-cli2 (0.22.1)
CONTRIBUTING.md
[warning] 70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OpenGrep (1.23.0)
tests/docs/onboarding.test.ts
[ERROR] 296-296: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 302-302: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (5)
stdout (1)
53-78: LGTM!docs/adr/ADR-001-use-sqlite-for-local-storage.md (1)
1-53: LGTM!docs/adr/ADR-002-use-esm-modules.md (1)
1-50: LGTM!docs/adr/ADR-003-use-commander-js-for-cli.md (1)
1-51: LGTM!CONTRIBUTING.md (1)
5-296: LGTM!
|
|
||
| **Chosen option: In-memory SQLite via `getDatabaseForTesting()`** | ||
|
|
||
| `getDatabaseForTesting()` opens a `better-sqlite3` connection to `:memory:`, initializes the schema, enables WAL mode, and returns the database handle. Each call produces an independent, empty database. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix implementation claims that contradict src/db/database.ts.
Line 33 states WAL mode is enabled, but src/db/database.ts:62-67 only enables foreign keys and initializes schema.
Line 65 states getDatabaseForTesting is not exported from src/db/database.ts, but the function is explicitly exported there.
Also applies to: 65-65
🤖 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 `@docs/adr/ADR-006-in-memory-sqlite-for-testing.md` at line 33, Correct the ADR
so it matches the actual behavior in src/db/database.ts: remove the claim that
getDatabaseForTesting enables WAL mode, since the implementation only enables
foreign keys and initializes the schema, and fix the statement about export
status because getDatabaseForTesting is explicitly exported. Update the
description in the ADR section that references getDatabaseForTesting to reflect
the real implementation and avoid contradictory claims.
| docker run --rm -it \ | ||
| -p 8000:8000 \ | ||
| --name soroban-local \ | ||
| stellar/quickstart:soroban-dev@sha256:9f5c75ce2e920a9b9a6e7e8d0b3a2c8f6e9b7c5d4a3b2c1d0e9f8a7b6c5d4e3f \ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one consistent pinned Quickstart image digest.
Line 21 and Line 147 pin different stellar/quickstart:soroban-dev@sha256:... digests for the same workflow. Standardize to a single digest to keep local and scripted setups reproducible.
Also applies to: 147-147
🤖 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 `@docs/e2e-sandbox.md` at line 21, The e2e sandbox is pinning two different
stellar/quickstart:soroban-dev image digests in the same workflow, which should
be standardized. Update the references in docs/e2e-sandbox.md so the same pinned
digest is used consistently at both locations, using the existing quickstart
image constant/reference to keep local and scripted setup reproducible.
| it("all ADRs have a consequences section or validation section", () => { | ||
| for (const file of adrFiles) { | ||
| const content = readFile(path.join(ADR_DIR, file)); | ||
| const hasConsequences = content.includes("### Consequences"); | ||
| const hasValidation = content.includes("## Validation"); | ||
| expect(hasConsequences || hasValidation).toBe(true); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Strengthen ADR completeness assertion to require both sections.
Line 167 currently allows ADRs missing either ### Consequences or ## Validation, which weakens the stated completeness contract.
Suggested fix
- it("all ADRs have a consequences section or validation section", () => {
+ it("all ADRs have both consequences and validation sections", () => {
for (const file of adrFiles) {
const content = readFile(path.join(ADR_DIR, file));
const hasConsequences = content.includes("### Consequences");
const hasValidation = content.includes("## Validation");
- expect(hasConsequences || hasValidation).toBe(true);
+ expect(hasConsequences).toBe(true);
+ expect(hasValidation).toBe(true);
}
});📝 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.
| it("all ADRs have a consequences section or validation section", () => { | |
| for (const file of adrFiles) { | |
| const content = readFile(path.join(ADR_DIR, file)); | |
| const hasConsequences = content.includes("### Consequences"); | |
| const hasValidation = content.includes("## Validation"); | |
| expect(hasConsequences || hasValidation).toBe(true); | |
| } | |
| it("all ADRs have both consequences and validation sections", () => { | |
| for (const file of adrFiles) { | |
| const content = readFile(path.join(ADR_DIR, file)); | |
| const hasConsequences = content.includes("### Consequences"); | |
| const hasValidation = content.includes("## Validation"); | |
| expect(hasConsequences).toBe(true); | |
| expect(hasValidation).toBe(true); | |
| } | |
| }); |
🤖 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/docs/onboarding.test.ts` around lines 167 - 173, The ADR completeness
test in the onboarding suite is too permissive because it accepts files with
either “### Consequences” or “## Validation” instead of requiring both. Update
the assertion in the test that iterates over adrFiles so it checks for both
sections in each ADR, using the existing readFile, hasConsequences, and
hasValidation logic in the test block. Keep the test name aligned with the
stricter contract and make the expectation fail unless both required sections
are present.
| it("README.md link to CONTRIBUTING.md is valid", () => { | ||
| const readme = readFile(path.join(PROJECT_ROOT, "README.md")); | ||
| expect(readme).toContain("CONTRIBUTING.md"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the README→CONTRIBUTING reference as an actual resolvable link.
Line 285 only checks raw text presence, so a broken/non-link mention still passes.
Suggested fix
it("README.md link to CONTRIBUTING.md is valid", () => {
const readme = readFile(path.join(PROJECT_ROOT, "README.md"));
- expect(readme).toContain("CONTRIBUTING.md");
+ const match = readme.match(/\[[^\]]+\]\(([^)]*CONTRIBUTING\.md)\)/i);
+ expect(match).not.toBeNull();
+ if (match) {
+ expect(fileExists(path.join(PROJECT_ROOT, match[1]))).toBe(true);
+ }
});🤖 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/docs/onboarding.test.ts` around lines 285 - 288, The
README-to-CONTRIBUTING test in onboarding.test.ts only checks for the raw string
and can pass even if it is not a real link. Update the existing “README.md link
to CONTRIBUTING.md is valid” test to parse the README content and assert that
the CONTRIBUTING reference is an actual resolvable Markdown link target, using
the existing readFile and path/project root setup in the test suite. Focus the
fix around the README.md assertion logic so it verifies the link destination
rather than simple text presence.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | 7efd589 | tests/commands/channels.test.ts | View secret |
| - | - | Generic High Entropy Secret | 617e5cd | tests/rpc/client.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
2 participants
Closes #207