feat: config-driven contract + concurrent-writer safety (rebased on main)#202
Merged
Merged
Conversation
Addresses the deferred review findings from PR #180/#188: 1. [MAJOR] config.yaml blocks are now read (were declared but never used). New config_loader.py reads config.yaml via PyYAML with stdlib fallback, so debounce windows, status tags, max_step_len, and require_* sets are config-driven instead of hardcoded. Silent config drift eliminated. 2. [MAJOR] Concurrent-writer safety: ledger _append_with_seq() now holds an exclusive fcntl.flock across seq-assignment + write, preventing the _next_seq race and large-entry interleaving between the recommender (record) and executor (transition) agents. fsync per write. POSIX-only; no-op fallback on non-fcntl platforms. 3. [MINOR] Status validation: transition() now raises ValueError on unknown statuses (e.g. 'assiged' typo) via VALID_STATUSES, so a typo can't silently break de-dup. Tests: 14/14 passing (added 4: config-overrides-max_step_len, config-overrides-debounce, transition-rejects-unknown-status, concurrent-writers-do-not-lose-entries with 8x25 parallel writes).
Owner
Author
🤖 DRC Agent AnalysisRecommendation: 🟠 P1 IMPORTANT Summary: Config Schema Validation Layer (Dreamer ID 2) as recommended by REALIST, with Merge & Harden In-Place (Dreamer ID 1) as prerequisite Next steps:
Strategic fit: Consulting: high · Product: high · Tech debt: reduces Analysed by GadgetLab DRC Agent (Dreamer → Realist → Critic) · Run |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the autopilot recommendation pipeline behave according to autopilot/config.yaml (instead of hardcoded constants) and adds file-level locking to make the decisions ledger safe under concurrent writers, plus validation to prevent unknown status typos from silently breaking de-duplication.
Changes:
- Added
autopilot/config_loader.pyand wired config-driven values into the contract validator, ledger debounce policy, and message status tags. - Implemented POSIX
fcntl.flock-based exclusive locking for seq assignment + JSONL appends to prevent concurrent write races. - Added status validation (
VALID_STATUSES) sotransition()rejects unknown lifecycle statuses.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| autopilot/config_loader.py | New cached loader with defaults + optional PyYAML parsing to drive runtime behavior from config.yaml. |
| autopilot/decisions/ledger.py | Adds file locking + fsync per append; moves debounce windows and ledger path to config; rejects unknown statuses. |
| autopilot/recommendation_contract.py | Reads validation rules (max step length / required fields by severity) from config instead of hardcoding. |
| autopilot/message_formatter.py | Uses config-driven status tags (with defaults) when rendering Slack messages. |
| autopilot/tests/test_recommendation_contract.py | Adds regression tests for config overrides, unknown status rejection, and concurrent writer safety. |
| autopilot/README.md | Updates module listing to reflect new config loader and ledger locking behavior. |
Comment on lines
44
to
48
| @@ -46,11 +48,14 @@ def _load(path: str = DEFAULT_LEDGER_PATH) -> list[dict]: | |||
| with open(path, encoding="utf-8") as f: | |||
Comment on lines
+21
to
25
| # Fallback tags if config is unavailable; live values come from config.yaml. | ||
| _STATUS_TAG_FALLBACK = { | ||
| "open": "🟡 Open", | ||
| "assigned": "🔵 Assigned", | ||
| "inflight": "🟠 In-flight", |
Comment on lines
82
to
85
| # P0 must have an owner; else withhold + escalate | ||
| if r.severity == "P0" and r.owner == "unassigned": | ||
| if r.severity in require_owner_for and r.owner == "unassigned": | ||
| errs.append("P0 with no owner — withhold, escalate to #morning-digest") | ||
|
|
Comment on lines
+149
to
+169
| def test_config_overrides_max_step_len(): | ||
| """validate() should honour max_step_len from a custom config file.""" | ||
| import config_loader | ||
|
|
||
| config_loader.reset_cache() | ||
| cfg = tempfile.NamedTemporaryFile( | ||
| mode="w", suffix=".yaml", delete=False, encoding="utf-8" | ||
| ) | ||
| cfg.write("message_contract:\n max_step_len: 20\n") | ||
| cfg.close() | ||
| try: | ||
| config_loader.DEFAULT_CONFIG_PATH = cfg.name | ||
| config_loader.reset_cache() | ||
| r = _good() | ||
| r.steps[0] = "x" * 25 # over the custom 20 limit, under default 120 | ||
| ok, errs = validate(r) | ||
| assert not ok | ||
| assert any("step 1" in e and "20" in e for e in errs) | ||
| finally: | ||
| config_loader.reset_cache() | ||
| os.unlink(cfg.name) |
Comment on lines
+176
to
+200
| config_loader.reset_cache() | ||
| cfg = tempfile.NamedTemporaryFile( | ||
| mode="w", suffix=".yaml", delete=False, encoding="utf-8" | ||
| ) | ||
| cfg.write( | ||
| "recommendation_debounce:\n" | ||
| " repost_policy:\n" | ||
| " open: {min_hours_between: 100000, never_repost: false}\n" | ||
| ) | ||
| cfg.close() | ||
| ledger = tempfile.NamedTemporaryFile(suffix=".jsonl", delete=False) | ||
| ledger.close() | ||
| try: | ||
| config_loader.DEFAULT_CONFIG_PATH = cfg.name | ||
| config_loader.reset_cache() | ||
| r = _good() | ||
| from decisions.ledger import record, should_post | ||
|
|
||
| record(r, ledger.name) | ||
| allow, reason = should_post(r, ledger.name) | ||
| assert allow is False, reason # 100000h window always suppresses | ||
| finally: | ||
| config_loader.reset_cache() | ||
| os.unlink(cfg.name) | ||
| os.unlink(ledger.name) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Same work as #189, superseded because the previous branch had diverged from main and could not be fast-forwarded. This branch is built on the current tip of main (no conflicts).
What it does
Closes out the major/minor findings the dual code review flagged but deferred from #188.
1. Config drift eliminated (major)
config.yamlblocks were declared but never read —ledger.py/recommendation_contract.py/message_formatter.pyhardcoded everything. Newconfig_loader.pyreadsconfig.yamlvia PyYAML (stdlib fallback), so debounce windows, status tags,max_step_len, andrequire_*severity sets are now config-driven.2. Concurrent-writer safety (major)
_append_with_seq()holds an exclusivefcntl.flockacross seq-assignment + write, withfsyncper write. POSIX-only; no-op fallback elsewhere.3. Status validation (minor)
transition()raisesValueErroron unknown statuses (e.g.assigedtypo) viaVALID_STATUSES.Verification (run on this branch)
PYTHONPATH=autopilot python3 autopilot/tests/test_recommendation_contract.py→ 14/14 passedpytest tests/unit→ 1001 passed, 0 failedSupersedes #189.