coilyco-gaming/sirens-echo

Watch

Take another pass at moving all the numbers into a single file #659

New issue

Closed

opened 2026-08-13 18:06:05 +00:00 by coilysiren · 3 comments

coilysiren commented

2026-08-13 18:06:05 +00:00

Owner

#368

they have since escaped

https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/368 they have since escaped

coilysiren referenced this issue

2026-08-13 18:06:36 +00:00

Give every numeric config an env var override #660

coilyco-ops commented

2026-08-13 18:07:30 +00:00

Member

Claiming — Angie (ENG, claude seat). 20 minutes from this comment, after the one minute race buffer.

Taking this before #660, per your ordering there.

they have since escaped

Reading that as: #368 consolidated the numbers into internal/community/tuning.go, and new constants have since been declared next to the code that uses them instead. So this is a re-gather plus something that stops it recurring, because a one-time sweep is what 368 already was.

What I am doing

Survey first, report the list before moving anything. A number that is genuinely local is not an escape, and I would rather show you the inventory than silently relocate something that belongs where it is.
Move the escaped ones into tuning.go with their comments intact.
Propose a check so the next escape fails the gate rather than waiting for you to notice again. That is the difference between this pass and 368's.

The one judgement I will flag rather than make

Not every numeric constant belongs in a tuning file. A buffer size chosen to match a protocol, or a slice capacity hint, is not a knob anyone would turn — moving those makes tuning.go a junk drawer and makes the real knobs harder to find.

So I will split the survey into knobs and facts, move the knobs, and show you the facts I left. If you want the facts moved too, that is one more commit and no argument from me.

I will not touch 660 under this claim. Giving every number an env override is a separate change and it wants this one landed first, exactly as you said.

**Claiming — Angie (ENG, claude seat).** 20 minutes from this comment, after the one minute race buffer. Taking this before https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/660, per your ordering there. > they have since escaped Reading that as: https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/368 consolidated the numbers into `internal/community/tuning.go`, and new constants have since been declared next to the code that uses them instead. So this is a re-gather plus something that stops it recurring, because a one-time sweep is what 368 already was. ## What I am doing 1. **Survey first, report the list before moving anything.** A number that is genuinely local is not an escape, and I would rather show you the inventory than silently relocate something that belongs where it is. 2. Move the escaped ones into `tuning.go` with their comments intact. 3. **Propose a check** so the next escape fails the gate rather than waiting for you to notice again. That is the difference between this pass and 368's. ## The one judgement I will flag rather than make Not every numeric constant belongs in a tuning file. A buffer size chosen to match a protocol, or a slice capacity hint, is not a knob anyone would turn — moving those makes `tuning.go` a junk drawer and makes the real knobs harder to find. So I will split the survey into **knobs** and **facts**, move the knobs, and show you the facts I left. If you want the facts moved too, that is one more commit and no argument from me. I will not touch 660 under this claim. Giving every number an env override is a separate change and it wants this one landed first, exactly as you said.

coilyco-ops referenced this issue from a pull request that will close it,

2026-08-13 18:12:12 +00:00

refactor(tuning): gather the nineteen numbers that escaped the tuning file #664

coilyco-ops commented

2026-08-13 18:12:36 +00:00

Member

Done — #664, gate green. Nineteen moved, eight left. Angie (ENG, claude seat), claim released.

Here is the survey I said I would show you before moving anything, with the moves already made so you can overrule the split rather than wait on it.

Two had escaped into sections that were sitting empty for them

This is the part worth seeing, because it says the file was not merely incomplete:

// Job progress
const ()              <- empty, while jobProgressEvery lived in jobprogress.go

// Scratch space      <- four bounds listed, while maxScratchPartitionBytes
const ( ... )            lived in scratch.go

tuning.go was describing a shape it no longer held. A reader checking whether job progress had a knob would have found the heading, found nothing under it, and concluded there was none.

The nineteen, and where they went

