Conversation
…und-trip validation (AX-10) Adds two files per ticket-authorised path allowlist: - tests/cli/crypt/main.go: minimal driver that encrypts a known plaintext with crypt.Encrypt, decrypts with crypt.Decrypt, and asserts round-trip equality via bytes.Equal. Exits non-zero on any encrypt/decrypt error or plaintext mismatch — the AX-10 acceptance criterion. - tests/cli/crypt/Taskfile.yaml: canonical build/test/vet/default targets (default deps-chains build/test/vet) plus driver subtask that runs go run ./tests/cli/crypt. Verified: driver exits 0 on round-trip match. Co-authored-by: Codex <noreply@openai.com> Via-codex-lane: supervised by Cerberus on Athena #110 request Closes tasks.lthn.sh/view.php?id=416
Removes github.com/stretchr/testify from go.mod/go.sum; rewrites assert/require calls across auth/, crypt/ subpackages, trust/, and cmd/testcmd _test.go files to stdlib t.Fatalf patterns. Adds package-local test_helpers_test.go where shared assertion helpers were inlined. go mod tidy, go vet, go test all clean (GOWORK=off). Closes tasks.lthn.sh/view.php?id=750 Co-authored-by: Codex <noreply@openai.com> Via-codex-lane: Cladius-solo dispatch (Mac codex CLI)
Added `// Note:` annotations across crypt/ package: - bench_test.go, checksum.go, hash.go, hmac.go, hmac_test.go, kdf.go, symmetric.go, symmetric_test.go Each crypto/* stdlib import now documents that it is intrinsic — go-crypt IS the core crypto primitive layer and cannot self-depend. Follow-up: auth/auth.go, cmd/crypt/cmd_keygen.go, and the crypt/rsa, crypt/lthn, crypt/chachapoly subdirs also import crypto/* without annotation — out of this ticket's scope, separate sweep needed. Closes tasks.lthn.sh/view.php?id=751 Co-authored-by: Codex <noreply@openai.com>
- Migrate module path: dappco.re/go/core/crypt -> dappco.re/go/crypt - Bump dappco.re/go/* deps to v0.8.0-alpha.1 in go.mod (any forge.lthn.ai/core/* paths migrated to canonical dappco.re/go/* form) - Update Go source imports across 30 .go files Co-Authored-By: Athena <athena@lthn.ai>
Replaced bytes/strings/fmt/encoding usages with core primitives across 20 files. Added cmd/testcmd/cmd_print.go internal helper to keep CLI output formatting away from fmt while pinned core lacks helpers. Closes tasks.lthn.sh/view.php?id=414 Co-authored-by: Codex <noreply@openai.com>
📝 WalkthroughWalkthroughModule and import paths were reorganised to a unified Changes
Sequence Diagram(s)(The conditions for adding sequence diagrams are not met: the changes are largely import/path refactors, test migrations, error-propagation fixes and delegations. No new multi-component control flow that benefits from a sequence diagram was introduced.) 🚥 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⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
bash /tmp/v090/audit.sh . → verdict: COMPLIANT (all 7 dimensions zero). go test -count=1 ./... → all green. Co-authored-by: Codex <noreply@openai.com> Co-Authored-By: Virgil <virgil@lethean.io>
bash /tmp/v090/audit.sh . → verdict: COMPLIANT (all 7 dimensions zero). go test -count=1 ./... → all green. Enchantrix directive applied: replaced in-repo crypto/encoding/hashing with calls into forge.lthn.ai/Snider/Enchantrix v0.0.5 where overlap existed. Net 232 → 53 lines (179-line reduction) — duplicate functionality consolidated into the canonical Lethean encryption layer. Co-authored-by: Codex <noreply@openai.com> Co-Authored-By: Virgil <virgil@lethean.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
crypt/openpgp/service_test.go (1)
15-40:⚠️ Potential issue | 🟠 MajorReplace custom helpers with
testify/assertortestify/requireThe tests use custom
must*/want*helper functions instead of testify assertions. Refactor to useassert.NoError,assert.NotEmpty,assert.Contains,assert.Equal, etc. from the testify library to align with repository standards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/openpgp/service_test.go` around lines 15 - 40, In TestService_EncryptDecrypt_Good replace the custom test helpers (mustNoError, wantNotEmpty, wantContains, wantEqual) with testify assertions: use require.NoError(t, err) for fatal errors (e.g., after s.CreateKeyPair, s.EncryptPGP, s.DecryptPGP) and assert.NotEmpty/ assert.Contains/ assert.Equal for non-fatal checks (e.g., privKey contains "-----BEGIN PGP PRIVATE KEY BLOCK-----", armored and buf.String() non-empty, decrypted equals data); update imports to include "github.com/stretchr/testify/assert" and "github.com/stretchr/testify/require" and remove references to the custom helpers so TestService_EncryptDecrypt_Good, CreateKeyPair, EncryptPGP, and DecryptPGP usages follow repository test conventions.auth/session_store_test.go (1)
138-143:⚠️ Potential issue | 🟡 MinorUse 10 goroutines for concurrency tests.
Line 138 and Line 331 set
n = 20; this conflicts with the repository test-concurrency requirement.As per coding guidelines,
**/*_test.go: Run concurrency tests with 10 goroutines via WaitGroup and pass -race detector.Also applies to: 331-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/session_store_test.go` around lines 138 - 143, Update the concurrency tests in session_store_test.go to use 10 goroutines instead of 20 by changing the variable n to 10 where it is defined (affecting both occurrences around the test's WaitGroup usage), ensure the WaitGroup usage (wg.Add(n)) matches the new n, and keep the existing goroutine launch logic (the anonymous func that runs per iteration) so the test complies with the repository concurrency guideline requiring 10 goroutines and -race detector runs.cmd/testcmd/cmd_runner.go (1)
43-45:⚠️ Potential issue | 🟠 Major
coverageoption is currently ignored.Line 44 always appends
-cover, so thecoverageflag does not control test execution as advertised.Suggested fix
- // Always add coverage - args = append(args, "-cover") + if coverage { + args = append(args, "-cover") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/testcmd/cmd_runner.go` around lines 43 - 45, The code always appends "-cover" to args, ignoring the coverage flag; update the logic in cmd_runner.go so that "-cover" is only appended when the coverage flag/variable (e.g., coverage, coverageEnabled, or the CLI flag you declared) is true—find where args is constructed and wrap the args = append(args, "-cover") call in an if check against that coverage flag, ensuring the test invocation honors the user's coverage choice.trust/trust_test.go (1)
196-199:⚠️ Potential issue | 🟡 MinorConcurrency test fan-out exceeds the project standard.
The changed tests run more than 10 goroutines; please align these cases to the 10-goroutine WaitGroup pattern.
As per coding guidelines,
**/*_test.go: Run concurrency tests with 10 goroutines via WaitGroup and pass -race detector.Also applies to: 295-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trust/trust_test.go` around lines 196 - 199, The test spawns more than the project-standard 10 goroutines; change the concurrency fan-out to exactly 10 by setting the const n = 10 and adjusting the WaitGroup accounting so wg.Add(...) equals n (not n*3) and each goroutine loop (register/get/remove) is constrained to spawn at most n workers; update the wg.Add call and the corresponding goroutine launches in trust_test.go (references: const n, var wg sync.WaitGroup, wg.Add) and make the same change in the other occurrence around lines 295-299.crypt/rsa/rsa_test.go (1)
26-99:⚠️ Potential issue | 🟠 MajorAssertion migration conflicts with test-file requirements
The updated tests no longer use testify
assert/require; they use custom helpers instead.As per coding guidelines,
**/*_test.go: Use testify assert/require with _Good/_Bad/_Ugly naming convention for tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/rsa/rsa_test.go` around lines 26 - 99, Tests currently use custom helpers wantNoError, wantNotEmpty, wantEqual and wantError which conflicts with the repo guideline that tests must use testify; replace all uses of wantNoError -> require.NoError, wantNotEmpty -> require.NotEmpty, wantEqual -> require.Equal (or assert.Equal where non-fatal is desired), and wantError -> require.Error/assert.Error as appropriate in TestRSA_Service_GenerateKeyPair_*; add imports for "github.com/stretchr/testify/require" and "github.com/stretchr/testify/assert", update calls in functions like TestRSA_Service_GenerateKeyPair_Good, TestRSA_Service_GenerateKeyPair_Bad, and TestRSA_Service_GenerateKeyPair_Ugly, and keep the existing Good/Bad/Ugly test names unchanged.trust/config_test.go (1)
33-260:⚠️ Potential issue | 🟠 MajorTest assertions diverge from mandated testify usage
This file now relies on local
must*/want*helpers instead of testifyassert/require, which conflicts with the repository test standard.As per coding guidelines,
**/*_test.go: Use testify assert/require with _Good/_Bad/_Ugly naming convention for tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trust/config_test.go` around lines 33 - 260, Tests are using local must*/want* helpers instead of the repo-mandated testify assert/require; replace those helpers with testify calls (require for fatal checks like require.NoError/NoNil/Len when the test cannot proceed, assert for non-fatal checks like assert.Equal/Contains/Len) and add the imports for "github.com/stretchr/testify/require" and "github.com/stretchr/testify/assert". Specifically, update usages of mustNoError -> require.NoError, mustNotNil/mustNil -> require.NotNil/require.Nil, wantError -> require.Error, wantEqual -> assert.Equal, wantLen -> assert.Len or require.Len when needed, wantContains -> assert.Contains, wantEmpty -> assert.Empty, wantTrue -> assert.True, and adjust tests such as TestConfig_LoadPolicies_Good_FieldMapping, TestConfig_LoadPolicies_Bad_InvalidJSON, TestConfig_PolicyEngine_ApplyPolicies_Good, TestConfig_PolicyEngine_ExportPolicies_Good_RoundTrip, and helper tests (toCapabilities/fromCapabilities) to use the appropriate assert/require calls. Ensure tests keep the same semantics (use require for preconditions/fatal failures) and remove or stop relying on the local must*/want* helper functions.trust/policy_test.go (1)
354-364:⚠️ Potential issue | 🟡 MinorTest assertions inside goroutines may not fail the test correctly.
Calling test assertion helpers (like
wantNotEmpty) from within goroutines can be problematic. If the assertion fails aftert.FailNow()or similar is called from a non-main goroutine, it may cause undefined behaviour or panics. The test should collect results and assert on the main goroutine afterwg.Wait().🛡️ Proposed fix to defer assertions to the main goroutine
+ results := make([]EvalResult, n) for i := range n { go func(idx int) { defer wg.Done() agents := []string{"Athena", "Clotho", "BugSETI-001"} caps := []Capability{CapPushRepo, CapCreatePR, CapCommentIssue} agent := agents[idx%len(agents)] cap := caps[idx%len(caps)] result := pe.Evaluate(agent, cap, "host-uk/core") - wantNotEmpty(t, result.Reason) + results[idx] = result }(i) } wg.Wait() + for _, result := range results { + wantNotEmpty(t, result.Reason) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trust/policy_test.go` around lines 354 - 364, The test currently calls wantNotEmpty(t, ...) from inside goroutines (within the loop that spawns goroutines using wg) which can cause panics; instead, have each goroutine call pe.Evaluate (using agents and caps as before) and store the returned result.Reason into a thread-safe collection (e.g., a channel or a pre-sized slice protected by the loop index), then after wg.Wait() iterate the collected reasons on the main goroutine and call wantNotEmpty for each; keep references to pe.Evaluate, agents, caps, wg and wantNotEmpty so you only move the assertions out of the goroutine and perform them after synchronization.trust/approval_test.go (1)
237-250:⚠️ Potential issue | 🟡 MinorTest assertion inside goroutine may cause issues on failure.
Similar to
policy_test.go, callingwantNoErrorfrom within a goroutine (line 245) can behave unexpectedly if the assertion fails. Consider collecting errors and asserting afterwg.Wait().🛡️ Proposed fix
+ errs := make([]error, n) for i := range n { go func(idx int) { defer wg.Done() id, err := q.Submit( core.Sprintf("agent-%d", idx), CapMergePR, "host-uk/core", ) - wantNoError(t, err) + errs[idx] = err mu.Lock() ids[idx] = id mu.Unlock() }(i) } wg.Wait() + for _, err := range errs { + wantNoError(t, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trust/approval_test.go` around lines 237 - 250, The test currently calls wantNoError inside a goroutine (inside the loop that calls q.Submit), which can misbehave on failure; change the pattern in the goroutine used in approval_test.go so the goroutine captures the result (id and err) into preallocated slices or a channel (e.g., errs[idx] = err and ids[idx] = id while protecting with mu or use a buffered channel), call wg.Done() as before, then after wg.Wait() iterate over errs and call wantNoError for each non-nil error and assert IDs as needed; specifically update the anonymous func used with q.Submit (and the ids and mu usage) to only record results, and move calls to wantNoError out of the goroutine and into the main test thread after wg.Wait().
🟠 Major comments (21)
crypt/test_helpers_test.go-11-179 (1)
11-179:⚠️ Potential issue | 🟠 MajorCustom assertion framework conflicts with the mandated test standard across the codebase.
This helper suite is replicated across multiple packages (trust, auth, crypt, cmd/testcmd) as a systematic project-wide pattern, replacing
testifyassertions in_test.gofiles. This diverges from the coding guidelines, which require**/*_test.go: Use testify assert/require with _Good/_Bad/_Ugly naming convention for tests.Consolidating to the mandated testify standard would ensure consistent test style and reduce duplication of helper functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/test_helpers_test.go` around lines 11 - 179, The test helpers (wantNoError, wantError, wantEqual, wantNil, wantLen, wantContains, wantNotPanic, fail, testMessage, etc.) conflict with project policy—replace usage of these helpers in crypt tests with testify's assert/require APIs and rename test functions to follow the _Good/_Bad/_Ugly naming convention; remove this local helper file from the package (or consolidate into a single, approved test utility package if absolutely necessary) and update any test call sites that reference functions like wantNoError, mustEqual, wantLen, wantContains, wantNil, wantNotEmpty, and wantNotPanic to use equivalent testify.assert/require calls (e.g., require.NoError, assert.Equal, require.Len, assert.Contains, require.Nil, require.NotEmpty, assert.NotPanics) to comply with the mandated standard.crypt/hash_test.go-12-47 (1)
12-47:⚠️ Potential issue | 🟠 MajorReplace custom
want*assertions with testify in_test.goLines 12-47 use local
want*helpers instead of testify assert/require. Per coding guidelines,**/*_test.gofiles must use testify with _Good/_Bad/_Ugly naming convention. The test names already follow the convention correctly; however, the assertions should be migrated from the custom helpers to testify.Suggested direction
+ import "github.com/stretchr/testify/assert" + import "github.com/stretchr/testify/require" - wantNoError(t, err) - wantTrue(t, match) - wantFalse(t, match) + require.NoError(t, err) + assert.True(t, match) + assert.False(t, match)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/hash_test.go` around lines 12 - 47, Replace the custom test helpers (wantNoError, wantNotEmpty, wantContains, wantTrue, wantFalse) with testify's assertions in the tests TestHash_VerifyPassword_Good, TestHash_VerifyPassword_Bad, and TestHash_HashBcrypt_Good: import testify/assert (or require where a failing assertion should stop the test), use assert.NoError/require.NoError for error checks, assert.NotEmpty for hash non-empty, assert.Contains for the "$argon2id$" substring, and assert.True/assert.False for match boolean checks when calling HashPassword/VerifyPassword and HashBcrypt/VerifyBcrypt to conform to the project's testify-based testing style.trust/test_helpers_test.go-11-241 (1)
11-241:⚠️ Potential issue | 🟠 MajorReplace custom assertion helpers with
testify/assertortestify/requireThe repository coding guidelines require test files to use testify's assertion packages with the
_Good/_Bad/_Uglynaming convention. This file creates a custom assertion framework (want*, must*) that circumvents that standard. Migrate the test helpers to usegithub.com/stretchr/testify/assertorrequireinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trust/test_helpers_test.go` around lines 11 - 241, This file implements custom assertion helpers (wantNoError, mustNoError, wantError, mustError, wantErrorIs, wantEqual, mustEqual, wantNotEqual, wantTrue, mustTrue, wantFalse, wantNil, wantNotNil, mustNotNil, wantEmpty, wantNotEmpty, mustNotEmpty, wantLen, mustLen, wantContains, wantGreater, wantNotPanic, fail, testMessage/testMessagef, isNil, isEmpty, lengthOf, containsValue) which violates the repo guideline; remove these custom helpers and migrate tests to use testify's assert/require (github.com/stretchr/testify/assert and/or require) or implement thin wrappers that delegate to assert/require with the repo's _Good/_Bad/_Ugly naming convention; update all tests currently calling the want*/must* helpers to call the corresponding assert/require functions (e.g., assert.NoError/require.NoError for wantNoError/mustNoError, assert.Error/require.Error for wantError/mustError, assert.Equal/require.Equal for wantEqual/mustEqual, assert.Nil/require.Nil, assert.NotNil/require.NotNil, assert.Len/require.Len, assert.Contains, assert.True/require.True, assert.False/require.False, assert.Greater, assert.PanicsNoError equivalent using require.NotPanics), remove unused helper functions (isNil/isEmpty/containsValue/testMessage/fail) once all tests are updated, and add the testify import to affected test files.crypt/chachapoly/chachapoly_test.go-25-31 (1)
25-31:⚠️ Potential issue | 🟠 MajorReplace custom assertion helpers with
testifyassertions.This file uses custom helpers (
wantNoError,wantEqual,wantError) which conflict with the repository coding guideline for*_test.gorequiring testify assert/require.Suggested patch pattern (apply similarly throughout the file)
import ( "crypto/rand" "testing" core "dappco.re/go" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" ) @@ - wantNoError(t, err) + require.NoError(t, err) @@ - wantEqual(t, plaintext, decrypted) + assert.Equal(t, plaintext, decrypted) @@ - wantError(t, err) + require.Error(t, err)Also applies to: 37–38, 47–51, 57–64, 70–76, 83–85, 91–93, 105–106, 112–113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/chachapoly/chachapoly_test.go` around lines 25 - 31, Replace the custom test helpers wantNoError, wantEqual, and wantError in chachapoly_test.go with testify assertions: where the code calls wantNoError(t, err) use require.NoError(t, err); where it calls wantEqual(t, expected, actual) use require.Equal(t, expected, actual) (or assert.Equal if non-fatal), and where it expects failures use require.Error/require.EqualError as appropriate; update all occurrences (including the calls around Decrypt and any other test sections listed) to use testify's require/assert and remove dependency on the custom helpers.crypt/openpgp/service.go-155-158 (1)
155-158:⚠️ Potential issue | 🟠 MajorAdd nil and Encrypted checks before subkey decryption.
subkey.PrivateKey.Decrypt(...)is called without guards. A subkey's private key can be nil or not encrypted, causing unnecessary errors or panics. The same pattern is correctly guarded incrypt/pgp/pgp.gousingif subkey.PrivateKey != nil && subkey.PrivateKey.Encrypted.Proposed fix
for _, subkey := range entity.Subkeys { + if subkey.PrivateKey == nil || !subkey.PrivateKey.Encrypted { + continue + } if err := subkey.PrivateKey.Decrypt([]byte(passphrase)); err != nil { return "", coreerr.E("openpgp.DecryptPGP", "failed to decrypt subkey", err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/openpgp/service.go` around lines 155 - 158, In the loop over entity.Subkeys where subkey.PrivateKey.Decrypt([]byte(passphrase)) is called, add guards to check that subkey.PrivateKey is not nil and that subkey.PrivateKey.Encrypted is true before calling Decrypt (mirroring the pattern used in crypt/pgp/pgp.go); update the logic around entity.Subkeys and the Decrypt call to skip subkeys that have a nil PrivateKey or are not encrypted to avoid panics and unnecessary errors.crypt/lthn/lthn_test.go-10-11 (1)
10-11:⚠️ Potential issue | 🟠 MajorReplace custom assertion helpers with testify
assert/require.This test file uses local
want*helpers instead of the required testify library. Switch all assertions across the file to userequirefor critical assertions andassertfor checks, maintaining the existing _Good/_Bad/_Ugly naming convention for test functions.Pattern for conversion
import ( "sync" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) @@ - wantNotEmpty(t, hash) - wantLen(t, hash, 64) + require.NotEmpty(t, hash) + assert.Len(t, hash, 64)Applies to lines: 10-11, 16-17, 22-24, 30, 37, 46-47, 53-54, 70.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/lthn/lthn_test.go` around lines 10 - 11, Replace the local want* helpers in lthn_test.go with testify assertions: import "github.com/stretchr/testify/require" and "github.com/stretchr/testify/assert", keep the existing TestGood/TestBad/TestUgly function names, use require.* for critical/fatal checks (e.g., require.NotEmpty(t, hash) and require.Len(t, hash, 64) instead of wantNotEmpty/wantLen) and use assert.* for non-fatal checks elsewhere; update all occurrences (the helper calls on lines referenced: wantNotEmpty, wantLen, and other want* usages) to their testify equivalents and remove the custom helper functions/imports. Ensure the test imports and calls use the testing.T passed into each test (require.NotNil/NoError/Equal/Contains/Len/NotEmpty as appropriate).cmd/crypt/ax7_commands_test.go-8-9 (1)
8-9:⚠️ Potential issue | 🟠 MajorUse testify assertions instead of
dappco.re/goassertions.Replace
AssertTrueandAssertFalsecalls with testify'sassertorrequirefunctions. Lines 8, 9, 16, and 23 currently use custom assertions from thedappco.re/gopackage; testify is the required assertion framework for test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crypt/ax7_commands_test.go` around lines 8 - 9, The tests currently use custom assertions AssertTrue/AssertFalse; replace them with testify assertions (assert or require) to conform to the project's test framework: for each call like AssertTrue(t, c.Command("crypt/hash").OK) or AssertFalse(t, someExpr) swap to assert.True(t, c.Command("crypt/hash").OK) or assert.False(t, someExpr) (or require.* if the check should abort the test), and import "github.com/stretchr/testify/assert" (or require) and remove the dappco.re/go assertion import; update all occurrences referenced (AssertTrue, AssertFalse and the related c.Command("crypt/hash").OK, c.Command("crypt/checksum").OK, etc.).cmd/testcmd/ax7_commands_test.go-8-8 (1)
8-8:⚠️ Potential issue | 🟠 MajorMigrate assertions to testify
assert/require.Lines 8, 14, and 21 use custom assertions from
dappco.re/goinstead of testify, which violates the repository test conventions for*_test.gofiles. The test function naming (_Good/_Bad/_Ugly) is correctly applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/testcmd/ax7_commands_test.go` at line 8, Replace the custom dappco.re/go assertions with testify's assertions: import "github.com/stretchr/testify/require" (or "github.com/stretchr/testify/assert" if non-fatal is desired) and replace calls like AssertTrue(t, c.Command("test").OK) with require.True(t, c.Command("test").OK). Do the same for the other uses on the file (lines with AssertTrue/other dappco assertion helpers), remove the dappco.re/go import, and update test calls (e.g., require.False/require.Equal as appropriate) to match the original intent.crypt/pgp/ax7_pgp_test.go-8-9 (1)
8-9:⚠️ Potential issue | 🟠 MajorReplace
core.Require*/core.Assert*with testifyassert/requirein test assertions.The code currently uses assertions from
dappco.re/go/core(e.g.,core.RequireNoError,core.AssertNoError), but the repository guideline requires testify-based assertions in*_test.gofiles. The test functions correctly follow the_Good/_Bad/_Uglynaming convention.Also applies to lines: 16-19, 24-25, 31-34, 40-42, 48-50, 56-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/pgp/ax7_pgp_test.go` around lines 8 - 9, Tests in ax7_pgp_test.go use core.Require*/core.Assert* helpers (e.g., core.RequireNoError, core.RequireTrue, core.AssertNoError) but should use testify assertions; update the imports to include "github.com/stretchr/testify/require" (and "github.com/stretchr/testify/assert" if needed) and replace core.RequireNoError -> require.NoError(t, err), core.RequireTrue -> require.True(t, cond) (or assert.* where non-fatal is desired), and core.AssertNoError -> assert.NoError(t, err) across all occurrences mentioned (lines grouped around the named calls) so tests use testify-style assertions consistently.cmd/testcmd/cmd_main.go-40-48 (1)
40-48:⚠️ Potential issue | 🟠 MajorAvoid shared mutable flags in command action.
Line 41–Line 47 mutate package-level variables before calling
runTest, which can cause cross-run state bleed and data races if commands are invoked concurrently.Suggested fix
Action: func(opts core.Options) core.Result { - testVerbose = opts.Bool("verbose") - testCoverage = opts.Bool("coverage") - testShort = opts.Bool("short") - testPkg = opts.String("pkg") - testRun = opts.String("run") - testRace = opts.Bool("race") - testJSON = opts.Bool("json") - return core.ResultOf(nil, runTest(testVerbose, testCoverage, testShort, testPkg, testRun, testRace, testJSON)) + verbose := opts.Bool("verbose") + coverage := opts.Bool("coverage") + short := opts.Bool("short") + pkg := opts.String("pkg") + run := opts.String("run") + race := opts.Bool("race") + json := opts.Bool("json") + return core.ResultOf(nil, runTest(verbose, coverage, short, pkg, run, race, json)) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/testcmd/cmd_main.go` around lines 40 - 48, The Action closure mutates package-level flags (testVerbose, testCoverage, testShort, testPkg, testRun, testRace, testJSON) causing shared mutable state; instead read values from core.Options into local variables (or construct a small TestOptions struct) and pass those locals into runTest (or change runTest signature to accept the TestOptions) so the package-level variables are not modified by Action and concurrent runs are safe; update references to use the new local variables/struct and remove the direct writes to the package-level symbols.trust/scope_test.go-12-13 (1)
12-13:⚠️ Potential issue | 🟠 MajorReplace custom test helpers with testify assertions throughout this test file.
The test file uses custom helper functions (
wantTrue,wantFalse,wantEqual, etc.) instead of the required testifyassert/requirepackage. Test function naming follows the _Good/_Bad/_Ugly convention correctly, but the assertion methodology must be updated to use testify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trust/scope_test.go` around lines 12 - 13, The tests in trust/scope_test.go use custom helpers (wantTrue, wantFalse, wantEqual, etc.) instead of testify; update each test to use testify assertions (import "github.com/stretchr/testify/assert" or "require" as needed) and replace calls like wantTrue(t, matchScope(pattern, repo)) with assert.True(t, matchScope(pattern, repo)), wantFalse -> assert.False, wantEqual -> assert.Equal, etc.; remove or stop relying on the custom helper functions and update the file imports accordingly so all assertions in functions testing matchScope and related logic use testify.trust/trust_test.go-13-17 (1)
13-17:⚠️ Potential issue | 🟠 MajorSwitch to
testifyassert/require for all assertions.Lines 13–17 use custom
want*helpers instead of the required testify library. ReplacewantEqual()withassert.Equal()orrequire.Equal(),wantTrue()withassert.True(),wantFalse()withassert.False(),mustNoError()withrequire.NoError(), and similar conversions throughout the entire test file.As per coding guidelines,
**/*_test.gomust use testify assert/require with_Good/_Bad/_Uglynaming convention. The file already follows the naming convention; adopt testify assertions to comply fully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trust/trust_test.go` around lines 13 - 17, Replace custom test helpers with testify assertions in TestTrust_Tier_String_Good and throughout this test file: change wantEqual(t, ...) calls (e.g., wantEqual(t, "untrusted", TierUntrusted.String())) to assert.Equal(t, "untrusted", TierUntrusted.String()) or require.Equal as appropriate; similarly convert wantTrue()/wantFalse() to assert.True/assert.False and mustNoError()/mustNoErr()/wantNoError() to require.NoError, keeping existing test names like TestTrust_Tier_String_Good; ensure the testify package is imported (testify/assert and/or testify/require) and remove references to the custom helper functions.auth/session_store_test.go-28-33 (1)
28-33:⚠️ Potential issue | 🟠 MajorUse testify assert/require instead of custom helpers, and fix concurrency test goroutine counts.
As per coding guidelines,
**/*_test.gofiles must use testify assert/require with the_Good/Bad/Uglynaming convention. The current custom helpers (mustNoError, wantEqual, wantErrorIs, etc.) in test_helpers_test.go diverge from this standard and must be replaced with testify.Additionally, concurrency tests currently run with
const n = 20but guidelines require 10 goroutines. Update TestSessionStore_MemorySessionStore_Concurrent_Good and TestSessionStore_SQLiteSessionStore_Concurrent_Good to useconst n = 10.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/session_store_test.go` around lines 28 - 33, Replace custom test helpers in auth/session_store_test.go and related tests (mustNoError, wantEqual, wantErrorIs, etc.) with testify's require/assert API (e.g., require.NoError(t, err), assert.Equal(t, expected, actual), require.ErrorIs/ assert.ErrorIs as appropriate) and adjust test names/variants to follow the _Good/_Bad/_Ugly convention where applicable; also change the concurrency goroutine count constant in TestSessionStore_MemorySessionStore_Concurrent_Good and TestSessionStore_SQLiteSessionStore_Concurrent_Good from const n = 20 to const n = 10 to meet guidelines. Ensure you import "github.com/stretchr/testify/assert" and/or "github.com/stretchr/testify/require" and remove or stop using the custom helpers from test_helpers_test.go.crypt/lthn/ax7_lthn_test.go-15-18 (1)
15-18:⚠️ Potential issue | 🟠 MajorSwitch entire test file to testify
assert/require.The test naming convention (
_Good/_Bad/_Ugly) is already correctly implemented. However, the current custom assertion API (AssertEqual,AssertTrue, etc. fromdappco.re/go) does not comply with the coding guideline mandating testifyassert/require. All assertions in this file (not just lines 15–18) must be migrated to testify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/lthn/ax7_lthn_test.go` around lines 15 - 18, Replace the custom dappco.re/go assertions in this test (and the entire file) with testify's assertions: import "github.com/stretchr/testify/assert" (or require) and change AssertEqual(t, expected, actual) to assert.Equal(t, expected, actual) (or require.Equal), and likewise convert any AssertTrue/AssertFalse/etc.; update TestAX7LTHN_SetKeyMap_Good to call preserveKeyMap(t), SetKeyMap(...), then assert.Equal(t, map[rune]rune{'x':'y'}, GetKeyMap()). Ensure all other tests in this file use testify assert/require consistently.cmd/crypt/cmd_encrypt.go-24-26 (1)
24-26:⚠️ Potential issue | 🟠 MajorPackage-level option state is not safe for concurrent command execution
Storing per-invocation options in globals can leak state between runs and introduces data-race risk if commands execute in parallel. Keep options local and pass them through function parameters.
Suggested refactor
- encryptPassphrase = opts.String("passphrase") - encryptAES = opts.Bool("aes") - return core.ResultOf(nil, runEncrypt(path)) + passphrase := opts.String("passphrase") + useAES := opts.Bool("aes") + return core.ResultOf(nil, runEncrypt(path, passphrase, useAES)) ... - encryptPassphrase = opts.String("passphrase") - encryptAES = opts.Bool("aes") - return core.ResultOf(nil, runDecrypt(path)) + passphrase := opts.String("passphrase") + useAES := opts.Bool("aes") + return core.ResultOf(nil, runDecrypt(path, passphrase, useAES))-func getPassphrase() (string, error) { - if encryptPassphrase != "" { - return encryptPassphrase, nil +func getPassphrase(passphrase string) (string, error) { + if passphrase != "" { + return passphrase, nil } return cli.Prompt("Passphrase", "") }Also applies to: 36-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crypt/cmd_encrypt.go` around lines 24 - 26, The package-level variables encryptPassphrase and encryptAES are used to store per-invocation options which is unsafe for concurrent commands; change the command handler to read opts.String("passphrase") and opts.Bool("aes") into local variables and pass them into runEncrypt (or create a new function like runEncryptWithOptions) instead of relying on the globals, and remove or stop using the package-level encryptPassphrase/encryptAES state so each invocation is fully local and concurrency-safe.cmd/crypt/cmd_encrypt.go-22-23 (1)
22-23:⚠️ Potential issue | 🟠 MajorUse the mandated error constructor
These new error returns use
cli.Err(...), but repository policy requirescoreerr.E("package.Function", "lowercase message", err).As per coding guidelines,
**/*.go: Usecoreerr.E("package.Function", "lowercase message", err)for error handling; never include secrets in error strings.Also applies to: 34-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crypt/cmd_encrypt.go` around lines 22 - 23, Replace uses of cli.Err(...) passed to core.Fail with the mandated coreerr.E(...) constructor: for each occurrence (e.g., the return core.Fail(cli.Err("encrypt requires a path")) and the ones around lines 34-35), call core.Fail(coreerr.E("crypt.Encrypt", "encrypt requires a path", nil)) or similar messages and pass the underlying error as the third argument if one exists; ensure messages are lowercase and contain no secrets and use "crypt.Encrypt" (or the exact package.Function name for this command handler) as the first argument.cmd/crypt/cmd_keygen.go-22-28 (1)
22-28:⚠️ Potential issue | 🟠 MajorAvoid global mutable flags in command actions
Persisting invocation options in package globals makes
runKeygennon-reentrant and can cause cross-invocation contamination. Passlength/hex/base64directly intorunKeygen.Suggested refactor
- keygenLength = opts.Int("length") - if keygenLength == 0 { - keygenLength = 32 + length := opts.Int("length") + if length == 0 { + length = 32 } - keygenHex = opts.Bool("hex") - keygenBase64 = opts.Bool("base64") - return core.ResultOf(nil, runKeygen()) + hexOut := opts.Bool("hex") + base64Out := opts.Bool("base64") + return core.ResultOf(nil, runKeygen(length, hexOut, base64Out))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crypt/cmd_keygen.go` around lines 22 - 28, The command action currently writes opts into package globals keygenLength, keygenHex, keygenBase64 and then calls runKeygen(), which makes the logic non-reentrant; change runKeygen to accept parameters (e.g. func runKeygen(length int, hex bool, base64 bool) error) and update the call site to pass opts.Int("length") (defaulting to 32), opts.Bool("hex"), and opts.Bool("base64") directly, removing reliance on the globals; update any internal references in runKeygen to use the parameter names and delete the now-unused global variables.crypt/checksum_test.go-14-77 (1)
14-77:⚠️ Potential issue | 🟠 MajorPlease restore testify assertions in this test file
The migrated
want*/must*helpers do not satisfy the required testifyassert/requirestandard for*_test.gofiles.As per coding guidelines,
**/*_test.go: Use testify assert/require with _Good/_Bad/_Ugly naming convention for tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/checksum_test.go` around lines 14 - 77, The tests in checksum_test.go use legacy helpers (wantEqual, mustNoError, mustTrue, wantError, wantContains) instead of testify; update each TestChecksum_* (e.g., TestChecksum_SHA256File_Good, TestChecksum_SHA512File_Good, TestChecksum_SHA256FileNonExistent_Bad, TestChecksum_SHA512FileNonExistent_Bad, TestChecksum_SHA256FileWithContent_Good) to use testify's assert/require: add imports for "github.com/stretchr/testify/assert" and "github.com/stretchr/testify/require", replace wantEqual -> assert.Equal(t, ...), mustNoError -> require.NoError(t, err), mustTrue -> require.True(t, ...), wantError -> assert.Error(t, err) and wantContains -> assert.Contains(t, err.Error(), ...); ensure calls around SHA256File and SHA512File use require.NoError where subsequent values are used and assert.Error/assert.Contains for error cases.crypt/chachapoly/ax7_chachapoly_test.go-11-56 (1)
11-56:⚠️ Potential issue | 🟠 MajorAssertion framework does not match project test policy
The test names follow
_Good/_Bad/_Ugly, but assertions are not using testifyassert/requireas required.As per coding guidelines,
**/*_test.go: Use testify assert/require with _Good/_Bad/_Ugly naming convention for tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/chachapoly/ax7_chachapoly_test.go` around lines 11 - 56, Replace the custom core assertion calls with testify's assert/require per project policy: in TestAX7Chachapoly_Encrypt_Good/Bad/Ugly and TestAX7Chachapoly_Decrypt_Good/Bad/Ugly, import testify's assert and require and convert core.AssertNoError/core.RequireNoError to require.NoError, core.AssertError to assert.Error (or require.Error when failure should stop the test), core.AssertNil to assert.Nil, core.AssertTrue to assert.True, and core.AssertEqual to assert.Equal; keep the same test names and use existing functions ax7Key, Encrypt and Decrypt for setup and assertions.crypt/pgp/test_helpers_test.go-11-175 (1)
11-175:⚠️ Potential issue | 🟠 MajorCustom assertion layer conflicts with the test standard
This file replaces the required testify
assert/requireusage with bespoke helpers, which diverges from the repository’s mandated test pattern.As per coding guidelines,
**/*_test.go: Use testify assert/require with _Good/_Bad/_Ugly naming convention for tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/pgp/test_helpers_test.go` around lines 11 - 175, This file defines custom test helpers (wantNoError, mustNoError, wantError, mustError, wantErrorIs, wantEqual, mustEqual, wantNotEqual, wantTrue, mustTrue, wantFalse, wantNil, wantNotNil, mustNotNil, wantEmpty, wantNotEmpty, mustNotEmpty, wantLen, mustLen, wantContains, wantGreater, wantNotPanic, fail, testMessage) which conflicts with the repo policy requiring testify's assert/require and the Good/Bad/Ugly naming; remove this bespoke helper layer and update tests to use testify's assert/require equivalents (e.g. assert.NoError/require.NoError, assert.Error, assert.Equal, assert.NotNil, assert.Empty, assert.Len, assert.Contains, assert.True/False, require.NoPanic, errors.Is checks via assert.ErrorIs/require.ErrorIs, etc.), and adjust test names to follow the Good/Bad/Ugly convention; ensure all references to the functions listed above are replaced across tests with the corresponding testify calls and remove this file.auth/test_helpers_test.go-11-179 (1)
11-179:⚠️ Potential issue | 🟠 MajorCustom test assertions break the required test style
This helper layer replaces the required testify
assert/requirepattern for test files.As per coding guidelines,
**/*_test.go: Use testify assert/require with _Good/_Bad/_Ugly naming convention for tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/test_helpers_test.go` around lines 11 - 179, The file defines many custom test helpers (wantNoError, mustNoError, wantError, mustError, wantEqual, mustEqual, wantNil, wantNotNil, wantEmpty, wantLen, wantContains, fail, testMessage, etc.) which violates the guideline to use testify's assert/require; remove or stop using these helpers and replace their call sites with the equivalent testify assertions (assert.NoError/require.NoError, assert.Error/require.Error, assert.Equal/require.Equal, assert.Nil/require.Nil, assert.NotNil/require.NotNil, assert.Empty/require.Empty, assert.Len/require.Len, assert.Contains/require.Contains, require.NotNil for must* semantics, and t.Helper() behavior handled by testify) and delete this helper file (or keep only minimal wrappers that delegate to testify) so tests follow the _Good/_Bad/_Ugly naming and use assert/require consistently; search for usages of the listed function names to update call sites.
🟡 Minor comments (8)
crypt/test_helpers_test.go-220-227 (1)
220-227:⚠️ Potential issue | 🟡 Minor
wantContainscan panic on nil collections.
reflect.ValueOf(collection).Kind()is called without checking for a nil interface value, sowantContains(t, nil, x)can panic.Defensive fix
func containsValue(collection, item any) bool { + if collection == nil { + return false + } + if s, ok := collection.(string); ok { substr, ok := item.(string) return ok && strings.Contains(s, substr) } v := reflect.ValueOf(collection) + if !v.IsValid() { + return false + } switch v.Kind() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/test_helpers_test.go` around lines 220 - 227, The function containsValue can panic when called with a nil interface; before using reflect.ValueOf(collection).Kind() (or accessing v) add a nil/invalid check: call v := reflect.ValueOf(collection) and return false if !v.IsValid() (or simply return false if collection == nil) so nil collections are handled safely, then continue with the existing switch on v.Kind() (function: containsValue).crypt/hash_test.go-4-6 (1)
4-6:⚠️ Potential issue | 🟡 MinorReorder imports: stdlib before third-party
The import block places third-party (
golang.org/x/crypto/bcrypt) before stdlib (testing). Reorder as: stdlib first, then blank line, then third-party.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/hash_test.go` around lines 4 - 6, Reorder the import block in crypt/hash_test.go so standard library imports come first: move "testing" above the third-party "golang.org/x/crypto/bcrypt" and add a blank line between stdlib and third-party imports; update the import statement ordering accordingly.cmd/testcmd/cmd_runner.go-67-73 (1)
67-73:⚠️ Potential issue | 🟡 MinorVerbose mode can print test output twice.
When verbose and non-JSON, output is already streamed via
io.MultiWriter; printingcombinedagain causes duplication.Also applies to: 108-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/testcmd/cmd_runner.go` around lines 67 - 73, The code currently streams output to both the terminal and internal buffers when verbose && !jsonOutput (cmd.Stdout/cmd.Stderr set via io.MultiWriter) but then prints the collected "combined" output again, causing duplicate output; change the logic so the final print of "combined" only happens when output was NOT already streamed to os.Stdout (i.e., only when !(verbose && !jsonOutput) or equivalently when !verbose || jsonOutput). Apply this same conditional fix for the other occurrence noted (the block around lines 108-117) so that combined is only written when cmd.Stdout/cmd.Stderr were not set to io.MultiWriter.crypt/lthn/ax7_lthn_test.go-7-13 (1)
7-13:⚠️ Potential issue | 🟡 Minor
preserveKeyMapdoes not preservenilstate correctly.If
GetKeyMap()isnil, the cleanup currently restores an empty map, notnil. This can alter global state between tests.Suggested fix
func preserveKeyMap(t *T) { t.Helper() original := GetKeyMap() - clone := make(map[rune]rune, len(original)) - for k, v := range original { - clone[k] = v - } - t.Cleanup(func() { SetKeyMap(clone) }) + var clone map[rune]rune + if original != nil { + clone = make(map[rune]rune, len(original)) + for k, v := range original { + clone[k] = v + } + } + t.Cleanup(func() { SetKeyMap(clone) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/lthn/ax7_lthn_test.go` around lines 7 - 13, The cleanup currently always restores a non-nil empty map by creating clone := make(map[rune]rune, len(original)) even when original is nil; modify the logic in the helper that captures GetKeyMap() so that if original == nil you call SetKeyMap(nil) in t.Cleanup, otherwise copy entries into a new map and call SetKeyMap(clone). Ensure you reference GetKeyMap and SetKeyMap (and the local clone variable) so the restored state matches the original nil vs non-nil semantics.cmd/testcmd/test_helpers_test.go-222-224 (1)
222-224:⚠️ Potential issue | 🟡 MinorGuard
containsValueagainstnilcollections.
reflect.ValueOf(collection).Kind()will panic for a nil/invalid value; add an early validity check.Suggested fix
func containsValue(collection, item any) bool { + if collection == nil { + return false + } if s, ok := collection.(string); ok { substr, ok := item.(string) return ok && strings.Contains(s, substr) } v := reflect.ValueOf(collection) + if !v.IsValid() { + return false + } switch v.Kind() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/testcmd/test_helpers_test.go` around lines 222 - 224, The containsValue function currently calls reflect.ValueOf(collection).Kind() which can panic for nil/invalid inputs; update containsValue to first get v := reflect.ValueOf(collection) and guard with v.IsValid() (and for pointer/interface kinds also check v.IsNil()) and return false (or an appropriate zero result) for invalid/nil collections before switching on v.Kind(); keep the rest of the logic (the reflect.Array/reflect.Slice branch) unchanged and reference containsValue and the reflect.ValueOf(collection).Kind() call when making the change.auth/test_helpers_test.go-220-228 (1)
220-228:⚠️ Potential issue | 🟡 MinorGuard nil before reflective kind checks
containsValuecan panic on nilcollectioninputs; returnfalseinstead of panicking.Suggested patch
func containsValue(collection, item any) bool { + if collection == nil { + return false + } if s, ok := collection.(string); ok { substr, ok := item.(string) return ok && strings.Contains(s, substr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/test_helpers_test.go` around lines 220 - 228, containsValue can panic when collection is nil because reflect.ValueOf(collection).Kind() is called on an invalid value; update containsValue to first guard against nil/invalid inputs (e.g., if collection == nil or v := reflect.ValueOf(collection); !v.IsValid() { return false }) before calling v.Kind(), then continue with the existing string and slice/array handling; this ensures containsValue returns false for nil collections instead of panicking.crypt/hash.go-202-213 (1)
202-213:⚠️ Potential issue | 🟡 Minor
padRawBase64does not handle invalid input length (len%4 == 1).When
len(s) % 4 == 1, this is an invalid state for base64 encoding (valid raw base64 can only have remainders of 0, 2, or 3). The current implementation returns the string unchanged in thedefaultcase, which will causeBase64Decodeto fail later. Consider returning an error or handling this explicitly.🛡️ Proposed fix to handle invalid input explicitly
-func padRawBase64(s string) string { +func padRawBase64(s string) (string, error) { switch len(s) % 4 { case 0: - return s + return s, nil case 2: - return s + "==" + return s + "==", nil case 3: - return s + "=" + return s + "=", nil default: - return s + return "", coreerr.E("crypt.padRawBase64", "invalid raw base64 length", nil) } }Then update
rawBase64Decodeto propagate the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/hash.go` around lines 202 - 213, The padRawBase64 function currently returns the input unchanged for the invalid case len(s)%4 == 1; change padRawBase64 to explicitly detect the remainder == 1 case and return an error (or a second return value) instead of the string, and update rawBase64Decode to accept and propagate that error (i.e., call padRawBase64, handle its error, and return the error to callers such as Base64Decode) so invalid raw base64 inputs are rejected early; update signatures of padRawBase64 and rawBase64Decode (and any callers) to support the error return.go.mod-68-80 (1)
68-80:⚠️ Potential issue | 🟡 MinorReplace directives violate coding guidelines.
The
go.modcontains multiplereplacedirectives, which violates the guideline: "Do not add replace directives togo.mod— use the workspace file (~/Code/go.work) instead for local dependencies."As per coding guidelines, these should be moved to a workspace file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 68 - 80, Remove the inline replace directives from go.mod and move them into your workspace file (~/Code/go.work): delete the seven "replace dappco.re/..." lines from go.mod and instead add corresponding workspace entries (use ./cli, use ./path/to/go-i18n, etc. or appropriate local module paths) in go.work so local modules are included via the workspace rather than by replacing in go.mod; reference the replace directives shown in go.mod (e.g., "replace dappco.re/go/cli", "replace dappco.re/go/i18n", "replace dappco.re/go/io", "replace dappco.re/go/log", "replace dappco.re/go/process", "replace dappco.re/go/store", "replace dappco.re/go/inference") when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1585c654-550f-4726-a8e8-e4732cb22428
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (75)
auth/auth.goauth/auth_test.goauth/ax7_auth_test.goauth/session_store.goauth/session_store_sqlite.goauth/session_store_test.goauth/test_helpers_test.gocmd/crypt/ax7_commands_test.gocmd/crypt/cmd.gocmd/crypt/cmd_checksum.gocmd/crypt/cmd_encrypt.gocmd/crypt/cmd_hash.gocmd/crypt/cmd_keygen.gocmd/testcmd/ax7_commands_test.gocmd/testcmd/cmd_commands.gocmd/testcmd/cmd_main.gocmd/testcmd/cmd_output.gocmd/testcmd/cmd_print.gocmd/testcmd/cmd_runner.gocmd/testcmd/output_test.gocmd/testcmd/test_helpers_test.gocrypt/ax7_crypt_test.gocrypt/bench_test.gocrypt/chachapoly/ax7_chachapoly_test.gocrypt/chachapoly/chachapoly.gocrypt/chachapoly/chachapoly_test.gocrypt/chachapoly/test_helpers_test.gocrypt/checksum.gocrypt/checksum_test.gocrypt/crypt.gocrypt/crypt_test.gocrypt/hash.gocrypt/hash_test.gocrypt/hmac.gocrypt/hmac_test.gocrypt/kdf.gocrypt/kdf_test.gocrypt/lthn/ax7_lthn_test.gocrypt/lthn/lthn.gocrypt/lthn/lthn_test.gocrypt/lthn/test_helpers_test.gocrypt/openpgp/ax7_service_test.gocrypt/openpgp/service.gocrypt/openpgp/service_test.gocrypt/openpgp/test_helpers_test.gocrypt/pgp/ax7_pgp_test.gocrypt/pgp/pgp.gocrypt/pgp/pgp_test.gocrypt/pgp/test_helpers_test.gocrypt/rsa/ax7_rsa_test.gocrypt/rsa/rsa.gocrypt/rsa/rsa_test.gocrypt/rsa/test_helpers_test.gocrypt/symmetric.gocrypt/symmetric_test.gocrypt/test_helpers_test.gogo.modinternal/corecompat/ax7_encode_test.gointernal/corecompat/encode.gotests/cli/crypt/Taskfile.yamltests/cli/crypt/main.gotrust/approval.gotrust/approval_test.gotrust/audit.gotrust/audit_test.gotrust/ax7_trust_test.gotrust/bench_test.gotrust/config.gotrust/config_test.gotrust/policy.gotrust/policy_test.gotrust/scope_test.gotrust/test_helpers_test.gotrust/trust.gotrust/trust_test.go
| import ( | ||
| "crypto/sha256" | ||
|
|
||
| core "dappco.re/go" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that core.T is a type alias or wrapper for *testing.T
ast-grep --pattern $'type T = $_'Repository: dAppCore/go-crypt
Length of output: 43
🏁 Script executed:
# Search for the definition of core.T
rg "type T " --type goRepository: dAppCore/go-crypt
Length of output: 43
🏁 Script executed:
# Also check if core.T is imported or defined in the file
head -20 crypt/ax7_crypt_test.goRepository: dAppCore/go-crypt
Length of output: 484
🏁 Script executed:
# Look for how core.T is used in test functions
rg "func Test.*\(.*\*?core\.T\)" crypt/ax7_crypt_test.go | head -5Repository: dAppCore/go-crypt
Length of output: 294
🏁 Script executed:
# Try to find core.T definition in the dappco.re/go package by looking at go.mod
cat go.mod | grep -A5 "dappco.re"Repository: dAppCore/go-crypt
Length of output: 1497
🏁 Script executed:
# Search for core.T definition across all Go files
rg "type T\s*(struct|interface)" --type goRepository: dAppCore/go-crypt
Length of output: 43
🏁 Script executed:
# Try to find if core package is defined locally in this repo
find . -type f -name "*.go" -path "*/core/*" | head -10Repository: dAppCore/go-crypt
Length of output: 43
🏁 Script executed:
# Check if there are any local core package definitions
fd "core" --type dRepository: dAppCore/go-crypt
Length of output: 82
🏁 Script executed:
# Check what's in the corecompat directory
find internal/corecompat -type f | head -10Repository: dAppCore/go-crypt
Length of output: 130
🏁 Script executed:
# Search for any mention of testing.T in the codebase to understand test patterns
rg "testing\.T" --type go | head -5Repository: dAppCore/go-crypt
Length of output: 411
🌐 Web query:
dappco.re/go core.T type golang testing
💡 Result:
dappco.re/go/core is a Go module (CoreGO) providing dependency injection, service lifecycle management, permission checks, and message-passing for Go applications. It features a central Core container, Options for configuration, Result for outputs, Command tree, ACTION/QUERY message bus, Entitled permissions, and generic Registry[T] collections. It requires Go 1.26+, licensed EUPL-1.2, repo at github.com/dAppCore/core. No specific "core.T" type was found in the documentation or tests. All tests use the standard *testing.T from Go's testing package (e.g., func TestWithService_NameDiscovery_Good(t *testing.T)). The module uses github.com/stretchr/testify/assert for assertions and runs 483 tests with 84.7% coverage via go test ./.... To use in testing: Import "testing" and "dappco.re/go/core", write functions like func TestSomething(t *testing.T) { c := core.New(...) ; assert.True(t, c.Services.OK) }.
Citations:
- 1: https://pkg.go.dev/dappco.re/go/core
- 2: https://github.com/dAppCore/core/tree/dev
- 3: https://github.com/dAppCore/core/blob/dev/contract_test.go
- 4: https://github.com/dAppCore/core/blob/dev/app_test.go
- 5: https://github.com/dAppCore/core
- 6: https://github.com/dAppCore/core/blob/dev/ipc_test.go
- 7: https://github.com/dAppCore/core/blob/dev/service_test.go
- 8: https://github.com/dAppCore/core/blob/dev/registry_test.go
🏁 Script executed:
# Search for any definition or mention of core.T as a test type
rg "core\.T" --type go -B2 -A2 | head -30Repository: dAppCore/go-crypt
Length of output: 1367
🏁 Script executed:
# Check if core package exports a T type by looking at imports
rg "import.*core" crypt/ax7_crypt_test.go -A10Repository: dAppCore/go-crypt
Length of output: 43
🏁 Script executed:
# Look for any type T definition anywhere in the codebase
rg "^type T " --type goRepository: dAppCore/go-crypt
Length of output: 43
🏁 Script executed:
# Verify if the file compiles by checking for any build markers or comments
head -30 crypt/ax7_crypt_test.goRepository: dAppCore/go-crypt
Length of output: 785
Test function signature uses core.T which is incompatible with Go's test discovery.
The Go test runner requires test functions with signature func TestXxx(t *testing.T). Using *core.T instead will prevent test discovery. While this pattern is used consistently elsewhere in the repository, I cannot locate a definition of core.T in the codebase or in the dappco.re/go package documentation. If core.T is not *testing.T or a proper wrapper for it, these tests cannot be executed by go test. Change test signatures to use the standard *testing.T type, or if core.T is intentional, verify it is properly embedded/aliased and document this decision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypt/ax7_crypt_test.go` around lines 3 - 7, The test functions in this file
use core.T which is incompatible with go test; replace usages of core.T with the
standard *testing.T (add import "testing") or, if core.T is intentionally a
wrapper, ensure core.T is an alias/embedding of testing.T and document it;
update all function signatures named Test... to use func TestXxx(t *testing.T)
and adjust any t.<methods> call sites to the testing.T API (or confirm core.T
implements testing.T methods) so the Go test runner can discover and run them.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
crypt/lthn/lthn.go (1)
22-23: Trim the package contract to the wrapper boundary.This file now delegates the hashing behaviour upstream, but the package comments still read as if the local code enforces the full RFC-0004 algorithm. Please either document this as a compatibility wrapper or point readers at the upstream implementation; otherwise the comments will drift from the code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypt/lthn/lthn.go` around lines 22 - 23, Update the package comment in crypt/lthn/lthn.go to reflect that this package is a compatibility wrapper that delegates hashing behavior to the upstream implementation (enchantrixlthn from forge.lthn.ai/Snider/Enchantrix/pkg/crypt/std/lthn) rather than implementing RFC-0004 itself; explicitly state it forwards calls to enchantrixlthn and add a link/reference to the upstream implementation and its RFC-0004 conformance so readers know where the actual algorithm lives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crypt/chachapoly/chachapoly.go`:
- Around line 9-16: The Encrypt and Decrypt functions return errors directly
from enchantrixchacha; change them to capture the returned error and, on error,
wrap it with coreerr.E using the specified identifiers and messages: for Encrypt
use coreerr.E("chachapoly.Encrypt", "lowercase message", err) and for Decrypt
use coreerr.E("chachapoly.Decrypt", "lowercase message", err); otherwise return
the successful result and nil. Ensure you reference the existing function names
Encrypt and Decrypt and the enchantrixchacha.Encrypt/enchantrixchacha.Decrypt
calls when implementing the changes.
In `@crypt/checksum.go`:
- Around line 29-34: The current file-hash helpers read the entire file into
memory (io.ReadAll) before hashing; change the reader-based variants (e.g.,
SHA256File and the other file helper around lines 52-57) to stream the file
through a hash.Hash instead of allocating a byte slice: open the file, create
the appropriate hash via stdcrypto.SHA256.New() (or the corresponding
algorithm), io.Copy the file into the hasher, then compute the hex string from
the sum (instead of calling enchantrixHexHash with a []byte). Ensure the
byte-slice helper can remain as-is, but the file-based helpers must use io.Copy
into the hasher to avoid O(file size) allocations.
In `@crypt/lthn/lthn.go`:
- Around line 50-51: The Hash wrapper in crypt/lthn/lthn.go now delegates to
enchantrixlthn.Hash(), which may change the hash output used by auth/auth.go for
persisted user IDs and key fingerprints; before merging, either (A) add a
golden-vector test set that verifies enchantrixlthn.Hash() produces
byte-for-byte identical outputs to the original LTHN algorithm for a
representative set of inputs (add vectors and tests asserting equality), or (B)
implement and document a migration/backfill strategy that updates existing
stored identifiers/fingerprints to the new format and add tests for that
migration path; ensure the Hash function, the new tests, and any migration
utility explicitly reference crypt/lthn.Hash and auth package usage so reviewers
can verify compatibility.
In `@crypt/rsa/rsa.go`:
- Around line 21-34: The three RSA wrapper methods (Service.GenerateKeyPair,
Service.Encrypt, Service.Decrypt) currently return raw errors from
s.enchantrix(); update each to wrap any non-nil error with coreerr.E(...)
supplying the method key and message (use "rsa.GenerateKeyPair", "failed to
generate key pair"; "rsa.Encrypt", "failed to encrypt"; "rsa.Decrypt", "failed
to decrypt") before returning, and add the coreerr import alongside the
enchantrixrsa import.
In `@crypt/symmetric.go`:
- Around line 20-28: Replace the raw errors returned by enchantrixchacha.Encrypt
and enchantrixchacha.Decrypt in ChaCha20Encrypt and ChaCha20Decrypt by wrapping
them with coreerr.E to match the AES helpers; e.g. call
enchantrixchacha.Encrypt(...) and if err != nil return nil,
coreerr.E("crypt.ChaCha20Encrypt", "encrypt failed", err), and similarly for
ChaCha20Decrypt use coreerr.E("crypt.ChaCha20Decrypt", "decrypt failed", err)
when returning errors from enchantrixchacha.Decrypt; keep the same return types
and behavior otherwise.
In `@go.mod`:
- Line 3: Remove all local module replace directives from go.mod (e.g., the
"replace dappco.re/go/cli => ../cli" block and the other replace entries
currently present) and instead declare those local replacements in the workspace
file (go.work) under the use directive; keep go 1.26.0 as-is, ensure go.mod
contains no replace lines, and add the corresponding module paths and local
directories to ~/Code/go.work so the build uses the local modules without
modifying go.mod.
- Around line 69-81: The go.mod currently contains a local workspace replace
"replace dappco.re/go/cli => ../cli" which violates the rule that local modules
must be wired via the workspace file; remove that local replace from go.mod and
add the equivalent local module mapping into the repository Go workspace (e.g.,
~/Code/go.work) instead, leaving the other external replaces (like
dappco.re/go/i18n, dappco.re/go/io, etc.) intact; ensure the module path
"dappco.re/go/cli" is referenced in the workspace file so builds still resolve
the local ../cli copy.
---
Nitpick comments:
In `@crypt/lthn/lthn.go`:
- Around line 22-23: Update the package comment in crypt/lthn/lthn.go to reflect
that this package is a compatibility wrapper that delegates hashing behavior to
the upstream implementation (enchantrixlthn from
forge.lthn.ai/Snider/Enchantrix/pkg/crypt/std/lthn) rather than implementing
RFC-0004 itself; explicitly state it forwards calls to enchantrixlthn and add a
link/reference to the upstream implementation and its RFC-0004 conformance so
readers know where the actual algorithm lives.
🪄 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: 48e28dff-451f-4450-9f56-72cd40e526b5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
crypt/chachapoly/chachapoly.gocrypt/checksum.gocrypt/lthn/lthn.gocrypt/lthn/lthn_test.gocrypt/rsa/rsa.gocrypt/symmetric.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- crypt/lthn/lthn_test.go
| func Encrypt(plaintext []byte, key []byte) ([]byte, error) { | ||
| aead, err := chacha20poly1305.NewX(key) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| nonce := make([]byte, aead.NonceSize(), aead.NonceSize()+len(plaintext)+aead.Overhead()) | ||
| if _, err := io.ReadFull(rand.Reader, nonce); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return aead.Seal(nonce, nonce, plaintext, nil), nil | ||
| return enchantrixchacha.Encrypt(plaintext, key) | ||
| } | ||
|
|
||
| // Decrypt decrypts data using ChaCha20-Poly1305. | ||
| // Usage: call Decrypt(...) during the package's normal workflow. | ||
| func Decrypt(ciphertext []byte, key []byte) ([]byte, error) { | ||
| aead, err := chacha20poly1305.NewX(key) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| minLen := aead.NonceSize() + aead.Overhead() | ||
| if len(ciphertext) < minLen { | ||
| return nil, coreerr.E("chachapoly.Decrypt", core.Sprintf("ciphertext too short: got %d bytes, need at least %d bytes", len(ciphertext), minLen), nil) | ||
| } | ||
|
|
||
| nonce, ciphertext := ciphertext[:aead.NonceSize()], ciphertext[aead.NonceSize():] | ||
|
|
||
| decrypted, err := aead.Open(nil, nonce, ciphertext, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if len(decrypted) == 0 { | ||
| return []byte{}, nil | ||
| } | ||
|
|
||
| return decrypted, nil | ||
| return enchantrixchacha.Decrypt(ciphertext, key) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file
fd -t f "chachapoly.go"Repository: dAppCore/go-crypt
Length of output: 92
🏁 Script executed:
# Search for coreerr.E usage pattern across Go files
rg "coreerr\.E\(" -t go -A 2 | head -50Repository: dAppCore/go-crypt
Length of output: 2120
🏁 Script executed:
# Check the actual content of the chachapoly.go file
cat -n crypt/chachapoly/chachapoly.goRepository: dAppCore/go-crypt
Length of output: 748
Wrap errors returned from this package's public API using coreerr.E(...) to maintain consistency.
Both Encrypt() and Decrypt() currently return errors directly from the enchantrixchacha library without wrapping them. Per coding guidelines for Go files, use coreerr.E("chachapoly.Encrypt", "lowercase message", err) and coreerr.E("chachapoly.Decrypt", "lowercase message", err) to wrap failures before returning them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypt/chachapoly/chachapoly.go` around lines 9 - 16, The Encrypt and Decrypt
functions return errors directly from enchantrixchacha; change them to capture
the returned error and, on error, wrap it with coreerr.E using the specified
identifiers and messages: for Encrypt use coreerr.E("chachapoly.Encrypt",
"lowercase message", err) and for Decrypt use coreerr.E("chachapoly.Decrypt",
"lowercase message", err); otherwise return the successful result and nil.
Ensure you reference the existing function names Encrypt and Decrypt and the
enchantrixchacha.Encrypt/enchantrixchacha.Decrypt calls when implementing the
changes.
| data, err := io.ReadAll(f) | ||
| if err != nil { | ||
| return "", coreerr.E("crypt.SHA256File", "failed to read file", err) | ||
| } | ||
|
|
||
| return hex.EncodeToString(h.Sum(nil)), nil | ||
| return enchantrixHexHash(stdcrypto.SHA256, data) |
There was a problem hiding this comment.
Keep file hashing streaming.
Line 29 and Line 52 now read the whole file into memory before hashing. cmd/crypt/cmd_checksum.go feeds arbitrary CLI paths into these helpers, so checksuming a large file becomes an O(file size) allocation and can exhaust the process on ordinary inputs. Please keep the file variants reader-based, even if the byte-slice helpers stay delegated.
Also applies to: 52-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypt/checksum.go` around lines 29 - 34, The current file-hash helpers read
the entire file into memory (io.ReadAll) before hashing; change the reader-based
variants (e.g., SHA256File and the other file helper around lines 52-57) to
stream the file through a hash.Hash instead of allocating a byte slice: open the
file, create the appropriate hash via stdcrypto.SHA256.New() (or the
corresponding algorithm), io.Copy the file into the hasher, then compute the hex
string from the sum (instead of calling enchantrixHexHash with a []byte). Ensure
the byte-slice helper can remain as-is, but the file-based helpers must use
io.Copy into the hasher to avoid O(file size) allocations.
| func Hash(input string) string { | ||
| salt := createSalt(input) | ||
| hash := sha256.Sum256([]byte(input + salt)) | ||
| return hex.EncodeToString(hash[:]) | ||
| } | ||
|
|
||
| // createSalt derives a quasi-salt by reversing the input and applying substitutions. | ||
| // For example: "hello" -> reversed "olleh" -> substituted "011eh" | ||
| func createSalt(input string) string { | ||
| if input == "" { | ||
| return "" | ||
| } | ||
| runes := []rune(input) | ||
| salt := make([]rune, len(runes)) | ||
| for i := range runes { | ||
| char := runes[len(runes)-1-i] | ||
| if replacement, ok := keyMap[char]; ok { | ||
| salt[i] = replacement | ||
| } else { | ||
| salt[i] = char | ||
| } | ||
| } | ||
| return string(salt) | ||
| return enchantrixlthn.Hash(input) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if auth/auth.go exists and examine the cited lines
fd -t f auth.goRepository: dAppCore/go-crypt
Length of output: 74
🏁 Script executed:
# Examine crypt/lthn/lthn.go to confirm the hash backend delegation
fd -t f lthn.go | grep -E "crypt.*lthn"Repository: dAppCore/go-crypt
Length of output: 80
🏁 Script executed:
# Search for references to lthn.Hash to understand usage patterns
rg "lthn\.Hash" --type go -B 2 -A 2Repository: dAppCore/go-crypt
Length of output: 12483
🏁 Script executed:
# Search for the old Hash implementation or any historical hash logic
rg "func Hash" --type go -B 2 -A 5 | head -40Repository: dAppCore/go-crypt
Length of output: 1517
🏁 Script executed:
# Check for any golden vectors or test data files
fd -t f \( -name "*golden*" -o -name "*fixture*" -o -name "*vector*" \) --ignore-caseRepository: dAppCore/go-crypt
Length of output: 292
🏁 Script executed:
# Search for migration or compatibility-related comments
rg "compat|migrat|golden|vector|sha.*256" --type go -iRepository: dAppCore/go-crypt
Length of output: 8911
🏁 Script executed:
# Look for the old LTHN algorithm or any backup/fallback logic
rg "enchantrix|lthn" --type go | grep -i "import\|package\|const\|var" | head -20Repository: dAppCore/go-crypt
Length of output: 311
🏁 Script executed:
# Check the full crypt/lthn/lthn.go file to see if old implementation is still there
cat -n crypt/lthn/lthn.goRepository: dAppCore/go-crypt
Length of output: 2765
🏁 Script executed:
# Check if there are any tests comparing old vs new hash output or golden vectors
rg "golden|vector|compat|enchantrix.*Hash" --type go -A 3Repository: dAppCore/go-crypt
Length of output: 6074
🏁 Script executed:
# Search for any version pinning or documentation of enchantrix in go.mod or comments
fd -t f go.modRepository: dAppCore/go-crypt
Length of output: 68
🏁 Script executed:
# Look for any comments or TODOs warning about hash compatibility or migration
rg "TODO|FIXME|WARNING|compat|migrat" crypt/lthn/ --type goRepository: dAppCore/go-crypt
Length of output: 43
🏁 Script executed:
# Check if there's a test file specifically for LTHN that shows hash consistency
cat crypt/lthn/lthn_test.go 2>/dev/null | head -100Repository: dAppCore/go-crypt
Length of output: 779
Establish hash backend compatibility before merging.
auth/auth.go persists lthn.Hash() output for user identifiers (line 179) and key fingerprints (line 226). Delegating to enchantrixlthn.Hash() changes the underlying hash algorithm; without byte-for-byte compatibility verification, this breaks all stored user IDs and existing fingerprints. The current test suite only validates output shape (64 hex chars), not compatibility with previously persisted data.
Add either:
- A pre-migration golden vector set demonstrating
enchantrixlthn.Hash()matches the original LTHN algorithm, or - A clear migration/backfill strategy for existing user records
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypt/lthn/lthn.go` around lines 50 - 51, The Hash wrapper in
crypt/lthn/lthn.go now delegates to enchantrixlthn.Hash(), which may change the
hash output used by auth/auth.go for persisted user IDs and key fingerprints;
before merging, either (A) add a golden-vector test set that verifies
enchantrixlthn.Hash() produces byte-for-byte identical outputs to the original
LTHN algorithm for a representative set of inputs (add vectors and tests
asserting equality), or (B) implement and document a migration/backfill strategy
that updates existing stored identifiers/fingerprints to the new format and add
tests for that migration path; ensure the Hash function, the new tests, and any
migration utility explicitly reference crypt/lthn.Hash and auth package usage so
reviewers can verify compatibility.
| func (s *Service) GenerateKeyPair(bits int) (publicKey, privateKey []byte, err error) { | ||
| const op = "rsa.GenerateKeyPair" | ||
|
|
||
| if bits < 2048 { | ||
| return nil, nil, coreerr.E(op, core.Sprintf("key size too small: %d (minimum 2048)", bits), nil) | ||
| } | ||
| privKey, err := rsa.GenerateKey(rand.Reader, bits) | ||
| if err != nil { | ||
| return nil, nil, coreerr.E(op, "failed to generate private key", err) | ||
| } | ||
|
|
||
| privKeyBytes := x509.MarshalPKCS1PrivateKey(privKey) | ||
| privKeyPEM := pem.EncodeToMemory(&pem.Block{ | ||
| Type: "RSA PRIVATE KEY", | ||
| Bytes: privKeyBytes, | ||
| }) | ||
|
|
||
| pubKeyBytes, err := x509.MarshalPKIXPublicKey(&privKey.PublicKey) | ||
| if err != nil { | ||
| return nil, nil, coreerr.E(op, "failed to marshal public key", err) | ||
| } | ||
| pubKeyPEM := pem.EncodeToMemory(&pem.Block{ | ||
| Type: "PUBLIC KEY", | ||
| Bytes: pubKeyBytes, | ||
| }) | ||
|
|
||
| return pubKeyPEM, privKeyPEM, nil | ||
| return s.enchantrix().GenerateKeyPair(bits) | ||
| } | ||
|
|
||
| // Encrypt encrypts data with a public key. | ||
| // Usage: call Encrypt(...) during the package's normal workflow. | ||
| func (s *Service) Encrypt(publicKey, data, label []byte) ([]byte, error) { | ||
| const op = "rsa.Encrypt" | ||
|
|
||
| block, _ := pem.Decode(publicKey) | ||
| if block == nil { | ||
| return nil, coreerr.E(op, "failed to decode public key", nil) | ||
| } | ||
|
|
||
| pub, err := x509.ParsePKIXPublicKey(block.Bytes) | ||
| if err != nil { | ||
| return nil, coreerr.E(op, "failed to parse public key", err) | ||
| } | ||
|
|
||
| rsaPub, ok := pub.(*rsa.PublicKey) | ||
| if !ok { | ||
| return nil, coreerr.E(op, "not an RSA public key", nil) | ||
| } | ||
|
|
||
| ciphertext, err := rsa.EncryptOAEP(sha256.New(), rand.Reader, rsaPub, data, label) | ||
| if err != nil { | ||
| return nil, coreerr.E(op, "failed to encrypt data", err) | ||
| } | ||
|
|
||
| return ciphertext, nil | ||
| return s.enchantrix().Encrypt(publicKey, data, label) | ||
| } | ||
|
|
||
| // Decrypt decrypts data with a private key. | ||
| // Usage: call Decrypt(...) during the package's normal workflow. | ||
| func (s *Service) Decrypt(privateKey, ciphertext, label []byte) ([]byte, error) { | ||
| const op = "rsa.Decrypt" | ||
|
|
||
| block, _ := pem.Decode(privateKey) | ||
| if block == nil { | ||
| return nil, coreerr.E(op, "failed to decode private key", nil) | ||
| } | ||
|
|
||
| priv, err := x509.ParsePKCS1PrivateKey(block.Bytes) | ||
| if err != nil { | ||
| return nil, coreerr.E(op, "failed to parse private key", err) | ||
| } | ||
| return s.enchantrix().Decrypt(privateKey, ciphertext, label) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the RSA service file
fd -e go crypt/rsa/rsa.goRepository: dAppCore/go-crypt
Length of output: 43
🏁 Script executed:
# Read the full context of the file to understand the error handling
cat -n crypt/rsa/rsa.goRepository: dAppCore/go-crypt
Length of output: 1759
🏁 Script executed:
# Search for coreerr usage patterns in the codebase
rg 'coreerr\.E\(' -A 2 --max-count=5Repository: dAppCore/go-crypt
Length of output: 9360
🏁 Script executed:
# Check imports in the rsa.go file to see what error handling is available
head -30 crypt/rsa/rsa.goRepository: dAppCore/go-crypt
Length of output: 1061
🏁 Script executed:
# Look for enchantrix type definition and its error returns
fd -e go enchantrixRepository: dAppCore/go-crypt
Length of output: 43
Wrap delegated RSA failures with coreerr.E() before returning them.
These three methods return raw Enchantrix errors, losing the package/method context that the rest of the module establishes via coreerr.E(...). This breaks consistency at the package boundary and violates the repository's error-handling rule.
Following the established pattern elsewhere in the codebase (e.g. crypt.Encrypt, crypt.Decrypt), wrap each error with context:
coreerr.E("rsa.GenerateKeyPair", "failed to generate key pair", err)
coreerr.E("rsa.Encrypt", "failed to encrypt", err)
coreerr.E("rsa.Decrypt", "failed to decrypt", err)Ensure coreerr is imported alongside the existing enchantrixrsa import.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypt/rsa/rsa.go` around lines 21 - 34, The three RSA wrapper methods
(Service.GenerateKeyPair, Service.Encrypt, Service.Decrypt) currently return raw
errors from s.enchantrix(); update each to wrap any non-nil error with
coreerr.E(...) supplying the method key and message (use "rsa.GenerateKeyPair",
"failed to generate key pair"; "rsa.Encrypt", "failed to encrypt";
"rsa.Decrypt", "failed to decrypt") before returning, and add the coreerr import
alongside the enchantrixrsa import.
| func ChaCha20Encrypt(plaintext, key []byte) ([]byte, error) { | ||
| aead, err := chacha20poly1305.NewX(key) | ||
| if err != nil { | ||
| return nil, coreerr.E("crypt.ChaCha20Encrypt", "failed to create cipher", err) | ||
| } | ||
|
|
||
| nonce := make([]byte, aead.NonceSize()) | ||
| if _, err := rand.Read(nonce); err != nil { | ||
| return nil, coreerr.E("crypt.ChaCha20Encrypt", "failed to generate nonce", err) | ||
| } | ||
|
|
||
| ciphertext := aead.Seal(nonce, nonce, plaintext, nil) | ||
| return ciphertext, nil | ||
| return enchantrixchacha.Encrypt(plaintext, key) | ||
| } | ||
|
|
||
| // ChaCha20Decrypt decrypts ciphertext encrypted with ChaCha20Encrypt. | ||
| // The key must be 32 bytes. Expects the nonce prepended to the ciphertext. | ||
| // Usage: call ChaCha20Decrypt(...) during the package's normal workflow. | ||
| func ChaCha20Decrypt(ciphertext, key []byte) ([]byte, error) { | ||
| aead, err := chacha20poly1305.NewX(key) | ||
| if err != nil { | ||
| return nil, coreerr.E("crypt.ChaCha20Decrypt", "failed to create cipher", err) | ||
| } | ||
|
|
||
| nonceSize := aead.NonceSize() | ||
| if len(ciphertext) < nonceSize { | ||
| return nil, coreerr.E("crypt.ChaCha20Decrypt", "ciphertext too short", nil) | ||
| } | ||
|
|
||
| nonce, encrypted := ciphertext[:nonceSize], ciphertext[nonceSize:] | ||
| plaintext, err := aead.Open(nil, nonce, encrypted, nil) | ||
| if err != nil { | ||
| return nil, coreerr.E("crypt.ChaCha20Decrypt", "failed to decrypt", err) | ||
| } | ||
|
|
||
| return plaintext, nil | ||
| return enchantrixchacha.Decrypt(ciphertext, key) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "symmetric.go" -path "*/crypt/*"Repository: dAppCore/go-crypt
Length of output: 82
🏁 Script executed:
cat -n crypt/symmetric.goRepository: dAppCore/go-crypt
Length of output: 3574
Wrap ChaCha20Encrypt and ChaCha20Decrypt errors with coreerr.E.
Lines 21 and 28 return raw Enchantrix errors. The AES helper functions in this same file consistently wrap errors with coreerr.E("crypt.FunctionName", "lowercase message", err). This inconsistency violates the coding guideline and weakens operational diagnostics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crypt/symmetric.go` around lines 20 - 28, Replace the raw errors returned by
enchantrixchacha.Encrypt and enchantrixchacha.Decrypt in ChaCha20Encrypt and
ChaCha20Decrypt by wrapping them with coreerr.E to match the AES helpers; e.g.
call enchantrixchacha.Encrypt(...) and if err != nil return nil,
coreerr.E("crypt.ChaCha20Encrypt", "encrypt failed", err), and similarly for
ChaCha20Decrypt use coreerr.E("crypt.ChaCha20Decrypt", "decrypt failed", err)
when returning errors from enchantrixchacha.Decrypt; keep the same return types
and behavior otherwise.
| module dappco.re/go/core/crypt | ||
| module dappco.re/go/crypt | ||
|
|
||
| go 1.26.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E 'go\.mod|go\.work'Repository: dAppCore/go-crypt
Length of output: 68
🏁 Script executed:
cat -n go.modRepository: dAppCore/go-crypt
Length of output: 3828
🌐 Web query:
Go 1.26.0 release date version
💡 Result:
Go 1.26.0 release date: 2026-02-10. Evidence: Go’s official release notes and the Go release history both state that go1.26.0 was released on 2026-02-10.
Citations:
Remove replace directives from go.mod and use the workspace file instead.
The replace directives on lines 69–81 violate the coding guideline. Replace replace dappco.re/go/cli => ../cli and other local dependency replacements by configuring them in the workspace file (~/Code/go.work) instead. Go 1.26.0 is a released version (February 2026) and is appropriate for use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 3, Remove all local module replace directives from go.mod
(e.g., the "replace dappco.re/go/cli => ../cli" block and the other replace
entries currently present) and instead declare those local replacements in the
workspace file (go.work) under the use directive; keep go 1.26.0 as-is, ensure
go.mod contains no replace lines, and add the corresponding module paths and
local directories to ~/Code/go.work so the build uses the local modules without
modifying go.mod.
| replace dappco.re/go/cli => ../cli | ||
|
|
||
| replace dappco.re/go/i18n => github.com/dappcore/go-i18n v0.8.0-alpha.1 | ||
|
|
||
| replace dappco.re/go/io => github.com/dappcore/go-io v0.8.0-alpha.1 | ||
|
|
||
| replace dappco.re/go/log => github.com/dappcore/go-log v0.8.0-alpha.1 | ||
|
|
||
| replace dappco.re/go/process => github.com/dappcore/go-process v0.8.0-alpha.1 | ||
|
|
||
| replace dappco.re/go/store => github.com/dappcore/go-store v0.8.0-alpha.1 | ||
|
|
||
| replace dappco.re/go/inference => github.com/dappcore/go-inference v0.8.0-alpha.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the go.mod file
fd -type f -name "go.mod" | head -20Repository: dAppCore/go-crypt
Length of output: 232
🏁 Script executed:
# Check if go.work exists
fd -type f -name "go.work"Repository: dAppCore/go-crypt
Length of output: 232
🏁 Script executed:
# Once we find go.mod, let's read lines 69-81 to verify the replacements
if [ -f "go.mod" ]; then
echo "=== go.mod lines 69-81 ===" && sed -n '69,81p' go.mod
echo -e "\n=== Total lines in go.mod ===" && wc -l go.mod
echo -e "\n=== Full go.mod for context ===" && cat -n go.mod
fiRepository: dAppCore/go-crypt
Length of output: 4414
🏁 Script executed:
# Search for go.work using find instead
find . -name "go.work" -type f 2>/dev/null
# Also check the parent directory structure to understand the workspace layout
ls -la ../ 2>/dev/null | head -20Repository: dAppCore/go-crypt
Length of output: 760
🏁 Script executed:
# Check if there are any other go.mod files in parent/sibling directories
find .. -maxdepth 2 -name "go.mod" -o -name "go.work" 2>/dev/null | head -20Repository: dAppCore/go-crypt
Length of output: 75
🏁 Script executed:
# Look for any workspace-related comments or documentation
find . -maxdepth 2 -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "go.work\|workspace" {} \; 2>/dev/nullRepository: dAppCore/go-crypt
Length of output: 118
🏁 Script executed:
# Check development documentation
cat ./docs/development.mdRepository: dAppCore/go-crypt
Length of output: 7852
🏁 Script executed:
# Check CLAUDE.md for setup instructions
cat ./CLAUDE.mdRepository: dAppCore/go-crypt
Length of output: 3485
Move the local dappco.re/go/cli replacement to the workspace file.
The replace dappco.re/go/cli => ../cli directive hard-codes a sibling path and violates the repository rule: local dependencies must be wired through ~/Code/go.work, not go.mod. Per the coding guidelines and project documentation, replace directives in go.mod should be reserved for external version mappings only.
Diff
-replace dappco.re/go/cli => ../cli
-
replace dappco.re/go/i18n => github.com/dappcore/go-i18n v0.8.0-alpha.1
replace dappco.re/go/io => github.com/dappcore/go-io v0.8.0-alpha.1📝 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.
| replace dappco.re/go/cli => ../cli | |
| replace dappco.re/go/i18n => github.com/dappcore/go-i18n v0.8.0-alpha.1 | |
| replace dappco.re/go/io => github.com/dappcore/go-io v0.8.0-alpha.1 | |
| replace dappco.re/go/log => github.com/dappcore/go-log v0.8.0-alpha.1 | |
| replace dappco.re/go/process => github.com/dappcore/go-process v0.8.0-alpha.1 | |
| replace dappco.re/go/store => github.com/dappcore/go-store v0.8.0-alpha.1 | |
| replace dappco.re/go/inference => github.com/dappcore/go-inference v0.8.0-alpha.1 | |
| replace dappco.re/go/i18n => github.com/dappcore/go-i18n v0.8.0-alpha.1 | |
| replace dappco.re/go/io => github.com/dappcore/go-io v0.8.0-alpha.1 | |
| replace dappco.re/go/log => github.com/dappcore/go-log v0.8.0-alpha.1 | |
| replace dappco.re/go/process => github.com/dappcore/go-process v0.8.0-alpha.1 | |
| replace dappco.re/go/store => github.com/dappcore/go-store v0.8.0-alpha.1 | |
| replace dappco.re/go/inference => github.com/dappcore/go-inference v0.8.0-alpha.1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 69 - 81, The go.mod currently contains a local workspace
replace "replace dappco.re/go/cli => ../cli" which violates the rule that local
modules must be wired via the workspace file; remove that local replace from
go.mod and add the equivalent local module mapping into the repository Go
workspace (e.g., ~/Code/go.work) instead, leaving the other external replaces
(like dappco.re/go/i18n, dappco.re/go/io, etc.) intact; ensure the module path
"dappco.re/go/cli" is referenced in the workspace file so builds still resolve
the local ../cli copy.
2 participants
Routine dev→main PR opened by Hephaestus PR-cadence task.
This PR exists to:
ahead_by: 5 commit(s) (per gh api compare)If CodeRabbit clears with no blocking comments, this PR is eligible for
gh pr merge --merge(real merge commit, no force-push). Conflicts and review feedback should be addressed on the dev branch before merge.Co-authored-by: Hephaestus hephaestus@cladius
Summary by CodeRabbit
Bug Fixes
Tests