Skip to content

feat(skills): add a lifecycle and drift audit for the skill bank - #776

Merged
HugoXK merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:feat/skill-bank-lifecycle
Nov 10, 2026
Merged

HugoXK merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:feat/skill-bank-lifecycle

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #775.

What changed

Add agentdescent skills report|audit|search and the SkillBank module: a
durable outcome ledger (uses/successes/failures), a bank-relative lift
audit that reports insufficient below --min-uses and quarantines below
--min-lift (read-only unless --apply), active-only retrieval through the
existing ArtifactGraph, and explicit retire/activate states. It does not
generate skills or decide acceptance; it owns the lifecycle after a skill has
been admitted, and makes drift visible instead of letting a bad skill keep being
retrieved.

Tip SHA: b0a3022
Ran:

  • pytest -q -n 12 (whole suite) -> 21 failed, 10 errors; 20 of those
    failures + all 10 errors are identical on unchanged main 27c43ee4dc03
    (no docker daemon: python:3.11-slim cannot be pulled; host sandbox and
    uninstall cases). The one extra, test_era_algotune.py::test_the_reference_scores_about_one_through_the_sandbox,
    passes serially at this tip.
  • pytest tests/test_skill_bank.py -> 8 passed
  • python -m tools.gen_api_docs --check -> pass
  • python -m tools.changelog_fragments check -> pass (63 fragments)
  • mkdocs build --strict -> pass
    Not run:
  • a real generated skill library -> the lifecycle is tested on synthetic outcome ledgers; no model call is involved

Review fixes at this tip

  • SkillBank._load checks isinstance(payload, Mapping) before .get, so a
    JSON array/null store is SkillBankError/exit 2 instead of AttributeError;
    a regression test covers [], null and a scalar.
  • Docstring store name is skills.json; unused imports are gone.
  • Docs note that retire/activate are library calls in v1 and that
    baseline includes quarantined/retired outcomes.

HugoXK added a commit to HugoXK/agentdescent that referenced this pull request Nov 5, 2026
@HugoXK HugoXK added area:cli The agentdescent command line (cli.py, doctor, plan/evolve/status/apply) enhancement New feature or request labels Nov 5, 2026
@HugoXK
HugoXK force-pushed the feat/skill-bank-lifecycle branch from ac38978 to c83f498 Compare November 5, 2026 07:24
@HugoXK
HugoXK requested a review from Birfy November 5, 2026 07:24
@github-actions github-actions Bot added the risk:T3 Risk tier T3 (core loop, release, process; needs Danyang's click), set by the controller label Nov 5, 2026
Birfy
Birfy previously requested changes Nov 5, 2026

@Birfy Birfy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED
Tip SHA: c83f498
Ran:

  • PYTHONPATH=. pytest tests/test_skill_bank.py tests/test_docs_links.py tests/test_changelog_fragments.py -q → pass
  • python -m tools.changelog_fragments check → pass
  • python -m tools.gen_api_docs --check → pass
  • local repro of non-object store payloads ([], null) → AttributeError escapes CLI
    Not run:
  • full matrix CI (test 3.9–3.13 still queued on this tip after ~20m; build already green)
  • mkdocs build --strict

Summary

Solid skill-bank slice that matches #775: durable outcome ledger, bank-relative lift audit (read-only / --apply), active-only ArtifactGraph search, explicit retire/activate on the library, CLI skills report|audit|search, docs + changelog + nav. Tests cover the acceptance list well. Controller labeled risk:T3 (path would be T2; size 610 lines > T2 250-line cap).

Blocker

SkillBank._load on this tip moved payload.get("schema_version", …) before the isinstance(payload, Mapping) guard. A JSON array or null store now raises bare AttributeError instead of SkillBankError, and cmd_skills only catches (SkillBankError, OSError, ValueError), so the CLI crashes with a traceback instead of exit 2.

# current (unsafe if payload is list/None)
version = int(payload.get("schema_version", 1))
if not isinstance(payload, Mapping) or version != SKILL_BANK_SCHEMA_VERSION:
    ...

Please check isinstance(payload, Mapping) first (the previous short-circuit form was safe), and add a regression test for a non-object JSON store (e.g. [] / null) asserting CLI exit 2 / ok: false. The schema_version string → -1 hardening is fine once the Mapping check is first.

Nits (non-blocking)

  • Module docstring example uses SkillBank("skills.jsonl") but the store is an atomic JSON snapshot, not JSONL — align the example with skills.json.
  • Unused imports: Iterable, List in skill_bank.py.
  • retire / activate exist on the library and are described in docs, but there is no CLI verb yet — fine for v1; a one-liner note in the docs Boundaries section would set expectations.
  • baseline() folds quarantined/retired outcomes into the bank rate; worth a sentence in docs so callers know a large quarantined cohort can drag the bar.

Once the Mapping-order fix + test land on a new tip, happy to re-review. T3 → merge still needs Danyang's click after PASS+green.

@HugoXK
HugoXK force-pushed the feat/skill-bank-lifecycle branch from c83f498 to 72b4ba5 Compare November 5, 2026 08:12
@HugoXK
HugoXK requested a review from Birfy November 5, 2026 08:12

@Birfy Birfy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip SHA: 72b4ba5
Ran:

  • pytest tests/test_skill_bank.py -q → green
  • ad-hoc: JSON [] / null store → SkillBankError (not AttributeError)
    Not run:
  • full matrix
  • mkdocs build --strict
    Verdict: PASS

Prior CR on c83f498 addressed (Mapping guard before schema_version). risk:T3 — awaiting Danyang click.

@Birfy
Birfy dismissed their stale review November 5, 2026 08:30

Superseded by PASS on tip 72b4ba5.

@HugoXK HugoXK added the awaiting-approval Green T2/T3 PR: waits for a PASS at its tip and Danyang's merge click label Nov 5, 2026
@HugoXK

HugoXK commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

@Birfy All required checks are green and the latest review is PASS at 72b4ba59. Ready for the owner merge click; awaiting-approval is set.

@HugoXK
HugoXK force-pushed the feat/skill-bank-lifecycle branch from 72b4ba5 to fc87a53 Compare November 5, 2026 09:33
@HugoXK

HugoXK commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main 4c0515b2; combined the traces and skills CLI/parser blocks. Targeted skill-bank + traces tests pass. Please re-review.

@Birfy Birfy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Tip SHA: fc87a53

The rebase onto 4c0515b2 dropped main's tenant report subcommand (added in #740) from agentdescent/cli.py. git diff origin/main...HEAD -- agentdescent/cli.py removes the elif action == "report" branch in cmd_tenant, its printer block, and the tenant_sub.add_parser("report", ...) parser, and puts back the old error string "tenant needs init, plan or run". On this tip agentdescent tenant report --help fails with invalid choice: 'report' (choose from init, plan, run). The same 31 deleted lines appear identically in #772, #776, #778, #780 and #782, so it looks like one shared conflict resolution kept the pre-#740 side.

The feature diff itself matches the previously PASSed tip; only the conflict resolution regressed. Please restore the tenant report code from main (merge origin/main and keep both sides), and it would be worth adding a CLI-level test for tenant report so this can't silently regress again (tests/test_persistent_tenancy.py / tests/test_tenant_platform.py still pass without it).

Ran: interdiff vs previous PASS tip; git diff origin/main...HEAD --stat; agentdescent tenant report --help on #782's tip; pytest tests/test_persistent_tenancy.py tests/test_tenant_platform.py (pass, they don't cover the CLI).
Not run: full suite. Note: GitHub Actions hasn't fired on any PR since the repo moved to ThinkFlowLab, so there is no CI on this tip.

@HugoXK

HugoXK commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

Restored the tenant report subcommand from current main 4c0515b2 and kept the feature block. Targeted tenant + feature + traces tests pass. Please re-review fee7481f55.

Birfy
Birfy previously approved these changes Nov 6, 2026

@Birfy Birfy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: PASS
Tip SHA: fee7481

Prior CR (tenant report dropped on rebase) is fixed. tenant report parser/handler match main; agentdescent tenant report --help works. Feature suite still green.

Ran:

  • git diff origin/main...HEAD -- agentdescent/cli.py (tenant report intact)
  • agentdescent tenant report --help → OK
  • pytest tests/test_skill_bank.py -q → 8 passed
    Not run: full matrix; mkdocs build --strict.

risk_gate → T3 (size 625 > T2 cap). Do not auto-merge; awaiting Danyang click / 「都合了」. Actions still dead — no fresh CI on this tip.

@Birfy

Birfy commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

Retrigger CI after Actions re-enabled for ThinkFlowLab/agentdescent.

@Birfy Birfy closed this Nov 6, 2026
@Birfy Birfy reopened this Nov 6, 2026
@Birfy

Birfy commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

@HugoXK This PR is CONFLICTING against current main (7651fb9c after recent merges). Please update the branch from main (merge preferred) and push so CI can re-run. Thanks!

@HugoXK

HugoXK commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator Author

Rebased onto main 426a1f54; required checks (build + test 3.9–3.13) are green at 09a3ec0904. Risk gate: T3 — ready for @Birfy's explicit merge click.

Birfy
Birfy previously approved these changes Nov 6, 2026

@Birfy Birfy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: PASS

Tip SHA: 09a3ec0

Rebase onto main 426a1f54. Feature modules unchanged vs prior PASS tip fee7481f5591; only cli.py / mkdocs.yml picked up main. tenant report intact. Required CI green (build + test 3.9–3.13); mergeState UNSTABLE from stale cancelled reviewed tip fail only. Prior CR was on an older tip and does not apply here.

Ran:

  • python3 .github/scripts/risk_gate.py --repo ThinkFlowLab/agentdescent --pr 776 → T3 (623 lines / 6 files > T2)
  • pytest tests/test_skill_bank.py -q → 8 passed
  • agentdescent skills --help / tenant report --help → OK
    Not run:
  • real generated skill library (lifecycle tested on synthetic ledgers)
  • mkdocs build --strict

risk:T3 + awaiting-approval → no squash-merge; waiting Danyang click / 「都合了」.

@Birfy

Birfy commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

@HugoXK This PR is CONFLICTING again against current main (5e41c68c after #780 expand-vs-evaluate landed). Please update from main (merge preferred) and push so CI can re-run. Thanks!

@Birfy

Birfy commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

@HugoXK This PR is CONFLICTING again against current main (3ec07362 after #795/#796/#797). Please update from main (merge preferred) and push so CI can re-run. Thanks!

@Birfy

Birfy commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

@HugoXK This PR is CONFLICTING again against current main (20689124 after #798 ReflectiveFusion landed). Please update from main (merge preferred) and push so CI can re-run. Thanks!

@Birfy

Birfy commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

@HugoXK This PR is CONFLICTING again against current main (7fdbfa57 after #800 and later lands). Please update from main (merge preferred) and push so CI can re-run. Thanks!

@Birfy

Birfy commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

@HugoXK This PR is CONFLICTING again against current main (b7da5e36 after #811/#815 landed). Please update from main (merge preferred) and push so CI can re-run. Thanks!

@Birfy

Birfy commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator

@HugoXK This PR is CONFLICTING again against current main (f1192800 after design-debt merge wave #810–#815). Please update from main (merge preferred) and push so CI can re-run. Thanks!

@Birfy

Birfy commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator

@HugoXK This PR is CONFLICTING again against current main (5a56eb5b after #816+#826). Please update from main (merge preferred) and push so CI can re-run. Thanks!

@Birfy

Birfy commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator

@HugoXK This PR is CONFLICTING again against current main (59ba1460 after design-debt + #830). Please update from main (merge preferred) and push so CI can re-run. Thanks!

@HugoXK

HugoXK commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main (91c936a6); required checks (build + test 3.9–3.13) green at 9a28dcd6. Local tests/test_skill_bank.py: 8 passed. The previous PASS is void at the new tip; reviewed tip is report-only. Ready for a fresh PASS and Danyang's merge click.

@Birfy

Birfy commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS

Tip SHA: 9a28dcd

Rebase onto main 91c936a6. git diff main...HEAD is +623/-0 over 6 files. skill_bank.py, docs/skill-bank.md, the changelog fragment and tests/test_skill_bank.py are byte-identical to the prior PASS tip 09a3ec09; cli.py / mkdocs.yml only picked up main. tenant report intact. Local tests/test_skill_bank.py: 8 passed. Required CI green (build + test 3.9–3.13).

risk_gate: T3 (size 623 lines) — not merging; waiting for Danyang's click.

@HugoXK
HugoXK force-pushed the feat/skill-bank-lifecycle branch from 9a28dcd to b0a3022 Compare November 9, 2026 09:09
@HugoXK HugoXK added awaiting-approval Green T2/T3 PR: waits for a PASS at its tip and Danyang's merge click and removed awaiting-approval Green T2/T3 PR: waits for a PASS at its tip and Danyang's merge click labels Nov 9, 2026
@HugoXK

HugoXK commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

Maintenance after #772 merged: rebased onto main (27c43ee4); required checks green at b0a30224. Local tests/test_skill_bank.py: 8 passed. Ready for a fresh PASS and the merge click.

@HugoXK HugoXK removed the awaiting-approval Green T2/T3 PR: waits for a PASS at its tip and Danyang's merge click label Nov 9, 2026
@Birfy

Birfy commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS (tip b0a3022)

Rebased onto main 27c43ee (after #772 merged). Feature diff is byte-identical to the previously PASSed tip 9a28dcd (+623/-0, 6 files). Required CI green on this tip, merge state CLEAN. risk_gate: T3 (size) — waiting for owner click; not merged by automation.

@HugoXK HugoXK added the awaiting-approval Green T2/T3 PR: waits for a PASS at its tip and Danyang's merge click label Nov 9, 2026
@HugoXK
HugoXK merged commit 1aaf1bd into ThinkFlowLab:main Nov 10, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:cli The agentdescent command line (cli.py, doctor, plan/evolve/status/apply) awaiting-approval Green T2/T3 PR: waits for a PASS at its tip and Danyang's merge click enhancement New feature or request risk:T3 Risk tier T3 (core loop, release, process; needs Danyang's click), set by the controller

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Skill bank lifecycle and drift audit

2 participants