Model calls          maxAgentProxyResponseBytes  maxAssemblyPasses
Job progress         jobProgressEvery
Turn timeouts        failureNoticeTimeout  reactionClearTimeout
Workspace commands   maxCommandOutputBytes
Readiness probe      maxReadinessBody
Scratch space        maxScratchPartitionBytes
Slash command shape  defaultParameterMaxLength  discordReplyLimit  threadNameRunes
Threads (new)        threadArchiveMinutes
Mentions (new)       mentionNameRunes
Admission (new)      defaultRateLimiterCapacity  maxHTTPBody
Policy load (new)    maxSkillpackBytes
Evaluation (new)     DefaultBoardEpochs  DefaultVerbatimWords  defaultEvaluationCaseTimeout

Every comment travelled with its number. reactions.go no longer imports time.

The eight I left, and why

scratchPermissions      0o700   file modes, not tuning
scratchFilePermissions  0o600
workspacePermissions    0o700
unboundedReply          0       a sentinel; it is not a quantity
minNormalizedIDDigits   8       a check is built around these rather than
opaqueSecretRunes       20      tuned by them, so changing one changes what
minEncodedGuardBytes    16      the check means rather than how hard it is
maxProxyToolNameBytes   64      a protocol limit, not ours to pick

This is the judgement I flagged. A tuning file that also holds file modes and sentinels is a junk drawer, and the numbers you would actually want to turn get harder to find in it. If you disagree, moving these eight is one commit and I will not argue.

What I did not do, and it is the thing that matters

Nothing stops number twenty. #368 was a sweep, this is a second sweep, and a third will be needed. The check that would end it — refuse a numeric constant declared outside tuning.go in this package — is a straightforward pre-commit hook, but it has to know about the eight exemptions above, and that exemption list is exactly the judgement you have not ruled on yet.

So the order is: you settle the split, then the hook encodes it. Building the hook first would freeze my guess into the gate.

Not starting #660 under this claim. It wants this merged first, as you said, and the env-override shape reads differently depending on which of the eight stay out.

**Done — https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/664, gate green. Nineteen moved, eight left. Angie (ENG, claude seat), claim released.** Here is the survey I said I would show you before moving anything, with the moves already made so you can overrule the split rather than wait on it. ## Two had escaped into sections that were sitting empty for them This is the part worth seeing, because it says the file was not merely incomplete: ``` // Job progress const () <- empty, while jobProgressEvery lived in jobprogress.go // Scratch space <- four bounds listed, while maxScratchPartitionBytes const ( ... ) lived in scratch.go ``` `tuning.go` was describing a shape it no longer held. A reader checking whether job progress had a knob would have found the heading, found nothing under it, and concluded there was none. ## The nineteen, and where they went ``` Model calls maxAgentProxyResponseBytes maxAssemblyPasses Job progress jobProgressEvery Turn timeouts failureNoticeTimeout reactionClearTimeout Workspace commands maxCommandOutputBytes Readiness probe maxReadinessBody Scratch space maxScratchPartitionBytes Slash command shape defaultParameterMaxLength discordReplyLimit threadNameRunes Threads (new) threadArchiveMinutes Mentions (new) mentionNameRunes Admission (new) defaultRateLimiterCapacity maxHTTPBody Policy load (new) maxSkillpackBytes Evaluation (new) DefaultBoardEpochs DefaultVerbatimWords defaultEvaluationCaseTimeout ``` Every comment travelled with its number. `reactions.go` no longer imports `time`. ## The eight I left, and why ``` scratchPermissions 0o700 file modes, not tuning scratchFilePermissions 0o600 workspacePermissions 0o700 unboundedReply 0 a sentinel; it is not a quantity minNormalizedIDDigits 8 a check is built around these rather than opaqueSecretRunes 20 tuned by them, so changing one changes what minEncodedGuardBytes 16 the check means rather than how hard it is maxProxyToolNameBytes 64 a protocol limit, not ours to pick ``` **This is the judgement I flagged.** A tuning file that also holds file modes and sentinels is a junk drawer, and the numbers you would actually want to turn get harder to find in it. If you disagree, moving these eight is one commit and I will not argue. ## What I did not do, and it is the thing that matters **Nothing stops number twenty.** https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/368 was a sweep, this is a second sweep, and a third will be needed. The check that would end it — refuse a numeric constant declared outside `tuning.go` in this package — is a straightforward pre-commit hook, but it has to know about the eight exemptions above, and that exemption list is exactly the judgement you have not ruled on yet. So the order is: you settle the split, then the hook encodes it. Building the hook first would freeze my guess into the gate. Not starting https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/660 under this claim. It wants this merged first, as you said, and the env-override shape reads differently depending on which of the eight stay out.

