fix(config): keep skill block from writing a second skillForge key

[LivXue]

Summary

raven skill block could leave a config file that Raven then refuses to load.

set_skill_blocked wrote data.setdefault("skillForge", {}) unconditionally. Dict keys
match byte for byte, but the schema treats skillForge and skill_forge as one field
(alias_generator=to_camel with populate_by_name), so a config file spelling the block
skill_forge gained a second block instead of having its own one patched. RavenConfig
forbids extra inputs, so load_raven_config binds the alias and rejects the leftover field
name: raven agent, raven gateway and the TUI RPC server all then refuse to start.

raven status keeps working throughout, because the base loader pops extension keys before
validating. That asymmetry is what makes the defect quiet: the command most people reach for
to check a config is the one that cannot see it.

The same misread has two softer faces, both closed by the same change. raven skill unblock
became a silent no-op on such a config, reporting success while the skill stayed blocked;
and blocking an already-listed skill reported it as newly blocked. Both read the blocklist
out of the empty block the call had just created.

The fix probes the spelling the file already carries and reuses it. That is not a new shape:
loader.py already does exactly this for the skillRouter and everosSkillLight
migrations, and set_sentinel_nudge_quota does it for sentinel.nudge_policy. Among the
top-level blocks a config writer creates, skillForge is the only one whose camelCase and
snake_case spellings differ at all.

The second commit closes what the first one opened. Making the written key conditional made
two lines that report the write false on a snake_case file: the log line, and the result
line of raven skill block and raven skill unblock, both of which named
skillForge.blocklist unconditionally. Measured on a skill_forge config, the write landed
on skill_forge.blocklist while both messages still said skillForge.blocklist, sending an
operator to grep their own config for a key it does not carry. They now report the setting
rather than the file key, which is what set_sentinel_nudge_quota already does: it respects
the same two spellings and logs sentinel nudge quota patched with no key path at all. The
--help text and the module docstring keep naming skillForge.blocklist, because they
describe the command rather than the write.

Checked and deliberately not fixed, with reasons:

• A config already corrupted by the old code does not heal. Running raven skill block
  again now selects the camelCase block and leaves the snake_case one in place, so such a
  file still needs a hand edit. Merging the two blocks automatically is lossy and ambiguous,
  and belongs in a migration argued on its own terms rather than inside a fix whose job is
  to stop producing the state.
• update_cron_config writes to_camel(key) unconditionally, which is the same shape one
  level down, on field keys rather than block keys. Its failure is milder and different:
  CronConfig does not forbid extras, so a file carrying both default_timezone and
  defaultTimezone loads with the camelCase one winning. Measured directly. Nothing breaks,
  but a later hand edit of the snake_case key is silently dead. Pre-existing, a different
  failure mode, and in another config surface.
• The three operator-blocklist refusals in raven/agent/tools/skill_hub.py name
  skillForge.blocklist in text the model reads. These were already inaccurate for a
  hand-written skill_forge config before this branch, since the loader has always accepted
  both spellings on the read side. Not introduced here, and fixing them widens the diff into
  the agent tool surface.

Type

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

Verification

Run on Linux with Python 3.12.13, on a branch rebased onto the current main (which now
carries the A2A work). The rebase conflicted only in tests/test_config_update.py, where
both sides appended a section at the end of the file; both sections were kept. The rebased
commit's diffstat is byte for byte the pre-rebase one, +70/-1 over two files, so the
resolution neither absorbed nor dropped anything.

• uv run --frozen --all-extras pytest tests/test_config_update.py -> 47 passed
• uv run --frozen --all-extras pytest tests/ -k "config or skill_forge or skill_block" ->
  1709 passed, 33 skipped, 0 failed
• make lint-python -> ruff check clean, 2008 files already formatted
• make lint-imports -> 10 contracts kept, 0 broken
• make lint-deps -> no dependency issues
• make lint-types -> all checks passed
• make check-commits, make check-large-files, make check-source-language -> exit 0

Neither commit's tests are theatre, and each was checked on its own. Reverting only the
first commit's production hunk turns 4 of its 5 tests red: duplicate key,
load_raven_config ValidationError, unblock no-op, and blocklist read from the wrong block.
The fifth covers camelCase behaviour that predates the fix and correctly stays green.
Restoring the message wording the second commit replaced turns its new CLI regression red on
the skillForge assertion, and restoring the fix turns it green.

End to end with an isolated RAVEN_HOME: a config carrying the duplicate block makes
raven agent exit with ValidationError: skill_forge Extra inputs are not permitted. The
same starting config driven through the fixed raven skill block gets past config
validation and reaches the expected missing-API-key error, with the user's existing
auto_install setting preserved beside the new blocklist.

