[LivXue]

Summary

The loader accepts a null skillForge section, but block/unblock either crashed or created a second section under the other spelling. Treat null as an empty section for a write, retain the existing spelling, and refuse other invalid section types without overwriting them.

The CLI regression checks block/unblock, case-insensitive idempotence, a loadable result and an absent unblock that leaves the original bytes unchanged. The unblock output must not name a key the write did not use.

Type

• Fix
• Feature
• Docs
• CI / tooling
• Refactor
• Other

Verification

• Relevant tests pass locally
• Relevant lint / type checks pass locally
• User-facing docs or screenshots are updated when needed

Native Windows, Python 3.12. PYTHONUTF8=1; the existing environment was selected through UV_PROJECT_ENVIRONMENT, and PYTHONPATH pointed at this checkout.

uv run --no-sync pytest -o addopts= tests/test_config_update.py tests/test_cli_skill_commands.py -k "null_section or invalid_section or skill_unblock_removes" -q --tb=short --show-capture=no

Result: 10 passed in the targeted selection. The broader two-file selection previously produced 89 passed and 4 failed at the pre-existing POSIX permission assertions in config-update tests. Those failures were reproduced in the main audit; the affected permission-writing code is unchanged. The relevant-tests box remains unchecked for the broader selection.

Whole-branch gates:

$changed = @(git diff --name-only origin/main...HEAD -- '*.py')
$production = @($changed | Where-Object { $_ -like 'raven/*' })
uv run --no-sync ruff check @changed
uv run --no-sync ruff format --check @changed
uv run --no-sync ty check @production
uv run --no-sync lint-imports
uv run --no-sync pre-commit run --from-ref origin/main --to-ref HEAD
uv run --no-sync python scripts/check_commit_messages.py origin/main..HEAD
uv run --no-sync python scripts/check_source_language.py origin/main...HEAD
uv run --no-sync python scripts/check_large_files.py origin/main...HEAD
git diff --check origin/main...HEAD

All listed gates passed. The whole-branch review covered callers, ownership, guard boundaries, failure paths, regression strength, architecture and compatibility. No screenshot change applies; directly affected code documentation is updated where needed. The full repository suite and Linux/macOS execution were not run locally; the local Docker Linux engine was unavailable.

Risk

• Security impact considered
• Backward compatibility considered
• Rollback path is clear for risky changes

Only an absent or null selected section is initialized. Other invalid types fail without rewriting the file. Existing dictionaries, spelling precedence and no-op behavior remain intact. Revert the squash commit to roll back.

Related Issues

#500

[gloryfromca]

No blockers; this can merge as far as I am concerned.

Reviewed the full github/main...HEAD diff, the set_skill_blocked callers, Raven/base loader behavior, the prior key-spelling fix and relevant history, backward compatibility across camelCase and snake_case inputs, and the added tests for signs of weakened coverage. I also checked the applicable AGENTS.md rules and CONTEXT-MAP.md architecture routing; no project-rule or architecture conflict was found.

Verification:

• uv run --all-extras pytest tests/test_config_update.py tests/test_cli_skill_commands.py -x -> 93 passed
• uv run --all-extras pytest tests/test_config_raven_sections.py -x -> 38 passed
• git diff --check github/main...HEAD -> clean

The first run without --all-extras stopped in autouse fixture setup because raven_everos was unavailable (4 setup errors); rerunning with the project extras reached and passed all focused tests.

[LivXue]

Windows compatibility review: no new issue found in the inspected scope.

Reviewed head 58cbcb4c9454a8e78c378dcd2217b93262a5d1d6 against base 4acc63cebd0752652194f9e4989628da29a8a25d: all 3 changed files, the block/unblock CLI callers, and the existing UTF-8 atomic-update path. Checked null-section initialization, spelling preservation, case-insensitive no-ops, and refusing invalid sections without rewriting the file.

Native Windows / Python 3.13.12, in a detached temporary checkout with PYTHONUTF8=1, UV_NO_SYNC=1 and isolated user/Raven homes:

uv run pytest tests/test_config_update.py tests/test_cli_skill_commands.py -k 'skill_block or skill_unblock' -n 0 -x --tb=short --basetemp "$reviewRunDir/pr914/tests"

Result: 18 passed, 75 deselected. The remaining configuration/CLI tests and concurrent readers holding files open were not exercised. This scoped review does not establish full Windows compatibility.

[LivXue]

No blockers in the reviewed scope.

Scope: I reviewed the full PR diff (3 changed files), including tests and documentation, and traced affected callers and contracts at 58cbcb4c9454. This review covers all changed areas, not only raven/agent.

Checked camelCase and snake_case selection, null and absent sections, malformed-section preservation, case-insensitive idempotence, atomic no-op writes, CLI behavior, and load_raven_config readback. An absent unblock does not rewrite a null section; an invalid non-object section is refused without overwriting it.

