fix: retrospective review fixes for merged PRs #27/#30/#38#88
Merged
Conversation
…y cap AnthropicAPIEnvVars stripped ANTHROPIC_BEDROCK_BASE_URL / ANTHROPIC_VERTEX_BASE_URL / ANTHROPIC_VERTEX_PROJECT_ID, but never the actual CLAUDE_CODE_USE_BEDROCK / CLAUDE_CODE_USE_VERTEX enable switches that route Claude Code onto Bedrock/Vertex billing in the first place — the vars bough did strip are only auxiliary endpoint/project overrides that do nothing unless one of these two is also set. An operator with a normal enterprise, AWS-billed CLAUDE_CODE_USE_BEDROCK=1 shell had that var pass straight through into the `claude --print` subprocess bough spawns, silently billing their AWS account instead of their Claude Code subscription — contradicting the README's "cannot silently flip to API-key billing" claim. bough doctor's DetectAnthropicAPIVars scans the same list, so it reported a false-clean posture the whole time. Both switches are now stripped and detected. Also, the observer daemon's advertised "30 calls/hour" self-DoS cap never actually accumulated: runObserverOnceQuiet spawns a fresh `bough observer run-once` subprocess every tick specifically so each pass gets an isolated provider/limiter lifecycle (a panic in one pass must not kill the daemon loop) — but that means each tick's claudecli.Limiter starts its hourWindow back at empty, so a daemon running at the recommended --interval 60 could fire up to 2x the documented hourly ceiling with no cap ever engaging. Extracted the tick body into tickOnce and wired a single daemon-lifetime *Limiter into the loop (MaxCallsPerSession disabled, since "session" there means "one manual run", not "daemon uptime") so the hourly cap now actually accumulates across ticks the way the README already claims it does. The circuit-breaker half of the same daemon gap (consecutive failures also never accumulate across ticks) needs the subprocess's exit status plumbed back to the caller and shutdown-vs-failure disambiguation — filed as #86 rather than folded in here. Surfaced by retrospective review of merged PR #38 (ikeikeikeike/bough).
allocateRoles called pickFreePort independently per role with no record of ports already claimed earlier in the same call, so two roles of a multi-port engine (e.g. rabbitmq's AMQP + Management) whose PortRangeDefault ranges overlap could be handed the identical port — pickFreePort's probe-release leaves the just-vacated port free again microseconds later, so an independent scan for the next role lands on it too. Up() then fails to publish the same host port twice for an otherwise perfectly contract-conformant plugin, a false negative produced by the harness itself. pickFreePort now takes the set of ports already claimed in this allocateRoles call and skips them. Also, faults.go's spawnFreshAndPickPort — used by all three Fault_* subtests — still returned the raw, unscanned PortRangeDefault Low instead of routing through the new picker. On a CI runner where another process already holds that port (the exact class of collision this PR's own doc comment names as the motivation), Fault_DatadirPermission and Fault_ImagePullFailure both only assert `upErr != nil`, so a port-bind failure made them silently false-pass without ever having exercised the fault they exist to guard. Now routed through pickFreePort like allocateRoles. Folded in while touching this code: pickFreePort now calls dockerutil.IsPortFree instead of duplicating its bind-probe-close body, its 64-attempt scan bound is a named constant (portScanAttempts), and it has unit test coverage for all three branches (degenerate range, scan-and-skip, exhaustion fallback) plus the cross-role claim tracking. Verified locally: full mysql plugin conformance suite (including Fault_PortConflict / Fault_ImagePullFailure) passes against real Docker. The narrower TOCTOU gap between pickFreePort's probe-release and Up()'s actual bind is structural (docker requires the host port be free at publish time, same caveat dockerutil.IsPortFree's own doc comment already carries) and is not closed by this fix; documented in pickFreePort's doc comment rather than papered over. Surfaced by retrospective review of merged PR #27 (ikeikeikeike/bough).
…ssion_id, case-sensitive match, sentinel bug, DRY
Nine distinct defects in the foundational v0.9.2 continuous-learning
code (inject-context, session-end, preserve-instincts, ecc-import),
none previously tracked as an open issue or already fixed by later
iteration on this subsystem:
- internal/session/evaluate.go: addTokens only recognized ASCII
a-z/0-9 as token characters, so instinctOverlap always evaluated to
0 for Japanese (or any non-Latin script) instinct/observation text
— permanently disabling confidence reinforcement/demotion for any
non-English-language project. Now uses unicode.IsLetter/IsDigit and
counts by rune, not byte length.
- internal/cli/ecc_import.go:
- `--apply` aborted the ENTIRE migration on the first project whose
copy failed, and because it iterated a Go map, which project
triggered the abort (and which were never attempted) was
non-deterministic across runs. Now iterates projects in sorted
order, continues past a copy failure, and reports a full
imported-N-of-M summary with a non-zero exit listing exactly what
failed.
- `--apply=false` silently still performed the copy: pflag's
BoolFunc callback ignored the string value it was passed and
unconditionally cleared dryRun. Now parses the value.
- The per-project WriteUpsert call did a full projects.json
read-modify-write per project — O(N) full-file rewrites for an
N-project corpus. Collapsed into one WriteUpsertMany call after
all copies succeed (internal/homunculus/project.go: WriteUpsert is
now WriteUpsertMany's single-project case).
- internal/cli/hook.go: the SessionEnd hook dispatch always called
runSessionEnd with a hardcoded "" session id, so eval/scores.jsonl's
session_id field was dead in normal operation even though Claude
Code's real hook payload carries it. Added extractSessionID
(mirrors buildMatchContext's probe-decode pattern) and wired it in.
- internal/session/preserve.go: firstActionLine matched "## Action"
with a case-sensitive ==, unlike every other implementation of this
helper in the codebase (inject.go, evolve/judge.go,
cli/claudemd.go), so a differently-cased heading (hand-edited, or
migrated in via ecc import) silently returned the wrong line into
MEMORY.md. Rewritten to mirror inject.go's helper exactly
(strings.EqualFold + stdlib Split/TrimSpace instead of a hand-rolled
splitLines/trimSpace that only stripped ASCII space/tab).
- internal/inject/inject.go: Options.withDefaults() treated
MinConfidence<=0 as "unset" and silently substituted the 0.50
default, so the documented `--min-confidence 0` ("no floor") was
indistinguishable from not passing the flag — dropping every
instinct in the real, reachable 0.30-0.49 band with no warning.
MinConfidence is now a *float64 (nil = unset, any non-nil value used
as-is); internal/cli/inject.go only sets it when
cmd.Flags().Changed("min-confidence").
- internal/cli/hook.go + inject.go: dispatchInjectContext
re-implemented newInjectContextCmd's RunE body inline instead of
sharing it — the two copies already needed an identical fix
hand-applied twice before (DetectIdentity(cwd) ->
DetectIdentity(resolveMonorepoRoot(cwd))). Extracted the shared
runInjectContext helper, mirroring the runSessionEnd/
runPreserveInstincts pattern session.go already uses.
Left out of scope, filed as follow-up issues rather than folded in:
#86 (the daemon's circuit breaker has the same
fresh-limiter-per-tick gap as its hourly cap, fixed in wave 4's PR #38
commit — needs subprocess exit-code plumbing), #87
(ecc_import.go's copyFile/expandHome duplicate internal/registry's
helpers at a different robustness level — a design decision, not a
narrow fix).
Surfaced by retrospective review of merged PR #30 (ikeikeikeike/bough).
ikeikeikeike
commented
Jul 16, 2026
| return p | ||
| } | ||
| } | ||
| return low |
Member
Author
There was a problem hiding this comment.
Retrospective review of this PR found one still-live gap in this exact fix: this fallback returns low unconditionally, ignoring claimed. If a multi-port engine's roles have narrow/overlapping PortRangeDefault ranges (the rabbitmq AMQP+Management shape cited above) and the second role's scan exhausts while low is already claimed by the first role, allocateRoles hands both roles the identical port — silently reintroducing the exact cross-role collision this PR was written to close, just via the fallback path instead of the primary scan path. Fixed in PR #113, which falls back to the first unclaimed port in [low, high] instead, with a regression test (TestPickFreePort_ExhaustionAvoidsClaimedLow) confirmed to fail against this code and pass against the fix.
ikeikeikeike
added a commit
that referenced
this pull request
Jul 16, 2026
fix: retrospective review fixes for merged PR #88
This was referenced Jul 16, 2026
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.
Retrospective
/code-reviewsweep of merged PRs that shipped without pre-merge review. Wave 4 of 4 (follows #84, #85) — this completes the sweep of all 20 target PRs.Highlights:
SanitizeAnthropicEnvstripped only the Bedrock/Vertex auxiliary endpoint vars but not the actualCLAUDE_CODE_USE_BEDROCK/CLAUDE_CODE_USE_VERTEXenable switches, so an enterprise shell could silently route bough'sclaude --printcalls onto AWS/GCP billing despite the README's claim (andbough doctorreported false-clean); the observer daemon's advertised 30-calls/hour cap never accumulated across ticks (fresh limiter per subprocess) — now enforced by a daemon-lifetime limiter; the session tokenizer was ASCII-only, permanently disabling confidence learning for Japanese/non-Latin instinct text;ecc import --applyaborted mid-migration in nondeterministic map order (now continues + summarizes), and--apply=falsesilently performed the real import; conformance'spickFreePortcould hand two roles of a multi-port engine the identical port, and the Fault_* subtests bypassed the picker entirely, letting two of them false-pass on port collisions.Deferred to issues rather than folded in: #86 (daemon circuit breaker needs subprocess exit-code plumbing), #87 (copyFile/expandHome triplication is a design decision).
Each commit is scoped to one source PR with full rationale in the message.