Skip to content

Add --write to migrate article-ids - #191

Open
ArmelRandy wants to merge 1 commit into
facebookresearch:mainfrom
ArmelRandy:feat/migrate-article-ids-write
Open

ArmelRandy wants to merge 1 commit into
facebookresearch:mainfrom
ArmelRandy:feat/migrate-article-ids-write

Conversation

@ArmelRandy

Copy link
Copy Markdown
Contributor

work list refuses formalizable leaves without an article_id. Today the way
out is to run migrate article-ids --json and copy each id into frontmatter by
hand, and the Formalize skill tells the agent to do exactly that. On a project
with 53 articles, that is 53 edits.

autoform migrate article-ids <blueprint> --write adds each planned id as the
first frontmatter line of the article that lacks one, or as a frontmatter block
of its own when the article has none. Nothing else in the file changes.

  • An article is written only while its bytes still have the hash the plan was
    made from. Every article is checked before the first write, and again just
    before its own replacement, so an article edited after planning is refused
    rather than overwritten. After a refusal, running it again completes the plan.
  • Writes go through a temporary file in the same directory and a rename. The
    file mode and line endings are kept. Symlinks are refused.
  • --write cannot be combined with --check. With --json, stdout still
    carries only the plan, and the added ids go to stderr.
  • The Formalize and Roadmap skills, the work list error and the README now
    point to --write.

Checked on copies of two consumer projects (53 and 12 articles): the ids
written are the ones --json planned, check gives the same result before and
after, and a second run changes nothing.

🤖 Generated with Claude Code

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 7, 2026
work list refuses formalizable leaves without an article_id, and the
Formalize skill tells the agent to copy each planned id into frontmatter by
hand. `migrate article-ids --write` adds each planned id as the first
frontmatter line of the article that lacks one, or as a frontmatter block of
its own, and changes nothing else.

An article is written only while its bytes still have the hash the plan was
made from. Every article is checked before the first write and again just
before its own replacement, so a concurrent edit is never overwritten, and
running the command again completes the plan. Writes go through a temporary
file in the same directory and a rename, keep the file mode and line
endings, and refuse symlinks. --write cannot be combined with --check; with
--json, stdout carries only the plan.

The Formalize and Roadmap skills, the work list error and the README now
point to --write.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Deicyde

Deicyde commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Review at 7481b64

Thanks for this. --write replaces the hand edits that work list forces today, and it finishes the follow-up the README named. I removed every article_id line from copies of Hartshorne (1792 articles), Hatcher (282) and the example (10), then ran --write. Every file came back byte-for-byte identical apart from the ID value, parsed fields were unchanged, the IDs written match what --json planned, and a second run wrote nothing. I also reproduced each case below on scratch copies and ran mutation tests on the new code. The PR merges cleanly onto main 8575a15, and CI is green. Two small fixes before merge. First, the opener search disagrees with the loader, so an article with CR-only line endings silently loses its frontmatter. Second, the reload after the writes can hide what was written. Four tests and three wording nits finish it off.

Should fix

1. An article with CR-only line endings loses its frontmatter (autoform_cli/article_identity.py:182)

_with_article_id finds the opener with text.partition("\n"), but the loader splits lines with str.splitlines() (autoform_cli/graph.py:384). splitlines() also ends a line at a lone \r, at NEL (U+0085) and at U+2028. When the opener line ends in one of those, the loader sees a frontmatter block but _with_article_id does not. It prepends a second block, and every key in the original block becomes body text.

Repro: roadmap/chapter/result.md is b"---\rnot_ready: true\r---\r\r# Result\r", and check reports 2 ready to state · 1 not ready. migrate article-ids --write prints OK: 3 articles have durable article_id metadata and exits 0. The file becomes b"---\r\narticle_id: af_6aab49995ec41e83a980fe4b\r\n---\r\n\r\n---\rnot_ready: true\r---\r\r# Result\r", and check now reports 3 ready to state. CR-only files are rare, but the loss is silent, and declaration: or statement: formalized would be dropped the same way.

Fix: find the opener the way the loader does.

    text = content.decode("utf-8")
    # Split as graph._parse_node does, so both agree on where the frontmatter is.
    first = text.splitlines(keepends=True)[0] if text else ""
    opener = first.splitlines()[0] if first else ""
    ending = first[len(opener):]
    newline = ending if ending in ("\r\n", "\r") else "\n"
    line = f"article_id: {article_id}{newline}"
    if opener.strip() == "---" and ending:
        return (first + line + text[len(first):]).encode("utf-8")
    return (f"---{newline}{line}---{newline}{newline}" + text).encode("utf-8")

