Skip to content

fix(observability): let the sink write to the file it read #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rldyourmnd merged 1 commit into from
Aug 14, 2026
Merged

fix(observability): let the sink write to the file it read #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 141 additions & 0 deletions .agent-runtime/goals/held-path-identity.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
{
"schema_version": "v1alpha1",
"revision": 11,
"goal": {
"id": "held-path-identity",
"intent": "The JSONL sink writes to the file it read, and every remaining check-then-use gap is stated rather than implied",
"acceptance": [
{
"id": "jsonl-single-open",
"acceptance": "The sink scans and appends through one descriptor",
"status": "complete",
"evidence": [
{
"type": "file",
"reference": "observability/sink_jsonl.go OpenJSONLSink; readJSONLFile",
"result": "One descriptor, opened O_RDWR|O_APPEND, validated and then read from and appended to."
}
]
},
{
"id": "stated-boundary",
"acceptance": "The security model names the remaining check-then-use gaps and what exploiting them requires",
"status": "complete",
"evidence": [
{
"type": "file",
"reference": "SECURITY.md",
"result": "Workspace and executable check-then-use are named, with what an actor needs and why executable_path records the inspected file."
}
]
},
{
"id": "revisit-trigger",
"acceptance": "The deferral records the decision that would re-open it",
"status": "complete",
"evidence": [
{
"type": "file",
"reference": "SECURITY.md",
"result": "States that executing commands which are not fully trusted makes both real, and names descriptor-anchored traversal as the fix."
}
]
}
],
"non_goals": [
"Descriptor-anchor workspace traversal in this pass",
"Redesign the sandbox model",
"Defend against a compromised host"
],
"state": "active",
"current_phase": "closure",
"receipts": {
"completeness_omission_audit": {
"phase": "completeness_omission_audit",
"summary": "Checked the other direction as well: a destination whose name resolves elsewhere, such as a symlink, is refused rather than followed, because a caller configured a destination and not whatever currently answers to it.",
"evidence": [
{
"type": "test",
"reference": "observability TestJSONLRefusesAPathThatResolvedElsewhere",
"result": "A symlinked destination is rejected."
}
],
"recorded_at": "2026-08-14T23:50:11.199764768Z"
},
"execute": {
"phase": "execute",
"summary": "Split readJSONL into a path form and a descriptor form, opened the sink once with O_RDWR|O_APPEND|O_CREATE, validated that descriptor, and read the history from it. Removed the now-dead scanJSONL wrapper.",
"evidence": [
{
"type": "file",
"reference": "observability/sink_jsonl.go",
"result": "One open for the sink's lifetime; the remaining os.Open is the separate read-only ReplayJSONL entry point."
}
],
"recorded_at": "2026-08-14T23:50:11.112800073Z"
},
"gap_plan": {
"phase": "gap_plan",
"summary": "Hold one descriptor for the sink's lifetime and read the history from it. Leave workspace and executable resolution as they are, and state them in the security model: both need concurrent write access to the machine already running the Task, which the stated trust boundary excludes.",
"evidence": [
{
"type": "file",
"reference": "SECURITY.md trust boundary",
"result": "Manifests and their commands are trusted reviewed inputs, so a party who can rewrite a PATH directory can do so with or without a race."
}
],
"recorded_at": "2026-08-14T23:50:11.081411822Z"
},
"orient": {
"phase": "orient",
"summary": "Three surfaces validate a pathname and act on it later. The JSONL sink was the only one where the split is a plain correctness defect rather than a race against an actor: it opened the path to scan, closed it, and opened it again to append, so recovered state and writes could describe different files with no attacker involved beyond a rename.",
"evidence": [
{
"type": "file",
"reference": "observability/sink_jsonl.go OpenJSONLSink",
"result": "scanJSONL opened and closed the path, then os.OpenFile opened it again; the SameFile check proved path equals descriptor at the second open, not that it was the file scanned."
}
],
"recorded_at": "2026-08-14T23:50:11.050697736Z"
},
"reconcile": {
"phase": "reconcile",
"summary": "The behavioural test first written to cover this did not discriminate: it renamed the path after the sink was open, which both the old and the new code survive because both held a descriptor by then. Its comment was corrected to say what it actually pins.",
"evidence": [
{
"type": "test",
"reference": "observability TestJSONLAppendsToTheFileItRead",
"result": "It fails if the single-descriptor construction is ever undone, and it does not claim to reproduce the original window."
}
],
"recorded_at": "2026-08-14T23:50:11.142761962Z"
},
"self_review": {
"phase": "self_review",
"summary": "The window itself is closed by construction rather than by a test. Reproducing it from outside would need a hook that exists only to be hooked, which is the kind of control this repository is trying to remove, so the limitation is stated instead of dressed up.",
"evidence": [
{
"type": "file",
"reference": "observability/sink_jsonl_test.go",
"result": "The test comment states plainly that it does not reproduce the defect it accompanies."
}
],
"recorded_at": "2026-08-14T23:50:11.171830052Z"
},
"verify": {
"phase": "verify",
"summary": "gofmt, go vet, staticcheck, full tests under umask 002, race, and all six contract checkers.",
"evidence": [
{
"type": "test",
"reference": "local gate transcript 2026-08-15",
"result": "All green."
}
],
"recorded_at": "2026-08-14T23:50:11.227703174Z"
}
},
"created_at": "2026-08-14T23:50:11.022035863Z",
"updated_at": "2026-08-14T23:50:11.311924048Z"
}
}
30 changes: 27 additions & 3 deletions .agent-runtime/goals/runtime-bounds-truth.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"schema_version": "v1alpha1",
"revision": 14,
"revision": 15,
"goal": {
"id": "runtime-bounds-truth",
"intent": "Task and Goal contracts enforce the bounds and outcomes they declare",
Expand Down Expand Up @@ -82,9 +82,33 @@
"Sandbox untrusted commands",
"Add Windows support"
],
"state": "active",
"state": "completed",
"current_phase": "closure",
"receipts": {
"closure": {
"phase": "closure",
"summary": "Merged as PR #56 with all four required checks green.",
"evidence": [
{
"type": "link",
"reference": "https://github.com/NDDev-it-com/agent-runtime/pull/56",
"result": "Merged to main."
}
],
"recorded_at": "2026-08-14T23:45:44.259561919Z",
"closure": {
"achieved_outcome": "The Task and Goal contracts enforce the bounds and outcomes they declare, and the wall-clock ceiling is stated rather than implied.",
"cleanup": "Task branch deleted locally and on origin.",
"remaining": [],
"next_work": [
{
"type": "issue",
"reference": "held path identity",
"result": "Close the JSONL sink's split between the file it reads and the file it writes, and state the remaining check-then-use gaps rather than implying they are closed."
}
]
}
},
"completeness_omission_audit": {
"phase": "completeness_omission_audit",
"summary": "Owning the process group also removed an undocumented overshoot: a 300ms timeout returned at 2.302s because the orphaned descendant held the inherited pipes for the full WaitDelay. It now returns at 301ms. The grace period is documented rather than removed, because a terminated process still needs bounded time to release its pipes.",
Expand Down Expand Up @@ -171,6 +195,6 @@
}
},
"created_at": "2026-08-14T23:40:47.471824358Z",
"updated_at": "2026-08-14T23:41:15.508130592Z"
"updated_at": "2026-08-14T23:45:44.259561919Z"
}
}
17 changes: 17 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,23 @@ contract.