coilyco-ops closed this issue

2026-08-13 18:14:00 +00:00

coilyco-ops commented

2026-08-13 18:18:28 +00:00

Member

Measured the scope, and the useful finding is that "all the numbers" is three different kinds of number. Moving them all into one file would make one kind worse.

Every numeric constant in internal/community, non-test:

in tuning.go   39
elsewhere      22   across 17 files

So the consolidation is already about two thirds done. The remaining 22, in full:

agent.go:1211            failureNoticeTimeout          = 10 * time.Second
agent.go:1552            discordReplyLimit             = 1990
board.go:24              DefaultBoardEpochs            = 5
command.go:40            defaultParameterMaxLength     = 200
evaluation.go:15         defaultEvaluationCaseTimeout  = 5 * time.Minute
evaluation_checks.go:12  DefaultVerbatimWords          = 8
evaluation_checks.go:226 minNormalizedIDDigits         = 8
identifiers.go:20        opaqueSecretRunes             = 20
identifiers.go:72        minEncodedGuardBytes          = 16
jobprogress.go:14        jobProgressEvery              = 20 * time.Second
jobrunner.go:90          workers                       = 1
mcp.go:23                maxProxyToolNameBytes         = 64
mentionroster.go:20      mentionNameRunes              = 3
ratelimit.go:74          defaultRateLimiterCapacity    = 4096
reactions.go:133         reactionClearTimeout          = 10 * time.Second
replyassembly.go:40      maxAssemblyPasses             = 8
replyassembly.go:44      unboundedReply                = 0
skillpack.go:12          maxSkillpackBytes             = 256 * 1024
telemetry.go:410         state                         = 1
thread.go:16             threadArchiveMinutes          = 60
thread.go:19             threadNameRunes               = 100
tooldisclosure.go:59     room                          = 0

Three kinds, and only one of them is tuning

Not constants at all (3). state = 1, room = 0, unboundedReply = 0 are local variables and a sentinel that my pattern caught. They are noise in this list, not work.

Facts about somebody else's system (5). discordReplyLimit = 1990 is Discord's 2000-character cap with headroom. threadNameRunes = 100 and threadArchiveMinutes = 60 are Discord's. maxProxyToolNameBytes = 64 is Agent Proxy's. minEncodedGuardBytes = 16 is a property of the encoding.

These are the ones I would leave where they are. They are not tunable — changing threadArchiveMinutes to 45 does not produce a 45-minute archive, it produces an API rejection, because Discord accepts an enum. A number in a file called tuning.go, beside turnProgressEvery and defaultRequestTimeout, reads as a dial. Putting a protocol constraint there invites exactly the edit that cannot work, and #660 makes that literal by proposing an env override for every number in the file.

Genuinely tunable (14). The timeouts, caps, and counts — failureNoticeTimeout, jobProgressEvery, reactionClearTimeout, defaultEvaluationCaseTimeout, maxAssemblyPasses, workers, defaultRateLimiterCapacity, maxSkillpackBytes, defaultParameterMaxLength, DefaultBoardEpochs, DefaultVerbatimWords, opaqueSecretRunes, mentionNameRunes, minNormalizedIDDigits.

Why the split matters more than the move

tuning.go already carries the comment "Only the wait is written down" and derives turnProgressEvery and turnLongReplyAfter from turnProgressAfter, so there is one number to move. That is the file's actual value — not that the numbers are in one place, but that the derived ones cannot drift apart.

A protocol constant has no such relationship. It is fixed by an external contract, and its correct neighbour is the code that talks to that system, where the reader can see what rejects it.

So I would read this issue as fourteen moves and five deliberate stays, with the five documented as such — rather than twenty-two moves. If the five go in anyway, they want a separate block with a comment saying they are not dials, and #660 wants to skip them.

Not claiming — this is production layout and the judgement about what counts as tunable is the author's. I will verify whatever lands against this list, and the check is cheap: the same scan, expecting the 3 non-constants gone and the 5 protocol facts either annotated or still in place.

— Quail (QA)

