Skip to content

chore: untrack node_modules, repair the lockfile, and add the first CI - #10

Merged
DealAppSeo merged 2 commits into
mainfrom
fix/cc-2026-08-05-untrack-node-modules
Aug 6, 2026
Merged

chore: untrack node_modules, repair the lockfile, and add the first CI#10
DealAppSeo merged 2 commits into
mainfrom
fix/cc-2026-08-05-untrack-node-modules

Conversation

@DealAppSeo

Copy link
Copy Markdown
Owner

The headline: a clean clone of this repo could not install

npm ci failed at the root and in packages/contracts — both lockfiles were out of sync with their package.json, and the root lockfile predated the workspaces entirely (0 references to packages/).

Committing node_modules was the workaround. That is why 4,526 of 4,617 tracked files — 98% of the repo — were vendored dependencies.

This is the first thing an external reviewer does, so it is the first thing to fix.

What changed

.gitignore did not exist at all — the root cause
node_modules untracked (4,526 files). Files stay on disk, history is not rewritten. Both package-lock.json files retained — they are the reproducible record
package-lock.json regenerated. npm ci now exits 0 and the lock finally knows about the workspaces
.github/workflows/ci.yml first CI in the repo

Verified, not assumed:

  • npm ci at root → RC 0 (was EUSAGE, could not install)
  • tracked files → 4,617 → 91
  • both lockfiles → still tracked
  • packages/contracts sources (contracts/, test/, abis/, ERC8004SPEC.md) → untouched

What I found and deliberately did not fix

The contract tests fail 26 of 61:

Event "NewFeedback" with argument count 11 not found in the contract ABI

This pre-dates this change, and I verified that rather than claiming it — restoring the original committed node_modules from origin/main and re-running produces the identical 35 pass / 26 fail with the same error. The lockfile regeneration did not cause it. packages/contracts is owned elsewhere, so it is surfaced here and left alone.

Separately, hardhat.config.ts rejects its own config (HHE15) when SEPOLIA_RPC_URL / MAINNET_RPC_URL are unset — before a single test runs. A reviewer cloning this repo sees a red suite that looks like broken contracts and is actually unset env. CI sets public read-only endpoints so an ENV failure can never be mistaken for a TEST failure.

The CI is deliberately not all-green

Four of five packages have no tests. A single green tick across the repo would claim coverage that does not exist.

  • install — blocking. It is the thing that was actually broken. Also guards that node_modules is never committed again.
  • contracts — reports its true tally, does not gate, warns only if failures exceed the 26 baseline.
  • coverage-map — prints per-package test presence so "none" is visible rather than implied.

An unwired safeguard is worse than an absent one: it converts a known gap into false coverage. Same is true of a package that silently has no tests.

Review note

The diff is ~4,500 files because that is how many vendored files were tracked. The reviewable surface is three files: .gitignore, .github/workflows/ci.yml, and package-lock.json.

🤖 Generated with Claude Code

A clean clone of this repository could not install. `npm ci` failed at the root
AND in packages/contracts — the lockfiles were out of sync with their
package.json files, and the root lockfile predated the workspaces entirely (0
references to packages/). Committing node_modules was the workaround, and it is
why 4,526 of 4,617 tracked files — 98% of the repo — were vendored dependencies.

This removes the workaround and fixes the cause:

  .gitignore        did not exist at all. That is the root cause.
  node_modules      untracked (4,526 files). Files stay on disk; history is not
                    rewritten. The two package-lock.json files are retained --
                    they are the reproducible record.
  package-lock.json regenerated. `npm ci` now exits 0 and the lock finally knows
                    about the workspaces.

VERIFIED, not assumed:
  npm ci at root                -> RC 0 (was EUSAGE, could not install)
  tracked files                 -> 4,617 -> 91
  both lockfiles                -> still tracked
  packages/contracts sources    -> untouched (contracts/, test/, abis/, spec)

WHAT I FOUND AND DID NOT FIX. The contract tests fail 26 of 61 on a drift
between the tests and the compiled ABI:

  Event "NewFeedback" with argument count 11 not found in the contract ABI

This PRE-DATES this change. I verified that rather than asserting it: restoring
the ORIGINAL committed node_modules from origin/main and re-running produces the
identical 35 pass / 26 fail with the same error, so the lockfile regeneration did
not cause it. packages/contracts is owned elsewhere, so it is surfaced here and
left alone.

Separately, `hardhat.config.ts` rejects its own config (HHE15) when
SEPOLIA_RPC_URL / MAINNET_RPC_URL are unset — before a single test runs. A
reviewer cloning this repo sees a red suite that looks like broken contracts and
is actually unset env. CI sets public read-only endpoints so an ENV failure can
never be mistaken for a TEST failure.

THE CI IS DELIBERATELY NOT ALL-GREEN. Four of five packages have no tests; a
single green tick would claim coverage that does not exist. So: `install` blocks
(it is the thing that was actually broken, plus a guard that fails if anyone
commits node_modules again), the contract suite reports its true tally without
gating and warns only if failures exceed the 26 baseline, and `coverage-map`
prints per-package test presence so "none" is visible rather than implied.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@DealAppSeo

