Conversation
dash-proxy's behaviour lives in a 58k-line Go tree and in two years of review threads; neither is readable at the start of a task. This writes what the code does, and why, into lode/ so it is read before the code rather than rediscovered from it. The lode holds thirteen baseline and subsystem files: summary, terminology, practices, the map, the workflow profile, plans/README, and one summary each for the request path, certificates (issuance, and store recovery), dynamic sources, the cache, resilience, CLI/RPC, observability and testing/CI. Every line range and count in them was computed with a script rather than read off the screen, and every constant was checked against its declaration. lode/review/ carries 112 rules across 8 files, rewritten from review voice into statements about the system and re-verified against the current code. They cite 90 of cubic's 92 accepted learnings for this repo; 18 rules merge two or more learnings, because cubic records one learning per thread and a dozen threads about the certificate-store export are one rule about path validation. The 2 uncited learnings are assertion-wording notes for specific tests, not invariants. 15 rules cite merged PR review threads (#91, #92, #95, #97, #103, #105, #112, #113, #116), which is where the reasoned rejections live: 5 rules are "Not a bug" entries that stop a future reviewer re-raising a settled question. A further 15 rules were found in the code during seeding with no learning behind them. Nothing was dropped because its subject had gone -- all 42 functions the learnings name still exist. Two claims in the drafts were wrong and are fixed rather than carried: the dynamic-domain payload caps are 1 MB and 10 000 entries, not 5 MB and 50 000; and "writeFileAtomic for every state file" is really four staging shapes with four different durability guarantees. Retired seven local commands the plugin now supersedes -- lfg, plan, tdd, finish-prs, github-review-pr, github-review-failures, github-review-comments -- and pointed CLAUDE.md's table at their /lode: equivalents. debug-flaky never existed here. architect, perf, review-pr and security are repo-specific and stay. Doc-versus-code disagreements found while reading are reported with the PR rather than recorded in the lode, which describes only the tree it ships in. Claude-Session: https://claude.ai/code/session_01Hga3idYBp7fWdj7GmmfofF
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 33 files
Confidence score: 4/5
internal/server/san_cert_manager.go(RestoreCertificateStore) writesacme.statethroughwriteManagerStateFilerather thanwriteFileStaged, so the documented restore staging guarantee may not hold; confirm the intended atomicity and update the implementation or rule.lode/review/certs-issuance.mduses staleWhere:anchors for several functions, making the review guidance difficult to navigate; replace them with valid locations.lode/cache/summary.mdomits status 308 from its cacheability description, which could mislead readers about permanent redirects; document the additionalStatusPermanentRedirectbehavior.lode/certs/store-and-recovery.mdmiscounts the estate, whilelode/review/acme-providers.mdomits the acceptedgcloudprovider alias; correct both descriptions to keep the documentation aligned with the code.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lode/review/certs-issuance.md">
<violation number="1" location="lode/review/certs-issuance.md:12">
P3: The `Where:` anchors in entries 2, 3, 4, 7, and 9 point to `internal/server/san_cert_manager.go#filterBatchMates`, `#preflightTrigger`, `#clearBatchQuarantine`, and `#attributeBatchFailure`, but all four functions are defined in `internal/server/san_cert_batch_guard.go` (lines 69, 103, 143, 161); `san_cert_manager.go` only calls them (lines 528, 599, 658, 669). This file's purpose is navigation, so each wrong anchor defeats it. Point the five `Where:` lines at `internal/server/san_cert_batch_guard.go` for those functions.</violation>
</file>
<file name="lode/certs/store-and-recovery.md">
<violation number="1" location="lode/certs/store-and-recovery.md:3">
P3: The opening line claims the estate is "four things" but enumerates three (acme.state, certs/, dynamic-domains.state); the account keys are inside certs/ and the routing table is deliberately excluded. Change "four things" to "three things" so the count matches the list that follows.</violation>
</file>
<file name="lode/review/cert-store-restore.md">
<violation number="1" location="lode/review/cert-store-restore.md:53">
P2: The rule claims "every write goes through writeFileStaged" during restore, but the acme.state write does not: RestoreCertificateStore ends with writeManagerStateFile (internal/server/san_cert_manager.go:996), which stages at the fixed name `path + ".tmp"` via os.WriteFile + os.Rename with no fsync, no directory sync, no file.Sync, and no temp cleanup on failure. So the guarantees this rule asserts for every restore write — that a forced restore cannot follow a planted `.tmp`/`.tmpdir` symlink, and cannot leave a stray temp behind — only hold for the account-key and dynamic-domains files, not for the acme.state write that the same sentence says is staged last. Either route the acme.state write through writeFileStaged so the claimed durability is real, or correct the rule to state that acme.state goes through writeManagerStateFile and carries weaker staging guarantees.</violation>
</file>
<file name="lode/review/acme-providers.md">
<violation number="1" location="lode/review/acme-providers.md:10">
P3: The 'Provider-name validation' section enumerates ParseProviderName's GoogleCloud aliases as `google`/`gcp`/`googledns` but omits `gcloud`, which provider.go:52 accepts (and which is the canonical ProviderName string value, provider.go:23). Add `gcloud` to the list so this reference doc matches the parser it describes.</violation>
</file>
<file name="lode/cache/summary.md">
<violation number="1" location="lode/cache/summary.md:123">
P3: The claim 'RFC 9111 §15.1's list minus 206' is incomplete: cacheableStatuses also adds 308 (StatusPermanentRedirect), which is not in the RFC's list. A reader would conclude 308 responses are never cached when the code does cache them. State it as the RFC list minus 206 plus 308.</violation>
</file>
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
Re-trigger cubic
| - **Proven by:** `TestVerifyCertificateArchive_DropsInvalidAccountKey` | ||
| - **Origin:** cubic learning d9a06e31 | ||
|
|
||
| ### A restore writes `acme.state` last, and every write goes through `writeFileStaged` |
There was a problem hiding this comment.
P2: The rule claims "every write goes through writeFileStaged" during restore, but the acme.state write does not: RestoreCertificateStore ends with writeManagerStateFile (internal/server/san_cert_manager.go:996), which stages at the fixed name path + ".tmp" via os.WriteFile + os.Rename with no fsync, no directory sync, no file.Sync, and no temp cleanup on failure. So the guarantees this rule asserts for every restore write — that a forced restore cannot follow a planted .tmp/.tmpdir symlink, and cannot leave a stray temp behind — only hold for the account-key and dynamic-domains files, not for the acme.state write that the same sentence says is staged last. Either route the acme.state write through writeFileStaged so the claimed durability is real, or correct the rule to state that acme.state goes through writeManagerStateFile and carries weaker staging guarantees.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lode/review/cert-store-restore.md, line 53:
<comment>The rule claims "every write goes through writeFileStaged" during restore, but the acme.state write does not: RestoreCertificateStore ends with writeManagerStateFile (internal/server/san_cert_manager.go:996), which stages at the fixed name `path + ".tmp"` via os.WriteFile + os.Rename with no fsync, no directory sync, no file.Sync, and no temp cleanup on failure. So the guarantees this rule asserts for every restore write — that a forced restore cannot follow a planted `.tmp`/`.tmpdir` symlink, and cannot leave a stray temp behind — only hold for the account-key and dynamic-domains files, not for the acme.state write that the same sentence says is staged last. Either route the acme.state write through writeFileStaged so the claimed durability is real, or correct the rule to state that acme.state goes through writeManagerStateFile and carries weaker staging guarantees.</comment>
<file context>
@@ -0,0 +1,110 @@
+- **Proven by:** `TestVerifyCertificateArchive_DropsInvalidAccountKey`
+- **Origin:** cubic learning d9a06e31
+
+### A restore writes `acme.state` last, and every write goes through `writeFileStaged`
+- **Holds because:** the index must never name files that are not on disk, so certificate pairs, then account keys, then `dynamic-domains.state`, then the index. `writeFileStaged` creates a unique same-directory temp at mode 0600, writes, fsyncs, closes, renames, syncs the directory, and removes the temp on every failure path — so a forced restore cannot follow a planted symlink, cannot inherit a pre-existing temp file's permissions for a private key, and cannot leave a truncated file or stray temp behind. The temp pattern is short and fixed for the same length reason as the exporter's.
+- **Where:** `internal/server/cert_store_restore.go#RestoreCertificateStore`, `#writeFileStaged`, `#syncOpenDir`
</file context>
| @@ -0,0 +1,97 @@ | |||
| How a handshake turns into an ACME order: the batch, the probes, the token, and | |||
There was a problem hiding this comment.
P3: The Where: anchors in entries 2, 3, 4, 7, and 9 point to internal/server/san_cert_manager.go#filterBatchMates, #preflightTrigger, #clearBatchQuarantine, and #attributeBatchFailure, but all four functions are defined in internal/server/san_cert_batch_guard.go (lines 69, 103, 143, 161); san_cert_manager.go only calls them (lines 528, 599, 658, 669). This file's purpose is navigation, so each wrong anchor defeats it. Point the five Where: lines at internal/server/san_cert_batch_guard.go for those functions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lode/review/certs-issuance.md, line 12:
<comment>The `Where:` anchors in entries 2, 3, 4, 7, and 9 point to `internal/server/san_cert_manager.go#filterBatchMates`, `#preflightTrigger`, `#clearBatchQuarantine`, and `#attributeBatchFailure`, but all four functions are defined in `internal/server/san_cert_batch_guard.go` (lines 69, 103, 143, 161); `san_cert_manager.go` only calls them (lines 528, 599, 658, 669). This file's purpose is navigation, so each wrong anchor defeats it. Point the five `Where:` lines at `internal/server/san_cert_batch_guard.go` for those functions.</comment>
<file context>
@@ -0,0 +1,97 @@
+
+### The triggering domain is probed before any lock is taken, and it is never dropped from its own batch
+- **Holds because:** a domain that cannot answer a pre-flight probe must cost nothing — not an order, not a lock, not a wait. `preflightTrigger` runs first, outside the single flight. The inverse matters as much: `filterBatchMates` drops unreachable *mates*, but the trigger is the name a real client is handshaking for, so removing it would produce a certificate that does not cover the request that paid for it.
+- **Where:** `internal/server/san_cert_manager.go#provisionCertificate`, `#filterBatchMates`
+- **Proven by:** `TestBatchGuard_UnreachableTriggerIsRefusedWithoutBurningAnOrder`, `TestBatchGuard_TriggerDomainIsNeverDroppedFromItsOwnBatch`, `TestBatchGuard_ReachableTriggerProvisionsDespiteAStaleHold`
+- **Origin:** cubic learnings 94f61938, c9fb7a60
</file context>
| @@ -0,0 +1,151 @@ | |||
| # Certificate store: export, verify, restore, import | |||
|
|
|||
| The certificate estate is four things in the data directory: `acme.state` (the | |||
There was a problem hiding this comment.
P3: The opening line claims the estate is "four things" but enumerates three (acme.state, certs/, dynamic-domains.state); the account keys are inside certs/ and the routing table is deliberately excluded. Change "four things" to "three things" so the count matches the list that follows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lode/certs/store-and-recovery.md, line 3:
<comment>The opening line claims the estate is "four things" but enumerates three (acme.state, certs/, dynamic-domains.state); the account keys are inside certs/ and the routing table is deliberately excluded. Change "four things" to "three things" so the count matches the list that follows.</comment>
<file context>
@@ -0,0 +1,151 @@
+# Certificate store: export, verify, restore, import
+
+The certificate estate is four things in the data directory: `acme.state` (the
+index), `certs/` (one directory per certificate plus the ACME account keys),
+`dynamic-domains.state`, and nothing else. `CertStorePaths`
</file context>
Suggested change
| The certificate estate is four things in the data directory: `acme.state` (the | |
| The certificate estate is three things in the data directory: `acme.state` (the |
| - **Proven by:** `TestREADMEProviderTable_MatchesRegistry`, `TestRunCommand_DNSProviderHelpMatchesRegistry` | ||
| - **Origin:** cubic learnings d6216872, fc516966 | ||
|
|
||
| ### Provider-name validation goes through `acme.ParseProviderName`, never through `providers.Names()` |
There was a problem hiding this comment.
P3: The 'Provider-name validation' section enumerates ParseProviderName's GoogleCloud aliases as google/gcp/googledns but omits gcloud, which provider.go:52 accepts (and which is the canonical ProviderName string value, provider.go:23). Add gcloud to the list so this reference doc matches the parser it describes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lode/review/acme-providers.md, line 10:
<comment>The 'Provider-name validation' section enumerates ParseProviderName's GoogleCloud aliases as `google`/`gcp`/`googledns` but omits `gcloud`, which provider.go:52 accepts (and which is the canonical ProviderName string value, provider.go:23). Add `gcloud` to the list so this reference doc matches the parser it describes.</comment>
<file context>
@@ -0,0 +1,85 @@
+- **Proven by:** `TestREADMEProviderTable_MatchesRegistry`, `TestRunCommand_DNSProviderHelpMatchesRegistry`
+- **Origin:** cubic learnings d6216872, fc516966
+
+### Provider-name validation goes through `acme.ParseProviderName`, never through `providers.Names()`
+- **Holds because:** `providers` imports `acme`, so validating from the registry package would create an import cycle. `ParseProviderName` carries the aliases as well as the canonical names (`cf`, `aws`/`r53`, `do`, `google`/`gcp`/`googledns`, `nc`, `gd`, `hz`, `vr`), which a derived list would silently drop.
+- **Where:** `internal/server/acme/provider.go#ParseProviderName` (44-69), `internal/server/acme/mapping.go#ParseProviderEntries`
</file context>
Suggested change
| ### Provider-name validation goes through `acme.ParseProviderName`, never through `providers.Names()` | |
| (`cf`, `aws`/`r53`, `do`, `gcloud`/`google`/`gcp`/`googledns`, `nc`, `gd`, `hz`, `vr`) |
| a cardinality bomb. | ||
|
|
||
| `cacheableStatuses` (`internal/server/cache_policy.go`) is RFC 9111 §15.1's list | ||
| minus 206: a partial response describes a byte range this proxy never tracks, |
There was a problem hiding this comment.
P3: The claim 'RFC 9111 §15.1's list minus 206' is incomplete: cacheableStatuses also adds 308 (StatusPermanentRedirect), which is not in the RFC's list. A reader would conclude 308 responses are never cached when the code does cache them. State it as the RFC list minus 206 plus 308.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lode/cache/summary.md, line 123:
<comment>The claim 'RFC 9111 §15.1's list minus 206' is incomplete: cacheableStatuses also adds 308 (StatusPermanentRedirect), which is not in the RFC's list. A reader would conclude 308 responses are never cached when the code does cache them. State it as the RFC list minus 206 plus 308.</comment>
<file context>
@@ -0,0 +1,140 @@
+a cardinality bomb.
+
+`cacheableStatuses` (`internal/server/cache_policy.go`) is RFC 9111 §15.1's list
+minus 206: a partial response describes a byte range this proxy never tracks,
+and storing one would answer a later full request with a fragment.
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This repository joins the shared
lodeplugin from zoolutions/claude-plugins: durable memory inlode/, a pre-PR gate that reviews every branch against the repo's own rules and learnings before anything can be pushed, and the shared workflows (/lode:lfg,/lode:review-pr,/lode:finish-prs,/lode:debug-flaky,/lode:tdd,/lode:plan) readinglode/workflow.md.lode/— summary, terminology, practices, map, and eight subsystem summaries (routing and services, certificates and ACME, dynamic sources, cache, resilience, CLI and RPC, observability, testing and CI). Every claim cites a file and function; every count was computed (16 RPC call sites, 14 commands, 18 cache-refusal reasons, 16 metrics collectors, 1,080 tests in 115 files, 1 MB and 10,000-entry domain-list caps).lode/review/— 112 rules in eight files: 90 of the 92 cubic learnings for this repo (18 rules merge two or more), 15 rules from the review threads of merged PRs feat(san-cert): import certificates from a Traefik acme.json #91 through fix(acme): align the cloudflare credential rule with what lego actually reads #116 including five reasoned rejections as Not a bug, and 16 invariants found in the code during seeding with no learning behind them (the commit message says 15; the gate counted 16). The two uncited learnings are assertion-wording notes for named tests, not invariants.lode/workflow.md(219 lines) — the profile, including that the local golangci-lint (2.13.2) is newer than CI's pin (v2.11.3), so a local run reports findings CI will not.lfg,plan,tdd,finish-prs,github-review-pr,github-review-failures,github-review-comments. Kept:architect,perf,review-pr(read-only, unlike/lode:review-pr),security. New.claude/settings.jsonenablinglode@zoolutions;CLAUDE.mdgains a Memory section and the new command rows;.gitignoregainslode/tmp/.No Go changes. Two things worth a follow-up:
.claude/rules/agents.md:17andcoding-style.md:102still say 9 RPC call sites where the code has 16; andscript/releaseis upstream's and pushesbasecamp/kamal-proxywith the old label, whilescript/release-dashis the one this repo uses. Fifteen smaller doc-versus-code gaps are recorded in the lode where they apply.Test plan
go version,go vet -h,go test -h,gofmt -l internal/ cmd/clean,golangci-lint --version, Makefile targets listed;go test ./internal/metrics/and./internal/server/acme/...green offline/lode:lfgon the rules-file fix reads the profile and reaches the gateDeviations & judgment calls
/lode:seed, resumed once after a rate limit (every draft re-verified; three defects in them fixed, including domain-list caps stated at five times their real values), then gated here.LODE_SKIP_GATE=1after the gate passed: the orchestrating session runs the cached 0.1.0 push hook keyed on a different checkout. A session opened here will not need it.Gate
Rounds: 1. Findings: 0 P1, 0 P2, 2 P3 (both recorded here). Rejected 0, deferred 0.
https://claude.ai/code/session_01Hga3idYBp7fWdj7GmmfofF
Summary by cubic
Adds durable project memory in
lode/and enables the sharedlodeplugin, so sessions read the repo's rules before exploring and a pre-PR gate reviews every branch against them before push. No Go code changes.Memory
lode/documents the request path, certificates, cache, dynamic sources, resilience, CLI/RPC, observability, and testing, with every claim cited to a file and function.lode/review/holds 112 rules: 90 from prior learnings, 15 from merged PR review threads, 16 found in the code during seeding.lode/workflow.mdis the profile the shared/lode:commands read, including that local golangci-lint (2.13.2) is newer than CI's pin (v2.11.3).Retired commands
lfg,plan,tdd,finish-prs,github-review-pr,github-review-failures, andgithub-review-comments; their/lode:equivalents replace them.architect,perf,review-pr, andsecuritystay..claude/settings.jsonenableslode@zoolutions;CLAUDE.mdand.gitignoreupdated.script/releasepushes the old image label whilescript/release-dashis the one used.Written for commit d749b26. Summary will update on new commits.