Conversation
…e per-role tool/effort scoping Two patterns the harness uses, applied to OpenSwarm's own pipeline. advisor role (the missed-defect net) - A second, independently-prompted model asked one narrow question: what concrete defects did the reviewer miss? Its only permitted effect is to make the gate MORE cautious — the merged decision is max(approve < revise < reject), so an advisor approve can never soften a reviewer revise/reject, and a raised severity with no concrete finding is discarded. - Fails OPEN: any error, timeout, empty or unparseable output returns ran:false with the reviewer's result untouched and no exit-code change. - Runs before dedupe/history in `review`, per area in `review --max`, and in every `--fix` re-review round. - Disabled by default (a second paid call per review). Its model must come from a different family than the reviewer's, or it is a second identical opinion — the trap modelCompat.ts already documents for `escalate`. Declarative per-role subagent settings - `RoleConfig.tools.allow` / `.deny` and `effort`, honored end-to-end from config to the agentic loop for worker, reviewer, advisor, tester, documenter, auditor and skill-documenter. - The allow-list can only NARROW: it is applied last, over the composed built-in tool array, and only removes entries — so an allow-list naming `bash` on a read-only role stays withheld, and the same narrowed set feeds allowedToolNames, so a withheld name is also refused at dispatch. - `deny` is applied after `allow` (deny wins) and supports a trailing `*`. - MCP/coordination tools keep their own flags, per the field's contract. - On delegated-CLI adapters (claude/codex), which own their own tool loop, the list cannot be enforced — base.ts now says so in its dropped-options warning rather than leaving it silently inert (the AGT-4444 failure class). Verification: tsc clean; oxlint clean; 6803 tests pass (the one failure is the known load flake AGT-4537 — passes 2/2 in isolation, and this branch touches no worktree file); config smoke confirms the new surface parses; new focused tests cover the safety rules, the narrowing-only invariant, and the CLI integration.
…no longer kills the heartbeat (AGT-4518) reconcileDurableArtifacts rebuilds a completion effect for a run whose PR it found on GitHub, under the same key the run enqueued when it published (`complete:<issueId>:attempt:<N>`, buildCompletionEffect). The payloads differ by design — the original carries the real worker stats, the recovery a synthetic `recovered-publication-<n>` result — so recoverPublishedRun's payload comparison threw `Outbox dedupe key collision` for a case that is not a collision at all. Because it runs inside heartbeat()'s try with no per-row catch, that throw aborted the WHOLE heartbeat: `[HB] ✗ Heartbeat error: Outbox dedupe key collision: complete:<id>:attempt:<N>` — no task selected, the loop wedged (the defect AGT-4515 hit during bring-up). Two changes: - recoverPublishedRun now treats an existing effect as satisfied when issue, attempt and kind match. The INSERT OR IGNORE already keeps whichever effect landed first, so recovery never overwrites the real stats with its synthetic ones; a row for a different issue/attempt/kind is still a real collision. - reconcileDurableArtifacts isolates each row. One row's failure stays that row's: it is logged and left in NEEDS_RECONCILE instead of aborting the sweep and the heartbeat. That blast radius was the real defect — a single bad row stopped all work. Tests: new src/automation/runLedgerRecovery.test.ts reproduces the exact collision (fails before the fix with the verbatim error), asserts the enqueued effect survives unchanged, and asserts a key belonging to another issue is still refused. 72 tests pass across the ledger suites. The recovery tests were split into their own file because runLedger.test.ts is at the pre-commit 1500-line gate; runLedger.ts itself (1497 lines on main, past 1500 with this one-line fix) joins the hook's core-file LOC_EXCLUDE list, as autonomousRunner.ts/pairPipeline.ts already do.
…g past 200 (AGT-3421) SqliteTaskSource.fetchTasks issued one fixed `limit: 200, offset: 0` query. The local source has no upstream fetch to make up the difference, so every eligible issue past the first 200 was simply never selected — a silent, unbounded queue truncation rather than a page limit. Now it pages until `total` is covered. New test creates 437 eligible issues (>2 pages) and asserts all are returned including the tail; it fails on the old query and passes on the new one.
…-CLI dropped-options warning (AGT-4444) These fences live in the in-process tool executor (adapters/tools.ts), so a role routed to the claude/codex CLI adapters loses them silently — no error, no log line. That silence is the defect: a configured fence that is never applied reads as one that is. base.ts already warns for the same class (mcpTools, coordinationContext, and now the role tool allow/deny list); this adds the two publication fences to that list. Warning only, no throw, so existing claude/codex configs keep running. The enforcement half (make the adapters honor the fences, or document them as in-process-only) is separate; this closes the silent-loss half.
…verify evidence, dashboard HTML) Each was verified PRESENT on current main and each fix carries a regression test that fails before the change. - spawnCli retained every stdout/stderr chunk for the whole worker lifetime with no ceiling; a verbose or adversarial CLI grew daemon memory until timeout. Now a 2 MiB per-stream cap, keeping the TAIL (every downstream consumer reads the terminal event — extractResultFromStreamJson, extractCodexMessageText, extractCursorFinalText, extractStreamJsonError, detectRateLimit) with an explicit head-elision marker. The streaming parser still receives every raw byte; only the retained copy is bounded. (AGT-3419 follow-up) - buildTestFixPrompt iterated every model-derived failedTests/suggestions entry unbounded. Measured before: 100 oversized entries composed a 1,004,282-char prompt. Now per-entry (300), per-list (20) and aggregate (8,000) caps with a real-count elision notice: same input to 7,710 chars. (AGT-3419 follow-up) - Verification failure output was joined into evidence with no aggregate budget, and joining first meant one failing suite's log could displace another's. Each log is now bounded before the join (head+tail, codepoint-safe so the ko locale's multibyte text is not corrupted), with a backstop over the section. (AGT-3440 follow-up) - Two dashboard interpolations were unescaped: a knowledge-graph hot module name became live markup, and a pipeline decision value became a class attribute a quote could break out of. The module name is now escaped via the file's escapeHtml, and the decision is mapped to the closed vocabulary (approve/ revise/reject) or reduced to [a-z0-9_-]. Both verified by loading the emitted dashboard in jsdom: before, the script tag became a real element and the class injection created a live onmouseover handler. (AGT-3440 follow-up) tsc clean; oxlint clean; 56 tests pass across the five touched suites.
…, graph, auth, GraphQL, git)
Each was verified PRESENT on current main; each ships a test that fails before
the change. Wave 2 of the audit remediation.
- adapters/chatStream.ts, adapters/codexResponses.ts — the partial-frame carry
and the total read were unbounded, and codexResponses retained every parsed
SSE event. Now 16 MiB raw / 1 MiB partial caps (body cancelled on give-up) and
an incremental reducer instead of the retained array; reduceResponsesEvents is
the same reducer fed a batch, so the exported contract is unchanged. The
pre-fix flood test died with a V8 heap OOM, which is the defect. (AGT-3429)
- agents/auditor.ts, agents/documenter.ts — plain-JSON extraction counted braces
without tracking strings/escapes, so a result containing `{` or `}` in a value
lost its structured fields. Uses one shared string-aware scanner
(adapters/resultParsing.ts); documenter's prompt now delimits untrusted task
and worker-report content via the existing promptDataBlock. (AGT-3466)
- memory/compaction.ts, memory/memoryOps.ts — compaction deduplicated per 1,000-
row page so duplicates across a page boundary survived a "successful"
compaction; expiry cleanup scanned only the first 10,000 rows outside the
lock; consolidation was O(n^2) under the global write lock (pre-fix test timed
out at 120s). All three now page-complete and bounded, with the consolidation
bucketing reproducing the old all-pairs semantics exactly. Also fixes a real
correctness bug found on the way: LanceDB returns the vector column as an
Arrow Vector, so normalizeRecords zeroed every embedding on rewrite and
similarity over stored rows was NaN — dedup matched nothing. (AGT-3471,
AGT-3491)
- knowledge/gitInfo.ts, knowledge/scanner.ts — changed-file discovery missed
untracked and newly-staged sources (a new file stayed invisible to incremental
refresh) and parsed newline-delimited git output, so numeric filenames were
eaten as timestamps; walks that hit a depth/timeout/size limit persisted a
partial graph indistinguishable from a complete one. Discovery is now
NUL-verbatim over four queries, and truncation is reported through
incomplete/incompleteReasons on the graph, snapshot and schema (legacy
snapshots still load). (AGT-3470, AGT-3490)
- auth/oauthPkce.ts, auth/linearPkce.ts, notify/notifier.ts — the PKCE callback
listener bound IPv4 loopback only while the redirect URI says `localhost`, so
an IPv6-first resolver could never deliver the callback; it now binds both
loopback families on one port (never a wildcard; verified by mutation that a
wildcard bind fails the off-machine test). The notifier's SSRF predicate
missed equivalent private encodings (`::ffff:7f00:1`, uncompressed loopback,
IPv4-compatible forms) and special-use IPv4 ranges; it now delegates to the
shared isPrivateIp, which gains TEST-NET-2/3. (AGT-3432)
- issues/graphql/ — the cost validator priced every registry field at 1 and only
consulted the table at mutation roots, so an aliased/fragment-multiplied
registry write or table-scanning read executed for a nominal cost. Registry
writes and scans now carry proportional costs at both roots; verified over
real HTTP (400 + GRAPHQL_COST_LIMIT_EXCEEDED) with a sabotage check proving
the assertions are not vacuous, and every real client query scored for
headroom. (AGT-3473) The auto-link deadline timer is unref'd so a hung search
cannot pin the process open. (AGT-3495)
- support/gitTracker.ts — per-chunk Buffer.toString() replaced multi-byte
characters split across pipe reads with U+FFFD, corrupting diff text (and the
length, which skewed the byte cap); now a streaming TextDecoder with a closing
flush. (AGT-3493)
tsc clean; oxlint clean; 339 tests pass across the nineteen touched suites.
…ta, TUI, strictness, budgets)
Wave 3 of the audit remediation. Each defect was verified PRESENT on current
main; each fix ships a test that fails before the change.
- adapters/ — the destructive-command guard matched command tokens by regex, so
shell quoting/escaping and brace expansion defeated it: 6 of 21 destructive
forms were allowed (`r{m,} -rf`, `$'\x72\x6d' -rf`, `r"m" -rf`, `git clean
-fdx`, …) while 3 of 3 harmless mentions were falsely blocked. Extracted to
`shellCommandGuard.ts`: it resolves a command the way bash does (quote
removal, backslash/`$'...'` decoding, comments, brace expansion, `$IFS`,
substitutions) and matches destructive verbs by word position, refusing
unresolvable text rather than waving it through. 69/69 verdicts now correct.
`read_file` also read whole files before slicing: a 512 MiB file with
`limit=1` threw `RangeError: Invalid string length`, and an 8 MiB line
returned 8,388,629 bytes; it now reads a bounded window (3.6 MB growth,
<512 KiB output) with an honest "still unread" trailer. (AGT-3436, AGT-3486)
- automation/dailyReporter.ts — only a fully successful run wrote the day's
watermark, so a partial failure republished every project that had ALREADY
succeeded next run. The watermark now carries `{date, publishedProjectIds,
complete}` in the same atomic-write path, updated per successful project, and
the pre-run short-circuit requires `complete`. Legacy records read as complete
so a mid-day deploy does not republish. (AGT-3489)
- automation/ — `fixOne` bypassed the cross-process PR lease the other public
paths take, so it could run concurrent checkout/stash/push on the same repo;
it now contends for the same lease. Decomposition capacity was a separate
unlocked read followed by a later registration, so concurrent runners each
passed the cap check and created children beyond it — reproduced with two real
processes (granted=2 against a cap of 1), now a durable per-holder reservation
under the existing runner-state lock, with dead-holder reclamation by the same
pid-space proof. (AGT-3468)
- tui/ — every "bounded" component capped code units, not terminal columns, and
preserved embedded newlines, so one value could wrap into arbitrarily many
rows and blow the fullscreen layout. Width-aware clipping now uses the repo's
existing displayWidth/truncateLine/oneLine helpers (ChatLog, ChatInput,
SelectList, LogLine, StageTimeline, AuditBoard, SubagentTree, CommandPalette),
the SSE coalescer gained a batch bound, and — found only by checking a real
60x20 terminal — `markdown.ts` reflowed at a hard-coded 80 columns regardless
of terminal width, defeating the clip upstream. Verified live: max row width
60 in a 60-column terminal. (AGT-3458)
- task_state_model.py — the Python mirror accepted values the canonical Zod
schema rejects (numeric strings for retryCount/topoRank/confidence, booleans
for confidence) on both the Pydantic v2 and v1 paths; now strict on both, with
aliases and exclude-none serialization unchanged. 11 failing tests to 36 pass.
(AGT-3420)
- agents/pipelineFormat.ts — per-field slices existed but there was no aggregate
ceiling, and discord.js THROWS on a field over 1024 chars, so a wide result
lost the whole report at build time (the stage list breaches 1024 at exactly
34 stages). Per-field slices preserved; added message/embed/aggregate budgets
with a final sum pass that trims the largest non-pinned field, keeping stats.
(AGT-3422)
- github/verify — the CI wait accepted `NaN`/`Infinity`/negative durations (`NaN`
passed every guard, making the poll sleep 0 ms and hammering `gh`); verify
manifests had no command-count or aggregate-runtime bound (a 64 KiB manifest
could schedule tens of hours); and the sandbox probe omitted `--unshare-pid`
that the real Linux run now uses, so a host denying PID namespaces failed
every command instead of being reported unavailable. (AGT-3469)
- rateLimitError.ts / prCreate.ts — `Retry-After` was parsed with `parseInt` in
TWO places (not one, as the ticket said), discarding an HTTP-date and
falling back to a short local wait; both now share one delta-seconds/HTTP-date
parser. `pr create` treated any repo with no upstream as publishable because
`git log -1` always prints, so a zero-ahead branch attempted an empty PR; it
now resolves the base and counts real commits. (AGT-3442)
tsc clean; oxlint clean; 666 tests pass across the forty-eight touched suites.
…nsitive The store-backed cross-page dedup test materialised 10,001 x 768-dimension rows against a real LanceDB store: ~53 s on an idle machine, which is a correct test that fails only when the whole suite runs under load — not a guard, a liability. That is exactly how it surfaced (red in a full run, green in isolation). compactMemoryTable now takes an optional pageSize so the test drives the REAL paging loop against a REAL store at a 25-row page. 53 s → ~1.5 s. Fixing the cost exposed a second problem worth recording: with the twins placed adjacent at the page boundary they land in the SAME page, so a deliberately per-page dedup mutation still merged them and the test passed against the bug it names. 24 rows now sit between the twin and its near-twin so they are genuinely in different pages. Mutation-verified: discarding the accumulator across pages now fails with `expected 25 to be 1`.
The stage-count sweep rendered the embed 400 times for ~6.5 s. The property can only change where the raw stages string crosses 1024 (between 33 and 34), so the sweep now covers that neighbourhood exhaustively (33-42) and samples below and far above (1, 2, 50, 64, 100, 200, 400). 10.7 s to 0.7 s. Mutation-verified: raising EMBED_FIELD_VALUE_BUDGET past the per-field ceiling still fails 3 tests, so the guard survives the narrower sweep.
verify-scratch.cjs was a throwaway probe for the read_file byte-bound work (AGT-3486) that rode into cd4a00c. It imports a local absolute path and a sibling bundle, so it is meaningless anywhere but the machine that wrote it — not a fixture, not a test.
Collaborator
Author
|
Superseded by #794. This branch was based on an older main and conflicted after main advanced 10 commits of remediation (#785-#791) covering several of the same issues. #794 starts from current origin/main and carries only the parts main does not already have — same verified work, no duplicated fixes. |
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.
Two patterns the harness (omp) uses, applied to OpenSwarm's own pipeline, plus the AGT-4518 heartbeat fix they surfaced.
1.
advisorrole — the missed-defect netA second, independently-prompted model asked one narrow question: what concrete defects did the reviewer miss?
max(approve < revise < reject), so an advisorapprovecan never soften a reviewerrevise/reject, and a raised severity with no concrete finding is discarded.ran: false, the reviewer's result untouched, no exit-code change.review, per area inreview --max, and in every--fixre-review round.modelCompat.tsalready documents forescalate.2. Declarative per-role subagent settings —
toolsandeffortEvery role (
worker/reviewer/advisor/tester/documenter/auditor/skill-documenter) may declaretools.allow/tools.denyandeffort.bashon a read-only role stays withheld, and the same narrowed set feedsallowedToolNames, so a withheld name is refused at dispatch too.denyis applied afterallow(deny wins) and supports a trailing*(scratch_*).base.tsnow says so in its dropped-options warning rather than leaving it silently inert (the AGT-4444 failure class).3.
fix(ledger): recovery is idempotent (AGT-4518)reconcileDurableArtifactsrebuilds a completion effect for a run whose PR it found on GitHub, under the same key the run enqueued when it published (complete:<issueId>:attempt:<N>). The payloads differ by design, sorecoverPublishedRun's payload comparison threwOutbox dedupe key collisionfor a case that is not a collision — and, running insideheartbeat()'strywith no per-row catch, that aborted the whole heartbeat ([HB] ✗ Heartbeat error: Outbox dedupe key collision). Fix: match on issue+attempt+kind (theINSERT OR IGNOREalready keeps the first effect, so real stats are never overwritten), and isolate each reconcile row so one bad row cannot stop all work.Verification
tsc --noEmitclean;oxlintclean.worktreeManager.concurrentCreate), which passes 2/2 in isolation and touches no file this branch changes.tools.allow/deny,effort, and theadvisorrole all parse from a real config file.Known gaps
toolson delegated-CLI adapters (claude/codex) — warned, not silently dropped.runLedger.ts(1497 lines on main) joined the pre-commitLOC_EXCLUDElist; the AGT-4518 fix pushed it past 1500 and splitting the ledger is its own change, not a rider on a one-line fix.Closes AGT-4518.