Copy link
Copy Markdown
Owner Author

Diagnosis: the 26 failing tests are correct. The contract broke ERC-8004 conformance.

Chasing Event "NewFeedback" with argument count 11 not found in the contract ABI to its root. The tests are right and the implementation is wrong — this is a spec divergence, not a stale test.

Three sources, two signatures

source args ends with
ERC8004SPEC.md:224 (canonical) 11 bytes32 feedbackHash
abis/ReputationRegistry.json (checked-in) 11 bytes32 feedbackHash
contracts/ReputationRegistryUpgradeable.sol:15 12 bytes32 feedbackHash, bytes x402PaymentProof

The Solidity source appends a 12th parameter — bytes x402PaymentProof — that appears in neither the spec nor the published ABI. The spec (line 224) is explicit:

event NewFeedback(uint256 indexed agentId, address indexed clientAddress, uint64 feedbackIndex,
  int128 value, uint8 valueDecimals, string indexed indexedTag1, string tag1, string tag2,
  string endpoint, string feedbackURI, bytes32 feedbackHash)

Why this is a conformance break, not a cosmetic mismatch

Adding a parameter changes the event signature, which changes topic0 (truncated; recompute with keccak256 of the signature strings above):

canonical (spec + abis/):  0x6a4a617435...
deployed  (.sol source):   0x58819acf68...

Different topics. Any ERC-8004-compliant indexer filtering on the canonical topic0 sees zero feedback events from this contract. The spec's own stated goal for this event — "exposes reputation signals to any smart contract, enabling on-chain composability" — does not hold as deployed. Third-party consumers silently observe an empty registry rather than an error.

Scope

All 26 failures are in the ReputationRegistry suite of test/core.ts (61 tests total: 35 pass, 26 fail). Every one traces to this single signature divergence — it is one defect, not 26.

Pre-existing. Verified by restoring the original committed node_modules from origin/main and re-running: identical 35/26 with the same error. The lockfile repair in this PR did not cause it.

Options — owner's call, not touched here

  1. Move the extension out. Keep NewFeedback byte-exact to spec; emit NewFeedbackPaymentProof(uint256 indexed agentId, address indexed clientAddress, uint64 feedbackIndex, bytes x402PaymentProof) alongside it. Restores conformance, keeps the x402 data, preserves indexed correlation. Cleanest of the three.
  2. Dual-emit the canonical event plus the extended one. Conformant but pays double log gas on every feedback.
  3. Accept the divergence and document it as a deliberate non-standard extension — which means dropping the ERC-8004 composability claim for this event.

I have not modified anything under packages/contracts. This is diagnosis only, so the decision sits with whoever owns the spec.

…not determine

A 12-agent audit of this repo aimed at exactly one failure shape — a component
reporting success on a question it never had the means to answer — turned that
lens on the job added earlier in this PR to defend against it, and it did not
survive.

`for p in packages/*/` globs ONE level. The only two real packages live one
level deeper (packages/defaults/hallucination-hal-local, .../identity-erc8004-viem),
so neither ever got a row. Worse, `packages/defaults` has no package.json, so it
was reported as test-script "**none**" while the `find` on the next line DID
recurse and credited it with 5 test files — a directory with no manifest
appearing to have tests. The job whose stated purpose is making absent tests
visible was manufacturing a coverage claim it had no means to determine.

Now enumerated by MANIFEST rather than by directory depth: `git ls-files
'*package.json'`, which cannot drift as the tree is reorganised.

And a second column the audit earned. A declared test script proves nothing if
its glob matches no file — hallucination-hal-local declares
`node --test --import tsx tests/*.test.ts` and ships only .mjs smoke files, so
it exits 0 over an empty set (reproduced on node v22.17.0: `# tests 0 / # pass 0
/ # fail 0`, exit 0). The table now resolves the script's own glob and prints
"**0 — greens over an empty set**" when it matches nothing. Verified locally:

  | contracts                        | npm run test:core && ... | n/a                        |
  | defaults/hallucination-hal-local | node --test ... *.test.ts | 0 - greens over empty set |
  | defaults/identity-erc8004-viem   | none                      | -                          |

Also: root package.json declared "license": "MIT" while LICENSE is the complete
Apache-2.0 (patent grant at S3 intact, real holder line) and README states
"Apache 2.0 - see LICENSE. Patent rights, if any, are granted under the Apache
2.0 patent grant clause" - a guarantee MIT cannot support, since MIT conveys no
express patent grant. package-lock.json had replicated the wrong value into a
second machine-read artefact. Corrected to Apache-2.0, matching LICENSE, README,
CONTRIBUTING and packages/defaults/identity-erc8004-viem.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@DealAppSeo
DealAppSeo merged commit 1202809 into main Aug 6, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant