fix(agent-inbox): number resolve against file position, not the pending view - #2739
fix(agent-inbox): number resolve against file position, not the pending view#2739davidnunez wants to merge 1 commit into
Conversation
|
Welcome to career-ops, @davidnunez! Thanks for your first PR. A few things to know:
We'll review your PR soon. Join our Discord if you have questions. |
📝 WalkthroughWalkthroughThe inbox CLI now assigns stable full-file positions, preserves gaps after resolution, rejects invalid or completed targets, and supports case-insensitive ChangesInbox resolution safeguards
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent-inbox-tests.mjs`:
- Around line 169-171: Update the list-output assertions in the relevant inbox
test to leave pending items after resolved items and verify that they retain
their original numbers, including visible gaps such as items 3 and 5. Do not
only assert item 1; preserve the existing resolved-items-hidden assertion.
In `@agent-inbox.mjs`:
- Around line 139-155: Make the resolve operation atomic by wrapping its
re-read, validation, update, and inbox-file replacement in the repository’s
shared lock, using the existing lock mechanism rather than adding a new one.
Move the already-resolved and --expect checks to operate on the locked re-read,
and update add to acquire that same lock so concurrent writes cannot overwrite
each other.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 46fe0bf9-890c-4c34-9331-0eba33d2dd36
📒 Files selected for processing (4)
agent-inbox-tests.mjsagent-inbox.mjsdocs/SCRIPTS.mdmodes/agent-inbox.md
| const items = parseItems(); | ||
| const target = items[n - 1]; | ||
| if (!target) { | ||
| const pending = items.filter((it) => !it.done).length; | ||
| fail(`no item #${n} — inbox has ${items.length} item(s), ${pending} pending. Run \`list --all\`.`); | ||
| } | ||
| // Already-resolved is an error, not a silent re-stamp: it is what a stale | ||
| // number from an older `list` most often lands on. | ||
| if (target.done) fail(`item #${n} is already resolved — refusing to overwrite it:\n #${n}: ${target.text}`); | ||
| // Optional caller-side guard: abort unless the target says what the caller | ||
| // thinks it says. Catches "right command, wrong target" generally. | ||
| if (hasOpt('expect')) { | ||
| const expect = opt('expect'); | ||
| if (!expect) fail('--expect needs a substring, e.g. --expect "Dana-Farber"'); | ||
| if (!target.text.toLowerCase().includes(expect.toLowerCase())) { | ||
| fail(`item #${n} does not contain --expect ${JSON.stringify(expect)} — refusing to resolve:\n #${n}: ${target.text}`); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the resolve write atomic.
Two resolve processes can parse and validate the same unchecked item before either process writes. The last writeFileSync() then discards the other result. The already-resolved and --expect checks do not prevent this race.
Use the repository shared lock pattern. Re-read, validate, update, and atomically replace the inbox file while that lock is held. Ensure add uses the same lock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent-inbox.mjs` around lines 139 - 155, Make the resolve operation atomic by
wrapping its re-read, validation, update, and inbox-file replacement in the
repository’s shared lock, using the existing lock mechanism rather than adding a
new one. Move the already-resolved and --expect checks to operate on the locked
re-read, and update add to acquire that same lock so concurrent writes cannot
overwrite each other.
…ng view `resolve N` indexed into the *pending* subset, so every resolve shifted all higher numbers down by one. Reading `list` once and firing a batch of resolves off that snapshot stamped results onto the wrong items — silently, with no error. Number against the full item list instead. `add` only ever appends, so a file position never changes meaning once printed; `list` now shows gaps as items resolve and prints a footer so the gaps don't read as a display bug. Two guards on the write path: re-resolving a done item aborts instead of overwriting its result (that is where a stale number most often lands), and --expect "<substring>" aborts unless the target contains that text. A valueless --expect fails rather than silently disabling the guard. Tests 8-10: batch-of-resolves regression against one snapshot, re-resolve refusal, and --expect mismatch/valueless/case-insensitive-match. Rebased onto main after santifer#2614 merged; the atomic-append fix is preserved and this change touches only parseItems/list/resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FxADPBJCghNfm5jPZ2Phhh
ce2210a to
e49d714
Compare
|
Rebased onto The rebase preserves #2614 in full; this PR touches only
Also trimmed the description: it previously cited a file that isn't part of this repository as prior art. The reasoning stands without it. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/SCRIPTS.md (1)
818-842: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDo not hardcode ranking metrics in this Markdown file.
The section embeds the
/5scale, the default and maximum limits, and the CV excerpt size. Generate these values from the canonical configuration or runtime help output so the documentation does not drift from evaluation behavior.As per coding guidelines,
**/*.{md,html,tex}says: “NEVER hardcode metrics -- read them from these files at evaluation time.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/SCRIPTS.md` around lines 818 - 842, Update the ranking documentation to avoid hardcoded evaluation metrics, including the /5 scale, default and maximum --limit values, and CV excerpt length. Generate these values from the canonical ranking configuration or runtime help output, following the repository guidance for Markdown metric values, so the documented behavior stays synchronized with evaluation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent-inbox.mjs`:
- Around line 190-192: Update the --expect handling in the resolve option
parsing flow so every occurrence is validated, rather than relying on
opt('expect') reading only the first value. Reject any valueless --expect,
including trailing duplicates, or reject duplicate --expect options before
resolving the target while preserving valid single-option behavior.
---
Outside diff comments:
In `@docs/SCRIPTS.md`:
- Around line 818-842: Update the ranking documentation to avoid hardcoded
evaluation metrics, including the /5 scale, default and maximum --limit values,
and CV excerpt length. Generate these values from the canonical ranking
configuration or runtime help output, following the repository guidance for
Markdown metric values, so the documented behavior stays synchronized with
evaluation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6f9672e5-eae0-44cf-8585-15aa150dbe88
📒 Files selected for processing (3)
agent-inbox-tests.mjsagent-inbox.mjsdocs/SCRIPTS.md
| if (hasOpt('expect')) { | ||
| const expect = opt('expect'); | ||
| if (!expect) fail('--expect needs a substring, e.g. --expect "Dana-Farber"'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject every valueless --expect occurrence.
opt('expect') reads only the first occurrence. Therefore, resolve 1 --expect "valid" --expect --result "..." succeeds instead of rejecting the trailing valueless option.
Validate all occurrences, or reject duplicate --expect options before resolving the target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent-inbox.mjs` around lines 190 - 192, Update the --expect handling in the
resolve option parsing flow so every occurrence is validated, rather than
relying on opt('expect') reading only the first value. Reject any valueless
--expect, including trailing duplicates, or reject duplicate --expect options
before resolving the target while preserving valid single-option behavior.
|
Heads-up, @davidnunez: this conflicts now and the cause is mine. Last night I pushed a fix to Don't rebase yet. Your change is about how |
2 participants
The bug
resolve Nindexes into the pending subset:Every resolve removes an item from that subset, so every higher number shifts down by one.
Read
listonce, then act on what you read, and the results land on the wrong items:Verbatim run against
main, five queued items,listread once:Resulting queue:
Three distinct failures from one snapshot:
charlieis silently skipped,deltais stampedwith a result it did not produce, and the third command dies with
no pending item #4 (3 pending)moments afterlistdisplayed five items. The second is the dangerous one — itprints
Resolved #3and exits 0.Why this is the designed usage, not misuse
modes/agent-inbox.mdtells the agent to drain the queue top-to-bottom and mark each item asit goes:
An agent reads
listonce into context, works the batch, and resolves against the numbers itwas shown. Re-running
listbetween every resolve is the only safe pattern under the oldbehavior, and nothing in the docs said so — the code comment claimed the opposite ("so
listthen
resolve Nline up"), which is true for exactly one resolve.The failure mode is the worst kind for this file:
data/agent-inbox.mdis a provenance log.A wrong result line is indistinguishable from a right one after the fact.
The fix
Number against the full item list.
addonly ever appends, so an item's file position neverchanges meaning once printed, and a batch read off one
liststays correct regardless oforder or how many land in between.
listkeeps showing pending-only by default, so numbers now have gaps (1, 3, 5) as itemsresolve. That reads as a display bug unless you say otherwise, so
listprints a footer:Two guards on the write path
A stale number from an older
listis the residual risk, so the write refuses in the twocases where it is most likely wrong:
[x]item aborts instead of overwriting its result. This iswhat a stale number most often lands on, and silently re-stamping destroys the earlier
provenance line.
--expect "<substring>"aborts unless the target item contains that text(case-insensitive). Turns "right command, wrong row" into an error. A valueless
--expectfails rather than silently disabling the guard.
Both print the offending item's text, so the error shows you what you almost hit.
modes/agent-inbox.mdnow instructs the draining agent to pass--expectevery time.Happy to split
--expectinto a follow-up PR if you'd rather keep this one purely a fix —it's bundled because it's the hardening for this specific failure, and the flag is opt-in and
backward-compatible.
Tests
agent-inbox-tests.mjsgains three cases:listonce, then fire fourresolves against that snapshot with no re-listing. Asserts each result landed on the item it
named, the survivor kept its original number, and the footer explains the gap. Fails against
the old code with results on the wrong items.
--expectmismatch exits 1 and writes nothing; valueless--expectexits 1 andwrites nothing; a case-insensitive match resolves normally.
Plus a
runFailhelper for exit-code assertions.node agent-inbox-tests.mjs→ 33 passed, 0 failed (16 onmain).node test-all.mjson a clean checkout of this branch → 3429 passed, 0 failed, 2 warnings(both environmental: no user
cv.md, and no Go compiler for the dashboard build).Relationship to #2614
#2614 (open) fixes a different bug in the same file — concurrent
addlosing items — andtouches only
addand its test. No logical overlap with this change; a textual conflict ispossible if both land. Happy to rebase on top of it, in either order.
Docs
docs/SCRIPTS.mdgains rows forlistandresolve(neither was registered).Summary by CodeRabbit
New Features
--expectvalidation when resolving items.Bug Fixes
Documentation