Malformed blocks were checked too. skillForge: null beside a skill_forge table used to
raise AttributeError and now works. skillForge: null alone, and skillForge set to a
string, still raise AttributeError, unchanged from before.

File mode was measured rather than assumed, because the new A2A onboarding narrows
config.json to 0600 and this branch writes to the same file: after
initialize_a2a_server, a raven skill block leaves the file at 0600, since
atomic_update preserves the mode it finds.

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

make lint also runs lint-ui, which fails in a fresh worktree with ERR_MODULE_NOT_FOUND
because ui-web/node_modules is not installed there. This change touches no JS or TS file,
so the Python lint targets above are the gates that cover it. No user-facing docs were
needed; the helper's docstring states the casing contract.

Risk

Behaviour changes only for a config file that spells the block skill_forge. For a file
using the camelCase spelling, which is what onboarding writes, the bytes written are
identical to before.

Two changes are visible to a snake_case user, and both are the point of the fix: the
blocklist is written into their existing section rather than into a new one, and
raven skill unblock now actually removes an entry instead of reporting success and doing
nothing.

One change is visible to every user regardless of spelling: the result line of
raven skill block and raven skill unblock now reads Skill blocklist = [...] where it
read skillForge.blocklist = [...], and the corresponding log line drops the key path the
same way. This is wording only; no value or exit code changes, and the --help text still
names skillForge.blocklist.

Rollback is a revert of the two commits. No migration runs, so nothing has to be undone on
disk. Reverting only the second leaves the first in place with the messages inaccurate on a
snake_case config, so revert both or neither.

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

No permissions, credentials or file modes are changed; the 0600 measurement above is the
check behind that sentence.

Related Issues

N/A

[gloryfromca]

Blocking: replace the malformed Claude model ID in the commit trailer

The code change itself looks correct, but the commit records Co-authored-by: Claude (claude-opus-5[1m]) .... The [1m] suffix is terminal-formatting residue, not the actual model ID required by AGENTS.md section 3.3, and squash merging would preserve incorrect contribution provenance. Please rewrite that trailer with the actual current-session model ID.

I reviewed the full diff; the set_skill_blocked callers; raw-config loading, migration, and RavenConfig alias behavior; relevant history; backward compatibility for camelCase and snake_case files; the added tests for weakening; and the applicable AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and architecture constraints. I found no code-level issue. uv run pytest tests/test_config_update.py tests/test_cli_skill_commands.py -q passed: 55 tests in 9.18s.

[LivXue]

Thanks for the depth here -- the callers, the migration paths and the alias behaviour were
the right things to pull on, and the check for weakened tests is the one I would most want
a second pair of eyes on.

On the trailer I disagree, and I would rather show the checks than assert it. [1m] is not
terminal-formatting residue: it is part of the model ID this session reports, denoting the
1M-context variant.