### Fixed

- A JSONL sink holds one descriptor for its lifetime. It used to open the path
to scan the existing history, close it, and open it again to append, so a
rename in between left the recovered duplicate-identity and size state
describing the first file while every write went to the second. A destination
whose name no longer resolves to the object just opened is refused rather than
followed.

### Changed

- The security model records the two check-then-use gaps that remain rather than
implying they are closed. Workspace resolution and executable resolution both
validate a pathname and act on it a moment later, so an actor with concurrent
write access to the running machine can move the used object away from the
checked one — which also means `executable_path` records what was inspected.
Both sit outside the stated trust boundary, and the entry says what would make
them real and what the fix would be if it ever changes.

- Malformed evidence can no longer be staged on a pending checklist item.
`Validate` checked evidence only on completed items, but acceptance evidence
is append-only for every item, so a bad record staged on a pending one could
Expand Down
29 changes: 27 additions & 2 deletions SECURITY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,28 @@ caller. `Result.executable_path` records the file that ran.
Use an external sandbox for untrusted code and provide only short-lived,
least-privilege credentials.

### Paths validated and paths used

Two runtime checks resolve a pathname and act on it a moment later rather than
on the object they examined. Workspace resolution returns a validated string
that `Open`, `Stat` and the child's working directory consume afterwards, and a
command name is resolved by inspecting a candidate file that `exec` opens
later. An actor able to rename or replace a directory or an executable in that
interval can move the used object away from the checked one — so
`Result.executable_path` records what was inspected, which under such a race is
not necessarily what ran.

Both require concurrent write access to the machine already running the Task,
which is outside the trust boundary above: manifests and their commands are
trusted, reviewed inputs, and a party who can rewrite a directory on the PATH
can do so with or without a race. They are recorded here rather than closed
because stating a gap is worth more than a control that does not remove it.

This holds only while that trust boundary does. If this runtime is ever used to
execute commands that are not fully trusted, these become real and the fix is
descriptor-anchored traversal and execution — the mechanism the JSONL sink and
the release publisher already use.

Avoid placing secrets in manifests, instructions, command arguments, logs, or
issue reports. Captured command output may contain sensitive data and should be
handled accordingly by callers.
Expand Down Expand Up @@ -73,7 +95,10 @@ are re-evaluated key by key, bounded, copied, and JSON encoded; custom errors an
stringers are never invoked. Redaction metadata contains only typed reasons and
aggregate counts, not paths, hashes, lengths, or removed values.

JSONL paths are trusted caller configuration. Existing files must be regular,
owner-only, supported-version canonical JSONL. Symlinks, partial records,
JSONL paths are trusted caller configuration. A sink opens its destination once
and holds that descriptor for its lifetime, so the history it recovers and the
records it appends describe one object; a name that no longer resolves to the
object just opened is refused rather than followed. Existing files must be
regular, owner-only, supported-version canonical JSONL. Symlinks, partial records,
duplicates, unsupported versions, oversize files, and corruption fail closed.
The sink does not encrypt data or protect against a compromised host.
46 changes: 32 additions & 14 deletions observability/sink_jsonl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,34 @@ func OpenJSONLSink(path string, options JSONLOptions) (*JSONLSink, error) {
if maxBytes < MaxEnvelopeBytes || maxBytes > MaximumMaxFileBytes {
return nil, &SinkError{Code: SinkFailure}
}
history, err := scanJSONL(path, maxBytes)
if err != nil {
return nil, err
}
file, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600)
// One descriptor for the whole life of the sink. O_APPEND puts every write
// at the end regardless of where reading left the offset, so the history
// this sink trusts and the file it extends are the same object. Opening to
// scan and reopening to append let a rename between the two split them:
// duplicate-identity and size state would describe the file that was read
// while the writes went somewhere else.
file, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_RDWR, 0o600)
if err != nil {
return nil, &SinkError{Code: SinkUnavailable, Retryable: true}
}
info, statErr := os.Lstat(path)
openedInfo, openedErr := file.Stat()
if statErr != nil || openedErr != nil || !info.Mode().IsRegular() || info.Mode().Perm()&0o077 != 0 || !os.SameFile(info, openedInfo) {
if openedErr != nil || !openedInfo.Mode().IsRegular() || openedInfo.Mode().Perm()&0o077 != 0 {
_ = file.Close()
return nil, &SinkError{Code: SinkFailure}
}
// The name must still resolve to the object now held. A path that resolved
// elsewhere is refused rather than followed, because a caller configured a
// destination, not whatever currently answers to it.
named, namedErr := os.Lstat(path)
if namedErr != nil || !os.SameFile(named, openedInfo) {
_ = file.Close()
return nil, &SinkError{Code: SinkFailure}
}
history, err := readJSONLFile(file, maxBytes, nil)
if err != nil {
_ = file.Close()
return nil, err
}
return &JSONLSink{name: options.Name, file: file, syncEveryWrite: options.SyncEveryWrite, ids: history.ids, maxFileBytes: maxBytes, size: history.size}, nil
}
func newJSONLSinkWriter(name string, file syncWriteCloser, syncEveryWrite bool) (*JSONLSink, error) {
Expand Down Expand Up @@ -156,27 +170,31 @@ type jsonlHistory struct {
size int64
}

// scanJSONL validates an existing history without retaining its envelopes.
func scanJSONL(path string, maxBytes int64) (jsonlHistory, error) {
return readJSONL(path, maxBytes, nil)
}

// readJSONL is the single definition of a valid JSONL history: canonical
// single-line envelopes, a newline-terminated final record, unique event
// identity, and a strictly increasing sequence within each subject stream. The
// sink applies it before appending and ReplayJSONL applies it before restoring,
// so one file can never be acceptable to append to yet impossible to replay.
// When collect is non-nil every decoded envelope is appended to it in file order.
func readJSONL(path string, maxBytes int64, collect *[]Envelope) (jsonlHistory, error) {
history := jsonlHistory{ids: map[string]bool{}, sequences: map[string]uint64{}}
file, err := os.Open(path)
if os.IsNotExist(err) {
return history, nil
return jsonlHistory{ids: map[string]bool{}, sequences: map[string]uint64{}}, nil
}
if err != nil {
return jsonlHistory{}, &SinkError{Code: SinkUnavailable, Retryable: true}
}
defer file.Close()
return readJSONLFile(file, maxBytes, collect)
}

// readJSONLFile reads the history of an already-open file. Taking the
// descriptor rather than the path is what lets the sink validate and then
// append to one object: reopening by name between the two left the recovered
// identity and size describing the file that was read while every append went
// to whatever the name resolved to afterwards.
func readJSONLFile(file *os.File, maxBytes int64, collect *[]Envelope) (jsonlHistory, error) {
history := jsonlHistory{ids: map[string]bool{}, sequences: map[string]uint64{}}
info, err := file.Stat()
if err != nil {
return jsonlHistory{}, &SinkError{Code: SinkUnavailable, Retryable: true}
Expand Down
Loading