Skip to content

fix: bound the decisions log on disk and on read (#603) - #606

Merged
Birfy merged 1 commit into
mainfrom
fix/603-decisions-log-bounded
Oct 21, 2026
Merged

Birfy merged 1 commit into
mainfrom
fix/603-decisions-log-bounded

Conversation

@Birfy

@Birfy Birfy commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

The decision log (<home>/decisions.jsonl) was capped to 2000 lines only when read: record() appended forever and list() parsed the whole file before discarding all but the newest 2000 lines. This bounds it on disk as well, and bounds the read.

复现

On main at abaf938b, with config.decisions: true, record 2001 decisions: the file keeps all 2001 lines and every read parses all of them. The new test reproduces that:

git checkout abaf938b -- core/src/host/decisions.js   # main source, new tests
node --test core/test/decisions.test.js
# not ok 4 - #603: record trims the log on disk, keeping the newest entries
# not ok 5 - #603: a byte cap bounds an oversized log left by an older build
# tests 5 / pass 3 / fail 2

改了谁

  • core/src/host/decisions.js: record() trims the log after each append to the newest 2000 lines and at most 1 MB, through a temp file + rename (the Bot side stays append-only); list() reads at most the last 1 MB instead of the whole file.
  • core/test/decisions.test.js: two tests — append 2001 entries and assert the on-disk file keeps exactly the newest 2000; an oversized log left by an older build is bounded by the 1 MB cap and the newest entry survives.
  • patches/ds-bot/files/src/host/decisions.js and patches/ds-bot/files/test/decisions.test.js: mirrors (decisions.js is a FlyBot-added file, so files/ is its generated form).
  • docs/subsystems/core-host.md: states the on-disk caps and the bounded read.
  • changelog.d/603-decisions-log-bounded.fixed.md.

跑了什么

  • node --test core/test/decisions.test.js → 5/5 pass (1.3 s), on the abaf938b tree.
  • node scripts/ds-bot-patches.mjs check → ok (pin v0.1.5, 10 patches, core/ equals ds-bot/ + patches).
  • node .github/scripts/changelog.mjs check → 19 fragments well formed.
  • node core/scripts/check-arch-docs.mjs with ARCH_DOCS_BASE = a base commit holding main's tree → arch-docs: OK (6 changed file(s)).
  • TZ=UTC DSH_HARNESS=… node core/scripts/lint.mjs → oxlint 0 errors, 43 warnings (pre-existing).

撤掉源码改动后变红

git checkout abaf938b -- core/src/host/decisions.js   # keep the new tests
node --test core/test/decisions.test.js
# not ok 4 - #603: record trims the log on disk, keeping the newest entries
# not ok 5 - #603: a byte cap bounds an oversized log left by an older build
# tests 5 / pass 3 / fail 2

With the fix restored: # tests 5 / pass 5 / fail 0.

没跑什么

core 全量一次性(本容器 pid 上限 256)、插件套件、smoke、真机、完整 npm run verify;由 CI 在 tip 上跑 tests-ok。

风险

Only core/src/host/decisions.js behaviour changed; no secrets, network, sharing or permission surface. (risk:T* is set by the controller.)

Fixes #603

record() appended forever and list() parsed the whole file before keeping the
newest 2000 lines. Trim the log in record() to the newest 2000 lines / 1 MB
(temp file + rename, so the Bot side stays append-only), and read at most the
last 1 MB in list(). Add the 2001-entry and oversized-file tests, update the
subsystem doc and add a changelog fragment.

@Birfy Birfy left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: PASS at 2a161c7 — tier from the diff: T2

Tip SHA: 2a161c7d411262a4a5c64009641000ce270a21a4
CI at this tip (run 39584454603): all 8 checks success —
  tests-ok, package, plugins (social|timeline|tree),
  ubuntu-latest Node 24, reviewed tip, risk tier
mergeable_state: clean
Revert-the-source evidence (from the body, checked against the new tests):
  git checkout abaf938b -- core/src/host/decisions.js; node --test core/test/decisions.test.js
  → not ok 4 (#603: record trims the log on disk) / not ok 5 (#603: a byte cap bounds …); 5 tests, 2 fail
  With the fix: 5/5 pass.

Tier: T2, not T1 — it is host code (core/src/host/) that deletes stored lines by design, so a bug in the trim path loses a Bot's recorded decisions rather than only misreporting them. It does not touch secrets, network, sharing or permission surfaces, so not T3.

I re-derived the trim logic from the patch instead of trusting the body, and it holds:

  • trim() walks from the newest line backwards, stopping at KEPT lines or when adding another line would pass MAX_BYTES; the newest line is always kept even if it alone is oversized (kept.length > 0 guard), so the log can never be emptied by the cap.
  • record() appends first, then trims only when linesOnDisk > KEPT || size > MAX_BYTES; linesOnDisk is counted once and then incremented, so the common path does not re-read the file on every write.
  • The rewrite goes through ${path}.tmp + rename, so a reader never sees a half-trimmed log, and record() serializes every step through its own promise chain — two appends cannot interleave a trim.
  • list() now reads at most the last MAX_BYTES from an oversized file and drops the partial first line, so a file left huge by an older build no longer costs a full parse.
  • Both tests pin the on-disk state (2001 appends → exactly 2000 lines with the oldest gone; oversized file → ≤ 1 MB with the newest entry alive), which is the right assertion — my earlier review of the sibling PR asked for exactly this.

Nothing blocking. Two non-blocking notes for later, neither for this PR: a hand-edited single line larger than 1 MB would be kept forever and re-trimmed on each append, and writeFile(tmp) gives the rewritten log the default mode instead of preserving the old file's permissions.

This is a reproducible defect fix on the admission line, so per the gate (PASS at tip + CI green + revert-red) I am queueing it.

@Birfy
Birfy added this pull request to the merge queue Oct 21, 2026
Merged via the queue into main with commit 7df78ba Oct 21, 2026
8 checks passed
@Birfy
Birfy deleted the fix/603-decisions-log-bounded branch October 21, 2026 10:35
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 21, 2026
Birfy added a commit to Birfy/FlyBot that referenced this pull request Oct 23, 2026
…#670)

Fixes ThinkFlowLab#609

Tip SHA: 4c8d98c

## Reproduce

The rule text says `risk:T*` is set by the controller from the diff,
never by hand, and the
mechanism does not hold up: `github-actions[bot]` labelled ThinkFlowLab#574 at
07:28:43Z, while ThinkFlowLab#606, ThinkFlowLab#610,
ThinkFlowLab#611, ThinkFlowLab#612, ThinkFlowLab#616, ThinkFlowLab#617, ThinkFlowLab#618 and ThinkFlowLab#619 carry no `risk:T*` at all even
though `risk tier (report
only)` succeeded on every one of them. The owner decision on the issue
is ②: keep the workflow
read-only, never label — and make the computed tier auditable instead of
remembered.

At this tip, `node .github/scripts/risk-gate.mjs --pr <n>` ends its
report with

    risk-tier: T3; basis: AGENTS.md=T3, plugins/social/src/social.js=T3

so a review can quote the tier the rules produced instead of restating a
number.

## What changed

- `.github/scripts/risk-gate.mjs`: `judge()` records every changed path
(and a rename's old path)
with the tier `tierOf()` gave it, and `summaryLine()` renders the final
line of the report from
that same result — highest tier first, paths capped at 10 with a `+N
more` tail. `render()` ends
the job summary with it, after the prose, so the two can never disagree.
- `.github/scripts/risk-gate.test.mjs`: four cases — the line's exact
shape for a T3 PR, the
ordering and the cap, the no-changed-files case, and "the line is the
last line of the report".
- `AGENTS.md` gains a bullet under the label section: quote the summary
line, the workflow never
writes a label, the controller copies it on its poll, no review
transcribes a tier.
- `.github/labels.json`: every `risk:T*` description keeps `advisory`
and `the merge gate
recomputes it` and now also names the controller as the writer (all ≤
100 characters).
- `docs/controller.md`: the tier stays auditable without the label —
quote the summary line.
- `.github/workflows/risk-tier.yml`: header comment only — no permission
added, no step changed.
It keeps `contents: read` and `pull-requests: read` and still never
labels, comments or merges.

## What I ran

- `TZ=UTC node --test .github/scripts/*.test.mjs` at this tip (with
`git` on `PATH`) →
  **144/144**.
- `node --test .github/scripts/risk-gate.test.mjs` → **18/18**,
including the four new cases.
- `node .github/scripts/risk-gate.mjs` behaviour checked by hand through
the harness result:
`judge([AGENTS.md, plugins/social/src/social.js])` → `risk-tier: T3;
basis: AGENTS.md=T3,
plugins/social/src/social.js=T3`; no files → `risk-tier: T0; basis: none
(no changed files)`.
- `changelog.mjs check` not run: a change users cannot see needs no
fragment (changelog.d/README).

## Red without the source change

- Removing the `basis.push([name, t])` line from `judge()` (the tests
untouched) →
`risk-gate.test.mjs` **16/18, 2 red**, exactly `summaryLine: one
machine-readable line…` and
  `summaryLine: highest tier first…`.
- Removing `summaryLine(result)` from the end of `render()` instead →
**17/18, 1 red**, exactly
`render: the machine-readable line is the last line of the report`.
Restored → 18/18.

## Not run

- The workflow itself (it needs Actions): the change to `risk-tier.yml`
is a comment, and the
  existing `the workflow stays report only` test still passes with it.
- `npm run verify` end to end: no `core/node_modules` in this sandbox
(the same on `main`).
- The `risk-tier` job on this PR runs the base copy of the script, so
its summary will only show
the new line after merge — that is the workflow's normal base-copy
behaviour, not a gap here.

## Risk

T3 by the repository's own rules (`.github/**`, `AGENTS.md`): a
reporting script plus text. It
adds no permission, no network call, no label, comment or merge, and
cannot block anything — the
job is still `continue-on-error: true` and not a required check. The
only behaviour it changes is
the last line of that job's summary. The one thing a reader must not
conclude is that the label
now arrives on time: it does not, and the docs keep saying why (ThinkFlowLab#629).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:automation Scheduled tasks and automation bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

decisions.jsonl never trims: the 2000-line cap is read-only, the file grows without bound

1 participant