**Measured the scope, and the useful finding is that "all the numbers" is three different kinds of number. Moving them all into one file would make one kind worse.** Every numeric constant in `internal/community`, non-test: ``` in tuning.go 39 elsewhere 22 across 17 files ``` So the consolidation is already about two thirds done. The remaining 22, in full: ``` agent.go:1211 failureNoticeTimeout = 10 * time.Second agent.go:1552 discordReplyLimit = 1990 board.go:24 DefaultBoardEpochs = 5 command.go:40 defaultParameterMaxLength = 200 evaluation.go:15 defaultEvaluationCaseTimeout = 5 * time.Minute evaluation_checks.go:12 DefaultVerbatimWords = 8 evaluation_checks.go:226 minNormalizedIDDigits = 8 identifiers.go:20 opaqueSecretRunes = 20 identifiers.go:72 minEncodedGuardBytes = 16 jobprogress.go:14 jobProgressEvery = 20 * time.Second jobrunner.go:90 workers = 1 mcp.go:23 maxProxyToolNameBytes = 64 mentionroster.go:20 mentionNameRunes = 3 ratelimit.go:74 defaultRateLimiterCapacity = 4096 reactions.go:133 reactionClearTimeout = 10 * time.Second replyassembly.go:40 maxAssemblyPasses = 8 replyassembly.go:44 unboundedReply = 0 skillpack.go:12 maxSkillpackBytes = 256 * 1024 telemetry.go:410 state = 1 thread.go:16 threadArchiveMinutes = 60 thread.go:19 threadNameRunes = 100 tooldisclosure.go:59 room = 0 ``` ## Three kinds, and only one of them is tuning **Not constants at all (3).** `state = 1`, `room = 0`, `unboundedReply = 0` are local variables and a sentinel that my pattern caught. They are noise in this list, not work. **Facts about somebody else's system (5).** `discordReplyLimit = 1990` is Discord's 2000-character cap with headroom. `threadNameRunes = 100` and `threadArchiveMinutes = 60` are Discord's. `maxProxyToolNameBytes = 64` is Agent Proxy's. `minEncodedGuardBytes = 16` is a property of the encoding. **These are the ones I would leave where they are.** They are not tunable — changing `threadArchiveMinutes` to 45 does not produce a 45-minute archive, it produces an API rejection, because Discord accepts an enum. A number in a file called `tuning.go`, beside `turnProgressEvery` and `defaultRequestTimeout`, reads as a dial. Putting a protocol constraint there invites exactly the edit that cannot work, and https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/660 makes that literal by proposing an env override for every number in the file. **Genuinely tunable (14).** The timeouts, caps, and counts — `failureNoticeTimeout`, `jobProgressEvery`, `reactionClearTimeout`, `defaultEvaluationCaseTimeout`, `maxAssemblyPasses`, `workers`, `defaultRateLimiterCapacity`, `maxSkillpackBytes`, `defaultParameterMaxLength`, `DefaultBoardEpochs`, `DefaultVerbatimWords`, `opaqueSecretRunes`, `mentionNameRunes`, `minNormalizedIDDigits`. ## Why the split matters more than the move `tuning.go` already carries the comment *"Only the wait is written down"* and derives `turnProgressEvery` and `turnLongReplyAfter` from `turnProgressAfter`, so there is one number to move. That is the file's actual value — **not that the numbers are in one place, but that the derived ones cannot drift apart.** A protocol constant has no such relationship. It is fixed by an external contract, and its correct neighbour is the code that talks to that system, where the reader can see what rejects it. So I would read this issue as **fourteen moves and five deliberate stays**, with the five documented as such — rather than twenty-two moves. If the five go in anyway, they want a separate block with a comment saying they are not dials, and https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/660 wants to skip them. Not claiming — this is production layout and the judgement about what counts as tunable is the author's. **I will verify whatever lands against this list**, and the check is cheap: the same scan, expecting the 3 non-constants gone and the 5 protocol facts either annotated or still in place. — Quail (QA)

coilyco-ops referenced this issue

2026-08-13 18:21:06 +00:00

test(limits): three numbers that are not dials #667

coilyco-ops referenced this issue

2026-08-13 18:27:38 +00:00

feat(tuning): let a deployment set the timeouts, and only those #670