Skip to content

methodology.md

Latest commit

 

History

History
202 lines (158 loc) · 12.5 KB

methodology.md

File metadata and controls

202 lines (158 loc) · 12.5 KB
Raw

Eval methodology & run log

The skill is tested TDD-style (RED → GREEN → REFACTOR) against OpenZeppelin, a quality bar the user trusts. OZ is the reference answer; it is not treated as 100% ideal — where our output diverges, the diff is discussed and may favor ours.

Procedure

  1. Clone OZ v5.6.1 into .baseline/openzeppelin-v5.6.1/ (gitignored).
  2. Pick a file. Read its existing comments + code and distill an implementation-context note (work/<File>.context.md) — the factual knowledge the author has, NOT the polished comments to copy. This mimics a real session where the agent knows the code it wrote.
  3. Copy the file to work/<File>.stripped.sol and remove all comments (keep SPDX + code).
  4. A sub-agent re-writes the comments from the stripped file + context note.
    • RED: sub-agent runs without the skill (it still inherits the user's global CLAUDE.md — this is the realistic "current behavior" baseline). Output → work/<File>.red.sol.
    • GREEN: sub-agent runs with the skill. Output → work/<File>.green.sol.
  5. Compare each output to the OZ original. Record meaningful diffs below.

Run 1 — utils/structs/MerkleTree.sol

Context: work/MerkleTree.context.md. Stripped: work/MerkleTree.stripped.sol.

RED (no skill) — work/MerkleTree.red.sol

Reproduced the user's complaints. Dominant failure = inline over-expansion; line count was close to the original (267 → 275) but the distribution was wrong.

# Failure OZ original RED baseline
1 Inline over-expansion (main pain) terse step labels: // Cache read, // Get leaf index, // Check if tree is full., // Rebuild branch from leaf to root 1–3 sentence paragraphs per step; commented the update loop body that OZ leaves mostly bare
2 Invented / wrong metadata // ... (last updated v5.5.0) ... re-added a version-stamp line and guessed it wrong (v5.1.0)
3 One-line doc inflated to a block /// @dev Error emitted when trying to update a leaf that was not previously pushed. expanded into a multi-line /** */ block with extra prose
4 Restructure + duplicate IMPORTANT compact NatSpec added a redundant "called only once" IMPORTANT duplicating the adjacent reset note
5 Lost load-bearing doc struct NOTE (root/history not stored) + WARNING dropped/merged some of it

Correctly preserved (skill must NOT break these): the guard comments // Workaround stack too deep and // ... cannot overflow because: 0 <= index < length.

Conclusion → the GREEN skill must target: terse inline labels (not paragraphs); never invent version/metadata stamps; /// for single-line docs; don't restructure or pad existing completeness; keep load-bearing guard comments.

GREEN (with skill) — work/MerkleTree.green.sol

Big improvement; the dominant failure is gone. Metrics (lower = terser):

metric OZ original RED (no skill) GREEN (skill)
total lines 267 275 221
comment lines 127 134 81
inline-body // 18 31 13

What the skill fixed vs RED:

  • Inline terse — GREEN reproduced OZ's short labels verbatim (// Cache read, // Get leaf index, // Check if tree is full., // Rebuild branch from leaf to root) and kept both guard comments. RED's paragraphs are gone.
  • No invented metadata — GREEN omitted the version stamp RED hallucinated.
  • Form — one-line errors stayed ///; multi-line docs stayed /** */.
  • API docs complete-but-terse — documented the surprising reverts, the same-fnHash precondition, the front-running WARNING, and the caller's oldRoot duty.

GREEN vs OZ (OZ not treated as ideal):

  • GREEN is terser than OZ and still conveys enough — matches the user's terse-adequate target.
  • GREEN documents push's full-tree Panic(RESOURCE_ERROR) revert, which the OZ NatSpec omits — arguably better (a surprising revert worth stating).
  • Borderline: one 2-sentence inline in update (frontier sibling) where the skill says "a second sentence is a red flag". It explains a genuinely non-obvious step OZ left bare — keep but consider allowing a single compact sentence for a real non-obvious why.
  • Minor nit: GREEN invented an xref anchor format {setup-Bytes32PushTree-uint8-bytes32-...} (non-standard). Candidate hygiene rule: don't guess doc-tooling anchor syntax; use a plain {symbol} or prose when unsure.

Verdict: skill works on run 1. Candidate REFACTOR tweaks above are minor.

REFACTOR (skill v2) — work/MerkleTree.green2.sol

Added three rules from user feedback and re-ran the same file. All three landed:

  1. Lists over prose — the header now uses OZ's * Depth: … * Zero value: … * Hashing function: … list instead of the v1 prose paragraph.
  2. Backtick references, no invented anchors — `push()`/`update()`; overloads use the signature form `setup(Bytes32PushTree storage, uint8, bytes32, function)`. The hallucinated {setup-Type-uint8-...} anchor is gone.
  3. Plain language — the update inline no longer says "frontier": "When this node is the last filled one at its level, its sibling is the stored side, not a proof element: …". Clear, if still ~3 lines (acceptable — a genuine non-obvious why OZ left bare).

All v1 wins retained (terse inline labels, guards, no metadata, /// one-liners, complete-but-terse API docs incl. the push full-tree revert OZ omits). Run 1 passes; skill is in good shape.

Later finding (scope): green2's update carried WARNING: This is front-running sensitive ... — a bare security label with no mechanism (the user couldn't tell the author understood Solidity's single-thread model; fnHash is view). The front-running risk is real (mempool tx-ordering), but coining/elaborating security warnings is not the authoring skill's job — it belongs to the review skill's vulnerability track, which must explain the mechanism, not just label it. Added a "Don't invent security analysis" rule to SKILL.md Hygiene; noted for the review session's MEMORY.

Run 2 — access/AccessControl.sol

Hardening on a different profile: header with usage examples + WARNING, an interface-implementing surface, "Requirements/emits" API blocks. Skill run only (RED skipped — failure modes already documented in run 1). Context: work/AccessControl.context.md. Output: work/AccessControl.green.sol.

metric OZ original GREEN (skill)
total lines 207 162
comment lines 121 76
@inheritdoc 1 6
  • Header reproduced faithfully — usage examples, the role-id keccak256 snippet, and the DEFAULT_ADMIN_ROLE WARNING all kept. No invented metadata.
  • Internal functions (_checkRole, _setRoleAdmin, _grantRole, _revokeRole) documented terse, like OZ (they are not on the interface).
  • Deliberate divergence (arguably better than OZ): the 5 external functions use @inheritdoc IAccessControl instead of OZ's re-documented "Requirements/emits" blocks. Verified lossless — IAccessControl fully documents those functions (Requirements + emits), so @inheritdoc drops nothing and removes the duplication/drift risk. Leaner by 45 lines.
  • Surfaced caveat → skill updated: @inheritdoc is only safe over a complete base doc; over a thin interface it silently drops caller info. Added that caveat to comment-types.md.
  • Minor: reference style was {Symbol} here (matches OZ docgen house style) vs backticks in run 1 — the agent followed each file's apparent convention, but the flip was memory-driven. The skill rule (follow the project's docgen convention, else backticks) is right; detection should come from surrounding files and stay consistent within a project.

Run 3 — regression re-run of both files (current skill)

Re-ran MerkleTree.sol and AccessControl.sol with the refined skill. Outputs: work/MerkleTree.green3.sol, work/AccessControl.green2.sol (initial), then work/AccessControl.green3.sol (after the @inheritdoc fix below).

MerkleTree (green3) — clean, plus the front-running fix. The bare WARNING: front-running sensitive label is gone; replaced by a factual precondition + concrete mechanism: "The caller is responsible for verifying that oldRoot equals the last known root … Because the proof must match the tree's current state, any push/update that changes the root invalidates an in-flight update proof." The "Don't invent security analysis" rule worked. All other properties retained (list header, backtick refs, /// one-liners, guards, terse inline).

AccessControl (green2) — surfaced a defect → skill fix. The agent produced an @inheritdoc IAccessControl + appended @dev Requirements: hybrid on grantRole/revokeRole/ renounceRole — tool-dependent, and it re-duplicated the requirement + emitted-event that the interface already documents. Root cause: the old "inherit only what's complete" caveat pushed the agent to verify the interface and patch the gap inline.

Fixes applied:

  • comment-types.md — @inheritdoc is all-or-nothing; in passive mode do not audit the base's completeness (avoids cross-file churn). Base has a doc → inherit; none → write own; thin → leave for review.
  • active-mode.md — review is where @inheritdoc completeness is checked; the fix goes into the interface/base, never an @inheritdoc+@dev hybrid.
  • SKILL.md — "Blank lines aren't free either" (default fewer; keep the ones the project's doc tooling needs, e.g. Markdown's blank-line-before-a-list).
  • Review-session MEMORY: added inherited-doc-completion-is-review-work.

AccessControl (green3) — verified. Re-ran after the fix: pure @inheritdoc IAccessControl on all three external functions, no hybrid, no duplication (agent stated it chose to inherit rather than re-document). Internal functions documented; header + WARNING intact; no invented metadata.

Skill is stable across both files. Candidate future test: an interface with thin docs, to confirm passive mode inherits-as-is and leaves completion to review.

Refinement — @inheritdoc softened from "all-or-nothing" to "no duplication"

User feedback: "all-or-nothing" is too rigid. A shared interface used by several contracts can't hold per-implementation differences (e.g. one contract accepts a narrower input range than a sibling), so those must be documented on the implementation. New rule (comment-types.md):

  • Don't re-state what the base already says (the duplication we hit on AccessControl.grantRole).
  • Do add a delta that is specific to this implementation and cannot live in the shared base — as @inheritdoc + a short @dev note (just the specific part) or a standalone doc, per project.
  • Two gaps, two homes: applies-to-all → review fixes the interface; per-implementation → document the delta on the impl now.

Candidate test (folds in the thin-interface one): a shared interface with two implementations whose input constraints differ — confirm the agent inherits the common doc and documents only each impl's delta, without duplicating the shared parts.

Run 4 — @inheritdoc delta scenario (synthetic) — PASS

Constructed (work/inheritdoc-test/): a documented IVault interface (deposit/withdraw, both require amount > 0, each emits its event) and two implementations — BasicVault (no extra constraints) and CappedVault (a global deposit cap; deposit additionally reverts CapExceeded). Agent given the interface + context + stripped impls. Outputs: work/inheritdoc-test/*.green.sol.

Result matched the intended matrix exactly:

function expected got
BasicVault.deposit pure @inheritdoc ✅
BasicVault.withdraw pure @inheritdoc ✅
CappedVault.withdraw pure @inheritdoc (no delta) ✅
CappedVault.deposit @inheritdoc + cap delta only ✅ @dev Additionally reverts with CapExceeded if totalDeposited + amount would exceed cap.

Notable judgment (stated by the agent): it did not restate the interface's amount > 0 / event; it treated the over-withdraw underflow as already implied by the interface's "must have at least amount" requirement; and it left CappedVault.withdraw a pure inherit because the totalDeposited decrement is internal bookkeeping, not caller-facing. The softened rule (forbid duplication, allow a per-implementation delta) works as intended. Declaration docs (/// on state vars, error, ctor @param) are terse and correct.