I checked this against _parse_frontmatter on 980 generated inputs, covering whitespace around the opener, a leading form feed, and every line break splitlines knows. In every case the loader reads the output as the original metadata plus article_id, followed by the original body. On files whose line breaks are all LF or CRLF, the output is byte-identical to the current code, and the three blueprints round-trip exactly as above. A test that fails at this head and passes with the fix:

def test_write_reads_frontmatter_lines_as_the_loader_does(tmp_path: Path) -> None:
    blueprint = _blueprint(tmp_path)
    result = blueprint / "roadmap/chapter/result.md"
    result.write_bytes(b"---\rnot_ready: true\r---\r\r# Result\r")

    written = {entry.article_path: entry.article_id for entry in write_article_ids(blueprint)}

    article_id = written["roadmap/chapter/result.md"]
    assert result.read_bytes() == f"---\rarticle_id: {article_id}\rnot_ready: true\r---\r\r# Result\r".encode()
    assert load_graph(blueprint).nodes["chapter/result"].not_ready

2. The reload after the writes can hide what was written (autoform_cli/article_identity.py:151-160)

That reload can fail in two ways that bypass ArticleIdWriteError.written. Both need another process to edit the blueprint during the run, which is the case the hash checks are there for.

  • An edit elsewhere breaks the blueprint. plan_article_ids then raises a plain GraphValidationError, and _migrate prints only that error. Repro: after the third replacement, an article that already has an ID gains - [x](missing.md) under ## Depends on. The run exits 2 with empty stdout and only error: chapter/other: dependency target does not exist: 'missing.md' on stderr. Meanwhile, three articles now carry an article_id that nothing reported.
  • An article is deleted after it was written. after[entry.path_id] then raises KeyError, and the CLI exits with a traceback, again listing nothing.

Fix: delete lines 151-160 as part of the same change as item 1. Once the write agrees with the loader, the reload can only catch concurrent edits, and _migrate already calls plan_article_ids right after printing the added article_id lines. In the first case the CLI then prints the three added lines before the error and exits 2. In the second it prints them before OK: 2 articles have durable article_id metadata. This also saves one of the three graph loads. Do not drop the reload before item 1 lands: at this head it is what catches a write that lands outside the loader's frontmatter (after a leading form feed, it reports article_id did not load back as written). A test that fails at this head and passes once the lines are gone:

def test_cli_lists_written_articles_when_the_blueprint_breaks_meanwhile(tmp_path: Path, monkeypatch, capsys) -> None:
    blueprint = _blueprint(tmp_path)
    other = blueprint / "roadmap/chapter/other.md"
    _article(other, "Other", "af_0123456789abcdef01234567")
    real_replace = article_identity._replace_if_unchanged
    calls: list[Path] = []

    def replace_then_break_a_link(path, expected, content, mode):
        real_replace(path, expected, content, mode)
        calls.append(path)
        if len(calls) == 3:
            broken = other.read_text(encoding="utf-8") + "\n## Depends on\n\n- [x](missing.md)\n"
            other.write_text(broken, encoding="utf-8")

    monkeypatch.setattr(article_identity, "_replace_if_unchanged", replace_then_break_a_link)

    assert main(["migrate", "article-ids", str(blueprint), "--write"]) == 2
    captured = capsys.readouterr()
    assert (captured.out + captured.err).count("added article_id af_") == 3
    assert "missing.md" in captured.err

Test gaps

I mutated the new code and ran tests/test_article_identity.py. Each of these mutants passes all 11 tests and changes behavior:

  1. stream = sys.stdout (autoform_cli/__main__.py:952): on a fresh blueprint, --write --json stdout no longer parses as JSON. The assertions at tests/test_article_identity.py:222-225 run after the previous call has already completed the blueprint, so captured.err == "" holds whichever stream is used.
  2. temporary.chmod(mode) deleted (autoform_cli/article_identity.py:200): an article at 0640 comes back 0600.
  3. The except ArticleIdWriteError handler disabled (autoform_cli/__main__.py:956): after a refusal mid-write, the CLI lists nothing it wrote.
  4. The byte-order-mark refusal deleted (autoform_cli/article_identity.py:134): the article gets a block prepended, the BOM ends up in the body, and the run exits 0.
  5. temporary.unlink(missing_ok=True) deleted (autoform_cli/article_identity.py:206): a refusal leaves .result.md.<random>.tmp beside the article.

