Skip to content

Commit efdf265

Browse files
dbejarano820dbejarano820
committed
docs: add code review findings and publish readiness checklist
1 parent d0246f0 commit efdf265

1 file changed

Lines changed: 107 additions & 0 deletions

File tree

‎CODE_REVIEW.md‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
# Code Review — hacienda-cr
2+
3+
**Date:** 2025-02-22
4+
**Reviewer:** Dojito (automated multi-agent review)
5+
**Repo:** DojoCodingLabs/hacienda-cr
6+
**Scope:** Full codebase (sdk, cli, mcp, shared)
7+
8+
---
9+
10+
## Summary
11+
12+
The codebase is **well-structured and high quality**. Clean architecture, comprehensive test coverage (623 tests passing), solid TypeScript typing, and good separation of concerns. The review found a mix of issues — mostly medium-severity items blocking npm publish readiness.
13+
14+
---
15+
16+
## High-Signal Issues Found
17+
18+
### #1 — Missing LICENSE file (now fixed)
19+
- **Severity:** Critical (blocks npm publish)
20+
- **Status:** ✅ Fixed in this PR
21+
- **Description:** README references MIT license but no LICENSE file existed
22+
23+
### #2 — npm org `@hacienda-cr` does not exist
24+
- **Severity:** Critical (blocks npm publish)
25+
- **File:** All package.json files
26+
- **Description:** The `@hacienda-cr` npm scope returns 404. Must create the org on npmjs.com before publishing.
27+
- **Action:** Run `npm login` then create the org at https://www.npmjs.com/org/create
28+
29+
### #3 — No NPM_TOKEN GitHub secret configured
30+
- **Severity:** Critical (blocks automated publish)
31+
- **File:** `.github/workflows/publish.yml`
32+
- **Description:** The publish workflow references `secrets.NPM_TOKEN` but no secret is set in the repo.
33+
- **Action:** Generate npm token and add as repo secret
34+
35+
### #4 — Sequence store has race condition under concurrent access
36+
- **Severity:** Medium
37+
- **File:** `packages/sdk/src/config/sequence-store.ts`
38+
- **Description:** `getNextSequence()` does read-increment-write non-atomically. Two concurrent calls can read the same value, both increment to N+1, and write the same number — producing duplicate sequence numbers. The atomic file rename helps with corruption but not with TOCTOU races.
39+
- **Recommendation:** Use file locking (e.g., `proper-lockfile`) or document that concurrent access requires external synchronization.
40+
41+
### #5 — MCP create_invoice tool always uses sequence=1
42+
- **Severity:** Medium
43+
- **File:** `packages/mcp/src/tools/create-invoice.ts` (lines ~130-140)
44+
- **Description:** The `create_invoice` tool hardcodes `sequence: 1` and `numeroConsecutivo: "001000010100000000001"` for every invoice. This means every invoice generated through MCP will have the same clave/consecutivo, causing 409 Conflict on submission.
45+
- **Evidence:**
46+
```ts
47+
const clave = buildClave({
48+
...
49+
sequence: 1, // Always 1!
50+
...
51+
});
52+
const seq = "0000000001"; // Always 1!
53+
```
54+
- **Recommendation:** Integrate with `SequenceStore.getNextSequence()` to auto-increment.
55+
56+
### #6 — `workspace:*` dependencies need resolution for npm publish
57+
- **Severity:** Medium (blocks correct installs from npm)
58+
- **File:** All package.json files
59+
- **Description:** Internal deps use `"workspace:*"` which pnpm/changesets should resolve to real versions at publish time. However, changesets must be configured correctly — verify with `pnpm publish --dry-run`.
60+
- **Status:** Changesets config looks correct (`"access": "public"`), should resolve automatically.
61+
62+
### #7 — CLI auth login doesn't persist password (by design) but no guidance on token persistence
63+
- **Severity:** Low
64+
- **File:** `packages/cli/src/commands/auth/login.ts`
65+
- **Description:** After `hacienda auth login`, the profile is saved but the password is not (correctly, for security). However, subsequent CLI commands need to re-authenticate, and there's no session token cache. Each command invocation requires `HACIENDA_PASSWORD` env var.
66+
- **Recommendation:** Document this clearly in CLI README or add token caching.
67+
68+
### #8 — `round5()` uses standard `Math.round` which has banker's rounding edge cases
69+
- **Severity:** Low
70+
- **File:** `packages/sdk/src/tax/calculator.ts`
71+
- **Description:** `Math.round(value * factor) / factor` can produce unexpected results for values exactly at .5 boundaries due to IEEE 754 floating-point representation. For example, `round5(1.000005)` could round incorrectly. However, since Hacienda uses 5 decimal places, hitting exact .5 boundaries at the 6th decimal is extremely rare in practice.
72+
- **Recommendation:** Consider using `Number(value.toFixed(5))` or a decimal library for mission-critical financial calculations.
73+
74+
---
75+
76+
## Publish Readiness Checklist
77+
78+
| Item | Status |
79+
|------|--------|
80+
| LICENSE file | ✅ Added |
81+
| `license` field in package.json | ✅ Added |
82+
| `author` field | ✅ Added |
83+
| `repository` field | ✅ Added |
84+
| `homepage` field | ✅ Added |
85+
| `files` field (dist only) | ✅ Already set |
86+
| CLI shebang | ✅ Present in built output |
87+
| ESM exports | ✅ Configured |
88+
| TypeScript declarations | ✅ Generated |
89+
| Changesets configured | ✅ `access: public` |
90+
| npm org `@hacienda-cr` | ❌ **Must create** |
91+
| `NPM_TOKEN` secret | ❌ **Must add** |
92+
| All tests pass | ✅ 623 passed |
93+
| Build succeeds | ✅ All 4 packages |
94+
| Lint/format/typecheck | ✅ (CI workflow covers) |
95+
| Acknowledgments | ✅ Added |
96+
| Powered by Dojo Coding | ✅ Added |
97+
98+
---
99+
100+
## What's Left to Go Live
101+
102+
1. **Create `@hacienda-cr` npm org** at npmjs.com
103+
2. **Add `NPM_TOKEN` secret** to GitHub repo settings
104+
3. **Run initial publish** — either:
105+
- Tag `v0.0.1` and push (triggers publish workflow), or
106+
- Manual: `pnpm build && pnpm -r publish --access public --no-git-checks`
107+
4. **Optional:** Create first changeset for v0.1.0 with `pnpm changeset`

0 commit comments

Comments
 (0)