1. It cannot be ANSI. ANSI SGR bold is ESC[1m -- an ESC byte followed by [1m, with
no closing bracket:

$ python3 -c "print('\x1b[1m'.encode()); print('[1m]'.encode())"
b'\x1b[1m'
b'[1m]'

The trailer carries a literal [1m], closing bracket included and no ESC byte. The two are
structurally different strings, so terminal residue is not a mechanism that can produce it.

2. It is already an established spelling on main, not a novel malformation:

$ git log origin/main --format='%(trailers:key=Co-authored-by,valueonly)' -400 \
    | grep -o 'Claude ([^)]*)' | sort | uniq -c | sort -rn
     53 Claude (claude-opus-5)
     16 Claude (claude-fable-5)
     13 Claude (claude-fable-5-1)
      5 Claude (claude-opus-5[1m])
      1 Claude (claude-sonnet-4-6)

3. Removing the suffix would introduce the misattribution the rule exists to prevent.
claude-opus-5 and claude-opus-5[1m] are different variants, and the 53 commits above use
the first for sessions that actually ran it. Writing claude-opus-5 on this commit would
attribute the work to a model that did not do it, which is the failure AGENTS.md 3.3 is
guarding against rather than a fix for it.

The repo's own gates accept the string as written: make check-commits passes locally, and
the commit messages job passes on this head.

So I have left the trailer as it stands rather than rewriting history for it, which also
keeps to the fix-on-next-commit convention this project settled on for trailer corrections.

If you still read the bracketed form as wrong, I would rather we change the convention than
this one commit: name the spelling you want for the 1M variant and I will use it here and
from now on. What I want to avoid is dropping the suffix, since that silently renames the
model rather than correcting the format.

[gloryfromca]

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

You are right, and I withdraw my trailer objection. I incorrectly treated the visible [1m] text as ANSI residue. An SGR sequence requires the ESC byte and does not include the closing bracket present here; I also verified that github/main contains five commits using the same claude-opus-5[1m] spelling. Together with your confirmation that this is the session-reported 1M variant ID, the trailer satisfies the provenance rule rather than violating it.

The code review had no other findings, so my blocker is fully lifted. I reran uv run pytest tests/test_config_update.py tests/test_cli_skill_commands.py -q: 55 passed in 6.15s.

[gloryfromca]

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

I reviewed the rebased full diff and used range-diff to separate the new main history from PR-owned changes. The original config fix is preserved; the new commit correctly makes the CLI and log wording casing-neutral without changing values or exit behavior, and its snake_case regression test exercises the operator-visible failure it prevents.

Coverage included the diff, the skill block/unblock callers, config loader and alias behavior, commit history and rebase resolution, camelCase backward compatibility, test-strength changes, and the applicable AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and architecture constraints. There are no review threads to resolve. uv run pytest tests/test_config_update.py tests/test_cli_skill_commands.py -q passed: 68 tests in 12.55s.

[0xKT]

Not a blocker. The fix is right and its own tests pin it: reverting the
if/elif/else at raven/config/update.py:225-230 back to base's single
setdefault("skillForge", {}) turns five tests red, and flipping only the else
branch turns four red, so none of the five new tests is vacuous. Three
observations, none of which holds the PR. Read on head 4af8cb0 against base
0cb8b79.

1. Two of the three reworded strings are pinned by nothing

The PR changes three user-facing strings away from skillForge.blocklist:
skill_commands.py:143 (Blocked), skill_commands.py:153 (Unblocked), and the
logger.info format at update.py:247-248. Only the first is held to that by a
test. grep -c Unblocked over both touched test files returns 0/0, and nothing
in either file asserts on the log format, so reverting either of the other two
leaves the suite green -- measured: reverting :143 gives 1 failed, 20 passed,
reverting :153 gives 21 passed, reverting the logger format gives 68 passed.

Two thirds of this change's own point can be undone later without a red test,
and the PR wrote the rule down itself, at
tests/test_cli_skill_commands.py:214: "The result line must not name a key
this write did not use." The unblock line is a result line of exactly that kind,
and it reaches the same key-selection branch -- on a snake_case config the write
lands on skill_forge.blocklist either way.

The cheap close: test_skill_unblock_removes_from_config
(tests/test_cli_skill_commands.py:205-211) already invokes the command and
already holds r. One added line -- assert "skillForge" not in r.stdout --
pins it.

2. The block-selection rule now has three copies and still no owner

The new block at update.py:225-230 is the same six-line decision as
raven/config/loader.py:1171-1176, character for character apart from
indentation:

if isinstance(data.get("skillForge"), dict):
    sf_key = "skillForge"
elif isinstance(data.get("skill_forge"), dict):
    sf_key = "skill_forge"
else:
    sf_key = "skillForge"

and loader.py:1090-1095 is a third, spelled "skillForge" in data and isinstance(data["skillForge"], dict). The two spellings are equivalent, so this
is not a behaviour divergence today -- it is the same rule written three times in
two files, which is how divergence starts. update.py:163 solves the identical
problem once more for a different block (np_key = "nudge_policy" if "nudge_policy" in sentinel else "nudgePolicy"), under a docstring that already
says "writing a second casing for a field already present would duplicate it".

I do not think a bug fix has to carry the extraction, which is why this is an
observation rather than a finding. But the count is now four hand-written
answers to one question and no function anywhere under raven/config/ that owns
it, so the next writer will write a fifth. The test helper at
tests/test_cli_skill_commands.py:188 (data.get("skillForge", {})) is already
a small instance of the same thing -- the new snake-case test cannot use it and
reads data["skill_forge"]["blocklist"] inline instead.

3. The new guard reads as if it handles a non-dict value, and does not

update.py:231 is still data.setdefault(sf_key, {}). When the key is present
but not a mapping -- {"skillForge": null} -- both isinstance branches are
false, sf_key falls to the else, setdefault returns the existing None, and
section.get("blocklist") raises AttributeError. The loader accepts that config
(a null block reads as absent), so the command is reachable on a file that loads
cleanly.

This predates the PR: base's data.setdefault("skillForge", {}) fails the same
way on the same input, and the PR neither introduced it nor claimed to fix it.
It is worth one line only because the code that now stands there is an
isinstance check, which reads like it covers exactly this case. If you touch
it, the sibling at update.py:163 shows the other shape -- a presence test plus
a value the code can rely on.