Verification (native Windows, Python 3.12, isolated source snapshots; local JUnit-output options omitted below):

• PR head: uv run --no-sync pytest -o addopts= tests/test_config_update.py tests/test_cli_skill_commands.py tests/test_config_raven_sections.py -q --tb=short -> 127 passed, 4 failed.
• merge-base control: uv run --no-sync pytest -o addopts= tests/test_config_update.py tests/test_cli_skill_commands.py tests/test_config_raven_sections.py -q --tb=short -> 119 passed, 4 failed.

The four failing IDs are unchanged Windows permission-mode tests and reproduce at the merge base. The null-section, spelling, invalid-section, and no-op cases passed. No new finding was identified. I also read the Python 3.13 Windows review. Its conclusions are consistent; the measurements here record a separate Python 3.12 run.

git diff --check over the complete merge-base-to-head range passed. No new inline finding is being filed.

[gloryfromca]

No blockers; this can merge as far as I am concerned.

The source tree is byte-identical to the previously reviewed clean revision: both commits have tree d30bf2d514d63f523977c4d0f82cb23e6d91c2a5 and parent 4acc63cebd0752652194f9e4989628da29a8a25d. The only change is removal of the Co-authored-by: Codex trailer. The rewritten commit remains English, ASCII-only, conventionally formatted, and within the repository's commit rules.

I did not rerun tests because no source or test byte changed; the prior 93 focused and 38 surrounding-config passing results apply to this exact tree. git diff --check github/main...HEAD remains clean.

[LivXue]

Windows compatibility re-review: no new issue found in the inspected scope.

Reviewed the complete 3-file PR diff at 986cc6efc4dc3201104c34e2d9e33209cc71cc07; API base 829eb28e1db877dffdd3df4f835d707e1f049d3a, merge base 4acc63cebd0752652194f9e4989628da29a8a25d. Checked null/absent/invalid skill sections, camelCase/snake_case preservation, case-insensitive no-ops, CLI block/unblock callers and the existing UTF-8 atomic-update path.

Git checks: source tree identical to previously reviewed 58cbcb4c9454a8e78c378dcd2217b93262a5d1d6; diff whitespace check passed and the merge-tree check against the recorded API base returned exit 0.

Native Windows / Python 3.13.12, detached checkout, PYTHONUTF8=1, UV_NO_SYNC=1, isolated user/Raven/temp directories. Reused a uv environment with matching pyproject.toml/uv.lock; PYTHONPATH selected this checkout and its bundled plugin roots, and the loaded Raven source path was asserted.

PR head:

uv run pytest 'tests/test_config_update.py' 'tests/test_cli_skill_commands.py' -k 'skill_block or skill_unblock' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\fa39f19b3fcb4d7daa4ad3b9472340f8\pr914\unit-pytest'
# 18 passed, 75 deselected in 8.81s; exit 0

Limits: The remaining configuration/CLI tests, concurrent readers holding files open and the full repository suite were not exercised. This scoped result does not establish full Windows compatibility.

[0xKT]

Not a blocker -- board acceptance of #914 at head 986cc6e merged onto main 829eb28: graded A. There are two small follow-ups. I reproduced both on that merged tree.

Severity of this one finding, not a verdict on the pull request. The blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.

1. The null-section rule lives in one of the two writers of skillForge.blocklist. Take a config of {"skillForge": null}:

• set_skill_blocked("zed", True) now succeeds and writes {"skillForge": {"blocklist": ["zed"]}}.
• settings.set with {"key": "skillForge.blocklist", "value": ["zed"]} still answers ConfigValidationError: config path skillForge.blocklist blocked by non-object and leaves the file unchanged. _write_raw_key (raven/rpc/methods/console.py:765-787) does node.setdefault(..., {}), which returns the existing None.

Object and absent sections succeed through both writers. Both refused this file on main, so nothing regressed, but the two writers now disagree about the same file.

2. The new ValueError on the unblock path is untested. I made the raise conditional on blocked (elif not isinstance(section, dict) and blocked:). That mutant still passes tests/test_config_update.py and tests/test_cli_skill_commands.py (93 passed), and unblock on an invalid section then no longer raises the ValueError. Parametrizing the existing test over blocked catches it: 6 of 12 cases fail under the mutant, and all 12 pass on the head.

@pytest.mark.parametrize("blocked", [True, False])
@pytest.mark.parametrize("spelling", ["skillForge", "skill_forge"])
@pytest.mark.parametrize("invalid", [[], 1, "invalid"])
def test_skill_block_preserves_an_invalid_section(cfg_path, spelling, invalid, blocked):
    cfg_path.write_text(json.dumps({spelling: invalid}), encoding="utf-8")
    before = cfg_path.read_bytes()
    with pytest.raises(ValueError, match="object or null"):
        set_skill_blocked("alpha", blocked, config_path=cfg_path)
    assert cfg_path.read_bytes() == before