These four tests catch all five mutants. Each passes at this head and fails on its mutant (add import stat):

def test_cli_write_json_keeps_stdout_for_the_plan(tmp_path: Path, capsys) -> None:
    blueprint = _blueprint(tmp_path)

    assert main(["migrate", "article-ids", str(blueprint), "--write", "--json"]) == 0
    captured = capsys.readouterr()
    assert json.loads(captured.out)["complete"] is True
    assert captured.err.count("added article_id af_") == 3


def test_write_keeps_the_file_mode(tmp_path: Path) -> None:
    blueprint = _blueprint(tmp_path)
    result = blueprint / "roadmap/chapter/result.md"
    result.chmod(0o640)

    write_article_ids(blueprint)

    assert stat.S_IMODE(result.stat().st_mode) == 0o640


def test_cli_lists_what_it_wrote_before_a_refusal(tmp_path: Path, monkeypatch, capsys) -> None:
    blueprint = _blueprint(tmp_path)
    real_replace = article_identity._replace_if_unchanged
    calls: list[Path] = []

    def edit_the_second(path, expected, content, mode):
        calls.append(path)
        if len(calls) == 2:
            path.write_text(path.read_text(encoding="utf-8") + "Edited meanwhile.\n", encoding="utf-8")
        return real_replace(path, expected, content, mode)

    monkeypatch.setattr(article_identity, "_replace_if_unchanged", edit_the_second)

    assert main(["migrate", "article-ids", str(blueprint), "--write"]) == 2
    err = capsys.readouterr().err
    assert err.count("added article_id af_") == 1
    assert "error: added 1 article_id(s) before stopping" in err
    assert not list(blueprint.rglob("*.tmp"))


def test_write_refuses_a_byte_order_mark(tmp_path: Path) -> None:
    blueprint = _blueprint(tmp_path)
    (blueprint / "roadmap/chapter/result.md").write_bytes(b"\xef\xbb\xbf---\nnot_ready: true\n---\n\n# Result\n")
    before = _bytes(blueprint)

    with pytest.raises(article_identity.ArticleIdWriteError, match="byte-order mark"):
        write_article_ids(blueprint)

    assert _bytes(blueprint) == before

With items 1 and 2 applied and all six tests added, tests/test_article_identity.py passes (17 tests) and ruff is clean.

Nits

  • autoform_cli/article_identity.py:147 and autoform_cli/README.md:735: the ; run the command again suffix and "after a refusal, running it again completes the plan" only fit an article that changed during the run. After a permission error, the rerun fails the same way (checked with chmod 555 roadmap). Move the advice into the message at :203 ("the article changed while the plan was applied; run the command again") and drop it from :147, so it appears only where it helps.
  • autoform_cli/README.md:734-735: "refuses symlinks" claims more than the code does. A symlinked article that the loader accepts (one pointing at a non-Markdown file inside roadmap/) is written through to its target. Only an article swapped for a symlink after planning is refused. "refuses an article that changed or became a symlink after planning" would match.
  • autoform_cli/__main__.py:256: autoform --help still describes migrate as "inspect authored migration contracts". Something like "plan or apply authored migrations" fits now.

Checked, no change needed

  • Refusals: an article changed after planning blocks every write, a symlink swapped in after planning is refused, an invalid blueprint writes nothing, and --write --check exits 2.
  • Comments, key order, CRLF, trailing whitespace on the opener, a missing final newline and the file mode all survive the write.
  • Refusing a byte-order mark is the right call. On main the loader already ignores frontmatter after a BOM (not_ready: true reads as False), which deserves its own issue.
  • With the PR merged into main 8575a15, test_article_identity, test_work, test_contract, test_cli and test_skill_examples pass (77 tests), and ruff is clean. The PR has small textual conflicts with Keep implementation notes separate from mathematical articles #172 (adjacent lines in skills/roadmap/SKILL.md) and Feedback form #195 (the import line in __main__.py).

Posted by PR swarm: PR Swarm Lead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants