Skip to content

fix: keep the update no-op check working on a trimmed status line#484

Open
bosenstrong (ddy4633) wants to merge 1 commit into
langchain-ai:mainfrom
ddy4633:fix-update-noop-trimmed-status-line
Open

fix: keep the update no-op check working on a trimmed status line#484
bosenstrong (ddy4633) wants to merge 1 commit into
langchain-ai:mainfrom
ddy4633:fix-update-noop-trimmed-status-line

Conversation

@ddy4633

Copy link
Copy Markdown

The bug

runGit() (src/agent/utils.ts:466-488) trims its whole output. git status --short reports an unstaged-only change with a leading space — " M openwiki/.last-update.json" — because the X column is empty and Y is M. Trimming the block eats that space on the first line, so isUpdateMetadataStatusLine()'s fixed line.slice(3) cut one character into the path:

" M openwiki/.last-update.json"  →  trimmed  →  "M openwiki/.last-update.json"
                                    slice(3)  →  "penwiki/.last-update.json"

That never equals UPDATE_METADATA_PATH, so the line counted as a meaningful worktree change and getUpdateNoopStatus() returned {shouldSkip: false, reason: "worktree has changes"}.

Why it matters

The mis-slice only happens when the metadata line is the first dirty entry — which is precisely the situation the no-op check exists to catch.

openwiki/.last-update.json is committed to the repo (this repo's own git ls-files openwiki lists it) and its timestamp is rewritten on every run. So for any user who commits their wiki, the worktree is permanently dirty on exactly that one file, and openwiki --update ran a full agent every single time instead of skipping. That is real token spend, and a regression of the behaviour #44 added.

Reproduced against a scratch git repo before patching:

{"shouldSkip":false,"reason":"worktree has changes"}   ← expected shouldSkip: true

After:

{"shouldSkip":true,"gitHead":"c3e79ef...","model":"m"}

The fix

Parse the status field with a pattern instead of a fixed offset. The greedy {1,2} handles both the untrimmed forms (" M path", "?? path", "R old -> new") and the trimmed first line.

Design question for you — I may have picked the wrong layer

The actual pollution is runGit()'s .trim(). Changing it to .trimEnd() is a one-character fix at the source and arguably more correct.

I did not do that because runGit's output feeds createGitSummary(), so it would change the text of every git section handed to the model — a much wider blast radius for a bug that shows up in one consumer. If you'd rather fix the source and let the prompt text shift, say so and I'll redo it that way.

How to test

pnpm test          # 54 files, 675 tests passed
pnpm run typecheck # clean

One test added to test/update-noop.test.ts: it commits .last-update.json first, then rewrites it, producing a genuine " M openwiki/.last-update.json" status line. No network, no API keys.

Regression proof — source reverted, test kept:

FAIL test/update-noop.test.ts > getUpdateNoopStatus > detects a no-op when only the committed run metadata is dirty
AssertionError: expected false to be true
Tests  1 failed | 8 passed (9)

Restored → 9 passed (9).

Reviewer notes

  • The status character class [ !?ACDMRTU] is the porcelain-v1 alphabet written out by hand. If git ever adds a status character, that line falls back to trimming the whole line — i.e. the old behaviour, which is conservative (does not skip), never a false "this is the metadata line".
  • Quoted paths containing spaces (?? "a b.md") are captured with their quotes, but the comparison against UPDATE_METADATA_PATH is exact equality, so there is no false match. Same as before this change.
  • The new test shells out to real git, consistent with the 8 existing tests in that file.

Unrelated findings

While reading pure-logic modules I noticed three things that look like genuine bugs but belong in their own PRs — happy to file them as issues:

  • src/okf/index-sync.ts:189-198existing ? edit : write is a truthiness check rather than !== null. An empty index.md reads back as "" → takes the write branch → the deepagents backend rejects "file already exists" → the throw escapes okf-middleware's afterAgent and fails the run after the model finished its work.
  • src/schedules.ts:151-159 — when a cron expression is too complex for launchd the function returns early without unloading the existing launch agent, so the old schedule keeps firing and the user's reschedule silently does nothing.
  • src/okf/frontmatter.ts:23FRONTMATTER_BLOCK does not match an empty front-matter block (---\n---), so normalizeConceptContent writes the old block into the body a second time.

runGit() trims its whole output, which strips the leading space of the
first line of `git status --short`. An unstaged-only change is reported as
" M openwiki/.last-update.json", so after trimming the first line reads
"M openwiki/.last-update.json" and the fixed `line.slice(3)` cut one
character into the path.

isUpdateMetadataStatusLine() then failed to recognise the metadata path and
getUpdateNoopStatus() reported "worktree has changes".

This only misfires when the metadata line is the first dirty entry — which
is exactly the case the no-op check exists for. openwiki/.last-update.json
is committed to the repo and rewritten on every run, so for any user who
commits their wiki, `openwiki --update` ran a full agent every time instead
of skipping. That is real token spend, and a regression of what langchain-ai#44 fixed.

Parsing the status field with a pattern instead of a fixed offset handles
both the untrimmed " M path" / "?? path" / "R  old -> new" forms and the
trimmed first line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant